Skip to content

feat(merge-queue): detect silent merge-queue eviction (BLO-23395) - #1220

Open
allyblockcast[bot] wants to merge 9 commits into
masterfrom
blo-23395-merge-queue-eviction-detector
Open

feat(merge-queue): detect silent merge-queue eviction (BLO-23395)#1220
allyblockcast[bot] wants to merge 9 commits into
masterfrom
blo-23395-merge-queue-eviction-detector

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The merge-queue reliability lane watches Blockcast/paperclip PRs as they move through GitHub's merge queue and is responsible for catching failure shapes that leave a PR silently stuck there
  • PR feat(vendor): bring claude_k8s adapter in-tree and retire CLAUDE_K8S_REF (BLO-17980) #1092 was legitimately evicted from the queue (master advanced 72 commits, branch went CONFLICTING/DIRTY) but sat that way for 9h13m with zero signal — the queue never even created a merge_group run for it, so there was no failing check, no comment, no wake, and the queue kept draining everything else fine so nothing looked stalled either
  • The existing runbooks/merge-queue-stalled-head.md only documents two failure shapes (a required check that concludes failing; a merge_group check that never reaches a terminal state) — silent eviction with zero runs is neither, and this is a foreseeable recurrence: any PR that waits long enough behind a busy master will eventually go CONFLICTING
  • This pull request adds a detector that classifies every non-merge removed_from_merge_queue event (conflict_unstageable vs. check_failure vs. manual), posts the classification to the PR, and routes it through the existing webhook wake path so the PR author's Paperclip agent is woken directly — a GitHub-side comment alone isn't enough, since an agent-authored PR has no human watching it
  • The benefit is nobody has to notice this by hand again — an evicted PR now gets a within-15-minute wake naming the eviction cause, closing the exact blind spot that let feat(vendor): bring claude_k8s adapter in-tree and retire CLAUDE_K8S_REF (BLO-17980) #1092 sit unnoticed

Linked Issues or Issue Description

No GitHub issue exists for this; tracked in Paperclip as BLO-23395. Following the bug-report template:

What happened: Blockcast/paperclip#1092 was added to master's merge queue at 2026-08-08T09:24:35Z and removed by github-merge-queue[bot] at 13:55:45Z. Nobody noticed for 9h13m, blocking six other issues behind it the whole time.

Root cause of the blindness (not of the eviction itself): the eviction was correct behavior — master had advanced 72 commits past the merge base, the branch became un-stageable, and the queue evicted it. The bug is that this failure shape emits no signal at all: zero merge_group runs were ever created for it (verified by enumerating every merge_group run in the incident window — no pr-1092), so there is no failing check to alert on, no PR comment, no check-run, and no reviewer wake. The only trace is a removed_from_merge_queue timeline event.

Expected: a removed_from_merge_queue event not immediately followed by a merge should notify the PR author's agent within 15 minutes, naming the eviction cause.

What Changed

  • .github/workflows/merge-queue-eviction-detector.yml — fires on GitHub's pull_request action=dequeued event (near-real-time; GitHub fires this for every queue removal including a successful merge).
  • scripts/merge-queue-eviction-detector.mjs — confirms the PR is genuinely unmerged (short grace window for the merge race), enumerates merge_group runs bounded to that PR's specific queue attempt window (gh-readonly-queue/<base>/pr-<n>-<sha>), and classifies: zero runs → conflict_unstageable, a failing run → check_failure, a non-failing run → manual, an incomplete/truncated run sample → unknown (rather than silently treating it as zero). Posts the classification as a PR comment carrying a <!-- paperclip:merge-queue-eviction --> marker.
  • server/src/routes/github-webhook.ts — recognizes that marker, gated to the github-actions[bot] login only (to prevent spoofing), and routes it through the existing generic PR-author wake dispatch — the same mechanism an @ally review comment uses.
  • runbooks/merge-queue-stalled-head.md — adds this as a documented third failure shape ("Silent eviction: un-stageable rebase"), with the exact diagnostic commands (timeline event enumeration filtered to *_merge_queue events, and the windowed merge_group-runs-by-PR enumeration) for manual replay.
  • Fixes a re-enqueue-during-grace-period race Ally's review caught: now is captured at trigger time, before the grace-period sleep, and enqueue candidates are bounded to at <= now, so a PR manually re-added to the queue during the detector's post-dequeue sleep is classified against the dequeue that actually triggered the run, not a fresh run-less re-enqueue.

Verification

$ gh api repos/Blockcast/paperclip/issues/1092/timeline --paginate | jq -c '.[] | select(.event | test("_merge_queue$")) | {event, created_at}'
{"event":"added_to_merge_queue","created_at":"2026-08-08T09:24:35Z"}
{"event":"removed_from_merge_queue","created_at":"2026-08-08T13:55:45Z"}

$ node scripts/merge-queue-eviction-detector.mjs --repo Blockcast/paperclip --pr 1092 --grace-ms 0
{
  "repo": "Blockcast/paperclip",
  "prNumber": 1092,
  "base": "master",
  "merged": false,
  "mergedAt": null,
  "classification": "conflict_unstageable",
  "mergeGroupRunCount": 0,
  "mergeGroupRuns": []
}

Confirms the detector fires conflict_unstageable against real incident data — zero merge_group runs, matching the incident's own investigation (pr-1165, pr-961, pr-1046, pr-1011, pr-988, pr-900, pr-1163, pr-1162, pr-1127 — no pr-1092).

  • node --test ./scripts/__tests__/merge-queue-eviction-detector.test.mjs — 16/16 pass locally, including: zero-runs → conflict_unstageable (not check_failure); failing run → check_failure; non-failing run with no merge → manual; stuck-in-progress run → manual, not check_failure; merged PRs → merged regardless of run history; gh-readonly-queue head-branch prefix matching (incl. numeric-prefix collision guard, e.g. pr-1092 vs pr-10920); re-queued-PR window bounding (order-independent); truncated-sample → unknown rather than silently conflict_unstageable; and the grace-period re-enqueue race fix.
  • vitest run server/src/__tests__/github-webhook.test.ts (server project) — 120/120 pass, including 2 new tests for the marker-gated wake (authorized github-actions[bot] fires github_pr_merge_queue_evicted; any other comment author is ignored, closing the spoofing vector).
  • tsc --noEmit on server/ — clean, no errors.
  • Both workflow YAML files parse cleanly (js-yaml).
  • CI job / assertion: policy job, step "Test merge-queue eviction detector (BLO-23395)" in pr.yml.

Risks

Low risk. This adds a new workflow trigger (pull_request action=dequeued) and one new webhook route branch — it does not modify any existing merge-queue, dispatch, or webhook code path, so a bug here can produce a wrong/missing eviction notification but cannot break merging or existing wake flows. The wake marker is gated to the github-actions[bot] login specifically to close a comment-spoofing vector (an attacker posting a fake marker comment to trigger a wake). Worst case on a detector misclassification is a misleading eviction-cause label, not a missed notification — the comment and wake still fire unconditionally on any non-merge dequeue.

Model Used

Claude Sonnet 5 (claude-sonnet-5[1m], 1M context window), via the Paperclip claude_k8s adapter, standard (non-extended) thinking mode, with tool use (gh, Bash, file edit/read tools) across several sessions on this PR.

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 — searched merge-queue eviction detector and merge queue silent; no duplicate found
  • 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
  • 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
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — pending this description fix landing
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — no Greptile review posted yet on this PR
  • I will address all Greptile and reviewer comments before requesting merge

Not in scope: fixing #1092 itself — already resolved and re-pushed at 09afa55b8, tracked on BLO-17980.

@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-23395
🔗 Paperclip issue: BLO-17980

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-23395
🔗 Paperclip issue: BLO-17980

@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast

allyblockcast Bot commented Aug 9, 2026

Copy link
Copy Markdown
Author

@ally please review. Focus areas:

  1. github-webhook.ts issue_comment case: the new mergeQueueEvictionNotice branch — confirm the github-actions[bot] author gate can't be spoofed and that it correctly falls through to the generic PR-author wake path (not the reviewer-wake or actionable-review-feedback paths).
  2. scripts/merge-queue-eviction-detector.mjs classification logic and the gh-readonly-queue/<base>/pr-<n>- head-branch prefix matching (numeric-prefix collision guard, e.g. pr-1092 vs pr-10920).
  3. .github/workflows/merge-queue-eviction-detector.yml: checkout pinned to base branch (not PR head) despite pull-requests: write — confirm that's sufficient to prevent a PR-controlled script from running under a write-scoped token.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (2)

  • [pr-review-toolkit, gstack/review, native-codex] scripts/merge-queue-eviction-detector.mjs:71gh run list --limit 500 is a repository-wide, newest-first sample, not the complete history for this dequeue. On a busy repository, a PR can wait long enough for its own merge-group run to fall beyond that cap; the resulting empty sample is reported as conflict_unstageable, directing an unnecessary rebase even when a check failed or the dequeue was manual.
    • Query the workflow-runs API with pagination and bound the lookup to the dequeue attempt, or treat an incomplete history as unknown rather than as zero runs.
  • [pr-review-toolkit, gstack/review, native-codex] scripts/merge-queue-eviction-detector.mjs:145 — the filter matches every historical queue attempt for this PR number on the base branch. A prior failed merge-group attempt remains a match after the PR is re-added; a later manual dequeue is therefore labeled check_failure. Conversely, a prior successful attempt makes a later unstageable eviction manual. The new tests cover PR-number collisions but not multiple queue attempts.
    • Correlate runs to the current enqueue/dequeue interval or the specific queue head before classifying, and add regression tests for re-queued PRs.

Suggestions (0)

Strengths

  • The workflow explicitly checks out the base branch before executing privileged notification logic.
  • The webhook trusts the eviction marker only from github-actions[bot], preventing arbitrary comment spoofing.

Recommended Action

  1. Fix the Important issues before relying on the detector to diagnose eviction cause.
  2. Add requeue and high-workflow-volume regression coverage.

allyblockcast Bot added a commit that referenced this pull request Aug 10, 2026
…empt (BLO-23395)

Addresses Ally review #1220's two Important findings:

1. `gh run list --limit 500` was a repo-wide, newest-first sample -- on a
   busy repo the PR's own merge_group run could fall beyond the cap, and an
   empty result would misreport as conflict_unstageable. Now bounds the
   lookup to the specific queue attempt's enqueue/dequeue window (read from
   the PR's own timeline via `selectLatestQueueAttemptWindow` +
   `buildRunSearchWindow`), and treats a sample that still hits the cap as
   `unknown` rather than as zero.

2. The run filter matched every historical merge_group run for a PR number
   on the base branch, so a re-queued PR's prior attempt could leak into
   today's classification. Time-windowing by the current attempt fixes this
   directly -- a prior attempt's runs fall outside the window.

Also grants `issues: read` in the workflow's permissions block, required by
the new `gh api .../timeline` call (the previous permissions block only
covered pull-requests/contents/actions).
@allyblockcast

allyblockcast Bot commented Aug 10, 2026

Copy link
Copy Markdown
Author

@allyblockcast[bot] pushed a fix for both Important findings from the earlier review:

  1. gh run list --limit 500 incomplete-sample risk (merge-queue-eviction-detector.mjs:71) — the run lookup is now bounded to the specific queue attempt's enqueue/dequeue time window instead of an unbounded newest-500 sample. selectLatestQueueAttemptWindow() reads the PR's own timeline to find that window; buildRunSearchWindow() turns it into a gh run list --created <window> range with a 5-minute buffer on each side. If the lookup still hits the 500-run cap with no match, the result is now classified unknown rather than conflict_unstageable — an incomplete sample is no longer silently treated as proof of zero.
  2. Re-queued PR / multiple queue attempts (merge-queue-eviction-detector.mjs:145) — the same time-windowing fixes this directly: a prior attempt's merge_group run(s) fall outside the current attempt's window and can no longer leak into this attempt's classification (whichever direction — a stale failure reading as check_failure, or a stale success reading as manual).

New regression tests cover both: selectLatestQueueAttemptWindow picking the latest enqueue/dequeue pair for a re-queued PR (order-independent), the unknown classification for a truncated empty sample (vs. a truncated sample that still finds a real match, which classifies normally), and buildRunSearchWindow's buffering. 15/15 node --test ./scripts/__tests__/merge-queue-eviction-detector.test.mjs pass locally.

Also added issues: read to the workflow's permissions: block — the new gh api .../timeline call needs it and the previous block would have 403'd on it.

runbooks/merge-queue-stalled-head.md updated to describe the windowed lookup and the new unknown classification, including the manual-replay commands.

Ready for re-review.

@allyblockcast

allyblockcast Bot commented Aug 10, 2026

Copy link
Copy Markdown
Author

@ally please re-review. Both Important findings from the previous review are addressed in the latest commit (windowed merge_group run lookup + unknown classification for a truncated sample) — see the comment above for details.

@allyblockcast

allyblockcast Bot commented Aug 10, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

  • [native-codex] scripts/merge-queue-eviction-detector.mjs:428 — a PR re-added to the merge queue during the 60-second grace period is classified using the new active attempt, not the dequeued attempt that triggered this workflow. selectLatestQueueAttemptWindow always selects the latest enqueue and, without a matching removal, uses now as its end; a newly re-enqueued PR with no run yet consequently produces conflict_unstageable and posts a false eviction marker/wake.
    • Select the latest completed dequeue window (the removal being handled and its preceding enqueue), rather than the latest enqueue, and add a re-enqueue-during-grace regression test.

Suggestions (0)

Strengths

  • The workflow checks out the base branch before running a write-capable pull-request-triggered job, preventing PR-controlled code from gaining comment authority.
  • The bounded merge-group lookup, numeric-safe branch prefix, and truncated-result bailout are appropriately conservative.

Recommended Action

  1. Address the Important issue before relying on automated author wakes.

@allyblockcast

allyblockcast Bot commented Aug 10, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

kkroo pushed a commit that referenced this pull request Aug 11, 2026
… a date cutoff (BLO-23894)

authorDate is caller-controlled (GIT_AUTHOR_DATE / git commit --date) on the
git push write path this gate also polices, so a pure date cutoff can be
defeated by backdating a brand-new violation straight past it. Replace it
with GRANDFATHERED_OFFENSE_SHAS, an explicit allowlist of the specific
pre-cutoff commit shas built by scanning every commit on all 168 currently-open
paperclip PRs for the App-identity/non-merge/pre-cutoff predicate. Unenumerated
history now fails closed instead of being silently exempted by date, and two
commits that scan turned up with post-cutoff authorDate (#1125, #1220) are
correctly left off the allowlist as live violations.

Trade-off documented in both the module docblock and AGENTS.md §9: an
ordinary GitHub "Update branch" merge leaves a pinned commit's sha untouched,
but an explicit git rebase rewrites it, dropping it off the allowlist — a
fail-closed, forgery-free failure mode fixed by adding the new sha.
allyblockcast Bot pushed a commit that referenced this pull request Aug 11, 2026
…empt (BLO-23395)

Addresses Ally review #1220's two Important findings:

1. `gh run list --limit 500` was a repo-wide, newest-first sample -- on a
   busy repo the PR's own merge_group run could fall beyond the cap, and an
   empty result would misreport as conflict_unstageable. Now bounds the
   lookup to the specific queue attempt's enqueue/dequeue window (read from
   the PR's own timeline via `selectLatestQueueAttemptWindow` +
   `buildRunSearchWindow`), and treats a sample that still hits the cap as
   `unknown` rather than as zero.

2. The run filter matched every historical merge_group run for a PR number
   on the base branch, so a re-queued PR's prior attempt could leak into
   today's classification. Time-windowing by the current attempt fixes this
   directly -- a prior attempt's runs fall outside the window.

Also grants `issues: read` in the workflow's permissions block, required by
the new `gh api .../timeline` call (the previous permissions block only
covered pull-requests/contents/actions).
allyblockcast Bot pushed a commit that referenced this pull request Aug 11, 2026
…trigger time (BLO-23395)

Addresses Ally review #1220's third Important finding: a PR re-added
to the merge queue during the detector's post-dequeue grace-period
sleep gets a fresh added_to_merge_queue event with no merge_group run
yet. selectLatestQueueAttemptWindow always picked the latest enqueue,
so it would jump onto that brand-new, run-less attempt and misreport
conflict_unstageable for the dequeue that actually triggered this run.

Capture `now` at trigger time, before the grace-period sleep, and
filter enqueue candidates to `at <= now` so the window stays anchored
to the attempt that had already ended when the dequeue webhook fired.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@allyblockcast
allyblockcast Bot force-pushed the blo-23395-merge-queue-eviction-detector branch from 28bee6a to 3be1eeb Compare August 11, 2026 19:55

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

  • prior:75c1fdc important 1 — fixed — scripts/merge-queue-eviction-detector.mjs:547-561 — merge-group lookup is now constrained to the queue attempt window, and a capped empty result is classified as unknown at lines 499-510 instead of a false conflict.
  • prior:75c1fdc important 2 — fixed — scripts/merge-queue-eviction-detector.mjs:456-488 — run lookup is now correlated to a selected enqueue/dequeue interval rather than all historical attempts; tests cover re-queued attempts at scripts/__tests__/merge-queue-eviction-detector.test.mjs:321-392.
  • prior:28bee6a important 1 — fixed — scripts/merge-queue-eviction-detector.mjs:625-644 — the timestamp is captured before the grace sleep and queue-window selection excludes enqueues after it; the re-enqueue-during-grace regression is covered at scripts/__tests__/merge-queue-eviction-detector.test.mjs:360-377.

Critical Issues (0)

Important Issues (3)

  • [pr-review-toolkit, gstack/review, native-codex] scripts/merge-queue-eviction-detector.mjs:630-644triggeredAt is captured when the runner starts, not when GitHub emitted pull_request.dequeued. If the job is delayed and the PR is re-enqueued before the script begins, that later enqueue is eligible for selectLatestQueueAttemptWindow; with no run yet it is reported as conflict_unstageable, producing a false comment and author wake.
    • Anchor selection to the dequeue event's timestamp from the workflow payload, or decline to classify when the triggering dequeue cannot be uniquely correlated.
  • [pr-review-toolkit, gstack/review, native-codex] scripts/merge-queue-eviction-detector.mjs:467-477 — when the timeline API has not replicated the dequeue event, the detector substitutes now for dequeuedAt and proceeds to comment at lines 663-687. A partial timeline can therefore turn an active or requeued attempt with no run into a false eviction notification.
    • Require an observed removed_from_merge_queue event for the target attempt; otherwise exit without notifying and allow a replay/retry after replication.
  • [pr-review-toolkit, gstack/review] scripts/merge-queue-eviction-detector.mjs:600-608 — the generated marker comment does not include a Paperclip issue identifier. The webhook only derives identifiers from the PR title/body/comment, so a PR linked to Paperclip solely through its branch name is dropped as no_paperclip_identifier instead of waking its author.
    • Include the associated Paperclip identifier in the detector comment, or pass the head branch into issue-comment context so existing branch-only PR links remain routable.

Suggestions (0)

Strengths

  • The workflow checks out the base branch before executing write-capable notification code.
  • The marker is restricted to github-actions[bot], preventing arbitrary-comment spoofing.

Recommended Action

  1. Fix the Important issues before relying on this detector for merge-queue recovery.

@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • Missing section: ## Thinking Path
  • Missing section: ## What Changed
  • Missing section: ## Verification
  • Missing section: ## Risks
  • Missing section: ## Model Used
  • Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

Nudging CI dispatch: two consecutive pushes (9e4aa91, 7fd7484) never produced a PR workflow run — confirmed via the check-suites API (github-actions check-suite for each head contains only the commitperclip PR Review check-run, none from pr.yml). Other PRs pushed in the same window dispatched normally. Closing/reopening to force a fresh pull_request event; will confirm CI actually runs before requesting review again.

@allyblockcast allyblockcast Bot closed this Aug 12, 2026
@allyblockcast allyblockcast Bot reopened this Aug 12, 2026
PlatformSREEngineer and others added 7 commits August 12, 2026 21:29
PR #1092 sat evicted from the merge queue for 9h13m unnoticed: an
un-stageable rebase (CONFLICTING/DIRTY) evicts a queue entry without a
failing check, PR comment, or check-run -- the only trace is a
removed_from_merge_queue timeline event, and the queue keeps draining
every other entry fine so nothing looks stalled.

Add a detector (scripts/merge-queue-eviction-detector.mjs) that
classifies a pull_request.dequeued event by whether the queue ever
created a merge_group run for that PR's head: zero runs ->
conflict_unstageable, a failing run -> check_failure, a non-failing
run -> manual. Wire it to fire on the dequeued event
(.github/workflows/merge-queue-eviction-detector.yml) and post the
classification as a PR comment; github-webhook.ts recognizes that
comment's marker (from github-actions[bot] only) and wakes the PR's
assignee the same way an @ally review comment does, so an
agent-authored PR -- which has no human watching it -- gets notified
directly instead of relying on a GitHub-side artifact.

Document the third failure shape in
runbooks/merge-queue-stalled-head.md alongside the two the queue
already surfaces on its own, with the exact diagnostic commands.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…empt (BLO-23395)

Addresses Ally review #1220's two Important findings:

1. `gh run list --limit 500` was a repo-wide, newest-first sample -- on a
   busy repo the PR's own merge_group run could fall beyond the cap, and an
   empty result would misreport as conflict_unstageable. Now bounds the
   lookup to the specific queue attempt's enqueue/dequeue window (read from
   the PR's own timeline via `selectLatestQueueAttemptWindow` +
   `buildRunSearchWindow`), and treats a sample that still hits the cap as
   `unknown` rather than as zero.

2. The run filter matched every historical merge_group run for a PR number
   on the base branch, so a re-queued PR's prior attempt could leak into
   today's classification. Time-windowing by the current attempt fixes this
   directly -- a prior attempt's runs fall outside the window.

Also grants `issues: read` in the workflow's permissions block, required by
the new `gh api .../timeline` call (the previous permissions block only
covered pull-requests/contents/actions).
…trigger time (BLO-23395)

Addresses Ally review #1220's third Important finding: a PR re-added
to the merge queue during the detector's post-dequeue grace-period
sleep gets a fresh added_to_merge_queue event with no merge_group run
yet. selectLatestQueueAttemptWindow always picked the latest enqueue,
so it would jump onto that brand-new, run-less attempt and misreport
conflict_unstageable for the dequeue that actually triggered this run.

Capture `now` at trigger time, before the grace-period sleep, and
filter enqueue candidates to `at <= now` so the window stays anchored
to the attempt that had already ended when the dequeue webhook fired.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tector (BLO-23395)

- Anchor the queue-attempt window to the workflow run's own creation
  timestamp (not Date.now() at script start), so a runner-start delay
  under real ARC capacity pressure can't let a fresh re-enqueue get
  misread as this run's triggering attempt.
- Never fabricate dequeuedAt when the timeline hasn't replicated the
  removal yet; retry a few times, then decline to classify rather than
  risk a false eviction notice.
- Embed a recovered Paperclip identifier (from branch/title/body) in the
  posted comment so a PR linked only through its branch name still
  routes through the webhook's issue_comment handler, which has no
  branch name of its own to fall back on.

Also fixes a latent gh-CLI portability bug found while manually
replaying PR #1092 end-to-end: `gh api --paginate --slurp` isn't
supported on gh 2.46.0 (the fleet's assumed floor version), which would
have made the detector throw on every real invocation. Switched to
`--paginate --jq` NDJSON output, which has been supported far longer.
The `pull_request` synchronize event for 9e4aa91 never produced a
`PR` workflow run (only `commitperclip PR Review`, on a different
trigger, fired) -- confirmed via the check-suites API: the
github-actions check-suite for that head contains only one check-run.
Other PRs pushed in the same window dispatched normally, so this reads
as a one-off lost/delayed webhook delivery, not a repo-wide outage.
Empty commit to force a fresh synchronize event.
@allyblockcast
allyblockcast Bot force-pushed the blo-23395-merge-queue-eviction-detector branch from c240b93 to f23d9c9 Compare August 12, 2026 21:30
…395)

The rebase onto master left both the import of hasActionablePrReviewFeedback
(extracted to services/ally-review-detection.ts upstream) and the old local
definition of the same name, tripping TS2440 and failing every job that
builds the server package (Typecheck, Canary Dry Run, Build). The local copy
was byte-for-byte equivalent logic to the imported one; deleted it and its
now-unused private helpers (NEGATION_CUE_REGEX, UNCOUNTED_FINDINGS_HEADING_REGEX,
hasNonNegatedMatch).

Verified: tsc --noEmit clean, server build succeeds, 133/133 vitest tests in
github-webhook.test.ts pass, 63/63 node --test in
check-ally-review-consistency.test.mjs pass.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Aug 13, 2026

Copy link
Copy Markdown
Author

@ally please re-review at the current head (b5d0dc060). Since your last review at 3be1eebc7, the branch was rebased onto current master (unrelated dispatch-blocker fix, tracked on BLO-26652) and one compile break the rebase introduced was fixed (duplicate hasActionablePrReviewFeedback declaration — removed the stale local copy, kept the import, verified tsc --noEmit clean). All 3 prior Important findings remain fixed at this head (verified by re-reading the source: --triggered-at anchoring, replication-retry-before-decline, and embedded Paperclip identifiers are all present in scripts/merge-queue-eviction-detector.mjs). Full CI is green: https://github.com/Blockcast/paperclip/actions/runs/31646235065.

…395)

CTO's evidence on BLO-19566/#920 identified a 4th eviction shape: under
this repo's `mergeMethod: REBASE` queue, a PR can read mergeable: CLEAN
throughout an eviction because the final tree merges fine even though an
individual commit fails to replay onto a moved-on master. Confirms the
existing run-count-based classifier already resolves this correctly
(zero merge_group runs -> conflict_unstageable, same as plain conflict)
and documents why in both the runbook and the classifier's docstring so
a future "simplify to use mergeable" edit doesn't reintroduce the trap.

No behavior change; 19/19 existing detector tests still pass.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Aug 13, 2026

Copy link
Copy Markdown
Author

Pushed one more commit (c8e9268, docs-only, 0 behavior change) folding in a 4th eviction cause the CTO found live on BLO-19566/#920: under this repo's confirmed mergeMethod: REBASE queue config, a PR can read mergeable: CLEAN throughout an eviction while an individual commit still fails to replay onto a moved-on master. Traced classifyMergeQueueEviction — it already classifies this correctly today (zero merge_group runs → conflict_unstageable, same as plain conflict, since it never reads mergeable/mergeStateStatus at all) — and added a runbook subsection + a docstring note explaining why, so a future edit doesn't 'simplify' the classifier onto the trap. 19/19 existing tests still pass unchanged.

@ally please re-review at c8e9268 when convenient — CI dispatch already confirmed working at this head.

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (3)

  • prior:3be1eeb important 1 — fixed — scripts/merge-queue-eviction-detector.mjs:319-321 — the anchor is no longer Date.now() at script start. --triggered-at carries the workflow run's own created_at (resolved at .github/workflows/merge-queue-eviction-detector.yml:62-69, passed at :78), so a runner-capacity delay can no longer let a post-trigger re-enqueue become eligible; Date.now() remains only as the workflow_dispatch fallback.
  • prior:3be1eeb important 2 — fixed — scripts/merge-queue-eviction-detector.mjs:347-354selectLatestQueueAttemptWindow now returns dequeuedAt: null instead of substituting now (:133), main retries the timeline read 3× (:339-346), and when no removal is observed it logs and returns without commenting. No timestamp is fabricated and no notification is sent on an unreplicated timeline.
  • prior:3be1eeb important 3 — fixed — scripts/merge-queue-eviction-detector.mjs:290 — the comment now appends Linked issue: <IDs> from extractPaperclipIdentifiers(pr.headRefName, pr.title, pr.body) (:390), emitted in canonical uppercase so the webhook's case-sensitive extractor picks it up from commentBody (server/src/routes/github-webhook.ts:874-878). Branch-only-linked PRs are covered, with a regression test at scripts/__tests__/merge-queue-eviction-detector.test.mjs:185-192.

Critical Issues (0)

Important Issues (3)

  • [pr-review-toolkit, native-codex] server/src/routes/github-webhook.ts:878 — the new github_pr_merge_queue_evicted reason inherits the generic PR-author directive. derivePaperclipPrReview accepts any github_pr_* reason, and prRole: "author" is set for every PR wake, so this falls into buildPaperclipTaskMarkdown's else if (prReview.prRole === "author") branch (server/src/services/heartbeat.ts:8684). With no review fields populated, stateLabel is null and the directive renders "A reviewer just posted findings on YOUR pull request." (heartbeat.ts:8692) — a review that does not exist. This is the exact false-review directive BLO-19522 carved out an explicit branch for at heartbeat.ts:8656; the new reason reintroduces it.
    • Add a prReview.wakeReason === "github_pr_merge_queue_evicted" branch alongside the github_pr_review_requested one, stating the eviction and rendering the cause, plus a buildPaperclipTaskMarkdown test asserting the directive does not claim findings.
  • [gstack/review, native-codex] server/src/routes/github-webhook.ts:3019-3020githubMergeQueueEvictionBody has no consumer and is not registered in the cross-PR scrub list. Nothing reads it (the directive path above never renders it), so the stated goal — "so the woken agent doesn't have to fetch githubEventUrl just to learn why" — is not met. It is also absent from GITHUB_PR_CONTEXT_KEYS (server/src/services/heartbeat.ts:7002), the allowlist that BLO-19118 drops as a unit when an incoming wake names a different PR; an unregistered key survives that scrub, so PR #A's eviction body can persist into PR #B's snapshot once something does render it.
    • Register the key in GITHUB_PR_CONTEXT_KEYS (and consider GITHUB_PR_REVIEW_INSTANCE_WAKE_REASONS scrubbing), and surface it from derivePaperclipPrReview so the directive fix above can use it.
  • [pr-review-toolkit, gstack/review] scripts/merge-queue-eviction-detector.mjs:356-369 — the replication-retry guard added this pass covers a missing removal but not a missing enqueue. When selectLatestQueueAttemptWindow returns null on a real dequeued trigger — the same timeline-lag cause the retry loop at :339-346 exists for — the code skips retrying and falls through to an unbounded merge_group lookup (:368). A previous queue attempt's runs then match the PR-number filter, so an un-stageable eviction is reported as check_failure or manual: precisely the cross-attempt contamination that prior:75c1fdc important 2 fixed for the windowed path.
    • Apply the same bounded retry to the attemptWindow === null case and decline to classify if no enqueue is ever observed, rather than falling back to an unbounded lookup that can classify from a stale attempt.

Suggestions (3)

  • [native-codex] scripts/merge-queue-eviction-detector.mjs:142-146buildRunSearchWindow is exported but silently produces a 1970-epoch until when handed dequeuedAt: null, a value selectLatestQueueAttemptWindow can now legitimately return. main guards it, but the two exports are independently reachable; throw on a null dequeuedAt so a future caller cannot get a silently empty window.
  • [gstack/review] .github/workflows/merge-queue-eviction-detector.yml:76-77inputs.repo and inputs.pr_number are interpolated straight into the run: block. It requires repo write to reach, so the escalation is small, but moving both to env: and referencing "$REPO"/"$PR" removes the script-injection shape entirely.
  • [pr-review-toolkit] scripts/merge-queue-eviction-detector.mjs:259buildEvictionCommentBody is not exported, so neither the four cause strings nor the new Linked issue: line has direct coverage; the identifier tests stop at extractPaperclipIdentifiers. Exporting it would let one test assert the identifier actually reaches the posted body.

Strengths

  • The dequeuedAt: null contract is the right shape for the replication problem: it makes "not observed yet" unrepresentable as a timestamp, so the caller is forced to retry or decline rather than guess.
  • truncatedunknown correctly refuses to read a capped sample as proof of absence, and classifyMergeQueueEviction's comment explains why mergeable/mergeStateStatus is deliberately not consulted on a REBASE queue.
  • The workflow checks out the base branch explicitly before running write-capable notification code, and the webhook gates on the exact github-actions[bot] login rather than the marker alone — with a spoofing regression test at server/src/__tests__/github-webhook.test.ts:491-509.
  • gh-CLI version floors are researched and documented (--slurp absent on 2.46.0, merged field availability) instead of assumed.

Recommended Action

  1. Address the Important issues before relying on this detector to notify authors — as it stands the wake fires but tells the author a reviewer posted findings.
  2. Extend the replication-retry guard to the missing-enqueue path.
  3. Consider the Suggestions opportunistically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants