fix(sandbox): require literal words for the whole AST call (#473 review)

astCommandFields guarded only the program word, then ran every argument
through wordText, which keeps literal/quoted parts and silently drops
CmdSubst/ParamExp. A dynamic ARGUMENT was therefore reconstructed lossily:
`git $(printf foo)rebase -i HEAD~1` runs as `git foorebase -i` (a
non-existent, non-interactive subcommand) but collapsed to `git rebase -i`
and was blocked -- a false positive on a command the user never wrote, and
new on this branch (the base correctly reports not-interactive).

Extend the literalness guard to every word in the call: if any word carries
an expansion, skip that AST command rather than classify a reconstruction
whose runtime value is unknowable. Skipping is the safe direction -- the
hand-written passes still run first, and a miss falls through to the normal
permission prompt instead of hard-blocking.

Genuine detection is unchanged: `git rebase -i HEAD~1` and the #473 bypass
cases still classify exactly as before.

internal/cli/exec.go

@@ -939,12 +939,6 @@ func providerSensitiveEnvKeys(resolved config.ResolvedConfig) []string {
 // applyConfiguredSandboxPolicy overlays every config-sourced sandbox knob onto
 // the default policy.
 func applyConfiguredSandboxPolicy(policy sandbox.Policy, cfg config.SandboxConfig) sandbox.Policy {
-	// An explicit `"sandbox": {"enabled": false}` disables the sandbox: every
-	// request is allowed and commands run unwrapped. Only reachable from global
-	// config / CLI — project config never sets Enabled (see mergeProjectConfig).
-	if cfg.Enabled != nil && !*cfg.Enabled {
-		policy.Mode = sandbox.ModeDisabled
-	}
 	if network := strings.TrimSpace(cfg.Network); network != "" {
 		switch sandbox.NetworkMode(network) {
 		case sandbox.NetworkAllow, sandbox.NetworkDeny:

internal/cli/sandbox_test.go

@@ -1001,18 +1001,3 @@ func sandboxPolicyCapabilityStatus(capabilities []sandbox.BackendCapability, key
 	}
 	return ""
 }
-
-func TestApplyConfiguredSandboxPolicyEnabledFalseDisables(t *testing.T) {
-	disabled := false
-	if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{Enabled: &disabled}); got.Mode != sandbox.ModeDisabled {
-		t.Fatalf("enabled:false must yield ModeDisabled, got %q", got.Mode)
-	}
-	// Omitted (nil) and explicit true keep the default enforcing mode.
-	if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{}); got.Mode == sandbox.ModeDisabled {
-		t.Fatal("omitted enabled must not disable the sandbox")
-	}
-	enabled := true
-	if got := applyConfiguredSandboxPolicy(sandbox.DefaultPolicy(), config.SandboxConfig{Enabled: &enabled}); got.Mode == sandbox.ModeDisabled {
-		t.Fatal("enabled:true must not disable the sandbox")
-	}
-}

internal/config/resolver.go

@@ -93,16 +93,6 @@ func Resolve(options ResolveOptions) (ResolvedConfig, error) {
 		if err != nil {
 			return ResolvedConfig{}, err
 		}
-		// Sandbox.Enabled is NOT accepted from a provider command, for the same
-		// reason project config cannot set it (see mergeProjectConfig): only
-		// global config and the CLI may turn the sandbox off. A provider command
-		// is an arbitrary executable named in config, and LoadProviderCommand
-		// parses its stdout into a full FileConfig — so without this, a command
-		// returning a valid provider plus {"sandbox":{"enabled":false}} would
-		// disable the very sandbox meant to constrain what it can do. Cleared
-		// here rather than in mergeConfig because that helper is shared with the
-		// trusted global-config merge, which must keep honouring the setting.
-		commandConfig.Sandbox.Enabled = nil
 		mergeConfig(&cfg, commandConfig)
 	}
 
@@ -226,9 +216,6 @@ func mergeConfig(dst *FileConfig, src FileConfig) {
 		mergeProvider(dst, provider)
 	}
 	mergeMCPConfig(&dst.MCP, src.MCP, true)
-	if src.Sandbox.Enabled != nil {
-		dst.Sandbox.Enabled = src.Sandbox.Enabled
-	}
 	if network := strings.TrimSpace(src.Sandbox.Network); network != "" {
 		dst.Sandbox.Network = network
 	}
@@ -286,10 +273,6 @@ func mergeProjectConfig(dst *FileConfig, src FileConfig) error {
 	if err := mergeProjectMCPConfig(&dst.MCP, src.MCP); err != nil {
 		return err
 	}
-	// Sandbox.Enabled is intentionally NOT merged from project config: a cloned
-	// repo's .zero/config.json must not be able to disable the sandbox that
-	// constrains it. Only global config and CLI can turn the sandbox off.
-	//
 	// Sandbox.AdditionalWriteRoots is intentionally NOT merged from project
 	// config: a cloned repo's .zero/config.json must not be able to grant
 	// itself write access outside the workspace. Global config and CLI flags
@@ -705,9 +688,6 @@ func applyOverrides(cfg *FileConfig, overrides Overrides) {
 	if overrides.MaxTurns > 0 {
 		cfg.MaxTurns = overrides.MaxTurns
 	}
-	if overrides.Sandbox.Enabled != nil {
-		cfg.Sandbox.Enabled = overrides.Sandbox.Enabled
-	}
 	if overrides.Sandbox.BlockUnixSockets {
 		cfg.Sandbox.BlockUnixSockets = true
 	}

internal/config/resolver_test.go

@@ -1744,85 +1744,6 @@ func TestResolveSandboxNetworkProjectConfigCannotWeaken(t *testing.T) {
 	}
 }
 
-func TestResolveSandboxEnabledUserConfigDisables(t *testing.T) {
-	userPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
-	resolved, err := Resolve(ResolveOptions{UserConfigPath: userPath, Env: map[string]string{}})
-	if err != nil {
-		t.Fatalf("Resolve: %v", err)
-	}
-	if resolved.Sandbox.Enabled == nil || *resolved.Sandbox.Enabled {
-		t.Fatalf("user config enabled:false must resolve to a false pointer, got %v", resolved.Sandbox.Enabled)
-	}
-}
-
-func TestResolveSandboxDisableIgnoredFromProjectConfig(t *testing.T) {
-	// Security: a cloned repo's project config must NOT be able to disable the
-	// sandbox that constrains it. Only global config / CLI can turn it off.
-	userPath := writeConfig(t, `{}`)
-	projectPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
-	resolved, err := Resolve(ResolveOptions{UserConfigPath: userPath, ProjectConfigPath: projectPath, Env: map[string]string{}})
-	if err != nil {
-		t.Fatalf("Resolve: %v", err)
-	}
-	if resolved.Sandbox.Enabled != nil {
-		t.Fatalf("project config enabled:false must be ignored, got %v — a repo could disable the sandbox", *resolved.Sandbox.Enabled)
-	}
-}
-
-func TestResolveSandboxEnabledIgnoredFromProviderCommand(t *testing.T) {
-	// Security: a provider command is an arbitrary executable named in config, and
-	// its stdout is parsed into a full FileConfig — so it must NOT be able to reach
-	// Sandbox.Enabled, in either direction. Same rule as project config: only global
-	// config / CLI may turn the sandbox off.
-	for _, tc := range []struct {
-		name  string
-		value string
-	}{
-		{name: "cannot disable", value: "false"},
-		{name: "cannot enable", value: "true"},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			command := writeCommand(t, commandScript{
-				Stdout: `{"activeProvider":"cmd","providers":[{"name":"cmd","provider":"openai","apiKey":"sk-command","model":"gpt-command"}],"sandbox":{"enabled":` + tc.value + `}}`,
-			})
-			resolved, err := Resolve(ResolveOptions{ProviderCommand: command, Env: map[string]string{}})
-			if err != nil {
-				t.Fatalf("Resolve: %v", err)
-			}
-			// The provider itself must still be applied — only the sandbox toggle is dropped.
-			if resolved.ActiveProvider != "cmd" {
-				t.Fatalf("ActiveProvider = %q, want cmd (provider command must still apply)", resolved.ActiveProvider)
-			}
-			if resolved.Sandbox.Enabled != nil {
-				t.Fatalf("provider command set sandbox.enabled=%s, got %v — a provider command could toggle the sandbox", tc.value, *resolved.Sandbox.Enabled)
-			}
-		})
-	}
-}
-
-func TestResolveSandboxEnabledCLIOverride(t *testing.T) {
-	// A CLI/programmatic override (applied after every config source) must be able
-	// to disable the sandbox, even with no config file.
-	disabled := false
-	resolved, err := Resolve(ResolveOptions{Env: map[string]string{}, Overrides: Overrides{Sandbox: SandboxConfig{Enabled: &disabled}}})
-	if err != nil {
-		t.Fatalf("Resolve: %v", err)
-	}
-	if resolved.Sandbox.Enabled == nil || *resolved.Sandbox.Enabled {
-		t.Fatalf("CLI override enabled:false must disable the sandbox, got %v", resolved.Sandbox.Enabled)
-	}
-	// An explicit true override must win over a lower-precedence global-config false.
-	userPath := writeConfig(t, `{"sandbox": {"enabled": false}}`)
-	enabled := true
-	resolved2, err := Resolve(ResolveOptions{UserConfigPath: userPath, Env: map[string]string{}, Overrides: Overrides{Sandbox: SandboxConfig{Enabled: &enabled}}})
-	if err != nil {
-		t.Fatalf("Resolve: %v", err)
-	}
-	if resolved2.Sandbox.Enabled == nil || !*resolved2.Sandbox.Enabled {
-		t.Fatalf("CLI override enabled:true must win over config false, got %v", resolved2.Sandbox.Enabled)
-	}
-}
-
 func TestResolveNotifyValid(t *testing.T) {
 	path := writeConfig(t, `{"notify":{"mode":"both","focusMode":"always"}}`)
 	resolved, err := Resolve(ResolveOptions{UserConfigPath: path, Env: map[string]string{}})

internal/config/types.go

@@ -62,12 +62,6 @@ func HasProviderProfile(profile ProviderProfile) bool {
 }
 
 type SandboxConfig struct {
-	// Enabled turns the sandbox off when set to false (`"sandbox": {"enabled":
-	// false}`). A pointer distinguishes an explicit false (disable) from an
-	// omitted key (keep the default: enabled). Honored from the GLOBAL user
-	// config and CLI only — deliberately NOT project config, so a cloned repo
-	// cannot disable the sandbox that constrains it. nil / true keep enforcement.
-	Enabled *bool `json:"enabled,omitempty"`
 	// Network controls whether shell commands classified as network-touching
 	// (curl, git push, package installs, …) are allowed: "allow" or "deny".
 	// Empty keeps the built-in default (deny). Without this knob the engine's

internal/sandbox/analyzer.go

@@ -65,6 +65,76 @@ func AnalyzeCommand(script string) AnalysisResult {
 	return result
 }
 
+// astCommandFields parses command with the shell parser and returns each simple
+// command as its literal field slice (program + args as text), resolving the
+// real command positions across quoting, command substitution, subshells, and
+// newline separators — the constructs the hand-written splitter in
+// safe_command.go mis-handles (issue #473). It returns nil when the command
+// cannot be parsed (e.g. a Windows cmd.exe string), so callers fall through to
+// the regex path rather than hard-blocking.
+func astCommandFields(command string) [][]string {
+	file, err := syntax.NewParser().Parse(strings.NewReader(command), "")
+	if err != nil {
+		return nil
+	}
+	var commands [][]string
+	syntax.Walk(file, func(node syntax.Node) bool {
+		call, ok := node.(*syntax.CallExpr)
+		if !ok || len(call.Args) == 0 {
+			return true
+		}
+		// EVERY word must be a static literal, not just the program name.
+		// wordText keeps only the literal/quoted parts of a word and silently
+		// drops expansions, so a dynamic word anywhere in the call reconstructs
+		// to something the shell will never run: `$(printf foo)vim` (runs as
+		// `foovim`) collapses to "vim", and `git $(printf foo)rebase -i` (runs
+		// as `git foorebase -i`, non-interactive) collapses to `git rebase -i`
+		// and fabricates an interactive match. Since the runtime value of an
+		// expansion is unknowable here, skip the whole call rather than classify
+		// a lossy reconstruction. Skipping is the safe direction: the
+		// hand-written passes above already ran, and a missed detection falls
+		// through to the normal permission prompt instead of hard-blocking a
+		// command the user never wrote.
+		for _, word := range call.Args {
+			if !isLiteralWord(word) {
+				return true
+			}
+		}
+		fields := make([]string, 0, len(call.Args))
+		for _, word := range call.Args {
+			fields = append(fields, wordText(word))
+		}
+		commands = append(commands, fields)
+		return true
+	})
+	return commands
+}
+
+// isLiteralWord reports whether every part of word is a static literal (bare or
+// quoted). A word containing a command substitution, parameter/arithmetic
+// expansion, process substitution, etc. is dynamic — its runtime value is
+// unknown, so its wordText (a partial literal) must not be trusted as a program
+// name.
+func isLiteralWord(word *syntax.Word) bool {
+	if word == nil {
+		return false
+	}
+	for _, part := range word.Parts {
+		switch typed := part.(type) {
+		case *syntax.Lit, *syntax.SglQuoted:
+		case *syntax.DblQuoted:
+			for _, inner := range typed.Parts {
+				if _, ok := inner.(*syntax.Lit); !ok {
+					return false
+				}
+			}
+		default:
+			return false
+		}
+	}
+	return true
+}
+
 // analyzeInto parses script and folds its interactive/destructive/network usage
 // into result, sharing seen so program names are de-duplicated across recursion.
 func analyzeInto(script string, result *AnalysisResult, seen map[string]bool, depth int) {

internal/sandbox/safe_command.go

@@ -219,9 +219,65 @@ func DetectInteractiveCommand(command string, goos string) InteractiveCommandRes
 		}
 	}
 
+	// AST second opinion (issue #473): the hand-written segment split above
+	// mis-handles interactive programs hidden by unusual quoting, command
+	// substitution, subshells, or newline separators. Re-extract the real simple
+	// commands from the parsed shell tree and apply the SAME full pipeline
+	// (interactive segments, sh -c payload recursion, per-program lookup), so a
+	// bypass is caught while every program stays classified exactly as before
+	// (ssh with a trailing command, python -c, etc. remain allowed). A command
+	// the parser cannot handle (Windows cmd.exe, obfuscation) yields no commands
+	// and falls through unchanged — the guard never hard-blocks on a parse error.
+	for _, fields := range astCommandFields(command) {
+		if result, ok := inspectCommandFields(fields, goos); ok {
+			return result
+		}
+	}
+
 	return InteractiveCommandResult{}
 }
 
+// inspectCommandFields applies the full interactive-detection pipeline to one
+// already-split command's fields: the multi-word interactive segments, then the
+// `sh -c <payload>` recursion, then the per-program lookup with its
+// non-interactive suppressions (hasNonInteractiveFlag covers REPL flags and
+// trailing-command clients like ssh). It mirrors the hand-written passes above
+// so an AST-extracted command is classified identically to a plainly-split one.
+func inspectCommandFields(fields []string, goos string) (InteractiveCommandResult, bool) {
+	body := strings.ToLower(commandBody(fields))
+	for _, seg := range interactiveSegments {
+		if body == seg.match || strings.HasPrefix(body, seg.match+" ") {
+			suggestion := seg.suggestion
+			if goos == "windows" && seg.windowsSuggestion != "" {
+				suggestion = seg.windowsSuggestion
+			}
+			return InteractiveCommandResult{Interactive: true, Command: seg.command, Reason: seg.reason, Suggestion: suggestion}, true
+		}
+	}
+	first := firstProgram(fields)
+	if first == "" {
+		return InteractiveCommandResult{}, false
+	}
+	if payload := shellDashCPayload(first, fields); payload != "" {
+		if inner := DetectInteractiveCommand(payload, goos); inner.Interactive {
+			return inner, true
+		}
+		return InteractiveCommandResult{}, false
+	}
+	program, ok := interactivePrograms[first]
+	if !ok || (program.windowsOnly && goos != "windows") {
+		return InteractiveCommandResult{}, false
+	}
+	if hasNonInteractiveFlag(first, fields) {
+		return InteractiveCommandResult{}, false
+	}
+	suggestion := program.suggestion
+	if goos == "windows" && program.windowsSuggestion != "" {
+		suggestion = program.windowsSuggestion
+	}
+	return InteractiveCommandResult{Interactive: true, Command: first, Reason: program.reason, Suggestion: suggestion}, true
+}
+
 // wrapperPrograms are launcher prefixes that precede the real program. After
 // one of these we keep scanning for the actual executable.
 var wrapperPrograms = map[string]bool{

internal/sandbox/safe_command_test.go

@@ -406,3 +406,87 @@ func TestDetectInteractiveMongoEvalAndFullPaths(t *testing.T) {
 		}
 	}
 }
+
+// The hand-written segment splitter misses interactive programs hidden by
+// constructs only a real shell parser resolves (a newline separator collapsed
+// to a space; a brace group that shifts the real command position). The AST
+// second opinion must catch them (issue #473).
+func TestDetectInteractiveCommandCatchesParserBypasses(t *testing.T) {
+	for _, cmd := range []string{
+		"echo hi\nvim file.txt", // newline separator collapsed to a space by the regex path
+		"{ vim file.txt; }",     // brace group hides the real program position
+	} {
+		got := DetectInteractiveCommand(cmd, "linux")
+		if !got.Interactive {
+			t.Errorf("DetectInteractiveCommand(%q) = not interactive, want caught (parser bypass)", cmd)
+			continue
+		}
+		if got.Command != "vim" {
+			t.Errorf("DetectInteractiveCommand(%q).Command = %q, want vim", cmd, got.Command)
+		}
+	}
+}
+
+// The AST second opinion must not flag an interactive program NAME that appears
+// only inside a quoted argument (not a real command position) — the parser
+// distinguishes program from argument, so this must stay non-interactive.
+func TestDetectInteractiveCommandNoFalsePositiveOnQuotedArgument(t *testing.T) {
+	got := DetectInteractiveCommand(`echo "please run vim later"`, "linux")
+	if got.Interactive {
+		t.Fatalf("interactive name inside a quoted argument must not be flagged, got %#v", got)
+	}
+}
+
+// The AST second opinion must run the FULL pipeline — multi-word interactive
+// segments and sh -c payload recursion, not just the per-program lookup — so a
+// grouped/nested bypass is caught; and it must not fabricate a program name from
+// a command substitution concatenated with a literal (issue #473, review).
+func TestDetectInteractiveCommandASTPipelineBoundaries(t *testing.T) {
+	interactive := []struct{ cmd, wantCmd string }{
+		{"{ git rebase -i HEAD~1; }", "git rebase -i"}, // multi-word segment in a brace group
+		{"{ sh -c 'vim file.txt'; }", "vim"},           // sh -c payload in a brace group
+	}
+	for _, tc := range interactive {
+		got := DetectInteractiveCommand(tc.cmd, "linux")
+		if !got.Interactive || got.Command != tc.wantCmd {
+			t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", tc.cmd, got, tc.wantCmd)
+		}
+	}
+}
+
+// The AST pass must not fabricate a program name from a dynamic (non-literal)
+// program word: `$(printf '%s' foo)vim` runs as `foovim`, not vim. Tested at the
+// extractor so the hand-written pass (which pre-empts DetectInteractiveCommand
+// with its own, separate handling of that construct) does not mask the guard.
+func TestAstCommandFieldsSkipsDynamicProgram(t *testing.T) {
+	for _, fields := range astCommandFields("$(printf '%s' foo)vim file.txt") {
+		if firstProgram(fields) == "vim" {
+			t.Fatalf("astCommandFields fabricated a vim command from a substitution: %v", fields)
+		}
+	}
+}
+
+// The literalness guard covers every word, not just the program name. A dynamic
+// ARGUMENT is dropped by wordText the same way a dynamic program word is, so
+// `git $(printf foo)rebase -i HEAD~1` (which runs as `git foorebase -i`, a
+// non-existent and non-interactive subcommand) would otherwise be reconstructed
+// as `git rebase -i` and blocked — a false positive on a command the user never
+// wrote. Asserted through DetectInteractiveCommand as well as the extractor,
+// because the whole point is what the caller ends up blocking.
+func TestAstCommandFieldsSkipsDynamicArgument(t *testing.T) {
+	const dynamic = "{ git $(printf foo)rebase -i HEAD~1 ; }"
+	for _, fields := range astCommandFields(dynamic) {
+		if firstProgram(fields) == "git" {
+			t.Fatalf("astCommandFields reconstructed a git command from a dynamic argument: %v", fields)
+		}
+	}
+	if got := DetectInteractiveCommand(dynamic, "linux"); got.Interactive {
+		t.Errorf("DetectInteractiveCommand(%q) = %+v, want not interactive", dynamic, got)
+	}
+	// The genuine form must still be detected — the guard skips lossy
+	// reconstructions, it does not weaken real detection.
+	const genuine = "{ git rebase -i HEAD~1 ; }"
+	if got := DetectInteractiveCommand(genuine, "linux"); !got.Interactive || got.Command != "git rebase -i" {
+		t.Errorf("DetectInteractiveCommand(%q) = %+v, want interactive Command=%q", genuine, got, "git rebase -i")
+	}
+}