fix(ci): scope the Ally review-consistency guard to PRs GitHub would merge (PEN-2847) - #1956
allyblockcast[bot] wants to merge 1 commit into
Conversation
…merge (PEN-2847) The ratchet landed in #1585 restored this guard's ability to signal, but left one class it can only dispose of by hand: a real violation on a PR that cannot merge at all. Each one needs a human-written baseline entry or the run stays red. As of 2026-09-20 exactly one unbaselined violation was outstanding repo-wide — #1220 @a9ee094a, `DIRTY` and untouched since 09-06 — so the hourly run was red, un-actionably, over a PR GitHub itself refuses to merge. All six founding baseline entries had meanwhile self-expired by head movement and were warning on every run asking to be removed. Findings on draft or conflicting PRs are now printed as warnings instead of failing the run. Everything else is audited and fails exactly as before. The two legs of "liveness" are not equally safe and only one is adopted. The `## Ratchet` note in this file previously rejected liveness scoping outright, measured against #1525; that rejection is narrowed, not reversed: - Cannot-merge (adopted). `DIRTY` and draft are states GitHub enforces at the merge button. Leaving one is a transition this hourly run re-observes, so the PR cannot reach master without first re-entering scope. For `DIRTY` the exposure is usually nil: resolving conflicts means pushing, which moves the head, which sheds the violation outright. - Not-updated-in-N-days (still rejected). Staleness is not a state GitHub enforces anything against. A `CLEAN` PR untouched for 30 days merges on a click, with no push, no transition and no run in between. That is unbounded fail-open on exactly the BLO-19778 incident, and it is why #1525's two-day `BEHIND` measurement killed the window approach and still does. Measured on the 121 open PRs of 2026-09-20: 71 live, 50 dormant. #1316 (`UNSTABLE`) and #1360 (`BEHIND`) — two of the four PRs that originally pinned this guard — classify live, so this grants them no amnesty. Three deliberate fail-closed choices, each covered by a surviving mutation: - `DORMANT_MERGE_STATES` is a deny-list of two, not an allow-list of the mergeable states. `mergeStateStatus` is computed lazily and GitHub extends the enum without notice; 5 of 121 PRs read `UNKNOWN` when this was written. An allow-list would drop every unrecognised and not-yet-computed state out of the audit. Unknown, missing and malformed all resolve to live. - A finding whose PR cannot be resolved stays failing. "I could not tell whether this matters" must not read as "this does not matter". - `assertLiveScopeNonVacuous` throws if every open PR classifies dormant. Otherwise a schema change or a bad edit here would downgrade every violation to a warning and print a green pass having failed nothing — "green because it stopped checking", which PEN-2847 names as worse than the permanent red it replaced. Same reflex as `assertPrListComplete`. Baseline emptied: the six founding entries had all self-expired. The committed -baseline test is re-pointed rather than deleted — it now feeds the exact set those entries used to suppress and asserts every one fails, so emptying the file cannot be mistaken for the ratchet still working. Signed-off-by: Devops <devops@paperclip.blockcast.net>
1 similar comment
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
Thinking Path
The ratchet I landed in #1585 restored this guard's ability to signal, but left one class it can only dispose of by hand: a real violation on a PR that cannot merge at all. Each one needs a human-written baseline entry, or the hourly run stays red. Re-measured on current state rather than working from the handoff: exactly one unbaselined violation was outstanding repo-wide — #1220
@a9ee094a,DIRTY, untouched since 09-06 — not the two last reported, because #1757's head moved fromf1060b70tob21d6f79and its finding self-expired. Meanwhile all six founding baseline entries had expired the same way and were warning on every run asking to be removed. So the guard was red, un-actionably, over a single PR GitHub itself refuses to merge.The obvious fix is the issue's "option 2, scope by liveness", but this file already contains my own measured argument rejecting that, pinned to #1525 —
BEHINDand two days old, inside any plausible window, so a window would have left the run red anyway. I did not want to silently reverse a recorded measurement, so I checked whether the two legs of "liveness" are equally safe. They are not, and only one is adopted here.Cannot-merge (adopted).
DIRTYand draft are states GitHub enforces at the merge button. Leaving one is a state transition this hourly run re-observes, so a deferred PR cannot reachmasterwithout first re-entering scope — bounded exposure. ForDIRTYthe exposure is usually nil, because resolving conflicts means pushing, which moves the head, which sheds the violation outright. Not-updated-in-N-days (still rejected). Staleness is not a state GitHub enforces anything against: aCLEANPR untouched for 30 days merges on a click, with no push, no transition, and no run in between. That is unbounded fail-open on exactly the BLO-19778 incident this guard exists for. #1525's measurement killed the window approach and still does — so there is deliberately no calendar window in this change.What Changed
prDormancy()classifies a PR as dormant only onisDraftormergeStateStatus === "DIRTY". Unbaselined findings on dormant PRs are printed as::warning::and no longer fail the run; everything else is audited and fails exactly as before.DORMANT_MERGE_STATESis a deny-list of two, not an allow-list of the mergeable states.mergeStateStatusis computed lazily server-side and GitHub extends the enum without notice — 5 of 121 open PRs readUNKNOWNwhen this was written. An allow-list would silently drop every unrecognised and not-yet-computed state out of the audit. Unknown, missing and malformed inputs all resolve to live.failing. "I could not tell whether this matters" must not read as "this does not matter".assertLiveScopeNonVacuous()throws if every open PR classifies dormant. Without it, a schema change or a bad edit here would downgrade every violation to a warning and print a green pass having failed nothing — "green because it stopped checking", which PEN-2847 explicitly names as worse than the permanent red it replaced. Same reflex as the existingassertPrListComplete/assertHeadSha.fetchOpenPrsnow requestsisDraft,mergeStateStatusin the existinggh pr listcall — no extra API round-trips per PR.Verification
Done-when #1 — a run goes green on current repo state. Full script against live GitHub:
The finding is still computed and still printed — it is deferred, not dropped.
Done-when #2 — a new violation still reds it. Three arms on live data, injecting one BLO-19778-shaped violation (two operative App reviews, differing bodies, same head) onto real open PR #1757
@b21d6f79:failing=0→ exit 0CLEANfailing=1→ exit 1DIRTYdeferred=1→ exit 0A proves the red in B is caused by the injection and not something pre-existing. B vs C is the discriminator: identical finding, identical head, only the merge state differs — so the scoping keys on mergeability and has not simply stopped checking.
The suite is not vacuous. 140/140 pass, and five mutations were each caught (22, 10, 4, 4 and 7 failing tests respectively): classifying every state dormant; allow-list instead of deny-list, so
UNKNOWNleaves scope; unresolvable PRs deferring instead of failing closed;assertLiveScopeNonVacuousremoved; drafts no longer deferred. An earlier mutation run reported two false "SURVIVED" results from a shell quoting error that meant the patch never applied — rewritten to verify each anchor matches before trusting the verdict.Degradation. If the Actions
GITHUB_TOKENcannot readmergeStateStatus, every PR reads live, and the run reds on #1220 exactly as it does today — visibly, in the safe direction.Risks
DIRTYPR whose conflicts vanish because its base moved becomes mergeable without its head moving, so its finding is un-deferred only on the next hourly run — up to ~1h of exposure. Same for un-drafting. Accepted: it is bounded by the poll interval and the PR cannot merge before re-entering scope, which is the property a calendar window does not have.COMMENTEDreview cannot be dismissed (422, no delete API), so it persists until that PR moves or closes. This PR changes what the auditor fails on, not what Ally posts; PEN-2865 owns the source.DIRTY/BEHINDand none touches liveness scoping, so no duplicate work — but whichever lands second will need a rebase.Model Used
claude-opus-5[1m]