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

Choose a reason for hiding this comment

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

[Suggestion] This new denied-read fixture pins the !isErrorPart success gate only for diffToolCalls/diffReads — but the same gate also guards successfulCallArgs and successfulReadFileArgs (transcripts.ts:400-401), and no fixture anywhere asserts that a failed call stays OUT of those two lists. A mutant hoisting successfulCallArgs.push(pending.args) / successfulReadFileArgs.push(pending.args) out of the !isErrorPart branch ships the entire suite green: "counts only successful tool calls" asserts only successfulToolCalls === 0 (its denied call's args are never inspected), this fixture asserts only launchPrompt/diffToolCalls/diffReads, and the only args-list assertions (~lines 232-235) use all-successful responses. Under that mutant a denied or hallucinated read_file of the brief or the findings list leaks into the evidence lists, and the certification atoms that read them — openedBrief (certification.ts:95), readBrief (:110), readFindingsPointer (:128) — certify an agent as having opened/read files it was refused, the exact "naming is not opening" fabrication this module exists to prevent. Verified by probe: with the mutant applied, the entire src/commands/review directory ships green (90 test files, 3816 tests); adding the two assertions below turns this test red under the mutant (expected [ …(4) ] to have a length of 3 but got 4 — the denied read's args leaked into successfulCallArgs) and stays green on the correct code. Since the denied read is already present in this fixture, the gate can be pinned right here — add, alongside the existing assertions:

expect(rec.successfulCallArgs).toHaveLength(3);
expect(rec.successfulReadFileArgs).toHaveLength(2);
中文说明

新增的被拒读取 fixture 只为 diffToolCalls/diffReads 钉住了 !isErrorPart 成功门控 —— 但同一个门控同时守着 successfulCallArgssuccessfulReadFileArgs(transcripts.ts:400-401),而全部 fixture 中没有任何一处断言失败的调用不会进入这两个列表。变异实验:把 successfulCallArgs.push(pending.args) / successfulReadFileArgs.push(pending.args) 提出 !isErrorPart 分支后,整个测试套件全绿通过 —— "counts only successful tool calls" 只断言了 successfulToolCalls === 0(其被拒调用的参数从未被检查),本 fixture 只断言 launchPrompt/diffToolCalls/diffReads,仅有的 args 列表断言(约 232-235 行)用的全是成功响应。在该变异下,一次被拒或被幻觉出的 read_file(读 brief 或 findings 列表)会泄漏进证据列表,读取这些列表的认证原子 —— openedBrief(certification.ts:95)、readBrief(:110)、readFindingsPointer(:128)—— 会把一个被拒绝读取文件的 agent 认证为已打开/已读取,正是本模块要防止的「命名不等于打开」式造假。探针验证:施加变异后整个 src/commands/review 目录全绿(90 个测试文件、3816 个用例);补上下面两条断言后,该测试在变异下翻红(expected [ …(4) ] to have a length of 3 but got 4 —— 被拒读取的参数泄漏进了 successfulCallArgs),在正确代码下保持绿色。被拒读取已经存在于本 fixture 中,门控可以直接在这里钉住 —— 在现有断言旁补上:

expect(rec.successfulCallArgs).toHaveLength(3);
expect(rec.successfulReadFileArgs).toHaveLength(2);

— qwen3.8-max via Qwen Code /review (v0.21.14)

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