feat(core): let plan mode vouch for extra read-only shell roots - #9948
feat(core): let plan mode vouch for extra read-only shell roots#9948TianYuan1024 wants to merge 2 commits into
Conversation
…LM#9694) Plan mode judges a shell command by matching its root against a hardcoded read-only set. A binary outside that set cannot be judged, so it classifies `unknown` and prompts — and plan-mode shell confirmations hide "Always allow" and accept `ProceedOnce` only, so the prompt returns on every invocation, forever. A team whose plan-mode sessions run through a project-specific read-only CLI clicks through every single read while `cat` and `git status` pass silently. Neither `permissions.allow` nor a `PreToolUse` hook helps: plan mode overrides the former for shell, and the latter runs after the decision. `permissions.planMode.extraReadOnlyCommands` names root commands the user vouches for. The entry is consulted at the very end of the dispatch chain, after every root the classifier already understands, so it can only add to the read-only set — listing `rm` or `git` leaves `rm -rf build` and `git push` classified exactly as before. Redirections, substitutions, env-assignment prefixes and pipes into unknown commands are untouched. What bounds the vouch is not the name but the shape of the invocation: - A refusal floor of 183 roots the vouch can never cover — interpreters, launchers, build and package tools, and builtins that rebind name resolution. Their payload is a code string, a recipe, or a downloaded package, so no argument inspection can see it. Companion regex matches versioned spellings by family (`python3.12`, `gcc-13`, `luajit-2.1.0-beta3`) rather than release by release. This is a floor under foreseeable mistakes, not a boundary: the list cannot be closed by enumeration, which is why the next item matters more. - The vouch is honoured only for an invocation the classifier can read literally — every argument a plain literal word naming no known command, so a launcher nobody enumerated cannot smuggle one through. - The setting is taken from user, system and system-default scopes only. A cloned repository cannot vouch for itself, which is what turns the floor from an adversarial boundary into a guard against user error. - A vouched root is treated as a possible git frontend: when its first non-flag argument is a git verb the invocation is screened by git's own evaluator, and it inherits git's planted-config gate for repositories that set `diff.external`, `core.fsmonitor`, a textconv driver, a clean/smudge filter, `gpg.program`, or a `!` shell alias. Scoped to plan mode through one accessor that returns an empty set in every other approval mode, so a vouch made while planning never widens auto-approval elsewhere. Dropped in `--bare` and safe mode. An entry vouches for the whole binary. Qwen Code cannot see inside a custom CLI, so a mutating sub-command is silenced too; that tradeoff is documented. Two defects found while building this are fixed here because the vouch turns each from a prompt into an unattended run, and both also affect built-in roots today: - Statements nested inside a heredoc redirect were dropped from the analysis. tree-sitter parses whatever follows the opener on the same line inside the redirect node, and the `redirected_statement` arm filtered every redirect child out — so `cat <<EOF && for ((i=0;i<1;i++)); do rm -rf build; done` classified `read-only` with no vouch involved. Now a skip-list of inert redirect leaves; everything else is evaluated, with unknown shapes floored at `unknown`. - The confirmation dialog classified each sub-command against the original cwd, so `cd /hostile && git status && curl x` dropped `git status` from the scope the user approved and then ran it in the planted repository. Both call sites now stop dropping sub-commands once one has planted state. Tests: 2,194 in the six affected core suites, 625 in the four cli config suites. `shellAstParser.test.ts` carries 881, including a two-way ratchet on the refusal floor — a deleted entry fails containment, an undeclared addition fails the count. Closes QwenLM#9694
|
Thanks for the PR!
Moving on to code review — with the standing caveat that the verdict caps at defer-to-maintainer for the size/security-surface reasons above. 🔍 中文说明感谢贡献！
进入代码审查 —— 但保留一个前提：由于上述规模/安全面原因，结论上限为"移交维护者"。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this problem was the minimal version of what ships here: a I walked the security surface adversarially and found no blocking issues. What I verified, with the pinning tests:
Findings (none blocking)
sequenceDiagram
participant S as Settings scopes
participant C as Config
participant T as Shell and Monitor tools
participant P as Plan mode policy
participant A as AST classifier
participant G as Git config probe
S->>C: merge planMode, workspace value stripped
C->>C: normalize entries to bare lowercase names
P->>C: getPlanModeReadOnlyRoots
C-->>P: set, empty outside Plan Mode
T->>A: classify with extra roots via same accessor
P->>A: classify with extra roots
A->>A: built-in roots dispatch first, vouch is terminal
A->>G: probe repo-local git config for vouched roots
G-->>A: program-executing keys present or not
A-->>P: read-only, write or unknown
Files changed (25 of 25 shown)
Test evidence (PR's own CI, fetched via API — PR code never executed here)At fetch time the main unit suite had not finished; per workflow this is a single fetch with no polling, and the The two red checks are classified as pre-existing infra noise, not PR-caused:
Not verified in this run: live product behavior (a real Plan Mode session where Sandboxed verification would settle this: 中文说明代码审查我对这个问题的独立方案是本 PR 落地方案的最小版本：一个校验为裸命令名的 我对安全面做了对抗性走查，未发现阻断问题。已验证（均有对应测试钉住）：
发现（均非阻断）
测试证据（来自 PR 自身 CI，经 API 获取 —— 本审查未执行任何 PR 代码）获取时主单元测试尚未完成；按流程只取一次快照、不轮询， 两个红色检查判定为既有基础设施噪音、非本 PR 造成： 本次运行未验证：真实产品行为（ 沙箱验证可以定论： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean code review with no blocking findings, but the Stage 0 size escalation on security-critical core surface caps this run at defer-to-maintainer; the cap is policy, not doubt about the code. Stepping back: this is a rare fork PR where the engineering discipline exceeds the bar most in-house work meets. The vouch is layered honestly — user-only scope as the load-bearing layer, shape guards as the readable layer, the refusal floor as a floor rather than a boundary, and the author says so in writing instead of overselling. The two-way ratchet on the refusal floor, the sibling-pair edge tests, the mutation-tested claim — that's the work of someone who expected to be attacked on this surface and pre-answered the attacks. I probed the same surface independently ( My honest reservations are about scope and process, not correctness:
If this were a 200-line PR with the same test discipline, I would have approved it. It isn't, so I'm handing it over: 834 production lines in the shell permission classifier, on a high-risk path, from a fork, is a maintainer decision by this repo's own rules regardless of how clean the review reads. 中文说明置信度：3/5 —— 代码审查干净、无阻断性发现，但 Stage 0 对安全关键核心面的规模升级将本次运行的上限锁定为"移交维护者"；该上限是政策性的，并非对代码质量的怀疑。 退一步看：这是一个少见的、工程纪律超过多数内部提交水准的 fork PR。担保机制的分层是诚实的 —— 用户专属作用域作为承重层，形状守卫作为可读层，拒绝底线作为"下限"而非"边界"，作者白纸黑字地如此表述，没有夸大。拒绝底线的双向棘轮、同族配对边界测试、经过变异验证的断言 —— 这是预期到该安全面会被攻击、并预先回应了攻击的工作。我独立探测了同一表面（ 我诚实的保留意见在范围与流程，而非正确性：
如果这是一个具备同样测试纪律的 200 行 PR，我会直接批准。它不是，所以我将其移交：834 行生产代码落在 shell 权限分类器这一高风险路径上、来自 fork，按本仓库自身的规则，无论审查读起来多么干净，这都是维护者的决定。 — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @wenshao (cc @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC, Why this run is not approving, despite a clean review:
Open questions for the maintainer, from the review:
The full reasoning is in the Stage 1 gate and Stage 2 review comments in this thread. 中文说明⏸️ 移交 @wenshao（抄送 @tanzhenxin @yiliang114 @LaZzyMan @doudouOUC，按 CODEOWNERS 为 本次运行不予批准的原因（尽管审查干净）：
留给维护者的开放问题（来自审查）：
完整推理见本线程的 Stage 1 门禁与 Stage 2 审查评论。 — Qwen Code · qwen3.8-max Reviewed at |
2 participants
What this PR does
Adds a setting that lets you tell Plan Mode which extra root commands are read-only, so a project-specific CLI stops triggering an approval prompt on every single read.
{ "permissions": { "planMode": { "extraReadOnlyCommands": ["ib"] } } }A listed root joins the classifier's built-in read-only set. The entry is consulted at the very end of the dispatch chain, after every root the classifier already understands has been matched, so it can only ever add to the read-only set — listing
rm,git, orteeleavesrm -rf build,git push, andtee out.txtclassified exactly as before. Redirections, command substitution, environment-assignment prefixes, and pipes into unknown commands are untouched: withiblisted,ib listruns silently whileib list > out.txtis still blocked as state-modifying andib list $(whoami)still prompts.What bounds the vouch
The interesting question is not "which names are allowed" but "what stops a vouch from laundering a write". Four layers, in decreasing order of how much weight they carry:
1. Only the user can vouch. The setting is read from user, system, and system-default scopes only; a workspace
.qwen/settings.jsonis stripped during the merge and a startup warning names the key. This is the load-bearing one. A cloned repository cannot vouch for itself, which means the lists below guard against user error rather than against an adversary who picks the entry.2. The invocation has to be one the classifier can read. A vouch says "this binary only reads"; it can never say "and so does whatever I pass it". So the vouch is honoured only when every argument is a plain literal word that names no command Qwen Code knows.
ib exec rm -rf buildprompts even thoughibis vouched andib execis not otherwise special — the refusal is on shape, so a launcher nobody enumerated cannot use the vouch to smuggle a known command past the analysis.3. A refusal floor of 183 roots. Shell and language interpreters, launchers, build and package tools, and builtins that rebind name resolution can never be vouched. Their payload is a code string, a Makefile recipe, or a package downloaded mid-command — never argv — so no argument inspection can see it. A companion regex matches versioned spellings by family (
python3.12,gcc-13,luajit-2.1.0-beta3,go1.22) rather than release by release.This list is a floor under foreseeable mistakes, not a boundary, and I want to be explicit about that rather than imply otherwise: it cannot be closed by enumeration.
uv run evil.pyand a custom CLI'sib get ./report.jsonare structurally identical, so no classifier can tell a user who vouched a payload-executor from one who vouched their own read-only tool. Layer 1 is what makes that acceptable — the wrong assertion is the user's own, in their own settings file.4. Git gets special handling, because a vouched wrapper of
gitis a case this setting explicitly supports. When a vouched root's first non-flag argument is a git verb, the whole invocation is screened by git's own evaluator — write verbs,branch -D,--output, the%G…signature formats. A vouched root also inherits git's planted-config gate, extended for the wrapper path to every repository-local key that makes a read verb execute a program:diff.external,core.fsmonitor, atextconvdriver, a clean/smudge filter,gpg.program, and!-prefixed shell aliases. Repositories that plant none of these — the ordinary case — are unaffected.Scope
The setting applies only in Plan Mode, read through one accessor that returns an empty set in every other approval mode, so vouching for a CLI while planning never widens auto-approval in default, auto-edit, auto, or yolo mode. Entries are dropped in
--bareand safe mode, matchingpermissions.autoMode.An entry vouches for the entire binary. Qwen Code cannot see inside a custom CLI, so if it has mutating sub-commands, listing it silences the prompt for those too. That tradeoff is documented.
Two fixes that are not about this setting
Both affect built-in roots today; they are here because the vouch turns each from a prompt into an unattended run.
redirected_statementarm filtered every redirect child out before evaluation.cat <<EOF && for ((i=0;i<1;i++)); do rm -rf build; doneclassifiedread-onlywith no vouch involved. Now a skip-list of inert redirect leaves, with unrecognised shapes floored atunknownso an unanticipated one prompts instead of vanishing.cd /hostile && git status && curl xdroppedgit statusfrom the scope the user approved, then ran it in the planted repository. Both call sites now stop dropping sub-commands once an earlier one has planted state (cd,export, …), mirroringPermissionManager.evaluateCompoundCommand.Why it's needed
Plan Mode decides whether a shell command is read-only by matching its root against a hardcoded set. A binary outside that set cannot be judged, so it classifies as unknown and triggers the "could not determine whether this shell command is read-only" prompt. Plan-mode shell confirmations deliberately hide "Always allow" and accept a one-time approval only, so that prompt reappears for every invocation, forever.
For a team whose Plan Mode sessions run through a project-specific read-only CLI, every read needs a manual click while the built-in equivalents (
cat,grep,git status) pass silently. There is no way out today: Plan Mode intentionally overridespermissions.allowfor shell, andPreToolUsehooks run after the permission decision and can only deny or ask. APermissionRequesthook can suppress the prompt, but only by writing a hook that re-implements the classification.Reviewer Test Plan
How to verify
The full scripted plan is committed at
.qwen/e2e-tests/2026-08-22-plan-mode-extra-read-only-commands.md. It uses a scratchQWEN_HOMEso the vouch never touches your real settings, and notes the/planstep every restart needs — approval mode is session state, so a post-restart case run without it silently exercises the default mode instead.Create a scratch workspace with a fake read-only CLI on
PATH(printf '#!/bin/sh\necho ok\n' > ib && chmod +x ib), putpermissions.planMode.extraReadOnlyCommands: ["ib"]in$QWEN_HOME/settings.json, and enter Plan Mode with/plan.Ask the model to run
ib domain list: it should run with no confirmation prompt. Remove the key and repeat — the prompt appears, and appears again on every identical invocation.Confirm the guardrails hold.
ib domain list > out.txtmust be rejected as state-modifying, not prompted.ib domain list $(whoami)andIB_TOKEN=x ib domain listmust still prompt.ib domain list | badcmdmust still prompt, whileib domain list | wc -lruns silently.Confirm the safety net cannot be switched off from settings. Add
"bash","rm","git","make", and"uv"and restart:bash -c 'echo hi',make, anduv run x.pymust still prompt;rm -rf tmpandgit push origin mainmust still be blocked.Confirm a workspace cannot vouch for itself: move the settings file into the repository's own
.qwen/, restart, and the prompt returns with a startup warning namingpermissions.planMode.Confirm the scope:
/approval-mode default, thenib domain list— the normal shell confirmation must appear./planagain and it stops prompting, with no restart.Finally, confirm invalid entries are ignored rather than fatal: set the list to
["", " ", "ib list", "/usr/local/bin/ib", "ib;rm", "IB"]and restart. The CLI starts normally andib domain listruns without a prompt from the"IB"entry alone.Evidence (Before & After)
N/A — no TUI change. The user-visible difference is the absence of a confirmation prompt, covered by the steps above and by unit tests.
shellAstParser.test.tscarries 881 of those. The refusal floor is pinned entry by entry with a two-way ratchet — a deleted entry fails containment, an undeclared addition fails the count — verified with a mutant that drops one name and fails the suite.Tested on
Risk & Scope
cat <<EOF && …shapes that previously classifiedread-onlynow classifywriteorunknown, and confirmation dialogs after acd/exportlist more sub-commands than before.ib add,ib tag); one that spells its config flag-cor-C; and one whose argument names a command the classifier knows (ib exec watch). All three are documented.permissions.allowfor unknown-classified shell commands in Plan Mode (changes Plan Mode's trust model). Sub-command scoping. The deprecated regex fallback used when tree-sitter is unavailable is left alone deliberately — it ignores the setting and keeps prompting, which fails closed. ExtendinggetLocalGitConfigRisk's new key set to literalgitis also left out:git lfs install --localwritesfilter.lfs.clean, so that would downgradegit diffin a large share of real checkouts and wants its own PR. A test pins the git-lfs case so this cannot drift.Linked Issues
Closes #9694