From 7cf45b07303a033042b91476320967edcd3a3e9e Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 2 Nov 2026 18:12:41 -0500 Subject: [PATCH 1/3] fix(review): run required_on_match reviewers on reused cohorts A follow-up review that reuses the PR's saved reviewer cohort only rebased the saved members, so a required_on_match reviewer added after the first review never ran until --fresh-session, a required member could lose new matches to another member, and a new file only a missing required reviewer matches failed the rerun as uncovered. Reuse now applies ensureRequiredOnMatchAgents and ensureSelectedGlobCoverage like fresh selection, appends missing required reviewers to the cohort, and persists it before reviewers start so their sessions are recorded and resumed. Closes #701 Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- README.md | 5 +- docs/review-guidance.md | 8 +- docs/review-lifecycle.md | 17 +- internal/pipeline/pipeline.go | 131 +++++++++++- internal/pipeline/required_cohort_test.go | 243 ++++++++++++++++++++++ 5 files changed, 396 insertions(+), 8 deletions(-) create mode 100644 internal/pipeline/required_cohort_test.go diff --git a/README.md b/README.md index 209e154e..10b52b8c 100644 --- a/README.md +++ b/README.md @@ -490,7 +490,10 @@ Set `required_on_match: true` when an agent must run whenever `file_globs` matches a changed file. This applies to profile, repo, and `--agents-dir` sources; invalid globs fail catalog loading, and an explicit `--max-agents` smaller than the combined set of applicable repo-local and matching -`required_on_match` reviewers fails rather than dropping reviewers. +`required_on_match` reviewers fails rather than dropping reviewers. The +guarantee holds on follow-up reviews that reuse the PR's saved reviewer cohort: +a matching required reviewer the cohort lacks joins it, and a required member +reviews every matching file. Legacy agent files that use `model: sonnet` or another provider-specific `model` value must be updated. Replace portable intent with `model_tier` diff --git a/docs/review-guidance.md b/docs/review-guidance.md index e92eca21..f8992e18 100644 --- a/docs/review-guidance.md +++ b/docs/review-guidance.md @@ -32,9 +32,11 @@ declaring it is rejected as invalid rather than colliding with that pass. Any source may set `required_on_match: true` with `file_globs`. Those reviewers are added deterministically whenever a glob matches a changed file, even if the -orchestrator omits them. An explicit maximum smaller than the combined set of -applicable repo-local and matching `required_on_match` reviewers fails instead -of silently dropping required coverage. +orchestrator omits them, and on follow-up reviews that reuse the PR's saved +reviewer cohort, even if the cohort was selected before they matched. An +explicit maximum smaller than the combined set of applicable repo-local and +matching `required_on_match` reviewers fails instead of silently dropping +required coverage. `cr review` does not load repo guidance from the PR head for the same review run. It reads `.codereview/agents/` from the PR base branch, pins that source diff --git a/docs/review-lifecycle.md b/docs/review-lifecycle.md index cfd18d52..eee5dd0c 100644 --- a/docs/review-lifecycle.md +++ b/docs/review-lifecycle.md @@ -48,15 +48,26 @@ when its conversation still exists; a saved session whose conversation is gone falls back to a fresh conversation instead of failing the reviewer (see the failure behavior below). +The one way a reused cohort grows is `required_on_match`. Every catalog +reviewer marked `required_on_match` whose globs match a current changed file +runs, scoped to all of its matching files, exactly as a fresh selection would +schedule it. A required reviewer the saved cohort lacks — because a later push +touched files it must see, or because it joined the catalog after the first +review — is appended to the cohort and persisted before reviewers start, so its +provider session is recorded and resumed on later passes. A required reviewer +already in the cohort has its assignment widened to every matching file rather +than leaving new matches to whichever member rebasing reached first. + `--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, and it starts new provider conversations only through the same missing-conversation fallback. Reuse fails with `--fresh-session` guidance when a cohort member is missing or -runtime-incompatible, `--max-agents` is smaller than the saved cohort, or a -changed file is coverable by a current catalog agent (broad with no file globs -or matching a file glob) but no saved cohort member can take it. If no current +runtime-incompatible, `--max-agents` is smaller than the saved cohort plus any +required reviewers joining it, or a changed file is coverable by a current +catalog agent (broad with no file globs or matching a file glob) but neither a +saved cohort member nor a joining required reviewer can take it. If no current catalog agent can cover a changed file, reuse leaves it unassigned and the downstream coverage gate reports `incomplete_unassigned` without forcing a fresh session. diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index affd8f6a..77de93c0 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "maps" "os" "path/filepath" "regexp" @@ -1862,6 +1863,10 @@ func rebaseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.Rev break } if !assigned { + if requiredOnMatchCovers(catalog, file) { + // joinRequiredReviewers adds the required reviewer that owns it. + continue + } for _, current := range catalog.Agents { if len(current.FileGlobs) == 0 || globsMatchFile(current.FileGlobs, file) { return freshError("saved reviewer cohort leaves changed file %q uncovered", file) @@ -1922,16 +1927,140 @@ func loadReviewerCohort(ctx context.Context, opts Options, req Request, scope le if err != nil { return llm.Selection{}, nil, false, err } - selection, resumes, err := rebaseReviewerCohort(req, catalog, cohort, changedFiles, maxAgents, opts.Adapter.Name()) + selection, resumes, grown, err := reuseReviewerCohort(req, catalog, cohort, changedFiles, maxAgents, opts.Adapter.Name(), opts.now()) if err != nil { return llm.Selection{}, nil, false, Failure(FailureTerminal, err) } + if grown != nil { + if err := opts.Store.ReplaceReviewerCohort(ctx, *grown); err != nil { + return llm.Selection{}, nil, false, err + } + } if !opts.Adapter.SupportsResume() { resumes = nil } return selection, resumes, true, nil } +// reuseReviewerCohort rebases a saved cohort onto the current change and joins +// the required_on_match reviewers it lacks. It returns the selection to run, +// the provider session to resume per reviewer, and — when a reviewer joined — +// the grown cohort the caller must persist before any reviewer runs. +func reuseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.ReviewerCohort, changedFiles []string, maxAgents int, adapter string, now time.Time) (llm.Selection, map[string]string, *ledger.ReviewerCohort, error) { + selection, resumes, err := rebaseReviewerCohort(req, catalog, cohort, changedFiles, maxAgents, adapter) + if err != nil { + return llm.Selection{}, nil, nil, err + } + selection, grown, err := joinRequiredReviewers(req, catalog, cohort, selection, changedFiles, maxAgents, now) + if err != nil { + return llm.Selection{}, nil, nil, err + } + return selection, cohortResumes(cohort, selection, resumes), grown, nil +} + +// requiredOnMatchCovers reports whether a required_on_match catalog reviewer +// matches file, so the file has an owner even when no saved member covers it. +func requiredOnMatchCovers(catalog agents.Catalog, file string) bool { + for _, candidate := range catalog.Agents { + if len(requiredOnMatchFiles(candidate, []string{file})) > 0 { + return true + } + } + return false +} + +// joinRequiredReviewers brings a reused cohort up to the required_on_match +// contract that a fresh selection enforces: every catalog reviewer marked +// required_on_match whose globs match a changed file runs, scoped to all of +// its matches. A cohort is saved at a PR's first review, so without this a +// reviewer that became required later — a later push touched files it must +// see, or the agent joined the catalog after that first review — would not +// run on the PR again until someone passed --fresh-session, and a required +// member already in the cohort could see a new matching file handed to +// another member instead. +// +// When the cohort gains a member, the grown cohort is returned for the caller +// to persist, so the newcomer's provider session is recorded and resumed on +// later passes like any original member's. +func joinRequiredReviewers(req Request, catalog agents.Catalog, cohort ledger.ReviewerCohort, selection llm.Selection, changedFiles []string, maxAgents int, now time.Time) (llm.Selection, *ledger.ReviewerCohort, error) { + freshError := func(format string, args ...any) (llm.Selection, *ledger.ReviewerCohort, error) { + return llm.Selection{}, nil, fmt.Errorf("pipeline: "+format+"; pass --fresh-session to select a new reviewer cohort", args...) + } + members := make(map[string]ledger.ReviewerCohortMember, len(cohort.Members)) + for _, member := range cohort.Members { + members[member.AgentID] = member + } + selection = ensureRequiredOnMatchAgents(selection, catalog, changedFiles) + selection = ensureSelectedGlobCoverage(selection, catalog, changedFiles) + var joined []llm.SelectedAgent + for _, selected := range selection.SelectedAgents { + if _, ok := members[selected.AgentID]; !ok { + joined = append(joined, selected) + } + } + if len(joined) == 0 { + return selection, nil, nil + } + if maxAgents > 0 && len(selection.SelectedAgents) > maxAgents { + return freshError("--max-agents %d is smaller than the saved reviewer cohort plus the %d required reviewers it lacks", maxAgents, len(joined)) + } + effectiveFast, _, err := resolveReviewerFastMode(req, selection, catalog) + if err != nil { + return llm.Selection{}, nil, err + } + for _, selected := range selection.SelectedAgents { + if member, ok := members[selected.AgentID]; ok && member.Fast != effectiveFast { + return freshError("saved reviewer %q fast-mode setting is incompatible with the required reviewers joining the cohort", selected.AgentID) + } + } + grown := cohort + grown.Members = append([]ledger.ReviewerCohortMember(nil), cohort.Members...) + grown.UpdatedAt = now + for _, selected := range joined { + agent, ok := catalog.Find(selected.AgentID) + if !ok { + return llm.Selection{}, nil, fmt.Errorf("pipeline: required reviewer %q not found while growing reviewer cohort", selected.AgentID) + } + runtimeConfig, err := resolveReviewerRuntimeConfig(req, agent) + if err != nil { + return llm.Selection{}, nil, err + } + grown.Members = append(grown.Members, ledger.ReviewerCohortMember{ + AgentID: selected.AgentID, + AssignmentMode: ledger.ReviewerAssignmentScoped, + Files: append([]string(nil), selected.Files...), + AllowedFiles: append([]string(nil), selected.AllowedFiles...), + Model: runtimeConfig.model, + Effort: runtimeConfig.effort, + Fast: effectiveFast, + }) + } + return selection, &grown, nil +} + +// cohortResumes returns the provider session to resume for each selected +// reviewer that has one saved in the cohort. A member that rebasing dropped +// for lack of scope and a required reviewer then brought back resumes its own +// conversation rather than starting over. +func cohortResumes(cohort ledger.ReviewerCohort, selection llm.Selection, resumes map[string]string) map[string]string { + out := make(map[string]string, len(resumes)) + maps.Copy(out, resumes) + for _, selected := range selection.SelectedAgents { + if _, ok := out[selected.AgentID]; ok { + continue + } + for _, member := range cohort.Members { + if member.AgentID == selected.AgentID { + if sessionID := strings.TrimSpace(member.ProviderSessionID); sessionID != "" { + out[selected.AgentID] = sessionID + } + break + } + } + } + return out +} + 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 diff --git a/internal/pipeline/required_cohort_test.go b/internal/pipeline/required_cohort_test.go new file mode 100644 index 00000000..efe9903e --- /dev/null +++ b/internal/pipeline/required_cohort_test.go @@ -0,0 +1,243 @@ +package pipeline + +import ( + "context" + "encoding/json" + "reflect" + "slices" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/agents" + "github.com/open-cli-collective/codereview-cli/internal/ledger" + "github.com/open-cli-collective/codereview-cli/internal/llm" + "github.com/open-cli-collective/codereview-cli/internal/runlifecycle" + "github.com/open-cli-collective/codereview-cli/internal/statepaths" +) + +func requiredCohortCatalog() agents.Catalog { + return agents.Catalog{Agents: []agents.Agent{ + {ID: "lang:go", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}, Provenance: agents.Provenance{Kind: agents.SourceProfile}}, + {ID: "privacy:data", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*", "!**/*.lock"}, RequiredOnMatch: true, Provenance: agents.Provenance{Kind: agents.SourceProfile}}, + }} +} + +func goOnlyCohort() ledger.ReviewerCohort { + return ledger.ReviewerCohort{ + Scope: ledger.ReviewerCohortScope{PRKey: "github.com/o/r#1", Profile: "default", PostingIdentity: "bot"}, + Adapter: "fake-llm", CreatedAt: fixedNow(), UpdatedAt: fixedNow(), + Members: []ledger.ReviewerCohortMember{{ + AgentID: "lang:go", AssignmentMode: ledger.ReviewerAssignmentScoped, + Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}, + Model: "claude-sonnet-5-5", Effort: "medium", ProviderSessionID: "go-session", + }}, + } +} + +func TestJoinRequiredReviewersAddsMissingRequiredReviewerAndGrowsCohort(t *testing.T) { + catalog := requiredCohortCatalog() + cohort := goOnlyCohort() + req := Request{Profile: testProfile(""), ProfileName: "default"} + changed := []string{"main.go", "tests/fixtures/loans.json", "Cargo.lock"} + + rebased, _, err := rebaseReviewerCohort(req, catalog, cohort, changed, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + selection, grown, err := joinRequiredReviewers(req, catalog, cohort, rebased, changed, 0, fixedNow()) + if err != nil { + t.Fatalf("joinRequiredReviewers: %v", err) + } + + var ids []string + for _, selected := range selection.SelectedAgents { + ids = append(ids, selected.AgentID) + } + if want := []string{"lang:go", "privacy:data"}; !reflect.DeepEqual(ids, want) { + t.Fatalf("selected agents = %#v, want saved member then joined required reviewer %#v", ids, want) + } + joined := selection.SelectedAgents[1] + wantFiles := []string{"main.go", "tests/fixtures/loans.json"} + if !reflect.DeepEqual(joined.Files, wantFiles) || !reflect.DeepEqual(joined.AllowedFiles, wantFiles) { + t.Fatalf("joined assignment = %#v, want every glob match %#v", joined, wantFiles) + } + if grown == nil { + t.Fatal("grown cohort = nil, want the joined reviewer persisted") + } + if len(grown.Members) != 2 || !reflect.DeepEqual(grown.Members[0], cohort.Members[0]) { + t.Fatalf("grown members = %#v, want the saved member unchanged first", grown.Members) + } + added := grown.Members[1] + if added.AgentID != "privacy:data" || added.AssignmentMode != ledger.ReviewerAssignmentScoped || + added.Model != "claude-sonnet-5-5" || added.Effort != "medium" || added.ProviderSessionID != "" || + !reflect.DeepEqual(added.Files, wantFiles) { + t.Fatalf("added member = %#v, want scoped resolved member without a session", added) + } + if grown.Scope != cohort.Scope || !grown.CreatedAt.Equal(cohort.CreatedAt) { + t.Fatalf("grown cohort identity = %#v, want the saved cohort's scope and creation time", grown) + } +} + +func TestJoinRequiredReviewersWidensSavedRequiredMemberToNewMatches(t *testing.T) { + catalog := requiredCohortCatalog() + cohort := goOnlyCohort() + cohort.Members = append(cohort.Members, ledger.ReviewerCohortMember{ + AgentID: "privacy:data", AssignmentMode: ledger.ReviewerAssignmentScoped, + Files: []string{"main.go"}, AllowedFiles: []string{"main.go"}, + Model: "claude-sonnet-5-5", Effort: "medium", ProviderSessionID: "privacy-session", + }) + req := Request{Profile: testProfile(""), ProfileName: "default"} + // handler.go is new since the cohort was saved; rebasing hands it to the + // first member whose globs match, which is not the required reviewer. + changed := []string{"main.go", "handler.go"} + + rebased, _, err := rebaseReviewerCohort(req, catalog, cohort, changed, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + selection, grown, err := joinRequiredReviewers(req, catalog, cohort, rebased, changed, 0, fixedNow()) + if err != nil { + t.Fatalf("joinRequiredReviewers: %v", err) + } + if grown != nil { + t.Fatalf("grown cohort = %#v, want nil when no reviewer joined", grown) + } + for _, selected := range selection.SelectedAgents { + if selected.AgentID == "privacy:data" { + if !reflect.DeepEqual(selected.Files, changed) || !reflect.DeepEqual(selected.AllowedFiles, changed) { + t.Fatalf("required member assignment = %#v, want every match %#v", selected, changed) + } + return + } + } + t.Fatalf("selection = %#v, want the saved required member", selection.SelectedAgents) +} + +func TestJoinRequiredReviewersLeavesSatisfiedCohortUnchanged(t *testing.T) { + catalog := agents.Catalog{Agents: requiredCohortCatalog().Agents[:1]} + cohort := goOnlyCohort() + req := Request{Profile: testProfile(""), ProfileName: "default"} + changed := []string{"main.go"} + + rebased, _, err := rebaseReviewerCohort(req, catalog, cohort, changed, 0, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + selection, grown, err := joinRequiredReviewers(req, catalog, cohort, rebased, changed, 0, fixedNow()) + if err != nil { + t.Fatalf("joinRequiredReviewers: %v", err) + } + if grown != nil || !reflect.DeepEqual(selection, rebased) { + t.Fatalf("selection = %#v grown = %#v, want the rebased selection untouched", selection, grown) + } +} + +func TestJoinRequiredReviewersRejectsMaxAgentsBelowGrownCohort(t *testing.T) { + catalog := requiredCohortCatalog() + cohort := goOnlyCohort() + req := Request{Profile: testProfile(""), ProfileName: "default"} + changed := []string{"main.go"} + + rebased, _, err := rebaseReviewerCohort(req, catalog, cohort, changed, 1, "fake-llm") + if err != nil { + t.Fatalf("rebaseReviewerCohort: %v", err) + } + _, _, err = joinRequiredReviewers(req, catalog, cohort, rebased, changed, 1, fixedNow()) + if err == nil || !strings.Contains(err.Error(), "--max-agents 1") || !strings.Contains(err.Error(), "--fresh-session") { + t.Fatalf("error = %v, want a max-agents conflict that points at --fresh-session", err) + } +} + +func TestCohortResumesRestoresSessionForReturningMember(t *testing.T) { + cohort := goOnlyCohort() + cohort.Members = append(cohort.Members, ledger.ReviewerCohortMember{AgentID: "privacy:data", ProviderSessionID: "privacy-session"}) + selection := llm.Selection{SelectedAgents: []llm.SelectedAgent{{AgentID: "lang:go"}, {AgentID: "privacy:data"}, {AgentID: "new:agent"}}} + + got := cohortResumes(cohort, selection, map[string]string{"lang:go": "go-session"}) + + want := map[string]string{"lang:go": "go-session", "privacy:data": "privacy-session"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("resumes = %#v, want %#v", got, want) + } +} + +// A required reviewer added to the catalog after a PR's first review must run +// on the next plain rerun, not only after --fresh-session, and must be saved +// into the cohort so its provider session can be recorded. +func TestPlainRerunRunsRequiredReviewerAddedAfterFirstReview(t *testing.T) { + ctx := context.Background() + store := openPipelineStore(t) + defer closeStore(t, store) + agentDir := t.TempDir() + writeRequiredAgentForGlob(t, agentDir, "go", "**/*.go") + trustCurrentTempFixtures(t) + provider, req := dryRunHarness(t) + removeRepoAgentFixture(provider) + req.Profile.AgentSources = []string{agentDir} + layout := statepaths.NewLayout(t.TempDir(), t.TempDir()) + actionIDs := actionSequence() + options := func(adapter llm.Adapter, runID string) Options { + return Options{ + Provider: provider, Adapter: adapter, Store: store, Layout: layout, Now: fixedNow, + NewRunID: func() string { return runID }, NewSessionRowID: sequence(runID + "-session"), + NewFindingID: findingSequence(runID + "-finding"), NewActionID: actionIDs, + MaxConcurrency: 1, + } + } + + first := &metadataHeavyAdapter{FakeAdapter: &llm.FakeAdapter{NameValue: "fake-llm"}} + if _, err := dryRunForTest(ctx, options(first, "run-first"), req); err != nil { + t.Fatalf("first DryRun: %v", err) + } + + writeRequiredAgentForGlob(t, agentDir, "privacy", "**/*") + second := &metadataHeavyAdapter{FakeAdapter: &llm.FakeAdapter{NameValue: "fake-llm"}} + if _, err := dryRunForTest(ctx, options(second, "run-second"), req); err != nil { + t.Fatalf("rerun DryRun: %v", err) + } + + var schemas, reviewed []string + for _, request := range second.Requests() { + var payload struct { + Schema string `json:"schema"` + Assignment reviewerPromptAssignment `json:"assignment"` + } + if err := json.Unmarshal([]byte(request.Prompt), &payload); err != nil { + t.Fatalf("decode rerun prompt: %v", err) + } + schemas = append(schemas, payload.Schema) + if payload.Schema == "findings" { + reviewed = append(reviewed, payload.Assignment.AgentID) + } + } + for _, schema := range schemas { + if schema == "selection" { + t.Fatalf("rerun schemas = %#v, want the saved cohort reused without a new selection", schemas) + } + } + slices.Sort(reviewed) + if want := []string{"harness:go", "harness:privacy"}; !reflect.DeepEqual(reviewed, want) { + t.Fatalf("rerun reviewers = %#v, want saved member plus joined required reviewer %#v", reviewed, want) + } + + prKey, err := statepaths.PRKey(req.PRRef.Host, req.PRRef.Owner, req.PRRef.Repo, req.PRRef.Number) + if err != nil { + t.Fatalf("PRKey: %v", err) + } + cohort, err := store.GetReviewerCohort(ctx, ledger.ReviewerCohortScope{ + PRKey: prKey, Profile: req.ProfileName, PostingIdentity: runlifecycle.PostingKey(req.PostingIdentity), + }) + if err != nil { + t.Fatalf("GetReviewerCohort: %v", err) + } + var members []string + for _, member := range cohort.Members { + members = append(members, member.AgentID) + } + if want := []string{"harness:go", "harness:privacy"}; !reflect.DeepEqual(members, want) { + t.Fatalf("cohort members = %#v, want the joined reviewer saved %#v", members, want) + } + if got := cohort.Members[1].ProviderSessionID; got != "metadata-heavy-reviewer-harness-privacy" { + t.Fatalf("joined member session = %q, want the rerun's reviewer session recorded", got) + } +} From 86527588076c2027ec487cc9f95350f49fa4945e Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 2 Nov 2026 18:24:42 -0500 Subject: [PATCH 2/3] fix(review): keep reused coverage whole when required reviewers join Widen a saved required member's assignment instead of replacing it with its glob matches, run the uncovered-file check on the final reused selection, count the whole saved cohort against --max-agents, and test the fast-mode conflict path. Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- README.md | 2 +- docs/review-lifecycle.md | 28 +++--- internal/pipeline/pipeline.go | 100 ++++++++++++++++------ internal/pipeline/pipeline_test.go | 12 +-- internal/pipeline/required_cohort_test.go | 95 ++++++++++++++++++++ 5 files changed, 194 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 10b52b8c..5bae76d7 100644 --- a/README.md +++ b/README.md @@ -493,7 +493,7 @@ smaller than the combined set of applicable repo-local and matching `required_on_match` reviewers fails rather than dropping reviewers. The guarantee holds on follow-up reviews that reuse the PR's saved reviewer cohort: a matching required reviewer the cohort lacks joins it, and a required member -reviews every matching file. +gains every matching file. Legacy agent files that use `model: sonnet` or another provider-specific `model` value must be updated. Replace portable intent with `model_tier` diff --git a/docs/review-lifecycle.md b/docs/review-lifecycle.md index eee5dd0c..45f74241 100644 --- a/docs/review-lifecycle.md +++ b/docs/review-lifecycle.md @@ -50,13 +50,14 @@ failure behavior below). The one way a reused cohort grows is `required_on_match`. Every catalog reviewer marked `required_on_match` whose globs match a current changed file -runs, scoped to all of its matching files, exactly as a fresh selection would -schedule it. A required reviewer the saved cohort lacks — because a later push -touched files it must see, or because it joined the catalog after the first -review — is appended to the cohort and persisted before reviewers start, so its -provider session is recorded and resumed on later passes. A required reviewer -already in the cohort has its assignment widened to every matching file rather -than leaving new matches to whichever member rebasing reached first. +runs with all of its matching files in its assignment, as a fresh selection +would schedule it. A required reviewer the saved cohort lacks — because a later +push touched files it must see, or because it joined the catalog after the +first review — is appended to the cohort and persisted before reviewers start, +so its provider session is recorded and resumed on later passes. A required +reviewer already in the cohort keeps the files rebasing gave it and gains every +matching file, rather than leaving new matches to whichever member rebasing +reached first. `--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 @@ -65,12 +66,15 @@ missing-conversation fallback. Reuse fails with `--fresh-session` guidance when a cohort member is missing or runtime-incompatible, `--max-agents` is smaller than the saved cohort plus any -required reviewers joining it, or a changed file is coverable by a current +required reviewers joining it, joining required reviewers would change the +saved members' fast-mode setting, or a changed file is coverable by a current catalog agent (broad with no file globs or matching a file glob) but neither a -saved cohort member nor a joining required reviewer can take it. If no current -catalog agent can cover a changed file, reuse leaves it unassigned and the -downstream coverage gate reports `incomplete_unassigned` without forcing a -fresh session. +saved cohort member nor a joining required reviewer takes it. The coverage +check runs on the final reused selection, after required reviewers join. +Generated lockfiles are exempt from it, as they are from reviewer coverage. If +no current catalog agent can cover a changed file, reuse leaves it unassigned +and the downstream coverage gate reports `incomplete_unassigned` without +forcing a fresh session. ## `--fresh-session` diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 77de93c0..24b9f28b 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -1851,7 +1851,9 @@ func rebaseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.Rev if covered[file] { continue } - assigned := false + // A file no saved member can take stays unassigned here; + // reuseReviewerCohort decides whether a joining required reviewer + // covers it or the cohort must be reselected. for i := range candidates { broadWithoutExclusions := candidates[i].member.AssignmentMode == ledger.ReviewerAssignmentBroad && !candidates[i].globs.HasExclusions() if !broadWithoutExclusions && !candidates[i].globs.Matches(file) { @@ -1859,20 +1861,8 @@ func rebaseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.Rev } candidates[i].files = append(candidates[i].files, file) covered[file] = true - assigned = true break } - if !assigned { - if requiredOnMatchCovers(catalog, file) { - // joinRequiredReviewers adds the required reviewer that owns it. - continue - } - for _, current := range catalog.Agents { - if len(current.FileGlobs) == 0 || globsMatchFile(current.FileGlobs, file) { - return freshError("saved reviewer cohort leaves changed file %q uncovered", file) - } - } - } } selection := llm.Selection{Reasoning: "reused reviewer cohort"} @@ -1955,24 +1945,84 @@ func reuseReviewerCohort(req Request, catalog agents.Catalog, cohort ledger.Revi if err != nil { return llm.Selection{}, nil, nil, err } + if file, uncovered := reusedCohortCoverageGap(catalog, selection, changedFiles); uncovered { + return llm.Selection{}, nil, nil, fmt.Errorf("pipeline: saved reviewer cohort leaves changed file %q uncovered; pass --fresh-session to select a new reviewer cohort", file) + } return selection, cohortResumes(cohort, selection, resumes), grown, nil } -// requiredOnMatchCovers reports whether a required_on_match catalog reviewer -// matches file, so the file has an owner even when no saved member covers it. -func requiredOnMatchCovers(catalog agents.Catalog, file string) bool { +// reusedCohortCoverageGap returns the first changed file that the final reused +// selection leaves without a reviewer although a current catalog agent could +// review it — one with no file globs, or whose globs match. Such a file needs a +// reselected cohort. A file no catalog agent can cover stays unassigned for the +// coverage gate to report, and generated lockfiles are no coverage obligation. +// The check runs on the final selection, after required reviewers join, so it +// judges the reviewers that will actually run. +func reusedCohortCoverageGap(catalog agents.Catalog, selection llm.Selection, changedFiles []string) (string, bool) { + covered := map[string]bool{} + for _, selected := range selection.SelectedAgents { + for _, file := range reviewerAssignmentScope(selected, changedFiles) { + covered[file] = true + } + } + for _, file := range changedFiles { + if covered[file] || isGeneratedLockfile(file) { + continue + } + for _, candidate := range catalog.Agents { + if len(candidate.FileGlobs) == 0 || globsMatchFile(candidate.FileGlobs, file) { + return file, true + } + } + } + return "", false +} + +// widenRequiredOnMatchAgents adds every changed file a required_on_match +// reviewer matches to its assignment, appending the reviewer when it is not +// selected. Unlike ensureRequiredOnMatchAgents, which replaces a selected +// required reviewer's scope with its matches, it keeps what the reviewer +// already holds: on reuse that scope came from the saved cohort, and a file a +// broad saved member was reviewing must not lose its owner. +func widenRequiredOnMatchAgents(selection llm.Selection, catalog agents.Catalog, changedFiles []string) llm.Selection { for _, candidate := range catalog.Agents { - if len(requiredOnMatchFiles(candidate, []string{file})) > 0 { - return true + matched := requiredOnMatchFiles(candidate, changedFiles) + if len(matched) == 0 { + continue + } + index := slices.IndexFunc(selection.SelectedAgents, func(selected llm.SelectedAgent) bool { + return selected.AgentID == candidate.ID + }) + if index < 0 { + selection.SelectedAgents = append(selection.SelectedAgents, llm.SelectedAgent{ + AgentID: candidate.ID, + Rationale: "required_on_match matched changed files", + Files: append([]string(nil), matched...), + AllowedFiles: append([]string(nil), matched...), + }) + continue + } + selected := &selection.SelectedAgents[index] + if len(selected.Files) == 0 && len(selected.AllowedFiles) == 0 { + // Already scoped to every changed file. + continue + } + for _, file := range matched { + if !slices.Contains(selected.Files, file) { + selected.Files = append(selected.Files, file) + } + if len(selected.AllowedFiles) > 0 && !slices.Contains(selected.AllowedFiles, file) { + selected.AllowedFiles = append(selected.AllowedFiles, file) + } } } - return false + return selection } // joinRequiredReviewers brings a reused cohort up to the required_on_match // contract that a fresh selection enforces: every catalog reviewer marked -// required_on_match whose globs match a changed file runs, scoped to all of -// its matches. A cohort is saved at a PR's first review, so without this a +// required_on_match whose globs match a changed file runs, with all of its +// matches in its assignment. A cohort is saved at a PR's first review, so without this a // reviewer that became required later — a later push touched files it must // see, or the agent joined the catalog after that first review — would not // run on the PR again until someone passed --fresh-session, and a required @@ -1990,7 +2040,7 @@ func joinRequiredReviewers(req Request, catalog agents.Catalog, cohort ledger.Re for _, member := range cohort.Members { members[member.AgentID] = member } - selection = ensureRequiredOnMatchAgents(selection, catalog, changedFiles) + selection = widenRequiredOnMatchAgents(selection, catalog, changedFiles) selection = ensureSelectedGlobCoverage(selection, catalog, changedFiles) var joined []llm.SelectedAgent for _, selected := range selection.SelectedAgents { @@ -2001,8 +2051,10 @@ func joinRequiredReviewers(req Request, catalog agents.Catalog, cohort ledger.Re if len(joined) == 0 { return selection, nil, nil } - if maxAgents > 0 && len(selection.SelectedAgents) > maxAgents { - return freshError("--max-agents %d is smaller than the saved reviewer cohort plus the %d required reviewers it lacks", maxAgents, len(joined)) + // The cap applies to the cohort that will be saved, which keeps members + // this pass does not run because nothing in their scope changed. + if maxAgents > 0 && len(cohort.Members)+len(joined) > maxAgents { + return freshError("--max-agents %d is smaller than the saved reviewer cohort of %d plus the %d required reviewers it lacks", maxAgents, len(cohort.Members), len(joined)) } effectiveFast, _, err := resolveReviewerFastMode(req, selection, catalog) if err != nil { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index a99f2fc2..58f7dacd 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -7390,7 +7390,7 @@ func TestRebaseReviewerCohortLeavesInherentlyUnmatchedFilesUnassigned(t *testing } } -func TestRebaseReviewerCohortRejectsFileCoveredByBroadCatalogAgent(t *testing.T) { +func TestReuseReviewerCohortRejectsFileCoveredByBroadCatalogAgent(t *testing.T) { req := Request{Profile: testProfile(""), ProfileName: "default"} cohort := ledger.ReviewerCohort{Adapter: "fake-llm", Members: []ledger.ReviewerCohortMember{{ AgentID: "repo:go", AssignmentMode: ledger.ReviewerAssignmentScoped, @@ -7402,9 +7402,9 @@ func TestRebaseReviewerCohortRejectsFileCoveredByBroadCatalogAgent(t *testing.T) {ID: "shared:general", ModelTier: "medium", Effort: "medium"}, }} - _, _, err := rebaseReviewerCohort(req, catalog, cohort, []string{"main.go", ".github/workflows/ci.yml"}, 0, "fake-llm") + _, _, _, err := reuseReviewerCohort(req, catalog, cohort, []string{"main.go", ".github/workflows/ci.yml"}, 0, "fake-llm", fixedNow()) if err == nil || !strings.Contains(err.Error(), ".github/workflows/ci.yml") || !strings.Contains(err.Error(), "--fresh-session") { - t.Fatalf("rebaseReviewerCohort error = %v, want broad catalog coverage to retain fresh-session guidance", err) + t.Fatalf("reuseReviewerCohort error = %v, want broad catalog coverage to retain fresh-session guidance", err) } } @@ -7531,7 +7531,7 @@ func TestRestoreOrchestratorSessionFromInterruptedRunUsesLatestChainSession(t *t } } -func TestRebaseReviewerCohortRejectsIncompatibleOrUncoveredState(t *testing.T) { +func TestReuseReviewerCohortRejectsIncompatibleOrUncoveredState(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-5-5", Effort: "medium", @@ -7563,9 +7563,9 @@ func TestRebaseReviewerCohortRejectsIncompatibleOrUncoveredState(t *testing.T) { tc.maxAgents = 1 tc.wantDetail = "--max-agents" } - _, _, err := rebaseReviewerCohort(req, candidateCatalog, candidate, tc.files, tc.maxAgents, tc.adapter) + _, _, _, err := reuseReviewerCohort(req, candidateCatalog, candidate, tc.files, tc.maxAgents, tc.adapter, fixedNow()) 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) + t.Fatalf("reuseReviewerCohort error = %v, want %q and fresh-session guidance", err, tc.wantDetail) } }) } diff --git a/internal/pipeline/required_cohort_test.go b/internal/pipeline/required_cohort_test.go index efe9903e..7a9adda5 100644 --- a/internal/pipeline/required_cohort_test.go +++ b/internal/pipeline/required_cohort_test.go @@ -241,3 +241,98 @@ func TestPlainRerunRunsRequiredReviewerAddedAfterFirstReview(t *testing.T) { t.Fatalf("joined member session = %q, want the rerun's reviewer session recorded", got) } } + +// A saved broad member that is also required_on_match keeps the files rebasing +// gave it: joining widens a required member's assignment, never narrows it to +// its glob matches, so a file it was reviewing cannot lose its only owner. +func TestReuseReviewerCohortKeepsFilesABroadRequiredMemberHeld(t *testing.T) { + catalog := agents.Catalog{Agents: []agents.Agent{ + {ID: "repo:req", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.go"}, RequiredOnMatch: true, Provenance: agents.Provenance{Kind: agents.SourceProfile}}, + {ID: "repo:docs", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.md"}, Provenance: agents.Provenance{Kind: agents.SourceProfile}}, + }} + cohort := goOnlyCohort() + cohort.Members = []ledger.ReviewerCohortMember{{ + AgentID: "repo:req", AssignmentMode: ledger.ReviewerAssignmentBroad, + Model: "claude-sonnet-5-5", Effort: "medium", ProviderSessionID: "req-session", + }} + req := Request{Profile: testProfile(""), ProfileName: "default"} + changed := []string{"main.go", "README.md"} + + selection, _, grown, err := reuseReviewerCohort(req, catalog, cohort, changed, 0, "fake-llm", fixedNow()) + if err != nil { + t.Fatalf("reuseReviewerCohort: %v", err) + } + if grown != nil { + t.Fatalf("grown cohort = %#v, want nil when no reviewer joined", grown) + } + if len(selection.SelectedAgents) != 1 || !reflect.DeepEqual(selection.SelectedAgents[0].Files, changed) { + t.Fatalf("selection = %#v, want the broad required member to keep every file %#v", selection.SelectedAgents, changed) + } +} + +// The cap counts the cohort that will be saved. A member this pass drops for +// an empty assignment stays in the saved cohort, so it counts too; otherwise +// one run would persist a cohort larger than the cap and the next would fail. +func TestJoinRequiredReviewersCapCountsMembersThisPassDoesNotRun(t *testing.T) { + catalog := requiredCohortCatalog() + catalog.Agents = append(catalog.Agents, agents.Agent{ID: "lang:ts", ModelTier: "medium", Effort: "medium", FileGlobs: []string{"**/*.ts"}, Provenance: agents.Provenance{Kind: agents.SourceProfile}}) + cohort := goOnlyCohort() + cohort.Members = append(cohort.Members, ledger.ReviewerCohortMember{ + AgentID: "lang:ts", AssignmentMode: ledger.ReviewerAssignmentScoped, + Files: []string{"app.ts"}, AllowedFiles: []string{"app.ts"}, + Model: "claude-sonnet-5-5", Effort: "medium", + }) + req := Request{Profile: testProfile(""), ProfileName: "default"} + + _, _, grown, err := reuseReviewerCohort(req, catalog, cohort, []string{"main.go"}, 2, "fake-llm", fixedNow()) + if err == nil || !strings.Contains(err.Error(), "--max-agents 2 is smaller than the saved reviewer cohort of 2 plus the 1 required reviewers") || !strings.Contains(err.Error(), "--fresh-session") { + t.Fatalf("error = %v, want the cap to count the dropped member and point at --fresh-session", err) + } + if grown != nil { + t.Fatalf("grown cohort = %#v, want nothing to persist on error", grown) + } +} + +func TestJoinRequiredReviewersFastMode(t *testing.T) { + const fastModel = "claude-opus-4-8" + for _, tc := range []struct { + name string + requiredFast bool + wantErr bool + }{ + {name: "joiner without fast support conflicts with fast saved member", requiredFast: false, wantErr: true}, + {name: "fast-capable joiner is saved with fast mode", requiredFast: true, wantErr: false}, + } { + t.Run(tc.name, func(t *testing.T) { + catalog := requiredCohortCatalog() + catalog.Agents[0].ModelID = fastModel + if tc.requiredFast { + catalog.Agents[1].ModelID = fastModel + } + req := Request{Profile: testProfile(""), ProfileName: "default", ReviewerFast: true} + goRuntime, err := resolveReviewerRuntimeConfig(req, catalog.Agents[0]) + if err != nil { + t.Fatalf("resolveReviewerRuntimeConfig: %v", err) + } + cohort := goOnlyCohort() + cohort.Members[0].Model, cohort.Members[0].Effort, cohort.Members[0].Fast = goRuntime.model, goRuntime.effort, true + + _, _, grown, err := reuseReviewerCohort(req, catalog, cohort, []string{"main.go"}, 0, "fake-llm", fixedNow()) + if tc.wantErr { + if err == nil || !strings.Contains(err.Error(), "fast-mode setting") || !strings.Contains(err.Error(), "--fresh-session") { + t.Fatalf("error = %v, want a fast-mode conflict that points at --fresh-session", err) + } + if grown != nil { + t.Fatalf("grown cohort = %#v, want nothing to persist on error", grown) + } + return + } + if err != nil { + t.Fatalf("reuseReviewerCohort: %v", err) + } + if grown == nil || len(grown.Members) != 2 || grown.Members[1].AgentID != "privacy:data" || !grown.Members[1].Fast { + t.Fatalf("grown cohort = %#v, want the joined reviewer saved with fast mode", grown) + } + }) + } +} From 5fe715d284a30beb378c5a5190a6b0f243da140e Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 2 Nov 2026 18:37:26 -0500 Subject: [PATCH 3/3] fix(review): check fast mode across the whole grown cohort and test the lockfile exemption Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- internal/pipeline/pipeline.go | 8 +++--- internal/pipeline/required_cohort_test.go | 32 ++++++++++++++++++++--- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/internal/pipeline/pipeline.go b/internal/pipeline/pipeline.go index 24b9f28b..74163bdc 100644 --- a/internal/pipeline/pipeline.go +++ b/internal/pipeline/pipeline.go @@ -2060,9 +2060,11 @@ func joinRequiredReviewers(req Request, catalog agents.Catalog, cohort ledger.Re if err != nil { return llm.Selection{}, nil, err } - for _, selected := range selection.SelectedAgents { - if member, ok := members[selected.AgentID]; ok && member.Fast != effectiveFast { - return freshError("saved reviewer %q fast-mode setting is incompatible with the required reviewers joining the cohort", selected.AgentID) + // Every saved member is compared, not only those running this pass: the + // grown cohort is saved whole, and one fast-mode setting must hold for it. + for _, member := range cohort.Members { + if member.Fast != effectiveFast { + return freshError("saved reviewer %q fast-mode setting is incompatible with the required reviewers joining the cohort", member.AgentID) } } grown := cohort diff --git a/internal/pipeline/required_cohort_test.go b/internal/pipeline/required_cohort_test.go index 7a9adda5..7fb19873 100644 --- a/internal/pipeline/required_cohort_test.go +++ b/internal/pipeline/required_cohort_test.go @@ -298,10 +298,15 @@ func TestJoinRequiredReviewersFastMode(t *testing.T) { for _, tc := range []struct { name string requiredFast bool + savedFast bool + changed []string wantErr bool }{ - {name: "joiner without fast support conflicts with fast saved member", requiredFast: false, wantErr: true}, - {name: "fast-capable joiner is saved with fast mode", requiredFast: true, wantErr: false}, + {name: "joiner without fast support conflicts with fast saved member", requiredFast: false, savedFast: true, changed: []string{"main.go"}, wantErr: true}, + {name: "fast-capable joiner is saved with fast mode", requiredFast: true, savedFast: true, changed: []string{"main.go"}, wantErr: false}, + // The saved member does not run this pass, but the grown cohort is + // saved whole, so its setting must still agree with the joiner's. + {name: "fast joiner conflicts with a normal-speed member this pass does not run", requiredFast: true, savedFast: false, changed: []string{"data.json"}, wantErr: true}, } { t.Run(tc.name, func(t *testing.T) { catalog := requiredCohortCatalog() @@ -315,9 +320,9 @@ func TestJoinRequiredReviewersFastMode(t *testing.T) { t.Fatalf("resolveReviewerRuntimeConfig: %v", err) } cohort := goOnlyCohort() - cohort.Members[0].Model, cohort.Members[0].Effort, cohort.Members[0].Fast = goRuntime.model, goRuntime.effort, true + cohort.Members[0].Model, cohort.Members[0].Effort, cohort.Members[0].Fast = goRuntime.model, goRuntime.effort, tc.savedFast - _, _, grown, err := reuseReviewerCohort(req, catalog, cohort, []string{"main.go"}, 0, "fake-llm", fixedNow()) + _, _, grown, err := reuseReviewerCohort(req, catalog, cohort, tc.changed, 0, "fake-llm", fixedNow()) if tc.wantErr { if err == nil || !strings.Contains(err.Error(), "fast-mode setting") || !strings.Contains(err.Error(), "--fresh-session") { t.Fatalf("error = %v, want a fast-mode conflict that points at --fresh-session", err) @@ -336,3 +341,22 @@ func TestJoinRequiredReviewersFastMode(t *testing.T) { }) } } + +// Generated lockfiles are no coverage obligation, so one that no saved member +// covers does not force a reselected cohort even when a broad catalog agent +// could review it. +func TestReuseReviewerCohortExemptsUncoveredLockfile(t *testing.T) { + catalog := agents.Catalog{Agents: []agents.Agent{ + requiredCohortCatalog().Agents[0], + {ID: "shared:general", ModelTier: "medium", Effort: "medium", Provenance: agents.Provenance{Kind: agents.SourceProfile}}, + }} + req := Request{Profile: testProfile(""), ProfileName: "default"} + + selection, _, grown, err := reuseReviewerCohort(req, catalog, goOnlyCohort(), []string{"main.go", "Cargo.lock"}, 0, "fake-llm", fixedNow()) + if err != nil { + t.Fatalf("reuseReviewerCohort: %v, want the uncovered lockfile exempt", err) + } + if grown != nil || len(selection.SelectedAgents) != 1 || !reflect.DeepEqual(selection.SelectedAgents[0].Files, []string{"main.go"}) { + t.Fatalf("selection = %#v grown = %#v, want the saved member alone on main.go", selection.SelectedAgents, grown) + } +}