feat(manifest): run manifest coverage contract for review (#367)

[Gongyl01]

Summary

Closes #367.

Adds a versioned, immutable RunManifest to ocr review that records per-item coverage (selected / completed / reused / failed / waived) and an authoritative terminal state (complete / partial / failed / skipped). On successful persistence, CLI JSON and session_end.run_manifest expose the same frozen manifest, so a partial review can no longer be mistaken for ordinary success or a complete no-findings result.

Built on top of PR #306's resumable sessions; this is a narrow output-contract follow-up, not a parallel persistence system.

• New internal/session/manifest.go: RunManifest value object (schema ocr.run-manifest/v1), ManifestBuilder (single state table, unified transition entry, RegisterSelected / MarkCompleted / MarkReused / MarkFailed / MarkWaived), SealSelected (closes the selected denominator before dispatch), Finalize(elapsed) (RunManifest, error) with hard validation, structured RunFailure + RunFailureClass enum, and sanitizeReason redaction floor.
• internal/agent/agent.go: a pre-dispatch pass (registerCoverage) registers all post-filter, non-deleted items and seals them before resume reuse or goroutine dispatch; markCompleted / markReused / markFailed record per-item outcomes. The live --max-tokens-budget gate records FailureBudget as the pending-item failure cause, so only undispatched/pending items are swept to failed(budget); BudgetExceeded() continues to feed the summary and warning. SetRunFailure is reserved for run-level stop causes at the trigger source (input failure on loadDiffs, internal failure on registerCoverage). initManifest initializes requested input and execution metadata; resolved input and repository identity are captured from the actual run and attached before finalization.
• internal/diff/git.go: ResolveInput freezes resolved base/head/exact range per input mode, including first-parent merge input to match GetDiff --diff-merges=first-parent; RemoteIdentity / canonicalRemote produce a credential-free repository identity by canonicalizing the origin URL to host[:port]/path (dropping userinfo/query/fragment) and SHA-256ing the result; local remotes omit identity.
• internal/session/history.go: NewManifestBuilder mounts the builder on SessionHistory; Finalize now returns error (was void) and uses sync.Once so session_end is written exactly once while replaying the first error on every call.
• internal/session/persist.go: WriteSessionEnd writes session_end as the last physical JSONL record, embeds the frozen manifest under run_manifest, and surfaces write/flush/close errors.
• cmd/opencodereview: review JSON exposes manifest; top-level status uses the four terminal states (scan stays legacy); aggregate-budget reporting remains available through summary.budget_exceeded; failed runs include the sanitized run-failure cause or failed/selected counts; text output no longer shows Looks good to me. for partial/failed; session list/show and the viewer prefer a valid session_end.run_manifest and otherwise display legacy/unknown state without faking complete.

Design (key invariants)

• Single source of truth. The terminal state is computed once in Finalize. On successful persistence, CLI JSON and session_end.run_manifest serialize the same frozen manifest value. JSON output no longer derives review status from warnings.
• Single outcome entry. All per-item outcomes go through one transition function. Registration after seal, transitions after freeze, unknown item IDs, invalid failure classes, empty waiver reasons, and conflicting transitions return errors. Repeating the same outcome is idempotent; re-marking a failed item with a different classification is rejected.
• Seal before dispatch. SealSelected closes the selected denominator after the pre-dispatch pass; resume-reused and to-be-dispatched items enter the same frozen set, so selected = completed ∪ reused ∪ failed ∪ waived always holds.
• Explicit stop cause. run_failure is recorded at the trigger source (input = diff resolution, internal = scheduler/invariant). It is never inferred from ctx.Err(). Ordinary aggregate budget exhaustion is a controlled coverage stop wired to --max-tokens-budget: it sets a pending FailureBudget cause and sweeps only uncovered items, without creating run_failure. Contract-level recorders for cancelled and run-level timeout, plus run_failure.classification=budget for a genuine counter/scheduler anomaly, remain available but have no live trigger in this release (no new SIGINT handler or global deadline).
• Coverage ≠ findings. Findings count never participates in terminal-state computation; complete may have zero or many findings.
• Manifest-safe redaction. Production manifest failure reasons use fixed, allow-listed summaries instead of raw provider errors. sanitizeReason provides a second redaction floor for URL credentials, Bearer/Basic tokens, credential-like assignments, and control characters. CLI output suppresses raw subtask_error warnings when a manifest is present. Existing session checkpoint and conversation persistence behavior is unchanged.
• Persistence errors surface. Finalize / WriteSessionEnd return errors up the stack across all exit paths—the no-files path, the loadDiffs-failure path, the normal dispatch path, and the scan path. On the normal path, dispatch and persistence errors are reported via errors.Join. A persistence failure does not rewrite the frozen manifest; it surfaces as a delivery error with a non-zero exit code.
• Resume preserves lineage, not copied input. A child run records its own currently resolved input and links the direct parent through parent_run_id; immutable-ref drift rejection is unchanged and remains out of scope.

Terminal state

State	Condition
complete	selected non-empty, failed empty, no run_failure
partial	0 < failed < selected, no run_failure
failed	all selected failed, or run_failure present
skipped	selected empty, no run_failure

waived items count as covered, so a run containing only completed / reused / waived items is complete. Findings count never affects the state.

scan is unchanged and continues to emit the legacy status values; the two commands do not yet share the same status contract.

Compatibility

• ocr review --format json intentionally migrates top-level status from the legacy success / completed_with_errors / completed_with_warnings values (and feat(agent): add token/tool-call cost guardrails to the review path #508's budget_exceeded) to the manifest terminal states complete / partial / failed / skipped.
• Budget-aware consumers should read summary.budget_exceeded or inspect manifest.coverage.failed[].classification == "budget"; the terminal state remains coverage-derived instead of being overwritten by the budget flag.
• A budget stop exits 0 when any selected item was covered (partial), and exits non-zero when every selected item failed (failed). scan retains its legacy output contract.

Upstream integration

This branch is synchronized through upstream/main@c391892 and preserves the upstream behavior merged while #367 was in progress:

• feat(agent): add token/tool-call cost guardrails to the review path #508 aggregate token budget: keeps the flag, dispatch guardrail, usage summary, warnings, and BudgetExceeded() signal; its former status=budget_exceeded output is adapted to the manifest terminal-state contract described above.
• fix: honor per-file review terminal states #582 per-file completion semantics: only task_done(status=DONE) marks an item completed; FAILED, provider errors, missing completion, and structured MainLoopStop reasons keep the item failed and flow into manifest coverage.
• Merge conflicts in CLI output, agent scheduling/budget handling, llmloop termination, and scan call sites were reconciled to retain both upstream behavior and the manifest's single-source-of-truth invariants.

How to test

# 1. Run a review; inspect the manifest in JSON output.
ocr review --from main --to feature --format json | jq .manifest

# 2. A mixed-success run shows terminal_state=partial and lists failed items.
#    Text mode no longer prints "Looks good to me." for partial/failed.

# 3. Resume records parent_run_id and marks checkpoint hits as reused.
ocr review --from main --to feature --resume <session-id> --format json | jq .manifest.parent_run_id
ocr review --from main --to feature --resume <session-id> --format json | jq .manifest.coverage.reused

# 4. session list/show and viewer prefer session_end.run_manifest.
ocr session list
ocr session show <session-id>
ocr viewer

Checklist

• go test -race -count=1 ./... green
• go vet ./... clean
• gofmt -l and go mod tidy produce no diff
• go build ./... succeeds; CLI smoke checks pass
• GitHub Actions and CLA checks are green
• Tests cover the implemented v1 verification matrix: full success, zero findings, mixed failures, all-failed, aggregate-budget partial/all-failed exit boundaries, run-level failure (input / internal), per-item timeout/budget/panic, skipped, resume (parent_run_id + reused), provider transition, task_done completion semantics, structured main-loop stops (max_rounds / empty_rounds / compression), cancellation contract, SealSelected lifecycle, failed-finalize rollback, conflicting/idempotent transitions, invalid failure class, empty waiver reason, cross-exit manifest consistency, malformed/legacy/aborted session display, large JSONL/non-EOF reader behavior, and security redaction
• On successful persistence, CLI JSON manifest and session_end.run_manifest serialize the same frozen manifest value
• Manifest fields and review CLI JSON do not expose raw provider errors, credentials, diff/prompt/response bodies, or provider tokens

Out of scope (deliberately)

• scan is not wired to the v1 manifest (Non-Goal; it passes a nil manifest and emitRunResult is nil-safe) and keeps its legacy status values.
• Waiver user entry is deferred to a follow-up; this release only ships the waived output semantics.
• cancelled and run-level timeout have contract coverage but no live source in this release—no new SIGINT handler or global deadline is added. Normal aggregate budget exhaustion is live through --max-tokens-budget and is represented as per-item failed(budget) coverage; run_failure.classification=budget is reserved for a separate run-level counter/scheduler anomaly and has no live trigger.
• Resume matching and checkpoint persistence are unchanged. A resumed child records its own currently resolved input and links the direct parent through parent_run_id; ref-drift rejection and "diff by immutable SHA" are deferred.
• Old sessions are displayed as legacy, never faked as v1 complete.

[github-actions]

🔍 OpenCodeReview found 7 issue(s) in this PR.

• ✅ Successfully posted inline: 7 comment(s)

[github-actions]

Performance: The stated motivation for this refactor is to handle lines exceeding the former 10 MiB scanner limit. However, bufio.NewReader uses a default 4096-byte buffer. For very large JSONL lines (multi-megabyte manifest records), ReadBytes will repeatedly grow its internal buffer through many small allocations and copies, causing significant GC pressure. Consider using bufio.NewReaderSize with a larger initial buffer to reduce allocation overhead for the expected large-line use case.

Suggestion:

Suggested change

	reader := bufio.NewReader(r)
	reader := bufio.NewReaderSize(r, 64*1024)

[lizhengfeng101]

merge conflicts

[lizhengfeng101]

LGTM

[lizhengfeng101]

LGTM