diff --git a/packages/cli/src/commands/review/lib/certification.test.ts b/packages/cli/src/commands/review/lib/certification.test.ts index dbad83254c3..65e51628166 100644 --- a/packages/cli/src/commands/review/lib/certification.test.ts +++ b/packages/cli/src/commands/review/lib/certification.test.ts @@ -111,11 +111,41 @@ describe('openedBrief / readBrief', () => { const needle = JSON.stringify(briefPath(PLAN, key)); const arg = `{"absolute_path":${needle}}`; + it('does not credit a shell command that merely MENTIONS the brief', () => { + // The trap a prose matcher walks into: `utils/findings.ts` has a + // same-purpose-looking `namesPath` that matches on a name boundary, and + // it credits this arg. Deleting a brief is not opening it — so this atom + // matches the whole JSON string value instead, and keeps a different + // name so no future consolidation unifies the two the wrong way. + const r = rec({ + successfulCallArgs: [ + JSON.stringify({ command: `rm ${briefPath(PLAN, key)}` }), + ], + }); + expect(openedBrief(r, PLAN, key)).toBe(false); + }); + it('credits any successful tool whose args name the exact brief path', () => { const r = rec({ successfulCallArgs: [arg] }); expect(openedBrief(r, PLAN, key)).toBe(true); }); + it('credits a match anywhere in the call list, not just the first call', () => { + // The shared `argsNameExactPath` wrapper is an existential over the whole + // list. Every other fixture here has 0 or 1 arg, so a first-element-only + // regression (`args.length > 0 && …(args[0]!, path)`) ships green while + // refusing an agent whose first successful call named another file and + // whose LATER call opened the brief — its work re-owed. Match in the + // second position pins the quantifier for all three atoms. + const other = `{"absolute_path":${JSON.stringify(`${PLAN}-other.txt`)}}`; + expect( + openedBrief(rec({ successfulCallArgs: [other, arg] }), PLAN, key), + ).toBe(true); + expect( + readBrief(rec({ successfulReadFileArgs: [other, arg] }), PLAN, key), + ).toBe(true); + }); + it('does not credit a record that made zero successful tool calls', () => { // `[].every(...)` is true: the existential quantifier needs its own // negative, like its two sibling atoms already have. diff --git a/packages/cli/src/commands/review/lib/certification.ts b/packages/cli/src/commands/review/lib/certification.ts index e85ad8ac124..d113dcf3b60 100644 --- a/packages/cli/src/commands/review/lib/certification.ts +++ b/packages/cli/src/commands/review/lib/certification.ts @@ -22,6 +22,7 @@ */ import type { AgentRecord } from './transcripts.js'; +import { serializedArgsNamePath } from './transcripts.js'; import { briefPath } from './prompt-record.js'; /** @@ -60,20 +61,38 @@ export function declaresOwnUncoverable( return m !== null && Number(m[1]) === chunk; } +/** + * Does ANY of these serialized tool-call args name the EXACT `path`? + * + * The match itself lives in `transcripts.ts` beside the code that serializes + * the args, and `parseTranscript`'s diff-read half calls the same function — + * so the bar's "exact path, not a look-alike" guarantee has ONE definition, + * not one per half. This wrapper only spreads it over a record's call list; + * every path atom below routes through it. + * + * The name is deliberately not `namesPath`: `utils/findings.ts` has a + * module-private `namesPath` that matches a path named in PROSE on a name + * boundary — it credits `rm /plan/chunk-3.brief.md` for naming the brief. + * Unifying these two would make `openedBrief` credit an agent for deleting a + * file it never opened, so they keep distinct names to stop a future reader + * treating either as THE path matcher. + */ +function argsNameExactPath(args: readonly string[], path: string): boolean { + return args.some((a) => serializedArgsNamePath(a, path)); +} + /** * Did this record's agent open the brief recorded under `key`? Compared as a * whole JSON string value (`successfulCallArgs` are serialized args), so a - * `${brief}.bak` cannot be credited for the brief — the same trap - * `parseTranscript` avoids for the diff path. "Open" is mention-level: any - * successful tool whose args name the exact path. + * `${brief}.bak` cannot be credited for the brief. "Open" is mention-level: + * any successful tool whose args name the exact path. */ export function openedBrief( rec: AgentRecord, planPath: string, key: string, ): boolean { - const needle = JSON.stringify(briefPath(planPath, key)); - return rec.successfulCallArgs.some((a) => a.includes(needle)); + return argsNameExactPath(rec.successfulCallArgs, briefPath(planPath, key)); } /** @@ -87,8 +106,10 @@ export function readBrief( planPath: string, key: string, ): boolean { - const needle = JSON.stringify(briefPath(planPath, key)); - return rec.successfulReadFileArgs.some((a) => a.includes(needle)); + return argsNameExactPath( + rec.successfulReadFileArgs, + briefPath(planPath, key), + ); } /** @@ -104,6 +125,5 @@ export function readFindingsPointer( pointer: string | null, ): boolean { if (pointer === null) return true; - const needle = JSON.stringify(pointer); - return rec.successfulReadFileArgs.some((a) => a.includes(needle)); + return argsNameExactPath(rec.successfulReadFileArgs, pointer); } diff --git a/packages/cli/src/commands/review/lib/transcripts.test.ts b/packages/cli/src/commands/review/lib/transcripts.test.ts index ace5d152da6..c42ed2322e7 100644 --- a/packages/cli/src/commands/review/lib/transcripts.test.ts +++ b/packages/cli/src/commands/review/lib/transcripts.test.ts @@ -30,6 +30,7 @@ import { transcriptDir, TranscriptsUnavailableError, type AgentRecord, + serializedArgsNamePath, } from './transcripts.js'; import { appendRunSession, recordResume } from './run-ledger.js'; @@ -233,6 +234,56 @@ describe('readTranscripts — defensive parsing', () => { expect(rec.successfulReadFileArgs[0]).toContain('/r/f.findings.md'); expect(rec.successfulReadFileArgs[0]).not.toContain('search_file_content'); }); + + it('counts a diff read via serializedArgsNamePath, not a look-alike', () => { + // The diff-read half of the shared needle: `diffToolCalls` is populated + // only when `diffPath` is passed, and no other test passes one. A swap of + // the two args at the `serializedArgsNamePath(JSON.stringify(args), path)` + // call site (both strings, so it compiles) ships green and every coverage + // gate then reads `diffToolCalls: 0`. Pin it with the exact read counted + // and two look-alikes — a `.bak` sibling and a shell command that only + // NAMES the diff — refused. + const b = { agentId: 'a1', agentName: 'general-purpose', sessionId: 'S1' }; + const call = (name: string, args: object): object[] => [ + { + ...b, + type: 'assistant', + message: { role: 'model', parts: [{ functionCall: { name, args } }] }, + }, + { + ...b, + type: 'tool_result', + message: { + role: 'user', + parts: [{ functionResponse: { name, response: { output: 'ok' } } }], + }, + }, + ]; + file( + 'agent-a1.jsonl', + [ + JSON.stringify({ + ...b, + type: 'user', + message: { role: 'user', parts: [{ text: 'chunk 1 of 1' }] }, + }), + ...call('read_file', { file_path: '/d.txt', offset: 0, limit: 40 }), + ...call('read_file', { file_path: '/d.txt.bak' }), + ...call('run_shell_command', { command: 'rm /d.txt' }), + ] + .map((r) => JSON.stringify(r)) + .join('\n') + '\n', + ); + const [rec] = readTranscripts(undefined, ENV, '/d.txt'); + expect(rec.diffToolCalls).toBe(1); + // The RANGE too, not only the count: `range` is wired through the same + // `namedTheDiff` decision, so dropping that wiring leaves the count + // right and every chunk-coverage ruling — which reads the lines, not + // the tally — with nothing to rule on. + expect(rec.diffReads).toEqual([[1, 40]]); + // And with no diffPath the field stays 0, whatever was read. + expect(readTranscripts(undefined, ENV)[0].diffToolCalls).toBe(0); + }); }); describe('wasGivenTheDiff', () => { @@ -730,3 +781,32 @@ describe('the incomplete-transcript shapes the resume path reads', () => { expect(readTranscripts(undefined, ENV)).toHaveLength(1); }); }); + +describe('serializedArgsNamePath — the one needle both halves use', () => { + const brief = '/plan/chunk-3.brief.md'; + + it('matches the path as a whole JSON string value', () => { + expect( + serializedArgsNamePath(JSON.stringify({ absolute_path: brief }), brief), + ).toBe(true); + }); + + it('does not credit a longer path holding this one as a prefix', () => { + expect( + serializedArgsNamePath( + JSON.stringify({ absolute_path: `${brief}.bak` }), + brief, + ), + ).toBe(false); + }); + + it('does not credit a shell command that merely mentions the path', () => { + // The divergence the review measured between this and the prose-boundary + // `namesPath` in `utils/findings.ts`, which returns true here. Both the + // diff-read half and the brief atoms route through THIS one, so the + // certification bar cannot credit `rm ` as opening it. + expect( + serializedArgsNamePath(JSON.stringify({ command: `rm ${brief}` }), brief), + ).toBe(false); + }); +}); diff --git a/packages/cli/src/commands/review/lib/transcripts.ts b/packages/cli/src/commands/review/lib/transcripts.ts index 8351a255acb..9448c99eb32 100644 --- a/packages/cli/src/commands/review/lib/transcripts.ts +++ b/packages/cli/src/commands/review/lib/transcripts.ts @@ -252,6 +252,31 @@ function rangeOf(args: Record): [number, number] | null { return [off + 1, off + limit]; } +/** + * Do these serialized tool-call args name the EXACT `path`? + * + * The comparison is against the whole JSON string value — `JSON.stringify` + * carries the closing quote — so a `${path}.bak`, or any longer path holding + * this one as a prefix, is NOT credited. Every certification atom that asks + * "did the agent name this file" routes here: the diff-read half in + * `parseTranscript` below, and the brief / findings atoms in + * `certification.ts`. One copy, so a fix to the match semantics + * (normalisation, escaping, a stricter compare) reaches the whole bar at once + * rather than half of it. + * + * NOT `namesPath` in `utils/findings.ts`: that one matches a path mentioned in + * PROSE on a name boundary, so it credits `rm /plan/chunk-3.brief.md` for + * naming the brief. Crediting an agent for deleting a file it never opened is + * precisely what this predicate must not do, which is why the two keep + * separate names. + */ +export function serializedArgsNamePath( + serializedArgs: string, + path: string, +): boolean { + return serializedArgs.includes(JSON.stringify(path)); +} + /** * Parse one transcript. Returns null for a file that is not one. * @@ -342,10 +367,8 @@ function parseTranscript(file: string, diffPath?: string): AgentRecord | null { // to open; a tool *result* that quotes it (a grep over `.qwen/tmp`, this // file in a diff) says nothing about what the agent opened. const args = (fc.args ?? {}) as Record; - // Match the path as a whole JSON string value, quotes included: a bare - // substring credits `…/diff.txt.bak` for `…/diff.txt`. const namedTheDiff = diffPath - ? JSON.stringify(args).includes(JSON.stringify(diffPath)) + ? serializedArgsNamePath(JSON.stringify(args), diffPath) : false; const pending: Pending = { namedTheDiff,