feat(review): content-anchored incremental rounds for the local review-fix loop - #9659
Conversation
…w-fix loop
A local or file-path review at high effort can now skip what it already
reviewed, anchored on CONTENT rather than on a commit.
The reviewed state is a dirty working tree: it has no commit to anchor on, and
`local-diff.ts`'s standing constraint — nothing on the capture path writes to
the index, the worktree, or any ref — rules out snapshot commits and stashes.
So the anchor is the hashed per-file state of exactly what the plan covered,
plus the HEAD the diff was measured against. `git hash-object` without `-w`
computes the blob id git WOULD store and writes nothing.
The identity is `<mode>:<blob>`, not the blob alone: an exec-bit flip or a
file/symlink typechange is its own diff lines, so identical bytes under a
different mode are not an identical change. Symlinks hash their link text at
120000, exactly what `git diff` renders. Whatever cannot be captured
faithfully is UNHASHABLE — which never equals itself, and is therefore
re-reviewed every round rather than silently certified.
Rendering attributes ride in the identity too, from `git check-attr` rather
than a hand-derivation: `.gitattributes` at any level, `.git/info/attributes`,
the commondir's copy in a linked worktree, `core.attributesFile`, and the
config-side `diff.<driver>.binary` that flips a section between readable hunks
and "Binary files differ" with the bytes standing still.
The scope is the same slicing the PR flow uses — the round's own files plus one
import hop — and every refusal falls back to the full capture with its reason
on stderr.
Hardening from review, each mutation-checked:
- The same-model gate is ruled over the provider-qualified identity the runtime
publishes, recorded by the capture itself. `{{model}}` interpolates the bare
model id, so two provider configurations exposing one model name passed each
other's gate.
- The cache is keyed by the SOURCE path, not the flattened token. `safeTarget`
is not injective — `src/foo.ts` and `src_foo.ts` collide — and two different
files were accepting each other's anchor and erasing each other's findings.
- The target stem is capped with a digest suffix. Unbounded, a legal deep path
made every write for that target throw ENAMETOOLONG.
- Either side of a rename keeps its section, so a staged `git mv` no longer
publishes an empty slice and spins the loop until HEAD moves.
- The review's own plumbing is excluded by path segment at any depth, and on
the TRACKED half too, so a round cannot review its own cache and never
converge.
- `check-attr`'s NUL protocol is read raw; the trimming wrapper stole the first
record from any path beginning with whitespace, and failed open on exactly
the driver lookup the identity exists for.
- The mid-capture tree guard samples three interleaved states, so a
phase-aligned write can no longer certify bytes no round reviewed.
- The decided stops carry a machine-readable `nothingToReview`, so
`qwen review run` stops reporting "Review did not complete" over a round that
decided something.
Replaces #9190, which cannot be merged or repaired: its base is the head of the
closed #9188, GitHub counts it as a stack, and stack validation fails on the
closed root while base changes are refused for stack members. The review is
there — 20 reviews, 166 inline comments.
8e96ae2 to
dff2b78
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
wenshao
changed the title
|
Re-run after the round-24 fix — two commits since the last reviewed head ( Problem: Unchanged, and still real — the local/file review-fix loop had no incremental support, so every fix round re-captured and re-reviewed the whole dirty tree, exactly where the loop spends its tokens because fix rounds run before anything is pushed. Quantified in the body; A/B-proved by the completed Direction: Aligned — core Template: Still narrative prose without the template headings. Maintainer-authored and the substance is thorough, so this stays a hygiene note rather than a gate — but please restore the structure before merge, especially a Reviewer Test Plan. Size: 32 files — 2,718 production-logic lines in Approach: The delta is exactly one focused fix plus one merge. The fix answers the race @qqqys flagged — the PR stop path writes the verdict sidecar and runs cleanup in the same breath, the parent's first poll is up to 250 ms away, so a decided up-to-date/empty-diff round could still exit 1 "Review did not complete". The fix makes cleanup spare a sidecar whose Risk: No elevated-revert-path signals — everything stays inside Moving on to code review. 🔍 中文说明第 24 轮修复后的 re-run——距上次受审 head（ 问题： 不变且真实——本地/单文件审查-修改循环没有增量能力，每一轮都重新捕获并重审整棵脏工作树，而修复轮跑在推送之前，正是循环烧 token 的地方。正文有量化；线程中已完成的 方向： 对齐—— 模板： 仍是无模板标题的叙事正文。维护者作者、内容详尽，作为卫生提示而非门控——合并前请恢复模板结构，尤其是 Reviewer Test Plan。 规模： 32 个文件—— 方案： 增量恰为一个聚焦修复加一次合并。修复回应 @qqqys 指出的竞态——PR 停止路径写下裁决 sidecar 后立刻运行 cleanup，而父进程首次轮询最长在 250 ms 之后，已判定的 up-to-date/empty-diff 轮次可能以 1 退出"Review did not complete"。修复让 cleanup 豁免 runId 与父进程戳记匹配的 sidecar，关闭后回退读得到裁决；外来或未戳记的照常清扫，下一轮 cleanup 收走保留的文件。两个方向均有测试钉住，技能行文同步更新。五个共享文件的合并解决（两个 风险： 无升级的回撤路径信号——改动全部位于 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review (at The sidecar-spare fix ( The merge of Non-blocking carry-overs: the body still lacks the template headings (restore before merge); Testing evidence. Unattended CI run — I did not build or execute the PR's code; the signal below is the PR's own CI on the reviewed commit, read via the API. It is green across the board, including
Sandboxed verification is already in flight on this exact head (run 33081172939, triggered by the same comment): it would settle whether the delta — the sidecar-spare fix plus the main merge — survives the A/B harnesses on THIS commit. The race itself was reproduced deterministically before the fix and re-verified after under human review, and the prior sandbox round already proved the headline incremental claim at the prior head — so this is belt-and-braces on the delta, not an open question on the feature. The human reviewer also verified locally on this head — full build under Node 22 with the relevant CLI and core suites green; that is @qqqys's report from the approval, recorded here with attribution, not independently re-run. 中文说明代码审查（位于 sidecar 豁免修复（ 合入 main（ 非阻塞遗留：正文仍缺模板标题（合并前恢复）； 测试证据。 无人值守 CI 运行——未构建或执行 PR 代码；下表是被审 commit 上 PR 自身 CI 经 API 读取的信号。全线绿色，包括本改动所在的 沙箱验证已在同一 head 上运行中（run 33081172939，由同一条评论触发）：它将验证增量——sidecar 豁免修复加 main 合并——能否通过该 commit 上的 A/B 测试台。竞态本身在修复前已被确定性复现、修复后在人工 review 下复核，且上一轮沙箱已在先前 head 上实证了增量头条主张——故此为增量上的双保险，而非功能上的悬而未决。人工评审者也在该 head 上做了本地验证——Node 22 下完整 build、相关 CLI 与 core 测试通过；那是 @qqqys 在 approve 中的报告，此处注明出处记录，并非独立复跑。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean across every stage on this head; what keeps it from a 5 is the backlog of hygiene notes and the road here, not doubt about where it landed. Stepping back: my independent answer to the delta's problem — "a decided stop can be lost because cleanup sweeps the sidecar before the parent's first poll" — is exactly what shipped: spare the artifact at the cleanup boundary, keyed by the run nonce the parent stamped, and let the already-nonce-fenced post-close read pick it up. The deeper alternative (a per-run nonce in the sidecar's NAME) is noted in The standing notes, carried forward:
Approving on the reviewed commit. This is one of the two approvals 中文说明置信度：4/5 —— 该 head 上每个阶段都干净；没到 5 分是卫生事项与一路走来体量的积累，而非对落点有疑虑。 退一步看：我对增量问题的独立方案——"已判定的停止可能丢失，因为 cleanup 在父进程首次轮询前扫掉了 sidecar"——正是本修复所交付的：在清扫边界上按父进程戳记的 run nonce 豁免该工件，让本就带 nonce 围栏的关闭后读取把它接住。更深的替代（在 sidecar 文件名里引入按轮 nonce）在 遗留事项，继续记录：
在被审 commit 上批准。这是 — 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. |
8c1330a to
dff2b78
Compare
wenshao
changed the title
| const cachePath = promoteCandidate(capture(), 'model-a'); | ||
|
|
||
| // Only the CONFIG changes: no file in the tree moves, and `check-attr` | ||
| //答案 is identical before and after. |
There was a problem hiding this comment.
[Suggestion] Garbled comment: a stray Chinese word 答案 ("answer") replaces the English continuation, leaving the sentence broken — "and check-attr 答案 is identical before and after" instead of "and check-attr's answer is identical". No behavioral impact; the cost is a confusing artifact in a test whose entire purpose is explaining a subtle invariant (the config-side diff.<driver>.binary flip) — a reader following the reasoning hits a foreign-language fragment mid-sentence where the subject of "is identical" should be.
| //答案 is identical before and after. | |
| // 's answer is identical before and after. |
中文说明
[Suggestion] 注释乱码：一个孤立的中文词 答案 取代了英文续句，句子因此断裂——"and check-attr 答案 is identical before and after"，本应是 "and check-attr's answer is identical"。无行为影响；代价是在一个以解释微妙不变量为全部目的的测试里留下令人困惑的痕迹（配置侧 diff.<driver>.binary 翻转）——读者跟随推理时，会在「is identical」的主语位置撞上一段外语。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| cache: cachePath, | ||
| model: 'model-a', | ||
| }); | ||
| expect(other['target']).toBe('src_foo.ts'); |
There was a problem hiding this comment.
[Suggestion] The new source-path gate — the gate this PR adds to close the safeTarget token collision — has its refusal reason asserted nowhere: zero tests pin the stderr sentence the cache belongs to source path …, although every sibling refusal reason (model, HEAD, target, stateId, skipped, TOCTOU) is pinned on stderr, and this file's own header declares stderr part of the interface under test ("SKILL.md branches on specific stderr strings"). The collision test asserts only other.incremental === undefined — an outcome all eight refusal clauses produce identically (verified: in that fixture the model/target/stateId/HEAD/skips/tree gates all pass, so only the source gate can fire — the behavior side IS uniquely pinned; the wording is the unpinned half). A refactor relocating the source check into readLocalCache (fail-quiet return null on mismatch) or rewording/dropping the reason leaves every test green: the refusal surfaces as the cache is missing or unreadable — a false diagnosis for exactly the collision this gate exists to name. Behavior stays safe; the cost is the "every reason said out loud" guarantee for the one gate with no other witness. Add expect(stderrLines.join('\n')).toContain('belongs to source path'); after the other capture.
中文说明
[Suggestion] 新增的 source 路径门——本 PR 为关闭 safeTarget token 碰撞而加的门——其拒绝理由没有任何断言：零测试钉住 stderr 句子 the cache belongs to source path …，尽管每个兄弟拒绝理由（model、HEAD、target、stateId、skipped、TOCTOU）都在 stderr 上被钉住，而且本文件头部自述 stderr 是被测接口的一部分（"SKILL.md branches on specific stderr strings"）。碰撞测试只断言 other.incremental === undefined——八个拒绝条款都产生相同结果（已核实：该 fixture 中 model/target/stateId/HEAD/skips/tree 门全部通过，只有 source 门能触发——行为侧确实被唯一钉住；措辞是没被钉的一半）。把 source 检查挪进 readLocalCache（不匹配时静默 return null）或改写／删除该理由的重构会让所有测试保持绿色：拒绝会以 the cache is missing or unreadable 呈现——恰对该门存在所要命名的碰撞给出错误诊断。行为仍安全；代价是这个唯一没有其他见证的门失去了「每个理由都大声说出来」的保证。建议在 other 捕获后加 expect(stderrLines.join('\n')).toContain('belongs to source path');。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| expect(plan.incremental!.scope!.deltaFiles).toEqual([]); | ||
| expect(plan.incremental!.scope!.interaction.map((e) => e.path)).toContain( |
There was a problem hiding this comment.
[Suggestion] The deleted-importer test pins the scope frame and the stderr sentence but not the publication half: it never asserts chunks.length > 0 or that the sliced diff at plan.diffPath actually contains src/c.ts's section — the only shape in the suite where the entire keep-set comes from interaction (deltaFiles is empty). A regression on the empty-delta path — an early return when changed.length === 0, or keep narrowed to deltaFiles — slices src/c.ts's section away: the plan carries chunks: [] beside an incremental block naming src/c.ts in interaction and no nothingToReview; SKILL's no-diff branch stops before compose, and qwen review run exits 1 "Review did not complete" over a live round. Every test stays green — mutation-verified: an empty-delta fast-path mutant left the full 38-test suite green while the round's plan carried chunks: 0; adding the pins flips it (expected 0 to be greater than 0), and restoring the correct code with pins stays green. The adjacent rename test explicitly pins publication; this one should mirror it.
中文说明
[Suggestion] 被删 importer 测试钉住了范围帧与 stderr 句子，但没钉发布一半：它从未断言 chunks.length > 0，也未断言 plan.diffPath 处的切片 diff 真的包含 src/c.ts 的 section——这是套件中整个 keep 集全部来自 interaction 的唯一形状（deltaFiles 为空）。空 delta 路径上的回归——changed.length === 0 时提前返回，或 keep 收窄为 deltaFiles——会把 src/c.ts 的 section 切掉：计划携带 chunks: []，旁边的 incremental 块在 interaction 里命名 src/c.ts，且无 nothingToReview；SKILL 的无 diff 分支在 compose 之前停止，qwen review run 对一个存活轮次以 exit 1 报「Review did not complete」。所有测试保持绿色——变异实测：空 delta 快速路径变异体让全部 38 个测试保持绿色，而该轮计划携带 chunks: 0；加上这些钉住后翻转（expected 0 to be greater than 0），恢复正确代码后钉住仍绿。相邻的 rename 测试明确钉了发布；本测试应镜像。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Same finding as the round-2 re-post rc:3833436414 — deferred to the next round together with it; see that thread for the specifics.
中文说明
与第 2 轮重发的 rc:3833436414 是同一发现——与它一起延迟到下一轮；具体情况见该线程。
There was a problem hiding this comment.
Deferred to the next round. This round's batch was capped at the six round-3 Criticals, all fixed in 87b769b (comma-driver fold, directory plumbing targets, ignore-rules exemption, directory-target tracked drop, cap-before-parse, file-review prose). R1-3 deleted-importer test still asserts only scope metadata; the chunks/diff-content pin asked for is not added yet. It stays queued for the next round.
中文说明
延迟到下一轮处理。本轮批次上限为六个 round-3 Critical，已在 87b769b 中全部修复（逗号驱动名折叠、目录形 plumbing 目标、ignore 规则豁免、目录目标的 tracked 剔除、限额先于解析、文件评审措辞）。deleted-importer 测试仍只断言 scope 元数据；所要求的 chunks/切片 diff 内容钉住尚未补上。该条保留在队列中，下一轮处理。
| expect(stderrLines.join('\n')).toContain('stateId does not match'); | ||
| }); | ||
|
|
||
| it('refusal reasons for model/HEAD/malformed gates reach stderr verbatim', () => { |
There was a problem hiding this comment.
[Suggestion] This test's title promises the malformed gate's stderr reason is pinned verbatim, but the body only asserts the model and HEAD reasons; the malformed/unreadable-cache reason ('the cache is missing or unreadable', produced by anchorRefusalReason for a missing/non-JSON cache and emitted through the single writeStderrLine) is asserted by no test anywhere in the package. A regression that rewords or drops that branch (or the whole !cache branch, if readLocalCache ever returns a shape-valid empty object instead of null) ships with the suite green: the degradation test ('a malformed cache … degrade to the full capture') only checks incremental === undefined, and this test never feeds a malformed cache. The file's own header makes stderr part of the interface under test ("every reason is said out loud"), so the lost reason silently degrades the refusal contract's observability. Add a third case — write 'not json' into the cache path, capture, and expect stderr to contain 'the cache is missing or unreadable' — or drop "malformed" from the test name.
中文说明
[Suggestion] 该测试标题承诺逐字钉住 malformed 门的 stderr 理由，但主体只断言了 model 与 HEAD 理由；malformed／不可读缓存的理由（'the cache is missing or unreadable'，由 anchorRefusalReason 在缓存缺失／非 JSON 时产生、经唯一的 writeStderrLine 发出）在整个包内没有任何测试断言。改写或删除该分支（或者当 readLocalCache 某日返回形状合法的空对象而非 null 时删掉整个 !cache 分支）的回归会在套件绿色下发布：降级测试（'a malformed cache … degrade to the full capture'）只检查 incremental === undefined，本测试又从不喂入 malformed 缓存。文件头部自述 stderr 是被测接口的一部分（"every reason is said out loud"），丢失的理由会静默削弱拒绝契约的可观测性。建议加第三个用例——向缓存路径写入 'not json'、捕获、断言 stderr 含 'the cache is missing or unreadable'——或从测试名中去掉 "malformed"。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| { diff: DIFF_A }, | ||
| { diff: Buffer.from('changed mid-hash\n') }, | ||
| ); | ||
| run({ model: 'model-a', cache: cachePath }); |
There was a problem hiding this comment.
[Suggestion] This test's stated isolation — "the anchor is otherwise valid and the refusal can only come from the guard" — is not achieved: model in argv is inert (CaptureLocalArgs has no model field; the round identity is read only from QWEN_CODE_MODEL_IDENTITY via roundModelIdFrom), so round 2's running identity is '' and the model gate (certifierMatchesRound('model-a', '')) would refuse the anchor even if the tree held still. Mutation-verified: moving the treeHeldStill clause below the model clause fails the test with the MODEL-gate message ("reviewed by model-a, not …") — the refusal comes from the model gate, proving the anchor is not otherwise valid and the test passes only because treeHeldStill is the first clause in anchorRefusalReason. If the clauses are ever reordered, this test fails although the guard still works — and the obvious "fix" (updating the expected substring to the model-mismatch message) leaves a test named for the guard that no longer exercises it. Set/restore process.env['QWEN_CODE_MODEL_IDENTITY'] = 'model-a' around run() (as the sibling suite's capture() does) and drop the dead model keys.
中文说明
[Suggestion] 该测试声明的隔离性——「锚点在其他方面有效，拒绝只能来自守卫」——并未成立：argv 里的 model 是惰性的（CaptureLocalArgs 没有 model 字段；轮次身份只经 roundModelIdFrom 从 QWEN_CODE_MODEL_IDENTITY 读取），所以第 2 轮的运行身份是 ''，即使树保持静止，模型门（certifierMatchesRound('model-a', '')）也会拒绝锚点。变异实测：把 treeHeldStill 条款移到模型条款之下，测试以模型门消息（"reviewed by model-a, not …"）失败——拒绝来自模型门，证明锚点在其他方面并不有效，测试通过仅仅因为 treeHeldStill 是 anchorRefusalReason 的第一条款。若条款顺序被重排，守卫仍工作时本测试却会失败——而显眼的「修复」（把期望子串改成模型不匹配消息）会留下一个以守卫命名却不再检验守卫的测试。建议在 run() 前后设置／恢复 process.env['QWEN_CODE_MODEL_IDENTITY'] = 'model-a'（如兄弟套件 capture() 所做），并删掉惰性的 model 键。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Same finding as the round-2 re-post rc:3833436424 — deferred to the next round together with it; see that thread for the specifics.
中文说明
与第 2 轮重发的 rc:3833436424 是同一发现——与它一起延迟到下一轮；具体情况见该线程。
There was a problem hiding this comment.
Deferred to the next round. This round's batch was capped at the six round-3 Criticals, all fixed in 87b769b (comma-driver fold, directory plumbing targets, ignore-rules exemption, directory-target tracked drop, cap-before-parse, file-review prose). R1-5 the toctou suite's unused model argument remains. It stays queued for the next round.
中文说明
延迟到下一轮处理。本轮批次上限为六个 round-3 Critical，已在 87b769b 中全部修复（逗号驱动名折叠、目录形 plumbing 目标、ignore 规则豁免、目录目标的 tracked 剔除、限额先于解析、文件评审措辞）。R1-5：toctou 套件中未使用的 model 参数仍在。该条保留在队列中，下一轮处理。
| // no verdict. The command derives it now, from `--file`. | ||
| const body = skillBody(); | ||
| expect(body).not.toContain("put through the CLI's own normalization"); | ||
| expect(body).toContain('**Do not pass `--target` for a file review'); |
There was a problem hiding this comment.
[Suggestion] The new local incremental contract added to SKILL.md is unpinned, in the same file whose existing comment documents that prompt-level gates silently drifted on main once and are pinned for exactly that reason. The large new Step-1 bullets — pass --cache .qwen/review-cache as a DIRECTORY, do not pass --model, read the nothingToReview FIELD not the stderr sentence — have no assertion; a future wording rewrite (e.g. restoring a hand-computed cache filename or --model {{model}}) turns no test red, and rounds silently lose both incremental scoping and the findings ledger — the two failures this PR exists to end (the directory-form and bare-model regressions are exercised in capture-local.incremental.test.ts only through the exact strings the skill must emit). Pin the contract strings beside the existing gate tests — e.g. expect(body).toContain('--cache .qwen/review-cache') and expect(body).toContain('nothingToReview: { reason:') (both verified present in the current SKILL.md).
中文说明
[Suggestion] SKILL.md 新增的本地增量契约没有被钉住，而同一文件的既有注释记载了 prompt 级门曾在 main 上静默漂移、正因此才被钉住。新的大段 Step 1 条目——以目录形式传 --cache .qwen/review-cache、不传 --model、读 nothingToReview 字段而非 stderr 句子——没有任何断言；未来的措辞重写（例如恢复手工计算的缓存文件名或 --model {{model}}）不会让任何测试变红，轮次会静默同时失去增量切片与 findings ledger——正是本 PR 存在所要终结的两个失败（目录形态与裸模型回归在 capture-local.incremental.test.ts 中仅通过 skill 必须发出的精确字符串被演练）。建议在既有门测试旁钉住契约字符串——例如 expect(body).toContain('--cache .qwen/review-cache') 与 expect(body).toContain('nothingToReview: { reason:')（两者均已核实在当前 SKILL.md 中存在）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Round 1 on this PR found nine, and the majority are regressions the
previous round's fixes introduced. Taken in that order:
**The PR cache lost its only writer.** The last round removed
`lastModelId: "{{model}}"` from Step 8's template because the LOCAL
capture had started recording the identity itself — but the PR flow has
no such writer in this PR (that producer is the follow-up's), so every
PR cache written after it carried no certifier, Step 1 omitted
`--since-model`, and `fetch-pr` refused every anchor as
`cross-model-anchor` for ever. The template line is back; the
"do not hand-carry a model" instruction stays where it belongs, on the
local bullet whose command does derive one.
**Two stops contradicted each other.** The `clean-tree` decided stop was
written without consulting `treeHeldStill`, so a capture whose own guard
had just proved the tree moved mid-capture still ended the round as
"nothing to review": stderr printed both lines back to back and the
just-written change went unreviewed while the run recorded clean. It is
gated now, like the skipped-content case beside it.
**A third decided shape had no stop at all.** A cached path that vanished
is a change by design, so the unchanged-since stop cannot fire, and the
clean-tree stop is gated on `!incremental` — leaving `chunks: []` with an
`incremental` block and no field: `agent-prompt --roster` threw and the
parent reported "Review did not complete". That shape is
`scope-emptied`, and only when no more specific stop already fired — the
first cut of this overwrote `unchanged-since-last-round`, which the
existing test caught.
**The attribute probe was buffer-bound.** `gitWithInputRaw` inherited
`execFileSync`'s 1 MB default while `check-attr --stdin -z` emits ~3
records per path — ~1.16 MB at this repo's file count. Past it the call
threw, the blanket catch answered an empty map, every identity became
UNHASHABLE, and since UNHASHABLE never equals itself the whole target
was silently re-reviewed every round with a stable stateId and no
refusal. Same 512 MiB ceiling `gitRaw` takes.
**The parent polled a name the skill lets the model choose.** The stop
was published inside the plan, whose `--out` is the orchestrator's to
pick — necessarily, since the target token does not exist at Step 1 — so
every file review's decided stop was invisible to `qwen review run`. It
is a sidecar now, named from the same target the parent derives.
**A decided stop passed `--fail-on request-changes`.** Both stop
branches open by rendering the cache's still-open findings, and the
common shape is a user who committed without fixing a Critical. Reported
with no verdict, the gate returned 0 over a blocker the round itself
called standing — passing the moment the author stopped touching the
tree. The sidecar carries the ledger's open-blocker count and the parent
maps a non-zero one to `REQUEST_CHANGES`.
**The ledger reads named a token that does not exist yet.** Step 1's
reads and Step 8's write all spelled `<target>.json` before the command
that derives `target` had run, and `safeTarget` is not hand-reproducible
past 64 characters or through a symlink. The capture publishes the
resolved `cachePath` and every reader takes it from there.
**Step 8's local paragraph pointed at a rule that cannot apply.** It
extended "the same fail-closed rule as the PR cache" to local rounds,
but that rule keys on a `sha` inside a posted review's marker, and a
local round posts nothing — read literally it skipped the cache write on
every round. The local conditions are stated in full instead.
**The cache key did not discriminate the subject.** The anchor gate's
`source` check is the second layer, not the first: it can only refuse a
cache the round already opened, leaving the ledger — read and written by
the orchestrator — sharing one file. `src/foo.ts` and `src_foo.ts`
erased each other's findings, a root file named `local` produced the
whole-tree key byte for byte, and one named `pr-<n>` produced PR <n>'s.
File reviews get their own namespace and a digest of the source path.
Safe to respell because nothing predicts the name any more.
Every fix is mutation-checked, and the ordering bug in the third one is
the reason: the fix that closes a finding is exactly as capable of
opening one.
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.
Not explored to full depth (tool budget reached): chunk 1: executing capture-local.incremental.test.ts to confirm the assertions pass (review worktree has no node_modules/dist; a full npm ci + build was out of reach for….
[Critical] R1-34 (still stands, mutated, from round 1): the Step 8 PR-cache template writes "lastModelId": "{{model}}" (re-added by the round-2 commit at SKILL.md:1354), but {{model}} interpolates the BARE model id while fetch-pr's same-model gate compares whole-string against the provider-qualified running identity (@, published as QWEN_CODE_MODEL_IDENTITY whenever authType/baseUrl is set). Every PR cache written from this template can never pass the gate: round N+1's Step 1 passes --since-model , fetch-pr refuses cross-model-anchor, and the round reviews the full diff; the cache is rewritten bare every round so it never heals. Cache-path incremental scoping and the upToDate 'No new changes' stop are dead for every PR; unposted rounds ('a run that does not post has only this cache' — Step 8's own words) lose incremental scoping entirely; the marker-recovery path mitigates posted rounds only. Round-1 blocker R1-34 explicitly ruled out restoring {{model}} as-is for exactly this reason. Witness (probe over the real gate): running identity qwen3-max@aaaaaaaa; certifierMatchesRound(bare, qualified) = false → cross-model-anchor refusal; qualified self-match = true. Fix: write the CLI-published qualified identity into the PR cache (the fetch-pr report already publishes it as reviewModelId), or land the retirement of the PR-cache anchor path whole. Unanchorable inline: the template line is net-unchanged against the base (deleted in round 1, re-added in round 2), so it sits in no hunk of the PR diff.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 1：executing capture-local.incremental.test.ts to confirm the assertions pass (review worktree has no node_modules/dist; a full npm ci + build was out of reach for…。
[Critical] R1-34 (still stands, mutated, from round 1): the Step 8 PR-cache template writes "lastModelId": "{{model}}" (re-added by the round-2 commit at SKILL.md:1354), but {{model}} interpolates the BARE model id while fetch-pr's same-model gate compares whole-string against the provider-qualified running identity (@, published as QWEN_CODE_MODEL_IDENTITY whenever authType/baseUrl is set). Every PR cache written from this template can never pass the gate: round N+1's Step 1 passes --since-model , fetch-pr refuses cross-model-anchor, and the round reviews the full diff; the cache is rewritten bare every round so it never heals. Cache-path incremental scoping and the upToDate 'No new changes' stop are dead for every PR; unposted rounds ('a run that does not post has only this cache' — Step 8's own words) lose incremental scoping entirely; the marker-recovery path mitigates posted rounds only. Round-1 blocker R1-34 explicitly ruled out restoring {{model}} as-is for exactly this reason. Witness (probe over the real gate): running identity qwen3-max@aaaaaaaa; certifierMatchesRound(bare, qualified) = false → cross-model-anchor refusal; qualified self-match = true. Fix: write the CLI-published qualified identity into the PR cache (the fetch-pr report already publishes it as reviewModelId), or land the retirement of the PR-cache anchor path whole. Unanchorable inline: the template line is net-unchanged against the base (deleted in round 1, re-added in round 2), so it sits in no hunk of the PR diff.
— qwen3.8-max via Qwen Code /review (v0.21.15)
| @@ -153,6 +191,59 @@ const escapeRe = (s: string): string => | |||
| * Only a per-run nonce in the child's artifact names could key these | |||
There was a problem hiding this comment.
[Suggestion] R1-25: Still stands — the composedNameFor doc block (untouched by the diff) still documents the OLD contract: "the file's basename for a file review", plus the accepted-residual-race bullet "two FILE targets with different paths but one basename (a monorepo's two index.ts) — the child names by basename, so their filenames collide" — directly contradicting the comment this same diff adds in classifyRunTarget ("It used to be the BASENAME"). Witness (probe): safeTarget('src/index.ts') → src_index.ts vs safeTarget('test/index.ts') → test_index.ts — the basename collision class is eliminated; the surviving class is root-level files literally named local/pr-<digits>. A maintainer diagnosing a poll reads the block's "do not diagnose any of these as a pin failure" and either treats distinct pins as the known basename collision or re-derives a basename pin — reintroducing the exact never-matching poll this PR fixes. Fix: rewrite the block for the new contract and narrow the residual list.
中文说明
[Suggestion] 仍然成立 —— composedNameFor 的文档块（diff 未触碰）仍在描述旧契约：「文件评审用文件的 basename」，以及那条「两个路径不同但 basename 相同的 FILE 目标（monorepo 里的两个 index.ts）—— 子进程按 basename 命名，因此文件名同样碰撞」的已接受残留竞争 —— 与同一 diff 在 classifyRunTarget 里新增的注释（「It used to be the BASENAME」）直接矛盾。实测（探针）：safeTarget('src/index.ts') → src_index.ts，safeTarget('test/index.ts') → test_index.ts —— basename 碰撞类已被消灭；残留的只有根目录下字面命名为 local/pr-<digits> 的文件。维护者排查轮询时读到该块的「不要把以上诊断为 pin 失败」，要么把不同的 pin 当作已知 basename 碰撞，要么重新按 basename 推导 pin —— 把本 PR 刚修好的「永不匹配轮询」又引回来。修复：按新契约重写该块，收窄残留列表。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the next round — this round's batch was bounded: it implemented the two still-open Criticals (the PR cache's recorded identity — {{model}} → the fetch report's reviewModelId — and the stale <target>.json naming prose) plus six Suggestions. This finding is acknowledged and queued; no code was changed for it in this round. composedNameFor's docblock is orphaned above nothingToReviewFrom and stale.
中文说明
延迟到下一轮——本轮批次有上限：本轮实现了两个仍未关闭的 Critical（PR 缓存记录的身份——{{model}} → fetch 报告的 reviewModelId——以及过时的 <target>.json 命名文案），另加六条 Suggestion。该发现已确认并排队；本轮未对其改动代码。
There was a problem hiding this comment.
Deferred to the next round. This round's batch was capped at the six round-3 Criticals, all fixed in 87b769b (comma-driver fold, directory plumbing targets, ignore-rules exemption, directory-target tracked drop, cap-before-parse, file-review prose). R1-25 the composedNameFor docblock is still orphaned/stale. It stays queued for the next round.
中文说明
延迟到下一轮处理。本轮批次上限为六个 round-3 Critical，已在 87b769b 中全部修复（逗号驱动名折叠、目录形 plumbing 目标、ignore 规则豁免、目录目标的 tracked 剔除、限额先于解析、文件评审措辞）。R1-25：composedNameFor 的 docblock 仍是孤立/过时状态。该条保留在队列中，下一轮处理。
| function planStemFor(cls: RunTargetClass): string { | ||
| switch (cls.kind) { | ||
| case 'pr': | ||
| return `pr-${cls.number}`; |
There was a problem hiding this comment.
[Suggestion] R1-27: Still stands — planStemFor's pr branch is still dead: no PR-flow artifact named qwen-review-pr-<n>-plan.json ever carries a nothingToReview field, so qwen review run <pr> still exits 1 "Review did not complete" over decided PR rounds; the branch suggests a capability that does not exist. A maintainer reading it concludes PR rounds participate in the stop-sidecar contract and diagnoses a real PR-flow gap as a naming bug. Fix: delete the pr branch (or wire the PR flow to a sidecar and say so), and document that decided-stop handling is local/file-only this round.
中文说明
[Suggestion] 仍然成立 —— planStemFor 的 pr 分支依旧是死代码：没有任何名为 qwen-review-pr-<n>-plan.json 的 PR 流产物会携带 nothingToReview 字段，因此 qwen review run <pr> 对已裁决的 PR 轮次仍会以 exit 1 报「Review did not complete」；该分支暗示了一种不存在的能力。读到的维护者会以为 PR 轮次参与 stop-sidecar 契约，并把真实的 PR 流缺口误诊为命名 bug。修复：删除 pr 分支（或真正把 PR 流接上 sidecar 并说明），并注明本轮裁决早停处理仅限 local/file。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the next round — this round's batch was bounded: it implemented the two still-open Criticals (the PR cache's recorded identity — {{model}} → the fetch report's reviewModelId — and the stale <target>.json naming prose) plus six Suggestions. This finding is acknowledged and queued; no code was changed for it in this round. planStemFor's pr branch is dead.
中文说明
延迟到下一轮——本轮批次有上限：本轮实现了两个仍未关闭的 Critical（PR 缓存记录的身份——{{model}} → fetch 报告的 reviewModelId——以及过时的 <target>.json 命名文案），另加六条 Suggestion。该发现已确认并排队；本轮未对其改动代码。
There was a problem hiding this comment.
Deferred to the next round. This round's batch was capped at the six round-3 Criticals, all fixed in 87b769b (comma-driver fold, directory plumbing targets, ignore-rules exemption, directory-target tracked drop, cap-before-parse, file-review prose). R1-27 the PR-flow stop still does not write the sidecar. It stays queued for the next round.
中文说明
延迟到下一轮处理。本轮批次上限为六个 round-3 Critical，已在 87b769b 中全部修复（逗号驱动名折叠、目录形 plumbing 目标、ignore 规则豁免、目录目标的 tracked 剔除、限额先于解析、文件评审措辞）。R1-27：PR 流程的 stop 仍未写 sidecar 文件。该条保留在队列中，下一轮处理。
| // decided — a cached second round on an unchanged tree, or a clean tree | ||
| // whose earlier blocker the ledger still renders as standing. The signal is | ||
| // a field the CLI wrote into its own plan, not a sentence the model chose. | ||
| const stop = nothingToReviewFrom(targetClass, cutoffMs); |
There was a problem hiding this comment.
[Suggestion] R1-29: Still stands — the stop sidecar's reason is still parsed and validated by nothingToReviewFrom and then discarded: RunReviewResult carries no stop field, human mode prints the literal line Event: null on a clean decided stop, and --json consumers cannot tell which decided stop fired. Witness (probe): a child that writes only the stop sidecar → human-mode stdout exactly ['Event: null\n'], exit 0; the --json result keys carry no stop/reason field. The PR's headline was ending verdict-less reporting for decided rounds; this is the same gap surviving on the prose side. Fix: surface it — stopReason: stop?.reason ?? null on RunReviewResult, and print result.verdictLine ?? (stop ? \Nothing to review: ${stop.reason}` : `Event: ${result.event}`)`.
中文说明
[Suggestion] 仍然成立 —— stop sidecar 的 reason 依旧被 nothingToReviewFrom 解析、校验后丢弃：RunReviewResult 没有 stop 字段，干净的裁决早停在人类模式下打印字面一行 Event: null，--json 消费者无法分辨触发的是哪种早停。实测（探针）：只写 stop sidecar 的子进程 → 人类模式 stdout 恰为 ['Event: null\n']、exit 0；--json 结果的键中没有 stop/reason 字段。本 PR 的招牌目标是终结已裁决轮次的无裁决上报；这是同一缺口在措辞侧的存活。修复：把它显式化 —— 给 RunReviewResult 加 stopReason: stop?.reason ?? null，打印改为 result.verdictLine ?? (stop ? \Nothing to review: ${stop.reason}` : `Event: ${result.event}`)`。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the next round — this round's batch was bounded: it implemented the two still-open Criticals (the PR cache's recorded identity — {{model}} → the fetch report's reviewModelId — and the stale <target>.json naming prose) plus six Suggestions. This finding is acknowledged and queued; no code was changed for it in this round. The stop reason is discarded; the human output should display stopReason.
中文说明
延迟到下一轮——本轮批次有上限：本轮实现了两个仍未关闭的 Critical（PR 缓存记录的身份——{{model}} → fetch 报告的 reviewModelId——以及过时的 <target>.json 命名文案），另加六条 Suggestion。该发现已确认并排队；本轮未对其改动代码。
There was a problem hiding this comment.
Deferred to the next round. This round's batch was capped at the six round-3 Criticals, all fixed in 87b769b (comma-driver fold, directory plumbing targets, ignore-rules exemption, directory-target tracked drop, cap-before-parse, file-review prose). R1-29 Event: null is still displayed and the stop reason is discarded from the result. It stays queued for the next round.
中文说明
延迟到下一轮处理。本轮批次上限为六个 round-3 Critical，已在 87b769b 中全部修复（逗号驱动名折叠、目录形 plumbing 目标、ignore 规则豁免、目录目标的 tracked 剔除、限额先于解析、文件评审措辞）。R1-29：Event: null 仍会被显示，且 stop 原因仍从结果中丢弃。该条保留在队列中，下一轮处理。
| .replace(/[^A-Za-z0-9._-]/g, '_') // separators and anything odd → underscore | ||
| .replace(/\.\.+/g, '_'); // no run of dots survives as a traversal token | ||
| return flat.replace(/^[._]+/, '') || 'target'; | ||
| const stem = flat.replace(/^[._]+/, '') || 'target'; |
There was a problem hiding this comment.
[Suggestion] R1-30: Still stands — this PR lands deep-target truncation (the SAFE_TARGET_MAX cap with a digest suffix, in this very function), but safeTarget's docblock above (unchanged) still asserts the opposite: "Deep-target truncation … stays OUT of this lift: it is a behavior change and lands with the sweep-side fix it needs, not smuggled through a move." A maintainer trusting the docblock assumes slug stability is absolute and writes a prefix-sweep or name-prediction that the digest-suffixed caps break. Fix: rewrite the paragraph to document the cap and its cleanup-sweep consequence.
中文说明
[Suggestion] 仍然成立 —— 本 PR 已落地深目标截断（就在这个函数里的 SAFE_TARGET_MAX 上限 + 摘要后缀），但其上方未改动的 docblock 仍断言相反内容：「深目标截断……不在本次搬迁范围内：它是行为变更，要和它所需的 sweep 侧修复一起落地，而不是借搬迁偷渡」。信任 docblock 的维护者会以为 slug 稳定性是绝对的，进而写出被摘要后缀上限打破的前缀清扫或名字预测。修复：重写该段，写明上限及其对清理清扫的影响。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the next round — this round's batch was bounded: it implemented the two still-open Criticals (the PR cache's recorded identity — {{model}} → the fetch report's reviewModelId — and the stale <target>.json naming prose) plus six Suggestions. This finding is acknowledged and queued; no code was changed for it in this round. safeTarget's docblock is stale.
中文说明
延迟到下一轮——本轮批次有上限：本轮实现了两个仍未关闭的 Critical（PR 缓存记录的身份——{{model}} → fetch 报告的 reviewModelId——以及过时的 <target>.json 命名文案），另加六条 Suggestion。该发现已确认并排队；本轮未对其改动代码。
There was a problem hiding this comment.
Deferred to the next round. This round's batch was capped at the six round-3 Criticals, all fixed in 87b769b (comma-driver fold, directory plumbing targets, ignore-rules exemption, directory-target tracked drop, cap-before-parse, file-review prose). R1-30 the safeTarget docblock still contradicts the truncation behavior. It stays queued for the next round.
中文说明
延迟到下一轮处理。本轮批次上限为六个 round-3 Critical，已在 87b769b 中全部修复（逗号驱动名折叠、目录形 plumbing 目标、ignore 规则豁免、目录目标的 tracked 剔除、限额先于解析、文件评审措辞）。R1-30：safeTarget 的 docblock 仍与截断行为相矛盾。该条保留在队列中，下一轮处理。
| // no verdict. The command derives it now, from `--file`. | ||
| const body = skillBody(); | ||
| expect(body).not.toContain("put through the CLI's own normalization"); | ||
| expect(body).toContain('**Do not pass `--target` for a file review'); |
There was a problem hiding this comment.
[Suggestion] R1-35: Still stands (file unchanged since round 1) — the new local incremental contract added to SKILL.md (the --cache directory form, the two stop branches, the "read cachePath, do not compute the name" discipline) is unpinned, in the same file whose existing comment documents that prompt-level gates silently drifted on main once and are pinned for exactly that reason. A future edit rewording or dropping one of these sentences fails nothing, and the drift surfaces as orchestrator amnesia rounds later. Fix: pin the load-bearing sentences the way the existing gates in this file pin theirs.
中文说明
[Suggestion] 仍然成立（文件自第 1 轮起未改动）—— SKILL.md 新增的本地增量契约（--cache 目录形式、两个早停分支、「读 cachePath、不要计算名字」纪律）没有被钉住；而同一文件里已有的注释明确记载：prompt 级门禁曾在 main 上静默漂移过一次，正因为此才被钉住。未来任何改写或删除这些句子的编辑都不会让测试失败，漂移会在若干轮之后以 orchestrator 失忆的形式浮现。修复：按本文件中既有门禁的钉法钉住这些承重句。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Deferred to the next round — this round's batch was bounded: it implemented the two still-open Criticals (the PR cache's recorded identity — {{model}} → the fetch report's reviewModelId — and the stale <target>.json naming prose) plus six Suggestions. This finding is acknowledged and queued; no code was changed for it in this round. SKILL.test.ts should pin the local incremental contract strings (--cache .qwen/review-cache, the nothingToReview field reads).
中文说明
延迟到下一轮——本轮批次有上限：本轮实现了两个仍未关闭的 Critical（PR 缓存记录的身份——{{model}} → fetch 报告的 reviewModelId——以及过时的 <target>.json 命名文案），另加六条 Suggestion。该发现已确认并排队；本轮未对其改动代码。
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
…'s gaps Nine Criticals, and the shape is the same as last round: the fixes were what opened them. **The cache rename was split-brain (R2-1).** `cachePathFor` namespaces a file review's cache by source path, and the directory resolver still probed the old `<dir>/<target>.json` — so a file review reported "the cache is missing or unreadable" over a cache sitting right there. The resolver goes through the same speller now. (The follow-up PR already carried this fix; it belonged here, with the rename. Second time a fix and its dependency landed in different halves of this stack.) Step 6's prose named the old spelling too. **The stderr clean-tree warning was not gated (R2-2).** The field-level stop got `treeHeldStill` last round; the sentence beside it did not, so the round still printed "the working tree changed while the capture was being hashed" and "the working tree is clean" back to back — and the orchestrator branches on prose. It now says the tree is NOT clean and to re-run. **Three stops, two branches (R2-3, R2-12).** `scope-emptied` appeared nowhere in SKILL.md, so the reason existed with nothing to act on it; and the skipped-only shape — no chunks, non-empty `skippedFiles`, field deliberately withheld — had no executable continuation at all. Both have branches now, the second saying explicitly that it is NOT a stop. **The stop sidecar was collidable (R2-13).** It decides `completed` and can carry a REQUEST_CHANGES event, while its name is the flattened target token, which is not injective — and the epoch fence separates earlier runs, not concurrent ones. `run` stamps its child with a nonce and accepts only a sidecar stamped back. **A file review of an unmodified file read as a clean stop (R2-15).** An empty diff is not decided for a file target — SKILL.md's no-diff branch owes it a whole-file review — and marking it decided turned "Review did not complete" into a passing gate over a file nobody read. **One fixed plan name for every file review (R2-8).** File reviews take no lease and the plan is re-read all round, so two concurrent ones overwrote each other's central artifact mid-run: the second reviewed the first's file and merged its findings into the wrong ledger. The name must be unique to the run; it does not have to match anything. **The file-path ledger was write-only (R2-20).** Every ledger, incremental and stop bullet sat under the `local` branch; the `file` branch had none, so round 2 of a file review presented zero blockers over a Critical round 1 had recorded open. Mutation-checked: reverting the file-review exclusion, the sidecar stamp, or the stderr gate each turns exactly one test red.
…nd the naming prose follows the writer
Two Criticals and six Suggestions.
The PR cache wrote `lastModelId` from `{{model}}`, which interpolates the
BARE model id — but the same-model gate inside `fetch-pr` compares
whole-string against the provider-qualified identity it samples from the
runtime. A bare-id cache is refused as `cross-model-anchor` on every later
round and never heals (each clean round rewrites it bare again), so
cache-path incremental scoping and the `upToDate` stop were dead for every
PR, and a run that does not post lost the anchor entirely. Step 8 now copies
the fetch report's `reviewModelId` — the CLI-published qualified identity,
the gate's kind of string — verbatim, omitting the field when the runtime
published none (the gate then fails closed to a full review, the designed
state for an unrecorded identity). Pinned in SKILL.test.ts.
The round-2 commit renamed the file-review cache to
`file-<target>-<digest>.json` but left three texts spelling the old
`<target>.json`: the Step-6 ledger-source parenthetical (the finding's
anchor), the Step-1 incremental bullet, and `capture-local`'s own
docblock/help. All four now point at the plan's published `cachePath`, and
DESIGN.md's identity-channel section names what Step 8 actually types.
The six Suggestions: the garbled comment restored to English; the
model-refusal message's cached-side fallback aligned to `||` so a
legitimately-empty recorded identity prints "an unrecorded model" instead of
a blank name (regression test added); the source-path gate's stderr sentence
pinned plus a hostile-source escaping variant; the malformed-cache refusal
added as the third leg its test title always promised; the driver-binary
fold matched segment-exactly so a driver whose name is a prefix of another
(`md` / `mdbook`) no longer folds its config into the other's paths
(two-driver fixture added); and the shared `IncrementalScope.fullDiffPath`
declared optional, matching the PR producer that never emits it.
Every guard mutation-probed: each one removed makes its test fail, each one
restored makes the suite green (packages/cli review suites 4363 passed,
packages/core SKILL.test.ts 24 passed; build, typecheck, lint clean).
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:
- R1-35 SKILL.md:118 stop bullets unpinned in SKILL.test.ts — already reported location (comment 3833436330, R2-3 thread; overlap drop, distinct claim on the same line)
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 — stopped at the round cap (5) without two consecutive dry rounds; every round reported findings.
Not explored to full depth (tool budget reached): chunk 4: run capture-local.toctou.test.ts under vitest to confirm runtime pass (no node_modules in the shared review worktree); "agent reverse-audit (round 5)": none — the full chunk (diff lines 3935-4128) was read un-truncated and every check above completed.; chunk 12: vitest run of packages/cli/src/utils/paths.test.ts (worktree has no installed dependencies; assertions verified by executing the identical logic instead); chunk 7: running diff-plan.slice.test.ts and local-anchor.batch.test.ts under vitest (no node_modules in the review worktree or its parent; full monorepo install + b….
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/paths.ts:296 — [review] repoRelativeOf's added from parameter is never set by any callerpackages/cli/src/commands/review/capture-local.ts:101 — [review] cachePath doc comment states the wrong naming for file reviewspackages/core/src/skills/bundled/review/SKILL.md:273 — [review] Step 9 cleanup parenthetical contradicts the new token rule — subdirectory file reviews leak all side filespackages/cli/src/commands/review/capture-local.ts:697 — [review] foreign --cache file leaks its blocker count into the stop sidecarpackages/cli/src/commands/review/lib/local-diff.ts:290 — [review] captureLocalDiff doc comment orphaned by the inserted filter functionspackages/cli/src/commands/review/capture-local.incremental.test.ts:1020 — [review] subdirectory-cwd test never pins that root-level files are still capturedpackages/cli/src/commands/review/capture-local.ts:96 — [review] cacheCandidatePath published while withheld — no machine-readable withhold signalpackages/cli/src/commands/review/capture-local.incremental.test.ts:536 — [review] file-review ledger namespace has no openBlockers witnesspackages/cli/src/commands/review/capture-local.incremental.test.ts:523 — [review] sidecar write unpinned for unchanged-since and scope-emptied stopspackages/cli/src/commands/review/capture-local.ts:597 — [review] stop/comments/test-title contradict the changedSince-keyed stop for pending deletionspackages/cli/src/commands/review/lib/local-anchor.integration.test.ts:66 — [review] ENOBUFS witness hardcodes the 'unhashable' literal instead of the UNHASHABLE constantpackages/cli/src/commands/review/lib/local-diff.ts:333 — [review] rename INTO plumbing silently drops the user-side changepackages/cli/src/commands/review/lib/local-diff.ts:307 — [review] plumbing dir set hardcoded in a regex, duplicating paths.ts constants with no couplingpackages/core/src/skills/bundled/review/SKILL.md:1355 — [review] local cache write misses the unreviewedDimensions depth-exception the PR paragraph carriespackages/cli/src/commands/review/capture-local.incremental.test.ts:556 — [review] file-target stop-sidecar name unpinnedpackages/core/src/skills/bundled/review/DESIGN.md:559 — [review] DESIGN.md says the race is 'closed' by 'the two captures'; the code says three-pass tightened sample
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the round cap (5) without two consecutive dry rounds; every round reported findings。
未探索到全部深度（达到工具调用预算）：chunk 4：run capture-local.toctou.test.ts under vitest to confirm runtime pass (no node_modules in the shared review worktree)；"agent reverse-audit (round 5)"：none — the full chunk (diff lines 3935-4128) was read un-truncated and every check above completed.；chunk 12：vitest run of packages/cli/src/utils/paths.test.ts (worktree has no installed dependencies; assertions verified by executing the identical logic instead)；chunk 7：running diff-plan.slice.test.ts and local-anchor.batch.test.ts under vitest (no node_modules in the review worktree or its parent; full monorepo install + b…。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 16 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
|
||
| - **`file`** (e.g., `src/foo.ts`): | ||
| - Run `"${QWEN_CODE_CLI:-qwen}" review capture-local --file <file> --target <filename> --out .qwen/tmp/qwen-review-<filename>-plan.json` to get its changes (`--out` is required — see the capture block below for the full form). An **untracked** target file is captured whole (every line reads as added), which is the right frame for a file that does not exist upstream yet. The path is taken relative to **your** working directory and must be inside the repo. | ||
| - Run `"${QWEN_CODE_CLI:-qwen}" review capture-local --file <file> --out .qwen/tmp/qwen-review-<target>-plan.json` to get its changes (`--out` is required — see the capture block below for the full form). **A file review carries the same ledger and incremental rules as `local` above — read those four bullets and apply them here**: append `--cache .qwen/review-cache` at high effort (the DIRECTORY; the command resolves this target's file from the target it derives, and that name is namespaced by source path so it is not yours to spell), read the cache's `findings` at medium and high alike, and branch on `nothingToReview` exactly as they say. Without this the file-path ledger was write-only: Step 8 wrote it and nothing ever read it back, so round 2 of a high-effort file review presented zero blockers over a Critical round 1 had recorded as open. **Do not pass `--target` for a file review and do not compute one**: the command derives it from `--file`, using the same repo-relative canonicalisation and flattening `qwen review run` uses to name the artifacts it waits for. Applying that recipe by hand is what made the two disagree — the hand version normalises characters but does not canonicalise, so `ln -s src srclink` then a review of `srclink/foo.ts` had the parent waiting on one name while every child artifact carried another, and a review that had already run reported no verdict. An **untracked** target file is captured whole (every line reads as added), which is the right frame for a file that does not exist upstream yet. The path is taken relative to **your** working directory and must be inside the repo. |
There was a problem hiding this comment.
[Suggestion] The capture command template embeds <target> in --out .qwen/tmp/qwen-review-<target>-plan.json, but that token only comes into existence INSIDE the very command being invoked — the same bullet says "the command derives it from --file", and the comment below says --out is "the ONE name you may choose". The template demands a hand-derived token the diff everywhere forbids ("READ IT; do not recompute it"). The orchestrator must substitute something: left literal, bash reads <target> as an input redirection from a nonexistent file and the command dies before the CLI starts; hand-flattened (the pre-PR recipe this PR blames), ln -s src srclink + review of srclink/foo.ts yields srclink_foo.ts while the CLI canonicalises to src_foo.ts — a plan named outside every CLI-derived prefix; a basename fallback re-opens the collision the comment forbids ("Never the basename either"). Fix: replace qwen-review-<target>-plan.json in both templates with a free placeholder consistent with the comment's own recipe, e.g. --out .qwen/tmp/qwen-review-file-<unique>-plan.json.
中文说明
capture 命令模板把 <target> 嵌在 --out .qwen/tmp/qwen-review-<target>-plan.json 里，但这个 token 要到被调用的命令内部才存在——同一条目写着「命令从 --file 推导它」，下方注释又说 --out 是「唯一可由你选择的名字」。模板要求一个手工推导的 token，而 diff 处处禁止手工推导（「读它，不要重算」）。编排器必须替换它：原样保留时，bash 把 <target> 读成来自不存在文件的输入重定向，命令在 CLI 启动前就死掉；手工扁平化（本 PR 归咎的 PR 前做法）时，ln -s src srclink + 评审 srclink/foo.ts 得到 srclink_foo.ts，而 CLI 规范化为 src_foo.ts——计划文件命名在所有 CLI 推导前缀之外；退回 basename 则重新打开注释明令禁止的碰撞（「也不许用 basename」）。修复：把两处模板中的 qwen-review-<target>-plan.json 换成与注释自身做法一致的自由占位符，如 --out .qwen/tmp/qwen-review-file-<unique>-plan.json。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| --out .qwen/tmp/qwen-review-<target>-plan.json | ||
| # The plan's own `--out` is the ONE name you may choose: you write it and you | ||
| # read it back, so it cannot diverge from anything. Make it UNIQUE to this | ||
| # run — `.qwen/tmp/qwen-review-file-<something distinct to this review>-plan.json` |
There was a problem hiding this comment.
[Suggestion] The free plan name this capture-block comment recommends (with its file- infix, run-unique) sits OUTSIDE the qwen-review-<safeTarget(target)>- prefix Step 9's cleanup <target> sweeps by — cleanup.ts is prefix-only, no age-based reclaim — and promptRecordDir is named from the plan's basename, so neither the plan nor its -prompts directory is ever deleted. This is the residual the dev-bot's round-2 reply on R2-8 acknowledged as still open. Every file review that follows this recipe writes .qwen/tmp/qwen-review-file-src_foo.ts-plan.json and …-plan-prompts/ (every agent's launch prompt and brief); cleanup src_foo.ts matches neither, so both accumulate permanently. Pre-PR the plan was qwen-review-<filename>-plan.json with --target <filename>, so cleanup's same-filename prefix swept it; the #9206 retention logic keys on the same prefix and is blind to these directories too. Fix: instruct Step 9 to also delete the plan path the round actually chose (the orchestrator knows the name it wrote), or keep the plan inside the token prefix — drop the file- infix from the recipe since it guarantees the name misses the sweep.
中文说明
本 capture 块注释推荐的自由计划名（带 file- 中缀、每轮唯一）落在 Step 9 cleanup <target> 清扫前缀 qwen-review-<safeTarget(target)>- 之外——cleanup.ts 只按前缀清扫、没有按时间回收——且 promptRecordDir 以计划文件的 basename 命名，因此计划文件与它的 -prompts 目录都不会被删除。这正是 dev-bot 在 R2-2/R2-8 回复中承认仍未关闭的残留。每个遵循此做法的文件评审都会写 .qwen/tmp/qwen-review-file-src_foo.ts-plan.json 与 …-plan-prompts/（每个代理的启动提示与 brief）；cleanup src_foo.ts 两者都匹配不上，于是永久累积。PR 之前计划文件是 qwen-review-<filename>-plan.json 且配 --target <filename>，cleanup 的同名前缀能扫到它；#9206 的保留逻辑也以同一前缀为键，对这些目录同样不可见。修复：让 Step 9 同时删除本轮实际选用的计划路径（编排器知道自己写的名字），或让计划留在 token 前缀之内——去掉做法中的 file- 中缀，因为它保证名字错过清扫。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // whose path flattens alike writes the same path and its blocker | ||
| // count would decide the other run's exit code. Absent when the |
There was a problem hiding this comment.
[Suggestion] This stop.json comment asserts a collision consequence the shipped code makes impossible — run.ts's nothingToReviewFrom rejects any stop whose runId does not match (run.ts:228, each run stamping a fresh randomUUID()), so a foreign run's blocker count can never decide this run's exit code — and omits the residual that actually remains: the later capture's write REPLACES the earlier run's sidecar (same non-injective name), the overwritten run's parent polls, never sees its own runId again, times out, and reports "Review did not complete: no composed verdict was produced" over a round its own capture decided — the exact failure class the stop mechanism exists to close, for the concurrent shape. A maintainer hardening this path on the comment's evidence defends against a blocker-count crossover that is already fenced, while the live hole stays undocumented here — unlike run.ts's composed-verdict pin, which carries an explicit "Known residual races, accepted" block. Fix: rewrite the middle sentences to name the real residual — the runId fence keeps a foreign stop from deciding this run, but the shared name means the later write replaces the earlier one, and the run whose sidecar was overwritten polls to its timeout as incomplete.
中文说明
这段 stop.json 注释断言了一个已被现有代码排除的碰撞后果——run.ts 的 nothingToReviewFrom 拒绝任何 runId 不匹配的 stop（run.ts:228，每轮都盖一个全新的 randomUUID()），因此别的轮次的 blocker 计数不可能决定本轮的退出码——同时漏掉了真正残留的后果：较晚的捕获写入会替换较早轮次的 sidecar（同样的非单射名字），被覆盖轮次的父进程轮询时再也见不到自己的 runId，超时后对一个自家捕获已裁决的轮次报「Review did not complete: no composed verdict was produced」——正是 stop 机制要关闭的失败类别，在并发形状下的表现。维护者若按注释的证据加固此路径，会去防一个已被栅栏堵住的 blocker 计数串扰，而真正的活洞在这里没有记录——不像 run.ts 的 composed 定位注释那样带有明确的「已知残留竞态，可接受」块。修复：改写中间几句，写明真实残留——runId 栅栏保证外来 stop 不能裁决本轮，但共享名字意味着较晚的写入替换较早的写入，sidecar 被覆盖的轮次会轮询至超时、以未完成收场。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| * The local flow writes it; the PR flow has no retained full-range diff to | ||
| * point at yet and omits the field. | ||
| */ | ||
| fullDiffPath?: string | null; |
There was a problem hiding this comment.
[Suggestion] IncrementalScope.fullDiffPath is write-only: capture-local persists the entire pre-narrowing diff to diff-full.txt and publishes the pointer here on every incremental local round, but no production code, brief renderer, or SKILL.md prose ever reads it (repo-wide grep: the writer at capture-local.ts:547-548 + two tests only — one a fixture field never referenced by any assertion). The JSDoc's "for a reader who needs all of it" names a reader that does not exist. Every incrementally-scoped local round copies the full-range diff (up to the capture's own size ceiling) into .qwen/tmp and publishes a path no consumer honours — disk and write cost proportional to the full diff, for a field whose contract is unverifiable precisely because nothing consumes it; a future change to the write or the field passes CI silently in either direction (AGENTS.md's dead-switch rule). Fix: either name the reader (SKILL.md's incremental block or the whole-diff brief frame should point agents at fullDiffPath when the sliced diff is insufficient) or drop the diff-full.txt write and the field until a consumer exists.
中文说明
IncrementalScope.fullDiffPath 只写不读：capture-local 每轮增量本地评审都会把收窄前的完整 diff 落盘到 diff-full.txt 并在此发布指针，但没有任何生产代码、brief 渲染器或 SKILL.md 文案读它（全仓 grep：只有 capture-local.ts:547-548 的写入者与两个测试——其一还是从未被断言引用的 fixture 字段）。JSDoc 里「给需要全部的读者」指向的读者并不存在。每个增量收窄的本地轮次都把全量 diff（上限为捕获自身的大小上限）复制进 .qwen/tmp 并发布一个没有消费者认账的路径——磁盘与写入成本与全量 diff 成正比，而该字段的契约恰恰因无人消费而不可验证；未来无论改写入还是改字段都会静默通过 CI（AGENTS.md 的死开关规则）。修复：要么指明读者（SKILL.md 的增量块或全量 diff brief 框架应在切片 diff 不够时把代理指向 fullDiffPath），要么在出现消费者之前删除 diff-full.txt 写入与该字段。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
|
||
| - **`file`** (e.g., `src/foo.ts`): | ||
| - Run `"${QWEN_CODE_CLI:-qwen}" review capture-local --file <file> --target <filename> --out .qwen/tmp/qwen-review-<filename>-plan.json` to get its changes (`--out` is required — see the capture block below for the full form). An **untracked** target file is captured whole (every line reads as added), which is the right frame for a file that does not exist upstream yet. The path is taken relative to **your** working directory and must be inside the repo. | ||
| - Run `"${QWEN_CODE_CLI:-qwen}" review capture-local --file <file> --out .qwen/tmp/qwen-review-<target>-plan.json` to get its changes (`--out` is required — see the capture block below for the full form). **A file review carries the same ledger and incremental rules as `local` above — read those four bullets and apply them here**: append `--cache .qwen/review-cache` at high effort (the DIRECTORY; the command resolves this target's file from the target it derives, and that name is namespaced by source path so it is not yours to spell), read the cache's `findings` at medium and high alike, and branch on `nothingToReview` exactly as they say. Without this the file-path ledger was write-only: Step 8 wrote it and nothing ever read it back, so round 2 of a high-effort file review presented zero blockers over a Critical round 1 had recorded as open. **Do not pass `--target` for a file review and do not compute one**: the command derives it from `--file`, using the same repo-relative canonicalisation and flattening `qwen review run` uses to name the artifacts it waits for. Applying that recipe by hand is what made the two disagree — the hand version normalises characters but does not canonicalise, so `ln -s src srclink` then a review of `srclink/foo.ts` had the parent waiting on one name while every child artifact carried another, and a review that had already run reported no verdict. An **untracked** target file is captured whole (every line reads as added), which is the right frame for a file that does not exist upstream yet. The path is taken relative to **your** working directory and must be inside the repo. |
There was a problem hiding this comment.
[Suggestion] The file-review bullet says to "read those four bullets and apply them here", but the local section now carries SIX sub-bullets and no subset of four matches the sentence's referent ("the same ledger and incremental rules" = bullets 1–2, "branch on nothingToReview" = the stop bullets) — the count is stale and excludes exactly the two newest branches (scope-emptied and the skippedFiles non-stop). A file review whose target is SKIPPED — an untracked binary or over-cap file — produces chunks: [] + non-empty skippedFiles + no nothingToReview (file reviews are excluded only from the clean-tree stop via the args.file === undefined gate); the branch governing that shape is the skippedFiles bullet — one of the two a literal "four bullets" reading drops. An orchestrator honouring the count matches none of the first four, falls into the no-diff bullet's false antecedent ("the file is tracked and unmodified") or proceeds into a zero-chunk plan where agent-prompt --roster throws and the parent reports "Review did not complete" over a shape that has a decided procedure. Fix: replace "read those four bullets" with "read those bullets" (or "six bullets"), noting that the clean-tree bullet cannot fire for a file review while the skippedFiles branch can.
中文说明
文件评审条目写「读那四个子弹点并在此应用」，但 local 小节现在有六个子子弹点，且任何四个的子集都对不上这句话的所指（「相同的 ledger 与增量规则」= 子弹点 1–2，「按 nothingToReview 分支」= 各早停子弹点）——计数过期，恰好排除了最新的两个分支（scope-emptied 与 skippedFiles 非早停）。目标被跳过的文件评审——未跟踪的二进制或超上限文件——产出 chunks: [] + 非空 skippedFiles + 无 nothingToReview（文件评审只经 args.file === undefined 门被排除在 clean-tree 早停之外）；管辖该形状的正是 skippedFiles 子弹点——字面「四个子弹点」读法丢掉的那两个之一。编排器若照计数只读前四个，会落入无 diff 条目的假前提（「文件已跟踪且未修改」），或继续进入零 chunk 计划、让 agent-prompt --roster 抛错、父进程对一个已有既定程序的形状报「Review did not complete」。修复：把「读那四个子弹点」改成「读那些子弹点」（或「六个子弹点」），并注明 clean-tree 子弹点对文件评审不会触发而 skippedFiles 分支会。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Six Criticals, all in the capture's named-path handling, all probed against the round-3 commit before fixing. **The driver fold re-parsed its own serialization (R3-1).** A driver NAME may contain a comma — `*.bin diff=a,b` is a legal gitattributes line — and the fold matched by splitting the comma-joined attribute string, which can never equal such a value. Its `binary` flag silently left the identity, so flipping the config changed how `git diff` rendered the same bytes while the identity stood still: the next round certified the previous verdict against a different rendering. The fold now matches a structured path→driver map recorded during parsing. **Directory-shaped plumbing targets reviewed nothing (R1-21, escalated).** `.qwen/reviews/` is the shape the named-path exemption was designed for, but the exemption matched `p === pathspec` — a child path never equals its parent directory — so every child was filtered with no skipped record. The exemption now keys on one named-plumbing predicate that covers the path and its children. **The same flow was dead under ignore rules (R3-2).** `.qwen/*` in `.gitignore` is the common configuration, and `--exclude-standard` applies to explicitly named pathspecs too, so the named file never reached the filter at all. A deliberately named plumbing path now wins over ignore rules; the new integration tests carry `.qwen/*` in `.gitignore`. **A directory target kept its tracked plumbing (R3-4).** The tracked drop skipped for ANY pathspec, but only a FILE-shaped pathspec is "what the user asked for": a `sub/` target kept every tracked plumbing section beneath it while the untracked half dropped plumbing descendants. The drop now runs unless the pathspec itself is plumbing, so both halves of the capture agree. **The cap gate ran after the parse it exists to skip (R1-20, escalated).** An oversized tracked diff was decoded and section-parsed before the 10 MB cap rejected it — and near `gitRaw`'s 512 MiB ceiling an all-ASCII diff decodes past Node's maximum string length, so the decode threw instead of producing the graceful skip record. The gate now runs before any decode; an over-cap diff is rejected whole. **A file review of an unchanged file was told the tree is clean (R3-3).** The field-level stop excludes `--file` captures; the stderr sentence beside it did not, so the orchestrator — which reads prose too — stopped on "the working tree is clean" over a pathspec-scoped capture that says nothing about the tree. The prose now carries the same exclusion and points at the no-diff branch's whole-file review. Every guard mutation-probed: removing it turns its witness red, restoring it turns the suite green (packages/cli review suites 4365 passed, 4 skipped; build, typecheck, lint clean).
…ists, one path spelling R20-3 follow-up: the gitlink exemption took the diff's ABSENCE as git's answer that the pointer did not move — but with the submodule's gitdir gone that absence is git's silence. The pointer is now read directly (`rev-parse HEAD` inside the submodule) and compared to HEAD's recorded oid; a submodule whose HEAD cannot be read is unmeasurable, and unmeasurable is uncertifiable. R20-4 follow-up: the reserved list was under-enumerated — a repo-root file literally named `pr` derives the bare token `pr`, whose sweep prefix engulfs EVERY PR family while the lease guard lives inside the `pr-<n>` branch a bare `pr` never enters. `cleanup` refuses the token outright now (no target legitimately owns that prefix), and the Step 9 reserved list names it. R21-1: `repoRelativeOf` returned node's platform-separated `relative()` output verbatim, which flows into git pathspecs, the candidate's recorded `source`, and `cachePathFor`'s digest — on win32 one file got two cache filenames across platforms and the Windows lane failed every posix-spelled assertion. `rel` is normalized to forward slashes (git's own spelling on every platform), after the escape check computes against the platform separator. R21-2: the sampling loop kept only `.diff` from re-captures 1 and 2 — a file entering the window that lands in a skip class is in no capture's diff BYTES, so the byte comparison read "held still" while two of the three captures explicitly skipped content, and every stop gate reads only capture 0's list. Skip-set movement is tree movement: the skipped path-sets ride the treeHeldStill comparison now. All but the win32 normalization mutation-checked red locally (that branch is a no-op where sep is '/'; the Windows lane is its enforcement).
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.
17 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R22-2 seam-rendering assertion at capture-local.incremental.test.ts:401 — already reported (comments 3835476147, 3836053684)
- R22-4 orphaned doc comments (local-diff.ts:303, run.ts:207, capture-local.ts:162) — already disclosed in the round-20 deferral list (review 5032455046)
- R22-5 stop round prints literal 'Event: null' (run.ts:783) — already disclosed in the round-17 and round-20 deferral lists
- R22-6 DESIGN.md two-capture 'closed' overclaim (DESIGN.md:559) — already disclosed in the round-17 deferral list (review 5025131830)
- R22-7 SKILL.md:227 unbounded template — already disclosed in the round-19 deferral list (review 5029347616)
- R22-8 'read those four bullets' miscount (SKILL.md:227) — already reported (comment 3835476181)
- R22-9 runId fence rejection branch untested (run.ts:235) — already disclosed in the round-20 deferral list (review 5032455046)
- R22-20 cachePath docblock .json claim (capture-local.ts:117) — already reported (comment 3836053701)
- R22-21 ENOBUFS 94-byte buffer margin (local-anchor.integration.test.ts:68) — already reported (comments 3835476155, 3836053724)
- R22-23 movedSince zero tests (local-anchor.test.ts) — already reported (comment 3836053728)
- R22-24 R18-2 global-config fixture vacuity (local-anchor.integration.test.ts:448) — already disclosed in the round-20 deferral list (review 5032455046)
- R22-25 sliceDiffByLines test duplication (local-anchor.test.ts:160) — already reported (comment 3836053731)
- R22-28 DESIGN.md identity-shape understatement (DESIGN.md:559) — already reported (comment 3836053733)
- R22-29 SKILL.md:270 HHMMSS same-second collision claim — already disclosed in the round-19 deferral list (review 5029347616)
- R22-31 outside-repo assertion unfalsifiable (run-classify.integration.test.ts:113) — already disclosed in the round-21 deferral list (review 5035530595)
- R22-32 persistence.md:16 case-sensitive .md rule — already disclosed in the round-16 deferral list (review 5017220467)
- R22-33 lastModelId copy/omit rules pinned by no test (persistence.md:105) — already disclosed in the round-21 deferral list (review 5035530595)
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and the suites did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: test-efficacy probe — inconclusive (the probe harness control was never validated; no survivors detected).
Not explored to full depth (tool budget reached): chunk 5: ran capture-local.incremental.test.ts at the reviewed commit — worktree has no node_modules/dist, so execution was skipped in favor of the manual trace above..
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 22, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-local.incremental.test.ts:574 — [probe] the incremental stops never pin the stop-sidecar writepackages/cli/src/commands/review/capture-local.incremental.test.ts:1684 — [probe] the visibility-bit stop refusal is pinned by no testpackages/cli/src/commands/review/capture-local.toctou.test.ts:43 — [probe] the TOCTOU suite is blind to sample orderingpackages/cli/src/commands/review/lib/local-anchor.integration.test.ts:425 — [probe] the combined-bit s tag test cannot go red under that regressionpackages/cli/src/commands/review/run.ts:192 — [review] the accepted-races list still describes the retired basename regimepackages/cli/src/commands/review/run.ts:521 — [review] the runId fence comment overstates the sidecar's verdict powerpackages/core/src/skills/bundled/review/SKILL.test.ts:1246 — [probe] the bounded-name test lacks the negative assertion
Convergence: round 22 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/cli/src/commands/review/capture-local.ts (findings in rounds 20, 21; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 17 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI and the suites did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：test-efficacy probe — inconclusive (the probe harness control was never validated; no survivors detected)。
未探索到全部深度（达到工具调用预算）：chunk 5：ran capture-local.incremental.test.ts at the reviewed commit — worktree has no node_modules/dist, so execution was skipped in favor of the manual trace above.。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 22 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 22 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/capture-local.ts（第 20、21 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
…ides R22-1: a moved or in-diff submodule pointer entered the hashed population and could only record UNHASHABLE — which never equals itself — so changedSince reported it every round and the unchanged-since stop was unreachable for the lifetime of any change set holding a dirty pointer, with a misdescribing diagnosis. Yet the module already measures exactly this identity (the R20-3 fix did it inside a special case). Make the identity real instead: hashWorktreeFiles answers 160000:<oid> for a readable, content-CLEAN submodule (the pointer oid says nothing about internal edits — git renders those as `-dirty` — so cleanliness is part of measurability and a dirty submodule stays UNHASHABLE), and revisionIdentities answers the same shape from ls-tree's recorded oid. With both sides real, vanishedStillOnDisk's R20-3 special case became redundant and is deleted along with its helper: a restored pointer certifies through the ordinary equality, an unreadable or dirty one refuses through the ordinary UNHASHABLE clause — net-negative plumbing. The submodule fixture now pins all three arms: the dirty pointer CONVERGES (unchanged-since reachable), internal dirt never certifies (no decided stop over `-dirty` bytes), and the odb-removed pointer still refuses. Both mutants — UNHASHABLE-again and dirt-invisible — turn it red.
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.
Not explored to full depth (tool budget reached): chunk 14: end-to-end vitest run of local-anchor.integration.test.ts (review worktree has no node_modules and the cli vitest globalSetup guard requires the full workspace …; chunk 5: live execution of the test file — the review worktree has no node_modules , so vitest would need npm ci plus the workspace build, which exceeded the remainin….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Deferred under the convergence posture (round 23, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/capture-local.incremental.test.ts:297 — [probe] deletion-convergence test pins only stderr prose, not the machine channelpackages/cli/src/commands/review/capture-local.incremental.test.ts:1579 — [probe] round-12 test exercises 'HEAD moved', not the vanished refusal it claimspackages/cli/src/commands/review/capture-local.toctou.test.ts:182 — [probe] TOCTOU test passes a dead model argv key; isolation claim falsepackages/cli/src/commands/review/capture-local.ts:100 — [probe] CaptureLocalResult omits nothingToReview from its declared shapepackages/cli/src/commands/review/lib/local-anchor.integration.test.ts:85 — [review] gitWithInputRaw's CRLF-rewrite axis has no witnesspackages/cli/src/commands/review/lib/local-anchor.integration.test.ts:434 — [probe] no arm requires a lowercase-s ls-files entry to be flaggedpackages/cli/src/commands/review/lib/local-anchor.integration.test.ts:448 — [probe] R18-2(b) stays green with --worktree removed from the sparse flag readpackages/cli/src/commands/review/lib/local-anchor.test.ts:160 — [review] slicer contracts duplicated between two new test filespackages/cli/src/commands/review/lib/local-anchor.ts:273 — [review] documented 'unknown' fallback sentinel does not existpackages/cli/src/commands/review/lib/local-anchor.ts:485 — [probe] one empty-string cache key fatals the whole ls-tree batchpackages/cli/src/commands/review/lib/local-anchor.ts:553 — [probe] gitlinkIdentity runs git -C outside the repo for escaping pathspackages/cli/src/commands/review/lib/local-anchor.ts:795 — [review] movedSince doc misdescribes what the stop keys onpackages/cli/src/commands/review/lib/local-diff.ts:315 — [review] captureLocalDiff JSDoc orphaned by the inserted helperspackages/cli/src/commands/review/lib/local-diff.ts:361 — [probe] dropPlumbingSections mutes a rename INTO a plumbing pathpackages/cli/src/commands/review/lib/report.ts:315 — [review] fullDiffPath written every incremental round, zero consumerspackages/cli/src/commands/review/run-classify.integration.test.ts:116 — [probe] outside-repo assertion is structurally vacuouspackages/cli/src/commands/review/run.ts:172 — [review] composedNameFor doc comment stale (basename naming, wrong race list)packages/cli/src/commands/review/run.ts:207 — [review] composedNameFor's contract JSDoc orphaned by stop-helper insertionpackages/cli/src/commands/review/run.ts:235 — [probe] stop-sidecar runId fence's refusal branch has no witnesspackages/cli/src/commands/review/run.ts:747 — [probe] stop-round prose prints the literal Event: null- …and 9 more (see the run report)
Convergence: round 23 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 1 (1 new). Findings keep coming back to the same files: packages/cli/src/commands/review/capture-local.ts (findings in round 22; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (6 Critical(s)), the rate of first-time findings is not falling (this round 6, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 14：end-to-end vitest run of local-anchor.integration.test.ts (review worktree has no node_modules and the cli vitest globalSetup guard requires the full workspace …；chunk 5：live execution of the test file — the review worktree has no node_modules , so vitest would need npm ci plus the workspace build, which exceeded the remainin…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
收敛姿态下延后（第 23 轮，非阻断）——已记录，本轮不要求修改：共 29 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 23 轮发布了 6 条行内评论，其中 6 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/capture-local.ts（第 22 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 6 条 Critical），首次发现的速率没有下降（本轮 6，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
# Conflicts: # packages/core/src/skills/bundled/review/references/persistence.md
…ude file targets, keep every byte Six findings, all confirmed against the tree: The candidate write now gates on the dropped-out-while-on-disk set: a refused-anchor round used to write a candidate that silently OMITTED the dropped path, Step 8 promoted the omission, and two rounds later a scope-emptied stop certified bytes no round read. The cache is read before the write now, the withholding is voiced with its own sentence, and the scoping branch reuses the early read. The unchanged-since-last-round stop gains the file-review exclusion BOTH sibling stops carry — a cached round-2 file review of an unmodified subject stopped decided while the identical tree without a cache routed to the whole-file review. The excluded shape gets its own honest stderr line instead of falling into the unhashable-paths diagnosis, and the directory-subject test now pins convergence as the ABSENCE of the wedge. gitlinkIdentity asks the submodule's OWN visibility bits: status --porcelain honours an assume-unchanged bit set inside the submodule, so cleanliness judged by status alone held the identity still over interior bytes no round can see — the fix-induced half of R22-1, closed with the same oracle one level down. canonicalise strips only the platform's separators on the ancestor walk: `\` is a legal POSIX filename byte this PR's own fixtures insist on, and the two-class strip corrupted a dangling `\link` into `link`. SKILL.md: the Step 1 file bullet's --out template carries the 24-char truncation (the full-basename spelling died with ENAMETOOLONG past ~226-byte basenames, measured), and both PR stops (up-to-date, empty diff) now write the stop sidecar with the run's nonce before cleanup — the reader predicted the name but nothing in the PR flow ever wrote it, so every decided PR stop exited 1 "Review did not complete". All four code fixes mutation-checked red; guards pin both prose fixes.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 140 passed · 0 failed · 140 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：140 通过 · 0 失败 · 140 总计 抖动门： Verification reportPR 9659 deep verification — follow-up round 2 (rounds 16–23 delta)Verdict: 中文摘要（点击展开）
Previous-finding status (follow-up round, re-measured at the new head)The previous round verified
No declined or deferred rows existed to re-measure. Central claim + A/B (re-measured at the new head)Central claim: a local/file review round can skip already-reviewed work — anchored on hashed per-file state ( Same deterministic scenario as the prior round (3-file import chain
25/25 ( Delta probes (rounds 16–23, prev
|
| Cell | prev (dc74e83b81) |
head (f1d9c624ab) |
|---|---|---|
r2: cache names {a.ts,b.ts}; b.ts becomes git-ignored and diverges |
anchor refused, but candidate written silently omitting b.ts | anchor refused naming b.ts; candidate withheld (no file, no cacheCandidateStateId), full review proceeds |
| r3 after promoting r2's candidate | decided unchanged-since-last-round — the exploit end-to-end: certifies over b.ts's diverged, never-read bytes |
still refused (round-1 cache still names b.ts), no stop |
| file review, cached round 2 of an unmodified subject | decided unchanged-since-last-round over a file target |
excluded: "a file review never stops decided here"; agrees with the no-cache shape |
discarded change (git checkout --) |
no channel names the affected paths | incremental.scope.supersededPaths names it; prose names the split |
R16 — both incremental stops gain the --no-untracked exclusion (delta-untracked-stops.mjs, 14/14, witness 03-delta-untracked-r16-prev-vs-head.png): unchanged-since and scope-emptied stops both decided on prev under --no-untracked (tracked-clean + pending untracked work), both withheld out loud on head; default-flag parity holds on both arms.
R20–23 — gitlink identity (delta-gitlink.mjs, 13/13, witness 04-delta-gitlink-wedge-prev-vs-head.png), real submodules:
| Cell | prev | head |
|---|---|---|
| clean moved pointer | candidate records unhashable → round 2 wedged ("could not be hashed on either side", never stops) |
candidate records 160000:<oid> (the submodule's own HEAD, verified against git) → round 2 decides unchanged-since-last-round |
| internal dirt (uncommitted edit) | unhashable (parity) | unhashable — dirt never certifies, no decided stop |
| internal edit hidden by assume-unchanged inside the submodule (status reads clean) | unhashable (parity, different reason) | unhashable — R23's lower-level oracle enumerates the submodule's own bits (control: status --porcelain provably clean over the hidden edit) |
Carried — bracketed visibility oracle under the shim race (delta-visibility.mjs, 12/12, witness 05-visibility-race-bracket-carried.png): a PATH shim delegating to real git counts invocations and clears the race bit right after the 3rd hash pass. Counters are asserted in-harness (hash:3, ls-files -v:4 on both arms) so a silent no-op shim would fail the run. Race withheld on both arms (the bracket existed at prev; the delta rewrote the file — regression check), honest path decides on both, failed enumeration fails closed.
Corrections
None. The previous round's descriptions of the code were accurate at its head; every carried measurement re-run here agreed with them.
Findings
1. (Suggestion — test portability / environment) The sparse-checkout exemption arms of the PR's own local-anchor.integration.test.ts require git ≥ 2.40; on this container's git 2.39.5 they are red, and the exemption they test silently never fires on such gits.
The cli gate ran the 18 changed test files: 344 passed, 3 failed — and the 3 reds are exactly the sparse-exemption arms (invisibleTrackedPaths — sparse-checkout owns its S bits):
FAIL src/commands/review/lib/local-anchor.integration.test.ts > ... > exempts out-of-cone S paths, keeps manual bits and non-sparse absences
AssertionError: expected [ 'sub/out.ts' ] to deeply equal []
Attribution, proven rather than inferred:
- Direct probe in a scratch sparse repo:
git sparse-checkout check-rules→error: unknown subcommand: 'check-rules'— the subcommand was added in git 2.40; the container (Debian bookworm) ships git 2.39.5. - The production code's fallback for exactly this case is documented in
invisibleTrackedPathsitself: "A failed check-rules (an older git without the subcommand) exempts nothing: fail closed, at the cost of the pre-exemption wedge on that git, never a certification." Observed behavior matches the comment precisely: the path stays flagged → candidates/stops withhold → over-review, never false certification. - Direction is safe, and not a regression: at prev (
dc74e83b81) the exemption did not exist at all, so sparse repos wedged identically on this git; the base tip has no mechanism. The PR's own CI runners (ubuntu-latest, git ≥ 2.43) exercise the green arm. - Blast radius: any environment with git < 2.40 — Debian stable and derivatives, and notably the
node:22-bookwormcontainers this repo's own lanes use — will see these three tests red whenever the file is run (e.g. by a flakiness gate), and sparse-checkout users there keep paying the pre-R18 over-review cost.
Repro: git sparse-checkout check-rules under git 2.39.5 (unknown subcommand), or (cd packages/cli && npx vitest run src/commands/review/lib/local-anchor.integration.test.ts) in this container.
Suggested fix (not applied or measured — author's call): guard the three arms with a capability check (skip when git sparse-checkout check-rules is unavailable), the same pattern the file's scripted-git oracle suite uses for version-dependent arms. The runtime behaviour needs no change — the fail-closed fallback is the right lean.
No behavioral findings: every probe of the PR's central mechanism and of the delta guards behaved as designed, in both directions (withhold and decide), on both arms. The PR text contained no steering or injection attempts.
Not covered
- Per-commit attribution. Depth-2 merge-ref checkout:
git rev-list HEAD^1..HEAD^2returns 1 (a shallow-boundary artifact) while the snapshot lists 40 commits;--is-shallow-repository= true. The delta commits (c723d5fd7a…f1d9c624ab) are individually reachable and each was diffed and scoped; the aggregateHEAD^1..HEADdiff was verified; per-commit claims for the older 23 commits remain out of reach. The added-then-reverted pair (af521c5d58/0d11241f39) was verified byte-net-zero. - The residual multi-write race (five-plus timed writes phase-aligned across neighbouring samples) — analytical only, as in the prior round; nothing in the delta claims to close it.
- Model-side orchestration. SKILL.md's stop/split bullets are pinned by
SKILL.test.ts(51/51) as text; Steps remain prose the model executes — no live model round, noqwen review runE2E with a model. The PR-flow stop-sidecar reader (run.ts:nothingToReviewFrom→readStopSidecar, runId- and cutoff-fenced) was checked statically against R23's prose contract; the writer side is model prose. - Sparse-checkout on git ≥ 2.40 — the container cannot stage it; only the fail-closed fallback was measurable here (Finding 1).
- Windows path behavior (R21-1 forward-slash normalization) — posix lane only; pinned by the unit suite, which passed.
- The prev control build carried one type-only cross-tree error (
acp-integration/acpAgent.tsvs head-treeacp-bridgetypes); emit completed and the prev CLI was smoke-tested operational. Not a PR defect — an artifact of compiling an older cli against head's unchanged-package dists.
Methodology
Single node:22-bookworm container, git 2.39.5, node v22.23.2; npm ci + npm run build pre-existed at the merge ref. Three trees: head (pre-built dist), tmp/base-tree (HEAD^1 = fdd7248acb), tmp/prev-tree (dc74e83b81, reachable in this checkout's object store). Both control trees rebuilt packages/core + packages/cli from source with the main tree's root and per-package node_modules symlinked (lockfile and package.json untouched by the PR, so the dependency tree is a clean control); internal-link check asserted as required — import.meta.resolve('@qwen-code/qwen-code-core') from each control tree's cli dist resolves into that tree's own core build (tmp/base-tree/packages/core/dist/index.js, tmp/prev-tree/packages/core/dist/index.js), so no control loaded head code. Harnesses drive the compiled CLIs (node <tree>/packages/cli/dist/index.js review capture-local …) against scratch git repos with QWEN_CODE_MODEL_IDENTITY=test-model@cafe1234; Step 8 promotion is simulated by copying the written candidate to the plan-published cachePath. The visibility race uses a PATH shim (shim/git) delegating to real git, counting hash-object and ls-files -v invocations and clearing the race bit after the Nth hash pass; counters are asserted in-harness. Mutation matrix: five one-point source mutations in the head tree, witness vitest suites, exact-string restore after each (post-matrix git status --porcelain over the review surface = clean); baseline 71/71 green is the positive control. (In 06-mutation-matrix-5-of-5-killed.png the runner's "result" field mislabels the pipe exit code — read the excerpts: every mutant went RED with the quoted assertion mismatch, which is the evidence the report cites.) Gates: the 18 changed cli test files in one vitest run + core SKILL.test.ts, with the 3 reds' environmental cause proven by a direct subcommand probe. Evidence images were produced with scripts/verify-capture.mjs (images 01–05, 07, 08 are live re-runs of the harnesses; 06 renders the recorded matrix log). Harness scripts, per-cell logs, gate/matrix/stability logs live beside this report.
Flakiness gate log
rounds=5 files=19 skipped=0
file packages/cli/src/commands/review/capture-local.incremental.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/capture-local.incremental.test.ts
file packages/cli/src/commands/review/capture-local.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/capture-local.test.ts
file packages/cli/src/commands/review/capture-local.toctou.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/capture-local.toctou.test.ts
file packages/cli/src/commands/review/capture-local.visibility.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/capture-local.visibility.test.ts
file packages/cli/src/commands/review/cleanup.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/cleanup.test.ts
file packages/cli/src/commands/review/lib/diff-plan.slice.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/diff-plan.slice.test.ts
file packages/cli/src/commands/review/lib/local-anchor.batch.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/local-anchor.batch.test.ts
file packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/local-anchor.integration.test.ts
file packages/cli/src/commands/review/lib/local-anchor.oracle.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/local-anchor.oracle.test.ts
file packages/cli/src/commands/review/lib/local-anchor.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/local-anchor.test.ts
file packages/cli/src/commands/review/lib/local-diff.integration.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/local-diff.integration.test.ts
file packages/cli/src/commands/review/lib/paths.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/paths.test.ts
file packages/cli/src/commands/review/run-classify.integration.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/run-classify.integration.test.ts
file packages/cli/src/commands/review/run-skill-parity.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/run-skill-parity.test.ts
file packages/cli/src/commands/review/run.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/run.test.ts
file packages/cli/src/commands/review/script-lint-isolation.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/script-lint-isolation.test.ts
file packages/cli/src/ui/hooks/session-swap-telemetry.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/session-swap-telemetry.test.ts
file packages/cli/src/utils/paths.test.ts: (cd packages/cli) npx --no-install vitest run ./src/utils/paths.test.ts
file packages/core/src/skills/bundled/review/SKILL.test.ts: (cd packages/core) npx --no-install vitest run ./src/skills/bundled/review/SKILL.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/capture-local.incremental.test.ts: PPPP
packages/cli/src/commands/review/capture-local.test.ts: PPP
packages/cli/src/commands/review/capture-local.toctou.test.ts: PPP
packages/cli/src/commands/review/capture-local.visibility.test.ts: PPP
packages/cli/src/commands/review/cleanup.test.ts: PPP
packages/cli/src/commands/review/lib/diff-plan.slice.test.ts: PPP
packages/cli/src/commands/review/lib/local-anchor.batch.test.ts: PPP
packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: FFF
packages/cli/src/commands/review/lib/local-anchor.oracle.test.ts: PPP
packages/cli/src/commands/review/lib/local-anchor.test.ts: PPP
packages/cli/src/commands/review/lib/local-diff.integration.test.ts: PPP
packages/cli/src/commands/review/lib/paths.test.ts: PPP
packages/cli/src/commands/review/run-classify.integration.test.ts: PPP
packages/cli/src/commands/review/run-skill-parity.test.ts: PPP
packages/cli/src/commands/review/run.test.ts: PPP
packages/cli/src/commands/review/script-lint-isolation.test.ts: PPP
packages/cli/src/ui/hooks/session-swap-telemetry.test.ts: PPP
packages/cli/src/utils/paths.test.ts: PPP
packages/core/src/skills/bundled/review/SKILL.test.ts: PPP
verdict: consistent-fail
summary: 1 of 19 changed test file(s) failed identically in every round — deterministic, so CI owns that signal
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/capture-local.incremental.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/capture-local.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/capture-local.toctou.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/capture-local.visibility.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/cleanup.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/diff-plan.slice.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/local-anchor.batch.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/local-anchor.integration.test.ts: F (exit 1)
--- output tail · round 1 · packages/cli/src/commands/review/lib/local-anchor.integration.test.ts ---
w/lib/local-anchor.integration.test.ts �[2m(�[22m�[2m20 tests�[22m�[2m | �[22m�[31m3 failed�[39m�[2m)�[22m�[33m 632�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — the attributes probe is not buffer-bound�[2m > �[22manswers for a path count whose check-attr output passes 1 MB�[32m 239�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — the attributes probe is byte-faithful�[2m > �[22mkeeps the record of a path that begins with whitespace�[32m 18�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — the attributes probe is byte-faithful�[2m > �[22mfolds a driver's binary flag only into the paths naming THAT driver�[32m 19�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — the attributes probe is byte-faithful�[2m > �[22mfolds a driver whose NAME CONTAINS A COMMA — matched as a value, never re-parsed�[32m 27�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — the attributes probe is byte-faithful�[2m > �[22ma driver literally named `set` is uncertifiable — the answer is ambiguous�[32m 16�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — the attributes probe is byte-faithful�[2m > �[22ma plain `diff` attribute is uncertifiable too — the sibling ambiguity�[32m 13�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — a decoded path is not a name�[2m > �[22mrefuses to hash a path carrying U+FFFD�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — every diff-driver spelling reaches the fold�[2m > �[22mfolds the EMPTY driver name, which git accepts as `diff..binary`�[32m 22�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — an undecodable driver name unhashes the WHOLE identity�[2m > �[22mmarks the identity UNHASHABLE, not a composite ending in the slot�[32m 15�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — an undecodable driver name unhashes the WHOLE identity�[2m > �[22mre-enters scope on the next round instead of comparing unchanged�[32m 17�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — a path listed twice is hashed once�[2m > �[22mdedups repeated input paths at the module boundary�[32m 21�[2mms�[22m�[39m
�[32m✓�[39m revisionIdentities — ledger paths are read literally�[2m > �[22msurvives a path beginning with pathspec magic�[32m 25�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — a state name spelled as a value is uncertifiable�[2m > �[22mfolds the whole identity to UNHASHABLE when `diff` answers `unset` or `set`�[32m 20�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — a state name spelled as a value is uncertifiable�[2m > �[22ma flip between the two ambiguous spellings re-enters scope�[32m 17�[2mms�[22m�[39m
�[32m✓�[39m hashWorktreeFiles — a state name spelled as a value is uncertifiable�[2m > �[22mkeeps unambiguous answers folding — the closure narrows, not widens�[32m 18�[2mms�[22m�[39m
�[31m �[31m×�[31m invisibleTrackedPaths — sparse-checkout owns its S bits�[2m > �[22mexempts out-of-cone S paths, keeps manual bits and non-sparse absences�[39m�[32m 36�[2mms�[22m�[39m
�[31m → expected [ 'sub/out.ts' ] to deeply equal []�[39m
�[31m �[31m×�[31m invisibleTrackedPaths — sparse-checkout owns its S bits�[2m > �[22mexempts the combined-bit lowercase `s` out-of-cone spelling too�[39m�[32m 26�[2mms�[22m�[39m
�[31m → expected [ 'sub/out.ts' ] to deeply equal []�[39m
�[32m✓�[39m invisibleTrackedPaths — sparse-checkout owns its S bits�[2m > �[22man inherited GLOBAL core.sparseCheckout never turns the exemption on�[32m 23�[2mms�[22m�[39m
�[31m �[31m×�[31m invisibleTrackedPaths — sparse-checkout owns its S bits�[2m > �[22mreads the flag as a canonicalized bool — legacy spellings count�[39m�[32m 24�[2mms�[22m�[39m
�[31m → expected [ 'sub/out.ts' ] to deeply equal []�[39m
�[32m✓�[39m hashWorktreeFiles — a configured `unspecified` driver is uncertifiable�[2m > �[22mtakes UNHASHABLE under diff.unspecified.binary, folds nothing without it�[32m 25�[2mms�[22m�[39m
�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 3 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[39m
�[41m�[1m FAIL �[22m�[49m src/commands/review/lib/local-anchor.integration.test.ts�[2m > �[22minvisibleTrackedPaths — sparse-checkout owns its S bits�[2m > �[22mexempts out-of-cone S paths, keeps manual bits and non-sparse absences
�[31m�[1mAssertionError�[22m: expected [ 'sub/out.ts' ] to deeply equal []�[39m
�[32m- Expected�[39m
�[31m+ Received�[39m
�[32m- []�[39m
�[31m+ [�[39m
�[31m+ "sub/out.ts",�[39m
�[31m+ ]�[39m
�[36m �[2m❯�[22m src/commands/review/lib/local-anchor.integration.test.ts:�[2m405:41�[22m�[39m
�[90m403| �[39
...truncated -- full content in the run artifacts.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
| - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): read `.qwen/review-cache/pr-<n>.json` **before** `fetch-pr` (it is a local file; nothing about it needs the fetch) and, when it holds a `lastCommitSha`, pass BOTH fields to the fetch verbatim: `--since <lastCommitSha> --since-model <lastModelId>` (omit `--since-model` when the cache has no `lastModelId`; do not substitute anything for it). **Copy them; do not compare them to anything.** The same-model gate is ruled inside `fetch-pr`, over the identity the runtime published — "clean up to `lastCommitSha`" is the recorded identity's verdict, and the command validates an anchor against the HISTORY, never against who certified it, so an anchor from another identity is ancestrally perfect and would scope this round past code it never reviewed. A hand-applied version of that gate was wrong every time it was written, because `{{model}}` interpolates the BARE model id while every identity the CLI records is provider-qualified: two provider configurations exposing one model name compared equal and passed each other's gate. When the gate refuses, the report says `cross-model-anchor` and the round reviews the full diff. Read the cache's `findings` ledger either way (Step 6 owes each entry a ruling; the work list carries across models, only the anchor does not). **You never run `git` against an anchor yourself** — no `git diff <sha>..HEAD`, no `cat-file`, no `merge-base --is-ancestor`: the command validates the anchor against the fetched history and computes the scoped diff and chunk plan in one pass, because a hand-run check is one a run can skip, and the hand-computed delta was exactly the shape this skill forbids everywhere else (the diff is a file the CLI writes, never a command you run). The report's `incremental` field is the decision; act on it with `lastModelId` from the cache and the current model ID (`{{model}}`): | ||
| - `effective: true` (no `upToDate`) → the report's diff and plan ARE the incremental scope (`since..head`); continue with them exactly as with a full plan. The file set is **widened by one import hop**: a still-clean source file that imports a changed one re-enters the scope with its own full-range hunks, because the round before cleared it against the callee's OLD shape. `incremental.scope` names each file's class — `deltaFiles` (touched since the anchor), `interaction[]` (widened back in, each with the edges that did it), `contextFileCount` (weighed and passed over) — and a chunk brief built for an interaction file points its agent at that seam instead of a from-scratch re-review. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. (Reachable only under a matching identity: the gate inside the command is what keeps a cross-model anchor from scoping anything.) | ||
| - `upToDate: true` **and** `comment.effective` is false (no `--comment` flag, and `review.comment` not enabled in settings) → inform the user "No new changes since last review" (this branch consumes no plan, so it holds even when `diffPath` is null), run `"${QWEN_CODE_CLI:-qwen}" review cleanup pr-<n>` to remove the worktree just created, and stop. **This branch does not apply on a resumed run** (`resumed: true` from the resume branch below): a continuation's `incremental` field is the interrupted attempt's history, not this run's decision, and taking the stop/cleanup here would destroy the very state `--resume` reused. | ||
| - `upToDate: true` **and** `comment.effective` is false (no `--comment` flag, and `review.comment` not enabled in settings) → inform the user "No new changes since last review" (this branch consumes no plan, so it holds even when `diffPath` is null). **Before the cleanup, write the stop sidecar** so `qwen review run` reads the round as DECIDED instead of exiting 1 "Review did not complete" over it: when the environment carries `QWEN_REVIEW_RUN_ID`, write `.qwen/tmp/qwen-review-pr-<n>-stop.json` containing exactly `{"reason": "<up-to-date|empty-diff>", "runId": "<the QWEN_REVIEW_RUN_ID value>"}` — the same reason+runId contract `capture-local` writes for local stops, runId copied verbatim (the parent's reader is nonce-fenced and discards any other stamp); without that variable no parent is reading and the file is not written. Then run `"${QWEN_CODE_CLI:-qwen}" review cleanup pr-<n>` to remove the worktree just created, and stop. **This branch does not apply on a resumed run** (`resumed: true` from the resume branch below): a continuation's `incremental` field is the interrupted attempt's history, not this run's decision, and taking the stop/cleanup here would destroy the very state `--resume` reused. |
There was a problem hiding this comment.
[Critical] 这里要求先写 stop sidecar、随后立即运行 cleanup pr-<n>，但 cleanup 会删除同一 qwen-review-pr-<n>-* 前缀下的 sidecar，而父进程的首次轮询要等 250 ms。只要写入和 cleanup 在首个 tick 前完成，in-run snapshot 与 child 退出后的 fallback 都读不到文件，一个已经判定为 up-to-date / empty-diff 的 PR 仍会落成 completed: false、exit 1（Review did not complete）。我在当前 head 的 run.test.ts fake-child 框架中让真实 cleanup 在首个 timer tick 前执行，稳定复现 result.completed === false；现有两个 sidecar 竞态测试都先推进 1000 ms，正好绕过了这个窗口。请让 sidecar 在 cleanup 边界上保持可消费（例如使用 cleanup 不会 sweep 的 per-run 路径，或由 cleanup 暂留当前 run 的 sidecar、父进程读取后再清理），并补一个 cleanup-before-first-poll 回归。
English: The new PR stop path writes the sidecar and immediately runs cleanup, but cleanup sweeps that same prefix before the parent's first 250 ms poll can be guaranteed. If both operations finish before the first tick, neither the in-run snapshot nor the post-close fallback can observe the decision, so an already-decided up-to-date / empty-diff round still returns completed: false and exit 1. I reproduced this deterministically at the current head by running the real cleanup before the first fake-timer tick; the existing race tests both advance 1000 ms first and therefore miss this window. Please keep the sidecar consumable across cleanup and add a cleanup-before-first-poll regression.
There was a problem hiding this comment.
Fixed in f425a8c — thank you, the window was exactly as you framed it: the local flow survives only because its stop (Step 1) and cleanup (Step 9) are minutes apart, and the round-23 protocol collapsed that gap to zero for PR stops.
Taken with your second suggested shape: cleanup now spares <prefix>stop.json exactly when its runId matches the environment the parent stamped — a foreign or unstamped sidecar is residue and sweeps as before, and the next run's cleanup (different nonce) collects the kept one. SKILL.md notes the retention beside the write instruction so the orchestrator does not tidy the file by hand.
Pinned from both sides of the window, per your repro note: cleanup.test spares the matching sidecar and sweeps a foreign one (mutation-checked — reverting the sparing turns it red), and run.test gains a zero-timer-advance arm (child writes the sidecar and closes on a microtask before ANY poll tick; the post-close fallback still reads the decision) — you were right that the existing race arms' 1000 ms advance was exactly how this window went unpinned.
修复于 f425a8c,采用您建议的第二种方案:cleanup 仅在 stop.json 的 runId 与父进程注入的环境值一致时暂留(异 runId/无 runId 视为残渣照常清扫,下一个 run 的 cleanup 自然回收);SKILL.md 在写入指令旁注明留存语义。两侧回归:cleanup.test 钉住暂留与异 runId 清扫(变异验红),run.test 补零推进臂(子进程在任何轮询 tick 前微任务关闭,post-close 兜底仍读到裁决)——您指出的"现有竞态测试都先推进 1000ms"正是该窗口漏钉的原因。
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. ✅
Human review on #9659: the round-23 PR stop protocol writes the sidecar and runs `cleanup pr-<n>` in the same breath, but cleanup swept the same prefix — and the parent's first in-run poll is up to 250 ms away, so a write+cleanup finishing before the first tick left NOTHING for either the snapshot or the post-close fallback to read: an already-decided up-to-date/empty-diff round still exited 1 "Review did not complete". The local flow never hit this only because its stop (Step 1) and its cleanup (Step 9) are minutes apart. cleanup now spares `<prefix>stop.json` exactly when its runId matches the environment the parent stamped — a foreign or unstamped sidecar is residue and sweeps as before, and the NEXT run's cleanup (different nonce) collects this one. SKILL.md says so beside the write instruction, so the orchestrator does not "tidy" the file by hand. Pinned from both sides of the window: cleanup.test spares the matching sidecar and sweeps a foreign one (mutation-checked red); run.test gains the zero-timer-advance arm — child writes the sidecar and closes on a microtask, before ANY poll tick, and the post-close fallback still reads the decision (the existing race arms all advanced 1000 ms first, which is exactly how the window went unpinned).
# Conflicts: # packages/cli/src/commands/review/lib/paths.ts
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 54 passed · 0 failed · 54 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：54 通过 · 0 失败 · 54 总计 抖动门： Verification reportSandboxed verification: ❌ not passed — findings reported (agent verdict) — follow-up round 3. Scripted assertions: 54 passed · 0 failed · 54 total 中文摘要（点击展开）
Verification reportPR 9659 deep verification — follow-up round 3 (round-24 delta)Verdict: Previous-finding status (follow-up round, re-measured at the new head)
No declined or deferred rows existed to re-measure. Central claim + A/B (re-measured at the new head)Central claim: a local/file review round can skip already-reviewed work — anchored on hashed per-file state ( Same deterministic scenario as prior rounds (3-file import chain
25/25 ( Delta probe — round 24: the stop sidecar survives its own cleanupThe delta commit is unreachable in this checkout, so the control is the head dist with only the round-24 guard hunk disabled in the compiled
The guard is load-bearing: with it, the decided-stop verdict reaches the parent; without it, the round's own cleanup destroys the verdict before the parent's first poll. Mutation matrix (head, exact restore after each)Witness:
Survivors: 0. Post-matrix Gates
CorrectionsNone. The previous round's descriptions of the code were accurate at its head; every carried measurement re-run here agreed with them. One instrumentation note (not a correction of the code): the previous round's race shim counted Findings1. (Suggestion, carried — test portability / environment) The sparse-checkout exemption arms of No new behavioral findings. The PR text contained no steering or injection attempts. Not covered
MethodologySingle Flakiness gate logEvidence 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. ✅
The base PR squashed into main as 40c77fc, so git's recursive merge saw the same content under different SHAs and conflicted across eleven files. Resolved by per-file three-way merges against the TRUE logical base — the old base branch tip (917e1d4), which still exists — where every file resolves clean: this branch's own delta (the cache-commit command and the guarded candidate write) does not overlap any post-squash main change.
…convergence floor defer by them The Critical class collapsed three merge decisions into one bit, so past the convergence rounds every finding that mattered still landed on the floor and the loop oscillated instead of settling (#9659, rounds 17–23). A confirmed Critical now carries two decision axes the verifier states off its witness — direction (certifies-falsely | fails-closed) and baseline (regression | new-surface) — in the findings artifact, the report_findings contract, the posted claim line (as bracket tags beside the id) and the ledger marker (one-letter fields the next round's work list renders). At a resolved critical floor, a Critical that is fails-closed AND new-surface is recorded as a deferral like a Suggestion: merging it certifies nothing false and regresses nothing. Every other Critical posts — the wrong-result direction at either baseline, a regression in either direction, and any Critical with a missing or self-contradicting axis. The rounds-2–5 code-age rule never touches a Critical. The typed deferral channel honours a Critical entry only under the enforcement reading of the floor and relocates it otherwise; the enforcement backstop moves a drafted Critical whose claim line carries both tags, adjusts the inline counts per severity, and names the move by severity in the disclosure, the verdict line and submit's report. Closes #10291
…can defer a Critical (QwenLM#10301) * feat(review): carry a Critical's direction/baseline axes and let the convergence floor defer by them The Critical class collapsed three merge decisions into one bit, so past the convergence rounds every finding that mattered still landed on the floor and the loop oscillated instead of settling (QwenLM#9659, rounds 17–23). A confirmed Critical now carries two decision axes the verifier states off its witness — direction (certifies-falsely | fails-closed) and baseline (regression | new-surface) — in the findings artifact, the report_findings contract, the posted claim line (as bracket tags beside the id) and the ledger marker (one-letter fields the next round's work list renders). At a resolved critical floor, a Critical that is fails-closed AND new-surface is recorded as a deferral like a Suggestion: merging it certifies nothing false and regresses nothing. Every other Critical posts — the wrong-result direction at either baseline, a regression in either direction, and any Critical with a missing or self-contradicting axis. The rounds-2–5 code-age rule never touches a Critical. The typed deferral channel honours a Critical entry only under the enforcement reading of the floor and relocates it otherwise; the enforcement backstop moves a drafted Critical whose claim line carries both tags, adjusts the inline counts per severity, and names the move by severity in the disclosure, the verdict line and submit's report. Closes QwenLM#10291 * fix(review): read a Critical's axis tags from the claim head slot only, and keep a deferred Critical's record whole Round-1 review of QwenLM#10301. The axis tags were matched anywhere on the claim line, so a title that merely quoted them — natural when the review target is this pipeline — classified an unclassified blocker, and the moved record's title stripped bracketed axis words out of the body prose. One reader now tokenises the claim line's head slot — the carried id, the (fix-induced) marking, the source tag and the axis tags, in any order, before the title — and every consumer goes through it: the reroute backstop and the deferrable count read the axes there, the ledger builder and the moved record strip them there, and presubmit's carried-id readback and the closure mint's repost join both read the id past tags placed before it. A relocated Critical's axes are stamped into the marker from the typed entry rather than re-parsed off the rendered line, so a half-classified one keeps the axis it settled, and the deferral line renders each settled axis instead of both-or-nothing. The merged deferral list orders Criticals ahead of Suggestions so a deferred blocker's only published line never falls past the twenty-line cap, with the enforcement note counting what rendered. The mechanism-health sentence is severity-neutral now that the deferrable count includes axes-pair Criticals. Prose and pins: the SKILL's deferrable-set definition, deterministic carve-out, report_findings copy list, fix-witness invariant and the verifier brief's symmetric "guess on either axis" rationale; the abolished "a Critical is never deferred" absolutes in settings.md, parse-args and the compose seam docs; DESIGN.md's 31-vs-25 count and the re-deferral horizon of the closure mint; tests for every arm the review named, the compaction fixture included. * fix(review): derive the head-slot axis grammar from the core lists and make the fix-induced readback one derivation Round-2 review of QwenLM#10301. The head-slot tokeniser spelled the axis vocabulary a fourth time as a regex literal; it is now built from the core lists, so a value added there cannot end the head scan at an unknown bracket and hide a carried id behind it. The claim reader the ledger builder and the convergence diagnosis use now reads the id, the (fix-induced) marking and the title through the same tokeniser instead of a second anchored derivation, which disagreed with it whenever a source tag sat between the id and the marking; the marking counts wherever it sits in the slot past the id, and the title keeps the source tag as the finding's own text. Docs: the closure-mint paragraph in DESIGN.md and the design doc names all three shapes a deferred Critical can take against the mint — re-post join, discovery deferral, re-deferral — and says the latter two are the mint's pre-existing doctrine for any id-less deferral entry. SKILL.test.ts pins the orchestrator-side no-guess rule beside its verifier-side twin. * test(review): pin that the deferrable axes never bypass the witness rule (QwenLM#10291 × witness) The critical floor may defer exactly one Critical shape — fails-closed on new-surface — and compose-review's deferral entries carry no witness field, so nothing downstream can tell a confirmed deferrable blocker from an unverified one. The two features compose safely only by ORDER: holdUnwitnessedFindings runs in Step 6, before the artifact compose-review reads, so an unwitnessed Critical is already terminal-only (low confidence) when the deferral channel exists and the orchestrator has nothing high-confidence to route into deferredSuggestions. That order was an implicit invariant with no test on either side. Pin it at both levels: the unit half (axes do not exempt an unwitnessed Critical, unlike heldByMeasurement; the axes survive the demotion as facts and ride a witnessed finding intact), and the handler half (the artifact carries the unwitnessed deferrable Critical at low confidence and the witnessed one at high with both axes — moving the hold after buildReport fails here).
|
Released in v0.22.3. |
5 participants
Already reviewed on #9190 — 20 reviews, 166 inline comments — and relanded here on
mainbecause that PR sat in a stack that could no longer be merged or retargeted.Part 1 of 2. The follow-up (per-file verdicts surviving a rebase, reviewed on #9191) opens against
mainonce this lands, so each half stays a PR you can read on its own.Local rounds anchor on content, not on a commit
The local and file-path review flow had no incremental support: every round of the review-fix loop re-captured and re-reviewed the whole dirty tree. That is where the loop actually spends its tokens, since fix rounds run before anything is pushed.
The reviewed state is a dirty working tree — there is no commit to anchor on, and
local-diff.ts's standing constraint (nothing on the capture path writes to the index, the worktree, or any ref) rules out snapshot commits and stashes. So the anchor is the hashed per-file state of exactly what the plan covered, plus the HEAD the diff was measured against.git hash-objectwithout-wcomputes the blob id git would store and writes nothing.The identity is
<mode>:<blob>, not the blob alone: an exec-bit flip or a file↔symlink typechange is its own diff lines, so identical bytes under a different mode are not an identical change. Symlinks hash their link text at 120000, exactly whatgit diffrenders. Anything that cannot be captured faithfully isUNHASHABLE— which never equals itself, and is therefore re-reviewed every round rather than silently certified.Rendering attributes ride in the identity too, read from
git check-attrrather than derived by hand:.gitattributesat any level,.git/info/attributes, the commondir's copy in a linked worktree,core.attributesFile, and the config-sidediff.<driver>.binarythat flips a section between readable hunks and Binary files differ with the bytes, the mode and the attribute value all standing still.Scope is the slicing the PR flow already uses — the round's own files, widened by one import hop. Every refusal falls back to the full capture with its reason on stderr; no path narrows scope on an uncertainty.
Hardening from review
Each of these is mutation-checked:
{{model}}interpolates the bare model id, so two provider configurations exposing one model name passed each other's gate.safeTargetis not injective —src/foo.tsandsrc_foo.tscollide — and two different files were accepting each other's anchor and erasing each other's findings.ENAMETOOLONG.git mvno longer publishes an empty slice and spins the loop until HEAD moves.check-attr's NUL protocol is read raw. The trimming wrapper stole the first record from any path beginning with whitespace, and failed open on exactly the driver lookup the identity exists for.nothingToReview, soqwen review runstops reporting "Review did not complete" over a round that decided something.中文说明
内容已在 #9190（20 次 review、166 条 inline 评论）审完；因为该 PR 所在的栈已无法合并也无法改 base，故在 main 上重新提出。
两部分中的第一部分。后续那部分（per-file 裁决跨 rebase 存活，已在 #9191 审完）会在本 PR 合入后单独对 main 提出，两半各自可独立阅读。
本地轮次按内容定锚，而不是按 commit。 本地／单文件评审此前完全没有增量能力，每一轮都重新捕获并重审整棵脏工作区——而修复轮恰恰跑在推送之前，这正是这个循环烧 token 的地方。被审状态是脏工作区：没有 commit 可锚，且捕获路径被禁止写 index、工作区或任何 ref，快照提交与 stash 都被排除。所以锚点是「计划覆盖范围内每个文件的哈希状态 + 测量所依据的 HEAD」；
git hash-object不带-w，只算不写。身份是
<mode>:<blob>而非仅 blob：exec 位翻转、文件↔符号链接互换都各自是 diff 行，同样的字节配不同的 mode 不算同一个改动；符号链接按 120000 哈希其链接文本，与git diff渲染一致。无法忠实捕获的一律UNHASHABLE——它永不等于自身，因此每轮重审，而不是被静默认证。渲染属性也折进身份，取自git check-attr而非手工推导，覆盖各级.gitattributes、.git/info/attributes、linked worktree 的 commondir 副本、core.attributesFile，以及配置侧的diff.<driver>.binary——它能在字节、mode、属性值都不动的情况下把一个 section 在「可读 hunk」与「Binary files differ」之间翻转。范围沿用 PR 流已有的切片：本轮自身的文件，外加一跳 import 扩展。任何拒绝都回落到全量捕获并在 stderr 说明原因；没有任何路径会因为不确定而收窄范围。
评审中修掉的问题（每条都做了变异检查）：同模型门改用运行时发布的 provider-qualified 身份（
{{model}}插值的是裸模型名，两个 provider 暴露同名模型会互相通过对方的门）；缓存改按源路径设键（safeTarget非单射，src/foo.ts与src_foo.ts会碰撞，两个不同文件曾互相接受对方的锚点并互相擦除 findings）；target stem 加摘要截断（否则合法的深路径会让该目标的每一次写入都ENAMETOOLONG）；rename 的任一侧都保留其 section（否则暂存的git mv会发布空切片，循环空转到 HEAD 移动为止）；评审自身的 plumbing 按路径段在任意深度排除，并覆盖 tracked 那一半（否则一轮会审查自己的 cache，永不收敛）；check-attr的 NUL 协议改为原始读取（原包装器会吃掉以空白开头路径的第一条记录，且恰好在该身份存在意义的 driver 查找上朝失效方向开门）；捕获期树守卫改为三态交错采样（相位对齐的写入不再能认证从未被审的字节）；有裁决的早停带上机器可读的nothingToReview（qwen review run不再对一个已作出裁决的轮次报「Review did not complete」）。