From b2888089c5b6108ebf291bfa34756b54e6a18086 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Tue, 24 Mar 2026 11:58:53 -0400 Subject: [PATCH] refactor(agent): unify preview and runtime arg builders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bd-pui.5 Share per-agent argument assembly between CommandLine previews and runtime builders while keeping preview-only omissions for repo paths, stdin markers, and resolved session paths. Validation: - go fmt ./... - go vet ./... - go test ./... - roborev fix --open --list (no open jobs) 🤖 Generated with [OpenAI Codex](https://openai.com/codex) Co-authored-by: OpenAI Codex --- internal/agent/codex.go | 55 ++++++++++++++++++---------------- internal/agent/codex_test.go | 12 ++++++++ internal/agent/copilot.go | 39 +++++++++++++----------- internal/agent/copilot_test.go | 10 +++++++ internal/agent/cursor.go | 13 +------- internal/agent/cursor_test.go | 7 +++++ internal/agent/droid.go | 10 +------ internal/agent/droid_test.go | 8 +++++ internal/agent/pi.go | 14 ++++----- internal/agent/pi_test.go | 17 +++++++++++ 10 files changed, 112 insertions(+), 73 deletions(-) diff --git a/internal/agent/codex.go b/internal/agent/codex.go index a429af3b8..0d32cb2a7 100644 --- a/internal/agent/codex.go +++ b/internal/agent/codex.go @@ -105,27 +105,30 @@ func (a *CodexAgent) CommandName() string { func (a *CodexAgent) CommandLine() string { agenticMode := a.Agentic || AllowUnsafeAgents() - // Show representative args (repo path is a runtime value) - sessionID := sanitizedResumeSessionID(a.SessionID) - args := []string{"exec", "--json"} - if sessionID != "" { - args = []string{"exec", "resume", "--json", sessionID} - } - if agenticMode { - args = append(args, codexDangerousFlag) - } else { - args = append(args, "--sandbox", "read-only") - } - if a.Model != "" { - args = append(args, "-m", a.Model) - } - if effort := a.codexReasoningEffort(); effort != "" { - args = append(args, "-c", fmt.Sprintf(`model_reasoning_effort="%s"`, effort)) - } + args := a.commandArgs(codexArgOptions{ + agenticMode: agenticMode, + autoApprove: !agenticMode, + preview: true, + }) return a.Command + " " + strings.Join(args, " ") } func (a *CodexAgent) buildArgs(repoPath string, agenticMode, autoApprove bool) []string { + return a.commandArgs(codexArgOptions{ + repoPath: repoPath, + agenticMode: agenticMode, + autoApprove: autoApprove, + }) +} + +type codexArgOptions struct { + repoPath string + agenticMode bool + autoApprove bool + preview bool +} + +func (a *CodexAgent) commandArgs(opts codexArgOptions) []string { sessionID := sanitizedResumeSessionID(a.SessionID) args := []string{ "exec", @@ -134,18 +137,18 @@ func (a *CodexAgent) buildArgs(repoPath string, agenticMode, autoApprove bool) [ args = append(args, "resume") } args = append(args, "--json") - if agenticMode { + if opts.agenticMode { args = append(args, codexDangerousFlag) } - if autoApprove { + if opts.autoApprove { // Use read-only sandbox for review mode instead of --full-auto // (which implies --sandbox workspace-write). Background review // jobs run in the user's repo and must not take index.lock. args = append(args, "--sandbox", "read-only") } - args = append(args, - "-C", repoPath, - ) + if !opts.preview { + args = append(args, "-C", opts.repoPath) + } if a.Model != "" { args = append(args, "-m", a.Model) } @@ -155,9 +158,11 @@ func (a *CodexAgent) buildArgs(repoPath string, agenticMode, autoApprove bool) [ if sessionID != "" { args = append(args, sessionID) } - // "-" must come after all flags to read prompt from stdin - // This avoids Windows command line length limits (~32KB) - args = append(args, "-") + if !opts.preview { + // "-" must come after all flags to read prompt from stdin + // This avoids Windows command line length limits (~32KB) + args = append(args, "-") + } return args } diff --git a/internal/agent/codex_test.go b/internal/agent/codex_test.go index ee0c413f6..e58bb101f 100644 --- a/internal/agent/codex_test.go +++ b/internal/agent/codex_test.go @@ -89,6 +89,18 @@ func TestCodexBuildArgsRejectsInvalidSessionResume(t *testing.T) { assertNotContainsArg(t, args, "-bad-session") } +func TestCodexCommandLineOmitsRuntimeOnlyArgs(t *testing.T) { + a := NewCodexAgent("codex").WithSessionID("session-123").WithModel("o4-mini").(*CodexAgent) + + cmdLine := a.CommandLine() + + assert.Contains(t, cmdLine, "exec resume --json") + assert.Contains(t, cmdLine, "session-123") + 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) +} + func TestCodexSupportsDangerousFlagAllowsNonZeroHelp(t *testing.T) { cmdPath := writeTempCommand(t, "#!/bin/sh\necho \"usage "+codexDangerousFlag+"\"; exit 1\n") diff --git a/internal/agent/copilot.go b/internal/agent/copilot.go index ce48bdff8..4536ceefc 100644 --- a/internal/agent/copilot.go +++ b/internal/agent/copilot.go @@ -50,16 +50,7 @@ var copilotReviewDenyTools = []string{ // In review mode, destructive tools are denied. In agentic mode, all tools // are allowed without restriction. func (a *CopilotAgent) buildArgs(agenticMode bool) []string { - args := []string{"-s", "--allow-all-tools"} - if a.Model != "" { - args = append(args, "--model", a.Model) - } - if !agenticMode { - for _, tool := range copilotReviewDenyTools { - args = append(args, "--deny-tool", tool) - } - } - return args + return a.commandArgs(agenticMode, true) } // CopilotAgent runs code reviews using the GitHub Copilot CLI @@ -122,10 +113,8 @@ func (a *CopilotAgent) CommandName() string { } func (a *CopilotAgent) CommandLine() string { - var args []string - if a.Model != "" { - args = append(args, "--model", a.Model) - } + agenticMode := a.Agentic || AllowUnsafeAgents() + args := a.commandArgs(agenticMode, false) return a.Command + " " + strings.Join(args, " ") } @@ -139,11 +128,9 @@ func (a *CopilotAgent) Review(ctx context.Context, repoPath, commitSHA, prompt s var args []string if supported { - args = a.buildArgs(agenticMode) + args = a.commandArgs(agenticMode, true) } else { - if a.Model != "" { - args = append(args, "--model", a.Model) - } + args = a.commandArgs(agenticMode, false) } cmd := exec.CommandContext(ctx, a.Command, args...) @@ -175,6 +162,22 @@ func (a *CopilotAgent) Review(ctx context.Context, repoPath, commitSHA, prompt s return result, nil } +func (a *CopilotAgent) commandArgs(agenticMode, includePermissions bool) []string { + args := []string{} + if includePermissions { + args = append(args, "-s", "--allow-all-tools") + } + if a.Model != "" { + args = append(args, "--model", a.Model) + } + if includePermissions && !agenticMode { + for _, tool := range copilotReviewDenyTools { + args = append(args, "--deny-tool", tool) + } + } + return args +} + func init() { Register(NewCopilotAgent("")) } diff --git a/internal/agent/copilot_test.go b/internal/agent/copilot_test.go index 8d2146576..bddc6b07c 100644 --- a/internal/agent/copilot_test.go +++ b/internal/agent/copilot_test.go @@ -75,6 +75,16 @@ func TestCopilotBuildArgs(t *testing.T) { }) } +func TestCopilotCommandLineUsesPreviewArgs(t *testing.T) { + a := NewCopilotAgent("copilot").WithModel("gpt-4o").(*CopilotAgent) + + cmdLine := a.CommandLine() + + assert.Contains(t, cmdLine, "--model gpt-4o") + assert.NotContains(t, cmdLine, "--allow-all-tools") + assert.NotContains(t, cmdLine, "--deny-tool") +} + func TestCopilotReview(t *testing.T) { skipIfWindows(t) diff --git a/internal/agent/cursor.go b/internal/agent/cursor.go index a0800008b..ce0296183 100644 --- a/internal/agent/cursor.go +++ b/internal/agent/cursor.go @@ -65,18 +65,7 @@ func (a *CursorAgent) CommandName() string { func (a *CursorAgent) CommandLine() string { agenticMode := a.Agentic || AllowUnsafeAgents() - // Show flags without the prompt (piped via stdin) - args := []string{"-p", "--output-format", "stream-json"} - model := a.Model - if model == "" { - model = "auto" - } - args = append(args, "--model", model) - if agenticMode { - args = append(args, "--force") - } else { - args = append(args, "--mode", "plan") - } + args := a.buildArgs(agenticMode) return a.Command + " " + strings.Join(args, " ") } diff --git a/internal/agent/cursor_test.go b/internal/agent/cursor_test.go index 3472665fb..22770026e 100644 --- a/internal/agent/cursor_test.go +++ b/internal/agent/cursor_test.go @@ -65,6 +65,13 @@ func TestCursorBuildArgs_Table(t *testing.T) { } } +func TestCursorCommandLineUsesBuildArgs(t *testing.T) { + a := NewCursorAgent("agent").WithModel("gpt-5.2-codex-high").WithAgentic(true).(*CursorAgent) + + want := strings.Join(a.buildArgs(true), " ") + assert.Equal(t, "agent "+want, a.CommandLine()) +} + func setupMockCursorAgent(t *testing.T, opts MockCLIOpts) (*CursorAgent, *MockCLIResult) { t.Helper() skipIfWindows(t) diff --git a/internal/agent/droid.go b/internal/agent/droid.go index aeb75ff0e..3f3cdf67a 100644 --- a/internal/agent/droid.go +++ b/internal/agent/droid.go @@ -65,15 +65,7 @@ func (a *DroidAgent) CommandName() string { func (a *DroidAgent) CommandLine() string { agenticMode := a.Agentic || AllowUnsafeAgents() - args := []string{"exec"} - if agenticMode { - args = append(args, "--auto", "medium") - } else { - args = append(args, "--auto", "low") - } - if effort := a.droidReasoningEffort(); effort != "" { - args = append(args, "--reasoning-effort", effort) - } + args := a.buildArgs(agenticMode) return a.Command + " " + strings.Join(args, " ") } diff --git a/internal/agent/droid_test.go b/internal/agent/droid_test.go index a62d913de..6854c22ff 100644 --- a/internal/agent/droid_test.go +++ b/internal/agent/droid_test.go @@ -2,6 +2,7 @@ package agent import ( "context" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "os" "path/filepath" @@ -64,6 +65,13 @@ func TestDroidBuildArgs(t *testing.T) { } } +func TestDroidCommandLineUsesBuildArgs(t *testing.T) { + a := NewDroidAgent("droid").WithReasoning(ReasoningThorough).WithAgentic(true).(*DroidAgent) + + want := strings.Join(a.buildArgs(true), " ") + assert.Equal(t, "droid "+want, a.CommandLine()) +} + func TestDroidName(t *testing.T) { a := NewDroidAgent("") require.Equal(t, "droid", a.Name(), "expected name 'droid', got %s", a.Name()) diff --git a/internal/agent/pi.go b/internal/agent/pi.go index 88f34c780..ffb92f8d3 100644 --- a/internal/agent/pi.go +++ b/internal/agent/pi.go @@ -107,18 +107,13 @@ func (a *PiAgent) CommandName() string { func (a *PiAgent) CommandLine() string { args := a.buildArgs("") - if len(args) == 0 { - args = []string{"-p", "--mode", "json"} - } return a.Command + " " + strings.Join(args, " ") } -func (a *PiAgent) buildArgs(repoPath string) []string { +func (a *PiAgent) buildArgs(sessionPath string) []string { args := []string{"-p", "--mode", "json"} - if repoPath != "" { - if sessionPath := resolvePiSessionPath(sanitizedResumeSessionID(a.SessionID)); sessionPath != "" { - args = append(args, "--session", sessionPath) - } + if sessionPath != "" { + args = append(args, "--session", sessionPath) } if a.Provider != "" { args = append(args, "--provider", a.Provider) @@ -165,7 +160,8 @@ func (a *PiAgent) Review( return "", fmt.Errorf("close temp prompt file: %w", err) } - args := a.buildArgs(repoPath) + sessionPath := resolvePiSessionPath(sanitizedResumeSessionID(a.SessionID)) + args := a.buildArgs(sessionPath) // Add the prompt file as an input argument (prefixed with @) // Pi treats @files as context/input. diff --git a/internal/agent/pi_test.go b/internal/agent/pi_test.go index 6b5007e40..959006d12 100644 --- a/internal/agent/pi_test.go +++ b/internal/agent/pi_test.go @@ -73,3 +73,20 @@ func TestPiReviewSessionFlag(t *testing.T) { assertContainsArg(t, args, "--mode") assertContainsArg(t, args, "json") } + +func TestPiCommandLineOmitsResolvedSessionPath(t *testing.T) { + dataDir := t.TempDir() + t.Setenv("PI_CODING_AGENT_DIR", dataDir) + + sessionID := "46109439-3160-40f0-81e7-7dfa4f3647b3" + sessionPath := filepath.Join(dataDir, "sessions", "--repo--", "2026-03-08T18-44-39-718Z_"+sessionID+".jsonl") + require.NoError(t, os.MkdirAll(filepath.Dir(sessionPath), 0o755)) + require.NoError(t, os.WriteFile(sessionPath, []byte("{}\n"), 0o644)) + + a := NewPiAgent("pi").WithSessionID(sessionID).(*PiAgent) + cmdLine := a.CommandLine() + + assert.NotContains(t, cmdLine, "--session") + assert.NotContains(t, cmdLine, sessionPath) + assert.Contains(t, cmdLine, "-p --mode json") +}