diff --git a/.github/workflows/comment-review-gate-merge-group.yml b/.github/workflows/comment-review-gate-merge-group.yml new file mode 100644 index 000000000000..64c1529cdaae --- /dev/null +++ b/.github/workflows/comment-review-gate-merge-group.yml @@ -0,0 +1,98 @@ +name: Comment-review gate (merge queue) + +# Writes the comment-shaped Ally review gate's verdict onto the merge-queue +# candidate commit. See scripts/mirror-comment-review-gate-to-merge-group.mjs +# for the full rationale and BLO-26602 for the issue. +# +# The short version: the gate posts its verdict on the PULL REQUEST head, driven +# by `pull_request`, `pull_request_review` and Ally's `issue_comment`. A +# `gh-readonly-queue/*` ref generates none of those, and `merge_group` appears +# zero times in the server -- so nothing has ever written this context on a +# queue head. Until something does, marking the context required would make +# every queue entry wait the full 6h `checkResponseTimeout` and eject, since the +# queue runs ALLGREEN and a merge_group run cannot be re-run. This workflow is +# that missing writer, and it is the prerequisite for the required-check step, +# not a replacement for it. +# +# DELIBERATELY NOT MARKED REQUIRED BY THIS CHANGE. The status this writes gates +# nothing until a human marks the context required in branch protection, which +# is a separate action that should happen only once this has been seen writing +# real verdicts on real queue refs. +# +# That is NOT the same as "changes no merge behaviour", and the distinction +# matters: this is itself a merge_group check, so if the JOB fails the entry is +# ejected under ALLGREEN whatever the status says. Adding a job to the queue is +# adding a way for a queue entry to die. That is precisely why the script fails +# OPEN on everything it can (see its FAILURE POSTURE header) and exits non-zero +# only where no status could be written at all -- the cases where exiting 0 +# would just buy the same ejection six hours later. + +on: + merge_group: + types: + - checks_requested + +permissions: + contents: read + pull-requests: read + statuses: write + +jobs: + mirror: + # NOT arc-merge-queue. That pool is admission-gated to exactly ONE workflow: + # the runner's `merge_queue_job_gate` hook asserts GITHUB_WORKFLOW_REF equals + # `mergeQueueJobGate.expectedWorkflowRef`, pinned in onprem-k8s at + # `arc/arc-merge-queue-values.yaml` to `.github/workflows/pr.yml@refs/heads/master`. + # Any other workflow is refused BEFORE checkout, so no in-repo guard can save + # it. Measured on this PR's own first queue entry (run 34228985576, queue ref + # pr-1719-1efd24d1): "Set up runner" failed with `FATAL: merge-queue runner + # admission refused: workflow ref ... is not the protected queue workflow`, + # and under ALLGREEN that ejected the entry 2m40s after it was staged. + # + # arc-light is where the repo's other merge_group workflow already runs + # (commitperclip-review.yml), and it succeeded on that same queue ref -- so + # this is the precedented pool for a merge_group job that is not pr.yml, + # and it leaves the dedicated queue pool's exclusivity intact rather than + # asking a runner admin to widen it. + runs-on: arc-light + # The job itself is three API calls, but ARC runners start cold and the + # checkout dominates. 10 matches the floor the rest of this repo settled on + # rather than inventing a tighter budget for one job. + timeout-minutes: 10 + steps: + # Checks out master, NOT the queue candidate, and that is deliberate: this + # job holds `statuses: write`, so running the candidate's copy of the + # script would let a diff under review rewrite the thing that reports on + # it. Same posture as commitperclip-review.yml. The values file read for + # the context name comes from master for the same reason. + - name: Checkout base branch (never queue-candidate code) + uses: actions/checkout@v6 + with: + ref: master + + - name: Mirror the gate verdict onto the queue head + env: + GH_TOKEN: ${{ github.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + MERGE_GROUP_HEAD_REF: ${{ github.event.merge_group.head_ref }} + MERGE_GROUP_HEAD_SHA: ${{ github.event.merge_group.head_sha }} + # The checkout above is master, so on the very queue entry that lands + # this change master does not have the script yet: the workflow file + # comes from the queue ref (which carries the diff) while the code + # comes from master (which does not). Without this guard the job would + # exit non-zero, and under `mergingStrategy: ALLGREEN` that ejects the + # entry -- so this change could never merge itself. The guard is not + # only a bootstrap shim: it is the right steady-state behaviour too, + # because deliberately running master's copy means "master has no + # copy" must be a no-op rather than a failure (a revert of the script + # would otherwise wedge the queue). Consequence to expect: the first + # real verdict lands on the NEXT entry queued after this merges, not + # on this one. + run: | + set -euo pipefail + script=./scripts/mirror-comment-review-gate-to-merge-group.mjs + if [ ! -f "$script" ]; then + echo "::notice::$script is not on master yet; nothing to mirror." + exit 0 + fi + node "$script" diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 261e9a607952..e7981139743e 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -364,6 +364,14 @@ jobs: run: node --test ./scripts/check-comment-review-gate-census.test.mjs timeout-minutes: 1 + - name: Test comment-review-gate merge-queue mirror + if: ${{ !cancelled() }} + # Guards the no-`pending` invariant in particular: a pending required + # status on a queue ref waits the full 6h checkResponseTimeout and + # ejects the entry, which is the outage BLO-26602 exists to avoid. + run: node --test ./scripts/mirror-comment-review-gate-to-merge-group.test.mjs + timeout-minutes: 1 + - name: Test CODEOWNERS guard if: ${{ !cancelled() }} run: node --test ./scripts/check-codeowners.test.mjs diff --git a/scripts/mirror-comment-review-gate-to-merge-group.mjs b/scripts/mirror-comment-review-gate-to-merge-group.mjs new file mode 100644 index 000000000000..44cad2925142 --- /dev/null +++ b/scripts/mirror-comment-review-gate-to-merge-group.mjs @@ -0,0 +1,441 @@ +#!/usr/bin/env node + +/** + * Writes the comment-shaped review gate's verdict onto the merge-queue + * candidate commit (BLO-26602). + * + * WHY THIS EXISTS. The gate itself lives in + * `server/src/services/pr-comment-review-gate.ts` and posts its verdict as a + * commit status on the PULL REQUEST head, driven by `pull_request` + * opened/reopened/synchronize, `pull_request_review`, and Ally's + * `issue_comment`. A merge-queue ref (`gh-readonly-queue//pr--`) + * generates NONE of those events, and `merge_group` appears zero times in the + * whole server — so nothing has ever written this context on a queue head. + * Measured 2026-08-19: zero legacy statuses on 5/5 sampled merge_group heads. + * + * That absence is what makes marking the context `required` unsafe today. This + * repo's queue runs `mergingStrategy: ALLGREEN` with + * `checkResponseTimeout: 21600` (6h), so a required context with no writer on + * the queue ref does not fail fast — every entry waits the full six hours, + * times out, and is ejected. A merge_group run cannot be re-run, so each + * ejection costs a full re-stage. This script is the missing writer. + * + * THE ONE INVARIANT THAT MATTERS: this never emits `pending`. A `pending` + * required status on a queue ref reproduces exactly the 6h-timeout stall the + * script exists to prevent, so any verdict that is not decisively blocking is + * mirrored as `success`. The gate is fail-open by construction and deliberately + * so — it observes only the comment surface, and a PR reviewed through a formal + * `pull_request_review` legitimately has no comment to find. Reporting + * non-success on absence would deadlock every formally-reviewed PR, which is + * the route BLO-29711 considered and rejected. Fail-open here is therefore not + * a shortcut; it is the same posture the gate already takes on the PR head. + * + * FAILURE POSTURE, and be precise about it: this script runs as a `merge_group` + * check, so a non-zero exit ejects the queue entry under `mergingStrategy: + * ALLGREEN`, and a merge_group run cannot be re-run. Exiting non-zero therefore + * costs a full re-stage and is never a free "be safe" option. Three classes, + * and every path lands in exactly one: + * + * 1. CANNOT DETERMINE THE CONTEXT (values file unreadable, key present but + * unparseable) or CANNOT ADDRESS THE QUEUE HEAD (workflow did not pass the + * env vars). No status can be written at all, so there is nothing to fail + * open *with*. Fail fast and loudly. Once the context is marked required + * this is strictly better than exiting 0: both end in ejection, but this + * one ejects in seconds with a named cause instead of after the 6h + * `checkResponseTimeout`. + * 2. KEY GENUINELY ABSENT from the values file. That is how the gate is + * switched off, so no-op and exit 0. + * 3. CONTEXT KNOWN, MIRROR FAILED (transient `gh` 5xx or secondary rate limit + * that outlived its retries, malformed API JSON, unparseable queue ref). + * We know what to post under, so post `success` naming the mirror failure. + * That turns an ejection into a visible-but-harmless status, which is the + * same fail-open posture as above and keeps the script strictly no worse + * than today's behaviour. + * + * Every `gh` call — both reads and the write — goes through `withRetry` + * first. That matters most for the READS: they are the only path where a + * one-off blip converts a decisively BLOCKING verdict into `success`, + * whereas a failed write merely costs a re-stage. Class 3 is the floor + * after retries are exhausted, not the first response to a 5xx. + * + * The context name is read from the deployed Helm values rather than hardcoded. + * This issue was itself stranded for weeks because the context was renamed + * (`review/ally-comment` -> `gate/ally-comment-findings`, BLO-29711) while prose + * elsewhere kept naming the retired one, which by then read `success` with a + * retirement pointer — a reassuring string under the old name. Reading the + * value the deployment actually ships closes that RENAME drift. It does not by + * itself close FORMATTING drift, so `readGateContext` accepts every YAML + * spelling of a one-line scalar and hard-fails on anything it cannot read, + * rather than degrading to a silent "gate is off" — see its doc comment. + */ + +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +/** GitHub truncates commit-status descriptions past 140 characters. */ +const MAX_DESCRIPTION = 140; + +const VALUES_PATH = resolve( + dirname(fileURLToPath(import.meta.url)), + "../deploy/helm/paperclip/values.blockcast.yaml", +); + +/** + * GitHub always formats a queue ref as + * `refs/heads/gh-readonly-queue//pr--`. The base branch may + * itself contain slashes, so anchor on the trailing `pr--` rather + * than splitting on `/`. + */ +export function parsePrNumberFromQueueRef(headRef) { + if (typeof headRef !== "string") return null; + const match = /\/pr-(\d+)-[0-9a-f]{7,40}$/.exec(headRef.trim()); + return match ? Number(match[1]) : null; +} + +/** Raised when the values file names the key but we cannot read its value. */ +export class GateContextError extends Error {} + +export const GATE_CONTEXT_KEY = "prCommentReviewGateStatusContext"; + +/** + * A `#` opens a comment in a YAML plain scalar only at the start or after + * whitespace, so `gate/a#b` is a legal one-token value rather than a truncation. + */ +function findPlainCommentStart(text) { + for (let index = 0; index < text.length; index += 1) { + if (text[index] !== "#") continue; + if (index === 0 || /\s/.test(text[index - 1])) return index; + } + return -1; +} + +function parseScalar(rest, lineNumber) { + const where = `${GATE_CONTEXT_KEY} on line ${lineNumber} of the Helm values`; + + const doubleQuoted = /^[ \t]*"((?:[^"\\]|\\.)*)"[ \t]*(?:#.*)?$/.exec(rest); + if (doubleQuoted) { + return doubleQuoted[1].replace(/\\(["\\/nt])/g, (_, ch) => (ch === "n" ? "\n" : ch === "t" ? "\t" : ch)).trim(); + } + + const singleQuoted = /^[ \t]*'((?:[^']|'')*)'[ \t]*(?:#.*)?$/.exec(rest); + if (singleQuoted) return singleQuoted[1].replace(/''/g, "'").trim(); + + const commentStart = findPlainCommentStart(rest); + const plain = (commentStart >= 0 ? rest.slice(0, commentStart) : rest).trim(); + + // `key:` with nothing after it is YAML null. That is a legal way to spell + // "unset", but it is also what a half-finished edit and a next-line scalar + // both look like from here, and guessing wrong silently disables the gate. + if (plain === "") { + throw new GateContextError(`${where} has no value on the same line. Write \`${GATE_CONTEXT_KEY}: ""\` to switch the gate off.`); + } + // Flow collections, anchors, aliases, tags and block scalars are all valid + // YAML and none of them is a status context. Refuse rather than mangle. + if (/^[|>&*![{]/.test(plain)) { + throw new GateContextError(`${where} is not a plain scalar (got \`${plain}\`).`); + } + if (/["']/.test(plain)) { + throw new GateContextError(`${where} has unbalanced quotes (got \`${plain}\`).`); + } + return plain; +} + +/** + * Reads `githubApp.prCommentReviewGateStatusContext` out of the Helm values. + * + * Returns `{ present, context }`. `present: false` means the key is absent, + * which is how the gate is switched off; `context: ""` means it is present and + * deliberately empty, which means the same thing. Throws `GateContextError` + * when the key IS present but its value cannot be read. + * + * That last distinction is the whole point of this function, and the earlier + * single-regex version did not make it. It matched only a double-quoted, + * comment-free, same-line value, so single-quoted, unquoted, and + * `"..." # trailing comment` spellings — all valid YAML, and all things a + * routine reformat of a deploy file produces — returned "" and were routed to + * the deliberate no-op branch. "Switched off" and "I could not read this" are + * not the same fact and must not share an encoding: once the context is marked + * required, the second one silently reproduces the 6h `checkResponseTimeout` + * ejection this script exists to prevent, with a reassuring log line and no + * failure signal. + * + * The risk is not hypothetical. `prReviewGateStatusContext` is UNQUOTED a few + * dozen lines below this key in the same file, so the unquoted spelling is + * already house style here; and this key's neighbour + * `prCommentReviewGateRetiredStatusContexts` is exactly the kind of entry that + * attracts an explanatory trailing comment. + * + * Two occurrences are ambiguous — one of them is presumably under a different + * parent mapping — so that is an error too rather than a first-match guess. + */ +export function readGateContext(valuesText) { + if (typeof valuesText !== "string") { + throw new GateContextError("Helm values were not readable as text."); + } + + const keyPattern = new RegExp(`^\\s*${GATE_CONTEXT_KEY}:(.*)$`); + const hits = []; + valuesText.split("\n").forEach((line, index) => { + const match = keyPattern.exec(line); + if (match) hits.push({ lineNumber: index + 1, rest: match[1] }); + }); + + if (hits.length === 0) return { present: false, context: "" }; + if (hits.length > 1) { + throw new GateContextError( + `${GATE_CONTEXT_KEY} appears ${hits.length} times in the Helm values (lines ${hits.map((h) => h.lineNumber).join(", ")}); refusing to guess which one the deployment ships.`, + ); + } + + return { present: true, context: parseScalar(hits[0].rest, hits[0].lineNumber) }; +} + +/** + * `repos/{o}/{r}/statuses/{sha}` returns every historical write, newest-first + * in practice but not contractually sorted, and the combined-status endpoint + * would collapse history we may want to reason about. Take the newest write for + * the context explicitly. + */ +export function selectLatestStatus(statuses, context) { + if (!Array.isArray(statuses) || !context) return null; + const matching = statuses.filter((s) => s && s.context === context); + if (matching.length === 0) return null; + return matching.reduce((newest, candidate) => + String(candidate.updated_at ?? "") > String(newest.updated_at ?? "") ? candidate : newest, + ); +} + +/** + * Maps the PR-head verdict onto the state this script writes on the queue head. + * + * `failure` and `error` both block, and both mean the gate reached a blocking + * conclusion, so they mirror as `failure`. EVERYTHING else — including a + * missing status and, defensively, `pending` — mirrors as `success`. See the + * no-`pending` invariant in the file header: a queue ref has no second chance, + * so the only two states this MAPPING may produce are "fail fast" and "let it + * through". (That is a claim about the mapping, not about the process: the + * script can still exit non-zero on the class-1 failures listed in the header, + * where no status can be written at all.) + */ +export function mirrorVerdict(status, { prNumber, prHeadSha } = {}) { + const shortSha = typeof prHeadSha === "string" ? prHeadSha.slice(0, 8) : "unknown"; + const prLabel = prNumber ? `#${prNumber}` : "the source PR"; + + if (!status) { + return { + state: "success", + description: truncate( + `No gate verdict on ${prLabel} head ${shortSha}; passing open (gate is fail-open on absence).`, + ), + }; + } + + const blocking = status.state === "failure" || status.state === "error"; + if (blocking) { + return { + state: "failure", + description: truncate( + status.description || `Comment-review gate reported ${status.state} on ${prLabel} head ${shortSha}.`, + ), + }; + } + + if (status.state !== "success") { + return { + state: "success", + description: truncate( + `Gate was '${status.state}' on ${prLabel} head ${shortSha}; passing open rather than stalling the queue.`, + ), + }; + } + + return { + state: "success", + description: truncate(status.description || `Comment-review gate clean on ${prLabel} head ${shortSha}.`), + }; +} + +export function truncate(text, limit = MAX_DESCRIPTION) { + const value = String(text ?? ""); + return value.length <= limit ? value : `${value.slice(0, limit - 1)}…`; +} + +/** + * Class-3 outcome from the file header: the context is known but the mirror + * itself could not run. Passing open under the real context turns what would + * otherwise be an ejection into a visible status a human can act on, which is + * the same fail-open posture the gate takes everywhere else. + */ +export function failOpenVerdict(error) { + const reason = String(error?.message ?? error ?? "unknown error").split("\n")[0]; + return { + state: "success", + description: truncate(`Gate mirror failed; passing open, see the merge-queue job log (${reason}).`), + }; +} + +export function isMainModule(argvPath = process.argv[1], moduleUrl = import.meta.url) { + return Boolean(argvPath) && resolve(argvPath) === fileURLToPath(moduleUrl); +} + +function ghRaw(args) { + return execFileSync("gh", args, { encoding: "utf8", maxBuffer: 32 * 1024 * 1024 }).trim(); +} + +function gh(args) { + return JSON.parse(ghRaw(args)); +} + +function sleepSync(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +/** + * Retries the transient `gh` shapes (5xx, secondary rate limit) before giving + * up. Both the reads and the write need this, for asymmetric reasons: + * + * - the WRITE has no fail-open available — if we cannot post, we cannot + * announce that we cannot post — and giving up costs a re-stage; + * - the READS have a fail-open, and that is exactly the problem. A single + * transient 5xx on either read lands in the class-3 catch and mirrors a + * genuinely BLOCKING verdict as `success`. Giving up on a read therefore + * costs strictly more than giving up on the write: a defeated gate rather + * than a delayed one. + * + * This narrows how often class 3 fires; it does not remove it. Fail-open after + * exhausted retries is still the terminal behaviour, because the alternative on + * a queue ref is an ejection that cannot be re-run. + */ +export function withRetry(label, fn, { attempts = 3, delayMs = 2000 } = {}) { + for (let attempt = 1; ; attempt += 1) { + try { + return fn(); + } catch (error) { + if (attempt >= attempts) throw error; + console.log(`::warning::${label} failed (attempt ${attempt}/${attempts}); retrying.`); + if (delayMs > 0) sleepSync(delayMs * attempt); + } + } +} + +/** + * Exported so a test can assert what actually reaches GitHub. The no-`pending` + * invariant is proven inside `mirrorVerdict`; this is the seam that proves the + * proven verdict is the one posted. + */ +export function buildStatusArgs({ repo, sha, context, verdict }) { + return [ + "api", + "-X", + "POST", + `repos/${repo}/statuses/${sha}`, + "-f", + `state=${verdict.state}`, + "-f", + `context=${context}`, + "-f", + `description=${verdict.description}`, + ]; +} + +function postStatus({ repo, sha, context, verdict, attempts = 3 }) { + withRetry( + `Posting ${context} on ${sha.slice(0, 8)}`, + () => + execFileSync("gh", buildStatusArgs({ repo, sha, context, verdict }), { + encoding: "utf8", + stdio: ["ignore", "ignore", "inherit"], + }), + { attempts }, + ); +} + +/** Everything between "we know the context" and "we know the verdict". */ +function determineVerdict({ repo, headRef, context, attempts = 3 }) { + const prNumber = parsePrNumberFromQueueRef(headRef); + if (!prNumber) { + throw new Error(`could not parse a PR number out of merge_group head_ref ${headRef}`); + } + + const prHeadSha = withRetry( + `Reading head SHA for PR #${prNumber}`, + () => ghRaw(["api", `repos/${repo}/pulls/${prNumber}`, "--jq", ".head.sha"]), + { attempts }, + ); + // Deliberately outside the retry: a well-formed response carrying a malformed + // SHA is not transient, so retrying it only delays the same failure. + if (!/^[0-9a-f]{40}$/.test(prHeadSha)) { + throw new Error(`unexpected head SHA for PR #${prNumber}: ${prHeadSha}`); + } + + const statuses = withRetry( + `Reading statuses for ${prHeadSha.slice(0, 8)}`, + () => gh(["api", `repos/${repo}/statuses/${prHeadSha}`, "--paginate"]), + { attempts }, + ); + return { + prNumber, + prHeadSha, + verdict: mirrorVerdict(selectLatestStatus(statuses, context), { prNumber, prHeadSha }), + }; +} + +function main() { + const repo = process.env.GITHUB_REPOSITORY; + const headRef = process.env.MERGE_GROUP_HEAD_REF; + const headSha = process.env.MERGE_GROUP_HEAD_SHA; + + // Class 1: without these we cannot address the queue head, so there is no + // status to fail open with. Fail fast rather than after the 6h timeout. + if (!repo || !headRef || !headSha) { + console.error("::error::GITHUB_REPOSITORY, MERGE_GROUP_HEAD_REF and MERGE_GROUP_HEAD_SHA are all required."); + process.exit(1); + } + + let resolved; + try { + resolved = readGateContext(readFileSync(VALUES_PATH, "utf8")); + } catch (error) { + // Class 1 again: the values file is gone, or names the key in a spelling we + // refuse to guess at. Either way we do not know what context to post under. + console.error(`::error::Cannot determine the comment-review gate context from ${VALUES_PATH}: ${error.message}`); + process.exit(1); + } + + if (!resolved.present || !resolved.context) { + // Class 2. An absent or deliberately-empty key is how the gate is switched + // off; writing nothing is the correct no-op. Writing a placeholder would + // create a status the repo would then have to live with, since commit + // statuses cannot be deleted. + console.log("Comment-review gate context is not set in Helm values; nothing to mirror."); + return; + } + + const { context } = resolved; + let outcome; + try { + outcome = determineVerdict({ repo, headRef, context }); + } catch (error) { + // Class 3: context known, mirror failed. Say so under the real context. + console.log(`::warning::Could not determine a gate verdict: ${error.message}`); + outcome = { verdict: failOpenVerdict(error) }; + } + + try { + postStatus({ repo, sha: headSha, context, verdict: outcome.verdict }); + } catch (error) { + console.error(`::error::Could not post ${context} on queue head ${headSha.slice(0, 8)}: ${error.message}`); + process.exit(1); + } + + const source = outcome.prNumber + ? `from PR #${outcome.prNumber} head ${outcome.prHeadSha.slice(0, 8)} ` + : ""; + console.log( + `Mirrored ${context}=${outcome.verdict.state} ${source}onto queue head ${headSha.slice(0, 8)}: ${outcome.verdict.description}`, + ); +} + +if (isMainModule()) main(); diff --git a/scripts/mirror-comment-review-gate-to-merge-group.test.mjs b/scripts/mirror-comment-review-gate-to-merge-group.test.mjs new file mode 100644 index 000000000000..3fb12008e771 --- /dev/null +++ b/scripts/mirror-comment-review-gate-to-merge-group.test.mjs @@ -0,0 +1,355 @@ +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { describe, it } from "node:test"; +import { fileURLToPath } from "node:url"; + +import { + GateContextError, + buildStatusArgs, + failOpenVerdict, + isMainModule, + mirrorVerdict, + parsePrNumberFromQueueRef, + readGateContext, + selectLatestStatus, + truncate, + withRetry, +} from "./mirror-comment-review-gate-to-merge-group.mjs"; + +const CONTEXT = "gate/ally-comment-findings"; +const SHA = "a".repeat(40); + +function status(overrides = {}) { + return { + context: CONTEXT, + state: "success", + description: "Ally's most recent consolidated-review comment for this head reports no unresolved findings.", + updated_at: "2026-09-06T00:00:00Z", + ...overrides, + }; +} + +describe("parsePrNumberFromQueueRef", () => { + it("parses the number out of a queue ref", () => { + assert.equal( + parsePrNumberFromQueueRef("refs/heads/gh-readonly-queue/master/pr-1431-dd3cf5e7"), + 1431, + ); + }); + + it("tolerates a base branch containing slashes", () => { + assert.equal( + parsePrNumberFromQueueRef("refs/heads/gh-readonly-queue/release/v2/pr-987-abc1234"), + 987, + ); + }); + + it("returns null for a ref that is not a queue ref", () => { + assert.equal(parsePrNumberFromQueueRef("refs/heads/master"), null); + assert.equal(parsePrNumberFromQueueRef(""), null); + assert.equal(parsePrNumberFromQueueRef(undefined), null); + }); +}); + +describe("readGateContext", () => { + const shipped = (value) => ["githubApp:", ` prCommentReviewGateStatusContext: ${value}`, ""].join("\n"); + + // The bug this replaced a single regex to fix: every spelling below is valid + // YAML and a routine reformat of a deploy file produces them, but only the + // first one used to parse. The rest returned "" and were routed to the + // deliberate no-op branch -- switching the merge-queue writer off with a + // reassuring log line and no failure signal. + it("reads every one-line YAML spelling of the value", () => { + const spellings = [ + `"${CONTEXT}"`, + `'${CONTEXT}'`, + CONTEXT, + `"${CONTEXT}" # BLO-29711`, + `'${CONTEXT}' # BLO-29711`, + `${CONTEXT} # BLO-29711`, + ` ${CONTEXT} `, + ]; + for (const spelling of spellings) { + assert.deepEqual( + readGateContext(shipped(spelling)), + { present: true, context: CONTEXT }, + `failed to read: ${spelling}`, + ); + } + }); + + // Regression guard for the real file rather than a synthetic one. This test + // runs on every PR, so a values-file reformat fails a re-runnable PR check + // instead of a merge-queue entry that cannot be re-run. + it("reads the context out of the values file the deployment actually ships", () => { + const values = readFileSync( + new URL("../deploy/helm/paperclip/values.blockcast.yaml", import.meta.url), + "utf8", + ); + const resolved = readGateContext(values); + assert.equal(resolved.present, true); + assert.ok(resolved.context.length > 0, "shipped values must name a non-empty gate context"); + }); + + it("is not confused by the retired-contexts key sitting next to it", () => { + const values = [ + "githubApp:", + ' prCommentReviewGateStatusContext: "gate/ally-comment-findings"', + ' prCommentReviewGateRetiredStatusContexts: "review/ally-comment"', + "", + ].join("\n"); + assert.equal(readGateContext(values).context, "gate/ally-comment-findings"); + }); + + it("distinguishes the key being absent from the gate being switched off", () => { + assert.deepEqual(readGateContext("githubApp: {}"), { present: false, context: "" }); + assert.deepEqual(readGateContext(shipped('""')), { present: true, context: "" }); + assert.deepEqual(readGateContext(shipped("''")), { present: true, context: "" }); + }); + + // "Switched off" and "I cannot read this" must not share an encoding: once + // the context is required, silently reading the second as the first is the 6h + // checkResponseTimeout ejection this whole script exists to prevent. + it("refuses to guess when the key is present but unreadable", () => { + const unreadable = [ + "", // `key:` with no value -- YAML null, but also a half-finished edit + " ", + '"gate/unterminated', + "[gate/ally-comment-findings]", + "&anchor", + "|", + ]; + for (const value of unreadable) { + assert.throws( + () => readGateContext(shipped(value)), + GateContextError, + `should have refused: ${JSON.stringify(value)}`, + ); + } + }); + + it("refuses to first-match when the key appears twice", () => { + const values = [shipped(`"${CONTEXT}"`), shipped('"gate/somewhere-else"')].join("\n"); + assert.throws(() => readGateContext(values), GateContextError); + }); + + it("keeps a '#' that is not a comment", () => { + assert.equal(readGateContext(shipped("gate/a#b")).context, "gate/a#b"); + }); + + it("treats a commented-out key as absent", () => { + assert.deepEqual( + readGateContext(`githubApp:\n # prCommentReviewGateStatusContext: "${CONTEXT}"\n`), + { present: false, context: "" }, + ); + }); + + it("rejects non-text input rather than reporting the gate as off", () => { + assert.throws(() => readGateContext(undefined), GateContextError); + }); +}); + +describe("failOpenVerdict", () => { + // Class 3 in the script header: the context is known but the mirror failed + // (gh 5xx, secondary rate limit, malformed API JSON). A crash here would + // eject the queue entry, which is neither of the two outcomes the script + // promises, so an unexpected error becomes a visible-but-harmless status. + it("passes open and names the failure", () => { + const verdict = failOpenVerdict(new Error("gh: HTTP 502")); + assert.equal(verdict.state, "success"); + assert.match(verdict.description, /gh: HTTP 502/); + }); + + it("never emits pending, and stays inside GitHub's 140-character limit", () => { + const verdict = failOpenVerdict(new Error("x".repeat(400))); + assert.notEqual(verdict.state, "pending"); + assert.ok(verdict.description.length <= 140, `got ${verdict.description.length}`); + }); + + it("survives a thrown non-Error", () => { + assert.equal(failOpenVerdict(undefined).state, "success"); + }); +}); + +describe("selectLatestStatus", () => { + it("takes the newest write for the context, not the first listed", () => { + const chosen = selectLatestStatus( + [ + status({ state: "failure", updated_at: "2026-09-06T00:00:00Z" }), + status({ state: "success", updated_at: "2026-09-07T00:00:00Z" }), + ], + CONTEXT, + ); + assert.equal(chosen.state, "success"); + }); + + it("ignores other contexts", () => { + assert.equal(selectLatestStatus([status({ context: "review/ally-complete" })], CONTEXT), null); + }); + + it("returns null when the context was never written", () => { + assert.equal(selectLatestStatus([], CONTEXT), null); + }); +}); + +describe("mirrorVerdict", () => { + // The invariant this whole script exists to hold. A `pending` required status + // on a queue ref waits the full 6h `checkResponseTimeout` and ejects the + // entry, and a merge_group run cannot be re-run -- which is the outage + // BLO-26602 correction #1 describes. Nothing may ever mirror as pending. + it("never emits pending, for any input", () => { + const inputs = [ + null, + status({ state: "pending" }), + status({ state: "success" }), + status({ state: "failure" }), + status({ state: "error" }), + status({ state: "something-new" }), + ]; + for (const input of inputs) { + assert.notEqual(mirrorVerdict(input, { prNumber: 1, prHeadSha: SHA }).state, "pending"); + } + }); + + it("mirrors a blocking verdict so the queue entry fails fast", () => { + for (const state of ["failure", "error"]) { + assert.equal(mirrorVerdict(status({ state }), { prNumber: 7, prHeadSha: SHA }).state, "failure"); + } + }); + + it("passes open when the gate never evaluated the PR head", () => { + const verdict = mirrorVerdict(null, { prNumber: 7, prHeadSha: SHA }); + assert.equal(verdict.state, "success"); + assert.match(verdict.description, /No gate verdict on #7/); + }); + + it("passes open rather than stalling on an unexpected state", () => { + const verdict = mirrorVerdict(status({ state: "pending" }), { prNumber: 7, prHeadSha: SHA }); + assert.equal(verdict.state, "success"); + assert.match(verdict.description, /passing open rather than stalling/); + }); + + it("carries the gate's own description through so the queue status is self-explaining", () => { + const verdict = mirrorVerdict( + status({ state: "failure", description: "Ally's review of this head carries an unresolved finding." }), + { prNumber: 7, prHeadSha: SHA }, + ); + assert.equal(verdict.description, "Ally's review of this head carries an unresolved finding."); + }); + + it("keeps every description inside GitHub's 140-character limit", () => { + const verdict = mirrorVerdict(status({ state: "failure", description: "x".repeat(400) }), { + prNumber: 7, + prHeadSha: SHA, + }); + assert.ok(verdict.description.length <= 140, `got ${verdict.description.length}`); + }); +}); + +describe("truncate", () => { + it("leaves short text alone", () => { + assert.equal(truncate("short"), "short"); + }); + + it("marks truncation visibly", () => { + assert.equal(truncate("abcdef", 4), "abc…"); + }); +}); + +describe("withRetry", () => { + const opts = { attempts: 3, delayMs: 0 }; + + it("returns the value without retrying when the call succeeds", () => { + let calls = 0; + const value = withRetry("read", () => { + calls += 1; + return "ok"; + }, opts); + assert.equal(value, "ok"); + assert.equal(calls, 1); + }); + + // The defect this guards: a transient 5xx on a READ used to land in the + // class-3 catch and mirror a blocking verdict as `success`. + it("survives a transient failure and returns the eventual value", () => { + let calls = 0; + const value = withRetry("read", () => { + calls += 1; + if (calls < 3) throw new Error("HTTP 502"); + return "ok"; + }, opts); + assert.equal(value, "ok"); + assert.equal(calls, 3); + }); + + it("rethrows the last error once attempts are exhausted, so class 3 is still the floor", () => { + let calls = 0; + assert.throws( + () => withRetry("read", () => { + calls += 1; + throw new Error(`HTTP 502 #${calls}`); + }, opts), + /HTTP 502 #3/, + ); + assert.equal(calls, 3); + }); +}); + +describe("buildStatusArgs", () => { + function argOf(args, key) { + const index = args.findIndex((arg) => typeof arg === "string" && arg.startsWith(`${key}=`)); + return index === -1 ? null : args[index].slice(key.length + 1); + } + + it("posts to the queue head under the configured context", () => { + const args = buildStatusArgs({ + repo: "Blockcast/paperclip", + sha: SHA, + context: CONTEXT, + verdict: { state: "success", description: "clean" }, + }); + assert.deepEqual(args.slice(0, 4), ["api", "-X", "POST", `repos/Blockcast/paperclip/statuses/${SHA}`]); + assert.equal(argOf(args, "context"), CONTEXT); + }); + + // The no-`pending` invariant is proven inside mirrorVerdict; this proves the + // verdict that was proven is the one that reaches GitHub. + it("carries the verdict's own state through to the wire", () => { + for (const state of ["success", "failure"]) { + const args = buildStatusArgs({ + repo: "o/r", + sha: SHA, + context: CONTEXT, + verdict: { state, description: "d" }, + }); + assert.equal(argOf(args, "state"), state); + } + }); + + it("never puts pending on the wire for any verdict this module produces", () => { + const verdicts = [ + mirrorVerdict(status({ state: "failure" }), { prNumber: 1, prHeadSha: SHA }), + mirrorVerdict(status({ state: "pending" }), { prNumber: 1, prHeadSha: SHA }), + mirrorVerdict(null, { prNumber: 1, prHeadSha: SHA }), + failOpenVerdict(new Error("boom")), + ]; + for (const verdict of verdicts) { + const args = buildStatusArgs({ repo: "o/r", sha: SHA, context: CONTEXT, verdict }); + assert.notEqual(argOf(args, "state"), "pending"); + } + }); +}); + +describe("isMainModule", () => { + it("is false when the entrypoint is a different file (so importing never runs main)", () => { + assert.equal(isMainModule("/some/other/entry.mjs", import.meta.url), false); + }); + + it("is true when the entrypoint is this module", () => { + assert.equal(isMainModule(fileURLToPath(import.meta.url), import.meta.url), true); + }); + + it("is false when there is no entrypoint at all", () => { + assert.equal(isMainModule("", import.meta.url), false); + }); +});