refactor(review): route the certification path atoms through one needle - #9484
Merged
Conversation
`openedBrief`, `readBrief` and `readFindingsPointer` each inlined the same
two lines — `JSON.stringify(path)` then `args.some(a => a.includes(...))` —
which is the single mechanism enforcing the bar's "exact path, not a
`${path}.bak` look-alike" guarantee. Three copies is the exact drift class
this module was created to remove, turned on the module itself: a fix to
the match (a serialization edge, path normalization, a stricter compare)
would have to land in three places in lockstep or leave one atom silently
weaker than its siblings.
Extract a private `namesPath(args, path)` and have all three route through
it. No behavior change — the existing `.bak`-trap pins in
`certification.test.ts` and `check-coverage.test.ts` cover each atom and
pass unchanged.
Collaborator
Collaborator
|
Re-run — three commits landed since the last pass on Template: complete ✓ — all required sections present, with the bilingual summary. Problem: exists, verified in the base tree. Base Direction: aligned. Internal hygiene for the Size: not a core path ( Approach: scope is right, and the follow-up commits sharpened it. The predicate now lives in Risk: no match against the revert-correlated high-risk paths; no elevated risk signals. Moving on to code review. 🔍 中文说明重新运行——自上次审查 模板：完整 ✓ —— 所有必需章节齐全，含中文说明。 问题：真实存在，已在 base 树核实。base 版 方向：对齐。 规模：非核心路径（ 方案：范围恰当，且后续 commit 使之更锋利。判定逻辑现位于 风险：未命中与 revert 相关的高风险路径；无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent proposal before reading the diff: given what the discussion since commit 1 surfaced, I'd do exactly what All four call sites are behavior-identical:
The naming-collision claim checks out against the real code: The two test commits pin exactly the regression classes this refactor could quietly introduce (additions only; existing suites unchanged): a shell command that merely MENTIONS the brief is not an open; matching is existential across the whole call list, not first-element-only — the third atom shares the same wrapper line; the parse half counts an exact diff read while refusing a No blockers, no convention violations. Test evidenceUnattended CI run — per gate rules I do not build or execute PR code; the evidence below is the PR's own CI on the reviewed commit, fetched once via the API. All checks green, nothing pending. CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 macOS/Windows unit legs are skipped by design in this repo (Linux-only unit suite); the test changes here are platform-neutral additions. Real-scenario (tmux) testing: N/A — unattended CI run, and nothing user-visible to drive; this is a pure internal refactor with zero behavior change. 中文说明代码审查读 diff 之前的独立方案：鉴于 commit 1 之后讨论中浮现的问题，我会做的正是 四处调用点行为完全一致：
命名冲突的说法经真实代码核实： 两个测试 commit 钉住的恰是这场重构可能悄悄引入的回归类别（纯新增；既有套件原样不动）：仅仅提及 brief 的 shell 命令不算打开；匹配是对整个调用列表的存在量词、而非只看首元素——第三个 atom 共享同一行包装器；解析半边计数精确的 diff 读取、同时拒绝 无阻塞项，无规范违规。 测试证据无人值守 CI 运行——按门禁规则不构建、不执行 PR 代码；以下证据取自 PR 自身 CI 在受审 commit 上的结果（API 一次性拉取）。所有检查绿色，无待决项。macOS/Windows 单元测试腿在本仓库按设计跳过（Linux-only 单测套件）；本次测试改动为平台中性的纯新增。真实场景（tmux）测试：N/A——无人值守 CI 运行，且无用户可见行为可驱动；纯内部重构，零行为变化。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage and CI green on the reviewed commit; approving pinned to Stepping back: this is what a finished follow-up looks like. Commit 1 deduplicated the three certification atoms; the follow-up commits finished the job properly — folding in the parse-side copy my previous pass had left "deliberately separate" (same expression, same semantics; the separation was a distinction without a difference), and renaming away from The two test commits are the best kind of follow-through — each pins a regression class the refactor could otherwise quietly introduce: a first-element-only quantifier, mention-as-open, Nothing here is trying too hard: 67 production lines, zero public surface changed, no drive-by edits. In six months this reads as the module finally obeying its own header completely — one definition per atom, one function for both halves of the needle. CI landed green on the reviewed commit ( 中文说明置信度：5/5 —— 每个阶段都干净，CI 在受审 commit 上全绿；批准锚定于 退一步看：这才是跟进工作完成的样子。Commit 1 去重了三个认证 atom；后续 commit 把这项工作真正收口——把上一轮审查留下的"刻意独立"的解析侧拷贝收了进来（同一表达式、同一语义；那种独立是无实义的区分），并在与 两个测试 commit 是跟进工作最好的形态——各自钉住一场重构本可能悄悄引入的回归类别：只看首元素的存在量词、以提及冒充打开、 这里没有任何用力过猛：67 个生产行、公共面零变化、无顺手改动。六个月后回头看，这是模块终于完全兑现自己头注释的一刻——每个 atom 一个定义，needle 的两半一个函数。 CI 已在受审 commit 上落绿（ — 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 19, 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": none — no check was cut short..
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 3b"：none — no check was cut short.。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…its own Two findings on the first round, both about the helper this PR introduced. The consolidation stopped one copy short. `parseTranscript` still inlined the same exact-path predicate for the diff-read half of the bar, so the count went from four copies to two — while the new comment claimed a fix to the match reaches all of them at once. The match now lives in `transcripts.ts`, beside the code that serializes the args, and both halves call it: the diff-read half directly, the three brief/findings atoms through a wrapper that spreads it over a record's call list. `certification.ts` already imported from `transcripts.ts`, so this needs no layering inversion. The helper also took the name of a module-private `namesPath` in `utils/findings.ts` whose semantics are deliberately different — boundary matching over PROSE, which credits `rm /plan/chunk-3.brief.md` for naming the brief. Unifying the two the obvious way would make `openedBrief` credit an agent for deleting a file it never opened. They keep distinct names now (`serializedArgsNamePath`, `argsNameExactPath`), and both doc comments say why, so a grep for "the path matcher" cannot land a reader on the wrong one. Tests: the needle's own cases (whole JSON string value, no `.bak` sibling, no shell mention) and the `rm <brief>` shape at the `openedBrief` atom. Relaxing the needle to a bare substring now reddens BOTH suites from one edit — the property the consolidation exists to buy, and the one the second copy denied.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…eTranscript's doc Three review Suggestions on the needle consolidation: - `serializedArgsNamePath` had been inserted between `parseTranscript`'s JSDoc and its declaration, orphaning the doc onto the wrong function. Move it above the JSDoc so each doc sits with its own function. - The diff-read half (`parseTranscript` with a `diffPath`) was exercised by no test — every `readTranscripts` call site omits `diffPath` and nothing set `diffToolCalls` non-zero, so an arg swap at the call site shipped green. Pin it: the exact diff read counted, a `.bak` sibling and a shell command that only names the diff refused, and `diffToolCalls: 0` without a `diffPath`. - `argsNameExactPath`'s existential (`some`) was only ever run on 0/1-element arrays, so a first-element-only regression shipped green. Pin a match in the second position for `openedBrief` and `readBrief`.
The new diff-read fixture asserts `diffToolCalls`, which a mutation dropping the `range: namedTheDiff ? rangeOf(args) : null` wiring survives — the count stays right while every chunk-coverage ruling, which reads the lines rather than the tally, is handed an empty list. The fixture's exact read now carries an `offset`/`limit` and the test asserts `diffReads` equals `[[1, 40]]`; dropping the wiring reddens it.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.14)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 502 passed · 0 failed · 502 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：502 通过 · 0 失败 · 502 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9484 — refactor(review): route the certification path atoms through one needleVerdict: 中文摘要
Central claim + A/BCentral claim: the four path-matching sites ( The A/B harness (
Load-bearing proof of the consolidation (mutation matrix, re-executed live in
Survivor adjudication: B2 and B3 survive on base — both are pre-existing coverage gaps (the exact gaps the PR's new tests close; M1/M2 prove closure at head). No head-side survivor: every single-point mutant of the new code is killed by the new tests. The suite's liveness is proven by M1–M4 (red with behavioral expected-vs-actual messages) and by the unmutated controls P1/P2. CorrectionsThese are corrections to the PR's description, not requests to change the code.
Findings
Not covered
MethodologyEnvironment: CI Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
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.
LGTM, looks ready to ship. ✅
doudouOUC
approved these changes
Aug 20, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed internal refactoring in PR #9484.
Changes (4 files, +165/−12):
- Extracts the shared
serializedArgsNamePathpredicate intotranscripts.tsand routesopenedBrief,readBrief, andreadFindingsPointerthrough it via a privateargsNameExactPathwrapper incertification.ts - The
parseTranscriptdiff-read half also routes through the same function, fulfilling the module's one-definition promise - Both functions carry cross-referencing docs to prevent accidental unification with the prose-boundary
namesPathinutils/findings.ts
Verification:
- ✅ Build: passes
- ✅ TypeScript: no type errors
- ✅ Tests: 59/59 pass (2 test files:
certification.test.ts— 22 tests,transcripts.test.ts— 38 tests, 1 pre-existing skip) - ✅ 0 blockers in existing comments
- ✅ Author replied to 5/7 threads
Assessment: Clean, behavior-identical refactoring of internal review tooling. No blockers — the existing open suggestions from the previous review round remain open for the author to address.
jimpablo
pushed a commit
to jimpablo/qwen-code
that referenced
this pull request
Aug 20, 2026
…read gate (QwenLM#9539) * test(review): single-encode the diff-read fixture and pin the failed-read gate Two follow-ups to the diff-read pin added in QwenLM#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. * test(review): pin the failed-read gate on the evidence arg lists (QwenLM#9539)
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
3 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
Extracts the one predicate the certification bar's path atoms all shared — "does any serialized tool-call arg name this exact path (and not a
${path}.baklook-alike)?" — into a privatenamesPath(args, path)helper, and routesopenedBrief,readBriefandreadFindingsPointerthrough it. Small follow-up to #9473 (lib/certification.ts), addressing the deferred R1-1 suggestion.Why it's needed
Those three atoms each inlined the same two lines (
JSON.stringify(path)thenargs.some(a => a.includes(...))), which is the single mechanism enforcing the bar's "exact path, not a look-alike" guarantee across the module. Three copies is the exact drift classcertification.tswas created to eliminate, turned on the module itself: a future fix to the match (a serialization edge, path normalization, a stricter compare) would have to land in all three in lockstep, and missing one would leave a single atom silently weaker than its siblings.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/lib/certification.test.ts src/commands/review/check-coverage.test.ts src/commands/review/recover-findings.test.ts src/commands/review/lib/layer-audit-gate.test.ts— 188 pass. The suites written against the pre-extraction atoms pass unchanged; in particular the${brief}.bak/${path}.baktrap pins (one per atom) still hold, which is the proof the extracted needle preserves the exact-path semantics.Evidence (Before & After)
N/A — non-user-visible pure refactor, zero behavior change.
Tested on
Risk & Scope
Linked Issues
Follow-up to #9473 (deferred R1-1).
中文说明
把认证 bar 的三个路径 atom（
openedBrief/readBrief/readFindingsPointer）共用的同一判定——「某条序列化工具参数是否命名了这个精确路径（而非${path}.bak之类的近似）」——抽成私有 helpernamesPath(args, path)，三处都改走它。这是 #9473 的小跟进，收掉 defer 的 R1-1 建议。原来三处各自内联了同两行（
JSON.stringify(path)+args.some(a => a.includes(...))），而这正是整个模块「精确路径、非近似」保证的唯一实现机制。三份拷贝正是certification.ts存在本身要消灭的漂移，如今出现在模块自己内部：日后要修这条匹配（序列化边界、路径规范化、更严格比较）必须三处同步改，漏一处就让某个 atom 静默弱于兄弟。纯重构、零行为变化：针对旧内联写法的全部测试原样通过，尤其每个 atom 各自的
.bak陷阱钉子仍然成立——这就是抽出的 needle 保持精确路径语义的证明。cd packages/cli && npx vitest run …certification.test.ts …check-coverage.test.ts …recover-findings.test.ts …layer-audit-gate.test.ts共 188 通过。