From 2efaa7740c2a52fcf80a2b0f75bb6a32f2dd8915 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 21:04:00 -0400 Subject: [PATCH 1/7] feat: resume pull request reviewer sessions Persist fixed reviewer cohorts and provider sessions across review rounds, checkpoint discussion replies before reviewer execution, and document recovery semantics.\n\nCloses #529 --- README.md | 4 +- cmd/cr/main_test.go | 2 +- docs/architecture.md | 4 + docs/development.md | 2 + docs/review-lifecycle.md | 92 ++++ internal/app/runtime.go | 23 + internal/cmd/datacmd/datacmd_test.go | 7 +- internal/cmd/reviewcmd/reviewcmd.go | 16 +- internal/cmd/reviewcmd/reviewcmd_test.go | 6 +- internal/cmd/sessionscmd/sessionscmd.go | 9 +- internal/cmd/sessionscmd/sessionscmd_test.go | 12 + internal/ledger/ledger.go | 336 ++++++++++++- internal/ledger/ledger_test.go | 120 ++++- internal/outbox/outbox.go | 108 ++++ internal/outbox/outbox_test.go | 54 ++ internal/pipeline/pipeline.go | 473 ++++++++++++++++-- internal/pipeline/pipeline_test.go | 241 ++++++++- internal/pipeline/prompts.go | 32 +- internal/reviewrun/reviewrun.go | 26 +- internal/reviewrun/reviewrun_test.go | 46 ++ internal/threadanalysis/threadanalysis.go | 48 +- .../threadanalysis/threadanalysis_test.go | 65 +++ 22 files changed, 1628 insertions(+), 98 deletions(-) create mode 100644 docs/review-lifecycle.md diff --git a/README.md b/README.md index 478d641..ad96e07 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ Use `cr` when you want to: - preview review actions before posting anything; - run a live PR review with idempotent posting and resume behavior; -- reuse PR-scoped LLM sessions by default or named sessions across related live reviews; +- reuse a fixed PR-scoped reviewer cohort and its LLM sessions by default; - inspect trusted reviewer agents available to a repository; - manage local review run data and credentials from the terminal. @@ -98,6 +98,8 @@ review data lives under the OS data directory for the `cr` binary. For repo review guidance, reviewer-facing dossier context, and dossier/workbench retention conventions, see [docs/review-guidance.md](docs/review-guidance.md). +For first runs, reruns, fresh sessions, thread checkpoints, and interrupted-run +recovery, see [docs/review-lifecycle.md](docs/review-lifecycle.md). ## Authentication And Setup diff --git a/cmd/cr/main_test.go b/cmd/cr/main_test.go index 6e186d2..a179c18 100644 --- a/cmd/cr/main_test.go +++ b/cmd/cr/main_test.go @@ -34,7 +34,7 @@ func TestRun(t *testing.T) { {name: "me command wired", args: []string{"me", "--help"}, wantCode: 0, wantStdout: "Resolve and cache", wantStdoutSubstring: true}, {name: "agents command wired", args: []string{"agents", "--help"}, wantCode: 0, wantStdout: "Inspect trusted review agents", wantStdoutSubstring: true}, {name: "review command wired", args: []string{"review", "--help"}, wantCode: 0, wantStdout: "Run an automated pull-request review", wantStdoutSubstring: true}, - {name: "sessions command wired", args: []string{"sessions", "--help"}, wantCode: 0, wantStdout: "Manage named LLM sessions", wantStdoutSubstring: true}, + {name: "sessions command wired", args: []string{"sessions", "--help"}, wantCode: 0, wantStdout: "Manage named orchestrator sessions", wantStdoutSubstring: true}, {name: "data command wired", args: []string{"data", "--help"}, wantCode: 0, wantStdout: "Manage local review data", wantStdoutSubstring: true}, {name: "benchmark command wired", args: []string{"benchmark", "--help"}, wantCode: 0, wantStdout: "Validate, inspect, and run benchmark suites", wantStdoutSubstring: true}, {name: "benchmark select command wired", args: []string{"benchmark", "select", "--help"}, wantCode: 0, wantStdout: "Run selector-only benchmark suites", wantStdoutSubstring: true}, diff --git a/docs/architecture.md b/docs/architecture.md index 18614b9..60bb2cd 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -3,6 +3,10 @@ This document records review-pipeline boundaries that are intended to stay stable as the implementation evolves. +The canonical runtime sequence, state ownership, rerun/fresh-session semantics, +and interrupted-run recovery contract live in +[review-lifecycle.md](review-lifecycle.md). + ## Durable LLM Execution Boundary All production structured LLM actions must flow through diff --git a/docs/development.md b/docs/development.md index abdfb1d..ecf4ebd 100644 --- a/docs/development.md +++ b/docs/development.md @@ -28,6 +28,8 @@ Architecture guardrails for LLM execution, model resolution, Git provider writes, command and review harness boundaries, inline thread lifecycle, and retention live in [`docs/architecture.md`](architecture.md). +The canonical first-run, rerun, fresh-session, checkpoint, and recovery sequence +lives in [`docs/review-lifecycle.md`](review-lifecycle.md). The temporary architecture refactor workstream context for issue #420 lives in [`docs/architecture-refactor-workstream.md`](architecture-refactor-workstream.md). diff --git a/docs/review-lifecycle.md b/docs/review-lifecycle.md new file mode 100644 index 0000000..5d8f665 --- /dev/null +++ b/docs/review-lifecycle.md @@ -0,0 +1,92 @@ +# Review Lifecycle + +This document is the canonical end-to-end lifecycle for `cr review`. It covers +the resumable reviewer sessions and early thread checkpoints introduced by +issue #529. Task artifact details remain in +[llm-task-artifacts.md](llm-task-artifacts.md). + +## State ownership + +| State | Scope | Authority | +|---|---|---| +| Review run, findings, and planned actions | PR head/base, profile, posting identity, attempt | Local ledger and run artifacts | +| Reviewer cohort and reviewer provider sessions | PR, profile, posting identity | Local ledger | +| Default orchestrator session | PR, profile, posting identity | Local named-session row | +| Explicit `--session` orchestrator session | Supplied name | Local named-session row | +| Posted thread summary | Posting identity and inline thread | PR marker; local action/task state is fallback until posting succeeds | +| Posted review actions | Run and action marker | PR state reconciled with the local outbox | + +The reviewer cohort is ordered and fixed after its first selection. Each member +stores its agent ID, broad or scoped assignment, runtime compatibility fields, +and latest provider session ID. The explicit `--session` flag never changes the +reviewer cohort's PR scope. + +## First run + +1. Resolve the PR, changed files, discussion, reviewer catalog, and runtime. +2. Run selection on the orchestrator provider session. +3. Persist the selected reviewer cohort before starting reviewers. +4. Analyze eligible human replies sequentially on the same orchestrator + session. +5. Persist thread reply and resolution actions, then run the partial outbox. + Replies are attempted before resolutions. The run remains open. +6. Give every active reviewer the analyzed discussion outcome and actual early + action status (`posted`, `pending`, or dry-run-only), then run reviewers. +7. Run rollup on the latest orchestrator provider session, merge final planning + around the early action IDs and statuses, and run the final outbox. + +A scoped cohort member whose rebased assignment is empty remains in the cohort +but does not receive a no-op LLM call. + +## Later runs and `--rerun` + +Plain follow-up runs and `--rerun` both load the original PR-scoped cohort, +validate every member against the current catalog and reviewer runtime, and +deterministically rebase assignments onto the current changed files. Selection +does not run again. Active reviewers resume their exact saved provider session. + +`--rerun` changes local gate behavior only: it bypasses approval, override, +resume, and marker gates to allocate a new review attempt. It does not select a +new cohort or start new provider conversations. + +Reuse fails with `--fresh-session` guidance when a cohort member is missing or +runtime-incompatible, `--max-agents` is smaller than the saved cohort, or the +cohort cannot cover every changed file. + +## `--fresh-session` + +`--fresh-session` clears the active PR-scoped reviewer cohort for this review, +does not resume the orchestrator provider session, runs selection again, and +persists the replacement cohort. It can be combined with `--rerun` when both +gate bypass and new reviewer/orchestrator conversations are required. + +## Interrupted-run recovery + +Structured LLM task metadata resumes an interrupted task inside the same run. +If interruption occurs after early thread actions are persisted, those actions +are valid partial planning state: recovery retries/reconciles them and continues +the remaining planning phases. Final planning preserves their IDs, attempts, +posted state, and pending errors. + +A posting-identity-authored `codereview:thread-summary` marker suppresses repeat +thread analysis. A newer human reply after that marker reopens the thread. Local +task and action state suppresses duplicate work only while the marked reply has +not reached the PR. + +## Failure and idempotency behavior + +- Reviewer-local LLM failures remain isolated and retain any provider session + ID reported by the failed attempt. +- Early posting failures remain pending and do not prevent reviewer execution. + Final posting retries them. +- A moved head or base stops checkpoint posting before reviewer work. +- Outbox reconciliation checks markers before every dispatch, so a process + crash after a provider write but before the local update does not duplicate + the reply or final review. +- Thread replies are ordered before resolutions, and a resolution is not sent + until its reply is posted or reconciled. +- Completed summary markers on the PR outrank stale local fallback state. + +`cr sessions` lists and manages named orchestrator sessions only. Reviewer +cohorts are automatic PR state and are replaced through +`cr review --fresh-session`. diff --git a/internal/app/runtime.go b/internal/app/runtime.go index 3c56b89..0eb54c9 100644 --- a/internal/app/runtime.go +++ b/internal/app/runtime.go @@ -412,6 +412,22 @@ func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitPr RetentionManualOnly: req.RetentionManualOnly, ResolveRepoRoot: resolveRepoRoot, GitCommand: gitCommand, + ThreadCheckpoint: func(ctx context.Context, run ledger.Run, pipelineReq pipeline.Request) error { + result, err := outbox.PostCheckpoint(ctx, outbox.Options{Store: ledgerStore, Provider: liveProvider, Limiter: limiter}, outbox.Request{ + Run: run, + PRRef: pipelineReq.PRRef, + PostingIdentity: pipelineReq.PostingIdentity, + DesiredOutcome: ledger.OutcomeComment, + ResolveThreadPermissionAdvisory: postingUsesGitHubApp(profile), + }) + if err != nil { + return err + } + if result.Aborted { + return gitprovider.ErrStaleSHA + } + return nil + }, } return reviewRunner{ pipeline: pipelineOpts, @@ -441,6 +457,13 @@ func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitPr } } +func postingUsesGitHubApp(profile config.Profile) bool { + if profile.ReviewerCredentials != nil { + return profile.ReviewerCredentials.AuthMode == config.GitAuthModeGitHubApp + } + return profile.Git.AuthMode == config.GitAuthModeGitHubApp +} + func buildApprovalOverrideClassifier(profile config.Profile, adapter llm.Adapter, warnings io.Writer) approvaloverride.Classifier { return &lazyApprovalOverrideClassifier{ profile: profile, diff --git a/internal/cmd/datacmd/datacmd_test.go b/internal/cmd/datacmd/datacmd_test.go index e85676b..9b79cc0 100644 --- a/internal/cmd/datacmd/datacmd_test.go +++ b/internal/cmd/datacmd/datacmd_test.go @@ -243,9 +243,10 @@ func TestDataPruneDefaultIgnoresConfiguredRetention(t *testing.T) { } layout := mustLayout(t) store := openLedgerForTest(t, layout) - allocateRun(t, store, layout, "live-31d", ledger.PostModeLive, testNow().Add(-31*24*time.Hour)) - allocateRun(t, store, layout, "live-91d", ledger.PostModeLive, testNow().Add(-91*24*time.Hour)) - allocateRun(t, store, layout, "dry-8d", ledger.PostModeDryRun, testNow().Add(-8*24*time.Hour)) + now := time.Now().UTC() + allocateRun(t, store, layout, "live-31d", ledger.PostModeLive, now.Add(-31*24*time.Hour)) + allocateRun(t, store, layout, "live-91d", ledger.PostModeLive, now.Add(-91*24*time.Hour)) + allocateRun(t, store, layout, "dry-8d", ledger.PostModeDryRun, now.Add(-8*24*time.Hour)) if err := store.Close(); err != nil { t.Fatalf("Close: %v", err) } diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index a824b53..2ecfcd4 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -37,11 +37,11 @@ default, if the posting identity has already approved the PR, cr exits before any LLM classifier or reviewer work, even if newer commits made that approval stale. Use --rerun to bypass these local gates and force a new live review. -Provider session reuse is independent of local review gates. Reviews reuse a -durable session scoped to the PR, profile, and posting identity by default, -including after pushes and across dry-run/live invocations. --session selects -a named live-review session instead; --fresh-session starts a fresh provider -conversation for this invocation. +Session reuse is independent of local review gates. Plain follow-up reviews and +--rerun reuse the PR's original reviewer cohort and each reviewer's provider +session. --session scopes only the orchestrator conversation. --fresh-session +reselects the reviewer cohort and starts fresh orchestrator and reviewer +conversations without changing local review gates. --fast requests fast execution for reviewer agents only; --no-fast disables it. CLI flags override the profile default. Unsupported runtimes or reviewer models @@ -106,14 +106,14 @@ func RegisterWithFactory(rootCmd *cobra.Command, opts *root.Options, factory Run } cmd.Flags().BoolVar(&flags.dryRun, "dry-run", false, "Plan review actions without posting") cmd.Flags().BoolVar(&flags.noPost, "no-post", false, "Alias for --dry-run") - cmd.Flags().BoolVar(&flags.rerun, "rerun", false, "Bypass local approval/override, resume, and marker gates; reuse the LLM session") + cmd.Flags().BoolVar(&flags.rerun, "rerun", false, "Bypass local approval/override, resume, and marker gates; reuse reviewer and orchestrator sessions") cmd.Flags().BoolVar(&flags.retryPosts, "retry-posts", false, "Retry missing or failed required posts without rerunning review or checking approval overrides") - cmd.Flags().BoolVar(&flags.freshSession, "fresh-session", false, "Start a fresh provider conversation without changing local review gates") + cmd.Flags().BoolVar(&flags.freshSession, "fresh-session", false, "Reselect reviewers and start fresh reviewer/orchestrator conversations without changing local review gates") cmd.Flags().BoolVar(&flags.fast, "fast", false, "Request fast execution for supported reviewer runtimes and models") cmd.Flags().BoolVar(&flags.noFast, "no-fast", false, "Disable fast execution for reviewer agents") cmd.Flags().StringArrayVar(&flags.agentsDirs, "agents-dir", nil, "Additional trusted agents directory") cmd.Flags().StringVar(&flags.failOn, "fail-on", "", "Exit 1 when a finding at or above severity exists") - cmd.Flags().StringVar(&flags.sessionName, "session", "", "Override the PR's default LLM session with a named live-review session") + cmd.Flags().StringVar(&flags.sessionName, "session", "", "Override the PR's default orchestrator session with a named live-review session") root.AddJSONFlag(cmd, &flags.jsonOutput) cmd.Flags().StringVar(&flags.selectionModel, "selection-model", "", "Override selection model for dry-run review") cmd.Flags().StringVar(&flags.selectionEffort, "selection-effort", "", "Override selection effort for dry-run review") diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index bf7cb0c..21f0f48 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -377,8 +377,10 @@ func TestReviewHelpDocumentsApprovalFastPaths(t *testing.T) { for _, want := range []string{ "already approved the PR", "--rerun to bypass these local gates", - "Provider session reuse is independent", - "--fresh-session starts a fresh provider", + "--rerun reuse the PR's original reviewer cohort", + "--session scopes only the orchestrator conversation", + "--fresh-session", + "reselects the reviewer cohort", "--fast requests fast execution for reviewer agents only", "approval override request newer than that marker", "--retry-posts is recovery-only", diff --git a/internal/cmd/sessionscmd/sessionscmd.go b/internal/cmd/sessionscmd/sessionscmd.go index f203ada..634716d 100644 --- a/internal/cmd/sessionscmd/sessionscmd.go +++ b/internal/cmd/sessionscmd/sessionscmd.go @@ -25,7 +25,8 @@ type commandFlags struct { func Register(rootCmd *cobra.Command, opts *root.Options) { cmd := &cobra.Command{ Use: "sessions", - Short: "Manage named LLM sessions", + Short: "Manage named orchestrator sessions", + Long: "Manage named orchestrator sessions. PR-scoped reviewer cohorts are reused automatically and reset with cr review --fresh-session.", } cmd.AddCommand(newListCommand(opts), newShowCommand(opts), newDeleteCommand(opts)) rootCmd.AddCommand(cmd) @@ -35,7 +36,7 @@ func newListCommand(opts *root.Options) *cobra.Command { var flags commandFlags cmd := &cobra.Command{ Use: "list", - Short: "List named LLM sessions", + Short: "List named orchestrator sessions", Args: exitcode.NoArgs("sessions list accepts no arguments"), RunE: func(cmd *cobra.Command, _ []string) error { store, cleanup, err := openStore(cmd.Context(), nil, "sessions.list", false) @@ -67,7 +68,7 @@ func newShowCommand(opts *root.Options) *cobra.Command { var flags commandFlags cmd := &cobra.Command{ Use: "show ", - Short: "Show one named LLM session", + Short: "Show one named orchestrator session", Args: exitcode.NonEmptyArg("sessions show requires "), RunE: func(cmd *cobra.Command, args []string) error { name := strings.TrimSpace(args[0]) @@ -100,7 +101,7 @@ func newDeleteCommand(opts *root.Options) *cobra.Command { var flags commandFlags cmd := &cobra.Command{ Use: "delete ", - Short: "Delete one named LLM session", + Short: "Delete one named orchestrator session", Args: exitcode.NonEmptyArg("sessions delete requires "), RunE: func(cmd *cobra.Command, args []string) error { name := strings.TrimSpace(args[0]) diff --git a/internal/cmd/sessionscmd/sessionscmd_test.go b/internal/cmd/sessionscmd/sessionscmd_test.go index 3bf2620..feda8c1 100644 --- a/internal/cmd/sessionscmd/sessionscmd_test.go +++ b/internal/cmd/sessionscmd/sessionscmd_test.go @@ -40,6 +40,18 @@ func TestSessionsListText(t *testing.T) { } } +func TestSessionsHelpDistinguishesReviewerCohorts(t *testing.T) { + cmd, out := newTestCommand() + if err := root.Execute(cmd, []string{"sessions", "--help"}); err != nil { + t.Fatalf("Execute help: %v", err) + } + for _, want := range []string{"named orchestrator sessions", "reviewer cohorts are reused automatically", "--fresh-session"} { + if !strings.Contains(out.String(), want) { + t.Fatalf("help = %q, want %q", out.String(), want) + } + } +} + func TestSessionsListJSONEmpty(t *testing.T) { statedirtest.Hermetic(t) layout := mustDefaultLayoutNoCreate(t) diff --git a/internal/ledger/ledger.go b/internal/ledger/ledger.go index 9346c82..6e0c6f1 100644 --- a/internal/ledger/ledger.go +++ b/internal/ledger/ledger.go @@ -25,7 +25,7 @@ import ( const ( // SchemaVersion is the current ledger schema version. - SchemaVersion = 3 + SchemaVersion = 4 // DefaultBusyTimeout is the SQLite busy timeout configured at open. DefaultBusyTimeout = 5 * time.Second writeQueueSize = 64 @@ -363,6 +363,45 @@ type NamedSession struct { LastUsedAt time.Time } +// ReviewerAssignmentMode records whether a cohort member reviews every changed +// file or only its persisted/rebased assignment. +type ReviewerAssignmentMode string + +const ( + // ReviewerAssignmentBroad assigns every changed file to a reviewer. + ReviewerAssignmentBroad ReviewerAssignmentMode = "broad" + // ReviewerAssignmentScoped assigns only the persisted/rebased file set. + ReviewerAssignmentScoped ReviewerAssignmentMode = "scoped" +) + +// ReviewerCohortScope identifies one durable reviewer cohort. +type ReviewerCohortScope struct { + PRKey string + Profile string + PostingIdentity string +} + +// ReviewerCohortMember is one ordered reviewer and its resumable runtime state. +type ReviewerCohortMember struct { + AgentID string + AssignmentMode ReviewerAssignmentMode + Files []string + AllowedFiles []string + Model string + Effort string + Fast bool + ProviderSessionID string +} + +// ReviewerCohort is the fixed reviewer set for one PR/profile/posting identity. +type ReviewerCohort struct { + Scope ReviewerCohortScope + Adapter string + CreatedAt time.Time + UpdatedAt time.Time + Members []ReviewerCohortMember +} + // Open opens or creates a ledger database at path and applies migrations. func Open(ctx context.Context, path string) (*Store, error) { if strings.TrimSpace(path) == "" { @@ -498,6 +537,18 @@ func migrations() []dbmig.Migration { return err }, }, + { + Version: 4, + Name: "reviewer cohorts", + Up: func(ctx context.Context, tx *sql.Tx) error { + for _, statement := range reviewerCohortSchemaStatements { + if _, err := tx.ExecContext(ctx, statement); err != nil { + return err + } + } + return nil + }, + }, } } @@ -951,6 +1002,62 @@ func (s *Store) InsertPlanningResult(ctx context.Context, findings []Finding, ac }) } +// MergePlanningResult atomically adds final findings/actions while preserving +// mutable state for action IDs already persisted by an early checkpoint. +func (s *Store) MergePlanningResult(ctx context.Context, findings []Finding, actions []PlannedAction) error { + for _, finding := range findings { + if err := validateFinding(finding); err != nil { + return err + } + } + for _, action := range actions { + if err := validatePlannedAction(action, true); err != nil { + return err + } + } + return s.write(ctx, func(ctx context.Context, db *sql.DB) error { + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("ledger: begin merge planning result: %w", err) + } + defer func() { _ = tx.Rollback() }() + for _, finding := range findings { + var existingRunID string + err := tx.QueryRowContext(ctx, `SELECT run_id FROM findings WHERE finding_id = ?`, finding.FindingID).Scan(&existingRunID) + switch { + case err == nil && existingRunID == finding.RunID: + continue + case err == nil: + return fmt.Errorf("ledger: finding %q belongs to run %q", finding.FindingID, existingRunID) + case !errors.Is(err, sql.ErrNoRows): + return fmt.Errorf("ledger: check merged finding %q: %w", finding.FindingID, err) + } + if err := insertFindingRow(ctx, tx, finding); err != nil { + return fmt.Errorf("ledger: merge planning result finding: %w", err) + } + } + for _, action := range actions { + var existingRunID string + err := tx.QueryRowContext(ctx, `SELECT run_id FROM planned_actions WHERE action_id = ?`, action.ActionID).Scan(&existingRunID) + switch { + case err == nil && existingRunID == action.RunID: + continue + case err == nil: + return fmt.Errorf("ledger: action %q belongs to run %q", action.ActionID, existingRunID) + case !errors.Is(err, sql.ErrNoRows): + return fmt.Errorf("ledger: check merged action %q: %w", action.ActionID, err) + } + if err := insertPlannedActionRow(ctx, tx, action); err != nil { + return fmt.Errorf("ledger: merge planning result action: %w", err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("ledger: commit merge planning result: %w", err) + } + return nil + }) +} + func insertPlannedActionRow(ctx context.Context, db execer, action PlannedAction) error { payload, err := encodePlannedActionPayload(action.Action) if err != nil { @@ -1135,6 +1242,169 @@ ON CONFLICT(name) DO UPDATE SET }) } +// ReplaceReviewerCohort atomically replaces the complete ordered cohort. +func (s *Store) ReplaceReviewerCohort(ctx context.Context, cohort ReviewerCohort) error { + if err := validateReviewerCohort(cohort); err != nil { + return err + } + return s.write(ctx, func(ctx context.Context, db *sql.DB) error { + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("ledger: begin replace reviewer cohort: %w", err) + } + defer func() { _ = tx.Rollback() }() + updatedAt := cohort.UpdatedAt + if updatedAt.IsZero() { + updatedAt = cohort.CreatedAt + } + if _, err := tx.ExecContext(ctx, ` +INSERT INTO reviewer_cohorts (pr_key, profile, posting_identity, adapter, created_at, updated_at) +VALUES (?, ?, ?, ?, ?, ?) +ON CONFLICT(pr_key, profile, posting_identity) DO UPDATE SET + adapter = excluded.adapter, + created_at = excluded.created_at, + updated_at = excluded.updated_at`, + cohort.Scope.PRKey, cohort.Scope.Profile, cohort.Scope.PostingIdentity, cohort.Adapter, + encodeTime(cohort.CreatedAt), encodeTime(updatedAt)); err != nil { + return fmt.Errorf("ledger: replace reviewer cohort: %w", err) + } + if _, err := tx.ExecContext(ctx, `DELETE FROM reviewer_cohort_members WHERE pr_key = ? AND profile = ? AND posting_identity = ?`, + cohort.Scope.PRKey, cohort.Scope.Profile, cohort.Scope.PostingIdentity); err != nil { + return fmt.Errorf("ledger: clear reviewer cohort members: %w", err) + } + for position, member := range cohort.Members { + files, _ := json.Marshal(member.Files) + allowedFiles, _ := json.Marshal(member.AllowedFiles) + if _, err := tx.ExecContext(ctx, ` +INSERT INTO reviewer_cohort_members ( + pr_key, profile, posting_identity, position, agent_id, assignment_mode, + files_json, allowed_files_json, model, effort, fast, provider_session_id +) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + cohort.Scope.PRKey, cohort.Scope.Profile, cohort.Scope.PostingIdentity, position, member.AgentID, + string(member.AssignmentMode), string(files), string(allowedFiles), member.Model, member.Effort, + boolToInt(member.Fast), member.ProviderSessionID); err != nil { + return fmt.Errorf("ledger: insert reviewer cohort member %q: %w", member.AgentID, err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("ledger: commit replace reviewer cohort: %w", err) + } + return nil + }) +} + +// GetReviewerCohort returns one cohort in its persisted order. +func (s *Store) GetReviewerCohort(ctx context.Context, scope ReviewerCohortScope) (ReviewerCohort, error) { + if err := validateReviewerCohortScope(scope); err != nil { + return ReviewerCohort{}, err + } + if err := s.checkOpen(); err != nil { + return ReviewerCohort{}, err + } + cohort := ReviewerCohort{Scope: scope} + var createdAt, updatedAt string + err := s.db.QueryRowContext(ctx, ` +SELECT adapter, created_at, updated_at FROM reviewer_cohorts +WHERE pr_key = ? AND profile = ? AND posting_identity = ?`, scope.PRKey, scope.Profile, scope.PostingIdentity). + Scan(&cohort.Adapter, &createdAt, &updatedAt) + if errors.Is(err, sql.ErrNoRows) { + return ReviewerCohort{}, ErrNotFound + } + if err != nil { + return ReviewerCohort{}, fmt.Errorf("ledger: get reviewer cohort: %w", err) + } + if cohort.CreatedAt, err = parseTime(createdAt); err != nil { + return ReviewerCohort{}, err + } + if cohort.UpdatedAt, err = parseTime(updatedAt); err != nil { + return ReviewerCohort{}, err + } + rows, err := s.db.QueryContext(ctx, ` +SELECT agent_id, assignment_mode, files_json, allowed_files_json, model, effort, fast, provider_session_id +FROM reviewer_cohort_members +WHERE pr_key = ? AND profile = ? AND posting_identity = ? +ORDER BY position`, scope.PRKey, scope.Profile, scope.PostingIdentity) + if err != nil { + return ReviewerCohort{}, fmt.Errorf("ledger: list reviewer cohort members: %w", err) + } + defer rows.Close() + for rows.Next() { + var member ReviewerCohortMember + var mode, files, allowedFiles string + var fast int + if err := rows.Scan(&member.AgentID, &mode, &files, &allowedFiles, &member.Model, &member.Effort, &fast, &member.ProviderSessionID); err != nil { + return ReviewerCohort{}, fmt.Errorf("ledger: scan reviewer cohort member: %w", err) + } + member.AssignmentMode = ReviewerAssignmentMode(mode) + member.Fast = fast != 0 + if err := json.Unmarshal([]byte(files), &member.Files); err != nil { + return ReviewerCohort{}, fmt.Errorf("ledger: decode reviewer cohort files: %w", err) + } + if err := json.Unmarshal([]byte(allowedFiles), &member.AllowedFiles); err != nil { + return ReviewerCohort{}, fmt.Errorf("ledger: decode reviewer cohort allowed files: %w", err) + } + cohort.Members = append(cohort.Members, member) + } + if err := rows.Err(); err != nil { + return ReviewerCohort{}, fmt.Errorf("ledger: list reviewer cohort members rows: %w", err) + } + return cohort, nil +} + +// UpdateReviewerCohortSession records the latest provider session for one member. +func (s *Store) UpdateReviewerCohortSession(ctx context.Context, scope ReviewerCohortScope, agentID, providerSessionID string, updatedAt time.Time) error { + if err := validateReviewerCohortScope(scope); err != nil { + return err + } + if strings.TrimSpace(agentID) == "" || strings.TrimSpace(providerSessionID) == "" || updatedAt.IsZero() { + return ErrInvalidInput + } + return s.write(ctx, func(ctx context.Context, db *sql.DB) error { + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("ledger: begin update reviewer cohort session: %w", err) + } + defer func() { _ = tx.Rollback() }() + result, err := tx.ExecContext(ctx, ` +UPDATE reviewer_cohort_members SET provider_session_id = ? +WHERE pr_key = ? AND profile = ? AND posting_identity = ? AND agent_id = ?`, + providerSessionID, scope.PRKey, scope.Profile, scope.PostingIdentity, agentID) + if err != nil { + return fmt.Errorf("ledger: update reviewer cohort session: %w", err) + } + affected, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("ledger: update reviewer cohort session rows: %w", err) + } + if affected == 0 { + return ErrNotFound + } + if _, err := tx.ExecContext(ctx, ` +UPDATE reviewer_cohorts SET updated_at = ? +WHERE pr_key = ? AND profile = ? AND posting_identity = ?`, encodeTime(updatedAt), scope.PRKey, scope.Profile, scope.PostingIdentity); err != nil { + return fmt.Errorf("ledger: update reviewer cohort timestamp: %w", err) + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("ledger: commit reviewer cohort session: %w", err) + } + return nil + }) +} + +// DeleteReviewerCohort removes one cohort and its members. +func (s *Store) DeleteReviewerCohort(ctx context.Context, scope ReviewerCohortScope) error { + if err := validateReviewerCohortScope(scope); err != nil { + return err + } + return s.write(ctx, func(ctx context.Context, db *sql.DB) error { + _, err := db.ExecContext(ctx, `DELETE FROM reviewer_cohorts WHERE pr_key = ? AND profile = ? AND posting_identity = ?`, scope.PRKey, scope.Profile, scope.PostingIdentity) + if err != nil { + return fmt.Errorf("ledger: delete reviewer cohort: %w", err) + } + return nil + }) +} + // GetNamedSession returns a named provider session row. func (s *Store) GetNamedSession(ctx context.Context, name string) (NamedSession, error) { if strings.TrimSpace(name) == "" { @@ -1351,6 +1621,40 @@ func validateNamedSession(session NamedSession) error { return nil } +func validateReviewerCohortScope(scope ReviewerCohortScope) error { + for field, value := range map[string]string{ + "pr_key": scope.PRKey, "profile": scope.Profile, "posting_identity": scope.PostingIdentity, + } { + if strings.TrimSpace(value) == "" { + return invalidInput(field, value) + } + } + return nil +} + +func validateReviewerCohort(cohort ReviewerCohort) error { + if err := validateReviewerCohortScope(cohort.Scope); err != nil { + return err + } + if strings.TrimSpace(cohort.Adapter) == "" { + return invalidInput("adapter", cohort.Adapter) + } + if cohort.CreatedAt.IsZero() || len(cohort.Members) == 0 { + return ErrInvalidInput + } + seen := map[string]bool{} + for _, member := range cohort.Members { + if strings.TrimSpace(member.AgentID) == "" || strings.TrimSpace(member.Model) == "" || seen[member.AgentID] { + return ErrInvalidInput + } + seen[member.AgentID] = true + if member.AssignmentMode != ReviewerAssignmentBroad && member.AssignmentMode != ReviewerAssignmentScoped { + return invalidInput("assignment_mode", string(member.AssignmentMode)) + } + } + return nil +} + func boolToInt(value bool) int { if value { return 1 @@ -1746,3 +2050,33 @@ var schemaStatements = []string{ last_used_at TEXT NOT NULL )`, } + +var reviewerCohortSchemaStatements = []string{ + `CREATE TABLE reviewer_cohorts ( + pr_key TEXT NOT NULL REFERENCES prs(pr_key) ON DELETE CASCADE, + profile TEXT NOT NULL, + posting_identity TEXT NOT NULL, + adapter TEXT NOT NULL, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + PRIMARY KEY(pr_key, profile, posting_identity) +)`, + `CREATE TABLE reviewer_cohort_members ( + pr_key TEXT NOT NULL, + profile TEXT NOT NULL, + posting_identity TEXT NOT NULL, + position INTEGER NOT NULL, + agent_id TEXT NOT NULL, + assignment_mode TEXT NOT NULL, + files_json TEXT NOT NULL, + allowed_files_json TEXT NOT NULL, + model TEXT NOT NULL, + effort TEXT NOT NULL, + fast INTEGER NOT NULL DEFAULT 0, + provider_session_id TEXT NOT NULL DEFAULT '', + PRIMARY KEY(pr_key, profile, posting_identity, agent_id), + UNIQUE(pr_key, profile, posting_identity, position), + FOREIGN KEY(pr_key, profile, posting_identity) + REFERENCES reviewer_cohorts(pr_key, profile, posting_identity) ON DELETE CASCADE +)`, +} diff --git a/internal/ledger/ledger_test.go b/internal/ledger/ledger_test.go index 4dae2ca..8fe9620 100644 --- a/internal/ledger/ledger_test.go +++ b/internal/ledger/ledger_test.go @@ -33,7 +33,7 @@ func TestOpenMigratesFreshDatabaseAndAppliesStartupContract(t *testing.T) { t.Fatalf("PRAGMA busy_timeout = %d, want %d", got, DefaultBusyTimeout.Milliseconds()) } - for _, table := range []string{"prs", "runs", "sessions", "findings", "planned_actions", "named_sessions"} { + for _, table := range []string{"prs", "runs", "sessions", "findings", "planned_actions", "named_sessions", "reviewer_cohorts", "reviewer_cohort_members"} { if !tableExists(t, store.db, table) { t.Fatalf("table %s does not exist", table) } @@ -52,6 +52,124 @@ func TestOpenMigratesFreshDatabaseAndAppliesStartupContract(t *testing.T) { } } +func TestReviewerCohortReplaceAndSessionUpdateAreAtomic(t *testing.T) { + store := openStore(t) + ctx := context.Background() + run := allocateRun(t, store, validAllocateRunParams()) + scope := ReviewerCohortScope{PRKey: run.PRKey, Profile: run.Profile, PostingIdentity: run.PostingIdentity} + first := ReviewerCohort{ + Scope: scope, Adapter: "codex_cli", CreatedAt: time.Date(2026, 7, 31, 10, 0, 0, 0, time.UTC), + Members: []ReviewerCohortMember{ + {AgentID: "repo:go", AssignmentMode: ReviewerAssignmentScoped, Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}, Model: "gpt-5.5", Effort: "high", ProviderSessionID: "go-1"}, + {AgentID: "shared:security", AssignmentMode: ReviewerAssignmentBroad, Model: "gpt-5.5", Effort: "high", ProviderSessionID: "security-1"}, + }, + } + if err := store.ReplaceReviewerCohort(ctx, first); err != nil { + t.Fatalf("ReplaceReviewerCohort first: %v", err) + } + + second := first + second.Members = []ReviewerCohortMember{{AgentID: "repo:docs", AssignmentMode: ReviewerAssignmentScoped, Files: []string{"README.md"}, AllowedFiles: []string{"README.md"}, Model: "gpt-5.5", Effort: "medium"}} + if err := store.ReplaceReviewerCohort(ctx, second); err != nil { + t.Fatalf("ReplaceReviewerCohort second: %v", err) + } + if err := store.UpdateReviewerCohortSession(ctx, scope, "repo:docs", "docs-2", time.Date(2026, 7, 31, 10, 5, 0, 0, time.UTC)); err != nil { + t.Fatalf("UpdateReviewerCohortSession: %v", err) + } + + got, err := store.GetReviewerCohort(ctx, scope) + if err != nil { + t.Fatalf("GetReviewerCohort: %v", err) + } + second.Members[0].ProviderSessionID = "docs-2" + second.UpdatedAt = time.Date(2026, 7, 31, 10, 5, 0, 0, time.UTC) + if !reflect.DeepEqual(got, second) { + t.Fatalf("GetReviewerCohort = %#v, want %#v", got, second) + } + if count := queryInt(t, store.db, "SELECT COUNT(*) FROM reviewer_cohort_members"); count != 1 { + t.Fatalf("reviewer cohort member count = %d, want atomic replacement count 1", count) + } +} + +func TestReviewerCohortConcurrentSessionUpdatesDoNotClobberSiblings(t *testing.T) { + store := openStore(t) + ctx := context.Background() + run := allocateRun(t, store, validAllocateRunParams()) + scope := ReviewerCohortScope{PRKey: run.PRKey, Profile: run.Profile, PostingIdentity: run.PostingIdentity} + cohort := ReviewerCohort{Scope: scope, Adapter: "codex_cli", CreatedAt: time.Date(2026, 7, 31, 10, 0, 0, 0, time.UTC), Members: []ReviewerCohortMember{ + {AgentID: "repo:go", AssignmentMode: ReviewerAssignmentBroad, Model: "gpt-5.5", Effort: "high"}, + {AgentID: "repo:test", AssignmentMode: ReviewerAssignmentBroad, Model: "gpt-5.5", Effort: "high"}, + }} + if err := store.ReplaceReviewerCohort(ctx, cohort); err != nil { + t.Fatalf("ReplaceReviewerCohort: %v", err) + } + + var wg sync.WaitGroup + for agent, session := range map[string]string{"repo:go": "go-session", "repo:test": "test-session"} { + wg.Add(1) + go func() { + defer wg.Done() + if err := store.UpdateReviewerCohortSession(ctx, scope, agent, session, time.Date(2026, 7, 31, 10, 5, 0, 0, time.UTC)); err != nil { + t.Errorf("UpdateReviewerCohortSession(%s): %v", agent, err) + } + }() + } + wg.Wait() + + got, err := store.GetReviewerCohort(ctx, scope) + if err != nil { + t.Fatalf("GetReviewerCohort: %v", err) + } + if got.Members[0].ProviderSessionID != "go-session" || got.Members[1].ProviderSessionID != "test-session" { + t.Fatalf("reviewer sessions = %#v, want both concurrent updates", got.Members) + } +} + +func TestMergePlanningResultPreservesCheckpointActionState(t *testing.T) { + store := openStore(t) + run := allocateRun(t, store, validAllocateRunParams()) + session := validSession(run.RunID) + insertSession(t, store, session) + finding := validFinding(run.RunID, session.SessionRowID) + insertFinding(t, store, finding) + checkpoint := validPlannedAction(run.RunID) + checkpoint.ActionID = "thread-reply" + checkpoint.Kind = PlannedActionThreadReply + checkpoint.FindingID = "" + checkpoint.ThreadID = "thread-1" + checkpoint.RollupComment = nil + checkpoint.ThreadReply = &plannedactions.ThreadReplyPayload{Body: "summary", Summary: true} + checkpoint.Status = PlannedActionPosted + checkpoint.Attempts = 1 + postedAt := time.Date(2026, 7, 31, 11, 0, 0, 0, time.UTC) + checkpoint.PostedAt = &postedAt + checkpoint.UpstreamID = strPtr("comment-1") + if err := store.InsertPlannedAction(context.Background(), checkpoint); err != nil { + t.Fatalf("InsertPlannedAction: %v", err) + } + final := validPlannedAction(run.RunID) + final.ActionID = "submit-review" + final.Kind = PlannedActionSubmitReview + final.FindingID = "" + final.RollupComment = nil + final.SubmitReview = &plannedactions.SubmitReviewPayload{Body: "review", Event: review.ReviewEventComment} + + if err := store.MergePlanningResult(context.Background(), []Finding{finding}, []PlannedAction{checkpoint, final}); err != nil { + t.Fatalf("MergePlanningResult: %v", err) + } + findings, err := store.ListFindings(context.Background(), run.RunID) + if err != nil || len(findings) != 1 { + t.Fatalf("merged findings = %#v, err = %v, want one unchanged finding", findings, err) + } + actions, err := store.ListPlannedActions(context.Background(), run.RunID) + if err != nil { + t.Fatalf("ListPlannedActions: %v", err) + } + if len(actions) != 2 || actions[0].ActionID != "submit-review" || actions[1].Status != PlannedActionPosted || actions[1].Attempts != 1 { + t.Fatalf("merged actions = %#v, want final action plus unchanged posted checkpoint", actions) + } +} + func TestOpenMigratesVersion1LedgerAndPreservesPlannedActions(t *testing.T) { path := filepath.Join(t.TempDir(), "ledger.db") plannedAt := time.Date(2026, 5, 30, 12, 2, 0, 0, time.UTC) diff --git a/internal/outbox/outbox.go b/internal/outbox/outbox.go index d6fb5f2..04a4505 100644 --- a/internal/outbox/outbox.go +++ b/internal/outbox/outbox.go @@ -302,6 +302,114 @@ func Post(ctx context.Context, opts Options, req Request) (Result, error) { return summarize(actions, outcome, exitCode, false), nil } +// PostCheckpoint posts only persisted thread replies/resolutions without +// completing the run. Provider failures remain pending for the final outbox. +func PostCheckpoint(ctx context.Context, opts Options, req Request) (Result, error) { + if opts.Now == nil { + opts.Now = time.Now + } + if err := validateRequest(opts, req); err != nil { + return Result{ExitCode: exitFailed}, err + } + live, ok := opts.Provider.(interface { + GetPR(context.Context, gitprovider.PRRef) (gitprovider.PR, error) + }) + if !ok { + return Result{ExitCode: exitFailed}, fmt.Errorf("outbox: checkpoint provider must read pull requests") + } + pr, err := live.GetPR(ctx, req.PRRef) + if err != nil { + return Result{ExitCode: exitUpstream}, err + } + if pr.Head.SHA != req.Run.SHA || pr.Base.SHA != req.Run.BaseSHA { + return Result{Outcome: ledger.OutcomeIncomplete, ExitCode: exitUpstream, Aborted: true}, nil + } + actions, err := opts.Store.ListPlannedActions(ctx, req.Run.RunID) + if err != nil { + return Result{ExitCode: exitFailed}, err + } + actions = sortActions(actions) + state, err := readHostState(ctx, opts.Provider, req.PRRef) + if err != nil { + return Result{ExitCode: exitUpstream}, err + } + for i := range actions { + if actions[i].Status != ledger.PlannedActionPending || !checkpointAction(actions[i]) { + continue + } + match, matchErr := reconcileAction(req, opts.Provider.Capabilities(), actions, actions[i], state) + if matchErr != nil || !match.ok { + continue + } + now := opts.Now().UTC() + actions[i].Status = ledger.PlannedActionPosted + actions[i].PostedAt = &now + actions[i].UpstreamID = strPtr(match.upstreamID) + actions[i].Error = nil + actions[i].FailureClass = nil + if err := opts.Store.UpdatePlannedAction(ctx, actions[i]); err != nil { + return Result{ExitCode: exitFailed}, err + } + } + for i := range actions { + if actions[i].Status != ledger.PlannedActionPending || !checkpointAction(actions[i]) { + continue + } + if actions[i].Kind == ledger.PlannedActionResolveThread && !sameThreadReplyPosted(actions[i], actions) { + continue + } + plan, _, planErr := buildActionPlan(opts.Provider, req, actions, actions[i]) + if planErr != nil { + if err := recordPendingError(ctx, opts.Store, &actions[i], planErr); err != nil { + return Result{ExitCode: exitFailed}, err + } + continue + } + if err := opts.Limiter.Wait(ctx, req.PRRef.Host); err != nil { + if updateErr := recordPendingError(ctx, opts.Store, &actions[i], err); updateErr != nil { + return Result{ExitCode: exitFailed}, updateErr + } + if isContextError(err) { + return checkpointResult(actions), err + } + continue + } + now := opts.Now().UTC() + actions[i].Attempts++ + actions[i].AttemptedAt = &now + if err := opts.Store.UpdatePlannedAction(ctx, actions[i]); err != nil { + return Result{ExitCode: exitFailed}, err + } + upstreamID, postErr := dispatch(ctx, opts.Provider, req.PRRef, plan) + if postErr != nil { + if err := recordPendingError(ctx, opts.Store, &actions[i], postErr); err != nil { + return Result{ExitCode: exitFailed}, err + } + continue + } + actions[i].Status = ledger.PlannedActionPosted + actions[i].PostedAt = &now + actions[i].UpstreamID = strPtr(upstreamID) + actions[i].Error = nil + actions[i].FailureClass = nil + if err := opts.Store.UpdatePlannedAction(ctx, actions[i]); err != nil { + return Result{ExitCode: exitFailed}, err + } + } + return checkpointResult(actions), nil +} + +func checkpointAction(action ledger.PlannedAction) bool { + return action.Kind == ledger.PlannedActionThreadReply || action.Kind == ledger.PlannedActionResolveThread +} + +func checkpointResult(actions []ledger.PlannedAction) Result { + result := summarize(actions, ledger.OutcomeIncomplete, exitOK, false) + result.Outcome = ledger.OutcomeIncomplete + result.ExitCode = exitOK + return result +} + func validateRequest(opts Options, req Request) error { if opts.Store == nil { return fmt.Errorf("outbox: store is required") diff --git a/internal/outbox/outbox_test.go b/internal/outbox/outbox_test.go index 9fa430b..7d83915 100644 --- a/internal/outbox/outbox_test.go +++ b/internal/outbox/outbox_test.go @@ -142,6 +142,60 @@ func TestPostEmbedsMarkersAndPostsInCanonicalOrder(t *testing.T) { } } +func TestPostCheckpointVerifiesPremisesPostsReplyBeforeResolveAndLeavesRunOpen(t *testing.T) { + store := openStore(t) + run := allocateRun(t, store, ledger.PostModeLive) + provider := newRecordingProvider() + ref := testPRRef() + if err := provider.SetPR(ref, gitprovider.PR{Ref: ref, Head: gitprovider.PRBranchRef{SHA: run.SHA}, Base: gitprovider.PRBranchRef{SHA: run.BaseSHA}}); err != nil { + t.Fatalf("SetPR: %v", err) + } + insertAction(t, store, plannedAction(run.RunID, "resolve-1", ledger.PlannedActionResolveThread, true, "thread-1", ResolveThreadPayload{})) + insertAction(t, store, plannedAction(run.RunID, "reply-1", ledger.PlannedActionThreadReply, true, "thread-1", ThreadReplyPayload{Body: "summary", Summary: true})) + + result, err := PostCheckpoint(context.Background(), Options{Store: store, Provider: provider, Limiter: noopLimiter{}, Now: fixedClock()}, testRequest(run)) + if err != nil { + t.Fatalf("PostCheckpoint: %v", err) + } + if !reflect.DeepEqual(provider.writes, []string{"ReplyToThread", "ResolveThread"}) { + t.Fatalf("checkpoint writes = %#v, want reply then resolve", provider.writes) + } + if result.Posted != 2 || result.Outcome != ledger.OutcomeIncomplete { + t.Fatalf("PostCheckpoint result = %#v, want two posted and incomplete", result) + } + stored, err := store.GetRun(context.Background(), run.RunID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if stored.CompletedAt != nil || stored.Outcome != nil { + t.Fatalf("checkpoint completed run = %#v, want open run", stored) + } +} + +func TestPostCheckpointPreservesPostingFailureAsPending(t *testing.T) { + store := openStore(t) + run := allocateRun(t, store, ledger.PostModeLive) + provider := newRecordingProvider() + ref := testPRRef() + if err := provider.SetPR(ref, gitprovider.PR{Ref: ref, Head: gitprovider.PRBranchRef{SHA: run.SHA}, Base: gitprovider.PRBranchRef{SHA: run.BaseSHA}}); err != nil { + t.Fatalf("SetPR: %v", err) + } + provider.SetError(gitprovider.OperationReplyToThread, errors.New("temporary posting failure")) + insertAction(t, store, plannedAction(run.RunID, "reply-1", ledger.PlannedActionThreadReply, true, "thread-1", ThreadReplyPayload{Body: "summary", Summary: true})) + + result, err := PostCheckpoint(context.Background(), Options{Store: store, Provider: provider, Limiter: noopLimiter{}, Now: fixedClock()}, testRequest(run)) + if err != nil { + t.Fatalf("PostCheckpoint: %v", err) + } + action := actionByID(t, store, run.RunID, "reply-1") + if action.Status != ledger.PlannedActionPending || action.Attempts != 1 || action.Error == nil { + t.Fatalf("checkpoint action = %#v, want pending attempted failure", action) + } + if result.Pending != 1 || result.Outcome != ledger.OutcomeIncomplete { + t.Fatalf("PostCheckpoint result = %#v, want pending incomplete", result) + } +} + func TestPostTreatsGitHubAppResolveThreadLimitationAsAdvisory(t *testing.T) { store := openStore(t) run := allocateRun(t, store, ledger.PostModeLive) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 1971d26..108bc44 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "sort" "strings" "sync" @@ -79,10 +80,18 @@ type Store interface { CompleteRun(context.Context, string, ledger.Outcome, time.Time) error } +type reviewerCohortStore interface { + GetReviewerCohort(context.Context, ledger.ReviewerCohortScope) (ledger.ReviewerCohort, error) + ReplaceReviewerCohort(context.Context, ledger.ReviewerCohort) error + UpdateReviewerCohortSession(context.Context, ledger.ReviewerCohortScope, string, string, time.Time) error + DeleteReviewerCohort(context.Context, ledger.ReviewerCohortScope) error +} + // NamedSessionStore persists cross-run LLM sessions. type NamedSessionStore interface { GetNamedSession(context.Context, string) (ledger.NamedSession, error) UpsertNamedSession(context.Context, ledger.NamedSession) error + DeleteNamedSession(context.Context, string) error } // LLMTaskProgress records task-aware LLM pipeline breadcrumbs without owning @@ -141,14 +150,15 @@ type ContextBudget struct { // Options contains dry-run pipeline dependencies. type Options struct { - Provider ReadProvider - Adapter llm.Adapter - Store Store - NamedSessions NamedSessionStore - Layout statepaths.Layout - Warnings io.Writer - TaskProgress LLMTaskProgress - ReviewProgress ReviewerProgress + Provider ReadProvider + Adapter llm.Adapter + Store Store + NamedSessions NamedSessionStore + Layout statepaths.Layout + Warnings io.Writer + TaskProgress LLMTaskProgress + ReviewProgress ReviewerProgress + ThreadCheckpoint func(context.Context, ledger.Run, Request) error Now func() time.Time NewRunID func() string @@ -348,6 +358,7 @@ type namedSessionState struct { supportsResume bool currentProviderSessionID string createdAt time.Time + store NamedSessionStore } type selectionSetupRequest struct { @@ -750,35 +761,73 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu return nil, false, err } - selection, selectionSession, selectionLedgerSession, err := runSelectionPhase(ctx, opts, selectionPhaseRequest{ - RunID: run.RunID, - DurableSession: namedSession.enabled && namedSession.supportsResume, - Profile: req.Profile, - SelectionModelOverride: req.SelectionModelOverride, - SelectionEffortOverride: req.SelectionEffortOverride, - SelectionPromptInstructions: req.SelectionPromptInstructions, - ReviewPR: prepared.reviewPR, - Catalog: prepared.catalog, - ParsedDiff: prepared.parsed, - Threads: prepared.threads, - ThreadContext: prepared.threadContext, - Artifacts: prepared.artifacts, - ResumeSessionID: namedSession.resumeID(), - MaxAgents: maxAgents, - }) + cohortScope := ledger.ReviewerCohortScope{PRKey: prepared.prKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)} + selection, reviewerResumeIDs, reusedCohort, err := loadReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, prepared.changedFiles, maxAgents) if err != nil { return executionPhaseFailure(err) } + if reusedCohort { + if err := restoreOrchestratorSessionFromRun(ctx, opts.Store, run.RunID, &namedSession); err != nil { + return nil, false, err + } + } + selectionInRun := false + if reusedCohort { + if _, selectionInRun, err = llmlifecycle.ReadMetadata(lifecyclePaths(prepared.artifacts), orchestratorSelectionStage); err != nil { + return nil, false, err + } + } + var selectionSession sessionDraft + var selectionLedgerSession ledger.Session + if !reusedCohort || selectionInRun { + selection, selectionSession, selectionLedgerSession, err = runSelectionPhase(ctx, opts, selectionPhaseRequest{ + RunID: run.RunID, + DurableSession: namedSession.enabled && namedSession.supportsResume, + Profile: req.Profile, + SelectionModelOverride: req.SelectionModelOverride, + SelectionEffortOverride: req.SelectionEffortOverride, + SelectionPromptInstructions: req.SelectionPromptInstructions, + ReviewPR: prepared.reviewPR, + Catalog: prepared.catalog, + ParsedDiff: prepared.parsed, + Threads: prepared.threads, + ThreadContext: prepared.threadContext, + Artifacts: prepared.artifacts, + ResumeSessionID: namedSession.resumeID(), + MaxAgents: maxAgents, + }) + if checkpointErr := namedSession.checkpointSessionID(ctx, selectionSession, opts.now()); checkpointErr != nil { + return nil, false, checkpointErr + } + if err != nil { + return executionPhaseFailure(err) + } + if !reusedCohort { + if err := persistReviewerCohort(ctx, opts, req, cohortScope, prepared.catalog, selection, now); err != nil { + return nil, false, err + } + } + } else { + opts.emitReviewerSelection(prepared.catalog, selection) + } result.Selection = selection result.Sessions = appendSessionIfPresent(result.Sessions, selectionLedgerSession) - namedSession.recordSessionID(selectionSession) selectionTaskIDs := []string{orchestratorSelectionStage} - threadResponses, err := analyzeReviewThreads(ctx, opts, req, run, prepared.artifacts, prepared.threadContext) + if reusedCohort && !selectionInRun { + selectionTaskIDs = nil + } + threadResponses, err := analyzeReviewThreads(ctx, opts, req, run, prepared.artifacts, prepared.threadContext, namedSession.resumeID(), func(sessionID string) error { + return namedSession.checkpointProviderSessionID(ctx, sessionID, opts.now()) + }) if err != nil { return executionPhaseFailure(err) } - findings, reviewerResults, reviewerSessions, reviewerLedgerSessions, findingSessions, reviewerFailures, err := runReviewers(ctx, opts, req, run.RunID, prepared.reviewPR, prepared.catalog, prepared.parsed, prepared.artifacts, selection, selectionTaskIDs, maxConcurrency) + checkpointActions, err := checkpointThreadResponses(ctx, opts, req, mode, run, result.EffectiveCaps, threadResponses) + if err != nil { + return nil, false, err + } + findings, reviewerResults, reviewerSessions, reviewerLedgerSessions, findingSessions, reviewerFailures, err := runReviewers(ctx, opts, req, run.RunID, prepared.reviewPR, prepared.catalog, prepared.parsed, prepared.artifacts, selection, selectionTaskIDs, maxConcurrency, cohortScope, reviewerResumeIDs, reviewerDiscussionCheckpoint{responses: threadResponses, actions: checkpointActions}) if err != nil { return executionPhaseFailure(err) } @@ -827,6 +876,9 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu MajorEventRequestsChanges: req.MajorRequestChanges, }) }) + if checkpointErr := namedSession.checkpointSessionID(ctx, rollupSession, opts.now()); checkpointErr != nil { + return nil, false, checkpointErr + } if err != nil { return executionPhaseFailure(err) } @@ -853,6 +905,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu if err != nil { return nil, false, err } + plan.Actions = preserveCheckpointActions(plan.Actions, checkpointActions) result.Plan = plan return findingSessions, false, nil } @@ -874,12 +927,22 @@ func persistExecutionResult(ctx context.Context, opts Options, req Request, run for _, action := range result.Plan.Actions { plannedActions = append(plannedActions, ledger.PlannedAction{Action: action.Action, RunID: run.RunID}) } - _, existingActions, hasPersistedPlanning, err := persistedPlanning(ctx, opts.Store, run.RunID) + _, _, hasPersistedPlanning, err := persistedPlanning(ctx, opts.Store, run.RunID) if err != nil { return err } if hasPersistedPlanning { - plannedActions = existingActions + store, ok := opts.Store.(checkpointPlanningStore) + if !ok { + return fmt.Errorf("pipeline: checkpoint planning store is required") + } + if err := store.MergePlanningResult(ctx, ledgerFindings, plannedActions); err != nil { + return err + } + plannedActions, err = opts.Store.ListPlannedActions(ctx, run.RunID) + if err != nil { + return err + } } else if err := opts.Store.InsertPlanningResult(ctx, ledgerFindings, plannedActions); err != nil { return err } @@ -1223,7 +1286,7 @@ func runSelectionPhase(ctx context.Context, opts Options, req selectionPhaseRequ return selection, selectionSession, ledgerSession, nil } -func analyzeReviewThreads(ctx context.Context, opts Options, req Request, run ledger.Run, artifacts ArtifactPaths, threads []threadcontext.Thread) ([]review.ThreadResponseAction, error) { +func analyzeReviewThreads(ctx context.Context, opts Options, req Request, run ledger.Run, artifacts ArtifactPaths, threads []threadcontext.Thread, resumeSessionID string, onSessionID func(string) error) ([]review.ThreadResponseAction, error) { eligible := threadcontext.PendingCRAuthoredFindingThreads(threads) if len(eligible) == 0 { return nil, nil @@ -1233,15 +1296,17 @@ func analyzeReviewThreads(ctx context.Context, opts Options, req Request, run le return nil, err } results, err := threadanalysis.AnalyzeThreads(ctx, threadanalysis.Options{ - Store: opts.Store, - RunID: run.RunID, - Adapter: opts.Adapter, - Model: runtimeConfig.model, - Effort: runtimeConfig.effort, - LifecyclePaths: lifecyclePaths(artifacts), - Progress: opts.TaskProgress, - Now: opts.now, - NewStepID: opts.newSessionRowID, + Store: opts.Store, + RunID: run.RunID, + Adapter: opts.Adapter, + Model: runtimeConfig.model, + Effort: runtimeConfig.effort, + LifecyclePaths: lifecyclePaths(artifacts), + Progress: opts.TaskProgress, + Now: opts.now, + NewStepID: opts.newSessionRowID, + ResumeSessionID: resumeSessionID, + OnSessionID: onSessionID, }, eligible, func(thread threadcontext.Thread) (string, error) { return artifacts.AgentLog("thread-analysis-" + string(thread.ID)) }) @@ -1251,6 +1316,92 @@ func analyzeReviewThreads(ctx context.Context, opts Options, req Request, run le return threadanalysis.ResponseActions(results), nil } +type checkpointPlanningStore interface { + InsertPlannedActions(context.Context, []ledger.PlannedAction) error + MergePlanningResult(context.Context, []ledger.Finding, []ledger.PlannedAction) error +} + +func checkpointThreadResponses(ctx context.Context, opts Options, req Request, mode executionMode, run ledger.Run, caps reviewplan.ProviderCaps, responses []review.ThreadResponseAction) ([]ledger.PlannedAction, error) { + if len(responses) == 0 { + return nil, nil + } + existing, err := opts.Store.ListPlannedActions(ctx, run.RunID) + if err != nil { + return nil, err + } + actions := checkpointActionsOnly(existing) + if len(actions) == 0 { + plan, err := reviewplan.BuildThreadResponses(reviewplan.ThreadResponseRequest{ + PostMode: mode.planPostMode, + ProviderCaps: caps, + Responses: responses, + Now: opts.now, + NewActionID: opts.newActionID, + }) + if err != nil { + return nil, err + } + actions = make([]ledger.PlannedAction, 0, len(plan.Actions)) + for _, action := range plan.Actions { + actions = append(actions, ledger.PlannedAction{Action: action.Action, RunID: run.RunID}) + } + store, ok := opts.Store.(checkpointPlanningStore) + if !ok { + return nil, fmt.Errorf("pipeline: checkpoint planning store is required") + } + if err := store.InsertPlannedActions(ctx, actions); err != nil { + return nil, err + } + } + if mode.live && opts.ThreadCheckpoint != nil { + if err := opts.ThreadCheckpoint(ctx, run, req); err != nil { + if errors.Is(err, gitprovider.ErrStaleSHA) { + return nil, err + } + opts.emitWarning(fmt.Sprintf("thread response checkpoint posting failed; final posting will retry: %v", err)) + } + } + stored, err := opts.Store.ListPlannedActions(ctx, run.RunID) + if err != nil { + return nil, err + } + return checkpointActionsOnly(stored), nil +} + +func checkpointActionsOnly(actions []ledger.PlannedAction) []ledger.PlannedAction { + out := make([]ledger.PlannedAction, 0, len(actions)) + for _, action := range actions { + if action.Kind == ledger.PlannedActionThreadReply || action.Kind == ledger.PlannedActionResolveThread { + out = append(out, action) + } + } + return out +} + +func preserveCheckpointActions(actions []reviewplan.Action, checkpoint []ledger.PlannedAction) []reviewplan.Action { + if len(checkpoint) == 0 { + return actions + } + byKindThread := map[string][]ledger.PlannedAction{} + for _, action := range checkpoint { + key := action.Kind.String() + "\x00" + action.ThreadID + byKindThread[key] = append(byKindThread[key], action) + } + for i := range actions { + if actions[i].Kind != reviewplan.ActionKindThreadReply && actions[i].Kind != reviewplan.ActionKindResolveThread { + continue + } + key := actions[i].Kind.String() + "\x00" + actions[i].ThreadID + matches := byKindThread[key] + if len(matches) == 0 { + continue + } + actions[i].Action = matches[0].Action + byKindThread[key] = matches[1:] + } + return actions +} + func (opts Options) capSelectionAgents(selection llm.Selection, catalog agents.Catalog, changedFiles []string, maxAgents int) (llm.Selection, error) { var required, shared []llm.SelectedAgent for _, selected := range selection.SelectedAgents { @@ -1366,6 +1517,182 @@ func ensureSelectedGlobCoverage(selection llm.Selection, catalog agents.Catalog, return selection } +func rebaseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.ReviewerCohort, changedFiles []string, maxAgents int, adapter string) (llm.Selection, map[string]string, error) { + freshError := func(format string, args ...any) (llm.Selection, map[string]string, error) { + return llm.Selection{}, nil, fmt.Errorf("pipeline: "+format+"; pass --fresh-session to select a new reviewer cohort", args...) + } + if cohort.Adapter != adapter { + return freshError("saved reviewer cohort adapter %q is incompatible with %q", cohort.Adapter, adapter) + } + if maxAgents > 0 && len(cohort.Members) > maxAgents { + return freshError("--max-agents %d is smaller than the saved reviewer cohort of %d", maxAgents, len(cohort.Members)) + } + + type candidate struct { + member ledger.ReviewerCohortMember + agent agents.Agent + files []string + } + candidates := make([]candidate, 0, len(cohort.Members)) + changed := stringSet(changedFiles) + covered := map[string]bool{} + for _, member := range cohort.Members { + agent, ok := catalog.Find(member.AgentID) + if !ok { + return freshError("saved reviewer %q is missing from the current catalog", member.AgentID) + } + runtimeConfig, err := resolveReviewerRuntimeConfig(req, agent) + if err != nil { + return llm.Selection{}, nil, err + } + if runtimeConfig.model != member.Model || runtimeConfig.effort != member.Effort || req.ReviewerFast != member.Fast { + return freshError("saved reviewer %q runtime is incompatible with the current runtime", member.AgentID) + } + current := candidate{member: member, agent: agent} + persistedFiles := member.Files + if member.AssignmentMode == ledger.ReviewerAssignmentScoped { + if len(member.AllowedFiles) > 0 { + persistedFiles = member.AllowedFiles + } + } + for _, file := range persistedFiles { + if changed[file] && !slices.Contains(current.files, file) { + current.files = append(current.files, file) + covered[file] = true + } + } + candidates = append(candidates, current) + } + for _, file := range changedFiles { + if covered[file] { + continue + } + assigned := false + for i := range candidates { + if candidates[i].member.AssignmentMode != ledger.ReviewerAssignmentBroad && !globsMatchFile(candidates[i].agent.FileGlobs, file) { + continue + } + candidates[i].files = append(candidates[i].files, file) + covered[file] = true + assigned = true + break + } + if !assigned { + return freshError("saved reviewer cohort leaves changed file %q uncovered", file) + } + } + + selection := llm.Selection{Reasoning: "reused reviewer cohort"} + resumes := map[string]string{} + for _, candidate := range candidates { + if len(candidate.files) == 0 { + continue + } + selected := llm.SelectedAgent{AgentID: candidate.member.AgentID, Rationale: "reused reviewer cohort"} + selected.Files = append([]string(nil), candidate.files...) + if candidate.member.AssignmentMode == ledger.ReviewerAssignmentScoped { + selected.AllowedFiles = append([]string(nil), candidate.files...) + } + selection.SelectedAgents = append(selection.SelectedAgents, selected) + if sessionID := strings.TrimSpace(candidate.member.ProviderSessionID); sessionID != "" { + resumes[candidate.member.AgentID] = sessionID + } + } + return selection, resumes, nil +} + +func loadReviewerCohort(ctx context.Context, opts Options, req Request, scope ledger.ReviewerCohortScope, catalog agents.Catalog, changedFiles []string, maxAgents int) (llm.Selection, map[string]string, bool, error) { + store, ok := opts.Store.(reviewerCohortStore) + if !ok { + return llm.Selection{}, nil, false, fmt.Errorf("pipeline: reviewer cohort store is required") + } + if req.FreshSession { + if err := store.DeleteReviewerCohort(ctx, scope); err != nil && !errors.Is(err, ledger.ErrNotFound) { + return llm.Selection{}, nil, false, err + } + return llm.Selection{}, nil, false, nil + } + cohort, err := store.GetReviewerCohort(ctx, scope) + if errors.Is(err, ledger.ErrNotFound) { + return llm.Selection{}, nil, false, nil + } + if err != nil { + return llm.Selection{}, nil, false, err + } + selection, resumes, err := rebaseReviewerCohort(req, catalog, cohort, changedFiles, maxAgents, opts.Adapter.Name()) + if err != nil { + return llm.Selection{}, nil, false, Failure(FailureTerminal, err) + } + if !opts.Adapter.SupportsResume() { + resumes = nil + } + return selection, resumes, true, nil +} + +func persistReviewerCohort(ctx context.Context, opts Options, req Request, scope ledger.ReviewerCohortScope, catalog agents.Catalog, selection llm.Selection, now time.Time) error { + if len(selection.SelectedAgents) == 0 { + return nil + } + store, ok := opts.Store.(reviewerCohortStore) + if !ok { + return fmt.Errorf("pipeline: reviewer cohort store is required") + } + cohort := ledger.ReviewerCohort{Scope: scope, Adapter: opts.Adapter.Name(), CreatedAt: now, UpdatedAt: now} + for _, selected := range selection.SelectedAgents { + agent, ok := catalog.Find(selected.AgentID) + if !ok { + return fmt.Errorf("pipeline: selected agent %q not found while saving reviewer cohort", selected.AgentID) + } + runtimeConfig, err := resolveReviewerRuntimeConfig(req, agent) + if err != nil { + return err + } + mode := ledger.ReviewerAssignmentScoped + if len(selected.Files) == 0 && len(selected.AllowedFiles) == 0 { + mode = ledger.ReviewerAssignmentBroad + } + cohort.Members = append(cohort.Members, ledger.ReviewerCohortMember{ + AgentID: selected.AgentID, + AssignmentMode: mode, + Files: append([]string(nil), selected.Files...), + AllowedFiles: append([]string(nil), selected.AllowedFiles...), + Model: runtimeConfig.model, + Effort: runtimeConfig.effort, + Fast: req.ReviewerFast, + }) + } + return store.ReplaceReviewerCohort(ctx, cohort) +} + +type runSessionStore interface { + ListSessionsForRun(context.Context, string) ([]ledger.Session, error) +} + +func restoreOrchestratorSessionFromRun(ctx context.Context, store Store, runID string, state *namedSessionState) error { + sessions, ok := store.(runSessionStore) + if !ok || state == nil || !state.supportsResume { + return nil + } + rows, err := sessions.ListSessionsForRun(ctx, runID) + if err != nil { + return err + } + var latest *ledger.Session + for i := range rows { + row := &rows[i] + if row.Role != ledger.SessionRoleOrchestrator || strings.TrimSpace(row.ProviderSessionID) == "" { + continue + } + if latest == nil || row.StartedAt.After(latest.StartedAt) { + latest = row + } + } + if latest != nil { + state.currentProviderSessionID = latest.ProviderSessionID + } + return nil +} + func (opts Options) emitReviewerCatalog(catalog agents.Catalog, changedFiles []string) { if opts.ReviewProgress == nil { return @@ -1461,7 +1788,7 @@ func appendSessionsIfPresent(sessions []ledger.Session, more ...ledger.Session) return sessions } -func runReviewers(ctx context.Context, opts Options, req Request, runID string, pr gitprovider.PR, catalog agents.Catalog, parsed ParsedDiff, artifacts ArtifactPaths, selection llm.Selection, dependencyTaskIDs []string, maxConcurrency int) ([]review.Finding, []llm.Findings, []sessionDraft, []ledger.Session, map[review.FindingID]string, []ReviewerFailure, error) { +func runReviewers(ctx context.Context, opts Options, req Request, runID string, pr gitprovider.PR, catalog agents.Catalog, parsed ParsedDiff, artifacts ArtifactPaths, selection llm.Selection, dependencyTaskIDs []string, maxConcurrency int, cohortScope ledger.ReviewerCohortScope, reviewerResumeIDs map[string]string, discussion reviewerDiscussionCheckpoint) ([]review.Finding, []llm.Findings, []sessionDraft, []ledger.Session, map[review.FindingID]string, []ReviewerFailure, error) { type job struct { selected llm.SelectedAgent agent agents.Agent @@ -1502,7 +1829,7 @@ func runReviewers(ctx context.Context, opts Options, req Request, runID string, return } defer func() { <-sem }() - result, session, ledgerSession, failure, err := runReviewer(reviewCtx, opts, req, runID, pr, parsed, artifacts, current.selected, current.agent, dependencyTaskIDs) + result, session, ledgerSession, failure, err := runReviewer(reviewCtx, opts, req, runID, pr, parsed, artifacts, current.selected, current.agent, dependencyTaskIDs, reviewerResume{scope: cohortScope, sessionID: reviewerResumeIDs[current.agent.ID], discussion: discussion}) mu.Lock() defer mu.Unlock() if failure != nil { @@ -1537,7 +1864,22 @@ func runReviewers(ctx context.Context, opts Options, req Request, runID string, return allFindings, reviewerResults, sessions, ledgerSessions, findingSessions, failures, nil } -func runReviewer(ctx context.Context, opts Options, req Request, runID string, pr gitprovider.PR, parsed ParsedDiff, artifacts ArtifactPaths, selected llm.SelectedAgent, agent agents.Agent, dependencyTaskIDs []string) (llm.Findings, sessionDraft, ledger.Session, *ReviewerFailure, error) { +type reviewerResume struct { + scope ledger.ReviewerCohortScope + sessionID string + discussion reviewerDiscussionCheckpoint +} + +type reviewerDiscussionCheckpoint struct { + responses []review.ThreadResponseAction + actions []ledger.PlannedAction +} + +func runReviewer(ctx context.Context, opts Options, req Request, runID string, pr gitprovider.PR, parsed ParsedDiff, artifacts ArtifactPaths, selected llm.SelectedAgent, agent agents.Agent, dependencyTaskIDs []string, resume ...reviewerResume) (llm.Findings, sessionDraft, ledger.Session, *ReviewerFailure, error) { + var resumeState reviewerResume + if len(resume) > 0 { + resumeState = resume[0] + } runtimeConfig, err := resolveReviewerRuntimeConfig(req, agent) if err != nil { return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err) @@ -1545,7 +1887,7 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p model, effort := runtimeConfig.model, runtimeConfig.effort changedFilePaths := patchPaths(parsed.Patches) assignmentScope := reviewerAssignmentScope(selected, changedFilePaths) - prompt, promptDeps, err := buildReviewerPrompt(artifacts, pr, selected, agent, changedFilePaths) + prompt, promptDeps, err := buildReviewerPrompt(artifacts, pr, selected, agent, changedFilePaths, resumeState.discussion) if err != nil { return llm.Findings{}, sessionDraft{}, ledger.Session{}, nil, Failure(FailureTerminal, err) } @@ -1590,6 +1932,7 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p logPath: logPath, prompt: prompt, baseRequest: request, + resumeSessionID: resumeState.sessionID, llmFailureStatus: llmTaskStatusFailedIsolated, }, func(data []byte) (llm.Findings, error) { return llm.DecodeFindings(data, llm.FindingsOptions{ @@ -1598,6 +1941,15 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p NewFindingID: opts.newFindingID, }) }) + if providerSessionID := strings.TrimSpace(session.ProviderReportedSessionID); providerSessionID != "" && strings.TrimSpace(resumeState.scope.PRKey) != "" { + store, ok := opts.Store.(reviewerCohortStore) + if !ok { + return llm.Findings{}, session, ledgerSession, nil, fmt.Errorf("pipeline: reviewer cohort store is required") + } + if updateErr := store.UpdateReviewerCohortSession(ctx, resumeState.scope, agent.ID, providerSessionID, opts.now()); updateErr != nil { + return llm.Findings{}, session, ledgerSession, nil, updateErr + } + } if err != nil { var taskErr *llmTaskError if errors.As(err, &taskErr) && taskErr.status == llmTaskStatusFailedIsolated { @@ -1738,8 +2090,12 @@ func prepareNamedSession(ctx context.Context, opts Options, req Request, live bo active: active, supportsResume: opts.Adapter.SupportsResume(), createdAt: now, + store: opts.NamedSessions, } if req.FreshSession { + if err := opts.NamedSessions.DeleteNamedSession(ctx, active.Name); err != nil && !errors.Is(err, ledger.ErrNotFound) { + return namedSessionState{}, fmt.Errorf("pipeline: delete named session %q: %w", active.Name, err) + } return state, nil } stored, err := opts.NamedSessions.GetNamedSession(ctx, active.Name) @@ -1807,13 +2163,34 @@ func (s *namedSessionState) resumeID() string { return s.currentProviderSessionID } -func (s *namedSessionState) recordSessionID(draft sessionDraft) { - if s == nil || !s.enabled || !s.supportsResume { - return +func (s *namedSessionState) checkpointSessionID(ctx context.Context, draft sessionDraft, lastUsedAt time.Time) error { + return s.checkpointProviderSessionID(ctx, draft.ProviderReportedSessionID, lastUsedAt) +} + +func (s *namedSessionState) checkpointProviderSessionID(ctx context.Context, providerSessionID string, lastUsedAt time.Time) error { + if s == nil || !s.enabled { + return nil } - if strings.TrimSpace(draft.ProviderReportedSessionID) != "" { - s.currentProviderSessionID = draft.ProviderReportedSessionID + providerSessionID = strings.TrimSpace(providerSessionID) + if providerSessionID == "" { + return nil } + if s.store == nil { + return fmt.Errorf("pipeline: named session store is required") + } + candidate := ledger.NamedSession{ + Name: s.active.Name, Profile: s.active.Profile, Provider: s.active.Provider, + Adapter: s.active.Adapter, Model: s.active.Model, Host: s.active.Host, + ProviderSessionID: providerSessionID, DurableSession: s.supportsResume, + CreatedAt: s.createdAt, LastUsedAt: lastUsedAt, + } + if err := s.store.UpsertNamedSession(ctx, candidate); err != nil { + return fmt.Errorf("pipeline: checkpoint named session %q: %w", s.active.Name, err) + } + if s.supportsResume { + s.currentProviderSessionID = providerSessionID + } + return nil } func (s *namedSessionState) buildCandidate(draft sessionDraft, lastUsedAt time.Time) *ledger.NamedSession { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 9584560..f9177e7 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -27,6 +27,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/llmadapters" "github.com/open-cli-collective/codereview-cli/internal/llmlifecycle" "github.com/open-cli-collective/codereview-cli/internal/marker" + "github.com/open-cli-collective/codereview-cli/internal/plannedactions" "github.com/open-cli-collective/codereview-cli/internal/reporoot" "github.com/open-cli-collective/codereview-cli/internal/review" "github.com/open-cli-collective/codereview-cli/internal/reviewplan" @@ -2803,6 +2804,18 @@ func TestDryRunFastFallsBackForUnsupportedModel(t *testing.T) { if meta.InputFingerprint != wantFingerprint { t.Fatalf("reviewer fingerprint = %q, want standard-speed %q", meta.InputFingerprint, wantFingerprint) } + checkpointPrompt, _, err := buildReviewerPrompt(result.Artifacts, result.PR, selected, agent, []string{"main.go"}, reviewerDiscussionCheckpoint{ + responses: []review.ThreadResponseAction{{Kind: review.ThreadResponseSummaryReply, ThreadID: "thread-1", Body: "Human clarified null handling.", Resolve: true, Rationale: "settled"}}, + actions: []ledger.PlannedAction{{Action: plannedactions.Action{Kind: ledger.PlannedActionThreadReply, ThreadID: "thread-1", Status: ledger.PlannedActionPosted}}}, + }) + if err != nil { + t.Fatalf("buildReviewerPrompt checkpoint: %v", err) + } + for _, want := range []string{`"discussion_outcomes"`, `"thread_id": "thread-1"`, `"post_status": "posted"`, "Human clarified null handling."} { + if !strings.Contains(checkpointPrompt, want) { + t.Fatalf("checkpoint prompt missing %q:\n%s", want, checkpointPrompt) + } + } } func TestDryRunFastFallsBackForUnsupportedRuntime(t *testing.T) { @@ -3125,7 +3138,7 @@ func TestDefaultSessionNameUsesPRProfileAndPostingIdentityOnly(t *testing.T) { } } -func TestDefaultSessionPersistsFromDryRunAndResumesLiveRerun(t *testing.T) { +func TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) defer closeStore(t, store) @@ -3167,7 +3180,6 @@ func TestDefaultSessionPersistsFromDryRunAndResumesLiveRerun(t *testing.T) { req.Rerun = true run := allocateLiveRun(t, store, provider, req, "run-default-live") liveAdapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} - liveAdapter.Queue(fakeLLMResult("selection-live", selectionJSON("harness:reviewer", "main.go"), 10, 2)) liveAdapter.Queue(fakeLLMResult("reviewer-live", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) liveAdapter.Queue(fakeLLMResult("rollup-live", rollupJSON("comment", []string{"live-finding-1"}), 30, 6)) @@ -3187,8 +3199,11 @@ func TestDefaultSessionPersistsFromDryRunAndResumesLiveRerun(t *testing.T) { t.Fatalf("Live: %v", err) } resumes := liveAdapter.Resumes() - if len(resumes) != 2 || resumes[0].SessionID != "rollup-dry" || resumes[1].SessionID != "selection-live" { - t.Fatalf("live resumes = %#v, want persisted dry-run session then within-run selection", resumes) + if len(resumes) != 2 || resumes[0].SessionID != "reviewer-dry" || resumes[1].SessionID != "rollup-dry" { + t.Fatalf("live resumes = %#v, want exact reviewer and orchestrator sessions", resumes) + } + if len(liveAdapter.Requests()) != 0 { + t.Fatalf("live starts = %#v, want cohort reuse without selection or fresh provider calls", liveAdapter.Requests()) } if liveResult.NamedSessionCandidate == nil || liveResult.NamedSessionCandidate.Name != stored.Name { t.Fatalf("live candidate = %#v, want shared default key %q", liveResult.NamedSessionCandidate, stored.Name) @@ -3211,6 +3226,14 @@ func TestFreshSessionSkipsStoredDefaultWithoutChangingItsKey(t *testing.T) { } req.FreshSession = true run := allocateLiveRun(t, store, provider, req, "run-default-fresh") + if err := store.ReplaceReviewerCohort(ctx, ledger.ReviewerCohort{ + Scope: ledger.ReviewerCohortScope{PRKey: run.PRKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)}, + Adapter: "fake-llm", CreatedAt: fixedNow().Add(-time.Hour), Members: []ledger.ReviewerCohortMember{{ + AgentID: "harness:reviewer", AssignmentMode: ledger.ReviewerAssignmentScoped, Files: []string{"main.go"}, Model: "claude-sonnet-4-6", Effort: "medium", ProviderSessionID: "old-reviewer-session", + }}, + }); err != nil { + t.Fatalf("ReplaceReviewerCohort: %v", err) + } adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} adapter.Queue(fakeLLMResult("selection-fresh", selectionJSON("harness:reviewer", "main.go"), 10, 2)) adapter.Queue(fakeLLMResult("reviewer-fresh", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) @@ -3241,8 +3264,15 @@ func TestFreshSessionSkipsStoredDefaultWithoutChangingItsKey(t *testing.T) { if err != nil { t.Fatalf("GetNamedSession: %v", err) } - if gotStored.ProviderSessionID != "stored-session" { - t.Fatalf("stored provider session = %q, want unchanged until caller commits candidate", gotStored.ProviderSessionID) + if gotStored.ProviderSessionID != "rollup-fresh" { + t.Fatalf("stored provider session = %q, want immediate fresh rollup checkpoint", gotStored.ProviderSessionID) + } + cohort, err := store.GetReviewerCohort(ctx, ledger.ReviewerCohortScope{PRKey: run.PRKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)}) + if err != nil { + t.Fatalf("GetReviewerCohort: %v", err) + } + if len(cohort.Members) != 1 || cohort.Members[0].ProviderSessionID != "reviewer-fresh" { + t.Fatalf("fresh cohort = %#v, want replacement reviewer session", cohort) } } @@ -3266,6 +3296,15 @@ func TestFreshSessionSkipsStoredNamedSession(t *testing.T) { if state.active.Name != "daily" || state.resumeID() != "" { t.Fatalf("fresh named state = %#v resume %q, want daily without stored resume", state, state.resumeID()) } + if _, err := store.GetNamedSession(ctx, "daily"); !errors.Is(err, ledger.ErrNotFound) { + t.Fatalf("GetNamedSession after fresh reset error = %v, want ErrNotFound", err) + } + if _, err := prepareNamedSession(ctx, Options{ + Adapter: &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true}, + NamedSessions: store, + }, req, true, "claude-sonnet-4-6", fixedNow()); err != nil { + t.Fatalf("prepareNamedSession missing fresh row: %v", err) + } } func TestLiveNamedSessionResumesOrchestratorOnlyAndReturnsCandidate(t *testing.T) { @@ -3327,8 +3366,8 @@ func TestLiveNamedSessionResumesOrchestratorOnlyAndReturnsCandidate(t *testing.T if err != nil { t.Fatalf("GetNamedSession: %v", err) } - if gotStored.ProviderSessionID != "stored-session" { - t.Fatalf("stored provider session = %q, want unchanged stored-session", gotStored.ProviderSessionID) + if gotStored.ProviderSessionID != "rollup-new" { + t.Fatalf("stored provider session = %q, want immediate rollup checkpoint", gotStored.ProviderSessionID) } } @@ -3379,8 +3418,55 @@ func TestLiveNamedSessionMissingRowStartsFreshAndReturnsCandidate(t *testing.T) if !reflect.DeepEqual(*result.NamedSessionCandidate, wantCandidate) { t.Fatalf("candidate = %#v, want %#v", *result.NamedSessionCandidate, wantCandidate) } - if _, err := store.GetNamedSession(ctx, req.SessionName); !errors.Is(err, ledger.ErrNotFound) { - t.Fatalf("GetNamedSession error = %v, want pipeline not to persist candidate", err) + if stored, err := store.GetNamedSession(ctx, req.SessionName); err != nil || stored.ProviderSessionID != "rollup-new" { + t.Fatalf("stored named session = %#v, err = %v, want immediate rollup checkpoint", stored, err) + } +} + +func TestLiveNamedSessionPersistsProviderSessionFromSelectionFailure(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + req.SessionName = "daily" + run := allocateLiveRun(t, store, provider, req, "run-live-selection-failure") + providerErr := errors.New("selection provider failed") + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + adapter.Queue(llm.FakeResult{SessionID: "selection-failed", WaitErr: providerErr}) + + _, err := liveForTest(ctx, Options{ + Provider: provider, Adapter: adapter, Store: store, NamedSessions: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), Now: fixedNow, + NewSessionRowID: sequence("session"), NewFindingID: findingSequence("finding"), NewActionID: actionSequence(), MaxConcurrency: 1, + }, req, run) + if !errors.Is(err, providerErr) { + t.Fatalf("Live error = %v, want selection provider failure", err) + } + stored, err := store.GetNamedSession(ctx, req.SessionName) + if err != nil || stored.ProviderSessionID != "selection-failed" { + t.Fatalf("stored named session = %#v, err = %v, want failed selection session", stored, err) + } +} + +func TestLiveNamedSessionCheckpointPersistenceFailureStopsPipeline(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + req.SessionName = "daily" + run := allocateLiveRun(t, store, provider, req, "run-live-checkpoint-failure") + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + adapter.Queue(fakeLLMResult("selection-new", selectionJSON("harness:reviewer", "main.go"), 10, 2)) + checkpointErr := errors.New("checkpoint write failed") + + _, err := liveForTest(ctx, Options{ + Provider: provider, Adapter: adapter, Store: store, + NamedSessions: failingNamedSessionStore{Store: store, upsertErr: checkpointErr}, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), Now: fixedNow, + NewSessionRowID: sequence("session"), NewFindingID: findingSequence("finding"), NewActionID: actionSequence(), MaxConcurrency: 1, + }, req, run) + if !errors.Is(err, checkpointErr) { + t.Fatalf("Live error = %v, want named-session checkpoint failure", err) } } @@ -4656,6 +4742,132 @@ func TestReviewerScopesSeparateReadAccessFromExpectedCoverage(t *testing.T) { } } +func TestRebaseReviewerCohortKeepsOrderAndDropsEmptyScopeCalls(t *testing.T) { + catalog := agents.Catalog{Agents: []agents.Agent{ + {ID: "repo:go", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}}, + {ID: "repo:docs", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"docs/**"}}, + }} + cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{ + {AgentID: "repo:docs", AssignmentMode: ledger.ReviewerAssignmentScoped, Files: []string{"docs/old.md"}, AllowedFiles: []string{"docs/old.md"}, Model: "claude-sonnet-4-6", Effort: "medium"}, + {AgentID: "repo:go", AssignmentMode: ledger.ReviewerAssignmentScoped, Files: []string{"old.go"}, AllowedFiles: []string{"old.go"}, Model: "claude-sonnet-4-6", Effort: "medium", ProviderSessionID: "go-session"}, + }} + req := Request{Profile: testProfile(""), ProfileName: "default"} + + selection, resumes, err := rebaseReviewerCohort(req, catalog, cohort, []string{"main.go"}, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + want := []llm.SelectedAgent{{AgentID: "repo:go", Rationale: "reused reviewer cohort", Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}}} + if !reflect.DeepEqual(selection.SelectedAgents, want) { + t.Fatalf("selected agents = %#v, want %#v", selection.SelectedAgents, want) + } + if !reflect.DeepEqual(resumes, map[string]string{"repo:go": "go-session"}) { + t.Fatalf("reviewer resumes = %#v, want exact saved session", resumes) + } +} + +func TestRebaseReviewerCohortAssignsNewFilesToPersistedBroadMember(t *testing.T) { + req := Request{Profile: testProfile(""), ProfileName: "default"} + cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{ + AgentID: "shared:general", AssignmentMode: ledger.ReviewerAssignmentBroad, Files: []string{"old.go"}, Model: "claude-sonnet-4-6", Effort: "medium", + }}} + catalog := agents.Catalog{Agents: []agents.Agent{{ID: "shared:general", ModelTier: "medium", Effort: "medium"}}} + + selection, _, err := rebaseReviewerCohort(req, catalog, cohort, []string{"main.go", "schema.sql"}, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + want := []string{"main.go", "schema.sql"} + if len(selection.SelectedAgents) != 1 || !reflect.DeepEqual(selection.SelectedAgents[0].Files, want) || len(selection.SelectedAgents[0].AllowedFiles) != 0 { + t.Fatalf("broad rebased selection = %#v, want files %#v without access constraint", selection.SelectedAgents, want) + } +} + +func TestPersistReviewerCohortTreatsFilesOnlyAssignmentAsScoped(t *testing.T) { + store := openPipelineStore(t) + defer closeStore(t, store) + ctx := context.Background() + provider, req := dryRunHarness(t) + run := allocateLiveRun(t, store, provider, req, "run-files-only-scope") + scope := ledger.ReviewerCohortScope{PRKey: run.PRKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity)} + catalog := agents.Catalog{Agents: []agents.Agent{{ID: "repo:go", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}}}} + selection := llm.Selection{SelectedAgents: []llm.SelectedAgent{{AgentID: "repo:go", Files: []string{"main.go"}}}} + + if err := persistReviewerCohort(ctx, Options{Store: store, Adapter: &llm.FakeAdapter{NameValue: "fake-llm"}}, req, scope, catalog, selection, fixedNow()); err != nil { + t.Fatalf("persistReviewerCohort: %v", err) + } + cohort, err := store.GetReviewerCohort(ctx, scope) + if err != nil { + t.Fatalf("GetReviewerCohort: %v", err) + } + if len(cohort.Members) != 1 || cohort.Members[0].AssignmentMode != ledger.ReviewerAssignmentScoped { + t.Fatalf("cohort members = %#v, want files-only scoped assignment", cohort.Members) + } + rebased, _, err := rebaseReviewerCohort(req, catalog, cohort, []string{"main.go"}, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + if len(rebased.SelectedAgents) != 1 || !reflect.DeepEqual(rebased.SelectedAgents[0].AllowedFiles, []string{"main.go"}) { + t.Fatalf("rebased selection = %#v, want scoped Files fallback", rebased.SelectedAgents) + } +} + +func TestRestoreOrchestratorSessionFromInterruptedRunUsesLatestSession(t *testing.T) { + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + run := allocateLiveRun(t, store, provider, req, "interrupted-orchestrator") + for _, session := range []ledger.Session{ + {SessionRowID: "selection", RunID: run.RunID, ProviderSessionID: "selection-session", Role: ledger.SessionRoleOrchestrator, Adapter: "fake", Model: "model", StartedAt: fixedNow()}, + {SessionRowID: "thread", RunID: run.RunID, ProviderSessionID: "thread-session", Role: ledger.SessionRoleOrchestrator, Adapter: "fake", Model: "model", StartedAt: fixedNow().Add(time.Second)}, + } { + if err := store.InsertSession(context.Background(), session); err != nil { + t.Fatalf("InsertSession: %v", err) + } + } + state := namedSessionState{enabled: true, supportsResume: true, currentProviderSessionID: "older-session"} + if err := restoreOrchestratorSessionFromRun(context.Background(), store, run.RunID, &state); err != nil { + t.Fatalf("restoreOrchestratorSessionFromRun: %v", err) + } + if state.resumeID() != "thread-session" { + t.Fatalf("restored orchestrator session = %q, want latest thread session", state.resumeID()) + } +} + +func TestRebaseReviewerCohortRejectsIncompatibleOrUncoveredState(t *testing.T) { + req := Request{Profile: testProfile(""), ProfileName: "default"} + cohort := ledger.ReviewerCohort{Adapter: "old-adapter", Members: []ledger.ReviewerCohortMember{{ + AgentID: "repo:go", AssignmentMode: ledger.ReviewerAssignmentScoped, Model: "claude-sonnet-4-6", Effort: "medium", + }}} + catalog := agents.Catalog{Agents: []agents.Agent{{ID: "repo:go", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}}}} + + for _, tc := range []struct { + name string + adapter string + files []string + maxAgents int + wantDetail string + }{ + {name: "runtime", adapter: "new-adapter", files: []string{"main.go"}, wantDetail: "--fresh-session"}, + {name: "uncovered", adapter: "old-adapter", files: []string{"schema.sql"}, wantDetail: "uncovered"}, + {name: "max agents", adapter: "old-adapter", files: []string{"main.go"}, maxAgents: 0, wantDetail: ""}, + } { + t.Run(tc.name, func(t *testing.T) { + candidate := cohort + if tc.name == "max agents" { + candidate.Members = append(candidate.Members, ledger.ReviewerCohortMember{AgentID: "repo:other", AssignmentMode: ledger.ReviewerAssignmentBroad, Model: "claude-sonnet-4-6", Effort: "medium"}) + catalog.Agents = append(catalog.Agents, agents.Agent{ID: "repo:other", ModelTier: "medium", Effort: "medium"}) + tc.maxAgents = 1 + tc.wantDetail = "--max-agents" + } + _, _, err := rebaseReviewerCohort(req, catalog, candidate, tc.files, tc.maxAgents, tc.adapter) + if err == nil || !strings.Contains(err.Error(), tc.wantDetail) || !strings.Contains(err.Error(), "--fresh-session") { + t.Fatalf("rebaseReviewerCohort error = %v, want %q and fresh-session guidance", err, tc.wantDetail) + } + }) + } +} + func TestBuildReviewerCoverageAllowsBroadReviewerSplitAssignments(t *testing.T) { got := buildReviewerCoverage( []llm.SelectedAgent{ @@ -5824,6 +6036,15 @@ func fakeLLMResult(sessionID, structured string, tokensIn, tokensOut int) llm.Fa } } +type failingNamedSessionStore struct { + *ledger.Store + upsertErr error +} + +func (s failingNamedSessionStore) UpsertNamedSession(context.Context, ledger.NamedSession) error { + return s.upsertErr +} + type providerOriginUsageAdapter struct { mu sync.Mutex name string diff --git a/internal/pipeline/prompts.go b/internal/pipeline/prompts.go index ccf0e97..6db2f7f 100644 --- a/internal/pipeline/prompts.go +++ b/internal/pipeline/prompts.go @@ -24,7 +24,7 @@ import ( const dossierFinalExcerptRunes = 240 -func buildReviewerPrompt(paths ArtifactPaths, pr gitprovider.PR, selected llm.SelectedAgent, agent agents.Agent, changedFiles []string) (string, []string, error) { +func buildReviewerPrompt(paths ArtifactPaths, pr gitprovider.PR, selected llm.SelectedAgent, agent agents.Agent, changedFiles []string, checkpoints ...reviewerDiscussionCheckpoint) (string, []string, error) { input, deps, err := reviewerPromptInputFromArtifacts(paths, pr, selected, agent) if err != nil { return "", nil, err @@ -40,6 +40,9 @@ func buildReviewerPrompt(paths ArtifactPaths, pr gitprovider.PR, selected llm.Se "pr": input.PR, "schema": "findings", } + if len(checkpoints) > 0 && len(checkpoints[0].responses) > 0 { + payload["discussion_outcomes"] = reviewerDiscussionOutcomes(checkpoints[0]) + } body, err := json.MarshalIndent(payload, "", " ") if err != nil { return "", nil, err @@ -47,6 +50,33 @@ func buildReviewerPrompt(paths ArtifactPaths, pr gitprovider.PR, selected llm.Se return string(body), deps, nil } +type reviewerDiscussionOutcome struct { + ThreadID string `json:"thread_id"` + Kind string `json:"kind"` + Body string `json:"body"` + Resolve bool `json:"resolve"` + Rationale string `json:"rationale,omitempty"` + PostStatus string `json:"post_status"` +} + +func reviewerDiscussionOutcomes(checkpoint reviewerDiscussionCheckpoint) []reviewerDiscussionOutcome { + out := make([]reviewerDiscussionOutcome, 0, len(checkpoint.responses)) + for _, response := range checkpoint.responses { + status := "not_planned" + for _, action := range checkpoint.actions { + if action.Kind == reviewplan.ActionKindThreadReply && action.ThreadID == response.ThreadID { + status = action.Status.String() + break + } + } + out = append(out, reviewerDiscussionOutcome{ + ThreadID: response.ThreadID, Kind: string(response.Kind), Body: response.Body, + Resolve: response.Resolve, Rationale: response.Rationale, PostStatus: status, + }) + } + return out +} + type selectionAgentPrompt struct { ID string `json:"id"` Name string `json:"name"` diff --git a/internal/reviewrun/reviewrun.go b/internal/reviewrun/reviewrun.go index d1660ff..c3760ca 100644 --- a/internal/reviewrun/reviewrun.go +++ b/internal/reviewrun/reviewrun.go @@ -263,6 +263,12 @@ func continueRun(ctx context.Context, opts Options, req Request, result Result) if run, loadErr := opts.Store.GetRun(context.Background(), result.Run.RunID); loadErr == nil { result.Run = run } + if errors.Is(err, gitprovider.ErrStaleSHA) { + result.Outbox = outbox.Result{Outcome: ledger.OutcomeAborted, ExitCode: exitUpstream, Aborted: true} + result.ExitCode = exitUpstream + result.Message = "review premises moved during thread response checkpoint" + return result, nil + } return result, err } if planResult != nil { @@ -328,7 +334,8 @@ func planOrResume(ctx context.Context, opts Options, req Request, result Result) if err != nil { return "", nil, err } - if result.Decision.Kind == gate.DecisionResume && len(actions) > 0 { + checkpointOnly := onlyThreadCheckpointActions(actions) + if result.Decision.Kind == gate.DecisionResume && len(actions) > 0 && !checkpointOnly { desired, err := desiredOutcomeFromActions(actions) if err != nil { if completeErr := opts.Store.CompleteRun(ctx, result.Run.RunID, ledger.OutcomeFailed, opts.now()); completeErr != nil { @@ -338,7 +345,7 @@ func planOrResume(ctx context.Context, opts Options, req Request, result Result) } return desired, nil, nil } - if result.Decision.Kind == gate.DecisionResume { + if result.Decision.Kind == gate.DecisionResume && !checkpointOnly { empty, err := planningStateEmpty(ctx, opts.Store, result.Run.RunID) if err != nil { return "", nil, err @@ -363,6 +370,9 @@ func planOrResume(ctx context.Context, opts Options, req Request, result Result) return "", nil, err } outcome := plannerFailureOutcome(pipeline.ClassifyFailure(err), result.Decision.Kind == gate.DecisionResume) + if errors.Is(err, gitprovider.ErrStaleSHA) { + outcome = ledger.OutcomeAborted + } if completeErr := opts.Store.CompleteRun(context.Background(), result.Run.RunID, outcome, opts.now()); completeErr != nil { return "", nil, completeErr } @@ -375,6 +385,18 @@ func planOrResume(ctx context.Context, opts Options, req Request, result Result) return desired, &planned, nil } +func onlyThreadCheckpointActions(actions []ledger.PlannedAction) bool { + if len(actions) == 0 { + return false + } + for _, action := range actions { + if action.Kind != ledger.PlannedActionThreadReply && action.Kind != ledger.PlannedActionResolveThread { + return false + } + } + return true +} + func plannerFailureOutcome(kind pipeline.FailureKind, resumed bool) ledger.Outcome { if kind == pipeline.FailureDurableBlocking || kind == pipeline.FailureTransient && resumed { return ledger.OutcomeIncomplete diff --git a/internal/reviewrun/reviewrun_test.go b/internal/reviewrun/reviewrun_test.go index 5e9ba7f..ac1d3f4 100644 --- a/internal/reviewrun/reviewrun_test.go +++ b/internal/reviewrun/reviewrun_test.go @@ -434,6 +434,25 @@ func TestRunDoesNotCommitNamedSessionCandidateAfterOutboxError(t *testing.T) { } } +func TestRunTreatsCheckpointPremiseMovementAsAborted(t *testing.T) { + ctx := context.Background() + fixture := newFixture(t) + planner := &fakePlanner{store: fixture.store, err: gitprovider.ErrStaleSHA} + opts := fixture.opts(planner) + opts.NewRunID = sequence("fresh") + + result, err := Run(ctx, opts, Request{Pipeline: fixture.req}) + if err != nil { + t.Fatalf("Run: %v", err) + } + if result.ExitCode != exitUpstream || !result.Outbox.Aborted || result.Outbox.Outcome != ledger.OutcomeAborted { + t.Fatalf("result = %#v, want checkpoint premise abort", result) + } + if result.Run.Outcome == nil || *result.Run.Outcome != ledger.OutcomeAborted { + t.Fatalf("run outcome = %v, want aborted", result.Run.Outcome) + } +} + func TestRunResumeExistingActionsSkipsPlanner(t *testing.T) { ctx := context.Background() fixture := newFixture(t) @@ -613,6 +632,33 @@ func TestRunResumeInvalidStoredActionsFailsRunWithRerunGuidance(t *testing.T) { } } +func TestRunResumeThreadCheckpointActionsContinuesPlanner(t *testing.T) { + ctx := context.Background() + fixture := newFixture(t) + run := fixture.allocateRun(t, "thread-checkpoint", testBaseSHA) + if err := fixture.store.InsertPlannedAction(ctx, ledger.PlannedAction{ + Action: plannedactions.Action{ + ActionID: "reply-early", Kind: ledger.PlannedActionThreadReply, ThreadID: "thread-1", PlannedAt: testNow(), + ThreadReply: &plannedactions.ThreadReplyPayload{Body: "summary", Summary: true}, Status: ledger.PlannedActionPending, Required: true, + }, + RunID: run.RunID, + }); err != nil { + t.Fatalf("InsertPlannedAction: %v", err) + } + planner := &fakePlanner{store: fixture.store, outcome: reviewplan.OutcomeComment} + + result, err := Run(ctx, fixture.opts(planner), Request{Pipeline: fixture.req}) + if err != nil { + t.Fatalf("Run: %v", err) + } + if planner.calls != 1 { + t.Fatalf("planner calls = %d, want checkpoint resume to continue planning", planner.calls) + } + if result.Outbox.Outcome != ledger.OutcomeComment { + t.Fatalf("outbox outcome = %q, want comment", result.Outbox.Outcome) + } +} + func TestRunResumePartialPlanningStateFailsWithoutReplayingLLM(t *testing.T) { ctx := context.Background() fixture := newFixture(t) diff --git a/internal/threadanalysis/threadanalysis.go b/internal/threadanalysis/threadanalysis.go index 43d5ca1..5e3f215 100644 --- a/internal/threadanalysis/threadanalysis.go +++ b/internal/threadanalysis/threadanalysis.go @@ -58,37 +58,44 @@ type Result struct { // Options are explicit call-site supplied dependencies for one analysis run. type Options struct { - Store llmlifecycle.Store - RunID string - Adapter llm.Adapter - Model string - Effort string - LogPath string - LifecyclePaths llmlifecycle.Paths - Progress llmlifecycle.Progress - Now func() time.Time - NewStepID func() string + Store llmlifecycle.Store + RunID string + Adapter llm.Adapter + Model string + Effort string + LogPath string + LifecyclePaths llmlifecycle.Paths + Progress llmlifecycle.Progress + Now func() time.Time + NewStepID func() string + ResumeSessionID string + OnSessionID func(string) error } // AnalyzeThread analyzes one normalized inline thread through the durable LLM lifecycle. func AnalyzeThread(ctx context.Context, opts Options, thread threadcontext.Thread) (Result, error) { + result, _, err := analyzeThread(ctx, opts, thread) + return result, err +} + +func analyzeThread(ctx context.Context, opts Options, thread threadcontext.Thread) (Result, llmlifecycle.SessionDraft, error) { var zero Result if err := validateOptions(opts); err != nil { - return zero, err + return zero, llmlifecycle.SessionDraft{}, err } threadID := strings.TrimSpace(string(thread.ID)) if threadID == "" { - return zero, fmt.Errorf("threadanalysis: thread ID is required") + return zero, llmlifecycle.SessionDraft{}, fmt.Errorf("threadanalysis: thread ID is required") } request, err := lifecycleRequestForThread(opts, threadID, thread) if err != nil { - return zero, err + return zero, llmlifecycle.SessionDraft{}, err } result, err := llmlifecycle.RunStructured(ctx, request, decodeResultForThread(threadID)) if err != nil { - return zero, err + return zero, result.Draft, err } - return result.Value, nil + return result.Value, result.Draft, nil } // AnalyzeThreads analyzes normalized inline threads in order, using logPath to @@ -107,7 +114,15 @@ func AnalyzeThreads(ctx context.Context, opts Options, threads []threadcontext.T if err != nil { return nil, err } - result, err := AnalyzeThread(ctx, opts, thread) + result, draft, err := analyzeThread(ctx, opts, thread) + if sessionID := strings.TrimSpace(draft.ProviderReportedSessionID); sessionID != "" { + if opts.OnSessionID != nil { + if checkpointErr := opts.OnSessionID(sessionID); checkpointErr != nil { + return nil, checkpointErr + } + } + opts.ResumeSessionID = sessionID + } if err != nil { return nil, err } @@ -160,6 +175,7 @@ func lifecycleRequestForThread(opts Options, threadID string, thread threadconte Progress: opts.Progress, Now: opts.Now, NewSessionRowID: opts.NewStepID, + ResumeSessionID: opts.ResumeSessionID, }, nil } diff --git a/internal/threadanalysis/threadanalysis_test.go b/internal/threadanalysis/threadanalysis_test.go index d1590bf..afb53a3 100644 --- a/internal/threadanalysis/threadanalysis_test.go +++ b/internal/threadanalysis/threadanalysis_test.go @@ -2,6 +2,7 @@ package threadanalysis import ( "context" + "errors" "fmt" "go/parser" "go/token" @@ -140,6 +141,70 @@ func TestAnalyzeThreadsPreservesSingleThreadArtifactsAndOrder(t *testing.T) { } } +func TestAnalyzeThreadsChainsOneProviderSessionInOrder(t *testing.T) { + threads := []threadcontext.Thread{promptThreadWithID("thread-1", "first"), promptThreadWithID("thread-2", "second")} + adapter := &llm.FakeAdapter{NameValue: "fake", SupportsResumeValue: true} + adapter.Queue(llm.FakeResult{SessionID: "thread-session-1", Response: llm.Response{StructuredOutput: []byte(validSkipOutput("thread-1"))}}) + adapter.Queue(llm.FakeResult{SessionID: "thread-session-2", Response: llm.Response{StructuredOutput: []byte(validSkipOutput("thread-2"))}}) + opts := testOptions(t, newFakeStore(), adapter) + opts.ResumeSessionID = "selection-session" + var checkpoints []string + opts.OnSessionID = func(sessionID string) error { + checkpoints = append(checkpoints, sessionID) + return nil + } + + if _, err := AnalyzeThreads(context.Background(), opts, threads, func(thread threadcontext.Thread) (string, error) { + return string(thread.ID) + ".log", nil + }); err != nil { + t.Fatalf("AnalyzeThreads: %v", err) + } + resumes := adapter.Resumes() + if len(resumes) != 2 || resumes[0].SessionID != "selection-session" || resumes[1].SessionID != "thread-session-1" { + t.Fatalf("thread resumes = %#v, want sequential orchestrator chain", resumes) + } + if !reflect.DeepEqual(checkpoints, []string{"thread-session-1", "thread-session-2"}) { + t.Fatalf("session checkpoints = %#v", checkpoints) + } +} + +func TestAnalyzeThreadsCheckpointsProviderSessionBeforeReturningFailure(t *testing.T) { + adapter := &llm.FakeAdapter{NameValue: "fake", SupportsResumeValue: true} + providerErr := errors.New("provider failed") + adapter.Queue(llm.FakeResult{SessionID: "thread-session-failed", WaitErr: providerErr}) + opts := testOptions(t, newFakeStore(), adapter) + var checkpoints []string + opts.OnSessionID = func(sessionID string) error { + checkpoints = append(checkpoints, sessionID) + return nil + } + + _, err := AnalyzeThreads(context.Background(), opts, []threadcontext.Thread{promptThread("reply")}, func(threadcontext.Thread) (string, error) { + return "thread.log", nil + }) + if !errors.Is(err, providerErr) { + t.Fatalf("AnalyzeThreads error = %v, want provider failure", err) + } + if !reflect.DeepEqual(checkpoints, []string{"thread-session-failed"}) { + t.Fatalf("session checkpoints = %#v, want failed provider session", checkpoints) + } +} + +func TestAnalyzeThreadsPropagatesSessionCheckpointFailure(t *testing.T) { + adapter := &llm.FakeAdapter{NameValue: "fake"} + adapter.Queue(llm.FakeResult{SessionID: "thread-session", Response: llm.Response{StructuredOutput: []byte(validSkipOutput("thread-1"))}}) + opts := testOptions(t, newFakeStore(), adapter) + checkpointErr := errors.New("persist checkpoint") + opts.OnSessionID = func(string) error { return checkpointErr } + + _, err := AnalyzeThreads(context.Background(), opts, []threadcontext.Thread{promptThread("reply")}, func(threadcontext.Thread) (string, error) { + return "thread.log", nil + }) + if !errors.Is(err, checkpointErr) { + t.Fatalf("AnalyzeThreads error = %v, want checkpoint failure", err) + } +} + func TestAnalyzeThreadsCacheHitAndStaleInputSkipAdapter(t *testing.T) { store := newFakeStore() seedAdapter := &llm.FakeAdapter{NameValue: "fake"} From 5f1acc582a4140ad37fdc6bf5375a97cf6467883 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 21:19:25 -0400 Subject: [PATCH 2/7] fix: restore checkpointed discussion context --- internal/pipeline/pipeline.go | 80 +++++++++++++++-- internal/pipeline/pipeline_test.go | 138 +++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 5 deletions(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 108bc44..cc86961 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -772,14 +772,21 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu } } selectionInRun := false + reviewerInRun := false if reusedCohort { if _, selectionInRun, err = llmlifecycle.ReadMetadata(lifecyclePaths(prepared.artifacts), orchestratorSelectionStage); err != nil { return nil, false, err } + if selectionInRun { + reviewerInRun, err = hasReviewerTaskMetadata(prepared.artifacts, selection.SelectedAgents) + if err != nil { + return nil, false, err + } + } } var selectionSession sessionDraft var selectionLedgerSession ledger.Session - if !reusedCohort || selectionInRun { + if !reusedCohort || selectionInRun && reviewerInRun { selection, selectionSession, selectionLedgerSession, err = runSelectionPhase(ctx, opts, selectionPhaseRequest{ RunID: run.RunID, DurableSession: namedSession.enabled && namedSession.supportsResume, @@ -827,7 +834,8 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu if err != nil { return nil, false, err } - findings, reviewerResults, reviewerSessions, reviewerLedgerSessions, findingSessions, reviewerFailures, err := runReviewers(ctx, opts, req, run.RunID, prepared.reviewPR, prepared.catalog, prepared.parsed, prepared.artifacts, selection, selectionTaskIDs, maxConcurrency, cohortScope, reviewerResumeIDs, reviewerDiscussionCheckpoint{responses: threadResponses, actions: checkpointActions}) + reviewerThreadResponses, reviewerCheckpointActions := recoverCheckpointThreadResponses(threadResponses, prepared.threadContext, checkpointActions) + findings, reviewerResults, reviewerSessions, reviewerLedgerSessions, findingSessions, reviewerFailures, err := runReviewers(ctx, opts, req, run.RunID, prepared.reviewPR, prepared.catalog, prepared.parsed, prepared.artifacts, selection, selectionTaskIDs, maxConcurrency, cohortScope, reviewerResumeIDs, reviewerDiscussionCheckpoint{responses: reviewerThreadResponses, actions: reviewerCheckpointActions}) if err != nil { return executionPhaseFailure(err) } @@ -1322,15 +1330,15 @@ type checkpointPlanningStore interface { } func checkpointThreadResponses(ctx context.Context, opts Options, req Request, mode executionMode, run ledger.Run, caps reviewplan.ProviderCaps, responses []review.ThreadResponseAction) ([]ledger.PlannedAction, error) { - if len(responses) == 0 { - return nil, nil - } existing, err := opts.Store.ListPlannedActions(ctx, run.RunID) if err != nil { return nil, err } actions := checkpointActionsOnly(existing) if len(actions) == 0 { + if len(responses) == 0 { + return nil, nil + } plan, err := reviewplan.BuildThreadResponses(reviewplan.ThreadResponseRequest{ PostMode: mode.planPostMode, ProviderCaps: caps, @@ -1358,6 +1366,9 @@ func checkpointThreadResponses(ctx context.Context, opts Options, req Request, m if errors.Is(err, gitprovider.ErrStaleSHA) { return nil, err } + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil, err + } opts.emitWarning(fmt.Sprintf("thread response checkpoint posting failed; final posting will retry: %v", err)) } } @@ -1368,6 +1379,65 @@ func checkpointThreadResponses(ctx context.Context, opts Options, req Request, m return checkpointActionsOnly(stored), nil } +func recoverCheckpointThreadResponses(current []review.ThreadResponseAction, threads []threadcontext.Thread, actions []ledger.PlannedAction) ([]review.ThreadResponseAction, []ledger.PlannedAction) { + seen := make(map[string]bool, len(current)) + for _, response := range current { + seen[response.ThreadID] = true + } + replies := make(map[string]ledger.PlannedAction) + resolves := make(map[string]bool) + for _, action := range actions { + switch action.Kind { + case ledger.PlannedActionThreadReply: + if action.ThreadReply != nil && action.ThreadReply.Summary { + replies[action.ThreadID] = action + } + case ledger.PlannedActionResolveThread: + resolves[action.ThreadID] = true + case ledger.PlannedActionInlineComment, ledger.PlannedActionRollupComment, ledger.PlannedActionSubmitReview: + } + } + out := append([]review.ThreadResponseAction(nil), current...) + recoveredActions := append([]ledger.PlannedAction(nil), actions...) + for _, thread := range threads { + threadID := string(thread.ID) + if seen[threadID] { + continue + } + summary, ok := thread.EffectiveSettledSummary() + if !ok || !summary.LastCommentAuthoredByPostingIdentity || !summary.LastCommentHasThreadSummaryMarker { + continue + } + body := summary.Body + resolve := resolves[threadID] + if reply, exists := replies[threadID]; exists { + body = reply.ThreadReply.Body + } else { + synthetic := ledger.PlannedAction{} + synthetic.Kind = ledger.PlannedActionThreadReply + synthetic.ThreadID = threadID + synthetic.Status = ledger.PlannedActionPosted + recoveredActions = append(recoveredActions, synthetic) + } + out = append(out, review.ThreadResponseAction{ + Kind: review.ThreadResponseSummaryReply, ThreadID: threadID, + Body: body, Resolve: resolve, + }) + } + return out, recoveredActions +} + +func hasReviewerTaskMetadata(artifacts ArtifactPaths, selected []llm.SelectedAgent) (bool, error) { + for _, reviewer := range selected { + if _, ok, err := llmlifecycle.ReadMetadata(lifecyclePaths(artifacts), reviewerTaskID(reviewer.AgentID)); err != nil { + return false, err + } else if ok { + return true, nil + } + } + return false, nil +} + func checkpointActionsOnly(actions []ledger.PlannedAction) []ledger.PlannedAction { out := make([]ledger.PlannedAction, 0, len(actions)) for _, action := range actions { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index f9177e7..0573e7a 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -35,6 +35,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/runlifecycle" "github.com/open-cli-collective/codereview-cli/internal/stagemodel" "github.com/open-cli-collective/codereview-cli/internal/statepaths" + "github.com/open-cli-collective/codereview-cli/internal/threadcontext" ) func dryRunForTest(ctx context.Context, opts Options, req Request) (Result, error) { @@ -704,6 +705,143 @@ func TestReviewPipelineAcceptanceHarnessResumesFailedDurableTask(t *testing.T) { } } +func TestLiveResumeRecoversPostedThreadSummaryForReviewerPrompt(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + provider, req := dryRunHarness(t) + human := gitprovider.Identity{Login: "human", ID: "human-id"} + provider.threads = []gitprovider.InlineThread{markedReviewThread(t, "thread-1", "main.go", 2, req.PostingIdentity, human)} + provider.caps.ThreadResolution = true + run := allocateLiveRun(t, store, provider, req, "run-thread-checkpoint-resume") + var checkpointCalls, posts, reconciles int + threadCheckpoint := func(ctx context.Context, run ledger.Run, _ Request) error { + checkpointCalls++ + actions, err := store.ListPlannedActions(ctx, run.RunID) + if err != nil { + return err + } + for _, action := range actions { + if action.Kind != ledger.PlannedActionThreadReply || action.Status != ledger.PlannedActionPending { + continue + } + if provider.threads[0].Comments[len(provider.threads[0].Comments)-1].ID == "thread-1-summary" { + now := fixedNow() + action.Status = ledger.PlannedActionPosted + action.PostedAt = &now + upstreamID := "thread-1-summary" + action.UpstreamID = &upstreamID + if err := store.UpdatePlannedAction(ctx, action); err != nil { + return err + } + reconciles++ + continue + } + markerText, err := marker.RenderThreadSummary(marker.ThreadSummaryMarker{RunID: run.RunID, ActionID: action.ActionID}) + if err != nil { + return err + } + postedAt := fixedNow().Add(2 * time.Minute) + provider.threads[0].Comments = append(provider.threads[0].Comments, gitprovider.ThreadComment{ + ID: "thread-1-summary", ThreadID: "thread-1", Body: markerText + "\n\nHuman clarified null handling.", + Author: req.PostingIdentity, CommitSHA: provider.pr.Head.SHA, Path: "main.go", Side: review.DiffSideRight, + Line: 2, SubjectType: review.AnchorKindLine, CreatedAt: postedAt, UpdatedAt: postedAt, + }) + posts++ + return context.Canceled + } + return nil + } + + firstAdapter := &llm.FakeAdapter{NameValue: "fake-llm"} + firstAdapter.Queue(fakeLLMResult("dossier-summary-session", discussionSummaryJSON(nil, nil), 1, 1)) + firstAdapter.Queue(fakeLLMResult("selection-session", selectionJSON("harness:reviewer", "main.go"), 1, 1)) + firstAdapter.Queue(fakeLLMResult("thread-session", `{ + "schema_version": 1, + "thread_id": "thread-1", + "decision": "summarize", + "summary": "Human clarified null handling.", + "resolve": true, + "rationale": "settled" + }`, 1, 1)) + _, err := liveForTest(ctx, Options{ + Provider: provider, Adapter: firstAdapter, Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), Now: fixedNow, + NewSessionRowID: sequence("session"), NewFindingID: findingSequence("finding"), NewActionID: actionSequence(), + MaxConcurrency: 1, ThreadCheckpoint: threadCheckpoint, + }, req, run) + if !errors.Is(err, context.Canceled) { + t.Fatalf("first Live error = %v, want checkpoint interruption", err) + } + if posts != 1 || checkpointCalls != 1 { + t.Fatalf("first checkpoint calls/posts = %d/%d, want 1/1", checkpointCalls, posts) + } + metadataPath, err := lifecyclePaths(ArtifactPathsFromDir(run.ArtifactPath)).Metadata("thread-analysis-thread-1") + if err != nil { + t.Fatalf("thread metadata path: %v", err) + } + if err := os.Remove(metadataPath); err != nil { + t.Fatalf("remove thread metadata: %v", err) + } + + secondAdapter := &llm.FakeAdapter{NameValue: "fake-llm"} + secondAdapter.Queue(fakeLLMResult("reviewer-session", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 1, 1)) + secondAdapter.Queue(fakeLLMResult("rollup-session", rollupJSON("comment", []string{"finding-1"}), 1, 1)) + _, err = liveForTest(ctx, Options{ + Provider: provider, Adapter: secondAdapter, Store: store, + Layout: statepaths.NewLayout(t.TempDir(), t.TempDir()), Now: fixedNow, + NewSessionRowID: sequence("resume-session"), NewFindingID: findingSequence("finding"), NewActionID: actionSequence(), + MaxConcurrency: 1, ThreadCheckpoint: threadCheckpoint, + }, req, run) + if err != nil { + t.Fatalf("second Live: %v", err) + } + if posts != 1 || reconciles != 1 || checkpointCalls != 2 { + t.Fatalf("checkpoint calls/posts/reconciles = %d/%d/%d, want 2/1/1", checkpointCalls, posts, reconciles) + } + requests := secondAdapter.Requests() + if len(requests) != 2 { + t.Fatalf("second adapter requests = %d, want reviewer and rollup only", len(requests)) + } + var reviewerPrompt string + for _, request := range requests { + if strings.Contains(request.Prompt, "Use schema_version 1 and fields: thread_id") { + t.Fatalf("resumed pipeline repeated thread analysis:\n%s", request.Prompt) + } + if strings.Contains(request.Prompt, `"schema": "findings"`) { + reviewerPrompt = request.Prompt + } + } + if reviewerPrompt == "" { + t.Fatal("resumed reviewer prompt not found") + } + for _, want := range []string{`"discussion_outcomes"`, `"thread_id": "thread-1"`, `"post_status": "posted"`, "Human clarified null handling."} { + if !strings.Contains(reviewerPrompt, want) { + t.Fatalf("resumed reviewer prompt missing %q:\n%s", want, reviewerPrompt) + } + } +} + +func TestRecoverCheckpointThreadResponsesUsesAuthoritativeMarkerWithoutLocalState(t *testing.T) { + bot := gitprovider.Identity{Login: "review-bot", ID: "review-bot-id"} + human := gitprovider.Identity{Login: "human", ID: "human-id"} + threads, err := threadcontext.Normalize([]gitprovider.InlineThread{ + crSettledReviewThread(t, "thread-1", "main.go", 2, bot, human, "Marker-only durable summary."), + }, threadcontext.Options{PostingIdentity: bot}) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + + responses, actions := recoverCheckpointThreadResponses(nil, threads, nil) + if len(responses) != 1 || responses[0].Body != "Marker-only durable summary." || responses[0].Kind != review.ThreadResponseSummaryReply { + t.Fatalf("recovered responses = %#v, want authoritative marker summary", responses) + } + outcomes := reviewerDiscussionOutcomes(reviewerDiscussionCheckpoint{responses: responses, actions: actions}) + if len(outcomes) != 1 || outcomes[0].Body != "Marker-only durable summary." || outcomes[0].PostStatus != ledger.PlannedActionPosted.String() { + t.Fatalf("discussion outcomes = %#v, want marker summary with posted status", outcomes) + } +} + func TestDryRunResumeReusesPersistedPlanningRows(t *testing.T) { ctx := context.Background() store := openPipelineStore(t) From 13e683da406999610dbab6f1cfa438575fafee03 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 21:28:43 -0400 Subject: [PATCH 3/7] fix: resume Codex reviewers in fresh workspaces --- internal/llmadapters/subprocess.go | 6 +-- internal/llmadapters/subprocess_test.go | 43 +++++++++++++++++--- internal/pipeline/pipeline_test.go | 52 ++++++++++++++++++++++++- 3 files changed, 92 insertions(+), 9 deletions(-) diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index 4d9d24f..a784f38 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -206,6 +206,9 @@ func (a *SubprocessAdapter) startJSONLSubprocess(ctx context.Context, req Reques execArgs := append(append([]string(nil), a.commandArgsPrefix...), args...) launchDir := scratch if a.kind == subprocessCodex && resumeSessionID != "" && req.ReviewerWorkspace != nil { + // Codex resume rejects --sandbox/--cd/--add-dir. Launching from the new + // disposable checkout rebinds its workspace while processEnv keeps tool + // caches and temporary writes inside the new invocation scratch root. launchDir = req.ReviewerWorkspace.RepoDir } env, err := a.processEnv(req, scratch) @@ -603,9 +606,6 @@ func (a *SubprocessAdapter) Resume(ctx context.Context, sessionID string, req Re if sessionID == "" { return a.Start(ctx, req) } - if req.ReviewerWorkspace != nil { - return nil, fmt.Errorf("%w: codex_cli resume does not support reviewer workspace roots", ErrUnsafeSubprocessConfig) - } return a.startJSONLSubprocess(ctx, req, sessionID) default: return nil, fmt.Errorf("llm subprocess: resume unsupported for %s", a.kind) diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index ec28a62..3fc8dac 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -875,8 +875,9 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { }) } - t.Run("resume reviewer workspace is rejected", func(t *testing.T) { + t.Run("resume reviewer workspace launches from new bounded workspace", func(t *testing.T) { tempDir := t.TempDir() + recordPath := filepath.Join(tempDir, "records.jsonl") scratchRoot := filepath.Join(tempDir, "workbench-scratch") if err := os.MkdirAll(scratchRoot, 0o700); err != nil { t.Fatalf("MkdirAll(scratchRoot): %v", err) @@ -885,8 +886,11 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { if err := os.MkdirAll(repoRoot, 0o700); err != nil { t.Fatalf("MkdirAll(repoRoot): %v", err) } - adapter := NewCodexCLIAdapter(SubprocessOptions{AllowBestEffortNoTools: true}) - _, err := adapter.Resume(context.Background(), "prior-session", Request{ + if err := os.WriteFile(filepath.Join(repoRoot, "main.go"), []byte("package main\n"), 0o600); err != nil { + t.Fatalf("WriteFile(repo): %v", err) + } + adapter := newCodexHelperAdapter("tool-success", recordPath, 5*time.Second, "CGO_CFLAGS=-O2", "CGO_CXXFLAGS=-stdlib=libc++") + stream, err := adapter.Resume(context.Background(), "prior-session", Request{ Model: "gpt-5.5", Effort: "high", Prompt: "resume prompt", @@ -895,8 +899,33 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { ScratchDir: scratchRoot, }, }) - if !errors.Is(err, ErrUnsafeSubprocessConfig) { - t.Fatalf("Resume error = %v, want ErrUnsafeSubprocessConfig", err) + if err != nil { + t.Fatalf("Resume: %v", err) + } + if _, err := stream.Wait(context.Background()); err != nil { + t.Fatalf("Wait: %v", err) + } + record := readHelperRecord(t, recordPath) + if !samePath(t, record.Cwd, repoRoot) || record.CwdEntries != 1 { + t.Fatalf("resume cwd = %q entries = %d, want workspace %q with checkout", record.Cwd, record.CwdEntries, repoRoot) + } + if len(record.AdapterArgs) < 2 || record.AdapterArgs[0] != "exec" || record.AdapterArgs[1] != "resume" { + t.Fatalf("resume args = %#v, want codex exec resume", record.AdapterArgs) + } + for _, flag := range []string{"--sandbox", "--cd", "--add-dir"} { + if containsFlag(record.AdapterArgs, flag) { + t.Fatalf("resume args = %#v, do not want unsupported %s", record.AdapterArgs, flag) + } + } + for name, path := range map[string]string{ + "TMPDIR": record.TMPDir, "GOTMPDIR": record.GoTmpDir, "GOCACHE": record.GoCache, "XDG_CACHE_HOME": record.XDGCacheHome, + } { + if !pathUnderAnyRoot(path, []string{scratchRoot}) { + t.Fatalf("%s = %q, want invocation path under scratch root %q", name, path, scratchRoot) + } + } + if !strings.Contains(record.CGOCFlags, "-O2 -fmodules-cache-path="+scratchRoot) || !strings.Contains(record.CGOCXXFlags, "-stdlib=libc++ -fmodules-cache-path="+scratchRoot) { + t.Fatalf("compiler env = %q / %q, want preserved flags and scratch-scoped module cache", record.CGOCFlags, record.CGOCXXFlags) } }) @@ -1421,6 +1450,10 @@ func TestSubprocessHelperProcess(_ *testing.T) { fmt.Println(`{"type":"thread.started","thread_id":"session-usage"}`) fmt.Println(`{"type":"item.completed","item":{"id":"item_0","type":"agent_message","text":"{\"ok\":true}"}}`) fmt.Println(`{"type":"turn.completed","usage":{"input_tokens":25475,"cached_input_tokens":19712,"output_tokens":812,"reasoning_output_tokens":271}}`) + case "tool-success": + fmt.Println(`{"type":"tool_use","name":"Read"}`) + fmt.Println(`{"type":"thread.started","thread_id":"session-1"}`) + fmt.Println(`{"type":"item.completed","item":{"id":"item_0","type":"agent_message","text":"{\"ok\":true}"}}`) case "noisy-success": fmt.Fprintln(os.Stderr, strings.Repeat("stderr-noise-", 32)) fmt.Println(`{"type":"thread.started","thread_id":"session-1"}`) diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 0573e7a..83cb6d0 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -3317,7 +3317,10 @@ func TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun(t *testing.T) req.Rerun = true run := allocateLiveRun(t, store, provider, req, "run-default-live") - liveAdapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + liveAdapter := &reviewerWorkspaceResumeAdapter{ + FakeAdapter: &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true}, + reviewerSessionID: "reviewer-dry", + } liveAdapter.Queue(fakeLLMResult("reviewer-live", findingsJSON("harness:reviewer", "main.go", "major", 2, "Fix this"), 20, 4)) liveAdapter.Queue(fakeLLMResult("rollup-live", rollupJSON("comment", []string{"live-finding-1"}), 30, 6)) @@ -3343,6 +3346,9 @@ func TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun(t *testing.T) if len(liveAdapter.Requests()) != 0 { t.Fatalf("live starts = %#v, want cohort reuse without selection or fresh provider calls", liveAdapter.Requests()) } + if err := liveAdapter.BoundaryError(); err != nil { + t.Fatalf("reviewer resume workspace boundary: %v", err) + } if liveResult.NamedSessionCandidate == nil || liveResult.NamedSessionCandidate.Name != stored.Name { t.Fatalf("live candidate = %#v, want shared default key %q", liveResult.NamedSessionCandidate, stored.Name) } @@ -6179,6 +6185,50 @@ type failingNamedSessionStore struct { upsertErr error } +type reviewerWorkspaceResumeAdapter struct { + *llm.FakeAdapter + mu sync.Mutex + reviewerSessionID string + checked bool + boundaryErr error +} + +func (a *reviewerWorkspaceResumeAdapter) Resume(ctx context.Context, sessionID string, req llm.Request) (llm.Stream, error) { + if sessionID == a.reviewerSessionID { + a.mu.Lock() + a.checked = true + switch { + case req.ReviewerWorkspace == nil: + a.boundaryErr = errors.New("reviewer workspace is missing") + case strings.TrimSpace(req.ReviewerWorkspace.RepoDir) == "": + a.boundaryErr = errors.New("reviewer workspace repo is missing") + case strings.TrimSpace(req.ReviewerWorkspace.ScratchDir) == "": + a.boundaryErr = errors.New("reviewer workspace scratch is missing") + default: + if info, err := os.Stat(req.ReviewerWorkspace.RepoDir); err != nil { + a.boundaryErr = fmt.Errorf("reviewer workspace repo unavailable: %w", err) + } else if !info.IsDir() { + a.boundaryErr = errors.New("reviewer workspace repo is not a directory") + } else if info, err := os.Stat(req.ReviewerWorkspace.ScratchDir); err != nil { + a.boundaryErr = fmt.Errorf("reviewer workspace scratch unavailable: %w", err) + } else if !info.IsDir() { + a.boundaryErr = errors.New("reviewer workspace scratch is not a directory") + } + } + a.mu.Unlock() + } + return a.FakeAdapter.Resume(ctx, sessionID, req) +} + +func (a *reviewerWorkspaceResumeAdapter) BoundaryError() error { + a.mu.Lock() + defer a.mu.Unlock() + if !a.checked { + return errors.New("reviewer resume was not checked") + } + return a.boundaryErr +} + func (s failingNamedSessionStore) UpsertNamedSession(context.Context, ledger.NamedSession) error { return s.upsertErr } From 1db9505284d49148dd7445ece1d5cb3b909a69e1 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 21:57:51 -0400 Subject: [PATCH 4/7] fix: persist durable reviewer sessions --- internal/llmadapters/subprocess_test.go | 38 +++++++++++++++++++++---- internal/pipeline/pipeline_test.go | 20 +++++++++---- internal/workbench/workbench.go | 1 + 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index 3fc8dac..ccb38f6 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -890,22 +890,44 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { t.Fatalf("WriteFile(repo): %v", err) } adapter := newCodexHelperAdapter("tool-success", recordPath, 5*time.Second, "CGO_CFLAGS=-O2", "CGO_CXXFLAGS=-stdlib=libc++") - stream, err := adapter.Resume(context.Background(), "prior-session", Request{ - Model: "gpt-5.5", - Effort: "high", - Prompt: "resume prompt", + request := Request{ + Model: "gpt-5.5", + Effort: "high", + Prompt: "initial prompt", + DurableSession: true, ReviewerWorkspace: &ReviewerWorkspaceRequest{ RepoDir: repoRoot, ScratchDir: scratchRoot, }, - }) + } + initialStream, err := adapter.Start(context.Background(), request) + if err != nil { + t.Fatalf("Start: %v", err) + } + if _, err := initialStream.Wait(context.Background()); err != nil { + t.Fatalf("initial Wait: %v", err) + } + if initialStream.SessionID() != "session-1" { + t.Fatalf("initial SessionID = %q, want session-1", initialStream.SessionID()) + } + request.Prompt = "resume prompt" + stream, err := adapter.Resume(context.Background(), initialStream.SessionID(), request) if err != nil { t.Fatalf("Resume: %v", err) } if _, err := stream.Wait(context.Background()); err != nil { t.Fatalf("Wait: %v", err) } - record := readHelperRecord(t, recordPath) + records := readHelperRecords(t, recordPath) + if len(records) != 2 { + t.Fatalf("helper records = %d, want durable start and resume", len(records)) + } + initialRecord, record := records[0], records[1] + if containsFlag(initialRecord.AdapterArgs, "--ephemeral") { + t.Fatalf("initial args = %#v, durable reviewer must not use --ephemeral", initialRecord.AdapterArgs) + } + assertFlagValue(t, initialRecord.AdapterArgs, "--sandbox", "workspace-write") + assertFlagValue(t, initialRecord.AdapterArgs, "--cd", repoRoot) if !samePath(t, record.Cwd, repoRoot) || record.CwdEntries != 1 { t.Fatalf("resume cwd = %q entries = %d, want workspace %q with checkout", record.Cwd, record.CwdEntries, repoRoot) } @@ -917,6 +939,10 @@ func TestSubprocessCodexSafetyModes(t *testing.T) { t.Fatalf("resume args = %#v, do not want unsupported %s", record.AdapterArgs, flag) } } + promptIndex := len(argsBeforePrompt(record.AdapterArgs)) + if promptIndex < 1 || record.AdapterArgs[promptIndex-1] != initialStream.SessionID() { + t.Fatalf("resume args = %#v, want exact session %q", record.AdapterArgs, initialStream.SessionID()) + } for name, path := range map[string]string{ "TMPDIR": record.TMPDir, "GOTMPDIR": record.GoTmpDir, "GOCACHE": record.GoCache, "XDG_CACHE_HOME": record.XDGCacheHome, } { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 83cb6d0..a544444 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -3304,6 +3304,15 @@ func TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun(t *testing.T) if err != nil { t.Fatalf("DryRun: %v", err) } + var durableReviewerStart bool + for _, request := range dryAdapter.Requests() { + if request.ReviewerWorkspace != nil { + durableReviewerStart = request.DurableSession + } + } + if !durableReviewerStart { + t.Fatal("initial reviewer request was not durable for resume-capable adapter") + } if dryResult.NamedSessionCandidate == nil { t.Fatal("dry-run candidate = nil") } @@ -3547,12 +3556,13 @@ func TestLiveNamedSessionMissingRowStartsFreshAndReturnsCandidate(t *testing.T) t.Fatalf("resumes = %#v, want rollup resume from fresh selection", resumes) } requests := adapter.Requests() - if len(requests) < 1 || !requests[0].DurableSession { - t.Fatalf("requests = %#v, want durable selection start on first named-session run", requests) + if len(requests) != 2 { + t.Fatalf("requests = %#v, want selection and reviewer starts", requests) } - for i := 1; i < len(requests); i++ { - if requests[i].DurableSession { - t.Fatalf("requests[%d] = %#v, do not want durable non-selection requests", i, requests[i]) + for i, request := range requests { + wantDurable := i == 0 || request.ReviewerWorkspace != nil + if request.DurableSession != wantDurable { + t.Fatalf("requests[%d] = %#v, DurableSession = %t, want %t for selection/reviewer starts only", i, request, request.DurableSession, wantDurable) } } if result.NamedSessionCandidate == nil { diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 53f8aac..72835f3 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -351,6 +351,7 @@ func PrepareReviewerRequest(ctx context.Context, deps Deps, adapter llm.Adapter, Effort: effort, Prompt: prompt, LogPath: logPath, + DurableSession: adapter.SupportsResume(), ReviewerWorkspace: &workspace, OnValidationRetry: func(req *llm.Request) error { if err := cleanupCurrent(); err != nil { From 8867e1d0b18f7c3079a7d5d2b643df20c28dbfe4 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 22:06:13 -0400 Subject: [PATCH 5/7] fix: keep resumable LLM sessions durable --- internal/llmlifecycle/lifecycle.go | 1 + internal/llmlifecycle/lifecycle_test.go | 27 +++++++++++++++ internal/pipeline/pipeline_test.go | 34 +++++++++++++++++++ .../threadanalysis/threadanalysis_test.go | 4 +++ internal/workbench/workbench.go | 1 - 5 files changed, 66 insertions(+), 1 deletion(-) diff --git a/internal/llmlifecycle/lifecycle.go b/internal/llmlifecycle/lifecycle.go index f92a9b6..1062f8c 100644 --- a/internal/llmlifecycle/lifecycle.go +++ b/internal/llmlifecycle/lifecycle.go @@ -293,6 +293,7 @@ func RunStructured[T any](ctx context.Context, req Request, decode llm.Decoder[T request.Effort = req.Effort request.Prompt = req.Prompt request.LogPath = req.LogPath + request.DurableSession = request.DurableSession || req.Adapter.SupportsResume() structured, runErr := llm.RunStructuredWithSessionResume(ctx, req.Adapter, resumeSessionID, request, decode) completed := now() draft := SessionDraft{ diff --git a/internal/llmlifecycle/lifecycle_test.go b/internal/llmlifecycle/lifecycle_test.go index f4763d1..73b3d4b 100644 --- a/internal/llmlifecycle/lifecycle_test.go +++ b/internal/llmlifecycle/lifecycle_test.go @@ -339,6 +339,33 @@ func TestRunStructuredSessionPersistenceFailureLeavesNoMetadata(t *testing.T) { } } +func TestRunStructuredFreshSessionDurabilityFollowsResumeSupport(t *testing.T) { + for _, tt := range []struct { + name string + supportsResume bool + }{ + {name: "resumable", supportsResume: true}, + {name: "non-resumable"}, + } { + t.Run(tt.name, func(t *testing.T) { + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: tt.supportsResume} + adapter.Queue(llm.FakeResult{ + SessionID: "provider-session-1", + Response: llm.Response{StructuredOutput: []byte(`{"ok":true}`)}, + }) + req := lifecycleRequest(t, newLifecycleStore(), adapter) + + if _, err := RunStructured(context.Background(), req, decodeLifecyclePayload); err != nil { + t.Fatalf("RunStructured: %v", err) + } + requests := adapter.Requests() + if len(requests) != 1 || requests[0].DurableSession != tt.supportsResume { + t.Fatalf("requests = %#v, DurableSession want %t", requests, tt.supportsResume) + } + }) + } +} + func TestRunStructuredRejectsStaleMetadataBeforeProviderCall(t *testing.T) { ctx := context.Background() store := newLifecycleStore() diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index a544444..f2e38bf 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -2408,6 +2408,40 @@ func TestRunStructuredTaskRejectsAdapterMismatchBeforeRetry(t *testing.T) { } } +func TestRunStructuredTaskStartsDurableOrchestratorAndResumesExactSession(t *testing.T) { + ctx := context.Background() + adapter := &llm.FakeAdapter{NameValue: "fake-llm", SupportsResumeValue: true} + adapter.Queue(fakeLLMResult("rollup-session", `{"ok":true}`, 1, 1)) + adapter.Queue(fakeLLMResult("continued-session", `{"ok":true}`, 1, 1)) + opts := Options{Adapter: adapter, Now: fixedNow, NewSessionRowID: sequence("session")} + first := llmTaskSpec{ + taskID: "orchestrator-rollup-first", phase: "rollup", allowNoRunCache: true, + inputFingerprint: "first", artifacts: ArtifactPathsFromDir(t.TempDir()), role: ledger.SessionRoleOrchestrator, + model: "model", effort: "medium", prompt: "prompt", + } + + _, firstSession, _, err := runStructuredTask(ctx, opts, first, func(data []byte) (bool, error) { return len(data) > 0, nil }) + if err != nil { + t.Fatalf("first runStructuredTask: %v", err) + } + requests := adapter.Requests() + if len(requests) != 1 || !requests[0].DurableSession { + t.Fatalf("fresh orchestrator requests = %#v, want durable start", requests) + } + second := first + second.taskID = "orchestrator-rollup-second" + second.inputFingerprint = "second" + second.artifacts = ArtifactPathsFromDir(t.TempDir()) + second.resumeSessionID = firstSession.ProviderReportedSessionID + if _, _, _, err := runStructuredTask(ctx, opts, second, func(data []byte) (bool, error) { return len(data) > 0, nil }); err != nil { + t.Fatalf("second runStructuredTask: %v", err) + } + resumes := adapter.Resumes() + if len(resumes) != 1 || resumes[0].SessionID != "rollup-session" { + t.Fatalf("orchestrator resumes = %#v, want exact rollup-session", resumes) + } +} + func TestRunStructuredTaskRejectsDependencyTaskIDMismatchBeforeRetry(t *testing.T) { ctx := context.Background() artifacts := ArtifactPathsFromDir(t.TempDir()) diff --git a/internal/threadanalysis/threadanalysis_test.go b/internal/threadanalysis/threadanalysis_test.go index afb53a3..4ee9c73 100644 --- a/internal/threadanalysis/threadanalysis_test.go +++ b/internal/threadanalysis/threadanalysis_test.go @@ -188,6 +188,10 @@ func TestAnalyzeThreadsCheckpointsProviderSessionBeforeReturningFailure(t *testi if !reflect.DeepEqual(checkpoints, []string{"thread-session-failed"}) { t.Fatalf("session checkpoints = %#v, want failed provider session", checkpoints) } + requests := adapter.Requests() + if len(requests) != 1 || !requests[0].DurableSession { + t.Fatalf("fresh thread-analysis requests = %#v, want durable start", requests) + } } func TestAnalyzeThreadsPropagatesSessionCheckpointFailure(t *testing.T) { diff --git a/internal/workbench/workbench.go b/internal/workbench/workbench.go index 72835f3..53f8aac 100644 --- a/internal/workbench/workbench.go +++ b/internal/workbench/workbench.go @@ -351,7 +351,6 @@ func PrepareReviewerRequest(ctx context.Context, deps Deps, adapter llm.Adapter, Effort: effort, Prompt: prompt, LogPath: logPath, - DurableSession: adapter.SupportsResume(), ReviewerWorkspace: &workspace, OnValidationRetry: func(req *llm.Request) error { if err := cleanupCurrent(); err != nil { From 45fe3c8d3ae11f47b96b71abb6c42a6b3450c51a Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 22:17:14 -0400 Subject: [PATCH 6/7] fix: preserve the orchestrator conversation chain --- internal/llmlifecycle/lifecycle.go | 34 ++++++++++++++++++++++++++++ internal/pipeline/pipeline.go | 36 +++++++++++++++++------------- internal/pipeline/pipeline_test.go | 32 +++++++++++++++++++++++--- 3 files changed, 84 insertions(+), 18 deletions(-) diff --git a/internal/llmlifecycle/lifecycle.go b/internal/llmlifecycle/lifecycle.go index 1062f8c..747a7e9 100644 --- a/internal/llmlifecycle/lifecycle.go +++ b/internal/llmlifecycle/lifecycle.go @@ -540,6 +540,40 @@ func ReadMetadata(paths Paths, taskID string) (Metadata, bool, error) { ) } +// ListMetadata lists committed task metadata in stable task-directory order. +func ListMetadata(paths Paths) ([]Metadata, error) { + if strings.TrimSpace(paths.LLMTasksDir) == "" { + return nil, fmt.Errorf("llmlifecycle: task directory is required") + } + entries, err := os.ReadDir(paths.LLMTasksDir) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("llmlifecycle: list task metadata: %w", err) + } + metadata := make([]Metadata, 0, len(entries)) + for _, entry := range entries { + if !entry.IsDir() { + continue + } + path := filepath.Join(paths.LLMTasksDir, entry.Name(), "metadata.json") + data, err := os.ReadFile(path) // #nosec G304 -- path is contained under the caller-owned task root. + if errors.Is(err, os.ErrNotExist) { + continue + } + if err != nil { + return nil, fmt.Errorf("llmlifecycle: read task metadata %q: %w", entry.Name(), err) + } + var meta Metadata + if err := json.Unmarshal(data, &meta); err != nil { + return nil, fmt.Errorf("llmlifecycle: decode task metadata %q: %w", entry.Name(), err) + } + metadata = append(metadata, meta) + } + return metadata, nil +} + func readMetadata(paths Paths, taskID, readError, decodeError string) (Metadata, bool, error) { path, err := paths.Metadata(taskID) if err != nil { diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index cc86961..a950c10 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -767,7 +767,7 @@ func executeLLMPhases(ctx context.Context, opts Options, req Request, mode execu return executionPhaseFailure(err) } if reusedCohort { - if err := restoreOrchestratorSessionFromRun(ctx, opts.Store, run.RunID, &namedSession); err != nil { + if err := restoreOrchestratorSessionFromRun(ctx, opts.Store, run.RunID, prepared.artifacts, &namedSession); err != nil { return nil, false, err } } @@ -1734,30 +1734,36 @@ func persistReviewerCohort(ctx context.Context, opts Options, req Request, scope return store.ReplaceReviewerCohort(ctx, cohort) } -type runSessionStore interface { - ListSessionsForRun(context.Context, string) ([]ledger.Session, error) -} - -func restoreOrchestratorSessionFromRun(ctx context.Context, store Store, runID string, state *namedSessionState) error { - sessions, ok := store.(runSessionStore) - if !ok || state == nil || !state.supportsResume { +func restoreOrchestratorSessionFromRun(ctx context.Context, store Store, runID string, artifacts ArtifactPaths, state *namedSessionState) error { + if state == nil || !state.supportsResume { return nil } - rows, err := sessions.ListSessionsForRun(ctx, runID) + metadata, err := llmlifecycle.ListMetadata(lifecyclePaths(artifacts)) if err != nil { return err } - var latest *ledger.Session - for i := range rows { - row := &rows[i] - if row.Role != ledger.SessionRoleOrchestrator || strings.TrimSpace(row.ProviderSessionID) == "" { + var latest ledger.Session + found := false + for _, meta := range metadata { + if meta.TaskID != orchestratorSelectionStage && meta.TaskID != orchestratorRollupStage && meta.Phase != string(stagemodel.StageThreadAnalysis) { continue } - if latest == nil || row.StartedAt.After(latest.StartedAt) { + if strings.TrimSpace(meta.SessionRowID) == "" { + continue + } + row, err := store.GetSession(ctx, meta.SessionRowID) + if err != nil { + return fmt.Errorf("pipeline: restore orchestrator task %q session %q: %w", meta.TaskID, meta.SessionRowID, err) + } + if row.RunID != runID || row.Role != ledger.SessionRoleOrchestrator || strings.TrimSpace(row.ProviderSessionID) == "" { + continue + } + if !found || row.StartedAt.After(latest.StartedAt) { latest = row + found = true } } - if latest != nil { + if found { state.currentProviderSessionID = latest.ProviderSessionID } return nil diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index f2e38bf..e6e1bf0 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -5000,23 +5000,49 @@ func TestPersistReviewerCohortTreatsFilesOnlyAssignmentAsScoped(t *testing.T) { } } -func TestRestoreOrchestratorSessionFromInterruptedRunUsesLatestSession(t *testing.T) { +func TestRestoreOrchestratorSessionFromInterruptedRunUsesLatestChainSession(t *testing.T) { store := openPipelineStore(t) defer closeStore(t, store) provider, req := dryRunHarness(t) run := allocateLiveRun(t, store, provider, req, "interrupted-orchestrator") + artifacts := ArtifactPathsFromDir(run.ArtifactPath) for _, session := range []ledger.Session{ {SessionRowID: "selection", RunID: run.RunID, ProviderSessionID: "selection-session", Role: ledger.SessionRoleOrchestrator, Adapter: "fake", Model: "model", StartedAt: fixedNow()}, - {SessionRowID: "thread", RunID: run.RunID, ProviderSessionID: "thread-session", Role: ledger.SessionRoleOrchestrator, Adapter: "fake", Model: "model", StartedAt: fixedNow().Add(time.Second)}, + {SessionRowID: "dossier", RunID: run.RunID, ProviderSessionID: "dossier-session", Role: ledger.SessionRoleOrchestrator, Adapter: "fake", Model: "model", StartedAt: fixedNow().Add(time.Second)}, } { if err := store.InsertSession(context.Background(), session); err != nil { t.Fatalf("InsertSession: %v", err) } } + for _, meta := range []llmlifecycle.Metadata{ + {TaskID: orchestratorSelectionStage, Phase: "selection", SessionRowID: "selection"}, + {TaskID: dossierSummaryTaskID, Phase: "dossier", SessionRowID: "dossier"}, + } { + if err := llmlifecycle.WriteMetadata(lifecyclePaths(artifacts), meta); err != nil { + t.Fatalf("WriteMetadata(%s): %v", meta.TaskID, err) + } + } state := namedSessionState{enabled: true, supportsResume: true, currentProviderSessionID: "older-session"} - if err := restoreOrchestratorSessionFromRun(context.Background(), store, run.RunID, &state); err != nil { + if err := restoreOrchestratorSessionFromRun(context.Background(), store, run.RunID, artifacts, &state); err != nil { t.Fatalf("restoreOrchestratorSessionFromRun: %v", err) } + if state.resumeID() != "selection-session" { + t.Fatalf("restored orchestrator session = %q, want selection session instead of newer dossier", state.resumeID()) + } + if err := store.InsertSession(context.Background(), ledger.Session{ + SessionRowID: "thread", RunID: run.RunID, ProviderSessionID: "thread-session", Role: ledger.SessionRoleOrchestrator, + Adapter: "fake", Model: "model", StartedAt: fixedNow().Add(2 * time.Second), + }); err != nil { + t.Fatalf("InsertSession(thread): %v", err) + } + if err := llmlifecycle.WriteMetadata(lifecyclePaths(artifacts), llmlifecycle.Metadata{ + TaskID: "thread-analysis-thread-1", Phase: string(stagemodel.StageThreadAnalysis), SessionRowID: "thread", + }); err != nil { + t.Fatalf("WriteMetadata(thread): %v", err) + } + if err := restoreOrchestratorSessionFromRun(context.Background(), store, run.RunID, artifacts, &state); err != nil { + t.Fatalf("restoreOrchestratorSessionFromRun after thread: %v", err) + } if state.resumeID() != "thread-session" { t.Fatalf("restored orchestrator session = %q, want latest thread session", state.resumeID()) } From 45e3ab137c054fef727c9238981720e158fac29f Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 22:29:09 -0400 Subject: [PATCH 7/7] fix: address resumable review feedback --- README.md | 41 +++++++++++++++----------- docs/development.md | 4 +-- internal/pipeline/pipeline.go | 46 +++++++----------------------- internal/pipeline/pipeline_test.go | 24 ++++++++++++++++ 4 files changed, 61 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index ad96e07..bd90a0c 100644 --- a/README.md +++ b/README.md @@ -746,13 +746,15 @@ cr review --fail-on major https://github.com/OWNER/REPO/pull/123 ``` Force a fresh local live review instead of using existing approval, override, -resume, or marker gates while continuing the PR's provider session: +resume, or marker gates while continuing the PR's orchestrator and reviewer +sessions: ```bash cr review --rerun https://github.com/OWNER/REPO/pull/123 ``` -Start both a fresh local review and a fresh provider conversation: +Start both a fresh local review and fresh orchestrator and reviewer +conversations, including reviewer reselection: ```bash cr review --rerun --fresh-session https://github.com/OWNER/REPO/pull/123 @@ -774,8 +776,9 @@ cr review https://github.com/OWNER/REPO/pull/123 ``` By default, dry-run and live reviews of the same PR, profile, and posting -identity reuse one provider session across pushes. Override that scope with a -named session for a series of related live reviews: +identity reuse one orchestrator session and one PR-scoped reviewer cohort +across pushes. Override only the orchestrator scope with a named session for a +series of related live reviews: ```bash cr review --session release-train https://github.com/OWNER/REPO/pull/123 @@ -1111,9 +1114,9 @@ Modes: |------|-----------| | `--dry-run` | Plan review actions, write local artifacts, and print the plan without posting. | | `--no-post` | Alias for `--dry-run`. | -| `--rerun` | Bypass existing local approval, approval-override, resume, and marker gates and start a new live review while retaining provider-session reuse. Mutually exclusive with `--retry-posts`. | +| `--rerun` | Bypass existing local approval, approval-override, resume, and marker gates and start a new live review while reusing the PR's original reviewer cohort plus reviewer and orchestrator sessions. Mutually exclusive with `--retry-posts`. | | `--retry-posts` | Retry missing or failed required posts for an existing run without rerunning LLM planning or checking approval overrides. Mutually exclusive with `--rerun` and incompatible with `--session`. | -| `--fresh-session` | Start a fresh provider conversation for this invocation without changing local review gates. Incompatible with `--retry-posts`, which does not run LLM planning. | +| `--fresh-session` | Reset the PR-scoped reviewer cohort, reselect reviewers, and start fresh reviewer and orchestrator conversations without changing local review gates. Incompatible with `--retry-posts`, which does not run LLM planning. | | `--fast` | Enable fast execution for reviewer agents, overriding the profile default. Incompatible with `--retry-posts`. | | `--no-fast` | Disable fast execution for reviewer agents, overriding the profile default. Mutually exclusive with `--fast`. | @@ -1132,7 +1135,7 @@ Review selection and execution flags: | `--reviewer-effort ` | Override reviewer-stage effort only with `low`, `medium`, or `high`. Requires `--dry-run` or `--no-post`. | | `--review-base-sha ` | Review this base commit SHA instead of the PR's current base SHA. Requires `--review-head-sha` and `--dry-run` or `--no-post`. | | `--review-head-sha ` | Review this head commit SHA instead of the PR's current head SHA. Requires `--review-base-sha` and `--dry-run` or `--no-post`. | -| `--session ` | Override the default PR/profile/posting-identity scope with a named LLM session for live reviews. Not allowed with `--dry-run`, `--no-post`, or `--retry-posts`. | +| `--session ` | Override the PR's default orchestrator session with a named live-review session. Reviewer cohorts remain PR-scoped. Not allowed with `--dry-run`, `--no-post`, or `--retry-posts`. | Review progress on stderr reports the merged reviewer catalog, final selected IDs and reasoning, and each reviewer assignment with winning provenance and @@ -1168,11 +1171,13 @@ whether it was ignored as unsupported, and the speed actually reported by the provider, or `unknown` when unavailable. Local run state and provider session state are independent. By default, each -PR/profile/posting-identity tuple gets one durable provider session shared by -dry-run and live reviews and retained when the PR head changes. `--session` -selects an explicit named live-review session instead. `--fresh-session` -skips provider resume for one invocation and replaces that scope's durable -session after successful planning (and, for live review, successful posting). +PR/profile/posting-identity tuple gets one durable orchestrator session and one +ordered reviewer cohort whose members retain their own provider sessions. Plain +follow-up reviews and `--rerun` reuse that cohort and those exact sessions even +when the PR head changes. `--session` selects an explicit named orchestrator +session only; the reviewer cohort remains PR-scoped. `--fresh-session` clears +both scopes for the invocation, runs selection again, and persists the new +orchestrator session and reviewer cohort. Live review uses a local gate before planning or posting. If the posting identity has already approved the PR, `cr review` exits immediately in Go code @@ -1323,7 +1328,8 @@ builds the comparison model, and writes JSON and Markdown artifacts. cr sessions list [--json] ``` -Lists named LLM sessions in name order. Text output shows name, profile, +Lists named orchestrator sessions in name order. Reviewer cohorts are automatic +PR-scoped state and are not listed here. Text output shows name, profile, provider, adapter, model, host, and last-used time. JSON output includes the provider session ID plus created and last-used timestamps. @@ -1333,8 +1339,8 @@ provider session ID plus created and last-used timestamps. cr sessions show [--json] ``` -Shows one named LLM session. Missing sessions return an error. Text output -includes the provider session ID. +Shows one named orchestrator session. Missing sessions return an error. Text +output includes the provider session ID. ### `cr sessions delete` @@ -1342,8 +1348,9 @@ includes the provider session ID. cr sessions delete [--json] ``` -Deletes one named LLM session row. It does not delete provider-side session -state. Missing sessions return an error. +Deletes one named orchestrator session row. It does not delete provider-side +session state or the PR-scoped reviewer cohort. Missing sessions return an +error. `sessions delete` emits progress on stderr for layout resolution, legacy migration, ledger open, and session deletion. diff --git a/docs/development.md b/docs/development.md index ecf4ebd..84a4814 100644 --- a/docs/development.md +++ b/docs/development.md @@ -8,8 +8,8 @@ Collective standards and automation remain canonical in their own repositories. codereview-cli is the Open CLI Collective code-review CLI and ships the `cr` binary. It provides configuration and credential commands, trusted-agent inspection, dry-run and live pull-request review orchestration, inline thread -response handling through `cr respond`, named LLM session management, and local -data lifecycle commands. +response handling through `cr respond`, named orchestrator sessions, automatic +PR-scoped reviewer cohorts, and local data lifecycle commands. The current Go code is a Cobra command tree in `internal/cmd/*` with a thin `cmd/cr` entrypoint, shared exit-code mapping in `internal/cmd/exitcode`, and diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index a950c10..6f73af4 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -75,18 +75,19 @@ type Store interface { InsertSession(context.Context, ledger.Session) error GetSession(context.Context, string) (ledger.Session, error) InsertPlanningResult(context.Context, []ledger.Finding, []ledger.PlannedAction) error + InsertPlannedActions(context.Context, []ledger.PlannedAction) error + MergePlanningResult(context.Context, []ledger.Finding, []ledger.PlannedAction) error ListFindings(context.Context, string) ([]ledger.Finding, error) ListPlannedActions(context.Context, string) ([]ledger.PlannedAction, error) - CompleteRun(context.Context, string, ledger.Outcome, time.Time) error -} - -type reviewerCohortStore interface { GetReviewerCohort(context.Context, ledger.ReviewerCohortScope) (ledger.ReviewerCohort, error) ReplaceReviewerCohort(context.Context, ledger.ReviewerCohort) error UpdateReviewerCohortSession(context.Context, ledger.ReviewerCohortScope, string, string, time.Time) error DeleteReviewerCohort(context.Context, ledger.ReviewerCohortScope) error + CompleteRun(context.Context, string, ledger.Outcome, time.Time) error } +var _ Store = (*ledger.Store)(nil) + // NamedSessionStore persists cross-run LLM sessions. type NamedSessionStore interface { GetNamedSession(context.Context, string) (ledger.NamedSession, error) @@ -940,11 +941,7 @@ func persistExecutionResult(ctx context.Context, opts Options, req Request, run return err } if hasPersistedPlanning { - store, ok := opts.Store.(checkpointPlanningStore) - if !ok { - return fmt.Errorf("pipeline: checkpoint planning store is required") - } - if err := store.MergePlanningResult(ctx, ledgerFindings, plannedActions); err != nil { + if err := opts.Store.MergePlanningResult(ctx, ledgerFindings, plannedActions); err != nil { return err } plannedActions, err = opts.Store.ListPlannedActions(ctx, run.RunID) @@ -1324,11 +1321,6 @@ func analyzeReviewThreads(ctx context.Context, opts Options, req Request, run le return threadanalysis.ResponseActions(results), nil } -type checkpointPlanningStore interface { - InsertPlannedActions(context.Context, []ledger.PlannedAction) error - MergePlanningResult(context.Context, []ledger.Finding, []ledger.PlannedAction) error -} - func checkpointThreadResponses(ctx context.Context, opts Options, req Request, mode executionMode, run ledger.Run, caps reviewplan.ProviderCaps, responses []review.ThreadResponseAction) ([]ledger.PlannedAction, error) { existing, err := opts.Store.ListPlannedActions(ctx, run.RunID) if err != nil { @@ -1353,11 +1345,7 @@ func checkpointThreadResponses(ctx context.Context, opts Options, req Request, m for _, action := range plan.Actions { actions = append(actions, ledger.PlannedAction{Action: action.Action, RunID: run.RunID}) } - store, ok := opts.Store.(checkpointPlanningStore) - if !ok { - return nil, fmt.Errorf("pipeline: checkpoint planning store is required") - } - if err := store.InsertPlannedActions(ctx, actions); err != nil { + if err := opts.Store.InsertPlannedActions(ctx, actions); err != nil { return nil, err } } @@ -1672,17 +1660,13 @@ func rebaseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.Rev } func loadReviewerCohort(ctx context.Context, opts Options, req Request, scope ledger.ReviewerCohortScope, catalog agents.Catalog, changedFiles []string, maxAgents int) (llm.Selection, map[string]string, bool, error) { - store, ok := opts.Store.(reviewerCohortStore) - if !ok { - return llm.Selection{}, nil, false, fmt.Errorf("pipeline: reviewer cohort store is required") - } if req.FreshSession { - if err := store.DeleteReviewerCohort(ctx, scope); err != nil && !errors.Is(err, ledger.ErrNotFound) { + if err := opts.Store.DeleteReviewerCohort(ctx, scope); err != nil && !errors.Is(err, ledger.ErrNotFound) { return llm.Selection{}, nil, false, err } return llm.Selection{}, nil, false, nil } - cohort, err := store.GetReviewerCohort(ctx, scope) + cohort, err := opts.Store.GetReviewerCohort(ctx, scope) if errors.Is(err, ledger.ErrNotFound) { return llm.Selection{}, nil, false, nil } @@ -1703,10 +1687,6 @@ func persistReviewerCohort(ctx context.Context, opts Options, req Request, scope if len(selection.SelectedAgents) == 0 { return nil } - store, ok := opts.Store.(reviewerCohortStore) - if !ok { - return fmt.Errorf("pipeline: reviewer cohort store is required") - } cohort := ledger.ReviewerCohort{Scope: scope, Adapter: opts.Adapter.Name(), CreatedAt: now, UpdatedAt: now} for _, selected := range selection.SelectedAgents { agent, ok := catalog.Find(selected.AgentID) @@ -1731,7 +1711,7 @@ func persistReviewerCohort(ctx context.Context, opts Options, req Request, scope Fast: req.ReviewerFast, }) } - return store.ReplaceReviewerCohort(ctx, cohort) + return opts.Store.ReplaceReviewerCohort(ctx, cohort) } func restoreOrchestratorSessionFromRun(ctx context.Context, store Store, runID string, artifacts ArtifactPaths, state *namedSessionState) error { @@ -2018,11 +1998,7 @@ func runReviewer(ctx context.Context, opts Options, req Request, runID string, p }) }) if providerSessionID := strings.TrimSpace(session.ProviderReportedSessionID); providerSessionID != "" && strings.TrimSpace(resumeState.scope.PRKey) != "" { - store, ok := opts.Store.(reviewerCohortStore) - if !ok { - return llm.Findings{}, session, ledgerSession, nil, fmt.Errorf("pipeline: reviewer cohort store is required") - } - if updateErr := store.UpdateReviewerCohortSession(ctx, resumeState.scope, agent.ID, providerSessionID, opts.now()); updateErr != nil { + if updateErr := opts.Store.UpdateReviewerCohortSession(ctx, resumeState.scope, agent.ID, providerSessionID, opts.now()); updateErr != nil { return llm.Findings{}, session, ledgerSession, nil, updateErr } } diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index e6e1bf0..b18582b 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -7086,6 +7086,14 @@ func (noopStore) InsertPlanningResult(context.Context, []ledger.Finding, []ledge return nil } +func (noopStore) InsertPlannedActions(context.Context, []ledger.PlannedAction) error { + return nil +} + +func (noopStore) MergePlanningResult(context.Context, []ledger.Finding, []ledger.PlannedAction) error { + return nil +} + func (noopStore) ListFindings(context.Context, string) ([]ledger.Finding, error) { return nil, nil } @@ -7094,6 +7102,22 @@ func (noopStore) ListPlannedActions(context.Context, string) ([]ledger.PlannedAc return nil, nil } +func (noopStore) GetReviewerCohort(context.Context, ledger.ReviewerCohortScope) (ledger.ReviewerCohort, error) { + return ledger.ReviewerCohort{}, ledger.ErrNotFound +} + +func (noopStore) ReplaceReviewerCohort(context.Context, ledger.ReviewerCohort) error { + return nil +} + +func (noopStore) UpdateReviewerCohortSession(context.Context, ledger.ReviewerCohortScope, string, string, time.Time) error { + return nil +} + +func (noopStore) DeleteReviewerCohort(context.Context, ledger.ReviewerCohortScope) error { + return nil +} + func (noopStore) CompleteRun(context.Context, string, ledger.Outcome, time.Time) error { return nil }