From 5cc6a70edc1cc3ddcdf67311369297f7059db120 Mon Sep 17 00:00:00 2001 From: CTO Date: Sun, 13 Sep 2026 10:36:16 +0000 Subject: [PATCH 1/3] feat(scripts): landing classifier for clean-reviewed PRs (BLO-32240) 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 --- .github/workflows/pr.yml | 8 + scripts/check-ally-review-consistency.mjs | 2 +- scripts/land-clean-prs.mjs | 469 ++++++++++++++++++++++ scripts/land-clean-prs.test.mjs | 318 +++++++++++++++ 4 files changed, 796 insertions(+), 1 deletion(-) create mode 100644 scripts/land-clean-prs.mjs create mode 100644 scripts/land-clean-prs.test.mjs diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 4df4308c63bd..4c3e15528e09 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -450,6 +450,14 @@ jobs: run: node --test ./scripts/check-ally-review-consistency.test.mjs timeout-minutes: 1 + - name: Test landing classifier + if: ${{ !cancelled() }} + # The classifier itself is not run here. It only ever reports in CI + # (`--apply` is opt-in) but it makes one `gh` call per open PR, and the + # routine that owns the live fire is BLO-32511. + run: node --test ./scripts/land-clean-prs.test.mjs + timeout-minutes: 1 + - name: Test comment-review-gate census if: ${{ !cancelled() }} # The census itself is not run here: it fails on 100% of sampled merges diff --git a/scripts/check-ally-review-consistency.mjs b/scripts/check-ally-review-consistency.mjs index 9bc269ed74e2..0c83c0e71196 100644 --- a/scripts/check-ally-review-consistency.mjs +++ b/scripts/check-ally-review-consistency.mjs @@ -158,7 +158,7 @@ function reviewDetails(reviews) { return reviews.map((review) => `${reviewState(review)}/${review.id}`).join(", "); } -function canonicalReviewHead(body) { +export function canonicalReviewHead(body) { const text = String(body ?? ""); const headings = Array.from(text.matchAll(CANONICAL_REVIEW_HEADING_RE)); const attestations = Array.from(text.matchAll(ATTESTED_HEAD_GLOBAL_RE)); diff --git a/scripts/land-clean-prs.mjs b/scripts/land-clean-prs.mjs new file mode 100644 index 000000000000..3f9ff442bc43 --- /dev/null +++ b/scripts/land-clean-prs.mjs @@ -0,0 +1,469 @@ +#!/usr/bin/env node + +/** + * Decides which open PRs may be handed to the merge queue, and why each of the + * rest may not. + * + * Nothing currently lands a PR that Ally has already reviewed clean. They sit + * open until a human notices, which on 2026-09-04 was the third of four + * measured causes of 101 issues stalled `in_review` for 22-87 days + * (BLO-32237). This script is the decision; the routine that runs it + * (BLO-32511) only invokes it and posts the receipt. + * + * The classifier is pure and exhaustive: every PR gets exactly one row, and a + * row is either an action or a named reason for inaction. There is no "no + * opinion" outcome, because a PR silently absent from the receipt is + * indistinguishable from one the script never fetched — the fail-open shape + * `assertPrListComplete` exists to prevent one layer up. + * + * Default is a dry run. `--apply` performs the actions. + * + * ## Why the review verdict is read from the body, not from `commit_id` + * + * GitHub rewrites `commit_id` on APPROVED reviews when the head moves, so a + * review can be recorded against a commit created after it was submitted + * (measured on #1104, 2026-08-07; see I3 in check-ally-review-consistency.mjs). + * The body's `Reviewed head:` attestation is the record of which tree was + * actually read, so `canonicalReviewHead` is the selector here. Using + * `commit_id` would let a force-push launder a stale review into a current one + * — and this script's whole output is "merge it", so that failure is fatal. + * + * ## Why the *newest* attesting review wins (BLO-32240, Ally 2026-09-07) + * + * More than one review may attest the same head. #1418 @958587ad carries two: + * `5124450619` with 1 Important, then `5125141599` clean 4h45m later, a verdict + * reconciliation at unchanged head. A classifier that scans all reviews at head + * and disqualifies on any Important reads that PR as blocked *permanently*, + * because the superseded review is immortal and the head never moves on its + * own. So: select the newest attesting operative review by `submitted_at` and + * judge only that one. This matches the gate's own stated semantics — + * `gate/ally-comment-findings` describes itself as reading Ally's "most recent" + * consolidated review for the head. + * + * The mirror case is the dangerous one and is tested: older clean, newer + * blocking must NOT enqueue. A naive "take any clean review at head" passes the + * first case and fails open on the second. + * + * Note this is not in tension with the "exactly one attestation" rule. That + * rule is about one `Reviewed head:` line per *body* (anti-forgery), enforced + * by `canonicalReviewHead`; it is not a cap on reviews per head. + */ + +import { execFileSync } from "node:child_process"; +import { resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { + allyReviewLane, + assertHeadSha, + assertPrListComplete, + canonicalReviewHead, + hasBlockingFindings, + hasStillPresentDisposition, + isAllyAppLogin, +} from "./check-ally-review-consistency.mjs"; + +/** + * An auto-merge request older than this is treated as wedged rather than + * pending. The merge queue drains in minutes; a request still outstanding + * overnight means it was armed against a gate that has since become + * unsatisfiable, and it blocks re-enqueue because `--auto` is already set. 13h + * clears a normal overnight queue without waiting a second working day. + */ +export const STALE_ENQUEUE_HOURS = 13; + +export const CODEOWNER_REQUEST_MARKER = ""; +export const STALE_ENQUEUE_MARKER = ""; + +/** Either label is an explicit human "not this one". */ +export const SKIP_LABELS = ["do-not-merge", "review-gate-override"]; + +/** + * A blast-radius cap, not a throughput target. A classifier bug that says + * "enqueue" for the wrong reason costs at most this many PRs per fire, and the + * receipt names every PR the cap deferred so the truncation is never silent. + */ +export const MAX_ENQUEUES_PER_FIRE = 10; + +/** + * SKIPPED and NEUTRAL are passes: a path-filtered job and a soft-reporting one + * both legitimately decline to run. Everything else — including a null + * conclusion, which is a check still in flight — is not a verdict of success + * and must not be read as one. + */ +const PASSING_CHECK_STATES = new Set(["SUCCESS", "NEUTRAL", "SKIPPED"]); + +/** + * Contexts that mirror Ally's review verdict rather than reporting CI. + * + * These are excluded from the check rule because the review rule below reads + * the same fact from the authoritative source, and reads it correctly. The + * statuses do not: measured on #1681 @c57fafa0 (2026-09-13), the newest + * attesting review is `Critical Issues (0)` / `Important Issues (0)` — clean, + * two non-blocking Suggestions — while `gate/ally-comment-findings` sits at + * `failure` claiming "carries an unresolved finding". Same divergence recorded + * on #1418 (BLO-32514), where the status was stale for over a day against a + * verdict it no longer described. + * + * Counting them as checks would be a permanent false hold: a commit status + * never moves on its own, so such a PR could never enqueue however clean its + * review became. That is the same immortal-stale-verdict failure the + * newest-review rule exists to prevent, arriving through a different door. + * + * Deliberately narrow. It matches only the `review/ally-*` and `gate/ally-*` + * legacy-status namespace. The bare `review` context is the PR-quality gate and + * is a real check; any Ally-named *check-run* is the workflow that publishes + * the status, and is also a real check. Neither is excluded. Verified on that + * head: the Ally 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. `allyVerdictAtHead` still + * blocks on Critical/Important findings and on a still-present prior + * disposition, from the newest review that attests the current head. + */ +const ALLY_VERDICT_STATUS_RE = /^(?:review|gate)\/ally-/i; + +/** + * Merge states that mean the PR cannot land as it stands. BLOCKED and BEHIND + * are deliberately absent: BLOCKED is the normal state of a PR awaiting the + * code-owner approval we may have just requested, and BEHIND is what the merge + * queue exists to resolve. `--auto` waits for both correctly. + */ +const UNLANDABLE_MERGE_STATES = new Set(["DIRTY", "UNSTABLE", "UNKNOWN"]); + +function isDismissedOrPending(review) { + const state = String(review?.state ?? "").toUpperCase(); + return state === "DISMISSED" || state === "PENDING"; +} + +function lower(value) { + return String(value ?? "").toLowerCase(); +} + +/** + * The latest state per check name. + * + * A re-run, or a cancelled-then-superseded run, leaves both attempts hanging + * off the same head. Reading them all makes a green PR look red (BLO-32733), + * so the newest row per name wins. `statusCheckRollup` unifies the check-run + * and legacy commit-status surfaces, which is why this reads one field rather + * than two endpoints that are each blind to the other. + */ +export function latestCheckStates(rollup) { + const latest = new Map(); + for (const context of rollup ?? []) { + const name = String(context?.name ?? context?.context ?? "").trim(); + if (name === "") continue; + const state = String( + context?.conclusion || context?.state || context?.status || "PENDING", + ).toUpperCase(); + const at = Date.parse( + context?.completedAt || context?.startedAt || context?.createdAt || "", + ); + const stamp = Number.isFinite(at) ? at : 0; + const seen = latest.get(name); + if (!seen || stamp >= seen.stamp) latest.set(name, { state, stamp }); + } + return new Map([...latest].map(([name, { state }]) => [name, state])); +} + +export function failingChecks(rollup) { + return [...latestCheckStates(rollup)] + .filter(([name]) => !ALLY_VERDICT_STATUS_RE.test(name)) + .filter(([, state]) => !PASSING_CHECK_STATES.has(state)) + .map(([name, state]) => `${name}=${state}`); +} + +/** Operative Ally App reviews whose body attests this exact head. */ +export function attestingAppReviews(reviews, headSha) { + const head = lower(headSha); + return (reviews ?? []).filter( + (review) => + allyReviewLane(review?.user) === "app" && + !isDismissedOrPending(review) && + canonicalReviewHead(review?.body) === head, + ); +} + +/** + * @returns {{verdict: "clean"|"blocking"|"stale-head"|"missing", review: object|null}} + */ +export function allyVerdictAtHead(pr) { + const attesting = attestingAppReviews(pr?.reviews, pr?.headRefOid); + if (attesting.length === 0) { + const hasOperativeAppReview = (pr?.reviews ?? []).some( + (review) => allyReviewLane(review?.user) === "app" && !isDismissedOrPending(review), + ); + return { verdict: hasOperativeAppReview ? "stale-head" : "missing", review: null }; + } + + const newest = attesting.reduce((a, b) => + (Date.parse(b?.submitted_at ?? "") || 0) >= (Date.parse(a?.submitted_at ?? "") || 0) ? b : a, + ); + const blocking = + hasBlockingFindings(newest.body) || hasStillPresentDisposition(newest.body); + return { verdict: blocking ? "blocking" : "clean", review: newest }; +} + +/** + * Requested reviewers with no APPROVED review at the current head. + * + * D7: GitHub does not enforce CODEOWNERS on this repo, so an outstanding owner + * request is a real gate that only this script observes. A team request can + * never be satisfied by a login match and so always counts as outstanding, + * which is correct — a team has not reviewed until one of its members has. + * + * This matches on `commit_id` rather than a body attestation because a human + * approval carries no attestation to read. That inherits GitHub's own + * re-anchoring behaviour on force-push, i.e. it can read a re-anchored stale + * approval as current. The residual is bounded by `dismiss_stale_reviews_on_push` + * server-side, and the direction matches what GitHub's own merge gate would do. + */ +export function unsatisfiedOwners(pr) { + const head = lower(pr?.headRefOid); + const approved = new Set( + (pr?.reviews ?? []) + .filter( + (review) => + String(review?.state ?? "").toUpperCase() === "APPROVED" && + lower(review?.commit_id) === head, + ) + .map((review) => lower(review?.user?.login)), + ); + return (pr?.reviewRequests ?? []) + .map((request) => + request?.login ? String(request.login) : request?.slug ? `team:${request.slug}` : null, + ) + .filter((who) => who !== null && !approved.has(lower(who))); +} + +/** + * The rules decidable from `gh pr list` alone, or null when this PR needs its + * checks and reviews fetched. + * + * Split out because `statusCheckRollup` is expensive: asking for it across all + * 139 open PRs in one GraphQL call returns 502/504 (measured 2026-09-13 at + * limits 150, 200 and 500). Deciding the cheap rules first means only the PRs + * that survive them cost two API calls each, and on a repo where most open PRs + * are already enqueued or human-authored that is most of them avoided. + */ +export function classifyFromListing(pr, { now = Date.now() } = {}) { + const row = (action, reason, detail = null) => ({ + number: pr?.number, + headSha: pr?.headRefOid, + action, + reason, + detail, + }); + + if (!isAllyAppLogin(pr?.author?.login) || pr?.author?.is_bot !== true) { + return row("skip", "human-author", pr?.author?.login ?? ""); + } + + const labels = (pr?.labels ?? []).map((label) => lower(label?.name)); + const optOut = SKIP_LABELS.find((label) => labels.includes(label)); + if (optOut) return row("skip", `label:${optOut}`); + + if (pr?.autoMergeRequest) { + const enabledAt = Date.parse(pr.autoMergeRequest.enabledAt ?? ""); + // An unparseable timestamp must not read as infinitely old: treating it as + // fresh leaves the PR alone, which is the recoverable direction. + const ageHours = Number.isFinite(enabledAt) ? (now - enabledAt) / 3_600_000 : 0; + if (ageHours > STALE_ENQUEUE_HOURS) { + return row("stale-enqueue", `auto-merge armed ${ageHours.toFixed(1)}h ago`); + } + return row("already-enqueued", `auto-merge armed ${ageHours.toFixed(1)}h ago`); + } + + return null; +} + +/** + * One row per PR. Rules are ordered so that the reason reported is the one a + * human would act on first: an explicit opt-out beats a red check, and a red + * check beats a missing review, because fixing the review would not help. + */ +export function classifyPr(pr, { now = Date.now() } = {}) { + const fromListing = classifyFromListing(pr, { now }); + if (fromListing) return fromListing; + + const row = (action, reason, detail = null) => ({ + number: pr?.number, + headSha: pr?.headRefOid, + action, + reason, + detail, + }); + + const failing = failingChecks(pr?.statusCheckRollup); + if (failing.length > 0) { + return row("skip", `checks:${failing[0].split("=")[1]}`, failing.join(", ")); + } + + const { verdict } = allyVerdictAtHead(pr); + if (verdict !== "clean") return row("skip", `review:${verdict}`); + + const owners = unsatisfiedOwners(pr); + if (owners.length > 0) { + return row("codeowner-review-requested", "owner-approval-pending", owners.join(", ")); + } + + const mergeState = String(pr?.mergeStateStatus ?? "UNKNOWN").toUpperCase(); + if (UNLANDABLE_MERGE_STATES.has(mergeState)) return row("skip", `mergestate:${mergeState}`); + + return row("enqueue", `mergestate:${mergeState}`); +} + +/** Classifies every PR and applies the per-fire enqueue cap. */ +export function classifyAll(prs, { now = Date.now(), maxEnqueues = MAX_ENQUEUES_PER_FIRE } = {}) { + let enqueued = 0; + return (prs ?? []).map((pr) => { + const classified = classifyPr(pr, { now }); + if (classified.action !== "enqueue") return classified; + if (enqueued >= maxEnqueues) { + return { ...classified, action: "skip", reason: `cap:${maxEnqueues}-per-fire` }; + } + enqueued += 1; + return classified; + }); +} + +/** + * A gh failure that means the whole fire is untrustworthy rather than this one + * PR being awkward. Continuing past exhausted rate limits or bad credentials + * would produce a receipt whose `skip:` rows are indistinguishable from real + * verdicts, so the fire aborts and says so. + */ +export function isFatalGhError(message) { + return /rate limit|bad credentials|HTTP 40[13]|could not read (?:Username|Password)/i.test( + String(message ?? ""), + ); +} + +export function renderReceipt(rows) { + const lines = [ + "| PR | action | reason | detail |", + "| --- | --- | --- | --- |", + ...(rows ?? []).map( + (r) => `| #${r.number} | \`${r.action}\` | \`${r.reason}\` | ${r.detail ?? ""} |`, + ), + ]; + const tally = new Map(); + for (const r of rows ?? []) tally.set(r.action, (tally.get(r.action) ?? 0) + 1); + lines.push( + "", + [...tally].sort().map(([action, count]) => `${action}: ${count}`).join(" · ") || + "no open PRs", + ); + return lines.join("\n"); +} + +const PR_LIST_FIELDS = + "number,headRefOid,author,labels,autoMergeRequest,mergeStateStatus,reviewRequests"; + +function gh(args) { + return execFileSync("gh", args, { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 }); +} + +/** + * Lists open PRs, then hydrates checks and reviews only for the ones the cheap + * rules could not decide. `statusCheckRollup` is deliberately absent from the + * list query — including it makes GitHub time out across this repo's open PR + * count — so it is fetched per PR alongside the REST reviews. + * + * Reviews come from REST rather than `gh pr view --json reviews` because the + * identity rules pin immutable REST account IDs, which the GraphQL shape does + * not carry. A login string alone is not review evidence (I5). + */ +function fetchOpenPrs(repo) { + const rows = JSON.parse( + gh(["pr", "list", "--repo", repo, "--state", "open", "--limit", "500", "--json", PR_LIST_FIELDS]), + ); + assertPrListComplete(rows, repo, 500); + return rows.map((row) => { + assertHeadSha(row, repo); + if (classifyFromListing(row)) return row; + return { + ...row, + statusCheckRollup: JSON.parse( + gh(["pr", "view", String(row.number), "--repo", repo, "--json", "statusCheckRollup"]), + ).statusCheckRollup, + reviews: JSON.parse(gh(["api", `repos/${repo}/pulls/${row.number}/reviews`, "--paginate"])), + }; + }); +} + +/** Posts `body` unless a comment carrying `marker` is already on the PR. */ +function commentOnce(repo, number, marker, body) { + const existing = JSON.parse( + gh(["api", `repos/${repo}/issues/${number}/comments`, "--paginate"]), + ); + if (existing.some((comment) => String(comment?.body ?? "").includes(marker))) return "already-posted"; + gh(["pr", "comment", String(number), "--repo", repo, "--body", `${marker}\n${body}`]); + return "posted"; +} + +function applyRow(repo, row) { + if (row.action === "enqueue") { + gh(["pr", "merge", String(row.number), "--repo", repo, "--auto"]); + return "auto-merge armed"; + } + if (row.action === "stale-enqueue") { + gh(["pr", "merge", String(row.number), "--repo", repo, "--disable-auto"]); + return commentOnce( + repo, + row.number, + STALE_ENQUEUE_MARKER, + `Auto-merge had been armed for over ${STALE_ENQUEUE_HOURS}h without landing, so the landing ` + + `routine disarmed it. It will re-arm on the next fire once this PR classifies clean.`, + ); + } + if (row.action === "codeowner-review-requested") { + return commentOnce( + repo, + row.number, + CODEOWNER_REQUEST_MARKER, + `This PR is clean at its current head but still has an outstanding code-owner review ` + + `request (${row.detail}). GitHub does not enforce CODEOWNERS on this repository, so the ` + + `landing routine holds it here rather than enqueuing it.`, + ); + } + return null; +} + +function main() { + const repo = process.env.LAND_CLEAN_PRS_REPO || "Blockcast/paperclip"; + const apply = process.argv.includes("--apply"); + const rows = classifyAll(fetchOpenPrs(repo)); + + for (const row of rows) { + if (!apply) continue; + try { + const outcome = applyRow(repo, row); + if (outcome) row.detail = [row.detail, outcome].filter(Boolean).join(" — "); + } catch (error) { + const message = error?.stderr?.toString() || error?.message || String(error); + if (isFatalGhError(message)) { + row.action = "aborted"; + row.detail = message.trim().split("\n")[0]; + console.log(renderReceipt(rows)); + console.error(`\nAborted the fire: ${row.detail}`); + process.exit(1); + } + row.detail = [row.detail, `failed: ${message.trim().split("\n")[0]}`] + .filter(Boolean) + .join(" — "); + } + } + + console.log(renderReceipt(rows)); + if (!apply) console.log("\n(dry run — pass --apply to act)"); +} + +export function isMainModule(argvPath = process.argv[1], moduleUrl = import.meta.url) { + return Boolean(argvPath) && resolve(argvPath) === fileURLToPath(moduleUrl); +} + +if (isMainModule()) { + main(); +} diff --git a/scripts/land-clean-prs.test.mjs b/scripts/land-clean-prs.test.mjs new file mode 100644 index 000000000000..ff4f6dc433f7 --- /dev/null +++ b/scripts/land-clean-prs.test.mjs @@ -0,0 +1,318 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; + +import { + MAX_ENQUEUES_PER_FIRE, + STALE_ENQUEUE_HOURS, + allyVerdictAtHead, + classifyAll, + classifyFromListing, + classifyPr, + failingChecks, + isFatalGhError, + isMainModule, + latestCheckStates, + unsatisfiedOwners, +} from "./land-clean-prs.mjs"; + +const HEAD = "958587ad1fe1eb52b06ca874c41734c9abdc2a10"; +const OTHER = "b3a240ec8c0108eab7e60c36a5a328c00b3a984d"; +const NOW = Date.parse("2026-09-13T12:00:00Z"); + +const ALLY_APP = { login: "allyblockcast[bot]", id: 290875700, type: "Bot" }; + +/** A canonical Ally body: one heading, one attestation, zero blocking findings. */ +function body(head = HEAD, { critical = 0, important = 0, stillPresent = false } = {}) { + return [ + "## Ally — Consolidated PR Review", + "", + `Reviewed head: ${head}`, + "", + ...(stillPresent ? ["- **prior:abc1234 important 1** — still-present — not fixed", ""] : []), + `### Critical Issues (${critical})`, + "", + `### Important Issues (${important})`, + ].join("\n"); +} + +function review(overrides = {}) { + return { + id: 1, + state: "COMMENTED", + user: ALLY_APP, + commit_id: HEAD, + submitted_at: "2026-09-13T06:00:00Z", + body: body(), + ...overrides, + }; +} + +function pr(overrides = {}) { + return { + number: 1418, + headRefOid: HEAD, + author: { login: "allyblockcast[bot]", is_bot: true }, + labels: [], + autoMergeRequest: null, + mergeStateStatus: "BLOCKED", + statusCheckRollup: [{ name: "verify", conclusion: "SUCCESS", completedAt: "2026-09-13T05:00:00Z" }], + reviewRequests: [], + reviews: [review()], + ...overrides, + }; +} + +const classify = (overrides) => classifyPr(pr(overrides), { now: NOW }); + +describe("classifyPr rule order", () => { + it("skips a human-authored PR before looking at anything else", () => { + const row = classify({ author: { login: "kkroo", is_bot: false }, labels: [{ name: "do-not-merge" }] }); + assert.equal(row.action, "skip"); + assert.equal(row.reason, "human-author"); + }); + + it("skips an opt-out label", () => { + for (const name of ["do-not-merge", "review-gate-override"]) { + const row = classify({ labels: [{ name }] }); + assert.equal(row.action, "skip"); + assert.equal(row.reason, `label:${name}`); + } + }); + + it("reports a fresh auto-merge request as already-enqueued", () => { + const row = classify({ autoMergeRequest: { enabledAt: "2026-09-13T10:00:00Z" } }); + assert.equal(row.action, "already-enqueued"); + }); + + it(`reports an auto-merge request older than ${STALE_ENQUEUE_HOURS}h as stale-enqueue`, () => { + const row = classify({ autoMergeRequest: { enabledAt: "2026-09-12T22:00:00Z" } }); + assert.equal(row.action, "stale-enqueue"); + }); + + it("skips on any check that is not SUCCESS/NEUTRAL/SKIPPED, naming the state", () => { + const row = classify({ + statusCheckRollup: [ + { name: "verify", conclusion: "SUCCESS" }, + { name: "e2e", conclusion: "FAILURE" }, + ], + }); + assert.equal(row.action, "skip"); + assert.equal(row.reason, "checks:FAILURE"); + assert.match(row.detail, /e2e=FAILURE/); + }); + + it("treats a check still in flight as not passing", () => { + const row = classify({ + statusCheckRollup: [{ name: "e2e", conclusion: null, status: "IN_PROGRESS" }], + }); + assert.equal(row.action, "skip"); + assert.equal(row.reason, "checks:IN_PROGRESS"); + }); + + it("holds a clean PR that still has an outstanding code-owner request", () => { + const row = classify({ reviewRequests: [{ login: "kkroo" }] }); + assert.equal(row.action, "codeowner-review-requested"); + assert.equal(row.detail, "kkroo"); + }); + + it("enqueues once the code owner has APPROVED at the current head", () => { + const row = classify({ + reviewRequests: [{ login: "kkroo" }], + reviews: [ + review(), + { id: 2, state: "APPROVED", user: { login: "kkroo", id: 1845185, type: "User" }, commit_id: HEAD, body: "lgtm" }, + ], + }); + assert.equal(row.action, "enqueue"); + }); + + it("skips DIRTY/UNSTABLE/UNKNOWN merge states but enqueues BLOCKED and BEHIND", () => { + for (const state of ["DIRTY", "UNSTABLE", "UNKNOWN"]) { + assert.equal(classify({ mergeStateStatus: state }).reason, `mergestate:${state}`); + assert.equal(classify({ mergeStateStatus: state }).action, "skip"); + } + for (const state of ["BLOCKED", "BEHIND", "CLEAN"]) { + assert.equal(classify({ mergeStateStatus: state }).action, "enqueue"); + } + }); +}); + +describe("Ally verdict selection (BLO-32240)", () => { + it("enqueues when an older blocking review is superseded by a newer clean one at the same head", () => { + const row = classify({ + reviews: [ + review({ id: 5124450619, submitted_at: "2026-09-06T06:28:15Z", body: body(HEAD, { important: 1 }) }), + review({ id: 5125141599, submitted_at: "2026-09-06T11:13:12Z", body: body(HEAD) }), + ], + }); + assert.equal(row.action, "enqueue"); + }); + + it("does NOT enqueue when the newest review at that head is the blocking one", () => { + const row = classify({ + reviews: [ + review({ id: 1, submitted_at: "2026-09-06T06:28:15Z", body: body(HEAD) }), + review({ id: 2, submitted_at: "2026-09-06T11:13:12Z", body: body(HEAD, { important: 1 }) }), + ], + }); + assert.equal(row.action, "skip"); + assert.equal(row.reason, "review:blocking"); + }); + + it("treats a still-present prior disposition as blocking", () => { + const row = classify({ reviews: [review({ body: body(HEAD, { stillPresent: true }) })] }); + assert.equal(row.reason, "review:blocking"); + }); + + it("distinguishes a stale-head review from no review at all", () => { + assert.equal(classify({ reviews: [review({ body: body(OTHER) })] }).reason, "review:stale-head"); + assert.equal(classify({ reviews: [] }).reason, "review:missing"); + }); + + it("ignores DISMISSED and PENDING reviews when selecting the verdict", () => { + const dismissed = classify({ reviews: [review({ state: "DISMISSED" })] }); + assert.equal(dismissed.reason, "review:missing"); + + // A dismissed blocking review must not veto a live clean one. + const row = classify({ + reviews: [ + review({ id: 1, state: "DISMISSED", submitted_at: "2026-09-06T12:00:00Z", body: body(HEAD, { critical: 2 }) }), + review({ id: 2, submitted_at: "2026-09-06T06:00:00Z", body: body(HEAD) }), + ], + }); + assert.equal(row.action, "enqueue"); + }); + + it("reads the attestation from the body, not from commit_id", () => { + // A force-push re-anchored commit_id to the current head while the body + // still attests the tree that was actually read. + const row = classify({ reviews: [review({ commit_id: HEAD, body: body(OTHER) })] }); + assert.equal(row.reason, "review:stale-head"); + + // And the converse: commit_id is stale but the body attests this head. + assert.equal(allyVerdictAtHead(pr({ reviews: [review({ commit_id: OTHER })] })).verdict, "clean"); + }); + + it("rejects a body carrying more than one attestation as non-canonical", () => { + const forged = `${body(HEAD)}\nReviewed head: ${OTHER}`; + assert.equal(classify({ reviews: [review({ body: forged })] }).reason, "review:stale-head"); + }); + + it("ignores reviews from identities that are not the Ally App", () => { + const seat = review({ user: { login: "allyblockcast", id: 296676656, type: "User" } }); + assert.equal(classify({ reviews: [seat] }).reason, "review:missing"); + }); +}); + +describe("per-fire cap", () => { + it(`enqueues at most ${MAX_ENQUEUES_PER_FIRE} PRs and names the deferred ones`, () => { + const prs = Array.from({ length: MAX_ENQUEUES_PER_FIRE + 3 }, (_, i) => pr({ number: 100 + i })); + const rows = classifyAll(prs, { now: NOW }); + assert.equal(rows.filter((r) => r.action === "enqueue").length, MAX_ENQUEUES_PER_FIRE); + const deferred = rows.filter((r) => r.reason === `cap:${MAX_ENQUEUES_PER_FIRE}-per-fire`); + assert.equal(deferred.length, 3); + assert.equal(rows.length, prs.length, "every PR gets exactly one row"); + }); + + it("does not spend cap on PRs that were not enqueueable anyway", () => { + const rows = classifyAll([pr({ number: 1, mergeStateStatus: "DIRTY" }), pr({ number: 2 })], { + now: NOW, + maxEnqueues: 1, + }); + assert.equal(rows[1].action, "enqueue"); + }); +}); + +describe("Ally verdict-mirror statuses are not CI checks", () => { + // Measured on #1681 @c57fafa0: newest attesting review clean, gate red. + const ALLY_RED = [ + { context: "gate/ally-comment-findings", state: "FAILURE" }, + { context: "review/ally-comment", state: "FAILURE" }, + { context: "review/ally-complete", state: "FAILURE" }, + ]; + + it("does not let a stale Ally status veto a clean review", () => { + const row = classify({ statusCheckRollup: [{ name: "verify", conclusion: "SUCCESS" }, ...ALLY_RED] }); + assert.equal(row.action, "enqueue", "a red Ally mirror must not block a PR its review says is clean"); + assert.deepEqual(failingChecks(ALLY_RED), []); + }); + + it("still blocks on the review itself, so the verdict is not lost", () => { + const row = classify({ + statusCheckRollup: [{ name: "verify", conclusion: "SUCCESS" }, ...ALLY_RED], + reviews: [review({ body: body(HEAD, { important: 1 }) })], + }); + assert.equal(row.reason, "review:blocking"); + }); + + it("keeps the bare `review` quality gate and Ally-named check-runs as real checks", () => { + assert.deepEqual(failingChecks([{ context: "review", state: "FAILURE" }]), ["review=FAILURE"]); + assert.deepEqual(failingChecks([{ name: "Ally review gate", conclusion: "FAILURE" }]), [ + "Ally review gate=FAILURE", + ]); + }); +}); + +describe("helpers", () => { + it("decides the cheap rules from the listing alone and defers the rest", () => { + // These three need no checks and no reviews, so the fetcher must not pay + // two API calls for them. + const listing = (overrides) => classifyFromListing(pr(overrides), { now: NOW }); + assert.equal(listing({ author: { login: "kkroo", is_bot: false } }).reason, "human-author"); + assert.equal(listing({ labels: [{ name: "do-not-merge" }] }).reason, "label:do-not-merge"); + assert.equal( + listing({ autoMergeRequest: { enabledAt: "2026-09-13T10:00:00Z" } }).action, + "already-enqueued", + ); + + // Anything else is undecided until its checks and reviews are fetched. + assert.equal(listing({}), null); + + // And the deferred verdict must not change when routed through classifyPr. + assert.equal(classifyPr(pr({ labels: [{ name: "do-not-merge" }] }), { now: NOW }).reason, "label:do-not-merge"); + }); + + it("keeps only the newest attempt per check name", () => { const states = latestCheckStates([ + { name: "verify", conclusion: "FAILURE", completedAt: "2026-09-13T05:00:00Z" }, + { name: "verify", conclusion: "SUCCESS", completedAt: "2026-09-13T06:00:00Z" }, + ]); + assert.deepEqual([...states], [["verify", "SUCCESS"]]); + assert.deepEqual(failingChecks([{ name: "lint", state: "SUCCESS" }]), []); + }); + + it("counts a team review request as never satisfied by a login approval", () => { + assert.deepEqual( + unsatisfiedOwners({ + headRefOid: HEAD, + reviewRequests: [{ slug: "reviewers" }], + reviews: [{ state: "APPROVED", user: { login: "kkroo" }, commit_id: HEAD }], + }), + ["team:reviewers"], + ); + }); + + it("does not credit an approval recorded against a different head", () => { + assert.deepEqual( + unsatisfiedOwners({ + headRefOid: HEAD, + reviewRequests: [{ login: "kkroo" }], + reviews: [{ state: "APPROVED", user: { login: "kkroo" }, commit_id: OTHER }], + }), + ["kkroo"], + ); + }); + + it("classifies rate-limit and auth failures as fatal to the fire", () => { + assert.ok(isFatalGhError("API rate limit exceeded for installation")); + assert.ok(isFatalGhError("HTTP 401: Bad credentials")); + assert.ok(!isFatalGhError("Pull request is in the merge queue")); + }); + + it("does not run main() when imported", () => { + // Passing `undefined` would re-trigger the parameter default (process.argv[1]), + // so name the paths explicitly. + assert.equal(isMainModule("", "file:///tmp/land-clean-prs.mjs"), false); + assert.equal(isMainModule("/tmp/other.mjs", "file:///tmp/land-clean-prs.mjs"), false); + assert.equal(isMainModule("/tmp/land-clean-prs.mjs", "file:///tmp/land-clean-prs.mjs"), true); + }); +}); From e9d085de0b22d9af1965299ffc178db44626034d Mon Sep 17 00:00:00 2001 From: CTO Date: Sun, 13 Sep 2026 13:59:44 +0000 Subject: [PATCH 2/3] fix(scripts): key Ally status exclusion on __typename, not name 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 --- scripts/land-clean-prs.mjs | 28 ++++++++++++++++++----- scripts/land-clean-prs.test.mjs | 40 ++++++++++++++++++++++++++++----- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/scripts/land-clean-prs.mjs b/scripts/land-clean-prs.mjs index 3f9ff442bc43..8a5d1a524bdd 100644 --- a/scripts/land-clean-prs.mjs +++ b/scripts/land-clean-prs.mjs @@ -113,9 +113,11 @@ const PASSING_CHECK_STATES = new Set(["SUCCESS", "NEUTRAL", "SKIPPED"]); * Deliberately narrow. It matches only the `review/ally-*` and `gate/ally-*` * legacy-status namespace. The bare `review` context is the PR-quality gate and * is a real check; any Ally-named *check-run* is the workflow that publishes - * the status, and is also a real check. Neither is excluded. Verified on that - * head: the Ally verdict exists only as commit statuses, with no check-run of - * that name, so the namespace boundary is exact. + * the status, and is also a real check. Neither is excluded — and that is + * enforced by `isAllyVerdictStatus` below rather than assumed, because the + * names are only unambiguous while no check-run happens to share one. + * Measured on #1821 @5cc6a70e: all three Ally rows are `StatusContext`, so the + * name-only reading was load-bearing and untyped. * * This removes a duplicate reading, not a gate. `allyVerdictAtHead` still * blocks on Critical/Important findings and on a still-present prior @@ -167,9 +169,25 @@ export function latestCheckStates(rollup) { return new Map([...latest].map(([name, { state }]) => [name, state])); } +/** + * A legacy commit status in the Ally verdict-mirror namespace. + * + * Keyed on `__typename`, not on the name alone: `statusCheckRollup` unions + * `CheckRun` and `StatusContext`, so a name-only predicate would also swallow a + * genuinely failing *check-run* that happened to carry one of these names — + * contradicting the comment above and letting a PR enqueue past a red required + * check. Fails safe: if `__typename` is ever absent the row is treated as a + * real check, which can only over-hold, never over-enqueue. + */ +function isAllyVerdictStatus(context) { + return ( + context?.__typename === "StatusContext" && + ALLY_VERDICT_STATUS_RE.test(String(context?.context ?? "")) + ); +} + export function failingChecks(rollup) { - return [...latestCheckStates(rollup)] - .filter(([name]) => !ALLY_VERDICT_STATUS_RE.test(name)) + return [...latestCheckStates((rollup ?? []).filter((c) => !isAllyVerdictStatus(c)))] .filter(([, state]) => !PASSING_CHECK_STATES.has(state)) .map(([name, state]) => `${name}=${state}`); } diff --git a/scripts/land-clean-prs.test.mjs b/scripts/land-clean-prs.test.mjs index ff4f6dc433f7..884b3d67a7ff 100644 --- a/scripts/land-clean-prs.test.mjs +++ b/scripts/land-clean-prs.test.mjs @@ -225,10 +225,12 @@ describe("per-fire cap", () => { describe("Ally verdict-mirror statuses are not CI checks", () => { // Measured on #1681 @c57fafa0: newest attesting review clean, gate red. + // Shape mirrors `gh pr view --json statusCheckRollup`, which unions + // CheckRun and StatusContext and tags every row with __typename. const ALLY_RED = [ - { context: "gate/ally-comment-findings", state: "FAILURE" }, - { context: "review/ally-comment", state: "FAILURE" }, - { context: "review/ally-complete", state: "FAILURE" }, + { __typename: "StatusContext", context: "gate/ally-comment-findings", state: "FAILURE" }, + { __typename: "StatusContext", context: "review/ally-comment", state: "FAILURE" }, + { __typename: "StatusContext", context: "review/ally-complete", state: "FAILURE" }, ]; it("does not let a stale Ally status veto a clean review", () => { @@ -246,9 +248,35 @@ describe("Ally verdict-mirror statuses are not CI checks", () => { }); it("keeps the bare `review` quality gate and Ally-named check-runs as real checks", () => { - assert.deepEqual(failingChecks([{ context: "review", state: "FAILURE" }]), ["review=FAILURE"]); - assert.deepEqual(failingChecks([{ name: "Ally review gate", conclusion: "FAILURE" }]), [ - "Ally review gate=FAILURE", + assert.deepEqual( + failingChecks([{ __typename: "StatusContext", context: "review", state: "FAILURE" }]), + ["review=FAILURE"], + ); + assert.deepEqual( + failingChecks([{ __typename: "CheckRun", name: "Ally review gate", conclusion: "FAILURE" }]), + ["Ally review gate=FAILURE"], + ); + }); + + it("does not let the name exclusion swallow a failing check-run", () => { + // The exclusion is keyed on __typename, not on the name: a CheckRun in the + // Ally namespace is the workflow that publishes the status, and a red one + // is a real failure. Excluding it would enqueue past a red required check. + for (const name of ["review/ally-complete", "gate/ally-comment-findings"]) { + assert.deepEqual(failingChecks([{ __typename: "CheckRun", name, conclusion: "FAILURE" }]), [ + `${name}=FAILURE`, + ]); + assert.equal( + classify({ statusCheckRollup: [{ __typename: "CheckRun", name, conclusion: "FAILURE" }] }) + .reason, + "checks:FAILURE", + ); + } + }); + + it("treats an untyped row as a real check, so the failure is over-hold not over-enqueue", () => { + assert.deepEqual(failingChecks([{ context: "review/ally-complete", state: "FAILURE" }]), [ + "review/ally-complete=FAILURE", ]); }); }); From 1e0881238eb9007872bb105ef4c4166c51459531 Mon Sep 17 00:00:00 2001 From: CTO Date: Sun, 13 Sep 2026 20:15:03 +0000 Subject: [PATCH 3/3] fix(scripts): key check dedup on surface, not name alone 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. --- scripts/land-clean-prs.mjs | 17 ++++++++++++----- scripts/land-clean-prs.test.mjs | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/scripts/land-clean-prs.mjs b/scripts/land-clean-prs.mjs index 8a5d1a524bdd..590ddd7d436b 100644 --- a/scripts/land-clean-prs.mjs +++ b/scripts/land-clean-prs.mjs @@ -143,13 +143,19 @@ function lower(value) { } /** - * The latest state per check name. + * The latest state per check, as `[name, state]` pairs. * * A re-run, or a cancelled-then-superseded run, leaves both attempts hanging * off the same head. Reading them all makes a green PR look red (BLO-32733), - * so the newest row per name wins. `statusCheckRollup` unifies the check-run + * so the newest row per check wins. `statusCheckRollup` unifies the check-run * and legacy commit-status surfaces, which is why this reads one field rather * than two endpoints that are each blind to the other. + * + * "Per check" means per (surface, name), not per name: the union carries + * `CheckRun` and `StatusContext` rows in independent namespaces, so keying on + * the name alone lets a newer green status context shadow an older failing + * check-run of the same name and enqueue past a red required check. Pairs + * rather than a Map for the same reason — a Map cannot hold both. */ export function latestCheckStates(rollup) { const latest = new Map(); @@ -163,10 +169,11 @@ export function latestCheckStates(rollup) { context?.completedAt || context?.startedAt || context?.createdAt || "", ); const stamp = Number.isFinite(at) ? at : 0; - const seen = latest.get(name); - if (!seen || stamp >= seen.stamp) latest.set(name, { state, stamp }); + const key = `${context?.__typename ?? ""}${name}`; + const seen = latest.get(key); + if (!seen || stamp >= seen.stamp) latest.set(key, { name, state, stamp }); } - return new Map([...latest].map(([name, { state }]) => [name, state])); + return [...latest.values()].map(({ name, state }) => [name, state]); } /** diff --git a/scripts/land-clean-prs.test.mjs b/scripts/land-clean-prs.test.mjs index 884b3d67a7ff..79b89c572916 100644 --- a/scripts/land-clean-prs.test.mjs +++ b/scripts/land-clean-prs.test.mjs @@ -308,6 +308,27 @@ describe("helpers", () => { assert.deepEqual(failingChecks([{ name: "lint", state: "SUCCESS" }]), []); }); + it("does not let a green status context shadow a failing check-run of the same name", () => { + // The two surfaces are independent namespaces inside one rollup, so the + // newest row must be picked within each — not across both. + const rollup = [ + { + __typename: "CheckRun", + name: "verify", + conclusion: "FAILURE", + completedAt: "2026-09-13T05:00:00Z", + }, + { + __typename: "StatusContext", + context: "verify", + state: "SUCCESS", + createdAt: "2026-09-13T06:00:00Z", + }, + ]; + assert.deepEqual(failingChecks(rollup), ["verify=FAILURE"]); + assert.equal(classify({ statusCheckRollup: rollup }).reason, "checks:FAILURE"); + }); + it("counts a team review request as never satisfied by a login approval", () => { assert.deepEqual( unsatisfiedOwners({