From 77baa944bdde26bead7c54ef53aae64abf27ace5 Mon Sep 17 00:00:00 2001 From: Ben Kruger Date: Wed, 23 Sep 2026 07:46:53 -0700 Subject: [PATCH 01/22] feat(source-review): review a checkout's first-party source with Opus, Astra, or Grok Separate from diff review. Keeps Classify-Code UTF-8 product files and drops tests, vendor, generated code, config, and docs. A short citation maps to the one kept path that ends with it. A finding that still does not match is skipped. Writes findings.json and a self-contained HTML page. --- bin/ci | 4 + cmd/metareview/main.go | 7 + cmd/metareview/main_test.go | 11 + docs/ARCHITECTURE.md | 8 +- internal/sourcereview/fixture_test.go | 89 +++ internal/sourcereview/git.go | 120 ++++ internal/sourcereview/model.go | 152 +++++ internal/sourcereview/model_test.go | 220 +++++++ internal/sourcereview/pack.go | 148 +++++ internal/sourcereview/pack_test.go | 178 ++++++ internal/sourcereview/page.go | 143 +++++ internal/sourcereview/page_test.go | 261 ++++++++ internal/sourcereview/parse.go | 228 +++++++ internal/sourcereview/parse_test.go | 195 ++++++ internal/sourcereview/run.go | 302 ++++++++++ internal/sourcereview/run_test.go | 565 ++++++++++++++++++ internal/sourcereview/select.go | 150 +++++ internal/sourcereview/select_test.go | 330 ++++++++++ metareview-whole-repo-source-review-goal.md | 265 ++++++++ ...eview-whole-repo-source-review-progress.md | 91 +++ 20 files changed, 3466 insertions(+), 1 deletion(-) create mode 100755 bin/ci create mode 100644 internal/sourcereview/fixture_test.go create mode 100644 internal/sourcereview/git.go create mode 100644 internal/sourcereview/model.go create mode 100644 internal/sourcereview/model_test.go create mode 100644 internal/sourcereview/pack.go create mode 100644 internal/sourcereview/pack_test.go create mode 100644 internal/sourcereview/page.go create mode 100644 internal/sourcereview/page_test.go create mode 100644 internal/sourcereview/parse.go create mode 100644 internal/sourcereview/parse_test.go create mode 100644 internal/sourcereview/run.go create mode 100644 internal/sourcereview/run_test.go create mode 100644 internal/sourcereview/select.go create mode 100644 internal/sourcereview/select_test.go create mode 100644 metareview-whole-repo-source-review-goal.md create mode 100644 metareview-whole-repo-source-review-progress.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/cmd/metareview/main.go b/cmd/metareview/main.go index 9d88cd47..547176c0 100644 --- a/cmd/metareview/main.go +++ b/cmd/metareview/main.go @@ -32,6 +32,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" @@ -120,6 +121,7 @@ Usage: metareview review pr-ready [--base ] [--previous-run ] [--max-attempts ] [--evidence ] [--mutation-report ]... [--github-pr ] [--include-working-tree] [--shard-result ]... [--cross-shard-result ] 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 learn --post-merge [--base ] [--github-pr ] [--session-root ] + metareview source-review --model astra|opus|grok --output [] Commands: setup --check Detect repository mode and prerequisites without writing files @@ -142,6 +144,7 @@ Commands: review pr-ready Run PR-ready branch review review record-lenses Record an adjudicated lens review over HEAD (satisfies the require-lenses gate) learn --post-merge Curate post-merge repository learning + source-review Review first-party source with Astra, Opus, or Grok `, version.Version) } @@ -167,6 +170,10 @@ func dispatch(args []string) { return } + if args[0] == "source-review" { + exit(sourcereview.CLI(args[1:], workdir, stdout, stderr)) + } + if len(args) >= 1 && args[0] == "setup" { handleSetup(args[1:]) return diff --git a/cmd/metareview/main_test.go b/cmd/metareview/main_test.go index b414457e..8e75c38f 100644 --- a/cmd/metareview/main_test.go +++ b/cmd/metareview/main_test.go @@ -85,6 +85,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 []") { + 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 e685cfe5..cf4c749b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -39,6 +39,11 @@ 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 []`). +It reviews the first-party source at HEAD — code `Classify` keeps, minus tests, vendored trees, +generated files, and non-UTF-8 — by calling the logged-in `claude`, `codex`, or `grok` CLI. +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 @@ -201,7 +206,8 @@ list below is illustrative, omitting e.g. `judge`, `gate`, `converge`, `export`) `BuildForBranch`, coverage/unreviewed), `githooktest` (black-box hook tests), `covergate` (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..5212d12f --- /dev/null +++ b/internal/sourcereview/git.go @@ -0,0 +1,120 @@ +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 +} + +// parseTree reads `git ls-tree -r -z` records and returns blob paths. +func parseTree(out []byte) ([]string, error) { + var paths []string + 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) < 2 { + return nil, fmt.Errorf("bad ls-tree meta") + } + if fields[1] != "blob" { + continue + } + p := string(rec[tab+1:]) + if p == "" { + continue + } + paths = append(paths, p) + } + return paths, nil +} + +func readBlobs(root string, paths []string, git GitFunc) (map[string][]byte, error) { + if len(paths) == 0 { + return map[string][]byte{}, nil + } + var stdin bytes.Buffer + for _, p := range paths { + if strings.Contains(p, "\n") { + return nil, fmt.Errorf("path contains a newline") + } + fmt.Fprintf(&stdin, "HEAD:%s\n", p) + } + 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..922fca7e --- /dev/null +++ b/internal/sourcereview/model.go @@ -0,0 +1,152 @@ +package sourcereview + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "os/exec" + "strings" +) + +// Runner runs one logged-in CLI. stdin is the prompt for claude and codex; +// grok receives the prompt as the argument of -p and stdin is nil. +type Runner interface { + Run(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{} + +func (OSRunner) Run(name string, args []string, stdin []byte) ([]byte, error) { + cmd := exec.Command(name, args...) + if stdin != nil { + cmd.Stdin = bytes.NewReader(stdin) + } + 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 "grok-4.7", nil + default: + return "", fmt.Errorf("unknown model %q (want astra, opus, or grok)", model) + } +} + +// 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(runner Runner, model string, prompt []byte) (string, error) { + switch strings.ToLower(strings.TrimSpace(model)) { + case "opus": + out, err := runner.Run("claude", []string{"-p", "--model", "opus", "--output-format", "json"}, prompt) + if err != nil { + return "", err + } + return claudeModelText(out) + case "astra": + out, err := runner.Run("codex", []string{"exec", "--json", "-m", "gpt-6-astra", "-"}, prompt) + if err != nil { + return "", err + } + return codexModelText(out) + case "grok": + out, err := runner.Run("grok", []string{"-p", string(prompt), "-m", "grok-4.7", "--output-format", "json"}, 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"` + Result *string `json:"result"` + } + 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.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"` + } + if err := json.Unmarshal(stdout, &doc); err != nil { + return "", fmt.Errorf("missing model text: %w", err) + } + 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..d143f31e --- /dev/null +++ b/internal/sourcereview/model_test.go @@ -0,0 +1,220 @@ +package sourcereview + +import ( + "bytes" + "encoding/json" + "errors" + "os/exec" + "strings" + "testing" +) + +type fakeRunner struct { + calls []fakeCall + reply func(n int, name string, args []string, stdin []byte) ([]byte, error) + onRun func() +} + +type fakeCall struct { + name string + args []string + stdin []byte +} + +func (f *fakeRunner) Run(name string, args []string, stdin []byte) ([]byte, error) { + if f.onRun != nil { + f.onRun() + } + f.calls = append(f.calls, fakeCall{name, append([]string(nil), args...), append([]byte(nil), stdin...)}) + if f.reply == nil { + return nil, errors.New("no reply") + } + return f.reply(len(f.calls)-1, name, args, stdin) +} + +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"} { + fr := &fakeRunner{reply: func(int, string, []string, []byte) ([]byte, error) { + switch model { + case "opus": + return []byte(`{"is_error":false,"result":"{\"findings\":[]}"}`), nil + case "astra": + 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(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": + if c.name != "claude" || !reflectArgs(c.args, []string{"-p", "--model", "opus", "--output-format", "json"}) || !bytes.Equal(c.stdin, prompt) { + t.Fatalf("claude call %+v", c) + } + case "astra": + if c.name != "codex" || !reflectArgs(c.args, []string{"exec", "--json", "-m", "gpt-6-astra", "-"}) || !bytes.Equal(c.stdin, prompt) { + t.Fatalf("codex call %+v", c) + } + case "grok": + if c.name != "grok" || !reflectArgs(c.args, []string{"-p", string(prompt), "-m", "grok-4.7", "--output-format", "json"}) || c.stdin != nil { + t.Fatalf("grok call %+v", c) + } + } + } +} + +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(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(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(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(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(fr, "opus", []byte("p")); err == nil { + t.Fatal("bad claude json") + } + if _, err := Call(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(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(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(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(fr, "astra", []byte("p")); err == nil { + t.Fatal("no agent message") + } + if _, err := Call(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(fr, "astra", []byte("p")); err == nil { + t.Fatal("nonzero") + } + if _, err := Call(fr, "grok", []byte("p")); err == nil { + t.Fatal("grok nonzero") + } +} + +func TestOSRunner(t *testing.T) { + out, err := OSRunner{}.Run("echo", []string{"hi"}, nil) + if err != nil || !bytes.Contains(out, []byte("hi")) { + t.Fatalf("echo: %v %q", err, out) + } + out, err = OSRunner{}.Run("cat", nil, []byte("yo")) + if err != nil || !bytes.Equal(out, []byte("yo")) { + t.Fatalf("cat: %v %q", err, out) + } + if _, err := (OSRunner{}).Run("false", nil, nil); err == nil { + t.Fatal("false should fail") + } + if _, err := (OSRunner{}).Run("sh", []string{"-c", "echo boom >&2; exit 1"}, nil); err == nil || !strings.Contains(err.Error(), "boom") { + t.Fatal(err) + } + if _, err := (OSRunner{}).Run("sh", []string{"-c", "echo usage-limit; exit 1"}, nil); err == nil || !strings.Contains(err.Error(), "usage-limit") { + t.Fatal(err) + } + if _, err := (OSRunner{}).Run("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("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 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/pack.go b/internal/sourcereview/pack.go new file mode 100644 index 00000000..0237b631 --- /dev/null +++ b/internal/sourcereview/pack.go @@ -0,0 +1,148 @@ +package sourcereview + +import ( + "bytes" + "fmt" + "sort" + "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.` + +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 bytes. 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 { + var b bytes.Buffer + b.WriteString(header) + if !strings.HasSuffix(header, "\n") { + b.WriteByte('\n') + } + for i, f := range files { + 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(f.Body) + } + 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, big []File + for _, f := range files { + if len(promptWith(instruction, []File{f})) <= budget { + whole = append(whole, f) + } 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 +} + +func packWhole(files []File, budget int) [][]byte { + sorted := append([]File(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) + }) + var bins [][]File + for _, f := range sorted { + placed := false + for i := range bins { + candidate := append(append([]File{}, bins[i]...), f) + if len(promptWith(instruction, candidate)) <= budget { + bins[i] = candidate + placed = true + break + } + } + if !placed { + bins = append(bins, []File{f}) + } + } + out := make([][]byte, 0, len(bins)) + for _, bin := range bins { + out = append(out, promptWith(instruction, bin)) + } + 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 := cutEnd(f.Body, start, room) + if err != nil { + return nil, fmt.Errorf("%s: %w", f.Path, err) + } + out = append(out, promptWith(header, []File{{Path: f.Path, Body: f.Body[start:end]}})) + start = end + } + return out, nil +} + +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 + 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..0506ecb2 --- /dev/null +++ b/internal/sourcereview/pack_test.go @@ -0,0 +1,178 @@ +package sourcereview + +import ( + "bytes" + "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"), f.Body...) + 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) + } + part := p[len(prefix):] + 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) + } + 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, append([]byte("pkg/app.go\n"), body...)) { + t.Fatalf("path is not immediately before the body:\n%s", p) + } + if !bytes.Contains(p, []byte(`{"findings":[...]}`)) { + t.Fatal("instruction missing findings object") + } +} diff --git a/internal/sourcereview/page.go b/internal/sourcereview/page.go new file mode 100644 index 00000000..8d65e81f --- /dev/null +++ b/internal/sourcereview/page.go @@ -0,0 +1,143 @@ +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 +} + +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") +} + +const pageScript = ` +(function () { + var severity = document.getElementById("severity"); + var directory = document.getElementById("directory"); + var findings = document.querySelectorAll("#findings .finding"); + function apply() { + var s = severity.value; + var d = directory.value; + 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; + el.style.display = (okS && okD) ? "" : "none"; + } + } + severity.addEventListener("change", apply); + directory.addEventListener("change", 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; + }); + } +})(); +` + +const pageCSS = ` +body { font: 16px/1.4 ui-sans-serif, system-ui, sans-serif; margin: 1.5rem; color: #1a1a1a; background: #f7f5f2; } +header { margin-bottom: 1rem; } +.meta { margin: 0.15rem 0; } +.counts { display: flex; gap: 1rem; padding: 0; list-style: none; } +.filters { display: flex; gap: 1rem; margin: 1rem 0; } +article.finding { background: #fff; border: 1px solid #ccc; margin: 0.5rem 0; padding: 0.5rem 0.75rem; } +article.finding > button { font: inherit; background: none; border: 0; padding: 0; cursor: pointer; text-align: left; } +.detail { display: none; gap: 1.5rem; margin-top: 0.75rem; } +.detail:not([hidden]) { display: flex; } +.detail pre { margin: 0; background: #f0ece6; padding: 0.5rem; overflow: auto; } +` + +// 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) + var b strings.Builder + b.WriteString("\n\n\n\nSource review\n\n\n\n
\n") + fmt.Fprintf(&b, "

%s

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

%s

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

%s

\n", html.EscapeString(p.ModelID)) + b.WriteString("
    \n") + fmt.Fprintf(&b, "
  • P0 %d
  • \n", p0) + fmt.Fprintf(&b, "
  • P1 %d
  • \n", p1) + fmt.Fprintf(&b, "
  • P2 %d
  • \n", p2) + fmt.Fprintf(&b, "
  • P3 %d
  • \n", p3) + b.WriteString("
\n
\n
\n") + b.WriteString("\n") + b.WriteString("\n
\n
\n") + for _, f := range p.Findings { + dir := path.Dir(f.File) + quote := "" + if p.Files != nil { + quote = excerpt(p.Files[f.File], f.StartLine, f.EndLine) + } + fmt.Fprintf(&b, "
\n", html.EscapeString(f.Severity), html.EscapeString(dir)) + fmt.Fprintf(&b, "\n", html.EscapeString(f.File), f.StartLine, f.EndLine) + b.WriteString("\n
\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..370a5f79 --- /dev/null +++ b/internal/sourcereview/page_test.go @@ -0,0 +1,261 @@ +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.Issue != "root broke" || report.Opened.Consequence != "no entry" || report.Opened.Quote != "package main" { + t.Fatalf("opened %+v", report.Opened) + } +} + +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") + } + 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"` + Issue string `json:"issue"` + Consequence string `json:"consequence"` + Quote string `json:"quote"` + } `json:"opened"` +} + +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(/