From 525331461a8feaf2c7c238b9dad4f5451ff09106 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 6 May 2026 19:19:50 -0400 Subject: [PATCH] fix: expose Codex diff snapshot directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Large diff reviews can hand Codex a filesystem path instead of inlining the whole diff, but Codex runs with the repository as its sandbox root. When the snapshot lived directly in the system temp directory, Codex could fail to read it unless the whole temp directory was exposed. Write each external diff snapshot into its own roborev temp subdirectory and pass that exact directory to Codex with --add-dir. This keeps the non-agentic read-only sandbox while avoiding broad access to all of /tmp. Validation: go fmt ./...; go vet ./...; go test ./... 🤖 Generated with [OpenAI Codex](https://openai.com/codex) Co-authored-by: OpenAI Codex --- internal/agent/codex.go | 49 +++++++++++++++++++++++++++++++++- internal/agent/codex_test.go | 48 ++++++++++++++++++++++++++++++--- internal/daemon/worker_test.go | 3 +++ internal/prompt/prompt.go | 13 ++++++--- 4 files changed, 105 insertions(+), 8 deletions(-) diff --git a/internal/agent/codex.go b/internal/agent/codex.go index 92a82110..a049ec1b 100644 --- a/internal/agent/codex.go +++ b/internal/agent/codex.go @@ -7,7 +7,10 @@ import ( "fmt" "io" "log" + "os" "os/exec" + "path/filepath" + "regexp" "strings" "sync" ) @@ -118,12 +121,14 @@ func (a *CodexAgent) CommandLine() string { func (a *CodexAgent) buildArgs( repoPath string, agenticMode, autoApprove, sandboxBroken bool, + prompt string, ) []string { return a.commandArgs(codexArgOptions{ repoPath: repoPath, agenticMode: agenticMode, autoApprove: autoApprove, sandboxBroken: sandboxBroken, + addDirs: codexDiffSnapshotDirs(prompt), }) } @@ -132,6 +137,7 @@ type codexArgOptions struct { agenticMode bool autoApprove bool sandboxBroken bool + addDirs []string preview bool } @@ -159,6 +165,9 @@ func (a *CodexAgent) commandArgs(opts codexArgOptions) []string { if !opts.preview { args = append(args, "-C", opts.repoPath) } + for _, dir := range opts.addDirs { + args = append(args, "--add-dir", dir) + } if a.Model != "" { args = append(args, "-m", a.Model) } @@ -176,6 +185,44 @@ func (a *CodexAgent) commandArgs(opts codexArgOptions) []string { return args } +var codexDiffSnapshotRE = regexp.MustCompile("`([^`]*roborev-snapshot-[^`]*\\.diff)`") + +func codexDiffSnapshotDirs(reviewPrompt string) []string { + matches := codexDiffSnapshotRE.FindAllStringSubmatch(reviewPrompt, -1) + if len(matches) == 0 { + return nil + } + seen := make(map[string]struct{}, len(matches)) + var dirs []string + for _, match := range matches { + if len(match) < 2 { + continue + } + path := filepath.Clean(match[1]) + if !filepath.IsAbs(path) { + continue + } + base := filepath.Base(path) + if !strings.HasPrefix(base, "roborev-snapshot-") || filepath.Ext(base) != ".diff" { + continue + } + dir := filepath.Dir(path) + if !strings.HasPrefix(filepath.Base(dir), "roborev-snapshot-") { + continue + } + info, err := os.Stat(path) + if err != nil || info.IsDir() { + continue + } + if _, ok := seen[dir]; ok { + continue + } + seen[dir] = struct{}{} + dirs = append(dirs, dir) + } + return dirs +} + func codexSupportsDangerousFlag(ctx context.Context, command string) (bool, error) { if cached, ok := codexDangerousSupport.Load(command); ok { return cached.(bool), nil @@ -240,7 +287,7 @@ func (a *CodexAgent) Review(ctx context.Context, repoPath, commitSHA, prompt str if sandboxBroken && autoApprove { log.Printf("codex: sandbox disabled via config, using %s", codexAutoApproveFlag) } - args := a.buildArgs(repoPath, agenticMode, autoApprove, sandboxBroken) + args := a.buildArgs(repoPath, agenticMode, autoApprove, sandboxBroken, prompt) runResult, runErr := runStreamingCLI(ctx, streamingCLISpec{ Name: "codex", diff --git a/internal/agent/codex_test.go b/internal/agent/codex_test.go index 1a9521dd..d11b015b 100644 --- a/internal/agent/codex_test.go +++ b/internal/agent/codex_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "slices" "strings" "testing" "time" @@ -59,7 +60,7 @@ func TestCodex_buildArgs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - args := a.buildArgs("/repo", tt.agentic, tt.autoApprove, tt.sandboxBroken) + args := a.buildArgs("/repo", tt.agentic, tt.autoApprove, tt.sandboxBroken, "") for _, flag := range tt.wantFlags { assert.Contains(t, args, flag, "buildArgs() missing expected flag %q, args: %v", flag, args) @@ -77,7 +78,7 @@ func TestCodex_buildArgs(t *testing.T) { func TestCodexBuildArgsWithSessionResume(t *testing.T) { a := NewCodexAgent("codex").WithSessionID("session-123").(*CodexAgent) - args := a.buildArgs("/repo", false, true, false) + args := a.buildArgs("/repo", false, true, false, "") require.GreaterOrEqual(t, len(args), 3) assert.Equal(t, "exec", args[0]) @@ -87,10 +88,51 @@ func TestCodexBuildArgsWithSessionResume(t *testing.T) { assert.Equal(t, "-", args[len(args)-1], "expected stdin marker '-' at end of args, got %v", args) } +func TestCodexBuildArgsAddsDiffSnapshotDirectory(t *testing.T) { + a := NewCodexAgent("codex") + snapshotDir, err := os.MkdirTemp("", "roborev-snapshot-*") + require.NoError(t, err) + t.Cleanup(func() { os.RemoveAll(snapshotDir) }) + diffFile := snapshotDir + string(os.PathSeparator) + "roborev-snapshot-content.diff" + require.NoError(t, os.WriteFile(diffFile, []byte("diff --git a/x b/x\n"), 0o600)) + + args := a.buildArgs( + "/repo", + false, + true, + false, + "Read the diff from: `"+diffFile+"`", + ) + + addDirIndex := slices.Index(args, "--add-dir") + require.NotEqual(t, -1, addDirIndex, "expected --add-dir in args: %v", args) + require.Less(t, addDirIndex+1, len(args), "expected --add-dir value in args: %v", args) + assert.Equal(t, snapshotDir, args[addDirIndex+1]) + assert.Less(t, addDirIndex, len(args)-1, "--add-dir must appear before stdin marker: %v", args) + assert.Equal(t, "-", args[len(args)-1], "expected stdin marker '-' at end of args, got %v", args) +} + +func TestCodexBuildArgsIgnoresDiffSnapshotOutsidePrivateDirectory(t *testing.T) { + a := NewCodexAgent("codex") + diffFile, err := os.CreateTemp("", "roborev-snapshot-*.diff") + require.NoError(t, err) + t.Cleanup(func() { os.Remove(diffFile.Name()) }) + + args := a.buildArgs( + "/repo", + false, + true, + false, + "Read the diff from: `"+diffFile.Name()+"`", + ) + + assert.NotContains(t, args, "--add-dir") +} + func TestCodexBuildArgsRejectsInvalidSessionResume(t *testing.T) { a := NewCodexAgent("codex").WithSessionID("-bad-session").(*CodexAgent) - args := a.buildArgs("/repo", false, true, false) + args := a.buildArgs("/repo", false, true, false, "") require.GreaterOrEqual(t, len(args), 2) assert.Equal(t, "exec", args[0]) diff --git a/internal/daemon/worker_test.go b/internal/daemon/worker_test.go index d3151c0c..dbece0cc 100644 --- a/internal/daemon/worker_test.go +++ b/internal/daemon/worker_test.go @@ -556,6 +556,9 @@ func TestWriteDiffSnapshot_WritesExternalReadableTempFile(t *testing.T) { assert.False(t, strings.HasPrefix(filepath.Clean(diffFile), filepath.Clean(gitDir)), "snapshot should not live in git dir: got %s, git dir %s", diffFile, gitDir) + assert.True(t, + strings.HasPrefix(filepath.Base(filepath.Dir(diffFile)), "roborev-snapshot-"), + "snapshot should live in a private roborev temp dir, got %s", diffFile) data, err := os.ReadFile(diffFile) require.NoError(t, err) assert.Contains(t, string(data), "diff --git") diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 2786379f..e8cf5e90 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -189,21 +189,26 @@ func WriteDiffSnapshot(repoPath, gitRef string, excludes []string) (string, func } func writeExternalDiffSnapshot(diff string) (string, func(), error) { - f, err := os.CreateTemp("", "roborev-snapshot-*.diff") + dir, err := os.MkdirTemp("", "roborev-snapshot-*") if err != nil { + return "", nil, fmt.Errorf("create snapshot dir: %w", err) + } + diffFile := dir + string(os.PathSeparator) + "roborev-snapshot-content.diff" + f, err := os.OpenFile(diffFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + os.RemoveAll(dir) return "", nil, fmt.Errorf("create snapshot: %w", err) } - diffFile := f.Name() _, writeErr := f.WriteString(diff) closeErr := f.Close() if writeErr != nil || closeErr != nil { - os.Remove(diffFile) + os.RemoveAll(dir) if writeErr != nil { return "", nil, fmt.Errorf("write snapshot: %w", writeErr) } return "", nil, fmt.Errorf("close snapshot: %w", closeErr) } - return diffFile, func() { os.Remove(diffFile) }, nil + return diffFile, func() { os.RemoveAll(dir) }, nil } // BuildDirtyWithSnapshot builds a dirty review prompt, writing the diff to a snapshot file