✨ feat(review): spend review slots breadth-first on deep queues - #7528
Merged
Merged
Conversation
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 <andy@clubanderson.com>
kubestellar-prow
Bot
added
dco-signoff: yes
Member
Author
|
/lgtm |
Member
Author
|
/approve |
Contributor
|
@clubanderson: you cannot LGTM your own PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clubanderson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
approved
Contributor
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
This was referenced Sep 18, 2026
clubanderson
added a commit
that referenced
this pull request
Sep 18, 2026
Carries v4 e74d45e..201c69c (v4.54.2, v4.55.0, v4.55.1) into v5, including #7510 (cmd/hive domain extraction), #7532 (manager.go split), #7527/#7528 (review routing + breadth-first slots), #7523 (token-access audit backport of v5 #6303), #7515 (branch-protection facts on blocked PR pills), #7535, #7539, #7520. Resolution notes: - cmd/hive, pkg/agent: declaration-level 3-way merge against base ff7630e; v5 seams (spokeStatePath, SetHubPushedDashboardURL, applyModeUnscheduledAlert, mutation boundary, runDuplicateSweepIfDue) preserved. F24 tombstone intact. - pkg/github/client.go: v5 fetchPRs signature (RepoPRBreakdown) + v4 BaseRef/prBaseRef; added Client.requiredStatusCheckContexts shim so v4's protection_facts.go compiles against v5, where the sweep lives in pkg/github/automerge. - pkg/github/token_access_audit{,_test}.go: v4 side (its backport of v5's own #6303, now using testutil.Eventually). - pkg/github/automerge/f3_trusted_merger_source_test.go: v4's f3ReadPackage path adjusted one level deeper for v5's subpackage. - pkg/review/prompts.go: v4 routing instruction + mentionableAuthor added; v5's #6212 deletion of dead BuildPerspectivePrompts / BuildSequentialPrompt kept. - pkg/agent tests: v4's normalizeModelName cases rewritten against v5's normalizeModelNameForBackend (v5 removed the wrapper in #5795). - docs/knowledge-curator.md: v5 promotion-only body + v4's knowledge.git_sources section (code exists on v5). roadmap.md: v4 rows. api-reference.md: v5 table, citations re-resolved (--fix). - .github/workflows/v2-ci.yml: v5 UID-isolation step + v4's fuller audit-log step comment. Refs #7463 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Andy Anderson <andy@clubanderson.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PlanDispatchspends 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, and
TestPlanDispatchStateMachineencodes it.On a deep queue it inverts the intent. The head of the queue absorbs the entire budget, every PR behind it gets nothing that cycle, and adding reviewers buys more opinions on one PR instead of coverage across many. On a spoke with hundreds of open PRs the scarce resource is PRs looked at, not depth per PR.
It also bounds blast radius now that reviewers publish: uncapped, a single PR can collect five comments at once.
Change
Adds
review.max_perspectives_per_pr- an opt-in cap on perspectives dispatched to one PR per cycle.No coverage is lost. A perspective skipped this cycle is still missing next cycle (
pendingMissingPerspectives) and gets dispatched then. The cap schedules depth rather than dropping it.Zero means no cap, so every existing hive keeps exactly the behavior it has - the three tests that encode the current fan-out semantics pass unchanged. The cap stays subordinate to
MaxParallelReviews, which still bounds total concurrency.Tests
Three new tests, all passing:
TestDispatchSpendsSlotsDepthFirstByDefault- pins the status quo: uncapped, all three slots land on PR 🌱 Sync workflows from kubestellar/infra #1.TestMaxPerspectivesPerPRSpreadsAcrossPRs- the same budget with the cap at 1 covers three distinct PRs once each.TestMaxPerspectivesPerPRNeverExceedsSlotBudget- a cap above the slot budget cannot raise concurrency.go build ./...,go vet ./pkg/review/ ./pkg/config/, and the fullpkg/reviewsuite are clean.Companion
Pairs with #7527 (routing blocking verdicts to a human). Together: review more of the queue, and make the findings that need a person actually reach one.