Skip to content

Commit beeeca0

Browse files
johanzanderclaude
andcommitted
fix: two bugs pr-state.sh only showed when run against a real fleet
The fixture tests could not see either of these. Both turned up on the first live run. 1. GraphQL node limit. `--json commits` expands each commit's authors connection, so gh's cost estimate is limit x commits x authors. At --limit 100 that is 1,000,000 possible nodes and the query is rejected outright, so the command produced no output at all. --limit 30 keeps the worst case at 300,000. There is no cheaper field for "when did HEAD last move": `gh pr list --json` has no last-commit date, and a review's own commit SHA is REST-only. The cap is announced when hit rather than silently truncating, per sweep-prs. 2. Lazy `mergeable`, which is the dangerous one. The first query on a cold PR returns UNKNOWN *and* only then triggers the computation, so treating UNKNOWN as "not conflicted" hides precisely the stale PRs this script exists to surface. Measured: a first fleet run classified #167 and #619 with no conflict flag; once earlier queries had warmed them, the identical command returned `needs-refresh` for both. They were CONFLICTING the whole time. sweep-prs documents this trap and retries for the same reason; this reintroduced it. Now: re-ask while anything is UNKNOWN, and if it survives, render it as "(+mergeability UNKNOWN — re-run)" rather than letting it fall through to the clean branch. Verified by mutation: forcing $mergeUnknown to false reddens test_unknown_mergeability_is_never_reported_as_clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent cd0c5a5 commit beeeca0

2 files changed

Lines changed: 72 additions & 4 deletions

File tree

backend/tests/test_pr_state.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,26 @@ def test_a_conflict_outranks_ci_because_a_conflicted_pr_has_no_run(run) -> None:
185185
assert "[sweep]" in out
186186

187187

188+
def test_unknown_mergeability_is_never_reported_as_clean(run) -> None:
189+
"""`mergeable` is computed LAZILY: the first query on a cold PR returns
190+
UNKNOWN and only then triggers the computation.
191+
192+
This was measured, not theorised. A first live fleet run classified #167 and
193+
#619 with no conflict flag; once earlier queries had warmed them, the
194+
identical command returned `needs-refresh` for both. They were CONFLICTING
195+
throughout. Treating UNKNOWN as "not conflicted" therefore hides exactly the
196+
stale PRs this script exists to surface — the same trap `sweep-prs`
197+
documents and retries for.
198+
199+
The script retries while anything is UNKNOWN. This pins the fallback: if it
200+
still is, say so rather than falling through to the clean branch.
201+
"""
202+
out = run([_pr(9, mergeable="UNKNOWN", merge_state="UNKNOWN")])
203+
204+
assert "UNKNOWN" in _rows(out)
205+
assert "re-run" in _rows(out)
206+
207+
188208
def test_red_ci_belongs_to_the_executor(run) -> None:
189209
out = run(
190210
[

scripts/pr-state.sh

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,54 @@ pr_filter="${1:-}"
4545
# decides whose turn it is. Compute it from reviews vs commits instead.
4646
fields='number,title,isDraft,mergeable,mergeStateStatus,headRefName,reviews,commits,statusCheckRollup,updatedAt'
4747

48-
if [ -n "$pr_filter" ]; then
49-
raw=$(gh pr view "$pr_filter" --json "$fields" | jq -c '[.]')
50-
else
51-
raw=$(gh pr list --state open --limit 100 --json "$fields")
48+
# `commits` is what bounds this, and the bound is GitHub's, not a preference.
49+
# `--json commits` expands each commit's authors connection, so gh's cost
50+
# estimate is limit x commits x authors: at --limit 100 that is 100 x 100 x 100 =
51+
# 1,000,000 possible nodes and the query is REJECTED outright --
52+
#
53+
# GraphQL: By the time this query traverses to the authors connection, it is
54+
# requesting up to 1,000,000 possible nodes which exceeds the maximum limit of
55+
# 500,000.
56+
#
57+
# Found by running this against a real fleet; the fixture tests could not see it.
58+
# 30 keeps the worst case at 300,000, inside the ceiling. There is no cheaper
59+
# field for "when did HEAD last move" -- `gh pr list --json` offers no
60+
# last-commit date, and a review's own commit SHA is REST-only (`gh api`, which
61+
# is on CLAUDE.md's ask list).
62+
PR_LIMIT=30
63+
64+
fetch() {
65+
if [ -n "$pr_filter" ]; then
66+
gh pr view "$pr_filter" --json "$fields" | jq -c '[.]'
67+
else
68+
gh pr list --state open --limit "$PR_LIMIT" --json "$fields"
69+
fi
70+
}
71+
72+
# `mergeable` is computed LAZILY, and this is the single most dangerous thing
73+
# about reading it. The first query on a cold PR returns UNKNOWN *and* triggers
74+
# the computation, so a one-shot pass reports UNKNOWN for precisely the stale
75+
# PRs worth finding -- and if UNKNOWN is treated as "not conflicted", every
76+
# conflicted PR in a cold fleet reads as fine.
77+
#
78+
# Measured here while building this: a first fleet run classified #167 and #619
79+
# as `needs-fix`/CI-red with no conflict flag; after the queries above had
80+
# warmed them, the identical command returned `needs-refresh` for both. Both
81+
# were CONFLICTING the whole time. `sweep-prs` documents the same trap and
82+
# retries for the same reason.
83+
#
84+
# So: ask again while anything is UNKNOWN, and if it still is, SAY SO rather
85+
# than letting it fall through to the not-conflicted branch.
86+
for _ in 1 2 3; do
87+
raw=$(fetch)
88+
[ "$(echo "$raw" | jq '[.[] | select(.mergeable == "UNKNOWN")] | length')" -eq 0 ] && break
89+
sleep 3
90+
done
91+
92+
if [ -z "$pr_filter" ] && [ "$(echo "$raw" | jq 'length')" -eq "$PR_LIMIT" ]; then
93+
# No silent caps (sweep-prs): a truncated fleet must not read as a clean one.
94+
echo "WARNING: hit the ${PR_LIMIT}-PR ceiling — older open PRs were NOT" >&2
95+
echo "classified. Pass a PR number to inspect one directly." >&2
5296
fi
5397

5498
# A quoted heredoc, not a single-quoted argument: the program below contains
@@ -69,6 +113,9 @@ read -r -d '' classify <<'JQ' || true
69113
| (verdict) as $v
70114
| (pushed) as $push
71115
| (.mergeable == "CONFLICTING" or .mergeStateStatus == "DIRTY") as $conflicted
116+
# UNKNOWN survived the retries above. It is NOT "not conflicted" -- it is "we
117+
# never found out", and it must never render as a clean PR.
118+
| (.mergeable == "UNKNOWN") as $mergeUnknown
72119
| ($v != null and $v.submittedAt > $push) as $unconsumed
73120
74121
# Order matters, and it is ordered by WHAT THE DIFF STILL OWES rather than by
@@ -103,6 +150,7 @@ read -r -d '' classify <<'JQ' || true
103150
104151
| "#\($p.number) \(if $p.isDraft then "draft" else "ready" end) \($state) [\($owner)]"
105152
+ (if $conflicted and $state != "needs-refresh" then " (+conflicted)" else "" end)
153+
+ (if $mergeUnknown then " (+mergeability UNKNOWN — re-run)" else "" end)
106154
+ "\n \($p.title[0:72])\n \($why)\n"
107155
JQ
108156

0 commit comments

Comments
 (0)