review errors

docs/operations.md

@@ -408,7 +408,7 @@ The fork's session guidance gets one more line (`forkGuidance` in `internal/agen
 
 Every new snapshot of a task gets a review: a separate agent session on the vendor the task's agent doesn't use reads the change and reports on it (`internal/agents/review.go`, `internal/store/review.go`). A turn that changes no files makes no snapshot and so no review, an interrupted turn makes no snapshot (see "Snapshots"), and a snapshot whose change (the diff below) has no files runs no reviewer: it gets a `reviews` row in state `skipped`, with no session, vendor or model. The snapshot at the end of a Merge turn that finishes into the squash (the turn ended cleanly with `merging_by` still set, read by `turnEnded` before it asks for the snapshot) gets no review and no `reviews` row at all, since the task closes a moment later; a Merge turn that errors or calls `stop_merge` is reviewed as usual. Besides `take_snapshot` (see "Snapshots"), nothing else starts a review, and nothing waits for one: reviews ride along for the human to look at when they want.
 
-- **Starting.** After `takeSnapshot` creates a snapshot, `afterSnapshot` runs `startReviewer`: it picks the vendor opposite the task's latest implementer session (`reviewVendor`: Claude Code for a Codex implementer, Codex for the others) and that vendor's model in `reviewModels`, checks the snapshot's commit out detached at `worktrees/<project-id>/review-<review-id>`, creates an `agent_sessions` row with `role` `reviewer` on the task (created by the task's creator), a `reviews` row (`running`, one per snapshot), and sends `reviewPrompt` as its first turn (actor `byre`): all of the task's work as `git diff` from the merge-base of the snapshot and the default branch, with its changed files, and the task's first message to its agent. Every review covers the whole task against the default branch, not only what changed since the previous snapshot, and a snapshot with no changes against it is `skipped`.
+- **Starting.** After `takeSnapshot` creates a snapshot, `afterSnapshot` runs `startReviewer`: it picks the vendor opposite the task's latest implementer session (`reviewVendor`: Claude Code for a Codex implementer, Codex for the others) and that vendor's model in `reviewModels`; when that vendor's CLI (`vendorCommands`: `claude`, `codex`, `agy`) isn't on the service's `PATH`, it uses the implementer's own vendor and model instead (`reviewerModel`). It then checks the snapshot's commit out detached at `worktrees/<project-id>/review-<review-id>`, creates an `agent_sessions` row with `role` `reviewer` on the task (created by the task's creator), a `reviews` row (`running`, one per snapshot), and sends `reviewPrompt` as its first turn (actor `byre`): all of the task's work as `git diff` from the merge-base of the snapshot and the default branch, with its changed files, and the task's first message to its agent. Every review covers the whole task against the default branch, not only what changed since the previous snapshot, and a snapshot with no changes against it is `skipped`.
 - **Running.** Reviewer sessions get `reviewGuidance` and only the `submit_review` tool, whose arguments (`reviewSchema`: `change`, a few sentences of Markdown on what the change does, required and non-empty; a Markdown `summary`, which the guidance and schema require to be exactly "No significant findings." when there are no findings and otherwise a bulleted list with one line per finding; `findings`, each with `severity`, `category`, `title`, `detail`, `path`, `line`, `end_line` and `images`; and top-level `images`) become the review's `result` and mark it `done`. The guidance encourages, without requiring, Mermaid blocks for a data flow, call graph, state machine or schema the change alters, and screenshots when the change alters something visible.
 - **Forwarding.** When a submitted review has findings, byre sends them to the task's live agent as a `byre` message (`forwardFindings` and `findingsMessage` in `internal/agents/review.go`): each finding's severity, title, location and detail, under a preamble asking the agent to check each one against the code and its brief, act on those that hold up, and say in its final message which it acted on and which it left and why. It goes through `Manager.Message`, so it is queued like any message while the agent is mid-turn. A task with no live agent session gets nothing, and the log says `review findings not forwarded`. Forwarding doesn't set any finding's state; **fix** and **ignore** stay available.
 - **Screenshots.** Reviewer sessions also get the Playwright MCP server (`playwrightMCP` in `internal/agents/review.go`: `npx @playwright/mcp` at a pinned version, headless Chrome, isolated profile, absolute file paths), passed to Claude as a `stdio` entry in `--mcp-config` and to Codex as `mcp_servers` in the thread's `config`. Its automatically named files go to `worktrees/<project-id>/review-<review-id>-media`, removed with the checkout. Each `images` entry is `{path, caption}`, the path absolute or relative to the checkout; at `submit_review` byre reads every one, refuses the whole submission (nothing recorded) for a missing file or one that isn't one of `store.ImageTypes` within `store.MaxBlobBytes`, then stores each as a blob and replaces its `path` with `blob`, since the files are deleted with the checkout. They run without the Stop-hook bounce, mid-turn delivery, end-of-turn commit, snapshots or turn summaries, and their `user.message` events carry no `head`. When the reviewer's turn ends byre completes its session, kills its process and removes the worktree; a review still `running` then fails with why (no submission, with the final message; an error; an interrupt). A process that exits mid-turn fails the review the same way. `StopReview` (`POST /api/reviews/{reviewId}/stop`, the Stop button on a running review's card) ends the reviewer the same way and fails the review ("the reviewer was stopped"), and `Watch` calls it for a reviewer whose process the health check finds `stuck` (no stream event for `StuckAfter`), which is how a reviewer whose connection died while the machine slept gets cleared. Merging or discarding the task fails its running reviews ("the task was closed") and ends their reviewers. On startup, a running reviewer resumes like any session, and a running review whose reviewer isn't `running` fails.

internal/agents/review.go

@@ -8,6 +8,7 @@ import (
 	"log/slog"
 	"net/http"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"time"
@@ -115,12 +116,15 @@ func playwrightMCP(outputDir string) mcp.Stdio {
 	}
 }
 
-func (m *Manager) implementerVendor(ctx context.Context, taskID string) api.AgentSessionVendor {
+var vendorCommands = map[api.AgentSessionVendor]string{api.Claude: "claude", api.Codex: "codex", api.Antigravity: "agy"}
+
+func (m *Manager) implementerModel(ctx context.Context, taskID string) (api.AgentSessionVendor, string) {
 	sessions, err := m.store.ListSessions(ctx, taskID)
 	if err != nil || len(sessions) == 0 {
-		return api.Claude
+		return api.Claude, reviewModels[api.Claude]
 	}
-	return sessions[len(sessions)-1].Vendor
+	last := sessions[len(sessions)-1]
+	return last.Vendor, last.Model
 }
 
 func reviewVendor(implementer api.AgentSessionVendor) api.AgentSessionVendor {
@@ -130,13 +134,22 @@ func reviewVendor(implementer api.AgentSessionVendor) api.AgentSessionVendor {
 	return api.Codex
 }
 
+func (m *Manager) reviewerModel(ctx context.Context, taskID string) (api.AgentSessionVendor, string) {
+	implVendor, implModel := m.implementerModel(ctx, taskID)
+	vendor := reviewVendor(implVendor)
+	_, err := exec.LookPath(vendorCommands[vendor])
+	if err != nil {
+		return implVendor, implModel
+	}
+	return vendor, reviewModels[vendor]
+}
+
 func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Snapshot) error {
 	project, err := m.store.GetProject(ctx, task.ProjectId)
 	if err != nil {
 		return err
 	}
-	vendor := reviewVendor(m.implementerVendor(ctx, task.Id))
+	vendor, model := m.reviewerModel(ctx, task.Id)
-	model := reviewModels[vendor]
 	prompt, err := m.reviewPrompt(ctx, task, project, snap)
 	if err != nil {
 		return err

internal/agents/snapshots.go

@@ -269,7 +269,7 @@ func (m *Manager) takeSnapshotTool(l *live) mcp.Tool {
 	}
 }
 
-const takeSnapshotDescription = "Snapshot a task's worktree now, uncommitted work included, and start a review of it: a reviewer on the other vendor reviews all of the task's work against the default branch. " +
+const takeSnapshotDescription = "Snapshot a task's worktree now, uncommitted work included, and start a review of it: a reviewer on the other vendor (or on yours, when the other isn't installed) reviews all of the task's work against the default branch. " +
 	"byre already does this at the end of every turn that changes files; call this to review work mid-turn, or to retry a review that failed (it snapshots even when nothing changed since the last snapshot)."
 
 func (m *Manager) dropSnapshots(repo, taskID string) error {