diff --git a/packages/cli/src/commands/review/capture-local.ts b/packages/cli/src/commands/review/capture-local.ts index 88559078ddf..d240a9a2945 100644 --- a/packages/cli/src/commands/review/capture-local.ts +++ b/packages/cli/src/commands/review/capture-local.ts @@ -31,6 +31,7 @@ import { stringifyPlanReport, type PlanReport, } from './lib/report.js'; +import { operatorReviewSettings } from './lib/review-settings.js'; interface CaptureLocalArgs { out: string; @@ -94,7 +95,7 @@ function runCaptureLocal(args: CaptureLocalArgs): void { // No ref to `git show` a pre-change file out of, so per-file line counts and // heaviness are unavailable — same as `plan-diff`. Chunk coverage, which is // what the topology needs, is not. - ...buildPlanReport(plan, null), + ...buildPlanReport(plan, null, operatorReviewSettings().reverseAuditRounds), untrackedFiles: capture.untracked, skippedFiles: capture.skipped, ...planEffortField(args.effort), diff --git a/packages/cli/src/commands/review/fetch-pr.test.ts b/packages/cli/src/commands/review/fetch-pr.test.ts index 7583be400a3..42f3e0c8be9 100644 --- a/packages/cli/src/commands/review/fetch-pr.test.ts +++ b/packages/cli/src/commands/review/fetch-pr.test.ts @@ -248,6 +248,10 @@ vi.mock('node:child_process', async (importOriginal) => { vi.mock('../../utils/stdioHelpers.js', () => ({ writeStdoutLine: vi.fn(), writeStderrLine: producerMocks.writeStderrLine, + // The settings fallback announces through the SAFE writer; this mock is a + // partial one, so an export it does not list is a load-time failure for + // every test in the file. + writeStderrLineSafe: producerMocks.writeStderrLine, })); vi.mock('../../services/review-worktree-lease.js', () => ({ diff --git a/packages/cli/src/commands/review/fetch-pr.ts b/packages/cli/src/commands/review/fetch-pr.ts index 1f69c8f71d2..497744b7d20 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -54,6 +54,7 @@ import { stringifyPlanReport, } from './lib/report.js'; import { resolveMergeBase, type GitProbe } from './lib/merge-base.js'; +import { operatorReviewSettings } from './lib/review-settings.js'; interface PrMetadata { headRefName: string; @@ -430,7 +431,11 @@ async function runFetchPr(args: FetchPrArgs): Promise { diffPath, diffPathAbsolute, prDescriptionHasHan: /\p{Script=Han}/u.test(meta.body ?? ''), - ...buildPlanReport(plan, (path) => fileLineCount(fetchedSha, path)), + ...buildPlanReport( + plan, + (path) => fileLineCount(fetchedSha, path), + operatorReviewSettings().reverseAuditRounds, + ), ...planEffortField(args.effort), }; diff --git a/packages/cli/src/commands/review/lib/budget.test.ts b/packages/cli/src/commands/review/lib/budget.test.ts index b44bec544c7..3bd4fa54b8c 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -13,6 +13,7 @@ import { stripBudgetGapLines, launchToolBudget, reverseAuditRoundCap, + cappedRoundTier, reviewBudget, } from './budget.js'; @@ -673,9 +674,12 @@ describe('reverseAuditRoundCap — the one reader of the plan field', () => { it('reads absent, out-of-band and garbled values as the tier', () => { // The range is floored at HUGE_REVERSE_AUDIT_ROUNDS (3) — the smallest - // cap the CLI writes — so 1 and 2 read as the tier, not as themselves: - // honouring them would force a non-converged round-cap stop where the - // full loop would have kept auditing. + // cap the CLI writes — so 1 and 2 read as the tier, not as themselves. + // Not because either always forces a non-converged stop: an all-dry loop + // DOES converge under a cap of two, since the convergence check runs + // before the cap gate. One cannot converge at all (it refuses the pair's + // second member), and two leaves no round for a loop that reports + // anything — so neither buys a cheaper review, only a capped verdict. for (const bad of [0, 1, 2, 2.5, '1', null] as unknown[]) { expect( reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: bad } }), @@ -802,3 +806,71 @@ describe('reviewBudget — the budget survives the trip through the plan', () => ).toBe(30); }); }); + +describe('cappedRoundTier — the operator ceiling may only lower a tier', () => { + const SMALL = { srcDiffLines: 100, diffLines: 100 }; + const LARGE = { srcDiffLines: 600, diffLines: 1000 }; + const HUGE = { srcDiffLines: 5000, diffLines: 5000 }; + + it('lowers each tier to what the operator asked for', () => { + expect(cappedRoundTier(SMALL, 4)).toBe(4); + expect(cappedRoundTier(SMALL, 3)).toBe(3); + expect(cappedRoundTier(LARGE, 3)).toBe(3); + expect(cappedRoundTier(SMALL, 9)).toBe(9); + }); + + it('REFUSES to raise any tier — the asymmetry is the whole knob', () => { + // A single operator-chosen count is what tiering removed: it is wrong for + // at least one topology, and most wrong for the one whose cap exists to + // stop six-hour reviews that post nothing. 20 buys nothing anywhere. + expect(cappedRoundTier(HUGE, 5)).toBe(3); + expect(cappedRoundTier(HUGE, 20)).toBe(3); + expect(cappedRoundTier(LARGE, 10)).toBe(5); + expect(cappedRoundTier(SMALL, 20)).toBe(10); + // Equal to the tier is not a lowering either — it changes nothing, and + // reading it as "honoured" would make a later tier change silently pinned + // to a number the operator picked against a different tier. + expect(cappedRoundTier(SMALL, 10)).toBe(10); + }); + + it('refuses a ceiling below the convergence minimum', () => { + // Neither one nor two buys a cheaper review, though not for the same + // reason: one refuses the convergence pair's second member so the loop can + // never reach two dry audits, while two lets an all-dry loop converge (the + // convergence check runs before the cap gate) but leaves no round for a + // loop that reports anything. Both end in a capped verdict. + for (const bad of [0, 1, 2, -3]) { + expect(cappedRoundTier(SMALL, bad)).toBe(10); + expect(cappedRoundTier(HUGE, bad)).toBe(3); + } + }); + + it('ignores a ceiling that is not a whole number', () => { + for (const bad of [3.5, Number.NaN, Number.POSITIVE_INFINITY] as number[]) { + expect(cappedRoundTier(SMALL, bad)).toBe(10); + } + expect(cappedRoundTier(SMALL, undefined)).toBe(10); + }); + + it('lowers what reviewBudget RECORDS, so every reader sees one number', () => { + // The setting has to reach the plan, not the gate: `reverseAuditRoundCap` + // clamps a stored value into the tier band, and a lowered value is inside + // it, so the reader honours it with no knowledge of the setting at all. + const b = reviewBudget({ srcDiffLines: 100, diffLines: 100 }, 4); + expect(b.reverseAuditRounds).toBe(4); + expect(reverseAuditRoundCap({ ...SMALL, budget: b })).toBe(4); + // …and an unset ceiling records the tier, exactly as before this setting. + expect( + reviewBudget({ srcDiffLines: 100, diffLines: 100 }).reverseAuditRounds, + ).toBe(10); + }); + + it('does not let the ceiling touch any other budget field', () => { + const plain = reviewBudget({ srcDiffLines: 900, diffLines: 900 }); + const capped = reviewBudget({ srcDiffLines: 900, diffLines: 900 }, 3); + expect({ ...capped, reverseAuditRounds: 0 }).toEqual({ + ...plain, + reverseAuditRounds: 0, + }); + }); +}); diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index cb984d0129a..1419880d1b2 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -257,6 +257,50 @@ export function reverseAuditRoundTier(size: DiffSize): number { : SMALL_REVERSE_AUDIT_ROUNDS; } +/** + * The round cap to record, given the topology tier and what the operator asked + * for — **the operator may only lower it.** + * + * The asymmetry is the whole design of this knob, and it is not timidity about + * letting people configure things. Raising is refused because a single + * configurable number is precisely what tiering removed: a round is one agent + * on a small diff and ~90 minutes on a huge one, so one operator-chosen count + * is wrong for at least one topology, and the topology it is most wrong for is + * the one whose cap exists to stop six-hour reviews that post nothing. + * Lowering carries no such hazard — it can only end the loop sooner. + * + * The two ways an operator actually means "run it longer" both have direct + * expressions elsewhere, and neither is a round count: "I have more wall clock + * than the huge tier assumes" is a review deadline, which the admission gate + * already prices a round against; "keep going while it is still finding real + * defects" is a property of the findings, not of a number chosen in advance. + * + * Below `HUGE_REVERSE_AUDIT_ROUNDS` is refused too, for the reason + * `reverseAuditRoundCap` refuses it in a plan — though not the reason an + * earlier draft of this gave. A cap of **one** refuses the convergence pair's + * second member, so the loop cannot produce the two dry audits convergence is + * defined by and every run stops non-converged. A cap of **two** does let an + * all-dry loop converge (the convergence check runs before the cap gate), but + * it leaves no round at all for a loop that reports anything, so the first + * finding makes the stop non-converged. Either way the purchase is a capped + * verdict rather than a cheaper review. + */ +export function cappedRoundTier( + size: DiffSize, + operatorCap: number | undefined, +): number { + const tier = reverseAuditRoundTier(size); + if ( + typeof operatorCap !== 'number' || + !Number.isInteger(operatorCap) || + operatorCap < HUGE_REVERSE_AUDIT_ROUNDS || + operatorCap >= tier + ) { + return tier; + } + return operatorCap; +} + /** * A line count this module is willing to size a plan from: a real, finite, * non-negative `number`. Everything else — absent, `null`, a numeric string, a @@ -314,8 +358,19 @@ const LINES_PER_TOOL_CALL = 20; * that lands on its floor costs one under-walked small diff. It fails toward the * cheap end on purpose — the floors are the *minimum* work, not the maximum, so * a garbled input still walks three angles and still verifies. + * + * `operatorRoundCap` is the standing `review.reverseAuditRounds` setting, read + * by the capture command and passed in rather than read here — this module has + * no imports, and a budget that loaded settings would make every caller's tests + * depend on the machine's own `~/.qwen`. It can only lower the round tier; see + * `cappedRoundTier`. Nothing else in the budget is operator-tunable, and that + * stays true: the rest of these fields size the work a review owes, and a + * caller who can shrink them is a caller who shrinks them. */ -export function reviewBudget(input: BudgetInput): ReviewBudget { +export function reviewBudget( + input: BudgetInput, + operatorRoundCap?: number, +): ReviewBudget { const src = sane(input.srcDiffLines); const total = sane(input.diffLines); @@ -350,7 +405,7 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { // tier from it would record the SMALL tier's ten rounds for a plan whose // size failed to arrive, where the flat cap recorded five. The tier does // its own usability check precisely so this call can hand it the truth. - reverseAuditRounds: reverseAuditRoundTier(input), + reverseAuditRounds: cappedRoundTier(input, operatorRoundCap), }; } @@ -388,9 +443,9 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { * * The range stays floored at `HUGE_REVERSE_AUDIT_ROUNDS`, the smallest cap * the CLI ever writes. A value of one or two is out of band (a hand-edited - * plan): honouring it would force a non-converged round-cap stop where the - * full loop would have kept auditing, so it too falls back to the tier — - * never less. + * plan): one cannot reach convergence at all, and two leaves no round for a + * loop that reports anything — see `cappedRoundTier` for why neither buys a + * cheaper review. Both fall back to the tier, never less. */ export function reverseAuditRoundCap(plan: unknown): number { const tier = reverseAuditRoundTier((plan ?? {}) as DiffSize); diff --git a/packages/cli/src/commands/review/lib/report.test.ts b/packages/cli/src/commands/review/lib/report.test.ts index 726bed3a146..f32239e2a2d 100644 --- a/packages/cli/src/commands/review/lib/report.test.ts +++ b/packages/cli/src/commands/review/lib/report.test.ts @@ -47,10 +47,14 @@ describe('buildPlanReport', () => { it('resolves the post-image through the injected dependency', () => { const plan = buildDiffPlan(editFile('src/a.ts', 3, 2), 400); const asked: string[] = []; - const report = buildPlanReport(plan, (p) => { - asked.push(p); - return 1000; - }); + const report = buildPlanReport( + plan, + (p) => { + asked.push(p); + return 1000; + }, + undefined, + ); expect(asked).toEqual(['src/a.ts']); expect(report.files[0].fileLines).toBe(1000); // pre = post - added + removed @@ -61,16 +65,20 @@ describe('buildPlanReport', () => { // 900 added lines into a file that ends up 1 000 long: it existed at 100 // lines, so it is not "large enough before" — not heavy. const plan = buildDiffPlan(editFile('src/a.ts', 3, 900), 400); - expect(buildPlanReport(plan, () => 1000).files[0].heavy).toBe(false); + expect(buildPlanReport(plan, () => 1000, undefined).files[0].heavy).toBe( + false, + ); // Same change into a file that ends up 6 000 long: it existed at 5 100, // and 900 changed lines clears the volume threshold. - expect(buildPlanReport(plan, () => 6000).files[0].heavy).toBe(true); + expect(buildPlanReport(plan, () => 6000, undefined).files[0].heavy).toBe( + true, + ); }); it('treats a null resolver as "no tree to read", so nothing is heavy', () => { // `plan-diff` has a bare diff file and no ref. It must not guess. const plan = buildDiffPlan(addFile('src/big.ts', 2000), 400); - const report = buildPlanReport(plan, null); + const report = buildPlanReport(plan, null, undefined); expect(report.files[0].fileLines).toBe(0); expect(report.files[0].preLines).toBe(0); expect(report.files[0].heavy).toBe(false); @@ -84,10 +92,14 @@ describe('buildPlanReport', () => { 'Binary files a/logo.png and b/logo.png differ', ].join('\n'); const asked: string[] = []; - const report = buildPlanReport(buildDiffPlan(diff, 400), (p) => { - asked.push(p); - return 500; - }); + const report = buildPlanReport( + buildDiffPlan(diff, 400), + (p) => { + asked.push(p); + return 500; + }, + undefined, + ); expect(asked).toEqual([]); expect(report.files[0].binary).toBe(true); expect(report.files[0].heavy).toBe(false); @@ -95,8 +107,10 @@ describe('buildPlanReport', () => { it('emits addedRanges only on heavy files', () => { const diff = editFile('src/heavy.ts', 3, 900) + addFile('src/light.ts', 20); - const report = buildPlanReport(buildDiffPlan(diff, 400), (p) => - p === 'src/heavy.ts' ? 6000 : 30, + const report = buildPlanReport( + buildDiffPlan(diff, 400), + (p) => (p === 'src/heavy.ts' ? 6000 : 30), + undefined, ); const heavy = report.files.find((f) => f.path === 'src/heavy.ts')!; const light = report.files.find((f) => f.path === 'src/light.ts')!; @@ -121,7 +135,11 @@ describe('buildPlanReport', () => { '-gone2', '', ].join('\n'); - const report = buildPlanReport(buildDiffPlan(diff, 400), () => 100); + const report = buildPlanReport( + buildDiffPlan(diff, 400), + () => 100, + undefined, + ); expect(report.files[0].hunks).toEqual([{ newStart: 1, newEnd: 2 }]); }); @@ -130,7 +148,11 @@ describe('buildPlanReport', () => { // `clearTimeout()` leaves nothing behind. This range points it at the `-` // lines that are the only evidence the call ever existed. const diff = editFile('src/heavy.ts', 3, 900); - const report = buildPlanReport(buildDiffPlan(diff, 400), () => 6000); + const report = buildPlanReport( + buildDiffPlan(diff, 400), + () => 6000, + undefined, + ); const f = report.files[0]; expect(f.heavy).toBe(true); expect(f.diffRange).toEqual({ @@ -143,6 +165,7 @@ describe('buildPlanReport', () => { const report = buildPlanReport( buildDiffPlan(addFile('src/a.ts', 20), 400), () => 30, + undefined, ); expect(report.files[0].heavy).toBe(false); expect(report.files[0].diffRange).toBeUndefined(); @@ -155,7 +178,7 @@ describe('buildPlanReport', () => { addFile('docs/g.md', 30) + addFile('package-lock.json', 40); const plan = buildDiffPlan(diff, 400); - const report = buildPlanReport(plan, () => 100); + const report = buildPlanReport(plan, () => 100, undefined); expect(report.srcDiffLines).toBe(plan.srcDiffLines); expect(report.testDiffLines).toBe(plan.testDiffLines); expect(report.docsDiffLines).toBe(plan.docsDiffLines); @@ -167,8 +190,10 @@ describe('buildPlanReport', () => { describe('stringifyPlanReport', () => { it('round-trips: the collapsed text parses back to the same object', () => { const diff = editFile('src/heavy.ts', 3, 900) + addFile('src/light.ts', 20); - const report = buildPlanReport(buildDiffPlan(diff, 400), (p) => - p === 'src/heavy.ts' ? 6000 : 30, + const report = buildPlanReport( + buildDiffPlan(diff, 400), + (p) => (p === 'src/heavy.ts' ? 6000 : 30), + undefined, ); expect(JSON.parse(stringifyPlanReport(report))).toEqual(report); }); @@ -177,6 +202,7 @@ describe('stringifyPlanReport', () => { const report = buildPlanReport( buildDiffPlan(editFile('src/heavy.ts', 3, 900), 400), () => 6000, + undefined, ); const text = stringifyPlanReport(report); // Not one giant line: `read_file` pages at line boundaries, so a compact @@ -193,6 +219,7 @@ describe('stringifyPlanReport', () => { const report = buildPlanReport( buildDiffPlan(editFile('src/heavy.ts', 3, 900), 400), () => 6000, + undefined, ); const collapsed = stringifyPlanReport(report).length; const indented = JSON.stringify(report, null, 2).length + 1; @@ -211,7 +238,11 @@ describe('stringifyPlanReport', () => { '+x', '', ].join('\n'); - const report = buildPlanReport(buildDiffPlan(diff, 400), () => 1); + const report = buildPlanReport( + buildDiffPlan(diff, 400), + () => 1, + undefined, + ); const parsed = JSON.parse(stringifyPlanReport(report)) as typeof report; expect(parsed.files[0].path).toBe(weird); }); diff --git a/packages/cli/src/commands/review/lib/report.ts b/packages/cli/src/commands/review/lib/report.ts index 3991fa496f6..db1da2a7998 100644 --- a/packages/cli/src/commands/review/lib/report.ts +++ b/packages/cli/src/commands/review/lib/report.ts @@ -106,10 +106,20 @@ export interface PlanReport { * `postImageLines` resolves a path's line count in the post-change tree. It is * null when there is no tree to resolve against — a bare diff file — in which * case heaviness cannot be decided and no file is heavy. + * + * `operatorRoundCap` is the standing `review.reverseAuditRounds` setting, which + * may only lower the reverse-audit round tier. It is a **required** parameter, + * and deliberately not resolved in here: three capture commands build a plan, + * an optional parameter is one a call site can quietly omit, and a setting that + * silently applies to two of the three review entry points is worse than one + * that applies to none. Passing `undefined` is how a caller says "no operator + * ceiling" — visibly, at the call site. Reading the setting here instead would + * also make this builder's tests depend on the machine's own `~/.qwen`. */ export function buildPlanReport( plan: DiffPlan, postImageLines: ((path: string) => number) | null, + operatorRoundCap: number | undefined, ): PlanReport { const files = plan.files.map((f): FileMetric => { const changedLines = f.addedLines + f.removedLines; @@ -162,10 +172,13 @@ export function buildPlanReport( generatedDiffLines: plan.generatedDiffLines, chunks: plan.chunks, files, - budget: reviewBudget({ - srcDiffLines: plan.srcDiffLines, - diffLines: plan.diffLines, - }), + budget: reviewBudget( + { + srcDiffLines: plan.srcDiffLines, + diffLines: plan.diffLines, + }, + operatorRoundCap, + ), }; } diff --git a/packages/cli/src/commands/review/lib/review-settings.test.ts b/packages/cli/src/commands/review/lib/review-settings.test.ts index cfc6503dee8..451aae11bc2 100644 --- a/packages/cli/src/commands/review/lib/review-settings.test.ts +++ b/packages/cli/src/commands/review/lib/review-settings.test.ts @@ -7,6 +7,29 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const loadSettingsMock = vi.hoisted(() => vi.fn()); +const stderr = vi.hoisted(() => vi.fn()); +// The two writers are NOT interchangeable, so the mock must not make them so: +// `writeStderrLine` throws what the underlying write throws, and +// `writeStderrLineSafe` swallows it. Mapping both to one non-throwing spy +// would mock away the exact distinction the degrade path depends on. +const stderrSafe = vi.hoisted(() => + vi.fn((message: string) => { + try { + stderr(message); + } catch { + /* the real safe writer swallows a failed write */ + } + }), +); +vi.mock('../../../utils/stdioHelpers.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + writeStderrLine: stderr, + writeStderrLineSafe: stderrSafe, + }; +}); vi.mock('../../../config/settings.js', async (importOriginal) => { const actual = await importOriginal(); @@ -22,6 +45,8 @@ function setReview(review: unknown): void { describe('operatorReviewSettings', () => { beforeEach(() => { loadSettingsMock.mockReset(); + stderr.mockReset(); + stderrSafe.mockClear(); }); it('resolves from operator scopes only — a repository must not set review policy', () => { @@ -102,6 +127,81 @@ describe('operatorReviewSettings', () => { expect(operatorReviewSettings().comment).toBe(false); } }); + + it('degrades to the safe defaults when the settings cannot be loaded', () => { + // `loadSettings` throws a FatalConfigError when any settings file fails to + // read or parse, and this is now read while the plan is being captured — + // the review's first step. A stray comma must not end the review. + loadSettingsMock.mockImplementation(() => { + throw new Error('Error in /home/u/.qwen/settings.json: Unexpected token'); + }); + expect(operatorReviewSettings()).toEqual({ + attribution: true, + comment: false, + effort: undefined, + reverseAuditRounds: undefined, + }); + // Every default is the conservative side: a review that loses its operator + // policy loses it toward doing more work and writing nothing public. + expect(stderr).toHaveBeenCalledWith( + expect.stringContaining('review settings could not be loaded'), + ); + }); + + it('survives a stderr that throws while announcing the degrade', () => { + // The degrade exists so a broken settings file cannot end a review. If it + // announces itself with the THROWING writer, the announcement can end the + // review instead — `process.stderr.write` throws on EPIPE or a closed fd, + // which is reachable whenever the reader goes away (`qwen … | head`) or a + // daemon redirects its stderr. Both failures at once is exactly the case + // this path was written for. + loadSettingsMock.mockImplementation(() => { + throw new Error('Error in /home/u/.qwen/settings.json: Unexpected token'); + }); + stderr.mockImplementation(() => { + throw Object.assign(new Error('write EPIPE'), { code: 'EPIPE' }); + }); + expect(() => operatorReviewSettings()).not.toThrow(); + expect(operatorReviewSettings()).toEqual({ + attribution: true, + comment: false, + effort: undefined, + reverseAuditRounds: undefined, + }); + }); + + it('passes a real reverse-audit ceiling through as a number', () => { + for (const rounds of [3, 4, 9, 100]) { + setReview({ reverseAuditRounds: rounds }); + expect(operatorReviewSettings().reverseAuditRounds).toBe(rounds); + } + // Whether the number is USABLE is not this module's ruling — 100 is passed + // through here and refused by `cappedRoundTier`, which is the only place + // that knows the plan's tier. + }); + + it('reads a garbled or unset ceiling as absent, never as a request for zero', () => { + // The schema default is 0, meaning "not set". A 0 that reached the budget + // as a real request would be a request for a zero-round reverse audit — + // which is why "the operator chose nothing" must not be representable as + // a number at all. + for (const rounds of [ + undefined, + 0, + -1, + 3.5, + '5', + true, + null, + Number.NaN, + Number.POSITIVE_INFINITY, + ]) { + setReview({ reverseAuditRounds: rounds }); + expect(operatorReviewSettings().reverseAuditRounds).toBeUndefined(); + } + setReview({}); + expect(operatorReviewSettings().reverseAuditRounds).toBeUndefined(); + }); }); describe('review settings in the /settings dialog', () => { @@ -114,5 +214,6 @@ describe('review settings in the /settings dialog', () => { expect(dialogKeys).toContain('review.effort'); expect(dialogKeys).toContain('review.comment'); expect(dialogKeys).toContain('review.severityFloor'); + expect(dialogKeys).toContain('review.reverseAuditRounds'); }); }); diff --git a/packages/cli/src/commands/review/lib/review-settings.ts b/packages/cli/src/commands/review/lib/review-settings.ts index 87faed915b6..12f15fe9627 100644 --- a/packages/cli/src/commands/review/lib/review-settings.ts +++ b/packages/cli/src/commands/review/lib/review-settings.ts @@ -5,6 +5,15 @@ */ import { loadSettings } from '../../../config/settings.js'; +import { writeStderrLineSafe } from '../../../utils/stdioHelpers.js'; + +/** What every field reads as when the settings cannot be loaded at all. */ +const SAFE_DEFAULTS: OperatorReviewSettings = { + attribution: true, + comment: false, + effort: undefined, + reverseAuditRounds: undefined, +}; export interface OperatorReviewSettings { attribution: boolean; @@ -17,6 +26,21 @@ export interface OperatorReviewSettings { effort?: string; /** The raw `review.severityFloor` value when set — same caveats as effort. */ severityFloor?: string; + /** + * The operator's reverse-audit round ceiling, when they set a real one. + * + * Only a positive integer survives; `0` (the schema default, meaning "not + * set"), a fraction, a negative, a string and a `NaN` all read as absent, + * because the difference between "the operator chose a number" and "the + * operator chose nothing" is the whole meaning of this field — a garbled + * value that fell through as `0` would otherwise read as a request for a + * zero-round audit. + * + * Whether the number is *usable* is not decided here: this module reports + * what the operator asked for, and `reviewBudget` decides what the plan's + * topology can honour (it may only lower a tier, never raise it). + */ + reverseAuditRounds?: number; } /** @@ -28,13 +52,40 @@ export interface OperatorReviewSettings { * its model (`attribution`), or how deeply the pipeline verifies (`effort`). */ export function operatorReviewSettings(): OperatorReviewSettings { - const review = loadSettings(undefined, { skipWorkspaceSettings: true }).merged - .review; + // `loadSettings` throws a FatalConfigError when a settings file cannot be + // READ (or its migration fails) — malformed JSON does not reach that path, + // it is copied aside and recovered ("Never crash due to a corrupted settings + // file"). An unreadable file is enough: this is read while a plan is being + // captured, the review's first step, so the throw would end the whole review + // over a permissions bit on a file none of these settings had to come from. + // Degrade to + // the defaults instead and say so: every default here is the conservative + // side (attribution on, no auto-posting, no effort override, no round + // ceiling), so a review that loses its operator policy loses it toward doing + // MORE work and writing nothing public — never the other way. + let review; + try { + review = loadSettings(undefined, { skipWorkspaceSettings: true }).merged + .review; + } catch (error) { + // The SAFE writer, not the throwing one. `process.stderr.write` throws on + // EPIPE or a closed fd, and this NOTE is incidental to the degrade — a + // throw here would propagate out of the catch and end the review by the + // very path added to stop a broken settings file from ending it. + writeStderrLineSafe( + `NOTE: review settings could not be loaded (${ + error instanceof Error ? error.message.split('\n')[0] : String(error) + }); this review uses the defaults — attribution on, no auto-posting, no ` + + `effort or round-cap override. Fix the settings file to restore them.`, + ); + return { ...SAFE_DEFAULTS }; + } // Settings loading performs no per-value type validation — the inferred // `boolean` types do not hold for hand-edited files (`"false"` as a quoted // string is the classic mistake), so each value is re-checked here. A // non-boolean `attribution` falls back to the schema default (on); a // non-boolean `comment` never enables auto-posting. + const rounds = review?.reverseAuditRounds; return { attribution: typeof review?.attribution === 'boolean' ? review.attribution : true, @@ -44,5 +95,9 @@ export function operatorReviewSettings(): OperatorReviewSettings { typeof review?.severityFloor === 'string' ? review.severityFloor : undefined, + reverseAuditRounds: + typeof rounds === 'number' && Number.isInteger(rounds) && rounds > 0 + ? rounds + : undefined, }; } diff --git a/packages/cli/src/commands/review/plan-diff.ts b/packages/cli/src/commands/review/plan-diff.ts index 93b75a52de7..ffbd69b46bd 100644 --- a/packages/cli/src/commands/review/plan-diff.ts +++ b/packages/cli/src/commands/review/plan-diff.ts @@ -37,6 +37,7 @@ import { type PlanReport, stringifyPlanReport, } from './lib/report.js'; +import { operatorReviewSettings } from './lib/review-settings.js'; interface PlanDiffArgs { diff_path: string; @@ -124,7 +125,7 @@ function runPlanDiff(args: PlanDiffArgs): void { // No `git show` is possible here — there is no ref to resolve a path // against — so per-file line counts and heaviness are unavailable. Chunk // coverage, which is what Step 3B needs, is not. - ...buildPlanReport(plan, null), + ...buildPlanReport(plan, null, operatorReviewSettings().reverseAuditRounds), ...planEffortField(args.effort), }; diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 4f85b0cf802..d48df80527d 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -768,6 +768,16 @@ const SETTINGS_SCHEMA = { { value: 'suggestion', label: 'Suggestions and Criticals' }, ], }, + reverseAuditRounds: { + type: 'number', + label: 'Reverse-audit round ceiling: review', + category: 'General', + requiresRestart: false, + default: 0, + description: + 'Lower the reverse-audit loop\'s round cap for every high-effort review. The cap is normally chosen from the diff topology (10 small / 5 chunked / 3 huge) because a round costs one agent on a small diff and ~90 minutes on a huge one; this setting can only LOWER whichever tier applies, never raise it — an unset or out-of-range value (below 3, or above the plan\'s own tier) leaves the tier alone. Understand what it buys before enabling: the loop ends on two consecutive dry rounds, so cutting the cap does not make reviews converge sooner, it makes them stop before converging more often — and every such stop is disclosed as unreviewed scope and caps the verdict at Comment, so a cheaper review is also one that can no longer Approve. To spend LESS on reviews generally, prefer "effort"; to let a productive loop run longer, set a review deadline rather than raising a round count. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.', + showInDialog: true, + }, }, }, output: { diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index a0089122169..dbbfc08f892 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -109,6 +109,18 @@ So the cap is read from the plan's topology tier (`reverseAuditRoundTier`): **10 What this does **not** change: a cap stop is still a non-converged stop. It writes the marker, caps the verdict, and owes its `unreviewedDimensions` entry, at ten exactly as at five. +### Why the operator ceiling can only lower a tier + +`review.reverseAuditRounds` lets an operator cut the round cap for every review they run. It cannot raise one, and the asymmetry is not timidity about configuration — it is the same argument tiering is built on, applied to a knob. + +A single operator-chosen count is exactly what tiering removed. A round is one agent on a 3A diff and ~90 minutes on a huge one, so one number is wrong for at least one topology, and the topology it is most wrong for is the one whose cap exists to stop six-hour reviews that post nothing. An operator who sets `8` has said something sensible about small diffs and something dangerous about large ones, and the setting cannot tell which they meant. Lowering carries no matching hazard: it can only end the loop sooner, and the floor at `HUGE_REVERSE_AUDIT_ROUNDS` keeps it above the point where a cap would pre-empt the two-consecutive-dry rule. + +The two things an operator means by "let it run longer" both have direct expressions that a round count only approximates. "My ceiling is not the six hours the huge tier assumes" is a **deadline**, and the admission gate already prices a round plus its reserve against one. "Keep going while it is still finding real defects" is a property of the **findings**, not of a number chosen before the review starts. Answering either with a bigger integer is answering a question about time or evidence with a question about counting. + +The setting is also deliberately not a flag. It resolves in the capture command and lands in `plan.budget.reverseAuditRounds`, so every reader — the admission gate, the cold-check note, `compose-review` — sees one number and none of them learns a setting was involved. That is the module's standing rule (a budget the caller passes is a budget the caller can inflate) satisfied rather than excepted: the operator sets a standing policy, the CLI resolves it once, and no per-invocation caller gets to name a budget. The reader needs no new code at all — a lowered value is inside the tier's `[3, tier]` band, which `reverseAuditRoundCap` already honours. + +One consequence belongs in the setting's own description, and is there: cutting the cap does not make reviews converge sooner, because the loop ends on two consecutive dry rounds. It makes them stop **before** converging more often — and every such stop is disclosed as unreviewed scope and caps the verdict at `COMMENT`. A cheaper review is also one that can no longer Approve. + ### Why the reverse audit fans out per chunk The original design gave one agent the whole diff plus a growing cumulative finding list. On a 5 800-line diff that is the most context-starved agent in the pipeline — exactly on the PRs where reverse audit matters most. Under Step 3B each round runs one auditor per chunk, each with the full cumulative finding list but only its own territory to re-read. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 666795c76dc..14e2323f223 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -204,9 +204,8 @@ Read from it: - `diffLines`, `diffChars`, and `srcDiffLines` / `testDiffLines` / `docsDiffLines` / `generatedDiffLines` - `chunks[]` — contiguous, non-overlapping line ranges tiling the whole diff. Each entry has `id`, `startLine`, `endLine` (1-based, inclusive), `lines`, `chars`, an `oversized` flag, and `files[]` naming the source files and new-side line ranges it covers. A chunk with `oversized: true` may exceed what one `read_file` call returns. - `files[]` — per-file `kind` (`source` / `test` / `generated`), `hunks[]` new-side ranges (Step 7 validates comment anchors against these), `addedRanges[]` and `diffRange` (present only on `heavy` files — the exact lines the PR wrote, and where that file's own diff lives, so an invariant agent can see what was deleted), change counts, and the `heavy` flag -- `budget` — how much walking the **size-elastic** parts of this run owe, sized from `srcDiffLines` except that an all-non-source diff (docs, lockfiles) counts its total lines at an eighth rate, so the size these tiers read is `effective = max(srcDiffLines, floor(diffLines / 8))`; recorded here rather than passed as a flag so every reader sees one number. `inlineAngles` and `sweep` scope Step 3C's low pass; `specialistCap` is the Agent 8 ceiling (**0** below 80 source lines — "one domain dominates the diff" is a judgement, and a judgement made about forty lines finds a dominant domain every time, because forty lines are usually all one thing — **and 0 again for a huge diff (effective ≥ 3000)**, where an Agent 8 whole-diff pass on top of the base fan-out is the marginal cost that tips a review too big to finish into posting nothing); `verifyShard` is Step 4's findings-per-verifier; `reverseAuditRounds` is the reverse-audit loop's round cap, **one value per topology**: **10** on a Step 3A diff, **5** on a Step 3B one, **3 for a huge diff** (effective ≥ 3000 lines). One number cannot price all three, because what is being capped is a _round_ and a round costs one auditor on 3A, one auditor per non-retired chunk on 3B, and ~90 minutes on a 4,000-line PR — where five rounds (450 min) alone exceed the six-hour ceiling before the fan-out and tail are counted, and the 6-hour timeouts that posted nothing were 4,000-5,300-line PRs (measured; DESIGN.md — The six-hour timeouts). Ten on 3A because the marginal round there is a single agent against a whole review of 17-28 calls: five was the 3B arithmetic applied where it does not hold, and it stopped loops that were still confirming Criticals to save ~5 calls. Three when huge is one audit round above the convergence floor of two — the all-dry rounds-1-and-2 shape converges under any cap of two or more, since the convergence check runs before the cap gate; the extra round buys hot chunks one more pass. The `agent-prompt` builder enforces the cap itself (a `ROUND CAP:` refusal, exit 4, that writes a marker `compose-review` caps on — same contract as the deadline gate below), so you never count rounds yourself. `agentToolBudget` is the base rate of the soft tool-call ceiling `agent-prompt` bakes into every finder and auditor brief — not the verifier's, not Agent 7's, and not Agent 0's, whose mandatory work scales with the linked issues rather than the diff. The ceiling is per **launch**: a scoped agent (a chunk, a heavy file) gets an allowance derived from its own territory — never above the plan's recorded allowance, which is clamped into the budget's own band in both directions, so the plan stays the one number every launch answers to — and every launch's assigned reads ride on top of the allowance rather than inside it, so a huge diff's mandatory chunk reads can never exhaust the exploration a whole-diff role owes — because a wave's wall clock is its slowest agent and the slowest agent is reliably one that kept exploring past any recall gain: the same 14-agent fan-out has measured 11.7 and 41 minutes on comparable diffs, the difference being individual agents spending 40-100 calls walking the tree (measured; DESIGN.md — The forty-one minute wave). The ceiling is soft and the briefs restate the recall rule beside it: at the budget an agent stops **exploring**, never reporting — findings in hand are filed, and each stopped check is disclosed on its own line in the fixed form `Budget gap: `, which `check-coverage` parses out of the transcripts (its report's `budgetGaps`) — see Step 3D for the ruling each gap is owed. **It never scales a dimension away** — which agents a review owes is the roster's answer and the roster reads `effort`, so a size input cannot become a back door into shrinking coverage. Nothing here is yours to override: a budget the caller can inflate is a budget that gets inflated. **A plan with no `budget` field** (written by an older CLI — the version-skew this skill has already measured once) falls back to the pre-budget flat behaviour: walk all six angles, run the sweep, cap Agent 8 at 2, shard verification at 8. Those four err toward more coverage, never less. The round cap is the one exception and is worth naming rather than lumping in: a field-less **huge** plan reads 3 where the flat fallback read 5 — deliberately _less_, because that tier is a finishability ruling and the reviews it exists for are the ones that ran six hours and posted nothing. - -A chunk is read with `read_file(file_path=diffPathAbsolute, offset=startLine - 1, limit=endLine - startLine + 1)` — `offset` is 0-based. +- `budget` — how much walking the **size-elastic** parts of this run owe, sized from `srcDiffLines` except that an all-non-source diff (docs, lockfiles) counts its total lines at an eighth rate, so the size these tiers read is `effective = max(srcDiffLines, floor(diffLines / 8))`; recorded here rather than passed as a flag so every reader sees one number. `inlineAngles` and `sweep` scope Step 3C's low pass; `specialistCap` is the Agent 8 ceiling (**0** below 80 source lines — "one domain dominates the diff" is a judgement, and a judgement made about forty lines finds a dominant domain every time, because forty lines are usually all one thing — **and 0 again for a huge diff (effective ≥ 3000)**, where an Agent 8 whole-diff pass on top of the base fan-out is the marginal cost that tips a review too big to finish into posting nothing); `verifyShard` is Step 4's findings-per-verifier; `reverseAuditRounds` is the reverse-audit loop's round cap, **one value per topology**: **10** on a Step 3A diff, **5** on a Step 3B one, **3 for a huge diff** (effective ≥ 3000 lines). One number cannot price all three, because what is being capped is a _round_ and a round costs one auditor on 3A, one auditor per non-retired chunk on 3B, and ~90 minutes on a 4,000-line PR — where five rounds (450 min) alone exceed the six-hour ceiling before the fan-out and tail are counted, and the 6-hour timeouts that posted nothing were 4,000-5,300-line PRs (measured; DESIGN.md — The six-hour timeouts). Ten on 3A because the marginal round there is a single agent against a whole review of 17-28 calls: five was the 3B arithmetic applied where it does not hold, and it stopped loops that were still confirming Criticals to save ~5 calls. Three when huge is one audit round above the convergence floor of two — the all-dry rounds-1-and-2 shape converges under any cap of two or more, since the convergence check runs before the cap gate; the extra round buys hot chunks one more pass. An operator may LOWER the tier for every review through the `review.reverseAuditRounds` setting (user scope only, floored at 3) — the capture command resolves it into this field, so you read one number here either way and never learn that a setting was involved; it can never RAISE a tier. The `agent-prompt` builder enforces the cap itself (a `ROUND CAP:` refusal, exit 4, that writes a marker `compose-review` caps on — same contract as the deadline gate below), so you never count rounds yourself. `agentToolBudget` is the base rate of the soft tool-call ceiling `agent-prompt` bakes into every finder and auditor brief — not the verifier's, not Agent 7's, and not Agent 0's, whose mandatory work scales with the linked issues rather than the diff. The ceiling is per **launch**: a scoped agent (a chunk, a heavy file) gets an allowance derived from its own territory — never above the plan's recorded allowance, which is clamped into the budget's own band in both directions, so the plan stays the one number every launch answers to — and every launch's assigned reads ride on top of the allowance rather than inside it, so a huge diff's mandatory chunk reads can never exhaust the exploration a whole-diff role owes — because a wave's wall clock is its slowest agent and the slowest agent is reliably one that kept exploring past any recall gain: the same 14-agent fan-out has measured 11.7 and 41 minutes on comparable diffs, the difference being individual agents spending 40-100 calls walking the tree (measured; DESIGN.md — The forty-one minute wave). The ceiling is soft and the briefs restate the recall rule beside it: at the budget an agent stops **exploring**, never reporting — findings in hand are filed, and each stopped check is disclosed on its own line in the fixed form `Budget gap: `, which `check-coverage` parses out of the transcripts (its report's `budgetGaps`) — see Step 3D for the ruling each gap is owed. **It never scales a dimension away** — which agents a review owes is the roster's answer and the roster reads `effort`, so a size input cannot become a back door into shrinking coverage. Nothing here is yours to override: a budget the caller can inflate is a budget that gets inflated. **A plan with no `budget` field** (written by an older CLI — the version-skew this skill has already measured once) falls back to the pre-budget flat behaviour: walk all six angles, run the sweep, cap Agent 8 at 2, shard verification at 8. Those four err toward more coverage, never less. The round cap is the one exception and is worth naming rather than lumping in: a field-less **huge** plan reads 3 where the flat fallback read 5 — deliberately _less_, because that tier is a finishability ruling and the reviews it exists for are the ones that ran six hours and posted nothing. + A chunk is read with `read_file(file_path=diffPathAbsolute, offset=startLine - 1, limit=endLine - startLine + 1)` — `offset` is 0-based. For **local-diff and file-path reviews**, capture and plan in one command: diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 7ae56c51aa7..46a97e944b9 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -254,6 +254,11 @@ "suggestion" ], "default": "auto" + }, + "reverseAuditRounds": { + "description": "Lower the reverse-audit loop's round cap for every high-effort review. The cap is normally chosen from the diff topology (10 small / 5 chunked / 3 huge) because a round costs one agent on a small diff and ~90 minutes on a huge one; this setting can only LOWER whichever tier applies, never raise it — an unset or out-of-range value (below 3, or above the plan's own tier) leaves the tier alone. Understand what it buys before enabling: the loop ends on two consecutive dry rounds, so cutting the cap does not make reviews converge sooner, it makes them stop before converging more often — and every such stop is disclosed as unreviewed scope and caps the verdict at Comment, so a cheaper review is also one that can no longer Approve. To spend LESS on reviews generally, prefer \"effort\"; to let a productive loop run longer, set a review deadline rather than raising a round count. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.", + "type": "number", + "default": 0 } } },