fix: read-only Codex sandbox with unified diff file fallback for oversized diffs

cmd/roborev/review.go

@@ -432,11 +432,22 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName
 	// Build prompt
 	pb := prompt.NewBuilderWithConfig(nil, cfg)
 	var reviewPrompt string
+	var snapshotCleanup func()
 	if diffContent != "" {
 		// Dirty review
-		reviewPrompt, err = pb.BuildDirty(repoPath, diffContent, 0, cfg.ReviewContextCount, a.Name(), reviewType)
+		dirtyResult, dirtyErr := pb.BuildDirtyWithSnapshot(repoPath, diffContent, 0, cfg.ReviewContextCount, a.Name(), reviewType)
+		reviewPrompt = dirtyResult.Prompt
+		snapshotCleanup = dirtyResult.Cleanup
+		err = dirtyErr
 	} else {
-		reviewPrompt, err = pb.Build(repoPath, gitRef, 0, cfg.ReviewContextCount, a.Name(), reviewType)
+		excludes := config.ResolveExcludePatterns(repoPath, cfg, reviewType)
+		result, buildErr := pb.BuildWithSnapshot(repoPath, gitRef, 0, cfg.ReviewContextCount, a.Name(), reviewType, excludes)
+		reviewPrompt = result.Prompt
+		snapshotCleanup = result.Cleanup
+		err = buildErr
+	}
+	if snapshotCleanup != nil {
+		defer snapshotCleanup()
 	}
 	if err != nil {
 		return fmt.Errorf("build prompt: %w", err)

internal/agent/agent_test_assert_helpers_test.go

@@ -56,30 +56,6 @@ func withUnsafeAgents(t *testing.T, allow bool) {
 	t.Cleanup(func() { SetAllowUnsafeAgents(prev) })
 }
 
-// assertContains fails the test if s does not contain substr.
-func assertContains(t *testing.T, s, substr string) {
-	t.Helper()
-	if !strings.Contains(s, substr) {
-		t.Errorf("expected string to contain %q\nDocument content:\n%s", substr, s)
-	}
-}
-
-// assertNotContains fails the test if s contains substr.
-func assertNotContains(t *testing.T, s, substr string) {
-	t.Helper()
-	if strings.Contains(s, substr) {
-		t.Errorf("expected %q to not contain %q", s, substr)
-	}
-}
-
-// assertEqual fails the test if got != want.
-func assertEqual(t *testing.T, got, want string) {
-	t.Helper()
-	if got != want {
-		t.Errorf("got %q, want %q", got, want)
-	}
-}
-
 // FailingWriter always returns an error on Write.
 type FailingWriter struct {
 	Err error

internal/agent/codex.go

@@ -141,12 +141,10 @@ func (a *CodexAgent) commandArgs(opts codexArgOptions) []string {
 		args = append(args, codexDangerousFlag)
 	}
 	if opts.autoApprove {
-		// Use full-access sandbox for review mode. The read-only
-		// sandbox blocks loopback networking, which prevents git
-		// commands from working in CI review jobs. roborev runs in
-		// trusted environments where the code is the operator's own,
-		// so sandbox enforcement is unnecessary.
-		args = append(args, "--sandbox", "danger-full-access")
+		// Use read-only sandbox for review mode. The worker
+		// writes the diff to a file that Codex can read within
+		// the sandbox, removing the need for git commands.
+		args = append(args, "--sandbox", "read-only")
 	}
 	if !opts.preview {
 		args = append(args, "-C", opts.repoPath)
@@ -183,7 +181,7 @@ func codexSupportsDangerousFlag(ctx context.Context, command string) (bool, erro
 }
 
 // codexSupportsNonInteractive checks that codex supports --sandbox,
-// needed for non-agentic review mode (--sandbox danger-full-access).
+// needed for non-agentic review mode (--sandbox read-only).
 func codexSupportsNonInteractive(ctx context.Context, command string) (bool, error) {
 	if cached, ok := codexAutoApproveSupport.Load(command); ok {
 		return cached.(bool), nil
@@ -212,8 +210,8 @@ func (a *CodexAgent) Review(ctx context.Context, repoPath, commitSHA, prompt str
 		}
 	}
 
-	// Non-agentic review mode uses --sandbox danger-full-access for
-	// non-interactive execution that can still run git commands.
+	// Non-agentic review mode uses --sandbox read-only for
+	// non-interactive sandboxed execution.
 	autoApprove := false
 	if !agenticMode {
 		supported, err := codexSupportsNonInteractive(ctx, a.Command)

internal/agent/codex_test.go

@@ -36,7 +36,7 @@ func TestCodex_buildArgs(t *testing.T) {
 			name:             "NonAgenticAutoApprove",
 			agentic:          false,
 			autoApprove:      true,
-			wantFlags:        []string{"--sandbox", "danger-full-access", "--json"},
+			wantFlags:        []string{"--sandbox", "read-only", "--json"},
 			wantMissingFlags: []string{codexDangerousFlag, codexAutoApproveFlag},
 		},
 		{
@@ -96,7 +96,7 @@ func TestCodexCommandLineOmitsRuntimeOnlyArgs(t *testing.T) {
 
 	assert.Contains(t, cmdLine, "exec resume --json")
 	assert.Contains(t, cmdLine, "session-123")
-	assert.Contains(t, cmdLine, "--sandbox danger-full-access")
+	assert.Contains(t, cmdLine, "--sandbox read-only")
 	assert.NotContains(t, cmdLine, " -C ")
 	assert.False(t, strings.HasSuffix(cmdLine, " -"), "command line should omit stdin marker: %q", cmdLine)
 }
@@ -133,8 +133,8 @@ func TestCodexReviewUsesSandboxNone(t *testing.T) {
 	args, err := os.ReadFile(mock.ArgsFile)
 	require.NoError(t, err)
 	argsStr := string(args)
-	assert.Contains(t, argsStr, "--sandbox danger-full-access",
-		"expected --sandbox danger-full-access in args, got: %s", strings.TrimSpace(argsStr))
+	assert.Contains(t, argsStr, "--sandbox read-only",
+		"expected --sandbox read-only in args, got: %s", strings.TrimSpace(argsStr))
 	assert.NotContains(t, argsStr, codexAutoApproveFlag,
 		"expected no %s in review mode, got: %s", codexAutoApproveFlag, strings.TrimSpace(argsStr))
 }

internal/agent/integration_helpers_test.go

@@ -0,0 +1,32 @@
+//go:build integration
+
+package agent
+
+import (
+	"strings"
+	"testing"
+)
+
+// assertContains fails the test if s does not contain substr.
+func assertContains(t *testing.T, s, substr string) {
+	t.Helper()
+	if !strings.Contains(s, substr) {
+		t.Errorf("expected string to contain %q\nDocument content:\n%s", substr, s)
+	}
+}
+
+// assertNotContains fails the test if s contains substr.
+func assertNotContains(t *testing.T, s, substr string) {
+	t.Helper()
+	if strings.Contains(s, substr) {
+		t.Errorf("expected %q to not contain %q", s, substr)
+	}
+}
+
+// assertEqual fails the test if got != want.
+func assertEqual(t *testing.T, got, want string) {
+	t.Helper()
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}

internal/agent/kilo_test.go

@@ -1,3 +1,5 @@
+//go:build integration
+
 package agent
 
 import (
@@ -11,42 +13,6 @@ import (
 	"testing"
 )
 
-func TestKiloModelFlag(t *testing.T) {
-	t.Parallel()
-	tests := []struct {
-		name         string
-		model        string
-		wantModel    bool
-		wantContains string
-	}{
-		{
-			name:      "no model omits flag",
-			model:     "",
-			wantModel: false,
-		},
-		{
-			name:         "explicit model includes flag",
-			model:        "anthropic/claude-sonnet-4-20250514",
-			wantModel:    true,
-			wantContains: "anthropic/claude-sonnet-4-20250514",
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			a := NewKiloAgent("kilo")
-			a.Model = tt.model
-			cl := a.CommandLine()
-			if tt.wantModel {
-				assertContains(t, cl, "--model")
-				assertContains(t, cl, tt.wantContains)
-			} else {
-				assertNotContains(t, cl, "--model")
-			}
-		})
-	}
-}
-
 func TestKiloReviewModelFlag(t *testing.T) {
 	t.Parallel()
 	skipIfWindows(t)
@@ -142,53 +108,6 @@ func TestKiloReviewStripsANSIFromJSONL(t *testing.T) {
 	assertNotContains(t, result, "\x1b[")
 }
 
-func TestKiloAgenticAutoFlag(t *testing.T) {
-	skipIfWindows(t)
-
-	withUnsafeAgents(t, false)
-	a := NewKiloAgent("kilo").WithAgentic(true).(*KiloAgent)
-	cl := a.CommandLine()
-	assertContains(t, cl, "--auto")
-
-	b := NewKiloAgent("kilo").WithAgentic(false).(*KiloAgent)
-	cl2 := b.CommandLine()
-	assertNotContains(t, cl2, "--auto")
-}
-
-func TestKiloVariantFlag(t *testing.T) {
-	t.Parallel()
-	tests := []struct {
-		name        string
-		reasoning   ReasoningLevel
-		wantVariant bool
-		wantValue   string
-	}{
-		{name: "thorough maps to high", reasoning: ReasoningThorough, wantVariant: true, wantValue: "high"},
-		{name: "fast maps to minimal", reasoning: ReasoningFast, wantVariant: true, wantValue: "minimal"},
-		{name: "standard omits variant", reasoning: ReasoningStandard, wantVariant: false},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			a := NewKiloAgent("kilo").WithReasoning(tt.reasoning).(*KiloAgent)
-			cl := a.CommandLine()
-			if tt.wantVariant {
-				assertContains(t, cl, "--variant")
-				assertContains(t, cl, tt.wantValue)
-			} else {
-				assertNotContains(t, cl, "--variant")
-			}
-		})
-	}
-}
-
-func TestKiloUsesJSONFormat(t *testing.T) {
-	t.Parallel()
-	a := NewKiloAgent("kilo")
-	cl := a.CommandLine()
-	assertContains(t, cl, "--format json")
-}
-
 func TestKiloReviewStderrOnExitZero(t *testing.T) {
 	t.Parallel()
 	skipIfWindows(t)

internal/agent/kilo_unit_test.go

@@ -0,0 +1,82 @@
+package agent
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestKiloModelFlag(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name         string
+		model        string
+		wantModel    bool
+		wantContains string
+	}{
+		{name: "no model omits flag", model: ""},
+		{
+			name:         "explicit model includes flag",
+			model:        "anthropic/claude-sonnet-4-20250514",
+			wantModel:    true,
+			wantContains: "anthropic/claude-sonnet-4-20250514",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			a := NewKiloAgent("kilo")
+			a.Model = tt.model
+			cl := a.CommandLine()
+			if tt.wantModel {
+				assert.Contains(t, cl, "--model")
+				assert.Contains(t, cl, tt.wantContains)
+			} else {
+				assert.NotContains(t, cl, "--model")
+			}
+		})
+	}
+}
+
+func TestKiloAgenticAutoFlag(t *testing.T) {
+	withUnsafeAgents(t, false)
+
+	a := NewKiloAgent("kilo").WithAgentic(true).(*KiloAgent)
+	assert.Contains(t, a.CommandLine(), "--auto")
+
+	b := NewKiloAgent("kilo").WithAgentic(false).(*KiloAgent)
+	assert.NotContains(t, b.CommandLine(), "--auto")
+}
+
+func TestKiloVariantFlag(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name        string
+		reasoning   ReasoningLevel
+		wantVariant bool
+		wantValue   string
+	}{
+		{name: "thorough maps to high", reasoning: ReasoningThorough, wantVariant: true, wantValue: "high"},
+		{name: "fast maps to minimal", reasoning: ReasoningFast, wantVariant: true, wantValue: "minimal"},
+		{name: "standard omits variant", reasoning: ReasoningStandard},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			a := NewKiloAgent("kilo").WithReasoning(tt.reasoning).(*KiloAgent)
+			cl := a.CommandLine()
+			if tt.wantVariant {
+				assert.Contains(t, cl, "--variant")
+				assert.Contains(t, cl, tt.wantValue)
+			} else {
+				assert.NotContains(t, cl, "--variant")
+			}
+		})
+	}
+}
+
+func TestKiloUsesJSONFormat(t *testing.T) {
+	t.Parallel()
+	a := NewKiloAgent("kilo")
+	assert.Contains(t, a.CommandLine(), "--format json")
+}