diff --git a/internal/agent/acp.go b/internal/agent/acp.go index ace326d3..bbe7e73c 100644 --- a/internal/agent/acp.go +++ b/internal/agent/acp.go @@ -1485,13 +1485,11 @@ func isConfiguredACPAgentName(name string, cfg *config.Config) bool { return false } - // Compare both raw and alias-normalized forms so ACP names configured as - // aliases (for example "claude") still match canonical requests - // (for example "claude-code"). - if rawName == configuredName { - return true - } - return resolveAlias(rawName) == resolveAlias(configuredName) + // Exact match only — no alias resolution. This prevents collisions + // where an alias like "agent" → "cursor" would incorrectly route + // cursor requests to ACP. Callers pass rawPreferred (pre-alias) so + // `acp.name = "claude"` matches request "claude" but not "claude-code". + return rawName == configuredName } func configuredACPAgent(cfg *config.Config) *ACPAgent { @@ -1509,9 +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. func GetAvailableWithConfig(preferred string, cfg *config.Config) (Agent, error) { - preferred = resolveAlias(strings.TrimSpace(preferred)) + rawPreferred := strings.TrimSpace(preferred) + preferred = resolveAlias(rawPreferred) - if isConfiguredACPAgentName(preferred, cfg) { + if isConfiguredACPAgentName(rawPreferred, cfg) { acpAgent := configuredACPAgent(cfg) if _, err := exec.LookPath(acpAgent.CommandName()); err == nil { return acpAgent, nil diff --git a/internal/agent/acp_test.go b/internal/agent/acp_test.go index 168989d7..31a23248 100644 --- a/internal/agent/acp_test.go +++ b/internal/agent/acp_test.go @@ -321,7 +321,7 @@ func TestGetAvailableWithConfigResolvedACPBranchFallsBackWhenConfiguredCommandMi }, } - resolved, err := GetAvailableWithConfig("nonexistent-agent", cfg) + resolved, err := GetAvailableWithConfig("custom-acp", cfg) if err != nil { t.Fatalf("GetAvailableWithConfig failed: %v", err) } @@ -1113,3 +1113,81 @@ func TestReadTextFileWindow(t *testing.T) { } }) } + +func TestACPAliasCollisionFixed(t *testing.T) { + // When acp.name = "agent", requesting "cursor" should resolve to the + // real cursor agent, not to ACP via the "agent" → "cursor" alias. + // The cursor agent's binary is called "agent" (not "cursor"). + fakeBin := t.TempDir() + agentBin := "agent" + if runtime.GOOS == "windows" { + agentBin += ".exe" + } + agentPath := filepath.Join(fakeBin, agentBin) + if err := os.WriteFile(agentPath, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("failed to create fake agent binary: %v", err) + } + t.Setenv("PATH", fakeBin) + + cfg := &config.Config{ + ACP: &config.ACPAgentConfig{ + Name: "agent", + Command: "acp-agent", + }, + } + + resolved, err := GetAvailableWithConfig("cursor", cfg) + if err != nil { + t.Fatalf("GetAvailableWithConfig failed: %v", err) + } + + if resolved.Name() != "cursor" { + t.Fatalf("Expected cursor agent, got %q", resolved.Name()) + } +} + +func TestGetAvailableWithConfigUnknownAgentErrors(t *testing.T) { + cfg := &config.Config{} + + _, err := GetAvailableWithConfig("typo-agent", cfg) + if err == nil { + t.Fatal("Expected error for unknown agent name") + } + if !strings.Contains(err.Error(), "unknown agent") { + t.Fatalf("Expected 'unknown agent' error, got: %v", err) + } +} + +func TestACPNameDoesNotMatchCanonicalRequest(t *testing.T) { + // acp.name = "claude" should match request "claude" but NOT "claude-code". + // Requesting the canonical name should go to the real agent, not ACP. + fakeBin := t.TempDir() + binName := "claude" + if runtime.GOOS == "windows" { + binName += ".exe" + } + claudePath := filepath.Join(fakeBin, binName) + if err := os.WriteFile(claudePath, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("failed to create fake claude binary: %v", err) + } + t.Setenv("PATH", fakeBin) + + cfg := &config.Config{ + ACP: &config.ACPAgentConfig{ + Name: "claude", + Command: defaultACPCommand, + }, + } + + resolved, err := GetAvailableWithConfig("claude-code", cfg) + if err != nil { + t.Fatalf("GetAvailableWithConfig failed: %v", err) + } + + if resolved.Name() == "acp" { + t.Fatalf("Request for 'claude-code' should not route to ACP when acp.name='claude'") + } + if resolved.Name() != "claude-code" { + t.Fatalf("Expected claude-code agent, got %q", resolved.Name()) + } +} diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 41d2b5ea..ab5992e1 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -5,6 +5,8 @@ import ( "fmt" "io" "os/exec" + "sort" + "strings" "sync" "sync/atomic" ) @@ -103,9 +105,24 @@ func SetAnthropicAPIKey(key string) { anthropicAPIKey.Store(key) } +// UnknownAgentError is returned when a requested agent name is not +// in the registry and is not a recognized alias. +type UnknownAgentError struct { + Name string + Known []string +} + +func (e *UnknownAgentError) Error() string { + return fmt.Sprintf( + "unknown agent %q (known: %s)", + e.Name, strings.Join(e.Known, ", "), + ) +} + // aliases maps short names to full agent names var aliases = map[string]string{ "claude": "claude-code", + "agent": "cursor", } // resolveAlias returns the canonical agent name, resolving aliases @@ -172,6 +189,19 @@ func GetAvailable(preferred string) (Agent, error) { // Resolve alias upfront for consistent comparisons preferred = resolveAlias(preferred) + // Reject unknown agent names (typos, config mistakes). + // Known-but-unavailable agents still fall back below. + if preferred != "" { + if _, ok := registry[preferred]; !ok { + known := Available() + sort.Strings(known) + return nil, &UnknownAgentError{ + Name: preferred, + Known: known, + } + } + } + // Try preferred agent first if preferred != "" && IsAvailable(preferred) { return Get(preferred) diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index bfbcbde1..460bb7ad 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -5,6 +5,8 @@ import ( "context" "errors" "os" + "path/filepath" + "runtime" "strings" "sync" "testing" @@ -31,6 +33,33 @@ func TestAgentRegistry(t *testing.T) { } } +func TestCanonicalNameAliases(t *testing.T) { + tests := []struct { + input string + want string + }{ + {input: "claude", want: "claude-code"}, + {input: "agent", want: "cursor"}, + {input: "cursor", want: "cursor"}, + } + + for _, tt := range tests { + if got := CanonicalName(tt.input); got != tt.want { + t.Errorf("CanonicalName(%q) = %q, want %q", tt.input, got, tt.want) + } + } +} + +func TestGetSupportsAgentAlias(t *testing.T) { + a, err := Get("agent") + if err != nil { + t.Fatalf("Get(agent) returned error: %v", err) + } + if a.Name() != "cursor" { + t.Fatalf("Get(agent) resolved to %q, want %q", a.Name(), "cursor") + } +} + func TestAvailableAgents(t *testing.T) { agents := Available() if len(agents) < len(expectedAgents) { @@ -361,3 +390,47 @@ func TestAgentReviewPassesModelFlag(t *testing.T) { }) } } + +func TestGetAvailableRejectsUnknownAgent(t *testing.T) { + _, err := GetAvailable("typo-agent") + if err == nil { + t.Fatal("Expected error for unknown agent name") + } + if !strings.Contains(err.Error(), "unknown agent") { + t.Fatalf("Expected 'unknown agent' error, got: %v", err) + } +} + +func TestGetAvailableFallsBackForKnownUnavailable(t *testing.T) { + // Isolate registry: "codex" has a missing binary, "claude-code" + // has its binary on PATH. Request "codex" and verify fallback + // returns "claude-code" without an "unknown agent" error. + fakeBin := t.TempDir() + binName := "claude" + if runtime.GOOS == "windows" { + binName += ".exe" + } + claudeBin := filepath.Join(fakeBin, binName) + if err := os.WriteFile(claudeBin, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("failed to create fake claude binary: %v", err) + } + t.Setenv("PATH", fakeBin) + + originalRegistry := registry + registry = map[string]Agent{ + "codex": NewCodexAgent("definitely-not-on-path"), + "claude-code": NewClaudeAgent(""), + } + t.Cleanup(func() { registry = originalRegistry }) + + resolved, err := GetAvailable("codex") + if err != nil { + if strings.Contains(err.Error(), "unknown agent") { + t.Fatalf("Known agent should not produce unknown agent error: %v", err) + } + t.Fatalf("Expected fallback, got error: %v", err) + } + if resolved.Name() != "claude-code" { + t.Fatalf("Expected fallback to 'claude-code', got %q", resolved.Name()) + } +} diff --git a/internal/daemon/server.go b/internal/daemon/server.go index b872ea94..8211f85c 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -606,9 +606,14 @@ func (s *Server) handleEnqueue(w http.ResponseWriter, r *http.Request) { // 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. + // Unknown agent names (typos) return 400; no agents at all returns 503. if resolved, err := agent.GetAvailableWithConfig(agentName, cfg); err != nil { - writeError(w, http.StatusServiceUnavailable, fmt.Sprintf("no review agent available: %v", err)) + var unknownErr *agent.UnknownAgentError + if errors.As(err, &unknownErr) { + writeError(w, http.StatusBadRequest, fmt.Sprintf("invalid agent: %v", err)) + } else { + writeError(w, http.StatusServiceUnavailable, fmt.Sprintf("no review agent available: %v", err)) + } return } else { agentName = resolved.Name() @@ -1972,7 +1977,12 @@ func (s *Server) handleFixJob(w http.ResponseWriter, r *http.Request) { reasoning := "standard" agentName := config.ResolveAgentForWorkflow("", parentJob.RepoPath, cfg, "fix", reasoning) if resolved, err := agent.GetAvailableWithConfig(agentName, cfg); err != nil { - writeError(w, http.StatusServiceUnavailable, fmt.Sprintf("no agent available: %v", err)) + var unknownErr *agent.UnknownAgentError + if errors.As(err, &unknownErr) { + writeError(w, http.StatusBadRequest, fmt.Sprintf("invalid agent: %v", err)) + } else { + writeError(w, http.StatusServiceUnavailable, fmt.Sprintf("no agent available: %v", err)) + } return } else { agentName = resolved.Name() diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index 92dc6e35..34a704d8 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -908,6 +908,7 @@ func TestHandleEnqueueAgentAvailability(t *testing.T) { tests := []struct { name string requestAgent string + defaultAgent string mockBinaries []string // binary names to place in PATH expectedAgent string // expected agent stored in job expectedCode int // expected HTTP status code @@ -933,6 +934,21 @@ func TestHandleEnqueueAgentAvailability(t *testing.T) { expectedAgent: "claude-code", expectedCode: http.StatusCreated, }, + { + name: "explicit agent alias resolves to cursor", + requestAgent: "agent", + mockBinaries: []string{"agent"}, + expectedAgent: "cursor", + expectedCode: http.StatusCreated, + }, + { + name: "default agent alias resolves to cursor", + requestAgent: "", + defaultAgent: "agent", + mockBinaries: []string{"agent"}, + expectedAgent: "cursor", + expectedCode: http.StatusCreated, + }, { name: "explicit codex kept when available", requestAgent: "codex", @@ -953,12 +969,21 @@ func TestHandleEnqueueAgentAvailability(t *testing.T) { mockBinaries: nil, expectedCode: http.StatusServiceUnavailable, }, + { + name: "unknown agent returns 400", + requestAgent: "typo-agent", + mockBinaries: nil, + expectedCode: http.StatusBadRequest, + }, } 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) + if tt.defaultAgent != "" { + server.configWatcher.Config().DefaultAgent = tt.defaultAgent + } // Isolate PATH: only mock binaries + git (no real agent CLIs) origPath := os.Getenv("PATH") diff --git a/internal/daemon/server_ops_test.go b/internal/daemon/server_ops_test.go index 59f73426..51899fcd 100644 --- a/internal/daemon/server_ops_test.go +++ b/internal/daemon/server_ops_test.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "time" @@ -906,3 +907,95 @@ func TestHandleFixJobStaleValidation(t *testing.T) { } }) } + +func TestHandleFixJobAgentAvailability(t *testing.T) { + // Create shared git repo + completed parent review job. + 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) + } + } + + tests := []struct { + name string + fixAgent string + mockBinaries []string + expectedCode int + }{ + { + name: "unknown fix agent returns 400", + fixAgent: "typo-agent", + mockBinaries: nil, + expectedCode: http.StatusBadRequest, + }, + { + name: "no agents available returns 503", + fixAgent: "codex", + mockBinaries: nil, + expectedCode: http.StatusServiceUnavailable, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server, db, tmpDir := newTestServer(t) + server.configWatcher.Config().FixAgent = tt.fixAgent + + repoDir := filepath.Join(tmpDir, "repo-fix-avail") + testutil.InitTestGitRepo(t, repoDir) + repo, _ := db.GetOrCreateRepo(repoDir) + commit, _ := db.GetOrCreateCommit( + repo.ID, "fix-avail-abc", "Author", "Subject", time.Now(), + ) + reviewJob, _ := db.EnqueueJob(storage.EnqueueOpts{ + RepoID: repo.ID, + CommitID: commit.ID, + GitRef: "fix-avail-abc", + Agent: "test", + }) + db.ClaimJob("w1") + db.CompleteJob(reviewJob.ID, "test", "prompt", "FAIL: issues found") + + // Isolate PATH + mockDir := t.TempDir() + mockScript := "#!/bin/sh\nexit 0\n" + 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) + } + } + origPath := os.Getenv("PATH") + os.Setenv("PATH", mockDir+string(os.PathListSeparator)+gitOnlyDir) + t.Cleanup(func() { os.Setenv("PATH", origPath) }) + + req := testutil.MakeJSONRequest( + t, http.MethodPost, "/api/job/fix", + fixJobRequest{ParentJobID: reviewJob.ID}, + ) + w := httptest.NewRecorder() + server.handleFixJob(w, req) + + if w.Code != tt.expectedCode { + t.Fatalf("Expected status %d, got %d: %s", + tt.expectedCode, w.Code, w.Body.String()) + } + }) + } +}