From c8b7f35c5dc77401317b21a6344f08d3d0732dae Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Wed, 15 Jul 2026 10:15:05 +0200 Subject: [PATCH 1/7] feat(session): run manifest record, terminal-state computation, coverage tracking Add an append-only run_manifest JSONL record written once by Finalize as the last line of the session file and retained in memory (Manifest() getter), so persisted and reported coverage are identical by construction. - manifest.go: RunManifest struct, pure ComputeTerminalState (table over skipped/complete/failed/partial incl. cancelled + waive), failure-class constants + ClassifyFailure. - history.go: RunMeta on SessionOptions; coverage sets tracked on SessionHistory (selected/completed/reused/failed/waived + typed failures); SetSelected/SetArtifactChecksum/MarkCancelled/RecordReviewItemWaived; Finalize computes state and writes the manifest as the final record. - persist.go: WriteRunManifest + WriteReviewItemWaived; failureClass added to review_item_failed; session_end no longer closes so the manifest stays last. - list.go: Summary.State populated from the run_manifest record. RecordReviewItemFailed gains a class param; existing agent callers pass panic/skipped_limit/ClassifyFailure(err). --- cmd/opencodereview/session_cmd_test.go | 2 +- internal/agent/agent.go | 6 +- internal/session/history.go | 211 ++++++++++++++++++++++++- internal/session/list.go | 8 + internal/session/list_test.go | 4 +- internal/session/manifest.go | 150 ++++++++++++++++++ internal/session/manifest_test.go | 138 ++++++++++++++++ internal/session/persist.go | 63 +++++++- internal/session/persist_test.go | 2 +- 9 files changed, 566 insertions(+), 18 deletions(-) create mode 100644 internal/session/manifest.go create mode 100644 internal/session/manifest_test.go diff --git a/cmd/opencodereview/session_cmd_test.go b/cmd/opencodereview/session_cmd_test.go index 1c32a71b..5fdcf31a 100644 --- a/cmd/opencodereview/session_cmd_test.go +++ b/cmd/opencodereview/session_cmd_test.go @@ -90,7 +90,7 @@ func TestRunSessionShow_Text(t *testing.T) { DiffCommit: "abc123", }) sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", []model.LlmComment{{Path: "a.go", Content: "note"}}) - sh.RecordReviewItemFailed("bad.go", "bad.go", "bad.go", "fp-bad", "boom") + sh.RecordReviewItemFailed("bad.go", "bad.go", "bad.go", "fp-bad", session.FailureProviderError, "boom") sh.Finalize() got := captureStdout(t, func() { diff --git a/internal/agent/agent.go b/internal/agent/agent.go index a50dead8..d13c2817 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -393,7 +393,7 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error defer func() { if r := recover(); r != nil { atomic.AddInt64(&a.subtaskFailed, 1) - a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath, fingerprint, fmt.Sprintf("panic: %v", r)) + a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath, fingerprint, session.FailurePanic, fmt.Sprintf("panic: %v", r)) fmt.Fprintf(stdout.Writer(), "[ocr] Subtask panic for %s: %v\n%s\n", d.NewPath, r, debug.Stack()) telemetry.ErrorEvent(ctx, "subtask.panic", fmt.Errorf("panic: %v", r), telemetry.AnyToAttr("file.path", d.NewPath)) @@ -413,7 +413,7 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error completed, skipReason, err := a.executeSubtask(fileCtx, d) if err != nil { atomic.AddInt64(&a.subtaskFailed, 1) - a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath, fingerprint, err.Error()) + a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath, fingerprint, session.ClassifyFailure(err), err.Error()) fmt.Fprintf(stdout.Writer(), "[ocr] Subtask error for %s: %v\n", d.NewPath, err) telemetry.ErrorEvent(fileCtx, "subtask.error", err, telemetry.AnyToAttr("file.path", d.NewPath)) @@ -422,7 +422,7 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error } if !completed { if skipReason != "" { - a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath, fingerprint, skipReason) + a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath, fingerprint, session.FailureSkippedLimit, skipReason) } return } diff --git a/internal/session/history.go b/internal/session/history.go index c64ac88e..2aa67fc9 100644 --- a/internal/session/history.go +++ b/internal/session/history.go @@ -49,6 +49,25 @@ type SessionHistory struct { persist *jsonlWriter FileSessions map[string]*FileSession llmFailures int64 + + // runMeta carries the cmd-layer-populated manifest metadata (version, + // provider, hashes, repo identity, resolved range SHAs). Static for the + // life of the run. + runMeta RunMeta + + // Coverage sets tracked for the run manifest. Guarded by mu. Each + // terminal review item lands in exactly one of completed/reused/failed/ + // waived; selected is the full reviewable set. sortedKeys yields the + // manifest's per-outcome path lists. + selected map[string]struct{} + completed map[string]struct{} + reused map[string]struct{} + failed map[string]struct{} + waived map[string]struct{} + failures []ManifestFailure + artifactSHA string + cancelled bool + manifest *RunManifest } // FileSession represents the conversation records for a single file subtask. @@ -103,6 +122,26 @@ type SessionOptions struct { DiffTo string DiffCommit string ResumedFrom string + // RunMeta carries cmd-layer-populated manifest metadata. Zero value is + // fine; the manifest simply omits the corresponding fields. + RunMeta RunMeta +} + +// RunMeta holds the run-identity metadata that only the command layer can +// populate (tool version, resolved provider, config/rules hashes, repo +// identity, resolved range SHAs). It is threaded into SessionOptions and +// folded into the run manifest at Finalize. +type RunMeta struct { + OCRVersion string + Provider string + Concurrency int + ConfigHash string + RulesHash string + RepoRemoteURL string + RepoHeadSHA string + RangeFromSHA string + RangeToSHA string + RangeCommitSHA string } // New creates a new SessionHistory with the given repo directory. @@ -120,6 +159,12 @@ func New(repoDir, gitBranch, model string, opts SessionOptions) *SessionHistory ResumedFrom: opts.ResumedFrom, StartTime: time.Now(), FileSessions: make(map[string]*FileSession), + runMeta: opts.RunMeta, + selected: make(map[string]struct{}), + completed: make(map[string]struct{}), + reused: make(map[string]struct{}), + failed: make(map[string]struct{}), + waived: make(map[string]struct{}), } p, err := newJSONLWriter(sessionID, repoDir, gitBranch, model, opts) @@ -161,6 +206,7 @@ func (sh *SessionHistory) RecordReviewItemDone(filePath, oldPath, newPath, finge } if filePath != "" { sh.GetOrCreateFileSession(filePath) + sh.trackCoverage("completed", filePath) } if p := sh.persist; p != nil { p.WriteReviewItemDone(filePath, oldPath, newPath, fingerprint, comments) @@ -177,14 +223,17 @@ func (sh *SessionHistory) RecordReviewItemReused(filePath, oldPath, newPath, fin } if filePath != "" { sh.GetOrCreateFileSession(filePath) + sh.trackCoverage("reused", filePath) } if p := sh.persist; p != nil { p.WriteReviewItemReused(filePath, oldPath, newPath, fingerprint, sourceSessionID, comments) } } -// RecordReviewItemFailed persists an incomplete file-level checkpoint. -func (sh *SessionHistory) RecordReviewItemFailed(filePath, oldPath, newPath, fingerprint, errorMsg string) { +// RecordReviewItemFailed persists an incomplete file-level checkpoint. class +// is one of the Failure* constants (see ClassifyFailure); it is recorded in +// the JSONL record and in the run manifest's typed failure list. +func (sh *SessionHistory) RecordReviewItemFailed(filePath, oldPath, newPath, fingerprint, class, errorMsg string) { if sh == nil { return } @@ -193,9 +242,85 @@ func (sh *SessionHistory) RecordReviewItemFailed(filePath, oldPath, newPath, fin } if filePath != "" { sh.GetOrCreateFileSession(filePath) + sh.mu.Lock() + sh.failed[filePath] = struct{}{} + sh.failures = append(sh.failures, ManifestFailure{Path: filePath, Class: class, Error: errorMsg}) + sh.mu.Unlock() } if p := sh.persist; p != nil { - p.WriteReviewItemFailed(filePath, oldPath, newPath, fingerprint, errorMsg) + p.WriteReviewItemFailed(filePath, oldPath, newPath, fingerprint, class, errorMsg) + } +} + +// RecordReviewItemWaived persists a review_item_waived checkpoint for a diff +// the operator chose to skip on a resumed run. A waive satisfies the coverage +// contract (counts like a completed item) and is reusable by later resumes. +func (sh *SessionHistory) RecordReviewItemWaived(filePath, oldPath, newPath, fingerprint string) { + if sh == nil { + return + } + if filePath == "" { + filePath = newPath + } + if filePath != "" { + sh.GetOrCreateFileSession(filePath) + sh.trackCoverage("waived", filePath) + } + if p := sh.persist; p != nil { + p.WriteReviewItemWaived(filePath, oldPath, newPath, fingerprint) + } +} + +// SetSelected records the full set of reviewable file paths for this run. It +// establishes the coverage denominator: selected == completed + reused + +// failed + waived. Call it after diffs are computed and before dispatch. +func (sh *SessionHistory) SetSelected(paths []string) { + if sh == nil { + return + } + sh.mu.Lock() + for _, p := range paths { + if p != "" { + sh.selected[p] = struct{}{} + } + } + sh.mu.Unlock() +} + +// SetArtifactChecksum records the checksum identifying the run's input set +// (sha256 over the sorted per-file fingerprints). It proves input identity +// without persisting source. +func (sh *SessionHistory) SetArtifactChecksum(sum string) { + if sh == nil { + return + } + sh.mu.Lock() + sh.artifactSHA = sum + sh.mu.Unlock() +} + +// MarkCancelled flags the run as cancelled so ComputeTerminalState degrades +// the state to partial/failed rather than reporting complete. +func (sh *SessionHistory) MarkCancelled() { + if sh == nil { + return + } + sh.mu.Lock() + sh.cancelled = true + sh.mu.Unlock() +} + +// trackCoverage adds a path to one of the coverage sets under lock. +func (sh *SessionHistory) trackCoverage(set, path string) { + sh.mu.Lock() + defer sh.mu.Unlock() + switch set { + case "completed": + sh.completed[path] = struct{}{} + case "reused": + sh.reused[path] = struct{}{} + case "waived": + sh.waived[path] = struct{}{} } } @@ -211,11 +336,91 @@ func (sh *SessionHistory) Finalize() { filesReviewed = append(filesReviewed, fp) } failures := atomic.LoadInt64(&sh.llmFailures) + manifest := sh.buildManifestLocked(duration) + sh.manifest = manifest sh.mu.Unlock() if p != nil { + // session_end first, then the run_manifest as the final line so the + // manifest is always the last record (immutability = written once, + // last). WriteRunManifest flushes and closes the file. p.WriteSessionEnd(duration, filesReviewed, failures) + p.WriteRunManifest(manifest) + p.flushAndClose() + } +} + +// buildManifestLocked assembles the immutable run manifest from the tracked +// coverage sets and run metadata. Caller must hold sh.mu. +func (sh *SessionHistory) buildManifestLocked(duration time.Duration) *RunManifest { + // selected is the coverage denominator; union in the recorded outcomes so + // the invariant holds even when SetSelected was not called explicitly. + selected := make(map[string]struct{}, len(sh.selected)) + for p := range sh.selected { + selected[p] = struct{}{} + } + for _, set := range []map[string]struct{}{sh.completed, sh.reused, sh.failed, sh.waived} { + for p := range set { + selected[p] = struct{}{} + } + } + + state := ComputeTerminalState(len(selected), len(sh.completed), len(sh.reused), + len(sh.failed), len(sh.waived), sh.cancelled) + + m := &RunManifest{ + SchemaVersion: ManifestSchemaVersion, + SessionID: sh.SessionID, + ParentSessionID: sh.ResumedFrom, + Repo: ManifestRepo{ + RemoteURL: sh.runMeta.RepoRemoteURL, + HeadSHA: sh.runMeta.RepoHeadSHA, + Branch: sh.GitBranch, + Dir: sh.RepoDir, + }, + ReviewMode: sh.ReviewMode, + Range: ManifestRange{ + From: sh.DiffFrom, + To: sh.DiffTo, + Commit: sh.DiffCommit, + FromSHA: sh.runMeta.RangeFromSHA, + ToSHA: sh.runMeta.RangeToSHA, + CommitSHA: sh.runMeta.RangeCommitSHA, + }, + OCRVersion: sh.runMeta.OCRVersion, + Provider: sh.runMeta.Provider, + Model: sh.Model, + Concurrency: sh.runMeta.Concurrency, + ConfigHash: sh.runMeta.ConfigHash, + RulesHash: sh.runMeta.RulesHash, + ArtifactSHA256: sh.artifactSHA, + Files: ManifestFiles{ + Selected: sortedKeys(selected), + Completed: sortedKeys(sh.completed), + Reused: sortedKeys(sh.reused), + Failed: sortedKeys(sh.failed), + Waived: sortedKeys(sh.waived), + }, + Failures: append([]ManifestFailure(nil), sh.failures...), + State: state, + StartedAt: sh.StartTime.UTC().Format(time.RFC3339), + CompletedAt: sh.EndTime.UTC().Format(time.RFC3339), + DurationMS: duration.Milliseconds(), } + return m +} + +// Manifest returns the run manifest retained in memory after Finalize, or nil +// if the session has not been finalized yet. The returned value is the same +// data written as the last JSONL record, so JSON output and the persisted +// session expose identical coverage. +func (sh *SessionHistory) Manifest() *RunManifest { + if sh == nil { + return nil + } + sh.mu.Lock() + defer sh.mu.Unlock() + return sh.manifest } // AppendTaskRecord adds a new task record to the file session for the given diff --git a/internal/session/list.go b/internal/session/list.go index 3e91494c..05fb4fb4 100644 --- a/internal/session/list.go +++ b/internal/session/list.go @@ -34,6 +34,9 @@ type Summary struct { TotalComments int `json:"total_comments"` LLMFailures int64 `json:"llm_failures"` Aborted bool `json:"aborted"` + // State is the run manifest's terminal state (complete/partial/failed/ + // skipped) when the session recorded a run_manifest; empty otherwise. + State string `json:"state,omitempty"` } // ItemDetail describes one file-level record within a session, used by `ocr session show`. @@ -72,6 +75,7 @@ type summaryRecord struct { FilesReviewed []string `json:"files_reviewed"` DurationSeconds float64 `json:"duration_seconds"` LLMFailures int64 `json:"llm_failures"` + State string `json:"state"` } // SessionsDir returns the on-disk directory that holds JSONL session files @@ -227,6 +231,10 @@ func applyRecordToSummary(s *Summary, rec summaryRecord) { s.TotalComments += countCommentsRaw(rec.Comments) case "review_item_failed": s.FailedFiles++ + case "run_manifest": + if rec.State != "" { + s.State = rec.State + } case "session_end": s.Aborted = false if s.CompletedFiles == 0 && s.ReusedFiles == 0 && s.FailedFiles == 0 && len(rec.FilesReviewed) > 0 { diff --git a/internal/session/list_test.go b/internal/session/list_test.go index 2c5c3946..076d5f4b 100644 --- a/internal/session/list_test.go +++ b/internal/session/list_test.go @@ -77,7 +77,7 @@ func TestLoadDetail_ReturnsItems(t *testing.T) { }) sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", []model.LlmComment{{Path: "a.go", Content: "note"}}) sh.RecordReviewItemReused("b.go", "b.go", "b.go", "fp-b", "prior-session", []model.LlmComment{{Path: "b.go", Content: "cached"}}) - sh.RecordReviewItemFailed("c.go", "c.go", "c.go", "fp-c", "boom") + sh.RecordReviewItemFailed("c.go", "c.go", "c.go", "fp-c", FailureProviderError, "boom") sh.Finalize() summary, items, err := LoadDetail(repoDir, sh.SessionID) @@ -166,7 +166,7 @@ func writeTestSession(t *testing.T, repoDir, from, to string, comments []model.L } for i := 0; i < failedCount; i++ { filePath := "failed-" + filepath.Base(t.TempDir()) + ".go" - sh.RecordReviewItemFailed(filePath, filePath, filePath, "fp-fail-"+filePath, "test error") + sh.RecordReviewItemFailed(filePath, filePath, filePath, "fp-fail-"+filePath, FailureProviderError, "test error") } if finalize { sh.Finalize() diff --git a/internal/session/manifest.go b/internal/session/manifest.go new file mode 100644 index 00000000..1e3dc986 --- /dev/null +++ b/internal/session/manifest.go @@ -0,0 +1,150 @@ +package session + +import ( + "context" + "errors" + "sort" +) + +// Failure classes recorded per failed review item. ClassifyFailure maps a Go +// error onto one of these; panic and skipped_limit are set explicitly by the +// dispatch paths that detect them. +const ( + FailureProviderError = "provider_error" + FailureTimeout = "timeout" + FailureCancelled = "cancelled" + FailurePanic = "panic" + FailureSkippedLimit = "skipped_limit" +) + +// ClassifyFailure maps an error to a failure class using the standard context +// error chain. Anything that is not a timeout/cancellation is treated as a +// provider error (the common case: LLM/API failure). +func ClassifyFailure(err error) string { + switch { + case err == nil: + return FailureProviderError + case errors.Is(err, context.DeadlineExceeded): + return FailureTimeout + case errors.Is(err, context.Canceled): + return FailureCancelled + default: + return FailureProviderError + } +} + +// ManifestSchemaVersion is the current run_manifest schema version. Bump it +// only on a breaking change to the field set so downstream readers can gate. +const ManifestSchemaVersion = 1 + +// Terminal run states. These are the machine contract surfaced in the run +// manifest and in `--format json`; they are independent of the legacy JSON +// `status` field and of the process exit code (which stays 0 for partial). +const ( + StateComplete = "complete" + StatePartial = "partial" + StateFailed = "failed" + StateSkipped = "skipped" +) + +// ManifestRepo identifies the repository under review. +type ManifestRepo struct { + RemoteURL string `json:"remote_url,omitempty"` + HeadSHA string `json:"head_sha,omitempty"` + Branch string `json:"branch,omitempty"` + Dir string `json:"dir,omitempty"` +} + +// ManifestRange records the review range and its resolved commit SHAs. +type ManifestRange struct { + From string `json:"from,omitempty"` + To string `json:"to,omitempty"` + Commit string `json:"commit,omitempty"` + FromSHA string `json:"from_sha,omitempty"` + ToSHA string `json:"to_sha,omitempty"` + CommitSHA string `json:"commit_sha,omitempty"` +} + +// ManifestFiles holds the per-outcome coverage sets, each a sorted, de-duped +// list of file paths. selected == completed + reused + failed + waived is the +// coverage invariant the manifest asserts. +type ManifestFiles struct { + Selected []string `json:"selected"` + Completed []string `json:"completed"` + Reused []string `json:"reused"` + Failed []string `json:"failed"` + Waived []string `json:"waived"` +} + +// ManifestFailure is one failed review item with its typed class. +type ManifestFailure struct { + Path string `json:"path"` + Class string `json:"class"` + Error string `json:"error,omitempty"` +} + +// RunManifest is the versioned, immutable summary of one review/scan run. It +// is written exactly once as the last line of the session JSONL file and is +// surfaced verbatim in `--format json` — the same Go value serialized both +// places, so persisted and reported coverage are identical by construction. +type RunManifest struct { + SchemaVersion int `json:"schema_version"` + SessionID string `json:"session_id"` + ParentSessionID string `json:"parent_session_id,omitempty"` + Repo ManifestRepo `json:"repo"` + ReviewMode string `json:"review_mode,omitempty"` + Range ManifestRange `json:"range"` + OCRVersion string `json:"ocr_version,omitempty"` + Provider string `json:"provider,omitempty"` + Model string `json:"model,omitempty"` + Concurrency int `json:"concurrency,omitempty"` + ConfigHash string `json:"config_hash,omitempty"` + RulesHash string `json:"rules_hash,omitempty"` + ArtifactSHA256 string `json:"artifact_sha256,omitempty"` + Files ManifestFiles `json:"files"` + Failures []ManifestFailure `json:"failures,omitempty"` + State string `json:"state"` + StartedAt string `json:"started_at,omitempty"` + CompletedAt string `json:"completed_at,omitempty"` + DurationMS int64 `json:"duration_ms"` +} + +// ComputeTerminalState derives the run's terminal state from the coverage +// counts. It is pure so it can be table-tested exhaustively. +// +// Rules (checked in order): +// - nothing selected -> skipped +// - cancelled -> partial if any item succeeded, else failed +// - every selected item failed -> failed +// - completed+reused+waived covers selected with no failures -> complete +// (a waive satisfies the coverage contract, same as a completed review) +// - anything else (mixed outcomes) -> partial +func ComputeTerminalState(selected, completed, reused, failed, waived int, cancelled bool) string { + if selected == 0 { + return StateSkipped + } + if cancelled { + if completed+reused > 0 { + return StatePartial + } + return StateFailed + } + if failed == selected { + return StateFailed + } + if failed == 0 && completed+reused+waived >= selected { + return StateComplete + } + return StatePartial +} + +// sortedKeys returns the map keys as a sorted slice. Always non-nil so the +// manifest emits [] rather than null for empty sets. +func sortedKeys(m map[string]struct{}) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/internal/session/manifest_test.go b/internal/session/manifest_test.go new file mode 100644 index 00000000..b8c30f58 --- /dev/null +++ b/internal/session/manifest_test.go @@ -0,0 +1,138 @@ +package session + +import ( + "bufio" + "encoding/json" + "os" + "strings" + "testing" +) + +func TestComputeTerminalState(t *testing.T) { + tests := []struct { + name string + selected, completed, reused, failed, waived int + cancelled bool + want string + }{ + {"nothing selected", 0, 0, 0, 0, 0, false, StateSkipped}, + {"all completed", 3, 3, 0, 0, 0, false, StateComplete}, + {"all reused", 2, 0, 2, 0, 0, false, StateComplete}, + {"completed + reused covers", 4, 2, 2, 0, 0, false, StateComplete}, + {"waive completes the set", 3, 2, 0, 0, 1, false, StateComplete}, + {"all waived", 2, 0, 0, 0, 2, false, StateComplete}, + {"all failed", 3, 0, 0, 3, 0, false, StateFailed}, + {"mixed success and failure", 3, 2, 0, 1, 0, false, StatePartial}, + {"mixed with waive but a failure remains", 4, 1, 1, 1, 1, false, StatePartial}, + {"cancelled with a success", 5, 2, 0, 0, 0, true, StatePartial}, + {"cancelled with a reused success", 5, 0, 1, 0, 0, true, StatePartial}, + {"cancelled with nothing done", 5, 0, 0, 0, 0, true, StateFailed}, + {"cancelled with only failures", 5, 0, 0, 2, 0, true, StateFailed}, + {"cancelled overrides otherwise-complete", 2, 2, 0, 0, 0, true, StatePartial}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ComputeTerminalState(tt.selected, tt.completed, tt.reused, tt.failed, tt.waived, tt.cancelled) + if got != tt.want { + t.Errorf("ComputeTerminalState(%d,%d,%d,%d,%d,%v) = %q, want %q", + tt.selected, tt.completed, tt.reused, tt.failed, tt.waived, tt.cancelled, got, tt.want) + } + }) + } +} + +func TestClassifyFailure(t *testing.T) { + // context errors are covered indirectly; assert the plain mapping here. + if got := ClassifyFailure(nil); got != FailureProviderError { + t.Errorf("nil err = %q", got) + } +} + +// lastJSONLRecord returns the last non-empty JSONL record in a session file. +func lastJSONLRecord(t *testing.T, path string) map[string]any { + t.Helper() + f, err := os.Open(path) + if err != nil { + t.Fatalf("open %s: %v", path, err) + } + defer f.Close() + var last map[string]any + sc := bufio.NewScanner(f) + sc.Buffer(make([]byte, 0, 1024*1024), 8*1024*1024) + for sc.Scan() { + line := strings.TrimSpace(sc.Text()) + if line == "" { + continue + } + var rec map[string]any + if err := json.Unmarshal([]byte(line), &rec); err != nil { + t.Fatalf("bad JSONL line %q: %v", line, err) + } + last = rec + } + return last +} + +func TestFinalizeWritesRunManifest(t *testing.T) { + sh := New(t.TempDir(), "main", "test-model", SessionOptions{ + ReviewMode: ReviewModeRange, + DiffFrom: "base", + DiffTo: "head", + RunMeta: RunMeta{ + OCRVersion: "1.2.3", + Provider: "anthropic", + Concurrency: 4, + ConfigHash: "cfg-hash", + RulesHash: "rules-hash", + }, + }) + sh.SetSelected([]string{"a.go", "b.go", "c.go"}) + sh.SetArtifactChecksum("artifact-sum") + sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", nil) + sh.RecordReviewItemReused("b.go", "b.go", "b.go", "fp-b", "old-session", nil) + sh.RecordReviewItemFailed("c.go", "c.go", "c.go", "fp-c", FailureProviderError, "boom") + sh.Finalize() + + // In-memory manifest matches what we persisted. + m := sh.Manifest() + if m == nil { + t.Fatal("Manifest() returned nil after Finalize") + } + if m.State != StatePartial { + t.Errorf("state = %q, want partial", m.State) + } + if m.SchemaVersion != ManifestSchemaVersion { + t.Errorf("schema_version = %d", m.SchemaVersion) + } + if len(m.Files.Selected) != 3 || len(m.Files.Completed) != 1 || len(m.Files.Reused) != 1 || len(m.Files.Failed) != 1 { + t.Errorf("coverage = %+v", m.Files) + } + if len(m.Failures) != 1 || m.Failures[0].Class != FailureProviderError { + t.Errorf("failures = %+v", m.Failures) + } + + // The LAST JSONL line is the run_manifest record with matching coverage. + path, err := SessionFilePath(sh.RepoDir, sh.SessionID) + if err != nil { + t.Fatal(err) + } + rec := lastJSONLRecord(t, path) + if rec["type"] != "run_manifest" { + t.Fatalf("last record type = %v, want run_manifest", rec["type"]) + } + if rec["state"] != StatePartial { + t.Errorf("persisted state = %v", rec["state"]) + } + + // No token/secret content leaks into the manifest record. The endpoint + // token / auth header must never appear anywhere in the serialized line. + blob, err := json.Marshal(rec) + if err != nil { + t.Fatal(err) + } + for _, bad := range []string{"api_key", "auth_token", "sk-ant", "authorization", "x-api-key", "Bearer"} { + if strings.Contains(strings.ToLower(string(blob)), strings.ToLower(bad)) { + t.Errorf("manifest leaks sensitive token pattern %q: %s", bad, blob) + } + } +} diff --git a/internal/session/persist.go b/internal/session/persist.go index 464da1b6..f3f00d45 100644 --- a/internal/session/persist.go +++ b/internal/session/persist.go @@ -165,20 +165,29 @@ func (jw *jsonlWriter) WriteSessionStart(startTime time.Time) string { // WriteReviewItemDone writes a file-level resume checkpoint for a completed diff. func (jw *jsonlWriter) WriteReviewItemDone(filePath, oldPath, newPath, fingerprint string, comments []model.LlmComment) string { - return jw.writeReviewItemRecord("review_item_done", filePath, oldPath, newPath, fingerprint, "", "", comments) + return jw.writeReviewItemRecord("review_item_done", filePath, oldPath, newPath, fingerprint, "", "", "", comments) } // WriteReviewItemReused writes a checkpoint reused from a previous session. func (jw *jsonlWriter) WriteReviewItemReused(filePath, oldPath, newPath, fingerprint, sourceSessionID string, comments []model.LlmComment) string { - return jw.writeReviewItemRecord("review_item_reused", filePath, oldPath, newPath, fingerprint, sourceSessionID, "", comments) + return jw.writeReviewItemRecord("review_item_reused", filePath, oldPath, newPath, fingerprint, sourceSessionID, "", "", comments) } // WriteReviewItemFailed writes a file-level checkpoint for a failed diff. -func (jw *jsonlWriter) WriteReviewItemFailed(filePath, oldPath, newPath, fingerprint, errorMsg string) string { - return jw.writeReviewItemRecord("review_item_failed", filePath, oldPath, newPath, fingerprint, "", errorMsg, nil) +// class is one of the session.Failure* constants; it is persisted as +// failureClass for downstream classification. +func (jw *jsonlWriter) WriteReviewItemFailed(filePath, oldPath, newPath, fingerprint, class, errorMsg string) string { + return jw.writeReviewItemRecord("review_item_failed", filePath, oldPath, newPath, fingerprint, "", class, errorMsg, nil) } -func (jw *jsonlWriter) writeReviewItemRecord(recordType, filePath, oldPath, newPath, fingerprint, sourceSessionID, errorMsg string, comments []model.LlmComment) string { +// WriteReviewItemWaived writes a checkpoint for a diff the operator waived on +// a resumed run. It carries no comments and no error; it counts toward +// coverage and is reusable by later resumes. +func (jw *jsonlWriter) WriteReviewItemWaived(filePath, oldPath, newPath, fingerprint string) string { + return jw.writeReviewItemRecord("review_item_waived", filePath, oldPath, newPath, fingerprint, "", "", "", nil) +} + +func (jw *jsonlWriter) writeReviewItemRecord(recordType, filePath, oldPath, newPath, fingerprint, sourceSessionID, failureClass, errorMsg string, comments []model.LlmComment) string { uuid := generateUUID() jw.mu.Lock() @@ -201,6 +210,9 @@ func (jw *jsonlWriter) writeReviewItemRecord(recordType, filePath, oldPath, newP if sourceSessionID != "" { rec["sourceSessionId"] = sourceSessionID } + if failureClass != "" { + rec["failureClass"] = failureClass + } if errorMsg != "" { rec["error"] = errorMsg } @@ -312,6 +324,41 @@ func (jw *jsonlWriter) WriteToolCall(filePath string, taskType TaskType, toolNam return uuid } +// WriteRunManifest writes the run_manifest record as a JSONL line. The +// manifest struct is serialized verbatim (the same value retained in memory +// and surfaced in --format json), then wrapped with the standard record +// envelope (uuid/parentUuid/type/sessionId/timestamp). +func (jw *jsonlWriter) WriteRunManifest(m *RunManifest) string { + if m == nil { + return "" + } + data, err := json.Marshal(m) + if err != nil { + fmt.Printf("[ocr session] failed to marshal run manifest: %v\n", err) + return "" + } + rec := make(map[string]any) + if err := json.Unmarshal(data, &rec); err != nil { + fmt.Printf("[ocr session] failed to encode run manifest: %v\n", err) + return "" + } + + uuid := generateUUID() + jw.mu.Lock() + defer jw.mu.Unlock() + rec["uuid"] = uuid + rec["parentUuid"] = jw.lastUUID + rec["type"] = "run_manifest" + rec["sessionId"] = jw.sessionID + rec["timestamp"] = time.Now().UTC().Format(time.RFC3339) + jw.writeRecordLocked(rec) + if jw.writer != nil { + jw.writer.Flush() + } + jw.lastUUID = uuid + return uuid +} + // WriteSessionEnd writes the final session_end summary record and closes the file. func (jw *jsonlWriter) WriteSessionEnd(duration time.Duration, filesReviewed []string, llmFailures int64) { uuid := generateUUID() @@ -334,9 +381,9 @@ func (jw *jsonlWriter) WriteSessionEnd(duration time.Duration, filesReviewed []s if jw.writer != nil { jw.writer.Flush() } - if jw.file != nil { - jw.file.Close() - } + // NOTE: the file is intentionally not closed here. Finalize writes the + // run_manifest as the final record after session_end, then closes via + // flushAndClose. Closing here would truncate the manifest. } func (jw *jsonlWriter) flushAndClose() { diff --git a/internal/session/persist_test.go b/internal/session/persist_test.go index bb9119d0..5c69efac 100644 --- a/internal/session/persist_test.go +++ b/internal/session/persist_test.go @@ -285,7 +285,7 @@ func TestReviewItemResumeRoundTrip(t *testing.T) { ExistingCode: "old()", }} sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", comments) - sh.RecordReviewItemFailed("b.go", "b.go", "b.go", "fp-b", "rate limit") + sh.RecordReviewItemFailed("b.go", "b.go", "b.go", "fp-b", FailureProviderError, "rate limit") sh.Finalize() state, err := LoadResumeState(repoDir, sh.SessionID) From 9411ee4bb5b8280261a9abd5a2455c402cba9522 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Wed, 15 Jul 2026 10:19:35 +0200 Subject: [PATCH 2/7] feat(agent): selected set, artifact checksum, cancellation marking Wire the run-manifest coverage denominator into the diff-review path: - recordSelectionAndArtifact records the non-deleted reviewable set (SetSelected) and an order-independent sha256 over the sorted per-file fingerprints (SetArtifactChecksum) before resume/dispatch, so selected == completed + reused + failed + waived holds. - Run marks the session cancelled when the context is cancelled. Failure classes (panic/skipped_limit/ClassifyFailure) were threaded through the dispatch paths in the prior commit. Adds a stub-LLM table test over success/mixed/timeout/cancel/panic/skip asserting manifest state + classes, plus an artifact-checksum order-independence test. --- internal/agent/agent.go | 29 +++++ internal/agent/manifest_test.go | 220 ++++++++++++++++++++++++++++++++ 2 files changed, 249 insertions(+) create mode 100644 internal/agent/manifest_test.go diff --git a/internal/agent/agent.go b/internal/agent/agent.go index d13c2817..5e698813 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -4,8 +4,10 @@ import ( "context" "crypto/sha256" "encoding/json" + "errors" "fmt" "runtime/debug" + "sort" "strings" "sync" "sync/atomic" @@ -231,6 +233,9 @@ func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { if len(comments) > 0 { telemetry.RecordCommentsGenerated(ctx, int64(len(comments))) } + if ctx.Err() != nil || errors.Is(err, context.Canceled) { + a.session.MarkCancelled() + } a.session.Finalize() return comments, err } @@ -359,6 +364,10 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error if len(a.diffs) == 0 { return nil, fmt.Errorf("all diffs filtered out by token size") } + // Establish the coverage denominator and input-identity checksum before + // resume/dispatch so the manifest invariant selected == completed + reused + // + failed + waived holds. + a.recordSelectionAndArtifact() toDispatch := a.applyResume(a.diffs) var wg sync.WaitGroup @@ -511,6 +520,26 @@ func reviewItemFingerprint(mode string, d model.Diff) string { return fmt.Sprintf("%x", sum) } +// recordSelectionAndArtifact records the reviewable file set on the session +// (coverage denominator) and an artifact checksum over the sorted per-file +// fingerprints. The checksum proves input identity without persisting source. +func (a *Agent) recordSelectionAndArtifact() { + mode := a.reviewMode() + selected := make([]string, 0, len(a.diffs)) + fps := make([]string, 0, len(a.diffs)) + for _, d := range a.diffs { + if d.IsDeleted { + continue + } + selected = append(selected, effectivePath(d)) + fps = append(fps, reviewItemFingerprint(mode, d)) + } + a.session.SetSelected(selected) + sort.Strings(fps) + sum := sha256.Sum256([]byte(strings.Join(fps, "\x00"))) + a.session.SetArtifactChecksum(fmt.Sprintf("%x", sum)) +} + func resumedFromSession(resume *session.ResumeState) string { if resume == nil { return "" diff --git a/internal/agent/manifest_test.go b/internal/agent/manifest_test.go new file mode 100644 index 00000000..2c1cdb88 --- /dev/null +++ b/internal/agent/manifest_test.go @@ -0,0 +1,220 @@ +package agent + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + "github.com/open-code-review/open-code-review/internal/config/template" + "github.com/open-code-review/open-code-review/internal/llm" + "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" + "github.com/open-code-review/open-code-review/internal/tool" +) + +// routeClient dispatches on marker strings embedded in the rendered prompt so +// a single stateless client can drive success / fail / timeout / cancel / +// panic per file within one concurrent batch. +type routeClient struct{} + +func (routeClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequest) (*llm.ChatResponse, error) { + var sb strings.Builder + for _, m := range req.Messages { + sb.WriteString(m.ExtractText()) + } + text := sb.String() + switch { + case strings.Contains(text, "DO_PANIC"): + panic("boom in subtask") + case strings.Contains(text, "DO_TIMEOUT"): + return nil, fmt.Errorf("llm call: %w", context.DeadlineExceeded) + case strings.Contains(text, "DO_CANCEL"): + return nil, fmt.Errorf("llm call: %w", context.Canceled) + case strings.Contains(text, "DO_FAIL"): + return nil, errors.New("provider exploded") + default: + content := "" + return &llm.ChatResponse{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{ + Content: &content, + ToolCalls: []llm.ToolCall{{ + ID: "c1", Type: "function", + Function: llm.FunctionCall{Name: "task_done", Arguments: "{}"}, + }}, + }}}, + Model: "fake", + Usage: &llm.UsageInfo{PromptTokens: 5, CompletionTokens: 2}, + }, nil + } +} + +func classCounts(m *session.RunManifest) map[string]int { + out := map[string]int{} + for _, f := range m.Failures { + out[f.Class]++ + } + return out +} + +func TestRunManifest_Scenarios(t *testing.T) { + bigTpl := strings.Repeat("word ", 120) + " {{diff}}" + + tests := []struct { + name string + maxTokens int + mainContent string + diffs []model.Diff + markCancelled bool + wantState string + wantSelected int + wantClasses map[string]int + }{ + { + name: "success", + maxTokens: 100000, + mainContent: "review {{diff}}", + diffs: []model.Diff{{NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1}}, + wantState: session.StateComplete, + wantSelected: 1, + wantClasses: map[string]int{}, + }, + { + name: "mixed success and provider failure", + maxTokens: 100000, + mainContent: "review {{diff}}", + diffs: []model.Diff{ + {NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1}, + {NewPath: "bad.go", OldPath: "bad.go", Diff: "+DO_FAIL", Insertions: 1}, + }, + wantState: session.StatePartial, + wantSelected: 2, + wantClasses: map[string]int{session.FailureProviderError: 1}, + }, + { + name: "timeout classified", + maxTokens: 100000, + mainContent: "review {{diff}}", + diffs: []model.Diff{{NewPath: "slow.go", OldPath: "slow.go", Diff: "+DO_TIMEOUT", Insertions: 1}}, + wantState: session.StateFailed, + wantSelected: 1, + wantClasses: map[string]int{session.FailureTimeout: 1}, + }, + { + name: "cancel with a success is partial", + maxTokens: 100000, + mainContent: "review {{diff}}", + diffs: []model.Diff{ + {NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1}, + {NewPath: "gone.go", OldPath: "gone.go", Diff: "+DO_CANCEL", Insertions: 1}, + }, + markCancelled: true, + wantState: session.StatePartial, + wantSelected: 2, + wantClasses: map[string]int{session.FailureCancelled: 1}, + }, + { + name: "panic isolated and classified", + maxTokens: 100000, + mainContent: "review {{diff}}", + diffs: []model.Diff{{NewPath: "panic.go", OldPath: "panic.go", Diff: "+DO_PANIC", Insertions: 1}}, + wantState: session.StateFailed, + wantSelected: 1, + wantClasses: map[string]int{session.FailurePanic: 1}, + }, + { + name: "skipped by token limit", + maxTokens: 50, + mainContent: bigTpl, + diffs: []model.Diff{{NewPath: "huge.go", OldPath: "huge.go", Diff: "+x", Insertions: 1}}, + wantState: session.StateFailed, + wantSelected: 1, + wantClasses: map[string]int{session.FailureSkippedLimit: 1}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange}) + a := New(Args{ + LLMClient: routeClient{}, + Model: "test", + Session: sess, + Tools: tool.NewRegistry(), + Template: template.Template{ + MaxTokens: tt.maxTokens, + MaxToolRequestTimes: 5, + MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: tt.mainContent}}}, + }, + MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done", Description: "done"}}}, + }) + a.currentDate = "2026-07-15 08:00" + a.diffs = tt.diffs + + _, _ = a.dispatchSubtasks(context.Background()) + if tt.markCancelled { + sess.MarkCancelled() + } + sess.Finalize() + + m := sess.Manifest() + if m == nil { + t.Fatal("nil manifest") + } + if m.State != tt.wantState { + t.Errorf("state = %q, want %q (files=%+v)", m.State, tt.wantState, m.Files) + } + if len(m.Files.Selected) != tt.wantSelected { + t.Errorf("selected = %v, want %d", m.Files.Selected, tt.wantSelected) + } + if m.ArtifactSHA256 == "" { + t.Error("artifact checksum should be recorded") + } + got := classCounts(m) + if len(got) != len(tt.wantClasses) { + t.Errorf("failure classes = %v, want %v", got, tt.wantClasses) + } + for k, v := range tt.wantClasses { + if got[k] != v { + t.Errorf("class %q count = %d, want %d (all=%v)", k, got[k], v, got) + } + } + }) + } +} + +// TestArtifactChecksumStableAndOrderIndependent verifies the artifact hash is +// deterministic regardless of diff ordering (fingerprints are sorted). +func TestArtifactChecksumStableAndOrderIndependent(t *testing.T) { + mk := func(order []model.Diff) string { + sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange}) + a := New(Args{ + LLMClient: routeClient{}, + Model: "test", + Session: sess, + Tools: tool.NewRegistry(), + Template: template.Template{ + MaxTokens: 100000, + MaxToolRequestTimes: 5, + MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}}, + }, + MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done"}}}, + }) + a.currentDate = "d" + a.diffs = order + a.recordSelectionAndArtifact() + sess.Finalize() + return sess.Manifest().ArtifactSHA256 + } + d1 := model.Diff{NewPath: "a.go", OldPath: "a.go", Diff: "+a", Insertions: 1} + d2 := model.Diff{NewPath: "b.go", OldPath: "b.go", Diff: "+b", Insertions: 1} + forward := mk([]model.Diff{d1, d2}) + reverse := mk([]model.Diff{d2, d1}) + if forward != reverse { + t.Errorf("artifact checksum order-dependent: %q vs %q", forward, reverse) + } + if forward == "" { + t.Error("empty checksum") + } +} From a6ff4a1e79d99b6a119837f8427ef69e19c2d5b2 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Wed, 15 Jul 2026 10:23:16 +0200 Subject: [PATCH 3/7] feat(cmd): populate run metadata (version, provider, hashes, repo identity, resolved range) runmeta.go builds session.RunMeta for review and scan: - config_hash: sha256 over an allowlisted, non-secret field set {provider_protocol, model, base_url_host (userinfo+query stripped), language, timeout}. Token/AuthHeader/ExtraHeaders are never in the input, so the hash is stable across key rotation. - rules_hash: sha256 over (label \0 raw bytes) of each loaded rule layer (custom/project/global) plus a label+version marker for the embedded system layer. - repo identity: origin remote URL with userinfo stripped + HEAD SHA. - resolved range SHAs rev-parsed from --from/--to/--commit. Threaded via agent.Args.RunMeta / scan.Args.RunMeta into session.New. llmRuntime now retains the resolved endpoint for non-secret hash inputs. Tests cover config-hash redaction/invariance, userinfo stripping, resolved range SHAs, and rules-hash sensitivity. --- cmd/opencodereview/review_cmd.go | 8 ++ cmd/opencodereview/runmeta.go | 166 +++++++++++++++++++++++++++++ cmd/opencodereview/runmeta_test.go | 159 +++++++++++++++++++++++++++ cmd/opencodereview/scan_cmd.go | 8 ++ cmd/opencodereview/shared.go | 4 + internal/agent/agent.go | 7 ++ internal/scan/agent.go | 4 + 7 files changed, 356 insertions(+) create mode 100644 cmd/opencodereview/runmeta.go create mode 100644 cmd/opencodereview/runmeta_test.go diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 8cf53275..f846e2f1 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -98,6 +98,13 @@ func runReview(args []string) error { rt.PlanToolDefs = append(rt.PlanToolDefs, mcpToolDefs...) rt.MainToolDefs = append(rt.MainToolDefs, mcpToolDefs...) + var lang string + if rt.AppCfg != nil { + lang = rt.AppCfg.Language + } + runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, opts.rulePath, opts.concurrency, + resolveRange(cc.RepoDir, opts.from, opts.to, opts.commit)) + ag := agent.New(agent.Args{ RepoDir: cc.RepoDir, From: opts.from, @@ -119,6 +126,7 @@ func runReview(args []string) error { Background: opts.background, GitRunner: cc.GitRunner, Resume: resumeState, + RunMeta: runMeta, }) // Silence progress output during execution; restored before the trace diff --git a/cmd/opencodereview/runmeta.go b/cmd/opencodereview/runmeta.go new file mode 100644 index 00000000..ebac2f9b --- /dev/null +++ b/cmd/opencodereview/runmeta.go @@ -0,0 +1,166 @@ +package main + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "net/url" + "os" + "path/filepath" + "strings" + + "github.com/open-code-review/open-code-review/internal/llm" + "github.com/open-code-review/open-code-review/internal/session" +) + +// configHashInput is the ALLOWLISTED field set folded into config_hash. It +// deliberately excludes every secret-bearing field of the resolved endpoint +// (Token, AuthHeader, ExtraHeaders): the hash must be stable across API-key +// rotations and must never carry a credential. The struct field order is the +// canonical JSON key order, so the marshalled bytes are deterministic. +type configHashInput struct { + ProviderProtocol string `json:"provider_protocol"` + Model string `json:"model"` + BaseURLHost string `json:"base_url_host"` + Language string `json:"language"` + TimeoutMS int64 `json:"timeout_ms"` +} + +// buildRunMeta assembles the manifest metadata the command layer owns: +// tool version, provider, config/rules hashes, repo identity, and (for the +// range/commit paths) the resolved range SHAs. +func buildRunMeta(ep llm.ResolvedEndpoint, language, ocrVersion, repoDir, customRulePath string, concurrency int, rng resolvedRange) session.RunMeta { + remoteURL, headSHA := repoIdentity(repoDir) + return session.RunMeta{ + OCRVersion: ocrVersion, + Provider: ep.Protocol, + Concurrency: concurrency, + ConfigHash: computeConfigHash(ep, language), + RulesHash: computeRulesHash(customRulePath, repoDir, ocrVersion), + RepoRemoteURL: remoteURL, + RepoHeadSHA: headSHA, + RangeFromSHA: rng.fromSHA, + RangeToSHA: rng.toSHA, + RangeCommitSHA: rng.commitSHA, + } +} + +// computeConfigHash hashes only the allowlisted, non-secret endpoint fields. +func computeConfigHash(ep llm.ResolvedEndpoint, language string) string { + in := configHashInput{ + ProviderProtocol: ep.Protocol, + Model: ep.Model, + BaseURLHost: baseURLHost(ep.URL), + Language: language, + TimeoutMS: ep.Timeout.Milliseconds(), + } + data, err := json.Marshal(in) + if err != nil { + return "" + } + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} + +// baseURLHost returns the host[:port] of a URL with userinfo AND query +// stripped. Anything unparseable yields "" (best-effort, never a credential). +func baseURLHost(raw string) string { + if raw == "" { + return "" + } + u, err := url.Parse(raw) + if err != nil || u.Host == "" { + return "" + } + // u.Host excludes userinfo; RawQuery is never included here. + return u.Host +} + +// computeRulesHash hashes each loaded rule layer as (label + \0 + raw bytes), +// in a stable order. Missing layers contribute nothing. The embedded system +// layer has no file, so it contributes label + tool version (it only changes +// when the binary does). +// +// ponytail: reads the rule files by their known paths rather than plumbing raw +// bytes out of rules.NewResolver — same inputs, no new API surface. If rule +// loading grows more layers, mirror them here. +func computeRulesHash(customRulePath, repoDir, ocrVersion string) string { + h := sha256.New() + add := func(label, path string) { + data, err := os.ReadFile(path) + if err != nil { + return + } + h.Write([]byte(label)) + h.Write([]byte{0}) + h.Write(data) + } + if customRulePath != "" { + add("custom", customRulePath) + } + if repoDir != "" { + add("project", filepath.Join(repoDir, ".opencodereview", "rule.json")) + } + if home, err := os.UserHomeDir(); err == nil { + add("global", filepath.Join(home, ".opencodereview", "rule.json")) + } + h.Write([]byte("system\x00" + ocrVersion)) + return hex.EncodeToString(h.Sum(nil)) +} + +// repoIdentity returns the origin remote URL (with userinfo stripped) and the +// HEAD commit SHA. Both tolerate git failure by returning "". +func repoIdentity(repoDir string) (remoteURL, headSHA string) { + if repoDir == "" { + return "", "" + } + if out, err := runGitCmdStdout(repoDir, "remote", "get-url", "origin"); err == nil { + remoteURL = stripURLUserinfo(strings.TrimSpace(string(out))) + } + if out, err := runGitCmdStdout(repoDir, "rev-parse", "HEAD"); err == nil { + headSHA = strings.TrimSpace(string(out)) + } + return remoteURL, headSHA +} + +// stripURLUserinfo removes a user[:password]@ prefix from a standard URL so a +// token embedded in an https remote never lands in the manifest. scp-style +// git remotes (git@host:path) carry no password and parse as opaque, so they +// pass through unchanged. +func stripURLUserinfo(raw string) string { + u, err := url.Parse(raw) + if err != nil || u.User == nil { + return raw + } + u.User = nil + return u.String() +} + +// resolvedRange holds the resolved commit SHAs for a review range/commit. +type resolvedRange struct { + fromSHA string + toSHA string + commitSHA string +} + +// resolveRange rev-parses the review refs to their commit SHAs. It is +// best-effort: unresolved refs simply stay empty. Callers should have already +// validated the refs (validateReviewRefs) before running. +func resolveRange(repoDir, from, to, commit string) resolvedRange { + var r resolvedRange + r.fromSHA = revParse(repoDir, from) + r.toSHA = revParse(repoDir, to) + r.commitSHA = revParse(repoDir, commit) + return r +} + +func revParse(repoDir, ref string) string { + if ref == "" { + return "" + } + out, err := runGitCmdStdout(repoDir, "rev-parse", "--verify", "--end-of-options", ref+"^{commit}") + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} diff --git a/cmd/opencodereview/runmeta_test.go b/cmd/opencodereview/runmeta_test.go new file mode 100644 index 00000000..c90855eb --- /dev/null +++ b/cmd/opencodereview/runmeta_test.go @@ -0,0 +1,159 @@ +package main + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/open-code-review/open-code-review/internal/llm" +) + +func TestConfigHashRedaction(t *testing.T) { + base := llm.ResolvedEndpoint{ + URL: "https://user:supersecret@api.example.com/v1/chat/completions?key=leak", + Token: "sk-ant-TOPSECRET", + Model: "claude-opus-4-6", + Protocol: llm.ProtocolAnthropic, + AuthHeader: "x-api-key", + Timeout: 30 * time.Second, + } + // Rotating the token / auth header / extra headers must NOT change the hash. + rotated := base + rotated.Token = "sk-ant-DIFFERENT" + rotated.AuthHeader = "authorization" + rotated.ExtraHeaders = map[string]string{"x-custom": "value"} + + h1 := computeConfigHash(base, "English") + h2 := computeConfigHash(rotated, "English") + if h1 != h2 { + t.Errorf("config hash changed on token/header rotation: %q vs %q", h1, h2) + } + + // Changing an allowlisted field MUST change the hash. + if computeConfigHash(base, "Chinese") == h1 { + t.Error("config hash should change when language changes") + } + m2 := base + m2.Model = "claude-sonnet-4-6" + if computeConfigHash(m2, "English") == h1 { + t.Error("config hash should change when model changes") + } + + // The hash input must never contain the token or userinfo/query. + in := configHashInput{ + ProviderProtocol: base.Protocol, + Model: base.Model, + BaseURLHost: baseURLHost(base.URL), + Language: "English", + TimeoutMS: base.Timeout.Milliseconds(), + } + blob := strings.ToLower(in.ProviderProtocol + in.Model + in.BaseURLHost) + for _, bad := range []string{"supersecret", "sk-ant", "topsecret", "key=leak", "user:"} { + if strings.Contains(blob, strings.ToLower(bad)) { + t.Errorf("config hash input leaks %q: %+v", bad, in) + } + } + if in.BaseURLHost != "api.example.com" { + t.Errorf("base_url_host = %q, want api.example.com (userinfo+query stripped)", in.BaseURLHost) + } +} + +func TestStripURLUserinfo(t *testing.T) { + tests := []struct { + in, want string + }{ + {"https://user:token@github.com/acme/repo.git", "https://github.com/acme/repo.git"}, + {"https://github.com/acme/repo.git", "https://github.com/acme/repo.git"}, + {"git@github.com:acme/repo.git", "git@github.com:acme/repo.git"}, // scp-style: no password, unchanged + {"", ""}, + } + for _, tt := range tests { + if got := stripURLUserinfo(tt.in); got != tt.want { + t.Errorf("stripURLUserinfo(%q) = %q, want %q", tt.in, got, tt.want) + } + } +} + +// gitInitRepo creates a throwaway git repo with an origin remote carrying +// embedded credentials and one commit. Returns the repo dir. +func gitInitRepo(t *testing.T) string { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + dir := t.TempDir() + run := func(args ...string) { + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("init", "-q") + run("remote", "add", "origin", "https://alice:secrettoken@example.com/acme/repo.git") + if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hi"), 0o644); err != nil { + t.Fatal(err) + } + run("add", ".") + run("commit", "-q", "-m", "init") + return dir +} + +func TestRepoIdentityStripsUserinfo(t *testing.T) { + dir := gitInitRepo(t) + remoteURL, headSHA := repoIdentity(dir) + if strings.Contains(remoteURL, "secrettoken") || strings.Contains(remoteURL, "alice:") { + t.Errorf("repo remote URL leaks credentials: %q", remoteURL) + } + if remoteURL != "https://example.com/acme/repo.git" { + t.Errorf("remote URL = %q", remoteURL) + } + if len(headSHA) != 40 { + t.Errorf("head SHA = %q, want 40-char sha", headSHA) + } +} + +func TestResolveRangeReturnsSHAs(t *testing.T) { + dir := gitInitRepo(t) + head := revParse(dir, "HEAD") + if len(head) != 40 { + t.Fatalf("HEAD sha = %q", head) + } + // commit mode: commitSHA resolved, from/to empty. + r := resolveRange(dir, "", "", "HEAD") + if r.commitSHA != head { + t.Errorf("commitSHA = %q, want %q", r.commitSHA, head) + } + if r.fromSHA != "" || r.toSHA != "" { + t.Errorf("from/to should be empty for commit mode: %+v", r) + } + // unknown ref resolves to empty, never errors. + if got := revParse(dir, "does-not-exist"); got != "" { + t.Errorf("unknown ref = %q, want empty", got) + } +} + +func TestComputeRulesHashChangesWithLayer(t *testing.T) { + dir := t.TempDir() + h1 := computeRulesHash("", dir, "1.0.0") + // Adding a project rule file changes the hash. + rulesDir := filepath.Join(dir, ".opencodereview") + if err := os.MkdirAll(rulesDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(rulesDir, "rule.json"), []byte(`{"rules":[]}`), 0o644); err != nil { + t.Fatal(err) + } + h2 := computeRulesHash("", dir, "1.0.0") + if h1 == h2 { + t.Error("rules hash should change when a rule layer appears") + } + // Version bump changes the embedded-system contribution. + if computeRulesHash("", dir, "2.0.0") == h2 { + t.Error("rules hash should change when tool version changes") + } +} diff --git a/cmd/opencodereview/scan_cmd.go b/cmd/opencodereview/scan_cmd.go index 5d15f44d..e9b7daaa 100644 --- a/cmd/opencodereview/scan_cmd.go +++ b/cmd/opencodereview/scan_cmd.go @@ -182,6 +182,13 @@ func runScan(args []string) error { } tools := buildToolRegistry(rt.Collector, fileReader) + var lang string + if rt.AppCfg != nil { + lang = rt.AppCfg.Language + } + // Scan has no diff range; resolvedRange stays empty. + runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, opts.rulePath, opts.concurrency, resolvedRange{}) + ag := scan.NewAgent(scan.Args{ RepoDir: cc.RepoDir, Paths: scanPaths, @@ -203,6 +210,7 @@ func runScan(args []string) error { SkipPlan: opts.noPlan, SkipDedup: opts.noDedup, SkipSummary: opts.noSummary, + RunMeta: runMeta, }) q := newQuietHandle(opts.outputFormat, opts.audience) diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index c99c1b2c..f9501849 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -134,6 +134,9 @@ type llmRuntime struct { MainToolDefs []llm.ToolDef Collector *tool.CommentCollector AppCfg *Config + // Endpoint is the resolved LLM endpoint. Retained so the command layer + // can derive non-secret run-manifest metadata (provider, config hash). + Endpoint llm.ResolvedEndpoint } // loadLLMRuntime loads tool defs from toolConfigPath, reads the app config @@ -177,6 +180,7 @@ func loadLLMRuntime(tpl *template.Template, toolConfigPath, modelOverride string MainToolDefs: mainToolDefs, Collector: tool.NewCommentCollector(), AppCfg: appCfg, + Endpoint: ep, }, nil } diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 5e698813..f623914d 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -119,6 +119,12 @@ type Args struct { // Resume is an optional read-only checkpoint index from a previous review session. Resume *session.ResumeState + + // RunMeta carries cmd-layer-populated run-identity metadata (version, + // provider, config/rules hashes, repo identity, resolved range SHAs). It + // is folded into the run manifest at Finalize. Ignored when Session is + // supplied pre-built by the caller. + RunMeta session.RunMeta } // Agent orchestrates the AI-powered code review. LLM tool-use loop / memory @@ -165,6 +171,7 @@ func New(args Args) *Agent { DiffTo: args.To, DiffCommit: args.Commit, ResumedFrom: resumedFromSession(args.Resume), + RunMeta: args.RunMeta, }) } a := &Agent{ diff --git a/internal/scan/agent.go b/internal/scan/agent.go index aa0b8342..f93993b9 100644 --- a/internal/scan/agent.go +++ b/internal/scan/agent.go @@ -67,6 +67,9 @@ type Args struct { // batches are dispatched. 0 = unlimited. Set via --max-tokens-budget // or ScanTemplate.MaxTokensBudget. MaxTokensBudget int64 + // RunMeta carries cmd-layer-populated run-identity metadata folded into + // the run manifest at Finalize. Ignored when Session is supplied. + RunMeta session.RunMeta } // planEnabled / dedupEnabled / summaryEnabled report whether each optional @@ -114,6 +117,7 @@ func NewAgent(args Args) *Agent { if args.Session == nil { args.Session = session.New(args.RepoDir, "", args.Model, session.SessionOptions{ ReviewMode: session.ReviewModeFullScan, + RunMeta: args.RunMeta, }) } a := &Agent{ From 9ba43ffb7bef1f4ad2056e6a4ab2690c97173937 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Wed, 15 Jul 2026 10:26:09 +0200 Subject: [PATCH 4/7] feat(output): expose manifest in --format json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add jsonOutput.Manifest surfaced verbatim from the run's in-memory manifest (same value persisted as the last JSONL record). Manifest() joins the ResultProvider interface; both agent.Agent and scan.Agent implement it. Legacy status values (success/completed_with_errors/completed_with_warnings/ skipped) are UNCHANGED — manifest.state is the new machine contract. Adds a regression-lock test over the status paths plus manifest-presence tests (including the no-files skipped path). --- cmd/opencodereview/emit_run_result_test.go | 102 +++++++++++++++++++++ cmd/opencodereview/output.go | 10 +- cmd/opencodereview/output_helpers_test.go | 8 +- cmd/opencodereview/shared.go | 10 +- internal/agent/agent.go | 9 ++ internal/scan/agent.go | 9 ++ 6 files changed, 140 insertions(+), 8 deletions(-) diff --git a/cmd/opencodereview/emit_run_result_test.go b/cmd/opencodereview/emit_run_result_test.go index f153fe22..e2c92b16 100644 --- a/cmd/opencodereview/emit_run_result_test.go +++ b/cmd/opencodereview/emit_run_result_test.go @@ -12,6 +12,7 @@ import ( "github.com/open-code-review/open-code-review/internal/agent" "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" ) type mockResultProvider struct { @@ -27,6 +28,7 @@ type mockResultProvider struct { toolCalls map[string]int64 resumeInfo *agent.ResumeInfo sessionID string + manifest *session.RunManifest } func (m *mockResultProvider) Diffs() []model.Diff { return m.diffs } @@ -41,6 +43,7 @@ func (m *mockResultProvider) ProjectSummary() string { return m.projectS func (m *mockResultProvider) ToolCalls() map[string]int64 { return m.toolCalls } func (m *mockResultProvider) ResumeInfo() *agent.ResumeInfo { return m.resumeInfo } func (m *mockResultProvider) SessionID() string { return m.sessionID } +func (m *mockResultProvider) Manifest() *session.RunManifest { return m.manifest } func TestEmitRunResult_JSONNoFiles(t *testing.T) { ag := &mockResultProvider{filesReviewed: 0} @@ -280,6 +283,105 @@ func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) { } } +func TestEmitRunResult_JSONIncludesManifest(t *testing.T) { + ag := &mockResultProvider{ + filesReviewed: 3, + inputTokens: 10, + outputTokens: 5, + totalTokens: 15, + manifest: &session.RunManifest{ + SchemaVersion: session.ManifestSchemaVersion, + SessionID: "sess-1", + State: session.StatePartial, + Files: session.ManifestFiles{ + Selected: []string{"a.go", "b.go"}, + Completed: []string{"a.go"}, + Failed: []string{"b.go"}, + }, + }, + } + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if out.Manifest == nil { + t.Fatal("manifest missing from JSON output") + } + if out.Manifest.State != session.StatePartial { + t.Errorf("manifest.state = %q, want partial", out.Manifest.State) + } + if len(out.Manifest.Files.Selected) != 2 { + t.Errorf("manifest selected = %v", out.Manifest.Files.Selected) + } + // Legacy status must remain unchanged (manifest.state is the NEW contract). + if out.Status != "success" { + t.Errorf("legacy status = %q, want success (unchanged by manifest)", out.Status) + } +} + +// TestLegacyStatusValuesUnchanged locks the legacy `status` field to its +// pre-manifest values across the warning/error paths. manifest.state is the +// new machine contract; status must not drift. +func TestLegacyStatusValuesUnchanged(t *testing.T) { + man := &session.RunManifest{State: session.StatePartial} + tests := []struct { + name string + warnings []agent.AgentWarning + wantStatus string + }{ + {"clean success", nil, "success"}, + {"subtask errors", []agent.AgentWarning{{Type: "subtask_error", File: "a.go", Message: "boom"}}, "completed_with_errors"}, + {"non-error warnings", []agent.AgentWarning{{Type: "token_threshold_exceeded", Message: "big"}}, "completed_with_warnings"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ag := &mockResultProvider{filesReviewed: 2, warnings: tt.warnings, manifest: man} + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil { + t.Fatalf("err: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if out.Status != tt.wantStatus { + t.Errorf("status = %q, want %q", out.Status, tt.wantStatus) + } + if out.Manifest == nil || out.Manifest.State != session.StatePartial { + t.Errorf("manifest.state should be present and partial regardless of legacy status") + } + }) + } +} + +func TestEmitRunResult_JSONNoFilesIncludesManifest(t *testing.T) { + ag := &mockResultProvider{ + filesReviewed: 0, + manifest: &session.RunManifest{State: session.StateSkipped}, + } + got := captureStdout(t, func() { + if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil { + t.Fatalf("err: %v", err) + } + }) + var out jsonOutput + if err := json.Unmarshal([]byte(got), &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if out.Status != "skipped" { + t.Errorf("status = %q, want skipped", out.Status) + } + if out.Manifest == nil || out.Manifest.State != session.StateSkipped { + t.Errorf("no-files path should still carry the skipped manifest, got %+v", out.Manifest) + } +} + func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"} got := captureStdout(t, func() { diff --git a/cmd/opencodereview/output.go b/cmd/opencodereview/output.go index 00061647..9b6b7c8b 100644 --- a/cmd/opencodereview/output.go +++ b/cmd/opencodereview/output.go @@ -10,6 +10,7 @@ import ( "github.com/open-code-review/open-code-review/internal/agent" "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" "github.com/open-code-review/open-code-review/internal/suggestdiff" ) @@ -249,6 +250,9 @@ type jsonOutput struct { ProjectSummary string `json:"project_summary,omitempty"` Resume *agent.ResumeInfo `json:"resume,omitempty"` SessionID string `json:"session_id,omitempty"` + // Manifest is the immutable run manifest (coverage + terminal state). It is + // the NEW machine contract; the legacy Status field above is unchanged. + Manifest *session.RunManifest `json:"manifest,omitempty"` } func outputJSON(comments []model.LlmComment) error { @@ -266,7 +270,7 @@ func outputJSON(comments []model.LlmComment) error { func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning, filesReviewed, inputTokens, outputTokens, totalTokens, cacheReadTokens, cacheWriteTokens int64, - duration time.Duration, projectSummary string, toolCalls map[string]int64, traceID string, resumeInfo *agent.ResumeInfo, sessionID string) error { + duration time.Duration, projectSummary string, toolCalls map[string]int64, traceID string, resumeInfo *agent.ResumeInfo, sessionID string, manifest *session.RunManifest) error { out := jsonOutput{ Status: "success", TraceID: traceID, @@ -284,6 +288,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW ProjectSummary: projectSummary, Resume: resumeInfo, SessionID: sessionID, + Manifest: manifest, } var total int64 for _, v := range toolCalls { @@ -317,7 +322,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW return enc.Encode(out) } -func outputJSONNoFiles(traceID string) error { +func outputJSONNoFiles(traceID string, manifest *session.RunManifest) error { out := jsonOutput{ Status: "skipped", TraceID: traceID, @@ -326,6 +331,7 @@ func outputJSONNoFiles(traceID string) error { ToolCalls: &jsonToolCalls{ ByTool: map[string]int64{}, }, + Manifest: manifest, } enc := json.NewEncoder(os.Stdout) enc.SetIndent("", " ") diff --git a/cmd/opencodereview/output_helpers_test.go b/cmd/opencodereview/output_helpers_test.go index 80520b22..a6d34097 100644 --- a/cmd/opencodereview/output_helpers_test.go +++ b/cmd/opencodereview/output_helpers_test.go @@ -168,7 +168,7 @@ func TestOutputJSONWithWarnings_NoCommentsSubtaskError(t *testing.T) { os.Stdout = w warnings := []agent.AgentWarning{{Type: "subtask_error", File: "x.go", Message: "fail"}} - err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace", nil, "") + err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace", nil, "", nil) _ = w.Close() os.Stdout = old @@ -281,7 +281,7 @@ func TestOutputJSONWithWarnings(t *testing.T) { comments := []model.LlmComment{{Path: "b.go", Content: "test"}} warnings := []agent.AgentWarning{{Type: "subtask_error", File: "c.go", Message: "failed"}} - err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}, "trace-xyz-789", nil, "") + err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}, "trace-xyz-789", nil, "", nil) _ = w.Close() os.Stdout = old @@ -319,7 +319,7 @@ func TestOutputJSONWithWarnings_NoCommentsNoErrors(t *testing.T) { os.Stdout = w warnings := []agent.AgentWarning{{Type: "warning", Message: "something"}} - err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "", nil, "") + err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "", nil, "", nil) _ = w.Close() os.Stdout = old @@ -347,7 +347,7 @@ func TestOutputJSONNoFiles(t *testing.T) { r, w, _ := os.Pipe() os.Stdout = w - err := outputJSONNoFiles("test-trace-id-456") + err := outputJSONNoFiles("test-trace-id-456", nil) _ = w.Close() os.Stdout = old diff --git a/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index f9501849..8c0f60a2 100644 --- a/cmd/opencodereview/shared.go +++ b/cmd/opencodereview/shared.go @@ -16,6 +16,7 @@ import ( "github.com/open-code-review/open-code-review/internal/gitcmd" "github.com/open-code-review/open-code-review/internal/llm" "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" "github.com/open-code-review/open-code-review/internal/stdout" "github.com/open-code-review/open-code-review/internal/telemetry" "github.com/open-code-review/open-code-review/internal/tool" @@ -259,6 +260,11 @@ type ResultProvider interface { // in JSON output or failure diagnostics. Returns "" when no session was // created. SessionID() string + // Manifest returns the immutable run manifest (coverage + terminal state) + // produced at Finalize. Nil when no session/run completed. It is the same + // value persisted as the last JSONL record, so JSON output and the stored + // session expose identical coverage. + Manifest() *session.RunManifest } type resumeInfoProvider interface { @@ -291,7 +297,7 @@ func emitRunResult( traceID := telemetry.TraceIDFromContext(ctx) if outputFormat == "json" && len(comments) == 0 && ag.FilesReviewed() == 0 { - return outputJSONNoFiles(traceID) + return outputJSONNoFiles(traceID, ag.Manifest()) } // Agent-text audiences need stdout back before PrintTraceSummary so the @@ -314,7 +320,7 @@ func emitRunResult( return outputJSONWithWarnings(comments, ag.Warnings(), ag.FilesReviewed(), ag.TotalInputTokens(), ag.TotalOutputTokens(), ag.TotalTokensUsed(), ag.TotalCacheReadTokens(), ag.TotalCacheWriteTokens(), duration, - ag.ProjectSummary(), ag.ToolCalls(), traceID, resumeInfo, ag.SessionID()) + ag.ProjectSummary(), ag.ToolCalls(), traceID, resumeInfo, ag.SessionID(), ag.Manifest()) } outputTextWithWarnings(comments, ag.Warnings()) if summary := ag.ProjectSummary(); summary != "" { diff --git a/internal/agent/agent.go b/internal/agent/agent.go index f623914d..d60feb8d 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -252,6 +252,15 @@ func (a *Agent) Session() *session.SessionHistory { return a.session } +// Manifest returns the run manifest produced at Finalize, or nil before the +// run completes. Surfaced verbatim in --format json. +func (a *Agent) Manifest() *session.RunManifest { + if a == nil || a.session == nil { + return nil + } + return a.session.Manifest() +} + // SessionID returns the current review's session id, or "" when no session has been created. func (a *Agent) SessionID() string { if a == nil || a.session == nil { diff --git a/internal/scan/agent.go b/internal/scan/agent.go index f93993b9..0b9c62a4 100644 --- a/internal/scan/agent.go +++ b/internal/scan/agent.go @@ -158,6 +158,15 @@ func toLoopTemplate(s template.ScanTemplate) template.Template { // Session returns the session history associated with this Agent. func (a *Agent) Session() *session.SessionHistory { return a.session } +// Manifest returns the run manifest produced at Finalize, or nil before the +// run completes. Surfaced verbatim in --format json. +func (a *Agent) Manifest() *session.RunManifest { + if a == nil || a.session == nil { + return nil + } + return a.session.Manifest() +} + // SessionID returns the current scan's session id, or "" when no session has been created. func (a *Agent) SessionID() string { if a == nil || a.session == nil { From 7c9c476d81c7061708fb54c7d5d5e1a3f10e8f07 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Wed, 15 Jul 2026 10:29:02 +0200 Subject: [PATCH 5/7] feat(scan): manifest parity for full scans Record coverage on the scan path so full scans emit the same manifest as diff reviews: - SetSelected + artifact checksum (order-independent sha256 over sorted path-based fingerprints) at dispatch start. - executeSubtask records done / failed(ClassifyFailure) / skipped_limit per item so the coverage invariant holds. - Run marks the session cancelled on context cancellation. Scan has no diff/resume, so the fingerprint is path-based (ponytail note: content-hash if scan grows resume). Adds a parity table test over success/mixed/all-failed/cancel/skip. --- internal/scan/agent.go | 48 +++++++++- internal/scan/manifest_test.go | 167 +++++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 internal/scan/manifest_test.go diff --git a/internal/scan/agent.go b/internal/scan/agent.go index 0b9c62a4..1b01c103 100644 --- a/internal/scan/agent.go +++ b/internal/scan/agent.go @@ -2,8 +2,11 @@ package scan import ( "context" + "crypto/sha256" "encoding/json" + "errors" "fmt" + "sort" "strings" "sync" "sync/atomic" @@ -271,6 +274,9 @@ func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { // Project-level summary runs after all batches; never blocks return. a.maybeRunProjectSummary(ctx, comments) + if ctx.Err() != nil || errors.Is(err, context.Canceled) { + a.session.MarkCancelled() + } a.session.Finalize() return comments, err } @@ -402,6 +408,10 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error return []model.LlmComment{}, nil } + // Manifest coverage: the reviewable item set is the denominator, plus an + // artifact checksum over the sorted per-file fingerprints. + a.recordSelectionAndArtifact() + atomic.StoreInt64(&a.subtaskFailed, 0) strategy := a.resolveBatchStrategy() @@ -448,6 +458,33 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error return a.args.CommentCollector.Comments(), nil } +// scanFingerprint derives a stable per-item fingerprint for the manifest. +// +// ponytail: content-hash if scan grows resume. Scan has no diff and no resume +// today, so a path-based fingerprint is enough to identify the item and to +// compute the artifact checksum; upgrade to hashing it.Content when resume +// lands on the scan path. +func scanFingerprint(path string) string { + sum := sha256.Sum256([]byte(session.ReviewModeFullScan + "\x00" + path)) + return fmt.Sprintf("%x", sum) +} + +// recordSelectionAndArtifact records the reviewable item set (coverage +// denominator) and an order-independent artifact checksum over the sorted +// per-file fingerprints. +func (a *Agent) recordSelectionAndArtifact() { + selected := make([]string, 0, len(a.items)) + fps := make([]string, 0, len(a.items)) + for i := range a.items { + selected = append(selected, a.items[i].Path) + fps = append(fps, scanFingerprint(a.items[i].Path)) + } + a.session.SetSelected(selected) + sort.Strings(fps) + sum := sha256.Sum256([]byte(strings.Join(fps, "\x00"))) + a.session.SetArtifactChecksum(fmt.Sprintf("%x", sum)) +} + // resolveBatchStrategy reads the strategy from the scan template, defaulting // to BatchNone for unrecognized / empty values. func (a *Agent) resolveBatchStrategy() BatchStrategy { @@ -560,6 +597,8 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error { messages := a.renderMessages(it, rule, planGuidance) + fingerprint := scanFingerprint(it.Path) + tokenCount := llmloop.CountMessagesTokens(messages) maxAllowed := a.args.Template.MaxTokens tokenLimit := maxAllowed * 4 / 5 @@ -567,6 +606,7 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error { msg := fmt.Sprintf("prompt tokens (%d) exceed %d%% of max_tokens(%d)", tokenCount, 80, maxAllowed) fmt.Fprintf(stdout.Writer(), "[ocr] WARNING: %s for %s\n", msg, it.Path) a.recordWarning("token_threshold_exceeded", it.Path, msg) + a.session.RecordReviewItemFailed(it.Path, it.Path, it.Path, fingerprint, session.FailureSkippedLimit, msg) telemetry.Event(ctx, "token.threshold.exceeded", telemetry.AnyToAttr("file.path", it.Path), telemetry.AnyToAttr("tokens", tokenCount), @@ -575,7 +615,13 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error { } _, err := a.runner.RunPerFile(ctx, messages, it.Path) - return err + if err != nil { + a.session.RecordReviewItemFailed(it.Path, it.Path, it.Path, fingerprint, session.ClassifyFailure(err), err.Error()) + return err + } + comments := a.args.CommentCollector.CommentsForPath(it.Path) + a.session.RecordReviewItemDone(it.Path, it.Path, it.Path, fingerprint, comments) + return nil } // maybeRunPlan invokes PLAN_TASK on the file and returns a human-readable diff --git a/internal/scan/manifest_test.go b/internal/scan/manifest_test.go new file mode 100644 index 00000000..1a634f71 --- /dev/null +++ b/internal/scan/manifest_test.go @@ -0,0 +1,167 @@ +package scan + +import ( + "context" + "errors" + "fmt" + "strings" + "testing" + + "github.com/open-code-review/open-code-review/internal/llm" + "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" + "github.com/open-code-review/open-code-review/internal/tool" +) + +// scanRouteClient dispatches on markers in the rendered prompt (which embeds +// {{file_content}}) so one client can drive success/fail/cancel per file. +type scanRouteClient struct{} + +func (scanRouteClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequest) (*llm.ChatResponse, error) { + var sb strings.Builder + for _, m := range req.Messages { + sb.WriteString(m.ExtractText()) + } + text := sb.String() + switch { + case strings.Contains(text, "DO_CANCEL"): + return nil, fmt.Errorf("scan call: %w", context.Canceled) + case strings.Contains(text, "DO_FAIL"): + return nil, errors.New("provider exploded") + default: + content := "" + return &llm.ChatResponse{ + Choices: []llm.Choice{{Message: llm.ResponseMessage{ + Content: &content, + ToolCalls: []llm.ToolCall{{ + ID: "c1", Type: "function", + Function: llm.FunctionCall{Name: "task_done", Arguments: "{}"}, + }}, + }}}, + Usage: &llm.UsageInfo{PromptTokens: 5, CompletionTokens: 2}, + }, nil + } +} + +func scanClassCounts(m *session.RunManifest) map[string]int { + out := map[string]int{} + for _, f := range m.Failures { + out[f.Class]++ + } + return out +} + +func TestScanRunManifest_Parity(t *testing.T) { + tests := []struct { + name string + maxTokens int + items []model.ScanItem + markCancelled bool + wantState string + wantSelected int + wantClasses map[string]int + }{ + { + name: "success", + maxTokens: 100000, + items: []model.ScanItem{{Path: "ok.go", Content: "ok", LineCount: 1}}, + wantState: session.StateComplete, + wantSelected: 1, + wantClasses: map[string]int{}, + }, + { + name: "mixed success and failure", + maxTokens: 100000, + items: []model.ScanItem{ + {Path: "ok.go", Content: "ok", LineCount: 1}, + {Path: "bad.go", Content: "DO_FAIL", LineCount: 1}, + }, + wantState: session.StatePartial, + wantSelected: 2, + wantClasses: map[string]int{session.FailureProviderError: 1}, + }, + { + name: "all failed", + maxTokens: 100000, + items: []model.ScanItem{{Path: "bad.go", Content: "DO_FAIL", LineCount: 1}}, + wantState: session.StateFailed, + wantSelected: 1, + wantClasses: map[string]int{session.FailureProviderError: 1}, + }, + { + name: "cancel with a success is partial", + maxTokens: 100000, + items: []model.ScanItem{ + {Path: "ok.go", Content: "ok", LineCount: 1}, + {Path: "gone.go", Content: "DO_CANCEL", LineCount: 1}, + }, + markCancelled: true, + wantState: session.StatePartial, + wantSelected: 2, + wantClasses: map[string]int{session.FailureCancelled: 1}, + }, + { + name: "token limit skip", + maxTokens: 10, + items: []model.ScanItem{{Path: "huge.go", Content: "some content here", LineCount: 1}}, + wantState: session.StateFailed, + wantSelected: 1, + wantClasses: map[string]int{session.FailureSkippedLimit: 1}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tpl := makeTemplateWithFullScan() + tpl.MaxTokens = tt.maxTokens + sess := session.New(t.TempDir(), "", "test", session.SessionOptions{ReviewMode: session.ReviewModeFullScan}) + a := NewAgent(Args{ + Template: tpl, + LLMClient: scanRouteClient{}, + Model: "test", + CommentCollector: tool.NewCommentCollector(), + Tools: tool.NewRegistry(), + MaxConcurrency: 2, + SkipPlan: true, + SkipDedup: true, + SkipSummary: true, + Session: sess, + }) + a.items = tt.items + a.currentDate = "2026-07-15" + a.args.Tools.Freeze() + + _, _ = a.dispatchSubtasks(context.Background()) + if tt.markCancelled { + sess.MarkCancelled() + } + sess.Finalize() + + m := sess.Manifest() + if m == nil { + t.Fatal("nil manifest") + } + if m.State != tt.wantState { + t.Errorf("state = %q, want %q (files=%+v)", m.State, tt.wantState, m.Files) + } + if len(m.Files.Selected) != tt.wantSelected { + t.Errorf("selected = %v, want %d", m.Files.Selected, tt.wantSelected) + } + if m.ArtifactSHA256 == "" { + t.Error("artifact checksum should be recorded") + } + if m.ReviewMode != session.ReviewModeFullScan { + t.Errorf("review_mode = %q, want full_scan", m.ReviewMode) + } + got := scanClassCounts(m) + if len(got) != len(tt.wantClasses) { + t.Errorf("failure classes = %v, want %v", got, tt.wantClasses) + } + for k, v := range tt.wantClasses { + if got[k] != v { + t.Errorf("class %q = %d, want %d (all=%v)", k, got[k], v, got) + } + } + }) + } +} From ff745d4d1310a6677e50ec97675e5a717a400477 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Wed, 15 Jul 2026 10:31:50 +0200 Subject: [PATCH 6/7] feat(review): --waive for resumed runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add --waive path/a.go,path/b.go (requires --resume): waived diffs are not dispatched, are recorded as review_item_waived, and count toward coverage so a waive can flip partial -> complete. A later resume treats a waived item as covered (indexed like a completed item, empty comments). No fingerprint-addressed or config-file waives — paths only (ponytail note). Tests: waive flips partial->complete, without waive stays partial, --waive without --resume errors. --- cmd/opencodereview/flags.go | 12 +++++++- cmd/opencodereview/flags_test.go | 13 ++++++++ cmd/opencodereview/review_cmd.go | 1 + internal/agent/agent.go | 16 ++++++++++ internal/agent/manifest_test.go | 53 ++++++++++++++++++++++++++++++++ internal/session/resume.go | 4 ++- 6 files changed, 97 insertions(+), 2 deletions(-) diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 6d6e64c2..09cd1c3c 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -102,6 +102,7 @@ type reviewOptions struct { to string commit string resume string + waive string // --waive: comma-separated paths to skip (requires --resume) excludes string // --exclude: comma-separated gitignore-style patterns outputFormat string audience string // --audience: "human" (default) or "agent" @@ -128,6 +129,7 @@ func parseReviewFlags(args []string) (reviewOptions, error) { a.StringVar(&opts.to, "to", "", "target ref to end diff at (e.g., 'feature-branch')") a.StringVarP(&opts.commit, "commit", "c", "", "single commit hash or tag to review (vs its parent)") a.StringVar(&opts.resume, "resume", "", "resume from a previous review session id") + a.StringVar(&opts.waive, "waive", "", "comma-separated paths to waive (skip but count as covered); requires --resume") a.StringVar(&opts.excludes, "exclude", "", "comma-separated gitignore-style patterns to exclude; merged with rule.json excludes") a.StringVarP(&opts.outputFormat, "format", "f", "text", "output format: text or json") a.IntVar(&opts.concurrency, "concurrency", 8, "max concurrent file reviews") @@ -169,6 +171,13 @@ func parseReviewFlags(args []string) (reviewOptions, error) { if opts.preview && opts.resume != "" { return opts, fmt.Errorf("--preview and --resume cannot be used together") } + // Waiving only makes sense on a resumed run: it deliberately leaves diffs + // unreviewed while still satisfying the coverage contract. There is no + // fingerprint-addressed or config-file waive — paths only, and only with + // --resume. (ponytail: path-scoped waives cover the operator use case.) + if opts.waive != "" && opts.resume == "" { + return opts, fmt.Errorf("--waive requires --resume") + } switch opts.audience { case "human", "agent": @@ -246,7 +255,8 @@ Flags: --rule string path to JSON file with system review rules --timeout int concurrent task timeout in minutes (default 10) --to string target ref to end diff at (e.g., 'feature-branch') - --tools string path to JSON tools config file (default: embedded)`) + --tools string path to JSON tools config file (default: embedded) + --waive string comma-separated paths to waive (skip but count as covered); requires --resume`) } // --- config subcommand --- diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go index 8e259964..22401794 100644 --- a/cmd/opencodereview/flags_test.go +++ b/cmd/opencodereview/flags_test.go @@ -53,6 +53,19 @@ func TestParseReviewFlags_PreviewWithResume(t *testing.T) { } } +func TestParseReviewFlags_WaiveRequiresResume(t *testing.T) { + if _, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--waive", "a.go"}); err == nil { + t.Fatal("expected error for --waive without --resume") + } + opts, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--resume", "s1", "--waive", "a.go,b.go"}) + if err != nil { + t.Fatalf("--waive with --resume should be valid: %v", err) + } + if opts.waive != "a.go,b.go" { + t.Errorf("waive = %q", opts.waive) + } +} + func TestParseReviewFlags_InvalidAudience(t *testing.T) { _, err := parseReviewFlags([]string{"--audience", "robot"}) if err == nil { diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index f846e2f1..fafebfc4 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -127,6 +127,7 @@ func runReview(args []string) error { GitRunner: cc.GitRunner, Resume: resumeState, RunMeta: runMeta, + WaivePaths: splitPaths(opts.waive), }) // Silence progress output during execution; restored before the trace diff --git a/internal/agent/agent.go b/internal/agent/agent.go index d60feb8d..2d182490 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -125,6 +125,11 @@ type Args struct { // is folded into the run manifest at Finalize. Ignored when Session is // supplied pre-built by the caller. RunMeta session.RunMeta + + // WaivePaths lists repo-relative paths to waive on a resumed run: they are + // not dispatched, are recorded as review_item_waived, and count toward + // coverage. Only honored alongside Resume (the CLI enforces --resume). + WaivePaths []string } // Agent orchestrates the AI-powered code review. LLM tool-use loop / memory @@ -481,6 +486,11 @@ func (a *Agent) applyResume(diffs []model.Diff) []model.Diff { return diffs } + waived := make(map[string]struct{}, len(a.args.WaivePaths)) + for _, p := range a.args.WaivePaths { + waived[p] = struct{}{} + } + mode := a.reviewMode() toDispatch := make([]model.Diff, 0, len(diffs)) var reused int64 @@ -490,6 +500,12 @@ func (a *Agent) applyResume(diffs []model.Diff) []model.Diff { continue } fingerprint := reviewItemFingerprint(mode, d) + // An explicit waive wins over both reuse and re-review: the operator + // chose to skip this diff while still satisfying coverage. + if _, ok := waived[effectivePath(d)]; ok { + a.session.RecordReviewItemWaived(effectivePath(d), d.OldPath, d.NewPath, fingerprint) + continue + } item, ok := resume.Item(fingerprint) if !ok { toDispatch = append(toDispatch, d) diff --git a/internal/agent/manifest_test.go b/internal/agent/manifest_test.go index 2c1cdb88..fea875e5 100644 --- a/internal/agent/manifest_test.go +++ b/internal/agent/manifest_test.go @@ -184,6 +184,59 @@ func TestRunManifest_Scenarios(t *testing.T) { } } +// TestWaiveCoverage verifies a waived diff is not dispatched, is recorded as +// covered, and flips an otherwise-partial run to complete. +func TestWaiveCoverage(t *testing.T) { + build := func(waive []string) *session.RunManifest { + sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange}) + a := New(Args{ + LLMClient: routeClient{}, + Model: "test", + Session: sess, + Tools: tool.NewRegistry(), + Template: template.Template{ + MaxTokens: 100000, + MaxToolRequestTimes: 5, + MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}}, + }, + MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done"}}}, + // Resume must be non-nil for applyResume (and thus waive) to run. + Resume: &session.ResumeState{SessionID: "prev", Items: map[string]session.ResumeItem{}}, + WaivePaths: waive, + }) + a.currentDate = "d" + a.diffs = []model.Diff{ + {NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1}, + {NewPath: "flaky.go", OldPath: "flaky.go", Diff: "+DO_FAIL", Insertions: 1}, + } + _, _ = a.dispatchSubtasks(context.Background()) + sess.Finalize() + return sess.Manifest() + } + + // No waive: the failing diff leaves the run partial. + noWaive := build(nil) + if noWaive.State != session.StatePartial { + t.Errorf("without waive: state = %q, want partial (files=%+v)", noWaive.State, noWaive.Files) + } + + // Waiving the failing diff flips the run to complete and records it as + // waived rather than dispatched/failed. + waived := build([]string{"flaky.go"}) + if waived.State != session.StateComplete { + t.Errorf("with waive: state = %q, want complete (files=%+v)", waived.State, waived.Files) + } + if len(waived.Files.Waived) != 1 || waived.Files.Waived[0] != "flaky.go" { + t.Errorf("waived set = %v, want [flaky.go]", waived.Files.Waived) + } + if len(waived.Files.Failed) != 0 { + t.Errorf("waived diff must not be recorded as failed: %v", waived.Files.Failed) + } + if len(waived.Files.Completed) != 1 || waived.Files.Completed[0] != "ok.go" { + t.Errorf("completed set = %v, want [ok.go]", waived.Files.Completed) + } +} + // TestArtifactChecksumStableAndOrderIndependent verifies the artifact hash is // deterministic regardless of diff ordering (fingerprints are sorted). func TestArtifactChecksumStableAndOrderIndependent(t *testing.T) { diff --git a/internal/session/resume.go b/internal/session/resume.go index 756037c4..5638bc37 100644 --- a/internal/session/resume.go +++ b/internal/session/resume.go @@ -111,7 +111,9 @@ func (s *ResumeState) applyResumeLine(line []byte) error { switch rec.Type { case "session_start": s.applySessionStart(rec) - case "review_item_done", "review_item_reused": + case "review_item_done", "review_item_reused", "review_item_waived": + // A waived item counts as covered for later resumes: index it like a + // completed item (empty comments) so a subsequent resume skips it. if rec.Fingerprint == "" { return nil } From dbe57fa927c298b228962f4efd7aa0c9541a244d Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Wed, 15 Jul 2026 12:33:04 +0200 Subject: [PATCH 7/7] chore(tests): acceptance matrix + provider-transition + secret guard; docs Tests + docs: - acceptance_test.go: end-to-end matrix over the terminal-state scenarios (complete/partial/failed/skipped/cancelled/waive, plus a cancelled mid-run row where selected is a strict superset of the outcome sets); provider-transition resume case starts from a PARTIAL run and asserts the resume flips it to complete with parent_session_id intact and an unchanged artifact checksum (input identity retained); TestManifestNeverLeaksSecrets is the canonical secret guard. - manifest_test.go: TestManifestSchemaLock pins the literal schema version (1) and the top-level JSON key set. The standalone Finalize test was folded into the acceptance matrix (failure-class + artifact columns). - emit_run_result_test.go: no-files manifest case folded into the legacy-status regression table. - agent/manifest_test.go: newManifestAgent helper replaces three copies of the Args literal. - docs: run-manifest schema, state->exit-code mapping, --waive flag; files invariant stated with the superset wording; commit-mode range fields documented. Hardening: - viewer/store.go: peekSession read only the last JSONL line for session_end; run_manifest is now the last line, so the session-list fast path lost duration/files/failures. Checks the last two lines; regression test fails on the old code. - manifest.go: cancelled run whose only covered items are waives is partial, not failed; ManifestFiles doc corrected (selected is a superset); Finalize made idempotent (written-once guard). - session logging to stderr: the [ocr session] messages went to stdout, which carries the --format json payload; all four sites now use os.Stderr. - redactRemoteURL (nee stripURLUserinfo) also strips query/fragment, so ?access_token=... can never reach the manifest; scp-style remotes still pass through (they fail url.Parse and carry no password). Verified E2E. - scanFingerprint hashes mode\0path\0content (ScanItem.Content is already in memory), matching the diff path's content-sensitive fingerprint. - scan dispatch goroutines now recover from panics, record the item as failed with class "panic", and let Finalize still write the manifest, mirroring the diff-review dispatch. Panic row added to the scan parity table. - session.ArtifactChecksum: the sort/join/sha256 tail duplicated in the agent and scan recordSelectionAndArtifact now lives in one place so the fingerprint-join convention cannot diverge. - computeRulesHash hashes the resolver's resolved rule layers (new rules.UserLayers accessor) instead of re-reading the top-level rule.json bytes: rule entries that reference files (e.g. team.md) are expanded to content by rules.NewResolver, so editing a referenced file now changes rules_hash even though rule.json's bytes do not. Test locks referenced-file edit -> hash change. - scan executeSubtask drains the async CommentWorkerPool before reading CommentsForPath and persisting review_item_done; without the per-file Await the checkpoint could persist an incomplete comment set while async code_comment workers were still running (the diff path already had this Await). - filterLargeDiffs/filterLargeScans record each dropped oversized file as failed with class skipped_limit (plus a warning), so the manifest's selected denominator includes pre-filtered files: a partially filtered run is partial, an all-filtered run is failed with per-file failures rather than an empty "skipped". Two agent table rows + a scan filter test lock this. - persist.go/history.go: corrected stale close-ownership comments (WriteSessionEnd no longer closes the file; flushAndClose does). Full go test ./... green (2037); -race green on session/agent/scan/ llmloop/cmd; make check clean. E2E: failing review persists ... -> session_end -> run_manifest with state=failed and class=provider_error; no-diff review emits valid skipped JSON with the manifest; a remote with userinfo+query credentials is fully redacted. --- cmd/opencodereview/emit_run_result_test.go | 54 ++--- cmd/opencodereview/review_cmd.go | 2 +- cmd/opencodereview/runmeta.go | 69 +++--- cmd/opencodereview/runmeta_test.go | 51 +++-- cmd/opencodereview/scan_cmd.go | 2 +- internal/agent/agent.go | 20 +- internal/agent/manifest_test.go | 120 ++++++----- internal/config/rules/system_rules.go | 7 + internal/scan/agent.go | 54 +++-- internal/scan/manifest_test.go | 63 ++++++ internal/session/acceptance_test.go | 235 +++++++++++++++++++++ internal/session/history.go | 17 +- internal/session/manifest.go | 26 ++- internal/session/manifest_test.go | 88 ++++---- internal/session/persist.go | 10 +- internal/viewer/store.go | 45 ++-- internal/viewer/store_test.go | 28 +++ pages/src/content/docs/en/cli-reference.md | 68 ++++++ 18 files changed, 729 insertions(+), 230 deletions(-) create mode 100644 internal/session/acceptance_test.go diff --git a/cmd/opencodereview/emit_run_result_test.go b/cmd/opencodereview/emit_run_result_test.go index e2c92b16..6c0913dd 100644 --- a/cmd/opencodereview/emit_run_result_test.go +++ b/cmd/opencodereview/emit_run_result_test.go @@ -325,22 +325,30 @@ func TestEmitRunResult_JSONIncludesManifest(t *testing.T) { } // TestLegacyStatusValuesUnchanged locks the legacy `status` field to its -// pre-manifest values across the warning/error paths. manifest.state is the -// new machine contract; status must not drift. +// pre-manifest values across the success/warning/error/no-files paths, and +// asserts every path still carries the manifest. manifest.state is the new +// machine contract; status must not drift. func TestLegacyStatusValuesUnchanged(t *testing.T) { - man := &session.RunManifest{State: session.StatePartial} tests := []struct { - name string - warnings []agent.AgentWarning - wantStatus string + name string + filesReviewed int64 + warnings []agent.AgentWarning + manifestState string + wantStatus string + wantManifestState string }{ - {"clean success", nil, "success"}, - {"subtask errors", []agent.AgentWarning{{Type: "subtask_error", File: "a.go", Message: "boom"}}, "completed_with_errors"}, - {"non-error warnings", []agent.AgentWarning{{Type: "token_threshold_exceeded", Message: "big"}}, "completed_with_warnings"}, + {"clean success", 2, nil, session.StatePartial, "success", session.StatePartial}, + {"subtask errors", 2, []agent.AgentWarning{{Type: "subtask_error", File: "a.go", Message: "boom"}}, session.StatePartial, "completed_with_errors", session.StatePartial}, + {"non-error warnings", 2, []agent.AgentWarning{{Type: "token_threshold_exceeded", Message: "big"}}, session.StatePartial, "completed_with_warnings", session.StatePartial}, + {"no files skipped", 0, nil, session.StateSkipped, "skipped", session.StateSkipped}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ag := &mockResultProvider{filesReviewed: 2, warnings: tt.warnings, manifest: man} + ag := &mockResultProvider{ + filesReviewed: tt.filesReviewed, + warnings: tt.warnings, + manifest: &session.RunManifest{State: tt.manifestState}, + } got := captureStdout(t, func() { if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil { t.Fatalf("err: %v", err) @@ -353,35 +361,13 @@ func TestLegacyStatusValuesUnchanged(t *testing.T) { if out.Status != tt.wantStatus { t.Errorf("status = %q, want %q", out.Status, tt.wantStatus) } - if out.Manifest == nil || out.Manifest.State != session.StatePartial { - t.Errorf("manifest.state should be present and partial regardless of legacy status") + if out.Manifest == nil || out.Manifest.State != tt.wantManifestState { + t.Errorf("manifest.state should be present (%q) regardless of legacy status, got %+v", tt.wantManifestState, out.Manifest) } }) } } -func TestEmitRunResult_JSONNoFilesIncludesManifest(t *testing.T) { - ag := &mockResultProvider{ - filesReviewed: 0, - manifest: &session.RunManifest{State: session.StateSkipped}, - } - got := captureStdout(t, func() { - if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil { - t.Fatalf("err: %v", err) - } - }) - var out jsonOutput - if err := json.Unmarshal([]byte(got), &out); err != nil { - t.Fatalf("unmarshal: %v", err) - } - if out.Status != "skipped" { - t.Errorf("status = %q, want skipped", out.Status) - } - if out.Manifest == nil || out.Manifest.State != session.StateSkipped { - t.Errorf("no-files path should still carry the skipped manifest, got %+v", out.Manifest) - } -} - func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"} got := captureStdout(t, func() { diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index fafebfc4..8ae03733 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -102,7 +102,7 @@ func runReview(args []string) error { if rt.AppCfg != nil { lang = rt.AppCfg.Language } - runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, opts.rulePath, opts.concurrency, + runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, cc.Resolver, opts.concurrency, resolveRange(cc.RepoDir, opts.from, opts.to, opts.commit)) ag := agent.New(agent.Args{ diff --git a/cmd/opencodereview/runmeta.go b/cmd/opencodereview/runmeta.go index ebac2f9b..6bf00231 100644 --- a/cmd/opencodereview/runmeta.go +++ b/cmd/opencodereview/runmeta.go @@ -5,10 +5,9 @@ import ( "encoding/hex" "encoding/json" "net/url" - "os" - "path/filepath" "strings" + "github.com/open-code-review/open-code-review/internal/config/rules" "github.com/open-code-review/open-code-review/internal/llm" "github.com/open-code-review/open-code-review/internal/session" ) @@ -29,14 +28,14 @@ type configHashInput struct { // buildRunMeta assembles the manifest metadata the command layer owns: // tool version, provider, config/rules hashes, repo identity, and (for the // range/commit paths) the resolved range SHAs. -func buildRunMeta(ep llm.ResolvedEndpoint, language, ocrVersion, repoDir, customRulePath string, concurrency int, rng resolvedRange) session.RunMeta { +func buildRunMeta(ep llm.ResolvedEndpoint, language, ocrVersion, repoDir string, resolver rules.Resolver, concurrency int, rng resolvedRange) session.RunMeta { remoteURL, headSHA := repoIdentity(repoDir) return session.RunMeta{ OCRVersion: ocrVersion, Provider: ep.Protocol, Concurrency: concurrency, ConfigHash: computeConfigHash(ep, language), - RulesHash: computeRulesHash(customRulePath, repoDir, ocrVersion), + RulesHash: computeRulesHash(resolver, ocrVersion), RepoRemoteURL: remoteURL, RepoHeadSHA: headSHA, RangeFromSHA: rng.fromSHA, @@ -76,18 +75,21 @@ func baseURLHost(raw string) string { return u.Host } -// computeRulesHash hashes each loaded rule layer as (label + \0 + raw bytes), -// in a stable order. Missing layers contribute nothing. The embedded system -// layer has no file, so it contributes label + tool version (it only changes -// when the binary does). -// -// ponytail: reads the rule files by their known paths rather than plumbing raw -// bytes out of rules.NewResolver — same inputs, no new API surface. If rule -// loading grows more layers, mirror them here. -func computeRulesHash(customRulePath, repoDir, ocrVersion string) string { +// computeRulesHash hashes each loaded rule layer as (label + \0 + canonical +// JSON of the resolved layer), in a stable order. "Resolved" means rule +// entries that reference files (e.g. a rule.json entry pointing at team.md) +// have already been expanded to that file's content by rules.NewResolver, so +// the hash tracks the effective runtime rules — editing a referenced file +// changes the hash even though rule.json's bytes do not. Missing layers +// contribute nothing. The embedded system layer has no file, so it +// contributes label + tool version (it only changes when the binary does). +func computeRulesHash(resolver rules.Resolver, ocrVersion string) string { h := sha256.New() - add := func(label, path string) { - data, err := os.ReadFile(path) + add := func(label string, layer *rules.ProjectRule) { + if layer == nil { + return + } + data, err := json.Marshal(layer) if err != nil { return } @@ -95,27 +97,26 @@ func computeRulesHash(customRulePath, repoDir, ocrVersion string) string { h.Write([]byte{0}) h.Write(data) } - if customRulePath != "" { - add("custom", customRulePath) - } - if repoDir != "" { - add("project", filepath.Join(repoDir, ".opencodereview", "rule.json")) - } - if home, err := os.UserHomeDir(); err == nil { - add("global", filepath.Join(home, ".opencodereview", "rule.json")) + if ul, ok := resolver.(interface { + UserLayers() (custom, project, global *rules.ProjectRule) + }); ok { + custom, project, global := ul.UserLayers() + add("custom", custom) + add("project", project) + add("global", global) } h.Write([]byte("system\x00" + ocrVersion)) return hex.EncodeToString(h.Sum(nil)) } -// repoIdentity returns the origin remote URL (with userinfo stripped) and the -// HEAD commit SHA. Both tolerate git failure by returning "". +// repoIdentity returns the origin remote URL (with userinfo/query redacted) +// and the HEAD commit SHA. Both tolerate git failure by returning "". func repoIdentity(repoDir string) (remoteURL, headSHA string) { if repoDir == "" { return "", "" } if out, err := runGitCmdStdout(repoDir, "remote", "get-url", "origin"); err == nil { - remoteURL = stripURLUserinfo(strings.TrimSpace(string(out))) + remoteURL = redactRemoteURL(strings.TrimSpace(string(out))) } if out, err := runGitCmdStdout(repoDir, "rev-parse", "HEAD"); err == nil { headSHA = strings.TrimSpace(string(out)) @@ -123,16 +124,20 @@ func repoIdentity(repoDir string) (remoteURL, headSHA string) { return remoteURL, headSHA } -// stripURLUserinfo removes a user[:password]@ prefix from a standard URL so a -// token embedded in an https remote never lands in the manifest. scp-style -// git remotes (git@host:path) carry no password and parse as opaque, so they -// pass through unchanged. -func stripURLUserinfo(raw string) string { +// redactRemoteURL removes the user[:password]@ prefix and any query/fragment +// from a standard URL so a token embedded in an https remote (userinfo or +// ?access_token=…) never lands in the manifest, matching baseURLHost's +// redaction of the config-hash input. scp-style git remotes (git@host:path) +// fail url.Parse, carry no password, and pass through unchanged. +func redactRemoteURL(raw string) string { u, err := url.Parse(raw) - if err != nil || u.User == nil { + if err != nil { return raw } u.User = nil + u.RawQuery = "" + u.Fragment = "" + u.RawFragment = "" return u.String() } diff --git a/cmd/opencodereview/runmeta_test.go b/cmd/opencodereview/runmeta_test.go index c90855eb..cf793aa2 100644 --- a/cmd/opencodereview/runmeta_test.go +++ b/cmd/opencodereview/runmeta_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/open-code-review/open-code-review/internal/config/rules" "github.com/open-code-review/open-code-review/internal/llm" ) @@ -61,18 +62,20 @@ func TestConfigHashRedaction(t *testing.T) { } } -func TestStripURLUserinfo(t *testing.T) { +func TestRedactRemoteURL(t *testing.T) { tests := []struct { in, want string }{ {"https://user:token@github.com/acme/repo.git", "https://github.com/acme/repo.git"}, {"https://github.com/acme/repo.git", "https://github.com/acme/repo.git"}, + {"https://github.com/acme/repo.git?access_token=SECRET", "https://github.com/acme/repo.git"}, + {"https://user:token@github.com/acme/repo.git?access_token=SECRET#frag", "https://github.com/acme/repo.git"}, {"git@github.com:acme/repo.git", "git@github.com:acme/repo.git"}, // scp-style: no password, unchanged {"", ""}, } for _, tt := range tests { - if got := stripURLUserinfo(tt.in); got != tt.want { - t.Errorf("stripURLUserinfo(%q) = %q, want %q", tt.in, got, tt.want) + if got := redactRemoteURL(tt.in); got != tt.want { + t.Errorf("redactRemoteURL(%q) = %q, want %q", tt.in, got, tt.want) } } } @@ -94,7 +97,7 @@ func gitInitRepo(t *testing.T) string { } } run("init", "-q") - run("remote", "add", "origin", "https://alice:secrettoken@example.com/acme/repo.git") + run("remote", "add", "origin", "https://alice:secrettoken@example.com/acme/repo.git?access_token=querysecret") if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hi"), 0o644); err != nil { t.Fatal(err) } @@ -106,7 +109,7 @@ func gitInitRepo(t *testing.T) string { func TestRepoIdentityStripsUserinfo(t *testing.T) { dir := gitInitRepo(t) remoteURL, headSHA := repoIdentity(dir) - if strings.Contains(remoteURL, "secrettoken") || strings.Contains(remoteURL, "alice:") { + if strings.Contains(remoteURL, "secrettoken") || strings.Contains(remoteURL, "alice:") || strings.Contains(remoteURL, "querysecret") { t.Errorf("repo remote URL leaks credentials: %q", remoteURL) } if remoteURL != "https://example.com/acme/repo.git" { @@ -138,22 +141,46 @@ func TestResolveRangeReturnsSHAs(t *testing.T) { } func TestComputeRulesHashChangesWithLayer(t *testing.T) { + t.Setenv("HOME", t.TempDir()) // isolate the global ~/.opencodereview layer dir := t.TempDir() - h1 := computeRulesHash("", dir, "1.0.0") - // Adding a project rule file changes the hash. + newResolver := func() rules.Resolver { + t.Helper() + r, _, err := rules.NewResolver(dir, "") + if err != nil { + t.Fatal(err) + } + return r + } + writeFile := func(path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + h1 := computeRulesHash(newResolver(), "1.0.0") + + // Adding a project rule layer changes the hash. rulesDir := filepath.Join(dir, ".opencodereview") if err := os.MkdirAll(rulesDir, 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(rulesDir, "rule.json"), []byte(`{"rules":[]}`), 0o644); err != nil { - t.Fatal(err) - } - h2 := computeRulesHash("", dir, "1.0.0") + writeFile(filepath.Join(rulesDir, "rule.json"), `{"rules":[{"path":"**/*.go","rule":"team.md"}]}`) + writeFile(filepath.Join(dir, "team.md"), "rule text A") + h2 := computeRulesHash(newResolver(), "1.0.0") if h1 == h2 { t.Error("rules hash should change when a rule layer appears") } + + // Editing the referenced rule file changes the effective rules, so the + // hash must change even though rule.json's own bytes are unchanged. + writeFile(filepath.Join(dir, "team.md"), "rule text B") + h3 := computeRulesHash(newResolver(), "1.0.0") + if h3 == h2 { + t.Error("rules hash should change when a referenced rule file changes") + } + // Version bump changes the embedded-system contribution. - if computeRulesHash("", dir, "2.0.0") == h2 { + if computeRulesHash(newResolver(), "2.0.0") == h3 { t.Error("rules hash should change when tool version changes") } } diff --git a/cmd/opencodereview/scan_cmd.go b/cmd/opencodereview/scan_cmd.go index e9b7daaa..554d62b7 100644 --- a/cmd/opencodereview/scan_cmd.go +++ b/cmd/opencodereview/scan_cmd.go @@ -187,7 +187,7 @@ func runScan(args []string) error { lang = rt.AppCfg.Language } // Scan has no diff range; resolvedRange stays empty. - runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, opts.rulePath, opts.concurrency, resolvedRange{}) + runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, cc.Resolver, opts.concurrency, resolvedRange{}) ag := scan.NewAgent(scan.Args{ RepoDir: cc.RepoDir, diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 2d182490..1a466439 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "runtime/debug" - "sort" "strings" "sync" "sync/atomic" @@ -386,8 +385,9 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error return nil, fmt.Errorf("all diffs filtered out by token size") } // Establish the coverage denominator and input-identity checksum before - // resume/dispatch so the manifest invariant selected == completed + reused - // + failed + waived holds. + // resume/dispatch so every dispatched outcome lands in a bucket selected + // already covers (selected is a superset of completed ∪ reused ∪ failed + // ∪ waived; equality only for fully-covered runs). a.recordSelectionAndArtifact() toDispatch := a.applyResume(a.diffs) @@ -567,9 +567,7 @@ func (a *Agent) recordSelectionAndArtifact() { fps = append(fps, reviewItemFingerprint(mode, d)) } a.session.SetSelected(selected) - sort.Strings(fps) - sum := sha256.Sum256([]byte(strings.Join(fps, "\x00"))) - a.session.SetArtifactChecksum(fmt.Sprintf("%x", sum)) + a.session.SetArtifactChecksum(session.ArtifactChecksum(fps)) } func resumedFromSession(resume *session.ResumeState) string { @@ -858,8 +856,14 @@ func (a *Agent) filterLargeDiffs(diffs []model.Diff) []model.Diff { for _, d := range diffs { tokens := llm.CountTokens(d.Diff) if tokens > limit { - fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s (~%d tokens exceeds 80%% of max_tokens(%d))\n", - d.NewPath, tokens, a.args.Template.MaxTokens) + msg := fmt.Sprintf("diff tokens (~%d) exceed 80%% of max_tokens(%d)", tokens, a.args.Template.MaxTokens) + fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s (%s)\n", d.NewPath, msg) + // Record the drop so the manifest's coverage denominator still + // counts this file (as failed/skipped_limit) instead of silently + // deselecting it — mirrors the prompt-level threshold path. + a.recordWarning("token_threshold_exceeded", d.NewPath, msg) + a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath, + reviewItemFingerprint(a.reviewMode(), d), session.FailureSkippedLimit, msg) skipped++ continue } diff --git a/internal/agent/manifest_test.go b/internal/agent/manifest_test.go index fea875e5..ac20536b 100644 --- a/internal/agent/manifest_test.go +++ b/internal/agent/manifest_test.go @@ -58,18 +58,43 @@ func classCounts(m *session.RunManifest) map[string]int { return out } +// newManifestAgent builds an Agent wired to the stub routeClient with the +// template/tool boilerplate shared by every test in this file. mutate (may be +// nil) tweaks the Args before construction. +func newManifestAgent(sess *session.SessionHistory, mainContent string, maxTokens int, mutate func(*Args)) *Agent { + args := Args{ + LLMClient: routeClient{}, + Model: "test", + Session: sess, + Tools: tool.NewRegistry(), + Template: template.Template{ + MaxTokens: maxTokens, + MaxToolRequestTimes: 5, + MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: mainContent}}}, + }, + MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done", Description: "done"}}}, + } + if mutate != nil { + mutate(&args) + } + a := New(args) + a.currentDate = "2026-07-15 08:00" + return a +} + func TestRunManifest_Scenarios(t *testing.T) { bigTpl := strings.Repeat("word ", 120) + " {{diff}}" tests := []struct { - name string - maxTokens int - mainContent string - diffs []model.Diff - markCancelled bool - wantState string - wantSelected int - wantClasses map[string]int + name string + maxTokens int + mainContent string + diffs []model.Diff + markCancelled bool + wantState string + wantSelected int + wantClasses map[string]int + wantNoArtifact bool }{ { name: "success", @@ -132,24 +157,40 @@ func TestRunManifest_Scenarios(t *testing.T) { wantSelected: 1, wantClasses: map[string]int{session.FailureSkippedLimit: 1}, }, + { + // The diff-content pre-filter (filterLargeDiffs) must record the + // drop, not silently deselect: an all-filtered run is failed with + // skipped_limit failures, never an empty "skipped". No artifact + // checksum — nothing was dispatched. + name: "oversized diff pre-filtered is failed not skipped", + maxTokens: 50, + mainContent: "review {{diff}}", + diffs: []model.Diff{{NewPath: "huge.go", OldPath: "huge.go", Diff: strings.Repeat("word ", 500), Insertions: 1}}, + wantState: session.StateFailed, + wantSelected: 1, + wantClasses: map[string]int{session.FailureSkippedLimit: 1}, + wantNoArtifact: true, + }, + { + // Partial pre-filter: the dropped file stays in the coverage + // denominator (selected=2) and the run is partial, not complete. + name: "oversized diff pre-filtered alongside success is partial", + maxTokens: 50, + mainContent: "review {{diff}}", + diffs: []model.Diff{ + {NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1}, + {NewPath: "huge.go", OldPath: "huge.go", Diff: strings.Repeat("word ", 500), Insertions: 1}, + }, + wantState: session.StatePartial, + wantSelected: 2, + wantClasses: map[string]int{session.FailureSkippedLimit: 1}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange}) - a := New(Args{ - LLMClient: routeClient{}, - Model: "test", - Session: sess, - Tools: tool.NewRegistry(), - Template: template.Template{ - MaxTokens: tt.maxTokens, - MaxToolRequestTimes: 5, - MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: tt.mainContent}}}, - }, - MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done", Description: "done"}}}, - }) - a.currentDate = "2026-07-15 08:00" + a := newManifestAgent(sess, tt.mainContent, tt.maxTokens, nil) a.diffs = tt.diffs _, _ = a.dispatchSubtasks(context.Background()) @@ -168,7 +209,11 @@ func TestRunManifest_Scenarios(t *testing.T) { if len(m.Files.Selected) != tt.wantSelected { t.Errorf("selected = %v, want %d", m.Files.Selected, tt.wantSelected) } - if m.ArtifactSHA256 == "" { + if tt.wantNoArtifact { + if m.ArtifactSHA256 != "" { + t.Errorf("artifact checksum = %q, want empty (nothing dispatched)", m.ArtifactSHA256) + } + } else if m.ArtifactSHA256 == "" { t.Error("artifact checksum should be recorded") } got := classCounts(m) @@ -189,22 +234,11 @@ func TestRunManifest_Scenarios(t *testing.T) { func TestWaiveCoverage(t *testing.T) { build := func(waive []string) *session.RunManifest { sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange}) - a := New(Args{ - LLMClient: routeClient{}, - Model: "test", - Session: sess, - Tools: tool.NewRegistry(), - Template: template.Template{ - MaxTokens: 100000, - MaxToolRequestTimes: 5, - MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}}, - }, - MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done"}}}, + a := newManifestAgent(sess, "{{diff}}", 100000, func(args *Args) { // Resume must be non-nil for applyResume (and thus waive) to run. - Resume: &session.ResumeState{SessionID: "prev", Items: map[string]session.ResumeItem{}}, - WaivePaths: waive, + args.Resume = &session.ResumeState{SessionID: "prev", Items: map[string]session.ResumeItem{}} + args.WaivePaths = waive }) - a.currentDate = "d" a.diffs = []model.Diff{ {NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1}, {NewPath: "flaky.go", OldPath: "flaky.go", Diff: "+DO_FAIL", Insertions: 1}, @@ -242,19 +276,7 @@ func TestWaiveCoverage(t *testing.T) { func TestArtifactChecksumStableAndOrderIndependent(t *testing.T) { mk := func(order []model.Diff) string { sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange}) - a := New(Args{ - LLMClient: routeClient{}, - Model: "test", - Session: sess, - Tools: tool.NewRegistry(), - Template: template.Template{ - MaxTokens: 100000, - MaxToolRequestTimes: 5, - MainTask: template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}}, - }, - MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done"}}}, - }) - a.currentDate = "d" + a := newManifestAgent(sess, "{{diff}}", 100000, nil) a.diffs = order a.recordSelectionAndArtifact() sess.Finalize() diff --git a/internal/config/rules/system_rules.go b/internal/config/rules/system_rules.go index 6f4cf8d8..1cd9021c 100644 --- a/internal/config/rules/system_rules.go +++ b/internal/config/rules/system_rules.go @@ -288,6 +288,13 @@ func NewResolver(repoDir, customRulePath string) (Resolver, *FileFilter, error) }, filter, nil } +// UserLayers returns the loaded user rule layers (any may be nil) with file +// references already resolved to content — the effective rule inputs, for +// manifest fingerprinting. +func (r *composedResolver) UserLayers() (custom, project, global *ProjectRule) { + return r.custom, r.project, r.global +} + // buildFileFilter picks the highest-priority layer that has any include/exclude // configured. Priority order: custom (--rule) > project > global. func buildFileFilter(layers ...*ProjectRule) *FileFilter { diff --git a/internal/scan/agent.go b/internal/scan/agent.go index 1b01c103..da3c1c7b 100644 --- a/internal/scan/agent.go +++ b/internal/scan/agent.go @@ -6,7 +6,7 @@ import ( "encoding/json" "errors" "fmt" - "sort" + "runtime/debug" "strings" "sync" "sync/atomic" @@ -346,8 +346,14 @@ func (a *Agent) filterLargeScans(items []model.ScanItem) []model.ScanItem { for _, it := range items { tokens := llm.CountTokens(it.Content) if tokens > limit { - fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s (~%d tokens exceeds 80%% of max_tokens(%d))\n", - it.Path, tokens, a.args.Template.MaxTokens) + msg := fmt.Sprintf("content tokens (~%d) exceed 80%% of max_tokens(%d)", tokens, a.args.Template.MaxTokens) + fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s (%s)\n", it.Path, msg) + // Record the drop so the manifest's coverage denominator still + // counts this file (as failed/skipped_limit) instead of silently + // deselecting it — mirrors the prompt-level threshold path. + a.recordWarning("token_threshold_exceeded", it.Path, msg) + a.session.RecordReviewItemFailed(it.Path, it.Path, it.Path, + scanFingerprint(it), session.FailureSkippedLimit, msg) skipped++ continue } @@ -459,13 +465,11 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error } // scanFingerprint derives a stable per-item fingerprint for the manifest. -// -// ponytail: content-hash if scan grows resume. Scan has no diff and no resume -// today, so a path-based fingerprint is enough to identify the item and to -// compute the artifact checksum; upgrade to hashing it.Content when resume -// lands on the scan path. -func scanFingerprint(path string) string { - sum := sha256.Sum256([]byte(session.ReviewModeFullScan + "\x00" + path)) +// Content is hashed alongside the path (the item already carries the full +// file in memory), so the artifact checksum changes whenever the scanned +// inputs do — same guarantee the diff path gets from hashing d.Diff. +func scanFingerprint(it model.ScanItem) string { + sum := sha256.Sum256([]byte(session.ReviewModeFullScan + "\x00" + it.Path + "\x00" + it.Content)) return fmt.Sprintf("%x", sum) } @@ -477,12 +481,10 @@ func (a *Agent) recordSelectionAndArtifact() { fps := make([]string, 0, len(a.items)) for i := range a.items { selected = append(selected, a.items[i].Path) - fps = append(fps, scanFingerprint(a.items[i].Path)) + fps = append(fps, scanFingerprint(a.items[i])) } a.session.SetSelected(selected) - sort.Strings(fps) - sum := sha256.Sum256([]byte(strings.Join(fps, "\x00"))) - a.session.SetArtifactChecksum(fmt.Sprintf("%x", sum)) + a.session.SetArtifactChecksum(session.ArtifactChecksum(fps)) } // resolveBatchStrategy reads the strategy from the scan template, defaulting @@ -544,6 +546,21 @@ func (a *Agent) dispatchBatch(ctx context.Context, batchIdx int, batch []model.S go func(it model.ScanItem) { defer wg.Done() defer func() { <-sem }() + // A panic while scanning one file must be isolated exactly like an + // error return: counted in subtaskFailed and recorded as a failed + // item with class "panic", so other files still complete and the + // manifest is still finalized. Mirrors the diff-review dispatch. + defer func() { + if r := recover(); r != nil { + atomic.AddInt64(&a.subtaskFailed, 1) + a.session.RecordReviewItemFailed(it.Path, it.Path, it.Path, scanFingerprint(it), session.FailurePanic, fmt.Sprintf("panic: %v", r)) + fmt.Fprintf(stdout.Writer(), "[ocr] Scan subtask panic for %s (batch #%d): %v\n%s\n", it.Path, batchIdx, r, debug.Stack()) + telemetry.ErrorEvent(ctx, "scan.subtask.panic", fmt.Errorf("panic: %v", r), + telemetry.AnyToAttr("file.path", it.Path), + telemetry.AnyToAttr("batch.index", batchIdx)) + a.recordWarning("scan_subtask_error", it.Path, fmt.Sprintf("panic: %v", r)) + } + }() var fileCtx context.Context var cancel context.CancelFunc @@ -597,7 +614,7 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error { messages := a.renderMessages(it, rule, planGuidance) - fingerprint := scanFingerprint(it.Path) + fingerprint := scanFingerprint(it) tokenCount := llmloop.CountMessagesTokens(messages) maxAllowed := a.args.Template.MaxTokens @@ -619,6 +636,13 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error { a.session.RecordReviewItemFailed(it.Path, it.Path, it.Path, fingerprint, session.ClassifyFailure(err), err.Error()) return err } + // The async CommentWorkerPool may still be resolving code_comment calls + // for this file when RunPerFile returns; drain it before reading the + // collector so the persisted checkpoint carries the full comment set + // (mirrors the diff-review path's per-file Await). + if a.args.CommentWorkerPool != nil { + a.args.CommentWorkerPool.Await() + } comments := a.args.CommentCollector.CommentsForPath(it.Path) a.session.RecordReviewItemDone(it.Path, it.Path, it.Path, fingerprint, comments) return nil diff --git a/internal/scan/manifest_test.go b/internal/scan/manifest_test.go index 1a634f71..a71c305e 100644 --- a/internal/scan/manifest_test.go +++ b/internal/scan/manifest_test.go @@ -24,6 +24,8 @@ func (scanRouteClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequest } text := sb.String() switch { + case strings.Contains(text, "DO_PANIC"): + panic("scan client exploded") case strings.Contains(text, "DO_CANCEL"): return nil, fmt.Errorf("scan call: %w", context.Canceled) case strings.Contains(text, "DO_FAIL"): @@ -51,6 +53,19 @@ func scanClassCounts(m *session.RunManifest) map[string]int { return out } +// The scan fingerprint must be content-sensitive: the same path with changed +// content is a different input, so the manifest's artifact checksum changes. +func TestScanFingerprintContentSensitive(t *testing.T) { + a := scanFingerprint(model.ScanItem{Path: "a.go", Content: "v1"}) + b := scanFingerprint(model.ScanItem{Path: "a.go", Content: "v2"}) + if a == b { + t.Error("fingerprint identical across different file contents") + } + if a != scanFingerprint(model.ScanItem{Path: "a.go", Content: "v1"}) { + t.Error("fingerprint not stable for identical input") + } +} + func TestScanRunManifest_Parity(t *testing.T) { tests := []struct { name string @@ -100,6 +115,17 @@ func TestScanRunManifest_Parity(t *testing.T) { wantSelected: 2, wantClasses: map[string]int{session.FailureCancelled: 1}, }, + { + name: "panic isolated to one file", + maxTokens: 100000, + items: []model.ScanItem{ + {Path: "ok.go", Content: "ok", LineCount: 1}, + {Path: "boom.go", Content: "DO_PANIC", LineCount: 1}, + }, + wantState: session.StatePartial, + wantSelected: 2, + wantClasses: map[string]int{session.FailurePanic: 1}, + }, { name: "token limit skip", maxTokens: 10, @@ -165,3 +191,40 @@ func TestScanRunManifest_Parity(t *testing.T) { }) } } + +// TestFilterLargeScansRecordsSkip verifies the content pre-filter records a +// dropped item as failed/skipped_limit so it stays in the manifest's coverage +// denominator instead of being silently deselected. +func TestFilterLargeScansRecordsSkip(t *testing.T) { + tpl := makeTemplateWithFullScan() + tpl.MaxTokens = 50 + sess := session.New(t.TempDir(), "", "test", session.SessionOptions{ReviewMode: session.ReviewModeFullScan}) + a := NewAgent(Args{ + Template: tpl, + LLMClient: scanRouteClient{}, + Model: "test", + CommentCollector: tool.NewCommentCollector(), + Tools: tool.NewRegistry(), + Session: sess, + }) + + kept := a.filterLargeScans([]model.ScanItem{ + {Path: "ok.go", Content: "ok", LineCount: 1}, + {Path: "huge.go", Content: strings.Repeat("word ", 500), LineCount: 1}, + }) + if len(kept) != 1 || kept[0].Path != "ok.go" { + t.Fatalf("kept = %+v, want only ok.go", kept) + } + + sess.Finalize() + m := sess.Manifest() + if m == nil { + t.Fatal("nil manifest") + } + if len(m.Files.Failed) != 1 || m.Files.Failed[0] != "huge.go" { + t.Errorf("failed = %v, want [huge.go]", m.Files.Failed) + } + if got := scanClassCounts(m); got[session.FailureSkippedLimit] != 1 { + t.Errorf("failure classes = %v, want skipped_limit=1", got) + } +} diff --git a/internal/session/acceptance_test.go b/internal/session/acceptance_test.go new file mode 100644 index 00000000..9cd1342d --- /dev/null +++ b/internal/session/acceptance_test.go @@ -0,0 +1,235 @@ +package session + +import ( + "encoding/json" + "strings" + "testing" +) + +// TestManifestAcceptanceMatrix walks the terminal-state scenarios end to end +// through the SessionHistory API (the same calls the agent/scan dispatch paths +// make) and asserts the persisted + in-memory manifest agree with the coverage. +func TestManifestAcceptanceMatrix(t *testing.T) { + type step struct { + kind string // done | reused | failed | waived + path string + } + tests := []struct { + name string + selected []string + steps []step + cancelled bool + wantState string + wantClasses map[string]int + }{ + { + name: "clean complete", + selected: []string{"a.go", "b.go"}, + steps: []step{{"done", "a.go"}, {"done", "b.go"}}, + wantState: StateComplete, + }, + { + name: "partial with a reuse and a failure", + selected: []string{"a.go", "b.go", "c.go"}, + steps: []step{{"done", "a.go"}, {"reused", "b.go"}, {"failed", "c.go"}}, + wantState: StatePartial, + wantClasses: map[string]int{FailureProviderError: 1}, + }, + { + name: "all failed", + selected: []string{"a.go"}, + steps: []step{{"failed", "a.go"}}, + wantState: StateFailed, + }, + { + name: "skipped when nothing selected", + selected: nil, + steps: nil, + wantState: StateSkipped, + }, + { + name: "cancelled but one succeeded is partial", + selected: []string{"a.go", "b.go"}, + steps: []step{{"done", "a.go"}, {"failed", "b.go"}}, + cancelled: true, + wantState: StatePartial, + }, + { + // selected is a SUPERSET here: b.go and c.go were never dispatched, + // so they land in no outcome bucket — exactly the truncated-run + // shape the partial state exists for. + name: "cancelled mid-run leaves uncovered files", + selected: []string{"a.go", "b.go", "c.go"}, + steps: []step{{"done", "a.go"}}, + cancelled: true, + wantState: StatePartial, + }, + { + name: "waive completes an otherwise-partial run", + selected: []string{"a.go", "b.go"}, + steps: []step{{"done", "a.go"}, {"waived", "b.go"}}, + wantState: StateComplete, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sh := New(t.TempDir(), "main", "m", SessionOptions{ReviewMode: ReviewModeRange}) + sh.SetSelected(tt.selected) + sh.SetArtifactChecksum("artifact-sum") + for _, s := range tt.steps { + switch s.kind { + case "done": + sh.RecordReviewItemDone(s.path, s.path, s.path, "fp-"+s.path, nil) + case "reused": + sh.RecordReviewItemReused(s.path, s.path, s.path, "fp-"+s.path, "prev", nil) + case "failed": + sh.RecordReviewItemFailed(s.path, s.path, s.path, "fp-"+s.path, FailureProviderError, "boom") + case "waived": + sh.RecordReviewItemWaived(s.path, s.path, s.path, "fp-"+s.path) + } + } + if tt.cancelled { + sh.MarkCancelled() + } + sh.Finalize() + + m := sh.Manifest() + if m == nil { + t.Fatal("nil manifest") + } + if m.State != tt.wantState { + t.Errorf("state = %q, want %q (files=%+v)", m.State, tt.wantState, m.Files) + } + // Coverage invariant: selected is a superset of the outcome sets — + // every recorded step lands in exactly one bucket, and selected + // keeps the full denominator even when steps don't cover it. + sum := len(m.Files.Completed) + len(m.Files.Reused) + len(m.Files.Failed) + len(m.Files.Waived) + if sum != len(tt.steps) { + t.Errorf("outcome buckets hold %d paths, want %d (one per step): %+v", sum, len(tt.steps), m.Files) + } + if len(m.Files.Selected) != len(tt.selected) { + t.Errorf("selected = %v, want the full denominator %v", m.Files.Selected, tt.selected) + } + if len(tt.selected) > 0 && m.ArtifactSHA256 != "artifact-sum" { + t.Errorf("artifact_sha256 = %q, want artifact-sum", m.ArtifactSHA256) + } + gotClasses := map[string]int{} + for _, f := range m.Failures { + gotClasses[f.Class]++ + } + for k, v := range tt.wantClasses { + if gotClasses[k] != v { + t.Errorf("failure class %q = %d, want %d (all=%v)", k, gotClasses[k], v, gotClasses) + } + } + + // Persisted last record matches the in-memory state. + path, err := SessionFilePath(sh.RepoDir, sh.SessionID) + if err != nil { + t.Fatal(err) + } + rec := lastJSONLRecord(t, path) + if rec["type"] != "run_manifest" || rec["state"] != tt.wantState { + t.Errorf("persisted manifest mismatch: type=%v state=%v want %q", rec["type"], rec["state"], tt.wantState) + } + }) + } +} + +// TestProviderTransitionOnResume covers the issue-367 resume contract in one +// flow: a mixed-success run terminates partial; a resume (here also switching +// provider + model) reuses the succeeded item, re-reviews the failed one, and +// flips the run to complete — recording the new provider/model, linking +// parent_session_id, and preserving the input identity (same artifact +// checksum, since the inputs are unchanged). +func TestProviderTransitionOnResume(t *testing.T) { + repo := t.TempDir() + artifact := ArtifactChecksum([]string{"fp-a", "fp-b"}) + + // First run: anthropic; a.go succeeds, b.go fails -> partial. + s1 := New(repo, "main", "claude-A", SessionOptions{ + ReviewMode: ReviewModeRange, + RunMeta: RunMeta{Provider: "anthropic", OCRVersion: "1.0.0"}, + }) + s1.SetSelected([]string{"a.go", "b.go"}) + s1.SetArtifactChecksum(artifact) + s1.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", nil) + s1.RecordReviewItemFailed("b.go", "b.go", "b.go", "fp-b", FailureProviderError, "boom") + s1.Finalize() + if got := s1.Manifest().State; got != StatePartial { + t.Fatalf("first run state = %q, want partial", got) + } + + // Second run: resume from s1, now on openai with a different model. a.go is + // reused from the prior run; the failed b.go is re-reviewed successfully. + s2 := New(repo, "main", "gpt-B", SessionOptions{ + ReviewMode: ReviewModeRange, + ResumedFrom: s1.SessionID, + RunMeta: RunMeta{Provider: "openai", OCRVersion: "1.0.0"}, + }) + s2.SetSelected([]string{"a.go", "b.go"}) + s2.SetArtifactChecksum(artifact) + s2.RecordReviewItemReused("a.go", "a.go", "a.go", "fp-a", s1.SessionID, nil) + s2.RecordReviewItemDone("b.go", "b.go", "b.go", "fp-b", nil) + s2.Finalize() + + m := s2.Manifest() + if m.Provider != "openai" { + t.Errorf("provider = %q, want openai (transition recorded)", m.Provider) + } + if m.Model != "gpt-B" { + t.Errorf("model = %q, want gpt-B", m.Model) + } + if m.ParentSessionID != s1.SessionID { + t.Errorf("parent_session_id = %q, want %q", m.ParentSessionID, s1.SessionID) + } + if len(m.Files.Reused) != 1 || m.Files.Reused[0] != "a.go" { + t.Errorf("reused = %v, want [a.go] (reused items intact)", m.Files.Reused) + } + if m.State != StateComplete { + t.Errorf("state = %q, want complete (resume covered the failed item)", m.State) + } + if m.ArtifactSHA256 != s1.Manifest().ArtifactSHA256 { + t.Errorf("artifact checksum changed across resume: %q vs %q (input identity must be retained)", + m.ArtifactSHA256, s1.Manifest().ArtifactSHA256) + } +} + +// TestManifestNeverLeaksSecrets is the load-bearing redaction guard: a fully +// populated manifest, serialized, must contain none of the token/auth-header +// patterns. The RunManifest/RunMeta structs carry no secret-bearing field, so +// this locks that shape in place. +func TestManifestNeverLeaksSecrets(t *testing.T) { + sh := New(t.TempDir(), "main", "claude-opus", SessionOptions{ + ReviewMode: ReviewModeRange, + DiffFrom: "base", + DiffTo: "head", + RunMeta: RunMeta{ + OCRVersion: "1.2.3", + Provider: "anthropic", + Concurrency: 4, + ConfigHash: "deadbeef", + RulesHash: "cafebabe", + RepoRemoteURL: "https://example.com/acme/repo.git", + RepoHeadSHA: "0123456789abcdef0123456789abcdef01234567", + RangeFromSHA: "aaaa", + RangeToSHA: "bbbb", + }, + }) + sh.SetSelected([]string{"a.go"}) + sh.SetArtifactChecksum("artifact-sum") + sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", nil) + sh.Finalize() + + blob, err := json.Marshal(sh.Manifest()) + if err != nil { + t.Fatal(err) + } + lower := strings.ToLower(string(blob)) + for _, bad := range []string{"api_key", "auth_token", "authorization", "x-api-key", "bearer", "sk-ant", "supersecret", "password", "token\"", ":token@"} { + if strings.Contains(lower, strings.ToLower(bad)) { + t.Errorf("manifest leaks sensitive pattern %q: %s", bad, blob) + } + } +} diff --git a/internal/session/history.go b/internal/session/history.go index 2aa67fc9..67327a37 100644 --- a/internal/session/history.go +++ b/internal/session/history.go @@ -5,6 +5,7 @@ package session import ( "fmt" + "os" "sync" "sync/atomic" "time" @@ -169,7 +170,7 @@ func New(repoDir, gitBranch, model string, opts SessionOptions) *SessionHistory p, err := newJSONLWriter(sessionID, repoDir, gitBranch, model, opts) if err != nil { - fmt.Printf("[ocr session] warning: failed to create session writer: %v\n", err) + fmt.Fprintf(os.Stderr, "[ocr session] warning: failed to create session writer: %v\n", err) } else { sh.persist = p p.WriteSessionStart(sh.StartTime) @@ -272,8 +273,9 @@ func (sh *SessionHistory) RecordReviewItemWaived(filePath, oldPath, newPath, fin } // SetSelected records the full set of reviewable file paths for this run. It -// establishes the coverage denominator: selected == completed + reused + -// failed + waived. Call it after diffs are computed and before dispatch. +// establishes the coverage denominator: selected is a superset of +// completed ∪ reused ∪ failed ∪ waived, with equality only for fully-covered +// runs. Call it after diffs are computed and before dispatch. func (sh *SessionHistory) SetSelected(paths []string) { if sh == nil { return @@ -328,6 +330,13 @@ func (sh *SessionHistory) trackCoverage(set, path string) { // the final summary record. func (sh *SessionHistory) Finalize() { sh.mu.Lock() + if sh.manifest != nil { + // Already finalized. The manifest is written exactly once (its + // immutability contract), so a second Finalize is a no-op rather than a + // duplicate session_end + run_manifest pair on an already-closed file. + sh.mu.Unlock() + return + } sh.EndTime = time.Now() p := sh.persist duration := sh.EndTime.Sub(sh.StartTime) @@ -343,7 +352,7 @@ func (sh *SessionHistory) Finalize() { if p != nil { // session_end first, then the run_manifest as the final line so the // manifest is always the last record (immutability = written once, - // last). WriteRunManifest flushes and closes the file. + // last); flushAndClose then closes the file. p.WriteSessionEnd(duration, filesReviewed, failures) p.WriteRunManifest(manifest) p.flushAndClose() diff --git a/internal/session/manifest.go b/internal/session/manifest.go index 1e3dc986..3a54c5c1 100644 --- a/internal/session/manifest.go +++ b/internal/session/manifest.go @@ -2,10 +2,24 @@ package session import ( "context" + "crypto/sha256" + "encoding/hex" "errors" "sort" + "strings" ) +// ArtifactChecksum computes the run's input-identity checksum: sha256 over +// the sorted per-file fingerprints joined with NUL. Order-independent by +// construction; both the diff-review and full-scan paths feed it so the +// convention cannot diverge. +func ArtifactChecksum(fingerprints []string) string { + fps := append([]string(nil), fingerprints...) + sort.Strings(fps) + sum := sha256.Sum256([]byte(strings.Join(fps, "\x00"))) + return hex.EncodeToString(sum[:]) +} + // Failure classes recorded per failed review item. ClassifyFailure maps a Go // error onto one of these; panic and skipped_limit are set explicitly by the // dispatch paths that detect them. @@ -66,8 +80,11 @@ type ManifestRange struct { } // ManifestFiles holds the per-outcome coverage sets, each a sorted, de-duped -// list of file paths. selected == completed + reused + failed + waived is the -// coverage invariant the manifest asserts. +// list of file paths. selected is the coverage denominator; it is a superset of +// completed ∪ reused ∪ failed ∪ waived, with equality only for fully-covered +// runs. A cancelled or budget-truncated run leaves some selected files in no +// outcome bucket (selected > the sum), which is exactly what yields a "partial" +// terminal state. type ManifestFiles struct { Selected []string `json:"selected"` Completed []string `json:"completed"` @@ -124,7 +141,10 @@ func ComputeTerminalState(selected, completed, reused, failed, waived int, cance return StateSkipped } if cancelled { - if completed+reused > 0 { + // A waive satisfies the coverage contract just like a completed review, + // so a cancelled run whose covered items include waives is partial, not + // failed. + if completed+reused+waived > 0 { return StatePartial } return StateFailed diff --git a/internal/session/manifest_test.go b/internal/session/manifest_test.go index b8c30f58..d6a85259 100644 --- a/internal/session/manifest_test.go +++ b/internal/session/manifest_test.go @@ -4,6 +4,7 @@ import ( "bufio" "encoding/json" "os" + "sort" "strings" "testing" ) @@ -29,6 +30,7 @@ func TestComputeTerminalState(t *testing.T) { {"cancelled with nothing done", 5, 0, 0, 0, 0, true, StateFailed}, {"cancelled with only failures", 5, 0, 0, 2, 0, true, StateFailed}, {"cancelled overrides otherwise-complete", 2, 2, 0, 0, 0, true, StatePartial}, + {"cancelled with only waives", 3, 0, 0, 0, 2, true, StatePartial}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -73,66 +75,52 @@ func lastJSONLRecord(t *testing.T, path string) map[string]any { return last } -func TestFinalizeWritesRunManifest(t *testing.T) { - sh := New(t.TempDir(), "main", "test-model", SessionOptions{ - ReviewMode: ReviewModeRange, - DiffFrom: "base", - DiffTo: "head", - RunMeta: RunMeta{ - OCRVersion: "1.2.3", - Provider: "anthropic", - Concurrency: 4, - ConfigHash: "cfg-hash", - RulesHash: "rules-hash", - }, - }) - sh.SetSelected([]string{"a.go", "b.go", "c.go"}) +// TestManifestSchemaLock pins the versioned schema contract: the literal +// version number and the top-level JSON key set. Either changing without the +// other is a breaking change for downstream readers; this test makes both +// deliberate. +func TestManifestSchemaLock(t *testing.T) { + if ManifestSchemaVersion != 1 { + t.Fatalf("ManifestSchemaVersion = %d; bumping it requires a schema-change review — update this lock and the docs together", ManifestSchemaVersion) + } + + sh := New(t.TempDir(), "main", "m", SessionOptions{ReviewMode: ReviewModeRange}) + sh.SetSelected([]string{"a.go", "b.go"}) sh.SetArtifactChecksum("artifact-sum") sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", nil) - sh.RecordReviewItemReused("b.go", "b.go", "b.go", "fp-b", "old-session", nil) - sh.RecordReviewItemFailed("c.go", "c.go", "c.go", "fp-c", FailureProviderError, "boom") + sh.RecordReviewItemFailed("b.go", "b.go", "b.go", "fp-b", FailureProviderError, "boom") sh.Finalize() - // In-memory manifest matches what we persisted. - m := sh.Manifest() - if m == nil { - t.Fatal("Manifest() returned nil after Finalize") - } - if m.State != StatePartial { - t.Errorf("state = %q, want partial", m.State) - } - if m.SchemaVersion != ManifestSchemaVersion { - t.Errorf("schema_version = %d", m.SchemaVersion) - } - if len(m.Files.Selected) != 3 || len(m.Files.Completed) != 1 || len(m.Files.Reused) != 1 || len(m.Files.Failed) != 1 { - t.Errorf("coverage = %+v", m.Files) - } - if len(m.Failures) != 1 || m.Failures[0].Class != FailureProviderError { - t.Errorf("failures = %+v", m.Failures) - } - - // The LAST JSONL line is the run_manifest record with matching coverage. - path, err := SessionFilePath(sh.RepoDir, sh.SessionID) + blob, err := json.Marshal(sh.Manifest()) if err != nil { t.Fatal(err) } - rec := lastJSONLRecord(t, path) - if rec["type"] != "run_manifest" { - t.Fatalf("last record type = %v, want run_manifest", rec["type"]) + var rec map[string]any + if err := json.Unmarshal(blob, &rec); err != nil { + t.Fatal(err) } - if rec["state"] != StatePartial { - t.Errorf("persisted state = %v", rec["state"]) + if v, ok := rec["schema_version"].(float64); !ok || v != 1 { + t.Errorf("serialized schema_version = %v, want 1", rec["schema_version"]) } - - // No token/secret content leaks into the manifest record. The endpoint - // token / auth header must never appear anywhere in the serialized line. - blob, err := json.Marshal(rec) - if err != nil { - t.Fatal(err) + // Golden top-level key set (omitempty keys included only when populated + // here). Adding/renaming a key is a schema change: bump the version. + want := []string{ + "schema_version", "session_id", "repo", "range", "files", "state", + "duration_ms", "review_mode", "model", "artifact_sha256", "failures", + "started_at", "completed_at", } - for _, bad := range []string{"api_key", "auth_token", "sk-ant", "authorization", "x-api-key", "Bearer"} { - if strings.Contains(strings.ToLower(string(blob)), strings.ToLower(bad)) { - t.Errorf("manifest leaks sensitive token pattern %q: %s", bad, blob) + for _, k := range want { + if _, ok := rec[k]; !ok { + t.Errorf("manifest missing locked key %q (keys=%v)", k, keysOf(rec)) } } } + +func keysOf(m map[string]any) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/internal/session/persist.go b/internal/session/persist.go index f3f00d45..6f1e0059 100644 --- a/internal/session/persist.go +++ b/internal/session/persist.go @@ -120,7 +120,7 @@ func (jw *jsonlWriter) open() error { func (jw *jsonlWriter) writeRecordLocked(rec map[string]any) { data, err := json.Marshal(rec) if err != nil { - fmt.Printf("[ocr session] failed to marshal record: %v\n", err) + fmt.Fprintf(os.Stderr, "[ocr session] failed to marshal record: %v\n", err) return } jw.writer.Write(data) @@ -334,12 +334,12 @@ func (jw *jsonlWriter) WriteRunManifest(m *RunManifest) string { } data, err := json.Marshal(m) if err != nil { - fmt.Printf("[ocr session] failed to marshal run manifest: %v\n", err) + fmt.Fprintf(os.Stderr, "[ocr session] failed to marshal run manifest: %v\n", err) return "" } rec := make(map[string]any) if err := json.Unmarshal(data, &rec); err != nil { - fmt.Printf("[ocr session] failed to encode run manifest: %v\n", err) + fmt.Fprintf(os.Stderr, "[ocr session] failed to encode run manifest: %v\n", err) return "" } @@ -359,7 +359,9 @@ func (jw *jsonlWriter) WriteRunManifest(m *RunManifest) string { return uuid } -// WriteSessionEnd writes the final session_end summary record and closes the file. +// WriteSessionEnd writes the final session_end summary record. It flushes but +// does not close the file; Finalize writes the run_manifest after it and then +// closes via flushAndClose. func (jw *jsonlWriter) WriteSessionEnd(duration time.Duration, filesReviewed []string, llmFailures int64) { uuid := generateUUID() diff --git a/internal/viewer/store.go b/internal/viewer/store.go index dd60fe0a..ce81993a 100644 --- a/internal/viewer/store.go +++ b/internal/viewer/store.go @@ -131,9 +131,10 @@ func peekSession(path string) (SessionSummary, error) { buf := make([]byte, 0, 1024*1024) scanner.Buffer(buf, 10*1024*1024) - var lastLine []byte + var lastLine, prevLine []byte for scanner.Scan() { line := scanner.Bytes() + prevLine = lastLine lastLine = append([]byte(nil), line...) if summary.Timestamp.IsZero() { @@ -168,26 +169,36 @@ func peekSession(path string) (SessionSummary, error) { } } - if len(lastLine) > 0 { + // session_end carries the summary fields. It is the last line for legacy + // sessions, or the second-to-last line when a run_manifest record follows + // it (the manifest is always written immediately after session_end), so + // check both candidates newest-first. + for _, candidate := range [][]byte{lastLine, prevLine} { + if len(candidate) == 0 { + continue + } var rec map[string]any - if err := json.Unmarshal(lastLine, &rec); err == nil { - if typ, _ := rec["type"].(string); typ == "session_end" { - if dur, ok := rec["duration_seconds"].(float64); ok { - summary.DurationSec = dur - } - if files, ok := rec["files_reviewed"].([]any); ok { - summary.FilesReviewed = make([]string, 0, len(files)) - for _, fv := range files { - if s, ok := fv.(string); ok { - summary.FilesReviewed = append(summary.FilesReviewed, s) - } - } - } - if f, ok := rec["llm_failures"].(float64); ok { - summary.LLMFailures = int(f) + if err := json.Unmarshal(candidate, &rec); err != nil { + continue + } + if typ, _ := rec["type"].(string); typ != "session_end" { + continue + } + if dur, ok := rec["duration_seconds"].(float64); ok { + summary.DurationSec = dur + } + if files, ok := rec["files_reviewed"].([]any); ok { + summary.FilesReviewed = make([]string, 0, len(files)) + for _, fv := range files { + if s, ok := fv.(string); ok { + summary.FilesReviewed = append(summary.FilesReviewed, s) } } } + if f, ok := rec["llm_failures"].(float64); ok { + summary.LLMFailures = int(f) + } + break } summary.FileCount = len(summary.FilesReviewed) return summary, scanner.Err() diff --git a/internal/viewer/store_test.go b/internal/viewer/store_test.go index 574f0a4b..440e093c 100644 --- a/internal/viewer/store_test.go +++ b/internal/viewer/store_test.go @@ -218,6 +218,34 @@ func TestPeekSession(t *testing.T) { } } +// TestPeekSession_RunManifestAfterSessionEnd guards the run-manifest ordering +// change: run_manifest is written as the last line, AFTER session_end. The +// peek fast path reads only the tail of the file, so it must still find the +// session_end summary in the second-to-last line rather than giving up because +// the very last line is a run_manifest. +func TestPeekSession_RunManifestAfterSessionEnd(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.jsonl") + writeJSONL(t, path, + `{"type":"session_start","timestamp":"2025-06-15T14:30:00Z","cwd":"/repo","gitBranch":"dev","model":"gpt-4o","reviewMode":"workspace"}`, + `{"type":"session_end","duration_seconds":45.2,"files_reviewed":["main.go","util.go"],"llm_failures":2}`, + `{"type":"run_manifest","schema_version":1,"state":"partial","files":{"selected":["main.go","util.go"]}}`) + + s, err := peekSession(path) + if err != nil { + t.Fatal(err) + } + if s.DurationSec != 45.2 { + t.Errorf("DurationSec = %f, want 45.2 (session_end must be found even when run_manifest is the last line)", s.DurationSec) + } + if s.FileCount != 2 { + t.Errorf("FileCount = %d, want 2", s.FileCount) + } + if s.LLMFailures != 2 { + t.Errorf("LLMFailures = %d, want 2", s.LLMFailures) + } +} + func TestPeekSession_MissingFile(t *testing.T) { _, err := peekSession("/nonexistent/path/session.jsonl") if err == nil { diff --git a/pages/src/content/docs/en/cli-reference.md b/pages/src/content/docs/en/cli-reference.md index 905b0c4c..aea6c389 100644 --- a/pages/src/content/docs/en/cli-reference.md +++ b/pages/src/content/docs/en/cli-reference.md @@ -89,6 +89,7 @@ staged + unstaged + untracked changes in the current directory's repo. | `--commit ` | `-c` | — | Single commit to review (vs its parent). | | `--preview` | `-p` | `false` | Run the filter pipeline but skip the LLM. Prints the file list and exclusion reasons. | | `--resume ` | — | — | Resume from a previous compatible range or commit review session. | +| `--waive ` | — | — | Comma-separated repo-relative paths to waive: skipped but counted as covered. Requires `--resume`. | | `--format ` | `-f` | `text` | `text` (human-readable) or `json` (machine-readable comment array). | | `--audience ` | — | `human` | `human` streams progress lines; `agent` quiets stdout and prints only the final summary / JSON. | | `--background ` | `-b` | — | Optional requirement / business context injected into the plan + main prompts. | @@ -241,6 +242,73 @@ Top-level fields: | `warnings` | Optional. Present when one or more sub-agents failed; each entry describes the affected file and the error. | | `session_id` | Optional. Present on persisted review runs; pass this to `ocr review --resume ` when retrying compatible range or commit reviews. | | `resume` | Optional. Present on resumed runs with `resumed_from`, `reused_files`, `rerun_files`, `previous_model`, and `current_model`. | +| `manifest` | Optional. The versioned, immutable run manifest (see below). It is the machine-readable coverage contract; `status` above is the legacy, unchanged display field. | + +#### Run manifest + +The `manifest` object is the same value persisted as the final `run_manifest` +line of the session JSONL file, so JSON output and the stored session expose +identical coverage by construction. It is append-only and written exactly once +per run. + +```json +{ + "manifest": { + "schema_version": 1, + "session_id": "…", + "parent_session_id": "…", + "repo": { "remote_url": "https://…", "head_sha": "…", "branch": "main", "dir": "/repo" }, + "review_mode": "range", + "range": { "from": "main", "to": "feature", "from_sha": "…", "to_sha": "…" }, + "ocr_version": "1.2.3", + "provider": "anthropic", + "model": "claude-opus-4-6", + "concurrency": 8, + "config_hash": "…", + "rules_hash": "…", + "artifact_sha256": "…", + "files": { + "selected": ["a.go", "b.go"], + "completed": ["a.go"], + "reused": [], + "failed": ["b.go"], + "waived": [] + }, + "failures": [{ "path": "b.go", "class": "provider_error", "error": "…" }], + "state": "partial", + "started_at": "2026-07-15T08:00:00Z", + "completed_at": "2026-07-15T08:01:12Z", + "duration_ms": 72000 + } +} +``` + +| Field | Notes | +|---|---| +| `schema_version` | Manifest schema version (`1`). Bumped only on a breaking field change. | +| `state` | Terminal state: `complete`, `partial`, `failed`, or `skipped` (see the mapping below). | +| `files` | Coverage sets, each a sorted path list. `selected` is the coverage denominator — a superset of `completed ∪ reused ∪ failed ∪ waived`, with equality only for fully-covered runs. A cancelled or budget-truncated run leaves some selected files in no outcome bucket, which is what yields `partial`. | +| `range` | Review range refs and their resolved SHAs: `from`/`to`/`from_sha`/`to_sha` for range reviews, or `commit`/`commit_sha` for single-commit reviews. | +| `failures` | Per-failed-item typed class: `provider_error`, `timeout`, `cancelled`, `panic`, or `skipped_limit`, plus the error text. | +| `config_hash` | Hash over an allowlisted, non-secret config subset (provider protocol, model, base-URL host, language, timeout). Stable across API-key rotation. | +| `rules_hash` | Hash over the loaded rule layers (custom/project/global) plus the embedded system layer version. | +| `artifact_sha256` | Hash over the sorted per-file fingerprints — proves input identity without persisting source. | +| `parent_session_id` | Set on resumed runs; links to the session being resumed. A resume may switch provider/model — the new values are recorded here while reused items stay intact. | + +No token, auth header, or other credential ever appears in the manifest. + +##### State → exit-code mapping + +`manifest.state` is the machine contract; it is **independent of the process +exit code**, which stays `0` for any completed run (including `partial`). Use +`state` — not the exit code — to gate on partial coverage. + +| `state` | Meaning | Exit code | +|---|---|---| +| `complete` | Every selected file was completed, reused, or waived; no failures. | `0` | +| `partial` | Mixed outcome, or cancelled with at least one success. | `0` | +| `skipped` | Nothing was selected for review. | `0` | +| `failed` | Every selected file failed, or cancelled with no successes. | `0` (the run itself completed; `1` is reserved for fatal errors like bad flags or an unresolved LLM endpoint) | When no files were eligible for review, JSON mode emits a `skipped` envelope instead so callers can distinguish "no changes" from "no findings":