fix(shell): harden read-only Git approval against executable config hooks - #10201
fix(shell): harden read-only Git approval against executable config hooks#10201SLP-DEV1 wants to merge 48 commits into
Conversation
github-actions
Bot
added
the
review/self-reported
7f00ff9 to
1bec78f
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @SLP-DEV1! The underlying problem is solid — #10193 carries a complete end-to-end reproduction of the approval-boundary bypass (diff.<driver>.command / diff.<driver>.textconv executing from auto-approved git diff / git log / git show), and it's a properly scoped follow-up to the #8575 / #8645 mitigation. Before we can review the change itself, though, the PR description needs to follow the repository's pull request template.
The current body uses free-form headings (Summary, Testing), but none of the template's required sections are present:
## What this PR does## Why it's needed## Reviewer Test Plan— with### How to verify,### Evidence (Before & After), and the### Tested ontable## Risk & Scope## Linked Issues- the Chinese translation in a
<details>block
A few of those sections carry real signal for this particular change:
- How to verify is the natural home for the reproduction from #10193 plus what a reviewer should now observe — e.g. that
git diff/git log/git showget downgraded out of read-only when a selected driver can execute, while ordinary read-only Git commands stay auto-approved. - Risk & Scope is where to spell out the tradeoff (repositories with any configured diff driver/textconv will now see confirmation prompts on those commands even when the helper is benign) and what stays out of scope (the remaining execution-bearing config keys from #8575).
- Tested on records which OS and Git version the reproduction ran on.
No code change is needed for this gate — please update the PR description to the template, then re-trigger with @qwen-code /triage and we'll pick it up from the code review.
中文说明
感谢提交 PR，@SLP-DEV1！问题本身是成立的 —— #10193 提供了完整的端到端复现（diff.<driver>.command / diff.<driver>.textconv 可以借由被自动批准的 git diff / git log / git show 执行外部程序），而且这是对 #8575 / #8645 缓解措施范围合理的后续补全。但在审查代码之前，PR 描述需要先按仓库的 PR 模板填写。
当前正文使用的是自由格式标题（Summary、Testing），模板要求的章节均缺失：
## What this PR does## Why it's needed## Reviewer Test Plan—— 含### How to verify、### Evidence (Before & After)和### Tested on表格## Risk & Scope## Linked Issues<details>中的中文翻译
其中几个章节对这个改动尤其重要：
- How to verify：适合放 #10193 的复现步骤，以及评审者应观察到的新行为 —— 例如当所选 driver 可执行时，
git diff/git log/git show被降级、不再按只读自动批准，而普通只读 Git 命令仍保持自动批准。 - Risk & Scope：用于说明权衡（只要仓库配置了 diff driver/textconv，即使 helper 无害，这些命令现在也会弹出确认），以及不在本次范围内的内容（#8575 中其余可执行的配置项）。
- Tested on：记录复现所用的操作系统和 Git 版本。
此关卡不需要改代码 —— 请按模板更新 PR 描述，然后回复 @qwen-code /triage 重新触发，我们会从代码审查继续。
— Qwen Code · qwen3.8-max
|
@qwen-code /triage |
|
/review |
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): 784 tests passed — this review observed 22084, 525, 25512, 1667, 605, 4333, 639 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/shellAstParser.ts:1143 — [review] Repeated uncached git-config probes per approval flow (code unchanged since round 1 — deferred by the code-age rule)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：784 tests passed — this review observed 22084, 525, 25512, 1667, 605, 4333, 639 passed。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /review All currently posted R4 findings have been addressed on the current head. The Git config gate now uses a default-deny per-risk consumer model, the remote |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- worktree-scope branch of localValue never exercised — already reported as R2-7 (comment 3875206626)
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/git-config-safety.ts:210 — [probe] D5-1 command-form core.pager risk branch has no test — code unchanged since round 4, deferred by the code-age rule
Convergence: round 5 posted 10 inline comment(s), 10 of them reported for the first time; the previous round posted 13 (13 new). Findings keep coming back to the same files: packages/core/src/utils/shellAstParser.ts (findings in rounds 2, 3, 4; 6 more now); packages/core/src/utils/git-config-safety.ts (findings in rounds 3, 4; 2 more now); packages/core/src/tools/shell.ts (findings in round 4; 1 more now), and 1 more file(s). 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.)
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 10 条行内评论，其中 10 条是首次提出；上一轮发布了 13 条（其中 13 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/shellAstParser.ts（第 2、3、4 轮已出过发现，本轮又有 6 条）；packages/core/src/utils/git-config-safety.ts（第 3、4 轮已出过发现，本轮又有 2 条）；packages/core/src/tools/shell.ts（第 4 轮已出过发现，本轮又有 1 条），另有 1 个文件。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
- fail closed for all read-only Git commands in promisor repositories - align Git boolean and effective pretty-format handling with execution behavior - block raw env/substitution bypasses in memory-scoped shell permissions - recognize remerge short spelling in both classifiers - centralize broad consumer exemptions and add diagnostic logging - add deny/allow/fallback/worktree regression coverage
8a91b46 to
6d34dc0
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the build-test harness exhausted its wall budget after building packages/core plus 12 dependent workspaces (all green); its dedicated test phase never started, so no full workspace suite ran through the harness — targeted suites covering the changed files were run separately by finder/verifier agents (git-config-safety.test.ts 16/16, shellAstParser/shellReadOnlyChecker suites 798 passed).
Not explored to full depth (tool budget reached): "agent 3c": executing the two new vitest suites — the review worktree has no node_modules and a full monorepo install/build exceeds the tool budget; their behavior was veri….
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/git-config-safety.ts:225 — [probe] core.pager boolean exemption fails open — git executes every non-empty core.pager value (pager.<cmd> booleans are genuine and must keep theirs)packages/core/src/memory/memory-scoped-agent-config.ts:31 — [probe] LEADING_ENV_ASSIGNMENT_RE duplicated verbatim across the two permission boundaries — centralise in shell-utils.ts before the copies driftpackages/core/src/utils/git-config-safety.ts:103 — [probe] SIGNATURE_PLACEHOLDER is a third copy of the %G[?GKFPST] class with no cross-file equivalence pinpackages/core/src/utils/shellAstParser.ts:1163 — [probe] primary risk consumer has no exhaustiveness tie to LocalGitConfigRisk — a future risk field fails open on the AST path (tsc mutation proven)packages/core/src/utils/git-config-safety.ts:206 — [probe] effective-scope log.showsignature / pretty.<name> activation untested with global values — effectiveValue->localValue mutation survivespackages/core/src/utils/shellAstParser.ts:1122 — [probe] branch/cat-file/remote members of FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS have no allow-side test — removal survives the suitepackages/core/src/utils/git-config-safety.ts:36 — [probe] no test induces a probe failure — an all-false PROBE_FAILED mutant fails open while the suite stays greenpackages/core/src/utils/git-config-safety.ts:242 — [probe] probe-failure fail-closed path emits no diagnostic trace (anchor could not be resolved to a diff hunk; kept terminal/deferred)packages/core/src/utils/shellAstParser.ts:1122 — [review] FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS lacks a membership-criterion comment at the natural edit pointpackages/core/src/utils/shellReadOnlyChecker.ts:90 — [review] R5-7 (still stands) git-option safety rules kept as two hand-synchronized copies across the AST and regex classifiers — lockstep test now mitigates drift, duplication remains
Convergence: round 6 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 10 (10 new). Findings keep coming back to the same files: packages/core/src/utils/git-config-safety.ts (findings in rounds 3, 5; 1 more now); packages/core/src/tools/shell.ts (findings in round 5; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — the build-test harness exhausted its wall budget after building packages/core plus 12 dependent workspaces (all green); its dedicated test phase never started, so no full workspace suite ran through the harness — targeted suites covering the changed files were run separately by finder/verifier agents (git-config-safety.test.ts 16/16, shellAstParser/shellReadOnlyChecker suites 798 passed)。
未探索到全部深度（达到工具调用预算）："agent 3c"：executing the two new vitest suites — the review worktree has no node_modules and a full monorepo install/build exceeds the tool budget; their behavior was veri…。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 10 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 10 条（其中 10 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/git-config-safety.ts（第 3、5 轮已出过发现，本轮又有 1 条）；packages/core/src/tools/shell.ts（第 5 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /review Round 6 is addressed on the current head |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- AST risk consumer has no exhaustiveness tie to LocalGitConfigRisk (future risk field fails open on the AST path) — already reported (round-6 deferred list, review 5073138457)
- branch/cat-file/remote members of FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS have no allow-side test — already reported (round-6 deferred list, review 5073138457)
- LEADING_ENV_ASSIGNMENT_RE duplicated across the two permission boundaries — already reported (round-6 deferred list, review 5073138457)
Not explored to full depth (tool budget reached): "agent 1a": mutation check that the brace-group test goes RED without the compound_statement guard — attempted twice, but the shared worktree was concurrently modified be…; "agent 4": none — no checks were cut short..
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/git-shell-execution-safety.test.ts:24 — [review] R7-2 new test file duplicates the sibling fixture and fallback looppackages/core/src/utils/git-config-safety.ts:239 — [review] R7-3 signatureVerifier negative cells untested (over-asking mutations survive)packages/core/src/utils/git-shell-execution-safety.test.ts:74 — [review] R7-4 fallback-mode brace-group rejection has no regression pin
Convergence: round 7 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/core/src/utils/git-config-safety.ts (findings in round 6; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (3 Critical(s)), the rate of first-time findings is not falling (this round 3, previous 2), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 1a"：mutation check that the brace-group test goes RED without the compound_statement guard — attempted twice, but the shared worktree was concurrently modified be…；"agent 4"：none — no checks were cut short.。
未审查：反向审计——评审时间预算不足，未能开始第 6 轮。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/git-config-safety.ts（第 6 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 3 条 Critical），首次发现的速率没有下降（本轮 3，上一轮 2），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /review Round 7 is addressed on current head
All currently posted review threads are resolved. Please review the current head. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- probe-failure and hook-only fail-closed paths emit no diagnostic trace (git-config-safety.ts:185-199) — already reported (round-6 deferred list, review 5073138457)
- LEADING_ENV_ASSIGNMENT_RE duplicated verbatim across the two permission boundaries (memory-scoped-agent-config.ts:31) — already reported (round-6 deferred list, review 5073138457)
- branch/cat-file/remote members of FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS have no allow-side test (shellAstParser.ts:1125) — already reported (round-6 deferred list, review 5073138457)
- AST risk consumer has no exhaustiveness tie to LocalGitConfigRisk (shellAstParser.ts:1163) — already reported (round-6 deferred list, review 5073138457)
Not reviewed: build-and-test — packages/cli and the downstream workspace suites never ran: the pre-existing packages/sdk-typescript bundle-size build failure (byte-identical at the merge base, so not a finding against this PR) aborts the harness before its test phase, and no CI test/build/lint/typecheck job ran on this head (fork workflows are action_required). packages/core was measured by hand on both arms — PR side 13 failing files / 80 tests, base side 11 / 75, net-new 2 files / 5 tests — and the cross-file tracer found no packages/cli consumer of any changed symbol..
Not explored to full depth (tool budget reached): "agent test-matrix": packages/cli unit tests and the integration suites were not run — only packages/core, so a CLI-side expectation depending on git remote show or unquoted-escap…; "agent reverse-audit (round 5)": the toctou layer beyond the two filed variants — I did not walk a config write landing between classification and spawn for an *existing* repository (R8-18 cove…; "agent reverse-audit (round 2)": end-to-end classifyShellCommandSafetyInDirectory('git status', <symlinked cwd>) under tsx — the run died in a tsx TransformError before printing, so the r…; "agent reverse-audit (round 4)": non-bash spawn shells — neither zsh nor ksh exists on this runner and I did not locate the shell-binary selection in shellExecutionService.ts within budge…; chunk 1: Bash-rule matching normalisation in PermissionManager.evaluateSingle / matchesRule (whether a persisted Bash(git diff) matches a raw env-prefixed bash -c '….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/shell-safety-rules.ts:341 — [review] D8-1 the %G term sits in a predicate shared by nine non-git roots and misses the quoted formpackages/core/src/utils/git-config-safety.test.ts:184 (+2 locations) — [probe] D8-2 new test files do not isolate ambient git config, so a global config reddens thempackages/core/src/utils/git-config-safety.ts:163 — [probe] D8-3 unconditional second synchronous git spawn, ~48% of the call, discarded on the fail-closed pathpackages/core/src/utils/git-config-safety.ts:144 — [probe] D8-4 three of the four hook-probe outcomes are unpinned; all three mutants ship greendocs/design/2026-08-08-read-only-git-config-safety.md:5 — [review] D8-5 doc now claims probe failures fail closed and deletes the carve-out, but an un-stat-able cwd fails openpackages/core/src/memory/memory-scoped-agent-config.shell-safety.test.ts:68 — [probe] D8-6 the new substitution disjunct survives deletion: the only test case also matches the env regexpackages/core/src/utils/git-config-safety.test.ts:90 — [probe] D8-7 --no-query is not a real git option; the only pin for it asserts an unfalsifiable behaviourpackages/core/src/tools/shell.ts:2048 — [probe] D8-8 the new env gate matches only NAME=; bash's NAME+= prefix is invisible to both copiespackages/core/src/utils/git-config-safety.test.ts:464 — [probe] D8-9 the only parser-unavailable test cannot fail: all nine cases return false in AST mode toopackages/core/src/memory/memory-scoped-agent-config.ts:315 — [probe] D8-10 the new deny branch reports read-only only for commands the classifier calls read-only
Convergence: round 8 posted 10 inline comment(s), 10 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/core/src/utils/git-config-safety.ts (findings in round 7; 5 more now); packages/core/src/utils/shellAstParser.ts (findings in round 7; 4 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (10 Critical(s)), the rate of first-time findings is not falling (this round 10, previous 3), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — packages/cli and the downstream workspace suites never ran: the pre-existing packages/sdk-typescript bundle-size build failure (byte-identical at the merge base, so not a finding against this PR) aborts the harness before its test phase, and no CI test/build/lint/typecheck job ran on this head (fork workflows are action_required). packages/core was measured by hand on both arms — PR side 13 failing files / 80 tests, base side 11 / 75, net-new 2 files / 5 tests — and the cross-file tracer found no packages/cli consumer of any changed symbol.。
未探索到全部深度（达到工具调用预算）："agent test-matrix"：packages/cli unit tests and the integration suites were not run — only packages/core, so a CLI-side expectation depending on git remote show or unquoted-escap…；"agent reverse-audit (round 5)"：the toctou layer beyond the two filed variants — I did not walk a config write landing between classification and spawn for an *existing* repository (R8-18 cove…；"agent reverse-audit (round 2)"：end-to-end classifyShellCommandSafetyInDirectory('git status', <symlinked cwd>) under tsx — the run died in a tsx TransformError before printing, so the r…；"agent reverse-audit (round 4)"：non-bash spawn shells — neither zsh nor ksh exists on this runner and I did not locate the shell-binary selection in shellExecutionService.ts within budge…；chunk 1：Bash-rule matching normalisation in PermissionManager.evaluateSingle / matchesRule (whether a persisted Bash(git diff) matches a raw env-prefixed bash -c '…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 10 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 8 轮发布了 10 条行内评论，其中 10 条是首次提出；上一轮发布了 3 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/git-config-safety.ts（第 7 轮已出过发现，本轮又有 5 条）；packages/core/src/utils/shellAstParser.ts（第 7 轮已出过发现，本轮又有 4 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 10 条 Critical），首次发现的速率没有下降（本轮 10，上一轮 3），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| // Keep this as a list of simple Git-supported key regexes. In particular, do | ||
| // not use JavaScript-only constructs such as non-capturing groups: this value | ||
| // is passed directly to `git config --get-regexp`. | ||
| const LOCAL_GIT_CONFIG_RISK_KEY_PATTERN = [ |
There was a problem hiding this comment.
[Critical] R8-4: [certifies-falsely] The risk-key probe is an allowlist, and one unlisted key hands execution to a different repository's config that this probe structurally never reads.
The gate asks git config --get-regexp for the 19 key patterns enumerated here and treats "nothing matched" as "no execution risk". That is only sound if the list is complete, and it cannot be: a single unlisted key can make Git consult a second config file. diff.submodule is the demonstrated corner, not the whole defect.
Build a superproject with one submodule, put [diff] submodule = diff in the superproject's .git/config and diff.external = touch /tmp/PWNED in the submodule's config (.git/modules/sub/config) — both are repository-controlled content that a cloned repo ships with — and advance the submodule's checked-out commit past the recorded gitlink. The probe then emits nothing at all (diff.submodule matches none of the anchored patterns), result.status === 1, getLocalGitConfigRisk returns NO_RISK, and git diff / git log -p stay classified read-only and are auto-approved. Both created /tmp/PWNED, because show_submodule_inline_diff spawns a child git diff inside the submodule, which reads the submodule's config and runs its diff.external. Unsetting diff.submodule creates nothing, and submodule.recurse = true instead creates nothing, so that key is the specific trigger. git-config-safety.test.ts has zero occurrences of submodule.
The class has no last corner in the other direction either: every entry here is scoped to one repository, so any future key (or flag) that makes Git consult a second config file, and any exec-bearing key Git adds in a later version, reopens the same hole silently.
Witness:
executed against git 2.43.0; probe pattern lifted from source (19 keys)
== probe output in superproject: (empty) == probe exit status: 1
== all diff.* keys the superproject has: local diff.submodule diff
ARM A: diff.submodule=diff -> git diff exit=0 sentinel created: YES
ARM C: git log -p -1 (diff.submodule=diff) -> sentinel created: YES
CONTROL 1: diff.submodule unset -> sentinel created: NO
CONTROL 2: submodule.recurse=true instead -> sentinel created: NO
ARM B: diff.submodule=log -> sentinel created: NO
PR classifier: risk={...all eleven fields false...}
readOnly("git diff")=true readOnly("git log -p -1")=true
BASE classifier: identical (the diff narrows the list 2 -> 19 keys without closing this one)
Please invert the default rather than lengthen the list: enumerate the small bounded set of local/worktree keys that are known inert (core.repositoryformatversion, core.bare, core.filemode, core.ignorecase, core.logallrefupdates, remote.*.url/fetch, branch.*.remote/merge, submodule.*.url/branch, user.*, ...) and fail closed on any key outside that set — one inert-key violation field instead of one field per attack. Separately, make the probe aware that a superproject's answer is not the whole answer: if .gitmodules exists or submodule.* entries are present, treat the command as risky regardless, since getLocalGitConfigRisk(cwd) cannot see .git/modules/*/config.
The fix has to keep every field of the returned record truthful, because shellAstParser.ts:1206 is return Object.values(getLocalGitConfigRisk(cwd)).some(Boolean); and the design doc specifies that form precisely so future risk fields cannot be omitted from the fallback gate — a field that is true for benign repositories would silently force approval across the whole fallback path.
For a witness, add a superproject whose .git/config contains only [diff] submodule = diff and assert getLocalGitConfigRisk(cwd) reports a risk, plus an end-to-end case asserting classifyShellCommandSafety('git diff', cwd) is not read-only for that repo; removing the new default-deny field must turn both red, where today they would stay green with arbitrary code executing.
中文说明
风险键探测是一份白名单，而其中一个未列出的键会把执行权交给本探测结构上永远读不到的另一个仓库的配置。
门控用这里枚举的 19 个键 pattern 调用 git config --get-regexp，并把「没有匹配」当作「没有执行风险」。只有在列表完备时这才成立，而它不可能完备：单个未列出的键就能让 Git 去读第二个配置文件。diff.submodule 只是已被证实的一个入口，不是缺陷的全部。
构造一个带一个 submodule 的父项目，在父项目的 .git/config 写入 [diff] submodule = diff，在 submodule 的配置（.git/modules/sub/config）写入 diff.external = touch /tmp/PWNED —— 两者都是克隆仓库自带的、受仓库控制的内容 —— 再把 submodule 检出的提交推进到记录的 gitlink 之后。此时探测完全不输出任何内容（diff.submodule 不匹配任何锚定 pattern），result.status === 1，getLocalGitConfigRisk 返回 NO_RISK，git diff / git log -p 仍被分类为只读并被自动批准。两者都创建了 /tmp/PWNED，因为 show_submodule_inline_diff 会在 submodule 内派生子 git diff，而它读取的是 submodule 的配置并执行其 diff.external。取消 diff.submodule 不会创建任何文件，改用 submodule.recurse = true 也不会，因此该键就是具体触发条件。git-config-safety.test.ts 中 submodule 出现次数为零。
这个类别在另一个方向上也没有「最后一个入口」：这里的每个条目都限定在单个仓库内，因此任何让 Git 去读第二个配置文件的未来键（或 flag），以及 Git 后续版本新增的任何可执行键，都会静默地重新打开同一个漏洞。
建议反转默认行为，而不是加长列表：枚举已知惰性的一小组 local/worktree 键（core.repositoryformatversion、core.bare、core.filemode、core.ignorecase、core.logallrefupdates、remote.*.url/fetch、branch.*.remote/merge、submodule.*.url/branch、user.* 等），对集合之外的任何键 fail closed —— 用一个 inert-key violation 字段取代「每种攻击一个字段」。另外，让探测意识到父项目的答案不是全部答案：如果存在 .gitmodules 或 submodule.* 条目，无论如何都视为有风险，因为 getLocalGitConfigRisk(cwd) 看不到 .git/modules/*/config。
修复必须保持返回记录中每个字段的真实性，因为 shellAstParser.ts:1206 是 return Object.values(getLocalGitConfigRisk(cwd)).some(Boolean);，而设计文档正是为了让未来的风险字段不会被漏出 fallback 门控才采用这种形式 —— 一个对无害仓库也为 true 的字段会静默地在整个 fallback 路径上强制放行。
修复验证：新增一个 .git/config 只含 [diff] submodule = diff 的父项目，断言 getLocalGitConfigRisk(cwd) 报告风险；再加一条端到端用例断言该仓库下 classifyShellCommandSafety('git diff', cwd) 不是 read-only。移除新的 default-deny 字段后两者必须变红 —— 而今天它们会在任意代码执行的情况下保持绿色。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| FILTER_PROCESS_KEY_PATTERN, | ||
| String.raw`^core\.pager$`, | ||
| PAGER_COMMAND_KEY_PATTERN, | ||
| String.raw`^log\.showsignature$`, |
There was a problem hiding this comment.
[Critical] R8-16: [fails-closed] [regression] A valueless config key — git's idiomatic spelling of boolean true — aborts the whole probe into all-eleven-bits-true, so one hand-written line denies every read-only Git command in the repository.
Git emits a valueless entry ([log] newline tab showSignature, no =) as scope\0key\0 with no \n in it. The parse loop reads if (newline < 0) return PROBE_FAILED;, so that perfectly valid git output is treated as a probe failure and every risk bit comes back true. The abort line itself pre-dates this PR, but this diff widens what can reach it from 2 key families to 18, adds the boolean-shaped ones whose bare spelling is idiomatic (log.showsignature, remote.*.promisor, core.hookspath, core.pager, extensions.partialclone, merge.*.driver, gpg.*.program), and makes it fire for entries in any scope — including global, because the abort happens before any scope filtering.
The result is that git status, git log -1, git rev-parse HEAD, git branch, git cat-file and git diff — commands with nothing to do with the offending key — all become unknown, so the shell tool asks every time and Plan mode never runs them; in regex-fallback mode any command merely containing the token git is denied. The same bare key in a user's own ~/.gitconfig turns off read-only Git auto-approval in every repository on the machine, and nothing is logged, because this path returns before the activeRisks diagnostic. Git itself reads the bare key as true (git config --bool --get log.showSignature -> true), so the narrow correct answer is signatureVerifier alone.
Witness:
git 2.43.0, real module via tsx, throwaway git-init repos
raw git output bytes: {"rc":0,"stdout":"local\u0000log.showsignature\u0000"} <- key, NUL, NO \n
git --bool --get log.showSignature: "true" <- git's own reading
PR risk: {all 11 bits true} status/log -1/rev-parse/branch/cat-file/diff -> read-only FALSE
BASE risk: {diffExternal:false, fsmonitor:false} same four commands -> read-only TRUE
sweep of every bare-key family ([log] showSignature, [core] fsmonitor, [core] hooksPath,
[core] pager, [remote "origin"] promisor, [merge "audit"] driver, [diff] external)
-> each returned ALL 11 bits true; unrelated bare key ([core] bare) -> NO_RISK
fix applied (valueless -> 'true'): bare log.showSignature -> all bits false;
+ gpg.program -> signatureVerifier ONLY true; bare core.hooksPath -> hooksPath true;
bare remote.origin.promisor -> promisorRemote true; genuine probe failure -> still all true
PR's own suites with the fix: 2 files passed, 21 tests passed
Keep the wire distinction git actually makes instead of treating "valueless" as "malformed": when newline < 0, record the entry with git's valueless-boolean semantics (value: 'true', or a valueless: true marker the boolean checks honour) and keep PROBE_FAILED for the non-zero/timeout paths. Do not normalise it to '' — isGitTrueValue('') is false, so a bare log.showSignature plus a local gpg.program = /tmp/evil-gpg would report NO_RISK and auto-approve git log -1 while git really does spawn that program, turning today's fail-closed into a fail-open. 'true' is safe in every direction: log.showsignature and remote.*.promisor become true (matching git), core.fsmonitor and core.pager are matched by BOOLEAN_VALUE and stay inert (matching git: built-in daemon, no external pager), and the string families stay non-empty and therefore risky, which is correct since git errors on a valueless value for those.
Two facts the fix must not violate: git-config-safety.ts:120 is if (value === '' || /^(?:false|no|off)$/i.test(value)) return false;, so a valueless key must not arrive there as ''; and git-config-safety.ts:111 const BOOLEAN_VALUE = /^(?:true|false|yes|no|on|off|0|1)$/i; is what keeps bare core.fsmonitor/core.pager inert once mapped to 'true', which two existing tests in this PR depend on. :191 must also keep returning PROBE_FAILED for a genuine probe failure, since the fallback at shellAstParser.ts:1206 consumes that record wholesale.
For a witness, extend the boolean-semantics test with two bare-key repos appended to .git/config: one with [log] newline tab showSignature plus gpg.program = /tmp/evil-gpg, asserting signatureVerifier === true, every other flag false, isShellCommandReadOnlyASTInDirectory('git log -1', cwd) === false and ('git rev-parse HEAD', cwd) === true; and one with the bare key and no local gpg.program, asserting the full risk record equals NO_RISK and ('git status', cwd) === true. Restoring if (newline < 0) return PROBE_FAILED; must turn both red.
One reachability caveat, measured rather than assumed: git's own CLI cannot write a valueless key (git config --add log.showSignature -> rc 129; passing '' writes a valued empty entry; git clone --filter=blob:none writes promisor = true with a value), so the trigger is a hand-edited or third-party-templated config file. That is legal, idiomatic git, and the failure is deterministic once such a line exists.
中文说明
无值配置键 —— git 表示布尔 true 的惯用写法 —— 会让整个探测中止并返回全部十一个风险位为 true，因此一行手写配置就能拒掉仓库里所有只读 Git 命令。
git 把无值条目（[log] 换行 制表符 showSignature，没有 =）输出为 scope\0key\0，其中没有 \n。解析循环里的 if (newline < 0) return PROBE_FAILED; 会把这份完全合法的 git 输出当作探测失败，于是每个风险位都返回 true。中止这一行本身早于本 PR，但这次改动把能触发它的键族从 2 个扩大到 18 个，加入了那些裸写法非常惯用的布尔型键（log.showsignature、remote.*.promisor、core.hookspath、core.pager、extensions.partialclone、merge.*.driver、gpg.*.program），并且让它在任何 scope 的条目上触发 —— 包括 global，因为中止发生在任何 scope 过滤之前。
结果是 git status、git log -1、git rev-parse HEAD、git branch、git cat-file、git diff —— 这些与该键毫无关系的命令 —— 全部变成 unknown，于是 shell 工具每次都要询问，Plan 模式则完全不执行；在 regex fallback 模式下，任何仅仅包含 git 这个词的命令都会被拒。用户自己 ~/.gitconfig 里的同一个裸键会关掉整台机器上每个仓库的只读 Git 自动批准，而且不会有任何日志，因为这条路径在 activeRisks 诊断之前就返回了。git 自己把裸键读作 true（git config --bool --get log.showSignature -> true），所以正确的窄答案是仅 signatureVerifier 为 true。
建议保留 git 真实做出的线上区分，不要把「无值」当作「格式错误」：当 newline < 0 时，按 git 的无值布尔语义记录该条目（value: 'true'，或一个由布尔检查识别的 valueless: true 标记），并把 PROBE_FAILED 留给非零退出/超时路径。不要把它归一化为 '' —— isGitTrueValue('') 是 false，因此裸 log.showSignature 加上本地 gpg.program = /tmp/evil-gpg 会报告 NO_RISK 并自动批准 git log -1，而 git 确实会派生那个程序，等于把今天的 fail-closed 变成 fail-open。'true' 在各个方向上都安全：log.showsignature 与 remote.*.promisor 变为 true（与 git 一致），core.fsmonitor 与 core.pager 被 BOOLEAN_VALUE 匹配而保持惰性（与 git 一致：内建 daemon、无外部 pager），字符串族保持非空因而有风险 —— 这是正确的，因为 git 对这些键的无值写法本身会报错。
修复不得违反两个既有事实：git-config-safety.ts:120 是 if (value === '' || /^(?:false|no|off)$/i.test(value)) return false;，所以无值键不能以 '' 到达那里；git-config-safety.ts:111 的 const BOOLEAN_VALUE = /^(?:true|false|yes|no|on|off|0|1)$/i; 正是让裸 core.fsmonitor/core.pager 在映射为 'true' 后保持惰性的原因，本 PR 的两个现有测试依赖它。:191 也必须继续对真正的探测失败返回 PROBE_FAILED，因为 shellAstParser.ts:1206 的 fallback 会整体消费该记录。
修复验证：在布尔语义测试中扩展两个把裸键追加到 .git/config 的仓库：一个是 [log] 换行 制表符 showSignature 加上 gpg.program = /tmp/evil-gpg，断言 signatureVerifier === true、其他所有标志为 false、isShellCommandReadOnlyASTInDirectory('git log -1', cwd) === false 且 ('git rev-parse HEAD', cwd) === true；另一个只有裸键、没有本地 gpg.program，断言完整风险记录等于 NO_RISK 且 ('git status', cwd) === true。恢复 if (newline < 0) return PROBE_FAILED; 后两者必须变红。
一个经测量而非假设的可达性说明：git 自己的 CLI 无法写出无值键（git config --add log.showSignature -> rc 129；传 '' 会写出带值的空条目；git clone --filter=blob:none 写出的是带值的 promisor = true），所以触发条件是手工编辑或第三方模板生成的配置文件。那是合法且惯用的 git 写法，一旦存在这样的行，失败就是确定性的。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| windowsHide: true, | ||
| }, | ||
| ); | ||
| if (result.status !== 0 || typeof result.stdout !== 'string') return true; |
There was a problem hiding this comment.
[Critical] R7-1: (fix-induced) [fails-closed] [regression] The hooks probe added to close R7-1 fails closed on git rev-parse exit 128, which is what any cwd outside a repository returns — so non-repo directories now report hooksPath: true and four previously green tests are red on this PR.
R7-1's own reported input is closed: the config-only probe is no longer blind to an executable hook in the default .git/hooks/, because hasExecutablePostIndexChangeHook resolves the effective hooks directory independently of config matching and the status === 1 early return now preserves that risk. That part is verified and pinned in both AST and fallback modes. But the change that closed it opened a new defect at the same site. git rev-parse --git-path hooks exits 128 with fatal: not a git repository in a directory that has no repository, and this line turns every non-zero exit into "an executable hook exists". localGitConfigMakesCommandUnsafe then fails closed on risk.hooksPath for all twelve read-only subcommands, not just diff/status.
This is not theoretical. CoreToolScheduler.test.ts > Plan shell routing schedules { command: 'git status' } with no directory, so shell.ts passes config.getTargetDir(), which is /tmp in those tests. The classification becomes unknown, getDefaultPermission returns ask instead of allow, the scheduler parks waiting for a confirmation the test never supplies, and execute is never called. In production the same thing happens for any agent whose cwd is a non-Git directory — a scratch dir, a non-git project, a container volume — and plan-mode routing plus speculative execution stop treating those commands as read-only, in a directory where there is no repository-controlled hook to fail closed against. The branch also fires inside a real repository whenever the probe cannot complete: git absent from the spawned PATH, or either spawnSync exceeding its timeout: 1000 on a loaded runner.
Witness:
A/B measured by the build-test agent, failing-FILE-SET difference (not counts)
BASE arm: coreToolScheduler.test.ts 382/382 pass
PR arm: coreToolScheduler.test.ts 4 failed | 378 passed
AssertionError: expected "spy" to be called once, but got 0 times
red: executes read-only run_shell_command calls without a prompt
executes read-only monitor calls without a prompt
cancels without execution when aborted while awaiting the host guard
skips guard evaluation entirely when no guard is configured
git -C /tmp rev-parse --git-path hooks -> fatal: not a git repository, exit 128
getLocalGitConfigRisk('/tmp') -> {..., hooksPath: true}
classifyShellCommandSafetyInDirectory('git status'|'git log -1'|'git show HEAD'|'git branch','/tmp')
-> unknown (pre-diff: NO_RISK, and log -1 / show HEAD / branch were read-only)
net-new failures on the PR side by measurement: 2 files / 5 tests (this one and R7-2)
the other 11 failing core files (75 tests) fail identically on BOTH arms
(all $HOME/QWEN_HOME path-leak assertions in this runner) -> pre-existing, not filed
Separate "no repository here" from "the probe failed": return false when the failure is not a git repository (rc 128), since a directory with no repository has no repository-controlled helper to execute, and keep true for timeouts (result.status === null) and other genuine failures inside a repository. If fail-closed-on-non-repo is genuinely intended, then this PR must also move those four Plan shell routing tests onto a repository targetDir — as it stands the packages/core suite is red.
The narrowing must not disturb two things this PR pins: git-shell-execution-safety.test.ts:67-90 requires, inside a real git init repo, that core.hooksPath=.githooks yields hooksPath === true and git status is not read-only in both AST and _setParserFailedForTesting() fallback modes; and PROBE_FAILED must still be returned when the config probe fails inside a repository, which the design doc this diff adds states as "probe/parse failures fail closed".
For a witness, the four CoreToolScheduler.test.ts > Plan shell routing tests above are already it — green at base, they must be green again. Add to git-config-safety.test.ts an assertion that getLocalGitConfigRisk(<dir with no .git>).hooksPath === false and that isShellCommandReadOnlyASTInDirectory('git log -1', <that dir>) === true; dropping the rc-128 branch must turn it red.
中文说明
为关闭 R7-1 而新增的 hooks 探测在 git rev-parse 退出码 128 时 fail closed，而 128 正是任何位于仓库之外的 cwd 会得到的结果 —— 于是非仓库目录现在会报告 hooksPath: true，并且有四个原本通过的测试在本 PR 上变红。
R7-1 自己报告的输入已经关闭：仅看配置的探测不再对默认 .git/hooks/ 中的可执行 hook 视而不见，因为 hasExecutablePostIndexChangeHook 会独立于配置匹配去解析生效的 hooks 目录，且 status === 1 的提前返回现在会保留该风险。这部分已验证，并在 AST 与 fallback 两种模式下都有测试固定。但关闭它的改动在同一位置打开了一个新缺陷。git rev-parse --git-path hooks 在没有仓库的目录里会以 fatal: not a git repository 退出 128，而这一行把所有非零退出都当成「存在可执行 hook」。随后 localGitConfigMakesCommandUnsafe 会对全部十二个只读子命令（不只是 diff/status）在 risk.hooksPath 上 fail closed。
这不是理论问题。CoreToolScheduler.test.ts > Plan shell routing 调度的是不带 directory 的 { command: 'git status' }，因此 shell.ts 传入 config.getTargetDir()，在这些测试中是 /tmp。分类结果变成 unknown，getDefaultPermission 返回 ask 而不是 allow，调度器停住等待一个测试永远不会给出的确认，于是 execute 从未被调用。生产环境中，任何 cwd 是非 Git 目录的 agent（临时目录、非 git 项目、容器卷）都会遇到同样的情况，plan-mode 路由与推测执行也不再把这些命令当作只读 —— 而在一个根本没有仓库的目录里，并不存在需要 fail closed 的、受仓库控制的 hook。该分支在真实仓库内也会在探测无法完成时触发：派生 PATH 中没有 git，或者在负载较高的 runner 上任一 spawnSync 超过其 timeout: 1000。
建议把「这里没有仓库」与「探测失败」区分开：当失败原因是 not a git repository（rc 128）时返回 false，因为没有仓库的目录就不存在受仓库控制的外部程序可执行；对超时（result.status === null）以及仓库内其他真正的失败保持返回 true。如果确实希望对非仓库 fail closed，那么本 PR 还必须把那四个 Plan shell routing 测试改到仓库型的 targetDir 上 —— 按当前状态，packages/core 测试套件是红的。
收窄不得破坏本 PR 固定的两件事：git-shell-execution-safety.test.ts:67-90 要求在真实 git init 仓库内，core.hooksPath=.githooks 得到 hooksPath === true 且 git status 在 AST 与 _setParserFailedForTesting() fallback 两种模式下都不是只读；以及当配置探测在仓库内部失败时仍必须返回 PROBE_FAILED，本次改动新增的设计文档把它表述为「探测/解析失败时 fail closed」。
修复验证：上面四个 CoreToolScheduler.test.ts > Plan shell routing 测试本身就是验证 —— 它们在 base 上是绿的，必须重新变绿。另在 git-config-safety.test.ts 中断言 getLocalGitConfigRisk(<无 .git 的目录>).hooksPath === false 且 isShellCommandReadOnlyASTInDirectory('git log -1', <该目录>) === true；移除 rc-128 分支后该断言必须变红。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
|
|
||
| const hooksDir = result.stdout.trim(); | ||
| if (!hooksDir) return true; | ||
| const hookPath = path.resolve(cwd, hooksDir, 'post-index-change'); |
There was a problem hiding this comment.
[Critical] R8-15: [certifies-falsely] [new-surface] The hook probe joins a logical caller-supplied cwd to a hooks path git computed relative to its own physical cwd, so a symlinked workspace resolves to a path that does not exist and the brand-new gate fails open.
git rev-parse --git-path hooks answers relative to the cwd git actually ran in, and git chdirs first — so the kernel has already resolved every symlink in it. path.resolve(cwd, hooksDir, 'post-index-change') then re-joins that answer onto the caller's unresolved string. When the two differ and git's answer contains .., the result points somewhere that does not exist, statSync throws ENOENT, and line 150 returns false — no hook — on the exact gate this function was added to close.
Take the ordinary monorepo shape ~/app -> ~/monorepo/packages/app: repo root /tmp/ra10201/mono with an executable .git/hooks/post-index-change and no core.hooksPath (so the presence disjunct at line 288 cannot save it), workspace /tmp/ra10201/app a symlink into it. git -C /tmp/ra10201/app rev-parse --git-path hooks prints ../../.git/hooks, and path.resolve('/tmp/ra10201/app', '../../.git/hooks', 'post-index-change') is /tmp/.git/hooks/post-index-change. All eleven risk bits come back false, git status classifies read-only, getDefaultPermission returns allow, and the repository-controlled hook runs with no confirmation — the hook really is live on that path, since git status from the symlinked cwd created the sentinel. A relative core.hooksPath breaks identically, because git re-bases it against the worktree root and re-expresses it relative to cwd (git -C sub rev-parse --git-path hooks -> ../.husky).
This needs a caller- or client-supplied path rather than process.cwd(), which is already physical: the tool's directory parameter, or a workspace string passed to loadCliConfig and stored unmodified at packages/cli/src/config/config.ts:1958 (targetDir: cwd). validateToolParams (shell.ts:5134-5152) checks only that directory is absolute, outside the user-skills dirs and inside a registered workspace — never that it is canonical.
Witness:
repo root /tmp/ra10201/mono, executable .git/hooks/post-index-change, NO core.hooksPath
workspace /tmp/ra10201/app -> symlink to /tmp/ra10201/mono/packages/app
rev-parse --git-path hooks (from symlinked cwd): "../../.git/hooks"
path.resolve(cwd, hooksDir): "/tmp/.git/hooks" resolved exists: false
risk via symlink: {... "hooksPath": false}
risk via realpath: {... "hooksPath": true}
classify git status via symlink: {"readOnly": true} -> getDefaultPermission 'allow'
hook fired by git status from symlinked cwd: {"sentinelCreated": true}
hook fired by git diff from symlinked cwd: {"sentinelCreated": false}
one-line fix applied (realpathSync base), same probe:
risk via symlink -> hooksPath: true ; classify git status -> readOnly: false
BASE arm: no probe and no protection existed at all (base risk record has 2 keys)
Resolve against the physical cwd: let base = cwd; try { base = realpathSync(cwd); } catch { /* keep cwd */ } and then path.resolve(base, hooksDir, 'post-index-change'), adding realpathSync to the existing node:fs import. Asking git instead (rev-parse --path-format=absolute --git-path hooks) also works but needs git >= 2.31, and on older git the probe would error and fail closed for every repository — the over-block R7-1 above is already about — so realpathSync is the better fix. It is also the in-tree convention for trust decisions (packages/cli/src/config/config.ts:952 already uses fs.realpathSync), which :1958 does not follow.
git-config-safety.ts:150 — return code === 'ENOENT' ? false : true; — must stay exactly as it is: a missing post-index-change is the normal state of every repository, so the fix has to make the path correct rather than make ENOENT fail closed.
For a witness, next to the existing 0o755 post-index-change case in git-shell-execution-safety.test.ts (:75-90), add a repo whose git root is above the workspace, make the workspace a symlink into a subdirectory of it, and assert getLocalGitConfigRisk(symlinkedWorkspace).hooksPath === true plus classifyShellCommandSafetyInDirectory('git status', symlinkedWorkspace) !== 'read-only'. Reverting the fix to path.resolve(cwd, ...) must turn both red.
One honest limit on trigger realism: the end-to-end chain and the hook firing were both observed, but no real client was observed sending a symlinked directory — the exposure is that any caller-supplied path with a symlink component reaches it.
中文说明
hook 探测把调用方提供的逻辑 cwd 与 git 相对于自身物理 cwd 计算出的 hooks 路径拼接在一起，因此软链接工作区会解析到一个不存在的路径，令这个全新的门控 fail open。
git rev-parse --git-path hooks 的回答是相对于 git 实际运行时所在的 cwd 的，而 git 会先 chdir —— 所以内核已经解析掉了其中的每个软链接。随后 path.resolve(cwd, hooksDir, 'post-index-change') 又把这个答案拼回调用方未解析的字符串上。当两者不一致且 git 的答案包含 .. 时，结果会指向一个不存在的位置，statSync 抛出 ENOENT，第 150 行返回 false —— 即「没有 hook」—— 而这恰恰发生在该函数被新增来关闭的那道门控上。
以最普通的 monorepo 形态 ~/app -> ~/monorepo/packages/app 为例：仓库根 /tmp/ra10201/mono 带一个可执行的 .git/hooks/post-index-change 且没有 core.hooksPath（因此第 288 行的存在性析取项救不了它），工作区 /tmp/ra10201/app 是指向其内部的软链接。git -C /tmp/ra10201/app rev-parse --git-path hooks 输出 ../../.git/hooks，而 path.resolve('/tmp/ra10201/app', '../../.git/hooks', 'post-index-change') 是 /tmp/.git/hooks/post-index-change。十一个风险位全部返回 false，git status 被分类为只读，getDefaultPermission 返回 allow，受仓库控制的 hook 在没有任何确认的情况下运行 —— 该 hook 在这条路径上确实是活的，因为从软链接 cwd 执行 git status 创建了哨兵文件。相对形式的 core.hooksPath 会以完全相同的方式失效，因为 git 会把它重新基于 worktree 根、并相对 cwd 重新表达（git -C sub rev-parse --git-path hooks -> ../.husky）。
这需要调用方或客户端提供的路径，而不是已经是物理路径的 process.cwd()：工具的 directory 参数，或传给 loadCliConfig 并在 packages/cli/src/config/config.ts:1958（targetDir: cwd）原样保存的工作区字符串。validateToolParams（shell.ts:5134-5152）只检查 directory 是否为绝对路径、是否在 user-skills 目录之外、是否在已注册工作区之内 —— 从不检查它是否已规范化。
建议针对物理 cwd 做解析：let base = cwd; try { base = realpathSync(cwd); } catch { /* keep cwd */ }，然后 path.resolve(base, hooksDir, 'post-index-change')，并把 realpathSync 加入现有的 node:fs import。改为问 git（rev-parse --path-format=absolute --git-path hooks）也可行，但需要 git >= 2.31，而在更旧的 git 上探测会报错并对每个仓库 fail closed —— 那正是上面 R7-1 所述的过度拦截 —— 所以 realpathSync 是更好的修复。它也符合仓库内既有的信任判定惯例（packages/cli/src/config/config.ts:952 已使用 fs.realpathSync），而 :1958 没有遵循。
git-config-safety.ts:150 —— return code === 'ENOENT' ? false : true; —— 必须原样保留：缺少 post-index-change 是每个仓库的常态，所以修复要让路径正确，而不是让 ENOENT fail closed。
修复验证：在 git-shell-execution-safety.test.ts 现有 0o755 post-index-change 用例（:75-90）旁边，新增一个 git 根位于工作区之上的仓库，把工作区做成指向其子目录的软链接，断言 getLocalGitConfigRisk(symlinkedWorkspace).hooksPath === true 且 classifyShellCommandSafetyInDirectory('git status', symlinkedWorkspace) !== 'read-only'。把修复回退为 path.resolve(cwd, ...) 后两者必须变红。
关于触发真实性的一个诚实限制：端到端链路和 hook 触发都被观察到，但没有观察到真实客户端发送带软链接的 directory —— 暴露面在于任何带软链接分量的调用方路径都能到达这里。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| alternateRefsCommand !== undefined && alternateRefsCommand !== '', | ||
| hooksPath: | ||
| executablePostIndexChangeHook || | ||
| (hooksPath !== undefined && hooksPath !== ''), |
There was a problem hiding this comment.
[Critical] R8-3: [fails-closed] [regression] A merely configured core.hooksPath sets the risk bit, so every husky / lefthook / pre-commit repository loses read-only Git auto-approval entirely — including the repository this review ran in.
hooksPath is executablePostIndexChangeHook || (hooksPath !== undefined && hooksPath !== ''). The second disjunct discards the probe's own proof that no executable hook exists: when core.hooksPath is set but the directory holds no post-index-change, the bit is true anyway, and localGitConfigMakesCommandUnsafe puts risk.hooksPath in its unconditional fail-closed branch, so all twelve read-only subcommands ask.
Measured in this very checkout: git config --local --get core.hooksPath -> .husky (husky sets it during npm ci), .husky/ contains only pre-commit, git rev-parse --git-path hooks -> .husky. hasExecutablePostIndexChangeHook correctly returns false, yet getLocalGitConfigRisk(<this repo>) returns {...all false..., hooksPath: true} and git status, git log -1 and git diff all classify non-read-only. That contradicts both the linked issue's acceptance criterion — "ordinary Git reads remain auto-approved" — and the model this diff states at shellAstParser.ts:1161-1162 ("Only narrowly proven non-consumers stay auto-approved"), in a case where the non-consumer is narrowly proven. gitWorktreeService.configureHooksPath (gitWorktreeService.ts:1798-1808) also writes core.hooksPath whenever it is unset, with no --local on the write, so from a linked worktree without extensions.worktreeConfig it lands in the main repository's .git/config — the first Qwen worktree creation can set it permanently.
Witness:
A/B on each arm's own built dist (arm proof: `hookspath` occurrences in the
compiled git-config-safety.js -- BASE: 0, PR: 2)
fixture repo, core.hooksPath=.githooks, .githooks/ holds only an executable pre-commit:
hooks-dir: .githooks post-index-change exists: false
BASE risk= {"diffExternal":false,"fsmonitor":false}
BASE readOnly("git status")=true ("git log -1")=true ("git diff")=true
PR risk= {..."hooksPath":true}
PR readOnly("git status")=false ("git log -1")=false ("git diff")=false
this review's own repository (husky):
hooksPath cfg: .husky .husky listing: . .. pre-commit
PR risk: {...all false..., "hooksPath":true} readOnly(status/log -1/diff)=false
fix applied (hooksPath: executablePostIndexChangeHook), then reverted:
fixture -> hooksPath:false, all three read-only again
this husky repo -> hooksPath:false, all three read-only again
and all four "a real executable post-index-change exists" arms STILL fail closed:
default .git/hooks | configured .githooks | configured dir seen from a
subdirectory (git returns ../../.githooks) | core.hooksPath=~/hooks (git expands ~)
test cost: exactly the two assertions named below fail, nothing else
Drive the bit from the resolved hook file only — hooksPath: executablePostIndexChangeHook — and drop the presence disjunct. git rev-parse --git-path hooks already returns the configured directory when core.hooksPath is set, so a repository-checked-in .husky/post-index-change (attacker-shippable, since hooks directories are tracked) still fails closed, and R7-1's default-.git/hooks protection is unchanged. The verifier could not construct a single state where the second disjunct adds coverage.
Two existing assertions pin the opposite today and must be rewritten in the same change: git-shell-execution-safety.test.ts "fails closed for alternate-refs commands and repository hook paths" (core.hooksPath=.githooks -> hooksPath true, git status false) and "keeps the new broad Git risks fail-closed in parser fallback mode" (['core.hooksPath', '.githooks', 'git status']). Point them at a directory holding an executable post-index-change so the downgrade is preserved for the case that matters. git-config-safety.ts:141 must keep resolving through path.resolve(cwd, hooksDir, 'post-index-change'), since the no-config case pinned by "detects executable hooks in the default Git hooks directory without config" depends on it — and shellAstParser.ts:1206's Object.values(...).some(Boolean) means every field of the record must stay truthful.
For a witness, add the complementary case the current code fails: core.hooksPath=.husky with only pre-commit present keeps git status read-only. That assertion is red today and goes green with the one-line change, while the rewritten executable-hook cases keep the downgrade pinned.
中文说明
仅仅是配置了 core.hooksPath 就会置起风险位，因此每个使用 husky / lefthook / pre-commit 的仓库都会完全失去只读 Git 自动批准 —— 包括本次评审所处的那个仓库。
hooksPath 是 executablePostIndexChangeHook || (hooksPath !== undefined && hooksPath !== '')。第二个析取项丢弃了探测自身给出的「不存在可执行 hook」的证明：当 core.hooksPath 已设置但目录里没有 post-index-change 时，该位仍为 true，而 localGitConfigMakesCommandUnsafe 把 risk.hooksPath 放在其无条件 fail-closed 分支中，于是全部十二个只读子命令都会询问。
在当前这个检出中实测：git config --local --get core.hooksPath -> .husky（husky 在 npm ci 期间设置），.husky/ 只有 pre-commit，git rev-parse --git-path hooks -> .husky。hasExecutablePostIndexChangeHook 正确返回 false，但 getLocalGitConfigRisk(<本仓库>) 返回 {...全 false..., hooksPath: true}，且 git status、git log -1、git diff 全部分类为非只读。这既违背关联 issue 的验收标准 ——「普通 Git 读取仍保持自动批准」—— 也违背本次改动在 shellAstParser.ts:1161-1162 陈述的模型（「只有被严格证明的非消费者才保持自动批准」），而这里恰恰是一个已被严格证明的非消费者。gitWorktreeService.configureHooksPath（gitWorktreeService.ts:1798-1808）还会在该键未设置时写入 core.hooksPath，且写入时没有 --local，因此从没有 extensions.worktreeConfig 的链接 worktree 执行时，它会落到主仓库的 .git/config 里 —— 第一次创建 Qwen worktree 就可能永久设置它。
建议只用解析出的 hook 文件来驱动该位 —— hooksPath: executablePostIndexChangeHook —— 并去掉存在性析取项。git rev-parse --git-path hooks 在设置了 core.hooksPath 时已经会返回配置的目录，因此仓库内检入的 .husky/post-index-change（由于 hooks 目录是被跟踪的，攻击者可以随仓库分发）仍会 fail closed，而 R7-1 对默认 .git/hooks 的保护保持不变。验证者无法构造出第二个析取项能增加覆盖的任何一个状态。
今天有两个现有断言固定了相反的行为，必须在同一次改动中重写：git-shell-execution-safety.test.ts 的 "fails closed for alternate-refs commands and repository hook paths"（core.hooksPath=.githooks -> hooksPath 为 true、git status 为 false）与 "keeps the new broad Git risks fail-closed in parser fallback mode"（['core.hooksPath', '.githooks', 'git status']）。把它们指向一个确实含有可执行 post-index-change 的目录，这样真正重要的那种情况仍被降级固定。git-config-safety.ts:141 必须继续通过 path.resolve(cwd, hooksDir, 'post-index-change') 解析，因为 "detects executable hooks in the default Git hooks directory without config" 所固定的无配置场景依赖它 —— 而 shellAstParser.ts:1206 的 Object.values(...).some(Boolean) 意味着该记录的每个字段都必须保持真实。
修复验证：补上当前代码通不过的那个互补用例 —— 只含 pre-commit 的 core.hooksPath=.husky 应让 git status 保持只读。该断言今天是红的，一行改动后变绿，同时重写后的可执行 hook 用例继续固定降级行为。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| return /[[*?]/.test(text) || hasShellBraceExpansion(text); | ||
| return ( | ||
| /[[*?]/.test(text) || | ||
| /\\./.test(text) || |
There was a problem hiding this comment.
[Critical] R7-2: (fix-induced) [fails-closed] [regression] The /\\./ term added to the shared hasShellPatternExpansion to close R7-2 leaves packages/core red at this commit and reclassifies ordinary read-only commands across ten root commands.
R7-2's own input is closed — all four backslash-escaped helper-option spellings it named now classify unknown in the AST and false in the fallback, and the new test pins them. But the fix landed in a predicate shared by awk|find|git|printf|rg|ripgrep|sed|sort|tree|uniq, and it broke two things there.
First, the suite does not pass. shellAstParser.test.ts > "classifies adversarial rule inputs in bounded time" is pre-existing and untouched by this diff, and it asserts 'read-only' for its sixth case, git status followed by '\\{'.repeat(10_000). That argument is a word node, so hasShellExpansion feeds its text here, /\\./ matches \\{, and the git gate downgrades the result to 'unknown'. Nothing in CI catches this: gh pr checks for this PR returns checks that are all review automation, with no test, build, lint or typecheck job among them, and the fork workflows are still action_required.
Second, the term reaches far past the escaped-helper-option case the new test names. git show HEAD\\~1, git log --author=John\\ Doe, git log --grep=fix\\., git log --pretty=format:%h\\ %s, git status path\\ with\\ space.txt, rg foo\\.bar, sed s/a\\.b/c/ file, find . -name foo\\ bar, printf %s\\n hello and sort -k1\\,2 file.txt all flip read-only -> unknown, while their quoted twins (rg 'foo\\.bar', printf '%s\\n') stay read-only, because hasShellExpansion only inspects word/concatenation node text. Escaped spaces in paths, \\(/\\) in grep patterns and %s\\n in printf are everyday shapes, so agents stop auto-executing them and prompt instead — and in Plan mode they do not run at all. Note that git log --grep='v1\\.2' -1, an ordinary history search for a version tag, is one of them.
Witness:
A/B on the two built dists (arm proof: BASE `return /[[*?]/.test(text) || hasShellBraceExpansion(text);`
PR `... || /\\./.test(text) || /%G/.test(text) || ...`)
PR arm: src/utils/shellAstParser.test.ts 1 failed | 553 passed
BASE arm: src/utils/shellAstParser.test.ts 548/548 green
FAIL classifyShellCommandSafety > classifies adversarial rule inputs in bounded time
expected [ 'unknown', 'read-only', ...(4) ] to deeply equal ...
- "read-only" (expected, element 6) + "unknown" (received)
the divergence is not about the 10 000 repeats:
BASE: classify("git status \\{") = read-only PR: = unknown
mutation (remove only `/\\./.test(text) ||`): that file 554/554 GREEN,
and two of this PR's own new tests go RED ("fails closed for unquoted
backslash-escaped Git helper options", "treats every %G pretty placeholder
as signature-verifying") -- so the term is load-bearing, not stray
quoted escapes stay read-only on BOTH arms (`printf '%s\\n'`, `rg '\\d+'`,
`sed -n 's/\\.$//p'`, `git grep '\\.foo'`, `find . -name '\\*'`)
-- the flip needs an UNQUOTED escape
Scope the escape rule to the git helper-option spelling it was written for, which is also what R8-13 asks for on the same code: normalize a copy of each argument (de-quote, de-escape, and handle the $'...' ANSI-C form) and test that copy against GIT_EXTERNAL_HELPER_OPTION inside evaluateGitSafety's invokesHelper (shellAstParser.ts:800-804), then drop /\\./ from this shared predicate. That keeps --show\\-signature, --ext\\-diff, --textcon\\v and %G\\K failing closed while restoring git status \\{..., git log --author=John\\ Doe and rg foo\\.bar. If instead the broad rule is genuinely intended, re-pin the sixth expectation at shellAstParser.test.ts:1062 to 'unknown' in the same commit and say why — but the two sides must not keep disagreeing.
git-shell-execution-safety.test.ts:132-143 requires git log --show\\-signature, git log --ext\\-diff -p, git grep --textcon\\v needle and git log --format=%G\\K to keep classifying 'unknown' (AST) and false (fallback), so an AST-only escape rule must not regress the fallback side of that assertion.
For a witness, "classifies adversarial rule inputs in bounded time" must go from red to green — keeping its expect(performance.now() - startedAt).toBeLessThan(1000) timing assertion intact — while "fails closed for unquoted backslash-escaped Git helper options" stays green. That pair is what proves the escape rule was scoped rather than deleted; add classifyShellCommandSafety('git log --author=John\\ Doe') === 'read-only' to pin the narrowing itself.
中文说明
为关闭 R7-2 而加入共享 hasShellPatternExpansion 的 /\\./ 项，使 packages/core 在本提交上是红的，并且让十个根命令下的普通只读命令被重新分类。
R7-2 自己报告的输入已经关闭 —— 它点名的四种反斜杠转义 helper 选项写法现在在 AST 中分类为 unknown、在 fallback 中为 false，并且新测试已固定它们。但这个修复落在一个被 awk|find|git|printf|rg|ripgrep|sed|sort|tree|uniq 共用的谓词里，并在那里破坏了两件事。
第一，测试套件不通过。shellAstParser.test.ts 的 "classifies adversarial rule inputs in bounded time" 是既有测试、本次改动并未触碰它，而它对第六个用例（git status 后跟 '\\{'.repeat(10_000)）断言 'read-only'。该参数是一个 word 节点，因此 hasShellExpansion 会把它的文本送到这里，/\\./ 匹配到 \\{，git 门控便把结果降级为 'unknown'。CI 里没有任何东西能发现这一点：本 PR 的 gh pr checks 返回的都是评审自动化检查，其中没有 test、build、lint 或 typecheck 作业，而 fork workflows 仍处于 action_required。
第二，该项的影响远超新测试所针对的转义 helper 选项场景。git show HEAD\\~1、git log --author=John\\ Doe、git log --grep=fix\\.、git log --pretty=format:%h\\ %s、git status path\\ with\\ space.txt、rg foo\\.bar、sed s/a\\.b/c/ file、find . -name foo\\ bar、printf %s\\n hello、sort -k1\\,2 file.txt 全部从 read-only 翻转为 unknown，而它们的引号孪生形式（rg 'foo\\.bar'、printf '%s\\n'）仍保持 read-only，因为 hasShellExpansion 只检查 word/concatenation 节点的文本。路径中的转义空格、grep 模式里的 \\(/\\)、printf 里的 %s\\n 都是日常写法，于是 agent 不再自动执行它们而改为询问 —— 在 Plan 模式下则完全不执行。注意 git log --grep='v1\\.2' -1（按版本标签搜索历史的普通命令）也在其中。
建议把转义规则收窄到它本来要针对的 git helper 选项写法，这也正是 R8-13 对同一段代码提出的要求：对每个参数的一个副本做归一化（去引号、去转义，并处理 $'...' 的 ANSI-C 形式），在 evaluateGitSafety 的 invokesHelper（shellAstParser.ts:800-804）里用该副本去匹配 GIT_EXTERNAL_HELPER_OPTION，然后从这个共享谓词中去掉 /\\./。这样 --show\\-signature、--ext\\-diff、--textcon\\v、%G\\K 仍然 fail closed，同时 git status \\{...、git log --author=John\\ Doe、rg foo\\.bar 恢复只读。如果确实想要这条宽泛规则，就在同一个提交里把 shellAstParser.test.ts:1062 的第六个期望重新固定为 'unknown' 并说明原因 —— 但两边不能继续互相矛盾。
git-shell-execution-safety.test.ts:132-143 要求 git log --show\\-signature、git log --ext\\-diff -p、git grep --textcon\\v needle、git log --format=%G\\K 继续分类为 'unknown'（AST）与 false（fallback），因此仅在 AST 侧生效的转义规则不得让该断言的 fallback 一侧退化。
修复验证："classifies adversarial rule inputs in bounded time" 必须由红变绿 —— 同时保持其 expect(performance.now() - startedAt).toBeLessThan(1000) 计时断言不变 —— 而 "fails closed for unquoted backslash-escaped Git helper options" 必须保持绿色。这一对正是「转义规则被收窄而非删除」的证明；再加一条 classifyShellCommandSafety('git log --author=John\\ Doe') === 'read-only' 把收窄本身固定下来。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| /^(add|remove|rm|rename|set-branches|set-head|set-url|update)$/; | ||
| const GIT_EXTERNAL_HELPER_OPTION = | ||
| /^--(?:ext-diff|filters|show-signature|textconv|open-files-in-pager)(?:=|$)/; | ||
| /^--(?:ext-diff|filters|show-signature|textconv|open-files-in-pager|remerge-diff)(?:=|$)|^--diff-merges=(?:remerge|r)$/; |
There was a problem hiding this comment.
[Critical] R8-13: [certifies-falsely] [new-surface] Every option-shape gate on the AST path matches raw argument-node text, so any spelling of a helper option the enumerated regex does not literally match is invisible to it — seven bypassing spellings measured, each executing a repository-controlled program under a read-only verdict.
This is a class finding, not one instance. The surface is hand-rolled matching of an untrusted-input grammar (how a shell token can spell a git option), and it is not closable entrance by entrance: this PR closed the backslash entrance and four other spellings are open at the same commit. Absorbed here as evidence, each independently executed with a sentinel: the space-separated value form (--diff-merges remerge, --diff-merges r), quote-concatenation (--ext""-diff, --out""put=, -""delete, -""print, --remerge""-diff), git's unique-prefix abbreviation (--textc, --textco, --filt), and ANSI-C quoting ($'--ext-diff', $'--ext\x2ddiff', git diff $'--textconv').
The mechanism differs per family but the root is one. options.some((arg) => GIT_EXTERNAL_HELPER_OPTION.test(arg)) tests raw node text: stripOuterQuotes strips only when a token starts and ends with a matching quote, so a $ prefix or spliced quotes defeat it; the matcher sees one token at a time, so a value in the following token (--diff-merges remerge) is never compared; and hasShellExpansion pattern-tests only word/concatenation nodes, while tree-sitter types $'...' as ansi_c_string, which is also absent from SHELL_EXPANSION_TYPES. The deprecated regex fallback is not affected — normalizeTokens tokenizes with shell-quote, so quoted spellings collapse there — which makes the primary path strictly weaker than the path it replaces.
The two entrances that matter most do not depend on the cwd-less consumers at all: in a repo whose own .git/config sets diff.external or diff.<driver>.textconv, isShellCommandReadOnlyASTInDirectory returns true for git log --ext""-diff -p and git grep --textc needle, because risk.diffExternal is consumed only for subcommand === 'diff' and risk.diffDriverTextconv only for blame|diff|log|show. So the command-side option test is the only protection for log/show/grep, and it is the layer that leaks.
Witness:
git 2.43.0 + real bash, throwaway repos with sentinel helpers; PR's own built dist
git log --diff-merges=remerge -1 -> ast unknown, regex false, DRIVER_EXECUTED
git log --diff-merges remerge -1 -> ast READ-ONLY, regex TRUE, DRIVER_EXECUTED
(both spellings produce identical git output; git rc=0 for both)
git log --ext""-diff -p -> astInDir TRUE (repo-local diff.external),
ran rc=0 SENTINEL CREATED; bare `git log -p` control: none
git grep --textc needle -> astInDir TRUE, SENTINEL CREATED (rc=1);
git grep --textconv needle -> unknown; git grep needle (control) -> no sentinel
git log -p $'--ext-diff' -> READ-ONLY, SENTINEL CREATED, git rc=0
controls: git log -p --ext-diff -> unknown; git log -p '--ext-diff' -> unknown
git diff --out""put=/tmp/OUT -> ast read-only (plain spelling -> write), file CREATED
find . -name victim.txt -""delete-> ast read-only (plain -> write), file DELETED
real ShellTool: getDefaultPermission("git grep --textc needle") = allow
("git grep --textconv needle") = ask ("git log -p -1") = ask
fallback is stricter, measured: regexFallbackQuoted = false for all four quoted spellings
BASE arm: closed three spellings base allowed (--diff-merges=remerge, =r, --remerge-diff)
and left every bypassing spelling exactly as permissive as base
three quoted spellings FAIL CLOSED (sed -""i, sed --in-""place, git remote show -""n -> unknown)
-> a per-entrance patch is not a fix
Normalize the argument text that option-shape predicates are matched against, the way the fallback already does. Build a normalized copy in evaluateCommandSafety used only for the option-shape tests — GIT_EXTERNAL_HELPER_OPTION, evaluateOutputOption, the evaluateFindSafety/evaluateSedSafety predicates, and the git remote -n test — leaving args untouched for pattern and value consumers. The copy must de-quote, de-escape, and handle the $'...' prefix with its \xHH/\NNN/\c sequences, because a de-quote/de-escape alone does not close ANSI-C: $'--ext-diff' has no adjacent identical quotes and no backslash, and $'--ext\x2ddiff' de-escapes to --extx2ddiff, still unmatched. Make the helper-option test pairwise so a value in the following token is seen — options.some((arg, i) => GIT_EXTERNAL_HELPER_OPTION.test(arg) || (arg === '--diff-merges' && /^(?:remerge|r)$/.test(options[i + 1] ?? ''))) — mirrored at shellReadOnlyChecker.ts:247. Do not try to make the regex abbreviation-tolerant: git's abbreviation rules are per-subcommand and unstable (git grep --textc accepted, git log --textc and git log --ext-di rejected rc=128, git blame --textc rejected rc=129), so the config layer is where abbreviation must fail closed — see the cat-file/grep finding for that half.
Three constraints the fix must respect: git-shell-execution-safety.test.ts:132-143 requires the four backslash spellings to keep classifying 'unknown'/false, so de-quoting must handle \X and quotes together rather than replace the /\\./ term; shellAstParser.ts:1001's args is the same array evaluateSedSafety/evaluateAwkSafety consume, so normalization must be a copy or it will corrupt sed 's/a""/b/'-style pattern arguments; and both copies of the pattern must change together, since they are already not identical (shellReadOnlyChecker.ts:90 carries an extra |%G term that shellAstParser.ts:142 lacks). git-config-safety.test.ts:396-401 also pins git diff and git status as read-only with merge.audit.driver set, which bounds how much over-denial a bare---diff-merges fix may introduce.
For a witness, add one case per family to git-shell-execution-safety.test.ts beside "fails closed for unquoted backslash-escaped Git helper options": classifyShellCommandSafety('git log --diff-merges remerge -1') -> 'unknown', classifyShellCommandSafety('git show --remerge""-diff') -> 'unknown', classifyShellCommandSafety('git diff --out""put=/tmp/x') -> not 'read-only', classifyShellCommandSafety("git log -p $'--ext-diff'") -> 'unknown', isShellCommandReadOnlyASTInDirectory('git log --ext""-diff -p', <repo with diff.external set>) -> false, and classifyShellCommandSafety('git grep --textc needle') -> 'unknown' in a repo with a textconv driver. Every one is red at this commit and goes green only with the normalization.
中文说明
AST 路径上所有基于选项形状的门控匹配的都是原始参数节点文本，因此凡是枚举正则没有字面匹配到的 helper 选项写法，它都看不见 —— 实测有七种可绕过写法，每一种都在 read-only 判定下执行了受仓库控制的程序。
这是一条类别级发现，不是单个实例。该表面是对不可信输入语法（shell token 能如何拼写一个 git 选项）的手写匹配，无法逐个入口关闭：本 PR 关闭了反斜杠入口，而在同一提交上还有另外四种写法是敞开的。以下都作为证据并入本条，且每一种都用哨兵独立执行验证过：空格分隔的值形式（--diff-merges remerge、--diff-merges r）、引号拼接（--ext""-diff、--out""put=、-""delete、-""print、--remerge""-diff）、git 的唯一前缀缩写（--textc、--textco、--filt），以及 ANSI-C 引用（$'--ext-diff'、$'--ext\x2ddiff'、git diff $'--textconv'）。
各家族的机制不同，但根因只有一个。options.some((arg) => GIT_EXTERNAL_HELPER_OPTION.test(arg)) 检测的是原始节点文本：stripOuterQuotes 只在 token 以匹配的引号开头且结尾时才剥离，所以 $ 前缀或拼接引号就能绕过它；匹配器一次只看一个 token，因此位于下一个 token 的值（--diff-merges remerge）永远不会被比较；而 hasShellExpansion 只对 word/concatenation 节点做模式检测，tree-sitter 却把 $'...' 归为 ansi_c_string，它同样不在 SHELL_EXPANSION_TYPES 中。已废弃的 regex fallback 不受影响 —— normalizeTokens 用 shell-quote 分词，引号写法在那里会被折叠 —— 这使得主路径严格弱于它所替代的那条路径。
最重要的两个入口根本不依赖无 cwd 的消费者：在自身 .git/config 设置了 diff.external 或 diff.<driver>.textconv 的仓库里，isShellCommandReadOnlyASTInDirectory 对 git log --ext""-diff -p 和 git grep --textc needle 都返回 true，因为 risk.diffExternal 只在 subcommand === 'diff' 时被消费，risk.diffDriverTextconv 只对 blame|diff|log|show 生效。所以对 log/show/grep 而言，命令侧的选项检测是唯一保护，而漏的正是这一层。
建议按 fallback 已有的做法，对用于选项形状谓词匹配的参数文本做归一化。在 evaluateCommandSafety 中构造一个归一化的副本，只用于选项形状检测 —— GIT_EXTERNAL_HELPER_OPTION、evaluateOutputOption、evaluateFindSafety/evaluateSedSafety 谓词，以及 git remote 的 -n 检测 —— 而 args 保持原样供模式与值类消费者使用。该副本必须去引号、去转义，并处理 $'...' 前缀及其 \xHH/\NNN/\c 序列，因为仅去引号/去转义无法关闭 ANSI-C：$'--ext-diff' 既没有相邻的相同引号也没有反斜杠，而 $'--ext\x2ddiff' 去转义后是 --extx2ddiff，仍然不匹配。让 helper 选项检测成对进行，以便看到下一个 token 中的值 —— options.some((arg, i) => GIT_EXTERNAL_HELPER_OPTION.test(arg) || (arg === '--diff-merges' && /^(?:remerge|r)$/.test(options[i + 1] ?? ''))) —— 并在 shellReadOnlyChecker.ts:247 做同样的镜像。不要试图让正则容忍缩写：git 的缩写规则是按子命令且不稳定的（git grep --textc 被接受，git log --textc 与 git log --ext-di 以 rc=128 被拒，git blame --textc 以 rc=129 被拒），所以缩写必须在配置层 fail closed —— 那一半见 cat-file/grep 那条发现。
修复需遵守三个约束：git-shell-execution-safety.test.ts:132-143 要求四种反斜杠写法继续分类为 'unknown'/false，所以去引号必须与 \X 一起处理，而不是替换 /\\./ 项；shellAstParser.ts:1001 的 args 正是 evaluateSedSafety/evaluateAwkSafety 消费的同一个数组，因此归一化必须是副本，否则会破坏 sed 's/a""/b/' 这类模式参数；两份 pattern 必须一起修改，因为它们本来就不相同（shellReadOnlyChecker.ts:90 多了一个 shellAstParser.ts:142 没有的 |%G 项）。git-config-safety.test.ts:396-401 还固定了在设置 merge.audit.driver 时 git diff 与 git status 为只读，这限定了裸 --diff-merges 修复可以引入多少过度拒绝。
修复验证：在 git-shell-execution-safety.test.ts 的 "fails closed for unquoted backslash-escaped Git helper options" 旁边，每个家族加一条用例：classifyShellCommandSafety('git log --diff-merges remerge -1') -> 'unknown'、classifyShellCommandSafety('git show --remerge""-diff') -> 'unknown'、classifyShellCommandSafety('git diff --out""put=/tmp/x') -> 不是 'read-only'、classifyShellCommandSafety("git log -p $'--ext-diff'") -> 'unknown'、isShellCommandReadOnlyASTInDirectory('git log --ext""-diff -p', <设置了 diff.external 的仓库>) -> false，以及在配置了 textconv driver 的仓库中 classifyShellCommandSafety('git grep --textc needle') -> 'unknown'。这些在本提交上全部是红的，只有做了归一化才会变绿。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| return mergeSafety( | ||
| node.parent?.namedChildCount === 1 ? 'read-only' : 'unknown', | ||
| node.parent?.namedChildCount === 1 && | ||
| node.parent.type !== 'compound_statement' |
There was a problem hiding this comment.
[Critical] R8-28: [certifies-falsely] The new persistent-assignment guard denylists exactly one parent node type, so the same hazard still classifies read-only through the negated_command (!) wrapper — and a repository can ship the payload it needs.
The guard added here narrows a pre-existing rule so that a lone assignment inside { ...; } is no longer read-only, on the stated rationale that brace groups execute in the current shell and the assignment therefore persists into the following command. CHILD_STATEMENT (shellAstParser.ts:677) already lists negated_command as a recursed-into current-shell wrapper, and ! has exactly the same persistence semantics — but it is not in the denylist, so node.parent?.namedChildCount === 1 is satisfied and the assignment stays read-only.
classifyShellCommandSafety('! HOME=.; git diff') returns 'read-only', and so does the directory-aware form, so getDefaultPermission returns 'allow' with no dialog. Running that exact string with bash -c from inside a repository that ships ./.gitconfig containing [diff] external = ./pwn.sh plus an executable ./pwn.sh created the sentinel with rc=0: HOME is already exported, so the bare assignment updates the child environment, git reads the repo-planted file as its global config, and executes the repo-planted diff.external. That is precisely the payload the diff's own pinned test stops one spelling of — { HOME=.; } && git diff and HOME=.; git diff both classify unknown today, while ! HOME=.; git diff does not. The same gap reaches binary resolution: ! PATH=.:$PATH; git status classifies 'read-only', and run from a repo root containing an executable ./git, bash executed the planted ./git while the config gate still probed the real cwd and reported no risk.
Scope, stated plainly rather than overstated: only names already in the exported environment are a live channel. ! GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=/tmp/evil && git diff classifies unknown (three assignments, so the plural node's namedChildCount is 3), and a single non-exported name reaches nothing. So HOME, PATH, GIT_EXEC_PATH, LESSOPEN, LD_PRELOAD and NODE_OPTIONS are the channel — and HOME/PATH alone are sufficient, as the two sentinels show. Note also that the merge base allowed this shape too, so this is an incomplete new guard rather than a regression; but the added line is where the hazard is supposed to stop, and the commit that adds it is titled "reject persistent brace-group assignments".
Witness:
PR arm, this commit's own built dist, git 2.43.0 / bash 5.2.21
"! HOME=.; git diff" classify=read-only getDefaultPermission => allow
"HOME=.; git diff" classify=unknown => ask <- the spelling the guard stops
"{ HOME=.; } && git diff" classify=unknown => ask <- the diff's own pinned test
"! PATH=.:$PATH; git status" classify=read-only => allow
bash persistence oracle: `! FOO=bar; echo ${FOO-unset}` -> "bar" | `( FOO=bar ); ...` -> "unset"
REAL PAYLOAD 1: repo ships ./.gitconfig `[diff] external = ./pwn.sh` + executable ./pwn.sh
bash -c '! HOME=.; git diff' -> rc 0, PWN SENTINEL CREATED
REAL PAYLOAD 2: repo ships executable ./git
bash -c '! PATH=.:$PATH; git status' -> rc 0, PLANTED ./git EXECUTED
sibling sweep of 11 current-shell wrapper shapes: negated_command is the ONLY live hole
(subshell classifies read-only but does not persist, so the fix's stated cost is nil;
the other nine already classify unknown)
fix flip (node.parent?.type === 'program' && node.parent.namedChildCount === 1):
both payloads -> ask ; plain `git diff` -> allow (unchanged)
five suites: 1 failed | 976 passed -- the single failure is the pre-existing R7-2 case
BASE arm: "! HOME=.; git diff" = read-only (identical); "{ HOME=.; } && git diff" = read-only (PR: unknown)
Replace the single-type denylist with the actual criterion — an assignment is inert only when nothing can follow it in the same shell:
node.parent?.type === 'program' && node.parent.namedChildCount === 1 ? 'read-only' : 'unknown'
That keeps the pinned bare FOO=bar read-only and makes ( FOO=bar ) 'unknown', which costs nothing since it is measured harmless today; it also closes the if/for/while variants that a two-entry denylist would leave. If naming the root type is unwanted, the minimal variant is !['compound_statement', 'negated_command'].includes(node.parent.type).
git-shell-execution-safety.test.ts:157 pins expect(await classifyShellCommandSafety('FOO=bar')).toBe('read-only'), so a lone top-level assignment must stay read-only; and shellAstParser.ts:677 is const CHILD_STATEMENT = /^(?:pipeline|list|subshell|compound_statement|negated_command)$/; — negated_command is already a recursed-into current-shell wrapper, so the guard has to name it (or drop the denylist) instead of relying on the parent chain. Certified for bash 5.2.21 only; no zsh or ksh exists on this runner.
For a witness, extend "does not let brace-group assignments persist through read-only classification" (line 144) with expect(await classifyShellCommandSafety('! HOME=.; git diff')).toBe('unknown') and expect(await isShellCommandReadOnlyASTInDirectory('! PATH=.:$PATH; git status', cwd)).toBe(false). Both are red at this commit, go green only with the widened guard, and redden again if it is reverted, while the existing '{ PATH=.; } && ls' -> 'unknown' and 'FOO=bar' -> 'read-only' assertions stay green.
中文说明
新增的持久化赋值守卫只把一种父节点类型列入黑名单，因此同样的危险仍能通过 negated_command（!）包装被分类为 read-only —— 而仓库可以自带它所需的载荷。
这里新增的守卫收窄了一条既有规则，使 { ...; } 中的孤立赋值不再算只读，其陈述的理由是：花括号组在当前 shell 中执行，因此赋值会持续到后面的命令。CHILD_STATEMENT（shellAstParser.ts:677）已经把 negated_command 列为会被递归进入的当前 shell 包装，而 ! 具有完全相同的持久化语义 —— 但它不在黑名单里，于是 node.parent?.namedChildCount === 1 得到满足，赋值仍被判为只读。
classifyShellCommandSafety('! HOME=.; git diff') 返回 'read-only'，带目录的形式同样如此，因此 getDefaultPermission 返回 'allow'，没有任何对话框。在一个自带 ./.gitconfig（内含 [diff] external = ./pwn.sh）以及可执行 ./pwn.sh 的仓库里，用 bash -c 运行这个完全相同的字符串会以 rc=0 创建哨兵文件：HOME 本来就是导出的，所以裸赋值会更新子进程环境，git 把仓库放置的那个文件当作它的 global 配置读取，并执行仓库放置的 diff.external。这正是本改动自己的固定测试拦住了其中一种写法的那类载荷 —— 今天 { HOME=.; } && git diff 与 HOME=.; git diff 都分类为 unknown，而 ! HOME=.; git diff 不是。同样的缺口也触及二进制解析：! PATH=.:$PATH; git status 分类为 'read-only'，而在仓库根目录放一个可执行 ./git 后运行它，bash 执行了被植入的 ./git，同时配置门控仍然探测真实 cwd 并报告无风险。
范围说明，直白而不夸大：只有已经存在于导出环境中的名字才是有效通道。! GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=/tmp/evil && git diff 分类为 unknown（三个赋值，因此复数节点的 namedChildCount 是 3），单个未导出的名字什么也影响不到。所以通道是 HOME、PATH、GIT_EXEC_PATH、LESSOPEN、LD_PRELOAD、NODE_OPTIONS —— 而仅 HOME/PATH 就已足够，两个哨兵实验证明了这点。另需注意 merge base 也允许这种形态，所以这是一个不完整的新守卫，而不是回归；但新增的这一行正是该危险本应被拦住的地方，而加入它的提交标题是「reject persistent brace-group assignments」。
建议把单类型黑名单换成真正的判据 —— 只有在同一 shell 中不可能有后续内容时，赋值才是惰性的：
node.parent?.type === 'program' && node.parent.namedChildCount === 1 ? 'read-only' : 'unknown'
这既保留了被固定的裸 FOO=bar 只读判定，也让 ( FOO=bar ) 变成 'unknown' —— 后者代价为零，因为实测它今天是无害的；它同时关闭了两项黑名单会留下的 if/for/while 变体。如果不想点名根类型，最小变体是 !['compound_statement', 'negated_command'].includes(node.parent.type)。
git-shell-execution-safety.test.ts:157 固定了 expect(await classifyShellCommandSafety('FOO=bar')).toBe('read-only')，因此顶层孤立赋值必须保持只读；而 shellAstParser.ts:677 是 const CHILD_STATEMENT = /^(?:pipeline|list|subshell|compound_statement|negated_command)$/; —— negated_command 已经是会被递归进入的当前 shell 包装，所以守卫必须点名它（或干脆去掉黑名单），而不能依赖父节点链。仅在 bash 5.2.21 下验证；本 runner 上不存在 zsh 或 ksh。
修复验证：在 "does not let brace-group assignments persist through read-only classification"（第 144 行）中扩展 expect(await classifyShellCommandSafety('! HOME=.; git diff')).toBe('unknown') 与 expect(await isShellCommandReadOnlyASTInDirectory('! PATH=.:$PATH; git status', cwd)).toBe(false)。两者在本提交上是红的，只有加宽守卫后才变绿，回退后再次变红，同时现有的 '{ PATH=.; } && ls' -> 'unknown' 与 'FOO=bar' -> 'read-only' 断言保持绿色。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| return childrenSafety(node, 'unknown'); | ||
| } | ||
|
|
||
| const FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS = new Set([ |
There was a problem hiding this comment.
[Critical] R8-14: [certifies-falsely] [new-surface] The fsmonitor/filter exemption is keyed on the subcommand, but fsmonitor consumption is keyed on the argument form — so four of the six "proven non-consumers" do consume it, and git show :<path> runs the repository's helper under auto-approval.
cat-file, log, rev-parse and show are exempted here on the strength of being non-consumers of command-form core.fsmonitor. That holds for commit-form revisions and fails for index-path revisions: resolving :path reads the index, and reading the index runs the fsmonitor hook. So in a repo whose only local risk is a command-form core.fsmonitor, git show :a.dat, git cat-file -p :a.dat, git rev-parse :a.dat, git log -1 :a.dat and echo ':a.dat' | git cat-file --batch-check each execute the configured helper while the classifier returns read-only, getDefaultPermission returns allow, and plan-mode routing plus speculative execution treat the command as read-only.
git show :<path> — read the staged version of a file — is an everyday agent command after a partial git add, so no exotic spelling is needed to reach this. The cat-file --batch* variant cannot be decided from arguments at all, because the revisions arrive on stdin.
Witness:
scratch repo, git init, local core.fsmonitor = <sentinel script>, ambient config pinned empty
getLocalGitConfigRisk(cwd) = {fsmonitor: true, ...all other ten bits false}
-> the broad pager/promisor/alternateRefs/hooksPath gate does not fire;
the per-subcommand matrix at :1183-1186 decides
git show :a.dat sentinelCreated TRUE classifierReadOnly TRUE
git cat-file -p :a.dat sentinelCreated TRUE classifierReadOnly TRUE
git rev-parse :a.dat sentinelCreated TRUE classifierReadOnly TRUE
git log -1 :a.dat sentinelCreated TRUE classifierReadOnly TRUE
echo ':a.dat' | git cat-file --batch-check sentinelCreated TRUE classifierReadOnly TRUE
git log -1 -- :a.dat (pathspec form) sentinelCreated false <- control
git log -1 / git show HEAD / git rev-parse HEAD / git branch --show-current
sentinelCreated false <- controls
git status sentinelCreated TRUE classifierReadOnly FALSE
<- the gate works exactly where the subcommand is not exempt
BASE arm, same fixture: all four `:path` forms readOnly true (base was equally permissive;
the failing branch is code this PR adds)
Decide the fsmonitor branch per invocation instead of per subcommand. gitReads already has the stripped args in hand a few lines below, so record { subcommand, args } and deny when risk.fsmonitor and either any argument starts with : (index-path / magic revision) or cat-file was given --batch/--batch-check. The coarser alternative — dropping cat-file, log, rev-parse and show from the exemption, leaving branch and remote, which measured clean — must then split the shared set, because the worktreeFilter half is not implicated by these measurements.
Two constraints: git-config-safety.test.ts:120-128 pins for (const command of ['git log -1', 'git show HEAD', 'git rev-parse HEAD']) ... toBe(true) under a command-form core.fsmonitor, so a blanket removal of those subcommands breaks that pin; and shellAstParser.ts:1183-1186 shares this one set between two bits — if ((risk.fsmonitor || risk.worktreeFilter) && !FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS.has(subcommand)) — so narrowing it for fsmonitor must not weaken the worktreeFilter branch, and must be coordinated with the cat-file edit requested in the textconv/smudge finding.
For a witness, extend "gates command-form fsmonitor with a default-deny consumer model" (line 101): add git show :file.txt, git cat-file -p :file.txt, git rev-parse :file.txt, git log -1 :file.txt and echo ':file.txt' | git cat-file --batch-check to the denied loop, keeping git log -1 / git show HEAD / git rev-parse HEAD in the allowed loop. Removing the argument-shape check must turn exactly those five red.
中文说明
fsmonitor/filter 豁免是按子命令建立的，但 fsmonitor 的消费是按参数形式发生的 —— 因此六个「已证明的非消费者」中有四个确实会消费它，而 git show :<path> 会在自动批准下运行仓库配置的助手程序。
这里的 cat-file、log、rev-parse、show 是基于「不消费命令形式 core.fsmonitor」而被豁免的。这对提交形式的 revision 成立，但对索引路径形式的 revision 不成立：解析 :path 会读取索引，而读取索引会运行 fsmonitor hook。所以在唯一本地风险是命令形式 core.fsmonitor 的仓库里，git show :a.dat、git cat-file -p :a.dat、git rev-parse :a.dat、git log -1 :a.dat 以及 echo ':a.dat' | git cat-file --batch-check 都会执行配置的助手程序，而分类器返回 read-only、getDefaultPermission 返回 allow，plan-mode 路由与推测执行也把该命令当作只读。
git show :<path> —— 读取文件的暂存版本 —— 是 agent 在部分 git add 之后的日常命令，因此到达这里不需要任何奇特写法。cat-file --batch* 变体则根本无法从参数判定，因为 revision 是从 stdin 进来的。
建议按每次调用而不是按子命令来判定 fsmonitor 分支。gitReads 在下面几行就已经拿到了剥离引号后的 args，因此记录 { subcommand, args }，并在 risk.fsmonitor 且（任一参数以 : 开头，即索引路径/魔法 revision，或 cat-file 带了 --batch/--batch-check）时拒绝。较粗的替代方案 —— 从豁免中去掉 cat-file、log、rev-parse、show，保留实测干净的 branch 与 remote —— 则必须拆分这个共享集合，因为这些测量并不涉及 worktreeFilter 那一半。
两个约束：git-config-safety.test.ts:120-128 在命令形式 core.fsmonitor 下固定了 for (const command of ['git log -1', 'git show HEAD', 'git rev-parse HEAD']) ... toBe(true)，因此整体移除这些子命令会破坏该固定；而 shellAstParser.ts:1183-1186 把这一个集合在两个风险位之间共享 —— if ((risk.fsmonitor || risk.worktreeFilter) && !FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS.has(subcommand)) —— 所以为 fsmonitor 收窄它不得削弱 worktreeFilter 分支，并且必须与 textconv/smudge 那条发现所要求的 cat-file 改动协调。
修复验证：扩展 "gates command-form fsmonitor with a default-deny consumer model"（第 101 行）：把 git show :file.txt、git cat-file -p :file.txt、git rev-parse :file.txt、git log -1 :file.txt 和 echo ':file.txt' | git cat-file --batch-check 加入拒绝循环，同时把 git log -1 / git show HEAD / git rev-parse HEAD 保留在允许循环中。移除参数形状检查后必须恰好让这五条变红。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
| return true; | ||
| if ( | ||
| risk.diffDriverTextconv && | ||
| ['blame', 'diff', 'log', 'show'].includes(subcommand) |
There was a problem hiding this comment.
[Critical] R8-24: [certifies-falsely] [new-surface] git cat-file and git grep have no config-side textconv/smudge coverage at all, and git's unique-prefix abbreviation of the option that reaches them defeats the only guard left.
Three gaps combine into one defect here. cat-file and grep are both absent from this diffDriverTextconv denial list; cat-file is additionally exempted from fsmonitor/worktreeFilter by FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS above; and the probe never enumerates filter.*.smudge at all — LOCAL_GIT_CONFIG_RISK_KEY_PATTERN carries only ^filter\..*\.clean$ and ^filter\..*\.process$. Yet cat-file consumes both drivers through its documented --textconv --path= and --filters --path= options, and grep consumes textconv through --textconv. So for these two subcommands the only remaining guard is the option-shape regex, which git's unique-prefix abbreviation defeats — and which the normalization fix requested in the option-spelling finding explicitly does not make abbreviation-tolerant, since git's abbreviation rules are per-subcommand and unstable.
In a repo with .gitattributes *.dat filter=x diff=x, local diff.x.textconv and filter.x.smudge, the commands git cat-file --textc|--textco|--text --path=f.dat <blob> run the textconv helper and git cat-file --filt|--fil --path=f.dat <blob> runs the smudge helper — all rc=0, all classified read-only, so getDefaultPermission returns allow with no dialog. The smudge half is structurally unreachable by the probe: with filter.x.smudge as the only local key, git config --get-regexp '^filter\..*\.(clean|process)$' returns rc 1 and getLocalGitConfigRisk reports all eleven bits false, so removing cat-file from the exemption set alone would not help. For grep, in a repo whose only local key is diff.tc.textconv, git grep --textc needle and git grep --textco needle run the helper while classified read-only, the exact spelling --textconv is denied, and git log -p -1 in the same repo is denied — which proves the matrix denies four subcommands for this exact bit and only these two fall through.
Witness:
git 2.43.0, this commit's own built dist; repo: .gitattributes `*.dat filter=x diff=x`,
local diff.x.textconv + filter.x.smudge + filter.x.clean, each a sentinel script
risk = { diffDriverTextconv: TRUE, worktreeFilter: TRUE, other nine bits false }
command gitRc textconv smudge astInDir fallback
git cat-file --textconv --path=f.dat <blob> 0 TRUE TRUE false false
git cat-file --textco --path=f.dat <blob> 0 TRUE TRUE TRUE true
git cat-file --textc --path=f.dat <blob> 0 TRUE TRUE TRUE true
git cat-file --text --path=f.dat <blob> 0 TRUE TRUE TRUE true
git cat-file --filters --path=f.dat <blob> 0 false TRUE false false
git cat-file --filt --path=f.dat <blob> 0 false TRUE TRUE true
git cat-file -p <blob> (control) 0 false false TRUE true
controls git status/diff/log -1/show HEAD/ls-files/blame f.dat/describe -> all denied
SMUDGE-ONLY arm (filter.x.smudge is the sole local key):
git config --get-regexp '^filter\..*\.(clean|process)$' -> rc=1, no match
getLocalGitConfigRisk -> ALL ELEVEN BITS FALSE (worktreeFilter: false)
git cat-file --filt --path=f.dat <blob> -> smudgeRan TRUE, astInDir TRUE
GREP arm (diff.tc.textconv is the sole local key; risk correctly {diffDriverTextconv:true}):
git grep --textc needle / --textco needle -> astInDir TRUE, SENTINEL CREATED (rc=0)
git grep --textconv needle -> false ; git grep needle -> true, no sentinel
real ShellTool: getDefaultPermission("git grep --textc needle") = allow
("git grep --textconv needle") = ask ("git log -p -1") = ask
fix flip (add 'grep'): all three grep forms -> false; five suites green except R7-2
BASE arm returned identical rows (base risk record has 2 keys), so the hazard pre-exists;
what is new is that the defect now lives in lines this diff adds
Three coupled edits, one defect: add 'cat-file' and 'grep' to this denial list, so it reads ['blame', 'cat-file', 'diff', 'grep', 'log', 'show']; add String.raw^filter..*.smudge$`` to LOCAL_GIT_CONFIG_RISK_KEY_PATTERN and fold it into the `worktreeFilter` bit; and drop `'cat-file'` from `FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS` — or split that set into an fsmonitor half and a filter half — matching the over-blocking posture this branch already accepts. Note the measured cost of adding `'grep'`: it also denies unflagged `git grep needle` in any repo with a textconv driver, which is fail-closed over-denial of the same kind, and no existing assertion pins the other way.
Constraints the edits must respect: git-config-safety.test.ts:281-287 pins 'git log -1', 'git show HEAD' and 'git rev-parse HEAD' -> true under filter.demo.clean/filter.demo.process, so widening worktreeFilter must keep those three exempt; shellAstParser.ts:1183-1186 shares one set between the fsmonitor and worktreeFilter bits, so narrowing it for cat-file must not drop the coverage the fsmonitor finding asks to add for cat-file --batch*/:path; and git-config-safety.test.ts:344 pins 'git cat-file -p HEAD:file' denied under promisorRemote, which the broad gate supplies independently of this set. No allow-side pin for git grep exists under any risk bit in any safety suite, so adding 'grep' breaks nothing.
For a witness, in "gates clean and process filters while preserving proven non-consumers" (:258) add filter.demo.smudge to the key loop and git cat-file --filt --path=file.txt HEAD:file.txt to the denied loop; beside the diff.audit.textconv case (:429-432) add expect(await isShellCommandReadOnlyASTInDirectory('git cat-file --textc --path=file.txt HEAD:file.txt', cwd)).toBe(false) and a 'git grep --textc file.txt' case asserting false. The abbreviations are what make these bite — both are measured read-only today, so a witness written with the exact spellings would pass without any of the three edits.
中文说明
git cat-file 与 git grep 在配置侧完全没有 textconv/smudge 覆盖，而 git 对触达它们的选项做唯一前缀缩写，恰好击败了仅剩的那道守卫。
这里三个缺口合成一个缺陷。cat-file 与 grep 都不在这份 diffDriverTextconv 拒绝列表中；cat-file 还额外被上面的 FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS 从 fsmonitor/worktreeFilter 中豁免；而探测根本没有枚举 filter.*.smudge —— LOCAL_GIT_CONFIG_RISK_KEY_PATTERN 只带 ^filter\..*\.clean$ 与 ^filter\..*\.process$。但 cat-file 会通过其文档化的 --textconv --path= 与 --filters --path= 选项消费这两种 driver，grep 会通过 --textconv 消费 textconv。所以对这两个子命令而言，唯一剩下的守卫就是选项形状正则，而 git 的唯一前缀缩写能击败它 —— 并且选项写法那条发现所要求的归一化修复明确不会让正则容忍缩写，因为 git 的缩写规则是按子命令且不稳定的。
在带 .gitattributes *.dat filter=x diff=x、本地 diff.x.textconv 与 filter.x.smudge 的仓库里，git cat-file --textc|--textco|--text --path=f.dat <blob> 会运行 textconv 助手，git cat-file --filt|--fil --path=f.dat <blob> 会运行 smudge 助手 —— 全部 rc=0，全部被分类为 read-only，于是 getDefaultPermission 返回 allow，没有任何对话框。smudge 那一半在结构上探测不到：当 filter.x.smudge 是唯一的本地键时，git config --get-regexp '^filter\..*\.(clean|process)$' 返回 rc 1，getLocalGitConfigRisk 报告全部十一个位为 false，因此仅仅把 cat-file 从豁免集合中移除并无帮助。对 grep 而言，在唯一本地键是 diff.tc.textconv 的仓库里，git grep --textc needle 与 git grep --textco needle 会运行助手却被分类为 read-only，精确写法 --textconv 被拒绝，而同一仓库中的 git log -p -1 也被拒绝 —— 这证明该矩阵对这一个风险位拒绝了四个子命令，只有这两个漏了过去。
三处相互耦合的修改，同一个缺陷：把 'cat-file' 与 'grep' 加入这份拒绝列表，使其成为 ['blame', 'cat-file', 'diff', 'grep', 'log', 'show']；把 String.raw^filter..*.smudge$`` 加入 LOCAL_GIT_CONFIG_RISK_KEY_PATTERN 并折进 `worktreeFilter` 位；以及把 `'cat-file'` 从 `FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS` 中去掉 —— 或把该集合拆成 fsmonitor 半与 filter 半 —— 与该分支已经接受的过度拦截姿态保持一致。注意加入 `'grep'` 的实测代价：它也会在任何配置了 textconv driver 的仓库中拒绝不带 flag 的 `git grep needle`，这属于同一种 fail-closed 的过度拒绝，且没有任何现有断言固定相反方向。
修改需遵守的约束：git-config-safety.test.ts:281-287 在 filter.demo.clean/filter.demo.process 下固定了 'git log -1'、'git show HEAD'、'git rev-parse HEAD' -> true，所以加宽 worktreeFilter 必须保留这三者的豁免；shellAstParser.ts:1183-1186 在 fsmonitor 与 worktreeFilter 两个位之间共享同一个集合，所以为 cat-file 收窄它不得丢掉 fsmonitor 那条发现要求为 cat-file --batch*/:path 增加的覆盖；git-config-safety.test.ts:344 固定在 promisorRemote 下拒绝 'git cat-file -p HEAD:file'，而这由宽门控独立提供，与该集合无关。任何安全测试套件中都不存在任何风险位下针对 git grep 的允许侧固定，因此加入 'grep' 不会破坏任何东西。
修复验证：在 "gates clean and process filters while preserving proven non-consumers"（:258）中把 filter.demo.smudge 加入键循环、把 git cat-file --filt --path=file.txt HEAD:file.txt 加入拒绝循环；在 diff.audit.textconv 用例（:429-432）旁边加入 expect(await isShellCommandReadOnlyASTInDirectory('git cat-file --textc --path=file.txt HEAD:file.txt', cwd)).toBe(false) 以及一条断言 false 的 'git grep --textc file.txt' 用例。正是缩写让这些用例有意义 —— 两者今天实测都是 read-only，所以用精确写法写的验证即使三处修改一处不做也会通过。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
7 candidate finding(s) this round's reviewers re-derived matched entries already carried on this PR and were set aside before verification (R8-5, R8-8, R8-2 ×2, R8-1, R8-6, R8-3) — a matched posted finding is ruled in the previous-round status as always, and a matched deferral stays on the standing deferral record.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": whether matchesRule matches a Bash(git diff) specifier against the raw env-prefixed command string or only against unwrapped virtual ops ( permission-manage….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/core/src/memory/memory-scoped-agent-config.ts:31 — [probe] A second byte-identical copy of the security-gate regexpackages/core/src/utils/git-config-safety.test.ts:36 (+2 locations) — [probe] Ambient host git config decides whether the new suites…packages/core/src/utils/git-config-safety.test.ts:119 — [probe] Three of the six shared proven-non-consumer entries are…packages/core/src/utils/git-config-safety.test.ts:465 — [probe] The parser-unavailable suite pins only the deny directionpackages/core/src/utils/git-config-safety.ts:145 — [probe] The executable-bit guard is never exercised in the false…packages/core/src/utils/git-config-safety.ts:291 — [probe] The three fail-closed early returns are the only outcomes…packages/core/src/utils/shellAstParser.ts:839 — [probe] '--no-query' is not an option git's remote show acceptspackages/core/src/utils/shellAstParser.ts:1158 — [probe] Every read-only git classification now pays two blocking…
[Critical] R9-4 [certifies-falsely] [new-surface] The new helper-option gate approximates git's option parser with two textual shapes and misses both of the ways git actually accepts these options: parse-options resolves any unambiguous ABBREVIATION of a long option, and it consumes the NEXT argv element as an option's value when no = is present. Both omissions let a repository-controlled helper run under an auto-approved command. This is a different root cause from the raw-text/quoting defect reported separately - normalizing quotes and escapes would close neither. Two measured entrances, each auto-approved with no confirmation dialog while the repository-controlled program really executed. (1) Abbreviation: in a repo with .gitattributes f.txt diff=pwn and [diff "pwn"] textconv=, git grep --textc line1 (also --textco, --textcon) classifies read-only and runs the textconv helper; in a repo with [filter "cl"] smudge=, git cat-file --filte :f.txt (also --filt, --fi) classifies read-only and runs the smudge helper. (2) Space-separated value: in a repo with a committed .gitattributes merge driver assignment and merge..driver defined, git log --diff-merges remerge -1, git log --diff-merges r -1 and git show --diff-merges remerge HEAD all re-perform the merge and execute the driver, byte-identically to the = spelling the guard does catch. The driver may live in the GLOBAL or SYSTEM gitconfig, which the local-only risk.mergeDriver bit never sees - and the space form is also open through the cwd-less classifier arms. Witness: Oracle, git 2.43.0 with marker helpers: git grep --textconv line1 rc=0 helper_ran=YES | --textcon YES | --textc YES | --te rc=129 helper_ran=NO (git: 'ambiguous option: te (could be --text or --textconv)') git cat-file --filters :f.txt rc=0 helper_ran=YES (EXEC[SMUDGE]) | --filte/--filt/--fi YES | control git cat-file -p :f.txt NO git log --diff-merges bogus -1 -> fatal: invalid value for '--diff-merges': 'bogus' (git consumes the NEXT token as the value) git log --diff-merges remerge -1 -> MERGEDRIVER-FIRED | --diff-merges r -1 -> MERGEDRIVER-FIRED | --diff-merges=remerge -1 -> MERGEDRIVER-FIRED cmp space-vs-equals output -> IDENTICAL, 207 B each | git log --cc -1 -> no marker Classifier (this commit): {"command":"git grep --textc line1","ast":"read-only","classify":"read-only","fallbackRO":true} (exact --textconv -> unknown/false) {"command":"git cat-file --filte :f.txt","ast":"read-only","smudgeRepoAutoApproved":true} (exact --filters -> unknown/false) {"command":"git log --diff-merges remerge -1","ast":"read-only","regexFallbackRO":true,"inCleanRepoAutoApproved":true} vs {"command":"git log --diff-merges=remerge -1","ast":"unknown","regexFallbackRO":false,"inCleanRepoAutoApproved":false} {"scope":"GLOBAL merge driver + repo .gitattributes","command":"git log --diff-merges remerge -1","autoApproved":true} {"arm":"parserInitFailed","command":"git log --diff-merges remerge -1","autoApprovedCleanRepo":true} A/B: BASE 60ccd1a auto-approved every spelling including --remerge-diff, so the guard is this diff's and the hole is in it. Tested fix (not a hypothesis): |^--diff-merges(?:=(?:remerge|r)|$) in BOTH copies plus prefix-of-name matching (62 prefix alternatives, (?:=|$) anchor kept, --diff-merges= untouched) flipped all abbreviated and space forms to unknown/false, left the exact spellings and git log --cc -1 unchanged, and kept 251 tests green across git-config-safety.test.ts, git-shell-execution-safety.test.ts and shellReadOnlyChecker.test.ts. Suggested fix: Two changes, both mirrored in shellReadOnlyChecker.ts:89. (1) Make the helper-option rule abbreviation-aware: match any argument whose --name is an unambiguous prefix of ext-diff|filters|show-signature|textconv|open-files-in-pager|remerge-diff. For log/show/diff git rejects these abbreviations, so the only cost is denying a command git would have refused anyway. (2) Deny the bare --diff-merges token as well as the = form - |^--diff-merges(?:=(?:remerge|r)|$) - since a per-token .test(arg) cannot see the following argument; alternatively add a paired check over beforeTerminator(rest) in evaluateGitSafety and over options in evaluateGitCommand. Denying the space form of harmless values (--diff-merges off) is the fail-closed direction and matches how --ext-diff and --filters are already treated. Fix constraint: packages/core/src/utils/shellReadOnlyChecker.ts:89 holds a second, byte-different copy of the rule - /(?:^--(?:ext-diff|filters|show-signature|textconv|open-files-in-pager|remerge-diff)(?:=|$)|^--diff-merges=(?:remerge|r)$|%G)/ - and the parser-failure path at shellAstParser.ts:1288-1300 gates on isShellCommandReadOnly(command), so fixing only the AST copy leaves the fallback permissive for exactly the spellings the guard was added for; both consumers test one token at a time (shellAstParser.ts:800, shellReadOnlyChecker.ts:246-250). git exact-matches --diff-merges VALUES (remerge/r accepted; R/re/rem/remerge-x rejected rc=128), so ^--diff-merges=(?:remerge|r)$ must not be loosened to a prefix. git-shell-execution-safety.test.ts 'keeps AST and fallback Git helper rules in lockstep' compares both arms on the same strings, so the abbreviation rule must land in both. Fix witness: packages/core/src/utils/git-shell-execution-safety.test.ts: a new case asserting classifyShellCommandSafety is 'unknown' for git log --diff-merges remerge -1, git log --diff-merges r -1, git grep --textc needle and git cat-file --filte :f.txt, that isShellCommandReadOnly('git show --diff-merges remerge HEAD') is false, and - with merge.pwn.driver configured in a createRepo() - that isShellCommandReadOnlyASTInDirectory('git show --diff-merges remerge HEAD', cwd) is false; plus git-config-safety.test.ts cases for the textconv and smudge abbreviations. All are the opposite today (measured). git log -p --diff-merges=off and --cc must stay read-only so the fix is not proven by blanket denial, and removing either half must red its own cases.
中文说明
仅完成部分审查，审查缺口已披露。
本轮评审重新推导出的 7 条候选发现与本 PR 已携带的条目匹配，已在验证前搁置（R8-5, R8-8, R8-2 ×2, R8-1, R8-6, R8-3）——被匹配的已发布条目照常在上一轮状态区裁定，被匹配的延后条目仍保留在延后清单记录中。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：whether matchesRule matches a Bash(git diff) specifier against the raw env-prefixed command string or only against unwrapped virtual ops ( permission-manage…。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
[Critical] R9-4 [certifies-falsely] [new-surface] The new helper-option gate approximates git's option parser with two textual shapes and misses both of the ways git actually accepts these options: parse-options resolves any unambiguous ABBREVIATION of a long option, and it consumes the NEXT argv element as an option's value when no = is present. Both omissions let a repository-controlled helper run under an auto-approved command. This is a different root cause from the raw-text/quoting defect reported separately - normalizing quotes and escapes would close neither. Two measured entrances, each auto-approved with no confirmation dialog while the repository-controlled program really executed. (1) Abbreviation: in a repo with .gitattributes f.txt diff=pwn and [diff "pwn"] textconv=, git grep --textc line1 (also --textco, --textcon) classifies read-only and runs the textconv helper; in a repo with [filter "cl"] smudge=, git cat-file --filte :f.txt (also --filt, --fi) classifies read-only and runs the smudge helper. (2) Space-separated value: in a repo with a committed .gitattributes merge driver assignment and merge..driver defined, git log --diff-merges remerge -1, git log --diff-merges r -1 and git show --diff-merges remerge HEAD all re-perform the merge and execute the driver, byte-identically to the = spelling the guard does catch. The driver may live in the GLOBAL or SYSTEM gitconfig, which the local-only risk.mergeDriver bit never sees - and the space form is also open through the cwd-less classifier arms. Witness: Oracle, git 2.43.0 with marker helpers: git grep --textconv line1 rc=0 helper_ran=YES | --textcon YES | --textc YES | --te rc=129 helper_ran=NO (git: 'ambiguous option: te (could be --text or --textconv)') git cat-file --filters :f.txt rc=0 helper_ran=YES (EXEC[SMUDGE]) | --filte/--filt/--fi YES | control git cat-file -p :f.txt NO git log --diff-merges bogus -1 -> fatal: invalid value for '--diff-merges': 'bogus' (git consumes the NEXT token as the value) git log --diff-merges remerge -1 -> MERGEDRIVER-FIRED | --diff-merges r -1 -> MERGEDRIVER-FIRED | --diff-merges=remerge -1 -> MERGEDRIVER-FIRED cmp space-vs-equals output -> IDENTICAL, 207 B each | git log --cc -1 -> no marker Classifier (this commit): {"command":"git grep --textc line1","ast":"read-only","classify":"read-only","fallbackRO":true} (exact --textconv -> unknown/false) {"command":"git cat-file --filte :f.txt","ast":"read-only","smudgeRepoAutoApproved":true} (exact --filters -> unknown/false) {"command":"git log --diff-merges remerge -1","ast":"read-only","regexFallbackRO":true,"inCleanRepoAutoApproved":true} vs {"command":"git log --diff-merges=remerge -1","ast":"unknown","regexFallbackRO":false,"inCleanRepoAutoApproved":false} {"scope":"GLOBAL merge driver + repo .gitattributes","command":"git log --diff-merges remerge -1","autoApproved":true} {"arm":"parserInitFailed","command":"git log --diff-merges remerge -1","autoApprovedCleanRepo":true} A/B: BASE 60ccd1a auto-approved every spelling including --remerge-diff, so the guard is this diff's and the hole is in it. Tested fix (not a hypothesis): |^--diff-merges(?:=(?:remerge|r)|$) in BOTH copies plus prefix-of-name matching (62 prefix alternatives, (?:=|$) anchor kept, --diff-merges= untouched) flipped all abbreviated and space forms to unknown/false, left the exact spellings and git log --cc -1 unchanged, and kept 251 tests green across git-config-safety.test.ts, git-shell-execution-safety.test.ts and shellReadOnlyChecker.test.ts. Suggested fix: Two changes, both mirrored in shellReadOnlyChecker.ts:89. (1) Make the helper-option rule abbreviation-aware: match any argument whose --name is an unambiguous prefix of ext-diff|filters|show-signature|textconv|open-files-in-pager|remerge-diff. For log/show/diff git rejects these abbreviations, so the only cost is denying a command git would have refused anyway. (2) Deny the bare --diff-merges token as well as the = form - |^--diff-merges(?:=(?:remerge|r)|$) - since a per-token .test(arg) cannot see the following argument; alternatively add a paired check over beforeTerminator(rest) in evaluateGitSafety and over options in evaluateGitCommand. Denying the space form of harmless values (--diff-merges off) is the fail-closed direction and matches how --ext-diff and --filters are already treated. Fix constraint: packages/core/src/utils/shellReadOnlyChecker.ts:89 holds a second, byte-different copy of the rule - /(?:^--(?:ext-diff|filters|show-signature|textconv|open-files-in-pager|remerge-diff)(?:=|$)|^--diff-merges=(?:remerge|r)$|%G)/ - and the parser-failure path at shellAstParser.ts:1288-1300 gates on isShellCommandReadOnly(command), so fixing only the AST copy leaves the fallback permissive for exactly the spellings the guard was added for; both consumers test one token at a time (shellAstParser.ts:800, shellReadOnlyChecker.ts:246-250). git exact-matches --diff-merges VALUES (remerge/r accepted; R/re/rem/remerge-x rejected rc=128), so ^--diff-merges=(?:remerge|r)$ must not be loosened to a prefix. git-shell-execution-safety.test.ts 'keeps AST and fallback Git helper rules in lockstep' compares both arms on the same strings, so the abbreviation rule must land in both. Fix witness: packages/core/src/utils/git-shell-execution-safety.test.ts: a new case asserting classifyShellCommandSafety is 'unknown' for git log --diff-merges remerge -1, git log --diff-merges r -1, git grep --textc needle and git cat-file --filte :f.txt, that isShellCommandReadOnly('git show --diff-merges remerge HEAD') is false, and - with merge.pwn.driver configured in a createRepo() - that isShellCommandReadOnlyASTInDirectory('git show --diff-merges remerge HEAD', cwd) is false; plus git-config-safety.test.ts cases for the textconv and smudge abbreviations. All are the opposite today (measured). git log -p --diff-merges=off and --cc must stay read-only so the fix is not proven by blanket denial, and removing either half must red its own cases.
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const LOCAL_GIT_CONFIG_RISK_KEY_PATTERN = [ | ||
| String.raw`^diff\.external$`, |
There was a problem hiding this comment.
[Critical] R8-1: [certifies-falsely] The risk-key probe is an allowlist over git's whole config namespace, so one unlisted key hands execution to a repository-controlled driver. This round produced a fresh demonstrated entrance: filter.<driver>.smudge — the third command-valued filter-driver key git's convert.c reads — is absent from both LOCAL_GIT_CONFIG_RISK_KEY_PATTERN and the worktreeFilter bit, so a repository whose only dangerous config is a smudge driver is reported as carrying no filter risk at all.
A repo with .gitattributes *.txt filter=d and .git/config [filter "d"] smudge = <cmd> (no clean, no process) makes git cat-file --filters HEAD:a.txt run <cmd> as the user, while every risk bit reads false — so Object.values(getLocalGitConfigRisk(cwd)).some(Boolean) in fallbackGitConfigMakesCommandUnsafe returns false and, on the parser-init-failure path, any git command the regex checker accepts is auto-approved. Because the enumeration is an allowlist, the next unlisted program-valued key reproduces this exactly; closing it key by key is what has been happening round after round.
Witness:
risk(smudge-only repo) = {all 11 bits false, incl. "worktreeFilter":false}
risk(clean-only repo).worktreeFilter = true
git 2.43.0: git cat-file --filters HEAD:a.txt -> SMUDGE-FIRED
git cat-file --filte/--filt/--fi HEAD:a.txt -> SMUDGE-FIRED
git show HEAD:a.txt -> (no marker)
PR classifier: {"command":"git cat-file --filte HEAD:a.txt","ast":"read-only","smudgeRepoAutoApproved":true}
differential: {"arm":"parserInitFailed","command":"git cat-file --filte HEAD:a.txt",
"smudgeRepoAutoApproved":true,"cleanRepoAutoApproved":false}
candidate fix measured: adding FILTER_SMUDGE_KEY_PATTERN -> worktreeFilter=true and the
parserInitFailed arm flips to false, but the AST arm stays true (cat-file is in
FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS); 21 tests in the two new suites stay green.
Close the class structurally rather than entrance by entrance: treat any repository-local (local/worktree scope) config value that names an executable as a risk, or invert to a default-deny model where a git command is auto-approved only when the resolved local config contains no key in git's own set of program-valued keys. As the immediate step for the demonstrated entrance, add the smudge pattern beside the existing two filter patterns and OR it into worktreeFilter, then audit git's documented program-valued keys against the whole list rather than waiting for the next round to find one:
const FILTER_SMUDGE_KEY_PATTERN = String.raw`^filter\..*\.smudge$`;
// include it in LOCAL_GIT_CONFIG_RISK_KEY_PATTERN, then:
worktreeFilter:
hasLocalValueMatching(FILTER_CLEAN_KEY) ||
hasLocalValueMatching(FILTER_SMUDGE_KEY) ||
hasLocalValueMatching(FILTER_PROCESS_KEY),Note the premise this fix must respect: adding the key closes the risk-model and fallback halves but not the AST arm, because cat-file is a member of FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS at shellAstParser.ts:1125, consulted at :1184-1187 — so a green worktreeFilter assertion is not evidence that git cat-file --filte is blocked. The probe must also keep using native git config semantics for includes and precedence (today git config --includes --show-scope --null --get-regexp at :168-175), and only local/worktree scope counts as repository-controlled.
Please add 'filter.demo.smudge' to the for (const key of ['filter.demo.clean', 'filter.demo.process']) loop at git-config-safety.test.ts:258, plus a case asserting isShellCommandReadOnlyASTInDirectory('git cat-file --filters :f.txt', cwd) === false, and confirm the mutation: remove the new OR term and watch both go red.
中文说明
[Critical] R8-1：[certifies-falsely] 风险键探测本质上是一份覆盖 git 整个配置命名空间的白名单，因此任何一个未被列入的键都会把执行权交给仓库可控的驱动程序。本轮找到了一个新的实证入口：filter.<driver>.smudge（git convert.c 读取的第三个命令型 filter driver 键）既不在 LOCAL_GIT_CONFIG_RISK_KEY_PATTERN 中，也不在 worktreeFilter 位中，所以一个唯一危险配置就是 smudge 驱动的仓库会被报告为完全不存在 filter 风险。
当仓库带有 .gitattributes *.txt filter=d 与 .git/config [filter "d"] smudge = <cmd>（没有 clean、没有 process）时，git cat-file --filters HEAD:a.txt 会以当前用户身份执行 <cmd>，而所有风险位都是 false —— 于是 fallbackGitConfigMakesCommandUnsafe 中的 Object.values(getLocalGitConfigRisk(cwd)).some(Boolean) 返回 false，并且在 parser 初始化失败路径上，任何被正则检查器接受的 git 命令都会被自动批准。由于这是白名单式枚举，下一个未列入的命令型键会精确复现同一问题；逐个键地封堵正是过去每一轮反复发生的事。
结构性地关闭这一类问题，而不是逐个入口封堵：把任何仓库本地（local/worktree 作用域）中取值指向可执行文件的配置都视为风险，或者反转为默认拒绝模型 —— 只有当解析出的本地配置不包含 git 自身任何命令型键时，才自动批准该 git 命令。针对已实证入口的即时步骤是：在现有两个 filter 模式旁加上 smudge 模式并 OR 进 worktreeFilter，然后拿 git 文档中的命令型键去审计整份列表，而不是等下一轮再发现一个。
请注意该修复必须遵守的前提：加键只关闭了风险模型与 fallback 两半，并没有关闭 AST 分支，因为 cat-file 是 shellAstParser.ts:1125 中 FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS 的成员（在 :1184-1187 被查询）—— 所以 worktreeFilter 断言变绿并不能证明 git cat-file --filte 已被拦截。探测还必须继续使用 git 原生的配置语义来处理 include 与优先级（当前是 :168-175 的 git config --includes --show-scope --null --get-regexp），并且只有 local/worktree 作用域才算仓库可控。
请把 'filter.demo.smudge' 加入 git-config-safety.test.ts:258 的 for (const key of ['filter.demo.clean', 'filter.demo.process']) 循环，并补一个断言 isShellCommandReadOnlyASTInDirectory('git cat-file --filters :f.txt', cwd) === false 的用例，然后验证变异：移除新增的 OR 项，确认这两个断言都变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| String.raw`^log\.showsignature$`, | ||
| String.raw`^gpg\.program$`, |
There was a problem hiding this comment.
[Critical] R8-2: [fails-closed] [regression] A valueless config key — git's idiomatic spelling of boolean true — collapses the whole probe into PROBE_FAILED. git config --null --get-regexp emits a valueless entry with no newline at all, so the pre-existing parse bailout at :199 (if (newline < 0) return PROBE_FAILED;) discards the entire probe and sets all eleven risk bits. Widening the fetched set from 2 families to 19 is what makes this reachable: the added ^log\.showsignature$, ^remote\..*\.promisor$, ^pager\..*$ and ^pretty\..*$ patterns are exactly the boolean keys people write valueless by hand.
One [log] showSignature line — in the repository's .git/config, or in the user's own ~/.gitconfig or /etc/gitconfig, which this probe also reads because it passes --includes --show-scope — permanently removes read-only auto-approval for every git command in every repository. localGitConfigMakesCommandUnsafe denies on pager/promisorRemote/alternateRefsCommand/hooksPath alone, so git status, git log, git diff, git show, git blame, git branch and git rev-parse all prompt; on the forced-YOLO memory-scoped path (memory-scoped-agent-config.ts:322-325) every git command is hard-denied. Nothing distinguishes this from a genuinely risky repository, and the early return skips the debug log so there is no trail. Git's own reading of that entry is "enabled", so the correct outcome is signatureVerifier gated on a gpg program — not a total deny.
Witness:
git 2.43.0, throwaway repo with .git/config containing "[log]\n\tshowsignature" (no '='):
git config --bool --get log.showsignature -> true
git config --includes --show-scope --null --get-regexp '^log\.showsignature$|...' -> rc=0
bytes: local\0log.showsignature\0local\0core.pager\nless -F\0 (od -c: the valueless entry has no \n)
running the file's own parse loop verbatim over that stdout:
BAILOUT at field index 3 = "log.showsignature" -> PROBE_FAILED
same shape measured for [remote "origin"] + a bare `promisor` line
contrast the explicitly-empty spelling: `git config log.showsignature ""` emits
local\0log.showsignature\n\0, parses to value === '', and git's --bool reading is false
-> so isGitTrueValue('') returning false is correct and is NOT the defect
premise of the fix, measured: valueless [log] showSignature + local gpg.program + a commit
carrying a gpgsig header -> GPG_PROGRAM_RAN=YES; `showSignature = ` (empty) -> did not execute
no test can express this today: the suite's only config-writing helper is
execFileSync('git', ['config', '--local', key, value]) (git-config-safety.test.ts:32-34),
which always supplies a value, so replacing the :199 guard with `continue` leaves all 15 tests green
Parse the valueless form instead of aborting the probe, mapping it to git's semantics (valueless means boolean true), and keep PROBE_FAILED for genuinely malformed framing such as an odd field count — truncated output already fails closed earlier, since exceeding maxBuffer yields status === null:
const newline = entry.indexOf('\n');
if (newline < 0) {
// git emits `scope\0key\0` with no value for a valueless entry and reads it as boolean true
effective.set(entry, { scope: fields[i]!, value: 'true' });
continue;
}Two measured facts this fix must not violate: the key/value split has to stay "first newline", because git emits scope\0key\nvalue\0 and values may contain literal newlines (a<LF>b is emitted local\0pretty.multiline\na\nb\0, and :196-203 relies on entry.indexOf('\n') slicing key from value); and "no value" and "empty value" are opposites in git — valueless prints true from --bool and really executed a local gpg.program marker, while showSignature = prints false and did not — so the sentinel must map to 'true', never to '', or isGitTrueValue's if (value === '' …) return false; at :120/:124 would turn today's fail-closed PROBE_FAILED into a fail-open signatureVerifier: false for exactly the spelling git treats as true. The in-file rule at :113-117 says the same thing: only explicit false values or numeric zero may fail open.
Please add a case to git-config-safety.test.ts that appends the valueless form directly (appendFileSync(path.join(cwd, '.git', 'config'), '[log]\n\tshowSignature\n')) with gpg.program set locally, asserting signatureVerifier === true while .pager === false and .hooksPath === false and the sibling string-valued bits stay false — then confirm the mutation: delete the valueless branch and watch those assertions go red.
中文说明
[Critical] R8-2：[fails-closed] [regression] 一个不带值的配置键 —— 也就是 git 表示布尔 true 的惯用写法 —— 会让整个探测塌缩成 PROBE_FAILED。git config --null --get-regexp 输出不带值的条目时完全没有换行符，因此 :199 处既有的解析兜底（if (newline < 0) return PROBE_FAILED;）会丢弃整个探测结果并把全部十一个风险位置为 true。把抓取范围从 2 个键族扩到 19 个才让这条路径可达：新增的 ^log\.showsignature$、^remote\..*\.promisor$、^pager\..*$ 和 ^pretty\..*$ 恰好就是人们手写时最常不带值的那些布尔键。
只要有一行 [log] showSignature —— 无论它在仓库的 .git/config 里，还是在用户自己的 ~/.gitconfig 或 /etc/gitconfig 里（因为探测传了 --includes --show-scope，这些也会被读到）—— 就会永久性地让每个仓库中的每条 git 命令都失去只读自动批准。localGitConfigMakesCommandUnsafe 仅凭 pager/promisorRemote/alternateRefsCommand/hooksPath 就会拒绝，于是 git status、git log、git diff、git show、git blame、git branch、git rev-parse 全部开始询问；在强制 YOLO 的 memory-scoped 路径上（memory-scoped-agent-config.ts:322-325）所有 git 命令会被硬拒绝。这种情况与真正有风险的仓库无法区分，而且提前返回会跳过 debug 日志，因此完全没有线索。git 自己对该条目的解读是"已启用"，所以正确结果应当是由 gpg program 门控的 signatureVerifier，而不是全面拒绝。
修复方式是解析不带值的形式而不是中止探测，把它映射为 git 的语义（不带值即布尔 true），并把 PROBE_FAILED 保留给真正畸形的帧结构（例如字段数为奇数）—— 输出被截断的情况本来就更早失败关闭了，因为超出 maxBuffer 会得到 status === null。
这个修复不得违反两条实测事实：键/值的切分必须保持"第一个换行符"，因为 git 输出的是 scope\0key\nvalue\0，而值本身可能包含字面换行（a<LF>b 会输出为 local\0pretty.multiline\na\nb\0，:196-203 依赖 entry.indexOf('\n') 来切分键与值）；并且"无值"与"空值"在 git 中含义相反 —— 无值时 --bool 打印 true 且确实执行了本地 gpg.program 标记脚本，而 showSignature = 打印 false 且没有执行 —— 所以哨兵值必须映射为 'true'，绝不能映射为 ''，否则 :120/:124 处 isGitTrueValue 的 if (value === '' …) return false; 会把今天失败关闭的 PROBE_FAILED 变成对 git 视为 true 的那种写法失败开放的 signatureVerifier: false。文件内 :113-117 的规则也表达了同一意思：只有显式的 false 值或数值零才可以失败开放。
请在 git-config-safety.test.ts 中新增一个直接追加无值形式的用例（appendFileSync(path.join(cwd, '.git', 'config'), '[log]\n\tshowSignature\n')），同时在本地设置 gpg.program，断言 signatureVerifier === true 而 .pager === false、.hooksPath === false，且同级的字符串取值风险位仍为 false —— 然后验证变异：删除无值分支，确认这些断言变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| windowsHide: true, | ||
| }, | ||
| ); | ||
| if (result.status !== 0 || typeof result.stdout !== 'string') return true; |
There was a problem hiding this comment.
[Critical] R7-1: [fails-closed] [regression] The hooks probe fails closed on every rev-parse failure, including git's "not a git repository" answer (exit 128), so a working directory that is not inside a repository reports hooksPath: true — which shellAstParser.ts:1163-1170 then fails closed on for every read-only git subcommand. A directory with no repository has no repository config and no hook that could possibly execute, so this is pure collateral.
In any cwd that is not a git repository — a scratch directory, a container workdir, a downloaded folder, or a host where the git binary is missing or the 1000 ms probe times out — git status, git log --oneline -5, git diff, git rev-parse --show-toplevel and git branch --list all fall from auto-approved read-only to a confirmation prompt; on the memory-scoped/dream-agent path (memory-scoped-agent-config.ts:320-324, return isReadOnly ? 'allow' : 'deny') the same bit is a hard denial rather than a prompt. It also leaves packages/core red, so the suite cannot be used to gate this change.
Witness:
getLocalGitConfigRisk('/tmp')
PR 6beb0f0 -> {"diffExternal":false,...,"hooksPath":true}
base 60ccd1a -> {"diffExternal":false,"fsmonitor":false}
classifyShellCommandSafetyInDirectory('git status'|'git log --oneline -5'|'git diff', '/tmp')
base -> read-only | PR -> unknown
git -C /tmp rev-parse --git-path hooks -> fatal: not a git repository (rc=128) -> `return true` at :137
config probe rc=1 -> `{ ...NO_RISK, hooksPath: true }` at :187
npx vitest run src/core/coreToolScheduler.test.ts -t "Plan shell routing" -> 4 failed:
'executes read-only run_shell_command calls without a prompt'
'executes read-only monitor calls without a prompt'
'cancels without execution when aborted while awaiting the host guard'
'skips guard evaluation entirely when no guard is configured'
AssertionError: expected "spy" to be called once, but got 0 times (at :11371/:11467/:11497)
(that harness sets getTargetDir: () => '/tmp')
test-delta: netNew = src/core/coreToolScheduler.test.ts (fails on the PR side only)
Separate "git could not answer because there is no repository" from a genuine probe failure — return false for the not-a-repository answer and keep return true for timeouts, unexpected statuses and unreadable stdout. Equivalently, short-circuit getLocalGitConfigRisk to NO_RISK when git -C <cwd> rev-parse --is-inside-work-tree says the directory is not in a work tree, since there is then no repository config and no hook for a git command to execute:
if (result.status === 128 && /not a git repository/i.test(String(result.stderr))) {
return false; // no repository here, so no hooks directory to probe
}
if (result.status !== 0 || typeof result.stdout !== 'string') return true;The constraint this must respect: git rev-parse --git-path hooks exits 128 both for "no repository" and for a repository git refuses to operate on (dubious ownership / safe.directory), and the latter must still fail closed via the existing PROBE_FAILED return at :191-193 — PROBE_FAILED at :41-53 sets all twelve risk flags true, per the in-file rule at :114-117. git-shell-execution-safety.test.ts:82 also pins the opposite direction for a real repo: expect(getLocalGitConfigRisk(cwd).hooksPath).toBe(true) with an executable .git/hooks/post-index-change and no config, so only the no-repository case may return false.
The four CoreToolScheduler Plan shell routing cases above are the acceptance test — they are red now and must go green, and removing the not-a-repository branch must red them again. Please also add the direct unit case in git-config-safety.test.ts asserting getLocalGitConfigRisk(<directory outside any repo>) is all-false and classifyShellCommandSafetyInDirectory('git status', <that directory>) is 'read-only'.
中文说明
[Critical] R7-1：[fails-closed] [regression] 钩子探测在 rev-parse 的每一种失败上都失败关闭，包括 git 的"这不是一个 git 仓库"回答（退出码 128），于是一个并不位于仓库内的工作目录会报告 hooksPath: true —— 而 shellAstParser.ts:1163-1170 随后会对每一个只读 git 子命令失败关闭。没有仓库的目录既没有仓库配置、也没有任何可能被执行的钩子，所以这完全是附带损害。
在任何不是 git 仓库的 cwd 中 —— 临时目录、容器工作目录、下载下来的文件夹，或者 git 可执行文件缺失、1000 ms 探测超时的主机 —— git status、git log --oneline -5、git diff、git rev-parse --show-toplevel、git branch --list 都会从自动批准的只读降级为需要确认；在 memory-scoped/dream-agent 路径上（memory-scoped-agent-config.ts:320-324，return isReadOnly ? 'allow' : 'deny'），同一个位是硬拒绝而不是询问。它还让 packages/core 处于红灯，因此该测试套件无法用来为这次改动把关。
修复方式是把"因为没有仓库所以 git 无法回答"与真正的探测失败区分开 —— 对"非仓库"回答返回 false，而对超时、意外状态码和不可读的 stdout 继续返回 true。等价做法是：当 git -C <cwd> rev-parse --is-inside-work-tree 表明该目录不在工作树中时，把 getLocalGitConfigRisk 短路为 NO_RISK，因为那时既没有仓库配置、也没有钩子可供 git 命令执行。
必须尊重的约束：git rev-parse --git-path hooks 在"没有仓库"和"git 拒绝操作的仓库"（可疑所有权 / safe.directory）两种情况下都返回 128，后者必须继续通过 :191-193 既有的 PROBE_FAILED 返回来失败关闭 —— :41-53 的 PROBE_FAILED 会把全部十二个风险位置为 true，这与文件内 :114-117 的规则一致。git-shell-execution-safety.test.ts:82 也为真实仓库钉住了相反方向：在存在可执行 .git/hooks/post-index-change 且没有配置时 expect(getLocalGitConfigRisk(cwd).hooksPath).toBe(true)，所以只有"非仓库"这一种情况可以返回 false。
上面四个 CoreToolScheduler Plan shell routing 用例就是验收测试 —— 它们现在是红的、必须变绿，并且移除"非仓库"分支后必须再次变红。也请在 git-config-safety.test.ts 中补一个直接的单元用例，断言 getLocalGitConfigRisk(<任何仓库之外的目录>) 全为 false，且 classifyShellCommandSafetyInDirectory('git status', <该目录>) 为 'read-only'。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const hooksDir = result.stdout.trim(); | ||
| if (!hooksDir) return true; |
There was a problem hiding this comment.
[Critical] R8-3: [certifies-falsely] [new-surface] The hook probe joins the caller-supplied logical cwd to the hooks path git computed, with path.resolve(cwd, hooksDir, 'post-index-change'). path.resolve is purely lexical, so when git returns a relative path containing .. and the cwd's own last component is a symlink, the .. pops the symlink itself instead of the directory it points to — the probe stats a path that does not exist, returns false, and the executable hook is missed. This is the fail-open direction: unlike the neighbouring over-denial findings, it removes a protection this same hunk added.
An agent working directory that is a symlink to a subdirectory of a repository — a common layout for mounted or relocated checkouts — whose real .git/hooks holds an executable post-index-change. Git really runs that hook for git status from the symlinked cwd, but the probe reports hooksPath: false, so the repository reads as no-risk and git status is auto-approved with no confirmation dialog while repository-controlled code executes. The physical-path form of the identical directory works correctly, so the defect is specific to the logical/symlinked cwd.
Witness:
probe run this round (git 2.43.0; repo at $BASE/real with sub/, executable
.git/hooks/post-index-change writing $BASE/marker; $BASE/link-to-sub -> $BASE/real/sub):
git -C $BASE/link-to-sub rev-parse --git-path hooks -> ../.git/hooks
git -C $BASE/real/sub rev-parse --git-path hooks -> ../.git/hooks
git -C $BASE/real rev-parse --git-path hooks -> .git/hooks
path.resolve($BASE/link-to-sub, '../.git/hooks', 'post-index-change')
-> $BASE/.git/hooks/post-index-change hookFileExists=false <-- wrong directory
path.resolve($BASE/real/sub, '../.git/hooks', 'post-index-change')
-> $BASE/real/.git/hooks/post-index-change hookFileExists=true
git -C $BASE/link-to-sub status --porcelain
-> post-index-change FIRED from the symlinked cwd (marker present)
neighbouring cases cleared, so the fix can be scoped:
--git-path hooks resolves to the common dir in a linked worktree (/tmp/wtp/main/.git/hooks)
and honours core.hooksPath (.githooks, ../.githooks, ../../.githooks from subdirectories),
so path.resolve lands on the real hook at every depth tested for PHYSICAL cwds
Resolve against the physical path git used rather than the logical cwd. Ask git for an absolute hooks path, or realpath the cwd before joining — an absolute hooksDir makes path.resolve(cwd, hooksDir, …) return hooksDir unchanged, which removes the lexical-.. hazard entirely:
const result = spawnSync(
'git',
['-C', cwd, 'rev-parse', '--path-format=absolute', '--git-path', 'hooks'],
{ encoding: 'utf8', maxBuffer: 16 * 1024, timeout: 1000, windowsHide: true },
);--path-format=absolute needs git >= 2.31, so either gate it and fall back to realpath-ing cwd on older git, or use --absolute-git-dir combined with the hooks suffix. The fix must keep the behaviour that is correct today: git-shell-execution-safety.test.ts:82 pins expect(getLocalGitConfigRisk(cwd).hooksPath).toBe(true) for a real repo holding an executable .git/hooks/post-index-change and no config, and --git-path hooks was measured to honour core.hooksPath and to resolve to the common dir in a linked worktree — so an absolute-path switch must lose neither. The probe is built at :130 and spawns with timeout: 1000.
Please add a case to git-shell-execution-safety.test.ts for a repository reached through a symlinked parent directory (symlink pointing at <repo>/sub) holding an executable post-index-change, asserting getLocalGitConfigRisk(symlinkCwd).hooksPath === true and isShellCommandReadOnlyASTInDirectory('git status', symlinkCwd) === false — both are the opposite today, and removing the absolute-path resolution must red them again.
中文说明
[Critical] R8-3：[certifies-falsely] [new-surface] 钩子探测把调用方提供的逻辑 cwd 与 git 计算出的 hooks 路径用 path.resolve(cwd, hooksDir, 'post-index-change') 拼接。path.resolve 是纯词法运算，所以当 git 返回的相对路径中包含 ..、而 cwd 自身最后一段又是符号链接时，这个 .. 弹出的是符号链接本身而不是它指向的目录 —— 探测会去 stat 一个不存在的路径，返回 false，从而漏掉那个可执行钩子。这是失败开放方向：与相邻那些过度拒绝的问题不同，它移除了同一个 hunk 新加的保护。
场景是：agent 的工作目录是一个指向仓库某个子目录的符号链接（挂载或迁移过的检出常见这种布局），而真实的 .git/hooks 中有一个可执行的 post-index-change。从这个符号链接 cwd 执行 git status 时 git 确实会运行该钩子，但探测报告 hooksPath: false，于是该仓库被读成无风险，git status 在没有任何确认对话框的情况下被自动批准，同时仓库可控的代码被执行。同一个目录的物理路径形式是正确的，所以缺陷专属于逻辑/符号链接 cwd。
修复方式是针对 git 实际使用的物理路径来解析，而不是逻辑 cwd：向 git 索取绝对路径的 hooks 目录，或者在拼接之前对 cwd 做 realpath —— 绝对的 hooksDir 会让 path.resolve(cwd, hooksDir, …) 原样返回 hooksDir，从而彻底消除词法 .. 的隐患。
--path-format=absolute 需要 git >= 2.31，所以要么对它做版本判断并在更旧的 git 上回退为对 cwd 做 realpath，要么使用 --absolute-git-dir 加上 hooks 后缀。修复必须保留今天正确的行为：git-shell-execution-safety.test.ts:82 钉住了真实仓库（含可执行 .git/hooks/post-index-change 且无配置）下 expect(getLocalGitConfigRisk(cwd).hooksPath).toBe(true)，并且实测 --git-path hooks 会尊重 core.hooksPath、在链接工作树中会解析到 common dir —— 所以切换到绝对路径两者都不能丢失。探测在 :130 构造，并以 timeout: 1000 派生进程。
请在 git-shell-execution-safety.test.ts 中补一个用例：通过一个符号链接父目录（符号链接指向 <repo>/sub）访问的仓库，其中含有可执行的 post-index-change，断言 getLocalGitConfigRisk(symlinkCwd).hooksPath === true 且 isShellCommandReadOnlyASTInDirectory('git status', symlinkCwd) === false —— 这两者今天都是相反的，并且移除绝对路径解析后必须再次变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| return /[[*?]/.test(text) || hasShellBraceExpansion(text); | ||
| return ( | ||
| /[[*?]/.test(text) || | ||
| /\\./.test(text) || |
There was a problem hiding this comment.
[Critical] R7-2: [fails-closed] [regression] The /\\./ term added to this shared predicate treats every backslash escape in an argument as pattern expansion. Because hasShellPatternExpansion reaches ten root command families, it both reddens a pre-existing expectation this diff does not update and withdraws auto-approval from ordinary read-only commands. packages/core is red at this commit and green on base.
shellAstParser.test.ts:1073 ("classifies adversarial rule inputs within the CPU budget") expects 'read-only' for git status followed by '\\{'.repeat(10_000) and receives 'unknown'. Beyond the red suite, legitimately read-only invocations carrying an escaped character lose auto-approval and start prompting — and in Plan mode route through plan-mode-shell-policy.ts:168-175 — across awk, find, git, printf, rg, ripgrep, sed, sort, tree and uniq. No test pins the new classification for any non-git root, so the widened blast radius ships unasserted in both directions.
Witness:
npx vitest run src/utils/shellAstParser.test.ts
BASE 60ccd1a -> Test Files 1 passed (1) / Tests 548 passed (548)
PR 6beb0f0 -> Test Files 1 failed / Tests 1 failed | 553 passed (554)
FAIL classifyShellCommandSafety > classifies adversarial rule inputs within the CPU budget
- "read-only" + "unknown" at shellAstParser.test.ts:1073 (6th input)
test-delta: netNew = src/utils/shellAstParser.test.ts (fails on the PR side only)
base-vs-PR through classifyShellCommandSafety (all read-only on base, unknown on the PR):
git log --author=Jane\ Doe | git diff -- C:\repo\src\file.ts | git show HEAD\~1
git log --grep=fix\:\ bug | git status -- src\utils | rg foo\.bar | printf foo\nbar
awk -F'\t' '{print $1}' file.csv | git log -S'foo\.' -1
(5 of a 52-command ordinary-read-only population flip while the fallback arm still
returns true, so the two arms also disagree on them)
controls unchanged in both arms: fully-quoted escapes such as grep -rn '\bfoo\b' src,
sed -n 's/\./x/p' file.txt, awk '/\./ { print }' file.txt
revert experiment (only the /\\./ term removed, nothing else touched):
shellAstParser.test.ts -> fully green
the PR's own new git-shell-execution-safety.test.ts
'fails closed for unquoted backslash-escaped Git helper options' -> RED
(expected 'read-only' to be 'unknown')
The two expectations are mutually unsatisfiable with the predicate as written, so relocating the rule is the only fix that satisfies both — simply deleting it is not available. Take the escape check out of the shared predicate and apply it where the raw-text/word-value mismatch actually matters: resolve each git argument to its shell word value (de-escape \X to X for word/concatenation nodes, drop quote characters) and match GIT_EXTERNAL_HELPER_OPTION against that, which reproduces all four spellings the rule exists for — --show\-signature, --ext\-diff, --textcon\v, --format=%G\K — without touching the other nine root families. If the blanket widening is deliberate instead, update the pinned expectation at shellAstParser.test.ts:1076-1083 in this same PR and state the auto-approval loss for escaped-argument commands in the description; the suite cannot land red either way.
The relocation has to keep firing on the right node set: hasShellPatternExpansion has exactly one consumer, hasShellExpansion at shellAstParser.ts:740-746, reached only from the root gate at :1047-1053 (/^(awk|find|git|printf|rg|ripgrep|sed|sort|tree|uniq)$/) and only for ['word','concatenation'] node types. Note also that the four spellings pinned at git-shell-execution-safety.test.ts:132-139 include one with no \- — 'git grep --textcon\\v needle', at :136 — so a /\\-/-only narrowing regresses it; only escape-normalization keeps all four. Any replacement check must stay O(args), since the same test asserts (cpuUsage.user + cpuUsage.system) / 1000 < maxClassificationCpuMs with const maxClassificationCpuMs = 4000; at shellAstParser.test.ts:642.
The acceptance criterion is both sides: shellAstParser.test.ts "classifies adversarial rule inputs within the CPU budget" goes green, and git-shell-execution-safety.test.ts "fails closed for unquoted backslash-escaped Git helper options" stays green. Please run the mutation both ways — remove the relocated check and confirm the escape test reds, restore the shared-predicate version and confirm the CPU-budget test reds.
中文说明
[Critical] R7-2：[fails-closed] [regression] 加到这个共享谓词里的 /\\./ 项会把参数中的每一个反斜杠转义都当作模式展开。由于 hasShellPatternExpansion 覆盖十个根命令族，它既让一个本 diff 未同步更新的既有断言变红，也让普通只读命令失去自动批准。在这个提交上 packages/core 是红的，而在 base 上是绿的。
除了红灯测试之外，携带转义字符的合法只读调用也会失去自动批准并开始询问 —— 在 Plan 模式下还会经过 plan-mode-shell-policy.ts:168-175 —— 影响范围覆盖 awk、find、git、printf、rg、ripgrep、sed、sort、tree、uniq。没有任何测试为任何非 git 根命令钉住新的分类结果，所以这次扩大的影响面在两个方向上都是无断言地发布。
关键点是：这两个断言在谓词现有写法下无法同时满足，因此把规则迁移位置是唯一能同时满足两者的修复 —— 单纯删除并不可行。把转义检查从共享谓词中取出，放到真正存在"原始文本 vs shell 词值"不匹配的地方：把每个 git 参数解析为它的 shell 词值（对 word/concatenation 节点把 \X 反转义为 X，并去掉引号字符），再拿它去匹配 GIT_EXTERNAL_HELPER_OPTION。这样能复现该规则本来要覆盖的全部四种写法 —— --show\-signature、--ext\-diff、--textcon\v、--format=%G\K —— 而不触碰其余九个根命令族。如果这种全面扩大是有意的，那么请在同一个 PR 中更新 shellAstParser.test.ts:1076-1083 的钉住期望，并在描述中说明带转义参数命令失去自动批准的代价；无论哪种方式，测试套件都不能带着红灯合入。
迁移必须继续作用在正确的节点集合上：hasShellPatternExpansion 只有一个消费者，即 shellAstParser.ts:740-746 的 hasShellExpansion，并且只从 :1047-1053 的根命令门（/^(awk|find|git|printf|rg|ripgrep|sed|sort|tree|uniq)$/）、只对 ['word','concatenation'] 节点类型生效。另外注意 git-shell-execution-safety.test.ts:132-139 钉住的四种写法中有一种不含 \- —— :136 的 'git grep --textcon\\v needle', —— 所以只收窄成 /\\-/ 会让它回归；只有转义归一化能同时保住四种。任何替代检查都必须保持 O(args)，因为同一个测试断言了 (cpuUsage.user + cpuUsage.system) / 1000 < maxClassificationCpuMs，其中 shellAstParser.test.ts:642 定义了 const maxClassificationCpuMs = 4000;。
验收标准是两侧同时成立：shellAstParser.test.ts 的 "classifies adversarial rule inputs within the CPU budget" 变绿，且 git-shell-execution-safety.test.ts 的 "fails closed for unquoted backslash-escaped Git helper options" 保持绿。请双向运行变异 —— 移除迁移后的检查确认转义测试变红，恢复共享谓词版本确认 CPU 预算测试变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| @@ -139,7 +139,7 @@ const WRITE_GIT_SUBCOMMAND = | |||
| const WRITE_GIT_REMOTE_ACTION = | |||
| /^(add|remove|rm|rename|set-branches|set-head|set-url|update)$/; | |||
| const GIT_EXTERNAL_HELPER_OPTION = | |||
There was a problem hiding this comment.
[Critical] R8-5: [certifies-falsely] [new-surface] Every option-shape gate on the AST path decides on the raw source text of an argument node (stripOuterQuotes(node.text) at :1001 and :1152) instead of the shell word value git actually receives. stripOuterQuotes only strips when the whole token is quoted, so any intra-word or partial quoting hides the option from the regex — and the /\\./ and /%G/ terms added beside it this round close only the backslash spelling, not quoting. The entrance space here is unbounded: quoting arrangements cannot be enumerated and closed one spelling at a time, so this is reported as one class-level finding with the demonstrated entrances as evidence.
A repository that ships a hostile local config plus a signed commit or a .gitattributes driver gets its own program executed with no confirmation dialog, because getDefaultPermission() returns 'allow' on a read-only classification. Every entrance below was measured end to end — the classifier said read-only AND the repository-controlled helper really ran. The remerge-diff, --diff-merges= and %G rules this diff adds inherit the same hole, so each new guard is void for the very spellings it was added for.
Witness:
measured through the production entry point isShellCommandReadOnlyASTInDirectory
(wired to shell.ts:2072 -> :2077 `return 'allow'`), git 2.43.0, marker helpers:
repo with diff.pwn.textconv + .gitattributes `* diff=pwn`:
git grep --textcon"v" needle -> read-only, TEXTCONV-RAN
git grep "--textco""nv" needle -> read-only, textconv ran
git cat-file "--textco""nv" HEAD:a.txt-> read-only, textconv ran
repo with local gpg.program + a commit carrying a gpgsig header:
git log '--pretty=format:%GB' -> read-only, GPG-PROGRAM-RAN
git log --show"-"signature -1 -> read-only, marker created
repo with diff.external:
git log -p -1 --ext-dif'f' -> astInDir=true fb=false helperExecuted=true
git log -p -1 "--ext"'-diff' -> likewise
git log -p -1 --ext-diff (control) -> astInDir=false
also read-only with the helper running:
git log --ext-""diff -p -1 | git log --ext-dif$'f' -p -1 | git log --ext-$'diff' -p -1
git show --ext-dif$'f' HEAD | git log --textcon'v' -1 | git log --show-signatur'e' -1
git log --open-files-in-page'r' | git log --remerge-dif'f' -1
git log --diff-merges="remerge" -1 | git log --diff-merges='remerge' -1
git show --diff-merges="r" HEAD | git log "--remerge-di""ff" -1
git log --diff-merges=$'remerge' -1
the regex-fallback arm returns false for every one of these, so the permissive side
is the arm that auto-approves
already closed, so the fix need not cover it: backslash-newline continuations
(--show\<NL>-signature, --ext\<NL>-diff, --textcon\<NL>v, --format=%G\<NL>K,
--remerge\<NL>-diff, --diff-merges=remerg\<NL>e, --filter\<NL>s) all return unknown
via classifyInternal's `command.includes('\\\n')` re-classification
Resolve each argument node to its shell word value once and match every option gate against that, in both classifiers: build the value from the node's literal leaves — word.text, the inner text of raw_string/string parts, decoded ansi_c_string content — unescaping \X to X outside single quotes and dropping quote characters, and returning a sentinel that forces 'unknown' for any part that is not statically resolvable (simple_expansion, expansion, arithmetic_expansion, command_substitution, ANSI-C quoting). GIT_EXTERNAL_HELPER_OPTION, the %G check and the backslash check then all read the value bash hands git, and the /\\./ and /%G/ terms can leave the shared predicate — which also resolves the red-suite finding on shell-safety-rules.ts. Three independent reviewers converged on this same structural fix rather than another special case.
Both raw-text read sites must be covered or the config gate keeps deciding on raw text: const args = argNodes.map((node) => stripOuterQuotes(node.text)); at :1001 and the identical mapping inside localGitConfigMakesCommandUnsafe at :1149-1153 — and stripOuterQuotes also produces args[0], the subcommand the config gate reads at :1149-1153. Routing the fix through hasShellPatternExpansion instead cannot work: hasShellExpansion at :742-745 consults that predicate only for ['word','concatenation'] node types and its call site is gated to ten roots at :1050, so a fully quoted argument never reaches it. beforeTerminator (:754) matches the literal token --, and the fix must keep git diff --word-diff=color -- file.txt read-only, pinned green at shellAstParser.test.ts:1352. The design doc requires the AST and fallback helper-option rules to stay in lockstep, so the normalization must land in shellReadOnlyChecker.ts too.
Please extend git-shell-execution-safety.test.ts "fails closed for unquoted backslash-escaped Git helper options" with git log --show"-"signature -1, git log -p --ext-dif'f', git grep --textcon"v" needle, git log --ext-dif$'f' -p, git log --diff-merges="remerge" -1 and git show --diff-merges="r" HEAD, each asserting classifyShellCommandSafety(...) === 'unknown', plus a repo-level isShellCommandReadOnlyASTInDirectory(...) === false case in a repo with a local gpg.program — all are read-only/true today, so removing the normalization must red them. Do not add the isShellCommandReadOnly half for these inputs: the fallback already returns false, so it does not discriminate.
中文说明
[Critical] R8-5：[certifies-falsely] [new-surface] AST 路径上所有基于选项形状的门都是拿参数节点的原始源文本（:1001 和 :1152 处的 stripOuterQuotes(node.text)）来判断，而不是 git 实际收到的 shell 词值。stripOuterQuotes 只在整个 token 都被引号包裹时才剥离，所以任何词内或部分引号都会让该选项从正则眼前消失 —— 而本轮在旁边新增的 /\\./ 和 /%G/ 两项只覆盖了反斜杠写法，没有覆盖引号写法。这里的入口空间是无界的：引号排列无法逐一枚举封堵，因此这条以类级问题上报，并把已实证的入口作为证据。
一个自带恶意本地配置、并带有签名提交或 .gitattributes 驱动的仓库，可以在没有任何确认对话框的情况下让自身程序被执行，因为 getDefaultPermission() 在只读分类上返回 'allow'。下面每一个入口都做了端到端实测 —— 分类器判定为只读，并且仓库可控的辅助程序确实运行了。本 diff 新增的 remerge-diff、--diff-merges= 和 %G 规则继承了同一个漏洞，所以每一个新加的守卫对它本要覆盖的那些写法都是无效的。
修复方式是把每个参数节点一次性解析为它的 shell 词值，并在两个分类器中都用该词值去匹配所有选项门：从节点的字面叶子构造该值 —— word.text、raw_string/string 部分的内部文本、解码后的 ansi_c_string 内容 —— 在单引号之外把 \X 反转义为 X 并去掉引号字符，同时对任何无法静态解析的部分（simple_expansion、expansion、arithmetic_expansion、command_substitution、ANSI-C 引号）返回一个强制 'unknown' 的哨兵值。这样 GIT_EXTERNAL_HELPER_OPTION、%G 检查和反斜杠检查读取的都是 bash 交给 git 的值，/\\./ 和 /%G/ 也就可以从共享谓词中移出 —— 这同时解决了 shell-safety-rules.ts 上的红灯测试问题。三位独立评审都收敛到同一个结构性修复，而不是再加一个特例。
两个读取原始文本的位置都必须覆盖，否则配置门仍然基于原始文本判断：:1001 的 const args = argNodes.map((node) => stripOuterQuotes(node.text)); 以及 :1149-1153 中 localGitConfigMakesCommandUnsafe 内完全相同的映射 —— 而且 stripOuterQuotes 还产生 args[0]，即配置门在 :1149-1153 读取的子命令。改为通过 hasShellPatternExpansion 来修复是行不通的：:742-745 的 hasShellExpansion 只对 ['word','concatenation'] 节点类型查询该谓词，且其调用点在 :1050 被限制为十个根命令，所以完全被引号包裹的参数永远到不了它那里。beforeTerminator（:754）匹配字面 token --，并且修复必须保持 git diff --word-diff=color -- file.txt 为只读（shellAstParser.test.ts:1352 已钉住为绿）。设计文档要求 AST 与 fallback 的辅助选项规则保持同步，因此归一化也必须落到 shellReadOnlyChecker.ts。
请在 git-shell-execution-safety.test.ts 的 "fails closed for unquoted backslash-escaped Git helper options" 中补充 git log --show"-"signature -1、git log -p --ext-dif'f'、git grep --textcon"v" needle、git log --ext-dif$'f' -p、git log --diff-merges="remerge" -1 和 git show --diff-merges="r" HEAD，每个都断言 classifyShellCommandSafety(...) === 'unknown'，再加一个仓库级用例：在设置了本地 gpg.program 的仓库中断言 isShellCommandReadOnlyASTInDirectory(...) === false —— 这些今天都是只读/true，所以移除归一化后必须变红。不要为这些输入补 isShellCommandReadOnly 那一半：fallback 已经返回 false，因此它没有区分度。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS = new Set([ | ||
| 'branch', | ||
| 'cat-file', |
There was a problem hiding this comment.
[Critical] R8-7: [certifies-falsely] The new "proven non-consumer" set names cat-file, show and rev-parse as exempt from the fsmonitor / worktree-filter gate, but those subcommands do execute the repository-controlled core.fsmonitor command whenever they resolve an index revision. Deriving mechanism consumption from subcommand identity has no last corner: index access is argument-driven (:path, :stage:path) and, for git cat-file --batch, stdin-driven — so no enumeration of subcommands can converge.
A repository whose .git/config sets core.fsmonitor to a command — the hook form getLocalGitConfigRisk reports as fsmonitor: true. git cat-file -p :a.c, git show :a.c, git rev-parse :a.c and echo :a.c | git cat-file --batch each classify read-only, so getDefaultPermission() returns 'allow' with no prompt, and each really runs the repository-configured program. The stdin form is invisible to any argument-based narrowing, which is why the class cannot be closed by trimming the set alone.
Witness:
ORACLE (git 2.43.0, repo with core.fsmonitor=<hook>, commit present):
git cat-file -p :a.c RC=0 FSMONITOR_EXECUTED: YES
git show :a.c RC=0 FSMONITOR_EXECUTED: YES
git rev-parse :a.c RC=0 FSMONITOR_EXECUTED: YES
echo :a.c | git cat-file --batch RC=0 FSMONITOR_EXECUTED: YES
controls: git log -1 / git show HEAD / git cat-file -p HEAD:a.c / git branch /
git remote -v -> FSMONITOR_EXECUTED: NO
CLASSIFIER (this commit, same repo, RISK.fsmonitor = true):
all four forms ASTreadOnly=true
FLIP (candidate fix applied in a scratch tree - `risk.fsmonitor || risk.worktreeFilter`
moved into the unconditional fail-closed block at :1163-1169 - then reverted):
all four forms ASTreadOnly=false
(controls also flip to false, which is the over-denial cost named below)
consequence chain read in code, not inferred:
shell.ts:2070-2078 const isReadOnly = await isShellCommandReadOnlyASTInDirectory(command, ...);
if (isReadOnly) { return 'allow'; }
with the author's own comment three lines above: "Without this gate the command
auto-executes silently with no confirmation dialog and no warning."
a second execution sweep independently extended the control set with no new member:
fsmonitor fired for status, diff, ls-files, grep, blame, describe --dirty and the
`:path` forms of rev-parse/show/cat-file --batch; and NOT for log (with or without
pathspec, --follow, --name-status, -p), branch -v/-a/--contains/--show-current,
remote -v/show -n/get-url, rev-parse HEAD, cat-file -p HEAD:a.txt, show HEAD,
show HEAD:a.txt, show --stat, describe --all
Close the class rather than the corner: move risk.fsmonitor || risk.worktreeFilter into the existing unconditional fail-closed block beside risk.pager / promisorRemote / alternateRefsCommand / hooksPath and delete FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS — which is exactly what fallbackGitConfigMakesCommandUnsafe already does via Object.values(getLocalGitConfigRisk(cwd)).some(Boolean). The minimal alternative is to drop 'cat-file', 'show' and 'rev-parse' from the set, since branch, log and remote were measured to be genuine non-consumers — but that still leaves the stdin-driven --batch form to argue about, so the fail-closed move is the one that actually closes the class.
The constraint to plan around: the same two tests currently pin the opposite expectation for the HEAD forms — for (const command of ['git log -1', 'git show HEAD', 'git rev-parse HEAD']) { expect(await isShellCommandReadOnlyASTInDirectory(command, cwd)).toBe(true); } in git-config-safety.test.ts, in both the fsmonitor and the clean/process-filter cases. Those assertions are correct today (git show HEAD and git rev-parse HEAD touch no index — measured), so the fail-closed fix must update them deliberately rather than silently; note the minimal alternative also flips git show HEAD and git rev-parse HEAD, and only git log -1 survives both options.
Please add git cat-file -p :file.txt, git show :file.txt, git rev-parse :file.txt and echo :file.txt | git cat-file --batch to the toBe(false) loop of "gates command-form fsmonitor with a default-deny consumer model" and to the same loop of "gates clean and process filters while preserving proven non-consumers" — all four are true/read-only at this commit, so they are red now and go green only with the fix, and reverting the fix must revert them.
中文说明
[Critical] R8-7：[certifies-falsely] 新增的"已证明非消费者"集合把 cat-file、show、rev-parse 列为可豁免于 fsmonitor / worktree-filter 门，但这些子命令在解析索引修订（index revision）时确实会执行仓库可控的 core.fsmonitor 命令。用子命令身份来推断机制消费没有最后一个角落可封：索引访问是由参数驱动的（:path、:stage:path），而对 git cat-file --batch 还是由标准输入驱动的 —— 所以任何子命令枚举都无法收敛。
场景是：仓库 .git/config 把 core.fsmonitor 设为一条命令（也就是 getLocalGitConfigRisk 报告为 fsmonitor: true 的钩子形式）。此时 git cat-file -p :a.c、git show :a.c、git rev-parse :a.c 以及 echo :a.c | git cat-file --batch 都被分类为只读，于是 getDefaultPermission() 在不询问的情况下返回 'allow'，而它们确实运行了仓库配置的程序。标准输入形式对任何基于参数的收窄都是不可见的，这正是为什么仅靠裁剪该集合无法关闭这一类问题。
修复方式是关闭整类问题而不是某个角落：把 risk.fsmonitor || risk.worktreeFilter 移入 :1163-1169 处既有的无条件失败关闭分支（与 risk.pager / promisorRemote / alternateRefsCommand / hooksPath 并列），并删除 FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS —— 这正是 fallbackGitConfigMakesCommandUnsafe 通过 Object.values(getLocalGitConfigRisk(cwd)).some(Boolean) 已经在做的事。最小替代方案是从集合中去掉 'cat-file'、'show'、'rev-parse'，因为实测 branch、log、remote 确实是真正的非消费者 —— 但那样仍留下由标准输入驱动的 --batch 形式需要争辩，所以失败关闭的做法才是真正能关闭整类问题的。
需要事先规划的约束：同样这两个测试目前对 HEAD 形式钉住了相反的期望 —— git-config-safety.test.ts 中 fsmonitor 用例与 clean/process-filter 用例里的 for (const command of ['git log -1', 'git show HEAD', 'git rev-parse HEAD']) { expect(await isShellCommandReadOnlyASTInDirectory(command, cwd)).toBe(true); }。这些断言今天是正确的（实测 git show HEAD 和 git rev-parse HEAD 不触碰索引），所以失败关闭的修复必须有意识地更新它们，而不是悄悄地改；注意最小替代方案同样会翻转 git show HEAD 和 git rev-parse HEAD，只有 git log -1 在两种方案下都能保住。
请把 git cat-file -p :file.txt、git show :file.txt、git rev-parse :file.txt 和 echo :file.txt | git cat-file --batch 加入 "gates command-form fsmonitor with a default-deny consumer model" 的 toBe(false) 循环，以及 "gates clean and process filters while preserving proven non-consumers" 的同一循环 —— 这四条在本提交上都是 true/只读，所以现在就是红的、只有修复后才变绿，并且回退修复必须让它们再次变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| hasShellSubstitution(this.params.command) || | ||
| LEADING_ENV_ASSIGNMENT_RE.test(this.params.command) |
There was a problem hiding this comment.
[Critical] R9-1: [certifies-falsely] [new-surface] The new GIT_CONFIG_* gate lives only in the default-permission fallback, but the confirmation surface it hands off to — getConfirmationDetails, in this same file and untouched by this diff — still derives rootCommand and the "Always allow" permissionRules from the env-stripped command, and allow-rule matching itself discards leading assignments. So the dialog this gate creates offers a rule describing the injection-free inner command, and accepting it switches the gate off permanently. This falsifies the design-doc sentence added in the same hunk, which frames rule override as only "Explicit user-configured Bash(...) allow rules".
The model proposes GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=/tmp/evil bash -c 'git diff' — a form that reaches a dialog only because this gate now stops it. The dialog shows root command git and offers Bash(git diff), with no warning mentioning the injection. One "Always allow" click persists that rule; from then on GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=/tmp/evil2 git status --short round-trips to finalPermission: 'allow' with pmForcedAsk: false, and git executes /tmp/evil2 with no prompt — precisely the repository-controlled-helper execution this PR's config probe exists to stop. The same override reaches a user who never saw this dialog: any pre-existing hand-written Bash(git status) or Bash(git *) rule already matches the env-injected form.
Witness:
driven through the real ShellToolInvocation and the real evaluatePermissionFlow at 6beb0f0bdf:
{"raw":"GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=/tmp/helper bash -c 'git diff'",
"defaultPermission":"ask","dialogCommand":"<raw, unchanged>","dialogRootCommand":"git",
"dialogPermissionRules":["Bash(git diff)"],"dialogWarnings":null}
{"raw":"GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=/tmp/helper git status",
"defaultPermission":"ask","dialogRootCommand":"git",
"dialogPermissionRules":["Bash(git status)"],"dialogWarnings":null}
round trip after persisting the dialog's own suggested rule:
suggested rules from dialog = ["Bash(git status)"]
{"laterCommand":"GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=/tmp/evil2 git status --short",
"defaultPermission":"ask","finalPermission":"allow","pmForcedAsk":false,"pmEvaluate":"allow"}
baseline A/B at 60ccd1a: the wrapper form -> defaultPermission "allow"
(silent auto-execute, no dialog, no rule), so the dialog this diff newly creates
is what introduces the laundering path for that form
tested fix: suppressing derived rules when the raw command carries an env prefix or a
wrapper flips the probe to dialogPermissionRules:[] and round-trip finalPermission:"ask",
and is not defeated by central injection because injectPermissionRulesIfMissing
(permission-helpers.ts:164-176) tests `!confirmationDetails.permissionRules` and [] is truthy
Suppress the derived rules for exactly the forms the new gate stops, keep the raw command in the dialog, and surface the injection as a warning so the user can see what they are approving — then update the design-doc sentence to say that auto-suggested rules are suppressed for these forms, not merely that user-configured rules can override:
const gatedRawForm =
LEADING_ENV_ASSIGNMENT_RE.test(this.params.command) ||
stripShellWrapper(this.params.command) !== this.params.command;
// ... when gatedRawForm: permissionRules = [], and push an env-injection warningTwo facts this fix rests on. permissions/rule-parser.ts:978 reads const normalizedCommand = stripLeadingVariableAssignments(command); — allow-rule matching discards leading assignments, so any persisted rule that omits the env prefix matches every later env-injected invocation of the same root/subcommand; a fix that only changes the displayed rule text, or that qualifies the rule with the prefix, does not close the match. And the design doc states explicit user-configured Bash(...) allow rules "remain a separate authorization layer and can intentionally override the default ask decision; this PR does not change that pre-existing rule-matching behavior", so the fix must narrow only the auto-suggested rule, never disable explicit rule matching — the existing lever for hiding "Always allow" is pmForcedAsk, set only when pmDecision === 'ask' && pm.hasMatchingAskRule(pmCtx) (permission-helpers.ts:139-141). One mechanism correction for whoever implements it: for the bare-prefix form stripShellWrapper is a no-op (measured stripped === raw); the prefix is dropped by rule extraction itself, shellAstParser.ts:1401-1404 returning [] for pure assignments.
Please extend the case this diff adds, "should keep env-prefixed Git wrappers confirmable before stripping" in shell.test.ts, to also assert (await invocation.getConfirmationDetails(signal)).permissionRules is [] for both commands in its list (today it yields Bash(git diff) and Bash(git status)), plus a round-trip assertion that after persisting the dialog's own suggested rule evaluatePermissionFlow still yields finalPermission: 'ask' for the same raw command — deleting the suppression must turn both red.
中文说明
[Critical] R9-1：[certifies-falsely] [new-surface] 新的 GIT_CONFIG_* 门只存在于默认权限回退分支中，但它交棒给的确认界面 —— 同一个文件里、本 diff 未改动的 getConfirmationDetails —— 仍然从剥离环境变量后的命令推导 rootCommand 和 "Always allow" 的 permissionRules，而 allow 规则匹配本身也会丢弃前导赋值。于是这个门所创建的对话框提供了一条描述"无注入内层命令"的规则，接受它就会永久关闭这个门。这与同一 hunk 中新增的设计文档表述相矛盾 —— 那句话把规则覆盖限定为"用户显式配置的 Bash(...) allow 规则"。
模型提出 GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.external GIT_CONFIG_VALUE_0=/tmp/evil bash -c 'git diff' —— 这种形式之所以会走到对话框，正是因为这个新门拦住了它（在此之前它会静默自动允许，既没有对话框也没有规则）。对话框显示根命令为 git，并提供 Bash(git diff)，警告中完全不提这次注入。用户点一次 "Always allow" 就会持久化该规则；此后 GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=/tmp/evil2 git status --short 会一路走到 finalPermission: 'allow' 且 pmForcedAsk: false，git 在没有任何提示的情况下执行 /tmp/evil2 —— 这恰恰是本 PR 的配置探测要阻止的"仓库可控辅助程序执行"。同样的覆盖也会影响从未见过这个对话框的用户：任何既有的手写 Bash(git status) 或 Bash(git *) 规则已经能匹配这种带环境注入的形式。
修复方式是：只对新门拦下的这些形式抑制推导出的规则，在对话框中保留原始命令，并把注入作为警告显示出来，让用户看清自己批准的是什么 —— 然后更新设计文档那句话，说明对这些形式会抑制自动建议的规则，而不只是说用户配置的规则可以覆盖。
该修复依赖两个事实。permissions/rule-parser.ts:978 是 const normalizedCommand = stripLeadingVariableAssignments(command); —— allow 规则匹配会丢弃前导赋值，所以任何省略了环境变量前缀的持久化规则都会匹配之后每一次同根/同子命令的带注入调用；只改显示的规则文本、或者给规则加上前缀限定，都关不上这个匹配。另外设计文档写明用户显式配置的 Bash(...) allow 规则"仍是独立的授权层，可以有意覆盖默认的 ask 决定；本 PR 不改变这一既有规则匹配行为"，所以修复只能收窄自动建议的规则，绝不能禁用显式规则匹配 —— 隐藏 "Always allow" 的既有开关是 pmForcedAsk，仅在 pmDecision === 'ask' && pm.hasMatchingAskRule(pmCtx) 时设置（permission-helpers.ts:139-141）。给实现者的一处机制更正：对纯前缀形式，stripShellWrapper 是空操作（实测 stripped === raw）；前缀是被规则抽取本身丢掉的 —— shellAstParser.ts:1401-1404 对纯赋值返回 []。
请扩展本 diff 新增的 shell.test.ts 用例 "should keep env-prefixed Git wrappers confirmable before stripping"，同时断言其命令列表中两条命令的 (await invocation.getConfirmationDetails(signal)).permissionRules 为 []（今天会得到 Bash(git diff) 和 Bash(git status)），并补一个往返断言：在持久化对话框自己建议的规则之后，对同一条原始命令 evaluatePermissionFlow 仍应得到 finalPermission: 'ask' —— 删除该抑制必须让这两条断言变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const GIT_EXTERNAL_HELPER_OPTION = | ||
| /(?:^--(?:ext-diff|filters|show-signature|textconv|open-files-in-pager)(?:=|$)|%G[?GKFPST])/; | ||
| /(?:^--(?:ext-diff|filters|show-signature|textconv|open-files-in-pager|remerge-diff)(?:=|$)|^--diff-merges=(?:remerge|r)$|%G)/; |
There was a problem hiding this comment.
[Critical] R9-2: [certifies-falsely] This diff widens the fallback classifier's %G[?GKFPST] to bare %G, but the AST arm keeps the narrow class in invokesHelper (shellAstParser.ts:803) and the new /%G/ term in hasShellPatternExpansion reaches only word and concatenation nodes — a fully quoted argument is a string/raw_string node and is never pattern-checked. The two arms therefore disagree on quoted %G formats, and the primary AST arm is the permissive one. That contradicts the design-doc sentence this same diff adds about keeping the AST and fallback helper-option rules in lockstep for %G* cases.
In a repository whose local config sets gpg.program to a hostile command, a fully quoted %G pretty format is auto-approved with no confirmation dialog while git executes that program. Ten of thirty-six probed spellings diverge and the AST arm is permissive in all ten — the double-quoted and single-quoted forms of %GV, %GB, %GU, %GX and bare %G — while the unquoted forms of the same five are correctly caught by the new /%G/ term. This is not forward-compatibility speculation: it reproduces on the installed git today.
Witness:
ORACLE - git 2.43.0, repo with local gpg.program=<marker> and a commit object fabricated
via `git hash-object -t commit` carrying a real gpgsig header:
%G? ran=YES %GG ran=YES %GS ran=YES %GK ran=YES %GF ran=YES %GP ran=YES %GT ran=YES
%GV ran=YES (output "%GV") %GB ran=YES %GU ran=YES %GX ran=YES %G ran=YES
unsigned-commit control: %G? ran=NO
git log --help | grep -o '%G[?A-Z]' -> %G? %GF %GG %GK %GP %GS %GT
(exactly the AST's [?GKFPST] class)
=> git performs the signature check, running the configured gpg.program, and then prints
the unrecognised placeholder literally. An earlier reviewer could not reproduce this
because its fixture had no commit carrying a gpgsig header - the control above shows why.
CLASSIFIER (this commit, repo with local gpg.program; risk.signatureVerifier=false since
neither log.showSignature nor a pretty config is set, so invokesHelper is the only gate):
TOTAL=36 spellings (12 placeholders x unquoted/dquote/squote)
DIVERGENT=10 AST_IS_THE_PERMISSIVE_ARM=10
dquote/squote %GV %GB %GU %GX %G -> AST(dir)=true, classify=read-only, fallbackRO=false
unquoted forms of the same five -> AST(dir)=false (caught by the new /%G/ term)
quoted real placeholders (%G? %GG %GS %GK %GF %GP %GT) -> AST(dir)=false
FLIP (candidate fix /%G[?GKFPST]/ -> /%G/ at shellAstParser.ts:803, applied then reverted):
TOTAL=36 DIVERGENT=0 DIVERGENT_WITH_AST_PERMISSIVE=0
also measured: hasShellPatternExpansion('format:%GV') === true, so the helper would catch
it if the node type let it reach it
Widen the AST-side check at shellAstParser.ts:803 from /%G[?GKFPST]/ to /%G/ so it matches the fallback for stripOuterQuotes-ed arguments, and apply the quote-normalized argument text so fully quoted spellings reach it; then add the quoted spellings to the lockstep case list. This is the %G half of the argument-value normalization recommended on the shellAstParser.ts helper-option finding, so the two fixes should land together rather than separately.
The reason the fix cannot live in the shared predicate: shellAstParser.ts:743-744 reads (['word', 'concatenation'].includes(node.type) && hasShellPatternExpansion(node.text)), so a fully quoted argument is neither node type and no change inside hasShellPatternExpansion can reach it — the fix must live in evaluateGitSafety, which tests options built from stripOuterQuotes-ed argument text. shellReadOnlyChecker.ts:89 and shellAstParser.ts:141-142 must stay in lockstep per the design doc.
Please extend git-shell-execution-safety.test.ts "treats every %G pretty placeholder as signature-verifying" — which today exercises only the unquoted git log --pretty=format:%GB — with git log "--format=%GV" and git log '--format=%GV' asserting classifyShellCommandSafety(...) === 'unknown', and add the same two rows to "keeps AST and fallback Git helper rules in lockstep" (ast: 'unknown', fallback: false). Both go red without the AST-side widening while the existing unquoted case stays green.
中文说明
[Critical] R9-2：[certifies-falsely] 本 diff 把 fallback 分类器的 %G[?GKFPST] 放宽为裸 %G，但 AST 分支在 invokesHelper（shellAstParser.ts:803）中仍保留窄字符类，而 hasShellPatternExpansion 中新增的 /%G/ 项只作用于 word 和 concatenation 节点 —— 完全被引号包裹的参数是 string/raw_string 节点，永远不会被模式检查。因此两个分支在带引号的 %G 格式上产生分歧，而主分支（AST）恰恰是宽松的那一个。这与本 diff 自己新增的设计文档表述相矛盾：那句话要求 AST 与 fallback 的辅助选项规则在 %G* 用例上保持同步。
在本地配置把 gpg.program 设为恶意命令的仓库中，一个完全带引号的 %G pretty 格式会在没有任何确认对话框的情况下被自动批准，而 git 会执行那个程序。实测 36 种写法中有 10 种产生分歧，并且这 10 种全部是 AST 分支宽松 —— 即 %GV、%GB、%GU、%GX 和裸 %G 的双引号与单引号形式 —— 而同这五种的不带引号形式已被新的 /%G/ 项正确拦截。这不是面向未来兼容性的推测：它在当前安装的 git 上就能复现。
关键证据是：git 会执行签名校验（从而运行配置的 gpg.program），然后把无法识别的占位符原样打印出来。此前一位评审无法复现，是因为其夹具中没有携带 gpgsig 头的提交 —— 上面的对照组正好说明了原因。
修复方式是把 shellAstParser.ts:803 处 AST 侧的检查从 /%G[?GKFPST]/ 放宽为 /%G/，使其对经过 stripOuterQuotes 的参数与 fallback 保持一致，并应用引号归一化后的参数文本以便完全带引号的写法也能被覆盖；然后把这些带引号写法加入同步用例列表。这是 shellAstParser.ts 辅助选项那条建议中"参数值归一化"的 %G 一半，所以两个修复应当一起落地，而不是分开。
修复不能放在共享谓词里的原因是：shellAstParser.ts:743-744 是 (['word', 'concatenation'].includes(node.type) && hasShellPatternExpansion(node.text))，完全带引号的参数两种节点类型都不是，所以在 hasShellPatternExpansion 内部做任何改动都触达不到它 —— 修复必须落在 evaluateGitSafety 中，那里检测的是由 stripOuterQuotes 后的参数文本构成的 options。按设计文档要求，shellReadOnlyChecker.ts:89 与 shellAstParser.ts:141-142 必须保持同步。
请扩展 git-shell-execution-safety.test.ts 的 "treats every %G pretty placeholder as signature-verifying"（今天只覆盖不带引号的 git log --pretty=format:%GB），加入 git log "--format=%GV" 和 git log '--format=%GV' 并断言 classifyShellCommandSafety(...) === 'unknown'，同时把这两行加入 "keeps AST and fallback Git helper rules in lockstep"（ast: 'unknown'、fallback: false）。不做 AST 侧放宽时这两条都会变红，而既有的不带引号用例保持绿。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| it('gates command-form fsmonitor with a default-deny consumer model', async () => { | ||
| const cwd = createRepo(); | ||
| config(cwd, 'core.fsmonitor', '/tmp/fsmonitor-helper'); |
There was a problem hiding this comment.
[Critical] R9-3: [certifies-falsely] [new-surface] The repository-local execution-risk model is scoped to exactly one repository: both probes spawn with -C <cwd> and nothing enumerates submodules. But git status and git diff in a superproject enter each initialized submodule to compute its dirty state, and execute helpers configured in the submodule's own local config — which the probe never reads. No test in git-config-safety.test.ts, git-shell-execution-safety.test.ts or shellAstParser.test.ts covers submodules.
A repository that ships an initialized submodule whose .git/modules/<name>/config sets core.fsmonitor (or core.hooksPath plus an executable post-index-change). The superproject probe finds nothing, every risk bit reads false, git status and git diff classify read-only, and shell.ts:2077 returns 'allow' with no confirmation dialog — while git enters the submodule and runs the repository-controlled program. .git/modules/<name>/config is the same attacker-controlled file class this PR's own threat model names: it ships inside the repo tree of any archive or image that includes .git, and an embedded submodule checkout carries it at <path>/.git/config.
Witness:
FIXTURE=/tmp/vfy-subm-1/super (submodule .git/modules/subm/config: core.fsmonitor=<marker>)
RISK={"diffExternal":false,"diffDriverCommand":false,"diffDriverTextconv":false,"fsmonitor":false,
"worktreeFilter":false,"pager":false,"signatureVerifier":false,"promisorRemote":false,
"mergeDriver":false,"alternateRefsCommand":false,"hooksPath":false}
SPAWN_CALLS_FOR_ONE_PROBE=2
["git",["-C","/tmp/vfy-subm-1/super","rev-parse","--git-path","hooks"]]
["git",["-C","/tmp/vfy-subm-1/super","config","--includes","--show-scope","--null",
"--get-regexp","^diff\\.external$|...|^core\\.hookspath$"]]
READONLY[git status]=true READONLY[git diff]=true
MARKER_FIRED_AFTER_CLASSIFIED_READONLY_GIT_STATUS=YES
MARKER_LINE=MARKER arg0=/tmp/vfy-subm-1/marker.sh arg1=2 cwd=/tmp/vfy-subm-1/super/subm
FIXTURE=/tmp/vfy-subm-2/super (submodule core.hooksPath + post-index-change mode 0755)
RISK={...all false...} READONLY[git status]=true READONLY[git diff]=true
shell: SUBMODULE_POST_INDEX_CHANGE cwd=/tmp/vfy-subm-2/super/subm
repeatability S0..S6 -> hook lines=1 on all 7 superproject `git status --porcelain` runs
(including S0 with nothing touched); 0/2 once the submodule index was already fresh
=> state-dependent, not unconditional
CONTROLS: superproject .git/hooks post-index-change count = 0 in both fixtures;
submodule filter.pwn.clean+smudge -> filter marker lines=0 (so the exposure is the
fsmonitor and hooksPath channels, not the filter channel); git log -1 / git show HEAD -> 0
STATIC: grep -rn submodule packages/core/src/utils/git-config-safety.ts -> 0 hits;
the only `submodule` in shellAstParser.ts is line 212, a KNOWN_SUBCOMMANDS entry used
for rule extraction, not risk
Make the probe follow the repositories git will actually enter: after resolving risk for cwd, enumerate initialized submodules (one git -C <cwd> submodule--helper list, or .gitmodules plus .git/modules/*) and OR in the same risk evaluation for each submodule worktree. If that cost is unacceptable, fail closed the way shellAstParser.ts:1153-1154 already does for cd — treat the presence of any initialized submodule as a risk for the subcommands that recurse into them (status, diff).
The cost constraint to design against: git-config-safety.ts:126-135 and :165-182 spawn git with timeout: 1000 / maxBuffer: 64 * 1024, and getLocalGitConfigRisk(cwd) is called uncached from shellAstParser.ts:1159 and :1206 on every read-only git classification — measured, 10 consecutive classifications spawned 20 git processes, and shell.ts:2147-2152 re-classifies each sub-command, so a compound command already pays one probe pair per read-only git sub-command. A recursive per-submodule probe multiplies that by the submodule count on every agent-issued git status/git diff, so bound it (one submodule--helper list plus one config read, or a short-lived per-cwd memo shared within a single permission decision) rather than fanning out a probe pair per submodule per classification. Any sharing must stay keyed on the exact caller-supplied cwd, and the description's rejection of a cross-invocation TTL cache still applies across decisions.
Please add a case beside "gates command-form fsmonitor with a default-deny consumer model": build a superproject plus submodule with git -c protocol.file.allow=always submodule add <path> <dest> (a recipe already in tree at filesearch/crawler.test.ts:891-903), then git config -f .git/modules/<name>/config core.fsmonitor /tmp/helper and assert isShellCommandReadOnlyASTInDirectory('git status', superCwd) and ('git diff', superCwd) are false, plus the core.hooksPath variant with an executable post-index-change in the submodule. All three return true today, and removing the submodule arm of the fix must red them again.
中文说明
[Critical] R9-3：[certifies-falsely] [new-surface] 仓库本地执行风险模型的作用域恰好只有一个仓库：两个探测都以 -C <cwd> 派生进程，且没有任何地方枚举子模块。但父项目中的 git status 和 git diff 会进入每个已初始化的子模块以计算其脏状态，并执行子模块自身本地配置中设置的辅助程序 —— 而探测从来不读取它。git-config-safety.test.ts、git-shell-execution-safety.test.ts、shellAstParser.test.ts 中都没有任何覆盖子模块的测试。
场景是：一个自带已初始化子模块的仓库，其 .git/modules/<name>/config 设置了 core.fsmonitor（或 core.hooksPath 加上一个可执行的 post-index-change）。父项目的探测什么也找不到，所有风险位都是 false，git status 和 git diff 被分类为只读，shell.ts:2077 在没有确认对话框的情况下返回 'allow' —— 而 git 进入子模块并运行了仓库可控的程序。.git/modules/<name>/config 正是本 PR 自己的威胁模型所指的那一类攻击者可控文件：它会随任何包含 .git 的归档或镜像一起出现在仓库树中，而内嵌的子模块检出会把它放在 <path>/.git/config。
修复方式是让探测跟随 git 真正会进入的那些仓库：在为 cwd 解析风险之后，枚举已初始化的子模块（一次 git -C <cwd> submodule--helper list，或 .gitmodules 加 .git/modules/*），并对每个子模块工作树 OR 进同样的风险评估。如果这个代价不可接受，就按 shellAstParser.ts:1153-1154 对 cd 已有的方式失败关闭 —— 把"存在任何已初始化子模块"本身视为对会递归进入子模块的子命令（status、diff）的风险。
需要针对设计的成本约束：git-config-safety.ts:126-135 与 :165-182 以 timeout: 1000 / maxBuffer: 64 * 1024 派生 git，而 getLocalGitConfigRisk(cwd) 在每一次只读 git 分类时都被 shellAstParser.ts:1159 和 :1206 无缓存地调用 —— 实测 10 次连续分类会派生 20 个 git 进程，并且 shell.ts:2147-2152 会对每个子命令重新分类，所以复合命令本来就已经为每个只读 git 子命令付出一对探测。按子模块递归探测会把这个数字在每次 agent 发起的 git status/git diff 上再乘以子模块数量，因此要给它设界（一次 submodule--helper list 加一次配置读取，或在单次权限决定内共享的、按 cwd 的短期 memo），而不是每个子模块每次分类都扇出一对探测。任何共享都必须以调用方提供的确切 cwd 为键，并且 PR 描述中拒绝跨调用 TTL 缓存的理由在跨决定层面依然适用。
请在 "gates command-form fsmonitor with a default-deny consumer model" 旁边新增用例：用 git -c protocol.file.allow=always submodule add <path> <dest> 构造父项目加子模块（仓库内 filesearch/crawler.test.ts:891-903 已有现成写法），然后执行 git config -f .git/modules/<name>/config core.fsmonitor /tmp/helper，断言 isShellCommandReadOnlyASTInDirectory('git status', superCwd) 与 ('git diff', superCwd) 为 false；再加上 core.hooksPath 变体（子模块内含可执行 post-index-change）。这三条今天都返回 true，并且移除修复中的子模块分支后必须再次变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review at head 6beb0f0bdf6a3b652a018251deb69bd9d5d6033e — verdict: 4 Criticals, 2 Suggestions. Core-gate: yes (packages/core/src/utils/**, tools/**, memory/**). Prod logic ≈ 375 lines (343+/32-).
Critical 1 — a valueless config key makes the probe declare total failure
packages/core/src/utils/git-config-safety.ts, probe parse loop, quoted at this head: if (newline < 0) return PROBE_FAILED;
git config --includes --show-scope --null --get-regexp emits scope, NUL, key, NUL — no newline and no value — for a boolean key written bare, e.g. a [log] section containing only showSignature. I reproduced that framing against a real git config probe.
This PR newly adds patterns for log.showsignature, pager.*, remote.*.promisor and extensions.partialclone, so such keys are now in scope. PROBE_FAILED sets all 11 risk bits and localGitConfigMakesCommandUnsafe fails closed, so every read-only git command loses auto-approval. On the memory-scoped-agent-config.ts forced-YOLO path it becomes a hard deny.
Critical 2 — fail-closed on a non-repo cwd
git-config-safety.ts, hasExecutablePostIndexChangeHook, quoted at head: if (result.status !== 0 || typeof result.stdout !== 'string') return true;
git -C <non-repo> rev-parse --git-path hooks exits 128, so hooksPath: true is reported for any cwd outside a repository — again withdrawing auto-approval from all read-only git commands there.
Critical 3 — hasShellPatternExpansion gains a backslash-escape term and breaks an unmodified test
packages/core/src/utils/shell-safety-rules.ts adds a term matching a backslash followed by any character. packages/core/src/utils/shellAstParser.test.ts:1058-1087 — which this PR does not touch — still expects 'read-only' for a git status command built from a repeated escaped brace; the new term makes it 'unknown'. This matches the red Test (ubuntu-latest, Node 22.x) lane.
The same term also withdraws auto-approval from any command containing a backslash escape across all 10 root families in the rule table (awk, find, git, printf, rg, ripgrep, sed, sort, tree, uniq).
Critical 4 — the hardening leaves a named code-execution bypass open
packages/core/src/utils/shellAstParser.ts:1125, quoted at head:
const FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS = new Set(['branch','cat-file','log','remote','rev-parse','show']);cat-file, show and rev-parse do consult the index when handed an index revision. I measured git cat-file -p :a.c, git show :a.c and git rev-parse :a.c each executing a repo-local core.fsmonitor program. Because the subcommand is on the exemption list, risk.fsMonitor is ignored at :1185 (!FS_MONITOR_AND_FILTER_SAFE_SUBCOMMANDS.has(subcommand)), the command stays read-only, it is auto-approved, and attacker-controlled core.fsmonitor in a cloned repo runs. This is precisely the class the PR exists to close.
Suggestions
- Quoted-
%Gdivergence between the two arms:hasShellExpansiononly pattern-checkswordandconcatenationnodes, sogit log '--format=%GV'(astringnode) never reaches the check, while the ASTinvokesHelperregex is/%G[?GKFPST]/and the deprecated fallback uses a bare%G. The two checkers disagree, which contradicts the design doc's lockstep claim. Not Critical —%GVis not a real placeholder and the reachable%G?/%GKforms are caught unquoted. LEADING_ENV_ASSIGNMENT_RE(shell.ts:2061-2066,memory-scoped-agent-config.ts:306+) is a new gate with no test asserting the'ask'vs'deny'split for an env-prefixed git command.
Git code-execution vector table at this head
| Vector | Closed? | Evidence |
|---|---|---|
diff.external, diff.<d>.command / .textconv |
yes | in pattern list; empty-subsection form handled |
core.fsmonitor (command form) |
no | exempted for cat-file/show/rev-parse + index revision — Critical 4 |
filter.<d>.clean / .process |
yes | in pattern list, fails closed |
filter.<d>.smudge |
n/a | no allowlisted checkout-side consumer found |
core.pager, pager.<cmd> |
yes | explicit fail-closed on risk.pager |
gpg.program + log.showSignature / %G* |
partial | config form closed; quoted %G diverges (Suggestion 1) |
merge.<d>.driver + --remerge-diff |
yes | remerge diff-merges form rejected |
core.alternateRefsCommand |
yes | fail-closed |
core.hooksPath + post-index-change |
yes, over-broad | closed, but fails closed off-repo — Critical 2 |
| partial-clone / promisor remote | yes | extensions.partialclone, remote.*.promisor |
core.sshCommand via git remote show |
yes | ---terminator fix in evaluateGitRemoteArgs |
git -c / -C overrides |
yes | rejected in both arms |
GIT_CONFIG_COUNT / _KEY_n / _VALUE_n env |
yes | verified at all 5 auto-approval consumers (shell.ts, permission-manager.ts:623, monitor.ts:192 via normalizeMonitorCommand, speculationToolGate.ts:110, memory-scoped) |
alias.* |
n/a | git ignores aliases shadowing builtins |
core.editor |
n/a | no reachable allowlisted consumer |
| submodule-local config | cannot tell | not probed |
Pre-existing Criticals
R9-1 (an "Always allow" click persisting a rule that strips leading env assignments, permanently disabling the new gate) — cannot tell; I could not locate the allow-rule normalization path. R9-3 (submodule-local config) — plausible, unverified.
Agent-assisted review. Findings were re-read in the file content at the exact head SHA above before filing. Posting as a comment only — no approval implied.
5 participants
What this PR does
This PR hardens the shell read-only approval boundary for Git commands whose apparently read-only behavior can still execute repository-controlled helpers.
It now covers:
diff.external,diff.<driver>.command, anddiff.<driver>.textconv, including empty-subsection keys such asdiff..command.core.fsmonitorfor read-only worktree/index consumers.filter.<driver>.clean/filter.<driver>.processfor worktree-content consumers.core.pager/pager.<cmd>values while preserving boolean/non-command pager values.gpg.program/gpg.<format>.programwhen signature verification is activated by effectivelog.showSignature,format.pretty, orpretty.<name>configuration.git log -1can lazy-fetch when the referenced commit is missing locally.merge.<driver>.driverand remerge-diff paths, including--diff-merges=remergeand the documented--diff-merges=rspelling.git remote show <name>, which is no longer auto-approved unless-n/--no-queryoccurs before the--terminator and prevents a remote query.GIT_CONFIG_*/ leading-env injection beforestripShellWrapper()can discard assignments, including the memory-scoped/dream-agent shell permission path.The important structural change is that broad Git execution mechanisms use a default-deny consumer model instead of enumerating every dangerous flag spelling. Shared safe-subcommand sets and AST/fallback consistency tests reduce the chance that the two classifiers drift apart.
The regex fallback used when tree-sitter WASM is unavailable also fails closed for every covered repository-config risk. Debug logging records the working directory and active Git config risk bits when the probe finds executable-risk configuration.
Why it's needed
#10193 demonstrated that a command classified as read-only could cross the approval boundary and execute repository-controlled code through Git configuration. Subsequent review rounds reproduced the same defect class through diff/textconv drivers, fsmonitor, clean/process filters, pagers, signature verification, lazy fetch/transport, merge drivers,
git remote show, Git boolean edge cases, abbreviated options, and command-scopeGIT_CONFIG_*injection.The invariant enforced by this PR is therefore broader than a single config key: a Git command must not be silently auto-approved when the selected execution path can invoke a repository-controlled external program.
Reviewer Test Plan
How to verify
diff.pwn.commandand confirmgit diffrequires confirmation while unrelatedgit log/git showremain read-only.diff.pwn.textconvand confirmgit blame,git diff,git log, andgit showconsumers are downgraded.core.fsmonitorand confirm affected consumers such asgit status,git diff,git blame, worktreegit grep,git ls-files, and dirty/brokengit describeare downgraded, while proven non-consumers such asgit log -1,git show HEAD, andgit rev-parse HEADstay read-only.filter.demo.cleanorfilter.demo.processand confirm worktree-content consumers are downgraded, includinggit ls-files -m,--modified, and abbreviated--mod, while the shared proven non-consumers stay read-only.core.pager/pager.logand confirm read-only Git commands are downgraded; boolean values such asfalse,true,0, and1must not be treated as executable pager commands.gpg.programorgpg.ssh.programtogether with signature-verifying config and confirmgit log/git showare downgraded. Verify that a global/effective%G*pretty format also activates protection when the executable GPG program is repository-local.git log -1,git log --max-count=1, pathspec forms, reproduced materializing flag families, andgit ls-files --with-tree.merge.<driver>.driverand verify remerge-capablegit log/git showpaths are not auto-approved,--diff-merges=ris recognized by both classifiers, and non-consumers such asgit diff/git statusremain read-only.git remote show originandgit remote show origin -- -nare not auto-approved, while real no-query forms such asgit remote show -n originremain read-only.bash -c 'git status'can remain allowed in a clean repo, whileGIT_CONFIG_COUNT=... bash -c 'git diff'and raw command-substitution prefixes are denied before wrapper stripping.-1,+1,0x1,01, and1kfail closed as true/risky while explicit zero remains false.Focused regression coverage lives in
packages/core/src/utils/git-config-safety.test.ts,packages/core/src/memory/memory-scoped-agent-config.shell-safety.test.ts, the AST/fallback classifier tests, and the existing shell permission tests.Evidence (Before & After)
Before: review reproduced multiple commands that were classified read-only and could execute repository- or command-controlled helpers without confirmation.
After: the current head contains the round-five hardening as one ordinary commit (
6d34dc0) directly on top of the reviewed80263c5head. The unsafe promisor exemption is removed, Git boolean/effective-pretty semantics are fail-closed, the memory-scoped sibling permission path is protected before wrapper stripping, AST/fallback remerge behavior is aligned, shared consumer exemptions are centralized, and deny/allow/fallback/worktree regression coverage is present.Current-head GitHub Actions are not reporting a test failure: both Qwen Code CI and Security Checks are
action_required, meaning the fork workflows need repository-side approval before jobs can start. No current-head CI jobs have run, so this PR intentionally does not claim a fresh green CI run.Tested on
Risk & Scope
core.fsmonitor=false,core.pager=false, andpager.status=falseare not treated as executable helpers.-1,+1,0x1, and1k.log.showSignature/ pretty formats) is resolved from effective configuration.NO_RISKresult could become fail-open after repository config changes. A future performance refactor can compute the probe once per permission flow and thread the result through without caching safe results across flows.Bash(...)allow rules in the normal permission manager remain a separate authorization layer and can override the defaultaskdecision. This pre-existing rule behavior is documented rather than changed in this security-focused PR.Linked Issues
Closes #10193
中文说明
此 PR 做了什么
此 PR 加固 shell 对 Git “只读命令”的自动批准边界。只要一个看似只读的 Git 操作可能通过仓库控制的配置执行外部程序，就不会静默自动批准。
覆盖范围包括：diff/textconv driver、命令形式
core.fsmonitor、clean/process filter、pager、GPG/签名验证、partial-clone/promisor 懒加载、merge driver/remerge、会真正查询远端的git remote show，以及普通 shell 与 memory/dream scoped shell 路径中的GIT_CONFIG_*/前置环境变量注入。本轮进一步移除了 promisor 仓库中
git log -1的例外：即使看起来只读取一个提交，当本地缺少该提交对象时 Git 仍可能触发 lazy fetch，因此 promisor 风险下所有原本只读的 Git 命令都 fail closed。Git 布尔值处理也覆盖+1、0x1、1k等 Git 接受的真值形式；签名激活侧使用 effective pretty 配置，而可执行的 GPG program 仍只把 local/worktree 视为仓库控制风险。AST 与 regex fallback 都识别
--diff-merges=remerge和--diff-merges=r，并通过共享回归用例保持一致。fsmonitor/filter 的安全非消费者列表集中维护，避免两份列表漂移。为什么需要此改动
#10193 以及后续 review 已复现多条“只读分类 → 无确认执行外部程序”的路径。本 PR 强制的安全不变量是：只要当前 Git 执行路径可能调用仓库控制的外部程序，就不能静默自动批准。
Reviewer Test Plan
如何验证
按英文部分的 12 个用例验证 diff/textconv、fsmonitor、filter、pager、GPG/effective pretty、promisor lazy fetch、merge driver、
git remote show、parser fallback、worktree scope 以及 memory-scoped raw env 注入。风险路径应要求确认或拒绝；明确证明安全的控制组继续保持只读。Evidence (Before & After)
当前 Round-5 修复已压成一个普通提交
6d34dc0，直接位于被 review 的80263c5之上。当前 HEAD 的 Qwen Code CI 与 Security Checks 仍为action_required，即 fork workflow 需仓库侧批准后才能真正启动；因此这里不声称当前 HEAD 已通过 CI。Tested on
Risk & Scope
NO_RISK结果造成 fail-open。Bash(...)allow rule 仍属于独立授权层；本 PR 不改变已有用户授权语义。关联 Issue
Closes #10193