ci(assign-reviewers): self-enroll this repo + seed the expertise map from approval history#78
Open
mattmillerai wants to merge 2 commits into
Open
ci(assign-reviewers): self-enroll this repo + seed the expertise map from approval history#78mattmillerai wants to merge 2 commits into
mattmillerai wants to merge 2 commits into
Conversation
…from approval history 17 open PRs here had gone unassigned, so nothing routed them to anyone who could approve. This repo has shipped an expertise-aware, load-balanced router since BE-1943 and never enrolled itself in it. Despite its name assign-reviewers.yml writes the ASSIGNEE field, not reviewer requests — "this org routes/alerts people via the ASSIGNEE field" (its header) — which is exactly the routing that was missing. WHY THE MAP IS SEEDED FROM APPROVALS, NOT GIT HISTORY: nearly every commit on main here has a single author, so authorship carries no routing signal at all. The buckets are instead populated from who has actually APPROVED merged PRs in the matching area: wei-hai 9, huntcsg 3, deepme987 3, sundar-svg 2, synap5e 1, luke-mino-altherr 1, bigcat88 1. This also has to exist before refresh-reviewers (#63, BE-4116) is useful: that generator treats the committed `rules:` globs as its BUCKET DEFINITIONS and only re-scores membership, so it cannot bootstrap a map from nothing. Two warnings are recorded in the file: it scores commit authorship (so on this repo its first drift PR will want to collapse buckets toward the single dominant committer — read it, don't rubber-stamp it), and the globs are therefore the durable part. `default_pool` is deliberately three proven approvers and deliberately excludes the person who opens most PRs here: selection drops the PR author, so a bucket whose only member is the author empties, and default_pool is the real safety net. Verification — the reusable parses reviewers.yml with a hand-rolled minimal YAML parser (js-yaml isn't available in github-script), so standard YAML validity is not sufficient. Extracted that parser AND its globToRegExp/matchesAny verbatim and drove the committed map through them: - 7 rules, every one with non-empty paths + reviewers; default_pool parsed. - 17 representative repo paths all route to the intended bucket; an unmatched path correctly falls through to default_pool. - Retro-validated against the 17 live assignments: 15/17 of the assignees fall inside what this map would pick, 0 fell through to default_pool. The two deviations are deliberate human overrides, not map errors — #71 went to guill as a privilege-boundary review rather than a groom-domain one, and #61 went to sundar-svg purely to spread load on a one-line binary deletion. - That retro pass is what caught `ci-assign-reviewers.yml` not matching `assign-*.yml`; the caller now names itself so this repo's own routing config doesn't fall through to default_pool. - actionlint clean; all 8 referenced logins confirmed as repo collaborators (GitHub silently drops an unassignable login). `num_reviewers: 1` matches how PRs here are actually tracked — one named owner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mattmillerai
marked this pull request as ready for review
July 26, 2026 07:42
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
…ap glob gaps Review-panel follow-ups on the self-enrollment caller: - Fork PRs: `pull_request` withholds repository secrets from fork-originated runs, so CLOUD_CODE_BOT_PRIVATE_KEY arrives empty and the App-token mint hard fails — a red X on every external contribution for a routing decision that could not have been made anyway. Guard the job to same-repo heads so it skips cleanly, and correct the header, which claimed the App token made fork PRs work here. The one path that would actually route them, `pull_request_target`, is named in the header along with why it is not worth taking (privileged run against untrusted HEAD, while the map is read from the HEAD sha). - Add `reopened` to the trigger types: a PR closed while still unrouted (e.g. its `opened` run failed) otherwise never gets a second routing event. Safe to re-fire — the reusable skips any PR that already has a non-author assignee. - reviewers.yml: `test-bump-callers.yml` does not match `bump-*.yml`, and `ci-detect-unreviewed-merge.yml` is reached by no glob in the housekeeping bucket; both fell through to default_pool. Name them, as every other bucket already names its own `test-*.yml` / `ci-*.yml`. - AGENTS.md: list the new caller alongside the other self-enrollment callers covered by the "pin a merged-main SHA, never a local path" rule. Verified by driving the committed map through the reusable's own hand-rolled YAML parser and globToRegExp (extracted verbatim from assign-reviewers.yml): 7 rules, all non-empty, and all 22 workflow files now route to an intended bucket with nothing falling through. actionlint clean; agents-md-integrity checker passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
2 participants
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.
17 open PRs in this repo had no assignee, so nothing routed them to anyone who could approve them. This repo has shipped an expertise-aware, load-balanced router since BE-1943 and never enrolled itself in it.
Worth stating up front, because the name misleads:
assign-reviewers.ymlwrites the ASSIGNEE field, not reviewer requests. From its own header — "this org routes/alerts people via the ASSIGNEE field". That is precisely the routing that was missing here.Why the map is seeded from approvals, not git history
Nearly every commit on
mainhere has a single author, so authorship carries no routing signal at all. Scoring it would put one name in every bucket. The buckets are instead populated from who has actually approved merged PRs in the matching area:assign-*runs_on/ self-hostedThis is a prerequisite for #63, not a duplicate of it
refresh-reviewers(#63, BE-4116) treats the committedrules:globs as its bucket definitions and only re-scores membership — it cannot bootstrap a map from nothing. Two warnings are recorded in the file itself:default_poolis load-bearingSelection drops the PR author, so a bucket whose only member is the author empties.
default_poolis three proven approvers and deliberately excludes the person who opens most PRs here.Verification
The reusable parses
reviewers.ymlwith a hand-rolled minimal YAML parser (js-yaml isn't available insidegithub-script), so standard YAML validity is not sufficient. I extracted that parser and itsglobToRegExp/matchesAnyverbatim fromassign-reviewers.ymland drove the committed map through them:paths+reviewers;default_poolparsed correctly.default_pool.default_pool.actionlintclean; all 8 referenced logins confirmed repo collaborators (GitHub silently drops an unassignable login, so this is worth checking rather than assuming).The two deviations are deliberate human overrides, not map errors:
That retro pass is also what caught a real gap:
ci-assign-reviewers.ymldoes not matchassign-*.yml, so this repo's own routing caller would have fallen through todefault_pool. It now names itself.num_reviewers: 1matches how PRs here are actually tracked — one named owner, not a pair. Easy to raise if single-owner PRs start stalling.ELI5
Pull requests here were piling up with nobody's name on them, so nobody knew they were meant to look. The repo already had a robot that puts the right person's name on a PR based on which files it touches — it had just never been switched on for itself.
This switches it on and gives it the phone book. The phone book is built from who has actually approved this kind of change before, not from who wrote the code, because one person wrote nearly all the code here and that tells you nothing about who should check it.
🤖 Generated with Claude Code