Review failed

docs/operations.md

@@ -238,7 +238,7 @@ The fork's session guidance gets one more line (`forkGuidance` in `internal/agen
 
 ## Reviews
 
-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, and a snapshot whose change (the diff below) has no files, such as a task's first snapshot taken before its agent changed anything, runs no reviewer: it gets a `reviews` row in state `skipped`, with no session, vendor or model. Nothing else starts a review, and nothing waits for one: reviews ride along for the human to look at when they want.
+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, and a snapshot whose change (the diff below) has no files, such as a task's first snapshot taken before its agent changed anything, 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, is interrupted, or calls `stop_merge` is reviewed as usual. 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`) 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`): the change as `git diff` from the previous snapshot (the snapshot commit's parent for the first) with its changed files, the merge-base diff of all the task's work for context, and the task's first message to its agent.
 - **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.

internal/agents/manager.go

@@ -762,7 +762,14 @@ func (m *Manager) turnEnded(ctx context.Context, l *live, res endResult) error {
 	if err != nil {
 		return err
 	}
-	m.snapshotSoon(l, api.SnapshotTriggerTurnEnd, 0)
+	mergingBy := ""
+	if finished && !handoff {
+		mergingBy, err = m.store.MergingBy(ctx, l.taskID)
+		if err != nil {
+			return err
+		}
+	}
+	m.snapshotSoon(l, api.SnapshotTriggerTurnEnd, 0, mergingBy == "")
 
 	if res.window > 0 {
 		err = m.updateContext(ctx, l, l.contextUsed, res.window)

internal/agents/snapshots.go

@@ -18,6 +18,7 @@ import (
 type snapRequest struct {
 	trigger api.SnapshotTrigger
 	seq     int64
+	review  bool
 }
 
 type snapState struct {
@@ -36,7 +37,7 @@ func (m *Manager) snapStateLocked(taskID string) *snapState {
 	return st
 }
 
-func (m *Manager) snapshotSoon(l *live, trigger api.SnapshotTrigger, seq int64) {
+func (m *Manager) snapshotSoon(l *live, trigger api.SnapshotTrigger, seq int64, review bool) {
 	m.snapMu.Lock()
 	defer m.snapMu.Unlock()
 
@@ -44,7 +45,7 @@ func (m *Manager) snapshotSoon(l *live, trigger api.SnapshotTrigger, seq int64)
 	if st.closed {
 		return
 	}
-	req := &snapRequest{trigger: trigger, seq: seq}
+	req := &snapRequest{trigger: trigger, seq: seq, review: review}
 	if st.running {
 		st.pending = req
 		return
@@ -64,8 +65,10 @@ func (m *Manager) runSnapshots(l *live, st *snapState, req *snapRequest) {
 			if err != nil {
 				slog.Error("take snapshot", "task", l.taskID, "trigger", req.trigger, "err", err)
 			}
+			if req.review {
 				go m.afterSnapshot(snap)
 			}
+		}
 		st.mu.Unlock()
 
 		m.snapMu.Lock()

internal/store/store.go

@@ -686,6 +686,15 @@ func (s *Store) SetMergingBy(ctx context.Context, id, login string) error {
 	})
 }
 
+func (s *Store) MergingBy(ctx context.Context, id string) (string, error) {
+	login := ""
+	err := s.db.QueryRowContext(ctx, `SELECT merging_by FROM tasks WHERE id = ?`, id).Scan(&login)
+	if errors.Is(err, sql.ErrNoRows) {
+		return "", ErrNotFound
+	}
+	return login, err
+}
+
 func (s *Store) TakeMergingBy(ctx context.Context, id string) (string, error) {
 	login := ""
 	err := s.write(ctx, func(tx *sql.Tx, emit emitFunc) error {