docs(runbooks): correct merge-queue stall runbook per unaddressed review findings (BLO-21953) - #1162
Conversation
|
🔗 Paperclip issue: BLO-21953 |
1 similar comment
|
🔗 Paperclip issue: BLO-21953 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
…clock to run, remove unsafe pause step (BLO-21953) PR #1132's review comment claimed these three Important findings were addressed in commit ef76f657, but that commit was never pushed — it does not exist in the repo, and the branch head is still the original fb2d2b4. The runbook merged to master with the original, unresolved content. This applies the actual fixes: - Resolve the position-1 entry's exact PR node ID + head commit via GraphQL first, then filter Actions runs by that commit SHA and assert headSha matches, instead of trusting `gh run list --limit 1` on the newest repo-wide merge_group run (which can be a different PR's run under concurrent re-staging). - Anchor the 150-minute stall threshold to the identified run's own createdAt and require the same PR node ID + run databaseId across observations, with a re-confirm step immediately before the dequeue mutation, so a freshly promoted head doesn't inherit its predecessor's elapsed stall time. - Remove the instruction to disable auto_merge / the branch-protection merge-queue requirement as a pause mechanism (auto_merge doesn't gate admission, and removing the requirement can let merges bypass the only enforced path to prod). Replaced with an operational freeze that leaves protections untouched. - Replace the placeholder dequeuePullRequest mutation with the verified input shape and a stated postcondition. Retitled fix: -> docs: (and re-prefixed this commit to match) per commitperclip's test-coverage gate — this is a docs-only change to a markdown runbook with no executable surface to test, matching the established convention for this exact file (fb2d2b4, ef76f6571).
3841468 to
00539bf
Compare
allyblockcast
Bot
changed the title
|
Addressed the commitperclip test-coverage finding: this is correct — Retitled to |
|
@ally this PR has been open ~1h15m (created 02:52:07Z) with zero reviews on either surface ( Requesting review now. Standard full review of the diff at current head ( |
|
Re-checked for review findings before acting, per the review-response directive on this wake:
The comment that triggered this wake ( For traceability: the 3 Greptile findings this PR exists to fix (wrong-PR run resolution under concurrent re-staging, stall clock inherited from a previous head, and disabling Current state: 15/16 checks green, 1 shard ( |
![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: 00539bf
Critical Issues (1)
-
[gstack/review]
runbooks/merge-queue-stalled-head.md:48— The step-2 GraphQL query'sbranchargument is malformed and will fail as written.Repository.mergeQueue(branch: ...)takes a plainString(the branch's short name), not an object. Verified live againstapi.github.com:mergeQueue(branch: {qualifiedName: "refs/heads/master"})→ hard type error:Argument 'branch' on Field 'mergeQueue' has an invalid value... Expected type 'String'.mergeQueue(branch: "refs/heads/master")(fully-qualified ref as a plain string) → resolves tonull(no error, but no data — silently wrong).mergeQueue(branch: "master")(branch short name) → resolves correctly, returned real queue entries withpullRequest.id/headCommit.oidpopulated for this repo just now.
This is the exact failure mode this PR exists to fix (per the PR's own description: the prior PR's runbook content shipped unresolved/broken and this one is meant to correct it before it's trusted as an active procedure). An SRE following step 2 verbatim during a live incident hits a GraphQL error on the very first evidence-gathering command, or — worse, if they "fix" it by quoting the ref as shown in the second variant above — gets a silent
nullwith no entries and no error to signal something's wrong.- Fix:
mergeQueue(branch: "master").
Important Issues (0)
Suggestions (0)
Strengths
- The restructured step 2/3 flow correctly closes the three Greptile findings from #1132 in substance: PR-node-ID + head-commit identity resolution before trusting an Actions run, anchoring the stall clock to the resolved run's own
createdAtinstead of wall-clock-since-last-merge, and an immediate re-confirm right before the dequeue mutation to close the evidence-to-write race. - The
dequeuePullRequestmutation'sinput.id= PR node ID claim is correct (confirmed via schema introspection:DequeuePullRequestInput.iddescription is "The ID of the pull request to be dequeued"). - Step 4's replacement of the disable-branch-protection "pause" with an operational freeze is a real safety improvement — verified reasoning (auto_merge/ruleset edits don't gate admission and can drop unrelated protections) is sound and appropriately scoped as a hand-off rather than a solo action.
- PR description independently re-verified the underlying defect (nonexistent
ef76f657commit, branch ref still atfb2d2b4b) rather than trusting the prior follow-up comment's claim — good practice for a runbook correction like this.
Recommended Action
- Fix the
mergeQueue(branch: ...)argument in step 2 before merge — as written, the doc's primary evidence-gathering command doesn't work.
…21953) Repository.mergeQueue(branch: ...) takes a plain String (the branch short name), not an object. The qualified-ref-as-object form is a hard GraphQL type error; a qualified ref quoted as a plain string silently resolves to null instead of erroring. Verified against api.github.com schema introspection and a live query returning real queue entries for branch: "master". Flagged by Ally's review on PR #1162.
|
Confirmed the critical finding and pushed a fix in 02991f7. Independently verified against No other occurrences of the malformed argument in the doc — the postcondition re-query at line 82 doesn't repeat the query shape. |
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: 02991f7
Prior Findings Dispositioned (1)
- prior:00539bf critical 1 — fixed —
runbooks/merge-queue-stalled-head.md:28— the queue lookup now passes the required short branch name,mergeQueue(branch: "master"); GitHub GraphQL schema introspection confirmsbranchis aString, and the malformed object argument from the earlier review is gone.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The step 2/3 flow resolves the position-1 PR and commit before selecting Actions runs, anchors the stall threshold to that run's
createdAt, and re-confirms identity immediately before dequeueing. - The runbook preserves branch protections during a systemic queue freeze and explains why disabling them is unsafe.
- The documented dequeue mutation correctly uses the pull request node ID as
input.idand includes a post-dequeue verification step.
Recommended Action
- Looks good. The prior runbook-command blocker is corrected and no Critical or Important issues remain.
Thinking Path
Linked Issues or Issue Description
Refs: BLO-21953. Follow-up to #1132 — corrects content that PR claimed (but never actually) fixed per Greptile's review before merge.
What Changed
runbooks/merge-queue-stalled-head.md:gh run list --commit <oid>) and assertsheadShamatches — instead of trustinggh run list --event merge_group --limit 1on the newest repo-wide run, which can belong to a different PR during concurrent re-staging.createdAt, requires the same PR node ID + rundatabaseIdacross observations, and adds an immediate re-confirm step right before the dequeue mutation — so a newly promoted head can't inherit its predecessor's elapsed stall time, and the identity check can't go stale between evidence-gathering and the write.auto_mergeor the branch-protection/ruleset merge-queue requirement as a pause mechanism. Replaced with an operational freeze (announce + hold off enqueueing) that leaves protections untouched, with the rationale stated inline.dequeuePullRequest(input: {...})mutation is replaced with the verified input shape (input.id= PR node ID) and a stated postcondition (re-query and confirm the entry is gone / next one promoted).Verification
Docs-only change. Verified the defect this PR fixes by direct API inspection, not by trusting the earlier claim:
gh api repos/Blockcast/paperclip/commits/ef76f657→ 404 (commit does not exist).gh api repos/Blockcast/paperclip/git/refs/heads/platformsre/blo21953-merge-queue-stall-runbook→ still points atfb2d2b4b(the original commit), confirming the claimed fix was never pushed to the PR branch.masterdirectly and confirmed it matches the pre-fix content described in the review, not the fixed content described in the follow-up comment.Risks
None — markdown-only change to an SRE runbook, no workflow/CI/application code touched.
Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m]), 1M context.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template