Improve robustness of roborev refine, get working with Claude Code (#61)

`claude` required a lot of twiddling to get working, whereas Codex was
fairly straightforward.

Even if you have ANTHROPIC_API_KEY set in your environment, roborev
refine won't use it unless you set it in `config.toml` to avoid surprise
bills (it will use your subscription if available, otherwise, you must
opt in to API billing).

---------

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

Author: wesm

README.md

@@ -236,6 +236,19 @@ Override the data directory with `ROBOREV_DATA_DIR`:
 export ROBOREV_DATA_DIR=/custom/path  # Default: ~/.roborev
 ```
 
+### Authentication
+
+**Claude Code** uses your Claude subscription by default. roborev deliberately ignores `ANTHROPIC_API_KEY` from the environment to avoid unexpected API charges.
+
+To use Anthropic API credits instead:
+
+```toml
+# ~/.roborev/config.toml
+anthropic_api_key = "sk-ant-..."
+```
+
+**Codex** uses the authentication configured in your Codex installation.
+
 ### Priority Order
 
 1. CLI flags (`--agent`, `--reasoning`)

cmd/roborev/main_test.go

@@ -699,7 +699,7 @@ func TestRefineLoopFindFailedReviewPath(t *testing.T) {
 		}
 
 		commits := []string{"commit1sha", "commit2sha", "commit3sha"}
-		review, err := findFailedReviewForBranch(client, commits)
+		review, err := findFailedReviewForBranch(client, commits, nil)
 
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -721,7 +721,7 @@ func TestRefineLoopFindFailedReviewPath(t *testing.T) {
 		}
 
 		commits := []string{"commit1sha"}
-		review, err := findFailedReviewForBranch(client, commits)
+		review, err := findFailedReviewForBranch(client, commits, nil)
 
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
@@ -836,7 +836,7 @@ func TestRefineLoopBranchReviewPath(t *testing.T) {
 		}
 
 		commits := []string{"commit1", "commit2"}
-		review, err := findFailedReviewForBranch(client, commits)
+		review, err := findFailedReviewForBranch(client, commits, nil)
 
 		// Should return nil since all pass
 		if err != nil {

cmd/roborev/refine.go

@@ -209,6 +209,9 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al
 	resolvedAgent := config.ResolveAgent(agentName, repoPath, cfg)
 	allowUnsafeAgents = resolveAllowUnsafeAgents(allowUnsafeAgents, unsafeFlagChanged, cfg)
 	agent.SetAllowUnsafeAgents(allowUnsafeAgents)
+	if cfg != nil {
+		agent.SetAnthropicAPIKey(cfg.AnthropicAPIKey)
+	}
 
 	// Resolve reasoning level from CLI or config (default: fast)
 	resolvedReasoning, err := config.ResolveRefineReasoning(reasoningStr, repoPath)
@@ -228,6 +231,8 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al
 	// Track current failed review - when a fix fails, we continue fixing it
 	// before moving on to the next oldest failed commit
 	var currentFailedReview *storage.Review
+	// Track reviews we've given up on this run to avoid re-selecting them
+	skippedReviews := make(map[int64]bool)
 
 	for iteration := 1; iteration <= maxIterations; {
 		// Get commits on current branch
@@ -244,7 +249,7 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al
 		// Only search for a new failed review if we don't have one to work on
 		// (either first iteration, or previous fix passed)
 		if currentFailedReview == nil {
-			currentFailedReview, err = findFailedReviewForBranch(client, commits)
+			currentFailedReview, err = findFailedReviewForBranch(client, commits, skippedReviews)
 			if err != nil {
 				return fmt.Errorf("error finding reviews: %w", err)
 			}
@@ -436,12 +441,11 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al
 			}
 			if noChangeAttempts >= 2 {
 				// Tried 3 times (including this one), give up on this review
-				fmt.Println("Marking as addressed after multiple failed attempts")
+				// Do NOT mark as addressed - the review still needs attention
+				fmt.Println("Giving up after multiple failed attempts (review remains unaddressed)")
 				client.AddResponse(currentFailedReview.JobID, "roborev-refine", "Agent could not determine how to address findings (attempt 3, giving up)")
-				if err := client.MarkReviewAddressed(currentFailedReview.ID); err != nil {
-					fmt.Printf("Warning: failed to mark review %d as addressed: %v\n", currentFailedReview.ID, err)
-				}
-				currentFailedReview = nil // Move on to next oldest failed commit
+				skippedReviews[currentFailedReview.ID] = true // Don't re-select this run
+				currentFailedReview = nil                     // Move on to next oldest failed commit
 			} else {
 				// Record attempt but don't mark addressed - might work on retry with different context
 				client.AddResponse(currentFailedReview.JobID, "roborev-refine", fmt.Sprintf("Agent could not determine how to address findings (attempt %d)", noChangeAttempts+1))
@@ -527,7 +531,8 @@ func resolveAllowUnsafeAgents(flag bool, flagChanged bool, cfg *config.Config) b
 // findFailedReviewForBranch finds an unaddressed failed review for any of the given commits.
 // Iterates oldest to newest so earlier commits are fixed before later ones.
 // Passing reviews are marked as addressed automatically.
-func findFailedReviewForBranch(client daemon.Client, commits []string) (*storage.Review, error) {
+// Reviews in the skip set are ignored (used for reviews we've given up on this run).
+func findFailedReviewForBranch(client daemon.Client, commits []string, skip map[int64]bool) (*storage.Review, error) {
 	// Iterate oldest to newest (commits are in chronological order)
 	for _, sha := range commits {
 		review, err := client.GetReviewBySHA(sha)
@@ -543,6 +548,11 @@ func findFailedReviewForBranch(client daemon.Client, commits []string) (*storage
 			continue
 		}
 
+		// Skip reviews we've given up on this run
+		if skip[review.ID] {
+			continue
+		}
+
 		verdict := storage.ParseVerdict(review.Output)
 		if verdict == "F" {
 			return review, nil

cmd/roborev/refine_test.go

@@ -263,7 +263,7 @@ func TestFindFailedReviewForBranch_OldestFirst(t *testing.T) {
 	// Commits in chronological order (oldest first, as returned by git log --reverse)
 	commits := []string{"oldest123", "middle456", "newest789"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -289,7 +289,7 @@ func TestFindFailedReviewForBranch_SkipsAddressed(t *testing.T) {
 
 	commits := []string{"commit1", "commit2", "commit3"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -300,6 +300,55 @@ func TestFindFailedReviewForBranch_SkipsAddressed(t *testing.T) {
 	}
 }
 
+func TestFindFailedReviewForBranch_SkipsGivenUpReviews(t *testing.T) {
+	client := newMockDaemonClient()
+
+	client.reviews = map[string]*storage.Review{
+		"commit1": {ID: 1, JobID: 100, Output: "Bug found."},       // Fail (oldest, but in skip set)
+		"commit2": {ID: 2, JobID: 200, Output: "Another bug."},     // Fail (should be returned)
+		"commit3": {ID: 3, JobID: 300, Output: "No issues found."}, // Pass
+	}
+
+	commits := []string{"commit1", "commit2", "commit3"}
+
+	// Skip review ID 1 (simulates "giving up" after 3 failed attempts)
+	skip := map[int64]bool{1: true}
+
+	found, err := findFailedReviewForBranch(client, commits, skip)
+	if err != nil {
+		t.Fatalf("findFailedReviewForBranch failed: %v", err)
+	}
+
+	// Should return commit2 (job 200), skipping commit1 which is in the skip set
+	if found == nil || found.JobID != 200 {
+		t.Errorf("expected job 200 (skipping given-up review), got %v", found)
+	}
+}
+
+func TestFindFailedReviewForBranch_AllSkippedReturnsNil(t *testing.T) {
+	client := newMockDaemonClient()
+
+	client.reviews = map[string]*storage.Review{
+		"commit1": {ID: 1, JobID: 100, Output: "Bug found."}, // Fail (in skip set)
+		"commit2": {ID: 2, JobID: 200, Output: "Another."},   // Fail (in skip set)
+	}
+
+	commits := []string{"commit1", "commit2"}
+
+	// Skip both reviews (simulates giving up on all failed reviews)
+	skip := map[int64]bool{1: true, 2: true}
+
+	found, err := findFailedReviewForBranch(client, commits, skip)
+	if err != nil {
+		t.Fatalf("findFailedReviewForBranch failed: %v", err)
+	}
+
+	// Should return nil since all failures are in skip set
+	if found != nil {
+		t.Errorf("expected nil (all skipped), got job %d", found.JobID)
+	}
+}
+
 func TestFindFailedReviewForBranch_AllPass(t *testing.T) {
 	client := newMockDaemonClient()
 
@@ -310,7 +359,7 @@ func TestFindFailedReviewForBranch_AllPass(t *testing.T) {
 
 	commits := []string{"commit1", "commit2"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -386,7 +435,7 @@ func TestFindFailedReviewForBranch_NoReviews(t *testing.T) {
 
 	commits := []string{"unreviewed1", "unreviewed2"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -407,7 +456,7 @@ func TestFindFailedReviewForBranch_MarksPassingAsAddressed(t *testing.T) {
 
 	commits := []string{"commit1", "commit2"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -443,7 +492,7 @@ func TestFindFailedReviewForBranch_MarksPassingBeforeFailure(t *testing.T) {
 
 	commits := []string{"commit1", "commit2"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -473,7 +522,7 @@ func TestFindFailedReviewForBranch_DoesNotMarkAlreadyAddressed(t *testing.T) {
 
 	commits := []string{"commit1", "commit2"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -507,7 +556,7 @@ func TestFindFailedReviewForBranch_MixedScenario(t *testing.T) {
 
 	commits := []string{"commit1", "commit2", "commit3", "commit4", "commit5"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -544,7 +593,7 @@ func TestFindFailedReviewForBranch_StopsAtFirstFailure(t *testing.T) {
 
 	commits := []string{"commit1", "commit2", "commit3"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 	if err != nil {
 		t.Fatalf("findFailedReviewForBranch failed: %v", err)
 	}
@@ -573,7 +622,7 @@ func TestFindFailedReviewForBranch_MarkAddressedError(t *testing.T) {
 
 	commits := []string{"commit1"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 
 	// Should return an error and not continue processing
 	if err == nil {
@@ -598,7 +647,7 @@ func TestFindFailedReviewForBranch_GetReviewBySHAError(t *testing.T) {
 
 	commits := []string{"commit1", "commit2"}
 
-	found, err := findFailedReviewForBranch(client, commits)
+	found, err := findFailedReviewForBranch(client, commits, nil)
 
 	// Should return an error and not continue processing
 	if err == nil {

internal/agent/agent.go

@@ -59,6 +59,7 @@ type CommandAgent interface {
 // Registry holds available agents
 var registry = make(map[string]Agent)
 var allowUnsafeAgents atomic.Bool
+var anthropicAPIKey atomic.Value
 
 func AllowUnsafeAgents() bool {
 	return allowUnsafeAgents.Load()
@@ -68,6 +69,19 @@ func SetAllowUnsafeAgents(allow bool) {
 	allowUnsafeAgents.Store(allow)
 }
 
+// AnthropicAPIKey returns the configured Anthropic API key, or empty string if not set
+func AnthropicAPIKey() string {
+	if v := anthropicAPIKey.Load(); v != nil {
+		return v.(string)
+	}
+	return ""
+}
+
+// SetAnthropicAPIKey sets the Anthropic API key for Claude Code
+func SetAnthropicAPIKey(key string) {
+	anthropicAPIKey.Store(key)
+}
+
 // aliases maps short names to full agent names
 var aliases = map[string]string{
 	"claude": "claude-code",