feat(land-clean-prs): report approvals that rotted, sweep multiple repos by allyblockcast[bot] · Pull Request #1957 · Blockcast/paperclip

allyblockcast · 2026-09-20T12:03:27Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

The landing routine (scripts/land-clean-prs.mjs, BLO-32511) is the subsystem that decides which open PRs may be handed to the merge queue and names a reason for every one it declines

A second, quieter waste sits in the same population and nothing was watching it: a PR that earned a human approval and then went CONFLICTING because master moved underneath it. The conflicting commit lands on the BASE, so no synchronize, no pull_request event and no check re-run fires — and mergeStateStatus is not a check, so it cannot be expressed as a gateSignals monitor either. Nothing wakes the assignee, ever

It needs addressing because human review throughput on this fleet measures ~2.4 actions/day, and this cohort has been cleaned up by hand four times (BLO-29984, BLO-31321, BLO-32205, BLO-33208) and regrown in the same repos every time. A fifth cleanup buys a sixth recurrence

This pull request folds the standing detector into the sweep that already enumerates that population, rather than shipping it as the separate scheduled workflow it started as

The benefit is one classifier, one receipt, and no new credential: the standalone version needed an org secret no agent can provision, so its schedule would have been permanently red and its alarm state indistinguishable from its broken state (BLO-33268)

Linked Issues or Issue Description

Refs BLO-33208 — the ticket this implements; supersedes Blockcast/trafficcontrol#1815, closed in favour of this.
Refs BLO-32606 — the path-filter suppression that makes the DIRTY rule ordering load-bearing.
Refs BLO-32240 / BLO-32511 — the script and routine this extends.

What Changed

approvalLanes(pr) splits APPROVED reviewers into human and bot lanes, keyed on the REST user.type and never on a [bot] login suffix. Only a human approval spent a scarce resource; conflating the two overstated BLO-33208's own cohort by ~2.7x.
A conflicted PR holding any approval is reported as its own approval-rotted action, so the receipt tally carries the count instead of burying it among every other skip. Reported, never acted on.
checkSettlement(rollup) refuses an all-green rollup that is either empty (nothing attested this head, which renders identically to everything passing) or whose newest row is under 15 minutes old (green only because the reds have not registered yet). Rows carrying no parseable timestamp count as settled — holding them would be permanent, and --auto re-gates server-side.
targetRepos() accepts a comma-separated LAND_CLEAN_PRS_REPO. Sweeping one repo is how the other two stayed unobserved between cleanups.
DIRTY is now decided ahead of the check rules. GitHub cannot evaluate a paths: filter on a PR whose merge commit will not compute, so path-filtered workflows are silently never dispatched and the surviving checks mean nothing — measured on onprem-k8s#3269: 4 workflow runs while dirty, 22 for the identical tree once mergeable.
Draft PRs are held (skip / draft). Separate commit, and found by running the sweep rather than reading it — see Verification.

Verification

node --test ./scripts/land-clean-prs.test.mjs     # 43 pass, 0 fail (was 31)

Twelve assertions ported from the closed detector's suite onto this classifier. Every guard was mutation-tested — reverted individually, one per run, confirming the suite goes red; a guard with no failing mutation is a comment, not a test:

mutation	failing tests
`approvalLanes` keys on `[bot]` suffix instead of `type`	3
empty rollup reads as a pass	3
settling floor removed	3
`targetRepos` stops splitting on `,`	3
`approval-rotted` collapses back into `skip`	5
DIRTY checked after the check rules	6
draft rule removed	3

Also driven live, dry-run, against Blockcast/multicast,Blockcast/trafficcontrol — the multi-repo loop, the per-repo receipt and the tally all behave. approval-rotted is currently 0 across both, consistent with the cohort BLO-33208 already measured down from 8.

That live run is what found the draft bug. trafficcontrol#1726 classified enqueue while draft: true, with a body opening "Draft, and blocked by design. Do not merge before magma#1936" — the two repos carry a byte-identical cdni_log.proto under one package and share a field-number space, so landing that half alone is the BLO-29906 breakage. The classifier never read isDraft; the field was not even requested. GitHub would have refused the auto-merge, so nothing breaks today — but that is luck, not a rule, and it was latent only because the routine swept one repo. The multi-repo change here is what makes it reachable, which is why the fix ships alongside it.

Risks

Rule reordering changes reported reasons. A DIRTY PR that also has a red check now reports mergestate:DIRTY rather than checks:…. That is the intent — on a conflicted PR the check reading is not trustworthy — but anything parsing the receipt's reason strings would see the shift. Nothing does today.
The 15-minute floor defers a land by one fire in the window right after checks report. Deliberately not the 24h the reporting-only detector used: that floor existed to avoid paging a human, and this script acts. Overridable via LAND_CLEAN_PRS_SETTLE_MINUTES.
The empty-rollup stop can over-hold a repo that genuinely publishes no checks at all; such a PR now never enqueues and says checks:none. Over-hold, never over-enqueue.
Multi-repo is opt-in — the env var is unset in the existing routine, so this is inert until someone widens it deliberately. Widening it exposes repos whose PR conventions this classifier has not been measured against, which is exactly how #1726 surfaced.
Low risk overall: the new paths only ever withhold an action or report. No new action is taken on any PR, and approval-rotted deliberately has no applyRow branch.

Model Used

Claude Opus 5 (claude-opus-5[1m]), 1M context, extended thinking, tool use via Claude Code / Paperclip claude_k8s adapter.

Checklist

Folds the standing DIRTY-after-APPROVAL detector into the script that already sweeps PRs, per the CTO ruling on BLO-33208. The standalone version (trafficcontrol#1815) is closed: it required a repo secret no agent can provision, so its twice-daily schedule would have been permanently red and its alarm state indistinguishable from its broken state (BLO-33268). An approval is the scarcest signal on a PR — measured fleet throughput is ~2.4 human review actions/day — and it is spent silently when master moves underneath the branch. The conflicting commit lands on the BASE, so no `synchronize`, no `pull_request` event and no check re-run fires, and `mergeStateStatus` is not a check so it cannot be a `gateSignals` monitor either. Nothing wakes the assignee. That cohort has been hand-cleaned four times (BLO-29984, BLO-31321, BLO-32205, BLO-33208) and regrown in the same repos every time. Three deltas over what already shipped: - `approvalLanes` splits APPROVED reviewers human vs bot, keyed on the REST `user.type` and never on a `[bot]` login suffix. Only a human approval spent a scarce resource; conflating them overstated BLO-33208's own cohort by ~2.7x. A conflicted PR holding one is reported as its own `approval-rotted` action so the receipt tally carries the count rather than burying it among every other skip. - `checkSettlement` refuses an all-green rollup that is empty (nothing attested this head, which renders identically to everything passing) or whose newest row is under 15m old (green only because the reds have not registered yet). Rows with no parseable timestamp count as settled — holding them would be permanent, and `--auto` re-gates anyway. - `targetRepos` accepts a comma-separated `LAND_CLEAN_PRS_REPO`. A single-repo sweep is how the other two repos stayed unobserved between cleanups. DIRTY now decides ahead of the check rules, which is load-bearing rather than cosmetic: GitHub cannot evaluate a `paths:` filter on a PR whose merge commit will not compute, so path-filtered workflows are silently never dispatched and the surviving checks mean nothing (BLO-32606, measured on onprem-k8s#3269 — 4 runs dirty, 22 for the identical tree once mergeable). Reported, never acted on. A deliberate sequencing hold is indistinguishable from a strand on every API surface; trafficcontrol#1726 was exactly that, and a rebase would have broken a shared proto field-number space. Refs: BLO-33208 Co-Authored-By: Paperclip <noreply@paperclip.ing>

Found by driving the sweep against trafficcontrol rather than by reading it. `#1726` classified `enqueue` while `draft: true`, with a body that opens "Draft, and blocked by design. Do not merge before magma#1936" — the two repos carry a byte-identical `cdni_log.proto` under one package, so they share a field-number space and landing this half alone is the BLO-29906 breakage. The classifier never read `isDraft`; the field was not even requested in `PR_LIST_FIELDS`. GitHub would have refused the auto-merge, so nothing would have broken today, but that is luck rather than a rule, and it evaporates the moment someone marks a PR ready while its sequencing constraint still holds. A wasted enqueue also spends the per-fire cap and fails identically on every subsequent fire. Draft is the author's own "not this one" — the same category as SKIP_LABELS — and it is the only machine-readable form a deliberate hold reliably takes, so it is decided in the listing rules alongside them and costs no extra API call. Latent until now only because the routine swept one repo; the multi-repo change in the previous commit is what makes it reachable. Refs: BLO-33208 Co-Authored-By: Paperclip <noreply@paperclip.ing>

allyblockcast · 2026-09-20T12:03:34Z

🔗 Paperclip issue: BLO-32205
🔗 Paperclip issue: BLO-33268
🔗 Paperclip issue: BLO-31321
🔗 Paperclip issue: BLO-32511
🔗 Paperclip issue: BLO-32606
🔗 Paperclip issue: BLO-29984
🔗 Paperclip issue: BLO-33208
🔗 Paperclip issue: BLO-29906
🔗 Paperclip issue: BLO-32240

allyblockcast · 2026-09-20T12:07:57Z

@ally please review at head 659be01 — folding the BLO-33208 approval-rot detector into scripts/land-clean-prs.mjs (supersedes the closed trafficcontrol#1815).

Review focus, in priority order:

Rule reordering is the riskiest change. classifyPr now decides mergestate:DIRTY before reading checks. Rationale is BLO-32606 (path-filtered workflows are never dispatched on a PR whose merge commit will not compute, so surviving checks mean nothing). Please check I have not made some other reason unreachable by moving it.
checkSettlement direction. Empty rollup → hold; under-floor → hold; non-empty but no parseable timestamp → pass. That third branch is deliberately the opposite of the first — holding undatable rows would be permanent, since a commit status never moves on its own, and --auto re-gates server-side. Is that the right call, or should it hold?
approvalLanes ignores head. An approval counts as spent whether or not the branch has moved since. Intentional (the resource was consumed either way), but it differs from unsatisfiedOwners, which does pin commit_id. Worth confirming the asymmetry reads as deliberate.
The draft guard (second commit) is a behaviour change to the existing classifier, not just the new detector. It holds any draft PR. Found by running the sweep against trafficcontrol, where #1726 — draft, body reading "Do not merge before magma#1936" — classified enqueue.

Every guard is mutation-tested (each reverted individually, suite confirmed red); the table is in the PR body.

github-actions · 2026-09-20T14:20:47Z

@ally head 659be01 has been awaiting review for 2.3h with no review on either surface (pulls/1957/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.