From f73860b3e8079d04e0946e607c184e1f8649c889 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 5 Aug 2026 16:24:26 -0700 Subject: [PATCH 1/3] fix(pr-risk): a repo-owned GitHub App is a runbook candidate, not `external` (BE-6715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/pr-risk.yml | 6 +- docs/callers/pr-risk.md | 2 +- scripts/pr-risk/README.md | 20 ++- scripts/pr-risk/grade-pr-risk.sh | 52 ++++++-- scripts/pr-risk/risk-map.v0.json | 2 +- scripts/pr-risk/runbook-registry.v0.json | 5 + scripts/pr-risk/tests/test_grade_pr_risk.sh | 127 ++++++++++++++++++++ 7 files changed, 199 insertions(+), 15 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 4aa81d9..6915384 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -258,8 +258,10 @@ on: bot_logins: description: >- Extra logins treated as bots, on top of any `[bot]`-suffixed login - (comma-separated). A bot with no runbook registry entry grades as - human — identity alone never buys trust. + and any author GitHub itself types as a `Bot` (comma-separated). A + real GitHub App needs no entry here; a machine USER account does. A + bot with no runbook registry entry grades as human — identity alone + never buys trust. type: string required: false default: github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow diff --git a/docs/callers/pr-risk.md b/docs/callers/pr-risk.md index 7d6e9e5..c5aabff 100644 --- a/docs/callers/pr-risk.md +++ b/docs/callers/pr-risk.md @@ -79,7 +79,7 @@ statuses: read |---|---|---| | `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose: a floating default let a caller SHA-pin `uses:` and still load the grader from HEAD of main. | | `fleet_logins` | `mattmillerai` | Logins whose PRs grade provenance `agent-supervised` alongside `agent-coded`. | -| `bot_logins` | `github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow` | Extra logins treated as bots. A bot with no runbook entry still grades as human — identity alone buys no trust. | +| `bot_logins` | `github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow` | Extra logins treated as bots. Needed only for **machine USER accounts** — a real GitHub App is recognized from GitHub's own actor type, no list entry required. A bot with no runbook entry still grades as human — identity alone buys no trust. | | `label_map` | `''` | Rename the five grader-owned labels as `tier=label` pairs. Tier keys are fixed; only the label text is yours. | | `wait_for_checks_minutes` | `10` | How long to wait for the rest of the check rollup to settle before labeling (clamped to 25 — what a 30-minute job can spend waiting). `0` labels immediately, expect R2 floors from still-pending checks. | | `repo_map_path` | `.github/risk.json` | Consumer risk-map override, read from the PR **base ref**. | diff --git a/scripts/pr-risk/README.md b/scripts/pr-risk/README.md index 9ce5413..633d694 100644 --- a/scripts/pr-risk/README.md +++ b/scripts/pr-risk/README.md @@ -34,9 +34,16 @@ record. 2. **Provenance** — what PROCESS produced the diff: `runbook` (a registered producer in [`runbook-registry.v0.json`](runbook-registry.v0.json) whose identity AND diff shape both assert), `agent-supervised`, `human`, or - `external` (fork / first-time contributor — R3, no exceptions, even when a - runbook shape matches). Identity is the server-attributed author login, - never the forgeable commit author string. + `external` (any fork, or a first-time **human** contributor — R3, no + exceptions, even when a runbook shape matches). Identity is the + server-attributed author login, never the forgeable commit author string. + A **non-fork bot is never `external`**, even though every GitHub App authors + with `author_association: NONE` — it is a runbook candidate, judged by the + registry's shape assertion, and falls back to `human` when no entry asserts. + "Is this a bot?" is answered from GitHub's own actor type (`Bot`) plus the + `bot_logins` list, so an App needs no list entry and a machine *user* account + does. The fork half is unconditional, so a bot opening a PR *from a fork* is + still `external` R3. 3. **Reversibility** — mutates persistent state or deletes data → R3; **removes** a file under a sensitive class → R3 (a delete, or a rename out of that class); no green check rollup → R2; green but no test file touched → R1; green with @@ -177,7 +184,12 @@ them by committing: - `.github/risk.json` — the repo's own path→tier map (same schema as [`risk-map.v0.json`](risk-map.v0.json)) - `.github/risk-runbooks.json` — the repo's own producer registry (same schema - as [`runbook-registry.v0.json`](runbook-registry.v0.json)) + as [`runbook-registry.v0.json`](runbook-registry.v0.json)). **List BOTH login + forms for a GitHub App** — `"my-bot[bot]"` *and* `"my-bot"` — exactly as the + shipped `dependabot` entry does. REST and the web UI show the suffixed form, + but the grader reads the PR over GraphQL, which reports a Bot login + **unsuffixed**; an entry that lists only `my-bot[bot]` never asserts on a live + grade, and the producer falls back to `human`. Both are read from the PR's **base ref**, so a PR cannot edit the rules that judge it (editing them — or the grader — at all is R3 by the map's own first diff --git a/scripts/pr-risk/grade-pr-risk.sh b/scripts/pr-risk/grade-pr-risk.sh index 9797da3..7b5b57b 100755 --- a/scripts/pr-risk/grade-pr-risk.sh +++ b/scripts/pr-risk/grade-pr-risk.sh @@ -30,7 +30,9 @@ # is the WORST tier over every rule any changed path matches. # AXIS 2 — PROVENANCE. runbook / agent-supervised / human / external. A PR whose identity # matches a runbook but whose DIFF SHAPE does not is not a runbook, and falls back to its -# underlying class. `external` (fork / first-time contributor) is R3 on provenance alone. +# underlying class. `external` (any fork, or a first-time HUMAN contributor) is R3 on +# provenance alone; a non-fork bot is a runbook candidate, not an outsider, because every +# GitHub App authors with `author_association: NONE`. # AXIS 3 — REVERSIBILITY. Single clean revert? Mutates persistent state or deletes data? # Did tests covering the touched lines actually run? Answered from the changed-path list # + change types + the PR's own check rollup. @@ -126,8 +128,8 @@ read_map() { # -> JSON on stdout, rc 0; rc 1 + reaso then "provenance_tiers carries a tier outside \(known)" # EVERY provenance class must be MAPPED, not just well-typed. Checking only the values let # a map that OMITS `external` pass, and the lookup then fell back to a tier of its own - # choosing — silently retiring the "external (fork / first-time contributor) is R3, no - # exceptions" invariant that both the default map comment and the README promise. A class + # choosing — silently retiring the "external (a fork, or a first-time human contributor) is + # R3, no exceptions" invariant that the default map comment and the README promise. A class # nobody mapped is a routing decision nobody made, so it is refused at load time. # (No apostrophes in here: the whole shape program is a single-quoted shell string.) elif ((["runbook","agent-supervised","human","external"] - [.provenance_tiers | keys[]]) | length) > 0 @@ -252,6 +254,18 @@ cat <<'JQ' # runbook — provenance alone is never sufficient. | ($r.author // null) as $author | ($author | classify_login($fleetl; $botl)) as $cls + # IS THE AUTHOR A BOT? The shared resolver answers from the login STRING — a `[bot]` suffix or + # the caller's `bot_logins` list — and on the live path that string is not enough on its own. + # GraphQL reports a Bot actor's login WITHOUT the `[bot]` suffix (`cloud-code-bot`, where REST + # and the web UI both show `cloud-code-bot[bot]`), so the suffix test can only ever fire on a + # REST-shaped or offline-corpus record. `author.__typename == "Bot"` is GitHub's OWN actor + # type for the same account and is not forgeable by the PR, so it is collected alongside the + # login and ORed in here. It is additive: a record without the field (every pre-schema-4 + # corpus row) falls back to the resolver's answer exactly as before, and `bot_logins` still + # works for machine USER accounts, which are typed `User` and carry no suffix at all. + # Deliberately kept OUT of classify_login: that jq is shared verbatim with agent-work's + # grade-collect.sh, and a field only this collector emits would fork the shared definition. + | (($r.author_is_bot // false) or $cls == "bot") as $is_bot | (($r.labels // []) | index("agent-coded") != null) as $agent_coded # The label list has a STATUS TWIN for the same reason the file list does: `agent-coded` is # read from it, and a TRUNCATED label list answers "is this agent-coded?" with a confident no @@ -264,11 +278,28 @@ cat <<'JQ' # class never routed unattended — silently unreachable. Unread is `unknown`, and `unknown` # refuses to grade the axis. | ($r.provenance_status // (if ($r | has("is_fork")) then "ok" else "absent" end)) as $pvst + # ORDER IS THE RULE HERE, AND IT IS NOT REARRANGEABLE. The fork test runs FIRST and stays + # unconditional: a fork PR is `external` no matter who authored it, so a bot on a fork can + # never escape R3 by presenting a bot login. Only AFTER that does the author-association half + # get narrowed to authors the shared resolver does NOT read as a bot. + # + # WHY THE NARROWING. A GitHub App is never an org member, so EVERY PR opened by a repo-owned + # App arrives with `author_association: NONE` — and testing that string before the login + # classification pinned every such PR `external` => R3 regardless of what it changed (measured + # on one consumer: 14 of 19 R3 grades in a 24-PR sample came from this, not from the diff). + # It was also unfixable from the consumer side, because both levers sit further down: the + # `bot_logins` input feeds `classify_login`, and `.github/risk-runbooks.json` is read by the + # shape assertion below. A non-fork bot now falls through to `runbook-candidate`, where the + # registry and its shape assertion decide — and a `runbook-candidate` with no asserting entry + # still lands on `human` below, so identity alone continues to buy no trust. + # The first-time-contributor guard is UNCHANGED for humans: `$cls` is "human" (or "unknown") + # for anyone the resolver does not classify as a bot, so a human's NONE still reads external. | (if $pvst != "ok" or $lbst != "ok" then "unknown" - elif ($r.is_fork // false) or (($r.author_association // "") | IN("FIRST_TIME_CONTRIBUTOR","FIRST_TIMER","NONE")) + elif ($r.is_fork // false) then "external" + elif ($is_bot | not) and (($r.author_association // "") | IN("FIRST_TIME_CONTRIBUTOR","FIRST_TIMER","NONE")) then "external" elif $agent_coded or $cls == "fleet" then "agent-supervised" - elif $cls == "bot" then "runbook-candidate" + elif $is_bot then "runbook-candidate" elif $cls == "human" then "human" else "unknown" end) as $base_class # The shape assertion: identity match AND every changed path inside permitted_paths AND the @@ -467,7 +498,7 @@ fetch_pr_record() { # -> record JSON on stdout, rc 1 on an unreadab q='query($owner:String!,$name:String!,$num:Int!){ repository(owner:$owner,name:$name){ pullRequest(number:$num){ number title state isDraft createdAt updatedAt closedAt mergedAt - author{ login } authorAssociation baseRefName headRefName isCrossRepository + author{ login __typename } authorAssociation baseRefName headRefName isCrossRepository additions deletions changedFiles labels(first:100){ pageInfo{ hasNextPage } nodes{ name } } commits(last:1){ nodes{ commit{ oid statusCheckRollup{ state @@ -613,7 +644,14 @@ fetch_pr_record() { # -> record JSON on stdout, rc 1 on an unreadab then {list: null, status: "unknown", reason: "changed-file list is short (\($files | length) of \(.changedFiles)) — GitHub caps the files API at 3000 files"} else {list: $files, status: "ok", reason: null} end) as $fread - | {schema_version:3, repo:$repo, pr:.number, title:.title, author:(.author.login // null), + | {schema_version:4, repo:$repo, pr:.number, title:.title, author:(.author.login // null), + # The actor type GitHub itself assigns the author, recorded because the LOGIN cannot answer + # it here: GraphQL reports a Bot login UNSUFFIXED, so `cloud-code-bot[bot]` arrives as + # `cloud-code-bot` and the `[bot]` test in the shared resolver never fires on this path. + # Additive (schema 3 -> 4): the grader reads it as `// false`, so a schema-3 corpus record + # still grades exactly as it did. + # (No apostrophes in here: this whole jq program is a single-quoted shell string.) + author_is_bot:((.author.__typename // "") == "Bot"), author_association:.authorAssociation, is_fork:(.isCrossRepository // false), labels:$labels, agent_coded:($labels | index("agent-coded") != null), labels_status:(if $labels_trunc then "unknown" else "ok" end), diff --git a/scripts/pr-risk/risk-map.v0.json b/scripts/pr-risk/risk-map.v0.json index 5f05718..991a5f5 100644 --- a/scripts/pr-risk/risk-map.v0.json +++ b/scripts/pr-risk/risk-map.v0.json @@ -68,7 +68,7 @@ "agent-supervised": "R1", "human": "R1", "external": "R3", - "_why": "external (a fork / first-time contributor) is R3 on provenance alone — an unreviewed outside diff is the one provenance class never routed unattended. runbook is R0 ONLY when the shape assertion also holds (see the runbook registry); a claimed-but-shape-failed runbook falls back to its underlying class." + "_why": "external (ANY fork, or a first-time HUMAN contributor) is R3 on provenance alone — an unreviewed outside diff is the one provenance class never routed unattended. A non-fork bot is NOT external even though every GitHub App authors with author_association NONE: it is a runbook candidate, graded by the registry's shape assertion, and falls back to human when no entry asserts. runbook is R0 ONLY when the shape assertion also holds (see the runbook registry); a claimed-but-shape-failed runbook falls back to its underlying class." }, "reversibility": { diff --git a/scripts/pr-risk/runbook-registry.v0.json b/scripts/pr-risk/runbook-registry.v0.json index 74dc25d..504446c 100644 --- a/scripts/pr-risk/runbook-registry.v0.json +++ b/scripts/pr-risk/runbook-registry.v0.json @@ -13,6 +13,11 @@ "IDENTITY is the server-attributed PR author login (and, where the producer is a workflow,", "the head-ref shape that workflow pushes) — never the forgeable commit author string.", "", + "LIST BOTH LOGIN FORMS FOR A GITHUB APP, as the dependabot entry below does: \"x[bot]\" AND", + "\"x\". REST and the web UI show the suffixed form, but the grader reads the PR over GraphQL,", + "which reports a Bot login UNSUFFIXED — so an entry carrying only \"x[bot]\" never asserts on", + "a live grade and its producer silently falls back to `human`.", + "", "This default registers only ecosystem-universal producers. A consumer repo registers its", "own producers (release bots, spec-sync pushes, caller bumpers) by committing", ".github/risk-runbooks.json; the workflow reads it from the PR's BASE ref, so a PR cannot", diff --git a/scripts/pr-risk/tests/test_grade_pr_risk.sh b/scripts/pr-risk/tests/test_grade_pr_risk.sh index c56b72d..6002d60 100755 --- a/scripts/pr-risk/tests/test_grade_pr_risk.sh +++ b/scripts/pr-risk/tests/test_grade_pr_risk.sh @@ -8,6 +8,9 @@ # * PROVENANCE ALONE IS NEVER SUFFICIENT: a runbook IDENTITY whose diff SHAPE does not # assert is not a runbook, and the failure is recorded. # * EXTERNAL IS NEVER OVERRIDDEN: a fork imitating a runbook's shape is still external R3. +# * EXTERNAL IS ABOUT FORKS AND FIRST-TIME HUMANS (new here): a repo-owned App authors with +# `author_association: NONE`, so it is a runbook candidate rather than an outsider — while a +# bot on a FORK, and a first-time human, both stay external R3. # * THE UNKNOWN CONTRACT: an unreadable input is tier null + status unknown + exit 1, # never a confident tier; a structurally empty map is refused outright (exit 2). # * THE GRADE CARRIES ITS MAP VERSION so a map revision can be replayed later. @@ -400,6 +403,130 @@ out="$(paged_pr --self-run-id 999)" eq "a PR with no checks is untouched by the drain" ok "$(jq -r '.checks_status // "ok"' <<<"$out")" eq "and still grades" ok "$(jq -r '.risk.status' <<<"$out")" +echo "— phase 21: a repo-owned App is a runbook candidate, not an outsider —" +# A GitHub App is never an org member, so EVERY PR a repo-owned App opens arrives with +# `author_association: NONE`. Testing that string BEFORE the login classification graded every +# such PR `external` => R3 regardless of its diff, and no consumer lever could reach it: the +# `bot_logins` input and .github/risk-runbooks.json are both read further down. The fix narrows +# the association half to non-bots; the FORK half stays unconditional, which is what keeps a bot +# on a fork from presenting a bot login to escape R3. +cat > "$SANDBOX/app-runbooks.json" <<'FIX' +{"registry_version":"v0-test","runbooks":[ + {"id":"data-snapshot-refresh", + "why":"a repo-owned App's cron that refreshes one committed data snapshot", + "identity":{"logins":["cloud-code-bot[bot]"]}, + "permitted_paths":["data/*.json"], + "shape":{"max_changed_files":1,"max_additions":5000,"max_deletions":5000}, + "daily_cap":4,"lane":"data-refresh"}]} +FIX +app_grade() { bash "$GRADER" --stdin --runbooks "$SANDBOX/app-runbooks.json" 2>/dev/null; } +SNAP='[{"path":"data/skills.json","additions":40,"deletions":12,"change_type":"MODIFIED"}]' + +# (1) non-fork App PR, author_association NONE, registry entry asserts -> runbook, R0 on the axis. +out="$(rec 22 'cloud-code-bot[bot]' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE false | app_grade)" +eq "a non-fork App PR is not external" runbook "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and names the runbook that asserted" data-snapshot-refresh "$(jq -r '.risk.axes.provenance.runbook' <<<"$out")" +eq "provenance proposes R0" R0 "$(jq -r '.risk.axes.provenance.tier' <<<"$out")" +# The other axes still decide: an unmapped data path floors R0 and green-but-no-test is R1, so +# the grade now RESPONDS to the diff instead of being pinned R3 by the author's association. +eq "the diff, not the association, decides the grade" R1 "$(jq -r '.risk.tier' <<<"$out")" + +# (2) the SAME App PR from a FORK is still external R3. The fork test runs first and is +# unconditional — reordering the two would open exactly this hole. +out="$(rec 23 'cloud-code-bot[bot]' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE true | app_grade)" +eq "a bot on a fork is still external" external "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and still grades R3" R3 "$(jq -r '.risk.tier' <<<"$out")" + +# (3) a non-fork HUMAN with author_association NONE is untouched: the first-time-contributor +# guard is narrowed to non-bots, not removed. +out="$(rec 24 'drive-by-human' 'feat: my first patch' "$SNAP" ok SUCCESS feature NONE false | app_grade)" +eq "a first-time human contributor is still external" external "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and still grades R3" R3 "$(jq -r '.risk.tier' <<<"$out")" + +# (4) identity alone buys NO trust: a bot with no asserting registry entry falls back to human. +out="$(rec 25 'unregistered-bot[bot]' 'chore: something' "$SNAP" ok SUCCESS bot/x NONE false | app_grade)" +eq "an unregistered bot grades human, never runbook" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "human provenance proposes R1, not R0" R1 "$(jq -r '.risk.axes.provenance.tier' <<<"$out")" +# ...and so does a REGISTERED bot whose diff shape does not assert (paths outside its set). +out="$(rec 26 'cloud-code-bot[bot]' 'chore: refresh' '[{"path":"src/evil.go","additions":9,"deletions":0,"change_type":"MODIFIED"}]' ok SUCCESS bot/refresh NONE false | app_grade)" +eq "a registered App failing its shape is human, not runbook" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +sf="$(jq -r '.risk.axes.provenance.shape_failures | length' <<<"$out")" +if [ "$sf" -ge 1 ]; then ok "and the shape failure is recorded"; else bad "and the shape failure is recorded" "$sf"; fi + +# (5) --bot-logins reaches the classification too: a bot login WITHOUT the `[bot]` suffix (a +# machine user, or an App whose login the API reports unsuffixed) is only a bot if the caller +# says so, and it must get the same non-external treatment when it is. +out="$(rec 27 'snapshot-machine' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE false \ + | bash "$GRADER" --stdin --runbooks "$SANDBOX/app-runbooks.json" --bot-logins snapshot-machine 2>/dev/null)" +eq "a --bot-logins machine user is not external" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +out="$(rec 28 'snapshot-machine' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE false | app_grade)" +eq "and without --bot-logins it reads as a first-time human" external "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" + +echo "— phase 22: on the LIVE path the login alone cannot tell you it is a bot —" +# The case above is the corpus/REST record shape. The live path does not see it: GraphQL reports +# a Bot actor login WITHOUT the `[bot]` suffix, so `cloud-code-bot[bot]` arrives as +# `cloud-code-bot` and the resolver's suffix test never fires. Measured on the real PR in the +# ticket: `author.login` is `cloud-code-bot`, `author.__typename` is `Bot`. That typename is +# GitHub's own actor resolution and is what the record carries, so the fix must survive it. +mkdir -p "$SANDBOX/binbot" +jq '.data.repository.pullRequest.author = {"login":"cloud-code-bot","__typename":"Bot"} + | .data.repository.pullRequest.authorAssociation = "NONE" + # phase 12 left changedFiles at 5 on the shared fixture; this stub serves ONE file, and a + # short read is `unknown` by design — so restate it rather than inherit a truncated record. + | .data.repository.pullRequest.changedFiles = 1 + | .data.repository.pullRequest.title = "data: refresh bundled skills snapshot (auto)" + | .data.repository.pullRequest.headRefName = "bot/refresh-skills" + | .data.repository.pullRequest.commits.nodes[0].commit.statusCheckRollup.contexts.nodes = + [{"__typename":"CheckRun","name":"unit tests","status":"COMPLETED","conclusion":"SUCCESS", + "checkSuite":{"workflowRun":{"databaseId":1000,"workflow":{"name":"CI"}}}}]' \ + "$SANDBOX/fixture.json" > "$SANDBOX/botfixture.json" +cat > "$SANDBOX/botfiles.json" <<'FIX' +[{"filename":"data/skills.json","additions":40,"deletions":12,"status":"modified"}] +FIX +cat > "$SANDBOX/binbot/gh" <<'STUB' +#!/usr/bin/env bash +fixture=""; filter="" +for ((i=1; i<=$#; i++)); do + a="${!i}" + case "$a" in + graphql) fixture="$FIXTURE_DIR/botfixture.json" ;; + *pulls/*/files*) fixture="$FIXTURE_DIR/botfiles.json" ;; + --jq) n=$((i+1)); filter="${!n}" ;; + esac +done +[ -n "$fixture" ] || { echo "gh stub: unhandled args: $*" >&2; exit 1; } +if [ -n "$filter" ]; then jq -c "$filter" "$fixture"; else cat "$fixture"; fi +STUB +chmod +x "$SANDBOX/binbot/gh" +bot_pr() { PATH="$SANDBOX/binbot:$PATH" bash "$GRADER" --repo test/repo --pr 42 "$@" 2>/dev/null; } + +out="$(bot_pr)" +eq "the live record carries GitHub's own actor type" true "$(jq -r '.author_is_bot' <<<"$out")" +eq "and the login really does arrive unsuffixed" cloud-code-bot "$(jq -r '.author' <<<"$out")" +eq "an App PR is no longer external on the live path" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +# ...and with the consumer's registry entry it reaches `runbook` — the ticket's end state. +out="$(bot_pr --runbooks "$SANDBOX/app-runbooks.json" --bot-logins '')" +eq "an unsuffixed Bot login still fails an entry that lists only the suffixed form" \ + human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +jq '.runbooks[0].identity.logins = ["cloud-code-bot[bot]","cloud-code-bot"]' \ + "$SANDBOX/app-runbooks.json" > "$SANDBOX/app-runbooks-both.json" +out="$(bot_pr --runbooks "$SANDBOX/app-runbooks-both.json")" +eq "an entry listing BOTH login forms asserts" runbook "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and the tier now responds to the diff" R0 "$(jq -r '.risk.axes.provenance.tier' <<<"$out")" +# The fork half is unconditional on the live path too — the API fork flag, not the actor. +jq '.data.repository.pullRequest.isCrossRepository = true' \ + "$SANDBOX/botfixture.json" > "$SANDBOX/bf2.json" && cp "$SANDBOX/bf2.json" "$SANDBOX/botfixture.json" +out="$(bot_pr --runbooks "$SANDBOX/app-runbooks-both.json")" +eq "a Bot author on a fork is still external R3" external "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and the grade is R3" R3 "$(jq -r '.risk.tier' <<<"$out")" +# A HUMAN author on the same live path keeps the first-time-contributor guard. +jq '.data.repository.pullRequest.isCrossRepository = false + | .data.repository.pullRequest.author = {"login":"drive-by","__typename":"User"}' \ + "$SANDBOX/botfixture.json" > "$SANDBOX/bf2.json" && cp "$SANDBOX/bf2.json" "$SANDBOX/botfixture.json" +out="$(bot_pr --runbooks "$SANDBOX/app-runbooks-both.json")" +eq "a live first-time human is still external" external "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and still grades R3" R3 "$(jq -r '.risk.tier' <<<"$out")" + echo echo "passed $PASS, failed $FAIL" [ "$FAIL" -eq 0 ] From 22663c6919a09e1e3d858a2ed2e0a19eba3d0070 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 5 Aug 2026 16:36:29 -0700 Subject: [PATCH 2/3] fix(pr-risk): a label must not route a bot past the runbook registry (BE-6715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/callers/pr-risk.md | 2 +- scripts/pr-risk/grade-pr-risk.sh | 21 +++++++++++- scripts/pr-risk/runbook-registry.v0.json | 2 +- scripts/pr-risk/tests/test_grade_pr_risk.sh | 37 +++++++++++++++++++++ 4 files changed, 59 insertions(+), 3 deletions(-) diff --git a/docs/callers/pr-risk.md b/docs/callers/pr-risk.md index c5aabff..9c8e235 100644 --- a/docs/callers/pr-risk.md +++ b/docs/callers/pr-risk.md @@ -78,7 +78,7 @@ statuses: read | Input | Default | Notes | |---|---|---| | `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose: a floating default let a caller SHA-pin `uses:` and still load the grader from HEAD of main. | -| `fleet_logins` | `mattmillerai` | Logins whose PRs grade provenance `agent-supervised` alongside `agent-coded`. | +| `fleet_logins` | `mattmillerai` | Logins whose PRs grade provenance `agent-supervised` alongside `agent-coded`. Both are read for **human** authors only: an author GitHub types as a `Bot` is a runbook candidate regardless, so listing a bot here (or labelling its PR) buys it nothing — only a registry entry that asserts can promote it. | | `bot_logins` | `github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow` | Extra logins treated as bots. Needed only for **machine USER accounts** — a real GitHub App is recognized from GitHub's own actor type, no list entry required. A bot with no runbook entry still grades as human — identity alone buys no trust. | | `label_map` | `''` | Rename the five grader-owned labels as `tier=label` pairs. Tier keys are fixed; only the label text is yours. | | `wait_for_checks_minutes` | `10` | How long to wait for the rest of the check rollup to settle before labeling (clamped to 25 — what a 30-minute job can spend waiting). `0` labels immediately, expect R2 floors from still-pending checks. | diff --git a/scripts/pr-risk/grade-pr-risk.sh b/scripts/pr-risk/grade-pr-risk.sh index 7b5b57b..b383503 100755 --- a/scripts/pr-risk/grade-pr-risk.sh +++ b/scripts/pr-risk/grade-pr-risk.sh @@ -294,12 +294,31 @@ cat <<'JQ' # still lands on `human` below, so identity alone continues to buy no trust. # The first-time-contributor guard is UNCHANGED for humans: `$cls` is "human" (or "unknown") # for anyone the resolver does not classify as a bot, so a human's NONE still reads external. + # + # AND THE BOT TEST RUNS BEFORE THE LABEL/FLEET TEST, for the same reason `classify_login` + # answers "bot" before it answers "fleet": a bot is a bot no matter what else is true of it. + # Both halves of that branch are reachable for a bot and neither is evidence about a bot: + # * `agent-coded` is a LABEL — anyone with write access applies it, and what it claims is + # that a human supervised an agent. That is a claim about a human author, not about an App. + # * `$cls == "fleet"` is reachable for a bot at all only because `$is_bot` no longer comes + # from the login string alone: a Bot actor whose GraphQL login arrives unsuffixed + # (`cloud-code-bot`) and also appears in `fleet_logins` classifies "fleet" while + # `author_is_bot` says Bot. Leaving `fleet` first lets this use site contradict the + # resolver's own documented bot-beats-fleet ordering. + # What this does NOT change: a REGISTERED producer was never at risk here, because the shape + # assertion below resolves `$rbk != null` to `runbook` ahead of `$base_class` either way. + # What it fixes is the UNREGISTERED bot, which either half would classify `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 default map tiers both R1, so nothing moves + # there; the two classes exist so a CONSUMER map can tier them apart, and a consumer that + # trusts its own supervised agents at R0 would otherwise hand R0 to any bot PR someone + # labelled `agent-coded`. Trust in this axis is earned by the shape assertion, never by a label. | (if $pvst != "ok" or $lbst != "ok" then "unknown" elif ($r.is_fork // false) then "external" elif ($is_bot | not) and (($r.author_association // "") | IN("FIRST_TIME_CONTRIBUTOR","FIRST_TIMER","NONE")) then "external" - elif $agent_coded or $cls == "fleet" then "agent-supervised" elif $is_bot then "runbook-candidate" + elif $agent_coded or $cls == "fleet" then "agent-supervised" elif $cls == "human" then "human" else "unknown" end) as $base_class # The shape assertion: identity match AND every changed path inside permitted_paths AND the diff --git a/scripts/pr-risk/runbook-registry.v0.json b/scripts/pr-risk/runbook-registry.v0.json index 504446c..ac5284d 100644 --- a/scripts/pr-risk/runbook-registry.v0.json +++ b/scripts/pr-risk/runbook-registry.v0.json @@ -31,7 +31,7 @@ { "id": "dependabot", "why": "Dependabot — the highest-volume automated producer, and the one with the best-known diff shape", - "identity": { "logins": ["dependabot[bot]", "dependabot", "dependabot-preview[bot]"], + "identity": { "logins": ["dependabot[bot]", "dependabot"], "head_ref_patterns": ["dependabot/**"] }, "_permitted_paths_note": "MUST stay a superset of the `deps` class in risk-map.v0.json. Two manifest lists that must agree are one list too many: when this one is SHORT, a Ruby or PHP dependabot PR fails paths_ok, falls back to `human`, and records a shape_failure that says dependabot failed to look like dependabot. The overall tier does not move — the path floor pins those manifests R3 anyway — but the noise trains readers to skip the one field designed to catch real impersonation.", "permitted_paths": ["go.mod", "go.sum", "**/go.mod", "**/go.sum", "**/go.work*", diff --git a/scripts/pr-risk/tests/test_grade_pr_risk.sh b/scripts/pr-risk/tests/test_grade_pr_risk.sh index 6002d60..3c4c08e 100755 --- a/scripts/pr-risk/tests/test_grade_pr_risk.sh +++ b/scripts/pr-risk/tests/test_grade_pr_risk.sh @@ -462,6 +462,43 @@ eq "a --bot-logins machine user is not external" human "$(jq -r '.risk.axes.prov out="$(rec 28 'snapshot-machine' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE false | app_grade)" eq "and without --bot-logins it reads as a first-time human" external "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +# (6) A LABEL CANNOT STAND IN FOR THE SHAPE ASSERTION. `agent-coded` and `fleet_logins` both +# sat ABOVE the bot test, and both are reachable for a bot only now that the association half +# has stopped pinning every App `external` first — so they are pinned here, alongside that fix. +# A REGISTERED producer was never at risk (the shape assertion resolves to `runbook` ahead of +# the base class either way) — this pins that, then pins the case that WAS wrong: the +# UNREGISTERED bot, which either half classified `agent-supervised`, contradicting the promise +# that a bot with no asserting entry falls back to `human`. The default map tiers both R1, so +# no grade moves here; the classes exist so a CONSUMER map can tier them apart, and a consumer +# that trusts its supervised agents at R0 would otherwise hand R0 to any `agent-coded` bot PR. +out="$(rec 29 'cloud-code-bot[bot]' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE false \ + | jq -c '.labels = ["agent-coded"]' | app_grade)" +eq "a registered App is runbook with or without the label" runbook "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and still earns R0 from the shape assertion" R0 "$(jq -r '.risk.axes.provenance.tier' <<<"$out")" +out="$(rec 30 'unregistered-bot[bot]' 'chore: something' "$SNAP" ok SUCCESS bot/x NONE false \ + | jq -c '.labels = ["agent-coded"]' | app_grade)" +eq "an agent-coded unregistered bot is human, not agent-supervised" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and human is R1, exactly what agent-supervised was" R1 "$(jq -r '.risk.axes.provenance.tier' <<<"$out")" +# The same for `fleet_logins`. That collision exists only because `author_is_bot` no longer +# comes from the login string: a Bot actor's GraphQL login arrives UNSUFFIXED, so an operator +# who lists it in --fleet-logins makes `classify_login` say "fleet" while GitHub says Bot. The +# resolver's own rule is bot-beats-fleet; this use site must not contradict it. +out="$(rec 31 'cloud-code-bot' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE false \ + | jq -c '.author_is_bot = true' \ + | bash "$GRADER" --stdin --fleet-logins cloud-code-bot 2>/dev/null)" +eq "a Bot actor in fleet_logins is still read as a bot" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +# ...and with a registry entry that lists the unsuffixed form, the same record reaches `runbook`. +jq '.runbooks[0].identity.logins = ["cloud-code-bot[bot]","cloud-code-bot"]' \ + "$SANDBOX/app-runbooks.json" > "$SANDBOX/app-runbooks-bothforms.json" +out="$(rec 32 'cloud-code-bot' 'chore: refresh skills snapshot' "$SNAP" ok SUCCESS bot/refresh NONE false \ + | jq -c '.author_is_bot = true' \ + | bash "$GRADER" --stdin --runbooks "$SANDBOX/app-runbooks-bothforms.json" --fleet-logins cloud-code-bot 2>/dev/null)" +eq "and the registry, not the fleet list, is what promotes it" runbook "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +# A HUMAN is untouched by the reorder — the label still classifies a human author. +out="$(rec 33 'a-teammate' 'feat: something' "$SNAP" ok SUCCESS feature MEMBER false \ + | jq -c '.labels = ["agent-coded"]' | app_grade)" +eq "an agent-coded human is still agent-supervised" agent-supervised "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" + echo "— phase 22: on the LIVE path the login alone cannot tell you it is a bot —" # The case above is the corpus/REST record shape. The live path does not see it: GraphQL reports # a Bot actor login WITHOUT the `[bot]` suffix, so `cloud-code-bot[bot]` arrives as From adfc838e7233f1240c5f9cc179e9c6fe2ba36f1f Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 5 Aug 2026 16:45:45 -0700 Subject: [PATCH 3/3] fix(pr-risk): restore an App's `[bot]` suffix in the grader, not in every registry (BE-6715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/pr-risk.yml | 5 +- docs/callers/pr-risk.md | 2 +- scripts/pr-risk/README.md | 14 +++--- scripts/pr-risk/grade-pr-risk.sh | 33 ++++++++++++- scripts/pr-risk/runbook-registry.v0.json | 11 +++-- scripts/pr-risk/tests/test_grade_pr_risk.sh | 52 ++++++++++++++++++--- 6 files changed, 97 insertions(+), 20 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 6915384..b0cf113 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -261,7 +261,10 @@ on: and any author GitHub itself types as a `Bot` (comma-separated). A real GitHub App needs no entry here; a machine USER account does. A bot with no runbook registry entry grades as human — identity alone - never buys trust. + never buys trust. LOAD-BEARING, not a hint: 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 really is a + machine account. List only accounts you control; prune retired ones. type: string required: false default: github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow diff --git a/docs/callers/pr-risk.md b/docs/callers/pr-risk.md index 9c8e235..6ac9046 100644 --- a/docs/callers/pr-risk.md +++ b/docs/callers/pr-risk.md @@ -79,7 +79,7 @@ statuses: read |---|---|---| | `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose: a floating default let a caller SHA-pin `uses:` and still load the grader from HEAD of main. | | `fleet_logins` | `mattmillerai` | Logins whose PRs grade provenance `agent-supervised` alongside `agent-coded`. Both are read for **human** authors only: an author GitHub types as a `Bot` is a runbook candidate regardless, so listing a bot here (or labelling its PR) buys it nothing — only a registry entry that asserts can promote it. | -| `bot_logins` | `github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow` | Extra logins treated as bots. Needed only for **machine USER accounts** — a real GitHub App is recognized from GitHub's own actor type, no list entry required. A bot with no runbook entry still grades as human — identity alone buys no trust. | +| `bot_logins` | `github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow` | Extra logins treated as bots. Needed only for **machine USER accounts** — a real GitHub App is recognized from GitHub's own actor type, no list entry required. A bot with no runbook entry still grades as human — identity alone buys no trust. **This list is load-bearing, not a hint:** a listed login skips the first-time-contributor test, so it moves a non-fork `NONE`/`FIRST_TIME_CONTRIBUTOR` PR from `external` (R3) to `human` (R1). Nothing validates that a listed login is really a machine account, so add one only for an account you control, and remove it when it is retired. | | `label_map` | `''` | Rename the five grader-owned labels as `tier=label` pairs. Tier keys are fixed; only the label text is yours. | | `wait_for_checks_minutes` | `10` | How long to wait for the rest of the check rollup to settle before labeling (clamped to 25 — what a 30-minute job can spend waiting). `0` labels immediately, expect R2 floors from still-pending checks. | | `repo_map_path` | `.github/risk.json` | Consumer risk-map override, read from the PR **base ref**. | diff --git a/scripts/pr-risk/README.md b/scripts/pr-risk/README.md index 633d694..28a2b0e 100644 --- a/scripts/pr-risk/README.md +++ b/scripts/pr-risk/README.md @@ -184,12 +184,14 @@ them by committing: - `.github/risk.json` — the repo's own path→tier map (same schema as [`risk-map.v0.json`](risk-map.v0.json)) - `.github/risk-runbooks.json` — the repo's own producer registry (same schema - as [`runbook-registry.v0.json`](runbook-registry.v0.json)). **List BOTH login - forms for a GitHub App** — `"my-bot[bot]"` *and* `"my-bot"` — exactly as the - shipped `dependabot` entry does. REST and the web UI show the suffixed form, - but the grader reads the PR over GraphQL, which reports a Bot login - **unsuffixed**; an entry that lists only `my-bot[bot]` never asserts on a live - grade, and the producer falls back to `human`. + as [`runbook-registry.v0.json`](runbook-registry.v0.json)). **Name a GitHub App + by its suffixed login** — `"my-bot[bot]"`, the form REST and the web UI show + you. The grader reads the PR over GraphQL, which reports a Bot login + **unsuffixed**, and restores the suffix before matching — one way only, and + only for an author GitHub's own actor type calls a `Bot`. A bare slug + (`"my-bot"`) still matches literally, because a machine **user** account has no + suffix and needs it; but a bare slug is equally matchable by a same-named + *human*, who would then inherit that runbook — so never name an App with one. Both are read from the PR's **base ref**, so a PR cannot edit the rules that judge it (editing them — or the grader — at all is R3 by the map's own first diff --git a/scripts/pr-risk/grade-pr-risk.sh b/scripts/pr-risk/grade-pr-risk.sh index b383503..951c22f 100755 --- a/scripts/pr-risk/grade-pr-risk.sh +++ b/scripts/pr-risk/grade-pr-risk.sh @@ -265,7 +265,21 @@ cat <<'JQ' # works for machine USER accounts, which are typed `User` and carry no suffix at all. # Deliberately kept OUT of classify_login: that jq is shared verbatim with agent-work's # grade-collect.sh, and a field only this collector emits would fork the shared definition. - | (($r.author_is_bot // false) or $cls == "bot") as $is_bot + # + # `== true`, NOT jq truthiness. This is the one field that can switch the `external` guard + # off, and in jq the string "false", 0 and "" are all truthy — so a foreign collector, or a + # corpus row someone hand-edited, could turn a stringly-typed "false" into a confident + # not-an-outsider. Only the literal boolean counts; anything else falls back to the login. + # + # NO STATUS TWIN HERE, deliberately, and it is the only provenance input without one. The + # twins exist where the un-collected default would ANSWER — reading an absent `is_fork` as + # "not a fork" retires `external => R3`. This default runs the other way: absent means "not + # known to be a bot", which sends the author BACK through the association test and grades it + # the stricter way, exactly as a pre-schema-4 record graded before. Nothing is retired, so + # there is nothing to refuse to answer. A schema-3 corpus row therefore still grades an App + # PR `external` where a live schema-4 grade now says `human`: that is a versioned behaviour + # change, which is what `schema_version` and `map_version` are on the record to express. + | ((($r.author_is_bot // false) == true) or $cls == "bot") as $is_bot | (($r.labels // []) | index("agent-coded") != null) as $agent_coded # The label list has a STATUS TWIN for the same reason the file list does: `agent-coded` is # read from it, and a TRUNCATED label list answers "is this agent-coded?" with a confident no @@ -331,8 +345,23 @@ cat <<'JQ' # login test is therefore REQUIRED, and head_ref_patterns are an ADDITIONAL condition where # the producer declares them. The parentheses are load-bearing — do not let this collapse # back into a bare or/and chain. + # + # AND THE APP'S SUFFIXED LOGIN IS RESTORED BEFORE MATCHING, in ONE direction only. GraphQL + # hands us `cloud-code-bot` where REST and the web UI say `cloud-code-bot[bot]`, so without + # this an entry listing the form a human can actually SEE never asserts on a live grade, and + # the miss is silent — `shape_failures` only records entries whose identity DID match. The + # direction matters and is the whole safety argument: the suffixed form is SYNTHESIZED, never + # stripped, and only when `author_is_bot` — GitHub's own actor type, not the caller's + # `bot_logins` — says the author is a Bot. So an App reaches an `"x[bot]"` entry, and a USER + # account named `x` still cannot: it can only ever present `x`, and nothing here turns + # `"x[bot]"` into `"x"`. That asymmetry is why this is safe where normalizing both sides is + # not. Consequence for registries: an App should be listed by its SUFFIXED login alone. A + # bare slug still matches literally — machine USER accounts have no suffix and need it — but + # a bare slug is also matchable by a same-named human, so never use one to name an App. + | (if $author != null and (($r.author_is_bot // false) == true) and ($author | endswith("[bot]") | not) + then [$author, ($author + "[bot]")] else [$author] end) as $author_forms | ([$RB.runbooks[]? | . as $bk - | select((($bk.identity.logins // []) | any(. as $l | ($author // "") | ascii_downcase == ($l | ascii_downcase))) + | select((($bk.identity.logins // []) | any(. as $l | $author_forms | any(. // "" | ascii_downcase == ($l | ascii_downcase)))) and ((($bk.identity.head_ref_patterns // []) | length) == 0 or (($r.head_ref // "") | matches_any($bk.identity.head_ref_patterns // [])))) | {id: $bk.id, lane: $bk.lane, daily_cap: $bk.daily_cap, diff --git a/scripts/pr-risk/runbook-registry.v0.json b/scripts/pr-risk/runbook-registry.v0.json index ac5284d..ac21b2d 100644 --- a/scripts/pr-risk/runbook-registry.v0.json +++ b/scripts/pr-risk/runbook-registry.v0.json @@ -13,10 +13,12 @@ "IDENTITY is the server-attributed PR author login (and, where the producer is a workflow,", "the head-ref shape that workflow pushes) — never the forgeable commit author string.", "", - "LIST BOTH LOGIN FORMS FOR A GITHUB APP, as the dependabot entry below does: \"x[bot]\" AND", - "\"x\". REST and the web UI show the suffixed form, but the grader reads the PR over GraphQL,", - "which reports a Bot login UNSUFFIXED — so an entry carrying only \"x[bot]\" never asserts on", - "a live grade and its producer silently falls back to `human`.", + "NAME A GITHUB APP BY ITS SUFFIXED LOGIN — \"x[bot]\", the form REST and the web UI show you.", + "The grader reads the PR over GraphQL, which reports a Bot login UNSUFFIXED, and it restores", + "the suffix before matching — but only ONE WAY, and only for an author GitHub's own actor", + "type calls a Bot. A bare slug \"x\" still matches literally, because a machine USER account", + "has no suffix and needs it; but a bare slug is equally matchable by a same-named HUMAN, who", + "would then inherit that runbook. So never name an App with one.", "", "This default registers only ecosystem-universal producers. A consumer repo registers its", "own producers (release bots, spec-sync pushes, caller bumpers) by committing", @@ -33,6 +35,7 @@ "why": "Dependabot — the highest-volume automated producer, and the one with the best-known diff shape", "identity": { "logins": ["dependabot[bot]", "dependabot"], "head_ref_patterns": ["dependabot/**"] }, + "_logins_note": "The bare `dependabot` is the ONE sanctioned exception to \"never name an App with a bare slug\", and it is here for the OFFLINE corpus, not the live path: pre-schema-4 rows carry the unsuffixed GraphQL login with no `author_is_bot`, so the grader cannot restore the suffix for them and only a literal entry matches. It costs little — the head_ref_patterns above additionally require a `dependabot/**` branch, so a same-named human would have to push one. Do NOT copy the pattern into a consumer registry.", "_permitted_paths_note": "MUST stay a superset of the `deps` class in risk-map.v0.json. Two manifest lists that must agree are one list too many: when this one is SHORT, a Ruby or PHP dependabot PR fails paths_ok, falls back to `human`, and records a shape_failure that says dependabot failed to look like dependabot. The overall tier does not move — the path floor pins those manifests R3 anyway — but the noise trains readers to skip the one field designed to catch real impersonation.", "permitted_paths": ["go.mod", "go.sum", "**/go.mod", "**/go.sum", "**/go.work*", "**/package.json", "**/package-lock.json", "**/npm-shrinkwrap.json", diff --git a/scripts/pr-risk/tests/test_grade_pr_risk.sh b/scripts/pr-risk/tests/test_grade_pr_risk.sh index 3c4c08e..cf4bf73 100755 --- a/scripts/pr-risk/tests/test_grade_pr_risk.sh +++ b/scripts/pr-risk/tests/test_grade_pr_risk.sh @@ -499,6 +499,19 @@ out="$(rec 33 'a-teammate' 'feat: something' "$SNAP" ok SUCCESS feature MEMBER f | jq -c '.labels = ["agent-coded"]' | app_grade)" eq "an agent-coded human is still agent-supervised" agent-supervised "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +# (7) `author_is_bot` is tested `== true`, NOT for jq truthiness. It is the one field that can +# switch the `external` guard off, and in jq the STRING "false" — what a foreign collector +# writing JSON by hand emits — is truthy. Read loosely, that grades a first-time outsider +# `human` R1 on a field nobody set to true. +for junk in '"false"' '0' '""' 'null'; do + out="$(rec 34 'drive-by-human' 'feat: my first patch' "$SNAP" ok SUCCESS feature NONE false \ + | jq -c ".author_is_bot = $junk" | app_grade)" + eq "author_is_bot=$junk is not a bot — still external" external "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +done +out="$(rec 35 'drive-by-human' 'feat: my first patch' "$SNAP" ok SUCCESS feature NONE false \ + | jq -c '.author_is_bot = true' | app_grade)" +eq "and only the literal boolean flips it" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" + echo "— phase 22: on the LIVE path the login alone cannot tell you it is a bot —" # The case above is the corpus/REST record shape. The live path does not see it: GraphQL reports # a Bot actor login WITHOUT the `[bot]` suffix, so `cloud-code-bot[bot]` arrives as @@ -513,6 +526,10 @@ jq '.data.repository.pullRequest.author = {"login":"cloud-code-bot","__typename" | .data.repository.pullRequest.changedFiles = 1 | .data.repository.pullRequest.title = "data: refresh bundled skills snapshot (auto)" | .data.repository.pullRequest.headRefName = "bot/refresh-skills" + # ...and the ROLLUP STATE with it. Without --self-run-id the grader reads `.state` directly, + # and phase 9 left it PENDING — which floored reversibility R2 and meant the SUCCESS CheckRun + # below was never actually consulted, so the fixture implied coverage it did not have. + | .data.repository.pullRequest.commits.nodes[0].commit.statusCheckRollup.state = "SUCCESS" | .data.repository.pullRequest.commits.nodes[0].commit.statusCheckRollup.contexts.nodes = [{"__typename":"CheckRun","name":"unit tests","status":"COMPLETED","conclusion":"SUCCESS", "checkSuite":{"workflowRun":{"databaseId":1000,"workflow":{"name":"CI"}}}}]' \ @@ -541,15 +558,38 @@ out="$(bot_pr)" eq "the live record carries GitHub's own actor type" true "$(jq -r '.author_is_bot' <<<"$out")" eq "and the login really does arrive unsuffixed" cloud-code-bot "$(jq -r '.author' <<<"$out")" eq "an App PR is no longer external on the live path" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" -# ...and with the consumer's registry entry it reaches `runbook` — the ticket's end state. +# ...and with the consumer's registry entry it reaches `runbook` — the ticket's end state. The +# entry names the App the way a human can SEE it (`cloud-code-bot[bot]`, what REST and the web +# UI show); the grader restores the suffix GraphQL dropped, so no consumer has to know this. +out="$(bot_pr --runbooks "$SANDBOX/app-runbooks.json" --bot-logins '')" +eq "an entry listing only the SUFFIXED form asserts against an unsuffixed Bot login" \ + runbook "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "and the tier now responds to the diff" R0 "$(jq -r '.risk.axes.provenance.tier' <<<"$out")" +# The rollup really is read on this path: green, but no test file in the diff, so reversibility +# proposes R1 and that — not the author's account type — is what decides the grade. +eq "the green rollup is actually consulted" R1 "$(jq -r '.risk.axes.reversibility.tier' <<<"$out")" +eq "and the overall grade is R1, decided by the diff" R1 "$(jq -r '.risk.tier' <<<"$out")" +# THE SYNTHESIS IS ONE-WAY AND GATED ON GITHUB'S ACTOR TYPE, which is the whole safety argument: +# the suffix is only ever ADDED, and only for an author GitHub types `Bot`. A USER account that +# happens to be named `cloud-code-bot` presents `cloud-code-bot` and nothing turns the entry's +# `cloud-code-bot[bot]` back into it — so it cannot inherit the App's runbook. +jq '.data.repository.pullRequest.author = {"login":"cloud-code-bot","__typename":"User"} + | .data.repository.pullRequest.authorAssociation = "MEMBER"' \ + "$SANDBOX/botfixture.json" > "$SANDBOX/bfuser.json" && cp "$SANDBOX/bfuser.json" "$SANDBOX/botfixture.json" out="$(bot_pr --runbooks "$SANDBOX/app-runbooks.json" --bot-logins '')" -eq "an unsuffixed Bot login still fails an entry that lists only the suffixed form" \ - human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +eq "a same-named USER cannot inherit the App's runbook" human "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +# A BARE-SLUG entry still matches literally — that is what a machine USER account needs — and +# that is exactly why a bare slug must never be used to name an App. +jq '.runbooks[0].identity.logins = ["cloud-code-bot"]' \ + "$SANDBOX/app-runbooks.json" > "$SANDBOX/app-runbooks-bare.json" +out="$(bot_pr --runbooks "$SANDBOX/app-runbooks-bare.json" --bot-logins '')" +eq "a bare-slug entry matches a same-named USER too" runbook "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" +# Restore the Bot author for the rest of the phase. +jq '.data.repository.pullRequest.author = {"login":"cloud-code-bot","__typename":"Bot"} + | .data.repository.pullRequest.authorAssociation = "NONE"' \ + "$SANDBOX/botfixture.json" > "$SANDBOX/bfbot.json" && cp "$SANDBOX/bfbot.json" "$SANDBOX/botfixture.json" jq '.runbooks[0].identity.logins = ["cloud-code-bot[bot]","cloud-code-bot"]' \ "$SANDBOX/app-runbooks.json" > "$SANDBOX/app-runbooks-both.json" -out="$(bot_pr --runbooks "$SANDBOX/app-runbooks-both.json")" -eq "an entry listing BOTH login forms asserts" runbook "$(jq -r '.risk.axes.provenance.provenance' <<<"$out")" -eq "and the tier now responds to the diff" R0 "$(jq -r '.risk.axes.provenance.tier' <<<"$out")" # The fork half is unconditional on the live path too — the API fork flag, not the actor. jq '.data.repository.pullRequest.isCrossRepository = true' \ "$SANDBOX/botfixture.json" > "$SANDBOX/bf2.json" && cp "$SANDBOX/bf2.json" "$SANDBOX/botfixture.json"