From ac67de2e462d113b4917b22f0adbbad802dede87 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 4 Aug 2026 21:58:15 +0800 Subject: [PATCH] fix(review): stop the reverse-audit loop while there is still time to report (#8468) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(review): stop the reverse-audit loop while there is still time to report Measured on CI run #8368 (+1699 lines): the iterative reverse audit ran to its 5-round cap, each round a per-chunk fan-out whose findings then went back through verification, and the loop consumed 3.5 of the job's 4 budgeted hours. The outer GNU-timeout kill arrived while round 5's findings were still being verified. The review died holding every confirmed finding it had; nothing reached the pull request. The loop's rounds are driven by the orchestrator, but every round begins at the same place: agent-prompt building the round's prompts. So the builder becomes the loop's clock. When the environment carries a review deadline (QWEN_REVIEW_DEADLINE_EPOCH, exported per attempt by the review workflow) and the remaining time is inside the reserve kept for the last verification, compose-review and submission (default 60 minutes, QWEN_REVIEW_DEADLINE_RESERVE_SECONDS to override), a reverse-audit round is refused: a BUDGET line on stderr, exit code 4, no prompt built and no record written. The message carries the exact unreviewedDimensions entry to file, so the disclosure that caps the verdict is the CLI's text, and Step 6 proceeds with the findings already confirmed. Local runs have no deadline and are untouched. A malformed deadline fails open — the outer kill still bounds the run, and a broken variable must degrade to today's behaviour rather than wedge every budgeted review at round 1. The verifier is deliberately not gated: the reserve exists so it can run. * fixup: scale the deadline reserve to the externally-chosen budget The budget is not this workflow's to assume: it arrives from a repository variable, a workflow input, or a /review --timeout=N comment. A fixed 60-minute reserve would consume most of a 70-minute budget and refuse the audit loop outright on a 30-minute one. The workflow now passes a reserve of a quarter of the attempt, floored at 10 minutes and capped at 60; the CLI constant remains only the fallback for a caller that sets a deadline without a reserve. * review feedback: admit the round only if IT fits, and cap deterministically Three findings from review, all taken: 1. The gate budgeted for the tail but not for the round it admits — the terminal round is by construction the one that starts closest to the boundary, so the killed-mid-verification failure survived one round wide. The gate now requires remaining >= round + reserve, where the round's cost is the previous round's, measured admission-to-admission from a stamp the builder writes (one per round; a same-round rebuild is not a round), falling back to a 30-minute constant for round 1, which starts with the most headroom. 2. The refusal was deterministic; the disclosure that caps the verdict was prose the orchestrator had to carry. The builder now records a budget-stop marker beside the prompt records and compose-review synthesizes the unreviewedDimensions entry from it — deduped against a relayed copy — so a run that drops the sentence still cannot approve past a truncated audit. 3. Exit code 4 is documented in the command's describe. Also restores the Step 5 bullet the previous commit's edit displaced (new findings merge into the cumulative list before the next round). * review feedback: pin the budget gate's all-chunks refusal and ordering Cover the two behaviours the review noted were only asserted on the bare --findings form: an exhausted budget refuses the loop's real --all-chunks round before ANY of the per-chunk records is written, and a malformed call (--round 0) still gets its validation error first — exit 4 is for a well-formed round the budget refuses, never a replacement error. Also name what the code already does: reserve=0 is the deliberate escape hatch (the gate shrinks to the round estimate alone), and the workflow's 3600s cap mirrors DEFAULT_RESERVE_SECONDS. * docs(review): describe the soft-deadline env vars for time-budgeted runs The review noted the two new variables appeared in no user-facing doc; the reserve in particular is an operator-facing knob. State what each does, the fail-open posture, and how the refusal surfaces in the verdict. * fix(cli): align budget-stop disclosure with the gate's refusal (#8468) A round-1 budget refusal left no reverse-audit records, so the Step 4/5 floor reported the deliberate stop as a rogue/unlaunched audit with a rebuild FIX the same gate deterministically rejects; the refusal's own disclosure was swallowed by the caller-echo dedup. The floor now stands down when the budget-stop marker exists, and compose-review renders the disclosure structurally, bilingually, from the marker. Also: `--role reverse-audit` requires `--round ` (an unlabeled admission stamps an entry no estimate can attribute), the budget gate runs after the plan/findings reads (a broken plan or unreadable findings deserves its own error, and nothing is stamped ahead of a buildable call), and the gate's admission boundary, measured-cost behaviour, and the workflow env contract are pinned by tests. * review: a budget stop excuses only the round it refused The budget-stop suppression keyed on the marker's existence alone, so every reverse-audit gap shape went silent once any round was refused — including the shapes that describe rounds which RAN before the budget hit. A hand-written round-1 launch is exactly as undelivered when round 3 later hits the budget, and suppressing its disclosure let 'stopped before round 3' imply the rounds that did run were faithful. Exactly one shape is by design under a marker: not-built — the refusal writes no record, so an audit with no records IS the audit the gate stopped, and its FIX (rebuild the round) would be refused by the same gate. The suppression now names that shape and no other; a rewritten, unlaunched or brief-unread round keeps its disclosure and its repair. The new test pins the operative halves: the verdict stays capped, the marker's disclosure posts, and the operator channel carries the rewritten round's exact repair. (The posted body collapses same-subject disclosures — both say 'reverse audit' — so the author sees the stop; repairs are acted on from stderr, where the rewritten fix rides.) * fix(review): fence budget state per run, and let gate errors beat budget stops Address the round-2 review threads on the reverse-audit budget gate: - Fence budget-rounds.json and budget-stop.json by the plan's own mtime. Every run rewrites the plan at its Step 1 capture, so records older than the plan belong to a previous run of the same PR: a run killed before cleanup no longer prices the next run's rounds off stale stamps (an hours-old stamp read as an hours-long round refused round 1 of a fresh budget) and no longer caps a later run's verdict on a stop that did not happen in it (R2-1, R2-2). - Refuse a structurally unbuildable plan (no chunks[], duplicate or non-integer ids) with its own error ahead of the budget gate, so the same corruption gets the same diagnosis whatever the clock says, and no budget-stop marker is written over a corrupt plan (R2-5). - Stamp a round admitted only after its build succeeds: a build that throws leaves no stamp, so the next round's cost is never measured from a build that produced nothing and floored to 600s (R2-6). - Keep the budget entry's 'reverse audit' subject out of the caller-echo prefix filter: other reverse-audit scopes the orchestrator disclosed (a twice-whiffed chunk from the rounds that DID run) are no longer silently dropped in the marker's shadow; the marker's own relays stay deduped by the phrase splice (R2-7). - Render --round unbracketed in the reverse-audit rebuild fix — the CLI refuses a round-less reverse-audit call, so the paste-and-run repair must not present the flag as optional (R2-14). - Document the deliberate one-verification overlap between the measured round estimate and the tail reserve, at both definitions (R2-13). - Test hardening, each assertion mutation-probed to fail its named mutant: a reshaped relay only the marker-phrase splice dedups (R2-8); the stamp's round label and the verifier's no-stamp invariant (R2-9); whole-line, unit-arithmetic and reserve-cap pins on the CI wiring contract (R2-10); the first-wins stamp survivor (R2-11); the reserve=0 escape hatch (R2-12). --------- Co-authored-by: verify Co-authored-by: qwen-code-dev-bot --- .github/workflows/qwen-code-pr-review.yml | 26 ++ docs/users/features/code-review.md | 2 + .../src/commands/review/agent-prompt.test.ts | 367 ++++++++++++++- .../cli/src/commands/review/agent-prompt.ts | 116 ++++- .../commands/review/check-coverage.test.ts | 9 +- .../commands/review/compose-review.test.ts | 179 ++++++++ .../cli/src/commands/review/compose-review.ts | 44 +- .../cli/src/commands/review/lib/coverage.ts | 26 +- .../src/commands/review/lib/deadline.test.ts | 420 ++++++++++++++++++ .../cli/src/commands/review/lib/deadline.ts | 389 ++++++++++++++++ .../core/src/skills/bundled/review/SKILL.md | 1 + 11 files changed, 1552 insertions(+), 27 deletions(-) create mode 100644 packages/cli/src/commands/review/lib/deadline.test.ts create mode 100644 packages/cli/src/commands/review/lib/deadline.ts diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 4173689426..6985b02805 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -816,6 +816,32 @@ jobs: OUTCOME='fatal' REASON='' KIND='' + # The in-process soft deadline: the review's reverse-audit loop + # stops itself while there is still time to verify, compose and + # post, instead of being killed by the GNU timeout below holding + # hours of confirmed findings (#8368: five audit rounds, 3.5h, + # killed mid-verification, nothing posted). Recomputed per attempt + # so a retry gets a fresh deadline; ignored by CLIs that predate + # the gate. + QWEN_REVIEW_DEADLINE_EPOCH="$(( $(date +%s) + attempt_timeout ))" + export QWEN_REVIEW_DEADLINE_EPOCH + # The budget itself is chosen outside this file — repository + # variable, workflow input, or a /review --timeout=N comment — so + # the reserve scales with whatever arrived instead of assuming a + # size: a quarter of the attempt, floored at 10 minutes (a tiny + # explicit budget degrades to skipping the audit loop, correctly) + # and capped at 60 (the verify+compose+post tail does not grow + # just because the budget did). The 3600 cap mirrors + # DEFAULT_RESERVE_SECONDS in packages/cli/src/commands/review/ + # lib/deadline.ts (the CLI's fallback when this var is absent) — + # keep the two in sync. + QWEN_REVIEW_DEADLINE_RESERVE_SECONDS="$(( attempt_timeout / 4 ))" + if [ "$QWEN_REVIEW_DEADLINE_RESERVE_SECONDS" -lt 600 ]; then + QWEN_REVIEW_DEADLINE_RESERVE_SECONDS=600 + elif [ "$QWEN_REVIEW_DEADLINE_RESERVE_SECONDS" -gt 3600 ]; then + QWEN_REVIEW_DEADLINE_RESERVE_SECONDS=3600 + fi + export QWEN_REVIEW_DEADLINE_RESERVE_SECONDS set +e # GNU timeout times out command children unless --foreground is used. timeout --kill-after=10s "${attempt_timeout}s" qwen \ diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index 50c33bf412..5b920061e5 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -352,6 +352,8 @@ The exit code is the contract a gate should read: `3` (not `2`) lets a gate distinguish "the review is blocking" from "the tool broke" — yargs already uses `1` for usage errors — without parsing any output. `--timeout-minutes` (default 120, floored at 1) terminates a hung review and exits `1`, and cancelling the command (Ctrl+C / SIGTERM) terminates the review's process group rather than orphaning it. +A time-budgeted run can also export a **soft** deadline so the review stops its open-ended reverse-audit loop while there is still time to verify, compose and post: `QWEN_REVIEW_DEADLINE_EPOCH` is the Unix-seconds moment the run will be killed, and `QWEN_REVIEW_DEADLINE_RESERVE_SECONDS` (default 3600; `0` keeps only the round estimate) is the tail that must remain for the last round's verification, `compose-review` and submission. When the remaining budget no longer fits another round plus that tail, the round builder refuses to build it, and the composed verdict discloses the truncated audit (an otherwise-Approve verdict is capped at Comment). A missing or malformed deadline leaves the review ungated — the outer timeout still bounds the run. + ## Cross-file Impact Analysis A dedicated cross-file tracer (Agent 1c) owns this walk end-to-end. When code changes modify exported functions, classes, or interfaces, it searches for all callers and checks compatibility: diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index e2b2cf5fec..44ba8babeb 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -20,12 +20,28 @@ import { afterEach, type Mock, } from 'vitest'; -import { mkdtempSync, rmSync, writeFileSync, readFileSync } from 'node:fs'; +import { + mkdirSync, + mkdtempSync, + rmSync, + utimesSync, + writeFileSync, + readFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; -import { join, resolve } from 'node:path'; +import { dirname, join, resolve } from 'node:path'; -vi.mock('../../utils/stdioHelpers.js', () => ({ writeStdoutLine: vi.fn() })); -import { writeStdoutLine } from '../../utils/stdioHelpers.js'; +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: vi.fn(), + writeStderrLine: vi.fn(), +})); +import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; +import { + DEADLINE_ENV, + RESERVE_ENV, + readBudgetStop, + readRoundStamps, +} from './lib/deadline.js'; import { buildChunkAgentPrompt, buildChunkLaunchPrompt, @@ -38,6 +54,7 @@ import { import { readRecordedPrompts, briefPath, + promptRecordDir, wasDeliveredVerbatim, } from './lib/prompt-record.js'; @@ -372,6 +389,7 @@ describe('agent-prompt (command boundary)', () => { role: 'reverse-audit', chunk: 14, findings, + round: 1, }), ).not.toThrow(); const recorded = readRecordedPrompts(plan); @@ -379,7 +397,9 @@ describe('agent-prompt (command boundary)', () => { expect(keys).toHaveLength(1); // The chunk in the key (the delivery check finds the record by it), plus // the findings digest — each round is its own record now. - expect(keys[0]).toMatch(/^reverse-audit--chunk-14--[0-9a-f]{12}$/); + expect(keys[0]).toMatch( + /^reverse-audit--chunk-14--round-1--[0-9a-f]{12}$/, + ); const briefText = readFileSync(briefPath(plan, keys[0]), 'utf8'); expect(briefText).toContain('offset=4024, limit=176'); // chunk 14 only expect(briefText).not.toContain('offset=3807'); // not chunk 13 @@ -485,6 +505,7 @@ describe('--all-chunks — every auditor of a Step 5 round, in one call', () => role: 'reverse-audit', 'all-chunks': true, findings, + round: 1, }); const printed = (writeStdoutLine as unknown as Mock).mock @@ -550,6 +571,7 @@ describe('--all-chunks — every auditor of a Step 5 round, in one call', () => role: 'reverse-audit', 'all-chunks': true, findings, + round: 1, }), ).toThrow(/no `chunks\[\]`/); expect(readRecordedPrompts(plan).size).toBe(0); @@ -584,6 +606,7 @@ describe('--all-chunks — every auditor of a Step 5 round, in one call', () => role: 'reverse-audit', 'all-chunks': true, findings, + round: 1, }), ).toThrow(/no positive integer id/); } finally { @@ -621,6 +644,7 @@ describe('--all-chunks — every auditor of a Step 5 round, in one call', () => role: 'reverse-audit', 'all-chunks': true, findings, + round: 1, }), ).toThrow(pattern); // Refused BEFORE any brief, record or stdout block — a partial round @@ -708,6 +732,7 @@ describe('--all-chunks — every auditor of a Step 5 round, in one call', () => role: 'reverse-audit', 'all-chunks': true, findings, + round: 1, }); const printed = (writeStdoutLine as unknown as Mock).mock .calls[0][0] as string; @@ -744,6 +769,7 @@ describe('--all-chunks — every auditor of a Step 5 round, in one call', () => 'all-chunks': true, findings, rules: rulesFile, + round: 1, }); const keys = [...readRecordedPrompts(plan).keys()]; expect(keys).toHaveLength(3); @@ -1426,7 +1452,7 @@ describe('--findings — fold the list in, print one block, record EXACTLY that }); it('a reverse auditor gets the do-not-re-report framing', () => { - const { printed, plan } = run({ role: 'reverse-audit' }); + const { printed, plan } = run({ role: 'reverse-audit', round: 1 }); expect(printed).toContain('Already confirmed — do not re-report these'); // and NOT the verifier's framing — the mirror of the assertion above. expect(printed).not.toContain('The findings you are ruling on'); @@ -1441,7 +1467,11 @@ describe('--findings — fold the list in, print one block, record EXACTLY that // --findings ` per chunk per round. The findings fold above the // chunk-scoped prompt; the record is that chunk's block, findings-free, keyed by // the chunk. (PLAN's chunks are 13/14/15 — chunk 14 is offset 4024, limit 176.) - const { printed, plan } = run({ role: 'reverse-audit', chunk: 14 }); + const { printed, plan } = run({ + role: 'reverse-audit', + chunk: 14, + round: 1, + }); expect(printed).toContain('Already confirmed — do not re-report these'); expect(printed).toContain('foo.ts:10 — the collision drops arguments'); expect(printed).toContain('offset=4024, limit=176'); // this chunk's range only @@ -1471,6 +1501,7 @@ describe('--findings — fold the list in, print one block, record EXACTLY that plan, role: 'reverse-audit', findings, + round: 1, }); const printed = (writeStdoutLine as unknown as Mock).mock .calls[0][0] as string; @@ -1502,6 +1533,7 @@ describe('--findings — fold the list in, print one block, record EXACTLY that plan, role: 'reverse-audit', findings, + round: 1, }), ).not.toThrow(); }); @@ -1747,6 +1779,7 @@ describe('buildWholeDiffBlock — the agents that walk the whole diff', () => { role: 'reverse-audit', chunk: 999, findings, + round: 1, }), ).toThrow(/the plan has no chunk 999/); } finally { @@ -2420,3 +2453,323 @@ describe('verify and reverse-audit briefs — the Step 4/5 methodology, in code' } }); }); + +describe('the reverse-audit budget gate — the loop must end by reporting', () => { + // Measured on CI run #8368 (+1699 lines): the audit loop ran to the 5-round + // cap, spent 3.5 of the job's 4 budgeted hours, and the outer kill arrived + // while round 5's findings were still being verified — nothing was posted. + // The gate turns that into a refusal at the round BUILDER, where the + // orchestrator has to come for its prompts. + const dirs: string[] = []; + beforeEach(() => { + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + }); + afterEach(() => { + delete process.env[DEADLINE_ENV]; + delete process.env[RESERVE_ENV]; + process.exitCode = undefined; + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); + }); + + function call(role: string, extra: Record = {}): string { + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-')); + dirs.push(dir); + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + const findings = join(dir, 'findings.md'); + writeFileSync(findings, ''); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role, + findings, + ...extra, + }); + return plan; + } + + it('refuses a round inside the reserve: exit 4, no prompt, no record', () => { + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + const plan = call('reverse-audit', { round: 2 }); + + expect(process.exitCode).toBe(4); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + // A refused round leaves no record for a delivery check to expect an + // agent against. + expect(readRecordedPrompts(plan).size).toBe(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('BUDGET:'); + expect(msg).toContain( + '`reverse audit — stopped before round 2 by the review time budget`', + ); + expect(msg).toContain('proceed to Step 6'); + // The deterministic half: the marker compose-review synthesizes the + // verdict-capping disclosure from, written even though nothing was built. + expect(readBudgetStop(plan)?.entry).toBe( + 'reverse audit — stopped before round 2 by the review time budget', + ); + // A refused round is not an admission; it must not be stamped as one. + expect(readRoundStamps(plan)).toHaveLength(0); + }); + + it('builds normally when the deadline is far, and when there is none', () => { + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const plan = call('reverse-audit', { round: 1 }); + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + // The admission is stamped WITH its round label, so the next round's gate + // can measure this one: both stamp consumers key on `round` — an + // unlabeled `{round: null}` stamp would slip the one-per-round guard and + // price a same-round rebuild at the 600s floor. + expect(readRoundStamps(plan)).toEqual([ + { round: 1, atMs: expect.any(Number) }, + ]); + + (writeStdoutLine as unknown as Mock).mockClear(); + delete process.env[DEADLINE_ENV]; + call('reverse-audit', { round: 1 }); + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + }); + + it('does not gate the verifier — the reserve exists so it can run', () => { + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-v-')); + dirs.push(dir); + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + const findings = join(dir, 'findings.md'); + writeFileSync(findings, '- x.test.ts:3 — off-by-one in retry cap\n'); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'verify', + findings, + }); + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + expect((writeStderrLine as unknown as Mock).mock.calls).toHaveLength(0); + // And it leaves no admission stamp: the stamps are the reverse-audit + // loop's clock, and a verifier build hoisted into the stamping path + // would corrupt the round measurements without ever being gated. + expect(readRoundStamps(plan)).toHaveLength(0); + }); + + it('honours a shorter reserve override', () => { + // 600s reserve + the 1800s round-1 estimate = 2400s required. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 2500); + process.env[RESERVE_ENV] = '600'; + call('reverse-audit', { round: 4 }); + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + }); + + it('refuses an --all-chunks round too: exit 4, and none of the per-chunk records', () => { + // The loop's real Step 5 form is --role reverse-audit --all-chunks + // --findings …, and that path writes one record PER CHUNK — so "no + // record written" is at its strongest here: PLAN has three chunks, and + // none of the three may exist for a delivery check to expect agents for. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + const plan = call('reverse-audit', { 'all-chunks': true, round: 3 }); + + expect(process.exitCode).toBe(4); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(readRecordedPrompts(plan).size).toBe(0); + expect(readBudgetStop(plan)?.entry).toBe( + 'reverse audit — stopped before round 3 by the review time budget', + ); + expect(readRoundStamps(plan)).toHaveLength(0); + }); + + it('throws the validation error first: a malformed call beats the budget refusal', () => { + // The ordering the gate's comment claims, pinned: an invalid --round gets + // the validation error even with the budget exhausted — exit 4 is for a + // well-formed round the time budget refuses, never a replacement error. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + expect(() => call('reverse-audit', { round: 0 })).toThrow( + /--round is a 1-based round number/, + ); + expect(process.exitCode).toBeUndefined(); + expect((writeStderrLine as unknown as Mock).mock.calls).toHaveLength(0); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + }); + + it('rejects a --round-less reverse-audit call — the clock keys on the label', () => { + // SKILL.md's Step 5 always passes --round : the label is the record + // key's round part and the budget gate's accounting unit. An unlabeled + // admission would stamp {round: null}, which the one-per-round guard + // cannot dedup and no later estimate can attribute. + expect(() => call('reverse-audit')).toThrow(/requires --round/); + expect(process.exitCode).toBeUndefined(); + expect((writeStderrLine as unknown as Mock).mock.calls).toHaveLength(0); + }); + + it('a broken plan still throws when the budget is exhausted — reads beat the gate', () => { + // The gate needs only the plan's PATH, but it must not speak first: a + // refusal would record a budget stop against a plan that cannot even + // parse, and stderr would say "proceed to Step 6" over a call that was + // never buildable. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-broken-')); + dirs.push(dir); + const plan = join(dir, 'no-such-dir', 'plan.json'); + const findings = join(dir, 'findings.md'); + writeFileSync(findings, ''); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + round: 2, + }), + ).toThrow(/cannot read the plan/); + expect(process.exitCode).toBeUndefined(); + expect((writeStderrLine as unknown as Mock).mock.calls).toHaveLength(0); + }); + + it('an unreadable findings file throws before the round is stamped admitted', () => { + // The stamp says the round was admitted; if the build then failed on its + // findings read, the next round's cost would be measured from a round + // that produced nothing. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-nofind-')); + dirs.push(dir); + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings: join(dir, 'no-such-findings.md'), + round: 2, + }), + ).toThrow(/cannot read the findings/); + expect(process.exitCode).toBeUndefined(); + expect(readRoundStamps(plan)).toHaveLength(0); + }); + + it('a build that throws after admission leaves no stamp', () => { + // The stamp is written after the build succeeds, not at admission: a + // stamp is the next round's cost measurement, and one left by a build + // that produced nothing would be floored to 600s — widening the next + // admission by 1200s in exactly the unsafe direction (a terminal round + // admitted on headroom it does not have). + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-throw-')); + dirs.push(dir); + const plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + const findings = join(dir, 'findings.md'); + writeFileSync(findings, ''); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + round: 2, + chunk: 99, // passes validation and both reads; the BUILD throws + }), + ).toThrow(/no chunk 99/); + expect(readRoundStamps(plan)).toHaveLength(0); + }); + + it('a structurally unbuildable plan gets its own error, never a budget stop', () => { + // Parses, but no round could ever be built from it. Near the deadline + // the gate must not speak first: refusing "on the budget" would write a + // marker over a corrupt plan, say "proceed to Step 6", and preempt the + // actionable repair (re-run the Step 1 capture) — and the same + // diagnosis must not flip with the clock. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-nochunks-')); + dirs.push(dir); + const plan = join(dir, 'plan.json'); + writeFileSync( + plan, + JSON.stringify({ diffPathAbsolute: PLAN.diffPathAbsolute }), + ); + const findings = join(dir, 'findings.md'); + writeFileSync(findings, ''); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + round: 2, + 'all-chunks': true, + }), + ).toThrow(/has no `chunks\[\]`/); + expect(process.exitCode).toBeUndefined(); + expect(readBudgetStop(plan)).toBeNull(); + expect((writeStderrLine as unknown as Mock).mock.calls).toHaveLength(0); + }); + + it("ignores a previous run's stamps — the plan rewrite fences them off", () => { + // A run killed by the outer deadline leaves budget-rounds.json behind + // (Step 9 cleanup never ran). The next review of the same PR rewrites + // the plan at capture, so those stamps predate the plan and must not + // price this run's rounds: an 8h-old stamp would read as an ~8h round + // and refuse round 1 of a fresh budget. + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-stale-')); + dirs.push(dir); + const plan = join(dir, 'plan.json'); + const recordDir = promptRecordDir(plan); + mkdirSync(recordDir, { recursive: true }); + writeFileSync( + join(recordDir, 'budget-rounds.json'), + JSON.stringify([{ round: 4, atMs: Date.now() - 28_800_000 }]), + ); + writeFileSync(plan, JSON.stringify(PLAN)); // this run's capture, after + const findings = join(dir, 'findings.md'); + writeFileSync(findings, ''); + // 5500s remaining fits reserve + the 1800s CONSTANT (5400) — admitted — + // while the stale ~28800s measurement would refuse. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 5500); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + round: 1, + }); + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + }); + + it("measures the previous round's cost at the gate, not the constant", () => { + // Round 1 admitted with a far deadline (it stamps); backdate the stamp + // 3000s. The second deadline leaves room for reserve + the CONSTANT + // round estimate (3600 + 1800 fits in 5500) but not for reserve + the + // MEASURED 3000s — so only a gate that measures refuses. The unsafe + // direction is under-estimation: admitting a terminal round that does + // not fit, the killed-mid-verification outcome this gate exists to + // prevent. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const plan = call('reverse-audit', { round: 1 }); + expect(readRoundStamps(plan)).toHaveLength(1); + writeFileSync( + join(promptRecordDir(plan), 'budget-rounds.json'), + JSON.stringify([{ round: 1, atMs: Date.now() - 3_000_000 }]), + ); + // Date the plan capture before the backdated stamp: the stamp belongs to + // THIS run, and the previous-run fence keys on the plan's mtime. + const captured = (Date.now() - 4_000_000) / 1000; + utimesSync(plan, captured, captured); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 5500); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings: join(dirname(plan), 'findings.md'), + round: 2, + }); + expect(process.exitCode).toBe(4); + // The stderr line names the MEASURED cost — a ~50-minute round, not the + // ~30-minute constant. + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('BUDGET:'); + expect(msg).toContain('~50-minute round'); + // A refusal is not an admission. + expect(readRoundStamps(plan)).toHaveLength(1); + }); +}); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 877926e973..ef9a58a52e 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -41,7 +41,14 @@ import type { CommandModule } from 'yargs'; import { createHash } from 'node:crypto'; import { readFileSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; -import { writeStdoutLine } from '../../utils/stdioHelpers.js'; +import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; +import { + expectedRoundSeconds, + reverseAuditBudgetExhausted, + reverseAuditBudgetMessage, + stampRound, + writeBudgetStop, +} from './lib/deadline.js'; import { READ_FILE_CHAR_CAP, chunkIdsProblem, @@ -1372,24 +1379,24 @@ function runRoster(report: PlanReport, planPath: string, rules?: string): void { * as `--roster`: one call, labelled numbered blocks, an end marker, and nothing * left to reconstruct. */ -function runAllChunks( - report: PlanReport, - planPath: string, - role: RoleId, - findingsContent: string, - rules?: string, - round?: number, -): void { +/** + * The structural floor an --all-chunks round stands on, refused as its OWN + * error. The same refusal coverage makes (`readPlan`), made BEFORE any + * brief, record or block is written. Filtering the unusable ids out instead + * shrank the round: `[13, "x", 15]` printed a complete-looking two-auditor + * round with one territory silently gone, and a duplicated id resolved both + * blocks to the first matching chunk and keyed them to one record — the + * second territory never audited, under an end marker that says the round + * is whole. Also called ahead of the budget gate: a plan no round could + * ever be built from must get this diagnosis whatever the clock says — + * refusing it as a budget stop would write a marker over a corrupt plan, + * say "proceed to Step 6", and preempt the one actionable repair. + */ +function requireAuditableChunks(report: PlanReport): DiffChunk[] { if (!Array.isArray(report.chunks) || report.chunks.length === 0) { throw new Error('agent-prompt: the plan has no `chunks[]`.'); } const chunks = report.chunks as DiffChunk[]; - // The same refusal coverage makes (`readPlan`), made BEFORE any brief, - // record or block is written. Filtering the unusable ids out instead shrank - // the round: `[13, "x", 15]` printed a complete-looking two-auditor round - // with one territory silently gone, and a duplicated id resolved both blocks - // to the first matching chunk and keyed them to one record — the second - // territory never audited, under an end marker that says the round is whole. const problem = chunkIdsProblem(chunks.map((c) => c?.id)); if (problem) { throw new Error( @@ -1398,6 +1405,18 @@ function runAllChunks( 'Re-run the Step 1 capture; do not hand-edit the plan.', ); } + return chunks; +} + +function runAllChunks( + report: PlanReport, + planPath: string, + role: RoleId, + findingsContent: string, + rules?: string, + round?: number, +): void { + const chunks = requireAuditableChunks(report); const digest = findingsDigest(findingsContent, rules); const roundPart = round !== undefined ? `--round-${round}` : ''; const blocks = chunks.map((c, i) => { @@ -1575,6 +1594,19 @@ function runAgentPrompt(args: AgentPromptArgs): void { ); } } + // The Step 5 loop's clock keys on the round label — the record key's + // round part, the identity line, and the budget gate's per-round stamps + // all read it, and SKILL.md's Step 5 calls always pass it. A round-less + // call would stamp an unlabeled admission no later estimate can + // attribute, so it gets its own error here with every other malformed + // call. + if (role === 'reverse-audit' && !hasRound) { + bad( + '--role reverse-audit builds one round of the Step 5 loop and ' + + 'requires --round : the label keys the record and the budget ' + + "gate's per-round accounting.", + ); + } } else if (hasFindings) { // `--findings` with no role: it has no prompt to fold into. A territory chunk // agent reviews the diff, not a findings list. Name the roles it needs from the @@ -1703,6 +1735,45 @@ function runAgentPrompt(args: AgentPromptArgs): void { } } + // The all-chunks dispatch below builds from `chunks[]`; validate that + // structure BEFORE the budget gate, so a plan no round could ever be + // built from gets its structural error whatever the clock says — refused + // as a budget stop it would write a marker over a corrupt plan and tell + // the orchestrator to proceed to Step 6 when the actionable repair is a + // Step 1 re-capture. + if (args.allChunks && args.role && findingsContent !== undefined) { + requireAuditableChunks(report); + } + + // The budget gate — after every validation and every file read, because a + // malformed call or a broken plan deserves its own error (refusing here + // would record a budget stop against a plan that cannot even parse) — + // before any build or record: a refused round must leave no prompt on + // disk for a later check to expect an agent for. + // Reverse-audit only: the loop is the one open-ended stage, and the + // reserve this gate protects exists precisely to let verify/compose run. + // What must fit is the round being admitted PLUS the tail — a gate that + // admits on the reserve alone hands the terminal round a start right at + // the boundary, which is the killed-mid-verification failure one round + // wide. The round's cost is the previous round's, measured admission to + // admission. The admission is stamped AFTER the build succeeds (below), + // never here: the stamp is what the next round's gate measures cost from, + // and a build that throws must not leave one behind — priced from a + // failed build, the next round would be floored to the 600s minimum, + // widening admission in exactly the unsafe direction. + if (args.role === 'reverse-audit') { + const spent = reverseAuditBudgetExhausted( + process.env, + expectedRoundSeconds(args.plan, args.round), + ); + if (spent !== null) { + writeBudgetStop(args.plan, spent, args.round); + writeStderrLine(reverseAuditBudgetMessage(spent, args.round)); + process.exitCode = 4; + return; + } + } + if (args.allChunks && args.role && findingsContent !== undefined) { runAllChunks( report, @@ -1712,6 +1783,11 @@ function runAgentPrompt(args: AgentPromptArgs): void { rules, args.round, ); + // Admitted AND built: stamp now, so the next round's gate can measure + // this one — see the gate comment above for why not earlier. + if (args.role === 'reverse-audit') { + stampRound(args.plan, args.round); + } return; } @@ -1769,6 +1845,12 @@ function runAgentPrompt(args: AgentPromptArgs): void { : prompt; recordPrompt(args.plan, key, printed); writeStdoutLine(printed); + // Admitted AND built — the single-build twin of the all-chunks stamp + // above (a `--chunk ` rebuild lands here; the one-per-round guard in + // `stampRound` keeps it from shrinking its own round's observed cost). + if (args.role === 'reverse-audit') { + stampRound(args.plan, args.round); + } } export const agentPromptCommand: CommandModule = { @@ -1776,7 +1858,9 @@ export const agentPromptCommand: CommandModule = { describe: "Build a review agent's launch prompt from the plan (the diff path, its line " + "ranges and the agent's own brief are welded in, not left to the caller to " + - 'remember)', + 'remember). Exit codes: 0 built; 4 the review time budget refused another ' + + 'reverse-audit round (a termination rule, not an error — see the BUDGET line ' + + 'on stderr); anything else is a bad call or a broken plan.', builder: (yargs) => yargs .option('plan', { diff --git a/packages/cli/src/commands/review/check-coverage.test.ts b/packages/cli/src/commands/review/check-coverage.test.ts index 3a8f194e5d..84d95f520a 100644 --- a/packages/cli/src/commands/review/check-coverage.test.ts +++ b/packages/cli/src/commands/review/check-coverage.test.ts @@ -1660,7 +1660,11 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', () // the dogfooded failure was the orchestrator hand-appending `(round N)` to // the identity line because the CLI gave it nowhere else to put it. expect(fix).toMatch(/no hand-added round number/); - expect(fix).toContain('[--round ]'); + // UNBRACKETED: `agent-prompt` refuses a round-less reverse-audit call, so + // a paste-and-run repair that bracketed --round as optional handed the + // orchestrator a first attempt the validation rejects. + expect(fix).toContain('--round '); + expect(fix).not.toContain('[--round ]'); }); it('names a rewritten verifier launch as itself too', () => { @@ -1680,6 +1684,9 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', () // told apart by their findings digest, not by that flag. expect(fix).toMatch(/no hand-added shard number,/); expect(fix).not.toContain('shard number (--round bakes it in)'); + // For verify the flag stays BRACKETED — only a repeat verification round + // passes one, unlike reverse-audit where the CLI refuses without it. + expect(fix).toContain('[--round ]'); }); it('flags a reverse audit built but whose agent never opened its brief', () => { diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index ac7d60e652..0c5a2bc8e1 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -17,6 +17,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { createHash } from 'node:crypto'; import { promptRecordDir, briefPath } from './lib/prompt-record.js'; +import { writeBudgetStop } from './lib/deadline.js'; import { getGhHost, setGhHost } from './lib/gh.js'; import { parseLedger } from './lib/ledger.js'; import { countInlineFindings } from './lib/inline-counts.js'; @@ -480,6 +481,184 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve expect(r.body).not.toContain('no blockers'); }); + it('a budget-stop marker caps APPROVE at COMMENT with nothing relayed by the caller', () => { + // The round builder refused a round and recorded the refusal; the + // disclosure that caps the verdict is synthesized from that marker, not + // from a sentence the orchestrator remembered to carry. + const plan = coveredPlan(); + writeBudgetStop( + plan, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 4, + ); + const r = composeReview(base({ planPath: plan })); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain( + 'reverse audit — stopped before round 4 by the review time budget', + ); + expect(r.body).not.toContain('LGTM'); + + // And said once when the orchestrator DID relay it. + const r2 = composeReview( + base({ + planPath: plan, + unreviewedDimensions: [ + 'reverse audit — stopped before round 4 by the review time budget', + ], + }), + ); + expect(r2.body.split('review time budget').length - 1).toBe(1); + + // Still once when the relay was RESHAPED — an orchestrator prefix ahead + // of the subject. The coverage prefix filter cannot see this one (it no + // longer starts with `reverse audit — `); only the marker-phrase splice + // dedups it, so this is the assertion that fails when the splice goes. + const r3 = composeReview( + base({ + planPath: plan, + unreviewedDimensions: [ + 'step 5 — reverse audit — stopped before round 4 by the review time budget', + ], + }), + ); + expect(r3.body.split('review time budget').length - 1).toBe(1); + }); + + it('the marker does not shadow other reverse-audit scopes the caller disclosed', () => { + // The budget entry claims the subject `reverse audit`; the caller-echo + // prefix filter must not let it swallow a DIFFERENT reverse-audit scope + // reported with its own reason — a whiffed chunk from the rounds that + // DID run is exactly what a partially-run audit still owes the author. + const plan = coveredPlan(); + writeBudgetStop( + plan, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 3, + ); + const r = composeReview( + base({ + planPath: plan, + unreviewedDimensions: [ + "reverse audit — chunk 2's auditor returned nothing substantive twice", + ], + }), + ); + expect(r.body).toContain( + 'Not reviewed: reverse audit — stopped before round 3 by the review time budget.', + ); + expect(r.body).toContain( + "Not reviewed: reverse audit — chunk 2's auditor returned nothing substantive twice.", + ); + // The marker's own disclosure still renders exactly once. + expect(r.body.split('review time budget').length - 1).toBe(1); + }); + + it('a round-1 budget stop stands alone — no rogue-audit gap, no rebuild FIX', () => { + // The gate refused round 1, so no reverse-audit record exists. Without + // the marker the floor would report the absence as a rogue/unlaunched + // audit and direct a rebuild the same gate deterministically refuses + // (exit 4) — misattributing a deliberate stop. The budget disclosure + // must stand alone, and the remediation must stay silent. + const plan = coveredPlan([]); // nothing ran: the round-1 refusal shape + writeBudgetStop( + plan, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 1, + ); + // Not base(): its planPath default runs coveredPlan() again on the same + // path and would re-record the Step 4/5 pair this case means to lack. + const r = composeReview({ planPath: plan, env: ENV, modelId: MODEL }); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain( + 'Not reviewed: reverse audit — stopped before round 1 by the review time budget.', + ); + expect(r.body).not.toContain('no auditor was launched'); + expect(r.body).not.toContain('its prompt was built'); + expect(r.remediation.join(' ')).not.toContain('reverse audit:'); + }); + + it('renders the budget stop bilingually on a Han-description PR', () => { + // Every sibling structural disclosure carries a zh pair; the budget stop + // used to ride the caller-prose path and posted English into both halves. + const plan = coveredPlan(['verify', 'reverse-audit'], { han: true }); + writeBudgetStop( + plan, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 4, + ); + // Not base(): its planPath default runs coveredPlan() again on the same + // path and would overwrite the han-stamped plan. + const r = composeReview({ planPath: plan, env: ENV, modelId: MODEL }); + expect(r.body).toContain( + 'Not reviewed: reverse audit — stopped before round 4 by the review time budget.', + ); + expect(r.body).toContain( + '未审查：反向审计——评审时间预算不足，未能开始第 4 轮。', + ); + }); + + it('a budget stop does not launder a rewritten pre-stop round', () => { + // Round 1 RAN — with a hand-written launch that opened its brief but + // never got the built prompt — and round 2 was then refused on the + // budget. The marker explains the audit that never ran; it says nothing + // about the one that did, and the rewritten disclosure is still owed: + // without it, "stopped before round 2" implies round 1 was faithful. + const plan = coveredPlan(['verify']); + const d = promptRecordDir(plan); + const brief = briefPath(plan, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const built = + 'You are review agent `reverse-audit`.\n' + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, 'reverse-audit.txt'), built); + transcript( + 'v-ra-rewritten', + `Audit the diff for gaps. Your brief: ${brief}. Diff: ${DIFF}.`, + { toolCalls: 2, opens: [brief] }, + ); + writeBudgetStop( + plan, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 2, + ); + + // Not base(): its planPath default runs coveredPlan() again on the same + // path and would lay a verbatim reverse-audit pair over this fixture. + const r = composeReview({ planPath: plan, env: ENV, modelId: MODEL }); + expect(r.event).toBe('COMMENT'); + // The marker still discloses and caps… + expect(r.body).toContain( + 'stopped before round 2 by the review time budget', + ); + // …and the rewritten round is NOT laundered: the operator channel carries + // its exact repair. (The posted body collapses same-subject disclosures — + // both say "reverse audit" — so the author sees the stop; the rewritten + // repair rides stderr, which is where repairs are acted on.) + expect(r.remediation.join(' ')).toContain('reverse audit:'); + expect(r.remediation.join(' ')).toContain('EXACTLY what it prints'); + }); + it('an uncoverable chunk caps APPROVE at COMMENT and names the chunk', () => { const r = composeReview( base({ uncoverableChunks: ['chunk 5 (src/big.min.js)'] }), diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 1143a210bf..e33a6fcd4b 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -30,6 +30,11 @@ import { verificationGaps, TranscriptsUnavailableError, } from './lib/coverage.js'; +import { + BUDGET_STOP_PHRASE, + budgetStopDisclosure, + readBudgetStop, +} from './lib/deadline.js'; import { shellQuotePath } from './lib/shell-quote.js'; import { gh, setGhHost } from './lib/gh.js'; import { @@ -351,6 +356,36 @@ function composeReviewBody( subjectZh?: string; reasonZh?: string; }> = []; + // The budget-stop marker: when the reverse-audit round builder refused a + // round on the review's time budget, it recorded the refusal beside the + // prompt records. Synthesizing the disclosure from the marker makes the + // verdict cap deterministic — the orchestrator's own copy of the entry + // (the stderr instruction asks for one) is a courtesy to the terminal + // reader, and a run that drops the sentence still cannot approve past a + // truncated audit. Rendered STRUCTURAL, both languages, like every other + // coverage entry — the orchestrator's relayed copy is English-only prose, + // so the marker's phrase dedups it out and the two channels never say it + // twice. + // The marker's entry is tracked by reference: its relays are deduped by + // the phrase splice here, so the caller-echo filter below must NOT also + // prefix-match on its `reverse audit` subject — that shadow silently + // dropped every OTHER reverse-audit scope the orchestrator disclosed + // (`reverse audit — chunk 2's auditor returned nothing substantive + // twice`), in exactly the runs where a partial audit makes such scopes + // likeliest. + let budgetEntry: (typeof coverageEntries)[number] | undefined; + if (input.planPath) { + const stop = readBudgetStop(input.planPath); + if (stop !== null) { + for (let i = unreviewed.length - 1; i >= 0; i--) { + if (unreviewed[i].includes(BUDGET_STOP_PHRASE)) { + unreviewed.splice(i, 1); + } + } + budgetEntry = budgetStopDisclosure(stop.round ?? undefined); + coverageEntries.push(budgetEntry); + } + } // The fixes for the gaps above, for stderr — never for the body. The gap says // what the review cannot certify, to the PR author; the remediation names the // command that repairs it, to the orchestrator. #7012's public body was fourteen @@ -881,8 +916,15 @@ function composeReviewBody( for (const d of unreviewed) { if (seenCaller.has(d)) continue; // a caller pasting itself twice seenCaller.add(d); + // The budget-stop entry never prefix-matches: its relays are already + // deduped by the marker phrase above, and letting its `reverse audit` + // subject claim the prefix swallowed unrelated reverse-audit scopes the + // caller disclosed with their own reasons (a bare subject echo still + // dedups). const echoesCoverage = covEntries.some( - (e) => d === e.subject || d.startsWith(`${e.subject} — `), + (e) => + d === e.subject || + (e !== budgetEntry && d.startsWith(`${e.subject} — `)), ); if (!echoesCoverage) callerLeft.push(d); } diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 7900c42a08..778aa12d32 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -71,6 +71,7 @@ import { } from './roster.js'; import { BRIEFS } from './agent-briefs.js'; import { chunkIdsProblem } from './diff-plan.js'; +import { readBudgetStop } from './deadline.js'; import { shellQuotePath } from './shell-quote.js'; export interface CoverageFromTranscripts { @@ -938,7 +939,13 @@ type GapText = Record, GapEntry>; const rebuildFix = (role: 'verify' | 'reverse-audit', noun: string): string => `build the prompt with \`"\${QWEN_CODE_CLI:-qwen}" review agent-prompt ` + `--plan --role ${role} --findings [--rules ] ` + - `[--round ]\` ` + + // --round is MANDATORY for a reverse-audit build (`agent-prompt` refuses a + // round-less call — the label keys the record and the budget gate's + // accounting), so the paste-and-run repair must not bracket it as optional: + // an orchestrator honouring the bracket convention would have its first + // repair attempt rejected. Verify genuinely takes it or not (only a repeat + // verification round passes one), so its brackets stay. + (role === 'reverse-audit' ? `--round \` ` : `[--round ]\` `) + (role === 'reverse-audit' ? `(an early round with nothing confirmed passes an empty file; ` : `(pass the shard's findings, never an empty file — a verifier that sees ` + @@ -1249,8 +1256,23 @@ export function verificationGaps( (k) => k === 'reverse-audit' || k.startsWith('reverse-audit--'), ); const reverse = bestDelivery(reverseKeys); + // A budget-stop marker means the round builder itself refused the reverse + // audit on the run's time budget. Exactly ONE gap shape is then by design: + // `not-built` — the refusal writes no record, so an audit with no records + // is the audit the gate stopped, and the gap's FIX (rebuild the round) + // would be refused by the very gate that stopped it — exit 4, + // deterministically, time only moves forward. compose-review synthesizes + // the marker's own disclosure instead: it names the stop honestly and caps + // the verdict. Every OTHER shape describes a round that predates the + // refusal — a built round nobody launched, a launch the orchestrator + // rewrote, a brief never opened — and those disclosures are still owed: a + // hand-written round-1 launch is exactly as undelivered when round 3 later + // hits the budget, and suppressing it would let "stopped before round 3" + // imply the rounds that did run were faithful. + const budgetStopped = readBudgetStop(planPath) !== null; + const reverseByDesign = budgetStopped && reverse === 'not-built'; // A repairable reverse-audit gap only at high: medium is complete without it. - const reverseGap = !balancedMedium && reverse !== 'ok'; + const reverseGap = !balancedMedium && !reverseByDesign && reverse !== 'ok'; if (reverseGap) { // The fix template carries `--plan `; a literal `` pasted into a // POSIX shell parses as input redirection, so the one repair round Step 6 diff --git a/packages/cli/src/commands/review/lib/deadline.test.ts b/packages/cli/src/commands/review/lib/deadline.test.ts new file mode 100644 index 0000000000..d8acc12bff --- /dev/null +++ b/packages/cli/src/commands/review/lib/deadline.test.ts @@ -0,0 +1,420 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { + mkdtempSync, + readFileSync, + rmSync, + utimesSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { + BUDGET_STOP_PHRASE, + DEADLINE_ENV, + RESERVE_ENV, + DEFAULT_RESERVE_SECONDS, + DEFAULT_ROUND_SECONDS, + budgetStopEntry, + budgetStopEntryZh, + expectedRoundSeconds, + readBudgetStop, + readRoundStamps, + reverseAuditBudgetExhausted, + reverseAuditBudgetMessage, + stampRound, + writeBudgetStop, +} from './deadline.js'; + +const NOW_MS = 1_754_000_000_000; +const NOW_S = NOW_MS / 1000; +const REQUIRED = DEFAULT_RESERVE_SECONDS + DEFAULT_ROUND_SECONDS; + +/** This test run's plan-capture instant: stamps and markers are fenced by + * the plan's mtime (a rerun rewrites the plan), and the fixture clock here + * is `NOW_MS`, not the wall clock — so the plan must be dated before the + * records the tests write against it. */ +const PLAN_CAPTURED_MS = NOW_MS - 10_000_000; + +function backdatePlan(p: string, atMs: number = PLAN_CAPTURED_MS): void { + utimesSync(p, atMs / 1000, atMs / 1000); +} + +describe('reverseAuditBudgetExhausted — the round must fit, and its tail', () => { + it('stays silent when no deadline is set — every local run', () => { + expect( + reverseAuditBudgetExhausted({}, DEFAULT_ROUND_SECONDS, NOW_MS), + ).toBeNull(); + expect( + reverseAuditBudgetExhausted( + { [DEADLINE_ENV]: '' }, + DEFAULT_ROUND_SECONDS, + NOW_MS, + ), + ).toBeNull(); + }); + + it('admits a round while round-plus-reserve still fits', () => { + const env = { [DEADLINE_ENV]: String(NOW_S + REQUIRED + 60) }; + expect( + reverseAuditBudgetExhausted(env, DEFAULT_ROUND_SECONDS, NOW_MS), + ).toBeNull(); + }); + + it('admits at EXACT cover — remaining equal to reserve plus round cost', () => { + // The `>=` is the documented rule: exact cover admits. Pin the boundary + // so a future "safety margin" edit cannot silently end the loop one + // round early whenever remaining lands exactly on it. + const env = { [DEADLINE_ENV]: String(NOW_S + REQUIRED) }; + expect( + reverseAuditBudgetExhausted(env, DEFAULT_ROUND_SECONDS, NOW_MS), + ).toBeNull(); + }); + + it('refuses a round the reserve alone would have admitted', () => { + // The review's table: a round admitted at reserve + ε runs 28-53 minutes + // and leaves the tail nothing. The gate must count the round itself. + const env = { + [DEADLINE_ENV]: String(NOW_S + DEFAULT_RESERVE_SECONDS + 300), + }; + const spent = reverseAuditBudgetExhausted( + env, + DEFAULT_ROUND_SECONDS, + NOW_MS, + ); + expect(spent).toEqual({ + remainingSeconds: DEFAULT_RESERVE_SECONDS + 300, + reserveSeconds: DEFAULT_RESERVE_SECONDS, + expectedRoundSeconds: DEFAULT_ROUND_SECONDS, + }); + }); + + it('honours a reserve override, in both directions', () => { + const env = { + [DEADLINE_ENV]: String(NOW_S + 600 + DEFAULT_ROUND_SECONDS + 60), + [RESERVE_ENV]: '600', + }; + expect( + reverseAuditBudgetExhausted(env, DEFAULT_ROUND_SECONDS, NOW_MS), + ).toBeNull(); + env[RESERVE_ENV] = '1200'; + expect( + reverseAuditBudgetExhausted(env, DEFAULT_ROUND_SECONDS, NOW_MS), + ).not.toBeNull(); + }); + + it('honours the reserve-0 escape hatch — only the round itself must fit', () => { + // `r >= 0` (not `> 0`) is the documented escape hatch: reserve 0 keeps + // only the refusal of a round that cannot finish before the deadline. + // An edit to `> 0` would silently fall back to the 3600s default and + // refuse the next round a full hour before the operator's deadline. + const env = { + [DEADLINE_ENV]: String(NOW_S + DEFAULT_ROUND_SECONDS + 60), + [RESERVE_ENV]: '0', + }; + expect( + reverseAuditBudgetExhausted(env, DEFAULT_ROUND_SECONDS, NOW_MS), + ).toBeNull(); + env[DEADLINE_ENV] = String(NOW_S + DEFAULT_ROUND_SECONDS - 60); + const spent = reverseAuditBudgetExhausted( + env, + DEFAULT_ROUND_SECONDS, + NOW_MS, + ); + expect(spent?.reserveSeconds).toBe(0); + }); + + it('fails OPEN on a malformed deadline — the outer kill still bounds the run', () => { + for (const bad of ['soon', 'NaN', '-5', '0']) { + expect( + reverseAuditBudgetExhausted( + { [DEADLINE_ENV]: bad }, + DEFAULT_ROUND_SECONDS, + NOW_MS, + ), + ).toBeNull(); + } + }); + + it('ignores a malformed reserve and keeps the default', () => { + const env = { + [DEADLINE_ENV]: String(NOW_S + REQUIRED - 1), + [RESERVE_ENV]: 'an hour', + }; + const spent = reverseAuditBudgetExhausted( + env, + DEFAULT_ROUND_SECONDS, + NOW_MS, + ); + expect(spent?.reserveSeconds).toBe(DEFAULT_RESERVE_SECONDS); + }); + + it('reports a past deadline as negative remaining, not a crash', () => { + const env = { [DEADLINE_ENV]: String(NOW_S - 120) }; + const spent = reverseAuditBudgetExhausted( + env, + DEFAULT_ROUND_SECONDS, + NOW_MS, + ); + expect(spent?.remainingSeconds).toBe(-120); + }); +}); + +describe('the round-cost estimate — measured when it can be', () => { + const dirs: string[] = []; + afterEach(() => { + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); + }); + function plan(): string { + const dir = mkdtempSync(join(tmpdir(), 'deadline-')); + dirs.push(dir); + const p = join(dir, 'plan.json'); + writeFileSync(p, '{}'); + backdatePlan(p); + return p; + } + + it('falls back to the constant when nothing has been measured', () => { + expect(expectedRoundSeconds(plan(), 1, NOW_MS)).toBe(DEFAULT_ROUND_SECONDS); + }); + + it('uses the previous round, admission to admission', () => { + const p = plan(); + stampRound(p, 1, NOW_MS - 2_400_000); // round 1 admitted 40 min ago + expect(expectedRoundSeconds(p, 2, NOW_MS)).toBe(2400); + }); + + it('measures the NEWEST previous round when several are on file', () => { + const p = plan(); + stampRound(p, 1, NOW_MS - 3_000_000); // round 1 admitted 50 min ago + stampRound(p, 2, NOW_MS - 1_200_000); // round 2 admitted 20 min ago + // Scanning oldest-first would report 3000; the loop's cost trend is the + // newest admission's. + expect(expectedRoundSeconds(p, 3, NOW_MS)).toBe(1200); + }); + + it('ignores a stamp of the SAME round — a rebuild is not a round', () => { + const p = plan(); + stampRound(p, 1, NOW_MS - 2_400_000); + stampRound(p, 2, NOW_MS - 60_000); // round 2 admitted a minute ago + // Rebuilding round 2 must not read its own stamp as "rounds cost 60s"; + // it reaches past it to round 1's. + expect(expectedRoundSeconds(p, 2, NOW_MS)).toBe(2400); + }); + + it('floors a suspiciously quick observation', () => { + const p = plan(); + stampRound(p, 1, NOW_MS - 30_000); + expect(expectedRoundSeconds(p, 2, NOW_MS)).toBe(600); + }); + + it('stamps once per round, and the FIRST admission is the one that survives', () => { + // First-wins is the load-bearing half: "refresh the stamp on rebuild" + // (last-wins) also leaves one stamp, but a chunk rebuild late in a round + // would then collapse the next round's estimate to the 600s floor and + // the gate would admit a terminal round on headroom it does not have. + const p = plan(); + stampRound(p, 1, NOW_MS - 100); + stampRound(p, 1, NOW_MS); + expect(readRoundStamps(p)).toEqual([{ round: 1, atMs: NOW_MS - 100 }]); + }); + + it('ignores stamps older than the plan — a previous run of the same PR', () => { + // The stamps key on the per-PR-stable plan path, and a run killed by the + // outer deadline never reaches cleanup — but every run rewrites the plan + // at its Step 1 capture, so the plan's mtime fences the runs apart. + // Without the fence, an hours-old stamp reads as an hours-long round and + // refuses round 1 of a fresh budget. + const p = plan(); + stampRound(p, 1, PLAN_CAPTURED_MS - 28_800_000); // 8h before this capture + stampRound(p, 2, PLAN_CAPTURED_MS - 27_000_000); + expect(readRoundStamps(p)).toEqual([]); + expect(expectedRoundSeconds(p, 1, NOW_MS)).toBe(DEFAULT_ROUND_SECONDS); + // A stamp from THIS run still measures, with the stale ones alongside. + stampRound(p, 1, NOW_MS - 2_400_000); + expect(readRoundStamps(p)).toEqual([ + { round: 1, atMs: NOW_MS - 2_400_000 }, + ]); + expect(expectedRoundSeconds(p, 2, NOW_MS)).toBe(2400); + }); + + it('persists a round-less stamp as null, outside the one-per-round guard', () => { + // The guard dedups by round LABEL; an unlabeled stamp has none to dedup + // by, so it persists — pinned here because `agent-prompt` rejects a + // round-less reverse-audit call and nothing else exercises the shape. + const p = plan(); + stampRound(p, undefined, NOW_MS - 100); + stampRound(p, undefined, NOW_MS); + expect(readRoundStamps(p)).toEqual([ + { round: null, atMs: NOW_MS - 100 }, + { round: null, atMs: NOW_MS }, + ]); + }); +}); + +describe('the budget-stop marker — the deterministic half of the disclosure', () => { + const dirs: string[] = []; + afterEach(() => { + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); + }); + + function stopPlan(): string { + const dir = mkdtempSync(join(tmpdir(), 'deadline-stop-')); + dirs.push(dir); + const p = join(dir, 'plan.json'); + writeFileSync(p, '{}'); + backdatePlan(p); + return p; + } + + it('round-trips, entry text and all', () => { + const p = stopPlan(); + writeBudgetStop( + p, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 4, + NOW_MS, + ); + const stop = readBudgetStop(p); + expect(stop?.entry).toBe( + 'reverse audit — stopped before round 4 by the review time budget', + ); + expect(stop?.entryZh).toBe('反向审计——评审时间预算不足，未能开始第 4 轮'); + expect(stop?.round).toBe(4); + expect(readBudgetStop(join(dirname(p), 'other.json'))).toBeNull(); + }); + + it('a marker from before the plan capture is a previous run — read as none', () => { + // Run 1 refuses a round, writes the marker, and is killed before Step 9 + // cleanup; run 2 rewrites the plan, admits every round, and never trips + // the gate. Its verdict must not be capped by a stop that did not happen + // in it. + const p = stopPlan(); + writeBudgetStop( + p, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 3, + PLAN_CAPTURED_MS - 28_800_000, // 8h before this run's capture + ); + expect(readBudgetStop(p)).toBeNull(); + // A marker written by THIS run replaces it and reads back. + writeBudgetStop( + p, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 3, + NOW_MS, + ); + expect(readBudgetStop(p)?.round).toBe(3); + }); + + it('the dedup phrase travels with the entry it identifies', () => { + // compose-review dedups the orchestrator's relayed copy by this phrase; + // a reword of the entry that left the phrase behind would post the + // disclosure twice. + expect(budgetStopEntry(2)).toContain(BUDGET_STOP_PHRASE); + expect(budgetStopEntry(undefined)).toContain(BUDGET_STOP_PHRASE); + }); + + it('the zh entry pairs the en one, for a numbered round and without', () => { + expect(budgetStopEntryZh(4)).toBe( + '反向审计——评审时间预算不足，未能开始第 4 轮', + ); + expect(budgetStopEntryZh(undefined)).toBe( + '反向审计——评审时间预算不足，未能开始下一轮', + ); + }); +}); + +describe('the CI wiring contract', () => { + it('the workflow exports the exact env names the gate reads', () => { + // Renaming either side compiles, lints, and leaves every test green — + // the CLI just never sees a deadline, every round is admitted, and the + // outer kill returns. Pin the two halves of the contract together. + const workflow = readFileSync( + join( + dirname(fileURLToPath(import.meta.url)), + '..', + '..', + '..', + '..', + '..', + '..', + '.github', + 'workflows', + 'qwen-code-pr-review.yml', + ), + 'utf8', + ); + // Whole line, not substring: `toContain('export QWEN_REVIEW_DEADLINE_EPOCH')` + // stayed green when the variable was renamed to any superstring + // (`..._EPOCH_SECONDS` is the natural drift beside `..._RESERVE_SECONDS`) + // and when the export was commented out — both leave the CLI deadline-less + // and the gate failing open on every round. + expect(workflow).toMatch(new RegExp(`^\\s*export ${DEADLINE_ENV}$`, 'm')); + expect(workflow).toMatch(new RegExp(`^\\s*export ${RESERVE_ENV}$`, 'm')); + // The units are part of the contract: a milliseconds deadline admits every + // round forever (remaining ≈ 1.7e12); minutes instead of seconds refuses + // round 1 on every budgeted run. Pin the arithmetic that fixes both to + // whole seconds of epoch / of reserve. + expect(workflow).toContain( + `${DEADLINE_ENV}="$(( $(date +%s) + attempt_timeout ))"`, + ); + expect(workflow).toContain(`${RESERVE_ENV}="$(( attempt_timeout / 4 ))"`); + // The workflow's reserve cap documents itself as mirroring + // DEFAULT_RESERVE_SECONDS ("keep the two in sync") — enforce the mirror, + // so a one-sided bump diverges a test instead of the CI tail. + expect(workflow).toContain(`-gt ${DEFAULT_RESERVE_SECONDS}`); + }); +}); + +describe('reverseAuditBudgetMessage', () => { + it('names the round, both costs, and the exact disclosure entry', () => { + const msg = reverseAuditBudgetMessage( + { + remainingSeconds: 1500, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 3, + ); + expect(msg).toContain('BUDGET:'); + expect(msg).toContain('25 minute(s) remain'); + expect(msg).toContain('~30-minute round'); + expect(msg).toContain('60-minute reserve'); + expect(msg).toContain(`\`${budgetStopEntry(3)}\``); + expect(msg).toContain('budget-stop marker'); + expect(msg).toContain('proceed to Step 6'); + expect(msg).toContain('do not relaunch auditors'); + }); + + it('says "the next round" when no round number was passed', () => { + const msg = reverseAuditBudgetMessage( + { + remainingSeconds: -30, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + undefined, + ); + expect(msg).toContain('0 minute(s) remain'); + expect(msg).toContain('stopped before the next round'); + }); +}); diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts new file mode 100644 index 0000000000..ffc6d590e3 --- /dev/null +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -0,0 +1,389 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The reverse-audit loop's clock. +// +// The iterative reverse audit (Step 5) is the one stage of a review whose cost +// 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 5-round cap. 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 +// round 5's findings were still being verified — the review died holding +// every confirmed finding it had, and nothing reached the pull request. +// +// So a time-budgeted run tells the CLI its deadline, and the round *builder* +// refuses to start a round that no longer fits. Two quantities have to fit, +// not one: the tail (the last verification, compose-review, submission — the +// reserve) AND the round being admitted, whose cost the gate now measures +// instead of guessing — the loop's terminal round is by construction the one +// that starts closest to the boundary, so a gate that admits a round on the +// reserve alone re-creates the killed-mid-verification failure one round wide. +// Each admission is stamped on disk; the next admission reads the previous +// stamp and uses the observed round cost, falling back to a conservative +// constant for round 1 (which starts with the most headroom). +// +// The refusal is deterministic twice over: the builder exits 4 with no prompt +// built (there is no round to launch without it), and a `budget-stop.json` +// marker is written beside the prompt records so `compose-review` synthesizes +// the verdict-capping disclosure itself — the orchestrator's copy of the +// entry is a courtesy to the terminal reader, not the mechanism. +// +// A run with no deadline in its environment — every local run — is untouched. +// A malformed deadline fails OPEN (the gate stays silent): the outer kill +// still bounds the run, and a broken environment variable must degrade to +// today's behaviour, not wedge every budgeted review at round 1. + +import { mkdirSync, readFileSync, statSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { promptRecordDir } from './prompt-record.js'; + +/** Unix seconds at which the review process will be killed. Set by CI. */ +export const DEADLINE_ENV = 'QWEN_REVIEW_DEADLINE_EPOCH'; + +/** Override for the tail reserve, in seconds. */ +export const RESERVE_ENV = 'QWEN_REVIEW_DEADLINE_RESERVE_SECONDS'; + +/** + * What must still fit after the last reverse-audit round completes: the + * verification of that round's findings, compose-review, anchor resolution + * and the submission itself. + * + * The measured round estimate ALSO contains one verification pass — a round + * is admitted only after the previous round's findings were verified and + * merged (SKILL.md Step 5), so an admission-to-admission span includes the + * verification between them — which means the gate holds back roughly one + * verification more than the terminal round strictly needs. That overlap is + * deliberate margin, not double-entry bookkeeping that slipped: round costs + * trend UP (each round re-reads the diff against a longer findings list, and + * repair relaunches land mid-loop), so the previous round's measurement + * under-predicts the next in exactly the runs that end near the boundary — + * and the two error directions are not symmetric. Over-reserving ends the + * loop at most one round early, disclosed as a budget stop; under-reserving + * is #8368 — killed mid-verification, holding every confirmed finding. + * + * This is only the fallback: the budget itself is + * chosen outside the CLI (a repository variable, a workflow input, a + * `/review --timeout=N` comment), so the review workflow passes a reserve + * scaled to the budget it resolved rather than trusting this constant to fit + * an arbitrary one. A local run has no deadline and no reserve at all. + */ +export const DEFAULT_RESERVE_SECONDS = 3600; + +/** + * The admission estimate for a round nothing has measured yet — round 1, or + * a record dir that lost its stamps. Thirty minutes covers a measured + * small-PR round (~17 min, #8456) with margin; a large PR's first round may + * exceed it, but round 1 starts with the most headroom, and every later + * admission uses the previous round's observed cost instead of this. + */ +export const DEFAULT_ROUND_SECONDS = 1800; + +/** Floor for an observed round cost — a quick same-round rebuild is not a round. */ +const MIN_OBSERVED_ROUND_SECONDS = 600; + +interface RoundStamp { + round: number | null; + atMs: number; +} + +const STAMPS_FILE = 'budget-rounds.json'; +const STOP_FILE = 'budget-stop.json'; + +/** + * Slack for the run-epoch fence below: absorbs the sub-millisecond skew + * between a file mtime (fractional) and `Date.now()` (integral) when a + * record is written moments after the plan. Real cross-run gaps are minutes + * to hours; two seconds is noise against them. + */ +const RUN_EPOCH_SLACK_MS = 2000; + +/** + * The run's epoch: records older than this predate the run and are ignored. + * + * The stamps and the stop marker key on the plan path, which is stable per + * PR — but every run rewrites the plan at its Step 1 capture (`fetch-pr` / + * `plan-diff` / `capture-local`), so the plan's own mtime dates the run. A + * budgeted run killed by the outer deadline leaves its records behind (the + * Step 9 cleanup never ran, and the workflow's start-of-run sweep removes + * worktrees, not these files); without the fence the next review of the + * same PR would price its rounds off the previous run's stamps (an + * hours-old stamp reads as an hours-long round and refuses round 1 of a + * fresh budget) and cap its verdict on a stop that did not happen in this + * run. An unstatable plan disables the fence — fail open, like every other + * malformed input this module reads. + */ +function runEpochMs(planPath: string): number { + try { + return statSync(planPath).mtimeMs - RUN_EPOCH_SLACK_MS; + } catch { + return Number.NEGATIVE_INFINITY; + } +} + +/** + * The admission stamps written so far THIS RUN, oldest first. Unreadable → + * empty; a stamp older than the plan's own capture belonged to a previous + * run of the same PR and is dropped (see `runEpochMs`). + */ +export function readRoundStamps(planPath: string): RoundStamp[] { + try { + const raw = readFileSync( + join(promptRecordDir(planPath), STAMPS_FILE), + 'utf8', + ); + const parsed = JSON.parse(raw) as unknown; + if (!Array.isArray(parsed)) return []; + const epoch = runEpochMs(planPath); + return parsed.filter( + (e): e is RoundStamp => + typeof e === 'object' && + e !== null && + typeof (e as RoundStamp).atMs === 'number' && + (e as RoundStamp).atMs >= epoch, + ); + } catch { + return []; + } +} + +/** + * Record an admission. One stamp per round: a per-chunk rebuild of a round + * already admitted must not shrink the observed cost of the round before it. + * Write errors are swallowed for the same reason `recordPrompt` swallows + * them — a read-only tmp dir must not stop a review being built. + */ +export function stampRound( + planPath: string, + round: number | undefined, + nowMs: number = Date.now(), +): void { + try { + const stamps = readRoundStamps(planPath); + if (round !== undefined && stamps.some((s) => s.round === round)) return; + stamps.push({ round: round ?? null, atMs: nowMs }); + const dir = promptRecordDir(planPath); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, STAMPS_FILE), JSON.stringify(stamps)); + } catch { + // Informational bookkeeping; the gate falls back to its constant. + } +} + +/** + * What the round about to be admitted is expected to cost, in seconds: the + * observed cost of the previous round (admission-to-admission — its agents, + * their verification, the orchestration between) when a stamp exists, else + * the conservative constant. The span deliberately overlaps the tail + * reserve by one verification pass — see `DEFAULT_RESERVE_SECONDS` for why + * that margin is kept rather than netted out. A stamp of the SAME round is + * ignored — that is a rebuild, and measuring it would report a round as + * cheap because its prompts were built twice quickly. + */ +export function expectedRoundSeconds( + planPath: string, + round: number | undefined, + nowMs: number = Date.now(), +): number { + const stamps = readRoundStamps(planPath); + for (let i = stamps.length - 1; i >= 0; i--) { + const s = stamps[i]; + if (round !== undefined && s.round === round) continue; + const observed = Math.round((nowMs - s.atMs) / 1000); + return Math.max(MIN_OBSERVED_ROUND_SECONDS, observed); + } + return DEFAULT_ROUND_SECONDS; +} + +export interface BudgetExhausted { + /** Whole seconds until the deadline; can be negative when already past. */ + remainingSeconds: number; + /** The tail reserve the remaining time failed to clear. */ + reserveSeconds: number; + /** The admission estimate for the refused round itself. */ + expectedRoundSeconds: number; +} + +/** + * Decide whether another reverse-audit round still fits the review's time + * budget: the remaining time must cover the round being admitted AND the + * tail after it. Returns `null` when it does — or when no (well-formed) + * deadline is present, which is every local run. + */ +export function reverseAuditBudgetExhausted( + env: NodeJS.ProcessEnv, + roundCostSeconds: number, + nowMs: number = Date.now(), +): BudgetExhausted | null { + const raw = env[DEADLINE_ENV]; + if (raw === undefined || raw.trim() === '') return null; + const deadline = Number(raw); + if (!Number.isFinite(deadline) || deadline <= 0) return null; + + let reserve = DEFAULT_RESERVE_SECONDS; + const reserveRaw = env[RESERVE_ENV]; + if (reserveRaw !== undefined && reserveRaw.trim() !== '') { + const r = Number(reserveRaw); + // `r >= 0` (not `> 0`) is deliberate: 0 is the escape hatch that shrinks + // the requirement to the round estimate alone, keeping only the refusal + // of a round that cannot finish before the deadline itself. + if (Number.isFinite(r) && r >= 0) reserve = r; + } + + const remainingSeconds = Math.floor(deadline - nowMs / 1000); + if (remainingSeconds >= reserve + roundCostSeconds) return null; + return { + remainingSeconds, + reserveSeconds: reserve, + expectedRoundSeconds: roundCostSeconds, + }; +} + +export interface BudgetStop { + /** The exact `unreviewedDimensions` entry, composed here so the text that + * caps the verdict is this module's in both channels. */ + entry: string; + /** The Chinese pair of `entry` — the posted body is bilingual. */ + entryZh: string; + round: number | null; + remainingSeconds: number; + reserveSeconds: number; + atMs: number; +} + +/** + * The phrase that identifies the budget-stop disclosure wherever it is + * relayed. Exported so `compose-review` dedups the orchestrator's copy + * against the marker's by the same text the entry itself is spelled with — + * a reword of the entry moves its key along with it. + */ +export const BUDGET_STOP_PHRASE = 'review time budget'; + +/** + * The disclosure as structural parts, both languages: compose-review renders + * it through the same bilingual coverage path as every other structural gap. + * The entry texts below are these parts joined, never the other way around. + */ +export function budgetStopDisclosure(round: number | undefined): { + subject: string; + reason: string; + subjectZh: string; + reasonZh: string; +} { + const which = round !== undefined ? `round ${round}` : 'the next round'; + const whichZh = round !== undefined ? `第 ${round} 轮` : '下一轮'; + return { + subject: 'reverse audit', + reason: `stopped before ${which} by the ${BUDGET_STOP_PHRASE}`, + subjectZh: '反向审计', + reasonZh: `评审时间预算不足，未能开始${whichZh}`, + }; +} + +/** The disclosure entry, spelled once for the marker AND the stderr message. */ +export function budgetStopEntry(round: number | undefined): string { + const d = budgetStopDisclosure(round); + return `${d.subject} — ${d.reason}`; +} + +/** The Chinese pair of `budgetStopEntry` — the marker carries both. */ +export function budgetStopEntryZh(round: number | undefined): string { + const d = budgetStopDisclosure(round); + return `${d.subjectZh}——${d.reasonZh}`; +} + +/** + * Persist the refusal beside the prompt records, where `compose-review` + * reads it back and synthesizes the verdict-capping disclosure without + * depending on the orchestrator to relay a sentence. Write errors are + * swallowed: the stderr instruction still carries the entry, and a gate + * that cannot write must still refuse. + */ +export function writeBudgetStop( + planPath: string, + spent: BudgetExhausted, + round: number | undefined, + nowMs: number = Date.now(), +): void { + try { + const dir = promptRecordDir(planPath); + mkdirSync(dir, { recursive: true }); + const stop: BudgetStop = { + entry: budgetStopEntry(round), + entryZh: budgetStopEntryZh(round), + round: round ?? null, + remainingSeconds: spent.remainingSeconds, + reserveSeconds: spent.reserveSeconds, + atMs: nowMs, + }; + writeFileSync(join(dir, STOP_FILE), JSON.stringify(stop, null, 2)); + } catch { + // See above: refusing is the load-bearing half. + } +} + +/** + * The budget-stop marker, if THIS RUN wrote one. Unreadable → null; so is a + * marker older than the plan's own capture — a previous run's refusal, left + * behind by a kill before cleanup, must not cap a verdict on a stop that + * did not happen in this run (see `runEpochMs`). A marker without a numeric + * `atMs` cannot prove which run it belongs to and is treated the same way — + * only this module writes markers, and it always dates them. + */ +export function readBudgetStop(planPath: string): BudgetStop | null { + try { + const raw = readFileSync( + join(promptRecordDir(planPath), STOP_FILE), + 'utf8', + ); + const parsed = JSON.parse(raw) as unknown; + if ( + typeof parsed !== 'object' || + parsed === null || + typeof (parsed as BudgetStop).entry !== 'string' || + typeof (parsed as BudgetStop).atMs !== 'number' || + (parsed as BudgetStop).atMs < runEpochMs(planPath) + ) { + return null; + } + return parsed as BudgetStop; + } catch { + return null; + } +} + +/** + * The refusal, spelled as the termination rule it is. Printed to stderr by + * `agent-prompt` alongside exit code 4; the disclosure sentence matches the + * `budget-stop.json` marker byte for byte, so both channels cap the verdict + * with one text. + */ +export function reverseAuditBudgetMessage( + spent: BudgetExhausted, + round: number | undefined, +): string { + const minutesLeft = Math.max(0, Math.floor(spent.remainingSeconds / 60)); + const reserveMinutes = Math.round(spent.reserveSeconds / 60); + const roundMinutes = Math.round(spent.expectedRoundSeconds / 60); + const which = round !== undefined ? `round ${round}` : 'the next round'; + return ( + `BUDGET: ${minutesLeft} minute(s) remain before this review's deadline — ` + + `not enough for the ~${roundMinutes}-minute round being asked for plus ` + + `the ${reserveMinutes}-minute reserve kept for its verification, ` + + `compose-review and submission — so no further reverse-audit round will ` + + `be built. This is the loop's termination rule, not an error: do not ` + + `rebuild ${which} and do not relaunch auditors. A budget-stop marker has ` + + `been recorded and compose-review will disclose it and cap the verdict ` + + `itself; also add exactly this entry to unreviewedDimensions so the ` + + `terminal report says it too — ` + + `\`${budgetStopEntry(round)}\` — ` + + `and proceed to Step 6 with the findings already confirmed. Spend what ` + + `time remains only on verifying findings already in hand, composing, and ` + + `submitting. A review that stops here still reports everything it ` + + `proved; a review that runs past its deadline is killed holding all of it.` + ); +} diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index dee1f4aca6..c548f29346 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -619,6 +619,7 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c - Stop after **two consecutive dry rounds**. 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. - Stop after **5 rounds** regardless (hard cap), and say so in the output rather than implying convergence. - New findings from each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. +- **The round builder is also the loop's clock.** In a time-budgeted run (CI exports `QWEN_REVIEW_DEADLINE_EPOCH`; a local run normally has no deadline and is untouched), `agent-prompt --role reverse-audit` refuses to build a round that no longer fits: the remaining time must cover **the round itself** (estimated from the previous round's measured cost — the builder stamps each admission — or a conservative constant for round 1) **plus** the reserve kept for its verification, compose-review and submission. On refusal it prints a `BUDGET:` line to stderr and exits **4**. That refusal is a termination rule, not an error — do not rebuild the round, do not relaunch auditors, and do not retry the command. The builder also records a budget-stop marker that `compose-review` reads directly, so the verdict is capped whether or not you relay anything; still add the exact entry the message names (`reverse audit — stopped before round by the review time budget`) to `unreviewedDimensions` so the terminal report and the body agree, and proceed to Step 6 with the findings already confirmed — spending what remains only on verifying findings already in hand, composing, and submitting. Why this exists, measured: a +1699-line PR's CI review ran the audit loop to the 5-round cap, spent 3.5 of its 4 budgeted hours there, and was killed by the outer CI timeout while round 5's findings were still being verified — every confirmed finding died with it. A review that stops on the budget still reports everything it proved; one that runs past it reports nothing. **Reverse audit findings go through Step 4 verification like any other finding.** They used to skip it on the theory that the auditor "already has full context." That premise fails exactly when the diff is large — the auditor with the least room to think was the one whose output nobody checked.