forked from heavy-duty/rig
fix(labels): distinguish an unreadable check rollup from no checks
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 <noreply@anthropic.com>
This commit is contained in:
parent
4ea3579c2e
commit
4f40cfabf2
3 changed files with 44 additions and 3 deletions
22
.github/scripts/labels-reconcile.sh
vendored
22
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -56,7 +56,14 @@ run() { # every mutation goes through here — DRY_RUN=1 logs instead of doing
|
||||||
|
|
||||||
requested() { grep -qxF "$1" <<<"$REQUESTED"; }
|
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
|
# The rollup mixes two node types with two different closed enums: CheckRun
|
||||||
# carries `conclusion` (CheckConclusionState), StatusContext carries `state`
|
# carries `conclusion` (CheckConclusionState), StatusContext carries `state`
|
||||||
# (StatusState). Rather than list the outcomes that block — the version that
|
# (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;
|
# 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.
|
# a false SUCCESS invites a human to merge a tree that will not merge.
|
||||||
jq -r '
|
jq -r '
|
||||||
|
if (has("statusCheckRollup") | not) then "UNREADABLE" else
|
||||||
|
|
||||||
# NEUTRAL and SKIPPED satisfy branch protection — a skipped required check
|
# NEUTRAL and SKIPPED satisfy branch protection — a skipped required check
|
||||||
# is not a failed one, and path-filtered jobs skip constantly here.
|
# is not a failed one, and path-filtered jobs skip constantly here.
|
||||||
["SUCCESS", "NEUTRAL", "SKIPPED"] as $passing
|
["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"
|
| if ($latest | length) == 0 then "NONE"
|
||||||
elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE"
|
elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE"
|
||||||
elif (($latest - $passing) | length) > 0 then "PENDING"
|
elif (($latest - $passing) | length) > 0 then "PENDING"
|
||||||
else "SUCCESS" end'
|
else "SUCCESS" end
|
||||||
|
|
||||||
|
end'
|
||||||
}
|
}
|
||||||
|
|
||||||
bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
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 '{}')"
|
GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')"
|
||||||
MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")"
|
MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")"
|
||||||
CHECKS="$(checks_state <<<"$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"
|
reconcile_pr "$n"
|
||||||
) || log "#$n: reconcile failed — continuing with the remaining PRs"
|
) || log "#$n: reconcile failed — continuing with the remaining PRs"
|
||||||
done
|
done
|
||||||
|
|
|
||||||
18
CHANGELOG.md
18
CHANGELOG.md
|
|
@ -8,6 +8,22 @@ on the way to cutting its first release, and this file starts there.
|
||||||
|
|
||||||
### Fixed
|
### 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
|
- **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
|
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
|
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 reconciler strips `state:needs-rebase` on sight via a `RETIRED` list, so
|
||||||
the retirement heals the existing board instead of stranding a label that
|
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) —
|
- **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,
|
the trait was named for who *lives on* a box; what it decides is one thing,
|
||||||
|
|
|
||||||
|
|
@ -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}'; }
|
'{__typename:"StatusContext", context:$n, state:$s, createdAt:$t}'; }
|
||||||
|
|
||||||
expect "no checks at all is NONE" NONE "$(rollup '[]' | checks_state)"
|
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 \
|
expect "all green is SUCCESS" SUCCESS \
|
||||||
"$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)"
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)"
|
||||||
expect "a queued run is PENDING" PENDING \
|
expect "a queued run is PENDING" PENDING \
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue