From dc601250fe6b273430efd0d16b7d75a9cf5e3f7c Mon Sep 17 00:00:00 2001 From: Andrew Anderson Date: Thu, 17 Sep 2026 22:50:59 -0400 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20feat(review):=20spend=20review=20sl?= =?UTF-8?q?ots=20breadth-first=20on=20deep=20queues?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PlanDispatch spends a fixed parallel-review budget in PR order, and an uncapped PR takes as many slots as it has missing perspectives. On a short queue that is exactly right - fanning every perspective out at once is the review swarm's designed behavior. On a deep one it inverts the intent: the head of the queue absorbs the entire budget, the PRs behind it get nothing this cycle, and adding reviewers buys more opinions on one PR rather than coverage across many. A spoke with hundreds of open PRs is the case where that matters, because there the scarce thing is PRs looked at, not depth per PR. Add review.max_perspectives_per_pr, which caps perspectives dispatched to one PR per cycle. It loses no coverage: a perspective skipped this cycle is still missing next cycle and gets dispatched then, so the cap schedules depth rather than dropping it. It also bounds how many comments a single PR can collect at once, which starts to matter now that reviewers publish their verdicts. Zero means no cap, so every existing hive keeps the behavior it has. The cap is subordinate to MaxParallelReviews, which still bounds total concurrency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Andrew Anderson --- changelog.d/added-review-breadth-first.md | 1 + src/cmd/hive/main.go | 19 +++--- src/pkg/config/config.go | 13 ++++ src/pkg/review/dispatch.go | 30 +++++++-- src/pkg/review/dispatch_test.go | 81 +++++++++++++++++++++++ 5 files changed, 131 insertions(+), 13 deletions(-) create mode 100644 changelog.d/added-review-breadth-first.md diff --git a/changelog.d/added-review-breadth-first.md b/changelog.d/added-review-breadth-first.md new file mode 100644 index 0000000000..c50bfca39b --- /dev/null +++ b/changelog.d/added-review-breadth-first.md @@ -0,0 +1 @@ +- Added `review.max_perspectives_per_pr`, an opt-in cap on how many review perspectives one PR may be given per dispatch cycle. Without it the parallel review budget is spent in PR order, so the head of a deep queue absorbs every slot and adding reviewers buys more opinions on one PR instead of coverage across many. Capping spends the same budget breadth-first, and loses no coverage because skipped perspectives are still dispatched on later cycles. Zero (the default) keeps the existing fan-out behavior. diff --git a/src/cmd/hive/main.go b/src/cmd/hive/main.go index ef95c90503..bf3dfb75f8 100644 --- a/src/cmd/hive/main.go +++ b/src/cmd/hive/main.go @@ -9487,15 +9487,16 @@ func planReviewDispatch(cfg *config.Config, actionable *github.ActionableResult, }) } plan := review.PlanDispatch(prs, artifact, state, review.DispatchOptions{ - RequireApproval: cfg.Review.RequireApproval, - FanOut: cfg.Review.FanOut, - MaxParallelReviews: cfg.Review.EffectiveMaxParallelReviews(), - ReviewerAgents: cfg.Review.ReviewerAgents, - FixerAgent: cfg.Review.FixerAgent, - PostComments: cfg.Review.PostComments, - ProjectOrg: cfg.Project.Org, - AIAuthor: cfg.EffectiveAIAuthor(), - Agents: agents, + RequireApproval: cfg.Review.RequireApproval, + FanOut: cfg.Review.FanOut, + MaxParallelReviews: cfg.Review.EffectiveMaxParallelReviews(), + MaxPerspectivesPerPR: cfg.Review.MaxPerspectivesPerPR, + ReviewerAgents: cfg.Review.ReviewerAgents, + FixerAgent: cfg.Review.FixerAgent, + PostComments: cfg.Review.PostComments, + ProjectOrg: cfg.Project.Org, + AIAuthor: cfg.EffectiveAIAuthor(), + Agents: agents, }) if len(plan.ReviewKicks)+len(plan.FixKicks) > 0 { logger.Info("review swarm dispatch planned", "review_kicks", len(plan.ReviewKicks), "fix_kicks", len(plan.FixKicks)) diff --git a/src/pkg/config/config.go b/src/pkg/config/config.go index d5c6d72109..871d93d70e 100644 --- a/src/pkg/config/config.go +++ b/src/pkg/config/config.go @@ -6294,6 +6294,19 @@ type ReviewConfig struct { // aggregate has no consumer and the reviewer is silent by construction. // Turning this on is what makes a review reach the human who has to decide. PostComments bool `yaml:"post_comments,omitempty" json:"post_comments,omitempty"` + // MaxPerspectivesPerPR caps how many review perspectives one PR may be + // given in a single dispatch cycle. It exists because parallel review + // slots are a fixed budget spent in PR order: without a cap, the first PR + // in a deep queue absorbs every slot for its own perspectives, so adding + // reviewers buys more opinions on one PR instead of coverage across many. + // Capping it spends the same budget breadth-first. No coverage is lost — + // the perspectives skipped this cycle are still "missing" next cycle and + // get dispatched then — so this schedules depth rather than dropping it. + // It also bounds how many comments a single PR can collect at once, which + // matters once reviewers publish. Zero means no cap — fanning every + // perspective out at once stays the default, so this only changes a hive + // that opts in because its queue is too deep to review in depth. + MaxPerspectivesPerPR int `yaml:"max_perspectives_per_pr,omitempty" json:"max_perspectives_per_pr,omitempty"` } // DuplicateSweepConfig gates the cross-PR duplicate sweep diff --git a/src/pkg/review/dispatch.go b/src/pkg/review/dispatch.go index d6dd0f5f33..98ea572455 100644 --- a/src/pkg/review/dispatch.go +++ b/src/pkg/review/dispatch.go @@ -38,10 +38,13 @@ type DispatchOptions struct { RequireApproval bool FanOut bool MaxParallelReviews int - ReviewerAgents []string - FixerAgent string - ProjectOrg string - AIAuthor string + // MaxPerspectivesPerPR caps perspectives dispatched to one PR per cycle. + // Zero means DefaultMaxPerspectivesPerPR. + MaxPerspectivesPerPR int + ReviewerAgents []string + FixerAgent string + ProjectOrg string + AIAuthor string // PostComments carries config.ReviewConfig.PostComments into the prompt // builder, so reviewers are told to publish their verdict on the PR. PostComments bool @@ -169,6 +172,12 @@ func PlanDispatch(prs []PullRequest, artifact Artifact, state DispatchState, opt continue } limit := len(missing) + // Breadth before depth: the parallel budget is spent in PR order, so + // an uncapped first PR would take every slot for its own perspectives + // and leave the rest of the queue unreviewed this cycle. + if perPR := opts.effectiveMaxPerspectivesPerPR(); perPR > 0 && limit > perPR { + limit = perPR + } if len(reviewers) == 1 && limit > 1 { limit = 1 } @@ -293,6 +302,19 @@ func (a Artifact) AggregateFor(repo string, number int, headSHA string) (Aggrega return Aggregate{}, false } +// effectiveMaxPerspectivesPerPR returns the per-PR perspective cap, or 0 for +// "no cap". Unlimited is the default deliberately: fanning every perspective +// out at once is the review swarm's designed behavior, and a hive that wants +// depth on each PR should keep getting it. The cap is for the opposite +// situation — a queue too deep to review in depth — and is opt-in so no +// existing hive silently changes shape. +func (o DispatchOptions) effectiveMaxPerspectivesPerPR() int { + if o.MaxPerspectivesPerPR <= 0 { + return 0 + } + return o.MaxPerspectivesPerPR +} + func reviewCapableAgents(opts DispatchOptions) []AgentCapability { allowed := map[string]bool{} for _, name := range opts.ReviewerAgents { diff --git a/src/pkg/review/dispatch_test.go b/src/pkg/review/dispatch_test.go index 2448dbaede..b42d079c09 100644 --- a/src/pkg/review/dispatch_test.go +++ b/src/pkg/review/dispatch_test.go @@ -133,3 +133,84 @@ func TestConfigReviewDefaults(t *testing.T) { t.Fatalf("configured max parallel = %d, want 2", got) } } + +func dispatchPRNum(n int, sha string) PullRequest { + pr := dispatchPR(sha) + pr.Number = n + return pr +} + +// TestDispatchSpendsSlotsDepthFirstByDefault documents the status quo the cap +// exists to change: the parallel budget is spent in PR order, so the head of +// the queue absorbs every slot for its own perspectives and the PRs behind it +// get nothing this cycle. That is the right behavior when the queue is short. +func TestDispatchSpendsSlotsDepthFirstByDefault(t *testing.T) { + prs := []PullRequest{dispatchPRNum(1, "sha1"), dispatchPRNum(2, "sha2"), dispatchPRNum(3, "sha3")} + plan := PlanDispatch(prs, Artifact{}, DispatchState{}, DispatchOptions{ + RequireApproval: true, + FanOut: true, + MaxParallelReviews: 3, + ProjectOrg: "acme", + Agents: []AgentCapability{reviewer("r1"), reviewer("r2"), reviewer("r3")}, + }) + + if len(plan.ReviewKicks) != 3 { + t.Fatalf("got %d kicks, want 3 (the full slot budget)", len(plan.ReviewKicks)) + } + for _, k := range plan.ReviewKicks { + if k.Number != 1 { + t.Fatalf("uncapped dispatch should concentrate on the first PR, got a kick for #%d", k.Number) + } + } +} + +// TestMaxPerspectivesPerPRSpreadsAcrossPRs is the point of the cap: the same +// budget, spent breadth-first, reviews every PR in the queue once instead of +// one PR three times. No coverage is lost — the perspectives skipped here are +// still missing next cycle and get dispatched then. +func TestMaxPerspectivesPerPRSpreadsAcrossPRs(t *testing.T) { + prs := []PullRequest{dispatchPRNum(1, "sha1"), dispatchPRNum(2, "sha2"), dispatchPRNum(3, "sha3")} + plan := PlanDispatch(prs, Artifact{}, DispatchState{}, DispatchOptions{ + RequireApproval: true, + FanOut: true, + MaxParallelReviews: 3, + MaxPerspectivesPerPR: 1, + ProjectOrg: "acme", + Agents: []AgentCapability{reviewer("r1"), reviewer("r2"), reviewer("r3")}, + }) + + if len(plan.ReviewKicks) != 3 { + t.Fatalf("got %d kicks, want 3", len(plan.ReviewKicks)) + } + seen := map[int]int{} + for _, k := range plan.ReviewKicks { + seen[k.Number]++ + } + if len(seen) != 3 { + t.Fatalf("capped dispatch covered %d distinct PRs, want 3: %+v", len(seen), seen) + } + for num, n := range seen { + if n != 1 { + t.Errorf("PR #%d got %d perspectives, want 1 under the cap", num, n) + } + } +} + +// TestMaxPerspectivesPerPRNeverExceedsSlotBudget keeps the cap subordinate to +// the parallel budget: raising it must not let dispatch run more reviews at +// once than the hive allows. +func TestMaxPerspectivesPerPRNeverExceedsSlotBudget(t *testing.T) { + prs := []PullRequest{dispatchPRNum(1, "sha1"), dispatchPRNum(2, "sha2")} + plan := PlanDispatch(prs, Artifact{}, DispatchState{}, DispatchOptions{ + RequireApproval: true, + FanOut: true, + MaxParallelReviews: 2, + MaxPerspectivesPerPR: 4, + ProjectOrg: "acme", + Agents: []AgentCapability{reviewer("r1"), reviewer("r2")}, + }) + + if len(plan.ReviewKicks) != 2 { + t.Fatalf("got %d kicks, want 2 (the slot budget, not the per-PR cap)", len(plan.ReviewKicks)) + } +}