From 4f40cfabf2a3b276a3e35c89fa5b8a4b6e1ad421 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 17:38:32 +0000 Subject: [PATCH] fix(labels): distinguish an unreadable check rollup from no checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `gh pr view` failed, the fallback left the `statusCheckRollup` key absent, and `(.statusCheckRollup // [])` collapsed that into the same NONE as a PR with genuinely no checks. NONE blocks nothing, so an API hiccup presented the PR as mergeable by a human — an unknown certified as green, the shape #87 exists to stop, in the one place it never looked. checks_state now returns UNREADABLE for an absent key versus NONE for a present-but-empty array, and the sweep leaves an UNREADABLE PR alone rather than recomputing on facts it did not read. Not a blocker on purpose: blocking would flap the board on one bad call. Fixtures 64 -> 66. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 22 ++++++++++++++++++++-- CHANGELOG.md | 18 +++++++++++++++++- test/labels-reconcile.sh | 7 +++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index d6c0c62..aa8f3ca 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -56,7 +56,14 @@ run() { # every mutation goes through here — DRY_RUN=1 logs instead of doing requested() { grep -qxF "$1" <<<"$REQUESTED"; } -checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE +checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE | UNREADABLE + # UNREADABLE is the absence of the key itself, which is what a failed fetch + # leaves behind — distinct from a present-but-empty rollup, which honestly + # means this PR has no checks. Collapsing the two let an API hiccup present + # as "nothing is failing", i.e. as mergeable-by-a-human: the same + # unknown-certified-as-green shape as the bug this machine exists to stop. + # The caller skips the PR entirely rather than labelling on facts it did not + # read; blocking on it instead would flap the whole board on one bad call. # The rollup mixes two node types with two different closed enums: CheckRun # carries `conclusion` (CheckConclusionState), StatusContext carries `state` # (StatusState). Rather than list the outcomes that block — the version that @@ -70,6 +77,8 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE # not in consequence: a false FAILURE parks the PR on the agent, who looks; # a false SUCCESS invites a human to merge a tree that will not merge. jq -r ' + if (has("statusCheckRollup") | not) then "UNREADABLE" else + # NEUTRAL and SKIPPED satisfy branch protection — a skipped required check # is not a failed one, and path-filtered jobs skip constantly here. ["SUCCESS", "NEUTRAL", "SKIPPED"] as $passing @@ -121,7 +130,9 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE | if ($latest | length) == 0 then "NONE" elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE" elif (($latest - $passing) | length) > 0 then "PENDING" - else "SUCCESS" end' + else "SUCCESS" end + + end' } bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK @@ -414,6 +425,13 @@ main() { GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')" MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")" CHECKS="$(checks_state <<<"$GH_VIEW")" + # Read failed: leave this PR exactly as it is. Recomputing on facts we + # did not read is how an API hiccup turns into a false "merge me" — + # and the next tick is 15 minutes away, not 15 hours. + if [ "$CHECKS" = UNREADABLE ]; then + log "#$n: could not read mergeability/checks — left alone this pass" + exit 0 + fi reconcile_pr "$n" ) || log "#$n: reconcile failed — continuing with the remaining PRs" done diff --git a/CHANGELOG.md b/CHANGELOG.md index f56dd58..a0c3bc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,22 @@ on the way to cutting its first release, and this file starts there. ### Fixed +- **An unreadable check rollup no longer reads as "nothing is failing"** (#90) + — when `gh pr view` failed, the fallback left the `statusCheckRollup` key + absent entirely, and `(.statusCheckRollup // [])` collapsed that into the + same `NONE` as a PR that genuinely has no checks. `NONE` blocks nothing, so + a transient API failure presented the PR as mergeable by a human: an unknown + certified as green, which is the exact shape of the bug #87 was opened to + stop, surviving in the one place that fix never looked. + + `checks_state` now separates the two — `UNREADABLE` for an absent key (a + read that failed), `NONE` for a present-but-empty array (a PR that really + has no checks) — and the sweep leaves an `UNREADABLE` PR exactly as it + found it rather than recomputing labels from facts it did not read. + Deliberately *not* a blocker: blocking would flap the whole board on one bad + API call, and the next tick is fifteen minutes away. Caught by the author + after opening the PR, not by review. + - **CI runs `test/labels-reconcile.sh`, which it had never run** (#90) — the file arrived with #87 and `ci.yml` was not extended to call it, so the label state machine that gates every PR in this repo went covered only by whoever @@ -209,7 +225,7 @@ on the way to cutting its first release, and this file starts there. The reconciler strips `state:needs-rebase` on sight via a `RETIRED` list, so the retirement heals the existing board instead of stranding a label that - nothing recomputes. Fixtures 51 → 64. + nothing recomputes. Fixtures 51 → 66. - **BREAKING: `--class human|server` is now `--root-door closed|open`** (#77) — the trait was named for who *lives on* a box; what it decides is one thing, diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 863e216..c756fb1 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -258,6 +258,13 @@ ctx_() { jq -n --arg n "$1" --arg s "$2" --arg t "${3:-2026-07-20T15:00:00Z}" \ '{__typename:"StatusContext", context:$n, state:$s, createdAt:$t}'; } expect "no checks at all is NONE" NONE "$(rollup '[]' | checks_state)" +# A failed fetch leaves no rollup KEY; a PR with no checks leaves an empty +# ARRAY. Collapsing the two let an API hiccup read as "nothing is failing" — +# the same unknown-certified-as-green shape as #136, in the one place that +# fix did not look. The caller skips an UNREADABLE PR rather than relabelling. +expect "a failed read is UNREADABLE, not NONE" UNREADABLE "$(echo '{}' | checks_state)" +expect "...and a real empty rollup is still NONE" NONE \ + "$(echo '{"mergeable":"MERGEABLE","statusCheckRollup":[]}' | checks_state)" expect "all green is SUCCESS" SUCCESS \ "$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)" expect "a queued run is PENDING" PENDING \