From 92edbcfdcc32909e8797ca2046986da713ec9a4d Mon Sep 17 00:00:00 2001 From: Ian Gulliver Date: Fri, 23 Oct 2026 16:08:54 -0700 Subject: [PATCH] Fix problems found in QA pass of build 1792795001 --- docs/operations.md | 2 +- internal/agents/lead.go | 3 ++ internal/agents/manager.go | 15 ++++++--- internal/agents/review.go | 27 +++++++++++++--- internal/agents/snapshots.go | 4 +-- internal/procgroup/procgroup.go | 39 +++++++++++++++++++++++- mac/Sources/Byre/Chat.swift | 9 ++++++ mac/Sources/Byre/TranscriptBuilder.swift | 3 ++ 8 files changed, 90 insertions(+), 12 deletions(-) diff --git a/docs/operations.md b/docs/operations.md index d84dc54..ef725c1 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -88,7 +88,7 @@ Vendor setup and protocol quirks are in [Claude Code](claude-protocol.md), [Code - A turn whose final message asks the human something in prose is bounced once, telling the agent to use `ask_human`. For Claude the bounce is a Stop hook that blocks the stop; Claude Code reports the block as a "Stop hook error" notification, which is expected. An agent told explicitly to ask in plain text will refuse the bounce. - A turn that asks a question has no summary; everything the human needs belongs in the question's context. Other turns use their final message as the summary; don't ask agents to write a second recap. -- A review covers the whole task against the default branch, so a newer one makes older ones moot. Findings reach the task's agent together, once the whole review workflow finishes; an individual reviewer finishing is not a completed review. +- A review covers the whole task against the default branch (a subtask against its parent's branch, which it merges into), so a newer one makes older ones moot. Findings reach the task's agent together, once the whole review workflow finishes; an individual reviewer finishing is not a completed review. - An audit checks only the newest commit of the default branch, one at a time per project, so it doesn't run on every commit but the newest one is always audited eventually. byre never fetches for an audit, so a commit pushed outside byre is audited only once a byre merge has fetched it. - A team task that comes up with some members failing to start isn't repaired; discard it and create it again. diff --git a/internal/agents/lead.go b/internal/agents/lead.go index 7d6cb7d..4466d15 100644 --- a/internal/agents/lead.go +++ b/internal/agents/lead.go @@ -310,6 +310,9 @@ func (m *Manager) leadTurnEnded(ctx context.Context, l *live, res endResult) err if handoff { payload["handoff"] = true } + if interrupted { + payload["interrupted"] = true + } err := m.emit(ctx, l, agentActor, "turn.ended", payload) if err != nil { return err diff --git a/internal/agents/manager.go b/internal/agents/manager.go index 0a561cd..93897f8 100644 --- a/internal/agents/manager.go +++ b/internal/agents/manager.go @@ -1003,11 +1003,15 @@ func (m *Manager) pump(l *live) { m.mu.Lock() delete(m.live, l.id) closing := m.closing - quiet := closing || l.stopping + stopping := l.stopping interrupted := l.interrupted m.mu.Unlock() - orphaned := len(l.subagents) > 0 && !closing + if closing { + return + } + + orphaned := len(l.subagents) > 0 if orphaned { err := m.store.SetSubagents(ctx, l.id, 0, "byre") if err != nil { @@ -1015,8 +1019,8 @@ func (m *Manager) pump(l *live) { } } - if quiet { - m.logUndelivered(ctx, l.id, "agent process ended while closing or stopping") + if stopping { + m.logUndelivered(ctx, l.id, "agent process ended while stopping") return } @@ -1259,6 +1263,9 @@ func (m *Manager) turnEnded(ctx context.Context, l *live, res endResult) error { if l.member != "" { payload["member"] = l.member } + if interrupted { + payload["interrupted"] = true + } owner := m.ownsTask(l.id) if owner { defer m.endMergeTurn(l.id, false) diff --git a/internal/agents/review.go b/internal/agents/review.go index 9f2ae92..a91fa72 100644 --- a/internal/agents/review.go +++ b/internal/agents/review.go @@ -119,9 +119,13 @@ func (m *Manager) beginReviews(ctx context.Context, ref store.SnapshotRef) error if project.ReviewWorkflow == "" { return finished(api.ReviewStateFailed, "", fmt.Sprintf("project %s has no review workflow", project.Name)) } - changes, err := git.Compare(ctx, project.RepoPath, project.DefaultBranch, snap.CommitSha, snap.TreeSha, "") + base, err := m.reviewBase(ctx, task, project) if err != nil { - return finished(api.ReviewStateFailed, project.ReviewWorkflow, fmt.Sprintf("byre couldn't compare snapshot #%d with %s: %v", snap.N, project.DefaultBranch, err)) + return err + } + changes, err := git.Compare(ctx, project.RepoPath, base, snap.CommitSha, snap.TreeSha, "") + if err != nil { + return finished(api.ReviewStateFailed, project.ReviewWorkflow, fmt.Sprintf("byre couldn't compare snapshot #%d with %s: %v", snap.N, base, err)) } if len(changes.Files) == 0 { slog.Info("review skipped, snapshot has no diff", "task", task.Id, "snapshot", snap.N) @@ -232,7 +236,11 @@ func (m *Manager) launchReviewer(ctx context.Context, task api.Task, project api } func (m *Manager) reviewPrompt(ctx context.Context, task api.Task, project api.Project, snap api.Snapshot, def api.WorkflowDefinition, stage string, inputs []api.Review) (string, error) { - changes, err := git.Compare(ctx, project.RepoPath, project.DefaultBranch, snap.CommitSha, snap.TreeSha, "") + base, err := m.reviewBase(ctx, task, project) + if err != nil { + return "", err + } + changes, err := git.Compare(ctx, project.RepoPath, base, snap.CommitSha, snap.TreeSha, "") if err != nil { return "", err } @@ -244,7 +252,7 @@ func (m *Manager) reviewPrompt(ctx context.Context, task api.Task, project api.P b := &strings.Builder{} fmt.Fprintf(b, "Review the work of the agent on the byre task %q, as reviewer %q of the project's review workflow %q.\n\n%s\n\n", task.Title, stage, project.ReviewWorkflow, strings.TrimSpace(workflow.Get(def, stage).Prompt)) fmt.Fprintf(b, "It is snapshot #%d of the task, commit %s, checked out in your working directory. The change is all of the task's work, `git diff %s %s` (from the merge-base with %s); %d files changed:\n", - snap.N, snap.CommitSha, changes.Base, snap.CommitSha, project.DefaultBranch, len(changes.Files)) + snap.N, snap.CommitSha, changes.Base, snap.CommitSha, base, len(changes.Files)) for i, f := range changes.Files { if i == maxReviewFiles { fmt.Fprintf(b, "- … and %d more\n", len(changes.Files)-maxReviewFiles) @@ -262,6 +270,17 @@ func (m *Manager) reviewPrompt(ctx context.Context, task api.Task, project api.P return b.String(), nil } +func (m *Manager) reviewBase(ctx context.Context, task api.Task, project api.Project) (string, error) { + if task.ParentTaskId == nil || task.WorkflowStage != nil { + return project.DefaultBranch, nil + } + parent, err := m.store.GetTask(ctx, *task.ParentTaskId) + if err != nil { + return "", err + } + return parent.Branch, nil +} + func quote(s string) string { lines := strings.Split(strings.TrimSpace(s), "\n") for i, l := range lines { diff --git a/internal/agents/snapshots.go b/internal/agents/snapshots.go index 7f529fd..a8d28a6 100644 --- a/internal/agents/snapshots.go +++ b/internal/agents/snapshots.go @@ -269,12 +269,12 @@ func (m *Manager) takeSnapshotTool(l *live) mcp.Tool { if err != nil { return "", err } - return "Worktree committed to the task's branch, and snapshot requested. If the project has a review workflow, its reviewers check it against the default branch; their findings come to you together once the workflow finishes.", nil + return "Worktree committed to the task's branch, and snapshot requested. If the project has a review workflow, its reviewers check it against the default branch (a subtask's against its parent's branch); their findings come to you together once the workflow finishes.", nil }, } } -const takeSnapshotDescription = "Commit and snapshot a task's worktree now: byre commits the whole worktree, uncommitted work included, to the task's branch, and returns once that commit is made. It then snapshots that commit on byre's own ref for the task (refs/byre/tasks//snap/). If the project has a review workflow, its reviewers check all of the task's work against the default branch, using each stage's model and instructions; findings arrive together once the workflow finishes. " + +const takeSnapshotDescription = "Commit and snapshot a task's worktree now: byre commits the whole worktree, uncommitted work included, to the task's branch, and returns once that commit is made. It then snapshots that commit on byre's own ref for the task (refs/byre/tasks//snap/). If the project has a review workflow, its reviewers check all of the task's work against the default branch (a subtask's against its parent's branch), using each stage's model and instructions; findings arrive together once the workflow finishes. " + "byre already does this at the end of every turn that changes files; call this to review work mid-turn, or to retry a review that failed (it snapshots even when nothing changed since the last snapshot)." func (m *Manager) dropSnapshots(repo, taskID string) error { diff --git a/internal/procgroup/procgroup.go b/internal/procgroup/procgroup.go index ba6248b..dfb5a89 100644 --- a/internal/procgroup/procgroup.go +++ b/internal/procgroup/procgroup.go @@ -3,6 +3,8 @@ package procgroup import ( "context" "os/exec" + "strconv" + "strings" "syscall" ) @@ -25,6 +27,41 @@ func group(cmd *exec.Cmd) *Cmd { return &Cmd{Cmd: cmd} } +// Kill also kills descendants that left the group: agents run their commands in sessions of their own. func (cmd *Cmd) Kill() error { - return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + pids := descendants(cmd.Process.Pid) + err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + for _, pid := range pids { + syscall.Kill(pid, syscall.SIGKILL) + } + return err +} + +func descendants(root int) []int { + out, err := exec.Command("ps", "-A", "-o", "pid=,ppid=").Output() + if err != nil { + return nil + } + children := map[int][]int{} + for _, line := range strings.Split(string(out), "\n") { + f := strings.Fields(line) + if len(f) != 2 { + continue + } + pid, err1 := strconv.Atoi(f[0]) + ppid, err2 := strconv.Atoi(f[1]) + if err1 != nil || err2 != nil { + continue + } + children[ppid] = append(children[ppid], pid) + } + pids := []int{} + queue := children[root] + for len(queue) > 0 { + pid := queue[0] + queue = queue[1:] + pids = append(pids, pid) + queue = append(queue, children[pid]...) + } + return pids } diff --git a/mac/Sources/Byre/Chat.swift b/mac/Sources/Byre/Chat.swift index 145dbb5..1c07b40 100644 --- a/mac/Sources/Byre/Chat.swift +++ b/mac/Sources/Byre/Chat.swift @@ -56,6 +56,7 @@ final class ChatModel { @ObservationIgnored private var members: [String: String] = [:] private(set) var scrollRequest: (seq: Int, id: Int?, anchor: UnitPoint)? private(set) var fetching = false + private(set) var stopping = false private(set) var older: [ByreEvent]? @ObservationIgnored var stuck = true @ObservationIgnored var prepended = false @@ -396,6 +397,9 @@ final class ChatModel { if let q = groups.first(where: { $0.id == "questions" }) { return flagged(q) } + if task.state == .running && stopping { + return ChatStatus(label: "Stopping…", help: "Waiting for the agent to stop its turn\(end)", tone: .working, group: nil) + } if let busy = task.state == .running ? store.taskState(task) : store.progress(task) { return ChatStatus(label: busy.label, help: "\(busy.title)\(end)", tone: .working, group: nil) } @@ -555,7 +559,11 @@ final class ChatModel { } func interrupt(_ sessionId: String, _ overlays: Overlays) { + stopping = true Task { + defer { + stopping = false + } do { _ = try await store.api.client.interruptSession(path: .init(sessionId: sessionId)).accepted } catch { @@ -1895,6 +1903,7 @@ private struct SnapshotRow: View { HStack(spacing: 8) { Rectangle().fill(Palette.border).frame(height: 1) Text("snapshot #\(snap.n)") + .fixedSize() if !inherited { Button("Changes") { store.pinSnapshot(snap.n) diff --git a/mac/Sources/Byre/TranscriptBuilder.swift b/mac/Sources/Byre/TranscriptBuilder.swift index 9d9414a..637e160 100644 --- a/mac/Sources/Byre/TranscriptBuilder.swift +++ b/mac/Sources/Byre/TranscriptBuilder.swift @@ -910,6 +910,9 @@ final class TranscriptBuilder { if bool(p, "handoff") { return note([.text("handoff written; continuing in a new session with a fresh context")]) } + if bool(p, "interrupted") { + return note([.text("turn stopped")]) + } return Row(.turnEnd(error: bool(p, "is_error") ? "turn ended: \(jsString(p["subtype"]).replacingOccurrences(of: "_", with: " "))" : nil), taskId: rowTask) case "session.created":