fix(ci): rescue the two readiness freezes the sweep skipped by design - #5955
Conversation
github-actions
Bot
added
the
readiness: checking
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence gathered. Composing the review. First-Principles-Verdict: CONCERNS A What this change shipsIntent: give frozen PR-readiness verdicts an automatic recovery path where none existed — a FIX.
Watch
[FIRST-PRINCIPLES-REVIEWED] 958b28f |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Both new arms inherit the existing rescue shapes (age-gated like mode 1, evidence-gated and self-terminating like mode 2), so no new loop or trigger class is introduced. [DESIGN-REVIEWED] 958b28f |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
The self-heal sweep rescues a `pending` verdict on age and a `failure` verdict on later check evidence. Two freeze modes fell outside both tests and had no other recovery path, because the sweep is the only thing that recomputes an unchanged commit once its `workflow_run` events are spent. An UNPUBLISHED verdict was the live one. `pr-readiness.yml` deliberately does not retry its status POST -- a retry can overwrite a concurrent run's newer verdict -- and instead instructs a human to re-run the workflow. When that POST fails the SHA carries no readiness status at all, so there is no `pending` to age out, and the sweep skipped the PR on the assumption that its own run was still coming. The one case the publisher delegates to a re-run was the one case nothing re-ran. PR #2783 proves it: both readiness runs for f014465 failed on `gh: HTTP 503` from the POST on 2026-08-17, and eight days later the PR was still MERGEABLE with no readiness verdict of any kind. Today's #5282 hit the same POST with a 502 and survived only because an unrelated later event happened to republish. Rescued now on the PR's own last-activity age, which a push bumps, so the grace window restarts on every new revision and a genuinely queueing run is left alone. A STALE GREEN was the unsafe-direction gap. The re-run mechanism behind mode 2 is direction-blind: a job re-run that flips a lane red after a green verdict emits no `workflow_run: completed` either, leaving the required aggregate green over a now-red revision -- which PERMITS a merge, where a stale red only blocks one. The `*)` arm skipped it as "success needs no rescue". `success` and `error` now share mode 2's evidence test, narrowed to failure-class conclusions: housekeeping check-runs (`Strip stale workflow-change override`, `Fork workflow-change guard`) legitimately complete days after a verdict, so an unnarrowed "any check completed later" test would re-fire most green PRs every sweep while proving nothing. No live poisoned green was found in the current 250 open PRs -- the one candidate, #4272, turned out to be exactly that housekeeping false positive -- so this arm closes the mechanism, not an observed incident. `--paginate` WITHOUT `--slurp` was a latent defect in the pre-existing `failure` arm, found in review and fixed in both arms. gh concatenates one JSON document per page and jq evaluates each separately, so `max` emits one timestamp PER PAGE; `date -d` then rejects the multi-line string, the epoch reads 0, and the PR is skipped. That silently exempted every PR with more than 100 check-runs, which on this repo is any PR whose lanes have been re-run -- #4718 alone carries ~90. Verified against the live API: three pages produced three `max` lines unslurped and one slurped. The statuses read is now checked for FAILURE before its filter runs. A 503 and a genuinely absent status both yield empty jq output, and the new unpublished arm dispatches on the latter -- so without this, transient GitHub trouble would re-fire arbitrary old PRs every 15 minutes, on a shared token budget, which is the same exhaustion that causes the missing verdict in the first place. `--limit 300` on the open-PR listing is raised to 900 with a loud warning when the ceiling is hit, here and in the two sibling sweeps that share the defect (fork-pr-label.yml, pr-merge-conflict-label.yml, both found in review). `gh pr list` returns newest-first and truncates silently, so a ceiling near the real count drops the OLDEST PRs -- precisely the frozen or unlabelled ones each sweep exists to catch. At 250 open the old value was already at 83% of its ceiling. Same truncation class as the `pr_status_report` limit bug. `gh label list --limit` in those files counts labels, not PRs, and is untouched. The unpublished case is carried as a sentinel `state` rather than an early record write, so it reuses the single staleness comparison and the single stale-file record writer instead of restating either. GitHub's status states are error/failure/pending/success, so the sentinel cannot collide. Not addressed here, because the fix is a policy decision rather than a defect: a required lane that did not exist when the head SHA was pushed can never be satisfied, and a recompute re-derives `not started` forever. #3089 and #3037 are frozen on First Principles Review, which became blocking in #4253 but only exists from #3436. Auto-skipping the lane would let an unreviewed PR pass, so it wants a rebase or a deliberate eligibility rule. Filed as #5957. Test plan: - test/test_pr_readiness_sweep.py: 31 passed (18 pre-existing, 13 added). - unpublished: rescued past the window, left alone inside it, respects MAX_DISPATCH, fails closed on an unparseable timestamp, still reads as unpublished when only a foreign-context status exists, and is NOT triggered by a failed statuses read. - stale green: rescued for all six failure-class conclusions, never rescued by a later pass, self-terminating on republish, untouched with no evidence. - pagination: evidence on a SECOND page is found by both arms, which is what an unslurped `--paginate` cannot do. - truncation: the ceiling is a variable, and a hit is reported. - the context-isolation test now proves a foreign status cannot DRIVE the decision, rather than relying on the absent-status shape it used to assert. - the gh stub models `--paginate --slurp` (outer array of pages) and can fail a statuses read on demand, so both fixes are exercised rather than assumed. - Revert-verify: undoing each of the seven properties independently (unpublished arm, green arm, ceiling, failure-class narrowing, slurp on each arm, statuses transport guard) fails a named test; tree restored to 31 green. - test_workflows_conformance / test_workflow_permissions / test_github_workflow_security / test_pr_quality_gates / test_ai_review_workflows + the sweep suite: 289 passed. - check_black_formatting.py and flake8 clean; all three workflows parse as YAML. - test_pr_readiness_publish.py has 4 pre-existing failures on this host (local jq rejects `label` as an identifier); identical on pristine main, untouched here.
12a5b6a to
958b28f
Compare
|
Round 1 reviewed. All four findings were real; all four are fixed in GPT BLOCKING — multi-page reads ( GPT BLOCKING — API failure read as an unpublished verdict ( Design CONCERNS — API cost of the green arm. The direction is right and the dangerous half is fixed; the magnitude is not what the review states. The First Principles CONCERNS — two unfixed truncation siblings. Correct and verified: Revert-verify now covers seven properties independently — the unpublished arm, the green arm, the ceiling, the failure-class narrowing, slurp on each arm, and the statuses transport guard. Each mutation fails a named test and the tree restores to 31 green: 289 passed across the sweep suite plus the workflow contract suites; black gate and flake8 clean; all three workflows parse. |
github-actions
Bot
added
readiness: checking
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: rescues the two readiness-freeze cases the self-heal sweep skipped — an unpublished SHA with no readiness status (distinguished from a transport failure) and a green verdict over check evidence that turned red after publish — and raises the silently-truncating gh pr list ceiling with a loud warning; clear root cause.
github-actions
Bot
removed
the
readiness: passed
2 participants
Problem / Motivation
pr-readiness-sweep.ymlis the only thing that recomputes a readiness verdict on an unchanged commit once itsworkflow_runevents are spent. It rescues apendingverdict on age and afailureverdict on later check evidence. Two freeze modes fell outside both tests and therefore had no recovery path at all.An unpublished verdict, which is live today.
pr-readiness.ymldeliberately does not retry its status POST — a retry can overwrite a concurrent run's newer verdict — and instead instructs a human to re-run the workflow. When that POST fails, the head SHA carries no readiness status, so there is nopendingto age out, and the sweep skipped the PR outright on[ -n "$status_json" ] || continuewith the comment "the PR's own run is coming; not stuck". The one case the publisher delegates to a re-run was the one case nothing re-ran.#2783 proves it. Both readiness runs for
f014465efailed ongh: HTTP 503from the status POST on 2026-08-17. Eight days later that PR is stillMERGEABLE, its head SHA carries zero commit statuses, and it wears areadiness: action requiredlabel with nothing backing it. Today's #5282 hit the same POST with a 502 and survived only because an unrelated later event happened to republish.A stale green, which is the unsafe direction. The re-run mechanism the sweep was built for is direction-blind: a job re-run that flips a lane red after a green verdict emits no
workflow_run: completedeither. The required aggregate then stays green over a now-red revision, which permits a merge — where a stale red only blocks one. The*)arm skipped it as "successneeds no rescue".A latent truncation.
gh pr list --limit 300against 250 open PRs.ghreturns newest-first and truncates silently, so crossing the ceiling drops the oldest PRs — precisely the frozen ones this sweep exists to rescue. Same bug class as thepr_status_reportlimit.Why it matters
A required status check that is absent or stale-green is worse than one that is stale-red: both block the pipeline's ability to reason about a PR, but these two let a revision through. #2783 has sat mergeable with no verdict for eight days, and the publisher's own error message tells a human to do the one thing no automation was willing to do.
What changed (motivation → approach → change)
Both new rescues reuse mode 2's shape — "evidence landed after the verdict, so the verdict is stale by construction" — because that shape is what makes a scheduled re-fire self-terminating rather than a dispatch loop.
updatedAt, so the grace window restarts on every new revision and a genuinely queueing run is left alone. Carried as a sentinelstaterather than an early record write, so it reuses the single staleness comparison and the single stale-file record writer instead of restating either. GitHub's status states areerror/failure/pending/success, so the sentinel cannot collide with a real one.successanderrorjoin mode 2's evidence test, narrowed to failure-class conclusions. This narrowing is the load-bearing part: housekeeping check-runs (Strip stale workflow-change override,Fork workflow-change guard) legitimately complete days after a verdict on a long-lived PR, so an unnarrowed "any check completed later" test would re-fire most green PRs on every sweep while proving nothing. A latersuccesscannot turn a green verdict red, so it is correctly ignored.--limitbecomesPR_LIST_LIMIT: "900"and a hit emits a::warning::action item instead of being absorbed as a measurement.Honest scope note: I found no live poisoned green in the current 250 open PRs. The one candidate my audit flagged, #4272, turned out to be exactly the housekeeping-check false positive described above. That path closes a mechanism, not an observed incident. The unpublished path closes both.
Tests
test/test_pr_readiness_sweep.py— 28 passed (18 pre-existing, 10 added). The harness extracts the sweep's onerun:block and executes it for real against aghstub, so these lock in the re-fire conditions, which is the whole risk surface: too narrow and a frozen verdict stays frozen, too broad and every PR gets dispatched every 15 minutes.MAX_DISPATCH; fails closed on an unparseable timestamp; still reads as unpublished when only a foreign-context status exists on the SHA.failure,timed_out,cancelled,action_required,stale,startup_failure); never rescued by a later pass orskipped; self-terminating once republished; untouched with no evidence at all.test_a_different_status_context_is_ignoredis reworked intotest_a_different_status_context_never_drives_the_decision. It previously asserted its intent through the absent-status shape, which is now a rescue path; it now gives the SHA a freshPR Readinesspending alongside a long-stale failingCoverage Gateand proves the foreign context cannot drive the decision. Its intent is preserved and strengthened.Revert-verify. Undoing each of the four properties independently — the unpublished arm, the green arm, the ceiling, and the failure-class narrowing — fails at least one named test, and the tree restores to 28 green:
Workflow contract suites unaffected:
test_workflows_conformance,test_workflow_permissions,test_github_workflow_security,test_pr_quality_gates,test_ai_review_workflows— 258 passed.scripts/check_black_formatting.pyandflake8clean on the changed files.test_pr_readiness_publish.pyhas 4 failures on my host (localjqrejectslabelas an identifier); they reproduce identically on pristinemainand this PR does not touchpr-readiness.yml.Manual verification
The sweep cannot be exercised end-to-end without merging it, so verification was the other direction: I reproduced the exact production states the new arms target from live API data, and confirmed the current sweep skips them.
repos/.../commits/f014465e/statusesreturns an empty array while the PR is open andMERGEABLE; bothpr-readiness.ymlruns for that SHA arepull_request_target completed/failure, and the failing step's log ends ongh: No server is currently available ... (HTTP 503)followed by the publisher's "re-run this workflow" error. Under this change the sweep records it asno readiness status published; PR last active …and dispatches.Scanning 252 open pull request(s)→Found 5 stale→ 5 dispatched, allpending/failure.Screenshots / video
N/A — CI workflow and test changes only, no user-visible surface.
Follow-up filed separately
A required lane that did not exist when the head SHA was pushed can never be satisfied, and a recompute re-derives
not startedforever. #3089 and #3037 are frozen this way on First Principles Review, which became blocking in #4253 but only exists from #3436. Auto-skipping the lane would let an unreviewed PR pass, so the fix is a deliberate eligibility rule or a rebase — a policy decision, not this PR's.