diff --git a/docs/users/configuration/settings.md b/docs/users/configuration/settings.md index d0e227591d7..bcc9c255f81 100644 --- a/docs/users/configuration/settings.md +++ b/docs/users/configuration/settings.md @@ -109,12 +109,13 @@ Settings are organized into categories. Most settings should be placed within th #### review -| Setting | Type | Description | Default | -| ---------------------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | -| `review.attribution` | boolean | Append the attribution footer naming the model and CLI version (e.g. `_— qwen3-coder via Qwen Code /review (v0.21.2)_`) to review bodies and inline comments posted by `/review`. Disable to post reviews without AI attribution. With the footer off, presubmit duplicate detection still recognizes earlier posts by the same GitHub account, but footer-less posts from other accounts escape it. | `true` | -| `review.effort` | enum | Default effort for `/review` when `--effort` is not given: `"low"`, `"medium"`, `"high"`, or `"auto"` (the built-in rule: high for PRs, medium for local changes). An explicit `--effort` wins; an effective `--comment` still forces high and `--fix` still floors at medium. | `"auto"` | -| `review.comment` | boolean | Treat every PR `/review` as if `--comment` was passed: findings are posted to the pull request without the flag. The post still binds to the PR named in the invocation. Enable only if you always want reviews published. | `false` | -| `review.severityFloor` | enum | The lowest severity a PR `/review` posts when `--severity-floor` is not given: `"auto"` (the round-adaptive default — Suggestions post through round 5, only Criticals from round 6, with otherwise-postable high-confidence Suggestions recorded and deferred, and rounds 2–5 deferring new Suggestions on code unchanged since the previous round; low-confidence and Nice-to-have findings stay terminal-only), `"critical"` (that posture from round 1), or `"suggestion"` (Suggestions post at every round; turns the convergence posture off). Non-PR targets have no rounds and ignore this. | `"auto"` | +| Setting | Type | Description | Default | +| --------------------------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | +| `review.attribution` | boolean | Append the attribution footer naming the model and CLI version (e.g. `_— qwen3-coder via Qwen Code /review (v0.21.2)_`) to review bodies and inline comments posted by `/review`. Disable to post reviews without AI attribution. With the footer off, presubmit duplicate detection still recognizes earlier posts by the same GitHub account, but footer-less posts from other accounts escape it. | `true` | +| `review.effort` | enum | Default effort for `/review` when `--effort` is not given: `"low"`, `"medium"`, `"high"`, or `"auto"` (the built-in rule: high for PRs, medium for local changes). An explicit `--effort` wins; an effective `--comment` still forces high and `--fix` still floors at medium. | `"auto"` | +| `review.comment` | boolean | Treat every PR `/review` as if `--comment` was passed: findings are posted to the pull request without the flag. The post still binds to the PR named in the invocation. Enable only if you always want reviews published. | `false` | +| `review.severityFloor` | enum | The lowest severity a PR `/review` posts when `--severity-floor` is not given: `"auto"` (the round-adaptive default — Suggestions post through round 5, only Criticals from round 6, with otherwise-postable high-confidence Suggestions recorded and deferred, and rounds 2–5 deferring new Suggestions on code unchanged since the previous round; low-confidence and Nice-to-have findings stay terminal-only), `"critical"` (that posture from round 1), or `"suggestion"` (Suggestions post at every round; turns the convergence posture off). Non-PR targets have no rounds and ignore this. | `"auto"` | +| `review.reverseAuditRounds` | number | Lower the reverse-audit loop's round cap for every high-effort review. The cap otherwise follows the diff topology (10 small / 5 chunked; a huge diff is 3 with a review deadline and 5 without). This can only **lower** whichever tier applies: a value below 3, above the tier, or not a whole number above zero is ignored. Cutting the cap does not make reviews converge sooner — the loop ends on two consecutive dry rounds — it makes them stop before converging more often, and every such stop caps the verdict at Comment. | `0` (unset) | These settings are read from operator scopes only (User, System, and SystemDefaults); values in a workspace `.qwen/settings.json` are ignored, so a repository cannot set review policy for its reviewers. diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index 17dced8d76e..48f7008f7ef 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -83,7 +83,7 @@ Step 3B: high, >500 src OR >3200 total: territory x dim. [N+5..7+3H calls] Step 4: Deduplicate --> Sharded verify (<=8 findings each) --> Aggregate [ceil(F/8) calls, F=findings] Step 5: Iterative reverse audit, fanned out per chunk; - stop after 2 consecutive dry rounds (cap 5) + stop after 2 consecutive dry rounds (cap 10/5/3 by topology) Step 6: Present findings + verdict (high; low pass: findings only) Canonicalize findings -> .qwen/tmp/...-findings.json Step 6B: Apply findings + record per-finding outcomes (--fix only) @@ -124,7 +124,7 @@ A **source** file that is largely rewritten (an existing file of 300+ lines that The checklist is split three ways on purpose. Handing one agent all eight checks over a 2 400-line file gets one of them done properly; three agents with two or three checks each get all of them done. Chunk agents do not substitute for this — on PR #6457 they held every one of these defects inside their assigned territory and reported none. What they lacked was not the lines but the question. -Findings are verified in **sharded batches** (at most 8 findings per verification agent, all launched together). A verifier may reject a Critical only by quoting the code that contradicts it (or when the diff's own comments document the flagged behavior as deliberate); anything less certain is downgraded to low confidence rather than deleted — a silently rejected Critical is invisible to every later stage, while a downgraded one still reaches a human. After verification, **iterative reverse audit** hunts for gaps, fanned out one auditor per chunk per round, each with the cumulative finding list. The loop stops after **two consecutive dry rounds** (or 5 rounds, hard cap — reported as such rather than as convergence). One dry round is not evidence of convergence, and reverse-audit findings are verified like any other. +Findings are verified in **sharded batches** (at most 8 findings per verification agent, all launched together). A verifier may reject a Critical only by quoting the code that contradicts it (or when the diff's own comments document the flagged behavior as deliberate); anything less certain is downgraded to low confidence rather than deleted — a silently rejected Critical is invisible to every later stage, while a downgraded one still reaches a human. After verification, **iterative reverse audit** hunts for gaps, fanned out one auditor per chunk per round, each with the cumulative finding list. The loop stops after **two consecutive dry rounds** (or at the plan's round cap — reported as such rather than as convergence). That cap follows the diff's topology: **10** on a small diff, where a round is a single auditor; **5** on a chunked one, where it is one auditor per chunk; and **3** on a huge diff (≥ 3000 effective lines) _when the run has a deadline_, because five ~90-minute rounds do not fit a six-hour CI ceiling and a review killed mid-flight posts nothing — with no deadline a huge diff keeps the chunked cap of 5. An operator can lower whichever cap applies for every review with the `review.reverseAuditRounds` setting; it can never raise one. One dry round is not evidence of convergence, and reverse-audit findings are verified like any other. ## Severity Levels @@ -421,12 +421,12 @@ Why the floors are where they are: on a nine-line typo fix, six inline walks are The high-effort pipeline bounds each stage (shard size, audit rounds), but total calls scale with findings — `ceil(F/8)` verification shards — and, under 3B, with chunk count (reverse audit runs per chunk per round). Typical 3A profile: -| Stage | LLM calls | Notes | -| -------------------------------- | ------------------------------ | -------------------------------------------------------------------------------------------------------------- | -| Review agents (Step 3) | 14 (+0-2) | Run in parallel; cross-repo skips Agents 1c and 7 (12), local/file skips Agent 0 (13) | -| Sharded verification (Step 4) | ceil(F/8) | F = findings; at most 8 per verification agent, launched together | -| Iterative reverse audit (Step 5) | 2-5 (3A); rounds × chunks (3B) | Two consecutive dry rounds to stop (cap 5); 3B fans out one auditor per chunk per round | -| **Total** | **~17-23 (~15-22)** | 3A same-repo: ~17-23 (typical ~17-19); cross-repo or local/file: ~15-22; 3B scales with chunks (see DESIGN.md) | +| Stage | LLM calls | Notes | +| -------------------------------- | ------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Review agents (Step 3) | 14 (+0-2) | Run in parallel; cross-repo skips Agents 1c and 7 (12), local/file skips Agent 0 (13) | +| Sharded verification (Step 4) | ceil(F/8) | F = findings; at most 8 per verification agent, launched together | +| Iterative reverse audit (Step 5) | 2-10 (3A); rounds × chunks (3B) | Two consecutive dry rounds to stop; the cap follows the topology — 10 on a small diff, 5 on a chunked one, 3 on a huge one when the run has a deadline. 3B fans out one auditor per chunk per round | +| **Total** | **~17-28 (~15-27)** | 3A same-repo: ~17-28 (typical ~17-19); cross-repo or local/file: ~15-27; 3B scales with chunks (see DESIGN.md) | Most PRs converge to the lower end of the range; the caps prevent runaway cost on pathological cases. At `--effort low` the review runs entirely inline — **0 subagent calls** — walking the diff once per angle instead of once in total. diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 3a9f71a1ad3..f6f458ba1d2 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -1028,6 +1028,62 @@ describe('--round — the CLI bakes the round into the identity line and the key } }); + it('takes the round cap from the CLOCK as well, on a sized huge plan', () => { + // Every other cap test here uses the unsized `PLAN` fixture, whose tier is + // the LARGE fallback whatever the clock says, or forces a cap by storing + // one — so the `hasReviewDeadline(process.env)` argument at all four call + // sites was mutation-invisible: hardcoding it to either constant left the + // whole suite green. A SIZED huge plan is the only shape where the flag + // decides anything. + const dir = mkdtempSync(join(tmpdir(), 'ap-clock-tier-')); + try { + const findings = join(dir, 'f.md'); + writeFileSync(findings, '- x'); + const handler = agentPromptCommand.handler as (a: unknown) => void; + const before = process.env[DEADLINE_ENV]; + const stderr = () => + (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + const huge = join(dir, 'huge.json'); + writeFileSync( + huge, + JSON.stringify({ ...PLAN, srcDiffLines: 5000, diffLines: 5000 }), + ); + try { + // No clock: the huge reduction does not apply, so the 3B tier stands + // and round 4 builds. + delete process.env[DEADLINE_ENV]; + process.exitCode = undefined; + (writeStderrLine as unknown as Mock).mockClear(); + handler({ plan: huge, role: 'reverse-audit', findings, round: 4 }); + expect(process.exitCode).toBeUndefined(); + expect(readRecordedPrompts(huge).size).toBe(1); + + (writeStderrLine as unknown as Mock).mockClear(); + handler({ plan: huge, role: 'reverse-audit', findings, round: 6 }); + expect(process.exitCode).toBe(4); + expect(stderr()).toContain('round cap is 5'); + + // A clock: the same plan, the same round, refused at the reduced tier. + process.env[DEADLINE_ENV] = String( + Math.floor(Date.now() / 1000) + 7200, + ); + process.exitCode = undefined; + (writeStderrLine as unknown as Mock).mockClear(); + handler({ plan: huge, role: 'reverse-audit', findings, round: 4 }); + expect(process.exitCode).toBe(4); + expect(stderr()).toContain('round cap is 3'); + } finally { + if (before === undefined) delete process.env[DEADLINE_ENV]; + else process.env[DEADLINE_ENV] = before; + } + } finally { + process.exitCode = undefined; + rmSync(dir, { recursive: true, force: true }); + } + }); + it('takes the round cap from the plan’s topology on the chunkless path', () => { // 3A is the topology that actually runs this path — one auditor a round, // the whole diff — and it is the one the tier raises. Both arms use the diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 66331528642..9a24c5ebf88 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -55,6 +55,7 @@ import { verifyBudgetMessage, writeBudgetStop, writeRoundCapStop, + hasReviewDeadline, } from './lib/deadline.js'; import { READ_FILE_CHAR_CAP, @@ -2004,7 +2005,8 @@ function admitReverseAuditRound( // The plan's round cap first: deterministic, and cheaper than the // deadline arithmetic. One value per topology (`reverseAuditRoundTier`) — // ten on a 3A diff, where a round is one auditor; five on a 3B one, where - // it is one per non-retired chunk; a reduced three for a huge + // it is one per non-retired chunk; and — only in a run that has a deadline, + // since the reduction answers a ceiling — a reduced three for a huge // diff, where a single reverse-audit round is ~90 minutes and the full // loop cannot finish (measured: the 6-hour CI reviews that posted nothing // were 4,000-5,300-line PRs). A round past the cap writes a marker so @@ -2177,7 +2179,7 @@ function runAllChunks( !admitReverseAuditRound( planPath, round, - reverseAuditRoundCap(report), + reverseAuditRoundCap(report, hasReviewDeadline(process.env)), chunks.length, ) ) { @@ -2233,7 +2235,10 @@ function runAllChunks( : `one per chunk still under audit (${skipped.length} retired ` + `chunk(s) skipped; the retirement note after the end-of-round line ` + `says which — relay it to the terminal)`; - const planRoundCap = reverseAuditRoundCap(report); + const planRoundCap = reverseAuditRoundCap( + report, + hasReviewDeadline(process.env), + ); const retirementNote = skipped.length === 0 ? [] @@ -2596,7 +2601,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { !admitReverseAuditRound( args.plan, args.round, - reverseAuditRoundCap(report), + reverseAuditRoundCap(report, hasReviewDeadline(process.env)), 1, ) ) { @@ -2679,7 +2684,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { !admitReverseAuditRound( args.plan, args.round, - reverseAuditRoundCap(report), + reverseAuditRoundCap(report, hasReviewDeadline(process.env)), planChunkIds.length, ) ) diff --git a/packages/cli/src/commands/review/capture-local.test.ts b/packages/cli/src/commands/review/capture-local.test.ts index f64b58c590d..0646bc33f44 100644 --- a/packages/cli/src/commands/review/capture-local.test.ts +++ b/packages/cli/src/commands/review/capture-local.test.ts @@ -15,8 +15,14 @@ import { mkdtempSync, rmSync, readFileSync, existsSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { seedParseArgs } from './lib/test-utils.js'; +import { DEADLINE_ENV } from './lib/deadline.js'; const captureMock = vi.hoisted(() => vi.fn()); +const settingsMock = vi.hoisted(() => vi.fn(() => ({ merged: {} }))); +vi.mock('../../config/settings.js', async (orig) => ({ + ...(await orig>()), + loadSettings: settingsMock, +})); vi.mock('./lib/local-diff.js', async (orig) => ({ ...(await orig>()), captureLocalDiff: captureMock, @@ -215,3 +221,63 @@ describe('capture-local (command boundary)', () => { expect(out).toContain('\\u001b'); }); }); + +describe('capture-local — the budget context the handler actually passes', () => { + // `BudgetContext`'s fields are optional, so dropping either from this call + // site compiles clean and every unit test beneath it stays green. Only a + // handler-level assertion on the written plan can see it — and this command + // had none. + it('carries the operator ceiling and the clock into the written plan', () => { + const before = process.env[DEADLINE_ENV]; + try { + const huge = Array.from( + { length: 9000 }, + (_, i) => `+const x${i} = ${i};`, + ).join('\n'); + capture({ + diff: Buffer.from( + [ + 'diff --git a/src/huge.ts b/src/huge.ts', + '--- /dev/null', + '+++ b/src/huge.ts', + '@@ -0,0 +1,9000 @@', + huge, + '', + ].join('\n'), + 'utf8', + ), + untracked: ['src/huge.ts'], + }); + + delete process.env[DEADLINE_ENV]; + settingsMock.mockReturnValue({ merged: {} }); + const noClock = join(dir, 'no-clock.json'); + run(noClock); + const a = JSON.parse(readFileSync(noClock, 'utf8')); + expect(a.srcDiffLines).toBeGreaterThanOrEqual(3000); + expect(a.budget.reverseAuditRounds).toBe(5); // huge, no clock → 3B tier + + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const withClock = join(dir, 'with-clock.json'); + run(withClock); + expect( + JSON.parse(readFileSync(withClock, 'utf8')).budget.reverseAuditRounds, + ).toBe(3); + + // …and the operator ceiling lowers whichever tier applies. + settingsMock.mockReturnValue({ + merged: { review: { reverseAuditRounds: 3 } }, + }); + delete process.env[DEADLINE_ENV]; + const capped = join(dir, 'capped.json'); + run(capped); + expect( + JSON.parse(readFileSync(capped, 'utf8')).budget.reverseAuditRounds, + ).toBe(3); + } finally { + settingsMock.mockReturnValue({ merged: {} }); + if (before === undefined) delete process.env[DEADLINE_ENV]; + else process.env[DEADLINE_ENV] = before; + } + }); +}); diff --git a/packages/cli/src/commands/review/capture-local.ts b/packages/cli/src/commands/review/capture-local.ts index 88559078ddf..0a57158aa84 100644 --- a/packages/cli/src/commands/review/capture-local.ts +++ b/packages/cli/src/commands/review/capture-local.ts @@ -31,6 +31,8 @@ import { stringifyPlanReport, type PlanReport, } from './lib/report.js'; +import { operatorReviewSettings } from './lib/review-settings.js'; +import { hasReviewDeadline } from './lib/deadline.js'; interface CaptureLocalArgs { out: string; @@ -94,7 +96,10 @@ 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, { + operatorRoundCap: operatorReviewSettings().reverseAuditRounds, + hasDeadline: hasReviewDeadline(process.env), + }), 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..a480466e242 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -54,6 +54,8 @@ import { stringifyPlanReport, } from './lib/report.js'; import { resolveMergeBase, type GitProbe } from './lib/merge-base.js'; +import { operatorReviewSettings } from './lib/review-settings.js'; +import { hasReviewDeadline } from './lib/deadline.js'; interface PrMetadata { headRefName: string; @@ -430,7 +432,10 @@ 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), { + operatorRoundCap: operatorReviewSettings().reverseAuditRounds, + hasDeadline: hasReviewDeadline(process.env), + }), ...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..6b37a5a04ee 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -13,6 +13,8 @@ import { stripBudgetGapLines, launchToolBudget, reverseAuditRoundCap, + reverseAuditRoundTier, + cappedRoundTier, reviewBudget, } from './budget.js'; @@ -546,7 +548,7 @@ describe('stripBudgetGapLines — the receipt judged without its disclosures', ( }); describe('reviewBudget — the reverse-audit round cap', () => { - it('gives each topology its own cap: ten on 3A, five on 3B, three when huge', () => { + it('gives each topology its own cap: ten on 3A, five on 3B, three when huge (with a clock)', () => { // The cap prices a ROUND, and a round costs two orders of magnitude more // in one topology than another: one auditor on 3A, one per non-retired // chunk on 3B, ~90 min on a huge PR (five of which cannot finish the PRs @@ -562,11 +564,16 @@ describe('reviewBudget — the reverse-audit round cap', () => { reviewBudget({ srcDiffLines: 2999, diffLines: 2999 }).reverseAuditRounds, ).toBe(5); expect( - reviewBudget({ srcDiffLines: 3000, diffLines: 3000 }).reverseAuditRounds, + reviewBudget( + { srcDiffLines: 3000, diffLines: 3000 }, + { hasDeadline: true }, + ).reverseAuditRounds, ).toBe(3); expect( - reviewBudget({ srcDiffLines: 10_000, diffLines: 12_000 }) - .reverseAuditRounds, + reviewBudget( + { srcDiffLines: 10_000, diffLines: 12_000 }, + { hasDeadline: true }, + ).reverseAuditRounds, ).toBe(3); }); @@ -587,7 +594,10 @@ describe('reviewBudget — the reverse-audit round cap', () => { // …and the huge floor wins over the topology gate: a 3B diff at 3000 // effective lines caps at three, not five. expect( - reviewBudget({ srcDiffLines: 3000, diffLines: 3200 }).reverseAuditRounds, + reviewBudget( + { srcDiffLines: 3000, diffLines: 3200 }, + { hasDeadline: true }, + ).reverseAuditRounds, ).toBe(3); }); @@ -597,7 +607,10 @@ describe('reviewBudget — the reverse-audit round cap', () => { // Pins the `effective`-vs-`src` dependence the mutation `effective` → // `src` would otherwise survive. expect( - reviewBudget({ srcDiffLines: 100, diffLines: 30_000 }).reverseAuditRounds, + reviewBudget( + { srcDiffLines: 100, diffLines: 30_000 }, + { hasDeadline: true }, + ).reverseAuditRounds, ).toBe(3); expect(reviewBudget({ srcDiffLines: 100, diffLines: 30_000 }).sweep).toBe( true, @@ -620,18 +633,30 @@ describe('reverseAuditRoundCap — the one reader of the plan field', () => { it('passes a value the plan owns through, at every tier', () => { expect( - reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 10 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: 10 } }, + true, + ), ).toBe(10); expect( - reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 5 } }), + reverseAuditRoundCap( + { ...LARGE, budget: { reverseAuditRounds: 5 } }, + true, + ), ).toBe(5); expect( - reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: 3 } }), + reverseAuditRoundCap( + { ...HUGE, budget: { reverseAuditRounds: 3 } }, + true, + ), ).toBe(3); // Below its tier but in band is honoured — nothing here inflates a cap // the plan itself wrote smaller. expect( - reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 5 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: 5 } }, + true, + ), ).toBe(5); }); @@ -639,69 +664,104 @@ describe('reverseAuditRoundCap — the one reader of the plan field', () => { // Every other case here sits on a boundary — at the floor, at the tier, or // outside — and a boundary-only suite cannot tell a tier-relative bound // from a constant one: mutating `v <= tier` to `v <= LARGE_…ROUNDS` passed - // all 57 tests before these three. The interior is where the two differ, - // and it is reachable in production, not just by hand: the operator round - // ceiling writes exactly such a value into the plan. + // all 57 tests before these three. The interior is where the two differ. expect( - reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 7 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: 7 } }, + true, + ), ).toBe(7); expect( - reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 4 } }), + reverseAuditRoundCap( + { ...LARGE, budget: { reverseAuditRounds: 4 } }, + true, + ), ).toBe(4); // The same mutation read from the other side: a constant bound of five // would hand a HUGE plan the four rounds it stores, past the finishability // tier that is the whole reason that tier exists. expect( - reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: 4 } }), + reverseAuditRoundCap( + { ...HUGE, budget: { reverseAuditRounds: 4 } }, + true, + ), ).toBe(3); }); it('clamps to the plan’s own tier, so a hand edit cannot cross topologies', () => { // The field is CLI-written and nothing here is the caller's to override. - // A single global upper bound of ten would have honoured all three of - // these; the tier is what makes them buy nothing. - expect( - reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: 10 } }), + // The first two are what distinguish a tier clamp from a single global + // bound of ten, which would have honoured both; the third (11 on a SMALL + // plan) is clamped either way and is here for the upper edge, not for the + // comparison. + expect( + reverseAuditRoundCap( + { ...HUGE, budget: { reverseAuditRounds: 10 } }, + true, + ), ).toBe(3); expect( - reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 10 } }), + reverseAuditRoundCap( + { ...LARGE, budget: { reverseAuditRounds: 10 } }, + true, + ), ).toBe(5); expect( - reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 11 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: 11 } }, + true, + ), ).toBe(10); }); 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 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: bad } }, + true, + ), ).toBe(10); expect( - reverseAuditRoundCap({ ...HUGE, budget: { reverseAuditRounds: bad } }), + reverseAuditRoundCap( + { ...HUGE, budget: { reverseAuditRounds: bad } }, + true, + ), ).toBe(3); } // A plan with no `reverseAuditRounds` at all reads as its tier. A plan // that HAS one in band keeps it — see the legacy-value test below. - expect(reverseAuditRoundCap(SMALL)).toBe(10); - expect(reverseAuditRoundCap({ ...SMALL, budget: {} })).toBe(10); - expect(reverseAuditRoundCap(HUGE)).toBe(3); + expect(reverseAuditRoundCap(SMALL, true)).toBe(10); + expect(reverseAuditRoundCap({ ...SMALL, budget: {} }, true)).toBe(10); + expect(reverseAuditRoundCap(HUGE, true)).toBe(3); }); it('rejects an in-band NON-INTEGER — the only guard that can catch it', () => { - // 2.5 above never reaches `Number.isInteger`: the `>= 3` floor rejects it - // first, so deleting the integer guard leaves the whole suite green and + // 2.5 above is rejected by the integer guard itself (the chain tests + // `Number.isInteger` before the `>= 3` floor), so it says nothing about + // what the floor would have caught on its own: delete the integer guard + // and 2.5 still falls to the floor, leaving the suite green while // `reverseAuditRounds: 3.5` becomes a cap of 3.5. These values sit inside - // every tier's band, so the integer check is the ONLY thing between them - // and a fractional round cap. + // every tier's band and above the floor, so the integer check is the ONLY + // thing between them and a fractional round cap. expect( - reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 3.5 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: 3.5 } }, + true, + ), ).toBe(10); expect( - reverseAuditRoundCap({ ...LARGE, budget: { reverseAuditRounds: 4.5 } }), + reverseAuditRoundCap( + { ...LARGE, budget: { reverseAuditRounds: 4.5 } }, + true, + ), ).toBe(5); }); @@ -712,43 +772,58 @@ describe('reverseAuditRoundCap — the one reader of the plan field', () => { // — pinning the claim the doc comment used to get wrong in the other // direction ("a legacy 3A plan gets the ten rounds its topology earns"). expect( - reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 5 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: 5 } }, + true, + ), ).toBe(5); expect( - reverseAuditRoundCap({ ...SMALL, budget: { reverseAuditRounds: 3 } }), + reverseAuditRoundCap( + { ...SMALL, budget: { reverseAuditRounds: 3 } }, + true, + ), ).toBe(3); }); it('treats a COERCIBLE garbage size as unsized, not as a zero-line diff', () => { - // `Number()` turns each of these into a finite 0, so a coerce-then- - // isFinite check calls them usable and hands the plan the SMALL tier — - // ten rounds, the most expensive cap — for a plan whose size is not - // known. `JSON.stringify` writes a NaN line count as `null`, so this is - // the shape the corrupted plan actually arrives in. + // `Number()` turns each of these into a finite number, so a coerce-then- + // isFinite check calls them all usable and sizes the plan from a value it + // never received. `null`, `''`, `false` and `[]` coerce to 0 and land on + // the SMALL tier — ten rounds, the most expensive cap — for a plan whose + // size is not known; `-5` lands there too once floored; `'1'` and + // `'3000'` coerce to real counts and are sized as though a string were a + // line count, `'3000'` reaching the HUGE tier rather than the fallback. + // `JSON.stringify` writes a NaN line count as `null`, so the first shape + // is the one a corrupted plan actually arrives in. for (const bad of [null, '', false, [], -5, '1', '3000'] as unknown[]) { - expect(reverseAuditRoundCap({ srcDiffLines: bad, diffLines: bad })).toBe( - 5, - ); + expect( + reverseAuditRoundCap({ srcDiffLines: bad, diffLines: bad }, true), + ).toBe(5); // …including when only ONE of the pair is garbage. - expect(reverseAuditRoundCap({ srcDiffLines: 100, diffLines: bad })).toBe( - 5, - ); - expect(reverseAuditRoundCap({ srcDiffLines: bad, diffLines: 100 })).toBe( - 5, - ); + expect( + reverseAuditRoundCap({ srcDiffLines: 100, diffLines: bad }, true), + ).toBe(5); + expect( + reverseAuditRoundCap({ srcDiffLines: bad, diffLines: 100 }, true), + ).toBe(5); } // A numeric-string size must not buy a tier a hand edit could not: "1" // would otherwise coerce a 5,800-line plan into the SMALL tier through // the very clamp that exists to stop it. expect( - reverseAuditRoundCap({ - srcDiffLines: '1', - diffLines: '1', - budget: { reverseAuditRounds: 10 }, - }), + reverseAuditRoundCap( + { + srcDiffLines: '1', + diffLines: '1', + budget: { reverseAuditRounds: 10 }, + }, + true, + ), ).toBe(5); // Zero is a real size — an empty diff is a small diff, not an unsized one. - expect(reverseAuditRoundCap({ srcDiffLines: 0, diffLines: 0 })).toBe(10); + expect(reverseAuditRoundCap({ srcDiffLines: 0, diffLines: 0 }, true)).toBe( + 10, + ); }); it('does not let reviewBudget RECORD a tier for a size it never received', () => { @@ -771,17 +846,20 @@ describe('reverseAuditRoundCap — the one reader of the plan field', () => { // The skew case cannot be sized, and an unsized plan could be the // 5,800-line one. Falling back to the large tier means such a plan is // never handed MORE rounds than every plan already runs with. - expect(reverseAuditRoundCap(undefined)).toBe(5); - expect(reverseAuditRoundCap({})).toBe(5); - expect(reverseAuditRoundCap({ budget: { reverseAuditRounds: 10 } })).toBe( - 5, - ); + expect(reverseAuditRoundCap(undefined, true)).toBe(5); + expect(reverseAuditRoundCap({}, true)).toBe(5); expect( - reverseAuditRoundCap({ - srcDiffLines: 'x', - diffLines: 10, - budget: { reverseAuditRounds: 10 }, - }), + reverseAuditRoundCap({ budget: { reverseAuditRounds: 10 } }, true), + ).toBe(5); + expect( + reverseAuditRoundCap( + { + srcDiffLines: 'x', + diffLines: 10, + budget: { reverseAuditRounds: 10 }, + }, + true, + ), ).toBe(5); }); }); @@ -802,3 +880,131 @@ 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, true)).toBe(4); + expect(cappedRoundTier(SMALL, 3, true)).toBe(3); + expect(cappedRoundTier(LARGE, 3, true)).toBe(3); + expect(cappedRoundTier(SMALL, 9, true)).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, true)).toBe(3); + expect(cappedRoundTier(HUGE, 20, true)).toBe(3); + expect(cappedRoundTier(LARGE, 10, true)).toBe(5); + expect(cappedRoundTier(SMALL, 20, true)).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, true)).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, true)).toBe(10); + expect(cappedRoundTier(HUGE, bad, true)).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, true)).toBe(10); + } + expect(cappedRoundTier(SMALL, undefined, true)).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 }, + { operatorRoundCap: 4 }, + ); + expect(b.reverseAuditRounds).toBe(4); + expect(reverseAuditRoundCap({ ...SMALL, budget: b }, true)).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 }, + { operatorRoundCap: 3 }, + ); + expect({ ...capped, reverseAuditRounds: 0 }).toEqual({ + ...plain, + reverseAuditRounds: 0, + }); + }); +}); + +describe('the huge reduction applies only where there is a wall to fit inside', () => { + const HUGE = { srcDiffLines: 5000, diffLines: 5000 }; + const LARGE = { srcDiffLines: 900, diffLines: 900 }; + const SMALL = { srcDiffLines: 100, diffLines: 100 }; + + it('a huge diff with no deadline is just a large 3B diff', () => { + // Three is not a claim that a huge diff converges sooner — it has more + // defects and more territory, and on recall it deserves MORE rounds. It is + // a claim that five ~90-minute rounds do not fit a six-hour ceiling. With + // no ceiling the premise is absent, and trading recall away to fit a wall + // that is not there is a pure loss on exactly the tier where recall + // matters most. + expect(reverseAuditRoundTier(HUGE, false)).toBe(5); + expect(reverseAuditRoundTier(HUGE, true)).toBe(3); + expect(reviewBudget(HUGE, { hasDeadline: false }).reverseAuditRounds).toBe( + 5, + ); + expect(reviewBudget(HUGE).reverseAuditRounds).toBe(5); // absent === no clock + }); + + it('changes nothing for any other topology — this is the huge tier’s rule alone', () => { + for (const clock of [true, false]) { + expect(reverseAuditRoundTier(SMALL, clock)).toBe(10); + expect(reverseAuditRoundTier(LARGE, clock)).toBe(5); + } + // …including the unsized fallback, which is the large tier either way. + expect(reverseAuditRoundTier({}, true)).toBe(5); + expect(reverseAuditRoundTier({}, false)).toBe(5); + }); + + it('the operator ceiling still only lowers, on both sides of the clock', () => { + // Without a clock the huge tier is 5, so an operator asking for 4 now gets + // it — the ceiling composes with the wider tier rather than being masked + // by the reduction. + expect(cappedRoundTier(HUGE, 4, false)).toBe(4); + expect(cappedRoundTier(HUGE, 4, true)).toBe(3); // 4 >= tier 3: no raise + expect(cappedRoundTier(HUGE, 20, false)).toBe(5); + expect(cappedRoundTier(HUGE, 2, false)).toBe(5); // below the floor + }); + + it('the reader clamps a recorded value against the tier IT sees', () => { + // The four clock combinations across capture and gate, all safe. A plan + // captured without a clock records 5; read under a clock the band is + // [3, 3] and it is cut to 3 — conservative, which is the right direction + // when a wall turns out to exist after all. + const noClock = { ...HUGE, budget: { reverseAuditRounds: 5 } }; + expect(reverseAuditRoundCap(noClock, false)).toBe(5); + expect(reverseAuditRoundCap(noClock, true)).toBe(3); + const withClock = { ...HUGE, budget: { reverseAuditRounds: 3 } }; + expect(reverseAuditRoundCap(withClock, true)).toBe(3); + expect(reverseAuditRoundCap(withClock, false)).toBe(3); // in [3,5], honoured + }); +}); diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index cb984d0129a..a827f50998d 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -41,6 +41,30 @@ export interface BudgetInput { diffLines: number; } +/** + * The two facts about the machine that the round cap depends on. + * + * Both are resolved by the capture command and passed in rather than read + * here: this module has no imports, and a budget that loaded settings or + * inspected `process.env` would make every caller's tests depend on the + * machine they run on. They arrive together because they answer the same + * question from two sides — how many reverse-audit rounds this plan may run. + */ +export interface BudgetContext { + /** + * The standing `review.reverseAuditRounds` setting, when the operator set a + * usable one — a settings value, not an environment one. It can only lower + * the round tier; see `cappedRoundTier`. + */ + operatorRoundCap?: number; + /** + * Is `QWEN_REVIEW_DEADLINE_EPOCH` set to something the gates will honour? + * This decides whether the huge tier's finishability reduction applies at + * all; see `HUGE_REVERSE_AUDIT_ROUNDS`. + */ + hasDeadline?: boolean; +} + export interface ReviewBudget { /** * How many of the low tier's directed angles to walk (Step 3C, A–F). @@ -112,15 +136,19 @@ export interface ReviewBudget { * one number: the same cap cannot price a single agent and a 19-way * fan-out. In a time-budgeted CI run the deadline gate already refuses a * round that will not fit; this static cap is the belt it works under and - * the ONLY bound a local run (no deadline) has. The huge tier is reduced - * to three, not two — not because two cannot converge (the all-dry + * the ONLY bound a local run (no deadline) has — which is also why the huge + * tier's reduction does not apply to such a run at all: with no ceiling to + * fit inside there is nothing for it to answer, so a huge diff without a + * deadline reads the 3B tier. Where a deadline does exist the huge tier is + * reduced to three, not two — not because two cannot converge (the all-dry * rounds-1-and-2 shape reaches CONVERGED at the round-3 build under any * cap of two or more, since the convergence check runs before the cap * gate) but to buy hot chunks one extra audit round before the cap. * * The budget tunes how many rounds the loop runs, never whether it runs: - * the reverse audit is a dimension of the high-effort contract. The CLI - * only ever writes one of the three tier values here. + * the reverse audit is a dimension of the high-effort contract. What the CLI + * writes here is a tier value, or — when the operator set a + * `review.reverseAuditRounds` ceiling below it — that ceiling. */ reverseAuditRounds: number; } @@ -161,6 +189,18 @@ export const LARGE_REVERSE_AUDIT_ROUNDS = 5; * loop. Not a convergability minimum: the all-dry rounds-1-and-2 shape * reaches CONVERGED under any cap of two or more, because the reverse * audit's convergence check runs before the round-cap gate. + * + * **Applied only when the run has a deadline.** This is not a claim that a + * huge diff converges sooner — it plainly does not; it has more defects and + * more territory, and on the recall axis it deserves MORE rounds than a small + * one, not fewer. It is a claim about a wall: five ~90-minute rounds do not + * fit a six-hour CI ceiling, and a review killed mid-flight posts nothing at + * all, so three rounds reported beat five rounds lost (measured; DESIGN.md — + * The six-hour timeouts). Where no wall exists — a local run with no + * `QWEN_REVIEW_DEADLINE_EPOCH` — the premise is absent and so is the + * reduction: a huge diff is then just a large 3B diff and gets the 3B tier. + * Trading recall away to fit a ceiling that is not there is a pure loss, and + * the tier this reduction cuts from is the one where recall matters most. */ export const HUGE_REVERSE_AUDIT_ROUNDS = 3; @@ -239,14 +279,22 @@ export function isTerritoryFanOut(plan: DiffSize): boolean { * which would have let a hand-edited huge plan reach the SMALL tier through * the very clamp `reverseAuditRoundCap` adds to prevent it. */ -export function reverseAuditRoundTier(size: DiffSize): number { +export function reverseAuditRoundTier( + size: DiffSize, + hasDeadline: boolean, +): number { const src = size?.srcDiffLines; const total = size?.diffLines; if (!usableLineCount(src) || !usableLineCount(total)) { return LARGE_REVERSE_AUDIT_ROUNDS; } const effective = effectiveLines(src, total); - if (effective >= HUGE_DIFF_FLOOR) return HUGE_REVERSE_AUDIT_ROUNDS; + // The huge reduction is a ruling about fitting inside a wall, so it applies + // only where there is one. Without a clock a huge diff is simply a large 3B + // diff and gets the 3B tier — see `HUGE_REVERSE_AUDIT_ROUNDS`. + if (effective >= HUGE_DIFF_FLOOR) { + return hasDeadline ? HUGE_REVERSE_AUDIT_ROUNDS : LARGE_REVERSE_AUDIT_ROUNDS; + } // The validated pair, not `size` again. Re-reading the raw object here would // give the huge gate and the topology gate two independent derivations of // the same two numbers inside one function — which is exactly the shape of @@ -257,6 +305,51 @@ 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, + hasDeadline: boolean, +): number { + const tier = reverseAuditRoundTier(size, hasDeadline); + 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 +407,16 @@ 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. + * + * `context` carries the two facts about the machine the round cap depends on; + * see `BudgetContext`. Nothing else in the budget is tunable from outside, 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, + context: BudgetContext = {}, +): ReviewBudget { const src = sane(input.srcDiffLines); const total = sane(input.diffLines); @@ -350,7 +451,11 @@ 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, + context.operatorRoundCap, + context.hasDeadline === true, + ), }; } @@ -362,7 +467,12 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { * not the schedule's). * * It takes the whole plan, not `plan.budget`, because the accepted range is - * now the plan's **own topology tier** rather than a global band. What that + * now the plan's **own topology tier** rather than a global band — and + * `hasDeadline` because that tier is clock-dependent on a huge diff (3 with a + * deadline, 5 without). A reader that sees a different clock than the capture + * did therefore clamps against a different band, which is safe in the + * direction that matters: a plan captured without a clock and read under one + * is cut to the shorter tier, never the reverse. What that * buys, stated as what actually happens rather than as a slogan: * * - **A hand-edited plan cannot cross tiers.** The field is CLI-written and @@ -382,18 +492,22 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { * overriding a number the plan states, which is the one thing a reader of * a CLI-written field must not do. * - It does **not** always err toward more auditing. A field-less **huge** - * plan now reads 3 where the flat fallback read 5 — deliberately less: the - * huge tier is a finishability ruling, and the reviews it exists for are - * the ones that ran six hours and posted nothing. + * plan reads 3 where the flat fallback read 5 — deliberately less — but + * only in a run that has a deadline; without one the huge tier is 5 and the + * fallback is unchanged. The reduction is a finishability ruling, and the + * reviews it exists for are the ones that ran six hours and posted nothing. * * 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); +export function reverseAuditRoundCap( + plan: unknown, + hasDeadline: boolean, +): number { + const tier = reverseAuditRoundTier((plan ?? {}) as DiffSize, hasDeadline); const v = (plan as { budget?: { reverseAuditRounds?: unknown } } | undefined) ?.budget?.reverseAuditRounds; return typeof v === 'number' && diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index b9368a197b3..dd465c1811b 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -10,7 +10,8 @@ // is open-ended: each round is a fan-out (one auditor per chunk on a 3B plan), // each round's findings go back through verification, and the loop runs until // two consecutive dry rounds or the plan's round cap (one value per topology: -// 10 on a 3A diff, 5 on a 3B one, 3 when huge). On a PR where every round +// 10 on a 3A diff, 5 on a 3B one, and 3 when huge — but only where a deadline +// exists, since that reduction answers a ceiling; 5 when huge without one). On a PR where every round // finds something, that is the whole budget. Measured on a real CI run // (#8368, +1699 lines): the audit loop ran to the 5-round cap, consumed 3.5 of // the job's 4 budgeted hours, and the outer GNU-timeout kill arrived while @@ -370,6 +371,25 @@ function readDeadlineSeconds(env: NodeJS.ProcessEnv): number | null { return deadline; } +/** + * Does this run have a clock at all? + * + * The budget's huge-diff round reduction is a *finishability* ruling — five + * ~90-minute rounds do not fit a six-hour CI ceiling — and a ruling about + * fitting inside a wall is meaningless where there is no wall. This is how the + * capture commands ask, and it deliberately reuses the same parse both gates + * read from, so "has a deadline" and "the gate will enforce a deadline" cannot + * come apart: a malformed value leaves the review ungated here exactly as it + * leaves it ungated there. + * + * The env, not `process.env`, for the reason every other function in this file + * takes it: a test must be able to ask the question without editing the + * process it runs in. + */ +export function hasReviewDeadline(env: NodeJS.ProcessEnv): boolean { + return readDeadlineSeconds(env) !== null; +} + /** * A non-negative seconds override from `env[key]`, or `fallback`. `>= 0` * (not `> 0`) is deliberate: 0 is a documented escape hatch on both gates. diff --git a/packages/cli/src/commands/review/lib/report.test.ts b/packages/cli/src/commands/review/lib/report.test.ts index 726bed3a146..35c4eb2c202 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; + }, + {}, + ); expect(asked).toEqual(['src/a.ts']); expect(report.files[0].fileLines).toBe(1000); // pre = post - added + removed @@ -61,16 +65,16 @@ 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, {}).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, {}).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, {}); expect(report.files[0].fileLines).toBe(0); expect(report.files[0].preLines).toBe(0); expect(report.files[0].heavy).toBe(false); @@ -84,10 +88,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; + }, + {}, + ); expect(asked).toEqual([]); expect(report.files[0].binary).toBe(true); expect(report.files[0].heavy).toBe(false); @@ -95,8 +103,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), + {}, ); const heavy = report.files.find((f) => f.path === 'src/heavy.ts')!; const light = report.files.find((f) => f.path === 'src/light.ts')!; @@ -121,7 +131,7 @@ describe('buildPlanReport', () => { '-gone2', '', ].join('\n'); - const report = buildPlanReport(buildDiffPlan(diff, 400), () => 100); + const report = buildPlanReport(buildDiffPlan(diff, 400), () => 100, {}); expect(report.files[0].hunks).toEqual([{ newStart: 1, newEnd: 2 }]); }); @@ -130,7 +140,7 @@ 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, {}); const f = report.files[0]; expect(f.heavy).toBe(true); expect(f.diffRange).toEqual({ @@ -143,6 +153,7 @@ describe('buildPlanReport', () => { const report = buildPlanReport( buildDiffPlan(addFile('src/a.ts', 20), 400), () => 30, + {}, ); expect(report.files[0].heavy).toBe(false); expect(report.files[0].diffRange).toBeUndefined(); @@ -155,7 +166,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, {}); expect(report.srcDiffLines).toBe(plan.srcDiffLines); expect(report.testDiffLines).toBe(plan.testDiffLines); expect(report.docsDiffLines).toBe(plan.docsDiffLines); @@ -167,8 +178,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), + {}, ); expect(JSON.parse(stringifyPlanReport(report))).toEqual(report); }); @@ -177,6 +190,7 @@ describe('stringifyPlanReport', () => { const report = buildPlanReport( buildDiffPlan(editFile('src/heavy.ts', 3, 900), 400), () => 6000, + {}, ); const text = stringifyPlanReport(report); // Not one giant line: `read_file` pages at line boundaries, so a compact @@ -193,6 +207,7 @@ describe('stringifyPlanReport', () => { const report = buildPlanReport( buildDiffPlan(editFile('src/heavy.ts', 3, 900), 400), () => 6000, + {}, ); const collapsed = stringifyPlanReport(report).length; const indented = JSON.stringify(report, null, 2).length + 1; @@ -211,7 +226,7 @@ describe('stringifyPlanReport', () => { '+x', '', ].join('\n'); - const report = buildPlanReport(buildDiffPlan(diff, 400), () => 1); + const report = buildPlanReport(buildDiffPlan(diff, 400), () => 1, {}); 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..f2f9ff457d8 100644 --- a/packages/cli/src/commands/review/lib/report.ts +++ b/packages/cli/src/commands/review/lib/report.ts @@ -13,7 +13,11 @@ import { statSync } from 'node:fs'; import { writeStderrLine } from '../../../utils/stdioHelpers.js'; import { classifyHeavy } from './heavy.js'; import type { DiffChunk, DiffPlan, PathKind } from './diff-plan.js'; -import { reviewBudget, type ReviewBudget } from './budget.js'; +import { + reviewBudget, + type BudgetContext, + type ReviewBudget, +} from './budget.js'; import type { RepositoryContext } from './repository-context.js'; export interface FileMetric { @@ -106,10 +110,21 @@ 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. + * + * `context` carries the two facts about the machine that the round cap depends + * on — the operator's `review.reverseAuditRounds` ceiling and whether this run + * has a deadline — and is a **required** parameter, deliberately not resolved + * in here. Three capture commands build a plan; an optional parameter is one a + * call site can quietly omit, and a policy that silently applies to two of the + * three review entry points is worse than one that applies to none. Passing + * `{}` is how a caller says "neither applies" — visibly, at the call site. + * Resolving them here instead would make this builder's tests depend on the + * machine's own `~/.qwen` and on its environment. */ export function buildPlanReport( plan: DiffPlan, postImageLines: ((path: string) => number) | null, + context: BudgetContext, ): PlanReport { const files = plan.files.map((f): FileMetric => { const changedLines = f.addedLines + f.removedLines; @@ -162,10 +177,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, + }, + context, + ), }; } 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..f100c24937d 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,42 @@ 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. Most of them are the + // conservative side outright — attribution on, no auto-posting, no round + // ceiling — so a review that loses those loses them toward doing more work + // and writing nothing public. `effort` is the honest exception: dropping an + // operator's `high` returns the built-in rule, which is medium on a local + // target, so that one field can degrade to LESS work. It is disclosed on + // stderr for exactly that reason rather than being silently absorbed. + 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 +97,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.test.ts b/packages/cli/src/commands/review/plan-diff.test.ts index 71f3c8dbe5d..c82a29a52cf 100644 --- a/packages/cli/src/commands/review/plan-diff.test.ts +++ b/packages/cli/src/commands/review/plan-diff.test.ts @@ -4,13 +4,20 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +const settingsMock = vi.hoisted(() => vi.fn(() => ({ merged: {} }))); +vi.mock('../../config/settings.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, loadSettings: settingsMock }; +}); import { mkdtempSync, rmSync, writeFileSync, readFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { planDiffCommand } from './plan-diff.js'; import { chunksCoverDiff } from './lib/diff-plan.js'; import { seedParseArgs } from './lib/test-utils.js'; +import { DEADLINE_ENV } from './lib/deadline.js'; let dir: string; let cwd: string; @@ -22,6 +29,10 @@ const run = (diffPath: string, out: string, maxChunkLines = 400) => }); beforeEach(() => { + // The settings mock is module-level, so a test that sets a ceiling leaves it + // set for every test after it — including the whole trailing describe, which + // would then run the real handler with an undeclared ceiling in play. + settingsMock.mockReturnValue({ merged: {} }); dir = mkdtempSync(join(tmpdir(), 'plan-diff-')); cwd = process.cwd(); process.chdir(dir); @@ -56,6 +67,93 @@ function makeDiff(path: string, n: number): string { ].join('\n'); } +describe('plan-diff — the round cap the handler actually records', () => { + // The capture handlers are where the two machine facts enter a plan, and + // until now nothing exercised that wiring: the budget unit tests pass the + // context directly, so a handler that forgot to read the environment would + // have kept every one of them green. This drives the real handler with a + // real env and reads the number out of the file it wrote. + const hugeDiff = () => makeDiff('src/huge.ts', 9000); + + it('records the huge tier only when the environment has a deadline', () => { + const diffPath = join(dir, 'huge.diff'); + writeFileSync(diffPath, hugeDiff()); + const before = process.env[DEADLINE_ENV]; + try { + delete process.env[DEADLINE_ENV]; + const noClock = join(dir, 'no-clock.json'); + run(diffPath, noClock); + const a = JSON.parse(readFileSync(noClock, 'utf8')); + expect(a.srcDiffLines).toBeGreaterThanOrEqual(3000); + expect(a.budget.reverseAuditRounds).toBe(5); + + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const withClock = join(dir, 'with-clock.json'); + run(diffPath, withClock); + expect( + JSON.parse(readFileSync(withClock, 'utf8')).budget.reverseAuditRounds, + ).toBe(3); + } finally { + if (before === undefined) delete process.env[DEADLINE_ENV]; + else process.env[DEADLINE_ENV] = before; + } + }); + + it('records the operator ceiling the settings actually carry', () => { + // The write half of `review.reverseAuditRounds`: capture command → + // buildPlanReport → reviewBudget. Every budget unit test passes the + // ceiling in directly, so a handler that never read the setting would + // have kept them all green. This drives the handler with the setting + // mocked at its source and reads the number out of the file it wrote. + const diffPath = join(dir, 'small.diff'); + writeFileSync(diffPath, makeDiff('src/small.ts', 120)); + const out = join(dir, 'ceiling.json'); + settingsMock.mockReturnValue({ merged: { review: {} } }); + run(diffPath, out); + expect( + JSON.parse(readFileSync(out, 'utf8')).budget.reverseAuditRounds, + ).toBe(10); + settingsMock.mockReturnValue({ + merged: { review: { reverseAuditRounds: 4 } }, + }); + const capped = join(dir, 'ceiling-4.json'); + run(diffPath, capped); + expect( + JSON.parse(readFileSync(capped, 'utf8')).budget.reverseAuditRounds, + ).toBe(4); + // …and a ceiling above the tier still buys nothing, through the handler. + settingsMock.mockReturnValue({ + merged: { review: { reverseAuditRounds: 20 } }, + }); + const raised = join(dir, 'ceiling-20.json'); + run(diffPath, raised); + expect( + JSON.parse(readFileSync(raised, 'utf8')).budget.reverseAuditRounds, + ).toBe(10); + }); + + it('reads a malformed deadline as no deadline, exactly as the gates do', () => { + // `hasReviewDeadline` shares the gates' parse on purpose: a value the gate + // will not enforce must not make the budget behave as though it would. + const diffPath = join(dir, 'huge2.diff'); + writeFileSync(diffPath, hugeDiff()); + const before = process.env[DEADLINE_ENV]; + try { + for (const bad of ['', ' ', 'soon', '0', '-1']) { + process.env[DEADLINE_ENV] = bad; + const out = join(dir, `bad-${bad.trim() || 'empty'}.json`); + run(diffPath, out); + expect( + JSON.parse(readFileSync(out, 'utf8')).budget.reverseAuditRounds, + ).toBe(5); + } + } finally { + if (before === undefined) delete process.env[DEADLINE_ENV]; + else process.env[DEADLINE_ENV] = before; + } + }); +}); + describe('plan-diff', () => { it('emits the same chunk plan a fetch report carries', () => { // This is what makes Step 3B reachable for a local-diff review: the diff --git a/packages/cli/src/commands/review/plan-diff.ts b/packages/cli/src/commands/review/plan-diff.ts index 93b75a52de7..0b79a07c342 100644 --- a/packages/cli/src/commands/review/plan-diff.ts +++ b/packages/cli/src/commands/review/plan-diff.ts @@ -37,6 +37,8 @@ import { type PlanReport, stringifyPlanReport, } from './lib/report.js'; +import { operatorReviewSettings } from './lib/review-settings.js'; +import { hasReviewDeadline } from './lib/deadline.js'; interface PlanDiffArgs { diff_path: string; @@ -124,7 +126,10 @@ 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, { + operatorRoundCap: operatorReviewSettings().reverseAuditRounds, + hasDeadline: hasReviewDeadline(process.env), + }), ...planEffortField(args.effort), }; diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 4f85b0cf802..68080f27030 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; a huge diff is 3 when the run has a review deadline and 5 when it does not, because that reduction answers a CI ceiling and applies only where one exists) 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 — a value that is not a whole number above zero, or that is out of range (below 3, or above the plan\'s own tier), is ignored and leaves the tier alone — JSON Schema has no integer type here, so a fraction validates in an editor and is then discarded at runtime. 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". Nothing here makes a loop run LONGER: a review deadline bounds a run rather than extending it, and on a huge diff setting one lowers the cap from 5 to 3 rather than raising it. 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..c8f985b0464 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -99,16 +99,41 @@ Requiring two consecutive dry rounds makes a single lazy or context-starved agen Five was one number standing in for three prices. What the cap bounds is a **round**, and a round costs one auditor on 3A, one auditor per non-retired chunk on 3B, and ~90 minutes on a huge diff. That is two orders of magnitude across the topologies a single cap had to serve, so it was necessarily wrong at one end: too loose to bound the huge case — which is why `HUGE_REVERSE_AUDIT_ROUNDS` had to be carved out of it — and, at the other end, tight enough on 3A to stop loops that were still confirming Criticals, for a saving of about five calls out of a review that cost 17-23 of them before this change. -The asymmetry that decides it is the one this whole design is built on: a missed issue costs another `/review` iteration, and per-run cost is the cheaper side of that trade (see "Competitors" below). On 3A the marginal round is a single agent; on huge it is an hour and a half of a six-hour ceiling. The cap should say so. +The asymmetry that decides it is the one this whole design is built on: a missed issue costs another `/review` iteration, and per-run cost is the cheaper side of that trade (see the competitor comparison under "Token cost analysis" below). On 3A the marginal round is a single agent; on huge it is an hour and a half of a six-hour ceiling. The cap should say so. So the cap is read from the plan's topology tier (`reverseAuditRoundTier`): **10** on 3A, **5** on 3B, **3** when huge. Three consequences worth naming: -- **The huge tier is checked first and wins.** It is a finishability ruling, and a huge diff is territory-fanned-out by construction anyway. A 3A diff can never be huge — `effective = max(src, floor(total/8))` is at most `max(500, 400)` under the 3A gate — so the two tiers cannot contend. +- **The huge tier is checked first and wins, and it applies only where there is a wall.** It is a finishability ruling, and a huge diff is territory-fanned-out by construction anyway. A 3A diff can never be huge — `effective = max(src, floor(total/8))` is at most `max(500, 400)` under the 3A gate — so the two tiers cannot contend. See "Why the huge reduction needs a clock" below for why it is conditional. - **One predicate, not two sets of numbers.** The tier reads `isTerritoryFanOut`, the same gate the roster turns on, which is why that function moved into `budget.ts`: a second copy of `500`/`3200` would eventually disagree with the roster about which fan-out a review owed. - **The cap is a belt, not the terminator.** The loop still ends on two consecutive dry rounds, and in a time-budgeted run the deadline gate — which prices the round it is admitting plus the reserve — is the operative bound. The static cap is what a local run (no deadline) has instead, which is exactly why it should not be a number borrowed from another topology's arithmetic. +### Why the huge reduction needs a clock + +Three is the one tier that is _lower_ than the topology below it, and read as a statement about auditing it is backwards: a huge diff has more defects and more territory than a chunked one, converges later, and on recall deserves more rounds, not fewer. PR #6457 is the standing counterexample — 5,801 lines, eight review rounds, still surfacing Criticals in code that had been in the diff since the first commit. + +It is not a statement about auditing. It is a statement about a wall: a reverse-audit round on a 4,000-line PR is ~90 minutes, five of them are 450, and a six-hour CI ceiling does not hold that plus the fan-out and the tail. What the survey measured is not slow reviews but absent ones — 26 timed-out review jobs in one window, ~122 hours of compute, **zero posted** (DESIGN.md — The six-hour timeouts). Three rounds reported beat five rounds lost. + +That argument is sound exactly where the wall is, and nowhere else. A local run exports no `QWEN_REVIEW_DEADLINE_EPOCH`, nothing kills it at six hours, and the reduction there trades recall away to fit a ceiling that does not exist — on the tier where recall matters most. So the reduction is now conditional on the run having a deadline at all: with a clock, 3; without one, a huge diff is a large 3B diff and gets 5. + +Two things this does not pretend to fix, both worth naming rather than discovering: + +- **The gate cannot price the early rounds.** `expectedRoundSeconds` falls back to a flat 30-minute constant until a round has been measured, and a huge round is ~90 — so on the runs that time out, the deadline gate under-prices rounds 1 and 2 by 3x and cannot refuse them. That, not the round count, is why a static reduction was needed on top of a working gate. A size-aware round-1 estimate is the change that would let the reduction retire entirely. +- **The cap is what stops retirement from paying for itself.** Chunk retirement (which predates this tier by five days) can only begin at round 3, and under a 3-round cap only round 3 can shrink before the loop ends. The expensive rounds are paid in full and the cheap ones are never reached, so the "5 × 90 = 450" arithmetic that justifies the cap is an arithmetic the cap guarantees stays true. + 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 **high-effort** review they run — medium skips the reverse audit and low runs none, so there is no cap for it to cut there. 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" are both about something other than a count. "Keep going while it is still finding real defects" is a property of the **findings**, not of a number chosen before the review starts. "My ceiling is not the six hours the huge tier assumes" has no expression today, and it is worth being precise about why rather than pointing at the deadline: `hasReviewDeadline` is a _presence_ check, so a huge diff reads 3 under any deadline however generous, and the round cap is evaluated before the deadline arithmetic — so setting a longer deadline **lowers** the cap rather than raising it. Saying "my ceiling is larger" would need the tier to read the deadline's size rather than its existence; see "Why the huge reduction needs a clock". 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. @@ -616,7 +641,7 @@ The countermeasure is cheap and needs no new machinery: before Step 4, sanity-ch | Iterative reverse audit | 2-10 | loop ends after two consecutive dry rounds; 10-round hard cap — one auditor a round on 3A, so the marginal round is one call. A 3A diff is never "huge": `effective = max(src, total/8) ≤ max(500, 400)`, so the huge tier cannot reach this table | | **Total** | **~17-28 (~15-27)** | Row maxima do not co-occur on typical runs (~17-19 is common), but the honest sum of ranges is 17-28 same-repo, 15-27 cross-repo/local. **Low effort: 0 subagent calls** — the angle rotation runs in the orchestrator's own context; medium launches its reduced fan-out | -**Large diffs (> 500 source lines OR > 3200 total diff lines, Step 3B, high effort) — `ceil(diffLines / 400)` chunk agents + `5..7` whole-diff agents + `3H` invariant agents (H = heavy files) + `ceil(F/8)` verify (F = findings) + `rounds × chunks` reverse audit.** The reverse audit dominates: it fans out one auditor per chunk per round, and the stop rule needs two consecutive dry rounds (hard cap 5 — **3 for a huge diff, effective ≥ 3000 lines**, which narrows the per-chunk multiplier to `2..3`). PR #6457 (5801 diff lines, 19 chunks, 1 heavy file) costs ~27-29 first-wave calls, then `19 × (2..5) = 38-95` reverse auditors — ~66-126 calls total depending on how long the audit keeps finding (a huge-diff cap-3 run of the same shape narrows this to `19 × (2..3) = 38-57`); ~70 is the clean-run floor, and the count scales with chunks and findings, not a fixed ceiling. +**Large diffs (> 500 source lines OR > 3200 total diff lines, Step 3B, high effort) — `ceil(diffLines / 400)` chunk agents + `5..7` whole-diff agents + `3H` invariant agents (H = heavy files) + `ceil(F/8)` verify (F = findings) + `rounds × chunks` reverse audit.** The reverse audit dominates: it fans out one auditor per chunk per round, and the stop rule needs two consecutive dry rounds (hard cap 5 — **3 for a huge diff, effective ≥ 3000 lines, when the run has a deadline**, which narrows the per-chunk multiplier to `2..3`; a huge diff with no deadline keeps the 3B cap of 5, so the multiplier stays `2..5`). PR #6457 (5801 diff lines, 19 chunks, 1 heavy file) costs ~27-29 first-wave calls, then `19 × (2..5) = 38-95` reverse auditors — ~66-126 calls total depending on how long the audit keeps finding (a huge-diff cap-3 run of the same shape narrows this to `19 × (2..3) = 38-57`); ~70 is the clean-run floor, and the count scales with chunks and findings, not a fixed ceiling. That is roughly 4x the small-diff budget, and it buys the thing the small-diff topology cannot deliver at that size: coverage. Ten dimension agents (the roster of the day; fourteen now) on a 5801-line diff each read the same truncated 14% window (see "Why the diff is a file, not a command"), so nine of the ten calls were redundant reads of the same hunks. Nineteen chunk agents each read a distinct ~390-line territory, and every line of the diff has exactly one accountable owner. The comparison to make is not ~70 calls vs ~17: PR #6457 took **eight** review rounds at 12-14 calls each — over 100 calls — and was still surfacing Criticals in code that had been in the diff since the first commit. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 666795c76dc..9a049fc5999 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) — but the huge reduction applies **only when the run has a deadline** (`QWEN_REVIEW_DEADLINE_EPOCH`); without a clock a huge diff is just a large 3B diff and gets 5. 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 not a claim that a huge diff converges sooner — it plainly does not, and on recall it deserves more rounds than a small one, not fewer; it is a claim that five ~90-minute rounds do not fit a six-hour ceiling, and a review killed mid-flight posts nothing at all. Where there is no ceiling the premise is absent and so is the reduction. Three 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 (honoured from the User, System and SystemDefaults scopes — never from the repository's own `.qwen/settings.json`; a value below 3, or above the tier, is ignored rather than clamped, so it leaves the tier alone) — 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: **in a run that has a deadline**, 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. Without a deadline it reads 5, the same as the flat fallback. + 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: @@ -684,8 +683,8 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c - A round is **dry** only when _every_ agent in it returned zero new findings **with** the evidence-bearing receipt (`No issues found — `). A round containing a twice-whiffed agent is **not dry** — silence is not convergence evidence — so the loop continues (the hard cap below still bounds it). - **When the loop ends with any scope still outstanding** (by cap, or by dry rounds elsewhere), terminal prose is not enough: add one self-explained entry per scope to `unreviewedDimensions` — e.g. `reverse audit of chunk 3 — the auditor returned nothing substantive twice` — so compose-review serializes it and caps a would-be Approve at `COMMENT`. The primary Step 3 pass did read that scope (its receipt stands), but this run's contract includes the reverse audit, and a verdict must not silently claim an audit that never ran. - Stop after **two consecutive dry rounds** (the 3A criterion — one auditor, so round-dry and territory-dry are the same thing). One dry round is not evidence of convergence: on PR #6457 the review returned "no blockers" twice and the very next round surfaced five Criticals, three of them in code that had been in the diff since the first commit. A single lazy agent must not be able to end the loop. A dry convergence pair satisfies this rule in one launch — its two members are exactly the two independent audits the rule demands; what the pair removes is the wall clock between them, not either audit. When the loop ends on this rule, the last reporting round's verifiers are already in flight (they launched with the next round's auditors) — wait for their verdicts and apply them in the final merge before Step 6. -- **On the 3B path the builder is also the convergence ledger**: when every chunk holds two consecutive substantive dry audits and none is due a cold check, `--all-chunks` builds nothing, prints a `CONVERGED` explanation to stderr and exits **5**. Stop the loop and proceed to Step 6 — this is a **clean** convergence, not a gap: no `unreviewedDimensions` entry is owed, because each chunk holds the two-dry rule's evidence chunk by chunk — two consecutive dry **audits**, though not necessarily in consecutive rounds (a chunk dry in rounds 1 and 2 skips round 3 and cold-checks dry in round 4, holding rounds 2 and 4). If an earlier round-cap or budget refusal told you to add its stop entry to `unreviewedDimensions`, remove it now — this convergence supersedes that stop (the marker on disk is cleared the same way). Exit 5 is mainly the CLI enforcing the stop the two-dry-rounds rule above used to leave to orchestrator discretion; the new savings are the odd-round skips and a convergence at the cap round (round 5 on a 3B diff, round 3 under the huge-diff cap — this ledger is 3B's, so the 3A tier's ten never applies here). (It cannot owe a verification launch: a reporting round makes its chunk hot, so every verifier launched with a later round that did run.) -- Stop at the plan's **`reverseAuditRounds` cap** — 10 on a 3A diff, 5 on a 3B one, 3 for a huge diff (effective ≥ 3000 lines) — and say so in the output rather than implying convergence. The cap is per topology because it prices a round, and a 3A round is one auditor where a huge-diff round is ~90 minutes; you never work this out yourself, the builder reads the plan's tier. The builder enforces this itself: a round past the cap gets a `ROUND CAP:` refusal on stderr and exit **4**, and — like the time-budget gate — writes a marker `compose-review` caps the verdict on whether or not you relay anything; still add the entry the message names to `unreviewedDimensions` so the terminal report agrees. If the cap round reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so verify them before Step 6 through `agent-prompt --role verify` **only** (never a hand-rolled agent), under the same bounded tail as the budget stop below: that builder is gated on the compose floor and refuses once too little time remains, and when the deadline is within the floor you stop waiting on any verifier batch still out and compose with the tags in hand — no fresh re-verification pass, and nothing already confirmed re-verified. This matters most on exactly the huge diffs the cap targets: a time-budgeted CI run that stops at the cap with ~30-90 minutes left must not spend it on an unbounded tail and die before compose. The tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. +- **On the 3B path the builder is also the convergence ledger**: when every chunk holds two consecutive substantive dry audits and none is due a cold check, `--all-chunks` builds nothing, prints a `CONVERGED` explanation to stderr and exits **5**. Stop the loop and proceed to Step 6 — this is a **clean** convergence, not a gap: no `unreviewedDimensions` entry is owed, because each chunk holds the two-dry rule's evidence chunk by chunk — two consecutive dry **audits**, though not necessarily in consecutive rounds (a chunk dry in rounds 1 and 2 skips round 3 and cold-checks dry in round 4, holding rounds 2 and 4). If an earlier round-cap or budget refusal told you to add its stop entry to `unreviewedDimensions`, remove it now — this convergence supersedes that stop (the marker on disk is cleared the same way). Exit 5 is mainly the CLI enforcing the stop the two-dry-rounds rule above used to leave to orchestrator discretion; the new savings are the odd-round skips and a convergence at the cap round (round 5 on a 3B diff, round 3 under the huge-diff cap when the run has a deadline and round 5 when it does not — this ledger is 3B's, so the 3A tier's ten never applies here). (It cannot owe a verification launch: a reporting round makes its chunk hot, so every verifier launched with a later round that did run.) +- Stop at the plan's **`reverseAuditRounds` cap** — 10 on a 3A diff, 5 on a 3B one, and 3 for a huge diff (effective ≥ 3000 lines) **when the run has a deadline**, 5 when it does not (the huge reduction answers a six-hour ceiling, so it applies only where there is one) — and say so in the output rather than implying convergence. The cap is per topology because it prices a round, and a 3A round is one auditor where a huge-diff round is ~90 minutes; you never work this out yourself, the builder reads the plan's tier. The builder enforces this itself: a round past the cap gets a `ROUND CAP:` refusal on stderr and exit **4**, and — like the time-budget gate — writes a marker `compose-review` caps the verdict on whether or not you relay anything; still add the entry the message names to `unreviewedDimensions` so the terminal report agrees. If the cap round reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so verify them before Step 6 through `agent-prompt --role verify` **only** (never a hand-rolled agent), under the same bounded tail as the budget stop below: that builder is gated on the compose floor and refuses once too little time remains, and when the deadline is within the floor you stop waiting on any verifier batch still out and compose with the tags in hand — no fresh re-verification pass, and nothing already confirmed re-verified. This matters most on exactly the huge diffs the cap targets: a time-budgeted CI run that stops at the cap with ~30-90 minutes left must not spend it on an unbounded tail and die before compose. The tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. - Findings **reported** by each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. **The merge runs unconditionally — before every round build and before Step 6, whether or not the previous round reported findings**: under the pipelined loop below, round _k_'s verdicts land during round _k+1_, and every termination mode (two dry rounds, CONVERGED, budget stop, the round cap) can arrive with the final rounds dry — a merge keyed to "some round reported something" would never apply the last verdicts that landed. Each merge applies every Step 4 verdict that has landed: confirmed removes the tag, rejected removes the entry. Verification status does not gate the merge — the list exists so auditors do not re-report what is already filed, and an unverified entry serves that purpose exactly as well as a confirmed one. The trade, named: an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood — the window is one round in one location, and the plan's round cap still bounds the loop. The tag is what keeps this mechanical rather than remembered: an entry enters the list tagged `— [unverified]`; the merge after its Step 4 verdict removes the tag (confirmed) or the entry (rejected). Step 6's confirmed-only read then has something to key on — anything still tagged is left out of the confirmed set — instead of a memory of which round each entry arrived in. The tag rides inside the findings file, which is hashed into the record key and copied to the digest-named list file each block points at — so a launch that drops the pointer matches no record, and the delivery floor counts the agent's read of that file exactly as it counts the brief's. - **A reporting round whose every finding the verifier rejected is retroactively dry.** The merge already removes a rejected entry from the cumulative list; from the merge that applies the last of a round's rejections, the round also stops counting as a reporting round, and the two-consecutive-dry rule reads rounds' **effective** status. Rejected means rejected — an entry confirmed at low confidence keeps its round a reporting round. Under the pipelined loop a round's verdicts land while the next round runs, so the upgrade usually arrives one round late, and that is still one round saved: a measured run held round 2 dry, watched round 3's sole finding be rejected, and then ran rounds 4 **and 5** — round 4's dry return plus the rejection already in hand was the two-dry evidence, and the fifth round audited nothing the loop had not already answered (measured; DESIGN.md — The rounds a rejected finding bought (PR #8353)). The rule leans on the rejection bar the verifier's brief already enforces — a rejection claims direct counter-evidence, never mere unverifiability — so a round retired by rejections is retired on evidence, not on doubt. **It pairs forward only, and is consulted when a round returns**: on round _k_'s dry return, first apply every verdict that has landed (the unconditional merge — the retirement takes effect at this application, not at some earlier moment), then end the loop if round _k−1_ was dry or is now retired. Round _k−1_ counts **launches, not labels**: the convergence pair is one round here — a pair member is never round _k−1_ on its own (the pair bullet's not-carried-forward rule stands), and a reporting pair retires only when every finding from **both** members is rejected. The upgrade never ends the loop by itself — a preceding dry round plus a freshly-retired round stops nothing while the next round is already in flight: that round was launched, and its return is taken whatever it says, because a launched auditor can be carrying a real Critical. This is the measured shape (round 4's return is where the loop closes under this rule — the measured run, which predates it, ran a fifth round; a cap-5 shape — under the 3-round huge-diff tier the upgrade can only ever retire rounds 1–2, since the cap round's verdicts land during its solo verification, after the loop has already ended) and the only pairing licensed here. It softens nothing else: a whiffed scope stays not-audited whatever the verdicts say, and on 3B the retirement ledger's per-chunk certificates are untouched — this rule reads at the level the round counter reads. - **Verification rides alongside the next round, not ahead of it.** When round _k_ returns with new findings, one response launches BOTH round _k_'s verifiers (Step 4, `--role verify --round k` with that round's new findings) AND round _k+1_'s auditors — build the two prompt sets first, then fire every agent together, exactly as Step 3 fans out. (Step 4's initial verification is the k=0 case of the same rule: its shards ride with the first reverse-audit launch — the convergence pair, whole-diff on 3A and per-chunk rounds 1 and 2 on 3B. The convergence pair is the one exception on the LAUNCH side: a pair member's return never triggers this rule per member — round 2's auditors are already in flight — and the pair bullets above define the one transition; the pair's findings still verify as the k=2 case, riding round 3.) The serial shape (audit → wait for verification → next round) spent 5-8 minutes per round waiting for verifiers whose results the next round's auditors never needed. Two orderings still hold: the **last** round's verification must complete before Step 6 (that ordering is what keeps unverified entries out of the report and the PR, backed by the tag backstop at the end of this step — which `compose-review` machine-checks from `findingsPath`, Step 6), and a rejected finding leaves the cumulative list at the next merge. diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 7ae56c51aa7..95befe03305 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; a huge diff is 3 when the run has a review deadline and 5 when it does not, because that reduction answers a CI ceiling and applies only where one exists) 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 — a value that is not a whole number above zero, or that is out of range (below 3, or above the plan's own tier), is ignored and leaves the tier alone — JSON Schema has no integer type here, so a fraction validates in an editor and is then discarded at runtime. 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\". Nothing here makes a loop run LONGER: a review deadline bounds a run rather than extending it, and on a huge diff setting one lowers the cap from 5 to 3 rather than raising it. 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 } } },