Add change summaries, inline diff hunks and screenshots to reviews

internal/agents/review.go

@@ -6,6 +6,8 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"net/http"
+	"os"
 	"path/filepath"
 	"strings"
 	"time"
@@ -25,13 +27,28 @@ 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, nits and any description of the change. 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.`
+- 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",
@@ -43,13 +60,16 @@ func reviewSchema() json.RawMessage {
 						"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"},
+						"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{"summary", "findings"},
+		"required": []any{"change", "summary", "findings"},
 	})
 	if err != nil {
 		panic(err)
@@ -83,6 +103,18 @@ 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 {
@@ -120,6 +152,11 @@ func (m *Manager) startReviewer(ctx context.Context, task api.Task, snap api.Sna
 	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{
@@ -238,13 +275,42 @@ func (m *Manager) submitReview(ctx context.Context, l *live, raw json.RawMessage
 	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")
 	}
-	if _, ok := result["findings"].([]any); !ok {
+	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")
@@ -255,6 +321,56 @@ func (m *Manager) submitReview(ctx context.Context, l *live, raw json.RawMessage
 	return "Review recorded. End your turn now with one line.", nil
 }
 
+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
@@ -312,6 +428,10 @@ func (m *Manager) removeReviewDir(project api.Project, reviewID string) {
 	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) {
@@ -380,6 +500,9 @@ func reviewLine(r api.Review) string {
 	}
 	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)