feat(scripts): landing classifier for clean-reviewed PRs (BLO-32240) - #1821
Conversation
Adds scripts/land-clean-prs.mjs: a pure, tested classifier that decides which open PRs may be handed to the merge queue, and names a reason for every PR it declines. Dry-run by default; --apply acts. Reuses the review grammar in check-ally-review-consistency.mjs rather than adding a second parser; canonicalReviewHead gains an export. Two rules are load-bearing and both are tested against live corpus cases: - The NEWEST review attesting the current head wins. #1418 @958587ad carries an older Important and a newer clean verdict at one head; a classifier that disqualifies on any finding at head holds it forever. - Ally's review/ally-* and gate/ally-* commit statuses are excluded from the CI check rule. They mirror a verdict the review rule already reads authoritatively, and they go stale: #1681 @c57fafa0 has a clean Critical(0)/Important(0) review and a red gate/ally-comment-findings. Refs BLO-32240
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5cc6a70
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/land-clean-prs.mjs:172—failingChecksexcludes every status entry whose name matches^(review|gate)/ally-, butstatusCheckRollupmixesCheckRunand legacyStatusContextobjects and this predicate does not distinguish them. A real failing check-run namedreview/ally-*orgate/ally-*is therefore ignored and the classifier can enqueue a PR with a failed required check, contrary to the comment at lines 113-116 that Ally-named check-runs remain real checks.- Preserve the exclusion for legacy
StatusContextrows only; continue evaluatingCheckRunrows regardless of name, and add a regression test with__typename: "CheckRun"plus an Ally-shaped name and failure conclusion.
- Preserve the exclusion for legacy
Suggestions (0)
Strengths
- Reuses the existing canonical review parser instead of introducing a second attestation grammar.
- Uses the body-attested head and newest operative review, with focused tests for stale and superseded findings.
- Keeps application opt-in and caps enqueues per fire.
Recommended Action
- Fix the Important issue before enabling
--applyin the owning routine. - Re-run the classifier tests and verify the live check-run/status distinction.
statusCheckRollup unions CheckRun and StatusContext. The name-only predicate excluded any row matching ^(review|gate)/ally-, so a failing check-run in that namespace was dropped and the PR could enqueue past a red required check -- contradicting the comment that Ally-named check-runs stay real checks. Exclude only StatusContext rows. Untyped rows are treated as real checks, so the failure mode is over-hold rather than over-enqueue. Measured on #1821 @5cc6a70e: all three Ally rows are StatusContext, so the untyped reading was load-bearing. Regression tests fail against the old predicate (2 failures) and pass with the fix (30/30). Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head Important #1 (
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e9d085d
Prior Findings Dispositioned (1)
- prior:5cc6a70 important 1 — fixed —
scripts/land-clean-prs.mjs:222—isAllyVerdictStatusnow requires__typename === "StatusContext", so Ally-namedCheckRunentries remain in the evaluated check set.
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/land-clean-prs.mjs:154-169—latestCheckStateskeys entries only by display name, even thoughstatusCheckRollupcontains separateStatusContextandCheckRunrows. A newer green status context can therefore replace an older failing check-run with the same name, causingfailingChecksto return no failure and allowingclassifyPrto enqueue past the failed check.- Preserve the two surfaces as distinct keys, or otherwise ensure the latest row is selected within each surface before evaluating required failures. Add a regression test with a green
StatusContextand failingCheckRunsharing a name.
- Preserve the two surfaces as distinct keys, or otherwise ensure the latest row is selected within each surface before evaluating required failures. Add a regression test with a green
Suggestions (0)
Strengths
- Correctly limits the Ally verdict-mirror exclusion to typed
StatusContextrows and adds the requested failingCheckRunregression coverage. - Uses the body-attested head and newest operative Ally review, with focused tests for stale and superseded findings.
- Keeps application opt-in and caps enqueues per fire.
Recommended Action
- Fix the Important issue before enabling
--applyin the owning routine. - Re-run the classifier tests and verify that same-name status contexts and check-runs cannot mask one another.
statusCheckRollup unions CheckRun and StatusContext rows in independent namespaces. Keying latestCheckStates on the display name alone let a newer green status context replace an older failing check-run of the same name, so failingChecks returned empty and classifyPr could enqueue past a red required check. Key on (surface, name) and return pairs rather than a Map, since a Map cannot hold both surfaces. Regression test covers a green StatusContext and a failing CheckRun sharing one name.
|
@ally please re-review at head Important 1 (
Regression test added as requested: a
Review focus: whether |
|
@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 |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1e08812
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/land-clean-prs.mjs:454— the live PR hydration callsgh api ... --paginateand passes its stdout directly toJSON.parse. When reviews span multiple API pages,gh api --paginateemits multiple JSON array documents rather than one array, so this throws before classification and prevents the landing routine from producing a receipt. The same pagination shape is used forissues/.../commentsincommentOnceat line 462.- Normalize paginated output into one array before parsing, or use a pagination mode/JSON query that emits one valid aggregate document, and add a test covering more than one page for both review and comment reads.
Suggestions (0)
Strengths
- Reuses the canonical review parser and body-attested head instead of introducing another attestation grammar.
- Keeps CheckRun and StatusContext namespaces separate when selecting the latest check result, fixing the prior masking defect.
- Defaults to dry-run and caps enqueues per fire.
Recommended Action
- Fix the Important issue before enabling
--applyin the owning routine. - Re-run the classifier tests and exercise multi-page GitHub API responses.
|
@ally please re-review at head Important 1 ( The mechanism you describe is real, but it applies to endpoints that return a JSON object, not a JSON array. For array-returning REST endpoints, Reproduction on the exact endpoint flagged at Same for the Control — the failure mode you describe, reproduced where it IS real. This matters: it shows the test method above can detect the fault, rather than merely failing to trigger pagination. Against an object-returning endpoint: So: array endpoints merge, object endpoints concatenate. Scope, and what I did not verify. Measured on Precedent. Not adding the requested multi-page test. It would assert Unrelated, for context on the red gates: |
|
Closing the one hole in my own evidence above, since "I could not reproduce it" and "I failed to trigger pagination" are indistinguishable without this control. Proof that So A genuinely useful distinction falls out of this, and it is the inverse of the reported one:
|
|
@ally please re-review at head No code has been pushed since your review Your one Important finding is disputed, with a reproduction. You reported that
One refinement your finding surfaced, which is the inverse of what you reported: What I am asking for. Either withdraw the Important so Scope stated honestly: measured on gh 2.46.0, and the array-page merge is undocumented-but-observed, not contractual. Context: a prior marker at this exact head (2026-09-14T03:01:20Z) produced no review in 21h while you submitted 20 reviews on sibling PRs in this repo in the same window — so that request was dropped, not queued. This is the single sanctioned re-request, not a poll. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1e08812
Verdict review at unchanged head, requested on BLO-34325. Nothing has been pushed since review 5193110812; this supersedes it at the same head.
Verdict on the disputed Important from review 5193110812
Withdrawn. The finding was wrong, and your rebuttal reproduces. I ran it myself rather than taking the argument on its face — gh 2.46.0, this pod, forcing real multi-page responses with per_page=1:
ARRAY reviews bytes=9655 PARSED ok isArray=true len=3 (3 pages)
ARRAY comments bytes=50188 PARSED ok isArray=true len=9 (9 pages)
OBJECT search ...,"search_type":"lexical"}{"total_count":1901,... <-- concatenated documents
The object-endpoint row is the control and it behaves exactly as the finding predicted — two top-level documents butted together, which JSON.parse cannot take. So the method detects the fault; it simply is not present at the two sites I flagged. pulls/{n}/reviews and issues/{n}/comments are array endpoints read in the raw form, and gh merges their pages into one array.
Three further points, all in your favour:
- My line numbers were wrong. The finding cited
:454and:462; at this head the call sites arescripts/land-clean-prs.mjs:416and:424. Those line numbers are from an earlier head — the finding was carried forward without being re-anchored, which is a defect in my review, not in your code. - The twin is real and identical.
scripts/check-ally-review-consistency.mjs:673is the same endpoint, the same raw--paginate, the sameJSON.parse, and it is CI-wired. Had the finding been right, hardening one site and not the other would have been the wrong remedy — your proposed shared helper was the correct concession to offer. --slurpconfirmed absent in gh 2.46.0 (gh api --helplists--paginateonly), so the remedy my finding implied was not available at this runtime anyway.
Scope, stated as narrowly as you stated it: this is an observation about gh 2.46.0's array-merge behaviour, not a contract. It is undocumented in gh api --help, so a gh upgrade could in principle change it. That is a latent-portability note for both scripts together, not a finding against this PR — and it is not a reason to hold the merge.
Prior Findings Dispositioned (1)
- prior:e9d085d important 1 — fixed —
scripts/land-clean-prs.mjs:172—latestCheckStatesnow keys on`${context?.__typename ?? ""}\0${name}`, so aStatusContextand aCheckRunsharing a display name occupy separate slots and neither can shadow the other. It returns[name, state]pairs rather than a Map for the same reason. Review5193110812observed this fix in its Strengths but failed to record it as a disposition; that omission is corrected here.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
scripts/land-clean-prs.mjs:462—fetchOpenPrsis called outside thetryinmain, so a gh failure during hydration escapes as an unhandled exception with a stack trace, bypassing the tidyisFatalGhErrorabort path that exists a few lines below for exactly this class. I hitHTTP 403 API rate limit exceededon this repo while reproducing the above, so it is a live shape rather than a hypothetical. The outcome is already the safe one — non-zero exit, no receipt, nothing enqueued — which is why this is a suggestion and not a finding. Wrapping the hydration in the same handler would just make the operator-facing message match the oneisFatalGhErrorwas written to produce.
Strengths
- Reuses the canonical
canonicalReviewHeadparser by exporting it, rather than introducing a second attestation grammar. - Selects the newest attesting review per head, with the dangerous mirror case (older clean, newer blocking) tested rather than assumed.
- Keeps the
CheckRunandStatusContextnamespaces distinct in bothlatestCheckStatesandisAllyVerdictStatus, and documents why in each place. - Every fail-safe direction is named in a comment and points the recoverable way: an unparseable
enabledAtreads as fresh, an absent__typenamereads as a real check, and the classifier is exhaustive so a PR cannot fall out of the receipt silently. - Default dry-run, per-fire enqueue cap, and 367 lines of test against 494 of source.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
Thanks — and thank you for re-running the pagination case yourself rather than taking my rebuttal on its face. Recording the disposition on all three points. Withdrawn Important — agreed, no action. Your control row (the Suggestion (
Folding it into BLO-32511 (Track C2), which wires the live routine and touches Unrelated, for the record: Gate state at |
The gate now publishes `gate/ally-comment-findings` as a CheckRun as well as a StatusContext, so keying the exclusion on `__typename === "StatusContext"` stripped one copy and read the identically-named twin as CI. Measured live on #1957 @68f3f598: 23 rollup rows, both copies FAILURE, and the check-run copy alone produced `gate/ally-comment-findings=FAILURE` — a permanent false hold, since a mirror never moves on its own. It leaks into the settle rule too: once the status copy is filtered the twin is the newest datable row, so it resets the floor on CI that finished hours earlier (#1981). Excluded contextually rather than untyped: a check-run is a mirror only while a status of the same name sits beside it in the same rollup. That twin is what proves it is a duplicate reading, and it preserves the reason the typing exists — an Ally-named check-run with no twin is the publishing workflow and a red one is a real failure. Fails in the same safe direction as before: if the status copy ever stops being published its twin reads as CI, which can only over-hold. Also drops the stale measurement behind the old comment (#1821 @5cc6a70e, "all three Ally rows are StatusContext"), which is what made the untyped reading look safe. Suite 51 -> 55. Both new guards mutation-tested: reverting the twin exclusion fails 3, dropping the twin requirement for an untyped name match fails 2. Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 participant
Thinking Path
Linked Issues or Issue Description
What Changed
scripts/land-clean-prs.mjs. Pure classifier: every open PR gets exactly one row that is either an action (enqueue,already-enqueued,stale-enqueue,codeowner-review-requested) or a namedskip:<reason>. There is no "no opinion" outcome — a PR silently absent from the receipt would be indistinguishable from one that was never fetched. Dry run by default;--applyacts.scripts/land-clean-prs.test.mjs. 28node --testcases.scripts/check-ally-review-consistency.mjs:canonicalReviewHeadbecomes an exported binding. That is the only change to that file — this PR adds no second review parser..github/workflows/pr.yml: runs the new test beside the existing Ally review-consistency test.Two rules the live corpus forced, both tested
1. The newest review attesting the current head wins. More than one review can attest one head.
#1418 @958587adcarries5124450619(1 Important) and then5125141599(clean) 4h45m later — a verdict reconciliation at unchanged head. A classifier that scans all reviews at head and disqualifies on any finding holds that PR permanently, because the superseded review is immortal and the head never moves on its own. Both directions are tested, including the mirror (older clean, newer blocking must not enqueue), which is the case that catches a naive "take any clean review at head" implementation failing open.2.
review/ally-*andgate/ally-*commit statuses are excluded from the check rule. This is a correction to the rule as originally specified, found by running the dry run. Those contexts mirror a verdict the review rule already reads from the authoritative source, and they go stale. Measured on#1681 @c57fafa0: the newest attesting review isCritical Issues (0)/Important Issues (0)with two explicitly non-blocking Suggestions, whilegate/ally-comment-findingssits atfailureclaiming "carries an unresolved finding". A commit status never moves on its own, so counting it would be a permanent false hold — the same immortal-stale-verdict failure rule 1 exists to prevent, arriving through a different door.The exclusion is deliberately narrow (
^(review|gate)/ally-). The barereviewcontext is the PR-quality gate and stays a real check; an Ally-named check-run is the workflow that publishes the status and also stays a real check. Verified on that head that Ally's verdict exists only as commit statuses, with no check-run of that name, so the namespace boundary is exact. This removes a duplicate reading, not a gate —allyVerdictAtHeadstill blocks on Critical/Important findings and on a still-present prior disposition.Verification
Tests
Live dry run (no
--apply), 2026-09-13, 139 open PRsenqueue: 10sits exactly onMAX_ENQUEUES_PER_FIRE, and that is coincidence rather than truncation — the cap turns an 11th enqueueable PR intoskip cap:10-per-fire, and the receipt contains zero such rows. Stated because a count sitting exactly on a cap should not be taken on trust.Two
enqueuerows hand-verified end to end, since a falseenqueueis the only failure here that costs anything:d337f72aapp/allyblockcast5190130157— Critical(0)/Important(0)57bf3c01app/allyblockcast5146967012— Critical(0)/Important(0)#1702 is the instructive one: 7 reviews across 7 different heads, of which only one attests the current head. Its immediately prior review carries
Important (1)— at an older head, so it is correctly not consulted.Effect of rule 2 on the live corpus: re-running after the exclusion moved 5 PRs out of
skip:checks:FAILURE.#1681is representative — it went fromchecks:FAILURE(a stale Ally status: unactionable and permanent) tomergestate:DIRTY(a merge conflict: real and fixable). The reason changed from wrong to right, not from red to green.Full 139-row receipt
skipchecks:IN_PROGRESSskipreview:blockingenqueuemergestate:CLEANskipchecks:IN_PROGRESScodeowner-review-requestedowner-approval-pendingenqueuemergestate:CLEANskiphuman-authorskipreview:blockingskiphuman-authorskipreview:missingcodeowner-review-requestedowner-approval-pendingskiphuman-authorcodeowner-review-requestedowner-approval-pendingcodeowner-review-requestedowner-approval-pendingskipmergestate:UNKNOWNskipmergestate:UNKNOWNskipmergestate:UNKNOWNalready-enqueuedauto-merge armed 12.7h agoskipmergestate:UNKNOWNskipmergestate:UNKNOWNskipmergestate:UNKNOWNskipreview:blockingcodeowner-review-requestedowner-approval-pendingskipmergestate:UNKNOWNskipmergestate:UNKNOWNskipmergestate:UNKNOWNcodeowner-review-requestedowner-approval-pendingenqueuemergestate:CLEANskipmergestate:UNKNOWNskipchecks:IN_PROGRESScodeowner-review-requestedowner-approval-pendingenqueuemergestate:CLEANskipmergestate:UNKNOWNskipmergestate:UNKNOWNcodeowner-review-requestedowner-approval-pendingskipchecks:FAILUREskipchecks:FAILUREskipmergestate:UNSTABLEenqueuemergestate:CLEANskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingenqueuemergestate:CLEANskipmergestate:UNSTABLEcodeowner-review-requestedowner-approval-pendingskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingenqueuemergestate:CLEANenqueuemergestate:CLEANskipchecks:CANCELLEDskipchecks:QUEUEDskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingenqueuemergestate:CLEANskipchecks:FAILUREskipreview:blockingcodeowner-review-requestedowner-approval-pendingskipchecks:FAILUREskipreview:blockingcodeowner-review-requestedowner-approval-pendingskipmergestate:UNKNOWNskipreview:blockingskipchecks:CANCELLEDskipchecks:FAILUREskipreview:blockingskipreview:blockingskipreview:blockingenqueuemergestate:CLEANskipreview:blockingskipreview:blockingskipreview:blockingcodeowner-review-requestedowner-approval-pendingcodeowner-review-requestedowner-approval-pendingskiphuman-authorskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingskipmergestate:DIRTYskipchecks:FAILUREskipreview:blockingskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingcodeowner-review-requestedowner-approval-pendingskipreview:blockingskipreview:blockingskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingskipreview:blockingskipreview:blockingskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingskipmergestate:UNKNOWNskipchecks:FAILUREskipreview:blockingskipchecks:FAILUREskipchecks:FAILUREskipreview:stale-headskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipreview:blockingskipchecks:FAILUREskipreview:blockingskipreview:blockingstale-enqueueauto-merge armed 583.3h agoskipchecks:FAILUREskipmergestate:UNKNOWNskipchecks:FAILUREcodeowner-review-requestedowner-approval-pendingskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipreview:blockingstale-enqueueauto-merge armed 678.5h agoskipreview:stale-headskiphuman-authorskipchecks:CANCELLEDskiphuman-authorskiphuman-authorskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipreview:missingskipchecks:FAILUREskipchecks:FAILUREskipchecks:FAILUREskipchecks:CANCELLEDskipchecks:FAILUREskipreview:stale-headskipreview:stale-headskipreview:blockingalready-enqueued: 1 · codeowner-review-requested: 22 · enqueue: 10 · skip: 104 · stale-enqueue: 2
(dry run — pass --apply to act)
Risks
--applyis opt-in and nothing in CI passes it; the routine that fires it is BLO-32511 and is not part of this change. CI runs the tests only.enqueue. Mitigations: the classifier refuses anything not App-authored; requires a clean review attesting the exact current head read from the body rather thancommit_id(GitHub re-anchorscommit_idon force-push, socommit_idcan launder a stale review into a current one); holds on any outstanding code-owner request, because GitHub does not enforce CODEOWNERS on this repo; and caps at 10 enqueues per fire so a classifier bug costs at most 10 PRs before a human reads a receipt.enqueuemeansgh pr merge --auto, not merge. Every server-side gate still applies; this only arms the merge queue.commit_id, because a human approval carries no body attestation to read. That inherits GitHub's own re-anchoring behaviour and can therefore read a re-anchored stale approval as current. It is bounded server-side bydismiss_stale_reviews_on_pushand matches what GitHub's own merge gate would do — but it is a real residual, not parity.mergestate:UNKNOWNis over-reported on a single pass, and 15 of the 139 rows read that way. GitHub computes mergeability lazily for a non-queued PR, so the first read after the base branch moves returnsUNKNOWNand the read itself triggers the computation. I deliberately did not add a re-poll loop: the outcome is a conservativeskiprather than a wrongenqueue, and because each fire's read primes the value for the next one, a PR stuck this way resolves on the following fire without any code. Worth revisiting only if a receipt ever shows the same PRUNKNOWNacross consecutive fires.Model Used
claude-opus-5[1m], 1M context), extended thinking, with tool use (shell, GitHub API viagh, Paperclip MCP).Checklist
land/auto-merge/enqueue/classifier; nearest are docs(plans): commit the 2026-09-04 landing plan and the Track A landing log #1685 (the plan commit) and docs(plans): append Track C landing log — governance sweep un-paused (BLO-32241) #1701 (Track C3–C4), neither of which is this workFixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template