[PR #140] [MERGED] fix(pr-risk): a repo-owned GitHub App is a runbook candidate, not `external` (BE-6715) #150

New issue

Closed

opened 2026-10-26 22:36:47 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:47 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/140
Author: @mattmillerai
Created: 8/5/2026
Status: ✅ Merged
Merged: 8/6/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-6715-pr-risk-bot-provenance

📝 Commits (3)

f73860b fix(pr-risk): a repo-owned GitHub App is a runbook candidate, not external (BE-6715)
22663c6 fix(pr-risk): a label must not route a bot past the runbook registry (BE-6715)
adfc838 fix(pr-risk): restore an App's [bot] suffix in the grader, not in every registry (BE-6715)

📊 Changes

7 files changed (+335 additions, -18 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+7 -2)
📝 docs/callers/pr-risk.md (+2 -2)
📝 scripts/pr-risk/README.md (+18 -4)
📝 scripts/pr-risk/grade-pr-risk.sh (+94 -8)
📝 scripts/pr-risk/risk-map.v0.json (+1 -1)
📝 scripts/pr-risk/runbook-registry.v0.json (+9 -1)
📝 scripts/pr-risk/tests/test_grade_pr_risk.sh (+204 -0)

📄 Description

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 still external R3.
author_association IN (FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, NONE) → external only for authors not classified as a bot. The first-time-contributor guard is untouched for humans.
A non-fork bot falls through to the existing runbook-candidate path, where the registry entry's identity + shape assertion decide. A candidate with no asserting entry still resolves to human — identity alone buys no trust, as before.
unknown handling is unchanged: a non-ok provenance_status or labels_status still 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 the bot_logins input)". 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 --pr returns author.login = "cloud-code-bot" where REST and the web UI both show cloud-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 graded external R3 (verified before adding this).

So the record now also carries author_is_bot, from author.__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 into classify_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_version 3 → 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_bot says Bot and the login carries none, it matches against x and x[bot]. One direction only (synthesized, never stripped), so an App reaches an "x[bot]" entry and a user named x cannot. 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 -L on those lines shows the external branch 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; the NONE string 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/), matching test-pr-risk.yml job for job:

shellcheck -x over all 11 scripts — clean
default map + registry structural validation — passes
grader suite 87 passed, 0 failed (was 63; +24 across two new phases)
label 48/0, targets 105/0, pin-contract 20/0, publish-surfaces 67/0, resolve-enabled 27/0
check_agents_md.py --root . — clean

New hermetic cases — phase 21 (--stdin, the corpus/REST record shape) and phase 22 (the --pr path with a stubbed gh whose 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-logins machine 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

✅ Non-fork App PR with author_association: NONE classifies runbook-candidate and grades runbook/R0 when a registry entry asserts.
✅ Fork PR authored by a bot still classifies external → R3 (both record shapes).
✅ Non-fork human with NONE still classifies external → R3.
✅ Bot with no registry entry grades human, not runbook.
✅ Hermetic cases added to the --stdin suite (and to the stubbed live path), run by test-pr-risk.yml.
⚠️ Partially, and the AC's own numbers do not hold — read this one. The named PR was re-graded live against the real API. Provenance is fixed and the substance of the AC is met: with a registry entry present it now yields provenance: runbook, id data-snapshot-refresh, R0 on that axis (before: external, R3). Two deviations. (a) That repo has no .github/risk-runbooks.json today — 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 is FAILURE, 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.json that 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

Widened past the literal plan to add the Bot actor type, because criteria 1 and 6 are unreachable without it (or without every consumer hand-listing each App in bot_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 author Bot. Pinned by a test that grades the same fixture as a User and asserts it does not inherit the runbook.
Bot + agent-coded label / a bot login in fleet_logins now resolve to runbook-candidate, not agent-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 pinned external two 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 tiers agent-supervised and human both R1, so no shipped grade moves; what it closes is a consumer map that tiers the two apart, where an agent-coded label 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.
Dropped 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.
Touching pr-risk.yml (one input description) adds no extra fleet churn: bump-pr-risk-callers.yml already triggers on scripts/pr-risk/**.

Provenance

Ticket BE-6715. Negative-claim falsification does not apply — this diff grants a capability (bot PRs can reach runbook/human instead 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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/140 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/5/2026 **Status:** ✅ Merged **Merged:** 8/6/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6715-pr-risk-bot-provenance` --- ### 📝 Commits (3) - [`f73860b`](https://github.com/Comfy-Org/github-workflows/commit/f73860b3e8079d04e0946e607c184e1f8649c889) fix(pr-risk): a repo-owned GitHub App is a runbook candidate, not `external` (BE-6715) - [`22663c6`](https://github.com/Comfy-Org/github-workflows/commit/22663c6919a09e1e3d858a2ed2e0a19eba3d0070) fix(pr-risk): a label must not route a bot past the runbook registry (BE-6715) - [`adfc838`](https://github.com/Comfy-Org/github-workflows/commit/adfc838e7233f1240c5f9cc179e9c6fe2ba36f1f) fix(pr-risk): restore an App's `[bot]` suffix in the grader, not in every registry (BE-6715) ### 📊 Changes **7 files changed** (+335 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+7 -2) 📝 `docs/callers/pr-risk.md` (+2 -2) 📝 `scripts/pr-risk/README.md` (+18 -4) 📝 `scripts/pr-risk/grade-pr-risk.sh` (+94 -8) 📝 `scripts/pr-risk/risk-map.v0.json` (+1 -1) 📝 `scripts/pr-risk/runbook-registry.v0.json` (+9 -1) 📝 `scripts/pr-risk/tests/test_grade_pr_risk.sh` (+204 -0) </details> ### 📄 Description ## 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`): 1. `is_fork == true` → `external`, unconditionally, tested **first**. Unchanged and load-bearing: a bot opening a PR from a fork is still `external` R3. 2. `author_association IN (FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, NONE)` → `external` **only for authors not classified as a bot**. The first-time-contributor guard is untouched for humans. 3. A non-fork bot falls through to the existing `runbook-candidate` path, where the registry entry's identity + shape assertion decide. A candidate with no asserting entry still resolves to `human` — identity alone buys no trust, as before. 4. `unknown` handling is unchanged: a non-`ok` `provenance_status` or `labels_status` still 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 the `bot_logins` input)". 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 `--pr` returns `author.login = "cloud-code-bot"` where REST and the web UI both show `cloud-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 graded `external` R3 (verified before adding this). So the record now also carries `author_is_bot`, from `author.__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 into `classify_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_version` 3 → 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_bot` says `Bot` and the login carries none, it matches against `x` **and** `x[bot]`. One direction only (synthesized, never stripped), so an App reaches an `"x[bot]"` entry and a user named `x` cannot. 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 -L` on those lines shows the `external` branch 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; the `NONE` string 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/`), matching `test-pr-risk.yml` job for job: - `shellcheck -x` over all 11 scripts — clean - default map + registry structural validation — passes - grader suite `87 passed, 0 failed` (was 63; +24 across two new phases) - label `48/0`, targets `105/0`, pin-contract `20/0`, publish-surfaces `67/0`, resolve-enabled `27/0` - `check_agents_md.py --root .` — clean New hermetic cases — **phase 21** (`--stdin`, the corpus/REST record shape) and **phase 22** (the `--pr` path with a stubbed `gh` whose 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-logins` machine 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 1. ✅ Non-fork App PR with `author_association: NONE` classifies `runbook-candidate` and grades `runbook`/R0 when a registry entry asserts. 2. ✅ Fork PR authored by a bot still classifies `external` → R3 (both record shapes). 3. ✅ Non-fork human with `NONE` still classifies `external` → R3. 4. ✅ Bot with no registry entry grades `human`, not `runbook`. 5. ✅ Hermetic cases added to the `--stdin` suite (and to the stubbed live path), run by `test-pr-risk.yml`. 6. ⚠️ **Partially, and the AC's own numbers do not hold — read this one.** The named PR was re-graded live against the real API. Provenance is fixed and the substance of the AC is met: with a registry entry present it now yields `provenance: runbook`, id `data-snapshot-refresh`, R0 on that axis (before: `external`, R3). Two deviations. (a) That repo has **no `.github/risk-runbooks.json` today** — 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 is `FAILURE`, 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.json` that 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 - **Widened past the literal plan** to add the `Bot` actor type, because criteria 1 and 6 are unreachable without it (or without every consumer hand-listing each App in `bot_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 author `Bot`. Pinned by a test that grades the same fixture as a `User` and asserts it does not inherit the runbook. - **Bot + `agent-coded` label / a bot login in `fleet_logins`** now resolve to `runbook-candidate`, not `agent-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 pinned `external` two 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 tiers `agent-supervised` and `human` both R1, so no shipped grade moves; what it closes is a consumer map that tiers the two apart, where an `agent-coded` label 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. - **Dropped `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. - Touching `pr-risk.yml` (one input description) adds no extra fleet churn: `bump-pr-risk-callers.yml` already triggers on `scripts/pr-risk/**`. ## Provenance Ticket BE-6715. Negative-claim falsification does not apply — this diff *grants* a capability (bot PRs can reach `runbook`/`human` instead 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:47 +00:00