Share review stage eligibility between advancement and completion

docs/operations.md

@@ -475,7 +475,7 @@ An implementer on Claude Code forks its own task with the `fork` MCP tool (`fork
 Every new snapshot of a task is reviewed by its project's review workflow: each stage of it is a separate reviewer agent session that reads the change and reports on it (`internal/agents/review.go`, `internal/store/review.go`). A project with no review workflow isn't reviewed. 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.
 
 - **Review workflows.** A project's review workflow is any saved workflow (see "Workflows") whose stages are single agents, named by `projects.review_workflow` (`review_workflow` on the Project, empty for none). It is set with `PUT /api/projects/{projectId}/review-workflow` (`{workflow}`; empty turns reviews off, a missing workflow or one with a team stage is refused, `reviewProblem` in `internal/agents/workflows.go`), from the Project tab's reviews menu or the lead's `set_review_workflow`, and records `project.updated` with the project. Each stage of the workflow is one reviewer on the stage's `model` (fixed: it doesn't change with the model the task's agent runs), told the stage's `prompt`; a stage that comes after others starts once all of them are `done` and gets their summaries and findings under `## From reviewer "<stage>"`. A project's snapshots are reviewed with the workflow as it is when each review stage starts: each `reviews` row names the definition it ran from (`reviews.definition_id`, see "Storage" under "Workflows"), and later stages of that snapshot follow it.
-- **Starting.** After `takeSnapshot` creates a snapshot, `afterSnapshot` runs `startReviews`: with no review workflow it does nothing; a snapshot with no changes against the default branch gets one `skipped` row; a review workflow that is missing or has a team stage gets one `failed` row naming it; otherwise it runs `startReviewer` for each stage the workflow starts with (`workflow.Starts`). When a review finishes `done`, `advanceReviews` starts each stage after it whose earlier stages are all `done` for that snapshot, unless the task closed or a later snapshot has reviews; a failed stage stops the stages after it. A stage that can't start (a model whose vendor's CLI isn't on the service's `PATH`, a checkout that fails) gets a `failed` row with the start error, so a snapshot's reviews never look complete with a stage missing; a failed row with no session shows in the transcript as an error note. `startReviewer` 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 stage per snapshot, its `stage` the stage's name), and sends `reviewPrompt` as its first turn (actor `byre`): the stage's prompt, all of the task's work as `git diff` from the merge-base of the snapshot and the default branch, with its changed files, the task's first message to its agent, and earlier stages' reviews. Every review covers the whole task against the default branch, not only what changed since the previous snapshot.
+- **Starting.** After `takeSnapshot` creates a snapshot, `afterSnapshot` runs `startReviews`: with no review workflow it does nothing; a snapshot with no changes against the default branch gets one `skipped` row; a review workflow that is missing or has a team stage gets one `failed` row naming it; otherwise it runs `startReviewer` for each stage the workflow starts with (`workflow.Starts`). When a review finishes `done`, `advanceReviews` starts each stage after it whose earlier stages are all `done` for that snapshot, unless the task closed or a later snapshot has reviews; a failed stage stops the stages after it. Both advancement and the completion check use `pendingReviewStages` to find successors that have not started and whose inputs are all `done`; it returns the input reviews together with each stage. A stage that can't start (a model whose vendor's CLI isn't on the service's `PATH`, a checkout that fails) gets a `failed` row with the start error, so a snapshot's reviews never look complete with a stage missing; a failed row with no session shows in the transcript as an error note. `startReviewer` 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 stage per snapshot, its `stage` the stage's name), and sends `reviewPrompt` as its first turn (actor `byre`): the stage's prompt, all of the task's work as `git diff` from the merge-base of the snapshot and the default branch, with its changed files, the task's first message to its agent, and earlier stages' reviews. Every review covers the whole task against the default branch, not only what changed since the previous snapshot.
 - **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.
 - **Delivery.** A snapshot's review is delivered once its workflow is done: every stage has a finished row and no stage is left to start (`reviewsSettled`), so the agent never starts fixing while a reviewer is still at work, which would supersede it. `reviewFinished` then sends every stage's findings to the task's live agent in one `byre` message (`deliverFindings` and `findingsMessage` in `internal/agents/review.go`): under a `## Reviewer "<stage>"` heading per stage with findings, each finding's number, severity, title, location and detail, after a preamble asking the agent to check each one against the code and its brief and, before changing anything, answer each with `respond_to_finding` (see "Agent responses"). Only the finish that settles the review sends it: of the snapshot's reviews, the one with the latest `finished_at` (then the highest id), so two stages finishing at once send one message; the startup passes that rerun `reviewFinished` send nothing. Nothing is sent when no stage found anything, or when a newer snapshot has been taken. 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 delivered`. Forwarding doesn't set any finding's state; **fix** and **ignore** stay available until the agent or the user answers.
 - **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` ends the reviewer the same way and fails the review; `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; then every `done` review of an active task's latest reviewed snapshot goes through `reviewFinished` again (`LatestDoneStages`), so a stage whose successors a restart cut off before they started still starts them.