Repository navigation
fix(cli): stop a configured sandbox from bypassing approval remotely - #1835
Open
ethicnology-agent wants to merge 2 commits into
Open
ethicnology-agent wants to merge 2 commits into
ethicnology-agent wants to merge 2 commits into
Conversation
runClaude derived the permission mode from sandbox configuration rather than from a sandbox that exists. applySandboxPermissionPolicy rewrote the mode to bypassPermissions whenever settings.sandboxConfig.enabled was set, before any execution path had been chosen, and resolveRemoteClaudePermissionMode re-applied it to every incoming message. claudeLocal is the only Claude path that calls initializeSandbox, and it appends --dangerously-skip-permissions itself, inside the branch where the sandbox has actually been put in place. The remote path never initializes one. Turning the sandbox on therefore removed every approval prompt from remote sessions while nothing confined them, which is the opposite of what the setting offers. Codex already binds its auto-approval to observed state: codexAppServerClient sets sandboxEnabled false, then true only after initializeSandbox and wrapForMcpTransport both succeed. This aligns Claude with that. The regression that encoded the old behaviour is removed and replaced by cases asserting that an explicit plan request stays plan, while the existing guard against an ambient default downgrading a bypassing session still holds.
applySandboxPermissionPolicy has no caller since the previous commit, and resolveRemoteClaudePermissionMode's sandboxEnabled parameter is no longer read. Remove both, with the tests that covered the helper directly. No behaviour change.
ethicnology-agent
force-pushed
the
fix/cli-sandbox-does-not-bypass-approval
branch
from
October 1, 2026 19:02
f516c7a to
9aa6d02
Compare
Author
|
Rebased onto current |
This branch has not been deployed
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.
Fixes the first defect reported in #1830.
Problem
runClaudederives the permission mode from sandbox configuration rather than from a sandbox that exists:applySandboxPermissionPolicyrewrites the mode tobypassPermissionsunconditionally, before any execution path has been chosen, andresolveRemoteClaudePermissionModere-applies it to every incoming message.claudeLocalis the only Claude path that callsinitializeSandbox(claudeLocal.ts:283), and it appends--dangerously-skip-permissionsitself inside the branch where that call succeeded — it even falls back to no sandbox and no flag on Windows. The remote path never initializes a sandbox at all.The result is that turning the sandbox on removes every approval prompt from remote sessions while nothing confines them. Enabling the feature makes a remote session strictly less safe than leaving it off, which is the opposite of what the setting offers. The app meanwhile displays "Sandbox: Enabled".
Codex in this repository already gets this right:
codexAppServerClientsetssandboxEnabled = false, thentrueonly afterinitializeSandboxandwrapForMcpTransportboth succeed, andrunCodexreads that observed value. This aligns Claude with the same rule.What this changes
Read the commits in order:
fix(cli): stop a configured sandbox from bypassing approval remotely— the behaviour change.runClaudeusesresolveInitialClaudePermissionModedirectly, andresolveRemoteClaudePermissionModeno longer takes a sandbox flag. Three cases replace the one that encoded the old behaviour: an explicitplanrequest staysplan,defaultstaysdefault, and the existing guard against an ambientdefaultdowngrading a bypassing session still holds.refactor(cli): drop the now-unused sandbox permission policy helper— removesapplySandboxPermissionPolicyand the dead parameter. No behaviour change; separated so the first commit stays small enough to judge.tsc --noEmitis clean and the CLI unit suite passes 993/993.What this deliberately does not change
claudeLocalstill adds--dangerously-skip-permissionsonce the sandbox is genuinely in place. That is a defensible bargain and this PR does not relitigate it.dangerouslySkipPermissionsmetadata flag still includessandboxEnabled. After this change it over-reports danger for remote sessions rather than under-reporting it, which is the safe direction; making it exact needs the local/remote distinction, and the run mode can change at runtime (runClaude.ts:793,:952). Worth a follow-up, not worth widening this diff.Caveats, stated plainly
I could not exercise the runtime local↔remote mode switch, so that path is reasoned about from source rather than observed. If you would rather a configured sandbox refuse to start a remote session instead of falling back to prompting, say so — that is a one-line change to the same place, and it is your product call, not mine.