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/agent/guardrails_test.go

@@ -169,37 +169,6 @@ func TestRunResetsEmptyTurnCounterOnToolCall(t *testing.T) {
 	}
 }
 
-// The whole point of #702's fix: the unknown-session error's normalized
-// signature must not vary with the session id, so a model probing ids 1, 2,
-// 3, … produces one repeated signature the failure guard can count. If the id
-// leaked into the first 80 normalized chars, each probe would reset the streak
-// and the halt would never fire.
-func TestUnknownExecSessionErrorSignatureIsIDInvariant(t *testing.T) {
-	a := errorSignature(tools.UnknownExecSessionError(1))
-	b := errorSignature(tools.UnknownExecSessionError(999999))
-	if a != b {
-		t.Fatalf("unknown-session signature varies with id:\n  %q\n  %q", a, b)
-	}
-}
-
-// End to end: with an id-invariant signature, probing a different unknown id
-// each turn now trips the repeated-failure halt at toolFailureStopAt, where
-// before the fix it never would.
-func TestUnknownExecSessionProbingTripsFailureHalt(t *testing.T) {
-	var state guardState
-	var stoppedAt int
-	for i := 1; i <= toolFailureStopAt; i++ {
-		out := state.observeToolResult(tools.WriteStdinToolName, true, tools.UnknownExecSessionError(i))
-		if out.Stop {
-			stoppedAt = i
-			break
-		}
-	}
-	if stoppedAt != toolFailureStopAt {
-		t.Fatalf("probing distinct unknown ids stopped at %d, want %d", stoppedAt, toolFailureStopAt)
-	}
-}
-
 func TestGuardStateResetsToolOnlyStreakOnEmptyNonToolTurn(t *testing.T) {
 	var state guardState
 	toolOnly := zeroruntime.CollectedStream{

internal/agent/system_prompt.md

@@ -100,9 +100,6 @@ work.
 - Use exec_command with `tty: true` for interactive terminal-style commands that
   need stdin beyond Ctrl-C. `/ps` and `/stop` are user-facing TUI commands; when
   you need to clean up a running foreground command yourself, use write_stdin.
-- write_stdin's session_id is only ever an id returned by a still-running
-  exec_command; never guess or probe ids. If you have no such session, start one
-  with exec_command, or use write_file/edit_file/apply_patch for file changes.
 - write_stdin with empty input polls an existing exec_command session, and
   `\u0003` interrupts it. Sending other stdin bytes may require approval because
   it can drive the running process beyond the original command. Non-tty sessions

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")
+	}
+}

internal/tools/exec_command.go

@@ -707,18 +707,6 @@ type writeStdinTool struct {
 	manager *execSessionManager
 }
 
-// UnknownExecSessionError is the result returned when write_stdin targets a
-// session_id with no live exec session. The stable recovery guidance leads and
-// the numeric id trails on purpose: the agent's repeated-failure guard keys on
-// a normalized, truncated prefix of the error string, so keeping the id out of
-// that prefix makes a model probing ids 1, 2, 3, … produce ONE signature that
-// finally trips the halt instead of a fresh signature per id — while also
-// telling the model how to actually recover. See TestUnknownExecSessionErrorSignatureIsIDInvariant
-// in internal/agent, which pins this invariant against the real guard.
-func UnknownExecSessionError(sessionID int) string {
-	return fmt.Sprintf("Error: write_stdin needs a session_id returned by a still-running exec_command; do not guess or probe session ids. Start a process with exec_command, or use write_file/edit_file/apply_patch for file changes. (no live session %d)", sessionID)
-}
-
 func (writeStdinTool) outputCategory(map[string]any) outputCategory {
 	return outputCategoryProcess
 }
@@ -734,7 +722,7 @@ func NewWriteStdinTool(manager *execSessionManager) Tool {
 			parameters: Schema{
 				Type: "object",
 				Properties: map[string]PropertySchema{
-					"session_id":        {Type: "integer", Description: "Identifier of a running unified exec session, as returned by exec_command. Never guess or probe ids.", Minimum: intPtr(1)},
+					"session_id":        {Type: "integer", Description: "Identifier of the running unified exec session."},
 					"chars":             {Type: "string", Description: "Bytes to write to stdin. Defaults to empty, which polls without writing.", Default: ""},
 					"yield_time_ms":     {Type: "integer", Description: "Wait before yielding output. Non-empty writes default to 250 ms and cap at 30000 ms; empty polls wait 5000-300000 ms by default.", Default: defaultPollYieldTimeMS, Minimum: intPtr(1), Maximum: intPtr(maxPollYieldTimeMS)},
 					"max_output_tokens": {Type: "integer", Description: "Output token budget. Defaults to 10000 tokens; larger requests may be capped by policy.", Default: defaultMaxOutputTokens, Minimum: intPtr(1), Maximum: intPtr(maxExecOutputTokenRequest)},
@@ -796,7 +784,7 @@ func (tool writeStdinTool) RunWithOptions(ctx context.Context, args map[string]a
 	}
 	session, ok := tool.manager.get(sessionID)
 	if !ok {
-		return errorResult(UnknownExecSessionError(sessionID))
+		return errorResult(fmt.Sprintf("Error: Unknown exec session_id %d.", sessionID))
 	}
 	session.touch()
 	interrupted := false

internal/tools/exec_command_test.go

@@ -881,7 +881,7 @@ func TestRegistryHonorsWriteStdinArgumentPermission(t *testing.T) {
 	registry.Register(NewWriteStdinTool(newExecSessionManager()))
 
 	poll := registry.Run(context.Background(), WriteStdinToolName, map[string]any{"session_id": 9999})
-	if poll.Status != StatusError || !strings.Contains(poll.Output, "still-running exec_command") {
+	if poll.Status != StatusError || !strings.Contains(poll.Output, "Unknown exec session_id") {
 		t.Fatalf("empty poll should reach tool without permission prompt, got status=%s output=%q", poll.Status, poll.Output)
 	}
 
@@ -901,25 +901,8 @@ func TestWriteStdinReportsUnknownSession(t *testing.T) {
 	if result.Status != StatusError {
 		t.Fatalf("status = %s, want error", result.Status)
 	}
-	// The message must lead with recovery guidance and still identify the id.
-	if !strings.Contains(result.Output, "still-running exec_command") {
-		t.Fatalf("unknown-session error must carry recovery guidance, got: %q", result.Output)
-	}
-	if !strings.Contains(result.Output, "1234") {
-		t.Fatalf("unknown-session error must name the offending id, got: %q", result.Output)
-	}
-}
-
-// The runtime rejects session_id < 1 (intArg min 1); the advertised schema must
-// say the same so a model sees the constraint before it probes id 0.
-func TestWriteStdinSchemaPinsSessionIDMinimum(t *testing.T) {
-	schema := NewWriteStdinTool(nil).(writeStdinTool).parameters
-	prop, ok := schema.Properties["session_id"]
-	if !ok {
-		t.Fatal("session_id property missing from write_stdin schema")
-	}
-	if prop.Minimum == nil || *prop.Minimum != 1 {
-		t.Fatalf("session_id schema Minimum = %v, want 1 to match the runtime floor", prop.Minimum)
+	if !strings.Contains(result.Output, "Unknown exec session_id 1234") {
+		t.Fatalf("unexpected output: %q", result.Output)
 	}
 }