Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cmd/roborev/tui/render_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,11 @@ func commandLineForJob(job *storage.ReviewJob) string {
if job == nil {
return ""
}
// Prefer the actual command line saved by the daemon worker.
if job.CommandLine != "" {
return stripControlChars(job.CommandLine)
}
// Fallback: reconstruct locally (may not reflect daemon config).
a, err := agent.Get(job.Agent)
if err != nil {
return ""
Expand Down
13 changes: 13 additions & 0 deletions internal/agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ var (
)

var allowUnsafeAgents atomic.Bool
var codexSandboxDisabled atomic.Bool
var anthropicAPIKey atomic.Value

func AllowUnsafeAgents() bool {
Expand All @@ -109,6 +110,18 @@ func SetAllowUnsafeAgents(allow bool) {
allowUnsafeAgents.Store(allow)
}

// CodexSandboxDisabled returns true when the Codex bwrap sandbox
// should be skipped (e.g. on machines without unprivileged user
// namespace support). When true, Codex runs with --full-auto
// instead of --sandbox read-only.
func CodexSandboxDisabled() bool {
return codexSandboxDisabled.Load()
}

func SetCodexSandboxDisabled(v bool) {
codexSandboxDisabled.Store(v)
}

// AnthropicAPIKey returns the configured Anthropic API key, or empty string if not set
func AnthropicAPIKey() string {
if v := anthropicAPIKey.Load(); v != nil {
Expand Down
46 changes: 30 additions & 16 deletions internal/agent/codex.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"io"
"log"
"os/exec"
"strings"
"sync"
Expand Down Expand Up @@ -106,26 +107,32 @@ func (a *CodexAgent) CommandName() string {
func (a *CodexAgent) CommandLine() string {
agenticMode := a.Agentic || AllowUnsafeAgents()
args := a.commandArgs(codexArgOptions{
agenticMode: agenticMode,
autoApprove: !agenticMode,
preview: true,
agenticMode: agenticMode,
autoApprove: !agenticMode,
sandboxBroken: CodexSandboxDisabled(),
preview: true,
})
return a.Command + " " + strings.Join(args, " ")
}

func (a *CodexAgent) buildArgs(repoPath string, agenticMode, autoApprove bool) []string {
func (a *CodexAgent) buildArgs(
repoPath string,
agenticMode, autoApprove, sandboxBroken bool,
) []string {
return a.commandArgs(codexArgOptions{
repoPath: repoPath,
agenticMode: agenticMode,
autoApprove: autoApprove,
repoPath: repoPath,
agenticMode: agenticMode,
autoApprove: autoApprove,
sandboxBroken: sandboxBroken,
})
}

type codexArgOptions struct {
repoPath string
agenticMode bool
autoApprove bool
preview bool
repoPath string
agenticMode bool
autoApprove bool
sandboxBroken bool
preview bool
}

func (a *CodexAgent) commandArgs(opts codexArgOptions) []string {
Expand All @@ -141,10 +148,13 @@ func (a *CodexAgent) commandArgs(opts codexArgOptions) []string {
args = append(args, codexDangerousFlag)
}
if opts.autoApprove {
// 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.sandboxBroken {
// --full-auto still uses bwrap internally, so we
// need the full bypass flag on broken systems.
args = append(args, codexDangerousFlag)
} else {
args = append(args, "--sandbox", "read-only")
}
}
if !opts.preview {
args = append(args, "-C", opts.repoPath)
Expand Down Expand Up @@ -226,7 +236,11 @@ func (a *CodexAgent) Review(ctx context.Context, repoPath, commitSHA, prompt str

// Use codex exec with --json for JSONL streaming output
// The prompt is piped via stdin using "-" to avoid command line length limits on Windows
args := a.buildArgs(repoPath, agenticMode, autoApprove)
sandboxBroken := CodexSandboxDisabled()
if sandboxBroken && autoApprove {
log.Printf("codex: sandbox disabled via config, using %s", codexAutoApproveFlag)
}
args := a.buildArgs(repoPath, agenticMode, autoApprove, sandboxBroken)

runResult, runErr := runStreamingCLI(ctx, streamingCLISpec{
Name: "codex",
Expand Down
15 changes: 12 additions & 3 deletions internal/agent/codex_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ func TestCodex_buildArgs(t *testing.T) {
name string
agentic bool
autoApprove bool
sandboxBroken bool
wantFlags []string
wantMissingFlags []string
}{
Expand All @@ -46,11 +47,19 @@ func TestCodex_buildArgs(t *testing.T) {
wantFlags: []string{codexDangerousFlag, "--json"},
wantMissingFlags: []string{codexAutoApproveFlag},
},
{
name: "SandboxBrokenFallback",
agentic: false,
autoApprove: true,
sandboxBroken: true,
wantFlags: []string{codexDangerousFlag, "--json"},
wantMissingFlags: []string{"--sandbox", codexAutoApproveFlag},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
args := a.buildArgs("/repo", tt.agentic, tt.autoApprove)
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)
Expand All @@ -68,7 +77,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)
args := a.buildArgs("/repo", false, true, false)

require.GreaterOrEqual(t, len(args), 3)
assert.Equal(t, "exec", args[0])
Expand All @@ -81,7 +90,7 @@ func TestCodexBuildArgsWithSessionResume(t *testing.T) {
func TestCodexBuildArgsRejectsInvalidSessionResume(t *testing.T) {
a := NewCodexAgent("codex").WithSessionID("-bad-session").(*CodexAgent)

args := a.buildArgs("/repo", false, true)
args := a.buildArgs("/repo", false, true, false)

require.GreaterOrEqual(t, len(args), 2)
assert.Equal(t, "exec", args[0])
Expand Down
5 changes: 3 additions & 2 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,9 @@ type Config struct {
SecurityBackupModel string `toml:"security_backup_model"`
DesignBackupModel string `toml:"design_backup_model"`

AllowUnsafeAgents *bool `toml:"allow_unsafe_agents"` // nil = not set, allows commands to choose their own default
ReuseReviewSession *bool `toml:"reuse_review_session"` // nil = not set; when true, reuse prior branch review sessions when possible
AllowUnsafeAgents *bool `toml:"allow_unsafe_agents"` // nil = not set, allows commands to choose their own default
DisableCodexSandbox bool `toml:"disable_codex_sandbox"` // use --full-auto instead of --sandbox read-only (for systems where bwrap is broken)
ReuseReviewSession *bool `toml:"reuse_review_session"` // nil = not set; when true, reuse prior branch review sessions when possible

// Agent commands
CodexCmd string `toml:"codex_cmd"`
Expand Down
1 change: 1 addition & 0 deletions internal/daemon/config_watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ func (cw *ConfigWatcher) reloadConfig() {

// Update global agent settings
agent.SetAllowUnsafeAgents(newCfg.AllowUnsafeAgents != nil && *newCfg.AllowUnsafeAgents)
agent.SetCodexSandboxDisabled(newCfg.DisableCodexSandbox)
agent.SetAnthropicAPIKey(newCfg.AnthropicAPIKey)

// Log what changed (for debugging)
Expand Down
1 change: 1 addition & 0 deletions internal/daemon/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type Server struct {
func NewServer(db *storage.DB, cfg *config.Config, configPath string) *Server {
// Always set for deterministic state - default to false (conservative)
agent.SetAllowUnsafeAgents(cfg.AllowUnsafeAgents != nil && *cfg.AllowUnsafeAgents)
agent.SetCodexSandboxDisabled(cfg.DisableCodexSandbox)
agent.SetAnthropicAPIKey(cfg.AnthropicAPIKey)
broadcaster := NewBroadcaster()

Expand Down
73 changes: 73 additions & 0 deletions internal/daemon/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"log"
"os"
"regexp"
"strings"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -501,6 +502,21 @@ func (wp *WorkerPool) processJob(workerID string, job *storage.ReviewJob) {
log.Printf("[%s] Agent %s not available, using %s", workerID, job.Agent, agentName)
}

// Store the actual command line so the TUI displays what the
// daemon executes, not a client-side reconstruction.
if err := wp.db.SaveJobCommandLine(job.ID, a.CommandLine()); err != nil {
log.Printf("[%s] Error saving command line: %v", workerID, err)
}

// Codex --sandbox read-only cannot read files inside .git/.
// When the prompt references a snapshot file (oversized diff),
// read the file and inline its content so Codex gets the full
// diff via stdin without needing filesystem access. The DB
// keeps the truncated prompt; only the agent prompt expands.
if agentName == "codex" && !agentic {
reviewPrompt = expandSnapshotInline(reviewPrompt)
}

// Use the effective worktree path for events (empty when worktree is gone or not a worktree job).
eventWorktreePath := ""
if effectiveRepoPath != job.RepoPath {
Expand Down Expand Up @@ -1047,6 +1063,63 @@ func preparePrebuiltPrompt(
return replacer.Replace(reviewPrompt), cleanup, nil
}

// snapshotFileRefRE matches the file-reference lines emitted by
// diffFileFallbackVariants in the prompt builder when a diff is too
// large to inline. Example:
//
// Read the diff from: `/path/to/.git/roborev-snapshot-1234.diff`
var snapshotFileRefRE = regexp.MustCompile(
"`([^`]+roborev-snapshot-[^`]+\\.diff)`",
)

// expandSnapshotInline reads snapshot files referenced in a prompt
// and replaces the file-reference section with the actual diff
// content. This allows sandboxed agents that cannot access .git/ to
// receive the full diff via stdin.
func expandSnapshotInline(reviewPrompt string) string {
m := snapshotFileRefRE.FindStringSubmatch(reviewPrompt)
if m == nil {
return reviewPrompt
}
snapshotPath := m[1]
data, err := os.ReadFile(snapshotPath)
if err != nil {
// File unreadable — return the prompt as-is; the agent
// will see the truncated reference (same as before).
log.Printf("expand snapshot inline: read %s: %v", snapshotPath, err)
return reviewPrompt
}

// Replace the entire "(Diff too large …) … Read the diff from …"
// block with an inline diff fence.
inline := "```diff\n" + string(data)
if len(data) > 0 && data[len(data)-1] != '\n' {
inline += "\n"
}
inline += "```\n"

// The fallback block always starts with "(Diff too large" and
// ends after the file-reference line. Replace that span.
start := strings.Index(reviewPrompt, "(Diff too large")
if start < 0 {
// Dirty-review fallback uses different wording.
start = strings.Index(reviewPrompt, "The full diff is also available at:")
if start < 0 {
return reviewPrompt
}
}
refEnd := strings.Index(reviewPrompt[start:], m[0])
if refEnd < 0 {
return reviewPrompt
}
// Include the trailing backtick and any newlines after it.
end := start + refEnd + len(m[0])
for end < len(reviewPrompt) && reviewPrompt[end] == '\n' {
end++
}
return reviewPrompt[:start] + inline + reviewPrompt[end:]
}

func shellQuoteForPrompt(s string) string {
if s == "" {
return "''"
Expand Down
43 changes: 43 additions & 0 deletions internal/daemon/worker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,49 @@ func TestProcessJob_LargeDiffRetriesWhenSnapshotFails(t *testing.T) {
assert.False(t, agentCalled, "agent should not run when large diff has no snapshot")
}

func TestExpandSnapshotInline_ReplacesFileRef(t *testing.T) {
// Write a temp snapshot file
f, err := os.CreateTemp(t.TempDir(), "roborev-snapshot-*.diff")
require.NoError(t, err)
_, err = f.WriteString("diff --git a/file.go b/file.go\n+added line\n")
require.NoError(t, err)
require.NoError(t, f.Close())

prompt := fmt.Sprintf(
"### Diff\n\n"+
"(Diff too large to include inline)\n\n"+
"The full diff has been written to a file for review.\n"+
"Read the diff from: `%s`\n\n"+
"Review the actual diff before writing findings.\n"+
"\n## Summary\n",
f.Name(),
)

result := expandSnapshotInline(prompt)

assert := assert.New(t)
assert.NotContains(result, "Diff too large")
assert.NotContains(result, "roborev-snapshot-")
assert.Contains(result, "```diff")
assert.Contains(result, "+added line")
assert.Contains(result, "## Summary")
}

func TestExpandSnapshotInline_NoRef(t *testing.T) {
prompt := "### Diff\n\n```diff\n+inline diff\n```\n"
result := expandSnapshotInline(prompt)
assert.Equal(t, prompt, result)
}

func TestExpandSnapshotInline_MissingFile(t *testing.T) {
prompt := "### Diff\n\n" +
"(Diff too large to include inline)\n\n" +
"Read the diff from: `/nonexistent/roborev-snapshot-123.diff`\n"
result := expandSnapshotInline(prompt)
// Should return unchanged when file doesn't exist
assert.Equal(t, prompt, result)
}

func TestWorkerPoolCancelJobFinalCheckDeadlockSafe(t *testing.T) {
tc := newWorkerTestContext(t, 1)
job := tc.createAndClaimJob(t, "deadlock-test", testWorkerID)
Expand Down
12 changes: 12 additions & 0 deletions internal/storage/db.go
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,18 @@ func (db *DB) migrate() error {
}
}

// Migration: add command_line column to review_jobs if missing
err = db.QueryRow(`SELECT COUNT(*) FROM pragma_table_info('review_jobs') WHERE name = 'command_line'`).Scan(&count)
if err != nil {
return fmt.Errorf("check command_line column: %w", err)
}
if count == 0 {
_, err = db.Exec(`ALTER TABLE review_jobs ADD COLUMN command_line TEXT`)
if err != nil {
return fmt.Errorf("add command_line column: %w", err)
}
}

// Run sync-related migrations
if err := db.migrateSyncColumns(); err != nil {
return err
Expand Down
4 changes: 4 additions & 0 deletions internal/storage/hydration.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type reviewJobScanFields struct {
Patch sql.NullString
DiffContent sql.NullString
OutputPrefix sql.NullString
CommandLine sql.NullString
TokenUsage sql.NullString
Agentic int
PromptPrebuilt int
Expand Down Expand Up @@ -98,6 +99,9 @@ func applyReviewJobScan(job *ReviewJob, fields reviewJobScanFields) {
if fields.UUID.Valid {
job.UUID = fields.UUID.String
}
if fields.CommandLine.Valid {
job.CommandLine = fields.CommandLine.String
}
if fields.TokenUsage.Valid {
job.TokenUsage = fields.TokenUsage.String
}
Expand Down
Loading
Loading