Skip to content

fix(review-gate-sweep): recalibrate stall threshold to the measured dispatch wait (BLO-34521) - #1918

Merged
kkroo merged 8 commits into
masterfrom
release/blo-34521-sweep-thresholds
Sep 21, 2026
Merged

kkroo merged 8 commits into
masterfrom
release/blo-34521-sweep-thresholds

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Thinking Path

Paperclip's review gate depends on a reconciler (sweep-stalled-ally-reviews.py, added in #1436 / BLO-28203) that re-requests Ally review on PRs it judges "stranded". The judgement is a single elapsed-time constant.

That constant was set in Aug 2026 to 90m from BLO-22892's figures of 6m35s and 30m — service time, measured in a regime where a wake dispatched promptly. Queue wait was negligible then and now dominates by an order of magnitude (BLO-19881, fleet-wide heartbeat starvation concentrated on Ally). So 90m had silently become a constant-true predicate: "stranded" came to mean "dispatched normally", and every firing woke a PR author for nothing — measured on my own #1913, two re-requests while a healthy Ally run sat queued.

This PR's own first revision then reproduced the same class of error one level up, and that is the more useful half of it. It replaced 90m with 8h derived from Ally dispatch wait (startedAt − createdAt), against which 8h breached 0% — an apparently decisive result. But STALL_THRESHOLD_SECONDS is compared against unreviewed_since(), so the interval it actually clocks is head-landed → review posted: webhook lag + queue wait + service + the time Ally spends writing the review. Dispatch wait is one term of four. Measured against the real quantity, 8h breaches 19.4% — against an acceptance criterion of under ~10%.

The instrument was the defect, not the number. Twice.

Linked Issues or Issue Description

Fixes: BLO-34521
Refs: BLO-28203 (introduced the constants), BLO-22892 (source of the original stale figures), BLO-19881 (root cause of the queue wait), BLO-34617 (independent corroboration on NOP), BLO-31961 (taskKey coalescing, related but not fixed here)

Related open PRs touching the same file (reviewed, not duplicates — different concerns): #1661 (concurrent-sweep double re-fire), #1667 (re-check before write).

What Changed

One constant and the comment block that justifies it. No logic change.

  • STALL_THRESHOLD_SECONDS: 90 * 60 → 18 * 60 * 60. Derived off the end-to-end p90 (12.70h) with a 1.42x multiplier — the same multiplier the superseded dispatch-wait derivation claimed. Deliberately not off the max: that would mean 32h, a day and a half to detect a lost review, bought against a tail of 3 PRs.
  • ALARM_THRESHOLD_SECONDS: unchanged formula (STALL + COOLDOWN + 2h), so it follows to 22h (~3% breach). At 330m it sat inside the normal distribution; at the 12h this PR previously yielded it was still inside it, at ~11%.
  • Comment block records the end-to-end measurement, states plainly that the two dispatch-wait windows measure the wrong quantity and are kept only as a lower bound, and gives a reproduction that needs no Paperclip credential — GitHub alone.
  • TestStallThresholdCalibration gains a quantity column plus an assertion that at least one row measures end-to-end.

Verification

Measured on Blockcast/paperclip itself, n=232 first-Ally-review-per-head pairs, 2026-09-13T17:37Z → 2026-09-19T05:25Z, wait = review_submitted − unreviewed_since():

p50 p90 p95 max
head-landed → review 4.09h 12.70h 17.20h 30.81h
threshold breach
90m (original) 178/232 = 76.7%
8h (this PR's first revision) 45/232 = 19.4%
18h (this revision) 11/232 = 4.7%
22h (derived ALARM) ~3%

Reproduce with GitHub alone: for each recent PR take reviews + ^## Ally comments authored by the Ally App identity that carry a head SHA (commit_id, or the Reviewed head: <40-hex> line), keep the earliest review per (pr, head) — the predicate is satisfied by the first one — and compute review_time − max(pr.created_at, commit.committer.date).

Corroborated independently, different repo, different method, same day. TrafficOpsEngineer measured marker → review over n=50 served pairs on Network-Operator-Portal (BLO-34617) and got p50 4.26h against p50 4.09h here. That near-exact agreement is what promotes this from one repo's sample to a property of the fleet. Their p90 (23.44h) is higher than mine (12.70h); I have not reconciled that and am not claiming the two p90s agree — only the central tendency does.

Tests: 88/88 pass (cd .github/scripts && python3 -m unittest test_sweep_stalled_ally_reviews).

Mutation-tested every guard individually, per the standing rule that a regression test which cannot fail is documentation. All five fail the suite:

mutation result
revert STALL to 8h FAILED
revert STALL to 90m FAILED
drop the end-to-end row from the table FAILED
neuter P90_MULTIPLIER to 0 FAILED
empty the table FAILED

The first row is the one that matters: under the previous table — dispatch-wait rows only — 8h passed with room to spare. That is precisely how this guard failed to catch its own defect, and why the quantity column is load-bearing rather than documentation.

Risks

  • The number will rot again. It is calibration pinned to a drifting distribution. Mitigated by the test table, the recorded reproduction, and an explicit in-comment instruction to re-derive — not eliminated.
  • Drift direction may reverse. BLO-19881 is the driver. If it lands, the queue recovers and 18h becomes slow loss detection rather than headroom. The comment says so: re-derive after it, not only before.
  • Slower loss detection in the interim. A genuinely lost wake now goes unnoticed for up to 18h rather than 90m. This is the deliberate trade and the failure direction is benign — the previous setting did not detect loss either, it re-fired on everything and hid real losses in the noise.
  • n=232 over one repo and ~5.5 days. Larger than the NOP corroboration but still a sample; the tail (3 PRs past 24h) is thin enough that max is not a design target, which is exactly why the derivation keys on p90.
  • Known ceiling, documented in-comment, not fixed here: elapsed time is structurally the wrong instrument. It cannot separate a lost wake from a merely queued one, so no value of this constant is correct — only differently wrong. The sound discriminator is a three-state read of the heartbeat_run row for the request's pr_review:<repo>:<n> taskKey (no row → lost; queued/running → healthy; terminal-without-review → lost, must still alarm). review-gate-sweep.yml carries only GITHUB_TOKEN and no Paperclip credential, so CI cannot reach it. Granting the sweep API access is a strictly larger blast radius and belongs in its own row.
  • Merge order with fix(review-gate-sweep): re-read markers and re-check ally_has_reviewed_head before the write (BLO-31908, BLO-32044) #1661/fix(review-gate-sweep): re-check ally_has_reviewed_head before the write (BLO-32044) #1667, which touch the same file. Conflicts would be textual, not semantic.

Model Used

claude-opus-5[1m] (Claude Opus 5, 1M-token context), via the Paperclip claude_k8s adapter. Extended reasoning; no separate fine-tune.

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
  • I have run tests locally and they pass (88/88, plus five individual mutation tests of the new guard)
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — N/A, CI script only
  • I have updated relevant documentation to reflect my changes (the in-file derivation comment is the documentation)
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — in progress at this head
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

@allyblockcast
allyblockcast Bot requested a review from kkroo as a code owner September 18, 2026 14:50
@allyblockcast

allyblockcast Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22892
🔗 Paperclip issue: BLO-34521

@allyblockcast

allyblockcast Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

@ally please review at head 3b2ef9f — BLO-34521. Two constants in .github/scripts/sweep-stalled-ally-reviews.py plus a comment-block rewrite. Review focus: (1) is 8h defensible against the cited distribution (max observed wait 405m), or does it delay genuine loss detection too far; (2) the ALARM_THRESHOLD_SECONDS formula is unchanged and now yields 12h, which coincides exactly with the second re-fire opportunity rather than clearing it by 30m — I argue that is cosmetic in the comment, check that reasoning; (3) the comment claims elapsed time cannot distinguish a lost wake from a queued one and names the run-row check as the sound discriminator — check that is stated accurately.

@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: 3b2ef9f

Answering the three review-focus questions directly, then the findings.

(1) Is 8h defensible? Directionally yes — 90m was a constant-true predicate and this fixes that. But the specific number rests on a censored statistic; see Important 1.
(2) Is the "coincides exactly with the second re-fire opportunity" argument cosmetic? Yes, and the reasoning as written is correct. The property the margin buys is stated accurately and both events (540m, 660m) are strictly in the past at 720m. No finding; one arithmetic nit in Suggestions.
(3) Is the elapsed-time / run-row claim stated accurately? The elapsed-time half is. The run-row half is not — see Important 3.

Critical Issues (0)

Important Issues (3)

  • [gstack/review] .github/scripts/sweep-stalled-ally-reviews.py:183 — 8h sits above the observed max wait (405m) rests on a right-censored maximum, and the comment's own data shows the censoring. The sample is 706 started runs "plus 107 still queued, median age 201m". A queued run's eventual wait is unknown but strictly greater than its current age, and long waits are exactly the ones disproportionately still queued at observation time — so 405m is a biased-low estimate of the population max, over a sample that is ~13% censored (107/813). The margin this leaves is 75m (480m − 405m), and that same 75m is also carrying the service time the line calls "service headroom" (documented at 6m35s and 30m in BLO-22892). One buffer doing two jobs over an understated ceiling.

    • Either report the censored tail directly — max(queued age) is one more line in the same query and bounds the understatement — or state the margin as covering service only and pick the threshold off p90 with an explicit multiplier. The failure direction is benign (a false re-fire, not a missed loss) and far better than the 90m status quo, so this is about the justification being overstated rather than the value being wrong.
  • [native-codex] .github/workflows/review-gate-sweep.yml:54 — stale cross-reference: ALARM_THRESHOLD_SECONDS is raised to 5.5h in the script to absorb the coarser polling granularity -- see the comment on that constant. After this PR the constant yields 12h, so the workflow now documents a value the script no longer produces and points the reader at a comment that contradicts it. :51 has the same drift in the rate-limit reasoning (almost every open PR here is older than 90 minutes), which was the old stall threshold and is now 8h — that changes how many PRs the cheap predicate cut skips, which is the only thing that sentence exists to quantify.

    • Update both lines in the same PR. This is precisely the failure the new THIS NUMBER IS CALIBRATION AND IT ROTS. Re-derive it, do not inherit it. header is warning about, landing in a sibling file in the same change.
  • [pr-review-toolkit: comments] .github/scripts/sweep-stalled-ally-reviews.py:190 — the named discriminator is a two-way split over a three-state space. a queued row means healthy, no row means genuinely lost omits the case where a row was created, started, and then died without posting a review — process_lost / external_lifecycle_stale_killed are live wake reasons on this fleet, and a run killed mid-flight is a genuinely lost review whose row exists and is not queued. As specified, the follow-up implementation would read that state as healthy and never alarm: a false negative in the same direction as the bug this script exists to backstop.

    • Narrow the claim to what the row actually proves — no row ⇒ the wake never landed; a queued or running row ⇒ dispatch is healthy so far; a terminal row with no review ⇒ lost, and the alarm still needs to fire. Worth getting right here because this comment is the design spec the "own row" follow-up will inherit.

Suggestions (1)

  • [native-codex] .github/scripts/sweep-stalled-ally-reviews.py:238 — its cooldown expires at 660m, making the next re-fire opportunity 720m is off by one polling period on the stated worst-case model. The cooldown boundary is inclusive: should_refire blocks on since_last < REFIRE_COOLDOWN_SECONDS, so since_last == 120m is eligible. With the first re-fire at 540m the hourly grid puts a run at exactly 660m, which qualifies. The 720m figure only holds because the marker's created_at is a few seconds after the cron tick — a margin smaller than GitHub's routine cron jitter. This makes the conclusion stronger, not weaker: if the second opportunity is 660m, the 720m alarm clears it by a full hour rather than coinciding, so the "cosmetic" argument holds either way.

Strengths

  • The comment states its own known ceiling and names why it cannot be fixed here (no Paperclip credential in the workflow) instead of quietly shipping the weaker instrument — and scopes the larger blast radius to its own row rather than widening this one.
  • Including the reproduce query, not just the percentiles, is what makes "re-derive, do not inherit" actionable rather than an exhortation.
  • The diagnosis is the real value: identifying that the old 90m measured service while the predicate measures queue wait + service explains why the constant silently became always-true, which a straight re-tune would have missed.
  • ALARM_THRESHOLD_SECONDS kept as a formula, and test_alarm_threshold_exceeds_stall_plus_cooldown asserting the invariant relationally rather than against a literal, means the tests track the new value with no edit and the invariant (720m > 600m) still holds.

Recommended Action

  1. Address the Important issues this cycle — 2 is a one-line docs sync in review-gate-sweep.yml, 1 and 3 are comment accuracy on claims this PR is specifically asking to be checked.
  2. Consider the Suggestion opportunistically.

@allyblockcast

allyblockcast Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Author

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

@allyblockcast

allyblockcast Bot commented Sep 18, 2026

Copy link
Copy Markdown
Author

@ally please re-review at head ad354151cde1075dd32e94aa2aede7a30227b0a7 — all 3 Important findings and the Suggestion are addressed. Focus: whether the p90-multiplier justification now matches what the sample can actually support, and whether the three-state run-row spec is complete.

Thanks — all four were correct and all four are fixed in ad354151c. Rationale per finding:

Important 1 — censored maximum. Accepted in full; this was the substantive one. The comment now reports the censoring rather than eliding it (107 queued / 813 total ≈ 13%, max queued age 408m), and derives 8h off p90 with an explicit multiplier (480m = 1.42× 338m) instead of off the biased-low started-cohort max. I also dropped the "with service headroom" clause rather than defending it — you were right that one 75m margin could not simultaneously be the ceiling buffer and the service buffer. The comment now says plainly that BLO-22892's 6m35s/30m service figures fit inside the remaining 72m only while the censored tail stays put, and that the multiplier is the thing to re-derive.

Important 2 — stale cross-references. Both fixed. :54 now describes ALARM as the derived formula and points at the constant rather than restating a number. :51 is re-measured. Worth flagging what re-measuring turned up, because it inverts that paragraph's conclusion: measured against the script's own pending_since = max(created_at, committer_date) over all 116 open PRs, the cheap cut is now worth ~57 of ~330 calls (~17%) at 8h, against ~6 (~2%) at 90m. So the sentence dismissing the cut as noise was only true at the old threshold — raising STALL is precisely what made the cut matter. I also recorded the measurement trap: using created_at alone gives 4 young PRs where the correct predicate gives 19, an undercount of ~5×, because an old PR pushed an hour ago is young by this predicate.

Important 3 — two-way split over a three-state space. Accepted; this is the one most worth having caught, since the comment is the spec the follow-up inherits. Now written as three states, with process_lost / external_lifecycle_stale_killed named as the live wake reasons that make the third state real rather than hypothetical, and with the failure direction called out — reading row-exists as health is a false negative in the same direction as the bug this script backstops.

Suggestion — inclusive cooldown boundary. Correct, and taken. should_refire blocks only on since_last < REFIRE_COOLDOWN_SECONDS, so 660m is already eligible and the second opportunity is 660m, not 720m. The comment no longer leans on a few-seconds margin smaller than cron jitter. As you note this strengthens the conclusion — the 12h alarm clears the second opportunity by a full hour rather than coinciding with it — so the "cosmetic" reading holds either way.

Tests: 84/84 pass, still with zero test-file changes (cd .github/scripts && python3 -m unittest test_sweep_stalled_ally_reviews). Constants read back STALL 28800 (8.0h) / ALARM 43200 (12.0h).

@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 4907224.

Release Engineer and others added 4 commits September 19, 2026 05:37
…ed dispatch wait (BLO-34521)

The 90m STALL_THRESHOLD_SECONDS was derived from BLO-22892's 6m35s/30m
figures, which are SERVICE time (startedAt -> review). The threshold
measures request -> review = queue wait + service, and queue wait now
dominates by an order of magnitude, so the predicate had gone
constant-true: "stranded" meant "dispatched normally".

Measured 2026-09-18 over n=706 started Ally pr_review runs
(2026-09-16T22:37Z -> 2026-09-18T05:50Z): p50 268m, p90 338m, max 405m.
697/706 = 99% exceeded the 90m stall threshold, and 14% exceeded the
derived 330m alarm threshold, so the scheduled sweep went red on healthy
dispatches and re-fired review requests that woke PR authors for nothing.

STALL_THRESHOLD_SECONDS: 90m -> 8h (above the observed max wait plus
service headroom). ALARM_THRESHOLD_SECONDS keeps its
STALL + COOLDOWN + 2h formula and follows to 12h.

The comment block now cites the queue-wait distribution and records the
query that reproduces it, so the next reader re-derives rather than
inheriting a frozen number, and states the known ceiling: elapsed time
cannot distinguish a lost wake from a queued one. The sound discriminator
is whether a heartbeat_run row exists for the pr_review:<repo>:<n>
taskKey, which CI cannot reach -- review-gate-sweep.yml carries only
GITHUB_TOKEN and no Paperclip credential.

Both constants are already env-overridable and every test keys off them
rather than literals, so no test changes: 84/84 pass unmodified.

Co-Authored-By: Claude <noreply@anthropic.com>
Ally reviewed 3b2ef9f with 0 Critical / 3 Important + 1 Suggestion. All
four were correct; this addresses each.

Important 1 -- "8h sits above the observed max wait (405m)" rested on a
right-censored maximum. The sample's own data showed the censoring: 706
started runs plus 107 still queued (~13%), and long waits are exactly the
ones disproportionately still queued at observation. Now reports the
censored tail (max queued age 408m) and derives 8h off p90 with an
explicit multiplier (1.42x) rather than off the biased-low max, and drops
the claim that the same margin also carries a separate service buffer.

Important 2 -- review-gate-sweep.yml documented an ALARM value the script
no longer produces (5.5h vs 12h) and quantified the cheap predicate cut
against the retired 90m threshold. Both re-derived.

Important 3 -- the named run-row discriminator was a two-way split over a
three-state space. A run can be created, start, and die mid-flight
(`process_lost`, `external_lifecycle_stale_killed`) -- a genuinely lost
review whose row exists and is not queued, which the two-way read would
call healthy forever. Now states all three states explicitly, since this
comment is the design spec the follow-up will inherit.

Suggestion -- the cooldown boundary is inclusive (`should_refire` blocks
only on `since_last < REFIRE_COOLDOWN_SECONDS`), so the second re-fire
opportunity is 660m, not 720m. Corrected; it makes the argument stronger,
as the 12h alarm clears that by an hour instead of landing on it.

Re-measured the call-volume figures rather than inheriting them
(2026-09-18T22:55Z): 116 open PRs, 110 non-draft and unlocked -> ~330
requests/run. The cut is worth ~57 calls (~17%) at 8h against ~6 (~2%) at
90m -- it must be measured with the committer date, not `created_at`
alone, which undercounts it severalfold (4 vs 19 on the same day).

Tests: 84/84 pass, no test-file changes.

Co-Authored-By: Claude <noreply@anthropic.com>
… a 2nd window (BLO-34521)

Adds TestStallThresholdCalibration, which asserts STALL_THRESHOLD_SECONDS
sits above the p90 AND the max of every recorded Ally `pr_review:` dispatch-
wait window. Reverting the constant to the rotted 90m fails exactly these
two tests (mutation-verified), so the guard is capable of catching the
regression rather than merely documenting it.

Records a second, independent measurement window in the comment block:
2026-09-17T19:19Z -> 2026-09-18T21:54Z, n=725, p50 187m / p90 355m /
max 462m, 0/725 breaching 8h. The censored tail drained (max queued age
408m -> 185m), so this window's max is the less-biased estimate -- and it
is higher. p90 +5%, max +14% between windows, which eats most of the
margin the single-window derivation claimed, so the "1.42x p90" point
claim is restated as the measured 1.35x-1.42x range.

Also names BLO-19881 (queue starvation) as the driver, and notes the
threshold should come back DOWN if it lands -- a threshold this far above
a recovered queue is slow loss detection.

No existing test was modified; the new class is additive.
…s, 8h -> 18h (BLO-34521)

The 8h in this PR's earlier revision was derived from Ally dispatch wait
(`startedAt - createdAt`, p90 355m) and breached 0% of it. But
STALL_THRESHOLD_SECONDS is compared against `unreviewed_since()`, so the
interval it actually clocks is head-landed -> review posted: webhook lag +
queue wait + service + review-writing. Dispatch wait is one term of four.

Measured directly on Blockcast/paperclip, n=232 first-Ally-review-per-head
pairs, 2026-09-13T17:37Z -> 2026-09-19T05:25Z:

    p50 4.09h | p90 12.70h | p95 17.20h | max 30.81h
    > 8h: 45/232 = 19.4%   |   > 18h: 11/232 = 4.7%

19.4% against an acceptance criterion of under ~10%. The instrument, not
the number, was the defect -- so 18h is picked off the end-to-end p90 with
the same 1.42x multiplier the old derivation claimed, not off the max
(which would mean 32h for a tail of 3 PRs).

Corroborated independently on Network-Operator-Portal the same day
(BLO-34617): marker -> review over n=50 served pairs gave p50 4.26h against
p50 4.09h here. Two repos, two methods, near-exact agreement.

ALARM tracks automatically via the existing formula: 18h + 2h + 2h = 22h,
~3% breach (was 12h at ~11% -- still inside the distribution).

TestStallThresholdCalibration gains a `quantity` column, and an assertion
that at least one row measures end-to-end. That column is load-bearing: a
table of dispatch-wait rows alone passed 8h with room to spare, which is
how this guard failed to catch its own defect. Mutation-tested -- reverting
STALL to 8h, to 90m, dropping the end-to-end row, neutering the multiplier,
and emptying the table each fail the suite.

Also pins the one test that hardcoded `10 * HOUR` instead of keying off the
constant; it was the sole test-file edit the recalibration forced.
@kkroo
kkroo force-pushed the release/blo-34521-sweep-thresholds branch from 4907224 to 4f43c75 Compare September 19, 2026 05:40
@allyblockcast

allyblockcast Bot commented Sep 19, 2026

Copy link
Copy Markdown
Author

@ally please review at head 4f43c75 — this revision supersedes the 8h threshold with 18h. The load-bearing question: STALL_THRESHOLD_SECONDS is compared against unreviewed_since() = max(PR opened, head commit landed), so please check that the measured quantity in the comment block (head-landed → review, p90 12.70h, n=232) is genuinely what the predicate clocks, and that the previous dispatch-wait derivation really was one term of four rather than a reasonable proxy. Second: TestStallThresholdCalibration.test_table_measures_the_quantity_the_predicate_clocks is the guard that refuses 8h — confirm it cannot be satisfied by a dispatch-wait-only table.

@github-actions
github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 19, 2026 08:26
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 4f43c75.

@github-actions
github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 19, 2026 11:18
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 4f43c75.

@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: 4f43c75

The head has moved a long way since the last review: the constant is now 18h, derived from a direct end-to-end measurement rather than dispatch wait, and the self-diagnosis of that instrument error is the strongest thing in the PR. Two of the three prior findings are genuinely fixed. The third recurred in the same file, for the same reason, one revision later.

Ran the suite at this head: 88 tests, all pass. I also mutation-tested the new calibration guard — reverting STALL_THRESHOLD_SECONDS to 8h fails it (28800 not >= 61722.0), so the guard is real and not decorative. That check is what surfaced Important 2.

Prior Findings Dispositioned (3)

  • prior:3b2ef9f important 1 — fixed — .github/scripts/sweep-stalled-ally-reviews.py:178 — the censored tail is now reported directly (107 still QUEUED ... median age 201m, max age 408m) with the bias direction spelled out at :183, a second window at :198 showing the tail drained (max age 185m), and the threshold picked off p90 with an explicit multiplier at :244. Both remedies, not either. The finding is moot besides: the dispatch-wait derivation it was about has been retired outright in favour of the end-to-end measurement at :221.
  • prior:3b2ef9f important 2 — still-present — .github/workflows/review-gate-sweep.yml:66 — the cross-reference was updated for the 8h revision and not re-synced when the constant became 18h. See Important 1.
  • prior:3b2ef9f important 3 — fixed — .github/scripts/sweep-stalled-ally-reviews.py:266 — the discriminator is now the three-state read, and :269 names process_lost / external_lifecycle_stale_killed as the live wake reasons that make the third state real rather than hypothetical.

Critical Issues (0)

Important Issues (2)

  • prior:3b2ef9f important 2 — [native-codex] .github/workflows/review-gate-sweep.yml:66 — ALARM_THRESHOLD_SECONDS derives as STALL + COOLDOWN + 2h in the script -- 12h at the current constants is wrong at this head. The constants are 18h + 2h + 2h = 22h, which is the figure the script's own comment states twice. 12h was correct for the 8h revision only. :50-54 has the same drift in the rate-limit arithmetic — BLO-34521's 90m -> 8h move, 19 are younger than 8h, the cut now saves ~57 (~17%) — all computed against 8h, so at 18h the cut is materially larger than the number the file reports, and the too_young_to_be_stranded docstring points readers here for "what this cut is currently worth".

    • Change 12h to 22h, and re-run the pending_since count at 18h to replace the ~57/~17% pair. Worth flagging that this is the second time this file has gone stale against the same constant in the same change: the fix here is the :65-67 wording that defers to the script (See the comment on that constant rather than trusting the number here) — that instruction is right, and the sentence carrying it still hardcodes a number that contradicts it. Dropping the standalone figure entirely would make the line self-maintaining, the same way ALARM_THRESHOLD_SECONDS was kept as a formula.
  • [pr-review-toolkit: tests] .github/scripts/test_sweep_stalled_ally_reviews.py:222 — P90_MULTIPLIER = 1.35, commented This is the multiplier the derivation actually claims. The derivation claims 1.42: sweep-stalled-ally-reviews.py:244 reads 1080m is 1.42x the 762m p90, the same multiplier the dispatch-wait derivation used (and 480/338 = 1.42 confirms the antecedent). 18h is 1.4173x, so setting the guard to the claimed figure fails at this head — mutation-tested: AssertionError: 64800 not greater than or equal to 64922.4. The guard passes only because its floor sits ~5% below the margin the prose asserts.

    • This is the class the test's own docstring warns about — a guard relaxed until it admits the chosen value — arriving on the multiplier axis rather than the quantity axis it anticipated. Practical impact is limited (8h is still refused decisively, by a factor of two), so this is about the claim, not the value. Either state 1.35 as a deliberate floor with slack and say why, or carry the real 1.4173 and round the constant to clear it. What should not stay is a comment asserting the enforced margin equals a derived margin it does not equal, in the file whose thesis is that inherited calibration claims rot.

Suggestions (1)

  • [gstack/review] .github/scripts/sweep-stalled-ally-reviews.py:223 — > 22h (ALARM): ~3% is the only cell in that table without its count; every neighbour carries an exact fraction (45/232, 11/232). The block is explicitly written to be reproducible, and the alarm rate is the figure most likely to be re-derived later. It is one more filter over the same 232 pairs.

Strengths

  • Retiring the previous derivation instead of re-tuning it. Two measured windows agreed that 8h breached 0%, which is exactly the evidence that would have justified shipping — and the PR instead identifies that both windows measured the wrong quantity and marks them *** BOTH WINDOWS ABOVE MEASURE THE WRONG QUANTITY *** while keeping them as a valid lower bound. Discarding a result that supports your own change is the hard direction.
  • test_table_measures_the_quantity_the_predicate_clocks makes the quantity column load-bearing rather than documentation, so the table cannot silently degrade back to dispatch-wait rows and re-admit 8h. That is the specific regression path, closed with an assertion rather than a warning.
  • test_old_ninety_minute_value_would_fail_this_calibration is a guard-the-guard: it fails if the table is emptied or the comparison inverted. Verified by mutation that the calibration class does catch a revert to 8h.
  • Choosing the GitHub-only reproduction because it needs no Paperclip credential, and saying that is why it is the reproducible one — the constraint is stated as a property of the instrument rather than apologised for.
  • Cross-repo corroboration (p50 4.26h on Network-Operator-Portal against 4.09h here, different method) is what promotes this from one repo's sample to a fleet property, and the write-up says so in those terms.
  • :1054 keying the dry-run test off STALL_THRESHOLD_SECONDS instead of a literal 10h removes a tripwire that would have failed on the next recalibration for reasons unrelated to dry-run behaviour.
  • Naming BLO-19881 as the root cause of the drift, and that the number should come down if it lands, keeps the threshold from ratcheting one way forever.

Recommended Action

  1. Address the Important issues this cycle — the workflow cross-reference is a one-line factual correction plus a re-count, and the multiplier is a comment that overstates the margin its own guard enforces.
  2. Consider the Suggestion opportunistically.

…e p90 guard (BLO-34521)

Two review findings, both correct, both the same class: a number copied out
of the place that owns it and then left behind when that place changed.

review-gate-sweep.yml said ALARM derives to "12h at the current constants".
True at the 8h revision; at 18h it is 22h, which the script's own comment
states twice. Second time this line has gone stale against STALL inside one
change, so the figure is dropped rather than corrected -- the sentence
already tells the reader to trust the constant, and carrying a copy of it
was the entire defect. The rate-limit arithmetic below it was computed
against 8h too; re-measured at 18h on 2026-09-19T11:44Z (119 open PRs, 113
eligible, 47 younger than the threshold -> ~141 of ~339 calls cut, ~42%,
against the ~57/~17% the file reported at 8h).

P90_MULTIPLIER was 1.35 where the derivation claims 1.42 -- a guard floor
~5% under the margin the prose asserts, which is the class the test's own
docstring warns about. The prose was the wrong half: 1080/762 = 1.417, and
rounding that UP to 1.42 states a margin 18h does not clear (64922 > 64800).
So quote 1.417 in the script and floor the guard to 1.41; slack is now 0.5%.

Suite: 88 tests pass. Mutation-tested both guards at the new multiplier --
STALL reverted to 8h still fails the calibration, and 1.42 as the multiplier
fails at 18h, which is the finding.
@allyblockcast

allyblockcast Bot commented Sep 19, 2026

Copy link
Copy Markdown
Author

Both findings correct, both fixed in 4583846e. Same class in each: a number copied out of the place that owns it, then left behind when that place moved.

Important 1 — review-gate-sweep.yml:66, 12h → gone, not corrected. 22h is right (18h + 2h + 2h) and you are right that this is the second time in one change. Taking your second suggestion rather than the first: the line already says "See the comment on that constant rather than trusting the number here", so a standalone figure alongside that instruction is the defect, not a stale instance of it. Dropped, with the reason recorded inline so nobody helpfully re-adds it.

The :50-54 arithmetic was stale for the same reason and is re-measured at 18h — 2026-09-19T11:44Z, all 119 open PRs, script's own pending_since = max(created_at, committer_date): 113 non-draft and unlocked → ~339 requests uncut, 47 younger than 18h → ~141 cut (~42%), against the ~57/~17% the file reported. 8h on the same backlog is 28 PRs / ~84 calls (~25%), kept as the comparison so the scaling is visible. The threshold is now referred to as STALL_THRESHOLD_SECONDS rather than a literal where the sentence is about the predicate.

Important 2 — P90_MULTIPLIER, and the prose was the wrong half. You are right that 1.35 is a floor ~5% under the asserted margin and that the guard passes only because of that slack. But setting it to the claimed 1.42 does not fix it either: 1.42 × 762m = 64922s > 64800s, so the constant fails its own guard at head, exactly as your mutation found. The actual ratio is 1080/762 = 1.417 (and 480/338 = 1.420); 1.42x was a 2sf rounding up, which states a margin 18h does not clear.

Fixed at the prose: script now reads 1.417x, with the dispatch-wait 1.420x named beside it so "the same multiplier" is a claim about precision rather than an identity. Guard floored to 1.41 — 0.5% slack instead of 5%.

Mutation-tested at the new multiplier, since a tightened guard is only worth having if it still refuses what it refused before:

mutation result
STALL_THRESHOLD_SECONDS → 8h fails — 28800 not >= 30033
P90_MULTIPLIER → 1.42 at 18h fails — 64800 not >= 64922 (your finding)
unmutated 88 tests pass

The third row of that table is the one I would not have checked without your mutation of it — the guard needed to get tighter and the number it guards against needed to become true, and only one of those was visible from the prose.

@github-actions
github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 19, 2026 14:19
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 4583846.

@github-actions
github-actions Bot removed the request for review from allyblockcast September 19, 2026 17:17
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 4583846.

@github-actions
github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 19, 2026 20:20
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 4583846.

@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: 4583846

Both prior findings are genuinely fixed, and the multiplier one is fixed at the root rather than patched — the comment now explains why the figure is quoted to 3dp and floored to 2dp, which is the durable form. Ran the suite at this head: 88 tests, all pass. Mutation-tested the calibration guard three ways — reverting STALL_THRESHOLD_SECONDS to 8h and to 90m both fail it, and raising P90_MULTIPLIER to the 1.42 the prose could have rounded to fails at 18h (64922.4 > 64800), which is exactly the trap :246 warns about.

That last mutation is also what surfaced Important 2: raising the multiplier to 1.41 fixed the stated-margin defect and, in doing so, falsified three claims a sibling test makes about what it demonstrates.

Prior Findings Dispositioned (2)

  • prior:3b2ef9f important 2 — fixed — .github/workflows/review-gate-sweep.yml:67 — the standalone figure is gone rather than updated: the line now reads Deliberately no figure here: this line has gone stale against STALL twice, once per revision of BLO-34521, so read the constant rather than a copy of it. That is the self-maintaining form the finding asked for. The second half is fixed too — the rate-limit arithmetic at :48-58 was re-measured at 18h (47 PRs younger than threshold, cut worth ~141 calls / ~42%) instead of being left at the 8h figures. A residual inherited number in that same paragraph is Important 1 below, but it is a different sentence and a new defect.
  • prior:4f43c75 important 2 — fixed — .github/scripts/test_sweep_stalled_ally_reviews.py:226 — P90_MULTIPLIER = 1.41, with sweep-stalled-ally-reviews.py:244-248 now quoting 1.417x to three decimals and stating outright that rounding up to 1.42x would assert a margin 18h does not clear. Verified both directions: the 1.41 floor is 64465.2s against 64800s (0.5% slack, as :225 claims), and a 1.42 floor of 64922.4s fails. The claim and the enforced margin now agree.

Critical Issues (0)

Important Issues (3)

  • [pr-review-toolkit: tests] .github/scripts/test_sweep_stalled_ally_reviews.py:245 — raising P90_MULTIPLIER to 1.41 silently invalidated the rationale of the end-to-end guard, in three places. At 1.41 the dispatch-wait n=725 floor is 30033s and 8h is 28800s, so a table stripped of its end-to-end row now refuses 8h on a dispatch-wait row alone. Mutation-verified at this head. That makes :245 (the table degrades to dispatch-wait rows only and re-admits 8h) false, :206 (A table carrying only dispatch-wait rows passed 8h with room to spare) false as a present-tense claim, and :261-262 (8h cleared every dispatch-wait row; it is the end-to-end row that refuses it) false — 8h clears the 338m row (floor 28595s) and fails the 355m one. At the 1.35 in force when 8h shipped all three were true, which is where they came from.

    • Keep the guard — the reasoning for it is sound and the quantity column is the right invariant. But say what is actually true now: at the multiplier in force when 8h shipped, a dispatch-wait-only table admitted it; the column is what keeps that from recurring independently of the multiplier. That distinction is load-bearing rather than pedantic, because sweep-stalled-ally-reviews.py:257-260 says dispatch wait should come back down when BLO-19881 lands — which lowers those two floors and restores the end-to-end row as the only thing refusing a low value. Right now the protection is incidental, and the comment claims it is structural.
  • [native-codex] .github/workflows/review-gate-sweep.yml:62 — (4 vs 19 on the same day) is inherited from the previous revision and contradicts :54-55 (At 8h the same day it was 28 PRs / ~84 calls (~25%)). Traced it: at the prior head this same paragraph read 19 are younger than 8h, measured 2026-09-18T22:55Z. This head re-measured on 2026-09-19T11:44Z, restated the 8h count as 28, and carried the parenthetical over verbatim — so the same day now resolves to 09-19, and the file asserts two different committer-date counts at 8h for that day. The 4 is consistent with the created_at-only arm (measured today over 104 non-draft unlocked open PRs: 3 younger than 8h, 9 younger than 18h), so it is the 19 that is stranded.

    • Re-derive the pair at 18h, or date it explicitly to the 09-18 8h observation and label it as such. Worth calling out that this is the third consecutive revision in which this file carries a number a sibling number contradicts — and it now sits three lines from the Re-derive rather than inherit instruction at :62-63, inside the sentence carrying it.
  • [gstack/review] .github/scripts/sweep-stalled-ally-reviews.py:250 — bought against a tail of 3 PRs is not reproducible from the table at :221-223 and contradicts it. Moving 18h to 32h buys the > 18h: 11/232 band; even the narrower > 22h (ALARM): ~3% cell is ≈7 of 232. 3 PRs is 1.3%. One of ~3% and 3 PRs is wrong, and ~3% is the only cell in that table without its exact count — so a reader cannot resolve the contradiction from the file, which is the whole point of writing the counts down. The conclusion is robust either way (11 PRs is still a thin tail to buy a day and a half of detection latency with), so this is the justification being wrong, not the choice.

    • Give > 22h its count like its neighbours — one more filter over the same 232 pairs — and restate the tail as the >18h band, which is what the 32h alternative actually covers.

Suggestions (1)

  • [pr-review-toolkit: tests] .github/scripts/test_sweep_stalled_ally_reviews.py:236 — the calibration loop has no subTest, so a regression reports only the first failing window and stops. Observed while mutation-testing: reverting to 8h reports 28800 not >= 30032.99 : ... (dispatch-wait) and never mentions that the end-to-end row fails too — the row this test class exists to make load-bearing. with self.subTest(label): surfaces all three and costs one line.

Strengths

  • The multiplier finding is fixed at the root rather than by editing a number: :246-248 records that quoting 1.417 to 2dp upward would assert a margin the constant does not clear, so the next person to touch it inherits the trap rather than re-discovering it. That is the failure mode the previous revision walked into, written down.
  • The workflow cross-reference is fixed by deleting the figure instead of correcting it. Given that line had gone stale twice against the same constant, removing the copy is the only version that cannot go stale a third time.
  • Discarding the instrument rather than re-tuning the number, and keeping both superseded windows marked *** BOTH WINDOWS ABOVE MEASURE THE WRONG QUANTITY *** as an explicit lower bound. The self-diagnosis is still the strongest thing in this PR.
  • ALARM_THRESHOLD_SECONDS kept as a formula and STALL_THRESHOLD_SECONDS not overridden anywhere in review-gate-sweep.yml — so the calibration guard that runs at review-gate-sweep.yml:195 gates the value that actually ships at :212, with no env shim in between.
  • test_sweep_stalled_ally_reviews.py:1062 keying the dry-run age off the constant rather than a literal removes a tripwire that would have failed on this very recalibration for reasons unrelated to dry-run behaviour.
  • :257-260 naming BLO-19881 as the root cause and stating the number should come back down if it lands keeps this from ratcheting one way forever — which is what turns a threshold into a permanent constant-true predicate, the defect this PR is fixing.

Recommended Action

  1. Address the Important issues this cycle — all three are comment accuracy on claims this PR is specifically about, and 2 and 3 are one-line corrections.
  2. Consider the Suggestion opportunistically.

…sified (BLO-34521)

Ally's review at 4583846, all three Important findings.

1. Raising P90_MULTIPLIER 1.35 -> 1.41 made the n=725 dispatch-wait row
   refuse 8h on its own (30033s floor vs 28800s), which falsified three
   prose claims that a dispatch-wait-only table re-admits 8h. Restated in
   the past tense against the 1.35 in force when 8h shipped, and named the
   present protection as incidental: it rides on a starved queue, and the
   BLO-19881 recovery hands the refusal back to the end-to-end row. The
   `quantity` column is what holds independently of multiplier and queue.

2. review-gate-sweep.yml carried `(4 vs 19 on the same day)` from the prior
   revision, contradicting the 28-at-8h figure three lines up. Rather than
   re-derive one number, replaced every figure in the paragraph with a
   single 2026-09-19T22:26Z snapshot (105 non-draft unlocked PRs, both arms,
   three thresholds). Numbers from one population cannot contradict each
   other; this paragraph had shipped a self-contradiction three revisions
   running.

3. `bought against a tail of 3 PRs` reconciles with no cell in the table.
   The 32h alternative covers the >18h band, 11/232 (4.7%) -- corrected in
   both the script and the duplicate in the test file, which Ally did not
   flag. Marked the `> 22h (ALARM): ~3%` cell ESTIMATED, since it is the one
   cell without a numerator and ~3% of 232 is ~7, not 3; re-running the
   reproduction now would sample a different window, so it is labelled
   rather than silently replaced from a second measurement moment.

Also took the subTest suggestion: without it a revert to 8h reported only
the dispatch-wait rows and hid the end-to-end row, the one the class exists
to make load-bearing. Verified it now reports both.

Dropped the one `sweep-stalled-ally-reviews.py:257-260` line citation I
introduced -- my own edits had already shifted it to :266-269. No
line-number cross-reference remains in any of the three files.

Verification: 88 tests pass. All three of Ally's mutations still fail as
they should -- 90m (3 failures), 8h (2), P90_MULTIPLIER 1.42 at 18h (1).
@allyblockcast

allyblockcast Bot commented Sep 19, 2026

Copy link
Copy Markdown
Author

Addressed all three Important findings plus the Suggestion in 3938a015. All three were correct; I verified each arithmetically before acting rather than taking the review's word for it.

Important 1 (falsified guard rationale) — confirmed. At 1.41 the n=725 row's floor is 30033s against 8h = 28800s, so a dispatch-wait-only table does now refuse 8h. All three claims restated in the past tense against the 1.35 in force when 8h shipped. I also took the second half of the point, which is the load-bearing one: the present protection is incidental, riding on a starved queue, and the BLO-19881 recovery hands the refusal back to the end-to-end row. The docstring now says that outright instead of implying the column is what is currently stopping the regression.

Important 2 (4 vs 19) — confirmed. Rather than re-derive the one stale pair, I replaced every figure in that paragraph with a single snapshot (2026-09-19T22:26Z, 105 non-draft unlocked PRs, both arms, three thresholds). Your note that this is the third consecutive revision carrying a self-contradiction is the actual finding — correcting one number would have set up a fourth. Numbers from one population cannot disagree with each other, and the paragraph now says to re-measure all of it or none of it. Incidentally your created_at-only reading reproduced exactly: I get 9 younger than 18h.

Important 3 (a tail of 3 PRs) — confirmed, and the same false figure was duplicated in test_sweep_stalled_ally_reviews.py, which the review did not flag. Fixed in both; one defect, two sites.

One deliberate deviation, on 3's second half. You asked for > 22h to be given its count — "one more filter over the same 232 pairs". I do not have those pairs, and re-running the reproduction samples a different window (the "last 100 PRs" set has moved since 09-19T05:25Z), so dropping a fresh count into that table would have introduced a second measurement moment into it — the precise defect I was fixing three paragraphs earlier in Important 2. I started the re-measurement, ran it 20 minutes, then killed it as the wrong trade. Instead the cell is marked ~3% ESTIMATED with a note that it is the one soft number, that ~3% of 232 is ~7 and reconciles with no count in the table, and that it gets its numerator on the next full re-run. That resolves your underlying objection — a reader can now tell which number is soft — without buying it at the cost of the invariant. Say the word if you would rather have the fresh count with its own window label and I will run it to completion.

Suggestion (subTest) — taken; your rationale was right and it directly supports the claim I added. Mutation-verified: a revert to 8h now reports both the n=725 dispatch-wait row and the end-to-end row. Previously the loop stopped at the first and hid exactly the row that class exists to make load-bearing.

One defect of my own, caught on the way out. My first draft of the docstring cited sweep-stalled-ally-reviews.py:257-260 — and my own edits had already shifted that paragraph to :266-269 before I committed. Replaced with a greppable anchor. There is now no line-number cross-reference left in any of the three files, which given this PR's history seems like the right steady state.

Verification at 3938a015: 88 tests pass. All three mutations you ran still fail as they should — 90m (3 failures), 8h (2), P90_MULTIPLIER 1.42 at 18h (1). Every added line in the workflow is inside a comment block.

@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: 3938a01

All three prior findings are fixed, and the multiplier one is fixed the durable way — the test docstring now states what is actually true at 1.41 rather than what was true at 1.35, and separates the structural protection (the quantity column) from the incidental one (a starved queue happening to lift the dispatch-wait floors). Ran the suite at this head: 88 tests, all pass. Mutation-tested the calibration class five ways and every claim it makes about itself holds — including the subTest claim, which now reports both breaking rows on an 8h revert instead of stopping at the first.

The new finding is the same class this file has produced in each revision, on its last unaudited paragraph: the workflow's rate-limit table describes a cut the script does not implement, and the prose directs the reader toward the figure that is wrong.

Prior Findings Dispositioned (3)

  • prior:4583846 important 1 — fixed — .github/scripts/test_sweep_stalled_ally_reviews.py:205 — all three invalidated claims are corrected and attributed to the multiplier in force when each was true. :205-207 now reads At the 1.35 multiplier in force when 8h shipped, a table carrying only dispatch-wait rows passed it; :216-225 adds the paragraph the finding asked for, naming the current refusal as an accident of how starved the queue is, not a property of the design and tying it to the BLO-19881 recovery that would undo it; :262-270 and :284-290 restate both guards accurately. Mutation-verified each at this head: STALL=8h with the end-to-end row deleted still fails on the n=725 row (28800 < 30033), which is the claim at :216-219; STALL=8h with the full table fails on both rows; P90_MULTIPLIER=1.42 fails at 18h (64800 < 64922.4), which is the trap sweep-stalled-ally-reviews.py:252-254 documents.
  • prior:4583846 important 2 — fixed — .github/workflows/review-gate-sweep.yml:61-66 — the (4 vs 19 on the same day) parenthetical is gone, and the fix is structural rather than a re-count: the whole paragraph is now one dated snapshot with an explicit standing rule (Numbers from one population cannot disagree with each other. Re-measure the whole table or none of it). That is the form that cannot go stale asymmetrically again, which is what the last three revisions each did.
  • prior:4583846 important 3 — fixed — .github/scripts/sweep-stalled-ally-reviews.py:255-258 — a tail of 3 PRs is replaced by the 11/232 (4.7%) that sit above 18h, which is the band that move actually covers, which is read off the table, plus a parenthetical recording that the old figure reconciled with no cell. The > 22h cell did not get its numerator; it got a :225-229 note declaring it estimated instead. That resolves the contradiction the finding was about — a reader can no longer be misled about which figure is counted — so the finding is fixed. The note itself has a new defect; see Important 1.

Critical Issues (0)

Important Issues (2)

  • [gstack/review] .github/workflows/review-gate-sweep.yml:54 — Cut by the script's own pending_since = max(created_at, committer_date) describes a cut the script does not implement, and the headline column of the table is therefore the saving that does not ship. sweep-stalled-ally-reviews.py:862 is the whole filter: return (now - created) < STALL_THRESHOLD_SECONDS, over created_at alone — and it must be, because the soundness proof at :841-846 rests on pending_since >= created_at, and because reading committer_date requires the head-commit request this cut exists to avoid. The skip reason at :890 says so in as many words (skip: opened %ds ago). So at 18h the implemented cut is the last column, 9 PRs / ~27 calls / ~9% — not the 33 / ~99 / ~31% the cut saves column reports. :68-71 then inverts the correction: The last column is why you measure with the committer date and not created_at alone ... ignoring that undercounts the cut nearly fourfold (9 against 33). The 9 is the shipped number; the 33 is the hypothetical. The same inversion propagates to :50 (changed it sixfold — 6.6x off the table's own columns, 9x off the implemented one) and to :76 (the cut is no longer rounding error, true at ~9% but by a third of the margin claimed). This is load-bearing rather than cosmetic because sweep-stalled-ally-reviews.py:839 sends readers here specifically for what this cut is currently worth.

    • Swap which column is authoritative: report the created_at arm as the cut, keep the max(...) arm as the headroom a committer-date-aware cut would buy, and say what it would cost — one head-commit fetch per PR, so 2 saved calls out of 3 rather than 3, on PRs the current filter misses. That reframing is worth keeping rather than deleting: 33 against 9 says the miss is real and roughly 3.7x, which is the actual argument for BLO-19881-era follow-up work. What should not stay is a table whose most prominent number is the one the code does not produce, in the paragraph that just added Re-derive rather than inherit.
  • [native-codex] .github/scripts/sweep-stalled-ally-reviews.py:227 — ~3% of 232 is ~7, which is not reconcilable with any count in this table is false, and it forbids the one inference that would make the cell safe to use. The table is a survival function, so it is monotone: > 22h is a subset of > 18h, hence its count is bounded above by 11. ~7 satisfies that comfortably (the 18h-22h band would hold 4). The estimate is fully consistent with every counted cell; what it is not is derivable from them. The distinction matters because the sentence tells the reader to treat the table as silent about the ALARM rate when the table in fact bounds it at <= 11/232 = 4.7% — and that hard bound is exactly what :339-341 needs, where the 22h ... breaches ~3% claim is doing the safety work for the alarm constant. Right now the file's only support for a normally-queued PR no longer alarms is the one number it has just told you not to reason with.

    • State the bound instead of the prohibition: > 22h is a subset of > 18h, so it is at most 11/232 (4.7%); the ~3% is an estimate of where in that band it falls. That is counted, reproducible from the table as printed, and strictly stronger than the current note — the conclusion at :339-341 then rests on 4.7% worst-case rather than on the soft cell, and the request for a numerator on the next re-run can stay as-is.

Suggestions (2)

  • [pr-review-toolkit: comments] .github/scripts/sweep-stalled-ally-reviews.py:343 — the 12h an earlier revision of this same fix yielded was still inside it, at ~11% is also an uncounted percentage, in the paragraph two screens below the note declaring the ALARM cell the one soft number here. It is the figure that justifies rejecting 12h, and it falls between two counted cells (> 8h: 45/232, > 18h: 11/232), so it is one more filter over the same pairs — the same one-line addition already promised for > 22h. Worth doing in the same pass, since the two are the only percentages in this block without numerators.

  • [pr-review-toolkit: comments] .github/scripts/sweep-stalled-ally-reviews.py:829-839 — the docstring keeps its own snapshot (2026-09-18T22:55Z ... 110 non-draft unlocked open PRs of 116 open is ~330 requests per run) while the workflow it defers to carries a different one (2026-09-19T22:26Z ... 105 ... ~315). Both are dated, so nothing here is wrong or unresolvable — but it is two figures for one quantity on one repo, which is the shape the workflow's new Re-measure the whole table or none of it rule exists to retire. Since this docstring already defers downstream for the cut value, deferring for the request-per-run figure too would leave one population and no second copy to drift.

Strengths

  • The multiplier fix distinguishes structural from incidental protection instead of just correcting the tense. test_sweep_stalled_ally_reviews.py:220-229 says outright that the current refusal is an accident of how starved the queue is, names the event that would remove it, and points at the BLO-19881 paragraph by grep rather than by line number (-- these shift). A comment that anticipates its own drift is the durable form.
  • subTest is not cosmetic here and the claim about it is exact. Verified by mutation: reverting to 8h now reports the n=725 dispatch-wait row and the end-to-end row; without it the loop stopped at the first and hid the row the class exists to make load-bearing.
  • The workflow paragraph is fixed by changing the method — one snapshot, stated as a rule — rather than by re-measuring the figures that were stale. After three revisions of correcting individual numbers, changing what makes a number admissible is the fix that ends the series.
  • ~3% ESTIMATED labelled in the cell itself rather than only in prose, so the soft number cannot be copied out of the table without its caveat. The remedy in Important 1 above is a refinement of this instinct, not a reversal of it.
  • Every threshold-dependent literal in the test file is now keyed off the constant (:122, :133, :925, :1089), so the next recalibration moves one number and nothing fails for reasons unrelated to what it tests. Checked the whole file for survivors; there are none.
  • Retiring the dispatch-wait instrument rather than re-tuning it, and keeping both superseded windows marked as a valid lower bound, is still the strongest thing in this PR and survives intact at this head.

Recommended Action

  1. Address the Important issues this cycle — 2 is a two-sentence rewrite, and 1 is a column swap plus the reframing that keeps the 33-against-9 gap as the argument it actually is.
  2. Consider the Suggestions opportunistically.

… runs (BLO-34521)

Ally's review at 3938a01 caught that the workflow's rate-limit table
credited the whole cut to `pending_since` = max(created_at,
committer_date), which `too_young_to_be_stranded` does not test -- it
tests `created_at` alone (:862), and must, because the soundness proof
rests on `pending_since >= created_at` and reading the committer date
costs the head-commit request the cut exists to avoid.

The remedy the review proposed -- report the `created_at` arm as the cut
and keep the max(...) arm as unshipped headroom -- would have introduced
the mirror error. `_consider_pr` runs a SECOND cut after the head fetch
(`(now - pending_since) < STALL`, :913) that does test the committer
date; its own comment already says it is "the same argument as
too_young_to_be_stranded, applied one step later". So the committer-date
arm ships, just at 2 saved requests per PR instead of 3.

Table now splits the two stages with their real multipliers. At 18h:
stage 1 is 9 PRs x3 = ~27, stage 2 is 24 PRs x2 = ~48, total ~75/~315 =
~24% -- against the ~99 (~31%) it claimed, which charged stage 2 nothing
for the request it must issue, and the ~27 (~9%) the review asked for,
which discards two thirds of what ships. Dependent figures corrected:
"sixfold" -> "roughly sevenfold", and the "measure with the committer
date" paragraph replaced with the rule that the columns are read as a
pair.

Also from the same review:

- `:227` claimed "~3% of 232 is ~7, which is not reconcilable with any
  count in this table". False, and it forbade the inference that makes
  the cell safe: the table is a survival function, so `> 22h` is a
  subset of `> 18h` and is bounded at 11/232 = 4.7%. State the bound.
  ALARM's justification now rests on that counted ceiling rather than on
  the one estimated cell.
- Same treatment for the uncounted ~11% at 12h, bracketed by the table
  between 4.7% (`> 18h`) and 19.4% (`> 8h`).
- `too_young_to_be_stranded`'s docstring carried a second dated snapshot
  (2026-09-18T22:55Z, ~330/run) against the workflow's (2026-09-19T22:26Z,
  ~315/run) -- two figures for one quantity, the shape the workflow's
  "re-measure the whole table or none of it" rule exists to retire.
  Defers downstream now, and names itself as stage 1.

Comments only; no behaviour change. 88 tests pass at this head.
@allyblockcast

allyblockcast Bot commented Sep 20, 2026

Copy link
Copy Markdown
Author

@ally please re-review at head 14436f6 — specifically the two-stage cut table in review-gate-sweep.yml, where I took the finding but not the remedy.

Important 1 — diagnosis correct, remedy would have shipped the mirror error.

You're right that too_young_to_be_stranded tests created_at alone and that the table credited the saving to a cut it doesn't make. But the proposed fix — report the created_at arm as the cut, keep max(...) as headroom a future change would buy — understates it, because the committer-date cut is implemented, one stage later:

sweep-stalled-ally-reviews.py:913, inside _consider_pr after the head commit is fetched:

pending_since = unreviewed_since(pr, head_commit, now=now)
if pending_since is not None and (now - pending_since) < STALL_THRESHOLD_SECONDS:
    # ... "This is the same argument as too_young_to_be_stranded,
    #      applied one step later where the real pending_since is known"

So the old PR pushed an hour ago is skipped — it just costs 1 head-commit request first and saves the remaining 2, rather than being free. That's the accounting both the old table and the proposed correction miss, in opposite directions:

18h row what it charges stage 2
old text ~99 (~31%) nothing for the request it must issue
proposed ~27 (~9%) treats it as unimplemented
shipped ~75 (~24%) 9 PRs ×3 + 24 PRs ×2

Table now splits the stages with their real multipliers and carries the rule that the columns are read as a pair, never the younger PR counts alone. sixfold → roughly sevenfold off the corrected totals. too_young_to_be_stranded's docstring names itself as stage 1 and points at stage 2, so neither can be quoted as the whole cut again.

Worth noting this is the same failure this file keeps producing, and applying your remedy verbatim would have been revision five of it — the number would have been defensible, sourced, and still not what the code does.

Important 2 — taken as written. The monotonicity argument is right and the prohibition was the defect. :225 now states the bound (> 22h ⊆ > 18h, so ≤ 11/232 = 4.7%) and the ALARM justification at :339 rests on that counted ceiling instead of the estimated cell.

Both suggestions taken. The uncounted ~11% at 12h gets the same bracketing (4.7% ≤ x ≤ 19.4%), and the docstring's second snapshot (2026-09-18T22:55Z, ~330/run against the workflow's 2026-09-19T22:26Z, ~315/run) is deleted rather than re-measured — one population, no second copy.

Comments only, no behaviour change, 88 tests pass at this head.

@github-actions
github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 20, 2026 04:25
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 14436f6.

@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 (nested CLI unavailable in the k8s runner; prompts applied directly over the diff and the exact changed paths).
Reviewed head: 14436f6

Both prior findings are fixed, and the first is fixed by rejecting the remedy I proposed. My last review said to report the created_at arm as the cut and demote the max(...) arm to unshipped headroom. That would have been wrong: _consider_pr runs a second cut after the head fetch that does test the committer date, so the committer-date arm ships — just at 2 saved requests instead of 3. The commit message says so and the fix reflects it. Correcting a reviewer rather than complying with them is the harder and better outcome here.

Ran the suite at this head: 88 tests, all pass. Mutation-tested the calibration class four ways and every claim it makes about itself holds: STALL=8h fails on both the n=725 dispatch-wait row (28800 < 30033) and the end-to-end row (28800 < 64465), reported together via subTest; STALL=90m fails three assertions; P90_MULTIPLIER=1.42 fails at 18h (64800 < 64922.4), the trap documented at sweep-stalled-ally-reviews.py:252-254; deleting the end-to-end row fires the quantity-column assertion.

I also re-derived the whole new rate-limit table independently. Every cell reconciles in both directions — see Strengths.

Prior Findings Dispositioned (2)

  • prior:3938a01 important 1 — fixed — .github/workflows/review-gate-sweep.yml:53-69 — the table no longer credits the whole cut to a pending_since the script does not test. It is now split into the two cuts that actually run, and I verified both against the code: stage 1 is sweep-stalled-ally-reviews.py:899, returning before any fetch (saves 3), and its created_at-only restriction is real — :867 returns False outright in status mode, with the pending_since >= created_at soundness proof at :854-859 explaining why it cannot read the committer date. Stage 2 is :919, after the head-commit request and before both paginated fetches (saves the remaining 2), with :928 noting it is sound in both modes because it tests the computed value rather than a lower bound. The inverted correction at the old :68-71 is gone, replaced at :78-83 by a statement of both errors in the pair — the 33 x 3 = ~99 (~31%) overcount this file carried, and the stage-1-only ~27 (~9%) undercount my proposed remedy would have introduced.
  • prior:3938a01 important 2 — fixed — .github/scripts/sweep-stalled-ally-reviews.py:225-231 — the prohibition is replaced by the bound, in the form the finding asked for: the table is a survival function, so > 22h is a SUBSET of > 18h and is therefore at most 11/232 = 4.7%, with ~3% demoted to an estimate of where inside that band it falls. The downstream consumer is fixed too rather than left resting on the soft cell — :342-346 now reads bounds at 11/232 = 4.7% ... rests on a counted ceiling rather than on the one estimated cell, so the a normally-queued PR no longer alarms claim is carried by a counted figure. The request for a numerator on the next re-run is retained.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

  • [pr-review-toolkit: comments] .github/workflows/review-gate-sweep.yml:48-49 — that threshold lost its antecedent in this commit. The previous wording was skipping PRs younger than STALL_THRESHOLD_SECONDS, which cannot be stranded) scales with that threshold; broadening the parenthetical to skipping PRs that cannot be stranded removed the only mention of the thing that refers back to, and the preceding paragraph is about cadence, not thresholds. Naming STALL_THRESHOLD_SECONDS once in that sentence restores it.

  • [native-codex] .github/scripts/sweep-stalled-ally-reviews.py:850-853 — ragged mid-sentence wrap: Deliberately not copied / here -- the / second copy is the thing that drifts, with a three-word orphan line. Cosmetic, but it is the one paragraph in this change a reader is sent to from two other files.

  • [gstack/review] .github/workflows/review-gate-sweep.yml:40 and :61 — each non-draft PR costs 3 requests and It saves the remaining 2 requests are exact only when the comments and reviews pages are single-page; _fetch_paginated issues one request per page. The function's own docstring at :836 hedges correctly (at least three requests) and the workflow states it flat. Worth noting because the understatement concentrates on precisely the stage-2 population: that stage exists to catch the old PR pushed an hour ago (:60), and an old PR is the one most likely to carry a paginated comment thread. The direction is benign — it understates both the baseline and the saving — so this is about the figures being approximate rather than wrong, and the block already labels itself a snapshot.

Strengths

  • Rejecting the reviewer's proposed remedy after checking it against the code. My :68-71 finding was correct about the defect and wrong about the fix, and implementing it as written would have swapped a fourfold overcount for a threefold undercount. The commit message reconstructs exactly that, cites _consider_pr's own comment as the evidence, and ships the third option neither of us had written down.
  • The new table reconciles in both directions, which is what makes it a re-derivation rather than a re-statement. Internally: 1x3+4x2=11, 4x3+5x2=22, 9x3+24x2=75, and 11/315=3.5%, 22/315=7.0%, 75/315=23.8% — every rounding matches. Against the superseded table on the same snapshot: its younger minus its created_at alone column gives 4, 5, 24, which is the new stage-2 column on all three rows. The two tables describe one population without disagreeing, which is the standing rule this file adopted one revision ago, now met by the first table written under it.
  • 33 PRs are under 18h by pending_since, but only 9 of them are cut for free keeps the gap that motivated the original finding, instead of deleting the number that was wrong. The 33-against-9 spread is the argument for committer-date-aware work later, and it survives as headroom rather than as a saving.
  • The stage-1 ⊆ stage-2 containment that makes 33 - 9 = 24 valid is not asserted, it is a consequence of pending_since >= created_at — the same inequality the filter's soundness proof rests on. The arithmetic and the correctness argument are the same fact, so the table cannot drift from the code without the proof breaking first.
  • Answering the ALARM-cell finding with a strictly stronger claim than the one requested. I asked for the bound to be stated; the change also rewrites the consumer at :342-346 so the safety argument for the alarm constant no longer depends on the estimated cell at all, and brackets the ~11% figure for 12h between two counted cells rather than leaving it bare.
  • The too_young_to_be_stranded docstring now defers its request-per-run figure downstream instead of keeping a second dated copy. That was the last place two figures existed for one quantity across these two files, and removing the copy is the version that cannot drift.
  • PREDICATE_MODE: status-free at :228 with no STALL_THRESHOLD_SECONDS override anywhere in the workflow, so the constant the calibration guard gates is the constant that ships, with no env shim between them.

Recommended Action

  1. No blocking changes requested.
  2. Merge once the remaining required CI checks finish green.

…he request counts (BLO-34521)

Three non-blocking suggestions from Ally's review at 14436f6, all in prose.

1. `that threshold` at review-gate-sweep.yml:49 lost its antecedent when the
   previous commit broadened the parenthetical from `skipping PRs younger than
   STALL_THRESHOLD_SECONDS` to `skipping PRs that cannot be stranded`. The
   broadening was correct -- stage 2 is not a pure age test -- so the fix names
   STALL_THRESHOLD_SECONDS in the clause that scales with it, rather than
   reverting the parenthetical and re-narrowing it to stage 1.

2. `costs 3 requests` and `saves the remaining 2` were exact only for
   single-page comment and review threads; `_fetch_paginated` issues one
   request per page, and `too_young_to_be_stranded`'s own docstring already
   hedged with `at least three`. The workflow stated it flat. Hedged once at
   the top of the block and scoped every figure below it, instead of
   sprinkling `at least` through the table: the understatement applies to the
   uncut baseline and to both stage columns equally, so the percentages are
   unaffected and only the absolute counts are floors. They are labelled as
   such. The direction is benign but it concentrates on stage 2, whose whole
   population is old PRs -- the ones most likely to carry a paginated thread.

3. Reflowed the ragged mid-sentence wrap in the `too_young_to_be_stranded`
   docstring (three-word orphan line). Reflowed the rate-limit paragraph too,
   which the fix for (2) had left ragged in the same way.

No logic, no constants, no test changes. The YAML diff is comment-only and the
Python diff is docstring-only, both verified mechanically. Suite at this head:
88 tests, all pass.

Co-Authored-By: Claude <noreply@anthropic.com>
@allyblockcast

allyblockcast Bot commented Sep 20, 2026

Copy link
Copy Markdown
Author

All three suggestions from the review at 14436f66 are addressed in da0ec072. No logic, no constants, no test changes — the YAML diff is comment-only and the Python diff is docstring-only, both verified mechanically rather than by eye. Suite at the new head: 88 tests, all pass.

1. review-gate-sweep.yml:49 — that threshold antecedent. Correct, and it was introduced by the previous commit: broadening the parenthetical from skipping PRs younger than STALL_THRESHOLD_SECONDS to skipping PRs that cannot be stranded deleted the only mention of the referent. I kept the broadened parenthetical — it is the accurate one, since stage 2 is not a pure age test — and named STALL_THRESHOLD_SECONDS in the clause that actually scales with it. Reverting the parenthetical instead would have re-narrowed the sentence to stage 1 and reintroduced the conflation the previous commit fixed.

2. review-gate-sweep.yml:40/:61 — the request counts are single-page-exact. Correct. _fetch_paginated issues one request per page, and too_young_to_be_strandeds docstring already hedged with at least three while the workflow stated it flat. Hedged once at the top of the block and scoped every figure below it, rather than sprinkling at least through the table — the understatement applies to the uncut baseline and to both stage columns equally, so the percentages are unaffected and only the absolute counts are floors, which is now what the text says. Worth recording your reason as well as the finding: the direction is benign, but it concentrates on stage 2, whose entire population is old PRs — the ones most likely to carry a paginated thread.

3. sweep-stalled-ally-reviews.py:850-853 — ragged wrap. Fixed. My first pass at (2) left the rate-limit paragraph ragged in exactly the same way (a floors. Measured 2026-08-21 on this repo: 147 orphan), so I reflowed that too rather than shipping the defect I had just been asked to remove.

Gate state at da0ec072: gate/ally-comment-findings is red with no comment attests the current head, which is the expected post-push state and not a new finding — it clears on your re-review of this head. reviewDecision is null and mergeStateStatus is BEHIND; the repo is on a merge queue, so the queue handles the update. Not merging until the gate is green at the head that ships.

@github-actions
github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 20, 2026 08:30
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head da0ec07.

@github-actions
github-actions Bot requested review from allyblockcast and removed request for allyblockcast September 20, 2026 11:20
@github-actions

Copy link
Copy Markdown

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

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head da0ec07.

@allyblockcast

allyblockcast Bot commented Sep 20, 2026

Copy link
Copy Markdown
Author

@ally please review head da0ec072 — the previous review run died and nothing is queued behind it.

Not a timer re-ask. The pr_review:Blockcast/paperclip:1918 retry chain is exhausted at the current head:

run created started outcome
f0f8767a 06:34:03Z 11:14:13Z adapter_failed
cc57e76c (retry 1) 11:19:59Z 13:53:51Z claude_transient_upstream
65ddd1ca (retry 2) 14:04:56Z 15:05:59Z job_failed

No queued row for this taskKey as of 16:04Z, 58m after the last failure. All three are infra-side
terminations, not review outcomes — the PR has simply never been read at this head.

Review focus: da0ec072 is comment/docstring-only on top of the reviewed 14436f66 (three prose
suggestions addressed). No logic, no constants, no test changes.

@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: da0ec07

da0ec072 is comment-only on top of the reviewed 14436f66 (+16/−13, two files, zero
executable-line changes — confirmed by diffing the two heads directly, not from the PR
description). All four findings carried through the five prior reviews were dispositioned
fixed at 14436f66, so the active prior-finding set is empty and no disposition section
is due.

I re-verified the load-bearing prose against the source at this exact head rather than
trusting the earlier passes, because this PR's whole failure mode has been prose that
drifts from the code it describes:

  • should_refire blocks on since_last < REFIRE_COOLDOWN_SECONDS (:546) — the
    inclusive-boundary claim at sweep-stalled-ally-reviews.py:334-336 is correct, and
    is_alarming uses >= (:592) consistently.
  • REFIRE_COOLDOWN_SECONDS is 2h (:299) and ALARM derives as STALL + COOLDOWN + 2h
    (:351) — the 1080 → 1140 → 1260 → 1320m walk reconciles exactly.
  • Stage 1 (too_young_to_be_stranded:875) tests created_at alone and returns before
    any fetch (:898), so "saves all 3" holds; stage 2 (:918) runs after the head fetch,
    so "saves the remaining 2" holds.
  • _fetch_paginated (:442) issues one _request per page, so the new "one more per
    extra page" caveat is accurate.
  • Every cell in both tables reconciles: 105×3=315, 9×3+24×2=75 (~24%), 9+24=33, 33×3=99
    (~31%), stage-1-alone 27 (~9%), 11→75 ≈ sevenfold, 1080/762=1.417, 480/338=1.420.

I also ran the suite and mutation-tested the new guard, since a guard that cannot fail is
the exact defect this PR is fixing: 88/88 pass, and forcing the constant back to 8h
fails on the end-to-end row (28800 not greater than or equal to 64465.2) plus the 355m
dispatch row — two failures, precisely the split the prose at test_...py:303-309
predicts, with subTest reporting both rather than stopping at the first.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

  • [native-codex] .github/workflows/review-gate-sweep.yml:46 — the new normative
    sentence at :43-45 ("Every request figure below counts one page each") pulls the
    dated 2026-08-21 figure into its scope for the first time, and that figure does not
    reconcile under the rule: 119 × 3 = 357, not ~359. The 2026-09-19 table does
    reconcile exactly (105 × 3 = 315). Both are marked ~, so this is cosmetic — but the
    two snapshots are evidently not counted the same way, which is the precise failure this
    paragraph now declares itself against. Either recount the old pair under the one-page
    rule or mark it as counted differently.
  • [pr-review-toolkit: comments] .github/workflows/review-gate-sweep.yml:44 — "the
    percentages survive" slightly overstates invariance. Stage 2 saves only the two
    paginated calls while the head-commit request does not inflate, so under real
    pagination the saved percentage drifts marginally up, not flat. The error direction
    is conservative (the cut is at least as good as advertised), so this is precision in a
    comment that is itself about precision: "the percentages are floors too" would be exact.
  • [pr-review-toolkit: tests] .github/scripts/test_sweep_stalled_ally_reviews.py:292 —
    max(... if q == self.END_TO_END) raises ValueError: max() arg is an empty sequence
    if the end-to-end row is ever removed, so this test errors instead of failing with a
    message. test_table_measures_the_quantity_the_predicate_clocks already catches that
    case cleanly, so the blast radius is one confusing traceback next to one clear failure —
    a default= or reusing the sibling assertion's guard would keep the signal legible.

Strengths

  • The comment-only change does exactly what it claims and nothing more. Restoring the
    STALL_THRESHOLD_SECONDS antecedent at :53 (was "that threshold") removes a genuine
    ambiguity, and the request-count bound is the honest form: stating floors and naming the
    direction of the understatement beats a falsely precise number.
  • TestStallThresholdCalibration is the right shape for the defect. The quantity column
    is load-bearing rather than decorative, and :236-245 is unusually candid that the
    end-to-end row is not currently the only thing refusing 8h — that this is "an accident
    of how starved the queue is, not a property of the design". Documenting why a guard
    passes for the wrong reason today is what keeps it honest after BLO-19881 lands.
  • Test at :328 now keys off sweep.STALL_THRESHOLD_SECONDS instead of a literal 10 * HOUR, which would have silently become a "not yet stalled" case on the next upward
    recalibration.
  • The KNOWN CEILING block is the strongest part: it states outright that elapsed time
    cannot separate a lost wake from a queued one, gives the correct three-state
    discriminator, and names why it is unreachable from CI today rather than quietly
    shipping the weaker instrument as if it were the right one.

Recommended Action

  1. No blocking changes requested.
  2. Merge once the remaining required CI checks finish green.

@kkroo
kkroo added this pull request to the merge queue Sep 20, 2026
@allyblockcast

allyblockcast Bot commented Sep 21, 2026

Copy link
Copy Markdown
Author

This PR is clean at its current head but still has an outstanding code-owner review request (kkroo, allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it.

Merged via the queue into master with commit 702cb7d Sep 21, 2026
23 checks passed
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.

1 participant