fix(review): preflight reviewer binary before creating review runs (#2092)

backend/internal/daemon/daemon.go

@@ -133,6 +133,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 {

backend/internal/review/launcher.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/aoagents/agent-orchestrator/backend/internal/domain"
@@ -17,6 +20,12 @@ const cancelInterruptDelay = 150 * time.Millisecond
 // 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)
@@ -67,6 +76,41 @@ func NewLauncher(reviewers ports.ReviewerResolver, runtime reviewerRuntime) Laun
 	return &agentLauncher{reviewers: reviewers, runtime: runtime}
 }
 
+// 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 {

backend/internal/review/launcher_test.go

@@ -2,6 +2,7 @@ package review
 
 import (
 	"context"
+	"errors"
 	"strings"
 	"testing"
 
@@ -54,6 +55,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
@@ -196,3 +213,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)
+	}
+}

backend/internal/review/review.go

@@ -276,6 +276,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))

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)
 	}
@@ -665,6 +674,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)
 	}
@@ -734,3 +746,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))
+	}
+}

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" });
 });

frontend/src/renderer/components/ProjectSettingsForm.tsx

@@ -26,7 +26,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 +332,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}
 						/>
 					</Field>
 				</CardContent>
@@ -391,17 +394,56 @@ function PermissionModeSelect({
 	);
 }
 
-function ReviewerSelect({ id, value, onChange }: { id: string; value: string; onChange: (value: string) => void }) {
+function ReviewerSelect({
+	id,
+	value,
+	onChange,
+	authorized,
+	installed,
+	supported,
+}: {
+	id: string;
+	value: string;
+	onChange: (value: string) => void;
+	authorized?: components["schemas"]["AgentInfo"][];
+	installed?: components["schemas"]["AgentInfo"][];
+	supported?: components["schemas"]["AgentInfo"][];
+}) {
+	const fallbackReviewers: components["schemas"]["AgentInfo"][] = [...KNOWN_REVIEWER_HARNESS_IDS].map((id) => ({
+		id,
+		label: id,
+	}));
+	const supportedReviewers = (supported ?? []).filter((a) => KNOWN_REVIEWER_HARNESS_IDS.has(a.id));
+	const displayReviewers = supportedReviewers.length > 0 ? supportedReviewers : fallbackReviewers;
+	const installedById = new Map((installed ?? []).map((a) => [a.id, a]));
+	const authorizedIds = new Set((authorized ?? []).map((a) => a.id));
+	const options = displayReviewers.map((agent) => {
+		const installedAgent = installedById.get(agent.id);
+		const isAuthorized = authorizedIds.has(agent.id) || installedAgent?.authStatus === "authorized";
+		const isSelectable = isAuthorized || !installedAgent;
+		return {
+			...agent,
+			disabled: !isSelectable,
+			reason: !installedAgent ? "Needs install" : !isAuthorized ? "Needs auth" : "",
+		};
+	});
 	return (
 		<Select value={value || "__default__"} onValueChange={(v) => onChange(v === "__default__" ? "" : v)}>
 			<SelectTrigger id={id} className="h-control-form w-full text-control">
 				<SelectValue />
 			</SelectTrigger>
 			<SelectContent>
 				<SelectItem value="__default__">Project default</SelectItem>
-				{REVIEWER_OPTIONS.map((reviewer) => (
-					<SelectItem key={reviewer} value={reviewer}>
-						{reviewer}
+				{options.map((reviewer) => (
+					<SelectItem key={reviewer.id} value={reviewer.id} disabled={reviewer.disabled}>
+						<span className="flex min-w-0 w-full items-center justify-between gap-4">
+							<span className="truncate">{reviewer.label}</span>
+							{reviewer.reason && (
+								<span className="inline-flex shrink-0 items-center gap-1 text-caption text-muted-foreground">
+									{reviewer.reason}
+								</span>
+							)}
+						</span>
 					</SelectItem>
 				))}
 			</SelectContent>