fix(review): screen content filters before the probe tree's restore too (#9558) - #10421
fix(review): screen content filters before the probe tree's restore too (#9558)#10421wenshao wants to merge 4 commits into
Conversation
…oo (#9558) `scratch-tree` refuses to create or reset a tree while the repository's LOCAL config defines a content filter, because a checkout executes `filter.<name>.smudge` whenever it rewrites a file and disabling hooks does not cover that. `test-efficacy`'s per-run restore is the same checkout one directory over — it rewrites every tracked file in the probe tree, twice per probe run — and screened nothing. The screen moves to `lib/worktree.ts` so both callers share one implementation rather than one growing a corner the other lacks, which is how the `config.worktree` scoping bug got in. `filter.<name>.process` joins `smudge` and `clean`: it is the third executable key — a long-running filter git speaks a protocol to — and enumerating two of three is how the first cut read as complete. **Repo-local only, and that is the load-bearing part.** `git lfs install` writes `filter.lfs.clean` into the user's GLOBAL config, so a screen over merged config would put every contributor with git-lfs into permanent refusal — the same failure as a tripwire that fires on every healthy run, which this pipeline has already shipped once. Both halves are pinned: removing the screen turns the new test red, and widening it to merged config turns the same test red from the other side.
github-actions
Bot
added
the
review/self-reported
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ — all required sections present, with the bilingual Problem: observed, not theoretical. This is the remainder of a measured finding from the #9221 adversarial rounds (R14-1, R15-2, and the filter half of R17-6), tracked in #9558 ( Direction: aligned. This is the ongoing #9556 class — hardening the review pipeline against a hostile PR's test code — and the linked issue prescribes exactly this scope: share Size: 5 files, +160 −72. Production logic ≈ 159 lines ( Approach: the diff is exactly the prescribed fix, and matches what I'd propose independently — move Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需章节齐全，含双语 问题：已观测到，不是理论性的。这是 #9221 对抗性测试轮次中实测 finding 的剩余部分（R14-1、R15-2 以及 R17-6 的 filter 那一半），记录于 #9558（ 方向：对齐。这属于进行中的 #9556 类别——加固 review 流水线以抵御恶意 PR 的测试代码——关联 issue 明确指定了这个范围：让 规模：5 个文件，+160 −72。生产逻辑约 159 行（ 方案：diff 恰是指定的修法，也与我独立会提出的方案一致——把 风险：无升级风险信号——改动文件均不命中与 revert 相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed the full diff and read each changed file at the reviewed commit. No blockers — the change is faithful to the fix #9558 prescribes and integrates cleanly:
One standing observation, non-blocking and pre-existing on Testing evidenceEvidence carried: the PR's own CI check results for the reviewed commit, fetched once via the API at review time (per policy this review never builds or runs PR code). The unit suite and the no-AK integration run were still in progress at fetch time — not verified: their final result on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查审阅了完整 diff，并在被审提交上逐文件阅读。无阻塞项——改动忠实实现了 #9558 指定的修法，且集成干净：
一条长期观察项，非阻塞且在 测试证据本节携带的证据：被审提交上 PR 自己的 CI 检查结果，审查时经 API 一次性获取（按策略本审查从不构建或运行 PR 代码）。获取时单元测试套件与 no-AK 集成测试仍在进行中——未验证： （CI 结果表见上方英文部分，由 finalize 工作流在 CI 完成后原地更新。） — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — minimal, exactly-prescribed security fix whose tests pin both directions; the one reservation is a pre-existing sibling surface named for follow-up, not a defect of this diff. This is what #9741's first commit should have stayed: five files that do the one thing #9558 prescribes — share the existing filter screen with the probe tree's restore, and enumerate the third executable key. My independent proposal before reading the diff was exactly this shape, and I found no simpler path the PR missed. The scoping decision is the substance of the change and it is right: a screen over merged config would refuse every contributor with git-lfs installed, trading a measured hole for a permanent false positive. The tests are the strongest part — each arm fails in the direction it exists to guard, including the false-positive arm that names The one thing I'd want the maintainer to see before this class is called closed is not in this diff: Verdict: approve. The unit suite was still in flight at review time, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 最小化、严格按指定范围实施的安全修复，测试钉住了两个方向；唯一的保留项是一处既存的同族面，点名留作后续跟进，并非本 diff 的缺陷。 这就是 #9741 的第一个提交本应保持的样子：五个文件，做 #9558 指定的那一件事——让既有的过滤器屏蔽与探针树的恢复共用，并把第三个可执行键纳入枚举。读 diff 之前我独立提出的方案正是这个形状，我也没有找到比这更简单的路径。作用域决策是这个改动的实质，而且是对的：对合并后配置做屏蔽会让每个装了 git-lfs 的贡献者被永久拒绝，用一个实测存在的漏洞换一个永久的误报。测试是最强的部分——每一臂都朝它要防的方向失败，包括点名 在这个类别被称为关闭之前，我希望维护者看到的一件事不在这个 diff 里： 结论：批准。审查时单元测试套件仍在运行，因此批准推迟到 — 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. ✅
| const r = spawnSync( | ||
| 'git', | ||
| [ | ||
| 'config', | ||
| '--file', | ||
| file, | ||
| '--get-regexp', |
There was a problem hiding this comment.
[Critical] R1-1: The screen reads each candidate config file with git config --file, which does not expand include.path / includeIf directives (git's documented default for --file), while the checkouts this screen authorises read merged config, which does. A filter planted behind an include directive is invisible to the screen and executed by the checkout: append [include] path = evil.inc to <common>/config, write [filter "evil"] smudge = CMD into the included file, add one attributes line — localFilterCommands returns [] and reports clean, and the authorised checkout --force HEAD -- . executes CMD on the host. Reproduced end-to-end at this commit (git 2.43.0), defeating both call sites the screen protects:
CASE A (include.path plant) localFilterCommands => [] => SCREEN BLIND (bypass confirmed)
CASE B (plain filter) localFilterCommands => ["filter.evil.smudge"] <- oracle flips
checkout --force HEAD -- . executed the included smudge (canary created)
The PR description's "Known gap" section documents this deferral, and the refusal of bare --includes is sound (it would follow a repo-local include pointing at the user's global config, re-import filter.lfs.clean, and reproduce the permanent-refusal failure). But the mechanism is live-verified at this commit, so the residual gap should be tracked and closed with the variant the description itself sketches — resolve each hit's origin file with --show-origin and refuse only when that origin is repo-local — or fail closed on any repo-local include.path / includeIf.*.path key. When that fix lands, a test must plant the filter via [include] path + an external file + an attributes line and assert refusal naming the key; it must be red against the current code and red again if the include handling is removed.
中文说明
屏蔽用 git config --file 读取每个候选配置文件，而这不会展开 include.path / includeIf 指令（git 对 --file 的文档化默认）；本屏蔽所授权的 checkout 读取的却是合并后配置，会展开 include。经由 include 指令植入的过滤器对屏蔽不可见，却会被 checkout 执行：向 <common>/config 追加 [include] path = evil.inc、在被包含文件中写入 [filter "evil"] smudge = CMD、再追加一行 attributes，localFilterCommands 即返回 [] 报干净，随后被授权的 checkout --force HEAD -- . 在宿主机上执行 CMD。已在本提交上端到端复现（git 2.43.0），两个被该屏蔽保护的调用点双双失守（见上方实测输出）。PR 描述的「已知缺口」一节记录了这一推迟，拒绝裸 --includes 的理由成立（它会跟随指向用户全局配置的仓库本地 include，把 filter.lfs.clean 拖回来，复刻永久拒绝失败）。但机制已在本提交上实测成立，这一残余缺口应被跟踪，并以描述自己勾画的变体关闭：用 --show-origin 解析每个命中的来源文件、仅当来源是仓库本地时才拒绝；或对任何仓库本地的 include.path / includeIf.*.path 键直接失败关闭。修复落地时，测试应通过 [include] path + 外部文件 + 一行 attributes 植入过滤器并断言点名该键的拒绝；该测试对当前代码必须为红，移除 include 处理后也必须为红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
There was a problem hiding this comment.
Tracked as #10441, not fixed here — and I want to be precise about which half I am accepting.
Your reproduction stands; I am not disputing the mechanism. What I am declining is landing the fix in this PR, for the reason your own finding grants: bare --includes re-imports filter.lfs.clean through a repo-local include naming the user's global config, and failing closed on any include.path / includeIf.*.path key refuses documented, legitimate per-repo configuration. #9741 shipped exactly that second variant and it came back as two standing Critical findings (R12-3, R14-1) about permanent refusal. This PR is #9741 re-opened at its original size specifically to stop absorbing adjacent fixes.
So the correct fix is the one you and the description both sketch — resolve each hit's origin with --show-origin and refuse only when that origin is repo-local — and #10441 carries it, including the --null requirement, the path-containment (not string-prefix) origin test, and the two test arms you specified: refusal named on a repo-local include plant, and NO refusal when the include lands in the user's global config.
Issue #9558's own "known limit" section scopes include.path / includeIf out of this screen ("this narrows rather than closes — include.path/includeIf, core.fsmonitor and core.hooksPath are each handled separately today"), so Closes #9558 remains accurate with the gap disclosed and tracked. The other three findings from this round are fixed in a426b4af / 377b5a87.
…config Round 1 review, three defects, all in the screen this PR installs. The revert checkout ran unscreened. `checkout <base> -- <paths>` in the mutation phase rewrites files, and a rewrite EXECUTES `filter.<name>.smudge` exactly as the restore's does. Screening once at the restore is not enough: everything between the two runs the PR's own test code, which can plant the filter mid-run, and the mutation phase's catch deliberately continues so the revert still runs — so a refused restore reaches the revert with the plant live. Issue #9558's triage named this site; closing that issue means screening it. The enumeration failed open on its own spawn. The screened file is attacker-writable and git prints every matching value in full, so padding one `smudge` past `spawnSync`'s default 1 MiB buffer killed git with ENOBUFS and the screen skipped the one file that defines the filter — reporting the repository clean while the checkout ran the last value. The spawn now carries the 64 MiB buffer its neighbours use, and any outcome other than exit 0 or 1 refuses instead of skipping. Existence is not readability either: git reports an unreadable `--file` with exit 1, byte-identical to "no key matched", so the candidate is settled here by construction — a regular file this process can open — rather than by asking git and reading the exit code. `localFilterCommands` therefore returns the keys AND the first candidate it could not read; both call sites refuse on either. The docstring claimed more than the code does. It opened with "Every checkout in this pipeline EXECUTES these" and enumerated only the screened sites, and said hooks were disabled pipeline-wide when only some spawns pass `core.hooksPath`. It now names the screened set as the screened set and says the creation checkouts are not in it. The test pinned the message but not the order: it planted a canary it never asserted, and committed no attributes line selecting the filter, so the restore executed no smudge with or without the screen. The fixture now commits `*.ts filter=evil`, the canary is repo-local rather than under /tmp, and its absence is asserted — that assertion is what goes red if the screen moves below the checkout.
…d test The canary lived inside the probe tree, where the restore's own `clean -ffdx` deleted it before the assertion ran — so moving the screen below the checkout left the suite green. It now sits in its own fixture directory outside the tree, removed with the rest. A checkout also runs the smudge only on files it REWRITES, and the fixture never dirtied one, so the restore rewrote nothing. The tracked file is now dirty before the run, which is the state a probe run actually leaves. The unit test named for the REVERT checkout drove `runOneMutant`, which never reaches the revert phase — it re-tested the restore under a misleading name. The revert screen is covered end-to-end in the integration suite instead.
Round 1 addressed — 5 fixed, 1 trackedCommits
On R1-6, because the first fix was not enoughCommitting
With both in place, moving the screen below the I also deleted the unit test I first wrote for R1-2: it drove On R1-1The mechanism is not in dispute — I reproduced it. What I am declining is landing the fix here, on the grounds the finding itself grants: bare Verification
Seven mutations, each red on its own — full table in the PR description. The three new ones:
中文说明第 1 轮已处理——5 条修复、1 条跟踪提交
关于 R1-6，因为第一次修得不够提交 我还删掉了最初为 R1-2 写的单元测试：它驱动 关于 R1-1机制没有争议——我复现了。我拒绝的是在此落地修复，理由正是该发现自己给出的：裸 验证
|
The red
|
| run | failed | file | this PR touches it? |
|---|---|---|---|
| first | 1 | packages/web-shell/client/components/MessageList.dom.test.tsx — expected 1800 to be 600 (scrollTop after act + nextFrame) |
no — this PR has zero web-shell changes |
| rerun | 3 | packages/cli/src/agent-view/supervisor-process.test.ts — Timed out waiting for condition in FakeAttachSocket.waitForOutput |
no |
This PR's six files are all under packages/cli/src/commands/review/. Neither suite imports anything from them.
Both failure shapes are timing-dependent, and both pass locally on this branch — the three agent-view tests run green here (5 passed | 81 skipped). npx vitest run src/commands/review, the suite this PR is about, is 116 files, 5 584 passed, 3 skipped, 0 failed.
For context, #10443 is currently fixing exactly this class on the shared runners: "under the resource pressure of the shared self-hosted runners a stall longer than that surfaces as an unhandled error and exits an all-green run red with no failing test anywhere in the log." main itself is also red right now (run 33223243920, c13aa351a), in a different step.
I have not pushed anything to chase this, deliberately — a push would cancel the in-flight review-pr run. Happy to rerun again if a third disjoint set would help make the point.
中文说明
红掉的 Test (ubuntu-latest, Node 22.x) 不是本 PR 引起的
同一个 job 跑了两次，失败的是两组互不相交的测试，没有一个在本 PR 触碰的文件里：
- 第一次：
packages/web-shell/client/components/MessageList.dom.test.tsx（expected 1800 to be 600，act+nextFrame之后的 scrollTop）——本 PR 对 web-shell 零改动。 - 重跑：
packages/cli/src/agent-view/supervisor-process.test.ts三条（FakeAttachSocket.waitForOutput的Timed out waiting for condition）。
本 PR 的六个文件全在 packages/cli/src/commands/review/ 下，上述两个套件都不引用其中任何东西。
两种失败形状都依赖时序，且在本分支本地都通过——那三条 agent-view 测试本地是绿的（5 passed | 81 skipped）。本 PR 真正关心的套件 npx vitest run src/commands/review 是 116 文件、5 584 通过、3 跳过、0 失败。
参考：#10443 正在修的就是共享 runner 上的这一类问题——「在共享自托管 runner 的资源压力下，超过 60 秒的停顿会表现为 unhandled error，把全绿的运行判成红色，而日志里没有任何失败的测试」。main 自己此刻也是红的（run 33223243920，c13aa351a），红在另一个步骤。
我没有为此推送任何东西——推送会取消正在进行的 review-pr。如果需要第三组互不相交的失败来佐证，我可以再重跑一次。
Correction to my previous comment on the red
|
Third run: the picture is now complete — filed as #10490Same commit (
Run 3 named the cause outright: Those are wall-clock budget assertions and wait-for-condition polls under CPU contention — In every one of the three runs: Filed as #10490 with the full evidence and two concrete suggestions, since this can redden any PR at random and the natural reading of a red I have not pushed anything to chase it — a push would cancel the in-flight review run, and there is nothing in this PR to change: its six files are all under 中文说明第三次运行：图景完整了——已归档为 #10490同一提交（ 第三次直接点明了成因： 三次运行里都有： 已带完整证据与两条具体建议归档为 #10490：它会随机让任何 PR 变红，而「红 = 这个 PR 弄坏了东西」在这里是错误读法。 我没有为此推送任何东西——推送会取消正在进行的评审运行，而且本 PR 也没有可改之处：它的六个文件全在 |
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is running in workflow run. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
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 reverse-audit (round 10)": end-to-end pipeline-order confirmation (whether the scratch tree's admin entry already exists when probe code plants, or the plant lands on the reuse path of a ….
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/worktree.ts:692 — [review] No test plants filter.<name>.clean — dropping clean from the regex alternation is an uncatchable mutation (measured: (smudge|process) mutant misses a repo-local filter.evil.cle…
中文说明
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 10)"：end-to-end pipeline-order confirmation (whether the scratch tree's admin entry already exists when probe code plants, or the plant lands on the reuse path of a …。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| 'config', | ||
| '--file', | ||
| file, | ||
| '--get-regexp', |
There was a problem hiding this comment.
[Critical] R1-1: Round 1, still stands. The screen reads each candidate config file with git config --file, which does not expand include.path / includeIf directives (git's documented default for --file), while the checkouts this screen authorises read merged config, which does. A filter planted behind an include directive is invisible to all three screens and executed by the checkout: append [include] path = evil.inc to <common>/config, write [filter "evil"] smudge = CMD into the included file, add one attributes line — the screen reports clean and the authorised checkout runs CMD on the host. The reply tracks this as #10441, and the tracking is right, but the mechanism is live at this commit and a reply does not retire a blocker: the finding stands until the fix lands (here or in #10441) — resolve each hit's origin file with --show-origin and refuse only when that origin is repo-local, or fail closed on any repo-local include.path / includeIf.*.path key. Bare --includes is not the fix: it follows a repo-local include naming the user's global config and re-imports filter.lfs.clean.
Witness:
Round-2 re-measurement (live, git 2.43.0, five independent agents):
git config --file .git/config --get-regexp '^filter\..*\.(smudge|clean|process)$' → exit 1, no output (SCREEN CLEAN)
git checkout --force HEAD -- . → executed the included smudge (canary created)
includeIf "gitdir:**" variant: screen exit 1, checkout created the canary; --local enumeration misses it identically
The fix must keep global-config filters exempt — expect(afterDetail).not.toContain('filter.lfs.clean') in test-efficacy.test.ts pins that a filter.lfs.clean in the user's GLOBAL config is not refused (the git-lfs contract). When the fix lands, a test that plants the filter via [include] path + an external file + an attributes line must assert refusal naming the key — please confirm it is red against the current code, and red again if the include handling is removed.
中文说明
第 1 轮发现，仍然成立。屏蔽用 git config --file 读取每个候选配置文件，而这不会展开 include.path / includeIf 指令（git 对 --file 的文档化默认）；本屏蔽所授权的 checkout 读取的却是合并后配置，会展开 include。经由 include 指令植入的过滤器对全部三处屏蔽不可见，却会被 checkout 执行：向 <common>/config 追加 [include] path = evil.inc、在被包含文件中写入 [filter "evil"] smudge = CMD、再追加一行 attributes——屏蔽报干净，被授权的 checkout 在宿主机上执行 CMD。回复已将此跟踪为 #10441，跟踪是对的，但机制在本提交上仍然成立，回复本身不能撤销阻塞项：在修复落地之前（在本 PR 或 #10441 中）该发现持续有效——用 --show-origin 解析每个命中的来源文件、仅当来源是仓库本地时才拒绝，或对任何仓库本地的 include.path / includeIf.*.path 键失败关闭。裸 --includes 不是修法：它会跟随指向用户全局配置的仓库本地 include，把 filter.lfs.clean 拖回来。
修复必须豁免全局配置中的过滤器——test-efficacy.test.ts 钉住用户全局配置里的 filter.lfs.clean 不得被拒绝（git-lfs 契约）。修复落地时，通过 [include] path + 外部文件 + 一行 attributes 植入过滤器的测试必须断言点名该键的拒绝——请确认它对当前代码为红，移除 include 处理后再次为红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * the `worktree add` spawns that CREATE the probe and base trees run | ||
| * unscreened today, and hooks are neutralised only at the spawns that pass |
There was a problem hiding this comment.
[Critical] R2-1: [certifies-falsely] [new-surface] The worktree add spawns that CREATE the probe and base trees (test-efficacy.ts:2657, base-tree.ts:268) run before any screen. Issue #9558's Stage-2 triage names exactly this: the screen belongs at run entry "so a single check covers the restore, the revert, and any other checkout the probe phase performs" — and scratch-tree screens its own rebuild worktree add. The issue's own persistence incident replays through this checkout: reviewing a malicious PR, its probe code plants filter.evil.smudge = CMD into the shared common config; every later screened checkout rightly refuses, but the plant persists (discard/cleanup never wipe the common dir). When a follow-up PR committing a .gitattributes that selects the filter is reviewed, the creation checkout rewrites every tracked file through the committed attributes + common config — CMD executes on the host before the first screen (the baseline restore's screen at test-efficacy.ts:2762) ever runs.
Witness:
git config filter.evil.smudge 'sh -c "echo SMUDGE-FIRED >> …; cat"' + committed *.ts filter=evil
git worktree add --detach … HEAD → canary after worktree add: SMUDGE-FIRED (creation checkout executed the planted command)
Contrast: scratch-tree screens BEFORE its own rebuild worktree add (scratch-tree.ts:424-446)
Screen the creation checkout with the same shared helper before the spawn (the review worktree and probeTree resolve the same <common>/config); the throw lands in the creation try/catch that already records every probe/mutant/hunk as inconclusive not-run. Alternatively track it as a named follow-up the way #10441 tracks include.path — the docstring disclosure alone leaves the triage-named coverage incomplete. The hooks/fsmonitor arms at this same site ride the class finding at lines 593-594. The screen must stay repo-local-only — issue #9558: "not the user's global or system config, which is their own contract"; a merged-config check at this new site would reproduce the git-lfs permanent refusal. An integration test in the shape of the new revert test — plant filter.evil.smudge = touch <canary> before runHandler, commit the selecting .gitattributes, assert the canary never exists — must go red when the pre-creation screen is removed.
中文说明
创建探针树与基础树的 worktree add 派发（test-efficacy.ts:2657、base-tree.ts:268）在任何屏蔽之前运行。issue #9558 的 Stage-2 分诊原话点名了此处：屏蔽应放在运行入口，「一次覆盖恢复、回退以及探针阶段执行的任何其他 checkout」——scratch-tree 也正是这样在自己的重建 worktree add 之前屏蔽的。issue 自己的持久化事故可以在这个 checkout 上完成重放：审查恶意 PR 时，其探针代码把 filter.evil.smudge = CMD 植入共享 common 配置；此后所有被屏蔽的 checkout 都会正确拒绝，但植入物持续存在（discard/cleanup 从不清扫 common 目录）。当一个提交了选中该过滤器的 .gitattributes 的后续 PR 被审查时，创建 checkout 会重写每个已跟踪文件、穿过已提交的 attributes 与 common 配置——CMD 在第一个屏蔽（test-efficacy.ts:2762 的基线恢复屏蔽）运行之前就已在宿主机上执行。
修复：在创建派发之前用同一共享 helper 屏蔽（审查 worktree 与 probeTree 解析同一个 <common>/config），throw 落入已把每个探针/突变体/分块记为 inconclusive not-run 的创建 try/catch；或者像 #10441 跟踪 include.path 那样作为命名后续跟踪——仅在 docstring 中披露会让分诊点名的覆盖不完整。本位置的 hooks/fsmonitor 入口由 593-594 行的类发现承接。屏蔽必须保持仅仓库本地——issue #9558：「不是用户的全局或系统配置，那是他们自己的契约」；在这个新位置做合并配置检查会复刻 git-lfs 永久拒绝。以新回退测试的形状写一个集成测试——在 runHandler 之前植入 filter.evil.smudge = touch <canary>、提交选中它的 .gitattributes、断言金丝雀从不出现——移除创建前屏蔽后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * `core.hooksPath` explicitly, not pipeline-wide. Do not read this comment as | ||
| * a completeness map — widening the screen means adding a call site, and the |
There was a problem hiding this comment.
[Critical] R2-2: [certifies-falsely] [new-surface] Class finding — the non-filter command-execution surface at the pipeline's git spawns is unbounded; close it structurally, not entrance by entrance. The screen enumerates exactly three filter.* keys at a subset of sites; the checkouts/spawns it authorises or coexists with also execute core.fsmonitor commands and core.hooksPath-planted hooks at spawns the screen never sees — and one planted core.fsmonitor fires at git worktree add, git checkout, git diff, git status, and git ls-files alike: every fsmonitor-capable spawn in the pipeline is an entrance, so per-site enumeration provably cannot converge (this review's audit loop found a new entrance on this surface in nearly every round).
Demonstrated entrances, all live-witnessed this round: (1) the revert checkout this diff newly screens (test-efficacy.ts:2997/3024) executes a planted core.fsmonitor AND a planted post-checkout hook while the screen passes — its paired restore spawns neutralise both via the pre-existing inert array (test-efficacy.ts:1665-1670), the revert spawn passes neither, and githooks(5) documents that pathspec checkouts fire post-checkout (measured flag=0); (2) scratch-tree's reset/rebuild checkouts (scratch-tree.ts:282/525) execute a planted fsmonitor — NO_HOOKS covers hooks only; (3) the creation spawns (test-efficacy.ts:2657, base-tree.ts:268) and fetch-pr's review-worktree add (fetch-pr.ts:976) pass no screen and no -c overrides — the planted post-checkout hook fires.
Witness:
Revert site: core.fsmonitor planted → git checkout <base> -- a.ts → canary FSMONITOR-FIRED + "warning: Empty last update token."
post-checkout planted in the common hooks dir → git checkout <sha> -- a.ts → hook fired flag=0 (also git checkout -- a.ts)
Contrast, restore shape (-c core.hooksPath=… -c core.fsmonitor=) → canary ABSENT
Sweep: one planted fsmonitor FIRED at worktree add, checkout, diff HEAD (clean and dirty), status --porcelain, ls-files -v/--others/-s
Structural options rather than entrance-by-entrance patches: (a) generalise the codebase's own established shape — the restore's inert pair (test-efficacy.ts:1665) and the residue probe's -c core.fsmonitor= (worktree.ts:1067, "the measurement must not itself become the execution") — to every spawn the pipeline controls; and/or (b) take the sandboxed guest-execution decision the #9556 class defers to. Per-site one-line fixes exist (the revert spawn, scratch-tree's spawns, the creation spawns), but the sweep shows the next entrance keeps opening. Neutralise at this layer, never refuse: legitimate user config must not wedge the pipeline — test-efficacy.test.ts pins not.toContain('filter.lfs.clean'); per-spawn -c overrides neutralise for that spawn only and never refuse the user. An integration test in the shape of the new revert test — plant core.fsmonitor = touch <canary> and separately a common-dir post-checkout hook, assert the canaries never exist — must go red when the neutralisation is removed.
中文说明
类发现——流水线 git 派发上的非过滤器命令执行面是无界的；请结构性关闭，而不是逐入口打补丁。屏蔽只在部分位置枚举恰好三个 filter.* 键；它所授权或共存的 checkout/派发还会执行 core.fsmonitor 命令与 core.hooksPath 植入的钩子——而这些派发屏蔽从不检查：一个植入的 core.fsmonitor 在 git worktree add、git checkout、git diff、git status、git ls-files 上都会触发——流水线里每个能触发 fsmonitor 的派发都是一个入口，因此逐站点枚举可证明地无法收敛（本轮审计循环几乎每一轮都在这个面上找到新入口）。
本轮全部实测的入口：(1) 本 diff 新加屏蔽的回退 checkout（test-efficacy.ts:2997/3024）在屏蔽放行时执行植入的 core.fsmonitor 与植入的 post-checkout 钩子——成对的恢复派发经由既有的 inert 数组（test-efficacy.ts:1665-1670）把两者都中和了，回退派发两个都没传；githooks(5) 明确 pathspec checkout 会触发 post-checkout（实测 flag=0）；(2) scratch-tree 的重置/重建 checkout（scratch-tree.ts:282/525）执行植入的 fsmonitor——NO_HOOKS 只覆盖钩子；(3) 创建派发（test-efficacy.ts:2657、base-tree.ts:268）与 fetch-pr 的审查 worktree add（fetch-pr.ts:976）既无屏蔽也无 -c 覆盖——植入的 post-checkout 钩子触发。
结构性选项（而非逐入口补丁）：(a) 把代码库自己既有的形状推广开——恢复的 inert 对（test-efficacy.ts:1665）与残留探针的 -c core.fsmonitor=（worktree.ts:1067，「测量本身不得成为执行」）——覆盖流水线控制的每个派发；和/或 (b) 做出 #9556 类别所推迟的沙箱化客户代码执行决策。逐位置的一行修复是存在的（回退派发、scratch-tree 派发、创建派发），但上述扫荡表明下一个入口还会不断出现。在这一层只做中和、绝不拒绝：正当的用户配置不得让流水线陷入永久拒绝——test-efficacy.test.ts 钉住 not.toContain('filter.lfs.clean')；逐派发的 -c 覆盖只对该次派发中和、从不拒绝用户。以新回退测试的形状写集成测试——植入 core.fsmonitor = touch <canary> 以及 common 目录的 post-checkout 钩子、断言金丝雀从不出现——移除中和后必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * The planting surface is two plain writes a probe can | ||
| * make into the COMMON dir this command's report calls shared: |
There was a problem hiding this comment.
[Critical] R2-3: [certifies-falsely] [new-surface] Planting into the user's GLOBAL config bypasses every screen this diff installs, and the completeness claim this diff re-issues is false. A probe runs as the same user: git config --global filter.evil.smudge CMD + git config --global core.attributesFile <file> (one line * filter=evil) — no repo write at all — and every screened checkout reads MERGED config and executes the plant while all three screens exit 1/clean. The plant persists outside every screen AND outside the common dir (~/.gitconfig is never wiped), firing on every later review and on the user's own git usage; a committed .gitattributes variant (no core.attributesFile) executes too. Reachability is confirmed: the unsandboxed runner spawn sets no env override (test-efficacy.ts:1816-1828) and sanitizedGitEnv() keeps HOME, so git config --global from probe code writes the user's real config.
Witness:
Isolated HOME: screen's exact spawn against the common config → exit 1 (CLEAN)
restore-shape checkout (-c core.hooksPath=… -c core.fsmonitor=) → executed the GLOBAL smudge, canary created
committed-.gitattributes variant → canary created
Two fix paths, author's choice: (a) add the resolved global file(s) to the candidates with an explicit decision about the git-lfs refusal cost — a name allowlist is unsafe (the attacker picks filter.lfs.smudge); a delta check (snapshot the user's global filter keys before the run, refuse keys appearing mid-run) would catch the plant without false-positiving pre-existing user filters, untested; or (b) correct the completeness claim this diff adds ("The planting surface is two plain writes … into the COMMON dir"; "a probe's planting surface is the repo-local files") and track the bypass the way #10441 tracks include.path. The git-lfs contract bounds any fix: git lfs install writes filter.lfs.clean globally, and test-efficacy.test.ts pins expect(afterDetail).not.toContain('filter.lfs.clean') — the user's own global filter must not wedge the pipeline. For path (a), a test under isolateHostGitConfig() planting via git config --global + core.attributesFile must refuse with the canary absent, and go red when the global file is removed from the candidate set.
中文说明
向用户全局配置植入可以绕过本 diff 安装的所有屏蔽，且本 diff 重新发布的完备性声明是假的。探针以同一用户身份运行：git config --global filter.evil.smudge CMD + git config --global core.attributesFile <file>（一行 * filter=evil）——完全不写仓库——每个被屏蔽的 checkout 读取的却是合并后配置并执行植入物，而三处屏蔽全部 exit 1 报干净。植入物存在于所有屏蔽之外、也存在于 common 目录之外（~/.gitconfig 从不被清除），在此后的每次审查与用户自己的 git 使用中触发；提交 .gitattributes 的变体（不用 core.attributesFile）同样执行。可达性已确认：非沙箱的 runner 派发不设置 env 覆盖（test-efficacy.ts:1816-1828），sanitizedGitEnv() 保留 HOME，因此探针代码里的 git config --global 写的就是用户的真实配置。
两条修复路径，由作者选择：(a) 把解析出的全局文件加入候选，并明确承担 git-lfs 拒绝成本的决定——按名字白名单不安全（攻击者可以取名 filter.lfs.smudge）；差量检查（运行前快照用户的全局过滤器键、拒绝运行中新出现的键）可以在不误伤既有用户过滤器的前提下捕获植入，未经测试；或 (b) 修正本 diff 新增的完备性声明（「植入面是两次写入 COMMON 目录的普通写入」「探针的植入面是仓库本地文件」），并像 #10441 跟踪 include.path 那样跟踪该绕过。任何修复都受 git-lfs 契约约束：git lfs install 把 filter.lfs.clean 写入全局，test-efficacy.test.ts 钉住 expect(afterDetail).not.toContain('filter.lfs.clean')——用户自己的全局过滤器不得让流水线陷入永久拒绝。路径 (a) 需要一个在 isolateHostGitConfig() 下经由 git config --global + core.attributesFile 植入、断言拒绝且金丝雀不出现的测试，且把全局文件移出候选集后变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| for (const line of r.stdout.split('\n')) { | ||
| const key = line.split(/\s+/)[0]; | ||
| if (key && !found.includes(key)) found.push(key); | ||
| } |
There was a problem hiding this comment.
[Critical] R2-4: [fails-closed] [new-surface] Pattern (2 locations — this is the key loop; the candidate walk at lines 654-656 is the second). The 64 MiB maxBuffer this diff raises from the 1 MiB default feeds a quadratic found.includes(key) dedup over up to ~2.9M unique keys and returns the list unbounded. One write into the config this diff itself documents as attacker-writable and never wiped — ~3M unique [filter "<n>"] keys stay under the cap at ~22 bytes/key — hangs the screen for tens of hours of synchronous CPU on a screen that runs once per mutant, and the unbounded list becomes a ~60 MB filters.keys.join(', ') refusal string in the revert records/out.json. Pre-diff, the 1 MiB ceiling ENOBUFS-killed git and skipped the loop entirely.
Witness:
PR arm (unmodified): 50k keys → 36,618 ms; 100k → 169,896 ms; 2,931,939 keys fit under the cap → extrapolated 40.6 h
BASE arm (base's exact spawn, no maxBuffer, same 100k fixture): ENOBUFS/SIGTERM at 1,052,672 bytes — loop skipped
Fix arm: Set dedup + slice(0, RESIDUE_PATH_CAP) → 100k keys in 334 ms, 12 keys returned
Bound both dimensions: collect keys into a Set (linear) and cap the reported list the way the residue cap does — first N keys plus the total, "…and M more" in the three refusal messages (RESIDUE_PATH_CAP = 12 is this file's own precedent); cap the candidate list and fail closed over the bound (see the comment at lines 654-656). Short-circuiting on first key does NOT close the candidate variant (a clean-file plant produces no keys). The refusal must keep naming the first planted key — test-efficacy.test.ts toContain('filter.evil.smudge') and scratch-tree.test.ts toContain('filter.evil.process') go red if the cap reports zero keys; the unreadable JSDoc's own rule "an unbounded join is its own denial-of-service" (worktree.ts:611-624) is the precedent the caps extend. A test planting cap+5 distinct filter keys through runOneMutant must assert the detail still names filter. keys with the "more" marker — removing the cap must make it red.
中文说明
模式（2 处——此处是键循环；654-656 行的候选遍历是第二处）。本 diff 把 maxBuffer 从默认 1 MiB 提高到 64 MiB，却把它喂给一个平方复杂度的 found.includes(key) 去重、且返回的列表无上界。向这个 diff 自己记为攻击者可写、永不清扫的配置写入一次——约 300 万个唯一 [filter "<n>"] 键（每键约 22 字节）恰好留在上限之下——就能让每次突变体都要运行一次的屏蔽同步阻塞数十小时，且无界列表会在回退记录/out.json 中变成约 60 MB 的 filters.keys.join(', ') 拒绝字符串。diff 之前，1 MiB 上限会以 ENOBUFS 杀掉 git、整个循环被跳过。
修复：给两个维度都加上界——用 Set 收集键（线性），并像残留上限那样给上报列表加上限：前 N 个键 + 总数，三处拒绝消息显示「…and M more」（RESIDUE_PATH_CAP = 12 是本文件自己的先例）；给候选列表加上限并在超限时失败关闭（见 654-656 行的评论）。按首个键短路不能关闭候选变体（干净文件植入不产生任何键）。拒绝消息必须继续点名第一个植入的键——若上限报出零个键，test-efficacy.test.ts 的 toContain('filter.evil.smudge') 与 scratch-tree.test.ts 的 toContain('filter.evil.process') 会变红；unreadable JSDoc 自己的规则「无界 join 本身就是拒绝服务」（worktree.ts:611-624）正是这些上限所延伸的先例。通过 runOneMutant 植入上限+5 个不同过滤器键的测试必须断言 detail 仍然点名 filter. 键并带有「more」标记——移除上限必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| } catch { | ||
| // No linked worktrees registered: the two candidates above are all of it. | ||
| } |
There was a problem hiding this comment.
[Critical] R2-5: [certifies-falsely] The bare catch around the <common>/worktrees readdir conflates every failure with "no linked worktrees registered". A probe that strips the directory's READ bit (chmod 0111, keeping search) makes the enumeration silently drop every per-worktree candidate while git still opens each config.worktree by direct path — the screen answers clean and the scratch-tree reset/rebuild checkout executes the planted smudge. The defect defeats the defense this PR itself documents: the file-level unreadability refusal was added beside it, while the directory-level hole stands. Plant extensions.worktreeConfig true (never matches the filter regex) + the filter into <common>/worktrees/<scratch-label>/config.worktree, then chmod — readdirSync throws EACCES, the catch swallows it, and the reset checkout resolves the per-worktree config by path traversal (search bit suffices). The chmod also removes the write bit, so git worktree prune exits 0 without removing the plant — the state persists into every later review. (The test-efficacy restore/revert sites are NOT bypassable this way — their screened tree's own config.worktree is candidate #2 via --git-dir — the fix belongs in the enumeration.)
Witness:
ROW-A pre-chmod control: screen catches the plant {"keys":["filter.evil.smudge"]}
ROW-B post-chmod: readdirSync EACCES → screen {"keys":[],"unreadable":null} (CLEAN) → checkout exit 0 → canary exists
ROW-B2 end-to-end: runScratchTree → available:true ("tracked files restored") and the reset checkout ran the plant
ROW-D: git worktree prune exit 0 — planted config.worktree still present
Fix arm (catch narrowed to ENOENT): screen unreadable=<common>/worktrees → runScratchTree refuses → canary absent; no-worktrees repo stays clean
| } catch { | |
| // No linked worktrees registered: the two candidates above are all of it. | |
| } | |
| } catch (e) { | |
| // ENOENT is "no linked worktrees registered" — the two candidates above | |
| // are all of it. Anything else is a directory this screen could not walk, | |
| // and git opens each config.worktree below it by direct path. | |
| if ((e as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| return { keys: [], unreadable: join(common, 'worktrees') }; | |
| } | |
| } |
The fix must populate the existing unreadable field — a new field fires none of the three callers' refusal branches (scratch-tree.ts:427, test-efficacy.ts:1656, :2998). A scratch-tree.test.ts case — enable extensions.worktreeConfig, plant the filter in a second linked worktree's config.worktree, chmod 0o111 the admin dir, assert available === false with 'could not be read to the end' — must go red when the catch reverts to the bare swallow.
中文说明
围绕 <common>/worktrees readdir 的裸 catch 把一切失败都当作「没有已注册的链接 worktree」。剥离该目录读位（chmod 0111，保留搜索位）的探针会让枚举静默丢弃所有按 worktree 的候选，而 git 仍按直接路径打开每个 config.worktree——屏蔽报干净，scratch-tree 的重置/重建 checkout 执行植入的 smudge。这个缺陷击败的正是本 PR 自己记下的防御：文件级不可读拒绝刚加在旁边，目录级的洞却仍在。植入 extensions.worktreeConfig true（永不匹配过滤器正则）+ 把过滤器写入 <common>/worktrees/<scratch-label>/config.worktree，然后 chmod——readdirSync 抛 EACCES，catch 吞掉，重置 checkout 按路径穿越解析该按 worktree 的配置（有搜索位即可）。chmod 同时移除了写位，git worktree prune 会 exit 0 却不移除植入物——状态持续到此后的每次审查。（test-efficacy 的恢复/回退位置不能被此手法绕过——被屏蔽树自己的 config.worktree 是候选 #2、经由 --git-dir 到达——修复属于枚举本身。）
修复必须填充既有的 unreadable 字段——新字段不会触发三个调用方的任何一个拒绝分支（scratch-tree.ts:427、test-efficacy.ts:1656、:2998）。scratch-tree.test.ts 用例——启用 extensions.worktreeConfig、把过滤器植入第二个链接 worktree 的 config.worktree、对管理目录 chmod 0o111、断言 available === false 且笔记包含 'could not be read to the end'——在 catch 回退为裸吞时必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // Same positive-control semantics as the default fake: the injected | ||
| // always-failing test must actually fail, or the run re-classes to | ||
| // control-failed and never reaches the revert this test is about. |
There was a problem hiding this comment.
[Suggestion] The reworked dead-runner fake uses fs.readFileSync but never imports fs — every one of the ~10 sibling fakes in this file carries import fs from 'node:fs';. Every st() call throws a ReferenceError the bare catch swallows into 'passed', so the added logic is dead code and the fixture is byte-equivalent to the unconditional all-pass fake it replaced — while the comment above it claims the opposite ("the injected always-failing test must actually fail"). The accident is load-bearing: 're-classes every survivor and spends nothing when the positive control fails' REQUIRES a dead runner whose injected control stays green. A maintainer reconciling the fake with its comment or its siblings adds the import — the control then genuinely fails, harnessValidated flips true, mutants run, and the test's assertions (harnessValidated toBe(false), mutants.probed toEqual([]), skippedForControl toBe(1), the note containing 'positive control FAILED') all break — inviting an assertion "fix" that silently deletes the dead-runner coverage this test exists to pin.
Witness:
Committed script verbatim against a marker file → {"numPassedTests":1,"numFailedTests":0, … [{"status":"passed"}]}
Same script + import fs from 'node:fs'; → {"numPassedTests":0,"numFailedTests":1, … [{"status":"failed"}]}
Restore the honest dead-runner shape the test needs — assertionResults: [{ status: 'passed' }], numFailedTests: 0, no st — and drop the inverted comment (or rewrite it to say the dead runner reports even the injected control green by design). The fix must keep every file, control included, reported passed — the test's own contract comment (~line 964): "A runner that reports green unconditionally — it never reads the file, so the injected control is green too."
中文说明
重写后的死运行器 fake 使用了 fs.readFileSync 却从未导入 fs——本文件里约 10 个兄弟 fake 全都带着 import fs from 'node:fs';。每次 st() 调用都抛出 ReferenceError、被裸 catch 吞成 'passed'，所以新增逻辑是死代码，该夹具与它所替换的无条件全过 fake 字节等价——而它上方的注释却声称相反（「注入的总是失败的测试必须真的失败」）。这个意外是承重结构：'re-classes every survivor and spends nothing when the positive control fails' 需要一个注入对照保持绿色的死运行器。维护者若按注释或兄弟 fake 对齐、补上导入——对照就真的失败了，harnessValidated 翻为 true，突变体开始运行，测试的断言（harnessValidated toBe(false)、mutants.probed toEqual([])、skippedForControl toBe(1)、包含 'positive control FAILED' 的笔记）全部破裂——诱发一次断言「修复」，悄悄删掉这个测试所要钉住的死运行器覆盖。
修复：恢复该测试真正需要的诚实死运行器形状——assertionResults: [{ status: 'passed' }]、numFailedTests: 0、去掉 st——并删除这条反转的注释（或改写为「死运行器按设计对注入对照也报绿」）。修复必须让每个文件（含对照）继续报 passed——测试自己的契约注释（约 964 行）：「无条件报绿的运行器——它从不读文件，因此注入的对照也是绿的。」
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // failure as a tripwire that fires on every healthy run. | ||
| const filters = localFilterCommands(probeTree); | ||
| if (filters.unreadable) { | ||
| return `the repository's local config file ${filters.unreadable} could not be read to the end, so whether this tree's restore would EXECUTE a content filter is unknown`; |
There was a problem hiding this comment.
[Suggestion] The two new test-efficacy refusal messages interpolate the plant-controlled filters.unreadable path and filters.keys raw, while scratch-tree.ts:429-441 — in this same diff — flattens the identical LocalFilterScreen outputs through inertPath (lib/paths.ts:402), the helper that exists so control/invisible characters in attacker-named values never reach agent-facing sinks. A config subsection name legally carries control/format characters and --get-regexp prints them verbatim; the sink trace settles it: these detail strings reach exactly one sink — the agent-facing out.json via JSON.stringify, which escapes Cc but NOT Cf (U+202E bidi overrides), Zl/Zp, or backticks — so those classes ride the agent-facing report unflattened, exactly where inertPath's docstring says such values must be flattened.
Witness:
git config "filter.$(printf 'evil\x1bX').smudge" … → config line [filter "evil^[X"] — --get-regexp prints the raw control byte
Sink trace: mutantResults[].detail / revert 'probe could not run' record → only writeFileSync(out, JSON.stringify(…)) — no terminal/Markdown sink, but Cf/Zl/Zp/backtick survive JSON escaping
| return `the repository's local config file ${filters.unreadable} could not be read to the end, so whether this tree's restore would EXECUTE a content filter is unknown`; | |
| return `the repository's local config file ${inertPath(filters.unreadable)} could not be read to the end, so whether this tree's restore would EXECUTE a content filter is unknown`; |
Apply the same wrap to filters.keys at line 1660 and to the revert twin at lines 2998-3008. Mirror scratch-tree.ts:429/437-441 exactly — reuse inertPath rather than a narrower local escape, or the two consumers drift. A variant of 'refuses to run through a repo-LOCAL content filter' planting a key with U+202E or ESC and asserting r.detail carries the flattened form must go red when the inertPath calls are removed.
中文说明
test-efficacy 的两条新拒绝消息把植入可控的 filters.unreadable 路径与 filters.keys 原样插值，而同一 diff 中的 scratch-tree.ts:429-441 却把同样的 LocalFilterScreen 输出经 inertPath（lib/paths.ts:402）压平——这个 helper 的存在就是为了不让攻击者命名值里的控制/隐形字符到达面向 agent 的汇聚点。配置子节名合法地携带控制/格式字符，--get-regexp 原样输出；汇聚点追踪给出了结论：这些 detail 字符串只到达一个汇聚点——经 JSON.stringify 写入面向 agent 的 out.json，JSON.stringify 会转义 Cc，但不会转义 Cf（U+202E 双向覆盖符）、Zl/Zp 与反引号——这些类别就这样未经压平进入面向 agent 的报告，恰是 inertPath 文档声明必须压平之处。
对 1660 行的 filters.keys 与 2998-3008 行的回退孪生消息做同样包裹。严格镜像 scratch-tree.ts:429/437-441——复用 inertPath 而不是更窄的本地转义，否则两个消费方会漂移。以「拒绝穿过仓库本地内容过滤器运行」的变体植入含 U+202E 或 ESC 的键、断言 r.detail 携带压平形式——移除 inertPath 调用后必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (r.error || (r.status !== 0 && r.status !== 1)) { | ||
| unreadable ??= file; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The fail-closed branch for a spawn failure or unexpected git config exit status — a named hardening of this round — has no test: every existing 'unreadable' case is caught earlier by the readability pre-check (statSync().isFile()), and the bigvalue test pads to ~1.2 MB, far under the 64 MiB cap, so git exits 0. Mutate the branch to a plain continue (the pre-hardening fail-open skip) and all three changed test files stay green; then a planted config the enumeration cannot complete — a smudge value padded past 64 MiB (ENOBUFS, r.error set) or a malformed config line git dies on with a non-0/1 exit (measured: exit 128) — reports the repository clean, and the checkout the screen authorises executes the filter. The PR's mutation-table row covers only the COMBINED maxBuffer+branch revert, which the maxBuffer half alone catches.
Witness:
Malformed config.worktree — git exits 128:
INTACT (PR code): {"keys":[],"unreadable":"…/config.worktree"} (refusal)
Branch relaxed to `continue`: {"keys":[],"unreadable":null} (clean); the relaxed mutant ran the changed suites green (181 + 31 passed)
Add a test that fails the enumeration itself: append a malformed line (e.g. a top-level not git config line) to a config.worktree fixture so git config --file dies non-0/1, then expect the inconclusive verdict with 'could not be read to the end' via runOneMutant. The witness must keep the candidate a readable regular file so it reaches the spawn branch, not the readability pre-check (worktree.ts:676-682). The new test must go red if the branch is relaxed to continue.
中文说明
针对派发失败或意外 git config 退出码的失败关闭分支——本轮点名的加固——没有测试：现有的每个「不可读」用例都被更早的可读性预检（statSync().isFile()）捕获，而 bigvalue 测试只填充到约 1.2 MB、远低于 64 MiB 上限，因此 git exit 0。把该分支突变为裸 continue（加固前的失败开放跳过），三个改动的测试文件全部保持绿色；此时一个令枚举无法完成的植入配置——填充超过 64 MiB 的 smudge 值（ENOBUFS、r.error 置位）或让 git 以非 0/1 退出码死掉的畸形配置行（实测 exit 128）——会报仓库干净，而被屏蔽授权的 checkout 会执行该过滤器。PR 描述里的变异表一行只覆盖了 maxBuffer+分支的组合回退，而那一行单靠 maxBuffer 一半就能抓住。
修复：增加一个让枚举本身失败的测试——向 config.worktree 夹具追加一行畸形行（例如顶层的 not git config），使 git config --file 以非 0/1 死掉，然后经 runOneMutant 断言带有 'could not be read to the end' 的 inconclusive 判定。见证必须保持候选是一个可读的常规文件、从而到达派发分支而非可读性预检（worktree.ts:676-682）。分支被放宽为 continue 时，新测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (filters.unreadable) { | ||
| return unavailable( | ||
| `the repository's local config file ${inertPath(filters.unreadable)} ` + |
There was a problem hiding this comment.
[Suggestion] unreadable is wired into three call sites but only one is tested: the restore side is pinned by test-efficacy.test.ts 'refuses a config candidate it cannot read — not "clean"'; the new runScratchTree branch here and the revert-phase throw (test-efficacy.ts:2998-3004) have no test at either site. Delete either untested if (filters.unreadable) block and the whole suite stays green — then a repository where a config candidate cannot be read (e.g. config.worktree replaced by a directory — the shape the restore test uses) is reported clean at those two sites, which proceed through a config the screen could not check: the fail-closed guarantee silently degrades to fail-open at two of the three screened sites.
Witness:
BOTH unreadable blocks deleted in the scratch tree → all three changed test files pass: Tests 212 passed (212)
Deleting the scratch-tree block alone: 181 passed (181), still green
Mirror the restore test at the scratch-tree site: after the first run(), mkdirSync a directory at <common>/worktrees/<label>/config.worktree (discovered the way the other-worktree test at scratch-tree.test.ts:135 discovers it) and assert available === false with the note containing 'could not be read to the end'; add the revert-phase unreadable twin beside the new revert integration test. The new tests must go red when either unreadable branch is deleted.
中文说明
unreadable 接入了三个调用点，但只有一个被测试：恢复侧由 test-efficacy.test.ts 的「拒绝穿过不可读的配置候选——而不是报干净」钉住；这里新的 runScratchTree 分支与回退阶段的 throw（test-efficacy.ts:2998-3004）在两处都没有测试。删除任一未被测试的 if (filters.unreadable) 块，整个套件保持绿色——于是配置候选不可读的仓库（例如 config.worktree 被换成目录——恢复测试所用的形状）在这两处会被报干净，照常穿过屏蔽未能检查的配置：失败关闭的保证在三处被屏蔽位置中的两处静默退化为失败开放。
修复：在 scratch-tree 位置镜像恢复测试——第一次 run() 之后，在 <common>/worktrees/<label>/config.worktree 处 mkdirSync 一个目录（按 scratch-tree.test.ts:135 的 other-worktree 测试发现它的方式），断言 available === false 且笔记包含 'could not be read to the end'；在新回退集成测试旁加上回退阶段不可读的孪生用例。删除任一 unreadable 分支时，新测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
CanReader
left a comment
CanReader
left a comment
There was a problem hiding this comment.
Flagging a merge-ordering dependency I ran into while reviewing this alongside #10474, because it is invisible from either PR on its own.
On origin/main today, localFilterCommands is defined in packages/cli/src/commands/review/scratch-tree.ts and does not exist in lib/worktree.ts:
git show origin/main:.../scratch-tree.ts | grep -c 'function localFilterCommands' -> 1
git show origin/main:.../lib/worktree.ts | grep -c 'function localFilterCommands' -> 0
This PR is the one that performs the move — deleting the definition from scratch-tree.ts and importing it from worktree.ts. Meanwhile #10474 ("resolve included filters by origin") edits that same function in lib/worktree.ts, i.e. it is written against a tree where this PR has already landed.
So the two are stacked whether or not they are labelled that way: #10474 cannot merge cleanly before this one, and if it somehow does, it is patching a file that does not contain the function. Worth making the dependency explicit in one of the two descriptions so whoever merges does not pick the wrong order. Since #10474 is a security fix (it closes an include.path bypass in the filter screen) there is some pressure to land it quickly, which makes getting the order right more than cosmetic.
On the move itself: putting the screen next to the other worktree-scoped git helpers makes sense, and the function's long comment travels with it rather than being left behind — the comment is doing real work here, since the --file-not-merged-config decision is the sort of thing someone would otherwise "simplify" later.
Two things I would check before merging, which I could not settle from the diff alone: that the moved function's sanitizedGitEnv() and any other helpers it closes over are available at the new location rather than newly imported from the old module (a cycle between scratch-tree.ts and lib/worktree.ts would be easy to introduce here), and that readdirSync — which the diff drops from scratch-tree.ts's imports — has no remaining use in that file.
3 participants
What this PR does
scratch-treerefuses to create or reset a tree while the repository's local config defines a content filter: a checkout executesfilter.<name>.smudgewhenever it rewrites a file, and pointingcore.hooksPathat nothing covers hooks, not filters.test-efficacy's per-run restore is the same checkout one directory over —checkout --force HEAD -- .rewrites every tracked file in the probe tree, twice per probe run — and it screened nothing.Three changes: the screen runs before the probe tree's restore; it moves to
lib/worktree.tsso both callers share one implementation instead of one growing a corner the other lacks; andfilter.<name>.processjoinssmudgeandcleanas the third executable key.Why it's needed
The planting surface is two plain writes a probe can make into the common dir:
git config filter.evil.smudge CMDand one line appended to$(git rev-parse --git-path info/attributes).discardandcleanupnever wipe the common dir, so a filter planted while reviewing one PR fires on every later matching checkout of the user's own repository.scratch-treealready refuses on that surface; the probe tree's restore ran through it twice per probe run.Repo-local only, and that is the load-bearing part.
git lfs installwritesfilter.lfs.cleaninto the user's global config. A screen over merged config would put every contributor with git-lfs into permanent refusal — the same failure as a tripwire that fires on every healthy run, which this pipeline has already shipped once (#9221 round 15:git statusreported nothing while the tripwire reported 3 957 paths).Why a new PR, and why it is smaller
This is #9741 at the size of its own first commit, rebased onto current
main. #9741 opened at 5 files / +160 −72 and closed at 19 files / +4741 −469 across 22 commits, with 230 review threads and roughly 100 unresolved Critical findings; its takeover reached the 100/100 round cap and its autofix loop stopped after three agent time-budget exhaustions, asking a human to "split or reduce the PR". The round-16 review said the same thing from the other side: "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 PR."The growth was not this change. Answering each Critical in turn added guards to
fetch-pr.ts,base-tree.ts,lib/platform/aone.ts,run.tsandlib/test-utils.ts— files this change never needed to touch — and each guard opened sibling entrances of its own. Two of the standing Critical findings are regressions the growth itself introduced: the widened refusal regex came to fail closed on repo-localcore.fsmonitor, the four proxy key families (R12-3) and anyinclude.path/includeIf.*.pathdirective (R14-1) — all documented, legitimate per-repo configuration. That is precisely the permanent-refusal failure the paragraph above exists to avoid. At this size those regressions do not exist, because the screen enumerates exactly three keys.Two further standing findings are properties of
main, not of this change:scratch-tree'sgitOut(R11-2) andbase-tree'sgitOut/git(R11-3) carry notimeout/killSignalonorigin/maintoday, and this PR neither introduces them nor touches those helpers. The rest — verdict channel, identity gate, teardown hook, creation screen, refspec grammar, transport-key enumeration — describe code that only ever existed inside #9741's growth and is absent here.Known gap, tracked rather than fixed here
git config --filedoes not expandinclude.path/includeIf— that is git's documented default for--file— while the checkouts this screen authorises read merged config, which does. A filter reached through an include directive therefore passes the screen. Round 1's review live-verified the bypass end-to-end; it is now tracked as #10441.The obvious fix is not the right one: adding
--includesmakes the screen follow aninclude.paththat names the user's own global config, which dragsfilter.lfs.cleanback in and reproduces exactly the permanent-refusal failure above — and failing closed on any repo-localinclude.pathkey refuses documented, legitimate per-repo configuration, which is what #9741 shipped and had to answer for twice. Doing it correctly means resolving each hit's origin file (--show-origin) and refusing only when that origin is repo-local. #10441 carries that, with the two test arms the review specified.Issue #9558's own "known limit" section scopes
include.path/includeIfout of this screen, soCloses #9558stands with the gap disclosed.Round 1 review — what changed
Three of the four other findings were defects in the screen this PR installs, and are fixed:
checkout <base> -- <paths>in the mutation phase rewrites files, so it executes a smudge exactly as the restore does — and everything between the two runs the PR's own test code, which can plant the filter mid-run while the phase's catch deliberately keeps going. Issue review: extend the content-filter screen to test-efficacy, scoped to repo-local config (not global) #9558's triage named this site, so closing that issue means screening it. Both call sites now share one screen.smudgepastspawnSync's default 1 MiB buffer killed git with ENOBUFS and the screen skipped the only file defining the filter. The spawn now carries the 64 MiB buffer its neighbours use, and anything but exit 0 or 1 refuses. Readability is settled by construction too, because git reports an unreadable--filewith exit 1 — indistinguishable from "no key matched".core.hooksPath. It now names the screened set as the screened set and says the creation checkouts are not in it.The fourth was a test defect worth its own line: the new test planted an execution canary it never asserted and committed no attributes line selecting the filter, so the restore executed no smudge either way. Fixing it surfaced two more reasons it could not fire — the restore's own
clean -ffdxdeleted a canary written inside the tree, and a checkout rewrites nothing when the tree already matches HEAD. The canary now lives outside the tree, the fixture is dirty before the run, and the assertion goes red when the screen moves below the checkout. It did not before.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review→ 116 files, 5 584 passed, 3 skipped, 0 failed.New coverage in
test-efficacy.test.ts: a repo-localfilter.evil.smudgemakes the mutant runinconclusivenaming the filter and never executes it; the same fixture with the filter moved to the user's global config runs through without a filter refusal; asmudgepadded past the default spawn buffer is still seen; a config candidate that cannot be read refuses rather than reporting clean.test-efficacy.integration.test.tsdrives the whole command with a fake runner that plants the filter mid-run, and asserts the revert never executes it.scratch-tree.test.tsgains thefilter.evil.processarm.Evidence (Before & After)
N/A — no user-visible or TUI surface. Each property is instead pinned by reverting it alone and watching the test go red:
test-efficacy's restoretest-efficacy.test.ts--file)test-efficacy.test.tsexpected … not to contain 'filter.lfs.clean'processfrom the key regexscratch-tree.test.tsfilter.evil.processno longer refusestest-efficacy.integration.test.tsmaxBufferand the fail-closed branch (the original shape)test-efficacy.test.tstest-efficacy.test.tscheckout --force/cleanspawnstest-efficacy.test.tsThe last row is the one the round-1 review asked for and the one my first attempt failed: with the canary inside the probe tree, or with a fixture that never dirties a tracked file, that mutation stays green.
Row two is the false-positive direction, and it fails by naming the global git-lfs filter. Without that test, widening the screen is the change a later round makes while "tightening" it — and, in #9741, did.
Tested on
Environment (optional)
macOS 26.6 (Darwin 25.6.0), Node 24,
packages/clivitest 3.2.7. Unit and integration tests; the integration suite drives the real command against a fake vitest bin.Risk & Scope
inconclusivefor that run instead of executing the filter. Confined topackages/cli/src/commands/review; no user-facing surface.include.pathgap described above (tracked as review: resolve filter-screen hits by origin file so include directives cannot hide a repo-local filter #10441); the pre-existing absence oftimeout/killSignalonscratch-tree's andbase-tree's git helpers, which ismain's state today and not this change's subject.Linked Issues
Closes #9558. Replaces #9741 (which replaced #9566). Follow-up from #9221; the class it belongs to is #9556. Round-1 review gap tracked as #10441.
中文说明
这个 PR 做了什么
scratch-tree在仓库本地配置定义了内容过滤器时拒绝创建或重置树：checkout 每次重写文件都会执行filter.<name>.smudge，而把core.hooksPath指向空处只覆盖钩子、覆盖不到过滤器。test-efficacy每次运行前的恢复是同一种 checkout 挪了一个目录——checkout --force HEAD -- .会重写探针树里每个已跟踪文件、每次探针运行两次——而它什么也没屏蔽。三处改动：屏蔽在探针树恢复之前执行；移到
lib/worktree.ts让两个调用方共用一份实现，而不是其中一个长出另一个没有的角落；filter.<name>.process作为第三个可执行键与smudge、clean并列。为什么需要它
植入面是探针能往共享的 common 目录做的两次普通写入：
git config filter.evil.smudge CMD，以及往$(git rev-parse --git-path info/attributes)追加一行。discard与cleanup从不清扫 common 目录，因此审查某个 PR 时植入的过滤器，会在此后用户自己仓库的每一次匹配 checkout 上触发。scratch-tree已经在这个面上拒绝了；探针树的恢复却每次探针运行穿过它两遍。只看仓库本地，这是承重部分。
git lfs install会把filter.lfs.clean写进用户的全局配置。对合并后配置做屏蔽，会让每个装了 git-lfs 的贡献者永久处于拒绝态——与「每次健康运行都触发的绊线」是同一种失败，而本流水线已经犯过一次（#9221 第 15 轮：git status一条没有，绊线报 3 957 条路径）。为什么新开 PR，以及为什么它更小
这是 #9741 回到它自己第一个提交的尺寸，重新基于当前
main。#9741 开张时是 5 文件 / +160 −72，关闭时是 19 文件 / +4741 −469、22 个提交、230 条评审线程、约 100 条未决 Critical；它的托管到达 100/100 轮次上限，autofix 循环在三次耗尽 agent 时间预算后停机，并要求人类「split or reduce the PR」。第 16 轮评审从另一侧说了同一件事：「一个不断产出兄弟项的簇，通常意味着修复是在处理同一个根因的各个实例——在下一轮之前先归因，或者把独立的簇拆成它自己的 PR。」膨胀不是这个改动带来的。逐条回答 Critical 会往
fetch-pr.ts、base-tree.ts、lib/platform/aone.ts、run.ts、lib/test-utils.ts里加护栏——都是本改动从不需要触碰的文件——而每道护栏又打开它自己的兄弟入口。其中两条仍然成立的 Critical，正是膨胀自身引入的回归：被扩宽的拒绝正则最终对仓库本地的core.fsmonitor、四个 proxy 键族（R12-3）以及任何include.path/includeIf.*.path指令（R14-1）fail-closed——而这些都是有文档的、正当的按仓库配置。这精确复刻了上一段所要避免的永久拒绝失败。在当前尺寸下这些回归不存在，因为屏蔽只枚举三个键。另有两条仍然成立的发现是
main的性质、而非本改动的：scratch-tree的gitOut（R11-2）与base-tree的gitOut/git（R11-3）在今天的origin/main上就不带timeout/killSignal，本 PR 既不引入也不触碰这些 helper。其余——verdict channel、identity gate、teardown hook、creation screen、refspec 文法、传输键枚举——描述的都是只存在于 #9741 膨胀过程中的代码，在这里并不存在。已知缺口，跟踪而非在此修复
git config --file不会展开include.path/includeIf——这是 git 对--file的文档化默认——而本屏蔽所授权的 checkout 读的是合并后配置，是会展开的。因此经由 include 指令到达的过滤器能通过屏蔽。第 1 轮评审已端到端实测该绕过，现跟踪为 #10441。显而易见的那个修法不对：加上
--includes会让屏蔽跟随一条指向用户自己全局配置的include.path，把filter.lfs.clean拖回来，精确复刻上面那个永久拒绝失败；而对任何仓库本地的include.path键直接失败关闭，则会拒绝有文档的、正当的按仓库配置——那正是 #9741 交付过、并为此两度被提出的东西。要做对，需要解析每个命中的来源文件（--show-origin）、仅当来源是仓库本地时才拒绝。#10441 承接此事，并带上评审指定的两条测试臂。issue #9558 自己的「已知边界」一节就把
include.path/includeIf划在本屏蔽之外，因此在缺口已披露的前提下Closes #9558依然成立。第 1 轮评审——改了什么
另外四条中的三条是本 PR 所安装屏蔽自身的缺陷，均已修复：
checkout <base> -- <paths>会重写文件，因而与恢复一样执行 smudge——而两者之间跑的是 PR 自己的测试代码，可以在运行中植入过滤器，同时该阶段的 catch 有意继续执行。issue review: extend the content-filter screen to test-efficacy, scoped to repo-local config (not global) #9558 的分诊点名了此处，所以关闭该 issue 就意味着必须屏蔽它。现在两个调用点共用同一道屏蔽。smudge填充到超过spawnSync默认的 1 MiB 缓冲区，就会让 git 被 ENOBUFS 杀掉，屏蔽随即跳过唯一定义过滤器的那个文件。该 spawn 现在带上与邻近 spawn 相同的 64 MiB 缓冲区，且除 exit 0 与 1 之外的任何结果都拒绝。可读性也改为构造性判定——因为 git 对不可读的--file报 exit 1，与「无匹配」无法区分。core.hooksPath。现在它把被屏蔽的集合如实称为被屏蔽的集合，并写明创建 checkout 不在其中。第四条是测试缺陷，值得单独一行：新测试植入了执行金丝雀却从未断言，也没有提交选中该过滤器的 attributes 行，因此无论有没有屏蔽，恢复都不会执行 smudge。修它的过程中又暴露出两个让金丝雀无法触发的原因——恢复自己的
clean -ffdx会删掉写在树内的金丝雀，而当树已与 HEAD 一致时 checkout 根本不重写文件。现在金丝雀位于树外、夹具在运行前就是脏的，把屏蔽挪到 checkout 之下会让该断言变红。改之前不会。审查者验证方案
如何验证
cd packages/cli && npx vitest run src/commands/review→ 116 个文件、5 584 通过、3 跳过、0 失败。test-efficacy.test.ts新增覆盖：仓库本地的filter.evil.smudge让突变体运行判为inconclusive、点名该过滤器且从不执行它；同一夹具把过滤器改放到用户全局配置后，运行照常通过、不产生过滤器拒绝；被填充到超出默认 spawn 缓冲区的smudge仍能被看到；读不到的配置候选会拒绝而不是报干净。test-efficacy.integration.test.ts用一个在运行中植入过滤器的 fake runner 驱动整条命令，断言回退从不执行它。scratch-tree.test.ts增加filter.evil.process一臂。证据（Before & After）
N/A——没有用户可见面或 TUI 面。每条性质改为以「单独回退该项 → 对应测试变红」钉住：
test-efficacy的恢复中移除屏蔽test-efficacy.test.ts--file）test-efficacy.test.tsexpected … not to contain 'filter.lfs.clean'processscratch-tree.test.tsfilter.evil.process不再拒绝test-efficacy.integration.test.tsmaxBuffer与失败关闭分支（原始形状）test-efficacy.test.tstest-efficacy.test.tscheckout --force/clean派发之下test-efficacy.test.ts最后一行正是第 1 轮评审所要求的、也是我第一次尝试没做到的：金丝雀若留在探针树内、或夹具从不弄脏已跟踪文件，该变异都会保持绿色。
第二行是误报方向，它失败时点名的正是全局的 git-lfs 过滤器。没有这条测试，扩宽屏蔽正是后来某一轮在「收紧」时会做出的改动——在 #9741 里也确实做了。
测试环境
macOS 26.6（Darwin 25.6.0）、Node 24、
packages/clivitest 3.2.7。单元与集成测试；集成套件以 fake vitest bin 驱动真实命令。风险与范围
inconclusive而不是执行该过滤器。限于packages/cli/src/commands/review，无用户可见面。include.path缺口（跟踪于 review: resolve filter-screen hits by origin file so include directives cannot hide a repo-local filter #10441）；以及scratch-tree与base-tree的 git helper 上本就缺失的timeout/killSignal——那是main今天的状态，不是本改动的主题。关联 Issue
Closes #9558。替代 #9741（后者替代 #9566）。来自 #9221 的后续；所属类别是 #9556。第 1 轮评审缺口跟踪于 #10441。