Fix wrong agent and duplicate reviews from post-commit hook (#197)

## Summary

Fixes two problems when a fix agent creates a commit and the post-commit
hook fires:

1. **Wrong agent**: The hook sends no `--agent` flag, so the server
defaulted to `codex` even when codex wasn't installed. The enqueue
handler now resolves the configured agent to the first **installed**
agent via the fallback chain (codex -> claude-code -> gemini -> ...), so
the job reflects what will actually run.

2. **Duplicate reviews**: Both the post-commit hook and
`enqueueIfNeeded` (called after a fix completes) could create separate
reviews for the same commit. Replace the fixed 2s sleep with a **10 x 1s
polling loop** that returns as soon as the hook's job is found, plus a
final check after the loop.

Also returns **503** when no review agent is installed at all, failing
fast instead of queuing a doomed job.

## Files changed

- `internal/daemon/server.go` — resolve agent to first available at
enqueue time, 503 on none
- `cmd/roborev/fix.go` — polling loop in `enqueueIfNeeded`
- `internal/daemon/server_test.go` — 5 agent availability subtests with
cross-platform PATH isolation

## Test plan

- [x] `go test ./internal/daemon/ -run
TestHandleEnqueueAgentAvailability -v` — all 5 subtests pass
- [x] `go test ./cmd/roborev/ -run TestEnqueueIfNeeded -v` — all 3
subtests pass
- [x] `go test ./...` — full suite passes

Fixes #196

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: wesm

cmd/roborev/fix.go

@@ -1004,12 +1004,15 @@ func addJobResponse(serverAddr string, jobID int64, commenter, response string)
 func enqueueIfNeeded(serverAddr, repoPath, sha string) error {
 	// Check if a review job already exists for this commit (e.g., from the
 	// post-commit hook). If so, skip enqueuing to avoid duplicates.
-	// We check twice with a 1-second delay to give the post-commit hook
-	// time to fire before we enqueue ourselves.
-	if hasJobForSHA(serverAddr, sha) {
-		return nil
+	// The post-commit hook normally completes before control returns here,
+	// but under heavy load it may take longer. Poll with short intervals
+	// up to a max wait to avoid both unnecessary delays and duplicates.
+	for range 10 {
+		if hasJobForSHA(serverAddr, sha) {
+			return nil
+		}
+		time.Sleep(1 * time.Second)
 	}
-	time.Sleep(2 * time.Second)
 	if hasJobForSHA(serverAddr, sha) {
 		return nil
 	}

internal/daemon/server.go

@@ -451,6 +451,16 @@ func (s *Server) handleEnqueue(w http.ResponseWriter, r *http.Request) {
 	// Resolve agent for review workflow at this reasoning level
 	agentName := config.ResolveAgentForWorkflow(req.Agent, repoRoot, s.configWatcher.Config(), "review", reasoning)
 
+	// Resolve to an installed agent: if the configured agent isn't available,
+	// fall back through the chain (codex -> claude-code -> gemini -> ...).
+	// Fail fast with 503 if nothing is installed at all.
+	if resolved, err := agent.GetAvailable(agentName); err != nil {
+		writeError(w, http.StatusServiceUnavailable, fmt.Sprintf("no review agent available: %v", err))
+		return
+	} else {
+		agentName = resolved.Name()
+	}
+
 	// Resolve model for review workflow at this reasoning level
 	model := config.ResolveModelForWorkflow(req.Model, repoRoot, s.configWatcher.Config(), "review", reasoning)
 

internal/daemon/server_test.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"sync"
 	"testing"
@@ -2582,6 +2583,132 @@ func TestHandleJobOutput_StreamingCompletedJob(t *testing.T) {
 }
 
 // TestHandleJobOutput_MissingJobID tests that missing job_id returns 400.
+func TestHandleEnqueueAgentAvailability(t *testing.T) {
+	// Shared read-only git repo created once (all subtests use different servers for DB isolation)
+	repoDir := filepath.Join(t.TempDir(), "repo")
+	testutil.InitTestGitRepo(t, repoDir)
+	headSHA := testutil.GetHeadSHA(t, repoDir)
+
+	// Create an isolated dir containing only a wrapper for git.
+	// We can't just use git's parent dir because it may contain real agent
+	// binaries (e.g. codex, claude) that would defeat the PATH isolation.
+	// Symlinks don't work reliably on Windows, so we use wrapper scripts.
+	gitPath, err := exec.LookPath("git")
+	if err != nil {
+		t.Fatal("git not found in PATH")
+	}
+	gitOnlyDir := t.TempDir()
+	if runtime.GOOS == "windows" {
+		wrapper := fmt.Sprintf("@\"%s\" %%*\r\n", gitPath)
+		if err := os.WriteFile(filepath.Join(gitOnlyDir, "git.cmd"), []byte(wrapper), 0755); err != nil {
+			t.Fatal(err)
+		}
+	} else {
+		wrapper := fmt.Sprintf("#!/bin/sh\nexec '%s' \"$@\"\n", gitPath)
+		if err := os.WriteFile(filepath.Join(gitOnlyDir, "git"), []byte(wrapper), 0755); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	mockScript := "#!/bin/sh\nexit 0\n"
+
+	tests := []struct {
+		name          string
+		requestAgent  string
+		mockBinaries  []string // binary names to place in PATH
+		expectedAgent string   // expected agent stored in job
+		expectedCode  int      // expected HTTP status code
+	}{
+		{
+			name:          "explicit test agent preserved",
+			requestAgent:  "test",
+			mockBinaries:  nil,
+			expectedAgent: "test",
+			expectedCode:  http.StatusCreated,
+		},
+		{
+			name:          "unavailable codex falls back to claude-code",
+			requestAgent:  "codex",
+			mockBinaries:  []string{"claude"},
+			expectedAgent: "claude-code",
+			expectedCode:  http.StatusCreated,
+		},
+		{
+			name:          "default agent falls back when codex not installed",
+			requestAgent:  "",
+			mockBinaries:  []string{"claude"},
+			expectedAgent: "claude-code",
+			expectedCode:  http.StatusCreated,
+		},
+		{
+			name:          "explicit codex kept when available",
+			requestAgent:  "codex",
+			mockBinaries:  []string{"codex"},
+			expectedAgent: "codex",
+			expectedCode:  http.StatusCreated,
+		},
+		{
+			name:         "no agents available returns 503",
+			requestAgent: "codex",
+			mockBinaries: nil,
+			expectedCode: http.StatusServiceUnavailable,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Each subtest gets its own server/DB to avoid SHA dedup conflicts
+			server, _, _ := newTestServer(t)
+
+			// Isolate PATH: only mock binaries + git (no real agent CLIs)
+			origPath := os.Getenv("PATH")
+			mockDir := t.TempDir()
+			for _, bin := range tt.mockBinaries {
+				name := bin
+				content := mockScript
+				if runtime.GOOS == "windows" {
+					name = bin + ".cmd"
+					content = "@exit /b 0\r\n"
+				}
+				if err := os.WriteFile(filepath.Join(mockDir, name), []byte(content), 0755); err != nil {
+					t.Fatal(err)
+				}
+			}
+			os.Setenv("PATH", mockDir+string(os.PathListSeparator)+gitOnlyDir)
+			t.Cleanup(func() { os.Setenv("PATH", origPath) })
+
+			reqData := map[string]string{
+				"repo_path":  repoDir,
+				"commit_sha": headSHA,
+			}
+			if tt.requestAgent != "" {
+				reqData["agent"] = tt.requestAgent
+			}
+			req := testutil.MakeJSONRequest(t, http.MethodPost, "/api/enqueue", reqData)
+			w := httptest.NewRecorder()
+
+			server.handleEnqueue(w, req)
+
+			if w.Code != tt.expectedCode {
+				t.Fatalf("Expected status %d, got %d: %s", tt.expectedCode, w.Code, w.Body.String())
+			}
+
+			if tt.expectedCode != http.StatusCreated {
+				return
+			}
+
+			var job storage.ReviewJob
+			if err := json.NewDecoder(w.Body).Decode(&job); err != nil {
+				t.Fatalf("Failed to decode response: %v", err)
+			}
+
+			if job.Agent != tt.expectedAgent {
+				t.Errorf("Expected agent %q, got %q", tt.expectedAgent, job.Agent)
+			}
+		})
+	}
+}
+
 func TestHandleJobOutput_MissingJobID(t *testing.T) {
 	server, _, _ := newTestServer(t)