Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions packages/cli/src/commands/review/check-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1607,4 +1607,53 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
/verification — its prompt was built, but no agent was launched with it/,
);
});

it('merges both steps into one gap when they failed the same way', () => {
// #7268: the posted body carried the verify and reverse-audit `rewritten`
// sentences back to back, near-identical but for the tail. One shape, one
// sentence, two subjects — and still both consequences and both honesty
// limits (each demonstrably RAN and opened its brief).
const p = plan();
step45(p, 'reverse-audit', { rewritten: true });
step45(p, 'verify', { rewritten: true });
const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.ok).toBe(false);
expect(r.gaps).toHaveLength(1);
const gap = r.gaps[0];
expect(gap).toMatch(/^verification and reverse audit — /);
expect(gap).toMatch(/each ran and opened its brief/);
expect(gap).toMatch(/written by hand/);
expect(gap).toMatch(/cannot be counted as verified/);
// The remediation stays per-role: the two rebuild commands differ.
const fix = r.remediation.join(' ');
expect(fix).toContain('--role reverse-audit');
expect(fix).toContain('--role verify');
expect(r.unverifiedFindings).toBe(true);
});

it('keeps two precise gaps when the steps failed differently', () => {
// Mixed shapes have different mechanisms and different fixes; a sentence
// vague enough to cover both would misname one of them.
const p = plan();
step45(p, 'reverse-audit', { rewritten: true });
step45(p, 'verify', { launch: false });
const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.gaps).toHaveLength(2);
expect(r.gaps.join(' ')).toMatch(
/reverse audit — an auditor ran and opened its brief/,
);
expect(r.gaps.join(' ')).toMatch(
/verification — its prompt was built, but no agent was launched with it/,
);
expect(r.gaps.join(' ')).not.toMatch(/verification and reverse audit/);
});

it('does not merge when the review posts no findings — verify was never owed', () => {
// A zero-finding review with the reverse audit skipped keeps the solo
// reverse-audit text: there is no verify failure to share a sentence with.
const p = plan(); // neither step on record
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.gaps).toHaveLength(1);
expect(r.gaps[0]).toMatch(/^reverse audit — /);
});
});
42 changes: 37 additions & 5 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1126,9 +1126,11 @@ describe('coverage is recomputed, never accepted', () => {
'a subject only the caller noticed — the auditor returned nothing twice',
],
});
// One clause for the shared subject — and it is the machine's sentence,
// not the caller's paraphrase.
expect(r.body.split(label)).toHaveLength(2);
// One clause for the shared subject — the machine's sentence, not the
// caller's paraphrase, and in the author's register: the internal
// codename stays off the posted body (it is the stderr selector).
expect(r.body.split('the whole-diff test-coverage check')).toHaveLength(2);
expect(r.body).not.toContain(label);
expect(r.body).toContain('no record shows its brief reaching an agent');
expect(r.body).not.toContain('described this gap in its own words');
// A subject the coverage recomputation cannot see survives untouched.
Expand Down Expand Up @@ -1250,7 +1252,10 @@ describe('coverage is recomputed, never accepted', () => {
],
modelId: MODEL,
});
expect(r.body.split(label)).toHaveLength(2);
// The caller's echo (internal label) dedupes against the internal
// subject; the one surviving sentence prints the author's phrase.
expect(r.body).not.toContain(label);
expect(r.body.split('the whole-diff test-coverage check')).toHaveLength(2);
expect(
r.body.match(/no record shows its brief reaching an agent/g) ?? [],
).toHaveLength(1);
Expand Down Expand Up @@ -1510,8 +1515,12 @@ describe('coverage is recomputed, never accepted', () => {
expect(fixes).toMatch(/rewritten launches: re-run/);
expect(fixes).toMatch(/unread briefs: relaunch/);
expect(fixes).toMatch(/agents that never opened the diff: relaunch/);
// And none of the three disclosures drags a command into the body.
// And none of the three disclosures drags a command into the body —
// nor the unread brief's filesystem path: the path names the file an
// OPERATOR makes the agent open, and it stays on stderr with the fix.
expect(r.body).not.toMatch(/agent-prompt|--roster|--chunk/);
expect(r.body).not.toContain('.brief.md');
expect(r.body).toContain('never opened its brief, so it reviewed without');
});

it('the handler prints every FIX to stderr, before the verdict, never to stdout', () => {
Expand Down Expand Up @@ -1630,6 +1639,29 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff
);
});

it('says one sentence when verify and the reverse audit failed the same way', () => {
// #7268's posted body carried the two `rewritten` sentences back to back,
// near-identical but for the tail. Both steps down the same way is one
// failure with two subjects — while the stderr remediation keeps BOTH
// rebuild commands, which differ.
const r = composeReview({
criticalsInline: 0,
suggestionsInline: 1,
planPath: coveredPlan([]), // neither verify nor reverse audit on record
env: ENV,
modelId: MODEL,
});
expect(r.event).toBe('COMMENT');
expect(r.body).toMatch(
/Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds/,
);
expect(r.body).not.toMatch(/reverse audit — no auditor/);
expect(r.body).not.toMatch(/verification — the review posts findings/);
const fixes = r.remediation.join(' ');
expect(fixes).toContain('--role reverse-audit');
expect(fixes).toContain('--role verify');
});

it('softens an unverified Request changes to Comment — no verifier, no blocker', () => {
// This test used to pin the opposite: "a confirmed Critical still blocks —
// a cap never softens a REQUEST_CHANGES". The never-soften rule presumes
Expand Down
47 changes: 32 additions & 15 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,15 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult {
// The coverage-derived disclosures, kept STRUCTURAL ({subject, reason})
// from the site that knows the boundary — reparsing the rendered prose for
// it was the bug. `unreviewed` above stays what the caller wrote, verbatim.
const coverageEntries: Array<{ subject: string; reason: string }> = [];
// The `public*` fields are the body's register (`Brief.publicLabel`, a
// path-free reason); `subject`/`reason` stay the internal keys every dedup
// and certification check below matches on.
const coverageEntries: Array<{
subject: string;
reason: string;
publicSubject?: string;
publicReason?: string;
}> = [];
// The fixes for the gaps above, for stderr — never for the body. The gap says
// what the review cannot certify, to the PR author; the remediation names the
// command that repairs it, to the orchestrator. #7012's public body was fourteen
Expand Down Expand Up @@ -678,29 +686,38 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult {
// cause would tell the author "no agent was launched" about an agent that
// demonstrably ran.
const seenSubjects = new Set<string>();
const byReason = new Map<string, string[]>();
for (const { subject, reason } of covEntries) {
if (seenSubjects.has(subject)) continue;
seenSubjects.add(subject);
const subjects = byReason.get(reason) ?? [];
subjects.push(subject);
byReason.set(reason, subjects);
const byReason = new Map<
string,
Array<{ subject: string; publicSubject?: string }>
>();
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) ?? [];
group.push({ subject: e.subject, publicSubject: e.publicSubject });
byReason.set(key, group);
}
for (const [reason, subjects] of byReason) {
for (const [reason, entries] of byReason) {
// Chunk subjects leave in the author's units, not the run's. `chunk 28`
// is bookkeeping — the id selects a rebuild command on stderr, and
// nothing on the PR page maps it to code. #7268's posted body enumerated
// all 49 of them, unsorted, across two of these sentences; the author's
// units are their files and, at the limit, the diff itself, which is what
// `describeChunkGap` renders. Role labels stay verbatim: they were
// written to be read (`roleLabel`), and reworking them here would fork
// the register the stderr twin shares.
// `describeChunkGap` renders. Role subjects ride their `publicSubject`
// (`Brief.publicLabel`) — the codename stays on stderr, where it is the
// selector — and the partition below keys on the INTERNAL subject, so a
// public phrase can never shadow a chunk id out of the chunk collapse.
const chunkIds: number[] = [];
const named: string[] = [];
for (const s of subjects) {
const m = /^chunk (\d+)$/.exec(s);
for (const e of entries) {
const m = /^chunk (\d+)$/.exec(e.subject);
if (m) chunkIds.push(Number(m[1]));
else named.push(s);
else named.push(e.publicSubject ?? e.subject);
}
const shown = [
...(chunkIds.length > 0
Expand Down
30 changes: 30 additions & 0 deletions packages/cli/src/commands/review/lib/agent-briefs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,17 @@ export type RoleId =
export interface Brief {
/** How the role is named to a human reading a coverage failure. */
label: string;
/**
* How the role is named in the POSTED review body — the author's register.
*
* `label` above carries the run's own codename (`Agent 1c: …`), which is the
* selector an operator acts on and means nothing on a PR page; #7550 moved
* chunk ids out of the posted body for exactly that reason, and this field
* does the same for role names: the dimension, said as what it checks.
* Distinct per role — two roles sharing a phrase would merge under one
* subject in a grouped disclosure.
*/
publicLabel: string;
/**
* Does a path rule belong in this agent's brief?
*
Expand Down Expand Up @@ -118,6 +129,7 @@ export interface Brief {
export const BRIEFS: Record<RoleId, Brief> = {
'0': {
label: 'Agent 0: Issue fidelity & root-cause ownership',
publicLabel: 'the linked-issue fidelity pass',
readsDiff: true,
brief: `You are **Agent 0: Issue Fidelity & Root-Cause Ownership**. Your scope is issue fidelity, not general code review — do not report ordinary code defects; other agents own those.

Expand All @@ -140,6 +152,7 @@ If \`gh\` fails (auth, rate limit, network), **retry that fetch once**. If it fa
'1a': {
reviewsCode: true,
label: 'Agent 1a: Line-by-line correctness',
publicLabel: 'the line-by-line correctness pass',
readsDiff: true,
brief: `You are **Agent 1a: the line-by-line scan**. Your dimension is defined by *how you walk*, not by a topic — a topical "find correctness bugs" brief makes every agent converge on the same visibly-suspicious hunks, which is redundancy, not coverage.

Expand All @@ -157,6 +170,7 @@ Scope guard: reading the enclosing function is for **context**. A defect entirel
'1b': {
reviewsCode: true,
label: 'Agent 1b: Removed-behavior audit',
publicLabel: 'the removed-behavior audit',
readsDiff: true,
brief: `You are **Agent 1b: the removed-behavior audit**. You own the diff's **deleted side**, and you are the only agent who can see it: the \`-\` lines exist *only* in the diff. The post-change tree carries no trace of what was removed — the line is simply not there, and nothing marks where it was — so no agent reading the new code alone can find this class of defect.

Expand All @@ -174,6 +188,7 @@ Each failure scenario must name what input or state now slips past the removed b
'1c': {
reviewsCode: true,
label: 'Agent 1c: Cross-file tracer',
publicLabel: 'the cross-file consistency pass',
readsDiff: true,
brief: `You are **Agent 1c: the cross-file tracer**. You own the *whole* cross-file walk, end to end. It used to be a duty shared by six agents, and a duty shared by six agents is a duty nobody finishes while the same symbols get grepped six times.

Expand All @@ -200,6 +215,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th
'2': {
reviewsCode: true,
label: 'Agent 2: Security',
publicLabel: 'the security pass',
readsDiff: true,
brief: `You are **Agent 2: Security**. Review the diff for:

Expand All @@ -216,6 +232,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th
'3': {
reviewsCode: true,
label: 'Agent 3: Code quality',
publicLabel: 'the code-quality pass',
readsDiff: true,
brief: `You are **Agent 3: Code Quality**. Review the diff for:

Expand All @@ -229,6 +246,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th
'4': {
reviewsCode: true,
label: 'Agent 4: Performance & efficiency',
publicLabel: 'the performance pass',
readsDiff: true,
brief: `You are **Agent 4: Performance & Efficiency**. Review the diff for:

Expand All @@ -243,6 +261,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th
'5': {
reviewsCode: true,
label: 'Agent 5: Test coverage',
publicLabel: 'the test-coverage pass',
readsDiff: true,
brief: `You are **Agent 5: Test Coverage**. Review the diff for:

Expand All @@ -259,6 +278,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th
'6a': {
reviewsCode: true,
label: 'Agent 6a: Undirected audit — attacker mindset',
publicLabel: 'the open-ended audit (attacker mindset)',
readsDiff: true,
brief: `You are **Agent 6a: the undirected audit, attacker mindset.**

Expand All @@ -278,6 +298,7 @@ You are undirected on purpose. Do not restrict yourself to the list.`,
'6b': {
reviewsCode: true,
label: 'Agent 6b: Undirected audit — 3 AM oncall mindset',
publicLabel: 'the open-ended audit (oncall mindset)',
readsDiff: true,
brief: `You are **Agent 6b: the undirected audit, 3 AM oncall mindset.**

Expand All @@ -297,6 +318,7 @@ You are undirected on purpose. Do not restrict yourself to the list.`,
'6c': {
reviewsCode: true,
label: 'Agent 6c: Undirected audit — six-months-later maintainer',
publicLabel: 'the open-ended audit (maintainer mindset)',
readsDiff: true,
brief: `You are **Agent 6c: the undirected audit, six-months-later maintainer mindset.**

Expand All @@ -315,6 +337,7 @@ You are undirected on purpose. Do not restrict yourself to the list.`,

'7': {
label: 'Agent 7: Build & test verification',
publicLabel: 'the build-and-test check',
readsDiff: false,
brief: `You are **Agent 7: Build & Test Verification**. You do not review the diff — you run the project's own deterministic checks and report what they say. Your evidence is **the commands you ran and their output**; a return that names no command has not done this job.

Expand All @@ -330,6 +353,7 @@ Use \`Source: [build]\` or \`Source: [test]\`, never \`[review]\`.`,

'test-matrix': {
label: 'Test coverage matrix (whole-diff)',
publicLabel: 'the whole-diff test-coverage check',
readsDiff: true,
brief: `You are the **test-coverage matrix** agent — Agent 5's cross-chunk counterpart. The territory agents each see either an implementation or a test, rarely both. You see the whole diff, so you own the pairing.

Expand All @@ -341,6 +365,7 @@ Use \`Source: [build]\` or \`Source: [test]\`, never \`[review]\`.`,
'invariant-a': {
reviewsCode: true,
label: 'Invariant agent A: state, timers, collections',
publicLabel: 'the invariant check (state, timers, collections)',
readsDiff: true,
brief: `You are **invariant agent A: state, timers, and collections.**

Expand All @@ -358,6 +383,8 @@ Report a **Critical** for each violation, and give **both** locations that toget
'invariant-b': {
reviewsCode: true,
label: 'Invariant agent B: counters, return values, error taxonomies',
publicLabel:
'the invariant check (counters, return values, error taxonomies)',
readsDiff: true,
brief: `You are **invariant agent B: counters, return values, and error taxonomies.**

Expand All @@ -375,6 +402,7 @@ Report a **Critical** for each violation, and give **both** locations that toget
'invariant-c': {
reviewsCode: true,
label: 'Invariant agent C: config fields, early returns',
publicLabel: 'the invariant check (config fields, early returns)',
readsDiff: true,
brief: `You are **invariant agent C: config fields and early returns.**

Expand All @@ -393,6 +421,7 @@ Report a **Critical** for each violation, and give **both** locations that toget
output: 'verdicts',
acceptsFindings: true,
label: 'Verification agent',
publicLabel: 'verification',
readsDiff: true,
brief: `You are a **verification agent**. You do not look for new problems — you rule on the findings you were handed, listed in the message that launched you, each with a file, a line, an issue, and a **failure scenario**. The failure scenario is the finding's testable claim, and your verdict is the **result of tracing it through the real code**, not a plausibility vote on how the finding reads.

Expand Down Expand Up @@ -422,6 +451,7 @@ Return, for each finding, one verdict:
acceptsChunk: true,
acceptsFindings: true,
label: 'Reverse audit agent',
publicLabel: 'reverse audit',
readsDiff: true,
brief: `You are a **reverse audit agent**. Prior agents have already reviewed this diff and their confirmed findings are listed in the message that launched you. Your job is not to re-report them — it is to find the **gaps**: the important issues no prior agent or round caught.

Expand Down
Loading
Loading