Review every new snapshot with an agent from the other vendor
This commit is contained in:
Ian Gulliver
@@ -0,0 +1,389 @@
|
||||
package agents
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"g.fc.run/misc/byre/internal/api"
|
||||
"g.fc.run/misc/byre/internal/git"
|
||||
"g.fc.run/misc/byre/internal/mcp"
|
||||
"g.fc.run/misc/byre/internal/store"
|
||||
)
|
||||
|
||||
const maxReviewFiles = 200
|
||||
|
||||
var reviewModels = map[api.AgentSessionVendor]string{api.Claude: "claude-opus-5-5", api.Codex: "gpt-6.1-sol"}
|
||||
|
||||
var severities = []string{"critical", "high", "medium", "low", "info"}
|
||||
|
||||
const reviewGuidance = `You are a reviewer running inside byre. Another agent is doing a task; you review the change it just made, and a human may read your review in byre's web UI.
|
||||
- Your working directory is a detached checkout of the snapshot under review. Read, build and run whatever you need there, but don't edit, commit or push: the checkout is discarded when you finish, and the task's own worktree belongs to its agent.
|
||||
- Report with byre's submit_review tool, exactly once, then end your turn with one line. A review that isn't submitted is lost.
|
||||
- Each finding says what is wrong, where (path and line), why it matters and what to do about it. Leave out praise and nits. No findings is a fine result.`
|
||||
|
||||
func reviewSchema() json.RawMessage {
|
||||
b, err := json.Marshal(map[string]any{
|
||||
"type": "object",
|
||||
"properties": map[string]any{
|
||||
"summary": map[string]any{"type": "string", "description": "A few sentences on the change and what matters most, in Markdown."},
|
||||
"findings": map[string]any{
|
||||
"type": "array",
|
||||
"items": map[string]any{
|
||||
"type": "object",
|
||||
"properties": map[string]any{
|
||||
"severity": map[string]any{"type": "string", "enum": severities},
|
||||
"category": map[string]any{"type": "string", "description": "A short label, such as correctness, security or tests."},
|
||||
"title": map[string]any{"type": "string", "description": "One line."},
|
||||
"detail": map[string]any{"type": "string", "description": "What is wrong, why it matters and what to do, in Markdown."},
|
||||
"path": map[string]any{"type": "string", "description": "Repo-relative path of the file, if the finding is about one."},
|
||||
"line": map[string]any{"type": "integer"},
|
||||
},
|
||||
"required": []any{"severity", "title", "detail"},
|
||||
},
|
||||
},
|
||||
},
|
||||
"required": []any{"summary", "findings"},
|
||||
})
|
||||
if err != nil {
|
||||
panic(err)
|
||||
}
|
||||
return b
|
||||
}
|
||||
|
||||
func (m *Manager) afterSnapshot(snap *api.Snapshot) {
|
||||
if snap == nil {
|
||||
return
|
||||
}
|
||||
ctx := context.Background()
|
||||
m.reviewMu.Lock()
|
||||
defer m.reviewMu.Unlock()
|
||||
|
||||
task, err := m.store.GetTask(ctx, snap.TaskId)
|
||||
if err != nil {
|
||||
slog.Error("load task to review", "task", snap.TaskId, "err", err)
|
||||
return
|
||||
}
|
||||
if task.Status != api.TaskStatusActive {
|
||||
return
|
||||
}
|
||||
err = m.startReviewer(ctx, task, *snap)
|
||||
if err != nil {
|
||||
slog.Error("start review", "task", task.Id, "snapshot", snap.N, "err", err)
|
||||
}
|
||||
}
|
||||
|
||||
func (m *Manager) reviewDir(projectID, reviewID string) string {
|
||||
return filepath.Join(m.worktreeRoot, projectID, "review-"+reviewID)
|
||||
}
|
||||
|
||||
func (m *Manager) implementerVendor(ctx context.Context, taskID string) api.AgentSessionVendor {
|
||||
sessions, err := m.store.ListSessions(ctx, taskID)
|
||||
if err != nil || len(sessions) == 0 {
|
||||
return api.Claude
|
||||
}
|
||||
return sessions[len(sessions)-1].Vendor
|
||||
}
|
||||
|
||||
func reviewVendor(implementer api.AgentSessionVendor) api.AgentSessionVendor {
|
||||
if implementer == api.Codex {
|
||||
return api.Claude
|
||||
}
|
||||
return api.Codex
|
||||
}
|
||||
|
||||
func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Snapshot) error {
|
||||
project, err := m.store.GetProject(ctx, task.ProjectId)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
vendor := reviewVendor(m.implementerVendor(ctx, task.Id))
|
||||
model := reviewModels[vendor]
|
||||
prompt, err := m.reviewPrompt(ctx, task, project, snap)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
reviewID := store.NewID()
|
||||
dir := m.reviewDir(project.Id, reviewID)
|
||||
err = git.AddDetachedWorktree(project.RepoPath, dir, snap.CommitSha)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
id := newUUID()
|
||||
sess := api.AgentSession{
|
||||
Id: id,
|
||||
TaskId: task.Id,
|
||||
Vendor: vendor,
|
||||
Model: model,
|
||||
VendorSessionId: initialVendorSessionID(vendor, id),
|
||||
State: api.AgentSessionStateIdle,
|
||||
Role: api.Reviewer,
|
||||
CreatedBy: task.CreatedBy,
|
||||
CreatedAt: time.Now().UTC(),
|
||||
}
|
||||
err = m.store.CreateSession(ctx, sess, project.Id)
|
||||
if err != nil {
|
||||
m.removeReviewDir(project, reviewID)
|
||||
return err
|
||||
}
|
||||
err = m.store.CreateReview(ctx, api.Review{
|
||||
Id: reviewID,
|
||||
TaskId: task.Id,
|
||||
SnapshotN: snap.N,
|
||||
AgentSessionId: sess.Id,
|
||||
Vendor: string(vendor),
|
||||
Model: model,
|
||||
State: api.ReviewStateRunning,
|
||||
CreatedAt: time.Now().UTC(),
|
||||
})
|
||||
if err != nil {
|
||||
m.endReviewer(ctx, sess.Id, project, reviewID)
|
||||
return err
|
||||
}
|
||||
|
||||
_, err = m.turn(ctx, sess, task, "byre", prompt, nil)
|
||||
if err != nil {
|
||||
m.endReviewer(ctx, sess.Id, project, reviewID)
|
||||
m.failReview(ctx, reviewID, "the reviewer didn't start: "+err.Error())
|
||||
return err
|
||||
}
|
||||
slog.Info("review started", "review", reviewID, "task", task.Id, "snapshot", snap.N, "vendor", vendor, "model", model)
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *Manager) reviewPrompt(ctx context.Context, task api.Task, project api.Project, snap api.Snapshot) (string, error) {
|
||||
previous := ""
|
||||
if snap.N > 1 {
|
||||
prev, err := m.store.GetSnapshot(ctx, task.Id, snap.N-1)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
previous = prev.CommitSha
|
||||
} else {
|
||||
parent, err := git.Parent(project.RepoPath, snap.CommitSha)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
previous = parent
|
||||
}
|
||||
changes, err := git.Compare(project.RepoPath, project.DefaultBranch, snap.CommitSha, snap.TreeSha, previous)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
whole, err := git.Compare(project.RepoPath, project.DefaultBranch, snap.CommitSha, snap.TreeSha, "")
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
brief, err := m.store.FirstTaskMessage(ctx, task.Id)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
b := &strings.Builder{}
|
||||
fmt.Fprintf(b, "Review the change the agent working on the byre task %q just made: look for bugs, security problems, missing or wrong tests, and anything that doesn't do what the task asked.\n\n", task.Title)
|
||||
fmt.Fprintf(b, "It is snapshot %d of the task, commit %s, checked out in your working directory. The change is `git diff %s %s`; %d files changed:\n",
|
||||
snap.N, snap.CommitSha, previous, snap.CommitSha, len(changes.Files))
|
||||
for i, f := range changes.Files {
|
||||
if i == maxReviewFiles {
|
||||
fmt.Fprintf(b, "- … and %d more\n", len(changes.Files)-maxReviewFiles)
|
||||
break
|
||||
}
|
||||
fmt.Fprintf(b, "- %s %s (+%d −%d)\n", f.Status, f.Path, f.Additions, f.Deletions)
|
||||
}
|
||||
fmt.Fprintf(b, "\nFor context, all of the task's work so far is `git diff %s %s` (from the merge-base with %s, %d files).\n", whole.Base, snap.CommitSha, project.DefaultBranch, len(whole.Files))
|
||||
if brief != "" {
|
||||
fmt.Fprintf(b, "\nThe task's brief, the first message its agent got:\n\n%s\n", quote(clip(brief, 4000)))
|
||||
}
|
||||
b.WriteString("\nWhen you're done, call submit_review once with your review, then end your turn.")
|
||||
return b.String(), nil
|
||||
}
|
||||
|
||||
func quote(s string) string {
|
||||
lines := strings.Split(strings.TrimSpace(s), "\n")
|
||||
for i, l := range lines {
|
||||
lines[i] = "> " + l
|
||||
}
|
||||
return strings.Join(lines, "\n")
|
||||
}
|
||||
|
||||
func (m *Manager) submitReviewTool(l *live) mcp.Tool {
|
||||
return mcp.Tool{
|
||||
Name: "submit_review",
|
||||
Description: "Submit your review. Call it exactly once, when you're done; its arguments are the review the human reads.",
|
||||
InputSchema: reviewSchema(),
|
||||
Handler: func(ctx context.Context, args json.RawMessage) (string, error) {
|
||||
return m.submitReview(ctx, l, args)
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
func (m *Manager) submitReview(ctx context.Context, l *live, raw json.RawMessage) (string, error) {
|
||||
result := map[string]any{}
|
||||
err := json.Unmarshal(raw, &result)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("submit_review takes a JSON object: %w", err)
|
||||
}
|
||||
if _, ok := result["summary"].(string); !ok {
|
||||
return "", errors.New("submit_review rejected, nothing was recorded: summary must be a string")
|
||||
}
|
||||
if _, ok := result["findings"].([]any); !ok {
|
||||
return "", errors.New("submit_review rejected, nothing was recorded: findings must be a list")
|
||||
}
|
||||
|
||||
err = m.store.FinishReview(ctx, l.reviewID, api.ReviewStateDone, result, "")
|
||||
if errors.Is(err, store.ErrNotFound) {
|
||||
return "", errors.New("this review was already submitted or has ended")
|
||||
}
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
return "Review recorded. End your turn now with one line.", nil
|
||||
}
|
||||
|
||||
func (m *Manager) reviewTurnEnded(ctx context.Context, l *live, res endResult) error {
|
||||
m.mu.Lock()
|
||||
interrupted := l.interrupted
|
||||
m.mu.Unlock()
|
||||
|
||||
err := m.emit(ctx, l, agentActor, "turn.ended", res.payload)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if res.window > 0 {
|
||||
err = m.updateContext(ctx, l, l.contextUsed, res.window)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
reason := fmt.Sprintf("the reviewer ended its turn without calling submit_review: %s", clip(strings.TrimSpace(res.result), 500))
|
||||
switch {
|
||||
case interrupted:
|
||||
reason = "the reviewer was interrupted"
|
||||
case res.isError:
|
||||
reason = fmt.Sprintf("the reviewer's turn ended with an error (%s): %s", res.subtype, clip(res.result, 500))
|
||||
}
|
||||
go m.finishReviewer(l, reason)
|
||||
return nil
|
||||
}
|
||||
|
||||
func (m *Manager) finishReviewer(l *live, reason string) {
|
||||
ctx := context.Background()
|
||||
project, err := m.store.GetProject(ctx, l.projectID)
|
||||
if err != nil {
|
||||
slog.Error("load project to end reviewer", "review", l.reviewID, "err", err)
|
||||
return
|
||||
}
|
||||
m.endReviewer(ctx, l.id, project, l.reviewID)
|
||||
m.failReview(ctx, l.reviewID, reason)
|
||||
}
|
||||
|
||||
func (m *Manager) endReviewer(ctx context.Context, sessionID string, project api.Project, reviewID string) {
|
||||
sess, err := m.store.GetSession(ctx, sessionID)
|
||||
if err != nil {
|
||||
slog.Error("load reviewer session", "session", sessionID, "err", err)
|
||||
}
|
||||
if err == nil && sess.State != api.AgentSessionStateCompleted {
|
||||
err = m.retire(ctx, sess, "byre")
|
||||
if err != nil {
|
||||
slog.Error("retire reviewer session", "session", sessionID, "err", err)
|
||||
}
|
||||
}
|
||||
m.removeReviewDir(project, reviewID)
|
||||
}
|
||||
|
||||
func (m *Manager) removeReviewDir(project api.Project, reviewID string) {
|
||||
err := git.RemoveDetachedWorktree(project.RepoPath, m.reviewDir(project.Id, reviewID))
|
||||
if err != nil {
|
||||
slog.Error("remove review worktree", "review", reviewID, "err", err)
|
||||
}
|
||||
}
|
||||
|
||||
func (m *Manager) failReview(ctx context.Context, reviewID, reason string) {
|
||||
err := m.store.FinishReview(ctx, reviewID, api.ReviewStateFailed, nil, reason)
|
||||
if errors.Is(err, store.ErrNotFound) {
|
||||
return
|
||||
}
|
||||
if err != nil {
|
||||
slog.Error("record failed review", "review", reviewID, "err", err)
|
||||
return
|
||||
}
|
||||
slog.Info("review failed", "review", reviewID, "reason", reason)
|
||||
}
|
||||
|
||||
func (m *Manager) cancelReviews(ctx context.Context, task api.Task, project api.Project) {
|
||||
m.reviewMu.Lock()
|
||||
defer m.reviewMu.Unlock()
|
||||
reviews, err := m.store.ListReviews(ctx, task.Id)
|
||||
if err != nil {
|
||||
slog.Error("list reviews to cancel", "task", task.Id, "err", err)
|
||||
return
|
||||
}
|
||||
for _, r := range reviews {
|
||||
if r.State != api.ReviewStateRunning {
|
||||
continue
|
||||
}
|
||||
m.endReviewer(ctx, r.AgentSessionId, project, r.Id)
|
||||
m.failReview(ctx, r.Id, "the task was closed")
|
||||
}
|
||||
}
|
||||
|
||||
func (m *Manager) recoverReviews(ctx context.Context) error {
|
||||
running, err := m.store.RunningReviews(ctx)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
for _, r := range running {
|
||||
sess, err := m.store.GetSession(ctx, r.AgentSessionId)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if sess.State == api.AgentSessionStateRunning {
|
||||
continue
|
||||
}
|
||||
task, err := m.store.GetTask(ctx, r.TaskId)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
project, err := m.store.GetProject(ctx, task.ProjectId)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
m.endReviewer(ctx, r.AgentSessionId, project, r.Id)
|
||||
m.failReview(ctx, r.Id, "byre restarted before the reviewer finished")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func reviewLine(r api.Review) string {
|
||||
if r.State == api.ReviewStateFailed {
|
||||
return fmt.Sprintf("review of snapshot %d failed: %s", r.SnapshotN, clip(r.Error, 300))
|
||||
}
|
||||
res := map[string]any{}
|
||||
if r.Result != nil {
|
||||
res = *r.Result
|
||||
}
|
||||
b := &strings.Builder{}
|
||||
fmt.Fprintf(b, "review of snapshot %d: %s", r.SnapshotN, clip(strings.TrimSpace(fmt.Sprint(res["summary"])), 600))
|
||||
findings, _ := res["findings"].([]any)
|
||||
for _, f := range findings {
|
||||
fm, _ := f.(map[string]any)
|
||||
where := ""
|
||||
if p, ok := fm["path"].(string); ok && p != "" {
|
||||
where = " (" + p
|
||||
if line, ok := fm["line"].(float64); ok && line > 0 {
|
||||
where += fmt.Sprintf(":%d", int(line))
|
||||
}
|
||||
where += ")"
|
||||
}
|
||||
fmt.Fprintf(b, "\n - [%v] %v%s: %s", fm["severity"], fm["title"], where, clip(strings.TrimSpace(fmt.Sprint(fm["detail"])), 300))
|
||||
}
|
||||
return b.String()
|
||||
}
|
||||
Reference in New Issue
Block a user