refactor(review): route the certification path atoms through one needle

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.

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);
 }

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 <file>` as opening it.
+    expect(
+      serializedArgsNamePath(JSON.stringify({ command: `rm ${brief}` }), brief),
+    ).toBe(false);
+  });
+});

packages/cli/src/commands/review/lib/transcripts.ts

@@ -252,6 +252,31 @@ function rangeOf(args: Record<string, unknown>): [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<string, unknown>;
-      // 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,