fix(review): gate the recovered incremental anchor on the model that certified it - #9184
Conversation
…certified it Incremental scoping is a same-model contract: "clean up to this commit" is one model's verdict. The cache path has always enforced it through lastModelId, but the anchor recovered from the posted review's ledger marker shipped bare, so a round run under a different model would scope sha..HEAD past code the current model never reviewed — permanently, since each clean round re-anchors past the last. The marker now carries the certifying model beside the anchor, riding and falling with it: withheld on fail-closed and truncated rounds, and dropped by the parser when the sha beside it did not survive. The recovered-ledger context section names the model and instructs the gate (absent counts as a mismatch — markers predating the field), and the skill's incremental check requires a model match on both the cache path and the marker-recovery path before scoping to the interdiff. The findings work list still carries across models — every entry is re-asserted against the code — only the anchor does not.
|
Re-run on @wenshao's Template looks good ✓ Problem: real, and demonstrated two ways. First, in the base-branch code: on Direction: aligned. This hardens Size: 604 production logic lines, 1587 test lines, 31 lines of docs and bundled-skill text. Production: Approach: scope grew from the original ~111 production lines because each review round demonstrated a new channel of the same class — the bare-id comparison in prompt text, the blanked runtime channel, round-start drift, the side-file divergence — and each addition closes one demonstrated hole, none speculative. The final commit is the right endpoint: it moves the last identity comparison out of prompt text into the CLI, and the skill test now asserts the absence of comparison wording, closing the class by construction rather than by another point fix. No drive-by changes. Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明应 @wenshao 的 模板完整 ✓ 问题：真实存在，且有两重证明。其一，base 分支代码可直接观察到： 方向：对齐。这是对 规模：604 行生产逻辑、1587 行测试、31 行文档与 bundled skill 文本。生产代码： 方案：范围从最初约 111 行生产代码增长，是因为每轮 review 都实证出同一类问题的一个新通道——prompt 文本中的裸 id 比较、被置空的运行时通道、轮次开始后的身份漂移、side file 分歧——每次增量都堵住一个被实证的洞，没有投机性改动。最后一个提交是正确的终点：把最后一处身份比较从 prompt 文本移入 CLI，且 skill 测试现在断言比较措辞的缺席，从构造上关闭整类问题，而不是再打一个点补丁。无顺手改动。 风险：无升级风险信号——改动文件均未命中与 revert 相关的路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal before going line-by-line: (1) qualify the marker's anchor with the model that certified it, (2) make the pair share one lifecycle — anywhere the anchor is withheld or fails validation, the model goes with it, (3) take the certifying identity from something a model cannot forge — the runtime-injected env, not the model-written state field, (4) gate both recovery routes on it in-process, absent counting as mismatch, (5) propagate it through the side file automatically. The PR matches that on every point and goes three legitimate steps further: a provider-qualified identity ( What I verified against head
Remaining open items — all Suggestion-level, none blocking: three prose locations still name Testing evidence — this is an unattended CI run, so nothing here was executed by triage; the evidence is the PR's own CI, read through the API: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 No red checks at the time of writing. The one to watch is Sandboxed verification would settle what the unit suite cannot: 中文说明代码审查逐行阅读前的独立方案：(1) 让 marker 的 anchor 携带认证它的模型；(2) 二者共存亡——anchor 被扣下或校验失败的任何地方，model 一并扣下；(3) 认证身份取自模型无法伪造的来源——运行时注入的环境变量，而非模型填写的 state 字段；(4) 两条恢复路径都在进程内对其做闸门判定，缺失按不匹配处理；(5) 自动传递到 side file。本 PR 每一点都与该方案吻合，并合理地多走了三步：provider 限定的身份（ 在 head
其余未决项——全部为 Suggestion 级，均不阻断：三处行文仍以 测试证据——这是无人值守的 CI 运行，triage 未执行任何 PR 代码；证据为 PR 自身的 CI，经 API 读取。写入时没有任何红色检查；需关注的是仍在运行的 沙盒验证可以补足单测无法覆盖的部分： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review, capped by policy: the Stage 0 core-size awareness flag applies (604 production lines touching core paths), and that cap drives the action, not doubt about the diff. Stepping back: against my independent proposal this PR matches on every point and goes three legitimate steps further — provider-qualified identity, the round-start drift gate, and every identity comparison moved out of prompt text. The two round-12 Criticals that the standing CHANGES_REQUESTED review rests on are closed at this head; I verified both fixes in the code, and each has a dedicated test pinning the exact scenario. Every Critical raised across the twelve rounds is resolved or substantively closed at this commit — R12-1 and R12-2 by the final commit, R9-3 by the wiring-test hygiene fix, R2-9/R1-3 by the maintainer's own decision to implement the richer identity. What remains is Suggestion-level doc wording and a recorded probe list — follow-up material. Six months from now this reads as careful, fail-safe engineering: every unknown degrades to reviewing more, never to skipping. Why defer instead of approve: this run's Stage 0 raises the 500+ production-line core-touch advisory, and an escalated PR caps at 3/5 with the decision handed to a maintainer. The flag is a formality here — the author is the maintainer this gate would escalate to, and nobody has engaged this PR more deeply — but the cap exists so the bot does not auto-approve a 600-line core change on its own authority, and this run will not be the one to decide the rule doesn't apply. CI on the reviewed head is also still running (the ubuntu unit suite), so there is no approve-on-green marker either — an approval should land on green, not in flight. @wenshao — your call, and it should be an easy one: sign off here (or a maintainer approval on the PR) and this is ready to merge once CI lands green. The open nits (R9-11 doc wording in three places, the R9-8 tautology, the R9-12 quoted notice, the J3 pin) and the eight round-12 probes are fine as follow-ups; none of them gate the merge in my reading. 中文说明置信度：3/5——审查是干净的，压分来自政策：Stage 0 的核心规模提示适用（604 行生产逻辑触及核心路径），该上限驱动行动，而非对 diff 有疑问。 退一步整体看：对照我的独立方案，本 PR 每一点都吻合，并合理地多走了三步——provider 限定的身份、轮次开始的漂移闸门、以及把所有身份比较移出 prompt 文本。支撑当前 CHANGES_REQUESTED 状态的第 12 轮两个 Critical 已在本 head 上关闭；我在代码中核实了两处修复，且各有一个专门测试钉死对应场景。十二轮中提出的每一个 Critical 都已在此 commit 上解决或实质关闭——R12-1 与 R12-2 由最后一个提交关闭，R9-3 由接线测试的环境清理修复，R2-9/R1-3 由维护者亲自选择实现更丰富的身份而关闭。剩下的只有 Suggestion 级的行文措辞与一份记录在案的探针清单——属于跟进事项。六个月后回头看，这是一份谨慎、fail-safe 的工程：任何未知都退化为"多审"，绝不"跳过"。 为什么转交而非批准：本轮 Stage 0 触发了 500+ 生产行核心触及提示，被升级的 PR 封顶 3/5、决定权交给维护者。这个提示在此只是形式——作者就是这道闸门要转交的那位维护者，没有人比 ta 更深入地参与了本 PR——但该上限的意义正在于：机器人不凭自身权威自动批准一个 600 行规模的核心改动，本轮也不会擅自判定规则不适用。受审 head 上的 CI 也还在运行（ubuntu 单测套件），因此也不发 approve-on-green 标记——批准应落在绿色之后，而不是飞行途中。 @wenshao——由你拍板，而且应该很轻松：在此确认（或在 PR 上给出维护者批准），CI 转绿后即可合入。未决的小项（三处 R9-11 行文、R9-8 同义反复、R9-12 引用的提示、J3 钉死）以及第 12 轮的八条探针都可以作为跟进；按我的判断，它们都不构成合入闸门。 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 4": none — no checks were cut short..
中文说明
未探索到全部深度（达到工具调用预算）："agent 4"：none — no checks were cut short.。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const model = | ||
| typeof input.modelId === 'string' && input.modelId.trim() !== '' | ||
| ? input.modelId.trim() | ||
| : undefined; |
There was a problem hiding this comment.
[Critical] R1-9: With review.attribution: false the visible footer is withheld, but this new marker field still publishes the exact model id in the posted review body — violating the setting's documented contract ("whether the posted review names its model", review-settings.ts; user docs: "Disable to post reviews without AI attribution"). composeReview calls ledgerMarkerFor(input, result.cappedBy) without attribution (line 484), and this extraction reads input.modelId unconditionally. Pre-PR markers carried findings/round/sha and no model identity, so this diff newly leaks exactly the information the setting removes, into a write this module calls public and irreversible. The module's own standard indicts it: forged footers are stripped from inline comments even with attribution off (submit.ts:154-156). — Failure scenario: an operator sets review.attribution: false (reviews post under a shared bot identity) → the footer is suppressed everywhere visible, but anyone with API access (gh api repos/…/pulls/…/reviews) or the raw-body edit view reads the exact model id out of the posted <!-- qwen-review-ledger {…,"model":"<exact model id>"} -->.
Witness (probe, both arms):
attribution=false →
markerLine: <!-- qwen-review-ledger {"v":1,"round":1,…,"sha":"deadbeef00112233","model":"test-model"} -->
footerPresent: false, rawModelIdPresent: true
attribution threaded into ledgerMarkerFor →
no "model" key, rawModelIdPresent: false, sha intact
Suggested fix (verified by the probe flip): thread attribution into ledgerMarkerFor and withhold model when it is off — the anchor then degrades to the skill's already-specified "absent model → mismatch → full-range" fail-safe; alternatively, document a marker exemption in the setting contract and the DESIGN bullet (today neither says so):
// compose-review.ts:484
ledgerMarkerFor(input, result.cappedBy, attribution)
// ledgerMarkerFor
const model =
attribution &&
typeof input.modelId === 'string' && input.modelId.trim() !== ''
? input.modelId.trim()
: undefined;中文说明
在 review.attribution: false 时,可见的 footer 被正确扣下,但新增的 marker model 字段仍会把确切的模型 id 发布到已提交的 review 正文中——违反了该设置的文档契约("发布的 review 是否注明其模型",review-settings.ts;用户文档:"禁用以发布不带 AI 署名的审查")。composeReview 调用 ledgerMarkerFor(input, result.cappedBy) 时未传 attribution(第 484 行),此处无条件读取 input.modelId。本 PR 之前的 marker 只携带 findings/round/sha、不含模型身份,因此本 diff 新泄露了该设置恰恰要移除的信息,且写入是本模块所称的"公开、不可逆"的。模块自身的标准也印证这一点:即使 attribution 关闭,伪造的 footer 也会从行内评论中被剥掉(submit.ts:154-156)。 — 失败场景:运维者设置 review.attribution: false(以共享机器人身份发布审查)→ 所有可见处的 footer 都被抑制,但任何有 API 访问权限的人(gh api repos/…/pulls/…/reviews)或原始正文编辑视图都能从已发布的 <!-- qwen-review-ledger {…,"model":"<确切模型 id>"} --> 中读出确切的模型 id。
见证(probe,双向验证):attribution=false → marker 行包含 "model":"test-model",footerPresent: false,rawModelIdPresent: true;将 attribution 传入 ledgerMarkerFor 后 → 无 "model" 键,rawModelIdPresent: false,sha 不受影响。
建议修复(已由 probe 翻转验证):将 attribution 传入 ledgerMarkerFor,关闭时扣下 model——anchor 随即退化为 skill 已规定的"模型缺失 → 不匹配 → 全量审查"的 fail-safe;或者在设置契约与 DESIGN 条目中明确声明 marker 豁免(目前两处都未说明)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): if `.qwen/review-cache/pr-<n>.json` exists, read it **in the same response as the fetch report** — both are `read_file`, genuinely parallel — for `lastCommitSha` and `lastModelId`. Compare to `fetchedSha` from the fetch report and the current model ID (`{{model}}`): | ||
| - If SHAs differ → continue with the worktree just created. Compute the incremental diff (`git diff <lastCommitSha>..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **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. | ||
| - If SHAs differ **and** model matches → continue with the worktree just created. Compute the incremental diff (`git diff <lastCommitSha>..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **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. | ||
| - If SHAs differ **but** model differs → continue with the worktree, but the scope is the **full diff**, never `<lastCommitSha>..HEAD`: "clean up to `lastCommitSha`" is {cached_model}'s verdict, and an incremental range scoped to another model's anchor leaves everything before it reviewed by no run of `{{model}}` — permanently, because this round's own cache write would re-anchor past it. Inform: "Previous round was reviewed by {cached_model}. Running full review with {{model}}." Still read the cache's `findings` ledger and owe each entry its Step 6 ruling — the work list carries across models (every entry is re-asserted against the code); only the anchor does not. |
There was a problem hiding this comment.
[Suggestion] R1-1: The two-anchor fallback is asymmetric: when the side file's preferred anchor fails the model gate, the cache's same-model anchor is an explicit fallback (the recovery paragraph's parenthetical), but when the CACHE's anchor fails the model gate, the side file's same-model anchor is never consulted — the recovery paragraph only opens "when the cache is absent or its lastCommitSha was rebased away". — Failure scenario: a machine that ran the last review (cache present) switches models between rounds; lastCommitSha differs from fetchedSha and lastModelId differs from the running model, while the PR's last posted review (e.g. posted by CI running the CURRENT model) carries a same-model anchor in the side file → this branch mandates the full diff and the recovery paragraph's entry condition excludes this case, so the valid same-model anchor is never read: one wasted full review per model switch on cache-owning machines — exactly the cost the recovered-anchor feature exists to avoid (fail-safe direction; self-heals after Step 8 rewrites the cache).
Suggested fix: in this branch, before falling back to the full diff, consult the side file's anchor under the same gate as the reverse-direction fallback (valid sha, ancestry checks, model == {{model}}) and scope to it when it passes.
中文说明
双 anchor 回退不对称:当旁路文件中被优先的 anchor 未通过模型闸门时,缓存中同模型的 anchor 是显式回退(恢复段落的括注);但当缓存的 anchor 未通过模型闸门时,旁路文件中同模型的 anchor 却永远不会被参考——恢复段落的进入条件是"缓存不存在或其 lastCommitSha 已被 rebase 掉"。 — 失败场景:运行过上一轮审查的机器(缓存存在)在两轮之间切换了模型;lastCommitSha 与 fetchedSha 不同且 lastModelId 与当前模型不同,而 PR 最近一次发布的审查(例如由运行当前模型的 CI 发布)在旁路文件中携带同模型的 anchor → 本分支强制全量 diff,且恢复段落的进入条件排除了这种情况,于是有效的同模型 anchor 永远不会被读取:缓存存在的机器上每次切换模型都浪费一次全量审查——恰恰是恢复 anchor 特性本要避免的成本(方向 fail-safe;Step 8 重写缓存后自愈)。
建议修复:在该分支中,退回全量 diff 之前,先以与反向回退相同的闸门(有效 sha、祖先检查、model == {{model}})参考旁路文件的 anchor,通过则按其划分范围。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| sha: 'abc1234def56789', | ||
| model: 'qwen3.7-max', |
There was a problem hiding this comment.
[Suggestion] R1-2: The recovery seam this PR's payoff depends on — posted marker → latestOwnLedger → the prev-ledger side file Step 1's gate reads — is pinned for sha only by the existing test 'carries the anchor sha through the recovery seam intact' (line 1039, whose comment exists precisely for this hazard); the new model field rides the same seam untested. — Failure scenario (mutation probe, executed): projecting latestOwnLedger's return onto known fields without model left all 380 existing tests green → in production the side file always lacks model, Step 1's gate reads every recovered anchor as absent-model mismatch, and incremental scoping from recovered markers silently never fires — every fresh-environment review degrades to full-range with no visible error (fail-safe direction, but the feature ships inert).
Suggested fix: add model: 'qwen3.7-max' to the anchored ledger in 'carries the anchor sha through the recovery seam intact' (line 1039) so the toEqual pin covers the new field beside the sha.
中文说明
本 PR 收益所依赖的恢复接缝——已发布 marker → latestOwnLedger → Step 1 闸门读取的 prev-ledger 旁路文件——目前只由现有测试 'carries the anchor sha through the recovery seam intact'(第 1039 行,其注释正是为这一风险而写)为 sha 钉住;新增的 model 字段经过同一接缝却未被测试。 — 失败场景(已执行的突变 probe):将 latestOwnLedger 的返回值投影为不含 model 的已知字段后,现有 380 个测试全部保持绿色 → 生产中旁路文件将永远缺少 model,Step 1 的闸门会把每个恢复的 anchor 判为"模型缺失 = 不匹配",从已发布 marker 恢复的增量范围划分将静默地永不生效——所有全新环境的审查都退化为全量,且没有任何可见错误(方向上是 fail-safe,但功能等于带着失效上线)。
建议修复:在 'carries the anchor sha through the recovery seam intact'(第 1039 行)的 anchored ledger 中加入 model: 'qwen3.7-max',使 toEqual 钉住新字段与 sha 并列。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const model = | ||
| typeof input.modelId === 'string' && input.modelId.trim() !== '' | ||
| ? input.modelId.trim() | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] R1-3: The attribution-off branch of this new guard — the branch its own comment says exists ("the boundary validation of modelId runs only when attribution is on, and the marker rides either way") — is exercised by no test; a mutation deleting the typeof check survives the whole suite. — Failure scenario (mutation probe, executed): deleting the typeof check left 380/380 tests green; in production, an operator runs with review.attribution: false and the state JSON omits modelId (legal — attribution off skips the modelId is required validation) on a clean round with fetchedSha set → input.modelId.trim() throws inside ledgerMarkerFor's try → returns null → the ENTIRE marker is silently dropped, not just the model: the round counter resets (the next round re-issues ids the PR already carries) and the findings work list is lost. Every marker-path test passes a string modelId, and the attribution-off tests use a plan with no prNumber, so this guard is never reached.
Suggested fix: add a marker-path test with attribution off and missing modelId — a covered plan with prNumber + fetchedSha, compose with attribution false, then assert parseLedger(r.body)!.sha is the fetched sha and ledger.model is toBeUndefined(), pinning both that the marker survives and that the model is withheld. (Pairs naturally with the R1-9 fix.)
中文说明
新增守卫的 attribution 关闭分支——其自身注释声称存在的分支("modelId 的边界校验只在 attribution 开启时运行,而 marker 无论如何都会挂载")——没有任何测试覆盖;删除 typeof 检查的突变体能在整个测试套件中存活。 — 失败场景(已执行的突变 probe):删除 typeof 检查后 380/380 测试仍全绿;生产环境中,运维者以 review.attribution: false 运行,且 state JSON 省略了 modelId(合法——attribution 关闭时跳过 modelId is required 校验),此时是带有 fetchedSha 的干净轮次 → input.modelId.trim() 在 ledgerMarkerFor 的 try 内抛错 → 返回 null → 整个 marker 被静默丢弃,而不仅仅是 model:轮次计数器重置(下一轮会重新签发本 PR 已携带的 id),findings 工作清单丢失。所有 marker 路径的测试都传入了字符串 modelId,而 attribution 关闭的测试使用的 plan 没有 prNumber,因此永远不会到达这个守卫。
建议修复:增加一个 attribution 关闭且缺失 modelId 的 marker 路径测试——使用带 prNumber + fetchedSha 的 covered plan,以 attribution false 组装,然后断言 parseLedger(r.body)!.sha 为 fetched sha 且 ledger.model 为 toBeUndefined(),同时钉住"marker 存活"与"model 被扣下"两点。(与 R1-9 的修复天然配套。)
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - **Own-account, latest round only.** The ledger claims "these are the findings the previous /review stood behind", and only this account's reviews can make that claim — another user's marker is data about _their_ tooling. Each posted round embeds a fresh full copy, so the newest marker is the whole state. | ||
| - **Data, not authority.** Every recovered entry is owed a Step 6 ruling against the code — the ledger routes work, it never rules. A tampered or stale marker therefore costs a few wasted rulings, not a wrong verdict, which is why parsing is fail-quiet and the round number feeds `compose-review` from a CLI-written side file rather than a model's memory. | ||
| - **Medium reads, high writes.** Recovering the ledger is free (the reviews were already fetched), so the default-effort re-review finally opens like a round-2 comment; the cache write and the posting that carries the marker keep their existing effort gates untouched. | ||
| - **The anchor names its model.** Incremental scoping is a same-model contract — "clean up to `sha`" is one model's verdict, and the cache path has always enforced that through `lastModelId` (a same-SHA re-run under a different model gets a full second opinion, not a skip). The marker's anchor shipped bare, so the recovery path had no way to honour the contract: a round run under model B that recovered model A's anchor would scope `sha..HEAD` past code B never reviewed — permanently, since each clean round re-anchors past the last. The marker now carries `model` beside `sha`, riding and falling with the anchor (withheld on fail-closed and truncated rounds alike, dropped by the parser when the sha beside it did not survive), and Step 1's recovered-anchor gate requires it to match the running model — absent, on markers from before the field, counts as a mismatch: the work list still carries (rulings re-assert against the code, so they cross models safely), only the anchor does not. |
There was a problem hiding this comment.
[Suggestion] R1-4: This added design bullet overstates pre-change behavior: "the cache path has always enforced that through lastModelId" — but the pre-change cache path enforced the same-model contract only in the same-SHA (skip) case; its SHAs-differ branch had no model condition until this very PR adds one (verified via git show at the merge base; the diff's own deleted SKILL.md line proves it). — Failure scenario: a maintainer relying on this bullet would believe (a) the only pre-change hole was the marker path and (b) legacy cache anchors already satisfy the same-model contract → the exact inference that would let a tainted legacy cache anchor keep scoping incremental rounds past code the current model never reviewed, since unlike bare-model markers (which the new absent=mismatch rule heals via full review) nothing heals a tainted cache entry.
Suggested fix: reword to say the cache path paired its anchor with lastModelId but enforced the contract only in the same-SHA skip case, and that this change extends the enforcement to the differ-SHA incremental branch and to the recovered-anchor path.
中文说明
新增的设计条目夸大了改动前的行为:"缓存路径一直通过 lastModelId 强制执行该契约"——但改动前的缓存路径只在同 SHA(跳过)情形下强制执行同模型契约;其 SHA 不同的分支在本 PR 之前没有任何模型条件(已用 git show 在 merge base 上验证;diff 自己删除的 SKILL.md 行即为证据)。 — 失败场景:依赖此条目的维护者会认为 (a) 改动前唯一的漏洞是 marker 路径,且 (b) 遗留缓存 anchor 已满足同模型契约 → 恰恰是让被污染的遗留缓存 anchor 继续把增量轮次的范围划到当前模型从未审查过的代码的推理;因为不像缺少模型的 marker(新的"缺失=不匹配"规则会通过全量审查自愈),被污染的缓存条目没有任何自愈机制。
建议修复:改为"缓存路径将其 anchor 与 lastModelId 配对,但此前仅在同 SHA 跳过情形下强制执行契约;本改动将强制执行扩展到 SHA 不同的增量分支与恢复 anchor 路径"。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| * beside it did not survive — a model naming no range qualifies nothing. | ||
| */ | ||
| model?: string; |
There was a problem hiding this comment.
[Suggestion] R1-5: Test-efficacy probe (harness validated): reverting this model?: string declaration hunk on its own left every affected test green — vitest transpiles without type-checking, so if the declaration were deleted or renamed, the entire unit suite still passes; the dangling ledger.model/payload.model references are caught only by the separate tsc typecheck step. — Concrete cost: a refactor that drops or renames the field turns no test red — only npm run typecheck notices. (Mitigating: the field's runtime read/write hunks — cap constant, serializer, parser — were all killed by probes, and ledger.test.ts gates the full-file revert.)
Suggested fix: confirm typecheck is relied upon as the gate for the declaration hunk (it runs in CI), or add a type-level pin so its removal reddens a test.
中文说明
测试有效性 probe(harness 已验证):单独回退这个 model?: string 声明 hunk 后,所有受影响的测试仍为绿色——vitest 转译时不做类型检查,因此即使删除或重命名该声明,整个单元测试套件依然通过;悬空的 ledger.model/payload.model 引用只能由独立的 tsc typecheck 步骤捕获。 — 具体代价:删除或重命名该字段的重构不会让任何测试变红——只有 npm run typecheck 能发现。(缓解因素:该字段的运行时读写 hunk——上限常量、序列化器、解析器——均被 probe 杀死,且 ledger.test.ts 对整文件回退是有效的。)
建议修复:确认 typecheck 被作为该声明 hunk 的闸门(CI 中有运行),或增加类型层面的钉住,使其被移除时测试变红。
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Declined — with the confirmation this finding asked for recorded here.
The disposition: npm run typecheck (tsc, run in CI and re-run in this round) IS the relied-upon gate for the model?: string declaration hunk. Vitest transpiles without type-checking by design, so a type declaration's removal or rename reddens no unit test and is caught by the separate typecheck step — that is the intended division of labor, not a gap to close with a type-level pin. The finding's own mitigation confirms the behavioral surface is already pinned: the runtime read/write hunks (cap constant, serializer, parser) were all killed by its probes, and ledger.test.ts gates the full-file revert. Adding a compile-time assertion so a field deletion reddens a test too would duplicate what tsc already catches in the same CI run — defense a senior engineer would call unnecessary, and this PR's diff budget is better spent on findings with live failure modes. No code change this round.
中文说明
拒绝处理——并按本条发现的要求，在此记录确认结论。
结论：npm run typecheck（tsc，CI 中运行，本轮也已重新运行）就是 model?: string 声明 hunk 所依赖的闸门。Vitest 按设计转译时不做类型检查，因此删除或重命名一个类型声明不会让任何单元测试变红，而是由独立的 typecheck 步骤捕获——这是预期的职责分工，而不是需要用类型层面钉住来弥补的缺口。本条发现自身的缓解说明也确认行为面已被钉住：运行时读写 hunk（上限常量、序列化器、解析器）均被其 probe 杀死，且 ledger.test.ts 对整文件回退是有效的。再增加一个编译期断言、让删除字段也能让测试变红，只是重复 tsc 在同一次 CI 运行中已经捕获的东西——属于资深工程师会认为不必要的防御，本 PR 的 diff 预算更适合花在存在真实失败模式的发现上。本轮不做代码改动。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — packages/core test suite timed out locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI.
中文说明
未审查：build-and-test — packages/core test suite timed out locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| const model = ledger.model?.trim(); | ||
| if (model) payload.model = model.slice(0, LEDGER_MAX_MODEL); |
There was a problem hiding this comment.
[Critical] R1-1: Truncating the certifying model ID creates identity collisions in the recovered-anchor gate — Failure scenario: a model ID longer than 64 characters is serialized as its prefix; a distinct model whose full ID equals that prefix then compares equal and scopes to sha..HEAD, skipping code it never reviewed.
Witness:
PR code: {"longLength":75,"recoveredLength":64,"gateWouldMatch":true}
With truncation removed from both serializer and parser: {"recoveredLength":75,"gateWouldMatchPrefix":false}
Preserve the complete model identity. If it cannot fit, withhold the sha/model pair, or store and compare a collision-resistant digest of the complete identity.
中文说明
**问题：**认证模型 ID 被截断后会产生身份碰撞。
**失败场景：**长度超过 64 字符的模型 ID 会被序列化为前缀；另一个完整 ID 恰好等于该前缀的模型随后会被错误判断为同一模型，并使用 sha..HEAD，跳过它从未审查过的代码。
**建议修复：**保留完整模型身份；若无法完整存储，则同时扣下 sha 与 model，或保存完整身份的抗碰撞摘要。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| const model = | ||
| typeof input.modelId === 'string' && input.modelId.trim() !== '' | ||
| ? input.modelId.trim() |
There was a problem hiding this comment.
[Critical] R1-2: The certifying model is accepted from model-authored compose state instead of trusted runtime provenance — Failure scenario: a review running under model B can write model A's ID into compose state; the posted ledger then certifies the anchor as A, so a later real A run accepts sha..HEAD and skips earlier code A never reviewed.
Witness: not run — the defect is the absence of trusted runtime provenance at the JSON boundary; the caller-controlled field flows directly into the certificate without an independent value to compare against.
Inject the current model identity at a trusted CLI/session boundary, or compare the supplied value against that trusted identity and fail closed on disagreement.
中文说明
**问题：**认证模型来自模型自身编写的 compose state，而非可信运行时来源。
**失败场景：**实际由模型 B 执行的审查可以把 modelId 写成模型 A；ledger 会把 anchor 认证为 A，之后真正的 A 会接受 sha..HEAD，跳过 A 从未审查过的早期代码。
**建议修复：**在可信 CLI/session 边界注入当前模型身份，或将输入值与可信身份比较并在不一致时 fail closed。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
| const model = | ||
| typeof input.modelId === 'string' && input.modelId.trim() !== '' | ||
| ? input.modelId.trim() |
There was a problem hiding this comment.
[Critical] R1-3: A bare model ID is not a unique resolved-model identity across providers and endpoints — Failure scenario: two provider or endpoint configurations can expose the same model ID while resolving to different underlying models; the second configuration accepts the first configuration's anchor and skips code it never reviewed.
Witness: not run — a live two-provider setup was unavailable, but the repository's provider-aware identity code explicitly compares authType and baseUrl in addition to the model ID.
Persist and compare a stable resolved-model identity including the relevant auth type and provider or endpoint discriminator, and use the same identity for cache and posted-ledger gates.
中文说明
**问题：**裸模型 ID 无法在不同 provider 或 endpoint 间唯一标识解析后的模型。
**失败场景：**两个 provider/endpoint 可以暴露相同模型 ID、但实际解析为不同模型；第二个配置会接受第一个配置的 anchor，并跳过它从未审查过的代码。
**建议修复：**持久化并比较包含 authType 以及 provider/endpoint 区分信息的稳定解析模型身份，并让本地缓存与已发布 ledger 使用同一身份。
— gpt-5.6-sol via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Not resolved in this round — escalated for a maintainer's decision rather than silently picking a direction.
What landed instead: the marker's model is now the identity the runtime publishes (QWEN_CODE_MODEL, injected at both CLI boundaries from the environment Config keeps on the active model), never the model-written state field alone — so the posted certificate names exactly the identity the session itself reports as active, which closes the typed-identity hole (R1-2). But it is still the bare model id, so the cross-provider collision this finding names remains open in principle.
Why it is escalated rather than implemented: making the identity provider-aware spans subsystems this PR does not touch, with a real tradeoff on a public surface. (1) The identity is decided at the publisher (publishModelEnv in core config) — enriching it there changes what every consumer of QWEN_CODE_MODEL sees. (2) The same-model gate is enforced by the skill comparing against {{model}}, and the cache path's lastModelId (model-written at Step 8) has the same bare-id limitation — pre-existing, and per this finding's own "same identity for both gates" requirement both would have to move together. (3) The marker rides a PUBLIC, irreversible comment: the obvious discriminators (baseUrl, endpoint URL) can be private infrastructure, so a provider-aware posted identity needs a sanitized format — a design choice, and the local cache and the posted marker may not be able to share one.
Options as I see them: (a) accept the bare runtime-published id as this PR's identity contract and record the multi-provider limitation in DESIGN.md; (b) track a follow-up that enriches the identity at the publisher (authType-qualified selectors already exist in the runtime, e.g. getFastModel) and defines a public-safe provider discriminator for the marker; (c) redesign identity inside this PR — I would advise against it, since the cache format, the skill's gate instructions, and the marker format would all change together, far beyond this diff's footprint.
Question for the maintainer: is the bare runtime model id acceptable as the same-model identity for this PR, with provider-aware identity tracked as a follow-up — or should this PR wait for the richer identity?
中文说明
本轮未解决——此处需要维护者拍板，因此选择升级上报，而不是悄悄选定某个方向。
本轮实际落地的内容：marker 的 model 现在取运行时发布的身份（QWEN_CODE_MODEL，由两个 CLI 边界从 Config 始终指向当前活跃模型的环境变量中注入），不再仅取模型自己编写的 state 字段——因此已发布的证书所载身份与会话自身上报的活跃身份完全一致，堵住了"自报身份"的漏洞（R1-2）。但它仍是裸模型 id，所以本条指出的跨 provider 碰撞在原则上依然存在。
升级上报而非直接实施的原因：让身份感知 provider 会波及本 PR 未触及的子系统，且在公开表面上存在真实取舍。(1) 身份由发布端决定（core config 中的 publishModelEnv）——在那里扩充身份会改变所有 QWEN_CODE_MODEL 消费方看到的内容。(2) 同模型闸门由 skill 比较 {{model}} 来执行，而缓存路径的 lastModelId（Step 8 由模型写入）同样存在裸 id 的局限——这是既有行为，且按本条发现自身"两个闸门使用同一身份"的要求，两者必须同步改动。(3) marker 搭载在公开、不可逆的评论中：最直接的区分信息（baseUrl、endpoint URL）可能是私有基础设施信息，因此感知 provider 的已发布身份需要一种脱敏格式——这是一个设计决策，而且本地缓存与已发布 marker 可能根本无法共用同一种格式。
我所看到的选项：(a) 接受裸的运行时发布 id 作为本 PR 的身份契约，并在 DESIGN.md 中记录多 provider 的局限；(b) 以跟进任务的方式追踪：在发布端扩充身份（运行时已存在带 authType 限定的 selector，如 getFastModel），并为 marker 定义一种可安全公开的 provider 区分信息；(c) 在本 PR 内重做身份设计——我不建议这样做，因为缓存格式、skill 的闸门指令、marker 格式需要一起改动，远超本 diff 的足迹。
向维护者提问：本 PR 的同模型身份是否可以接受裸的运行时模型 id、将感知 provider 的身份作为跟进任务追踪——还是本 PR 应等待更丰富的身份方案？
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review-round summary — PR #9184 (round 2)Addressed the review feedback on the ledger-marker model gate. The round Dispositions
What changedAttribution-off leak (rc:3786241088). With Model-authored provenance (rc:3786251565). The certifying model was read Truncation collision (rc:3786251561). The model id was truncated to 64 Asymmetric fallback (rc:3786241097). The "SHAs differ but model differs" Recovery-seam pin (rc:3786241102). Added Attribution-off branch test (rc:3786241108). Added marker-path tests: DESIGN.md wording (rc:3786241113). The bullet no longer claims the cache Declined (rc:3786241120). The test-efficacy probe asked to confirm Escalated (rc:3786251570). Provider-aware model identity (authType + Conflict notesNone. VerificationCommands actually run this round, and their results:
中文说明Autofix 审查轮次总结 — PR #9184（第 2 轮）本轮处理了关于 ledger-marker 模型闸门的审查反馈，在分支头 处理结果
改动内容attribution 关闭时的泄露（rc:3786241088）。 当 模型自报来源（rc:3786251565）。 认证模型此前读自 截断碰撞（rc:3786251561）。 模型 id 此前被截断为 64 字符，因此过长的 id 不对称回退（rc:3786241097）。 "SHAs 不同但模型不同"的缓存分支现在带有一个 恢复接缝钉住（rc:3786241102）。 在 attribution 关闭分支测试（rc:3786241108）。 新增 marker 路径测试： DESIGN.md 措辞（rc:3786241113）。 该条目不再声称缓存路径"一直通过 已拒绝（rc:3786241120）。 测试有效性 probe 要求确认 typecheck 是 已升级（rc:3786251570）。 感知 provider 的模型身份（authType + endpoint 冲突说明无。 验证本轮实际运行的命令及结果：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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 reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const certifying = runtime !== '' ? runtime : declared; | ||
| const model = attribution && certifying !== '' ? certifying : undefined; |
There was a problem hiding this comment.
[Critical] R2-9: Re-check of the round-1 cross-provider identity blocker (filed by @wenshao's reviewer): still stands at this commit. The marker's certifying identity is still the bare runtime model string with no authType/provider/endpoint discriminator; the autofix round escalated exactly this point for a maintainer decision (accept the bare runtime id and record the limitation, or track a provider-aware identity as a follow-up), and that decision is still open — so the blocker is re-asserted for this round's verdict. — Failure scenario: two provider or endpoint configurations expose the same model ID while resolving to different underlying models → the second configuration accepts the first configuration's anchor and skips code it never reviewed.
Witness: not run — the blocker is the absence of a provider discriminator in the identity; read at cb366f55: both arms above are bare model ids (runtime = QWEN_CODE_MODEL, declared = state.modelId), and the escalation reply confirms the round-1 remedy was not implemented.
Suggested fix: persist and compare a stable resolved-model identity including the relevant auth type and provider or endpoint discriminator, using the same identity for the cache and posted-ledger gates — or record a maintainer decision accepting the bare runtime id as this PR's identity contract and track the richer identity as a follow-up.
中文说明
【Critical】 对第 1 轮跨 provider 身份阻断项(由 @wenshao 的审查者提出)的复核:在本提交上仍然存在。marker 的认证身份仍是裸的运行时模型字符串,没有 authType/provider/endpoint 区分信息;autofix 轮次已就这一点升级上报、等待维护者决策(接受裸运行时 id 并记录其局限,或将 provider 感知的身份作为跟进任务追踪),该决策仍未落地——因此本条阻断项在本轮裁定中重新成立。 — 失败场景:两个 provider 或 endpoint 配置暴露相同的模型 ID、但解析为不同的底层模型 → 第二个配置会接受第一个配置的 anchor,并跳过它从未审查过的代码。
见证:未运行——阻断点是身份中缺少 provider 区分信息;已在 cb366f55 上直接阅读代码:上述两个分支都是裸模型 id,升级回复也确认第 1 轮的补救方案未实施。
建议修复:持久化并比较包含相关 auth type 与 provider/endpoint 区分信息的稳定解析模型身份,缓存与已发布 ledger 两个闸门使用同一身份——或由维护者明确决策接受裸运行时 id 作为本 PR 的身份契约,并将更丰富的身份作为跟进任务追踪。
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
This round deliberately leaves the blocker unresolved. At this commit the marker's certifying identity is still the bare runtime model string (QWEN_CODE_MODEL, with the model-written state field as the fallback) — no authType/provider/endpoint discriminator — so the round-1 claim still stands, and this round did not attempt to implement either remedy unilaterally.
The previous round escalated exactly this point for a maintainer decision, and that decision is still open. The two options, restated:
- Option A — accept the bare runtime model id as this PR's identity contract, record the limitation (two provider/endpoint configurations exposing the same model id while resolving to different underlying models would share anchors), and track a provider-aware identity as a follow-up issue.
- Option B — require the richer identity in this PR: persist and compare a stable resolved-model identity including the relevant auth type and provider/endpoint discriminator, used identically for the cache and posted-ledger gates.
Recommendation on record: Option A. This PR's threat model is forgery through model-typed channels, which the runtime injection closes; cross-provider id collision is a narrower scenario, and fixing it requires a design decision across the core auth/provider/config surface, which is maintainer-only per AGENTS.md. This thread stays UNRESOLVED until a maintainer chooses; the answer arrives as ordinary feedback next round.
中文说明
本轮有意保持该阻断项未解决。在本提交上,marker 的认证身份仍是裸的运行时模型字符串(QWEN_CODE_MODEL,以模型自己填写的 state 字段作为回退)——没有 authType/provider/endpoint 区分信息——因此第 1 轮的主张依然成立,本轮也没有单方面实施任何一种补救方案。
上一轮已就这一点升级上报、等待维护者决策,该决策仍未落地。两个选项重述如下:
- 选项 A — 接受裸运行时模型 id 作为本 PR 的身份契约,记录其局限(两个暴露相同模型 id、却解析为不同底层模型的 provider/endpoint 配置会共享 anchor),并将 provider 感知的身份作为跟进 issue 追踪。
- 选项 B — 在本 PR 中就要求更丰富的身份:持久化并比较包含相关 auth type 与 provider/endpoint 区分信息的稳定解析模型身份,缓存与已发布 ledger 两个闸门使用同一身份。
记录在案的建议:选项 A。本 PR 的威胁模型是经由模型可填写通道的伪造,运行时注入已经关闭了该通道;跨 provider 的 id 冲突是更窄的场景,其修复需要在 core 的 auth/provider/config 表面做设计决策——按 AGENTS.md 属维护者专属。该线程保持未解决,直到维护者作出选择;答复会在下一轮作为普通反馈到达。
| // into this subprocess — not the id the state JSON typed. | ||
| process.env['QWEN_CODE_MODEL'], |
There was a problem hiding this comment.
[Suggestion] R2-1: The runtime-model injection at this posting boundary has no wiring test — deleting the fourth argument survives the entire suite. submit.test.ts never sets QWEN_CODE_MODEL, and its marker suite ('the ledger marker on the body that reaches GitHub') asserts only round and findings on parseLedger(posted().body); the PR's new wiring test covers only the compose-review handler, which writes nothing and posts nothing — submit is the only boundary whose body reaches GitHub. — Failure scenario: a future edit drops process.env['QWEN_CODE_MODEL'] from runSubmit's compose(...) call → runtimeModelId becomes undefined, the posted marker's model silently falls back to the model-written state.modelId, and every posted anchor is again certified by whatever id the state JSON typed — the exact cross-model forgery this PR exists to prevent — while submit.test.ts (65/65) and the compose-review/ledger/pr-context suites (319/319) all stay green.
Witness (probe, both arms): unmutated — probe passes, posted marker "model":"the-session-model"; with the argument dropped — posted marker {"model":"typed-by-the-model","sha":"deadbeef00112233"} with the whole suite still green.
Suggested fix: mirror the compose-review wiring test in submit.test.ts — save/set process.env['QWEN_CODE_MODEL'] = 'the-session-model' (restore in finally), run runSubmit with a state whose modelId is a different literal ('typed-by-the-model') and a plan carrying fetchedSha, then assert parseLedger(posted().body)?.model === 'the-session-model'.
中文说明
【Suggestion】 R2-1:此发布边界的运行时模型注入没有任何接线测试——删掉第四个参数后整个测试套件仍然全绿。submit.test.ts 从未设置 QWEN_CODE_MODEL,其 marker 测试块('the ledger marker on the body that reaches GitHub')只对 parseLedger(posted().body) 断言了 round 与 findings;本 PR 新增的接线测试只覆盖了 compose-review handler,而该 handler 什么都不写、什么都不发布——submit 才是唯一让正文到达 GitHub 的边界。 — 失败场景:未来某次修改把 process.env['QWEN_CODE_MODEL'] 从 runSubmit 的 compose(...) 调用中删掉 → runtimeModelId 变为 undefined,已发布 marker 的 model 静默回退到模型自己写的 state.modelId,所有已发布的 anchor 又变成由 state JSON 所填写的任意 id 来认证——正是本 PR 要防止的跨模型伪造——而 submit.test.ts(65/65)与 compose-review/ledger/pr-context 套件(319/319)全部保持绿色。
见证(probe,双向验证):未突变——probe 通过,发布的 marker 为 "model":"the-session-model";删掉该参数后——发布的 marker 为 {"model":"typed-by-the-model","sha":"deadbeef00112233"},整套测试仍全绿。
建议修复:在 submit.test.ts 中镜像 compose-review 的接线测试——保存并设置 process.env['QWEN_CODE_MODEL'] = 'the-session-model'(在 finally 中恢复),以 modelId 为不同字面量('typed-by-the-model')的 state 和带 fetchedSha 的 plan 运行 runSubmit,然后断言 parseLedger(posted().body)?.model === 'the-session-model'。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - **Own-account, latest round only.** The ledger claims "these are the findings the previous /review stood behind", and only this account's reviews can make that claim — another user's marker is data about _their_ tooling. Each posted round embeds a fresh full copy, so the newest marker is the whole state. | ||
| - **Data, not authority.** Every recovered entry is owed a Step 6 ruling against the code — the ledger routes work, it never rules. A tampered or stale marker therefore costs a few wasted rulings, not a wrong verdict, which is why parsing is fail-quiet and the round number feeds `compose-review` from a CLI-written side file rather than a model's memory. | ||
| - **Medium reads, high writes.** Recovering the ledger is free (the reviews were already fetched), so the default-effort re-review finally opens like a round-2 comment; the cache write and the posting that carries the marker keep their existing effort gates untouched. | ||
| - **The anchor names its model.** Incremental scoping is a same-model contract — "clean up to `sha`" is one model's verdict. The cache path has paired its anchor with `lastModelId` from the start, but enforced the contract only in the same-SHA skip case (a re-run under a different model gets a full second opinion, not a skip); its differing-SHA incremental branch carried no model condition until this change. The marker's anchor shipped bare, so the recovery path had no way to honour the contract at all: a round run under model B that recovered model A's anchor would scope `sha..HEAD` past code B never reviewed — permanently, since each clean round re-anchors past the last. The marker now carries `model` beside `sha`, riding and falling with the anchor (withheld on fail-closed and truncated rounds alike; an id over the marker's cap withholds the PAIR — a truncated id is a prefix, and a prefix can equal another model's full id; dropped by the parser when the sha beside it did not survive), and Step 1's recovered-anchor gate requires it to match the running model — absent, on markers from before the field, counts as a mismatch: the work list still carries (rulings re-assert against the code, so they cross models safely), only the anchor does not. Two boundaries of that carriage: the identity the marker carries is the one the session publishes (`QWEN_CODE_MODEL`, injected at the CLI boundaries), with the model-written state field only the fallback for runs no session published — a review must not be able to certify an anchor to a model it names but is not; and `review.attribution: false` withholds it with the footer, because the setting's contract is whether the posted review names its model, and the marker rides the posted body. |
There was a problem hiding this comment.
[Suggestion] R2-2: The setting's enlarged behavioural surface is documented only here (DESIGN/SKILL); the user-facing settings entry — whose own convention already lists consequences of disabling it — does not mention the new one. With attribution off the marker omits model, and the new same-model gate reads absence as a mismatch, so for an attribution-off user every round in a fresh environment (CI, another clone — the normal state per SKILL.md, where the cache is absent) silently degrades from incremental to full-range review. Pre-PR, the recovered anchor had no model condition and passed; post-PR it never passes. — Concrete cost: a user wondering why incremental scoping never engages in CI has docs/users/configuration/settings.md (the review.attribution row, which already documents a sibling side effect) as the lookup surface and finds nothing there.
Suggested fix: add a sentence to the review.attribution row in docs/users/configuration/settings.md (and docs/users/features/code-review.md): with attribution off, the ledger marker also withholds the model, so recovered anchors in fresh environments fall back to a full review.
中文说明
【Suggestion】 R2-2:该设置被扩大的行为面只记录在这里(DESIGN/SKILL);面向用户的设置条目——其自身惯例本就会列出禁用后的后果——没有提到新增的这一条。attribution 关闭时 marker 会省略 model,而新的同模型闸门把缺失视为不匹配,因此对关闭 attribution 的用户来说,在全新环境(CI、另一份 clone——按 SKILL.md 这是常态,缓存不存在)中的每一轮都会从增量审查静默退化为全量审查。本 PR 之前,恢复的 anchor 没有模型条件、可以通过;之后永远无法通过。 — 具体代价:用户想知道为什么增量范围划分在 CI 里从不生效时,docs/users/configuration/settings.md(review.attribution 一行,已经记录了同类的副作用)是其查阅入口,但那里找不到任何相关信息。
建议修复:在 docs/users/configuration/settings.md 的 review.attribution 一行(以及 docs/users/features/code-review.md)补充一句:attribution 关闭时,ledger marker 也会扣下模型,因此全新环境中恢复的 anchor 会退化为全量审查。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
|
||
| - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): if `.qwen/review-cache/pr-<n>.json` exists, read it **in the same response as the fetch report** — both are `read_file`, genuinely parallel — for `lastCommitSha` and `lastModelId`. Compare to `fetchedSha` from the fetch report and the current model ID (`{{model}}`): | ||
| - If SHAs differ → continue with the worktree just created. Compute the incremental diff (`git diff <lastCommitSha>..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **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. | ||
| - If SHAs differ **and** model matches → continue with the worktree just created. Compute the incremental diff (`git diff <lastCommitSha>..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **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. |
There was a problem hiding this comment.
[Suggestion] R2-5: This new differing-SHA model gate reads lastModelId — a value the model itself types into .qwen/review-cache/pr-<n>.json at Step 8 ("lastModelId": "{{model}}"; a repo-wide grep confirms no CLI writer of the cache exists — only the unused REVIEW_CACHE_DIR constant). The boundary this PR's own added DESIGN.md paragraph claims — "a review must not be able to certify an anchor to a model it names but is not", enforced for the marker by runtime injection — is therefore not enforced on the very cache gate this same diff adds. — Failure scenario: a round under model A (including one steered by prompt injection in the PR content) writes "lastModelId": "B" beside the honest lastCommitSha at Step 8; the operator later runs model B for the cross-model re-review the skill explicitly supports — round N+1 under B reads "SHAs differ and model matches" (forged), scopes lastCommitSha..HEAD, and permanently skips code only A ever reviewed. Mitigating: pre-PR this branch had no model condition at all, so the forged outcome does not exceed the pre-PR baseline — a defeated improvement plus an overstated design claim, not a new worst case.
Suggested fix: mirror the marker's fix pattern on the cache boundary — route the Step 8 cache write through a CLI subcommand that stamps QWEN_CODE_MODEL so lastModelId is injected, not typed; or, at minimum, name in the new DESIGN.md paragraph that the cache path's lastModelId remains a model-typed channel and the "must not be able to certify" sentence bounds only the marker.
中文说明
【Suggestion】 R2-5:这个新增的"SHA 不同"模型闸门读取的 lastModelId,是模型自己在 Step 8 写入 .qwen/review-cache/pr-<n>.json 的值("lastModelId": "{{model}}";全仓 grep 确认不存在任何 CLI 写入方——只有未使用的 REVIEW_CACHE_DIR 常量)。因此,本 PR 自己在 DESIGN.md 新增段落中声明的边界——"审查不能把 anchor 认证到自己口头声称但并非实际运行的模型"(marker 侧已通过运行时注入强制)——在同一个 diff 新增的这个缓存闸门上并未被强制。 — 失败场景:模型 A 的一轮审查(包括被 PR 内容中的 prompt injection 操纵的一轮)在 Step 8 把 "lastModelId": "B" 与诚实的 lastCommitSha 写在一起;之后运维者按 skill 明确支持的跨模型复审运行模型 B——B 的第 N+1 轮读到"SHA 不同且模型匹配"(伪造),范围划为 lastCommitSha..HEAD,永久跳过只有 A 审查过的代码。缓解因素:本 PR 之前该分支根本没有模型条件,因此伪造结果并未超过改动前基线——这是被击败的改进加上被夸大的设计声明,而非新的最坏情形。
建议修复:在缓存边界镜像 marker 的修复模式——把 Step 8 的缓存写入改由 CLI 子命令完成并盖上 QWEN_CODE_MODEL,使 lastModelId 是被注入的而非模型填写的;或至少在 DESIGN.md 新段落中说明:缓存路径的 lastModelId 仍是模型可填写的通道,"不能认证"那句话只约束 marker。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - If SHAs match **but** model differs → continue. Inform: "Previous review used {cached_model}. Running full review with {{model}} for a second opinion." | ||
|
|
||
| - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `pr-context` writes it into the side file `qwen-review-pr-<n>-prev-ledger.json` with the rest of the ledger. So when the cache is absent or its `lastCommitSha` was rebased away: proceed with the setup batch as usual, and when the side file lands, read its `sha`. **Validate before scoping** — inside the worktree, `git cat-file -e <sha>^{commit}` and `git merge-base --is-ancestor <sha> HEAD` — and on success treat it exactly as `lastCommitSha` above: the same outcomes, decided AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). A sha that fails either check — rebased away, or not this history's — falls back to the full diff with a logged warning, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted; and a side file with no `sha` field means the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions), had its ledger truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the next round's scope and retire silently), or predates the field — in every case there is no anchor to recover, and the review is full-range. | ||
| - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `model`, the model whose verdict certified it, and `pr-context` writes them into the side file `qwen-review-pr-<n>-prev-ledger.json` with the rest of the ledger. So when the cache is absent or its `lastCommitSha` was rebased away: proceed with the setup batch as usual, and when the side file lands, read its `sha` **and `model`**. **Validate before scoping** — inside the worktree, `git cat-file -e <sha>^{commit}` and `git merge-base --is-ancestor <sha> HEAD`, and require the side file's `model` to equal `{{model}}` — the same same-model contract the cache path enforces through `lastModelId`: an anchor certified by a different model scopes this round past code `{{model}}` never reviewed. A side file whose `model` differs — or is absent, on markers from before the field — recovers the findings work list but NO anchor: the review is full-range, with the cache path's "Previous round was reviewed by …" notice. On a sha and model that both pass, treat the sha exactly as `lastCommitSha` above: the same outcomes, decided AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). A sha that fails either check — rebased away, or not this history's — falls back to the full diff with a logged warning, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted (the model gate rules on whichever anchor is preferred; when the preferred anchor fails it, fall back to the other anchor if that one passes — an older same-model anchor scopes a superset range, which is safe — and to the full diff otherwise); and a side file with no `sha` field means the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions), had its ledger truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the next round's scope and retire silently), certified its range under a model id too long for the marker's cap (the anchor pair is withheld whole — a truncated id is a prefix, and a prefix can equal another model's full id), or predates the field — in every case there is no anchor to recover, and the review is full-range. |
There was a problem hiding this comment.
[Suggestion] R2-6: This fallback clause routes only model-gate failure of the preferred anchor to the other anchor, but the paragraph's own entry states (cache absent, or its lastCommitSha rebased away) make that success unreachable — in both entry states the cache anchor cannot pass any whole-gate reading. Meanwhile the reachable failure class — the preferred cache anchor fails the ancestor check while the side file's same-model anchor passes both checks — is routed by the preceding universal sentence to the full diff, discarding a valid anchor. — Failure scenario: machine X caches round 5 under model A; a force-push rebases the cached sha away; the account's last posted marker is round 3, sha S, model A, with S still an ancestor of the fetched head. The round rule leaves the cache sha preferred (5 > 3); its failure is the ancestor check, not the model gate, so this parenthetical never fires and the review runs full-range instead of S..HEAD — the exact fresh-environment/force-push cost the recovery feature exists to avoid. Direction is fail-safe (never under-reviews).
Suggested fix: key the fallback to the full gate, not the model check alone — e.g. "use whichever anchor passes the recovered-anchor gate whole (valid sha, ancestor of the fetched head, model == {{model}}); when both pass, prefer the higher-round sha" — which subsumes the current clause and covers the rebased-preferred case.
中文说明
【Suggestion】 R2-6:该回退子句只把"被优先 anchor 未通过模型闸门"这一种失败路由到另一个 anchor,但本段落自身的进入条件(缓存不存在、或其 lastCommitSha 被 rebase 掉)决定了这种路由不可能成功——在两种进入状态下,缓存 anchor 在任何整体闸门解读下都不可能通过。而可达的失败类别——被优先的缓存 anchor 未通过祖先检查、而旁路文件中同模型的 anchor 两项检查都通过——却被前面那句通用规则路由到全量 diff,丢弃了一个有效 anchor。 — 失败场景:机器 X 在模型 A 下缓存了第 5 轮;一次 force-push 把缓存的 sha rebase 掉;该账号最近一次发布的 marker 是第 3 轮、sha S、模型 A,且 S 仍是 fetched head 的祖先。轮次规则使缓存 sha 仍被优先(5 > 3);它的失败是祖先检查而非模型闸门,因此本括注永远不会触发,审查只能全量运行而不是 S..HEAD——恰恰是恢复 anchor 特性本要避免的全新环境/force-push 成本。方向是 fail-safe(不会审查不足)。
建议修复:把回退条件改为完整闸门而非仅模型检查——例如"使用任一通过完整恢复闸门的 anchor(有效 sha、是 fetched head 的祖先、model == {{model}});两者都通过时优先更高轮次的 sha"——这包含现有子句并覆盖"被优先 anchor 被 rebase 掉"的情形。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): if `.qwen/review-cache/pr-<n>.json` exists, read it **in the same response as the fetch report** — both are `read_file`, genuinely parallel — for `lastCommitSha` and `lastModelId`. Compare to `fetchedSha` from the fetch report and the current model ID (`{{model}}`): | ||
| - If SHAs differ → continue with the worktree just created. Compute the incremental diff (`git diff <lastCommitSha>..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **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. | ||
| - If SHAs differ **and** model matches → continue with the worktree just created. Compute the incremental diff (`git diff <lastCommitSha>..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **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. | ||
| - If SHAs differ **but** model differs → continue with the worktree, and the scope is the **full diff**, never `<lastCommitSha>..HEAD`: "clean up to `lastCommitSha`" is {cached_model}'s verdict, and an incremental range scoped to another model's anchor leaves everything before it reviewed by no run of `{{model}}` — permanently, because this round's own cache write would re-anchor past it. One reprieve — the mirror of the recovery paragraph's fallback: when the side file lands with the setup batch, if ITS anchor passes the recovered-anchor gate below (valid sha, ancestor of the fetched head, `model` == `{{model}}`), scope to it instead — decided AFTER the setup batch but BEFORE any agent launches — because an anchor the CURRENT model certified in a round some other environment posted is exactly the contract's own terms. Otherwise inform: "Previous round was reviewed by {cached_model}. Running full review with {{model}}." Still read the cache's `findings` ledger and owe each entry its Step 6 ruling — the work list carries across models (every entry is re-asserted against the code); only the anchor does not. |
There was a problem hiding this comment.
[Suggestion] R2-7: This reprieve routes a passing side-file anchor to "scope to it instead" but not to "the same outcomes" as lastCommitSha — so when the recovered sha equals the fetched head the scope is empty, and the no-new-changes stop (keyed to the cache's own SHAs matching, false in this branch by construction) is unreachable. The recovery paragraph the reprieve claims to mirror handles this geometry expressly ("treat the sha exactly as lastCommitSha above: the same outcomes … a same-SHA stop still runs cleanup"). — Failure scenario: environment A runs model M, reviews head H, posts (marker sha=H, model=M). Machine X's cache holds an older round under model N (lastCommitSha S ≠ H). Machine X fetches the same head H (no new commits) and runs /review under M at high effort: the reprieve passes (H is valid; a commit is its own ancestor; M == M) and the scope becomes H..H = empty — the agent fan-out launches over nothing, the round posts a noise verdict and re-anchors, and with --comment the comment pass the operator explicitly requested silently produces zero comments. Direction is fail-safe.
Witness: git merge-base --is-ancestor cb366f55 cb366f55 → exit 0 at the reviewed commit (a commit is its own ancestor, so the reprieve gate passes on the equal-sha geometry); the claim itself is about model-facing spec prose and was traced, not run.
Suggested fix: route a passing anchor to the same outcomes as lastCommitSha — e.g. "scope to it instead, applying the same outcomes as lastCommitSha above (if it equals the fetched head, the same-SHA decision applies: stop with cleanup unless comment.effective, which runs the full review)".
中文说明
【Suggestion】 R2-7:该 reprieve(豁免)子句把通过闸门的旁路文件 anchor 路由为"改按其划分范围",却没有路由到与 lastCommitSha "相同的后续结果"——因此当恢复出的 sha 恰好等于 fetched head 时,审查范围为空,而"无新变更"的停止分支(以缓存自身 SHA 匹配为键,在本分支按构造恒为假)永远不可达。reprieve 自称镜像的恢复段落对这一几何形状有明确处理("将该 sha 完全按上面的 lastCommitSha 对待:相同的后续结果……同 SHA 停止仍会运行 cleanup")。 — 失败场景:环境 A 运行模型 M,审查 head H 并发布(marker sha=H,model=M)。机器 X 的缓存里是模型 N 的更早一轮(lastCommitSha S ≠ H)。机器 X 拉取到同一个 head H(没有新提交)并以高 effort 运行 /review(模型 M):reprieve 通过(H 有效;一个 commit 是自己的祖先;M == M),范围变成 H..H = 空——agent 扇出在零 hunk 上启动,本轮发布一个无意义的裁定并重新锚定,而使用 --comment 时运维者明确要求的评论发布环节静默地产出零条评论。方向是 fail-safe。
见证:git merge-base --is-ancestor cb366f55 cb366f55 → 在被审查提交上 exit 0(一个 commit 是自己的祖先,因此 reprieve 闸门在"sha 相等"的几何下通过);该主张本身针对模型面对的规范文本,是通过追踪而非运行验证的。
建议修复:把通过闸门的 anchor 路由到与 lastCommitSha 相同的后续结果——例如"改按其划分范围,并套用上面 lastCommitSha 的相同后续结果(若其等于 fetched head,则适用同 SHA 判定:除非 comment.effective 为真而运行完整审查,否则运行 cleanup 后停止)"。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 3164 passed · 0 failed · 3164 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：3164 通过 · 0 失败 · 3164 总计 Verification reportPR #9184 Deep Verification —
|
| cell | oracle | head | base |
|---|---|---|---|
| clean round posts a marker | parseLedger(posted body) |
marker + sha |
marker + sha |
| identity channel | marker model |
qwen3.7-max ✓ |
absent (the hole) ✓ as predicted |
| cross-model recovery (model B) | rendered section names certifier + gate + absent=mismatch | decidable ✓ | undecidable ✓ as predicted |
| same-model recovery (model A) | recovered model == running model + gate text |
decidable ✓ | undecidable ✓ as predicted |
| legacy (pre-field) marker | fail-quiet parse; absent=mismatch still taught | ✓ | ✓ (own shape) |
| gate-decidable scenarios | count | 2/2 | 0/2 |
Assertions: head 20/20, base 17/17 (base's expectations encode the predicted hole — expected failures are passes).
Table 2 — real CLI subprocess A/B (cli-wiring-harness.mjs, node dist/index.js review compose-review with QWEN_CODE_MODEL=session-model-X exported as a session's shell tool does; witness: 02-cli-wiring-env-identity-head-vs-base.png):
| cell | head | base |
|---|---|---|
| subprocess exits 0 | ✓ | ✓ |
| marker on composed body | ✓ | ✓ |
| anchor certified by the env identity | session-model-X ✓ |
absent ✓ as predicted |
| typed state-JSON id does not certify | ✓ | n/a |
Head also passes the boundary/forgery probe battery — 50/50 scripted checks (04-boundary-forgery-probes.png): runtime/declared fallbacks, attribution-off withholding (incl. the runtime identity — a sibling the PR's own tests do not pin), fail-closed and module-self-capped rounds withholding the pair, hand-edited markers (model without sha, invalid sha, over-cap forged model, non-string shapes, dropped+anchor, --> comment-close injection → fail-quiet null), cap edges including astral ids measured in UTF-16 units (64 rides whole, 66 withholds the pair), -- comment-safety round-trip, render sanitisation of a hostile model id, and own-account/latest-round recovery selection.
One boundary documented, not a defect: with attribution on and an empty declared modelId, composeReviewBody throws modelId is required before the marker composes even when a runtime identity is present — the footer validation the runtime field deliberately does not exempt. Asserted identical on base (A/A), so it is pre-existing.
Corrections
- Description vs final code. The PR body describes commit 1 ("the marker records the model whose clean verdict certified the anchored commit"). The head commit
cb366f55went further: the certifying identity is now the runtime-publishedQWEN_CODE_MODELinjected at both CLI boundaries, with the model-written state field demoted to fallback. This is a stronger property than the body states (a model cannot type another model's id into the anchor); DESIGN.md documents it ("Two boundaries of that carriage") and the tests pin it (M1/M5 below). Reviewers should read the change as "identity source moved to the runtime", not merely "field added". No code change requested. - No inaccuracies found in prior review comments to correct; no instruction-injection attempts detected in PR text.
Findings
Both are Suggestion-level coverage gaps from the mutation matrix — completeness reporting, not merge conditions. Neither is load-bearing: the behavior each guard produces was independently proven correct by the harnesses above.
F1 — The SKILL.md gate hunks are pinned by no test (mutant M8 survived)
Reverting the entire Step-1 gate text — both the new cache-path bullet ("If SHAs differ but model differs → full diff…") and the recovery paragraph's model-match requirement — leaves the guard suite green: 16/16 pass on SKILL.test.ts. Yet that file's own convention is to pin load-bearing clauses with revert-guards (several existing tests carry the comment "a revert … must fail a test, not slip through"). Repro: restore the base skill text (git show HEAD^1:packages/core/src/skills/bundled/review/SKILL.md over the file) and run npx vitest run src/skills/bundled/review/SKILL.test.ts from packages/core — all green; or node <this dir>/mutate.mjs, row M8. The prompt-level gate is the enforcement this PR exists to ship; a future paraphrase or revert would pass CI silently. The positive control confirms the suite can go red (mutant M8b, a known-pinned clause, killed 1/16 on exactly its test), so this is a genuine pin absence, not a dead suite. Severity is bounded: the cli-side inputs (identity carriage, rendered instruction) are test-pinned (M1–M5, M7 all killed), so a SKILL.md revert would degrade gate instruction only.
Suggested fix (unmeasured — it is a test addition, not a behavior change)
Add a SKILL.test.ts revert-guard pinning the load-bearing clauses, in the file's existing style, e.g. expect(body).toContain('If SHAs differ **but** model differs'), expect(body).toContain('require the side file\'s \model` to equal `{{model}}`'), and expect(body).toContain('an absent `model` counts as a mismatch')` — the fixture that would go red is those assertions themselves.
F2 — The submit boundary's runtime-identity injection is pinned by no test (mutant M6 survived)
Deleting process.env['QWEN_CODE_MODEL'] from runSubmit's compose() call leaves submit.test.ts green: 65/65 pass. Repro: replace that argument with undefined in packages/cli/src/commands/review/submit.ts and run npx vitest run src/commands/review/submit.test.ts from packages/cli — all green; or node <this dir>/mutate.mjs, row M6. The compose-review boundary has a wiring test for the identical line; submit — the path that posts the body the marker rides — has none. A regression here would silently demote the posted anchor's certifier to the typed state-JSON id, the exact attack the head commit removes. The behavior itself is verified correct at head (Table 1 + probes exercise the same compose seam submit calls).
Suggested fix (unmeasured — test addition only)
Mirror the compose wiring test in submit.test.ts: export QWEN_CODE_MODEL around runSubmit, then assert parseLedger(posted().body)?.model equals the session model and not state.modelId.
Mutation matrix (all 9 outcomes as predicted; witness: 03-mutation-matrix-live.png)
| mutant | guard | suite | result | classification |
|---|---|---|---|---|
| M1 | runtime identity precedence in compose | compose-review (233) | RED — 2 named tests, e.g. expected 'typed-by-the-model' to be 'the-session-model' |
killed |
| M2 | serialize: over-cap model withholds the anchor PAIR | ledger (18) | RED — normalises the model on both sides |
killed |
| M3 | parse: model dropped when sha did not survive / over-cap | ledger (18) | RED — 2 named tests | killed |
| M4 | attribution off withholds the model | compose-review (233) | RED — withholds the model WITH the footer |
killed |
| M5 | compose-review CLI boundary env injection | compose-review (233) | RED — wiring test | killed |
| M6 | submit CLI boundary env injection | submit (65) | GREEN | survivor → coverage gap (F2) |
| M7 | renderLedgerSection certifier + gate + absent=mismatch | pr-context (74) | RED — names the anchor model beside the sha… |
killed |
| M8 | SKILL.md gate text (both paths) | SKILL.test (16) | GREEN | survivor → coverage gap (F1) |
| M8b | POSITIVE CONTROL (known-pinned clause) | SKILL.test (16) | RED — exactly its test | killed (proves suite live) |
Vacuity: the central new tests are non-vacuous — each killed mutant failed the intended behavioral assertion (expected-vs-actual model/section text quoted in logs/mut-M*.log), never an import or fixture break. Unmutated controls ran green first in the same worktree (cli 390/390, core 16/16).
Not covered
- Reviewer Test Plan, E2E step ("post a high-effort round on a test PR, re-run under a different model id from a clean checkout"): this lane has no GitHub credentials and must not post. The recovery half of that scenario is simulated end-to-end against built code (Tables 1–2, including the clean-checkout case: recovery from a posted body with no local cache). What remains untested is the orchestrator's compliance with the gate instruction — model behavior, unreachable from code on either arm.
- Prompt-level enforcement is not an invariant — by design of the review system (the pre-existing cache-path gate is equally prompt-enforced). The A/B proves the gate's inputs now reach the decision point and base structurally could not; it cannot prove the model obeys.
- Per-commit A/B: commit 2 (
5f3f779..cb366f55, 8 review files) was isolated by tree diff and its claim verified by the dedicated cells (Tables 2, M1, M5). Commit 1's own tree is only reachable against a base not present locally (both candidate base OIDs are non-ancestral to it in the shallow fetch), so it was verified as the aggregate minus commit 2. The metadata's two commits both exist as objects; the depth-2 graft cutsrev-listancestry (reports 1 of 2) — attribution was done by tree diff instead. review submitagainst a GitHub wire (auth/posting path) — not driven; the compose seam it calls is covered.- Base OID drift: the snapshot's
baseRefOid(9aa57044…) is not an ancestor of the checkout base9f8f65dde0in either direction locally; the A/B used the merge-ref base per the CI contract. Whether main's movement touched review code is moot for this diff's A/B (base arm rebuilt atHEAD^1). - Repo-wide vitest beyond the targeted packages; lint; macOS/Windows (no platform-specific paths in the diff).
previous-report.mdabsent — first round, nothing carried forward.
Methodology
Environment: node:22-bookworm CI container (node v22.23.2, npm 10.9.8, no zstd), merge-ref checkout at depth 2; npm ci + npm run build pre-run at HEAD. The verified state is the merge commit c40516793e (clean merge of the PR head into the base tip — no conflict residue), so what landed is what was measured. Base arm: git worktree add tmp/base-tree HEAD^1 with root/package node_modules symlinked; only packages/cli rebuilt there (after regenerating the gitignored git-commit.ts). Realpath assertion: from the base tree, node_modules/@qwen-code/qwen-code-core resolves into the head tree — a clean control anyway, because the PR changes 0 .ts files in core (verified docs-only: DESIGN.md/SKILL.md). Harnesses are mock-free with respect to the units: compiled dist/ modules, a real node dist/index.js review compose-review subprocess, and temp fixtures replicating the repo's own coveredPlan() layout (coverage transcripts, prompt records, briefs, Step 4/5 records). Mutation matrix: source mutants in a scratch worktree (tmp/mut-tree), scoped vitest runs, reverted immediately after each; unmutated controls green first. Targeted gates on the main tree: packages/cli review suites (2625 pass / 4 pre-existing skips / 0 fail, 75 files), core guard suites (SKILL.test.ts + bundled-skills.integration.test.ts, 28/28), repo-wide npm run typecheck clean. Both same-model-gate inputs were traced to one state source: BundledSkillLoader resolves {{model}} from Config.getModel() and publishModelEnv() keeps QWEN_CODE_MODEL on the same value across model switches, so the cache-path and recovery-path predicates compare the same identity. Raw logs: logs/ (per-cell, per-mutant, gates); harnesses: ab-harness.mjs, cli-wiring-harness.mjs, probe-harness.mjs, mutate.mjs, fixture.mjs. Evidence images rendered with scripts/verify-capture.mjs.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9184 (round 3)Five of the six findings are resolved in code this round; the remaining Feedback points and dispositionsR2-9 — [Critical] Cross-provider identity blocker re-check (compose-review.ts:584) — ESCALATED, unresolvedThe round-1 blocker still stands at this commit, and this round does not
Recommendation recorded on the thread: Option A — this PR's threat model R2-1 — [Suggestion] No wiring test at the submit posting boundary (submit.ts:502) — RESOLVEDVerified the claim first: Change: added Mutation probe (evidence the test pins the wiring): temporarily replacing the R2-2 — [Suggestion] Attribution-off side effect missing from user-facing docs (DESIGN.md:459) — RESOLVEDVerified in code: with Change: one sentence added to each user-facing surface, as suggested — R2-5 — [Suggestion] DESIGN.md overclaims the cache path's model gate (SKILL.md:146) — RESOLVED (minimal option)Verified: Step 8 has the model type Change: implemented the finding's explicitly-offered minimal option — the R2-6 — [Suggestion] Two-anchor fallback keyed to the model check alone (SKILL.md:152) — RESOLVEDVerified by tracing the paragraph: when the preferred (higher-round) cache Change: the two-anchor edge is now keyed to the WHOLE gate — when both R2-7 — [Suggestion] Reprieve routes a passing anchor to scoping but not to the same outcomes (SKILL.md:147) — RESOLVEDVerified: a commit is its own ancestor ( Change: the reprieve now treats a passing side-file sha exactly as Notes
VerificationCommands actually run this round (runner: clean
Not run (with reason): 中文说明Autofix 轮次总结 — PR #9184(第 3 轮)本轮在代码中解决了六条反馈中的五条;剩余的 Critical(R2-9)是一项维护者决策,上一轮已升级上报,目前仍未有答复——本轮有意保持其未解决状态,并在该线程上重新列出了待决问题。 反馈点与处置R2-9 —【Critical】跨 provider 身份阻断项复核(compose-review.ts:584)— 已升级,保持未解决第 1 轮的阻断项在本提交上仍然成立,本轮也不回避它:marker 的认证身份依然是裸的运行时模型字符串(
记录在线程上的建议:选项 A — 本 PR 的威胁模型是经由模型可填写通道的伪造,运行时注入已经关闭了该通道;跨 provider 的 id 冲突是更窄的场景,其修复横跨 core 的 auth/provider/config 表面(按 AGENTS.md 属维护者专属)。该线程保持未解决,直到维护者给出答复;答复会在下一轮作为普通反馈到达。 R2-1 —【Suggestion】submit 发布边界没有接线测试(submit.ts:502)— 已解决先核实了该主张: 改动:在 突变 probe(证明该测试钉住了接线的证据):把 R2-2 —【Suggestion】attribution 关闭的副作用未写入面向用户的文档(DESIGN.md:459)— 已解决已在代码中核实: 改动:按建议在两个面向用户的入口各补一句—— R2-5 —【Suggestion】DESIGN.md 夸大了缓存路径的模型闸门(SKILL.md:146)— 已解决(取最小选项)已核实:Step 8 由模型把 改动:实现了该发现明确给出的最小选项——DESIGN.md 段落现在给该保证划定了范围:它只约束 marker(其身份在 CLI 边界被注入);缓存路径的 R2-6 —【Suggestion】双 anchor 回退只以模型检查为键(SKILL.md:152)— 已解决通过追踪段落逻辑核实:当被优先的(更高轮次)缓存 anchor 被 rebase 掉、而旁路文件的同模型 anchor 通过所有检查时,旧子句永远不会触发(失败发生在祖先检查而非模型闸门),一个有效 anchor 被丢弃、退化为全量 diff——方向 fail-safe,但这恰是恢复特性本要避免的全新环境/force-push 成本。 改动:双 anchor 边界现在以完整闸门为键——两个 anchor 都在时,每一个都必须整体通过恢复闸门(有效 sha、是 fetched head 的祖先、 R2-7 —【Suggestion】reprieve 把通过的 anchor 路由到范围划分、却没有路由到相同结果(SKILL.md:147)— 已解决已核实:一个 commit 是自己的祖先( 改动:reprieve 现在把通过闸门的旁路文件 sha 完全按 备注
验证本轮实际运行的命令(运行环境:工作流在本轮之前执行了干净的
未运行的命令(附原因): 🧭 Gate advisory — this round modified areas outside the PR footprint (machine-measured, not agent-authored):
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Deferring to @wenshao — the review is clean; the hold is a policy cap, not a finding. This re-run reviewed head 中文说明⏸️ 转交 @wenshao——审查是干净的；卡住的是政策上限，不是任何发现。 本轮 re-run 审查了 head — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| typeof runtimeModelId === 'string' ? runtimeModelId.trim() : ''; | ||
| const declared = | ||
| typeof input.modelId === 'string' ? input.modelId.trim() : ''; | ||
| const certifying = runtime !== '' ? runtime : declared; |
There was a problem hiding this comment.
[Critical] R2-9: Re-check of the round-1 cross-provider identity blocker: still stands at this commit. The marker's certifying identity is still the bare runtime model string (QWEN_CODE_MODEL, with the model-written state field as the fallback) — no authType/provider/endpoint discriminator. The previous round escalated exactly this point for a maintainer decision (accept the bare runtime id and record the limitation, or track a provider-aware identity as a follow-up), and that decision is still open — so the blocker is re-asserted for this round's verdict. — Failure scenario: two provider or endpoint configurations expose the same model ID while resolving to different underlying models → the second configuration accepts the first configuration's anchor and skips code it never reviewed.
Witness: not run — the blocker is the absence of a provider discriminator in the identity; demonstrating the collision needs a live two-provider setup resolving one model id to two models. Read at 4938c6b1: both identity arms are bare model strings (runtime = QWEN_CODE_MODEL env, declared = state.modelId), and the escalation reply confirms the round-1 remedy was not implemented.
Suggested fix: persist and compare a stable resolved-model identity including the relevant auth type and provider or endpoint discriminator, using the same identity for the cache and posted-ledger gates — or record a maintainer decision accepting the bare runtime id as this PR's identity contract and track the richer identity as a follow-up.
中文说明
【Critical】 R2-9：对第 1 轮跨 provider 身份阻断项的复核：在本提交上仍然存在。marker 的认证身份仍是裸的运行时模型字符串（QWEN_CODE_MODEL，以模型自己填写的 state 字段作为回退）——没有 authType/provider/endpoint 区分信息。上一轮已就这一点升级上报、等待维护者决策（接受裸运行时 id 并记录其局限，或将 provider 感知的身份作为跟进任务追踪），该决策仍未落地——因此本条阻断项在本轮裁定中重新成立。 — 失败场景：两个 provider 或 endpoint 配置暴露相同的模型 ID、但解析为不同的底层模型 → 第二个配置会接受第一个配置的 anchor，并跳过它从未审查过的代码。
见证：未运行——阻断点是身份中缺少 provider 区分信息；要证明该碰撞需要一套真实的双 provider 环境，使同一个模型 id 解析到两个不同的模型。已在 4938c6b1 上直接阅读代码：上述两个分支都是裸模型 id（runtime = QWEN_CODE_MODEL 环境变量，declared = state.modelId），升级回复也确认第 1 轮的补救方案未实施。
建议修复：持久化并比较包含相关 auth type 与 provider/endpoint 区分信息的稳定解析模型身份，缓存与已发布 ledger 两个闸门使用同一身份——或由维护者明确决策接受裸运行时 id 作为本 PR 的身份契约，并将更丰富的身份作为跟进任务追踪。
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Still escalated for a maintainer decision — deliberately unchanged this round. The point turns on the identity contract: accept the bare runtime model id (QWEN_CODE_MODEL, typed-state fallback) and record the two-provider collision as a documented limitation, or track a provider-aware identity (auth type + provider/endpoint discriminator) as a follow-up. That call is a maintainer's, not the review loop's, so the thread stays open with the same question as rounds 1–2. One fact for the decision: the platform/provider abstraction that landed on main since the last escalation is about review platforms (GitHub vs Aone Code), not model providers — it does not settle this. No other round-3 finding touches the identity contract.
中文说明
仍在等待维护者决策——本轮刻意未做改动。这一点取决于身份契约的选择：接受裸运行时模型 id（QWEN_CODE_MODEL，以模型填写的 state 字段为回退）并把双 provider 碰撞作为已记录的局限，还是将 provider 感知的身份（auth type + provider/endpoint 区分信息）作为跟进任务追踪。该决定权在维护者而非审查循环，因此线程保持打开，问题与第 1–2 轮相同。为决策补充一个事实：自上次升级后 main 上落地的 platform/provider 抽象针对的是审查平台（GitHub 与 Aone Code），而非模型 provider，并不能解决本问题。第 3 轮其余反馈均不涉及身份契约。
| const certifying = runtime !== '' ? runtime : declared; | ||
| const model = attribution && certifying !== '' ? certifying : undefined; |
There was a problem hiding this comment.
[Suggestion] R3-1: The attribution-off suppression of the marker's model is untested for the runtime-injected identity — the new, primary channel this PR introduces. Both existing attribution-off tests omit runtimeModelId, so the interaction of the two new behaviors (runtime identity × attribution off) is pinned by no test. The current code is correct — the risk is a future edit shipping a leak with CI green. — Failure scenario: mutating the gate to (attribution || runtime !== '') && certifying !== '' leaks the session-published model into the posted marker whenever a runtime identity exists, even with review.attribution: false — yet passes all 299 compose+submit tests. Every session publishes QWEN_CODE_MODEL, so the trigger is an ordinary review run with the operator setting disabled; the wrong outcome is the model id appearing in a public, irreversible review body against the setting's contract.
Witness (probe, both arms): baseline Tests 299 passed (299); with the gate mutated → Tests 299 passed (299) (the leak ships CI-green); the regression test proposed below fails under the mutation with AssertionError: expected 'the-session-model' to be undefined and passes on the PR's actual code.
Suggested fix — add one case beside the existing attribution-off tests:
it('withholds the runtime model from the marker when attribution is off', () => {
const r = composeReview({ ...input, modelId: MODEL }, 'unknown', false, 'the-session-model');
expect(parseLedger(r.body)!.model).toBeUndefined();
expect(r.body).not.toContain('the-session-model');
});中文说明
【Suggestion】 R3-1：marker 中 model 的 attribution-off 扣下逻辑，对于运行时注入的身份——本 PR 引入的新的主通道——没有任何测试覆盖。现有的两个 attribution-off 测试都未传 runtimeModelId，因此两个新行为（运行时身份 × attribution 关闭）的交集没有任何测试钉住。当前代码是正确的——风险在于未来某次修改会在 CI 全绿的情况下把泄露带上线。 — 失败场景：把闸门突变为 (attribution || runtime !== '') && certifying !== '' 后，只要存在运行时身份，即使 review.attribution: false，会话发布的模型也会泄入已发布的 marker——而全部 299 个 compose+submit 测试仍然通过。每个会话都会发布 QWEN_CODE_MODEL，因此触发条件就是一次普通的、运维者关闭了该设置的审查运行；错误结果是在违反该设置契约的情况下，模型 id 出现在公开、不可逆的 review 正文中。
见证（probe，双向验证）：基线 Tests 299 passed (299)；突变后 → Tests 299 passed (299)（泄露在 CI 全绿下通过）；下方建议的回归测试在突变下以 AssertionError: expected 'the-session-model' to be undefined 失败，在 PR 实际代码上通过。
建议修复：在现有 attribution-off 测试旁补充一个用例（见上方代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (model === undefined || model.length <= LEDGER_MAX_MODEL) { | ||
| payload.sha = ledger.sha; | ||
| if (model) payload.model = model; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-2: The new model field adds up to ~78 bytes to every clean marker; a previously-fitting marker near LEDGER_MAX_BYTES (8192) overflows, and the byte-cap loop's first casualty is a finding — which then withholds the anchor pair anyway. Shedding the anchor pair first would keep the whole work list. — Failure scenario: a clean round whose findings serialize to roughly 8.1–8.2 KiB (this file's own comment says 8 KiB "holds the largest ledger a real round has produced") now exceeds 8192 once the model rides along: the loop drops the last-written finding, and dropped > 0 then withholds sha and model in the same render. The anchor is lost either way, but the next round additionally loses the tail finding from its work list — disclosed via dropped, not silent, yet a full-range re-review is no guarantee the dropped claim is re-found.
Witness (probe over the real serializeLedger/parseLedger, same findings list, only the model field varied):
{"cap":8192,"findingsIn":25,"pre":{"len":8192,"findings":25},"post":{"len":8116,"findings":24,"dropped":1},"proposed":{"len":8162,"findings":25}}The pre-PR shape fits exactly at the cap with anchor + all 25 findings; the post-PR shape overflows and loses one finding plus the anchor pair; shedding the anchor pair first fits (8162) with all findings intact.
Suggested fix: in the truncation loop, when overflow appears, first re-render without the anchor pair (drop sha+model, keep all findings) and only start shedding findings if that still exceeds the cap.
中文说明
【Suggestion】 R3-2：新增的 model 字段会为每个干净 marker 增加最多约 78 字节；原本恰好不超限、接近 LEDGER_MAX_BYTES（8192）的 marker 因此溢出，而字节上限循环的第一个牺牲品是一条 finding——随后 anchor 对反正也会被一并扣下。先丢弃 anchor 对就能保住完整的工作清单。 — 失败场景：一个 findings 序列化后约 8.1–8.2 KiB 的干净轮次（本文件自己的注释说 8 KiB "容纳了真实轮次产生的最大 ledger"），在 model 随行后便会超过 8192：循环丢弃最后写入的 finding，dropped > 0 又在同一次渲染中扣下 sha 与 model。无论哪种顺序 anchor 都会丢失，但下一轮还会额外丢失工作清单末尾的那条 finding——通过 dropped 披露、并非静默，可全量复审并不能保证被丢的那条会被重新发现。
见证（在真实 serializeLedger/parseLedger 上做 probe，findings 清单相同，仅变化 model 字段）：见上方 JSON——PR 前形状恰好贴线容纳 anchor + 全部 25 条 findings；PR 后形状溢出并丢失 1 条 finding 加 anchor 对；先丢 anchor 对则可容纳（8162）且 findings 完整。
建议修复：在截断循环中，出现溢出时先尝试不带 anchor 对重新渲染（丢 sha+model、保留全部 findings），仍超限再开始丢弃 findings。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // record. Kept local so this suite's wire assertion stands on its own. | ||
| const SESSION = 'SUBM'; | ||
|
|
||
| function coveredPlanAt(prNumber: number, fetchedSha: string): string { |
There was a problem hiding this comment.
[Suggestion] R3-3: The new ~140-line coveredPlanAt re-implements the coverage fixture that compose-review.test.ts already owns (plan() + transcript() + recordBuilt() + recordMatrix() + recordStep45()) instead of reusing or extracting it, so production-owned contracts (plan JSON schema, transcript JSONL shape, prompt-record layout, chunk-prompt grammar) now live in two files. Drift is already present in this diff: the original writes record filenames through encodeURIComponent(key) — matching production prompt-record.ts, with a comment saying the name must match; the copy writes raw ${key}.txt and lost that rationale. — Concrete cost: any future change to the coverage contract must be made in two files in lockstep; a future role key with characters encodeURIComponent transforms would be found by one suite and missed by the other, reddening (or silently satisfying) the wiring test for reasons unrelated to what it asserts. Today the keys (chunk-1, verify, reverse-audit, test-matrix) are identity under encoding, so nothing breaks now.
Suggested fix: extract the covered-plan fixture into a shared module parameterised by session id and dir — packages/cli/src/commands/review/lib/test-utils.ts is this folder's existing home for shared review test helpers — and call it from both suites; or at minimum carry over the encodeURIComponent filename handling and its why.
中文说明
【Suggestion】 R3-3：新增的约 140 行 coveredPlanAt 重新实现了 compose-review.test.ts 已经拥有的覆盖夹具（plan() + transcript() + recordBuilt() + recordMatrix() + recordStep45()），而不是复用或抽取它，使得生产侧拥有的契约（plan JSON 结构、transcript JSONL 形状、prompt-record 布局、chunk-prompt 语法）现在存在于两个文件中。本 diff 中已经出现漂移：原件用 encodeURIComponent(key) 写记录文件名——与生产代码 prompt-record.ts 一致，并附有名称必须匹配的注释；副本直接写原始的 ${key}.txt，丢掉了该理由。 — 具体代价：未来任何对覆盖契约的改动都必须在两个文件中同步修改；未来某个经 encodeURIComponent 变换的角色 key 会被一个套件找到、被另一个套件漏掉，使接线测试因与其断言内容无关的原因变红（或静默通过）。当前的 key（chunk-1、verify、reverse-audit、test-matrix）在编码下均为自身，所以现在不会出问题。
建议修复：把覆盖夹具抽取为按 session id 与 dir 参数化的共享模块——packages/cli/src/commands/review/lib/test-utils.ts 是该目录现有的共享测试辅助之家——供两个套件调用；至少也应带上 encodeURIComponent 文件名处理及其理由。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| - For Approve/Request changes verdicts: a review summary with the verdict | ||
| - For Comment verdict with all inline comments posted: no separate summary (inline comments are sufficient) | ||
| - Model and CLI version attribution footer on each comment (e.g., _— qwen3-coder via Qwen Code /review (v0.21.2)_); set `review.attribution` to `false` in your user or system `settings.json` (the workspace `.qwen/settings.json` is ignored for `review.*` settings) to post without it | ||
| - Model and CLI version attribution footer on each comment (e.g., _— qwen3-coder via Qwen Code /review (v0.21.2)_); set `review.attribution` to `false` in your user or system `settings.json` (the workspace `.qwen/settings.json` is ignored for `review.*` settings) to post without it — note this also withholds the model from the review's machine-ledger marker, so in fresh environments (no review cache) the recovered incremental anchor fails the same-model check and the re-review falls back to full-range |
There was a problem hiding this comment.
[Suggestion] R3-4: The same file's "Cross-model review" section (lines 319-347) documents the model-switch × incremental-review interaction for the same-SHA skip case only; the new differing-SHA full-range gate this PR adds — and its user-facing notice — are documented nowhere in user docs (SKILL.md/DESIGN.md are orchestrator-facing). — Failure scenario: before this PR, switching models and re-reviewing a PR with new commits silently scoped to lastCommitSha..HEAD; now it runs full-range and emits "Previous round was reviewed by {cached_model}. Running full review with {{model}}." A user who hits that notice looks in the section that exists precisely for this interaction — it documents the sibling same-SHA notice verbatim — and finds only "runs a full review instead of skipping", with no mention that incremental scoping and the recovered anchor are now also same-model gated. The message has nowhere to be looked up.
Suggested fix: extend the "Cross-model review" section to state that a model mismatch also forfeits the incremental range — the differing-SHA cache path and the recovered-anchor path fall back to the full diff with the findings list carrying over — alongside the existing skip case.
中文说明
【Suggestion】 R3-4：同一文件的 "Cross-model review" 小节（319-347 行）只记录了模型切换 × 增量审查在"同 SHA 跳过"一种情形下的交互；本 PR 新增的"SHA 不同则全量"闸门及其面向用户的通知，在用户文档中没有任何记载（SKILL.md/DESIGN.md 面向 orchestrator，不是用户文档）。 — 失败场景：本 PR 之前，切换模型后对有新提交的 PR 复审会静默地把范围划为 lastCommitSha..HEAD；现在会全量运行并输出 "Previous round was reviewed by {cached_model}. Running full review with {{model}}."。遇到该通知的用户去查正是为此交互而设的小节——那里逐字记载了同 SHA 的姊妹通知——却只看到 "runs a full review instead of skipping"，完全没有提及增量范围划分与恢复的 anchor 现在同样受同模型闸门约束。这条通知无处可查。
建议修复：在 "Cross-model review" 小节补充：模型不匹配同样会放弃增量范围——SHA 不同的缓存路径与恢复 anchor 路径都退回全量 diff，findings 清单照常继承——与现有的跳过情形并列。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
@qwen-code /resolve |
|
🤖 Could not produce a passing fix for this feedback (round 14/100). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/32076785809 🧠 Handled by Qwen Code · model/模型 |
…y holds R11-3: the section's RULED-FOR-YOU verdict was rendered from the ledger this run RECOVERED, while the sha Step 1 passes comes from the side file — and `persistRecoveredLedger`'s never-lower-round guard deliberately keeps a HIGHER-round file when the recovery walk comes back short (a concurrent lane, a paginated fetch that returned less than it should, a latest review deleted or edited). In that state a HOLDS about the recovered sha is obeyed against a different one, certified by whichever model ran THAT round — so the round scopes past a range only that model reviewed, permanently, since its own clean verdict re-anchors past it. Compose's drift gate cannot catch it: the re-run re-stamps under the running model, so the stamp agrees with the runtime and nothing looks wrong. The verdict now rules on what the file HOLDS, read back off disk after the persist decision rather than inferred from it — the guard's outcome is exactly the thing a caller would get wrong by reasoning about it. A divergence is a no-verdict state: both shas are named and the round reviews the full range, because nothing available here can say who reviewed the span between them. The findings still carry. Two new tests, both mutation-checked: the renderer's divergence refusal (and that agreement, and a file holding no anchor, still rule normally), and `persistedAnchorSha` reading back what the guard actually kept — the second is what fails when the read-back is stubbed out, which the renderer test alone could not see.
R11-3 fixed —
|
…md (QwenLM#9371) The growth brake tells the address-review agent to stop with a handoff but never names the output file, so round 14 on QwenLM#9184 wrote handoff.md — a file owned by run-agent.mjs that the verdict gate does not accept. The round was reported as missing its required outputs even though the defer-to-human was correct (run 32076785809). Name failure.md (the stop file run-agent.mjs wraps into the handoff comment) as the handoff target, forbid agent-written handoff.md, and pin the directive in the workflow contract test.
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.
7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- F1 tautological provider-separation assertion at compose-review.test.ts:4875 — already reported (comment 3792869885, R9-8)
- F2 round-start reviewModelId stamp untested at fetch-pr.ts:1288 — already reported (comment 3792869900, R9-13)
- F4 DESIGN.md:498 names the wrong env var as the marker's identity carrier — already reported (comment 3792869891, R9-11)
- G8b fetch-pr.ts:183 field doc misnames the certifying identity's source — already reported (comment 3792869893, R9-11)
- G7 boundary wiring tests never set QWEN_CODE_MODEL_IDENTITY at submit.test.ts:1466 (+1 location) — already reported (comment 3792869882, R9-7)
- G9 code-review.md:348 quoted cross-model note exists nowhere (incl. the dangling '(below)') — already reported (comment 3792869896, R9-12 / R11-8)
- J3 authType discriminator unpinned at config-session-env.test.ts:366 — already reported (comment 3792869903, R9-15)
Not reviewed: reverse audit — reached the 5-round cap without converging (rounds 3, 4 and 5 each reported new findings; every round's findings were verified before composing).
Not explored to full depth (tool budget reached): chunk 7: executing submit.test.ts (and config-session-env.test.ts ) to confirm the fixture passes the gate at runtime — no node_modules exists in the review worktre….
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/pr-context.ts:1628 — [review] persistedSha wiring untested at both jointspackages/cli/src/commands/review/pr-context.test.ts:189 — [review] buildMarkdown wiring test never exercises the blanked-identity bare-id fallbackpackages/cli/src/commands/review/compose-review.ts:856 — [review] runtimeModelId JSDoc names the wrong env slot (new location of the R9-11 defect class)packages/core/src/config/config.ts:4326 — [probe] qualified per-session identity registry write untested at the production jointdocs/users/features/code-review.md:350 — [review] new paragraph contradicts the unchanged rebased-away sentence three lines belowpackages/cli/src/commands/review/pr-context.ts:1171 — [probe] parseable sha-less higher-round side file slips past the divergence gatepackages/cli/src/commands/review/pr-context.ts:1176 — [probe] divergence wording false for the same-round higher-reviewId keeppackages/cli/src/commands/review/pr-context.ts:1171 — [probe] certifier-only divergence slips past the sha-only gate
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 7 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — reached the 5-round cap without converging (rounds 3, 4 and 5 each reported new findings; every round's findings were verified before composing)。
未探索到全部深度（达到工具调用预算）：chunk 7：executing submit.test.ts (and config-session-env.test.ts ) to confirm the fixture passes the gate at runtime — no node_modules exists in the review worktre…。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
|
||
| - **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 it to the fetch as `--since <lastCommitSha>`. **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. **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. | ||
| - **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` **whose `lastModelId` equals `{{model}}`**, pass it to the fetch as `--since <lastCommitSha>`. **A cached anchor from another model is not passed at all** — an absent `lastModelId` counts as a mismatch, on caches written before the field. "Clean up to `lastCommitSha`" is that model's verdict, and `fetch-pr` validates the anchor against the HISTORY, not against who certified it: hand it a cross-model anchor and it rules `effective`, scoping this round past code `{{model}}` never reviewed — permanently, because this round's own cache write re-anchors past it. 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}}`): |
There was a problem hiding this comment.
[Critical] R12-1: The cache-path same-model gate compares BARE model ids on both sides — Step 8 writes "lastModelId": "{{model}}" (the bare config.getModel(), SKILL.md:1280; BundledSkillLoader.ts:109-113), and this gate compares it to the current bare {{model}} — so two provider configurations exposing the same model name pass each other's gate: the exact case the recovery path in this same PR rejects via whole-string equality on provider-qualified identities (certifierMatchesRound; round-model.test.ts pins certifierMatchesRound('m@9f8e7d6c', 'm@1a2b3c4d')).toBe(false)). — Failure scenario: round N under provider P1's qwen3.7-max caches lastModelId: "qwen3.7-max" + sha S; the user switches to provider P2 (different baseUrl/authType, same model name — this PR's own motivating case); round N+1's gate matches on the bare name and passes --since S → fetch-pr validates history only and rules effective → the round scopes S..HEAD past code only P1 reviewed, and its own clean round re-anchors past it permanently. Had the cache been absent (CI machine), the identical switch is caught by the recovery path. DESIGN.md:498's carve-out argues only a FORGED cache id, not this honest-write/honest-compare pass-through.
Witness: not run — the gate is prompt text applied by the orchestrating model; the runnable sub-facts are settled by quoted code (BundledSkillLoader.ts:109-113: {{model}} = bare config.getModel(); SKILL.md:1280 bare write; SKILL.test.ts:175 pins the bare-comparison wording).
| - **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` **whose `lastModelId` equals `{{model}}`**, pass it to the fetch as `--since <lastCommitSha>`. **A cached anchor from another model is not passed at all** — an absent `lastModelId` counts as a mismatch, on caches written before the field. "Clean up to `lastCommitSha`" is that model's verdict, and `fetch-pr` validates the anchor against the HISTORY, not against who certified it: hand it a cross-model anchor and it rules `effective`, scoping this round past code `{{model}}` never reviewed — permanently, because this round's own cache write re-anchors past it. 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}}`): | |
| when it holds a `lastCommitSha` **whose `lastModelId` equals the session-published identity (`$QWEN_CODE_MODEL_IDENTITY`, falling back to `{{model}}` when blank)**, pass it to the fetch as `--since <lastCommitSha>` |
(Pre-existing bare-valued caches then fail safe — one full re-review until the next clean round rewrites the cache; alternatively, have fetch-pr read the cache and rule this gate in code, where the qualified identity is already available.)
中文说明
【Critical】 R12-1：缓存路径的同模型闸门两侧比较的都是裸模型 id——Step 8 写入 "lastModelId": "{{model}}"（裸的 config.getModel()，SKILL.md:1280；BundledSkillLoader.ts:109-113），本闸门又把它与当前的裸 {{model}} 比较——因此两个暴露相同模型名的 provider 配置会互相通过对方的闸门：而这正是本 PR 恢复路径上用 provider 限定身份的整串相等（certifierMatchesRound）明确拒绝的情形（round-model.test.ts 钉住了 certifierMatchesRound('m@9f8e7d6c', 'm@1a2b3c4d')).toBe(false)）。— 失败场景：provider P1 的 qwen3.7-max 下运行的第 N 轮缓存了 lastModelId: "qwen3.7-max" + sha S；用户切换到 provider P2（baseUrl/authType 不同、模型名相同——本 PR 自己的动机场景）；第 N+1 轮的闸门按裸名匹配、传入 --since S → fetch-pr 只校验历史并裁定 effective → 本轮范围取 S..HEAD，跳过了只有 P1 审过的代码，且本轮自己的干净轮次会把锚点永久地前移。若缓存不存在（CI 机器），同样的切换会被恢复路径拦下。DESIGN.md:498 的豁免只论证了伪造缓存 id 的情形，未覆盖这种诚实写入 + 诚实比较的直通。
见证：未运行——闸门是 orchestrating 模型应用的提示词文本；可运行的子事实已由引用的代码证实（BundledSkillLoader.ts:109-113：{{model}} = 裸 config.getModel()；SKILL.md:1280 裸写入；SKILL.test.ts:175 钉住裸比较措辞）。
**建议修复：**让 Step 8 持久化会话发布的限定身份、闸门对其比较（$QWEN_CODE_MODEL_IDENTITY，为空时回退 {{model}}，镜像 roundModelIdFrom 的优先序）；或让 fetch-pr 读取缓存并在代码里裁定该闸门（那里已可取得限定身份）。既有的裸值缓存则失败安全（一次完整重审）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const identityDrifted = | ||
| roundStart !== '' && runtime !== '' && roundStart !== runtime; |
There was a problem hiding this comment.
[Critical] R12-2: The drift gate disengages whenever the post-time runtime channel is blank — even when the plan's reviewModelId stamp proves the round STARTED under a session-published identity — so certifying falls back to the model-written input.modelId, the exact channel this diff's docstrings retire ("never with the model-written state field alone"). The recovery side (certifierMatchesRound) rules an empty running as always-mismatch; the certifying side skips on the same emptiness with provable disagreement available (roundStart !== declared is checkable — or withhold, as the fix below does). — Failure scenario: round starts under A → fetch-pr stamps reviewModelId: 'A@digest'; the post then runs where the runtime slots are empty (a standalone/deferred qwen review submit from a terminal outside a session shell — round-model.ts documents the empty case as reachable in normal operation) → identityDrifted is false regardless of the stamp → the marker certifies the sha to the typed id → a later round under a matching typed id passes certifierMatchesRound and scopes sha..HEAD past code it never reviewed — the regression this PR exists to close.
Witness: probe over the real composeReview — plan stamped model-a@aaaaaaaa, runtimeModelId: '', typed model-b → observed {"sha":"deadbeef00112233","model":"model-b"}; flip-patch roundStart !== '' && (runtime === '' || roundStart !== runtime) → {} (pair withheld), both controls unchanged.
| const identityDrifted = | |
| roundStart !== '' && runtime !== '' && roundStart !== runtime; | |
| const identityDrifted = | |
| roundStart !== '' && (runtime === '' || roundStart !== runtime); |
(Fail-safe cost: one full re-review for same-model out-of-session posts; alternatively, document in this comment why a stamped round with a dead runtime channel may trust the typed field.)
中文说明
【Critical】 R12-2：当发布时刻的运行时通道为空时,漂移闸门整体失效——即使 plan 的 reviewModelId 戳已证明本轮开始于某个会话发布的身份——于是 certifying 回退到模型自己写入的 input.modelId,而这正是本 diff 的 docstring 明确废黜的通道（「绝不能仅凭模型写入的 state 字段」）。恢复侧（certifierMatchesRound）把空 running 裁为必定不匹配;认证侧却在同样为空、且存在可证明的分歧（roundStart !== declared 可查——或如下方修复直接扣下）时直接跳过。— **失败场景：**轮次在 A 下开始 → fetch-pr 打上 reviewModelId: 'A@digest';随后发布运行在运行时槽为空的环境（会话 shell 之外的终端里独立/延迟执行 qwen review submit——round-model.ts 明确该空值情形在正常操作中可达）→ 无论戳为何值 identityDrifted 恒为 false → marker 把该 sha 认证到模型手写的 id → 之后手写 id 相同的一轮通过 certifierMatchesRound、范围取 sha..HEAD,跳过它从未审过的代码——正是本 PR 要堵的回归。
见证：对真实 composeReview 的探针——plan 打戳 model-a@aaaaaaaa、runtimeModelId: ''、手写 model-b → 观测到 {"sha":"deadbeef00112233","model":"model-b"};补丁翻转为 roundStart !== '' && (runtime === '' || roundStart !== runtime) → {}（锚点对被扣下）,两个对照均不变。
**建议修复：**见上方 suggestion 块（失败安全代价：同模型的会话外发布多付一次完整重审;或者在此注释中说明:为何带戳但运行时通道为死的轮次可以信任手写字段）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
🤖 AutoFix stopped after 5 consecutive rounds that failed to push anything (timeouts and/or gate rejections). Retrying at the same per-round budget is not converging — this usually means the PR is too large or conflicts with a fast-moving What I found before stopping: PR #9184 is not converging — maintainer decision requiredThe workflow's convergence brake has engaged: this counting window has been over The decision neededHow to proceed with PR #9184 (review-anchor model gate): split it, redesign Evidence: one defect class keeps reappearing on code earlier rounds addedThe PR wires a provider-qualified model identity through four boundaries Run log: https://github.com/QwenLM/qwen-code/actions/runs/32099558400 🧠 Handled by Qwen Code · model/模型 |
R12-1 and R12-2 are the sixth and seventh findings in one class — two
boundaries meaning different strings by the round's identity — so these
close the class rather than the two instances.
R12-1: the cache-path gate compared BARE ids on both sides. Step 8 writes
`lastModelId: "{{model}}"` and the gate compared it to `{{model}}`, both
the bare `config.getModel()`, so two provider configurations exposing one
model name passed each other's gate — the exact case the recovery path in
this PR rejects. Self-consistent is not sound; it was consistently wrong
across providers, and I deferred it last round as an asymmetry when it was
a hole.
The gate moves into `fetch-pr`, beside the one the anchor already goes
through: `--since-model` carries WHO certified the anchor, the skill
copies both fields verbatim, and `certifierMatchesRound` — the same
function the marker-recovery ruling uses — decides. A mismatch reports
`cross-model-anchor` and reviews the full range, refused before the
history is consulted at all.
That leaves ZERO identity comparisons in prompt text. Six rounds have each
closed one channel and the next round found another; the reason the class
kept regenerating is that a comparison written in prompt text cannot
share the CLI's notion of the string, and `{{model}}` is structurally the
wrong one — it interpolates the bare id where everything the CLI records
is provider-qualified. The SKILL guard now asserts the absence, not just
the presence: no `lastModelId equals`, no `model matches`/`model differs`.
R12-2: the drift gate disengaged whenever the post-time runtime channel
was blank, even with the plan's stamp proving the round STARTED under a
published identity — so `certifying` fell back to the model-written
`input.modelId`, the channel these docstrings retire. The recovery side
already rules an empty running identity a mismatch; the certifying side
does now too. An UNSTAMPED round still keeps its old behaviour, because
it cannot prove disagreement either.
Two new tests, both mutation-checked.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — blocked (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: 3560 passed · 0 failed · 3560 total 中文 — 判定：❌ 不通过 · 阻塞（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：3560 通过 · 0 失败 · 3560 总计 Verification reportPR #9184 Deep Verification (round 2) —
|
| # | finding (round 1) | severity | status at 411287758d |
re-measurement |
|---|---|---|---|---|
| F1 | SKILL.md gate hunks pinned by no test (M8 survived) | Suggestion | fixed | Reverting SKILL.md to the base text now reddens 2 tests, incl. the new pins the same-model gate on both incremental-anchor paths (21/21 unmutated control → 19/21 reverted). Agree with round-1 classification. |
| F2 | submit boundary identity injection unpinned (M6 survived) |
Suggestion | fixed | Deleting roundModelIdFrom(process.env) from runSubmit's compose call now reddens exactly injects the session model into the posted marker — QWEN_CODE_MODEL reaches the wire (wiring) (68/68 unmutated control → 1 red reverted). |
Both were re-measured by mutation at the new head, not diffed off the old report.
Central claim + A/B
Central claim: an incremental anchor can no longer scope a round under an identity that did not review it — the marker carries the certifying identity end-to-end, the cache path hands its certifier to the CLI, and the gate is ruled inside fetch-pr / pr-context (never in prompt text), with absent/blank counting as mismatch.
The A/B drives the compiled dist/ of each arm through four seams; base expectations encode the hole (expected failures are passes). Witnesses: 01-fetch-gate-ab-head-vs-base.png, 02-compose-drift-gate-ab-head-vs-base.png, 03-marker-verdict-ab-head-vs-base.png.
Table 1 — fetch-pr --since-model gate (real fetchPrCommand.handler, mocked git/gh only; witness 01):
| cell (same scenario both arms) | head | base |
|---|---|---|
anchor certified by another provider (same-name@other-digest) |
refused cross-model-anchor, before any history probe; full-range diff still produced; stderr names both identities |
scoped effective: true (the hole) |
| anchor nobody certified (pre-field cache) | refused | scoped (the hole) |
| bare id vs qualified runner | refused | scoped |
| same qualified identity (positive control) | scoped effective: true |
scoped |
Head 4/4, base 4/4. Subprocess wiring: review fetch-pr --help exposes --since-model at head, not at base.
Table 2 — compose drift gate + identity carriage (real composeReview over covered-plan fixtures; witness 02):
| cell | head | base |
|---|---|---|
| aligned stamp | marker certifies with the runtime identity | bare anchor (hole) |
| runtime supersedes typed state-JSON id | ✓ | n/a |
| reviewed as A, posting as B | PAIR withheld; findings still post | bare anchor posted (hole) |
| stamped, BLANK runtime (terminal post) | PAIR withheld | bare anchor (hole) |
| unstamped plan, blank runtime | declared id certifies (today's behaviour kept) | bare anchor |
| attribution off | sha posts WITHOUT certifier | same shape |
Head 6/6, base 6/6.
Table 3 — marker round-trip + rendered verdict (pure dist functions; witness 03): head 44/44, base 17/17. Covers: model rides beside sha and drops with it (invalid sha, over-cap pair-withhold at 64/65 boundary, truncated-round withhold); cross-model / same-model / legacy / divergence / unpublished-runner verdicts rendered with both identities named; roundModelIdFrom (identity preferred, blank→bare fallback, neither→'') and certifierMatchesRound (whole-string, every unknown a mismatch) exist only at head; base's rendered section hands the anchor over with no model condition.
Table 4 — shell identity channel (core dist): head 13/13, base 6/6. Per-session registry passes the qualified identity down; a session with none gets '' written, so the spawn-site {...process.env, ...} spread cannot leak a stale parent value (base leaks it — hole); the global fallback is guarded (identityDescribes, suffix-anchored, @-in-model handled); unregister drops model+identity together.
Load-bearing totals: head-side gate-decidable scenarios 2/2 at the fetch seam and 6/6 at compose; base structurally 0. The mutation matrix (witness 04-mutation-matrix-live.png, below) shows every new guard's kill set.
Corrections
- Description vs measured mechanics — "incremental scoping resumes from the next posted round" does not hold. The Risk & Scope section states the first round treats pre-field anchors as mismatches and scoping resumes from the next posted round. Measured: the recovery re-run is instructed to pass
--since <sha>only (SKILL.md's single--since-modelmention is the cache path), and an uncertified--sinceis refused by the gate (Table 1, cell 2) — so the recovery path's scoping never engages as instructed either. See F3. - Commit
c16d871's "the cache path … is self-consistent" was invalidated by the final commit. Atc16d871the cache-path gate lived in prompt text comparinglastModelId(bare) to{{model}}(bare) — self-consistent.411287758dmoved the comparison intofetch-pr, where the running side isroundModelIdFrom(process.env)— provider-qualified whenever the session publishes an identity. The writer side still emits the bare id. The two sides are now different identity spaces; the gate refuses the legitimate same-model case (F3).
Findings
F3 (blocking) — the gate's certifier space matches neither production writer: same-model incremental review is dead
Mechanism. The gate is certifierMatchesRound(sinceModel, roundModelIdFrom(process.env)) in fetch-pr — whole-string equality. The running side is the provider-qualified identity <model>@<8hex(authType␀baseUrl): Config.publishModelEnv registers it per session, and shellExecutionService spreads getShellContextEnvVars() into every shell subprocess (both proven live: Table 4, and the M10 kill). The certifier side comes from two writers, and neither can produce that string:
- Cache path — Step 8's template is
"lastModelId": "{{model}}"(SKILL.md L1280), and{{model}}is the BAREconfig.getModel()— the PR's own DESIGN.md (L723) and SKILL text state this, and no substitution mechanism exists anywhere in the repo (grepped). Step 1 passes it verbatim as--since-model. - Recovery path — the re-run instruction is "re-run the
fetch-prcommand from above with--since <sha>";--since-modelappears exactly once in SKILL.md (cache path). The side file carries the qualifiedmodel, but nothing tells the orchestrator to copy it.
Dynamic proof. Table 1 cells 2–3: bare certifier vs qualified runner → cross-model-anchor at head while base SCOPES the identical scenario; absent certifier → refused at head. The PR's own fetch-pr suite never exercises the production writer's shape — its fixtures pass qualified certifiers (the accept-path-against-real-artifacts gap).
Blast radius (every session that publishes a qualified identity — i.e. any real provider config, since resolvedModelIdentity digests whenever authType or baseUrl is non-empty):
- Cache-path incremental scoping (
differing-SHAbranch) never engages: every high-effort round re-reviews the full diff, and re-caches the bare id, forever. - The same-SHA
upToDate"No new changes since last review" stop is unreachable: the cross-model refusal short-circuits beforeresolveIncrementalAnchorcomputesupToDate(proven: nocat-file/merge-baseprobe ran in the refused cells). Re-reviewing an unchanged PR is now always a full review — the exact cost class DESIGN.md measured at 119 min / 34M tokens for one round. - Recovery-path scoping never engages as instructed (uncertified
--sincerefused), contradicting the description's resume claim.
Bounds. Fails safe in every direction — no cross-model scope-skip is possible; the refusal is loud on stderr (Incremental anchor not used — it was certified by "X", and this review runs as X@<digest>); findings/work-list carriage is unaffected; low/medium effort never consulted the cache. The regression is cost/feature, not correctness — but it defeats the payoff this PR exists for, on the happy path, indefinitely.
Repro (scripted): harnesses/__verify_fetch_ab.test.ts cells "bare model id vs provider-qualified runner is NOT a match" and "uncertified anchor" (head refuses / base scopes), plus git grep -n "since-model" packages/core/src/skills/bundled/review/SKILL.md (1 hit — cache path only) and L1280's "lastModelId": "{{model}}".
Suggested fix direction (unmeasured — new surface; do not merge as-is)
The cache must carry the qualified identity, which the model cannot know — so a CLI writer is required: e.g. a review cache-write pr-<n> step (called by Step 8) that stamps roundModelIdFrom(process.env) beside lastCommitSha, mirroring how compose-review certifies the marker; and the SKILL's recovery re-run must pass --since-model copied verbatim from the side file's model (the fetch-pr --since-model help already anticipates "the model beside the marker sha"). Do NOT bridge by prefix-matching a bare certifier against a qualified runner — that is exactly the two-providers-one-name hole the digest closes. The un-instructed recovery re-run could alternatively be made safe by having fetch-pr read the side file itself when --since-model is absent, but that widens the command's trust surface; the copy-two-fields shape is cheaper.
F4 (Suggestion) — the reviewModelId stamp write is pinned by no test (M11 survived)
Removing ...(roundModelId ? { reviewModelId: roundModelId } : {}) from the fetch report leaves fetch-pr.test.ts 133/133 green, and no other suite (cli, core, integration) mentions reviewModelId on the write side. The READ side is pinned (compose-review tests drive the drift gate from a stamped plan fixture), so a regression that drops the stamp would silently disarm the drift gate — the deferred-post hole (reviewed as A, /model to B, post) reopens with every suite green. Same class as round-1's F2. Fixture that would pin it: a reportFor({}) assertion that report.reviewModelId equals the env identity.
F5 (nit) — the runtime === '' || clause in identityDrifted is provably redundant
M4 (delete the clause) survives 280/280 because roundStart !== '' already implies roundStart !== runtime when runtime is ''; M4b (disable the whole check) kills 2 named tests, so the behaviour is pinned — the clause is documentation, not logic. Harmless; noted so a future "simplification" isn't read as a regression.
Mutation matrix (live; witness 04-mutation-matrix-live.png)
| mutant | guard | suite | result | classification |
|---|---|---|---|---|
| M1 | certifierMatchesRound empty==empty passes |
round-model | KILLED (1) | pinned |
| M2 | roundModelIdFrom absent-only fallback |
round-model | KILLED (1) | pinned |
| M3 | fetch-pr gate disabled | fetch-pr | KILLED (1) | pinned |
| M4 | drop runtime === '' || clause |
compose-review | SURVIVED | dead code (redundant; see F5) |
| M4b | drift check disabled entirely | compose-review | KILLED (2) | positive control for M4's suite |
| M5 | omit identity key instead of blank | shellContextEnv | KILLED (6) | pinned |
| M10 | identity loses provider digest | config-session-env | KILLED (2) | pinned |
| MF1 | SKILL.md gate text reverted to base | SKILL.test | KILLED (2) | pin for F1's fix |
| MF2 | submit injection removed | submit | KILLED (1) | pin for F2's fix |
| M11 | reviewModelId stamp write removed |
fetch-pr | SURVIVED | coverage gap → F4 |
Controls green first (cli 141, core 58). Every killed mutant failed the intended assertion (names quoted in the witness). Survivors adjudicated: M4 dead code, M11 coverage gap; neither mirrors a pre-existing gap.
Not covered
- Orchestrator compliance — F3's writer-side links ("the model writes the bare
{{model}}"; "the recovery re-run passes no certifier") are static: the template line, DESIGN.md's{{model}}=config.getModel(), the absence of any substitution code, and the single--since-modelmention. The gate's behaviour for those inputs is dynamic (Table 1). Whether a real orchestrator improvises--since-modelfrom the side file is model behavior, unreachable from code on either arm. - Real GitHub posting (
review submitagainst the wire) — not driven; the compose seam it calls is covered (Table 2). - Per-commit attribution — depth-2 checkout; verified the aggregate
HEAD^1..HEADdiff; per-commit claims taken from messages, not isolated trees. - Repo-wide vitest beyond the targeted packages (typecheck ran repo-wide, clean); macOS/Windows (no platform-specific paths in the diff).
- The
upToDateregression cell is proven by the refusal's short-circuit (no ancestry probes ran), not by a dedicated upToDate fixture on both arms.
Methodology
Environment: node:22-bookworm CI container, merge-ref checkout at depth 2; npm ci + npm run build pre-run at HEAD. Base arm: git worktree add tmp/base-tree HEAD^1; PR touches no package.json/lockfile, so root node_modules was reused, with @qwen-code/* links re-pointed into the base tree and realpath-asserted (readlink -f node_modules/@qwen-code/qwen-code-core → base tree) before any base cell ran; base core+cli rebuilt there (nested per-package node_modules symlinked, since ajv@8 etc. are not hoisted). Harnesses are mock-free w.r.t. the units under test: compiled dist modules, the real fetchPrCommand.handler/composeReview over on-disk fixtures replicating the repo's own covered-plan layout (git/gh mocked as external systems, same seam the package suite uses). Mutation matrix applied/reverted per row in the main tree, scoped vitest runs, tree verified clean after. Raw logs in logs/; harnesses in harnesses/ + *.mjs/*.sh. Evidence rendered with scripts/verify-capture.mjs. Assertion count = gate tests (cli review 3362 + core 65 + SKILL 21) + harness checks (61 marker + 19 shell) + harness vitest tests (20) + wiring probes (2) + matrix rows as classified (10) = 3560.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
R12-1 and R12-2 fixed — and the class they belong to is closed
The root cause is structural, and it is prompt textA comparison written in prompt text cannot share the CLI's notion of the string. Worse, the only model value a skill body can interpolate is R9-1 moved the recovery gate into the CLI for exactly this reason. R12-1 is the last one left, and it moves too: There are now zero identity comparisons in prompt text. The SKILL guard asserts the absence, not just the presence: expect(body).not.toMatch(/`lastModelId` equals/);
expect(body).not.toMatch(/model matches|model differs/);That is what makes this structural rather than a seventh point fix: the class cannot regenerate in a place that no longer exists. Net effect on the diff is close to flat — the cache bullet shrank by roughly what the flag and its gate added. R12-2 — the certifying side skipped where the recovery side refusesCorrect, and it is the same asymmetry in miniature. With the post-time channel blank — a deferred On the convergence questionMy read, for whatever it is worth to the decision: the growth was findings-driven, and the findings were one class with one root. With the prompt-text surface gone I would expect the class to stop rather than produce an eighth instance. If it does not, that is evidence for splitting rather than for another round. If you would rather split regardless, the natural seam is Two new tests, both mutation-checked. 中文说明R12-1、R12-2 已修,并且关掉了它们所属的那一类。收敛评论的诊断是对的:R9-7/R9-11/R9-13/R9-15、R11-8、R12-1、R12-2 是同一类——两个边界对「本轮身份」理解成不同的字符串——每轮关掉一条通道,下一轮就找到下一条。 根因是结构性的:写在提示词里的比较无法与 CLI 共享同一个字符串概念,而技能正文唯一能插值的模型值 R9-1 已经把恢复路径的门禁移进 CLI,R12-1 是最后一处: R12-2 是同一种不对称的缩影:发布侧通道为空时跳过检查,于是回退到模型自己写的字段。恢复侧早已把空判为不匹配,现在认证侧也一样。 关于收敛:增长由发现驱动,而发现是同一类同一个根因。提示词表面消失后我预期这一类会停止;若仍出现第八例,那才是应该拆分的证据。若你倾向拆分,我建议改选「接受并延后尾部」——两半单独没有意义。这个决定我没有代你做。 |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-dev-bot
added
the
autofix/needs-human
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
#9184 landed as a SQUASH, so git cannot see that this branch already contains its commits and replays them as conflicts. Every line of main's side is present here already — verified line by line, not assumed — so all four hunks take this branch's version, which is a superset: - fetch-pr.ts: main has nothing at either site (the `computeIncrementalScope` import and `lineage-unfollowable` are this PR's). - SKILL.md: the two sides differ only in the `effective: true` bullet, where this PR's slicing/widening description supersedes the squash's `(since..head)`. - fetch-pr.test.ts: the spelling reconciled when 9184 was merged in here — `ruling(...)` rather than `.incremental` — plus this PR's own slicing tests.
|
Released in v0.21.14. |
5 participants
What this PR does
Incremental review's "skip what the previous round already cleared" shortcut is a same-model contract: "clean up to this commit" is one model's verdict. The local cache has always enforced that — a same-SHA re-run under a different model gets a full second opinion instead of a skip — but the anchor recovered from the posted review's machine ledger shipped bare, with no record of who certified it. This PR makes the ledger marker carry the certifying model beside the anchor and teaches the incremental checks to refuse a cross-model anchor:
Why it's needed
Without the gate, a round run under model B that recovered model A's anchor scoped its review to changes since A's round — leaving everything before that anchor reviewed by no run of B, permanently, because B's own clean round would re-anchor past it. The cache path already refused the same-SHA shortcut across models, so the cross-model hole existed only on the recovery path that every fresh environment (CI, another clone, a colleague's checkout) takes — precisely where nothing else could catch it.
Reviewer Test Plan
How to verify
Unit suites cover the three seams: the marker round-trip keeps the model beside the anchor and drops it whenever the anchor is withheld, truncated, or fails validation; a clean composed round's posted body carries the model while a capped round withholds both; and the rendered context section names the certifying model and the gate, absent-counts-as-mismatch included. Run the review command suites in
packages/cliand the bundled-skill guard tests inpackages/core(npx vitest run src/commands/reviewand the skills test respectively). For end-to-end behavior: post a high-effort review round on a test PR, then re-run under a different model id from a clean checkout — the run should announce a full review instead of scoping to the recovered anchor, while still ruling on the previous round's findings.Evidence (Before & After)
N/A (no UI change).
Tested on
Environment (optional)
Unit tests only (vitest); no daemon or TUI involved.
Risk & Scope
Linked Issues
None.
中文说明
本 PR 做了什么
增量审查的"跳过上一轮已确认干净的部分"这一捷径,本质上是一个同模型契约:"截至该 commit 是干净的"只是某一个模型的裁定。本地缓存一直在强制执行这一点——同一 SHA 换模型重跑会得到一次完整的第二意见审查,而不是跳过——但从已发布 review 的机器 ledger 中恢复的 anchor 之前是"裸"的,不记录是谁认证的。本 PR 让 ledger marker 在 anchor 旁携带认证模型,并让增量检查拒绝跨模型的 anchor:
为什么需要
没有这道闸门时,模型 B 的轮次若恢复了模型 A 的 anchor,就会把审查范围缩小到 A 那一轮之后的变更——anchor 之前的所有代码将永远没有被 B 的任何一轮审查过,因为 B 自己的干净轮次还会把 anchor 继续向前推。缓存路径本来就拒绝跨模型的同 SHA 捷径,所以这个跨模型漏洞只存在于恢复路径上——而 CI、另一台机器、他人的 checkout 等所有全新环境走的恰恰是这条路径,也正是没有其他机制能兜底的地方。
审阅者测试计划
如何验证
单元测试覆盖三个接缝:marker 往返序列化在 anchor 旁保留模型,并在 anchor 被扣下、截断或校验失败时一并丢弃;干净轮次组装出的发布正文携带模型,被 cap 的轮次两者都不携带;渲染出的 context 小节写明认证模型与闸门规则,包括"缺失按不匹配处理"。分别运行
packages/cli的 review 命令测试套件与packages/core的 bundled skill 守护测试(npx vitest run src/commands/review及相应 skills 测试)。端到端行为:在测试 PR 上发布一轮高 effort 审查,然后从干净 checkout 换一个模型 id 重跑——运行应宣布进行完整审查而不是缩小到恢复的 anchor,同时仍对上一轮的 findings 逐条裁定。证据(前后对比)
N/A(无 UI 变更)。
测试平台
Linux ✅;macOS/Windows⚠️ (单元测试与平台无关)。
环境(可选)
仅单元测试(vitest);不涉及 daemon 或 TUI。
风险与范围
关联 Issue
无。