feat(review): add runtime-axis, table-sweep and isolation witness forms - #9445
Merged
Merged
Conversation
The witness rule's forms all answer "what does this input produce": probe runs a unit harness, base-tree runs one input on two trees, extract-step executes a lifted run: body, the sweep iterates a population. Three common claims fall outside that shape. A forward-compatibility claim is unfalsifiable on the one runtime the harness happens to be running, and a green matrix is only evidence about the versions in it. base-tree's version axis is git; this adds the runtime axis, capped at one other version and priced at a tarball rather than an install and a build. A hardcoded table mirroring another system's namespace looks like data rather than logic, so it gets checked by reading it against a list somebody retypes — which is the mirrored oracle the sweep already rejects, wearing a disguise. Parsing the literal out of the source and diffing it against the authority at runtime makes both directions visible, and the direction that ships is the one no test written against the table can see. A claim about an aggregate invites a per-component dump, after which the verdict rests on code the review itself wrote. Removing one contributor and re-reading leaves both numbers coming out of unmodified code, and settles sum-versus-maximum in a single pair of readings. None of the three needs a new CLI surface: the witness field already carries free text, so the witness rule's enumeration of forms is the only other line that had to move.
wenshao
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
yiliang114
as code owners
August 19, 2026 01:22
Collaborator
Author
|
Companion issue for the two capabilities that need real CLI surface, deliberately left out of this PR: #9446 ( |
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
Author
|
@qwen-code /takeover |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: this is a capability addition, not a bug fix, and the gap is an observed one rather than a theoretical one — all three forms came out of a hand verification of a daemon PR where the existing witness forms could not reach the claims, and the motivating examples are concrete (a heap-space taxonomy that moves between Node 22 and Node 24; a sum-vs-maximum aggregate question). The shell-checkable claims carry their own evidence in the description. Direction: aligned. This extends the bundled Size: the files live under Approach: the scope feels exactly right. No new subcommand, flag, or artifact field — the Risk: no elevated risk signals — two Markdown files, no match against the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 这是能力新增而非 bug 修复，且缺口是真实观察到的，不是理论性的——三种形式都来自一次手工验证 daemon PR 时发现现有 witness 形式够不到那些断言，动机示例也很具体（Node 22 与 Node 24 之间会位移的 heap space 分类；一个"求和还是取最大"的聚合问题）。描述中可在 shell 里核对的主张自带证据。 方向： 对齐。这扩展的是内置 规模： 文件位于 方案： 范围恰到好处。没有新子命令、新 flag、新 artifact 字段—— 风险： 无升级风险信号——两个 Markdown 文件，未命中任何与 revert 相关的路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewFor a prose-only change the correctness bar is internal consistency and the skill's own invariants, so that is what this pass checked. Independent proposal before reading the diff: add the capability prose where Step 4 already describes the brief's capabilities, update the one exhaustive witness-forms enumeration, put rationale in DESIGN.md under the existing Verified claims — the PR's self-description is accurate on every checkable point:
Three non-blocking observations, none rising to a change request:
Numeric claims: the Node 22/24 heap-space counts (11 vs 13) in the motivating prose are the author's own shell output, quoted in the description; I could not re-run them here and nothing in the change's correctness depends on them — they are motivation, while the structural invariants are what the CI suites pin. Testing evidence — the PR's own CI (unattended run; no PR code executed here)The Ubuntu unit suite — the PR's real gate, and the one running the skill's structural tests ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 No user-visible or TUI change, so no real-scenario terminal capture applies ( 中文说明代码审查对纯散文改动，正确性标准是内部一致性与 skill 自身的不变量，本次审查就围绕这两点。读 diff 之前的独立方案：在 Step 4 已有的"brief 携带哪些能力"处新增能力散文、更新唯一一份穷举的 witness 形式清单、把理由按现有 可核对的主张全部属实：
三条不构成阻塞的观察：
数字主张： 动机散文里 Node 22/24 的 heap space 数（11 对 13）是作者自己的 shell 输出，已在描述中引用；本审查无法在此重跑，且改动的正确性并不依赖这些数字——它们是动机，结构性不变量由 CI 套件钉住。 测试证据——PR 自己的 CI（无人值守运行；此处未执行任何 PR 代码）Ubuntu 单元套件——本 PR 真正的闸门，也是跑 skill 结构性测试（ — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — solid; the three observations from Stage 2 are non-blocking wording/architecture questions, not doubts about the change itself. Stepping back: my independent proposal for "make forward-compatibility, namespace-table, and aggregate claims reachable by the verifier" was exactly what this PR does — capability prose in Step 4, one exhaustive enumeration updated, rationale under DESIGN.md's The reservations are the small ones already named: the "all below" phrasing misplaces one of the four forms by two paragraphs, the table-sweep scope line sits in slight tension with the runtime axis's no-matrix cap, and the verifier brief in Approval is deferred only because CI has not landed: the Ubuntu unit suite — the gate that runs the skill's structural tests — is still in flight on this commit. Approval will attach, pinned to the reviewed commit, once every check on it completes green; a red landing or a moved head withholds it. 中文说明回头看：我为"让前向兼容、命名空间表、聚合值这三类断言对 verifier 可达"准备的独立方案，正是这个 PR 的做法——Step 4 的能力散文、更新唯一一份穷举清单、理由按 DESIGN.md 的 保留意见就是 Stage 2 已点名的那些小问题："all below" 的措辞把四种形式中的一种错置了两段；table sweep 的版本范围与 runtime 轴的"不要长成矩阵"略有张力；而 verifier 实际阅读的 brief（ 批准之所以暂缓，只因 CI 尚未落定：Ubuntu 单元套件——跑 skill 结构性测试的那道闸门——在该 commit 上仍在运行。待其上所有检查全绿后，批准会以钉住被审 commit 的方式附上；落红或 head 移动则不予批准。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
previously approved these changes
Aug 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 8 by the review time budget.
Test Plan (not a blocker): src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/commands/review/run-skill-parity.test.ts — no such file or directory; src/commands/review/lib/stale-bundle.test.ts — no such file or directory; src/commands/review/lib/review-digest-covers-only-bundled.test.ts — no such file or directory; src/services/BundledSkillLoader.test.ts — no such file or directory; and 6 more.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 8 轮。
Test Plan（非阻断）：src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/commands/review/run-skill-parity.test.ts — no such file or directory; src/commands/review/lib/stale-bundle.test.ts — no such file or directory; src/commands/review/lib/review-digest-covers-only-bundled.test.ts — no such file or directory; src/services/BundledSkillLoader.test.ts — no such file or directory; and 6 more。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
|
||
| The brief also carries the **A/B capability**, which is the probe's counterpart for a claim that a probe structurally cannot settle. A probe runs the PR's code and answers "what does it do now"; it cannot answer "and what did it do before". A whole class of finding is exactly that difference — "this changes the output format", "this only adds a field", "cancelled and failed used to be indistinguishable" — and recovering the old behaviour by reading the diff is the step that goes wrong quietly, because the new lines are always present and always look right. So a verifier facing a comparative claim can run `qwen review base-tree`, which builds the merge base in a sibling worktree, and then run the same input on both sides and quote both outputs. Until this existed, `mergeBaseSha` was used for exactly one thing — choosing the diff range — and no step in this pipeline had ever built the code the PR is a change _to_. It costs an install and a build (reused across the review once built), so it is spent per finding rather than per review, and an unavailable base (no merge base, a stale one, a base that will not compile) is a fact about the harness that never becomes a finding against the PR. | ||
|
|
||
| The A/B's version axis is git, and it is not the only one. A claim that the code **handles the next version of something it does not ship** — a runtime whose enumeration changes under it, a dependency that removed an API in its next major, a wire format that gained a field — is unfalsifiable on the one runtime the harness happens to be running, and a green CI does not close it either: a matrix is evidence about the versions in the matrix. So a verifier facing a forward-compatibility claim **installs the other version and runs the smallest discriminator on both**, rather than ruling on the claim from a changelog. This is cheap in a way `base-tree` is not — a tarball and one `-e`, no install and no build — and it is decisive in a way reading is not: a heap-space set written against the eleven names Node 22 reports classifies cleanly there and silently drops the two more Node 24 reports, and nothing in the source says which of the two you are on. Keep it to **one** other version, the one the claim names, and quote both outputs side by side as the witness; a version the harness cannot fetch is `witness: not run — <why>` like any other unreachable claim. Do not grow it into a matrix — every extra version is a run the review pays for and a verdict nobody asked about. |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-12: Both new measured anecdotes — the Node 22/24 heap-space measurement here and the two-child daemon measurement at line 615 — carry no (measured; DESIGN.md — …) pointer and are recorded as free ## Why sections instead of ### headings under "## Measured incidents behind the SKILL.md rules" (all 60 existing pointers resolve there). The PR description says this is deliberate, matching how extract-step and the renderer document theirs — and that convention exists. But in both cited precedents the SKILL.md capability paragraph does NOT duplicate the measurement numbers; the numbers live in exactly one file, so there is no twin to diverge. This PR's paragraphs duplicate their numbers into SKILL.md without a link — and the duplicated twin has already diverged (R1-8's heap-peak split; R1-7's ten-vs-eleven tension), with SKILL.test.ts staying green (22/22) because nothing in it sees either anecdote. — Failure scenario: going forward, either file's anecdote can be edited or deleted with both pointer tests staying green — the only two measurements in the Step-4 capability cluster with no test linkage — so the numbers the new witness forms stand on can silently diverge from their record, as they already did in this commit.
Suggested fix: either give each anecdote a Measured-incidents ### entry with the matching pointer, or single-source the numbers in DESIGN.md and keep the SKILL.md paragraphs qualitative, as the cited precedents do.
中文说明
两则新实测轶事——此处的 Node 22/24 堆空间测量与 615 行的双子进程 daemon 测量——都没有 (measured; DESIGN.md — …) 指针，并以自由的 ## Why 小节记录，而非 "## Measured incidents behind the SKILL.md rules" 下的 ### 标题（既有 60 个指针全部解析到那里）。PR 描述称这是刻意为之、与 extract-step 和 renderer 的记录方式一致——该惯例确实存在。但两个先例中，SKILL.md 的能力段落都没有复制测量数字；数字只存在于一个文件，因此没有可能漂移的孪生副本。本 PR 的段落把数字复制进了 SKILL.md 却没有链接——而孪生副本已经漂移（R1-8 的堆峰值分歧、R1-7 的十/十一之名分歧），SKILL.test.ts 依然全绿（22/22），因为测试根本看不到这两则轶事。失败场景：今后任一文件的轶事被修改或删除，两个指针测试都保持绿色——这是 Step-4 能力簇中唯一两个没有测试链接的测量——新 witness 形式所依赖的数字可能与其记录悄悄漂移，正如本提交中已经发生的。
建议修复：或为每则轶事在 Measured incidents 下建 ### 条目并加上对应指针；或把数字单一来源在 DESIGN.md、SKILL.md 段落保持定性描述，与所引先例一致。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Collaborator
There was a problem hiding this comment.
Declined — the recorded round-1 decision stands; its counter-evidence was re-verified against the current head this round. The finding's premise ("in both cited precedents the numbers live in exactly one file, so there is no twin to diverge") is contradicted by the impact-sweep measurement itself: grep -c '195 of 434' returns SKILL.md ×1, DESIGN.md ×3, agent-briefs.ts ×1 — five untethered copies across three files, all predating this PR, with both pointer tests green. An untethered number in a Step 4 capability paragraph is therefore the established pattern, not something this PR introduces, and the proposed remedy does not follow from its premise; its alternative (single-source in DESIGN.md, keep SKILL.md qualitative) would make these the only Step 4 capability paragraphs without a concrete number. The real observation underneath — the twin diverged inside the original commit — was fixed by reconciling the numbers (R1-8). If the untethered-measurement problem is worth closing, it should be closed across all five copies at once, which is worth its own follow-up issue; the thread stays open for a maintainer to schedule that.
中文说明
拒绝 —— 第 1 轮记录在案的决定维持不变，其反证据已在本轮对照当前 head 重新验证。该 finding 的前提（"两个被引先例中数字只存在于一个文件，因此不存在会分叉的孪生副本"）被 impact sweep 测量本身推翻：grep -c '195 of 434' 返回 SKILL.md ×1、DESIGN.md ×3、agent-briefs.ts ×1 —— 共 5 份无指针副本分布在三个文件中，全部早于本 PR，且两个指针测试均为绿色。因此 "Step 4 能力段落里出现无指针的数字" 是既有惯例，并非本 PR 引入，所提补救也就无法从其前提推出；而它给出的替代方案（数字只留在 DESIGN.md、SKILL.md 保持定性）会使这三段成为 Step 4 中唯一不含具体数字的能力段落。其下的真实观察 —— 孪生副本在原始提交内分叉 —— 已通过对齐数字修复（R1-8）。若 "无指针测量值" 问题值得关闭，应当一次性覆盖全部 5 份副本，这值得单开一个后续 issue；本 thread 保持打开，由维护者安排。
Collaborator
Author
|
@qwen-code /takeover |
wenshao
added
the
autofix/takeover
Collaborator
Author
|
@qwen-code /takeover |
Round 1 review, R1-1: the Step 4 verifier never reads SKILL.md — its whole instruction set is the brief agent-briefs.ts builds — and every pre-existing form in the witness rule's enumeration has a capability paragraph there, including the impact sweep (phrased "sweep the real population", which is why a search for the SKILL.md name missed it). The three new forms had none, so the only agent that produces witnesses could not reach them. They now sit next to their kin: the table sweep after the population sweep, the version axis after the base-tree A/B, elimination after the drive block, and all three in the brief's own witness-form list. Also from round 1: - The subtraction rule was wrong for the aggregate the worked example actually is. A difference is the removed contributor's value only under a sum; under a maximum, removing a non-holder moves nothing and removing the holder exposes the next-largest, so the stated rule would have had a verifier quote 0.2 MB as a child's heap. Both files now read the pair for the combining rule, and name reducing the population to one contributor as what yields a value outright (R1-10). - Whether a silent contributor is skipped or counted as zero is invisible under a sum and a maximum alike; it shows only where a zero moves the figure. Scoped to counts, denominators and averages (R1-11). - The two-child reading is quoted the same way in both files now, and no longer claims to prove more than it does: flat under addition excludes a sum, not every alternative (R1-8, R1-9). - Node 22 exposes eight old-generation names, not ten; the ten-name set is the classifier's, and the two Node 24 adds are named (R1-7). - One name for the capability, "version axis", matching its defining paragraph and the DESIGN.md heading (R1-3), and the witness rule no longer points below for a form that is above it (R1-2). - The enumeration trap is conditional on an unbounded entrance space, and most of these tables are bounded — the lens's own carve-out (R1-5). - "Installs the other version" against "no install and no build" read as a contradiction; the cost is a download, no dependency install (R1-6). - The one-version cap and "every version the repo supports" contradicted each other. Both now say: the versions the claim names, which for a support range is the floor and the newest (R1-4).
Collaborator
Author
|
Round 1 addressed in R1-1 — accepted, and it was the finding that made this PR workCorrect, and my description was wrong to say the change was confined to two files. The verifier never reads SKILL.md; R1-10 — accepted; it was a real defect, not a wording one"The difference is that contributor's own value" is true for a sum and false for the aggregate my own worked example uses. Worse, it mis-described the method that produced the example: I did not subtract anything — I reduced the population to one child and read it. So a verifier following the sentence on a maximum would have quoted 0.2 MB as a child's heap when the child held 103.7 MB, and the witness rule would have promoted that to verdict-settling evidence. Both files now read the pair for the combining rule (doubles with the population ⇒ sum; flat ⇒ not a sum; reduce to one contributor ⇒ that contributor's value outright) and state the sum-only restriction on subtraction explicitly. R1-2 … R1-9, R1-11 — accepted
R1-12 — rejected: the stated precedent does not holdThe finding rests on "in both cited precedents the SKILL.md capability paragraph does NOT duplicate the measurement numbers; the numbers live in exactly one file, so there is no twin to diverge." The impact sweep / extract-step precedent duplicates its number across three files, with no pointer:
The observation underneath it is still real, and R1-8 is its proof — the twin diverged inside this commit. But the remedy the finding proposes does not follow from the premise it argues, and the alternative it offers second (single-source in DESIGN.md, keep SKILL.md qualitative) would make these three paragraphs the only capability paragraphs in Step 4 without a concrete number, which is the property that makes the surrounding ones persuasive. Reconciling the numbers is what I did. If the untethered-measurement problem is worth closing, it should be closed across all five copies at once rather than only on the newest two — worth its own issue, and I am happy to file it. Two notes that are mine rather than the review's, both from following R1-1 into The companion issue #9446 was written on a false premise and has been corrected. I greppped SKILL.md for a live-service capability and found none, and filed the gap on that basis. The capability exists —
中文说明第 1 轮的意见已在 R1-1 —— 接受，是让这个 PR 真正成立的那条。 正确，我的描述说"改动局限于两个文件"是错的。verifier 从不读 SKILL.md，它的全部指令是 R1-10 —— 接受，这是真缺陷而非措辞问题。 "the difference is that contributor's own value" 对求和成立，对我自己举的那个例子（取最大）不成立。更糟的是它误描述了产出该例子的方法：我根本没有做减法，而是把种群缩到只剩一个子进程再直接读。按那句话在取最大的聚合上操作的 verifier，会在子进程实际持有 103.7 MB 时把 0.2 MB 当作它的堆值引用，而 witness rule 会把它提升为定论级证据。两个文件现在都改为按组合规则读这一对数字（随种群翻倍 ⇒ 求和；保持不动 ⇒ 不是求和；缩到单个贡献者 ⇒ 直接就是它自己的值），并显式写明减法只在求和下成立。 R1-2 … R1-9、R1-11 —— 全部接受：witness rule 不再对位于其上方的形式说 "below"（R1-2）；统一命名为 version axis，"runtime axis" 两处引用已删除，它对一个涵盖依赖大版本与线上格式的能力来说也过窄（R1-3）；版本范围一律以断言点名的版本为准，支持区间的完整性断言点名"下限与最新"两个，作为有界例外而非放行（R1-4）；enumeration trap 改为"入口空间无界时"，并明说有界命名空间正是该透镜的例外，所以这类表大多是正当的（R1-5）；成本措辞与 DESIGN.md 对齐为"一次下载和一行 R1-12 —— 拒绝：所依据的先例不成立。 该条立论于"两个被引先例中 SKILL.md 的能力段落都没有重复测量数字，数字只存在于一个文件，因此不存在会分叉的孪生副本"。事实是 impact sweep / extract-step 这一先例把它的数字重复在三个文件里且都没有指针： 其下的观察本身仍然成立，R1-8 就是它的证据——孪生副本确实在本次提交内分叉了。但该条提出的补救并不从它论证的前提推出；而它给的第二个选项（数字只留在 DESIGN.md、SKILL.md 保持定性）会使这三段成为 Step 4 中唯一不含具体数字的能力段落，而具体数字正是周围那些段落有说服力的原因。我采取的是把数字对齐。若"无指针测量值"这个问题值得关掉，应当一次性覆盖全部 5 份副本，而不是只管最新的两份——那值得单开一个 issue，我可以去开。 另外两点是我自己的，不是评审提的，都来自顺着 R1-1 追进
|
qwen-code-ci-bot
approved these changes
Aug 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/commands/review/run-skill-parity.test.ts — no such file or directory; src/commands/review/lib/stale-bundle.test.ts — no such file or directory; src/commands/review/lib/review-digest-covers-only-bundled.test.ts — no such file or directory; src/services/BundledSkillLoader.test.ts — no such file or directory; and 8 more.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/commands/review/run-skill-parity.test.ts — no such file or directory; src/commands/review/lib/stale-bundle.test.ts — no such file or directory; src/commands/review/lib/review-digest-covers-only-bundled.test.ts — no such file or directory; src/services/BundledSkillLoader.test.ts — no such file or directory; and 8 more。
— qwen3.8-max via Qwen Code /review (v0.21.14)
doudouOUC
approved these changes
Aug 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Independent review — verified against HEAD ebc35fc5
Verdict: APPROVE. I re-checked all twelve round-1 threads against the current HEAD rather than trusting their outdated flags, reproduced the prose's shell-checkable claims locally, and re-ran the six guard suites.
The twelve round-1 threads — each checked at this HEAD, each resolved
The second commit is a dedicated fix round, and I verified every item against the text as it now stands:
- R1-1 (forms missing from the verify brief) — fixed.
agent-briefs.tsnow carries all three capability paragraphs ("hardcoded table of another system's namespace", "the A/B's other axis is not always git", "change the population rather than instrumenting the reader"), and the witness-return enumeration gained the three matching outputs — "the two versions' outputs, the table's set difference, the two readings either side of a removal". - R1-2 (false "all below" pointer) — fixed. The enumeration now reads "the version axis's two-version pair (above), and — all below — the impact sweep, its table sweep specialization, and an isolation by elimination pair"; I confirmed the paragraph order in SKILL.md (version axis at 605 above the rule at 609; the other three below it).
- R1-3 (naming drift) — fixed. SKILL.md, the witness rule, the table sweep's back-reference, and DESIGN.md's heading all say "version axis"; the old "runtime axis" variant is gone from the files.
- R1-4 ("every version the repo supports") — fixed. The table sweep now says "run it on the versions its claim covers — for a support range, the floor and the newest", which is exactly the bounded exception the version-axis paragraph defines.
- R1-5 (enumeration trap misread) — fixed, and I verified the premise:
ENUMERATION_TRAP_LENSin agent-briefs.ts states its trigger is unboundedness and explicitly carves out bounded grammars, so "a bounded namespace is the carve-out that lens names" is accurate, not just asserted. - R1-6 (equivocating "install") — mitigated: the cost line is now "a download and one
-e, no dependency install and no build", which separates fetching the named version from an install-and-build. Residual looseness in the verb at most; not worth a round. - R1-7 / R1-8 (the two anecdotes quoted inconsistently across files) — fixed. The daemon measurement is now "193.6 → 377.5 MB / 103.5 → 103.7 MB" in both files, and the heap-space anecdotes no longer contradict: DESIGN.md tells the eight-name old-generation-set variant, SKILL.md and the brief the eleven-name full-set variant.
- R1-9 ("a maximum and could not be anything else") — fixed. DESIGN.md now says the pair "excludes a sum … and no more than that on its own", names the three readings that reproduce it, and points at the second removal as the discriminator.
- R1-10 (difference = contributor's value, unconditionally) — fixed in all three places: "The difference is a contributor's value only under a sum — under a maximum, removing a non-holder moves nothing and removing the holder exposes the next-largest."
- R1-11 (zero-folding claim overreached) — fixed. The paragraph now explicitly excludes it: "whether a contributor reporting nothing is skipped or folded in as a zero is invisible under a sum and a maximum alike, and shows only in a figure a zero would move".
- R1-12 (new anecdotes carry no measured pointer) — this one is a design decision rather than a fix, and I checked the convention it appeals to:
(measured; DESIGN.md — …)pointers anchor only into the "Measured incidents" section, which records review runs going wrong; capability rationale lives in## Why …sections, exactly as extract-step ("Why extract-step exists, and why it stubs nothing") and the renderer do. The three new## Whysections follow that pattern, and SKILL.test.ts's bidirectional pointer invariant still passes, so nothing dangles.
Facts in the prose, reproduced locally
The two shell-checkable claims hold on my own machines: getHeapSpaceStatistics() reports 11 spaces on Node v22.22.3 (eight non-new-generation) and 13 on Node v24.12.0, the two additions being exactly shared_trusted_space and shared_trusted_large_object_space. The daemon anecdote is an observation rather than a reproducible command, and the prose around it no longer claims more than the pair shows.
Guard suites re-run at this HEAD
All six suites from the test plan pass here: SKILL.test.ts 22, run-skill-parity 2, stale-bundle 28, review-digest-covers-only-bundled 6, BundledSkillLoader 27, agent-prompt 269 — the same counts the PR reports, so the parity suite confirms no --out template was disturbed.
CI is fully green, the change is prose-only (no schema, flag, or artifact field), and the only residual is the R1-6 verb looseness noted above.
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Collaborator
Collaborator
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round — re-verification of the round-1 suggestions, no code changes neededThis round re-verified the twelve round-1 review suggestions against the current head (
The two issue-level comments need no action: the companion-issue note (#9446) is informational, and the round-1 response comment is the record this round re-verified. VerificationCommands run this round against the unchanged head (
中文说明Autofix 轮次 —— 对第 1 轮建议的复核，无需代码改动本轮在当前 head（
两条 issue 级评论无需处理：配套 issue 备注（#9446）为信息性内容；第 1 轮回应评论即本轮复核所依据的记录。 验证本轮在未改动的 head（
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Collaborator
|
Released in v0.21.15. |
yiliang114
pushed a commit
to yiliang114/qwen-code
that referenced
this pull request
Aug 22, 2026
…enLM#9655) * feat(review): report the address a drive's service actually bound A port is a request, not a fact. Handed one that is taken, `qwen serve` prints `port 8931 is in use, trying 8932...` and listens on the next. A verifier that goes on addressing the port it asked for then reads a different, stale process for the rest of the run — its readiness probe passes against whatever is squatting there, the drive completes, and every number in the witness is about the wrong daemon. Nothing in the report says so, because nothing in the report knew: `drive` had no port handling at all. Measured during a daemon verification, it cost a full cycle before the readings stopped making sense. `--capture name=<regex>` reads named facts back out of the run's own output into `captured`, the bound address first among them. Four choices in it are the ones that matter: - It reads the UNTRIMMED log. `trimCapture` keeps the tail and a service prints its address at the head, so capturing from the report's `output` would lose exactly the value this exists for, on the loudest runs — the ones most likely to need it. - A pattern that never matched is `null`, never `''`, and the note NAMES it. That is the moment a witness is about to quote a value the run never produced, and the reader has to know which one. - A malformed pattern rejects the whole set before anything starts. Silently dropping the bad entry would leave a missing key beside the good ones, which reads as "the service never printed it" — the one meaning `null` is reserved for. Finding out after a 300-second drive costs the drive. - Captures are taken on every outcome, not only `completed`: a drive that timed out still bound its port, and that address is often what explains where the rest of it went. The verify brief carries it too. A capability taught only where the verifier does not read is inert — the lesson from QwenLM#9445's first review round — so the brief now says to bind ephemeral where the service allows it and quote the captured address rather than the one on the command line. Refs QwenLM#9446. * fix(review): correct drive capture examples and bound its outputs (QwenLM#9655) * fix(review): a declared capture group the match left unfilled is null Round 1/2 review. `m[1] ?? m[0]` reads as "group 1, or else the whole match", and for an OPTIONAL group that is a silent substitution rather than a fallback: `(?:a(x))?b` against `b` matches with group 1 absent, so the caller that asked what `x` matched received the whole match `b` under the same name, with nothing in the report saying a different question had been answered. Whether the pattern DECLARES a group and whether group 1 happened to participate are different questions; conflating them loses a value quietly, which is the one failure mode this command exists to remove. The declaration is now settled when the pattern is parsed — by matching `<source>|` against the empty string, guarded, since it builds a second pattern from the first — and a declared-but-unfilled group is `null`. A pattern with no group still yields the whole match. The docblock's `null`-versus-`''` claim was too strong beside that, and the narrowing belongs to the pattern rather than the service: `''` means the group captured zero characters, which a `*`-quantified group does wherever it is anchored (`pid=(\d*)` returns `''` against `pid=abc`). Said so, and named `+` as what a "printed nothing" test needs. Also from the two rounds: - The head-trim clause and the capture block contradicted each other in the note. `output` is trimmed at the head, `captured` is read before that trim, so a completed noisy drive said "early output is missing" directly beside a value that came out of the missing head — and beside a null the reader would reasonably blame the trim for. A scoping clause now says captures read the untrimmed log, covering the matched case (R1-8) and the missed one (D2-2) together. - Four invariants the docblocks state were pinned by nothing: the unfilled-group rule, the empty-capture-is-a-measurement rule, the first-match rule against a log holding two, and the pattern-length cap's upper bound (only its lower bound was tested). - The CLI seam has a test (D2-1). The handler casts `argv as unknown as DriveArgs`, which type-checks whatever the option is called, so a rename of either side silently disabled `--capture` end to end while every runDrive test stayed green and tsc exited 0. It now drives the real builder with a real flag string and asserts the value reaches the report the handler prints. Each of these was checked by mutation: reverting the group rule, the option name, the first-match rule or the scoping clause reds exactly its own test, and restoring them returns 50 passed. * fix(review): scope drive capture notes to the outcomes they describe (QwenLM#9655) * fix(review): stop the capture guidance from authorising a misattribution @yiliang114's P1. Captures are extracted after the drive loop has ended, so nothing in `captured` can reach a request the script already made — and the brief told the verifier to quote the captured address in the witness anyway. That combination is worse than not capturing at all: a script that talks to 8931 while the service logs its fallback to 8932 reaches its sentinel, returns `observed: true` beside `captured.baseUrl: …8932` with nothing contradicting it, and the witness then attributes 8931's readings to the daemon on 8932. Before this change the same run would at least have quoted the address it really read. The mechanism that prevents the wrong-process read lives in the script, not in the report, and the brief now says so: derive the address from the service's own output before the first request, bind ephemeral wherever the service allows one so the fallback never fires, and let `--capture` show that the address the script used is the one the service printed. `null` is named as "never measured", explicitly not as permission to fall back to the address on the command line. The recipe is spelled out rather than described, because the failure it replaces is one of omission. The report carries the same caveat where it cannot be missed: `captured` is documented as a RECORD of the run and never an input to it. Verified rather than read: the recipe's `sed` and the `--capture` pattern were run against real `listening on` lines, http and https, and agree on the same value — which is the property that makes the capture corroborate the script instead of competing with it. The rendered brief was printed and re-read after the edit, and again after eslint caught an unnecessary `\$` escape in it, to confirm `$BASE` still survives to the agent verbatim. No behaviour change: 340 passed across drive, agent-prompt and run-skill-parity; tsc 0 errors; eslint clean. * fix(review): make the taught bound-address recipe actually capture Round 5 Critical, and it was mine: the recipe added last round sends the service to a file of its own, and `drive` runs the script as `bash <script> > <the drive log> 2>&1` while `extractCaptures` reads only that log. So the service's `listening on` line never reached the capture. Reproduced against the real `runDrive` before touching anything: a faithful run of the recipe returns `completed`, `observed: true`, and `captured.baseUrl: null`, under a note asserting the value "was never measured" — while the service had printed it all along. A recipe that cannot work is worse than no recipe. The service's output has to reach two places, and the shape that does it is a temp file the script greps plus a `cat` of that file before the first request. Three details in it are load-bearing, and each is a finding from this round: - `mktemp`, not a file beside the code. `--cwd` is the reviewed worktree, and an untracked `svc.log` left there is inlined into the next capture of that tree as though the PR added it. - `cat` BEFORE the request. Captures take the first match, so the service's own line wins over any response body containing one — a status endpoint quoting its own banner cannot forge the address. Verified with a service whose body advertises a different port. - No `trap … EXIT` of the caller's own. The wrapper writes its completion sentinel from an EXIT trap and a second one replaces it; measured, a script with its own trap comes back `timed-out` with a null exit code having run perfectly. I was about to use one for cleanup. Block-buffered stdout gets a line too — it is the one failure this shape cannot fix, and the note's advice to raise `--timeout` cannot touch it. The recipe is now pinned by a test that runs the brief's OWN text: the script body and the capture pattern are extracted from the brief rather than retyped, filled with a real service, and executed under the same redirect contract `runDrive` imposes. No tmux is involved, because what broke was the shell. Reverting the recipe to either broken form — the one shipped last round, or merely dropping the `cat` — reds it, and restoring returns 54 passed. 341 passed across drive, agent-prompt and run-skill-parity; tsc 0 errors; eslint clean, including two problems of my own the linter caught in the new test. * fix(review): re-read the drive log once its sentinel is observed Round 6 Critical. The poll loop reads the log and then the sentinel, and breaks on `completed` without reading again — while the wrapper writes the sentinel from an EXIT trap, strictly after the script's last write to the log. A final write landing between those two back-to-back reads is on disk and not in the snapshot, a window one `readFileSync` of a near-cap log wide. The ordering predates this PR, where it cost a truncated tail in `output` and read as a display artefact. Extracting `captured` from the same snapshot turned it into a machine-readable measurement with a false cause attached: `outcome: completed`, `captured.<name>: null`, and a note asserting the pattern never matched — for a value the log on disk contains. That is the shape this command exists to prevent, so the escalation is the defect even though the loop is older than the diff. Reproduced before changing anything, on a 7.9 MiB log with a real writer process swept across the read window: before trials=70 completed=70 stale-tail hits=1 (delay 248.72 ms) outcome completed, captured null, finalmetric=7 on disk after trials=70 completed=70 stale-tail hits=0 same sweep Every log write happens-before the sentinel write, so a read taken after observing it is complete. Scoped to that branch alone: the other exits stopped the run rather than watching it finish and have no such guarantee to lean on, and the existing `existsSync` guard is kept so a script that wrote nothing still reports an empty capture rather than throwing. No unit test: reproducing this deterministically needs the log mutated between two reads inside one iteration, which wants a `readFile` seam this command does not have, and drive.test.ts states that it never mocks `node:fs`. A widened-race test hits ~1 in 70 and a flaky test is its own defect. The measurement above stands as the evidence. 344 passed across drive, agent-prompt and run-skill-parity; tsc 0 errors; eslint clean. --------- Co-authored-by: wenshao <nigolaschao777@gmail.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds three witness forms to the verifier's brief in Step 4, and keeps the witness rule's enumeration of forms current so the new ones are actually reachable. The runtime axis says that when a claim is about a version of something the code does not ship — the runtime it will execute on, a dependency's next major, a wire format that will gain a field — the verifier installs that one other version and runs the smallest discriminator on both sides, rather than ruling on the claim from a changelog. The table sweep aims the impact sweep's oracle rule at the commonest enumerable a diff contains, a hardcoded table mirroring another system's namespace, and says to parse the literal out of the source rather than retyping it before taking the set difference against the authority at runtime. Isolation by elimination is the form for a claim about an aggregate: instead of adding a per-component dump and reading that, remove one contributor and read the aggregate again, so both numbers come out of unmodified code.
The rationale for each goes to DESIGN.md, alongside the existing sections for
base-tree,extract-stepand the renderer.The verify brief in
agent-briefs.tscarries the same three capabilities, because that brief — not SKILL.md — is the verifier's entire instruction set, and every pre-existing form in the witness rule's enumeration already has a capability paragraph there. Its own witness-form list gains the three new outputs. Beyond that nothing changes: no new subcommand, no new flag, no new artifact field, since thewitnessfield already carries free text.Why it's needed
The forms the pipeline has are all shaped the same way — start something, feed it one input, read its output. That shape is what makes them cheap and repeatable, and it is also what puts three ordinary kinds of claim out of reach.
A forward-compatibility claim cannot be settled on the one runtime the harness happens to be running, and CI does not settle it either: a green matrix is evidence about the versions in the matrix, which is exactly the reading that feels like coverage and is not. The cost of closing it turned out to be trivial next to
base-tree— a tarball and one-e, no install and no build — which is also the reason the paragraph caps it at a single other version instead of letting it grow into a matrix the review pays for and nobody asked about.A hardcoded table of another system's namespace — heap-space names, error codes, MIME types, status codes, locales — reads as data rather than logic, so it gets checked by eye against a list somebody retypes. That is the mirrored oracle the sweep already rejects for parsers, wearing a disguise, and with the extra hazard that a table short enough to retype accurately is a table one feels confident about. Diffing the parsed literal against the authority makes both directions visible, and they are not the same finding: a name the table has and the authority does not is dead weight, while a name the authority has and the table does not is an under-count — the direction that ships, and the one no test written against the table can see, because the table is what the test enumerates.
A claim about an aggregate invites the repair that costs the verdict its standing. Adding a per-component dump means the numbers now come out of a build the review edited, and a witness exists to be a run of the code under review rather than a run of the reviewer's version of it. Removing a contributor inverts that dependency: the reader is untouched, the population changes, and the delta between two readings is the removed contributor's own value.
All three came out of verifying a daemon PR by hand and finding that the existing forms could not reach the claims it made. The two capabilities that would need real CLI surface — a budgeted live-service launcher and a graft that reverts named components to base — are filed separately as an issue rather than smuggled in here.
Reviewer Test Plan
How to verify
The change is prose in a skill, so the check is that it says the right thing and that the skill's own invariants still hold.
packages/core/src/skills/bundled/review/SKILL.test.tsenforces the load-bearing one: every(measured; DESIGN.md — …)pointer in SKILL.md must anchor at a DESIGN.md incident heading, and every incident heading must be pointed at. The new paragraphs deliberately carry no such pointer — none of this comes from a review run going wrong, which is what that section records — so their rationale lands in DESIGN.md as## Why …sections, matching howextract-stepand the renderer document theirs.The parity suite is the one worth naming: it reads the
--outtemplates out of SKILL.md and renders them, so a skill edit that disturbs a template fails there rather than months later in a review. No template is touched here, and it stays green.Reading the paragraphs themselves, the two claims in them that are checkable in a shell are checkable in one line each. The runtime axis cites the heap-space taxonomy moving under a set written against one Node:
The two extra names on Node 24 are
shared_trusted_spaceandshared_trusted_large_object_space.Isolation by elimination cites a two-child daemon whose summed RSS moved 193.6 → 377.5 MB while its reported heap peak moved 103.5 → 103.7 MB — a sum and a maximum, distinguished by one pair of readings, taken from the same unmodified status endpoint.
Evidence (Before & After)
N/A — no user-visible or TUI change.
Tested on
Environment (optional)
Local
npm run buildfollowed by the five suites above on Node v22.22.2; the taxonomy figures additionally on Node v24.19.0.Risk & Scope
witnessfield's contents were already free text.Linked Issues
None. The companion issue proposing
review live-serviceandreview graftis referenced from this PR's thread.中文说明
这个 PR 做了什么
在 Step 4 verifier 的 brief 与 SKILL.md 里新增三种 witness form，并同步更新 witness rule 对"witness 可以是什么"的枚举，否则新增的形式实际上够不到。runtime 轴：当一条断言讲的是"代码能处理某个它并不携带的版本"——它将要运行其上的 runtime、某依赖的下一个大版本、将来会新增字段的线上格式——verifier 应当装上那一个版本、在两边跑最小判别式，而不是拿 changelog 来裁决。table sweep：把 impact sweep 已有的 oracle 规则对准 diff 里最常见的可枚举物——一张镜像了别的系统命名空间的硬编码表——要求把字面量从源码里解析出来而不是重新敲一遍，再对着运行时的权威做集合差。isolation by elimination：面对聚合值的断言，不要加 per-component dump 再去读它，而是移除一个贡献者后重读聚合，这样两个数字都出自未经修改的代码。
三者的理由写进 DESIGN.md，和已有的
base-tree、extract-step、renderer 的## Why …小节并列。agent-briefs.ts里的 verify brief 也带上了同样这三种能力 —— verifier 的全部指令来自那份 brief 而非 SKILL.md，而 witness rule 枚举里既有的每一种形式在那里都有对应的能力段落；它自己的 witness 形式清单也补上了三种新输出。除此之外什么都没动：没有新子命令、新 flag、新 artifact 字段，witness字段本来就是自由文本。为什么需要
现有的形式全是同一个形状——起一个东西、喂一个输入、读它的输出。这个形状既是它们便宜且可复现的原因，也正是它把三类很平常的断言挡在门外的原因。
前向兼容断言在"harness 恰好跑着的那一个 runtime"上无法证伪，CI 也关不上这个口子：绿色矩阵只是关于矩阵里那些版本的证据，而这恰恰是最像覆盖率、实则不是的读法。补上它的代价远低于
base-tree——一个 tarball 加一行-e，不需要 install 也不需要 build——这也正是段落把它限死在一个版本的理由：不能让它长成一个 review 要付钱、却没人问过的矩阵。硬编码表（heap space 名、error code、MIME type、status code、locale）读起来像数据而不像逻辑，于是就被"人肉重敲一份清单再对眼"地检查了。那正是 sweep 早已为 parser 拒绝过的 mirrored oracle 换了身衣服，而且多一层危险：一张短到能准确重敲的表，恰恰是最容易让人产生信心的表。把解析出的字面量对着权威做差，两个方向就都显形了，而它们不是同一个 finding：表里有、权威里没有的名字是死条目；权威里有、表里没有的名字是低估——那是会上线的方向，也是任何针对该表写的测试都看不见的方向，因为表本身就是测试所枚举的东西。
聚合值断言会诱使你做出那个让结论失去立场的修补。加一个 per-component dump，意味着数字从此出自一个被 review 自己改过的 build，而 witness 存在的意义就是"它是被评审代码的一次运行"，而不是"评审者那一版代码的一次运行"。移除一个贡献者则把这个依赖反转过来：读取方毫发无损，变的是种群，两次读数之差就是被移除者自己的值。
三者都来自一次手工验证 daemon PR 时的发现：现有形式够不到它提出的那些断言。真正需要新增 CLI 表面的两项能力——受预算约束的 live-service 启动器，以及把指定组件回退到 base 的 graft——单独开 issue，而不是夹带进这个 PR。
Reviewer Test Plan
如何验证
改动是一个 skill 里的散文，所以要检查的是它说得对不对、以及 skill 自身的不变量还成不成立。
packages/core/src/skills/bundled/review/SKILL.test.ts守着那条承重的：SKILL.md 里每一个(measured; DESIGN.md — …)指针都必须锚定到 DESIGN.md 的一个 incident 标题上，反之每个 incident 标题也必须被指到。新增段落刻意不带这种指针——这些内容都不是"某次 review 跑歪了"，而那才是该小节记录的东西——所以它们的理由以## Why …小节落在 DESIGN.md，与extract-step和 renderer 的写法一致。其中 parity 套件值得点名：它把
--out模板从 SKILL.md 里读出来再渲染，所以一次动到模板的 skill 编辑会当场失败，而不是几个月后在某次 review 里悄悄出事。本 PR 未触碰任何模板，它保持绿色。段落本身可在 shell 里核对的两处论据，各只要一行。runtime 轴引用的是 heap space taxonomy 会在"针对某一个 Node 写就"的集合底下发生位移：
Node 24 多出来的两个名字是
shared_trusted_space与shared_trusted_large_object_space。isolation by elimination 引用的是一个双子进程 daemon：其 RSS 之和从 193.6 走到 377.5 MB，而上报的 heap 峰值只从 103.5 走到 103.7 MB —— 一个求和、一个取最大，靠同一个未经修改的状态端点、一对读数就区分开了。
Evidence (Before & After)
N/A —— 无用户可见或 TUI 变更。
测试环境
本地
npm run build后在 Node v22.22.2 上跑上述五个套件；taxonomy 数字另在 Node v24.19.0 上核对。风险与范围
witness字段的内容本来就是自由文本。关联 Issue
无。提议
review live-service与review graft的配套 issue 会在本 PR 讨论串中引用。