diff --git a/README.md b/README.md index b5621455..0adce418 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,30 @@ standalone or as a deeper review engine inside metaswarm, Superpowers, and Beads **→ New here? Read [USAGE.md](USAGE.md) for a task-oriented walkthrough, and [INSTALL.md](INSTALL.md) to get set up.** +## Review a whole repository + +From a metareview source checkout, review a repository with Grok: + +```bash +bin/source-review --model grok --repo /path/to/repo +``` + +Both `--model` and `--repo` are required. Replace `grok` with `opus` or `astra` +to use another model; use `--repo .` to review your current directory. The script +builds metareview automatically. You need Go and a logged-in model CLI (`grok`, +`claude`, or `codex`). + +Each run saves `findings.json` and an offline `review.html` in its own directory +under `/tmp`; the script prints that directory before starting. It reviews the +committed first-party source at `HEAD`, skipping tests, docs, dependencies, and +generated files. + +For path filters, concurrency, or timeout controls, use the full command: + +```bash +bin/metareview source-review --model grok --output /tmp/grok-review --jobs 8 --call-timeout 30m /path/to/repo +``` + ## Use Cases - **Workflow runs.** `metareview fsm` drives `sdlc-loop` (discover → adjudicate → fix → verify) and `review-loop` as an audited state machine: the agent does the host nodes' work in its own warm session, judge calls are auditable and swappable, and resume is a fork. Contract: `metareview fsm --agent-prompt`; guide: `docs/fsm/driving-a-workflow.md`. diff --git a/bin/ci b/bin/ci new file mode 100755 index 00000000..9121eef7 --- /dev/null +++ b/bin/ci @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +set -euo pipefail +cd "$(dirname "$0")/.." +exec make cover diff --git a/bin/source-review b/bin/source-review new file mode 100755 index 00000000..8de250aa --- /dev/null +++ b/bin/source-review @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +set -euo pipefail + +usage() { + printf 'Usage: %s --model --repo \n' "$0" +} + +model="" +repo="" +while [[ $# -gt 0 ]]; do + case "$1" in + --model|--repo) + if [[ $# -lt 2 || -z "$2" || "$2" == --* ]]; then + printf 'Missing value for %s\n' "$1" >&2 + usage >&2 + exit 2 + fi + case "$1" in + --model) model="$2" ;; + --repo) repo="$2" ;; + esac + shift 2 + ;; + --help|-h) usage; exit 0 ;; + *) printf 'Unknown argument: %s\n' "$1" >&2; usage >&2; exit 2 ;; + esac +done + +if [[ -z "$model" || -z "$repo" ]]; then + printf 'Both --model and --repo are required\n' >&2 + usage >&2 + exit 2 +fi +case "$model" in + grok|opus|astra) ;; + *) printf 'Unknown model: %s\n' "$model" >&2; usage >&2; exit 2 ;; +esac + +# Resolve the caller's repo before moving into the metareview checkout to build. +repo="$(cd -- "$repo" && pwd -P)" +root="$(cd -- "$(dirname "${BASH_SOURCE[0]}")/.." && pwd -P)" +cd -- "$root" +go build -o bin/metareview ./cmd/metareview + +# Each run gets its own reports, outside the repository being reviewed. +output="$(mktemp -d "/tmp/metareview-${model}-XXXXXX")" +printf 'Reports: %s\n' "$output" >&2 +exec "$root/bin/metareview" source-review --model "$model" --output "$output" "$repo" diff --git a/cmd/metareview/main.go b/cmd/metareview/main.go index fb287a13..37bfa8ff 100644 --- a/cmd/metareview/main.go +++ b/cmd/metareview/main.go @@ -36,6 +36,7 @@ import ( "github.com/dsifry/metareview/internal/reviewprompt" "github.com/dsifry/metareview/internal/reviewstate" "github.com/dsifry/metareview/internal/setup" + "github.com/dsifry/metareview/internal/sourcereview" "github.com/dsifry/metareview/internal/status" "github.com/dsifry/metareview/internal/taskdone" "github.com/dsifry/metareview/internal/version" @@ -128,6 +129,7 @@ Usage: metareview review record-lenses [--scope pr-ready|task-done|epic-ready] [--base ] [--verdict ] [--mode subagent-adjudicated|in-session-emulated] [--lenses a,b,c] [--from-run ] metareview review checkpoint --scope pr-ready|task-done|epic-ready metareview learn --post-merge [--base ] [--github-pr ] [--session-root ] + metareview source-review --model astra|opus|grok --output [--jobs ] [--call-timeout ] [--path ]... [] --base last-reviewed (task-done, epic-ready, pr-ready, record-lenses) reviews only what is new since the last passing review of that scope: the head printed by 'review checkpoint'. @@ -158,6 +160,7 @@ Commands: review record-lenses Record an adjudicated lens review over HEAD (satisfies the require-lenses gate) review checkpoint Print the head of the last passing review of a scope on an ancestor of HEAD learn --post-merge Curate post-merge repository learning + source-review Review first-party source with Astra, Opus, or Grok `, version.Version) } @@ -183,6 +186,11 @@ func dispatch(args []string) { return } + if args[0] == "source-review" { + exit(sourcereview.CLI(args[1:], workdir, stdout, stderr)) + return + } + // `review --help` prints usage and runs nothing: without this, the review // subcommands took "--help" as the target and ran (and logged) a full review of it. if args[0] == "review" && slices.ContainsFunc(args[1:], func(a string) bool { return a == "--help" || a == "-h" }) { diff --git a/cmd/metareview/main_test.go b/cmd/metareview/main_test.go index 5b5dd3b0..c6d9e5a1 100644 --- a/cmd/metareview/main_test.go +++ b/cmd/metareview/main_test.go @@ -86,6 +86,17 @@ func TestRunHelpAndVersion(t *testing.T) { } } +func TestRunSourceReviewHelp(t *testing.T) { + code, out, errOut := runCLI(t, t.TempDir(), nil, "source-review", "--help") + if code != 0 || !strings.Contains(out, "source-review --model astra|opus|grok --output ") || errOut != "" { + t.Fatalf("code=%d out=%q err=%q", code, out, errOut) + } + code, help, _ := runCLI(t, t.TempDir(), nil, "--help") + if code != 0 || !strings.Contains(help, "metareview source-review --model astra|opus|grok --output [--jobs ] [--call-timeout ] [--path ]... []") { + t.Fatalf("top-level help missing source-review: %q", help) + } +} + func TestRunUnknownCommand(t *testing.T) { code, _, errOut := runCLI(t, t.TempDir(), nil, "bogus") if code != 2 || !strings.Contains(errOut, "Unknown command") { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0e495b6d..95f2800d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -39,6 +39,28 @@ abbreviated — task-done/epic-ready/pr-ready also take `--evidence ` (a v | **epic-ready** | `review epic-ready ` | the epic's **integration diff** (base..HEAD, the union of the children's changes), **with the roll-up as context** — child evidence present? contradictions? intent drift? registry coverage? | deterministic heuristics (roll-up freshness) **+ a required adjudicated review** over the integration diff (base..HEAD) via the `epic-review-loop` workflow — same require-lenses gate as pr-ready/task-done | | **learn** | `learn --post-merge ` | what the merged PR + bot findings teach us | learning extraction | +`source-review` is a separate command (`source-review --model astra|opus|grok --output [--jobs ] +[--call-timeout ] [--path ]... []`). +It reviews the first-party source at HEAD — code `Classify` keeps, minus tests, vendored trees, +generated files, and non-UTF-8, narrowed to `--path` when given (a sample of the real checkout) — by calling the logged-in `claude`, `codex`, or `grok` CLI, up to `--jobs` +prompts at once (default 8), each call killed with its process group past `--call-timeout` (default 30m). +Every model gets the same setup — one system prompt, one findings JSON schema, no tools, one answer — so they +are compared on the same job: `claude -p --tools "" --system-prompt --json-schema --strict-mcp-config`, +`grok --prompt-file --verbatim --json-schema --tools read_file --disallowed-tools read_file,search_tool,use_tool +--max-turns 1 --system-prompt-override --reasoning-effort medium` +(`grok --tools ""` retains the default tools; a nonempty allowlist followed by denial actually removes them; +`grok -p` hands a large prompt to its agent as an excerpt it re-reads with tools), and +`codex exec -s read-only --skip-git-repo-check --output-schema` +(Codex has no switch to remove its tools or replace its system prompt, so it is the one that differs). +Calls start in an empty temporary working directory, cleaned after success or failure, so the host project's +rules are not injected. Per-process settings disable Claude customizations and Grok's Claude/Cursor compatibility +instructions. Native provider context can still differ. Grok's requested model is also set as its initial +default: switching models after initialization otherwise replaces the supplied system prompt. Claude's effort +environment override is set explicitly because it takes precedence over the CLI flag. Neither provider's +global configuration is edited. Explicit token-limit stops and Grok structured-output failures fail the call, +even if response text happens to parse as findings JSON. +It does not use the diff-review anchor gate, `ValidatePayload`, `internal/shardpack`, or `review-lenses`. + `epic-ready` runs *after* every child task is task-done-reviewed. It reviews the epic's **integration diff** (base..HEAD, the union of the children's changes) **with the roll-up — child review logs, evidence, parent intent — as context**; the roll-up's own freshness is guarded by the deterministic pre-checks (re-read every @@ -489,7 +511,8 @@ list below is illustrative, omitting e.g. `judge`, `gate`, `converge`, `export`) (floor gate). - **Context & evidence:** `gitcontext` (exclude-filtered diff), `githubcontext`, `contextpack`, `contextprofile`, `shardpack` (shard packs), `evidence`, `mutation` (Stryker/gremlins report), `knowledge`, - `markdown`, `classify` (file class), `testconv` (test-file convention). + `markdown`, `classify` (file class), `testconv` (test-file convention), + `sourcereview` (whole-repo first-party source review; not the diff gate). - **FSM:** `fsm/{cli,machine,workflow,run,record,sandbox,kind}`. - **Sources & learning:** `tasksource`, `epicsource` (Beads etc.), `learning`, `learnsource`, `sessionhistory`, `integration` (metaswarm). diff --git a/internal/sourcereview/fixture_test.go b/internal/sourcereview/fixture_test.go new file mode 100644 index 00000000..5c596fbb --- /dev/null +++ b/internal/sourcereview/fixture_test.go @@ -0,0 +1,89 @@ +package sourcereview + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func gitEnv() []string { + return append(os.Environ(), + "GIT_CONFIG_GLOBAL=/dev/null", + "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_AUTHOR_NAME=Test", + "GIT_AUTHOR_EMAIL=t@example.com", + "GIT_COMMITTER_NAME=Test", + "GIT_COMMITTER_EMAIL=t@example.com", + ) +} + +func gitRun(t *testing.T, root string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = root + cmd.Env = gitEnv() + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } +} + +func newRepoBytes(t *testing.T, files map[string][]byte) (root, commit string) { + t.Helper() + root = t.TempDir() + gitRun(t, root, "init", "-b", "main") + gitRun(t, root, "config", "user.email", "t@example.com") + gitRun(t, root, "config", "user.name", "Test") + for p, body := range files { + abs := filepath.Join(root, filepath.FromSlash(p)) + if err := os.MkdirAll(filepath.Dir(abs), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(abs, body, 0o644); err != nil { + t.Fatal(err) + } + } + gitRun(t, root, "add", "-A") + gitRun(t, root, "commit", "-m", "fixture") + cmd := exec.Command("git", "rev-parse", "HEAD") + cmd.Dir = root + cmd.Env = gitEnv() + out, err := cmd.Output() + if err != nil { + t.Fatal(err) + } + return root, strings.TrimSpace(string(out)) +} + +func specFiles() map[string][]byte { + return map[string][]byte{ + "pkg/app.go": []byte("package pkg\n\nfunc App() int { return 1 }\n"), + "pkg/note.go": []byte("DO NOT EDIT\npackage pkg\n\nfunc Note() string { return \"n\" }\n"), + "pkg/app_test.go": []byte("package pkg\n\nfunc TestApp(t *testing.T) {}\n"), + "vendor/lib.go": []byte("package vendor\n\nfunc Lib() {}\n// DROPME-vendor\n"), + "go.sum": []byte("example.com/mod v1.0.0 h1:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa=\n"), + "pkg/gen.go": []byte("// Code generated by fixture. DO NOT EDIT.\npackage pkg\n\nfunc Gen() {}\n"), + "pkg/blob.dat": {0xff, 0xfe, 0x00, 0x01}, + "README.md": []byte("# Demo\n\nDROPME-readme\n"), + "config.yaml": []byte("name: demo\nDROPME-config\n"), + } +} + +func droppedOnlyFiles() map[string][]byte { + all := specFiles() + delete(all, "pkg/app.go") + delete(all, "pkg/note.go") + return all +} + +var specDropped = []string{ + "README.md", + "config.yaml", + "go.sum", + "pkg/app_test.go", + "pkg/blob.dat", + "pkg/gen.go", + "vendor/lib.go", +} diff --git a/internal/sourcereview/git.go b/internal/sourcereview/git.go new file mode 100644 index 00000000..c05c3d81 --- /dev/null +++ b/internal/sourcereview/git.go @@ -0,0 +1,126 @@ +package sourcereview + +import ( + "bytes" + "errors" + "fmt" + "os/exec" + "strconv" + "strings" +) + +// GitFunc runs git with optional stdin and returns stdout. A nil GitFunc means realGit. +type GitFunc func(root string, args []string, stdin []byte) ([]byte, error) + +// gitBin is the git executable. Tests point it at a missing name to cover a failed start. +var gitBin = "git" + +func realGit(root string, args []string, stdin []byte) ([]byte, error) { + cmd := exec.Command(gitBin, append([]string{"-C", root}, args...)...) + if stdin != nil { + cmd.Stdin = bytes.NewReader(stdin) + } + out, err := cmd.Output() + if err != nil { + var ee *exec.ExitError + if errors.As(err, &ee) { + return nil, fmt.Errorf("git %s: %s", strings.Join(args, " "), bytes.TrimSpace(ee.Stderr)) + } + return nil, err + } + return out, nil +} + +func resolveTop(root string, git GitFunc) (string, error) { + out, err := git(root, []string{"rev-parse", "--show-toplevel"}, nil) + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + +type treeBlob struct { + path string + id string +} + +// parseTree reads `git ls-tree -r -z` records, preserving arbitrary filenames +// and the object IDs used to read this exact snapshot. +func parseTree(out []byte) ([]treeBlob, error) { + var blobs []treeBlob + rest := out + for len(rest) > 0 { + rec, next, _ := bytes.Cut(rest, []byte{0}) + rest = next + if len(rec) == 0 { + continue + } + tab := bytes.IndexByte(rec, '\t') + if tab < 0 { + return nil, fmt.Errorf("bad ls-tree record") + } + fields := strings.Fields(string(rec[:tab])) + if len(fields) < 3 { + return nil, fmt.Errorf("bad ls-tree meta") + } + if fields[1] != "blob" { + continue + } + p := string(rec[tab+1:]) + if p == "" { + continue + } + blobs = append(blobs, treeBlob{path: p, id: fields[2]}) + } + return blobs, nil +} + +func readBlobs(root string, blobs []treeBlob, git GitFunc) (map[string][]byte, error) { + if len(blobs) == 0 { + return map[string][]byte{}, nil + } + var stdin bytes.Buffer + paths := make([]string, 0, len(blobs)) + for _, b := range blobs { + // Object IDs avoid line-delimited filenames and any later HEAD lookup. + fmt.Fprintln(&stdin, b.id) + paths = append(paths, b.path) + } + out, err := git(root, []string{"cat-file", "--batch"}, stdin.Bytes()) + if err != nil { + return nil, err + } + return parseBatch(out, paths) +} + +func parseBatch(out []byte, paths []string) (map[string][]byte, error) { + files := make(map[string][]byte, len(paths)) + rest := out + for _, p := range paths { + nl := bytes.IndexByte(rest, '\n') + if nl < 0 { + return nil, fmt.Errorf("short cat-file header for %s", p) + } + header := string(rest[:nl]) + rest = rest[nl+1:] + parts := strings.Split(header, " ") + if len(parts) < 3 || parts[1] != "blob" { + return nil, fmt.Errorf("cat-file %s: %s", p, header) + } + size, err := strconv.Atoi(parts[2]) + if err != nil { + return nil, fmt.Errorf("cat-file %s: %w", p, err) + } + if size < 0 || len(rest) < size+1 { + return nil, fmt.Errorf("short cat-file blob %s", p) + } + if rest[size] != '\n' { + return nil, fmt.Errorf("cat-file %s: missing trailing newline", p) + } + body := make([]byte, size) + copy(body, rest[:size]) + files[p] = body + rest = rest[size+1:] + } + return files, nil +} diff --git a/internal/sourcereview/model.go b/internal/sourcereview/model.go new file mode 100644 index 00000000..94c8083d --- /dev/null +++ b/internal/sourcereview/model.go @@ -0,0 +1,295 @@ +package sourcereview + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "os" + "os/exec" + "strings" + "syscall" + "time" +) + +// Runner runs one logged-in CLI. stdin is the prompt for claude and codex; +// grok reads the prompt from the --prompt-file file and stdin is nil. When ctx +// ends, the process and its children are killed. +type Runner interface { + Run(ctx context.Context, name string, args []string, stdin []byte) ([]byte, error) +} + +// OSRunner is the real process runner. It does not read an API key and does not +// add a token to the command. +type OSRunner struct{} + +// waitDelay bounds how long Run waits for output pipes after the process group +// is killed (the internal/fsm/cmdexec pattern). +const waitDelay = 2 * time.Second + +const ( + grokModelID = "grok-4.7" + reviewEffort = "medium" +) + +func (OSRunner) Run(ctx context.Context, name string, args []string, stdin []byte) ([]byte, error) { + // Review only the supplied source. Starting in the host checkout injects + // unrelated project rules and skills into the model's conversation. + dir, err := os.MkdirTemp("", "metareview-workspace-*") + if err != nil { + return nil, err + } + defer func() { _ = os.RemoveAll(dir) }() + cmd := exec.CommandContext(ctx, name, args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "CLAUDE_CODE_SAFE_MODE=1", + // Claude gives this environment variable precedence over --effort. + "CLAUDE_CODE_EFFORT_LEVEL="+reviewEffort, + // Selecting a different Grok model after initialization re-renders its + // coding-agent system prompt, discarding --system-prompt-override. + // Make the requested model the session default before initialization. + fmt.Sprintf(`GROK_CONFIG={"models":{"default":%q}}`, grokModelID), + "GROK_CLAUDE_AGENTS_ENABLED=false", "GROK_CLAUDE_RULES_ENABLED=false", "GROK_CLAUDE_SKILLS_ENABLED=false", + "GROK_CURSOR_AGENTS_ENABLED=false", "GROK_CURSOR_RULES_ENABLED=false", "GROK_CURSOR_SKILLS_ENABLED=false", + ) + if stdin != nil { + cmd.Stdin = bytes.NewReader(stdin) + } + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) } + cmd.WaitDelay = waitDelay + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err = cmd.Run() + if err != nil { + // Codex puts the turn failure on stdout and a snapshot warning on + // stderr. Keep both so a non-zero exit records the CLI's reason. + msg := strings.TrimSpace(stderr.String()) + if out := strings.TrimSpace(stdout.String()); out != "" { + if msg != "" { + msg = msg + "\n" + out + } else { + msg = out + } + } + if msg == "" { + msg = err.Error() + } + return stdout.Bytes(), fmt.Errorf("%s: %s", name, msg) + } + return stdout.Bytes(), nil +} + +// ModelID is the id shown on the page and passed to the CLI. +func ModelID(model string) (string, error) { + switch strings.ToLower(strings.TrimSpace(model)) { + case "opus": + return "opus", nil + case "astra": + return "gpt-6-astra", nil + case "grok": + return grokModelID, nil + default: + return "", fmt.Errorf("unknown model %q (want astra, opus, or grok)", model) + } +} + +// Every model gets the same review setup: this system prompt, this answer +// schema, no tools, one answer. The prompt text and the parser are shared too, +// so the models are compared on the same job. + +// findingsSchema constrains the answer to {"findings":[...]} with all eight +// fields present and no others. Codex's strict schema mode rejects an object +// without "additionalProperties": false. +const findingsSchema = `{"type":"object","properties":{"findings":{"type":"array","items":{"type":"object","properties":{"tag":{"type":"string","enum":["bug","advisory"]},"file":{"type":"string"},"start_line":{"type":"integer"},"end_line":{"type":"integer"},"issue":{"type":"string"},"consequence":{"type":"string"},"confidence":{"type":"integer"},"severity":{"type":"string","enum":["P0","P1","P2","P3"]}},"required":["tag","file","start_line","end_line","issue","consequence","confidence","severity"],"additionalProperties":false}}},"required":["findings"],"additionalProperties":false}` + +// reviewSystem replaces each CLI's agent system prompt. Without it Grok tries +// to call tools and --max-turns 1 cancels the turn. +const reviewSystem = "You are a code reviewer. You have no tools. Everything you need is in the user message. Answer in a single message." + +// claudeArgs runs Claude as one turn with no tools, no MCP servers, and no +// saved session, at medium effort to match Grok's --reasoning-effort medium. +// The prompt is on stdin. +func claudeArgs() []string { + return []string{ + "-p", "--model", "opus", "--output-format", "json", + "--tools", "", "--system-prompt", reviewSystem, "--json-schema", findingsSchema, + "--strict-mcp-config", "--no-session-persistence", + "--effort", reviewEffort, + } +} + +// codexArgs runs Codex with the shared schema, a read-only sandbox, and no +// saved session. The prompt is on stdin ("-"). Codex has no switch to remove +// its tools or replace its system prompt, so it is the one model that differs. +func codexArgs(schemaFile string) []string { + return []string{ + "exec", "--json", "-m", "gpt-6-astra", + "-s", "read-only", "--ephemeral", "--skip-git-repo-check", "--output-schema", schemaFile, "-", + } +} + +// grokArgs runs Grok as one turn with no tools. -p hands a large prompt to +// Grok's agent as an excerpt plus a file to read back with tools, which made +// one hh prompt take 11 model calls; --prompt-file with --verbatim sends it whole. +// Effort is shared with Claude. Speed and independently verified quality are +// measured together; a lower raw finding count alone does not establish a loss. +// Grok treats --tools "" as the default toolset. Use a non-empty allowlist, +// then deny that tool and the always-on MCP dispatchers. Deny takes precedence, +// leaving no tools without maintaining an exhaustive list of built-in tools. +func grokArgs(promptFile string) []string { + return []string{ + "--prompt-file", promptFile, "--verbatim", + "-m", grokModelID, "--output-format", "json", + "--json-schema", findingsSchema, + "--tools", "read_file", "--disallowed-tools", "read_file,search_tool,use_tool", + "--max-turns", "1", "--no-subagents", "--disable-web-search", + "--system-prompt-override", reviewSystem, + "--reasoning-effort", reviewEffort, + } +} + +// Temp-file seams. Production uses the os functions. Tests replace them to +// cover the error returns. +var ( + createTemp = os.CreateTemp + removeFile = os.Remove +) + +// writeTempFile writes data to a new file in os.TempDir(), which is never +// inside the reviewed tree or the output directory. +func writeTempFile(pattern string, data []byte) (string, error) { + f, err := createTemp("", pattern) + if err != nil { + return "", err + } + _, werr := f.Write(data) + cerr := f.Close() + if err := errors.Join(werr, cerr); err != nil { + _ = removeFile(f.Name()) + return "", err + } + return f.Name(), nil +} + +// Call is the one function that runs claude, codex, or grok. The prompt text +// and the parser stay the same; only the CLI and the model id change. +func Call(ctx context.Context, runner Runner, model string, prompt []byte) (string, error) { + switch strings.ToLower(strings.TrimSpace(model)) { + case "opus": + out, err := runner.Run(ctx, "claude", claudeArgs(), prompt) + if err != nil { + return "", err + } + return claudeModelText(out) + case "astra": + schema, err := writeTempFile("metareview-schema-*.json", []byte(findingsSchema)) + if err != nil { + return "", err + } + defer func() { _ = removeFile(schema) }() + out, err := runner.Run(ctx, "codex", codexArgs(schema), prompt) + if err != nil { + return "", err + } + return codexModelText(out) + case "grok": + path, err := writeTempFile("metareview-grok-prompt-*.txt", prompt) + if err != nil { + return "", err + } + defer func() { _ = removeFile(path) }() + out, err := runner.Run(ctx, "grok", grokArgs(path), nil) + if err != nil { + return "", err + } + return grokModelText(out) + default: + return "", fmt.Errorf("unknown model %q", model) + } +} + +func claudeModelText(stdout []byte) (string, error) { + var doc struct { + IsError bool `json:"is_error"` + StopReason string `json:"stop_reason"` + Result *string `json:"result"` + StructuredOutput json.RawMessage `json:"structured_output"` + } + if err := json.Unmarshal(stdout, &doc); err != nil { + return "", fmt.Errorf("missing model text: %w", err) + } + if doc.IsError { + msg := "" + if doc.Result != nil { + msg = *doc.Result + } + return "", fmt.Errorf("claude: is_error: %s", msg) + } + if doc.StopReason == "max_tokens" { + return "", errors.New("claude: incomplete review: max_tokens") + } + if len(doc.StructuredOutput) > 0 && !bytes.Equal(doc.StructuredOutput, []byte("null")) { + if doc.StructuredOutput[0] != '{' { + return "", errors.New("claude: structured_output must be an object") + } + return string(doc.StructuredOutput), nil + } + if doc.Result == nil || *doc.Result == "" { + return "", errors.New("missing model text") + } + return *doc.Result, nil +} + +func codexModelText(stdout []byte) (string, error) { + var text string + found := false + for _, line := range strings.Split(string(stdout), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + var ev struct { + Type string `json:"type"` + Item struct { + Type string `json:"type"` + Text string `json:"text"` + } `json:"item"` + } + if json.Unmarshal([]byte(line), &ev) != nil { + continue + } + if ev.Type == "item.completed" && ev.Item.Type == "agent_message" { + text = ev.Item.Text + found = true + } + } + if !found || text == "" { + return "", errors.New("missing model text") + } + return text, nil +} + +func grokModelText(stdout []byte) (string, error) { + var doc struct { + Text *string `json:"text"` + StopReason string `json:"stopReason"` + StructuredOutputError string `json:"structuredOutputError"` + } + if err := json.Unmarshal(stdout, &doc); err != nil { + return "", fmt.Errorf("missing model text: %w", err) + } + if doc.StopReason == "max_tokens" { + return "", errors.New("grok: incomplete review: max_tokens") + } + if doc.StructuredOutputError != "" { + return "", fmt.Errorf("grok: %s", doc.StructuredOutputError) + } + if doc.Text == nil || *doc.Text == "" { + return "", errors.New("missing model text") + } + return *doc.Text, nil +} diff --git a/internal/sourcereview/model_test.go b/internal/sourcereview/model_test.go new file mode 100644 index 00000000..d2abef54 --- /dev/null +++ b/internal/sourcereview/model_test.go @@ -0,0 +1,496 @@ +package sourcereview + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "sync" + "testing" + "time" +) + +// fakeRunner records calls. It is safe for concurrent calls: n is the call's +// start order, active/maxActive count calls running at the same time, and wait +// (when set) runs inside the call before reply so a test can block on ctx. +type fakeRunner struct { + mu sync.Mutex + calls []fakeCall + active int + maxActive int + reply func(n int, name string, args []string, stdin []byte) ([]byte, error) + wait func(ctx context.Context, prompt []byte) error + onRun func() +} + +type fakeCall struct { + name string + args []string + stdin []byte + // prompt is the prompt the CLI received: stdin, or the --prompt-file bytes. + prompt []byte +} + +func (f *fakeRunner) Run(ctx context.Context, name string, args []string, stdin []byte) ([]byte, error) { + if f.onRun != nil { + f.onRun() + } + prompt := append([]byte(nil), stdin...) + for i, a := range args { + if a == "--prompt-file" && i+1 < len(args) { + prompt, _ = os.ReadFile(args[i+1]) + } + } + f.mu.Lock() + n := len(f.calls) + f.calls = append(f.calls, fakeCall{name, append([]string(nil), args...), append([]byte(nil), stdin...), prompt}) + f.active++ + if f.active > f.maxActive { + f.maxActive = f.active + } + f.mu.Unlock() + defer func() { + f.mu.Lock() + f.active-- + f.mu.Unlock() + }() + if f.wait != nil { + if err := f.wait(ctx, prompt); err != nil { + return nil, err + } + } + if f.reply == nil { + return nil, errors.New("no reply") + } + return f.reply(n, name, args, stdin) +} + +func (f *fakeRunner) callList() []fakeCall { + f.mu.Lock() + defer f.mu.Unlock() + return append([]fakeCall(nil), f.calls...) +} + +var bg = context.Background() + +func TestModelID(t *testing.T) { + cases := map[string]string{"opus": "opus", " astra ": "gpt-6-astra", "GROK": "grok-4.7"} + for in, want := range cases { + got, err := ModelID(in) + if err != nil || got != want { + t.Fatalf("%q -> %q %v", in, got, err) + } + } + if _, err := ModelID("sonnet"); err == nil { + t.Fatal("expected unknown model") + } +} + +func TestCallShapes(t *testing.T) { + prompt := []byte("review this") + for _, model := range []string{"opus", "astra", "grok"} { + var schemaFile string + fr := &fakeRunner{reply: func(_ int, _ string, args []string, _ []byte) ([]byte, error) { + switch model { + case "opus": + return []byte(`{"is_error":false,"result":"{\"findings\":[]}"}`), nil + case "astra": + // The schema file holds the shared schema during the call. + schemaFile = args[len(args)-2] + if got, err := os.ReadFile(schemaFile); err != nil || string(got) != findingsSchema { + t.Fatalf("codex schema file %s: %v %q", schemaFile, err, got) + } + return []byte("not json\n{\"type\":\"item.completed\",\"item\":{\"type\":\"agent_message\",\"text\":\"first\"}}\n{\"type\":\"item.completed\",\"item\":{\"type\":\"agent_message\",\"text\":\"{\\\"findings\\\":[]}\"}}\n"), nil + default: + return []byte(`{"text":"{\"findings\":[]}"}`), nil + } + }} + text, err := Call(bg, fr, model, prompt) + if err != nil { + t.Fatal(model, err) + } + if model == "astra" { + if text != `{"findings":[]}` { + t.Fatalf("astra text %q", text) + } + } else if text != `{"findings":[]}` { + t.Fatalf("%s text %q", model, text) + } + c := fr.calls[0] + switch model { + case "opus": + want := []string{"-p", "--model", "opus", "--output-format", "json", "--tools", "", "--system-prompt", sharedSystem, "--json-schema", findingsSchema, "--strict-mcp-config", "--no-session-persistence", "--effort", "medium"} + if c.name != "claude" || !reflectArgs(c.args, want) || !bytes.Equal(c.stdin, prompt) { + t.Fatalf("claude call %+v", c) + } + case "astra": + want := []string{"exec", "--json", "-m", "gpt-6-astra", "-s", "read-only", "--ephemeral", "--skip-git-repo-check", "--output-schema", schemaFile, "-"} + if c.name != "codex" || !reflectArgs(c.args, want) || !bytes.Equal(c.stdin, prompt) { + t.Fatalf("codex call %+v", c) + } + if filepath.Dir(schemaFile) != filepath.Clean(os.TempDir()) { + t.Fatalf("schema file %s not in os.TempDir()", schemaFile) + } + if _, err := os.Stat(schemaFile); !os.IsNotExist(err) { + t.Fatalf("schema file %s left behind", schemaFile) + } + case "grok": + if c.name != "grok" || len(c.args) < 2 || !reflectArgs(c.args, grokWant(c.args[1])) || c.stdin != nil || !bytes.Equal(c.prompt, prompt) { + t.Fatalf("grok call %+v", c) + } + } + } +} + +// sharedSystem is the system prompt every model gets, spelled out so a change +// to reviewSystem is a visible test change. +const sharedSystem = "You are a code reviewer. You have no tools. Everything you need is in the user message. Answer in a single message." + +// grokWant is the exact Grok argv the goal names, for a given prompt file. +func grokWant(promptFile string) []string { + return []string{ + "--prompt-file", promptFile, "--verbatim", + "-m", "grok-4.7", "--output-format", "json", + "--json-schema", findingsSchema, + "--tools", "read_file", "--disallowed-tools", "read_file,search_tool,use_tool", + "--max-turns", "1", "--no-subagents", "--disable-web-search", + "--system-prompt-override", sharedSystem, + "--reasoning-effort", "medium", + } +} + +func TestGrokPromptFile(t *testing.T) { + prompt := []byte("review this ✓\n") + tree, out := t.TempDir(), t.TempDir() + replies := map[string]func() ([]byte, error){ + "success": func() ([]byte, error) { return []byte(`{"text":"{\"findings\":[]}"}`), nil }, + "nonzero": func() ([]byte, error) { return nil, errors.New("grok: exit status 1") }, + "cannot-start": func() ([]byte, error) { return nil, errors.New(`exec: "grok": executable file not found in $PATH`) }, + } + for name, reply := range replies { + var path string + fr := &fakeRunner{reply: func(_ int, _ string, args []string, _ []byte) ([]byte, error) { + path = args[1] + got, err := os.ReadFile(path) + if err != nil || !bytes.Equal(got, prompt) { + t.Fatalf("%s: prompt file during the call: %v %q", name, err, got) + } + for _, dir := range []string{tree, out} { + if strings.HasPrefix(path, dir) { + t.Fatalf("%s: prompt file %s inside %s", name, path, dir) + } + } + if filepath.Dir(path) != filepath.Clean(os.TempDir()) { + t.Fatalf("%s: prompt file %s not in os.TempDir()", name, path) + } + return reply() + }} + _, err := Call(bg, fr, "grok", prompt) + if (name == "success") != (err == nil) { + t.Fatalf("%s: err %v", name, err) + } + if _, statErr := os.Stat(path); !os.IsNotExist(statErr) { + t.Fatalf("%s: prompt file %s left behind", name, path) + } + } + // A real process that cannot start also removes the file. + before, _ := filepath.Glob(filepath.Join(os.TempDir(), "metareview-grok-prompt-*")) + t.Setenv("PATH", t.TempDir()) + if _, err := Call(bg, OSRunner{}, "grok", prompt); err == nil { + t.Fatal("grok should not start with an empty PATH") + } + after, _ := filepath.Glob(filepath.Join(os.TempDir(), "metareview-grok-prompt-*")) + if len(after) > len(before) { + t.Fatalf("prompt file left behind: %v", after) + } +} + +func TestFindingsSchema(t *testing.T) { + var s struct { + Type string `json:"type"` + Required []string + AdditionalProperties *bool `json:"additionalProperties"` + Properties struct { + Findings struct { + Type string `json:"type"` + Items struct { + Required []string `json:"required"` + AdditionalProperties *bool `json:"additionalProperties"` + Properties map[string]json.RawMessage `json:"properties"` + } `json:"items"` + } `json:"findings"` + } `json:"properties"` + } + if err := json.Unmarshal([]byte(findingsSchema), &s); err != nil { + t.Fatal(err) + } + // Codex's strict mode rejects any object that allows extra properties. + if s.AdditionalProperties == nil || *s.AdditionalProperties || s.Properties.Findings.Items.AdditionalProperties == nil || *s.Properties.Findings.Items.AdditionalProperties { + t.Fatal("every object must set additionalProperties to false") + } + want := []string{"tag", "file", "start_line", "end_line", "issue", "consequence", "confidence", "severity"} + if !reflectArgs(s.Properties.Findings.Items.Required, want) || s.Properties.Findings.Type != "array" { + t.Fatalf("schema required %v", s.Properties.Findings.Items.Required) + } + for _, f := range want { + if _, ok := s.Properties.Findings.Items.Properties[f]; !ok { + t.Fatalf("schema has no property %s", f) + } + } + for f, typ := range map[string]string{"start_line": "integer", "end_line": "integer", "confidence": "integer"} { + if !strings.Contains(string(s.Properties.Findings.Items.Properties[f]), `"`+typ+`"`) { + t.Fatalf("%s is not %s", f, typ) + } + } + for f, enum := range map[string]string{"tag": `["bug","advisory"]`, "severity": `["P0","P1","P2","P3"]`} { + if !strings.Contains(string(s.Properties.Findings.Items.Properties[f]), enum) { + t.Fatalf("%s enum", f) + } + } +} + +func TestWritePromptFileErrors(t *testing.T) { + prevCreate, prevRemove := createTemp, removeFile + t.Cleanup(func() { createTemp, removeFile = prevCreate, prevRemove }) + createTemp = func(string, string) (*os.File, error) { return nil, errors.New("create") } + for _, model := range []string{"grok", "astra"} { + fr := &fakeRunner{} + if _, err := Call(bg, fr, model, []byte("p")); err == nil || !strings.Contains(err.Error(), "create") || len(fr.callList()) != 0 { + t.Fatalf("%s: %v", model, err) + } + } + // A file that is already closed fails Write; the partial file is removed. + var made string + createTemp = func(dir, pattern string) (*os.File, error) { + f, err := prevCreate(dir, pattern) + if err != nil { + return nil, err + } + made = f.Name() + _ = f.Close() + return f, nil + } + fr := &fakeRunner{} + if _, err := Call(bg, fr, "grok", []byte("p")); err == nil { + t.Fatal("write to a closed file") + } + if len(fr.callList()) != 0 { + t.Fatal("grok ran without a prompt file") + } + if _, err := os.Stat(made); !os.IsNotExist(err) { + t.Fatal("partial prompt file left behind") + } +} + +func reflectArgs(got, want []string) bool { + if len(got) != len(want) { + return false + } + for i := range got { + if got[i] != want[i] { + return false + } + } + return true +} + +func TestCallErrors(t *testing.T) { + fr := &fakeRunner{reply: func(int, string, []string, []byte) ([]byte, error) { + return nil, errors.New("executable file not found") + }} + if _, err := Call(bg, fr, "opus", []byte("p")); err == nil { + t.Fatal("start failure") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte(`{"is_error":true,"result":"boom"}`), nil + } + if _, err := Call(bg, fr, "opus", []byte("p")); err == nil || !strings.Contains(err.Error(), "is_error") { + t.Fatal(err) + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte(`{"is_error":false}`), nil + } + if _, err := Call(bg, fr, "opus", []byte("p")); err == nil { + t.Fatal("missing result") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte(`{"is_error":false,"result":""}`), nil + } + if _, err := Call(bg, fr, "opus", []byte("p")); err == nil { + t.Fatal("empty result") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte("not-json"), nil + } + if _, err := Call(bg, fr, "opus", []byte("p")); err == nil { + t.Fatal("bad claude json") + } + if _, err := Call(bg, fr, "grok", []byte("p")); err == nil { + t.Fatal("bad grok json") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte(`{"text":""}`), nil + } + if _, err := Call(bg, fr, "grok", []byte("p")); err == nil { + t.Fatal("empty grok text") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte(`{}`), nil + } + if _, err := Call(bg, fr, "grok", []byte("p")); err == nil { + t.Fatal("nil grok text") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte("{\"type\":\"item.completed\",\"item\":{\"type\":\"agent_message\",\"text\":\"\"}}\n"), nil + } + if _, err := Call(bg, fr, "astra", []byte("p")); err == nil { + t.Fatal("empty codex text") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return []byte("nope\n"), nil + } + if _, err := Call(bg, fr, "astra", []byte("p")); err == nil { + t.Fatal("no agent message") + } + if _, err := Call(bg, fr, "nope", []byte("p")); err == nil { + t.Fatal("unknown model") + } + fr.reply = func(int, string, []string, []byte) ([]byte, error) { + return nil, errors.New("exit status 1") + } + if _, err := Call(bg, fr, "astra", []byte("p")); err == nil { + t.Fatal("nonzero") + } + if _, err := Call(bg, fr, "grok", []byte("p")); err == nil { + t.Fatal("grok nonzero") + } +} + +func TestModelTextRejectsIncompleteAnswer(t *testing.T) { + for _, c := range []struct { + model string + raw string + }{ + {"opus", `{"stop_reason":"max_tokens","result":"{\"findings\":[]}"}`}, + {"grok", `{"stopReason":"max_tokens","text":"{\"findings\":[]}"}`}, + {"grok", `{"structuredOutputError":"model did not produce structured output","text":"{\"findings\":[]}"}`}, + } { + t.Run(c.model+c.raw, func(t *testing.T) { + fr := &fakeRunner{reply: func(int, string, []string, []byte) ([]byte, error) { + return []byte(c.raw), nil + }} + if text, err := Call(bg, fr, c.model, []byte("review source")); err == nil || text != "" { + t.Fatalf("incomplete response accepted: text=%q err=%v", text, err) + } + }) + } +} + +func TestOSRunner(t *testing.T) { + out, err := OSRunner{}.Run(bg, "echo", []string{"hi"}, nil) + if err != nil || !bytes.Contains(out, []byte("hi")) { + t.Fatalf("echo: %v %q", err, out) + } + out, err = OSRunner{}.Run(bg, "cat", nil, []byte("yo")) + if err != nil || !bytes.Equal(out, []byte("yo")) { + t.Fatalf("cat: %v %q", err, out) + } + if _, err := (OSRunner{}).Run(bg, "false", nil, nil); err == nil { + t.Fatal("false should fail") + } + if _, err := (OSRunner{}).Run(bg, "sh", []string{"-c", "echo boom >&2; exit 1"}, nil); err == nil || !strings.Contains(err.Error(), "boom") { + t.Fatal(err) + } + if _, err := (OSRunner{}).Run(bg, "sh", []string{"-c", "echo usage-limit; exit 1"}, nil); err == nil || !strings.Contains(err.Error(), "usage-limit") { + t.Fatal(err) + } + if _, err := (OSRunner{}).Run(bg, "sh", []string{"-c", "echo out-line; echo err-line >&2; exit 1"}, nil); err == nil || !strings.Contains(err.Error(), "err-line") || !strings.Contains(err.Error(), "out-line") { + t.Fatal(err) + } + if _, err := (OSRunner{}).Run(bg, "no-such-binary-xyz", nil, nil); err == nil { + t.Fatal("missing binary") + } + // exec.Command is used; a lookup failure is *exec.Error, not ExitError. + if _, err := exec.LookPath("echo"); err != nil { + t.Fatal(err) + } +} + +func TestOSRunnerIsolatesWorkspace(t *testing.T) { + t.Setenv("GROK_CLAUDE_SKILLS_ENABLED", "true") + t.Setenv("CLAUDE_CODE_SAFE_MODE", "0") + t.Setenv("CLAUDE_CODE_EFFORT_LEVEL", "high") + t.Setenv("GROK_CONFIG", `{"models":{"default":"a-different-model"}}`) + for _, exit := range []string{"0", "1"} { + out, err := OSRunner{}.Run(bg, "sh", []string{"-c", `pwd; printf '%s\n' "$GROK_CLAUDE_SKILLS_ENABLED" "$CLAUDE_CODE_SAFE_MODE" "$GROK_CONFIG" "$CLAUDE_CODE_EFFORT_LEVEL"; exit "$1"`, "sh", exit}, nil) + if (err == nil) != (exit == "0") { + t.Fatalf("exit %s: %v", exit, err) + } + lines := strings.Split(strings.TrimSpace(string(out)), "\n") + if len(lines) != 5 || !strings.HasPrefix(filepath.Base(lines[0]), "metareview-workspace-") || lines[1] != "false" || lines[2] != "1" || lines[3] != `{"models":{"default":"grok-4.7"}}` || lines[4] != "medium" { + t.Fatalf("unexpected workspace or environment: %q", out) + } + if _, err := os.Stat(lines[0]); !os.IsNotExist(err) { + t.Fatalf("workspace remains after exit %s: %v", exit, err) + } + } +} + +func TestOSRunnerWorkspaceFailure(t *testing.T) { + t.Setenv("TMPDIR", filepath.Join(t.TempDir(), "missing")) + if _, err := (OSRunner{}).Run(bg, "echo", []string{"not started"}, nil); err == nil { + t.Fatal("workspace creation failure must fail the call") + } +} + +func TestOSRunnerKillsProcessGroup(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "child.pid") + ctx, cancel := context.WithTimeout(bg, 300*time.Millisecond) + defer cancel() + start := time.Now() + _, err := OSRunner{}.Run(ctx, "sh", []string{"-c", `sleep 30 & echo $! > "$0"; wait`, pidFile}, nil) + if err == nil { + t.Fatal("expected a killed process") + } + if took := time.Since(start); took > 10*time.Second { + t.Fatalf("kill took %s", took) + } + raw, rerr := os.ReadFile(pidFile) + if rerr != nil { + t.Fatal(rerr) + } + pid, perr := strconv.Atoi(strings.TrimSpace(string(raw))) + if perr != nil { + t.Fatal(perr) + } + deadline := time.Now().Add(5 * time.Second) + for exec.Command("kill", "-0", strconv.Itoa(pid)).Run() == nil { + if time.Now().After(deadline) { + t.Fatalf("child %d still running", pid) + } + time.Sleep(50 * time.Millisecond) + } +} + +func TestClaudeIsErrorWithoutResult(t *testing.T) { + if _, err := claudeModelText([]byte(`{"is_error":true}`)); err == nil { + t.Fatal("expected is_error") + } +} + +func TestCodexSkipsBlank(t *testing.T) { + raw := "\n \n{\"type\":\"turn.completed\"}\n{\"type\":\"item.completed\",\"item\":{\"type\":\"agent_message\",\"text\":\"{\\\"findings\\\":[]}\"}}\n" + text, err := codexModelText([]byte(raw)) + if err != nil || text != `{"findings":[]}` { + t.Fatalf("%q %v", text, err) + } + b, err := json.Marshal(map[string]string{"text": "ok"}) + if err != nil || !bytes.Contains(b, []byte("ok")) { + t.Fatal(err) + } +} diff --git a/internal/sourcereview/output.go b/internal/sourcereview/output.go new file mode 100644 index 00000000..69c98de1 --- /dev/null +++ b/internal/sourcereview/output.go @@ -0,0 +1,115 @@ +package sourcereview + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" +) + +// reportLstat is a seam for filesystem metadata failures. +var reportLstat = os.Lstat + +// publishReports stages both files on the destination filesystem before changing +// either saved report. Publication errors restore the previous pair. Renaming +// two files is not an atomic transaction for concurrent readers or process crashes. +func publishReports(opts Options, findings, page []byte) error { + ctx := opts.Context + if ctx == nil { + ctx = context.Background() + } + write, rename, remove := opts.WriteFile, opts.Rename, opts.Remove + if write == nil { + write = os.WriteFile + } + if rename == nil { + rename = os.Rename + } + if remove == nil { + remove = os.Remove + } + stage, err := os.MkdirTemp(opts.OutputDir, ".metareview-publish-*") + if err != nil { + return err + } + cleanup := true + defer func() { + if cleanup { + _ = os.RemoveAll(stage) + } + }() + type report struct { + name string + body []byte + exists bool + backedUp bool + published bool + } + reports := []report{{name: "findings.json", body: findings}, {name: "review.html", body: page}} + for i := range reports { + if err := ctx.Err(); err != nil { + return err + } + r := &reports[i] + if err := write(filepath.Join(stage, r.name), r.body, 0o644); err != nil { + return err + } + info, err := reportLstat(filepath.Join(opts.OutputDir, r.name)) + if err != nil && !os.IsNotExist(err) { + return err + } + r.exists = err == nil + if r.exists && info.IsDir() { + return fmt.Errorf("report destination is a directory: %s", r.name) + } + } + rollback := func(cause error) error { + var restoreErrors []error + for i := len(reports) - 1; i >= 0; i-- { + r := &reports[i] + dest := filepath.Join(opts.OutputDir, r.name) + if r.backedUp { + if err := rename(filepath.Join(stage, r.name+".previous"), dest); err != nil { + restoreErrors = append(restoreErrors, err) + } + } else if r.published { + if err := remove(dest); err != nil { + restoreErrors = append(restoreErrors, err) + } + } + } + if len(restoreErrors) > 0 { + // Never delete the only surviving copy if restoration itself fails. + cleanup = false + return errors.Join(cause, fmt.Errorf("restore reports (backups retained in %s): %w", stage, errors.Join(restoreErrors...))) + } + return cause + } + for i := range reports { + if err := ctx.Err(); err != nil { + return rollback(err) + } + r := &reports[i] + if r.exists { + if err := rename(filepath.Join(opts.OutputDir, r.name), filepath.Join(stage, r.name+".previous")); err != nil { + return rollback(err) + } + r.backedUp = true + } + } + for i := range reports { + if err := ctx.Err(); err != nil { + return rollback(err) + } + r := &reports[i] + if err := rename(filepath.Join(stage, r.name), filepath.Join(opts.OutputDir, r.name)); err != nil { + return rollback(err) + } + r.published = true + } + if err := ctx.Err(); err != nil { + return rollback(err) + } + return nil +} diff --git a/internal/sourcereview/output_test.go b/internal/sourcereview/output_test.go new file mode 100644 index 00000000..2ee6f0eb --- /dev/null +++ b/internal/sourcereview/output_test.go @@ -0,0 +1,220 @@ +package sourcereview + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestReportPublicationCancellationPreservesPreviousPair(t *testing.T) { + for cancelAfter := 0; cancelAfter <= 6; cancelAfter++ { + t.Run(fmt.Sprintf("after-operation-%d", cancelAfter), func(t *testing.T) { + dir := t.TempDir() + names := []string{"findings.json", "review.html"} + seedReports(t, dir, names) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + n := 0 + completed := func() { + n++ + if n == cancelAfter { + cancel() + } + } + if cancelAfter == 0 { + cancel() + } + err := publishReports(Options{Context: ctx, OutputDir: dir, + WriteFile: func(p string, body []byte, mode os.FileMode) error { + if err := os.WriteFile(p, body, mode); err != nil { + return err + } + completed() + return nil + }, Rename: func(from, to string) error { + if err := os.Rename(from, to); err != nil { + return err + } + completed() + return nil + }}, []byte("new findings"), []byte("new page")) + if !errors.Is(err, context.Canceled) { + t.Fatal(err) + } + checkOldReports(t, dir, names) + }) + } +} + +func TestRunCancellationDuringReportSetup(t *testing.T) { + root, _ := newRepoBytes(t, droppedOnlyFiles()) + dir := t.TempDir() + names := []string{"findings.json", "review.html"} + seedReports(t, dir, names) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + err := Run(Options{Context: ctx, Repo: root, Model: "opus", OutputDir: dir, + MkdirAll: func(p string, mode os.FileMode) error { cancel(); return os.MkdirAll(p, mode) }}) + if !errors.Is(err, context.Canceled) { + t.Fatal(err) + } + checkOldReports(t, dir, names) +} + +var oldReports = map[string]string{"findings.json": "old findings", "review.html": "old page"} + +func seedReports(t *testing.T, dir string, names []string) { + t.Helper() + for _, name := range names { + if err := os.WriteFile(filepath.Join(dir, name), []byte(oldReports[name]), 0o600); err != nil { + t.Fatal(err) + } + } +} + +func checkOldReports(t *testing.T, dir string, names []string) { + t.Helper() + for _, name := range names { + body, err := os.ReadFile(filepath.Join(dir, name)) + if err != nil || string(body) != oldReports[name] { + t.Fatalf("lost previous %s: %q %v", name, body, err) + } + } + entries, err := os.ReadDir(dir) + if err != nil || len(entries) != len(names) { + t.Fatalf("extra or missing files after failure: %v %v", entries, err) + } +} + +func TestReportPublicationFailuresPreservePreviousResults(t *testing.T) { + for _, names := range [][]string{{"findings.json", "review.html"}, {"findings.json"}, {"review.html"}, nil} { + for failWrite := 1; failWrite <= 2; failWrite++ { + t.Run(fmt.Sprintf("partial-write-%d-existing-%d-%v", failWrite, len(names), names), func(t *testing.T) { + dir := t.TempDir() + seedReports(t, dir, names) + n := 0 + failure := errors.New("no space left on device") + err := publishReports(Options{OutputDir: dir, WriteFile: func(p string, body []byte, mode os.FileMode) error { + n++ + if n == failWrite { + _ = os.WriteFile(p, []byte("partial write"), mode) + return failure + } + return os.WriteFile(p, body, mode) + }}, []byte("new findings"), []byte("new page")) + if !errors.Is(err, failure) { + t.Fatal(err) + } + checkOldReports(t, dir, names) + }) + } + for failRename := 1; failRename <= len(names)+2; failRename++ { + t.Run(fmt.Sprintf("rename-%d-existing-%d-%v", failRename, len(names), names), func(t *testing.T) { + dir := t.TempDir() + seedReports(t, dir, names) + n := 0 + failure := errors.New("rename failed") + err := publishReports(Options{OutputDir: dir, Rename: func(from, to string) error { + n++ + if n == failRename { + return failure + } + return os.Rename(from, to) + }, Remove: os.Remove}, []byte("new findings"), []byte("new page")) + if !errors.Is(err, failure) { + t.Fatal(err) + } + checkOldReports(t, dir, names) + }) + } + } +} + +func TestReportPublicationReplacesPreviousPair(t *testing.T) { + dir := t.TempDir() + seedReports(t, dir, []string{"findings.json", "review.html"}) + if err := publishReports(Options{OutputDir: dir}, []byte("new findings"), []byte("new page")); err != nil { + t.Fatal(err) + } + for name, want := range map[string]string{"findings.json": "new findings", "review.html": "new page"} { + body, err := os.ReadFile(filepath.Join(dir, name)) + if err != nil || string(body) != want { + t.Fatalf("publication failed: %q %v", body, err) + } + } + if entries, _ := os.ReadDir(dir); len(entries) != 2 { + t.Fatalf("left staging files: %v", entries) + } +} + +func TestReportPublicationKeepsBackupsWhenRestoreFails(t *testing.T) { + dir := t.TempDir() + seedReports(t, dir, []string{"findings.json", "review.html"}) + err := publishReports(Options{OutputDir: dir, Rename: func(from, to string) error { + if strings.HasSuffix(from, ".previous") { + return errors.New("restore failed") + } + if filepath.Dir(from) != dir && filepath.Base(from) == "review.html" { + return errors.New("publish failed") + } + return os.Rename(from, to) + }}, []byte("new findings"), []byte("new page")) + if err == nil || !strings.Contains(err.Error(), "backups retained") { + t.Fatal(err) + } + stages, _ := filepath.Glob(filepath.Join(dir, ".metareview-publish-*")) + if len(stages) != 1 { + t.Fatalf("lost backups: %v", stages) + } + for name, want := range oldReports { + body, err := os.ReadFile(filepath.Join(stages[0], name+".previous")) + if err != nil || string(body) != want { + t.Fatalf("backup %s lost: %q %v", name, body, err) + } + } +} + +func TestReportPublicationReportsCleanupFailure(t *testing.T) { + dir := t.TempDir() + err := publishReports(Options{OutputDir: dir, Rename: func(from, to string) error { + if filepath.Base(from) == "review.html" { + return errors.New("publish failed") + } + return os.Rename(from, to) + }, Remove: func(string) error { return errors.New("remove failed") }}, []byte("new findings"), []byte("new page")) + if err == nil || !strings.Contains(err.Error(), "remove failed") { + t.Fatal(err) + } +} + +func TestReportPublicationFilesystemErrors(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "file") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := publishReports(Options{OutputDir: file}, nil, nil); err == nil { + t.Fatal("staging under a file accepted") + } + seedReports(t, dir, []string{"findings.json"}) + if err := os.Mkdir(filepath.Join(dir, "review.html"), 0o700); err != nil { + t.Fatal(err) + } + if err := publishReports(Options{OutputDir: dir}, nil, nil); err == nil || !strings.Contains(err.Error(), "directory") { + t.Fatal(err) + } + body, err := os.ReadFile(filepath.Join(dir, "findings.json")) + if err != nil || string(body) != oldReports["findings.json"] { + t.Fatal("changed findings despite invalid page destination") + } + prev := reportLstat + reportLstat = func(string) (os.FileInfo, error) { return nil, errors.New("metadata failed") } + t.Cleanup(func() { reportLstat = prev }) + if err := publishReports(Options{OutputDir: dir}, nil, nil); err == nil || !strings.Contains(err.Error(), "metadata failed") { + t.Fatal(err) + } +} diff --git a/internal/sourcereview/pack.go b/internal/sourcereview/pack.go new file mode 100644 index 00000000..f630b503 --- /dev/null +++ b/internal/sourcereview/pack.go @@ -0,0 +1,226 @@ +package sourcereview + +import ( + "bytes" + "fmt" + "sort" + "strconv" + "strings" + "unicode/utf8" +) + +// MaxPromptBytes is the prompt budget. AGENTS.md uses 120000 as the size that +// cannot be held in one review context. +const MaxPromptBytes = 120000 + +// instruction is the same text for every model. Only the CLI and the model id change. +const instruction = `Review the first-party source below and return only {"findings":[...]}. +Each finding's tag is bug or advisory. Each finding's severity is P0, P1, P2, or P3. +Every field is present: tag, file, start_line, end_line, issue, consequence, confidence, severity. +confidence is an integer from 0 to 100. +file is one of the paths given below. start_line and end_line are line numbers in that file. +Return {"findings":[]} when there is nothing to report. +Do not write anything outside that JSON object. +Each source line below starts with its line number and "| ". That prefix is not part of the file. Cite those numbers.` + +// numberLines prefixes each line of body with its line number and "| ", +// counting from first. Given the numbers, a model does not have to count lines +// to cite them: on one hh file this cut Opus from 47 s to 29 s and Grok from +// 634 s to 540 s per prompt, with no fewer findings. +func numberLines(body []byte, first int) []byte { + var b bytes.Buffer + n := first + for _, line := range bytes.SplitAfter(body, []byte("\n")) { + if len(line) == 0 { + continue + } + fmt.Fprintf(&b, "%d| ", n) + b.Write(line) + n++ + } + return b.Bytes() +} + +func sliceNote(path string, line int) string { + return fmt.Sprintf("This prompt contains a byte slice of %s starting at original line %d. Citations use the original path and the original line numbers.", path, line) +} + +func sliceHeader(path string, line int) string { + return instruction + "\n" + sliceNote(path, line) + "\n" +} + +// promptWith writes the instruction header and each path on its own line +// immediately before that file's numbered lines. A following path stays on its +// own line when the previous body does not end in a newline. +func promptWith(header string, files []File) []byte { + return promptFrom(header, files, 1) +} + +// promptFrom is promptWith with the first file numbered from first: a slice of +// a large file is numbered from the original line that holds its first byte. +func promptFrom(header string, files []File, first int) []byte { + var b bytes.Buffer + b.WriteString(header) + if !strings.HasSuffix(header, "\n") { + b.WriteByte('\n') + } + for i, f := range files { + start := 1 + if i == 0 { + start = first + } + if i > 0 && (len(files[i-1].Body) == 0 || files[i-1].Body[len(files[i-1].Body)-1] != '\n') { + b.WriteByte('\n') + } + b.WriteString(f.Path) + b.WriteByte('\n') + b.Write(numberLines(f.Body, start)) + } + return b.Bytes() +} + +// Pack packs whole kept files into the smallest number of prompts that stay +// within budget (first-fit decreasing). A file that cannot fit in one prompt +// is cut into consecutive non-empty UTF-8 slices that partition it. +func Pack(files []File, budget int) ([][]byte, error) { + if budget <= 0 { + return nil, fmt.Errorf("prompt budget must be positive") + } + var whole []sizedFile + var big []File + for _, f := range files { + size := len(f.Path) + 1 + numberedSize(f.Body) + if len(instruction)+1+size <= budget { + whole = append(whole, sizedFile{File: f, size: size}) + } else { + big = append(big, f) + } + } + prompts := packWhole(whole, budget) + sort.Slice(big, func(i, j int) bool { return big[i].Path < big[j].Path }) + for _, f := range big { + parts, err := sliceFile(f, budget) + if err != nil { + return nil, err + } + prompts = append(prompts, parts...) + } + return prompts, nil +} + +type sizedFile struct { + File + size int +} + +// numberedSize measures the rendered body without building a prompt. +func numberedSize(body []byte) int { + size := len(body) + for line := 1; len(body) > 0; line++ { + size += len(strconv.Itoa(line)) + 2 + _, body, _ = bytes.Cut(body, []byte("\n")) + } + return size +} + +func packWhole(files []sizedFile, budget int) [][]byte { + sorted := append([]sizedFile(nil), files...) + sort.Slice(sorted, func(i, j int) bool { + if len(sorted[i].Body) == len(sorted[j].Body) { + return sorted[i].Path < sorted[j].Path + } + return len(sorted[i].Body) > len(sorted[j].Body) + }) + type bin struct { + files []File + size int + separator bool + } + var bins []bin + for _, f := range sorted { + placed := false + for i := range bins { + size := bins[i].size + f.size + if bins[i].separator { + size++ + } + if size <= budget { + bins[i].files = append(bins[i].files, f.File) + bins[i].size = size + bins[i].separator = len(f.Body) == 0 || f.Body[len(f.Body)-1] != '\n' + placed = true + break + } + } + if !placed { + bins = append(bins, bin{files: []File{f.File}, size: len(instruction) + 1 + f.size, + separator: len(f.Body) == 0 || f.Body[len(f.Body)-1] != '\n'}) + } + } + out := make([][]byte, 0, len(bins)) + for _, bin := range bins { + out = append(out, promptWith(instruction, bin.files)) + } + return out +} + +func sliceFile(f File, budget int) ([][]byte, error) { + var out [][]byte + start := 0 + for start < len(f.Body) { + line := lineAt(f.Body, start) + header := sliceHeader(f.Path, line) + room := budget - len(header) - len(f.Path) - 1 + end, err := cutNumbered(f.Body, start, room, line) + if err != nil { + return nil, fmt.Errorf("%s: %w", f.Path, err) + } + out = append(out, promptFrom(header, []File{{Path: f.Path, Body: f.Body[start:end]}}, line)) + start = end + } + return out, nil +} + +// cutNumbered is cutEnd for a slice whose lines are numbered from line: the +// numbered slice, not the raw bytes, must fit in room. While it does not, the +// raw cut shrinks in proportion to the overshoot (numbering can multiply the +// size of short lines). size > room, so each pass cuts strictly fewer bytes. +func cutNumbered(content []byte, start, room, line int) (int, error) { + raw := room + for { + end, err := cutEnd(content, start, raw) + if err != nil { + return 0, err + } + size := len(numberLines(content[start:end], line)) + if size <= room { + return end, nil + } + raw = (end - start) * room / size + } +} + +func lineAt(content []byte, offset int) int { + return bytes.Count(content[:offset], []byte("\n")) + 1 +} + +// cutEnd returns the byte end of a non-empty slice that starts at start, is at +// most room bytes, and ends on a UTF-8 character boundary. +func cutEnd(content []byte, start, room int) (int, error) { + if room < 1 { + return 0, fmt.Errorf("prompt overhead exceeds the budget") + } + end := start + room + // end == len(content) takes the rest of the file; content[end] would be out of range. + if end >= len(content) { + end = len(content) + } else { + for end > start && !utf8.RuneStart(content[end]) { + end-- + } + } + if end == start { + return 0, fmt.Errorf("a character does not fit in one prompt") + } + return end, nil +} diff --git a/internal/sourcereview/pack_test.go b/internal/sourcereview/pack_test.go new file mode 100644 index 00000000..1c26b300 --- /dev/null +++ b/internal/sourcereview/pack_test.go @@ -0,0 +1,263 @@ +package sourcereview + +import ( + "bytes" + "regexp" + "strconv" + "strings" + "testing" + "unicode/utf8" +) + +func TestPackWholeFilesSmallest(t *testing.T) { + files := []File{ + {Path: "a.go", Body: bytes.Repeat([]byte("x"), 100000)}, + {Path: "b.go", Body: bytes.Repeat([]byte("y"), 50000)}, + {Path: "c.go", Body: bytes.Repeat([]byte("z"), 60000)}, + } + prompts, err := Pack(files, MaxPromptBytes) + if err != nil { + t.Fatal(err) + } + if len(prompts) != 2 { + t.Fatalf("prompts = %d, want 2", len(prompts)) + } + seen := map[string]int{} + for _, p := range prompts { + if len(p) > MaxPromptBytes { + t.Fatalf("prompt is %d bytes", len(p)) + } + if !bytes.Contains(p, []byte(instruction)) { + t.Fatal("missing instruction") + } + if bytes.Contains(p, []byte("byte slice of")) { + t.Fatal("whole file prompt has a slice note") + } + for _, f := range files { + needle := append([]byte(f.Path+"\n"), numberLines(f.Body, 1)...) + if bytes.Contains(p, needle) { + seen[f.Path]++ + } + } + } + for _, f := range files { + if seen[f.Path] != 1 { + t.Fatalf("%s seen %d times", f.Path, seen[f.Path]) + } + } +} + +func TestPackSlicesPartitionUTF8(t *testing.T) { + body := bytes.Repeat([]byte("世"), 70000) + f := File{Path: "pkg/wide.go", Body: body} + prompts, err := Pack([]File{f}, MaxPromptBytes) + if err != nil { + t.Fatal(err) + } + if len(prompts) < 2 { + t.Fatalf("prompts = %d", len(prompts)) + } + off := 0 + files := map[string][]byte{f.Path: body} + for _, p := range prompts { + if len(p) > MaxPromptBytes { + t.Fatalf("prompt is %d bytes", len(p)) + } + header := sliceHeader(f.Path, lineAt(body, off)) + prefix := header + f.Path + "\n" + if !strings.HasPrefix(string(p), prefix) { + t.Fatalf("prompt missing slice header at offset %d", off) + } + // The body has no newlines, so each slice is one numbered line: the + // original line that holds the slice's first byte. + numbered := p[len(prefix):] + mark := strconv.Itoa(lineAt(body, off)) + "| " + if !bytes.HasPrefix(numbered, []byte(mark)) { + t.Fatalf("slice at offset %d is not numbered %q", off, mark) + } + part := numbered[len(mark):] + if len(part) == 0 { + t.Fatal("empty slice") + } + if !utf8.Valid(part) { + t.Fatal("slice is not valid UTF-8") + } + if !bytes.HasPrefix(body[off:], part) { + t.Fatal("slice is not the next bytes of the file") + } + line := lineAt(body, off) + text := `{"findings":[{"tag":"bug","file":"pkg/wide.go","start_line":` + strconv.Itoa(line) + `,"end_line":` + strconv.Itoa(line) + `,"issue":"i","consequence":"c","confidence":80,"severity":"P1"}]}` + got, _, err := Parse(text, files) + if err != nil { + t.Fatalf("finding at original line %d: %v", line, err) + } + if got[0].File != f.Path || got[0].StartLine != line { + t.Fatalf("citation %+v", got[0]) + } + off += len(part) + } + if off != len(body) { + t.Fatalf("partition covered %d of %d", off, len(body)) + } +} + +func TestPackMixedAndSeparators(t *testing.T) { + small := File{Path: "small.go", Body: []byte("package small\n")} + empty := File{Path: "empty.go", Body: nil} + nonl := File{Path: "nonl.go", Body: []byte("package nonl")} + big := File{Path: "big.go", Body: bytes.Repeat([]byte("世"), 70000)} + prompts, err := Pack([]File{small, empty, nonl, big}, MaxPromptBytes) + if err != nil { + t.Fatal(err) + } + var whole, slices int + for _, p := range prompts { + if bytes.Contains(p, []byte("byte slice of")) { + slices++ + if !bytes.Contains(p, []byte("big.go\n")) { + t.Fatal("slice prompt missing path") + } + continue + } + whole++ + for _, f := range []File{small, empty, nonl} { + if !bytes.Contains(p, []byte(f.Path+"\n")) { + t.Fatalf("whole prompt missing %s", f.Path) + } + } + if bytes.Contains(p, []byte("big.go\n")) { + t.Fatal("big file packed whole") + } + } + if whole != 1 || slices < 2 { + t.Fatalf("whole=%d slices=%d", whole, slices) + } +} + +func TestCutEndAndSliceErrors(t *testing.T) { + if _, err := cutEnd([]byte("世"), 0, 0); err == nil { + t.Fatal("room 0") + } + if _, err := cutEnd([]byte("世"), 0, 1); err == nil { + t.Fatal("rune does not fit") + } + end, err := cutEnd([]byte("世世"), 0, 4) + if err != nil || end != 3 { + t.Fatalf("end=%d err=%v", end, err) + } + end, err = cutEnd([]byte("ab"), 0, 10) + if err != nil || end != 2 { + t.Fatalf("short end=%d err=%v", end, err) + } + // Room exactly equal to the bytes left takes the rest without reading past the end. + end, err = cutEnd([]byte("abc"), 1, 2) + if err != nil || end != 3 { + t.Fatalf("exact end=%d err=%v", end, err) + } + header := sliceHeader("p.go", 1) + if _, err := sliceFile(File{Path: "p.go", Body: []byte("世")}, len(header)+len("p.go")+1+1); err == nil { + t.Fatal("sliceFile should reject a rune that does not fit") + } + if _, err := sliceFile(File{Path: "p.go", Body: []byte("hello")}, 10); err == nil { + t.Fatal("sliceFile should reject a tiny budget") + } + if _, err := Pack([]File{{Path: "a.go", Body: []byte("package a\n")}}, 0); err == nil { + t.Fatal("budget 0") + } + if _, err := Pack([]File{{Path: "a.go", Body: bytes.Repeat([]byte("x"), 50)}}, 10); err == nil { + t.Fatal("pack should surface the slice error") + } + if prompts, err := Pack(nil, MaxPromptBytes); err != nil || len(prompts) != 0 { + t.Fatalf("empty pack %v %v", prompts, err) + } + two, err := Pack([]File{ + {Path: "z.go", Body: bytes.Repeat([]byte("世"), 70000)}, + {Path: "a.go", Body: bytes.Repeat([]byte("世"), 70000)}, + }, MaxPromptBytes) + if err != nil || len(two) < 2 { + t.Fatalf("two big files: %v %d", err, len(two)) + } +} + +func TestPromptNamesPathBeforeBody(t *testing.T) { + body := []byte("package pkg\nfunc A() {}\n") + p := promptWith(instruction, []File{{Path: "pkg/app.go", Body: body}}) + if !bytes.Contains(p, []byte("pkg/app.go\n1| package pkg\n2| func A() {}\n")) { + t.Fatalf("path is not immediately before the numbered body:\n%s", p) + } + if !bytes.Contains(p, []byte(`{"findings":[...]}`)) || !bytes.Contains(p, []byte("Cite those numbers.")) { + t.Fatal("instruction missing findings object or line-number note") + } +} + +func TestNumberLines(t *testing.T) { + cases := []struct { + body string + first int + want string + }{ + {"", 1, ""}, + {"a\n", 1, "1| a\n"}, + {"a\nb", 1, "1| a\n2| b"}, + {"a\n\nb\n", 7, "7| a\n8| \n9| b\n"}, + } + for _, c := range cases { + if got := string(numberLines([]byte(c.body), c.first)); got != c.want { + t.Fatalf("numberLines(%q, %d) = %q, want %q", c.body, c.first, got, c.want) + } + } +} + +func TestSliceNumbersFromOriginalLine(t *testing.T) { + // 30,000 two-byte lines do not fit one prompt, and numbering more than + // doubles their size, so each cut has to shrink to fit the numbered text. + var body bytes.Buffer + for i := 1; i <= 30000; i++ { + body.WriteString("x\n") + } + f := File{Path: "short.go", Body: body.Bytes()} + const budget = 20000 + prompts, err := sliceFile(f, budget) + if err != nil { + t.Fatal(err) + } + if len(prompts) < 2 { + t.Fatalf("prompts = %d", len(prompts)) + } + number := regexp.MustCompile(`(?m)^(\d+)\| `) + off := 0 + for _, p := range prompts { + if len(p) > budget { + t.Fatalf("prompt is %d bytes", len(p)) + } + numbered := p[bytes.Index(p, []byte("short.go\n"))+len("short.go\n"):] + first := number.FindSubmatch(numbered) + if first == nil || string(first[1]) != strconv.Itoa(lineAt(f.Body, off)) { + t.Fatalf("slice at offset %d starts at %q, want line %d", off, first, lineAt(f.Body, off)) + } + part := number.ReplaceAll(numbered, nil) + if !bytes.HasPrefix(f.Body[off:], part) { + t.Fatalf("slice at offset %d is not the next bytes of the file", off) + } + off += len(part) + } + if off != len(f.Body) { + t.Fatalf("partition covered %d of %d", off, len(f.Body)) + } +} + +func TestCutNumbered(t *testing.T) { + // Numbered, "a\nb\nc" is 14 bytes for a room of 5; the cut shrinks until + // the numbered text fits. + end, err := cutNumbered([]byte("a\nb\nc\n"), 0, 5, 1) + if err != nil || end != 1 { + t.Fatalf("end=%d err=%v", end, err) + } + end, err = cutNumbered([]byte("abcdef"), 0, 5, 1) + if err != nil || end != 2 { + t.Fatalf("end=%d err=%v", end, err) + } + if _, err := cutNumbered([]byte("abc"), 0, 3, 100000); err == nil { + t.Fatal("a number wider than the room cannot fit") + } +} diff --git a/internal/sourcereview/page.go b/internal/sourcereview/page.go new file mode 100644 index 00000000..2e3fc2fd --- /dev/null +++ b/internal/sourcereview/page.go @@ -0,0 +1,711 @@ +package sourcereview + +import ( + "fmt" + "html" + "path" + "sort" + "strings" + + "github.com/dsifry/metareview/internal/lensoutput" +) + +// Page is the input to the self-contained HTML file. +type Page struct { + Repo string + Commit string + ModelID string + Findings []lensoutput.TypedFinding + Files map[string][]byte +} + +var severities = []string{"P0", "P1", "P2", "P3"} + +func severityCounts(fs []lensoutput.TypedFinding) (p0, p1, p2, p3 int) { + for _, f := range fs { + switch f.Severity { + case "P0": + p0++ + case "P1": + p1++ + case "P2": + p2++ + case "P3": + p3++ + } + } + return p0, p1, p2, p3 +} + +func findingDirs(fs []lensoutput.TypedFinding) []string { + seen := map[string]bool{} + var dirs []string + for _, f := range fs { + d := path.Dir(f.File) + if !seen[d] { + seen[d] = true + dirs = append(dirs, d) + } + } + sort.Strings(dirs) + return dirs +} + +func excerpt(body []byte, start, end int) string { + lines := splitLines(body) + if start < 1 || end > len(lines) || end < start { + return "" + } + return strings.Join(lines[start-1:end], "\n") +} + +// area is the part of the tree a finding belongs to on the map: the first two +// segments of its directory ("app/models" for app/models/concerns/x.rb). +func area(file string) string { + parts := strings.Split(path.Dir(file), "/") + if len(parts) > 2 { + parts = parts[:2] + } + return strings.Join(parts, "/") +} + +// summary is the first sentence of an issue, for the collapsed row. The full +// issue is shown when the finding is opened. +func summary(issue string) string { + s := strings.TrimSpace(issue) + if i := strings.Index(s, ". "); i >= 0 { + s = s[:i+1] + } + return s +} + +func severityRank(s string) int { + for i, v := range severities { + if v == s { + return i + } + } + return len(severities) +} + +// fileGroup is every finding in one file, in the order the page shows them. +type fileGroup struct { + file string + findings []lensoutput.TypedFinding + worst int +} + +// groupByFile puts the files with the most severe findings first, then the +// files with the most findings. Inside a file, findings go by severity, then line. +func groupByFile(fs []lensoutput.TypedFinding) []fileGroup { + index := map[string]int{} + var groups []fileGroup + for _, f := range fs { + i, ok := index[f.File] + if !ok { + i = len(groups) + index[f.File] = i + groups = append(groups, fileGroup{file: f.File, worst: len(severities)}) + } + groups[i].findings = append(groups[i].findings, f) + if r := severityRank(f.Severity); r < groups[i].worst { + groups[i].worst = r + } + } + for _, g := range groups { + sort.SliceStable(g.findings, func(a, b int) bool { + ra, rb := severityRank(g.findings[a].Severity), severityRank(g.findings[b].Severity) + if ra != rb { + return ra < rb + } + return g.findings[a].StartLine < g.findings[b].StartLine + }) + } + sort.SliceStable(groups, func(a, b int) bool { + if groups[a].worst != groups[b].worst { + return groups[a].worst < groups[b].worst + } + if len(groups[a].findings) != len(groups[b].findings) { + return len(groups[a].findings) > len(groups[b].findings) + } + return groups[a].file < groups[b].file + }) + return groups +} + +// areaRow is one line of the map: an area and its findings' severities, worst first. +type areaRow struct { + name string + severities []string +} + +func areaRows(fs []lensoutput.TypedFinding) []areaRow { + index := map[string]int{} + var rows []areaRow + for _, f := range fs { + a := area(f.File) + i, ok := index[a] + if !ok { + i = len(rows) + index[a] = i + rows = append(rows, areaRow{name: a}) + } + rows[i].severities = append(rows[i].severities, f.Severity) + } + for _, r := range rows { + sort.SliceStable(r.severities, func(a, b int) bool { + return severityRank(r.severities[a]) < severityRank(r.severities[b]) + }) + } + sort.SliceStable(rows, func(a, b int) bool { + if len(rows[a].severities) != len(rows[b].severities) { + return len(rows[a].severities) > len(rows[b].severities) + } + return rows[a].name < rows[b].name + }) + return rows +} + +// thousands writes n with comma separators: 1346 -> "1,346". +func thousands(n int) string { + s := fmt.Sprint(n) + for i := len(s) - 3; i > 0; i -= 3 { + s = s[:i] + "," + s[i:] + } + return s +} + +// lede is the one sentence at the top of the page: what ran, what it found, +// and where to start. +func lede(model string, reviewed int, counts []int) string { + total := 0 + for _, c := range counts { + total += c + } + s := fmt.Sprintf("Reviewed %s with %s", plural(reviewed, "file", "files"), model) + if total == 0 { + return s + " and found nothing to report." + } + s += "." + worst := 0 + for counts[worst] == 0 { + worst++ + } + switch c := counts[worst]; { + case total == 1: + return s + fmt.Sprintf(" The one finding is a %s.", severities[worst]) + case c == total: + return s + fmt.Sprintf(" All %s findings are %s.", thousands(c), severities[worst]) + default: + return s + fmt.Sprintf(" Start with the %s at %s.", thousands(c), severities[worst]) + } +} + +func plural(n int, one, many string) string { + if n == 1 { + return "1 " + one + } + return thousands(n) + " " + many +} + +const pageScript = ` +(function () { + var severity = document.getElementById("severity"); + var directory = document.getElementById("directory"); + var search = document.getElementById("search"); + var shown = document.getElementById("shown"); + var findings = document.querySelectorAll("#findings .finding"); + var groups = document.querySelectorAll("#findings .group"); + var areas = document.querySelectorAll("#map .area"); + var segments = document.querySelectorAll("#bar .seg"); + var area = ""; + function press(list, attr, value) { + for (var i = 0; i < list.length; i++) { + list[i].setAttribute("aria-pressed", list[i].getAttribute(attr) === value ? "true" : "false"); + } + } + function apply() { + var s = severity.value; + var d = directory.value; + var q = search ? search.value.toLowerCase() : ""; + var n = 0; + for (var i = 0; i < findings.length; i++) { + var el = findings[i]; + var okS = s === "" || el.getAttribute("data-severity") === s; + var okD = d === "" || el.getAttribute("data-dir") === d; + var okA = area === "" || el.getAttribute("data-area") === area; + var file = el.getAttribute("data-file") || ""; + var okQ = q === "" || file.toLowerCase().indexOf(q) >= 0 || el.textContent.toLowerCase().indexOf(q) >= 0; + var ok = okS && okD && okA && okQ; + el.style.display = ok ? "" : "none"; + if (ok) n++; + } + for (var g = 0; g < groups.length; g++) { + var items = groups[g].querySelectorAll(".finding"); + var any = false; + for (var k = 0; k < items.length; k++) { + if (items[k].style.display !== "none") any = true; + } + groups[g].style.display = any ? "" : "none"; + } + press(segments, "data-severity", s); + press(areas, "data-area", area); + if (shown) { + shown.textContent = n === findings.length + ? "Showing all " + n + : "Showing " + n + " of " + findings.length; + } + } + severity.addEventListener("change", apply); + directory.addEventListener("change", apply); + if (search) search.addEventListener("input", apply); + for (var a = 0; a < segments.length; a++) { + segments[a].addEventListener("click", function () { + var v = this.getAttribute("data-severity"); + severity.value = severity.value === v ? "" : v; + apply(); + }); + } + for (var b = 0; b < areas.length; b++) { + areas[b].addEventListener("click", function () { + var v = this.getAttribute("data-area"); + area = area === v ? "" : v; + apply(); + }); + } + var buttons = document.querySelectorAll("#findings .finding > button"); + for (var j = 0; j < buttons.length; j++) { + buttons[j].addEventListener("click", function () { + var detail = this.parentNode.querySelector(".detail"); + detail.hidden = !detail.hidden; + this.setAttribute("aria-expanded", detail.hidden ? "false" : "true"); + }); + } +})(); +` + +const pageCSS = ` +:root { + color-scheme: light; + --ground: #ffffff; + --ink: #1f232b; + --soft: #5b6472; + --faint: #8b93a0; + --rule: #e3e6eb; + --well: #f4f5f7; + --hover: #f7f8fa; + --p0: #b3122e; + --p1: #d9480f; + --p2: #c08a1e; + --p3: #4a7fa3; + --empty: #e3e6eb; + --focus: #1f232b; + --display: "Avenir Next Condensed", "Avenir Next", "Helvetica Neue", Arial, sans-serif; + --ui: "Avenir Next", "Helvetica Neue", "Segoe UI", Arial, sans-serif; + --prose: Charter, "Iowan Old Style", "Sitka Text", Georgia, serif; + --code: "SF Mono", ui-monospace, Menlo, Consolas, monospace; +} +@media (prefers-color-scheme: dark) { + :root { + color-scheme: dark; + --ground: #16191e; + --ink: #e8eaee; + --soft: #a9b0bb; + --faint: #737b87; + --rule: #2b3038; + --well: #1e2228; + --hover: #1c2026; + --p0: #f0506b; + --p1: #f57c3a; + --p2: #e0b14a; + --p3: #74a9cf; + --empty: #2b3038; + --focus: #e8eaee; + } +} +* { box-sizing: border-box; } +html { background: var(--ground); } +body { + margin: 0; + padding: 3rem clamp(1rem, 3vw, 3.5rem) 5rem; + color: var(--ink); + font: 16px/1.5 var(--ui); + -webkit-font-smoothing: antialiased; +} +button, input, select { font: inherit; color: inherit; } +:focus-visible { outline: 2px solid var(--focus); outline-offset: 2px; } + +/* Masthead: the repo, what reviewed it, and how much it found. */ +.masthead { + display: grid; + grid-template-columns: minmax(0, 1fr) auto; + gap: 0.5rem 2rem; + align-items: end; +} +.masthead h1 { + margin: 0; + font-family: var(--display); + font-size: clamp(2.6rem, 6vw, 4.5rem); + font-weight: 700; + line-height: 0.95; + letter-spacing: -0.02em; +} +#repo { margin: 0.5rem 0 0; color: var(--soft); overflow-wrap: anywhere; } +.lede { + max-width: 34rem; + margin: 1.4rem 0 0; + font-family: var(--prose); + font-size: 1.3rem; + line-height: 1.4; +} +.facts { + display: grid; + grid-template-columns: auto minmax(0, 1fr); + gap: 0.15rem 1rem; + margin: 0; + font-size: 0.9rem; +} +.facts dt { color: var(--faint); } +.facts dd { margin: 0; font-variant-numeric: tabular-nums; } +#commit { font-family: var(--code); font-size: 0.82rem; overflow-wrap: anywhere; } + +/* The tally: one bar split by severity, with the four counts under it. */ +.tally { margin: 2.5rem 0 0; } +.total { + margin: 0 0 0.6rem; + font-family: var(--display); + font-size: 1.5rem; + font-weight: 600; +} +.total span { color: var(--soft); font-weight: 500; } +#bar { + display: flex; + gap: 3px; + height: 0.9rem; +} +#bar .seg { + min-width: 0.5rem; + padding: 0; + border: 0; + border-radius: 2px; + cursor: pointer; +} +#bar .seg[aria-pressed="true"] { box-shadow: 0 0 0 2px var(--ground), 0 0 0 4px var(--ink); } +#bar .seg:hover { filter: brightness(1.1); } +.counts { + display: flex; + flex-wrap: wrap; + gap: 0.3rem 1.8rem; + margin: 0.7rem 0 0; + padding: 0; + list-style: none; + font-size: 0.95rem; + font-variant-numeric: tabular-nums; +} +.counts li::before { + content: ""; + display: inline-block; + width: 0.65rem; + height: 0.65rem; + margin-right: 0.45rem; + border-radius: 2px; + background: var(--empty); +} +#count-p0::before, .sev-P0 { background: var(--p0); } +#count-p1::before, .sev-P1 { background: var(--p1); } +#count-p2::before, .sev-P2 { background: var(--p2); } +#count-p3::before, .sev-P3 { background: var(--p3); } + +/* The map: where in the tree the findings sit, one tick per finding. */ +.map-head { + margin: 3rem 0 0.25rem; + font-family: var(--display); + font-size: 1.5rem; + font-weight: 600; +} +.hint { margin: 0 0 1rem; color: var(--soft); font-size: 0.9rem; } +#map { + display: grid; + grid-template-columns: repeat(auto-fill, minmax(min(30rem, 100%), 1fr)); + gap: 0 2.5rem; + margin: 0; + padding: 0; + list-style: none; +} +#map .area { + display: grid; + grid-template-columns: 9.5rem minmax(0, 1fr) 2.2rem; + gap: 0.9rem; + align-items: center; + width: 100%; + padding: 0.3rem 0.4rem; + background: transparent; + border: 0; + border-radius: 3px; + cursor: pointer; + text-align: left; +} +#map .area:hover { background: var(--hover); } +#map .area[aria-pressed="true"] { background: var(--well); box-shadow: inset 3px 0 0 var(--ink); } +.area-name { + overflow: hidden; + font-family: var(--code); + font-size: 0.8rem; + text-overflow: ellipsis; + white-space: nowrap; +} +.track { display: block; } +.ticks { display: flex; gap: 1px; min-width: 4px; } +.tick { flex: 1 1 0; min-width: 1px; height: 1rem; border-radius: 1px; } +.area-count { color: var(--soft); font-size: 0.85rem; text-align: right; font-variant-numeric: tabular-nums; } + +/* Controls stay on screen while the list scrolls. */ +.controls { + position: sticky; + top: 0; + z-index: 2; + display: flex; + flex-wrap: wrap; + gap: 0.6rem 1.2rem; + align-items: center; + margin: 3rem 0 0; + padding: 0.9rem 0; + background: var(--ground); + border-bottom: 1px solid var(--rule); +} +.controls label { display: flex; gap: 0.45rem; align-items: center; color: var(--soft); font-size: 0.9rem; } +select, input[type="search"] { + padding: 0.35rem 0.55rem; + color: var(--ink); + background: var(--ground); + border: 1px solid var(--rule); + border-radius: 4px; +} +select { max-width: 17rem; } +input[type="search"] { width: 16rem; } +#shown { margin-left: auto; color: var(--soft); font-size: 0.9rem; font-variant-numeric: tabular-nums; } + +/* Findings, grouped by file. */ +#findings { margin-top: 0.5rem; } +.group { padding: 1.4rem 0 0.4rem; border-bottom: 1px solid var(--rule); } +.group-head { + display: flex; + gap: 1rem; + align-items: baseline; + justify-content: space-between; + margin: 0 0 0.35rem; + font-family: var(--code); + font-size: 0.86rem; + font-weight: 600; + overflow-wrap: anywhere; +} +.group-head .dir { color: var(--faint); font-weight: 400; } +.group-head .n { flex: none; color: var(--faint); font-family: var(--ui); font-weight: 400; } +article.finding { margin: 0; } +article.finding > button { + display: grid; + grid-template-columns: 2.6rem 5.2rem minmax(0, 1fr) auto; + gap: 0.9rem; + align-items: baseline; + width: 100%; + padding: 0.55rem 0.5rem; + background: transparent; + border: 0; + border-radius: 4px; + cursor: pointer; + text-align: left; +} +article.finding > button:hover { background: var(--hover); } +.badge { + justify-self: start; + padding: 0.05rem 0.4rem; + border-radius: 3px; + color: #fff; + font-size: 0.75rem; + font-weight: 700; + letter-spacing: 0.02em; +} +@media (prefers-color-scheme: dark) { .badge { color: #16191e; } } +.lines { color: var(--faint); font-family: var(--code); font-size: 0.8rem; font-variant-numeric: tabular-nums; } +.summary { font-family: var(--prose); font-size: 1rem; line-height: 1.45; overflow-wrap: anywhere; } +.consequence { overflow-wrap: anywhere; } +.tag { color: var(--faint); font-size: 0.8rem; } +.tag-bug { color: var(--soft); } +/* Open: the rest of the issue appears in place, and the impact and code sit under it. */ +.rest { display: none; } +article.finding > button[aria-expanded="true"] { background: var(--well); border-radius: 4px 4px 0 0; } +article.finding > button[aria-expanded="true"] .rest { display: inline; } +article.finding > button[aria-expanded="true"] .lead { font-weight: 600; } +.detail { display: none; } +.detail:not([hidden]) { + display: grid; + grid-template-columns: minmax(0, 36rem) minmax(0, 1fr); + gap: 2rem; + margin: 0 0 1rem; + padding: 0.6rem 0.5rem 1.2rem 10.1rem; + background: var(--well); + border-radius: 0 0 4px 4px; +} +.prose { font-family: var(--prose); max-width: 38rem; } +.consequence { margin: 0; color: var(--ink); line-height: 1.55; } +.consequence::before { content: "Impact: "; color: var(--ink); font-family: var(--ui); font-size: 0.85rem; font-weight: 600; } +.confidence { margin: 0.9rem 0 0; color: var(--faint); font-family: var(--ui); font-size: 0.82rem; } +.code { + display: flex; + max-height: 24rem; + overflow: auto; + background: var(--ground); + border: 1px solid var(--rule); + border-radius: 5px; + align-self: start; +} +.gutter, .quote { + margin: 0; + padding: 0.8rem 0.9rem; + font-family: var(--code); + font-size: 0.78rem; + line-height: 1.55; + white-space: pre; +} +.gutter { + position: sticky; + left: 0; + flex: none; + padding-right: 0.7rem; + color: var(--faint); + background: var(--ground); + border-right: 1px solid var(--rule); + text-align: right; + user-select: none; +} +.quote { flex: 1; color: var(--ink); } +.empty { + margin: 3rem 0; + font-family: var(--prose); + font-size: 1.15rem; + color: var(--soft); +} +@media (max-width: 760px) { + body { padding: 1.5rem 1rem 3rem; } + .masthead { grid-template-columns: 1fr; } + #map { grid-template-columns: 1fr; } + #map .area { grid-template-columns: 7rem minmax(0, 1fr) 2rem; } + article.finding > button { grid-template-columns: 2.6rem minmax(0, 1fr); } + .lines { grid-column: 2; grid-row: 2; } + .summary { grid-column: 2; grid-row: 1; } + .tag { display: none; } + .detail:not([hidden]) { grid-template-columns: 1fr; padding-left: 0.5rem; } + input[type="search"] { width: 100%; } + #shown { margin-left: 0; } +} +@media (prefers-reduced-motion: no-preference) { + article.finding > button, #map .area { transition: background-color 120ms ease; } +} +` + +// Render returns one HTML document. It has no external assets. The quote is the +// excerpt of the cited lines; it is not a field of the findings file. +func Render(p Page) []byte { + p0, p1, p2, p3 := severityCounts(p.Findings) + counts := []int{p0, p1, p2, p3} + total := len(p.Findings) + groups := groupByFile(p.Findings) + name := path.Base(p.Repo) + var b strings.Builder + fmt.Fprintf(&b, "\n\n\n\n\n%s source review\n\n\n\n
\n
\n") + fmt.Fprintf(&b, "

%s

\n", html.EscapeString(name)) + fmt.Fprintf(&b, "

%s

\n", html.EscapeString(p.Repo)) + fmt.Fprintf(&b, "

%s

\n", html.EscapeString(lede(p.ModelID, len(p.Files), counts))) + b.WriteString("
\n
\n") + fmt.Fprintf(&b, "
Model
%s
\n", html.EscapeString(p.ModelID)) + fmt.Fprintf(&b, "
Commit
%s
\n", html.EscapeString(p.Commit)) + fmt.Fprintf(&b, "
Reviewed
%s
\n", plural(len(p.Files), "file", "files")) + b.WriteString("
\n
\n") + + b.WriteString("
\n") + fmt.Fprintf(&b, "

%s in %s

\n", plural(total, "finding", "findings"), plural(len(groups), "file", "files")) + b.WriteString("
\n") + for i, s := range severities { + if counts[i] == 0 { + continue + } + fmt.Fprintf(&b, "\n", s, s, counts[i], s, s) + } + b.WriteString("
\n
    \n") + for i, s := range severities { + fmt.Fprintf(&b, "
  • %s %d
  • \n", strings.ToLower(s), s, counts[i]) + } + b.WriteString("
\n
\n") + + if total > 0 { + b.WriteString("
\n

Where the findings are

\n") + b.WriteString("

One mark per finding, worst first. Select an area to show only its findings.

\n
    \n") + rows := areaRows(p.Findings) + most := len(rows[0].severities) + for _, r := range rows { + fmt.Fprintf(&b, "
  • \n", len(r.severities)) + } + b.WriteString("
\n
\n") + } + + b.WriteString("
\n") + b.WriteString("\n") + b.WriteString("\n") + b.WriteString("\n") + fmt.Fprintf(&b, "

Showing all %d

\n", total) + b.WriteString("
\n
\n") + if total == 0 { + fmt.Fprintf(&b, "

No findings. %s reviewed %s and reported nothing to fix.

\n", html.EscapeString(p.ModelID), plural(len(p.Files), "file", "files")) + } + for _, g := range groups { + b.WriteString("
\n") + dirPart, base := path.Split(g.file) + fmt.Fprintf(&b, "

%s%s%s

\n", html.EscapeString(dirPart), html.EscapeString(base), plural(len(g.findings), "finding", "findings")) + for _, f := range g.findings { + dir := path.Dir(f.File) + quote := "" + if p.Files != nil { + quote = excerpt(p.Files[f.File], f.StartLine, f.EndLine) + } + lines := fmt.Sprintf("L%d", f.StartLine) + if f.EndLine != f.StartLine { + lines = fmt.Sprintf("L%d–%d", f.StartLine, f.EndLine) + } + tag := string(f.Tag) + fmt.Fprintf(&b, "
\n", html.EscapeString(f.File), html.EscapeString(f.Severity), html.EscapeString(dir), html.EscapeString(area(f.File))) + // The row shows the issue's first sentence; opening it reveals the rest in place. + issue := strings.TrimSpace(f.Issue) + lead := summary(issue) + fmt.Fprintf(&b, "\n", + html.EscapeString(f.Severity), html.EscapeString(f.Severity), lines, html.EscapeString(lead), html.EscapeString(issue[len(lead):]), html.EscapeString(tag), html.EscapeString(tag)) + b.WriteString("\n
\n") + } + b.WriteString("
\n") + } + b.WriteString("
\n\n\n\n") + return []byte(b.String()) +} diff --git a/internal/sourcereview/page_test.go b/internal/sourcereview/page_test.go new file mode 100644 index 00000000..bf82b92d --- /dev/null +++ b/internal/sourcereview/page_test.go @@ -0,0 +1,426 @@ +package sourcereview + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/dsifry/metareview/internal/lensoutput" +) + +func TestRenderFiltersAndQuote(t *testing.T) { + files := map[string][]byte{ + "main.go": []byte("package main\n"), + "pkg/app.go": []byte("alpha\nbeta\n"), + "a.go": []byte("a\n"), + "b/c.go": []byte("c\n"), + } + findings := []lensoutput.TypedFinding{ + {Tag: lensoutput.TagBug, File: "main.go", StartLine: 1, EndLine: 1, Issue: "root broke", Consequence: "no entry", Confidence: 90, Severity: "P0"}, + {Tag: lensoutput.TagAdvisory, File: "pkg/app.go", StartLine: 1, EndLine: 2, Issue: "alpha broke", Consequence: "wrong answer", Confidence: 70, Severity: "P1"}, + {Tag: lensoutput.TagBug, File: "a.go", StartLine: 1, EndLine: 1, Issue: "a", Consequence: "a", Confidence: 60, Severity: "P2"}, + {Tag: lensoutput.TagBug, File: "b/c.go", StartLine: 1, EndLine: 1, Issue: "c", Consequence: "c", Confidence: 50, Severity: "P3"}, + } + page := Render(Page{Repo: "/repo", Commit: "abc123", ModelID: "opus", Findings: findings, Files: files}) + html := string(page) + for _, want := range []string{ + `id="repo">/repo`, `id="commit">abc123`, `id="model">opus`, + `id="count-p0">P0 1`, `id="count-p1">P1 1`, `id="count-p2">P2 1`, `id="count-p3">P3 1`, + `id="severity"`, `id="directory"`, `value="."`, `value="pkg"`, + "root broke", "no entry", "package main", "alpha broke", "wrong answer", "alpha\nbeta", + } { + if !strings.Contains(html, want) { + t.Fatalf("page missing %q", want) + } + } + if strings.Contains(html, "http://") || strings.Contains(html, "https://") || strings.Contains(html, "fetch(") || strings.Contains(html, "`) { + t.Fatal("shipped script missing") + } + report := runPage(t, page) + if report.Counts["p0"] != "P0 1" || report.Counts["p3"] != "P3 1" { + t.Fatalf("counts %+v", report.Counts) + } + // The two findings the filter drives are the first two, which differ in + // severity and directory. The harness records what the shipped script did. + if len(report.Steps) < 2 { + t.Fatalf("steps %+v", report) + } + sev := report.Steps[0] + if sev.Displays[0] != "" || sev.Displays[1] != "none" { + t.Fatalf("severity filter displays %v", sev.Displays) + } + dir := report.Steps[1] + if dir.Displays[0] != "" || dir.Displays[1] != "none" { + t.Fatalf("directory filter displays %v", dir.Displays) + } + if report.Opened.Hidden || report.Opened.Expanded != "true" || report.Opened.Issue != "root broke" || report.Opened.Consequence != "no entry" || report.Opened.Quote != "package main" { + t.Fatalf("opened %+v", report.Opened) + } + if !report.Closed.Hidden || report.Closed.Expanded != "false" { + t.Fatalf("closed %+v", report.Closed) + } + all := []string{"", "", "", ""} + if report.Bar.Severity != "P0" || !reflectArgs(report.Bar.Displays, []string{"", "none", "none", "none"}) || report.Bar.Pressed != "true" || report.Bar.Shown != "Showing 1 of 4" { + t.Fatalf("bar %+v", report.Bar) + } + if report.BarCleared.Severity != "" || !reflectArgs(report.BarCleared.Displays, all) || report.BarCleared.Shown != "Showing all 4" { + t.Fatalf("bar cleared %+v", report.BarCleared) + } + // The biggest area is the repo root: main.go and a.go. + if report.Area.Name != "." || !reflectArgs(report.Area.Displays, []string{"", "none", "", "none"}) || !reflectArgs(report.Area.Groups, []string{"", "none", "", "none"}) || report.Area.Pressed != "true" { + t.Fatalf("area %+v", report.Area) + } + if !reflectArgs(report.AreaCleared.Displays, all) || report.AreaCleared.Pressed != "false" { + t.Fatalf("area cleared %+v", report.AreaCleared) + } + if !reflectArgs(report.Search.Displays, []string{"none", "", "none", "none"}) || report.Search.Shown != "Showing 1 of 4" { + t.Fatalf("search %+v", report.Search) + } + if !reflectArgs(report.FileSearch.Displays, []string{"none", "", "none", "none"}) || report.FileSearch.Shown != "Showing 1 of 4" { + t.Fatalf("filename search %+v", report.FileSearch) + } +} + +func TestPageHelpers(t *testing.T) { + for n, want := range map[int]string{0: "0", 7: "7", 999: "999", 1000: "1,000", 1346: "1,346", 1234567: "1,234,567"} { + if got := thousands(n); got != want { + t.Fatalf("thousands(%d) = %q", n, got) + } + } + if plural(1, "file", "files") != "1 file" || plural(1346, "file", "files") != "1,346 files" { + t.Fatal("plural") + } + for _, c := range []struct { + counts []int + want string + }{ + {[]int{0, 0, 0, 0}, "Reviewed 3 files with opus and found nothing to report."}, + {[]int{0, 37, 136, 123}, "Reviewed 3 files with opus. Start with the 37 at P1."}, + {[]int{0, 0, 1, 0}, "Reviewed 3 files with opus. The one finding is a P2."}, + {[]int{0, 0, 0, 2}, "Reviewed 3 files with opus. All 2 findings are P3."}, + } { + if got := lede("opus", 3, c.counts); got != c.want { + t.Fatalf("lede %v = %q", c.counts, got) + } + } + if summary(" One. Two. ") != "One." || summary("No period") != "No period" || summary("v1.2 is out") != "v1.2 is out" { + t.Fatal("summary") + } + if area("main.go") != "." || area("app/x.rb") != "app" || area("app/models/concerns/x.rb") != "app/models" { + t.Fatal("area") + } + if severityRank("P0") != 0 || severityRank("P3") != 3 || severityRank("bogus") != 4 { + t.Fatal("severityRank") + } + f := func(file, sev string, line int) lensoutput.TypedFinding { + return lensoutput.TypedFinding{File: file, Severity: sev, StartLine: line, EndLine: line} + } + groups := groupByFile([]lensoutput.TypedFinding{ + f("z.go", "P2", 9), f("z.go", "P2", 3), f("z.go", "P1", 5), + f("b.go", "P1", 1), + f("a.go", "P1", 1), + f("c.go", "P3", 1), + }) + var order []string + for _, g := range groups { + order = append(order, g.file) + } + // Worst severity first; among equals, more findings first; then by path. + if !reflectArgs(order, []string{"z.go", "a.go", "b.go", "c.go"}) { + t.Fatalf("group order %v", order) + } + z := groups[0].findings + if z[0].StartLine != 5 || z[1].StartLine != 3 || z[2].StartLine != 9 { + t.Fatalf("findings in z.go %+v", z) + } + rows := areaRows([]lensoutput.TypedFinding{f("b/x.go", "P3", 1), f("a/x.go", "P3", 1), f("c/x.go", "P2", 1), f("c/y.go", "P1", 1)}) + if rows[0].name != "c" || !reflectArgs(rows[0].severities, []string{"P1", "P2"}) || rows[1].name != "a" || rows[2].name != "b" { + t.Fatalf("area rows %+v", rows) + } +} + +func TestRenderRangesAndGutter(t *testing.T) { + files := map[string][]byte{"pkg/app.go": []byte("alpha\nbeta\ngamma\n")} + page := string(Render(Page{Repo: "/src/app", Commit: "c", ModelID: "grok-4.7", Files: files, Findings: []lensoutput.TypedFinding{ + {Tag: lensoutput.TagBug, File: "pkg/app.go", StartLine: 2, EndLine: 3, Issue: "First sentence. Second sentence.", Consequence: "bad", Confidence: 80, Severity: "P2"}, + }})) + for _, want := range []string{ + "

app

", "L2–3", `", `
beta` + "\ngamma
", + `First sentence. Second sentence.`, + `pkg/app.go`, "Model confidence 80 of 100", "The one finding is a P2.", `data-area="pkg"`, `data-file="pkg/app.go"`, + } { + if !strings.Contains(page, want) { + t.Fatalf("page missing %q", want) + } + } +} + +func TestRenderEmpty(t *testing.T) { + page := Render(Page{Repo: "r", Commit: "c", ModelID: "gpt-6-astra", Findings: nil, Files: nil}) + html := string(page) + for _, id := range []string{"count-p0", "count-p1", "count-p2", "count-p3"} { + if !strings.Contains(html, `id="`+id+`">`) || !strings.Contains(html, "0") { + t.Fatalf("missing zero %s", id) + } + } + if strings.Contains(html, `class="finding"`) { + t.Fatal("empty page has a finding") + } + if !strings.Contains(html, `id="severity"`) || !strings.Contains(html, `id="directory"`) { + t.Fatal("filters missing") + } + if !strings.Contains(html, "No findings. gpt-6-astra reviewed 0 files and reported nothing to fix.") || strings.Contains(html, `id="map"`) || strings.Contains(html, `class="seg`) { + t.Fatal("empty page should explain itself and draw no map or bar segments") + } + report := runPage(t, page) + if report.FindingCount != 0 { + t.Fatalf("findings %d", report.FindingCount) + } + for _, k := range []string{"p0", "p1", "p2", "p3"} { + if !strings.HasSuffix(report.Counts[k], " 0") { + t.Fatalf("%s = %s", k, report.Counts[k]) + } + } +} + +type pageReport struct { + FindingCount int `json:"findingCount"` + Counts map[string]string `json:"counts"` + Steps []struct { + Displays []string `json:"displays"` + } `json:"steps"` + Opened struct { + Hidden bool `json:"hidden"` + Expanded string `json:"expanded"` + Issue string `json:"issue"` + Consequence string `json:"consequence"` + Quote string `json:"quote"` + } `json:"opened"` + Closed struct { + Hidden bool `json:"hidden"` + Expanded string `json:"expanded"` + } `json:"closed"` + Bar struct { + Severity string `json:"severity"` + Displays []string `json:"displays"` + Pressed string `json:"pressed"` + Shown string `json:"shown"` + } `json:"bar"` + BarCleared struct { + Severity string `json:"severity"` + Displays []string `json:"displays"` + Shown string `json:"shown"` + } `json:"barCleared"` + Area struct { + Name string `json:"name"` + Displays []string `json:"displays"` + Groups []string `json:"groups"` + Pressed string `json:"pressed"` + } `json:"area"` + AreaCleared struct { + Displays []string `json:"displays"` + Pressed string `json:"pressed"` + } `json:"areaCleared"` + Search struct { + Displays []string `json:"displays"` + Shown string `json:"shown"` + } `json:"search"` + FileSearch struct { + Displays []string `json:"displays"` + Shown string `json:"shown"` + } `json:"fileSearch"` +} + +func runPage(t *testing.T, page []byte) pageReport { + t.Helper() + dir := t.TempDir() + htmlPath := filepath.Join(dir, "page.html") + scriptPath := filepath.Join(dir, "dom.js") + if err := os.WriteFile(htmlPath, page, 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(scriptPath, []byte(pageHarness), 0o644); err != nil { + t.Fatal(err) + } + cmd := exec.Command("node", scriptPath, htmlPath) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("node: %v\n%s", err, out) + } + var report pageReport + if err := json.Unmarshal(out, &report); err != nil { + t.Fatalf("report: %v\n%s", err, out) + } + return report +} + +const pageHarness = ` +const fs = require("fs"); +const html = fs.readFileSync(process.argv[2], "utf8"); +const m = html.match(/