diff --git a/packages/cli/src/commands/review/check-coverage.test.ts b/packages/cli/src/commands/review/check-coverage.test.ts index a791e878c15..985a3c87425 100644 --- a/packages/cli/src/commands/review/check-coverage.test.ts +++ b/packages/cli/src/commands/review/check-coverage.test.ts @@ -39,6 +39,7 @@ import { findingsFilePath, } from './lib/prompt-record.js'; import { requiredAgents, type RosterPlan } from './lib/roster.js'; +import { BRIEFS } from './lib/agent-briefs.js'; import { checkCoverageCommand } from './check-coverage.js'; import { writeStderrLine } from '../../utils/stdioHelpers.js'; @@ -858,6 +859,191 @@ describe('budget-gap disclosures — guarded, parsed, never punished', () => { expect(r.budgetGaps).toEqual([]); }); + it("names a rostered discloser by its brief's publicLabel, not its prompt", () => { + // The fallback label is the launch prompt's first line, and a real + // posted body rendered a disclosure as "You are review agent + // `reverse-audit` — Reverse audit agen...:" — plumbing, truncated, on a + // public PR page. A record matching a built role prompt gets the + // author-register name instead. The key is spelled the way + // agent-prompt records a findings-taking role — with its digest + // suffix — because that is the shape the lookup has to survive. + transcript('a1', good(1), { calls: 3, range: [0, 100] }); + transcript('a2', good(2), { calls: 2, range: [100, 100] }); + const p = plan(); + const role = 'reverse-audit'; + const key = `${role}--round-1--abc123def456`; + const d = promptRecordDir(p); + const brief = briefPath(p, key); + const prompt = + `You are ${role}.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), prompt); + transcript('tm-gap', prompt, { + calls: 3, + opens: [brief], + text: + 'No issues found — mapped the behaviours.\n' + + 'Budget gap: the negative-path matrix rows', + }); + + const gaps = coverageFromTranscripts(p, ENV).budgetGaps; + const entry = gaps.find((g) => + g.gaps.includes('the negative-path matrix rows'), + ); + expect(entry?.agent).toBe(BRIEFS[role].publicLabel); + expect(entry?.agent).not.toContain('You are'); + }); + + it('keeps the launch first line for a discloser no built prompt matches', () => { + // The boundary of the rename above: only a record matching a BUILT + // role prompt escapes the fallback, and the fallback is the launch + // prompt's first line, truncated — the exact register the production + // spill wore. A discloser whose prompt the run wrote itself keeps + // that name; a regression to a worse default fails right here, in + // the channel where the spill landed. + transcript('a1', good(1), { calls: 3, range: [0, 100] }); + transcript('a2', good(2), { calls: 2, range: [100, 100] }); + const p = plan(); + const prompt = + 'You are review agent `free-lance`, an extra pass this run wrote for itself.\n' + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + transcript('stray', prompt, { + calls: 2, + range: [0, 100], + text: 'Walked what I could.\nBudget gap: the stray pass', + }); + + const gaps = coverageFromTranscripts(p, ENV).budgetGaps; + const entry = gaps.find((g) => g.gaps.includes('the stray pass')); + expect(entry?.agent).toBe( + 'You are review agent `free-lance`, an extra pass this run...', + ); + }); + + it('names idle and unopened rostered agents in the same register', () => { + // The fallback name rides the posted body's coverage lines too: a + // rostered whole-diff agent that made no tool call, or none against + // the diff, must not read "You are ..." there either. + const p = plan(); + const d = promptRecordDir(p); + const launch = (key: string, offset: number): string => { + const brief = briefPath(p, key); + const prompt = + `You are ${key}.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=${offset}, limit=100)`; + writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), prompt); + return prompt; + }; + transcript('tm-idle', launch('reverse-audit--round-1--abc123def456', 0), { + calls: 0, + }); + const unopenedKey = 'verify--round-1--fed456abc123'; + transcript('tm-unopened', launch(unopenedKey, 100), { + opens: [briefPath(p, unopenedKey)], + }); + + const r = coverageFromTranscripts(p, ENV); + expect(r.idleAgents).toEqual([BRIEFS['reverse-audit'].publicLabel]); + expect(r.unopenedAgents).toEqual([BRIEFS['verify'].publicLabel]); + }); + + it('keeps the file on a file-scoped rostered label', () => { + // A `${role}--${file}` launch is rostered per heavy file; dropping the + // file makes N per-file agents of one role read as one repeated line in + // the posted body, indistinguishable — the author cannot tell which + // file's check stopped. The label keeps the file, the way + // `publicRoleLabel` renders these same roles elsewhere. Not `plan()`: + // its files are not heavy, so its roster carries no invariant agents + // for the label lookup to resolve against. + const p = join(dir, 'plan.json'); + writeFileSync( + p, + JSON.stringify({ + diffPathAbsolute: DIFF, + srcDiffLines: 5000, + diffLines: 5000, + files: [ + { path: 'src/a.ts', kind: 'source', removedLines: 0, heavy: true }, + { path: 'src/b.ts', kind: 'source', removedLines: 0, heavy: true }, + ], + chunks: [ + { id: 1, startLine: 1, endLine: 100 }, + { id: 2, startLine: 101, endLine: 200 }, + ], + }), + ); + const d = promptRecordDir(p); + mkdirSync(d, { recursive: true }); + const role = 'invariant-a'; + const launch = (file: string): string => { + const key = `${role}--${file}`; + const brief = briefPath(p, key); + const prompt = + `You are ${key}.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), prompt); + return prompt; + }; + transcript('inv-idle-a', launch('src/a.ts'), { calls: 0 }); + transcript('inv-idle-b', launch('src/b.ts'), { calls: 0 }); + const old = new Date(2020, 0, 1); + utimesSync(p, old, old); + + const r = coverageFromTranscripts(p, ENV); + const base = BRIEFS[role].publicLabel; + expect(r.idleAgents).toHaveLength(2); + expect(r.idleAgents).toContain(`${base} on src/a.ts`); + expect(r.idleAgents).toContain(`${base} on src/b.ts`); + // The Chinese twin keeps the file too: dropping it renders two + // identical zh clauses with no file discriminator — the exact + // indistinguishability this test exists to prevent, on the zh side. + expect(r.publicLabelsZh[`${base} on src/a.ts`]).toBe( + `${BRIEFS[role].publicLabelZh}(src/a.ts)`, + ); + expect(r.publicLabelsZh[`${base} on src/b.ts`]).toBe( + `${BRIEFS[role].publicLabelZh}(src/b.ts)`, + ); + }); + + it('never reads a digest or chunk suffix as a file on a rostered label', () => { + // Two-segment keys are not all file-scoped: `verify--` and + // `reverse-audit--chunk-N` are Step 3B/4 plumbing. Only the roster's + // per-file requirements carry a file into the label; anything else + // keeps the bare publicLabel, so no digest or chunk id reaches the + // posted body. + const p = plan(); + const d = promptRecordDir(p); + const brief = briefPath(p, 'verify--abc123def456'); + const prompt = + `You are verify--abc123def456.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync( + join(d, `${encodeURIComponent('verify--abc123def456')}.txt`), + prompt, + ); + transcript('tm-digest', prompt, { calls: 0 }); + // The chunk-suffixed findings-role key shape: a key-shape heuristic in + // place of the roster lookup would read `chunk-1` as a file and leak + // the chunk id onto the public label, so it keeps the bare publicLabel. + const chunkKey = 'reverse-audit--chunk-1--round-1--abc123def456'; + const chunkBrief = briefPath(p, chunkKey); + const chunkPrompt = + `You are ${chunkKey}.\n` + + `read_file(file_path="${chunkBrief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, `${encodeURIComponent(chunkKey)}.txt`), chunkPrompt); + transcript('tm-chunk-key', chunkPrompt, { calls: 0 }); + + const r = coverageFromTranscripts(p, ENV); + expect(r.idleAgents).toHaveLength(2); + expect(r.idleAgents).toContain(BRIEFS['verify'].publicLabel); + expect(r.idleAgents).toContain(BRIEFS['reverse-audit'].publicLabel); + }); + it('reports none when nobody disclosed one', () => { transcript('a1', good(1), { calls: 3 }); transcript('a2', good(2), { calls: 2 }); diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 02c366b3090..a9bd8e30340 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -808,6 +808,810 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve expect(r.body.split('review time budget').length - 1).toBe(1); }); + it("an idle rostered agent does not shadow the caller's relay of the same role", () => { + // A whiffed auditor that made zero tool calls lands in `idleAgents`, + // named by its brief's publicLabel — the very subject the orchestrator + // spells its scoped whiff relay in. The caller-echo prefix filter must + // not let the idle entry swallow that relay: the entry explains the + // idleness, the relay carries the scope, and the skill's promise is + // that such prose renders verbatim. The Step 4/5 floor fails the same + // run — its `reverse audit` gap entry shares the subject, so it shares + // the exemption. + transcript('a1', goodPrompt(1), { toolCalls: 3 }); + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const launch = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, 'reverse-audit.txt'), launch); + transcript('v-reverse_audit', launch, { toolCalls: 0 }); + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would relaunch a WORKING reverse auditor over this + // fixture's idle one. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + "reverse audit — chunk 2's auditor returned nothing substantive twice", + ], + }); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain( + 'Not reviewed: reverse audit — the agent made no tool call: it read nothing.', + ); + expect(r.body).toContain( + "Not reviewed: reverse audit — chunk 2's auditor returned nothing substantive twice.", + ); + }); + + it("an unopened rostered agent does not shadow the caller's relay of the same role", () => { + // The sibling push site: an agent that worked but never opened the diff + // is named in the same register, and its entry must not swallow a relay + // the caller spelled with its own reason either. + transcript('a1', goodPrompt(1), { toolCalls: 3 }); + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'verify'); + writeFileSync(brief, 'The verify brief.'); + const launch = + `You are review agent \`verify\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, 'verify.txt'), launch); + transcript('v-verify', launch, { opens: [brief] }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + 'verification — the findings file was never ruled on', + ], + }); + expect(r.body).toContain( + 'Not reviewed: verification — pointed at diff lines it never opened: ' + + 'it made tool calls, but none of them read the diff.', + ); + expect(r.body).toContain( + 'Not reviewed: verification — the findings file was never ruled on.', + ); + }); + + it("a Step 4/5 gap entry does not shadow the caller's relay of the same role", () => { + // The third push site of `roleLabelEntries`: when the reverse audit + // never ran at all, the floor's gap entry carries the subject + // `reverse audit` with no idle or unopened entry sharing it — that + // entry alone can swallow the scoped relay the orchestrator spelled + // with its own reason. The gap explains the missing floor, the relay + // carries the scope; both must render. The idle/unopened tests above + // would each still pass if the verification-gap addition were removed + // (their own entries cover the shared subject); this one pins the site. + const p = coveredPlan(['verify']); // reverse audit absent: the floor fails + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would re-record the very step this case means to lack. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + "reverse audit — chunk 2's auditor returned nothing substantive twice", + ], + }); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain( + 'Not reviewed: reverse audit — no auditor was launched with a prompt ' + + 'this skill builds', + ); + expect(r.body).toContain( + "Not reviewed: reverse audit — chunk 2's auditor returned nothing substantive twice.", + ); + }); + + it("a blind rostered agent does not shadow the caller's relay of the same role", () => { + // The blind loop's push into `roleLabelEntries`: a readsDiff-false role + // prompt plus an orchestrator-inserted chunk phrase launches an agent + // that is blind YET rostered — named by the role's publicLabel. Its + // entry must not swallow a relay spelled in that register; delete the + // push and the relay below is silently dropped (probe-reproduced). + // No working chunk-1 agent: one would supersede the blind entry. + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + // The built block never names the diff file; the chunk phrase is the + // orchestrator's insertion at launch. + const builtBlock = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")`; + writeFileSync(join(d, 'reverse-audit.txt'), builtBlock); + transcript('v-ra-blind', `${builtBlock}\nYou own chunk 1 of 2.`, { + toolCalls: 0, + }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + "reverse audit — chunk 1's auditor returned nothing substantive twice", + ], + }); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain( + 'Not reviewed: reverse audit — launched with a prompt that never ' + + 'named the diff file, so it could not have read it.', + ); + expect(r.body).toContain( + "Not reviewed: reverse audit — chunk 1's auditor returned nothing substantive twice.", + ); + }); + + it('a budget stop does not swallow an idle round of the same role', () => { + // The publicLabel register is shared by every round of a role: a round-2 + // budget stop and a round-1 auditor that made zero tool calls are ONE + // subject with two distinct failures, and the render dedup keys on the + // subject+reason pair or the second disclosure never reaches the body — + // silently, in the channel whose whole promise is the disclosure. Both + // lines render; the floor's brief-unread gap for the same subject joins + // them, one sentence per reason. + transcript('a1', goodPrompt(1), { toolCalls: 3, range: [0, 100] }); + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const key = 'reverse-audit--round-1--abc123def456'; + const brief = briefPath(p, key); + writeFileSync(brief, 'The reverse-audit brief.'); + const launch = + `You are review agent \`reverse-audit\` round 1.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), launch); + transcript('v-ra-idle', launch, { toolCalls: 0 }); + writeBudgetStop( + p, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 2, + ); + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would relaunch a WORKING reverse auditor over this + // fixture's idle one. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain( + 'Not reviewed: reverse audit — stopped before round 2 by the review time budget.', + ); + expect(r.body).toContain( + 'Not reviewed: reverse audit — the agent made no tool call: it read nothing.', + ); + // The third reason the comment above promises: the floor's brief-unread + // gap shares the subject, and the subject+reason dedup keeps it beside + // the stop and the idle round. + expect(r.body).toContain( + 'Not reviewed: reverse audit — it was launched with the built prompt ' + + 'but never opened its brief, so it audited without the gaps-only ' + + 'method and the finding format it was launched to follow.', + ); + }); + + it('an unopened chunk entry still dedups a caller relay of the same chunk', () => { + // The `chunk N` guard on `roleLabelEntries` earns its keep: chunk + // subjects keep the prefix dedup (a caller echoing `chunk 1 — …` is + // pasting the gate's own line, #7188), and only role-publicLabel entries + // join the exemption. Delete the guard at either loop and this relay + // renders twice — the coverage sentence and its verbatim echo. The + // fixture isolates the guard: an UNOPENED chunk agent (brief opened, + // zero diff reads) — a zero-tool-call idle chunk agent would also push a + // same-subject entry that dedups the relay regardless of the guard. + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + transcript('a1', goodPrompt(1), { opens: [briefPath(p, 'chunk-1')] }); + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would relaunch a WORKING chunk-1 agent over this + // fixture's unopened one. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + 'chunk 1 — the agent read its brief but never the diff', + ], + }); + // The chunk subject renders in the author's units (its files), the + // way every chunk gap does; the relay dedup keys on the internal + // `chunk 1` subject all the same. + expect(r.body).toContain( + 'Not reviewed: the diff section covering src/a.ts — pointed at diff ' + + 'lines it never opened: it made tool calls, but none of them read ' + + 'the diff.', + ); + expect(r.body).not.toContain('the agent read its brief but never the diff'); + }); + + it('an idle chunk entry still dedups a caller relay of the same chunk', () => { + // The idle loop's twin of the unopened/blind chunk guards, on the + // nothing-built shape (#7012): a zero-call chunk agent keeps its + // `chunk N` subject — internal register — and the prefix dedup with it. + // Nothing is BUILT in this fixture: a built chunk brief the idle agent + // failed to open would add a same-subject unread-brief entry that + // dedups the relay on its own and hides the guard's chunk half. With + // the chunk half of the guard letting the idle entry join the + // exemption, the relayed `chunk 1 — …` line double-renders the subject. + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + transcript('a1', goodPrompt(1), { toolCalls: 0 }); + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would relaunch a WORKING chunk-1 agent over this one. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: ['chunk 1 — the agent never started its walk'], + }); + expect(r.body).toContain( + 'Not reviewed: the diff section covering src/a.ts — the agent made ' + + 'no tool call: it read nothing.', + ); + expect(r.body).not.toContain('the agent never started its walk'); + }); + + it('an idle rostered agent keeps its Chinese twin in a bilingual body', () => { + // The rename resolves rostered names through the brief's publicLabel; + // the Chinese half must say them the way the sibling floor/budget-stop + // lines do (subjectZh), not fall back to the English label beside + // `反向审计` for the same missing pass. + transcript('a1', goodPrompt(1), { toolCalls: 3, range: [0, 100] }); + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false, han: true }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const key = 'reverse-audit--round-1--abc123def456'; + const brief = briefPath(p, key); + writeFileSync(brief, 'The reverse-audit brief.'); + const launch = + `You are review agent \`reverse-audit\` round 1.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), launch); + transcript('v-ra-idle', launch, { toolCalls: 0 }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '未审查:反向审计——该 agent 未发起任何工具调用:它什么都没读。', + ); + // The floor's brief-unread gap shares the subject; its zh half rides + // the same subjectZh/reasonZh plumbing and renders its own sentence. + expect(r.body).toContain( + '未审查:反向审计——它用构建的 prompt 启动,却从未打开自己的 brief,审计时缺失了只报缺口的方法和它本应遵循的发现格式。', + ); + }); + + it('an unopened rostered agent keeps its Chinese twin in a bilingual body', () => { + // The unopened loop's subjectZh: every bilingual test before exercised + // idle, budget-gap, rewritten-launch or floor entries, never an + // unopened rostered agent — dropping the field there embeds the English + // publicLabel in the zh half beside its proper Chinese siblings. + transcript('a1', goodPrompt(1), { toolCalls: 3 }); + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false, han: true }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'verify'); + writeFileSync(brief, 'The verify brief.'); + const launch = + `You are review agent \`verify\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, 'verify.txt'), launch); + transcript('v-verify', launch, { opens: [brief] }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '未审查:验证——它被指向 diff 的行却从未打开:有工具调用,但没有一次读取 diff。', + ); + }); + + it('a blind rostered agent keeps its Chinese twin in a bilingual body', () => { + // The blind loop's subjectZh, sibling of the unopened case above: a + // blind YET rostered agent names the role's publicLabelZh in the zh + // half, not the English label. No working chunk-1 agent: one would + // supersede the blind entry. + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false, han: true }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const builtBlock = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")`; + writeFileSync(join(d, 'reverse-audit.txt'), builtBlock); + transcript('v-ra-blind', `${builtBlock}\nYou own chunk 1 of 2.`, { + toolCalls: 0, + }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '未审查:反向审计——启动 prompt 从未提到 diff 文件,它不可能读过 diff。', + ); + }); + + it('a rostered budget-gap discloser keeps its Chinese twin in a bilingual body', () => { + // The budget-gap sentence's zh half names the agent too; it rides the + // same label lookup as the coverage entries. + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false, han: true }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const key = 'reverse-audit'; + const brief = briefPath(p, key); + writeFileSync(brief, 'The reverse-audit brief.'); + const launch = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), launch); + transcript('v-ra-gap', launch, { + toolCalls: 2, + range: [0, 100], + text: + 'No issues found — walked the diff.\n' + + 'Budget gap: the round-2 scope walk', + }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + 'Not explored to full depth (tool budget reached): ' + + 'reverse audit: `the round-2 scope walk`.', + ); + expect(r.body).toContain( + '未探索到全部深度(达到工具调用预算):反向审计:`the round-2 scope walk`。', + ); + }); + + it('a fallback-label entry keeps the prefix dedup for a caller relay', () => { + // The exemption register is ROSTERED publicLabels only: a non-rostered + // launch keeps its fallback label (the truncated `You are review agent + // …` first line), and that is internal register — a caller spelling + // a relay in it is pasting the gate's own line (#7188), so the prefix + // match keeps it. When the guard approximated "rostered" as "not a + // `chunk N` label", fallback-label entries joined the exemption and a + // relayed gate line double-rendered end-to-end (probe-reproduced): a + // free-lance agent goes idle, coverage names it by the fallback, the + // orchestrator relays in the same register. + transcript('a1', goodPrompt(1), { toolCalls: 3 }); + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const strayPrompt = + 'You are review agent `free-lance`, an extra pass this run wrote for itself.\n' + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + transcript('stray', strayPrompt, { toolCalls: 0 }); + const fallbackLabel = + 'You are review agent `free-lance`, an extra pass this run...'; + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would relaunch a WORKING agent over this idle one. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + `${fallbackLabel} — made zero tool calls on the payment-flow walk`, + ], + }); + const sentence = `Not reviewed: ${fallbackLabel} — the agent made no tool call: it read nothing.`; + expect(r.body).toContain(sentence); + // The relay rides the internal subject register and loses the + // collision to the coverage-derived line — rendered once, not twice. + expect(r.body.split(sentence).length - 1).toBe(1); + expect(r.body).not.toContain( + 'made zero tool calls on the payment-flow walk', + ); + }); + + it('a blind chunk entry still dedups a caller relay of the same chunk', () => { + // The blind-loop sibling of the unopened-chunk guard: a blind agent's + // label is `chunk N` — internal register — and keeps the prefix dedup + // the exemption exists to waive for role publicLabels ONLY. Without the + // guard here, a nothing-built run's blind chunk-1 entry joined the + // exemption and a relayed `chunk 1 — …` line double-rendered; the + // mutant survives every other test, so this one is its oracle. + transcript('a1', blindPrompt(1), { toolCalls: 0 }); + const p = plan({ step45: false }); + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would relaunch a WORKING chunk-1 agent over this one. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: ['chunk 1 — the agent was launched blind'], + }); + expect(r.body).toContain('never named the diff file'); + expect(r.body).not.toContain('the agent was launched blind'); + }); + + it("a rewritten-launch disclosure in the role register does not shadow the caller's relay", () => { + // The rename gives a rewritten-launch disclosure a role publicLabel + // subject: a chunk agent launched wearing a built role prompt matches + // it (`wasDeliveredVerbatim` is line containment), so `disclose` names + // it `reverse audit`. A caller relay spelled in that register must not + // be swallowed by the prefix match — the exemption the idle/blind/ + // unopened loops earn applies at the `cov.disclosures` spread too. + // Pre-fix the relay below was silently lost end-to-end + // (probe-reproduced); the structural line and the scoped relay must + // both render. + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const rolePrompt = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, 'reverse-audit.txt'), rolePrompt); + transcript('v-ra-rewritten', `${rolePrompt}\nYou own chunk 1 of 2.`, { + toolCalls: 2, + range: [0, 100], + opens: [brief], + }); + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would re-record the very step this case means to lack. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + "reverse audit — chunk 1's auditor returned nothing substantive twice", + ], + }); + expect(r.body).toContain( + 'Not reviewed: reverse audit — ran on a prompt the run wrote itself ' + + '(none was built for this chunk), so the brief with its method and ' + + 'rules never reached it.', + ); + expect(r.body).toContain( + "Not reviewed: reverse audit — chunk 1's auditor returned nothing substantive twice.", + ); + }); + + it('a rewritten-launch disclosure keeps its Chinese twin in a bilingual body', () => { + // The rename lets the rewritten-launch disclosures carry a role + // publicLabel subject; the bilingual body's Chinese half must say it + // in Chinese (subjectZh), the way every other rostered entry this + // report plumbs does — pre-fix the zh sentence embedded the English + // label beside proper Chinese siblings (probe-reproduced). + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false, han: true }); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const rolePrompt = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, 'reverse-audit.txt'), rolePrompt); + transcript('v-ra-rewritten', `${rolePrompt}\nYou own chunk 1 of 2.`, { + toolCalls: 2, + range: [0, 100], + opens: [brief], + }); + + // Not base(): its planPath default runs coveredPlan() on the same + // path, which would re-record the very step this case means to lack. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '未审查:反向审计——运行在这次 run 自行编写的 prompt 上(该 chunk 从未构建过 prompt),承载方法与规则的 brief 从未到达该 agent。', + ); + }); + + it('a rostered drifted-launch disclosure names the role in both registers', () => { + // The sibling `b === undefined` disclose site has a relay-shadow test + // and a Chinese-twin test; this site — a BUILT chunk prompt the launch + // drifted from — had neither. A rostered launch (built role prompt plus + // orchestrator chunk phrase, brief unopened) must disclose under the + // role's publicLabel with its zh twin, not the fallback plumbing line. + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false, han: true }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const rolePrompt = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, 'reverse-audit.txt'), rolePrompt); + transcript('v-ra-drifted', `${rolePrompt}\nYou own chunk 1 of 2.`, { + toolCalls: 2, + range: [0, 100], + opens: [], + }); + + // Not base(): its planPath default runs coveredPlan() again on the same + // path and would lay a verbatim reverse-audit pair over this fixture. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + 'Not reviewed: reverse audit — launched with a prompt that is not the one the CLI built.', + ); + expect(r.body).toContain( + '未审查:反向审计——启动时使用的 prompt 不是 CLI 构建的那一份。', + ); + }); + + it('a bare caller echo of an idle rostered subject dedups against the idle entry', () => { + // The bare-echo arm (`d === e.subject`) earns its keep on the exempt + // register: an idle rostered auditor plus a bare relay of its subject + // (no reason after the em-dash) must not render the whiff sentence + // beside the true idle disclosure — a factually wrong claim about the + // run PLUS a double disclosure. Drop the arm and the mutant survives + // every other test; the flip renders the whiff. + transcript('a1', goodPrompt(1), { toolCalls: 3 }); + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const launch = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, 'reverse-audit.txt'), launch); + transcript('v-reverse_audit', launch, { toolCalls: 0 }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: ['reverse audit'], + }); + expect(r.body).toContain( + 'Not reviewed: reverse audit — the agent made no tool call: it read nothing.', + ); + expect(r.body).not.toContain( + 'the agent returned no evidence of its walk twice', + ); + }); + + it('a verbatim re-relay of a coverage sentence dedups in the exempt register too', () => { + // The exemption waives the PREFIX match for role publicLabels — never + // the echo check itself. A relay restating an entry's exact sentence + // carries no new scope (a #7188-style paste of a prior body's "Not + // reviewed" line is exactly this shape); without the exact-sentence + // arm the idle sentence below rendered twice (probe-reproduced). + transcript('a1', goodPrompt(1), { toolCalls: 3 }); + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const launch = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}")`; + writeFileSync(join(d, 'reverse-audit.txt'), launch); + transcript('v-reverse_audit', launch, { toolCalls: 0 }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + unreviewedDimensions: [ + 'reverse audit — the agent made no tool call: it read nothing', + ], + }); + const sentence = + 'Not reviewed: reverse audit — the agent made no tool call: it read nothing.'; + expect(r.body.split(sentence).length - 1).toBe(1); + }); + + it('folds two rounds of one role disclosing the identical budget gap', () => { + // The publicLabel register folds every round of a role onto one agent + // name, so a same-round relaunch that hits the same ceiling and + // re-discloses the identical line arrives as two items that render two + // textually identical clauses — and a duplicate inside the + // MAX_BUDGET_GAP_LINES budget can push a distinct third gap into the + // truncation. (agent, gap) dedup folds them the way the parser folds a + // within-return restatement. + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + const d = promptRecordDir(p); + const brief = briefPath(p, 'reverse-audit'); + writeFileSync(brief, 'The reverse-audit brief.'); + const launch = + `You are review agent \`reverse-audit\`.\n` + + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(d, 'reverse-audit.txt'), launch); + transcript('v-ra-gap-1', launch, { + toolCalls: 2, + range: [0, 100], + text: 'Walked the diff.\nBudget gap: the retry-path walk', + }); + transcript('v-ra-gap-2', launch, { + toolCalls: 2, + range: [0, 100], + text: 'Walked the diff again.\nBudget gap: the retry-path walk', + }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + 'Not explored to full depth (tool budget reached): ' + + 'reverse audit: `the retry-path walk`.', + ); + expect( + r.body.split('reverse audit: `the retry-path walk`').length - 1, + ).toBe(1); + }); + + it('a chunk-agent budget-gap discloser keeps the English name in the zh half', () => { + // The zh sentence's agent name rides `it.agentZh ?? it.agent`: every + // bilingual budget-gap test above uses a rostered discloser carrying a + // zh twin, so the fallback side never executes — a mutant dropping it + // survives them all and prints literal `undefined:` for a + // non-rostered discloser. A chunk agent is exactly that. + transcript('a2', goodPrompt(2), { toolCalls: 2, range: [100, 100] }); + const p = plan({ step45: false, han: true }); + recordBuilt(p, 1); + recordBuilt(p, 2); + recordMatrix(p); + transcript('a1', goodPrompt(1), { + toolCalls: 2, + range: [0, 100], + text: 'No issues found.\nBudget gap: the negative-path walk', + }); + + // Not base(): same planPath-default hazard as the idle case above. + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: p, + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + '未探索到全部深度(达到工具调用预算):chunk 1:`the negative-path walk`。', + ); + expect(r.body).not.toContain('undefined:'); + }); + it('a round-1 budget stop stands alone — no rogue-audit gap, no rebuild FIX', () => { // The gate refused round 1, so no reverse-audit record exists. Without // the marker the floor would report the absence as a rogue/unlaunched @@ -850,7 +1654,12 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve // The round-cap marker still discloses and caps the verdict… expect(r.event).toBe('COMMENT'); expect(r.body).toContain('reverse-audit round cap of 3'); - // …but the not-built gap and its rebuild remediation are still owed. + // …but the not-built gap and its rebuild remediation are still owed — + // the gap in the posted body, not only the FIX on stderr. + expect(r.body).toContain( + 'Not reviewed: reverse audit — no auditor was launched with a prompt ' + + 'this skill builds', + ); expect(r.remediation.join(' ')).toContain('reverse audit:'); }); @@ -917,9 +1726,13 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve 'stopped before round 2 by the review time budget', ); // …and the rewritten round is NOT laundered: the operator channel carries - // its exact repair. (The posted body collapses same-subject disclosures — - // both say "reverse audit" — so the author sees the stop; the rewritten - // repair rides stderr, which is where repairs are acted on.) + // its exact repair, and the posted body renders the floor's rewritten gap + // beside the stop — one subject, two distinct reasons, and the dedup + // keeps both (the rewritten repair itself rides stderr, which is where + // repairs are acted on). + expect(r.body).toContain( + 'Not reviewed: reverse audit — an auditor ran and opened its brief', + ); expect(r.remediation.join(' ')).toContain('reverse audit:'); expect(r.remediation.join(' ')).toContain('EXACTLY what it prints'); }); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 64b7ff5f709..24a462624e9 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -92,6 +92,14 @@ export type ReviewEvent = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'; */ export const LOW_SIGNAL_SRC_DIFF_LINES = 100; +/** + * The bare chunk subject, exactly as coverage's labels and the uncoverable + * push below spell it. One predicate for every site that must tell a chunk + * id from prose: the richer caller form `chunk 5 (src/big.min.js)` is + * prose, and deliberately does NOT match. + */ +const CHUNK_SUBJECT_RE = /^chunk (\d+)$/; + /** * Reads a PR's description body, given its `owner/repo` and number. The one * production implementation calls `gh pr view`; the bilingual fallback uses it @@ -609,7 +617,13 @@ function composeReviewBody( // 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. + // likeliest. The same holds for every entry named by a role's + // publicLabel — idle/unopened agents below, the Step 4/5 floor further + // down — so they join one exemption set; a bare subject echo still + // dedups. Chunk and internal-subject entries keep the prefix match: a + // caller echoing those is pasting the gate's own line (#7188), and the + // coverage-derived text wins the collision. + const roleLabelEntries = new Set<(typeof coverageEntries)[number]>(); let budgetEntry: (typeof coverageEntries)[number] | undefined; if (input.planPath) { const stop = readBudgetStop(input.planPath); @@ -631,6 +645,7 @@ function composeReviewBody( ) : budgetStopDisclosure(stop.round ?? undefined); coverageEntries.push(budgetEntry); + roleLabelEntries.add(budgetEntry); } } // The fixes for the gaps above, for stderr — never for the body. The gap says @@ -676,6 +691,11 @@ function composeReviewBody( // zero-certified test falls to the `coverage` disclosure instead. let plannedChunks: Array<{ id: number; files: string[] }> = []; let coveredChunks: number[] = []; + // The Chinese twins of rostered public labels, keyed by the label — + // coverage resolves them once, at the single name-derivation point; the + // idle/unopened/blind entries and the budget-gap agent names below look + // them up for the bilingual body. + let publicLabelsZh: Record = {}; // The deterministic script-lint gate. `compose-review` is the authority here: // it reads the report the orchestrator's `qwen review script-lint` step wrote @@ -760,6 +780,7 @@ function composeReviewBody( const cov = coverageFromTranscripts(input.planPath, input.env); plannedChunks = cov.plannedChunks; coveredChunks = cov.coveredChunks; + publicLabelsZh = cov.publicLabelsZh; for (const id of cov.missingChunks) missingReceipts.push(id); for (const id of cov.uncoverableChunks) { // The caller may already have named this chunk, but in a richer form: @@ -773,11 +794,18 @@ function composeReviewBody( if (!already) uncoverable.push(prefix); } for (const label of cov.idleAgents) { - coverageEntries.push({ + const entry = { subject: label, reason: 'the agent made no tool call: it read nothing', reasonZh: '该 agent 未发起任何工具调用:它什么都没读', - }); + subjectZh: publicLabelsZh[label], + }; + coverageEntries.push(entry); + // A chunk agent's idle label is `chunk N` and a non-rostered + // launch keeps its fallback label — both are internal register and + // keep the #7188 prefix dedup; only ROSTERED labels (the caller's + // own relay register) join the exemption (see above). + if (publicLabelsZh[label] !== undefined) roleLabelEntries.add(entry); } if (cov.idleAgents.length > 0) { remediation.push( @@ -794,13 +822,21 @@ function composeReviewBody( // this line: the line lands in the posted body, and `qwen review // agent-prompt` is not something a PR author can run. for (const label of cov.blindAgents) { - coverageEntries.push({ + const entry = { subject: label, reason: 'launched with a prompt that never named the diff file, so it ' + 'could not have read it', reasonZh: '启动 prompt 从未提到 diff 文件,它不可能读过 diff', - }); + subjectZh: publicLabelsZh[label], + }; + coverageEntries.push(entry); + // Mirror the idle/unopened loops: the rename means a blind agent + // can carry a role publicLabel (an orchestrator-inserted chunk + // phrase that matched a readsDiff-false role), and a caller relay + // spelled in that register must not be swallowed by the prefix + // match (see `roleLabelEntries`). + if (publicLabelsZh[label] !== undefined) roleLabelEntries.add(entry); } if (cov.blindAgents.length > 0) { remediation.push( @@ -815,14 +851,17 @@ function composeReviewBody( // spent its run somewhere else, which on a diff with deletions means it // reviewed a file the removed lines are simply not in. for (const label of cov.unopenedAgents) { - coverageEntries.push({ + const entry = { subject: label, reason: 'pointed at diff lines it never opened: it made tool calls, but ' + 'none of them read the diff', reasonZh: '它被指向 diff 的行却从未打开:有工具调用,但没有一次读取 diff', - }); + subjectZh: publicLabelsZh[label], + }; + coverageEntries.push(entry); + if (publicLabelsZh[label] !== undefined) roleLabelEntries.add(entry); } if (cov.unopenedAgents.length > 0) { remediation.push( @@ -842,7 +881,16 @@ function composeReviewBody( // rewritten, missing-role and unread-brief entries arrive structurally // (`cov.disclosures`, push order preserved) — their labels can carry // em-dashes of their own, which is why they are never reparsed here. - coverageEntries.push(...cov.disclosures); + for (const e of cov.disclosures) { + coverageEntries.push(e); + // The rename lets a rewritten-launch disclosure carry a role + // publicLabel as its subject; a caller relay spelled in that + // register must not be swallowed by the prefix match, so it joins + // the exemption exactly the way the idle/blind/unopened entries do. + // missingRoles/unreadBriefs entries keep internal subjects that + // never key into `publicLabelsZh` and keep the prefix dedup. + if (publicLabelsZh[e.subject] !== undefined) roleLabelEntries.add(e); + } if (cov.rewrittenPrompts.length > 0) { remediation.push( 'rewritten launches: re-run `"${QWEN_CODE_CLI:-qwen}" review ' + @@ -922,12 +970,16 @@ function composeReviewBody( // Structural, both languages — no boundary is recovered from rendered // prose (reparsing was the bug the disclosure entries already fixed). for (const gap of verification.gaps) { - coverageEntries.push({ + const entry = { subject: gap.subject, reason: gap.reason, subjectZh: gap.subjectZh, reasonZh: gap.reasonZh, - }); + }; + coverageEntries.push(entry); + // Step 4/5 gap subjects are role publicLabels — the exemption + // register (see `roleLabelEntries`). + roleLabelEntries.add(entry); } remediation.push(...verification.remediation); criticalsUnverified = @@ -1221,7 +1273,7 @@ function composeReviewBody( const bareIds: number[] = []; const callerNamed: string[] = []; for (const e of uncoverable) { - const m = /^chunk (\d+)$/.exec(e); + const m = CHUNK_SUBJECT_RE.exec(e); if (m) bareIds.push(Number(m[1])); else callerNamed.push(e); } @@ -1253,15 +1305,19 @@ function composeReviewBody( for (const d of unreviewed) { if (seenCaller.has(d)) continue; // a caller pasting itself twice seenCaller.add(d); - // The budget-stop entry never prefix-matches: its relays are already - // deduped by the marker phrase above, and letting its `reverse audit` - // subject claim the prefix swallowed unrelated reverse-audit scopes the - // caller disclosed with their own reasons (a bare subject echo still - // dedups). + // Role-publicLabel subjects never prefix-match (`roleLabelEntries`): + // a relay that begins with one and brings its own reason after the + // em-dash is a SCOPE those entries do not explain, and the skill + // promises such prose renders verbatim. A bare subject echo still + // dedups, and so does a verbatim re-relay of an entry's own sentence — + // it carries no new scope, and a #7188-style paste of a prior body's + // line is exactly that shape. const echoesCoverage = covEntries.some( (e) => d === e.subject || - (e !== budgetEntry && d.startsWith(`${e.subject} — `)), + d === + `${e.publicSubject ?? e.subject} — ${e.publicReason ?? e.reason}` || + (!roleLabelEntries.has(e) && d.startsWith(`${e.subject} — `)), ); if (!echoesCoverage) callerLeft.push(d); } @@ -1297,9 +1353,27 @@ function composeReviewBody( // and the body must not say it twice in two registers. These are // "stopped at the budget", not "nobody looked": the phrasing must not // claim the stronger gap, and the entries do not join the capping lists. - const budgetGapItems: Array<{ agent: string; gap: string }> = []; + const budgetGapItems: Array<{ + agent: string; + agentZh?: string; + gap: string; + }> = []; + // The publicLabel register folds every round of a role onto one agent + // name, so two rounds disclosing the identical gap arrive as two items + // that would render two textually identical clauses — fold them the way + // the parser folds a within-return restatement. + const seenBudgetGapItems = new Set(); for (const g of budgetGapNotes) { - for (const gap of g.gaps) budgetGapItems.push({ agent: g.agent, gap }); + for (const gap of g.gaps) { + const dedupKey = `${g.agent}\u0000${gap}`; + if (seenBudgetGapItems.has(dedupKey)) continue; + seenBudgetGapItems.add(dedupKey); + budgetGapItems.push({ + agent: g.agent, + agentZh: publicLabelsZh[g.agent], + gap, + }); + } } const keptBudgetGaps = budgetGapItems.filter( (it) => !unreviewed.some((d) => d.includes(it.gap)), @@ -1311,8 +1385,9 @@ function composeReviewBody( shown.map((it) => `${it.agent}: ${mdField(it.gap)}`).join('; ') + (more > 0 ? `, and ${more} more` : ''); const zhList = - shown.map((it) => `${it.agent}:${mdField(it.gap)}`).join(';') + - (more > 0 ? `,另有 ${more} 条` : ''); + shown + .map((it) => `${it.agentZh ?? it.agent}:${mdField(it.gap)}`) + .join(';') + (more > 0 ? `,另有 ${more} 条` : ''); notReviewedParts.push({ en: `Not explored to full depth (tool budget reached): ${enList}.`, zh: `未探索到全部深度(达到工具调用预算):${zhList}。`, @@ -1323,38 +1398,47 @@ function composeReviewBody( // paragraphs — a posted body on #7166 was ninety-nine clauses over four // causes, the six real findings buried beneath. Grouped by the reason // STRING, so a reason embedding per-subject detail (an unread brief\'s own - // path) differs per entry and keeps its own line. One subject that appears - // under two causes keeps the FIRST — the categories push in precision - // order, and a chunk flagged `rewritten` is also, to the roster, a - // requirement with no verbatim launch; repeating it under the later, vaguer - // cause would tell the author "no agent was launched" about an agent that - // demonstrably ran. - const seenSubjects = new Set(); + // path) differs per entry and keeps its own line. + const seenEntries = new Set(); const byReason = new Map< string, Array<{ subject: string; publicSubject?: string; subjectZh?: string }> >(); const reasonZhOf = new Map(); for (const e of covEntries) { - if (seenSubjects.has(e.subject)) continue; - seenSubjects.add(e.subject); // Keyed on the reason the body will PRINT — public over internal. Two // unread briefs differ internally only by their brief paths; grouped on // those, the path-free public sentence would render once per role, which // is the per-subject repetition this map exists to kill. - const key = e.publicReason ?? e.reason; - const group = byReason.get(key) ?? []; + const reasonKey = e.publicReason ?? e.reason; + // Deduped on the subject AND the printed reason, not the subject alone: + // the publicLabel register is shared by every round and shard of a role, + // so one subject can owe several distinct disclosures — a budget stop + // and an idle round both render as `reverse audit`, and a subject-only + // shadow silently dropped the second, in the channel whose whole promise + // is the disclosure. True duplicates (one subject, one printed reason) + // still collapse. Bare chunk subjects keep the first-cause-wins shadow: + // the chunk categories derive from one another for a single chunk, and + // the later, vaguer cause would say "no agent was launched" about a + // chunk an earlier line already showed launched — the precise cause + // keeps the subject. + const dedupKey = CHUNK_SUBJECT_RE.test(e.subject) + ? e.subject + : `${e.subject}\u0000${reasonKey}`; + if (seenEntries.has(dedupKey)) continue; + seenEntries.add(dedupKey); + const group = byReason.get(reasonKey) ?? []; group.push({ subject: e.subject, publicSubject: e.publicSubject, subjectZh: e.subjectZh, }); - byReason.set(key, group); + byReason.set(reasonKey, group); // One printed reason, one translation: entries sharing the printed // English reason share the Chinese one by construction (both derive from // the same source string). Entries with none fall back to the English. - if (e.reasonZh !== undefined && !reasonZhOf.has(key)) { - reasonZhOf.set(key, e.reasonZh); + if (e.reasonZh !== undefined && !reasonZhOf.has(reasonKey)) { + reasonZhOf.set(reasonKey, e.reasonZh); } } for (const [reason, entries] of byReason) { @@ -1371,7 +1455,7 @@ function composeReviewBody( const named: string[] = []; const namedZh: string[] = []; for (const e of entries) { - const m = /^chunk (\d+)$/.exec(e.subject); + const m = CHUNK_SUBJECT_RE.exec(e.subject); if (m) chunkIds.push(Number(m[1])); else { named.push(e.publicSubject ?? e.subject); @@ -1582,7 +1666,7 @@ function composeReviewBody( // certified. const disclosedChunkIds = new Set(); for (const e of coverageEntries) { - const m = /^chunk (\d+)$/.exec(e.subject); + const m = CHUNK_SUBJECT_RE.exec(e.subject); if (m) disclosedChunkIds.add(Number(m[1])); } const nothingCertified = diff --git a/packages/cli/src/commands/review/lib/budget.test.ts b/packages/cli/src/commands/review/lib/budget.test.ts index b252fd41548..c1fc68d8908 100644 --- a/packages/cli/src/commands/review/lib/budget.test.ts +++ b/packages/cli/src/commands/review/lib/budget.test.ts @@ -331,12 +331,94 @@ describe('budgetGapDisclosures — the one parser of the disclosure format', () 'Budget gap: nothing to report', 'Budget gap: no gaps found', 'Budget gap: none ( all checks completed)', + // Bracket-wrapped non-answers reached a real posted body: the + // wrapping parenthesis defeated the leading-token match. + 'Budget gap: (none)', + 'Budget gap: [N/A]', + // Punctuation-only placeholders wrapped in brackets, with and + // without a trailing period — pinned so a later cleanup cannot + // delete the trailing-strip and flip these into phantom gaps. + 'Budget gap: (-)', + 'Budget gap: (-).', + 'Budget gap: .(-)', + // Quote-wrapped non-answers, straight and smart. + 'Budget gap: "none"', + "Budget gap: 'none'", + 'Budget gap: ‘none’', + 'Budget gap: {N/A}', + 'Budget gap: “none”', + 'Budget gap: "N/A"', + // Fullwidth/CJK wrappers — the disclosure marker is bilingual, and a + // bilingual agent's `(none)` defeated the classifier exactly the way + // `(none)` did before the wrapper pairs joined the strip. + 'Budget gap: (none)', + 'Budget gap: 【N/A】', + 'Budget gap: 「none」', + 'Budget gap: 『none』', + 'Budget gap: 《none》', + 'Budget gap: 〈none〉', + 'Budget gap: 〔N/A〕', + 'Budget gap: [none]', + 'Budget gap: {N/A}', + // Fullwidth terminal punctuation rides OUTSIDE the wrappers the same + // way a halfwidth period does — `(none)。` must lose the period + // before the wrapper strip can see the pair, or the phantom leaks + // through into the posted body's budget-gap sentence. + 'Budget gap: (none)。', + 'Budget gap: 「none」。', + 'Budget gap: 【N/A】。', + 'Budget gap: 〈none〉。', + 'Budget gap: "none"。', + 'Budget gap: none。', + 'Budget gap: none.', + // Interrogative non-answers, fullwidth and its halfwidth twin: + // `none?` is the same non-answer, and a bare token in ANY trailing + // punctuation drops. + 'Budget gap: none?', + 'Budget gap: none?', + 'Budget gap: (none?)', + 'Budget gap: "none?"', + 'Budget gap: N/A?', + // The marker itself is bilingual: the line starts at the CJK marker. + '预算缺口:(none)。', + // List markers from the brief's bullet format wrap placeholders the + // same way brackets do. + 'Budget gap: - none', + 'Budget gap: — none', + 'Budget gap: * none', + 'Budget gap: _ none', + 'Budget gap: - None.', + // Empty wrapper pairs: nothing inside a balanced pair is nothing. + 'Budget gap: ""', + 'Budget gap: ()', + 'Budget gap: []', + 'Budget gap: {}', + "Budget gap: ''", + 'Budget gap: “”', 'Budget gap:', ]) { expect(budgetGapDisclosures(line)).toEqual([]); } }); + it('keeps a bracket-wrapped gap that names a real check — wrappers and all', () => { + // The survival half of the edge-strip, which the negatives above cannot + // pin: a drop-if-wrapped mutant passes every negative case yet silently + // discloses nothing here, in the channel where a lost gap is + // unobservable downstream. The pushed gap keeps `raw`, wrappers + // included — that is what makes this assertion discriminate. + expect( + budgetGapDisclosures('Budget gap: (retry path) the remaining callers'), + ).toEqual(['(retry path) the remaining callers']); + // … and the same holds marker-wrapped: the brief hands agents a bullet + // format, so `- ` is plausible input. A drop-if-marker-leading + // mutant passes every negative above yet silently discards this — the + // loss of a real disclosure shipping green. + expect( + budgetGapDisclosures('Budget gap: - the auth flow is untested'), + ).toEqual(['- the auth flow is untested']); + }); + it('keeps a REAL gap in parentheses — the paren strip fires only for placeholders', () => { // The strip exists for `(none — all planned checks completed)`; a // genuine parenthesized disclosure must survive it … diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index 3047b471bea..12f1cb3969f 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -455,16 +455,61 @@ function stripWrappers(s: string): string { return out; } -const TRAILING_GAP_CHAR_RE = /[.!…,;:\s]/; +// Edge characters also include list markers (`- none`, `* none`): the +// budget brief hands agents a bullet format, so a placeholder can arrive +// marker-wrapped exactly the way it arrives bracket-wrapped. The +// fullwidth terminals join for the same reason the fullwidth wrappers do: +// `(none)。` must lose its period before the wrapper strip can see the pair. +const TRAILING_GAP_CHAR_RE = /[-—–*_.!?…,;:。.,!?;:\s]/; + +/** Wrapping bracket/quote pairs stripped only SYMMETRICALLY. */ +const GAP_WRAPPER_CLOSES: Record = { + '(': ')', + '[': ']', + '{': '}', + '"': '"', + "'": "'", + '“': '”', + '‘': '’', + // The disclosure marker is bilingual, and so are the wrappers a + // bilingual agent reaches for — `(none)` defeated the classifier + // exactly the way `(none)` did before these joined. + '(': ')', + '【': '】', + '「': '」', + '『': '』', + '《': '》', + '〈': '〉', + '〔': '〕', + '[': ']', + '{': '}', +}; -/** Trailing punctuation/whitespace strip for the normalize and fold keys. */ -function stripTrailingGapChars(s: string): string { - // Walked backwards rather than replaced with an end-anchored class run: - // that shape backtracks quadratically when a long run fails to reach - // the end. +/** + * Edge punctuation and balanced wrapper pairs stripped to a fixpoint, for + * the normalize and fold keys: punctuation rides OUTSIDE wrappers (`(-).` + * must lose its period AND its bracket), so a fixed-order single pass per + * side strands one of the layers. The balance keeps an inner completion + * clause's closing paren intact for the classifier's parenthesized shape. + * An empty wrapper pair strips to nothing — nothing inside a balanced + * pair is nothing, and a bare `()` or `""` cannot be ruled on as a gap. + */ +function stripGapWrappers(s: string): string { + // Two-pointer (walk both ends inward) rather than end-anchored class + // runs — this parse runs on every agent return, so nothing that could + // backtrack quadratically when a long run fails to reach the edge. + let start = 0; let end = s.length; - while (end > 0 && TRAILING_GAP_CHAR_RE.test(s.charAt(end - 1))) end--; - return s.slice(0, end); + for (;;) { + while (start < end && TRAILING_GAP_CHAR_RE.test(s.charAt(start))) start++; + while (end > start && TRAILING_GAP_CHAR_RE.test(s.charAt(end - 1))) end--; + if (end - start < 2) break; + const close = GAP_WRAPPER_CLOSES[s.charAt(start)]; + if (close === undefined || s.charAt(end - 1) !== close) break; + start++; + end--; + } + return s.slice(start, end); } /** Truncate on code points — a slice through a surrogate pair is mojibake. */ @@ -516,20 +561,19 @@ export function budgetGapDisclosures(finalText: string): string[] { raw = raw.slice(0, -1); } raw = stripWrappers(raw.trim()).trim(); - 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(')') - ? normalized.slice(1, -1).trim() - : normalized; - if (normalized.length === 0 || PLACEHOLDER_GAP_RE.test(unparenthesized)) { + // The normalized form sheds edge punctuation and wrapping brackets, + // quotes, and parens — balanced pairs only, never one side: `(none)` + // and `"none"` reached a real posted body as phantom gaps because the + // wrapping defeated the match below. Judged bare and wrapped alike, by + // the one strict classifier — its doc names why the shapes are narrow. + const normalized = stripGapWrappers(raw); + if (normalized.length === 0 || PLACEHOLDER_GAP_RE.test(normalized)) { 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 wrapper-stripped text, so one gap restated with and + // without wrappers — `(auth flow untested.)` and `auth flow untested` + // — discloses once. + const key = normalized.toLowerCase(); if (seen.has(key)) continue; seen.add(key); gaps.push(truncateGap(raw)); diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 883de9cc898..c4ee68a3e3e 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -166,6 +166,15 @@ export interface CoverageFromTranscripts { * hit its ceiling mid-check. */ budgetGaps: Array<{ agent: string; gaps: string[] }>; + /** + * The Chinese twins of the rostered public labels in this report, keyed + * by the label — for the Chinese half of a bilingual posted body. Only + * labels resolved through a brief's `publicLabel` carry a twin; a + * fallback label (a truncated launch line) has none, and the renderer + * falls back to the English text, the way it does for every subject + * without a Chinese variant. + */ + publicLabelsZh: Record; /** Chunk ids a working agent actually reviewed. */ coveredChunks: number[]; /** @@ -386,6 +395,11 @@ export function coverageFromTranscripts( const blindAgents: string[] = []; const idleAgents: string[] = []; const unopenedAgents: string[] = []; + // The Chinese twins of every rostered public label resolved below, keyed + // by the label — the bilingual body looks them up for the idle/unopened/ + // blind entries and the budget-gap agent names, the way the Step 4/5 + // floor gaps and the budget-stop marker carry theirs inline. + const publicLabelsZh: Record = {}; const rewrittenPrompts: string[] = []; const driftedLaunches: string[] = []; // Did this record's agent open the brief recorded under `key`? Compared as a @@ -501,6 +515,41 @@ export function coverageFromTranscripts( } return g; }; + // The rostered role a record's launch was built for, said as the brief's + // publicLabel — the register the posted body renders disclosures in — + // with its Chinese twin for the bilingual body. `null` when the launch + // matches no built role prompt (a chunk agent's label is already its + // chunk; anything else keeps the fallback). + const rosteredLabel = ( + rec: AgentRecord, + ): { label: string; labelZh?: string } | null => { + for (const key of built.keys()) { + const b = builtOf(key); + if (b === undefined || !wasDeliveredVerbatim(rec.launchPrompt, b)) { + continue; + } + // Record keys carry run suffixes (`reverse-audit--round-N--`, + // `verify--`); BRIEFS is keyed by the bare role. + const brief = BRIEFS[key.split('--')[0] as keyof typeof BRIEFS]; + if (!brief?.publicLabel) continue; + // A file-scoped launch (an invariant agent per heavy file) keeps its + // file, resolved through the ROSTER requirement rather than a key + // split: only invariant agents are rostered per file, the roster's + // key is spelled exactly the way `agent-prompt` records it, and a + // split would misread `verify--` or `reverse-audit--chunk-N` + // as file-scoped, leaking a digest or a chunk id onto the PR page. + // Without the file, N per-file agents of one role would read as one + // repeated line — the author could not tell which file's check + // stopped. + const req = rosterForRun.find((r) => r.key === key); + return { + label: (req && publicRoleLabel(req)) || brief.publicLabel, + labelZh: (req && publicRoleLabelZh(req)) || brief.publicLabelZh, + }; + } + return null; + }; + // A record's gaps are silenced only by a GAP-FREE superseding record — a // genuine repair. Two relaunches that both hit the ceiling and both // disclose would otherwise supersede each other and drop every gap. @@ -553,7 +602,16 @@ export function coverageFromTranscripts( for (const rec of records) { const chunk = assignedChunk(rec); - const name = label(rec, chunk); + // Resolved ONCE, at the single name-derivation point, so every report + // category below rides it: the fallback label is the launch prompt's + // first line, and a real posted body rendered a disclosure as "You are + // review agent `reverse-audit` — Reverse audit agen...:" — the run's + // own plumbing, truncated, on a public PR page. + const rostered = rosteredLabel(rec); + const name = rostered?.label ?? label(rec, chunk); + if (rostered?.labelZh !== undefined) { + publicLabelsZh[rostered.label] = rostered.labelZh; + } // Could this agent have read the diff at all? The prompt is the harness's // record of what was asked of it. 23 of 23 real chunk agents were launched @@ -603,6 +661,10 @@ export function coverageFromTranscripts( 'ran on a prompt the run wrote itself (none was built for this ' + 'chunk), so the brief with its method and rules never reached it', { + // The rename can make `name` a rostered publicLabel; the + // bilingual body needs its twin, the way every other + // rostered entry this report plumbs carries one. + subjectZh: rostered?.labelZh, reasonZh: '运行在这次 run 自行编写的 prompt 上(该 chunk 从未构建过 ' + 'prompt),承载方法与规则的 brief 从未到达该 agent', @@ -630,7 +692,10 @@ export function coverageFromTranscripts( disclose( name, 'launched with a prompt that is not the one the CLI built', - { reasonZh: '启动时使用的 prompt 不是 CLI 构建的那一份' }, + { + subjectZh: rostered?.labelZh, + reasonZh: '启动时使用的 prompt 不是 CLI 构建的那一份', + }, ), ); } @@ -974,6 +1039,7 @@ export function coverageFromTranscripts( missingChunks, uncoverableChunks: [...uncoverable].sort((a, b) => a - b), budgetGaps, + publicLabelsZh, coveredChunks: [...covered].sort((a, b) => a - b), plannedChunks: plan.chunks.map((c) => ({ id: c.id,