feat(review): require an executed witness on every confirmed Critical - #9065
Conversation
A confirmed Critical now carries a witness — the observed output that settled the verdict (a probe's two sides, an A/B's quoted pair, an extract-step run, a repo-wide sweep count) — or one line saying why no run could settle the claim. A confirmed Critical carrying neither is mechanically sorted to low confidence: terminal-only, never posted. Mining the maintainer dogfood corpus (356 review sessions plus 182 real-environment verification sessions) measured what the optional probe/A-B capabilities produce when left to verifier discretion: in the rounds that held up, every posted hard finding quoted executed output, and the one claim written from a reading alone was publicly retracted a round later when its first measurement returned zero (PR 8225). - SKILL.md: the witness rule and the impact sweep in Step 4; witness in the finding format, the findings artifact contract, the Step 7 inline comment bodies, and the Guidelines - DESIGN.md: the decision narrative and two measured entries (the retracted read-only claim; the mirrored oracle's false positives) - agent-briefs.ts: the verify brief demands the witness line (or its 'not run' reason) with every confirmed Critical, and gains the impact-sweep capability with its external-authority guard - findings.ts: optional witness field, round-tripped like outcomeNote so the report and comment bodies quote one recorded string
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @wenshao. The witness rule looks like a worthwhile tightening of the verify pipeline — but the PR body doesn't follow the PR template, so I have to stop at the gate before looking at the change itself.
What's missing:
## What this PR does— currently## Summaryplus the## Changestable## Why it's needed— currently## Why## Reviewer Test Planwith itsHow to verify/Evidence (Before & After)/Tested onsubsections — currently## Test plan## Risk & Scope## Linked Issues- The
<details> 中文说明 </details>translation block
The content itself looks all there — the measured dogfood evidence, the round-2 retraction story, the test plan — it just needs to move into the template's shape, the same one your recent review-skill PRs use. Once the body is updated, re-trigger with @qwen-code /triage and I'll pick it up from there.
中文说明
感谢提交！witness 规则看起来是对 verify 流程很有价值的一次收紧——但 PR 正文没有遵循 PR 模板，所以我必须先在门口停下，无法进入代码审查本身。
缺失的部分：
## What this PR does——目前写的是## Summary加## Changes表格## Why it's needed——目前写的是## Why## Reviewer Test Plan（含How to verify/Evidence (Before & After)/Tested on小节）——目前写的是## Test plan## Risk & Scope## Linked Issues<details> 中文说明 </details>中文翻译块
内容本身都在——dogfood 的测量证据、round-2 撤回的教训、测试计划——只需要挪进模板的结构里（和你最近几个 review-skill PR 一样）。更新正文后用 @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. |
|
@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: 41 passed · 0 failed · 41 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：41 通过 · 0 失败 · 41 总计 Verification reportPR 9065 deep verification —
|
| cell | oracle | head | base (control) |
|---|---|---|---|
| executed witness survives artifact write | f1.witness === 'BASE: 2 calls / PR: 1 call — probe flipped' in written JSON |
✅ kept | ❌ dropped (expected red) |
not run — form survives |
f3.witness exact match |
✅ kept | ❌ dropped (expected red) |
second hop (artifact fed back through --input) |
both witnesses exact after hop 2 | ✅ kept | — |
| absence stays absence | f2 has no witness key |
✅ | ✅ |
whitespace-only witness dropped (asString semantics) |
f4 has no witness key |
✅ | ✅ |
| nothing else moved | artifact minus witness byte-identical; counts, stderr summary, --print stdout identical |
✅ | ✅ |
14/14 assertions. Head 2/2 witnesses retained across two hops; control 0/2 — the pair of counts is the proof the field's survival is the PR's hunk and nothing else's.
A/B cell table — claim 2, verify brief (harness 02-brief-ab.mjs, witness 02-ab-verify-brief-head-vs-base.png)
Both arms drive the real handler (review agent-prompt --plan … --role verify --findings …) and read the brief file it writes; corroborated by direct BRIEFS import (independent mechanism).
| cell | oracle | head | base (control) |
|---|---|---|---|
| three witness demands in the written brief | toContain × 3 |
3/3 present | 0/3 present (expected) |
| brief generation itself unbroken | invariant What is NOT a finding present, exit 0, one brief file |
✅ | ✅ |
direct BRIEFS.verify.brief import |
same three strings | present | absent |
15/15 assertions. Observed aside, no defect: the record key digest (verify--7d941993d32d) is identical on both arms because the digest keys on findings/rules, not brief text — pre-existing behavior, unchanged by the PR.
Corrections
None — first round, and no prior review comments to check against the code.
Findings
No blockers, no defects. Informational items only:
- (Informational) Mutation survivor adjudicated as a dead axis, not a gap worth pinning. M2 (unconditional
...( { witness } )— key always present,undefinedwhen absent) leaves the whole suite green. That is not an untested behavior:JSON.stringifydropsundefinedvalues, so the written artifact is byte-identical either way, and no code anywhere reads key presence ('witness' in f) — the read-site census found none. The truthiness guard is also redundant withasString, which never returns an empty string, sowitness ?andwitness !== undefined ?agree on every possible input. A fixture could pin in-memory key presence (expect('witness' in validateFindings([{...base}])[0]).toBe(false)), but nothing downstream would ever observe the difference — completeness reporting, not a merge condition. The matrix itself is load-bearable: vacuity reverts V1/V2 fail the intended assertions (see below), and positive control M4 (disable theoutcomeNotespread) turns the pre-existing round-trip test red, proving the suite is falsifiable (03-mutation-matrix.png, scored 7/7 by05-score-mutations.mjs). - (Informational)
witnessis parsed by the Web Shell renderer but not displayed.parseCodeReviewDocumentaccepts it (probe04-renderer-probe.mjs: 5/5 — the shipped contract fixture withwitnessinjected on one finding and on all findings parses clean, counts/verdict/fields unchanged; witness04-renderer-accepts-witness.png), and the renderer's own suite passes 19/19. The artifact-detail UI simply has no field for it yet. The PR claims "unaffected" — corroborated behaviorally, not just by reading — and claims no UI, so this is a possible follow-up, not a defect. - (Informational) The PR body's
2428/2429, one failure = stale-bundle.test.tsreconciles. In this container's fresh build the fullsrc/commands/reviewsuite is green: 2425 passed, 4 skipped, 0 failed (68 files). The skips are environmental, named: 3×script-lint.test.tsshellcheck tests (noshellcheckbinary in the container) and 1×save-artifact.test.tscase-insensitive-alias test (Linux FS). Thestale-bundlefailure the author saw did not reproduce here, consistent with their own "stale local build" attribution. - (Informational) No length cap on
witness. A maximal-length witness rides the artifact and, per Step 7, the inline comment verbatim. This is consistent with every sibling free-text field (failureScenario,suggestedFix— none capped; onlysummarygetscompressSummary), so it is no new surface, named only so the asymmetry is a conscious one.
Consistency checks that passed and are worth a line each: the finding-format renumber (5 → Witness, 6 → Suggested fix) leaves no stale numbered references anywhere in SKILL.md; both DESIGN.md headings cited by the new SKILL.md text exist (The read-only claim retracted in round 2 (PR #8225), The mirrored oracle's false positives (PR #8225)), as does the pre-existing one it quotes (The double-execute the probe caught); witness is read camelCase-only, matching category/assets siblings (aliases exist only for older fields); ESLint clean on all four changed TS files with a live control (a planted any violation was caught by the same invocation).
Not covered
- Model-compliance with the witness rule — that a verifier actually emits the witness line and that the orchestrator sorts a witness-less confirmed Critical to low confidence. This is prompt-guided model behavior; the sandbox has no model to drive it. The PR scopes it out itself ("enforcement is … not a hard gate in compose-review"). Verified instead: the demand text is present where the demotion reads from (brief A/B), the SKILL.md rule is internally consistent, and the mechanical consequence it relies on (low-confidence never posts) is existing compose-review behavior, cited by code reading only — I did not execute a compose run.
- Terminal report and inline comment bodies quoting the witness — composed by the orchestrator model from the artifact (SKILL.md Steps 5/7), no code path to exercise. What was proven is the data half: the string survives in the artifact for that copy (
01harness, hop 2). - Repo-wide gates — only the affected surfaces ran:
findings.test.ts+agent-prompt.test.ts(317/317), fullsrc/commands/review(2425 pass / 4 env skips), coreSKILL.test.ts(8/8), renderer test (19/19). Typechecking is covered by the pre-run build at HEAD (env contract). The repo-wide suite and a live end-to-end/reviewrun were not in scope. - Multi-commit attribution — N/A:
git rev-list HEAD^1..HEAD^2= 1, matching the metadata snapshot's single-commit array (8c4df33…); shallow depth 2 acknowledged.
Methodology
Environment: the CI verify container (node:22-bookworm), detached merge-ref checkout at 50ce56f (depth 2), npm ci + npm run build completed at HEAD before the clock started; no GitHub token, no writes. Base arm: git worktree add tmp/base-tree HEAD^1 proved too costly (project-reference type resolution failed on unrelated upstream modules), so the control was built per the cheaper recipe — a byte copy of head's packages/cli/dist with exactly the two changed modules recompiled from git show HEAD^1:… sources via esbuild; diff -rq confirmed the two .js files as the only behavioral difference (their stale .d.ts/.map are head versions and inert at runtime). Workspace-link confound ruled out: neither module imports @qwen-code/* at runtime (only node builtins and relative imports — grepped), and the PR makes no core code changes (two .md assets only), so nothing the harness loads differs between trees beyond the hunks under test. Harnesses drove the real compiled CLI end-to-end (yargs handlers, real file writes, real stderr), never the unit under test through stubs; the renderer probe bundled the real CodeReviewArtifactDetail.tsx (esbuild, css-loader emptied) and fed it the shipped contract fixture. Mutations were applied surgically to the HEAD tree, run, and restored from git (git status clean after); expectations were declared before each run and scored mechanically from the logs (05-score-mutations.mjs). All harness scripts, per-cell logs, mutation logs, and the ANSI-stripped scoring live beside this report; terminal captures were produced with scripts/verify-capture.mjs. Base worktree removed after cell capture.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Re-run on the new head —
Moving on to code review. 🔍 中文说明基于新 head 的重跑——
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline for the commit landed since the last pass: No Criticals found on the full diff at this head. What I checked on the new test against the base tree, not just its own comments:
Everything reviewed at the prior heads carries over unchanged and was re-confirmed against this head's diff: the TestingUnattended CI run — no PR code was built or executed here; the evidence is the PR's own CI on the reviewed head, quoted from the checks API. CI on this head is still running: at time of writing, Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Evidence scope, stated plainly: with the new wiring test, every mechanical half of this PR is now pinned by a test that fails if its production code is removed — the demotion predicate and its handler call site ( 中文说明代码审查先写独立方案（针对上一轮之后落地的提交）： 在此 head 的完整 diff 上未发现 Critical。对新测试的核查是对照基线树做的，而非只看它自己的注释：
此前各 head 审查过的内容原样保留，并已在此 head 的 diff 上复核： 测试无人值守 CI 运行——未构建或执行任何 PR 代码；证据为该 head 上 PR 自身 CI 的检查 API 引文。该 head 的 CI 仍在运行：撰写时 证据边界如实说明：加上新的接线测试后，本 PR 的每个机械半边现在都有"删掉其生产代码即红"的测试钉住——降级谓词及其 handler 调用点（ — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — the new commit closes the last coverage gap a reviewer could point at, everything else carries over from heads this PR landed fully green on, and the standing reservation is unchanged: the prose half is measured only by dogfood, which no CI job can pin. Reflection at the new head: The push that dismissed the deferred approval is a 24-line test — the kind of re-review a gate should be cheap to give. Its claim is precise: the unit tests pinned Verdict: approve — held only on CI. Two Approval deferred until CI lands green on 中文说明Confidence: 4/5 —— 新提交补上了审查者还能指出的最后一个覆盖缺口，其余部分原样继承自本 PR 曾全绿落地的各 head；一贯的保留意见也不变：文案半边只能靠 dogfood 度量，任何 CI 任务都钉不住它。 对新 head 的反思： 让延迟批准失效的推送是一个 24 行的测试——正是门槛应当廉价给予的那种复审。它的诉求很精确：单测此前只在孤立状态下钉住 结论：批准——仅受 CI 约束。此 head 上还有两个 批准延迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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. Suggestions are inline.
Not explored to full depth (tool budget reached): "PR #9065 (QwenLM/qwen-code) adds a \"witness\" requirement…": none — all checks above completed within budget..
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："PR #9065 (QwenLM/qwen-code) adds a \"witness\" requirement…"：none — all checks above completed within budget.。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // `witness` round-trips for the same reason `outcomeNote` does: the Step 4 | ||
| // witness rule attaches it once, and the report and the comment bodies read | ||
| // it back out of the artifact instead of transcribing the evidence again. | ||
| const witness = asString(o, 'witness'); |
There was a problem hiding this comment.
[Suggestion] R1-1: The witness demotion is promised as mechanical — SKILL.md: "deliberately mechanical, the same shape as the — [unverified] tag"; this file's verify brief: "This is mechanical downstream"; DESIGN.md: "the enforcement shape is borrowed from the — [unverified] tag" — but no code anywhere reads witness: the sort exists only as Step 4 prose for the orchestrating model. The precedent being borrowed HAS a machine half (compose-review scans the findings file for surviving — [unverified] tags, caps the verdict, and posts a disclosure count) that this change does not carry over; validateFindings defaults an omitted confidence to high — the fail-open direction for this rule — and this same command already mechanically demotes Criticals via the test-delta holdback, so the pattern is local. — Failure scenario: a verifier confirms a Critical and argues in prose instead of returning a witness: / not run — line; the orchestrator, sorting findings against a ~1300-line SKILL.md, misses the demotion (or omits confidence, silently defaulted to high); nothing between Step 4 and Step 7 inspects witness, so the unwitnessed Critical posts as a blocker without executed evidence — the exact failure mode this PR exists to prevent — with no count or telemetry showing whether the sort ran. Fix: enforce at the posting boundary — compose-review (beside findings-unverified-at-compose) or the findings command counts/demotes witness-less high-confidence Criticals — or soften the "mechanical" / "borrowed enforcement shape" wording to name the orchestrator sort as the only enforcer.
中文说明
[建议] witness 降级被承诺为机械执行——SKILL.md："deliberately mechanical, the same shape as the — [unverified] tag"；verify brief："This is mechanical downstream"；DESIGN.md："the enforcement shape is borrowed from the — [unverified] tag"——但没有任何代码读取 witness：这个分检只存在于编排模型执行的 Step 4 文字里。被借用的先例有机器的一半（compose-review 会扫描 findings 文件中残留的 — [unverified] 标记、封顶裁决并发布披露计数），本改动没有带上这一半；validateFindings 把缺省的 confidence 默认为 high——对此规则是 fail-open 的方向——而同一个命令已经通过 test-delta 抑制机械地降级过 Critical，模式就在本地。失败场景：verifier 确认了一个 Critical 却用文字论证、没有返回 witness: / not run — 行；编排器在约 1300 行的 SKILL.md 背景下做分检时漏掉了降级（或没写 confidence，被静默默认为 high）；Step 4 到 Step 7 之间没有任何东西检查 witness，于是没有实测证据的 Critical 以 blocker 身份发布——正是本 PR 要防止的失效模式——且没有任何计数或遥测显示分检是否执行过。修复：在发布边界强制——compose-review（在 findings-unverified-at-compose 旁边）或 findings 命令统计并降级缺少 witness 的高置信 Critical；或者把 "mechanical" / "borrowed enforcement shape" 的措辞弱化为"编排器分检是唯一执行者"。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 4855a8a, at the boundary you named as local precedent: qwen review findings now runs holdUnwitnessedCriticals beside the test-delta holdback — a high-confidence [review]-source Critical with no witness is demoted to low confidence at canonicalization, each named on stderr, and the appended sentence tells the reader which rule moved it and the way back. Deterministic sources are exempt (their witness is constitutive), and the hold is idempotent on re-feed. SKILL.md and DESIGN.md now describe the machine half instead of only promising one; a compose-review-level cap stays follow-up.（已修：findings 规范化处代码强制降级，逐条 stderr 披露；确定性来源豁免；重复喂入幂等。）
| - **confirmed (low confidence)** — the mechanism is real but the trigger is uncertain (timing, environment, configuration). Say what would confirm it. Carry the severity. | ||
| - **rejected** — the code does not do what the finding claims (**quote the contradicting code**), or it matches an Exclusion Criterion (one-line reason). | ||
|
|
||
| **A confirmed Critical returns its witness.** Alongside the verdict, include a \`witness:\` line quoting the observed output that settled it — the probe's two sides, the A/B's \`BASE:\`/\`PR:\` pair, the extracted step's run, the sweep count — trimmed to the deciding lines. When every run-capability above is genuinely inapplicable and the confirmation rests on the trace alone, write the one line \`witness: not run — <why no run could settle this claim>\` instead; writing that line is also the moment you notice when the claim was runnable after all. This is mechanical downstream: a confirmed Critical returning neither the witness nor the reason line is filed at **low confidence** — terminal-only, never posted — whatever your prose argued, because the evidence a run produced is the one part of a Critical its author can act on without re-deriving the bug. |
There was a problem hiding this comment.
[Suggestion] R1-1 (second anchor of the same finding — the first is on findings.ts): this paragraph promises every verifier "This is mechanical downstream: a confirmed Critical returning neither the witness nor the reason line is filed at low confidence — terminal-only, never posted", but there is no machine half: no code reads witness, the sort is Step 4 prose applied by the orchestrating model, and validateFindings defaults an omitted confidence to high — so a missed sort or an omitted field fails open and the unwitnessed Critical posts, with no signal that the demotion was skipped. Either give the rule its machine half (count/demote witness-less high-confidence Criticals in compose-review or the findings command, mirroring the — [unverified] cap this rule's design cites) or soften "This is mechanical downstream" to name the orchestrator sort as the only enforcer. — Failure scenario: see the sister comment on findings.ts; the harm is identical, this is where the promise is made to the verifier.
中文说明
[建议] R1-1（同一发现的第二个锚点，第一个在 findings.ts）：这段向每个 verifier 承诺 "This is mechanical downstream: …… terminal-only, never posted"，但不存在机器的一半：没有任何代码读取 witness，分检是编排模型执行的 Step 4 文字，且 validateFindings 把缺省的 confidence 默认为 high——因此漏掉的分检或缺省的字段会 fail-open，没有 witness 的 Critical 照样发布，且没有任何信号表明降级被跳过。要么给这条规则补上机器的一半（在 compose-review 或 findings 命令中统计并降级缺少 witness 的高置信 Critical，对照本规则设计所引用的 — [unverified] 封顶机制），要么把 "This is mechanical downstream" 弱化为"编排器分检是唯一执行者"。失败场景：见 findings.ts 上的姊妹评论；危害相同，这里是向 verifier 作出承诺的位置。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Same fix as the sister comment — 4855a8a gives the promise its machine half at the findings canonicalization, and this brief line now says so explicitly ('enforced in code at the findings canonicalization, not merely by the orchestrator's read of its rules'), so the verifier is told the truth about who enforces it.（同姊妹评论的修复；brief 措辞已改为如实描述执行者。）
| 5. **Witness** — for a Critical: the observed output that settled the verdict, trimmed to the deciding lines — the probe's two sides, the A/B's quote pair, the sweep count over the real population, the failing test text — or the verifier's `not run — <reason>` line (Step 4's witness rule). A Suggestion carries one when a run produced it; it is not owed one. | ||
| 6. **Suggested fix** — Concrete code suggestion when possible | ||
|
|
||
| For **pattern-aggregated** findings, use the aggregated format from Step 4 (Pattern, Occurrences, Example, Failure scenario, Suggested fix, Severity) with the source tag added. |
There was a problem hiding this comment.
[Suggestion] R1-2: The individual-finding format one paragraph above gained a Witness slot (new item 5), but this pattern-aggregated format — mandated for the same report — did not: the closed enumeration (Pattern, Occurrences, Example, Failure scenario, Suggested fix, Severity) has no witness line. That contradicts Step 4's own promise this PR adds ("The witness rides the finding from here on — into the findings artifact, the terminal report, and ... the inline comment body"), while Step 4's Pattern aggregation explicitly anticipates Critical pattern groups ("For Critical patterns, always list all locations") and groups only same-confidence findings — a high-confidence Critical aggregate is an expected case. — Failure scenario: several confirmed Criticals of the same type (one probe or sweep confirming a pattern — this PR's flagship "195 of 434 real run: bodies" form) get aggregated; the orchestrator renders the enumerated fields with no witness line, so the executed evidence is silently absent from the terminal report for exactly the severity the rule targets — or it improvises a slot, the ad-hoc transcription drift the witness artifact field exists to prevent. Fix: add a Witness line to the Step 4 aggregated template (the representative instance's witness, or the group's shared not run — <reason> line) and add Witness to this enumeration — the fix spans both locations, so no one-click suggestion block.
中文说明
[建议] R1-2：上一段的单条发现格式新增了 Witness 槽位（新的第 5 项），但同一个报告所强制使用的这个 pattern 聚合格式却没有：封闭的字段列表（Pattern、Occurrences、Example、Failure scenario、Suggested fix、Severity）没有 witness 行。这与本 PR 新增的 Step 4 承诺（"The witness rides the finding from here on — into the findings artifact, the terminal report, and ... the inline comment body"）矛盾；而 Step 4 的 Pattern 聚合明确预期 Critical 模式组（"For Critical patterns, always list all locations"）且只聚合相同置信度的发现——高置信 Critical 聚合是预期中的情况。失败场景：多个同类型已确认 Critical（一个 probe 或 sweep 确认了一个模式——本 PR 的旗舰形态 "195 of 434 real run: bodies"）被聚合；编排器按枚举字段渲染、没有 witness 行，于是实测证据在该规则针对的严重级别上从终端报告中静默缺失——或者编排器临时加一个槽位，即 witness 工件字段本要防止的随手转录漂移。修复：给 Step 4 的聚合模板加一行 Witness（代表性实例的 witness，或该组共享的 not run — <reason> 行），并把 Witness 加入此处的枚举——修复横跨两处，故不提供一键 suggestion 块。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 4855a8a, both locations: the Step 4 aggregated template gains a Witness line (the representative instance's witness — often the one sweep or probe that confirmed the whole pattern — or the group's shared not run — <reason> line), and the Step 6 enumeration now reads (Pattern, Occurrences, Example, Failure scenario, Witness, Suggested fix, Severity).（两处已同步补上 Witness 槽位。）
| ? compressSummary(shortSummary) | ||
| : compressSummary(summary), | ||
| failureScenario, | ||
| ...(witness ? { witness } : {}), |
There was a problem hiding this comment.
[Suggestion] R1-3: The Web Shell artifact renderer — the documented second consumer of this artifact shape (CodeReviewArtifactDetail.tsx) — parses every other optional evidence field (suggestedFix, category, outcomeNote, heldByMeasurement) but never reads witness, silently dropping the executed evidence from display. Witness (probe, flipped): with the unmodified PR code, a finding carrying witness: 'BASE: 2 calls / PR: 1 call — probe flipped' parses to keys ["confidence","failureScenario","id","locations","severity","shortSummary","source","summary"] — witness silently dropped, parse succeeds; adding the field to parseFinding round-trips it byte-for-byte. — Failure scenario: a review run that does exactly what this PR intends — attaches executed evidence to every confirmed Critical — renders in the Web Shell UI with the evidence absent: the author sees the Critical claim and failure scenario but not the probe flip / A/B pair / sweep count the field exists to hand them, and the gap is invisible because parsing succeeds. The PR's "the Web Shell renderer is unaffected" claim answers "does it break" (no) but not "does it show the field this PR exists to deliver" (no). Fix: add witness to parseFinding in packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx (an optionalString like outcomeNote) and render it with the finding's expandable fields — or document it as deliberately deferred.
中文说明
[建议] R1-3：Web Shell 工件渲染器——该工件格式的、有文档记载的第二个消费者（CodeReviewArtifactDetail.tsx）——解析了其他每一个可选证据字段（suggestedFix、category、outcomeNote、heldByMeasurement），却从不读取 witness，实测证据被静默丢弃、不显示。Witness（probe，已翻转）：在未改动的 PR 代码上，携带 witness: 'BASE: 2 calls / PR: 1 call — probe flipped' 的 finding 解析出的键为 ["confidence","failureScenario","id","locations","severity","shortSummary","source","summary"]——witness 被静默丢弃且解析成功；把该字段加入 parseFinding 后可逐字节往返。失败场景：一次完全按本 PR 意图执行的审查——给每个已确认 Critical 附上实测证据——在 Web Shell UI 中渲染时证据缺席：作者看到 Critical 主张和失败场景，却看不到该字段本要交付的 probe 翻转 / A/B 引文对 / sweep 计数，而且因为解析成功，这个缺口不可见。PR 中 "Web Shell 渲染器不受影响" 的说法回答的是"会不会坏"（不会），而不是"会不会显示本 PR 存在的意义所交付的字段"（不会）。修复：在 packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx 的 parseFinding 中加入 witness（像 outcomeNote 一样的 optionalString）并随 finding 的可展开字段渲染——或明确记录为刻意推迟。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 4855a8a: parseFinding carries witness (an optionalString, exactly the outcomeNote shape your probe suggested) and the finding card renders it between Failure scenario and Suggested fix, with en/zh labels (Witness / 实测证据). Pinned by the renderer test — the fixture's Critical now carries a witness and the assertions require it displayed. Thanks for running the probe; 'parses successfully while dropping the one field the PR exists to deliver' is exactly the class of gap that hides behind a green parse.（已修：解析+渲染+中英文标签，渲染测试钉住显示。）
…finding travels Three findings from this PR's own dogfood review round: - The demotion was promised as mechanical but no code read witness, and validateFindings defaults an omitted confidence to high — the fail-open direction. qwen review findings now demotes any high-confidence [review]-source Critical that arrives without the field to low confidence at canonicalization, naming each on stderr — the same place and shape as the test-delta holdback. Deterministic sources are exempt: a [build]/[test]/[probe] finding IS a run's output. Idempotent by construction. - The pattern-aggregated format had no witness slot, so the executed evidence would vanish (or be improvised) exactly when one sweep confirms a whole Critical pattern. The Step 4 aggregate template and Step 6's enumeration gain the Witness line. - The Web Shell renderer parsed every other optional evidence field and silently dropped witness from display. parseFinding carries it and the finding card renders it, with en/zh labels.
|
@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: 96 passed · 0 failed · 96 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：96 通过 · 0 失败 · 96 总计 Verification reportPR 9065 deep verification (round 2) —
|
| # | Round-1 finding | Sev. | Status at 4855a8a |
|---|---|---|---|
| 1 | Mutation survivor (unconditional witness key) adjudicated a dead axis |
Info | Stands, re-measured. The demotion added this round reads f.witness !== undefined, which cannot distinguish key-absent from key-present-with-undefined; JSON.stringify drops undefined; a fresh census found the three read sites (findings.ts demotion, renderer finding.witness &&, renderer optionalString(source['witness'])) are all value reads, and no key-presence read ('witness' in) exists anywhere in packages/. Re-run as M7: survives 89/89 again. Adjudication unchanged. |
| 2 | Renderer parses witness but does not display it |
Info | Fixed. The new commit adds the Detail block; probe 04-renderer-witness-displayed.png renders the REAL bundled component under jsdom: en label Witness + verbatim value shown (exactly once, only on the injected finding), zh-CN label 实测证据, and the unmodified contract fixture (no witness) renders with no label — old artifacts unchanged. Renderer suite 19/19, now asserting display (test comment cites "PR 9065 review R1-3"). |
| 3 | Suite reconciliation: 2428/2429, stale-bundle.test.ts failure environmental |
Info | Re-measured, green. Full src/commands/review at the new head: 2429 passed, 4 skipped, 0 failed (68 files) — round 1 was 2425; the +4 are this PR's new demotion tests. The 4 skips are byte-identical environmental ones (3× script-lint shellcheck — no binary in container; 1× save-artifact case-insensitive-alias — Linux FS). The stale-bundle failure again did not reproduce, consistent with the author's stale-local-build attribution. |
| 4 | No length cap on witness |
Info | Stands. No cap added this round; sibling free-text fields remain uncapped (only summary gets compressSummary). Named again so the asymmetry stays conscious. |
Central claim and A/B proof
The round-2 delta's central claim: the witness rule has a machine half — qwen review findings demotes any high-confidence [review]-source Critical arriving without a witness field to low confidence at canonicalization, names each on stderr, exempts deterministic sources, and is idempotent. Secondary: the witness round-trip (round 1's central claim, re-measured because findings.ts changed) and the verify brief's demands (agent-briefs.ts changed).
A/B cell table — demotion + round-trip (harness 01-demotion-ab.mjs, witness 01-ab-demotion-head-vs-control.png)
Both arms drive the real compiled CLI (node <dist>/index.js review findings --input … --out … --print), twice: fresh input, then the written artifact fed back through --input. Fixture: 8 findings probing all four predicate guards plus parse boundaries.
| cell | oracle | head | base (control) |
|---|---|---|---|
| no-witness review Critical demoted | w-none.confidence === 'low', appended sentence present, original scenario survives |
✅ | ❌ stays high (expected red) |
| each demotion named on stderr | exact findings: <id> filed at low confidence — … line for w-none, w-defaults, w-ws |
3/3 | 0/3 (expected) |
| omitted confidence/source fail toward the rule | w-defaults (defaults high/review) demoted |
✅ | ❌ not demoted (expected red) |
| whitespace-only witness is no witness | w-ws demoted, no witness key carried |
✅ | ❌ not demoted (expected red) |
| executed witness keeps it high | w-exec high, witness byte-exact |
✅ | ❌ witness dropped (expected red) |
not run — form counts as a witness |
w-notrun high, witness byte-exact |
✅ | ❌ dropped (expected red) |
| deterministic source exempt | w-testsrc high, untouched, no stderr line |
✅ | ✅ (vacuous on base) |
| Suggestion never judged | w-sugg high |
✅ | ✅ |
| already-low untouched | w-alreadylow low, failureScenario unchanged, no stderr |
✅ | ✅ |
| counts | stderr summary 7 Critical, 1 Suggestion; 4 low-confidence (head) vs 1 low-confidence (base) |
✅ | ✅ |
| hop 2 idempotence | w-none still low, failureScenario byte-identical to hop 1, zero re-disclosures |
✅ | — |
| hop 2 round-trip | both witnesses survive the second --input hop |
✅ | ❌ still absent (expected) |
| bystander parity | w-testsrc/w-sugg byte-identical across arms; w-exec identical except witness/confidence |
✅ | ✅ |
42/42 assertions. 5/5 demotion-shape cells flip between head and control is the load-bearing pair; bystander parity shows nothing else moved. Interaction checked by reading + M1: the witness hold runs after the test-delta holdback, and a test-delta-held finding is a Suggestion by then, so the severity guard keeps the two disclosures from double-firing.
A/B cell table — verify brief (harness 02-brief-ab.mjs, witness 02-ab-verify-brief-head-vs-control.png)
Both arms drive the real handler (review agent-prompt --plan … --role verify --findings …) and read the brief file it writes.
| cell | oracle | head | base (control) |
|---|---|---|---|
| three witness demands present | A confirmed Critical returns its witness. / witness: not run — / sweep the real population |
3/3 | 0/3 (expected) |
| code-enforcement statement (round-2 addition) | enforced in code at the findings canonicalization |
✅ | ❌ absent (expected) |
| brief generation unbroken | What is NOT a finding present, exit 0, exactly one brief file |
✅ | ✅ |
15/15 assertions. The brief's new enforcement statement matches the code (harness 1 proves the demotion exists at exactly that canonicalization point) — the two ends of the capability agree.
Vacuity + mutation matrix (harness 03-mutations.mjs, witness 03-mutation-matrix.png)
Baseline unmutated: 89/89 green. Each mutant applied surgically to findings.ts, full suite run, source restored byte-identical from git (verified per mutant).
| mutant | target | expected | result |
|---|---|---|---|
| V1 | revert witness spread in validateFindings |
kill | killed — keeps witness… red at the intended assertion: expected undefined to be 'BASE: 2 calls / PR: 1 call — probe fl…' |
| V2 | disable holdUnwitnessedCriticals (pass-through) |
kill | killed — files an unwitnessed… red: expected 'high' to be 'low' |
| M1 | drop severity guard | kill | killed — is idempotent (Suggestion clause) |
| M2 | drop confidence guard | kill | killed — is idempotent (double-append breaks toEqual) |
| M3 | drop source guard | kill | killed — exempts deterministic sources |
| M4 | drop witness guard | kill | killed — leaves a witnessed Critical alone (expected 'low' to be 'high') |
| M5 | delete the stderr disclosure loop | survive | survived 89/89 — adjudicated below |
| M6 | rewrite appended sentence (witness rule → witness-rule) |
kill | killed — toContain('witness rule') |
| M7 | unconditional witness key (round-1 M2) |
survive | survived 89/89 — dead axis, re-adjudicated above |
| P1 | flip confidence default to low |
kill | killed — 3 tests red (positive control: the suite is falsifiable) |
29/29 scoring assertions. Every guard the PR introduces is pinned by exactly one intended test; no mutant regressed a pinned behavior.
Corrections
None.
Findings
No blockers, no defects. Informational items only:
- (Informational) M5 survivor: the stderr disclosure is behavior, not pinned coverage. Deleting the handler's
for (const id of witnessHold.unwitnessed)loop leaves the entire 89-test suite green. Classification: coverage gap, not dead code and not a defect — the disclosure is load-bearing for trust (a demotion nobody is told about reads as the reviewer's own judgement, per the code's own comment), and harness 1 proves the behavior correct on both arms (3 exact lines present on head, zero on base, none re-disclosed on hop 2). The suite pins the demotion function but nothing at the handler level exercises the witness rule's stderr. A fixture that would pin it: afindings (command boundary)case feeding one unwitnessed Critical through the handler and asserting the stderr line — it ships here only as a suggestion; the behavior itself is verified. Completeness reporting, not a merge condition. - (Informational) Renderer strictness asymmetry on a whitespace witness — unreachable. The renderer's
optionalString/stringthrows on a whitespace-only value where the CLI'sasStringsilently drops it. No input can reach the divergence: the only writer of these artifacts isvalidateFindings, which normalizes whitespace-only witnesses to absent before writing, and the renderer's fail-closed style is identical for every sibling field (failureScenario,suggestedFix) — pre-existing parse posture, not new surface. Named so the boundary is a known one. - (Informational, carried) No length cap on
witness— see status table row 4. - (Informational) Round-1 digest aside re-measured: the record-key digest is
verify--0f663a427ce1on all four harness-2 runs — head and control alike, even though their brief texts differ — confirming it still keys on findings/plan, not brief text. Pre-existing behavior, unchanged by this PR, no action.
Consistency checks that passed and are worth a line each: Step 4's aggregate template carries the Witness line between Failure scenario and Suggested fix, matching Step 6's enumeration order exactly; all four DESIGN.md headings cited by the new SKILL.md text exist verbatim (The read-only claim retracted in round 2 (PR #8225), The mirrored oracle's false positives (PR #8225), The double-execute the probe caught, plus the new decision narrative Why a confirmed Critical carries a witness); the renumbered finding format (5 → Witness, 6 → Suggested fix) leaves no stale numbered references; ESLint clean on all 7 changed TS files with a live control (a planted any + unused-var probe was caught by the same invocation); web-shell renderer's hand-duplicated vocabulary lists remain in sync (its suite is green).
Not covered
- Model compliance with the witness rule — that a verifier actually emits the witness/
not run —line and that the orchestrator applies the Step 4 sort. Prompt-guided model behavior; the sandbox has no model. Verified instead: the demand text is in the brief the verifier reads (harness 2), the code backstop catches whatever the sort misses (harness 1), and the brief's enforcement statement matches the code. - Terminal report and inline comment bodies quoting the witness — composed by the orchestrator from the artifact (SKILL.md Steps 5/7); no code path to exercise. Proven half: the string survives the artifact across hops (harness 1, hop 2).
- Low-confidence-never-posts at compose time — the demotion's downstream consequence rides the pre-existing orchestrator rule (low-confidence findings are terminal-only;
buildLedger's comment documents it) rather than a code gate incompose-review. Verified by code reading only; no compose run executed. This is the one link in the chain enforced by prompt rather than code, and it predates the PR. - Repo-wide gates — only affected surfaces ran: cli two-file suite (321/321), full
src/commands/review(2429 + 4 env skips), coreSKILL.test.ts(8/8), renderer suite (19/19), ESLint on changed files. Typechecking is covered by the pre-run build at HEAD. Repo-wide suite and a live end-to-end/reviewrun were out of scope. - Per-commit attribution — the metadata snapshot lists two commits (
8c4df33,4855a8a) but the checkout is depth 2:git rev-list HEAD^1..HEAD^2returns 1 and8c4df33is unreachable (git cat-filefails). This is the shallow-boundary artifact, not a real count — so the aggregateHEAD^1..HEADdiff was verified and per-commit separation (what round 1's head had vs. what commit 2 added) is reported from the commit messages and the previous report, not from an exercised diff. - Base drift — main advanced from
52cfb18(snapshotbaseRefOid) to7f458b4(localHEAD^1) between rounds; the merge applied cleanly and the A/B control uses the local base tip per the merge-ref contract. No file this PR touches was changed by the intervening main commits in a conflicting way (merge succeeded).
Methodology
Environment: the CI verify container (node:22-bookworm, node v22.23.2), detached merge-ref checkout cdd6639 at depth 2, npm ci + npm run build completed at HEAD before the clock; no GitHub token, no writes to GitHub. Base arm: a byte copy of head's packages/cli/dist with exactly the two changed modules (findings.js, lib/agent-briefs.js) recompiled from git show HEAD^1:… sources via esbuild; diff -rq confirmed those two .js files as the only difference (stale .d.ts/.map are head versions, inert at runtime). Workspace-link confound ruled out up front: neither changed module imports @qwen-code/* at runtime (grep of their import lists — node builtins and relative paths only), and the PR's core-package changes are two .md assets. Harnesses drove the real compiled CLI end-to-end (yargs handlers, real file writes, real stderr) and the real bundled renderer component under jsdom (esbuild, CSS loaders emptied, workspaceActions.readWorkspaceFile the only mock — matching the suite's own seam); per-cell logs live in logs/. Mutations were applied to the HEAD tree with exact single-occurrence string replacements (occurrence count asserted per mutant), run against the full findings.test.ts with the JSON reporter, and restored via git checkout with a byte-identity check after each; git status ended clean. Evidence images were produced with scripts/verify-capture.mjs from live re-runs of harnesses 1–4 and a replay of the saved gate logs (5).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
The visual preview reported no screenshot change for the renderer edit because no scenario reached the new UI: the code-review artifact fixture carried no witness, so the row never rendered. The Critical now carries one and the shot is gated on its visibility, so the scenario is a coverage witness for the field, not just for the card.
|
@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: 134 passed · 0 failed · 134 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：134 通过 · 0 失败 · 134 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions (including a real Playwright visual-scenario run), targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 134 passed · 0 failed · 134 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证容器中执行（与 base 构建 A/B 对照、无 mock harness、真实 Playwright 视觉场景实跑、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。脚本断言：134 通过 · 0 失败 · 134 总计。
PR 9065 deep verification (round 3) —
|
| # | Round-2 finding | Sev. | Status at 55ac5a20 |
|---|---|---|---|
| 1 | M7 survivor (unconditional witness key) adjudicated a dead axis |
Info | Stands, re-measured. Re-run as M7 in this round's matrix: survives 43/43 again. Closure compared: the aggregate HEAD^1..HEAD diff touches exactly the 9 files round 2 verified plus screenshots.spec.ts; commit 3's delta is the spec hunk alone (fixture witness: field + the gate assertion, quoted in the delta table below), so findings.ts/findings.test.ts — the matrix's input closure — are unchanged since round 2's verification. Fresh census re-confirmed the three read sites are value reads and no 'witness' in key-presence read exists in packages/. |
| 2 | M5 survivor (stderr disclosure loop) adjudicated a coverage gap | Info | Stands, re-measured. M5 survives 43/43 again; harness 01 re-proves the disclosure behavior correct on both arms (3 exact lines on head, 0 on base, zero re-disclosures on hop 2). The suggested handler-boundary fixture still ships as a suggestion only. |
| 3 | Renderer strictness asymmetry on a whitespace witness — unreachable | Info | Stands, re-derived. CodeReviewArtifactDetail.tsx and findings.ts unchanged by commit 3; harness 01's w-ws cell re-ran (whitespace witness demoted, key absent from the artifact), so no input can reach the renderer divergence. |
| 4 | No length cap on witness |
Info | Stands. No cap added; grep of findings.ts shows witness still rides asString with no compression (sibling free-text fields remain uncapped too). |
| 5 | Renderer "parses but does not display" — fixed in round 2 | — | Still fixed, now also pinned by the visual scenario. Renderer suite 19/19 at this head; and harness 04 C1 renders the witness row in a real chromium (crop 04-witness-row-crop-dark.png), while C2 shows the new spec gate fails if the display block is removed — the e2e pin round 2's fix previously lacked. |
| 6 | Record-key digest aside (keys on findings/plan, not brief text) | Info | Re-measured. Harness 02: the verify--<12hex> digest is byte-identical across head and control runs with differing brief texts. |
Central claim of the round-3 delta and A/B proof
The third commit's claim: the code-review visual scenario is now a coverage witness for the witness UI — the fixture's Critical carries a witness, and the screenshot is gated on getByText('forced a 1ms timeout') being visible, so a renderer that drops the row fails the scenario instead of shipping a silent gap. Secondary (re-measured from rounds 1–2): the demotion machine half and the witness round-trip (harness 01), the verify brief's demands (harness 02).
A/B cell table — visual scenario (harness 04-visual-scenario.mjs, witness 08-visual-scenario-ab.png, shots 05-visual-shot-head-dark.png / 06-visual-shot-base-dark.png, crop 04-witness-row-crop-dark.png)
All cells drive the REAL Playwright visual spec (-g "code review artifact", chromium, vite dev server) — C1/C2 at head, C3 in a scratch worktree at HEAD^1 (its packages/web-shell/node_modules symlinked to the head package's nested modules; the only seam needed, see Methodology).
| cell | oracle | head | base / mutant |
|---|---|---|---|
| C1 scenario passes with the gate | exit 0, 2 passed (dark+light), both screenshots captured |
✅ | — |
| C2 gate is load-bearing | renderer's witness <Detail> block removed → expect(page.getByText('forced a 1ms timeout')).toBeVisible() fails at spec line 1065, "element(s) not found" |
— | ❌ 2 failed (expected red: the round-1 R1-3 defect shape now fails the scenario) |
| C3 base never reached the new UI | base spec (no fixture witness, no gate) + base renderer | — | ✅ 2 passed, no witness row anywhere |
| C4 the shot carries the row | head vs base screenshot pixel delta | 11 023 / 1 024 000 px (1.076 %) dark; 10 858 (1.060 %) light — non-zero and localized (rows 303–799, the card body the row shifts) | 0 (itself) |
| C5 witness row inspectable | crop of the head shot shows the WITNESS label + verbatim probe text between FAILURE SCENARIO and SUGGESTED FIX |
✅ (read back from the PNG) | — |
19/19 assertions. The C2 cell is the load-bearing pair: the gate fails on exactly the defect the renderer fix closed in round 2, so the scenario now holds the display behavior the way the unit suite holds the parse. C3 + C4 confirm the commit's premise — on base nothing reached the new UI, and the only visible delta between the arms is the witness row (the bounding box spans the card body because the added row shifts the content below it; the arms differ by nothing else).
Re-measured A/B tables from rounds 1–2 (harnesses 01–03)
Harness 01 (01-demotion-ab.mjs, witness 01-ab-demotion-head-vs-control.png) — real compiled CLI, head dist vs control dist (head dist with findings.js and lib/agent-briefs.js recompiled from HEAD^1; diff -rq shows exactly those two modules + maps differ): 53/53. Demotion cells flip head/control as in round 2 (w-none/w-defaults/w-ws demoted + 3 exact stderr disclosures on head, 0 on base; w-exec/w-notrun high with byte-exact witnesses on head, dropped on base; [test] exempt; Suggestion never judged; already-low untouched; summary 8 total — 7 Critical, 1 Suggestion … 4 low-confidence head vs 1 base); hop-2 idempotence and two-hop witness round-trip hold on head and fail on base as expected; bystander parity byte-identical.
Harness 02 (02-brief-ab.mjs, witness 02-ab-verify-brief-head-vs-control.png) — real review agent-prompt --role verify handler on both arms: 19/19. The four witness passages (A confirmed Critical returns its witness. / witness: not run — / sweep the real population / enforced in code at the findings canonicalization) present 4/4 on head, 0/4 on control; brief generation unbroken on both; record-key digest identical across arms.
Harness 03 (03-mutations.mjs, witness 03-mutation-matrix.png) — surgical mutants on findings.ts, full findings.test.ts per mutant, byte-identical restore asserted each time: 43/43. Baseline 89/89; V1/V2 vacuity reverts killed at the intended assertions (expected undefined to be 'BASE: 2 calls / PR: 1 call — probe flipped'; expected 'high' to be 'low'); M1–M4 and M6 each killed by exactly one intended test; M5 and M7 survive (adjudicated above); P1 positive control kills 3 tests. No mutant regressed a pinned behavior; no test went red for a compile-breaking reason (the one such construction in my first M4 attempt was discarded and re-run with an interface-preserving mutation).
Corrections
None.
Findings
No blockers, no defects. Informational items only (all carried, statuses in the table above):
- (Informational) M5 survivor — the stderr disclosure is behavior, not pinned coverage (coverage gap; behavior verified by harness 01 on both arms).
- (Informational) M7 survivor — unconditional
witnesskey is a dead axis (no key-presence reader exists). - (Informational) No length cap on
witness(siblings uncapped; asymmetry conscious). - (Informational) Renderer
optionalStringthrows on whitespace-only values where the CLI silently drops them — unreachable divergence (writer normalizes first).
Not covered
- Model compliance with the witness rule (verifier emits the witness/
not run —line; orchestrator applies the Step 4 sort) and low-confidence-never-posts at compose time — carried from round 2, unchanged: no model in the sandbox; the code backstop (harness 01) and the brief demand (harness 02) are the verified halves. - Per-commit attribution — the snapshot lists three commits but the checkout is depth 2:
git rev-list --count HEAD^1..HEAD^2returns 1 and4855a8ais unreachable (git cat-filefails;git show HEAD^2 --statrenders the whole repo as new files — missing parent tree). The aggregateHEAD^1..HEADdiff was verified; commit 3's delta is attributed via the aggregate file list minus round-2's verified set (the spec hunk quoted above). - The rest of the visual suite — only the
code review artifactscenario ran (-g); no other scenario is touched by this PR. The fullscreen capture step inside the scenario did run (it is part of the two passing tests). - Base drift — main advanced
7f458b4 → 9d55fab(#8981, autofix budgets) between rounds; the merge applied cleanly and#8981touches none of this PR's ten files, so the control (built fromHEAD^1) and the head differ only by the PR. - Playwright browser cache vs pin (environment note, not a PR finding) — the container's pre-staged browsers (chromium revision 1208) do not match the repo's resolved
@playwright/test@1.61.1(needs 1228); I rannpx playwright install chromiumto close it. Any lane relying on the stale pre-staged cache against the current pin would fail at launch the same way my first run did. - Worktree seam for the base arm (environmental, A/A-proven) — a bare worktree at
HEAD^1cannot serve the vite app because nestedpackages/web-shell/node_modules(tailwindcss v4,@datafe-open/*) is not tracked; the first base-arm attempt failed ontailwindcss/theme.cssresolution while the identical head run passed. Symlinking the package's nestednode_modulesinto the worktree made the base arm pass 2/2 — the failure was the seam, not the base code.
Methodology
Environment: the CI verify container (node:22-bookworm, node v22.23.2, chromium 1228 installed this round), detached merge-ref checkout bd7af69c at depth 2, npm ci + npm run build completed at HEAD before the clock; no GitHub token, no writes to GitHub. Control (harnesses 01–02): byte copy of head's packages/cli/dist with exactly findings.js and lib/agent-briefs.js (+maps) recompiled from git show HEAD^1: sources via esbuild — diff -rq confirmed those as the only differences; neither changed module imports @qwen-code/* (import census: yargs, node builtins, relative paths), ruling out the workspace-symlink confound. Harness 04 drove the real Playwright visual spec against the real vite dev server: C1/C2 at head (C2 = renderer with the witness <Detail> block removed, restored byte-identical afterwards), C3 in a scratch worktree at HEAD^1 with the package's nested node_modules symlinked in; C4 pixel-compared the captured PNGs with sharp. Mutation anchors were single-occurrence (asserted), suites ran under vitest's JSON reporter, sources restored and byte-checked per mutant; git status ended clean and the worktree was removed. Input-closure comparison for carried measurements: aggregate diff = round-2's nine verified files + screenshots.spec.ts; commit 3's hunk (fixture witness: + gate) is the only delta, and all harnesses were re-run at the new head regardless. Gates: full src/commands/review 2429 passed + 4 environmental skips (3× script-lint shellcheck, 1× save-artifact case-insensitive alias — byte-identical to round 2), core SKILL.test.ts 8/8, renderer suite 19/19, web-shell tsc --noEmit clean (its include covers client/e2e), ESLint clean on all 8 changed TS/TSX files with a live control (planted any + unused var caught by the same invocation). Raw logs in logs/, harnesses at the artifact root, per-cell witnesses in evidence/ (01–03, 07, 08 rendered by scripts/verify-capture.mjs; 04–06 are the real screenshots and a crop of the diff region).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
| const witnessHold = holdUnwitnessedCriticals(findings); | ||
| findings = witnessHold.findings; |
There was a problem hiding this comment.
[Suggestion] Missing handler-level integration test for the witness rule — the holdUnwitnessedCriticals call and its stderr disclosure are exercised by unit tests, but there is no handler-level test that feeds an unwitnessed Critical through the findingsCommand pipeline and asserts the confidence is demoted to low and the stderr contains the filed at low confidence message. The existing runCapturingStderr helper and the test-delta stderr test serve as the pattern.
Failure scenario: If the holdUnwitnessedCriticals call is accidentally removed from the handler or moved past buildReport, the witness rule stops working silently — no handler test catches the regression. The existing handler tests all use a base fixture (no source/confidence fields) that defaults to review/high, so every Critical is silently demoted, but no test asserts on the resulting confidence or stderr output.
中文说明
建议 缺少 witness 规则的处理程序级别集成测试——holdUnwitnessedCriticals 调用及其 stderr 披露由单元测试覆盖，但没有任何处理程序级别测试将一个无 witness 的 Critical 通过 findingsCommand 管道输入并断言置信度被降级为 low 且 stderr 包含 filed at low confidence 消息。现有的 runCapturingStderr 辅助函数和 test-delta stderr 测试可作为模板。
失败场景：如果 holdUnwitnessedCriticals 调用被意外从处理程序中移除或被移到 buildReport 之后，witness 规则会静默停止工作——没有任何处理程序测试能捕获此回归。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Added in b6e8f34: the handler-boundary test feeds an unwitnessed Critical and a witnessed sibling through findingsCommand.handler via the existing runCapturingStderr pattern, and asserts the demoted confidence, the appended rule sentence, the 'w1 filed at low confidence' stderr line (and its absence for the witnessed one), and the low-confidence count in the report — so unwiring the call, or moving it past buildReport, fails here rather than silently.（已补 handler 级集成测试，钉住接线与 stderr 披露。）
The unit tests pin holdUnwitnessedCriticals in isolation; this pins the wiring — an unwitnessed Critical fed through the findings command comes out at low confidence with the stderr disclosure, and a witnessed sibling stays high. Removing the call from the handler, or moving it past buildReport, now fails a test instead of silently disarming the rule.
|
@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: 112 passed · 0 failed · 112 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：112 通过 · 0 失败 · 112 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) — verified head 中文 — 判定：✅ 通过 · 可合入（agent 判定）第四轮：第三轮后 PR 新增第四个纯测试提交
Verification reportPR 9065 deep verification (round 4) —
|
| # | Round-3 finding | Sev. | Status at b6e8f349 |
|---|---|---|---|
| 1 | M7 survivor (unconditional witness key) adjudicated a dead axis |
Info | Stands, re-measured. M7 survives the 90-test suite again this round. Fresh census: zero key-presence readers ('witness' in, hasOwnProperty('witness') → NONE in packages/); the three read sites are value reads (f.witness !== undefined, optionalString(source['witness'], …), finding.witness && …), and JSON.stringify drops undefined-valued keys, so an unconditional key is unobservable in the artifact. |
| 2 | M5 survivor (stderr disclosure loop) adjudicated a coverage gap | Info | Fixed by commit b6e8f349. The disclosure-silenced mutant (W3, round-3's M5 shape) is now killed by exactly the new wiring test (demotes an unwitnessed Critical through the whole handler, and says so on stderr), re-asserted strictly in harness 03b. The commit's own claim — "removing the call, or moving it past buildReport, now fails a test" — is exactly what W1/W2/W3 prove. |
| 3 | Renderer strictness asymmetry on a whitespace witness — unreachable | Info | Stands, re-derived. string() throws on value.trim() === '' while the CLI's asString silently drops whitespace witnesses; unreachable because the writer normalizes first — harness 01's w-ws cell re-proves the key is absent from the head artifact, so no input reaches the renderer divergence. |
| 4 | No length cap on witness |
Info | Stands. No cap added; witness still rides asString uncompressed (siblings suggestedFix/failureScenario remain uncapped too). |
| 5 | Renderer "parses but does display" — fixed in round 2, pinned by visual gate in round 3 | — | Still fixed, re-measured. Renderer suite 19/19 at this head; harness 04 C1 renders the WITNESS row in real chromium (read back from 06-visual-shot-head-dark.png: the row sits between FAILURE SCENARIO and SUGGESTED FIX with the verbatim probe text), and C2 shows the spec gate fails at line 1065 when the display block is removed. |
| 6 | Record-key digest aside (keys on findings/plan, not brief text) | Info | Re-measured. Harness 02: the verify--<12hex> digest is byte-identical across head and control runs whose brief texts differ (4/4 vs 0/4 witness passages). |
Central claim of the round-4 delta and its proof
Commit b6e8f349 claims the new wiring test pins holdUnwitnessedCriticals into the handler before buildReport, closing the round-3 M5 coverage gap: "Removing the call from the handler, or moving it past buildReport, now fails a test instead of silently disarming the rule." The proof is a mutation A/B across the suite (harnesses 03/03b, witness 04-wiring-vacuity-positive-control.png): baseline 90/90 green, then three single-point mutants of findings.ts, each killed by exactly the new test and nothing else.
| mutant | mutation | suite | killed by |
|---|---|---|---|
| baseline | — | 90/90 | — |
| W1 | handler call → no-op passthrough ({ findings, unwitnessed: [] }) |
89/90 | the wiring test alone |
| W2 | call moved after buildReport(findings) |
89/90 | the wiring test alone |
| W3 | disclosure loop silenced (void id;) — round-3 M5 shape |
89/90 | the wiring test alone |
| P1b | held-back disclosure substring broken (positive control) | 89/90 | the announces-every-hold test alone |
The wiring test's name matches its fixture (name-as-claim check): it feeds one unwitnessed (w1) and one witnessed (w2) Critical through the real handler and asserts w1 low + rule text + stderr disclosure, w2 high, and counts.byConfidence.low === 1 — the artifact the test reads is the file the handler wrote, so the scenario reaches the code under test, and W1's red run proves the assertions are responsive. Attribution by count: the suite grew 89 → 90 across rounds, exactly one test, the wiring test — consistent with the commit being test-only.
Vacuity note on my own harness (honesty row): the first positive control I planted (P1, mutating held back from Critical — test-delta measured → …(test-delta) — measured) survived because the mutated string still contained the asserted substring held back from Critical. That is a harness flaw, not a suite property; P1b (breaking the substring) kills exactly the intended test and is the control cited above.
Re-measured A/B tables from rounds 1–3
Harness 01 (01-demotion-ab.mjs, witness 01-ab-demotion-head-vs-control.png) — real compiled CLI (dist/index.js review findings), head dist vs control dist (head dist with exactly findings.js + lib/agent-briefs.js recompiled from HEAD^1 via esbuild; diff -rq shows only those two modules + maps differ; import census: neither module — nor stdioHelpers.js they pull in — imports @qwen-code/*, ruling out the workspace-symlink confound): 51/51. Demotion cells flip head/control exactly as in round 3: w-none/w-defaults/w-ws demoted + 3 exact stderr disclosures on head, 0 on control; w-exec/w-notrun high with byte-exact witnesses on head, dropped on control; [test] source exempt; Suggestion never judged; already-low untouched and not re-annotated; summary 8 total — 7 Critical, 1 Suggestion … 4 low-confidence head vs 1 low-confidence control; hop-2 idempotence (no re-disclosures, no double-append) and two-hop witness round-trip hold on head; bystander fields byte-identical across arms.
Harness 02 (02-brief-ab.mjs, witness 02-ab-verify-brief-head-vs-control.png) — real review agent-prompt --role verify handler on both arms: 20/20. The four witness passages (A confirmed Critical returns its witness. / witness: not run — / sweep the real population / enforced in code at the findings canonicalization) present 4/4 on head, 0/4 on control; verdict branch intact on both; launch prompt printed on both; record-key digest identical across arms.
Harness 03 (03-mutations.mjs, witness 03-mutation-matrix.png) — 12 single-point mutants on findings.ts, full 90-test suite per mutant, byte-identical restore sha-asserted each time: 26/26 harness checks. Baseline 90/90; V1/V2 vacuity reverts killed at the intended assertions (round-trip + wiring / unit demotion + wiring); M1–M4, M6 each killed by their intended unit tests (M4/M6 with the wiring test as collateral); M7 survives (adjudicated above); P1's survival is the harness flaw described above, superseded by P1b. No mutant regressed from killed to survived vs round 3; M5 flipped survived → killed — the improvement commit 4 claims.
Harness 04 (04-visual-scenario.mjs, witness 08-visual-scenario-gate-ab.png, shots 06-visual-shot-head-dark.png / 07-visual-shot-head-light.png) — real Playwright visual spec (-g "code review artifact", chromium 1228, vite dev server) at head: 6/6. C1 passes 2/2 with the witness gate intact; C2 (renderer's witness <Detail> block removed) fails at screenshots.spec.ts:1065 with element(s) not found on both themes, then restores byte-identical.
Targeted gates (witness 05-targeted-gates.png): full src/commands/review 2436 passed + 4 environmental skips (3× script-lint shellcheck, 1× save-artifact case-insensitive alias — same shape as rounds 2–3); core SKILL.test.ts 8/8; renderer suite 19/19; web-shell tsc --noEmit clean; ESLint clean on all 8 changed TS/TSX files with a live control (planted unused var + any caught by the same invocation, then restored).
Corrections
None.
Findings
No blockers, no defects. Informational items only (statuses in the table above):
- (Informational) M7 survivor — unconditional
witnesskey is a dead axis (no key-presence reader exists; re-censused this round). - (Informational) No length cap on
witness(siblings uncapped; asymmetry conscious). - (Informational) Renderer
string()throws on whitespace-only values where the CLI silently drops them — unreachable divergence (writer normalizes first; re-proven by harness 01'sw-wscell).
Not covered
- Model compliance with the witness rule (verifier emits the witness/
not run —line; orchestrator applies the Step 4 sort) and low-confidence-never-posts at compose time — carried from rounds 1–3, unchanged: no model in the sandbox; the code backstop (harness 01) and the brief demand (harness 02) are the verified halves. - Per-commit attribution — the snapshot lists four commits but the checkout is depth 2:
git rev-list HEAD^1..HEAD^2returns onlyb6e8f349, andgit cat-file -t 4855a8ad→Not a valid object name. The aggregateHEAD^1..HEADdiff was verified; commit 4's delta is attributed by content (the wiring test block is the only test content not attributable to commits 1–3) and by the suite's 89 → 90 count delta. - Visual base arm (round-3 C3/C4) — those cells proved round-3's premise (base never reached the new UI; the only pixel delta was the witness row). They are premise cells for commit 3, not measurements of commit 4, whose delta is confined to
packages/clitest code: the aggregate web-shell hunks at this head (CodeReviewArtifactDetail.tsx+11, its test +8,screenshots.spec.ts+6,i18n.tsx+2) are byte-for-byte the content rounds 2–3 verified. C1/C2 were re-run at the new head regardless. compose-review-level cap and witness obligation on Step 6 ledger rulings / open-Critical re-checks — deliberately deferred by the author to follow-ups; not code in this PR.- Repo-wide gates beyond the affected workspaces were not run (targeted gates only).
Methodology
Environment: the CI verify container (node:22-bookworm, node v22.23.2), detached merge-ref checkout 593fb39e at depth 2, npm ci + npm run build completed at HEAD before the clock; no GitHub token, no writes to GitHub. Chromium for Playwright (revision 1228, matching the pinned @playwright/test@1.61.1) was installed this round — this container shipped no pre-staged browsers. Control (harnesses 01–02): byte copy of head's packages/cli/dist with exactly findings.js and lib/agent-briefs.js (+maps) recompiled from git show HEAD^1: sources via esbuild — diff -rq confirmed those as the only differences; import census (yargs, node builtins, relative paths only) rules out the workspace-symlink confound. Harnesses 01/02 drove the real compiled CLI as subprocesses with real file I/O and real stderr; harnesses 03/03b mutated packages/cli/src/commands/review/findings.ts in place and ran the full findings.test.ts suite under vitest's JSON reporter, restoring and sha-checking the source per mutant; harness 04 drove the real Playwright visual spec against the real vite dev server, removing the renderer's witness block for C2 and restoring byte-identically. Mutation anchors were single-occurrence (asserted); git status ended clean. Raw logs in logs/, harnesses at the artifact root, per-cell witnesses in evidence/ (01–05, 08 rendered by scripts/verify-capture.mjs; 06–07 are the real scenario screenshots).
Evidence images
— Qwen Code · sandboxed verification
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the witness rule's machine half against the fail-safety criteria at head b6e8f34. Approving — the demotion rule is airtight in the directions that matter:
- Not spoofable by embedded text: detection is a structured-field read (
f.witness !== undefinedaftervalidateFindingstruthy-gatesasString(o, 'witness')); nothing scanssummary/failureScenariofor awitness:marker, so a marker embedded in quoted/finding text cannot manufacture a witness. Empty-string witness is discarded → demoted. - Honest witnesses never demoted: the check has no content matching, so every form passes — probe two-sides,
BASE:/PR:pair, extract-step run, failing build/test text, render read-back, sweep count, and thenot run — <why>line. Both forms pinned by test (leaves a witnessed Critical alone — either form of the field counts). - Machine half and skill-side sort agree: SKILL.md Step 4 ("neither witness nor one-line reason → low confidence") matches the code rule (high-confidence
review-source Critical withoutwitness→ low).SOURCESis a closed validated set, so exempting all non-reviewsources is exact, not a guess; an omittedconfidencedefaults tohighat validation, and the demotion catches exactly that fail-open path. - Disclosure preserved: demoted findings are kept in the artifact with the original
failureScenariointact plus a sentence naming the rule and the way back, each named on stderr (filed at low confidence), landing terminal-only under "Needs Human Review" — nothing deleted, nothing posted. - Scope contained: non-Critical severities, low-confidence Criticals, and deterministic sources untouched (all pinned); idempotent on
--inputre-feed (pinned). - Tests pin both directions, and every prior review finding is addressed at head: the round-1 machine-half gap (R1-1), aggregated-format Witness slot (R1-2), web-shell renderer (R1-3), and the handler-level integration test requested in the second round landed in the head commit itself — it exercises the full
findingsCommandhandler and asserts demotion, the appended sentence, stderr disclosure, and report counts.
P3 nits, non-blocking:
- A whitespace-only witness (
" ") is truthy and would count; trimming before the truthy check invalidateFindingsis a cheap hardening. - No test pins the field-vs-text distinction adversarially — a finding whose
failureScenariocontains the literal textwitness: …but no field must still demote. The property is structurally guaranteed today (no text scan exists), but such a test would catch a future regression to text-scanning. - SKILL.md/DESIGN.md list
[build]/[test]/[probe]as the exempt deterministic sources while the code (correctly) exempts all non-reviewsources including[lint]— prose could name all four.
CI: 62 check runs on head — all success/skipped except review-pr (in_progress, the review automation itself). No known-flake involvement.
…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).
…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.
…them (QwenLM#9095) * feat(review): make the reviewer close unbounded classes instead of enumerating 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. * fix(review): wire the enumeration-trap lens into 3B chunk agents and 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. * fix(review): make the class-level finding one coherent concept across 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. * fix(review): complete the class-level finding contract across all three 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. * fix(review): return the Uncoverable receipt and stop, not the review 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. * fix(review): stop supersession from retiring a proven blocker, and unbound 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. * fix(review): give the class-level finding a witness so it can post and 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 QwenLM#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). * fix(review): name the real low-confidence guard, list superseded in the 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 QwenLM#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.
|
Released in v0.21.12. |
4 participants
What this PR does
Makes the executed witness a confirmed Critical's admission ticket in
/review. A verifier that confirms a Critical must now return, alongside the verdict, the observed output that settled it — the probe's two sides, the A/B'sBASE:/PR:pair, an extract-step run, the failing build/test text, the render read-back — or a one-line reason why no run could settle the claim (witness: not run — <why>). A confirmed Critical carrying neither is mechanically sorted to low confidence: terminal-only, under "Needs Human Review", never posted to the PR — and the sort has a machine half:qwen review findingsdemotes any high-confidence[review]-source Critical that arrives without the field at canonicalization, naming each on stderr (deterministic[build]/[test]/[probe]sources are exempt — they ARE a run's output). The pattern-aggregated format carries a Witness slot, and the Web Shell artifact renderer parses and displays the field. The witness rides the finding as data — into the findings artifact (new optionalwitnessfield), the terminal report, and, on a posting run, the inline comment body — so the evidence is quoted from one recorded string instead of transcribed twice more.The PR also names a new witness form, the impact sweep: for a mechanically enumerable defect, run the check over the repo's real population and quote the count ("195 of 434 real
run:bodies reach this path"). A count of zero is the retraction that keeps a false Critical off the PR. Two guards keep a sweep evidence rather than theatre: its oracle must be an external authority (the real parser, the real tool,bash -n) — never a reimplementation of the logic under test — and a nonzero count is spot-checked by reading one hit before it is quoted.Why it's needed
The verify brief accumulated three execution capabilities (probe, A/B, extract-step), all optional, spent at the verifier's discretion. Mining the maintainer dogfood corpus (356 review sessions + 182 real-environment verification sessions, 2026-06 through 2026-08) measured what discretion produces: in the review rounds that held up, evidence-gathering was ~80% of all tool calls and every posted hard finding quoted executed output; the one round-1 claim written from a reading alone was publicly retracted in round 2 when its first measurement (a sweep of all 434 real
run:step bodies) returned zero (PR 8225). Two sweeps whose oracle reimplemented the logic under test manufactured false findings out of their own bugs — hence the external-authority guard.The demotion is deliberately mechanical — the same shape as the
— [unverified]tag: a state the orchestrator can check, not a diligence the verifier remembers to have had. A verifier that traced a genuinely unrunnable claim still posts it; the one-line reason is cheap, and writing it is exactly the moment the verifier notices when the claim was runnable after all.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/findings.test.ts src/commands/review/agent-prompt.test.ts— the new coverage:witnessround-trips throughvalidateFindings(fed back via--input, the field survives; absent staysundefined), and the verify brief pins the witness demand, thenot run —reason line, and the sweep capability — the three assertions the orchestrator's low-confidence demotion sorts on.cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts— the skill's structural checks still pass over the edited SKILL.md.agent-briefs.ts(the two inserted brief passages) andfindings.ts(interface + parse + spread, modeled onoutcomeNote).Evidence (Before & After)
N/A — prompt/docs text plus one optional data field; no UI. Before: a verifier could confirm a runnable Critical from a reading alone and nothing downstream could tell. After:
agent-prompt.test.tspins the brief's demand, and a confirmed Critical without a witness or reason line is sorted low-confidence by SKILL.md Step 4's rule.Tested on
Environment (optional)
Unit tests only (
npx vitest runinpackages/cliandpackages/core). Fullsrc/commands/reviewsuite: 2428/2429 green — the one failure isstale-bundle.test.ts, which fails on a clean checkout of this machine (stale local build), unrelated to this change.Risk & Scope
not runescape hatch keeps genuinely unrunnable claims postable./reviewrun in this PR. The demotion is code-enforced at the findings canonicalization (holdUnwitnessedCriticals, mirroring the test-delta holdback); acompose-review-level cap and extending the witness obligation to Step 6's ledger rulings and open-Critical re-checks are deliberately left to follow-ups.witnessis optional in the findings artifact; older artifacts without it validate unchanged. The Web Shell renderer now parses and displays the field (en/zh labels); artifacts without it render exactly as before.Linked Issues
Refs #7981 (hardening and capability gaps surfaced by multi-model dogfooding of /review).
中文说明
What this PR does（本 PR 做什么）
让实测证据成为
/review中 confirmed Critical 的准入门票。verifier 确认一条 Critical 时，现在必须随 verdict 一起返回裁决该结论的实际观测输出——probe 的双侧结果、A/B 的BASE:/PR:引文对、extract-step 的运行结果、失败 build/test 的原文、render 回读——或者一行说明为何无法通过运行来裁决（witness: not run — <why>）。两者皆无的 confirmed Critical 会被机械降为 low confidence：只出现在终端的 "Needs Human Review" 区，绝不发布到 PR——且该分拣有机器的一半：qwen review findings在规范化时对缺少该字段的高置信[review]源 Critical 执行降级并逐条打印到 stderr（确定性的[build]/[test]/[probe]源豁免——它们本身就是运行产物）。pattern 聚合格式带有 Witness 槽位，Web Shell 工件渲染器会解析并显示该字段。witness 作为数据随 finding 流转——进入 findings artifact（新增可选witness字段）、终端报告，以及发布运行中的行内评论正文——证据从同一条记录字符串引用，而不是再转录两次。本 PR 还命名了一种新的 witness 形态：impact sweep（影响面清点）——对可机械枚举的缺陷，把检查跑遍仓库的真实全体并引用计数（"434 个真实
run:体中 195 个会走到这条路径"）。计数为零正是把假 Critical 挡在 PR 之外的撤回依据。两条护栏使 sweep 成为证据而非表演：oracle 必须是外部权威（真实 parser、真实工具、bash -n），绝不能是被测逻辑的复刻实现；非零计数须先抽读一个命中再引用。Why it's needed（为什么需要）
verify brief 先后积累了三种执行能力（probe、A/B、extract-step），但全部是可选项，用不用取决于 verifier 的自觉。对维护者 dogfood 语料（356 个审查会话 + 182 个真实环境验证会话，2026-06 至 08）的挖掘量化了"自觉"的实际产出：在站得住的审查轮次里，取证约占全部工具调用的 80%，每一条发布的硬发现都引用了实测输出；唯一一条仅凭阅读写下的 round-1 断言在 round 2 被公开撤回——它的第一次测量（对全部 434 个真实
run:step 体的 sweep）返回了零命中（PR 8225）。两个用复刻实现当 oracle 的 sweep 曾把自己的 bug 制造成假发现——因此有外部权威护栏。降级规则刻意做成机械的——与
— [unverified]tag 同构：是编排者可检查的状态，而不是指望 verifier 记得的自觉。追踪了确实无法运行的断言的 verifier 仍可发布该发现；一行理由成本极低，而写这一行的时刻恰恰是 verifier 发现"其实可以跑"的时刻。Reviewer Test Plan（评审验证计划）
How to verify（如何验证）
cd packages/cli && npx vitest run src/commands/review/findings.test.ts src/commands/review/agent-prompt.test.ts——新增覆盖：witness经validateFindingsround-trip（通过--input喂回字段不丢；缺省保持undefined）；verify brief 钉住 witness 要求、not run —理由行与 sweep 能力——这正是编排侧 low-confidence 降级所依赖的三条断言。cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts——skill 结构检查在改动后的 SKILL.md 上仍然通过。agent-briefs.ts（插入的两段 brief）和findings.ts（接口 + 解析 + 展开，仿照outcomeNote）。Evidence (Before & After)（前后证据）
N/A——prompt/文档文本加一个可选数据字段，无 UI。之前：verifier 可以仅凭阅读确认一条可运行的 Critical，下游无从分辨。之后：
agent-prompt.test.ts钉住了 brief 的要求，且无 witness 也无理由行的 confirmed Critical 会被 SKILL.md Step 4 的规则降为 low confidence。Tested on（测试平台）
Environment（环境，可选）
仅单元测试（在
packages/cli和packages/core中npx vitest run）。src/commands/review全套件：2428/2429 通过——唯一失败是stale-bundle.test.ts，在本机干净检出上同样失败（本地陈旧构建），与本次改动无关。Risk & Scope（风险与范围）
not run逃生口保证确实无法运行的断言仍可发布。/review实跑。降级已在 findings 规范化处代码强制（holdUnwitnessedCriticals，镜像 test-delta holdback）；compose-review层的封顶、以及把 witness 义务扩展到 Step 6 的 ledger 核销与 open-Critical re-check，刻意留给后续 PR。witness为可选字段，旧 artifact 不受影响。Web Shell 渲染器现在会解析并显示该字段（中英文标签）；不带该字段的旧 artifact 渲染与之前完全一致。Linked Issues（关联 issue）
Refs #7981（多模型 dogfooding 暴露的 /review hardening 与能力缺口）。