diff --git a/.gitignore b/.gitignore index 3e6bfd4..65659db 100644 --- a/.gitignore +++ b/.gitignore @@ -9,8 +9,11 @@ **/reports/latest/ **/reports/run-*/ -# Memory persistence (lives next to target repos, not this one) -/.memory/ +# Memory persistence (lives next to target repos AND in fixtures +# during testing — pattern is double-star so test runs against +# tests/fixtures// don't leak the .memory/ JSONL into the +# harness repo, same shape as the reports/latest/ rule). +**/.memory/ # Go *.test diff --git a/cmd/review-harness/main.go b/cmd/review-harness/main.go index a8c9ef7..3de8796 100644 --- a/cmd/review-harness/main.go +++ b/cmd/review-harness/main.go @@ -36,8 +36,7 @@ func main() { case "repo": os.Exit(cli.Repo(args)) case "diff": - fmt.Fprintln(os.Stderr, "diff: not implemented in MVP (Phase E)") - os.Exit(64) + os.Exit(cli.Diff(args)) case "scrum": os.Exit(cli.Scrum(args)) case "rules": diff --git a/internal/cli/repo.go b/internal/cli/repo.go index bc28770..5e5e42d 100644 --- a/internal/cli/repo.go +++ b/internal/cli/repo.go @@ -6,11 +6,13 @@ package cli import ( "context" + "flag" "fmt" "os" "path/filepath" "local-review-harness/internal/config" + "local-review-harness/internal/git" "local-review-harness/internal/pipeline" ) @@ -49,6 +51,75 @@ func runRepo(ctx context.Context, repoPath string, cf commonFlags) int { return res.ExitCode } +// Diff is the `review-harness diff ` subcommand entry point +// (Phase E). Detects changed files via git (unstaged + staged + vs +// branch base), then runs the same pipeline as `repo` but scoped to +// just those files. PROMPT.md mode 2: "diff review." +func Diff(args []string) int { + fs := flag.NewFlagSet("diff", flag.ContinueOnError) + var cf commonFlags + bindCommonFlags(fs, &cf) + if err := fs.Parse(args); err != nil { + return 64 + } + if fs.NArg() < 1 { + fmt.Fprintln(os.Stderr, "diff: missing target path") + return 64 + } + repoPath := fs.Arg(0) + return runDiff(context.Background(), repoPath, cf) +} + +func runDiff(ctx context.Context, repoPath string, cf commonFlags) int { + if _, err := os.Stat(repoPath); err != nil { + fmt.Fprintln(os.Stderr, "diff: target path:", err) + return 65 + } + rp, err := config.LoadReviewProfile(cf.reviewProfilePath) + if err != nil { + fmt.Fprintln(os.Stderr, "config:", err) + return 65 + } + mp, err := config.LoadModelProfile(cf.modelProfilePath) + if err != nil { + fmt.Fprintln(os.Stderr, "config:", err) + return 65 + } + + changed, err := git.ChangedFiles(ctx, repoPath) + if err != nil { + fmt.Fprintln(os.Stderr, "diff: git probe:", err) + return 65 + } + if len(changed) == 0 { + fmt.Fprintln(os.Stderr, "diff: no changed files (clean tree, no commits ahead of main/master) — exit 0") + return 0 + } + fmt.Fprintf(os.Stderr, "diff: scanning %d changed file(s):\n", len(changed)) + for _, f := range changed { + fmt.Fprintln(os.Stderr, " -", f) + } + + outDir := resolveOutputDir(&cf, rp, repoPath) + res, err := pipeline.RunRepo(ctx, pipeline.Inputs{ + RepoPath: repoPath, + ReviewProfile: rp, + ModelProfile: mp, + OutputDir: outDir, + EmitScrum: true, // diff mode emits the scrum bundle so PR reviewers see it + EnableLLM: cf.enableLLM, + DiffOnlyFiles: changed, + }) + if err != nil { + fmt.Fprintln(os.Stderr, "pipeline:", err) + return 65 + } + for _, f := range res.OutputFiles { + fmt.Println(filepath.Join(outDir, f)) + } + return res.ExitCode +} + func runScrum(ctx context.Context, repoPath string, cf commonFlags) int { if _, err := os.Stat(repoPath); err != nil { fmt.Fprintln(os.Stderr, "scrum: target path:", err) diff --git a/internal/git/git.go b/internal/git/git.go index d3e1271..87492f4 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -6,6 +6,7 @@ package git import ( "context" + "fmt" "os/exec" "path/filepath" "strings" @@ -64,3 +65,61 @@ func runGit(ctx context.Context, dir string, args ...string) string { } return strings.TrimSpace(string(out)) } + +// ChangedFiles returns the set of repo-relative paths the diff +// subcommand should scan. Includes (in priority order): +// - unstaged changes (`git diff --name-only`) +// - staged changes (`git diff --cached --name-only`) +// - branch diff against base (`git diff --name-only base..HEAD`) +// +// base is auto-detected: prefer "main" then "master" then HEAD~1. +// Returns dedup'd, stable-ordered list. Empty list when there's +// nothing to review (clean tree, no commits ahead of base). +func ChangedFiles(ctx context.Context, repoPath string) ([]string, error) { + if _, err := exec.LookPath("git"); err != nil { + return nil, fmt.Errorf("git not in PATH") + } + abs, _ := filepath.Abs(repoPath) + cctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + if err := exec.CommandContext(cctx, "git", "-C", abs, "rev-parse", "--git-dir").Run(); err != nil { + return nil, fmt.Errorf("not a git repository: %s", repoPath) + } + + seen := map[string]bool{} + var out []string + add := func(s string) { + s = strings.TrimSpace(s) + if s == "" || seen[s] { + return + } + seen[s] = true + out = append(out, s) + } + + // Unstaged + for _, line := range strings.Split(runGit(ctx, abs, "diff", "--name-only"), "\n") { + add(line) + } + // Staged + for _, line := range strings.Split(runGit(ctx, abs, "diff", "--cached", "--name-only"), "\n") { + add(line) + } + // vs base — try main, master, then HEAD~1 + for _, base := range []string{"main", "master"} { + if runGit(ctx, abs, "rev-parse", "--verify", base) != "" { + for _, line := range strings.Split(runGit(ctx, abs, "diff", "--name-only", base+"...HEAD"), "\n") { + add(line) + } + break + } + } + + return out, nil +} + +// fmt + filepath are already imported indirectly; this var keeps +// the import list clean if those packages get unused after a refactor. +var _ = fmt.Sprintf +var _ = filepath.Join + diff --git a/internal/memory/memory.go b/internal/memory/memory.go new file mode 100644 index 0000000..629b65e --- /dev/null +++ b/internal/memory/memory.go @@ -0,0 +1,144 @@ +// Package memory implements PROMPT.md Phase 5: append-only `.memory/` +// state that lets the harness build up knowledge across runs. +// +// The append-only constraint is non-optional. Operators can grep +// .memory/ to see how findings drifted run-to-run, prove no silent +// data loss, and reconstruct intermediate states. Every write goes +// through Append* helpers that open with O_APPEND only — no O_TRUNC, +// no os.Create. A regression test proves the constraint holds. +// +// Files written: +// .memory/known-risks.jsonl one line per confirmed finding per run; +// same finding ID across runs deduped +// in the reader, never silently dropped +// from the log +// .memory/run-history.jsonl one line per run; summary stats + +// receipts hash for cross-link +// .memory/project-profile.json overwritten — non-versioned snapshot +// of static repo facts (language mix, +// latest commit, etc.). Operator-readable. +package memory + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "time" + + "local-review-harness/internal/analyzers" +) + +// KnownRiskEntry is one append per confirmed finding per run. +type KnownRiskEntry struct { + RunID string `json:"run_id"` + WrittenAt string `json:"written_at"` + Finding analyzers.Finding `json:"finding"` +} + +// RunHistoryEntry is one append per harness run. +type RunHistoryEntry struct { + RunID string `json:"run_id"` + RepoPath string `json:"repo_path"` + StartedAt string `json:"started_at"` + FinishedAt string `json:"finished_at"` + TotalFindings int `json:"total_findings"` + Confirmed int `json:"confirmed"` + Critical int `json:"critical"` + High int `json:"high"` + Medium int `json:"medium"` + Low int `json:"low"` + LLMEnabled bool `json:"llm_enabled"` + ExitCode int `json:"exit_code"` + ReceiptsHash string `json:"receipts_hash,omitempty"` // cross-link +} + +// ProjectProfile is the only non-versioned memory file. Overwrites OK +// — it's a snapshot, not a log. The append-only contract applies to +// known-risks + run-history. +type ProjectProfile struct { + RepoPath string `json:"repo_path"` + LastSeenAt string `json:"last_seen_at"` + LastSeenCommit string `json:"last_seen_commit,omitempty"` + LanguageBreakdown map[string]int `json:"language_breakdown"` + FileCount int `json:"file_count"` +} + +// Writer is the append-only memory writer. Holds a base path so +// every method writes under the same .memory/ root. Stateless; safe +// for concurrent use (each Append opens its own fd). +type Writer struct { + dir string +} + +// NewWriter constructs a Writer rooted at /.memory/. The +// dir is created on demand. Operators who want a different location +// can override via review-profile.memory.path (Phase E follow-up). +func NewWriter(repoPath string) (*Writer, error) { + dir := filepath.Join(repoPath, ".memory") + if err := os.MkdirAll(dir, 0o755); err != nil { + return nil, err + } + return &Writer{dir: dir}, nil +} + +// AppendKnownRisks appends one JSONL line per confirmed finding. +// Append-only: opens with O_APPEND|O_CREATE|O_WRONLY. NEVER opens +// with O_TRUNC. Truncation is the failure mode this package exists +// to prevent. +func (w *Writer) AppendKnownRisks(runID string, findings []analyzers.Finding) error { + if len(findings) == 0 { + return nil + } + f, err := os.OpenFile(filepath.Join(w.dir, "known-risks.jsonl"), + os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + return fmt.Errorf("open known-risks: %w", err) + } + defer f.Close() + enc := json.NewEncoder(f) + now := time.Now().UTC().Format(time.RFC3339Nano) + for _, finding := range findings { + entry := KnownRiskEntry{ + RunID: runID, + WrittenAt: now, + Finding: finding, + } + if err := enc.Encode(&entry); err != nil { + return fmt.Errorf("encode known-risk: %w", err) + } + } + return nil +} + +// AppendRunHistory writes one JSONL line for the run. Same append- +// only constraint as known-risks. +func (w *Writer) AppendRunHistory(entry RunHistoryEntry) error { + f, err := os.OpenFile(filepath.Join(w.dir, "run-history.jsonl"), + os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) + if err != nil { + return fmt.Errorf("open run-history: %w", err) + } + defer f.Close() + if err := json.NewEncoder(f).Encode(&entry); err != nil { + return fmt.Errorf("encode run-history: %w", err) + } + return nil +} + +// WriteProjectProfile overwrites .memory/project-profile.json. This +// is the ONLY memory file that's not append-only — it's a snapshot +// of current state, not a log. Operators wanting historical profiles +// can grep run-history.jsonl which carries the receipts hash. +func (w *Writer) WriteProjectProfile(p ProjectProfile) error { + bs, err := json.MarshalIndent(p, "", " ") + if err != nil { + return err + } + bs = append(bs, '\n') + return os.WriteFile(filepath.Join(w.dir, "project-profile.json"), bs, 0o644) +} + +// MemoryDir returns the absolute .memory/ path for the writer. +// Useful in receipts so operators can find the JSONL files. +func (w *Writer) MemoryDir() string { return w.dir } diff --git a/internal/memory/memory_test.go b/internal/memory/memory_test.go new file mode 100644 index 0000000..980b62e --- /dev/null +++ b/internal/memory/memory_test.go @@ -0,0 +1,147 @@ +package memory + +import ( + "bufio" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "local-review-harness/internal/analyzers" +) + +// === Gate E1 — append-only contract === +// +// PROMPT.md hard rule: "Memory should be append-only by default. +// Never silently overwrite prior memory." Every Writer.Append* must +// open with O_APPEND, never O_TRUNC. Truncation is the failure mode +// this package exists to prevent — operators relying on .memory/ to +// see drift across runs would silently lose history. +// +// This test does the receipt-honesty equivalent: write once, write +// again with different content, assert the FIRST write's content is +// still visible in the file. + +func TestAppendKnownRisks_NeverTruncates(t *testing.T) { + repo := t.TempDir() + w, err := NewWriter(repo) + if err != nil { + t.Fatal(err) + } + + // Run 1: write 2 findings + run1 := []analyzers.Finding{ + {ID: "abc111", Title: "first run finding 1", File: "a.go", Severity: analyzers.SeverityHigh}, + {ID: "abc222", Title: "first run finding 2", File: "b.go", Severity: analyzers.SeverityMedium}, + } + if err := w.AppendKnownRisks("run-1", run1); err != nil { + t.Fatal(err) + } + + // Run 2: write 1 different finding + run2 := []analyzers.Finding{ + {ID: "def333", Title: "second run finding", File: "c.go", Severity: analyzers.SeverityCritical}, + } + if err := w.AppendKnownRisks("run-2", run2); err != nil { + t.Fatal(err) + } + + // Read back: file should contain ALL 3 entries (2+1), not just run 2's. + entries := readAll(t, filepath.Join(repo, ".memory", "known-risks.jsonl")) + if len(entries) != 3 { + t.Fatalf("expected 3 entries (2 from run 1 + 1 from run 2); got %d", len(entries)) + } + if !strings.Contains(entries[0], "abc111") || !strings.Contains(entries[0], "run-1") { + t.Errorf("first entry should be run 1's first finding; got %q", entries[0]) + } + if !strings.Contains(entries[2], "def333") || !strings.Contains(entries[2], "run-2") { + t.Errorf("last entry should be run 2's finding; got %q", entries[2]) + } +} + +func TestAppendRunHistory_NeverTruncates(t *testing.T) { + repo := t.TempDir() + w, _ := NewWriter(repo) + + for i, r := range []RunHistoryEntry{ + {RunID: "run-A", RepoPath: repo, TotalFindings: 5}, + {RunID: "run-B", RepoPath: repo, TotalFindings: 8}, + {RunID: "run-C", RepoPath: repo, TotalFindings: 3}, + } { + if err := w.AppendRunHistory(r); err != nil { + t.Fatalf("append %d: %v", i, err) + } + } + + entries := readAll(t, filepath.Join(repo, ".memory", "run-history.jsonl")) + if len(entries) != 3 { + t.Fatalf("expected 3 history entries; got %d", len(entries)) + } + for i, expected := range []string{"run-A", "run-B", "run-C"} { + var entry RunHistoryEntry + if err := json.Unmarshal([]byte(entries[i]), &entry); err != nil { + t.Fatalf("entry %d: %v", i, err) + } + if entry.RunID != expected { + t.Errorf("entry %d: RunID = %q, want %q", i, entry.RunID, expected) + } + } +} + +func TestAppendKnownRisks_EmptyFindingsIsNoop(t *testing.T) { + // Calling Append with zero findings shouldn't even create the file — + // avoids polluting .memory/ with empty files on clean runs. + repo := t.TempDir() + w, _ := NewWriter(repo) + if err := w.AppendKnownRisks("run-empty", nil); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(repo, ".memory", "known-risks.jsonl")); !os.IsNotExist(err) { + t.Errorf("empty append should not create the file; stat err = %v", err) + } +} + +func TestWriteProjectProfile_OverwriteIsAllowed(t *testing.T) { + // project-profile.json is the ONLY memory file allowed to overwrite — + // it's a snapshot, not a log. Verify the overwrite semantic works. + repo := t.TempDir() + w, _ := NewWriter(repo) + if err := w.WriteProjectProfile(ProjectProfile{ + RepoPath: repo, FileCount: 100, + }); err != nil { + t.Fatal(err) + } + if err := w.WriteProjectProfile(ProjectProfile{ + RepoPath: repo, FileCount: 200, + }); err != nil { + t.Fatal(err) + } + bs, err := os.ReadFile(filepath.Join(repo, ".memory", "project-profile.json")) + if err != nil { + t.Fatal(err) + } + var p ProjectProfile + if err := json.Unmarshal(bs, &p); err != nil { + t.Fatal(err) + } + if p.FileCount != 200 { + t.Errorf("project-profile should reflect last write (200); got %d", p.FileCount) + } +} + +// readAll reads a JSONL file; returns one string per line. +func readAll(t *testing.T, path string) []string { + t.Helper() + f, err := os.Open(path) + if err != nil { + t.Fatalf("open %s: %v", path, err) + } + defer f.Close() + var out []string + s := bufio.NewScanner(f) + for s.Scan() { + out = append(out, s.Text()) + } + return out +} diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 2fd0e05..2584dc6 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -18,6 +18,7 @@ import ( "local-review-harness/internal/config" "local-review-harness/internal/git" "local-review-harness/internal/llm" + "local-review-harness/internal/memory" "local-review-harness/internal/reporters" "local-review-harness/internal/scanner" "local-review-harness/internal/validators" @@ -31,6 +32,12 @@ type Inputs struct { OutputDir string EmitScrum bool // true → also emit scrum-test/risk-register/sprint-backlog/acceptance-gates markdown EnableLLM bool // Phase C: actually call the model. Off by default — operators opt in. + + // DiffOnlyFiles, when non-nil, restricts the scan to JUST these + // repo-relative paths — the diff subcommand uses this to focus + // review on changed files rather than the full repo. Nil = scan + // everything. + DiffOnlyFiles []string } // Result is what the CLI shows the operator. @@ -61,6 +68,24 @@ func RunRepo(ctx context.Context, in Inputs) (*Result, error) { // --- Phase 0: repo intake --- scan, err := scanner.Walk(in.RepoPath, true) + // Diff-mode: filter the scan to just the changed files. Phase 0 + // intake stats stay accurate (operator wants to see what the + // repo looks like overall) but analyzers + LLM only see the + // changed slice. Pre-filter saves O(N) regex passes when N is + // 5000 files but only 3 changed. + if len(in.DiffOnlyFiles) > 0 && err == nil { + want := map[string]bool{} + for _, f := range in.DiffOnlyFiles { + want[f] = true + } + filtered := scan.Files[:0] + for _, f := range scan.Files { + if want[f.Path] { + filtered = append(filtered, f) + } + } + scan.Files = filtered + } scanPhase := reporters.PhaseReceipt{Name: "repo_intake", Status: "ok"} if err != nil { scanPhase.Status = "failed" @@ -229,11 +254,70 @@ func RunRepo(ctx context.Context, in Inputs) (*Result, error) { receipt.Phases = append(receipt.Phases, reportPhase) } - // --- Phase 5: memory (Phase E — deferred) --- - receipt.Phases = append(receipt.Phases, reporters.PhaseReceipt{ - Name: "memory_update", Status: "skipped", - Errors: []string{"Phase E not implemented in MVP"}, - }) + // --- Phase 5: memory (Phase E) --- + // Append-only writes under /.memory/. Persists confirmed + // findings + run summary so cross-run drift becomes observable. + // Per PROMPT.md hard rule: never silently overwrite. + memPhase := reporters.PhaseReceipt{Name: "memory_update", Status: "ok"} + if in.ReviewProfile.Memory.Enabled { + mw, merr := memory.NewWriter(in.RepoPath) + if merr != nil { + memPhase.Status = "degraded" + memPhase.Errors = append(memPhase.Errors, "memory writer: "+merr.Error()) + } else { + // Confirmed findings only — suspected ones are still being + // validated and may not be real. Keeps the .memory/ log + // authoritative. + confirmed := []analyzers.Finding{} + for _, f := range findings { + if f.Status == analyzers.StatusConfirmed { + confirmed = append(confirmed, f) + } + } + if err := mw.AppendKnownRisks(runID, confirmed); err != nil { + memPhase.Status = "degraded" + memPhase.Errors = append(memPhase.Errors, "known-risks: "+err.Error()) + } + runEntry := memory.RunHistoryEntry{ + RunID: runID, + RepoPath: in.RepoPath, + StartedAt: receipt.StartedAt, + FinishedAt: time.Now().UTC().Format(time.RFC3339Nano), + TotalFindings: staticOut.Summary.Total, + Confirmed: staticOut.Summary.Confirmed, + Critical: staticOut.Summary.Critical, + High: staticOut.Summary.High, + Medium: staticOut.Summary.Medium, + Low: staticOut.Summary.Low, + LLMEnabled: in.EnableLLM, + ExitCode: res.ExitCode, + } + if err := mw.AppendRunHistory(runEntry); err != nil { + memPhase.Status = "degraded" + memPhase.Errors = append(memPhase.Errors, "run-history: "+err.Error()) + } + profile := memory.ProjectProfile{ + RepoPath: in.RepoPath, + LastSeenAt: time.Now().UTC().Format(time.RFC3339Nano), + LastSeenCommit: intake.LatestCommit, + LanguageBreakdown: intake.LanguageBreakdown, + FileCount: intake.FileCount, + } + if err := mw.WriteProjectProfile(profile); err != nil { + memPhase.Status = "degraded" + memPhase.Errors = append(memPhase.Errors, "project-profile: "+err.Error()) + } + memPhase.OutputFiles = []string{ + ".memory/known-risks.jsonl", + ".memory/run-history.jsonl", + ".memory/project-profile.json", + } + } + } else { + memPhase.Status = "skipped" + memPhase.Errors = append(memPhase.Errors, "memory disabled in review-profile") + } + receipt.Phases = append(receipt.Phases, memPhase) // --- Receipt --- receipt.Summary = staticOut.Summary diff --git a/internal/scanner/walk.go b/internal/scanner/walk.go index 1455bc7..e78563f 100644 --- a/internal/scanner/walk.go +++ b/internal/scanner/walk.go @@ -46,6 +46,11 @@ var SkipDirs = map[string]bool{ "venv": true, ".idea": true, ".vscode": true, + // Harness's own memory dir. Per PROMPT.md the harness writes to + // /.memory/ which contains JSONL findings + run history; + // scanning those would surface the harness's own audit trail + // quotes (e.g. planted-secret evidence) as new findings. + ".memory": true, } // SkipPathsAbsolute is checked when scanning the harness's own repo —