don't review on interrupt

This commit is contained in:
Ian Gulliver
2026-10-18 15:23:42 -07:00
parent 6b86a7d5a9
commit 923d0a2c00
2 changed files with 3 additions and 3 deletions
+2 -2
View File
@@ -379,7 +379,7 @@ The **Terminal** tab (`mac/Sources/Byre/Terminal.swift`) draws it with SwiftTerm
byre snapshots each task's worktree at the end of every turn that has no subagents running (`internal/agents/snapshots.go`, `internal/git/snapshot.go`), so a snapshot always sits on a turn boundary, the only place a Codex conversation can be forked. A snapshot builds the tree the same way as the Changes view (a copy of the real index, `git add --all`, `git write-tree`), commits it with `git commit-tree` whose parent is the worktree's `HEAD`, authored as the session's creator with the message `byre snapshot <n> (<trigger>)`, and points `refs/byre/tasks/<task-id>/snap/<n>` at it (`git update-ref` with an empty old value, so an existing ref is never overwritten). A row in the `snapshots` table records id, task, session, `n` (from 1 per task), commit and tree shas, `event_seq` (the task's latest event when the snapshot started, at or after the turn's `turn.ended`, so the tree holds at least the work up to that event), the trigger and the time, and the same transaction records a `snapshot.created` event carrying the row. byre snapshots each task's worktree at the end of every turn that has no subagents running (`internal/agents/snapshots.go`, `internal/git/snapshot.go`), so a snapshot always sits on a turn boundary, the only place a Codex conversation can be forked. A snapshot builds the tree the same way as the Changes view (a copy of the real index, `git add --all`, `git write-tree`), commits it with `git commit-tree` whose parent is the worktree's `HEAD`, authored as the session's creator with the message `byre snapshot <n> (<trigger>)`, and points `refs/byre/tasks/<task-id>/snap/<n>` at it (`git update-ref` with an empty old value, so an existing ref is never overwritten). A row in the `snapshots` table records id, task, session, `n` (from 1 per task), commit and tree shas, `event_seq` (the task's latest event when the snapshot started, at or after the turn's `turn.ended`, so the tree holds at least the work up to that event), the trigger and the time, and the same transaction records a `snapshot.created` event carrying the row.
The trigger is `turn_end`: the snapshot starts after byre's end-of-turn commit and the `turn.ended` event. A turn that ends while the session's subagent count (see "Subagent count") is above zero takes no snapshot and requests no review, since its work is half done; the CLI starts another turn when each background subagent finishes, and the first of those to end with none running takes the snapshot and its review. The trigger `answer` is a snapshot taken just before answers from the human are handed to the agent (see "Inbox"); it is taken even when its tree is unchanged, is never reviewed (its transcript row has no review button), and doesn't wait in the queue below but runs at once under the task's mutex. Snapshots taken by earlier builds also carry `message` (mid-turn, when the agent wrote text after tool calls), `tool_call`, `ask_human` and `timer`. A snapshot whose tree equals the task's latest snapshot's tree is skipped, so a turn that changed no files leaves none. Snapshots run one at a time per task: a trigger that arrives while one is running replaces any already waiting one, and the waiting one runs when the current one finishes. Failures are logged and dropped. The trigger is `turn_end`: the snapshot starts after byre's end-of-turn commit and the `turn.ended` event. A turn that ends while the session's subagent count (see "Subagent count") is above zero takes no snapshot and requests no review, since its work is half done; the CLI starts another turn when each background subagent finishes, and the first of those to end with none running takes the snapshot and its review. An interrupted turn likewise takes no snapshot and requests no review, so the next turn to end uninterrupted snapshots its work and has it reviewed even when that turn changed no files. The trigger `answer` is a snapshot taken just before answers from the human are handed to the agent (see "Inbox"); it is taken even when its tree is unchanged, is never reviewed (its transcript row has no review button), and doesn't wait in the queue below but runs at once under the task's mutex. Snapshots taken by earlier builds also carry `message` (mid-turn, when the agent wrote text after tool calls), `tool_call`, `ask_human` and `timer`. A snapshot whose tree equals the task's latest snapshot's tree is skipped, so a turn that changed no files leaves none. Snapshots run one at a time per task: a trigger that arrives while one is running replaces any already waiting one, and the waiting one runs when the current one finishes. Failures are logged and dropped.
The `take_snapshot` MCP tool asks for a snapshot now (`RequestSnapshot` and `takeSnapshotTool` in `internal/agents/snapshots.go`): a task's agent calls it with no arguments for its own task, and the lead with a `task_id`. The snapshot has trigger `requested`, is taken even when its tree equals the latest snapshot's (so it retries a failed review of unchanged work), and is reviewed like any other (see "Reviews"); it counts as a review request (`BeginReviewRequest`) from the call until the reviewer starts. It is attributed to the task's latest implementer session, authored as that session's creator, and can be taken mid-turn, uncommitted work included. A requested snapshot that is replaced while waiting passes its trigger and its taken-even-when-unchanged to the one that replaces it. Refused for a closed task, while the task is merging or being discarded, and for a task with no agent session. The `take_snapshot` MCP tool asks for a snapshot now (`RequestSnapshot` and `takeSnapshotTool` in `internal/agents/snapshots.go`): a task's agent calls it with no arguments for its own task, and the lead with a `task_id`. The snapshot has trigger `requested`, is taken even when its tree equals the latest snapshot's (so it retries a failed review of unchanged work), and is reviewed like any other (see "Reviews"); it counts as a review request (`BeginReviewRequest`) from the call until the reviewer starts. It is attributed to the task's latest implementer session, authored as that session's creator, and can be taken mid-turn, uncommitted work included. A requested snapshot that is replaced while waiting passes its trigger and its taken-even-when-unchanged to the one that replaces it. Refused for a closed task, while the task is merging or being discarded, and for a task with no agent session.
@@ -401,7 +401,7 @@ The fork's session guidance gets one more line (`forkGuidance` in `internal/agen
## Reviews ## 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 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. 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. 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`) 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`) 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`.
- **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. - **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.
+1 -1
View File
@@ -1174,7 +1174,7 @@ func (m *Manager) turnEnded(ctx context.Context, l *live, res endResult) error {
} }
unlock() unlock()
held := len(l.subagents) > 0 held := len(l.subagents) > 0 || interrupted
turn := m.event(l, agentActor, "turn.ended", payload) turn := m.event(l, agentActor, "turn.ended", payload)
next, finalizeBy, err := m.store.EndTurn(ctx, l.id, agentActor, nil, &turn, review && !held, finished && !handoff) next, finalizeBy, err := m.store.EndTurn(ctx, l.id, agentActor, nil, &turn, review && !held, finished && !handoff)
if err != nil { if err != nil {