Repository navigation
feat(review): audit the applied --fix for unpinned new assumptions - #10169
Conversation
Step 6B applied findings to the working tree and forbade re-running the review over the result, so `--fix` output shipped with no independent check at all — while the fix round is the loop's largest single source of its own next round (a third of post-first-round findings, #9578), and PR #9793's two fix-introduced Criticals survived a mutation probe of the intended fix sites. Add a scoped fix audit, not a re-review: `review fix-delta` records the working tree before the first edit (a tree object through a throwaway index — the user's index and the stash stack are never touched) and diffs the tree against it afterwards, with the review's own side files excluded; `agent-prompt --role fix-audit` renders the `fixed` findings above those hunks into one digest-keyed list file and prints the launch block for a single agent whose brief asks one question per hunk — what does this edit newly assume, and does anything in the tree pin it — and reports only the unpinned. The role reads no diff, carries no finder machinery and no project rules, produces no verdict, and its output is a disclosure (the affected finding's outcomeNote plus a terminal block), never a finding. The command refuses an artifact without outcomes, one with no `fixed` finding, and empty hunks beside a ledger that claims a fix. Closes #10154
github-actions
Bot
added
the
review/self-reported
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 /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 冲突，直到移除标签或达到轮次上限。移除 |
Co-authored-by: Qwen-Coder <[email protected]>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round summary — PR #10169Two Criticals and six Suggestions resolved in code this round; five test-coverage Suggestions deferred to the next round under the per-round batch bound (see the replies on their threads). The review-body Test Plan item is declined with evidence below. Criticals — resolved[rc:3864817485] The submodule blind spot ( [rc:3864817496] Suggestions — resolved[rc:3864817503] stderr summary re-parses the rendered patch ( [rc:3864817542] [rc:3864817525] The interactive path promises an audit that cannot run ( [rc:3864817511] DESIGN.md invariant wording ( [rc:3864817597] Step 6B guard leaves producer paths unpinned ( [rc:3864817610] Note-propagation halves unpinned ( Suggestions — deferred to the next round[rc:3864817555] (hostile Review body — declined with evidence[rv:5032883255] "Test Plan (not a blocker)" listing five test files as Verification
中文说明评审轮次总结 — PR #10169本轮在代码中解决了两个 Critical 和六条 Suggestion；五条测试覆盖类 Suggestion 按每轮批次上限顺延至下一轮（见各自线程上的回复）。评审正文中的 Test Plan 条目以下文证据予以拒绝。 Critical — 已解决[rc:3864817485] 子模块盲区（ [rc:3864817496] Suggestion — 已解决[rc:3864817503] stderr 摘要重新解析渲染后的补丁（ [rc:3864817542] 单次运行的 fix auditor 被允许 [rc:3864817525] 交互路径承诺了无法运行的审计（ [rc:3864817511] DESIGN.md 不变量措辞（ [rc:3864817597] Step 6B 守卫未钉住生产端路径（ [rc:3864817610] 备注传播的两半未被钉住（ Suggestion — 顺延至下一轮[rc:3864817555]（敌意 评审正文 — 以证据拒绝[rv:5032883255] "Test Plan（非阻断）" 将五个测试文件列为 验证
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/模型 |
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Round 2 — PR #10169 review feedback addressedCommit: Feedback dispositionsCritical — resolved in code (reproduced before fixing):
Suggestions — resolved in code (7):
Deferred to round 3 via thread replies (8 — per-round batch cap ~8, Critical/Required first): Recorded, not actionable / not requested this round:
Mutation probes (every new guard has a red witness)
Verification
中文说明第 2 轮 — PR #10169 评审反馈处理提交： 反馈处置Critical — 已在代码中解决（先复现后修复）：
Suggestion — 已在代码中解决（7 条）：
经线程回复推迟到第 3 轮（8 条——每轮批次上限约 8 条，Critical/Required 优先）： 已记录、本轮不可处理/未被要求处理：
变异探针（每个新守卫都有变红见证）
验证
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/模型 |
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Round 3 address summary — PR #10169Commit: Feedback dispositionsCritical findings — all four reproduced on git 2.39.5, fixed, and pinned
Suggestions
Deferred to the next round (batch cap, Criticals first)
Review body [rv:5036894641]The four items the reviewer itself deferred ("recorded, not requested in this round") were left untouched as instructed. The Test Plan "no such file or directory" notes reflect reviewer-side paths missing the A note on the TMPDIR pinSetting Mutation probes (each turned its pin red, then restored to green)
Verification
中文说明第 3 轮处理摘要 — PR #10169提交： 反馈处理Critical 发现 — 四条全部在 git 2.39.5 上复现、修复并钉住
Suggestion 发现
推迟到下一轮（批量上限，Critical 优先）
评审正文 [rv:5036894641]评审者自己延后的四条（"已记录，本轮不要求修改"）按指示未动。Test Plan 里的"no such file or directory"是评审侧路径缺了 关于 TMPDIR 钉子的说明在本套件中，测试里设置 变异探针（每次均使对应钉子变红，随后恢复为绿）
验证
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/模型 |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #10169, round 4 feedbackCritical-only mode was active (growth brake: test lines over the window budget). A growth audit ran first (verdict Findings resolved in code (all 6)[Critical] R3-1 (rc:3869897225) — quoted [Critical] R3-4 (rc:3869897240) — in-worktree git dir contaminates the hunks. Reproduced at this commit ( [Critical] R4-1 (rc:3869897249) — clean nested repos stamped as dirt. Reproduced at this commit (tsx probe driving the real [Suggestion] R4-2 (rc:3869897261) — non-empty-path [Suggestion] R1-7 (rc:3869897268) — hostile host environment untested. Added the described case: ambient [Suggestion] R1-11 (rc:3869897277) — subdirectory review run untested. Added the described case: Conflict
VerificationCommands actually run, in order:
中文说明轮次总结 — PR #10169，第 4 轮反馈当前处于仅处理 Critical 的模式（增长刹车：测试行数超出窗口预算）。本轮首先执行了增长审计（结论 已在代码中解决的发现（共 6 条）[Critical] R3-1 (rc:3869897225) — 带引号的 [Critical] R3-4 (rc:3869897240) — 工作树内的 git 目录污染 hunks。 已在本提交上复现（ [Critical] R4-1 (rc:3869897249) — 干净的嵌套仓库被盖章为脏。 已在本提交上复现（tsx 端到端驱动真实 [Suggestion] R4-2 (rc:3869897261) — 非空路径上的 [Suggestion] R1-7 (rc:3869897268) — 敌对宿主环境无测试。 按描述补充用例：在 snapshot+since 运行周围植入既有 [Suggestion] R1-11 (rc:3869897277) — 子目录评审运行无测试。 按描述补充用例：在 snapshot+since 前 冲突
验证实际执行的命令（按顺序）：
Deferred non-Critical feedbackCritical-only mode is active: the PR's diff grew src 233 / test 583 net lines beyond this counting window's baseline (budgets: 400/400). The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback continues to flow unaffected during a growth-only engagement (the per-author batch budget applies only after 5 change-producing rounds). ( 中文说明已进入仅处理 Critical 的模式：本计数窗口内 diff 净增长已达 源码 233 / 测试 583 行（预算 400/400）。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。纯增长触发期间维护者反馈照常流动（按作者的批次预算仅在完成 5 个产生改动的轮次后生效）。（评论 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/模型 |
# Conflicts: # packages/core/src/skills/bundled/review/DESIGN.md
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix round — PR #10169 (round 6)Commits: merge of Base conflict (
|
…10169) Close the round-6 Criticals structurally instead of per entrance: - The blind-spot probe now answers in two states — confirmed dirt and unresolved paths — instead of one folded flag. Unopenable directories, failed inner probes and over-budget walks are disclosed at every comparison and never enter the snapshot baseline, the exclusion families are applied to what the ignored-directory walk discovers, and the empty-diff all-clear is hedged to what `git add -A` captures. - The `add -A` capture is ruled on the child's own stderr line-by-line (spawnSync keeps stderr on exit-0 paths too): only the zero-commit nested-repository error, the embedded-repository notice with its hints, and the autocrlf normalisation warnings are tolerated — any other note refuses the snapshot and names the gap. - The hunks artifact is written as git's raw patch bytes; `-z` names are decoded one by one, so non-UTF-8 fix content and filenames survive.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Round 7 — addressed all 3 Critical findings (R5-1, R5-2, R6-1)Commit: bbdcf58 on All three inline Critical findings were reproduced end-to-end against the pre-round commit before fixing, fixed at the root cause, and pinned with tests that fail against the pre-round code. The round-6 deferred list was recorded as not requested this round and is untouched. No conflict resolution was needed ( R5-1 — blind-spot dirt model closed fail-closed instead of per-entrance (rc:3876194645)All four reproduced defects shared one root cause: the model hand-enumerated the working-tree states that can hold invisible edits, and anything unenumerated fell through to silence. The probe's answer is now split instead of folded, and every unanswerable state is disclosed, never skipped:
Pins: 5 new tests (one per entrance above, each naming the path with R5-2 — tolerated skips are structural, not textual (rc:3876194650)Both escape holes reproduced first: a mode-000 directory makes The capture is now ruled on the child's own notes:
Chose the stderr-inspection option over pathspec exclusion of discovered zero-commit repos: exclusion needs a whole-tree discovery walk and, probed, an unreadable directory still exits 0 with only a warning even without Pins: refusal on the exit-0 truncation, refusal on the masking combination (both skipped on win32/root where the permission lever does not exist), and an autocrlf-survival witness for the tolerance line. The existing zero-commit test stays green unchanged. R6-1 — hunks artifact is git's raw patch again (rc:3876194659)Reproduced: a latin-1 0xE9 byte in a fix round-tripped to EF BF BD, the artifact failed
Pin: a file committed as raw Deferred this roundThe review body's round-6 convergence-posture list (8 items, "recorded, not requested in this round") is untouched, as instructed. Verification
中文说明第 7 轮 — 已处理全部 3 条 Critical 发现（R5-1、R5-2、R6-1）提交： 三条行内 Critical 发现均先在修复前对未修改的提交做了端到端复现，然后按根因修复，并用对修复前代码会失败（变红）的测试钉住。第 6 轮延后清单本轮按要求不做处理。本轮无需解决冲突（ R5-1 — 盲区脏模型改为失败即披露，不再逐入口修补（rc:3876194645）四个已复现的缺陷共享同一根因：模型手工枚举"可能承载不可见编辑"的工作树状态，凡未枚举到的都静默漏掉。探测结果现在拆分为两类而不是折叠为一类，任何无法回答的状态都会被点名披露、绝不跳过：
钉子：5 个新测试（上述每个入口一个，各自点名路径、含 R5-2 — 被容忍的跳过改为结构化判定，不再是文本匹配（rc:3876194650）两个逃逸洞均先复现：权限 000 的目录使 捕获现在基于子进程自己的输出逐行判定：
在"stderr 逐行检查"与"经 pathspec 排除已发现的零提交仓库"两个方案中选择了前者：排除方案需要全树发现遍历，且实测即使去掉 钉子：退出码 0 截断时拒绝、掩蔽组合时拒绝（两者在 win32/root 下跳过，因为权限杠杆在那里不存在），外加 autocrlf 容忍行的存活见证。现有零提交测试原样保持绿色。 R6-1 — hunks 工件重新成为 git 的原始补丁（rc:3876194659）已复现：修复内容里的 latin-1 0xE9 字节往返后变成 EF BF BD，工件无法通过
钉子：提交一个原始字节为 本轮延后评审正文中第 6 轮收敛姿态清单（8 条，"已记录、本轮不要求"）按指示未做处理。 验证
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/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x), Post Coverage Comment (ubuntu-latest, 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x), Post Coverage Comment (ubuntu-latest, 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
# Conflicts: # packages/cli/src/commands/review.ts # packages/core/src/skills/bundled/review/DESIGN.md
…s hand-off R10-1: the redirect check lstat'd only the prefixes of the excluded directories, so a symlink planted at a family-matching NAME under .qwen/tmp redirected every side file the flow writes under it; lstat the family entries themselves and refuse one that resolves to a directory. R5-1: two new entrances to the false bare all-clear. A TRACKED symlink reaching a git repository emits no status entry, so the walk never found the repo behind it — probe an out-of-root link target that holds one instead of only naming the scope. A nested repository's own mode-160000 gitlinks take the root's index-sweep ruling, so a dead interior gitlink is unresolved rather than silently skipped. Also from the deferred list: fix-delta --since now fingerprints the hunks file it writes, and agent-prompt --role fix-audit reads the hunks only against --hunks-fingerprint, closing the tree-writable rewrite window between the two processes. Flow bookkeeping is derived from the Step 1 plan path (the plan file and its prompt-record directory) instead of a hand-listed suffix set, and the staged half of the family re-inclusion is gated fail-closed on the snapshot's own record of what it re-included. A filter screen that cannot be read to the bottom now refuses the capture rather than blanking nothing, and the capture pins core.fileMode=true so an exec-bit edit survives a repo-local fileMode=false.
… and keep the capture out of nested repositories The `--since` capture is seeded from the snapshot's own tree instead of from HEAD. Seeded from HEAD at both moments, the comparison also carried every path the tracked set gained or lost between the moments (a commit in the window) and every path an ignore rule written in the window hid from `add -A`, and a deletion classifier had to guess, per record, which of the two had made it — one entrance per round (a rule hiding an embedded repository's gitlink; a HEAD gate asked after the move). With the first tree as the second seed an entry the first capture recorded is an entry the second one holds whatever the rules say now, so a deletion in the hunks is a path gone from the disk and nothing else: the deletion classifier and its note are removed. What the seed cannot carry — a HEAD-tracked path absent at the snapshot and on disk again — is re-admitted off the snapshot's recorded HEAD. A commit in the window is disclosed as a moved HEAD and withholds the bare all-clear; so is a record that names no HEAD, or one whose HEAD the repository no longer holds. The addition-side classifier records what the rules hid by kind — files, links, nested repositories (a `sub/` listing, or a staged gitlink typed off its index mode) — and reads an addition as a rule's removal only when its kind matches what stood there; a file or a link the fix put in a hidden repository's or file's place rides the hunks. The re-admission skips a path that now sits behind a link, inside a repository the capture recorded as a gitlink, or under a component that became a plain file, instead of dying on the pathspec. `git add -A` decides whether a gitlink is modified by running a status inside the checkout, and that child executes the checkout's own repo-local filters, fsmonitor command and hooks — measured, and no `-c` on the parent reaches it. The seed's gitlinks are now excluded from `add -A` by pathspec and refreshed by hand off `rev-parse HEAD` with `update-index --index-info`, entry for entry as git rules them; both discovery statuses run `--ignore-submodules=all`; every gitlink is probed by the index sweep after its own config has been screened, with gitlinks ordered before symlinks so a repository answers under its registered name. A checkout whose `.git` git does not accept as its own — a hollow directory, a gitfile naming an enclosing checkout's git dir — stands rather than being refreshed off the HEAD discovery would find by walking up. Also: an empty gitlink checkout (a clone's, or a non-recursive `submodule update --init`'s at level 2) is skipped like an absent one at both levels, read with readdirSync rather than existsSync; a nested repository's skip-worktree bits take the sparse-checkout exemption `local-anchor.ts` already pays for; every name that reaches a probed repository is recorded against its filesystem identity and `--since` reads the baseline by identity too — only where both names resolve to the same place now, so a rename or an inode reused after a delete stays disclosed — and a route that changed is not a vanished-plus-appeared pair; the discovery-status and `add` refusals are escaped at the render boundary; and the fix auditor's input renders the finding's path through `inertPath`.
Review rounds 27 and 28 addressed — all 23 open Critical threads, base currentHead is
Verification, from this worktree at
中文说明第 27、28 轮评审已处理——23 条未决 Critical 线程全部关闭，base 为最新head 为
验证（本 worktree， |
…d siblings The capture's gitlink refresh writes a null OID as long as the repository's object names (64 hex under sha256), and leaves an entry standing when the checkout's HEAD is of the other length. The scratch directory's parent is resolved from `rev-parse --absolute-git-dir` as bytes, with only git's one trailing newline removed; a git dir whose name cannot be spelled into `GIT_INDEX_FILE` falls back to the system temp directory when that is outside the tree, and refuses otherwise. `core.ignoreCase=false` is pinned on every spawn that measures a working tree — the capture's and the nested-repository probes' — wherever that tree's filesystem is case-sensitive: a `true` there is never git's own, and under it `add -A` and a nested `status` fold a case-colliding sibling into silence. The probe is read-only, once per path, on the tree's own volume: its `.git` entry lstat'd under `.GIT`, with ENOENT, a different inode, an unverifiable inode, or a hard-linked gitfile read as sensitive. The hidden set's `--cached` half probes the entry before typing it, so a staged gitlink whose checkout is gone is recorded nowhere. Both family listings read their recorded mode: a mode-160000 entry is never handed to `add -u` or `add -A -f` (an explicit gitlink pathspec runs a status inside the checkout, measured), and is named on stderr once; the staged half hands `add -f` only a path that lstats and names what it left out. A baseline recorded through a link out of the tree is withdrawn when the probe no longer reaches the name as scope, so a removed or replaced external link routes as vanished or appeared, and the identity alias reads the withdrawn baseline rather than the snapshot. The nested-repository probe asks about skip-worktree bits before it rules on dirt, so a hand-set bit cannot hide an edit behind pre-existing dirt; the sparse-checkout exemption is granted only where the rules govern the worktree (every unflagged tracked path in-rules, with the index's gitlinks kept out of both sets), and the ungoverned or unaskable paths are named on stderr beside `git sparse-checkout reapply`. SKILL.md routes "edits no outcome owns" one way — a foreign write leaves the ledger alone; only an unrecorded fix of a finding is a ledger to correct — with pins. Six new tests use a spawn-only nested-repo helper so they run on the Windows lane; the newline-name and exec-bit cases are gated `win32`; case-sensitivity cases skip on an insensitive volume.
Review round 29 addressed — all 8 open Critical threads and the 4 body-only Criticals, base currentHead is
Two of the inline findings widened on measurement: R29-12's Verification, from this worktree at
中文说明第 29 轮评审已处理——8 条未决 Critical 线程与正文里的 4 条 Critical 全部关闭，base 为最新head 为
两条行内发现在实测中扩大了范围：R29-12 在 git 2.55 上 验证见上方命令与计数（macOS 全量 + Linux 容器全量 + agent-prompt + SKILL）；lint/格式/typecheck 全绿；新守卫上 14 个变异体全部被对应用例杀死；提交前跑了三轮成对审计（无方向通读 + 对抗式反向审计，独立 agent 只读冻结副本、用真 git 造见证），共抓到第一版引入或遗留的 13 个缺陷——大小写探针探的是 git dir 所在卷、家族 gitlink 重复计数、身份折叠重导入已撤回基线、混合哈希算法的 HEAD 长度错误、探针侧的 ignoreCase 折叠、已脏仓库跳过位检查、子模块 gitlink 卡死 sparse 对照、git dir 名字按首个换行截断、硬链接 gitfile 骗过探针，以及四个小项——每个都配了钉子，直到第三轮只剩一条 Low、第四轮增量人工复核。大小写用例在本机 |
With #10136 on the branch two things answer to "fix audit": the PR re-review's narrowed round, read off the plan's `incremental.posture`, and Step 6B's one agent over the hunks a local `--fix` applied. Step 6B now says which one it is and why the two never meet in one run (the round needs a pull-request target, where `fix.effective` is false), so a reader cannot carry the round's convergence carve-out into a local `--fix` review. DESIGN.md records the same; SKILL.test.ts pins the sentence.
Base updated onto #10136 — head
|
Thirty review rounds went into making `fix-delta` certify that its hunks were the whole edit: nested-repository digests, ignored-path and symlink classification, sparse/skip-worktree bits, redirected excludes, filter screening, record fingerprints. fix-delta.ts grew from 222 to 5,680 lines and held 142 of the loop's 185 Criticals, each round's fixes opening the next round's entrances — for a check whose output is a disclosure that changes no verdict. Replace the certification with a fixed scope printed on every `--since` run: the hunks hold what `git add -A` records in this repository, and an edit inside a submodule or nested repository, or to a gitignored file, is named as outside it. What stays is what the audit needs: the throwaway-index capture, the review's own side files excluded (both the name and the directory form, at any depth, plus the command's own --out/--since), the second capture seeded from the snapshot tree, and a moved-HEAD disclosure. HEAD is now read once and the capture is seeded from that sha, so the record and the tree describe one moment (R30-1). agent-prompt drops --hunks-fingerprint and the C-quoted patch-path parser; whether a fixed finding's edit is among the hunks is now the auditor's check, which has both in front of it. SKILL.md Step 6B, DESIGN.md and the user docs follow; lib/worktree.ts is back to main and lib/git.ts keeps only gitWithEnv.
Five everyday shapes the shrink regressed, each measured in a scratch
repository:
- A literal exclude for --out/--since under an ignored directory made
`add` refuse the whole capture ("The following paths are ignored"),
so wherever `.qwen/tmp` is ignored — qwen-code's own `.qwen/*`, the
`.qwen/` rule /setup-github writes, a bare `tmp/` — the audit never
ran. A side file an ignore rule already hides is no longer excluded
(add -A cannot capture it anyway).
- The throwaway index lived under os.tmpdir(); a TMPDIR inside the
working tree captured the index itself. It now lives under the
absolute git dir, which also holds in a linked worktree or submodule.
- `core.autocrlf` printed one conversion warning per file into the
stderr the orchestrator relays, and past Node's 1 MiB default the add
was killed (ENOBUFS). The add pins core.safecrlf=false (the stored
blob is unchanged) and gitWithEnv takes gitRaw's 512 MiB ceiling.
- In a cone-mode sparse checkout an untracked file outside the cone made
`add` exit 1; the capture passes --sparse. Its snapshot cost in very
large sparse checkouts is measured and stated in DESIGN.md.
- The fix-audit input showed a finding's first location only, so a fix
landing at its second location read as unattested. Every location is
listed and the brief (and the ledger-note template) ask about any.
Names on stderr now go through inertPath, the header check requires the
patch to open with `diff --git`, the user docs tell this agent apart from
the fix-audit-shaped review round, and every guard has a witness that
goes red when it is removed (43 mutants), including runs from a
subdirectory and from a linked worktree.
Round 30:
|
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Independent Critical-only review — head 6c3eb96e
Not approving, on budget rather than on a finding. One gate is confirmed closed and one is not confirmed at all; details below so the next pass can start from the right place.
Confirmed fixed: R30-1, the only Critical from the latest audit round
Round 30 (2026-09-21T14:30:27Z) was the most recent full audit and carried a single Critical, R30-1, d:"c" / b:"n", against packages/cli/src/commands/review/fix-delta.ts: --snapshot recorded head from a second rev-parse HEAD taken after the capture, so the snapshot could pair a tree with a head commit it did not correspond to — a false certification about the state being audited.
At head 6c3eb96e that is gone. head is resolved exactly once and threaded through as data:
if (args.snapshot) {
const head = headCommit(root); // :223 single resolution
const tree = snapshotWorkingTree(root, head, excludes); // :224 passed in as `seed`
const snapshot: FixSnapshot = { root, tree, head }; // :225 same value recordedsnapshotWorkingTree(root, seed, excludes) takes the commit as a parameter and uses it only to seed the throwaway index (read-tree ...(seed === null ? ['--empty'] : [seed]), :153-158). Its sole rev-parse is --absolute-git-dir for scratch placement — it never resolves HEAD itself, so the recorded head and the captured tree cannot diverge. headCommit uses rev-parse --verify --quiet HEAD^{commit}, returning null for an unborn HEAD, and the --empty arm handles that case coherently.
Adjacent details in the same function read correctly and are worth recording since this file has been the source of every recent Critical: the scratch directory is created under the git dir rather than the system temp dir, with a comment naming the failure it avoids (a TMPDIR inside the working tree put the throwaway index into the capture, so an untouched tree read as changed); finally { rmSync(scratch, { recursive: true, force: true }) } leaves no residue; --sparse stops an untracked file outside a sparse cone from failing the whole capture; and core.safecrlf=false plus advice.addEmbeddedRepo=false keep the relayed stderr to one legible line.
Rounds 27-29 filed 13, 10 and 12 Criticals respectively, all in this same surface. Round 30's ledger reports prevPosted: 8 with fresh: 1, i.e. the previously posted set had been cleared and only R30-1 was new, so round 30 is the authoritative statement of what remained — and it is now closed.
Not confirmed: three commits no audit round has seen
6c3eb96e was committed 2026-09-23T20:25:19Z, roughly two days after round 30. Two further commits precede it on the same day (8a437378 at 18:38, 2f1bf302 at 18:56), the newest being fix(review): keep fix-delta working under .qwen/ ignore rules. No qwen-code-ci-bot round and no other review has been submitted against any of them, so the newest state of fix-delta.ts, agent-prompt.ts, agent-briefs.ts and lib/git.ts is unaudited.
I attempted to isolate that delta and could not do it cleanly inside the budget: a two-dot compare from the last pre-round-30 commit returns 112 commits and ~8k added lines, because it also carries upstream main movement rather than this PR's own work. Separating the PR's three commits from the rebase noise needs a per-commit read, which I did not have time to do.
So the gate I cannot certify is narrow and specific: an independent Critical-only scan of the three commits landed after round 30, concentrated in the ignore-rule handling that the newest commit's own message says it changes. Everything I did read at head is consistent and I found no new Critical, but I did not complete the pass, and on a 2180-line feature whose last four audit rounds produced 36 Criticals I am not willing to treat an unfinished scan as a clean one.
Also unconfirmed for the same reason: the 100+ review threads. Pages 1 and 2 (200 threads) are fully resolved and the remaining pages were not fetched, so I am reporting no unresolved thread rather than asserting there are none.
CI
review-pr was in progress at head; no failure attributable to this diff, and CI state is not the basis for this verdict.
Verdict: COMMENT — R30-1, the sole Critical outstanding from the latest audit round, is verifiably fixed at head: the snapshot's head is resolved once and passed into the capture rather than re-read afterwards. The remaining gate is an independent Critical-only scan of the three commits landed after round 30, which no audit round has covered and which I could not isolate and complete within budget. Next step: re-request review at this head so a fresh round covers the ignore-rule change, or point the next pass directly at the 8a437378..6c3eb96e per-commit delta in fix-delta.ts.
R31-1: the scope line claimed coverage the capture does not deliver. It now describes the capture as it behaves: the hunks hold the files HEAD tracks and every other file no ignore rule hides; an edit to a gitignored file HEAD does not track, or to any path in the review's .qwen/tmp name families (tracked or not), is out of scope; and a hunk shows a file as git stores it (a binary file as `Binary files … differ`, a Git LFS file as its pointer). SKILL.md, DESIGN.md and the user docs relay the same wording, and tests pin each qualification against the behaviour. The command's own --out/--since files are now excluded from the diff range only, never from a capture. `add` refuses a pathspec whose literal prefix names an ignored path, which is why the previous version asked `check-ignore` first — and asked it of the user's index, so a file HEAD tracks but the user's index dropped leaked into the hunks (triage finding). `diff-tree` has no such refusal, so the probe is gone. The unreachable `(no location)` branch in renderFixAuditInput is gone; validateFindings guarantees a location.
Round 31 addressed: head
|
Dismissing as stale: the one live Critical (R31-1, FIX_DELTA_SCOPE wording) was fixed at the current head aac172e (the scope line now states what the capture does; threads resolved). See review comment for verification detail.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving at aac172e, and I've dismissed the stale bot CHANGES_REQUESTED as superseded.
State of play, verified rather than assumed:
- The one live Critical from round 31 (R31-1) is fixed at this head. The
FIX_DELTA_SCOPEline now says what the capture does — files HEAD tracks plus files no ignore rule hides; submodule/nested-repo/gitignored-untracked edits and the.qwen/tmpname families are named as out of scope; binary and clean-filtered (LFS) presentation is stated. The author answered all three directions in the thread and the thread is resolved. The accompanying refactor (family excludes hoisted to a module constant; the command's own side files excluded from the diff only, never the capture —addrefuses ignored literal prefixes,diff-treehas no such rule) is the right split and is pinned by the added tests. - All 224 review threads are resolved (I paged to the end; zero unresolved). The round-31 ledger's only Critical is the one above; the deferred list is explicitly recorded as follow-up by the convergence posture.
- CI:
Test (ubuntu-latest)is green on this head (the Windows/macOS Test lanes are skipped by path filtering — nothing in them runs this code path's platform specifics, and the mocked poses make the guard tests platform-independent).Desktop Shellfailed in 0s without executing (matrix-expansion infra shape — its logs are already expired), which is not attributable to this diff; I've re-run it.web-shell E2E Smokeandreview-prare still queued. - The "unresolved, please confirm" item in the bot's last review (the sparse-checkout >1M-file claim) was one the bot itself reported it could not verify for lack of infrastructure; it does not gate this approval.
Thanks to wenshao for carrying this through 31 rounds.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — the 'Integration Tests (CLI, No Sandbox)' CI job was skipped at this commit and its suite did not run locally.
Not reviewed: reverse audit — the loop stopped at the plan's round cap (5 rounds) with the final round still reporting findings, so the audit did not certify the diff clean.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": live git probes — the sandbox's git guard refused every attempt to create a scratch repository outside the checkout ( mktemp and fixed /tmp and project-tmp p…; "agent reverse-audit (round 1)": ran only the added test ( npx vitest run … -t "pins the Step 6B fix audit" , green), not the rest of the 72-test file.; "agent reverse-audit (round 1)": did not resolve which layer sets the fix-audit wave child's subagent type (no subagent_type literal in emit-workflow.ts ), so the pinned "the generated scrip…; chunk 7: executing packages/cli/src/commands/review/recover-findings.test.ts — this worktree is a fresh checkout with no built workspace dist/ (vitest's globalSetup gu…; "agent reverse-audit (round 1)": emit-workflow --batch acceptance of a fix-audit manifest (role metadata and the generated script's subagent_type: "review-agent" ) — read from agent-prompt…, and 3 more.
Test Plan (not a blocker): src/commands/review/fix-delta.test.ts — no such file or directory; src/commands/review/agent-prompt.test.ts — no such file or directory; src/commands/review.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory.
Deferred under the convergence posture (round 32, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/cli/src/commands/review/fix-delta.ts:166 — [review] Critical [fails-closed] [new-surface] a nested git repository with no commits makes git add -A fail, killing both captures and skipping the audit with a message naming neither t…packages/cli/src/commands/review/fix-delta.ts:220 — [review] runFixDelta never validates --out through the house assertWritableOutPath gate, so a directory or blank --out is discovered only after the whole capture ranpackages/core/src/skills/bundled/review/DESIGN.md:1293 — [review] the #9793 paragraph opens by narrating the incident as a --fix round and closes by saying its fix was applied by its author on a PR target, where fix.effective is falsepackages/core/src/skills/bundled/review/DESIGN.md:1293 — [review] the paragraph's only citation, config.ts:1506 , does not resolve — MAX_SUBAGENT_DEPTH_LIMIT = 100 is at packages/core/src/config/config.ts:1618packages/core/src/skills/bundled/review/DESIGN.md:935 — [review] "nothing downstream reads that field" is false of outcomeNote : renderFixAuditInput renders it as the next audit's Fixer's notepackages/core/src/skills/bundled/review/SKILL.md:248 — [review] the tool-budget exemption sentence still says "five exemptions" and omits the fix auditor, against the six the briefs now declarepackages/core/src/skills/bundled/review/SKILL.md:1130 — [review] the two refusal messages the skill branches on have no coupling test tying SKILL.md's quoted text to the CLI's literalspackages/cli/src/commands/review/agent-prompt.ts:3758 — [review] --role fix-audit --hunks without --findings gets the generic findings-role refusal, whose closing advice is invalid for this rolepackages/cli/src/commands/review/agent-prompt.ts:4418 — [review] the --findings option's help still names only verify/reverse-audit and calls the mechanism a copy, which is false for the new rolepackages/core/src/skills/bundled/review/SKILL.md:1134 (+2 locations) — [review] the reach statement bounds the audit to the --fix path while the same file orders it on the interactive fix these issues path, where fix.effective is fals…packages/cli/src/commands/review/compose-review.ts:1780 — [review] two other copies of the "exactly one dimension reads no diff" claim are now false — the exempt-head set carries two headspackages/cli/src/commands/review/fix-delta.test.ts:615 — [review] the suite's only stderr-escaping witness covers --since ; the --snapshot confirmation line is asserted nowhere, so deleting its inertPath leaves the suite greenpackages/core/src/skills/bundled/review/SKILL.test.ts:1171 — [review] the Step 6B chain's manifest link is pinned by nothing — neither the producer's && nor either end of the path appears in a testpackages/core/src/skills/bundled/review/SKILL.test.ts:1210 — [review] the refusal diagnoses' repair clauses — the only part that acts on the ledger/tree disagreement — are pinned by nothingpackages/cli/src/commands/review/agent-prompt.ts:2707 — [review] the --hunks guard claims provenance but tests only "is a git patch": the plan's own reviewed diff passes it and would be audited as the applied hunkspackages/cli/src/commands/review/fix-delta.test.ts:279 — [review] no assertion distinguishes " --since replaces its --out " from an append, so a stale patch could ride the audit input with the suite greenpackages/cli/src/commands/review/fix-delta.test.ts:412 — [review] an out-of-cone tracked path is recorded as deleted by the capture and the scope line does not disclose it; a widened cone then reports it as a new file the fixer never create…packages/core/src/skills/bundled/review/SKILL.test.ts:1154 — [review] the amended-ledger --outcomes re-run is pinned for presence only, so its order relative to the report_findings re-issue is held by nothingpackages/cli/src/commands/review/agent-prompt.test.ts:8628 — [review] the fix-audit brief test claims "each absence is pinned here" but the severity ladder and the recall rule are asserted nowherepackages/cli/src/commands/review/fix-delta.test.ts:449 — [review] the scope line's "a gitignored file HEAD does not track" clause is pinned only as text — no test edits an ignored untracked file between the two moments- …and 3 more (see the run report)
Convergence: round 32 posted 1 inline comment(s), 1 of them reported for the first time. Findings keep coming back to the same files: packages/cli/src/commands/review/fix-delta.ts (findings in round 31; 1 more now). (Evidence: the previous round was recovered from a marker this account did not post, so those rounds may not be this account's own.) A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
未审查（原文为英文）：build-and-test — the 'Integration Tests (CLI, No Sandbox)' CI job was skipped at this commit and its suite did not run locally.
未审查（原文为英文）：reverse audit — the loop stopped at the plan's round cap (5 rounds) with the final round still reporting findings, so the audit did not certify the diff clean.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：live git probes — the sandbox's git guard refused every attempt to create a scratch repository outside the checkout ( mktemp and fixed /tmp and project-tmp p…；"agent reverse-audit (round 1)"：ran only the added test ( npx vitest run … -t "pins the Step 6B fix audit" , green), not the rest of the 72-test file.；"agent reverse-audit (round 1)"：did not resolve which layer sets the fix-audit wave child's subagent type (no subagent_type literal in emit-workflow.ts ), so the pinned "the generated scrip…；chunk 7：executing packages/cli/src/commands/review/recover-findings.test.ts — this worktree is a fresh checkout with no built workspace dist/ (vitest's globalSetup gu…；"agent reverse-audit (round 1)"：emit-workflow --batch acceptance of a fix-audit manifest (role metadata and the generated script's subagent_type: "review-agent" ) — read from agent-prompt…，另有 3 条。
Test Plan（非阻断）：src/commands/review/fix-delta.test.ts — no such file or directory; src/commands/review/agent-prompt.test.ts — no such file or directory; src/commands/review.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory。
收敛姿态下延后（第 32 轮，非阻断）——已记录，本轮不要求修改；其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface：未认证任何错误结果，且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中：共 23 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 32 轮发布了 1 条行内评论，其中 1 条是首次提出。发现反复回到同一批文件：packages/cli/src/commands/review/fix-delta.ts（第 31 轮已出过发现，本轮又有 1 条）。（证据说明：上一轮的数据来自并非本账号发布的标记，上述轮次可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— DeepSeek/deepseek-v4.1-flash via Qwen Code /review (v0.24.4)
| seed: string | null, | ||
| excludes: readonly string[], | ||
| ): string { | ||
| // Under the git dir, never the system temp dir: a TMPDIR inside the |
There was a problem hiding this comment.
[Critical] R32-1: [certifies-falsely] [new-surface] The throwaway index is created inside the git directory while the capture excludes only the review's name families, so in a checkout whose git dir sits inside the worktree under a name other than .git, the capture records the git dir itself and the hunks are never empty.
snapshotWorkingTree puts the throwaway index under rev-parse --absolute-git-dir, on the assumption that the git dir is outside the capture — but the capture's only excludes are the .qwen/tmp name families. Git protects a literal .git component only, so an in-tree git dir under any other name is ordinary capturable content: git add -A --sparse -- . stages HEAD, config, refs/**, objects/** and the qwen-fix-delta-* scratch directory the capture itself just created under it. Because each capture creates a differently named scratch directory, two captures of a completely untouched tree produce two different trees, so the hunks file is never empty and --role fix-audit reports edits that do not exist.
Failure scenario: a repository created with git init --separate-git-dir=<root>/.gitdir, then the Step 6B pair with nothing edited in between. On an untouched fixture this file's own command prints fix-delta: 44 file(s) changed since the snapshot — .gitdir/objects/10/fa14c5ab…, and 36 more and writes a 13,156-byte hunks file holding 44 .gitdir/ entries; agent-prompt --role fix-audit then refuses with "the ledger records no fixed outcome, but --hunks carries edits … a write from outside this flow (a watcher, a formatter)" — naming a cause that does not exist — and the skip condition the skill documents (a fixed outcome beside an empty hunks file) can never hold in that checkout. The object churn also buries the real hunks in the auditor's only input.
Witness — [probe] two captures of an untouched --separate-git-dir fixture, and the same experiment with the in-tree git dir excluded:
intact: T1 != T2 — 45 changed names, 13,166 patch bytes
first entry: diff --git a/.gitdir/objects/10/fa14c5ab…
fixed: T1 == T2 — 0 names, 0 bytes
Suggested fix: exclude the in-tree git dir from both captures whenever it resolves inside the working tree. snapshotWorkingTree already resolves it for the scratch directory, so derive the root-relative path and append :(exclude,literal)<rel> to the add pathspec and to the diff-tree range. Pass it byte-exact: a git dir whose name is not valid UTF-8 must not travel through argv (argv coerces such a name to U+FFFD and the exclusion then matches a lookalike), which is why the earlier revision of this file carried a raw-bytes pathspec form.
The scratch index must stay outside the capture — that is why it lives under the git dir at all (packages/cli/src/commands/review/fix-delta.ts:142-145: "Under the git dir, never the system temp dir: a TMPDIR inside the working tree … put the throwaway index itself into the capture"), so a fix that moves it back to os.tmpdir() re-opens that case.
The test that must go with this fix: a --separate-git-dir sibling of the suite's TMPDIR case — init with --separate-git-dir=<repo>/.gitdir, snapshot, edit nothing, --since, and assert the hunks file is empty. Please run the mutation to confirm it goes red when the exclusion is removed.
中文说明
[Critical] 一次性索引被创建在 git 目录内部，而抓取只排除了评审自己的名字族；于是在 git 目录位于工作树内、且名字不是 .git 的检出中，抓取会把 git 目录本身记录下来，hunks 永远不为空。
snapshotWorkingTree 把临时索引放在 rev-parse --absolute-git-dir 之下，前提是 git 目录在抓取范围之外——但抓取的排除项只有 .qwen/tmp 名字族。git 只保护字面量 .git 组件，因此名字不同的树内 git 目录是普通可抓取内容：git add -A --sparse -- . 会暂存 HEAD、config、refs/**、objects/**，以及抓取自己刚在该 git 目录下创建的 qwen-fix-delta-* 临时目录。由于每次抓取创建名字不同的临时目录，对一棵完全未改动的树做两次抓取会得到两棵不同的树，因此 hunks 永不为空，--role fix-audit 会报告并不存在的编辑。
失败场景： 用 git init --separate-git-dir=<root>/.gitdir 创建的仓库，两次抓取之间不做任何编辑。在未改动的夹具上，本文件的命令输出 fix-delta: 44 file(s) changed since the snapshot — .gitdir/objects/10/fa14c5ab…, and 36 more，并写出 13,156 字节、含 44 条 .gitdir/ 条目的 hunks 文件；随后 agent-prompt --role fix-audit 以 "the ledger records no fixed outcome, but --hunks carries edits … a write from outside this flow (a watcher, a formatter)" 拒绝——指出了一个并不存在的原因——而 skill 记载的跳过条件（fixed outcome 配空 hunks 文件）在该检出中永远无法成立。对象噪声也会把真正的 hunks 埋没在审计 agent 唯一的输入里。
见证（[probe] 未改动的 --separate-git-dir 夹具两次抓取，以及排除树内 git 目录后的同一实验）：未修复 T1 != T2，45 个变更名，13,166 字节补丁，首条 diff --git a/.gitdir/objects/10/fa14c5ab…；修复后 T1 == T2，0 个名字，0 字节。
建议修复： 当树内 git 目录解析后位于工作树内时，把它从两次抓取中排除。snapshotWorkingTree 已经为临时目录解析过它，因此推导出相对于 root 的路径，并把 :(exclude,literal)<rel> 追加到 add 的 pathspec 与 diff-tree 的范围中。必须以字节精确方式传递：名字不是合法 UTF-8 的 git 目录不能走 argv（argv 会把这类名字强制转成 U+FFFD，排除项于是匹配到仿冒名），这正是本文件早先版本采用原始字节 pathspec 形式的原因。
临时索引必须留在抓取范围之外——这正是它位于 git 目录之下的原因（packages/cli/src/commands/review/fix-delta.ts:142-145："Under the git dir, never the system temp dir: a TMPDIR inside the working tree … put the throwaway index itself into the capture"），因此把它移回 os.tmpdir() 会重新打开该问题。
该修复必须配套的测试：在既有 TMPDIR 用例旁增加 --separate-git-dir 兄弟用例——用 --separate-git-dir=<repo>/.gitdir 初始化、snapshot、不做任何编辑、--since，并断言 hunks 文件为空。请运行变异确认：移除该排除后它必须变红。
— DeepSeek/deepseek-v4.1-flash via Qwen Code /review (v0.24.4)
First batch of the round-31 deferrals from QwenLM#10169 (QwenLM#12609): the parts a `/review --fix` user can actually hit. - D31-3: the HEAD-moved line said a committed change is not in the hunks. The second capture still diffs the working tree against the snapshot, so a committed edit IS in them, and so is anything a pull, rebase or checkout in the window brought in; only a gitignored file a commit started tracking is missing. The line, SKILL.md, DESIGN.md and the user docs now say that, and a test pins the missing half. - D31-25: an edit that drops an ignore rule brings what it hid in as additions (a hidden nested repository as its gitlink). The scope line states it. - D31-5 / T31-10 / T31-11: the file-target "fix these issues" path now lists Step 6B's order: snapshot before the first edit; outcomes rebuilt from the saved artifact into Step 6B's own --out path (anywhere outside the review's .qwen/tmp family would land in the hunks); then Step 6B's audit block as written, against that rebuilt artifact. It also says the plan is usually gone by then. - D31-28 / T31-9 / D31-13: the hunks are what changed on disk, not "exactly" this step's edits; several auditor lines for one finding share its single note; the docs name the interactive path as well. - D31-33 / D31-8: the audit input's heading says it lists every `fixed` outcome in the artifact, and a finding's recorded fix constraint is rendered beside its witness. - D31-4 / T31-6: the yargs-boundary test normalizes the root path for Windows, and the sparse-checkout test checks that the checkout is sparse and what the snapshot records for an out-of-cone file. D31-1 is left as is: recover-findings runs only on a pull-request --resume, and the fix audit never runs on a pull-request target, so the dropped-disclosure path it describes is not reachable. Co-authored-by: wenshao <[email protected]>
6 participants
What this PR does
When
/review --fixapplies its findings to the working tree, the review now audits what it just applied — with one bounded agent, and without re-reviewing the tree. Before the first edit, a newqwen review fix-delta --snapshotrecords the working tree as a git tree object through a throwaway index (the user's own index and the shared stash stack are never touched), seeded from a HEAD it reads once and records beside the tree. After the edits and the outcome ledger,fix-delta --sincecaptures the tree again — seeded from the snapshot's own tree — and diffs the two, so the result is exactly the hunks the fix applied: not the change under review, and not the review's own side files, which are excluded by name family at any depth (directory contents included), together with the command's own--out/--sincefiles.qwen review agent-prompt --role fix-auditthen takes the outcome-bearing findings artifact and those hunks, renders only the findings whose outcome isfixedabove the hunks into one digest-keyed list file, and prints the launch block for a singlefix-auditagent, recorded and delivered the same way every other agent's block is.The agent's brief asks one question of every applied hunk: what does this edit newly assume — a bound, a key, a lifetime, a shared resource, an ordering, a default, an invariant about callers — and does anything in the tree pin it (a test that goes red, a type, an assertion, a single source the value derives from)? It reports only the unpinned assumptions, each with what would pin it, plus an
unattested:line for afixedfinding whose file no hunk touches — or an all-clear receipt naming what it walked. Structurally it cannot become the forbidden re-review: it declaresreadsDiff: false, so the builder never hands it the reviewed diff; its output kind carries none of the finder machinery (no finding format, no severity ladder, no Exclusion Criteria, no recall rule, no project review rules); it is budget-exempt because its load is the hunks, not the diff; and it writes nothing, since the tree it reads is the user's own with the fix in it. The skill routes its return as a disclosure, never a finding: each line is appended to the affected finding's ledger note (so it rides the artifact and thereport_findingsre-issue asoutcomeNote) and printed under a Fix audit heading in the terminal summary, beside the scope linefix-deltaprinted; it never enters the findings artifact, never counts toward the fix-induced census, and changes no verdict. An audit that could not run is disclosed asFix audit: not run — <why>and moved past. The command refuses the states in which the audit could only return a false all-clear: an artifact whose outcomes were never recorded, nofixedfinding (skip when the hunks are empty too; a ledger/tree mismatch when they are not), an empty hunks file beside a ledger that claims a fix, and a file with nodiff --githeader. The interactivefix these issuespath gets the same snapshot and audit where the plan survives. DESIGN.md gains the rationale and the measured incident behind it; the user docs describe the step.Why it's needed
Step 6B's rule — do not re-run Steps 1–6 to check your own work — is right: a full re-review of an edited tree is a new review of different code, and its verdict is not this review's. Its consequence was that
--fixoutput shipped with no independent check at all. The mutation guidance in that step is addressed to the same agent that just made the edit, and nothing checks whether it was followed. Meanwhile the fix round is measured as the loop's largest single source of its own next round: roughly a third of every post-first-round finding was introduced by the fix immediately before it (#9578). PR #9793's post-mortem names the class a self-probe cannot reach: both Criticals the next round filed were fix-introduced — a hand-pickedhops < 16bound below the configurableMAX_SUBAGENT_DEPTH_LIMIT = 100, and acallId-only dedup written once one registry entry could host several runtimes — and a mutation probe of the three intended fix sites, which caught every mutant, reached neither. Those were assumptions the edit made that nothing pinned, and asking that question directly, of the hunks alone, is cheaper and safer than a round. Reach, stated exactly: this audit runs where Step 6B runs — the local and file--fixpath thatfix.effectiveadmits; the #9793 incident itself happened on the posted-comment path of a PR target, which #10153 covers. The class is the same on both; this PR reaches one of them. The three open design questions in the issue are resolved the way its triage leaned: output goes to the finding's outcome note plus a terminal block and never into the artifact; no extra effort gate is needed because an effective--fixalready floors the effort at medium; and onlyfixedoutcomes are audited.What the hunks cover — a stated scope, not a certification
fix-delta --sinceprints its scope on every run: the hunks hold whatgit add -Arecords in this repository — the files HEAD tracks, and every other file no ignore rule hides. An edit inside a submodule or a nested repository, to a gitignored file HEAD does not track, or to any path in the review's.qwen/tmpname families (tracked or not) is not in them, and a hunk shows a file as git stores it (a binary file asBinary files … differ, a Git LFS file as its pointer). One exception is detected rather than stated: a HEAD that moved between the two moments is disclosed (HEAD moved between the two moments (a -> b)), since a change that landed by commit alone leaves no hunk. The skill relays both lines under the Fix audit heading, so an all-clear never reads as covering more than the command saw.This PR spent thirty review rounds building the other design — a
fix-deltathat certified its own completeness by probing nested repositories for dirt and identity digests, classifying ignored paths, symlinks, sparse/skip-worktree bits, redirected excludes and filters, and fingerprinting its side files against planted rewrites.fix-delta.tsgrew from 222 to 5,680 lines, 142 of the 185 Criticals the loop filed landed in that one file, and each round's fixes opened the next round's entrances. For a check whose output is an advisory that changes no verdict, that was an enumeration with no last entry, so it was replaced: the PR's diff dropped from +16,032 to about +2,100 lines (about 700 of them non-test source),lib/worktree.tsis back tomain, andlib/git.tsgains onlygitWithEnv(the sanitised environment plus aGIT_INDEX_FILEredirect). DESIGN.md records the measurement.What the replacement keeps doing, because an ordinary run needs it: an ignore rule that hides
.qwen/(qwen-code's own.qwen/*, the.qwen/rule/setup-githubwrites, a baretmp/) does not stop the capture — the review's side files are excluded by name-family globs, whichaddnever refuses, and the command's own--out/--sinceonly from the diff range, sinceaddrefuses a literal pathspec that names an ignored path; the throwaway index lives under the git dir, so aTMPDIRinside the working tree is not captured;core.autocrlfrepositories do not flood the relayed stderr with a warning per file; a cone-mode sparse checkout captures with--sparse; and the audit input lists every location of a finding, so a fix that lands at its second location is not reported unattested.Reviewer Test Plan
How to verify
Non-UI change; covered by deterministic unit tests against real git repositories. From the repository root, after
npm run build(the package-local suites import workspace packages through theirdist/):tsc --noEmitis clean forpackages/cli;eslint --max-warnings 0andprettier --checkare clean on every touched file.What the tests pin, each checked by mutation (41 mutants, all killed — every guard below was removed or weakened and its test went red): the hunks carry the fix's modification, new test file and deletion but not the reviewed change already in the tree; the user's index bytes, staged set and stash list are unchanged, even with a
GIT_INDEX_FILEexported in the environment; the side-file families are excluded at any depth and with their directory contents, while other.qwen/tmpcontent (and a near-miss name) still shows; in-repo--out/--sincefiles stay out, and a repository that ignores.qwen/(or*.diff, with a leftover hunks file on disk) still captures with the flow's exact paths (.qwen/,.qwen/*with a re-include,tmp/,.qwen/tmp/), including from a subdirectory and in a linked worktree; aTMPDIRinside the working tree is not captured and no scratch directory is left behind;core.autocrlfwarnings and embedded-repository advice stay out of the relayed stderr (with a premise check that this git does warn without the pin); a cone-mode sparse checkout captures an untracked file outside the cone;gitWithEnvreturns output past Node's 1 MiB default; a rename is listed once, as a rename; nested paths and the eight-name truncation in the summary;--outinto a directory that does not exist yet; HEAD is read once and the capture is seeded from that sha, never the symbolicHEAD(R30-1); the second capture is seeded from the snapshot tree, so a force-added ignored file the fix edits is in the hunks and one untracked by a commit in the window is no phantom deletion; a moved HEAD and an unborn → born HEAD are disclosed; the scope line prints on empty and non-empty runs; non-ASCII names stay readable and control characters are flattened (inertPath) before they reach stderr; the mode, empty---since, malformed-record, foreign-root and missing-tree refusals; and the yargs wiring, driven throughparseAsync.agent-prompt --role fix-audit: the brief carries the method and none of the finder machinery, onlyfixedfindings are rendered, each refusal above, every location rendered, each flattened byinertPath, the entry carrying nothing the CLI computed about the hunks, a file that does not open withdiff --gitrefused, and that the unattested check lives in the brief rather than a path parser.Evidence (Before & After)
N/A (no UI change). Behavioural delta in one line: Step 6B previously ended at the outcome ledger with no independent check of the edit; it now records the tree before the first edit, audits exactly the applied hunks with one scoped agent, and surfaces unpinned new assumptions as outcome notes and a terminal block, beside the stated scope of what the hunks cover — without touching the findings artifact or the verdict.
Tested on
Environment (optional)
Linux, git 2.47, Node 22; vitest against real git repositories.
Risk & Scope
fix-deltaleaves unreferenced tree objects in the object database (the residuegit stash createleaves; ordinary gc collects them). Adding a secondreadsDiff: falserole extends the derived non-diff dimension-gap exemption incompose-reviewto thefix audithead, which never appears in that field because the audit runs after the verdict is composed.add --sparserecords each out-of-cone file as absent (measured: 200k such files 3.1 s against 0.46 s, 400k 16.6 s; past about a million it exceeds the git timeout and the audit reportsnot run); DESIGN.md states the limit.add --sparseneeds git 2.34 or newer (the certifying version passed it too; the review pipeline already needs 2.31 forrev-parse --path-format). A nested repository with no commit checked out makesgit add -Afail, so the audit reportsnot runfor that tree rather than a wrong result. So does a git dir whose path is not valid UTF-8 (the throwaway index lives there, and an environment variable cannot carry such a path). Side files under.qwen/tmpare not fingerprinted — a process the reviewed code left running could rewrite the snapshot record or the hunks between the moments and forge an advisory; that sits inside the same stated risk. A local run of the whole review suite reports one vitest worker RPC timeout (Timeout calling "onTaskUpdate") after every test passed; it reproduces withfix-delta.test.tsexcluded, so it is not this change.fix-deltaand--hunksare additive;agent-promptwithout--role fix-auditbehaves exactly as before, and the skill's other steps are unchanged.Linked Issues
Closes #10154. Related: #10153 (the human-fixer half), #9578 (the fix-induced measurement), #9793 (the post-mortem this is built on).
中文说明
本 PR 做了什么
/review --fix把 findings 应用到工作树之后，评审现在会审计它刚刚应用的内容——用一个有界的 agent，而不是重评这棵树。第一次编辑之前，新的qwen review fix-delta --snapshot通过一次性的临时 index 把工作树记录为 git tree 对象（用户自己的 index 与共享的 stash 栈都不会被碰），播种所用的 HEAD 只读取一次并与 tree 一起记录。编辑与 outcome 账本完成后，fix-delta --since再捕获一次——从 snapshot 自己的 tree 播种——并对两者做 diff，得到的正好是修复应用的 hunks：不含被评审的改动本身，也不含评审自己的 side file（按名字族在任意深度排除，包含目录内容），以及命令自身的--out/--since文件。随后qwen review agent-prompt --role fix-audit接收带 outcome 的 findings artifact 与这些 hunks，只把 outcome 为fixed的 findings 渲染在 hunks 之上、写入一个按 digest 命名的列表文件，并打印单个fix-auditagent 的启动 block，记录与投递方式与其它 agent 一致。该 agent 的 brief 对每个已应用 hunk 只问一个问题：这处编辑新引入了什么假设——边界、键、生命周期、共享资源、顺序、默认值、对调用方的不变量——树里有没有东西钉住它？它只报未被钉住的假设并给出钉法；对文件未被任何 hunk 触及的
fixedfinding 报一行unattested:；否则返回一行说明走查了什么的全清回执。结构上它不可能变成被禁止的重评：readsDiff: false、不带任何 finder 机制、预算豁免、且不写任何东西。skill 把它的返回当作披露而非 finding：追加到对应 finding 的账本 note（以outcomeNote随 artifact 与report_findings重发），并在终端 Fix audit 标题下与fix-delta打印的范围行一起列出；它永远不进 findings artifact、不计入 fix-induced 统计、不改变 verdict。命令拒绝只能得到虚假全清的状态：outcome 未记录的 artifact；没有fixedfinding（hunks 也为空则跳过，否则是账本/树不一致）；账本声称修了但 hunks 为空；以及不含diff --git头的文件。交互式fix these issues路径在 plan 仍在时同样做 snapshot 与审计。为什么需要
Step 6B 的规则——不要重跑 Steps 1–6 来检查自己的工作——是对的：对已编辑的树做全量重评是对不同代码的新评审，其 verdict 不属于本次评审。但后果是
--fix的产出完全没有独立检查。该步骤里的变异指导是写给刚做完编辑的同一个 agent 的，没有人检查它是否被遵守。与此同时，实测表明修复轮是本环路下一轮 findings 的最大单一来源：首轮后约三分之一的 findings 由紧邻的修复引入（#9578）。PR #9793 的复盘点明了自我探测碰不到的那一类：下一轮提交的两个 Critical 都是修复引入的——一个手写的hops < 16低于可配置的MAX_SUBAGENT_DEPTH_LIMIT = 100，一个在单个 registry 条目可以承载多个 runtime 之后写下的callId单键去重——而对三个既定修复点的变异探测（全部变异都被捕获）两者都够不到。它们是编辑做出却无人钉住的假设，直接对 hunks 本身问这个问题，比多跑一轮更便宜也更安全。覆盖范围明说：这个审计跑在 Step 6B 运行的地方——fix.effective允许的本地与文件--fix路径；#9793 事故本身发生在 PR 目标的已发布评论路径，那由 #10153 覆盖。两条路径上的问题类别相同；本 PR 覆盖其中一条。issue 里三个待定设计问题按其 triage 的倾向落地：输出进 finding 的 outcome note 与终端块、绝不进 artifact；无需额外的 effort 门控，因为生效的--fix已把 effort 下限定为 medium；只审fixed的 outcome。hunks 覆盖什么——声明范围，而非担保
fix-delta --since每次运行都打印范围：hunks 包含git add -A在本仓库记录的内容——HEAD 跟踪的文件，以及其余未被忽略规则隐藏的文件。子模块或嵌套仓库内部的编辑、对 HEAD 未跟踪的 gitignore 文件的编辑、以及评审.qwen/tmp名字族下的任何路径（无论是否跟踪）都不在其中；hunk 按 git 存储的形式显示文件（二进制文件为Binary files … differ，Git LFS 文件为其指针）。唯一被检测而非仅声明的例外是两次之间 HEAD 移动（HEAD moved between the two moments (a -> b)），因为只靠 commit 落地的改动不留 hunk。skill 把这两行放在 Fix audit 标题下转述，全清永远不会被读成覆盖了命令没看到的东西。本 PR 曾用三十轮评审构建另一种设计——让
fix-delta担保自己的完整性：探测嵌套仓库的脏状态与身份摘要，分类忽略路径、symlink、sparse/skip-worktree 位、被重定向的 excludes 与 filter，并对 side file 做指纹以防被改写。fix-delta.ts从 222 行涨到 5,680 行，环路提交的 185 条 Critical 中有 142 条落在这一个文件，每轮修复都打开下一轮的入口。对一个只产出、不改变 verdict 的建议性检查而言，这是没有尽头的枚举，因此被替换：PR 的 diff 从 +16,032 行降到约 +2,100 行（其中非测试源码约 700 行），lib/worktree.ts回到main，lib/git.ts只新增gitWithEnv。DESIGN.md 记录了这一测量。替换后仍保留的、普通运行所需的行为：忽略
.qwen/的仓库（qwen-code 自己的.qwen/*、/setup-github写入的.qwen/、裸tmp/）照常捕获——评审的 side file 用名字族 glob 排除（add不会拒绝），命令自己的--out/--since只从 diff 范围里排除，因为add会拒绝指向被忽略路径的 literal pathspec；临时 index 放在 git dir 下，工作树内的TMPDIR不会被捕获；core.autocrlf仓库不会让转述的 stderr 被逐文件告警淹没；cone 模式稀疏检出用--sparse捕获；审计输入列出 finding 的全部位置，修复落在第二个位置时不会被误报为 unattested。Reviewer Test Plan
复现命令与计数见上方英文部分（
fix-delta.test.ts34 passed；agent-prompt.test.ts379 passed；review 套件 126 个文件全部通过；SKILL.test.ts72 passed）。packages/cli的tsc --noEmit干净，所有触及文件eslint --max-warnings 0与prettier --check干净。上方列出的每个守卫都做了变异验证（41 个变异体全部被杀死）。风险与范围
fix-delta会在对象库留下无引用的 tree 对象（与git stash create相同，gc 会回收）。add --sparse会把每个圈外文件逐一记为不存在（实测：20 万个 3.1 s 对比 0.46 s，40 万个 16.6 s；超过约 100 万个会超出 git 超时，审计报告not run）；DESIGN.md 写明了这一上限。add --sparse需要 git 2.34 及以上（担保版本同样无条件传入；review 流水线本就因rev-parse --path-format需要 2.31）。没有检出任何 commit 的嵌套仓库会让git add -A失败，此时审计报告not run，不会给出错误结果。git dir 路径不是合法 UTF-8 时同样如此（临时 index 放在那里，而环境变量带不了这种路径）。.qwen/tmp下的 side file 不做指纹——评审代码残留的进程可以在两次之间改写 snapshot 记录或 hunks，伪造一条建议，这落在同一声明风险之内。本地跑整个 review 套件会在全部用例通过后报一次 vitest worker RPC 超时（Timeout calling "onTaskUpdate"）；排除fix-delta.test.ts后同样复现，与本改动无关。关联 Issue
Closes #10154。相关：#10153、#9578、#9793。