diff --git a/packages/cli/src/commands/review/lib/authorization.ts b/packages/cli/src/commands/review/lib/authorization.ts index 49b7695be44..52166f87d2f 100644 --- a/packages/cli/src/commands/review/lib/authorization.ts +++ b/packages/cli/src/commands/review/lib/authorization.ts @@ -264,6 +264,18 @@ function lookupRecordedHost( return { host: undefined, unbound: false }; } +/** + * The structural class of a write refusal. The advice at the submit call + * site branches on THIS, never on the refusal text: `why` embeds the + * operator's verbatim recorded arguments (JSON.stringify of the raw + * record), and any marker string can itself appear inside that quoted + * record — text that embeds operator input cannot classify itself. + */ +export type ReviewWriteRefusalClass = + | 'topology' + | 'comment-not-requested' + | 'unbound'; + /** * Exactly three things authorise a public write, and all are facts rather than * impressions: `--comment` in the arguments the user typed (re-parsed from the @@ -275,6 +287,12 @@ function lookupRecordedHost( export function reviewWriteAuthorization(req: WriteAuthorizationRequest): { ok: boolean; why: string; + /** + * The refusal class — present on every refusal, absent on success. See + * `ReviewWriteRefusalClass` for why the caller branches on this instead + * of matching `why`. + */ + cls?: ReviewWriteRefusalClass; /** * The host the recorded target names, when it names one: a pr-url target * carries it; a bare pr-number supplies a recorded `--host` flag or none. @@ -343,6 +361,7 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): { // a plain re-run of the review fixes that — typing `--comment` does not. return { ok: false, + cls: req.defaultComment === true ? 'unbound' : 'comment-not-requested', why: req.defaultComment === true ? `no review arguments were recorded at ${path}, so no recorded ` + @@ -354,16 +373,17 @@ 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 { ok: false, + cls: commentRequested ? 'unbound' : 'comment-not-requested', why: commentRequested ? `the review arguments (${JSON.stringify(raw.trim())}) do not name a ` + 'pull request, so they cannot authorise posting to one' @@ -371,6 +391,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 = @@ -378,6 +404,7 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): { if (authorisedPr === undefined) { return { ok: false, + cls: 'unbound', why: `the review arguments (${JSON.stringify(raw.trim())}) do not name a ` + 'pull request, so they cannot authorise posting to one', @@ -386,6 +413,7 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): { if (authorisedPr !== req.pr) { return { ok: false, + cls: 'unbound', why: `the review arguments authorise pull request #${authorisedPr}, but ` + `this submission targets #${req.pr}`, @@ -397,6 +425,7 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): { if (authorisedRepo.toLowerCase() !== req.repo.toLowerCase()) { return { ok: false, + cls: 'unbound', why: `the review arguments authorise ${authorisedRepo}, but this ` + `submission targets ${req.repo}`, @@ -423,6 +452,7 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): { if (!hostUnasserted && !hostsEquivalent(t.host.toLowerCase(), writeHost)) { return { ok: false, + cls: 'unbound', why: `the review arguments authorise ${t.host}, but this submission ` + `targets ${req.host ?? 'github.com'}`, @@ -430,6 +460,22 @@ export function reviewWriteAuthorization(req: WriteAuthorizationRequest): { } } + if (!verdict.comment.effective) { + // 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. + return { + ok: false, + cls: 'topology', + 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', + }; + } + return { ok: true, why: verdict.comment.requested diff --git a/packages/cli/src/commands/review/parse-args.test.ts b/packages/cli/src/commands/review/parse-args.test.ts index cccf52d4a5d..f2cac5aae37 100644 --- a/packages/cli/src/commands/review/parse-args.test.ts +++ b/packages/cli/src/commands/review/parse-args.test.ts @@ -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' }); @@ -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 diff --git a/packages/cli/src/commands/review/parse-args.ts b/packages/cli/src/commands/review/parse-args.ts index 07564c73340..17b7a77f5e5 100644 --- a/packages/cli/src/commands/review/parse-args.ts +++ b/packages/cli/src/commands/review/parse-args.ts @@ -39,6 +39,20 @@ export type ReviewEffort = 'low' | 'medium' | 'high'; */ export type ReviewSeverityFloor = 'critical' | 'suggestion'; +/** + * The review topology: which shape the run takes. `auto` is the standing + * effort-driven pipeline (the 3A/3B/3C fan-outs). `minimal` is the A/B + * comparison arm from issue #9783 — a single careful senior-engineer pass + * over the diff in the orchestrator's own context, at most fifteen findings, + * each carrying a concrete failure scenario; no subagent fan-out, no + * verification, no reverse audit, no posting. It exists so the full pipeline + * and the minimal prompt can be run over the same PR set and compared per + * model. It is deliberately orthogonal to `effort` — it is a different + * _shape_ of review, not a depth of the same one — and selecting it skips the + * agent machinery (roster, coverage, budget) entirely rather than shrinking it. + */ +export type ReviewTopology = 'auto' | 'minimal'; + export type ReviewTarget = | { type: 'pr-number'; number: number } | { @@ -111,6 +125,14 @@ export interface ParsedReviewArgs { */ severityFloor: ReviewSeverityFloor | 'auto'; severityFloorSource: 'explicit' | 'configured' | 'default'; + /** + * The review topology. `auto` (the default) runs the standing effort-driven + * pipeline; `minimal` runs the single-pass A/B arm and, because it neither + * posts, edits, nor continues an interrupted pipeline run, forces + * `comment.effective`, `fix.effective`, and `resume.effective` to false. + */ + topology: ReviewTopology; + topologySource: 'explicit' | 'default'; /** The `--host` flag's value, when present — recorded verbatim so the * write gate can bind a recorded bare-number target's platform (the * target itself carries no host in that spelling). */ @@ -174,6 +196,14 @@ export const SEVERITY_FLOORS: ReadonlySet = new Set([ 'auto', ]); +export const TOPOLOGIES: ReadonlySet = new Set([ + 'minimal', + // `auto` is a legal EXPLICIT value for the same reason it is for + // `--severity-floor`: typing `--topology auto` means "the standing + // effort-driven pipeline", not a typo to reject. + 'auto', +]); + // The verdict's owner/repo/number are interpolated into `gh` commands by the // caller, so they must be established trustworthily, not merely extracted: // the scheme is case-insensitive, the number must END at the path segment @@ -220,6 +250,11 @@ function asSeverityFloor(value: string): ReviewSeverityFloor | 'auto' | null { : null; } +function asTopology(value: string): ReviewTopology | null { + const lower = value.toLowerCase(); + return TOPOLOGIES.has(lower) ? (lower as ReviewTopology) : null; +} + /** * Single-dash tokens count as flags too: `-c` is never a plausible review * target, and classifying it as a file path demoted the real target the @@ -354,6 +389,7 @@ export function parseReviewArgs( let resumeRequested = false; let explicitEffort: ReviewEffort | null = null; let explicitFloor: ReviewSeverityFloor | 'auto' | null = null; + let explicitTopology: ReviewTopology | null = null; let recordedHostFlag: string | undefined; // The configured default gets the same validation as an explicit flag: @@ -402,6 +438,8 @@ export function parseReviewArgs( // deferred-warning problem; its issues are a separate list because its // resolution sentence is its own. const floorIssues: EffortIssue[] = []; + // `--topology` shares the value-token grammar too, for the same reason. + const topologyIssues: EffortIssue[] = []; // First pass: pull out flags (and each value-taking flag's value token, // when the spaced form legitimately consumes one). Non-flag tokens are kept @@ -410,7 +448,7 @@ export function parseReviewArgs( interface Kept { token: string; /** Set when this token arrived as an invalid value of the named flag. */ - invalidValueOf?: '--effort' | '--severity-floor'; + invalidValueOf?: '--effort' | '--severity-floor' | '--topology'; } const kept: Kept[] = []; @@ -528,6 +566,40 @@ export function parseReviewArgs( continue; } + if (token === '--topology' || token.startsWith('--topology=')) { + if (token.includes('=')) { + const value = token.slice(token.indexOf('=') + 1); + const topologyValue = asTopology(value); + if (topologyValue !== null) { + explicitTopology = topologyValue; + } else if (value !== '' && isPrShapedToken(value)) { + kept.push({ token: value, invalidValueOf: '--topology' }); + } else { + topologyIssues.push({ kind: 'invalid-eq', value }); + } + continue; + } + const next = i + 1 < tokens.length ? tokens[i + 1] : undefined; + const nextTopology = next !== undefined ? asTopology(next) : null; + if (nextTopology !== null) { + explicitTopology = nextTopology; + i++; + continue; + } + if (next === '') { + topologyIssues.push({ kind: 'missing' }); + i++; + continue; + } + if (next === undefined || isFlag(next)) { + topologyIssues.push({ kind: 'missing' }); + continue; + } + kept.push({ token: next, invalidValueOf: '--topology' }); + i++; + continue; + } + if (isFlag(token)) { unknownFlags.push(token); warnings.push(`Unrecognized flag ${JSON.stringify(token)}; ignored.`); @@ -668,10 +740,18 @@ export function parseReviewArgs( (k) => k.invalidValueOf !== undefined && isPrUrlToken(k.token), )?.token : undefined; + // Each flag's deferred-warning list, keyed by the flag an invalid value + // arrived as; resolved inside the guard below, where `invalidValueOf` is + // known to be set. + const issueListFor = { + '--effort': effortIssues, + '--severity-floor': floorIssues, + '--topology': topologyIssues, + }; let rescuedPr = false; for (const k of kept) { - const issues = k.invalidValueOf === '--effort' ? effortIssues : floorIssues; if (k.invalidValueOf !== undefined) { + const issues = issueListFor[k.invalidValueOf]; const survives = isPrShaped(k.token) ? !hasValidCandidate && distinctPr.size === 1 : soleCandidate; @@ -750,31 +830,65 @@ export function parseReviewArgs( const isPr = target.type === 'pr-number' || target.type === 'pr-url'; + // The topology resolves like the effort — an explicit flag beats the + // standing `auto` default. There is no configured (settings) topology: the + // minimal arm is an explicit A/B comparison, never a background default. + const topology: ReviewTopology = explicitTopology ?? 'auto'; + const topologySource: ParsedReviewArgs['topologySource'] = + explicitTopology !== null ? 'explicit' : 'default'; + // The minimal arm is terminal-only — it neither posts to a PR nor edits a + // working tree — so both write operations are gated off it, and so is + // `--resume`: a fresh single pass cannot continue an interrupted pipeline + // run, and letting `fetch-pr --resume` consume that state would destroy a + // run this arm never continues. This keeps the guarantee in code rather + // than in whichever prose the orchestrator reads. + const isMinimal = topology === 'minimal'; + const commentRequested = commentRequestedByFlag || defaults.comment === true; - const commentEffective = commentRequested && isPr; + const commentEffective = commentRequested && isPr && !isMinimal; if (commentRequestedByFlag && !isPr) { warnings.push( 'Warning: `--comment` flag is ignored because the review target is not a PR.', ); + } else if (commentRequested && isPr && isMinimal) { + // Only when minimal is THE reason a would-be-effective comment is + // suppressed: on a non-PR target the comment does not apply anyway, and + // that case keeps its usual handling above. The text names the source + // the request actually came from, the same distinction the + // forced-by-comment warning makes: a setting-driven operator told the + // `--comment` flag is ignored goes hunting a flag they never typed. + warnings.push( + commentRequestedByFlag + ? 'Warning: `--comment` is ignored because `--topology minimal` is terminal-only — the minimal arm posts nothing.' + : 'Warning: the `review.comment` setting is ignored because `--topology minimal` is terminal-only — the minimal arm posts nothing.', + ); } - const resumeEffective = resumeRequested && isPr; + const resumeEffective = resumeRequested && isPr && !isMinimal; if (resumeRequested && !isPr) { warnings.push( 'Warning: `--resume` flag is ignored because the review target is not a PR — only a PR review has interrupted state to continue.', ); + } else if (resumeRequested && isPr && isMinimal) { + warnings.push( + 'Warning: `--resume` is ignored because `--topology minimal` runs a fresh single pass — it neither continues nor consumes an interrupted run.', + ); } // `--fix` edits a working tree, so it needs one that outlives the review. A // PR review's tree is the ephemeral worktree Step 9 removes; a `local` or // `file` review's tree is the user's own checkout. - const fixEffective = fixRequested && !isPr; + const fixEffective = fixRequested && !isPr && !isMinimal; if (fixRequested && isPr) { warnings.push( 'Warning: `--fix` flag is ignored because a PR review runs in an ephemeral ' + 'worktree that is deleted when the review ends — there is no durable tree to ' + 'fix. Use `--comment` to publish the findings instead.', ); + } else if (fixRequested && isMinimal) { + warnings.push( + 'Warning: `--fix` is ignored because `--topology minimal` is terminal-only — the minimal arm edits nothing.', + ); } let effort: ReviewEffort; @@ -918,6 +1032,37 @@ export function parseReviewArgs( ); } + // The topology's deferred warnings, composed now that the resolution is + // final — the same shape as the effort's and the floor's. + const topologyResolution = + topologySource === 'explicit' + ? `--topology ${topology} (the last valid occurrence) is in effect` + : 'using the default topology (auto)'; + for (const issue of topologyIssues) { + switch (issue.kind) { + case 'invalid-eq': + warnings.push( + `Invalid --topology value ${JSON.stringify(issue.value)}; ${topologyResolution}.`, + ); + break; + case 'missing': + warnings.push(`--topology requires a value; ${topologyResolution}.`); + break; + case 'discarded': + warnings.push( + `Invalid --topology value ${JSON.stringify(issue.value)} discarded; ${topologyResolution}.`, + ); + break; + case 'kept-as-target': + warnings.push( + `Invalid --topology value ${JSON.stringify(issue.value)}; treating it as the review target — ${topologyResolution}.`, + ); + break; + default: + break; + } + } + return { target, effort, @@ -926,6 +1071,8 @@ export function parseReviewArgs( fix: { requested: fixRequested, effective: fixEffective }, severityFloor, severityFloorSource, + topology, + topologySource, ...(recordedHostFlag !== undefined ? { host: recordedHostFlag } : {}), resume: { requested: resumeRequested, effective: resumeEffective }, extraTokens, @@ -972,7 +1119,7 @@ function reviewDefaultsFromSettings(): { export const parseArgsCommand: CommandModule = { command: 'parse-args [raw]', describe: - 'Parse the /review skill argument string (--comment, --fix, --resume, --effort, --severity-floor, target disambiguation) and emit the verdict as JSON; pass the string on stdin via --stdin (a positional that begins with a dash never reaches this handler — yargs rejects it as an unknown flag)', + 'Parse the /review skill argument string (--comment, --fix, --resume, --effort, --severity-floor, --topology, target disambiguation) and emit the verdict as JSON; pass the string on stdin via --stdin (a positional that begins with a dash never reaches this handler — yargs rejects it as an unknown flag)', builder: (yargs) => yargs .positional('raw', { diff --git a/packages/cli/src/commands/review/publish-assets.test.ts b/packages/cli/src/commands/review/publish-assets.test.ts index 6f4428b2c2e..92b25748fb7 100644 --- a/packages/cli/src/commands/review/publish-assets.test.ts +++ b/packages/cli/src/commands/review/publish-assets.test.ts @@ -1091,3 +1091,87 @@ describe('publish-assets — host binds even without --reviewed-repo', () => { expect(ghWithInputMock).not.toHaveBeenCalled(); }); }); + +describe('publish-assets — minimal topology at the second caller shape', () => { + // The gate is shape-sensitive: this caller passes an env-resolved host and + // no absentHostFollowsRecording, so an absent host reads as a github.com + // claim. The minimal fall-through must order its refusals the SAME way + // here — the topology names only the sole blocker — or a future reorder + // regresses one caller's shape while the other caller's suite stays green. + let dir: string; + let argsFile: string; + let savedSessionId: string | undefined; + let savedGhHost: string | undefined; + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'publish-assets-min-')); + argsFile = join(dir, 'args.txt'); + process.env['QWEN_REVIEW_ASSETS_REPO'] = 'owner/assets'; + savedSessionId = process.env['QWEN_CODE_SESSION_ID']; + delete process.env['QWEN_CODE_SESSION_ID']; + savedGhHost = process.env['GH_HOST']; + delete process.env['GH_HOST']; + reviewSettingsMock.mockReturnValue({}); + ghMock.mockReset(); + ghWithInputMock.mockReset(); + setGhHostMock.mockReset(); + stderrSpy.mockClear(); + process.exitCode = undefined; + }); + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + delete process.env['QWEN_REVIEW_ASSETS_REPO']; + if (savedSessionId !== undefined) { + process.env['QWEN_CODE_SESSION_ID'] = savedSessionId; + } + if (savedGhHost !== undefined) process.env['GH_HOST'] = savedGhHost; + else delete process.env['GH_HOST']; + process.exitCode = undefined; + }); + + const png = (name: string): string => { + const p = join(dir, name); + writeFileSync(p, Buffer.from('89504e470d0a1a0a', 'hex')); + return p; + }; + const runAt = (): void => + runPublishAssets({ + pr: 8346, + reviewedRepo: undefined, + files: [png('a.png')], + findings: undefined, + findingsOut: undefined, + out: join(dir, 'm.json'), + host: undefined, + userAuthorized: false, + skillArgs: argsFile, + } as never); + + it('a fully-bound minimal record names the topology (evidence-images advice)', () => { + writeFileSync(argsFile, '8346 --topology minimal --comment\n'); + runAt(); + expect(process.exitCode).toBe(3); + const why = (stderrSpy.mock.calls.map((c) => c[0]) as string[]).join(' '); + expect(why).toContain('`--topology minimal`'); + expect(why).toContain('Evidence images'); + expect(ghWithInputMock).not.toHaveBeenCalled(); + }); + + it('a wrong-host minimal record leads with the host binding, not the topology', () => { + // Without --reviewed-repo the gate binds number and host alone; the + // Enterprise-host record fails the host check before the topology + // refusal, exactly as it does at submit's shape — leading with the + // topology here would send the operator to re-run without it into the + // same still-unnamed host refusal. + writeFileSync( + argsFile, + 'https://ghe.corp.example/reviewed/upstream/pull/8346 --topology minimal --comment\n', + ); + runAt(); + expect(process.exitCode).toBe(3); + const why = (stderrSpy.mock.calls.map((c) => c[0]) as string[]).join(' '); + expect(why).toContain('authorise ghe.corp.example'); + expect(why).toContain('targets github.com'); + expect(why).not.toContain('`--topology minimal`'); + expect(ghWithInputMock).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/commands/review/submit-aone.test.ts b/packages/cli/src/commands/review/submit-aone.test.ts index 264fb36aa22..04963ec3364 100644 --- a/packages/cli/src/commands/review/submit-aone.test.ts +++ b/packages/cli/src/commands/review/submit-aone.test.ts @@ -425,6 +425,7 @@ describe('submit posts an authorised Aone target through a1', () => { it('an UNAUTHORISED Aone run takes the normal auth-refusal path first', () => { authMock.mockReturnValue({ ok: false, + cls: 'comment-not-requested', why: '`--comment` was not in the review arguments', }); expect(() => diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index 957dbcff31c..ee99fae8c09 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -584,10 +584,12 @@ describe('authorization — URL-shaped host and repo binding at the submit call expect(bySetting.why).not.toContain( '`--comment` was not in the review arguments', ); + expect(bySetting.cls).toBe('unbound'); const byFlag = authFor('src/foo.ts --comment'); expect(byFlag.ok).toBe(false); expect(byFlag.why).toContain('do not name a'); + expect(byFlag.cls).toBe('unbound'); // Neither source requested it: the original wording stands. const neither = authFor('src/foo.ts'); @@ -595,6 +597,92 @@ describe('authorization — URL-shaped host and repo binding at the submit call expect(neither.why).toContain( '`--comment` was not in the review arguments', ); + expect(neither.cls).toBe('comment-not-requested'); + }); + + it('a minimal-topology record names the topology, not a missing PR', () => { + // `--topology minimal` is a third cause of `comment.effective === false` + // beside the two the refusal wording knew about. The record names its PR + // perfectly, so the target-shape message is factually wrong and sends the + // operator to fix a target problem that does not exist — re-running with + // identical arguments refuses again for the unnamed reason. The topology + // is the REAL blocker; name it. + const byFlag = authFor('123 --topology minimal --comment'); + expect(byFlag.ok).toBe(false); + expect(byFlag.why).toContain('`--topology minimal`'); + expect(byFlag.why).not.toContain('do not name a'); + expect(byFlag.cls).toBe('topology'); + + const bySetting = authFor('123 --topology minimal', { + defaultComment: true, + }); + expect(bySetting.ok).toBe(false); + expect(bySetting.why).toContain('`--topology minimal`'); + expect(bySetting.why).not.toContain('do not name a'); + expect(bySetting.cls).toBe('topology'); + + // No comment source at all: the topology is STILL the blocker to name — + // even a typed --comment would not lift the refusal, so the missing-flag + // wording would bury the fact that the topology bars every post. + const neither = authFor('123 --topology minimal'); + expect(neither.ok).toBe(false); + expect(neither.why).toContain('`--topology minimal`'); + expect(neither.cls).toBe('topology'); + }); + + it('a minimal record names the topology only when it is the sole blocker', () => { + // The topology refusal's remedy is "re-run the review without it" — a + // remedy that cannot lift the refusal while the record ALSO fails to + // bind this write. Lead with the binding refusal there: a non-PR record + // falls through to the target-shape wording, and a PR record naming + // another number, repo, or host leads with the binding mismatch — the + // topology refusal still fires, correctly, once the binding stops + // being a blocker. + const fileTarget = authFor('src/foo.ts --topology minimal --comment'); + expect(fileTarget.ok).toBe(false); + expect(fileTarget.why).toContain('do not name a'); + expect(fileTarget.why).not.toContain('`--topology minimal`'); + expect(fileTarget.cls).toBe('unbound'); + + const wrongPr = authFor('456 --topology minimal --comment'); + expect(wrongPr.ok).toBe(false); + expect(wrongPr.why).toContain('authorise pull request #456'); + expect(wrongPr.why).toContain('targets #123'); + expect(wrongPr.why).not.toContain('`--topology minimal`'); + expect(wrongPr.cls).toBe('unbound'); + + const wrongHost = authFor( + 'https://ghe.corp.example/o/r/pull/123 --topology minimal --comment', + ); + expect(wrongHost.ok).toBe(false); + expect(wrongHost.why).toContain('authorise ghe.corp.example'); + expect(wrongHost.why).toContain('targets github.com'); + expect(wrongHost.why).not.toContain('`--topology minimal`'); + expect(wrongHost.cls).toBe('unbound'); + + const wrongRepo = authFor( + 'https://github.com/x/y/pull/123 --topology minimal --comment', + ); + expect(wrongRepo.ok).toBe(false); + expect(wrongRepo.why).toContain('authorise x/y'); + expect(wrongRepo.why).toContain('targets o/r'); + expect(wrongRepo.why).not.toContain('`--topology minimal`'); + expect(wrongRepo.cls).toBe('unbound'); + }); + + it('the fast path honours the user ask even under minimal (documented layering)', () => { + // posting.md documents the slow/fast contrast this PR's topology refusal + // makes observable on the identical record: the slow path refuses + // ("…ran with `--topology minimal`…"), while the `--user-authorized` + // fast path never consults the topology — the skill's Step 7 rule, not + // the gate, is the layer that catches a minimal run whose decline was + // missed. Pin the fast side here (the slow side is pinned above), so a + // drift in either direction reddens instead of silently rewriting the + // documented layering. + const auth = authFor('123 --topology minimal --comment', { + userAuthorized: true, + }); + expect(auth.ok).toBe(true); }); it('a missing args file names the missing invocation, not a missing flag, when the setting authorises', () => { @@ -619,10 +707,12 @@ describe('authorization — URL-shaped host and repo binding at the submit call 'no recorded invocation names a pull request', ); expect(bySetting.why).not.toContain('`--comment`'); + expect(bySetting.cls).toBe('unbound'); const byFlag = reviewWriteAuthorization(base); expect(byFlag.ok).toBe(false); expect(byFlag.why).toContain('cannot show that `--comment` was requested'); + expect(byFlag.cls).toBe('comment-not-requested'); // Both production callers pass a strict boolean (destructured default / // the resolved setting), so pin the flag branch with the explicit false @@ -636,6 +726,7 @@ describe('authorization — URL-shaped host and repo binding at the submit call expect(byFlagExplicit.why).toContain( 'cannot show that `--comment` was requested', ); + expect(byFlagExplicit.cls).toBe('comment-not-requested'); }); it('surfaces the recorded host on the --user-authorized fast path too', () => { @@ -1532,6 +1623,26 @@ describe('the posting gate', () => { expect(advice()).toContain('invoked naming it'); writeStderrSpy.mockClear(); + // A minimal-topology run: the record bound this target on every axis, + // so the binding arm's "Nothing recorded" preamble is false for it and + // its remedies misdirect — "a review invoked naming it" re-refuses + // while the topology stands, and `--user-authorized` mechanically + // posts what the topology bars. The advice restates the refusal's own + // remedy and nothing else. + runSubmit( + args({ + skillArgs: file( + 'advice-minimal.txt', + '6771 --topology minimal --comment', + ), + }), + ); + expect(advice()).toContain('posts nothing at any effort'); + expect(advice()).toContain('without `--topology minimal`'); + expect(advice()).not.toContain('Nothing recorded authorises binding'); + expect(advice()).not.toContain('--user-authorized'); + writeStderrSpy.mockClear(); + // Nothing recorded at all, with the setting authorising: the refusal // names the missing invocation, and the advice preamble must not // contradict it by presupposing recorded arguments exist. @@ -1545,6 +1656,63 @@ describe('the posting gate', () => { expect(process.exitCode).toBe(3); }); + it('classifies the advice on the refusal class, never on quoted operator text', () => { + // The gate's `why` embeds the operator's verbatim recorded arguments via + // JSON.stringify, and writeSkillArgs records the invocation byte-for-byte + // while tokenizeArgs strips only single/double quotes — so a + // markdown-backticked mention of the topology phrase never parses as the + // flag yet still reaches `why`. Substring-matching it steered this + // missing-`--comment` refusal into the topology arm: claiming the run + // "ran under `--topology minimal`" when it did not, and prescribing a + // re-run without a flag that was never in effect while the real blocker + // stayed unnamed (probe: exit 3, `gh` never called — the write stays + // fail-closed, only the advice class was wrong). The advice keys on the + // gate's structural refusal class instead. + const advice = () => + (writeStderrSpy.mock.calls.map((c) => c[0]) as string[]).join(' '); + + runSubmit( + args({ + skillArgs: file('advice-backtick.txt', '6771 `--topology minimal`'), + }), + ); + expect(advice()).toContain('`--comment` was not in the review arguments'); + expect(advice()).toContain('Re-run with `--comment`'); + expect(advice()).not.toContain('posts nothing at any effort'); + expect(advice()).not.toContain('Nothing recorded authorises binding'); + expect(ghMock).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(3); + }); + + it('a minimal refusal names the remedies that actually authorise posting', () => { + // The canonical minimal invocation records NO comment source, and the + // refusal's own remedy — "re-run the review without it" — makes no + // sufficiency promise. Advice promising posting on the bare re-run alone + // would send the operator straight into the missing-`--comment` refusal: + // the futile retry loop the gate's wording exists to prevent. The arm + // must name what actually authorises the post — `--comment` or the + // `review.comment` setting — whatever comment shape the record carried. + const advice = () => + (writeStderrSpy.mock.calls.map((c) => c[0]) as string[]).join(' '); + + runSubmit( + args({ + skillArgs: file( + 'advice-minimal-nocomment.txt', + '6771 --topology minimal', + ), + }), + ); + expect(advice()).toContain('posts nothing at any effort'); + expect(advice()).toContain('without `--topology minimal`'); + expect(advice()).toContain('`--comment`'); + expect(advice()).toContain('`review.comment`'); + expect(advice()).not.toContain('Nothing recorded authorises binding'); + expect(advice()).not.toContain('--user-authorized'); + expect(ghMock).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(3); + }); + it('posts when the user typed `--comment`', () => { // The bare-number recording carries no host, and this test runs // through the session-less --skill-args seam — the submission cwd's diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index b151e082f46..20a8f3c7d4d 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -74,6 +74,7 @@ import { import { recordedSeverityFloor, reviewWriteAuthorization, + type ReviewWriteRefusalClass, } from './lib/authorization.js'; import { hostsEquivalent, @@ -331,6 +332,7 @@ function authorization( ): { ok: boolean; why: string; + cls?: ReviewWriteRefusalClass; recordedHost?: string; recordedUnbound?: boolean; viaSkillArgsOverride?: boolean; @@ -731,28 +733,48 @@ function submit( // Not an error the caller can retry around — a refusal it must accept. The // findings are not lost: they are in the terminal output and the saved // report, and the user can ask for them to be posted. - // The advice must match the refusal class, or it misdirects the retry: - // the gate refuses either because comment was never requested (its `why` - // carries `` `--comment` was ``) or because nothing recorded authorises - // this target — a binding miss, or no recorded arguments at all. The - // second arm's preamble stays neutral ("Nothing recorded…") because a - // setting-driven missing-args refusal lands here too, and "The recorded - // arguments do not bind" would contradict its `why` ("no review - // arguments were recorded"). `--comment` cannot fix the second class — - // the flag stands in for nothing a target binding needs, and the - // `review.comment` setting already stood in for the flag on exactly - // those refusals — so advising it there buys the futile retry loop - // authorization.ts's refusal wording exists to prevent. - const advice = auth.why.includes('`--comment` was') - ? `This is the correct outcome of a review the user did not ask to ` + - `publish — report the findings in the terminal and stop. Re-run with ` + - `\`--comment\`, or pass --user-authorized only after the user has ` + - `asked, in a message they typed, for this review to be published.` - : `Nothing recorded authorises binding this target — report the ` + - `findings in the terminal and stop. Posting to this pull request ` + - `needs a review invoked naming it, or --user-authorized after the ` + - `user has asked, in a message they typed, for this review to be ` + - `published.`; + // The advice must match the refusal class, or it misdirects the retry — + // and it branches on the gate's structural `cls`, never on the refusal + // text: `why` embeds the operator's verbatim recorded arguments, and any + // marker string can itself appear inside that quoted record. A + // `--topology minimal` refusal is its own class: the record bound this + // target on every axis, so the binding arm's "Nothing recorded…" + // preamble and "a review invoked naming it" remedy are both wrong on it + // — the remedy re-refuses while the topology stands — and its other + // remedy, `--user-authorized`, mechanically posts what the topology + // bars. The topology arm restates the refusal's own remedy and names the + // comment source a re-run still needs — the canonical minimal record + // carries none, and the bare re-run re-refuses without it. The gate + // otherwise refuses either because comment was never requested, or + // because nothing recorded authorises this target — a binding miss, or + // no recorded arguments at all. The last arm's preamble stays neutral + // ("Nothing recorded…") because a setting-driven missing-args refusal + // lands here too, and "The recorded arguments do not bind" would + // contradict its `why` ("no review arguments were recorded"). + // `--comment` cannot fix that class — the flag stands in for nothing a + // target binding needs, and the `review.comment` setting already stood + // in for the flag on exactly those refusals — so advising it there buys + // the futile retry loop authorization.ts's refusal wording exists to + // prevent. + const advice = + auth.cls === 'topology' + ? `This is the correct outcome of a review run under ` + + `\`--topology minimal\` — the arm posts nothing at any effort. ` + + `Report the findings in the terminal and stop. Re-run the review ` + + `without \`--topology minimal\` — with posting requested ` + + `(\`--comment\` or the \`review.comment\` setting) — to make ` + + `posting available.` + : auth.cls === 'comment-not-requested' + ? `This is the correct outcome of a review the user did not ask ` + + `to publish — report the findings in the terminal and stop. ` + + `Re-run with \`--comment\`, or pass --user-authorized only ` + + `after the user has asked, in a message they typed, for this ` + + `review to be published.` + : `Nothing recorded authorises binding this target — report the ` + + `findings in the terminal and stop. Posting to this pull ` + + `request needs a review invoked naming it, or --user-authorized ` + + `after the user has asked, in a message they typed, for this ` + + `review to be published.`; refuse( `REFUSED to post to ${args.repo}#${args.pr}: ${auth.why}.\n` + `Posting is a public, irreversible write, and this run has no ` + diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 21acebb22d6..fc3083053e4 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -1942,7 +1942,8 @@ export class GeminiChat { */ private pendingPartialAssistantTurnIndex: number | null = null; private pendingPartialAssistantRecord: - Parameters[0] | null = null; + | Parameters[0] + | null = null; private readonly imagePayloadStore = new InMemoryImagePayloadStore(); diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 7b70cc85de4..cc393c1208b 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -679,10 +679,10 @@ The countermeasure is cheap and needs no new machinery: before Step 4, sanity-ch - **A `--quick` boolean:** two modes, but "quick" hides what is and isn't checked (rules? cross-file? build?). - **Three levels (chosen):** **low** = 3-6 directed angles (per `plan.budget.inlineAngles`) plus a gap sweep, all in the orchestrator's own context over the chunk plan — hunk-visible bugs only, ≤10 unverified findings. **medium** = the high pipeline minus its most expensive passes: the parallel finder fan-out over a reduced dimension set (no adversarial personas, no Agent 8), build & test, and a single verification pass — verified findings, Approve capped at Comment, no reverse audit. **high** = the full pipeline, unchanged. -**Guardrails, because an unverified pass is recall-limited by construction.** These guardrails defend against findings that no verifier ever checked, which since medium became a verified fan-out means **low alone**; medium shares only the cache and posting rules (its Approve cap is Step 6's own rule, not one of these). +**Guardrails, because an unverified pass is recall-limited by construction.** These guardrails defend against findings that no verifier ever checked, which since medium became a verified fan-out means **low and `--topology minimal` alone**; medium shares only the cache and posting rules (its Approve cap is Step 6's own rule, not one of these). - Labeled **unverified**; no Approve/Request-changes verdict is emitted. A verdict is a claim the pipeline earns in Steps 4–5; a quick pass claims findings, not absence of findings. -- Never posts to the PR: `--comment` forces high, and a "post comments" follow-up after a quick pass is declined. +- Never posts to the PR: `--comment` forces high at low effort, and the parser forces `comment.effective` to false on the minimal arm (terminal-only); a "post comments" follow-up is declined in both. - Never consults or writes the incremental cache — otherwise a medium run's SHA would make a later high run report "No new changes since last review", silently converting a quick pass into a full-review verdict. - Scope handling (worktree, diff capture, chunk plan) is identical at all levels. The levels change who reads the diff and what runs afterwards, never how the diff is obtained — the base-resolution and truncation traps do not care how fast the user wants the answer. @@ -690,6 +690,8 @@ The countermeasure is cheap and needs no new machinery: before Step 4, sanity-ch ## LLM call budget +**`--topology minimal` — 0 subagent calls.** The minimal arm (issue #9783, Step 3M) is a single careful pass over the diff in the orchestrator's own context — no fan-out, no verification, no reverse audit, no build/test. It costs one model turn, the same shape as the low tier's inline pass, and it is priced here for completeness, not as a recommended default: it exists so the full pipeline and this minimal prompt can be run over the same PR set and compared per model. This section tracks per-topology _cost_; the A/B the minimal arm enables extends it with per-model _quality_, which is what decides whether any cell of a future model-family × effort routing table routes away from the full pipeline. Until that data exists, every review runs the topology below. + **Small diffs (≤ 500 source lines AND ≤ 3200 total diff lines, Step 3A, high effort) — 17-28 calls (typically 17-19):** | Stage | Calls | Why | diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index d4279bb1a8a..224a53cd487 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -1,7 +1,7 @@ --- name: review -description: Review changed code for correctness, security, code quality, and performance. Use when the user asks to review code changes, a PR, or specific files. Invoke with `/review`, `/review `, `/review `, `/review --comment` to post inline comments on the PR, `/review --fix` to apply the findings to your working tree, or `/review --resume` to continue an interrupted review of that PR instead of starting over. Add `--effort low|medium|high` to trade depth for speed (defaults to high for PRs, medium for local changes). -argument-hint: '[pr-number|file-path] [--effort low|medium|high] [--severity-floor critical|suggestion] [--comment] [--fix] [--resume]' +description: Review changed code for correctness, security, code quality, and performance. Use when the user asks to review code changes, a PR, or specific files. Invoke with `/review`, `/review `, `/review `, `/review --comment` to post inline comments on the PR, `/review --fix` to apply the findings to your working tree, or `/review --resume` to continue an interrupted review of that PR instead of starting over. Add `--effort low|medium|high` to trade depth for speed (defaults to high for PRs, medium for local changes). Add `--topology minimal` to run the single-pass A/B comparison arm instead of the pipeline. +argument-hint: '[pr-number|file-path] [--effort low|medium|high] [--severity-floor critical|suggestion] [--topology minimal] [--comment] [--fix] [--resume]' allowedTools: - task - run_shell_command @@ -69,7 +69,8 @@ It prints a JSON verdict; use it **verbatim**: - `comment.requested` / `comment.effective` — `effective` is what gates Step 7 (true also when only the `review.comment` setting is on); `requested && !effective` means the user asked on a non-PR target, and the warning for that is already in `warnings`. - `fix.requested` / `fix.effective` — `--fix` is `--comment` reflected, and gated on the opposite target. `--comment` writes to a **pull request**, so it needs one; `--fix` writes to a **working tree**, so it needs one that outlives the review. A PR review's tree is the ephemeral worktree `fetch-pr` creates and Step 9 deletes, so `--fix` on a PR target is ignored with a warning — edits there are discarded minutes later, and reporting findings as "fixed" into a directory that no longer exists is worse than not fixing them. `effective` is what gates Step 6B. An effective `--fix` also floors the effort at **medium**: it edits the user's files, and low runs no verification, so applying an unverified finding is the same mistake as posting one, aimed at their working tree instead of a pull request. It does not force **high** — medium's findings are verified, and the reverse audit high adds hunts for findings that are _missing_, which is not what deciding whether to apply one turns on. - `severityFloor` + `severityFloorSource` — the posting floor for a PR review: `critical` posts only Criticals (otherwise-postable high-confidence Suggestions are recorded and deferred — Step 6's convergence posture; low-confidence and Nice-to-have findings stay terminal-only as ever), `suggestion` posts Criticals and Suggestions at every round, and `auto` — the default — is the **round-adaptive rule you resolve in Step 6**, where the round is known: `suggestion` through round 5, `critical` from round 6 — **or `critical` from any round once the recovered ledger's `flatRounds` streak has reached its bar** (Step 6's signal-driven trigger: the first-time-finding rate has not fallen for that many consecutive rounds, so the loop is re-deriving the same set and the floor stems it early). The parser cannot resolve `auto` itself (the round comes from the previous posted round's ledger, not fetched yet), so carry the verdict's value forward and resolve it there. Explicit flag beats the `review.severityFloor` setting beats `auto`; a non-PR target has no rounds, so the flag warns and is ignored there. The floor governs what the review **posts**, never what it finds, verifies, or reports in the terminal. -- `resume.requested` / `resume.effective` — `--resume` continues an interrupted run of the same PR instead of starting over. `effective` is what gates the resume branch below, and it is a TARGET-SHAPE gate rather than a promise: a cross-repo `pr-url` with no matching remote is `effective: true` but routes to lightweight mode, which never calls `fetch-pr` — item 3 below owns telling the user the flag is inert there. `requested && !effective` means a local or file target, already warned in `warnings`. It never changes the effort: a continuation is pinned to the interrupted run's recorded level, and an explicitly different `--effort` makes `fetch-pr` refuse the resume and run fresh at the requested one. +- `topology` + `topologySource` — the shape of the run. `auto` (the default) runs the standing effort-driven pipeline described below. `minimal` runs the single-pass A/B comparison arm (Step 3M) instead — and when it is set, it OVERRIDES the effort dispatch entirely. In this step you run `parse-args` and the **diff capture only** (`fetch-pr` for a same-repo PR, the lightweight `fetch-diff` for a cross-repo PR, or the local capture for a local/file target — exactly as below), then jump straight to **Step 3M**. You SKIP the rest of Step 1's setup — the rules load, `pr-context`, `comment-status`, and the incremental-cache check — and you skip the fan-out, verification, reverse audit, and posting. `minimal` is terminal-only; the parser has already forced `comment.effective`, `fix.effective`, and `resume.effective` to false, and its warnings for that are in `warnings`. There is no configured topology — it is only ever an explicit flag. +- `resume.requested` / `resume.effective` — `--resume` continues an interrupted run of the same PR instead of starting over. `effective` is what gates the resume branch below, and it is a TARGET-SHAPE gate rather than a promise: a cross-repo `pr-url` with no matching remote is `effective: true` but routes to lightweight mode, which never calls `fetch-pr` — item 3 below owns telling the user the flag is inert there. `requested && !effective` means a local or file target, or `--topology minimal` (a fresh single pass neither continues nor consumes an interrupted run), already warned in `warnings`. It never changes the effort: a continuation is pinned to the interrupted run's recorded level, and an explicitly different `--effort` makes `fetch-pr` refuse the resume and run fresh at the requested one. - `warnings` — surface every entry to the user, word for word. - `extraTokens` / `unknownFlags` — leftover input the parser refused to guess about; mention them to the user rather than silently dropping them. @@ -85,7 +86,9 @@ What each level runs: - **medium** — **balanced**: the high pipeline with its most expensive passes removed. It runs the parallel review agents (Step 3A/3B) over a **reduced dimension set** — issue fidelity (Agent 0, PR targets only), correctness (Agents 1a/1b/1c), **security (Agent 2)**, quality (Agents 3a/3b/3c), performance (Agent 4), **test coverage (Agent 5)**, and **build & test (Agent 7)** — followed by a **single verification pass** (Step 4). It loads and enforces project rules (Step 2) and runs `comment-status` like high. It **skips** the adversarial-persona agents (6a/6b/6c), the language-pitfall and wrapper/proxy specialists (Agents 1d/1e), the diff-specialist finders (Agent 8), the **reverse audit** (Step 5), the incremental cache, and PR posting (`--comment` still forces high). Findings are **verified** (Step 4 ran — they are not "unverified" the way low's are), but without the reverse-audit second pass. Reach for it when high is too slow/expensive but a real bug-catching review is still needed: it keeps the two things that reliably catch bugs cheaply — the finder fan-out and `build-test` (which mechanically catches compile/test failures) — and drops the depth passes with the lowest marginal yield. Measured against high on the same PR it lands at roughly **one-third to one-half** the time and tokens. It reliably catches mechanical defects (compile errors, failing tests) and obvious correctness bugs, but is **not an exhaustive correctness audit** — a subtle Critical that only the reverse audit or the adversarial personas would surface can slip; for a security-sensitive or pre-release review, use `--effort high`. - **high** — the full pipeline: parallel review agents (Step 3A/3B — the full dimension set including security, test-coverage, the language-pitfall and wrapper/proxy specialists 1d/1e, the adversarial personas 6a/6b/6c, and Agent 8), verification (Step 4), iterative reverse audit (Step 5), PR submission (Step 7), incremental cache (Step 8). -At every effort level, the mechanics of obtaining the diff — worktree flow, diff capture, base resolution, chunk plan — are shared: the truncation and wrong-base traps this step exists for do not care how fast you want the answer. The _reviewed range_ can still differ: the incremental cache is a high-only feature, so a high re-review of a previously-reviewed PR may scope to `lastCommitSha..HEAD` while a low/medium pass (which never consults the cache) always reviews the full PR diff. +The three levels above are the standing effort axis. **`--topology minimal` is a separate axis — a different _shape_ of review, not a depth of one — and it overrides the effort dispatch.** It is the A/B comparison arm from issue #9783: a single careful senior-engineer pass over the diff in this context, at most fifteen findings, each carrying a concrete failure scenario; no subagents, no build/test, no verification, no reverse audit, no posting, no incremental cache, no project rules. It exists so the full pipeline and this minimal prompt can be run over the same PR set and compared per model — the hypothesis being that the scaffolding's marginal value shrinks (even turns negative) as the model gets stronger. When the verdict's `topology` is `minimal`, capture the diff exactly as this step describes, then run **Step 3M** and skip everything else. + +At every effort level — and under `--topology minimal` — the mechanics of obtaining the diff — worktree flow, diff capture, base resolution, chunk plan — are shared: the truncation and wrong-base traps this step exists for do not care how fast you want the answer. The _reviewed range_ can still differ: the incremental cache is a high-only feature, so a high re-review of a previously-reviewed PR may scope to `lastCommitSha..HEAD` while a low/medium/minimal pass (which never consults the cache) always reviews the full PR diff. The parser already classified the target, so there is nothing to disambiguate by hand. For a `pr-url` target, determine if the local repo can access this PR: @@ -330,6 +333,8 @@ Do NOT inject review rules into Agent 7 (Build & Test) — it runs deterministic ## Step 3: Parallel review (high and medium effort) +**If the verdict's `topology` is `minimal`, skip everything in this step and its sub-steps and run Step 3M instead** — the single-pass A/B arm defined after Step 3C. The rest of this dispatch applies only to `topology: auto`. + **Steps 3A/3B and 4 run at high and medium effort; Step 5 (reverse audit) is high only.** At **low** effort skip 3A/3B/4/5 and run **Step 3C** instead — an inline pass with no subagents, defined after the agent dimensions. **Medium** runs 3A/3B and Step 4 with the reductions the effort table names: a smaller dimension set (skip the adversarial personas 6a/6b/6c, the language-pitfall and wrapper/proxy specialists 1d/1e, and the Agent 8 diff-specialists), a capped territory fan-out on large diffs (Step 3B below), and **no reverse audit** — it stops after Step 4. The incremental cache and PR posting stay high-only at medium too. Launch review agents by invoking all `agent` tools in a **single response**. The runtime executes agent tools concurrently — they will run in parallel. You MUST include all tool calls in one response; do NOT send them one at a time. @@ -589,6 +594,29 @@ Then skip Steps 4 and 5 entirely and go to Step 6 with these adjustments: - Step 6B never runs either, and cannot: an effective `--fix` floors the effort at medium (Step 1), so no low pass is ever a `--fix` run. If the user asks to apply the findings after a quick pass, the same reasoning as posting applies with the target changed — editing their files on the strength of an unverified finding is the mistake, not publishing it — so point at `/review --fix`, which re-runs at medium and produces findings a verifier has ruled on. - In Step 8, save the report (marked with the effort level) but do **not** write the incremental cache — a quick pass must never make a later full review report "No new changes since last review". Step 9 cleanup runs as usual. +## Step 3M: Minimal single pass (`--topology minimal`, the A/B arm) + +This arm exists for one reason: to be run over the same PR set as the full pipeline and compared, per model, so we learn whether the scaffolding still earns its cost (issue #9783). It is deliberately **not** the low-effort angle rotation — it is a single careful pass with no angle list, no sweep, no fan-out, and no verification. Do not "improve" it by re-adding the scaffolding; the whole point is to measure the pass without it. + +There are no subagents: you are the reviewer, in this context. Read the diff via the chunk plan — `read_file` per chunk range, paging oversized chunks; the read-cap rules from Step 1 apply unchanged, and chunks whose `maxLineChars` exceeds the read cap are uncoverable here exactly as in 3A. (For a file-path review of an unchanged file there is no plan — read the whole file, paging until `isTruncated` is false, per Step 1's no-diff branch.) Where a hunk is ambiguous without its surroundings, you may read the enclosing function (cross-repo lightweight mode has no tree — review from the diff alone there); do **not** grep the codebase and do **not** build or run anything. Project rules are not loaded (Step 2 is skipped). + +Review this diff the way a careful senior engineer would, in one pass. For every changed line ask what input, state, timing, or platform makes it wrong; for every deleted or replaced line ask where the invariant it enforced is re-established; watch for the failure modes the change itself introduces. Do not rotate the pass into separate angle walks — that is the low tier, not this one. + +Report **at most fifteen findings**, most severe first, each in the standard finding format. The quality bar that stands in for the scaffolding is the **Failure scenario**: every finding must name the concrete input/state/timing that triggers it and the wrong outcome that results (or, for a quality finding, the concrete cost). A finding for which you cannot construct a scenario is not reported — drop it at the source rather than filing it half-believed. The reporting gate applies unchanged: a Suggestion with no concrete scenario or cost is dropped; a suspected Critical you cannot pin down is kept with `Confidence: low`. Sort by severity. If the diff is genuinely clean, report nothing — do not pad toward the cap. + +Then skip Steps 4 and 5 entirely and go to Step 6 with these adjustments: + +- Use Step 6's structure, but label the review **"Minimal pass (topology: minimal) — findings are unverified"** (translated per output language) in the Summary, and skip verification stats (there was no verification). +- Still make Step 6's `report_findings` call, with `level: "low"`. No findings artifact exists on this arm (the Step 8 bullet forbids creating one), so the entries come from the composed finding list — `severity`, `file`/`line`, `summary`, `shortSummary`, `failureScenario` — with `confidence: "low"` only on the candidates you kept under `Confidence: low`, omitted elsewhere: the `low` level is the only one clients render the unverified marker for, and it already labels the whole list unverified — passing the resolved effort instead (high on a PR target) would render these unverified findings indistinguishably from a verified high-effort review, and a blanket `confidence` would erase the one distinction the pass recorded. Step 6's delivery rule applies unchanged — a failure is disclosed and moved past, never a reason to change the findings. +- Emit **no verdict** — no Approve / Request changes / Comment, and skip the open-Criticals re-check. Chunks that are uncoverable by `maxLineChars` are still listed under "Not reviewed". +- Offer no follow-up tip from Step 6's list — its `post comments` tips key on `comment.effective` being false, which this arm forces, so they would invite exactly the posting this arm declines, and Step 6's trigger-phrase handler routes that ask toward Step 7. The only follow-up this arm offers is the pointer to `/review --effort high`. +- Step 7 never runs and cannot: the parser forced `comment.effective` to false for this topology. If the user asks to post the findings, decline and point at `/review --effort high` (unverified findings must not be posted publicly). +- Step 6B never runs either: the parser forced `fix.effective` to false. If the user asks to apply the findings, point at `/review --fix`, which re-runs at medium with verified findings. +- In Step 8, save the report (marked `topology: minimal`) but do **not** create or register the structured artifact and do **not** write the incremental cache — the artifact persists a composed verdict and this pass emits none, so there is no composed input for `save-artifact` to read, and a cache write would make a later full review report "No new changes since last review". Step 9 cleanup runs as usual. +- Step 9's completion line takes the `minimal pass, not posted ( unverified findings)` disposition — the one Step 9's list reserves for this topology. + +(Why Step 3M repeats Step 3C's closing adjustments almost verbatim rather than referencing them: the two passes are different experiments and each must stay readable on its own. The shared parts — no posting, no fix, no cache, no verdict — are the same for the same reason in both: an unverified single-context pass must not publish, edit, or certify.) + ## Step 4: Deduplicate, verify, and aggregate (high and medium effort) ### Deduplication @@ -999,7 +1027,7 @@ The three words are three different claims and are not interchangeable. `fixed` Report the outcome counts in the terminal summary, and list each `skipped` finding with its reason. **Do not re-run Steps 1–6** to check your own work: a re-review of a tree you just edited is a new review of different code, and its verdict is not this review's. -Append a follow-up tip after the verdict (high and medium effort — only a **low** quick pass emits no verdict and uses Step 3C's tip instead; its "post comments" follow-up is declined per Step 3C). **Tip lines are user-facing terminal prose — translate them into your output language** (critical rule 2). The English templates below define the _content_ and the _command keywords_ (which stay verbatim — `post comments`, `fix these issues`, `commit` are trigger phrases the user types back); translate the surrounding sentence. With a Chinese output language, "Tip: type `post comments` to publish findings as PR inline comments." becomes "提示：输入 `post comments` 将发现作为 PR 行内评论发布。" At **medium**, also add: "Tip: run `/review --effort high` for the full verified review (adds the reverse audit, the language-pitfall and wrapper/proxy specialists, the adversarial personas, and Agent 8 — and can certify Approve)." Choose the rest based on remaining state: +Append a follow-up tip after the verdict (high and medium effort — only a **low** quick pass and a `--topology minimal` pass emit no verdict and follow their own tip rules instead (Step 3C / Step 3M); their "post comments" follow-ups are declined per those steps). **Tip lines are user-facing terminal prose — translate them into your output language** (critical rule 2). The English templates below define the _content_ and the _command keywords_ (which stay verbatim — `post comments`, `fix these issues`, `commit` are trigger phrases the user types back); translate the surrounding sentence. With a Chinese output language, "Tip: type `post comments` to publish findings as PR inline comments." becomes "提示：输入 `post comments` 将发现作为 PR 行内评论发布。" At **medium**, also add: "Tip: run `/review --effort high` for the full verified review (adds the reverse audit, the language-pitfall and wrapper/proxy specialists, the adversarial personas, and Agent 8 — and can certify Approve)." Choose the rest based on remaining state: - **Local review with unfixed findings** (Step 6B did not run — `--fix` was not passed): "Tip: type `fix these issues` to apply fixes interactively, or re-run with `/review --fix` to have the review apply and account for them itself." - **Local review where Step 6B ran**: offer no fix tip — the findings already carry outcomes. If any came back `skipped`, say so with their reasons instead. @@ -1007,16 +1035,16 @@ Append a follow-up tip after the verdict (high and medium effort — only a **lo - **PR review, zero findings** (only if `comment.effective` is false): "Tip: type `post comments` to approve this PR on GitHub." - **Local review, all clear** (Approve or all issues fixed): "Tip: type `commit` to commit your changes." -If the user responds with "fix these issues" (local review only), use the `edit` tool to fix each remaining finding interactively based on the suggested fixes from the review — do NOT re-run Steps 1-6. This is the same work Step 6B does; when the review has a findings artifact, record the outcomes into it the same way (`review findings --outcomes`) and re-issue the `report_findings` call with the outcomes, exactly as Step 6B prescribes, rather than leaving the list, the tree, and the client display disagreeing about what was applied. +If the user responds with "fix these issues" (local review only), use the `edit` tool to fix each remaining finding interactively based on the suggested fixes from the review — do NOT re-run Steps 1-6. This is the same work Step 6B does; when the review has a findings artifact, record the outcomes into it the same way (`review findings --outcomes`) and re-issue the `report_findings` call with the outcomes, exactly as Step 6B prescribes, rather than leaving the list, the tree, and the client display disagreeing about what was applied. Under `--topology minimal`, decline per Step 3M instead — the findings are unverified; point at `/review --fix`, which re-runs at medium with verified findings. -If the user responds with "post comments" (or similar intent like "yes post them", "publish comments"), proceed directly to Step 7 using the findings already collected — do NOT re-run Steps 1-6. +If the user responds with "post comments" (or similar intent like "yes post them", "publish comments"), proceed directly to Step 7 using the findings already collected — do NOT re-run Steps 1-6. Under `--topology minimal`, decline per Step 3M instead — the findings are unverified, and the `--user-authorized` fast path would post them on the ask alone. ## Step 7: Submit PR review **This step lives in `references/posting.md` — read it with `read_file` from this skill's base directory the moment posting becomes live for this run, and follow it.** Posting is live when the Step 1 verdict reported `comment.effective: true`, or when the user asks in this session to post or publish the comments. Do not read it on a run that will not post. What binds every run, posted or not: - Never run a `gh` command that writes to the pull request — nor an `a1` command that writes to the MR — `qwen review submit` is the only write path in this skill, and it refuses when the run is not authorised. The one carve-out is Step 4's render-adjudication post to the user-designated `QWEN_REVIEW_SCRATCH_REPO` — that repo, that check, nothing else. -- Posting is a PR-only, high-only action: on a non-PR target there is nothing to post to, and at **low or medium** effort a "post comments" follow-up is declined with a pointer at `--effort high` (low's findings are unverified; medium's verdict is capped at Comment — `--comment` forces high). +- Posting is a PR-only, high-only action: on a non-PR target there is nothing to post to, and at **low or medium** effort — or under `--topology minimal` — a "post comments" follow-up is declined with a pointer at `--effort high` (low's findings are unverified; medium's verdict is capped at Comment — `--comment` forces high; minimal's findings are unverified and the arm posts nothing). - You do not author PR-facing prose: `compose-review` computes the review body, and the only text that reaches the PR is that computed body plus the inline finding comments, both riding the one sanctioned write `references/posting.md` defines. ## Step 8: Save review report and cache @@ -1047,6 +1075,7 @@ where `` is the same suffix as above (`pr-6740`, `local`, a filename) an - `, not posted ( Critical, Suggestion)` — **high or medium** effort without `--comment`/publish authorization (medium never posts — `--comment` forces high); `` is Approve / Request changes / Comment (a medium verdict never exceeds Comment — see Step 5). - `, partial ( inline posted, summary posted)` — Aone mid-batch failure only: `submit` answered `{"posted": false, "partial": true}` (part of the review IS on the MR). Use `summary not posted` when `summaryPosted` is false. This disposition is NEITHER `posted` NOR `not posted` — see the Aone refinements below — and it never carries a `Posted:` line. - `quick pass, not posted ( unverified findings)` — **low** effort only. +- `minimal pass, not posted ( unverified findings)` — `--topology minimal` only (Step 3M). Minimal emits no verdict, so it cannot take a `, not posted` form, and it is not the low tier, so it cannot take the quick-pass form either — this disposition is the only contract-conformant line for the arm. For any `posted` disposition, the line immediately **above** this one is `Posted: ` — the review link `submit` returned (Step 7) — or, when Step 7's platform fallback says the link was not returned, the no-link note that fallback prescribes. The link rides its own line because the completion line's shape is fixed and scrapers must not have to strip a URL out of it. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 1901d309443..d9aabab1b8b 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -1128,6 +1128,29 @@ describe('bundled review skill', () => { expect(core).toContain('- `modelId` — for the footer.'); }); + it('pins the minimal arm report_findings override on the unverified level', () => { + // Step 6 mandates `report_findings` at the run's RESOLVED effort with + // entries copied from the findings artifact, and Step 3M forbids the + // artifact. Without its own override — the one Step 3C has — the arm + // either skips the call for lack of an artifact or reports at the + // resolved effort (high on a PR target): clients render the unverified + // marker only for `level: "low"`, so either shape defeats the + // labeled-unverified property the parser force-offs and the posting + // declines reserve for this arm. + const body = coreBody(); + const start = body.indexOf('## Step 3M'); + const end = body.indexOf('## Step 4'); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + const section = body.slice(start, end); + expect(section).toContain('`report_findings`'); + expect(section).toContain('`level: "low"`'); + expect(section).toContain('the composed finding list'); + expect(section).toContain( + 'would render these unverified findings indistinguishably from a verified high-effort review', + ); + }); + it('keeps template tokens out of the raw-loaded reference files', () => { // BundledSkillLoader interpolates only the core body it injects; the // reference files are read raw via read_file, so a token there reaches diff --git a/packages/core/src/skills/bundled/review/references/persistence.md b/packages/core/src/skills/bundled/review/references/persistence.md index a7ed1902c18..02e2b7ff5b0 100644 --- a/packages/core/src/skills/bundled/review/references/persistence.md +++ b/packages/core/src/skills/bundled/review/references/persistence.md @@ -5,7 +5,7 @@ _Reference file of the `review` skill, loaded on demand — the core body before Step 8 except cross-repo lightweight runs, which skip Step 8 entirely (Step 1 names the skip)._ -**Steps 8 and 9 are four responses, not ten.** Every command in this tail is cheap; the model turns between them are not — and this stretch runs after the verdict is already computed (and, on a posting run, already posted), so every extra turn is pure latency to the reader. Measured across six CI reviews, the one-command-per-turn shape cost 4–6 minutes after submission, before the artifact-root fumbling below stretched it further (measured; DESIGN.md — The one-command-per-turn tail). The dependency chain is short, so batch to it, issuing each group as separate tool calls in one response exactly as the Step 1 setup batch does: (1) `cost-ledger` plus the `read_file` of the findings artifact — everything the report's content still needs; (2) write the Markdown report; (3) `save-artifact` and the incremental-cache write, when this run owes one — both read only files that already exist, and neither reads the other; (4) `record_artifact` (its `workspacePath` comes from save-artifact's stdout, which is why it is not in group 3) together with Step 9's `cleanup`. **Group (4) fires only after group (3) succeeded**: `cleanup` deletes the `.qwen/tmp` side files `save-artifact` reads, so a failed group (3) is resolved first — the JSON helper is fail-closed (below), and destroying its only inputs would convert a recoverable failure into a permanent one. When it **cannot** be resolved (a malformed input, a disk error — synthesizing a replacement is forbidden by the same fail-closed rule), the run still ends properly: disclose the failure, skip `record_artifact` (there is nothing valid to register), copy the findings/composed inputs beside the Markdown report so the artifact stays rebuildable, and **still run `cleanup`** — Step 9's bypass audit and the completion line are never gated behind a success that will not come. A group's remaining reads may join its response; nothing here needs a turn of its own. Lower tiers drop the commands they never owed (low saves no artifact and writes no cache), not the batching. +**Steps 8 and 9 are four responses, not ten.** Every command in this tail is cheap; the model turns between them are not — and this stretch runs after the verdict is already computed (and, on a posting run, already posted), so every extra turn is pure latency to the reader. Measured across six CI reviews, the one-command-per-turn shape cost 4–6 minutes after submission, before the artifact-root fumbling below stretched it further (measured; DESIGN.md — The one-command-per-turn tail). The dependency chain is short, so batch to it, issuing each group as separate tool calls in one response exactly as the Step 1 setup batch does: (1) `cost-ledger` plus the `read_file` of the findings artifact — everything the report's content still needs; (2) write the Markdown report; (3) `save-artifact` and the incremental-cache write, when this run owes one — both read only files that already exist, and neither reads the other; (4) `record_artifact` (its `workspacePath` comes from save-artifact's stdout, which is why it is not in group 3) together with Step 9's `cleanup`. **Group (4) fires only after group (3) succeeded**: `cleanup` deletes the `.qwen/tmp` side files `save-artifact` reads, so a failed group (3) is resolved first — the JSON helper is fail-closed (below), and destroying its only inputs would convert a recoverable failure into a permanent one. When it **cannot** be resolved (a malformed input, a disk error — synthesizing a replacement is forbidden by the same fail-closed rule), the run still ends properly: disclose the failure, skip `record_artifact` (there is nothing valid to register), copy the findings/composed inputs beside the Markdown report so the artifact stays rebuildable, and **still run `cleanup`** — Step 9's bypass audit and the completion line are never gated behind a success that will not come. A group's remaining reads may join its response; nothing here needs a turn of its own. Lower tiers drop the commands they never owed (low saves no artifact and writes no cache; a `--topology minimal` run owes neither at ANY effort — no verdict, no composed input, no artifact, no cache), not the batching. ### Report persistence @@ -25,19 +25,19 @@ Report content should include: - Review timestamp and target description - **Provenance — the commits and the toolchain.** The head SHA reviewed (`fetchedSha` from the fetch report) and the base it was diffed against — **the range the round actually used**: `incremental.diffBase` on a delta-scoped round (`incremental.effective` and no `upToDate`), `mergeBaseSha` on every other, since recording the merge base for a round that reviewed `diffBase..head` hands the later reader a scope the run never had — plus the platform and the Node/npm versions the gates ran on, and one line per gate with its result (`build`, `test`, `script-lint`, `test-efficacy`, `test-plan` — ran / clean / failed / skipped, and why). A saved report is read by someone who cannot re-derive what it was about: without the SHA pair a "Verdict: Approve" names no commit, so it can be neither checked against the PR nor distinguished from an approval of a different head; and without the gate line a reader cannot tell a gate that passed from one that never ran. Both facts are already in reports this run has open — copy them, do not re-measure. -- Effort level the review ran at (low / medium / high; **low** findings are marked unverified — medium and high verify them in Step 4) +- Effort level the review ran at (low / medium / high; **low** findings are marked unverified — medium and high verify them in Step 4; under `--topology minimal` mark the topology too — its findings are unverified whatever effort the run resolved) - Diff statistics (files changed, lines added/removed) — omit if reviewing a file with no diff -- Build & test results (Agent 7 output summary) — high and medium effort +- Build & test results (Agent 7 output summary) — high and medium effort (absent under `--topology minimal` — no agents ran) - All findings with verification status. Read them out of the findings artifact `qwen review findings` wrote (`.qwen/tmp/qwen-review-{target}-findings.json`) rather than re-typing them from the terminal — a third transcription of the same list is a third chance for a severity to drift, which has happened inside a single review. - **Per-finding outcomes, when Step 6B ran** — `fixed` / `skipped` / `no_change_needed`, with the reason for every `skipped`. The artifact already carries them; a `--fix` run whose archive does not say which findings were applied is a report that reads as if all of them were. -- Verdict (high and medium effort — a low quick pass claims none; a medium verdict never exceeds Comment, since it runs no reverse audit — see Step 5) +- Verdict (high and medium effort — a low quick pass and a `--topology minimal` pass claim none; a medium verdict never exceeds Comment, since it runs no reverse audit — see Step 5) - **The cost ledger — run it, do not compute it.** `"${QWEN_CODE_CLI:-qwen}" review cost-ledger --plan --out .qwen/reviews/-cost-ledger.json` aggregates the model calls the harness recorded for this review — the main loop and each agent, with input / cached / output / thinking token counts and wall time — from the harness's own usage records, the same records the coverage gate trusts. The window is bounded: it starts at the plan's mtime, and the ledger runs at this step, so the pre-plan bootstrap turns and the composition after this snapshot are not captured, and side queries such as chat compression leave no usage records to capture at all. Paste its printed block into the report verbatim, and relay the first line in the terminal summary. The printed block lists only the eight biggest agents; the `--out` JSON keeps every one, so the diffable record survives in full (worktree mode: resolve `--out` against the main project directory, like the report itself). If it prints `cost-ledger unavailable`, note that instead — it is informational and never blocks a review. Why it is in the archive: a "this version got slower" report is unanswerable from memory, and the one time it was answered properly took hours of telemetry forensics to find a repair round that had silently doubled a run. The ledger makes the next such question a diff of two saved reports. **The report's verdict is not yours to type.** `compose-review` printed the exact `Verdict:` line in Step 6 and persisted the same line as `verdictLine` inside `.qwen/tmp/qwen-review-{target}-composed.json` — copy either, verbatim. Do not reconstruct it from `event` + `cappedBy`: a presubmit downgrade also depends on fields that pair does not carry, and a rebuilt line can differ from the computed one. (And not `$(jq …)`: a `jq` binary is not guaranteed on the host, and a substitution that fails leaves the archived verdict blank or literal — worse than absent, because it looks written.) A run has written an Approve into its saved report minutes after reading the capped verdict (measured; DESIGN.md — The narrated-away cap). The terminal is prose and the archive is forever; this line is the one place the archive can be made to tell the truth for free. If the composed event is not the one you expected, fix the run — not the report. -After the Markdown report exists, create and register the structured review artifact for **medium and high** effort (low has no canonical composed verdict and must not invent one) — the creation is group (3) of the batching rule above; the registration rides group (4) alongside cleanup, which never touches `.qwen/reviews/`. Use the same filename stem as the Markdown report with a `.json` extension: +After the Markdown report exists, create and register the structured review artifact for **medium and high** effort (low has no canonical composed verdict and must not invent one; a `--topology minimal` run owes none at ANY effort — it emits no verdict at all, so there is no composed JSON to persist — Step 3M) — the creation is group (3) of the batching rule above; the registration rides group (4) alongside cleanup, which never touches `.qwen/reviews/`. Use the same filename stem as the Markdown report with a `.json` extension: ```bash "${QWEN_CODE_CLI:-qwen}" review save-artifact \ @@ -72,7 +72,7 @@ The JSON helper is fail-closed because it carries the authoritative review resul ### Incremental review cache -If reviewing a PR **at high effort**, update the review cache for incremental review support. Low and medium reviews must NOT write it — a cache hit would make a later high-effort review of the same SHA report "No new changes since last review", silently converting a cheaper pass into a full-review verdict. +If reviewing a PR **at high effort**, update the review cache for incremental review support. Low and medium reviews must NOT write it, and neither must a `--topology minimal` run at any effort — a cache hit would make a later high-effort review of the same SHA report "No new changes since last review", silently converting a cheaper pass into a full-review verdict. **The cache advances exactly when the marker anchored — read the marker, do not re-derive the net.** `compose-review` already computed whether this round may certify a range: its posted body's ledger marker carries a `sha` on a clean round and withholds it otherwise (unproven coverage, an undecided blocker, any cap other than a depth-only `unreviewed-dimension` — where depth-only means every entry names the build-and-test dimension or is the machine's own relayed stop entry; a whiffed LENS in that field withholds). The cache and the marker must never disagree about what a clean round is, and a hand-copied condition list here is how they drifted once already — the list in this paragraph aged out of sync with the module and told a whiffed-lens round to cache the sha the marker had refused. So the rule is mechanical: **write `lastCommitSha` into the cache only if the composed body's marker carries a `sha`** (check the composed JSON's body for `"sha"` inside the `qwen-review-ledger` comment); when it does not, **skip the cache write entirely and say so in the terminal output**. Caching this SHA would scope the next high-effort run to `lastCommitSha..HEAD` — or, worse, let the same-SHA shortcut report "No new changes since last review" and skip the run outright, Step 6 re-check included: a whiffed Security lens at SHA A followed by an incremental review at SHA B means no run ever reviews A's diff for security, and an existing blocker this run could only mark `cannot tell` would never be re-checked at the same SHA, while the cached verdict reads as full coverage. Leave the previous cache entry in place (or none), so the next high-effort run re-covers the whole range — re-detecting any uncoverable chunk and re-ruling on any undecided blocker, keeping both disclosures alive: diff --git a/packages/core/src/skills/bundled/review/references/posting.md b/packages/core/src/skills/bundled/review/references/posting.md index 87961a6230f..40a7ec51c8a 100644 --- a/packages/core/src/skills/bundled/review/references/posting.md +++ b/packages/core/src/skills/bundled/review/references/posting.md @@ -4,7 +4,7 @@ _Reference file of the `review` skill, loaded on demand — the core body (Steps 1–6 and 9) is already in your context. Load this file when, and only when, posting is live for the run: the Step 1 verdict reported `comment.effective: true`, or the user asked this session to post the -comments — on a PR target at high effort. Never write to the PR/MR without +comments — on a PR target at high effort, and never under `--topology minimal` (the arm posts nothing at any effort). Never write to the PR/MR without having read it._ **The whole rule in one sentence, so it survives even when the rest is compressed away: never run a `gh` command that writes to the pull request — nor an `a1` command that writes to the MR — `qwen review submit` is the only write path in this skill, and it refuses when the run is not authorised.** Everything below only spells out what "writes" covers so a compressor cannot quietly narrow it to a single API route. It is **every write path to the PR/MR**, not one: no `gh api repos/.../pulls//reviews` (not to submit, not to "test" an anchor), no `gh pr comment`, no `gh pr review`, no `gh issue comment`, no `gh api` with POST/PATCH/PUT/DELETE against the PR's `issues/*` or `pulls/*` endpoints, and — on an Aone target — no `a1 repo mr comment create`, no `a1 repo mr approve`, no `a1 repo mr edit`: no posting a finding or a verdict "by hand" when `submit` refused, in whole or in part — "by hand" is never an agent action; a remedy that names the USER as its actor is for the user to perform, not for you to perform for them. And no editing or deleting existing comments on either platform. (One narrowly-scoped carve-out exists and it does not touch the PR: the Step 4 render-adjudication check may post a minimal payload to the repo the **user designated** in `QWEN_REVIEW_SCRATCH_REPO` — that repo, that check, nothing else; absent the setting there is no carve-out at all, and nothing about the PR, its code, or its authors is ever posted there.) **You do not author PR-facing prose at all** — `compose-review` computes the review body from structured state (the verdict, the downgrade reasons, the body-Criticals), and there is no free-text field to pass through it; a free-form note you want to add is a note for the **terminal summary**, which the user reads, not for the pull request. The only text that reaches the PR is that computed body plus the inline finding comments, and both ride the one sanctioned write below. This bypass has happened, invisibly to everything downstream (measured; DESIGN.md — The gh pr comment bypass). On GitHub targets, `cleanup` audits the review window and flags issue comments by the reviewing account (submit never posts one — see Step 9), so that bypass is at least named in the terminal — a tripwire, not permission. On Aone targets the same tripwire is keyed on comment ids: there the sanctioned submit POSTS COMMENTS (the inline findings and the summary — Aone has no review object), so `cleanup` lists the MR's comments through the `a1` CLI and flags any comment the authenticated account posted — or edited — inside the window that the receipt `submit` wrote does not vouch for. The one write in this skill lives behind a check: @@ -32,7 +32,7 @@ It also refuses a payload that contradicts itself — a body promising inline co If **none** of the three holds, `submit` refuses and nothing is written. You MUST NOT reach around it — no `gh api .../pulls/.../reviews`, no other comment/review write, at all in this run — regardless of the verdict, the number of Criticals, or any "Tip: post comments" text you are about to print. A Request-changes verdict with unposted Criticals is the correct, complete outcome of a review without an effective comment authorisation: the findings live in the terminal (Step 6) and the saved report (Step 8), and the follow-up tip invites the user to post if they want. Do not rationalize a post because the findings "seem important" — the user decides when feedback becomes public. This gate has been violated in dogfooding (measured; DESIGN.md — The self-filed COMMENT review (PR #6771)); the check is arithmetic, not judgment: no flag, no standing setting, and no explicit request ⇒ no write. -Also skip this step (independently of the gate above) if the review target is not a PR, or if the review ran at low or medium effort. **Low**'s findings are unverified and must never be posted. **Medium**'s findings ARE verified (Step 4 ran), but posting is a high-only action — `--comment` forces high, and medium's verdict is capped at Comment — so a medium review reports to the user and does not post to the PR. Decline a "post comments" follow-up after either, and point at `--effort high`. +Also skip this step (independently of the gate above) if the review target is not a PR, or if the review ran at low or medium effort, or if it ran under `--topology minimal` at any effort. **Low**'s findings are unverified and must never be posted. **Medium**'s findings ARE verified (Step 4 ran), but posting is a high-only action — `--comment` forces high, and medium's verdict is capped at Comment — so a medium review reports to the user and does not post to the PR. **Minimal**'s findings are unverified and the arm posts nothing at any effort — it resolves effort high by default on a PR target while the `--user-authorized` fast path never consults the topology, so this rule is the layer that catches a run whose Step 6 decline was missed. Decline a "post comments" follow-up after any of the three, and point at `--effort high`. **Use the "Create Review" API to submit verdict + inline comments in a single call** (like Copilot Code Review). This eliminates separate summary comments — the inline comments ARE the review.