feat(eval): golden-prompt harness over frozen review inputs

CLAUDE.md

@@ -16,6 +16,8 @@ cmd/
       install_skill.go # codecanary install-skill — write embedded Claude skill to disk
       setup.go     # codecanary setup [local|github]
       auth.go      # codecanary auth [status|delete]
+  evalsnap/        # Dev tool — freeze a PR's review inputs into a corpus fixture
+    main.go        # NOT part of the codecanary binary
 internal/
   review/
     runner.go            # Core review pipeline — single Run() entry point
@@ -43,8 +45,12 @@ internal/
     local.go             # Local diff & git operations
     state.go             # Local state persistence
     docs.go              # Project doc discovery
+    prompt_golden_test.go # Renders corpus fixtures into prompts, diffs vs goldens
+    testdata/corpus/     # Frozen review inputs + prompt goldens + SIZES.txt (public PRs only)
   credentials/     # Credential storage (keychain with file fallback)
     keyring.go     # Store/Retrieve/Delete — keychain first, ~/.codecanary/credentials.json fallback
+  evalcorpus/      # Frozen review inputs — fixture format + load/save (dev tooling)
+    corpus.go      # Fixture type, Dir/Load/Save/List
   skills/          # Claude Code skills embedded in the binary via //go:embed
     skills.go      # Exports CodecanaryFix() returning the skill body
     codecanary-fix/SKILL.md  # Canonical skill source (duplicated at .claude/skills/codecanary-fix/SKILL.md; parity enforced by skills_test.go)
@@ -71,6 +77,19 @@ install.sh         # Downloads and installs codecanary binary permanently
 ## Binary
 
 - **`codecanary`** — single binary for reviews, setup, and credential management. Installed locally via `install.sh`, also used by the GitHub Action.
+- **`evalsnap`** (`cmd/evalsnap`) — developer tool, not shipped. `go build ./cmd/review` does not pull it in.
+
+## Prompt evaluation harness
+
+`internal/review/testdata/corpus/` holds frozen review inputs captured from real PRs. `TestPromptGolden` renders each into a prompt and diffs it against a checked-in golden, and `SIZES.txt` records every prompt's rendered size.
+
+This exists because prompt edits are otherwise invisible in code review: their effect surfaces later as a change in the findings, when the cause is hard to attribute. Instruction overhead is paid on every review forever, so growth should be a number in the diff, not a surprise.
+
+It measures prompt *construction*, not review *quality* — no model runs. Judging whether a prompt change helps needs labelled findings, repeated runs to establish variance, and real model calls; that is a separate layer that does not exist yet.
+
+Regenerate goldens with `go test ./internal/review/ -run Golden -update`, and state the size delta in the PR description.
+
+**Fixtures committed to this repo must come from public repositories only.** A fixture embeds the PR diff and the full contents of every file the reviewer read; committing one from a private repo publishes that source permanently. `evalsnap` refuses to write a private repo's fixture into any git-tracked directory. Keep those outside the repo and point `$CODECANARY_EVAL_CORPUS` at them. See `internal/review/testdata/corpus/README.md`.
 
 ## Build
 

cmd/evalsnap/main.go

@@ -0,0 +1,327 @@
+// Command evalsnap freezes a pull request's review inputs into a corpus
+// fixture, so a prompt can later be rendered from it without touching the
+// network or a working tree.
+//
+// It is developer tooling and is deliberately not part of the codecanary
+// binary: `go build ./cmd/review` does not pull it in.
+//
+// Fidelity comes from reusing the review package's own fetch path rather than
+// reimplementing it — the same FetchPR, the same file-content reader with the
+// same size and ignore filtering, the same project-doc discovery. A fixture is
+// therefore what a real review would have seen, not an approximation of it.
+// That reuse is also why the tool must run from inside a checkout of the
+// target repository at the PR's head commit: file contents and project docs
+// are read from the working tree, and reading them at the wrong commit would
+// bake a mismatched snapshot into the corpus. The tool verifies HEAD before
+// capturing rather than trusting the caller.
+//
+// Usage:
+//
+//	cd /path/to/target-repo
+//	git fetch origin pull/1234/head && git checkout FETCH_HEAD
+//	evalsnap --repo owner/name --pr 1234 --out "$CODECANARY_EVAL_CORPUS"
+//
+// --out has no default: a fixture embeds the PR's diff and the full contents
+// of every file the reviewer read, so where it lands is a decision to make
+// deliberately rather than one to fall into. Fixtures from a private
+// repository must go somewhere git does not track — guardPrivateSource
+// enforces that rather than trusting it.
+package main
+
+import (
+	"flag"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"time"
+
+	"github.com/alansikora/codecanary/internal/evalcorpus"
+	"github.com/alansikora/codecanary/internal/review"
+)
+
+func main() {
+	if err := run(); err != nil {
+		fmt.Fprintf(os.Stderr, "evalsnap: %v\n", err)
+		os.Exit(1)
+	}
+}
+
+func run() error {
+	var (
+		repo       = flag.String("repo", "", "GitHub repo as owner/name (required)")
+		pr         = flag.Int("pr", 0, "pull request number (required)")
+		out        = flag.String("out", "", "corpus directory to write into (required)")
+		name       = flag.String("name", "", "fixture name (default: <owner>-<name>-pr<number>)")
+		configPath = flag.String("config", "", "review config path (auto-detected when empty)")
+		force      = flag.Bool("force", false, "capture even if HEAD is not the PR's head commit")
+	)
+	flag.Parse()
+
+	switch {
+	case *repo == "":
+		return fmt.Errorf("--repo is required")
+	case *pr == 0:
+		return fmt.Errorf("--pr is required")
+	case *out == "":
+		return fmt.Errorf("--out is required (the corpus directory to write into)")
+	}
+
+	// Safety before work: this is the check that prevents publishing private
+	// source, so it runs before any fetch, any read, and any write.
+	if err := guardPrivateSource(*repo, *out); err != nil {
+		return err
+	}
+
+	prData, err := review.FetchPR(*repo, *pr)
+	if err != nil {
+		return fmt.Errorf("fetching PR: %w", err)
+	}
+
+	headSHA, err := review.HeadSHA()
+	if err != nil {
+		return fmt.Errorf("reading HEAD (run this from inside a checkout of %s): %w", *repo, err)
+	}
+	prHead, err := prHeadSHA(*repo, *pr)
+	if err != nil {
+		return err
+	}
+	if headSHA != prHead && !*force {
+		fmt.Fprintf(os.Stderr, `
+File contents and project docs are read from the working tree, so capturing
+now would freeze a snapshot that does not match the diff. Check out the PR
+head first:
+
+    git fetch origin pull/%d/head && git checkout FETCH_HEAD
+
+Pass --force to capture anyway.
+
+`, *pr)
+		return fmt.Errorf("HEAD is %s but PR #%d is at %s", short(headSHA), *pr, short(prHead))
+	}
+
+	cfg, err := loadConfig(*configPath)
+	if err != nil {
+		return fmt.Errorf("loading review config: %w", err)
+	}
+
+	// Freeze the raw PR plus what the content reader left out, not an
+	// already-scoped PR: the harness applies the review's own scoping when it
+	// renders, so a change to that scoping shows up in the goldens instead of
+	// being baked into the fixture.
+	fc := review.FetchFileContents(
+		prData.Files, cfg.Ignore, cfg.EffectiveMaxFileSize(), cfg.EffectiveMaxTotalSize())
+	if len(fc.Excluded) > 0 {
+		fmt.Fprintf(os.Stderr, "excluded %d ignored/binary file(s): %s\n",
+			len(fc.Excluded), strings.Join(fc.Excluded, ", "))
+	}
+	if len(fc.DiffOnly) > 0 {
+		fmt.Fprintf(os.Stderr, "%d file(s) over the size limits, diff only: %s\n",
+			len(fc.DiffOnly), strings.Join(fc.DiffOnly, ", "))
+	}
+
+	fixture := &evalcorpus.Fixture{
+		Name:        fixtureName(*name, *repo, *pr),
+		Repo:        *repo,
+		PRNumber:    *pr,
+		HeadSHA:     headSHA,
+		CapturedAt:  time.Now().UTC().Format(time.RFC3339),
+		PR:          toPRInput(prData, fc),
+		Config:      toConfigInput(cfg),
+		ProjectDocs: review.ReadProjectDocs(prData.Files),
+	}
+
+	path, err := evalcorpus.Save(*out, fixture)
+	if err != nil {
+		return err
+	}
+
+	info, err := os.Stat(path)
+	if err != nil {
+		return err
+	}
+	fmt.Printf("wrote %s (%s, %d files, %d project doc(s))\n",
+		path, humanBytes(info.Size()), len(fixture.PR.Files), len(fixture.ProjectDocs))
+	return nil
+}
+
+// loadConfig resolves the review config the same way a real review does,
+// falling back to an empty config when the target repo has none — a repo
+// without a .codecanary config is a perfectly valid thing to capture, and a
+// nil config is exactly what the prompt builder would receive there.
+func loadConfig(path string) (*review.ReviewConfig, error) {
+	if path == "" {
+		found, err := review.FindConfig()
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "no review config found; capturing with an empty config\n")
+			return &review.ReviewConfig{}, nil
+		}
+		path = found
+	}
+	return review.LoadConfig(path)
+}
+
+// guardPrivateSource refuses to write a fixture captured from a private
+// repository into a directory that git tracks.
+//
+// A fixture embeds the PR's diff and the full contents of every file the
+// reviewer read. Committing one captured from a private repository publishes
+// that source, and git history makes the mistake permanent — so this is
+// checked before any file content is read, not after.
+//
+// The test is "does git ignore the destination", not "is the destination
+// inside this repository": a corpus kept anywhere git would track it carries
+// the same risk, and an ignored path is the arrangement that is actually safe.
+func guardPrivateSource(repo, out string) error {
+	private, err := isPrivateRepo(repo)
+	if err != nil {
+		return fmt.Errorf("could not determine whether %s is private (refusing to guess): %w", repo, err)
+	}
+	if !private {
+		return nil
+	}
+	tracked, err := gitWouldTrack(out)
+	if err != nil {
+		return err
+	}
+	if !tracked {
+		return nil
+	}
+	fmt.Fprintf(os.Stderr, `
+A fixture embeds the PR's diff and the full contents of every file the
+reviewer read. Committing it would publish that source permanently.
+
+Write the corpus somewhere git does not track — a private repository, or a
+gitignored directory — and point the harness at it:
+
+    export %s=/path/to/private/corpus
+    evalsnap --repo %s --pr N --out "$%s"
+
+`, evalcorpus.EnvCorpusDir, repo, evalcorpus.EnvCorpusDir)
+	return fmt.Errorf("refusing to capture: %s is private but %s is tracked by git", repo, out)
+}
+
+// isPrivateRepo asks GitHub rather than inferring from the name, so a rename
+// or a transfer cannot quietly turn the guard off.
+func isPrivateRepo(repo string) (bool, error) {
+	cmd := exec.Command("gh", "repo", "view", repo, "--json", "isPrivate", "--jq", ".isPrivate")
+	var stderr strings.Builder
+	cmd.Stderr = &stderr
+	out, err := cmd.Output()
+	if err != nil {
+		return false, fmt.Errorf("%w: %s", err, strings.TrimSpace(stderr.String()))
+	}
+	return strings.TrimSpace(string(out)) == "true", nil
+}
+
+// gitWouldTrack reports whether git would track files written to dir: it is
+// inside a work tree and not ignored. A path outside any repository, or one
+// git ignores, is safe to write a private corpus into.
+func gitWouldTrack(dir string) (bool, error) {
+	abs, err := filepath.Abs(dir)
+	if err != nil {
+		return false, err
+	}
+	// check-ignore needs an existing ancestor to resolve against.
+	probe := abs
+	for {
+		if _, err := os.Stat(probe); err == nil {
+			break
+		}
+		parent := filepath.Dir(probe)
+		if parent == probe {
+			return false, nil // nothing on this path exists yet
+		}
+		probe = parent
+	}
+
+	inTree := exec.Command("git", "-C", probe, "rev-parse", "--is-inside-work-tree")
+	if out, err := inTree.Output(); err != nil || strings.TrimSpace(string(out)) != "true" {
+		return false, nil // not a git work tree at all
+	}
+
+	// check-ignore exits 0 when the path IS ignored, 1 when it is not.
+	ignored := exec.Command("git", "-C", probe, "check-ignore", "-q", abs)
+	if err := ignored.Run(); err == nil {
+		return false, nil // ignored, therefore not tracked
+	}
+	return true, nil
+}
+
+// prHeadSHA asks GitHub for the PR's head commit so the working tree can be
+// checked against it before anything is read from disk.
+func prHeadSHA(repo string, pr int) (string, error) {
+	cmd := exec.Command("gh", "api",
+		fmt.Sprintf("repos/%s/pulls/%d", repo, pr), "--jq", ".head.sha")
+	var stderr strings.Builder
+	cmd.Stderr = &stderr
+	out, err := cmd.Output()
+	if err != nil {
+		return "", fmt.Errorf("resolving PR head sha: %w: %s", err, strings.TrimSpace(stderr.String()))
+	}
+	return strings.TrimSpace(string(out)), nil
+}
+
+func fixtureName(explicit, repo string, pr int) string {
+	if explicit != "" {
+		return explicit
+	}
+	return fmt.Sprintf("%s-pr%d", strings.ReplaceAll(repo, "/", "-"), pr)
+}
+
+func toPRInput(pr *review.PRData, fc review.FileContentsResult) evalcorpus.PRInput {
+	return evalcorpus.PRInput{
+		Number:       pr.Number,
+		Title:        pr.Title,
+		Body:         pr.Body,
+		Author:       pr.Author,
+		BaseBranch:   pr.BaseBranch,
+		HeadBranch:   pr.HeadBranch,
+		Diff:         pr.Diff,
+		Files:        pr.Files,
+		FileContents: fc.Contents,
+		DiffOnly:     fc.DiffOnly,
+		Excluded:     fc.Excluded,
+	}
+}
+
+func toConfigInput(cfg *review.ReviewConfig) *evalcorpus.ConfigInput {
+	if cfg == nil {
+		return nil
+	}
+	rules := make([]evalcorpus.RuleInput, 0, len(cfg.Rules))
+	for _, r := range cfg.Rules {
+		rules = append(rules, evalcorpus.RuleInput{
+			ID:           r.ID,
+			Description:  r.Description,
+			Severity:     r.Severity,
+			Paths:        r.Paths,
+			ExcludePaths: r.ExcludePaths,
+		})
+	}
+	return &evalcorpus.ConfigInput{
+		Rules:       rules,
+		Context:     cfg.Context,
+		Ignore:      cfg.Ignore,
+		MaxDiffSize: cfg.MaxDiffSize,
+	}
+}
+
+func short(sha string) string {
+	if len(sha) > 7 {
+		return sha[:7]
+	}
+	return sha
+}
+
+func humanBytes(n int64) string {
+	switch {
+	case n >= 1<<20:
+		return fmt.Sprintf("%.1f MB", float64(n)/(1<<20))
+	case n >= 1<<10:
+		return fmt.Sprintf("%.1f KB", float64(n)/(1<<10))
+	default:
+		return fmt.Sprintf("%d B", n)
+	}
+}