fix(review): harden the pipeline against four live-run failures - #9086
Conversation
Measured on three parallel PR reviews (qwen3.8-max, 2026-08-13, PRs #9013/#9014/#9045) run via `qwen review run`: - run.ts: pin the composed-verdict and report scans to the run's own target. The generic newest-composed scan captured a concurrent run's artifact — two of the three runs republished a neighbour PR's verdict (one reported REQUEST_CHANGES for a review whose own report said Comment). Also keep re-reading while the child runs: a coverage re-check legitimately recomposed a verdict 12 minutes after the first write, and the first-snapshot capture would republish the superseded one. - budget.ts: drop placeholder gaps whose completion word carries a trailing budget adverbial. Three "none — all checks … completed within budget" non-answers reached two posted bodies because the completion idiom required the completion word to end the text. - coverage.ts: label a non-chunk agent by the brief codename found anywhere in its launch prompt. Launchers prepend context lines, so the first-line label gave twelve finders one shared PR-summary sentence, and every budget-gap disclosure rendered as the same truncated PR quote instead of a name. - copy_bundle_assets.js: emit dist/cli.js with a shebang and the execute bit. shellContextEnv blanks a QWEN_CODE_CLI a POSIX shell cannot exec, so every review subcommand issued from a session launched off the bundle silently fell back to the PATH's global install — all three runs executed the machine's auto-updated release instead of the tree they were launched from.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@wenshao The change itself reads as well-evidenced, but the PR body doesn't follow the PR template — only What this PR does is present. Missing sections:
Why it's needed— the motivation is currently woven into the four fix descriptions; a short standalone section is enoughReviewer Test PlanwithHow to verify,Evidence (Before & After), and theTested onOS tableRisk & ScopeLinked Issues- the Chinese
<details>translation
Your recent PRs (#9028, #9058) follow the full bilingual template, so this looks like an oversight rather than a deliberate skip. Please restructure the existing content under the template headings — the material is already there, nothing new needs writing — and the gate will re-run.
中文说明
@wenshao 改动本身证据充分，但 PR 正文没有遵循 PR 模板——目前只有 What this PR does。缺少的章节：
Why it's needed——动机目前散落在四个 fix 的描述里，单独写一小节即可Reviewer Test Plan，含How to verify、Evidence (Before & After)和Tested on系统表格Risk & ScopeLinked Issues- 中文
<details>翻译
你最近的 PR（#9028、#9058）都遵循了完整的双语模板，这次看起来是疏忽而非有意省略。请把已有内容重组到模板标题下——素材已经齐备，不需要新写内容——之后 gate 会重新运行。
— 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. |
The backtick-collapse fixture's first line was itself the brief codename shape, so the new codename extraction labels it `agent security` and the first-line assertion no longer holds. Keep the sanitization intent on a prose-only first line, and pin the codename behaviour — a prepended context line must not reintroduce the shared-PR-quote label — as its own case.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "PR #9086 fixes four live-observed defects in the /review…": none — all checks above completed within budget.; "PR #9086 fixes four live-observed defects in the /review…": repo-wide typecheck/lint not run (only the touched vitest suites).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget..
中文说明
未探索到全部深度（达到工具调用预算）："PR #9086 fixes four live-observed defects in the /review…"：none — all checks above completed within budget.；"PR #9086 fixes four live-observed defects in the /review…"：repo-wide typecheck/lint not run (only the touched vitest suites).；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| if (!target) return null; | ||
| if (/^\d+$/.test(target)) return target; | ||
| const m = /\/pull\/(\d+)\/?(?:[?#]|$)/.exec(target); | ||
| return m ? m[1] : null; |
There was a problem hiding this comment.
[Critical] prNumberFromTarget re-derives PR-target classification with a narrower regex than the pipeline's canonical parser (parse-args.ts' PR_URL_RE ends in (?=$|[/?#]), is case-insensitive, and normalizes pure-integer tokens via Number()), so the new target-pinned scan can never claim the run's own verdict for target shapes the child accepts. Proven by executing both classifiers on the real sources: /pull/9014/files, /pull/9014/commits, /pull/9014/checks (the URLs GitHub's Files-changed tab hands users) → parent null / child PR 9014; 0042 → parent '0042' (pins pr-0042-) / child 42 (writes pr-42-); docs/pull/42 → parent '42' / child file; uppercase URLs (HTTPS://…/PULL/9014) → parent null (no i flag) / child PR 9014. — Failure scenario: qwen review run https://github.com/o/r/pull/9014/files --comment → the child runs the full PR review and posts it, writing qwen-review-pr-9014-composed.json; the parent pinned composedPatternFor(null) whose (?!pr-\d+-) lookahead rejects that name, so both the capture loop and the post-close fallback miss it → a completed, already-posted review is reported as completed: false, "no composed verdict was produced", exit 1. The pre-diff generic COMPOSED_PATTERN matched all of these names, so this diff introduces the regression; the SKILL.md text says of the canonical parser "do not re-classify tokens by hand", and this is exactly such a hand re-classification.
Derive the pin from the classifier the child uses so the two can never drift (the import is a second location, hence no suggestion block):
import { parseReviewArgs } from './parse-args.js';
export function prNumberFromTarget(target?: string): string | null {
if (!target) return null;
const t = parseReviewArgs(target).target;
return t.type === 'pr-number' || t.type === 'pr-url'
? String(t.number)
: null;
}中文说明
prNumberFromTarget 用比管线权威解析器更窄的正则重新推导 PR target 分类（parse-args.ts 的 PR_URL_RE 以 (?=$|[/?#]) 收尾、大小写不敏感、纯数字 token 经 Number() 归一化），导致新的按 target 钉死的工件扫描对子进程接受的若干 target 形态永远无法认领本次运行自己的 verdict。在真实源码上执行两个分类器对比证实：/pull/9014/files、/pull/9014/commits、/pull/9014/checks（浏览器 Files changed 页签给出的 URL）→ 父进程 null / 子进程 PR 9014；0042 → 父进程 '0042'（钉 pr-0042-）/ 子进程 42（写 pr-42-）；docs/pull/42 → 父进程 '42' / 子进程 file；大写 URL（HTTPS://…/PULL/9014）→ 父进程 null（缺 i 标志）/ 子进程 PR 9014。— 故障场景：qwen review run https://github.com/o/r/pull/9014/files --comment → 子进程完整执行 PR review 并已发布，写出 qwen-review-pr-9014-composed.json；父进程钉的是 composedPatternFor(null)，其 (?!pr-\d+-) 前瞻恰好排除该文件名，轮询与退出后兜底扫描双双错过 → 一个已完成（且已发布）的 review 被报告为 completed: false、"no composed verdict was produced"、exit 1。改动前的通用 COMPOSED_PATTERN 能匹配上述所有文件名，因此是本 diff 引入的回归；SKILL.md 正文明确要求"不要手工重新分类 token"，而这里恰是一次手工重分类。修复建议：改用子进程同款分类器派生钉选模式，使两者永不漂移（需同时改 import，故不用 suggestion 块）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 6f21649 — prNumberFromTarget now delegates to parseReviewArgs, the same classifier the child names its artifacts from, so the pin cannot diverge by construction. The four divergent shapes are pinned as tests: /pull/9014/files → 9014, 0042 → 42 (Number-normalized, matching the child's pr-42- names), docs/pull/42 → file target (no pin), and uppercase scheme/path URLs.
| const cliEntry = join(distDir, 'cli.js'); | ||
| if (existsSync(cliEntry)) { | ||
| const source = readFileSync(cliEntry, 'utf8'); |
There was a problem hiding this comment.
[Suggestion] The shebang-injection + chmod-755 block ships with no test gating it — the test-efficacy hunk probe flagged this file as hunk-survived, and scripts/tests/package-assets.test.js already runs copyBundleAssets against fixture trees containing dist/cli.js, so the new block executes there but is asserted nowhere. Deleting the block entirely, dropping the fs.chmodSync call (a shebang without the exec bit is still not exec-able), or dropping the startsWith('#!') guard all survive with the whole scripts suite green. — Concrete cost: a future packaging change silently regresses the block and reproduces the exact incident this PR fixes — shellContextEnv blanks a QWEN_CODE_CLI it cannot exec, so every agent-issued review subcommand falls back to the PATH's global install, with no CI signal pointing at the cause.
Add a case beside the existing fixtures, following that file's pattern:
// fixture root with dist/cli.js → copyBundleAssets({ root }) →
assert.ok(readFileSync(cliEntry, 'utf8').startsWith('#!/usr/bin/env node\n'));
assert.strictEqual(statSync(cliEntry).mode & 0o777, 0o755);
// run copyBundleAssets again → the shebang must not be doubled中文说明
shebang 注入 + chmod-755 代码块没有任何测试钉住——测试有效性探针将该文件标记为 hunk-survived；scripts/tests/package-assets.test.js 已经会在包含 dist/cli.js 的 fixture 树上运行 copyBundleAssets，新代码块会被执行，但没有任何断言。整块删除、去掉 fs.chmodSync（只有 shebang 没有执行位仍不可执行）、或去掉 startsWith('#!') 守卫，整个 scripts 套件都将保持绿色。— 具体代价：未来某次打包改动静默回归该代码块，复现本 PR 所修复的事故本身——shellContextEnv 将无法直接执行的 QWEN_CODE_CLI 置空，所有 agent 发起的 review 子命令回落到 PATH 上的全局安装，且没有任何 CI 信号指向原因。建议在现有 fixture 旁补一个用例：断言输出以 #!/usr/bin/env node\n 开头、mode 为 0o755，并再运行一次验证 shebang 不会叠加。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 6f21649 — added the case to scripts/tests/package-assets.test.js per the suggested pattern: shebang asserted, mode 0o755 asserted (off-win32 — Windows has no POSIX exec bit), and a second copyBundleAssets run must leave the file byte-identical (no stacked shebang). Deleting the block, the chmod, or the guard now fails the suite.
| export function composedPatternFor(prNumber: string | null): RegExp { | ||
| return prNumber | ||
| ? new RegExp(`^qwen-review-pr-${prNumber}-.*composed\\.json$`) | ||
| : /^qwen-review-(?!pr-\d+-).*composed\.json$/; | ||
| } |
There was a problem hiding this comment.
[Suggestion] Target-name pinning cannot separate concurrent runs of the same target class. Probe-measured: prNumberFromTarget(undefined) → null for every no-target run, so two concurrent local runs build the identical (?!pr-\d+-) pattern, and both children write the same fixed filename qwen-review-local-composed.json — whichever composes last wins on disk for both capture loops (the mtime re-capture makes the later write overwrite the earlier capture), and the cutoff guard cannot help because one shared file is newer than both cutoffs. — Concrete cost: two qwen review run (no target) in one repo at once — two CI jobs on a shared workspace, or a re-run started while the first is still running — the run that finishes first can capture and republish the other run's composed verdict and exit code: the measured failure mode this PR fixes, one target-class over. Not a regression (same-target local runs collided pre-diff too), but the fixed shared filename shows target identity is not a sufficient key.
Either accept and document the residual race here (so the next incident is not diagnosed from scratch), or thread a per-run nonce from runReview into the child's artifact names so each parent pins to its own files exactly.
中文说明
按 target 名称钉选无法隔离同一 target 类别的并发运行。探针实测：所有无 target 运行的 prNumberFromTarget(undefined) 都是 null，两个并发 local 运行构造出完全相同的 (?!pr-\d+-) 模式，且两个子进程写同一个固定文件名 qwen-review-local-composed.json——谁后 compose，谁就在磁盘上对两个捕获循环生效（mtime 重捕获会让更晚的写入覆盖先前的捕获），cutoff 守卫也无济于事，因为同一个共享文件比两个 cutoff 都新。— 具体代价：同一仓库同时运行两个无 target 的 qwen review run（共享工作区的两个 CI 任务，或第一个还没跑完就重新启动）——先结束的运行可能捕获并转发另一个运行的 composed verdict 与退出码：这正是本 PR 修复的实测故障形态，只是挪了一个 target 类别。这不是回归（改动前同 target 的 local 运行同样会互相冲突），但固定的共享文件名说明 target 身份不是充分的键。建议要么在此处接受并记录这一残留竞态（避免下次事故从零诊断），要么从 runReview 向子进程工件名传入每次运行唯一的 nonce，使每个父进程精确钉选自己的文件。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Documented in 6f21649, per the accept-and-document option: composedPatternFor's doc now names the residual same-target race (two no-target runs share the fixed qwen-review-local-composed.json, last compose wins for both parents) and why a per-run nonce would have to come from the bundled skill's artifact naming, not this command — so the next incident is not diagnosed as a pin failure.
Review feedback on the target pin: prNumberFromTarget re-derived PR classification with a narrower regex than parse-args — /pull/<n>/files URLs went unpinned, 0042 pinned pr-0042- while the child writes pr-42-, and docs/pull/42 pinned a file target as a PR — so a completed (and posted) review could be reported as one that produced no verdict. Delegate to parseReviewArgs, whose verdict is what the child names its artifacts from, and pin the divergent shapes as tests. Also gate the bundle's shebang/exec-bit block with a package-assets case (mode asserted off-win32, double-run must not stack shebangs), and document the accepted same-target residual race on composedPatternFor.
|
@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
2 additional image(s) did not pass the hosting checks (PNG magic, unique sanitized name, ≤2 MB, max 8) and remain in the run artifacts.
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Re-run after Template ✓ — all headings present, bilingual body intact. Problem: observed, not theoretical — the four defects still come from the same live Direction: aligned as before — reliability of the repo's own review pipeline, which CI and maintainers consume. No auth/sandbox/public-contract surface. Size: not a core-module PR — Approach: the one new commit is exactly the minimal fix. The test asserted Risk: no elevated risk signals — no high-risk-path matches. Moving on to code review. 🔍 中文说明由 模板 ✓ —— 全部标题齐备，双语正文完整。 问题：已观测而非理论——四个缺陷仍来自同一轮 方向：与之前一致——仓库自身 review 管线的可靠性，CI 与维护者都在消费。不涉及 auth/sandbox/公开契约面。 规模：非核心模块 PR—— 方案：新提交恰为最小修复。原测试断言 风险：无升级风险信号——未命中高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRead at My independent proposal for the delta: the old assertion The two open round-7 suggestions from the
Neither blocks the merge — both are edge cases in this PR's own new logic with loud failure modes, and both already carry inline suggestions. Worth landing as a follow-up so the pinning is as airtight as the rest of this PR made it. Not verified: live behaviour on this exact head is not exercised by this run — unattended CI never runs PR code. What IS settled is in the verification record below. CI evidenceThe PR's own CI on the reviewed commit, quoted from the checks API (fetched once, not polled). Both CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 On the behavioural record: the completed sandboxed 中文说明代码审查在 我对这个增量的独立方案：旧断言拿文件系统和它从未承诺持有的值比较——
两者都不阻塞合并——都是本 PR 自身新逻辑里的边缘情形，失败模式响亮，且都已有行内建议。值得作为后续落地，让钉选像本 PR 其余部分一样严密。 未验证：本运行不实演该头上的现场行为——无人值守 CI 从不执行 PR 代码。已确证的内容见下方验证记录。 CI 证据被审提交上 PR 自己的 CI，引自 checks API（一次性抓取，不轮询）。两个 （CI 表格见上方英文部分，由 finalize 任务持续更新。） 行为证据方面：针对 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean across every stage; the residue is two non-blocking round-7 suggestions (an edge-case fail-fast, an oracle anchor) and an A/B re-proof still in flight on this head — none of it touches what the green suite already pins. Stepping back: the one commit since the last pass is exactly the repair I would have written, and it is a good one to end on. The mtime test had quietly started attesting libuv's The picture as a whole: four defects that are unambiguously real (two of them preserved in this thread's own early bot reviews), four adversarial review rounds, a completed sandboxed A/B that flips all four behaviours head-vs-base, a green Approving, pinned to the reviewed commit. 中文说明置信度：4/5 —— 各阶段均干净；遗留的是两条不阻塞的第七轮建议（一个边缘情形的快速失败、一个 oracle 尾锚），以及该头上仍在运行的 A/B 重证——都不触及绿色套件已钉死的内容。 退一步看：上次通过后的这一个提交正是我会写的那种修复，作为收尾恰到好处。mtime 测试悄悄开始为 libuv 的 整体图景：四个毫无疑问真实的缺陷（其中两个就保存在本线程早先的 bot review 里）、四轮对抗性审查、一次在 head/base 间将四项行为全部按声明翻转的已完成沙盒 A/B、该头上转绿的 批准，钉死在被审提交上。 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap (5) before two consecutive dry rounds (rounds 3 and 5 dry, round 4 reported).
Not explored to full depth (tool budget reached): "PR #9086 hardens the /review pipeline (per-target artifact…": none — all checks completed within the tool budget.; "PR #9086 fixes four live-observed defects in the /review…": none — every check I started completed within budget..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap (5) before two consecutive dry rounds (rounds 3 and 5 dry, round 4 reported)。
未探索到全部深度（达到工具调用预算）："PR #9086 hardens the /review pipeline (per-target artifact…"：none — all checks completed within the tool budget.；"PR #9086 fixes four live-observed defects in the /review…"：none — every check I started completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| export function composedPatternFor(prNumber: string | null): RegExp { | ||
| return prNumber | ||
| ? new RegExp(`^qwen-review-pr-${prNumber}-.*composed\\.json$`) | ||
| : /^qwen-review-(?!pr-\d+-).*composed\.json$/; |
There was a problem hiding this comment.
[Critical] The local/file-class pin derives the target class from the artifact name shape: the (?!pr-\d+-) lookahead rejects the run's OWN composed artifact whenever a FILE target's basename starts with pr-<digits>-, and the PR branch's .* wildcard conversely claims that file's artifact. reportPatternFor has the same seam on both branches. Probe-verified against the exported functions at this commit: composedPatternFor(null).test('qwen-review-pr-9086-notes.md-composed.json') → false (the pre-diff generic pattern matched it), while composedPatternFor('42').test('qwen-review-pr-42-notes.ts-composed.json') → true.
— Failure scenario: qwen review run pr-9086-notes.md (a valid file target — parseReviewArgs classifies it file, pin is null) → the child writes qwen-review-pr-9086-notes.md-composed.json per the skill's {target} = filename convention → the lookahead rejects the run's own name, so neither the poll loop nor the post-exit fallback captures it → a completed review is reported as "no composed verdict was produced", completed:false, exit 1. Mirror direction: a concurrent review run 42 matches that same artifact through ^qwen-review-pr-42-.*composed\.json$ and republishes the file review's verdict as PR 42's — the cross-run swap this PR exists to kill. A file review of notes-pr-42.md likewise loses its reportPath to reportPatternFor(null).
| : /^qwen-review-(?!pr-\d+-).*composed\.json$/; | |
| : /^qwen-review-(?!pr-\d+-composed\.json$).*composed\.json$/; |
Exclude only the exact PR artifact shape — a PR review writes exactly qwen-review-pr-<n>-composed.json, while a file-target name carries more between pr-<n>- and -composed.json; end-anchor the PR branch to the exact composed name, and anchor reportPatternFor to the timestamped -pr-<n>.md shape symmetrically. Add regression tests for file targets named pr-<n>-… claiming their own artifacts.
中文说明
本地/文件类钉选从工件名称形态推导 target 类别：(?!pr-\d+-) 前瞻会在 FILE target 的文件名以 pr-<数字>- 开头时拒绝本次运行自己的 composed 工件，而 PR 分支的 .* 通配反过来会认领该文件工件。reportPatternFor 在两个分支上有同样的接缝。已在本提交的导出函数上探针验证：composedPatternFor(null).test('qwen-review-pr-9086-notes.md-composed.json') → false（改动前的通用模式能匹配），而 composedPatternFor('42').test('qwen-review-pr-42-notes.ts-composed.json') → true。
— 故障场景：qwen review run pr-9086-notes.md（合法 file target——parseReviewArgs 分类为 file，钉为 null）→ 子进程按 skill 的 {target} = 文件名约定写出 qwen-review-pr-9086-notes.md-composed.json → 前瞻拒绝本运行自己的文件名，轮询与退出后兜底双双错过 → 已完成的 review 被报告为 "no composed verdict was produced"、completed:false、exit 1。反向：并发的 review run 42 经 ^qwen-review-pr-42-.*composed\.json$ 匹配同一工件，把文件 review 的 verdict 当作 PR 42 的对外转发——正是本 PR 要消灭的跨运行错发。对 notes-pr-42.md 的文件 review 同样会因 reportPatternFor(null) 丢失自己的 reportPath。
修复建议：只排除精确的 PR 工件形态（PR review 恰好写 qwen-review-pr-<n>-composed.json，而文件 target 名在 pr-<n>- 与 -composed.json 之间还有更多内容），PR 分支端锚定到精确名，reportPatternFor 对称地锚定带时间戳的 -pr-<n>.md 形态；补一条文件名为 pr-<n>-… 的回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 607ce99 — the shape heuristics are gone. classifyRunTarget (parse-args verdict) yields a target class, and composedPatternFor pins the ONE exact filename that class produces per the skill's --out template: qwen-review-pr-<n>-composed.json, qwen-review-<file basename>-composed.json, or the fixed qwen-review-local-composed.json. Both probe directions are pinned as tests: the pr-42-notes.ts file run claims its own artifact, and review run 42 no longer claims it (the PR pattern lost its .* wildcard).
| * be mistaken for the local run (the reverse direction is already covered by | ||
| * the PR pin). | ||
| * | ||
| * Known residual race, accepted: the pin separates target CLASSES, not runs. |
There was a problem hiding this comment.
[Suggestion] The null pin pools ALL non-PR runs into one class, so concurrent file-target and no-target runs still cross-capture each other's verdicts and reports — probe-verified at this commit: a no-target run whose own qwen-review-local-composed.json (REQUEST_CHANGES) was written first republished a neighbour's qwen-review-b.ts-composed.json (APPROVE) as its own verdict. The acceptance rationale below argues only the two-no-target-run shape (both share the one fixed filename); file targets DO carry deterministic identity (qwen-review-<filename>-composed.json), and excluding the fixed local name needs no nonce and no name derivation.
— Concrete cost: same repo, overlapping window — review run a.ts + review run b.ts (or a no-target run alongside either): whichever non-PR run composes last is captured by every other non-PR parent, with the neighbour's event deciding this run's exit code (B's APPROVE masks A's REQUEST_CHANGES under --fail-on request-changes). reportPatternFor(null) pools the same way over the saved reports.
Fix: pin the no-target run to exactly ^qwen-review-local-composed\.json$ and exclude the fixed local name from the file-run pin (/^qwen-review-(?!pr-\d+-|local).*composed\.json$/), with the symmetric exclusion in reportPatternFor(null); add a handler test seeding a neighbour file-target artifact. (Residual same-filename collisions remain, as documented.)
中文说明
null 钉把所有非 PR 运行池化为一类，因此并发的文件 target 运行与无 target 运行仍会互相捕获对方的 verdict 与报告——已在本提交探针验证：一个无 target 运行自己的 qwen-review-local-composed.json（REQUEST_CHANGES）先写入，却把邻居的 qwen-review-b.ts-composed.json（APPROVE）当作自己的 verdict 对外转发。下方的接受性理由只论证了"两个无 target 运行"这一形态（二者共享同一个固定文件名）；文件 target 其实带有确定性身份（qwen-review-<filename>-composed.json），且排除固定的 local 名既不需要 nonce 也不需要任何名称推导。
— 具体代价：同仓库重叠时间窗内 review run a.ts + review run b.ts（或无 target 运行与任一文件运行并发）：谁后 compose 就被其他每个非 PR 父进程捕获，邻居的 event 决定本运行的退出码（--fail-on request-changes 下 B 的 APPROVE 会掩盖 A 的 REQUEST_CHANGES）。reportPatternFor(null) 对保存的报告以同样方式池化。
修复建议：把无 target 运行钉死到恰好 ^qwen-review-local-composed\.json$，并在文件运行钉中排除固定的 local 名（/^qwen-review-(?!pr-\d+-|local).*composed\.json$/），reportPatternFor(null) 对称处理；补一个播种邻居文件工件的 handler 测试。（同名文件碰撞仍是残留项，如文档所述。）
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 607ce99 by the same restructure — non-PR runs no longer pool: a file run pins its own basename artifact exactly, and a bare run pins exactly qwen-review-local-composed.json, so concurrent file/local runs cannot cross-capture composed verdicts. The residual is now strictly same-target (two bare runs, or the same PR twice), documented on composedPatternFor. Report scans for non-PR runs still pool (stems are model-chosen — three formats in one live day); noted in reportPatternFor's doc, and the verdict/exit code never rides the report.
| const codename = /You are review agent `([^`\n]+)`/.exec(rec.launchPrompt); | ||
| if (codename) return `agent ${codename[1]}`; |
There was a problem hiding this comment.
[Suggestion] Three confirmed issues at this one site. (1) Duplication: cost-ledger.ts:201 (labelOf) already parses the same You are review agent `x` identity format into the same agent x label — the format (owned by agent-prompt.ts:669/1422) now has two independent parsers; when it changes and this copy is missed, labels silently fall back to the truncated first line — exactly the regression this PR fixes. (2) Round folding: the capture keeps only the backticked slug and drops the (round N) suffix baked into the identity line (agent-prompt.ts:1418-1422), so reverse-audit rounds 1 and 2 that both disclose budget gaps render in the posted body as two indistinguishable agent reverse-audit lines (gapsSuperseded only silences identical built prompts). (3) File folding: it likewise drops the Your file: `<path>`. suffix invariant roles carry (one launch per heavy file) — pre-diff the first-line label carried both suffixes, and cost-ledger's parser of this same format deliberately keeps round and file as distinct rows ("The role alone would fold those parallel runs into one (×N) row … and lose the per-file breakdown"). Probe-verified: two invariant-a transcripts with distinct files render indistinguishably at this commit.
— Concrete cost: a high-effort run whose two reverse-audit rounds both hit the tool ceiling posts two indistinguishable agent reverse-audit gap disclosures; a 3B review of two heavy files posts two indistinguishable agent invariant-a entries — the reader cannot tell which round/file disclosed what; and the two parsers must move in lockstep with the identity format forever.
Fix: extract a shared helper (cost-ledger already imports CHUNK_RE from ./lib/coverage.js, so the seam exists) that captures role + optional round + optional file and returns e.g. agent reverse-audit (round 2) / agent invariant-a (packages/a/big.ts); call it with the full prompt here and with the isolated identity line from cost-ledger — the two parsers make opposite, deliberate scoping choices, so the helper must take the text scope as input.
中文说明
同一位置有三个已确认的问题。(1) 重复：cost-ledger.ts:201（labelOf）已把同一 You are review agent `x` 身份行格式解析成同样的 agent x 标签——该格式由 agent-prompt.ts:669/1422 拥有，现在有了两个独立解析器；格式一旦变化而本处副本漏改，标签会静默回落到截断首行——正是本 PR 修复的回归。(2) 轮次折叠：捕获只保留反引号内的角色名，丢掉身份行中烘焙的 (round N) 后缀（agent-prompt.ts:1418-1422），于是两个都披露预算缺口的 reverse-audit 轮次在公开正文中渲染为两条无法区分的 agent reverse-audit（gapsSuperseded 只对完全相同的构建 prompt 消音）。(3) 文件折叠：同样丢掉 invariant 角色携带的 Your file: `<path>`. 后缀（每个 heavy 文件一次启动）——改动前首行标签携带这两个后缀，且 cost-ledger 对同一格式的解析器有意保留轮次与文件作为独立行（"仅角色名会把这些并行运行折叠成一个 (×N) 行……丢掉按文件的细分"）。已探针验证：两个文件不同的 invariant-a 转录在本提交下渲染得无法区分。
— 具体代价：一次 high-effort 运行的两个 reverse-audit 轮次都触到工具上限时，公开正文出现两条无法区分的 agent reverse-audit 缺口披露；对两个 heavy 文件的 3B review 出现两条无法区分的 agent invariant-a——读者无法分辨哪一轮/哪个文件披露了什么；且两个解析器从此必须与身份行格式永远锁步演进。
修复建议：抽一个共享 helper（cost-ledger 已从 ./lib/coverage.js 引入 CHUNK_RE，接缝已存在），捕获角色 + 可选轮次 + 可选文件，返回如 agent reverse-audit (round 2) / agent invariant-a (packages/a/big.ts)；此处以完整 prompt 调用，cost-ledger 以其分离出的身份行调用——两个解析器的作用域选择相反且都是刻意的，helper 应把文本作用域作为输入。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 607ce99 — all three: (1) one parser now lives in lib/agent-identity.ts (labelFromIdentityLine), used by both cost-ledger's labelOf and coverage's label; (2)+(3) the shared grammar keeps the (round N) and Your file: suffixes (round wins over file, as cost-ledger's rows always ordered them), so reverse-audit rounds and per-file invariant launches stay distinct in disclosures too. The consumers keep their own line-selection policies: cost-ledger still trusts only the first line (your quoted-mention test still passes), coverage takes the first line-anchored identity line because launchers prepend context.
| @@ -409,7 +412,7 @@ export const INLINE_BUDGET_GAP_RE = | |||
| * them linear too. | |||
| */ | |||
| const PLACEHOLDER_GAP_RE = | |||
There was a problem hiding this comment.
[Suggestion] Two probe-verified vocabulary drifts inside the one idiom family this change is widening. (a) The new trailing adverbial accepts within|under|inside, but the sibling stayed branch accepts under|within|below — below is missing from the new group. (b) The stayed branch received none of the qualifier vocabulary this very diff legitimises two lines away (inside, (?:the\s+)?, (?:tool(?:[- ]call)?\s+)?), so stayed combined with those qualifiers keeps as a real gap.
— Concrete cost: probed against the real budgetGapDisclosures at this commit: none — all checks completed below budget. → kept; none — stayed inside budget. → kept; none — stayed under the tool budget → kept; none — stayed below the tool-call budget. → kept — each surfaces in check-coverage's operator NOTE and the posted body as a fake coverage gap: the over-disclosure mode this PR exists to end, wearing words this regex already uses for budget position. Both inputs are attested: the doc comment cites N/A - stayed under budget, but … as an observed live shape, and the tool budget/the tool-call budget are the qualifiers from the 2026-08-13 live round this change patches. Flip-checked: aligning the vocabularies drops all four while every keep-case (but…/except… continuations) still keeps, all 49 existing tests green.
Fix: lift the budget adverbial into one shared subpattern used by all three branches — prepositions within|under|inside|below, optional (?:the\s+)?, optional tool(?:[- ]call)? qualifier — e.g. stayed\s+(?:within|under|below|inside)\s+(?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget\b for the stayed branch; the same constant is the natural home for the my/our/this/that/a/an determiner widening noted in the terminal report.
中文说明
本次放宽的同一习语家族内有两处经探针验证的词汇漂移。(a) 新的尾部 budget 状语接受 within|under|inside，但兄弟 stayed 分支接受 under|within|below——新组缺了 below。(b) stayed 分支没有获得本 diff 在两行之外刚刚合法化的限定词词汇（inside、(?:the\s+)?、(?:tool(?:[- ]call)?\s+)?），于是 stayed 与这些限定词组合时仍被当作真实缺口保留。
— 具体代价：在本提交的真实 budgetGapDisclosures 上探针验证：none — all checks completed below budget. → 保留；none — stayed inside budget. → 保留；none — stayed under the tool budget → 保留；none — stayed below the tool-call budget. → 保留——每一条都会作为假覆盖缺口进入 check-coverage 的 NOTE 与公开正文：正是本 PR 要终结的过度披露形态，而用的词汇恰是这个正则自己已用于 budget 位置的词。两种输入均有实证：文档注释引用 N/A - stayed under budget, but … 为现场观察形态，the tool budget/the tool-call budget 正是本改动修补的 2026-08-13 实轮的限定词。翻转验证：对齐词汇后上述四条全部被丢弃，所有保留形态（but…/except… 从句）仍保留，既有 49 条测试全绿。
修复建议：把 budget 状语抽成三个分支共用的子模式——介词 within|under|inside|below、可选 (?:the\s+)?、可选 tool(?:[- ]call)? 限定词——例如 stayed 分支改为 stayed\s+(?:within|under|below|inside)\s+(?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget\b；终端报告提到的 my/our/this/that/a/an 限定词放宽也宜放进同一常量。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 607ce99 — one vocabulary across the family: below joined the completion tail's position words, and the stayed branch took the full qualifier set (inside, optional the, optional tool/tool-call). All four probed strings are pinned as drop cases; the end-anchor keeps (stayed under budget, but … still discloses).
| const reportPath = newestArtifactSince( | ||
| REVIEWS_DIR, | ||
| reportPatternFor(prNumber), | ||
| cutoffMs, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The report-scan pinning for PR targets has no handler-level test: the new handler tests assert only completed/event/composedPath, and the only reportPath assertion in run.test.ts is the local case (review.md). reportPatternFor itself has unit tests; this wiring is the untested part.
— Failure scenario: a future change that regresses this call site (reverts to /\.md$/, or wires composedPattern here) leaves every test in the diff green, while two concurrent PR reviews in one repo put the neighbour's .qwen/reviews/<date>-pr-<other>.md into this run's --json reportPath.
Fix: extend one of the new handler tests to seed a neighbour report (e.g. 2026-08-13-162336-pr-9013.md) alongside this run's own …-pr-9014.md and assert result.reportPath points at 9014's.
中文说明
PR target 的报告扫描钉选没有 handler 级测试：新 handler 测试只断言 completed/event/composedPath，run.test.ts 中唯一的 reportPath 断言是 local 场景（review.md）。reportPatternFor 本身有单测；这条接线是未被测试的部分。
— 故障场景：未来某次改动让该调用点回归（退回 /\.md$/，或在此误接 composedPattern）时，diff 内所有测试仍绿，而同仓库两个并发 PR review 会把邻居的 .qwen/reviews/<date>-pr-<other>.md 写进本运行的 --json reportPath。
修复建议：在某个新 handler 测试中播种一份邻居报告（如 2026-08-13-162336-pr-9013.md），与本运行自己的 …-pr-9014.md 并存，断言 result.reportPath 指向 9014 的那份。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 607ce99 — the concurrent-run handler test now seeds a strictly-newer neighbour report (…-pr-9013.md, mtime forced +60s) beside this run's own and asserts result.reportPath lands on pr-9014.md, so a regression to /\.md$/ (or wiring the composed pattern there) fails the handler test, not just the pattern units.
| } catch { | ||
| return; // swept between scan and stat; keep the captured verdict | ||
| } |
There was a problem hiding this comment.
[Suggestion] The scan-vs-sweep race branch (new in this diff) has no test: nothing drives statSync throwing or the catch path. The race is real by construction — the child's Step 9 cleanup unlinks the composed artifact while the parent may still be polling before close.
— Failure scenario: if the try/catch regresses in a refactor, statSync throws out of the setInterval callback → uncaught exception → the run crashes in its closing window instead of finishing on the captured verdict.
Fix: spy statSync to throw once after the first capture and assert the run still completes with the captured verdict.
中文说明
扫描-清扫竞争分支（本 diff 新增）没有测试：没有任何用例让 statSync 抛错或走到 catch 路径。该竞争在构造上真实存在——子进程的 Step 9 清理会在父进程 close 前仍可能轮询时删除 composed 工件。
— 故障场景：若 try/catch 在重构中回归，statSync 会从 setInterval 回调中抛出 → 未捕获异常 → 运行在收尾窗口内崩溃，而不是带着已捕获的 verdict 正常结束。
修复建议：spy statSync 令其在首次捕获后抛错一次，断言运行仍以已捕获的 verdict 完成。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Resolved structurally in 607ce99 — the branch no longer exists: newestArtifactSince now returns {path, mtime}, the capture poll reuses the scan's own stat, and the second statSync (with its scan-vs-sweep window and untestable catch) is deleted. newestArtifactSince's unit test asserts the attached mtime.
…entity-line parser
Round-2 review feedback, all six findings:
- run.ts: replace name-shape pins with the exact composed filename each
target class produces (pr-<n> / file basename / the fixed 'local' token,
per the skill's --out template). The (?!pr-\d+-) lookahead rejected a
file run's own artifact whenever the reviewed file was named
pr-<digits>-…, the PR branch's .* wildcard claimed that same artifact,
and the pooled null class let concurrent file and no-target runs
cross-capture each other's verdicts. Target classification now comes
from classifyRunTarget (parse-args' verdict, basename for files).
- run.ts: newestArtifactSince returns {path, mtime}, so the capture poll
reuses the scan's own stat instead of re-statting the path — the
scan-vs-sweep window (and its untestable catch branch) is gone
structurally.
- lib/agent-identity.ts: one parser for the identity line agent-prompt
bakes into every launch, shared by cost-ledger's row labels and
coverage's disclosure labels — the two hand-rolled copies could drift,
and coverage's copy dropped the (round N) and owned-file suffixes,
folding reverse-audit rounds into indistinguishable disclosure lines.
cost-ledger still feeds it only the first line (quoted identity lines
below must never be credited); coverage scans for the first
line-anchored identity line (launchers prepend context lines).
- lib/budget.ts: one vocabulary for the budget-idiom family — 'below'
joins the completion tail's position words, and the stayed idiom takes
the same qualifiers ('stayed inside the tool-call budget').
- run.test.ts: handler-level assertion that the report scan is pinned
(a strictly newer neighbour report must not become this run's
reportPath), alongside the pattern-level cases for every shape the
review probed.
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 reviewed: reverse audit — stopped at the round cap (5) before two consecutive dry rounds (pair round 2 reported, rounds 3 and 4 reported, round 5 dry).
Not explored to full depth (tool budget reached): "PR #9086 fixes four live-observed defects in the /review…": none — all planned checks completed within the tool budget.; "PR #9086 fixes four live-observed defects in the /review…": did not execute the package unit-test suites ( run.test.ts , agent-identity.test.ts , budget.test.ts , cost-ledger.test.ts , check-coverage.test.ts , compo….
Test Plan (not a blocker): 168 passed — this review observed 19461 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — stopped at the round cap (5) before two consecutive dry rounds (pair round 2 reported, rounds 3 and 4 reported, round 5 dry)。
未探索到全部深度（达到工具调用预算）："PR #9086 fixes four live-observed defects in the /review…"：none — all planned checks completed within the tool budget.；"PR #9086 fixes four live-observed defects in the /review…"：did not execute the package unit-test suites ( run.test.ts , agent-identity.test.ts , budget.test.ts , cost-ledger.test.ts , check-coverage.test.ts , compo…。
Test Plan（非阻断）：168 passed — this review observed 19461 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| /** A role that IS a chunk assignment — `chunk 3 of 7` — labels as its id. */ | ||
| const CHUNK_ROLE_RE = /^chunk (\d+) of \d+$/; | ||
|
|
||
| const IDENTITY_LINE_RE = /^You are review agent `([^`\n]+)`(.*)$/; |
There was a problem hiding this comment.
[Suggestion] Identity parsing fails outright on lines carrying a trailing \r (CRLF-split prompts): in IDENTITY_LINE_RE, . cannot match \r and $ does not anchor before it, so labelFromIdentityLine returns null — probe-verified (the same line with/without trailing \r → null vs match; adding .replace(/\r$/, '') flips the probe). labelFromLaunchPrompt splits on '\n' only and cost-ledger feeds launch.slice(0, nl), so the residue reaches the parser either way. The replaced cost-ledger regexes were unanchored and \r-tolerant, so this is a tolerance regression; budget.ts in the same subsystem already splits on /\r?\n/. — Failure scenario: a launch prompt recorded with CRLF line endings → every identity parse fails → coverage falls back to the truncated first line, silently reactivating the exact "truncated PR quote instead of a codename" defect this PR fixes, and cost-ledger folds round/file-distinct rows back together.
| const IDENTITY_LINE_RE = /^You are review agent `([^`\n]+)`(.*)$/; | |
| const IDENTITY_LINE_RE = /^You are review agent `([^`\n]+)`(.*)\r?$/; |
中文说明
身份行解析在带尾随 \r 的行（CRLF 换行的 prompt）上彻底失败：IDENTITY_LINE_RE 中 . 不匹配 \r，$ 也不锚定在它之前，于是 labelFromIdentityLine 返回 null（已探针验证：同一行带/不带尾 \r → null vs 匹配；加 .replace(/\r$/, '') 即翻转）。labelFromLaunchPrompt 只按 '\n' 切分、cost-ledger 喂入 launch.slice(0, nl)，残留的 \r 两条路径都会到达解析器。被替换的 cost-ledger 正则未锚定、容忍 \r，因此这是一次容忍度回归；同子系统的 budget.ts 已按 /\r?\n/ 切分。— 故障场景：以 CRLF 记录的 launch prompt → 所有身份解析失败 → coverage 回退到截断的首行，本 PR 修复的"PR 引文当标签"缺陷被静默复活，cost-ledger 把按轮次/文件区分的行重新折叠。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 78a1fc0 — labelFromIdentityLine strips a trailing \r before matching, covering both feeders (the \n-splitting scan and cost-ledger's first-line slice); CRLF cases pinned for both entry points.
| * (`N/A - stayed under budget, but the Windows matrix never ran`); | ||
| * (`N/A - stayed under budget`), with the same position words and | ||
| * budget qualifiers as the completion tail below (`stayed inside the | ||
| * tool-call budget`) — one vocabulary for one idiom family; text |
There was a problem hiding this comment.
[Suggestion] The no-gap vocabulary is unpinned at the producer: toolBudgetBlock in agent-prompt.ts fixes only the disclosure form for unfinished checks (Budget gap: <the check>) and says nothing about the no-gaps case, so agents improvise the phrasing and this consumer-side whitelist must be re-patched for every new one — the diff's own comment records the recurrence (three improvisations reached two posted bodies in one live round). — Failure scenario: probe-verified budgetGapDisclosures('Budget gap: each check completed within budget') returns the text as a real gap (each is outside the head alternation (?:all|every(?:thing)?|planned|further|no further)) → the next improvisation parses as a phantom gap and reaches the posted verdict body again, exactly the defect this PR fixes, until another regex patch lands. Suggested fix: pin the no-gap case at the producing depth — extend toolBudgetBlock in agent-prompt.ts (e.g. "If every check completed within budget, write exactly one final line Budget gap: none"), keeping this idiom tolerance as the backstop for non-conforming returns.
中文说明
无缺口词汇没有在生产端钉死：agent-prompt.ts 的 toolBudgetBlock 只规定了未完成检查的披露格式（Budget gap: <the check>），对"无缺口"情形只字未提，于是 agent 自由发挥措辞，这个消费端白名单每出现一种新措辞就要再补一次——本 diff 自己的注释记录了复发史（一轮实跑中三条即兴措辞进入了两个公开正文）。— 故障场景：探针验证 budgetGapDisclosures('Budget gap: each check completed within budget') 会把文本当作真实缺口返回（each 不在头部交替 (?:all|every(?:thing)?|planned|further|no further) 内）→ 下一次即兴措辞又会解析成幻影缺口进入公开 verdict 正文，正是本 PR 修复的缺陷，直到再补一次正则。修复建议：在生产端钉死无缺口情形——扩展 agent-prompt.ts 的 toolBudgetBlock（例如"若所有检查都在预算内完成，只写一行 Budget gap: none"），此处习语容忍保留为对不合规范返回的兜底。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed at the producer in 78a1fc0 — toolBudgetBlock now states the no-gap rule explicitly: write NO Budget gap: line when nothing was cut short, because a "none" written in the format is parsed as a gap someone must rule on. The consumer-side filter stays as defence in depth for old transcripts and non-compliant agents.
| * and never matches, so the agent's own line is still the first hit. | ||
| */ | ||
| export function labelFromLaunchPrompt(prompt: string): string | null { | ||
| for (const line of prompt.split('\n')) { |
There was a problem hiding this comment.
[Suggestion] labelFromLaunchPrompt eagerly materializes the entire launch prompt as a line array though only the first few lines are ever inspected; when no identity line exists, the caller's fallback (coverage.ts:371) performs a second full split of the same string. — Failure scenario: label() runs this for every agent record in coverageFromTranscripts — every compose-review, check-coverage gate, and recompose; launcher-built prompts carry a diff-sized tail, so a roster of ~14 agents allocates transient strings proportional to ~14× the full prompt size (megabytes on a large PR) per coverage pass, doubled for identity-less prompts. Suggested fix: match against the prompt directly with the multiline flag — the scan stops at the match and allocates nothing for the tail:
export function labelFromLaunchPrompt(prompt: string): string | null {
const m = /^You are review agent `([^`\n]+)`(.*)$/m.exec(prompt);
return m ? labelFromIdentityLine(m[0]) : null;
}中文说明
labelFromLaunchPrompt 把整个 launch prompt 急切地物化为行数组，而实际只检查前几行；当不存在身份行时，调用方兜底（coverage.ts:371）还会对同一字符串做第二次完整切分。— 故障场景：label() 在 coverageFromTranscripts 里对每个 agent 记录都执行它——每次 compose-review、check-coverage 门禁与 recompose 都会触发；launcher 构造的 prompt 拖着 diff 大小的尾巴，~14 个 agent 的 roster 每次覆盖计算要分配约 14× 全 prompt 大小的临时字符串（大 PR 上以 MB 计），无身份行的 prompt 还要翻倍。修复建议：改用多行标志直接在 prompt 上匹配——扫描在命中处停止、不为尾部申请内存（代码见上）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 78a1fc0 — labelFromLaunchPrompt is now a single multiline exec over the prompt (match handed to labelFromIdentityLine); no line array is materialized and the identity-less fallback no longer implies a second full split.
| labelFromIdentityLine( | ||
| 'You are review agent `invariant-a` — Whole-file invariants. Your file: `packages/cli/src/a.ts`.', | ||
| ), | ||
| ).toBe('agent invariant-a (packages/cli/src/a.ts)'); |
There was a problem hiding this comment.
[Suggestion] The documented "round wins over file when both suffixes appear" precedence has no test — round and file are only tested separately, while buildRoleLaunchPrompt (agent-prompt.ts:1421-1424) emits (round N) and Your file: as independently optional suffixes on the same identity line. — Failure scenario: mutation-verified — swapping the round and file checks in labelFromIdentityLine survives the entire review suite (69 test files, 2440 passed). If that reorder ever ships, a both-suffix launch labels agent <role> (<file>) instead of agent <role> (round N), and cost-ledger folds two rounds of the same owned file into one (×2) row — the exact fold the round suffix exists to prevent. Suggested fix: add one combined case:
expect(
labelFromIdentityLine(
'You are review agent `invariant-a` — Whole-file invariants (round 2). Your file: `packages/cli/src/a.ts`.',
),
).toBe('agent invariant-a (round 2)');中文说明
文档声明的"两个后缀同时出现时 round 优先于 file"没有测试——round 与 file 只分别被测过，而 buildRoleLaunchPrompt（agent-prompt.ts:1421-1424）把 (round N) 与 Your file: 作为同一身份行上各自可选的后缀发出。— 故障场景：已变异验证——交换 labelFromIdentityLine 中 round 与 file 的检查后，整个 review 测试套件仍全绿（69 个测试文件、2440 通过）。若该重排有一天被合入，双后缀启动会被标记为 agent <role> (<file>) 而非 agent <role> (round N)，cost-ledger 会把同一文件的两轮折叠成一行（×2）——正是 round 后缀要防止的折叠。修复建议：补一个组合用例（代码见上）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in 78a1fc0 — the both-suffixes case ((round 2) + Your file: on one line) is pinned to label agent invariant-a (round 2), so the mutation that swaps the round/file checks no longer survives.
| 'none — all planned checks completed except the Windows matrix', | ||
| // The budget adverbial is end-anchored like its siblings: a clause | ||
| // continuing past it discloses skipped work. | ||
| 'none — all checks completed within budget, but the Windows matrix never ran', |
There was a problem hiding this comment.
[Suggestion] The new trailing budget-adverbial group was added to BOTH branches of PLACEHOLDER_GAP_RE, but the exception-clause keep case added in this diff covers only the dash form — the parenthesis-form analogue has no test (current behaviour is probe-verified correct: the (?!…but…) lookahead stops the span before but). — Failure scenario: a future edit loosening the [.!…,;:\s]*\)\s*$ tail — plausibly to tolerate trailing prose in the same idiom family — would silently filter a real disclosure out of the posted body with every existing test green.
| 'none — all checks completed within budget, but the Windows matrix never ran', | |
| 'none — all checks completed within budget, but the Windows matrix never ran', | |
| 'None (all checks completed within the tool-call budget, but the Windows matrix never ran)', |
中文说明
新增的尾随 budget 状语组被加进 PLACEHOLDER_GAP_RE 的两个分支，但本 diff 新增的例外从句保留用例只覆盖了破折号形态——括号形态的对应物没有测试（当前行为已探针验证为正确：(?!…but…) 前瞻使匹配跨度停在 but 之前）。— 故障场景：未来为容忍同一习语族的尾部散文而放宽 [.!…,;:\s]*\)\s*$ 尾部时，会把一条真实披露静默地从公开正文中滤掉，而所有现有测试保持绿色。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in 78a1fc0 — the parenthesis-form keep case (None (all checks completed within the tool-call budget, but the Windows matrix never ran)) sits beside the dash form, so loosening the \)\s*$ tail now fails a test.
| export function reportPatternFor(cls: RunTargetClass): RegExp { | ||
| return cls.kind === 'pr' | ||
| ? new RegExp(`-pr-${cls.number}\\.md$`) | ||
| : /^(?!.*-pr-\d+\.md$).*\.md$/; |
There was a problem hiding this comment.
[Suggestion] The non-PR report pattern rejects every name ending -pr-<digits>.md, but a file review of a file literally named pr-<digits>.md produces exactly that suffix (SKILL.md prescribes <YYYY-MM-DD>-<HHMMSS>-<filename>.md), so such a run rejects its OWN report — the same self-rejection shape the composed-pattern tests explicitly guard against (pr-42-notes.ts), left in place for reports. Verdict and exit code are unaffected (the composed pin is exact for file targets); only the informational reportPath is damaged. — Failure scenario: qwen review run docs/pr-1234.md completes and Step 8 writes .qwen/reviews/2026-08-13-101010-pr-1234.md; the negative lookahead excludes it → the JSON result carries reportPath: null although the report exists, and a concurrent review run 1234 in the same window claims the file run's report through its -pr-1234\.md$ pin. Suggested fix: pin the prescribed suffix per class instead of shape-excluding — file → -${escapeRe(cls.base)}$ (the base already carries .md), local → -local\.md$; caveat: report stems are model-chosen (this diff's own doc names three formats observed in one day), so per-class pinning must tolerate stem variation or accept null when the model deviates.
中文说明
非 PR 报告模式拒绝所有以 -pr-<数字>.md 结尾的名字，但对恰好名为 pr-<数字>.md 的文件做文件 review 时恰会产出该后缀（SKILL.md 规定 <YYYY-MM-DD>-<HHMMSS>-<filename>.md），于是该运行会拒绝认领自己的报告——composed 模式的测试明确防过的自拒绝形态（pr-42-notes.ts）在报告侧仍然保留。verdict 与退出码不受影响（composed 钉选对文件 target 是精确的），只有信息性的 reportPath 受损。— 故障场景：qwen review run docs/pr-1234.md 完成且 Step 8 写出 .qwen/reviews/2026-08-13-101010-pr-1234.md；负向前瞻将其排除 → 报告存在但 JSON 结果中 reportPath: null，同窗口内并发的 review run 1234 会经其 -pr-1234\.md$ 钉选认领该文件运行的报告。修复建议：按类别钉死规定后缀而非形态排除——file → -${escapeRe(cls.base)}$（base 已含 .md）、local → -local\.md$；注意：报告文件名主干由模型即兴（本 diff 自己的注释记录了一天观察到三种格式），按类钉死需容忍主干变化，或在模型偏离时接受 null。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 78a1fc0 — reportPatternFor gained a file branch pinned to the filename slot of the report stem (-<basename>.md$, or -<basename>$ when the basename already ends .md), so docs/pr-1234.md claims its own report instead of tripping the local branch's PR exclusion. The remaining by-name ambiguity (a PR run cannot be told from a file run whose basename is pr-<n>.md) is documented on the function — the report is informational, the composed pin stays exact.
| * OTHER run's verdict the moment it appeared (measured: two of three | ||
| * parallel PR reviews republished a neighbour's `composedPath`). | ||
| * | ||
| * Known residual race, accepted: the pin separates target IDENTITIES, not |
There was a problem hiding this comment.
[Suggestion] The residual-race note omits two collision classes the basename-keyed pin itself defines — both probe-verified at this commit. (1) Cross-class: a FILE target whose basename is exactly local or pr-<digits> produces a composed-pattern byte-identical to the local/PR pin (composedPatternFor({kind:'file', base:'pr-9014'}) ≡ PR-9014's pin; base local ≡ the local pin). (2) File↔file: two FILE targets with different paths but the same basename (a monorepo's index.ts in two packages) produce the identical pin, so concurrent reviews of DISTINCT files pool verdicts. The paragraph's examples name only same-target (same-argument) collisions — "two bare runs, or the same PR twice" — and "the pin separates target IDENTITIES, not runs" would tell the next maintainer these cross-class swaps cannot happen; neither is separable parent-side while the child names artifacts by basename. — Failure scenario: qwen review run pr-9014 (an extensionless file literally named pr-9014) concurrent with review run 9014 → both children write qwen-review-pr-9014-composed.json and whichever composes last wins both republished verdicts → a CI gate reading event/exit code gets the other run's verdict; same shape for same-basename files in different packages, where one run exits 0 carrying the OTHER file's verdict with composedPath pointing at a verdict that is not its own. Suggested fix: extend this note to name both classes — the pin keys on the child's artifact NAME (the basename for file targets), so distinct targets sharing one name pool exactly like same-target runs; only a child-minted nonce (or a path-sanitized {target} token in the skill's file-review artifact names) resolves that.
中文说明
残留竞态注释遗漏了按 basename 钉选自身定义的两类碰撞——均已在本提交探针验证。（1）跨类别：basename 恰为 local 或 pr-<数字> 的 FILE target 会产出与 local/PR 钉选逐字节相同的模式（composedPatternFor({kind:'file', base:'pr-9014'}) ≡ PR-9014 的钉选；base local ≡ local 钉选）。（2）文件↔文件：路径不同但 basename 相同的两个 FILE target（monorepo 里两个包的 index.ts）产出相同钉选，不同文件的并发 review 会互串 verdict。该段落的例子只举了同 target（同参数）碰撞——"两个裸运行，或同一 PR 跑两次"——而"钉选分离的是 target 身份，不是运行"会让下一位维护者以为这些跨类错发不可能发生；在子进程按 basename 命名工件的前提下，二者都无法在父进程侧分离。— 故障场景：qwen review run pr-9014（恰好名为 pr-9014 的无扩展名文件）与 review run 9014 并发 → 两个子进程都写 qwen-review-pr-9014-composed.json，后 compose 者赢得双方对外转发的 verdict → 读 event/退出码的 CI 门禁拿到另一次运行的 verdict；不同包同名文件同形，一次运行会以另一文件的 verdict 退出 0 且 composedPath 指向不属于自己的 verdict。修复建议：在本注释中补名这两类——钉选以子进程工件名（文件 target 的 basename）为键，因此共享同一名字的不同 target 与同 target 运行一样互串；只有子进程铸造的 nonce（或 skill 文件 review 工件名中带路径净化的 {target} token）能解决。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Documented in 78a1fc0 — the residual-race note now enumerates all three collision classes as filename collisions (same target twice; different paths sharing one basename; a basename spelling local/pr-<digits>), and states plainly that the pin separates composed FILENAMES — as much identity as the child's naming carries — with the per-run nonce being the skill's to mint.
| // The skill's `{target}` token for a file review is the file's basename | ||
| // (`--target <filename>` in the capture step), so that is the identity | ||
| // the child's artifact names carry. | ||
| return { kind: 'file', base: t.path.split(/[\\/]/).pop() ?? t.path }; |
There was a problem hiding this comment.
[Suggestion] For a target ending in a path separator, split(/[\\/]/).pop() returns '' (split never returns an empty array, so ?? t.path is dead code) and the composed pin becomes ^qwen-review--composed\.json$ — a name no child artifact can carry (the skill's {target} token renders non-empty basenames). Probe-verified end to end at this commit: parseReviewArgs('src/') classifies file with no warnings, classifyRunTarget('src/') → {kind:'file', base:''}, and the child CAN complete — captureLocalDiff({file:'src/'}) produces a real diff (toRepoPathspec resolves src/ → the legal directory pathspec src). Severity is Suggestion rather than Critical because the failure is fail-closed: no wrong verdict is ever republished, and the exit is 1, never 0. — Failure scenario: qwen review run src/ (a tab-completed path) → the child reviews everything under src/ and composes a verdict, but the parent's pin never matches, Step 9 sweeps the real artifact, the post-close scan finds nothing → "Review did not complete: no composed verdict was produced", exit 1 for a review that completed — the failure the pin's own JSDoc exists to prevent, reintroduced for the trailing-separator input class; a gate that blocks only on exit 3 (treating 1 as retryable tool-breakage) would also lose the REQUEST_CHANGES-vs-tool-error distinction.
| return { kind: 'file', base: t.path.split(/[\\/]/).pop() ?? t.path }; | |
| const segs = t.path.split(/[\\/]/).filter(Boolean); | |
| const base = segs.pop(); | |
| if (!base) return { kind: 'local' }; | |
| return { kind: 'file', base }; |
中文说明
对以路径分隔符结尾的 target，split(/[\\/]/).pop() 返回 ''（split 从不返回空数组，故 ?? t.path 是死代码），composed 钉选变为 ^qwen-review--composed\.json$——任何子进程工件都不可能叫这个名字（skill 的 {target} token 渲染非空 basename）。已在本提交端到端探针验证：parseReviewArgs('src/') 无警告地分类为 file，classifyRunTarget('src/') → {kind:'file', base:''}，且子进程能完成——captureLocalDiff({file:'src/'}) 产出真实 diff（toRepoPathspec 把 src/ 解析为合法目录 pathspec src）。定为 Suggestion 而非 Critical，因为失败是 fail-closed 的：绝不会错发 verdict，退出码是 1 而不会是 0。— 故障场景：qwen review run src/（tab 补全的路径）→ 子进程 review src/ 下全部内容并 compose 出 verdict，但父进程的钉选永远匹配不上，Step 9 清走真实工件，退出后扫描一无所获 → "Review did not complete: no composed verdict was produced"，一个已完成的 review 以退出码 1 收场——正是钉选自身 JSDoc 声明要防止的失败，在尾分隔符输入类上被重新引入；只在退出码 3 上设卡（把 1 当作可重试的工具故障）的门禁还会丢失 REQUEST_CHANGES 与工具故障的区分。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 78a1fc0 — trailing path separators are stripped before taking the basename, so review run src/ pins qwen-review-src-composed.json (with || trimmed as the degenerate-path fallback; the ?? dead code is gone). Pinned as a classify test.
…de no-gap rule, edge-case pins - agent-identity: tolerate a trailing CR (CRLF-recorded prompts fed through \n-splitting callers failed every parse and fell back to first-line prose); scan the launch prompt with one multiline match instead of materializing a line array per agent record; pin the round-over-file precedence with a both-suffixes test. - agent-prompt: state the no-gap rule at the producer — write NO 'Budget gap:' line when nothing was cut short — instead of leaving each agent to improvise a 'none' phrasing the consumer-side placeholder filter must chase forever. - run.ts: strip trailing path separators before taking a file target's basename (a tab-completed 'src/' pinned 'qwen-review--composed.json', which no child artifact carries — fail-closed exit 1 on a completed review); pin file-run reports by their filename slot so a file named 'pr-1234.md' claims its own report; document the two collision classes the basename-keyed pin defines (same-basename files, basenames spelling 'local'/'pr-<n>'). - budget.test: pin the parenthesis-form exception keep case beside the dash form.
|
@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: 189 passed · 0 failed · 189 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：189 通过 · 0 失败 · 189 总计 Verification reportPR 9086 deep verification —
|
| # | Previous-report finding (PR 9065 round 2) | Sev. | Status at this head |
|---|---|---|---|
| 1 | Mutation survivor (unconditional witness key) — dead axis |
Info | Not applicable — belongs to PR 9065's findings.ts; not re-measured here (different PR). |
| 2 | Renderer parses but does not display witness |
Info | Not applicable — different PR's renderer surface. |
| 3 | Suite reconciliation / stale-bundle environmental |
Info | Not applicable — though this round's full-surface run is likewise green with the same 4 environmental skips (see Targeted gates). |
| 4 | No length cap on witness |
Info | Not applicable — different PR. |
Scope selection
Central claim — a review run claims only its own target's artifacts:
the composed-verdict and report scans are pinned to the run's classified
target (PR / file / local, classified by the child's own parser), and the
capture poll keeps re-reading while the child runs so a recomposed verdict
supersedes the first snapshot. Pre-fix, a generic newest-file scan captured a
concurrent run's artifacts (measured live: two of three parallel reviews
republished a neighbour's verdict), and the first snapshot stuck.
Secondary claims: (2) the placeholder-gap filter drops completion idioms
carrying a trailing budget adverbial (…completed within budget) while a
clause continuing past it still discloses; (3) disclosure/ledger labels come
from one shared identity-line parser that finds the brief codename anywhere in
a launcher-prepended prompt (CRLF-tolerant, suffix precedence preserved); (4)
the bundle emits dist/cli.js with a shebang and the execute bit so
shellContextEnv keeps QWEN_CODE_CLI instead of blanking it.
Out of scope (listed under Not covered): model-side behavior, Windows exec
semantics, repo-wide gates, per-commit attribution.
Central claim — A/B proof
Harness 1 (harness/01-ab-artifact-pinning.mjs, witness
01-ab-artifact-pinning-head-vs-base.png) drives the real compiled
handler — yargs, the capture poll timer, the post-exit fallback scan, the
JSON contract, the exit code — on both arms. The control is a byte copy of
head's packages/cli/dist in which run.js was swapped for an esbuild
compile of git show HEAD^1:…run.ts; at this harness's run time that was the
control's only difference (diff -rq verified; the remaining four changed
modules were reverted in the same control afterwards, for harnesses 2–3 —
none of them is imported by the run path, so harness 1's cells are unaffected).
The child the handler spawns is intercepted at the
process.argv[1] seam (a launcher that serves a scripted fake review per
scenario — artifact writes, delayed recompose, Step-9-style sweep before
exit); no code under test is stubbed. Each scenario encodes the base bug as
the control's predicted behavior, so a base cell that republishes the
neighbour passes its assertion.
| scenario | oracle | head | base (control) |
|---|---|---|---|
| s1: PR 9014, neighbour 9013 composes first, own lands later | event / composedPath / reportPath | COMMENT, …pr-9014-composed.json, …-pr-9014.md |
❌ APPROVE, …pr-9013-composed.json, …-pr-9013.md — the live bug, all three fields wrong (expected red) |
s2: same target recomposes (APPROVE → REQUEST_CHANGES, newer mtime) |
final event | REQUEST_CHANGES |
❌ APPROVE — first snapshot sticks (expected red) |
| s3: bare local run, newer PR artifact present | event / composedPath | COMMENT / …local-composed.json |
❌ APPROVE / …pr-9013-… (expected red) |
s4: file run src/widget.ts, newer PR artifact present |
event / composedPath | COMMENT / …widget.ts-… |
❌ APPROVE / …pr-777-… (expected red) |
s5: PR-42 run vs a NEWER qwen-review-pr-42.md-composed.json (a file run of a file named pr-42.md) |
event / composedPath | COMMENT / …pr-42-composed.json |
❌ APPROVE / the newer file-run artifact (expected red) |
s6: tab-completed src/ |
composedPath | …qwen-review-src-composed.json |
same (shape sanity, both arms) |
5/5 flip scenarios flipped; 67/67 assertions. Every cell exited 0 with
completed: true — including the base cells, which is exactly the danger the
PR fixes: the wrong verdict republishes as a plausible-looking completed run.
s1's base cell reproduces the description's live evidence field for field
(review run 9014 → "event": "APPROVE", "composedPath": "…pr-9013…").
The recompose cell (s2) proves the poll's re-read against a real mtime
advance, and the sweep before child exit in every scenario proves the verdict
came from the poll capture, not the post-exit fallback.
Reviewer Test Plan, walked step by step
- "run the three named suites → 168 passed" — executed: run.test.ts 39 +
budget.test.ts 49 + check-coverage.test.ts 80 = 168/168. The count is
exact at this head. - "bundle, then
head -c 2 dist/cli.js=#!, mode 755, direct exec" —
verified on the CI-built bundle (harness 4 cell 0: shebang, 755,
./dist/cli.js --versionexit 0, version printed) and by re-running the
real scripts of both arms over a reverted bundle state (cells 2–4). - "optionally reproduce the concurrency bug on the base commit" —
reproduced deterministically (s1 base cell), without a model, via the
handler-level fake child.
Secondary claims
Budget placeholder filter (harness 2, 02-budget-regex-ab-and-ladder.png)
Drives the real compiled budgetGapDisclosures() of both arms. 50/50.
- 11 drop shapes flip head↔base: the three live-leaked strings from the
description, the parenthesis form, and the round-2 vocabulary (belowin
the completion tail,inside/the/tool[-call]qualifiers on the stayed
idiom) — head returns[], base keeps each (expected leaks), including the
siblingnone — all checks finished under budgetthat base also leaks
(base has no adverbial tail at all after the completion word). - 7 keep shapes hold on BOTH arms: a clause past the adverbial (dash and
parenthesis forms),except, the negation guard, a bare real gap — no
over-drop regression. - Hostile ladder (the regex runs before the 160-char truncation, on
model-authored lines): filler, many near-miss completion words, and the
parenthesized near-miss, rung by rung up to 20k characters (the filler
shape tops out at ~2.4k because its generator pads shorter) — ≤ 0.7 ms
on every rung of every shape, no superlinear trend.
Identity-line labels (harness 3, 03-identity-label-ab.png)
Drives the real coverageFromTranscripts() of both arms with the same
fixture shape the PR's regression test uses. 22/22.
- Prepended-context prompt: head labels the disclosure
agent 6c; base
labels itPR #9045 modifies getAuthTypeFromEnv() to infer auth.— the
live defect, flipped. - CRLF-recorded prompt: head still
agent 6c(round-3 fix). - No-drift oracle: the new shared parser vs a verbatim copy of base's inline
cost-ledger logic on six first-line shapes (plain, round, chunk, owned-file,
round+file precedence, spaced role) — identical output on all six; the two
retired copies have not drifted into the one that replaced them. - Edge shapes: trailing CR, round-beats-file, chunk-beats-round, missing
closing backtick → null, empty role → null, first line-anchored identity
wins over a quoted identity below, CRLF scan.
Bundle executability (harness 4, 04-bundle-exec-ab.png)
Runs the real copy_bundle_assets.js of each arm over a fake repo root
(symlinked sources + copied dist whose cli.js was reverted to the base
bundle state: no shebang, 0644). 21/21.
- Base script over the base state: still no shebang, still 644, and the real
shellContextEnvpredicate (fresh process per probe) blanks
QWEN_CODE_CLI— the defect chain end to end. - Head script over the same state: shebang added (exactly one), mode 755,
predicate keeps the entry,./cli.js --versionexits 0. A second head run
does not stack shebangs (idempotent). - Predicate boundaries pinned: 755-without-shebang → blanked;
shebang-without-exec-bit → blanked; both → kept.
One probe subtlety, for the record: isUnusableScriptEntry caches per path
per process (documented design — an entry's usability is fixed for a real CLI
process's lifetime), so predicate probes run in fresh subprocesses; an
in-process re-probe of a mutated file reads the stale cache and production
never does that.
Producer-side half of the budget fix (the agent-prompt no-gap rule): the text
is present at head and absent at base (grep both trees); model compliance
with it is out of sandbox reach — the consumer-side filter is what harness 2
proves.
Vacuity + mutation matrix (harness 5, 05-mutation-matrix.png)
Baseline unmutated: 401/401 (five cli suites) + 34/34 (package-assets). Each
mutant applied as an exact single-point replacement (occurrence count
asserted), full targeted suite run, source restored byte-identical (verified
per mutant; git status ended clean).
| mutant | target | result |
|---|---|---|
| M1b | run.ts: full base capture (generic pattern + first-snapshot latch) | killed — 2 red: expected 'APPROVE' to be 'COMMENT' (concurrent), expected 'APPROVE' to be 'REQUEST_CHANGES' (recompose) |
| M1c | run.ts: latch only (pin kept) | killed — recompose test red at the intended assertion |
| M1 | run.ts: generic pattern only (re-read kept) | survived 39/39 — adjudicated below |
| M2 | budget.ts: adverbial tail removed (dash branch) | killed — drops non-answers… red |
| M3 | agent-identity.ts: CRLF tolerance removed | killed — tolerates a trailing carriage return red |
| M4 | coverage.ts: identity lookup removed | killed — both codename tests red (check-coverage + compose-review) |
| M5 | copy_bundle_assets.js: shebang/exec block removed | killed — emits an executable dist/cli.js red |
| P1 | run.ts: exit code 3 → 0 (positive control) | killed — 3 red, proving the suite is falsifiable |
29/29 scoring assertions. Every guard the PR introduces is pinned by its
intended test — with the one survivor below, whose mechanism was diagnosed
rather than assumed: reverting ONLY the pattern half survives because the
recompose re-read (the fix's other half) self-heals it inside the suite's
fixture shapes — the concurrent test's neighbour composes FIRST, so a
re-reading generic scan ends on the run's own (newest) artifact anyway.
Reverting both halves (M1b) or the latch alone (M1c) is caught; the pattern
function itself is pinned by direct unit tests; only the handler-level WIRING
of the pattern is pinned transitively.
Findings
No blockers, no defects. One informational item:
- (Informational) M1 survivor — the pattern wiring is pinned only
transitively. Classification: coverage gap, not dead code and not a
defect. The behavior is proven correct on both arms by harness 1 (5/5
flips through the real handler), and the composite regression is pinned
(M1b dies), but a future change that re-widens the scan pattern while
keeping the re-read would pass the whole suite green whenever the run's own
artifact ends up newest. A fixture that would pin the wiring half: the
existing concurrent test with the neighbour artifact made strictly NEWEST
(utimes +60s, as harness 1's s5 does) — then even a re-reading generic
scan ends on the neighbour. Completeness reporting, not a merge condition.
Rejected candidate finding, for the record: the new bundle block calls
fs.chmodSync while the import diff adds only named imports — checked; the
file's pre-existing import fs from 'node:fs' (line 40) supplies it, and the
live script runs prove the path executes.
Targeted gates
- Three suites named in the description (run, budget, check-coverage):
168/168 — the description's count reproduced exactly. - Mutation baseline adds agent-identity + compose-review: 401/401;
package-assets (npm run test:scriptsconfig): 34/34. - Full
src/commands/reviewsurface: 70 files, 2512 passed, 4
skipped, 0 failed. The 4 skips are byte-identical environmental ones:
3×script-lintshellcheck cases (no shellcheck binary in the container)
and 1×save-artifactcase-insensitive-alias case (Linux FS is
case-sensitive) — both confirmed via theskipIfguards in source. - ESLint on the 7 changed production files: clean, with a live
control — a plantedconst eslintLiveProbe_unused = 1;in run.ts was
caught by the same invocation (no-unused-vars), then removed;
git statusclean. - Typecheck/build: covered by the pre-run
npm run buildat HEAD (the
environment contract's completed build; every harness above runs the
compiled output of it).
Not covered
- Model-side behavior — that agents honor the producer-side no-gap rule,
that a real review recomposes, that live concurrent reviews interleave as
scripted. The sandbox has no model. Harness 1 reproduces the shape of
the wire events (artifact scan → capture → republish) end to end through
the real handler; it does not reproduce the model-driven trigger. - Live GitHub reproduction of the concurrency bug — requires two real
model-driven reviews; the base-arm cells carry the proof instead. - Windows — the shebang/exec-bit block runs unconditionally; win32 exec
semantics were not probed (the description itself marks Windows⚠️ ). - cost-ledger end to end — its row labels now delegate to the shared
parser; the parser is proven no-drift against base's inline logic for the
first-line inputslabelOffeeds it (harness 3B), and its suite rides the
full-surface run; a transcript-driven cost-ledger A/B was not built. - Repo-wide gates — only the affected surfaces ran; untouched packages
rely on the PR's own CI. - Per-commit attribution — the snapshot lists 5 commits; the checkout is
depth 2,git rev-list HEAD^1..HEAD^2returns 1 (the shallow-boundary
artifact,git rev-parse --is-shallow-repository= true), so the
aggregateHEAD^1..HEADdiff was verified and the per-commit claims
(round-2/round-3 polishes) are taken from the commit messages, not
exercised separately. - Base worktree —
tmp/base-treeserved the A/B compiles and was
removed after the cells were captured.
Methodology
Environment: the CI verify container (node:22-bookworm, node v22.23.2),
detached merge-ref checkout at depth 2, npm ci + npm run build (+ bundle)
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 five
changed runtime modules (run.js, lib/budget.js, lib/coverage.js,
cost-ledger.js, agent-prompt.js) recompiled via esbuild from
git show HEAD^1:… sources; diff -rq confirmed only those families differ.
Workspace-link confound checked: the run.js closure imports no
@qwen-code/* module (grep), and the PR touches no core package, so the
hoisted root link (node_modules/@qwen-code/qwen-code-core →
packages/core, realpath asserted) is identical for both arms. Harness 1
drove the handler through a launcher pinned as process.argv[1], so the
spawned child was a scripted fake review while the parent ran the real CLI;
harnesses 2–4 imported the real compiled modules of each arm; harness 4
executed both arms' real copy_bundle_assets.js over a fake root and probed
getShellContextEnvVars in fresh subprocesses. Mutations were single-point
string replacements with asserted occurrence counts, run against the targeted
suite, restored byte-identically after each (verified); the earlier manual
M1/M1b/M1c diagnoses preceded the driver run. Per-cell stdout/stderr and all
gate logs live in logs/; evidence images were produced with
scripts/verify-capture.mjs from live re-runs of harnesses 1–5 (all re-runs
exited 0, matching the original runs).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "Context: PR #9086 fixes four live-observed defects in the…": none — all planned checks completed within budget.; "Context: PR #9086 fixes four live-observed defects in the…": nothing — all planned checks completed (did not run repo-wide npm run lint , nor the entire scripts suite beyond the targeted tests; these are out of scope for….
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："Context: PR #9086 fixes four live-observed defects in the…"：none — all planned checks completed within budget.；"Context: PR #9086 fixes four live-observed defects in the…"：nothing — all planned checks completed (did not run repo-wide npm run lint , nor the entire scripts suite beyond the targeted tests; these are out of scope for…。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| 'nothing was cut short, write NO `Budget gap:` line at all — the format ' + | ||
| 'is only for checks the ceiling stopped, and a "none" written in it is ' + | ||
| 'parsed as a gap someone must rule on. The ' + |
There was a problem hiding this comment.
[Suggestion] The new baked prompt text claims a "none" disclosure "is parsed as a gap someone must rule on", but the parser this same PR widens (budgetGapDisclosures/PLACEHOLDER_GAP_RE in lib/budget.ts) DROPS bare none and the none-idiom family — probe-verified: Budget gap: none, None., and Budget gap: none — all checks completed all return []. The prompt asserts the negation of the parser it ships beside. — Failure scenario: no runtime misbehavior today (the false claim happens to deter the desired output, and stray none lines are filtered); the cost is a maintenance trap in a subsystem whose stated purpose is eliminating format/parser drift — a future maintainer reconciling the contradiction can "fix" the parser to match the prompt (dropping the placeholder filter), reintroducing phantom None. gaps into posted bodies, or relax the filter believing the prompt already suppresses the input. The contradiction is invisible from either side alone: the prompt is only ever read by models, the regex only by the coverage tool.
| 'nothing was cut short, write NO `Budget gap:` line at all — the format ' + | |
| 'is only for checks the ceiling stopped, and a "none" written in it is ' + | |
| 'parsed as a gap someone must rule on. The ' + | |
| 'nothing was cut short, write NO `Budget gap:` line at all — the format ' + | |
| 'is only for checks the ceiling stopped, and a "none" phrasing the ' + | |
| 'parser does not recognise still survives as a gap to rule on. The ' + |
中文说明
新加入的 prompt 文本声称 "none" 披露会"被解析为一个需要裁决的缺口"，但本 PR 同时放宽的解析器（lib/budget.ts 的 budgetGapDisclosures/PLACEHOLDER_GAP_RE）恰恰会丢弃裸 none 及 none 习惯用语族——已探针验证：Budget gap: none、None.、Budget gap: none — all checks completed 均返回 []。prompt 断言了与它一同发布的解析器相反的语义。— 故障场景：当前没有运行时错误行为（错误说法恰好抑制了期望的输出，且零散的 none 行会被过滤）；代价是维护陷阱——该子系统声明的目的正是消除格式/解析器漂移：未来协调这一矛盾的维护者可能按 prompt 去"修"解析器（移除占位符过滤），让幽灵 None. 缺口重新进入公开正文，或者误以为 prompt 已抑制该类输入而放宽过滤。该矛盾从任何一侧单独看都不可见：prompt 只被模型读取，正则只被 coverage 工具读取。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in a66d700 — the sentence now states what actually happens: a "none" put in the format is at best filtered out, and any wording the filter does not recognize is published as a phantom coverage gap. No claim contradicts the parser anymore; the instruction (write no line at all) is unchanged.
| 'Budget gap: none — stayed under the tool budget', | ||
| 'Budget gap: none — stayed below the tool-call budget.', |
There was a problem hiding this comment.
[Suggestion] The widened budget-adverbial fixtures enumerate the qualifier family but omit the space-separated tool call form that the regex accepts ((?:tool(?:[- ]call)?\s+)?budget) — only hyphenated tool-call and bare tool budget are pinned, in both the completion and stayed branches. — Failure scenario: a fixture gap, not a code bug today (probe-verified: Budget gap: none — all checks completed within the tool call budget. classifies as a placeholder) — but the suite's stated purpose is pinning the whole idiom vocabulary: if a future edit narrows [- ] to -, an agent's honest none — … within the tool call budget disclosure stops matching PLACEHOLDER_GAP_RE and is rendered into the posted verdict body as a real coverage gap — the exact defect class this PR fixes — with no test failing.
| 'Budget gap: none — stayed under the tool budget', | |
| 'Budget gap: none — stayed below the tool-call budget.', | |
| 'Budget gap: none — stayed under the tool budget', | |
| 'Budget gap: none — stayed below the tool-call budget.', | |
| 'Budget gap: none — all checks completed within the tool call budget.', | |
| 'Budget gap: none — stayed inside the tool call budget.', |
中文说明
放宽后的 budget 状语夹具枚举了限定词族，但遗漏了正则实际接受（(?:tool(?:[- ]call)?\s+)?budget）的空格分隔 tool call 形态——completion 与 stayed 两个分支只钉了连字符 tool-call 与裸 tool budget。— 故障场景：今天是夹具缺口而非代码 bug（已探针验证：Budget gap: none — all checks completed within the tool call budget. 被归为占位符）——但本套件声明的目的是钉死整个习惯用语词汇表：若未来某次编辑把 [- ] 收窄为 -，agent 诚实写出的 none — … within the tool call budget 披露将不再匹配 PLACEHOLDER_GAP_RE，从而被渲染进公开 verdict 正文当作真实覆盖缺口——正是本 PR 修复的缺陷类别——且没有任何测试失败。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in a66d700 — the space-separated tool call form is pinned in both the completion-tail and stayed branches.
| */ | ||
| const PLACEHOLDER_GAP_RE = | ||
| /^(?:<[^>]*>$|[-—*_~`]+$|(?:none|n\/a|nothing|no (?:gaps?|checks?))\b(?:[.!…,;:\s]*$|\s+(?:skipped|found|to report)\b[.!…,;:\s]*$|\s*[-—–]\s*(?:stayed\s+(?:under|within|below)\s+budget\b[.!…,;:\s]*$|(?:all|every(?:thing)?|planned|further|no further)\b(?:(?!\b(?:but|except|excepting|excluding)\b).)*(?<!\b(?:none|nothing|no|zero|never|not)\s)\b(?:complete[ds]?|done|finished|covered)\b[.!…,;:\s]*$)|\s*\(\s*(?:all|every(?:thing)?)\b(?:(?!\b(?:but|except|excepting|excluding)\b)[^()])*(?<!\b(?:none|nothing|no|zero|never|not)\s)\b(?:complete[ds]?|done|finished|covered)\b[.!…,;:\s]*\)\s*$))/i; | ||
| /^(?:<[^>]*>$|[-—*_~`]+$|(?:none|n\/a|nothing|no (?:gaps?|checks?))\b(?:[.!…,;:\s]*$|\s+(?:skipped|found|to report)\b[.!…,;:\s]*$|\s*[-—–]\s*(?:stayed\s+(?:under|within|below|inside)\s+(?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget\b[.!…,;:\s]*$|(?:all|every(?:thing)?|planned|further|no further)\b(?:(?!\b(?:but|except|excepting|excluding)\b).)*(?<!\b(?:none|nothing|no|zero|never|not)\s)\b(?:complete[ds]?|done|finished|covered)\b(?:\s+(?:within|under|inside|below)\s+(?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget)?[.!…,;:\s]*$)|\s*\(\s*(?:all|every(?:thing)?)\b(?:(?!\b(?:but|except|excepting|excluding)\b)[^()])*(?<!\b(?:none|nothing|no|zero|never|not)\s)\b(?:complete[ds]?|done|finished|covered)\b(?:\s+(?:within|under|inside|below)\s+(?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget)?[.!…,;:\s]*\)\s*$))/i; |
There was a problem hiding this comment.
[Suggestion] The budget vocabulary this diff widens is hand-copied three times inside this one PLACEHOLDER_GAP_RE literal — the qualifier (?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget appears in the stayed idiom, the dash-form completion tail, and the paren-form completion tail, with the position words duplicated in two different orders (under|within|below|inside vs within|under|inside|below) — despite the docblock above declaring "one vocabulary for one idiom family". — Failure scenario: this regex is widened repeatedly as live phrasings slip past it (this PR is one such widening). The next vocabulary addition — a new position word or qualifier — requires three coordinated edits inside a ~700-char single-line literal; missing one branch means disclosures in that branch's form survive as phantom gaps downstream — the exact cost the docblock names — and the divergence is unobservable downstream. This diff itself had to touch both branch forms in lockstep (budget.test.ts's keep-side additions annotated "in both branch forms"), demonstrating the coupling. Extract the shared fragments so a vocabulary change is one edit:
const BUDGET_QUALIFIER = String.raw`(?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget`;
const BUDGET_POSITION = String.raw`(?:within|under|inside|below)`;
// …and build PLACEHOLDER_GAP_RE via new RegExp(…) referencing each
// fragment at all three sites中文说明
本 diff 放宽的 budget 词汇在这一个 PLACEHOLDER_GAP_RE 字面量里被手工复制了三遍——限定词 (?:the\s+)?(?:tool(?:[- ]call)?\s+)?budget 出现在 stayed 习惯用语、破折号形 completion 尾部与括号形 completion 尾部三处，位置词以两种不同顺序重复（under|within|below|inside 与 within|under|inside|below）——尽管上方 docblock 声明"一个习惯用语族一套词汇"。— 故障场景：该正则随着实跑措辞不断漏过而被反复放宽（本 PR 正是这样一次放宽）。下一次词汇新增——新的位置词或限定词——需要在这个约 700 字符的单行字面量里做三处协同编辑；漏改一个分支就会让该形态的披露在下游作为幽灵缺口存活——正是 docblock 点名的代价——且这种分歧在下游不可观测。本 diff 自己就不得不锁步修改两个分支形态（budget.test.ts 保留侧新增用例注明"in both branch forms"），足见耦合。建议抽取共享片段，使词汇变更只需一处编辑。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in a66d700 — the vocabulary is spelled once (BUDGET_QUALIFIED, with COMPLETION_TAIL derived) and PLACEHOLDER_GAP_RE is built from it; the three hand-copies (and their two divergent word orders) are gone. The regex body is otherwise byte-equivalent, and the full drop/keep suite pins the semantics.
| expect(r.body).toContain( | ||
| 'Not reviewed: `"You are review agent security — inspect auth"`', | ||
| ); | ||
| expect(r.body).toContain('Not reviewed: `"agent security"`'); |
There was a problem hiding this comment.
[Suggestion] This diff makes publicAgentSubject's docstring in compose-review.ts:1750-1754 (a file the diff does not touch) wrong: it says "Any other label is the truncated first line of a launch prompt: prose", but after coverage's label() prefers parsed identity labels, the non-chunk labels reaching publicAgentSubject are primarily codename labels (agent security, agent reverse-audit (round 2)) — as this added assertion itself demonstrates. They flow through on two live paths: the "Not reviewed" entries (compose-review.ts:785/807/829) and the budget-gap lines (compose-review.ts:1325/1332). — Failure scenario: the next reader changing that function's quoting/capping logic is told the input is always truncated launch prose and may reason on that basis — e.g. about the 60-char compressSummary cap, which short codename labels now pass through intact — the exact misreading the comment exists to prevent. The behavior is correct; only the provenance sentence is stale (made newly wrong by this diff's label change). Update the docstring, e.g.: "Any other label is a parsed identity label (agent <codename>, optionally round/file-suffixed), or, failing that, the truncated first line of a launch prompt."
中文说明
本 diff 使 compose-review.ts:1750-1754（diff 未触碰的文件）中 publicAgentSubject 的 docstring 失效：它声称"其他任何标签都是 launch prompt 截断后的首行：散文"，但在 coverage 的 label() 优先采用解析出的身份标签之后，到达 publicAgentSubject 的非 chunk 标签主要是代号标签（agent security、agent reverse-audit (round 2)）——本新增断言恰好证明了这一点。它们经由两条活路径流过："Not reviewed" 条目（compose-review.ts:785/807/829）与 budget-gap 行（compose-review.ts:1325/1332）。— 故障场景：下一个修改该函数引号/截断逻辑的读者会被告知输入恒为截断的 launch 散文，并可能据此推理——例如关于 60 字符的 compressSummary 上限，而短代号标签现在会原样通过——正是该注释意在防止的误读。行为本身正确，只是这句来源说明已过时（由本 diff 的标签变更新近致错）。建议把 docstring 更新为："其他任何标签都是解析出的身份标签（agent <代号>，可带 round/file 后缀），否则才是 launch prompt 截断后的首行。"
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in a66d700 — publicAgentSubject's provenance note now describes the parsed codename labels coverage prefers (with first-line prose as the fallback), including that short codename labels pass compressSummary's cap untouched.
| const best = newestArtifactSince(REVIEW_TMP_DIR, composedPattern, cutoffMs); | ||
| composedPath = best?.path ?? null; |
There was a problem hiding this comment.
[Suggestion] When the exact-name pin and the child's artifact name ever diverge, review run reports a completed (possibly already posted) review as "no composed verdict was produced" without naming the filename it expected — the failure carries no diagnostic of its own cause. The no-verdict prose branch (run.ts:551-563) sets detail = 'no composed verdict was produced', and RunReviewResult carries nothing that names the expectation. — Failure scenario: the pin is an exact-filename contract with the skill's prose template. When the next drift occurs (a skill edit to the naming template, or an exotic target rendered differently), every affected review run exits 1 with Review did not complete: no composed verdict was produced (CLI exit 0) — neither the stdout prose nor the --json result names the expected artifact, and the child's Step 9 has already swept .qwen/tmp, so the directory that would show the near-miss name is empty by the time anyone looks. A child exit code of 0 plus "no verdict" sends a 3 AM investigation toward the child instead of the pin. Derive the expected filename from targetClass and append it to the detail string and the JSON result: no composed verdict was produced (expected .qwen/tmp/qwen-review-pr-9014-composed.json).
中文说明
当精确文件名钉选与子进程的工件名发生漂移时，review run 会把一个已完成（甚至可能已发布）的 review 报告为 "no composed verdict was produced"，却不给出它所期望的文件名——失败本身不携带关于自身原因的诊断。无 verdict 的文案分支（run.ts:551-563）将 detail 置为 'no composed verdict was produced'，且 RunReviewResult 中没有任何字段说明期望值。— 故障场景：该钉选是与 skill 散文模板之间的精确文件名契约。下一次漂移发生时（skill 对命名模板的修改，或某个被不同渲染的特殊 target），每个受影响的 review run 都会以 Review did not complete: no composed verdict was produced (CLI exit 0) 退出 1——stdout 文案与 --json 结果都不说明期望的工件名，而子进程的 Step 9 早已清空 .qwen/tmp，等有人查看时那个能显示"差一点就匹配"文件名的目录已经空了。子进程 exit 0 加上 "no verdict" 会把凌晨三点的排查引向子进程而非钉选。建议从 targetClass 推导期望文件名并附加到 detail 与 JSON 结果中：no composed verdict was produced (expected .qwen/tmp/qwen-review-pr-9014-composed.json)。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in a66d700 — the pin is now derived from composedNameFor(targetClass), the no-verdict prose names the expected path (no composed verdict was produced (expected .qwen/tmp/<name>)), and the JSON result carries expectedComposedName, so a naming drift is diagnosable after Step 9 has swept the directory.
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. ✅
…ation, honest prompt claim - budget.ts: spell the budget-position vocabulary once (BUDGET_QUALIFIED / COMPLETION_TAIL) and build PLACEHOLDER_GAP_RE from it — the literal carried three hand-copies that had already drifted twice in two review rounds; the space-separated 'tool call' form is pinned in both branches. - agent-prompt.ts: the no-gap rule now states what actually happens to a 'none' disclosure — at best filtered, and any unrecognized wording is published as a phantom coverage gap — instead of claiming the parser treats it as a gap, which was the negation of the filter shipped beside it. - run.ts: derive the composed pin from composedNameFor and name the expected filename in both the no-verdict prose and the JSON result (expectedComposedName) — a naming drift between the pin and the skill's template was undiagnosable once Step 9 swept the near-miss. - compose-review.ts: publicAgentSubject's provenance note now describes the codename labels coverage prefers, with first-line prose as the fallback.
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 reviewed: reverse audit — stopped at the round cap (5) before two consecutive dry rounds (rounds 1 and 4 dry; rounds 2, 3 and 5 reported).
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; chunk 1: none — all planned checks were completed within budget. Wait — ironically, per the new instructions, I must not write a "Budget gap: none" line. I should not wr….
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — stopped at the round cap (5) before two consecutive dry rounds (rounds 1 and 4 dry; rounds 2, 3 and 5 reported)。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；chunk 1：none — all planned checks were completed within budget. Wait — ironically, per the new instructions, I must not write a "Budget gap: none" line. I should not wr…。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| : `no composed verdict was produced (expected ${join( | ||
| REVIEW_TMP_DIR, | ||
| composedNameFor(targetClass), | ||
| )})`; |
There was a problem hiding this comment.
[Suggestion] The new no-verdict diagnostic surface — this (expected <file>) suffix plus the expectedComposedName JSON field (run.ts:82, :542) — has zero test coverage: no assertion in run.test.ts names either. The message exists so a completed-but-uncaptured review (naming drift between the pin and the skill's qwen-review-{target}-composed.json template) stays diagnosable after Step 9 sweeps .qwen/tmp. Distinct from the round-4 diagnosability fix (which ADDED this surface): this is about the surface itself being unguarded. — Failure scenario: a future edit that drops the suffix, formats the path wrong, or names a file other than the one composedPatternFor actually scans for turns no test red (mutation-verified: both mutations leave the suite 39/39 green while a probe test flips) and the diagnostic silently points investigators at the wrong filename.
Add a handler test where the child exits 0 and no artifact ever appears for target '9014', asserting the output contains no composed verdict was produced (expected + qwen-review-pr-9014-composed.json, and in JSON mode result.expectedComposedName === 'qwen-review-pr-9014-composed.json'.
中文说明
新的 no-verdict 诊断面——这个 (expected <file>) 后缀加上 expectedComposedName JSON 字段（run.ts:82、:542）——零测试覆盖：run.test.ts 中没有任何断言涉及二者。该消息的存在是为了让"已完成但未被捕获"的 review（钉选与 skill 的 qwen-review-{target}-composed.json 模板之间发生命名漂移）在 Step 9 清扫 .qwen/tmp 之后仍然可以诊断。与第四轮的诊断性修复（添加了该诊断面本身）不同：这里指的是诊断面本身无测试守卫。— 故障场景：未来某次编辑删掉后缀、拼错路径、或写出一个 composedPatternFor 实际并不扫描的文件名，没有任何测试会变红（已通过变异验证：两种变异下套件仍 39/39 全绿，而探针测试翻转），诊断信息会悄悄把调查者指向错误的文件名。建议补一个 handler 测试：子进程退出码为 0 且 target '9014' 始终无工件出现时，断言输出包含 no composed verdict was produced (expected + qwen-review-pr-9014-composed.json，且 JSON 模式下 result.expectedComposedName === 'qwen-review-pr-9014-composed.json'。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in 281708f — a handler test asserts both surfaces for target 9014: the prose contains no composed verdict was produced (expected plus qwen-review-pr-9014-composed.json, and the JSON result carries expectedComposedName. Mutation-verified locally: dropping the suffix fails the test (1 failed | 39 passed).
| /** A role that IS a chunk assignment — `chunk 3 of 7` — labels as its id. */ | ||
| const CHUNK_ROLE_RE = /^chunk (\d+) of \d+$/; |
There was a problem hiding this comment.
[Suggestion] The refactor's stated purpose is "one format, one parser, so the two readers cannot drift apart again", but it re-introduces a second chunk-id grammar: CHUNK_ROLE_RE here is anchored and case-sensitive, while coverage's CHUNK_RE (/\bchunk\s+(\d+)\s+of\s+\d+\b/i, lib/coverage.ts:252) is unanchored and case-insensitive — and the deleted cost-ledger code explicitly reused coverage's CHUNK_RE for chunk roles precisely so the two readers stayed aligned. — Failure scenario: a hand-written or relaunched identity line whose chunk role deviates from the exact CLI-emitted form (e.g. Chunk 3 of 7 — hand-edited launch prompts are a recorded occurrence, agent-prompt.ts:1675 documents one) makes coverage's assignedChunk still resolve chunk 3 while cost-ledger's labelFromIdentityLine falls through to agent Chunk 3 of 7 — the same agent rendered as a chunk owner in the posted body and a role agent in the ledger row. Probe-verified on the unmodified PR; CLI-built launches match both regexes, so no test turns red today.
One grammar for both readers — aligned to coverage's shape, still anchored for the role slot (flip-verified: the divergence disappears and the existing agent-identity tests still pass):
| /** A role that IS a chunk assignment — `chunk 3 of 7` — labels as its id. */ | |
| const CHUNK_ROLE_RE = /^chunk (\d+) of \d+$/; | |
| /** A role that IS a chunk assignment — `chunk 3 of 7` — labels as its id. */ | |
| const CHUNK_ROLE_RE = /^chunk\s+(\d+)\s+of\s+\d+$/i; |
中文说明
本次重构的自述目标是"一种格式、一个解析器，两个读者永不漂移"，但它重新引入了第二套 chunk id 语法：此处的 CHUNK_ROLE_RE 有锚定且大小写敏感，而 coverage 的 CHUNK_RE（/\bchunk\s+(\d+)\s+of\s+\d+\b/i，lib/coverage.ts:252）无锚定且大小写不敏感——且被删除的 cost-ledger 代码正是显式复用 coverage 的 CHUNK_RE 来保持两个读者一致的。— 故障场景：手写或重启的 identity 行若 chunk 角色偏离 CLI 精确发射形态（例如 Chunk 3 of 7——手工编辑的 launch prompt 有真实记录，agent-prompt.ts:1675 记载过一次），coverage 的 assignedChunk 仍解析出 chunk 3，而 cost-ledger 的 labelFromIdentityLine 落空为 agent Chunk 3 of 7——同一个 agent 在公开正文里是 chunk 负责人、在台账行里却成了角色 agent。已在未修改的 PR 上探针验证；CLI 构建的 launch 两种正则都匹配，所以今天没有测试会变红。修复建议：两个读者共用一套语法——对齐 coverage 的形态并保持角色槽锚定（已做翻转验证：漂移消失，现有 agent-identity 测试仍全部通过）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 281708f — CHUNK_ROLE_RE now takes coverage's CHUNK_RE shape (\s+, case-insensitive), still anchored because here the whole role slot is the candidate. One grammar again for both readers.
| // A NEIGHBOUR review composes first — the shape that made two of three | ||
| // live parallel runs republish the wrong PR's verdict. | ||
| mkdirSync(REVIEW_TMP_DIR, { recursive: true }); | ||
| writeFileSync( | ||
| join(REVIEW_TMP_DIR, 'qwen-review-pr-9013-composed.json'), | ||
| JSON.stringify({ event: 'APPROVE', verdictLine: 'Verdict: Approve' }), |
There was a problem hiding this comment.
[Suggestion] This fixture writes the neighbour's composed artifact strictly OLDER than this run's own, so the verdict-side assertions below (completed, event === 'COMMENT', composedPath contains pr-9014) cannot distinguish the new pinned capture from the pre-fix generic newest-composed scan — the exact opposite of the live failure the comment cites. The report half of the same test IS discriminating because it forces the neighbour's report strictly newer with utimesSync; the composed half omits that step. — Failure scenario: under a regression of composedPatternFor (or a reintroduced unpinned /^qwen-review-.*composed\.json$/ scan) the last poll still finds this run's file newest, so this handler test stays green and only the regex-level unit tests — which never exercise the capture loop — would catch it. Mutation-verified: with the own artifact strictly newest the regression sails through 39/39; with the fix below applied, the same mutation fails red.
Mirror the report half: force the neighbour's composed artifact strictly newer — insert inside spawnMock, right after this writeFileSync closes:
utimesSync(
join(REVIEW_TMP_DIR, 'qwen-review-pr-9013-composed.json'),
Date.now() / 1000 + 60,
Date.now() / 1000 + 60,
);中文说明
该 fixture 把邻居的 composed 工件写得比本运行的更早，导致下方的 verdict 侧断言（completed、event === 'COMMENT'、composedPath 包含 pr-9014）无法区分新的按 target 钉死的捕获与修复前的通用"最新 composed"扫描——恰好与注释引用的实跑故障形态相反。同一测试的 report 半侧是有判别力的，因为它用 utimesSync 强制邻居报告严格更新；composed 半侧漏掉了这一步。— 故障场景：composedPatternFor 回归（或重新引入未钉死的 /^qwen-review-.*composed\.json$/ 扫描）时，最后一次轮询仍会发现本运行自己的文件最新，该 handler 测试保持绿色，只有从不经过捕获循环的正则层单测能捕获它。已通过变异验证：自身工件严格最新时回归 39/39 通过；应用下方修复后同一变异失败变红。修复建议：镜像 report 半侧的做法，强制邻居的 composed 工件严格更新——在 spawnMock 内、这个 writeFileSync 闭合之后插入上述 utimesSync。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in 281708f — the fixture now forces the neighbour's composed artifact strictly newer with utimesSync, mirroring the report half. Mutation-verified locally: reverting composedPatternFor to the generic /^qwen-review-.*composed\.json$/ scan now fails this handler test too (3 failed | 37 passed), where before it stayed green.
… align the chunk-role grammar - run.test.ts: assert the no-verdict report names the artifact it waited for, in prose and as expectedComposedName — mutation-verified: dropping the suffix now fails. - run.test.ts: force the neighbour's composed artifact strictly NEWER in the concurrent-run fixture. With it older, an unpinned newest-composed scan landed on the right file anyway and the regression passed; mutation-verified: reverting composedPatternFor to the generic scan now fails the handler test, not only the pattern units. - agent-identity.ts: CHUNK_ROLE_RE takes coverage's CHUNK_RE shape (whitespace-tolerant, case-insensitive) so a hand-edited 'Chunk 3 of 7' cannot resolve as a chunk owner in the posted body and a role agent in the ledger row.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — chunk 2 fully read; hunks outside my chunk (agent-prompt.ts, compose-review.ts, cost-ledger.ts, scripts/) were left to their owning agents by design.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; chunk 3: nothing — all checks I planned are complete. I used about 20 calls, comfortably within budget., and 3 more.
Test Plan (not a blocker): 168 passed — this review observed 19457 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — chunk 2 fully read; hunks outside my chunk (agent-prompt.ts, compose-review.ts, cost-ledger.ts, scripts/) were left to their owning agents by design.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；chunk 3：nothing — all checks I planned are complete. I used about 20 calls, comfortably within budget.，另有 3 条。
Test Plan（非阻断）：168 passed — this review observed 19457 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| export function composedNameFor(cls: RunTargetClass): string { | ||
| switch (cls.kind) { | ||
| case 'pr': | ||
| return `qwen-review-pr-${cls.number}-composed.json`; |
There was a problem hiding this comment.
[Suggestion] R6-1: composedNameFor hardcodes the exact composed filenames, duplicating the bundled skill's --out .qwen/tmp/qwen-review-{target}-composed.json template and its {target} = pr-<n> / local / filename convention — and the tests pin it only against self-referential literals, so nothing reads the skill template as oracle. Classification cannot drift (the parent delegates to the child's parseReviewArgs), but the final filename template is a second copy that no test links to the skill's. — Failure scenario: a future edit to the skill's --out template or {target} convention drifts silently: every review run of the drifted class completes and posts, yet the parent reports "no composed verdict was produced" and exits 1 — the exact live defect this PR's pinning exists to end. Suggested fix: add a parity test (in the style of scripts/tests/review-source-digest.test.ts) that extracts the composed --out template from the bundled SKILL.md and asserts composedNameFor's output matches it for each target class.
中文说明
[Suggestion] R6-1：composedNameFor 硬编码了确切的 composed 文件名，等于把 bundled skill 的 --out .qwen/tmp/qwen-review-{target}-composed.json 模板及其 {target} = pr-<n> / local / 文件名约定又抄了一份——而测试只用自引用的字面量来钉它，没有任何测试以 skill 模板为基准做对照。分类不会漂移（父进程委托给子进程自己的 parseReviewArgs），但最终文件名模板是第二份拷贝，且没有任何测试把它与 skill 的那份关联起来。— 故障场景：未来对 skill 的 --out 模板或 {target} 约定的修改会静默漂移：漂移类别的每次 review run 都正常完成并已发布，父进程却报告 "no composed verdict was produced" 并以 exit 1 结束——正是本 PR 的钉选机制要消灭的实跑缺陷。修复建议：仿照 scripts/tests/review-source-digest.test.ts 增加对照测试——从 bundled SKILL.md 提取 composed --out 模板，断言 composedNameFor 对每类 target 的输出与之相符。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in 7c94c18 — new run-skill-parity.test.ts reads the Step 6 --out template straight out of the bundled SKILL.md and asserts composedNameFor renders it for each target class (pr-9014 / foo.ts / local). A skill-side template edit now fails there, next to the code that must follow it. The test skips itself (rather than failing) when SKILL.md is absent, so a sparse checkout is not reported as a contract drift.
| export function reportPatternFor(cls: RunTargetClass): RegExp { | ||
| switch (cls.kind) { | ||
| case 'pr': | ||
| return new RegExp(`-pr-${cls.number}\\.md$`); |
There was a problem hiding this comment.
[Suggestion] R6-2: the same contract gap on the report half — reportPatternFor's three shapes encode SKILL.md Step 8's report stems (-pr-<number>.md, -<filename>.md) but are pinned only against hand-typed strings, including the unstated assumption that a file target ending in .md does not get a doubled .md. — Failure scenario: a Step 8 template edit leaves every reportPatternFor test green while review run silently reports reportPath: null — or, since the local branch claims any non-PR-shaped .md, a concurrent run's report; unlike the composed side, the no-report case carries no expected… diagnostic, so the drift is invisible at exactly the moment it happens. Suggested fix: the same parity test as above, extended to rendered PR/file/local report names from SKILL.md Step 8.
中文说明
[Suggestion] R6-2：report 半边存在同样的契约缺口——reportPatternFor 的三种形态编码了 SKILL.md Step 8 的报告文件名模板（-pr-<number>.md、-<filename>.md），但测试只用手敲的字符串来钉，其中包括一个未言明的假设：以 .md 结尾的 file target 不会被重复加上 .md。— 故障场景：Step 8 模板的修改会让 reportPatternFor 的所有测试继续绿灯，而 review run 静默报告 reportPath: null——或者，由于 local 分支会认领任何非 PR 形态的 .md，还可能认领并发运行的报告；与 composed 半边不同，无报告的情形没有 expected… 诊断信息，漂移在发生的那一刻就是不可见的。修复建议：同上面对照测试，扩展到用 SKILL.md Step 8 渲染出的 PR/file/local 报告名做断言。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Covered by the same parity test in 7c94c18 — it extracts Step 8's .qwen/reviews/<YYYY-MM-DD>-<HHMMSS>-<stem>.md stems, renders the PR/file/local cases, and asserts each class accepts its own and refuses the neighbours' (including the no-doubled-.md assumption for a file target ending in .md).
| const parsed = labelFromIdentityLine(identity); | ||
| if (parsed === null) return fallback; |
There was a problem hiding this comment.
[Suggestion] R6-3: the documented "trust only the first line" invariant at this site has no test holding it. Verified by mutation: switching labelOf to the sibling entry point labelFromLaunchPrompt keeps the entire cost-ledger suite green (45/45), because every fixture's launch prompt carries the identity line first or nowhere. — Failure scenario: a future consolidation of both callers on labelFromLaunchPrompt passes the whole suite, and then a launch whose prepended context quotes another agent's identity line above the agent's own labels the ledger row by the quote — folding two agents' costs into one row. Suggested fix: add a cost-ledger fixture whose launch prompt opens with a prepended context line quoting another agent's identity line above its own, asserting the row keeps the agent's own label — or an agent-identity test asserting the two entry points differ on that prompt.
中文说明
[Suggestion] R6-3：此处文档化的"只信任首行"不变量没有任何测试钉住。已用突变验证：把 labelOf 换成姊妹入口 labelFromLaunchPrompt，整套 cost-ledger 测试依旧全绿（45/45），因为每个 fixture 的 launch prompt 要么首行就是 identity 行，要么根本没有 identity 行。— 故障场景：未来把两个调用方统一到 labelFromLaunchPrompt 的改动会通过全部测试，之后一旦某个 launch 的前置上下文在 agent 自己的 identity 行之上引用了另一个 agent 的 identity 行，账本行就会被贴上引文里那个 agent 的标签——把两个 agent 的开销折进同一行。修复建议：在 cost-ledger 增加一个 fixture，其 launch prompt 首行是前置上下文、且在其中引用另一个 agent 的 identity 行（位于自身 identity 行之上），断言该行仍保留 agent 自己的标签——或在 agent-identity 增加断言两个入口对该 prompt 给出不同结果的测试。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in 7c94c18 — a cost-ledger fixture whose launch opens with a prepended context line above the identity line: the row keeps the transcript's own id, never a label lifted from below. Mutation-verified locally: switching labelOf to labelFromLaunchPrompt now fails this test (1 failed | 45 passed). An agent-identity test also pins that the two entry points genuinely differ on that prompt, so neither policy can be collapsed into the other unnoticed.
| const source = readFileSync(cliEntry, 'utf8'); | ||
| if (!source.startsWith('#!')) { | ||
| writeFileSync(cliEntry, `#!/usr/bin/env node\n${source}`); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R6-4: this shebang rewrite bumps dist/cli.js's mtime, which stampReviewSourceDigest (same file) reads as the bundle's WRITE time. A normal npm run bundle stamps before the rewrite, so that run is fine — but the rewrite poisons the anchor for any later standalone run of this script (a flow the gate's own comment — "or this script run on its own" — and the trailing isDirectRun() entry point contemplate). Probe-reproduced: on a standalone re-run, a review source edited in the window between the esbuild write and the rewrite then passes newestSource > builtAt, the stamp certifies a bundle built BEFORE that edit, and bundleStalenessNotices — the warning the review skill's Step 0 stops on — stays silent. — Failure scenario: standalone re-run after a review-source edit → the digest stamp lands on a stale bundle, and a review silently measures old behaviour. Suggested fix: preserve the bundle's mtime across the rewrite — capture fs.statSync(cliEntry) before writing and fs.utimesSync(cliEntry, atime, mtime) after (chmod does not touch mtime, so idempotence is unaffected).
中文说明
[Suggestion] R6-4：这段 shebang 重写会抬高 dist/cli.js 的 mtime，而 stampReviewSourceDigest（同一文件）正是把该 mtime 当作 bundle 的写入时间来用。正常的 npm run bundle 在重写之前就已盖章，所以那条路径没问题——但重写会污染锚点，影响之后任何单独运行本脚本的场景（这正是 gate 注释里"or this script run on its own"和文件末尾 isDirectRun() 入口所设想的流程）。已用探针复现：单独重跑时，在 esbuild 写入与 shebang 重写之间这个窗口内被修改过的 review 源文件会通过 newestSource > builtAt 判断，stamp 于是盖在了一个早于该修改构建的 bundle 上，而 bundleStalenessNotices——review skill Step 0 要求看到就必须停下来的警告——保持沉默。— 故障场景：修改 review 源文件后单独重跑脚本 → 过期 stamp 盖在陈旧 bundle 上，review 会静默地测量旧行为。修复建议：重写前后保持 bundle 的 mtime——写入前先 fs.statSync(cliEntry) 取 {atime, mtime}，写入后 fs.utimesSync(cliEntry, atime, mtime) 恢复（chmod 不影响 mtime，幂等性不受影响）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Good catch — fixed in 7c94c18. The rewrite now captures statSync(cliEntry) before writing and restores {atime, mtime} after, so the digest anchor no longer moves; the chmod stays outside the guard (it does not touch mtime, and a bundle that already carries a shebang may still arrive without the exec bit). The fixture's dist/cli.js is stamped 60s in the past and the test asserts the mtime survives the rewrite.
| // Re-running the bundle step must not stack a second shebang. | ||
| copyBundleAssets({ root: rootDir }); | ||
| expect(readFileSync(cliEntry, 'utf8')).toBe(once); |
There was a problem hiding this comment.
[Suggestion] R6-5: the second copyBundleAssets call exercises the already-has-a-shebang branch but re-asserts content only. Verified by mutation: moving fs.chmodSync(cliEntry, 0o755) inside the if (!source.startsWith('#!')) guard keeps the whole suite green (34/34), because the second call enters that branch with mode already 0o755 from the first call, and nothing re-asserts the mode afterward. — Failure scenario: a dist/cli.js that already carries a shebang but lacks the exec bit (a future esbuild banner starts emitting one, or a packaging step preserves the shebang but drops modes) hits the guard's skip branch; with chmod demoted inside the guard the exec bit is never set, shellContextEnv blanks QWEN_CODE_CLI, and every review subcommand silently runs the PATH-resolved qwen — the exact bug this block fixes. Suggested fix: before the second call reset the mode (fs.chmodSync(cliEntry, 0o644) on non-win32), then after it re-assert expect(fs.statSync(cliEntry).mode & 0o777).toBe(0o755) alongside the content check.
中文说明
[Suggestion] R6-5：第二次 copyBundleAssets 调用虽然走进了"已有 shebang"分支，但只复断了内容。已用突变验证：把 fs.chmodSync(cliEntry, 0o755) 挪进 if (!source.startsWith('#!')) 守卫内部，整套测试依旧全绿（34/34）——因为第二次调用进入该分支时 mode 已是第一次调用设置的 0o755，而且之后没有任何对 mode 的复断。— 故障场景：一个已带 shebang 但缺执行位的 dist/cli.js（比如未来 esbuild banner 开始输出 shebang，或某个打包步骤保留 shebang 但丢掉权限位）会命中守卫的跳过分支；若 chmod 被挪进守卫内部，执行位永远不会被补上，shellContextEnv 会把 QWEN_CODE_CLI 置空，每个 review 子命令都会静默执行 PATH 解析出来的 qwen——正是这段代码要修的 bug。修复建议：第二次调用前重置权限（非 win32 上 fs.chmodSync(cliEntry, 0o644)），调用后除内容外再断言 expect(fs.statSync(cliEntry).mode & 0o777).toBe(0o755)。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added in 7c94c18 — the second copyBundleAssets call now arrives at mode 0644 (non-win32) and the test re-asserts 0o755 after it, alongside the content check. Mutation-verified locally: moving fs.chmodSync inside the if (!source.startsWith("#!")) guard now fails this test (1 failed | 33 passed).
|
@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: 227 passed · 0 failed · 227 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：227 通过 · 0 失败 · 227 总计 Verification reportPR 9086 deep verification (follow-up round) —
|
| # | Previous finding | Sev. | Status at the new head |
|---|---|---|---|
| 1 | M1 mutant survived — reverting ONLY the pattern-wiring half of the capture fix stayed green, because the recompose re-read self-healed it in the suite's fixture shapes (neighbour artifact older). Classified coverage gap; the named pinning fixture was "the concurrent test with the neighbour made strictly NEWEST". | Info | Fixed. Round-5 commit 281708f4 added exactly that fixture (utimesSync … +60 s on both the neighbour's composed artifact and report). Re-measured: the same mutant now goes red on the intended assertion — harness 5, M1 (21/21, 05-mutation-matrix.png). |
| 2 | Rejected candidate: fs.chmodSync in the new bundle block with only named imports in the diff. |
Info | Stands resolved, re-verified. The pre-existing default import fs from 'node:fs' is still present (line 40); harness 4 executed the block live on all three arms. |
The previous round's four claim verifications (artifact pinning, budget filter, identity labels, bundle exec) are all re-measured below at the new head.
Scope selection
Central claim (re-measured) — a review run claims only its own target's artifacts: the composed-verdict and report scans are pinned to the run's classified target (PR / file / local, classified by the child's own parser), and the capture poll keeps re-reading while the child runs so a recomposed verdict supersedes the first snapshot.
Delta claims (new this round) —
D1 (round 6): the shebang rewrite preserves dist/cli.js's atime/mtime so stampReviewSourceDigest's staleness refusal survives a standalone re-run;
D2 (round 6): run-skill-parity.test.ts pins composedNameFor/reportPatternFor to the bundled SKILL.md templates (and actually reads them);
D3 (round 5): the pattern-wiring mutant is now killed (previous finding #1);
D4 (round 4): the no-verdict report names the expected artifact filename in prose and as expectedComposedName;
D5 (round 5): CHUNK_ROLE_RE aligns with coverage's CHUNK_RE (case/whitespace-tolerant);
D6 (round 6): package-assets pins the mtime and the 0644-arrival exec-bit re-set;
D7 (round 6): cost-ledger's first-line-only label invariant is pinned against consolidation on the scan entry point.
Out of scope (under Not covered): model-side behavior, Windows exec semantics, repo-wide gates, per-commit attribution.
Central claim — A/B proof at the new head
Harness 1 (harness/01-driver.mjs, witness 01-ab-artifact-pinning-head-vs-base.png) drives the real compiled handler of each arm — yargs module, capture poll timer, post-exit fallback scan, JSON contract, exit codes — with the child intercepted at the process.argv[1] seam (a launcher serving a scripted fake review per scenario: timed artifact writes, mtime-pinned recompose, Step-9-style sweep before exit). Control arm: a byte copy of head's packages/cli/dist with exactly the six changed runtime modules recompiled from git show HEAD^1: sources and the new agent-identity module removed (diff -rq confirmed only those families differ; rebuild script at harness/00-rebuild-base-dist.sh). Base cells encode the pre-fix bug as the predicted behavior, so a base cell that republishes the neighbour passes. 64/64, including one completion-marker assertion per cell proving the fake child ran to the end — a guard this round earned the hard way (see Methodology).
| scenario | oracle | head | base (control) |
|---|---|---|---|
| s1: PR 9014, neighbour 9013 composes first, strictly newer artifacts incl. report | event / composedPath / reportPath | COMMENT, own pr-9014 files |
❌ APPROVE, neighbour's pr-9013 files, all three fields wrong (expected red) |
s2: same target recomposes (APPROVE → REQUEST_CHANGES, mtime +60 s), sweep before exit |
final event | REQUEST_CHANGES |
❌ APPROVE — first snapshot sticks (expected red) |
| s3: bare local run, newer PR artifact present | event / composedPath | COMMENT / qwen-review-local-composed.json |
❌ APPROVE / pr-9013 (expected red) |
s4: file run src/widget.ts, newer PR artifact present |
event / composedPath | COMMENT / widget.ts artifact |
❌ APPROVE / pr-777 (expected red) |
s5: PR-42 run vs a NEWER qwen-review-pr-42.md-composed.json (file run of a file named pr-42.md) |
event / composedPath | COMMENT / own pr-42 artifact |
❌ APPROVE / the newer file-run artifact (expected red) |
s6: tab-completed src/ target |
composedPath | qwen-review-src-composed.json |
same (shape sanity, both arms) |
| s7: child exits 0 without composing (json + prose cells) | diagnostic | completed:false, expectedComposedName: qwen-review-pr-9014-composed.json, prose names the file, exit 1 |
completed:false, no expectedComposedName field, generic prose, exit 1 |
6/6 flip scenarios flipped (s1–s5 flip, s6 holds on both arms as designed). s7 proves D4 on head and its absence on base. s1's base cell reproduces the description's live evidence field for field (review run 9014 republishing pr-9013's verdict), and the sweep-before-exit in every scenario proves the verdict came from the poll capture, not the post-exit fallback.
Reviewer Test Plan, walked step by step
- "run the three named suites → 168 passed" — executed: 169/169 at this head. The plan's number was exact when written; the round-4 commit added one more test to
run.test.tsafterwards. Noted as informational, not a defect. - "bundle, then
head -c 2 dist/cli.js=#!, mode 755, direct exec" — verified on the CI-built bundle (harness 4: shebang present, 755,./dist/cli.js --versionexits 0 printing a version) and by re-running the real scripts of all three arms over reverted bundle states. - "optionally reproduce the concurrency bug on the base commit" — reproduced deterministically without a model (s1/s3/s4/s5 base cells).
Delta claims
D1 — mtime preservation is load-bearing (harness 4, 04-bundle-exec-mtime-ab.png)
Three arms, each arm's real copyBundleAssets run twice over a fake root whose review source is strictly NEWER than the bundle (so the stamp's honesty refusal is live): main-base (HEAD^1 script, no exec block), round5 (head script with the single line fs.utimesSync(cliEntry, atime, mtime); removed — the intermediate build), and head. 37/37.
| behavior | main-base | round5 (no mtime preserve) | head |
|---|---|---|---|
| run A: shebang / mode | unchanged (644, none) | added once, 755 | added once, 755 |
| run A: cli.js mtime | untouched | bumped to now | preserved (±5 ms of the 2-minute-old build time) |
| run A: stamp | refused (source newer) | refused | refused |
| run B: stamp | refused | ❌ STAMPS the stale bundle — the refusal is permanently defeated (expected red) | still refused — refusal alive |
| run B: shebang stacking / mode from 0644 | n/a | no stack, 755 restored | no stack, 755 restored |
shellContextEnv predicate on the final entry |
blanked (QWEN_CODE_CLI defeated) | kept | kept |
| direct exec | fails (EACCES) | prints body | prints body |
The round5 arm is the exact pre-round-6 state: one standalone script run silently certifies a stale bundle forever after — the staleness warning the skill's Step 0 stops on would never fire. Head's one-line fix removes it. Predicate boundaries pinned separately: 755-without-shebang → blanked; shebang-without-exec-bit → blanked; both → kept.
D2 — the skill-parity test is live and reads SKILL.md
The test exists at head, and it ran, not skipped, in the baseline (42/42 across run.test.ts + run-skill-parity.test.ts; the it.skip path only triggers when packages/core/src/skills/bundled/review/SKILL.md is absent, and it exists here — I independently confirmed the --out .qwen/tmp/qwen-review-{target}-composed.json template at SKILL.md line 843 and the three Step-8 report stems at lines 1174–1176). Mutant M4 (drift composedNameFor to qwen-review-pull-…) is killed by the parity test itself (composedNameFor renders Step 6's --out template goes red) — the oracle is the skill file, not a self-referential literal.
D5 — chunk grammar alignment (harness 3, section D)
Roles that ARE exactly a chunk assignment (chunk 3 of 7, Chunk 3 of 7, CHUNK 3 OF 7, tab-separated) resolve identically through coverage's CHUNK_RE and the parser's anchored CHUNK_ROLE_RE — the hand-edited case/space shapes the round-5 commit names no longer split into a chunk owner in one reader and a role agent in the other. One boundary shape diverges by design (chunk 3 of 7 (extra)): see Findings #2.
D7 — cost-ledger first-line-only invariant
The new cost-ledger.test.ts case pins it, and mutant M6 (switching labelOf to the whole-prompt labelFromLaunchPrompt) is killed by exactly that test (labels from the FIRST line only — an identity quoted below never wins). The two entry points' genuine difference is also asserted by the PR's own agent-identity.test.ts, which rides the green surface run.
Secondary claims re-measured
Budget placeholder filter (harness 2, 02-budget-regex-ab-and-ladder.png)
Drives the real compiled budgetGapDisclosures() of both arms. 74/74.
- 12 trailing-adverbial drop shapes flip head↔base: the three live-leaked strings from the description, the parenthesis form, the round-4 vocabulary (
belowin the completion tail,inside/the/tool[-call]qualifiers, the space-separatedtool callform in both branches), and the siblingnone — all checks finished under budget— head returns[], base keeps each (expected leaks). - 2 both-drop controls (
none — all planned checks completed,none.) hold on both arms; 7 real disclosures keep on BOTH arms (clauses past the adverbial in dash and parenthesis forms,except, the negation guard, a bare gap, awhich-clause continuation) — no over-drop. - Hostile ladder (the regex runs over model-authored lines before the 160-char truncation): four shapes × rungs at 2 k/3 k/5 k/20 k characters, one worker per rung under a 30 s cap — ≤ 1.354 ms on every rung of every shape on both arms; the round-4 rebuild of the regex from
BUDGET_QUALIFIED/COMPLETION_TAILintroduced no superlinear trend.
Identity labels (harness 3, 03-identity-label-ab.png)
31/31. Prepended-context and CRLF prompts label agent 6c (the live defect's fix, still holding); line-one identity beats a quoted identity below; round/file suffix precedence; chunk roles; null shapes. The no-drift oracle compares head's parser against a verbatim port of base's retired inline labelOf grammar (verbatim from git show HEAD^1:cost-ledger.ts, including its AUDIT_BRIEF_RE gate and the unanchored CHUNK_RE): identical output on all seven first-line shapes.
Vacuity + mutation matrix (harness 5, 05-mutation-matrix.png)
Baselines unmutated and green: run+parity 42/42, package-assets 34/34, cost-ledger green (within the surface run). Each mutant applied as an exact single-point replacement (occurrence count asserted), run against the targeted suite, source restored byte-identical (sha256-verified; git status ended clean). 21/21.
| mutant | target | result |
|---|---|---|
| M1 | run.ts: composedPatternFor → generic /^qwen-review-.*composed\.json$/ (previous round's survivor) |
killed — ignores a concurrent run's other-PR verdict… red. Previous-round finding #1 is resolved by the strictly-newer-neighbour fixture. |
| M2 | copy_bundle_assets.js: fs.utimesSync(cliEntry, atime, mtime); removed |
killed — emits an executable dist/cli.js… red on the mtime assertion |
| M3 | copy_bundle_assets.js: chmod demoted inside the shebang guard | killed — same test red on the 0644-arrival double run |
| M4 | run.ts: composed name template drifted to pull-<n> |
killed — composedNameFor renders Step 6's --out template red (parity test reads SKILL.md) |
| M5 | run.ts: no-verdict prose (expected … → (missing … |
killed — names the artifact it waited for… red |
| M6 | cost-ledger.ts: labelFromIdentityLine(identity) → labelFromLaunchPrompt(launch) |
killed — labels from the FIRST line only… red |
| P7 | run.ts: exitCodeFor blocking exit 3 → 0 (positive control) |
killed — splits completed / no-verdict / blocking red, proving the harness falsifies these suites |
No survivors this round; every guard the delta introduces is pinned by its intended test.
Findings
No blockers, no defects. Two informational items:
- (Informational) The Reviewer Test Plan's suite count ("→ 168 passed") is stale by exactly one test at the current head — measured 169/169. The round-4 commit (after the description was written) added
names the artifact it waited for when no verdict appearstorun.test.ts. The plan is otherwise fully executable; every step ran. - (Informational, boundary) The anchored
CHUNK_ROLE_REand coverage's unanchoredCHUNK_REdiverge on roles that merely CONTAIN a chunk phrase —`chunk 3 of 7 (extra)`resolves aschunk 3through coverage's whole-prompt scan but asagent chunk 3 of 7 (extra)through the parser (and base's retired cost-ledger grammar resolved it aschunk 3, so head's ledger label moves on this shape). The builder emits exactlychunk N of M(agent-prompt), so this shape is not producer-reachable; the anchored reading ("a role with extra text is not a pure chunk assignment") is defensible. Completeness reporting, not a merge condition.
Targeted gates
- Three suites named in the description (run, budget, check-coverage): 169/169 (count moved from the description's 168 — see Findings pre-release: fix ci #1).
- Mutation baselines: run + run-skill-parity 42/42; package-assets (
vitest --config scripts/tests/vitest.config.ts) 34/34. - Full
src/commands/reviewsurface: 71 files, 2520 passed, 4 skipped, 0 failed. The 4 skips are byte-identical environmental ones, confirmed via junit +skipIfguards + container probe: 3×script-lintshellcheck cases (command -v shellcheck→ absent) and 1×save-artifactcase-insensitive-alias case (Linux FS is case-sensitive). - ESLint on the 8 changed production files: clean, with a live control — a planted
const eslintLiveProbe_unused = 1;in run.ts was caught by the same invocation (@typescript-eslint/no-unused-vars), then restored;git statusclean. - Typecheck/build: covered by the pre-run
npm run buildat HEAD (every harness runs the compiled output of it); the bundle under test is the CI-builtdist/.
Not covered
- Model-side behavior — that agents honor the producer-side no-gap rule, that a real review recomposes, that live concurrent reviews interleave as scripted. The sandbox has no model. Harness 1 reproduces the shape of the pipeline (artifact scan → capture → republish) end to end through the real handler; not the model-driven trigger.
- Live GitHub reproduction of the concurrency bug — the base-arm cells carry the proof instead.
- Windows — shebang/exec-bit semantics (the package-assets mode assertion is gated off-win32 by the test itself); the description marks Windows
⚠️ . - Per-commit attribution — the snapshot lists 8 commits; the checkout is depth 2 and
git rev-list HEAD^1..HEAD^2returns 1 (the shallow-boundary artifact,git rev-parse --is-shallow-repository= true). The aggregateHEAD^1..HEADdiff was verified; the round-4/5/6 commit claims are taken from the commit messages and verified in aggregate, not per-commit. - cost-ledger end to end — its wiring is pinned by the suite + mutant M6, and the shared parser is proven no-drift; a transcript-driven cost-ledger A/B was not built.
- Repo-wide gates — only affected surfaces ran; untouched packages rely on the PR's own CI.
Methodology
Environment: the CI verify container (node:22-bookworm, node v22.23.2), detached merge-ref checkout at depth 2, npm ci + npm run build (+ bundle) completed at HEAD before the clock; no GitHub token, no writes to GitHub. Base arm: byte copy of head's packages/cli/dist with exactly the six changed runtime modules recompiled via esbuild from git show HEAD^1: sources and lib/agent-identity removed; diff -rq confirmed only those families differ (rebuild script shipped at harness/00-rebuild-base-dist.sh). Workspace-link confound checked: readlink -f node_modules/@qwen-code/qwen-code-core resolves inside this same tree for both arms, and the run-path closure imports no @qwen-code/* module (grep), so the control is clean. Harness 1 drove the handler through a launcher pinned as process.argv[1]; the first run of this round exposed a harness bug (the child JSON-parsed the script's PATH instead of its contents, and the pre-seeded neighbour artifacts made the base cells pass vacuously through the post-exit fallback) — fixed, and a fake-child completion marker is now asserted in every cell, which is why each scenario's reach is proven, not assumed. Harness 4's round5 arm differs from head by exactly one deleted line (named above). Mutations were single-point replacements with asserted occurrence counts, restored byte-identically after each (sha256-verified; final git status clean). Evidence images were produced with scripts/verify-capture.mjs from live re-runs of harnesses 1–5 (all re-runs exited 0, matching the scoring runs). Per-cell logs, harness summaries, the PR diff, and the surface junit live in logs/.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
… hold the pins to the skill Round-6 review feedback, all five findings: - copy_bundle_assets.js: preserve dist/cli.js's atime/mtime across the shebang rewrite. stampReviewSourceDigest reads that mtime as the build time, so a bumped one certifies a bundle as newer than review sources edited before it and the staleness warning the skill's Step 0 stops on never fires. A full bundle stamps before reaching here, but a standalone run of this script — a flow the gate's own comment contemplates — was exposed. - package-assets.test.js: pin both halves the block owes. The mtime is asserted against a fixture built 60s in the past, and the second run now arrives at mode 0644 so the exec bit must be re-set — demoting the chmod inside the shebang guard previously stayed green. - run-skill-parity.test.ts: new. composedNameFor and reportPatternFor encode the bundled skill's Step 6 --out template and Step 8 report stems, and were pinned only against self-referential literals. This reads the templates out of SKILL.md and renders them per target class, so a skill-side edit fails next to the code that must follow it instead of silently in a later review. - cost-ledger.test.ts: pin the first-line-only invariant — a launch whose prepended context sits above the identity line keeps the transcript's own id, never a label lifted from below. Consolidating both callers on labelFromLaunchPrompt now fails. - agent-identity.test.ts: assert the two entry points genuinely differ on that prompt, so neither caller's policy can be collapsed into the other unnoticed.
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Verified all four fixes against head 7c94c18. Approving.
1. Cross-run verdict capture (run.ts). Root cause confirmed: the unpinned COMPOSED_PATTERN + newest-mtime scan over shared .qwen/tmp. The fix pins exact filenames and classifies via the child's own parseReviewArgs (no second classifier to diverge). Cross-checked the skill oracle: SKILL.md writes --out .qwen/tmp/qwen-review-{target}-composed.json with {target} = pr-<n> / <filename> / local, exactly what composedNameFor renders, and run-skill-parity.test.ts reads those templates out of SKILL.md so future prose/code drift fails in CI. Fail-closed direction preserved: a naming drift now reports "no composed verdict was produced (expected …)" + exit 1 instead of grabbing a neighbour's verdict, and local/file runs can never claim PR-scoped artifacts. The mtime-keyed re-read correctly republishes the last recompose (partial writes retry next tick). Residual same-target/same-basename races are documented and accepted — filename identity is all the child's naming carries.
2. Placeholder budget gaps (budget.ts). The completion idiom now tolerates one end-anchored budget adverbial; the vocabulary is spelled once (BUDGET_QUALIFIED) across the stayed idiom and both completion branches, ending the hand-copied drift. Direction is safe: this only widens the DROP set for non-answers — text continuing past the adverbial still discloses (both dash and paren forms pinned in tests), the negation lookbehind and exception lookahead survive, and a real gap names a check, never the none|n/a|nothing prefix. No finding-drop path opened.
3. Disclosure labels (coverage.ts + new lib/agent-identity.ts). One format, one parser: cost-ledger keeps its first-line-only trust boundary, coverage scans for the first line-anchored identity line — the intentional difference is pinned by a dedicated test. Verified the identity-line shapes against agent-prompt.ts (chunk ${id} of ${total} and `${role}` …${roundLabel}); the anchored CHUNK_ROLE_RE resolves identically to coverage's CHUNK_RE on every role the builder produces. CRLF tolerance closes a silent parse-failure path. Fallback chain preserved, so no disclosure can lose its label.
4. dist/cli.js exec (copy_bundle_assets.js). Shebang prepended once (idempotent guard), chmod 755 outside the guard so a shebang-carrying-but-644 entry is still fixed, and mtime preserved so the review-source digest's staleness contract is not corrupted. The test pins all three properties plus the Windows carve-out; fs default import verified.
Sibling consistency: no contradiction with the merged hardening — #9065 (findings.ts untouched), #9067 (compose-review.ts change here is comment-only; ledger.ts untouched), #9095 (agent-prompt.ts edit is confined to the toolBudgetBlock disclosure sentence), #9102 and #8981 (no overlap).
CI: all 145 checks green/skipped except the still-running review-pr automation and a cancelled route check — no substantive failures.
P2/P3 nits, non-blocking: (a) in run.ts, a recompose landing in the last COMPOSED_POLL_MS window before Step 9 sweeps could in theory be republished one version stale — bounded and pre-existing in spirit, since recompose happens minutes before cleanup; (b) labelFromLaunchPrompt scans the whole prompt, so a launcher that prepends a verbatim quoted identity line could mislabel a disclosure — display-only, fallback intact, and the entry-point divergence is deliberately tested; (c) mergeable_state was still computing at review time — worth a re-check before merge.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review summary — 1 Critical (test-only), 0 in the shipped logic
Reviewed at 7c94c1872b from a clean scratch worktree off the merge base b65a0d6001, re-deriving every claim rather than reading the PR body. The four production fixes hold up. The one blocking issue is a newly added test that fails ~50% of the time, which will red test:ci on unrelated PRs.
Blocking
scripts/tests/package-assets.test.js:68—expect(...mtimeMs).toBe(builtAt.getTime())re-pins libuv'sDate→timespectruncation instead of the invariant under test. 4 failures in 8 runs through the exact CI entry point. Details, isolation probe, and a verified one-line fix are in the inline comment. Notably the production mtime-preservation is exact (0/1000 drift) — only the assertion is wrong.
What I verified and found sound
- Concurrency pin (
run.ts).classifyRunTargetdelegates toparseReviewArgs, so the parent's classification cannot diverge from the child's by construction — this genuinely closes the earlier Critical about a narrower local regex, and the four divergent shapes (/pull/9014/files,0042, uppercase URLs,docs/pull/42) are pinned inrun.test.ts:159-190. Moving from a name-shape lookahead to exactcomposedNameForfilenames also closes the second Critical (a file target namedpr-<n>-…no longer has its own artifact rejected). The residual collision classes are enumerated in the docstring rather than left to be rediagnosed, andrun-skill-parity.test.tsreads the--outtemplate and Step 8 stems out of the bundled SKILL.md — I confirmed both anchors exist at lines 835 and 1163-1165, so the test is not silently skipping. - Recompose freshness. The poll now advances only on a strictly newer mtime and only after a successful parse, so a half-written rewrite keeps the previous verdict rather than dropping to none, and a genuine recompose supersedes it.
newestArtifactSincereturning{path, mtime}avoids re-statting into the Step 9 sweep; all four call sites are updated. PLACEHOLDER_GAP_RE(budget.ts). I ran the old literal and the new built regex side by side over 45 strings. The only divergences are the 8 intended ones (the end-anchored budget adverbial, in both completion branches and thestayedbranch). Every keep-shape still keeps — includingnone — all checks completed within budget, except the Windows matrixand… within budget but the fuzz run was cut, so the negation and exception guards survive the literal →new RegExprewrite. HoistingBUDGET_QUALIFIEDremoves the three hand-copied vocabularies that had already drifted twice.- Shared identity parser (
agent-identity.ts). One grammar for coverage's disclosure labels and cost-ledger's rows, with the deliberate split kept explicit: cost-ledger feeds it only the first line, coverage scans line-anchored.CHUNK_ROLE_REnow matches coverage'sCHUNK_REshape, and the\rstrip is real ($would not anchor past a CRLF remnant and every label would fall back to prose). Theagent-prompt.tsproducer-side text is now honest about what the filter does rather than overclaiming. - Bundle fix (
copy_bundle_assets.js). The premise checks out againstshellContextEnv.ts:isUnusableScriptEntrygates onX_OKand a two-byte#!header, so a 0644 shebang-lessdist/cli.jsis blanked and"${QWEN_CODE_CLI:-qwen}"silently falls through to PATH. Shebang injection is idempotent,chmodcorrectly sits outside the guard (a shebang-bearing 0644 entry is exactly the shape that blanks the variable), and the mtime restore protectsstampReviewSourceDigest's build-time anchor on a standalone re-run. - Suite state.
src/commands/reviewat this commit: 2444 passed. Three files fail in my sandbox (comment-status.integration,test-efficacy,stale-bundle) — I re-ran them on the merge base and they fail identically there, so they are pre-existing/environmental, not this PR's. ESLint clean on all nine changed source files.
Non-blocking observation, no change requested
This PR is on its ~6th review round, so per the repo's own guidance I am deliberately not opening new Suggestion threads. One thing worth recording for later rather than fixing here: making dist/cli.js exec-able means the shape shellContextEnv's filter was originally written for — a vendored dist/cli.js handed to a shell — now passes the gate and gets exec'd via #!/usr/bin/env node instead of falling back to qwen. That is the intended direction for this repo's own bundle, but it does hand a env node dependency to any host that vendors this dist/ without node on PATH, which is the same hazard cli-entry.js:226-231 already reasons about for the standalone shim. Worth a follow-up issue, not a change to this diff.
中文说明
审查结论 —— 1 个 Critical（仅测试），生产逻辑 0 个
在合并基 b65a0d6001 之上另开干净 worktree，对 7c94c1872b 逐条重新验证，而不是采信 PR 描述。四个生产修复都站得住；唯一阻塞项是新增的一条约 50% 失败的测试，它会把 test:ci 的红灯带到无关 PR 上。
阻塞项：scripts/tests/package-assets.test.js:68 把 libuv 的 Date → timespec 截断也钉进了断言，8 次跑挂 4 次。生产侧的 mtime 保持其实是精确的（1000 次 0 漂移），错的只有断言。细节、定位探针和已验证的一行修法见行内评论。
已验证无误：classifyRunTarget 委托给 parseReviewArgs，父子分类按构造不可能分歧，四种边界形态都有测试；改用精确文件名后，名为 pr-<n>-… 的 file target 不再被自己的工件拒绝；run-skill-parity.test.ts 确实从 SKILL.md 第 835 / 1163-1165 行读取模板，不是空跑。轮询只在 mtime 严格变新且解析成功后推进，半写文件不会把 verdict 打掉。PLACEHOLDER_GAP_RE 我用新旧两版在 45 条样本上对跑，差异只有 8 条预期中的放宽，所有应保留的形态（含 … within budget, except …）都仍保留。共享 identity 解析器保留了两个读者各自的取行策略，\r 处理是真问题。bundle 修复的前提在 shellContextEnv.ts 得到确认（同时校验 X_OK 与 #! 头），幂等、chmod 位置、mtime 保持都正确。src/commands/review 2444 条通过；沙箱里挂的 3 个文件在合并基上同样挂，属既有/环境问题。九个改动文件 ESLint 全绿。
不阻塞、不要求改动：本 PR 已到第 6 轮左右，按仓库自己的规则我不再新开 Suggestion。仅记录一点供后续：让 dist/cli.js 可执行意味着 shellContextEnv 过滤器最初针对的那种形态（被交给 shell 的 vendored dist/cli.js）现在会通过闸门并经 #!/usr/bin/env node 直接执行，而不再回落到 qwen。对本仓库自己的 bundle 这是预期方向，但对任何 vendored 了这个 dist/ 且 PATH 上没有 node 的宿主，就多了一个 env node 依赖——正是 cli-entry.js:226-231 已经为 standalone shim 推理过的同一个坑。建议开 follow-up issue，而不是改这个 diff。
| const once = readFileSync(cliEntry, 'utf8'); | ||
| expect(once.startsWith('#!/usr/bin/env node\n')).toBe(true); | ||
| expect(once).toContain('console.log("bundle");'); | ||
| expect(fs.statSync(cliEntry).mtimeMs).toBe(builtAt.getTime()); |
There was a problem hiding this comment.
[Critical] The new mtime assertion is a ~50% coin flip, not a pin: it compares the filesystem's stored mtime against builtAt.getTime(), but utimesSync(cliEntry, builtAt, builtAt) on line 60 routes the Date through libuv's double seconds → timespec conversion, which truncates. For roughly half of all millisecond values the fractional part lands 1 ns low, so statSync().mtimeMs reads X - 0.001 and toBe(X) fails — and builtAt is Date.now() - 60_000, i.e. a fresh, unrepeatable value on every run.
Measured at this commit (7c94c18, macOS/APFS, Node v24), through the exact CI entry point vitest run --config ./scripts/tests/vitest.config.ts:
run 1: 34 passed run 5: 34 passed
run 2: 1 failed | 33 run 6: 1 failed | 33
run 3: 34 passed run 7: 1 failed | 33
run 4: 1 failed | 33 run 8: 34 passed
The failure message is exactly - 1786674157097 / + 1786674157096.999. A direct probe over 1000 consecutive ms values isolates where the drift comes from:
mtime !== builtAt right after the TEST's own utimesSync: 496 /1000
mtime changed by the PRODUCTION restore round-trip: 0 /1000
So the production fix is correct — fs.utimesSync(cliEntry, atime, mtime) preserves the bundle's write time exactly, 1000/1000. The flake is entirely this assertion re-pinning libuv's Date → timespec truncation, which is not the invariant under test. test:scripts runs inside test:ci and test:release, so this lands as an intermittent red on unrelated PRs; the green Ubuntu run on this commit simply drew the winning half.
Fix: assert against the mtime the filesystem actually recorded, captured once after the setup, instead of against the Date that was handed to utimesSync:
const builtAt = new Date(Date.now() - 60_000);
utimesSync(cliEntry, builtAt, builtAt);
const builtMs = fs.statSync(cliEntry).mtimeMs;
stubConsole();
...
expect(fs.statSync(cliEntry).mtimeMs).toBe(builtMs);Verified in a scratch tree at this commit: 8/8 green with that change, and it keeps its teeth — replacing fs.utimesSync(cliEntry, atime, mtime) in copy_bundle_assets.js with a no-op still fails it 3/3.
中文说明
[Critical] 这条 mtime 断言是约 50% 的抛硬币，而不是钉子：它拿文件系统存储的 mtime 去比 builtAt.getTime()，但第 60 行的 utimesSync(cliEntry, builtAt, builtAt) 会让 Date 经过 libuv 的 double 秒 → timespec 转换而被截断。约一半的毫秒值其小数部分会低 1 ns，于是 statSync().mtimeMs 读到 X - 0.001，toBe(X) 失败——而 builtAt 是 Date.now() - 60_000，每次运行都是新的、不可复现的值。
在本提交（7c94c1872b，macOS/APFS，Node v24）用 CI 的原样入口 vitest run --config ./scripts/tests/vitest.config.ts 实测：8 次里 4 次失败，报错正是 - 1786674157097 / + 1786674157096.999。用 1000 个连续毫秒值做探针可以把漂移定位清楚：测试自己的 utimesSync 造成 496/1000 不一致，而生产代码的恢复往返是 0/1000——也就是说 fs.utimesSync(cliEntry, atime, mtime) 这个修复本身是正确的，抖动完全来自这条断言把 libuv 的 Date → timespec 截断也一起钉住了，而那并不是被测的不变量。test:scripts 在 test:ci 和 test:release 里都会跑，所以这会以间歇性红灯落到无关的 PR 上；本提交上 Ubuntu 那次绿灯只是抽到了好的那一半。
修法：断言应对比文件系统实际记录下来的 mtime（在 setup 之后取一次），而不是对比交给 utimesSync 的那个 Date。已在本提交的 scratch tree 上验证：改后 8/8 全绿，且仍然有效——把 copy_bundle_assets.js 里的 fs.utimesSync(cliEntry, atime, mtime) 换成空操作后，它依然 3/3 失败。
There was a problem hiding this comment.
Confirmed and fixed in b996796 — thank you, this was a real flake I introduced and your isolation probe pinned the cause exactly. Applied your fix verbatim: capture fs.statSync(cliEntry).mtimeMs after the setup and assert against that, so the test pins only whether the rewrite moves the stored time rather than libuv's double → timespec conversion.
Reproduced your measurement on my side before and after, through the same CI entry point (vitest run --config ./scripts/tests/vitest.config.ts):
- after the fix: 10/10 green
- teeth intact: replacing the production
fs.utimesSync(cliEntry, atime, mtime)with a no-op fails it 3/3
Also noted your follow-up observation about env node: making dist/cli.js exec-able means a vendored dist/ on a host without node on PATH now execs the shebang instead of falling back to qwen — the same hazard cli-entry.js reasons about for the standalone shim. Agreed it belongs in a follow-up issue rather than this diff; I will open one.
There was a problem hiding this comment.
Follow-up issue opened: #9117 (vendored dist/cli.js and the env node dependency), crediting your observation.
There was a problem hiding this comment.
Verified at b996796c0b — fixed, and I re-derived it independently rather than re-running your numbers.
Flake gone, through the same CI entry point (vitest run --config ./scripts/tests/vitest.config.ts): 12/12 green. At the previously measured ~50% failure rate, twelve consecutive greens would happen by chance with probability ≈0.02%, so this is the cause being removed rather than a lucky streak.
Teeth intact, checked with three mutations I picked rather than the one you used — each is caught:
| Mutation | Result |
|---|---|
drop fs.utimesSync(cliEntry, atime, mtime) |
1 failed | 33 passed |
move fs.chmodSync inside the if (!source.startsWith('#!')) guard |
1 failed | 33 passed |
replace the guard with if (true) (doubled shebang) |
1 failed | 33 passed |
So the case still pins all three behaviours it is there for — mtime preservation, the exec bit on an entry that already carries a shebang, and shebang idempotence — and no longer pins libuv's conversion. The added comment above builtMs states the reason accurately; my probe measured 496/1000, so "about half of all millisecond values" is right.
Thanks for opening #9117 for the env node follow-up.
中文说明
已在 b996796c0b 验证——已修复，且我是独立重新推导的，没有直接采信你的数字。
抖动消失：用同一个 CI 入口跑 12/12 全绿。按此前实测的约 50% 失败率，连续 12 次全绿的偶然概率约 0.02%，所以这是病因被移除，而不是运气。
teeth 仍在：我另选了三种突变，全部被抓到——去掉 fs.utimesSync 恢复、把 fs.chmodSync 挪进 if (!source.startsWith('#!')) 守卫内、把守卫改成 if (true)（重复 shebang），三者均 1 failed | 33 passed。也就是说这条用例仍然钉住它该钉的三个行为（mtime 保持、已带 shebang 时仍设执行位、shebang 幂等），而不再钉住 libuv 的转换。builtMs 上方新增的注释表述准确——我的探针实测 496/1000，"约一半毫秒值"是对的。
也感谢为 env node 那条观察开了 #9117。
…runcation The assertion compared the recorded mtime against the Date handed to utimesSync, so it also pinned libuv's double-seconds → timespec conversion: about half of all millisecond values read back 1 ns low (X - 0.001), and builtAt is a fresh Date.now() - 60_000 every run — a ~50% coin flip that would have landed intermittent reds on unrelated PRs through test:ci. Capture what the filesystem actually recorded after the setup and compare against that; the invariant under test is only whether the shebang rewrite moves the stored time. 10/10 green through the CI entry point, and it keeps its teeth: removing the production fs.utimesSync(cliEntry, atime, mtime) restore fails it 3/3.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Approve — the one blocking finding is fixed and independently re-verified
Re-reviewed at b996796c0b. The delta since my last pass (7c94c1872b) is exactly one file, scripts/tests/package-assets.test.js: the mtime assertion now compares against the mtime the filesystem actually recorded, plus a comment explaining why. Nothing in the shipped logic moved, so the verification from my previous pass still applies to it unchanged.
The blocking finding
Fixed. Re-derived independently rather than re-running the author's numbers, through the exact CI entry point:
- 12/12 green. At the previously measured ~50% failure rate, twelve consecutive greens would occur by chance with probability ≈0.02% — this is the cause being removed, not a lucky streak.
- Teeth intact, under three mutations I chose rather than the one the fix was validated with: dropping
fs.utimesSync(cliEntry, atime, mtime), movingfs.chmodSyncinside theif (!source.startsWith('#!'))guard, and replacing that guard withif (true). Each fails1 failed | 33 passed. The case still pins all three behaviours it exists for — mtime preservation, the exec bit on an entry that already carries a shebang, and shebang idempotence.
The new comment above builtMs is accurate: my probe measured 496/1000, so "about half of all millisecond values" is right.
Every unresolved Critical on this PR, checked against the code as it stands
All 31 threads here read isResolved: false, so that flag carries no information — each Critical is ruled on by reading b996796c0b, not by its thread state.
| Critical | Ruling | Evidence at b996796c0b |
|---|---|---|
run.ts:94 — prNumberFromTarget re-derives PR classification with a narrower regex than parse-args.ts' PR_URL_RE |
fixed by this diff | the symbol no longer exists; run.ts:107 calls parseReviewArgs, the same classifier the child names its artifacts from, so divergence is impossible by construction. The four divergent shapes are pinned at run.test.ts:159-190 |
run.ts:124 — the (?!pr-\d+-) name-shape lookahead rejects a file run's OWN composed artifact |
fixed by this diff | the composed path is now exact filenames from composedNameFor, no lookahead. The only surviving pr-\d occurrences are a docstring describing the removed pin and the local report pattern, a separate informational surface; run.test.ts pins both directions for a file target named pr-42-notes.ts and one named pr-1234.md |
package-assets.test.js:68 — the ~50% mtime flake |
fixed by this diff | measured above |
Current-head verification
- Named suites: 442 passed (
run,budget,check-coverage,agent-identity,cost-ledger,run-skill-parity,compose-review). scripts/tests/package-assets.test.js: 34 passed, 12 runs in a row.- ESLint and Prettier clean on the changed file.
- CI: everything finished is green;
Test (ubuntu-latest, Node 22.x)andreview-prwere still in progress when I submitted — the merge gate covers those.
Approving on the substance. Six review rounds in, I am deliberately opening no new Suggestion threads; the env node observation from my last pass is tracked in #9117 rather than widening this diff.
中文说明
批准 —— 唯一阻塞项已修复并经独立复验
在 b996796c0b 重新审查。相对上一轮 7c94c1872b 的增量恰好只有 scripts/tests/package-assets.test.js 一个文件：mtime 断言改为对比文件系统实际记录的值，并加了一段说明注释。生产逻辑没有任何改动，所以上一轮对它的验证结论原样继续成立。
阻塞项已修复，且我是独立重推的，不是复读作者的数字：用 CI 原样入口跑 12/12 全绿（按此前约 50% 的失败率，连续 12 次全绿的偶然概率约 0.02%）；并用我自己另选的三种突变确认 teeth 仍在——去掉 fs.utimesSync 恢复、把 fs.chmodSync 挪进 shebang 守卫内、把守卫改成 if (true)，三者均 1 failed | 33 passed。这条用例仍钉住它该钉的三个行为，而不再钉住 libuv 的转换。新增注释表述准确（我实测 496/1000）。
本 PR 上所有未关闭的 Critical，逐条对当前代码裁定（此处 31 条线程全部显示 isResolved: false，该标志不携带信息，故一律以读 b996796c0b 的代码为准）：
run.ts:94（分类正则比parse-args.ts窄）——已由本 diff 修复：该符号已不存在，run.ts:107调用parseReviewArgs，与子进程命名工件所依据的分类器同一个，按构造不可能分歧；四种边界形态在run.test.ts:159-190有钉。run.ts:124（(?!pr-\d+-)形状钉子会拒绝 file run 自己的工件）——已由本 diff 修复：composed 路径改为composedNameFor的精确文件名，无 lookahead；残留的pr-\d只是描述已移除钉子的注释，以及 local report 这一独立的信息性表面，且pr-42-notes.ts与pr-1234.md两个方向都有测试钉住。package-assets.test.js:68（约 50% 抖动）——已由本 diff 修复，见上。
当前 head 验证：指定套件 442 条通过；package-assets 34 条通过、连跑 12 次；改动文件 ESLint 与 Prettier 全绿；CI 已完成的项全绿，提交时 Test (ubuntu-latest, Node 22.x) 与 review-pr 仍在进行中，由合并闸门把关。
基于实质内容批准。已到第 6 轮，我不再新开 Suggestion；上一轮的 env node 观察由 #9117 跟踪，不再扩大本 diff。
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 reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) jobs were skipped in CI; the OS matrix did not run locally (Linux only).
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": nothing — stayed within budget, no checks left incomplete.; "You are review agent reverse-audit — Reverse audit agent…": none — finished all checks within budget (~14 tool calls).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) jobs were skipped in CI; the OS matrix did not run locally (Linux only)。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：nothing — stayed within budget, no checks left incomplete.；"You are review agent reverse-audit — Reverse audit agent…"：none — finished all checks within budget (~14 tool calls).；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const trimmed = t.path.replace(/[\\/]+$/, ''); | ||
| return { kind: 'file', base: trimmed.split(/[\\/]/).pop() || trimmed }; |
There was a problem hiding this comment.
[Suggestion] A separators-only target (/, //, \) produces the empty base the adjacent comment declares impossible — t.path.replace(/[\\/]+$/, '') strips trailing separators, but an all-separator path yields '', and trimmed.split(/[\\/]/).pop() || trimmed is '', so composedNameFor pins qwen-review--composed.json — a name no child artifact can ever carry. Concrete cost: qwen review run / passes buildReviewPrompt's guard (no whitespace, leading dash, or quote), the parent pins that unmatchable name, a child launch is wasted, and the run reports "no composed verdict was produced (expected .qwen/tmp/qwen-review--composed.json)" with exit 1 instead of failing fast — violating the invariant the comment three lines up states. Probe-verified at this commit: red on the PR code, green with the guard fix below.
if (
/\s/.test(args.target) ||
args.target.startsWith('-') ||
/['"]/.test(args.target) ||
/^[\\/]+$/.test(args.target)
) {中文说明
[Suggestion] 仅由路径分隔符组成的 target（/、//、\）会产生相邻注释声称不可能的空 base——t.path.replace(/[\\/]+$/, '') 只去掉尾部分隔符，纯分隔符路径得到 ''，trimmed.split(/[\\/]/).pop() || trimmed 也是 ''，于是 composedNameFor 钉出 qwen-review--composed.json——一个任何子进程工件都不可能携带的名字。具体代价：qwen review run / 能通过 buildReviewPrompt 的守卫（无空白、无前导连字符、无引号），父进程钉上这个不可匹配的名字，白白启动一次子进程，最后报告 "no composed verdict was produced (expected .qwen/tmp/qwen-review--composed.json)" 并 exit 1，而不是快速失败——恰好违反了三行上方注释声明的不变量。已在本提交上探针验证：原代码红，加上下面的守卫修复后绿。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in da50ceb — applied your guard verbatim; buildReviewPrompt now refuses a separators-only target, so /, // and \ fail fast instead of burning a child review on an unmatchable pin. Pinned as a test case, mutation-verified: dropping the guard fails it.
| const m = | ||
| /--out\s+\.qwen\/tmp\/(qwen-review-\{target\}-composed\.json)/.exec( | ||
| skill as string, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The composed-name oracle matches the SKILL.md --out filename as an unanchored prefix, so a skill edit that appends a suffix to the artifact name leaves this oracle green while the pin drifts — one direction of the exact defect class this test exists to catch. Concrete cost: if Step 6 becomes --out .qwen/tmp/qwen-review-{target}-composed.json.tmp (e.g. an atomic write-then-rename), the capture still matches as a prefix, the test passes against the stale literal, the child writes the suffixed artifact and the parent pins the unsuffixed name — every affected review run reports "no composed verdict was produced" and exits 1 after the review completed. Probe-verified end-to-end in the vitest harness: with that drift applied the current test still passes 2/2; the anchored capture below flips red under the drift and stays green without it. (The sibling Step-8 stems oracle is not exposed — its capture ends at the closing backtick.)
| const m = | |
| /--out\s+\.qwen\/tmp\/(qwen-review-\{target\}-composed\.json)/.exec( | |
| skill as string, | |
| ); | |
| const m = | |
| /--out\s+\.qwen\/tmp\/(qwen-review-\{target\}-composed\.json)(?![\w.])/.exec( | |
| skill as string, | |
| ); |
中文说明
[Suggestion] composed 文件名的对照 oracle 以未加尾锚的前缀方式匹配 SKILL.md 的 --out 文件名，因此给工件名追加后缀的 skill 修改不会让本 oracle 变红，钉选却已漂移——这正是本测试要消灭的缺陷类的一个方向。具体代价：若 Step 6 变为 --out .qwen/tmp/qwen-review-{target}-composed.json.tmp（例如原子性先写后改名），捕获组仍按前缀匹配成功，测试继续拿着过期字面量通过；子进程写出带后缀的工件，父进程钉的是不带后缀的名字——每次受影响的 review run 都会在 review 实际完成后报告 "no composed verdict was produced" 并 exit 1。已在 vitest 环境中端到端探针验证：施加该漂移后当前测试仍 2/2 通过；下面的尾锚捕获在漂移下变红、无漂移时保持绿。（Step 8 报告 stem 的姊妹 oracle 不受影响——其捕获在反引号处收尾。）
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Fixed in da50ceb — applied your (?![\w.]) anchor. Mutation-verified end to end: rewriting SKILL.md Step 6 to …composed.json.tmp now fails the oracle (it passed 2/2 before the anchor), and the unmodified skill stays green.
There was a problem hiding this comment.
Same correction: da50ceba73 missed the squash merge by 42 seconds, so the anchor is not in main. Carried unchanged in #9128, re-verified there (mutation: the …composed.json.tmp drift fails the oracle).
|
@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: 230 passed · 0 failed · 230 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：230 通过 · 0 失败 · 230 总计 Verification reportPR 9086 deep verification (follow-up round 2) —
|
| # | Previous finding | Sev. | Status at the new head |
|---|---|---|---|
| 1 | M1 mutant survived — reverting ONLY the pattern-wiring half of the capture fix stayed green (neighbour fixture older at that time). Named pinning fixture: the concurrent test with the neighbour made strictly NEWEST. | Info | Fixed, re-verified. The round-5 fixture is in place at this head; re-measured mutant M1 (composedPatternFor → generic scan) is killed on ignores a concurrent run's other-PR verdict… — harness 5, 16/16 (05-mutation-matrix-all-killed.png). |
| 2 | Rejected candidate: fs.chmodSync with only named imports in the diff. |
Info | Stands resolved, re-verified. The default import fs from 'node:fs' is present (line 35 of scripts/copy_bundle_assets.js); harness 4 executed the chmod path live on all three arms (exec bit restored from a 0644 arrival on every arm that has the block). |
| 3 | Reviewer Test Plan's suite count "→ 168 passed" stale by one test. | Info | Stands, re-measured. G1 measured 169/169 at this head — the plan number is still one behind (round-4 added names the artifact it waited for… after the description was written). Every step of the plan remains executable; all ran. |
| 4 | Boundary: anchored CHUNK_ROLE_RE vs coverage's unanchored CHUNK_RE diverge on `chunk 3 of 7 (extra)` (head agent chunk 3 of 7 (extra) vs base/coverage chunk 3). |
Info | Stands, re-measured. Harness 3 section C re-drove the shape on both arms; the builder emits exactly chunk N of M, so it is not producer-reachable. Completeness reporting, not a merge condition. |
Scope selection
Central claim (re-measured) — a review run claims only its own target's artifacts: the composed-verdict and report scans are pinned to the run's classified target, and the capture poll keeps re-reading while the child runs so a recomposed verdict supersedes the first snapshot.
Delta claim (this round's headline, D8) — the package-assets mtime assertion no longer pins libuv's double-seconds → timespec truncation: it compares the post-rewrite read-back against what the filesystem ACTUALLY recorded after setup, so it is deterministic across both truncation classes, and it keeps its teeth (removing the production fs.utimesSync restore still fails it).
Carried-forward secondary claims (re-measured) — the budget placeholder filter drops the live-leaked completion-idiom shapes without over-dropping real gaps; identity labels prefer the brief codename over a launcher-prepended first line, with no drift against the retired base grammar; the bundle emits an executable dist/cli.js and preserves its write time.
Out of scope (under Not covered): model-side behavior, Windows exec semantics, repo-wide gates, per-commit attribution.
Delta claim — the round-7 mtime assertion
Mechanism, measured by census (harness 6, 06-delta-mechanism-census-and-teeth.png)
Drove the REAL fs.utimesSync(file, new Date(T), new Date(T)) → fs.statSync round-trip (the exact test sequence, plus the production stat→Dates→rewrite→utimesSync round-trip from copy_bundle_assets.js) over 5000 timestamps — 5 seconds × all 1000 millisecond residues, anchored at now − 60 s like the test:
| measured fact (node v22.23.2, ext4, this container) | value |
|---|---|
| setup stores a timespec LOWER than intended | 2480/5000 = 49.6% (delta distribution: −1024 ns ×2170, −768 ns ×310) |
| production round-trip moves the stored time | 0/5000 (reading the recorded timespec back through statSync().mtime Dates and re-applying is idempotent here) |
round-6-style comparison (final mtimeMs vs the Date handed to utimesSync) fails |
2480/5000 — EXACTLY the truncation class (set equality asserted) |
head-style comparison (final mtimeMs vs the recorded post-setup mtimeMs) fails |
0/5000 |
This is the coin flip the commit describes, deterministically partitioned: whether a given run flakes depends only on where Date.now() lands among the millisecond residues.
A/B at the real vitest entry point (harness 7, 07-delta-vitest-ab-flake.png)
Scratch copies generated from the REAL test file by asserted string replacements (nothing retyped); the round-6 arm reverts exactly the delta the commit describes (drop the post-setup capture, compare against builtAt.getTime()). Cells ran through vitest run --config scripts/tests/vitest.config.ts — the test:scripts half of test:ci:
| cell | reconstructed round-6 form | head form |
|---|---|---|
| forced TRUNC-class clock (T=1786683292402, stores 768 ns low) | ❌ red — AssertionError: expected 1786683292401.999 to be 1786683292402 (expected red; the failure is the intended mtime assertion, values printed) |
✅ green |
| forced EXACT-class clock (T=1786683292400, stores exact) | ✅ green | ✅ green |
| natural wall clock, 12 runs | 7/12 RED — the flake as CI sees it | 0/12 red |
| 5000-point census | 2480/5000 fail | 0/5000 fail |
The red cell's failure value — one thousandth of a millisecond low — is the commit's parenthetical "(X - 0.001)" reproduced byte for byte. Reconstruction caveat, stated plainly: the round-6 file itself is unreachable (depth-2 checkout); the reverted form is the minimal inverse of the described delta, and it is validated BY BEHAVIOR — it flakes at the described ~50% rate (7/12 against the census's 49.6%) with the described failure shape, while the alternative accessor (mtime.getTime()) provably cannot flake (0/5000 in the census), so no other candidate form fits the description.
Teeth at the new head (harness 8, same capture: 06-delta-mechanism-census-and-teeth.png)
Removed the production line fs.utimesSync(cliEntry, atime, mtime); from scripts/copy_bundle_assets.js and ran the REAL unmodified test 3× through the same entry point: 3/3 red, every failure on the intended mtime assertion (expected/received values printed — not an import or fixture break), matching the commit's claim. Source restored byte-identical (sha256-verified); git status ended clean. The teeth are in the production line; the round-7 change moved only what the test compares, not what it can catch.
Central claim — artifact-pinning A/B re-measured (harness 1, 01-ab-artifact-pinning-head-vs-base.png)
Drove the real compiled handler of each arm (yargs module, capture poll, post-exit fallback, JSON contract, exit codes) with the child intercepted at the process.argv[1] seam (a scripted fake review per scenario). Control arm: a byte copy of head's packages/cli/dist with exactly the six changed runtime modules recompiled from git show HEAD^1: sources and the new agent-identity module removed — diff -rq confirmed only those families differ (harness/00-rebuild-base-dist.sh). Base cells encode the pre-fix bug (generic COMPOSED_PATTERN, first-snapshot capture, /\.md$/ report scan — all confirmed present in HEAD^1's run.ts) as the predicted behavior. 66/66, including a fake-child completion-marker assertion per cell proving each scenario ran to the end:
| scenario | oracle | head | base (control) |
|---|---|---|---|
| s1: PR 9014, neighbour 9013 strictly newer (composed + report) | event / composedPath / reportPath | COMMENT, own pr-9014 files |
❌ APPROVE, neighbour's pr-9013 files, all three fields wrong (expected red) |
s2: same target recomposes (APPROVE → REQUEST_CHANGES), sweep before exit |
final event | REQUEST_CHANGES |
❌ APPROVE — first snapshot sticks (expected red) |
| s3: bare local run, newer PR artifact present | event / composedPath | COMMENT / qwen-review-local-composed.json |
❌ APPROVE / pr-9013 (expected red) |
s4: file run src/widget.ts, newer PR artifact present |
event / composedPath | COMMENT / widget.ts artifact |
❌ APPROVE / pr-777 (expected red) |
s5: PR-42 run vs a NEWER qwen-review-pr-42.md-composed.json |
event / composedPath | COMMENT / own pr-42 artifact |
❌ APPROVE / the newer file-run artifact (expected red) |
s6: tab-completed src/ target |
composedPath | qwen-review-src-composed.json |
same (shape sanity, both arms) |
| s7: child exits 0 without composing (json + prose cells) | diagnostic | completed:false, expectedComposedName: qwen-review-pr-9014-composed.json, prose names the file, exit 1 |
completed:false, no expectedComposedName field, generic prose, exit 1 |
5/5 flip scenarios flipped (s1–s5); s6 holds on both arms as designed; s7 proves the round-4 diagnostic on head and its absence on base. The sweep-before-exit in every scenario proves the verdict came from the poll capture, not the post-exit fallback.
Reviewer Test Plan, walked step by step
- "run the three named suites → 168 passed" — executed: 169/169 at this head (plan number stale by one; finding 如何自定义密钥文件 .env可能与其他文件冲突 #3 above).
- "bundle, then
head -c 2 dist/cli.js=#!, mode 755, direct exec" — executed verbatim:npm run bundleat this head (stamped 74 files),head -c 2printed#!, mode-rwxr-xr-x,./dist/cli.js --versionexec'd directly printing0.21.11. Also re-proven on all three harness-4 arms. - "optionally reproduce the concurrency bug on the base commit" — reproduced deterministically without a model (s1/s3/s4/s5 base cells).
Secondary claims re-measured
Budget placeholder filter (harness 2, 02-budget-regex-ab-and-ladder.png)
Drives the real compiled budgetGapDisclosures() of both arms. 76/76.
- 11 trailing-adverbial/qualifier drop shapes flip head↔base: the live-leaked
none — all checks above completed within budget.and its siblings, the parenthesis form, the round-4 vocabulary (below/inside/the/tool[-call]qualifiers, the space-separatedtool callform in both branches,stayed inside the tool-call budget), andnone — all checks finished under budget— head returns[], base keeps each (expected leaks). One calibration note:N/A - stayed below budgetwas entered as a flip and measured both-drop — base's stayed idiom already acceptedunder|within|below(verified inHEAD^1'sPLACEHOLDER_GAP_RE); it is reported as a control, the honest reading. - 4 both-drop controls and 7 real disclosures keep on BOTH arms (clauses past the adverbial in dash and parenthesis forms,
except/butexceptions, the negation guard, two bare real gaps) — no over-drop. - Hostile ladder (the regex runs over model-authored lines before the 160-char truncation): four shapes × rungs at 2 k/3 k/5 k/20 k characters, one worker per rung under a 30 s cap, max of 3 reps — ≤ 1.92 ms on every rung of every shape on both arms; no superlinear trend at the new head.
Identity labels (harness 3, 03-identity-label-ab.png)
21/21. Prepended-context prompts label agent 6c on head while the base grammar returns the truncated PR quote (the live defect, still flipped); CRLF-recorded prompts parse on head with no prose fallback (base's cost-ledger is insensitive to CRLF by its unanchored role regex — measured, not a flip; base's COVERAGE never parsed identity at all, its label() took split('\n')[0] verbatim, confirmed in HEAD^1); round/file suffix precedence; chunk roles incl. case/whitespace variants; quoted-identity-below never wins. The no-drift oracle compares head's parser against a verbatim port of base's retired labelOf grammar (from git show HEAD^1:cost-ledger.ts, including its AUDIT_BRIEF_RE gate): identical output on all six first-line shapes; the seventh (chunk-with-extra-text) diverges by design (previous finding #4).
Bundle exec + mtime, three arms (harness 4, 04-bundle-exec-mtime-three-arms.png)
Each arm runs ITS OWN real copyBundleAssets twice over a fake root whose review source is strictly NEWER than the bundle (the stamp's honesty refusal live). 27/27.
| behavior | main-base (HEAD^1 script) | round5 (head minus the mtime-restore line) | head |
|---|---|---|---|
| run A: shebang / mode | none / 644 | added once / 755 | added once / 755 |
| run A: mtime | untouched (no block) | moved +120 s | preserved (< 5 ms of the 2-minute-old build time) |
| run A: stamp | refused (source newer) | refused | refused |
| run B: stamp | refused | ❌ STAMPS the stale bundle — the refusal permanently defeated (expected red) | still refused — refusal alive |
| run B: shebang stacking / mode from 0644 | n/a | no stack / 755 restored | no stack / 755 restored |
real getShellContextEnvVars on the final entry |
BLANKED (QWEN_CODE_CLI defeated) | kept | kept |
| direct exec | EACCES | prints body | prints body |
Predicate boundaries pinned through the real getShellContextEnvVars: 755-without-shebang → blanked; shebang-without-exec-bit → blanked; both → kept.
Vacuity + mutation matrix (harness 5, 05-mutation-matrix-all-killed.png)
Baselines unmutated and green at this head (run + run-skill-parity + cost-ledger; package-assets 34/34). Each mutant an exact single-point replacement (occurrence count asserted), run against the targeted suite, source restored byte-identical (sha256-verified). 16/16 — no survivors.
| mutant | target | result |
|---|---|---|
| M1 | run.ts: composedPatternFor → generic /^qwen-review-.*composed\.json$/ (previous round's survivor) |
killed — ignores a concurrent run's other-PR verdict… red |
| M2 | copy_bundle_assets.js: fs.utimesSync(cliEntry, atime, mtime); removed |
killed — emits an executable dist/cli.js… red on the mtime assertion |
| M3 | copy_bundle_assets.js: chmod demoted inside the shebang guard | killed — same test red on the 0644-arrival double run |
| M4 | run.ts: composed name template drifted to pull-<n> |
killed — composedNameFor renders Step 6's --out template red (the parity test reads SKILL.md — the oracle is the skill file, not a self-referential literal) |
| M5 | run.ts: no-verdict prose (expected … → (missing … |
killed — names the artifact it waited for… red |
| M6 | cost-ledger.ts: labelOf consolidated onto the whole-prompt labelFromLaunchPrompt |
killed — labels from the FIRST line only… red |
| P7 | run.ts: exitCodeFor blocking exit 3 → 0 (positive control) |
killed — splits completed / no-verdict / blocking red, proving the harness falsifies these suites |
The delta test's own teeth are the harness-8 row above (3/3 red on the intended assertion with the production restore removed).
Corrections
- Correction to the round-7 commit message's unit. It describes the truncation as "about half of all millisecond values land 1 ns low (X - 0.001)". The census measures the delta as 768–1024 ns (−1024 ns ×2170, −768 ns ×310 of 5000) — i.e. ≈ 0.001 milliseconds, so the parenthetical "(X - 0.001)" with X in ms is the accurate reading and the red cell reproduced it exactly (
expected 1786683292401.999 to be 1786683292402); the "1 ns" wording understates the measured delta ~1000×. The mechanism, the ~half distribution (49.6%), and the fix are all verified as described — this is a prose-unit correction, not a request to change code.
Findings
No blockers, no defects. Informational items:
- (Informational, carried over, re-measured) The Reviewer Test Plan's suite count ("→ 168 passed") remains stale by exactly one test — measured 169/169 at this head. Every step of the plan is otherwise executable and ran.
- (Correction, above) The commit message's "1 ns" vs the measured 768–1024 ns delta.
- (Informational, boundary, carried over, re-measured) The anchored
CHUNK_ROLE_REand coverage's unanchoredCHUNK_REstill diverge on roles that merely CONTAIN a chunk phrase (`chunk 3 of 7 (extra)`→ headagent chunk 3 of 7 (extra), coveragechunk 3). The builder emits exactlychunk N of M, so the shape is not producer-reachable; the anchored reading is defensible. Completeness reporting, not a merge condition.
Targeted gates (09-targeted-gates.png)
- G1 — three suites named in the description (run, budget, check-coverage): 169/169, 0 failed.
- G2 — full
scripts/tests/package-assets.test.js(scripts config): 34/34, 0 failed, 0 skipped (container uid 1000, so the chmod-gated case ran). - G3 — full
src/commands/reviewsurface: 71 files, 2522 passed, 4 skipped, 0 failed. The 4 skips are environmental: 3×script-lintshellcheck cases (shellcheckabsent from the image) and 1×save-artifactcase-insensitive-alias case (Linux FS is case-sensitive) — attributed from the run log. - G4 — ESLint on the 7 changed production
.tsfiles: clean, with a live control — a plantedconst eslintLiveProbe_unused = 1;in run.ts was caught by the same invocation, then the file was restored byte-identical (sha256-verified). - Typecheck/build: covered by the pre-run
npm run build(+ bundle) at HEAD; every harness ran the compiled output of it.
Not covered
- Model-side behavior — that agents honor the producer-side no-gap rule, that a real review recomposes, that live concurrent reviews interleave as scripted. The sandbox has no model. Harness 1 reproduces the shape of the pipeline (artifact scan → capture poll → republish) end to end through the real handler; not the model-driven trigger.
- The round-6 test file itself — unreachable at depth 2 (
b996796c0b^1is beyond the shallow boundary). The round-6 A/B arm is the minimal inverse reconstruction of the described delta, validated by behavior (7/12 natural flake against the census's 49.6%; the failure value matches "(X - 0.001)"; the only alternative accessor provably cannot flake). - Per-commit attribution — the snapshot lists 9 commits; the checkout is depth 2 and
git rev-list HEAD^1..HEAD^2returns 1 (the shallow-boundary artifact,git rev-parse --is-shallow-repository= true). The aggregateHEAD^1..HEADdiff was verified; the round-4–7 commit claims are taken from the commit messages and verified in aggregate, not per-commit. - Windows — shebang/exec-bit semantics (the package-assets mode assertion is gated off-win32 by the test itself); the description marks Windows
⚠️ . - Live GitHub reproduction of the concurrency bug — the base-arm cells carry the proof instead.
- cost-ledger end to end — its wiring is pinned by the suite + mutant M6, and the shared parser is proven no-drift against the retired grammar; a transcript-driven cost-ledger A/B was not built.
- Repo-wide gates — only affected surfaces ran; untouched packages rely on the PR's own CI.
Methodology
Environment: the CI verify container (node:22-bookworm, node v22.23.2, npm 10.9.8, ext4), detached merge-ref checkout at depth 2, npm ci + npm run build (+ bundle) completed at HEAD before the clock; no GitHub token, no writes to GitHub. Base arm for the handler A/B: byte copy of head's packages/cli/dist with exactly the six changed runtime modules recompiled via esbuild from git show HEAD^1: sources and lib/agent-identity removed; diff -rq confirmed only those families differ (script shipped at harness/00-rebuild-base-dist.sh). Workspace-link confound checked: readlink -f node_modules/@qwen-code/qwen-code-core resolves inside this same tree, and the run-path closure imports no @qwen-code/* module (grep over run.js/parse-args.js/paths.js/review-settings.js/stale-bundle.js/stdioHelpers.js — one comment hit only), so the control is clean. Harness 1 drove each arm's compiled handler through a launcher pinned as process.argv[1], with a fake-child completion marker asserted in every cell. Harness 7's scratch cells were generated from the real test file by asserted string replacements, lived under scripts/tests/zz-verify-*.test.js for the run, and were removed afterwards (git status clean). Mutations were single-point replacements with asserted occurrence counts, restored byte-identical after each (sha256-verified; final git status clean). Evidence images were produced with scripts/verify-capture.mjs from live re-runs of all nine harnesses (every re-run exited 0, matching the scoring runs). Per-cell logs, harness summaries/tallies, and the raw vitest/eslint outputs live in logs/; assertions.json is the sum of the harness tallies (66 + 76 + 21 + 27 + 16 + 4 + 6 + 4 + 10 = 230).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. ✅
|
Released in v0.21.12. |
4 participants
What this PR does
Fixes four defects in the
/reviewpipeline that were caught by runningqwen review runend-to-end against three real open PRs (#9013, #9014, #9045) with qwen3.8-max, and pins each fix with regression tests. All four were observed live, not hypothesized.review runrepublished a concurrent run's verdict (run.ts). The composed-verdict scan matched anyqwen-review-*composed.jsonin the shared.qwen/tmpand took the newest, so with two reviews running in one repo, whichever composed first was captured by the other run. The scan (poll and post-exit fallback) is now pinned to the run's own target — a PR run only claimsqwen-review-pr-<n>-…composed.jsonand…-pr-<n>.md, a local/file run refuses PR-scoped artifacts — and the poller keeps re-reading while the child runs, because one live review legitimately recomposed its verdict 12 minutes after the first write and the first-snapshot capture would have republished the superseded one.Placeholder budget gaps reached posted bodies (
lib/budget.ts).PLACEHOLDER_GAP_REdrops "nothing to disclose" non-answers, but its completion idiom required the completion word to end the text, sonone — all checks … completed within budget.slipped through into public verdict bodies. The completion branches now tolerate one end-anchored budget adverbial (within/under/inside (the) (tool[-call]) budget); a clause continuing past it still keeps.Gap disclosures labelled by a PR quote instead of an agent name (
lib/coverage.ts).label()took the launch prompt's first line, and launchers prepend a context sentence — so all twelve finders of one run shared a single PR-summary first line and every budget-gap disclosure rendered as the same truncated PR quote. The brief's codename line (You are review agent `6c`) is now matched anywhere in the prompt and wins; the first line remains the fallback.dist/cli.jswas not executable, silently defeating QWEN_CODE_CLI (scripts/copy_bundle_assets.js).shellContextEnvblanks aQWEN_CODE_CLIthat a POSIX shell cannot exec (no shebang, or no execute bit) so"${QWEN_CODE_CLI:-qwen}"doesn't die on exit 126 — but the bundle step emitteddist/cli.jsat mode 644 with no shebang, so the variable arrived blank in every shell of a session launched off the bundle and everyqwen review …subcommand fell back to the PATH's global install. The bundle now emitsdist/cli.jswith#!/usr/bin/env nodeand mode 755.Why it's needed
A live round of three parallel
qwen review runs produced concretely wrong outputs on all four paths: two of the three runs reported a neighbour PR's verdict as their own (one externally reported REQUEST_CHANGES for a review whose own saved report says Comment); three "none — …" placeholder lines were rendered into two public verdict bodies as coverage gaps; every gap disclosure in one run carried the same unreadable truncated PR quote as its agent label; and all subcommands of all three runs silently executed the machine's auto-updated global install instead of the tree they were launched from, which defeats the skill's entire version-skew defense for source-tree launches. Each of these misleads either a CI consumer ofreview run --json, a PR author reading the posted body, or a maintainer measuring a build.Reviewer Test Plan
How to verify
Run the pinned regression suites — all four fixes have dedicated cases (concurrent-artifact isolation, recompose freshness, the live placeholder strings kept and dropped, the codename label, pattern pinning):
cd packages/cli && npx vitest run src/commands/review/run.test.ts src/commands/review/lib/budget.test.ts src/commands/review/check-coverage.test.ts→ 168 passed. For the bundle fix:npm run build && npm run bundle, thenhead -c 2 dist/cli.jsprints#!,ls -l dist/cli.jsshows mode 755, and./dist/cli.js --versionexecs directly. Optionally reproduce the concurrency bug on the base commit: start tworeview runs of different PRs in one repo and observe the later-starting run'scomposedPathpoint at whichever PR composes first; on this branch each run only ever claims its ownqwen-review-pr-<n>-…artifacts.Evidence (Before & After)
Before (live run, base commit):
review run 9014 --jsonreturned"event": "REQUEST_CHANGES", "composedPath": ".../qwen-review-pr-9013-composed.json"while its own saved report concludedVerdict: Comment; the posted body of #9013 carriedNot explored to full depth (tool budget reached): …: none — all checks above completed within budget.; the process table showedfnm …/bin/qwen review build-testresolving to~/.qwen/updates/npm/…/0.21.11(global install) inside a review launched from a freshly bundled tree whosedist/cli.jswas-rw-r--r--with no shebang. After: the new unit tests pin each of these shapes (composedPatternFor('9014')rejectsqwen-review-pr-9013-composed.json;budgetGapDisclosures('Budget gap: none — all checks above completed within budget.')returns[]while the same text with a trailing clause is kept; a prompt with a prepended context line labels asagent 6c), and the bundle emits#!/usr/bin/env node+ mode 755, verified by direct exec and by the exactshellContextEnvusability predicate.Tested on
Environment (optional)
macOS (darwin 25.6), Node v24.18.1, npm 11.16.0; live-run evidence gathered with
node dist/cli.js review run <pr> --jsonunder tmux against dashscope qwen3.8-max.Risk & Scope
qwen-review-pr-…would no longer be picked up — no shipped path produces that shape. The placeholder-gap widening is end-anchored and negation-guarded, so a real gap that mentions budget mid-sentence still discloses; live keep-shapes are pinned in tests.unreviewed-dimensioncap on this repo, small-PR high-effort latency, headless progress opacity, model-improvised report filename stems) need design decisions and are deliberately not patched here.dist/cli.jsgaining a shebang and the execute bit is additive; npm-published entry points are unaffected.Linked Issues
None — the defects were found by direct live-run observation against PRs #9013 / #9014 / #9045 (referenced for evidence only; this PR does not close them).
中文说明
本 PR 做了什么
修复
/review管线的四个缺陷。这些缺陷是用qwen review run对三个真实 open PR（#9013、#9014、#9045）配合 qwen3.8-max 做端到端实跑时抓到的，每个修复都配了回归测试。四个问题全部是现场实测观察到的，不是推测。review run转发了并发运行的另一个 review 的 verdict（run.ts）。 composed verdict 扫描用qwen-review-*composed.json在共享的.qwen/tmp里取最新文件，同仓库两个 review 并发时，谁先 compose 就被对方捕获。现在扫描（轮询和退出后兜底）按本次运行的 target 钉死——PR 运行只认自己的qwen-review-pr-<n>-…composed.json和…-pr-<n>.md，local/file 运行拒绝 PR 前缀的工件——且轮询在子进程运行期间持续读取最新版本，因为实测有一次 review 在首次写入 12 分钟后合法地二次 compose，只取首个快照会转发已被取代的 verdict。占位符 budget gap 进入了公开正文（
lib/budget.ts）。PLACEHOLDER_GAP_RE负责丢弃"没有可披露内容"的非披露，但其 completion 习语要求 completion 词收尾，于是none — all checks … completed within budget.漏进了公开 verdict 正文。completion 分支现在容忍一个端锚定的 budget 状语（within/under/inside (the) (tool[-call]) budget）；后面还有从句的文本仍然保留为真披露。缺口披露的 agent 标签渲染成 PR 引文而非 agent 名（
lib/coverage.ts）。label()只取 launch prompt 首行，而 launcher 会前置一句上下文——一次运行的 12 个 finder 共享同一句 PR 概述首行，所有 budget gap 披露都渲染成同一句截断的 PR 引文。现在在 prompt 全文中匹配 brief 的代号行（You are review agent `6c`）并优先使用；首行仍作兜底。dist/cli.js不可执行，静默击穿 QWEN_CODE_CLI（scripts/copy_bundle_assets.js）。shellContextEnv会把 POSIX shell 无法直接执行的QWEN_CODE_CLI（无 shebang 或无执行位）置空，以免"${QWEN_CODE_CLI:-qwen}"死于 exit 126——但 bundle 步骤产出的dist/cli.js是 644 且无 shebang，于是从 bundle 启动的会话里每个 shell 拿到的该变量都是空的，所有qwen review …子命令都回落到 PATH 上的全局安装。现在 bundle 产出带#!/usr/bin/env node且 mode 755 的dist/cli.js。为什么需要
一轮三个并发
qwen review run实跑在四条路径上都产出了具体的错误输出：三跑有二把邻居 PR 的 verdict 当成自己的对外报告（其中一个对外报 REQUEST_CHANGES，而它自己保存的报告结论是 Comment）；三条 "none — …" 占位行被渲染进两个公开 verdict 正文当作覆盖缺口；一次运行的所有缺口披露都挂着同一句不可读的截断 PR 引文当 agent 标签；三次运行的全部子命令都静默执行了机器上自动更新的全局安装而非启动它们的源码树，这使 skill 的整套防版本漂移体系对源码树启动完全失效。每一条都会误导review run --json的 CI 消费者、读公开正文的 PR 作者、或测量构建的维护者。Reviewer 测试计划
如何验证
跑钉死的回归套件——四个修复各有专门用例（并发工件隔离、二次 compose 取最新、实跑占位符原句的保留与丢弃、代号标签、模式钉死）：
cd packages/cli && npx vitest run src/commands/review/run.test.ts src/commands/review/lib/budget.test.ts src/commands/review/check-coverage.test.ts→ 168 通过。bundle 修复：npm run build && npm run bundle后head -c 2 dist/cli.js输出#!，ls -l dist/cli.js显示 755，./dist/cli.js --version可直接执行。可选：在 base 提交上复现并发 bug——同仓库启动两个不同 PR 的review run，观察后启动的运行其composedPath指向先 compose 的那个 PR；本分支上每个运行只认自己的qwen-review-pr-<n>-…工件。证据（Before & After）
Before（实跑，base 提交）：
review run 9014 --json返回"event": "REQUEST_CHANGES", "composedPath": ".../qwen-review-pr-9013-composed.json"，而它自己保存的报告结论是Verdict: Comment；#9013 的公开正文出现Not explored to full depth (tool budget reached): …: none — all checks above completed within budget.；进程表显示从新 bundle 的源码树启动的 review 内部fnm …/bin/qwen review build-test解析到~/.qwen/updates/npm/…/0.21.11（全局安装），彼时dist/cli.js是-rw-r--r--且无 shebang。After：新增单测钉死上述每种形态（composedPatternFor('9014')拒绝qwen-review-pr-9013-composed.json；budgetGapDisclosures('Budget gap: none — all checks above completed within budget.')返回[]而同文本带后续从句时保留；带前置上下文行的 prompt 标签为agent 6c），bundle 产出#!/usr/bin/env node+ 755，经直接执行和shellContextEnv的可用性判定原样验证。已测试系统
环境（可选）
macOS（darwin 25.6）、Node v24.18.1、npm 11.16.0；实跑证据在 tmux 下用
node dist/cli.js review run <pr> --json对 dashscope qwen3.8-max 采集。风险与范围
qwen-review-pr-…的路径将不再被识别——现有代码没有会产生这种形态的路径。占位符放宽是端锚定且带否定守卫的，句中提及 budget 的真披露仍会保留；实跑的保留形态已入测试。unreviewed-dimension封顶、小 PR 高档位延迟、headless 无进度、报告文件名由模型即兴命名）需要设计决策，本 PR 有意不修。dist/cli.js增加 shebang 与执行位是增量性的；npm 发布的入口不受影响。关联 Issue
无——缺陷由对 PR #9013 / #9014 / #9045 的直接实跑观察发现（仅作证据引用；本 PR 不关闭它们）。