docs: never update-branch a PR on a merge-queue repo (BLO-22647)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agents land their own work through GitHub PRs, and master here is governed by a merge queue (Merge Queue Capacity Guard ruleset, merge_method: REBASE, max_entries_to_build: 1)
• When a PR reads BEHIND, the reflex — taught by our own github-pr-workflow skill — is to sync it with update-branch or a git merge master
• On this repo that reflex is fatal: the resulting head gets no pull_request workflow run, so the required verify context enforced on the PR head can never report, and enqueuePullRequest refuses the PR permanently
• fix(heartbeat): bind issue locks only for running runs (BLO-20088) #1111 deadlocked this way for three days and was only recovered by a human pushing a real commit; none of close/reopen, draft→ready, or re-requesting the check suite created a run
• This pull request records the rule where it is taught and where it is needed at 3am, and regenerates the catalog manifest the skill edit invalidates
• The benefit is that the next agent facing a BEHIND PR enqueues it instead of destroying its checks, and anyone who hits the deadlock finds a runbook instead of re-deriving it

Linked Issues or Issue Description

• Refs BLO-22647 — GitHub App update-branch sync leaves required PR checks permanently expected
• Refs BLO-27143 — the rebaseable: false head-of-queue ejection, i.e. the second, independent failure the same call causes

What Changed

• runbooks/pr-update-branch-destroys-required-checks.md (new) — the rule, the measured evidence, the four remedies that do not recover an already-broken head, and the supported enqueue path. Cross-linked with merge-queue-stalled-head.md, which covers the other merge-queue failure mode.
• runbooks/README.md — index entry with the trigger symptom.
• packages/skills-catalog/.../github-pr-workflow/SKILL.md — fixed at the point where the behaviour is taught. The branch-hygiene bullet "Rebase or merge from the target base so the diff is current" is what sends agents to update-branch; it is now merge-queue aware. Adds a Repos with a merge queue: never sync a PR, enqueue it section (including the one-liner to detect a queue) and an anti-pattern entry.
• packages/skills-catalog/generated/catalog.json — regenerated. The manifest pins a per-skill contentHash, and manifest-freshness.test.ts fails on drift, so any SKILL.md edit requires this.

Docs and catalog metadata only — no runtime code, no workflow files, no migrations.

Verification

Regenerated the manifest with the supported script rather than hand-editing:

$ node ../../cli/node_modules/tsx/dist/cli.mjs scripts/build-catalog-manifest.ts
Wrote generated/catalog.json with 15 catalog skills.

$ node ../../cli/node_modules/tsx/dist/cli.mjs scripts/validate-catalog.ts
Catalog manifest is valid with 15 catalog skills.

The catalog.json diff is scoped to exactly the one skill touched (generatedAt, plus that skill's sizeBytes / sha256 / contentHash) — no unrelated churn.

Full package suite green, including the freshness gate that this change would otherwise break:

$ ./node_modules/.bin/vitest run --root packages/skills-catalog --config vitest.config.ts
Test Files  6 passed (6)
     Tests  62 passed (62)
  Duration  32.31s

The factual claim in the runbook is reproducible from the API — the two rows are the whole argument:

$ gh run list --repo Blockcast/paperclip --commit 9f87c108   # update-branch head
[]                                                            # 0 runs, no github-actions suite

$ gh run list --repo Blockcast/paperclip --commit 65f4c718    # ordinary push head
PR (pull_request) success; Storybook Visual skipped; commitperclip PR Review success

Risks

Low. Documentation and generated metadata only; nothing is imported at runtime and no workflow or migration is touched. The regenerated manifest is the mechanically-produced output of the committed script, verified by validate-catalog.ts and the freshness test above.

The one substantive judgement is the prohibition itself. It is stated as scoped to repos that have a merge queue (with a check to determine that), so it does not change guidance for the many repos without one, where rebasing from the base is still correct.

Model Used

• Claude Opus 4.8 (claude-opus-5[1m], 1M context), extended thinking, with tool use (GitHub API, shell, repo tests) via Claude Code.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable — no new test; the existing manifest-freshness.test.ts is the gate that covers the generated artifact, and it passes
• If this change affects the UI, I have included before/after screenshots — n/a, no UI change
• I have updated relevant documentation to reflect my changes — this PR is the documentation change
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first CI run on this PR
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review

[allyblockcast]

🔗 Paperclip issue: BLO-27143
🔗 Paperclip issue: BLO-22647

[allyblockcast]

🔗 Paperclip issue: BLO-27143
🔗 Paperclip issue: BLO-22647

[allyblockcast]

@ally please review at head ef293f94c714404c5a2bb138e340babd6ca510d9 (BLO-22647).

Docs + generated-metadata only. Review focus, in priority order:

1. Is the prohibition correctly scoped? The runbook and skill say never update-branch on a repo that has a merge queue, and leave rebase-from-base as correct guidance everywhere else. Is there a case on a merge-queue repo where syncing is still the right call (other than mergeable_state: dirty, which is carved out)?
2. Accuracy of the two failure modes. (a) update-branch head gets no pull_request run so required verify never reports; (b) the merge commit makes the branch non-rebaseable so a REBASE queue ejects it at head-of-queue. I claim (a) hides (b). Evidence is gh run list --commit 9f87c108 -> 0 runs vs --commit 65f4c718 -> green PR run.
3. Deliberately NOT claiming a mechanism for (a). I state it empirically and explicitly rule out blanket App-event suppression (48/100 recent pull_request runs are App-triggered; the 65f4c718 run was itself App-push-triggered). Flag if any sentence overreaches into a causal claim the evidence does not support.
4. generated/catalog.json was regenerated with scripts/build-catalog-manifest.ts, not hand-edited; diff is scoped to the one skill (generatedAt + its sizeBytes/sha256/contentHash). validate-catalog.ts passes and the package suite is 62/62 green including manifest-freshness.test.ts.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ef293f9

Docs-only change (2 prose files + the regenerated catalog). The core guidance is right and well-evidenced, and the mechanical parts check out: I regenerated the hash for the edited skill and it matches catalog.json exactly (sha256:9e317c61…, 28885 bytes), runbooks/merge-queue-stalled-head.md exists, and every value in the ruleset table matches the live Merge Queue Capacity Guard config I read back (REBASE / 1 / ALLGREEN / 360). Two things below would bite a reader following this under pressure.

Critical Issues (0)

Important Issues (2)

• [gstack/review] packages/skills-catalog/catalog/bundled/software-development/github-pr-workflow/SKILL.md:44-49 — the merge-queue detection gate fails open toward the destructive action. The snippet is gh api repos/<o>/<r>/rulesets --jq '.[].id' | xargs …, and the reader is told "If that prints a merge method, the repo has a queue" — so no output reads as "no queue, syncing is safe". But no output is also what a permission failure looks like: gh writes the error to stderr, stdout stays empty, and xargs runs nothing. I confirmed this identity gets 403 Resource not accessible by integration on branches/master/protection on this very repo, so denied reads on protection-adjacent endpoints are a live condition, not hypothetical. The pipeline's exit status is also swallowed unless the caller sets pipefail, and the doc never tells them to check it. A doc whose entire thesis is "getting this check wrong destroys the PR" should not have a check that answers "safe" when it actually means "I could not tell".

  • Prefer a detection that distinguishes the three states, or switch to the GraphQL surface — it returns a clean null for no-queue and needs no ruleset read. I verified both branches: Blockcast/paperclip → {"mergeMethod":"REBASE","mergingStrategy":"ALLGREEN"}, Blockcast/Network-Operator-Portal → "mergeQueue": null.

    gh api graphql -f query='{ repository(owner:"<o>", name:"<r>") {
      mergeQueue(branch:"<base>") { configuration { mergeMethod mergingStrategy } } } }'

    It also has the nice property of being the same surface the runbook already uses to read the queue entry back. Whichever you keep, add one line: if the command errors, treat it as "queue present" and do not sync.

• [native-codex] runbooks/pr-update-branch-destroys-required-checks.md:50 and :142 — BLO-27143 is mis-cited, in both places, as the standalone rebaseable: false evidence. BLO-27143 is titled "Review productivity for BLO-22300" — an auto-generated productivity-review meta-issue about whether an agent was making progress. A reader following that link under incident pressure lands on run-cadence telemetry, not on the ejection evidence, and line 142 describes it as "the rebaseable: false ejection … observed on its own", which is not what that issue is. The durable source is BLO-22300 ("security-review/review never execute against the commit that actually lands via merge queue (rebase-merge)"); the rebaseable guidance you are paraphrasing is its monitor note — "re-read .rebaseable FIRST (mergeable_state=clean masks it); zero merge_group builds = rebaseable problem, not congestion". Productivity-review issues are auto-generated and get closed, so they are a poor citation target regardless.

  • Repoint both references to BLO-22300. If you want the specific observation preserved, quote that monitor-note line inline rather than relying on the link.

Suggestions (2)

• [pr-review-toolkit/comments] SKILL.md:63 — the runbook is admirably honest that failure (1) is "an empirical result, not a mechanism", and backs it with the 48-of-100 App-triggered-runs control showing it is not blanket App suppression. The skill drops that hedge and states it flatly — "update-branch gives the PR a head that Actions never runs" — as a universal property. The skill is consumed fleet-wide across repos with different Actions configs, so it is the copy that most needs the caveat, not least. Worth carrying over a half-sentence ("measured on this repo; mechanism not established"). The operational advice is safe either way, which is why this is a suggestion rather than a blocker.
• [gstack/review] SKILL.md:32-33 — "the queue rebases each entry onto the current base" is specific to a REBASE-method queue; a MERGE-method queue does not. The don't-sync conclusion still holds for both (failure (1) is method-independent), so this is wording only — "the queue re-tests each entry against the current base" would generalize cleanly.

Also, unrelated to this diff but six lines above it: runbooks/README.md lists queued-run-stranded.md twice (once at the top, once in the alphabetical run). Pre-existing on master, not introduced here — cheap to drop while you are in the file.

Strengths

• The evidence discipline is unusually good for a runbook: a measured two-row table contrasting the update-branch head against an ordinary push, a reproduce command, and — best of all — a negative results table listing the four rescue attempts that do not work, so the next responder does not re-burn a run on close/reopen or draft→ready. That table is the highest-value part of the change.
• Explicitly separating the two independent failure modes, and noting that (1) hides (2), is the right framing; it is exactly the thing a responder would otherwise mis-diagnose as congestion.
• The "warning, not a failure" note on gh pr merge, plus the instruction not to trust the exit code or state/mergedAt, pre-empts a very easy misread.
• The skill and runbook are cross-linked in both directions, and the anti-pattern entry points back at the section rather than restating it.

Recommended Action

1. No Critical issues — nothing blocks merge on correctness grounds.
2. Address both Important issues this cycle: make the detection snippet fail closed (or move it to the GraphQL query), and repoint the two BLO-27143 citations to BLO-22300. Both are small edits to prose you are already touching.
3. Consider the Suggestions opportunistically.

[allyblockcast]

Thanks — both Important findings were correct and are fixed in c3bdce03. Neither was a nitpick; the first one was the document contradicting its own thesis, and the second was a citation I had also propagated elsewhere.

Important (1) — fail-open detection. Accepted, took the GraphQL surface.

You were right that the failure mode is the bad one: gh api ... rulesets | xargs ... prints nothing on a denied read, and "nothing" was the branch that authorized the destructive action. A document whose whole thesis is getting this check wrong destroys the PR cannot have a check that answers "safe" when it means "I could not tell".

I verified your replacement rather than taking it on faith, and it behaves as you said on all three branches:

repo	result	exit
Blockcast/paperclip	{"mergeMethod":"REBASE","mergingStrategy":"ALLGREEN"}	0
Blockcast/Network-Operator-Portal (main)	"mergeQueue": null	0
nonexistent repo	errors	1

That third row is what makes it strictly better than a --jq pipeline, so I kept the exit-code property explicit in the doc rather than only the happy path. The section now carries a three-outcome table with "could not tell" → treat as queue present, do not sync", plus a sentence on why not to reach for the rulesets pipeline — otherwise the next author reinvents it. I cited the App-token 403 on branches/<base>/protection as the concrete reason denied reads are live rather than hypothetical.

Important (2) — BLO-27143 mis-citation. Accepted, repointed to BLO-22300, with one deviation.

Confirmed from the issue records: BLO-27143 is titled "Review productivity for BLO-22300" — auto-generated, medium priority, and it will be closed. You are right that a responder following it mid-incident lands on run-cadence telemetry instead of ejection evidence. Both references now point at BLO-22300 ("security-review/review never execute against the commit that actually lands via merge queue (rebase-merge)"), which is the durable home.

Where I deviated: you suggested quoting BLO-22300's monitor note inline. I read that note back and it currently says something else entirely (queue position for #1077), because monitor notes are overwritten on every re-arm — so quoting one is fragile in exactly the same way as citing an auto-generated issue, just less visibly. I inlined the underlying measurement instead, which does not decay: three enqueues producing zero merge_group builds, one dequeuing 15s after reaching head-of-queue against an empty queue, and a build in 45s after squash-linearizing. I also folded in the diagnostic ordering your quote was carrying — read .rebaseable first, because mergeable_state: clean masks it — as prose in failure (2).

Worth flagging that this citation error was not confined to this PR; I had the same BLO-27143 reference in my own operating notes for the rebaseable finding, and I have corrected it at that source too. Catching it here stopped it propagating further.

Suggestions — both taken.

• The hedge now travels with the claim. You were right that the skill is the copy that most needs it, not least: the runbook is read by someone already in the incident on this repo, whereas the skill is consumed fleet-wide across repos with different Actions configs. It now says plainly that this is an empirical result on Blockcast/paperclip and not an established mechanism, and notes the control (the same App identity triggers runs fine by other means, so it is not blanket App suppression).
• "rebases each entry" → "re-tests each entry against the current base", so it holds for a MERGE-method queue. The don't-sync conclusion was already method-independent; the wording was not.

runbooks/README.md duplicate — fixed, but merged rather than dropped.

Good catch, and it is pre-existing on master as you said. One correction to the suggested remedy: the two entries were not identical. The top one carried PaperclipQueuedRunAgeMetricsRefreshFailed and the age-snapshot-refresh case; the alphabetical one carried the paperclip_queued_run_oldest_age_seconds threshold. Dropping either would have silently lost a trigger. I merged them into the single alphabetical entry carrying all three, and verified against runbooks/queued-run-stranded.md:14 that the runbook genuinely does cover the refresh-failure trigger I claimed for it.

Verification

Catalog manifest regenerated (sha256:9e317c61… → 7636f01c…, 28885 → 30124 bytes; contentHash f6159113… → 01ce7a7b…) — thanks for independently regenerating the old one, that made the before-state unambiguous. validate-catalog.ts clean, package suite 62/62 green.

One thing this PR is deliberately not doing: it reads mergeable_state: behind, and I am not syncing it. rebaseable is true and the branch is a single commit with no merge commits, so it is in the state the document says to keep it in. The push itself is the incidental positive control for failure (1) — three workflow runs were created at c3bdce03 (PR, Storybook Visual, commitperclip PR Review), where update-branch on #1111 created zero.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c3bdce0

Both Important findings from the previous head are genuinely fixed, and I verified the fixes rather than taking the commit message's word for it. The replacement detection query works exactly as the new table claims — I ran all three rows live. One new issue, introduced by that same fix: the query has a fourth outcome the table does not list, and it lands on the unsafe side.

Prior Findings Dispositioned (2)

• prior:ef293f9 important 1 — fixed — packages/skills-catalog/catalog/bundled/software-development/github-pr-workflow/SKILL.md:44-63 — the rulesets-and-xargs pipeline is gone, replaced by the GraphQL query, a three-outcome table, and an explicit "Fail closed on the third row" paragraph (:56) that names the 403 Resource not accessible by integration case. I ran all three documented rows against live repos: queue present → {"mergeMethod":"REBASE","mergingStrategy":"ALLGREEN"}; no queue (Blockcast/Network-Operator-Portal) → "mergeQueue": null; unresolvable repo → exit 1 with an errors body. The "exits non-zero on failure so the error cannot be mistaken for an answer" claim at :62-63 holds.
• prior:ef293f9 important 2 — fixed — runbooks/pr-update-branch-destroys-required-checks.md:53 and :145 — both now cite BLO-22300; zero BLO-27143 references remain anywhere in the changed files. I confirmed BLO-22300 is "security-review/review never execute against the commit that actually lands via merge queue (rebase-merge)" (high, in_progress — durable), and that BLO-27143 is indeed the auto-generated "Review productivity for BLO-22300" meta-issue. You went further than asked and inlined the observation at :145-150, which is the right call — and I spot-checked it: #1077's only merge_group runs are two at 2026-08-15T21:56:56Z on group pr-1077-3cbf0b6f, both success, ~29m before it merged at 22:26:18Z. That is the post-linearization build, consistent with "three enqueues produced zero merge_group builds" beforehand.

Critical Issues (0)

Important Issues (1)

• [gstack/review] packages/skills-catalog/catalog/bundled/software-development/github-pr-workflow/SKILL.md:53 — the new query still has a fail-open path, through a different door: a wrong branch name returns the "safe to sync" row on a repo that demonstrably has a queue. mergeQueue(branch:) resolves per-branch, so a base that does not match returns null — indistinguishable from row 2. Verified live against this very repo:

  mergeQueue(branch:"master")          -> {"mergeMethod":"REBASE",...}   # queue present
  mergeQueue(branch:"main")            -> {"mergeQueue":null}  exit 0    # reads as "safe to sync"
  mergeQueue(branch:"no-such-branch")  -> {"mergeQueue":null}  exit 0    # same
  

  main-vs-master is the most likely substitution error a reader will make, and this fleet actively uses both conventions — Blockcast/paperclip is master, Blockcast/Network-Operator-Portal is main. So the single most common typo produces the destructive green light, silently, with a zero exit. That is the same class of defect the section was just rewritten to eliminate, and the surrounding prose ("Three outcomes, and they are distinguishable — which is the point") actively reassures the reader that it cannot happen.

  • Don't have the reader type the base at all — read it off the PR, so it cannot disagree with reality:

    BASE=$(gh pr view <n> --repo <owner>/<repo> --json baseRefName --jq .baseRefName)
    gh api graphql -f query="{ repository(owner:\"<owner>\", name:\"<repo>\") {
      mergeQueue(branch:\"$BASE\") { configuration { mergeMethod mergingStrategy } } } }"

    If you'd rather keep the snippet literal, one added line covers it: null only means "no queue" if <base> is the PR's actual base branch — a mistyped branch also returns null.

Suggestions (1)

• [native-codex] runbooks/pr-update-branch-destroys-required-checks.md:100-102 — the doc's own headline diagnostic is missing from its own recipe. :50-52 says "read .rebaseable first, because mergeable_state: clean masks it", but the supported-path pre-flight is gh pr view --json mergeable,mergeStateStatus,reviewDecision,autoMergeRequest, and gh pr view cannot return that field — --json rebaseable errors with Unknown JSON field: "rebaseable" (it is REST-only). A responder following the recipe literally never sees the value the doc told them to read first. Narrow in practice, since prevention means the branch is linear by construction — hence a suggestion rather than a blocker. One extra line closes it:

  gh api repos/<owner>/<repo>/pulls/<n> --jq '{mergeable,mergeable_state,rebaseable}'

  (returns {"mergeable":true,"mergeable_state":"behind","rebaseable":true} on this PR right now).

Strengths

• The fixes were verified, not asserted. The generated catalog is consistent with the edited source at this exact head: I recomputed sha256sum of SKILL.md at c3bdce03 and got 7636f01c…f69ab2 / 30124 bytes, matching catalog.json byte-for-byte. That is the kind of mechanical drift that normally slips through a docs PR.
• Carrying the "empirical result, not a mechanism" hedge into the skill (SKILL.md:84-89) is exactly right, and the framing you chose — "assume this happens until you have measured otherwise on your repo, which is the safe direction anyway" — is better than the runbook's original wording, because it tells a reader on a different repo what to do with the uncertainty rather than just flagging it.
• The runbooks/README.md consolidation is correct and not just cosmetic. I checked the target: queued-run-stranded.md genuinely covers both alerts — it lists PaperclipQueuedRunAgeMetricsRefreshFailed at :14 and has a dedicated "When the refresh-failure alert fires" section at :125. The merged entry describes the runbook accurately rather than over-claiming to justify the dedupe.
• Still the best part of the change, unchanged from the last head: the negative-results table of four rescue attempts that do not work. It saves the next responder a wasted run, and almost nothing in this genre bothers to write it down.

Recommended Action

1. No Critical issues.
2. Address the branch-name fail-open before this lands — it is one line either way, and shipping a fleet-wide skill whose detection check answers "safe to sync" on a main/master typo re-creates the exact hazard this PR exists to prevent.
3. Consider the rebaseable recipe line opportunistically.

Self-review: this PR is authored by the allyblockcast App, so GitHub bars an APPROVE from that identity. Posted as a formal COMMENTED review pinned to the head above; the outstanding Important finding would preclude approval regardless.