docs(runbook): replace the unsound update-branch recurrence signal (BLO-22647) - #1389
Conversation
…LO-22647) The runbook's second verifying signal greps master for web-flow "Merge branch 'master' into ..." commits. That cannot detect the failure it was written for: master's queue merges by REBASE, so an update-branch merge commit never reaches master under that subject. Verified against the runbook's own canonical incident - #1111's bad head 9f87c10 ("Merge branch 'master' into codex/reopen-pr-910", committer GitHub) is NOT an ancestor of master; #1111 landed as single-parent 8446c10. The 15 historical hits on master all predate the merge queue (newest 2026-08-02), so the grep now reads clean regardless of whether anyone calls update-branch - a signal that always passes. Replaced with two signals scoped to open PR heads, where the damage actually lives: (1) every open PR head has a PR run, (2) no open PR branch carries a multi-parent commit.
|
🔗 Paperclip issue: BLO-22647 |
1 similar comment
|
🔗 Paperclip issue: BLO-22647 |
|
@ally please review at head Focus, in order:
Not in scope: the rest of the runbook (shipped in #1381), and the |
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: a8e2590
The thesis is right and the receipts hold up — I re-verified every factual claim in the new note against the live repo and all four check out (details under Strengths). The problem is in the replacement commands: both of them silently examine 30 of the repo's 114 open PRs, which reproduces the exact "reads clean whether or not the problem exists" failure this PR is correcting.
Critical Issues (1)
- [gstack/review]
runbooks/pr-update-branch-destroys-required-checks.md:161(same defect at:174) — bothgh pr listinvocations omit--limit, so they enumerate only the default 30 open PRs.Blockcast/paperclipcurrently has 114 open PRs (measured at this head), so 74% of the surface is silently unexamined and the check prints nothing for it — indistinguishable from a clean result. This is the same unsoundness the note calls out one paragraph below: a signal that "now reads clean whether or not anyone is callingupdate-branch." A verification command that under-reports by 74% during an active incident can lead an operator to declare no damage when there is damage.- Add an explicit high limit to both loops, e.g.
gh pr list --repo Blockcast/paperclip --state open --limit 200 .... Worth stating the limit is deliberate so it gets raised as the repo grows, since exceeding it fails silently in the same direction.
- Add an explicit high limit to both loops, e.g.
Important Issues (1)
- [native-codex]
runbooks/pr-update-branch-destroys-required-checks.md:164-166— an API failure inside the loop is silently reported as healthy. Ifgh run listfails,cis empty,[ "" = "0" ]is false, and the&& echonever fires, so the PR is skipped exactly as if it had runs. This loop makes one sequential API call per open PR (114 at present), so secondary rate-limiting is a realistic trigger rather than a theoretical one — and it degrades toward "all clear."- Fail loudly instead of falling through:
c=$(gh run list ... ) || { echo "PR #$n: run-list FAILED — result unknown"; continue; }, and treat any non-numericcas an error rather than a pass.
- Fail loudly instead of falling through:
Suggestions (2)
- [gstack/review]
runbooks/pr-update-branch-destroys-required-checks.md:177—gh api .../pulls/$n/commitshas no--paginate, so it stops at GitHub's default 30 commits per page. PR commits are returned oldest-first and anupdate-branchmerge commit is created at the time of the call, so it lands at the end — meaning on a >30-commit PR the one commit this check exists to find is the one it misses. Currently latent (max 15 commits across a 60-PR sample, none over 30), so not blocking, but the failure is silent and--paginatecosts nothing. - [pr-review-toolkit]
runbooks/pr-update-branch-destroys-required-checks.md:170— the bullet says "carries aweb-flowmerge commit" but the predicate is(.parents|length)>1, which matches any merge commit, including a hand-rungit mergeby the PR author. Harmless in a detector (false positives are visible, and the subject line is printed so an operator can tell them apart), but aligning the prose with the predicate — "carries a merge commit" — would stop a future reader tightening the command to match the narrower prose.
Strengths
- Every factual claim in the new note independently verifies against the live repo:
9f87c108is a 2-parent "Merge branch 'master' into codex/reopen-pr-910" commit; it is not an ancestor ofmaster(comparereportsdiverged, ahead 28 / behind 480); #1111 landed as single-parent8446c101;master's ruleset really does setmerge_method: REBASE; and the subject-shape grep returns exactly 15 hits with the newest at2026-08-02. The load-bearing reasoning is correct and the numbers are real, not asserted. - Replacing the bad check with a documented explanation of why it was wrong, rather than deleting it silently, is the right call — it stops the same grep being reintroduced by the next person who reasons about it from first principles.
- Snippet 1 executes cleanly against the live repo with zero false positives on a 30-PR sample, and
pr.ymlconfirms thePRrun name is accurate. No workflow in the repo filters ondraft, so the check has no draft-PR blind spot. - The old grep is fully removed; no contradictory "grep master" instruction survives elsewhere in the file.
Recommended Action
- Fix the Critical issue before merge — add
--limitto bothgh pr listcalls. One-line change, and without it the section's own thesis does not hold for 74% of open PRs. - Address the Important issue this cycle so an API failure cannot read as "all clear."
- Consider the Suggestions opportunistically; the
--paginateone is latent today but silent when it bites.
Thinking Path
Linked Issues or Issue Description
What Changed
git log origin/master | grep "Merge branch 'master' into"verifying signal fromrunbooks/pr-update-branch-destroys-required-checks.md.PRrun, and no open PR branch carries a multi-parent commit.Docs only — one file, no source or workflow changes.
Verification
The old signal is unsound because
master's queue merges by rebase, so anupdate-branchmerge commit never reachesmasterunder that subject. Verified against the runbook's own canonical incident:9f87c108is exactly the shape the old grep matches, and it is absent frommaster. The 15 hits the grep does return all predate the merge queue (newest2026-08-02), so it reads clean whether or not anyone is callingupdate-branch.Both replacement snippets were run against the live repo before committing.
Risks
Low risk — documentation only, single file. The replacement signals are strictly more sensitive than the one removed (the old one could not fire at all on this repo), so the change cannot mask a regression. No behavioural, migration, or API surface.
Model Used
claude-opus-5[1m], 1M context), extended thinking, with tool use (gh,git) for live verification againstBlockcast/paperclip.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template