From 2401c6fc939e6eab7be39fef04289fc6d52ab8de Mon Sep 17 00:00:00 2001 From: Ian Gulliver Date: Thu, 15 Oct 2026 20:24:15 -0700 Subject: [PATCH] Review failed --- docs/operations.md | 2 +- internal/agents/manager.go | 9 ++++++++- internal/agents/snapshots.go | 9 ++++++--- internal/store/store.go | 9 +++++++++ 4 files changed, 24 insertions(+), 5 deletions(-) diff --git a/docs/operations.md b/docs/operations.md index 3625a3f..67c1401 100644 --- a/docs/operations.md +++ b/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//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`): 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. diff --git a/internal/agents/manager.go b/internal/agents/manager.go index 89866b8..75bbe66 100644 --- a/internal/agents/manager.go +++ b/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) diff --git a/internal/agents/snapshots.go b/internal/agents/snapshots.go index 2193cfe..049f46c 100644 --- a/internal/agents/snapshots.go +++ b/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,7 +65,9 @@ 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) } - go m.afterSnapshot(snap) + if req.review { + go m.afterSnapshot(snap) + } } st.mu.Unlock() diff --git a/internal/store/store.go b/internal/store/store.go index af16d1e..4c6bb45 100644 --- a/internal/store/store.go +++ b/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 {