chore(tests): acceptance matrix + provider-transition + secret guard; docs

Tests + docs:
- acceptance_test.go: end-to-end matrix over the terminal-state scenarios
  (complete/partial/failed/skipped/cancelled/waive, plus a cancelled
  mid-run row where selected is a strict superset of the outcome sets);
  provider-transition resume case starts from a PARTIAL run and asserts
  the resume flips it to complete with parent_session_id intact and an
  unchanged artifact checksum (input identity retained);
  TestManifestNeverLeaksSecrets is the canonical secret guard.
- manifest_test.go: TestManifestSchemaLock pins the literal schema
  version (1) and the top-level JSON key set. The standalone Finalize
  test was folded into the acceptance matrix (failure-class + artifact
  columns).
- emit_run_result_test.go: no-files manifest case folded into the
  legacy-status regression table.
- agent/manifest_test.go: newManifestAgent helper replaces three copies
  of the Args literal.
- docs: run-manifest schema, state->exit-code mapping, --waive flag;
  files invariant stated with the superset wording; commit-mode range
  fields documented.

Hardening:
- viewer/store.go: peekSession read only the last JSONL line for
  session_end; run_manifest is now the last line, so the session-list
  fast path lost duration/files/failures. Checks the last two lines;
  regression test fails on the old code.
- manifest.go: cancelled run whose only covered items are waives is
  partial, not failed; ManifestFiles doc corrected (selected is a
  superset); Finalize made idempotent (written-once guard).
- session logging to stderr: the [ocr session] messages went to stdout,
  which carries the --format json payload; all four sites now use
  os.Stderr.
- redactRemoteURL (nee stripURLUserinfo) also strips query/fragment, so
  ?access_token=... can never reach the manifest; scp-style remotes
  still pass through (they fail url.Parse and carry no password).
  Verified E2E.
- scanFingerprint hashes mode\0path\0content (ScanItem.Content is
  already in memory), matching the diff path's content-sensitive
  fingerprint.
- scan dispatch goroutines now recover from panics, record the item as
  failed with class "panic", and let Finalize still write the manifest,
  mirroring the diff-review dispatch. Panic row added to the scan
  parity table.
- session.ArtifactChecksum: the sort/join/sha256 tail duplicated in the
  agent and scan recordSelectionAndArtifact now lives in one place so
  the fingerprint-join convention cannot diverge.
- computeRulesHash hashes the resolver's resolved rule layers (new
  rules.UserLayers accessor) instead of re-reading the top-level
  rule.json bytes: rule entries that reference files (e.g. team.md)
  are expanded to content by rules.NewResolver, so editing a referenced
  file now changes rules_hash even though rule.json's bytes do not.
  Test locks referenced-file edit -> hash change.
- scan executeSubtask drains the async CommentWorkerPool before reading
  CommentsForPath and persisting review_item_done; without the per-file
  Await the checkpoint could persist an incomplete comment set while
  async code_comment workers were still running (the diff path already
  had this Await).
- filterLargeDiffs/filterLargeScans record each dropped oversized file
  as failed with class skipped_limit (plus a warning), so the
  manifest's selected denominator includes pre-filtered files: a
  partially filtered run is partial, an all-filtered run is failed
  with per-file failures rather than an empty "skipped". Two agent
  table rows + a scan filter test lock this.
- persist.go/history.go: corrected stale close-ownership comments
  (WriteSessionEnd no longer closes the file; flushAndClose does).

Full go test ./... green (2037); -race green on session/agent/scan/
llmloop/cmd; make check clean. E2E: failing review persists
... -> session_end -> run_manifest with state=failed and
class=provider_error; no-diff review emits valid skipped JSON with the
manifest; a remote with userinfo+query credentials is fully redacted.

Author: chethanuk

cmd/opencodereview/emit_run_result_test.go

@@ -325,22 +325,30 @@ func TestEmitRunResult_JSONIncludesManifest(t *testing.T) {
 }
 
 // TestLegacyStatusValuesUnchanged locks the legacy `status` field to its
-// pre-manifest values across the warning/error paths. manifest.state is the
-// new machine contract; status must not drift.
+// pre-manifest values across the success/warning/error/no-files paths, and
+// asserts every path still carries the manifest. manifest.state is the new
+// machine contract; status must not drift.
 func TestLegacyStatusValuesUnchanged(t *testing.T) {
-	man := &session.RunManifest{State: session.StatePartial}
 	tests := []struct {
-		name       string
-		warnings   []agent.AgentWarning
-		wantStatus string
+		name              string
+		filesReviewed     int64
+		warnings          []agent.AgentWarning
+		manifestState     string
+		wantStatus        string
+		wantManifestState string
 	}{
-		{"clean success", nil, "success"},
-		{"subtask errors", []agent.AgentWarning{{Type: "subtask_error", File: "a.go", Message: "boom"}}, "completed_with_errors"},
-		{"non-error warnings", []agent.AgentWarning{{Type: "token_threshold_exceeded", Message: "big"}}, "completed_with_warnings"},
+		{"clean success", 2, nil, session.StatePartial, "success", session.StatePartial},
+		{"subtask errors", 2, []agent.AgentWarning{{Type: "subtask_error", File: "a.go", Message: "boom"}}, session.StatePartial, "completed_with_errors", session.StatePartial},
+		{"non-error warnings", 2, []agent.AgentWarning{{Type: "token_threshold_exceeded", Message: "big"}}, session.StatePartial, "completed_with_warnings", session.StatePartial},
+		{"no files skipped", 0, nil, session.StateSkipped, "skipped", session.StateSkipped},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			ag := &mockResultProvider{filesReviewed: 2, warnings: tt.warnings, manifest: man}
+			ag := &mockResultProvider{
+				filesReviewed: tt.filesReviewed,
+				warnings:      tt.warnings,
+				manifest:      &session.RunManifest{State: tt.manifestState},
+			}
 			got := captureStdout(t, func() {
 				if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil {
 					t.Fatalf("err: %v", err)
@@ -353,35 +361,13 @@ func TestLegacyStatusValuesUnchanged(t *testing.T) {
 			if out.Status != tt.wantStatus {
 				t.Errorf("status = %q, want %q", out.Status, tt.wantStatus)
 			}
-			if out.Manifest == nil || out.Manifest.State != session.StatePartial {
-				t.Errorf("manifest.state should be present and partial regardless of legacy status")
+			if out.Manifest == nil || out.Manifest.State != tt.wantManifestState {
+				t.Errorf("manifest.state should be present (%q) regardless of legacy status, got %+v", tt.wantManifestState, out.Manifest)
 			}
 		})
 	}
 }
 
-func TestEmitRunResult_JSONNoFilesIncludesManifest(t *testing.T) {
-	ag := &mockResultProvider{
-		filesReviewed: 0,
-		manifest:      &session.RunManifest{State: session.StateSkipped},
-	}
-	got := captureStdout(t, func() {
-		if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil {
-			t.Fatalf("err: %v", err)
-		}
-	})
-	var out jsonOutput
-	if err := json.Unmarshal([]byte(got), &out); err != nil {
-		t.Fatalf("unmarshal: %v", err)
-	}
-	if out.Status != "skipped" {
-		t.Errorf("status = %q, want skipped", out.Status)
-	}
-	if out.Manifest == nil || out.Manifest.State != session.StateSkipped {
-		t.Errorf("no-files path should still carry the skipped manifest, got %+v", out.Manifest)
-	}
-}
-
 func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) {
 	ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"}
 	got := captureStdout(t, func() {

cmd/opencodereview/review_cmd.go

@@ -102,7 +102,7 @@ func runReview(args []string) error {
 	if rt.AppCfg != nil {
 		lang = rt.AppCfg.Language
 	}
-	runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, opts.rulePath, opts.concurrency,
+	runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, cc.Resolver, opts.concurrency,
 		resolveRange(cc.RepoDir, opts.from, opts.to, opts.commit))
 
 	ag := agent.New(agent.Args{

cmd/opencodereview/runmeta.go

@@ -5,10 +5,9 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"net/url"
-	"os"
-	"path/filepath"
 	"strings"
 
+	"github.com/open-code-review/open-code-review/internal/config/rules"
 	"github.com/open-code-review/open-code-review/internal/llm"
 	"github.com/open-code-review/open-code-review/internal/session"
 )
@@ -29,14 +28,14 @@ type configHashInput struct {
 // buildRunMeta assembles the manifest metadata the command layer owns:
 // tool version, provider, config/rules hashes, repo identity, and (for the
 // range/commit paths) the resolved range SHAs.
-func buildRunMeta(ep llm.ResolvedEndpoint, language, ocrVersion, repoDir, customRulePath string, concurrency int, rng resolvedRange) session.RunMeta {
+func buildRunMeta(ep llm.ResolvedEndpoint, language, ocrVersion, repoDir string, resolver rules.Resolver, concurrency int, rng resolvedRange) session.RunMeta {
 	remoteURL, headSHA := repoIdentity(repoDir)
 	return session.RunMeta{
 		OCRVersion:     ocrVersion,
 		Provider:       ep.Protocol,
 		Concurrency:    concurrency,
 		ConfigHash:     computeConfigHash(ep, language),
-		RulesHash:      computeRulesHash(customRulePath, repoDir, ocrVersion),
+		RulesHash:      computeRulesHash(resolver, ocrVersion),
 		RepoRemoteURL:  remoteURL,
 		RepoHeadSHA:    headSHA,
 		RangeFromSHA:   rng.fromSHA,
@@ -76,63 +75,69 @@ func baseURLHost(raw string) string {
 	return u.Host
 }
 
-// computeRulesHash hashes each loaded rule layer as (label + \0 + raw bytes),
-// in a stable order. Missing layers contribute nothing. The embedded system
-// layer has no file, so it contributes label + tool version (it only changes
-// when the binary does).
-//
-// ponytail: reads the rule files by their known paths rather than plumbing raw
-// bytes out of rules.NewResolver — same inputs, no new API surface. If rule
-// loading grows more layers, mirror them here.
-func computeRulesHash(customRulePath, repoDir, ocrVersion string) string {
+// computeRulesHash hashes each loaded rule layer as (label + \0 + canonical
+// JSON of the resolved layer), in a stable order. "Resolved" means rule
+// entries that reference files (e.g. a rule.json entry pointing at team.md)
+// have already been expanded to that file's content by rules.NewResolver, so
+// the hash tracks the effective runtime rules — editing a referenced file
+// changes the hash even though rule.json's bytes do not. Missing layers
+// contribute nothing. The embedded system layer has no file, so it
+// contributes label + tool version (it only changes when the binary does).
+func computeRulesHash(resolver rules.Resolver, ocrVersion string) string {
 	h := sha256.New()
-	add := func(label, path string) {
-		data, err := os.ReadFile(path)
+	add := func(label string, layer *rules.ProjectRule) {
+		if layer == nil {
+			return
+		}
+		data, err := json.Marshal(layer)
 		if err != nil {
 			return
 		}
 		h.Write([]byte(label))
 		h.Write([]byte{0})
 		h.Write(data)
 	}
-	if customRulePath != "" {
-		add("custom", customRulePath)
-	}
-	if repoDir != "" {
-		add("project", filepath.Join(repoDir, ".opencodereview", "rule.json"))
-	}
-	if home, err := os.UserHomeDir(); err == nil {
-		add("global", filepath.Join(home, ".opencodereview", "rule.json"))
+	if ul, ok := resolver.(interface {
+		UserLayers() (custom, project, global *rules.ProjectRule)
+	}); ok {
+		custom, project, global := ul.UserLayers()
+		add("custom", custom)
+		add("project", project)
+		add("global", global)
 	}
 	h.Write([]byte("system\x00" + ocrVersion))
 	return hex.EncodeToString(h.Sum(nil))
 }
 
-// repoIdentity returns the origin remote URL (with userinfo stripped) and the
-// HEAD commit SHA. Both tolerate git failure by returning "".
+// repoIdentity returns the origin remote URL (with userinfo/query redacted)
+// and the HEAD commit SHA. Both tolerate git failure by returning "".
 func repoIdentity(repoDir string) (remoteURL, headSHA string) {
 	if repoDir == "" {
 		return "", ""
 	}
 	if out, err := runGitCmdStdout(repoDir, "remote", "get-url", "origin"); err == nil {
-		remoteURL = stripURLUserinfo(strings.TrimSpace(string(out)))
+		remoteURL = redactRemoteURL(strings.TrimSpace(string(out)))
 	}
 	if out, err := runGitCmdStdout(repoDir, "rev-parse", "HEAD"); err == nil {
 		headSHA = strings.TrimSpace(string(out))
 	}
 	return remoteURL, headSHA
 }
 
-// stripURLUserinfo removes a user[:password]@ prefix from a standard URL so a
-// token embedded in an https remote never lands in the manifest. scp-style
-// git remotes (git@host:path) carry no password and parse as opaque, so they
-// pass through unchanged.
-func stripURLUserinfo(raw string) string {
+// redactRemoteURL removes the user[:password]@ prefix and any query/fragment
+// from a standard URL so a token embedded in an https remote (userinfo or
+// ?access_token=…) never lands in the manifest, matching baseURLHost's
+// redaction of the config-hash input. scp-style git remotes (git@host:path)
+// fail url.Parse, carry no password, and pass through unchanged.
+func redactRemoteURL(raw string) string {
 	u, err := url.Parse(raw)
-	if err != nil || u.User == nil {
+	if err != nil {
 		return raw
 	}
 	u.User = nil
+	u.RawQuery = ""
+	u.Fragment = ""
+	u.RawFragment = ""
 	return u.String()
 }
 

cmd/opencodereview/runmeta_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/open-code-review/open-code-review/internal/config/rules"
 	"github.com/open-code-review/open-code-review/internal/llm"
 )
 
@@ -61,18 +62,20 @@ func TestConfigHashRedaction(t *testing.T) {
 	}
 }
 
-func TestStripURLUserinfo(t *testing.T) {
+func TestRedactRemoteURL(t *testing.T) {
 	tests := []struct {
 		in, want string
 	}{
 		{"https://user:token@github.com/acme/repo.git", "https://github.com/acme/repo.git"},
 		{"https://github.com/acme/repo.git", "https://github.com/acme/repo.git"},
+		{"https://github.com/acme/repo.git?access_token=SECRET", "https://github.com/acme/repo.git"},
+		{"https://user:token@github.com/acme/repo.git?access_token=SECRET#frag", "https://github.com/acme/repo.git"},
 		{"git@github.com:acme/repo.git", "git@github.com:acme/repo.git"}, // scp-style: no password, unchanged
 		{"", ""},
 	}
 	for _, tt := range tests {
-		if got := stripURLUserinfo(tt.in); got != tt.want {
-			t.Errorf("stripURLUserinfo(%q) = %q, want %q", tt.in, got, tt.want)
+		if got := redactRemoteURL(tt.in); got != tt.want {
+			t.Errorf("redactRemoteURL(%q) = %q, want %q", tt.in, got, tt.want)
 		}
 	}
 }
@@ -94,7 +97,7 @@ func gitInitRepo(t *testing.T) string {
 		}
 	}
 	run("init", "-q")
-	run("remote", "add", "origin", "https://alice:secrettoken@example.com/acme/repo.git")
+	run("remote", "add", "origin", "https://alice:secrettoken@example.com/acme/repo.git?access_token=querysecret")
 	if err := os.WriteFile(filepath.Join(dir, "f.txt"), []byte("hi"), 0o644); err != nil {
 		t.Fatal(err)
 	}
@@ -106,7 +109,7 @@ func gitInitRepo(t *testing.T) string {
 func TestRepoIdentityStripsUserinfo(t *testing.T) {
 	dir := gitInitRepo(t)
 	remoteURL, headSHA := repoIdentity(dir)
-	if strings.Contains(remoteURL, "secrettoken") || strings.Contains(remoteURL, "alice:") {
+	if strings.Contains(remoteURL, "secrettoken") || strings.Contains(remoteURL, "alice:") || strings.Contains(remoteURL, "querysecret") {
 		t.Errorf("repo remote URL leaks credentials: %q", remoteURL)
 	}
 	if remoteURL != "https://example.com/acme/repo.git" {
@@ -138,22 +141,46 @@ func TestResolveRangeReturnsSHAs(t *testing.T) {
 }
 
 func TestComputeRulesHashChangesWithLayer(t *testing.T) {
+	t.Setenv("HOME", t.TempDir()) // isolate the global ~/.opencodereview layer
 	dir := t.TempDir()
-	h1 := computeRulesHash("", dir, "1.0.0")
-	// Adding a project rule file changes the hash.
+	newResolver := func() rules.Resolver {
+		t.Helper()
+		r, _, err := rules.NewResolver(dir, "")
+		if err != nil {
+			t.Fatal(err)
+		}
+		return r
+	}
+	writeFile := func(path, content string) {
+		t.Helper()
+		if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+	h1 := computeRulesHash(newResolver(), "1.0.0")
+
+	// Adding a project rule layer changes the hash.
 	rulesDir := filepath.Join(dir, ".opencodereview")
 	if err := os.MkdirAll(rulesDir, 0o755); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.WriteFile(filepath.Join(rulesDir, "rule.json"), []byte(`{"rules":[]}`), 0o644); err != nil {
-		t.Fatal(err)
-	}
-	h2 := computeRulesHash("", dir, "1.0.0")
+	writeFile(filepath.Join(rulesDir, "rule.json"), `{"rules":[{"path":"**/*.go","rule":"team.md"}]}`)
+	writeFile(filepath.Join(dir, "team.md"), "rule text A")
+	h2 := computeRulesHash(newResolver(), "1.0.0")
 	if h1 == h2 {
 		t.Error("rules hash should change when a rule layer appears")
 	}
+
+	// Editing the referenced rule file changes the effective rules, so the
+	// hash must change even though rule.json's own bytes are unchanged.
+	writeFile(filepath.Join(dir, "team.md"), "rule text B")
+	h3 := computeRulesHash(newResolver(), "1.0.0")
+	if h3 == h2 {
+		t.Error("rules hash should change when a referenced rule file changes")
+	}
+
 	// Version bump changes the embedded-system contribution.
-	if computeRulesHash("", dir, "2.0.0") == h2 {
+	if computeRulesHash(newResolver(), "2.0.0") == h3 {
 		t.Error("rules hash should change when tool version changes")
 	}
 }

cmd/opencodereview/scan_cmd.go

@@ -187,7 +187,7 @@ func runScan(args []string) error {
 		lang = rt.AppCfg.Language
 	}
 	// Scan has no diff range; resolvedRange stays empty.
-	runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, opts.rulePath, opts.concurrency, resolvedRange{})
+	runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, cc.Resolver, opts.concurrency, resolvedRange{})
 
 	ag := scan.NewAgent(scan.Args{
 		RepoDir:               cc.RepoDir,

internal/agent/agent.go

@@ -7,7 +7,6 @@ import (
 	"errors"
 	"fmt"
 	"runtime/debug"
-	"sort"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -386,8 +385,9 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error
 		return nil, fmt.Errorf("all diffs filtered out by token size")
 	}
 	// Establish the coverage denominator and input-identity checksum before
-	// resume/dispatch so the manifest invariant selected == completed + reused
-	// + failed + waived holds.
+	// resume/dispatch so every dispatched outcome lands in a bucket selected
+	// already covers (selected is a superset of completed ∪ reused ∪ failed
+	// ∪ waived; equality only for fully-covered runs).
 	a.recordSelectionAndArtifact()
 	toDispatch := a.applyResume(a.diffs)
 
@@ -567,9 +567,7 @@ func (a *Agent) recordSelectionAndArtifact() {
 		fps = append(fps, reviewItemFingerprint(mode, d))
 	}
 	a.session.SetSelected(selected)
-	sort.Strings(fps)
-	sum := sha256.Sum256([]byte(strings.Join(fps, "\x00")))
-	a.session.SetArtifactChecksum(fmt.Sprintf("%x", sum))
+	a.session.SetArtifactChecksum(session.ArtifactChecksum(fps))
 }
 
 func resumedFromSession(resume *session.ResumeState) string {
@@ -858,8 +856,14 @@ func (a *Agent) filterLargeDiffs(diffs []model.Diff) []model.Diff {
 	for _, d := range diffs {
 		tokens := llm.CountTokens(d.Diff)
 		if tokens > limit {
-			fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s (~%d tokens exceeds 80%% of max_tokens(%d))\n",
-				d.NewPath, tokens, a.args.Template.MaxTokens)
+			msg := fmt.Sprintf("diff tokens (~%d) exceed 80%% of max_tokens(%d)", tokens, a.args.Template.MaxTokens)
+			fmt.Fprintf(stdout.Writer(), "[ocr] Skipping %s (%s)\n", d.NewPath, msg)
+			// Record the drop so the manifest's coverage denominator still
+			// counts this file (as failed/skipped_limit) instead of silently
+			// deselecting it — mirrors the prompt-level threshold path.
+			a.recordWarning("token_threshold_exceeded", d.NewPath, msg)
+			a.session.RecordReviewItemFailed(d.NewPath, d.OldPath, d.NewPath,
+				reviewItemFingerprint(a.reviewMode(), d), session.FailureSkippedLimit, msg)
 			skipped++
 			continue
 		}