Prevent .git/index.lock contention and enforce read-only sandbox for review agents (#518) (#539)

## Summary

Fixes #518.

- Set `GIT_OPTIONAL_LOCKS=0` in the environment of all agent
subprocesses via `configureSubprocess()`. Git 2.15+ honours this
variable and skips the optional index lock that read-only commands (`git
status`, `git diff`) normally take to refresh cached stat data. This
prevents background agents from contending with the user's own git
operations.
- Enforce read-only sandbox for codex review mode by replacing
`--full-auto` (which implies `--sandbox workspace-write`) with
`--sandbox read-only`. Previously, codex had full write access to the
working tree during background reviews — it could run `git add`, modify
files, and take mandatory index locks. Now codex review jobs are
read-only, matching Claude Code's existing `--allowedTools
Read,Glob,Grep` restriction. Agentic mode (fix/refine jobs, `--agentic`)
is unchanged and still uses
`--dangerously-bypass-approvals-and-sandbox`.
- Use `cmd.Environ()` instead of `os.Environ()` when building subprocess
environments so `PWD` is correctly synthesized from `cmd.Dir` rather
than inheriting the daemon's stale working directory.
- Move `configureSubprocess()` after Claude's custom env setup so
`GIT_OPTIONAL_LOCKS=0` is appended to the final environment rather than
being overwritten.
- Probe `codex exec --help` (not top-level `--help`) for `--sandbox`
support to match the subcommand where the flag is actually used.

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: wesm

internal/agent/agent_test.go

@@ -344,7 +344,7 @@ func TestSmartAgentReviewPassesModelFlag(t *testing.T) {
 			helpOutput := ""
 			jsonOutput := `{"type": "result", "result": "review result"}`
 			if tt.name == "codex" {
-				helpOutput = "--full-auto"
+				helpOutput = "--sandbox"
 				jsonOutput = `{"type": "item.completed", "item": {"type": "agent_message", "text": "review result"}}`
 			}
 

internal/agent/agent_test_helpers.go

@@ -195,7 +195,7 @@ func mockAgentCLI(t *testing.T, opts MockCLIOpts) *MockCLIResult {
 		if err := os.WriteFile(helpFile, []byte(opts.HelpOutput), 0644); err != nil {
 			t.Fatalf("write help output file: %v", err)
 		}
-		script.WriteString(fmt.Sprintf(`if [ "$1" = "--help" ]; then cat %q; exit 0; fi`, helpFile) + "\n")
+		script.WriteString(fmt.Sprintf(`case "$*" in *--help*) cat %q; exit 0;; esac`, helpFile) + "\n")
 	}
 
 	// Capture args

internal/agent/claude.go

@@ -7,7 +7,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"os"
 	"os/exec"
 	"slices"
 	"strings"
@@ -191,19 +190,24 @@ func (a *ClaudeAgent) Review(ctx context.Context, repoPath, commitSHA, prompt st
 
 	cmd := exec.CommandContext(ctx, a.Command, args...)
 	cmd.Dir = repoPath
-	tracker := configureSubprocess(cmd)
 
 	// Strip CLAUDECODE to prevent nested-session detection (#270),
 	// and handle API key (configured key or subscription auth).
+	// Use cmd.Environ() (not os.Environ()) so PWD=<cmd.Dir> is
+	// synthesized correctly. Set env before configureSubprocess so
+	// GIT_OPTIONAL_LOCKS=0 is appended to the final environment.
 	stripKeys := []string{"ANTHROPIC_API_KEY", "CLAUDECODE"}
+	baseEnv := cmd.Environ()
 	if apiKey := AnthropicAPIKey(); apiKey != "" {
-		cmd.Env = append(filterEnv(os.Environ(), stripKeys...), "ANTHROPIC_API_KEY="+apiKey)
+		cmd.Env = append(filterEnv(baseEnv, stripKeys...), "ANTHROPIC_API_KEY="+apiKey)
 	} else {
-		cmd.Env = filterEnv(os.Environ(), stripKeys...)
+		cmd.Env = filterEnv(baseEnv, stripKeys...)
 	}
 	// Suppress sounds from Claude Code (notification/completion sounds)
 	cmd.Env = append(cmd.Env, "CLAUDE_NO_SOUND=1")
 
+	tracker := configureSubprocess(cmd)
+
 	var stderr bytes.Buffer
 	stdoutPipe, err := cmd.StdoutPipe()
 	if err != nil {

internal/agent/codex.go

@@ -116,7 +116,7 @@ func (a *CodexAgent) CommandLine() string {
 	if agenticMode {
 		args = append(args, codexDangerousFlag)
 	} else {
-		args = append(args, codexAutoApproveFlag)
+		args = append(args, "--sandbox", "read-only")
 	}
 	if a.Model != "" {
 		args = append(args, "-m", a.Model)
@@ -140,7 +140,10 @@ func (a *CodexAgent) buildArgs(repoPath string, agenticMode, autoApprove bool) [
 		args = append(args, codexDangerousFlag)
 	}
 	if autoApprove {
-		args = append(args, codexAutoApproveFlag)
+		// 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,
@@ -174,15 +177,17 @@ func codexSupportsDangerousFlag(ctx context.Context, command string) (bool, erro
 	return supported, nil
 }
 
-func codexSupportsAutoApproveFlag(ctx context.Context, command string) (bool, error) {
+// codexSupportsNonInteractive checks that codex supports --sandbox,
+// 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
 	}
-	cmd := exec.CommandContext(ctx, command, "--help")
+	cmd := exec.CommandContext(ctx, command, "exec", "--help")
 	output, err := cmd.CombinedOutput()
-	supported := strings.Contains(string(output), codexAutoApproveFlag)
+	supported := strings.Contains(string(output), "--sandbox")
 	if err != nil && !supported {
-		return false, fmt.Errorf("check %s --help: %w: %s", command, err, output)
+		return false, fmt.Errorf("check %s exec --help: %w: %s", command, err, output)
 	}
 	codexAutoApproveSupport.Store(command, supported)
 	return supported, nil
@@ -202,16 +207,16 @@ func (a *CodexAgent) Review(ctx context.Context, repoPath, commitSHA, prompt str
 		}
 	}
 
-	// When piping stdin, codex needs --full-auto to run non-interactively.
-	// Agentic mode uses --dangerously-bypass-approvals-and-sandbox which implies auto-approve.
+	// Non-agentic review mode uses --sandbox read-only for
+	// non-interactive execution without writing to the working tree.
 	autoApprove := false
 	if !agenticMode {
-		supported, err := codexSupportsAutoApproveFlag(ctx, a.Command)
+		supported, err := codexSupportsNonInteractive(ctx, a.Command)
 		if err != nil {
 			return "", err
 		}
 		if !supported {
-			return "", fmt.Errorf("codex requires %s for stdin input; upgrade codex or use --agentic", codexAutoApproveFlag)
+			return "", fmt.Errorf("codex version too old for non-interactive execution; upgrade codex or use --agentic")
 		}
 		autoApprove = true
 	}

internal/agent/codex_test.go

@@ -36,8 +36,8 @@ func TestCodex_buildArgs(t *testing.T) {
 			name:             "NonAgenticAutoApprove",
 			agentic:          false,
 			autoApprove:      true,
-			wantFlags:        []string{codexAutoApproveFlag, "--json"},
-			wantMissingFlags: []string{codexDangerousFlag},
+			wantFlags:        []string{"--sandbox", "read-only", "--json"},
+			wantMissingFlags: []string{codexDangerousFlag, codexAutoApproveFlag},
 		},
 		{
 			name:             "AgenticNoAutoApprove",
@@ -106,9 +106,9 @@ func TestCodexReviewUnsafeMissingFlagErrors(t *testing.T) {
 	assert.Contains(t, err.Error(), "does not support")
 }
 
-func TestCodexReviewAlwaysAddsAutoApprove(t *testing.T) {
+func TestCodexReviewUsesReadOnlySandbox(t *testing.T) {
 	a, mock := setupMockCodex(t, false, MockCLIOpts{
-		HelpOutput:  "usage " + codexAutoApproveFlag,
+		HelpOutput:  "usage --sandbox",
 		CaptureArgs: true,
 		StdoutLines: []string{
 			`{"type":"item.completed","item":{"type":"agent_message","text":"ok"}}`,
@@ -120,12 +120,16 @@ func TestCodexReviewAlwaysAddsAutoApprove(t *testing.T) {
 
 	args, err := os.ReadFile(mock.ArgsFile)
 	require.NoError(t, err)
-	assert.Contains(t, string(args), codexAutoApproveFlag, "expected %s in args, got %s", codexAutoApproveFlag, strings.TrimSpace(string(args)))
+	argsStr := string(args)
+	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))
 }
 
 func TestCodexReviewWithSessionResumePassesResumeArgs(t *testing.T) {
 	a, mock := setupMockCodex(t, false, MockCLIOpts{
-		HelpOutput:  "usage " + codexAutoApproveFlag,
+		HelpOutput:  "usage --sandbox",
 		CaptureArgs: true,
 		StdoutLines: []string{
 			`{"type":"item.completed","item":{"type":"agent_message","text":"ok"}}`,
@@ -153,10 +157,7 @@ func TestCodexReviewTimeoutClosesStdoutPipe(t *testing.T) {
 	})
 
 	cmdPath := writeTempCommand(t, `#!/bin/sh
-if [ "$1" = "--help" ]; then
-  echo "usage `+codexAutoApproveFlag+`"
-  exit 0
-fi
+case "$*" in *--help*) echo "usage --sandbox"; exit 0;; esac
 (sleep 0.2) &
 printf '%s\n' '{"type":"item.completed","item":{"type":"agent_message","text":"partial"}}'
 exit 0
@@ -313,7 +314,7 @@ func TestCodexParseStreamJSON(t *testing.T) {
 
 func TestCodexReviewPipesPromptViaStdin(t *testing.T) {
 	a, mock := setupMockCodex(t, false, MockCLIOpts{
-		HelpOutput:   "usage " + codexAutoApproveFlag,
+		HelpOutput:   "usage --sandbox",
 		CaptureStdin: true,
 		StdoutLines: []string{
 			`{"type":"item.completed","item":{"type":"agent_message","text":"ok"}}`,
@@ -331,7 +332,7 @@ func TestCodexReviewPipesPromptViaStdin(t *testing.T) {
 
 func TestCodexReviewNoValidJSONReturnsError(t *testing.T) {
 	a, _ := setupMockCodex(t, false, MockCLIOpts{
-		HelpOutput:  "usage " + codexAutoApproveFlag,
+		HelpOutput:  "usage --sandbox",
 		StdoutLines: []string{"plain text output"},
 	})
 

internal/agent/process.go

@@ -20,6 +20,18 @@ type subprocessTracker struct {
 
 func configureSubprocess(cmd *exec.Cmd) *subprocessTracker {
 	cmd.WaitDelay = subprocessWaitDelay
+
+	// Prevent agents from taking .git/index.lock in the user's repo.
+	// Git 2.15+ honours GIT_OPTIONAL_LOCKS=0: read-only commands like
+	// "git status" and "git diff" skip the optional index lock they
+	// normally take to refresh cached stat data. Without this, agent
+	// processes running in the background contend with the user's own
+	// git operations (staging, committing, etc.).
+	if cmd.Env == nil {
+		cmd.Env = cmd.Environ()
+	}
+	cmd.Env = append(cmd.Env, "GIT_OPTIONAL_LOCKS=0")
+
 	tracker := &subprocessTracker{}
 	// Ensure Cancel is always set. Go's exec.CommandContext only provides a
 	// default Kill cancel when both Cancel==nil and WaitDelay==0. Since we

internal/agent/process_test.go

@@ -130,6 +130,46 @@ func TestContextProcessErrorDoesNotMaskSignalExitAfterContextDone(t *testing.T)
 	}
 }
 
+func TestConfigureSubprocessSetsOptionalLocks(t *testing.T) {
+	skipIfWindows(t)
+
+	cmd := exec.CommandContext(context.Background(), "sh", "-c", "echo $GIT_OPTIONAL_LOCKS")
+	configureSubprocess(cmd)
+
+	out, err := cmd.Output()
+	require.NoError(t, err)
+	require.Equal(t, "0\n", string(out),
+		"configureSubprocess should set GIT_OPTIONAL_LOCKS=0")
+}
+
+func TestConfigureSubprocessPreservesExistingEnv(t *testing.T) {
+	skipIfWindows(t)
+
+	cmd := exec.CommandContext(context.Background(),
+		"sh", "-c", "echo $MY_TEST_VAR:$GIT_OPTIONAL_LOCKS")
+	cmd.Env = append(os.Environ(), "MY_TEST_VAR=hello")
+	configureSubprocess(cmd)
+
+	out, err := cmd.Output()
+	require.NoError(t, err)
+	require.Equal(t, "hello:0\n", string(out),
+		"configureSubprocess should preserve existing env and add GIT_OPTIONAL_LOCKS=0")
+}
+
+func TestConfigureSubprocessPreservesPWD(t *testing.T) {
+	skipIfWindows(t)
+
+	dir := t.TempDir()
+	cmd := exec.CommandContext(context.Background(), "sh", "-c", "echo $PWD")
+	cmd.Dir = dir
+	configureSubprocess(cmd)
+
+	out, err := cmd.Output()
+	require.NoError(t, err)
+	require.Equal(t, dir+"\n", string(out),
+		"configureSubprocess should preserve PWD matching cmd.Dir")
+}
+
 func TestConfigureSubprocessDoesNotMarkCanceledWhenProcessAlreadyExited(t *testing.T) {
 	skipIfWindows(t)