feat: resume reviewer sessions across review rounds

[rianjs]

Summary\n- persist a fixed reviewer cohort and exact provider sessions per PR/profile/posting identity\n- checkpoint discussion summaries and resolutions before reviewer execution with PR-backed recovery\n- document first-run, rerun, fresh-session, and interrupted-run semantics\n\n## Verification\n- make check\n- focused affected-package tests (559 passed)\n- targeted race tests (16 passed)\n- command help rendered and inspected\n\nCloses #529

[rianjs]

TDD assessment: PR #530 / issue #529

Verdict: changes requested

The implementation covers the main cohort/session/checkpoint design and its focused suite passes, but an interrupted run after a successful early checkpoint can omit the already-resolved discussion outcome from reviewer prompts on recovery. That violates the explicit requirement that reused reviewers receive discussion outcomes.

Strengths

• PR-scoped ordered cohort persistence, runtime compatibility checks, exact reviewer session checkpoint/resume, and --fresh-session replacement are covered in pipeline and ledger tests.
• Thread analysis is sequential and chains the orchestrator provider session; checkpoint posting validates head/base, posts reply before resolve, and keeps the run open.
• The lifecycle document, README link, and command-help assertions clearly describe rerun, fresh-session, recovery, and cr sessions scope.

Findings

High — post-checkpoint recovery drops discussion outcomes before reviewer execution

analyzeReviewThreads returns no responses once the authoritative PR summary marker suppresses the completed thread (pipeline.go:1289-1317). checkpointThreadResponses then returns immediately without loading already-persisted checkpoint actions when responses are empty (pipeline.go:1324-1327). The reviewer prompt only includes discussion_outcomes when responses are nonempty (prompts.go:43-45). Therefore, if the reply is posted and the process dies before reviewers, recovery correctly avoids duplicate analysis but reruns reviewers without the required outcome/status context.

Fix: reconstruct the outcome/status from persisted checkpoint actions (or persist a small durable outcome record) when remote markers suppress reanalysis, and pass it to resumed reviewers.

Medium — no end-to-end test proves the authoritative-marker interruption contract

Existing tests independently cover thread sequencing/checkpointing, cohort session reuse, and the reviewrun partial-checkpoint continuation, but not one real pipeline/reviewrun scenario: post a summary marker, interrupt before reviewers, recover the same run, assert no second thread LLM call/post, and assert the reviewer prompt includes the original outcome with reconciled status. This is the acceptance boundary most likely to regress.

Low — ordinary follow-up and --rerun coverage is indirect

TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun starts with a dry run and invokes pipeline.Live directly. Add command/reviewrun-level tests for both a normal later invocation and --rerun, asserting one fixed cohort, exact reviewer and orchestrator resume IDs, and no new selection. Add the paired --fresh-session --rerun test asserting both are reset.

Recommended tests/fixes

1. Add the high-severity same-run interruption test above; fix recovery so it preserves discussion outcomes.
2. Add a PR-marker-authoritative test with stale/missing local task data to confirm marker recovery does not duplicate reply/resolve and still gives reviewers the outcome.
3. Add reviewrun command-path tests for normal follow-up, --rerun, and --fresh-session --rerun session/cohort semantics.

Commands run

rtk git status --short --branch
rtk git diff --stat main...HEAD
rtk git diff --name-status main...HEAD
rtk go test ./internal/ledger ./internal/outbox ./internal/pipeline ./internal/reviewrun ./internal/threadanalysis ./internal/cmd/reviewcmd ./internal/cmd/sessionscmd
rtk make test
rtk go test -race ./internal/ledger ./internal/outbox ./internal/pipeline ./internal/reviewrun ./internal/threadanalysis
rtk git diff --check main...HEAD

All test commands passed; the focused command reported 532 passing tests. No repository files were changed.

[rianjs]

Addressed the Terra assessment in 5f1acc5. Recovery now reconstructs reviewer discussion context from the authoritative PR summary marker even when local thread task/action state is missing, reports it as posted, avoids re-analysis/duplicate posting, and preserves persisted action details when available. Added crash/resume and marker-only regressions; focused suites and targeted race tests pass.

[rianjs]

Empirical verification (feature binary, disposable draft PR #531):

• Fresh baseline run 718fde73-8b25-4b1b-bd7e-bc128476146d selected go:implementation-tests + session-probe:division, created durable reviewer sessions 019fbb13-fa20-76a0-84aa-0a15791f48d9 and 019fbb13-fa14-7791-9bdd-c7887e0389d0, and chained selection -> rollup on orchestrator 019fbb13-ba80-7f80-ae2d-f234577f58c0.
• --rerun run 8dea2464-633b-43e7-a25c-d19cd4ff95f1 logged reused reviewer cohort, omitted selection, and resumed both exact reviewer IDs. I sent SIGINT after planning and before posting.
• A plain cr review resumed that exact run ID with no LLM/selection work and completed its pending posts (posted=2, pending=0).
• Final rerun f266e01d-fd1a-4e96-8c87-e07853371c0e again omitted selection, resumed both exact reviewer IDs, and resumed the original orchestrator ID exactly.
• --fresh-session replaced the earlier cohort/orchestrator IDs before the corrected baseline.

The empirical runs caught and drove fixes for Codex reviewer workspace resume, ephemeral reviewer starts, ephemeral orchestrator starts, and dossier-session displacement of the orchestrator chain. Targeted tests cover each regression.

Limitation: the disposable profile posts as my Git identity, so it cannot manufacture a newer reply from a distinct human identity without interactive GitHub App credentials. PR-marker crash recovery, early reply ordering, and duplicate suppression are covered by pipeline/outbox/acceptance tests plus the one-time Terra assessment.

[rianjs]

Automated PR Review

Reviewed commit: 45fe3c8d3ae1
Profile: probe - Posting as: rianjs

Summary

Reviewer	Findings
go:implementation-tests	0
policies:conventions	2
structure:repo-health	1

policies:conventions (2 findings)

Major - README.md:14

This PR updates the top-level README bullets and CLI help to the new orchestrator-vs-reviewer-cohort model, but the detailed cr review and cr sessions sections later in the same file still describe the old generic LLM-session contract (--fresh-session as a one-off provider conversation, --session as overriding the whole PR scope, and cr sessions as listing/deleting generic LLM sessions at lines 1114-1175 and 1326-1345). That leaves the canonical user-facing docs internally contradictory about the exact behavior introduced here. Update these later README sections to match docs/review-lifecycle.md and the revised Cobra help text.

Minor - docs/development.md:28

The project overview still says the CLI provides "named LLM session management," but this change narrows that contract everywhere else to named orchestrator sessions plus automatic PR-scoped reviewer cohorts. Since this file was touched to point at the new lifecycle doc, keep the overview terminology aligned as well so repo-local guidance does not preserve the old session model.

structure:repo-health (1 finding)

Major - internal/pipeline/pipeline.go:9

The new resumable-review state is now a required part of the pipeline contract, but Store still exposes only the preexisting methods and the implementation falls back to runtime type assertions for checkpointPlanningStore and reviewerCohortStore later in the file. That weakens the boundary: a wrapper, fake, or future alternate store can still satisfy pipeline.Store and compile, then fail only when a resume/checkpoint path hits "checkpoint planning store is required" or "reviewer cohort store is required". For a durable-state feature, this should be enforced at compile time. Fold the new planning/cohort methods into pipeline.Store (or replace the ad hoc assertions with one composite required store interface in Options) so unsupported stores fail during wiring/tests instead of during live recovery.

Reviewer Coverage

Reviewer	Status	Inspected	Skipped	Constraints
go:implementation-tests	complete_broad	cmd/cr/main_test.go, internal/app/runtime.go, internal/cmd/datacmd/datacmd_test.go, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/cmd/sessionscmd/sessionscmd.go, internal/cmd/sessionscmd/sessionscmd_test.go, internal/ledger/ledger.go, internal/ledger/ledger_test.go, internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_test.go, internal/llmlifecycle/lifecycle.go, internal/llmlifecycle/lifecycle_test.go, internal/outbox/outbox.go, internal/outbox/outbox_test.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/prompts.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go, internal/threadanalysis/threadanalysis.go, internal/threadanalysis/threadanalysis_test.go	unavailable	Focused go test runs could not be used as a strong signal in this environment because package builds/import checks fail under the workspace path with spaces (for example, clang/go list path handling errors), so findings are based on source inspection rather than successful local execution.; Review scope limited to the assigned Go implementation and behavioral test files.
policies:conventions	complete_broad	README.md, docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/cmd/reviewcmd/reviewcmd.go, internal/cmd/reviewcmd/reviewcmd_test.go, internal/cmd/sessionscmd/sessionscmd.go, internal/cmd/sessionscmd/sessionscmd_test.go	unavailable	Review scoped to the assigned changed files and repo-local guidance available in this checkout; sibling shared cli-common convenience copies were not present locally to inspect.
structure:repo-health	complete_broad	docs/development.md, internal/ledger/ledger.go, internal/ledger/ledger_test.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/reviewrun/reviewrun.go, internal/reviewrun/reviewrun_test.go	unavailable	Reviewed only the assigned files for structural repository-health risks, with limited spot checks of adjacent code solely to confirm changed contracts.

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 5m 08s | unavailable | gpt-5.4 | cr dev

Field	Value
Model	gpt-5.4
Reviewers	go:implementation-tests, policies:conventions, structure:repo-health
Engine	codex_cli · gpt-5.4
Reviewed by	cr · rianjs
Duration	5m 08s wall · 9m 58s compute
Cost	unavailable
Tokens	2.6M in / 29.6k out

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost	Duration
orchestrator-selection	gpt-5.4	30.8k	1.2k	23.3k	unavailable	unavailable	27s
go:implementation-tests	gpt-5.4	1.4M	11.5k	1.3M	unavailable	unavailable	4m 04s
policies:conventions	gpt-5.4	187.3k	4.5k	152.3k	unavailable	unavailable	1m 31s
structure:repo-health	gpt-5.4	870.6k	10.8k	780.7k	unavailable	unavailable	3m 40s
orchestrator-rollup	gpt-5.4	71.3k	1.6k	56.3k	unavailable	unavailable	15s