fix(review): clear the deferred-suggestion backlog from #9175's review rounds - #9342
Conversation
…w rounds
Nineteen deferred findings from the fifteen review rounds, none Critical,
each taken now with its pin. The behavior changes:
- persistRecoveredLedger takes named flags ({noOwnReview, identityKnown})
— two adjacent positional booleans encoded a safety invariant a swapped
call silently inverted (R14-3); the docblock says four outcomes and the
call-site summary names the anonymous one (R14-1); the swap-pin comment
describes the real failure mode, overwrite not deletion (R14-2).
- test-delta imports TEST_COMMAND_RE from beside the emitter instead of
keeping a byte-identical copy — the two shell-re-execution gates cannot
drift apart silently (R13-3).
- The deferred-suggestions clause clamps its round exactly as the marker
stamp does, so the two cannot disagree at LEDGER_MAX_ROUND (R13-6).
- isNonDiffDimensionGap derives its exempt heads from BRIEFS
(readsDiff: false roles by publicLabel) instead of hardcoding prose
variants, with a cross-pin test over every brief (R13-5).
- The stop-relay splice matches the full canonical entry text (verbatim
or prefix-reshaped) instead of the bare phrase — a free-form
line-coverage disclosure that merely mentions the budget now reaches
the posted body (R12-6).
- recoverLedger reports merged: true when the union fires; the rendered
section then names the mixed provenance instead of calling the own
subset another account's claims, and the PARTIAL note stops pinning a
two-marker dropped sum on one round's size cap (R12-12, R12-14).
- The ZH budget-gap classifier strips symmetric full-width parens — the
IME-default wrap survived as a phantom gap in exactly the output
language the branch exists for (R2-17) — and the fold key strips
full-width trailing punctuation, so a gap restated with and without a
full stop discloses once (R6-26).
- A resumed run's note and caveat name an unattempted retry once — the
provisional clause excludes commands the still-to-run clause already
names, and no longer claims a never-started retry was killed on a
shortened deadline (R10-17); the caveat emits BOTH segments when
unattempted work and re-clamped suites coexist (R9-50).
- --resume tells a COMPLETED zero-suite run apart from one that ended
before its test phase, keyed on ok and a new structural buildOnly
stamp (both gate-validated), instead of misadvising a full re-run.
The test-strength half: symbolic FOREIGN_ROUND_HEADROOM boundary pins,
own-base and zero-base (R13-7); a deep-squat id in the squat fixture so
the general idRound > markerRound predicate is pinned (R13-8); an
at-cap union case proving the re-cap trims foreign entries first
(R13-9); the null-login headroom bound the doc promises (R13-10); a
missing-test-key fixture for the resume gate's commandsOk clause
(R3-48); fence- and site-scoped agent-prompt assertions for the timeout
directive, the resume wrapper, --worktree and the continuation
paragraph (R11-13); handler-level pins for identity-throw isolation and
both deletion-licence conjuncts (R11-17); and a marker-carrying handler
test wiring foreign recovery through to the rendered context and the
sha-less side file (R1-40).
Key mutants verified killed: reversed union concatenation, off-by-one
headroom boundary, me-guarded outage bound, disabled anonymous guard.
Still deferred, unchanged: R4-4 (failing-file parser redesign — a
design task, not a cleanup) and R11-9.
|
Re-run at the current head — the delta since the last reviewed commit is two clean
Moving on to code review. 🔍 中文说明在当前 head 上重跑——距上次评审提交，增量只是两次干净的
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal before the diff is unchanged from prior passes — named flags for the adjacent-boolean pair, BRIEFS-derived exemption, one shared re-execution grammar, structural stamps for the resume split, a deferred-round clamp identical to the marker's — so this re-run concentrated on what is new at this head: two
Non-blocking, on the record from CI test evidenceUnattended run — triage never builds or executes PR code; the evidence below is the PR's own CI read through the API at review time. Every check on the reviewed commit has completed and nothing failed. The ubuntu unit suite ( On substantiation: the earlier sandboxed
中文说明代码审查读 diff 前的独立方案与之前各轮一致——相邻布尔改具名参数、豁免表从 BRIEFS 派生、共享一条再执行语法、resume 分裂用结构化戳、deferred 轮次与盖章同钳——因此本次集中在当前 head 的新表面：两次 合并完整保留了批次：两个合并提交均为 bot 的 不阻断、记录在案（ CI 测试证据无人值守——triage 从不构建或执行 PR 代码；以下为评审时刻经 API 读取的 PR 自身 CI。被审提交上所有检查已完成、无失败。ubuntu 单测套件（覆盖全部变更文件与新钉）在本 head 为绿；macOS/Windows 单测矩阵与 CLI 集成任务被 CI 路由跳过，与本 PR 此前每个提交一致——路由决定，非红色信号。变更面是 review 命令内部的报告合成与 resume 记账，单测套件直接覆盖。实质性佐证：此前 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean re-run at the merged head; the delta since the last reviewed commit is two clean update-branch merges with the batch's hunks verified intact, and the substantive batch was already proven load-bearing by the A/B verify run. Stepping back: nothing changed my read of the batch itself — it is still what good follow-through on a fifteen-round review looks like, and my independent proposal matched the PR's approach at every point. What this re-run had to earn was the merges: main moved a lot while this PR sat in review (10 commits in, including #9092 reshaping the same subsystem), and I did not want to attest to a merge I hadn't looked at. Both are bot The 4/5 rather than 5/5 is the standing nits, none of them gates: the two formal template sections stay absent (maintainer's own follow-up, noted in Stage 1); the four round-8 probe deferrals stay on the record (write-side negative unwitnessed, reshaped-relay anchor decision unpinned, deletion-licence conjunct without a negative witness, merged-provenance wording promising a split the rendering can't show); and the PR body's test count is stale (3266 vs the suite actually run). One operational note for whoever merges: main has moved three commits past the last update-branch — #9092 among them, touching Approving now, pinned to the exact commit reviewed. ✅ 中文说明置信度：4/5 —— 合并后 head 上的干净复审；距上次评审提交的增量只是两次干净的 update-branch 合并，已核实批次 hunk 完好，实质批次此前已由 A/B verify 证明承重。 整体看：对批次本身的判断没有变化——这依然是十五轮评审之后教科书式的收尾，我独立给出的方案与 PR 处处一致。本次复审需要挣得的是合并：PR 在评审期间 main 前进了很多（合入 10 个提交，包括重塑同一子系统的 #9092），我不愿为没看过的合并背书。两次都是 bot 的 update-branch 合并——该机制遇冲突即失败而非解决，因此没有人工冲突解决可审；批次的 hunk 在合并后的树中原样就位（唯一被 main 同时修改的文件 给 4/5 而非 5/5 是那些不阻断的小瑕疵：两个形式模板章节依旧缺失（维护者自己的后续 PR，Stage 1 已说明）；四条第 8 轮 probe 级延后记录在案（写侧负例无见证、重排 relay 的锚判定未钉、删除许可合取无负见证、合并来源措辞承诺了渲染无法呈现的逐条划分）；PR 正文测试数过期（3266 对实测套件）。一条给合并者的运维提示：main 在上次 update-branch 之后又前进了三个提交——其中包括 #9092，它改动的 现在批准，钉住在实际评审的提交上。✅ — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const endedEarly = | ||
| neverTested && (previous.buildOnly === true || previous.ok === false); |
There was a problem hiding this comment.
[Critical] R1-1: The new nothing-to-resume split keys on buildOnly/ok, but two real producer shapes carry neither signal, so both are misclassified as "completed with no suite to run" — the false-completion class this split was added to kill.
Shape 1 (the harmful one): a single-root repo whose whole-call budget is spent below BUDGET_MIN_ATTEMPT_MS before the test loop. The loop exits at the floor pushing the dir into prose-only notRun; nothing sets ok: false; single-root never records a testScope; no stamp is written. The report's own carried note says "1 suite(s) still to run" while --resume answers "completed with no suite to run", certifying an existing, unrun suite as completed and dropping the "Re-run build-test without --resume" advice — the only path that can ever run the suite. The pre-diff two-way split answered "ended before its test phase" here, and agent-briefs.ts:538 / agent-prompt.ts:1431-1438 still document that answer ("report the dimension UNFINISHED") — this PR changes the behavior without updating either site.
Shape 2 (bounded): the zero-affected early return at npm-toolchain.ts:612-636 builds the same never-tested report without the stamp even for a deliberate --build-only probe, so a resumed probe loses the probe's answer — extensionally harmless (zero suites are scoped) but it contradicts the docblock at npm-toolchain.ts:209-227 that assigns deliberate probes to the endedEarly arm.
Failure scenario: single-package repo with a test script and non-empty diff; install+build leave <15s of budget (default 570s, or a smaller explicit --budget) → fresh run reports budget spent with 1 suite still to run, ok: true → --resume answers "completed with no suite to run" → Agent 7 trusts the resume verdict, reports build-and-test finished with zero suites run → the one suite the diff may have broken ships unmeasured.
Witness (probe through the real npmToolchainAdapter.run, budget 20):
FRESH ok=true | testScope=undefined | buildOnly=undefined
note: "…the whole-call budget (20s) was spent with 1 suite(s) still to run — not run: ."
RESUME: "…Nothing to resume: the run being continued completed with no suite to run."
endedEarly-message=false | rerunAdvice=false
CONTROLS: buildOnly:true → early-end message | ok:false → early-end message
FLIP: structural stamp applied → endedEarly-message=true | rerunAdvice=true (104/104 tests green)
Suggested fix: stamp both shapes structurally on the fresh path — (1) when the test loop breaks on the budget floor with zero suites run, record a dedicated field (e.g. endedBeforeTests: true) shape-validated in previousReport like buildOnly, and extend endedEarly to read it; (2) add ...(args.buildOnly ? { buildOnly: true } : {}) to the zero-affected early-return object (it already branches on args.buildOnly for its note). Add fixtures for both shapes to the resume-note test and update the agent-briefs.ts/agent-prompt.ts continuation prose to match.
中文说明
新的「无任务可恢复」三分支以 buildOnly/ok 为判据，但有两种真实的生产者形态两个信号都不携带，因此都被误判为「completed with no suite to run」——正是这个分支要消灭的「虚假完成」类别。
形态 1（有害的一种）： 单根仓库的整体调用预算在测试循环开始前就跌破 BUDGET_MIN_ATTEMPT_MS。循环在地板处退出，把目录推进仅存在于文案的 notRun；没有任何代码把 ok 置为 false；单根仓库从不记录 testScope；也不写任何戳。报告自带的 note 说「1 suite(s) still to run」，而 --resume 却回答「completed with no suite to run」——把一个真实存在、从未运行的套件认证为已完成，并且丢掉了「Re-run build-test without --resume」这条唯一能让该套件跑起来的建议。改动前的二分支在此形态下回答「ended before its test phase」，且 agent-briefs.ts:538 / agent-prompt.ts:1431-1438 至今仍按该答案书写（「report the dimension UNFINISHED」）——本 PR 改变了行为却没有同步这两处。
形态 2（影响有限）： npm-toolchain.ts:612-636 的零影响提前返回构造同样的「从未测试」报告时也不打戳，即便是刻意的 --build-only 探针——恢复这样的探针报告将失去探针专属答案。外延上无害（作用域里本来就没有套件），但与 npm-toolchain.ts:209-227 把刻意探针划给 endedEarly 分支的 docblock 相矛盾。
失败场景：带测试脚本的单包仓库 + 非空 diff；install+build 后预算只剩不足 15 秒（默认 570 秒，或显式传更小的 --budget）→ 新跑报告「预算耗尽、1 个套件未运行」且 ok: true → --resume 回答「completed with no suite to run」→ Agent 7 信任恢复结论，上报 build-and-test 已完成而实际零套件运行 → diff 可能破坏的那个套件未经测量就放行。
证据（通过真实 npmToolchainAdapter.run 探针执行，budget 20）：新跑 ok=true | testScope=undefined | buildOnly=undefined，note 为「预算耗尽、1 个套件未运行」；恢复回答「completed with no suite to run」，endedEarly-message=false | rerunAdvice=false；对照形态（buildOnly:true 与 ok:false）均正确返回 early-end 文案；打上结构化戳后翻转为 endedEarly-message=true | rerunAdvice=true（104/104 测试仍绿）。
修复建议：在新跑路径上对两种形态都做结构化打标——(1) 测试循环在预算地板处以零套件退出时记录专用字段（如 endedBeforeTests: true），像 buildOnly 一样在 previousReport 中做形状校验，并扩展 endedEarly 读取它；(2) 在零影响提前返回对象中补上 ...(args.buildOnly ? { buildOnly: true } : {})（该处 note 已按 args.buildOnly 分支）。为两种形态各补夹具，并同步更新 agent-briefs.ts/agent-prompt.ts 的续跑文案。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // tests and no scope needs to tell a deliberate probe apart from a | ||
| // completed zero-suite run, and the note is prose an agent must never | ||
| // have to parse. | ||
| ...(args.buildOnly ? { buildOnly: true } : {}), |
There was a problem hiding this comment.
[Suggestion] R1-3 (location 2 of 2): the WRITE half of the buildOnly stamp is untested end to end — no fresh --build-only run asserts report.buildOnly === true, and the resume split reads the stamp only from handcrafted fixtures. Deleting this stamp spread leaves the whole suite green (the zero-suite test at build-test.test.ts:3207 writes buildOnly: true into its report by hand); a real --build-only probe's report would then carry no stamp and a later --resume on it would take the neverTested && !endedEarly arm — reporting "completed with no suite to run" instead of "ended before its test phase … Re-run build-test without --resume", the probe/completed misclassification this diff's split was created to prevent.
Concrete cost: deleting this line is a silent, test-invisible regression of exactly the behavior the sibling declaration comment (see the other location's comment) describes. Witness: Agent 7 efficacy probe (harnessValidated: true) — the declaration hunk revert survived all tests; the handcrafted-fixture absorption means this spread has no asserting test.
Suggested fix: in one of the fresh-probe tests assert the stamp and its absence (expect(probe.buildOnly).toBe(true); expect(withTests.buildOnly).toBeUndefined();), or add an end-to-end probe-then-resume test.
中文说明
R1-3（共 2 处，第 2 处）：buildOnly 戳的「写入端」完全没有测试——没有任何新建 --build-only 运行断言 report.buildOnly === true，而恢复分支只从手工夹具读取该戳。删掉这行展开，整个测试套件仍全绿（build-test.test.ts:3207 的零套件测试是手工把 buildOnly: true 写进报告的）；于是真实 --build-only 探针报告将不再带戳，后续 --resume 会走 neverTested && !endedEarly 分支，回答「completed with no suite to run」而不是「ended before its test phase … Re-run build-test without --resume」——正是本次 diff 新增三分支要消灭的探针/完成误判。
具体代价：删除这一行是对声明处注释所描述行为的静默回归，且测试不可见。证据：Agent 7 有效性探针（harnessValidated: true）——回退声明 hunk 后所有测试仍绿；手工夹具吸收了本应存在的断言。
修复建议：在某个新建探针测试中断言戳的存在与缺失，或补一个「探针→恢复」端到端测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // refusals green (each still failed on build/timedOut) while a | ||
| // report truncated of its `test` key cleared the gate and died at | ||
| // `previous.test.filter` — the raw crash the gate exists to replace. | ||
| { toolchain: 'npm', build: [], timedOut: [] }, |
There was a problem hiding this comment.
[Suggestion] R1-2: This new "witness" fixture for the !commandsOk(shape.test) gate clause is masked — it also omits affected and ok, each an independent refusing clause (!affectedOk, and the !okOk this same PR adds). Deleting !commandsOk(shape.test) from the gate keeps every refusal in this test green, so the fixture cannot detect the exact mutation its comment claims to pin, while a genuinely otherwise-complete report truncated of only its test key would clear the mutated gate and raw-crash at shape.test.find — the same crash class the comment names.
Witness (mutation probe): mutated gate + original fixtures → test STILL green (mutation survives); mutated gate + fixture fixed per the suggestion below → RED: expected [Function] to throw error matching /is not one/ but got "Cannot read properties of undefined (reading 'find')".
| { toolchain: 'npm', build: [], timedOut: [] }, | |
| { toolchain: 'npm', affected: ['packages/core'], ok: true, build: [], timedOut: [] }, |
With affected and ok present, the refusal rides on the test clause alone, and removing that clause lets the fixture past the shape gate to a crash whose message does not match /is not one/ — turning the test red as intended.
中文说明
R1-2：这个为 !commandsOk(shape.test) 门控条款新增的「见证」夹具被遮蔽了——它同时缺少 affected 和 ok，而这两者各自是独立的拒绝条款（!affectedOk，以及本 PR 自己新增的 !okOk）。从门控中删除 !commandsOk(shape.test) 后，本测试中所有拒绝仍为绿——夹具无法捕获其注释声称要钉住的那个变异；而一份只缺 test 键、其余完整的真实报告却能通过被变异后的门控，在 shape.test.find 处裸崩溃——正是注释点名的崩溃类别。
证据（变异探针）：变异后的门控 + 原夹具 → 测试仍绿（变异存活）；变异后的门控 + 按下方建议修复的夹具 → 红：expected [Function] to throw error matching /is not one/ but got "Cannot read properties of undefined (reading 'find')"。
补上 affected 与 ok 后，拒绝只由 test 条款承担；删除该条款时夹具将穿过形状门控、以不匹配 /is not one/ 的消息崩溃，测试如愿变红。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| * report has no tests and no scope BY CHOICE, and without the stamp that | ||
| * shape is indistinguishable from a completed zero-suite run. | ||
| */ | ||
| buildOnly?: boolean; |
There was a problem hiding this comment.
[Suggestion] R1-3 (location 1 of 2): Nothing in the suite gates this buildOnly report-contract member. The efficacy probe reverted this declaration hunk on its own and every affected test stayed green (harnessValidated: true) — vitest is transpile-only, so the type-level declaration is caught only by npm run typecheck, never by the tests. The --resume nothing-to-resume split keys on this member (lib/npm-toolchain.ts:226-228) and the producer stamps it (lib/npm-toolchain.ts:681); if a follow-up change renames or drops the stamp or the member while leaving the other side, no test fails and the message can silently regress into calling a deliberate --build-only probe a "completed with no suite to run" report — the exact misclassification the field's own comment says it exists to prevent.
Suggested fix: extend the nothing-to-resume split tests (the buildOnly: true previous-report cases around lines 3213-3248) with an assertion that the probe-vs-completed classification depends on the stamped field written by a REAL fresh --build-only run (see the sibling comment at lib/npm-toolchain.ts:681), so its removal turns a test red.
中文说明
R1-3（共 2 处，第 1 处）：测试套件中没有任何东西钉住这个 buildOnly 报告契约成员。有效性探针单独回退此声明 hunk 后所有受影响测试仍绿（harnessValidated: true）——vitest 只做转译，类型层声明只能被 npm run typecheck 捕获，测试永远抓不到。--resume 的「无任务可恢复」分支以该成员为判据（lib/npm-toolchain.ts:226-228），生产者负责打戳（lib/npm-toolchain.ts:681）；若后续改动重命名或删除戳/成员中的一侧而留下另一侧，没有任何测试会失败，消息可能静默回归为把刻意的 --build-only 探针说成「completed with no suite to run」——正是该字段注释声称要防止的误判。
修复建议：在「无任务可恢复」分支测试（约 3213-3248 行 buildOnly: true 的用例）中补充断言，使探针/完成的分类依赖一次真实新建 --build-only 运行写下的戳字段（见 lib/npm-toolchain.ts:681 处的姊妹评论），从而删除该字段时测试变红。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| !buildOnlyOk || | ||
| !okOk || |
There was a problem hiding this comment.
[Suggestion] R1-4: These two new validation clauses have no test. The efficacy probe reverted just these clauses in previousReport and every affected test stayed green (harnessValidated: true); no test exercises rejection of a previous report with a non-boolean buildOnly or a missing/non-boolean ok.
Failure scenario: with the guard removed, a corrupted or hand-edited previous report ("buildOnly": "yes" or "ok": "false") is accepted instead of refused; the resume split then reads previous.buildOnly === true || previous.ok === false, so a probe report with a string stamp fails === true and is classified as a completed zero-suite run — printing the wrong message. The guard's own comment states it exists so a corrupted stamp "refuses here instead of steering the message off a non-boolean truthiness", yet no test fails when it is deleted.
Suggested fix: add cases to the previousReport validation tests asserting it throws for { buildOnly: "yes" } and for a report missing ok (or with ok: "false").
中文说明
R1-4：这两条新增校验条款没有任何测试。有效性探针仅回退 previousReport 中这两条条款，所有受影响测试仍全绿（harnessValidated: true）；没有测试覆盖「previous 报告的 buildOnly 非布尔、或 ok 缺失/非布尔时被拒绝」。
失败场景：守卫被移除后，损坏或手工篡改的 previous 报告（"buildOnly": "yes" 或 "ok": "false"）会被接受而非拒绝；恢复分支随后读取 previous.buildOnly === true || previous.ok === false，字符串戳的探针报告在 === true 上失败，被归类为「跑完但无套件」，打印错误消息。守卫自己的注释写明其存在意义是让损坏的戳「在此被拒绝，而不是凭非布尔真值去操纵消息」，然而删掉它并没有测试变红。
修复建议：在 previousReport 校验测试中补充用例，断言对 { buildOnly: "yes" } 以及缺失 ok（或 ok: "false"）的报告抛错。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| '', | ||
| `**This list is PARTIAL**: ${ledger.dropped} further finding(s) from round ${ledger.round} did not fit the marker's size cap and are not here. Absence below is not evidence a finding was fixed — say so rather than reporting the missing ones as retired.`, | ||
| merged | ||
| ? `**This list is PARTIAL**: ${ledger.dropped} further finding(s) from the merged rounds did not fit the marker size caps (losses span both source markers and the merge's own re-cap, not round ${ledger.round}'s marker alone) and are not here. Absence below is not evidence a finding was fixed — say so rather than reporting the missing ones as retired.` |
There was a problem hiding this comment.
[Suggestion] R1-14: The merged PARTIAL note asserts a loss attribution that is false on shapes the union actually produces: "did not fit the marker size caps" and "losses span both source markers and the merge's own re-cap" — but dropped is a three-term sum (pr-context.ts:894-897) any subset of which may be zero, and the in-memory merge re-cap (merged.slice(0, LEDGER_MAX_FINDINGS)) is not a marker cap.
The PR's own test proves the shape reachable (pr-context.test.ts:1358, 'the merge cap trims FOREIGN entries first'): own marker 50 complete entries (dropped 0) + foreign 5 disjoint (dropped 0) → union re-caps at 50 → dropped = 0+0+5, loss ONLY from the merge re-cap — yet the note claims both source markers lost entries to marker size caps. Witness (probe through the real wiring, recoverLedger → renderLedgerSection): merged: true, dropped: 5, rendered note observed verbatim: "This list is PARTIAL: 5 further finding(s) … did not fit the marker size caps (losses span both source markers and the merge's own re-cap, not round 8's marker alone)…" — with both source markers complete.
Failure scenario: the renderLedgerSection docblock itself names the dismissal path this invites — a Step 6 reader cross-referencing both markers' bodies finds both complete while the note insists both lost entries, dismisses the warning as stale, with 5 findings genuinely absent from the work list. The actionable core ("Absence below is not evidence a finding was fixed") stays true in every shape, so this is a false attribution that invites dismissal, not a flipped ruling.
Suggested fix: make the attribution honest about the sum — e.g. "(the loss may come from either source marker's cap, the merge's re-cap, or both — do not assume any single round's marker lost entries)" — and update the renderer assertion at pr-context.test.ts:1705, which currently pins the false wording (necessary, not optional).
中文说明
R1-14：合并版 PARTIAL 注释的损失归属在 union 真实会产生的形态上是假的：「did not fit the marker size caps」与「losses span both source markers and the merge's own re-cap」——但 dropped 是三项之和（pr-context.ts:894-897），任一项都可能为零，而内存中的合并再截断（merged.slice(0, LEDGER_MAX_FINDINGS)）并不是 marker 容量上限。
PR 自己的测试就证明该形态可达（pr-context.test.ts:1358「merge cap 先裁外部条目」）：自己的 marker 50 条完整（dropped 0）+ 外部 5 条不相交（dropped 0）→ union 在 50 处再截断 → dropped = 0+0+5，损失全部来自合并再截断——注释却声称两个来源 marker 都因容量上限丢了条目。证据（通过真实链路探针执行 recoverLedger → renderLedgerSection）：merged: true, dropped: 5，逐字观察到渲染出的注释声称两个 marker 都丢了条目，而两个来源 marker 都是完整的。
失败场景：renderLedgerSection 的 docblock 自己点名了由此引发的Dismiss路径——Step 6 读者交叉核对两个 marker 的正文，发现两者都完整，而注释坚持两者都丢了条目，于是把警告当作过期信息Dismiss掉，而工作列表确实缺了 5 条发现。注释的可操作核心（「列表缺失不代表发现已修复」）在所有形态下仍为真，因此这是一个诱发Dismiss的虚假归属，而不是会直接翻转结论的错误。
修复建议：让归属对三项之和保持诚实——例如「（损失可能来自任一来源 marker 的容量上限、合并再截断，或兼而有之——不要假定某一轮的 marker 丢了条目）」——并同步更新 pr-context.test.ts:1705 的渲染断言，它目前钉住的正是这句错误文案（此测试改动是必需的，不是可选的）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const prevLedgerAuthor = prevRecovered?.foreign | ||
| ? (prevRecovered.author ?? null) | ||
| : null; | ||
| const prevLedgerMerged = prevRecovered?.merged ?? false; |
There was a problem hiding this comment.
[Suggestion] R1-5: The new merged provenance flag's handler passthrough is ungated end to end — no handler-level test ever constructs the merged shape (an own marker plus a higher-round foreign winner). Mutate the passthrough — hardcode const prevLedgerMerged = false; — and all 98 pr-context tests stay green: the recoverLedger seam test asserts merged only on the recovery return value (~line 1320), the renderer test passes true directly (~line 1683), and the four new handler fixtures are marker-less-stranger, foreign-only, own-only, and marker-less-longReview. (Note: the alternate "drop the 9th positional arg" variant IS caught by typecheck — TS2345 at line 1547 — so the surviving mutation is the hardcode/false-value one, but it survives everything.)
Witness (mutation probe): baseline vitest run pr-context.test.ts → 98/98 green; mutation → 98/98 STILL green; a temporary handler fixture (own round-7 marker + ci-bot round-8 winner) asserting "MERGED over this account's own latest findings" FAILS under the mutation and passes on restored code.
Failure scenario: if the passthrough regresses, a real cross-account recovery where a foreign round wins over this account's own marker renders the pure-foreign "THEIR claims" wording over the mixed list — the own certified subset attributed to the foreign poster, the exact provenance inversion this diff's merged branch was added to fix.
Suggested fix: add a handler-level fixture with an own marker plus a newer foreign winner asserting the written context contains "MERGED over this account's own latest findings" and does not contain 'THEIR claims'.
中文说明
R1-5：新的 merged 来源标记在 handler 层的透传完全没有门控——没有任何 handler 级测试构造过合并形态（自己的 marker + 更高轮次的外部胜出者）。对透传做变异——硬编码 const prevLedgerMerged = false;——全部 98 个 pr-context 测试仍绿：recoverLedger 接缝测试只对恢复返回值断言 merged（约 1320 行），渲染器测试直接传 true（约 1683 行），四个新 handler 夹具分别是无标记陌生人、纯外部、纯自己、无标记 longReview。（注：另一种「删掉第 9 个位置参数」的变体会被 typecheck 抓住——1547 行 TS2345——因此存活的变异是硬编码/置假值这一种，但它对现有测试完全免疫。）
证据（变异探针）：基线 98/98 绿；变异后仍 98/98 绿；临时 handler 夹具（自己的 round-7 marker + ci-bot 的 round-8 胜出者）断言 "MERGED over this account's own latest findings" 时在变异下失败、在还原后的代码上通过。
失败场景：透传一旦回归，真实场景中外部轮次压过本账号 marker 的跨账号恢复会把纯外部的 "THEIR claims" 文案渲染在混合列表上——本账号认证的子集被归给外部发布者，正是本 diff 的 merged 分支要修复的来源反转。
修复建议：补一个 handler 级夹具：自己的 marker + 更新的外部胜出者，断言写出的 context 包含 "MERGED over this account's own latest findings" 且不含 'THEIR claims'。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // other → removed (a stale counter would stamp rounds nobody posted); | ||
| // recovery threw → round counter kept, age-sensitive `commitId`/`reviewId` | ||
| // stripped. | ||
| // recovered anonymously over an existing file → only the round counter and |
There was a problem hiding this comment.
[Suggestion] R1-8: This call-site comment block now contradicts itself and its own helper. The diff adds a fourth outcome to the enumeration below (this line), but the opening line at 1497 still says "The side file's three outcomes live in the helper" — while the persistRecoveredLedger docblock the same PR updated now reads "Four outcomes, each honest about what this run learned:" (pr-context.ts:940, - Three outcomes → + Four outcomes in this diff). The author was maintaining the count as an invariant and missed one of the two sites. The anonymous-recovery clause also omits that sha/commitId are dropped in that outcome, which the helper docblock spells out and the adjacent threw-outcome clause does mention ("stripped").
Failure scenario: this file's comments are the module's invariant record, and this call site is where the safety flags (noOwnReview, identityKnown) are derived against those outcomes — the same PR converts the positional booleans to named flags precisely because that derivation must be auditable. A maintainer or audit agent reconciling whether every persist outcome is defended by the flags meets a 3-vs-4 contradiction between the only call site and the helper it points at, and must re-derive the outcome set by hand; an auditor trusting "three" can read the newly added fourth entry as vestigial commentary rather than a live branch the identityKnown flag gates.
Suggested fix: change the opening line to // The side file's four outcomes live in the helper: and optionally note the sha/commitId drop in the anonymous-outcome clause.
中文说明
R1-8：这个调用点注释块现在自相矛盾、且与其指向的 helper 矛盾。diff 在下方枚举中新增了第四种结果（即本行），但 1497 行的开头仍写「The side file's three outcomes live in the helper」——而同一 PR 更新过的 persistRecoveredLedger docblock 现在写的是「Four outcomes…」(940 行，diff 中 - Three outcomes → + Four outcomes)。作者一直把这个计数当作不变量维护，却漏了两处中的一处。匿名恢复子句还遗漏了该结果会丢弃 sha/commitId，helper 的 docblock 明确写了这一点，相邻的 threw 子句也提到了（"stripped"）。
失败场景：本文件的注释是模块的不变量记录，而这个调用点正是依据这些结果派生安全标志（noOwnReview、identityKnown）的地方——同一 PR 把相邻布尔改成具名标志，正是因为该派生必须可审计。维护者或审计 agent 核对「每个 persist 结果是否都被标志防御」时，会在唯一调用点与其指向的 helper 之间遇到 3 对 4 的矛盾，只能手工重推结果集；信任 "three" 的审计者可能把新增的第四条当作冗余注释，而非 identityKnown 标志实际门控的活分支。
修复建议：把开头行改为 // The side file's four outcomes live in the helper:，可选地在匿名结果子句中注明 sha/commitId 的丢弃。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(foreign?.ledger.round).toBe(2); | ||
| // Pure-foreign (no own base): nothing was merged, so the renderer's | ||
| // whole-list THEIR-claims sentence is the accurate one. | ||
| expect(foreign?.merged).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R1-11: merged is true even when this account's own latest marker carries ZERO findings — an ordinary LGTM round. ledgerMarkerFor posts markers unconditionally for PR plans (an approve round posts "findings":[]), and recoverLedger's union fires on best.foreign && bestOwn without checking bestOwn.ledger.findings.length > 0; no fixture pins the empty-own edge, so the "nothing was merged ⇒ THEIR-claims sentence is the accurate one" invariant this assertion documents is violated for a second reachable shape.
Witness (probe): own round-5 findings: [] marker + foreign round-6 marker (1 finding, dropped: 2) → merged = true | findings = ["R6-1"] | dropped = 2, and renderLedgerSection renders "…MERGED over this account's own latest findings — entries this account certified are its own claims…" over a list with zero own entries, beside a PARTIAL note blaming both markers for a loss that sits entirely in the foreign one.
Failure scenario: maintainer's round-7 LGTM marker + ci-bot round-8 marker with findings → merged: true over a purely foreign list → provenance wording claims own-certified entries exist when none do (and the PARTIAL note misattributes when dropped > 0) — a provenance misstatement in the module whose docblocks treat provenance wording as load-bearing. The operative "absence is not evidence of a fix" warning survives, so this is a Suggestion.
Suggested fix: gate mergedOverOwn on bestOwn.ledger.findings.length > 0 (with an empty own subset nothing is merged over), and add a fixture here: own marker with findings: [] + foreign winner ⇒ merged false / THEIR wording.
中文说明
R1-11：即使本账号最新 marker 一条发现都没有（普通的 LGTM 轮），merged 也会是 true。ledgerMarkerFor 对 PR 计划无条件追加 marker（批准轮会发布 "findings":[]），而 recoverLedger 的 union 在 best.foreign && bestOwn 时即触发，不检查 bestOwn.ledger.findings.length > 0；没有夹具钉住「自己为空」的边界，因此本断言所记录的「没有东西被合并 ⇒ THEIR-claims 句子才准确」这一不变量，在第二种可达形态下被破坏。
证据（探针）：自己的 round-5 空 findings marker + 外部 round-6 marker（1 条发现、dropped: 2）→ merged = true，列表纯外部，渲染出「…MERGED over this account's own latest findings — entries this account certified are its own claims…」——列表中本账号条目为零，旁边的 PARTIAL 注释还把完全落在外部 marker 上的损失归给双方。
失败场景：维护者的 round-7 LGTM marker + ci-bot 的 round-8 带发现 marker → 在纯外部列表上得到 merged: true → 来源文案声称存在本账号认证的条目而实际没有（dropped > 0 时 PARTIAL 注释还会错误归因）——在一个把来源措辞当作承重结构的模块里，这是一次来源表述错误。可操作的核心警告（「缺失不代表已修复」）仍然存活，因此定为 Suggestion。
修复建议：给 mergedOverOwn 加上 bestOwn.ledger.findings.length > 0 的门控（自己的子集为空时不存在「合并覆盖」），并在此补夹具：findings: [] 的自己 marker + 外部胜出者 ⇒ merged 为 false / THEIR 文案。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(ids).toHaveLength(LEDGER_MAX_FINDINGS); | ||
| expect(ids.some((id) => id.startsWith('R8-'))).toBe(false); | ||
| // …and `dropped` counts them honestly. | ||
| expect(atCap?.ledger.dropped).toBe(5); |
There was a problem hiding this comment.
[Suggestion] R1-13: The union's dropped sum has three terms — foreign winner's dropped, own marker's dropped, and the re-cap (pr-context.ts:894-897) — but this merge-cap fixture's source markers carry no dropped field, so the only recoverLedger-level assertion pins the re-cap term alone; either marker-borne term can be deleted with the whole suite staying green. No other test routes a marker-borne dropped through recoverLedger (renderer tests hand ledgers directly to renderLedgerSection), and serializeLedger really writes dropped when the count/byte caps trim — the union's own comment names the production shape ("a union of up to twice the cap is reachable on a long-lived PR with a CI-bot interleave").
Witness (mutation probe): mutant const dropped = merged.length - capped.length; → entire pr-context.test.ts suite 98/98 GREEN (mutation survives the whole file); the fix below under the mutant → RED AssertionError: expected 5 to be 10; same fix against correct code → green.
Failure scenario: a future refactor silently zeroing the marker-borne terms would under-count or suppress the PARTIAL note on exactly the lossy long-lived-PR unions this PR added — a truncated merged list reads as complete.
| expect(atCap?.ledger.dropped).toBe(5); | |
| expect(atCap?.ledger.dropped).toBe(3 + 2 + 5); |
…together with giving the fixture's markers declared losses — add "dropped":3 to the own marker's JSON and "dropped":2 to the foreign one — so all three sum terms are pinned.
中文说明
R1-13：union 的 dropped 有三项——外部胜出者的 dropped、自己 marker 的 dropped、再截断（pr-context.ts:894-897）——但这个满容量夹具的来源 marker 都没带 dropped 字段，因此 recoverLedger 层面唯一的断言只钉住了再截断项；删掉任一 marker 携带项，整个套件仍全绿。没有其它测试把 marker 携带的 dropped 送进 recoverLedger（渲染器测试是把 ledger 直接交给 renderLedgerSection），而 serializeLedger 在数量/字节上限裁剪时确实会写 dropped——union 自己的注释也点名了生产形态（「在 CI-bot 交错的长命 PR 上，两倍于上限的 union 可达」）。
证据（变异探针）：变异 const dropped = merged.length - capped.length; → 整个 pr-context.test.ts 98/98 全绿（变异在整个文件存活）；下方修复在变异下 → 红 AssertionError: expected 5 to be 10；同一修复在正确代码上 → 绿。
失败场景：未来某次重构若静默把 marker 携带项清零，恰好会在本 PR 新增的有损长命 PR union 上少计甚至抑制 PARTIAL 注释——被截断的合并列表将显得完整。
同时给夹具的两个 marker 声明损失——自己的 marker JSON 加 "dropped":3、外部的加 "dropped":2——使三项加和全部被钉住。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…e is not completion R1-1 from the automatic review, probe-confirmed. The nothing-to-resume split keyed on buildOnly and ok, and two real producer shapes carry neither: a single-root run whose whole-call budget fell below the attempt floor before the first suite (build green, ok stays true, single-root writes no testScope, the dir lands in prose-only notRun), and the zero-affected early return, which dropped the buildOnly stamp even for a deliberate probe. The first is the harmful one: --resume answered 'completed with no suite to run' — certifying an existing, unrun suite as finished and dropping the fresh-run advice that is the only path to ever running it — exactly the false-completion class the split was added to kill. The fresh path now stamps endedBeforeTests: true whenever the test phase was entered and ran nothing (zero results with notRun non-empty); the field is documented on the report, shape-validated in the resume gate like buildOnly, and the endedEarly split reads it as its third signal. The zero-affected return carries the probe stamp its own note already branches on. Both continuation prose sites (the role-7 brief and the prompt paragraph) now describe the stamped shape instead of 'a shape that carries no field at all'. Pins: the fresh single-root budget-floor test asserts the stamp (write half); the nothing-to-resume test gains the floored control asserting the early-end answer with the re-run advice (read half — removing the stamp read fails exactly this); the zero-affected test asserts the probe stamp present on a probe and absent otherwise.
|
Round 1's blocker is fixed in f38007c. R1-1 — taken with both shapes, and the finding is right that I had seen the edge and wrongly accepted it: the split's own development notes treated the single-root budget-floor shape as a tolerable misword, and your harm chain (the resume verdict certifies the one existing suite as finished and drops the only advice that ever runs it) is what that acceptance actually cost. The fresh path now stamps The round's Suggestions are deferred without exception, per the standing policy on this stack; several (R1-3's write-half E2E, R1-13's dropped-sum terms) are partially covered by the new pins where they overlapped the blocker's fix surface. 中文说明第一轮的阻断项已在 f38007c 修复。 R1-1 —— 两种形态一并采纳。这条发现说得对：我在实现时看到过这个边缘并错误地接受了它——开发笔记里把单根预算地板形态当作可容忍的措辞偏差，而你给出的危害链（resume 结论把唯一存在的套件认证为已完成，并丢掉唯一能让它运行的建议）正是那个接受的真实代价。新跑路径现在在「测试阶段已进入但零套件运行」时打上 本轮 Suggestion 按本栈现行策略一律延后；其中数条（R1-3 的写半边 E2E、R1-13 的 dropped 三项）在与阻断项修复面重叠处已被新钉部分覆盖。 |
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.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-4 buildOnly report-contract member ungated (build-test.ts:137) — already reported (comment 3795737144)
- R1-5 previousReport validation clauses untested (build-test.ts:584) — already reported (comment 3795737151)
- R1-6 merged provenance flag handler passthrough ungated (pr-context.ts:1496) — already reported (comment 3795737179)
Not explored to full depth (tool budget reached): chunk 6: none — no check was cut short..
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.test.ts:1049 — [probe] Vacuous marker-withholding assertion: the fixture plan carries no fetchedShapackages/cli/src/commands/review/compose-review.ts:578 — [probe] Documented canonicalDimensionHead variants (build & test, -verification) untestedpackages/cli/src/commands/review/lib/budget.ts:826 — [probe] Of the six added CJK trailing-punct chars, only 。 is exercised by a testpackages/cli/src/commands/review/lib/budget.ts:826 — [probe] ASCII ? missing from TRAILING_GAP_CHAR_RE while fullwidth ？ is addedpackages/cli/src/commands/review/pr-context.test.ts:1319 — [probe] The merged flag's own-winner FALSE direction is asserted nowherepackages/cli/src/commands/review/pr-context.test.ts:2121 — [probe] The deletion-licence test asserts bare toHaveBeenCalled() with no path checkpackages/cli/src/commands/review/pr-context.test.ts:1449 — [probe] Symbolic FOREIGN_ROUND_HEADROOM boundary fixtures do not pin the constant's valuepackages/cli/src/commands/review/pr-context.ts:1519 — [probe] The !recoveryThrew conjunct of the deletion licence is unpinned
中文说明
已审查。 建议见行内评论。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）：chunk 6：none — no check was cut short.。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // refusals green (each still failed on build/timedOut) while a | ||
| // report truncated of its `test` key cleared the gate and died at | ||
| // `previous.test.filter` — the raw crash the gate exists to replace. | ||
| { toolchain: 'npm', build: [], timedOut: [] }, |
There was a problem hiding this comment.
[Suggestion] R1-2: Still stands at this commit (round-2 re-check) — the witness fixture for the !commandsOk(shape.test) gate clause is masked: it also omits affected and ok, each an independent refusing clause (!affectedOk, and the !okOk this same PR adds). Deleting !commandsOk(shape.test) from the gate keeps every refusal in this test green, so the fixture cannot detect the exact mutation its comment claims to pin, while a genuinely otherwise-complete report truncated of only its test key would clear the mutated gate and raw-crash at previous.test.filter — the same crash class the comment names. — Failure scenario: delete the test clause → this fixture is still refused by !affectedOk/!okOk → suite stays green → a real report missing only test dies on a raw TypeError instead of the named refusal.
Witness (round-1 mutation probe, re-verified at this commit — the fixture and both masker clauses are unchanged): mutated gate + original fixture → test STILL green; mutated gate + fixture fixed below → RED (Cannot read properties of undefined (reading 'find')).
| { toolchain: 'npm', build: [], timedOut: [] }, | |
| { toolchain: 'npm', affected: ['packages/core'], ok: true, build: [], timedOut: [] }, |
With affected and ok present, the refusal rides on the test clause alone, and removing that clause lets the fixture past the shape gate to a crash whose message does not match /is not one/ — turning the test red as intended.
中文说明
第二轮复查：仍然存在。为 !commandsOk(shape.test) 门控条款新增的「见证」夹具被遮蔽——它同时缺少 affected 和 ok，而这两者各自是独立的拒绝条款（!affectedOk，以及本 PR 自己新增的 !okOk）。从门控中删除 !commandsOk(shape.test) 后，本测试中所有拒绝仍为绿——夹具无法捕获其注释声称要钉住的那个变异；而一份只缺 test 键、其余完整的真实报告却能通过被变异后的门控，在 previous.test.filter 处裸崩溃——正是注释点名的崩溃类别。
证据（第一轮变异探针，本提交上复核——夹具与两个遮蔽条款均未变）：变异后的门控 + 原夹具 → 测试仍绿；变异后的门控 + 按下方建议修复的夹具 → 红。
补上 affected 与 ok 后，拒绝只由 test 条款承担；删除该条款时夹具将穿过形状门控、以不匹配 /is not one/ 的消息崩溃，测试如愿变红。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // tests and no scope needs to tell a deliberate probe apart from a | ||
| // completed zero-suite run, and the note is prose an agent must never | ||
| // have to parse. | ||
| ...(args.buildOnly ? { buildOnly: true } : {}), |
There was a problem hiding this comment.
[Suggestion] R1-3: Still stands at this commit (round-2 re-check) — the WRITE half of the buildOnly stamp on the main run is untested end to end: no fresh --build-only run over buildable packages asserts report.buildOnly === true for this results-initializer producer, and the resume split reads the stamp only from handcrafted fixtures. The new round-2 pins cover the endedBeforeTests write half and the zero-affected probe stamp, but this producer remains ungated. — Failure scenario: deleting this spread ships green; a real --build-only probe's report then carries no stamp and a later --resume on it takes the neverTested && !endedEarly arm — reporting "completed with no suite to run" instead of "ended before its test phase … Re-run build-test without --resume", the probe/completed misclassification this diff's split was created to prevent.
Witness (round-2 mutation probe): delete this spread → build-test.test.ts 104/104 stay green (round-1 Agent 7 efficacy probe reached the same result, harnessValidated: true).
Suggested fix: in a main-path probe test assert the stamp and its absence (expect(buildOnly.buildOnly).toBe(true) beside expect(rep.buildOnly).toBeUndefined() on the non-probe sibling), or add an end-to-end probe → --out → --resume test.
中文说明
第二轮复查：仍然存在。buildOnly 戳在主跑路径上的「写入端」完全没有测试：没有任何对可构建包的新建 --build-only 运行断言这个 results 初始化器生产者写下的 report.buildOnly === true，而恢复分支只从手工夹具读取该戳。第二轮新增的钉住覆盖了 endedBeforeTests 的写半边与零影响探针戳，但这个生产者仍无门控。
失败场景：删除这行展开后整个套件仍全绿；于是真实 --build-only 探针报告将不再带戳，后续 --resume 会走 neverTested && !endedEarly 分支，回答「completed with no suite to run」而不是「ended before its test phase … Re-run build-test without --resume」——正是本次 diff 新增三分支要消灭的探针/完成误判。
证据（第二轮变异探针）：删除该展开 → build-test.test.ts 104/104 仍全绿（第一轮 Agent 7 有效性探针同结果，harnessValidated: true）。
修复建议：在主路径探针测试中断言戳的存在与缺失，或补一个「探针 → --out → --resume」端到端测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| function canonicalDimensionHead(s: string): string { | ||
| return s | ||
| .toLowerCase() | ||
| .replace(/&/g, 'and') |
There was a problem hiding this comment.
[Suggestion] R1-7: Still stands at this commit (round-2 re-check; file unchanged since round 1) — canonicalDimensionHead drops the separator-less and/& forms (build&test, buildandtest, build andtest) that the replaced regex deliberately accepted via build[-\s]?(?:and|&)[-\s]?test — the new canonicalizer maps &→and with no surrounding separators, producing buildandtest, while the derived set holds only build-and-test. — Failure scenario (consequence chain verified in code): a budget-stopped round whose only unreviewedDimensions entry is build&test — the integration suite never ran → isNonDiffDimensionGap false → dimensionGapsAreDepthOnly false → failClosed → the ledger anchor's fetchedSha withheld → the next round re-reviews the full range instead of the incremental delta — the ~119-min/34M-token loop the JSDoc beside this code measures. Direction is safe (over-withholds; no unreviewed line rides past the anchor) — a cost regression on a rare variant, not an incorrect verdict.
Witness (both matchers executed at round 1, re-verified against the current code): all four separator-less variants old: true | new: false; canonical forms all true/true.
| .replace(/&/g, 'and') | |
| .replace(/&/g, ' and ') |
so a tight ampersand gains separators before the hyphen collapse (build&test → build and test → build-and-test).
中文说明
第二轮复查：仍然存在（文件自第一轮以来未变）。canonicalDimensionHead 丢掉了无分隔符的 and/& 形态（build&test、buildandtest、build andtest），而被替换的正则是通过 build[-\s]?(?:and|&)[-\s]?test 刻意接受它们的——新规范化器把 &→and 时不补分隔符，得到 buildandtest，而派生集合只有 build-and-test。
失败场景（后果链已在代码中核实）：某轮因预算停止，unreviewedDimensions 唯一条目是 build&test — … → isNonDiffDimensionGap 为 false → dimensionGapsAreDepthOnly 为 false → failClosed → 账本锚点的 fetchedSha 被扣住 → 下一轮全量重审而非增量——正是旁边 JSDoc 记录的那个约 119 分钟/3400 万 token 的循环。方向是安全的（只会多扣、不会让未审查的行跟着锚点溜走）——罕见变体上的成本回归，不是错误裁决。
证据（第一轮实际执行两个匹配器，本轮对照当前代码复核）：四个无分隔符变体全部 old: true | new: false；规范形态全部 true/true。
使紧邻的 & 在连字符合并前先获得分隔符（build&test → build and test → build-and-test）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ? (prevRecovered.author ?? null) | ||
| : null; | ||
| const prevLedgerMerged = prevRecovered?.merged ?? false; | ||
| // The side file's three outcomes live in the helper: recovered → written |
There was a problem hiding this comment.
[Suggestion] R1-8: Still stands at this commit (round-2 re-check by direct read) — this call-site comment block contradicts itself and its own helper: the enumeration below carries FOUR outcomes (recovered → written; proven-absence → removed; anonymous → counter-only advance; recovery threw → stripped), but this opening line still says "The side file's three outcomes live in the helper" — while the persistRecoveredLedger docblock the same PR updated reads "Four outcomes, each honest about what this run learned". The anonymous-recovery clause also omits that sha/commitId are dropped in that outcome, which the helper docblock spells out and the adjacent threw-outcome clause does mention ("stripped"). — Failure scenario: this file's comments are the module's invariant record, and this call site is where the safety flags (noOwnReview, identityKnown) are derived against those outcomes — the same PR converts the positional booleans to named flags precisely because that derivation must be auditable. An auditor trusting "three" can read the fourth entry as vestigial commentary rather than a live branch the identityKnown flag gates.
Witness: direct read at this commit — line 1497 says "three outcomes"; the four-outcome enumeration follows immediately; the helper docblock says "Four outcomes".
Suggested fix: change the opening line to // The side file's four outcomes live in the helper: and note the sha/commitId drop in the anonymous-outcome clause.
中文说明
第二轮复查：仍然存在（直接读取核实）。这个调用点注释块自相矛盾、且与其指向的 helper 矛盾：下方枚举有四种结果（recovered → 完整写入；证明缺席 → 删除；匿名 → 仅推进计数器；恢复抛错 → 剥离时效字段），但开头行仍写「The side file's three outcomes live in the helper」——而同一 PR 更新过的 persistRecoveredLedger docblock 现在写的是「Four outcomes…」。匿名恢复子句还遗漏了该结果会丢弃 sha/commitId，helper 的 docblock 明确写了这一点，相邻的 threw 子句也提到了（"stripped"）。
失败场景：本文件的注释是模块的不变量记录，而这个调用点正是依据这些结果派生安全标志（noOwnReview、identityKnown）的地方——同一 PR 把相邻布尔改成具名标志，正是因为该派生必须可审计。信任 "three" 的审计者可能把新增的第四条当作冗余注释，而非 identityKnown 标志实际门控的活分支。
证据：本提交直接读取——1497 行写 "three outcomes"，紧随其后的枚举列出四种结果，helper docblock 写 "Four outcomes"。
修复建议：把开头行改为 // The side file's four outcomes live in the helper:，并在匿名结果子句中注明 sha/commitId 的丢弃。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // stripped only the ASCII set kept `渗透测试未进行。` and `渗透测试未进行` as | ||
| // two gaps — double-spending MAX_GAPS_PER_AGENT slots in exactly the | ||
| // output language the ZH branch exists for. | ||
| const TRAILING_GAP_CHAR_RE = /[.!…,;:\s。，；：！？、]/; |
There was a problem hiding this comment.
[Suggestion] R1-9: Still stands at this commit (round-2 re-check; the class is unchanged since round 1) — the CJK extension of the fold key's trailing-punctuation strip omits the fullwidth full stop ． (U+FF0E) that the module's own placeholder classifier treats as trailing CJK punctuation (ZH_TAIL = '[。．.!！…,，;；:：\\s]*$'). Every other ZH_TAIL character is covered by the class; ． is the single drift — so a Chinese gap restated once with ． and once without still fails to fold: the exact double-spend defect this hunk was written to kill, left open for one of the script's own stop characters. — Failure scenario: under outputLanguage: 中文 an agent writes Budget gap: 渗透测试未进行． (U+FF0E) and restates Budget gap: 渗透测试未进行 in its summary → stripTrailingGapChars keeps ． → two fold keys → double-spending MAX_GAPS_PER_AGENT (8) slots; with 8+ disclosed gaps the duplicate pushes a genuine gap past the cap break, silently dropping a real disclosure — the direction the module header names unsafe.
Witness (round-1 probe through the real parser; the class at line 826 is unchanged at this commit): U+3002 (。): count = 1 vs U+FF0E (．): count = 2; adding ． to the class flips the pair to count = 1.
| const TRAILING_GAP_CHAR_RE = /[.!…,;:\s。，；：！？、]/; | |
| const TRAILING_GAP_CHAR_RE = /[.!…,;:\s。，；：！？、．]/; |
(or derive both the classifier's tail and the fold key from one shared trailing-punctuation constant so the two cannot drift again).
中文说明
第二轮复查：仍然存在（字符类自第一轮以来未变）。fold key 尾标点剥离的 CJK 扩展遗漏了全角句点 ．（U+FF0E），而模块自己的占位分类器把 ． 当作中文尾标点（ZH_TAIL）。ZH_TAIL 的其它字符该类都覆盖了，唯独 ． 漂移——于是同一条中文缺口一次带 ．、一次不带地复述时仍无法折叠：正是本 hunk 要消灭的重复消耗缺陷，对该脚本自己认可的标点之一仍然敞开。
失败场景：outputLanguage: 中文 下，agent 写 Budget gap: 渗透测试未进行．（U+FF0E），又在总结中复述 Budget gap: 渗透测试未进行 → stripTrailingGapChars 保留 ． → 两个 fold key → 重复消耗 MAX_GAPS_PER_AGENT（8）额度；当披露的缺口达到 8 个以上，重复项会把一个真实缺口挤过 cap 的 break，静默丢弃真实披露——正是模块头部点名不安全的方向。
证据（第一轮通过真实解析器探针执行；本提交上字符类未变）：U+3002（。）: count = 1，U+FF0E（．): count = 2；把 ． 加入字符类后翻转为 count = 1。
或让分类器尾部与 fold key 共享同一个尾标点常量，使两者不再可能漂移。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // capping. (The anchor DECISION below stays exact-text: a reshaped | ||
| // relay spliced here still withholds, over-withholding being the safe | ||
| // direction.) | ||
| const entries = [...canonicalStopEntries]; |
There was a problem hiding this comment.
[Suggestion] R1-12: Still stands at this commit (round-2 re-check) — the phrase→full-entry splice switch falsifies four docblocks in lib/deadline.ts (untouched by this PR). The diff removes all four phrase-constant imports from this file and the splice now keys on full canonical entry text, but the docblocks over BUDGET_STOP_PHRASE/BUDGET_STOP_PHRASE_ZH/ROUND_CAP_PHRASE/ROUND_CAP_PHRASE_ZH (deadline.ts:503-508, 510-513, 549-553, 555) still claim the constants are "Exported so compose-review dedups the orchestrator's copy against the marker's". Verified at this commit: the only remaining consumers are deadline.ts's own reason-string interpolation and deadline.test.ts. — Failure scenario: a maintainer reworking the stop machinery trusts the docblocks and rewords the entry assembly while keeping the phrase substring, expecting the splice to keep deduping; the splice requires the full canonical entry text, so a phrase-only relay survives beside the structural stop line — the same gap rendered twice in the posted body, the exact duplication incident those docblocks record the history of.
Witness: round-1 read at the commit (the four docblocks claim the dedup contract; grep confirms no phrase-constant import remains in compose-review.ts), re-verified this round against the unchanged deadline.ts docblocks.
Suggested fix: update the four deadline.ts docblocks to state the constants now only spell the reason clause inside the entries, and that compose-review's splice/depth-exemption keys on the full canonical entry text, not the phrase.
中文说明
第二轮复查：仍然存在。「短语→完整条目」的 splice 切换使 lib/deadline.ts（本 PR 未触碰）中的四处 docblock 失真。diff 从本文件移除了全部四个短语常量导入，splice 现在以完整规范条目文本为键，但这些常量上方的 docblock 仍声称它们「导出是为了让 compose-review 用同一文本把编排器副本与 marker 副本去重」。已在本提交核实：剩余消费者只有 deadline.ts 自身的 reason 串插值和 deadline.test.ts。
失败场景：维护者改造停止机制时信任这些 docblock，在保留短语子串的前提下改写条目组装，以为 splice 仍会去重；而 splice 需要完整规范条目文本，于是仅含短语的转述行会与结构化停止行并存——同一个缺口在发布的正文里渲染两次，正是这些 docblock 记录过的那次重复事故。
证据：第一轮在本提交读取核实（四处 docblock 仍声称去重契约；grep 确认 compose-review.ts 不再有短语常量导入），本轮对照未变的 deadline.ts docblock 复核。
修复建议：更新 deadline.ts 的四处 docblock，说明这些常量现在只负责拼出条目内的 reason 子句，compose-review 的 splice/深度豁免以完整规范条目文本为键，而非短语。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(ids).toHaveLength(LEDGER_MAX_FINDINGS); | ||
| expect(ids.some((id) => id.startsWith('R8-'))).toBe(false); | ||
| // …and `dropped` counts them honestly. | ||
| expect(atCap?.ledger.dropped).toBe(5); |
There was a problem hiding this comment.
[Suggestion] R1-13: Still stands at this commit (round-2 re-check) — the union's dropped sum has three terms — foreign winner's dropped, own marker's dropped, and the re-cap (pr-context.ts:893-896) — but this merge-cap fixture's source markers carry no dropped field, so the only recoverLedger-level assertion pins the re-cap term alone; either marker-borne term can be deleted with the whole suite staying green. No other test routes a marker-borne dropped through recoverLedger (renderer tests hand ledgers directly to renderLedgerSection), and serializeLedger really writes dropped when the count/byte caps trim. — Failure scenario: a future refactor silently zeroing the marker-borne terms would under-count or suppress the PARTIAL note on exactly the lossy long-lived-PR unions this PR added — a truncated merged list reads as complete.
Witness (round-1 mutation probe; line 1400 remains the only union-level dropped assertion at this commit): mutant const dropped = merged.length - capped.length; → entire pr-context.test.ts suite 98/98 GREEN (mutation survives the whole file); the fix below under the mutant → RED expected 5 to be 10.
| expect(atCap?.ledger.dropped).toBe(5); | |
| expect(atCap?.ledger.dropped).toBe(3 + 2 + 5); |
…together with giving the fixture's markers declared losses — add "dropped":3 to the own marker's JSON and "dropped":2 to the foreign one — so all three sum terms are pinned.
中文说明
第二轮复查：仍然存在。union 的 dropped 有三项——外部胜出者的 dropped、自己 marker 的 dropped、再截断（pr-context.ts:893-896）——但这个满容量夹具的来源 marker 都没带 dropped 字段，因此 recoverLedger 层面唯一的断言只钉住了再截断项；删掉任一 marker 携带项，整个套件仍全绿。没有其它测试把 marker 携带的 dropped 送进 recoverLedger（渲染器测试是把 ledger 直接交给 renderLedgerSection），而 serializeLedger 在数量/字节上限裁剪时确实会写 dropped。
失败场景：未来某次重构若静默把 marker 携带项清零，恰好会在本 PR 新增的有损长命 PR union 上少计甚至抑制 PARTIAL 注释——被截断的合并列表将显得完整。
证据（第一轮变异探针；本提交上 1400 行仍是唯一的 union 层 dropped 断言）：变异 const dropped = merged.length - capped.length; → 整个 pr-context.test.ts 98/98 全绿（变异在整个文件存活）；下方修复在变异下 → 红 expected 5 to be 10。
同时给夹具的两个 marker 声明损失——自己的 marker JSON 加 "dropped":3、外部的加 "dropped":2——使三项加和全部被钉住。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| '', | ||
| `**This list is PARTIAL**: ${ledger.dropped} further finding(s) from round ${ledger.round} did not fit the marker's size cap and are not here. Absence below is not evidence a finding was fixed — say so rather than reporting the missing ones as retired.`, | ||
| merged | ||
| ? `**This list is PARTIAL**: ${ledger.dropped} further finding(s) from the merged rounds did not fit the marker size caps (losses span both source markers and the merge's own re-cap, not round ${ledger.round}'s marker alone) and are not here. Absence below is not evidence a finding was fixed — say so rather than reporting the missing ones as retired.` |
There was a problem hiding this comment.
[Suggestion] R1-14: Still stands at this commit (round-2 re-check) — the merged PARTIAL note asserts a loss attribution that is false on shapes the union actually produces: "did not fit the marker size caps" and "losses span both source markers and the merge's own re-cap" — but dropped is a three-term sum (pr-context.ts:893-896) any subset of which may be zero, and the in-memory merge re-cap (merged.slice(0, LEDGER_MAX_FINDINGS)) is not a marker cap. The PR's own test proves the shape reachable (pr-context.test.ts:1358, 'the merge cap trims FOREIGN entries first'): own marker 50 complete entries (dropped 0) + foreign 5 disjoint (dropped 0) → union re-caps at 50 → dropped = 0+0+5, loss ONLY from the merge re-cap — yet the note claims both source markers lost entries to marker size caps. — Failure scenario: the renderLedgerSection docblock itself names the dismissal path this invites — a Step 6 reader cross-referencing both markers' bodies finds both complete while the note insists both lost entries, dismisses the warning as stale, with 5 findings genuinely absent from the work list. The actionable core ("Absence below is not evidence a finding was fixed") stays true in every shape, so this is a false attribution that invites dismissal, not a flipped ruling.
Witness (round-1 probe through the real wiring, recoverLedger → renderLedgerSection): merged: true, dropped: 5, the rendered note claimed both source markers lost entries while both were complete.
Suggested fix: make the attribution honest about the sum — e.g. "(the loss may come from either source marker's cap, the merge's re-cap, or both — do not assume any single round's marker lost entries)" — and update the renderer assertion at pr-context.test.ts:1705, which currently pins the false wording (necessary, not optional).
中文说明
第二轮复查：仍然存在。合并版 PARTIAL 注释的损失归属在 union 真实会产生的形态上是假的：「did not fit the marker size caps」与「losses span both source markers and the merge's own re-cap」——但 dropped 是三项之和（pr-context.ts:893-896），任一项都可能为零，而内存中的合并再截断（merged.slice(0, LEDGER_MAX_FINDINGS)）并不是 marker 容量上限。PR 自己的测试就证明该形态可达（pr-context.test.ts:1358「merge cap 先裁外部条目」）：自己的 marker 50 条完整（dropped 0）+ 外部 5 条不相交（dropped 0）→ union 在 50 处再截断 → dropped = 0+0+5，损失全部来自合并再截断——注释却声称两个来源 marker 都因容量上限丢了条目。
失败场景：renderLedgerSection 的 docblock 自己点名了由此引发的 dismiss 路径——Step 6 读者交叉核对两个 marker 的正文，发现两者都完整，而注释坚持两者都丢了条目，于是把警告当作过期信息 dismiss 掉，而工作列表确实缺了 5 条发现。注释的可操作核心（「列表缺失不代表发现已修复」）在所有形态下仍为真，因此这是一个诱发 dismiss 的虚假归属，而不是会直接翻转结论的错误。
证据（第一轮通过真实链路探针执行 recoverLedger → renderLedgerSection）：merged: true, dropped: 5，渲染出的注释声称两个 marker 都丢了条目，而两个来源 marker 都是完整的。
修复建议：让归属对三项之和保持诚实——例如「（损失可能来自任一来源 marker 的容量上限、合并再截断，或兼而有之——不要假定某一轮的 marker 丢了条目）」——并同步更新 pr-context.test.ts:1705 的渲染断言，它目前钉住的正是这句错误文案（此测试改动是必需的，不是可选的）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| 'continued (a continuation has no recorded scope to read, and answers ' + | ||
| '"ended before its test phase" without running anything): report the ' + | ||
| 'dimension UNFINISHED and do not spend a continuation on it. A resumed ' + | ||
| 'empty `test[]`, no `testScope`, and `"endedBeforeTests": true` — the ' + |
There was a problem hiding this comment.
[Suggestion] The rewritten third-shape continuation sentence is pinned by no test at either prose site — this round's new pins (the adjacent testScope.notRun/"clamped": true sentence) make the unpinned rewrite beside them stand out. No assertion in agent-prompt.test.ts matches endedBeforeTests, UNFINISHED, or third shape, neither in this resume paragraph nor in the role-7 base brief's matching sentence at lib/agent-briefs.ts:538. — Failure scenario: deleting the sentence ships green; Agent 7 then treats the endedBeforeTests shape as continuable and spends one of its MAX_RESUME_CALLS on a --resume that can only answer "ended before its test phase" — the wasted-continuation loop the sentence exists to prevent.
Witness (mutation probe this round): deleted the sentence at BOTH sites → full agent-prompt.test.ts suite Tests 263 passed (263) — the deletion ships green.
Suggested fix: pin a distinctive fragment per site, e.g. expect(p).toContain('"endedBeforeTests": true') and the 'do not spend a continuation on it' clause, on both the buildRoleBrief resume paragraph and the BRIEFS role-7 brief.
中文说明
重写后的「第三形态」续跑句子在两个文案位点都没有测试钉住——本轮新增的钉住（相邻的 testScope.notRun/"clamped": true 句）反而让旁边这句未钉住的重写显得突出。agent-prompt.test.ts 中没有任何断言匹配 endedBeforeTests、UNFINISHED 或 third shape，无论是这个续跑段落还是 lib/agent-briefs.ts:538 处 role-7 基础 brief 里的对应句子。
失败场景：删除这句话后整个套件仍绿；Agent 7 随后会把 endedBeforeTests 形态当作可续跑，把一次 MAX_RESUME_CALLS 花在一个只能回答「ended before its test phase」的 --resume 上——正是这句话要防止的浪费续跑循环。
证据（本轮变异探针）：在两个位点同时删除该句 → agent-prompt.test.ts 全套 Tests 263 passed (263)——删除后仍全绿。
修复建议：在两个位点各钉一个可区分的片段，例如对 buildRoleBrief 续跑段落与 BRIEFS role-7 brief 同时断言 expect(p).toContain('"endedBeforeTests": true') 与 'do not spend a continuation on it' 子句。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // existing suite as finished (multi-root runs carry the same fact in | ||
| // testScope.notRun; the stamp is simply the phase-level truth either way). | ||
| if (results.test.length === 0 && notRun.length > 0) { | ||
| results.endedBeforeTests = true; |
There was a problem hiding this comment.
[Suggestion] The resume merge inherits a stale endedBeforeTests: true stamp into the merged report even after the continuation ran suites — the one phase-level field not given the recompute/replace treatment the merge applies to ok, note, and caveat for exactly this staleness reason. The persisted report then asserts "the test phase ENTERED and ran nothing" (the field's own contract, build-test.ts:148) beside a test[] that shows suites ran. — Failure scenario: multi-root repo, install+build spends the whole-call budget → fresh report stamped endedBeforeTests: true with testScope.notRun listing the starved suites; the --resume continuation runs those suites and writes the merged report to --out still carrying the stamp. Every CURRENT reader is shape-guarded (the split reads the stamp only when test[] is empty and testScope absent), so nothing misreads it today — but the artifact this module persists for agents to trust contains a provably false structural claim, and any future reader keying on the stamp alone (which exists precisely so agents "never have to parse" prose) certifies "nothing ran" for a run that ran.
Witness (probe through the real runBuildTest this round): a multi-root previous report with endedBeforeTests: true resumed and ran both starved suites — merged report came back {"endedBeforeTests":true,"testCount":2,"ok":true}; the CLI handler writes that exact object to --out.
Suggested fix (in the resume merge, not at this fresh-path stamp site): recompute the stamp beside ok, e.g.
endedBeforeTests: mergedTest.length === 0 ? previous.endedBeforeTests : undefined,(JSON.stringify drops undefined keys).
中文说明
恢复合并（resume merge）会把过期的 endedBeforeTests: true 戳带进合并后的报告——即使续跑已经运行了套件。这是唯一一个没有像 ok、note、caveat 那样在合并时被重算/替换的阶段级字段，而那些字段被重算正是出于同样的过期原因。于是持久化的报告会在 test[] 明明显示套件已运行的同时，仍断言「测试阶段已进入但零套件运行」（该字段自己在 build-test.ts:148 的契约）。
失败场景：多根仓库，install+build 耗尽整体调用预算 → 新跑报告打上 endedBeforeTests: true，testScope.notRun 列出被饿死的套件；--resume 续跑运行这些套件后，写出的合并报告仍带着该戳。当前所有读取者都有形状守卫（split 只在 test[] 为空且无 testScope 时读戳），所以今天没有任何误读——但本模块持久化给 agent 信任的产物里包含一个可证伪的结构化断言，而未来任何只凭戳判断的读取者（戳的存在意义正是让 agent「永远不必解析」文案）会把一次真实运行过的跑批认证为「什么都没跑」。
证据（本轮通过真实 runBuildTest 探针执行）：带 endedBeforeTests: true 的多根 previous 报告恢复后运行了两个被饿死的套件——合并报告返回 {"endedBeforeTests":true,"testCount":2,"ok":true}；CLI handler 会把这个对象原样写入 --out。
修复位置在 resume 合并处而非本行（新跑打戳位点）：在重算 ok 的旁边重算该戳，例如 endedBeforeTests: mergedTest.length === 0 ? previous.endedBeforeTests : undefined（JSON.stringify 会丢弃 undefined 键）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Round 2 reports no blocker — nothing to fix under the Criticals-only rule governing this stack. The re-checked Suggestions (R1-2/3/7/8/9/11/12/13/14 and the new third-shape prose pin) are acknowledged and stay deferred, logged as candidates for the next backlog sweep — the same ledger discipline this PR itself is an instance of. 中文说明第二轮无阻断项——按本栈「只修 Critical」规则无需改动。 复检的 Suggestion（R1-2/3/7/8/9/11/12/13/14 及新增的第三形态文案钉）已确认并继续延后，记为下一次积压清理的候选——与本 PR 自身所示的台账纪律一致。 |
|
@qwen-code /triage |
…t from a merged one Every standing Suggestion from rounds 1-2, taken under the through-round-5 policy. The behavior half: - The union fires only over a NON-EMPTY own list (R1-11): an ordinary LGTM round posts findings: [], and flagging that shape merged made the provenance wording claim own-certified entries exist when none do. An empty own base recovers as what it is — pure foreign. - The merged PARTIAL note drops its false attribution (R1-14): dropped is a three-term sum any subset of which may be zero, and the re-cap is not a marker cap, so the note no longer claims both source markers lost entries to marker size caps — it names the possible sources without pinning any single round. - canonicalDimensionHead keeps the separator-less spellings (R1-7): a tight ampersand gains spaces before the hyphen collapse, and a squashed twin set accepts buildandtest / build andtest — the forms the replaced regex took via its optional separators, whose refusal re-opened the anchor-withholding cost on a spelling variant. - The fold key covers the fullwidth full stop U+FF0E (R1-9), the one ZH_TAIL stop character the CJK extension missed. - The call-site outcome summary says FOUR and names the anonymous outcome's sha/commitId drop (R1-8); the four deadline.ts phrase docblocks stop claiming a dedup-key role the splice no longer gives them (R1-12) — the splice keys on full canonical entry text. The pin half, each mutation-verified where a probe was named: - The gate's test clause gets an UNMASKED witness (R1-2: every other walked field present and valid, so the refusal rides on test alone) and the two newest clauses get theirs (R1-4: buildOnly 'yes', ok 'false', ok missing). - The main-path probe stamp's write half (R1-3): the fresh --build-only A/B asserts buildOnly true on the probe and absent on the sibling. - The merged passthrough is pinned at the handler (R1-5): an own marker plus a higher-round foreign winner renders the MERGED wording — the hardcode-false mutant fails exactly this. - The at-cap union fixture's markers declare their own losses (R1-13): dropped asserts the full three-term sum, so zeroing either marker-borne term goes red. - The third-shape continuation sentence is counted at BOTH prose sites (the round-2 addendum): two occurrences of the endedBeforeTests literal and the do-not-spend clause, so a one-site deletion drops the count.
|
All twelve standing Suggestions from rounds 1-2 are taken in d138578 — the deferral policy on this stack now runs full-take through round 5. Behavior: the union fires only over a non-empty own list (R1-11 — an LGTM round's Pins: the gate's 中文说明第 1-2 轮全部十二条在挂 Suggestion 已在 d138578 一并采纳——本栈的延后策略调整为第 5 轮（含）前全量处理。 行为半：union 仅在本账号清单非空时触发（R1-11，LGTM 轮的空清单按纯外部恢复）；合并 PARTIAL 注释不再把三项和钉到单一轮次或断言两个源标记都有丢失（R1-14）； 钉半：门禁 |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/pr-context.ts:1501 — [probe] merged forwarded unconditionally; merged:true with author:null renders a contradictory sectionpackages/cli/src/commands/review/lib/npm-toolchain.ts:354 — [review] caveat-side provisionalOnly exclusion branch never asserted on testScope.caveatpackages/cli/src/commands/review/pr-context.test.ts:2185 — [probe] new handler tests depend on real-FS state at a hardcoded /tmp side-file pathpackages/cli/src/commands/review/build-test.test.ts:1614 — [probe] stamp producer guard notRun.length > 0 has no witness in either direction
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // existing suite as finished (multi-root runs carry the same fact in | ||
| // testScope.notRun; the stamp is simply the phase-level truth either way). | ||
| if (results.test.length === 0 && notRun.length > 0) { | ||
| results.endedBeforeTests = true; |
There was a problem hiding this comment.
[Suggestion] R2-2: Still stands — the resume merge inherits a stale endedBeforeTests: true stamp into the merged report: the one phase-level field not given the recompute/replace treatment the merge applies to ok, note, and caveat. A multi-root run whose install+build spends the whole-call budget is stamped on the fresh path (this line) WITH testScope.notRun; --resume then runs the starved suites, and the merge's {...previous} spread (npm-toolchain.ts ~380-405) carries the stamp into an artifact whose test[] shows suites ran. — Failure scenario: multi-root repo, budget exhausted before tests → fresh report stamped endedBeforeTests: true → resume runs all starved suites green → the persisted merged report still asserts "the test phase ENTERED and ran nothing" beside a non-empty test[]. Every current reader is shape-guarded (the split reads the stamp only when test[] is empty and testScope absent), so nothing misreads today — but the stamp exists so readers never have to parse prose, and any future reader keying on it alone certifies "nothing ran" for a run that ran.
Witness:
probe (multi-root floored report, resume ran 3 suites):
PR: {"suitesRan":3,"endedBeforeTests":true,"okAfter":true}
flip (strip stamp when mergedTest.length > 0): {"suitesRan":3,"okAfter":true} — stamp gone, 104/104 green
Suggested fix (fix site is the resume merge object, ~380-405, not this stamp site):
endedBeforeTests: mergedTest.length === 0 ? previous.endedBeforeTests : undefined,
// JSON.stringify drops undefined keys中文说明
R2-2：仍然存在 —— resume 合并会把过期的 endedBeforeTests: true 戳带进合并后的报告：这是合并对 ok、note、caveat 都做了重算/替换、唯独没有同样处理的阶段级字段。多根仓库的 install+build 耗尽整体预算时会在新跑路径（本行）打上该戳、且带 testScope.notRun；--resume 随后跑完被饿死的套件，而合并处的 {...previous} 展开（npm-toolchain.ts ~380-405）把戳带进 test[] 非空的产物。失败场景：多根仓库、测试前预算耗尽 → 新跑报告带 endedBeforeTests: true → resume 全部跑绿 → 持久化的合并报告仍断言「测试阶段已进入且什么都没跑」。当前所有读取方都有形状守卫（split 只在 test[] 为空且无 testScope 时读戳），今天没有误读——但该戳的存在意义就是让读取方不必解析 prose，未来任何只凭戳判断的读取方都会把跑过的运行认证为「什么都没跑」。证据：探针（多根预算耗尽报告、resume 跑 3 个套件）显示 PR 侧 {"suitesRan":3,"endedBeforeTests":true,"okAfter":true}；按隐含修复（mergedTest 非空时去戳）翻转后戳消失（104/104 绿）。修复位置在 resume 合并对象（~380-405）：endedBeforeTests: mergedTest.length === 0 ? previous.endedBeforeTests : undefined（JSON.stringify 会丢弃 undefined 键）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ); | ||
| // …and the entry still counts against the anchor (not a relay, not | ||
| // depth-only): the marker is withheld, exactly as when it was spliced. | ||
| expect(r.body).not.toContain('"sha"'); |
There was a problem hiding this comment.
[Suggestion] Vacuous marker-withholding assertion: the fixture mints no marker, so this assertion passes whatever the anchor decision. coveredPlan() writes prNumber/fetchedSha only when passed (compose-review.test.ts:158/169) — here neither is — so ledgerMarkerFor returns null at if (!isPr) return null (compose-review.ts:974) before the anchor decision ever runs, and "sha" cannot appear in r.body regardless. — Failure scenario: a regression that re-classified the free-form budget-mentioning entry as a depth-only relay (the case that would LET the anchor ride) keeps this assertion green. The property is genuinely pinned only by the sibling test 'sees a lens gap that merely mentions the budget in its reason' (~line 5064, which sets prNumber: 8255, fetchedSha: 'deadbeef00112233'); if that sibling is later edited or removed, this test still passes while anchor-counting for free-form disclosures silently regresses.
Suggested fix:
// mirror the sibling's fixture so a marker can actually be minted:
const plan = coveredPlan(['verify', 'reverse-audit'], {
prNumber: 8255,
fetchedSha: 'deadbeef00112233',
});
// ...and assert the withholding structurally:
expect(parseLedger(r.body)?.sha).toBeUndefined();(or drop the assertion and the last two comment lines)
中文说明
空洞的「marker 扣住」断言：该夹具根本不会铸出 marker，所以无论锚决策如何此断言都通过。coveredPlan() 只在传参时写入 prNumber/fetchedSha（compose-review.test.ts:158/169），此处两者都没传，ledgerMarkerFor 在 compose-review.ts:974 的 if (!isPr) return null 处直接返回 null，根本走不到锚决策，r.body 里无论如何都不会出现 "sha"。失败场景：若回归把「自由文本提及预算」的条目重新归类为 depth-only relay（正是会放行锚的情形），此断言仍绿。该性质实际只由兄弟测试 'sees a lens gap that merely mentions the budget in its reason'（~5064 行，设置了 prNumber/fetchedSha）钉住；若兄弟测试被改删，本测试仍绿，而自由文本披露的锚计数行为已静默回归。修复建议：镜像兄弟测试的夹具（coveredPlan(['verify', 'reverse-audit'], { prNumber: 8255, fetchedSha: 'deadbeef00112233' })）并改用 expect(parseLedger(r.body)?.sha).toBeUndefined()；或直接删除该断言及末两行注释。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| * dedup must read BOTH languages: a relayed Chinese stop entry that only the | ||
| * English phrase was checked against survived the splice and was rendered | ||
| * under the whiffed-agent cause beside the structural stop line. */ | ||
| /** The Chinese pair, spelled into `budgetStopEntryZh`. Same non-dedup-key |
There was a problem hiding this comment.
[Suggestion] BUDGET_STOP_PHRASE_ZH / ROUND_CAP_PHRASE_ZH are orphaned by this PR's splice deletion: the diff removes the phrases array in compose-review.ts — their only read site — leaving both exports with zero consumers repo-wide (verified: exactly 2 grep hits, both declarations; deadline.test.ts imports only the English pair), while these new doc comments describe them as load-bearing vocabulary ("spelled into budgetStopEntryZh"). The asymmetry is real: budgetStopDisclosure interpolates BUDGET_STOP_PHRASE (deadline.ts:539) and roundCapStopDisclosure interpolates ROUND_CAP_PHRASE (:579), but the zh reasons hardcode the phrase text (评审时间预算不足…, …反审轮数上限内未收敛). — Concrete cost: two dead exports whose docs assert a single-source spelling the code does not honor — a future reword of a zh stop entry silently diverges from the constant that claims to spell it, and the next deletion audit must re-derive that these are orphans.
Suggested fix: either interpolate the constants into the zh reasons to match the English discipline the docs describe —
reasonZh: `…${BUDGET_STOP_PHRASE_ZH}不足，未能开始${whichZh}…`
// and: `在 ${cap} 轮的${ROUND_CAP_PHRASE_ZH}内未收敛`— or delete both orphaned exports and their doc comments.
中文说明
本 PR 删除 compose-review.ts 中的 phrases 数组（BUDGET_STOP_PHRASE_ZH / ROUND_CAP_PHRASE_ZH 的唯一读取点）后，这两个导出在整库零消费者（已验证：grep 仅 2 处命中、均为声明处；deadline.test.ts 只导入英文对），而新加的文档注释却称其为承重词汇（"spelled into budgetStopEntryZh"）。不对称真实存在：英文侧 budgetStopDisclosure 插值 BUDGET_STOP_PHRASE（deadline.ts:539）、roundCapStopDisclosure 插值 ROUND_CAP_PHRASE（:579），中文 reason 却硬编码文案。具体代价：两个死导出的文档宣称「单一来源拼写」而代码并不兑现——未来改写中文停止条目会与该常量静默脱钩，下一次删除审计还得重新推导它们是孤儿。修复建议：要么在中文 reason 中插值这两个常量以匹配文档描述的英文纪律，要么删除两个孤儿导出及其文档注释。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| build: [], | ||
| timedOut: [], | ||
| }, | ||
| // The two newest clauses need their own witnesses too: a corrupted |
There was a problem hiding this comment.
[Suggestion] This comment promises witnesses for "the two newest clauses", but only one got one: the refusal-fixture loop below gains witnesses for every other new previousReport clause (buildOnly: 'yes', ok: 'false', missing ok, missing test) — none for a corrupted endedBeforeTests stamp. — Failure scenario: mutant: delete !endedBeforeOk from the conjunction in previousReport (build-test.ts:~582) — every fixture stays green, because no fixture carries e.g. endedBeforeTests: 'yes'. A report file corrupted to that shape then clears the gate, previous.endedBeforeTests === true reads false in the resume split, and a budget-floored single-root report is misclassified as "completed with no suite to run" — certifying the unrun suite as finished, the exact misclassification the stamp exists to prevent.
Suggested fix — add one fixture to the loop:
{
toolchain: 'npm',
affected: ['packages/core'],
ok: true,
endedBeforeTests: 'yes',
test: [],
build: [],
timedOut: [],
},中文说明
此注释承诺「最新的两个子句各有见证」，但实际只给了一个：下方拒绝夹具循环为其它每个新 previousReport 子句都加了见证（buildOnly: 'yes'、ok: 'false'、缺 ok、缺 test），唯独没有损坏的 endedBeforeTests 戳的见证。失败场景：变异体——从 previousReport 的合取中删除 !endedBeforeOk（build-test.ts:~582），所有夹具仍绿，因为没有夹具携带如 endedBeforeTests: 'yes'。损坏成该形状的报告文件将清掉门槛，previous.endedBeforeTests === true 在 resume split 中读作 false，预算地板处的单根报告被误判为「completed with no suite to run」——把未运行的套件认证为已完成，正是该戳要防止的误分类。修复建议：向循环添加夹具 { toolchain: 'npm', affected: ['packages/core'], ok: true, endedBeforeTests: 'yes', test: [], build: [], timedOut: [] }。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ], | ||
| 'maintainer', | ||
| ); | ||
| expect(atBound?.ledger.round).toBe(8 + FOREIGN_ROUND_HEADROOM); |
There was a problem hiding this comment.
[Suggestion] These SYMBOLIC boundary fixtures cannot kill the FOREIGN_ROUND_HEADROOM value mutants their own comment claims to kill: fixture rounds AND expected values are both computed from the imported constant, so any mutated value satisfies the boundary arithmetic in lockstep, and nothing else in the suite pins 64 (the literal fixtures constrain it only to a wide interval — tightest bounds H ≥ 6 and H ≤ 499). — Failure scenario: edit FOREIGN_ROUND_HEADROOM from 64 to 6 and the entire suite stays green (probed). H=6 then ships: a legitimate interleaved ci-bot marker 7+ rounds ahead is refused at the bound (pr-context.ts:832), recovery falls back to the own marker, and the re-run re-reviews the full diff of an unchanged PR — the regression the module's own docstring cites as measured (~119/128 minutes, ~34M tokens). H=499 ships the opposite: the counter-inflation bound widens ~8x.
Witness:
probe: FOREIGN_ROUND_HEADROOM 64→6 → 99/99 green (mutant ships)
+ literal pin expect(FOREIGN_ROUND_HEADROOM).toBe(64) → AssertionError: expected 6 to be 64 (1 failed | 98 passed)
pin and mutant reverted → 203/203 green
Suggested fix:
expect(FOREIGN_ROUND_HEADROOM).toBe(64);beside this block — or pin with literals on at least one side (marker(8 + 64) adopted / marker(8 + 65) refused, expecting own round 8).
中文说明
这些「符号化」边界夹具无法击杀其注释自称已击杀的 FOREIGN_ROUND_HEADROOM 取值变异：夹具轮次与期望值都由导入的常量计算，任何变异值都随算术联动产物自洽，套件中也没有别处钉住 64（字面量夹具只把常量约束在宽区间内——最紧界为 H ≥ 6 与 H ≤ 499）。失败场景：把 FOREIGN_ROUND_HEADROOM 从 64 改为 6，整套测试仍绿（已探针验证）。H=6 上线后：合法插队、领先 7 轮以上的 ci-bot 标记会在边界处被拒（pr-context.ts:832），恢复退回自身标记，重跑将对未变更的 PR 全量重审——正是模块 docstring 引为实测的回归（~119/128 分钟、~34M tokens）。H=499 则走向反面：对抗灌水边界放宽约 8 倍。修复建议：在该块旁加 expect(FOREIGN_ROUND_HEADROOM).toBe(64);，或至少一侧用字面量钉住（marker(8 + 64) 被采纳 / marker(8 + 65) 被拒、期望自身轮次 8）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // full stop set — including the fullwidth full stop ．(U+FF0E) beside 。 | ||
| // (U+3002): the classifier treats both as trailing, and a fold key that | ||
| // dropped only one of them kept the double-spend open for the other. | ||
| const TRAILING_GAP_CHAR_RE = /[.!…,;:\s。，；：！？、．]/; |
There was a problem hiding this comment.
[Suggestion] The widened trailing class and the classifier's unchanged ZH_TAIL diverge on ？(U+FF1F)/、(U+3001), re-opening the bare-vs-wrapped split this hunk's unwrap was added to close: the new full-width unwrap strips one paren layer, then judges the inner text against PLACEHOLDER_GAP_RE, whose ZH_TAIL ([。．.!！…,，;；:：\s]*$) lacks ？ and 、 — so a wrapped placeholder's inner trailing ？/、 is never stripped while the bare form's is (via this widened class). — Failure scenario: under outputLanguage: 中文 an agent returns Budget gap: （无？） or （无、） — the "nothing to disclose" non-answer in full-width parens, the exact incident shape the unwrap closes. Bare 无？ drops but wrapped （无？） survives as a gap, is handed to the orchestrator to rule on, and renders in the posted body — costing real rounds for a non-answer, and judging identical content differently bare vs wrapped. Over-disclosure direction, hence Suggestion.
Witness (A/B through the real classifier entry point):
BASE(merge base): 无？→["无？"] （无？）→["（无？）"] 无、→["无、"] （无、）→["（无、）"] (consistent)
PR (this commit): 无？→[] （无？）→["（无？）"] 无、→[] （无、）→["（无、）"] (split introduced)
PR + ZH_TAIL fix: （无？）→[] （无、）→[] real-gap control still keeps; budget.test.ts 75/75
Suggested fix — mirror the two missing tail chars into ZH_TAIL (~line 745) so bare and wrapped judge identically:
const ZH_TAIL = '[。．.!！…,，;；:：？、\\s]*$';and add a fixture pair beside the new full-width tests: （无？） and （无、） drop.
中文说明
放宽后的尾字符类与分类器未改动的 ZH_TAIL 在 ？(U+FF1F)/、(U+3001) 上发生分叉，重新打开了本 hunk 的全角解包要消灭的「裸写/带括号」分裂：新的全角解包剥掉一层括号后，把内层文本交给 PLACEHOLDER_GAP_RE 判断，而其 ZH_TAIL 不含 ？ 与 、——于是带括号占位符的内层尾部 ？/、 永不被剥离，裸写形式却会（经由这个放宽的字符类）。失败场景：outputLanguage: 中文 下 agent 返回 Budget gap: （无？） 或 （无、）——全角括号包裹的「无可披露」空答案，正是解包要闭合的事变形状。裸写 无？ 被丢弃，带括号的 （无？） 却作为缺口存活、交给编排者裁决并渲染进发布的正文——为一个空答案耗费真实轮次，且同样内容裸写与带括号判定不一（过度披露方向，故为建议级）。证据：真实分类器入口 A/B——基线侧两形态一致保留；PR 侧裸写被丢弃、带括号存活（分裂由本 diff 引入）；按建议修复后带括号形态也丢弃、真实缺口对照仍保留（budget.test.ts 75/75）。修复建议：把缺失的两个尾字符镜像进 ZH_TAIL（~745 行），并在新的全角测试旁补 （无？）、（无、） 被丢弃的夹具对。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…ran-nothing stamp
All six round-3 Suggestions, taken under the through-round-5 policy.
The behavior half:
- The resume merge recomputes endedBeforeTests beside ok (R2-2): the
stamp claims the test phase ENTERED and ran nothing, and a
continuation that runs the starved suites falsifies it — the
{...previous} spread carried it stale into an artifact whose test[]
showed suites ran. The merge keeps it only while mergedTest is empty
(the claim still holds then); a resume that ran suites drops it, and
JSON.stringify sheds the undefined key. Every current reader is
shape-guarded, so nothing misread today — but the stamp exists so a
reader never has to parse prose, and a future reader keying on it
alone would certify "nothing ran" for a run that ran.
The pin half, each mutation-verified:
- The free-form budget test mints a real marker (R3-1): the plan now
carries prNumber/fetchedSha, so the anchor decision actually runs
and the withholding assertion reads the parsed ledger — the vacuous
not-toContain sha passed whatever the decision. The input is built
directly, not through base(): its default coveredPlan() rewrites
the shared plan.json and drops the PR identity before the override
takes effect. The exempt-all isRelayedStopEntry mutant flips the
sha assertion.
- The refusal loop gains the endedBeforeTests 'yes' witness (R3-3):
the comment promised witnesses for the two newest clauses but only
buildOnly had one — deleting !endedBeforeOk from the gate shipped
green. The gate-mutation probe now fails exactly the new fixture.
- FOREIGN_ROUND_HEADROOM is pinned literally at 64 (R3-4): the
symbolic boundary fixtures compute both sides from the constant, so
any mutated value satisfies the arithmetic in lockstep — the 64->6
mutant shipped green before and fails the pin now.
- ZH_TAIL mirrors the fold key's ？ (U+FF1F) and 、 (U+3001) (R3-5):
the full-width unwrap judges the inner text against the classifier,
whose tail class lacked both — bare 无？ dropped while wrapped
（无？） survived as a gap, identical content judged differently
bare vs wrapped. The fixture pair drops both.
- The two zh phrase constants are interpolated into the zh reasons
(R3-2): the splice deletion left them with zero read sites while
their docblocks claimed they spell the entries — they do now,
matching the English discipline, rendered text byte-identical.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9342 (round taking the six round-3 Suggestions)DispositionsImplemented this round (six, commit
Re-verified as already resolved in code (thirteen — the round-1 Critical plus the twelve round-1/2 Suggestions taken in
Declined / deferred / escalated: none. No conflict resolution was needed ( Verification
中文说明轮次总结 — PR #9342（采纳第 3 轮六条 Suggestion 的一轮）处理结果本轮实现（六条，提交
复核确认代码中已解决（十三条 —— 第 1 轮 Critical 及已在
拒绝 / 延后 / 升级维护者裁决： 无。无需冲突解决（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review: PR #9342 — fix(review): clear the deferred-suggestion backlog from #9175's review rounds
Reviewed at: 92ccb889dfcde9ee26aff2a29346dc99f741dc7e
7 chunks, 12 agents, 0 budget gaps. No Criticals, no blockers.
Summary
The PR faithfully clears the 19-item deferred backlog from #9175. Every change carries its pin — the safety-shaped persistRecoveredLedger API, the shared TEST_COMMAND_RE grammar, the endedBeforeTests structural stamp, BRIEFS-derived dimension exemption, full-entry stop-relay splice, merged provenance rendering, bilingual CJK classifier fixes, and the resumed-run caveat corrections. The test work is the strong kind: symbolic boundary pins, fence-scoped prompt assertions, handler-level wiring pins.
The blocker R1-1 (Critical, stale) is fully addressed: endedBeforeTests stamp on both producer shapes, buildOnly stamp on the deliberate probe, previousReport shape validation, updated prose in agent-briefs.ts/agent-prompt.ts, and the merge recompute that clears the stamp when continuation ran suites.
Findings
Two Suggestion-level coverage gaps:
-
deferredRoundclamping untested (compose-review.ts:1024) —Math.min(prevRound + 1, LEDGER_MAX_ROUND)has no test exercising theprevRound === LEDGER_MAX_ROUNDcap. A mutation replacingMath.minwithprevRound + 1would stay green. (The gap is low-risk:LEDGER_MAX_ROUND = 10000, and no real scenario reaches it.) -
buildOnly/endedBeforeTestsfield declarations not directly gated (build-test.ts:131,148) — The interface membersbuildOnly?: booleanandendedBeforeTests?: booleanhave no test that fails when they are removed or renamed. The validation logic that reads them (inpreviousReport) is gated, but the declarations themselves are not.
Existing comments
30 existing inline comments remain open. The single blocker (R1-1) is stale and fixed. The 17 no-conflict Suggestions are all pre-existing and re-checked — they stand as recorded.
Not reviewed
Build-and-test did not run: the worktree environment lacks Python for node-gyp rebuild on Windows, blocking packages/audio-capture before the affected workspace (packages/cli) was reached. The test-efficacy harness ran and confirmed 6 probed assertions, 3 mutants, 6 hunk probes, and 1 killed mutant.
Verdict
COMMENT — no blockers. The two Suggestion-level coverage gaps are acknowledged. CI is still pending (all_pending), so the event is downgraded from APPROVE per the presubmit gate.
审查：PR #9342 — 修复（审查）：清除 #9175 审查轮次中积累的延后建议积压
审查提交： 92ccb889dfcde9ee26aff2a29346dc99f741dc7e
7 个区块，12 个代理，0 个预算缺口。 无关键问题，无阻断项。
总结
该 PR 忠实清除了 #9175 的 19 项延后积压。每项变更都有对应的测试钉住——安全化的 persistRecoveredLedger API、共享的 TEST_COMMAND_RE 语法、endedBeforeTests 结构戳、BRIEFS 派生的维度豁免、完整条目的 stop-relay 拼接、合并溯源渲染、双语 CJK 分类器修复以及恢复运行注意事项修正。测试工作属于强类型：符号化边界钉、围栏作用域提示词断言、处理器级接线钉。
阻断项 R1-1（关键，已过期）已完全修复：两种生产者形态的 endedBeforeTests 戳、刻意探针的 buildOnly 戳、previousReport 形状校验、agent-briefs.ts/agent-prompt.ts 中更新的文案，以及在继续运行执行了套件时清除该戳的合并重新计算。
发现
两个建议级覆盖面缺口：
-
deferredRound钳制未测试（compose-review.ts:1024）——Math.min(prevRound + 1, LEDGER_MAX_ROUND)没有测试用例覆盖prevRound === LEDGER_MAX_ROUND的封顶场景。将该处替换为prevRound + 1的变异会使测试套件保持绿色。（风险较低：LEDGER_MAX_ROUND = 10000，实际场景无法达到。） -
buildOnly/endedBeforeTests字段声明无直接测试钉住（build-test.ts:131,148）——接口成员buildOnly?: boolean和endedBeforeTests?: boolean没有测试在它们被删除或重命名时失败。读取这些字段的验证逻辑（previousReport中）有测试，但声明本身没有。
现有评论
30 条现有行内评论仍处于开放状态。唯一的阻断项（R1-1）已过期且已修复。17 条无冲突的建议均为预先存在并已重新检查——它们按记录保持。
未审查
构建和测试未运行：工作树环境缺少 Python 的 node-gyp rebuild（Windows），在到达受影响的工作区（packages/cli）之前被 packages/audio-capture 阻塞。测试有效性工具已运行并确认了 6 个探针断言、3 个变异体、6 个代码块探针和 1 个击杀变异体。
裁决
COMMENT — 无阻断项。两个建议级覆盖面缺口已记录。CI 仍在运行中（all_pending），因此根据预提交门控，事件从 APPROVE 降级为 COMMENT。
— Qwen Code
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 — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:2337 — [review] At-cap branch of the deferred-round clamp has no paired testpackages/cli/src/commands/review/compose-review.test.ts:5070 — [review] Exemption-variants list omits the … verification dressing the replaced regex acceptedpackages/cli/src/commands/review/pr-context.ts:1169 — [review] Merged provenance sentence over-promises on the same-round shapepackages/cli/src/commands/review/build-test.ts:137 — [test] hunk-survived: the buildOnly/endedBeforeTests interface declarations ride on typecheck alone (residual sliver of the R1-3 lineage)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // ？ (U+FF1F) and 、 (U+3001) mirror the fold key's trailing strip: | ||
| // a wrapped placeholder's inner text must judge identically to its | ||
| // bare form, which loses those tails before the classifier sees it. | ||
| const ZH_TAIL = '[。．.!！…,，;；:：？、\\s]*$'; |
There was a problem hiding this comment.
[Suggestion] The round-3 fix (the widened ZH_TAIL above) completed bare-vs-wrapped parity for the ZH classifier branch, but left the English branch split open: the normalize/fold strip (TRAILING_GAP_CHAR_RE, :832 below) now strips CJK trailing punctuation from bare gaps, while every EN-branch tail class in PLACEHOLDER_GAP_RE (:765–775) is still ASCII-only ([.!…,;:\s]*$). A wrapped English placeholder carrying a CJK trailing stop therefore survives as a phantom gap while its identical bare twin is dropped — exactly the bare-vs-wrapped split the comment added by this fix promises is closed ("a wrapped placeholder's inner text must judge identically to its bare form"). This is the still-open sibling entrance of the R3-5 family. — Concrete cost: under CJK output language an agent writing Budget gap: (none。) publishes the phantom (none。) into the body's budget-gap block while bare none。 drops — consuming one of 8 MAX_GAPS_PER_AGENT slots and an orchestrator ruling. Direction is safe (over-disclosure, never a dropped real gap), so bounded phantom rulings, not a wrong verdict.
Witness (probe on this PR's code):
bare none。 => []
wrapped (none。) => ["(none。)"]
wrapped （none。） => ["（none。）"]
bare stayed under budget。 => []
wrapped (N/A - stayed under budget。) => ["(N/A - stayed under budget。)"]
ZH parity: （无。） => []
FLIP: widening the five EN tail classes to the TRAILING_GAP_CHAR_RE set
collapses the split (75/75 budget tests stay green)
Suggested fix: widen the five EN-branch tail classes in PLACEHOLDER_GAP_RE to the TRAILING_GAP_CHAR_RE set — or apply stripTrailingGapChars to the unwrapped inner text before classification — and extend the new parity test with Budget gap: (none。) / Budget gap: （none。） fixtures. (The fix spans lines outside this hunk, so no one-click suggestion block.)
中文说明
第 3 轮的修复（上方拓宽的 ZH_TAIL）补齐了中文分类分支的裸/包裹判断奇偶性，但英文分支仍然裂开：归一化/折叠剥离（TRAILING_GAP_CHAR_RE，下方 :832）现在会从裸缺口剥离全角尾标点，而 PLACEHOLDER_GAP_RE（:765–775）中所有英文分支的尾类仍是纯 ASCII（[.!…,;:\s]*$）。携带全角句号的包裹英文占位符会作为幽灵缺口存活，而内容相同的裸形态却被丢弃——正是本修复新增注释承诺已经关闭的裸/包裹分裂（「包裹占位符的内文必须与其裸形态判断一致」）。这是 R3-5 家族仍然敞开的同类入口。——具体代价：中文输出语言下，agent 写 Budget gap: (none。) 时，裸 none。 被丢弃而包裹 (none。) 作为幽灵进入正文的预算缺口块，消耗 8 个 MAX_GAPS_PER_AGENT 名额之一并引来一次编排裁定。方向安全（过度披露，绝不丢弃真实缺口），代价是有限的幽灵裁定，而非错误裁决。
证据（在本 PR 代码上探针执行）：裸 none。 => []；包裹 (none。) => ["(none。)"]；（none。） => ["（none。）"]；裸 stayed under budget。 => []；包裹形态存活；中文对照 （无。） => [] 保持奇偶。翻转验证：把五个英文尾类拓宽为 TRAILING_GAP_CHAR_RE 集合后分裂消失（75/75 budget 测试仍绿）。
修复建议：把 PLACEHOLDER_GAP_RE 中五个英文分支的尾类拓宽为 TRAILING_GAP_CHAR_RE 的集合——或在分类前对去括号后的内文应用 stripTrailingGapChars——并为新的奇偶测试补 Budget gap: (none。) / Budget gap: （none。） 夹具。（修复涉及本块之外的多行，故不提供一键 suggestion。）
— qwen3.8-max via Qwen Code /review (v0.21.13)
…he deferred-round clamp pins at the cap
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Round 4 address summary — PR #9342Commit: Feedback points and dispositions1.
|
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D5-2 hunk-survived: the buildOnly/endedBeforeTests interface declarations ride on typecheck alone — already recorded in the round-4 deferred list (review 4955731398)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 7: none — finished all checks within budget (~11 of ~37 calls)..
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/pr-context.ts:891 — [probe] mergedOverOwn decided pre-composition — mixed-provenance wording renders over 100%-own lists (zero-contribution, at-cap re-cap, and same-round id-collision triggers)packages/cli/src/commands/review/agent-prompt.test.ts:2707 — [probe] first build-test fence pins are asymmetric — wrapper/--worktree pinned, --plan/--out not (deletion ships green; first call then dies on demandOption)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 7：none — finished all checks within budget (~11 of ~37 calls).。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round triage — no new actionable feedback, no code changesThe only feedback newer than the last evaluation is automated review
No merge was performed (conflict flag: false; no conflict with main). No 中文说明Autofix 轮次分诊 — 无新的可处理反馈，不做代码变更上次评估之后唯一的增量反馈是自动审查
本轮未执行合并（冲突标志：false；与 main 无冲突）。无提交；分支头仍为 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
@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: 85 passed · 0 failed · 85 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：85 通过 · 0 失败 · 85 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) — follow-up round at head Scripted assertions: 85 passed · 0 failed · 85 total. Verified head: 中文 — 判定：✅ 通过 · 可合入（agent 判定，跟进轮）
Previous-round finding status (re-measured at the new head, never diffed)
Carried-forward "not covered" rows (re-checked): per-commit attribution still out of reach (shallow, grafted — see below); no end-to-end Delta since the previous round (scope of the new probes)Two new commits:
Central claim + A/B (re-run at the new base)Central claim (carried): the batch's behavior fixes flip broken behavior to fixed and the pins hold it down. Verified head
The 24 reds attribute as: resume split/stamps 8 (build-test), ZH classifier 3 (budget), free-form splice survival + deferred clamp 2 (compose-review), merged provenance + headroom + union + host-baking trio + handler wiring 9 (pr-context), signature-flip through the adapted call 1 (persist: "a recovery that THREW…"), Agent-7 continuation prose 1 (agent-prompt). Versus round 1's 23, the +1 is the main-brought persisted-anchor test adapted by Control purity: the effective diff touches no Classifier direct A/B (
|
| mutant | suite | result |
|---|---|---|
| unmutated control (4 target suites) | vitest | 471/471 green |
| M1: fullwidth-paren unwrap reverted | budget.test.ts | 2 red — exactly "strips full-width parens too" + "closes the same split for ENGLISH no-answers wearing a CJK tail"; the fold test correctly survives (different hunk, pinned by its own test) |
M2: deferred-round Math.min(…, LEDGER_MAX_ROUND) removed |
compose-review.test.ts | 1 red — exactly "names the round AT the ledger cap — the clause and the marker agree" |
| M3: anonymous guard disabled | pr-context(-persist) | 2 red — exactly the two ANONYMOUS persist tests |
Positive control: M1–M3 are themselves the expected-caught mutations; each killed exactly its intended fixture and nothing else.
Named-flags swap unrepresentable (R14-3) — 04-swap-probe-typecheck-flip.png
Probe persistRecoveredLedger("/tmp/x.json", null, true, false): base tsc --noEmit exit 0 (empty output — the swap compiled); head exit 2 with exactly error TS2554: Expected 3 arguments, but got 4.; probe removed, head clean gate exit 0 with zero output.
Targeted gates & identity checks
npx vitest run src/commands/reviewat head: 83 files, 3383 passed | 4 skipped (head-gate-review-suite.log,head-gate-junit.xml).tsc --noEmitover packages/cli at head: 0 errors.TEST_COMMAND_RE(R13-3): head's test-delta imports it fromlib/npm-toolchain.jsand keeps no local regex literal; base's local copy and base's export are byte-identical (/^npm test(?: --workspace="[\w@./-]+")?$/) — scripted check PASS.FOREIGN_ROUND_HEADROOMliteral pin (R3-4):expect(FOREIGN_ROUND_HEADROOM).toBe(64)present at pr-context.test.ts:1501 beside the symbolic boundary fixtures — scripted check PASS.
Corrections
- The PR body says "3266 tests across 83 files pass"; measured at the new head: 3383 passed | 4 skipped (3387), 83 files. The count has been stale since the body was written and moved further with the main merge. Description inaccuracy only.
Findings (non-blocking)
- Splice over-matches quoted canonical text — stands (documented tradeoff). Re-measured with a fresh probe at the new head: a free-form disclosure that quotes the canonical stop entry verbatim mid-prose (
security — per the record 'reverse audit — stopped before round 4 by the review time budget' the relaunch never ran) is spliced from the rendered body while the structural stop line renders and the ledger stays intact. The code comment names this as the accepted verbatim/reshaped-relay treatment; the safe direction is preserved. No action needed. - Stale test count in the body — see Corrections.
- New: the effective diff carries 3 formatting-drift files from the branch's merge of main.
docs/developers/daemon/17-configuration.mdanddocs/users/qwen-serve.mddiffer from the base tip only in table padding (120 + 72 table rows content-identical after padding normalization — scripted check PASS), andpackages/web-shell/client/i18n.tsxby a single ternary collapse whose multi-line and one-line forms evaluate identically across 5 probe inputs (the ZHturnOutputs.artifactUnavailablekey does not exist at the pre-merge head902db628, so the drift rode in with the merge; the merged main state of these files differs from the recorded base tip). Semantically neutral; merging will apply harmless formatting noise to main. Noted so a reviewer isn't surprised; no action required.
Not covered
- Per-commit attribution: shallow checkout —
git rev-list HEAD^1..HEAD^2= 1 vs 7 commits in the metadata. Six of the seven commit objects exist locally but the history is grafted (the merge commit7738c47cis missing), so per-commit diffs were not individually exercised; verified the aggregateHEAD^1..HEADdiff. The delta's adaptation hunks were attributed to999f61d9by content + commit message, cross-checked against the aggregate. - End-to-end
--resume/pr-contextruns against a live repo: all resume/persist behavior verified at unit/handler level with real fixtures and the both-tree equivalence harness; no full CLI invocation. - The two deferred items (R4-4 failing-file parser redesign, R11-9) — out of this PR's scope by design.
- Windows/macOS — Linux container only.
- The 4
skippedtests in the review suite are pre-existing skips (not introduced by this PR; not investigated further).
Methodology
Environment: CI merge-ref checkout (depth 2; HEAD 88665968c5, base tip HEAD^1 = 179c8f80fd, PR head HEAD^2 = 999f61d9), node 22 container, no GitHub token. Harnesses drove source TS via tsx/vitest with no mocks of the code under test; the base arm is a git worktree at HEAD^1 with head's six test files copied in (A/A control run first: base's own files 825/825) and package-level node_modules symlinked from the unchanged install (realpath-asserted; core/webui/lockfile proven identical across arms). Mutations applied in a second scratch worktree at HEAD, each reverted via git checkout after its run. Two harness expectations were corrected mid-round before the counted runs (the fold cells' surviving surface form — the PR's own fold test pins first-occurrence raw form; and the F1 probe's plan fixture, which needed a marker-minting plan for the ledger assertion); the counts above are the final runs. Raw logs: ab-classifier-{base,head}.log, ladder-{base,head}.log, base-arm-6files.log, red-list.txt, adapt-equivalence.log, mutation-{control,m1,m2,m3}.log, f1-probe.log, typecheck-{base-with-probe,head-with-probe,head-clean}.log, head-gate-review-suite.log, base-tree-build*.log in this directory; captures 01–06 in evidence/. Assertion counts map one-to-one to the cells above: 1 A/A + 24 base-arm reds + 10+10 classifier + 5+5 ladder + 14 equivalence + 4 mutation cells + 1 F1 probe + 2 swap probe + 2 gates + 4 identity checks + 3 formatting-neutrality checks = 85.
— Qwen Code · sandboxed verification
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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D6-9 hunk-survived: the buildOnly/endedBeforeTests interface declarations ride on typecheck alone (residual sliver of the R1-3 lineage) — already recorded in the round-4 deferred list (review 4955731398)
- D6-10 first build-test fence pins are asymmetric — wrapper/--worktree pinned, --plan/--out not — already recorded in the round-5 deferred list (agent-prompt.test.ts:2707)
- D6-11 ASCII ? missing from TRAILING_GAP_CHAR_RE while fullwidth ？ is added — already recorded in the round-2 deferred list (review 4952261985)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 5: none — no check was cut short..
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/i18n.tsx:4192 — [probe] merge commit injected formatting-only changes into three out-of-scope files (i18n.tsx + two docs)packages/cli/src/commands/review/lib/npm-toolchain.ts:1089 — [probe] endedBeforeTests stamp has no producer-side negative witness (notRun>0 conjunct unpinned)packages/cli/src/commands/review/compose-review.ts:1877 — [review] comment parenthetical "(both predicates are exact now)" is false for the containment splicepackages/cli/src/commands/review/compose-review.ts:1296 — [probe] splice/decision asymmetry: exact-text anchor decision unpinned (containment mutant ships green)packages/cli/src/commands/review/lib/budget.ts:907 — [probe] paren unwrap never re-runs stripWrappers on inner text — (**无**) survives while bare twin dropspackages/cli/src/commands/review/pr-context.test.ts:1780 — [probe] anchor-strip invariant never exercised with an anchored foreign winner (strip-skip mutant ships green)packages/cli/src/commands/review/build-test.test.ts:3491 — [probe] caveat-side dedup of unattempted retries unpinned (note-side only)packages/cli/src/commands/review/pr-context.ts:1122 — [probe] deletion branch reads only noOwnReview, never consults identityKnown at its destructive arm
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 5：none — no check was cut short.。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D7-1 author-less merged provenance contradiction (preamble keys on author, PARTIAL note keys on merged) — already recorded in the round-3 deferred list (review 4954497805)
- D7-2 endedBeforeTests stamp writer-side negative witness missing — already recorded in the round-6 deferred list (review 4960699192)
- D7-3 caveat-side provisionalOnly dedup filter unpinned — already recorded in the round-3 deferred list (review 4954497805) and the round-6 deferred list (review 4960699192)
- D7-4 buildOnly/endedBeforeTests interface declarations hunk-survived (ride on typecheck alone) — already recorded in the round-4 deferred list (review 4955731398)
- D7-5 first build-test fence --plan/--out pins asymmetric (deletion ships green) — already recorded in the round-5 deferred list (review 4957341417)
- D7-9 paren unwrap never re-strips inner markdown wrappers (（无） phantom) — already recorded in the round-6 deferred list (review 4960699192)
- D7-10 deletion licence's !recoveryThrew conjunct unpinned (deletion mutant survives all 112 tests) — already recorded in the round-4 deferred list (review 4955731398)
- D7-12 merged-shape anchor-strip invariant unpinned (strip-skip mutants ship green) — already recorded in the round-6 deferred list (review 4960699192)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — ended at the 5-round cap without two consecutive dry rounds (rounds 1–2, 3 and 5 reported; round 4 dry).
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/budget.ts:914 — [probe] empty-content guard reads wrapped text while the classifier reads unwrapped — （）/() disclosed, bare twins droppedpackages/cli/src/commands/review/pr-context.ts:1307 — [probe] merged preamble promises per-entry provenance no rendered row can signal (same-round union collapses the id-prefix hint)packages/cli/src/commands/review/pr-context.ts:892 — [probe] merged PARTIAL note's "not attributable to any single round's marker" clause is false when the own marker is the sole loss source
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 8 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — ended at the 5-round cap without two consecutive dry rounds (rounds 1–2, 3 and 5 reported; round 4 dry)。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #9342 (base-conflict resolution round)This round was invoked with Conflict resolution
The merged signature is Feedback dispositions
No inline comments existed this round, so there are no thread resolutions VerificationCommands actually run this round, in order:
中文说明轮次总结 — PR #9342（base 冲突解决轮）本轮以 冲突解决
合并后的签名为 反馈处置
本轮不存在 inline 评论，因此没有线程解决记录（省略 验证本轮实际执行的命令（按顺序）：
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — ended at the 5-round cap without two consecutive dry rounds (rounds 1–2 dry, rounds 3 and 4 reported findings, round 5 dry).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 1: executing packages/cli/src/commands/review/agent-prompt.test.ts and build-test.test.ts under vitest (no node_modules in worktree; correctness established by….
Test Plan (not a blocker): lib/budget.test.ts — no such file or directory.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/npm-toolchain.ts:1088 — [probe] endedBeforeTests write-side negative unwitnessed (notRun>0 conjunct unpinned)packages/cli/src/commands/review/compose-review.ts:1693 — [probe] exact-text anchor decision unpinned against prefix-reshaped relayspackages/cli/src/commands/review/pr-context.ts:1748 — [probe] deletion licence's reviews.length>0 conjunct has no negative witnesspackages/cli/src/commands/review/pr-context.ts:1373 — [probe] merged provenance sentence promises a per-entry split the rendering cannot signal
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — ended at the 5-round cap without two consecutive dry rounds (rounds 1–2 dry, rounds 3 and 4 reported findings, round 5 dry)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 1：executing packages/cli/src/commands/review/agent-prompt.test.ts and build-test.test.ts under vitest (no node_modules in worktree; correctness established by…。
Test Plan（非阻断）：lib/budget.test.ts — no such file or directory。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review-address round — no actionNothing actionable arrived in this window. The working tree is unchanged, no commit was created, and the branch stays at the previously pushed head. Triage of this round's feedback
Verification
中文说明Autofix 评审处理轮次 — 无操作本窗口内没有可处理的内容。工作区未做任何改动，未创建提交，分支保持在先前已推送的提交上。 本轮反馈的分诊结果
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定：
|
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. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved. The R1-1 Critical (nothing-to-resume false-completion) is addressed at the head: the endedBeforeTests structural stamp and the buildOnly stamp on the zero-affected early return are both present, matching the suggested fix. CI green on the head. The remaining open threads are Suggestion-level or fixed-but-unresolved hygiene; non-blocking.
|
Released in v0.21.14. |
…rd proved Round 2's six, all taken. The fallback JOB compared review commit ids against the PR's head at fallback time, not the head the run reviewed. On every trigger but pull_request_target the head-moved guard above deliberately does not run, so a push landing between the post and this step leaves that value pointing at bytes no review ever covered: the match fails and the contradictory comment posts anyway — the #9342 shape, re-opened for the trigger + post + push + fail-after-post interleaving. `review-pr` now publishes the head its review step recorded as a job output, and the guard reads it, falling back to the fresh head only when the job died before that step (a run that posted nothing either). The in-job twin needs none of this — its unconditional head-moved check exits first — and that asymmetry is now pinned per site rather than left to be rediscovered. Both skip messages claimed "this run already posted a review". Reviews carry no run id, so the window (bot account + head + submitted at or after this run was created) also matches an overlapping sibling run's review, which this workflow's own concurrency note says can happen. The suppression is right either way — a review IS sitting above the comment — but the oncall reading the summary was told something the guard never proved; both now say what it did. The guard's opening paragraphs still described the round-1 `startedAt` anchor while the code (and the paragraph below it, and the runtime warning) said creation. A maintainer reading top-down got the anchor that re-runs break — the defect round 1 removed. Test stub: `gh run view` now answers by running the caller's own --jq over an object carrying both timestamps, instead of a `case` on "$*" that matched substrings in order. A combined `--json createdAt,startedAt --jq '.startedAt'` was answered from the createdAt branch, leaving the re-run pin green for a guard reading the attempt-scoped field — the exact regression it exists to catch.
…osted (QwenLM#9462) * fix(ci): stop the fallback comment from denying a review it already posted The review job can fail AFTER posting its review — the CLI exiting silently, a cleanup step dying — and both fallback sites then announce that review as one that could not be posted, retry instruction attached. Measured on PR QwenLM#9342: the review posted at 11:56:34Z, review-pr failed at 12:00:53Z ("Qwen review completed but produced no output"), and the comment landed at 12:01:00Z saying the pipeline "failed before a review could be posted. … retry with @qwen-code /review" — a fresh ~3-hour review, asked for beside the review that had just landed. The autofix takeover loop reads the same feed a human does. Both sites now check, before composing a body, whether a review this run posted is already on the PR. The check is scoped three ways so a stale review can never buy silence on a genuinely dead pipeline: the bot's own account, the head this run reviewed, and a submission at or after this run started. Where the proof is unavailable — no start time, no head, a failed listing — the guard declines to fire and the comment posts, the same call the head-moved guard already makes. The job-level step now reads state and headRefOid in one `gh pr view` (the in-job step already did), which is where its head value comes from. Tests run the steps' real bash over review fixtures, because the guard IS a filter: silence when this run posted the review, and posting for each near-miss on its own — an earlier run's review at the same head, another account's, one of a different head, a PENDING one, none at all, an unavailable start time, and a failed reviews listing. One existing assertion tightened: "no `gh run view`" was the proxy for "no head comparison on comment runs", and the new guard asks that same command for startedAt on every event, so it now pins the head lookups themselves. The stub's state,headRefOid branch learned the pr_closed scenario its state-only sibling already knew. * fix(ci): anchor the already-posted guard on the run's creation, and say when it cannot run Round 1's two blockers, both re-verified against this repo's own run data. The time anchor reset on job re-runs. `gh run view --json startedAt` returns the LATEST attempt's start while the run id stays the same — the dedup above relies on that stability — so a re-run pushed attempt 1's review outside "this run": runs 32219268680 (created 05:23:57Z, startedAt 05:51:26Z) and 32218596441 (05:13:04Z → 05:22:05Z) both show the ~9-28 minute shift. Attempt 1 posts its review, the job fails after the post, someone re-runs it, attempt 2 fails before posting — and the guard, anchored on attempt 2's start, lets the contradictory comment through. Exactly the shape this PR exists to stop, on the path most likely to reach it. Both sites now anchor on `createdAt`, which is attempt-stable; a review submitted after the run was created still cannot belong to an earlier run, so the stale-review protection is unchanged. The guard also swallowed its own lookup failures. A transient failure in either call emptied the value, the guard declined, and the false comment posted with nothing in the log separating "the guard ran, nothing matched" from "the lookup died" — while every sibling lookup in these steps announces its failures. Both unavailable paths now emit a `::warning::` and a step-summary line before posting. No behavior change: posting was, and remains, the fail-open direction. Tests: a re-run fixture per site, where the stub answers `createdAt` and `startedAt` with DIFFERENT values and attempt 1's review sits between them — reverting either site to `startedAt` fails exactly these two; and a per-site assertion that both unavailable paths announce themselves. Also from round 1, both verified before taking: the stub's standalone `*state*)` branch is dead (no `--json state` call remains in either extracted step) and is removed, so its scenarios cannot be edited into a no-op; and the harness now substitutes `${{ vars.* }}` before running the in-job script, which bash rejected as a bad substitution — the assignment was skipped, `MAX_TIMEOUT_MINUTES` stayed unset, and eight error lines rode every suite run, so "the step's real bash" was not quite true for that line. * fix(ci): read the head this run reviewed, and claim only what the guard proved Round 2's six, all taken. The fallback JOB compared review commit ids against the PR's head at fallback time, not the head the run reviewed. On every trigger but pull_request_target the head-moved guard above deliberately does not run, so a push landing between the post and this step leaves that value pointing at bytes no review ever covered: the match fails and the contradictory comment posts anyway — the QwenLM#9342 shape, re-opened for the trigger + post + push + fail-after-post interleaving. `review-pr` now publishes the head its review step recorded as a job output, and the guard reads it, falling back to the fresh head only when the job died before that step (a run that posted nothing either). The in-job twin needs none of this — its unconditional head-moved check exits first — and that asymmetry is now pinned per site rather than left to be rediscovered. Both skip messages claimed "this run already posted a review". Reviews carry no run id, so the window (bot account + head + submitted at or after this run was created) also matches an overlapping sibling run's review, which this workflow's own concurrency note says can happen. The suppression is right either way — a review IS sitting above the comment — but the oncall reading the summary was told something the guard never proved; both now say what it did. The guard's opening paragraphs still described the round-1 `startedAt` anchor while the code (and the paragraph below it, and the runtime warning) said creation. A maintainer reading top-down got the anchor that re-runs break — the defect round 1 removed. Test stub: `gh run view` now answers by running the caller's own --jq over an object carrying both timestamps, instead of a `case` on "$*" that matched substrings in order. A combined `--json createdAt,startedAt --jq '.startedAt'` was answered from the createdAt branch, leaving the re-run pin green for a guard reading the attempt-scoped field — the exact regression it exists to catch. * fix(ci): attribute the guard by time alone — the head is not a stable run attribute Round 3's blocker, and the second time the head clause re-opened the contradiction this PR exists to close. Two entrances this round, both after a "Re-run failed jobs": attempt 2 dies before the review step writes its head, so the guard falls back to a head attempt 1 never reviewed; or a push lands and attempt 2 records the NEW head — in both, attempt 1's own review no longer matches `.commit_id`, and the fallback posts "failed before a review could be posted … retry" beneath the review the same run had posted. Rather than patch the head lookup a third time, the head clause is gone. What the guard proves is now narrower and stable: a bot review of this PR was submitted while this run was alive — bot account plus the attempt-stable `createdAt` window. That closes both entrances at once and takes the round-2 cross-job wiring with it (review-pr's `expected_head_sha` output and the env line that read it), so there is no untested chain left whose silent breakage would restore the fresh-head comparison. The job-level step no longer needs the PR head either and reverts to its state-only query; the test stub's state-only branch, removed in round 1 as dead, has a caller again. The comment blocks now state the guarantee the concurrency model actually supports. They claimed a review inside the window "cannot belong to an earlier run", but per-run concurrency groups deliberately allow overlapping runs on the same head, so an earlier-created run's review can match and this run's failure then goes unannounced. That is accepted, and said plainly: the silence coincides with a bot review a reader can see — the very state that makes the comment's claim false — while the bot-author and creation-time clauses still rule out silence with no review at all. Tests: the moved-head case flips from "posts" to "silences" and is pinned per site (a review on ANY head inside the window silences); re-introducing a head clause fails exactly that test; and a structural pin asserts the wiring is absent rather than merely unused. * test(ci): skip the guard's jq-driven cases where jq is absent, instead of failing them The stub answers the guard's reviews and run-view lookups by running the caller's own `--jq` filter — that filter IS the thing under test — so those cases need jq on PATH. A reviewer running the suite on Windows without jq saw them as failures of the guard rather than as untested, which is the wrong signal in the wrong direction. Probed once per run and skipped honestly. Measured with a jq that exits 127: the file goes from 31 failures to 26 failures plus 13 skips — the 26 are the retry-loop cases, which have parsed the review log with jq since long before this change and are equally untestable without it. GitHub's windows-latest image ships jq, so CI coverage is unchanged either way; what changes is what a jq-less machine reports. * docs(ci): remove the head-keyed leftovers the guard no longer has Round 5's four, all leftovers of the round-3 design change rather than new behavior. The job-level block still explained why it compared against the head this run reviewed — naming `pr_head`, "the reviewed head's review" and a `review-pr` job output, none of which survive: the shipped filter is author scope plus the creation-time window, and the wiring was deleted with the head clause. A maintainer reading it would look for a comparison that is not there. The in-job block stated the createdAt-not-startedAt rationale twice, once with the measured run ids and once without; the measured one stays. Same in the tests: the stub's comment listed a head clause the filter deliberately does not have (`attributes by TIME, not by head` is the test that pins its absence), and the harness still declared and injected `reviewedHead`/`REVIEWED_HEAD_SHA`, which nothing reads since the wiring went — a knob that looks live and cannot be. * docs(ci): drop the duplicated anchor rationale and the last stale-head leftovers Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): exclude foreign same-account approvals from the already-posted guard * fix(ci): attribute the already-posted guard by composed-review markers The foreign-approval exclusion list shipped incomplete: the triage skill's commit-pinned APPROVE body also posts under the same account, matches the guard's author and window clauses, and silenced the fallback for a genuinely dead run — the failure shape this guard exists to stop. The producer set is open, so no exclusion list can be finished; every miss fails in the dangerous direction. Match positively instead: a review silences the fallback only if its body carries what only this pipeline's composed reviews carry — the "via Qwen Code /review" attribution footer or the invisible qwen-review-ledger marker. Every composed body carries at least one (a zero-findings APPROVE included); no foreign approval carries either. A marker that ever changes shape stops the guard firing and the comment posts — the pre-guard status quo, not a masked dead run. --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
5 participants
What this PR does
Clears the deferred-suggestion backlog accumulated across PR #9175's fifteen review rounds — nineteen findings, none Critical, each deferred at the time under the one-blocker-per-round policy and taken now with its pin. Roughly half are behavior fixes (a safety-shaped API, a shared re-execution grammar, provenance-honest rendering, bilingual classifier gaps, honest resume accounting) and half are test-strength work (symbolic boundary pins, fence-scoped prompt assertions, handler-level wiring pins), plus the doc corrections the fixes make true.
Behavior changes:
persistRecoveredLedgertakes named flags — two adjacent positional booleans encoded "deletion requires proven identity" and a swapped call inverted it silently (R14-3, with R14-1/R14-2's doc corrections);test-deltaimportsTEST_COMMAND_REfrom beside the emitter instead of keeping a byte-identical copy, so the two shell-re-execution gates cannot drift (R13-3); the deferred-suggestions clause clamps its round exactly as the marker stamp does (R13-6);isNonDiffDimensionGapderives its exempt heads fromBRIEFS(readsDiff: falseroles bypublicLabel) instead of hardcoding prose variants (R13-5); the stop-relay splice matches the full canonical entry text rather than the bare phrase, so a free-form line-coverage disclosure that merely mentions the budget reaches the posted body (R12-6);recoverLedgerreportsmerged: truewhen the union fires and the rendered section names the mixed provenance instead of calling the own subset another account's claims, with the PARTIAL note no longer pinning a two-marker dropped sum on one round's size cap (R12-12/R12-14); the ZH budget-gap classifier strips symmetric full-width parens and the fold key strips full-width trailing punctuation (R2-17/R6-26); a resumed run names an unattempted retry once and its caveat carries both the still-to-run and the provisional halves when they coexist (R10-17/R9-50);--resumetells a completed zero-suite run apart from one that ended before its test phase, keyed onokand a new structuralbuildOnlystamp, both gate-validated.Test-strength: symbolic
FOREIGN_ROUND_HEADROOMboundary pins own-base and zero-base (R13-7), a deep-squat id pinning the generalidRound > markerRoundpredicate (R13-8), an at-cap union case proving the re-cap trims foreign entries first (R13-9), the null-login headroom bound the doc promises (R13-10), a missing-test-key fixture for the resume gate (R3-48), fence- and site-scoped agent-prompt assertions for the timeout directive / resume wrapper /--worktree/ continuation paragraph (R11-13), handler-level pins for identity-throw isolation and both deletion-licence conjuncts (R11-17), and a marker-carrying handler test wiring foreign recovery through to the rendered context and the sha-less side file (R1-40).Still deferred, deliberately: R4-4 (class-level failing-file parser redesign — a design task, not a cleanup) and R11-9.
Why it's needed
Each of these was confirmed real during #9175's rounds (most carry the reviewer's own probe and flip check) and deferred only to keep that PR's diff converging. Left alone they are standing costs: the positional-boolean call compiles a silent side-file deletion, the duplicated grammar drifts in the direction that under-measures, the merged ledger renders false provenance for the account's own certified entries, the full-width-paren phantom gap re-creates the #9094 incident in exactly the output language the ZH branch was added for, and the unpinned boundaries (headroom, squat filter, union order, outage bound) let the probes' mutants ship green.
Reviewer Test Plan
How to verify
npm run typecheck(0 errors) andnpx vitest run src/commands/reviewinpackages/cli— 3266 tests across 83 files pass, including the new pins. The high-signal checks:pr-context.test.ts("split trust surface" describe) holds the symbolic headroom boundaries, the at-cap own-first union, and the null-login bound;pr-context-persist.test.tsexercises the named-flags API end to end;build-test.test.tshas the completed-zero-suite vs ended-early split and the both-halves caveat;lib/budget.test.tshas the full-width paren and fold cases;compose-review.test.tshas the free-form-disclosure survival and the BRIEFS cross-pin.Mutants verified killed during development (mutate, observe the named test fail, restore): reversed union concatenation → "own-first is load-bearing";
>=headroom boundary → "implausibly far past our own";me &&-guarded outage bound → "holds the headroom under a NULL login"; disabled anonymous guard → both ANONYMOUS persist tests.Evidence (Before & After)
N/A — no user-visible surface; the observable changes are review-pipeline report prose and rendered context files, pinned by the unit tests above.
Tested on
中文说明
清掉 #9175 十五轮评审沉淀的延后建议积压——十九条，均非 Critical，当时按「每轮只修阻断项」的策略延后，现在逐条落地并配钉住测试。行为修复约占一半：
persistRecoveredLedger改具名参数（相邻布尔换位会静默删除 side 文件）；test-delta与--resume共享同一条命令语法常量；deferred 轮次与标记盖章同钳；isNonDiffDimensionGap从 BRIEFS 派生豁免头；stop-relay splice 改为完整规范文本匹配（自由文本披露不再被吞）；合并账本渲染如实标注混合来源与 dropped 归属；中文分类器补全角括号与全角尾标点；resume 的 note/caveat 对未尝试重试只点名一次、双态并存时两段都披露；--resume用ok与新的buildOnly结构戳区分「跑完但无套件」与「测试阶段前结束」。测试强化另一半：headroom 符号化边界钉、深度抢注、满容量 own-first、空登录界、缺 test 键夹具、fence 作用域提示词断言、handler 级身份隔离与删除许可两半、外部标记全链路接线钉。开发中已做变异验证（并集反序、边界差一、me 守卫、匿名守卫禁用，均恰好击杀对应新测试）。仍延后：R4-4（failing-file 解析器重设计）与 R11-9。