fix(pr-risk): a repo-owned GitHub App is a runbook candidate, not external (BE-6715) - #140
fix(pr-risk): a repo-owned GitHub App is a runbook candidate, not external (BE-6715)#140mattmillerai wants to merge 3 commits into
external (BE-6715)#140Conversation
…ternal` (BE-6715) The provenance axis classified any PR whose `author_association` is NONE as `external`, pinned R3. A GitHub App is never an org member, so every PR a repo-owned App opens arrives NONE and graded R3 regardless of its diff — measured on one consumer, 14 of 19 R3 grades in a 24-PR sample came from this rather than from the change. It was unfixable consumer-side: the association test ran before both levers (`bot_logins` feeds the login resolver, and `.github/risk-runbooks.json` is read by the shape assertion further down). The fork half stays unconditional and runs FIRST, so a bot on a fork is still `external` R3; only the author-association half is narrowed to non-bots. A non-fork bot falls through to `runbook-candidate`, where the registry and its shape assertion decide, and a candidate with no asserting entry still lands on `human` — identity alone continues to buy no trust. The login string alone could not carry this on the live path: GraphQL reports a Bot login UNSUFFIXED (`cloud-code-bot`, where REST shows `cloud-code-bot[bot]`), so the resolver's `[bot]` test never fires there. The record now also carries `author_is_bot` from `author.__typename == "Bot"` — GitHub's own actor type, not forgeable by the PR — and ORs it in outside the shared `classify_login` jq so that definition does not fork from agent-work's collector. Additive, schema 3 -> 4: a record without the field grades exactly as before.
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 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 (6)
📝 WalkthroughWalkthroughThe PR updates PR risk grading to identify GitHub Apps by actor type, support configured machine-user bots, keep fork and first-time human contributors external, and evaluate eligible non-fork bots through runbooks. Documentation, registry guidance, schema output, and tests are updated. ChangesBot provenance classification
Sequence Diagram(s)sequenceDiagram
participant GitHubGraphQL
participant grade-pr-risk.sh
participant runbook-registry
participant scorecard
GitHubGraphQL->>grade-pr-risk.sh: Return author.__typename
grade-pr-risk.sh->>grade-pr-risk.sh: Classify fork, human, or bot provenance
grade-pr-risk.sh->>runbook-registry: Match bot login variants
runbook-registry-->>grade-pr-risk.sh: Return eligibility or shape failure
grade-pr-risk.sh->>scorecard: Emit author_is_bot in schema version 4
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/pr-risk/grade-pr-risk.sh`:
- Around line 297-302: Update the jq classification chain in the PR risk grading
expression so the `$is_bot` branch precedes the `$agent_coded or $cls ==
"fleet"` branch, routing every non-fork bot to `runbook-candidate` regardless of
labels or fleet configuration. Ensure an unregistered bot resolves to `human`
through the existing downstream behavior, and add regression cases covering
bot-plus-agent-coded and bot-plus-fleet combinations.
In `@scripts/pr-risk/runbook-registry.v0.json`:
- Around line 16-20: Remove the obsolete dependabot-preview[bot] entry from
identity.logins in the runbook registry, preserving the existing dependabot[bot]
and dependabot entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98861850-75ac-49e8-b199-5481c97061dd
📒 Files selected for processing (7)
.github/workflows/pr-risk.ymldocs/callers/pr-risk.mdscripts/pr-risk/README.mdscripts/pr-risk/grade-pr-risk.shscripts/pr-risk/risk-map.v0.jsonscripts/pr-risk/runbook-registry.v0.jsonscripts/pr-risk/tests/test_grade_pr_risk.sh
![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 9 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 4 |
| 🟢 Low | 5 |
Panel: 8/8 reviewers contributed findings.
…(BE-6715) Review follow-up on this PR (CodeRabbit). Narrowing the author-association half made the `agent_coded or fleet` branch REACHABLE for a GitHub App for the first time — before, every App PR was pinned `external` two branches earlier and never got there. Left above the bot test, either half classifies an UNREGISTERED bot `agent-supervised`, contradicting this axis's stated promise (and the map's `_why`) that a non-fork bot with no asserting entry falls back to `human`. The `fleet` half is newly colliding for a second reason: `$is_bot` no longer comes from the login string alone, so a Bot actor whose GraphQL login arrives unsuffixed and also sits in `fleet_logins` classified "fleet" while GitHub said Bot — the use site contradicting `classify_login`'s own bot-beats-fleet rule. So the bot test now runs first. A registered producer was never at risk either way (the shape assertion resolves `$rbk != null` ahead of the base class), and the default map tiers `agent-supervised` and `human` both R1 so no shipped grade moves. What it closes is a consumer map that tiers the two apart: such a map would hand R0 to any bot PR someone labelled `agent-coded`, with no registry entry and no shape assertion behind it. Trust in this axis is earned by the shape assertion, never by a label. Also drops `dependabot-preview[bot]` from the default registry: the Dependabot Preview service shut down in Aug 2021, and the dead login is suffix-only, which is exactly what this PR's new "list BOTH login forms" note says never asserts on a live grade. Four regression cases added to phase 21; two of them fail on the old ordering. Grader suite 94/0, all six pr-risk suites green, shellcheck clean, defaults validate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…very registry (BE-6715) Cursor-review panel follow-up on this PR (7 findings; the panel's other two were the ordering and dependabot-preview issues fixed in 22663c6). IDENTITY (Medium, 2 reviewers). Telling consumers to list the bare slug alongside `x[bot]` made registry identity ambiguous: the matcher compares login strings only, so a same-named USER account satisfied a bare-slug entry and inherited that runbook. The grader now synthesizes the suffixed form instead — ONE way (added, never stripped) and only when `author_is_bot`, GitHub's own actor type, says Bot. An App reaches an `"x[bot]"` entry; a user named `x` cannot, because nothing turns `"x[bot]"` into `"x"`. That asymmetry is what makes this safe where the two-sided normalization the description declined would not be. Registry advice inverts accordingly: name an App by its SUFFIXED login alone. Bare slugs still match literally — machine USER accounts need that — with the same-named-human caveat now stated. This also mostly retires the silent identity miss a third finding raised: the convention it depended on remembering is now the grader's job. STRICT BOOLEAN (Low). `author_is_bot` is the one field that can switch the `external` guard off, and jq truthiness reads the string "false", 0 and "" as true. Tested `== true` now, so only the literal boolean counts. BOT_LOGINS AUTHORITY (Medium + Low, 5 reviewers). Not a code change: the input is genuinely a tier-lowering knob now — a listed login skips the first-time-contributor test, moving a non-fork NONE PR from `external` R3 to `human` R1 — and nothing validates that the login is a machine account. Gating it on `author_is_bot` was suggested but would delete the feature, since machine USER accounts are typed `User` and are the only reason the input exists. Documented as load-bearing in both the input description and the caller doc instead. STATUS TWIN (Medium). Documented why `author_is_bot` deliberately has none: the twins exist where the un-collected default would ANSWER, and this one defaults to the STRICTER path (back through the association test), so no rule is retired. The schema-3/schema-4 corpus divergence is a versioned behaviour change, which is what `schema_version` records. FIXTURE (Low). Phase 22 left the rollup PENDING from the inherited fixture, so its SUCCESS CheckRun was never consulted and reversibility floored R2 unread. Rollup set to SUCCESS; the phase now asserts the green rollup is read and the overall grade is R1 from the diff. Grader suite 102/0 (+8). Every new guard mutation-tested: reverting the normalization fails 2, reverting `== true` fails 3. All six pr-risk suites green, shellcheck clean, defaults validate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review round resolved — 11 threads (2 CodeRabbit, 9 from the cursor-review panel), all addressed and resolved across 22663c6 and adfc838. CI green; suite is 102/0, up from 87. Two findings changed the design, not just the code:
Smaller: Declined, with reasoning on the threads: gating Every new guard is mutation-tested: reverting the normalization fails 2 assertions, reverting |
2 participants
ELI-5
The risk grader gave every robot-opened PR the scariest score (R3) no matter what the PR actually changed. That is because GitHub says a robot is "NONE" when asked how it relates to the repo — the same answer it gives a total stranger — and the grader treated "NONE" as "stranger from outside". So the label stopped telling you anything about the diff, and worse, it trained people to ignore R3. This teaches the grader to check "is this from a fork?" first (that guard is untouched and still absolute), and only then treat "NONE" as stranger-ish — and only for authors that are not robots. A robot with no registered runbook still gets no trust; it just gets graded on its diff instead of on its account type.
What changed
grade-pr-risk.sh, provenance axis ($base_class):is_fork == true→external, unconditionally, tested first. Unchanged and load-bearing: a bot opening a PR from a fork is stillexternalR3.author_association IN (FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, NONE)→externalonly for authors not classified as a bot. The first-time-contributor guard is untouched for humans.runbook-candidatepath, where the registry entry's identity + shape assertion decide. A candidate with no asserting entry still resolves tohuman— identity alone buys no trust, as before.unknownhandling is unchanged: a non-okprovenance_statusorlabels_statusstill refuses to grade the axis.The ordering constraint from the ticket is enforced by construction and pinned by tests: the bot test cannot run before the fork test.
One thing the ticket did not anticipate, and why the diff is bigger than "narrow the
or"The plan says to key on "authors that are known bots (
[bot]suffix or thebot_loginsinput)". That is not sufficient on the live path, and I found it by grading the real PR rather than by reasoning about it. GraphQL reports a Bot actor's login without the[bot]suffix. The API read that backs--prreturnsauthor.login = "cloud-code-bot"where REST and the web UI both showcloud-code-bot[bot], so the resolver's suffix test can only ever fire on a REST-shaped or offline-corpus record — never on a CI grade. With the narrowing alone, the named PR still gradedexternalR3 (verified before adding this).So the record now also carries
author_is_bot, fromauthor.__typename == "Bot"— GitHub's own actor resolution for that account, in the same spirit as "the author login is the resolution GitHub itself made", and not forgeable by the PR being graded. It is ORed into the bot test rather than folded intoclassify_login, because that jq is shared verbatim with the fleet's offline collector and a field only this collector emits would fork the shared definition. Additive:schema_version3 → 4, read as// false, so an existing schema-3 corpus record grades exactly as it did.Corollary, revised after review. The first cut told consumers to list both login forms. The panel pointed out that a bare slug is matchable by a same-named human, who would then inherit the runbook — so the grader now restores the suffix itself: when
author_is_botsaysBotand the login carries none, it matches againstxandx[bot]. One direction only (synthesized, never stripped), so an App reaches an"x[bot]"entry and a user namedxcannot. Registry advice therefore inverts, in the same three places: name an App by its suffixed login alone. Bare slugs still match literally — machine USER accounts need that — with the impersonation caveat stated.Chesterton's Fence
This changes an existing guard, so:
git log -Lon those lines shows theexternalbranch has one origin, #111 (feat(pr-risk): advisory PR risk grader…), where it landed as part of the initial grader with the intent recorded in the map comment — "an unreviewed outside diff is the one provenance class never routed unattended". The intent is about outsiders, and a repo-owned App is not one; theNONEstring was standing in for "outsider" and turned out to over-match. The half of the rule that actually implements the intent — the fork flag, which is what makes a diff outside — is untouched and now runs first. The map comment, the README, and the loader's own quoted invariant are all updated in the same commit so the promise and the code still agree.Verification
Repo suites (all green, run from
scripts/pr-risk/), matchingtest-pr-risk.ymljob for job:shellcheck -xover all 11 scripts — clean87 passed, 0 failed(was 63; +24 across two new phases)48/0, targets105/0, pin-contract20/0, publish-surfaces67/0, resolve-enabled27/0check_agents_md.py --root .— cleanNew hermetic cases — phase 21 (
--stdin, the corpus/REST record shape) and phase 22 (the--prpath with a stubbedghwhose fixture mirrors the real GraphQL response,{"login":"cloud-code-bot","__typename":"Bot"}). Both phases cover all four acceptance cases, plus a registered App whose diff shape fails, a--bot-loginsmachine user, and an entry that lists only the suffixed login.Live regression check, real API: PRs #132/#98/#93 in this repo grade byte-identical before and after.
Acceptance criteria
author_association: NONEclassifiesrunbook-candidateand gradesrunbook/R0 when a registry entry asserts.external→ R3 (both record shapes).NONEstill classifiesexternal→ R3.human, notrunbook.--stdinsuite (and to the stubbed live path), run bytest-pr-risk.yml.provenance: runbook, iddata-snapshot-refresh, R0 on that axis (before:external, R3). Two deviations. (a) That repo has no.github/risk-runbooks.jsontoday — a 404 — so I verified with an equivalent registry passed via--runbooks; the consumer still has to commit the file (listing both login forms) for this to take effect there. (b) The grade lands R2, decided by reversibility, not R1 from the path floor: that PR's check rollup isFAILURE, so reversibility is pinned R2 by "no GREEN check rollup", and its single path matches no rule in the default map, so the path floor is R0, not R1. The AC's R1 presumably assumed a consumer.github/risk.jsonthat also does not exist yet. Either way the grade now responds to the diff and the rollup instead of to the author's account type, which is the point.Judgment calls
Botactor type, because criteria 1 and 6 are unreachable without it (or without every consumer hand-listing each App inbot_logins, which is the same "consumer cannot fix it" complaint the ticket opens with). Flagging it explicitly since it is scope the ticket did not ask for.Did not touch the runbook identity matcher— reversed after review. The original call was that normalizing[bot]suffixes would broaden a trust check, because a human could register the plain username and match an App's entry. That is true of two-sided normalization; it is not true of the one-way, actor-type-gated synthesis that shipped instead (see the corollary above), which only ever adds the suffix and only when GitHub itself types the authorBot. Pinned by a test that grades the same fixture as aUserand asserts it does not inherit the runbook.agent-codedlabel / a bot login infleet_loginsnow resolve torunbook-candidate, notagent-supervised. Corrected after review (CodeRabbit): narrowing the association half made that branch reachable for an App for the first time — before, every App PR was pinnedexternaltwo branches earlier and never reached it, so "pre-existing ordering" was the wrong read. A registered producer was never affected either way (the shape assertion resolves ahead of the base class) and the default map tiersagent-supervisedandhumanboth R1, so no shipped grade moves; what it closes is a consumer map that tiers the two apart, where anagent-codedlabel alone would have bought a bot R0 with no registry entry behind it. Four regression cases in phase 21, two of which fail on the old ordering.dependabot-preview[bot]from the default registry (also CodeRabbit): the Dependabot Preview service shut down in Aug 2021, and the dead login is suffix-only — exactly what this PR's new "list BOTH login forms" note says never asserts on a live grade.pr-risk.yml(one input description) adds no extra fleet churn:bump-pr-risk-callers.ymlalready triggers onscripts/pr-risk/**.Provenance
Ticket BE-6715. Negative-claim falsification does not apply — this diff grants a capability (bot PRs can reach
runbook/humaninstead of being pinned R3) rather than denying one; no deny/dead-end path or capability-denying string was added. The behavior was nevertheless confirmed empirically against the live API, not just against tests I wrote.