From 8d84abe19e0f3321acdba77df267d974974da03d Mon Sep 17 00:00:00 2001 From: CTO Date: Mon, 7 Sep 2026 23:36:50 +0000 Subject: [PATCH 1/5] feat(ci): write the comment-review gate verdict onto merge-queue heads (BLO-26602) The comment-shaped Ally review gate posts its verdict as a commit status 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 `gate/ally-comment-findings` on a merge-queue head. That absence is the whole reason the context cannot be marked required today: the queue runs ALLGREEN with `checkResponseTimeout: 21600`, so a required context with no writer on the queue ref does not fail fast -- every entry waits six hours, times out and is ejected, and a merge_group run cannot be re-run. This adds the missing writer. The mirror never emits `pending`. A pending required status on a queue ref reproduces exactly that stall, so anything short of a decisively blocking verdict is mirrored as `success`. That matches the gate's existing fail-open posture on the PR head, which is deliberate: the gate observes only the comment surface, and a PR reviewed through a formal `pull_request_review` legitimately has no comment to find. The context name is read from `values.blockcast.yaml` rather than hardcoded. This work was itself stranded for weeks by the `review/ally-comment` -> `gate/ally-comment-findings` rename, after which the retired context kept reading `success` with a retirement pointer -- a reassuring string under the old name. Reading the value the deployment ships makes that drift impossible. Deliberately NOT marked required here. Landing the writer is observe-only and changes no merge behaviour; requiring it is a separate human branch-protection action that should follow evidence of real verdicts on real queue refs. Co-Authored-By: Claude --- .../comment-review-gate-merge-group.yml | 59 +++++ .github/workflows/pr.yml | 8 + ...ror-comment-review-gate-to-merge-group.mjs | 212 ++++++++++++++++++ ...omment-review-gate-to-merge-group.test.mjs | 158 +++++++++++++ 4 files changed, 437 insertions(+) create mode 100644 .github/workflows/comment-review-gate-merge-group.yml create mode 100644 scripts/mirror-comment-review-gate-to-merge-group.mjs create mode 100644 scripts/mirror-comment-review-gate-to-merge-group.test.mjs 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..8db5bcbd5c9a --- /dev/null +++ b/.github/workflows/comment-review-gate-merge-group.yml @@ -0,0 +1,59 @@ +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. Landing the writer is +# observe-only: it adds a status to queue heads and changes no merge behaviour. +# Requiring it is a separate, human, branch-protection action that should happen +# only once this has been seen writing real verdicts on real queue refs. + +on: + merge_group: + types: + - checks_requested + +permissions: + contents: read + pull-requests: read + statuses: write + +jobs: + mirror: + # The merge-queue candidate is the sole landing candidate and must not queue + # behind the pull-request backlog that saturates arc-light -- same reasoning + # as every merge_group job in pr.yml. + runs-on: arc-merge-queue + # 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 }} + run: node ./scripts/mirror-comment-review-gate-to-merge-group.mjs 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..739189ca4e1e --- /dev/null +++ b/scripts/mirror-comment-review-gate-to-merge-group.mjs @@ -0,0 +1,212 @@ +#!/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, and + * it keeps this script strictly no worse than today's behaviour. + * + * 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 makes that class of drift impossible + * here. + */ + +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; +} + +/** Reads `githubApp.prCommentReviewGateStatusContext` out of the Helm values. */ +export function readGateContext(valuesText) { + if (typeof valuesText !== "string") return ""; + const match = /^\s*prCommentReviewGateStatusContext:\s*"([^"]*)"\s*$/m.exec(valuesText); + return match ? match[1].trim() : ""; +} + +/** + * `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 outcomes this may produce are "fail fast" and "let it + * through". + */ +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)}…`; +} + +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 main() { + const repo = process.env.GITHUB_REPOSITORY; + const headRef = process.env.MERGE_GROUP_HEAD_REF; + const headSha = process.env.MERGE_GROUP_HEAD_SHA; + + if (!repo || !headRef || !headSha) { + console.error("::error::GITHUB_REPOSITORY, MERGE_GROUP_HEAD_REF and MERGE_GROUP_HEAD_SHA are all required."); + process.exit(1); + } + + const context = readGateContext(readFileSync(VALUES_PATH, "utf8")); + if (!context) { + // An empty context is how the gate is switched off (values.yaml ships ""). + // 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 empty in Helm values; nothing to mirror."); + return; + } + + const prNumber = parsePrNumberFromQueueRef(headRef); + if (!prNumber) { + console.error(`::error::Could not parse a PR number out of merge_group head_ref: ${headRef}`); + process.exit(1); + } + + const prHeadSha = ghRaw(["api", `repos/${repo}/pulls/${prNumber}`, "--jq", ".head.sha"]); + if (!/^[0-9a-f]{40}$/.test(prHeadSha)) { + console.error(`::error::Unexpected head SHA for PR #${prNumber}: ${prHeadSha}`); + process.exit(1); + } + + const statuses = gh(["api", `repos/${repo}/statuses/${prHeadSha}`, "--paginate"]); + const verdict = mirrorVerdict(selectLatestStatus(statuses, context), { prNumber, prHeadSha }); + + execFileSync( + "gh", + [ + "api", + "-X", + "POST", + `repos/${repo}/statuses/${headSha}`, + "-f", + `state=${verdict.state}`, + "-f", + `context=${context}`, + "-f", + `description=${verdict.description}`, + ], + { encoding: "utf8", stdio: ["ignore", "ignore", "inherit"] }, + ); + + console.log( + `Mirrored ${context}=${verdict.state} from PR #${prNumber} head ${prHeadSha.slice(0, 8)} onto queue head ${headSha.slice(0, 8)}: ${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..609560ab9696 --- /dev/null +++ b/scripts/mirror-comment-review-gate-to-merge-group.test.mjs @@ -0,0 +1,158 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import { fileURLToPath } from "node:url"; + +import { + isMainModule, + mirrorVerdict, + parsePrNumberFromQueueRef, + readGateContext, + selectLatestStatus, + truncate, +} 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", () => { + it("reads the context the deployment actually ships", () => { + const values = ['githubApp:', ` prCommentReviewGateStatusContext: "${CONTEXT}"`, ""].join("\n"); + assert.equal(readGateContext(values), CONTEXT); + }); + + it("treats an empty context as the gate being switched off", () => { + assert.equal(readGateContext(' prCommentReviewGateStatusContext: ""'), ""); + assert.equal(readGateContext("githubApp: {}"), ""); + }); +}); + +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("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); + }); +}); From 24d5206d6787af916b41a4a5ab6f7ad799dc33d4 Mon Sep 17 00:00:00 2001 From: CTO Date: Tue, 8 Sep 2026 02:50:02 +0000 Subject: [PATCH 2/5] fix(ci): don't let the merge-queue gate writer deadlock its own landing (BLO-26602) The mirror step ran `node ./scripts/mirror-...mjs` unconditionally against a checkout of master. On the very queue entry that lands this change that file is not on master yet -- the workflow file comes from the queue ref, which carries the diff, while the code comes from master, which does not. Node would exit MODULE_NOT_FOUND, and under `mergingStrategy: ALLGREEN` a failing check ejects the entry, so the change could never merge itself. Guard the invocation on the script being present. This is not only a bootstrap shim: deliberately running master's copy means "master has no copy" has to be a no-op rather than a failure, or a revert of the script would wedge the queue for every PR. Consequence to expect when reading the acceptance evidence: the first real verdict lands on the NEXT entry queued after this merges, not on this one. Co-Authored-By: Claude --- .../comment-review-gate-merge-group.yml | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/.github/workflows/comment-review-gate-merge-group.yml b/.github/workflows/comment-review-gate-merge-group.yml index 8db5bcbd5c9a..3045e1940ffe 100644 --- a/.github/workflows/comment-review-gate-merge-group.yml +++ b/.github/workflows/comment-review-gate-merge-group.yml @@ -56,4 +56,23 @@ jobs: GITHUB_REPOSITORY: ${{ github.repository }} MERGE_GROUP_HEAD_REF: ${{ github.event.merge_group.head_ref }} MERGE_GROUP_HEAD_SHA: ${{ github.event.merge_group.head_sha }} - run: node ./scripts/mirror-comment-review-gate-to-merge-group.mjs + # 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" From 80ef3bf48075987f035e8e9fcbc99250619b91f0 Mon Sep 17 00:00:00 2001 From: CTO Date: Tue, 8 Sep 2026 05:01:04 +0000 Subject: [PATCH 3/5] fix(ci): read every YAML spelling of the gate context, and never crash the queue (BLO-26602) Two Important findings from Ally's review of #1719, both correct. readGateContext matched only a double-quoted, comment-free, same-line value. Single-quoted, unquoted, and `"..." # trailing comment` are all valid YAML and all returned "", which main() routed to the deliberate no-op branch -- so a routine reformat of a deploy file switched the merge-queue writer off with a reassuring log line and no failure signal. Reproduced against the old regex: of Ally's four forms only the first parsed. The risk is not hypothetical; `prReviewGateStatusContext` is already unquoted in the same file, a few dozen lines below the key being read. Parse each one-line YAML scalar form properly, strip trailing comments (only where YAML actually starts one -- after whitespace, so `gate/a#b` survives), and split the return into `{present, context}`. "Switched off" and "I cannot read this" no longer share an encoding: an absent key is a no-op, a present but unreadable one is a hard error. Once the context is required that hard error is strictly better than a silent no-op -- both end in ejection, but one names the cause in seconds instead of after the 6h checkResponseTimeout. Second: no path in main() was exception-guarded, so readFileSync on a moved values file, a transient `gh` 5xx and a malformed API JSON all crashed with a non-zero exit -- which under ALLGREEN ejects a queue entry that cannot be re-run. Confirmed by running the script with the values file absent: unhandled ENOENT. Once the context is known we can always say something under it, so those now mirror as `success` naming the mirror failure, turning an ejection into a visible-but-harmless status. The status POST itself has no fail-open available, so it retries transient failures before giving up. Ally also caught that the header's two claims could not both hold: if a non-zero exit ejects an entry, the change is not "observe-only ... changes no merge behaviour". Correct, and the observe-only framing was load-bearing for landing this without a branch-protection decision. The status gates nothing until a human marks it required; the JOB can still eject an entry, which is exactly why every recoverable path fails open. Both comments now say so. Verified end to end against all four classes: values absent -> exit 1 with a named error; key unparseable -> exit 1; key absent -> no-op exit 0; `gh` 5xx and unparseable queue ref -> fail-open success posted, exit 0. The new test parses the real shipped values.blockcast.yaml, so a values reformat now fails a re-runnable PR check instead of a queue entry that cannot be re-run. Co-Authored-By: Claude --- .../comment-review-gate-merge-group.yml | 16 +- ...ror-comment-review-gate-to-merge-group.mjs | 275 +++++++++++++++--- ...omment-review-gate-to-merge-group.test.mjs | 123 +++++++- 3 files changed, 359 insertions(+), 55 deletions(-) diff --git a/.github/workflows/comment-review-gate-merge-group.yml b/.github/workflows/comment-review-gate-merge-group.yml index 3045e1940ffe..05c69cf636b9 100644 --- a/.github/workflows/comment-review-gate-merge-group.yml +++ b/.github/workflows/comment-review-gate-merge-group.yml @@ -14,10 +14,18 @@ name: Comment-review gate (merge queue) # 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. Landing the writer is -# observe-only: it adds a status to queue heads and changes no merge behaviour. -# Requiring it is a separate, human, branch-protection action that should happen -# only once this has been seen writing real verdicts on real queue refs. +# 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: diff --git a/scripts/mirror-comment-review-gate-to-merge-group.mjs b/scripts/mirror-comment-review-gate-to-merge-group.mjs index 739189ca4e1e..5571d5786e4b 100644 --- a/scripts/mirror-comment-review-gate-to-merge-group.mjs +++ b/scripts/mirror-comment-review-gate-to-merge-group.mjs @@ -28,16 +28,38 @@ * `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, and - * it keeps this script strictly no worse than today's behaviour. + * 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, secondary rate limit, + * 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. * * 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 makes that class of drift impossible - * here. + * 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"; @@ -65,11 +87,102 @@ export function parsePrNumberFromQueueRef(headRef) { return match ? Number(match[1]) : null; } -/** Reads `githubApp.prCommentReviewGateStatusContext` out of the Helm values. */ +/** 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") return ""; - const match = /^\s*prCommentReviewGateStatusContext:\s*"([^"]*)"\s*$/m.exec(valuesText); - return match ? match[1].trim() : ""; + 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) }; } /** @@ -94,8 +207,10 @@ export function selectLatestStatus(statuses, context) { * 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 outcomes this may produce are "fail fast" and "let it - * through". + * 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"; @@ -140,6 +255,20 @@ export function truncate(text, limit = MAX_DESCRIPTION) { 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 (${reason}); passing open. See the merge-queue job log.`), + }; +} + export function isMainModule(argvPath = process.argv[1], moduleUrl = import.meta.url) { return Boolean(argvPath) && resolve(argvPath) === fileURLToPath(moduleUrl); } @@ -152,60 +281,116 @@ function gh(args) { return JSON.parse(ghRaw(args)); } +function sleepSync(ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +/** + * The one call with no fail-open available: if we cannot post, we cannot + * announce that we cannot post. Retry the transient shapes (`gh` 5xx, secondary + * rate limit) before giving up, since giving up costs a re-stage. + */ +function postStatus({ repo, sha, context, verdict, attempts = 3 }) { + for (let attempt = 1; ; attempt += 1) { + try { + execFileSync( + "gh", + [ + "api", + "-X", + "POST", + `repos/${repo}/statuses/${sha}`, + "-f", + `state=${verdict.state}`, + "-f", + `context=${context}`, + "-f", + `description=${verdict.description}`, + ], + { encoding: "utf8", stdio: ["ignore", "ignore", "inherit"] }, + ); + return; + } catch (error) { + if (attempt >= attempts) throw error; + console.log(`::warning::Posting ${context} on ${sha.slice(0, 8)} failed (attempt ${attempt}/${attempts}); retrying.`); + sleepSync(2000 * attempt); + } + } +} + +/** Everything between "we know the context" and "we know the verdict". */ +function determineVerdict({ repo, headRef, context }) { + const prNumber = parsePrNumberFromQueueRef(headRef); + if (!prNumber) { + throw new Error(`could not parse a PR number out of merge_group head_ref ${headRef}`); + } + + const prHeadSha = ghRaw(["api", `repos/${repo}/pulls/${prNumber}`, "--jq", ".head.sha"]); + if (!/^[0-9a-f]{40}$/.test(prHeadSha)) { + throw new Error(`unexpected head SHA for PR #${prNumber}: ${prHeadSha}`); + } + + const statuses = gh(["api", `repos/${repo}/statuses/${prHeadSha}`, "--paginate"]); + 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); } - const context = readGateContext(readFileSync(VALUES_PATH, "utf8")); - if (!context) { - // An empty context is how the gate is switched off (values.yaml ships ""). - // 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 empty in Helm values; nothing to mirror."); + 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 prNumber = parsePrNumberFromQueueRef(headRef); - if (!prNumber) { - console.error(`::error::Could not parse a PR number out of merge_group head_ref: ${headRef}`); - process.exit(1); + 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) }; } - const prHeadSha = ghRaw(["api", `repos/${repo}/pulls/${prNumber}`, "--jq", ".head.sha"]); - if (!/^[0-9a-f]{40}$/.test(prHeadSha)) { - console.error(`::error::Unexpected head SHA for PR #${prNumber}: ${prHeadSha}`); + 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 statuses = gh(["api", `repos/${repo}/statuses/${prHeadSha}`, "--paginate"]); - const verdict = mirrorVerdict(selectLatestStatus(statuses, context), { prNumber, prHeadSha }); - - execFileSync( - "gh", - [ - "api", - "-X", - "POST", - `repos/${repo}/statuses/${headSha}`, - "-f", - `state=${verdict.state}`, - "-f", - `context=${context}`, - "-f", - `description=${verdict.description}`, - ], - { encoding: "utf8", stdio: ["ignore", "ignore", "inherit"] }, - ); - + const source = outcome.prNumber + ? `from PR #${outcome.prNumber} head ${outcome.prHeadSha.slice(0, 8)} ` + : ""; console.log( - `Mirrored ${context}=${verdict.state} from PR #${prNumber} head ${prHeadSha.slice(0, 8)} onto queue head ${headSha.slice(0, 8)}: ${verdict.description}`, + `Mirrored ${context}=${outcome.verdict.state} ${source}onto queue head ${headSha.slice(0, 8)}: ${outcome.verdict.description}`, ); } diff --git a/scripts/mirror-comment-review-gate-to-merge-group.test.mjs b/scripts/mirror-comment-review-gate-to-merge-group.test.mjs index 609560ab9696..1d7008b02ff9 100644 --- a/scripts/mirror-comment-review-gate-to-merge-group.test.mjs +++ b/scripts/mirror-comment-review-gate-to-merge-group.test.mjs @@ -1,8 +1,11 @@ import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; import { describe, it } from "node:test"; import { fileURLToPath } from "node:url"; import { + GateContextError, + failOpenVerdict, isMainModule, mirrorVerdict, parsePrNumberFromQueueRef, @@ -47,14 +50,122 @@ describe("parsePrNumberFromQueueRef", () => { }); describe("readGateContext", () => { - it("reads the context the deployment actually ships", () => { - const values = ['githubApp:', ` prCommentReviewGateStatusContext: "${CONTEXT}"`, ""].join("\n"); - assert.equal(readGateContext(values), CONTEXT); + 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("treats an empty context as the gate being switched off", () => { - assert.equal(readGateContext(' prCommentReviewGateStatusContext: ""'), ""); - assert.equal(readGateContext("githubApp: {}"), ""); + it("survives a thrown non-Error", () => { + assert.equal(failOpenVerdict(undefined).state, "success"); }); }); From 2ff08042713f550b1876504f80b1435388954743 Mon Sep 17 00:00:00 2001 From: CTO Date: Tue, 8 Sep 2026 07:04:45 +0000 Subject: [PATCH 4/5] fix(ci): retry the gate reads, so a 502 cannot defeat a blocking verdict (BLO-26602) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ally's review of 80ef3bf4 (Important 1): `postStatus` retried the write while the two reads in `determineVerdict` did not, so a single transient 5xx on either landed in the class-3 catch and mirrored a genuinely BLOCKING gate verdict as `success`. The asymmetry was backwards. Giving up on a read costs strictly more than giving up on the write — a defeated gate rather than a delayed one — and once the context is marked required this was the only path that silently drops a real `failure`. Extract the retry loop from `postStatus` into `withRetry` and route both reads through it. Class-3 fail-open stays the terminal behaviour after retries are exhausted; this only narrows how often it fires. The head-SHA shape check stays outside the retry, since a well-formed response carrying a malformed SHA is not transient. Verified with a stub `gh`, pre-fix vs fixed on identical input: one 502 on the head-SHA read -> pre-fix `state=success` (defeated), fixed `state=failure` (preserved) three 502s (retries exhausted) -> `state=success`, exit 0 (class 3 is still the floor) Also exports `buildStatusArgs` so the no-`pending` invariant — proven inside `mirrorVerdict` — is now asserted on what actually reaches GitHub, which was the untested seam Ally flagged. 29 -> 35 tests, all passing. --- ...ror-comment-review-gate-to-merge-group.mjs | 106 +++++++++++++----- ...omment-review-gate-to-merge-group.test.mjs | 86 ++++++++++++++ 2 files changed, 161 insertions(+), 31 deletions(-) diff --git a/scripts/mirror-comment-review-gate-to-merge-group.mjs b/scripts/mirror-comment-review-gate-to-merge-group.mjs index 5571d5786e4b..7ca7cec00dc1 100644 --- a/scripts/mirror-comment-review-gate-to-merge-group.mjs +++ b/scripts/mirror-comment-review-gate-to-merge-group.mjs @@ -45,11 +45,18 @@ * `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, secondary rate limit, - * 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. + * 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 @@ -286,51 +293,88 @@ function sleepSync(ms) { } /** - * The one call with no fail-open available: if we cannot post, we cannot - * announce that we cannot post. Retry the transient shapes (`gh` 5xx, secondary - * rate limit) before giving up, since giving up costs a re-stage. + * 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. */ -function postStatus({ repo, sha, context, verdict, attempts = 3 }) { +export function withRetry(label, fn, { attempts = 3, delayMs = 2000 } = {}) { for (let attempt = 1; ; attempt += 1) { try { - execFileSync( - "gh", - [ - "api", - "-X", - "POST", - `repos/${repo}/statuses/${sha}`, - "-f", - `state=${verdict.state}`, - "-f", - `context=${context}`, - "-f", - `description=${verdict.description}`, - ], - { encoding: "utf8", stdio: ["ignore", "ignore", "inherit"] }, - ); - return; + return fn(); } catch (error) { if (attempt >= attempts) throw error; - console.log(`::warning::Posting ${context} on ${sha.slice(0, 8)} failed (attempt ${attempt}/${attempts}); retrying.`); - sleepSync(2000 * attempt); + 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 }) { +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 = ghRaw(["api", `repos/${repo}/pulls/${prNumber}`, "--jq", ".head.sha"]); + 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 = gh(["api", `repos/${repo}/statuses/${prHeadSha}`, "--paginate"]); + const statuses = withRetry( + `Reading statuses for ${prHeadSha.slice(0, 8)}`, + () => gh(["api", `repos/${repo}/statuses/${prHeadSha}`, "--paginate"]), + { attempts }, + ); return { prNumber, prHeadSha, diff --git a/scripts/mirror-comment-review-gate-to-merge-group.test.mjs b/scripts/mirror-comment-review-gate-to-merge-group.test.mjs index 1d7008b02ff9..3fb12008e771 100644 --- a/scripts/mirror-comment-review-gate-to-merge-group.test.mjs +++ b/scripts/mirror-comment-review-gate-to-merge-group.test.mjs @@ -5,6 +5,7 @@ import { fileURLToPath } from "node:url"; import { GateContextError, + buildStatusArgs, failOpenVerdict, isMainModule, mirrorVerdict, @@ -12,6 +13,7 @@ import { readGateContext, selectLatestStatus, truncate, + withRetry, } from "./mirror-comment-review-gate-to-merge-group.mjs"; const CONTEXT = "gate/ally-comment-findings"; @@ -254,6 +256,90 @@ describe("truncate", () => { }); }); +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); From 5501264a44b603e54e32a44559f40dc134c2a77b Mon Sep 17 00:00:00 2001 From: CTO Date: Tue, 8 Sep 2026 21:27:16 +0000 Subject: [PATCH 5/5] fix(ci): run the queue mirror on arc-light, so it is admitted at all (BLO-26602) The first queue entry for this PR was ejected 2m40s after staging. The cause was not the script and not the bootstrap guard: the `arc-merge-queue` 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 (`arc/arc-merge-queue-values.yaml`) to `.github/workflows/pr.yml@refs/heads/master`. Anything else is refused before checkout, so no in-repo guard could ever have helped. Measured: run 34228985576 on queue ref pr-1719-1efd24d1 failed at "Set up runner" with `FATAL: merge-queue runner admission refused: workflow ref ... is not the protected queue workflow at the merge-group head ref`, and ALLGREEN ejected the entry. Move to arc-light, where this repo's other merge_group workflow (commitperclip-review.yml) already runs -- it succeeded on that same queue ref. That keeps the dedicated queue pool's exclusivity intact instead of asking a runner admin to widen it. Also takes Ally's suggestion 1 from the review at 2ff08042: put the log pointer before the reason in the class-3 fail-open description, so the 140-char truncation eats the diagnostic tail rather than the actionable half. Verified on the `gh`-failure shape that dominates the class -- "see the merge-queue job log" now survives. Tests 35/35 green. Co-Authored-By: Claude --- .../comment-review-gate-merge-group.yml | 20 +++++++++++++++---- ...ror-comment-review-gate-to-merge-group.mjs | 2 +- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/.github/workflows/comment-review-gate-merge-group.yml b/.github/workflows/comment-review-gate-merge-group.yml index 05c69cf636b9..64c1529cdaae 100644 --- a/.github/workflows/comment-review-gate-merge-group.yml +++ b/.github/workflows/comment-review-gate-merge-group.yml @@ -39,10 +39,22 @@ permissions: jobs: mirror: - # The merge-queue candidate is the sole landing candidate and must not queue - # behind the pull-request backlog that saturates arc-light -- same reasoning - # as every merge_group job in pr.yml. - runs-on: arc-merge-queue + # 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. diff --git a/scripts/mirror-comment-review-gate-to-merge-group.mjs b/scripts/mirror-comment-review-gate-to-merge-group.mjs index 7ca7cec00dc1..44cad2925142 100644 --- a/scripts/mirror-comment-review-gate-to-merge-group.mjs +++ b/scripts/mirror-comment-review-gate-to-merge-group.mjs @@ -272,7 +272,7 @@ export function failOpenVerdict(error) { const reason = String(error?.message ?? error ?? "unknown error").split("\n")[0]; return { state: "success", - description: truncate(`Gate mirror failed (${reason}); passing open. See the merge-queue job log.`), + description: truncate(`Gate mirror failed; passing open, see the merge-queue job log (${reason}).`), }; }