From ab4365e06bde4871558667d61fb5b7051d417bbc Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 20 Aug 2026 12:18:12 +0800 Subject: [PATCH 1/2] test(review): single-encode the diff-read fixture and pin the failed-read gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the diff-read pin added in #9484: - The fixture's launch line was `JSON.stringify(...)` before the trailing `.map((r) => JSON.stringify(r))` encoded it a second time, so `parseTranscript` parsed a bare string and dropped it — `launchPrompt` was silently `''`. Make it a plain object literal like its siblings, and assert `launchPrompt` so the encoding can't regress unseen. - Add a FAILED (`response: { error }`) read of the diff to the fixture and assert `diffToolCalls` stays 1 and `diffReads` stays `[[1, 40]]`. Hoisting the counter out of the `!isErrorPart` branch — which otherwise ships green across the whole suite — would credit a denied read as a diff read. --- .../commands/review/lib/transcripts.test.ts | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/review/lib/transcripts.test.ts b/packages/cli/src/commands/review/lib/transcripts.test.ts index c42ed2322e7..3d08dfcad45 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,31 +259,47 @@ 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]]); // And with no diffPath the field stays 0, whatever was read. expect(readTranscripts(undefined, ENV)[0].diffToolCalls).toBe(0); From 0c1ebf686f70313297a2f5653b3e792c81bc1faa Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 20 Aug 2026 10:23:25 +0000 Subject: [PATCH 2/2] test(review): pin the failed-read gate on the evidence arg lists (#9539) --- packages/cli/src/commands/review/lib/transcripts.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/cli/src/commands/review/lib/transcripts.test.ts b/packages/cli/src/commands/review/lib/transcripts.test.ts index 3d08dfcad45..eba2659b264 100644 --- a/packages/cli/src/commands/review/lib/transcripts.test.ts +++ b/packages/cli/src/commands/review/lib/transcripts.test.ts @@ -301,6 +301,11 @@ describe('readTranscripts — defensive parsing', () => { // 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); });