From a725bc668b6c834cbea5495251a1f8b850bb6549 Mon Sep 17 00:00:00 2001 From: Ian Gulliver Date: Mon, 19 Oct 2026 08:39:45 -0700 Subject: [PATCH] cancel review --- docs/operations.md | 2 +- internal/agents/review.go | 26 ++++++++++++++++++++++---- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/docs/operations.md b/docs/operations.md index 5918cb4..5237792 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -415,7 +415,7 @@ Every new snapshot of a task gets a review: a separate agent session on the vend - **Starting.** After `takeSnapshot` creates a snapshot, `afterSnapshot` runs `startReviewer`: it picks the model from the task creator's `users.review_model` and `users.second_review_model` (`reviewerModel`): the second review model when the task's latest implementer session runs exactly the review model, the review model otherwise, and the vendor follows from the model. They are returned as `review_model` and `second_review_model` on `GET /api/me`, set together with `PUT /api/me/review-models` from the Settings tab's Review models menus, and record `user.updated`. A model whose vendor's CLI isn't on the service's `PATH` fails the review with the start error. It then checks the snapshot's commit out detached at `worktrees//review-`, 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//review--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. +- **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//review--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. Reviews are of all the task's work, so a newer one makes older ones moot: once a snapshot's review starts, or is skipped for having no diff, byre fails the task's running reviews of earlier snapshots ("superseded by snapshot N") and ends their reviewers (`failRunningReviews`). Each snapshot's review starts on its own goroutine under `reviewMu`, so they can run out of order; `afterSnapshot` starts no review for a snapshot when the task already has a review of a later one. 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. - **Events.** `review.started`, `review.finished` (state `done`, `failed` or `skipped`; a skipped review emits only `review.finished`) and `review.finding` (a finding's state set, with the user as actor) carry the whole review, with the task's ids and no `agent_session_id`. A reviewer's own events carry its session id and the task's ids: the transcript skips every event of a session whose `session.created` has `role` `reviewer`, and draws each `review.finished` as a note with its outcome; the lead's `read_transcript` leaves the reviewer's events out and shows each `review.finished` with its summary and findings (`reviewLine`). `GET /api/tasks/{taskId}/sessions` and everything that looks for the task's agent (`ListSessions`) see only implementer sessions. - **UI.** The **Reviews** tab (`mac/Sources/Byre/Reviews.swift`) shows one review at a time, picked by snapshot (see "Snapshots" for the picker): its snapshot, state or severity counts, error, summary, screenshots and findings, and, while it runs, a **Stop** button (`POST /api/reviews/{reviewId}/stop`). The `change` field is not shown; it goes to the lead and into notes. A finding with `path` and `line` shows, under its title, the lines `line` to `end_line` plus 3 either side from that file's diff for the snapshot against the default branch (`changes?snapshot=`, the diff the reviewer was given), with new-file line numbers and the finding's own lines marked; it shows nothing when those lines aren't in the diff, and "diff not available" with the error once the task is closed. Clicking a screenshot opens it full size. Each open finding has **fix** and **ignore** buttons (also on review cards in the inbox). **fix** sends the finding's severity, title, location and detail to the task's agent as a message from the user: through `POST /api/sessions/{id}/messages` to its latest live session (queued like any message while the agent is mid-turn, see "Agent sessions"), or starting a session when it has none; the view stays where it is, so several findings can be sent in a row. Then it, or **ignore** on its own, records the finding's state (`fixed` or `ignored`) with `PUT /api/reviews/{reviewId}/findings/{index}`, the index being the finding's position in `result.findings`; the review's `finding_states` (the `reviews.finding_states` column, a JSON object from index to state) holds them, and the buttons give way to "fix sent" or "ignored". Only a `done` review's findings take a state, and there is no way to clear one. The tab loads when it opens, when the task changes, and on each `review.*` event in the task's stream. The tab follows the latest `review.*` event of the highest-numbered snapshot: while that review runs, the tab label (and its entry in the More menu) carries no badge, hiding any earlier review's count; once it finishes with open findings (those with no state), the tab (or the More menu holding it) is highlighted with a badge of their count. Each snapshot row's **review** button shows that snapshot's latest `review.*` event: a fill in the accent tint (`Palette.accentTint`, the same in every task) while the review runs, and otherwise a mark over its top-right corner: a grey ● when it was skipped, a green ✓ when it finished with no open findings, and, when it has open findings, the button highlighted with their count in a badge. A failed review leaves the button unmarked. While any of a task's reviews is `running` and its agent is idle, the task's state is `reviewing` (see "Task state in the store"), which the task switcher and the Project tab show as **Reviewing** (see "Task state"). `GET /api/tasks/{taskId}/reviews` lists a task's reviews oldest first. `sqlite3 byre.db "SELECT id, task_id, snapshot_n, vendor, state, error FROM reviews ORDER BY created_at DESC LIMIT 20"` shows recent ones. diff --git a/internal/agents/review.go b/internal/agents/review.go index 1870b1c..63be255 100644 --- a/internal/agents/review.go +++ b/internal/agents/review.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "math" "net/http" "os" "path/filepath" @@ -92,6 +93,17 @@ func (m *Manager) afterSnapshot(snap *api.Snapshot) { if task.Status != api.TaskStatusActive { return } + reviews, err := m.store.ListReviews(ctx, task.Id) + if err != nil { + slog.Error("list reviews to review", "task", task.Id, "err", err) + return + } + for _, r := range reviews { + if r.SnapshotN > snap.N { + slog.Info("review not started, a newer snapshot has one", "task", task.Id, "snapshot", snap.N, "newer", r.SnapshotN) + return + } + } err = m.startReviewer(ctx, task, *snap) if err != nil { slog.Error("start review", "task", task.Id, "snapshot", snap.N, "err", err) @@ -157,6 +169,7 @@ func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Sna return err } slog.Info("review skipped, snapshot has no diff", "task", task.Id, "snapshot", snap.N) + m.failRunningReviews(ctx, task.Id, project, snap.N, fmt.Sprintf("superseded by snapshot %d", snap.N)) return nil } @@ -211,6 +224,7 @@ func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Sna return err } slog.Info("review started", "review", reviewID, "task", task.Id, "snapshot", snap.N, "vendor", vendor, "model", model) + m.failRunningReviews(ctx, task.Id, project, snap.N, fmt.Sprintf("superseded by snapshot %d", snap.N)) return nil } @@ -496,17 +510,21 @@ func (m *Manager) failReview(ctx context.Context, reviewID, reason string) { func (m *Manager) cancelReviews(ctx context.Context, task api.Task, project api.Project) { m.reviewMu.Lock() defer m.reviewMu.Unlock() - reviews, err := m.store.ListReviews(ctx, task.Id) + m.failRunningReviews(ctx, task.Id, project, math.MaxInt, "the task was closed") +} + +func (m *Manager) failRunningReviews(ctx context.Context, taskID string, project api.Project, before int, reason string) { + reviews, err := m.store.ListReviews(ctx, taskID) if err != nil { - slog.Error("list reviews to cancel", "task", task.Id, "err", err) + slog.Error("list reviews to cancel", "task", taskID, "err", err) return } for _, r := range reviews { - if r.State != api.ReviewStateRunning { + if r.State != api.ReviewStateRunning || r.SnapshotN >= before { continue } m.endReviewer(ctx, r.AgentSessionId, project, r.Id) - m.failReview(ctx, r.Id, "the task was closed") + m.failReview(ctx, r.Id, reason) } }