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

Squash of the test/docs slice, the adversarial-review fixes, and the
PR-review resolutions (Gemini/CodeAnt comments evaluated on merit).

Tests + docs (was 0ca7a40):
- 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 now 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 previous check
  compared the constant to itself). 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 corrected to the superset wording; commit-mode range
  fields documented.

Adversarial-review fixes (was 6afe1bf):
- viewer/store.go (HIGH): 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).

PR-review resolutions:
- session logging to stderr (gemini r3585811920): the [ocr session]
  messages went to stdout, which carries the --format json payload;
  all four sites (incl. two pre-existing) now use os.Stderr.
- remote-URL redaction (codeant r3585831274): redactRemoteURL (nee
  stripURLUserinfo) also strips query/fragment, so ?access_token=...
  can no longer reach the manifest; scp-style remotes still pass
  through (they fail url.Parse and carry no password). Verified E2E.
- content-based scan fingerprint (codeant r3585834011): scanFingerprint
  hashes mode\0path\0content (ScanItem.Content is already in memory),
  matching the diff path's content-sensitive fingerprint; the
  path-only ponytail debt is paid.
- scan panic recovery (codeant r3585834021): scan dispatch goroutines
  now recover, record the item as failed with class "panic", and let
  Finalize still write the manifest — mirrors the diff-review dispatch.
  Pre-existing gap, but newly load-bearing for the finalize-always
  contract. Panic row added to the scan parity table.
- superset comments (codeant r3585835296): the false equality
  invariant wording in SetSelected and recordSelectionAndArtifact now
  matches the manifest contract.
- 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.

Full go test ./... green (2034); -race green on session/agent/scan/
viewer/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/runmeta.go

@@ -108,31 +108,35 @@ func computeRulesHash(customRulePath, repoDir, ocrVersion string) string {
 	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

@@ -61,18 +61,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 +96,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 +108,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" {

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 {

internal/agent/manifest_test.go

@@ -58,6 +58,30 @@ func classCounts(m *session.RunManifest) map[string]int {
 	return out
 }
 
+// newManifestAgent builds an Agent wired to the stub routeClient with the
+// template/tool boilerplate shared by every test in this file. mutate (may be
+// nil) tweaks the Args before construction.
+func newManifestAgent(sess *session.SessionHistory, mainContent string, maxTokens int, mutate func(*Args)) *Agent {
+	args := Args{
+		LLMClient: routeClient{},
+		Model:     "test",
+		Session:   sess,
+		Tools:     tool.NewRegistry(),
+		Template: template.Template{
+			MaxTokens:           maxTokens,
+			MaxToolRequestTimes: 5,
+			MainTask:            template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: mainContent}}},
+		},
+		MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done", Description: "done"}}},
+	}
+	if mutate != nil {
+		mutate(&args)
+	}
+	a := New(args)
+	a.currentDate = "2026-07-15 08:00"
+	return a
+}
+
 func TestRunManifest_Scenarios(t *testing.T) {
 	bigTpl := strings.Repeat("word ", 120) + " {{diff}}"
 
@@ -137,19 +161,7 @@ func TestRunManifest_Scenarios(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange})
-			a := New(Args{
-				LLMClient: routeClient{},
-				Model:     "test",
-				Session:   sess,
-				Tools:     tool.NewRegistry(),
-				Template: template.Template{
-					MaxTokens:           tt.maxTokens,
-					MaxToolRequestTimes: 5,
-					MainTask:            template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: tt.mainContent}}},
-				},
-				MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done", Description: "done"}}},
-			})
-			a.currentDate = "2026-07-15 08:00"
+			a := newManifestAgent(sess, tt.mainContent, tt.maxTokens, nil)
 			a.diffs = tt.diffs
 
 			_, _ = a.dispatchSubtasks(context.Background())
@@ -189,22 +201,11 @@ func TestRunManifest_Scenarios(t *testing.T) {
 func TestWaiveCoverage(t *testing.T) {
 	build := func(waive []string) *session.RunManifest {
 		sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange})
-		a := New(Args{
-			LLMClient: routeClient{},
-			Model:     "test",
-			Session:   sess,
-			Tools:     tool.NewRegistry(),
-			Template: template.Template{
-				MaxTokens:           100000,
-				MaxToolRequestTimes: 5,
-				MainTask:            template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}},
-			},
-			MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done"}}},
+		a := newManifestAgent(sess, "{{diff}}", 100000, func(args *Args) {
 			// Resume must be non-nil for applyResume (and thus waive) to run.
-			Resume:     &session.ResumeState{SessionID: "prev", Items: map[string]session.ResumeItem{}},
-			WaivePaths: waive,
+			args.Resume = &session.ResumeState{SessionID: "prev", Items: map[string]session.ResumeItem{}}
+			args.WaivePaths = waive
 		})
-		a.currentDate = "d"
 		a.diffs = []model.Diff{
 			{NewPath: "ok.go", OldPath: "ok.go", Diff: "+ok", Insertions: 1},
 			{NewPath: "flaky.go", OldPath: "flaky.go", Diff: "+DO_FAIL", Insertions: 1},
@@ -242,19 +243,7 @@ func TestWaiveCoverage(t *testing.T) {
 func TestArtifactChecksumStableAndOrderIndependent(t *testing.T) {
 	mk := func(order []model.Diff) string {
 		sess := session.New(t.TempDir(), "main", "test", session.SessionOptions{ReviewMode: session.ReviewModeRange})
-		a := New(Args{
-			LLMClient: routeClient{},
-			Model:     "test",
-			Session:   sess,
-			Tools:     tool.NewRegistry(),
-			Template: template.Template{
-				MaxTokens:           100000,
-				MaxToolRequestTimes: 5,
-				MainTask:            template.LlmConversation{Messages: []template.ChatMessage{{Role: "user", Content: "{{diff}}"}}},
-			},
-			MainToolDefs: []llm.ToolDef{{Type: "function", Function: llm.FunctionDef{Name: "task_done"}}},
-		})
-		a.currentDate = "d"
+		a := newManifestAgent(sess, "{{diff}}", 100000, nil)
 		a.diffs = order
 		a.recordSelectionAndArtifact()
 		sess.Finalize()

internal/scan/agent.go

@@ -6,7 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"sort"
+	"runtime/debug"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -459,13 +459,11 @@ func (a *Agent) dispatchSubtasks(ctx context.Context) ([]model.LlmComment, error
 }
 
 // scanFingerprint derives a stable per-item fingerprint for the manifest.
-//
-// ponytail: content-hash if scan grows resume. Scan has no diff and no resume
-// today, so a path-based fingerprint is enough to identify the item and to
-// compute the artifact checksum; upgrade to hashing it.Content when resume
-// lands on the scan path.
-func scanFingerprint(path string) string {
-	sum := sha256.Sum256([]byte(session.ReviewModeFullScan + "\x00" + path))
+// Content is hashed alongside the path (the item already carries the full
+// file in memory), so the artifact checksum changes whenever the scanned
+// inputs do — same guarantee the diff path gets from hashing d.Diff.
+func scanFingerprint(it model.ScanItem) string {
+	sum := sha256.Sum256([]byte(session.ReviewModeFullScan + "\x00" + it.Path + "\x00" + it.Content))
 	return fmt.Sprintf("%x", sum)
 }
 
@@ -477,12 +475,10 @@ func (a *Agent) recordSelectionAndArtifact() {
 	fps := make([]string, 0, len(a.items))
 	for i := range a.items {
 		selected = append(selected, a.items[i].Path)
-		fps = append(fps, scanFingerprint(a.items[i].Path))
+		fps = append(fps, scanFingerprint(a.items[i]))
 	}
 	a.session.SetSelected(selected)
-	sort.Strings(fps)
-	sum := sha256.Sum256([]byte(strings.Join(fps, "\x00")))
-	a.session.SetArtifactChecksum(fmt.Sprintf("%x", sum))
+	a.session.SetArtifactChecksum(session.ArtifactChecksum(fps))
 }
 
 // resolveBatchStrategy reads the strategy from the scan template, defaulting
@@ -544,6 +540,21 @@ func (a *Agent) dispatchBatch(ctx context.Context, batchIdx int, batch []model.S
 		go func(it model.ScanItem) {
 			defer wg.Done()
 			defer func() { <-sem }()
+			// A panic while scanning one file must be isolated exactly like an
+			// error return: counted in subtaskFailed and recorded as a failed
+			// item with class "panic", so other files still complete and the
+			// manifest is still finalized. Mirrors the diff-review dispatch.
+			defer func() {
+				if r := recover(); r != nil {
+					atomic.AddInt64(&a.subtaskFailed, 1)
+					a.session.RecordReviewItemFailed(it.Path, it.Path, it.Path, scanFingerprint(it), session.FailurePanic, fmt.Sprintf("panic: %v", r))
+					fmt.Fprintf(stdout.Writer(), "[ocr] Scan subtask panic for %s (batch #%d): %v\n%s\n", it.Path, batchIdx, r, debug.Stack())
+					telemetry.ErrorEvent(ctx, "scan.subtask.panic", fmt.Errorf("panic: %v", r),
+						telemetry.AnyToAttr("file.path", it.Path),
+						telemetry.AnyToAttr("batch.index", batchIdx))
+					a.recordWarning("scan_subtask_error", it.Path, fmt.Sprintf("panic: %v", r))
+				}
+			}()
 
 			var fileCtx context.Context
 			var cancel context.CancelFunc
@@ -597,7 +608,7 @@ func (a *Agent) executeSubtask(ctx context.Context, it model.ScanItem) error {
 
 	messages := a.renderMessages(it, rule, planGuidance)
 
-	fingerprint := scanFingerprint(it.Path)
+	fingerprint := scanFingerprint(it)
 
 	tokenCount := llmloop.CountMessagesTokens(messages)
 	maxAllowed := a.args.Template.MaxTokens