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; SetRunFailure records 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; 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); text output no longer shows Looks good to me. for partial/failed; session list/show and the viewer prefer session_end.run_manifest and fall back to legacy 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(). Contract-level recorders and sweeping for cancelled, run-level timeout, and run-level budget are in place but have no live source in this release (no new SIGINT handler, global deadline, or run-level budget pool).
• 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.

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 build ./... succeeds
• go test ./internal/session/... ./internal/agent/... ./cmd/opencodereview/... green
• go vet ./... and go fmt ./... clean
• Tests cover the implemented v1 verification matrix: full success, zero findings, mixed failures, all-failed, run-level failure (input / internal), per-item timeout/budget/panic, skipped, resume (parent_run_id + reused), provider transition (the resumed child records its current provider/model, links parent_run_id, and preserves checkpoint reuse), cancellation (contract-level: run_failure=cancelled sweeps pending items to failed(cancelled)), SealSelected lifecycle, conflicting/idempotent transitions, invalid failure class, empty waiver reason, cross-exit manifest consistency, legacy/aborted session display, 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, run-level timeout, and run-level budget have contract and unit tests but no live source in this release—no new SIGINT handler, global deadline, or run-level budget pool is added. The recorders, sweep, and Finalize contract are complete so they activate automatically when those run controls are added later.
• 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]

Breaking API change: When a manifest is present, the JSON status field changes from "success" to the manifest's TerminalState value (e.g., "complete", "partial", "failed"). Downstream consumers that check for status == "success" will no longer match successful reviews. Consider documenting this as a breaking change in release notes, or maintaining backward compatibility by mapping StateComplete → "success" if there are existing consumers of this JSON output.

[github-actions]

When runErr == nil but manifest.TerminalState == StateFailed, the error message lacks diagnostic detail. Consider including the RunFailure classification and reason (when available) or the count of failed items so the stderr message gives the user actionable context without requiring them to re-run with JSON output.

For example:

if manifest.RunFailure != nil {
    return fmt.Errorf("review failed (%s): %s", manifest.RunFailure.Classification, manifest.RunFailure.Reason)
}
return fmt.Errorf("review failed: %d of %d selected item(s) failed",
    len(manifest.Coverage.Failed), len(manifest.Coverage.Selected))

[github-actions]

If a persisted manifest has an empty or unrecognized TerminalState (e.g., due to data corruption or a future schema version), this will return an empty string, resulting in a blank status column in the table and detail views. Consider adding a fallback for safety:

if s.RunManifest != nil {
    if ts := s.RunManifest.TerminalState; ts != "" {
        return string(ts)
    }
    return "unknown"
}

This is defensive — Finalize always sets a valid state, but data loaded from disk could be malformed.

[github-actions]

Missing test coverage: there are no tests verifying that StopEmptyRounds or StopCompression are returned from their respective trigger points. The StopMaxRounds path is tested in TestRunPerFile_MaxToolRequestsWithoutTaskDoneDoesNotComplete, but the other two stop reasons should have analogous tests to prevent regressions.

For StopEmptyRounds: configure a fake client that returns tool calls with no valid results for maxConsecutiveEmptyRounds consecutive rounds, then assert stop == StopEmptyRounds.

For StopCompression: configure a scenario where addNextMessage returns false, then assert stop == StopCompression.

[github-actions]

Bug: The sweep loop mutates items from stateSelected to stateFailed before validation. If validateLocked subsequently returns an error, the builder is left unfrozen but the sweep mutations are irreversible — those items can never be re-transitioned (the transition method rejects non-selected states). This means:

1. A retried Finalize after fixing the validation issue will find no stateSelected items to sweep, so the sweep classification/reason from the first (failed) attempt is permanently baked in.
2. Any Mark* calls between the failed and retried Finalize for swept items will return errors rather than applying the correct outcome.

Consider either (a) moving the sweep after validation (build coverage, validate structural invariants that don't depend on the sweep, then sweep and rebuild), or (b) snapshotting item states before the sweep and restoring them if validation fails.

[github-actions]

Bug: When ReadBytes('\n') encounters a non-EOF error (e.g., disk I/O error), it can return partial bytes along with the error. The current code visits these partial bytes before checking the error. In peekSession, this overwrites lastLine with corrupt data, potentially replacing a valid previous last line. If the file's last complete line was a valid session_end record but a subsequent partial read occurs, the session_end detection will silently fail because the partial data won't parse as JSON.

The fix is to only visit lines when there is no error, or at minimum skip visiting when the error is not io.EOF:

Suggestion:

Suggested change

	func readJSONLLines(r io.Reader, visit func([]byte)) error {
	reader := bufio.NewReader(r)
	for {
	line, err := reader.ReadBytes('\n')
	if len(line) > 0 {
	visit(line)
	}
	switch err {
	case nil:
	continue
	case io.EOF:
	return nil
	default:
	return err
	}
	}
	}
	func readJSONLLines(r io.Reader, visit func([]byte)) error {
	reader := bufio.NewReader(r)
	for {
	line, err := reader.ReadBytes('\n')
	if err != nil {
	if err == io.EOF {
	// Process any remaining bytes before EOF (last line without trailing newline).
	if len(line) > 0 {
	visit(line)
	}
	return nil
	}
	// Non-EOF error: discard partial data to avoid corrupting downstream state.
	return err
	}
	visit(line)
	}
	}

[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)