fix(review): keep quoted code from blinding the footer strip - #10458
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the reset — this is a much easier PR to judge than the one it replaced. Template looks good ✓ Problem: observed, and I confirmed it myself rather than taking the description's word for it. The cited comment ( Direction: aligned. The version stamp is the only way a reader can tell which CLI produced a review, and a second unversioned footer beside it defeats exactly that. CHANGELOG has no direct reference to review-comment attribution; the nearest signal is that the reference product does treat render-fidelity as its own bug class (terminal hyperlinks whose targets carry control or invisible characters now render as plain text; markdown rendering wrongly disabled for a whole message). That supports the "match what GitHub displays, not the raw bytes" framing, but it is a signal, not a criterion. Size: not applicable — Approach: I wrote my own proposal before opening the diff — blank quoted code with offset-preserving placeholders, take "which lines are code" from Risk: no elevated risk signals — none of the six files match the high-risk path list. Two notes on where this run sits. The head moved to Moving on to code review. 🔍 中文说明感谢这次重置 —— 相比它替换掉的那个版本，这个 PR 好判断得多。 模板完整 ✓ 问题： 已观测到，而且我自己核实过，没有只采信描述。被引用的那条评论（ 方向： 对齐。版本号戳是读者判断某条评审出自哪个 CLI 版本的唯一依据，旁边多一条不带版本号的 footer 恰好破坏的就是这一点。CHANGELOG 没有与评审评论署名直接对应的条目；最接近的信号是参考产品确实把"渲染保真"当作独立的一类缺陷来修（目标含控制字符或不可见字符的终端超链接改为按纯文本渲染；整条消息的 markdown 渲染被错误关闭）。这支持"匹配 GitHub 显示的内容而非原始字节"这个思路，但它是信号，不是判据。 规模： 不适用 —— 方案： 我在打开 diff 之前先写了自己的方案 —— 用保持偏移的占位符把引用的代码置空、用 风险： 无升级风险信号 —— 六个文件都不匹配高风险路径清单。 关于本次运行所处位置的两点说明。head 已从上一次审查的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo Critical blockers. I went looking for the failure rather than confirming the fix, and the things that could have been quietly wrong here were not. The offset arithmetic had to be rewritten for blanking to be safe at all, and it was. The early return in front of the strip is a fast path, not a silent disable. This is the shape that usually kills a strip — a guard that returns the body unchanged — so I tried to prove it sound rather than read it as intent. The projection emits only literal input characters, entity decodes (which require a literal The parser delegation is reuse, not a new dependency.
The four The new perf bound is well placed and closes the reservation the last pass recorded. 40 ms with The test oracles are the right ones. Two non-blocking observations, neither worth another round:
Not verified: I did not execute branch code (this gate reviews statically), so the A/B numbers on the real witness body — 3006 characters returned byte-identical on CI evidenceThe PR's own CI on this commit, read through the API. Nothing is red. The two legs that actually cover this code are still in flight, so there is no suite result to attest to yet — I am reporting what exists, not guessing the rest. The Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the one claim static review cannot: 中文说明代码审查没有 Critical 阻塞项。我是带着"去找它失败"而不是"去确认它修好了"的心态看的，而这里可能悄悄出错的地方都没有出错。 为了让置空本身安全，偏移算术本来就必须重写，而它确实重写了。 剥离函数前面的提前返回是快路径，不是让剥离悄悄失效的开关。 这通常正是会废掉一个 strip 的形状 —— 一个原样返回正文的守卫 —— 所以我去证明它成立，而不是把它当意图读过去。投影只会产出：字面输入字符、实体解码（需要一个字面的 解析器委托是复用，不是新增依赖。
四处 新增的性能上界放得准，并且结掉了上一次审查记录下来的那条保留意见。 40 ms 配 测试的判据选得对。 两条不阻塞的观察，都不值得再走一轮：
未验证：我没有执行分支代码（本门禁为静态审查），所以关于真实见证正文的 A/B 数字 —— CI 证据以上是通过 API 读到的、该提交自身的 CI。没有红项。真正覆盖这段代码的两个检查仍在运行，因此目前还没有可以背书的套件结果 —— 我报告的是已存在的东西，不猜测其余部分。本 PR 的 沙箱验证可以结掉静态审查无法结掉的那一条主张： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — clean across every stage and I could verify the load-bearing parts by enumeration rather than by trust; the point I am holding back is the two non-blocking observations above, neither of which I would make a merge condition. Stepping back. The thing that decides this PR for me is not the fix, it is the reset. Fourteen rounds of hardening produced machinery whose own new code kept supplying the next round's Criticals, and by the end it was over-stripping visible reviewer content — the worse of the two failure modes this strip can have, because a duplicate attribution line is cosmetic and a swallowed finding is not. Cutting the branch back to the original fix plus the two changes that retire confirmed defects, and archiving the loop's head instead of continuing to patch it, is the correct read of that pattern. The diff is now smaller than the sum of the defects it retires. Against my own proposal: it matches on the blanking and the parser delegation, and beats it on the fold. I would have patched each fold site as I found it — which is precisely how the duplicates channel got missed, since it reaches the fold with no ingest-time strip ahead of it. Putting the guarantee at What I am satisfied about, having gone looking for the failure: the early return in front of the strip is provably a fast path rather than a silent disable; the single-slice cut is the only form that cannot splice a NUL into a posted body; every one of the four new call sites really does receive a single line, which is the whole licence for the What I am not blocking on, stated plainly so it is not silently dropped: a mid-line unterminated The honest gap in the evidence is unchanged from the last pass and is not closable from here: the A/B claim on the real witness body rests on the author's measurement because this gate does not execute branch code. I confirmed the witness independently through the API, the static case is complete, the So: approve, deferred until CI lands green on One housekeeping note for whoever merges: my 中文说明信心度：4/5 —— 各阶段都干净，而且承重的部分我能靠枚举而不是靠采信来验证；我保留的那一分来自上面两条不阻塞的观察，其中任何一条我都不当作合并条件。 退一步看。真正让我判定这个 PR 的不是那处修复，而是这次重置。十四轮加固造出的机器，其自身的新代码不断供给下一轮的 Critical，到最后它已经在过度剥离评审者的可见内容 —— 那是这个 strip 两种失败模式里更糟的一种，因为多一条署名行只是外观问题，而吞掉一条发现不是。把分支砍回"原始修复 + 消掉已确认缺陷的那两处改动"，并把 loop 的最后 head 归档而不是继续修补，是对这个模式的正确判读。现在这个 diff 比它所消掉的缺陷总量还要小。 对照我自己的方案：置空与解析器委托两点一致，折叠这一点它更好。我原本会逐个折叠站点去补 —— 而这恰恰就是重复项通道被漏掉的方式，因为它到达折叠处时前面没有任何摄入时剥离。把这条保证放在 我是带着"去找它失败"的心态看的，以下几点让我满意：剥离函数前面的提前返回可以被证明只是快路径，而不是让剥离悄悄失效的开关；单次切片的剪切是唯一不可能把 NUL 拼进发布正文的形式；四个新调用点确实都收到单行输入，而这正是 以下是我不阻塞、但要明说以免被悄悄丢掉的内容：正文中一个行中位置的未闭合 证据上诚实的缺口与上一次相同，且从这里无法关闭：关于真实见证正文的 A/B 主张依赖作者的测量，因为本门禁不执行分支代码。我通过 API 独立核实了那条见证评论，静态层面的论证是完整的，正在进行中的 所以：批准，但延迟到 CI 在 给合并者的一条事务性提示：我早先某轮针对 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
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
left a comment
qwen-code-ci-bot
left a comment
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
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): 5563 tests pass — this review observed 25943 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：5563 tests pass — this review observed 25943 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 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 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #10458 review feedbackAll nine inline findings (5 Critical, 4 Suggestion) were reproduced with probes on the round-start code, then resolved in code; the Findings and dispositions
Changed files
Verification
中文说明本轮总结 — PR #10458 审查反馈九条行内发现(5 条 Critical、4 条 Suggestion)全部在本轮开始时的代码上用探针复现,随后在代码中解决;携带它们的 发现与处置
变更文件
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 9 selected review thread(s). · 已关闭全部选中的 9 条评审线程。 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
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 7 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 7 轮。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
🔀 Base updated: red check(s) [Post Coverage Comment (ubuntu-latest, 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Post Coverage Comment (ubuntu-latest, 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 7 by the review time budget.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/review-footer.ts:818 — [probe] list-aware code threshold keep direction unpinned — MUTANT M3 escapes all 838 tests flipping keep→strippackages/cli/src/commands/review/compose-review.ts:452 — [probe] boundDeferredLine truncates AFTER the strip — a mid-line forged footer can land trailing on the 240-char cutpackages/cli/src/commands/review/compose-review.ts:452 — [probe] mid-line forged footer in a deferred title posts even without truncation (attribution-off leg only)
Convergence: round 3 posted 8 inline comment(s), 2 of them reported for the first time; the previous round posted 6 (5 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/review-footer.ts (findings in rounds 1, 2; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 7 轮。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 8 条行内评论，其中 2 条是首次提出；上一轮发布了 6 条（其中 5 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/review-footer.ts（第 1、2 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind Round summary — PR #10458 (same-run verification repair)The previous commit (9833c142f8) resolved all 14 inline findings but was Root causeThe round-3 structural fix delegated the CommonMark scan to markdown-it, Fix
Why it was not pushed: Note: the base has since been auto-updated; the verdict below predates that update, and the next round's re-measurement may charge the round. tests failed in packages/cli 中文说明🤖 AutoFix 更新了一个过期的 base —— 修复未通过验证，但本 PR 落后于 验证门的拒绝原因与日志证据见上方英文部分（gate-rejection 不翻译）。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33276063414 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 the gate hoisted ahead of the blanking never runs ahead of it — already reported (comment 3887713285)
- R2-3 second encoding of the heading/list-item/thematic-break families, already divergent — already reported (comment 3887713291)
- R2-4 two new classification branches have no fixture in either direction — already reported (comment 3887713292)
- R2-5 the marker-swallow refusal names a code fence for unclosed raw-HTML blocks — already reported (comment 3887713295)
Not reviewed: build-and-test — Agent 7's harness exhausted its budget before building packages/cli; no test suite ran in that dimension (the changed files' unit tests ran green at HEAD under other agents' probes: review-footer.test.ts 83/83, the new compose-review and submit tests).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 4": (none — no Budget gap: lines; all planned checks completed).
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:392 — [review] D4-1 collapseEntry duplicates collapseToLine's fold on the inline '\n' spelling — deferred under the code-age rule (anchored in code unchanged since the round-3 head)
Convergence: round 4 posted 5 inline comment(s), 1 of them reported for the first time; the previous round posted 8 (2 new). Findings keep coming back to the same files: packages/cli/src/commands/review/compose-review.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Agent 7's harness exhausted its budget before building packages/cli; no test suite ran in that dimension (the changed files' unit tests ran green at HEAD under other agents' probes: review-footer.test.ts 83/83, the new compose-review and submit tests)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 4"：(none — no Budget gap: lines; all planned checks completed)。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 5 条行内评论，其中 1 条是首次提出；上一轮发布了 8 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/compose-review.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
🤖 AutoFix ran out of time before finishing (timeout (3600000ms)) (attempt 3/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (3600000ms)）（第 3/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33294796641 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Post Coverage Comment (ubuntu-latest, 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Post Coverage Comment (ubuntu-latest, 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- F1 the hoisted gate never runs ahead of the blanking (review-footer.ts:439) — already reported (comments 3885887429, 3887059768, 3887713285)
- F2 the marker-swallow refusal names a code fence for unclosed raw-HTML blocks (review-footer.ts:570) — already reported (comments 3887059777, 3887713295)
- F3 the thematic-break/heading scanLines arms have no fixture in either direction (review-footer.ts:782) — already reported (comments 3887059774, 3887713292)
Not reviewed: build-and-test — the packages/cli full suite did not complete within the harness budget (infrastructure timeout; the three changed test files pass 838/838), and Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 3c": none — the channel inventory and the mechanism trace completed within budget..
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — the packages/cli full suite did not complete within the harness budget (infrastructure timeout; the three changed test files pass 838/838), and Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 3c"：none — the channel inventory and the mechanism trace completed within budget.。
未审查：反向审计——评审时间预算不足，未能开始第 6 轮。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
🔀 Base updated: red check(s) [Post Coverage Comment (ubuntu-latest, 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Post Coverage Comment (ubuntu-latest, 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 the hoisted gate never runs ahead of the blanking (review-footer.ts:440) — already reported (comments 3885887429, 3887059768, 3887713285)
- R2-3 second encoding of the heading/list-item/thematic-break families (review-footer.ts:577) — already reported (comment 3887713291)
- R2-5 marker-swallow refusal names a code fence for comment openers (review-footer.ts:570) — already reported (comments 3887059777, 3887713295)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — the packages/cli full vitest suite did not complete within the harness budget on this runner (both full-budget attempts timed out; the four review test files pass 944/944 under a targeted run).
3 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/review-footer.ts:572 — [review] S4: The type-4 declaration opener in RAW_HTML_BLOCK_OPENERS (this line) is the only entry with zero test coverage — comment, PI and CDATA each have dedicated assertions …packages/cli/src/commands/review/lib/review-footer.ts:793 — [review] S5: The lists stack is never exercised past one level in any test — every list fixture in the new tests is a single flat item — so the innermost-item content-indent read…packages/cli/src/commands/review/lib/review-footer.ts:767 — [review] S6: The new indent < 4 fence-open guard (this block) — which stops a tab-indented delimiter (a tab lands on the 4-column stop) from opening fence state — has no test; th…packages/cli/src/commands/review/lib/review-footer.ts:504 — [review] S2 blanked-kind set written twice in blankQuotedCode (.some gate and .map) — drift hazard into partial blankingpackages/cli/src/commands/review/lib/review-footer.ts:724 — [review] S3 list-relative indented-code threshold inscribed verbatim three times in scanLines (:724, :795, :818)
Convergence: round 6 posted 6 inline comment(s), 1 of them reported for the first time; the previous round posted 5 (0 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/review-footer.ts (findings in rounds 2, 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — the packages/cli full vitest suite did not complete within the harness budget on this runner (both full-budget attempts timed out; the four review test files pass 944/944 under a targeted run)。
3 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 6 条行内评论，其中 1 条是首次提出；上一轮发布了 5 条（其中 0 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/review-footer.ts（第 2、3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #10458 (round 7)This round closes the six open Critical families at the root instead of per-entrance, and resolves the two cheap Suggestion enablers (R1-5, R2-4). R2-3 and R2-5 are deferred to the next round (recorded below and on their threads). Structural fix (R2-1 — all five rounds of blockers)
Verified against the parser before implementing: every pinned keep-shape (closed/unclosed fences, indented blocks, tab stops, quoted fences, CRLF/bare-CR twins, the tail-straddling fence) classifies as code, and every probe-verified entrance from rounds 2–6 classifies as prose — lazy continuation (plain, quoted, and in list items), actual-gap list content indents ( One round-6 witness deserves a note: Shared projection root (R3-2 + R6-1)
Channel fixes (R2-2, R3-1, R4-1)
Note on layering (mutation audit): the ingest strip and the leg strips redundantly cover each other — removing any single one leaves the witnesses green, because the other layer catches the same shapes; removing the ingest strip AND both leg strips together reds the witnesses. Both layers are kept because R3-1 and R4-1 each prescribe one, and they defend different future drifts (an ingest weakening vs. a materialization at the render leg). The gate projections ( Enablers (R1-5, R2-4)
Mutation probes (all restored to green afterwards)
Pre-round check (required for Critical resolution): with the pre-round source restored, all 8 new witness tests are red (4 in each suite). Deferred to the next round
Verification
中文说明本轮总结 — PR #10458（第 7 轮）本轮从根因上关闭六个未决 Critical 簇，而不是逐入口修补；同时解决两条低成本的 Suggestion 前置项（R1-5、R2-4）。R2-3 与 R2-5 延后到下一轮（已在下方及其线程中记录）。 结构性修复（R2-1 —— 连续五轮的阻断项）
实现前已对照解析器逐一验证：所有钉住的保留形状（闭合/未闭合围栏、缩进块、Tab 制表位、引用块内围栏、CRLF/裸 CR 孪生、跨越尾部界限的围栏）都分类为代码；第 2–6 轮经探针确认的全部入口都分类为正文——惰性延续（普通、引用内、列表项内）、按真实间距计算的列表内容缩进（ 第 6 轮有一处见证需要说明： 共享投影根因（R3-2 + R6-1）
通道修复（R2-2、R3-1、R4-1）
关于分层（变异审计）：摄取剥离与渲染腿剥离互相冗余覆盖——单独移除任一层，见证测试仍绿（另一层接住了相同形状）；同时移除摄取剥离与两条渲染腿剥离则见证变红。两层都保留，因为 R3-1 与 R4-1 各指定一层，且各自防御不同的未来漂移（摄取被弱化 vs. 渲染腿显形）。门投影（ 前置项（R1-5、R2-4）
变异探针（随后全部恢复为绿）
轮前检查（Critical 解决所必需）：恢复轮前源码后，全部 8 个新见证测试变红（两个套件各 4 个）。 延后到下一轮
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 26 selected review thread(s). · 已关闭全部选中的 26 条评审线程。 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
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — the packages/cli full vitest suite did not complete within the harness budget on this runner (infrastructure kill at 540s; the four review test files pass 952/952 under a targeted run).
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:5074 — [review] D7-1: the two new verbatim-exit strips (bodyCriticalBlock :5074, formatCannotTell :1556) are untested defense-in-depth — removing both keeps 635/635 greenpackages/cli/src/commands/review/lib/review-footer.test.ts:327 — [review] D7-2: the bare-CR fixture does not outcome-distinguish the LINE_ENDING_RE/markdown-it line-alignment — a consistent /\n/ segmentation mutation passes it
Convergence: round 7 posted 5 inline comment(s), 4 of them reported for the first time; the previous round posted 6 (1 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/review-footer.ts (findings in rounds 2, 3, 6; 3 more now); packages/cli/src/commands/review/compose-review.ts (findings in rounds 2, 4; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — the packages/cli full vitest suite did not complete within the harness budget on this runner (infrastructure kill at 540s; the four review test files pass 952/952 under a targeted run)。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 5 条行内评论，其中 4 条是首次提出；上一轮发布了 6 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/review-footer.ts（第 2、3、6 轮已出过发现，本轮又有 3 条）；packages/cli/src/commands/review/compose-review.ts（第 2、4 轮已出过发现，本轮又有 1 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
🤖 AutoFix ran out of time before finishing (timeout (7200000ms)) (attempt 5/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 在完成前耗尽了时间（timeout (7200000ms)）（第 5/100 次尝试）—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33348016796 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #10458 (review round 8, Critical-only mode)All five Critical inline findings from the round-7 review were reproduced at Feedback points and dispositions1. [rv round 7 / rc:3891115885] R2-1 —
|
| # | Guard | Mutation | Result |
|---|---|---|---|
| P1 | fold→strip→cap order (R2-2) | revert to strip-after-bound | duplicates test red; restored → green |
| P2 | unclosed-only neutralization (R6-1) | revert to full delimiter neutralization | closed-comment test red; restored → green |
| P3 | EOL-blank for truncation openers (R3-2) | always literalize unclosable openers | dangling-opener test red; restored → green |
| P4 | rawHtml not counted (R7-1) |
count rawHtml in the exposure check |
swallows test red; restored → green |
| P5 | wide anywhere-strip indent (R2-1) | narrow back to {0,3} |
list-interrupt witness red; restored → green |
| P6 | token-map fence classification (R2-1) | drop the fence branch from scanLines |
quoted-fence keep test red; restored → green |
Verification
- Reproduction at HEAD: the 6 new/updated witness tests failed at the exact
pre-fix commit with the shapes the findings describe (e.g.
expected 'x\n\n_— m via Qwen Code /review_ <!\-\-' to be 'x',
expected true to be false,
expected 'x _— m via Qwen Code /review_' not to match /via\s+Qwen\s+Code\s+\/review/). npx vitest run src/commands/review/lib/review-footer.test.ts— 90 passed (86 prior + 4 new).npx vitest run src/commands/review/compose-review.test.ts(two new tests) — passed.npx vitest run src/commands/review/— 116 files, 5657 passed | 17 skipped, 0 failed.npm run typecheck— passed.npm run lint— passed.npm run build— passed.- Mutation probes P1–P6 as tabled above (each red under mutation, green restored).
No settings source changed, so npm run generate:settings-schema was not
required. Integration tests were not run: the touched behavior is unit-level
(pure string sanitation), fully exercised by the focused Vitest suites above,
and the integration harness needs no-sandbox CLI runs the review itself
disclosed as skipped on this runner class.
中文说明
轮次总结 — PR #10458(评审第 8 轮,仅 Critical 模式)
第 7 轮评审的 5 条 Critical 行内发现全部在 HEAD 上用聚焦的红色测试复现、
按根因修复、由同一批测试见证,并逐条做了变异探针。无需解决冲突
(--conflict false)。评审自己按收敛姿态记录的延后项(D7-1、D7-2)标注为
"本轮不要求修改"——保持不动。没有升级给维护者的事项:每一条都有基于证据
的代码修复。
反馈条目与处置
1. [第 7 轮评审 / rc:3891115885] R2-1 — scanLines 手工建模 CommonMark 块语法 — 已修复(结构性)
复现:四个活化入口全部在 HEAD 探针验证为红(列表打断被忽略保留了伪造
footer;列表内围栏误分类;setext 下划线误分类;内容缩进阈值被压低)。
按发现要求做了结构性修复,而不是逐入口打补丁:scanLines 现在从
BLOCK_PARSER.parse(body, {}) 的 token 映射派生每一行的类别——fence →
引用,code_block → 引用,html_block → 可映射的可见内容(标签型 1/6/7)
或新的 rawHtml 类别(2-5 型),其余 → 可映射正文——与空白化已采用的委托
相同。约 230 行手工模型(围栏配对、HTML 块状态、列表内容缩进、setext/
标题/主题线检测、懒延续跟踪)被净删除(源码行数为负)。引用块深度/内容仍
按行计算,供段落合并与引用形状映射使用。新增四条见证断言(剥离列表打断的
footer;保留列表内围栏与 setext 引用;剥离 4 空格标记的内容缩进形状)。
2. [rc:3891115888] R6-1 — 三种注释形状逃过 stripReviewFooterLine — 已修复
复现:注释拆分标记(Qwen<!\-\- --> Code)、完整伪造 footer 之后的非空注释、
footer 之前的闭合注释对,三者均在 HEAD 存活;组合层 bodyCriticals 孪生用例
在署名关闭时发出伪造署名(发布正文中是空格拆分的标记)。
修复:单行 strip 现在匹配"删除注释后"的显示投影(projectInvisibles 借助
索引映射整段删除闭合的 <!\-\-…--> 段,使标记短语重新拼合、剪切消费掉该
段),仅对未闭合起始符保留等长定界符中和(钉住的
'x <!\-\- _— …' → 'x' 用例)。钉住错误闭合对输出('x <!\-\- hidden')的测试
现在钉住 'x'。见证:三个单元形状,外加署名关闭组合断言(对伪造标记采用
空白容忍匹配)。
3. [rc:3891115891] R3-2 — 字面化的悬空起始符破坏 $ 锚定 — 已修复
复现:stripReviewFooter('x\n\n_— m via Qwen Code /review_ <!\-\-') 与起始符
在后续独立行的孪生用例在 HEAD 逐字节返回;
stripForgedFooterLines('_— m via Qwen Code /review_ <!\-\-') 保留该行。
在 projectInvisibles 中修复:不可闭合的起始符,若其后到行尾只剩空白
(截断孪生),则空白化到行尾,使尾部锚定能到达其前面的 footer;若该行在
起始符之后仍有可见内容,则保持字面,保留第 6 轮的钉住(代码中引用的
闭合符不能闭合正文注释;内容在前的形状仍按原样剥离)。见证覆盖同行形状、
后续行形状,以及 anywhere-strip 孪生。
4. [rc:3891115892] R2-2 — 重复项腿在 240 字符上限之后才剥离 — 已修复
复现:被代码形状保护的伪造 footer 折叠后超过 240 字符时,上限剪切落在
标记中间并追加 …,$ 锚定的正则无法越过它匹配——署名关闭正文发出了
伪造标记。
按"折叠 → 剥离 → 限长"重排修复,与所有兄弟腿一致(formatCannotTell 先剥
后限;延后/改道腿摄取时剥、渲染时限):
boundDeferredLine(stripReviewFooterLine(collapseToLine(entry)))。折叠仍
保持在剥离之前,使被软换行拆开的 footer 先重新拼合。见证:缩进代码与未
闭合围栏两个孪生用例,折叠长度 241——署名关闭零标记,署名开启恰好只有
正规标记一条。
5. [rc:3891115902] R7-1 — 2-5 型原始 HTML 状态使 swallowsAppendedMarker 过度拒绝 — 已修复
复现:对以悬空 <!\-\- / <? / 无 > 的 <!A / <![CDATA[ 块结尾的正文,
swallowsAppendedMarker 返回 true(署名关闭门会错误要求为一个不存在的
围栏重写),而合并基线返回 false。
修复:新的 token 映射分类器给 2-5 型 html_block 一个独立的 rawHtml
类别(按起始语法检测),swallowsAppendedMarker 不计入它——这些块内追加的
标记渲染为无,正是署名关闭想要的。标签型块保留 html,钉住的 <pre>
暴露仍为 true。见证:四种悬空形状为 false,闭合注释对照为 false,<pre>
对照为 true。
评审封套 [rv:5062461023] — CHANGES_REQUESTED "部分审查——缺口已披露"
其可执行内容即上面 5 条行内发现,已全部解决。两条"未审查"披露是基础设施
缺口(集成测试在 CI 被跳过,完整的 packages/cli vitest 运行被运行器预算在
540 秒强杀——聚焦的评审测试套件通过,见验证一节);工作流的 CI 仍是最终
门槛。已考虑 diff 增长提示与收敛观察:本轮结构性委托 scanLines 以净负的
源码行数落地修复,这也正是提示所指的根因定位。
变异探针(每个守卫都有见证)
| # | 守卫 | 变异 | 结果 |
|---|---|---|---|
| P1 | 折叠→剥离→限长顺序 (R2-2) | 回退到限长后剥离 | 重复项测试变红;恢复 → 绿 |
| P2 | 仅中和未闭合起始符 (R6-1) | 回退到全部定界符中和 | 闭合注释测试变红;恢复 → 绿 |
| P3 | 截断起始符的行尾空白化 (R3-2) | 不可闭合起始符一律字面化 | 悬空起始符测试变红;恢复 → 绿 |
| P4 | rawHtml 不计入暴露 (R7-1) |
暴露检查计入 rawHtml |
swallows 测试变红;恢复 → 绿 |
| P5 | anywhere-strip 宽缩进 (R2-1) | 收窄回 {0,3} |
列表打断见证变红;恢复 → 绿 |
| P6 | token 映射围栏分类 (R2-1) | 从 scanLines 移除围栏分支 |
引用围栏保留测试变红;恢复 → 绿 |
验证
- HEAD 复现:6 条新增/更新的见证测试在修复前的提交上按发现描述的形态失败
(如expected 'x\n\n_— m via Qwen Code /review_ <!\-\-' to be 'x'、
expected true to be false、
expected 'x _— m via Qwen Code /review_' not to match /via\s+Qwen\s+Code\s+\/review/)。 npx vitest run src/commands/review/lib/review-footer.test.ts— 90 通过(86 原有 + 4 新增)。npx vitest run src/commands/review/compose-review.test.ts(两条新测试)— 通过。npx vitest run src/commands/review/— 116 个文件,5657 通过 | 17 跳过,0 失败。npm run typecheck— 通过。npm run lint— 通过。npm run build— 通过。- 变异探针 P1–P6 见上表(变异下均为红,恢复后为绿)。
未改动任何 settings 源,因此不需要 npm run generate:settings-schema。
未运行集成测试:本次触碰的行为是单元级(纯字符串净化),已完全由上述聚焦
Vitest 套件覆盖,且集成测试需要无沙箱 CLI 运行——评审本身已披露该类运行
器上此通道被跳过。
Deferred non-Critical feedback
Critical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (@qwen-code /retry starts a fresh counting window.)
中文说明
已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 @qwen-code /retry 可开启新的计数窗口。）
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。
salvage merge; resolved 0 of 5 selected thread(s), 5 left for a later round. · 评审线程关闭被跳过——守卫:salvage merge;选中 5 条,本轮关闭 0 条,其余 5 条留待后续轮次。
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/模型 qwen3.8-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — packages/cli full vitest suite did not complete within the harness budget (infrastructure timeouts; the four review test files pass 957/957 under targeted runs).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — ran to the 5-round cap without converging (rounds 3, 4 and 5 each reported findings; the cap-round findings were verified).
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/cli/src/commands/review/lib/review-footer.ts:531 — [probe] Critical [fails-closed] [new-surface] R6-1: (fix-induced) neutralizeUnclosedOpeners pays a per-opener end-scan — quadratic on unbounded one-line bodies (sibling of R8-3; fi…packages/cli/src/commands/review/compose-review.ts:1684 — [probe] empty-bullet: a tab-indented footer-only duplicates entry passes the emptiness filter, then fold+strip reduce it to '' — a bare '- ' bullet posts and the count sentence still…packages/cli/src/commands/review/lib/review-footer.ts:916 — [probe] FORGED_FOOTER_LINE_ANYWHERE_RE keeps a single optional '>' allowance — a forged footer quoted at blockquote depth >= 2 survives the attribution-off inline-comment leg
Convergence: round 8 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 5 (4 new). Findings keep coming back to the same files: packages/cli/src/commands/review/lib/review-footer.ts (findings in rounds 2, 3, 6, 7; 4 more now); packages/cli/src/commands/review/compose-review.ts (findings in round 2; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (6 Critical(s)), the rate of first-time findings is not falling (this round 6, previous 4), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — packages/cli full vitest suite did not complete within the harness budget (infrastructure timeouts; the four review test files pass 957/957 under targeted runs)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — ran to the 5-round cap without converging (rounds 3, 4 and 5 each reported findings; the cap-round findings were verified)。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 8 轮发布了 6 条行内评论，其中 6 条是首次提出；上一轮发布了 5 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/lib/review-footer.ts（第 2、3、6、7 轮已出过发现，本轮又有 4 条）；packages/cli/src/commands/review/compose-review.ts（第 2 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 6 条 Critical），首次发现的速率没有下降（本轮 6，上一轮 4），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
🤖 AutoFix stopped: this counting window now contains 3 agent time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 已停止：当前计数窗口内已累计 3 次时间预算耗尽（含其间推送过的轮次；本轮本身可能以别的方式失败）。即 3 次完整 agent 运行没有推送任何内容。应由人工拆分或缩减该 PR（或同时提高 agent 时间预算与其步骤兜底），然后评论 Run log: https://github.com/QwenLM/qwen-code/actions/runs/33396430867 🧠 Handled by Qwen Code · model/模型 |
A posted review comment carries one attribution footer: the model writes one into its draft and the CLI strips that copy before appending the canonical, version-stamped one. The strip matches what GitHub displays, and in that projection an unterminated `<!--` runs to the end of the input. Inside a code fence it does not — GitHub renders fenced content literally — so a witness block quoting an HTML marker cut short took the trailing footer out of the projection with it, the strip found nothing to remove, and both footers posted. Blank fenced and indented code, delimiters included, before projecting — length-preservingly, so the cut still maps back to the original bytes. Which lines are code is the CommonMark parser's token map, not a hand model: the scan the line-aware strips shared disagreed with the renderers on lazy continuation, list content indents, tab stops and fence delimiters inside raw-HTML blocks, and under the blanking each disagreement kept a forged footer the render showed. markdown-it is already a packages/cli dependency; the hand-built scanner goes away. The one-line channels — compose-review's folded deferral titles, reroute records and ingested entries, submit's relocated claim — strip again after the fold: the collapse flattens a footer the blanking kept as quoted code into a single posted line.
d69deb9 to
5b1e510
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Taking this PR over from the autofix loop — label removed (the round-14 address leg had failed anyway). Shape decision. Fourteen rounds grew the diff from 79 to ~1,300 source lines, and from round 5 on the review itself was flagging that as a human call. Reading the rounds back: rounds 1–4 found real defects in the original fix (the hand scanner's lazy-continuation / list-indent / tab misclassifications, the What the branch is now (5b1e510, force-pushed): the original blanking fix + markdown-it block classification (the hand-built scanner is deleted) + the three fold-site strips. +617/−130 lines against Verification: the real witness comment (discussion_r3885165818) replayed through Open threads: replying to and resolving all 32. The ones about dropped machinery dissolve with it; the class findings (hand-modeled inline grammar, single-spelling marker phrase, a closed comment spanning a paragraph break) are pre-existing on 中文说明从 autofix loop 手里接管这个 PR——已摘掉 label(第 14 轮的 address leg 本来也失败了)。 形态决定。 14 轮评审把 diff 从 79 行涨到约 1,300 行源码,从第 5 轮起评审自己就在提示这需要人来拍板。回看各轮:第 1–4 轮找到的是原始修复里的真缺陷(手写扫描器在懒续行 / 列表缩进 / tab 上的误分类、围栏 info string 里的 分支现状(5b1e510,已 force-push):原始置空修复 + markdown-it 块级分类(手写扫描器整体删除)+ 三处折叠站点剥离。相对 验证: 真实肇事评论(discussion_r3885165818)回放进 未关闭线程: 32 条全部回复并 resolve。关于已删机器的随之消解;类级发现(手工建模的行内语法、标记短语单一拼写、跨段落的闭合注释)在 🤖 Generated with Claude Code — Claude Fable 5.1 |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
One blocking item — the full trace is in the Stage 2 comment above.
The duplicates channel is a fifth fold site that never reaches the new line strip: suggestionsDroppedAsDuplicates goes through strippedList → quotedProse → stripReviewFooter (which now blanks quoted code), and is then folded by boundDeferredLine / asListLine with no stripReviewFooterLine on the collapsed line. An entry whose trailing footer sits in an indented block or an unclosed fence is therefore kept by the blanking — your own new test pins exactly that behaviour — then flattened by the fold, so it posts a forged attribution above the canonical footer. On main that entry stripped, so this path regresses rather than improving.
The other four fold sites are covered, two of them transitively, so this is the one list that bypasses both ingestEntryList and toDeferredEntries.
Everything else I checked came out clean: the length-preserving blanking and its offset arithmetic, the projection gate, the reduced line-kind union and its three readers, the surviving hand check on HTML-block kinds, and the reuse of the parser this directory already treats as the authority. The shape decision after the reset is the right one, and the performance note in Stage 2 is explicitly not a merge condition.
一条阻塞项——完整追踪见上方 Stage 2 评论:重复项通道是第五个折叠站点,始终没有走到新的行级剥离,因此尾部 footer 位于缩进块或未闭合围栏内的条目会被置空保留、再被折叠压平,最终在规范 footer 上方发出一条伪造署名;在 main 上这个条目是会被剥掉的,所以这条路径是回退。其余部分核查均干净,重置后的形态决定是对的,Stage 2 里的性能说明明确不是合并条件。
The duplicates channel reaches its one-line fold through quotedProse alone — no ingest-time line strip ahead of it — so a forged footer the blanking keeps as quoted code was flattened into the posted list line and posted above the canonical footer. Every deferred, relocated, duplicate-dropped and cannot-tell exit folds through boundDeferredLine, so the folded-line strip lives there, ahead of the character cap whose ellipsis would break the trailing match. Two tests pin the channel.
|
Confirmed and fixed in a4ab212 — the duplicates channel was the one exit that reached its fold with no line strip ahead of it, and on this branch that regressed against Taken the choke-point route rather than a fifth per-site patch: the folded-line strip now lives in Two tests pin the channel — the three code-shaped entries (tab-indented, unclosed fence, four-space block) under both attribution legs, and the 213-char entries where the footer would straddle the cap — both red with the On the performance note: understood as a conscious choice. The parse is block-only ( 🤖 Generated with Claude Code — Claude Fable 5.1 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 68 passed · 0 failed · 68 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：68 通过 · 0 失败 · 68 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10458 verification (round 2) —
|
| # | Previous finding | Sev | Status at a4ab212b |
|---|---|---|---|
| — | Central A/B: base posts 2 attributions, head posts 1 | load-bearing | stands — re-proven at the new head on shapes A/B/C (§A/B #1) |
| — | Layered-guard matrix: base 342 dup → H2 alone 62 → H1 alone 132 → head 2 | load-bearing | superseded — H2 (stripFooterSpans on the ON leg) does not exist at this head. normalizeInlineComments is now byte-identical on both arms (sha256 fe72810f…), so there is only one guard, not two. The matrix cannot be re-run. |
| — | Gate failure: compose-review.test.ts > convergence telemetry … > clamps the count at its origin, Test timed out in 15000ms |
environmental | did not reproduce — gate is fully green: 117 files, 5754 passed, 17 skipped, exit 0, 32.4 s |
| F1 | ON leg still posts two attributions on bare-CR bodies with a mid-body forged footer (2/1641) | Suggestion, pre-existing | stands, and is larger than the previous round measured — the mid-body class is 149/918 at head vs 140/918 at base (§N2). The specific bare-CR trailing spelling is fine on both arms (shape D: head 1, base 1). |
| F2 | Appended canonical footer renders as code when the draft leaves a construct unclosed; swallowsAppendedMarker is called only on the OFF leg |
Observation, pre-existing | stands, identical on both arms — 176/1395 head vs 176/1395 base (assertion T7 pins the equality). submit.ts:568 still reads !attribution && swallowsAppendedMarker(stripped). |
| F3 | Declared tradeoff magnitude: head leaves more quoted footers rendering inside code (+113) | Note | stands — measured +126 on this round's corpus (489 vs 363 non-canonical markers inside code). Different corpus, so the magnitude is not directly comparable; direction and size agree. |
| — | Perf: 29.5 ms at 115 KB; mutant M5 (inline pass re-enabled) 29,692 ms against a 2000 ms test bound | verified | partly superseded — M5 now survives (0 red / 84 green). The 2000 ms bound that killed it at the old head is absent at this head. The ladder itself was not re-run (budget); see Not covered. |
| — | Mutation matrix 6/6 killed | verified | re-measured as 7/9 killed with a different mutant set matched to this head's guards. Positive control landed in the mutated file's own suite. |
Central claim and the A/B
Central claim. A drafted review comment whose witness block quotes an unterminated <!\-\- must post exactly one attribution footer: the model's own forged copy is stripped, the canonical version-stamped one is appended beside nothing, and no visible content is erased.
Control validity. Only three source files differ between arms. package.json, package-lock.json, packages/cli/package.json and packages/core are untouched; packages/core has the identical tree hash on both arms (0d42e4ac2ae07658543d8b8bd8861ed8a993b8b7), so resolving it through the head tree's workspace symlink is provably identical input rather than a confound. inline-counts.ts (96ee74f0…) and ledger.ts (792a8881…) are byte-identical. review-footer.ts differs: head 614a03d9…, base a857f150… — the base hash matches the previous round's base, so only the head moved between rounds. Each arm was esbuild-bundled self-contained; the base worktree needed packages/{core,cli}/node_modules linked in (non-hoisted deps), and both realpaths were asserted (/__w/qwen-code/qwen-code/packages/{core,cli}/node_modules) before any cell was trusted. require.resolve was not used — these packages are ESM-only with import-only exports and it throws ERR_PACKAGE_PATH_NOT_EXPORTED.
Oracle. Independent of the module under test on the axis where hand modelling still lives: render with markdown-it (html: true), then extract displayed text with parse5 (a WHATWG-conformant HTML parser), skipping comment nodes and script/style/template content. Block structure agrees with the module by construction (the module delegates to markdown-it's token map), so independence is on the HTML/render axis.
A/B #1 — at the destination
Each arm's posted body was built with normalizeInlineComments's expression lifted verbatim from submit.ts. At this head that expression is byte-identical on both arms, so one source serves both (the previous round had to use each arm's own).
| cell | head | base |
|---|---|---|
shape A (PR test-plan minimal): stripReviewFooter result |
strips 43 → 14 ch, ends at the fence | byte-identical, 43 → 43 (nothing stripped) |
shape A-control (quoted opener closed -->) |
strips | strips |
| shape A posted prose attributions | 1 | 2 |
shape B (details/summary witness block) posted prose attributions |
1 | 2 |
| shape C (indented code quoting the opener) | 1 | 2 |
| shape D (bare-CR twin of A) | 1 | 1 |
| shape E (mid-body forged footer + trailing prose) | 2 | 2 |
| attribution-OFF posted prose attributions, all 6 shapes | 0 | 0 |
| head strip output a byte prefix of the body, all 6 shapes | yes | — |
Witness: 01-ab-destination-base-posts-2-footers-head-1.png. Base's rendered HTML for shape A shows both footers posting:
<pre><code><!-- x
</code></pre>
<p><em>— m via Qwen Code /review</em></p>
<p><em>— qwen3-coder via Qwen Code /review (v1.2.3)</em></p>Head's shows one. The closed-opener control strips on base, confirming the PR's claim that the unterminated opener is the whole trigger.
A/B #3 — the named mechanism, isolated
15 shapes, each tagged with the class it belongs to. Head duplicates on 0 of the 10 code-quotation shapes; base on 9.
| class | n | base dup | head dup | fixed |
|---|---|---|---|---|
code quotation (fence, ~~~, info string, indented, blockquoted, list-indent, tab, CRLF, bare-CR) |
10 | 9 | 0 | 9 |
| canonical footer swallowed by the body's own unclosed construct | 2 | 0 | 0 | 0 |
| code span in prose | 1 | 0 | 0 | 0 |
declared limit — unterminated <!\-\- in ordinary prose, and its lazy-continuation twin |
2 | 2 | 2 | 0 |
Witness: 02-mechanism-table-9-of-10-code-shapes-fixed.png. The two declared-limit shapes are identical on both arms and are exactly what Risk & Scope names ("the projection still takes an unterminated <!\-\- in ordinary prose as running to the end of the input"). The lazy-continuation case is prose because the parser delegation now classifies it correctly — that is the fix working, not failing.
A/B #2 — corpus differential
1562 bodies built (900 synthesized sweeps + real repo docs/**.md chunks + real git diff --color=always bytes), 167 dropped as self-poisoned, 1395 measured. The drop matters: git diff HEAD^1..HEAD and this repo's own docs contain the literal marker phrase as source — it spells FOOTER_MARKER, the regexes and the comments in the very file under test — so those chunks displayed a marker no model ever forged and inflated both arms identically (a first cut scored gitdiff with no appended footer at 21). A differential over a corpus that already contains the answer measures nothing.
| cell (corpus = 1395) | head | base |
|---|---|---|
| threw on any body | 0 | 0 |
| P0 strip output not a byte prefix of the body | 0 | 0 |
| P1 visible word erased at the destination | 0 | 1 |
| P3 forged prose footer survived (ON leg, prose ≥ 2) | 278 | 283 |
| P4 attribution-OFF left visible attribution | 150 | 151 |
| F2 appended canonical footer displays as code | 176 | 176 |
| canonical footer displays as prose | 1151 | 1151 |
| F3 markers rendering inside code (declared tradeoff) | 489 | 363 |
| head over-stripped a word base displayed | 0 | — |
Witness: 03-corpus-differential-head-vs-base.png. P3's raw counts look marginal (278 vs 283) because the corpus is dominated by classes the trailing strip never covered on either arm; §A/B #3 and the decomposition below isolate the class the PR actually addresses.
Decomposed by where the forged footer sits:
| class | bodies | head dup | base dup |
|---|---|---|---|
| TRAILING (the trailing strip's scope) | 477 | 129 | 143 |
| MID-BODY (never in a trailing-anchored strip's scope) | 918 | 149 | 140 |
Corrections
To the PR description's mutation-verification numbers. Three of the five claimed counts do not match what I measured at this head; two match exactly. Stated as a correction to the description, not a request to change code:
| claim in "How to verify" | PR said | measured | |
|---|---|---|---|
no blanking (stripTrailingFooter(body, body)) |
7 red | 7 red (M1) | match |
| raw-HTML block kinds counted as visible HTML | 3 red | 13 red (M3) | understated |
the three compose-review ingest-site strips reverted |
3 red | 1 red (M7, ingestEntryList only) |
overstated — I mutated one site, not all three, so this is my scope, not necessarily a wrong claim |
the boundDeferredLine fold strip reverted |
2 red | 2 red (M6) | match |
the submit claim-line strip reverted |
1 red | 1 red (M8) | match |
To the test count. The description says "117 files, 5753 tests pass (18 skipped)"; measured at this head: 117 files, 5754 passed, 17 skipped (5771 total). Off by one in both directions — consistent with a slightly different head state when the description was written, not a mechanism claim.
Findings
N1 — Suggestion: the folded one-line channels are still blinded, on shapes the whole-body strip now closes
The PR's second commit adds stripReviewFooterLine precisely so the one-line channels strip after the fold, with the stated rationale "no blanking: a folded line has no block structure". That rationale addresses the code blind spot but not the comment one, and the comment one survives the fold.
Collapsing a multi-line entry concatenates a witness block's quoted <!\-\- onto the same line as the trailing forged footer. stripReviewFooterLine still runs projectInvisibles, which drops from an unterminated <!\-\- to end-of-input — so the $-anchored REVIEW_FOOTER_RE never sees the footer and the line is returned unchanged.
What GitHub displays settles it: markdown-it's inline HTML rule needs a closer, so an unterminated <!\-\- is escaped to literal text and the forged footer renders as prose, not as a quotation.
| multi-line entry | folded strip | displays prose attribution |
|---|---|---|
PR central shape: closed ``` fence quoting <!\-\- x |
stripped | 0 |
same with a ~~~ fence |
KEPT forge | 1 |
| same with an info string | stripped | 0 |
UNCLOSED fence quoting <!\-\- x |
KEPT forge | 1 |
indented code block quoting <!\-\- x |
KEPT forge | 1 |
CONTROL: closed fence, opener closed (-->) |
stripped | 0 |
| CONTROL: fence with no opener at all | stripped | 0 |
The rendered HTML is the proof it is visible, not quoted:
<ul>
<li><strong>Deferred</strong>: ~~~ <!-- x <em>— m via Qwen Code /review</em></li>
</ul>The contrast that makes this a sibling the fix missed rather than a claim the fix never made — the whole-body strip closes two of these three:
| shape | stripReviewFooter |
stripReviewFooterLine |
|---|---|---|
~~~ fence quoting <!\-\- x |
stripped | KEPT forge |
indented code block quoting <!\-\- x |
stripped | KEPT forge |
UNCLOSED fence quoting <!\-\- x |
KEPT forge | KEPT forge |
Bound — what this is not. The PR's own exact shape is *not* affected, and that is worth stating because it explains why the new tests do not catch this: a run of three backticks is masked as a code **span** by `projectInvisibles` before the comment rule runs, so the fold self-neutralizes. Only the ~~~ twin, the unclosed fence, and the indented block reach the comment rule. Both controls (opener closed; no opener) strip correctly. No content is erased anywhere: stripReviewFooterLine either strips the trailing footer or returns the line byte-identical.
Reachability. The duplicates channel reaches boundDeferredLine through quotedProse alone, and quotedProse loops on the whole-body stripReviewFooter — which now keeps a footer quoted in code. The fold then flattens it into one line that still contains the quoted <!\-\-. A deferred title, reroute record, ingested entry or duplicate-dropped entry whose text quotes an HTML marker is the same trigger the PR says already happened in production, arriving on a different channel.
Cost if unfixed. One duplicate attribution line on a list entry — the same cost class the PR itself names ("a miss costs a duplicate attribution line"). Not a Critical.
Reproduce: cd tmp/pr10458-verify-20260905-184337 && node sibling-fold.mjs. Witness: 05-folded-channel-sibling-still-forged.png.
Candidate direction (not applied, not measured as a fix)
The natural candidate is to give stripReviewFooterLine the same treatment the whole-body strip got — but a folded line has no block structure to classify, so blankQuotedCode does not transfer directly. Two narrower options: mask a folded line's backtick and tilde runs before projecting (which is what already saves the case), or treat an unterminated `<!\-\-` on a **single line** as literal text rather than as running to end-of-input, since that is what markdown-it's inline rule actually does. I did not build or measure either — budget went to bounding the defect. Whichever is chosen needs a fixture for the `~~~` twin and the indented block, because the shape that the current tests use cannot see this.
N2 — Note, bounded: dropping H2 leaves the ON leg with no mid-body strip, and head measures worse than base on 9 corpus bodies
At the previous round's head, the ON leg of normalizeInlineComments carried a second guard (stripFooterSpans), and the layered matrix showed neither half alone closed the corpus (H2 alone 62 dup, H1 alone 132, both 2). The reset dropped H2. At this head normalizeInlineComments is byte-identical on both arms, so mid-body forged footers are handled by neither arm.
Measured on the 918 mid-body corpus bodies: head 149 duplicates, base 140 — head is worse by 9. Per the rule that a declined or deferred row is not exempt from re-measurement, this is reported as worsened against the previously verified head, and as no worse in kind against base.
All nine are adjudicated, not waved away:
- Every one belongs to a single synthetic family: a mid-body entity-encoded marker (
/review) combined with a trailing 4-space-indented forged footer (assertionsR5,E4). - On all nine, head declined to cut rather than mis-cut —
stripReviewFooterreturned the body byte-identical (R4). - On all nine, base's cut erased a word head still displays (
R3, 9/9): base cut from the mid-body entity footer to end-of-input, destroying a trailing indented code block that GitHub renders visibly. Head preserves the block and leaves the mid-body footer. Base erased real prose on 0 of them. - Removing that one spelling from the corpus: head worse on 0 bodies, better on 14 (
E1,E2). - The attribution-OFF leg is never worse: 0 bodies (
R2).
So this is the PR's declared tradeoff behaving as declared — "over-stripping visible content is the failure mode this PR avoids" — and the residual cost is a mid-body forged footer the ON leg has no strip for. That gap is pre-existing on base (140 bodies) and is F1 from the previous round, not something this PR introduced. Listed so a reviewer sees that the reset traded a closed gap for a smaller, declared one.
Reproduce: node regression-probe.mjs then node ab-bound-worse.mjs.
N3 — Coverage gap: canProjectFooterMarker's < clause is load-bearing and nothing asserts it
Mutant M2 (drop || s.includes('<')) survived 208 green / 0 red. It is not dead code — I verified the clause decides real outcomes:
| body | contains /review? |
contains &? |
head strips it? |
|---|---|---|---|
finding\n\n_— m via Qwen Code /<!\-\- x -->review_ |
no | no | stripped |
finding\n\n_— m via Qwen Code /rev<!\-\- x -->iew_ |
no | no | stripped |
Both shapes assemble the marker phrase only by a dropped comment joining two literal halves — exactly the case the < clause exists to admit. Under M2 both would return early and post a forged footer. Classification: coverage gap (behaviour is right, nothing pins it). A fixture for either row above would kill M2.
N4 — Coverage gap: nothing pins the inline pass off, and head now parses the whole body
Mutant M5 (re-enable markdown-it's inline pass by deleting BLOCK_PARSER.core.ruler.disable(['inline'])) survived 84 green / 0 red in review-footer.test.ts. At the previous round's head this same mutant was killed by a 2000 ms test bound (re-enabling cost 29,692 ms); that test is absent at this reset head.
This matters more than an ordinary survivor because of what else changed: stripReviewFooter now calls blankQuotedCode → scanLines → a full-body markdown-it block parse, where base only ever projected the last STRIP_TAIL_LIMIT (8192) characters. The source comment states the inline pass is "quadratic on some one-line bodies". So the guard that keeps a quadratic pass off an untrusted-text path is now enforced only by a comment. Classification: coverage gap; the missing fixture is a timing bound on a parse-heavy body.
Not covered
- Performance ladder. Not run — budget was spent on the N1 sibling sweep and the mutation matrix. This is the round's most significant gap, because head moved a full-body markdown-it parse into
stripReviewFooter(§N4) and no timing assertion in this report bounds it. The previous round measured 29.5 ms at 115 KB on its head; that number does not transfer to this one and should not be quoted for it.perf-ladder.mjsis written and in the artifact dir (5 rungs × 5 hostile shapes, each rung in its own child under a 30 s cap, both arms) but was never executed. - The "strip runs BEFORE the cap" ordering claim. Not verified.
boundDeferredLinestrips ahead ofMAX_DEFERRED_SUGGESTION_CHARS(240) because the cap's ellipsis would break the$-anchored match. My fixture built a 327-char folded line whose cap cut landed inside the prefix at char 240, ~60 chars short of the footer, so neither "cap alone" nor "cap-then-strip" produced a footer fragment and the ordering was not discriminated. A correct fixture needs the prefix at ~215–235 chars so the cut lands inside the footer. The claim is plausible and unrefuted; it is simply untested here. - Three assertions from an early cut of the foldline harness were mis-specified and are not counted. Recorded rather than silently dropped:
L3expected the folded strip to close all 7 shapes — wrong, the correct count is 3 survivors, re-measured asG6;L7/L9were the cap-ordering fixture above. All three were removed fromassertions.jsonland their intent is either re-measured (L3→G6) or listed here (L7/L9). The first cut ofab-differential.mjsalso had two broken detectors (erasure/over-strip compared displayed text without whitespace normalization, so the appended footer's block-count change read as erasure) — they fired on both arms, which is how they were caught; the superseded log is kept atlogs/ab-differential.superseded-oracle-bug.log. - Per-commit attribution. The checkout is shallow. The snapshot lists 2 commits but
5b1e5102("keep quoted code from blinding the footer strip") is not locally reachable; onlya4ab212band the base tip are.git rev-list --count HEAD^1..HEAD^2returns1— the shallow-boundary trap — so it was not used as a reachability check; the snapshot comparison was. Everything above verifies the aggregateHEAD^1..HEADdiff. In particular, the blanking and the parser delegation (commit 1) and the fold-site strips (commit 2) could not be attributed separately. - The real production comment.
pull/10445#discussion_r3885165818was not fetched — this environment has no GitHub token and makes no network calls. A faithfuldetails/summaryreconstruction (shape B) stands in, so the shape is reproduced end-to-end at the destination, not the production bytes. The PR's stated 3006 → 2967 character figures were therefore not checked. - Live posting. The destination is reconstructed from
submit.ts's exact expression, not a realqwen reviewsubmit against GitHub. - Base-arm vitest. The base worktree cannot run the suite (its
globalSetupguard stops on unbuilt workspace packages), so the mutation matrix and gate ran head-only. This is sound here: a mutant is a head-source edit compared against head tests, and no base cell depends on a base test run. - Repo-wide gates. Only
cd packages/cli && npx vitest run src/commands/review/was run, plus the mutation runs. No repo-wide lint, typecheck, or full test suite. The PR claimseslint --max-warnings 0andprettier --checkpass on the six changed files; I did not re-run either. - Corpus. 1395 bodies after the self-poison filter — not the 60k the PR's own fuzz used, and not the PR's generator. Differential counts are over my corpus and are not comparable to the previous round's (1641 bodies, different generator).
- Windows/macOS line handling beyond the CRLF and bare-CR variants present in the corpus.
Methodology
Environment: the CI node:22-bookworm container at refs/pull/10458/merge, depth 2, with npm ci and npm run build already completed at head. Arms were compiled with esbuild (--bundle --platform=node --format=esm --loader:.wasm=binary plus a createRequire banner) from packages/cli/src/commands/review/lib/review-footer.ts into self-contained arm-{head,base}.mjs; the base side came from git worktree add tmp/base-tree HEAD^1 with packages/{core,cli}/node_modules symlinked in and both realpaths asserted. Arm exports were sanity-checked before any cell ran (stripReviewFooterLine present on head only; FOOTER_MARKER and reviewFooter identical on both). Harnesses (ab-repro.mjs, ab-differential.mjs, ab-decompose.mjs, regression-probe.mjs, ab-bound-worse.mjs, ab-foldline.mjs, sibling-fold.mjs, corpus.mjs, lib-oracle.mjs, lib-assert.mjs, mutation-matrix.sh) drive the compiled arms directly — real module code, no stubs — and the oracle renders through real markdown-it + parse5. Every mutant was an interface-preserving single-point source edit applied by exact-anchor replacement (the script aborts if an anchor does not occur exactly once), run against the test files that import it, then restored with its sha256 re-checked; the tree was confirmed clean afterwards (git status --porcelain empty, all three source hashes back to 614a03d9…/e654cac0…/f7aee700…). Mutant logs are kept both raw and ANSI-stripped, because vitest colorizes when redirected and an anchored grep of the summary line silently matches nothing — that bug produced a first all-survived matrix which was discarded, not reported. Raw per-cell logs, the mutation TSV, the corpus residual JSON, the assertion inventory, and the five evidence PNGs are in tmp/pr10458-verify-20260905-184337/.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/commands/review/compose-review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/compose-review.test.ts
file packages/cli/src/commands/review/lib/review-footer.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/review-footer.test.ts
file packages/cli/src/commands/review/submit.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/compose-review.test.ts: PPPPP
packages/cli/src/commands/review/lib/review-footer.test.ts: PPPPP
packages/cli/src/commands/review/submit.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 66 passed · 0 failed · 66 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：66 通过 · 0 失败 · 66 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10458 verification (round 3) —
|
| # | Round-2 finding | Sev | Status at this head |
|---|---|---|---|
| — | Central A/B: base posts 2 attributions, head posts 1 | load-bearing | stands, re-proven from scratch — base 2 → head 1 on shape A, and 5 sibling shapes flip (round 2 measured 3): details/summary fold, indented block, ~~~ fence, blockquoted fence, fence info string (§A/B #1) |
| — | Gate failure: convergence telemetry … clamps the count at its origin, Test timed out in 15000ms |
environmental | did not reproduce again — 117/117 files, 5754 passed, 17 skipped, exit 0, 33.5 s (T1) |
| — | Layered-guard matrix superseded (H2 gone, normalizeInlineComments byte-identical on both arms) |
load-bearing | still superseded — carried forward. Compared: the HEAD^1..HEAD diff touches submit.ts only at the import list and relocatedAoneCriticalEntry's claim; normalizeInlineComments is not in the diff, and the head/base commit OIDs are unchanged from round 2. The matrix still cannot be re-run. |
| — | Perf: 29.5 ms at 115 KB; M5 29,692 ms against a 2000 ms bound | verified | superseded by direct measurement. Those numbers were the pre-reset head's and round 2 correctly refused to transfer them. Measured here: head max 7.9 ms at 65,536 chars, exponent ≤ 0.77; M5 100.81 ms (§Perf, §N4) |
| — | Mutation matrix 7/9 killed | verified | partly re-measured. The full 9-mutant matrix was not re-run; the 2 survivors (M2, M5) were probed directly and both are now proven load-bearing rather than dead, with M1 as the positive control and the suite-level control landed in the mutated files' own suites (D4) |
| F1 | ON leg has no mid-body strip (2/1641 bare-CR; 149/918 mid-body at head vs 140/918 base) | Suggestion, pre-existing | not re-measured — see Not covered. The corpus generator is unrecoverable. Re-measured on one shape instead: mid-body forged footer + trailing prose is identical on both arms (2 prose markers each, A10), and the bare-CR twin of shape A strips on both (A15) |
| F2 | Appended canonical footer renders as code when the draft leaves a construct unclosed; swallowsAppendedMarker only on the OFF leg |
Observation, pre-existing | stands, identical on both arms — re-measured. Shape G (unclosed fence): head code=2 prose=0, base code=2 prose=0; the appended canonical footer lands inside the never-closed fence on both arms (A8) |
| F3 | Declared tradeoff magnitude: head leaves more quoted footers rendering inside code (+126) | Note | not re-measured — needs the corpus. Direction is visible on the shape set (shape G keeps 2 markers inside code on both arms) but the magnitude is not re-asserted |
| N1 | Folded one-line channels still blinded on 3 siblings (~~~, unclosed fence, indented) |
Suggestion | stands, re-measured exactly — and the candidate fix is now BUILT AND MEASURED. Same 3 survivors, same ids (G3). Fix: 14/14 cells clean, zero collateral, zero over-strip, whole-body strip untouched, suite counts unchanged (§N1) |
| N2 | Dropping H2 leaves the ON leg with no mid-body strip; head worse than base on 9 corpus bodies | Note, bounded | not re-measured — the 9-body adjudication was corpus-derived. Carried forward as round 2's measurement, attributed to round 2's corpus |
| N3 | canProjectFooterMarker's < clause load-bearing, nothing pins it |
Coverage gap | stands and strengthened. Both comment-split bodies strip at head (46→7) and are returned byte-identical under M2; at the destination M2 posts 2 prose attributions vs head's 1 (M2, M2b) |
| N4 | Nothing pins the inline pass off; head now parses the whole body | Coverage gap | stands, and its premise is corrected. The guard is load-bearing for cost — head 0.63 ms vs M5 100.81 ms at 65,536 chars, head ≤ 2.33 ms across all 15 ladder cells — but the growth is linear (exponent 0.91), not the "quadratic" the source comment claims (M5, M5b, M5d) |
Central claim and the A/B
Central claim. A drafted review comment whose witness block quotes an unterminated <!\-\- must post exactly one attribution footer: the model's own forged copy is stripped, the canonical version-stamped one is appended beside nothing, and no visible content is erased.
Secondary claims. (S1) the fold-site strips close the one-line channels, and the strip runs before the 240-char cap. (S2) delegating code classification to markdown-it's token map does not regress correctness or put an unbounded cost on untrusted text.
Control validity. Only three source files differ between arms; package.json, package-lock.json and both workspace manifests are untouched, so reusing the root node_modules for the base side is a clean control rather than a dependency confound. packages/core has the identical tree hash on both arms (0d42e4ac2ae07658543d8b8bd8861ed8a993b8b7), so resolving @qwen-code/qwen-code-core through the head tree's workspace symlink is provably identical input — that matters because inline-counts.ts imports it. Both packages/{core,cli}/node_modules realpaths were asserted inside the base worktree before any cell was trusted. require.resolve was not used: these packages are ESM-only with import-only exports and it throws ERR_PACKAGE_PATH_NOT_EXPORTED. Arm exports were sanity-checked first — head 19 keys with stripReviewFooterLine present, base 18 without it, FOOTER_MARKER and REVIEW_FOOTER_RE identical on both.
Oracle. Independent of the module under test on the axis where hand modelling still lives: render with markdown-it (html: true), then extract displayed text with parse5 (a WHATWG-conformant HTML parser), skipping comment nodes and script/style/template. Markers are counted separately in prose and in code — a marker inside <pre><code> is a quotation, not an attribution, and conflating the two turns the PR's declared tradeoff into a fake regression. Block structure agrees with the module by construction (the module delegates to markdown-it's token map), so the independence is on the HTML/render axis.
A/B #1 — at the destination (12 shapes, 20 assertions)
Each arm's posted body was built with normalizeInlineComments's expression lifted verbatim from submit.ts:293-320. Witness: 01-ab-destination-base-posts-2-footers-head-1.png.
| shape | what it is | base | head |
|---|---|---|---|
| A | PR test-plan minimal: closed ``` fence quoting an unterminated <!\-\- |
byte-identical, 43→43, 2 prose attributions | strips 43→14, 1 prose attribution |
| Ac | CONTROL: same fence, quoted opener closed (-->) |
strips, 1 | strips, 1 |
| An | CONTROL: same fence, opener neutralized (<!- -) |
strips, 1 | strips, 1 |
| B | witness block inside a details/summary fold (production shape) |
2 | 1 |
| C | indented code block quoting the opener | 2 | 1 |
| D | bare-CR twin of A | 1 | 1 |
| E | DECLARED LIMIT: unterminated opener in ordinary prose | 0 (identical) | 0 (identical) |
| F | ~~~ fence quoting the opener |
2 | 1 |
| G | UNCLOSED fence quoting the opener | code=2 prose=0 |
code=2 prose=0 (identical — F2 class) |
| H | blockquoted fence quoting the opener | 2 | 1 |
| I | opener lodged in a fence info string | 2 | 1 |
| J | mid-body forged footer + trailing prose | 2 | 2 (identical — outside a trailing strip's scope) |
Base's rendered HTML for shape A shows both footers posting; head's shows one:
<!\-\- BASE --> <!\-\- HEAD -->
<pre><code><!-- x <pre><code><!-- x
</code></pre> </code></pre>
<p><em>— m via Qwen Code /review</em></p> <p><em>— qwen3-coder via Qwen Code /review (v1.2.3)</em></p>
<p><em>— qwen3-coder via Qwen Code /review (v1.2.3)</em></p>Both controls strip on base, which is what isolates the trigger: the unterminated opener is the whole cause, exactly as the description claims (A5, A6). Four invariants hold on all 12 shapes: the attribution-OFF leg leaves 0 prose attributions on both arms (A11); head's strip output is a byte prefix of the input, so nothing is invented (A12); the rendered code content is byte-identical to the input wherever head strips, so no witness block is erased (A13); and head erases no prose word other than the marker (A14).
A/B #2 — S1, the cap-ordering claim (round 2: NOT VERIFIED → now verified)
Round 2's fixture put the 240-char cut inside the prefix, so neither order was discriminated. Instead of guessing a better fixture, the prefix length was swept across 195..250 (56 lengths) and the window mapped empirically. collapseToLine and boundDeferredLine are module-private, so both were lifted verbatim and parameterised only by the order; the real function is pinned by the suite (D2).
| region | lengths | strip-then-cap (head's real order) | cap-then-strip (counterfactual) |
|---|---|---|---|
| no cut (folded ≤ 240) | 18 | agrees | agrees |
| cut lands inside the footer | 1 (prefixLen=213) |
no marker posted | posts _— m via Qwen Code /review… |
| cut lands in the prefix | 11 | agrees | agrees |
Head's real order leaks the marker on 0/56 lengths (C2); the counterfactual leaks inside the window (C3), so C2 is not vacuous. Round 2's fixture sat in the third region, which is precisely why it could not discriminate (C4). The cap still bounds the line and the ellipsis appears iff the cap truncated the stripped line (0/56 violations, C5), and the surrogate-pair guard holds with an astral char straddling the cut at pads 236–239 (C6).
A/B #3 — S2, the perf ladder (round 2 wrote it, never ran it)
8 shapes × 5 rungs (2 k / 3 k / 5 k / 20 k / 65,536 chars) × 2 arms = 80 runs, each in its own child under a 30 s cap. Witness: 02-perf-ladder-head-linear-base-bounded.png.
| cell | head | base |
|---|---|---|
| rungs hitting the 30 s cap | 0/80 | 0/80 |
| throws / errors | 0 | 0 |
| growth exponent, 2 k → 65,536 (1.0 = linear, 2.0 = quadratic) | max 0.77 over 7 marker-bearing shapes | — |
| worst absolute cost at 65,536 chars (GitHub's comment cap) | 7.9 ms (7.9–10.1 ms across the ladder's three valid runs) | 0.8 ms |
| absolute cost at cap on every marker-bearing shape | ≤ 7.9 ms | ≤ 0.76 ms |
marker-free 65 k body (canProjectFooterMarker early return) |
~0 ms (vs 6.21 ms for a marker-bearing one) | 0.5 ms |
fixpoint-loop proxy at 65,536 (quotedProse's shape) |
2 iterations, worst 27.0 ms total | — |
Two shapes are worth reading off the table directly, because they show why base looks flat: on blockquote-fences and list-fences base reads 0.0 ms at every rung — not because it is faster, but because their tails contain an unterminated <!\-\-, so projectInvisibles jumps straight to end-of-input and does almost no work. That is the very short-circuit this PR fixes, showing up as a timing artefact on the control arm.
What this settles, and what it does not. The scaling hazard round 2 flagged is disproved: head's full-body markdown-it block parse is at most linear, and at the largest body GitHub accepts it costs single-digit milliseconds. The input is untrusted — the review body is the model's draft, but it quotes the PR diff, PR body and file contents, all authored by a fork contributor — so the ladder was run on that premise and traced to that writer.
What is nonetheless true and worth a reviewer's eye: STRIP_TAIL_LIMIT no longer bounds total work. Base projected only the last 8,192 characters, so its cost was O(1) in body length by design; head's blankQuotedCode → scanLines → BLOCK_PARSER.parse(body) runs over the whole body, making it O(n). That is a deliberate, necessary consequence of the fix (a fence's state is only knowable from where it opened, as the new comment says), the measured ceiling is ~10 ms at the hard cap, and it is recorded as a Note (§N5), not a defect.
Corrections
Stated as corrections to text, not as requests to change code.
1. To a source comment — "quadratic" overstates the measured cost. review-footer.ts states the inline pass is "pure cost (and quadratic on some one-line bodies)". Re-enabling it (mutant M5) and laddering a hostile one-line body — a run of [ with no ], found by screening 13 candidate kinds — gives 9.65 ms at 5 k → 100.81 ms at 65,536, an exponent of 0.91: linear with a large constant, not quadratic. The guard is genuinely load-bearing (head is 0.63 ms on the same input, ≤2.33 ms across all 15 cells), so nothing here argues for removing it; the wording just promises a steeper curve than the code produces. Worth knowing because a future reader may assume a 64 KB body costs seconds.
2. To the description's test count. "117 files, 5753 tests pass (18 skipped)" — measured 117 files, 5754 passed, 17 skipped (5771 total), exit 0 (T3). Off by one in both directions, same as round 2; consistent with a slightly different head state when the description was written, not a mechanism claim.
3. In the description's favour — two of its mutation claims are exact. Round 2 found three of five mismatched. The two it re-ran here are confirmed precisely, including file attribution: "no blanking → 7 tests red across review-footer.test.ts and submit.test.ts" measured 7 red in exactly those two files (D1); "the boundDeferredLine fold strip reverted → 2 tests red (the duplicates channel, with and without the character cap in play)" measured 2 red, whose names are strips a forged footer the blanking kept inside a code shape — the strip runs AFTER the fold and strips a duplicates entry before the 240-char bound cuts the footer (D2). Both fail the intended assertion rather than breaking an import or a fixture: 5747 + 7 = 5754 and 5752 + 2 = 5754, matching the head baseline's 5754 passed (D3).
4. To round 2's own perf framing. Round 2 carried forward "29.5 ms at 115 KB" from the pre-reset head while correctly warning it did not transfer. It is now replaced by measurement at this head and should no longer be quoted for it.
Findings
N1 — Suggestion (stands): the folded one-line channels are still blinded on three siblings, and the candidate fix is now measured
stripReviewFooterLine exists so the one-line channels strip after the fold, with the stated rationale "no blanking: a folded line has no block structure". That rationale addresses the code blind spot but not the comment one. Collapsing a multi-line entry concatenates a witness block's quoted <!\-\- onto the same line as the trailing forged footer, and projectInvisibles still drops from an unterminated <!\-\- to end-of-input — so the $-anchored REVIEW_FOOTER_RE never sees the footer and the line is returned unchanged.
What GitHub displays settles that this is visible rather than quoted: markdown-it's inline HTML rule needs a closer, so an unterminated <!\-\- is escaped to literal text and the forged footer renders as prose.
The posted context is load-bearing, and getting it wrong hides the defect. A first cut of this round's harness posted the bare folded line as - ${line}; a title beginning with ~~~ or ``` then opened a real fence inside the list item, the marker vanished into an info string, and the sweep reported 0 survivors — reading as "N1 no longer reproduces". The real channels never post a bare title: renderDeferredEntry (`compose-review.ts:694-713`) returns `${loc}${agg} — [${entry.source}]${classified} ${entry.title}` and `boundDeferredLine` folds that whole line, so the deferred channel's folded line is always a paragraph. Measured in that faithful context:
| multi-line title (deferred context) | head fold strip | posted prose attribution | with candidate fix |
|---|---|---|---|
PR central shape: closed ``` fence quoting <!\-\- x |
stripped | 0 | 0 |
same with a ~~~ fence |
KEPT forge | 1 | 0 |
| same with an info string | stripped | 0 | 0 |
UNCLOSED fence quoting <!\-\- x |
KEPT forge | 1 | 0 |
INDENTED code block quoting <!\-\- x |
KEPT forge | 1 | 0 |
CONTROL: closed fence, opener closed (-->) |
stripped | 0 | 0 |
| CONTROL: fence with no opener at all | stripped | 0 | 0 |
Exactly round 2's three survivors (G3). The rendered HTML is the proof it is prose, not a quotation:
<!\-\- head -->
<li>packages/cli/src/commands/review/lib/review-footer.ts:441 — [review] ~~~ <!-- x ~~~ <em>— m via Qwen Code /review</em></li>
<!\-\- candidate fix -->
<li>packages/cli/src/commands/review/lib/review-footer.ts:441 — [review] ~~~ <!-- x ~~~</li>The contrast that makes this a sibling the fix missed rather than a claim it never made: the whole-body stripReviewFooter does close the ~~~ and indented shapes, while stripReviewFooterLine does not — the two channels disagree on the same input (G4).
Bound — what this is not. The PR's own exact shape is *not* affected, which is why the new tests cannot see this: a run of three backticks is masked as a code **span** by `projectInvisibles` before the comment rule runs, so the fold self-neutralizes. Both controls strip correctly. Nothing is erased anywhere — stripReviewFooterLine either strips the trailing footer or returns the line byte-identical. Cost if unfixed is one duplicate attribution line on a list entry, the same cost class the PR itself names; not a Critical.
New bound this round (G10). The duplicates channel differs: asListLine(boundDeferredLine(entry)) posts a bare entry, and when its folded line begins with a fence delimiter it opens a real code block, or — for the indented shape, whose 4-space indent collapseToLine trims away — begins with <!\-\- and becomes a raw-HTML block. Either way the marker does not post as prose there (head prose=0 code=0, kept). So N1's prose-attribution consequence is specific to contexts where the folded line is a paragraph, which the deferred channel guarantees by construction.
Reachability. A deferred title, reroute record, ingested entry or duplicate-dropped entry whose text quotes an HTML marker cut short is the same trigger the PR says already happened in production, arriving on a channel commit 2 was added to close.
Reproduce: cd tmp/pr10458-verify-20260905-193845 && node sibling-fold.mjs. Witness: 04-n1-folded-siblings-survive-and-measured-fix.png.
Candidate fix — built, applied, and measured (round 2 named it but did not build it)
Round 2 offered two directions and measured neither. The render-faithful one is implemented here: on a single line, an unterminated <!\-\- is projected as literal text rather than as running to end-of-input, because that is what markdown-it's inline HTML rule does — it requires a closer, so an unterminated opener is escaped to <!-- and the rest of the line renders as prose. Threaded as a singleLine flag from stripReviewFooterLine → stripTrailingFooter → projectInvisibles, defaulting false, so the multi-line path and the declared aggressive reading the attribution-off strip relies on are untouched by construction. Five exact-anchor replacements; the build script aborts if any anchor is not unique.
-function projectInvisibles(input: string): Projection {
+function projectInvisibles(input: string, singleLine = false): Projection {
if (ch === '<' && input.startsWith('<!\-\-', i)) {
const close = input.indexOf('-->', i + 4);
+ if (close === -1 && singleLine) {
+ // A folded line has no closer, so markdown-it's inline HTML rule does
+ // not fire and the opener renders as literal text.
+ push('<!\-\-', i, i + 4);
+ i += 4;
+ continue;
+ }
i = close === -1 ? n : close + 3;
-return stripTrailingFooter(line, line);
+return stripTrailingFooter(line, line, true);Measured on all four axes, not eyeballed:
| axis | result |
|---|---|
| hostile fixtures go clean | 14/14 cells (7 shapes × 2 posted contexts) post 0 forged prose attributions (G5) |
| benign fixtures byte-identical (zero collateral) | 12/12 benign folded lines carrying no forged footer, incl. a closed <!\-\- … -->, an unterminated opener mid-line, entities, generics, a lone -->, nested backtick/tilde runs (G7) |
| zero over-strip | 3/3 lines whose marker is legitimately quoted in a one-line code span are left alone, exactly as at head; the marker still renders inside <code> (G8) |
| whole-body strip untouched | 7/7 multi-line titles: stripReviewFooter output byte-identical to head (G9) |
| affected suite counts unchanged | 5754 passed / 17 skipped / 117 files with the patch applied — identical to head (T2) |
The suite is green both with and without the patch, so per the unpinned-axis rule the fixture that would pin it is named rather than implied: a stripReviewFooterLine case using a ~~~ fence or an indented block quoting an unterminated <!\-\-, asserting the forged footer is stripped. The existing tests use the ``` shape, which cannot see this. The fix should ship with that fixture.
Reproduce: bash build-fix.sh (bundles arm-fixn1.mjs from a scratch copy next to the original so ./inline-counts.js resolves) then bash run-fix-suite.sh (applies to the real tree, runs the suite, restores, verifies sha256).
N3 — Coverage gap (stands, strengthened): canProjectFooterMarker's < clause is load-bearing and nothing asserts it
Round 2 found mutant M2 (drop || s.includes('<')) survives the suite. It is not dead code, and this round measured the consequence rather than inferring it. Two bodies assemble the marker phrase only by a dropped comment joining two literal halves, so neither contains a literal /review nor an & — the < clause is the sole reason they are admitted:
| body | literal /review? |
&? |
<? |
head | M2 | prose attributions posted (head / M2) |
|---|---|---|---|---|---|---|
finding\n\n_— m via Qwen Code /<!\-\- x -->review_ |
no | no | yes | strips 46→7 | byte-identical | 1 / 2 |
finding\n\n_— m via Qwen Code /rev<!\-\- x -->iew_ |
no | no | yes | strips 46→7 | byte-identical | 1 / 2 |
Classification: coverage gap — the behaviour is right, nothing pins it. Either row above as a fixture kills M2 (M2, M2b).
N4 — Coverage gap (stands, premise corrected): nothing pins the inline pass off, and head now parses the whole body
Mutant M5 (delete BLOCK_PARSER.core.ruler.disable(['inline'])) survives the suite at this head; the 2000 ms bound that killed it at the pre-reset head is gone. The guard is load-bearing for cost:
| one-line body | chars | head | M5 (inline re-enabled) |
|---|---|---|---|
[ run (hostile) |
5,000 | 0.78 ms | 9.65 ms |
[ run (hostile) |
20,000 | 1.17 ms | 36.05 ms |
[ run (hostile) |
65,536 | 0.63 ms | 100.81 ms (160×; 108–205× across four measurements — head's sub-ms timing is the noisy side, so quote M5's absolute) |
* run |
65,536 | 1.07 ms | 43.37 ms |
*a pairs |
65,536 | 0.64 ms | 37.17 ms |
Head stays ≤ 2.33 ms across all 15 cells (M5b). M5 is behaviour-preserving on every correctness fixture (M5c), so it is a cost guard: the missing fixture is a timing bound, not an output assertion. A first screen over 13 candidate kinds mattered here — plain, words and backticks move only 1–5×, so a ladder built on them reported the guard as not load-bearing, which would have been a wrong conclusion caused by a benign shape.
This matters more than an ordinary survivor because of what else changed: stripReviewFooter now calls a full-body parse where base projected only the last 8,192 characters, so the guard that keeps a costly pass off an untrusted-text path is enforced only by a comment — a comment whose "quadratic" wording §Corrections 1 shows is inaccurate. Classification: coverage gap.
Witness: 03-m5-inline-pass-guard-is-load-bearing-for-cost.png.
N5 — Note (new this round): STRIP_TAIL_LIMIT no longer bounds total work
Not a defect, recorded so a reviewer sees the design change rather than discovering it later. Base's stripReviewFooter cost was independent of body length by construction — it sliced the last 8,192 characters and projected only those. Head's canProjectFooterMarker scan and blankQuotedCode → scanLines → BLOCK_PARSER.parse(body) both run over the whole body, so cost is now O(n) with a measured ceiling of ~8 ms at GitHub's 65,536-character comment cap and at most linear growth (§A/B #3). The tail bound the regex needs is untouched, and the change is necessary — a fence's state is only knowable from where it opened. Two consequences worth naming: the constant named STRIP_TAIL_LIMIT now bounds only the regex's reach and no longer bounds the function's work, and quotedProse calls stripReviewFooter in a fixpoint loop, so per-call cost is multiplied (measured proxy: 2 iterations, worst 27.0 ms total at 65,536 chars; quotedProse is module-private, so the proxy is a lower bound on the iteration count).
Not covered
- Round 2's corpus differential (1395 bodies; the P0–P4 invariants, F1's 149/918-vs-140/918 mid-body counts, F3's +126 tradeoff magnitude, and N2's nine-body adjudication). Not re-measured. Round 2's corpus generator lived in its artifact dir and the workflow does not carry harnesses forward —
tmp/did not exist when this round started. The tree half of the input closure is provably identical (same head/base commit OIDs, same source sha256s as round 2), but a measurement also consumes its own generator, and that is not recoverable, so those numbers are attributed to round 2 and deliberately not re-asserted here. What was re-measured instead: the mid-body class on one shape (A10, identical arms) and the F2 class on one shape (A8, identical arms). - Round 2's full 9-mutant matrix. Not re-run. This round probed the 2 survivors (M2, M5), landed M1 as the positive control, and validated 2 of the description's 5 mutation claims at suite level. The other three claims — raw-HTML block kinds counted as visible HTML (round 2: 13 red vs 3 claimed), the three
compose-reviewingest-site strips (round 2: 1 red from mutating one site, so round 2 flagged its own scope rather than a wrong claim), and thesubmitclaim-line strip (1 red) — carry round 2's numbers, unre-measured. - The real production comment
pull/10445#discussion_r3885165818. Not fetched: no GitHub token, no network calls. Shape B is a faithfuldetails/summaryreconstruction, so the shape is reproduced end-to-end at the destination, not the production bytes. The description's stated 3006 → 2967 character figures were therefore not checked. - Live posting. The destination is reconstructed from
submit.ts's exact expression, not a realqwen reviewsubmit against GitHub. - Per-commit attribution. The checkout is shallow (depth 2). The snapshot lists 2 commits but
5b1e5102is not locally reachable (git cat-file -tfails), and neither is the snapshot'sbaseRefOid9b188ea3.git rev-list HEAD^1..HEAD^2returns1— the shallow-boundary trap — so it was not used as a reachability check; comparison against the snapshot'scommitsarray was. Everything above verifies the aggregateHEAD^1..HEADdiff; the blanking and parser delegation (commit 1) could not be attributed separately from the fold-site strips (commit 2). - The duplicates channel's
stripCommentGrammar.asListLineruns it afterboundDeferredLine, but it is module-private and not exported by the arm, so my duplicates-context render omits it.G10's bounding conclusion could shift for entries whose comment grammar is neutralized before rendering. quotedProse's real loop. Module-private; §N5's iteration count is astripReviewFooter-only fixpoint proxy and a lower bound.- Base-arm vitest. The base worktree cannot run the suite (its
globalSetupguard stops on unbuilt workspace packages), so mutation and gates ran head-only. Sound here: a mutant is a head-source edit judged by head tests, and no base cell depends on a base test run. - Repo-wide gates. Only
cd packages/cli && npx vitest run src/commands/review/pluseslint/prettieron the 6 changed files. No repo-wide lint, nonpm run typecheck, no full test suite, no integration tests. - Windows/macOS line handling beyond the CRLF and bare-CR variants exercised as shapes A/D.
- Flakiness gate. Not run by me; the workflow runs its own.
Methodology
Environment: the CI node:22-bookworm container at refs/pull/10458/merge, depth 2, with npm ci and npm run build already completed at head. Both arms were esbuild-bundled (--bundle --platform=node --format=esm --loader:.wasm=binary plus a createRequire banner) from packages/cli/src/commands/review/lib/review-footer.ts into self-contained arm-{head,base}.mjs; the base side came from git worktree add tmp/base-tree HEAD^1 with packages/{core,cli}/node_modules linked in and both realpaths asserted before any cell was trusted, and was removed with git worktree remove --force once the cells were captured. Mutant and candidate-fix arms were built from scratch copies placed next to the original (__mutant-*.ts, __fix-n1.ts) because a copy under tmp/ cannot resolve the relative ./inline-counts.js; each mutation is an interface-preserving single-point edit applied by exact-anchor replacement that aborts if the anchor is not unique, and the scratch file is deleted immediately after bundling. Harnesses (ab-destination.mjs, cap-order.mjs, perf-ladder.mjs + perf-rung.mjs, mutation-probe.mjs + time-one.mjs, sibling-fold.mjs, desc-claims.mjs, gates.mjs, merge-assertions.mjs) drive the compiled arms directly — real module code, no stubs — and the oracle renders through real markdown-it + parse5, counting markers separately in prose and in code. Every timing cell ran in its own child under timeout 30, so a hang would be a recorded >30 s result rather than a dead harness; P0 asserts all 80 ladder runs produced a finite measurement before any growth conclusion is drawn from them, because a first cut ran with a broken rung script and one assertion then "passed" vacuously on an empty list. Four assertions were mis-specified in first cuts and are corrected in place with the reason recorded in the harness source (A8, C5, P1, P2) plus two parse bugs (gates.mjs and desc-claims.mjs both needed ANSI stripping / FAIL-header matching — vitest colorizes when redirected, so an anchored regex on the summary line silently matches nothing); none was silently dropped, and no mis-specified assertion is counted in assertions.json. In-place tree mutations (the candidate-fix suite run, both lint liveness probes, both description-claim mutants) were each restored and verified by sha256 — review-footer.ts 614a03d9…, compose-review.ts e654cac0…, submit.ts f7aee700… — and git status --porcelain is empty. Evidence images were produced with node scripts/verify-capture.mjs (no browser, no pseudo-terminal), one per headline claim, each a live re-run of the harness it depicts; every number in §A/B #3, §N4 and §Corrections 1 is taken from the run whose image is published, because sub-millisecond timings drift between runs (head's worst-at-cap measured 7.9–10.1 ms across the ladder's three valid runs, and M5's ratio 108–205× across four measurements, which is why those cells quote absolutes and name the spread). Two capture re-runs were needed for process reasons worth recording: piping a harness into the capture helper truncates output, because Node's stdout is asynchronous on a pipe and these harnesses end in process.exit() — a file redirect makes the writes synchronous, so print-n1.sh exists; and trimming the rebuildable mutant bundles to shrink the artifact broke mutation-probe.mjs's imports, so one capture came back 21 rows tall and was discarded, rebuilt and re-run. A near-miss on the reading side is also recorded: a stale perf-ladder.stdout.log from the first, broken ladder run still showed a table of err and a grep … || sed … fallback silently preferred it, which briefly looked like the published image contradicting its own assertions; the stale file is deleted, and the authoritative per-run records are logs/*.log, logs/*.json and logs/*.assertions.jsonl, all written by the same run at the same millisecond. Raw per-cell logs, the assertion inventory, the ladder JSON and the suite logs are in tmp/pr10458-verify-20260905-193845/logs/. The mutant and fix arm bundles were deleted from the artifact to keep it at 43 MB; build-mutants.sh and build-fix.sh regenerate them in ~20 s.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/commands/review/compose-review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/compose-review.test.ts
file packages/cli/src/commands/review/lib/review-footer.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/review-footer.test.ts
file packages/cli/src/commands/review/submit.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/compose-review.test.ts: PPPPP
packages/cli/src/commands/review/lib/review-footer.test.ts: PPPPP
packages/cli/src/commands/review/submit.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
APPROVE (verified at head a4ab212)
The long review loop and its final blocker
After ~12 review rounds, the last CHANGES_REQUESTED (at the previous head 5b1e5102) carried exactly one blocker: the duplicates channel reached the fold through quotedProse alone, so a footer that the new multi-line strip legitimately KEEPS as quoted code (indented block / unclosed fence) got flattened by the collapse into a posted line — a forged attribution above the canonical footer, a regression against main. The head commit fixes it at the shared boundary rather than per site: boundDeferredLine — which all four one-line exits already pass through — now applies stripReviewFooterLine after the collapse and BEFORE the cap (the ordering reasoned out: the cap's ellipsis would break the $-anchored match inside a footer), with a 54-line regression test. The requesting bot then approved this exact head with CI green, and no review on the head carries an open item.
My Critical-only scan of the full production delta
- review-footer.ts rewrite: the hand-rolled line classifier (which disagreed with renderers on lazy continuation, tab stops, and fences inside raw HTML — each round's finding) is replaced by the markdown-it CommonMark token map with
html: trueand the inline pass disabled; quoted code is blanked to same-length NULs before the trailing-tail match, and I checked the index arithmetic of that length-preservation (body.slice(0, body.length - tail.length + keep)equals the old two-slice form for a projection that is byte-identical in length). The newcanProjectFooterMarkerfast path admits exactly the characters the projection can assemble from (/review,&,<) — a conservative pre-filter whose failure direction is "run the full strip anyway". - The folded-line strip is deliberate and documented: a collapsed line carries no block structure, so fence-delimiter-looking text on it is posted content and must not blind the line strip — the opposite of the multi-line case, correctly split into two functions.
- Every fold site now double-strips (multi-line first, line-level after collapse) —
toDeferredEntries,floorEnforcedReroute,ingestEntryList,boundDeferredLine, and submit's relocated-claim exit — with thebodyCriticalsfree-form exit the single, explicitly-explained exception (entries quoted as-is, never folded, so the flattened-forgery shape cannot arise there). - Perf note (parser per call) was ruled a non-merge-condition by the reviewer itself; the module-level parser and fast-path precheck make it bounded.
CI at head
15 green (including the lanes that exercise the changed CLI package), zero failures; one label meta-run cancelled (fleet-side, non-attributable) and one still in progress — neither gates.
…text A folded one-line entry has no later line for a `-->` to sit on, so CommonMark's inline HTML rule never fires and GitHub escapes an unterminated `<!--` to literal text — the forged footer after it renders as prose. The projection read it as a comment running to the end of the input, the reading the multi-line strips need, so a witness block's quoted opener folded onto the footer's line hid the footer from every one-line channel. The projection now takes the reading from its caller: stripReviewFooterLine reads literally, the multi-line strips swallow. Two coverage gaps closed alongside: the `<` arm of the marker gate is pinned by a comment-split phrase, and the block-only parse is pinned by a latency bound on a hostile one-line body.
|
Thanks — the round-3 sandboxed report was worth acting on. Addressed in 45481e0:
Suite: 🤖 Generated with Claude Code — Claude Fable 5.1 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 47 passed · 0 failed · 47 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：47 通过 · 0 失败 · 47 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10458 verification (round 4) —
|
| # | Round-3 finding | Sev | Status at this head |
|---|---|---|---|
| — | Central A/B: base posts 2 attributions, head posts 1 | load-bearing | stands, re-proven — shape A base 43→43 with 2 prose attributions, head 43→14 with 1; 6/8 central shapes flip 2→1 (§A/B #1) |
| — | Gate failure (convergence telemetry…, Test timed out in 15000ms) |
environmental | did not reproduce — 117/117 files, 5758 passed, 17 skipped, exit 0, 32.85 s (D0) |
| — | Layered-guard matrix superseded (H2 gone) | load-bearing | still superseded — not re-run. The mutant H2 named no longer exists in the code, so there is nothing to revert; submit.ts is byte-identical to round 3 (f7aee700…) but review-footer.ts is not, so the closure is not identical and the measurement was not carried forward either |
| — | Perf: 29.5 ms at 115 KB; M5 29,692 ms vs a 2000 ms bound | verified | re-measured at this head — worst 10.034 ms at 65,536 chars, 13.658 ms at 262,144; growth exponent ≤ 0.41 (§Perf) |
| — | Mutation matrix 7/9 killed, survivors M2 and M5 | verified | both survivors now killed. M2 → 1 red, M5 → 1 red, with the positive control (7 red) landed in the same files (§Mutations) |
| F1 | ON leg has no mid-body strip | Suggestion, pre-existing | not re-measured — corpus generator unrecoverable. Re-measured on one shape instead: mid-body forged footer + trailing prose is identical on all three arms (shape J, prose=2 everywhere; §A/B #1) |
| F2 | Appended canonical footer renders as code when the draft leaves a construct unclosed | Observation, pre-existing | stands, identical on all arms — re-measured. Shape G (unclosed fence): prose=0 code=2 on base, r3 and head (§A/B #1) |
| F3 | Declared-tradeoff magnitude (+126 quoted footers left in code) | Note | not re-measured — needs the corpus. Direction visible on the shape set (shape G keeps 2 markers inside code on all arms); magnitude not re-asserted |
| N1 | Folded one-line channels blinded on 3 siblings | Suggestion | FIXED — and the fix shipped with the fixture round 3 named. 0 forged prose attributions across all 28 cells (base 10, r3 2). Reverting the landed hunk turns exactly the 2 new tests red (N1b, D1, D2) |
| N2 | Dropping H2 leaves the ON leg with no mid-body strip | Note, bounded | not re-measured — corpus-derived; attributed to round 2 |
| N3 | canProjectFooterMarker's < clause load-bearing, nothing pins it |
Coverage gap | FIXED. The fixture round 3 named is now in the tree (a comment splitting the marker phrase is seen through — the '<' gate admits it); dropping the < arm turns exactly that 1 test red (D4, D5) |
| N4 | Nothing pins the inline pass off | Coverage gap | FIXED. A timing fixture now bounds it (the block-only parse stays cheap on a hostile one-line body, 40 ms budget × 5 pool multiplier); re-enabling the inline pass turns exactly that 1 test red at 437 ms (D6, D7) |
| N5 | STRIP_TAIL_LIMIT no longer bounds total work |
Note | stands as a Note, re-measured. The worst head/base cost ratio grows 4.93× at 2 k → 10.44× at 262,144 over a 131× size increase, absolute ceiling 13.658 ms (P4, P1) |
| — | Round-3 Correction 1: the source comment's "quadratic" overstates the cost | Correction | acted on. The comment now reads "two orders of magnitude on hostile one-line bodies (a 256 KiB run of [ measured ~1 ms block-only, ~400 ms with the inline pass; linear either way)" — the inaccurate word is gone, and the figures match this round's measurement (P3 exponent 0.41; D6 mutant at 437 ms) |
Central claim and the A/B
Central claim. A drafted review comment whose witness block quotes an unterminated <!\-\- must post exactly one attribution footer: the model's forged copy is stripped, the canonical version-stamped one lands beside nothing, and no visible content is erased.
Secondary claims. (S1) the fold-site strips close the one-line channels, and the fold now reads an unterminated opener as literal text. (S2) delegating code classification to markdown-it and parsing the whole body does not regress correctness or put an unbounded cost on untrusted text.
Control validity. Only review-footer.ts differs between head and r3; between head and base, three source files differ. package.json, package-lock.json and both workspace manifests are untouched by git diff HEAD^1..HEAD, so reusing the root node_modules for the base side is a clean control rather than a dependency confound. packages/core has the identical tree hash on both arms (0d42e4ac2ae07658543d8b8bd8861ed8a993b8b7), and inline-counts.ts is byte-identical base vs head — that matters because review-footer.ts imports both. readlink -f node_modules/@qwen-code/qwen-code-core was asserted inside the base worktree before any cell was trusted; require.resolve was not used (these packages are ESM-only with import-only exports and it throws ERR_PACKAGE_PATH_NOT_EXPORTED). Arm exports were censused first — head 19 keys with stripReviewFooterLine, r3 19 with it, base 18 without — and the r3 arm's fold probe returns the unstripped line while head's returns the stripped one, confirming the reconstruction differs by exactly the intended edit.
Oracle. Independent of the module under test on the axis where hand modelling still lives: render with markdown-it (html: true), then extract displayed text with parse5 (a WHATWG-conformant HTML parser), skipping comment nodes and script/style/template. Markers are counted separately in prose and in code, because a marker inside <pre><code> is a quotation, not an attribution — conflating them turns the PR's declared tradeoff into a fake regression.
A/B #1 — attribution ON, at the destination (12 shapes, 10 assertions)
Posted body built with normalizeInlineComments's expression lifted verbatim from submit.ts:293-320: ${stripReviewFooter(body)}\n\n${reviewFooter('qwen3-coder','1.2.3')}.
| shape | base | r3 | head |
|---|---|---|---|
A — closed ``` fence quoting an unterminated <!\-\- (the PR's minimal repro) |
43→43, prose=2 | 43→14, prose=1 | 43→14, prose=1 |
Ac — CONTROL, quoted opener closed (-->) |
prose=1 | prose=1 | prose=1 |
An — CONTROL, opener neutralized (<!- -) |
prose=1 | prose=1 | prose=1 |
B — witness block in a details/summary fold (production shape) |
prose=2 | prose=1 | prose=1 |
| C — indented code block | prose=2 | prose=1 | prose=1 |
| D — bare-CR twin of A | prose=1 | prose=1 | prose=1 |
| E — DECLARED LIMIT, opener in ordinary prose | prose=2 | prose=2 | prose=2 (identical) |
F — ~~~ fence |
prose=2 | prose=1 | prose=1 |
| G — UNCLOSED fence | prose=0 code=2 | prose=0 code=2 | prose=0 code=2 (F2 class, identical) |
| H — blockquoted fence | prose=2 | prose=1 | prose=1 |
| I — opener lodged in a fence info string | prose=2 | prose=1 | prose=1 |
| J — mid-body forged footer + trailing prose | prose=2 | prose=2 | prose=2 (identical) |
Base's rendered HTML for shape A shows both footers posting; head's shows one:
<!\-\- BASE --> <!\-\- HEAD -->
<pre><code><!-- x </code></pre> <pre><code><!-- x </code></pre>
<p><em>— m via Qwen Code /review</em></p> <p><em>— qwen3-coder via Qwen Code /review (v1.2.3)</em></p>
<p><em>— qwen3-coder via Qwen Code /review (v1.2.3)</em></p>Both controls strip on base (AB5), which isolates the trigger: the unterminated opener is the whole cause, exactly as the description claims. Three invariants hold on all 12 shapes: r3 and head agree everywhere, so commit 3 changes nothing on the multi-line ON leg (AB8, corroborated by S3 — 7/7 multi-line bodies byte-identical); head's strip output is a byte prefix of the input, so nothing is invented (AB9); and on all 9 shapes where head strips, the rendered code text is identical before and after, so no witness block is erased (AB10).
A/B #2 — S1, N1 at the real one-line destinations (28 cells per arm, 6 assertions)
Witness: 01-n1-destination-head-closes-what-round3-left.png.
Round 3 posted the bare folded line. The real channels do not: boundDeferredLine runs first, and its output then passes through mdField (deferred, relocated — which runs stripCommentGrammar and wraps the result in a backtick code span) or asListLine (duplicates, cannot-tell — which runs stripCommentGrammar and posts bare prose). stripCommentGrammar replaces <!\-\-/--> with a space, so the opener the strip sees never reaches the render. All four destinations were driven, with renderDeferredEntry's real file:line — [source] prefix so a leading ~~~ cannot open a fence of its own.
| folded title shape | deferred (code span) base/r3/head | duplicates (bare prose) base/r3/head |
|---|---|---|
closed ``` fence quoting <!\-\- x |
code=1 / code=1 / code=0 | prose=1 / prose=0 / prose=0 |
~~~ fence |
code=1 / code=1 / code=0 | 0 / 0 / 0 (marker lands in an info string) |
| UNCLOSED fence | code=1 / code=1 / code=0 | 0 / 0 / 0 |
| INDENTED block | code=1 / code=1 / code=0 | prose=1 / prose=1 / prose=0 |
| fence info string | code=1 / 0 / 0 | prose=1 / 0 / 0 |
| CONTROL opener closed | code=1 / 0 / 0 | prose=1 / 0 / 0 |
| CONTROL no opener | code=1 / 0 / 0 | prose=1 / 0 / 0 |
Cells posting a visible forged prose attribution: base 10 → r3 2 → head 0 (N1a, N1b, N1c). The posted bytes for the ~~~ sibling, deferred channel:
base/r3: - `packages/…/review-footer.ts:441 — [review] ~~~ x ~~~ _— m via Qwen Code /review_`
head : - `packages/…/review-footer.ts:441 — [review] ~~~ x ~~~`
Nothing else is removed — head's line is r3's minus the trailing attribution, and both controls strip at head (N1d, 8 cells).
Corrections
Stated as corrections to text, not as requests to change code.
1. To round 3's own N1 render — the deferred channel posts a code span, not prose. Round 3 published <li>packages/…/review-footer.ts:441 — [review] ~~~ <!\-\- x ~~~ <em>— m via Qwen Code /review</em></li> as proof the surviving footer renders as prose. It does not: line 6175 posts `- ${mdField(entry)}` and mdField wraps the folded line in backticks after running stripCommentGrammar, so the real posted body is - `… ~~~ x ~~~ _— m via Qwen Code /review_` rendering as <li><code>…</code></li> — a quotation, which by this PR's own doctrine "can neither BE attribution nor hide any". Round 3's Not covered named this omission for the duplicates channel but applied it only there. The direction of N1 survives — the INDENTED shape on the bare-prose duplicates/cannot-tell channels did post a visible forged attribution at r3 (prose=1) — but its visible scope was 1 shape × 2 channels, not 3 shapes, and the fix is nonetheless load-bearing because the strip runs before the grammar is neutralized.
2. To the description's test count. "117 files, 5757 tests pass (18 skipped)" — measured 117 files, 5758 passed, 17 skipped (5775 total), exit 0 (D11). Off by one in both directions, the same class of mismatch round 3 recorded (5754/17); consistent with a slightly different tree state when the description was written, not a mechanism claim.
3. In the description's favour — four of its mutation claims are exact. "no blanking → 7 tests red across review-footer.test.ts and submit.test.ts" measured 7 red in exactly those two files (5 + 2); "the folded line's literal reading reverted → 2 tests red (the ~~~/indented fold shapes, and an unclosed-fence deferred title through composeReview)" measured 2 red with exactly those two names; "the < arm of the marker gate dropped → 1 test red" measured 1; "markdown-it's inline pass re-enabled → 1 test red (a 256 KiB one-line body must strip within 40 ms; measured 1.2 ms block-only, 395 ms with the inline pass)" measured 1 red at 437 ms, against this round's independent 13.658 ms block-only at 262,144 chars. All four fail the intended assertion with expected-vs-actual values, not an import, compile or fixture break (D3).
4. To the source comment's "quadratic" wording — resolved. Round 3's Correction 1 measured an exponent of 0.91 against a comment claiming "quadratic". The comment now says "two orders of magnitude on hostile one-line bodies … linear either way", which this round's measurement supports (exponent ≤ 0.41 across four hostile kinds, P3).
Mutation matrix — all four killed (12 assertions)
Witness: 02-mutation-matrix-all-four-killed.png. Each mutant is an interface-preserving single-point edit applied by exact-anchor replacement (aborts if the anchor is not unique), run against the test files that import the mutated file, then restored and sha256-verified. The unmutated baseline is green — 117/117 files, 5758 passed, 17 skipped, exit 0 — so every red below is attributable to its own edit (D0).
| guard mutated | suite that should catch it | red | green | intended assertion? | classification |
|---|---|---|---|---|---|
commit 3's 'literal' reading → 'swallow' (= round 3's head) |
review-footer.test.ts + compose-review.test.ts |
2 | 780 | yes — expected 'x ~~~ <!\-\- x ~~~ _— m via Qwen Code /…' to be 'x ~~~ <!\-\- x ~~~' and expected 2 to be 1 |
pinned — and the 2 red are exactly the tests commit 3 added (D1, D2, D3) |
canProjectFooterMarker's < arm dropped (round 3's survivor M2) |
review-footer.test.ts |
1 | 86 | yes | pinned — round 3 recorded this surviving; N3 closed (D4, D5) |
BLOCK_PARSER.core.ruler.disable(['inline']) removed (round 3's survivor M5) |
review-footer.test.ts |
1 | 86 | yes — the 40 ms timing bound, at 437 ms | pinned — round 3 recorded this surviving; N4 closed (D6, D7) |
blankQuotedCode disabled — positive control |
review-footer.test.ts + submit.test.ts |
7 | 204 | yes | pinned, 5 + 2 in exactly the two files the description names (D8, D9) |
The control is landed in the same files as the mutants, so "the suite does not cover this" and "my command collected nothing" are distinguishable; the two single-file mutants collect the same 87 tests, so M2 and M5 were judged by the same command on the same file (D10). No survivor to adjudicate this round: every guard the PR adds on the changed axis is pinned, which is the property round 3 could not establish.
Findings
F-new1 — Note (new, pre-existing, not declared): the aggressive reading erases prose GitHub displays
The description's declared-limits paragraph names four costs of keeping the end-of-input reading in multi-line bodies, and states the design principle beside them: "a miss costs a duplicate attribution line, which is why over-stripping visible content is the failure mode this PR avoids." One cost of that reading is not in the list: when a forged footer precedes a mid-line unterminated <!\-\-, the projection swallows to end-of-input, the $-anchored match lands on the footer, and the cut removes everything the swallow hid — including prose GitHub renders.
| body | GitHub displays | head stripReviewFooter |
base |
|---|---|---|---|
_— …/review_ <!\-\- x\n\nreal finding text |
— m via Qwen Code /review <!\-\- x real finding text |
"" |
"" |
lead\n\n_— …/review_ <!\-\- x\n\ntail |
lead — m via Qwen Code /review <!\-\- x tail |
lead |
lead |
real finding text and tail are displayed prose in the input and are gone from the output (C5). Bound — what this is not: it is identical on head and base (C4), so it is pre-existing and not a regression; it is not reachable from compose-review's verbatim exits, where quotedProse runs stripCommentGrammar before the chain; and the trigger requires a model to draft a forged footer and an unterminated opener on the same line, footer first. Cost is lost finding text rather than a wrong attribution. It is recorded because the description's own stated principle says this is the failure mode the PR avoids, and a reader weighing the declared-limits list would otherwise not know the list is incomplete.
Reproduce: cd tmp/pr10458-verify-20260905-205746 && node offleg-census.mjs (section the over-strip).
F-new2 — Note (new, about the rationale): the stated reason for keeping 'swallow' on the attribution-off line strips has no witness
Both the description and projectInvisibles's doc comment justify the split reading with "the attribution-off line strips rely on the aggressive reading to remove a forged footer trailed by junk". That is a falsifiable claim about two real destinations, so it was tested by census rather than by reading: arm-litlines flips the two remaining single-line projection sites (stripByProjection, stripForgedFooterLines) to 'literal', changing nothing else, and 432 distinct bodies — opener kind × line prefix × trailing junk × body extent × both footer/opener orders — were driven through both attribution-OFF destinations (submit.ts:316 and submit.ts:562/1365).
| cells (864 total) | count |
|---|---|
| readings agree, no visible difference | 840 |
'swallow' posts fewer visible attributions — the declared reliance |
0 |
'literal' posts fewer — the declared limit's cost |
24 (all on stripForUnattributedPost) |
The declared reliance has no witness in 432 bodies (C2). The mechanism is visible in the data: at submit.ts:316 stripReviewFooter runs first, and on any body carrying an unterminated opener it swallows to end-of-input, so stripForgedFooterLines downstream never sees a discriminating projection; at submit.ts:562 the chain's own stripFooterSpans covers the same shapes.
Bound — the reading is not valueless, and this is not a request to change it. 184/864 cells differ in text while posting the same attribution count: 'swallow' removes residue 'literal' leaves (real finding vs <!\-\- x\n\nreal finding), so the choice is defensible on cleanliness grounds (C6). The 24-cell gap is pre-existing — head's posted output is byte-identical to base's on all 24 (C7) — and is exactly what the description declares out of scope. The finding is that the stated reason is unsupported, which matters here because this PR's own history is 14 review rounds of machinery accreting around reasoning nobody re-measured; a comment that names a load-bearing purpose which is not load-bearing will be read as a reason not to touch the code.
The converse measurement, so the boundary is honest: the 'literal' reading's own premise was verified rather than assumed. A mid-line unterminated opener renders the footer as prose (<p>x <!-- y <em>— m via Qwen Code /review</em></p>, S0a), so commit 3's reasoning holds where it is applied. One case it over-projects: a line-leading opener is a CommonMark HTML block rendering as nothing (prose=0 code=0, S0b), not prose — the consequence is only that an already-invisible footer gets stripped, which costs nothing.
Reproduce: node offleg-census.mjs (census section) and node sibling-sweep.mjs (S0).
N5 — Note (stands, re-measured): STRIP_TAIL_LIMIT no longer bounds total work
Recorded so a reviewer sees the design change rather than discovering it. Base's cost was near-independent of body length by construction (it projected only the last 8,192 characters); head's blankQuotedCode → scanLines → BLOCK_PARSER.parse(body) runs over the whole body. Measured over a 131× size increase: base's cost rises 2.66–3.02×, head's 2.57–5.63×, and the worst head/base ratio grows 4.93× → 10.44× — the signature of O(n) against O(1) once both carry a large constant term (P4). Absolute ceiling: 10.034 ms at GitHub's 65,536-character comment cap (P2) and 13.658 ms at 262,144, inside the new timing test's own 200 ms budget (P1). Growth exponent ≤ 0.41 across four hostile kinds, 0 capped runs and 0 errors in 24 per-rung child processes (P0, P3). Not a defect; the change is necessary, since a fence's state is only knowable from where it opened.
Not covered
- Round 2's corpus differential (1395 bodies; F1's mid-body counts, F3's +126 tradeoff magnitude, N2's nine-body adjudication). Not re-measured — the generator lived in round 2's artifact dir and the workflow does not carry harnesses forward. The input closure is not identical this round (
review-footer.tschanged), so the shortcut was unavailable even in principle; those numbers stay attributed to round 2 and are deliberately not re-asserted. Re-measured on single shapes instead: F1's mid-body class (shape J, identical on all three arms) and F2's class (shape G,code=2 prose=0on all three arms). - Round 3's layered-guard combination row. Not re-run: the H2 mutant it reverted no longer exists in the code, so there is no hunk to revert.
- Round 3's remaining description-claim mutants. Three of its five mutation claims were re-verified here (
no blanking,<arm, inline pass) plus commit 3's; the raw-HTML block kinds counted as visible HTML (round 2: 13 red vs 3 claimed) and the threecompose-reviewingest-site strips were not re-run and carry round 2's numbers. - The real production comment
pull/10445#discussion_r3885165818. Not fetched: no GitHub token, no network calls. Shape B is a faithfuldetails/summaryreconstruction, so the shape is reproduced end-to-end at the destination, not the production bytes; the description's stated 3006 → 2967 figures were not checked. This reproduces the wire shape the incident reported, not the model-side drafting that produced it. - Live posting. Destinations are reconstructed from
submit.ts:293-320,compose-review.ts:6175,:928,:5861,:1936andlib/md-field.ts, lifted verbatim, not a realqwen reviewsubmit against GitHub.linkifyCommentRefsis omitted from the reconstructedasListLine;A0asserts no fixture carries acomment <digits>ref, so it is identity on every input driven. submit.ts:215's relocated-claim channel was not driven to its own posted destination (the claim passes anENTRY_FENCE_DELIMITER_REgate before posting); its strip is covered indirectly byD8, where thesubmit.test.tskilling test isthe relocated claim strips the one-line shape that posts.- Per-commit attribution. The checkout is shallow (depth 2). The snapshot lists 3 commits but only
45481e00is locally reachable —5b1e5102anda4ab212bboth failgit cat-file -t, and so does the snapshot'sbaseRefOid9b188ea3.git rev-list HEAD^1..HEAD^2returns1, the shallow-boundary trap, so it was not used as the reachability check; comparison against the snapshot'scommitsarray was. Round 3's head was reconstructed by reverting one anchor rather than by checkout, and that reconstruction is validated by its own census (r3 reproduces exactly the 2 survivors round 3 reported, on the 2 channels round 3's render could see). Commit 1's blanking and parser delegation therefore could not be attributed separately from commit 2's fold-site strips. - Base-arm vitest. The base worktree cannot run the suite (its
globalSetupguard stops on unbuilt workspace packages), so mutation and gates ran head-only. Sound here: a mutant is a head-source edit judged by head tests, and no base cell depends on a base test run. - Repo-wide gates. Only
cd packages/cli && npx vitest run src/commands/review/pluseslint/prettieron the 6 changed files. No repo-wide lint, nonpm run typecheck, no full test suite, no integration tests, noactionlint/shellcheck(no workflow files in the diff). - Windows/macOS line handling beyond the CRLF and bare-CR variants exercised as shapes A/D.
- The 432-body census is a family, not a proof over all inputs. It covers opener kind × prefix × junk × extent × both orders; a body outside that grid could discriminate the readings where these did not.
- Flakiness gate. Not run by me; the workflow runs its own.
Methodology
Environment: the CI node:22-bookworm container (node v22.23.2) at refs/pull/10458/merge, depth 2, with npm ci and npm run build already completed at head. Four behavioural arms were esbuild-bundled (--bundle --platform=node --format=esm --loader:.wasm=binary plus a createRequire banner) from packages/cli/src/commands/review/lib/review-footer.ts into self-contained arm-{head,r3,base,litlines}.mjs; a first cut used --packages=external and failed at runtime on core's transitive ajv path, so the arms are fully inlined. The base side came from git worktree add tmp/base-tree HEAD^1 with node_modules linked in and both realpaths asserted before any cell was trusted, and was removed with git worktree remove --force once the cells were captured. arm-r3 reconstructs round 3's unreachable head by reverting commit 3's single behavioural anchor ('literal' → 'swallow' in stripReviewFooterLine); arm-litlines flips the two other single-line projection sites. Both were built from scratch copies placed next to the original (__var-*.ts) because a copy under tmp/ cannot resolve the relative ./inline-counts.js; every variant is an interface-preserving single-point edit applied by exact-anchor replacement that aborts if the anchor is not unique, and each scratch file was deleted immediately after bundling with the original's sha256 verified restored (5d777212…, checked after every build). Harnesses (n1-destination.mjs, ab-central.mjs, sibling-sweep.mjs, offleg-census.mjs, perf.mjs, mutation-assertions.mjs + shared oracle.mjs) drive the compiled arms directly — real module code, no stubs — and the oracle renders through real markdown-it + parse5, counting markers separately in prose and in code. Private functions (collapseToLine, boundDeferredLine, renderDeferredEntry, asListLine, normalizeInlineComments) are lifted verbatim from the head tree and parameterised only by the arm, so base's fold site is base's real one (collapseToLine(rendered) with no strip). Every perf rung ran in its own child under timeout 30, and P0 asserts all 24 runs produced a finite measurement before any growth conclusion is drawn from them. In-place tree mutations (four mutants, both lint liveness probes) were each restored and sha256-verified, and git status --porcelain is empty. Three assertions were mis-specified in first cuts and are corrected in place with the reason recorded in the harness source: C2 asserted the declared reliance was real and measurement disproved it, so it was re-specified to the fact the census established; P4 used an arbitrary "base ratio < 3×" threshold that failed at 3.41× over a 131× size increase, so it was re-specified to the head/base ratio growth the data actually shows; and AB11/AB14 hunted the attribution-OFF tradeoff at a destination where stripReviewFooter preempts it, so that block was removed and replaced by the census — no assertion from any first cut is counted. One parse bug was fixed the same way: an unanchored /Tests\s+(.*)/ matched the first test name containing "Tests" and read 0 failures for all four mutants, which briefly made four real kills look like a dead harness; anchoring it to ^\s*Tests\s+(.*)$/m restored the true counts (2/1/1/7). Evidence images were produced with node scripts/verify-capture.mjs (no browser, no pseudo-terminal), each a live re-run of the harness it depicts. Raw per-cell logs, the assertion inventory, the census survivor lists, the perf ladder JSON and the suite/mutation logs are in tmp/pr10458-verify-20260905-205746/logs/; the ~21 MB arm bundles are regenerable in ~20 s via build-arms.sh, build-variants.sh and build-litlines.sh.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/commands/review/compose-review.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/compose-review.test.ts
file packages/cli/src/commands/review/lib/review-footer.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/review-footer.test.ts
file packages/cli/src/commands/review/submit.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/compose-review.test.ts: PPPPP
packages/cli/src/commands/review/lib/review-footer.test.ts: PPPPP
packages/cli/src/commands/review/submit.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/compose-review.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/review-footer.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
APPROVE (re-review at new head 45481e0; the approval at a4ab212b was procedurally dismissed by this push)
What moved since the last review
One additional commit since the previously-approved head, and it is a strictly-scoping hardening of the folded-line strip that closed the last blocker: projectInvisibles now takes a caller-picked reading for an UNCLOSED <!-- — 'swallow' (default, unchanged) for the multi-line strips, where an unterminated opener legitimately runs to end-of-input, and 'literal' for stripReviewFooterLine, because on a folded single line CommonMark's inline-HTML rule never fires (no later line for a -->), GitHub escapes the opener to literal text, and the forged footer after it renders as prose — exactly the shape the duplicates-fold fix posts. I verified the CommonMark claim, the default-preserving parameterization (existing callers byte-identical), and that a CLOSED comment still drops whole under both readings (pinned by its own test pair, including the invisible-footer-inside-comment case). The end-of-review-footer test also pins the boundDeferredLine chain's ordering through a real composeReview with an unclosed fence plus opener.
Together with the unchanged remainder, this retires the last bypass the round-13 blocker described — the author beat the reviewer to their own channel again, one fold site ahead of the report.
Historical items
The final blocker at 5b1e5102 (duplicates channel reaching the fold without the line strip) stays fixed at this head: boundDeferredLine still applies the line strip before the cap, and the new commit's tests keep the whole chain green. No other review on the head carries an open item.
My Critical scan and CI
The two coverage additions (the <-arm gate pinned by a comment-split marker phrase; a 256 KiB hostile-body latency bound proving the inline pass stays disabled) are test-only. One tail-window subtlety (an unclosed opener starting before the 8192-char tail bound) is pre-existing, unchanged by this commit, and unreachable through the one-line channels' upstream caps. CI at this new head: every lane cancelled or still queued/in-progress from the push churn, zero failures — cancellations never gate per policy; the first green run of the changed suites on 45481e00 is worth watching but the code review is complete on the merits above.
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
…ecycle Resolves the #10458 overlap: review-footer.ts keeps main's parser-backed scanLines; the stamp's single-line model (fenceOpener, HTML_BLOCK_OPEN_RE, HTML_BLOCK_TAG_NAMES) moves into thread-lifecycle.ts, its only consumer; the Aone relocation reads the claim line through carriedClaimLine and then main's stripReviewFooterLine; the one-line footer-span strip treats a code-indented line as quotation, matching the multi-line map.
|
Released in v0.23.1. |
4 participants
What this PR does
A posted review comment carries one attribution footer: the reviewing model writes one into its draft, and the CLI strips that copy before appending the canonical, version-stamped one. This PR fixes a case where the strip silently found nothing to remove, so both footers posted.
The strip matches what GitHub displays, not the raw bytes — an HTML comment or an entity reference hidden inside the marker phrase must not shield a forged footer. In that displayed projection, a comment opener with no closer runs to the end of the input, which is what a browser does with it. Inside a code fence it does not: GitHub renders fenced content literally, opener and all. The projection did not make that distinction, so an unterminated
<!--quoted anywhere in a witness block swallowed the trailing footer along with everything after it, the strip saw no footer to remove, and the canonical one landed beside the model's own.Three changes, all in
packages/cli/src/commands/review/:markdown-it's block token map (already apackages/clidependency, same construction asaudit-layers.ts). The hand-built scanner the line-aware strips shared disagreed with the renderers on lazy continuation (an indented line right after a paragraph line is prose), list content indents, tab stops, and fence delimiters inside raw-HTML blocks — and once the blanking trusted it, each disagreement kept a forged footer the render showed. The scanner goes away; every strip in the file reads the same map.compose-review's deferral titles, reroute records, ingested entries and duplicate-dropped entries, andsubmit's relocated claim collapse multi-line text to one posted line, which flattens a footer the blanking kept as quoted code. The trailing strip now runs on the folded line as well (stripReviewFooterLine, no blanking: a folded line has no block structure) — at the ingest sites, and as the fold's own guarantee insideboundDeferredLine, the per-entry bound every deferred, relocated, duplicate-dropped and cannot-tell exit passes through, ahead of its character cap. The duplicates channel reaches that fold with no ingest-time strip ahead of it, which is why the guarantee lives at the choke point rather than per exit. On a folded line an unterminated<!--is read as literal text — a single line has no later line for a closer, so CommonMark's inline HTML rule never fires and GitHub escapes it — where the multi-line strips keep reading it as a comment running to the end of the input; the fold puts a witness block's quoted opener on the footer's own line, and swallowing there hid the footer.Why it's needed
It happened on a real review, on a comment whose subject was an HTML dedup marker — its witness block quoted the marker cut short, which is exactly the shape that trips this. The comment posted with two footers, one of them the model's own unversioned copy: #10445 (comment)
The blind spot is not limited to that shape. Any review that quotes an unterminated comment opener in its evidence — reviews of HTML markers, of templating that emits them, of the truncated output of a tool that prints one — loses the strip for that comment, and the duplicate attribution is what the version stamp exists to prevent. The same strip also normalizes bodies for the dedup and re-check lookups, so a body it cannot normalize is a body those lookups can mismatch.
Reviewer Test Plan
How to verify
Reproduce on the real comment: fetch that comment body, cut the canonical footer the CLI appended, and run
stripReviewFooterover what remains (the model's draft, ending in its own footer). Onmainit comes back byte-identical — 3006 characters in, 3006 out, one forged footer still trailing. With this change it strips to 2967 characters, ends at</details>, and carries no footer. Neutralizing the quoted opener (<!- -) makesmainstrip too — the opener is the whole trigger.Minimal shape, if you would rather not fetch the comment: a body of
```\n<!-- x\n```\n\n_— m via Qwen Code /review_strips to the fenced block.Unit tests:
cd packages/cli && npx vitest run src/commands/review/— 117 files, 5757 tests pass (18 skipped). Every new guard was mutation-verified by reverting it and re-running:stripTrailingFooter(body, body)) — 7 tests red acrossreview-footer.test.tsandsubmit.test.ts;compose-reviewingest-site strips reverted — 3 tests red;boundDeferredLinefold strip reverted — 2 tests red (the duplicates channel, with and without the character cap in play);<!--reverted — 2 tests red (the~~~/indented fold shapes, and an unclosed-fence deferred title throughcomposeReview);<arm of the marker gate dropped — 1 test red (a marker phrase split by a closed comment);submitclaim-line strip reverted — 1 test red.eslint --max-warnings 0andprettier --checkpass on the six changed files.Evidence (Before & After)
N/A — not user-visible in the TUI. The observable difference is the posted comment body, covered by the witness replay above and the submit-level test asserting exactly one
via Qwen Code /reviewin the posted payload with the witness block intact.Tested on
Environment (optional)
Unit tests only.
Risk & Scope
stripForgedFooterLines,stripCommentMarkerLines,stripParagraphMarkers, the span strips,swallowsAppendedMarker) now classify code through the parser, so the shapes where the hand scanner disagreed with GitHub — lazy continuations, list content indents, tab-indented code, fence-looking lines inside a raw-HTML block — flip toward what the render shows. (3) The one-line channels strip after the fold. Each is pinned by tests.main, with one narrow exception named below: in a multi-line body the projection still takes an unterminated<!--in ordinary prose as running to the end of the input (GitHub escapes a mid-line one as literal text; the attribution-off line strips rely on the aggressive reading to remove a forged footer trailed by junk — only the folded single-line channels read it literally); a-->quoted in a later code block therefore no longer closes such an opener, wheremainhappened to strip by reading the quoted closer — that mid-line-opener shape is the one regression this PR accepts, at the cost of one cosmetic duplicate line (the line-leading twin is handled: the parser reads it as an HTML block and the quoted closer still closes); the marker phrase is matched on one literal spelling; a closed comment spanning a paragraph break is still dropped whole. The strip is best-effort sanitation of the reviewing model's own draft — a miss costs a duplicate attribution line, which is why over-stripping visible content is the failure mode this PR avoids.autofix/takeoverthis PR ran 14 review rounds, and the diff grew from 79 to ~1,300 source lines as each round hardened the machinery the previous round added (a render-faithful raw-HTML projection, opener neutralization, paragraph-bound closers, terminator fail-open guards, fence-residue gates) — with the next round's Criticals landing in that new code. I reset the branch to the shape above: the original fix, the parser delegation the loop introduced (which retired the five confirmed round-1 defects), and the fold-site strips (round-1 R1-9). Everything else is dropped rather than fixed; the loop's last head is preserved atarchive/pr10458-loop-round14(d69deb9) for reference.Linked Issues
None.
中文说明
这个 PR 做了什么
一条发布出去的评审评论只带一条署名 footer:模型在草稿里写一条,CLI 在追加带版本号的规范 footer 之前把模型那条剥掉。本 PR 修的是剥离步骤悄悄什么都没剥掉、于是两条 footer 同时发出去的情况。
剥离匹配的是 GitHub 显示出来的内容,而不是原始字节——藏在标记短语里的 HTML 注释或实体引用不能给伪造 footer 当挡箭牌。在这个显示投影里,没有闭合的注释起始符会一直吞到输入末尾,这也是浏览器的行为。但在代码围栏里不是这样:GitHub 把围栏内容原样渲染,包括那个起始符。投影没有区分这一点,于是 witness 代码块里任何一个未闭合的
<!--都会把它后面的一切连同末尾的 footer 一起吞掉,剥离步骤看不到 footer 可剥,规范 footer 就落在了模型自己那条旁边。三处改动,全部在
packages/cli/src/commands/review/下:markdown-it的块级 token map 决定(它已是packages/cli的依赖,与audit-layers.ts同一构造)。行级 strip 共用的手写扫描器在懒续行(段落行紧跟的缩进行是正文)、列表内容缩进、tab 制表位、原始 HTML 块内的围栏分隔行这几处都与渲染器不一致——一旦置空信任了它,每一处不一致都会留下一条渲染可见的伪造 footer。手写扫描器整体删除;文件里所有 strip 读同一份 map。compose-review的 deferral 标题、reroute 记录、摄入的条目、重复项条目,以及submit的 relocated claim,都会把多行文本折叠成一行发出去,这会把置空保留下来的"代码内 footer"压平。尾部剥离现在也在折叠后的行上跑一次(stripReviewFooterLine,不置空:折叠后的行没有块结构)——摄入站点各跑一次,并作为折叠自身的保证放在boundDeferredLine里,即 deferred / relocated / 重复项 / cannot-tell 每个出口都要经过的逐条上界,且在字符截断之前。重复项通道到达这个折叠时前面没有任何摄入时剥离,所以这条保证放在汇点而不是逐出口补。折叠后的单行里,未闭合的<!--按字面文本处理——单行没有后续行可放闭合符,CommonMark 的行内 HTML 规则不会触发,GitHub 会把它转义——而多行剥离仍把它视作吞到输入末尾的注释;折叠会把 witness 块里引用的起始符放到 footer 同一行,在那里吞掉就会藏住 footer。为什么需要
这在一次真实评审里发生了,而且发生在一条主题恰好是 HTML 去重标记的评论上——它的 witness 代码块引用了被截断的标记,正是触发这个问题的形状。那条评论带着两条 footer 发了出去,其中一条是模型自己写的不带版本号的副本:#10445 (comment)
盲区不止这一种形状。任何在证据里引用了未闭合注释起始符的评审——评审 HTML 标记、评审会生成这类标记的模板、评审某个打印它的工具被截断的输出——都会对那条评论失去剥离能力,而重复署名正是版本戳要防的东西。同一个剥离函数还负责把正文归一化后用于去重和复检查找,所以一个归一化不了的正文,也是这些查找可能错配的正文。
评审者测试计划
如何验证
在真实评论上复现:取回那条评论正文,截掉 CLI 追加的规范 footer,对剩下的部分(模型草稿,以它自己的 footer 结尾)跑
stripReviewFooter。在main上它逐字节原样返回——3006 个字符进、3006 个字符出,伪造 footer 仍在末尾。改动之后剥到 2967 个字符,结尾停在</details>,不再带任何 footer。把被引用的起始符改掉(<!- -),main也能剥——起始符就是全部诱因。不想去取评论的话,最小形状是:正文
```\n<!-- x\n```\n\n_— m via Qwen Code /review_会被剥到只剩围栏块。单测:
cd packages/cli && npx vitest run src/commands/review/—— 117 个文件、5757 个用例通过(18 个跳过)。每个新增守卫都做了突变验证(去掉它再跑一遍):stripTrailingFooter(body, body))——review-footer.test.ts与submit.test.ts共 7 个用例变红;compose-review三处摄入站点剥离 —— 3 个用例变红;boundDeferredLine的折叠剥离 —— 2 个用例变红(重复项通道,含/不含字符截断两种);<!--的字面读法 —— 2 个用例变红(~~~/缩进折叠形状,以及经composeReview的未闭合围栏 deferred 标题);<分支 —— 1 个用例变红(被闭合注释切开的标记短语);submit的 claim 行剥离 —— 1 个用例变红。六个改动文件通过
eslint --max-warnings 0与prettier --check。证据(前后对比)
N/A —— TUI 里看不到。可观察的差别是发布出去的评论正文,由上面的真实评论回放和 submit 层测试(断言发布载荷中恰有一条
via Qwen Code /review且 witness 块完整)覆盖。测试平台
环境(可选)
仅单元测试。
风险与范围
stripForgedFooterLines、stripCommentMarkerLines、stripParagraphMarkers、span strip、swallowsAppendedMarker)现在通过解析器判定代码,所以手写扫描器与 GitHub 不一致的那些形状——懒续行、列表内容缩进、tab 缩进代码、原始 HTML 块内形似围栏的行——都朝渲染结果的方向翻转。(3) 单行通道在折叠后再剥一次。每一条都有测试钉住。main上既有行为,仅下述一处窄例外:多行正文里,普通正文中未闭合的<!--投影仍视作吞到输入末尾(行中的 GitHub 会转义成字面文本;attribution-off 的行级剥离依赖这种激进读法去掉后面跟着杂字的伪造 footer——只有折叠后的单行通道按字面读);因此后面代码块里引用的-->不再能闭合这样的起始符,而main在这个形状上恰好靠读到被引用的闭合符剥掉了——这个"行中起始符"形状是本 PR 接受的唯一一处回退,代价是一条外观上的重复行(行首的孪生形状已处理:解析器把它读成 HTML 块,被引用的闭合符仍能闭合);标记短语只按一种字面拼写匹配;跨段落边界的闭合注释仍整体丢弃。这个 strip 是对评审模型自身草稿的尽力清理——漏掉一次的代价是一条重复署名行,这正是本 PR 要避免"多剥掉可见内容"这种更糟失败模式的原因。autofix/takeover下本 PR 跑了 14 轮评审,diff 从 79 行涨到约 1,300 行源码——每一轮都在加固上一轮新加的机器(渲染保真的原始 HTML 投影、起始符中和、段落边界闭合搜索、终止符 fail-open 守卫、围栏残留门),而下一轮的 Critical 又落在这些新代码里。我把分支重置成上面的形状:原始修复、loop 引入的解析器委托(它消掉了第 1 轮确认的五个缺陷)、以及折叠站点剥离(第 1 轮 R1-9)。其余全部舍弃而不是修补;loop 的最后 head 保留在archive/pr10458-loop-round14(d69deb9)供查阅。关联 Issue
无。