byre/internal/agents/review.go

package agents

import (
	"context"
	"encoding/json"
	"errors"
	"fmt"
	"log/slog"
	"net/http"
	"os"
	"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 all of its work so far, 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.
- Start the review with ` + "`change`" + `: what the change does, in a few sentences, for someone who hasn't read it.
- 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.
- With no findings, the summary is exactly "No significant findings." and nothing else. With findings, the summary is a bulleted Markdown list, one line per finding, and nothing else.
- Show rather than quote. Give a finding's file and lines and byre shows that part of the diff, so don't paste code. When a picture would explain something better than prose, such as a data flow, call graph, state machine or schema the change alters, draw it in a ` + "```mermaid" + ` block. When the change alters something you can see, you can run it (on any free port on 127.0.0.1) and attach screenshots taken with the Playwright tools. Most reviews need neither; leave out a diagram or screenshot that adds nothing to the text.`

func reviewSchema() json.RawMessage {
	images := map[string]any{
		"type":        "array",
		"description": "Screenshots, only when they show something the text can't.",
		"items": map[string]any{
			"type": "object",
			"properties": map[string]any{
				"path":    map[string]any{"type": "string", "description": "A PNG, JPEG, GIF or WebP file of at most 5 MiB you saved: absolute, or relative to your working directory. byre keeps a copy."},
				"caption": map[string]any{"type": "string", "description": "One line saying what it shows."},
			},
			"required": []any{"path"},
		},
	}
	b, err := json.Marshal(map[string]any{
		"type": "object",
		"properties": map[string]any{
			"change":  map[string]any{"type": "string", "description": "What the change does, in a few sentences of Markdown, for someone who hasn't read it."},
			"summary": map[string]any{"type": "string", "description": "Exactly \"No significant findings.\" when findings is empty; otherwise a bulleted Markdown list with one line per finding, and nothing else."},
			"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", "description": "Line in the file as of this snapshot; byre shows the diff around it."},
						"end_line": map[string]any{"type": "integer", "description": "Last line, when the finding covers a range."},
						"images":   images,
					},
					"required": []any{"severity", "title", "detail"},
				},
			},
			"images": images,
		},
		"required": []any{"change", "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) reviewMediaDir(projectID, reviewID string) string {
	return m.reviewDir(projectID, reviewID) + "-media"
}

func playwrightMCP(outputDir string) mcp.Stdio {
	return mcp.Stdio{
		Name:    "playwright",
		Command: "npx",
		Args:    []string{"--yes", "@playwright/mcp@0.0.83", "--headless", "--isolated", "--browser", "chrome", "--file-paths", "absolute", "--output-dir", outputDir},
	}
}

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
	}
	if prompt == "" {
		now := time.Now().UTC()
		err = m.store.CreateReview(ctx, api.Review{
			Id:         store.NewID(),
			TaskId:     task.Id,
			SnapshotN:  snap.N,
			State:      api.ReviewStateSkipped,
			CreatedAt:  now,
			FinishedAt: &now,
		})
		if err != nil {
			return err
		}
		slog.Info("review skipped, snapshot has no diff", "task", task.Id, "snapshot", snap.N)
		return nil
	}

	reviewID := store.NewID()
	dir := m.reviewDir(project.Id, reviewID)
	err = git.AddDetachedWorktree(project.RepoPath, dir, snap.CommitSha)
	if err != nil {
		return err
	}
	err = os.MkdirAll(m.reviewMediaDir(project.Id, reviewID), 0o755)
	if err != nil {
		m.removeReviewDir(project, reviewID)
		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) {
	changes, err := git.Compare(project.RepoPath, project.DefaultBranch, snap.CommitSha, snap.TreeSha, "")
	if err != nil {
		return "", err
	}
	if len(changes.Files) == 0 {
		return "", nil
	}
	brief, err := m.store.FirstTaskMessage(ctx, task.Id)
	if err != nil {
		return "", err
	}

	b := &strings.Builder{}
	fmt.Fprintf(b, "Review the work of the agent on the byre task %q: 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 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))
	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)
	}
	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 c, ok := result["change"].(string); !ok || strings.TrimSpace(c) == "" {
		return "", errors.New("submit_review rejected, nothing was recorded: change must say what the change does")
	}
	if _, ok := result["summary"].(string); !ok {
		return "", errors.New("submit_review rejected, nothing was recorded: summary must be a string")
	}
	findings, ok := result["findings"].([]any)
	if !ok {
		return "", errors.New("submit_review rejected, nothing was recorded: findings must be a list")
	}

	holders := []map[string]any{result}
	for _, f := range findings {
		fm, ok := f.(map[string]any)
		if !ok {
			return "", errors.New("submit_review rejected, nothing was recorded: each finding must be an object")
		}
		holders = append(holders, fm)
	}
	pending := []reviewImage{}
	for _, holder := range holders {
		imgs, err := m.readReviewImages(l, holder["images"])
		if err != nil {
			return "", fmt.Errorf("submit_review rejected, nothing was recorded: %w", err)
		}
		pending = append(pending, imgs...)
	}
	for _, img := range pending {
		b, err := m.store.CreateBlob(ctx, img.contentType, img.data, agentActor)
		if err != nil {
			return "", err
		}
		img.entry["blob"] = b.Id
		delete(img.entry, "path")
	}

	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
	}
	if len(findings) > 0 {
		go m.forwardFindings(l.taskID, l.reviewID)
	}
	go m.reviewFinished(l.reviewID)
	return "Review recorded. End your turn now with one line.", nil
}

func (m *Manager) forwardFindings(taskID, reviewID string) {
	ctx := context.Background()
	r, err := m.store.GetReview(ctx, reviewID)
	if err != nil {
		slog.Error("load review to forward", "review", reviewID, "err", err)
		return
	}
	sessions, err := m.store.ListSessions(ctx, taskID)
	if err != nil {
		slog.Error("list sessions to forward review", "review", reviewID, "err", err)
		return
	}
	for _, s := range sessions {
		if s.State == api.AgentSessionStateCompleted {
			continue
		}
		_, err = m.Message(ctx, s.Id, "byre", findingsMessage(r), "")
		if err != nil {
			slog.Error("forward review findings", "review", reviewID, "session", s.Id, "err", err)
			return
		}
		slog.Info("review findings forwarded", "review", reviewID, "session", s.Id)
		return
	}
	slog.Info("review findings not forwarded, task has no live agent", "review", reviewID, "task", taskID)
}

func findingsMessage(r api.Review) string {
	res := map[string]any{}
	if r.Result != nil {
		res = *r.Result
	}
	b := &strings.Builder{}
	fmt.Fprintf(b, "A reviewer looked at snapshot #%d of your work and raised the findings below. Reviewers can be wrong or miss context: check each one against the code and your brief, act on the ones that hold up, and in your final message say briefly which you acted on and which you're leaving and why.", r.SnapshotN)
	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 = " (`" + where + "`)"
		}
		fmt.Fprintf(b, "\n\n**[%v] %v**%s\n\n%s", fm["severity"], fm["title"], where, strings.TrimSpace(fmt.Sprint(fm["detail"])))
	}
	return b.String()
}

type reviewImage struct {
	entry       map[string]any
	contentType string
	data        []byte
}

func (m *Manager) readReviewImages(l *live, raw any) ([]reviewImage, error) {
	if raw == nil {
		return nil, nil
	}
	list, ok := raw.([]any)
	if !ok {
		return nil, errors.New("images must be a list")
	}
	out := []reviewImage{}
	for _, item := range list {
		entry, ok := item.(map[string]any)
		if !ok {
			return nil, errors.New("each image must be an object with a path")
		}
		p, ok := entry["path"].(string)
		if !ok || p == "" {
			return nil, errors.New("each image needs a path")
		}
		if !filepath.IsAbs(p) {
			p = filepath.Join(l.dir, p)
		}
		info, err := os.Stat(p)
		if err != nil {
			return nil, fmt.Errorf("image %s: %w", p, err)
		}
		if !info.Mode().IsRegular() {
			return nil, fmt.Errorf("image %s is not a file", p)
		}
		if info.Size() > store.MaxBlobBytes {
			return nil, fmt.Errorf("image %s is %d bytes; images are limited to %d MiB", p, info.Size(), store.MaxBlobBytes>>20)
		}
		data, err := os.ReadFile(p)
		if err != nil {
			return nil, fmt.Errorf("image %s: %w", p, err)
		}
		contentType := http.DetectContentType(data)
		if !store.ImageTypes[contentType] {
			return nil, fmt.Errorf("image %s is %s; use PNG, JPEG, GIF or WebP", p, contentType)
		}
		out = append(out, reviewImage{entry: entry, contentType: contentType, data: data})
	}
	return out, 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)
	}
	err = os.RemoveAll(m.reviewMediaDir(project.Id, reviewID))
	if err != nil {
		slog.Error("remove review media", "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)
	go m.reviewFinished(reviewID)
}

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) StopReview(ctx context.Context, reviewID, reason string) (api.Review, error) {
	m.reviewMu.Lock()
	defer m.reviewMu.Unlock()
	r, err := m.store.GetReview(ctx, reviewID)
	if err != nil {
		return api.Review{}, err
	}
	if r.State != api.ReviewStateRunning {
		return api.Review{}, fmt.Errorf("%w: review %s is %s, not running", ErrConflict, r.Id, r.State)
	}
	task, err := m.store.GetTask(ctx, r.TaskId)
	if err != nil {
		return api.Review{}, err
	}
	project, err := m.store.GetProject(ctx, task.ProjectId)
	if err != nil {
		return api.Review{}, err
	}
	m.endReviewer(ctx, r.AgentSessionId, project, r.Id)
	m.failReview(ctx, r.Id, reason)
	return m.store.GetReview(ctx, r.Id)
}

func (m *Manager) stopStuckReviewers(ctx context.Context, h Health) {
	for _, p := range h.Processes {
		if p.Health != api.HealthStuck {
			continue
		}
		m.mu.Lock()
		l := m.live[p.SessionId]
		reviewID := ""
		if l != nil {
			reviewID = l.reviewID
		}
		m.mu.Unlock()
		if reviewID == "" {
			continue
		}
		_, err := m.StopReview(ctx, reviewID, fmt.Sprintf("the reviewer went %d minutes without an event", int(StuckAfter/time.Minute)))
		if err != nil && !errors.Is(err, ErrConflict) {
			slog.Error("stop stuck reviewer", "review", reviewID, "session", p.SessionId, "err", err)
		}
	}
}

func (m *Manager) recoverReviews(ctx context.Context) error {
	err := m.store.ClearReviewRequests(ctx)
	if err != nil {
		return err
	}
	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))
	}
	if r.State == api.ReviewStateSkipped {
		return fmt.Sprintf("review of snapshot %d skipped: no changes", r.SnapshotN)
	}
	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))
	if c, ok := res["change"].(string); ok && c != "" {
		fmt.Fprintf(b, "\n  change: %s", clip(strings.TrimSpace(c), 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()
}