Fix agent resolution and improve skill definitions (#449)

## Summary

- Add variadic `backups ...string` parameter to `GetAvailable` and
`GetAvailableWithConfig` so configured backup agents are tried before
the hardcoded codex→claude-code→gemini→... fallback chain
- Update all call sites (handleEnqueue, handleFixJob, CI poller, review,
fix, analyze, refine, batch) to resolve the backup agent from config via
`ResolveBackupAgentForWorkflow` and pass it through
- When the backup agent is selected and no explicit model was requested,
use `ResolveBackupModelForWorkflow` for the model instead of the default
model
- Fix backup agent in refine and review workflows inheriting the primary
agent's model instead of resolving its own backup model
- Extract shared `applyModelForAgent()` helper used by both refine and
review for backup-vs-primary model resolution, replacing inline
duplicated logic
- Add regression tests that call `applyModelForAgent` directly to verify
backup agent model inheritance and empty-model guard
- Add invocation guards to review skills preventing false triggers when
users paste existing review results
- Make fix skill check conversation context before running discovery
commands, skipping fetch when findings are already present; require
matching pasted findings to jobs by commit SHA before closing
- Replace "ask to commit" in fix skill with deference to project
CLAUDE.md conventions
- Add general principle to all skills: treat instructions as guidelines,
use conversation context, defer to project-level config on conflicts

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

---------

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

Author: wesm

cmd/roborev/analyze.go

@@ -806,17 +806,27 @@ func runFixAgent(cmd *cobra.Command, repoPath, agentName, model, reasoning, prom
 	cliAgentChanged := agentName != "" &&
 		agent.CanonicalName(agentName) != agent.CanonicalName(configAgent)
 	agentName = config.ResolveAgentForWorkflow(agentName, repoPath, cfg, "fix", reasoning)
-	if cliAgentChanged && model == "" {
-		model = config.ResolveWorkflowModel(repoPath, cfg, "fix", reasoning)
-	} else {
-		model = config.ResolveModelForWorkflow(model, repoPath, cfg, "fix", reasoning)
-	}
+	backupAgent := config.ResolveBackupAgentForWorkflow(repoPath, cfg, "fix")
 
-	a, err := agent.GetAvailableWithConfig(agentName, cfg)
+	a, err := agent.GetAvailableWithConfig(agentName, cfg, backupAgent)
 	if err != nil {
 		return fmt.Errorf("get agent: %w", err)
 	}
 
+	// Use backup model when the backup agent was selected and no
+	// explicit model was passed via CLI.
+	preferredForAnalyze := config.ResolveAgentForWorkflow(agentName, repoPath, cfg, "fix", reasoning)
+	usingBackup := backupAgent != "" &&
+		agent.CanonicalName(a.Name()) == agent.CanonicalName(backupAgent) &&
+		agent.CanonicalName(a.Name()) != agent.CanonicalName(preferredForAnalyze)
+	if usingBackup && model == "" {
+		model = config.ResolveBackupModelForWorkflow(repoPath, cfg, "fix")
+	} else if cliAgentChanged && model == "" {
+		model = config.ResolveWorkflowModel(repoPath, cfg, "fix", reasoning)
+	} else {
+		model = config.ResolveModelForWorkflow(model, repoPath, cfg, "fix", reasoning)
+	}
+
 	// Configure agent: agentic mode, with model and reasoning
 	reasoningLevel := agent.ParseReasoningLevel(reasoning)
 	a = a.WithAgentic(true).WithReasoning(reasoningLevel)

cmd/roborev/fix.go

@@ -326,13 +326,26 @@ func resolveFixAgent(repoPath string, opts fixOptions) (agent.Agent, error) {
 	}
 
 	agentName := config.ResolveAgentForWorkflow(opts.agentName, repoPath, cfg, "fix", reasoning)
-	modelStr := resolveFixModel(opts.agentName, opts.model, repoPath, cfg, reasoning)
+	backupAgent := config.ResolveBackupAgentForWorkflow(repoPath, cfg, "fix")
 
-	a, err := agent.GetAvailableWithConfig(agentName, cfg)
+	a, err := agent.GetAvailableWithConfig(agentName, cfg, backupAgent)
 	if err != nil {
 		return nil, fmt.Errorf("get agent: %w", err)
 	}
 
+	// Use backup model when the backup agent was selected and no
+	// explicit model was passed via CLI.
+	preferredAgent := config.ResolveAgentForWorkflow(opts.agentName, repoPath, cfg, "fix", reasoning)
+	usingBackup := backupAgent != "" &&
+		agent.CanonicalName(a.Name()) == agent.CanonicalName(backupAgent) &&
+		agent.CanonicalName(a.Name()) != agent.CanonicalName(preferredAgent)
+	var modelStr string
+	if usingBackup && opts.model == "" {
+		modelStr = config.ResolveBackupModelForWorkflow(repoPath, cfg, "fix")
+	} else {
+		modelStr = resolveFixModel(opts.agentName, opts.model, repoPath, cfg, reasoning)
+	}
+
 	reasoningLevel := agent.ParseReasoningLevel(reasoning)
 	a = a.WithAgentic(true).WithReasoning(reasoningLevel)
 	if modelStr != "" {

cmd/roborev/refine.go

@@ -355,20 +355,24 @@ func runRefine(ctx RunContext, opts refineOptions) error {
 
 	// Resolve agent for refine workflow at this reasoning level
 	resolvedAgent := config.ResolveAgentForWorkflow(opts.agentName, repoPath, cfg, "refine", resolvedReasoning)
+	backupAgent := config.ResolveBackupAgentForWorkflow(repoPath, cfg, "refine")
 	allowUnsafe := resolveAllowUnsafeAgents(opts.allowUnsafeAgents, opts.unsafeFlagChanged, cfg)
 	agent.SetAllowUnsafeAgents(allowUnsafe)
 	if cfg != nil {
 		agent.SetAnthropicAPIKey(cfg.AnthropicAPIKey)
 	}
 
-	// Resolve model for refine workflow at this reasoning level
-	resolvedModel := config.ResolveModelForWorkflow(opts.model, repoPath, cfg, "refine", resolvedReasoning)
-
-	// Get the agent with configured reasoning level and model
-	addressAgent, err := selectRefineAgent(cfg, resolvedAgent, reasoningLevel, resolvedModel)
+	// Get the agent with configured reasoning level (model applied after
+	// backup determination to avoid baking the primary model into a
+	// backup agent).
+	addressAgent, err := selectRefineAgent(cfg, resolvedAgent, reasoningLevel, backupAgent)
 	if err != nil {
 		return fmt.Errorf("no agent available: %w", err)
 	}
+	addressAgent, _ = applyModelForAgent(
+		addressAgent, resolvedAgent, backupAgent,
+		opts.model, repoPath, cfg, "refine", resolvedReasoning,
+	)
 	fmt.Printf("Using agent: %s\n", addressAgent.Name())
 
 	// 3. Refinement loop
@@ -1149,10 +1153,46 @@ func verifyRepoState(
 	return nil
 }
 
-func selectRefineAgent(cfg *config.Config, resolvedAgent string, reasoningLevel agent.ReasoningLevel, model string) (agent.Agent, error) {
-	baseAgent, err := agent.GetAvailableWithConfig(resolvedAgent, cfg)
+func selectRefineAgent(cfg *config.Config, resolvedAgent string, reasoningLevel agent.ReasoningLevel, backups ...string) (agent.Agent, error) {
+	baseAgent, err := agent.GetAvailableWithConfig(resolvedAgent, cfg, backups...)
 	if err != nil {
 		return nil, err
 	}
-	return baseAgent.WithReasoning(reasoningLevel).WithModel(model), nil
+	return baseAgent.WithReasoning(reasoningLevel), nil
+}
+
+// applyModelForAgent resolves the correct model for the selected agent
+// and applies it. When the selected agent is the configured backup (not
+// the preferred primary), the backup model is used instead of the
+// primary model. Returns the agent with the model applied (if any) and
+// the resolved model string.
+func applyModelForAgent(
+	a agent.Agent,
+	preferredAgent string,
+	backupAgentName string,
+	cliModel string,
+	repoPath string,
+	cfg *config.Config,
+	workflow string,
+	reasoning string,
+) (agent.Agent, string) {
+	usingBackup := backupAgentName != "" &&
+		agent.CanonicalName(a.Name()) == agent.CanonicalName(backupAgentName) &&
+		agent.CanonicalName(a.Name()) != agent.CanonicalName(preferredAgent)
+
+	var model string
+	if usingBackup && cliModel == "" {
+		model = config.ResolveBackupModelForWorkflow(
+			repoPath, cfg, workflow,
+		)
+	} else {
+		model = config.ResolveModelForWorkflow(
+			cliModel, repoPath, cfg, workflow, reasoning,
+		)
+	}
+
+	if model != "" {
+		a = a.WithModel(model)
+	}
+	return a, model
 }

cmd/roborev/refine_test.go

@@ -953,6 +953,140 @@ func TestResolveReasoningWithFast(t *testing.T) {
 	}
 }
 
+// TestApplyModelForAgent_BackupKeepsOwnModel verifies that when the backup
+// agent is selected (primary unavailable) and no backup model is configured,
+// applyModelForAgent does not apply the primary model to the backup agent.
+// Regression test for f7385273.
+func TestApplyModelForAgent_BackupKeepsOwnModel(t *testing.T) {
+	// Make only codex available (not gemini, which will be "primary").
+	t.Cleanup(testutil.MockExecutableIsolated(t, "codex", 0))
+
+	selected, err := selectRefineAgent(
+		nil, "gemini", agent.ReasoningStandard, "codex",
+	)
+	if err != nil {
+		t.Fatalf("selectRefineAgent: %v", err)
+	}
+	if selected.Name() != "codex" {
+		t.Fatalf("expected backup agent codex, got %s", selected.Name())
+	}
+
+	// Call the real helper with no CLI model and no config.
+	// The primary would resolve to some default model, but the backup
+	// agent should NOT inherit it.
+	result, model := applyModelForAgent(
+		selected,
+		"gemini", // preferred (unavailable)
+		"codex",  // backup (selected)
+		"",       // no CLI model
+		"",       // no repo path
+		nil,      // no config
+		"refine",
+		"standard",
+	)
+
+	codexAgent, ok := result.(*agent.CodexAgent)
+	if !ok {
+		t.Fatalf("expected *CodexAgent, got %T", result)
+	}
+	if codexAgent.Model != "" {
+		t.Errorf(
+			"backup agent should keep its default model (empty), got %q",
+			codexAgent.Model,
+		)
+	}
+	if model != "" {
+		t.Errorf("resolved model should be empty for unconfigured backup, got %q", model)
+	}
+}
+
+// TestApplyModelForAgent_EmptyModelPreservesAgentDefault verifies that
+// applyModelForAgent does not clear an agent's existing model when the
+// resolved model is empty.
+// Regression test for f7385273.
+func TestApplyModelForAgent_EmptyModelPreservesAgentDefault(t *testing.T) {
+	t.Cleanup(testutil.MockExecutable(t, "codex", 0))
+
+	a, err := agent.Get("codex")
+	if err != nil {
+		t.Fatalf("agent.Get: %v", err)
+	}
+	// Pre-set a model on the agent.
+	a = a.WithModel("o3")
+
+	// Agent is the preferred agent (not a backup). No CLI model, no
+	// config → resolved model will be empty.
+	result, _ := applyModelForAgent(
+		a,
+		"codex", // preferred
+		"",      // no backup
+		"",      // no CLI model
+		"",      // no repo path
+		nil,     // no config
+		"review",
+		"standard",
+	)
+
+	codexAgent, ok := result.(*agent.CodexAgent)
+	if !ok {
+		t.Fatalf("expected *CodexAgent, got %T", result)
+	}
+	if codexAgent.Model != "o3" {
+		t.Errorf(
+			"agent model should remain %q, got %q",
+			"o3", codexAgent.Model,
+		)
+	}
+}
+
+// TestApplyModelForAgent_SameAgentPrimaryAndBackup verifies that when
+// primary and backup resolve to the same agent, the agent is treated as
+// the primary (not backup) and gets the workflow model, not the backup model.
+func TestApplyModelForAgent_SameAgentPrimaryAndBackup(t *testing.T) {
+	t.Cleanup(testutil.MockExecutable(t, "codex", 0))
+
+	a, err := agent.Get("codex")
+	if err != nil {
+		t.Fatalf("agent.Get: %v", err)
+	}
+
+	// Config with distinct primary and backup models so we can tell
+	// which resolution path was taken.
+	cfg := &config.Config{
+		ReviewModel:       "primary-model",
+		ReviewBackupModel: "backup-model",
+	}
+
+	// Primary and backup are the same agent. The helper should NOT
+	// treat this as "using backup" — the CanonicalName comparison
+	// against preferredAgent must prevent that.
+	result, model := applyModelForAgent(
+		a,
+		"codex", // preferred (same as backup)
+		"codex", // backup (same as preferred)
+		"",      // no CLI model
+		"",      // no repo path
+		cfg,
+		"review",
+		"standard",
+	)
+
+	codexAgent, ok := result.(*agent.CodexAgent)
+	if !ok {
+		t.Fatalf("expected *CodexAgent, got %T", result)
+	}
+	// Should pick the primary workflow model, not the backup.
+	if model != "primary-model" {
+		t.Errorf("expected primary model %q, got %q", "primary-model", model)
+	}
+	if codexAgent.Model != "primary-model" {
+		t.Errorf(
+			"expected agent model %q, got %q",
+			"primary-model", codexAgent.Model,
+		)
+	}
+}
+
 func TestRefineFlagValidation(t *testing.T) {
 	tests := []struct {
 		name    string

cmd/roborev/review.go

@@ -379,20 +379,23 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName
 	}
 
 	// Resolve agent using workflow-specific resolution (matches daemon behavior)
-	agentName = config.ResolveAgentForWorkflow(agentName, repoPath, cfg, workflow, reasoning)
+	preferredAgent := config.ResolveAgentForWorkflow(agentName, repoPath, cfg, workflow, reasoning)
+	backupAgent := config.ResolveBackupAgentForWorkflow(repoPath, cfg, workflow)
 
-	// Get the agent
-	a, err := agent.GetAvailableWithConfig(agentName, cfg)
+	// Get the agent (try backup before hardcoded chain)
+	a, err := agent.GetAvailableWithConfig(preferredAgent, cfg, backupAgent)
 	if err != nil {
 		return fmt.Errorf("get agent: %w", err)
 	}
 
-	// Resolve model using workflow-specific resolution (matches daemon behavior)
-	model = config.ResolveModelForWorkflow(model, repoPath, cfg, workflow, reasoning)
-
-	// Configure agent with model and reasoning
+	// Configure agent with model and reasoning. applyModelForAgent
+	// handles backup-vs-primary model resolution.
 	reasoningLevel := agent.ParseReasoningLevel(reasoning)
-	a = a.WithReasoning(reasoningLevel).WithModel(model)
+	a = a.WithReasoning(reasoningLevel)
+	a, model = applyModelForAgent(
+		a, preferredAgent, backupAgent,
+		model, repoPath, cfg, workflow, reasoning,
+	)
 
 	// Configure provider for pi agent
 	if provider != "" {

internal/agent/acp.go

@@ -1507,7 +1507,10 @@ func configuredACPAgent(cfg *config.Config) *ACPAgent {
 // It treats cfg.ACP.Name as an alias for "acp" and applies cfg.ACP command/mode/model
 // at resolution time instead of package-init time.
 // It also applies command overrides for other agents (codex, claude, cursor, pi).
-func GetAvailableWithConfig(preferred string, cfg *config.Config) (Agent, error) {
+//
+// Optional backup agent names are tried after the preferred agent but
+// before the hardcoded fallback chain (see GetAvailable).
+func GetAvailableWithConfig(preferred string, cfg *config.Config, backups ...string) (Agent, error) {
 	rawPreferred := strings.TrimSpace(preferred)
 	preferred = resolveAlias(rawPreferred)
 
@@ -1525,11 +1528,11 @@ func GetAvailableWithConfig(preferred string, cfg *config.Config) (Agent, error)
 			}
 		}
 
-		// Finally fall back to normal auto-selection.
-		return GetAvailable("")
+		// Finally fall back to normal auto-selection (with backups).
+		return GetAvailable("", backups...)
 	}
 
-	resolved, err := GetAvailable(preferred)
+	resolved, err := GetAvailable(preferred, backups...)
 	if err != nil {
 		return nil, err
 	}

internal/agent/acp_test.go

@@ -1191,3 +1191,33 @@ func TestACPNameDoesNotMatchCanonicalRequest(t *testing.T) {
 		t.Fatalf("Expected claude-code agent, got %q", resolved.Name())
 	}
 }
+
+func TestGetAvailableWithConfigPassesBackupsThrough(t *testing.T) {
+	// When preferred is unavailable, backups should be tried before the chain.
+	fakeBin := t.TempDir()
+	binName := "gemini"
+	if runtime.GOOS == "windows" {
+		binName += ".exe"
+	}
+	geminiBin := filepath.Join(fakeBin, binName)
+	if err := os.WriteFile(geminiBin, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil {
+		t.Fatalf("create fake gemini binary: %v", err)
+	}
+	t.Setenv("PATH", fakeBin)
+
+	originalRegistry := registry
+	registry = map[string]Agent{
+		"codex":  NewCodexAgent("definitely-not-on-path"),
+		"gemini": NewGeminiAgent(""),
+	}
+	t.Cleanup(func() { registry = originalRegistry })
+
+	cfg := &config.Config{}
+	resolved, err := GetAvailableWithConfig("codex", cfg, "gemini")
+	if err != nil {
+		t.Fatalf("expected fallback, got error: %v", err)
+	}
+	if resolved.Name() != "gemini" {
+		t.Fatalf("expected backup agent 'gemini', got %q", resolved.Name())
+	}
+}