Skip to content
Merged
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
35 changes: 30 additions & 5 deletions packages/cli/src/commands/review/lib/transcripts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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' },
),
Comment on lines +283 to +287

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred under the convergence posture — recorded, not declined. This is a real and adjacent gap (the same !isErrorPart gate guards successfulCallArgs/successfulReadFileArgs too), but it is one more step of the same coverage tail: #9484 pinned the brief atoms, #9539 pinned the diff-read count and range, and this would pin the two arg lists next. Each is a Suggestion, not a blocker, and the PR chain here has already run well past the ~5-round convergence bar. Landing it as its own tiny PR every time trades review cost against a mutant nobody has shipped; better batched into the next substantive change to this reader than chased on its own. Happy to fold it in when transcripts.ts is next touched for real.

]
.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);
});
Expand Down
Loading