test(review): single-encode the diff-read fixture and pin the failed-read gate - #9539
Merged
Conversation
…read gate 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.
Collaborator
|
|
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real and verified in code, not theoretical. The fixture's launch line is pre-stringified and then encoded a second time by the trailing Direction: aligned — this is a direct follow-up to #9484, strengthening the exact test that landed there. Test infrastructure for the review/certification path is an area this repo actively invests in. Size: not applicable — no core paths touched (single test file under Approach: the scope feels right and minimal. Two fixes mapped 1:1 to two verified gaps; the Risk: no elevated risk signals — the only changed file is a test, which the high-risk-path check excludes. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在且已在代码中核实，不是理论性问题。fixture 的 launch 行先被 方向：对齐——这是 #9484 的直接跟进，加固的正是那次落地的测试。review/certification 路径的测试基建是本仓库持续投入的领域。 规模：不适用——未触及核心路径（仅 方案：范围恰当且最小化。两处修复与两个已核实的缺口一一对应； 风险：无升级风险信号——唯一改动文件是测试，高风险路径检查不覆盖测试文件。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewBefore reading the diff, my independent take from the description alone: make the launch entry a plain object so the trailing Tracing it against the production code confirms each piece:
No blockers, no convention issues — the added comments match the file's existing style of explaining what each assertion pins. Testing evidenceThis is an unattended CI run, so per policy I did not build or execute the PR's code; the evidence below is the PR's own CI read through the API at review time. The ubuntu unit suite — the gate that matters for a test-only change — was still running when I looked; nothing is red so far. The Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 What this run verified, and how: the fixture/gate claims above are from reading the diff against the production code at the reviewed commit; the suite passing is the PR's own CI (pending). No user-visible behavior changes, so no real-scenario testing applies — and there is no behavioural claim here that sandboxed verification would settle; the deliverable is the test itself, and its pinning power is checkable statically (done above). 中文说明代码审查：在读 diff 之前，我根据描述独立构想的方案与 PR 完全一致——launch 行改为普通对象字面量让结尾 测试证据：本次为无人值守 CI 运行，按规则未构建或执行 PR 代码，以上证据为通过 API 读取的 PR 自身 CI。审查时 ubuntu 单测套件（测试类改动唯一关键的门槛）仍在运行，暂无红色项； — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — a small, surgical test fix whose two claims I verified against the base code before looking at the diff, and whose approach matches the one I would have proposed independently. Stepping back: both problems are real and confirmed in the tree as it stands — the double-encoded launch line (the record's The only open item is CI: the ubuntu unit suite was still in flight at review time. Approval deferred until CI lands green on 中文说明回头看整体：两个问题都真实存在，且我在看 diff 之前已在 base 代码中核实——双重编码的 launch 行（该测试里 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
previously approved these changes
Aug 20, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 3b": running packages/cli/src/commands/review/lib/transcripts.test.ts to confirm the traced assertions pass (npm install corrupted on this runner after three attem….
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 3b"：running packages/cli/src/commands/review/lib/transcripts.test.ts to confirm the traced assertions pass (npm install corrupted on this runner after three attem…。
— qwen3.8-max via Qwen Code /review (v0.21.14)
Comment on lines
+283
to
+287
| ...call( | ||
| 'read_file', | ||
| { file_path: '/d.txt', offset: 40, limit: 40 }, | ||
| { error: 'denied' }, | ||
| ), |
Collaborator
There was a problem hiding this comment.
[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 成功门控 —— 但同一个门控同时守着 successfulCallArgs 和 successfulReadFileArgs（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)
Collaborator
Author
There was a problem hiding this comment.
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.
Collaborator
Author
|
@qwen-code /takeover |
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
Collaborator
Collaborator
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9539 (round 2 of the branch, first round addressing feedback)Feedback dispositions
Evidence gathered before changing anything (source-blind probe)
Change made
// 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);Counts match the fixture: 3 successful calls (2× No conflict resolution was needed ( Verification
中文说明Autofix 审查轮次 — PR #9539（分支第 2 轮，处理反馈的第 1 轮）反馈处理结果
修改前收集的证据（来源无关探针）
本轮修改
// 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);计数与 fixture 一致：3 次成功调用（2 次 无需解决冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
approved these changes
Aug 20, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max via Qwen Code /review (v0.21.14)
yiliang114
approved these changes
Aug 20, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. CI green. The fixture is now single-encoded (the first arg is a plain object literal, stringified once by the trailing .map), and the added denied-read fixture pins the success gate on diffReads/successfulCallArgs/successfulReadFileArgs — a failed read_file no longer counts as a diff read.
Collaborator
|
Released in v0.21.15. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Two follow-up fixes to the diff-read test added in #9484 (
transcripts.test.ts): single-encode the fixture's launch line, and pin the success-gating ofdiffToolCalls/diffReadsagainst a failed diff read.Why it's needed
The fixture's first JSONL entry was
JSON.stringify({...})before the trailing.map((r) => JSON.stringify(r))encoded every line a second time, soparseTranscriptparsed a bare string and silently dropped the launch line — the record'slaunchPromptwas'', unlike every sibling fixture. The test passed only because it readdiffToolCalls/diffReads; anyone extending or copying the pattern would hit an invisibleexpected '' to be 'chunk 1 of 1'. Separately, no fixture combined a diff-namingread_filewith an error response, so a mutation hoistingdiffToolCalls++/diffReads.pushout of the!isErrorPartbranch would ship green — a denied read of the diff would then count as a diff read, and retirement / layer-audit / coverage would credit chunk territory to an agent that read nothing.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/lib/transcripts.test.ts src/commands/review/lib/certification.test.ts— 60 pass. The launch line is now a plain object literal (.mapencodes it once), andrec.launchPromptis asserted'chunk 1 of 1'. A fourth call —read_fileof/d.txtanswered with{ error: 'denied' }— is added, withdiffToolCallsasserted1anddiffReadsasserted[[1, 40]](the denied read's[41, 80]absent). Both assertions flip red under the respective mutations the #9484 review witnessed.Evidence (Before & After)
N/A — test-only.
Tested on
Risk & Scope
Linked Issues
Follow-up to #9484.
中文说明
#9484 里加的 diff-read 测试的两处跟进修复（
transcripts.test.ts）：把 fixture 的 launch 行改成单次编码，并补上「失败的 diff 读取不计入」的 success-gating 钉子。原来 fixture 第一条 JSONL 是
JSON.stringify({...})，又被结尾的.map((r) => JSON.stringify(r))二次编码，于是parseTranscript解析出一个裸字符串并静默丢弃这条 launch 行——launchPrompt变成''，和其它兄弟 fixture 不一致。测试之所以通过，只因为它读的是diffToolCalls/diffReads；任何扩展或照抄此模式的人都会撞上看不出原因的expected '' to be 'chunk 1 of 1'。另外，没有任何 fixture 把「命名 diff 的 read_file」与错误响应组合，于是把diffToolCalls++/diffReads.push从!isErrorPart分支里提出去的变异能全绿通过——一次被拒绝的 diff 读取就会被算作 diff 读取，retirement / layer-audit / coverage 会把 chunk territory 记给一个什么都没读的 agent。改法：launch 行改成普通对象字面量（
.map只编码一次）并断言launchPrompt；新增第四个调用——read_file读/d.txt但响应{ error: 'denied' }——断言diffToolCalls仍为1、diffReads仍为[[1, 40]]（被拒读取的[41, 80]不出现）。两条断言分别在 #9484 评审验证过的变异下翻红。cd packages/cli && npx vitest run …transcripts.test.ts …certification.test.ts共 60 通过。