Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions packages/cli/src/commands/review/lib/authorization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,12 +354,12 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): {
}

const verdict = parseReviewArgs(raw, { comment: req.defaultComment });
if (!verdict.comment.effective) {
// The refusal must name the REAL blocker. When comment was requested —
// by the flag or the standing `review.comment` setting — but the target
// is not a PR, effective is false because the arguments name no pull
// request to bind the write to; blaming a missing `--comment` flag the
// operator never typed (and implying typing one would fix it) misdirects.
if (!verdict.comment.effective && verdict.topology !== 'minimal') {
// When comment was requested — by the flag or the standing
// `review.comment` setting — but the target is not a PR, effective is
// false because the arguments name no pull request to bind the write to;
// blaming a missing `--comment` flag the operator never typed (and
// implying typing one would fix it) misdirects.
const commentRequested =
verdict.comment.requested || req.defaultComment === true;
return {
Expand All @@ -371,6 +371,12 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): {
`(${JSON.stringify(raw.trim())})`,
};
}
// A minimal record falls through to the binding checks below: the refusal
// must name the REAL blocker, and the topology is it only when it is the
// SOLE one — "re-run the review without it" cannot lift a refusal that a
// non-PR target, or another PR's number, repo, or host, still holds, and
// leading with the topology sends the operator to re-run into the same
// refusal with the binding blocker still unnamed.

const t = verdict.target;
const authorisedPr =
Expand Down Expand Up @@ -430,6 +436,19 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): {
}
}

if (!verdict.comment.effective) {
// Minimal, and bound to this write on every axis above — the parser

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The reorder makes the gate shape-sensitive — A/B probe verified: at the round-3 anchor cba3a2966c the same minimal record answered the topology refusal under every caller shape; at this commit it flips to the host/repo-binding refusals on the second caller's shape — yet no test exercises a minimal record through publish-assets' call shape (repo optional, host env-resolved including GH_HOST, absentHostFollowsRecording omitted so an absent host reads as a github.com claim): topology appears zero times in publish-assets.test.ts, whose only refusal-wording gate cases are non-minimal.

The gate's own header says "one function, two callers, one lesson-ledger", but this round's lesson is test-pinned for one caller only: a future refusal-ordering refactor — the exact edit class of this round; round 3's ordering was itself wrong and had to be reordered — regresses minimal handling on the publish-assets shape (wrong blocker named, or the evidence-image advice wrapper misapplied) while every suite stays green.

Mirror the new submit tests in publish-assets.test.ts, matching the existing run(...) / writeFileSync(argsFile, …) shape:

it('a fully-bound minimal record names the topology (evidence-images advice)', () => {
  writeFileSync(argsFile, '8346 --topology minimal --comment\n');
  run({ files: [pngFile('a.png')] });
  expect(process.exitCode).toBe(3);
  const why = (stderrSpy.mock.calls.map((c) => c[0]) as string[]).join(' ');
  expect(why).toContain('`--topology minimal`');
});

plus a wrong-host record without --reviewed-repo, expecting the host-binding refusal and not the topology wording.

中文说明

重排使门控变得「形状敏感」——A/B 探针已验证:在第 3 轮锚点 cba3a2966c 上,同一条 minimal 记录在所有调用者形状下都回答 topology 拒绝;而在本提交上,在第二调用者的形状下翻转为 host/仓库绑定拒绝——然而没有任何测试让 minimal 记录走一遍 publish-assets 的调用形状(repo 可选、host 经环境解析含 GH_HOST、省略 absentHostFollowsRecording 因而缺省 host 读作 github.com 主张):publish-assets.test.tstopology 出现零次,其仅有的拒绝措辞门控用例均为非 minimal。

门控自己的头部注释写着「一个函数,两个调用者,一本教训台账」,但本轮的教训只为其中一个调用者钉了测试:未来一次拒绝排序重构——正是本轮的编辑类别;第 3 轮的排序本身就是错的、不得不重排——会在 publish-assets 形状上使 minimal 处理回归(点名错误的阻塞,或误用证据图片建议包装),而所有套件保持绿色。

请在 publish-assets.test.ts 中仿照新增的 submit 测试,按既有 run(...) / writeFileSync(argsFile, …) 形态补上(见上方代码块),另加一个不带 --reviewed-repo 的错误 host 记录用例,断言得到 host 绑定拒绝而非 topology 文案。

— qwen3.8-max via Qwen Code /review (v0.22.0)

// forced effective false, so the topology is now the sole blocker, and
// its remedy lifts the refusal.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This comment states two invariants unconditionally — "the topology is now the sole blocker, and its remedy lifts the refusal" — that are false for the no-comment-source sub-case this same block serves. The final if (!verdict.comment.effective) fires for any minimal record bound on every axis, including one with no --comment flag and no review.comment setting — the neither case the new test explicitly routes through this block. For that record topology is a blocker but not the sole one, and the remedy does not lift the refusal: executed at HEAD, re-running record 123 answers "--comment was not in the review arguments". The new test's own comment is careful — topology is "STILL the blocker to name" there — exactly where this code comment overclaims.

A maintainer of this refusal-wording-critical gate trusts this comment six months later: they "simplify" the neither test branch away as contradicting the documented invariant, or diagnose an operator's "did exactly what the message said, refused again" report as a gate bug instead of the expected second blocker.

Suggested change
// Minimal, and bound to this write on every axis above — the parser
// forced effective false, so the topology is now the sole blocker, and
// its remedy lifts the refusal.
// Minimal, and bound to this write on every axis above. When a comment
// source was recorded, the parser forced effective false, so the
// topology is the sole blocker and its remedy lifts the refusal; when
// none was, the topology is still the blocker to name — even a typed
// --comment would not lift the refusal while minimal stands.
中文说明

该注释无条件地断言了两个不变量——「topology 现在是唯一阻塞,且其补救能解除拒绝」——但对同一代码块所服务的「无 comment 来源」子情形并不成立。末尾的 if (!verdict.comment.effective)任何在各轴上都绑定的 minimal 记录都会触发,包括既没有 --comment 标志、也没有 review.comment 设置的记录——即新测试显式经过此块的 neither 用例。对该记录而言,topology 是一个阻塞而非唯一阻塞,且其补救无法解除拒绝:已在 HEAD 上执行验证,重跑记录 123 得到 "--comment was not in the review arguments"。新测试自己的注释很谨慎——那里写的是 topology 「仍是要点名的阻塞」——而这正是本代码注释过度断言之处。

六个月后,维护这个「拒绝措辞攸关」门控的维护者会信任这段注释:他们要么把 neither 测试分支当作与文档化不变量矛盾而「顺手简化」掉,要么把操作者「我完全照做了提示却再次被拒」的报告误诊为门控 bug,而不是预期中的第二个阻塞。

(建议修复:把注释限定为实际不变量,见上方代码块。)

— qwen3.8-max via Qwen Code /review (v0.22.0)

return {
ok: false,
why:
`the review arguments (${JSON.stringify(raw.trim())}) ran with ` +
'`--topology minimal`, which is terminal-only and cannot authorise ' +
'posting — re-run the review without it',
Comment on lines +474 to +475

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This new topology refusal is a third refusal class, but its only shape-sensitive consumer — the advice dispatch at submit.ts:746 (auth.why.includes('--comment was')) — still knows only two classes. The refusal string contains no such substring, so the dispatch selects the second arm, whose preamble "Nothing recorded authorises binding this target" is false for this class — the record DID name the target and bound every axis (that is why it reached the topology block at all) — and whose remedy "a review invoked naming it" re-refuses while the topology is kept: the futile-retry loop the dispatch's own comment block exists to prevent.

Aggravating: that arm's other remedy — "--user-authorized after the user has asked" — mechanically succeeds against the same record (fast path ok: true observed at HEAD), so the advice appended to this refusal actively points at the bypass of that very refusal.

Fix in submit.ts — a third dispatch arm keyed on the topology marker, checked before the existing branch, whose advice restates the refusal's own remedy (re-run without --topology minimal) and does not offer --user-authorized.

中文说明

这个新的 topology 拒绝是第三个拒绝类别,但它唯一的「按措辞分流」的消费者——submit.ts:746 的建议分发(auth.why.includes('--comment was'))——仍然只认识两个类别。该拒绝字符串不含该子串,因此分发落入第二个分支:其前言「没有任何记录授权绑定此目标」对本类别是错误的——记录确实指名了目标并在各轴上完成绑定(正因如此才走到 topology 块)——其补救「用指名该目标的评审重跑」在保留 topology 时会再次被拒:正是分发自己的注释块所要防止的「徒劳重试」循环。

更糟的是:该分支的另一个补救——「用户已要求后用 --user-authorized」——对同一条记录在机制上会成功(已在 HEAD 观察到快速路径返回 ok: true),因此附加在这条拒绝之后的建议,恰恰指向了绕过该拒绝本身的通路。

请在 submit.ts 中修复:新增以 topology 标记为键的第三个分发分支,置于既有分支之前检查,其建议复述拒绝自身的补救(去掉 --topology minimal 重跑),且不提供 --user-authorized

— qwen3.8-max via Qwen Code /review (v0.22.0)

};
}

return {
ok: true,
why: verdict.comment.requested
Expand Down
206 changes: 206 additions & 0 deletions packages/cli/src/commands/review/parse-args.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,199 @@ describe('parseReviewArgs — --severity-floor (the convergence posture knob)',
});
});

describe('parseReviewArgs — --topology (the minimal-prompt A/B arm)', () => {
it('defaults to auto: the standing effort-driven pipeline', () => {
const got = parseReviewArgs('6711');
expect(got.topology).toBe('auto');
expect(got.topologySource).toBe('default');
});

it('parses both forms case-insensitively; the last valid occurrence wins', () => {
expect(parseReviewArgs('6711 --topology minimal')).toMatchObject({
topology: 'minimal',
topologySource: 'explicit',
});
expect(parseReviewArgs('6711 --topology=Minimal')).toMatchObject({
topology: 'minimal',
});
expect(
parseReviewArgs('6711 --topology minimal --topology auto'),
).toMatchObject({ topology: 'auto', topologySource: 'explicit' });
});

it('an explicit --topology auto is explicit, not the default', () => {
const got = parseReviewArgs('6711 --topology auto');
expect(got.topology).toBe('auto');
expect(got.topologySource).toBe('explicit');
});

it('selecting minimal does not change the target', () => {
expect(parseReviewArgs('6711 --topology minimal').target).toEqual({
type: 'pr-number',
number: 6711,
});
expect(parseReviewArgs('src/foo.ts --topology minimal').target).toEqual({
type: 'file',
path: 'src/foo.ts',
});
});

it('minimal gates --comment: terminal-only, posts nothing', () => {
const got = parseReviewArgs('6711 --topology minimal --comment');
expect(got.comment.requested).toBe(true);
expect(got.comment.effective).toBe(false);
expect(
got.warnings.some(
(w) => w.includes('`--comment`') && w.includes('terminal-only'),
),
).toBe(true);
});

it('minimal gates --fix: terminal-only, edits nothing', () => {
const got = parseReviewArgs('src/foo.ts --topology minimal --fix');
expect(got.fix.requested).toBe(true);
expect(got.fix.effective).toBe(false);
expect(
got.warnings.some(
(w) => w.includes('`--fix`') && w.includes('terminal-only'),
),
).toBe(true);
});

it('minimal gates --resume: a fresh single pass cannot continue an interrupted run', () => {
// The third flag the minimal arm gates: an effective resume would make
// `fetch-pr --resume` consume an interrupted pipeline run's lease and
// worktree for a pass that never continues it — destroying resumable
// state instead of either continuing or leaving it alone.
const got = parseReviewArgs('6711 --topology minimal --resume');
expect(got.resume.requested).toBe(true);
expect(got.resume.effective).toBe(false);
expect(
got.warnings.some(
(w) => w.includes('`--resume`') && w.includes('--topology minimal'),
),
).toBe(true);
});

it('an invalid value warns naming what is in effect, and never eats the target', () => {
const got = parseReviewArgs('--topology minial 6711');
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
expect(got.topology).toBe('auto');
expect(
got.warnings.some(
(w) =>
w.includes('Invalid --topology value "minial"') &&
w.includes('default topology'),
),
).toBe(true);
});

it('an invalid equals-form value warns instead of vanishing', () => {
const got = parseReviewArgs('6711 --topology=minial');
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
expect(got.topology).toBe('auto');
expect(
got.warnings.some((w) => w.includes('Invalid --topology value "minial"')),
).toBe(true);
});

it('a sole invalid value becomes the target, and the warning says so', () => {
const got = parseReviewArgs('--topology minial');
expect(got.target).toEqual({ type: 'file', path: 'minial' });
expect(
got.warnings.some(
(w) =>
w.includes('Invalid --topology value "minial"') &&
w.includes('treating it as the review target'),
),
).toBe(true);
});

it('a PR-shaped value is rescued as the target, not discarded', () => {
// `--topology 6711` (forgot the value) must review PR 6711, not silently
// fall back to the local diff — the same rescue --effort/--severity-floor get.
const got = parseReviewArgs('--topology 6711');
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
expect(got.topology).toBe('auto');
});

it('minimal does not force effort the way --comment does', () => {
// minimal is terminal-only, so the comment-forces-high rule never fires;
// a local target's effort stays at its default.
const got = parseReviewArgs('src/foo.ts --topology minimal');
expect(got.effort).toBe('medium');
expect(got.effortSource).toBe('default');
});

it('the equals form rescues a PR-shaped value exactly as the spaced form does', () => {
// Sibling probes pin this for --effort/--severity-floor (the round-8
// regression); the topology copy must not diverge. Deleting the
// equals-form rescue branch reviews the local tree instead of PR 6711.
expect(parseReviewArgs('--topology=6711').target).toEqual({
type: 'pr-number',
number: 6711,
});
});

it('a quoted-empty value is consumed as missing, never an empty-string target', () => {
// Deleting the consumption branch leaves '' as the sole candidate, and
// it classifies as an empty-string file target.
const bare = parseReviewArgs('--topology ""');
expect(bare.target).toEqual({ type: 'local' });
expect(
bare.warnings.some((w) => w.includes('--topology requires a value')),
).toBe(true);

const afterTarget = parseReviewArgs('6711 --topology ""');
expect(afterTarget.target).toEqual({ type: 'pr-number', number: 6711 });
expect(
afterTarget.warnings.some((w) =>
w.includes('--topology requires a value'),
),
).toBe(true);
});

it('flag-final or flag-followed is a missing value, never a consumed flag', () => {
// Deleting the branch eats the following token into the kept pool, so
// `--comment` never registers.
const flagFinal = parseReviewArgs('6711 --topology');
expect(flagFinal.target).toEqual({ type: 'pr-number', number: 6711 });
expect(flagFinal.topology).toBe('auto');
expect(
flagFinal.warnings.some((w) => w.includes('--topology requires a value')),
).toBe(true);

const followed = parseReviewArgs('6711 --topology --comment');
expect(followed.target).toEqual({ type: 'pr-number', number: 6711 });
expect(followed.comment.requested).toBe(true);
expect(followed.topology).toBe('auto');
expect(
followed.warnings.some((w) => w.includes('--topology requires a value')),
).toBe(true);
});

it('minimal gates the review.comment setting too, and the warning names it', () => {
// The suppression gate is written over the SETTING-OR-FLAG request, so a
// settings-driven comment is gated exactly like a flagged one — pinning
// `effective: false` here witnesses the gate itself: narrowing it to the
// flag alone would let the terminal-only arm post while every flag-based
// test stays green. And the warning must name the setting, not a flag
// the operator never typed — the forced-by-comment warning makes the
// same distinction.
const got = parseReviewArgs('6711 --topology minimal', { comment: true });
expect(got.comment.effective).toBe(false);
expect(
got.warnings.some(
(w) =>
w.includes('`review.comment` setting') && w.includes('terminal-only'),
),
).toBe(true);
expect(got.warnings.some((w) => w.includes('`--comment` is ignored'))).toBe(
false,
);
});
});

describe('parseReviewArgs — settings-provided defaults', () => {
it('applies the configured effort when --effort is absent', () => {
const got = parseReviewArgs('6711', { effort: 'medium' });
Expand Down Expand Up @@ -1155,6 +1348,19 @@ describe('parseArgsCommand wiring', () => {
expect(written).toBe(String(vi.mocked(writeStdoutLine).mock.calls[0][0]));
});

it('--topology minimal survives the stdin → yargs → handler path', async () => {
// The flag must reach the printed verdict through the real handler, not
// just the pure function: a wiring drop leaves every pure-function test
// green while real `/review … --topology minimal` runs the full pipeline.
fsState.stdin = '6711 --topology minimal --comment\n';
await runCli(['parse-args', '--stdin']);
const got = printedVerdict();
expect(got.target).toEqual({ type: 'pr-number', number: 6711 });
expect(got.topology).toBe('minimal');
expect(got.topologySource).toBe('explicit');
expect(got.comment).toEqual({ requested: true, effective: false });
});

// The real CLI nests this command under `review`, which changes what
// yargs puts in argv._ (['review', 'parse-args'] instead of
// ['parse-args']) — the smuggle guard once read that command path as
Expand Down
Loading
Loading