feat(review): close unbounded finding classes instead of enumerating them - #9095
Conversation
…umerating them Reviews of PRs that hand-roll an unbounded or adversarial surface (parsing untrusted input, matching a rendered format, re-implementing a grammar) spun for many rounds: the reviewer filed each newly-found sibling entrance of the same divergence class as a fresh Critical, so the fix chased corners one at a time and never converged — the diff (and its test file) bloated round after round. The class-closing insight only surfaced retrospectively. Teach /review to reach it prospectively, in three places: - Agent 3b (Altitude & abstraction fit) gains an enumeration-trap check: a change that hand-rolls an unbounded/adversarial surface is a wrong-shape altitude finding — defer to a real parser / the tool's authoritative output / a fail-closed decision — filed ONCE, not enumerated case by case. - The incremental re-review's divergence-class handling distinguishes a bounded family (enumerate the countable entrances) from an unbounded one (escalate to one class-level Critical demanding the structural fix; stop filing siblings, and rule it fixed only when the structural change lands). - Pattern aggregation also groups by root cause, not only same-symptom-across- locations: many symptoms of one structural defect merge into a single finding whose suggested fix is the class-closing change. Prompt text only; no control-flow change. Severity-as-direction and the Criticals-only verdict already existed and are unchanged.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks @wenshao — the change itself reads well, but the PR body doesn't follow the PR template. None of the required sections are present — ## What this PR does, ## Why it's needed, ## Reviewer Test Plan (with How to verify / Evidence / Tested on), ## Risk & Scope, ## Linked Issues — the body uses a custom Problem / Change / Not changed / Testing structure instead.
The content is already all there, so this is a re-format rather than new work: map the existing Problem / Change / Testing text onto the template headings (a prompt-only change gets N/A under Evidence, and Linked Issues can simply say none). The recent PRs in this same series — #8960, #9020, #8956, #8751 — all follow the template.
Once the body is updated, re-trigger with @qwen-code /triage and the run will pick back up at the gate.
中文说明
感谢 @wenshao —— 改动本身没有问题，但 PR 正文未遵循 PR 模板：必需章节均缺失——## What this PR does、## Why it's needed、## Reviewer Test Plan（含 How to verify / Evidence / Tested on）、## Risk & Scope、## Linked Issues——正文使用了自定义的 Problem / Change / Not changed / Testing 结构。
内容本身已经齐全，所以这只是重新排版，而非新增工作：把现有 Problem / Change / Testing 的文本对应到模板标题即可（纯 prompt 改动的 Evidence 填 N/A，Linked Issues 写"无"即可）。同系列近期 PR——#8960、#9020、#8956、#8751——均遵循模板。
更新正文后使用 @qwen-code /triage 重新触发，审查会从门槛处继续。
— Qwen Code · qwen3.8-max
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. |
|
Body updated to follow the PR template (mapped the existing Problem/Change/Testing content onto What this PR does / Why it's needed / Reviewer Test Plan / Risk & Scope / Linked Issues; prompt-only change → N/A under Evidence, no linked issue). @qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…": did not run packages/cli / packages/core vitest suites — this worktree has no node_modules and I ruled out test impact by reading the test code and the budg…; "PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…": did not execute the two relevant test files ( agent-prompt.test.ts , SKILL.test.ts ) — the worktree has no node_modules, installing dependencies exceeds the bu…; "PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…": full vitest run of agent-prompt.test.ts (cli) and SKILL.test.ts (core) not executed — the worktree has no node_modules ( vitest fails with ERR_MODULE_….
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…"：did not run packages/cli / packages/core vitest suites — this worktree has no node_modules and I ruled out test impact by reading the test code and the budg…；"PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…"：did not execute the two relevant test files ( agent-prompt.test.ts , SKILL.test.ts ) — the worktree has no node_modules, installing dependencies exceeds the bu…；"PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…"：full vitest run of agent-prompt.test.ts (cli) and SKILL.test.ts (core) not executed — the worktree has no node_modules ( vitest fails with ERR_MODULE_…。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **Too shallow in the other direction — the wrong owner.** The defect is upstream (another module, another service, the data's producer) and the diff compensates for it downstream. Say whose bug it is. | ||
| - **Too deep — over-engineering.** A new abstraction, indirection layer, options object, or configuration point serving exactly one call site; a generalisation for a second case that does not exist. The cost is real and concrete: every future reader pays for the indirection, and the shape is fixed by a single example that may be unrepresentative. | ||
| - **Blast radius.** When a change to shared infrastructure exists to serve one caller, name the *other* callers it now also affects, and what it means for them. | ||
| - **Wrong shape — the enumeration trap.** A change that HAND-ROLLS parsing or matching of an **unbounded or adversarial surface** — untrusted input, a rendered format (what a viewer/renderer displays), or a spec/grammar — with per-corner special-cases that will accumulate: \`indexOf\`/\`slice\`/regex walking structured input, "match what the renderer renders" logic, a re-implemented grammar, a growing hand-listed set of cases. Such a surface has **no last corner**, so patching cases one at a time never converges — it is a change written at the wrong altitude: it should defer to an ABSTRACTION (a real parser, the tool's own authoritative structured output, or a fail-closed decision), not re-implement the surface inline. The finding is **not the current corner but the shape**: name the class-closing fix and file it ONCE, as this change's altitude finding, in place of enumerating its cases. Severity follows the risk the shape carries — a hand-rolled parser that can be fooled into a wrong pass/credit/accept is **Critical**, not a Suggestion. |
There was a problem hiding this comment.
[Suggestion] The prospective enumeration-trap check only reaches the whole-diff Agent 3b brief (Step 3A). Step 3B territory agents get the review dimensions as a one-word summary ("…security, code quality, performance…") from buildChunkAgentPrompt and never receive the 3b brief — repo-wide, "enumeration trap" appears in exactly three places, none of them the chunk brief — so the check does not exist on the chunked plan, while SKILL.md line 749 states unqualifiedly that it files this finding "prospectively — in round 1". The same function already carries the precedent for exactly this case: MODELED_SYSTEM_EXECUTION_LENS is attached to the chunk brief with the comment "Agent 2's brief never reaches a chunk agent. Attach the same lens here". — Failure scenario: a large chunked PR introduces a hand-rolled parser of untrusted input inside one chunk; the territory agent owning altitude sees only "code quality", no round-1 shape finding is filed, and the trap is caught only by the cross-round backstop after siblings have "recurred across two or more prior rounds" — two rounds of the exact spin the prospective check exists to prevent.
Suggested fix: attach a territory-scoped enumeration-trap line to the chunk brief in buildChunkAgentPrompt (the same way MODELED_SYSTEM_EXECUTION_LENS is attached), and/or qualify the SKILL.md parenthetical to the plan where the check actually runs.
中文说明
[Suggestion] 前瞻性的 enumeration-trap 检查只送达 whole-diff 的 Agent 3b brief(Step 3A)。Step 3B 的 territory agent 只从 buildChunkAgentPrompt 拿到一个词的维度概要("…security, code quality, performance…"),永远收不到 3b brief——全仓库中 "enumeration trap" 只出现三处,没有一处在 chunk brief 里——所以在 chunked plan 上该检查不存在,而 SKILL.md 第 749 行却无条件地声称它会"在 round 1 提前"提出该 finding。同一个函数里已有完全同类的先例:MODELED_SYSTEM_EXECUTION_LENS 就是以 "Agent 2's brief never reaches a chunk agent. Attach the same lens here" 的注释挂到 chunk brief 上的。— 失败场景:一个大型 chunked PR 在某个 chunk 里手搓了一个不可信输入解析器;负责 altitude 的 territory agent 只看到 "code quality",round 1 不会有任何 shape finding,只有当 sibling "连续 ≥2 轮复发"后才会被跨轮兜底捕获——先空转两轮,正是前瞻性检查本要避免的。
建议修复:在 buildChunkAgentPrompt 中把 territory 范围的 enumeration-trap 行挂到 chunk brief 上(与 MODELED_SYSTEM_EXECUTION_LENS 的做法相同),和/或把 SKILL.md 中的括注限定到该检查实际运行的 plan。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
|
|
||
| **Bounded family → enumerate; unbounded family → escalate (do not spin).** The "a still-open sibling is a fresh finding" rule above is for a **bounded** family — a countable set of entrances (a fence and its code-span twin), where naming the next one converges. When the family is **unbounded** — fresh sibling entrances have recurred across **two or more prior rounds**, or the surface is inherently infinite (hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar) — stop filing sibling N as a fresh Critical: patching entrances one at a time will not converge. Instead carry, or open, **one class-level Critical** — `the <X> family is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance` — and rule THAT Critical `fixed` only when the structural change lands, never when the latest entrance is patched. Once it is open for a family, do not enumerate further siblings of that family as separate findings; they are evidence under the one Critical. (Agent 3b's enumeration-trap check files this same finding _prospectively_ — in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family that was already being enumerated.) |
There was a problem hiding this comment.
[Suggestion] This new unbounded-family escalation rule is a load-bearing ledger-ruling protocol hunk, but unlike the three comparable hunks before it, it lands with no pin test in SKILL.test.ts. That file pins exactly this species of hunk — the round-cap protocol, the CONVERGED relay-entry removal, and the match-remote routing each carry a toContain test whose comment says a revert "must fail a test, not slip through" (added by PRs 8658, 8850 and 8903, verified in git history). — Failure scenario: a later edit paraphrasing or reverting the incremental re-review section (a section already re-edited several times) silently drops or inverts "rule THAT Critical fixed only when the structural change lands, never when the latest entrance is patched" with nothing red, and the pipeline regresses to the exact failure this PR exists to close.
Suggested fix: add a pin in packages/core/src/skills/bundled/review/SKILL.test.ts following the existing pattern, e.g. expect(skillBody()).toContain('rule THAT Critical fixed only when the structural change lands'), with the same "a revert must fail here" rationale comment the sibling tests carry.
中文说明
[Suggestion] 这条新的 unbounded-family 升级规则是一个承重的 ledger-ruling 协议块,但与它之前的三个同类块不同,落地时没有附带 SKILL.test.ts 里的 pin 测试。该文件恰恰为这一类块打过桩——round-cap 协议、CONVERGED relay-entry 移除、match-remote 路由各有一条 toContain 测试,注释写明 revert "must fail a test, not slip through"(由 PR 8658、8850、8903 添加,已在 git 历史中核实)。— 失败场景:以后某次编辑改写或回退 incremental re-review 一节(该节已被多次重编),悄悄丢掉或反转 "rule THAT Critical fixed only when the structural change lands, never when the latest entrance is patched",而没有任何测试变红,流水线退回本 PR 要关闭的那个失败。
建议修复:按现有模式在 packages/core/src/skills/bundled/review/SKILL.test.ts 里加一个 pin,例如 expect(skillBody()).toContain('rule THAT Critical fixed only when the structural change lands'),并附上 sibling 测试同款的 "a revert must fail here" 理由注释。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…pin the escalation Two review suggestions on the enumeration-trap change: - The prospective check only reached the whole-diff Agent 3b brief (Step 3A); Step 3B replaces the dimension agents with per-territory chunk agents that get the dimensions as a one-word summary, so a large chunked PR — exactly where the round-by-round bloat happens — never received it, contradicting SKILL.md's unqualified "files it prospectively in round 1". Attach a territory-scoped enumeration-trap instruction to the chunk brief in buildChunkAgentPrompt, the same way MODELED_SYSTEM_EXECUTION_LENS already is, so the round-1 shape finding exists on both topologies. - The unbounded-family escalation is a load-bearing ledger-ruling hunk but, unlike the sibling round-cap / CONVERGED / match-remote hunks, shipped with no SKILL.test.ts pin. Add one so a paraphrase or revert of "fixed only when the structural change lands" fails a test instead of slipping through.
|
Both addressed in 3B gap (agent-briefs.ts:376) — right, and it's the load-bearing case: the bloat happens on large chunked PRs, which is exactly the 3B path the whole-diff 3b brief never reaches. Wired a territory-scoped enumeration-trap instruction into No pin test (SKILL.md:749) — added, following the round-cap / CONVERGED / match-remote pattern: 241 tests pass (SKILL.test 9/9, agent-prompt 232/232); tsc/eslint/prettier clean. 中文说明两条都已在 3B 缺口:对,而且是关键场景——膨胀发生在大的分块 PR 上,正是 whole-diff 的 3b brief 到不了的 3B 路径。已把一条按领地范围的 enumeration-trap 指令接进 缺 pin 测试:已加,遵循 round-cap / CONVERGED / match-remote 的模式, 241 测试通过;tsc/eslint/prettier 干净。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
|
|
||
| **Bounded family → enumerate; unbounded family → escalate (do not spin).** The "a still-open sibling is a fresh finding" rule above is for a **bounded** family — a countable set of entrances (a fence and its code-span twin), where naming the next one converges. When the family is **unbounded** — fresh sibling entrances have recurred across **two or more prior rounds**, or the surface is inherently infinite (hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar) — stop filing sibling N as a fresh Critical: patching entrances one at a time will not converge. Instead carry, or open, **one class-level Critical** — `the <X> family is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance` — and rule THAT Critical `fixed` only when the structural change lands, never when the latest entrance is patched. Once it is open for a family, do not enumerate further siblings of that family as separate findings; they are evidence under the one Critical. (Agent 3b's enumeration-trap check files this same finding _prospectively_ — in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family that was already being enumerated.) |
There was a problem hiding this comment.
[Critical] R1-1: The cross-round escalation contract is internally inconsistent. Recurrence across two rounds is enough to label a potentially finite family unbounded; the separate open-blocker re-check still requires sibling enumeration; existing sibling IDs are never superseded; and this path always opens a Critical even though Agent 3b says severity follows demonstrated risk. — Failure scenario: a finite or Suggestion-level family recurs across two rounds or is recovered from a non-ledger blocker, so the workflow can keep old and new entrance findings open while adding a permanent class-level Critical, or block harmless fidelity gaps solely because the surface is infinite.
Witness: not run — this is a conflict among normative review-workflow instructions; no deterministic runtime unit can choose how a reviewer resolves the contradictory obligations.
Suggested fix: define one shared bounded/unbounded rule for both ledger and open-blocker paths, determine boundedness from the surface rather than round count alone, preserve severity from demonstrated risk, and explicitly supersede sibling IDs under one stable class-level finding.
中文说明
[Critical] R1-1：跨轮升级协议内部矛盾。仅连续两轮出现 sibling 就会把可能有限的家族判为无界；另一条已有 blocker 复查路径仍要求枚举 sibling；旧 sibling ID 没有被替代；而且这里无条件打开 Critical，尽管 Agent 3b 明确要求严重度随实际风险。— 失败场景：有限或仅 Suggestion 级的家族连续两轮出现，或 blocker 来自非 ledger 渠道，流程会在保留旧/新入口 finding 的同时再增加永久 class-level Critical，或仅因表面无限而阻断无害的保真问题。
证据：未运行——这是规范性 review 指令之间的冲突，没有确定性运行时单测能决定 reviewer 如何化解这些相互矛盾的义务。
建议修复：让 ledger 与 open-blocker 路径共用同一套 bounded/unbounded 规则；按表面本身而不是轮数判断 boundedness；按已证明风险保留严重度；并用一个稳定 class-level finding 显式替代 sibling ID。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| After verification, identify **confirmed** findings that describe the **same type of problem** across different locations (e.g., "missing error handling" appearing in 8 places). Only group findings with the **same confidence level** together — do not mix high-confidence and low-confidence findings in the same pattern group. For each pattern group: | ||
| After verification, identify **confirmed** findings that describe the **same type of problem** across different locations (e.g., "missing error handling" appearing in 8 places). Only group findings with the **same confidence level** together — do not mix high-confidence and low-confidence findings in the same pattern group. | ||
|
|
||
| **Also group by ROOT CAUSE, not only by symptom type.** When several confirmed findings are different symptoms of ONE root cause that needs a single structural fix — six XML-corner bypasses whose root is hand-rolled parsing, many call sites broken by one wrong contract — merge them into a single finding whose **Suggested fix is the class-closing change** and whose individual symptoms are the evidence, not N findings to patch one at a time. This is the within-round twin of the unbounded-family escalation in Step 6. |
There was a problem hiding this comment.
[Critical] R1-3: Root-cause aggregation does not survive the rest of the pipeline. Mixed-severity findings are promoted to the group’s highest severity, findings discovered during reverse audit skip the aggregation pass, and expanding one aggregate into per-location comments causes the next-round ledger to assign separate IDs. — Failure scenario: several parser symptoms share one root cause, but the review can post a Suggestion-only location as Critical, publish reverse-audit siblings separately, and then recover multiple independent findings instead of one structural blocker in the next round.
Witness:
Expected: one finding, R1-1
Received:
R1-1 src/a.ts Hand-rolled parser bypass family
R1-2 src/b.ts Hand-rolled parser bypass family
R1-3 src/c.ts Hand-rolled parser bypass family
Suggested fix: aggregate the final union after reverse audit, require same-severity members or preserve per-location severity, and build the machine ledger from canonical finding IDs so one aggregate can reference multiple posted locations without splitting identity.
中文说明
[Critical] R1-3：按根因聚合无法贯穿后续流水线。不同严重度的 finding 会被提升为组内最高严重度；reverse audit 新发现的 finding 不再经过聚合；一个 aggregate 展开为多个行内评论后，下一轮 ledger 会给每个位置分配独立 ID。— 失败场景：多个解析器症状共享一个根因，但 review 可能把仅 Suggestion 的位置作为 Critical 发布、把 reverse-audit sibling 分别发布，并在下一轮恢复为多个独立 finding，而不是一个结构性 blocker。
证据：探针期望一个 finding，实际得到 R1-1、R1-2、R1-3 三个独立 ledger 条目。
建议修复：在 reverse audit 后对最终并集再次聚合；要求成员严重度一致或保留每个位置的严重度；并从 canonical finding ID 构建 machine ledger，使一个 aggregate 能引用多个评论位置而不拆分身份。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
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 explored to full depth (tool budget reached): "PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…": running npx vitest run src/skills/bundled/review/SKILL.test.ts — the review worktree has no node_modules , so vitest cannot start; the new test's two substri…; "PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…": none — the dimension was fully covered within the assigned reads.; "PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…": could not run packages/cli test suites ( agent-prompt.test.ts ) — vitest config loading fails in this worktree because root node_modules/vitest is a broken/…; "PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…": none — all checks in my dimension completed within budget..
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
[Critical] R1-1 (existing Critical at SKILL.md:749, posted on the prior commit 8c586a7 by a concurrent review run) — re-checked against the reviewed commit 63aeb72: STILL STANDS. The cross-round escalation contract is internally inconsistent: recurrence across two rounds suffices to label a potentially finite family unbounded; the open-blocker re-check below the paragraph still mandates sibling enumeration unconditionally (line 758: 'A sibling entrance you found still open is a new finding (report it)'); sibling ids already in the ledger are never superseded when the class-level Critical opens; and the path opens a class-level Critical unconditionally although Agent 3b says severity follows the risk the shape carries. The paragraph is byte-identical to the version the Critical was filed on; this round's verified findings R2-3, R2-4, R2-7 and R2-8 confirm the same mechanisms. Not re-posted inline because the original thread is already on the PR at this line.
[Critical] R1-3 (existing Critical at SKILL.md:601, posted on the prior commit 8c586a7 by a concurrent review run) — re-checked against the reviewed commit 63aeb72: STILL STANDS. Root-cause aggregation does not survive the rest of the pipeline: the aggregate inherits the group's highest severity (line 614) while the new paragraph adds no same-severity constraint, so a Suggestion-only location can post under a Critical aggregate's per-location comments; confirmed reverse-audit findings proceed to Step 6 as a separate source from the aggregated output (line 695), so siblings found late skip the merge; and the machine ledger marker is built from the drafted per-location inline comments (compose-review.ts), so one aggregate expanded to N comments yields N separate ledger ids next round, splitting the class identity the merge exists to create. This round's verified findings R2-5 and R2-9 confirm the adjacent confidence and evidence facets of the same paragraph. Not re-posted inline because the original thread is already on the PR at this line.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）："PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…"：running npx vitest run src/skills/bundled/review/SKILL.test.ts — the review worktree has no node_modules , so vitest cannot start; the new test's two substri…；"PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…"：none — the dimension was fully covered within the assigned reads.；"PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…"：could not run packages/cli test suites ( agent-prompt.test.ts ) — vitest config loading fails in this worktree because root node_modules/vitest is a broken/…；"PR #9095 (QwenLM/qwen-code) is a prompt-text-only change…"：none — all checks in my dimension completed within budget.。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
[Critical] R1-1 (existing Critical at SKILL.md:749, posted on the prior commit 8c586a7 by a concurrent review run) — re-checked against the reviewed commit 63aeb72: STILL STANDS. The cross-round escalation contract is internally inconsistent: recurrence across two rounds suffices to label a potentially finite family unbounded; the open-blocker re-check below the paragraph still mandates sibling enumeration unconditionally (line 758: 'A sibling entrance you found still open is a new finding (report it)'); sibling ids already in the ledger are never superseded when the class-level Critical opens; and the path opens a class-level Critical unconditionally although Agent 3b says severity follows the risk the shape carries. The paragraph is byte-identical to the version the Critical was filed on; this round's verified findings R2-3, R2-4, R2-7 and R2-8 confirm the same mechanisms. Not re-posted inline because the original thread is already on the PR at this line.
[Critical] R1-3 (existing Critical at SKILL.md:601, posted on the prior commit 8c586a7 by a concurrent review run) — re-checked against the reviewed commit 63aeb72: STILL STANDS. Root-cause aggregation does not survive the rest of the pipeline: the aggregate inherits the group's highest severity (line 614) while the new paragraph adds no same-severity constraint, so a Suggestion-only location can post under a Critical aggregate's per-location comments; confirmed reverse-audit findings proceed to Step 6 as a separate source from the aggregated output (line 695), so siblings found late skip the merge; and the machine ledger marker is built from the drafted per-location inline comments (compose-review.ts), so one aggregate expanded to N comments yields N separate ledger ids next round, splitting the class identity the merge exists to create. This round's verified findings R2-5 and R2-9 confirm the adjacent confidence and evidence facets of the same paragraph. Not re-posted inline because the original thread is already on the PR at this line.
— qwen3.8-max via Qwen Code /review (v0.21.11)
| '**Shape check (part of code quality — the altitude lens, scoped to your ' + | ||
| 'territory).** If the code in your chunk HAND-ROLLS parsing or matching of an ' + |
There was a problem hiding this comment.
[Suggestion] The enumeration-trap lens this PR adds is written twice as two independent paraphrases — this chunk-prompt paragraph and the "Wrong shape — the enumeration trap" bullet in Agent 3b's brief (agent-briefs.ts:376) — instead of once as a shared constant. This file pair already solves exactly this dual-delivery problem: MODELED_SYSTEM_EXECUTION_LENS is defined once in agent-briefs.ts:203, interpolated into Agent 2's brief (agent-briefs.ts:327), then imported and injected into buildChunkAgentPrompt (agent-prompt.ts:576-584), with a pin test commented "The same lens text Agent 2 carries — one source, both topologies." The two new copies share ~90% of their content (surface taxonomy, corner mechanisms, "no last corner", the real-parser / authoritative-output / fail-closed fix trio, "file it ONCE", Critical-when-foolable); the delta is scope framing, which the precedent handles in the wrapping injection text rather than by paraphrasing the lens. The copies already diverge today on the Critical trigger — "a wrong result" here vs "a wrong pass/credit/accept" in the 3b copy. — Failure scenario: a future edit extending one copy (a new surface class, a reworded severity rule) silently leaves the other delivery path stale, so the same defect shape gets inconsistent findings/severity depending only on whether the diff is large enough to trigger chunking — with no shared symbol to grep and no test tying the copies together, the drift is undetectable until contradictory reviews appear.
Suggested fix: extract the lens into one exported constant in agent-briefs.ts (mirroring MODELED_SYSTEM_EXECUTION_LENS), interpolate it into the 3b brief where the bullet now sits, and inject the same constant here under the territory-scoped heading:
// agent-briefs.ts
export const ENUMERATION_TRAP_LENS = `- **Wrong shape — the enumeration trap.** …`;
// 3b brief template literal: ${ENUMERATION_TRAP_LENS}
// buildChunkAgentPrompt (this hunk):
'**Shape check (part of code quality — the altitude lens, scoped to your territory).**',
ENUMERATION_TRAP_LENS,中文说明
[Suggestion] 本 PR 新增的 enumeration-trap 检查被写成两份相互独立的改写文本——此处的 chunk-prompt 段落与 Agent 3b brief 中的 "Wrong shape — the enumeration trap" 条目（agent-briefs.ts:376）——而不是一个共享常量。这个文件对已经有解决同一"双投递"问题的现成模式：MODELED_SYSTEM_EXECUTION_LENS 在 agent-briefs.ts:203 定义一次，插入 Agent 2 的 brief（agent-briefs.ts:327），再被导入并注入 buildChunkAgentPrompt（agent-prompt.ts:576-584），并配有注释为 "The same lens text Agent 2 carries — one source, both topologies" 的 pin 测试。两份新文本约 90% 内容相同（面的分类、角落机制、"no last corner"、真解析器/权威输出/fail-closed 修法三件套、"file it ONCE"、可被欺骗即 Critical）；差异只是范围表述，而先例做法是把这部分放在注入处的包装文本里，而不是改写 lens 本身。两份文本今天在 Critical 触发条件上已经分叉——此处是 "a wrong result"，3b 那份是 "a wrong pass/credit/accept"。— 失败场景：以后某次编辑扩展其中一份（新增面的类别、改写严重度规则），会悄悄让另一条投递路径过时，于是同一种缺陷形状仅仅因为 diff 是否大到触发分块，就得到不一致的 finding/严重度——没有共享符号可 grep、也没有测试把两份绑在一起，漂移要到产生相互矛盾的评审才会被发现。
建议修复：把 lens 抽成 agent-briefs.ts 中的一个导出常量（仿照 MODELED_SYSTEM_EXECUTION_LENS），在 3b brief 现条目处插入它，并在此处以领地范围的标题注入同一常量（见英文侧代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(body).toContain('one class-level Critical'); | ||
| expect(body).toContain( | ||
| 'rule THAT Critical `fixed` only when the structural change lands', |
There was a problem hiding this comment.
[Suggestion] This diff adds four coordinated instructional hunks but pins only one — the test-efficacy probe (harnessValidated: true) reverted each hunk one at a time, and reverting the chunk shape check (agent-prompt.ts:526-537), the 3b enumeration-trap bullet (agent-briefs.ts:376), or the root-cause grouping paragraph (SKILL.md:601) leaves every test green (5 of 6 hunks survived; only the ledger paragraph this test pins is gated). The two command-side copies are exactly the hunks that reach review agents — SKILL.md itself says "the command's copy is the one that arrives". (The SKILL.md:487 role-table row is a documented non-normative digest and needs no pin; pinning the agent-briefs.ts:368 heading line is optional.) — Failure scenario: a later cleanup rewriting buildChunkAgentPrompt or the 3b brief drops the shape check / enumeration-trap text with every test in the repo passing; chunk agents and Agent 3b silently stop filing class-closing findings, and the pipeline regresses to per-corner enumeration — the exact failure this PR exists to close — while the pinned backstop reads as if the prospective twin still existed.
Suggested fix: pin the load-bearing hunks beside this test, in the same house style:
// SKILL.test.ts
expect(body).toContain('Also group by ROOT CAUSE, not only by symptom type');
// agent-prompt.test.ts
expect(buildChunkAgentPrompt(PLAN, 13)).toContain('the finding is the SHAPE, not the current corner');
expect(BRIEFS['3b'].brief).toContain('Wrong shape — the enumeration trap');中文说明
[Suggestion] 本 diff 新增四个相互配合的指令块，但只给其中一个打了桩——test-efficacy 探针（harnessValidated: true）逐个回滚每个块：回滚 chunk 形状检查（agent-prompt.ts:526-537）、3b 的 enumeration-trap 条目（agent-briefs.ts:376）或根因聚类段（SKILL.md:601）后所有测试仍全绿（6 个块中 5 个回滚后无人察觉；只有本测试所固定的 ledger 段被门控）。两个命令侧副本恰恰是真正送达评审 agent 的块——SKILL.md 自己也写明 "the command's copy is the one that arrives"。（SKILL.md:487 的角色表行是文档化的非规范摘要，无需打桩；agent-briefs.ts:368 的标题行可选。）— 失败场景：以后某次清理重写 buildChunkAgentPrompt 或 3b brief，删掉形状检查/enumeration-trap 文本而仓库所有测试照常通过；chunk agent 与 Agent 3b 悄悄不再提交关类 finding，流水线退回逐角落枚举——正是本 PR 要关闭的失败——而被固定的兜底读起来仿佛前瞻版本仍存在。
建议修复：在此测试旁以同款风格给承重块打桩（见英文侧代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| After verification, identify **confirmed** findings that describe the **same type of problem** across different locations (e.g., "missing error handling" appearing in 8 places). Only group findings with the **same confidence level** together — do not mix high-confidence and low-confidence findings in the same pattern group. For each pattern group: | ||
| After verification, identify **confirmed** findings that describe the **same type of problem** across different locations (e.g., "missing error handling" appearing in 8 places). Only group findings with the **same confidence level** together — do not mix high-confidence and low-confidence findings in the same pattern group. | ||
|
|
||
| **Also group by ROOT CAUSE, not only by symptom type.** When several confirmed findings are different symptoms of ONE root cause that needs a single structural fix — six XML-corner bypasses whose root is hand-rolled parsing, many call sites broken by one wrong contract — merge them into a single finding whose **Suggested fix is the class-closing change** and whose individual symptoms are the evidence, not N findings to patch one at a time. This is the within-round twin of the unbounded-family escalation in Step 6. |
There was a problem hiding this comment.
[Suggestion] This root-cause grouping rule is silent on confidence, while the paragraph directly above says "do not mix high-confidence and low-confidence findings in the same pattern group", and the aggregate format carries no Confidence field — the machine-level Finding has exactly one confidence, defaulting to high when omitted. A root-cause family whose members split across confidence levels — the ordinary case for the "six XML-corner bypasses" shape this paragraph names, where some corners are probe-demonstrated and others inferred — cannot satisfy both instructions at once. — Failure scenario: merging across confidence leaves the aggregate's confidence undefined; at the machine level it defaults high and posts inline, carrying low-confidence symptoms onto the PR against the never-post-low-confidence guarantee. Honoring the confidence rule instead emits a posted aggregate plus a terminal-only twin naming the same fix (contradicting "merge them into a single finding"), or leaves the low-confidence symptoms as standalone Needs-Human-Review entries — the per-corner enumeration shape this paragraph exists to kill, rendered a second time.
Suggested fix:
| **Also group by ROOT CAUSE, not only by symptom type.** When several confirmed findings are different symptoms of ONE root cause that needs a single structural fix — six XML-corner bypasses whose root is hand-rolled parsing, many call sites broken by one wrong contract — merge them into a single finding whose **Suggested fix is the class-closing change** and whose individual symptoms are the evidence, not N findings to patch one at a time. This is the within-round twin of the unbounded-family escalation in Step 6. | |
| **Also group by ROOT CAUSE, not only by symptom type.** When several confirmed findings are different symptoms of ONE root cause that needs a single structural fix — six XML-corner bypasses whose root is hand-rolled parsing, many call sites broken by one wrong contract — merge them into a single finding whose **Suggested fix is the class-closing change** and whose individual symptoms are the evidence, not N findings to patch one at a time. A root-cause merge carries the confidence of the class-closing claim itself; low-confidence symptoms are folded as evidence under it rather than left as standalone entries. This is the within-round twin of the unbounded-family escalation in Step 6. |
中文说明
[Suggestion] 这条根因聚类规则对置信度只字未提，而紧邻上一段写着"同一 pattern group 不得混用高/低置信度 finding"，且聚合格式没有 Confidence 字段——机器层的 Finding 恰有一个 confidence，缺省时默认为 high。一个成员置信度参差的根因家族——正是本段所举 "six XML-corner bypasses" 形状的常态（部分角落由探针证实、部分仅靠推断）——无法同时满足两条指令。— 失败场景：跨置信度合并则聚合的置信度无定义；机器层默认 high 并行内发布，把低置信度症状带上 PR，违反"低置信度绝不发行内评论"的保证。若遵守置信度规则，则产出一份已发布的聚合加一份仅终端的同修复孪生（与"合并为单一 finding"矛盾），或把低置信度症状留成独立的 Needs-Human-Review 条目——本段要消灭的逐角落枚举形状被二次渲染。
建议修复：采用 suggestion 块中的改法，给新段落补一句置信度条款（根因合并承载关类断言自身的置信度，低置信度症状作为其下证据折叠，而非留成独立条目）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(body).toContain('remove it now — this convergence supersedes'); | ||
| }); | ||
|
|
||
| it('pins the unbounded-family escalation on the divergence-class ruling', () => { |
There was a problem hiding this comment.
[Suggestion] This pin protects the escalation paragraph's trigger phrases but not its anti-enumeration clause — deleting only "Once it is open for a family, do not enumerate further siblings of that family as separate findings; they are evidence under the one Critical." from SKILL.md:749 leaves both pinned substrings intact and this test green (observed by mutation probe: the deletion left the suite 9/9 green; adding the assertion below made the mutant fail). That clause is the one that stops the enumeration spin the paragraph exists to end, and it sits in tension with the still-unconditional sibling-filing rules on both sides of it — making it the most probable target of a follow-up "resolve the contradiction" edit, which this test's own comment says "must fail a test, not slip through". — Failure scenario: a later edit resolves that tension by deleting or paraphrasing the anti-enumeration sentence (leaving both pinned substrings intact); the pin stays green, and the pipeline silently regresses to filing per-entrance sibling Criticals for unbounded families every round — the exact spin the paragraph exists to end — while the test reads as protecting it.
Suggested fix:
expect(body).toContain(
'do not enumerate further siblings of that family as separate findings',
);中文说明
[Suggestion] 这个 pin 保护了升级段落的触发短语，却没有保护它的反枚举条款——只从 SKILL.md:749 删掉 "Once it is open for a family, do not enumerate further siblings of that family as separate findings; they are evidence under the one Critical." 这一句，两个被固定的子串都完好，本测试仍绿（突变探针实测：删除后套件 9/9 全绿；加上下面的断言后突变体变红）。该条款正是终止本段所要终结的枚举空转的那一句，且与两侧仍然无条件的 sibling 上报规则相抵触——最可能成为后续"消除矛盾"编辑的目标，而本测试自己的注释写明这类编辑"必须让测试红，而不是悄悄溜过"。— 失败场景：以后某次编辑为消除该矛盾而删除或改写反枚举句（两个固定子串完好），pin 仍绿，流水线悄悄退回对无界家族逐入口上报 sibling Critical——正是本段要终结的空转——而测试读起来仍在保护它。
建议修复：加一条断言（见英文侧代码）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
… the pipeline Round on the boundedness change surfaced two real design gaps (gpt-5.6-sol) plus four coordination suggestions; all addressed. - R1-1: the cross-round escalation was internally inconsistent — it labelled a family unbounded from round count, governed only the ledger path (not the open-blocker re-check), never superseded prior sibling ids, and forced Critical against 3b's severity-from-risk rule. Rewrite it as one rule over BOTH sibling paths: boundedness is a property of the SURFACE (recurrence is only a signal); an unbounded family collapses into one class-level finding under a single stable id that supersedes the family's prior ids; severity is the demonstrated risk of the shape, not automatically a blocker; fixed only when the structural fix lands. - R1-3: root-cause grouping did not survive the pipeline — the pattern merge promotes to the highest severity, expands into per-location comments, and so splits back into N ledger ids next round. Reframe it as the SAME single class-level finding: one anchor at the root, symptoms as body evidence, the root's own severity and confidence, decided on the post-reverse-audit union. - Extract the enumeration-trap text into one exported ENUMERATION_TRAP_LENS constant (mirroring MODELED_SYSTEM_EXECUTION_LENS), interpolated into the 3b brief and injected into the chunk brief, so the two delivery paths cannot drift. - Pin the load-bearing hunks: the surface/collapse/structural-fix clauses, the root-cause-not-pattern-merge rule, and the lens reaching both the 3b and chunk briefs.
|
All six addressed in R1-1 (escalation internally inconsistent) — rewrote it as one rule over both sibling paths (the ledger R1-3 (root-cause aggregation doesn't survive the pipeline) — reframed it as the same single class-level finding, explicitly not a pattern-aggregation: one anchor at the root, symptoms as body evidence, the root's own severity (no promotion to the highest symptom) and confidence (a low-confidence symptom can't promote the whole onto the PR — the confidence Suggestion), decided on the post-reverse-audit union so a reverse-audit sibling doesn't post separately. One id, so no next-round split. Suggestions: lens written twice → extracted one exported 243 tests pass (SKILL.test 10, agent-prompt 233); tsc/eslint/prettier clean. 中文说明六条都已在 R1-1(升级规则自相矛盾):重写为覆盖两条 sibling 路径(ledger R1-3(根因聚合过不了流水线):改写为同一条 class-level finding,明确不是 pattern-aggregation:单一 anchor 落在根因、症状作为正文证据、用根因自身的严重度(不提升到最高症状)与置信度(低置信症状不会把整条推上 PR——即置信度那条 Suggestion)、并在 reverse audit 之后的最终并集上判定(否则 reverse-audit 的同根症状会单独发)。一条 id,故下一轮不再拆分。 Suggestion:lens 写了两份→抽成一个导出的 243 测试通过;tsc/eslint/prettier 干净。 |
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 — reached the 5-round cap without converging (round 5 still surfaced a new finding; verified post-cap).
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — reached the 5-round cap without converging (round 5 still surfaced a new finding; verified post-cap)。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
|
|
||
| **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the <X> surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by <class-id>` in the status table, fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round recovers one entry, not N. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) |
There was a problem hiding this comment.
[Critical] R3-1: This rule claims to govern both sibling-entrance paths (“so the two cannot disagree”), but the diff patched only one of them. The re-check bullet (line 758) received the bounded/unbounded carve-out; the ledger fixed bullet (line 745) was left with its unconditional inline restatement of the old rule — “a still-open sibling becomes a fresh R<round>-<n> entry, never a reason to withhold the original’s fixed”. That same bullet’s delegation (“The sibling-entrance rule from the re-check below applies here unchanged”) now points at the amended rule, so the bullet simultaneously delegates to the conditional rule and restates the unconditional one — the colon presents the stale restatement as the very content of the delegated rule. Pre-diff both paths were consistent; the contradiction is newly created by this diff. Reported by five independent review agents; the verifier traced all three lines at HEAD. — Failure scenario: a round ≥ 2 re-review whose ledger carries a divergence-class entry on an unbounded surface (a hand-rolled parser bypass family — the exact overlap of “divergence-class” and “unbounded” both texts define), one entrance patched, siblings still open → an executor reading the ledger bullet — the instruction local to the ruling being made — files each sibling as a fresh R<round>-<n> entry instead of collapsing the family into the one class-level finding; the enumeration this PR exists to end is rebuilt on the path this paragraph claims to govern, and which instruction wins is per-run judgement.
Suggested fix — mirror the carve-out into the ledger bullet’s restatement: “…checked one by one — for a bounded family a still-open sibling becomes a fresh R<round>-<n> entry (for an unbounded surface, apply the bounded/unbounded rule below instead), never a reason to withhold the original’s fixed.”
中文说明
[Critical] R3-1：该规则声称同时管辖两条 sibling 入口路径（“因此二者不可能打架”），但本 diff 只修补了其中一条。复核 bullet（758 行）拿到了 bounded/unbounded 分支；ledger fixed bullet（745 行）仍保留旧规则的无条件内联复述——“a still-open sibling becomes a fresh R<round>-<n> entry”。同一 bullet 的委托句（“The sibling-entrance rule from the re-check below applies here unchanged”）现在指向已修改的规则，于是该 bullet 既委托给条件化规则、又复述无条件规则——冒号把过时的复述呈现为被委托规则的内容本身。diff 之前两条路径一致；该矛盾是本 diff 新引入的。五个独立评审 agent 报告了此问题，verifier 已在 HEAD 逐行核实。— 失败场景：第 2 轮及以后的复审，ledger 中有一条无界面上的 divergence 类条目（例如手搓解析器绕过家族——正是两段文字定义的交集），一个入口已修补、兄弟入口仍开放 → 执行者按 ledger bullet 的本地复述把每个 sibling 作为新的 R<round>-<n> 条目上报，而不是把家族收敛为一条 class-level finding；本 PR 要终结的枚举在这段文字声称管辖的路径上被重建，哪条指令胜出取决于每次运行的判断。
建议修复——把同样的分支镜像到 ledger bullet 的复述中：“…checked one by one — for a bounded family a still-open sibling becomes a fresh R<round>-<n> entry (for an unbounded surface, apply the bounded/unbounded rule below instead), never a reason to withhold the original’s fixed.”
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
|
|
||
| **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the <X> surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by <class-id>` in the status table, fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round recovers one entry, not N. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) |
There was a problem hiding this comment.
[Critical] R3-6: The collapse creates a superseded lifecycle state that the round-handoff machinery never consumes. Probe-verified against the sources at the reviewed commit: (a) pr-context’s classifyInlineThreads fetches all pull comments unfiltered (no resolved-thread filter, no author filter) and promotes every thread whose body carries a blocker signal — the **[Critical]** marker the siblings carry — so superseded sibling comments stay on the PR and are re-rendered into “Blockers to re-check” every later round; (b) the re-check’s verdict set is exactly three bullets — still stands / fixed by this diff / cannot tell — with no disposition for a superseded family member; (c) still stands mandates re-posting inline under the original id, and buildLedger/CARRIED_ID_RE then re-carry all N ids into the next ledger marker. “The next round recovers one entry, not N” holds only for the ledger-marker recovery source, not the PR-thread recovery source the same Step 6 section walks. — Failure scenario: round N files siblings R_N-1..R_N-k as **[Critical]** inline comments on an unbounded surface; round N+1 correctly executes the collapse (class finding posted, siblings ruled superseded by <class-id>); in round N+2 all k sibling threads are back in “Blockers to re-check” (probe-observed: all resurfaced). Every available verdict violates the collapse rule: still stands re-posts each sibling and re-carries all k ids (probe-observed: the next-round ledger recovered N entries, not one); fixed is forbidden until the structural change lands; cannot tell on each sibling caps the event at COMMENT every round until the structural fix.
Suggested fix — define the re-check outcome for a blocker that is a superseded family member (e.g. a fourth verdict superseded by <class-id> recorded in the status table, neither re-posted nor counted toward cannotTellCriticals, ruled against the class finding), or instruct resolving the sibling threads at collapse time; and scope “recovers one entry, not N” to the ledger marker.
中文说明
[Critical] R3-6：collapse 产生的 superseded 生命周期状态没有任何轮次交接机制消费。已在被审 commit 的源码上以探针核实：(a) pr-context 的 classifyInlineThreads 无过滤地拉取全部评论（无 resolved 过滤、无作者过滤），凡正文带 blocker 信号（即 sibling 所带的 **[Critical]** 标记）的线程一律提升——于是被 supersede 的 sibling 评论留在 PR 上，每一轮都被重新渲染进 “Blockers to re-check”；(b) 复核的裁决集恰好三条——still stands / fixed by this diff / cannot tell——没有针对被 supersede 的家族成员的处置；(c) still stands 强制按原 id 重新发行内评论，buildLedger/CARRIED_ID_RE 随即把全部 N 个 id 带进下一轮 ledger 标记。“下一轮只恢复一条、而不是 N 条” 只对 ledger-marker 恢复源成立，对同一 Step 6 章节同样要走的 PR-thread 恢复源不成立。— 失败场景：第 N 轮在无界面上把 sibling R_N-1..R_N-k 作为 **[Critical]** 行内评论发布；第 N+1 轮正确执行 collapse（发布 class finding，sibling 判 superseded by <class-id>）；第 N+2 轮全部 k 条 sibling 线程回到 “Blockers to re-check”（探针实测：全部重现）。所有可用裁决都违反 collapse 规则：still stands 会逐条重发 sibling 并重新携带全部 k 个 id（探针实测：下一轮 ledger 恢复了 N 条而不是一条）；fixed 在结构性修法落地前被禁止；对每条 sibling 判 cannot tell 则每轮都把事件压在 COMMENT，直到结构性修法落地。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
|
|
||
| **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the <X> surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by <class-id>` in the status table, fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round recovers one entry, not N. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) |
There was a problem hiding this comment.
[Critical] R3-7: The rule’s completeness claims are false for a third sibling-entrance path: a brand-new sibling found in the current round by a Step 3 finder or Step 5 auditor over the incremental diff, while the family’s class finding is already on the ledger and open. Nothing folds it into the class finding: Step 4’s root-cause collapse decides “on the final union after the reverse audit” — that union is the within-round findings union and contains no ledger entries (the ledger is consumed exclusively by Step 6), so a live class finding cannot be a merge partner; dedup merges on “same file, same line range, same root cause” — the new hole is a different location; no agent prompt carries the ledger (verified in agent-prompt.ts), and RECALL mandates filing every candidate whose failure scenario can be named. — Failure scenario: round N posts the class finding R_N-1; round N+1 the author answers with per-entrance patches — the exact response the rule anticipates (“never when the latest entrance is patched”); the incremental diff is exactly those patches; a finder finds the fresh patch left a corner open (the “sibling hole one backtick away” shape the re-check bullet itself cites) and files it; Step 6 rules R_N-1 still stands (re-reported) and posts the residual hole under a fresh id — the family occupies two ids, and each further entrance-patch round that leaves a residual corner mints another, rebuilding the enumeration through the finder path.
Suggested fix — extend the rule to name the third path: when a family’s class finding is already on the ledger and open, a new entrance discovered in the current round folds into the class finding’s re-report as evidence under the class id at Step 6 rendering, rather than being filed under its own id.
中文说明
[Critical] R3-7：该规则的完备性声明对第三条 sibling 入口路径不成立：当前轮内 Step 3 finder 或 Step 5 auditor 在增量 diff 中新发现的 sibling——此时家族的 class finding 已在 ledger 上且仍开放。没有任何机制把它折叠进 class finding：Step 4 的根因收敛“在 reverse audit 之后的最终并集上判定”——该并集是本轮 findings 的并集，不含 ledger 条目（ledger 只由 Step 6 消费），所以活的 class finding 无法成为合并对象；dedup 按 “same file, same line range, same root cause” 合并——新洞在不同位置；没有任何 agent prompt 携带 ledger（已在 agent-prompt.ts 核实），而 RECALL 要求上报每一个能说出失败场景的候选。— 失败场景：第 N 轮发布 class finding R_N-1；第 N+1 轮作者以逐入口补丁回应——正是该规则预期的回应（“never when the latest entrance is patched”）；增量 diff 恰是这些补丁；finder 发现新补丁仍漏掉一个角落（复核 bullet 自己引用的 “sibling hole one backtick away” 形状）并上报；Step 6 判 R_N-1 still stands（重报）并把残余洞以新 id 发布——家族占用两个 id，此后每一轮留下残余角落的入口补丁都再铸一个新 id，枚举经 finder 路径被重建。
建议修复——扩展规则以点名第三条路径：当家族的 class finding 已在 ledger 上且仍开放时，本轮新发现的入口在 Step 6 渲染时折叠进 class finding 的重报、作为 class id 下的证据，而不是以自己的 id 上报。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
|
||
| - **still stands** — the defect is present in the code you just read. It blocks: the event is `REQUEST_CHANGES`, and the finding goes inline (or into the body if it cannot be anchored). | ||
| - **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. **And "the mechanism" means the FAMILY, not the one input the fix answered**: when the blocker is a divergence-class defect — a parser bypass, an escaping hole, a filter gap — enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling `fixed`. A re-check that tested only the reported input has ruled `fixed` over a sibling hole one backtick away (measured; DESIGN.md — The code-span door beside the fixed fence). A sibling entrance you found still open is a **new finding** (report it), and the original blocker is still `fixed` only if its own input is closed — the two rulings are separate, and conflating them is how the second hole ships unreviewed. | ||
| - **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. **And "the mechanism" means the FAMILY, not the one input the fix answered**: when the blocker is a divergence-class defect — a parser bypass, an escaping hole, a filter gap — enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling `fixed`. A re-check that tested only the reported input has ruled `fixed` over a sibling hole one backtick away (measured; DESIGN.md — The code-span door beside the fixed fence). A sibling entrance you found still open is a **new finding** (report it) — **for a bounded family**; for an unbounded surface, apply the bounded/unbounded rule above instead, collapsing the family into the one class-level finding rather than filing the sibling. Either way, the original blocker is still `fixed` only if its own input is closed — the two rulings are separate, and conflating them is how the second hole ships unreviewed. |
There was a problem hiding this comment.
[Suggestion] R3-2: The diff made this bullet’s sibling-filing consequence bounded/unbounded-conditional but left its enumeration precondition unconditional: “enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling fixed”. Line 749 defines an unbounded family as one whose entrances “cannot be enumerated and closed one by one” — so for an unbounded surface the precondition is unsatisfiable by definition, and the “rather than filing the sibling” clause confirms “instead” reaches only the filing, not the enumeration duty. — Failure scenario: a re-review round whose open blocker is divergence-class on an unbounded surface → obeying the precondition literally, the re-checker either spends the round enumerating unbounded corner entrances (the bloat this PR exists to end) or cannot complete the enumeration and falls to cannot tell — capping the verdict at COMMENT even after the structural fix lands, when the class rule (“Rule the class finding fixed only when the structural change lands”) would allow fixed.
Suggested fix — scope the precondition: “when the blocker is a divergence-class defect — for a bounded family, enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling fixed; for an unbounded surface the family ruling is the structural-change test of the bounded/unbounded rule above”.
中文说明
[Suggestion] R3-2：本 diff 把该 bullet 的 sibling 上报后果改为按 bounded/unbounded 分支，但其枚举前提仍无条件：“enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling fixed”。749 行把无界家族定义为入口“cannot be enumerated and closed one by one”的家族——因此对无界面该前提按定义不可满足，且 “rather than filing the sibling” 子句表明 “instead” 只覆盖上报动作，不覆盖枚举义务。— 失败场景：某一复审轮的开放 blocker 是无界面上的 divergence 类 → 字面服从前提时，复核者要么花一整轮枚举无界的角落入口（正是本 PR 要终结的膨胀），要么无法完成枚举而落入 cannot tell——即便结构性修法已落地，裁决也被压在 COMMENT，而 class 规则（“只有结构性修法落地才判 fixed”）本可允许 fixed。
建议修复——给前提加范围：“when the blocker is a divergence-class defect — for a bounded family, enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling fixed; for an unbounded surface the family ruling is the structural-change test of the bounded/unbounded rule above”。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
|
||
| - **still stands** — the defect is present in the code you just read. It blocks: the event is `REQUEST_CHANGES`, and the finding goes inline (or into the body if it cannot be anchored). | ||
| - **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. **And "the mechanism" means the FAMILY, not the one input the fix answered**: when the blocker is a divergence-class defect — a parser bypass, an escaping hole, a filter gap — enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling `fixed`. A re-check that tested only the reported input has ruled `fixed` over a sibling hole one backtick away (measured; DESIGN.md — The code-span door beside the fixed fence). A sibling entrance you found still open is a **new finding** (report it), and the original blocker is still `fixed` only if its own input is closed — the two rulings are separate, and conflating them is how the second hole ships unreviewed. | ||
| - **fixed by this diff** — you traced the blocker's **mechanism** through the code as it now stands and it can no longer fire. Say nothing; do not re-report it. A GitHub thread can read `isResolved: false, isOutdated: false` for a bug a later commit fixed on an adjacent line — the flag tracks the anchored line, not the fix, so the flag is not evidence either way. Only the code is. **And "the mechanism" means the FAMILY, not the one input the fix answered**: when the blocker is a divergence-class defect — a parser bypass, an escaping hole, a filter gap — enumerate the sibling entrances to the same mechanism and check each one at the reviewed commit before ruling `fixed`. A re-check that tested only the reported input has ruled `fixed` over a sibling hole one backtick away (measured; DESIGN.md — The code-span door beside the fixed fence). A sibling entrance you found still open is a **new finding** (report it) — **for a bounded family**; for an unbounded surface, apply the bounded/unbounded rule above instead, collapsing the family into the one class-level finding rather than filing the sibling. Either way, the original blocker is still `fixed` only if its own input is closed — the two rulings are separate, and conflating them is how the second hole ships unreviewed. |
There was a problem hiding this comment.
[Suggestion] R3-5: The test-efficacy probe (harnessValidated: true) reverted this hunk — the bounded/unbounded amendment to this bullet — one at a time, and it reverted cleanly with every test still green: the cross-reference routing unbounded families to the collapse rule (“apply the bounded/unbounded rule above instead”) is pinned by nothing; the pinned clauses live in the Step-6 paragraph, not here. — Failure scenario: a follow-up edit resolving the tension between “enumerate sibling entrances” and “collapse the family” can rewrite this bullet back to plain sibling enumeration with SKILL.test.ts green. The open-blocker re-check path would then file N sibling findings per round for an unbounded surface — the enumeration failure mode this PR exists to end, rebuilt on the re-check path.
Suggested fix — add a pin in SKILL.test.ts: expect(skillBody()).toContain('apply the bounded/unbounded rule above instead'); (the clause is unique in SKILL.md).
中文说明
[Suggestion] R3-5：test-efficacy 探针（harnessValidated: true）逐块回滚时，本块——该 bullet 的 bounded/unbounded 修订——回滚后所有测试仍全绿：把无界家族路由到 collapse 规则的交叉引用（“apply the bounded/unbounded rule above instead”）没有任何桩；已钉的子串在 Step-6 段落里，不在这里。— 失败场景：后续为消除 “枚举 sibling 入口” 与 “收敛家族” 之间张力的编辑，可以把该 bullet 改回纯粹的 sibling 枚举而 SKILL.test.ts 保持绿。open-blocker 复核路径随后会对无界面每轮上报 N 条 sibling finding——本 PR 要终结的枚举失败模式在复核路径上被重建。
建议修复——在 SKILL.test.ts 加一条桩：expect(skillBody()).toContain('apply the bounded/unbounded rule above instead');（该子句在 SKILL.md 中唯一）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(BRIEFS['3b'].brief).toContain(ENUMERATION_TRAP_LENS); | ||
| expect(buildChunkAgentPrompt(PLAN, 13)).toContain(ENUMERATION_TRAP_LENS); |
There was a problem hiding this comment.
[Suggestion] R3-3: Both assertions take their expected value from the same constant they are checking the presence of, so the oracle moves together with the code under test — the test pins delivery but cannot see any change to the lens content. Probe-verified in this tree: the mutation export const ENUMERATION_TRAP_LENS = ''; survives (toContain('') is true of every string — both assertions stayed green); stripping the constant’s operational clauses (“file it ONCE, in place of enumerating cases”, the severity rule) also stayed green, because the single-source design updates both delivery sites consistently; the flip check (adding the literal content pin below) turned the mutant red. The SKILL.md half of this same content pins its load-bearing clauses with literal substrings in SKILL.test.ts; the TS constant has no equivalent content pin. — Failure scenario: a follow-up paraphrase of the constant that drops its operational clauses passes both assertions while the 3A and 3B prompts ship with a weakened or emptied enumeration-trap instruction — the regression is invisible to the suite that claims to guard it.
| expect(BRIEFS['3b'].brief).toContain(ENUMERATION_TRAP_LENS); | |
| expect(buildChunkAgentPrompt(PLAN, 13)).toContain(ENUMERATION_TRAP_LENS); | |
| expect(BRIEFS['3b'].brief).toContain(ENUMERATION_TRAP_LENS); | |
| expect(buildChunkAgentPrompt(PLAN, 13)).toContain(ENUMERATION_TRAP_LENS); | |
| expect(ENUMERATION_TRAP_LENS).toContain('file it ONCE, in place of enumerating cases'); |
中文说明
[Suggestion] R3-3：两条断言的期望值取自它们正在检查其存在性的同一个常量，因此 oracle 与被测代码同步移动——测试钉住了投递路径，却看不到 lens 内容的任何变化。已在本树以探针核实：突变 export const ENUMERATION_TRAP_LENS = ''; 存活（toContain('') 对任何字符串为真——两条断言仍绿）；删掉常量的操作性子句（“file it ONCE, in place of enumerating cases”、严重度规则）后仍绿，因为单一来源设计会一致地更新两处投递；翻转检查（加入下方的字面内容桩）使突变体变红。同一内容的 SKILL.md 半边在 SKILL.test.ts 里用字面子串钉住了承重子句；TS 常量没有等价的内容桩。— 失败场景：后续对该常量的改写若丢掉操作性子句，两条断言照旧通过，而 3A 与 3B prompt 携带被削弱乃至清空的 enumeration-trap 指令上线——声称守卫它的测试套件对该回归不可见。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(skillBody()).toContain( | ||
| 'A root-cause family is one class-level finding, NOT a pattern-aggregation', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-8: The pin asserts only the heading clause; the clauses doing the rule’s actual work against the pattern-merge path — “its severity is the demonstrated risk of the root (not the highest symptom), at the root’s own confidence” — are pinned by nothing (grep-verified across all packages). SKILL.md now carries three severity rules in tension: dedup says highest severity, the pattern merge says highest severity, and the root-cause paragraph says root risk at root confidence. Probe-verified: replacing the two clauses with their exact opposite (“the highest severity among the symptoms, at their highest confidence”) left the suite 10/10 green — the test cannot distinguish the rule from its negation. — Failure scenario: a follow-up edit “resolving” the three-rule tension by harmonising on highest-severity drops the root-risk/root-confidence clauses; every test stays green. The regression restores exactly what the paragraph forbids: a low-confidence symptom promoting the aggregate onto the PR, and symptom-max severity replacing root risk.
| expect(skillBody()).toContain( | |
| 'A root-cause family is one class-level finding, NOT a pattern-aggregation', | |
| ); | |
| expect(skillBody()).toContain( | |
| 'A root-cause family is one class-level finding, NOT a pattern-aggregation', | |
| ); | |
| expect(skillBody()).toContain( | |
| 'the demonstrated risk of the **root** (not the highest symptom)', | |
| ); | |
| expect(skillBody()).toContain("at the **root's own confidence**"); |
中文说明
[Suggestion] R3-8：该桩只断言了标题子句；对 pattern-merge 路径真正起作用的子句——“its severity is the demonstrated risk of the root (not the highest symptom), at the root’s own confidence”——没有任何桩（已 grep 全部 packages 核实）。SKILL.md 现在携带三条相互拉扯的严重度规则：dedup 取最高严重度、pattern merge 取最高严重度、根因段落取根因风险与根因自身置信度。探针核实：把这两个子句替换为它们的 exact 反面（“the highest severity among the symptoms, at their highest confidence”）后套件仍 10/10 全绿——测试无法区分规则与其反面。— 失败场景：后续以“统一为最高严重度”来“消解”三规则张力的编辑会丢掉根因风险/根因置信度子句；所有测试保持绿。该回归恰好恢复了段落所禁止的行为：低置信度症状把聚合推上 PR，以及症状最高严重度取代根因风险。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…ee sibling paths Third round closed the remaining gaps in the cross-round escalation: - R3-1: the rule claimed to govern both sibling paths but the ledger `fixed` bullet still restated the unconditional old rule while delegating to the amended one. Mirror the bounded/unbounded carve-out into that restatement. - R3-6: the collapse invented a `superseded` state the machinery never consumed, so a prior sibling resurfaced in Blockers-to-re-check every round with only still-stands / fixed / cannot-tell available — each violating the collapse. Add a fourth `superseded by <class-id>` disposition (not re-posted, not counted toward cannotTellCriticals; the open class finding carries the block), and scope "recovers one entry, not N" to the ledger marker. - R3-7: a brand-new sibling found in the current round by a finder/auditor, while the class finding is already on the ledger, had no home. Name that third path: it folds into the class finding's re-report as evidence under the class id. - R3-2: the open-blocker re-check kept an unconditional enumeration precondition, unsatisfiable for an unbounded surface. Scope it to bounded families. - R3-3/R3-5/R3-8: strengthen the pins — the lens's operational clauses (the delivery assertions were toContain(constant), tautological), the re-check cross-reference, the superseded disposition, and the root-risk/root-confidence clauses of the root-cause rule.
|
All seven addressed in R3-1 (only one path patched) — the ledger R3-6 (superseded state the machinery never consumed) — right: a prior sibling resurfaces in "Blockers to re-check" every round, and all three verdicts violate the collapse. Added a fourth disposition, R3-7 (third path — new sibling this round) — named it: a brand-new sibling found in the current round by a finder/auditor, while the class finding is already on the ledger and open, folds into the class finding's re-report as evidence under the class id at Step 6, not filed under its own id. R3-2 (unconditional enumeration precondition) — scoped it: bounded → enumerate the entrances; unbounded → do not attempt to (they can't be), the family ruling is the structural-change test. Suggestions R3-3/R3-5/R3-8 — strengthened the pins: the lens's operational clauses literally (the delivery assertions were 243 tests pass (SKILL.test 10, agent-prompt 233); tsc/eslint/prettier clean. 中文说明七条全部在 R3-1(只 patch 了一条路径):ledger R3-6(superseded 状态机器不消费):对——旧 sibling 每轮在 "Blockers to re-check" 复现,三种 verdict 都违反 collapse。加了第四种 disposition R3-7(第三条路径——本轮新发现的 sibling):已命名——class finding 已在 ledger 且开着时,本轮 finder/auditor 在增量 diff 上新发现的 sibling,并入 class finding 的 re-report、作为 class id 下的证据,不单列 id。 R3-2(无条件枚举 precondition):已收窄——有界则枚举入口;无界则不尝试枚举(枚举不了),族的裁决用结构性修法测试。 Suggestion R3-3/R3-5/R3-8:强化 pin——lens 的操作性子句(原来的交付断言是 243 测试通过;tsc/eslint/prettier 干净。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — reached the 5-round cap without converging (round 5 surfaced a new candidate; it was verified and rejected after the cap).
[Critical] R4-2: The enumeration lens conflates adversarial input with an unbounded entrance space. It groups an unbounded or adversarial surface into one category and then concludes that the category has no last corner. A finite, exhaustively specified grammar can accept adversarial input while still having a bounded set of productions and entrances, so this instruction can force a class-level structural replacement where exhaustive validation is possible. Separate the predicates: collapse only when the entrance space is actually unbounded; classify adversarial-but-finite surfaces by whether their grammar can be exhaustively validated.
Witness: not run — this is a semantic contradiction in the normative classification rule, not an executable behavior with a deterministic probe.
中文说明
[Critical] R4-2：enumeration lens 把对抗性输入与无界入口空间混为一谈。它把 unbounded or adversarial surface 合并成一类，随后断言整类都 has no last corner。有限且完整规定的 grammar 可以接收对抗性输入，同时仍只有有界的 production 和入口集合，因此该指令可能在可穷尽验证时仍强制要求 class-level 结构替换。请拆分两个判断：只有入口空间确实无界时才 collapse；对对抗性但有限的表面，根据 grammar 是否可穷尽验证来分类。
证据：未运行——这是规范分类规则中的语义矛盾，不是可用确定性 probe 执行的行为。
[Critical] R1-3 (still stands): Root-family normalization still crosses the verification boundary. Before verification, same-root candidates can be deduplicated at the highest severity and an entire merged entry can be treated as pre-confirmed because one member is deterministic, losing the per-symptom severity and confidence needed to derive root risk. After symptom verification, the new paragraph replaces those findings with a materially new class-level claim, root anchor, severity, confidence, and structural remediation, but that synthesized finding is never sent through a verifier before it can be posted as a Critical. Preserve each symptom's verified evidence through deduplication, then verify the exact synthesized class finding before it may supersede or be posted.
Witness: not run — this is a deterministic workflow-ordering defect in prompt instructions; there is no runtime unit whose output can establish that an orchestrating model re-verifies the synthesized class claim.
中文说明
[Critical] R1-3（仍然成立）：root-family 归一化仍然越过验证边界。验证前，同根因候选会按最高严重度去重，并可能因其中一个成员具有确定性证据而把整个合并项视为已确认，导致用于推导根因风险的逐症状严重度与置信度丢失。症状验证后，新段落又用新的类级结论、根锚点、严重度、置信度和结构性修法替换这些 finding，但该合成 finding 在作为 Critical 发布前从未经过 verifier。请在去重过程中保留每个症状已验证的证据，然后在允许替代或发布前验证完整的合成 class finding。
证据：未运行——这是 prompt 指令中的确定性流程顺序缺陷；没有运行时单元能证明 orchestrator 会重新验证合成的 class 结论。
[Critical] R1-1 (still stands): The new superseded by <class-id> lifecycle can retire a previously proven blocker in favor of a weaker class finding. If a sibling is a high-confidence Critical but the synthesized class finding is low-confidence or non-Critical, supersession removes the sibling from re-reporting and from cannotTellCriticals; only the weaker class finding carries forward, so the review can reach a zero-Critical verdict while the concrete proven defect remains live. Permit supersession only when the replacement preserves at least the strongest demonstrated severity and confidence, or keep the prior Critical open until an equivalently strong verified class finding replaces it.
Witness: not run — this is a normative cross-round lifecycle defect; no deterministic runtime unit chooses the confidence and severity of a synthesized class replacement.
中文说明
[Critical] R1-1（仍然成立）：新增的 superseded by <class-id> 生命周期可能用更弱的 class finding 移除先前已证明的 blocker。如果 sibling 是高置信度 Critical，而合成 class finding 只有低置信度或非 Critical，supersession 会停止重报 sibling，也不计入 cannotTellCriticals；只有较弱的 class finding 继续传递，因此具体且已证实的缺陷仍存在时，评审仍可能得到零 Critical 裁决。仅当替代项至少保留最强已证明的严重度与置信度时才允许 supersede；否则在同等强度且已验证的 class finding 出现前继续保留旧 Critical。
证据：未运行——这是规范性的跨轮生命周期缺陷；没有确定性运行时单元决定合成 class 替代项的置信度与严重度。
中文说明
未审查：reverse audit — reached the 5-round cap without converging (round 5 surfaced a new candidate; it was verified and rejected after the cap)。
[Critical] R4-2: The enumeration lens conflates adversarial input with an unbounded entrance space. It groups an unbounded or adversarial surface into one category and then concludes that the category has no last corner. A finite, exhaustively specified grammar can accept adversarial input while still having a bounded set of productions and entrances, so this instruction can force a class-level structural replacement where exhaustive validation is possible. Separate the predicates: collapse only when the entrance space is actually unbounded; classify adversarial-but-finite surfaces by whether their grammar can be exhaustively validated.
Witness: not run — this is a semantic contradiction in the normative classification rule, not an executable behavior with a deterministic probe.
中文说明
[Critical] R4-2：enumeration lens 把对抗性输入与无界入口空间混为一谈。它把 unbounded or adversarial surface 合并成一类，随后断言整类都 has no last corner。有限且完整规定的 grammar 可以接收对抗性输入，同时仍只有有界的 production 和入口集合，因此该指令可能在可穷尽验证时仍强制要求 class-level 结构替换。请拆分两个判断：只有入口空间确实无界时才 collapse；对对抗性但有限的表面，根据 grammar 是否可穷尽验证来分类。
证据：未运行——这是规范分类规则中的语义矛盾，不是可用确定性 probe 执行的行为。
[Critical] R1-3 (still stands): Root-family normalization still crosses the verification boundary. Before verification, same-root candidates can be deduplicated at the highest severity and an entire merged entry can be treated as pre-confirmed because one member is deterministic, losing the per-symptom severity and confidence needed to derive root risk. After symptom verification, the new paragraph replaces those findings with a materially new class-level claim, root anchor, severity, confidence, and structural remediation, but that synthesized finding is never sent through a verifier before it can be posted as a Critical. Preserve each symptom's verified evidence through deduplication, then verify the exact synthesized class finding before it may supersede or be posted.
Witness: not run — this is a deterministic workflow-ordering defect in prompt instructions; there is no runtime unit whose output can establish that an orchestrating model re-verifies the synthesized class claim.
中文说明
[Critical] R1-3（仍然成立）：root-family 归一化仍然越过验证边界。验证前，同根因候选会按最高严重度去重，并可能因其中一个成员具有确定性证据而把整个合并项视为已确认，导致用于推导根因风险的逐症状严重度与置信度丢失。症状验证后，新段落又用新的类级结论、根锚点、严重度、置信度和结构性修法替换这些 finding，但该合成 finding 在作为 Critical 发布前从未经过 verifier。请在去重过程中保留每个症状已验证的证据，然后在允许替代或发布前验证完整的合成 class finding。
证据：未运行——这是 prompt 指令中的确定性流程顺序缺陷；没有运行时单元能证明 orchestrator 会重新验证合成的 class 结论。
[Critical] R1-1 (still stands): The new superseded by <class-id> lifecycle can retire a previously proven blocker in favor of a weaker class finding. If a sibling is a high-confidence Critical but the synthesized class finding is low-confidence or non-Critical, supersession removes the sibling from re-reporting and from cannotTellCriticals; only the weaker class finding carries forward, so the review can reach a zero-Critical verdict while the concrete proven defect remains live. Permit supersession only when the replacement preserves at least the strongest demonstrated severity and confidence, or keep the prior Critical open until an equivalently strong verified class finding replaces it.
Witness: not run — this is a normative cross-round lifecycle defect; no deterministic runtime unit chooses the confidence and severity of a synthesized class replacement.
中文说明
[Critical] R1-1（仍然成立）：新增的 superseded by <class-id> 生命周期可能用更弱的 class finding 移除先前已证明的 blocker。如果 sibling 是高置信度 Critical，而合成 class finding 只有低置信度或非 Critical，supersession 会停止重报 sibling，也不计入 cannotTellCriticals；只有较弱的 class finding 继续传递，因此具体且已证实的缺陷仍存在时，评审仍可能得到零 Critical 裁决。仅当替代项至少保留最强已证明的严重度与置信度时才允许 supersede；否则在同等强度且已验证的 class finding 出现前继续保留旧 Critical。
证据：未运行——这是规范性的跨轮生命周期缺陷；没有确定性运行时单元决定合成 class 替代项的置信度与严重度。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| 'the cross-chunk half of removed-behavior. Audit the deletions in your own territory; do ' + | ||
| 'not conclude a deletion is unreplaced merely because its replacement is not in your range.', | ||
| '', | ||
| '**Shape check (part of code quality — the altitude lens, scoped to your ' + |
There was a problem hiding this comment.
[Critical] R4-1: Unreachable chunks receive conflicting output contracts. When unreachable is true, the prompt first requires the agent to return exactly Uncoverable: chunk …, but this unconditional block then asks it to review every dimension, apply the shape lens, and use the normal finding format. The adjacent modeled-system and budget blocks correctly guard on !unreachable; this one does not. A model can therefore turn a disclosed coverage gap into a malformed finding or failed coverage receipt. Guard the entire ordinary review/finding block with !unreachable, or return immediately after constructing the exact receipt.
Witness: not run — contradictory prompt instructions do not have a deterministic execution that can choose which instruction a model will follow.
中文说明
[Critical] R4-1：不可读取的 chunk 收到了互相冲突的输出契约。unreachable 为 true 时，prompt 先要求 agent 精确返回 Uncoverable: chunk …，但这个无条件区块随后又要求它审查所有维度、应用 shape lens，并使用普通 finding 格式。相邻的 modeled-system 和 budget 区块都正确使用了 !unreachable，这里只是没有。模型因此可能把已披露的覆盖缺口变成格式错误的 finding 或失败的覆盖回执。请用 !unreachable 包住整个普通审查/finding 区块，或在构建精确回执后立即返回。
证据：未运行——互相矛盾的 prompt 指令不存在可确定选择模型会遵循哪条指令的运行时执行。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
…block R4-1: an unreachable chunk was handed a conflicting contract — "return exactly the Uncoverable line" and then the ordinary review block (dimensions, the shape lens this PR added, the finding format). The modeled-system and tool-budget blocks already guard on !unreachable; this makes the whole ordinary contract do the same by construction: the unreachable branch now returns right after building the receipt, so no review instruction reaches it. Pins that an unreachable chunk carries neither the enumeration-trap lens nor the review block.
|
Addressed in `462086d`. R4-1 (unreachable chunk, conflicting contract) — right, and you named the clean fix: the unreachable branch now returns right after building the Uncoverable receipt, so no ordinary review instruction reaches it — the whole contract (dimensions, the shape lens this PR added, the finding format) is 234 tests pass; tsc/eslint/prettier clean. 中文说明已在 `462086d` 处理。 R4-1(不可达 chunk 契约冲突):对,你也给了干净修法——不可达分支现在在构造完 Uncoverable 回执后立即 return,任何常规审查指令都到不了它:整个契约(维度、本 PR 加的 shape lens、finding format)因此按构造就是 234 测试通过;tsc/eslint/prettier 干净。 |
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 — reached the 5-round cap without converging (round 5 surfaced a new candidate; verified after the cap).
Not explored to full depth (tool budget reached): "PR #9095 (QwenLM/qwen-code): teaches the /review skill to…": none — all checks in my method completed within budget.; "PR #9095 (QwenLM/qwen-code): teaches the /review skill to…": could not execute vitest — neither the review worktree nor the parent checkout has node_modules installed and a monorepo install exceeded the tool budget; t….
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — reached the 5-round cap without converging (round 5 surfaced a new candidate; verified after the cap)。
未探索到全部深度（达到工具调用预算）："PR #9095 (QwenLM/qwen-code): teaches the /review skill to…"：none — all checks in my method completed within budget.；"PR #9095 (QwenLM/qwen-code): teaches the /review skill to…"：could not execute vitest — neither the review worktree nor the parent checkout has node_modules installed and a monorepo install exceeded the tool budget; t…。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **fixed** — the mechanism can no longer fire. Say so, by id, in one line: `R1-2 fixed by <what>`. Do not re-report it as a finding. The sibling-entrance rule from the re-check below applies here unchanged: for a divergence-class entry, `fixed` is a ruling about the family's entrances, checked one by one — for a **bounded** family a still-open sibling becomes a fresh `R<round>-<n>` entry (for an unbounded surface, apply the bounded/unbounded rule below instead of filing the sibling), never a reason to withhold the original's `fixed`. | ||
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
| - **superseded by `<class-id>`** — the entry is a member of a family that collapsed into one class-level finding (the bounded/unbounded rule below). Record `superseded by <class-id>` in the status table; do **not** re-report it and do **not** count it toward `cannotTellCriticals` — the open class finding is the single blocker that carries the family, so the block is preserved without re-enumerating. This is the disposition for a prior sibling that resurfaces in the re-check below after the collapse: it is neither `still stands` (which would re-enumerate and re-carry its id) nor `fixed` (its own mechanism is not closed until the structural change lands) nor `cannot tell` (which would cap the verdict every round until then). |
There was a problem hiding this comment.
[Critical] R1-1: The new superseded by <class-id> lifecycle can retire a previously proven blocker in favor of a weaker class finding. The disposition's block-preservation claim — "the open class finding is the single blocker that carries the family, so the block is preserved" — holds only while the class finding is actually posted and blocking. If a sibling is a high-confidence Critical but the synthesized class finding lands low-confidence (terminal-only: low-confidence findings are never posted; only high-confidence findings count toward the verdict; the ledger is built only from posted findings) or is judged non-Critical ("an infinite surface is not automatically a blocker"), supersession removes the sibling from re-reporting and from cannotTellCriticals — the review can reach a zero-Critical verdict while the concrete proven defect remains live. Re-checked at HEAD: no clause preserves the strongest demonstrated severity/confidence through the collapse; this round independently rediscovered the same mechanism and verified the chain end-to-end (only the trigger frequency is uncertain). — Failure scenario: confirmed high-confidence Critical siblings collapse into a low-confidence class finding → nothing posted, siblings ruled superseded, verdict reaches zero Criticals, the next round recovers no ledger entry, and the proven defect stays live in the code.
Suggested fix:
Supersede only when the class finding preserves at least the strongest severity and confidence demonstrated by the siblings it absorbs; otherwise keep the prior Critical open until an equivalently strong verified class finding replaces it.
中文说明
[Critical] R1-1：新的 superseded by <class-id> 生命周期可能用更弱的 class finding 移除先前已证明的 blocker。该处置的保块声明——"the open class finding is the single blocker that carries the family, so the block is preserved"——只在 class finding 确实被发布且构成阻断时成立。若 sibling 是高置信度 Critical，而合成的 class finding 只有低置信度（terminal-only：低置信度 finding 从不发布；裁决只计高置信度；ledger 只由已发布 finding 构建）或被判为非 Critical（"an infinite surface is not automatically a blocker"），supersession 会停止重报 sibling、也不计入 cannotTellCriticals——具体且已证实的缺陷仍存在时，评审仍可能得到零 Critical 裁决。已在 HEAD 复查：collapse 中没有任何条款保留已证明的最强严重度/置信度；本轮独立重新发现了同一机制并端到端核实了整条链（仅触发频率不确定）。— 失败场景：已确认的高置信度 Critical sibling 收敛进低置信度 class finding → 无发布、sibling 被判 superseded、裁决达到零 Critical、下一轮恢复不到任何 ledger 条目，已证实的缺陷继续留在代码里。
建议修复：
仅当 class finding 至少保留其吸收 sibling 所证明的最强严重度与置信度时才允许 supersede；否则在同等强度且已验证的 class finding 出现前继续保留原 Critical。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // (3A) and injected into the chunk brief (3B) by buildChunkAgentPrompt, each | ||
| // under its own scope framing. Scope-neutral body; the wrapping text supplies | ||
| // "for the whole change" vs "for your territory". | ||
| export const ENUMERATION_TRAP_LENS = `A change that HAND-ROLLS parsing or matching of an **unbounded or adversarial surface** — untrusted input, a rendered format (what a viewer/renderer displays), or a spec/grammar — with per-corner special-cases that accumulate (\`indexOf\`/\`slice\`/regex over structured input, "match what the renderer renders" logic, a re-implemented grammar, a growing hand-listed case set) has **no last corner**, so enumerating cases never converges. The finding is the SHAPE, not the current corner: name the class-closing fix — defer to a real parser, the tool's own authoritative structured output, or a fail-closed decision — and file it ONCE, in place of enumerating cases. Severity follows the risk the shape carries — a hand-rolled parser that can be fooled into a wrong result is **Critical**.`; |
There was a problem hiding this comment.
[Critical] R4-2: The enumeration lens conflates adversarial input with an unbounded entrance space. It groups an unbounded or adversarial surface into one category and concludes the whole category has **no last corner**. A finite, exhaustively specified grammar can accept adversarial input while still having a bounded set of productions and entrances, so this instruction can force a class-level structural replacement where exhaustive validation is possible — and for exactly that input class it contradicts the SKILL.md escalation definition ("a family is unbounded when its entrances cannot be enumerated and closed one by one"). Re-checked at HEAD: the lens text is unchanged since this blocker was filed; the final commit (462086d) touched only agent-prompt.ts. — Failure scenario: a hand-rolled parser for a small, exhaustively specified format that receives untrusted input (bounded entrance space, adversarial data) gets classified as "no last corner" and hit with a structural-replacement demand, escalating a family that exhaustive per-production validation would converge.
Suggested fix: separate the predicates — collapse only when the entrance space is actually unbounded; classify adversarial-but-finite surfaces by whether their grammar can be exhaustively validated.
中文说明
[Critical] R4-2：enumeration lens 把对抗性输入与无界入口空间混为一谈。它把 unbounded or adversarial surface 合并成一类，随后断言整类都 has **no last corner**。有限且完整规定的 grammar 可以接收对抗性输入，同时仍只有有界的 production 和入口集合，因此该指令可能在可穷尽验证时仍强制要求 class-level 结构替换——而且恰好对这一输入类与 SKILL.md 的升级定义（"a family is unbounded when its entrances cannot be enumerated and closed one by one"）矛盾。已在 HEAD 复查：该 blocker 提出后 lens 文本未变；最后一个 commit（462086d）只改了 agent-prompt.ts。— 失败场景：为接收不可信输入的小型完备规定格式手搓解析器（入口空间有界、数据对抗），被分类为 "no last corner" 并被要求结构替换，升级了一个本可通过逐 production 穷尽验证收敛的家族。
建议修复：拆分两个判断——只有入口空间确实无界时才 collapse；对对抗性但有限的表面，根据 grammar 是否可穷尽验证来分类。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // guard against with `!unreachable`; returning here makes the whole ordinary | ||
| // contract do the same by construction. | ||
| return parts.join('\n'); |
There was a problem hiding this comment.
[Suggestion] The R4-1 early return leaves the three pre-existing !unreachable guards (modeled-system lens ~:572, tool-budget block ~:594, Covered receipt ~:633) as always-true dead conditions — unreachable is a const that already returned when true — while their comments still present the per-block guards as the exclusion mechanism. Two contradictory accounts of which line enforces the one-instruction contract. Verification ruled out the latent-trap arm: removing the early return now fails the new chunk-15 pin test, so that regression is not silent; the standing cost is the dead conditions plus the misleading enforcement attribution. — Concrete cost: a maintainer editing the modeled-system, budget, or receipt blocks sees if (!unreachable) beside comments like "NOT for an unreachable chunk" and concludes unreachable chunks flow through that code — maintaining or copying a redundant pattern whose real enforcement sits ~60 lines earlier and is invisible from those sites.
Suggested fix: drop the three now-constant guards (the early return already excludes unreachable chunks from the entire ordinary contract), or rewrite their comments to state they are deliberate belt-and-braces in case the early return is ever removed.
中文说明
[Suggestion] R4-1 的提前 return 使三个既有 !unreachable 守卫（modeled-system lens ~:572、tool-budget 块 ~:594、Covered 回执 ~:633）成为恒真的死条件——unreachable 是 const，为 true 时早已 return——而它们的注释仍把排除机制归于逐块守卫。对"哪一行在强制执行单指令契约"出现了两种互相矛盾的说法。验证排除了潜在陷阱一面：删除提前 return 现在会让新的 chunk-15 pin 测试失败，因此该回归并非无声；留下的代价是死条件与误导性的归属。— 具体代价：维护 modeled-system、budget 或回执块的编辑看到 if (!unreachable) 与 "NOT for an unreachable chunk" 之类的注释，会以为 unreachable chunk 会流经这些代码——从而维护或复制一个冗余模式，而真正的强制执行在约 60 行之前、从这些位置看不到。
建议修复：删除三个现已恒真的守卫（提前 return 已把 unreachable chunk 排除在整个普通契约之外），或把注释改写为"以防提前 return 被移除而刻意保留的双保险"。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // The rule must govern BOTH sibling paths: the open-blocker re-check routes an | ||
| // unbounded family to the collapse rule instead of enumerating (R3-1/R3-5). | ||
| expect(body).toContain('apply the bounded/unbounded rule above instead'); |
There was a problem hiding this comment.
[Suggestion] The comment claims to pin the rule governing BOTH sibling-entrance paths, but the pinned string matches only the re-check bullet; the ledger-side routing clause (for an unbounded surface, apply the bounded/unbounded rule below instead of filing the sibling) (SKILL.md ~745) is pinned by nothing. Probe-verified: deleting that parenthetical keeps the suite 10/10 green; adding the pin below makes the mutant fail. — Failure scenario: a future "resolve the contradiction" edit deletes the ledger bullet's unbounded-routing clause; the suite stays green and the ledger path again instructs filing still-open siblings of an unbounded family as fresh entries while the re-check path folds them into the class finding — the two-path disagreement the collapse rule exists to prevent.
| // The rule must govern BOTH sibling paths: the open-blocker re-check routes an | |
| // unbounded family to the collapse rule instead of enumerating (R3-1/R3-5). | |
| expect(body).toContain('apply the bounded/unbounded rule above instead'); | |
| // The rule must govern BOTH sibling paths: the open-blocker re-check routes an | |
| // unbounded family to the collapse rule instead of enumerating (R3-1/R3-5). | |
| expect(body).toContain('apply the bounded/unbounded rule above instead'); | |
| expect(body).toContain('apply the bounded/unbounded rule below instead'); |
中文说明
[Suggestion] 注释声称 pin 了管辖两条 sibling 入口路径的规则，但被 pin 的字符串只匹配 re-check bullet；ledger 侧的路由从句 (for an unbounded surface, apply the bounded/unbounded rule below instead of filing the sibling)（SKILL.md ~745）没有任何 pin。探针核实：删除该括号从句套件仍 10/10 绿；加上下方的 pin 后 mutant 失败。— 失败场景：未来一次"化解矛盾"的编辑删掉 ledger bullet 的无界路由从句；套件保持绿，ledger 路径又会指示把无界家族仍开放的 sibling 作为新条目上报，而 re-check 路径把它们折叠进 class finding——collapse 规则要防止的两路径分歧。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| | `1c` | **Cross-file tracer** (needs a local tree). Owns the whole cross-file walk. _Consumer direction_: grep every caller of every changed export and check it against the new contract. _Producer direction_: for every field the diff **adds**, grep its **read sites** — a live path reading a field the diff never populates is Critical, and nothing in the build will tell you. | | ||
| | `2` | **Security.** Injection, XSS, SSRF, path traversal, authn/authz bypass, secrets in logs, weak crypto, hardcoded credentials. Includes **option/argument injection into subprocess calls** — a user-controlled positional that starts with `-` or is `.`/`..` becomes a git/gh flag or pathspec (`--output=`, `-f`, `checkout .`); `execFile` does not stop it — validate the value against the subcommand grammar (a ref/name allowlist, reject a leading `-`); a `--` separator ends option parsing but does **not** neutralize a pathspec (`checkout -- .` still discards changes), so the value allowlist is the fix. | | ||
| | `3a` | **Reuse & duplication.** Does the codebase already have this? Greps the shared/utility modules and adjacent files for the _behaviour_ (a literal, an error string, a regex — not a plausible function name), and **names the existing helper to call instead**; a duplication finding that names nothing is not a finding. Also owns **dead code the diff leaves behind**. | | ||
| | `3b` | **Altitude & abstraction fit.** Is each change at the right depth — or a bandaid on shared infrastructure, a downstream compensation for an upstream bug, or a new abstraction serving a single call site? **Names the depth the change should live at**, and the blast radius on the other callers. Also flags the **enumeration trap** — a change that hand-rolls an unbounded/adversarial surface (parsing untrusted input, matching a rendered format, a grammar) instead of deferring to a real parser / authoritative output / a fail-closed decision is a class-closing finding, named once, not enumerated case-by-case. | |
There was a problem hiding this comment.
[Suggestion] Source: [test] — the test-efficacy probe (harnessValidated: true) reverted this hunk — the role-table rewrite whose only substantive change is the enumeration-trap sentence in the 3b row — one hunk at a time, and it reverted cleanly with every test still green. Every other load-bearing SKILL.md change this PR makes is pinned by explicit toContain assertions; nothing pins this row. — Failure scenario: a later edit reverts or paraphrases the enumeration-trap obligation in the 3b role description; the whole suite still passes and the lens silently drops out of the skill's role contract — even though teaching the review skill to close unbounded classes is this PR's stated purpose.
Suggested fix — add to SKILL.test.ts beside the existing pins:
expect(body).toContain('Also flags the **enumeration trap**');中文说明
[Suggestion] 来源：[test]——测试有效性探针（harnessValidated: true）逐 hunk 回退了本 hunk——role table 重写中唯一的实质改动是 3b 行的 enumeration-trap 句——回退干净且所有测试仍绿。本 PR 作出的其他每个 load-bearing SKILL.md 改动都有显式 toContain 断言保护；这一行没有任何 pin。— 失败场景：之后的编辑回退或改写 3b 角色描述中的 enumeration-trap 义务；整个套件仍通过，lens 静默掉出 skill 的角色契约——尽管教会 review skill 关闭无界类正是本 PR 声明的目的。
建议修复——在 SKILL.test.ts 既有 pin 旁添加：
expect(body).toContain('Also flags the **enumeration trap**');— qwen3.8-max via Qwen Code /review (v0.21.11)
| const p = buildChunkAgentPrompt(PLAN, 15); | ||
| expect(p).not.toContain(ENUMERATION_TRAP_LENS); | ||
| expect(p).not.toContain('## What to review'); |
There was a problem hiding this comment.
[Suggestion] The test's comment obligates excluding "the dimension review, the shape lens, or the finding format" from an unreachable chunk's brief, but the assertions pin only the first two — FINDING_FORMAT ("Format each finding using this structure:"), SEVERITY ("Apply the severity definitions") and EXCLUSIONS ("## What is NOT a finding") have no absence pin. Probe-verified: inserting those blocks into the unreachable branch keeps the full suite 234/234 green. — Failure scenario: a restructuring that moves FINDING_FORMAT/SEVERITY/EXCLUSIONS above the early return — or re-adds them to the unreachable branch — restores the two-masters shape the R4-1 fix removed, undetected; an agent obeying the format instead of returning the exact receipt turns a disclosed uncoverable gap into a hard coverage failure.
| const p = buildChunkAgentPrompt(PLAN, 15); | |
| expect(p).not.toContain(ENUMERATION_TRAP_LENS); | |
| expect(p).not.toContain('## What to review'); | |
| const p = buildChunkAgentPrompt(PLAN, 15); | |
| expect(p).not.toContain(ENUMERATION_TRAP_LENS); | |
| expect(p).not.toContain('## What to review'); | |
| expect(p).not.toContain('Format each finding using this structure:'); | |
| expect(p).not.toContain('Apply the severity definitions'); | |
| expect(p).not.toContain('## What is NOT a finding'); |
中文说明
[Suggestion] 测试注释要求把"维度审查、shape lens、或 finding format"排除出不可达 chunk 的 brief，但断言只 pin 了前两者——FINDING_FORMAT（"Format each finding using this structure:"）、SEVERITY（"Apply the severity definitions"）与 EXCLUSIONS（"## What is NOT a finding"）没有缺失 pin。探针核实：把这些块插入 unreachable 分支，整个套件仍 234/234 绿。— 失败场景：一次把 FINDING_FORMAT/SEVERITY/EXCLUSIONS 移到提前 return 之上——或重新加回 unreachable 分支——的重构会无声地恢复 R4-1 修复所移除的 two-masters 形状；agent 若遵从格式而不返回精确回执，会把已披露的 uncoverable 缺口变成硬性覆盖失败。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **fixed** — the mechanism can no longer fire. Say so, by id, in one line: `R1-2 fixed by <what>`. Do not re-report it as a finding. The sibling-entrance rule from the re-check below applies here unchanged: for a divergence-class entry, `fixed` is a ruling about the family's entrances, checked one by one — for a **bounded** family a still-open sibling becomes a fresh `R<round>-<n>` entry (for an unbounded surface, apply the bounded/unbounded rule below instead of filing the sibling), never a reason to withhold the original's `fixed`. | ||
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
| - **superseded by `<class-id>`** — the entry is a member of a family that collapsed into one class-level finding (the bounded/unbounded rule below). Record `superseded by <class-id>` in the status table; do **not** re-report it and do **not** count it toward `cannotTellCriticals` — the open class finding is the single blocker that carries the family, so the block is preserved without re-enumerating. This is the disposition for a prior sibling that resurfaces in the re-check below after the collapse: it is neither `still stands` (which would re-enumerate and re-carry its id) nor `fixed` (its own mechanism is not closed until the structural change lands) nor `cannot tell` (which would cap the verdict every round until then). |
There was a problem hiding this comment.
[Suggestion] The diff adds a second consequence-free ruling (superseded: no block, no cannotTellCriticals cap, no re-report) but leaves the re-check's cheap-gradient warning — "Of the three verdicts, this is the only one with no consequence … Do not take it without the trace" (~:765) — asserting uniqueness for fixed, and attaches no membership-trace obligation to the new ruling. buildLedger ingests only re-posted findings (compose-review.ts ~2370), so a mis-superseded blocker leaves no next-round trace. — Failure scenario: a re-checker facing a live blocker thread rules superseded by <class-id> while misjudging membership (the entry is not part of that family) or citing a class finding never actually posted/open; because the ruling is costless and carries no trace duty, the blocker retires free and silent — not re-reported, not capping the verdict, never re-entering the ledger.
Suggested fix: update the ~:765 warning to cover both consequence-free verdicts ("Of the four verdicts, fixed and superseded are the two with no consequence…") and add the membership-trace obligation to the superseded bullet: do not take it without verifying the family was actually collapsed into the cited <class-id> and the entry genuinely belongs to it.
中文说明
[Suggestion] 本 diff 增加了第二个无后果裁决（superseded：不阻断、不计入 cannotTellCriticals、不重报），但 re-check 的廉价梯度警告——"Of the three verdicts, this is the only one with no consequence … Do not take it without the trace"（~:765）——仍断言只有 fixed 无后果，且新裁决没有任何成员资格 trace 义务。buildLedger 只摄取重报的 finding（compose-review.ts ~2370），因此被误判 superseded 的 blocker 不会留下任何下一轮痕迹。— 失败场景：re-check 者面对一个活的 blocker 线程判 superseded by <class-id>，但成员资格判断错误（该条目不属于那个家族）或引用了一个从未发布/并不开放的 class finding；由于该裁决无代价且无 trace 义务，blocker 免费且无声地退役——不重报、不压裁决、永不重回 ledger。
建议修复：把 ~:765 的警告更新为覆盖两个无后果裁决（"Of the four verdicts, fixed and superseded are the two with no consequence…"），并在 superseded bullet 中加入成员资格 trace 义务：未核实家族确实已折叠进所引用的 <class-id>、且该条目确属该家族之前，不得作出此裁决。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…bound the lens trigger - R1-1 (release-direction): the `superseded` collapse could retire a proven high-confidence Critical sibling behind a class finding that lands low-confidence (never posted) or non-Critical, reaching a zero-Critical verdict while the defect stays live. Supersession now preserves the strongest evidence: collapse only when the class finding is filed at least at the highest severity AND confidence any absorbed sibling demonstrated; otherwise keep the prior Critical open until an equally-strong verified class finding replaces it. - R4-2: the lens conflated "adversarial" with "unbounded" — a small, exhaustively specified grammar accepting untrusted input is bounded and closable by validation. The trigger is now the unboundedness of the entrance space, not the hostility of the input; the SKILL.md 3b digest is aligned. - Superseded is consequence-free like `fixed`: updated the cheap-gradient warning to cover both verdicts and added a membership-trace obligation to the disposition. - Suggestions: note the now-inert downstream `!unreachable` guards as deliberate belt-and-braces; pin the ledger-side routing clause, the 3b-row enumeration-trap sentence, the strongest-evidence clause, and the finding-format/severity/ exclusions absence on an unreachable chunk.
|
Addressed in `3af978e`. (The R1-1 (release-direction — supersession retiring a proven blocker) — real and the most important of the round. Fixed: supersession now preserves the strongest evidence — a family collapses only when the class finding is filed at at least the highest severity AND confidence any absorbed sibling demonstrated; a proven high-confidence Critical entrance is never retired behind a low-confidence/non-Critical class finding (which never posts → nothing carries the block → zero-Critical verdict over a live defect). If the class finding can't carry that strength, the prior Critical stays open until an equally-strong verified class finding replaces it. This makes the whole R4-2 (lens conflates adversarial with unbounded) — right, and it contradicted the SKILL.md definition. Separated the predicates: the trigger is now the unboundedness of the entrance space, not the hostility of the input — a small, exhaustively-specified grammar taking untrusted input is bounded and closable by exhaustive validation, so no structural-replacement demand there. Aligned the 3b digest. Suggestions: 245 tests pass (SKILL.test 11, agent-prompt 234); tsc/eslint/prettier clean. 中文说明已在 `3af978e` 处理。( R1-1(放行方向——supersession 退役已证实的 blocker):真实,也是本轮最重要的。已修:supersession 现在保留最强证据——只有当 class finding 以不低于被吸收 sibling 展示过的最高严重度与置信度归档时才收敛;已证实的高置信 Critical 入口绝不被退役到低置信/非 Critical 的 class finding 背后(后者不会发布 → 没人承载 block → 在活着的缺陷上得到零 Critical 裁决)。若 class finding 扛不住该强度,原 Critical 保持开启,直到一个等强的、已验证的 class finding 替换它。这样整个 R4-2(lens 混淆对抗性与无界):对,且与 SKILL.md 定义矛盾。已拆分谓词:触发条件现在是入口空间的无界性,而非输入的敌意——一个小的、可穷举规约的文法即便接收不可信输入也是有界、可用穷举校验关闭的,不该要求结构性替换。3b 摘要已对齐。 Suggestion: 245 测试通过;tsc/eslint/prettier 干净。 |
| // (3A) and injected into the chunk brief (3B) by buildChunkAgentPrompt, each | ||
| // under its own scope framing. Scope-neutral body; the wrapping text supplies | ||
| // "for the whole change" vs "for your territory". | ||
| export const ENUMERATION_TRAP_LENS = `A change that HAND-ROLLS parsing or matching of a surface whose **entrance space is unbounded** — untrusted input read a rendered format's way, a re-implemented general grammar, \`indexOf\`/\`slice\`/regex over structured input whose per-corner special-cases keep accumulating ("match what the renderer renders" logic, a growing hand-listed case set) — has **no last corner**, so enumerating cases never converges. (Adversarial input alone does NOT make a surface unbounded: a small, exhaustively specified grammar has a bounded, enumerable set of productions and IS closable by exhaustive validation — do not demand a structural replacement there. The trigger is unboundedness of the entrance space, not the mere hostility of the input.) The finding is the SHAPE, not the current corner: name the class-closing fix — defer to a real parser, the tool's own authoritative structured output, or a fail-closed decision — and file it ONCE, in place of enumerating cases. Severity follows the risk the shape carries — a hand-rolled parser that can be fooled into a wrong result is **Critical**.`; |
There was a problem hiding this comment.
[Critical] The prospective class-level finding has no path to high confidence, so it never posts and never reaches the ledger the Step 6 backstop reads.
ENUMERATION_TRAP_LENS instructs the finder that "The finding is the SHAPE, not the current corner ... file it ONCE, in place of enumerating cases" — i.e. explicitly not to produce a concrete triggering corner. But the verify brief in this same file (line 703, unchanged by this PR) gates the top verdict on exactly that artifact:
confirmed (high confidence) — the trace works: you can restate the failure scenario against the real code, naming the triggering input/state and quoting the line(s) that produce the wrong outcome.
A shape finding filed per the lens supplies neither, so the best a compliant verifier can return is confirmed(low). SKILL.md Step 4 then makes that terminal-only — "Low-confidence findings appear only in terminal output ... and are never posted as PR inline comments" — and buildLedger only ever sees posted comments plus body Criticals, so the finding never enters the machine ledger.
That breaks this PR's own headline mechanism. SKILL.md:750 states the cross-round rule is a backstop for a filing that already happened: "(Agent 3b's enumeration-trap check files this same finding prospectively in round 1, before the siblings accumulate; this rule is its cross-round backstop...)". In round 1 there are no absorbed siblings to inherit strength from, so the new supersession floor ("at least the highest severity AND confidence any absorbed sibling demonstrated") does not rescue it — and that clause names this precise failure mode itself: "which never posts, so nothing carries the block and the defect stays live at a zero-Critical verdict".
Second, independent mechanism, post-merge: main has since landed holdUnwitnessedCriticals (packages/cli/src/commands/review/findings.ts:538, #9065), which mechanically demotes any Critical + high + source === 'review' finding whose witness is undefined. A prospectively-filed shape Critical has no executed witness, and nothing in this PR mentions witnesses (0 occurrences in both changed files); the two SKILL.md edits merge cleanly, so nothing forces reconciliation.
Suggested fix: give the class-level finding an explicit evidence contract that satisfies both gates — e.g. require it to carry one demonstrated corner as its witness/trigger while still being filed once as the shape (the corner is evidence for the class, not a separate finding), and state the verifier's rule for it, so it can be confirmed high and posted.
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
| - **superseded by `<class-id>`** — the entry is a member of a family that collapsed into one class-level finding (the bounded/unbounded rule below). Record `superseded by <class-id>` in the status table; do **not** re-report it and do **not** count it toward `cannotTellCriticals` — the open class finding is the single blocker that carries the family, so the block is preserved without re-enumerating. This is the disposition for a prior sibling that resurfaces in the re-check below after the collapse: it is neither `still stands` (which would re-enumerate and re-carry its id) nor `fixed` (its own mechanism is not closed until the structural change lands) nor `cannot tell` (which would cap the verdict every round until then). Because it is consequence-free (no block, no `cannotTellCriticals` cap, no re-report — and `buildLedger` ingests only re-posted findings, so it leaves no trace), do not take it without verifying the family was actually collapsed into the cited `<class-id>` and this entry genuinely belongs to it; a mis-applied `superseded` retires a live blocker silently. | ||
|
|
||
| **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the <X> surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by <class-id>` (the disposition above), fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round's **ledger marker** recovers one entry, not N, and a prior sibling that resurfaces on the PR as its own thread is ruled `superseded`, not re-posted. **A brand-new sibling found in the current round** — by a Step 3 finder or Step 5 auditor over the incremental diff, while the class finding is already on the ledger and open — folds the same way: into the class finding's re-report as evidence under the class id at Step 6 rendering, never filed under its own id. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. **Supersession preserves the strongest evidence**: collapse a family only when the class finding is filed at **at least the highest severity AND confidence any absorbed sibling demonstrated** — a proven high-confidence Critical entrance must not be retired behind a low-confidence or non-Critical class finding (which never posts, so nothing carries the block and the defect stays live at a zero-Critical verdict). If the class finding cannot carry that strength, keep the prior Critical open until an equally-strong verified class finding replaces it. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) |
There was a problem hiding this comment.
[Suggestion] DESIGN.md still records the superseded, unconditional sibling rule this paragraph replaces.
packages/core/src/skills/bundled/review/DESIGN.md:429 describes the Step 6 bar as:
The re-check bar in Step 6 now says it outright: enumerate the sibling entrances before ruling
fixed, report a still-open sibling as a new finding, and keep the two rulings separate.
That is now wrong for exactly the case this PR adds: for an unbounded surface the rules here say the opposite — do not file sibling N, collapse the family into one class-level finding. DESIGN.md is the maintainer-facing narrative ("the narrative lives in this skill's DESIGN.md for humans auditing the rule"), and SKILL.test.ts only enforces the (measured; DESIGN.md — …) pointer bijection for the "Measured incidents" section, so nothing mechanical catches this drift.
Concrete cost: a maintainer auditing the sibling-entrance rule via DESIGN.md gets the superseded policy and may "restore" it or treat the bounded/unbounded fork as unbacked drift. The repo convention supports updating it — 6 of the last 8 origin/main commits touching SKILL.md also touched DESIGN.md.
Suggested fix: amend the DESIGN.md:429 bullet to state the bounded/unbounded fork, or add the rationale entry for this change.
…d reach the ledger The headline mechanism was inert: the lens told the finder to file the SHAPE and not a concrete corner, but Step 4's high-confidence bar requires naming the triggering input and quoting the lines, so a shape finding confirmed only low — terminal-only, never posted, never in the ledger the Step 6 backstop reads (and holdUnwitnessedCriticals from #9065 would demote it regardless). Resolve the tension with an evidence contract rather than dropping either half: the class-level finding is still filed ONCE as the shape (class-closing fix, not per-corner enumeration), but it CARRIES ONE demonstrated corner as its witness — the concrete input/state and the lines producing the wrong outcome, executed where possible — so a verifier confirms it high, it posts, and it survives the unwitnessed-Critical hold. The corner is the class's evidence, not a separate finding. Applied to both the lens (3A/3B) and the Step 6 collapse rule, and pinned in both suites. Also update DESIGN.md's sibling-rule narrative to record the bounded/unbounded fork this PR adds (it still described the old unconditional rule).
|
Addressed in `e954880`. Critical (class finding has no path to high confidence → never posts → never in the ledger) — this is the sharpest finding of the whole review: the headline mechanism was inert. The lens said "file the shape, not a corner," but Step 4's high-confidence bar needs the triggering input + quoted lines, so a shape finding confirmed only low → terminal-only → never posted → never in the ledger the Step 6 backstop reads — and Resolved with an evidence contract, not by dropping either half: the class-level finding is still filed ONCE as the shape (class-closing fix, no per-corner enumeration), but it now carries one demonstrated corner as its witness — the concrete input/state and the lines producing the wrong outcome, executed where possible. So a verifier confirms it high, it posts, it enters the ledger, and it survives the unwitnessed-Critical hold. The corner is the class's evidence, not a separate finding. Applied to both the lens (3A/3B) and the Step 6 collapse rule; pinned in both suites. Suggestion (DESIGN.md drift) — right, 245 tests pass (SKILL.test 11 incl. the DESIGN.md-pointer bijection, agent-prompt 234); tsc/eslint/prettier clean. 中文说明已在 `e954880` 处理。 Critical(class finding 到不了高置信 → 不发布 → 不进 ledger):这是整轮评审最尖锐的一条——headline 机制其实是失效的。lens 说"报形状、别报具体角落",但 Step 4 的高置信门槛需要触发输入 + 引用行号,于是形状 finding 只能 confirmed low → 仅终端 → 不发布 → 不进 Step 6 backstop 读的 ledger;而且 用证据契约解决,而不是砍掉任一半:class-level finding 仍只报一次、按形状(关类修法、不逐角落枚举),但现在携带一个已演示的角落作为 witness——具体输入/状态 + 产生错误结果的行,尽量执行验证。于是 verifier 能确认为高、能发布、进 ledger、并扛过 unwitnessed-Critical hold。该角落是类的证据,不是单独 finding。lens(3A/3B)与 Step 6 collapse 规则都已应用,两套测试都加了 pin。 Suggestion(DESIGN.md 漂移):对, 245 测试通过;tsc/eslint/prettier 干净。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "PR #9095 (QwenLM/qwen-code): teaches the /review skill to…": repo-wide npm run typecheck not run (the two TS files compile under vitest and the change is string constants + imports; type risk negligible).; "PR #9095 (QwenLM/qwen-code): teaches the /review skill to…": full cli/core suites beyond the two directly-touched test files not run.; "PR #9095 (QwenLM/qwen-code), round 5: teaches the /review…": none — finished within budget with no check left undone.; "PR #9095 (QwenLM/qwen-code): teaches the /review skill to…": did not execute the two new test files — the worktree has no node_modules and installing the monorepo's dependencies exceeded this dimension's remit (Agent 7 ….
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："PR #9095 (QwenLM/qwen-code): teaches the /review skill to…"：repo-wide npm run typecheck not run (the two TS files compile under vitest and the change is string constants + imports; type risk negligible).；"PR #9095 (QwenLM/qwen-code): teaches the /review skill to…"：full cli/core suites beyond the two directly-touched test files not run.；"PR #9095 (QwenLM/qwen-code), round 5: teaches the /review…"：none — finished within budget with no check left undone.；"PR #9095 (QwenLM/qwen-code): teaches the /review skill to…"：did not execute the two new test files — the worktree has no node_modules and installing the monorepo's dependencies exceeded this dimension's remit (Agent 7 …。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
| - **superseded by `<class-id>`** — the entry is a member of a family that collapsed into one class-level finding (the bounded/unbounded rule below). Record `superseded by <class-id>` in the status table; do **not** re-report it and do **not** count it toward `cannotTellCriticals` — the open class finding is the single blocker that carries the family, so the block is preserved without re-enumerating. This is the disposition for a prior sibling that resurfaces in the re-check below after the collapse: it is neither `still stands` (which would re-enumerate and re-carry its id) nor `fixed` (its own mechanism is not closed until the structural change lands) nor `cannot tell` (which would cap the verdict every round until then). Because it is consequence-free (no block, no `cannotTellCriticals` cap, no re-report — and `buildLedger` ingests only re-posted findings, so it leaves no trace), do not take it without verifying the family was actually collapsed into the cited `<class-id>` and this entry genuinely belongs to it; a mis-applied `superseded` retires a live blocker silently. | ||
|
|
||
| **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the <X> surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. **The class finding carries one demonstrated entrance as its witness** — the concrete input and the line(s) producing the wrong outcome — so it clears Step 4's high-confidence bar and posts (a shape with no concrete corner confirms only low, is terminal-only, and never reaches the ledger this backstop reads — nor survives the unwitnessed-Critical hold); the entrance is the class's evidence, not a separate finding. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by <class-id>` (the disposition above), fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round's **ledger marker** recovers one entry, not N, and a prior sibling that resurfaces on the PR as its own thread is ruled `superseded`, not re-posted. **A brand-new sibling found in the current round** — by a Step 3 finder or Step 5 auditor over the incremental diff, while the class finding is already on the ledger and open — folds the same way: into the class finding's re-report as evidence under the class id at Step 6 rendering, never filed under its own id. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. **Supersession preserves the strongest evidence**: collapse a family only when the class finding is filed at **at least the highest severity AND confidence any absorbed sibling demonstrated** — a proven high-confidence Critical entrance must not be retired behind a low-confidence or non-Critical class finding (which never posts, so nothing carries the block and the defect stays live at a zero-Critical verdict). If the class finding cannot carry that strength, keep the prior Critical open until an equally-strong verified class finding replaces it. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) |
There was a problem hiding this comment.
[Suggestion] This new rule cites an "unwitnessed-Critical hold" that exists nowhere in the repository — and the companion pin-test comment (agent-prompt.test.ts:328) names it holdUnwitnessedCriticals, styled exactly like a real compose-review input such as cannotTellCriticals. A repo-wide grep for unwitnessed returns exactly two hits: the two lines this PR adds. The substantive protection in the same sentence IS real but lives elsewhere: Step 4's low-confidence → terminal-only → never-posted rule, and buildLedger ingesting only posted findings. — Concrete cost: a maintainer validating this witness clause (or debugging how an unwitnessed Critical got posted) follows the function-shaped name, greps for the guard, and finds nothing; the pin test defends a load-bearing clause by citing a nonexistent second line of defense. Note: current main (#9065) has since landed a real holdUnwitnessedCriticals in findings.ts:538 — if that mechanism is meant, say so explicitly (it demotes only witness-less Criticals, and a not run — <reason> line counts as a witness); otherwise name the low-confidence rule. Suggested fix — replace "— nor survives the unwitnessed-Critical hold" with:
(a shape with no concrete corner confirms only low, and low-confidence findings are terminal-only: they never post and never reach the ledger this backstop reads)
and reword the test comment at agent-prompt.test.ts:328 to match.
中文说明
[Suggestion] 这条新规则引用了一个在仓库中任何地方都不存在的 "unwitnessed-Critical hold"——配套的 pin-test 注释（agent-prompt.test.ts:328）还以 holdUnwitnessedCriticals 命名它，写法与真实的 compose-review 输入（如 cannotTellCriticals）完全一致。全仓库 grep unwitnessed 恰好命中两处：即本 PR 新增的这两行。同一句话中实质性的保护确实存在，但由别的机制承担：Step 4 的 low-confidence → terminal-only → 永不发布规则，以及 buildLedger 只摄取已发布的 findings。— 具体代价：维护者验证该 witness 条款（或排查某个无 witness 的 Critical 为何被发布）时会按这个函数形状的名字去 grep，结果一无所获；pin test 用一个不存在的第二道防线来论证关键条款。注意：当前 main（#9065）已在 findings.ts:538 落地了真实的 holdUnwitnessedCriticals——如果指的就是该机制，请明确说明（它只降级缺少 witness 的 Critical，且 not run — <reason> 行也算 witness）；否则请改为引用 low-confidence 规则。建议修复——把 "— nor survives the unwitnessed-Critical hold" 替换为：
(a shape with no concrete corner confirms only low, and low-confidence findings are terminal-only: they never post and never reach the ledger this backstop reads)
并同步改写 agent-prompt.test.ts:328 的测试注释。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(ENUMERATION_TRAP_LENS).toContain( | ||
| "Carry ONE demonstrated corner as the finding's witness", | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The lens's bounded-surface exception — "(Adversarial input alone does NOT make a surface unbounded…)" in agent-briefs.ts:210 — is its only false-positive guard and the exact fix prior-round Critical R4-2 demanded, but no test pins it. Mutation probe (run against this commit, tree restored afterwards): deleting the sentence leaves all 234 tests green — all four clause pins, both delivery assertions, and the unreachable-chunk test survive; adding the pin below turns that mutant red. — Failure scenario: a future edit removes the exception sentence; every test this diff adds still passes; the lens then reads "hand-rolled parsing of hostile input ⇒ unbounded ⇒ class-closing finding", and 3b/chunk agents — which receive the lens, not SKILL.md's pinned "Boundedness is a property of the SURFACE" clause — file spurious Critical "replace with a real parser" findings against small exhaustively-specified grammars where exhaustive validation would converge.
| expect(ENUMERATION_TRAP_LENS).toContain( | |
| "Carry ONE demonstrated corner as the finding's witness", | |
| ); | |
| expect(ENUMERATION_TRAP_LENS).toContain( | |
| "Carry ONE demonstrated corner as the finding's witness", | |
| ); | |
| expect(ENUMERATION_TRAP_LENS).toContain( | |
| 'Adversarial input alone does NOT make a surface unbounded', | |
| ); |
中文说明
[Suggestion] lens 中的 bounded-surface 例外——agent-briefs.ts:210 里的 "(Adversarial input alone does NOT make a surface unbounded…)"——是它唯一的防误报护栏，也正是上一轮 Critical R4-2 要求的修复，但没有任何测试钉住它。变异探针（在本 commit 上执行，事后已还原工作区）：删除该句后全部 234 个测试仍绿——四个条款 pin、两个 delivery 断言、unreachable-chunk 测试全部存活；加上下方的 pin 即可杀死该变异。— 失败场景：未来某次编辑删除该例外句；本 diff 新增的所有测试依然通过；lens 随之读作 "手搓解析对抗性输入 ⇒ 无界 ⇒ 关类 finding"，而 3b/chunk agent（它们收到的是 lens，而非 SKILL.md 里被 pin 的 "Boundedness is a property of the SURFACE" 条款）会对可穷尽验证的小型完备规定文法误报 Critical 级的 "换真解析器" finding，阻断本可收敛的合并。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **fixed** — the mechanism can no longer fire. Say so, by id, in one line: `R1-2 fixed by <what>`. Do not re-report it as a finding. The sibling-entrance rule from the re-check below applies here unchanged: for a divergence-class entry, `fixed` is a ruling about the family's entrances, checked one by one — for a **bounded** family a still-open sibling becomes a fresh `R<round>-<n>` entry (for an unbounded surface, apply the bounded/unbounded rule below instead of filing the sibling), never a reason to withhold the original's `fixed`. | ||
| - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: <the claim>` — and into the body entry if it cannot be anchored (`R1-2 <the claim>`). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. | ||
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
| - **superseded by `<class-id>`** — the entry is a member of a family that collapsed into one class-level finding (the bounded/unbounded rule below). Record `superseded by <class-id>` in the status table; do **not** re-report it and do **not** count it toward `cannotTellCriticals` — the open class finding is the single blocker that carries the family, so the block is preserved without re-enumerating. This is the disposition for a prior sibling that resurfaces in the re-check below after the collapse: it is neither `still stands` (which would re-enumerate and re-carry its id) nor `fixed` (its own mechanism is not closed until the structural change lands) nor `cannot tell` (which would cap the verdict every round until then). Because it is consequence-free (no block, no `cannotTellCriticals` cap, no re-report — and `buildLedger` ingests only re-posted findings, so it leaves no trace), do not take it without verifying the family was actually collapsed into the cited `<class-id>` and this entry genuinely belongs to it; a mis-applied `superseded` retires a live blocker silently. |
There was a problem hiding this comment.
[Suggestion] This new fourth disposition never reaches the ruling menu the orchestrator is actually handed: pr-context.ts:699 renders the recovered-ledger preamble every round with a hardcoded "(fixed / still stands / cannot tell)" enumeration, omitting superseded. Grep confirms this is the only disposition enumeration in the CLI, and before this diff it was exhaustive — the diff newly incompletes it. This is the residual of prior-round Critical R3-6: the disposition itself now exists and is routed into the re-check, but the menu the orchestrator reads first still names only the three verdicts the rule text says are each wrong for an absorbed sibling. — Failure scenario: a later round whose recovered ledger carries a sibling beside the class id; the orchestrator reads the preamble menu first; still stands re-enumerates, fixed is premature until the structural change lands, cannot tell caps the verdict every round — so an executor anchored on the preamble takes a cannot-tell cap or re-files a retired sibling, reproducing the enumeration churn the collapse exists to end. Suggested fix in packages/cli/src/commands/review/pr-context.ts:699:
**Every entry below is owed a this-round ruling** (fixed / still stands / cannot tell / superseded by <class-id>) under Step 6's previous-round rules
or drop the parenthetical there and defer wholly to "Step 6's previous-round rules".
中文说明
[Suggestion] 新增的第四种处置到不了 orchestrator 实际拿到的裁决菜单：pr-context.ts:699 每一轮都把 recovered-ledger 的前言渲染为硬编码的 "(fixed / still stands / cannot tell)" 枚举，遗漏了 superseded。grep 确认这是 CLI 中唯一的处置枚举，且在本 diff 之前它是完备的——本 diff 使它变得不完备。这是上一轮 Critical R3-6 的残留：处置本身现已存在并已路由进 re-check，但 orchestrator 最先读到的菜单仍只列出三个裁决，而规则文本明确说这三个对被吸收的 sibling 全都是错的。— 失败场景：后续某轮恢复的 ledger 中 class id 旁边还有 sibling；orchestrator 先读到前言菜单；still stands 会重新枚举，fixed 在结构性修法落地前为时过早，cannot tell 每轮都压住裁决——于是锚定在前言上的执行者要么接受 cannot-tell 封顶，要么重新上报已退役的 sibling，把 collapse 要终结的枚举空转重建出来。建议修复 packages/cli/src/commands/review/pr-context.ts:699：
**Every entry below is owed a this-round ruling** (fixed / still stands / cannot tell / superseded by <class-id>) under Step 6's previous-round rules
或直接删去该括号枚举，完全交由 "Step 6's previous-round rules" 决定。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…he menu, pin the exception Three accuracy/coverage suggestions on the current head (the review's other five were stale re-posts of already-landed fixes): - The witness clause cited an "unwitnessed-Critical hold" that does not exist on this branch (the real holdUnwitnessedCriticals is #9065, not yet merged here); the solid guarantee is Step 4's low-confidence → terminal-only → never-posted rule. Reword SKILL.md and the test comment to that real mechanism. - pr-context's recovered-ledger preamble hardcoded the ruling menu as "(fixed / still stands / cannot tell)", omitting the superseded disposition this PR added — an executor reading the preamble first would never see it. Add it. - Pin the lens's bounded-surface exception ("Adversarial input alone does NOT make a surface unbounded") so R4-2's false-positive guard cannot be silently deleted.
|
Addressed in `f0b83ca`. (Five of the eight comments on this head are stale re-posts of already-landed fixes — R3-3 content pins, R4-1 early-return, R5-509 belt-and-braces, R5-140 ledger pin, R5-177 format-absence — all present at HEAD.) Three were genuinely new:
Bounded-surface exception unpinned — added a literal content pin for "Adversarial input alone does NOT make a surface unbounded" so R4-2's false-positive guard can't be silently deleted (the delivery assertions can't see its loss). 310 tests pass (SKILL.test 11, agent-prompt 234, pr-context 65); tsc/eslint/prettier clean. 中文说明已在 `f0b83ca` 处理。(本 head 上八条里有五条是已落地修复的过期重贴——R3-3、R4-1、R5-509、R5-140、R5-177,HEAD 都在。)三条是真新的:
有界面例外没被 pin:加了字面内容 pin,防止 R4-2 的误报守卫被静默删除。 310 测试通过;tsc/eslint/prettier 干净。 |
…ss-escalation # Conflicts: # packages/cli/src/commands/review/pr-context.ts
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 648 passed · 0 failed · 648 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：648 通过 · 0 失败 · 648 总计 Verification reportPR #9007 Deep Verification (follow-up round) —
|
| # | Finding (previous round, head aea72fd3) |
Severity | Status at 23f1af33 |
|---|---|---|---|
| 1 | Ownership commit gate (canCommitSessionOwnership consulted in ownershipReceipt.delivered()) unpinned by any test — M3 survivor (309/309 green with the gate removed) |
Suggestion (coverage gap) | stands — re-measured: with the gate removed, transport 311/311 and the dispatch trio 55/55 stay green. The delta's two new transport tests pin rollback/serialization paths, not the commit gate. Positive control M3b now kills 9 (was 7) — the rollback half got more pinned while the commit half remains unpinned. The predicate itself is unit-pinned at registry level (rejects an ownership commit while session/close is in flight), so the gap is exactly "nobody drives the dispatch-level close-race through delivered()". |
Re-measured carry-forward measurements (all re-run at the new head, none diffed from the old report): A/B cells C1–C7 (new base tip), estimator oracle (65,536 code units + sweeps), mutation matrix M1–M3b, and all focused gates. The input closure changed (6 new commits), so no measurement was carried by shortcut.
Central claim + A/B
Central claim: daemon-owned ACP HTTP pre-attach buffering is bounded by frame count and serialized bytes at per-stream / per-connection / process-global scope, and refusal fails closed on the admitting owner instead of silently evicting an older frame.
The A/B drives each arm's compiled dist/ through real loopback HTTP (express + real SseStream + real HTTP client). Import closure of the acp-http dist is node-builtins-only (re-scanned per file at the new head), so no workspace symlink can contaminate an arm; the base control's packages/cli/node_modules symlink resolves into the main tree, which is a clean control because the PR leaves package.json/package-lock.json/packages/core untouched (verified by diff). Witness: 01-ab-head-arm-bounds-fail-closed.png, 02-ab-base-arm-silent-eviction.png.
| Cell | Scenario | HEAD (23f1af33) |
BASE (299d29c4) |
|---|---|---|---|
| C1 | 257 id-bearing frames, one session, no stream attached | #257 refused (failed); owning session closed; connection survives; guard logs ACP pre-attach frame limit |
all 257 accepted (void API); buffer capped at 256 by silently evicting the oldest (buffer[0].id 1→2) |
| C2 | 257 id-less replies | #257 refused; session closed | buffer grows to 257 > 256 (id-less exempt up to the 1024 hard cap) |
| C3 | 3 × ~120 KiB frames vs 300 KB per-connection cap (constructor seam) | frames 1–2 buffered, bytes charged exactly (245,794); #3 refused; connection retired; guard logs connection budget |
all 3 retained; no byte bound; connection alive |
| C4 | two connections, shared 6-frame global budget | A:3+B:3 charged; B's 4th refused; B destroyed; bystander A intact with all 3; guardFailures counted exactly once |
no global budget — B retains all 4 |
| C5 | buffered frames flushed on attach, real SSE wire | 3 frames byte-equal, in order; promises resolve delivered |
same delivery parity (void API) |
| C6 | 3 × 256 KiB buffered, attach with non-reading client | charged 786,480 B at buffering; seam snapshot pendingDeliveryFrames=3, budget usedFrames=3; after client close all counters 0 |
no lease accounting (fields absent) |
| C7 | delivery-receipt lifecycle | unsettled while buffered; teardown → discarded; wire delivery → delivered |
receipt never invoked (ownership granted pre-delivery by construction) |
Head arm 29/29; base arm 19/19 (every base assertion encodes the predicted broken behaviour).
Delta probes (the six commits since the last round)
Ambiguous WebSocket deliveries (6f3b0770) — mock-free, real ws server/client over loopback TCP, compression off, paused receiver so the write genuinely stays in flight (ws-ambiguity.mjs; witness 03-ws-ambiguous-delivery-preserved.png):
| Probe | HEAD | BASE |
|---|---|---|
| W1 healthy send to a reading client | delivered, frame received byte-equal |
n/a |
| W2 accepted-but-unconfirmed send when the stream closes mid-flight | outcome_unknown (not delivered/failed/closed) |
no sendSerialized at all — ambiguity inexpressible (void send) |
| W3 buffered frame + receipt, WS stream closed mid-delivery | receipt settles exactly outcomeUnknown, never discarded → ownership not rolled back for a delivery that may have happened |
n/a |
Dispatch-level semantics read from code: ownershipReceipt.outcomeUnknown commits only while !conn.destroyed && canCommitSessionOwnership(...), else detaches-without-deleting — never a destructive rollback. Vacuity: M5 (flatten outcome_unknown→delivered) kills exactly the 2 committed ambiguity tests (2 red / 18 green).
Failed-delivery reporting (16cde030) — base replyConn was conn.sendConn(success(id, result)), fire-and-forget: a client whose response could not be delivered waited forever with no signal. Head tracks the outcome and, on 'failed' with the connection alive, sends error(id, INTERNAL_ERROR, 'Response delivery failed') on the same connection. The information is observable to the JSON-RPC client (and guardFailures/preAttach counters surface to GET /daemon/status for guard refusals). Pinned end-to-end by the committed over-the-wire tests (session/load returns an internal error when its %s response cannot be serialized, live+buffered: -32603 frame arrives, connection survives, provisional ownership rolled back, bridge client detached). Vacuity: M4 (delete the retry) kills exactly those 2 tests (2 red / 309 green).
Secondary claims
- Estimator (wire oracle =
Buffer.byteLength(JSON.stringify(·))itself): exact equality on all 65,536 single UTF-16 code units, 2,000 seeded random strings, 31 adversarial surrogate/escape combos; early-exit returns exactlymin(actual, limit+1)across a 48-point limit sweep; 2 MB mixed payload in 35 ms. Base formula replay under-charges 4/6 escape-requiring shapes (700×U+0001: actual 4202 vs base 702) and matched where no escaping was needed (CJK) — the regression is exactly bounded. Witness:04-estimator-oracle-exact.png. - Transactional ownership: rollback half pinned (M3b kills 9, +2 vs last round); commit half = Finding 1.
Mutation matrix (re-run live at the new head)
Witness: 05-mutation-matrix-at-new-head.png. Raw logs logs/cap-*.log, logs/mut-*.log.
| Mutant | Change | Suite | Result | First failing assertion |
|---|---|---|---|---|
| M1 | registry: disable per-connection + global refusal | connection-registry + pre-attach-budget | KILLED — 5 red / 56 green | bounds retained serialized payload bytes before stream attachment (+4 budget tests) |
| M2 | spawnChannel: revert estimator to base formula | acp-bridge focused | KILLED — 1 red / 46 green | charges JSON string escaping before admitting prepared responses (expected [Function] to throw an error) |
| M3 | dispatch: remove canCommitSessionOwnership gate in delivered() |
transport + dispatch trio | SURVIVED — 0 red / 311+55 green | — |
| M3b | dispatch: disable rollback-on-discard (positive control) | transport | KILLED — 9 red / 302 green | rolls back an undelivered fresh session when the connection closes (+8) |
| M4 | dispatch: delete failed-delivery error-frame retry | transport | KILLED — 2 red / 309 green | the two session/load … cannot be serialized tests |
| M5 | ws-stream: flatten outcome_unknown → delivered |
ws-stream | KILLED — 2 red / 18 green | close() marks an active accepted send outcome unknown (+1) |
M3b/M4/M5 prove the harness can make every relevant suite red through the receipt/ambiguity mechanisms, so M3's survival is a coverage statement, not harness failure. All mutants reverted after their runs; git diff -- packages/ clean afterwards.
Findings
- Suggestion (carried forward, stands) — ownership commit gate unpinned. Reproduce: apply
mut-apply.py m3(removes thecanCommitSessionOwnershipconsultation fromownershipReceipt.delivered()), thencd packages/cli && npx vitest run src/serve/acp-http/transport.test.ts src/serve/acp-http/dispatch-error.test.ts src/serve/acp-http/dispatch.sandbox.test.ts src/serve/acp-http/workspace-qualified-acp.test.ts→ 366/366 green;mut-apply.py m3bon the same transport file → 9 red. Removing the gate leaves all 311 transport + 55 dispatch tests green, while the symmetric rollback path is pinned by 9 tests. The gate is live, not dead:closingSessions.add()runs in thesession/closehandler andcloseSessionStreambumps the generation, so a reply whose write settles after a close/destroy race is exactly the interleaving the gate decides — and the registry-level predicate test passes unchanged under M3 because M3 removes the caller, not the predicate. A fixture that would pin it:session/newreply in flight on a live connection stream,session/closefor that id landing before the write settles, then assert the session is not re-owned. Not merge-blocking — the guard exists and is correct as written; completeness reporting.
Not covered
- Real-daemon E2E (committed plan: 128 × 1 MiB distinct-payload RSS, dynamic-workspace mounts, live
GET /daemon/status?detail=full): not executed — the PR states it was not run locally either. The plan file is again absent from this worktree (root-owned.qwen/prevents restore; read viagit show HEAD:— environmental, same as last round). The A/B above proves the same mechanisms at registry/stream level over real HTTP/SSE/WS; that is the handling, not the end-to-end RSS trigger. - Per-commit attribution for the six delta commits: the depth-2 merge-ref checkout makes only the aggregate reachable; the delta was scoped by commit messages plus code inspection (base-side diffs of
ws-stream.ts,sse-stream.ts,dispatch.ts), and each named fix was driven through a harness. - Dispatch-level A/B against base for ownership transactionality (needs the full bridge runtime); covered by C7 + gates + M3/M3b/M4.
- Ordinary live SSE/WS new-frame queues and transient single-frame
JSON.stringifyamplification (PR-declared non-goals); Windows; repo-wide lint/typecheck (PR's own CI ran them; the base-controlnpm run buildand HEAD builds ran clean here as part of A/B setup).
Methodology
Environment: node:22-bookworm CI container, Node v22.23.2, tree at refs/pull/9007/merge (depth 2), npm ci + npm run build pre-run at HEAD. A/B harness (ab-pre-attach.mjs) imports each arm's compiled packages/cli/dist/src/serve/acp-http/*.js (closure node-builtin-only, re-verified); base control built in a scratch worktree at HEAD^1 (npm run build -w packages/acp-bridge -w packages/cli after wiring root + per-package node_modules symlinks and regenerating the gitignored git-commit.ts; BUILD_EXIT=0) and removed after capture; base arm's internal-dependency realpath asserted. WS probe (ws-ambiguity.mjs) uses the real ws package with compression disabled and a paused receiver to keep writes genuinely in flight. Estimator oracle (estimator-oracle.mjs) compares against JSON.stringify bytes directly and replays the base formula mechanically. Gates ran via npx vitest run <exact files> per package. Mutations applied by mut-apply.py with backups in mut-backup/, each restored and verified by empty git diff; the full matrix was re-executed under scripts/verify-capture.mjs (mutation-runner.sh). Raw per-cell logs in logs/.
Evidence images
1 additional image(s) did not pass the hosting checks (PNG magic, unique sanitized name, ≤2 MB, max 8) and remain in the run artifacts.
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Thanks for the PR — re-run after the template fix and the eight hardening rounds.
Moving on to code review. 🔍 中文说明感谢贡献——这是模板修复与八轮加固后的重跑。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed the full diff (336 lines) at
TestingEvidence carried here: the PR's own CI checks at the reviewed commit, read via the API — per triage rules nothing in this PR was built or executed by the reviewer. The author's stated local counts (8/8 CI status at the reviewed commit (unit suite still running — table updates automatically when CI settles):
Not verified: the emergent reviewer behavior this PR exists for — that a round-1 review of an enumeration-trap-shaped PR actually files one class-level finding instead of enumerating corners, and that later rounds collapse recurring siblings. The tests pin the prompt text and invariants, which is the right floor for a prompt change, but they pass even if the model never follows the guidance. Sandboxed verification would settle the round-1 half: Not verified (by design): nothing user-visible or TUI changed, so no before/after capture applies. 中文说明代码审查在
测试本节携带的证据：被审提交上 PR 自己的 CI 检查结果，经 API 读取——按 triage 规则，评审者未构建或执行本 PR 的任何代码。作者自报的本地数字（ （CI 状态表见上方英文部分；套件落定后由 finalize 工作流自动更新。） 未验证：本 PR 真正要改变的涌现评审行为——第 1 轮评审面对枚举陷阱形状的 PR 时是否真的只报一条 class 级 finding 而非逐角枚举、后续轮次是否收敛复发 sibling。测试钉住的是 prompt 文本与不变量——对 prompt 改动这是正确的下限——但即便模型从不遵循指引，这些测试也会通过。沙箱验证可 settling 其中第 1 轮的一半： （按设计）未验证：无用户可见或 TUI 变化，不适用 before/after 捕获。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean static review across every stage; the one reservation is inherent to prompt changes: whether reviewers actually close classes in round 1 is emergent behavior the pinned tests can't prove, only future My independent proposal for this problem — a round-1 shape lens on the altitude agent plus a bounded/unbounded fork in the cross-round sibling rule — is exactly what this PR does, and it goes further where it should: the lens is delivered to the 3B chunk path (where the large-PR bloat actually lives), and the collapse has a ledger disposition that survives round recovery. I didn't find a simpler path it missed. The eight review rounds did real work here, and it shows: the witness requirement, strongest-evidence preservation, and the unreachable-chunk early return each answer a concrete hole found in the mechanism, not decorative hardening. What's left is dense prose, but it is dense in the way this skill's files already are, with a pinned test documenting why each clause exists. If I'm maintaining this in six months, the pins and the DESIGN.md lesson tell me why every sentence is there — that's the thank-them version. The remaining CHANGES_REQUESTED reviews on the PR are from earlier Verdict: approve. The unit suite is still in flight on the merge head, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 各阶段静态审查均干净；唯一保留意见是 prompt 改动的固有属性：评审者是否真的会在第 1 轮关类，是钉字符串测试无法证明的涌现行为，只有后续真实的 我对这个问题的独立方案——高度 agent 上的第 1 轮形状 lens，加跨轮 sibling 规则里的有界/无界分叉——正是本 PR 所做的，而且它在应该走得更远的地方走得更远：lens 送达 3B 分块路径（大 PR 膨胀真正发生处），收敛有能跨轮恢复存活的 ledger 处置。我没有找到它遗漏的更简路径。 八轮评审做了实打实的工作，且清晰可见：见证要求、最强证据保留、不可达 chunk 提前返回，各自回应机制中被发现的具体漏洞，而非装饰性加固。剩下的文本很密，但与本 skill 文件一贯的密度一致，且每条子句都有钉测试说明其存在理由。六个月后维护它时，钉与 DESIGN.md 的经验会告诉我每句话为何在那里——这是值得感谢作者的版本。 PR 上现存的 CHANGES_REQUESTED 评审来自更早的 裁决：通过。merge head 上的单元套件仍在运行，因此批准推迟到 CI 在该提交上全绿——finalize 步骤会在所有检查绿完后发布钉住该提交的批准；若有检查变红或 head 移动则不发布。 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved as requested.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 4": nothing — I did not rebuild the bundle to measure exact byte delta. I'll disclose this as the only check I did not run, but it's not a budget gap per se, it's a…; "agent 5": could not execute the test suite — the review worktree has no node_modules and vitest fails resolving ajv/dist/2020.js from packages/core/src/utils/schemaV….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
未探索到全部深度（达到工具调用预算）："agent 4"：nothing — I did not rebuild the bundle to measure exact byte delta. I'll disclose this as the only check I did not run, but it's not a budget gap per se, it's a…；"agent 5"：could not execute the test suite — the review worktree has no node_modules and vitest fails resolving ajv/dist/2020.js from packages/core/src/utils/schemaV…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| After verification, identify **confirmed** findings that describe the **same type of problem** across different locations (e.g., "missing error handling" appearing in 8 places). Only group findings with the **same confidence level** together — do not mix high-confidence and low-confidence findings in the same pattern group. | ||
|
|
||
| ### Pattern aggregation | ||
| **A root-cause family is one class-level finding, NOT a pattern-aggregation.** When several confirmed findings are different symptoms of ONE structural root cause — six XML-corner bypasses whose root is a hand-rolled parser, many call sites broken by one wrong contract — do **not** run them through the pattern merge above: that promotes the group to its highest severity, expands into one posted comment per location, and so recovers as N separate ids next round — the enumeration this exists to end, rebuilt. Instead file **one** finding, with a **single** anchor at the root and the symptoms cited as evidence in its body: its severity is the demonstrated risk of the **root** (not the highest symptom), at the **root's own confidence** (so a low-confidence symptom cannot promote the whole aggregate onto the PR). This is the within-round face of the unbounded-family rule in Step 6 — the same single class-level finding — so **decide it on the final union after the reverse audit** (Step 5), not only here, or a reverse-audit sibling of the same root posts separately. |
There was a problem hiding this comment.
[Critical] R2-1: Step 4 deduplicates same-issue/same-root candidates before verification and keeps the highest carried severity, but this later class-collapse rule requires the root's own risk and confidence. Once the earlier merge has erased the members, the final union has no defined way to recover those values. A low-confidence Critical symptom can therefore promote a Suggestion-level root into a false blocker. Preserve root-family members until the final collapse, or narrow the earlier dedup so it cannot erase root severity/confidence.
中文说明
[Critical] R2-1：Step 4 在验证前就按同一 issue/root cause 去重，并保留最高严重度；但这里随后又要求 class finding 使用 root 自身的风险与置信度。前一阶段一旦把成员折叠掉，final union 就没有机制恢复这些信息。于是一个低置信度的 Critical symptom 可能把本应为 Suggestion 的 root 提升成误阻塞。请保留 root-family 成员直到最终 collapse，或收窄前置 dedup，避免它抹掉 root 的 severity/confidence。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // (3A) and injected into the chunk brief (3B) by buildChunkAgentPrompt, each | ||
| // under its own scope framing. Scope-neutral body; the wrapping text supplies | ||
| // "for the whole change" vs "for your territory". | ||
| export const ENUMERATION_TRAP_LENS = `A change that HAND-ROLLS parsing or matching of an **unbounded or adversarial surface** — untrusted input, a rendered format (what a viewer/renderer displays), or a spec/grammar — with per-corner special-cases that accumulate (\`indexOf\`/\`slice\`/regex over structured input, "match what the renderer renders" logic, a re-implemented grammar, a growing hand-listed case set) has **no last corner**, so enumerating cases never converges. The finding is the SHAPE, not the current corner: name the class-closing fix — defer to a real parser, the tool's own authoritative structured output, or a fail-closed decision — and file it ONCE, in place of enumerating cases. Severity follows the risk the shape carries — a hand-rolled parser that can be fooled into a wrong result is **Critical**.`; |
There was a problem hiding this comment.
[Critical] R2-2: This lens equates an adversarial/untrusted surface with having no last corner, while Step 6 defines unboundedness by whether the entrances can actually be enumerated and closed. A finite closed set of untrusted tokens is bounded under Step 6 but unbounded here, so the same review receives incompatible instructions and can suppress distinct bounded defects or manufacture a class-level Critical. Limit this lens to surfaces whose entrances cannot be enumerated; adversarial input alone must not imply unboundedness.
中文说明
[Critical] R2-2：这里把 adversarial/untrusted surface 等同于“没有最后一个 corner”，而 Step 6 按入口是否能被枚举并逐一关闭来定义 unbounded。一个有限、封闭的不可信 token 集在 Step 6 中是 bounded，在这里却会被视为 unbounded，导致同一评审收到冲突指令，可能漏报有限家族中的独立缺陷，或制造不合理的 class-level Critical。请把 lens 限定为入口无法穷举的 surface，并明确 adversarial input 本身不等于 unbounded。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| '', | ||
| '**Shape check (part of code quality — the altitude lens, scoped to your ' + | ||
| 'territory).** For the code in YOUR chunk: ' + | ||
| ENUMERATION_TRAP_LENS, |
There was a problem hiding this comment.
[Critical] R2-3: On Step 3B this lens is delivered only as For the code in YOUR chunk, while chunk agents are explicitly blind to cross-file tracing and there is no whole-diff Agent 3b/equivalent shape owner. If parser primitives are in one chunk and the accumulating compensating cases are in others, no agent sees the structural family as a whole; medium skips reverse audit and high's reverse audit is also per-chunk. Retain a whole-diff 3b shape pass for 3B, or add a cross-chunk shape aggregator over the relevant hunks/findings.
中文说明
[Critical] R2-3：Step 3B 只把该 lens 以 For the code in YOUR chunk 的方式交给分块 agent，而 chunk agent 明确不负责 cross-file tracing，也没有 whole-diff Agent 3b 或等价的 shape owner。如果 parser primitive 在一个 chunk、不断累积的补丁/特殊 case 在其他 chunk，就没有 agent 能看到完整 structural family；medium 跳过 reverse audit，high 的 reverse audit 也按 chunk 运行。请为 3B 保留 whole-diff 3b shape pass，或增加跨 chunk 的 shape 聚合器。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. | ||
| - **superseded by `<class-id>`** — the entry is a member of a family that collapsed into one class-level finding (the bounded/unbounded rule below). Record `superseded by <class-id>` in the status table; do **not** re-report it and do **not** count it toward `cannotTellCriticals` — the open class finding is the single blocker that carries the family, so the block is preserved without re-enumerating. This is the disposition for a prior sibling that resurfaces in the re-check below after the collapse: it is neither `still stands` (which would re-enumerate and re-carry its id) nor `fixed` (its own mechanism is not closed until the structural change lands) nor `cannot tell` (which would cap the verdict every round until then). | ||
|
|
||
| **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the <X> surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by <class-id>` (the disposition above), fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round's **ledger marker** recovers one entry, not N, and a prior sibling that resurfaces on the PR as its own thread is ruled `superseded`, not re-posted. **A brand-new sibling found in the current round** — by a Step 3 finder or Step 5 auditor over the incremental diff, while the class finding is already on the ledger and open — folds the same way: into the class finding's re-report as evidence under the class id at Step 6 rendering, never filed under its own id. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) |
There was a problem hiding this comment.
[Critical] R2-4: This paragraph folds later sibling evidence into the stable class id and recomputes severity from the demonstrated shape risk, but the adjacent still stands rule requires the original id to keep its severity. A class first filed as Suggestion can therefore remain non-blocking after a later sibling proves wrong results. Keep the stable id, but explicitly allow severity reclassification when newly verified root evidence changes the demonstrated risk.
中文说明
[Critical] R2-4：本段要求把后续 sibling evidence 折入稳定 class id，并按已证明的 shape risk 重新确定严重度；但相邻的 still stands 规则又要求原 id 保持原严重度。因此一个最初按 Suggestion 上报的 class，即使后续 sibling 已证明会产生错误结果，也可能继续不阻塞。请保留稳定 id，但明确允许在新的已验证 root evidence 改变风险时重新分类 severity。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| '', | ||
| '**Shape check (part of code quality — the altitude lens, scoped to your ' + | ||
| 'territory).** For the code in YOUR chunk: ' + | ||
| ENUMERATION_TRAP_LENS, |
There was a problem hiding this comment.
[Suggestion] R2-6: This shape-check mission is emitted even for an unreachable chunk, after that prompt has required an exact Uncoverable: receipt and stop. The adjacent modeled-system lens and tool-budget block are both gated out for this same two-masters reason. Gate this block on !unreachable as well, otherwise following it can turn a disclosed gap into a hard coverage failure.
中文说明
[Suggestion] R2-6：即使 chunk 不可读取，这个 shape-check mission 仍会在要求“只返回精确 Uncoverable: receipt 并停止”之后被注入。相邻的 modeled-system lens 和 tool-budget block 都因为同样的 two-masters 问题而排除了 unreachable chunk。这里也应加 !unreachable gate，否则 agent 遵循该任务可能把已披露的 gap 变成 coverage hard failure。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(body).toContain('apply the bounded/unbounded rule above instead'); | ||
| // A resurfaced sibling of a collapsed family has its own disposition, so the | ||
| // re-check does not fall to still-stands / cannot-tell every round (R3-6). | ||
| expect(body).toContain('superseded by `<class-id>`'); |
There was a problem hiding this comment.
[Suggestion] R2-7: This assertion pins the disposition label but not its load-bearing do **not** count it toward cannotTellCriticals clause. A probe removed that clause and this suite still passed 10/10, so a later cleanup can restore the every-round COMMENT cap without a regression signal. Add a separate pin for the cannotTellCriticals exclusion.
中文说明
[Suggestion] R2-7：这个断言只钉住 disposition 名称，没有钉住关键的 do **not** count it toward cannotTellCriticals 条款。探针删除该条款后，本套件仍 10/10 通过，因此后续 cleanup 可以在无回归信号的情况下恢复“每轮都被 COMMENT cap”的问题。请为 cannotTellCriticals 排除条款增加独立 pin。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ); | ||
| // The rule must govern BOTH sibling paths: the open-blocker re-check routes an | ||
| // unbounded family to the collapse rule instead of enumerating (R3-1/R3-5). | ||
| expect(body).toContain('apply the bounded/unbounded rule above instead'); |
There was a problem hiding this comment.
[Suggestion] R2-8: The test pins only the open-blocker routing text. A probe removed the ledger fixed path's apply the bounded/unbounded rule below instead of filing the sibling clause and this suite still passed 10/10. Pin that second path too, otherwise one of the two paths this rule claims to unify can regress independently.
中文说明
[Suggestion] R2-8：测试只钉住了 open-blocker 路由文本。探针删除 ledger fixed 路径中的 apply the bounded/unbounded rule below instead of filing the sibling 后，本套件仍 10/10 通过。请同时 pin 第二条路径，否则该规则声称统一的两条路径中仍有一条可以独立回归。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| | `1c` | **Cross-file tracer** (needs a local tree). Owns the whole cross-file walk. _Consumer direction_: grep every caller of every changed export and check it against the new contract. _Producer direction_: for every field the diff **adds**, grep its **read sites** — a live path reading a field the diff never populates is Critical, and nothing in the build will tell you. | | ||
| | `2` | **Security.** Injection, XSS, SSRF, path traversal, authn/authz bypass, secrets in logs, weak crypto, hardcoded credentials. Includes **option/argument injection into subprocess calls** — a user-controlled positional that starts with `-` or is `.`/`..` becomes a git/gh flag or pathspec (`--output=`, `-f`, `checkout .`); `execFile` does not stop it — validate the value against the subcommand grammar (a ref/name allowlist, reject a leading `-`); a `--` separator ends option parsing but does **not** neutralize a pathspec (`checkout -- .` still discards changes), so the value allowlist is the fix. | | ||
| | `3a` | **Reuse & duplication.** Does the codebase already have this? Greps the shared/utility modules and adjacent files for the _behaviour_ (a literal, an error string, a regex — not a plausible function name), and **names the existing helper to call instead**; a duplication finding that names nothing is not a finding. Also owns **dead code the diff leaves behind**. | | ||
| | `3b` | **Altitude & abstraction fit.** Is each change at the right depth — or a bandaid on shared infrastructure, a downstream compensation for an upstream bug, or a new abstraction serving a single call site? **Names the depth the change should live at**, and the blast radius on the other callers. Also flags the **enumeration trap** — a change that hand-rolls an unbounded/adversarial surface (parsing untrusted input, matching a rendered format, a grammar) instead of deferring to a real parser / authoritative output / a fail-closed decision is a class-closing finding, named once, not enumerated case-by-case. | |
There was a problem hiding this comment.
[Suggestion] R2-9: The test-efficacy probe reverted only this new Agent 3b role-table clause and all affected tests stayed green. The other behavioral SKILL.md hunks have pins, but this responsibility summary can silently disappear. Add a focused SKILL.test.ts assertion for the enumeration-trap clause in the 3b row.
中文说明
[Suggestion] R2-9：test-efficacy probe 只回退了 Agent 3b role-table 的新增条款，所有相关测试仍保持绿色。其他行为性 SKILL.md hunk 都有 pin，但这里的职责摘要可以静默消失。请在 SKILL.test.ts 中为 3b 行的 enumeration-trap 条款增加聚焦断言。
— qwen3.8-max via Qwen Code /review (v0.21.11)
|
Released in v0.21.12. |
…arsing prose Every genuine Critical in review rounds 5-8 lived in one place: the deferral channel was free text re-parsed for provenance it did not carry. A separator regex classified deterministic source, a marker regex caught mis-routed Criticals, and each round's probe found the spelling the last fix excluded — kebab paths, the SKILL's own aggregate suffix, an en dash, a title-borne [test], (Critical), a fullwidth colon. A whole-module self-audit (with reproduced witnesses) found eight more shapes and named the class: the fourth, fifth and sixth rounds of the same enumeration trap this repo's own review doctrine (#9095) says to close structurally. So the entry is typed. `deferredSuggestions` is an array of `{file, line?, source, severity, title, locations?}` copied from the findings artifact the model already wrote in Step 6: deterministic derives from `source` (build/test/probe), relocation from `severity === Critical` (counts toward C, blocks, rides the ledger, classified by its FIELD — a title mentioning [test] no longer exempts an unverified claim), a `Nice to have` or malformed or free-text entry is refused at the boundary like a NaN count (the channel that un-posts findings is not guessed at), and the human line `file:line — [source] title (+N locations)` is RENDERED by compose-review — nothing downstream parses it back. Both regexes and the split helper's string grammar are gone; the SKILL contract, the state bullet, the submit seam test and the whole deferral describe block are rewritten for the typed shape (no test probes a spelling any more). The self-audit's side-file findings land in the same commit: the recovered write never lowers the round (a stale walked list — a concurrent lane, or a paginated fetch that came back short — overwrote round 7 with round 2 and dropped the anchor sha; compare on round, reviewId as tiebreak), and login comparison is case-insensitive per GitHub (a case mismatch read "own review exists" as proven absence and deleted the counter). Both pinned. This pulls the structural half of #9176 into the PR; the issue keeps its remaining bullets (structured `deferred` artifact marker, age-evidence arm, R/D-id timing).
…ings (QwenLM#9118) * feat(review): adopt a round-aware convergence posture for posted findings Re-reviews of the same PR regenerate non-Critical feedback at zero marginal cost after every push: the review files findings on code the previous round just added, the next push implements them, and the diff widens — which allocates more agents, which file more findings. Measured from the outside: one managed PR reached +13k lines across 8 review rounds with its per-round Critical count flat and was closed unmerged; on two others 78-86% of the growth was test lines. Every existing brake lives in the calling workflow and covers only bot-managed PRs; a human contributor answering a push-triggered review round by round rides the same loop with no brake at all. Convergence therefore has to be a property of /review itself, not of any orchestration around it. Teach the reviewer to raise its POSTING bar as rounds accumulate — finding, verification, the terminal report and the findings artifact are untouched: - The convergence posture (Step 6): with no flag, Suggestions post through round 5; from round 6 only Criticals post, and non-Critical findings are recorded, not requested — one line each into the new `deferredSuggestions` compose state, which compose-review renders as a disclosed, non-capping list on every event (nothing is silently dropped, and a deferral never withholds the incremental anchor). A Critical is never deferred, at any round under any floor. An APPROVE over a non-empty deferral list opens "No blocking issues" instead of "No issues found", and the round number in the clause comes from the same side-file read the ledger marker stamps, so the two cannot disagree. - The code-age rule (rounds 2-5): a NEW non-Critical finding anchored on code unchanged since the previous round's reviewed head defers the same way — that code was read last round and not flagged, so filing a nit on it now is re-derivation churn. The age reference is the previous review's own commit_id, which pr-context now recovers into the prev-ledger side file: unlike the ledger sha (a range certification, withheld on fail-closed rounds on purpose) it exists on every posted round, so the fail-closed full-range re-review — the common case in a bot loop — can still apply the rule. Validation and the newly-reachable exception mirror the anchor check; an absent or invalid reference skips the rule, never the review. - `--severity-floor <critical|suggestion>` and `review.severityFloor` make the posture an explicit knob in both directions: `critical` applies it from round 1, `suggestion` turns it off, and `auto` — the default — is the round-adaptive rule. Grammar, deferred warnings, non-PR gating and operator-scope resolution all mirror `--effort`/`--comment`. * chore(review): regenerate settings.schema.json for review.severityFloor * fix(review): close the round-1 review findings on the convergence posture Five verified findings from the PR's own reviews, each with its mechanism: - The verdict surface now carries the deferrals. A deferrals-only APPROVE set lowSignal, so verdictLine printed "none of the N review agents reported a finding" while the same body listed the findings two paragraphs down — on exactly the posture's canonical end state. lowSignal is skipped when deferrals exist, ComposeReviewResult gains deferredCount, and the verdict line names the deferrals instead. - Deferred findings count toward the verifier-delivery floor. They publish in the body as the deferral list, and an unverified claim does not become publishable by being deferred — a deferrals-only run owes a verifier exactly as a posting run does. - The deferrable set is narrowed to high-confidence Suggestions that would otherwise post. Low-confidence and Nice-to-have findings stay terminal-only: routing them through the deferral list would publish what the review contract keeps off the PR. - The code-age rule is hardened on both operands and both premises: the documented command quotes the PR-controlled path and passes --literal-pathspecs (an unquoted name executes shell; a glob name ages the finding against a sibling file's hunks), and age suppression is skipped for findings in scope the previous round disclosed as not reviewed — "the previous round saw this code" is false there. - One side-file read per compose: the deferral clause and the ledger marker take the same prevRound value, so a mid-compose update can no longer publish two different round numbers in one review. The deferral list also caps each entry at 240 chars (twenty 4,000-char entries would push the body past GitHub's 65,536 rejection line), and the disclosed 20×240 cap is now stated in the prose the list's survival promise lives in. Smaller closures from the same rounds: two invalid flag values are two typos, not a target and a tiebreak (neither becomes a file target); the --severity-floor warning arms invalid-eq and kept-as-target gain the tests whose absence a mutation probe demonstrated; the commitId fixture gains a newer marker-less review so a latest-review-wins mutant fails; the severityFloor setting joins the settings-dialog membership assertion and the configured-floor wiring tests; and the /review argument-hint now advertises the flag. * fix(review): carry deferredCount through the persisted verdict The save-artifact validator constructs the persisted verdict from the composed JSON and gained the field with absent-means-zero semantics: a composed file written by a build predating the posture must not fail a save over a count that only affects display, while a present value of the wrong shape is refused like every other field. * test(review): pin deferredCount passthrough and the pre-posture default in the saved artifact * fix(review): close the round-2 findings — deterministic deferrals and the truthful verdict line - The verifier-delivery floor now excludes deterministic [build]/[test]/ [probe] deferrals by their source tag, the same exclusion body Criticals get: a pre-confirmed finding never produces a verifier delivery, so counting it demanded a delivery that cannot exist — the cap never lifted, the anchor was withheld every round, and the posture's own enforcement regenerated the full-range re-review loop it exists to end. The deferral entry format now carries the source tag (SKILL prose + a pin), and the determinism regex is one shared constant for both scans. - verdictLine's "(listed in the body)" turns cap-aware past the 20-line render cap, so a verdict counting 21 no longer certifies a body listing 20; the line caps are module-scoped for the two readers. - The mutation-demonstrated assertion gaps are closed: deferredCount is asserted on the REQUEST_CHANGES and COMMENT return sites, and save-artifact's refuse arm has its wrong-shape cases ('two', -1, 1.5). * fix(review): close the round-3 findings — source-position tags, a Critical tripwire, and honest age-reference lifecycle - Deterministic classification of a deferral reads the SOURCE position only (tag immediately after the entry's first em-dash): a whole-entry scan classified a finding deterministic off a title that mentioned [test] and skipped the verifier floor for an unverified claim (probe-confirmed). - A deferral carrying a Critical marker is refused outright: the channel is model-written free text that casts no vote on C, and a Critical routed there composed an APPROVE over a blocker in a probe. Marker forms only, so prose like "critical-path" passes; artifact-level reconciliation stays a structural follow-up. - The prev-ledger side file now carries the winning review's own id, and a run that recovers no ledger strips a stale file's commitId/reviewId while keeping the round counter: an age reference the PR's current reviews no longer vouch for can wrongly defer a finding on code changed-and-reverted since the true previous round — snapshot diffs are not monotonic over intervals, which refutes the earlier keep-it-it's-conservative ruling. The write/strip logic is an extracted, filesystem-tested helper, closing the serialization blind spot two rounds of review asked about. - Prose repairs: the code-age rule is auto-only (an explicit suggestion floor turns it off, as the resolve-floor paragraph already promised); the fallback names the commitId, not the ledger sha; the not-reviewed check binds to the review the side file's reviewId names; and the context-unavailable state skips the age rule (the side file may be a previous run's). save-artifact's null-deferredCount reads as zero — the same absence semantics compose-review's toCount gives the field's siblings — now stated and tested rather than accidental. * fix(review): close the round-4 findings — licence the deferral channel, harden the side file, age aggregates per location Code: - The deferral channel gains its licence check: the resolved severityFloor rides the compose state, and a non-empty deferral under an explicit suggestion floor (posture off) or on round 1 of auto (no posture, no age reference) is refused — the one channel that removes findings from posting now gets the same deterministic treatment as the counts. Rounds 2-5 under auto stay licensed: the code-age rule defers there, which the reviewer's proposed round<6 condition would have wrongly outlawed. - Ledger recovery skips PENDING drafts (the API serves the caller's own unsubmitted reviews; a crashed run's draft is not a previous round), and the side file's lifecycle is three-way honest: recovered → written whole (atomically, temp+rename — a mid-write failure must never leave a truncated file that parses as no round); reviews read but no ledger for this account → file REMOVED (another account's round counter must not stamp this account's first review round N+1); recovery threw → round counter kept, age-sensitive commitId/reviewId stripped. - Disposal rule v3: an invalid flag value survives by what it could BE — a PR-shaped token survives unless a typed target exists (an unrelated typo must not change WHICH codebase is reviewed), a file-shaped token only as the sole kept token. - The per-entry cap backs off a split surrogate pair (zh titles ride the deferral list untranslated; unit 240 on a high surrogate shipped U+FFFD). Prose (SKILL): - Pattern aggregates age per location: ANY changed location posts the aggregate; deferral only when every location is unchanged and covered. - The reviewId body is consulted only after fetching a truncated tail (the 8,000-char render cap can hide the very "Not reviewed" disclosure the age rule depends on); an unreadable body skips the rule. - context-unavailable resolves the auto floor as round 1: no posture, full posting, said in the terminal — a posting bar in doubt fails open. - Deferred findings number under D<round>-<n>, never consuming an R id the ledger's buildLedger would reassign to a posted sibling. - The Step 8 record sentence now states exactly what survives where, and the state contract documents severityFloor. Tests pin all of it, including the mutation-shown gaps: the marker/clause round agreement, the exactly-20 verdict-line boundary, the submit-seam deferral passthrough, and sha survival through the side-file rewrites. Structural remainders are filed instead of grown: QwenLM#9176 (typed deferral channel derived from the findings artifact), QwenLM#9177 (whole-body byte budget). * fix(review): close the round-5 findings — carry the floor unresolved, cap unlicensed deferrals, guard the counter The round-5 reviews caught a shipped design contradiction: the SKILL told Step 6 to carry the RESOLVED floor into the compose state, so a legal rounds-2-5 age-rule deferral arrived as the string 'suggestion' — which the licence check reads as the operator's explicit posture-off override — and the compose threw, losing the entire round, Criticals included. Two fixes, one per side of the contract: - The state carries the verdict's floor UNRESOLVED: auto stays the literal 'auto', and the module licenses it by the round it derives itself. A SKILL pin makes the sentence load-bearing, and an end-to-end test pins the legal round-3 shape (side file at round 2, auto floor, deferral → clean APPROVE naming round 3). - Unlicensed deferrals CAP instead of throwing: prevRound is a best-effort side-file read whose every failure mode returns 0, so a missing file at a true round 6 must degrade to a disclosed, capped, anchor-withheld verdict — never to no verdict at all. The findings render under a warning clause; the Critical-marker tripwire stays a refusal (a Critical rendered as "recorded, not requested" would be the worse outcome). Adjacent round-5 closures: the tripwire is separator-agnostic (an ASCII hyphen where the format prescribes an em dash was the cheapest real miss); side-file removal is gated on a POSITIVELY read non-empty reviews list (ghApiAll flattens error envelopes to [], and an empty list must not delete a live round counter and its anchor); the atomic temp name is per-process so concurrent same-PR fetches cannot rename each other's bytes, with debris unlinked on a failed rename; two PR-shaped invalid flag values are refused as ambiguous instead of first-wins; the apostrophe escaping clause and the unresolved-carry sentence gain SKILL pins; and review.severityFloor joins the user-facing settings reference. Declined, recorded on the PR: requiring per-file coverage evidence for the age rule — the anchor chain already encodes it (an incremental round's anchor certifies the preceding full coverage; broken links are fail-closed rounds whose disclosures the rule consults). * fix(review): close the round-6 Criticals — relocate stray Criticals, license by evidence at hand, one separator grammar Round 6 tripled the finding count and aimed almost entirely at rounds 4-5's hardening code — the expansion signal this PR's own posture exists to answer — so this round lands only the confirmed Criticals and defers the Suggestion tail on the record: - A Critical-marked deferral is RELOCATED into the body Criticals instead of thrown: it counts toward C, the event blocks, and the round posts — the same doctrine the round-5 fix applied to the licence check, closing the last channel where one bad entry could cost a composed round (with real drafted Criticals attached). A lookbehind spares hyphenated compounds — the SKILL's own "non-Critical findings" phrasing was a realistic false positive that would have blocked over a nit. - The two deferral regexes share one separator grammar: the deterministic classifier now accepts hyphen/en/em like the tripwire, so two spellings of one [test] finding no longer produce opposite verdicts (the unmatched form demanded a verifier that cannot exist — the self-inflicted cap). - The licence closes its evidence gaps: an ABSENT severityFloor beside a non-empty deferral list is unlicensed (the field ships with the channel; omission must not silently re-license what an explicit suggestion floor forbade), auto in the context-unavailable state is unlicensed (the round is unknowable), and the unlicensed cap now joins the certification ladder so "Reviewed — no blockers." cannot open a body whose own warning says findings may be under-posted. - Deferred entries render Markdown-neutralized (mdField, the budget-gap rule — model text reaching a public body), carry a truncation ellipsis when the per-entry cap cut them, and --severity-floor accepts the documented `auto` spelling while quoted-empty flag values are consumed as missing instead of becoming empty file targets. - Prose: the age rule's deferrable set names the same high-confidence otherwise-postable Suggestions as the floor paragraph (never low-confidence/NTH); a reviewId body absent because it matched the canonical LGTM filter is disclosure-free by definition, not unreadable; and the posture round is the side file's — the cache scopes the diff but never decides the posture. The Suggestion tail and the two structural families are recorded, not grown: QwenLM#9176 gains the age-evidence and id-timing bullets; QwenLM#9177 already carries the whole-body budget. Declines stand where argued (R2-2/R2-15 anchor-chain; sole-file-shaped promotion keeps the forgot-the-level use). * fix(review): close the round-7 findings — kebab paths, the equals-form ambiguity hole, and the relocated blocker's ledger seat Round 7 caught a regression the round-6 fix itself introduced, plus one genuine hole in the round-5/6 ambiguity guard: - The deterministic classifier anchors on the first WHITESPACE-FLANKED separator after the leading file:line token. The round-6 negated-class walk stopped at the first hyphen INSIDE a kebab-case path — this repo's enforced .ts convention — so the common spelling of a [test] deferral was misclassified non-deterministic and demanded a verifier that cannot exist: the permanent self-cap loop, on exactly the entries the exclusion exists for. Kebab, [build] and [probe] cases pin all three tags on both separators. - The PR-target ambiguity pool counts BOTH flag spellings: an equals-form invalid value never enters the disposal set, so which of two PR numbers got reviewed depended on which syntax was typed. All four spelling combinations now land on the same loud refusal, pinned. - A relocated Critical rides the machine ledger: the split moved into a shared helper that the body composer and the marker builder both call, so a mis-routed blocker keeps its id continuity ("the findings always ride" includes the mis-routed ones). - The age rule's two diff-output doubt states fail open like every other arm: a non-matching pathspec (prove it with tree-relative cat-file before reading the diff's silence) is about the path, not the code, and a zero-hunk non-empty diff — a PR-controlled .gitattributes binary mark — is a file-level change; both post. Commands pinned to the worktree root. - The mutation-shown pin and test gaps are closed: explicit auto override, quoted-empty consumption on both flags, the invalid-configured-floor handler seam, the SKILL's validation commands, round-source and context-unavailable clauses, and the aggregate age clause. The four re-asserted body Criticals remain tracked (QwenLM#9176/QwenLM#9177 and the two serialization-ordering threads) — their standing disposition is the round-6 batch record, and their final resolution is the maintainer's merge decision. * fix(review): close the round-8 Criticals — one entry grammar, one rescue rule, deletion only on proven absence Round 8 landed three genuine Criticals, all on rounds 6-7's hardening code, plus a fourth from a local lane sharing the first's mechanism: - The deterministic classifier's entry grammar tolerates the shapes the SKILL itself prescribes: the aggregate `(+N locations)` suffix between the anchor and the separator, a leading space (entries are trimmed before the scan; the filter trimmed only for emptiness), and an en dash. Every one classified a pre-confirmed [test] deferral non-deterministic and demanded a verifier that cannot exist — the permanent self-cap on the posture's own stop signal, probe-demonstrated three ways. - A relocated Critical is classified by that same position-anchored rule, not the whole-entry tag scan the model's own body Criticals get: a title-borne [test] in a relocated unverified claim exempted it from the floor and posted it as a blocking Request changes with no verifier. The split helper reports the deterministic count and the body composer keeps the two provenances apart. - The equals-form PR-shaped invalid value joins the disposal pool exactly as the spaced form does — the round-7 fix wired it into refusal only, so `--severity-floor=6711` reviewed the local tree while `--severity-floor 6711` rescued PR 6711. Every spelling converges; the ambiguity pool counts distinct values, so two spellings of one PR are one candidate. - Side-file deletion requires PROVEN absence — a walked review list with no submitted review by this account — never "recovery returned null": an own review whose marker fails to parse (edited or damaged bot body, marker- less follow-up) is a persistent state, and deleting there stamped the next round "round 1" mid-PR and reset the posture clock. Recovery is three-valued; the middle state strips conservatively. Adjacent closures: an unrecognised severityFloor (model-transcribed drift like "Critical" or "auto ") is the unknown state — unlicensed with a list, inert without one — instead of a refusal that lost zero-deferral rounds over a field that changed no output; the interface doc says UNRESOLVED where it said RESOLVED (the round-5 regression's own wording); the deferrable-set description at four sites names the real set (otherwise-postable high-confidence Suggestions; low-confidence and NTH stay terminal-only) and the rounds-2-5 age deferrals; and the mutation- shown gaps are pinned — pure deferrals stay out of the ledger, a relocation-only run incurs no licence cap, an invalid configured floor is silent on non-PR targets, and a 64-hex commit id survives recovery. * feat(review): type the deferral channel — carry the fields, stop re-parsing prose Every genuine Critical in review rounds 5-8 lived in one place: the deferral channel was free text re-parsed for provenance it did not carry. A separator regex classified deterministic source, a marker regex caught mis-routed Criticals, and each round's probe found the spelling the last fix excluded — kebab paths, the SKILL's own aggregate suffix, an en dash, a title-borne [test], (Critical), a fullwidth colon. A whole-module self-audit (with reproduced witnesses) found eight more shapes and named the class: the fourth, fifth and sixth rounds of the same enumeration trap this repo's own review doctrine (QwenLM#9095) says to close structurally. So the entry is typed. `deferredSuggestions` is an array of `{file, line?, source, severity, title, locations?}` copied from the findings artifact the model already wrote in Step 6: deterministic derives from `source` (build/test/probe), relocation from `severity === Critical` (counts toward C, blocks, rides the ledger, classified by its FIELD — a title mentioning [test] no longer exempts an unverified claim), a `Nice to have` or malformed or free-text entry is refused at the boundary like a NaN count (the channel that un-posts findings is not guessed at), and the human line `file:line — [source] title (+N locations)` is RENDERED by compose-review — nothing downstream parses it back. Both regexes and the split helper's string grammar are gone; the SKILL contract, the state bullet, the submit seam test and the whole deferral describe block are rewritten for the typed shape (no test probes a spelling any more). The self-audit's side-file findings land in the same commit: the recovered write never lowers the round (a stale walked list — a concurrent lane, or a paginated fetch that came back short — overwrote round 7 with round 2 and dropped the anchor sha; compare on round, reviewId as tiebreak), and login comparison is case-insensitive per GitHub (a case mismatch read "own review exists" as proven absence and deleted the counter). Both pinned. This pulls the structural half of QwenLM#9176 into the PR; the issue keeps its remaining bullets (structured `deferred` artifact marker, age-evidence arm, R/D-id timing). * fix(review): bound the relocation exit and dedupe rescued targets by identity Round-9 findings on the pre-typed head, the halves that survive typing: - The relocation exit applies the same per-entry bound as the deferred exit — newline collapse, the 240-char cap without splitting a surrogate pair, the ellipsis on a trim, and Markdown neutralization — through one shared helper. Relocated titles were spliced into the body verbatim, an unbounded feed the deferred exit's cap was added precisely to prevent. - Rescued PR-shaped flag values dedupe by RESOLVED TARGET (number, plus host/owner/repo for a URL), not by raw string: a bare number and a same-number URL are one PR, and a raw-token Set read them as two and silently fell back to the local tree. Of several spellings of one rescued PR exactly one becomes the target; the restatements no longer surface as "Ignoring extra argument(s)" on the invocation the dedupe blesses. - The persist test's debris check asserts on the directory listing rather than a temp name no code path writes (the temp is per-process). The round's other two findings — the whole-entry Critical tripwire and its title-borne false positives — no longer have an input form: the channel is typed (6118109) and severity is a field.
4 participants
What this PR does
Teaches
/reviewto reach the class-closing finding prospectively instead of enumerating an unbounded defect family one entrance at a time. Three prompt-only edits: (1) Agent 3b (Altitude & abstraction fit) gains an enumeration-trap check — a change that hand-rolls an unbounded or adversarial surface (indexOf/slice/regex over structured input, "match what the renderer renders" logic, a re-implemented grammar, a growing hand-listed case set) is a wrong-shape altitude finding: name the class-closing fix (defer to a real parser, the tool's own authoritative structured output, or a fail-closed decision) and file it once, not case by case; severity follows the risk the shape carries. (2) The incremental re-review's divergence-class handling now distinguishes a bounded family (enumerate the countable entrances, as today) from an unbounded one (fresh siblings recurring across ≥2 rounds, or an inherently infinite surface) — for the unbounded case it stops filing sibling N and carries one class-level Critical demanding the structural fix, ruledfixedonly when that fix lands. (3) Pattern aggregation also groups by root cause, not only same-symptom-across-locations. Severity-as-direction (Critical blocks, Suggestion does not), the Criticals-only verdict, and the existing incremental-ruling protocol are unchanged; this adds one escalation layer on top of them.Why it's needed
When a PR hand-rolls an unbounded or adversarial surface — parsing untrusted input, matching a rendered format, re-implementing a spec/grammar —
/reviewspins for many rounds. The reviewer already models a divergence "family" and correctly files each newly-found sibling entrance as a fresh Critical for a bounded family; but an unbounded surface has no last entrance, so patching them one at a time never converges. The diff — and disproportionately its test file — bloats round after round, and the class-closing insight ("stop enumerating; use a real parser") only surfaces retrospectively. This is the exact dynamic behind the long-runningautofix/takeoverreviews: a hand-rolled CommonMark scanner chased corner after corner until it was replaced by a real parser, and a hand-rolled surefire-XML parser is doing the same now. The reviewer should reach that conclusion in round 1, not round 8.Reviewer Test Plan
How to verify
Prompt-text change with a unit-test oracle. Run the skill invariants and the brief-assembly tests:
npx vitest run packages/core/src/skills/bundled/review/SKILL.test.ts(8/8 — the incident-pointer bidirectional invariant and pinned-string checks stay green, confirming nomeasured; DESIGN.mdpointer was added) andnpx vitest run packages/cli/src/commands/review/agent-prompt.test.ts(232/232 — the briefs the command builds still assemble, including the extended Agent 3b brief).tsc/eslint/prettierare clean. To read the behavior change directly, diffSKILL.md's "Previous round's findings" section (the bounded-vs-unbounded escalation) and Agent 3b's brief inagent-briefs.ts(the enumeration-trap bullet).Evidence (Before & After)
N/A — prompt/skill text only, no user-visible or TUI surface.
Tested on
Environment (optional)
N/A — unit tests only; no runtime/sandbox involved.
Risk & Scope
/reviewruns; this PR verifies the prompt/invariants, not the emergent reviewing outcome. The autofix-loop side (round budgets, handoff) is deliberately out of scope — it belongs to autofix, not/review.Linked Issues
None. Companion to the
/reviewhardening series (#8956, #9020) and the toolchain-adapter reviews it generalizes from; referenced without a closing keyword.中文说明
这个 PR 做了什么
让
/review提前得到"关类"结论,而不是把一个无界缺陷家族逐个入口枚举。三处纯 prompt 改动:(1) Agent 3b(层次与抽象合理性)新增 enumeration-trap 检查——手搓无界/对抗性面(用indexOf/slice/正则扫结构化输入、"匹配渲染结果"、重实现文法、逐个 case 堆列表)是错形状的 altitude finding:一次性给关类修法(真解析器 / 工具权威结构化输出 / fail-closed),不逐个报;严重度随风险。(2) 增量复审的 divergence-class 处理区分有界家族(照旧枚举可数入口)与无界家族(sibling 连续 ≥2 轮复发、或面本身无穷)——无界时停止报第 N 个 sibling,改成一条 class-level Critical 要求结构性修法,只有该修法落地才判fixed。(3) Pattern aggregation 除"同类型跨位置"外也按根因聚类。严重度即方向(Critical 阻断、Suggestion 不阻断)、Criticals-only 裁决、以及既有增量 ruling 协议均未改动;本 PR 只在其上加一层升级。为什么需要
当一个 PR 手搓无界/对抗性的面——解析不可信输入、匹配渲染结果、重实现 spec/文法——
/review会空转很多轮。reviewer 已会建模 divergence"家族",并对有界家族正确地把每个新 sibling 入口当作新 Critical;但无界面没有"最后一个入口",逐个打补丁永不收敛。diff——尤其是测试文件——一轮轮膨胀,而"该换真解析器"这个关类结论总是事后才浮现。这正是长跑的autofix/takeover评审背后的机制:手搓 CommonMark 扫描器逐角落追,直到换真解析器;手搓 surefire-XML 解析器现在在重演。reviewer 应当第 1 轮就得到该结论,而非第 8 轮。Reviewer Test Plan
如何验证
纯 prompt 文本改动,以单测为判据。跑 skill 不变量与 brief 组装测试:
npx vitest run packages/core/src/skills/bundled/review/SKILL.test.ts(8/8——incident-pointer 双向不变量与钉字符串检查仍绿,证明未新增measured; DESIGN.md指针)与npx vitest run packages/cli/src/commands/review/agent-prompt.test.ts(232/232——命令构建的 briefs 仍能组装,含扩展后的 Agent 3b brief)。tsc/eslint/prettier干净。要直接读行为改动,看SKILL.md的 "Previous round's findings" 段(有界/无界升级)与agent-briefs.ts里 Agent 3b 的 brief(enumeration-trap 那条)。证据(Before & After)
N/A —— 仅 prompt/skill 文本,无用户可见或 TUI 面。
测试环境
运行环境(可选)
N/A —— 仅单测,无运行时/沙箱。
风险与范围
/review运行中观察;本 PR 验证的是 prompt/不变量,而非涌现的评审结果。autofix 循环侧(轮次预算、交接)刻意不在范围内——那属于 autofix,不属于/review。关联 Issue
无。作为
/review加固系列(#8956、#9020)及其所泛化的 toolchain-adapter 评审的配套;仅引用,不带关闭关键字。