feat(ci): write the comment-review gate verdict onto merge-queue heads (BLO-26602) - #1719
feat(ci): write the comment-review gate verdict onto merge-queue heads (BLO-26602)#1719allyblockcast[bot] wants to merge 5 commits into
Conversation
…s (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 <noreply@anthropic.com>
…ng (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 <noreply@anthropic.com>
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 24d5206
Critical Issues (0)
Important Issues (2)
-
[gstack/review]
scripts/mirror-comment-review-gate-to-merge-group.mjs:71—readGateContextonly matches a double-quoted, comment-free, same-line value, and every other valid YAML spelling silently degrades to "gate switched off".Verified by running the exported function at this head against four forms:
form result prCommentReviewGateStatusContext: "gate/ally-comment-findings"(current)"gate/ally-comment-findings"single-quoted ""unquoted (valid YAML) ""double-quoted + trailing # BLO-29711comment""An empty result is routed at
:171to the deliberate no-op branch, so a routine reformat of a deploy file turns the writer off with a reassuring log line and no failure signal. The two states are indistinguishable, and the person editing Helm values has no reason to suspect they are touching the merge queue. Once the context is marked required — the stated next step — that silent no-op is precisely the 6hcheckResponseTimeoutejection this script exists to prevent.This is also narrower than the header's own claim at
:38-44, that reading the shipped value "makes that class of drift impossible here": it closes rename drift but opens formatting drift in its place.- Parse the YAML rather than regexing it, or at minimum accept single-quoted/unquoted values and strip trailing comments — and distinguish key absent (off → no-op) from key present but unparseable (hard error). The trailing-comment case is the most likely to bite, since the sibling
prCommentReviewGateRetiredStatusContextsline is exactly the kind of entry that attracts an explanatory comment.
- Parse the YAML rather than regexing it, or at minimum accept single-quoted/unquoted values and strip trailing comments — and distinguish key absent (off → no-op) from key present but unparseable (hard error). The trailing-comment case is the most likely to bite, since the sibling
-
[native-codex]
scripts/mirror-comment-review-gate-to-merge-group.mjs:165— the workflow guards the script's absence on master but not the values file's, and no path inmain()is exception-guarded, so a third outcome exists that the documented contract says cannot.readFileSync(VALUES_PATH, …)throws uncaught on ENOENT — confirmed by running the script with the values file absent: unhandledError: ENOENT … values.blockcast.yaml, non-zero exit. The workflow's[ ! -f "$script" ]guard (.github/workflows/comment-review-gate-merge-group.yml:74) does not cover it, and nobody renaming a Helm values file would expect to wedge the merge queue. The same applies to a transientghnon-zero at:181,:187and:190(5xx / secondary rate limit) and toJSON.parseat:150— the API-call paths are the more frequent trigger; the values file is the more surprising one.This contradicts the stated invariant at
:31-36that "the only two outcomes this may produce are 'fail fast' and 'let it through'". A crash is neither, and by the workflow's own model at:60-67— "the job would exit non-zero, and undermergingStrategy: ALLGREENthat ejects the entry" — each crash costs a full re-stage of an entry that cannot be re-run.Note the header's two claims cannot both hold: if a non-zero exit ejects an entry (the premise the bootstrap guard is built on), then the change is not "observe-only … changes no merge behaviour" (
:16-17) — it introduces a new way for a queue entry to die. Worth settling which is true before the required-check step, since the observe-only framing is what justifies landing this without a branch-protection decision.- Wrap
main()so any unexpected error mirrors assuccesswith a description naming the mirror failure. That is consistent with the fail-open posture already argued for on line:31, keeps the script "strictly no worse than today's behaviour" as the header claims, and turns an ejection into a visible-but-harmless status.
- Wrap
Suggestions (2)
-
[pr-review-toolkit/code]
.github/workflows/comment-review-gate-merge-group.yml:51—ref: masteris hardcoded while the workflow has nobranches:filter andparsePrNumberFromQueueRefgoes out of its way to handle a base containing slashes (refs/heads/gh-readonly-queue/release/v2/pr-987-…, tested).github.event.merge_group.base_refis the protected target branch, not candidate code, so it preserves the security posture the comment argues for while being correct for any queued base. No impact today — this repo only queuesmaster— but the parser shows non-master bases were anticipated. -
[pr-review-toolkit/tests]
scripts/mirror-comment-review-gate-to-merge-group.test.mjs:1— every exported pure function has coverage (19/19 pass at this head, run locally), butmain()has none: not the env-var validation, not the empty-context no-op, and not that the POST at.mjs:190actually carriesmirrorVerdict's state. The no-pendinginvariant is proven inmirrorVerdictand not proven to reach GitHub. Extracting the POST argv into a small exported builder would make the wiring assertable without a network call. Relatedly, the status is posted with notarget_url— pointing it at the source PR would make a queue-head status self-diagnosing.
Strengths
selectLatestStatusexplicitly takes the newest write rather than trusting response order, and this is not theoretical caution: the PR head at this SHA currently carries duplicate writes of bothgate/ally-comment-findingsand the retiredreview/ally-comment.- The no-
pendinginvariant is stated, justified against the concrete 6h/ALLGREEN mechanism, and tested across every state including an unknown one — the right shape for an invariant whose violation is silent. - Reading the context from shipped Helm values instead of hardcoding is the correct response to the BLO-29711 rename, even though the parse needs widening.
- Checking out
masterrather than the queue candidate while holdingstatuses: writeis the right call, and the comment explains why rather than just what. - The header's factual claims check out against the live ruleset:
grouping_strategy: ALLGREEN,check_response_timeout_minutes: 360.max_entries_to_build: 1also means each merge group carries exactly one PR, so the single-PR assumption inparsePrNumberFromQueueRefis sound — worth a comment, since raising that cap later would silently pass unchecked entries. isMainModuleso importing under test never runsmain()— and it is itself tested.
Recommended Action
- Widen the values parse and separate "off" from "unparseable" (Important 1) — this is the one that fails silently, and silence is the failure mode the required-check step cannot tolerate.
- Guard
main()so an unexpected error mirrors fail-open rather than crashing (Important 2), and reconcile the observe-only claim with the ejection premise before marking the context required. - Consider the suggestions opportunistically.
Neither Important finding blocks the thesis of this change — the writer is the right missing piece and the fail-open posture is well argued. Both are about the paths where it stops being fail-open.
…h 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 80ef3bf
Prior Findings Dispositioned (2)
-
prior:24d5206 important 1 — fixed —
scripts/mirror-comment-review-gate-to-merge-group.mjs:107—parseScalarreplaces the single double-quote-only regex. Re-ran the exportedreadGateContextat this head against the same four forms that failed before, plus five more:form before at this head : "gate/ally-comment-findings"✅ ✅ single-quoted ""✅ unquoted ""✅ double-quoted + trailing # BLO-29711""✅ unquoted + trailing comment — ✅ gate/a#b(hash is not a comment)— ✅ preserved key absent — {present:false}key:with no value /[a,b]/&anchor/|— GateContextErrorkey appears twice — GateContextErrorcommented-out key — {present:false}The absent-vs-unparseable distinction the finding asked for is now carried by
{present}plus a dedicated error type, and routed separately at:363(class 2 no-op) and:356(class 1 hard-fail).findPlainCommentStartat:99correctly implements the YAML rule that#opens a comment only at start-of-token. -
prior:24d5206 important 2 — fixed —
scripts/mirror-comment-review-gate-to-merge-group.mjs:354— every path inmain()is now guarded and the header contract at:33-52was rewritten to describe three outcome classes rather than two. Verified by execution at this head:- values file absent →
::error::Cannot determine the comment-review gate context from …: ENOENT, exit 1. Previously an uncaughtErrorand a stack trace. - env vars absent → named
::error::, exit 1 (:348). - key absent →
nothing to mirror, exit 0 (:368). ghread fails 502 with the context known →::warning::, thenPOST … -f state=success -f context=gate/ally-comment-findingscarryingGate mirror failed (…); passing open, exit 0 (:374-380).
The finding's second half — that the header's "observe-only … changes no merge behaviour" claim could not coexist with the ejection premise — is also resolved rather than papered over: that sentence is gone, and
.github/workflows/comment-review-gate-merge-group.yml:22-28now states the opposite explicitly ("Adding a job to the queue is adding a way for a queue entry to die"). The class-1 choice to exit non-zero instead of failing open is a different answer from the one I suggested, and it is the better one — you cannot post a fail-open status under a context you were unable to read. - values file absent →
Critical Issues (0)
Important Issues (1)
-
[native-codex]
scripts/mirror-comment-review-gate-to-merge-group.mjs:328— the two reads indetermineVerdictare not retried, while the write inpostStatusis. A single transient 5xx therefore converts a blocking gate verdict intosuccess.postStatusretries three times with backoff (:293-318), on the stated reasoning that "giving up costs a re-stage". The reads at:328(repos/{o}/{r}/pulls/{n}) and:333(repos/{o}/{r}/statuses/{sha}) callghRaw/ghdirectly with no equivalent, so a 502 or secondary-rate-limit on either lands in the class-3 catch at:376and passes open. Confirmed at this head with a stubghthat fails reads: the script posted-f state=success -f context=gate/ally-comment-findings -f description=Gate mirror failed (Command failed: gh api repos/…/pulls/1719 --jq .head.sha); passing open.For class 3 as the header defines it — "unparseable queue ref", "malformed API JSON" — passing open is right, because a retry cannot help. A transient 5xx is the one member of that class where a retry can, and it is also the most frequent. Once the context is marked required, this is the only path that silently drops a real
failure: the PR head here currently carriesgate/ally-comment-findings=failure, and one blip on the status read would mirror that as a pass with a description nobody is required to read.Giving up on the read costs strictly more than giving up on the write — a re-stage versus a defeated gate — so if either call deserves the retry it is this one.
- Route the two reads through the same retry as
postStatus(extract the loop, or callghRaw/ghthrough a smallwithRetry). Keep the class-3 fail-open as the terminal behaviour after retries are exhausted; this only narrows how often it fires.
- Route the two reads through the same retry as
Suggestions (3)
- [pr-review-toolkit/tests]
scripts/mirror-comment-review-gate-to-merge-group.test.mjs:1— 29/29 pass at this head (up from 19), and the regression test at:82reading the real shipped values file is the strongest addition: a deploy-file reformat now fails a re-runnable PR check rather than a merge-queue entry that cannot be re-run. Still uncovered:main()itself — the class-1/2/3 routing, the env-var validation, and that the POST at:296actually carriesmirrorVerdict's state. The no-pendinginvariant is proven inmirrorVerdictand still not proven to reach GitHub. Extracting thegh apiargv into an exported builder would make that assertable without a network call, and would have let the class-3 behaviour I had to verify with a stubghbe a unit test instead. - [pr-review-toolkit/code]
scripts/mirror-comment-review-gate-to-merge-group.mjs:296— the status is still posted with notarget_url. Pointing it at the source PR would make a queue-head status self-diagnosing, which matters most in exactly the fail-open cases where the description ends with "See the merge-queue job log" and the reader has no link to it. - [gstack/review]
.github/workflows/comment-review-gate-merge-group.yml:59—ref: masterremains hardcoded while the workflow has nobranches:filter andparsePrNumberFromQueueRefgoes out of its way to handle a base containing slashes (tested at:38).github.event.merge_group.base_refis the protected target branch, not candidate code, so it preserves the security posture the comment at:51-55argues for while being correct for any queued base. No impact today — onlymasteris queued.
Strengths
- Both prior findings were answered at the mechanism rather than the symptom.
readGateContextdid not merely gain three more regexes; it gained a return shape ({present, context}) and an error type that make "off" and "unreadable" structurally unable to share an encoding — which was the actual defect. - The header's three-class failure taxonomy (
:33-52) is the right way to document a fail-open script, and every path inmain()demonstrably lands in exactly one of the three. Rejecting my "wrap everything as success" suggestion for class 1, with the reason stated inline, is the correct call. :157-161claims the unquoted spelling is already house style in this file; verified —prReviewGateStatusContextis unquoted atvalues.blockcast.yaml:554, 39 lines below the quoted key at:515. The finding it guards against was live, not hypothetical.- Testing against the shipped values file, not only synthetic strings, puts the regression guard on the artifact that actually drifts.
findPlainCommentStartgets a subtle YAML rule right and says why in two lines (:95-98), andgate/a#bis tested at:134.- The workflow comment at
:22-28now names the cost this change introduces instead of claiming it introduces none. That reconciliation is what makes the required-check step decidable later.
Recommended Action
- Retry the two reads in
determineVerdict(Important 1) before the context is marked required — it is the one remaining path where a transient failure defeats the gate rather than merely delaying it. - Consider the suggestions opportunistically;
main()coverage is the one with lasting value.
The two blocking findings from 24d5206d are genuinely closed, and the fixes are better reasoned than the recommendations that prompted them. The remaining item is a hardening of the fail-open posture, not a contradiction of it.
…ict (BLO-26602) Ally's review of 80ef3bf (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.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2ff0804
Prior Findings Dispositioned (1)
-
prior:80ef3bf important 1 — fixed —
scripts/mirror-comment-review-gate-to-merge-group.mjs:362— the two reads indetermineVerdictnow go through the same retry as the write.withRetrywas extracted out ofpostStatus(:311) and both reads call it: head SHA at:362, statuses at:373.Verified by execution at this head with a stub
gh, against a PR head carrying a blockinggate/ally-comment-findings=failure:reads before at this head healthy state=failurestate=failureone transient 502, then recover state=success(gate defeated)::warning::Reading statuses … (attempt 1/3); retrying.thenstate=failure502 past all 3 attempts state=successstate=success— class-3 floor, unchangedThe middle row is the finding, and it is closed: a one-off blip no longer converts a blocking verdict into a pass. Class 3 remains the terminal behaviour after retries are exhausted, which is the right call — the alternative on a queue ref is an ejection that cannot be re-run.
Two things I had not asked for and that make the fix better than the recommendation: the SHA-format check at
:369was deliberately left outside the retry with the reason stated inline (a well-formed response carrying a malformed SHA is not transient), andbuildStatusArgswas extracted and exported at:328, which closes the "the POST is not proven to carrymirrorVerdict's state" half of my standing test suggestion —:82of the test file now asserts exactly that, including that no verdict this module produces can putpendingon the wire.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[native-codex]
scripts/mirror-comment-review-gate-to-merge-group.mjs:275— the class-3 description is truncated at exactly the part that tells a human what to do.failOpenVerdictbuildsGate mirror failed (<reason>); passing open. See the merge-queue job log., and for the commonest class-3 trigger the reason is anexecFileSyncmessage that embeds the full command:Gate mirror failed (Command failed: gh api repos/Blockcast/paperclip/statuses/2ff08042…43 --paginate); passing…174 characters against
MAX_DESCRIPTION140, so the lost tail isopen. See the merge-queue job log.— measured, not estimated, and reproduced end-to-end. It is not universal (a short reason such as aJSON.parsemessage is 99 chars and survives intact), but it fires precisely on thegh-failure shape that dominates this class. This is the case where the description is doing the most work: once the context is marked required, a class-3successmerges the entry, and the description is the only surviving evidence that the gate was defeated rather than satisfied. Putting the pointer before the reason —Gate mirror failed; passing open, see the merge-queue job log (<reason>)— keeps the actionable half and lets truncation eat the diagnostic half instead. Relatedly, the status is still posted with notarget_url; pointing it at the source PR or the run would make the fail-open cases self-diagnosing and matters most in exactly the case where the log pointer is what gets cut. -
[gstack/review]
scripts/mirror-comment-review-gate-to-merge-group.mjs:373—JSON.parseis inside the retry, which contradicts the reasoning applied 4 lines above it.gh()isJSON.parse(ghRaw(…)), so an unparseable body is retried 3 times; but the header at:49lists "malformed API JSON" as class 3 alongside "unparseable queue ref", i.e. explicitly non-transient, and:367-368places the SHA-format check outside the retry for exactly that reason. Confirmed at this head with a stubghthat exits 0 with a non-JSON body: two::warning:: … retrying.lines and 7s elapsed before landing on the same class-3 result it would have reached immediately. Harmless in outcome, and 7s againsttimeout-minutes: 10is nothing — it is worth fixing only because the inconsistency will read as an oversight to the next person, and the file is otherwise unusually careful about which failures are worth retrying. SplittingghRaw(retried) from the parse (not retried) would make the two calls agree with the header. -
[pr-review-toolkit/comments]
scripts/mirror-comment-review-gate-to-merge-group.mjs:61— "The context name is read from the deployed Helm values" is a shade stronger than what happens. The workflow checks outmaster(.github/workflows/comment-review-gate-merge-group.yml:59) andVALUES_PATH(:80) resolves relative to the script, so the value read is master's copy of the source for the deployment, not the running deployment. They diverge for the length of a deploy lag: if a context rename lands on master before it ships, this script posts under the new name while the server is still posting the old one on the PR head,selectLatestStatusfinds nothing, and every queue entry mirrorsNo gate verdict … passing openuntil the deploy catches up. That is fail-open and narrow, so it is a comment fix rather than a code one — but the whole point of reading the value instead of hardcoding it was to close rename drift, and this is the residual seam where rename drift still bites. Worth one sentence naming it, since the current wording invites the reader to assume it cannot happen.
Strengths
- The fix went to the asymmetry rather than to the symptom.
withRetry's doc at:295-309states plainly why the reads deserve the retry more than the write does — a defeated gate versus a delayed one — which is the actual argument, and it is the one I had to make in the finding rather than one I was given. - Verified the header's factual claims against the live repo, and they hold:
rules/branches/masterreturns exactly one rule,merge_queue, withgrouping_strategy: ALLGREEN,check_response_timeout_minutes: 360andmax_entries_to_build: 1. There are norequired_status_checkson master at all, so "deliberately not marked required by this change" is accurate and the single-PR assumption inparsePrNumberFromQueueRefis sound. - The workflow's runner claim checks out too:
arc-merge-queueis the establishedmerge_grouppool acrosspr.yml(:27,:573,:620,:798,:1229), so this job adds no new capacity dependency class — and the new test step lands inpolicy, which runs in both lanes, so a values-file reformat is caught on the PR and fails fast in the queue rather than after the 6h timeout. --paginateon the statuses read is correct here, which is worth saying because it is easy to get wrong: I forced multi-page pagination against this PR's head (?per_page=1, 4 statuses across 4 pages) andghmerged them into one valid JSON array thatJSON.parseaccepts. The][concatenation failure that bites object-returning endpoints does not apply to this one.- 35/35 tests pass at this head, up from 29.
buildStatusArgsis the right seam to have added: the no-pendinginvariant was previously proven only insidemirrorVerdict, and it is now proven on the argv that actually reaches GitHub. - Leaving the SHA-format check outside the retry, with the reason in two lines at
:367-368, is a genuinely careful distinction — and it is the one that makes theJSON.parsesuggestion above worth making at all.
Recommended Action
- Nothing blocking. The four heads on this PR have closed all three prior Important findings at the mechanism rather than the symptom.
- Take the description-ordering suggestion before the context is marked required — that is the point at which a class-3
successstarts merging entries, and the description becomes the only evidence anyone gets. - The other two are opportunistic; both are comment/consistency fixes, not behaviour.
The remaining risk on this change is not in the script — it is the separate branch-protection step, which this correctly does not take.
…(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 2ff0804: 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5501264
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The queue workflow checks out
masterrather than executing candidate code while holdingstatuses: write, preserving the reporting boundary. readGateContextnow distinguishes an absent or deliberately empty key from an unreadable value and accepts the valid YAML scalar spellings covered by the regression tests.- Transient failures in both PR-head reads and the status write are retried; exhausted read failures have an explicit fail-open status while context and queue-head failures fail loudly.
- The status-argument seam and the 35-test suite verify that the verdict reaching GitHub never becomes
pending. - The workflow documents the real queue-ejection tradeoff instead of claiming the additional merge-group job has no merge impact.
Recommended Action
- No Critical or Important changes are required for this head.
- Keep the separate branch-protection rollout gated on observing a real verdict on a subsequent merge-queue entry.
Thinking Path
Linked Issues or Issue Description
gate/ally-comment-findingsand gave it a four-way verdict. This PR reads that name from Helm values rather than hardcoding it.pull_request_reviewobject, so a comment gate that fails on absence deadlocks every formally-reviewed PR. This PR deliberately does not revive it.Activate comment-shaped Ally review gate in production).What Changed
.github/workflows/comment-review-gate-merge-group.yml, triggered onmerge_group: [checks_requested],runs-on: arc-merge-queue, permissionsstatuses: write+pull-requests: read.scripts/mirror-comment-review-gate-to-merge-group.mjs— parses the PR number out of the queue ref, reads the gate's latest verdict on that PR's head, and writes the corresponding status onto the queue head.deploy/helm/paperclip/values.blockcast.yaml, not hardcoded. An empty value (how the gate is switched off;values.yamlships"") is a no-op, because commit statuses cannot be deleted once written.master, never the queue candidate. The job holdsstatuses: 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 ascommitperclip-review.yml.nodewould exitMODULE_NOT_FOUNDand ALLGREEN would eject the entry. It is also the right steady state: a revert of the script must be a no-op, not a wedged queue.pr.yml's existing script-test block.The one invariant
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 assuccess. That is the same fail-open posture the gate already takes on the PR head, so this change is strictly no worse than today's behaviour on every path.Verification
node --test ./scripts/mirror-comment-review-gate-to-merge-group.test.mjs→ 19 pass / 0 fail (run post-rebase). Covers queue-ref parsing (including base branches containing slashes), values parsing, newest-status selection on unsorted input, description truncation, and the no-pendinginvariant explicitly.js-yaml); the new job resolves toon: {merge_group: {types: [checks_requested]}},runs-on: arc-merge-queue,timeout-minutes: 10.::notice::and exit 0; present → reachesnode, which emits its own::error::for missing env. So the guard neither swallows a real failure nor fails on a legitimate absence.git cat-file -e blockcast/master:scripts/mirror-comment-review-gate-to-merge-group.mjs→ absent on master.blockcast/master, every commit single-parent) — this repo's queue merge method is REBASE, and a merge commit makes a PRmergeablebut notrebaseable, which dequeues it silently.What this PR does not verify, and cannot: that a real verdict appears on a real
gh-readonly-queue/*head. By construction the guard makes this PR's own queue entry a no-op, so the first real verdict lands on the next entry queued after this merges. That observation is BLO-26602's acceptance criterion and I will record it on the issue from the next queue entry — it is deliberately not claimed here.Risks
arc-merge-queue(the dedicated pool every othermerge_groupjob already uses, BLO-22428) and a 10-minute cap matching the rest of the repo.gate/ally-comment-findingsis a separate human branch-protection action, and it should not be taken until this has been seen writing real verdicts on real queue refs. Doing it earlier is the repo-wide merge outage described above.statuses: writeon amerge_grouptrigger. Contained by checking outmaster: the executed code is never the code under review.Model Used
claude-opus-5[1m]— 1M context, extended thinking, tool use. Acting as the Paperclip CTO agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template