Repository navigation
fix(review): run required_on_match reviewers on reused cohorts - #703
Conversation
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
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 7cf45b07303a
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| documentation:docs | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| go:implementation | 0 |
| architecture:solid | 2 |
go:implementation-tests (1 finding)
Minor - internal/pipeline/pipeline.go:2013
The new fast-mode conflict branch in joinRequiredReviewers (saved member Fast != effectiveFast once required reviewers join) has no test. It is a distinct failure path from the --max-agents rejection and is the only guard against silently running a saved member with a different fast setting than it was persisted with. A regression, such as dropping the check or comparing the wrong value, would not be caught. Fix: add a focused table case in required_cohort_test.go with a saved member whose Fast differs from the resolved effective fast for the grown selection, and assert the error mentions the fast-mode setting and --fresh-session. Also assert that no cohort is returned when this error occurs.
architecture:solid (2 findings)
Major - internal/pipeline/pipeline.go:1993
U-L1: the reused-cohort coverage guarantee is checked before the step that can break it. rebaseReviewerCohort runs the uncovered-file check, but joinRequiredReviewers then calls ensureRequiredOnMatchAgents, which overwrites a saved required member's Files/AllowedFiles with only its glob matches. Files rebasing counted as covered by that member can lose their owner with no error. Reproduced with a probe: saved broad member
req(required_on_match, globs ), catalog agentdocs() not in the cohort, changed files main.go and README.md. reuseReviewerCohort returns req scoped to [main.go], grown=nil, err=nil, so README.md has no reviewer. docs/review-lifecycle.md says reuse fails with --fresh-session guidance when a file is coverable by a catalog agent but no saved or joining reviewer can take it, and this path does not. The ensureSelectedGlobCoverage backstop only helps if another selected agent matches the file, and its own comment notes that unassigned files make hasIncompleteReviewerCoverage block approval on every later pass. Reachable when a broad saved member later matches a required glob, or when a saved member becomes required_on_match or has its globs narrowed. Before this PR the broad member kept the file. Suggested fix: run the uncovered check on the final selection, after the join, for example in reuseReviewerCohort, using the existing lockfile exemption and the same freshError. That also removes the temporal coupling where rebaseReviewerCohort now returns a selection that is only valid if the caller joins afterwards. Add a regression test for the broad-member case.
Major - internal/pipeline/pipeline.go:2004
U-L1: the --max-agents guard checks the wrong quantity, so a run can persist a cohort that the next run rejects. The PR description and docs/review-lifecycle.md define the limit as the saved cohort plus joining required reviewers, but the code compares len(selection.SelectedAgents), which leaves out saved members rebasing dropped for an empty assignment. Those members stay in the cohort (grown.Members copies all of cohort.Members). Reproduced with a probe: cohort {lang:go, lang:ts}, only main.go changed, required
privacyjoins, --max-agents 2. The first run passes with 2 selected and persists 3 members. The same command then fails with--max-agents 2 is smaller than the saved reviewer cohort of 3. So the cap is exceeded in durable state, and the failure appears one run later than the contract says. The existing max-agents test misses this because its cohort has no dropped member. Suggested fix: compare len(cohort.Members)+len(joined) with maxAgents, which also makes the error text accurate, and add a test with a dropped member.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; body-inspected: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; skipped: none; constraints: Tests were not executed; review is static, based on the diff and the test function list.documentation:docs— complete (broad); scope: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; body-inspected: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; skipped: none; constraints: Verified docs against the PR description only; did not cross-check implementation code.policies:conventions— complete (broad); scope: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; body-inspected: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; skipped: none; constraints: Reviewed only the docs diffs (README.md, docs/review-guidance.md, docs/review-lifecycle.md); Go implementation and tests were not in assigned scope. Shared cli-common docs were not available in context; no convention drift could be checked against them.structure:repo-health— complete (broad); scope: 2 files; examples:docs/review-lifecycle.md,internal/pipeline/pipeline.go; body-inspected: 2 files; examples:docs/review-lifecycle.md,internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed the diff for the two assigned files only; did not run tests or make check. The new test file internal/pipeline/required_cohort_test.go is outside the assigned scope and was not inspected.go:implementation— complete (broad); scope: 1 file; examples:internal/pipeline/pipeline.go; body-inspected: 1 file; examples:internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed only the pipeline.go diff; did not run tests or linters, and did not inspect ledger.ReplaceReviewerCohort internals or the new test file.architecture:solid— complete (broad); scope: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; body-inspected: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; skipped: none; context (not coverage): 1 file; examples:docs/review-lifecycle.md; constraints: Ran go vet and the cohort-related tests in internal/pipeline (passing) plus throwaway probe tests in a temporary copy; did not run the full make check.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 42s | ~$1.16 (est.) | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.359
| Field | Value |
|---|---|
| Model | claude-sonnet-5-5, claude-opus-5-5 |
| Reviewers | go:implementation-tests, documentation:docs, policies:conventions, structure:repo-health, go:implementation, architecture:solid |
| Engine | claude_cli · claude-sonnet-5-5, claude-opus-5-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 42s wall · 5m 39s compute |
| Cost | ~$1.16 (est.) |
| Pricing basis | baseline-2026-10-02/pricing |
| Tokens | 74 in / 20.7k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5-5- In: 4
- Out: 1.2k
- Cache read: 9.2k
- Cache create: 11.8k
- Cost: ~$0.06 (est.)
- Duration: 20s
go:implementation-tests— claude-sonnet-5-5- In: 12
- Out: 1.9k
- Cache read: 81.1k
- Cache create: 15.0k
- Cost: ~$0.09 (est.)
- Duration: 38s
documentation:docs— claude-sonnet-5-5- In: 10
- Out: 1.3k
- Cache read: 58.9k
- Cache create: 12.3k
- Cost: ~$0.07 (est.)
- Duration: 24s
policies:conventions— claude-sonnet-5-5- In: 8
- Out: 1.0k
- Cache read: 40.2k
- Cache create: 11.0k
- Cost: ~$0.06 (est.)
- Duration: 26s
structure:repo-health— claude-sonnet-5-5- In: 10
- Out: 1.5k
- Cache read: 63.7k
- Cache create: 15.9k
- Cost: ~$0.09 (est.)
- Duration: 25s
go:implementation— claude-sonnet-5-5- In: 10
- Out: 1.7k
- Cache read: 65.0k
- Cache create: 16.3k
- Cost: ~$0.10 (est.)
- Duration: 35s
architecture:solid— claude-opus-5-5- In: 14
- Out: 11.5k
- Cache read: 177.9k
- Cache create: 46.9k
- Cost: ~$0.64 (est.)
- Duration: 2m 32s
orchestrator-rollup— claude-sonnet-5-5- In: 6
- Out: 692
- Cache read: 68.2k
- Cache create: 4.8k
- Cost: ~$0.04 (est.)
- Duration: 15s
| } | ||
| 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) |
There was a problem hiding this comment.
The new fast-mode conflict branch in joinRequiredReviewers (saved member Fast != effectiveFast once required reviewers join) has no test. It is a distinct failure path from the --max-agents rejection and is the only guard against silently running a saved member with a different fast setting than it was persisted with. A regression, such as dropping the check or comparing the wrong value, would not be caught. Fix: add a focused table case in required_cohort_test.go with a saved member whose Fast differs from the resolved effective fast for the grown selection, and assert the error mentions the fast-mode setting and --fresh-session. Also assert that no cohort is returned when this error occurs.
Reply inline to this comment.
| if len(joined) == 0 { | ||
| return selection, nil, nil | ||
| } | ||
| if maxAgents > 0 && len(selection.SelectedAgents) > maxAgents { |
There was a problem hiding this comment.
U-L1: the --max-agents guard checks the wrong quantity, so a run can persist a cohort that the next run rejects. The PR description and docs/review-lifecycle.md define the limit as the saved cohort plus joining required reviewers, but the code compares len(selection.SelectedAgents), which leaves out saved members rebasing dropped for an empty assignment. Those members stay in the cohort (grown.Members copies all of cohort.Members). Reproduced with a probe: cohort {lang:go, lang:ts}, only main.go changed, required privacy joins, --max-agents 2. The first run passes with 2 selected and persists 3 members. The same command then fails with --max-agents 2 is smaller than the saved reviewer cohort of 3. So the cap is exceeded in durable state, and the failure appears one run later than the contract says. The existing max-agents test misses this because its cohort has no dropped member. Suggested fix: compare len(cohort.Members)+len(joined) with maxAgents, which also makes the error text accurate, and add a test with a dropped member.
Reply inline to this comment.
| for _, member := range cohort.Members { | ||
| members[member.AgentID] = member | ||
| } | ||
| selection = ensureRequiredOnMatchAgents(selection, catalog, changedFiles) |
There was a problem hiding this comment.
U-L1: the reused-cohort coverage guarantee is checked before the step that can break it. rebaseReviewerCohort runs the uncovered-file check, but joinRequiredReviewers then calls ensureRequiredOnMatchAgents, which overwrites a saved required member's Files/AllowedFiles with only its glob matches. Files rebasing counted as covered by that member can lose their owner with no error. Reproduced with a probe: saved broad member req (required_on_match, globs /*.go), catalog agent docs (/*.md) not in the cohort, changed files main.go and README.md. reuseReviewerCohort returns req scoped to [main.go], grown=nil, err=nil, so README.md has no reviewer. docs/review-lifecycle.md says reuse fails with --fresh-session guidance when a file is coverable by a catalog agent but no saved or joining reviewer can take it, and this path does not. The ensureSelectedGlobCoverage backstop only helps if another selected agent matches the file, and its own comment notes that unassigned files make hasIncompleteReviewerCoverage block approval on every later pass. Reachable when a broad saved member later matches a required glob, or when a saved member becomes required_on_match or has its globs narrowed. Before this PR the broad member kept the file. Suggested fix: run the uncovered check on the final selection, after the join, for example in reuseReviewerCohort, using the existing lockfile exemption and the same freshError. That also removes the temporal coupling where rebaseReviewerCohort now returns a selection that is only valid if the caller joins afterwards. Add a regression test for the broad-member case.
Reply inline to this comment.
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
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 7958e27adfbd
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| documentation:docs | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| go:implementation | 0 |
| architecture:solid | 1 |
go:implementation-tests (1 finding)
Minor - internal/pipeline/pipeline.go:1969
reusedCohortCoverageGap now exempts generated lockfiles (
isGeneratedLockfile) from the reused-cohort coverage obligation. This is a new behavior, and the PR description calls it out. None of the listed tests exercises it throughreuseReviewerCohort. The Cargo.lock fixture in TestJoinRequiredReviewersAddsMissingRequiredReviewerAndGrowsCohort callsjoinRequiredReviewersdirectly and never reaches the gap check. If the exemption were dropped, a lockfile-only change that no saved member covers would fail the rerun with a spurious --fresh-session error, and no test would catch it. Fix: add a small case to the TestReuseReviewerCohortRejects… table, or a separate test. It should runreuseReviewerCohortwith a catalog agent that has no file globs or whose globs matchCargo.lock, and a changed set ofmain.goplusCargo.lock. Assert that no error is returned.
architecture:solid (1 finding)
Minor - internal/pipeline/pipeline.go:2064
U-L1: the fast-mode guard checks only the members running this pass, but the grown cohort saves every member. This is the same gap the --max-agents guard just closed by counting the cohort that will be saved. The PR says a fast-mode conflict between saved members and the grown set fails with --fresh-session guidance, but if no saved member is selected this pass, the loop has nothing to compare. Reproduced with a probe: cohort {lang:go Fast=false}, run with ReviewerFast=true, only data.json changed, required privacy:data joins. reuseReviewerCohort returns no error and grown holds lang:go fast=false and privacy:data fast=true. On the next run with main.go changed, --fast fails with
saved reviewer "lang:go" fast-mode setting is incompatible. Without --fast the same cohort returns no error, even though privacy:data was saved with Fast=true. rebaseReviewerCohort drops it, widenRequiredOnMatchAgents brings it back, and the early return at len(joined)==0 skips this check. Impact is narrow because it needs a changed fast setting on a pass where only a joiner's files changed, but the mixed setting is stored durably. Suggested fix: iterate cohort.Members rather than selection.SelectedAgents here, matching the cap's comment, and add a table case to TestJoinRequiredReviewersFastMode where the saved member is not selected.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 3 files; examples:internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/required_cohort_test.go; body-inspected: 3 files; examples:internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/required_cohort_test.go; skipped: none; constraints: Prior inline threads (max-agents count, coverage check ordering, fast-mode test) appear addressed by this revision and were not re-raised. Static review of the diff and test function list only; tests were not executed.documentation:docs— complete (constrained); scope: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; body-inspected: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; skipped: none; constraints: Verified docs against the PR description only; did not cross-check implementation code.policies:conventions— complete (constrained); scope: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; body-inspected: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; skipped: none; constraints: Reviewed only the docs diffs (README.md, docs/review-guidance.md, docs/review-lifecycle.md); Go implementation and tests were not in assigned scope. Shared cli-common docs were not available in context; no convention drift could be checked against them.structure:repo-health— complete (constrained); scope: 2 files; examples:docs/review-lifecycle.md,internal/pipeline/pipeline.go; body-inspected: 2 files; examples:docs/review-lifecycle.md,internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed the diff for the two assigned files only; did not run tests or make check. The new test file internal/pipeline/required_cohort_test.go is outside the assigned scope and was not inspected.go:implementation— complete (constrained); scope: 1 file; examples:internal/pipeline/pipeline.go; body-inspected: 1 file; examples:internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed only the pipeline.go diff; did not run tests or linters, and did not inspect ledger.ReplaceReviewerCohort internals or the new test file.architecture:solid— complete (constrained); scope: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; body-inspected: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; skipped: none; constraints: Ran go test and go vet on internal/pipeline and probe tests in temporary copies; did not run make check. TestExecuteHoldsActiveRunsLockForRunDuration failed only in a non-git copy and passed in a git clone, so the diff did not cause it.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 8m 43s | ~$0.89 (est.) | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.359
| Field | Value |
|---|---|
| Model | claude-sonnet-5-5, claude-opus-5-5 |
| Reviewers | go:implementation-tests, documentation:docs, policies:conventions, structure:repo-health, go:implementation, architecture:solid |
| Engine | claude_cli · claude-sonnet-5-5, claude-opus-5-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 8m 43s wall · 10m 17s compute |
| Cost | ~$0.89 (est.) |
| Pricing basis | baseline-2026-10-02/pricing |
| Tokens | 62 in / 13.2k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5-5- In: 12
- Out: 2.2k
- Cache read: 176.1k
- Cache create: 17.9k
- Cost: ~$0.13 (est.)
- Duration: 38s
documentation:docs— claude-sonnet-5-5- In: 8
- Out: 915
- Cache read: 89.0k
- Cache create: 11.5k
- Cost: ~$0.07 (est.)
- Duration: 16s
policies:conventions— claude-sonnet-5-5- In: 8
- Out: 938
- Cache read: 81.2k
- Cache create: 10.3k
- Cost: ~$0.07 (est.)
- Duration: 25s
structure:repo-health— claude-sonnet-5-5- In: 8
- Out: 1.1k
- Cache read: 106.0k
- Cache create: 15.0k
- Cost: ~$0.09 (est.)
- Duration: 26s
go:implementation— claude-sonnet-5-5- In: 8
- Out: 1.1k
- Cache read: 108.4k
- Cache create: 15.0k
- Cost: ~$0.09 (est.)
- Duration: 27s
architecture:solid— claude-opus-5-5- In: 12
- Out: 6.3k
- Cache read: 390.6k
- Cache create: 24.0k
- Cost: ~$0.40 (est.)
- Duration: 7m 51s
orchestrator-rollup— claude-sonnet-5-5- In: 6
- Out: 576
- Cache read: 81.6k
- Cache create: 3.8k
- Cost: ~$0.04 (est.)
- Duration: 10s
| return llm.Selection{}, nil, err | ||
| } | ||
| for _, selected := range selection.SelectedAgents { | ||
| if member, ok := members[selected.AgentID]; ok && member.Fast != effectiveFast { |
There was a problem hiding this comment.
U-L1: the fast-mode guard checks only the members running this pass, but the grown cohort saves every member. This is the same gap the --max-agents guard just closed by counting the cohort that will be saved. The PR says a fast-mode conflict between saved members and the grown set fails with --fresh-session guidance, but if no saved member is selected this pass, the loop has nothing to compare. Reproduced with a probe: cohort {lang:go Fast=false}, run with ReviewerFast=true, only data.json changed, required privacy:data joins. reuseReviewerCohort returns no error and grown holds lang:go fast=false and privacy:data fast=true. On the next run with main.go changed, --fast fails with saved reviewer "lang:go" fast-mode setting is incompatible. Without --fast the same cohort returns no error, even though privacy:data was saved with Fast=true. rebaseReviewerCohort drops it, widenRequiredOnMatchAgents brings it back, and the early return at len(joined)==0 skips this check. Impact is narrow because it needs a changed fast setting on a pass where only a joiner's files changed, but the mixed setting is stored durably. Suggested fix: iterate cohort.Members rather than selection.SelectedAgents here, matching the cap's comment, and add a table case to TestJoinRequiredReviewersFastMode where the saved member is not selected.
Reply inline to this comment.
| } | ||
| } | ||
| for _, file := range changedFiles { | ||
| if covered[file] || isGeneratedLockfile(file) { |
There was a problem hiding this comment.
reusedCohortCoverageGap now exempts generated lockfiles (isGeneratedLockfile) from the reused-cohort coverage obligation. This is a new behavior, and the PR description calls it out. None of the listed tests exercises it through reuseReviewerCohort. The Cargo.lock fixture in TestJoinRequiredReviewersAddsMissingRequiredReviewerAndGrowsCohort calls joinRequiredReviewers directly and never reaches the gap check. If the exemption were dropped, a lockfile-only change that no saved member covers would fail the rerun with a spurious --fresh-session error, and no test would catch it. Fix: add a small case to the TestReuseReviewerCohortRejects… table, or a separate test. It should run reuseReviewerCohort with a catalog agent that has no file globs or whose globs match Cargo.lock, and a changed set of main.go plus Cargo.lock. Assert that no error is returned.
Reply inline to this comment.
…he lockfile exemption Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 5fe715d284a3
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| documentation:docs | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| go:implementation | 0 |
| architecture:solid | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 3 files; examples:internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/required_cohort_test.go; body-inspected: 3 files; examples:internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go,internal/pipeline/required_cohort_test.go; skipped: none; constraints: Static review of the diff and test function list only; tests were not executed. The earlier inline threads (max-agents count, coverage-check ordering, fast-mode test, whole-cohort fast comparison, lockfile exemption test) look addressed in this revision and were not re-raised.documentation:docs— complete (constrained); scope: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; body-inspected: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; skipped: none; constraints: Verified docs against the PR description only; did not cross-check implementation code.policies:conventions— complete (constrained); scope: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; body-inspected: 3 files; examples:README.md,docs/review-guidance.md,docs/review-lifecycle.md; skipped: none; constraints: Reviewed only the docs diffs (README.md, docs/review-guidance.md, docs/review-lifecycle.md); Go implementation and tests were not in assigned scope. Shared cli-common docs were not available in context; no convention drift could be checked against them.structure:repo-health— complete (constrained); scope: 2 files; examples:docs/review-lifecycle.md,internal/pipeline/pipeline.go; body-inspected: 2 files; examples:docs/review-lifecycle.md,internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed the diff for the two assigned files only; did not run tests or make check. Test files and README.md/docs/review-guidance.md changes are outside the assigned scope and were not inspected.go:implementation— complete (constrained); scope: 1 file; examples:internal/pipeline/pipeline.go; body-inspected: 1 file; examples:internal/pipeline/pipeline.go; skipped: none; constraints: Reviewed only the pipeline.go diff (base and the prior reviewed head); did not run tests or linters, and did not inspect ledger.ReplaceReviewerCohort internals or the test file.architecture:solid— complete (constrained); scope: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; body-inspected: 2 files; examples:internal/pipeline/pipeline.go,internal/pipeline/required_cohort_test.go; skipped: none; constraints: Ran go vet and go test on internal/pipeline in a temporary clone at 5fe715d, both passing; did not run make check or tests for other packages.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 9m 50s | ~$0.63 (est.) | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.359
| Field | Value |
|---|---|
| Model | claude-sonnet-5-5, claude-opus-5-5 |
| Reviewers | go:implementation-tests, documentation:docs, policies:conventions, structure:repo-health, go:implementation, architecture:solid |
| Engine | claude_cli · claude-sonnet-5-5, claude-opus-5-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 9m 50s wall · 11m 03s compute |
| Cost | ~$0.63 (est.) |
| Pricing basis | baseline-2026-10-02/pricing |
| Tokens | 54 in / 7.1k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5-5- In: 8
- Out: 1.1k
- Cache read: 172.4k
- Cache create: 12.3k
- Cost: ~$0.09 (est.)
- Duration: 22s
documentation:docs— claude-sonnet-5-5- In: 8
- Out: 870
- Cache read: 135.9k
- Cache create: 12.0k
- Cost: ~$0.08 (est.)
- Duration: 20s
policies:conventions— claude-sonnet-5-5- In: 8
- Out: 954
- Cache read: 122.4k
- Cache create: 9.6k
- Cost: ~$0.07 (est.)
- Duration: 21s
structure:repo-health— claude-sonnet-5-5- In: 8
- Out: 1.1k
- Cache read: 161.8k
- Cache create: 10.2k
- Cost: ~$0.08 (est.)
- Duration: 22s
go:implementation— claude-sonnet-5-5- In: 8
- Out: 1.0k
- Cache read: 165.4k
- Cache create: 11.4k
- Cost: ~$0.09 (est.)
- Duration: 23s
architecture:solid— claude-opus-5-5- In: 8
- Out: 1.6k
- Cache read: 326.0k
- Cache create: 9.1k
- Cost: ~$0.17 (est.)
- Duration: 8m 55s
orchestrator-rollup— claude-sonnet-5-5- In: 6
- Out: 437
- Cache read: 91.9k
- Cache create: 2.6k
- Cost: ~$0.03 (est.)
- Duration: 18s
Closes #701
Problem
required_on_match: truepromises a reviewer runs whenever itsfile_globsmatch a changed file. Fresh selection keeps that promise throughensureRequiredOnMatchAgents. A follow-up review that reuses the PR's saved reviewer cohort (a plain rerun or--rerun, without--fresh-session) only rebases the saved members, so:--fresh-session.saved reviewer cohort leaves changed file … uncovered; pass --fresh-session.Fix
Cohort reuse now applies the same required-reviewer rule as fresh selection, then persists the result.
joinRequiredReviewersadds every matchingrequired_on_matchreviewer to the rebased selection (widenRequiredOnMatchAgents), then runsensureSelectedGlobCoverageas fresh selection does. A missing required reviewer is appended, scoped to its matches. A required member already in the cohort keeps the files rebasing gave it and gains its matches. It is widened, never narrowed, so a file a broad saved member held cannot lose its only owner.rebaseReviewerCohorttoreusedCohortCoverageGap. It now runs inreuseReviewerCohorton the final selection, after the join, and fails with the same--fresh-sessionguidance. A file a joining required reviewer takes no longer fails the rerun. Generated lockfiles are exempt from the check, as they are from reviewer coverage.ReplaceReviewerCohortbefore any reviewer starts, so the newcomer's provider session is recorded and resumed on later passes like any original member's.cohortResumesrestores the saved session for a member that rebasing dropped for lack of scope and a required reviewer then brought back.--max-agentssmaller than the saved cohort plus the joining required reviewers fails with--fresh-sessionguidance, matching the existing fresh-selection rule that a cap never drops required reviewers. The count is the cohort that will be saved, including members this pass does not run because nothing in their scope changed. A fast-mode conflict fails the same way. Every saved member is compared with the grown set's fast setting, including members this pass does not run, because the grown cohort is saved whole.Docs:
docs/review-lifecycle.mddescribes the one way a reused cohort grows and updates the reuse-failure conditions.README.mdanddocs/review-guidance.mdstate that therequired_on_matchguarantee holds on reuse.Tests
internal/pipeline/required_cohort_test.go:TestPlainRerunRunsRequiredReviewerAddedAfterFirstReview: end to end throughDryRun. The first review runs with one agent. Arequired_on_matchagent is then added to the catalog. The plain rerun makes no selection call, runs both reviewers, and saves the newcomer in the cohort with its recorded session. Without the fix the rerun runs only the original reviewer.TestJoinRequiredReviewersAddsMissingRequiredReviewerAndGrowsCohort: the joined reviewer is scoped to every glob match (a lockfile its globs exclude stays out). The grown cohort keeps the saved member unchanged and the saved scope and creation time. This also covers case 3: before the fix,rebaseReviewerCohortfailed on the fixture file.TestJoinRequiredReviewersWidensSavedRequiredMemberToNewMatches: a new file rebasing handed to another member also reaches the required member. No cohort write when nobody joins.TestReuseReviewerCohortKeepsFilesABroadRequiredMemberHeld: a broad saved member that isrequired_on_matchon**/*.gokeepsREADME.mdwhen a non-member catalog agent matches it. Narrowing it to its glob matches would have left the file with no reviewer.TestJoinRequiredReviewersCapCountsMembersThisPassDoesNotRun: a cohort of two, one dropped this pass, plus one joiner under--max-agents 2fails instead of saving three members.TestJoinRequiredReviewersFastMode: a joiner without fast support next to a fast saved member fails with--fresh-sessionguidance and persists nothing. A fast-capable joiner is saved with fast mode on. A fast joiner next to a normal-speed member that does not run this pass also fails.TestReuseReviewerCohortExemptsUncoveredLockfile: an uncoveredCargo.lockdoes not force a reselected cohort when a broad catalog agent exists.TestJoinRequiredReviewersLeavesSatisfiedCohortUnchanged,TestJoinRequiredReviewersRejectsMaxAgentsBelowGrownCohort,TestCohortResumesRestoresSessionForReturningMember.reuseReviewerCohort, where the check lives; they are renamedTestReuseReviewerCohortRejects….make checkpasses (tidy, fmt, lint with 0 issues, tests, build).