-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(core): let plan mode vouch for extra read-only shell roots #9735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
TianYuan1024
wants to merge
16
commits into
QwenLM:main
from
TianYuan1024:feat/plan-mode-extra-readonly-commands
Closed
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
229aff5
feat(core): let plan mode vouch for extra read-only shell roots (#9694)
TianYuan1024 13c2f12
test: add the plan-mode read-only roots accessor to two Config doubles
TianYuan1024 a47196c
feat(core): widen the non-vouchable root denylist for plan mode
TianYuan1024 d629c2e
Merge branch 'main' into feat/plan-mode-extra-readonly-commands
wenshao eed50f9
fix(core): close the launcher, state-planter, and expansion holes in …
TianYuan1024 38bd5d4
docs: make the plan-mode settings snippets copy-pasteable
TianYuan1024 5c7f571
fix(core): bound the vouch by invocation shape instead of by name lists
TianYuan1024 9435437
fix(cli): take the plan-mode vouch only from scopes the user maintains
TianYuan1024 16b4a17
fix(core): stop the vouch refusing the reads it exists to allow
TianYuan1024 10f5d0c
Merge branch 'main' into feat/plan-mode-extra-readonly-commands
TianYuan1024 169ed51
chore(ci): record cd-cua-driver.yml's shipped size in the workflow si…
TianYuan1024 74a4bbb
fix(core): give a vouched git frontend git's own planted-config gate
TianYuan1024 64e9fd8
Merge remote-tracking branch 'origin/main' into feat/plan-mode-extra-…
TianYuan1024 a085ee7
fix(core): stop a vouched wrapper from carrying git's redirecting opt…
TianYuan1024 09a3a53
fix(core): screen a vouched git frontend through git's own evaluator
TianYuan1024 8bcfc52
Merge remote-tracking branch 'origin/main' into feat/plan-mode-extra-…
TianYuan1024 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
139 changes: 139 additions & 0 deletions
139
.qwen/e2e-tests/2026-08-22-plan-mode-extra-read-only-commands.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| # E2E test plan: Plan mode extra read-only commands (issue #9694) | ||
|
|
||
| Verifies `permissions.planMode.extraReadOnlyCommands` — a user-extensible set of | ||
| read-only root commands for Plan mode. | ||
|
|
||
| ## Setup | ||
|
|
||
| Create a scratch workspace with a fake read-only CLI on `PATH`: | ||
|
|
||
| ```bash | ||
| mkdir -p /tmp/ib-e2e/bin /tmp/ib-e2e/.qwen | ||
| printf '#!/bin/sh\necho ok\n' > /tmp/ib-e2e/bin/ib | ||
| chmod +x /tmp/ib-e2e/bin/ib | ||
| export PATH="/tmp/ib-e2e/bin:$PATH" | ||
| cd /tmp/ib-e2e | ||
| ``` | ||
|
|
||
| `/tmp/ib-e2e/.qwen/settings.json`: | ||
|
|
||
| ```json | ||
| { | ||
| "permissions": { | ||
| "planMode": { | ||
| "extraReadOnlyCommands": ["ib"] | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Launch from `/tmp/ib-e2e`, not from the repo.** Workspace settings resolve | ||
| from the process cwd with no upward search, so a CLI started in the repo root | ||
| never loads the scratch workspace's `.qwen/settings.json` and every case below | ||
| silently behaves as if the vouch were absent. `npm run dev` runs the CLI with | ||
| cwd set to the package root, so it cannot be used here. Use either: | ||
|
|
||
| ```bash | ||
| node /path/to/qwen-code/scripts/dev.js # derives repo paths from its own location | ||
| # or, after `npm run build && npm run bundle` in the repo: | ||
| node /path/to/qwen-code/bundle/qwen.js | ||
| ``` | ||
|
|
||
| Dry-run the baseline against the globally installed `qwen` first — the setting | ||
| does not exist there, so expect the unknown-read cases to prompt and the | ||
| state-modifying cases to be blocked, exactly as they are after the change. | ||
| Only cases 1, 5b, 7, 8, and 9 change behavior with the setting present. | ||
|
TianYuan1024 marked this conversation as resolved.
Outdated
|
||
|
|
||
| ## Cases | ||
|
|
||
| ### 1. The vouched root stops prompting | ||
|
|
||
| - Enter Plan mode (`/plan`). | ||
| - Ask the model to run `ib domain list`. | ||
| - **Expect**: the command runs with no confirmation prompt, and Plan mode stays | ||
| active. | ||
| - **Before the change / with the settings key removed**: the "Plan mode could | ||
| not determine whether this shell command is read-only" prompt appears, and | ||
| appears again on every subsequent identical invocation. | ||
|
|
||
| ### 2. Redirection is still blocked | ||
|
|
||
| - Still in Plan mode, ask for `ib domain list > out.txt`. | ||
| - **Expect**: rejected with the plan-mode write-block message ("classified as | ||
| state-modifying"). No prompt, no file created. | ||
|
|
||
| ### 3. Command substitution still prompts | ||
|
|
||
| - Ask for `ib domain list $(whoami)`. | ||
| - **Expect**: the one-time `unknown` confirmation prompt, with "Always allow" | ||
| hidden. | ||
|
|
||
| ### 4. Environment-assignment prefix still prompts | ||
|
|
||
| - Ask for `IB_TOKEN=x ib domain list`. | ||
| - **Expect**: the one-time `unknown` confirmation prompt. | ||
|
|
||
| ### 5. A pipe into an unknown command still prompts | ||
|
|
||
| - Ask for `ib domain list | badcmd`. | ||
| - **Expect**: the one-time `unknown` confirmation prompt. | ||
| - Ask for `ib domain list | wc -l`. | ||
| - **Expect**: runs without a prompt (`wc` is a built-in read-only root). | ||
|
|
||
| ### 6. The safety net cannot be switched off from settings | ||
|
|
||
| Add `"bash"`, `"time"`, `"hash"`, and `"rm"` to `extraReadOnlyCommands` and | ||
| restart. | ||
|
|
||
| - Ask for `bash -c 'echo hi'`. | ||
| **Expect**: still prompts — the classifier refuses to let any caller vouch a | ||
| shell interpreter. | ||
| - Ask for `time rm -rf tmp`. | ||
| **Expect**: still prompts — `time` is a launcher, so vouching it is not a | ||
| vouch for what it wraps. | ||
| - Ask for `hash -p ./bin/git git && git status`. | ||
| **Expect**: still prompts — `hash` re-binds how the later `git` resolves. | ||
| - Ask for `rm -rf tmp`. | ||
| **Expect**: still blocked as state-modifying — `rm` keeps its built-in write | ||
| classification. | ||
| - Ask for `git push origin main` with `"git"` also listed. | ||
|
TianYuan1024 marked this conversation as resolved.
Outdated
|
||
| **Expect**: still blocked as state-modifying. | ||
|
|
||
| ### 6b. An unrecognised launcher fails closed too | ||
|
|
||
| With only `"ib"` vouched, ask for `ib exec rm -rf tmp`. | ||
|
|
||
| - **Expect**: prompts. A vouched root that is handed a command the classifier | ||
| recognises (`rm`) is refused on shape, without `ib` needing to be known as a | ||
| launcher. This is what keeps the guarantee from depending on an exhaustive | ||
| list of launcher names. | ||
|
|
||
| ### 7. The vouch is scoped to Plan mode | ||
|
|
||
| - Leave Plan mode: `/approval-mode default`. | ||
| - Ask for `ib domain list`. | ||
| - **Expect**: the normal shell confirmation prompt appears. The Plan-mode vouch | ||
| must not auto-approve here. | ||
| - Switch back to Plan mode (`/plan`) and repeat case 1 — it stops prompting | ||
| again, without a restart. | ||
|
|
||
| ### 8. Monitor tool parity | ||
|
|
||
| - In Plan mode, ask the model to start a monitor on `ib domain watch`. | ||
|
TianYuan1024 marked this conversation as resolved.
Outdated
|
||
| - **Expect**: no confirmation prompt (the monitor tool shares the plan-mode | ||
| shell policy). | ||
|
|
||
| ### 9. Invalid entries are ignored, not fatal | ||
|
|
||
| Set `extraReadOnlyCommands` to | ||
| `["", " ", "ib list", "/usr/local/bin/ib", "ib;rm", "IB"]` and restart. | ||
|
|
||
| - **Expect**: the CLI starts normally. `ib domain list` runs without a prompt | ||
| (from the `"IB"` entry, which normalizes to `ib`); the malformed entries are | ||
| silently dropped. | ||
|
|
||
| ## Cleanup | ||
|
|
||
| ```bash | ||
| rm -rf /tmp/ib-e2e | ||
| ``` | ||
183 changes: 183 additions & 0 deletions
183
docs/design/2026-08-22-plan-mode-extra-read-only-commands.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| --- | ||
| title: 'Plan Mode Configurable Read-Only Root Commands' | ||
| date: '2026-08-22' | ||
| status: 'implemented' | ||
| --- | ||
|
|
||
| # Plan Mode Configurable Read-Only Root Commands | ||
|
|
||
| ## Problem | ||
|
|
||
| The shell AST classifier recognises a hardcoded set of read-only root commands | ||
| (`READ_ONLY_ROOT_COMMANDS` in `packages/core/src/utils/shellAstParser.ts`). Any | ||
| other binary classifies as `unknown`, which in Plan mode forces the "could not | ||
| determine whether this shell command is read-only" confirmation. Plan-mode shell | ||
| confirmations set `hideAlwaysAllow` and accept only `ProceedOnce`, so the | ||
| approval never sticks — it fires again for every exact invocation. | ||
|
|
||
| Teams that drive Plan mode sessions through a project-specific read-only CLI are | ||
| therefore prompted on every read, while the built-in equivalents (`cat`, `grep`, | ||
| `git status`) pass silently. There is no configuration escape hatch today: | ||
|
TianYuan1024 marked this conversation as resolved.
Outdated
|
||
|
|
||
| - no setting extends the root set; | ||
| - Plan mode deliberately overrides `permissions.allow` for shell, so an allow | ||
| rule does not help (`planShellRequiresConfirmation` in `coreToolScheduler`); | ||
| - `PreToolUse` hooks run after the permission decision and can only deny or ask. | ||
|
|
||
| Reported as issue #9694. | ||
|
|
||
| ## Goals | ||
|
|
||
| - Let a user extend the classifier's _known-safe root set_ from settings. | ||
| - Keep every other Plan mode guarantee byte-for-byte identical. | ||
| - Confine the vouch to Plan mode. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - Honouring `permissions.allow` for `unknown`-classified shell commands in Plan | ||
| mode. That changes Plan mode's trust model and needs its own design. | ||
| - Sub-command scoping (a `READ_ONLY_GIT_SUBCOMMANDS` analogue for arbitrary | ||
| CLIs). Custom CLIs place their verb at varying argument positions, so a | ||
| first-argument table would not generalise. | ||
| - Threading the setting into the deprecated regex checker | ||
| (`shellReadOnlyChecker.ts`), the synchronous concurrency-batching check, the | ||
| speculation gate, or memory-scoped agent config. | ||
|
|
||
| ## Design | ||
|
|
||
| ### The single behavioural hook | ||
|
|
||
| `evaluateCommandSafety` dispatches on the root command through an ordered | ||
| if/else chain, ending in: | ||
|
|
||
| ```ts | ||
| result = | ||
| READ_ONLY_ROOT_COMMANDS.has(root) || extra.has(root) | ||
| ? 'read-only' | ||
| : 'unknown'; | ||
| ``` | ||
|
|
||
| Every root the classifier understands specially — `WRITE_ROOT_COMMAND`, the | ||
| `git`/`find`/`sed`/`awk`/`sort`/`tree`/`uniq`/`tee`/`dd` handlers, the `kill` | ||
| family — is matched _before_ this terminal branch. That ordering is the safety | ||
| property: a caller-supplied root can only ever add to the read-only set, never | ||
| override a built-in write classification. `rm`, `git push`, and `tee out.txt` | ||
| stay `write` even when listed. | ||
|
|
||
| Post-processing after the chain is untouched, so redirections, command and | ||
| process substitution, and environment-assignment prefixes still merge a vouched | ||
| root up to `unknown`/`write`. | ||
|
|
||
| ### Threading, not global state | ||
|
|
||
| `ShellSafetyOptions { extraReadOnlyRoots?: ReadonlySet<string> }` is an optional | ||
| trailing parameter on the four public entry points, threaded as a required | ||
| argument through the mutually recursive evaluators (`evaluateStatementSafety`, | ||
| `evaluateCommandSafety`, `evaluateSubstitutions`, `evaluateRedirectionSafety`, | ||
| `childrenSafety`, `classifyInternal`). | ||
|
|
||
| A module-level mutable registry was rejected: `qwen serve` runs multiple | ||
| workspace `Config` instances in one process, so a process-global would leak one | ||
| workspace's vouch into another. | ||
|
|
||
| ### Normalisation and the mode gate | ||
|
|
||
| `Config` owns both. `normalizePlanModeReadOnlyRoots` trims, lowercases, and | ||
| drops: | ||
|
|
||
| - anything that is not a bare command name (the classifier matches the | ||
| lowercased root token, so a path or an argument string could never match); | ||
| - anything that is not an array of strings — settings are merged per key with | ||
| no type validation, so a hand-written `"extraReadOnlyCommands": "mycli"` | ||
| would otherwise be iterated one character at a time and a number or object | ||
| would throw out of the `Config` constructor during startup. | ||
|
|
||
| Which roots are _refusable_ is deliberately not decided here — see below. | ||
|
|
||
| ### Refusing launchers and state planters | ||
|
|
||
| A vouch says "this binary only reads". It can never say "and so does whatever I | ||
| pass it", so two families must never classify read-only however a caller | ||
| vouches for them, and both are decided inside the classifier | ||
| (`NEVER_READ_ONLY_ROOT_COMMANDS`) rather than by filtering the caller's set — | ||
| no caller can vouch them back in: | ||
|
|
||
| - **Launchers**: shell and language interpreters, multi-call binaries, and | ||
| wrappers that exec a command from their arguments. `time rm -rf build` is not | ||
| what the user meant by vouching `time`. | ||
| - **State planters**: builtins that rebind how a _later_ command resolves. | ||
| Statements are classified independently, so nothing else models | ||
| `hash -p ./evil/git git && git status` turning a trusted root into an | ||
| attacker-chosen binary. | ||
|
|
||
| The state-planter family is an enumerable set of bash builtins. The launcher | ||
| family is not — review demonstrated 16 missing names across two rounds — so the | ||
| list is only half the defence. The other half is structural: a vouched root is | ||
| refused the moment one of its arguments names a command the classifier knows | ||
| (`vouchedRootIsSafe`), matched on the basename. That closes the demonstrated | ||
| shape (`<launcher> <recognised write command>`) for launchers nobody has | ||
| enumerated, at the cost of an occasional extra prompt when a CLI's own | ||
| sub-command shares a name with a real command. Refusing costs a prompt; | ||
| accepting wrongly costs the write. | ||
|
|
||
| Residual: a launcher wrapping something the classifier does not recognise | ||
|
TianYuan1024 marked this conversation as resolved.
Outdated
|
||
| (`time ./script.sh`) still classifies read-only if that launcher is vouched and | ||
| absent from the list. That is the documented whole-binary scope of a vouch. | ||
|
|
||
| ### Substitutions hidden in expansion pattern words | ||
|
|
||
| tree-sitter-bash parses the pattern word of `${v%%…}`, `${v%…}`, `${v##…}` and | ||
| `${v#…}` as a single leaf, so a `$(…)` inside it produces no | ||
| `command_substitution` node even though bash runs it while expanding. The | ||
| substitution walker therefore missed it, and `echo ${HOME%%$(rm -rf build)}` | ||
| classified read-only — a pre-existing hole for built-in roots that the vouch | ||
| would have widened to arbitrary user-named ones. `evaluateSubstitutions` now | ||
| treats any `$(`/backtick still present in an expansion, after the substitution | ||
| walk collected nothing, as exactly that hidden channel. | ||
|
|
||
| ### Mode scoping | ||
|
|
||
| `Config.getPlanModeReadOnlyRoots()` returns the normalised set only while | ||
| `getApprovalMode() === ApprovalMode.PLAN`, and an empty set otherwise. Callers | ||
| pass it through unconditionally; the gate lives in one place. | ||
|
|
||
| ### Why four call sites | ||
|
|
||
| Classification alone is not enough. With the root vouched, | ||
| `planShellRequiresConfirmation` becomes false, but `finalPermission` is still | ||
| computed from `ShellToolInvocation.getDefaultPermission()` via | ||
| `evaluatePermissionFlow`, which would keep returning `ask` and keep the prompt. | ||
| The vouch is therefore passed at: | ||
|
|
||
| - `plan-mode-shell-policy.ts` — the Plan mode classification; | ||
| - `ShellToolInvocation` / `MonitorToolInvocation` — `getDefaultPermission` and | ||
| the read-only sub-command filter in `getConfirmationDetails`; | ||
| - `PermissionManager.resolveDefaultPermission` — the L3 `default` resolution for | ||
| compound sub-commands. | ||
|
|
||
| `PermissionManagerConfig.getPlanModeReadOnlyRoots` is optional so existing test | ||
| doubles keep compiling. | ||
|
|
||
| ### Failure behaviour | ||
|
|
||
| When the tree-sitter WASM parser is unavailable, `isShellCommandReadOnly*AST` | ||
| falls back to the deprecated regex checker, which does not know about the vouch | ||
| and keeps prompting. `classifyShellCommandSafety*` has no fallback and returns | ||
| `unknown`. Both directions fail closed, which is why the fallback was left | ||
| alone rather than given a duplicate copy of the setting. | ||
|
|
||
| ## Settings | ||
|
|
||
| ```json | ||
| { | ||
| "permissions": { | ||
| "planMode": { | ||
| "extraReadOnlyCommands": ["ib"] | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Merged as a union across scopes, `requiresRestart`, and dropped in `--bare` / | ||
| safe mode — mirroring `permissions.autoMode`. Documented for users in | ||
| `docs/users/features/approval-mode.md`. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.