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