diff --git a/packages/cli/src/commands/review/lib/transcripts.test.ts b/packages/cli/src/commands/review/lib/transcripts.test.ts index c42ed2322e7..eba2659b264 100644 --- a/packages/cli/src/commands/review/lib/transcripts.test.ts +++ b/packages/cli/src/commands/review/lib/transcripts.test.ts @@ -244,7 +244,11 @@ describe('readTranscripts — defensive parsing', () => { // 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[] => [ + const call = ( + name: string, + args: object, + response: object = { output: 'ok' }, + ): object[] => [ { ...b, type: 'assistant', @@ -255,32 +259,53 @@ describe('readTranscripts — defensive parsing', () => { type: 'tool_result', message: { role: 'user', - parts: [{ functionResponse: { name, response: { output: 'ok' } } }], + parts: [{ functionResponse: { name, response } }], }, }, ]; file( 'agent-a1.jsonl', [ - JSON.stringify({ + // A plain object literal like its siblings — pre-stringifying it here + // would let the trailing `.map` encode it twice, so `parseTranscript` + // parses a bare string and silently drops the launch line. + { ...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' }), + // A FAILED read of the diff: names it, but the response is an error. + // Hoisting the `diffToolCalls++` / `diffReads.push` out of the + // `!isErrorPart` branch would count this as a diff read. + ...call( + 'read_file', + { file_path: '/d.txt', offset: 40, limit: 40 }, + { error: 'denied' }, + ), ] .map((r) => JSON.stringify(r)) .join('\n') + '\n', ); const [rec] = readTranscripts(undefined, ENV, '/d.txt'); + // The launch line survived — proof the fixture is single-encoded. + expect(rec.launchPrompt).toBe('chunk 1 of 1'); + // Only the ONE successful, exact-path read counts: not the `.bak` + // sibling, not the shell mention, not the denied read. 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. + // the tally — with nothing to rule on. The denied read's [41, 80] is + // absent, pinning the success gate on `diffReads` as well. expect(rec.diffReads).toEqual([[1, 40]]); + // The same gate guards the evidence lists the certification atoms read + // (`openedBrief`, `readBrief`, `readFindingsPointer`): the denied read + // must stay out of them too, not only out of the diff fields. + expect(rec.successfulCallArgs).toHaveLength(3); + expect(rec.successfulReadFileArgs).toHaveLength(2); // And with no diffPath the field stays 0, whatever was read. expect(readTranscripts(undefined, ENV)[0].diffToolCalls).toBe(0); });