diff --git a/backend/internal/daemon/daemon.go b/backend/internal/daemon/daemon.go index 039ca7b6f5..5656dc205c 100644 --- a/backend/internal/daemon/daemon.go +++ b/backend/internal/daemon/daemon.go @@ -150,6 +150,7 @@ func Run() error { return fmt.Errorf("wire session service: %w", err) } lcStack.trackerDone = startTrackerIntake(ctx, store, sessionSvc, log) + agentSvc := agentsvc.New() go func() { if _, err := agentSvc.Refresh(ctx); err != nil { diff --git a/backend/internal/review/launcher.go b/backend/internal/review/launcher.go index dd21b922f8..65f783eaa9 100644 --- a/backend/internal/review/launcher.go +++ b/backend/internal/review/launcher.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os" + "os/exec" "path/filepath" "strings" "time" @@ -21,6 +22,12 @@ const reviewerTaskMessagePrefix = "Read and follow the AO review task in `" // It is the side of the engine that talks to the reviewer registry and runtime; // the engine owns the orchestration and persistence. type Launcher interface { + // Preflight checks whether the reviewer for the given harness is available + // to run (binary on PATH, etc.) without starting a runtime pane. It runs + // only when a reviewer launch is actually required, after ReviewRun rows + // have been created. On failure the engine's Trigger() calls failRuns() to + // mark those rows as failed, matching the existing Spawn failure semantics. + Preflight(ctx context.Context, harness domain.ReviewerHarness, workspacePath string) error // Spawn launches a fresh reviewer and returns the runtime handle id of the // live pane (stable per worker, reused across passes). Spawn(ctx context.Context, spec LaunchSpec) (handleID string, err error) @@ -74,6 +81,41 @@ func NewLauncher(reviewers ports.ReviewerResolver, runtime reviewerRuntime, data return &agentLauncher{reviewers: reviewers, runtime: runtime, dataDir: dataDir} } +// Preflight checks whether the reviewer for the given harness can be launched +// without starting a runtime pane. It uses the same source of truth as Spawn: +// resolve the adapter, build the real ReviewCommand, and validate the +// executable. The only difference from Spawn is that Preflight stops before +// runtime.Create(). +func (l *agentLauncher) Preflight(ctx context.Context, harness domain.ReviewerHarness, workspacePath string) error { + reviewer, ok := l.reviewers.Reviewer(harness) + if !ok { + return fmt.Errorf("no reviewer adapter for harness %q", harness) + } + cmd, err := reviewer.ReviewCommand(ctx, ports.ReviewInvocation{WorkspacePath: workspacePath}) + if err != nil { + return fmt.Errorf("reviewer command: %w", err) + } + if len(cmd.Argv) == 0 { + return fmt.Errorf("reviewer produced empty command") + } + // Unwrap any leading env KEY=value ... prefix so the real binary is + // validated. Mirrors launchBinary in the session manager, which already + // skips the same prefix to validate the worker agent binary. + bin := cmd.Argv[0] + if filepath.Base(bin) == "env" { + for _, arg := range cmd.Argv[1:] { + if !strings.Contains(arg, "=") { + bin = arg + break + } + } + } + if _, err := exec.LookPath(bin); err != nil { + return fmt.Errorf("reviewer binary %q not found: %w", bin, err) + } + return nil +} + // reviewerHandleID is the stable runtime handle for a worker's reviewer pane, so // one live reviewer is reused across passes. func reviewerHandleID(workerID domain.SessionID) string { diff --git a/backend/internal/review/launcher_test.go b/backend/internal/review/launcher_test.go index 2c311ce622..d344363b9c 100644 --- a/backend/internal/review/launcher_test.go +++ b/backend/internal/review/launcher_test.go @@ -2,6 +2,7 @@ package review import ( "context" + "errors" "os" "path/filepath" "strings" @@ -56,6 +57,22 @@ func (f *fakeCancellableReviewer) ReviewCancel(context.Context) (ports.ReviewCan return ports.ReviewCancelSpec{Mode: mode, Interrupts: f.interrupts}, nil } +type fakeReviewerForPreflight struct { + CommandErr error + Argv []string +} + +func (f *fakeReviewerForPreflight) ReviewCommand(_ context.Context, _ ports.ReviewInvocation) (ports.ReviewCommandSpec, error) { + if f.CommandErr != nil { + return ports.ReviewCommandSpec{}, f.CommandErr + } + return ports.ReviewCommandSpec{Argv: f.Argv}, nil +} + +func (f *fakeReviewerForPreflight) ReviewMessage(_ context.Context, _ ports.ReviewInvocation) (string, error) { + return "", nil +} + type fakeReviewerResolver struct { reviewer ports.Reviewer ok bool @@ -303,3 +320,58 @@ func TestLauncherSpawnNoAdapter(t *testing.T) { t.Fatalf("err = %v, want no-adapter", err) } } + +func TestLauncherPreflightResolvesAdapter(t *testing.T) { + reviewer := &fakeReviewerForPreflight{Argv: []string{"go"}} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, &fakeRuntime{}, "") + if err := l.Preflight(context.Background(), domain.ReviewerClaudeCode, "/ws/mer-1"); err != nil { + t.Fatalf("Preflight: %v", err) + } +} + +func TestLauncherPreflightNoAdapter(t *testing.T) { + l := NewLauncher(fakeReviewerResolver{ok: false}, &fakeRuntime{}, "") + if err := l.Preflight(context.Background(), domain.ReviewerClaudeCode, "/ws/mer-1"); err == nil || !strings.Contains(err.Error(), "no reviewer adapter") { + t.Fatalf("err = %v, want 'no reviewer adapter'", err) + } +} + +func TestLauncherPreflightReviewCommandError(t *testing.T) { + reviewer := &fakeReviewerForPreflight{CommandErr: errors.New("reviewer unavailable")} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, &fakeRuntime{}, "") + if err := l.Preflight(context.Background(), domain.ReviewerClaudeCode, "/ws/mer-1"); err == nil || !strings.Contains(err.Error(), "reviewer unavailable") { + t.Fatalf("err = %v, want containing 'reviewer unavailable'", err) + } +} + +func TestLauncherPreflightEmptyArgv(t *testing.T) { + reviewer := &fakeReviewerForPreflight{} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, &fakeRuntime{}, "") + if err := l.Preflight(context.Background(), domain.ReviewerClaudeCode, "/ws/mer-1"); err == nil || !strings.Contains(err.Error(), "empty command") { + t.Fatalf("err = %v, want 'empty command'", err) + } +} + +func TestLauncherPreflightBinaryNotFound(t *testing.T) { + reviewer := &fakeReviewerForPreflight{Argv: []string{"this-binary-does-not-exist-12345"}} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, &fakeRuntime{}, "") + if err := l.Preflight(context.Background(), domain.ReviewerClaudeCode, "/ws/mer-1"); err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("err = %v, want 'not found'", err) + } +} + +func TestLauncherPreflightSkipsEnvPrefix(t *testing.T) { + reviewer := &fakeReviewerForPreflight{Argv: []string{"env", "OPENCODE_CONFIG_CONTENT=cfg", "go"}} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, &fakeRuntime{}, "") + if err := l.Preflight(context.Background(), domain.ReviewerClaudeCode, "/ws/mer-1"); err != nil { + t.Fatalf("Preflight: %v", err) + } +} + +func TestLauncherPreflightEnvPrefixWithMissingBinary(t *testing.T) { + reviewer := &fakeReviewerForPreflight{Argv: []string{"env", "KEY=val", "nonexistent-binary-999"}} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, &fakeRuntime{}, "") + if err := l.Preflight(context.Background(), domain.ReviewerClaudeCode, "/ws/mer-1"); err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("err = %v, want 'not found'", err) + } +} diff --git a/backend/internal/review/review.go b/backend/internal/review/review.go index 51f55f35d6..4596de4b71 100644 --- a/backend/internal/review/review.go +++ b/backend/internal/review/review.go @@ -296,6 +296,13 @@ func (e *Engine) Trigger(ctx stdctx.Context, workerID domain.SessionID) (Trigger } } if handleID == "" { + // Preflight before launching a new reviewer pane. Runs only when a + // fresh launch is actually required (not when an existing pane is + // reused via Notify). On failure failRuns marks the created runs as + // failed, matching the Spawn error semantics. + if err := e.launcher.Preflight(ctx, harness, worker.Metadata.WorkspacePath); err != nil { + return TriggerResult{}, failRuns(0, fmt.Errorf("reviewer preflight: %w", err)) + } h, err := e.launcher.Spawn(ctx, reviewLaunchSpec(worker, harness, created[0], queue, 0)) if err != nil { return TriggerResult{}, failRuns(0, fmt.Errorf("launch reviewer: %w", err)) diff --git a/backend/internal/review/review_test.go b/backend/internal/review/review_test.go index 813c4147ba..b28c46fe4d 100644 --- a/backend/internal/review/review_test.go +++ b/backend/internal/review/review_test.go @@ -164,6 +164,8 @@ type fakeLauncher struct { cancelledHarness domain.ReviewerHarness specs []LaunchSpec handles []string + preflightErr error + preflighted bool } func (f *fakeLauncher) Spawn(_ context.Context, spec LaunchSpec) (string, error) { @@ -193,6 +195,10 @@ func (f *fakeLauncher) Cancel(_ context.Context, handleID string, harness domain f.cancelledHarness = harness return f.cancelErr } +func (f *fakeLauncher) Preflight(_ context.Context, _ domain.ReviewerHarness, _ string) error { + f.preflighted = true + return f.preflightErr +} func liveWorker() domain.SessionRecord { return domain.SessionRecord{ @@ -478,6 +484,9 @@ func TestTriggerNotifiesLiveReviewerOnNewCommit(t *testing.T) { if !launcher.notified || launcher.spawned { t.Fatalf("expected notify on live reviewer: %+v", launcher) } + if launcher.preflighted { + t.Fatal("expected preflight not to run when reusing a live pane") + } if launcher.gotHandle != "review-mer-1" { t.Fatalf("notify handle = %q", launcher.gotHandle) } @@ -765,6 +774,9 @@ func TestTriggerSkipsApprovedAndRunningCurrentHead(t *testing.T) { if res.Created || len(res.CreatedRuns) != 0 || launcher.spawned || launcher.notified { t.Fatalf("expected no new work: res=%+v launcher=%+v", res, launcher) } + if launcher.preflighted { + t.Fatal("expected preflight not to run") + } if len(res.Reviews) != 2 || res.Reviews[0].Status != ReviewStateUpToDate || res.Reviews[1].Status != ReviewStateRunning { t.Fatalf("review states = %+v", res.Reviews) } @@ -834,3 +846,56 @@ func TestListReturnsHandleAndRuns(t *testing.T) { t.Fatalf("list = %+v", got) } } + +func TestTriggerPreflightFailureRecordsFailedRun(t *testing.T) { + store := &fakeStore{} + launcher := &fakeLauncher{preflightErr: fmt.Errorf("codex: %w", ports.ErrAgentBinaryNotFound)} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + _, err := eng.Trigger(context.Background(), "mer-1") + if err == nil { + t.Fatal("expected error from preflight, got nil") + } + if !errors.Is(err, ports.ErrAgentBinaryNotFound) { + t.Fatalf("err = %v, want wrapped ErrAgentBinaryNotFound", err) + } + if !launcher.preflighted { + t.Fatal("expected Preflight to be called") + } + if launcher.spawned { + t.Fatal("expected no spawn attempt when preflight fails") + } + if len(store.runs) != 1 { + t.Fatalf("expected 1 review run (failed), got %d", len(store.runs)) + } + run := store.runs[0] + if run.Status != domain.ReviewRunFailed || run.Verdict != domain.VerdictNone { + t.Fatalf("run = %+v, want failed with no verdict", run) + } + if !strings.Contains(run.Body, "codex") || !strings.Contains(run.Body, ports.ErrAgentBinaryNotFound.Error()) { + t.Fatalf("run body = %q, want preflight cause", run.Body) + } +} + +func TestTriggerProceedsNormallyAfterSuccessfulPreflight(t *testing.T) { + store := &fakeStore{} + launcher := &fakeLauncher{handle: "review-mer-1"} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if !launcher.preflighted { + t.Fatal("expected Preflight to be called") + } + if !res.Created || res.ReviewerHandleID != "review-mer-1" { + t.Fatalf("result = %+v", res) + } + if !launcher.spawned { + t.Fatal("expected spawn after successful preflight") + } + if len(store.runs) != 1 { + t.Fatalf("expected 1 review run, got %d", len(store.runs)) + } +} diff --git a/frontend/src/renderer/components/ConnectMobileModal.test.tsx b/frontend/src/renderer/components/ConnectMobileModal.test.tsx index 936bb004f5..083dca086e 100644 --- a/frontend/src/renderer/components/ConnectMobileModal.test.tsx +++ b/frontend/src/renderer/components/ConnectMobileModal.test.tsx @@ -2,6 +2,6 @@ import { expect, test } from "vitest"; import { pairingPayload } from "./ConnectMobileModal"; test("QR payload carries host, port, and password for one-scan connect", () => { - const s = pairingPayload("192.168.1.42", 3011, "xKb1Z3A1"); - expect(JSON.parse(s)).toEqual({ v: 1, host: "192.168.1.42", port: 3011, password: "xKb1Z3A1" }); + const s = pairingPayload("192.168.1.42", 3011, "fake-password-for-testing"); + expect(JSON.parse(s)).toEqual({ v: 1, host: "192.168.1.42", port: 3011, password: "fake-password-for-testing" }); }); diff --git a/frontend/src/renderer/components/ProjectSettingsForm.tsx b/frontend/src/renderer/components/ProjectSettingsForm.tsx index 7355c129d2..14080b057b 100644 --- a/frontend/src/renderer/components/ProjectSettingsForm.tsx +++ b/frontend/src/renderer/components/ProjectSettingsForm.tsx @@ -1,5 +1,6 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { useState } from "react"; +import { TriangleAlert } from "lucide-react"; import type { components } from "../../api/schema"; import { agentsQueryKey, agentsQueryOptions, refreshAgents } from "../hooks/useAgentsQuery"; import { useWorkspaceQuery, workspaceQueryKey } from "../hooks/useWorkspaceQuery"; @@ -26,7 +27,7 @@ const PERMISSION_MODE_OPTIONS = [ { value: "bypass-permissions", label: "Bypass permissions" }, ] as const; -const REVIEWER_OPTIONS = ["claude-code", "codex", "opencode"] as const; +const KNOWN_REVIEWER_HARNESS_IDS = new Set(["claude-code", "codex", "opencode"]); const projectQueryKey = (id: string) => ["project", id] as const; @@ -332,6 +333,9 @@ function SettingsBody({ project, projectId, onSaved }: { project: Project; proje id="reviewerHarness" value={form.reviewerHarness} onChange={(v) => setForm((f) => ({ ...f, reviewerHarness: v }))} + authorized={agentCatalog?.authorized} + installed={agentCatalog?.installed} + supported={agentCatalog?.supported} /> @@ -391,17 +395,84 @@ function PermissionModeSelect({ ); } -function ReviewerSelect({ id, value, onChange }: { id: string; value: string; onChange: (value: string) => void }) { +const REVIEWER_AGENT_PRIORITY = ["claude-code", "codex", "cursor", "opencode", "aider"] as const; +const REVIEWER_AGENT_PRIORITY_RANK = new Map( + REVIEWER_AGENT_PRIORITY.map((agent, index) => [agent, index]), +); + +function agentLabelCompare(a: components["schemas"]["AgentInfo"], b: components["schemas"]["AgentInfo"]): number { + return a.label.localeCompare(b.label) || a.id.localeCompare(b.id); +} + +function ReviewerSelect({ + id, + value, + onChange, + disabled = false, + authorized, + installed, + supported, +}: { + id: string; + value: string; + onChange: (value: string) => void; + disabled?: boolean; + authorized?: components["schemas"]["AgentInfo"][]; + installed?: components["schemas"]["AgentInfo"][]; + supported?: components["schemas"]["AgentInfo"][]; +}) { + const fallbackAgents: components["schemas"]["AgentInfo"][] = [...KNOWN_REVIEWER_HARNESS_IDS].map((id) => ({ + id, + label: id, + })); + const filteredSupported = (supported ?? fallbackAgents).filter((a) => KNOWN_REVIEWER_HARNESS_IDS.has(a.id)); + const supportedAgents = filteredSupported.length > 0 ? filteredSupported : fallbackAgents; + const installedAgents = installed ?? supportedAgents; + const authorizedAgents = authorized ?? supportedAgents; + const authorizedIds = new Set(authorizedAgents.map((agent) => agent.id)); + const installedById = new Map(installedAgents.map((agent) => [agent.id, agent])); + const options = supportedAgents + .map((agent) => { + const installedAgent = installedById.get(agent.id); + const authStatus = installedAgent?.authStatus; + const isAuthorized = authorizedIds.has(agent.id) || authStatus === "authorized"; + const isAuthUnknown = Boolean(installedAgent) && !isAuthorized && authStatus !== "unauthorized"; + const isSelectable = isAuthorized || isAuthUnknown; + const rank = isAuthorized ? 0 : isAuthUnknown ? 1 : installedAgent ? 2 : 3; + return { + ...agent, + disabled: !isSelectable, + priorityRank: REVIEWER_AGENT_PRIORITY_RANK.get(agent.id) ?? Number.MAX_SAFE_INTEGER, + rank, + reason: !installedAgent ? "Needs install" : isAuthUnknown ? "Auth unknown" : !isAuthorized ? "Needs auth" : "", + warning: isAuthUnknown, + }; + }) + .sort((a, b) => a.rank - b.rank || a.priorityRank - b.priorityRank || agentLabelCompare(a, b)); + return ( - onChange(v === "__default__" ? "" : v)} disabled={disabled}> + - + Project default - {REVIEWER_OPTIONS.map((reviewer) => ( - - {reviewer} + {options.map((agent) => ( + + + {agent.label} + {agent.reason && ( + + {agent.warning && + )} + ))}