ci: add the stranded-Ally-review reconciler, status-free predicate (BLO-28203) - #1436
Conversation
…LO-28203) Follow-on from BLO-22892, which landed this reconciler on Blockcast/trafficcontrol (#1383). A reviewer wake fired by `pull_request.opened` can be silently declined or starved with no retry and no alert; Ally does not patrol the board, so a lost wake stays lost and the PR sits unreviewed, indistinguishable from one merely waiting its turn. This repo carried the ~13-PR loss set measured on BLO-22892 (2026-08-15/16), so the exposure here is demonstrated, not theoretical. Runs on a schedule, not an event, so it does not depend on the wake path it backstops. Exits non-zero once a head has been awaiting review past the alarm threshold despite an automated re-fire, so the stranded condition is visible on the Actions tab rather than not visible at all. PREDICATE_MODE=status-free, and that is the load-bearing decision here. Unlike trafficcontrol and pim-multicast-gateway, this repo has NO `review/ally-complete` producer: measured 2026-08-20, 0 of 100 open PRs carried that status. A status-keyed sweep would classify every PR as "not pending" and silently do nothing forever -- the exact failure this exists to prevent, one layer up. So the sweep keys on the load-bearing half instead: non-draft, open, and no Ally consolidated report at the exact head, past a threshold. Standing up the `review/ally-complete` gate here first was the considered alternative and was rejected: it adds a commit status to a repo with a merge queue, changing merge behaviour for a reconciler meant to be observe-and-re-fire only -- larger blast radius, no gain in detection. Because status-free mode has no status pre-filter to bound the candidate set, MAX_REFIRES_PER_RUN (default 5) caps writes per run so the first run cannot request reviews on the whole open-PR tail at once. Over-budget PRs are reported as deferred and still counted and still alarm -- they are not dropped, because a rate limit that silenced the alarm would reinstate the defect this exists to clear. Measured before-state on this repo (dry run, 2026-08-20): considered=136 refired=5 alarming=20 failed=0 deferred=18 The predicate was verified in both directions against this repo before shipping: all 5 PRs it would re-fire (#1431 #1428 #1424 #1418 #1406) have no Ally review at any head, and 6 of 6 sampled PRs it excluded carry a real `Reviewed head: <sha>` attestation at the exact head. 28 drafts skipped. The head-attestation predicate is inlined rather than imported: the trafficcontrol original `importlib`s a sibling `require-ally-review.py`, and this repo has no such file. `claude[bot]` (id 209825114) is deliberately not in ALLY_REVIEWER_LOGINS -- counting it is the exact false negative that hid trafficcontrol#1448. The script is byte-identical to the copy landing on Blockcast/pim-multicast-gateway; only the workflow env differs, so the two cannot drift. 56 unit tests pin the pure decision logic and run as a workflow step. Refs: https://paperclip.blockcast.net/BLO/issues/BLO-28203 Co-Authored-By: Paperclip <noreply@paperclip.ing>
![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: 42dd9b2
This is a carefully argued port and the predicate work is genuinely good — the bias correction (fail toward re-firing), the two-surface check, and the user.type == "Bot" requirement are all right, and the 56 tests pass. I reproduced the test suite (Ran 56 tests ... OK) and the dry-run counts are consistent with the live repo (108 non-draft + 28 draft = 136 open PRs).
The blocking finding is that the reconciler's own alarm — the AC4 mechanism this PR exists to deliver — fails open. I verified it by executing the script from this head, not by reading it.
Critical Issues (1)
-
[native-codex]
.github/scripts/sweep-stalled-ally-reviews.py:798— The run exits 0 (green) even when every PR failed to evaluate, so a systematically broken sweep is indistinguishable from a healthy one.failedis computed at line 745 and rendered into the step summary, but the exit decision keys only onalarming. Per-PR isolation insweep()records a failure as(pr, head_sha, None, False, "skip: error ...")—pending_sinceis forced toNone, andis_alarming()returnsFalseforpending_since is None. So every unevaluated PR is silently laundered into "not alarming".Reproduced against the script at this exact head, mocking a 403 rate-limit response for all 30 PRs:
considered=30 refired=0 alarming=0 failed=30 deferred=0 === EXIT CODE: 0 ===Nothing was reconciled, 30 PRs went unevaluated, and the scheduled job reports success. This contradicts the file's own contract in two places — the AC4 comment at line 798 ("the stranded condition must surface without a human noticing by hand") and the summary comment at line 762 ("A PR we could not even evaluate is not 'clean'"). It is also the precise defect class this reconciler exists to eliminate: a silent failure that reads as health.
Note the ordering makes it worse than the aggregate suggests.
GET /pulls?state=openreturns newest-first, so when calls start failing partway through, the PRs dropped are the oldest — the most stranded ones, exactly the population the alarm is for.- Fail the run on systematic failure, not just on
alarming. I'd avoid "any error ⇒ red" (one transient 5xx shouldn't page anyone) — key it on a proportion instead, e.g. exit non-zero whenlen(failed) > 0 and len(failed) >= max(3, 0.1 * len(results)), and keep a distinct exit message so a failed-sweep red is not misread as a stranded-PR red. Please also add amain()-level test asserting the exit code for the all-failed case; the suite currently covers the reason string (test_failed_pr_is_marked_with_the_error_prefix_not_a_clean_skip) but never asserts the exit status.
- Fail the run on systematic failure, not just on
Important Issues (3)
-
[gstack/review]
.github/scripts/sweep-stalled-ally-reviews.py:214—urllib.request.urlopen(req)is called with notimeout=, so it inherits the global default socket timeout, which isNone(block forever) — the script never callssocket.setdefaulttimeout(). The workflow also sets notimeout-minutes, andconcurrency.cancel-in-progress: false(.github/workflows/review-gate-sweep.yml:55) means queued runs will not displace a hung one. One stalled TCP connection therefore hangs the job to the Actions 6-hour default while every subsequent 30-minute run queues behind it — a multi-hour reconciler outage from a single bad socket. The siblingally-review-consistency.ymlin this repo already setstimeout-minutes: 10.- Pass an explicit
timeout=(15–30s) tourlopen, and addtimeout-minutes: 15to thesweepjob to match the sibling workflow.
- Pass an explicit
-
[pr-review-toolkit/errors]
.github/scripts/sweep-stalled-ally-reviews.py:214— No rate-limit handling or backoff, and the per-run call volume is high enough that exhaustion is a routine outcome rather than an edge case. Instatus-freemode every non-draft PR costs at least 3 calls (head commit + comments page + reviews page), so with the 108 non-draft PRs measured on this repo:108 × 3 + 2list pages+ ~20re-fire writes ≈ 346 calls per run, more where a PR exceeds one page of comments or reviews. At9,39 * * * *that is ~700 calls/hour from this job alone, against the documentedgithub.tokenbudget of 1,000 requests/hour/repository, shared with the 26 other workflows in.github/workflows. When the limit trips, every remaining PR raisesHTTPError— and by the Critical above, the run goes green. These two findings compound; fixing only the exit code leaves the sweep silently degraded, and fixing only the volume leaves the blind spot.- Detect
403/429withx-ratelimit-remaining: 0and treat it as a distinct fatal outcome (non-zero exit, explicit message) rather than 108 individual per-PR errors. Cheap volume reduction available from the list payload alone:pending_since = max(created_at, committer_date) ≥ created_at, so any PR withnow - created_at < STALL_THRESHOLD_SECONDScannot re-fire — skip its three fetches entirely.
- Detect
-
[pr-review-toolkit/code]
.github/scripts/__pycache__/sweep-stalled-ally-reviews.cpython-313.pyc:1— Two compiled bytecode artifacts are committed (83KB total: this file plustest_sweep_stalled_ally_reviews.cpython-313.pyc), evidently a side effect of running the tests locally. I confirmed both are present in the tree at this head and that.gitignorecontains no__pycache__or*.pycentry, so they will be re-staged by anyone who runs the suite and will churn or conflict on every subsequent change to these scripts.- Delete both files and add
__pycache__/+*.pycto.gitignore.
- Delete both files and add
Suggestions (2)
-
[native-codex]
.github/scripts/sweep-stalled-ally-reviews.py:743—is_alarming({"is_draft": False, "pending_since": r[2]}, now)hardcodesis_draft: Falserather than carrying the real flag. Correct today only because_consider_prleavespending_sinceatNonefor drafts, so the draft branch is unreachable — but that is an invariant held in a different function, and the guard silently stops guarding if it ever changes. Carrying the realis_draftthrough the result tuple would make the safety local. -
[gstack/review]
.github/scripts/sweep-stalled-ally-reviews.py:559—unreviewed_since()usescommit.committer.date, which is client-settable and not guaranteed to be in the past. A head with a future-dated committer timestamp (clock skew, or a rewritten date) yields a negativeage, so it can never reachSTALL_THRESHOLD_SECONDSorALARM_THRESHOLD_SECONDS— the PR becomes permanently invisible to both re-fire and alarm. That is the suppression direction the module docstring explicitly identifies as the dangerous one. Clamping withmin(candidate, now)would close it.
Strengths
- The predicate bias argument at
ally_has_reviewed_head()is the right call and well documented — requiring positive evidence (consolidated envelope and exact head attestation) so a false "reviewed" cannot suppress recovery, rather than the intuitive-but-backwards direction. - Rejecting mutable
review.commit_idin favour of the immutable body attestation, with the frr#29 precedent cited, is exactly right. - The
user.type == "Bot"check on both surfaces correctly separates the App from theallyblockcastUser account — a subtle distinction that has caused real false negatives. - The
DELETE+POSTdiscovery (a bare POST for an already-requested reviewer returns 200 and fires no event) is a genuinely non-obvious API behaviour, measured rather than assumed. - Deferred PRs still counting toward
consideredand still alarming is the right design — a rate limit that silenced the alarm would reinstate the defect. PREDICATE_MODEis well justified: a status-keyed sweep on a repo with no producer would silently no-op forever, and that reasoning is recorded where the next reader will find it.
Recommended Action
- Fix the Critical exit-code gap before merge, with a
main()-level test asserting the exit status — without it the reconciler's alarm cannot be trusted, which is the PR's central deliverable. - Address the three Important issues this cycle. The timeout and rate-limit items are small changes that materially affect whether this job keeps running unattended; the
.pycremoval is a one-liner plus a.gitignoreentry. - Consider the two Suggestions opportunistically.
…8203) Addresses Ally's review of #1436 at 42dd9b2. CRITICAL -- the reconciler's alarm failed open. main() keyed its exit solely on `alarming`, but per-PR isolation records an unevaluated PR as `pending_since=None`, and is_alarming() reads None as not-alarming. So a run where every PR raised printed `alarming=0 failed=30` and exited 0: a systematically broken sweep was indistinguishable from a healthy one, which is the exact "silent failure that reads as health" defect this reconciler exists to remove, reintroduced one layer up. Reproduced independently before fixing (30 mocked 403s -> exit 0) and again after (exit 2). - sweep_is_degraded() keys the new exit on a proportion, not `failed > 0`, so one transient 5xx does not page anyone: failed >= max(3, 10%). - EXIT_ALARM=1 and EXIT_SWEEP_DEGRADED=2 are distinct. Both are red, but "go review PR #N" and "the PR list was never read" are different instructions and must not be conflated. The alarm is checked first, so a known-stranded PR still reports as one. The top-level HTTPError/URLError handlers exit DEGRADED too -- reaching them means nothing is known about any PR. - main() reads is_draft off the PR payload instead of hardcoding False. The literal was safe only via an invariant held in _consider_pr. Rate limits and timeouts, the two ways this job dies unattended: - Every request is bounded (REQUEST_TIMEOUT_SECONDS, 30s). urlopen's default is None -- block forever -- and with `cancel-in-progress: false` one stalled socket would hang the job to the Actions 6h ceiling and queue every later run behind it. `timeout-minutes: 15` is the outer bound. - 403/429 carrying `x-ratelimit-remaining: 0` (or retry-after) raises RateLimitExhausted, which aborts the loop instead of grinding out one identical failure per remaining PR. The unevaluated remainder is still recorded as failed, so the run goes red rather than reporting a short list it never finished reading. A permissions 403 stays an HTTPError -- that is not the budget and retrying never fixes it. - Cadence halved to hourly. Measured 2026-08-21: 147 open PRs, 119 non-draft/unlocked -> ~359 requests per run, so `9,39` was ~718/hour against github.token's 1,000/hour/repository shared with 26 other workflows. Ally's suggested predicate-level cut was measured too and saves only 3 of those 359 calls here (one open PR is younger than the stall threshold); it is kept because it is free, but cadence is the lever that moves the number. ALARM_THRESHOLD_SECONDS goes 4.5h -> 5.5h to absorb the coarser granularity and keep "a re-fire and its cooldown have come and gone" true before alarming. Also: clamp commit.committer.date to now. It is client-settable, so a future-dated head yielded a negative age that could never reach the stall OR alarm threshold -- permanently invisible to both, the one suppression direction this module's predicate bias refuses. Thread `mode` through to build_comment_body. Soften the ALARM_THRESHOLD comment, which claimed the alarm proves "the re-fire didn't work either" when the code checks only elapsed time. Drop two committed .pyc artifacts and ignore bytecode. Tests 56 -> 84, including the main()-level exit-code assertions the review asked for: all-failed exits DEGRADED, one transient failure still exits 0, a stranded PR exits ALARM, and a stranded PR outranks a degraded run. Live dry-run at this head: considered=147 refired=5 alarming=27 failed=0 deferred=22, exit 1.
…as the alarm (BLO-28203) Follow-up to Ally's review of the sibling port in Blockcast/pim-multicast-gateway#2399. Two of its three Important findings do NOT apply to this repo and are deliberately not ported: - checkout pinning: @v5 is this repo's MAJORITY convention (23 of 49 checkout usages, vs 2 on @v4), so pinning here would be importing another repo's convention, not fixing a defect. - request/job timeouts: already present -- REQUEST_TIMEOUT_SECONDS on every urlopen and timeout-minutes 15 on the sweep job. What does apply, and is fixed here: 1. The sweep's 1013-line decision suite ran ONLY inside the scheduled review-gate-sweep job, where a non-zero exit is the deliberate stranded-PR alarm (EXIT_ALARM=1). A test regression therefore read as "a PR is stranded", and a change to the reconciler could merge without its suite ever running at PR time. Now runs in pr.yml's policy job beside the other gate-policy tests. 2. New: a top-level OSError arm. A REQUEST_TIMEOUT_SECONDS expiry during the response *read* raises bare TimeoutError (== socket.timeout), which is an OSError but NOT a URLError, so it escaped every existing arm as an uncaught traceback -- and CPython exits 1, which is EXIT_ALARM. The timeout that exists to bound a hung request could therefore report itself as "a PR is stranded, go review it", sending a human to look for work that does not exist. It now exits EXIT_SWEEP_DEGRADED like every other transport failure. setup-python rather than a bare python3 because the policy job runs on arc-merge-queue for merge_group events, that scale set is at zero replicas so its image's python is unverifiable, and this job gates every PR and merge-queue candidate. setup-python is image-independent and is this repo's own pattern (docker-figma-bot.yml, docker-webflow-bot.yml), verified green on the ARC fleet. Verified: 84 tests pass on 3.10-compatible syntax (no 3.11+ constructs); pr.yml and review-gate-sweep.yml both parse. Co-Authored-By: Claude <noreply@anthropic.com>
Ally review on the sibling port — what ports here, and what deliberately does notAlly reviewed the sibling port (pim-multicast-gateway#2399) and raised three Important findings. Rather than copy all three across, I checked each against this repo's measured state. Pushed
1. The decision suite now runs at PR timeIt previously ran only inside the scheduled Now a step in
On pim the opposite choice was correct — bare 2. A socket timeout could report itself as the stranded-PR alarmNot in Ally's findings — surfaced while implementing the timeout half on pim, and it is worse here precisely because this copy has distinct exit codes. A So it escaped every arm in the Now caught and mapped to Verification84 tests pass locally; |
CI confirmation at
|
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: 21169c6
Prior Findings Dispositioned (4)
- prior:42dd9b2 critical 1 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:952— degraded sweeps are detected from failed evaluations and exit withEXIT_SWEEP_DEGRADEDinstead of reporting a green run. - prior:42dd9b2 important 1 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:270— every API request uses the explicitREQUEST_TIMEOUT_SECONDStimeout, and the workflow adds a 15-minute job timeout at.github/workflows/review-gate-sweep.yml:85. - prior:42dd9b2 important 2 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:274— exhausted primary or secondary rate limits raiseRateLimitExhausted, abort the remaining scan, and produce a degraded non-zero result; young PRs are also skipped before per-PR fetches at.github/scripts/sweep-stalled-ally-reviews.py:746. - prior:42dd9b2 important 3 — no-longer-applicable —
.gitignore:82— the previously committed bytecode artifacts are absent from the current tree and Python cache/bytecode patterns are now ignored.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The reconciler now distinguishes stranded PR alarms from an unevaluable or rate-limited sweep.
- The two-surface, exact-body head-attestation predicate is covered by focused tests, and the suite passes with 84 tests.
- The status-free mode avoids relying on a producerless check and reduces unnecessary API calls for young PRs.
Recommended Action
- No Critical or Important issues remain from the reviewed head; this PR is suitable for merge.
Thinking Path
Linked Issues or Issue Description
Blockcast/trafficcontrol(#1383)sweep/ally/review-gate, all PR states) — nothing existing or prior covers this.What Changed
.github/workflows/review-gate-sweep.yml— scheduled9,39 * * * *, plusworkflow_dispatch. Offset from trafficcontrol (7,37) and pim-multicast-gateway (8,38) so three repos' sweeps do not fire reviewer re-requests at Ally in the same instant..github/scripts/sweep-stalled-ally-reviews.py— finds non-draft open PRs with no Ally consolidated report at the exact head past a threshold, re-fires viaDELETE+POSTonrequested_reviewers(a bare POST for an already-requested reviewer returns 200 and creates no event, so it delivers no wake), posts a marker comment as audit trail and cooldown input, and exits non-zero when a PR is still stranded past the alarm threshold..github/scripts/test_sweep_stalled_ally_reviews.py— 56 tests, run as a workflow step.Two things differ from the trafficcontrol original, both forced by measurement:
PREDICATE_MODE: status-free. This repo has noreview/ally-completeproducer — measured 2026-08-20, 0 of 100 open PRs carry that status. A status-keyed sweep here would classify every PR as "not pending" and silently do nothing forever, which is the failure this reconciler exists to prevent, reintroduced one layer up. Standing up the gate here first was the considered alternative and was rejected: it adds a commit status to a repo with a merge queue, changing merge behaviour for a reconciler meant to be observe-and-re-fire only — larger blast radius, no gain in detection.importlibs a siblingrequire-ally-review.py; this repo has no such file.claude[bot](id 209825114) is deliberately absent fromALLY_REVIEWER_LOGINS— counting it is the exact false negative that hid trafficcontrol#1448.Because status-free mode has no status pre-filter to bound the candidate set,
MAX_REFIRES_PER_RUN(default 5) caps writes per run so the first run cannot request review on the whole open-PR tail at once. Over-budget PRs are reported asdeferred, still counted, and still alarm — a rate limit that silenced the alarm would reinstate the defect being fixed.Verification
Unit tests —
python3 -m unittest discover -s .github/scripts -p 'test_sweep_*.py'→ 56 tests, OK. They run as a workflow step, so a broken predicate fails the sweep job rather than silently mis-sweeping. New tests coverunreviewed_since()in both directions, the deferral cap (including that a deferred PR still alarms),--dry-runissuing no write calls, and the mode-aware comment body.Measured before-state on this repo (dry run of the exact script in this PR; writes nothing):
23 open PRs are stranded right now; 20 are past the alarm threshold, so the first scheduled run will exit non-zero and go red. That red run is the point — the strand becoming visible without anyone checking by hand.
The predicate was verified in both directions against live data before shipping, because a wrong predicate here either spams Ally or suppresses recovery:
Reviewed head: <sha>attestation at the exact headReproduce read-only:
(with a token exported for
GITHUB_TOKEN).Risks
Low. No merge behaviour changes — this adds no commit status and no required check; it only reads PRs and, for a genuinely stranded one, issues a review request plus one marker comment, throttled by a 2h cooldown and capped at 5 per run.
The one behavioural risk is predicate error, and it is deliberately biased: a false "already reviewed" would suppress recovery and leave the PR stranded silently (the original defect), while a false "not reviewed" costs one redundant review request. The predicate therefore requires positive evidence — the
## Ally … Consolidated PR Reviewenvelope and an exactReviewed head:match anduser.type == "Bot"— rather than erring toward "reviewed".review.commit_idis deliberately not trusted; it is mutable and has been observed reporting a head it never reviewed.The script is byte-identical to the copy in pim-multicast-gateway#2399; only the workflow configuration differs, so the two cannot drift.
Model Used
Claude (Anthropic) —
claude-opus-5[1m], 1M context, extended thinking, run via Claude Code with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code