diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 9f36c75dc03..48fd449ee38 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -2773,8 +2773,18 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { // The command runs install + builds + tests in one process; the agent's default // 120s shell timeout would kill it — the very failure this command prevents, one // level up. So the block tells the agent to pass the tool's max, 600000ms. + // + // Pinned PER SITE, not per prompt: three sites supply the directive (the + // first call, the resume paragraph's "Same …", the efficacy probe's + // "… too"), so a whole-prompt `toContain` stayed green with any one of + // them deleted — and the deleted first-call directive is exactly the + // 120s mid-install kill this assertion's own comment names. const p = buildRoleBrief(PR_PLAN, '7', { planPath: '/abs/tmp/plan.json' }); - expect(p).toContain(`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}`); + expect(p).toContain( + `Invoke it with \`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\`:`, + ); + expect(p).toContain(`Same \`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\``); + expect(p).toContain(`\`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\` too`); }); it('tells Agent 7 how to CONTINUE a run one call could not finish', () => { @@ -2784,8 +2794,14 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { // call teaches the agent to report a truncated dimension as a finished // one — which is what three live reviews did. const p = buildRoleBrief(PR_PLAN, '7', { planPath: '/abs/tmp/plan.json' }); - expect(p).toContain('testScope.notRun'); - expect(p).toContain('"clamped": true'); + // Anchored to the continuation PARAGRAPH's own sentence: the bare + // literals are also supplied verbatim by the role-7 base brief + // (agent-briefs), so `toContain('testScope.notRun')` stayed green with + // the whole paragraph deleted. + expect(p).toContain( + 'Work is left when `testScope.notRun` is non-empty, or when any ' + + '`test[]` entry has `"clamped": true`', + ); // Asserted on the CONTINUATION BLOCK ALONE, which is the whole point. The // first cut of this test searched the entire prompt: `--resume` matched the @@ -2802,11 +2818,45 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { // `resolve` — not spelled as POSIX literals: on Windows the prompt carries // `C:\\abs\\tmp\\plan.json`, and a hardcoded expectation fails there for a // reason that has nothing to do with the continuation block. - expect(resumeBlock[0]).toContain('review build-test'); + // The FULL wrapper, on THIS block: the whole-prompt pins are satisfied + // by the first invocation block and vice versa, so a wrapper deleted + // from either one shipped green — and a resume block without it execs + // bare PATH `qwen`, an old global that lacks `build-test` entirely. + expect(resumeBlock[0]).toContain( + '"${QWEN_CODE_CLI:-qwen}" review build-test', + ); expect(resumeBlock[0]).toContain(`--plan ${resolve('/abs/tmp/plan.json')}`); + // The tree too: a continuation against a different tree measures a + // different run. Never asserted before — a dropped `--worktree` line + // shipped green. + expect(resumeBlock[0]).toContain( + `--worktree ${resolve('.qwen/tmp/review-pr-6766')}`, + ); expect(resumeBlock[0]).toContain( `--out ${join(resolve('/abs/tmp'), 'qwen-review-pr-6766-build-test.json')}`, ); + expect(resumeBlock[0]).toContain('--resume'); + + // And the FIRST invocation block carries its own wrapper and tree — the + // same two elements, scoped to the block that must supply them. + const firstBlock = fences.find( + (f) => f.includes('review build-test') && !f.includes('--resume'), + ); + expect(firstBlock).toBeDefined(); + expect(firstBlock).toContain('"${QWEN_CODE_CLI:-qwen}" review build-test'); + expect(firstBlock).toContain( + `--worktree ${resolve('.qwen/tmp/review-pr-6766')}`, + ); + + // The third-shape sentence, at BOTH prose sites — the role-7 base brief + // and the welded resume paragraph each carry it, so a single toContain + // is satisfied by either and a one-site deletion ships green. Counted, + // not just matched: deleting the sentence anywhere drops the count, and + // an agent missing it treats the endedBeforeTests shape as continuable, + // spending a MAX_RESUME_CALLS slot on a --resume that can only answer + // "ended before its test phase". + expect(p.split('"endedBeforeTests": true').length - 1).toBe(2); + expect(p.split('do not spend a continuation on it').length - 1).toBe(2); }); it('welds the PR into Agent 0 — an unqualified number judges the wrong issue', () => { diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index fa30a2eca0b..2f0820285c3 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -1430,13 +1430,14 @@ export function buildRoleBrief( 'at 106s and `packages/cli` at 401s, before the rest). Work is left when ' + '`testScope.notRun` is non-empty, or when any `test[]` entry has ' + '`"clamped": true` — a suite the budget started too late and killed, which ' + - 'says nothing about the suite. A third shape carries no field at all: a ' + + 'says nothing about the suite. A third shape ends before any suite: a ' + 'single-package repo whose budget ran out before its one suite has an ' + - 'empty `test[]` and no `testScope`, and only its `note` says so — read ' + - 'the note before calling the dimension finished. That shape cannot be ' + - 'continued (a continuation has no recorded scope to read, and answers ' + - '"ended before its test phase" without running anything): report the ' + - 'dimension UNFINISHED and do not spend a continuation on it. A resumed ' + + 'empty `test[]`, no `testScope`, and `"endedBeforeTests": true` — the ' + + "report's own stamp — with the note naming the unrun suite. That shape " + + 'cannot be continued (a continuation has no recorded scope to read; a ' + + '`--resume` on it answers "ended before its test phase" and points at a ' + + 'fresh run): report the dimension UNFINISHED and do not spend a ' + + 'continuation on it. A resumed ' + 'call skips install and build and ' + 'runs only what is left, merging into the SAME report file. Same ' + `\`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\`, and at most ` + diff --git a/packages/cli/src/commands/review/build-test.test.ts b/packages/cli/src/commands/review/build-test.test.ts index 2dc7f410507..8299a633a34 100644 --- a/packages/cli/src/commands/review/build-test.test.ts +++ b/packages/cli/src/commands/review/build-test.test.ts @@ -449,6 +449,30 @@ describe('runBuildTest', () => { expect(rep.testScope?.caveat).toBeUndefined(); expect(rep.note).toContain('no package to build'); expect(rep.note).toContain('complete answer'); + // Not a probe: the stamp must not appear on an ordinary zero-affected run. + expect(rep.buildOnly).toBeUndefined(); + + // The probe stamp rides the zero-affected return too — this producer + // path branches on buildOnly for its note but used to drop the stamp, + // so a resumed probe report lost its probe answer. + const probe = runBuildTest({ + plan: planPath, + worktree: root, + timeout: 5, + install: false, + buildOnly: true, + exec: (command) => { + calls.push(command); + return { + command, + exitCode: 0, + seconds: 1, + timedOut: false, + output: '', + }; + }, + }); + expect(probe.buildOnly).toBe(true); }); it('runs nothing but discloses the caveat for out-of-workspace files that are not inert', () => { @@ -931,6 +955,12 @@ describe('runBuildTest', () => { // And the note must not claim tests it did not run. expect(buildOnly.note).toContain('build-only'); expect(buildOnly.note).not.toContain('ran the tests'); + // The WRITE half of the probe stamp, on the main results-initializer + // path: a probe report without it read as a completed zero-suite run on + // --resume — the probe/completed misclassification the split exists to + // prevent. The non-probe sibling must not carry it. + expect(buildOnly.buildOnly).toBe(true); + expect(withTests.buildOnly).toBeUndefined(); }); it('scopes build AND tests to the changed workspace and its dependents', () => { @@ -1577,6 +1607,11 @@ describe('runBuildTest', () => { expect(rep.note).not.toContain('defines no test script'); expect(rep.note).toContain('not run: .'); expect(rep.ok).toBe(true); + // The structural stamp: build green, no probe, no scope — without it the + // resume split read this exact report as COMPLETED with no suite to run, + // certified the one existing suite as finished, and dropped the fresh + // re-run advice that is the only path to ever running it. + expect(rep.endedBeforeTests).toBe(true); }); it('runs the AFFECTED workspace first, so the budget trims dependents, never the changed suite', () => { @@ -3204,6 +3239,114 @@ describe('runBuildTest', () => { expect(rep.note).not.toContain('reached every suite'); }); + it('a COMPLETED zero-suite run is not "ended before its test phase"', () => { + // A single-root package with no test script finishes a fresh run + // completely: test [], no scope, ok true. Calling that "ended before + // its test phase" was self-contradictory beside the report's own note + // ("defines no test script, so no tests ran"), and its re-run advice + // re-derived the same zero-suite answer at the price of a full fresh + // install+build. The split is structural — ok and the buildOnly stamp + // — never the note's prose. + threePackages(); + const outPath = join(root, 'report.json'); + const base = { + toolchain: 'npm', + run: runId(), + affected: ['packages/core'], + buildSet: ['packages/core'], + widenedWith: [], + install: okResult('npm ci --no-audit --no-fund'), + build: [okResult('npm run build --workspace="packages/core"')], + test: [], + ok: true, + timedOut: [], + note: 'the package defines no test script, so no tests ran', + }; + writeFileSync(outPath, JSON.stringify(base)); + const attempt = () => + runBuildTest({ + plan: planPath, + worktree: root, + out: outPath, + timeout: 60, + install: true, + resume: true, + exec: okResult, + }); + const completed = attempt(); + expect(completed.note).toContain('completed with no suite to run'); + expect(completed.note).not.toContain('ended before its test phase'); + expect(completed.note).not.toContain('Re-run build-test'); + + // The deliberate probe keeps the early-end message: its report has no + // tests and no scope BY CHOICE, and only the stamp tells it apart. + writeFileSync(outPath, JSON.stringify({ ...base, buildOnly: true })); + expect(attempt().note).toContain('ended before its test phase'); + + // And the budget-floor shape keeps it too — build green, ok true, no + // scope, but the fresh path stamped that the test phase ran nothing. + // This is the shape whose real suite the false-completion answer + // certified as finished. + writeFileSync( + outPath, + JSON.stringify({ ...base, endedBeforeTests: true }), + ); + const floored = attempt(); + expect(floored.note).toContain('ended before its test phase'); + expect(floored.note).toContain('Re-run build-test without --resume'); + expect(floored.note).not.toContain('completed with no suite to run'); + }); + + it('a continuation that runs suites drops the stale endedBeforeTests stamp', () => { + // The stamp is a phase-level claim — "the test phase ENTERED and ran + // nothing" — and the merge must recompute it for the same staleness + // reason it recomputes `ok`, `note`, and `caveat`: a continuation that + // runs the starved suites falsifies it. Persisting the stale stamp + // beside a non-empty test[] would assert "nothing ran" for a run that + // ran — and the stamp exists so a reader never has to parse prose. + threePackages(); + const outPath = join(root, 'report.json'); + writeFileSync( + outPath, + JSON.stringify({ + toolchain: 'npm', + run: runId(), + affected: ['packages/core'], + buildSet: ['packages/core', 'packages/a', 'packages/b'], + widenedWith: [], + install: null, + build: [okResult('npm run build --workspace="packages/core"')], + test: [], + ok: true, + endedBeforeTests: true, + timedOut: [], + note: + 'the whole-call budget (16s) was spent with 3 suite(s) still ' + + 'to run — not run: packages/a, packages/b, packages/core', + testScope: { + workspaces: [], + notRun: ['packages/a', 'packages/b', 'packages/core'], + }, + }), + ); + + const rep = runBuildTest({ + plan: planPath, + worktree: root, + out: outPath, + timeout: 60, + install: true, + resume: true, + exec: okResult, + }); + + expect(rep.test).toHaveLength(3); + expect(rep.ok).toBe(true); + // Suites ran, so the stamp is now false and must not survive the merge. + expect(rep.endedBeforeTests).toBeUndefined(); + expect(JSON.stringify(rep)).not.toContain('"endedBeforeTests"'); + }); + it('keeps reporting work left when a retry is killed AGAIN by the budget', () => { // The ordinary outcome when an expensive suite is admitted late: it is // re-clamped rather than finished. Reporting that as a completed run @@ -3337,6 +3480,85 @@ describe('runBuildTest', () => { expect(rep.note).not.toContain('Every suite in scope has now run'); expect(rep.note).toContain('npm test --workspace="packages/a"'); + // Named ONCE. The unattempted retry rides both lists — its command is + // still-to-run, its stale clamped entry survives in the merged test[] + // — and two additive-looking clauses both naming it made one command + // read as two against the continuation budget, with the provisional + // clause claiming it was "killed on a deadline the budget shortened" + // this call, false for a retry never started. The still-to-run clause + // fully describes it; the provisional clause must not repeat it. + expect(rep.note).toContain('still to run'); + expect(rep.note).not.toContain('still provisional'); + }); + + it('the caveat names BOTH halves when work is unattempted AND re-clamped', () => { + // An else-if kept the provisional half out of the caveat whenever + // outstanding work existed — and the brief quotes the caveat as the + // live limitation, so a reader of it alone under-counted what is + // left. The two segments stay disjoint: a re-clamped suite killed + // again THIS call is provisional; an unreached workspace is still to + // run. + threePackages(); + const outPath = join(root, 'report.json'); + writeFileSync( + outPath, + JSON.stringify({ + toolchain: 'npm', + run: runId(), + affected: ['packages/core'], + buildSet: ['packages/core', 'packages/b'], + widenedWith: [], + install: null, + build: [okResult('npm run build --workspace="packages/core"')], + test: [ + { + command: 'npm test --workspace="packages/core"', + exitCode: null, + seconds: 100, + timedOut: true, + output: '', + deadlineMs: 100_000, + clamped: true, + }, + ], + ok: false, + timedOut: [], + note: 'one clamped, one unreached', + testScope: { + workspaces: ['packages/core'], + notRun: ['packages/b'], + }, + }), + ); + + // The retry is admitted with a budget-shortened deadline and killed + // again (exec burns ~6s and reports the timeout); what remains is + // below the attempt floor, so packages/b is never started. + const rep = runBuildTest({ + plan: planPath, + worktree: root, + out: outPath, + timeout: 60, + budget: 20, + install: true, + resume: true, + exec: (command) => { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 6000); + return { + command, + exitCode: null, + seconds: 6, + timedOut: true, + output: '', + }; + }, + }); + + const caveat = rep.testScope?.caveat ?? ''; + expect(caveat).toContain('still to run'); + expect(caveat).toContain('packages/b'); + expect(caveat).toContain('provisional'); + expect(caveat).toContain('npm test --workspace="packages/core"'); }); it("runs the AFFECTED pending suite first — the fresh path's invariant", () => { @@ -3713,6 +3935,59 @@ describe('runBuildTest', () => { { toolchain: 'npm', test: [] }, { toolchain: 'npm', test: [], build: [] }, { toolchain: 'npm', test: [], timedOut: [] }, + // The `test` clause needs its own UNMASKED witness: a fixture that + // also omitted `affected` and `ok` was refused by their clauses + // whatever happened to the `test` clause, so deleting + // `!commandsOk(shape.test)` kept every refusal green while a report + // truncated of only its `test` key cleared the mutated gate and + // died at `previous.test.filter` — the raw crash the gate exists to + // replace. Every other walked field is present and valid here, so + // the refusal rides on the `test` clause alone. + { + toolchain: 'npm', + affected: ['packages/core'], + ok: true, + build: [], + timedOut: [], + }, + // The two newest clauses need their own witnesses too: a corrupted + // stamp must refuse here, not steer the nothing-to-resume message + // off a non-boolean truthiness ('"buildOnly": "yes"' or + // '"endedBeforeTests": "yes"' fails `=== true` and would read as a + // completed zero-suite run). + { + toolchain: 'npm', + affected: ['packages/core'], + ok: true, + buildOnly: 'yes', + test: [], + build: [], + timedOut: [], + }, + { + toolchain: 'npm', + affected: ['packages/core'], + ok: true, + endedBeforeTests: 'yes', + test: [], + build: [], + timedOut: [], + }, + { + toolchain: 'npm', + affected: ['packages/core'], + ok: 'false', + test: [], + build: [], + timedOut: [], + }, + { + toolchain: 'npm', + affected: ['packages/core'], + test: [], + build: [], + timedOut: [], + }, ]) { writeFileSync(outPath, JSON.stringify(partial)); expect(() => diff --git a/packages/cli/src/commands/review/build-test.ts b/packages/cli/src/commands/review/build-test.ts index 80750d4e960..51ccc6fb96c 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/packages/cli/src/commands/review/build-test.ts @@ -128,6 +128,24 @@ export interface BuildTestReport { install: CommandResult | null; build: CommandResult[]; test: CommandResult[]; + /** + * True when the run was a deliberate `--build-only` probe. Structural, + * because `--resume`'s nothing-to-resume answer keys on it: a probe's + * report has no tests and no scope BY CHOICE, and without the stamp that + * shape is indistinguishable from a completed zero-suite run. + */ + buildOnly?: boolean; + /** + * True when the test phase was ENTERED and ran nothing — the whole-call + * budget fell below the attempt floor (or the unbuilt closure covered + * every suite) before the first test command started. Structural for the + * same reason `buildOnly` is: a single-root run in this state carries no + * `testScope` and keeps `ok: true` (the build passed), so without the + * stamp `--resume` read it as a COMPLETED zero-suite run — certifying an + * existing, unrun suite as finished and dropping the re-run advice that + * is the only path to ever running it. + */ + endedBeforeTests?: boolean; /** * What the test phase covered, so the review can state exactly what was and * was not run: `workspaces` lists exactly the suites the run executes, and @@ -528,6 +546,20 @@ function previousReport(out: string | undefined): BuildTestReport { const strings = (v: unknown): boolean => Array.isArray(v) && v.every((e) => typeof e === 'string' && e.length > 0); const affectedOk = strings((parsed as { affected?: unknown }).affected); + // Read by the nothing-to-resume split (deliberate probe vs completed + // zero-suite run) — validated like every other field the continuation + // reads, so a corrupted stamp refuses here instead of steering the + // message off a non-boolean truthiness. + const buildOnlyShape = (parsed as { buildOnly?: unknown }).buildOnly; + const buildOnlyOk = + buildOnlyShape === undefined || typeof buildOnlyShape === 'boolean'; + const endedBeforeShape = (parsed as { endedBeforeTests?: unknown }) + .endedBeforeTests; + const endedBeforeOk = + endedBeforeShape === undefined || typeof endedBeforeShape === 'boolean'; + // Same rule for `ok`, which the split reads beside it: required on the + // report, so undefined is refused too. + const okOk = typeof (parsed as { ok?: unknown }).ok === 'boolean'; const notBuiltShape = (parsed as { notBuilt?: unknown }).notBuilt; const notBuiltOk = notBuiltShape === undefined || strings(notBuiltShape); const scope = shape.testScope; @@ -547,6 +579,9 @@ function previousReport(out: string | undefined): BuildTestReport { !commandsOk(shape.build) || !strings(shape.timedOut) || !affectedOk || + !buildOnlyOk || + !endedBeforeOk || + !okOk || !notBuiltOk || !scopeOk || !runOk diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index e0cee5648f3..f236285d7fe 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -27,6 +27,7 @@ import { writeRoundCapStop, } from './lib/deadline.js'; import { getGhHost, setGhHost } from './lib/gh.js'; +import { BRIEFS } from './lib/agent-briefs.js'; import { LEDGER_MAX_ROUND, parseLedger } from './lib/ledger.js'; import { countInlineFindings } from './lib/inline-counts.js'; import { @@ -1223,8 +1224,9 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve // 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. + // longer starts with `reverse audit — `); only the canonical-entry + // splice dedups it, so this is the assertion that fails when the splice + // goes. const r3 = composeReview( base({ planPath: plan, @@ -1236,6 +1238,57 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve expect(r3.body.split('review time budget').length - 1).toBe(1); }); + it('a free-form disclosure that mentions the budget still reaches the body', () => { + // The splice dedups relays of the CANONICAL entry (verbatim or + // prefix-reshaped — both contain its full text); it must not retire a + // genuine line-coverage disclosure whose free-form reason merely mentions + // the phrase. A substring-of-phrase splice dropped exactly that entry + // from the posted body: the review capped and withheld the anchor for a + // security scope the rendered body never named — the module's contract + // is that a disclosed gap reaches the author. A PR plan, so a marker can + // actually be minted: without prNumber the anchor decision never runs + // (`!isPr` returns null first), and the withholding assertion below + // passed whatever the decision — vacuous. + const plan = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeBudgetStop( + plan, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 4, + ); + const freeForm = + 'security — the review time budget ended the round before the security relaunch returned evidence'; + // Built directly, not through base(): its default `planPath: + // coveredPlan()` rewrites the shared plan.json fixture, dropping this + // test's prNumber/fetchedSha before the override takes effect. + const r = composeReview({ + planPath: plan, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + unreviewedDimensions: [freeForm], + }); + // Rendered: the author sees the security scope by name… + expect(r.body).toContain(`Not reviewed: ${freeForm}.`); + // …beside the structural stop line, not instead of it… + expect(r.body).toContain( + 'reverse audit — stopped before round 4 by the review time budget', + ); + // …and the entry still counts against the anchor (not a relay, not + // depth-only): the minted marker's sha is withheld, exactly as when it + // was spliced — asserted on the parsed ledger, so a reclassification + // that LET the anchor ride fails here. + expect(parseLedger(r.body)?.round).toBe(1); + expect(parseLedger(r.body)?.sha).toBeUndefined(); + }); + 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 @@ -5967,7 +6020,7 @@ describe('the ledger marker reaches the POSTED body', () => { it("sees a debt the deterministic gates push in AFTER the caller's entries", () => { // `unreviewed` has three writers, at three different points: the caller's - // own entries, the budget-phrase splice that removes some of them, and the + // own entries, the canonical-relay splice that removes some of them, and the // script-lint / layer-audit gates that push machine-owed debts later. A // decision that reads any single snapshot misses one of them — an earlier // fix read too late and missed the splice, its replacement read too early @@ -5986,13 +6039,50 @@ describe('the ledger marker reaches the POSTED body', () => { ).toBe(true); }); - it('sees a lens gap the budget-phrase splice removes from the rendered list', () => { - // The splice keeps the body from saying one gap twice, and it matches on a - // PHRASE — so an entry that merely mentions the review time budget in its - // free-form reason leaves `unreviewedDimensions` before anything else reads - // it. Harmless while every cap withheld the anchor; not harmless once one - // cap does not, because the spliced entry is the line-coverage claim the - // anchor decision exists to respect. + it('stays tied to the briefs: every readsDiff flag round-trips the exemption', () => { + // The exempt heads are DERIVED from BRIEFS (`readsDiff: false` roles by + // their publicLabel), and this pins the tie in both directions: a label + // rename or a new non-diff role that broke the derivation would fail + // here loudly instead of silently re-opening the full-diff re-review + // loop (exemption lost) or widening the anchor past a whiffed lens + // (exemption over-granted). + expect( + Object.fromEntries( + Object.values(BRIEFS).map((b) => [ + b.publicLabel, + isNonDiffDimensionGap(`${b.publicLabel} — some reason`), + ]), + ), + ).toEqual( + Object.fromEntries( + Object.values(BRIEFS).map((b) => [b.publicLabel, !b.readsDiff]), + ), + ); + + // The prose spellings the replaced regex accepted — tight ampersand and + // separator-less — must not silently lose the exemption: refusing them + // withholds the anchor and re-opens the full-diff re-review cost on a + // spelling variant. + const variants = [ + 'build&test — the integration suite never ran', + 'the build&test check — skipped', + 'buildandtest — skipped', + 'build andtest — skipped', + ]; + expect( + Object.fromEntries(variants.map((v) => [v, isNonDiffDimensionGap(v)])), + ).toEqual(Object.fromEntries(variants.map((v) => [v, true]))); + // …while a squashed OTHER dimension stays out. + expect(isNonDiffDimensionGap('securityaudit — skipped')).toBe(false); + }); + + it('sees a lens gap that merely mentions the budget in its reason', () => { + // A free-form entry whose reason mentions the review time budget is a + // line-coverage claim, not a relay of the machine's stop entry — the + // splice (now matching the full canonical text) leaves it alone, and the + // anchor decision must read it as the whiffed lens it names. This pins + // the marker-less shape; the marker-present sibling lives beside the + // splice tests ('a free-form disclosure … still reaches the body'). const r = composeReview({ planPath: coveredPlan(['verify', 'reverse-audit'], { prNumber: 8255, @@ -6138,6 +6228,39 @@ describe('composeReview — convergence-posture deferrals (typed channel; disclo expect(parseLedger(r.body)?.findings).toEqual([]); }); + it('names the round AT the ledger cap — the clause and the marker agree', () => { + // `deferredRound` clamps exactly as the marker stamp does: `prevRound` + // can BE the cap (parseLedger accepts round == LEDGER_MAX_ROUND), and an + // unclamped +1 named round 10001 in the deferral clause beside a + // round-10000 marker — the two halves of one compose disagreeing about + // which round this is. The sibling test above pins the marker's + // round-trip at the cap; without THIS pin the Math.min mutation on the + // clause side ships green. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ v: 1, round: LEDGER_MAX_ROUND, findings: [] }), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + severityFloor: 'auto', + deferredSuggestions: [nit({ file: 'src/a.ts', line: 42 })], + }); + expect(r.body).toContain( + `convergence posture (round ${LEDGER_MAX_ROUND}, not a blocker)`, + ); + expect(r.body).not.toContain(`round ${LEDGER_MAX_ROUND + 1}`); + // The clause and the marker must name the SAME round — at the cap too. + expect(parseLedger(r.body)?.round).toBe(LEDGER_MAX_ROUND); + }); + it('renders the list on COMMENT and REQUEST_CHANGES alike — no event squeezes it out', () => { const comment = composeReview( base({ diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 67eb9914598..cf4408e6906 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -38,11 +38,8 @@ import { type Severity, type Source, } from './findings.js'; +import { BRIEFS } from './lib/agent-briefs.js'; import { - BUDGET_STOP_PHRASE, - BUDGET_STOP_PHRASE_ZH, - ROUND_CAP_PHRASE, - ROUND_CAP_PHRASE_ZH, budgetStopDisclosure, budgetStopEntry, budgetStopEntryZh, @@ -780,22 +777,70 @@ export interface ComposeReviewResult { dimensionGapsAreDepthOnly?: boolean; } +/** + * A dimension head reduced to its comparable core: lowercased, `&` read as + * `and`, hyphen/space runs collapsed to one hyphen, and the label dressing + * (`the …`, `… check`, `… verification`) stripped — so the orchestrator's + * prose variants (`build-and-test`, `build & test`, `the build-and-test + * check`) all reduce to the same core as the brief's `publicLabel`. + */ +function canonicalDimensionHead(s: string): string { + return ( + s + .toLowerCase() + // Spaced, not bare: a tight ampersand (`build&test`) must gain its + // separators BEFORE the hyphen collapse, or it canonicalises to + // `buildandtest` while the derived set holds `build-and-test` — the + // replaced regex accepted the tight form via `[-\s]?`. + .replace(/&/g, ' and ') + .replace(/[-\s]+/g, '-') + .replace(/^the-/, '') + .replace(/-(?:check|verification)$/, '') + ); +} + +/** The fully separator-less spelling — the loosest form the old regex took. */ +function squashedDimensionHead(s: string): string { + return canonicalDimensionHead(s).replace(/-/g, ''); +} + +/** + * The exempt heads, DERIVED from the briefs rather than restated: every role + * whose brief sets `readsDiff: false`, by its `publicLabel`. A hardcoded + * head list drifted from the machine source of truth it documented — a + * label rename (or a second non-diff role) would silently stop or fail to + * extend the exemption, and every budget-stopped round on a large repo + * would withhold the incremental anchor again: the full-diff re-review loop + * this exemption exists to kill, back by way of a string. + */ +const NON_DIFF_DIMENSION_HEADS: ReadonlySet = new Set( + Object.values(BRIEFS) + .filter((b) => !b.readsDiff) + .map((b) => canonicalDimensionHead(b.publicLabel)), +); +/** Squashed twins of the set above, for the separator-less prose spellings + * (`buildandtest`, `build andtest`) the replaced regex accepted via its + * optional separators — refusing them re-opened the anchor-withholding + * cost on a rare variant, in the safe but expensive direction. */ +const NON_DIFF_DIMENSION_HEADS_SQUASHED: ReadonlySet = new Set( + [...NON_DIFF_DIMENSION_HEADS].map((h) => h.replace(/-/g, '')), +); + /** * Does this `unreviewedDimensions` entry name a dimension that reads no diff? * * Entries are prose the orchestrator writes, in the shape the skill documents: * a dimension name, optionally followed by its own reason after an em-dash * (`build-and-test — the integration suite never ran`). Only the head is - * matched, and only against the ONE dimension whose brief sets - * `readsDiff: false`. + * matched, and only against dimensions whose brief sets `readsDiff: false` + * (English labels only — the entries are the orchestrator's English prose; + * `publicLabelZh` is a rendering concern). */ export function isNonDiffDimensionGap(entry: string): boolean { - const head = entry - .split(/[—–-]{1,2}\s/)[0] - .trim() - .toLowerCase(); - return /^(?:the\s+)?build[-\s]?(?:and|&)[-\s]?test(?:\s+check|\s+verification)?$/.test( - head, + const head = entry.split(/[—–-]{1,2}\s/)[0].trim(); + return ( + NON_DIFF_DIMENSION_HEADS.has(canonicalDimensionHead(head)) || + NON_DIFF_DIMENSION_HEADS_SQUASHED.has(squashedDimensionHead(head)) ); } @@ -1171,9 +1216,11 @@ export function composeReview( * The previous posted round's number, recovered from the side file * `pr-context` wrote — never from the model. 0 when the plan names no PR or * no previous round was recovered: this is round 1. Shared by the marker - * (which stamps `prevRound + 1`) and the deferred-suggestions clause (which - * names the round the posture engaged on), so the two cannot disagree about - * which round this is. + * (which stamps `Math.min(prevRound + 1, LEDGER_MAX_ROUND)`) and the + * deferred-suggestions clause (which names the round the posture engaged on, + * clamped identically), so the two cannot disagree about which round this + * is — at the cap included, where an unclamped `prevRound + 1` on either + * side would name round 10001 beside a round-10000 marker. */ function prevRoundFor(planPath: string | undefined): number { try { @@ -1562,27 +1609,31 @@ function composeReviewBody( // (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. + // coverage entry — the orchestrator's compliant relay is byte-identical + // canonical text, so the canonical-entry splice 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 + // the canonical-entry 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. /** - * Entries the budget-phrase splice below removes from the rendered list. + * Entries the canonical-relay splice below removes from the rendered list. * * The splice exists so the body does not say the same gap twice, and it - * matches on a PHRASE — so an entry that merely mentions the review time + * matches entries CONTAINING a full canonical stop entry — verbatim relays + * and prefix-reshaped ones alike ("step 5 — " ahead of the subject), which + * the coverage prefix filter cannot see. An earlier match on the bare stop + * PHRASE spliced more: an entry that merely mentioned the review time * budget in its free-form reason ("security — the review time budget ended - * the round before the security relaunch returned evidence") is spliced out - * too. Harmless while every cap withheld the anchor; not harmless now that - * one cap does not, because the spliced entry is exactly the line-coverage - * claim the anchor decision must see. Kept here so the decision can read the - * list AS DISCLOSED while the body renders the spliced one. + * the round before the security relaunch returned evidence") was dropped + * from the posted body, though it is exactly the line-coverage claim both + * the author and the anchor decision must see. Such entries now stay in + * `unreviewed` — rendered and capping. The spliced relays are kept here so + * the decision can read the list AS DISCLOSED while the body renders the + * structural stop line once. * * Collected rather than snapshotted: the deterministic gates push their own * machine-owed debts into `unreviewed` AFTER this point, and a snapshot @@ -1619,20 +1670,29 @@ function composeReviewBody( budgetStopEntryZh(stop.round ?? undefined), ]); // A round-cap stop and a time-budget stop both cap the verdict, but - // read differently and dedup against a different relayed phrase. The - // marker's `cause` picks which; an absent cause is a time stop, for - // markers written before the cause field existed. + // read differently. The marker's `cause` picks which pair of canonical + // entries exists; an absent cause is a time stop, for markers written + // before the cause field existed. const isRoundCap = stop.cause === 'round-cap'; - // BOTH languages: the exemption admits the Chinese pair as a compliant - // relay, so the splice must retire it too — an English-only phrase let - // a relayed `budgetStopEntryZh` survive into the whiffed-dimension - // rendering beside the structural stop line, the same gap said twice - // with the wrong cause on one of them. - const phrases = isRoundCap - ? [ROUND_CAP_PHRASE, ROUND_CAP_PHRASE_ZH] - : [BUDGET_STOP_PHRASE, BUDGET_STOP_PHRASE_ZH]; + // Spliced on the FULL canonical entry text (both languages: the + // exemption admits the Chinese pair as a compliant relay, so the + // splice must retire it too, or the same gap renders twice beside the + // structural stop line) — as a substring, because an orchestrator + // relay arrives verbatim OR reshaped with a prefix ("step 5 — " ahead + // of the subject), and the coverage prefix filter cannot see the + // reshaped one. What the predicate must NOT be is the bare stop + // PHRASE: that retired more than the relays — a genuine line-coverage + // disclosure that merely mentions the budget in its free-form reason + // ("security — the review time budget ended the round before the + // security relaunch returned evidence") was dropped from the posted + // body, and the module's contract is that a disclosed gap reaches the + // author. Such entries now stay in `unreviewed` — rendered AND + // capping. (The anchor DECISION below stays exact-text: a reshaped + // relay spliced here still withholds, over-withholding being the safe + // direction.) + const entries = [...canonicalStopEntries]; for (let i = unreviewed.length - 1; i >= 0; i--) { - if (phrases.some((ph) => unreviewed[i].includes(ph))) { + if (entries.some((c) => unreviewed[i].includes(c))) { splicedForBudgetPhrase.push(unreviewed[i]); unreviewed.splice(i, 1); } @@ -2204,7 +2264,8 @@ function composeReviewBody( // both before this line (the orchestrator's own entries) and after the // snapshot an earlier fix took (the script-lint and layer-audit gates, whose // debts are machine-owed line-coverage claims). Reading it here plus the - // entries the phrase splice removed is the only list that sees every writer. + // entries the canonical-entry splice removed is the only list that sees + // every writer. // // The stop's own relayed entry classifies as DEPTH, and only against the // marker. A budget/round-cap stop truncates how many audit PASSES ran over @@ -2219,9 +2280,10 @@ function composeReviewBody( // head-plus-phrase, and that shape also covers a genuine line-coverage claim // whose whiffed scope IS the reverse audit — `reverse audit — the review // time budget ended the round before the chunk-2 relaunch returned - // evidence` — which the phrase splice then also removes from the rendered - // body, so the anchor rode past a whiffed audit while the posted review - // showed only the benign disclosure. The machinery mints its entries from + // evidence` — which the then-substring splice also removed from the + // rendered body, so the anchor rode past a whiffed audit while the posted + // review showed only the benign disclosure (both predicates are exact + // now). The machinery mints its entries from // one generator pair, the stderr instruction relays them verbatim, and only // that text is exempt: marker-anchored (no marker, no exemption) AND // text-anchored (an edited or paraphrased entry withholds — over-withholding @@ -3071,7 +3133,12 @@ function composeReviewBody( .map(renderDeferredEntry) .map(boundDeferredLine); const deferredMore = deferredSuggestions.length - deferredShown.length; - const deferredRound = deferredSuggestions.length ? prevRound + 1 : 0; + // Clamped exactly as the marker stamp is: `prevRound` can BE the cap + // (parseLedger accepts round == LEDGER_MAX_ROUND), and an unclamped +1 + // here named a past-cap round beside a round-at-cap marker. + const deferredRound = deferredSuggestions.length + ? Math.min(prevRound + 1, LEDGER_MAX_ROUND) + : 0; // The unlicensed-deferral disclosure precedes the list it disclaims: the // findings stay visible, but nothing may read the paragraph below as a // sanctioned deferral when the posture never licensed one. diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 329d38edbbf..16bd58247b9 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -535,7 +535,7 @@ You are undirected on purpose. Do not restrict yourself to the list.`, Read the JSON it prints: - \`toolchain: "npm"\` → use its \`build[]\` / \`test[]\` results. A failure in a file **the diff changed** is a **Critical** (\`Source: [build]\` or \`[test]\`); a failure in a file it did **not** touch is pre-existing — say so, do not file it against this PR. A non-empty \`timedOut\`, or a failed \`install\`, is environment/infrastructure — informational, never a Critical. On \`ok: true\`, name the workspaces built and the commands run; a return that names no command is a whiff. Report the TEST coverage from \`testScope\`, never from assumption. \`testScope.workspaces\` lists exactly the suites that ran — say "tests scoped to — the changed workspaces and their declared dependents that define a test script". \`testScope.notRun\`, when present, names suites the whole-call budget stopped before they ran — say they did not run, never fold them into the coverage. When \`testScope.caveat\` is present, the scope may be incomplete — quote the caveat and say exactly that. A green run is a claim about those suites only — do not phrase it as the whole suite passing. -- **A suite left unrun is not a suite that passed — continue the run.** \`testScope.notRun\` names suites the whole-call budget could not reach, and a \`test[]\` entry with \`"clamped": true\` is a suite the budget started too late and killed (its deadline was shortened, so its timeout says nothing about the suite). A third shape carries no field at all — a single-package repo whose budget ran out before its one suite has an empty \`test[]\` and no \`testScope\`, and only the \`note\` says so; read it before calling the dimension finished. That third shape cannot be continued — a continuation has no recorded scope to read, and answers "ended before its test phase" without running anything — so report the dimension UNFINISHED and do not spend a continuation on it. The first two mean the dimension is unfinished AND continuable: re-run the SAME \`build-test\` command with \`--resume\` — it skips install and build, runs only what is left, and merges into the same report file. The ${SHELL_TOOL_MAX_TIMEOUT_MS / 1000}-second ceiling is per CALL, so this is the only way a repo whose suites do not fit one call ever finishes them (measured on this repo: \`packages/cli\` alone needs 401s, and install + builds + \`packages/core\` had already spent 285s). Keep resuming while work is left, up to ${MAX_RESUME_CALLS} continuations; then report what the run has, with \`notRun\` disclosed. +- **A suite left unrun is not a suite that passed — continue the run.** \`testScope.notRun\` names suites the whole-call budget could not reach, and a \`test[]\` entry with \`"clamped": true\` is a suite the budget started too late and killed (its deadline was shortened, so its timeout says nothing about the suite). A third shape ends before any suite — a single-package repo whose budget ran out before its one suite has an empty \`test[]\`, no \`testScope\`, and \`"endedBeforeTests": true\` (the report's own stamp), with the \`note\` naming the unrun suite; read them before calling the dimension finished. That third shape cannot be continued — a continuation has no recorded scope to read, and a \`--resume\` on it answers "ended before its test phase" and points at a fresh run — so report the dimension UNFINISHED and do not spend a continuation on it. The first two mean the dimension is unfinished AND continuable: re-run the SAME \`build-test\` command with \`--resume\` — it skips install and build, runs only what is left, and merges into the same report file. The ${SHELL_TOOL_MAX_TIMEOUT_MS / 1000}-second ceiling is per CALL, so this is the only way a repo whose suites do not fit one call ever finishes them (measured on this repo: \`packages/cli\` alone needs 401s, and install + builds + \`packages/core\` had already spent 285s). Keep resuming while work is left, up to ${MAX_RESUME_CALLS} continuations; then report what the run has, with \`notRun\` disclosed. - **When any \`test[]\` command failed (exit non-zero, not a timeout), MEASURE which failures are the PR's before ruling by path.** The path rule above misclassifies in both directions — an environment-flaky test in a touched file gets filed as a Critical it did not cause, and a PR that breaks a test in an UNTOUCHED file gets waved through as pre-existing. The measurement is two commands: \`qwen review base-tree --plan --worktree --out /qwen-review-pr--base-tree.json\` (builds the merge base beside the worktree). **Read \`available\` before using \`path\`** — a tree that was created but did NOT build populates \`path\` too, and a base that failed to build says nothing whatsoever about the PR, so measuring against it turns an infrastructure failure into a list of Criticals. \`available: false\` (local/lightweight review, no merge base, a base that would not compile) means the path rule stands — say so and stop here, and \`qwen review test-delta --report --baseline --pr-worktree --out /qwen-review-pr--test-delta.json\`. Read its verdict: a file in \`netNew\` fails on the PR side only — **that is the Critical**, whatever file the diff touches; a file in \`shared\` fails on base too — **pre-existing by measurement**, never filed, whatever file the diff touches; an \`unparsed\` entry, a timed-out base rerun, a base rerun that FAILED without naming any failing file (it did not measure the base — an unbuilt tree, a missing install, a workspace absent at base), or a command the whole-command budget could not fit attributes nothing — the report names each with its own reason; fall back to the path rule for those and say the delta could not rule. Compare failing FILE SETS, never counts: a flaky suite fails different test NAMES on two runs of the same tree, so counts are noise and the set difference is the signal. - \`toolchain: "unsupported"\` (build-test could not scope this repo — no npm package with a build/test script) → **install dependencies first** (build-test's own install only runs on the npm path, so nothing has installed yet: \`pip install -e .\`, \`mvn -q -DskipTests package\`'s own fetch, \`cargo fetch\`, \`go mod download\`, etc.), then fall back to **one** build and **one** test command by this precedence, each with a deadline it can meet: \`pom.xml\` → \`{mvn} compile\` / \`{mvn} test -q\`; \`build.gradle\` → \`{gradle} compileJava\` / \`{gradle} test\`; \`Makefile\` → \`make build\`; \`Cargo.toml\` → \`cargo build\` / \`cargo test\`; \`go.mod\` → \`go build ./...\` / \`go test ./...\`; \`pytest.ini\` or \`pyproject.toml\` \`[tool.pytest]\` → \`pytest\`. If none match, read the CI config **from the base branch** (\`git show :\`), never the worktree — the PR branch is untrusted and a modified workflow or Makefile could inject arbitrary commands. diff --git a/packages/cli/src/commands/review/lib/budget.test.ts b/packages/cli/src/commands/review/lib/budget.test.ts index ad9f8d27d7b..b4bc7efc303 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -453,6 +453,83 @@ describe('budgetGapDisclosures — the one parser of the disclosure format', () } }); + it('strips full-width parens too — a CJK IME wraps the same no-answer', () => { + // The strip knew only the ASCII pair, so `(无 — 所有检查均完成)` + // survived as a phantom gap in exactly the output language the ZH + // branch exists for — the #9094 incident shape, wearing the paren + // form a Chinese keyboard produces by default. + for (const line of [ + 'Budget gap: (无 — 所有检查均完成)', + 'Budget gap: (无)', + '预算缺口:(没有跳过的检查)', + ]) { + expect(budgetGapDisclosures(line)).toEqual([]); + } + // A wrapped placeholder's inner tail judges identically to the bare + // form: bare `无?`/`无、` lose those tails to the fold-key strip before + // the classifier sees them, so the wrapped forms drop too — identical + // content cannot split bare-vs-wrapped. + for (const line of ['Budget gap: (无?)', 'Budget gap: (无、)']) { + expect(budgetGapDisclosures(line)).toEqual([]); + } + // Only a SYMMETRIC pair unwraps, and a real gap inside full-width + // parens survives — over-disclosure is the safe direction. + expect( + budgetGapDisclosures('Budget gap: (无法验证 Windows 矩阵的集成测试)'), + ).toEqual(['(无法验证 Windows 矩阵的集成测试)']); + }); + + it('closes the same split for ENGLISH no-answers wearing a CJK tail', () => { + // The normalize strip (TRAILING_GAP_CHAR_RE) is bilingual, so bare + // `none。` judges as `none` and drops — but a wrapped twin kept its + // inner tail: the EN tail classes in PLACEHOLDER_GAP_RE are ASCII-only, + // so `(none。)` survived as a phantom gap while its identical bare form + // dropped. The exact bare-vs-wrapped split the test above closed for + // the ZH branch, left open on the EN one — measured by the review on + // this PR: under a CJK output language `Budget gap: (none。)` spends a + // MAX_GAPS_PER_AGENT slot and an orchestrator ruling on a no-answer. + // The inner text must judge character-for-character as its bare twin, + // so the wrapped forms drop too — in both paren shapes. + for (const line of [ + 'Budget gap: none。', + 'Budget gap: (none。)', + 'Budget gap: (none。)', + 'Budget gap: none - stayed under budget。', + 'Budget gap: (none - stayed under budget。)', + 'Budget gap: (N/A - stayed under budget。)', + 'Budget gap: (none — all checks completed。)', + ]) { + expect(budgetGapDisclosures(line)).toEqual([]); + } + // A REAL gap keeps its CJK tail in both forms — the strip equalizes + // judgment, never swallows. + expect(budgetGapDisclosures('Budget gap: (渗透测试未进行。)')).toEqual([ + '(渗透测试未进行。)', + ]); + }); + + it('folds a Chinese gap restated with and without a trailing full stop', () => { + // The fold key promises one disclosure per gap; a key that stripped only + // ASCII trailing punctuation kept `渗透测试未进行。` and `渗透测试未进行` + // as two, double-spending MAX_GAPS_PER_AGENT slots. + const text = [ + 'Budget gap: 渗透测试未进行。', + 'some other line', + 'Budget gap: 渗透测试未进行', + ].join('\n'); + expect(budgetGapDisclosures(text)).toEqual(['渗透测试未进行。']); + + // Both CJK full stops — 。(U+3002) and .(U+FF0E) — are ZH_TAIL + // characters, and the fold key must strip both: covering only one left + // the double-spend open for the other. + const ff0e = [ + 'Budget gap: 渗透测试未进行.', + 'some other line', + 'Budget gap: 渗透测试未进行', + ].join('\n'); + expect(budgetGapDisclosures(ff0e)).toEqual(['渗透测试未进行.']); + }); + it('keeps a REAL Chinese gap — 无法 is a prefix of the token, not the token', () => { // Chinese has no word boundary, so a token is only a token when // punctuation, whitespace or end-of-text follows it. `无法验证…` diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index 371b6695734..43dd674992a 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -742,7 +742,10 @@ const ZH_DONE = '(?:均|都|皆)?(?:已|均已|都已)?(?:完成|完毕|结束| const ZH_BUDGET_TAIL = '(?:[,,、]?\\s*(?:未触及|未达到|未超出|未用尽|没有触及|在|不超过)' + '(?:工具)?(?:调用)?预算(?:上限|限制|范围内)?)?'; -const ZH_TAIL = '[。..!!…,,;;::\\s]*$'; +// ? (U+FF1F) and 、 (U+3001) mirror the fold key's trailing strip: +// a wrapped placeholder's inner text must judge identically to its +// bare form, which loses those tails before the classifier sees it. +const ZH_TAIL = '[。..!!…,,;;::?、\\s]*$'; // NO whitespace between the pieces. Four optional groups chained across `\s*` // is the overlapping-quantifier shape this module's header bans and its // 'stays linear on pathological inputs' test exists for — and Chinese does not @@ -818,7 +821,15 @@ function stripWrappers(s: string): string { return out; } -const TRAILING_GAP_CHAR_RE = /[.!…,;:\s]/; +// Both scripts' trailing punctuation: the fold key promises that a gap +// restated with and without a trailing stop discloses once, and a key that +// stripped only the ASCII set kept `渗透测试未进行。` and `渗透测试未进行` as +// two gaps — double-spending MAX_GAPS_PER_AGENT slots in exactly the +// output language the ZH branch exists for. The class mirrors ZH_TAIL's +// full stop set — including the fullwidth full stop .(U+FF0E) beside 。 +// (U+3002): the classifier treats both as trailing, and a fold key that +// dropped only one of them kept the double-spend open for the other. +const TRAILING_GAP_CHAR_RE = /[.!…,;:\s。,;:!?、.]/; /** Trailing punctuation/whitespace strip for the normalize and fold keys. */ function stripTrailingGapChars(s: string): string { @@ -882,17 +893,30 @@ export function budgetGapDisclosures(finalText: string): string[] { const normalized = stripTrailingGapChars(raw).trim(); // Judged on the paren-stripped text, bare and wrapped alike, by the // one strict classifier — its doc names why the shapes are narrow. - const unparenthesized = - normalized.startsWith('(') && normalized.endsWith(')') + // Both paren shapes: under `outputLanguage: 中文` the full-width pair + // (U+FF08/U+FF09)is what an IME produces, and a strip that knew only + // the ASCII pair let `(无 — 所有检查均完成)` through as a phantom gap — + // the #9094 incident shape this classifier exists to kill, surviving + // in exactly the output language the ZH branch was added for. Only a + // SYMMETRIC pair is unwrapped, so a mixed or unbalanced wrap stays + // whole and errs toward disclosure. The inner text is trailing-stripped + // exactly as the bare form already was (`normalized` above): the EN tail + // classes are ASCII-only, so a wrapped `none。` kept its CJK tail and + // survived as a phantom while its identical bare twin dropped — one + // normalize, one judgment, bare and wrapped alike. + const unparenthesized = stripTrailingGapChars( + (normalized.startsWith('(') && normalized.endsWith(')')) || + (normalized.startsWith('(') && normalized.endsWith(')')) ? normalized.slice(1, -1).trim() - : normalized; + : normalized, + ); if (normalized.length === 0 || PLACEHOLDER_GAP_RE.test(unparenthesized)) { continue; } - // Folded on the paren-stripped text with its OWN trailing punctuation - // gone, so one gap restated with and without parentheses — `(auth - // flow untested.)` and `auth flow untested` — discloses once. - const key = stripTrailingGapChars(unparenthesized).toLowerCase(); + // Folded on the same normalized text the classifier judged, so one gap + // restated with and without parentheses — `(auth flow untested.)` and + // `auth flow untested` — discloses once. + const key = unparenthesized.toLowerCase(); if (seen.has(key)) continue; seen.add(key); gaps.push(truncateGap(raw)); diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index 9423e5b8595..28311a9c126 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -588,16 +588,24 @@ export interface BudgetStop { } /** - * 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. + * The phrase the budget-stop entry is spelled with — interpolated into the + * disclosure below, so a reword changes every rendering in one place. + * + * NOT a dedup key: `compose-review` once spliced relayed copies by this + * substring, and the phrase alone also matched genuine free-form + * line-coverage disclosures that merely mention the budget — those were + * silently dropped from the posted body. The splice now keys on the FULL + * canonical entry text (`budgetStopEntry`/`budgetStopEntryZh`), so a + * phrase-only relay is no longer deduped: it renders beside the structural + * stop line, which is the honest outcome for text the machinery did not + * mint. */ export const BUDGET_STOP_PHRASE = 'review time budget'; -/** The Chinese pair — the marker's zh entries carry it, and the body-side - * dedup must read BOTH languages: a relayed Chinese stop entry that only the - * English phrase was checked against survived the splice and was rendered - * under the whiffed-agent cause beside the structural stop line. */ +/** The Chinese pair, spelled into `budgetStopEntryZh`. Same non-dedup-key + * status as the English phrase above: the splice reads the full canonical + * entry in BOTH languages (a relayed Chinese entry checked against only + * the English text once survived and double-rendered), never the bare + * phrase. */ export const BUDGET_STOP_PHRASE_ZH = '评审时间预算'; /** @@ -617,7 +625,7 @@ export function budgetStopDisclosure(round: number | undefined): { subject: 'reverse audit', reason: `stopped before ${which} by the ${BUDGET_STOP_PHRASE}`, subjectZh: '反向审计', - reasonZh: `评审时间预算不足,未能开始${whichZh}`, + reasonZh: `${BUDGET_STOP_PHRASE_ZH}不足,未能开始${whichZh}`, }; } @@ -634,12 +642,12 @@ export function budgetStopEntryZh(round: number | undefined): string { } /** - * The phrase identifying a ROUND-CAP disclosure wherever it is relayed — - * the cap analogue of `BUDGET_STOP_PHRASE`, so `compose-review` dedups the - * orchestrator's relayed copy against the marker's by shared text. + * The phrase the round-cap entry is spelled with — the cap analogue of + * `BUDGET_STOP_PHRASE`, and like it NOT a dedup key: `compose-review` + * splices relays by the full canonical entry text, never this substring. */ export const ROUND_CAP_PHRASE = 'reverse-audit round cap'; -/** The Chinese pair, for the same bilingual-dedup reason as the budget one. */ +/** The Chinese pair, spelled into the zh entry — same non-dedup-key status. */ export const ROUND_CAP_PHRASE_ZH = '反审轮数上限'; /** @@ -657,7 +665,7 @@ export function roundCapStopDisclosure(cap: number): { subject: 'reverse audit', reason: `did not converge within the ${ROUND_CAP_PHRASE} of ${cap}`, subjectZh: '反向审计', - reasonZh: `在 ${cap} 轮的反审轮数上限内未收敛`, + reasonZh: `在 ${cap} 轮的${ROUND_CAP_PHRASE_ZH}内未收敛`, }; } diff --git a/packages/cli/src/commands/review/lib/npm-toolchain.ts b/packages/cli/src/commands/review/lib/npm-toolchain.ts index 7a30ed193ad..3f743310ef4 100644 --- a/packages/cli/src/commands/review/lib/npm-toolchain.ts +++ b/packages/cli/src/commands/review/lib/npm-toolchain.ts @@ -69,8 +69,10 @@ function testCommand(dir: string): string { * strings verbatim under `shell: true`, and the run-identity check pins a * report to this run's TREE, not to this program's authorship — a report * edited in place keeps its identity. Anything outside the emitter's own - * grammar is therefore refused before it can be re-run, the same policy - * `test-delta` already applies to report-derived commands it re-executes. + * grammar is therefore refused before it can be re-run — and `test-delta` + * imports this same predicate for the report commands it re-runs: one + * grammar, beside the emitter, for every site that hands a stored command + * to a shell, so a grammar change cannot silently diverge the two gates. * The character class covers every workspace dir this repo shape produces; * a dir exotic enough to fall outside it costs that report its resume (a * named refusal, pointing at a fresh run), never a verbatim re-execution. @@ -210,14 +212,38 @@ function resumeNpmToolchain( // scope it would run is computed by the phase that never happened. Saying // it reached every suite would be this PR's own Chinese-placeholder defect // in English — prose asserting the opposite of the evidence beside it. + // + // But the SAME shape also belongs to a run that finished: a single-root + // package with no test script completes with `test: []` and no scope, + // and calling that "ended before its test phase" was self-contradictory + // beside its own carried note ("defines no test script, so no tests + // ran") — and its re-run advice re-derived the same zero-suite answer at + // the price of a full fresh install+build. The split is structural: + // `buildOnly` marks the deliberate probe, `ok: false` marks the runs + // that died early (failed install, disk gate, budget in the build); + // an `ok: true` non-probe with no scope and no results ran to + // completion and simply had nothing to run. The third signal, + // `endedBeforeTests`, covers the shape the other two cannot: a + // single-root run whose budget fell below the attempt floor before the + // first suite — build green (`ok: true`), no probe, no scope — where + // only the fresh path's own stamp knows the phase ran nothing it was + // supposed to run. const neverTested = previous.test.length === 0 && !previous.testScope; + const endedEarly = + neverTested && + (previous.buildOnly === true || + previous.ok === false || + previous.endedBeforeTests === true); return withNote( - neverTested + endedEarly ? 'Nothing to resume: the run being continued ended before its test ' + 'phase, so it left no scope to continue — no suite ran. Re-run ' + 'build-test without --resume.' - : 'Nothing to resume: the run being continued reached every suite in ' + - 'scope.', + : neverTested + ? 'Nothing to resume: the run being continued completed with no ' + + 'suite to run.' + : 'Nothing to resume: the run being continued reached every suite ' + + 'in scope.', ); } @@ -315,10 +341,24 @@ function resumeNpmToolchain( `command(s), ${outstanding.length} still to run: ` + outstanding.join(', '), ); - } else if (stillClamped.length > 0) { + } + // NOT an else: a resume can leave BOTH unattempted work and suites + // killed again on a shortened deadline, and an else-if kept the + // provisional half out of the caveat exactly then — the brief quotes + // the caveat as the live limitation, so a reader of it alone + // under-counted the work left. An unattempted retry is already named + // by the still-to-run segment above; naming its stale clamped entry + // here too would claim it was "killed" on a deadline this call never + // gave it, so the provisional segment lists only commands the + // still-to-run segment does not. + const provisionalOnly = stillClamped.filter( + (c) => !outstanding.includes(c), + ); + if (provisionalOnly.length > 0) { liveSegments.push( - `a --resume call left ${stillClamped.length} command(s) provisional ` + - `(killed on a budget-shortened deadline): ${stillClamped.join(', ')}`, + `a --resume call left ${provisionalOnly.length} command(s) ` + + `provisional (killed on a budget-shortened deadline): ` + + `${provisionalOnly.join(', ')}`, ); } const caveat = liveSegments.join('; '); @@ -343,6 +383,12 @@ function resumeNpmToolchain( // Recomputed, not inherited: the previous `false` may have been nothing // but the clamped timeout this call just replaced with a pass. ok: previous.build.every(succeeded) && mergedTest.every(succeeded), + // Recomputed for the same staleness reason: the stamp claims the test + // phase ENTERED and ran nothing, which any suite in `mergedTest` + // falsifies. A continuation that ran none keeps it — the claim still + // holds then. JSON.stringify drops the undefined key. + endedBeforeTests: + mergedTest.length === 0 ? previous.endedBeforeTests : undefined, timedOut, // REPLACED, not appended. The note being continued says things that were // true when it was written and are not now — "the whole-call budget was @@ -415,11 +461,20 @@ function resumedNote( `run — not run: ${stillPending.join(', ')}. Resume again to reach them.`, ); } - if (stillClamped.length > 0) { + // An unattempted retry rides BOTH lists — its command is still-to-run, and + // its stale clamped entry survives in the merged test[] — and two + // additive-looking clauses both naming it made one command read as two or + // three against the MAX_RESUME_CALLS budget. Worse, this clause claims the + // command was "killed on a deadline the budget shortened", which is false + // for a retry this call never started. So the provisional clause names + // only commands the still-to-run clause does not; a command in both is + // fully described by "not run". + const provisionalOnly = stillClamped.filter((c) => !stillPending.includes(c)); + if (provisionalOnly.length > 0) { parts.push( - `${stillClamped.length} command(s) are still provisional — killed on a ` + - `deadline the budget shortened, not on their own: ` + - `${stillClamped.join(', ')}. Resume again to give them a full one.`, + `${provisionalOnly.length} command(s) are still provisional — killed ` + + `on a deadline the budget shortened, not on their own: ` + + `${provisionalOnly.join(', ')}. Resume again to give them a full one.`, ); } if (stillPending.length === 0 && stillClamped.length === 0) { @@ -577,6 +632,10 @@ function runNpmToolchain(args: ToolchainRunArgs): BuildTestReport { build: [], test: [], ...(testScope ? { testScope } : {}), + // The probe stamp rides EVERY producer path: this return already + // branches on args.buildOnly for its note, and a probe report without + // the stamp lost its probe answer on --resume. + ...(args.buildOnly ? { buildOnly: true } : {}), ok: true, timedOut: [], note: args.buildOnly @@ -633,6 +692,11 @@ function runNpmToolchain(args: ToolchainRunArgs): BuildTestReport { ok: true, timedOut: [], note: '', + // Stamped structurally, not narrated: `--resume` on a report with no + // tests and no scope needs to tell a deliberate probe apart from a + // completed zero-suite run, and the note is prose an agent must never + // have to parse. + ...(args.buildOnly ? { buildOnly: true } : {}), }; // The install. It lives here, not in the orchestrator, because nothing before @@ -1014,6 +1078,16 @@ function runNpmToolchain(args: ToolchainRunArgs): BuildTestReport { if (r.exitCode !== 0) results.ok = false; } + // The test phase ran NOTHING and left work behind: stamp it. A single-root + // run in this state writes no testScope and keeps ok: true (the build + // passed), so the stamp is the only structural evidence separating "ended + // before its test phase" from "completed with no suite to run" — the + // resume split reads it, and without it a --resume certified the one + // existing suite as finished (multi-root runs carry the same fact in + // testScope.notRun; the stamp is simply the phase-level truth either way). + if (results.test.length === 0 && notRun.length > 0) { + results.endedBeforeTests = true; + } // A budget stop is STRUCTURAL, not just prose: `testScope.workspaces` is // documented (and quoted by the agent's brief) as exactly the suites that // ran, so the trimmed suites leave it, and `notRun` names them. Sorted, so diff --git a/packages/cli/src/commands/review/pr-context-persist.test.ts b/packages/cli/src/commands/review/pr-context-persist.test.ts index 90841f046e5..59c32db0ef5 100644 --- a/packages/cli/src/commands/review/pr-context-persist.test.ts +++ b/packages/cli/src/commands/review/pr-context-persist.test.ts @@ -45,8 +45,7 @@ describe('persistRecoveredLedger', () => { persistRecoveredLedger( side, { ledger, commitId: 'a'.repeat(40), reviewId: 42 }, - true, - true, + { noOwnReview: true, identityKnown: true }, ); const written = JSON.parse(readFileSync(side, 'utf8')); expect(written).toEqual({ @@ -73,7 +72,10 @@ describe('persistRecoveredLedger', () => { side, JSON.stringify({ ...ledger, commitId: 'b'.repeat(40), reviewId: 7 }), ); - persistRecoveredLedger(side, null, false, true); + persistRecoveredLedger(side, null, { + noOwnReview: false, + identityKnown: true, + }); const written = JSON.parse(readFileSync(side, 'utf8')); expect(written).toEqual(ledger); expect(written.round).toBe(3); @@ -95,7 +97,10 @@ describe('persistRecoveredLedger', () => { side, JSON.stringify({ ...ledger, commitId: 'b'.repeat(40), reviewId: 7 }), ); - persistRecoveredLedger(side, null, true, true); + persistRecoveredLedger(side, null, { + noOwnReview: true, + identityKnown: true, + }); expect(existsSync(side)).toBe(false); } finally { rmSync(dir, { recursive: true, force: true }); @@ -119,24 +124,21 @@ describe('persistRecoveredLedger', () => { commitId: 'a'.repeat(40), reviewId: 20, }, - false, - true, + { noOwnReview: false, identityKnown: true }, ); expect(JSON.parse(readFileSync(side, 'utf8'))).toEqual(newer); // Same round, older reviewId: also kept. persistRecoveredLedger( side, { ledger: { ...ledger, round: 7 }, commitId: null, reviewId: 60 }, - false, - true, + { noOwnReview: false, identityKnown: true }, ); expect(JSON.parse(readFileSync(side, 'utf8'))).toEqual(newer); // A genuinely newer recovery still writes. persistRecoveredLedger( side, { ledger: { ...ledger, round: 8 }, commitId: null, reviewId: 80 }, - false, - true, + { noOwnReview: false, identityKnown: true }, ); expect(JSON.parse(readFileSync(side, 'utf8')).round).toBe(8); } finally { @@ -148,7 +150,10 @@ describe('persistRecoveredLedger', () => { const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); const side = join(dir, 'side.json'); try { - persistRecoveredLedger(side, null, false, true); + persistRecoveredLedger(side, null, { + noOwnReview: false, + identityKnown: true, + }); expect(existsSync(side)).toBe(false); // No debris of any name — the temp is per-process (`..tmp`), so // asserting on the directory listing is the only check independent of @@ -182,8 +187,7 @@ describe('persistRecoveredLedger', () => { commitId: 'c'.repeat(40), reviewId: 101, }, - false, - false, + { noOwnReview: false, identityKnown: false }, ); expect(JSON.parse(readFileSync(side, 'utf8'))).toEqual(own); } finally { @@ -197,9 +201,9 @@ describe('persistRecoveredLedger', () => { // their ids), while adopting the findings re-opens the swap. The anchor // and the age reference go — an anonymous round cannot be re-vouched, // and a sha superseded by rounds this account never certified must not - // scope the next review. `noOwnReview` is passed TRUE here on purpose: - // it is ignored on the recovered path, so a positional swap of the two - // booleans would delete the file and fail both assertions. + // scope the next review. `noOwnReview` is TRUE here on purpose: the + // recovered path ignores it, which is exactly what this fixture pins — + // the deletion licence must have no reach into a recovered write. const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); const side = join(dir, 'side.json'); try { @@ -224,8 +228,7 @@ describe('persistRecoveredLedger', () => { commitId: 'c'.repeat(40), reviewId: 200, }, - true, - false, + { noOwnReview: true, identityKnown: false }, ); const written = JSON.parse(readFileSync(side, 'utf8')); expect(written).toEqual({ @@ -251,8 +254,7 @@ describe('persistRecoveredLedger', () => { persistRecoveredLedger( side, { ledger: { ...ledger, round: 4 }, commitId: null, reviewId: 40 }, - false, - false, + { noOwnReview: false, identityKnown: false }, ); const written = JSON.parse(readFileSync(side, 'utf8')); expect(written.round).toBe(4); @@ -299,8 +301,7 @@ describe('persistedAnchorSha', () => { foreign: false, author: null, } as unknown as Parameters[1], - false, - true, + { noOwnReview: false, identityKnown: true }, ); // The guard kept round 6 — so the anchor on disk is round 6's, not the // round-5 one this run recovered. diff --git a/packages/cli/src/commands/review/pr-context.test.ts b/packages/cli/src/commands/review/pr-context.test.ts index a1095258fa3..fc719780581 100644 --- a/packages/cli/src/commands/review/pr-context.test.ts +++ b/packages/cli/src/commands/review/pr-context.test.ts @@ -72,8 +72,13 @@ import { latestLedger, recoverLedger, renderLedgerSection, + FOREIGN_ROUND_HEADROOM, } from './pr-context.js'; -import { serializeLedger, type Ledger } from './lib/ledger.js'; +import { + serializeLedger, + LEDGER_MAX_FINDINGS, + type Ledger, +} from './lib/ledger.js'; // Guards the recognition of legacy suggestion-summary comments. This is what // decides which issue comment is excluded from the "Already discussed" list. @@ -1327,6 +1332,9 @@ describe('latestLedger — the split trust surface', () => { expect(foreign?.ledger.model).toBeUndefined(); expect(foreign?.ledger.findings).toEqual(anchored.findings); expect(foreign?.ledger.round).toBe(2); + // Pure-foreign (no own base): nothing was merged, so the renderer's + // whole-list THEIR-claims sentence is the accurate one. + expect(foreign?.merged).toBe(false); }); it('carries the anchor through intact for the OWN account', () => { @@ -1485,6 +1493,11 @@ describe('latestLedger — the split trust surface', () => { const squatting = 'LGTM '; + const foreignOnly = recoverLedger( + [ + review('maintainer', '2026-01-01T00:00:00Z', emptyOwn), + review('stranger', '2026-01-09T00:00:00Z', doctored), + ], + 'maintainer', + ).recovered; + expect(foreignOnly?.merged).toBe(false); + expect(foreignOnly?.ledger.findings.map((f) => f.id)).toEqual(['R7-2']); + }); + + it('the merge cap trims FOREIGN entries first — own-first is load-bearing', () => { + // Both markers can legitimately carry LEDGER_MAX_FINDINGS entries, so a + // union of up to twice the cap is reachable on a long-lived PR with a + // CI-bot interleave. Own-first concatenation is what makes the re-cap + // trim the foreign tail; a reversed concatenation survived the suite + // (every merge fixture held 1-2 entries) and would trim THIS account's + // certified entries first — the suppression class the union was added + // to kill, reintroduced by an ordering nobody pinned. + const ownFindings = Array.from( + { length: LEDGER_MAX_FINDINGS }, + (_, i) => + `{"id":"R7-${i + 1}","sev":"S","file":"a.ts","title":"own ${i + 1}"}`, + ).join(','); + const foreignFindings = Array.from( + { length: 5 }, + (_, i) => + `{"id":"R8-${i + 1}","sev":"S","file":"b.ts","title":"theirs ${i + 1}"}`, + ).join(','); + // Both source markers declare their OWN losses: the union's dropped is a + // three-term sum (own marker's, foreign marker's, the re-cap), and a + // fixture whose markers carried none pinned only the re-cap term — a + // refactor zeroing either marker-borne term shipped green. + const atCap = recoverLedger( + [ + review( + 'maintainer', + '2026-01-01T00:00:00Z', + `x `, + ), + review( + 'stranger', + '2026-01-09T00:00:00Z', + `x `, + ), + ], + 'maintainer', + ).recovered; + // Every own id survives the cap… + const ids = atCap?.ledger.findings.map((f) => f.id) ?? []; + expect(ids.filter((id) => id.startsWith('R7-'))).toHaveLength( + LEDGER_MAX_FINDINGS, + ); + // …the trimmed entries are exactly the foreign tail… + expect(ids).toHaveLength(LEDGER_MAX_FINDINGS); + expect(ids.some((id) => id.startsWith('R8-'))).toBe(false); + // …and `dropped` is the full three-term sum: own marker's 3, foreign + // marker's 2, plus the 5 the re-cap trimmed. + expect(atCap?.ledger.dropped).toBe(3 + 2 + 5); }); it('does not adopt a foreign round implausibly far past our own', () => { @@ -1588,6 +1672,43 @@ describe('latestLedger — the split trust surface', () => { ); expect(near?.ledger.round).toBe(11); expect(near?.foreign).toBe(true); + + // The boundary itself, SYMBOLICALLY — near/far fixtures alone constrain + // the constant only to a wide interval, and both a 6 and a 499 mutant + // left the suite green: one refuses a bot a week ahead (the measured + // full-diff re-review regression), the other widens the per-hostile-post + // counter-inflation bound ~8x. The symbolic fixtures cannot kill a value + // mutant either — rounds AND expectations both compute from the + // constant, so any mutated value satisfies the arithmetic in lockstep; + // pin the value itself: + expect(FOREIGN_ROUND_HEADROOM).toBe(64); + // Last admitted: + const atBound = latestLedger( + [ + review('maintainer', '2026-01-05T00:00:00Z', marker(8)), + review( + 'ci-bot', + '2026-01-09T00:00:00Z', + marker(8 + FOREIGN_ROUND_HEADROOM), + ), + ], + 'maintainer', + ); + expect(atBound?.ledger.round).toBe(8 + FOREIGN_ROUND_HEADROOM); + // …and first refused: + const pastBound = latestLedger( + [ + review('maintainer', '2026-01-05T00:00:00Z', marker(8)), + review( + 'ci-bot', + '2026-01-09T00:00:00Z', + marker(8 + FOREIGN_ROUND_HEADROOM + 1), + ), + ], + 'maintainer', + ); + expect(pastBound?.ledger.round).toBe(8); + expect(pastBound?.foreign).toBe(false); }); it('bounds foreign rounds from zero when this account never posted', () => { @@ -1601,6 +1722,52 @@ describe('latestLedger — the split trust surface', () => { 'maintainer', ); expect(found?.ledger.round).toBe(3); + + // The zero-base boundary, symbolically: rounds ≤ the headroom recover, + // the first past it does not. + const atBound = latestLedger( + [ + review( + 'ci-bot', + '2026-01-02T00:00:00Z', + marker(FOREIGN_ROUND_HEADROOM), + ), + ], + 'maintainer', + ); + expect(atBound?.ledger.round).toBe(FOREIGN_ROUND_HEADROOM); + expect( + latestLedger( + [ + review( + 'ci-bot', + '2026-01-02T00:00:00Z', + marker(FOREIGN_ROUND_HEADROOM + 1), + ), + ], + 'maintainer', + ), + ).toBeNull(); + }); + + it('holds the headroom under a NULL login — the outage fallback stays bounded', () => { + // The FOREIGN_ROUND_HEADROOM doc promises: "Under a FAILED identity + // lookup (null login) … recovery is bounded to rounds ≤ the headroom." + // A mutant guarding the bound on a known identity (`me && …`) survived + // the whole suite — the only null-login fixture used round 2, which + // clears any plausible bound — and during a rate-limit blip a squatter's + // round-9999 marker beside the bot's round-3 one was adopted + // round-first: compose's capped stamp then pins the counter at the cap, + // the permanent win the headroom exists to prevent, reopened exactly + // during the identity-outage fallback. + const found = latestLedger( + [ + review('ci-bot', '2026-01-02T00:00:00Z', marker(3)), + review('stranger', '2026-01-09T00:00:00Z', marker(9999)), + ], + null, + ); + expect(found?.ledger.round).toBe(3); }); it('refuses an out-of-range round from any account', () => { @@ -1755,6 +1922,47 @@ describe('renderLedgerSection', () => { expect(own).not.toContain('THEIR claims'); }); + it('a MERGED list is mixed provenance — never all THEIR claims', () => { + // The union merges a foreign winner OVER this account's own findings, so + // the rendered table holds both accounts' entries. The pure-foreign + // sentence attributed the whole list — the own certified subset + // included — to the foreign poster, inverting the trust distinction the + // author parameter exists to enforce; and the PARTIAL note pinned a + // dropped sum spanning two markers plus the merge re-cap on one round's + // size cap, sending a Step 6 reader to cross-reference a round that + // lost nothing. + const mergedSection = renderLedgerSection( + { + v: 1, + round: 8, + findings: [ + { id: 'R7-1', sev: 'C', file: 'a.ts', title: 'own certified' }, + { id: 'R8-1', sev: 'S', file: 'b.ts', title: 'theirs' }, + ], + dropped: 2, + }, + 'm', + 'qwen-code-ci-bot', + true, + ); + expect(mergedSection).toContain( + "MERGED over this account's own latest findings", + ); + expect(mergedSection).toContain( + 'entries this account certified are its own claims', + ); + expect(mergedSection).not.toContain('THEIR claims'); + // The dropped sum is a three-term total any subset of which can be + // zero, and the note must not pin it on any single round or claim both + // sources lost entries — a reader cross-referencing a complete marker + // would dismiss the warning as stale. + expect(mergedSection).toContain('did not survive into this merged list'); + expect(mergedSection).toContain('not attributable to any single round'); + expect(mergedSection).not.toContain('from round 8 did not fit'); + // No anchor travels with a foreign winner, merged or not. + expect(mergedSection).toContain('no incremental anchor'); + }); + it('says so when the ledger is PARTIAL, and stays silent when it is not', () => { // The size cap can drop entries. A truncated list rendered under "every // entry below is owed a ruling" reads as complete, and the next round @@ -1870,6 +2078,7 @@ describe('renderLedgerSection', () => { recovered, 'model-a@aaaaaaaa', null, + false, 'ffff1111ffff1111', ); expect(diverged).toContain('Do NOT pass any sha'); @@ -1887,12 +2096,13 @@ describe('renderLedgerSection', () => { recovered, 'model-a@aaaaaaaa', null, + false, 'aaaa2222aaaa2222', ), ).toContain('the same-model contract HOLDS'); // …and so does a side file that holds no anchor to disagree with. expect( - renderLedgerSection(recovered, 'model-a@aaaaaaaa', null, null), + renderLedgerSection(recovered, 'model-a@aaaaaaaa', null, false, null), ).toContain('the same-model contract HOLDS'); }); @@ -2246,7 +2456,8 @@ describe('buildMarkdown host baking', () => { [longReview], null, undefined, - undefined, + null, + false, 'ghe.example.com', ); expect(md).toContain( @@ -2276,6 +2487,7 @@ describe('buildMarkdown host baking', () => { ledger, '', 'qwen-code-ci-bot', + false, 'ghe.example.com', ); expect(md).toContain("**@qwen-code-ci-bot**'s last posted review"); @@ -2319,7 +2531,8 @@ describe('buildMarkdown host baking', () => { [], null, undefined, - undefined, + null, + false, 'ghe.example.com', ); expect(md).toContain( @@ -2403,6 +2616,174 @@ describe('runPrContext identity failure (handler level)', () => { }); expect(rmSyncMock).not.toHaveBeenCalled(); }); + + const run = async () => + (prContextCommand.handler as (a: unknown) => Promise)({ + _: [], + $0: 'qwen', + pr_number: '6711', + owner_repo: 'o/r', + out: '/tmp/ctx.md', + }); + const contextWrite = () => + (writeFileSyncMock.mock.calls.find( + (c) => c[0] === '/tmp/ctx.md', + )?.[1] as string) ?? ''; + + it('recovery SURVIVES the identity throw — isolation, not just non-deletion', async () => { + // The marker-less fixture above cannot tell the two arms apart: with the + // try/catch around currentUser() removed, the throw degrades recovery to + // "no ledger" and rmSync is still never called — green — while a fresh + // machine on a rate-limit blip recovers nothing, compose restarts at + // round 1 and re-issues R1-* ids the PR already carries. A marker in the + // walked list is the discriminator: isolation keeps the ledger section + // in the written context; a swallowed-by-the-outer-catch recovery loses + // it. + currentUserMock.mockImplementation(() => { + throw new Error('rate limited'); + }); + ghApiAllMock.mockReset(); + ghApiAllMock + .mockReturnValueOnce([]) + .mockReturnValueOnce([]) + .mockReturnValueOnce([ + { + id: 9, + user: { login: 'someone' }, + state: 'COMMENTED', + submitted_at: '2026-08-01', + body: 'x ', + }, + ]); + await run(); + // Narrowed to the side file: recovery WRITES here, and the write path's + // debris cleanup legitimately rm's its own `.tmp` (the mocked + // writeFileSync never created it, so the real rename throws). + expect( + rmSyncMock.mock.calls.some((c) => + String(c[0]).endsWith('prev-ledger.json'), + ), + ).toBe(false); + expect(contextWrite()).toContain('## Previous /review round'); + }); + + it('deletes ONLY under the full licence, and both conjuncts have teeth', async () => { + // The two unpinned halves of the deletion flag. A confirmed identity + // over a walked list with no own review and nothing recovered IS the + // licence — deletion fires: + currentUserMock.mockReturnValue('bot'); + await run(); + expect(rmSyncMock).toHaveBeenCalled(); + }); + + it('a marker-less OWN review is a persistent state, not proven absence', async () => { + // An own follow-up whose marker fails to parse must not read as "no + // prior round": deleting the side file here resets the posture clock + // mid-PR — the documented regression the `sawOwnReview` conjunct + // exists to prevent. + currentUserMock.mockReturnValue('someone'); + await run(); + expect(rmSyncMock).not.toHaveBeenCalled(); + }); + + it('wires the foreign marker through to the rendered context and the side file', async () => { + // Both handler describes used marker-less fixtures, so recoverLedger + // returned null in every handler test and the foreign→author wiring was + // never executed: `prevLedgerAuthor = null` and a dropped `.foreign ?` + // conditional both shipped green. Cross-account recovery — the primary + // case — must render whose claims these are, and the persisted side + // file must not carry the foreign sha. + currentUserMock.mockReturnValue('maintainer'); + ghApiAllMock.mockReset(); + ghApiAllMock + .mockReturnValueOnce([]) + .mockReturnValueOnce([]) + .mockReturnValueOnce([ + { + id: 11, + user: { login: 'ci-bot' }, + state: 'COMMENTED', + submitted_at: '2026-08-01', + body: 'x ', + }, + ]); + await run(); + const ctx = contextWrite(); + expect(ctx).toContain('**@ci-bot**'); + expect(ctx).toContain('THEIR claims'); + // The side-file write is the atomic temp write; the foreign anchor was + // stripped at the recovery seam and must not reappear on disk. + const sideWrite = writeFileSyncMock.mock.calls.find((c) => + String(c[0]).includes('prev-ledger.json'), + ); + expect(sideWrite).toBeDefined(); + expect(String(sideWrite?.[1])).not.toContain('"sha"'); + + // And the OWN anchored ledger renders as this account's, sha intact — + // the dropped-conditional mutant rendered it as another account's + // claims beside its own "reviewed at" sha. + vi.clearAllMocks(); + ensureAuthenticatedMock.mockReturnValue(undefined); + ghMock.mockReturnValue(metaJson); + currentUserMock.mockReturnValue('maintainer'); + ghApiAllMock + .mockReturnValueOnce([]) + .mockReturnValueOnce([]) + .mockReturnValueOnce([ + { + id: 12, + user: { login: 'maintainer' }, + state: 'COMMENTED', + submitted_at: '2026-08-02', + body: 'x ', + }, + ]); + await run(); + const ownCtx = contextWrite(); + expect(ownCtx).toContain("this account's last posted review"); + expect(ownCtx).not.toContain('THEIR claims'); + expect(ownCtx).toContain('reviewed at'); + const ownSideWrite = writeFileSyncMock.mock.calls.find((c) => + String(c[0]).includes('prev-ledger.json'), + ); + expect(String(ownSideWrite?.[1])).toContain('"sha"'); + }); + + it('wires the MERGED union through the handler to the rendered context', async () => { + // The passthrough is one hardcodable constant: with + // `const prevLedgerMerged = false;` at the call site every other test + // stayed green — the recovery seam asserts `merged` on the return + // value, the renderer test passes `true` directly, and no handler + // fixture held BOTH an own marker and a higher-round foreign winner. + // A real cross-account recovery would then render the pure-foreign + // THEIR-claims wording over a list whose own subset this account + // certified. + currentUserMock.mockReturnValue('maintainer'); + ghApiAllMock.mockReset(); + ghApiAllMock + .mockReturnValueOnce([]) + .mockReturnValueOnce([]) + .mockReturnValueOnce([ + { + id: 21, + user: { login: 'maintainer' }, + state: 'COMMENTED', + submitted_at: '2026-08-01', + body: 'x ', + }, + { + id: 22, + user: { login: 'ci-bot' }, + state: 'COMMENTED', + submitted_at: '2026-08-02', + body: 'x ', + }, + ]); + await run(); + const ctx = contextWrite(); + expect(ctx).toContain("MERGED over this account's own latest findings"); + expect(ctx).not.toContain('THEIR claims'); + }); }); describe('runPrContext host baking (handler level)', () => { diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index dc57f9e6d16..13800857872 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -787,7 +787,7 @@ export interface RecoveredLedger { * identity vouches for, and the round is full-range. That is the fallback's * price, paid only while the identity endpoint is down. */ -const FOREIGN_ROUND_HEADROOM = 64; +export const FOREIGN_ROUND_HEADROOM = 64; /** * The latest machine ledger posted on this PR — with the trust surface split. @@ -835,7 +835,19 @@ export function recoverLedger( login: string | null, ): { recovered: - | (RecoveredLedger & { foreign: boolean; author: string | null }) + | (RecoveredLedger & { + foreign: boolean; + author: string | null; + /** + * True when the union fired: a foreign winner was merged OVER this + * account's own latest findings. The renderer keys its provenance + * wording on it — a merged list is NOT "another account's claims" + * (the own subset is this account's own), and its `dropped` sum + * spans two markers plus the re-cap, so the PARTIAL note must not + * attribute it to one round's size cap. + */ + merged: boolean; + }) | null; sawOwnReview: boolean; } { @@ -937,7 +949,14 @@ export function recoverLedger( // shared id space — and the union is exactly what makes the headroom doc's // "re-ruled entry by entry" true for entries a displacement would have // removed from view. - if (best.foreign && bestOwn) { + let mergedOverOwn = false; + // Non-empty own list only: an ordinary LGTM round posts `"findings":[]`, + // and with zero own entries there is nothing to merge OVER — flagging that + // shape `merged` made the provenance wording claim own-certified entries + // exist when none do (and misattributed the PARTIAL note's sum). The + // foreign winner recovers as pure-foreign, which is exactly what it is. + if (best.foreign && bestOwn && bestOwn.ledger.findings.length > 0) { + mergedOverOwn = true; const ownIds = new Set(bestOwn.ledger.findings.map((f) => f.id)); const merged = [ ...bestOwn.ledger.findings, @@ -961,6 +980,7 @@ export function recoverLedger( reviewId: best.id, foreign: best.foreign, author: best.author, + merged: mergedOverOwn, }, sawOwnReview, }; @@ -970,13 +990,19 @@ export function recoverLedger( export function latestLedger( reviews: RawReview[], login: string | null, -): { ledger: Ledger; foreign: boolean; author: string | null } | null { +): { + ledger: Ledger; + foreign: boolean; + author: string | null; + merged: boolean; +} | null { const { recovered } = recoverLedger(reviews, login); return recovered ? { ledger: recovered.ledger, foreign: recovered.foreign, author: recovered.author, + merged: recovered.merged, } : null; } @@ -1008,7 +1034,7 @@ export function persistedAnchorSha(sideFilePath: string): string | null { /** * Persist (or degrade) the prev-ledger side file for this run's recovery. - * Three outcomes, each honest about what this run learned: + * Four outcomes, each honest about what this run learned: * * - Recovered: the ledger's own fields plus `commitId`/`reviewId` — the age * reference and its provenance for Step 6's convergence posture. Readers @@ -1055,9 +1081,14 @@ export function persistedAnchorSha(sideFilePath: string): string | null { export function persistRecoveredLedger( sideFilePath: string, recovered: RecoveredLedger | null, - noOwnReview: boolean, - identityKnown: boolean, + // Named, not positional: the pair encodes a safety invariant (deletion is + // licensed only under a PROVEN identity) that two adjacent bare booleans + // could not defend — a swapped call compiled cleanly, and on an + // identity-known run whose recovery threw it deleted the side file and + // reset the id space with every suite green. + flags: { noOwnReview: boolean; identityKnown: boolean }, ): void { + const { noOwnReview, identityKnown } = flags; // Unique per process: two same-PR fetches racing on one fixed `.tmp` can // rename each other's bytes (A renames B's write; B's ENOENT is // swallowed), leaving the side file disagreeing with the context A holds @@ -1296,11 +1327,20 @@ function anchorRuling( * foreign work list as this account's own certified round. Such a ledger * reaches here already stripped of its `sha`, so the gate above never rules * on one: a foreign anchor is not withheld by comparison, it is absent. + * `merged` refines that: when the foreign winner was merged OVER this + * account's own findings (the union), the list is MIXED — calling it all + * "THEIR claims" gave false provenance for the own subset and inverted the + * exact trust distinction the author sentence exists to enforce — and its + * `dropped` sum spans two markers plus the merge re-cap, so the PARTIAL + * note must not pin the loss on one round's size cap (a Step 6 reader + * cross-referencing that round's body finds it complete and dismisses the + * warning as stale). */ export function renderLedgerSection( ledger: Ledger, running: string, author: string | null = null, + merged = false, /** * The `sha` the prev-ledger side file holds after this run's persist * decision — what Step 1 will actually pass. Null when the file holds none @@ -1330,13 +1370,15 @@ export function renderLedgerSection( return [ '## Previous /review round (machine ledger)', '', - `Round ${ledger.round}${ledger.sha ? `, reviewed at \`${code(ledger.sha)}\`${ledger.model ? ` by \`${code(ledger.model)}\`` : ''}` : ''}, recovered from the marker ${author ? `**@${cell(author)}**'s last posted review carried — another account, so these are THEIR claims and no incremental anchor travelled with them (the sha never crosses accounts; this round is full-range unless a local cache supplies one)` : `this account's last posted review carried`}. **Every entry below is owed a this-round ruling** (fixed / still stands / cannot tell / superseded by ) under Step 6's previous-round rules — the ledger is a work list, not a verdict; re-assert each claim against the code before repeating or retiring it.${ledger.sha ? ` ${anchorRuling(ledger, running, code, persistedSha)}` : ''}`, + `Round ${ledger.round}${ledger.sha ? `, reviewed at \`${code(ledger.sha)}\`${ledger.model ? ` by \`${code(ledger.model)}\`` : ''}` : ''}, recovered from ${author ? (merged ? `**@${cell(author)}**'s round-${ledger.round} marker MERGED over this account's own latest findings — entries this account certified are its own claims, the rest are @${cell(author)}'s, and no incremental anchor travelled with the foreign marker (the sha never crosses accounts; this round is full-range unless a local cache supplies one)` : `the marker **@${cell(author)}**'s last posted review carried — another account, so these are THEIR claims and no incremental anchor travelled with them (the sha never crosses accounts; this round is full-range unless a local cache supplies one)`) : `the marker this account's last posted review carried`}. **Every entry below is owed a this-round ruling** (fixed / still stands / cannot tell / superseded by ) under Step 6's previous-round rules — the ledger is a work list, not a verdict; re-assert each claim against the code before repeating or retiring it.${ledger.sha ? ` ${anchorRuling(ledger, running, code, persistedSha)}` : ''}`, // A truncated ledger must not read like a complete one. `dropped` exists // to draw that line, and this is the only place a reader sees the list. ...(ledger.dropped ? [ '', - `**This list is PARTIAL**: ${ledger.dropped} further finding(s) from round ${ledger.round} did not fit the marker's size cap and are not here. Absence below is not evidence a finding was fixed — say so rather than reporting the missing ones as retired.`, + merged + ? `**This list is PARTIAL**: ${ledger.dropped} further finding(s) did not survive into this merged list — lost to a source marker's size cap or to the merge's own re-cap, and not attributable to any single round's marker. Absence below is not evidence a finding was fixed — say so rather than reporting the missing ones as retired.` + : `**This list is PARTIAL**: ${ledger.dropped} further finding(s) from round ${ledger.round} did not fit the marker's size cap and are not here. Absence below is not evidence a finding was fixed — say so rather than reporting the missing ones as retired.`, ] : []), '', @@ -1358,6 +1400,8 @@ export function buildMarkdown( me: string = '', /** Set only when the ledger came from another account — see the section. */ prevLedgerAuthor: string | null = null, + /** True when the ledger is the union of a foreign winner over own findings. */ + prevLedgerMerged = false, /** The PR host (GitHub Enterprise); baked into the emitted refetch commands. */ host?: string, /** See `renderLedgerSection` — the anchor that survives on disk. */ @@ -1441,6 +1485,7 @@ export function buildMarkdown( prevLedger, roundModelIdFrom(process.env), prevLedgerAuthor, + prevLedgerMerged, persistedSha, ), ); @@ -1676,30 +1721,37 @@ async function runPrContext(args: PrContextArgs): Promise { const prevLedgerAuthor = prevRecovered?.foreign ? (prevRecovered.author ?? null) : null; - // The side file's three outcomes live in the helper: recovered → written + const prevLedgerMerged = prevRecovered?.merged ?? false; + // The side file's four outcomes live in the helper: recovered → written // whole (any account — the round counter is a shared id space, and the // anchor was already stripped at the seam for a foreign winner); // demonstrably no prior round for THIS account and none recovered from any // other → removed (a stale counter would stamp rounds nobody posted); - // recovery threw → round counter kept, age-sensitive `commitId`/`reviewId` - // stripped. + // recovered anonymously over an existing file → only the round counter and + // tiebreak advance, the persisted list survives and `sha`/`commitId` are + // dropped (an anonymous round cannot be re-vouched); recovery threw → + // round counter kept, age-sensitive `commitId`/`reviewId` stripped. persistRecoveredLedger( join(dirname(out), `qwen-review-pr-${prNumber}-prev-ledger.json`), prevRecovered, - // Deletion is licensed ONLY by proof of true absence: a CONFIRMED - // identity, and a non-empty list this run walked in which no submitted - // review by that identity exists. An empty `reviews` may be an error - // envelope ghApiAll flattened to []; an own review whose marker fails to - // parse is a persistent state, not absence; and a failed identity lookup - // proves nothing about anyone — all take the conservative strip path. - // (A recovered foreign ledger also protects the file, but through the - // helper's own recovered-first branch, not through this flag.) - reviews.length > 0 && identityKnown && !recoveryThrew && !sawOwnReview, - // Separately from deletion: an ANONYMOUS recovery (identity unknown) - // must not replace the persisted work list — the helper's fourth - // outcome. Every marker walks as foreign without a `me`, so the union - // never protected the own list this run. - identityKnown, + { + // Deletion is licensed ONLY by proof of true absence: a CONFIRMED + // identity, and a non-empty list this run walked in which no submitted + // review by that identity exists. An empty `reviews` may be an error + // envelope ghApiAll flattened to []; an own review whose marker fails + // to parse is a persistent state, not absence; and a failed identity + // lookup proves nothing about anyone — all take the conservative strip + // path. (A recovered foreign ledger also protects the file, but + // through the helper's own recovered-first branch, not through this + // flag.) + noOwnReview: + reviews.length > 0 && identityKnown && !recoveryThrew && !sawOwnReview, + // Separately from deletion: an ANONYMOUS recovery (identity unknown) + // must not replace the persisted work list — the helper's fourth + // outcome. Every marker walks as foreign without a `me`, so the union + // never protected the own list this run. + identityKnown, + }, ); const persistedSha = persistedAnchorSha( @@ -1726,6 +1778,7 @@ async function runPrContext(args: PrContextArgs): Promise { prevLedger, me, prevLedgerAuthor, + prevLedgerMerged, bakeHost, persistedSha, ); diff --git a/packages/cli/src/commands/review/test-delta.ts b/packages/cli/src/commands/review/test-delta.ts index c76671696d5..289609a4c75 100644 --- a/packages/cli/src/commands/review/test-delta.ts +++ b/packages/cli/src/commands/review/test-delta.ts @@ -51,24 +51,30 @@ import { type CommandResult, } from './build-test.js'; import { failingFilesOf } from './lib/failing-files.js'; +import { TEST_COMMAND_RE } from './lib/npm-toolchain.js'; /** * The exact shapes `build-test` emits for a test command — and the only ones * this command will hand to a shell. * * The report is a FILE this reads and then executes from, with `shell: true`, - * in the base worktree. Nothing else in the pipeline re-executes a string it - * read back off disk, so nothing else has to care where that string came from; - * this does. The workspace token is a directory, and a directory is a name a - * pull request can choose: `packages/x";curl …|sh;"` is a legal path in git - * and on Linux, and it round-trips through the report into a shell. + * in the base worktree. Nothing else re-executes a string it read back off + * disk except `build-test --resume`, and both gates are the SAME imported + * predicate, defined beside the emitter (`testCommand` in npm-toolchain): + * two byte-identical copies once guarded the two re-execution sites, and a + * grammar change applied to one would have silently diverged them — this + * file skipping a valid stored command (the under-measurement direction this + * command exists to avoid) while the other kept accepting it. The workspace + * token is a directory, and a directory is a name a pull request can choose: + * `packages/x";curl …|sh;"` is a legal path in git and on Linux, and it + * round-trips through the report into a shell. * * Restricting to the emitter's own grammar costs nothing real — `build-test` - * produces `npm test` and `npm test --workspace=""`, both matched here — - * and anything outside it is skipped and disclosed rather than run, which is + * produces `npm test` and `npm test --workspace=""`, both matched — and + * anything outside it is skipped and disclosed rather than run, which is * the same treatment every other thing this command cannot do gets. */ -const RERUNNABLE_COMMAND_RE = /^npm test(?: --workspace="[\w@./-]+")?$/; +const RERUNNABLE_COMMAND_RE = TEST_COMMAND_RE; /** `trimOutput`'s own marker — the one signal that a stored output is partial. */ const TRIM_MARKER_RE = /\.\.\. \[\d+ characters omitted/;