diff --git a/cmd/opencodereview/emit_run_result_test.go b/cmd/opencodereview/emit_run_result_test.go index f153fe22..6c0913dd 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,91 @@ 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 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) { + tests := []struct { + name string + filesReviewed int64 + warnings []agent.AgentWarning + manifestState string + wantStatus string + wantManifestState string + }{ + {"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: 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) + } + }) + 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 != tt.wantManifestState { + t.Errorf("manifest.state should be present (%q) regardless of legacy status, got %+v", tt.wantManifestState, out.Manifest) + } + }) + } +} + func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) { ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"} got := captureStdout(t, func() { 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/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/review_cmd.go b/cmd/opencodereview/review_cmd.go index 8cf53275..8ae03733 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, cc.Resolver, 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,8 @@ func runReview(args []string) error { Background: opts.background, GitRunner: cc.GitRunner, Resume: resumeState, + RunMeta: runMeta, + WaivePaths: splitPaths(opts.waive), }) // 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..6bf00231 --- /dev/null +++ b/cmd/opencodereview/runmeta.go @@ -0,0 +1,171 @@ +package main + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "net/url" + "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" +) + +// 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 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(resolver, 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 + 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 string, layer *rules.ProjectRule) { + if layer == nil { + return + } + data, err := json.Marshal(layer) + if err != nil { + return + } + h.Write([]byte(label)) + h.Write([]byte{0}) + h.Write(data) + } + 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/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 = redactRemoteURL(strings.TrimSpace(string(out))) + } + if out, err := runGitCmdStdout(repoDir, "rev-parse", "HEAD"); err == nil { + headSHA = strings.TrimSpace(string(out)) + } + return remoteURL, headSHA +} + +// 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 { + return raw + } + u.User = nil + u.RawQuery = "" + u.Fragment = "" + u.RawFragment = "" + 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..cf793aa2 --- /dev/null +++ b/cmd/opencodereview/runmeta_test.go @@ -0,0 +1,186 @@ +package main + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/open-code-review/open-code-review/internal/config/rules" + "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 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 := redactRemoteURL(tt.in); got != tt.want { + t.Errorf("redactRemoteURL(%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?access_token=querysecret") + 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:") || strings.Contains(remoteURL, "querysecret") { + 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) { + t.Setenv("HOME", t.TempDir()) // isolate the global ~/.opencodereview layer + dir := t.TempDir() + 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) + } + 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(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 5d15f44d..554d62b7 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, cc.Resolver, 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/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/cmd/opencodereview/shared.go b/cmd/opencodereview/shared.go index c99c1b2c..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" @@ -134,6 +135,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 +181,7 @@ func loadLLMRuntime(tpl *template.Template, toolConfigPath, modelOverride string MainToolDefs: mainToolDefs, Collector: tool.NewCommentCollector(), AppCfg: appCfg, + Endpoint: ep, }, nil } @@ -255,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 { @@ -287,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 @@ -310,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 a50dead8..1a466439 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "encoding/json" + "errors" "fmt" "runtime/debug" "strings" @@ -117,6 +118,17 @@ 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 + + // 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 @@ -163,6 +175,7 @@ func New(args Args) *Agent { DiffTo: args.To, DiffCommit: args.Commit, ResumedFrom: resumedFromSession(args.Resume), + RunMeta: args.RunMeta, }) } a := &Agent{ @@ -231,6 +244,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 } @@ -240,6 +256,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 { @@ -359,6 +384,11 @@ 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 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) var wg sync.WaitGroup @@ -393,7 +423,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 +443,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 +452,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 } @@ -456,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 @@ -465,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) @@ -511,6 +552,24 @@ 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) + a.session.SetArtifactChecksum(session.ArtifactChecksum(fps)) +} + func resumedFromSession(resume *session.ResumeState) string { if resume == nil { return "" @@ -797,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 new file mode 100644 index 00000000..ac20536b --- /dev/null +++ b/internal/agent/manifest_test.go @@ -0,0 +1,295 @@ +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 +} + +// 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 + wantNoArtifact bool + }{ + { + 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}, + }, + { + // 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 := newManifestAgent(sess, tt.mainContent, tt.maxTokens, nil) + 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 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) + 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) + } + } + }) + } +} + +// 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 := newManifestAgent(sess, "{{diff}}", 100000, func(args *Args) { + // Resume must be non-nil for applyResume (and thus waive) to run. + args.Resume = &session.ResumeState{SessionID: "prev", Items: map[string]session.ResumeItem{}} + args.WaivePaths = waive + }) + 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) { + mk := func(order []model.Diff) string { + sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange}) + a := newManifestAgent(sess, "{{diff}}", 100000, nil) + 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") + } +} 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 aa0b8342..da3c1c7b 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" + "runtime/debug" "strings" "sync" "sync/atomic" @@ -67,6 +70,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 +120,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{ @@ -154,6 +161,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 { @@ -258,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 } @@ -327,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 } @@ -389,6 +414,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() @@ -435,6 +464,29 @@ 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. +// 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) +} + +// 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])) + } + a.session.SetSelected(selected) + a.session.SetArtifactChecksum(session.ArtifactChecksum(fps)) +} + // resolveBatchStrategy reads the strategy from the scan template, defaulting // to BatchNone for unrecognized / empty values. func (a *Agent) resolveBatchStrategy() BatchStrategy { @@ -494,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 @@ -547,6 +614,8 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error { messages := a.renderMessages(it, rule, planGuidance) + fingerprint := scanFingerprint(it) + tokenCount := llmloop.CountMessagesTokens(messages) maxAllowed := a.args.Template.MaxTokens tokenLimit := maxAllowed * 4 / 5 @@ -554,6 +623,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), @@ -562,7 +632,20 @@ 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 + } + // 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 } // 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..a71c305e --- /dev/null +++ b/internal/scan/manifest_test.go @@ -0,0 +1,230 @@ +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_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"): + 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 +} + +// 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 + 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: "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, + 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) + } + } + }) + } +} + +// 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 c64ac88e..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" @@ -49,6 +50,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 +123,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,11 +160,17 @@ 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) 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) @@ -161,6 +207,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 +224,39 @@ 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 + } + if filePath == "" { + filePath = newPath + } + 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, 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 } @@ -193,9 +265,64 @@ func (sh *SessionHistory) RecordReviewItemFailed(filePath, oldPath, newPath, fin } if filePath != "" { sh.GetOrCreateFileSession(filePath) + sh.trackCoverage("waived", filePath) } if p := sh.persist; p != nil { - p.WriteReviewItemFailed(filePath, oldPath, newPath, fingerprint, errorMsg) + p.WriteReviewItemWaived(filePath, oldPath, newPath, fingerprint) + } +} + +// SetSelected records the full set of reviewable file paths for this run. It +// 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 + } + 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{}{} } } @@ -203,6 +330,13 @@ func (sh *SessionHistory) RecordReviewItemFailed(filePath, oldPath, newPath, fin // 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) @@ -211,11 +345,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); flushAndClose then 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..3a54c5c1 --- /dev/null +++ b/internal/session/manifest.go @@ -0,0 +1,170 @@ +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. +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 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"` + 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 { + // 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 + } + 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..d6a85259 --- /dev/null +++ b/internal/session/manifest_test.go @@ -0,0 +1,126 @@ +package session + +import ( + "bufio" + "encoding/json" + "os" + "sort" + "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}, + {"cancelled with only waives", 3, 0, 0, 0, 2, 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 +} + +// 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.RecordReviewItemFailed("b.go", "b.go", "b.go", "fp-b", FailureProviderError, "boom") + sh.Finalize() + + blob, err := json.Marshal(sh.Manifest()) + if err != nil { + t.Fatal(err) + } + var rec map[string]any + if err := json.Unmarshal(blob, &rec); err != nil { + t.Fatal(err) + } + if v, ok := rec["schema_version"].(float64); !ok || v != 1 { + t.Errorf("serialized schema_version = %v, want 1", rec["schema_version"]) + } + // 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 _, 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 464da1b6..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) @@ -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,7 +324,44 @@ func (jw *jsonlWriter) WriteToolCall(filePath string, taskType TaskType, toolNam return uuid } -// WriteSessionEnd writes the final session_end summary record and closes the file. +// 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.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.Fprintf(os.Stderr, "[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. 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() @@ -334,9 +383,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) 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 } 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":