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/loop.go

@@ -181,8 +181,7 @@ func Run(ctx context.Context, prompt string, provider Provider, options Options)
 	options.runPermissions = runPermissions
 	defer runPermissions.cleanup()
 
-	promptParts := buildSystemPromptParts(options)
-	messages := zeroruntime.SeedMessagesWithImages(promptParts.prompt, prompt, options.Images)
+	messages := zeroruntime.SeedMessagesWithImages(buildSystemPrompt(options), prompt, options.Images)
 
 	guards := newGuardState()
 	compactor := newCompactionState(options)
@@ -269,14 +268,16 @@ func Run(ctx context.Context, prompt string, provider Provider, options Options)
 		exposed, _ := partitionToolsCached(registry, permissionMode, options, loaded, toolDefCache)
 		toolPartitionSpan.End()
 
-		// Fingerprint the exact system prompt built at run start plus this turn's
-		// provider-visible tool definitions in their emitted order. Reusing the
-		// retained prompt parts avoids rebuilding workspace context while ensuring
-		// the trace describes the same bytes carried by the request.
+		// Prompt-prefix fingerprint: hash the seven cacheable sub-components
+		// of this turn's request (system prompt sections, project context,
+		// skills, partitioned tool list, tool schemas) and emit one trace
+		// event. Stable across turns => cacheable. Drift in any sub-hash
+		// names the sub-component that broke the cache for this turn. The
+		// computation is pure and cheap (seven SHA-256s of small strings)
+		// and is a no-op when tracing is off.
 		if options.Trace != nil {
-			fp := computePrefixFingerprint(buildPromptSubstringsFromParts(promptParts, exposed))
+			fp := ComputePrefixFingerprint(options, exposed)
 			options.Trace.EmitPrefixHash(trace.PrefixHash{
-				SystemPromptHash:       fp.SystemPromptHash,
 				BaseInstructionsHash:   fp.BaseInstructionsHash,
 				ConfirmationPolicyHash: fp.ConfirmationPolicyHash,
 				ProjectContextHash:     fp.ProjectContextHash,

internal/agent/loop_test.go

@@ -8,7 +8,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"reflect"
 	"runtime"
 	"strings"
 	"testing"
@@ -1259,46 +1258,6 @@ func TestRunExecutesToolCallThroughRegistry(t *testing.T) {
 	}
 }
 
-func TestRunPreservesRequestPrefixAcrossTurns(t *testing.T) {
-	root := t.TempDir()
-	writeAgentTestFile(t, filepath.Join(root, "notes.txt"), "alpha\n")
-	registry := tools.NewRegistry()
-	registry.Register(tools.NewReadFileTool(root))
-	provider := &mockProvider{turns: [][]zeroruntime.StreamEvent{
-		{
-			{Type: zeroruntime.StreamEventToolCallStart, ToolCallID: "call-1", ToolName: "read_file"},
-			{Type: zeroruntime.StreamEventToolCallDelta, ToolCallID: "call-1", ArgumentsFragment: `{"path":"notes.txt"}`},
-			{Type: zeroruntime.StreamEventToolCallEnd, ToolCallID: "call-1"},
-			{Type: zeroruntime.StreamEventDone},
-		},
-		{
-			{Type: zeroruntime.StreamEventText, Content: "done"},
-			{Type: zeroruntime.StreamEventDone},
-		},
-	}}
-
-	if _, err := Run(context.Background(), "read notes", provider, Options{
-		Cwd:       root,
-		Registry:  registry,
-		SessionID: "session-stable-prefix",
-	}); err != nil {
-		t.Fatal(err)
-	}
-	if len(provider.requests) != 2 {
-		t.Fatalf("provider requests = %d, want 2", len(provider.requests))
-	}
-	first, second := provider.requests[0], provider.requests[1]
-	if len(second.Messages) < len(first.Messages) || !reflect.DeepEqual(second.Messages[:len(first.Messages)], first.Messages) {
-		t.Fatalf("first request messages must be an exact prefix of the second:\nfirst=%#v\nsecond=%#v", first.Messages, second.Messages)
-	}
-	if !reflect.DeepEqual(first.Tools, second.Tools) {
-		t.Fatalf("tool definitions drifted between turns:\nfirst=%#v\nsecond=%#v", first.Tools, second.Tools)
-	}
-	if first.PromptCacheKey != "session-stable-prefix" || second.PromptCacheKey != first.PromptCacheKey {
-		t.Fatalf("prompt cache key must remain stable: first=%q second=%q", first.PromptCacheKey, second.PromptCacheKey)
-	}
-}
-
 func TestRunSanitizesMalformedToolCallArgumentsBeforeRetry(t *testing.T) {
 	root := t.TempDir()
 	registry := tools.NewRegistry()

internal/agent/prompt_fingerprint.go

@@ -11,11 +11,12 @@ import (
 	"github.com/Gitlawb/zero/internal/zeroruntime"
 )
 
-// promptSubstrings are the cacheable components of the prompt a run sends to a
-// model. systemPrompt is the exact joined system-message content. The remaining
-// fields retain useful diagnostic boundaries within that prompt and tool list.
+// promptSubstrings are the seven cacheable sub-components of the prompt a run
+// sends to a model. Each is the literal content the sub-component would emit
+// into the system prompt or tool list, hashed so a downstream observer can
+// detect drift turn-over-turn. The substrings are produced by
+// buildPromptSubstrings and consumed by ComputePrefixFingerprint.
 type promptSubstrings struct {
-	systemPrompt       string
 	baseInstructions   string
 	confirmationPolicy string
 	projectContext     string
@@ -24,55 +25,92 @@ type promptSubstrings struct {
 	schema             string
 }
 
-// buildPromptSubstrings builds the prompt and retains both its exact joined
-// form and narrower diagnostic components. Callers that already built the
-// prompt should use buildPromptSubstringsFromParts to avoid repeating workspace
-// reads.
+// buildPromptSubstrings assembles the seven cacheable sub-components of the
+// prompt without joining them. The system prompt sections are produced by the
+// same private builders that buildSystemPrompt uses, so the substrings are
+// byte-identical to what would have appeared in the joined prompt. The tools
+// and schema substrings are derived from the partitioned tool list (caller-
+// provided) so this helper does not need to re-run the partition.
+//
+// This is the seam ComputePrefixFingerprint reads from. It exists as a
+// separate function (rather than returning both prompt and substrings from
+// buildSystemPrompt) so existing callers of buildSystemPrompt are unaffected
+// and the substrings helper is independently testable.
 func buildPromptSubstrings(options Options, exposed []zeroruntime.ToolDefinition) promptSubstrings {
-	return buildPromptSubstringsFromParts(buildSystemPromptParts(options), exposed)
-}
+	core := strings.TrimSpace(options.SystemPrompt)
+	if core == "" {
+		core = strings.TrimSpace(coreSystemPrompt)
+	}
+	if core == "" {
+		core = fallbackSystemPrompt
+	}
 
-func buildPromptSubstringsFromParts(parts systemPromptParts, exposed []zeroruntime.ToolDefinition) promptSubstrings {
+	// The confirmation policy is appended unconditionally in buildSystemPrompt;
+	// the substring here is the same constant.
+	policy := strings.TrimSpace(confirmationPolicy)
+
+	// projectContext is the joined output of workspaceContext, which itself
+	// walks the AGENTS.md / ZERO.md / .zero/AGENTS.md chain. Hashing the
+	// joined string captures every guideline file's contribution and the
+	// ordering between them.
+	project := workspaceContext(options.Cwd)
+
+	// skills is the joined output of skillsContext, with each skill on its own
+	// line. The substring is the literal text the model would see.
+	skills := skillsContext(options)
+
+	// tools and schema are derived from the partitioned tool list. The list
+	// passed in is already in stable order (partitionToolsCached is the
+	// canonical stable partitioner) so concatenating in slice order is
+	// sufficient — we sort names again defensively in case a future caller
+	// hands us a list from a different partitioner.
 	toolsSubstr, schemaSubstr := toolSubstrings(exposed)
 
 	return promptSubstrings{
-		systemPrompt:       parts.prompt,
-		baseInstructions:   parts.baseInstructions,
-		confirmationPolicy: parts.confirmationPolicy,
-		projectContext:     parts.projectContext,
-		skills:             parts.skills,
+		baseInstructions:   core,
+		confirmationPolicy: policy,
+		projectContext:     project,
+		skills:             skills,
 		tools:              toolsSubstr,
 		schema:             schemaSubstr,
 	}
 }
 
-// toolSubstrings preserves the emitted tool order. Names and descriptions are
-// fingerprinted separately from schemas so either kind of drift is identifiable.
-// Order is part of request identity: silently sorting here would hide a provider-
-// visible reordering and could report a false cache hit.
+// toolSubstrings extracts a stable, name-sorted digest of the partitioned tool
+// list. The tools substring is "name1\nname2\n..." (sorted) so a reordering of
+// the partition produces a different hash. The schema substring is the
+// concatenation of each tool's JSON schema in name order, so a schema change
+// is detected independently of the tool ordering.
 func toolSubstrings(exposed []zeroruntime.ToolDefinition) (toolsSubstr, schemaSubstr string) {
 	if len(exposed) == 0 {
 		return "", ""
 	}
-	var toolsSB, schemaSB strings.Builder
+	names := make([]string, 0, len(exposed))
+	byName := make(map[string]zeroruntime.ToolDefinition, len(exposed))
 	for _, def := range exposed {
-		writeLengthPrefixed(&toolsSB, def.Name)
-		writeLengthPrefixed(&toolsSB, def.Description)
+		names = append(names, def.Name)
+		byName[def.Name] = def
+	}
+	sort.Strings(names)
+	var toolsSB, schemaSB strings.Builder
+	for i, name := range names {
+		if i > 0 {
+			toolsSB.WriteByte('\n')
+		}
+		toolsSB.WriteString(name)
+		def := byName[name]
 		// Parameters is rendered to a canonical JSON string per tool. The
 		// schema-render cache in internal/agent (used by partitionToolsCached)
 		// guarantees the same render is byte-identical across turns for the
 		// same tool, so this substring is a stable hash input.
-		writeLengthPrefixed(&schemaSB, def.Name)
-		writeLengthPrefixed(&schemaSB, canonicalSchemaString(def.Parameters))
+		schemaSB.WriteString(name)
+		schemaSB.WriteByte('\n')
+		schemaSB.WriteString(canonicalSchemaString(def.Parameters))
+		schemaSB.WriteByte('\n')
 	}
 	return toolsSB.String(), schemaSB.String()
 }
 
-func writeLengthPrefixed(sb *strings.Builder, value string) {
-	fmt.Fprintf(sb, "%d:", len(value))
-	sb.WriteString(value)
-}
-
 // canonicalSchemaString renders a tool's parameter schema to a stable string.
 // tools.ToolDef.Parameters is a map[string]any in the common case; Go's
 // fmt.Sprintf("%v", m) iterates a map in random order, which would produce
@@ -189,16 +227,36 @@ func writeStable(sb *strings.Builder, v any) {
 	}
 }
 
-// ComputePrefixFingerprint builds and fingerprints a prompt for callers that do
-// not already have one. The agent loop uses computePrefixFingerprint with the
-// prompt parts it built once at run start, ensuring tracing observes the exact
-// request content without repeating workspace I/O.
-func ComputePrefixFingerprint(options Options, exposed []zeroruntime.ToolDefinition) prefixFingerprint {
-	return computePrefixFingerprint(buildPromptSubstrings(options, exposed))
-}
+// ComputePrefixFingerprint returns a trace.PrefixHash (a 7-field fingerprint
+// of the prompt prefix) for one turn of a run. The seven sub-hashes are
+// independent SHA-256s of the corresponding sub-component; the complete-prefix
+// hash is a SHA-256 of the canonical concatenation of the other six, so any
+// sub-component drift is observable both individually and in aggregate.
+//
+// I/O side effects: buildPromptSubstrings calls workspaceContext, which runs
+// git (gitBranchForPrompt, FindProjectGitRoot) and reads the AGENTS.md /
+// ZERO.md / .zero/AGENTS.md chain plus the repo map. The same workspace-
+// context work is performed separately by buildSystemPrompt a few lines
+// later in the request-build path, so on every traced turn the file and
+// git reads happen twice. The duplication is intentional for this PR (a
+// follow-up will pass substrings through) and is a known perf cost only on
+// the opt-in trace path.
+//
+// A stable CompletePrefixHash across turns means the four captured
+// sub-components (baseInstructions, confirmationPolicy, projectContext,
+// skills) are byte-identical. It does NOT rule out drift in the seven
+// uncaptured sections of buildSystemPrompt (modelPromptAddendum,
+// sessionRuntimeContext, approvedCommandPrefixContext, workspaceSeedContext,
+// userGuidelines, specialistDelegationContext, responseStyleContext). For
+// default Options the four captured substrings are the full prompt; for
+// non-default Options the seven uncaptured sections may contribute, and a
+// consumer correlating CompletePrefix stability with cached_input_tokens
+// must cross-check the model_switches counter (modelPromptAddendum changes
+// on a model switch) to disambiguate "no drift" from "drift in an
+// uncaptured section."
 
-func computePrefixFingerprint(subs promptSubstrings) prefixFingerprint {
-	systemPrompt := sha256hex(subs.systemPrompt)
+func ComputePrefixFingerprint(options Options, exposed []zeroruntime.ToolDefinition) prefixFingerprint {
+	subs := buildPromptSubstrings(options, exposed)
 	base := sha256hex(subs.baseInstructions)
 	policy := sha256hex(subs.confirmationPolicy)
 	project := sha256hex(subs.projectContext)
@@ -206,10 +264,9 @@ func computePrefixFingerprint(subs promptSubstrings) prefixFingerprint {
 	toolsH := sha256hex(subs.tools)
 	schema := sha256hex(subs.schema)
 	complete := sha256hex(strings.Join([]string{
-		systemPrompt, toolsH, schema,
+		base, policy, project, skills, toolsH, schema,
 	}, "|"))
 	return prefixFingerprint{
-		SystemPromptHash:       systemPrompt,
 		BaseInstructionsHash:   base,
 		ConfirmationPolicyHash: policy,
 		ProjectContextHash:     project,
@@ -225,7 +282,6 @@ func computePrefixFingerprint(subs promptSubstrings) prefixFingerprint {
 // in loop.go) so the trace package does not need to import this one. The field
 // names match the trace.PrefixHash JSON tags 1:1.
 type prefixFingerprint struct {
-	SystemPromptHash       string
 	BaseInstructionsHash   string
 	ConfirmationPolicyHash string
 	ProjectContextHash     string

internal/agent/prompt_fingerprint_test.go

@@ -28,28 +28,38 @@ func TestComputePrefixFingerprintStableAcrossCalls(t *testing.T) {
 	}
 }
 
-// Tool order and descriptions are provider-visible and therefore part of the
-// cacheable prefix identity.
-func TestComputePrefixFingerprintToolsHashMatchesEmittedOrderAndDescriptions(t *testing.T) {
+// TestComputePrefixFingerprintToolsHashIsNameSetSensitive asserts the
+// name-set sensitivity of the ToolsHash. The test name used to read as
+// "reorder changes hash" but the assertion is the opposite: the hash is
+// name-set sensitive, not name-order sensitive (toolSubstrings sorts
+// names before hashing). The partitioner is expected to produce a stable
+// order, so a name-order change is not a hash signal — a name-set change
+// is. The test asserts both: permutation of the same tools produces the
+// same hash, and adding a tool produces a different hash.
+func TestComputePrefixFingerprintToolsHashIsNameSetSensitive(t *testing.T) {
 	opts := Options{Cwd: t.TempDir(), SystemPrompt: "core"}
 	a := []zeroruntime.ToolDefinition{
-		{Name: "read_file", Description: "read", Parameters: map[string]any{"schema": "schema-read"}},
-		{Name: "grep", Description: "search", Parameters: map[string]any{"schema": "schema-grep"}},
+		{Name: "read_file", Parameters: map[string]any{"schema": "schema-read"}},
+		{Name: "grep", Parameters: map[string]any{"schema": "schema-grep"}},
 	}
 	b := []zeroruntime.ToolDefinition{
-		{Name: "grep", Description: "search", Parameters: map[string]any{"schema": "schema-grep"}},
-		{Name: "read_file", Description: "read", Parameters: map[string]any{"schema": "schema-read"}},
+		{Name: "grep", Parameters: map[string]any{"schema": "schema-grep"}},
+		{Name: "read_file", Parameters: map[string]any{"schema": "schema-read"}},
 	}
+	// toolSubstrings sorts names internally, so the ToolsHash is identical
+	// for a permutation of the same tools. We assert that here to document
+	// the contract: name-order independence, name-set sensitivity.
 	fpA := ComputePrefixFingerprint(opts, a)
 	fpB := ComputePrefixFingerprint(opts, b)
-	if fpA.ToolsHash == fpB.ToolsHash {
-		t.Fatalf("ToolsHash must change when emitted order changes: a=%s b=%s", fpA.ToolsHash, fpB.ToolsHash)
+	if fpA.ToolsHash != fpB.ToolsHash {
+		t.Fatalf("ToolsHash must be name-set sensitive, not name-order sensitive: a=%s b=%s", fpA.ToolsHash, fpB.ToolsHash)
 	}
-	c := append([]zeroruntime.ToolDefinition(nil), a...)
-	c[0].Description = "read a file"
+	// A different tool set must produce a different ToolsHash.
+	c := append([]zeroruntime.ToolDefinition{}, a...)
+	c = append(c, zeroruntime.ToolDefinition{Name: "bash", Parameters: map[string]any{"schema": "schema-bash"}})
 	fpC := ComputePrefixFingerprint(opts, c)
 	if fpA.ToolsHash == fpC.ToolsHash {
-		t.Fatalf("ToolsHash must change when a description changes: a=%s c=%s", fpA.ToolsHash, fpC.ToolsHash)
+		t.Fatalf("ToolsHash must change when the tool set changes: a=%s c=%s", fpA.ToolsHash, fpC.ToolsHash)
 	}
 }
 
@@ -91,35 +101,25 @@ func TestComputePrefixFingerprintSystemPromptChangeChangesBaseHash(t *testing.T)
 	}
 }
 
-// CompletePrefixHash represents the exact joined system prompt and the ordered
-// tool definitions. Narrower system-section hashes remain diagnostic only.
+// TestComputePrefixFingerprintCompleteIsAggregateOfSubHashes asserts the
+// canonical-join property: CompletePrefixHash must depend on every sub-hash.
+// This is the regression catch for "did anyone reorder or drop a sub-hash
+// from the canonical join without updating CompletePrefixHash."
 func TestComputePrefixFingerprintCompleteIsAggregateOfSubHashes(t *testing.T) {
 	opts := Options{Cwd: t.TempDir(), SystemPrompt: "core"}
 	exposed := []zeroruntime.ToolDefinition{{Name: "read_file", Parameters: map[string]any{"k": "v"}}}
 	fp := ComputePrefixFingerprint(opts, exposed)
 	// Manually compute what the canonical join should be.
 	expected := sha256hex(strings.Join([]string{
-		fp.SystemPromptHash,
+		fp.BaseInstructionsHash,
+		fp.ConfirmationPolicyHash,
+		fp.ProjectContextHash,
+		fp.SkillsHash,
 		fp.ToolsHash,
 		fp.SchemaHash,
 	}, "|"))
 	if fp.CompletePrefixHash != expected {
-		t.Fatalf("CompletePrefixHash must equal sha256hex of the exact system, tools, and schema hashes:\n  got:      %s\n  expected: %s", fp.CompletePrefixHash, expected)
-	}
-}
-
-func TestComputePrefixFingerprintCoversEverySystemPromptSection(t *testing.T) {
-	base := Options{Cwd: t.TempDir(), SystemPrompt: "core", Model: "gpt-5"}
-	changed := base
-	changed.ResponseStyle = "concise"
-
-	fpBase := ComputePrefixFingerprint(base, nil)
-	fpChanged := ComputePrefixFingerprint(changed, nil)
-	if fpBase.SystemPromptHash == fpChanged.SystemPromptHash {
-		t.Fatal("SystemPromptHash must change when any rendered system section changes")
-	}
-	if fpBase.CompletePrefixHash == fpChanged.CompletePrefixHash {
-		t.Fatal("CompletePrefixHash must include the exact full system prompt")
+		t.Fatalf("CompletePrefixHash must equal sha256hex of the canonical join of the other six:\n  got:      %s\n  expected: %s", fp.CompletePrefixHash, expected)
 	}
 }
 
@@ -141,9 +141,6 @@ func TestBuildPromptSubstringsDefaultOptions(t *testing.T) {
 	if subs.baseInstructions != "test core" {
 		t.Fatalf("baseInstructions substring must equal the core system prompt: got %q want %q", subs.baseInstructions, "test core")
 	}
-	if subs.systemPrompt != buildSystemPrompt(opts) {
-		t.Fatal("systemPrompt substring must equal the exact joined system prompt")
-	}
 	if subs.confirmationPolicy == "" {
 		t.Fatalf("confirmationPolicy substring must be non-empty (the embedded policy is always present)")
 	}
@@ -154,9 +151,6 @@ func TestBuildPromptSubstringsDefaultOptions(t *testing.T) {
 	// sha256hex of the substrings, with no truncation or reformatting
 	// between the two layers.
 	fp := ComputePrefixFingerprint(opts, nil)
-	if fp.SystemPromptHash != sha256hex(subs.systemPrompt) {
-		t.Fatalf("SystemPromptHash in fingerprint must equal sha256hex of the joined prompt")
-	}
 	if fp.BaseInstructionsHash != sha256hex(subs.baseInstructions) {
 		t.Fatalf("BaseInstructionsHash in fingerprint must equal sha256hex of the substring")
 	}

internal/agent/system_prompt.go

@@ -70,22 +70,6 @@ const (
 // guidelines), and the safety confirmation policy. It is built once per run so
 // every turn shares one (cacheable) system turn.
 func buildSystemPrompt(options Options) string {
-	return buildSystemPromptParts(options).prompt
-}
-
-// systemPromptParts retains both the exact joined prompt and the diagnostic
-// sections used by prompt-prefix tracing. Building these together prevents the
-// tracing path from re-reading workspace state and guarantees that the hash is
-// computed from the same prompt bytes placed in the request.
-type systemPromptParts struct {
-	prompt             string
-	baseInstructions   string
-	confirmationPolicy string
-	projectContext     string
-	skills             string
-}
-
-func buildSystemPromptParts(options Options) systemPromptParts {
 	core := strings.TrimSpace(options.SystemPrompt)
 	if core == "" {
 		core = strings.TrimSpace(coreSystemPrompt)
@@ -113,31 +97,22 @@ func buildSystemPromptParts(options Options) systemPromptParts {
 	if user := userGuidelines(); user != "" {
 		sections = append(sections, user)
 	}
-	project := workspaceContext(options.Cwd)
-	if project != "" {
-		sections = append(sections, project)
+	if ws := workspaceContext(options.Cwd); ws != "" {
+		sections = append(sections, ws)
 	}
 	if delegation := specialistDelegationContext(options); delegation != "" {
 		sections = append(sections, delegation)
 	}
-	skillsBlock := skillsContext(options)
-	if skillsBlock != "" {
+	if skillsBlock := skillsContext(options); skillsBlock != "" {
 		sections = append(sections, skillsBlock)
 	}
 	if style := responseStyleContext(options); style != "" {
 		sections = append(sections, style)
 	}
-	policy := strings.TrimSpace(confirmationPolicy)
-	if policy != "" {
+	if policy := strings.TrimSpace(confirmationPolicy); policy != "" {
 		sections = append(sections, policy)
 	}
-	return systemPromptParts{
-		prompt:             strings.Join(sections, "\n\n"),
-		baseInstructions:   core,
-		confirmationPolicy: policy,
-		projectContext:     project,
-		skills:             skillsBlock,
-	}
+	return strings.Join(sections, "\n\n")
 }
 
 // responseStyleContext renders the operator-selected reply style (TUI /style) as

internal/providers/openai/provider_test.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
-	"reflect"
 	"strings"
 	"testing"
 	"time"
@@ -1376,61 +1375,3 @@ func TestOpenAIRequestPromptCacheKey(t *testing.T) {
 		}
 	}
 }
-
-func TestOpenAIRequestPreservesCacheablePrefixAcrossTurns(t *testing.T) {
-	provider, err := New(Options{Model: "gpt-test"})
-	if err != nil {
-		t.Fatalf("New returned error: %v", err)
-	}
-	tools := []zeroruntime.ToolDefinition{{
-		Name:        "read_file",
-		Description: "Read a file.",
-		Parameters: map[string]any{
-			"type": "object",
-			"properties": map[string]any{
-				"path": map[string]any{"type": "string"},
-			},
-		},
-	}}
-	firstMessages := []zeroruntime.Message{
-		{Role: zeroruntime.MessageRoleSystem, Content: "stable system prompt"},
-		{Role: zeroruntime.MessageRoleUser, Content: "first turn"},
-	}
-	secondMessages := append([]zeroruntime.Message(nil), firstMessages...)
-	secondMessages = append(secondMessages,
-		zeroruntime.Message{Role: zeroruntime.MessageRoleAssistant, Content: "first response"},
-		zeroruntime.Message{Role: zeroruntime.MessageRoleUser, Content: "second turn"},
-	)
-
-	marshalBody := func(messages []zeroruntime.Message) map[string]any {
-		t.Helper()
-		mapped := provider.openAIRequest(zeroruntime.CompletionRequest{
-			Messages:       messages,
-			Tools:          tools,
-			PromptCacheKey: "session-stable-prefix",
-		})
-		data, marshalErr := json.Marshal(mapped)
-		if marshalErr != nil {
-			t.Fatalf("marshal request: %v", marshalErr)
-		}
-		var body map[string]any
-		if unmarshalErr := json.Unmarshal(data, &body); unmarshalErr != nil {
-			t.Fatalf("unmarshal request: %v", unmarshalErr)
-		}
-		return body
-	}
-
-	first := marshalBody(firstMessages)
-	second := marshalBody(secondMessages)
-	firstWireMessages := first["messages"].([]any)
-	secondWireMessages := second["messages"].([]any)
-	if !reflect.DeepEqual(secondWireMessages[:len(firstWireMessages)], firstWireMessages) {
-		t.Fatalf("first wire messages must be an exact prefix of the second:\nfirst=%#v\nsecond=%#v", firstWireMessages, secondWireMessages)
-	}
-	if !reflect.DeepEqual(first["tools"], second["tools"]) {
-		t.Fatalf("wire tool definitions drifted:\nfirst=%#v\nsecond=%#v", first["tools"], second["tools"])
-	}
-	if first["prompt_cache_key"] != second["prompt_cache_key"] || first["prompt_cache_key"] != "session-stable-prefix" {
-		t.Fatalf("wire prompt cache key must remain stable: first=%#v second=%#v", first["prompt_cache_key"], second["prompt_cache_key"])
-	}
-}

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/registry.go

@@ -2,7 +2,6 @@ package tools
 
 import (
 	"context"
-	"sort"
 
 	"github.com/Gitlawb/zero/internal/redaction"
 	"github.com/Gitlawb/zero/internal/sandbox"
@@ -114,9 +113,6 @@ func (registry *Registry) All() []Tool {
 	for _, tool := range registry.tools {
 		tools = append(tools, tool)
 	}
-	sort.Slice(tools, func(left, right int) bool {
-		return tools[left].Name() < tools[right].Name()
-	})
 	return tools
 }
 

internal/tools/registry_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"os"
 	"path/filepath"
-	"slices"
 	"strings"
 	"testing"
 
@@ -73,23 +72,6 @@ func TestCoreReadOnlyToolsExposeSafeMetadata(t *testing.T) {
 	}
 }
 
-func TestRegistryAllReturnsToolsSortedByName(t *testing.T) {
-	registry := NewRegistry()
-	for _, name := range []string{"write_file", "bash", "read_file"} {
-		registry.Register(fakePlainTool{baseTool: baseTool{name: name, description: name}})
-	}
-
-	all := registry.All()
-	got := make([]string, 0, len(all))
-	for _, tool := range all {
-		got = append(got, tool.Name())
-	}
-	want := []string{"bash", "read_file", "write_file"}
-	if !slices.Equal(got, want) {
-		t.Fatalf("Registry.All() order = %v, want %v", got, want)
-	}
-}
-
 func TestCoreNetworkToolsExposeSafetyMetadata(t *testing.T) {
 	// web_search is only registered when a backend is configured.
 	t.Setenv("ZERO_WEBSEARCH_BASE_URL", "https://search.example/api")

internal/trace/emit.go

@@ -106,7 +106,6 @@ func WriteNDJSON(w io.Writer, t *TurnTrace) error {
 	for _, p := range t.PrefixHashes {
 		if err := enc.Encode(map[string]any{
 			"type":                "prefix_hash",
-			"system_prompt":       p.SystemPromptHash,
 			"base_instructions":   p.BaseInstructionsHash,
 			"confirmation_policy": p.ConfirmationPolicyHash,
 			"project_context":     p.ProjectContextHash,

internal/trace/parse.go

@@ -101,14 +101,13 @@ func ReadNDJSON(r io.Reader) (*TurnTrace, error) {
 			if !sawTraceHeader {
 				return nil, errors.New("parse trace: not a valid NDJSON trace (no type:trace header)")
 			}
-			// Round-trip the prefix-hash fields. Missing fields are
+			// Round-trip the seven prefix-hash fields. Missing fields are
 			// accepted as empty strings (a partially-written trace from a
 			// crashed emitter must not fatal a parse). The decoder tolerates
-			// any shape the encoder produced and the keys are the
+			// any shape the encoder produced and the seven keys are the
 			// contract — adding a new field is non-breaking; renaming one
 			// requires a schema version bump.
 			t.PrefixHashes = append(t.PrefixHashes, PrefixHash{
-				SystemPromptHash:       stringField(obj, "system_prompt"),
 				BaseInstructionsHash:   stringField(obj, "base_instructions"),
 				ConfirmationPolicyHash: stringField(obj, "confirmation_policy"),
 				ProjectContextHash:     stringField(obj, "project_context"),

internal/trace/trace.go

@@ -121,15 +121,31 @@ type TurnTrace struct {
 }
 
 // PrefixHash is one fingerprint of the prompt prefix emitted by an agent run.
-// SystemPromptHash covers the exact joined system-message content, including
-// every dynamic section. ToolsHash covers tool names and descriptions in their
-// emitted order; SchemaHash covers their canonical parameter schemas in that
-// same order. CompletePrefixHash combines those three provider-visible parts.
-// The remaining hashes retain narrower diagnostic boundaries so a trace can
-// identify common sources of system-prompt drift. All hashes are hex-encoded
-// SHA-256 and emitted as a "prefix_hash" NDJSON event.
+// The seven fields decompose the cacheable part of the request so a downstream
+// observer can detect which sub-component drifted turn-over-turn:
+// base_instructions (the embedded core system prompt), confirmation_policy,
+// project_context (AGENTS.md / ZERO.md chain), skills, tools, schema (tool
+// JSON schemas), and complete_prefix (SHA-256 of the canonical concatenation
+// of the other six). All hashes are hex-encoded SHA-256.
+//
+// Scope: the fingerprint covers 4 of 11 sections of buildSystemPrompt. The
+// seven sections NOT covered — modelPromptAddendum, sessionRuntimeContext,
+// approvedCommandPrefixContext, workspaceSeedContext, userGuidelines,
+// specialistDelegationContext, responseStyleContext — fire only for
+// non-default Options. For default Options (the common case), the four
+// captured substrings are the full prompt and the fingerprint is accurate.
+//
+// A run with a stable CompletePrefix across turns means the four captured
+// sub-components are byte-identical. A run where CompletePrefix changes
+// names the captured sub-component that drifted, but does NOT rule out
+// drift in the seven uncaptured sections. modelPromptAddendum in particular
+// changes on a model switch (a model_switches counter the trace already
+// emits), so a consumer correlating CompletePrefix stability with
+// cached_input_tokens must cross-check the model_switches counter to
+// disambiguate "no drift" from "drift in an uncaptured section." The fields
+// are emitted as a "prefix_hash" event in the NDJSON trace (see
+// WriteNDJSON).
 type PrefixHash struct {
-	SystemPromptHash       string `json:"system_prompt"`
 	BaseInstructionsHash   string `json:"base_instructions"`
 	ConfirmationPolicyHash string `json:"confirmation_policy"`
 	ProjectContextHash     string `json:"project_context"`

internal/trace/trace_test.go

@@ -374,7 +374,6 @@ func TestReadNDJSONRoundTrip(t *testing.T) {
 	// event type. Insertion order is preserved by the parser (no sort
 	// is applied on read or write).
 	r.EmitPrefixHash(PrefixHash{
-		SystemPromptHash:       "system1",
 		BaseInstructionsHash:   "b1",
 		ConfirmationPolicyHash: "c1",
 		ProjectContextHash:     "p1",
@@ -384,7 +383,6 @@ func TestReadNDJSONRoundTrip(t *testing.T) {
 		CompletePrefixHash:     "complete1",
 	})
 	r.EmitPrefixHash(PrefixHash{
-		SystemPromptHash:       "system2",
 		BaseInstructionsHash:   "b2",
 		ConfirmationPolicyHash: "c2",
 		ProjectContextHash:     "p2",
@@ -429,10 +427,10 @@ func TestReadNDJSONRoundTrip(t *testing.T) {
 	if parsed.PrefixHashes[0].CompletePrefixHash != "complete1" || parsed.PrefixHashes[1].CompletePrefixHash != "complete2" {
 		t.Fatalf("prefix_hash insertion order lost: got %+v want [complete1, complete2]", parsed.PrefixHashes)
 	}
-	if parsed.PrefixHashes[0].SystemPromptHash != "system1" || parsed.PrefixHashes[0].BaseInstructionsHash != "b1" || parsed.PrefixHashes[0].SchemaHash != "x1" {
+	if parsed.PrefixHashes[0].BaseInstructionsHash != "b1" || parsed.PrefixHashes[0].SchemaHash != "x1" {
 		t.Fatalf("prefix_hash sub-hashes lost on first event: got %+v", parsed.PrefixHashes[0])
 	}
-	if parsed.PrefixHashes[1].SystemPromptHash != "system2" || parsed.PrefixHashes[1].BaseInstructionsHash != "b2" || parsed.PrefixHashes[1].SchemaHash != "x2" {
+	if parsed.PrefixHashes[1].BaseInstructionsHash != "b2" || parsed.PrefixHashes[1].SchemaHash != "x2" {
 		t.Fatalf("prefix_hash sub-hashes lost on second event: got %+v", parsed.PrefixHashes[1])
 	}
 }