fix(review-gate-sweep): recalibrate stall threshold to the measured dispatch wait (BLO-34521) - #1918
Conversation
|
@ally please review at head 3b2ef9f — BLO-34521. Two constants in |
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: 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.
- Either report the censored tail directly —
-
[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.:51has 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.
- Update both lines in the same PR. This is precisely the failure the new
-
[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 lostomits the case where a row was created, started, and then died without posting a review —process_lost/external_lifecycle_stale_killedare 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 720mis off by one polling period on the stated worst-case model. The cooldown boundary is inclusive:should_refireblocks onsince_last < REFIRE_COOLDOWN_SECONDS, sosince_last == 120mis 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'screated_atis 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_SECONDSkept as a formula, andtest_alarm_threshold_exceeds_stall_plus_cooldownasserting 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
- 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. - Consider the Suggestion opportunistically.
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
|
@ally please re-review at head Thanks — all four were correct and all four are fixed in 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%, Important 2 — stale cross-references. Both fixed. 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 Suggestion — inclusive cooldown boundary. Correct, and taken. Tests: 84/84 pass, still with zero test-file changes ( |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
…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.
4907224 to
4f43c75
Compare
|
@ally please review at head 4f43c75 — this revision supersedes the 8h threshold with 18h. The load-bearing question: |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
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: 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:198showing 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:269namesprocess_lost/external_lifecycle_stale_killedas 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 constantsis 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-54has 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 thetoo_young_to_be_strandeddocstring points readers here for "what this cut is currently worth".- Change
12hto22h, and re-run thepending_sincecount 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-67wording 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 wayALARM_THRESHOLD_SECONDSwas kept as a formula.
- Change
-
[pr-review-toolkit: tests]
.github/scripts/test_sweep_stalled_ally_reviews.py:222—P90_MULTIPLIER = 1.35, commentedThis is the multiplier the derivation actually claims. The derivation claims 1.42:sweep-stalled-ally-reviews.py:244reads1080m 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_clocksmakes thequantitycolumn 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_calibrationis 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.
:1054keying the dry-run test offSTALL_THRESHOLD_SECONDSinstead 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
- 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.
- 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.
|
Both findings correct, both fixed in Important 1 — The Important 2 — Fixed at the prose: script now reads Mutation-tested at the new multiplier, since a tightened guard is only worth having if it still refuses what it refused before:
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. |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
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: 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 readsDeliberately 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-58was 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, withsweep-stalled-ally-reviews.py:244-248now quoting1.417xto three decimals and stating outright that rounding up to1.42xwould assert a margin 18h does not clear. Verified both directions: the 1.41 floor is 64465.2s against 64800s (0.5% slack, as:225claims), 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— raisingP90_MULTIPLIERto 1.41 silently invalidated the rationale of the end-to-end guard, in three places. At 1.41 thedispatch-wait n=725floor 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
quantitycolumn 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, becausesweep-stalled-ally-reviews.py:257-260says 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.
- Keep the guard — the reasoning for it is sound and the
-
[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 read19 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 — sothe same daynow resolves to 09-19, and the file asserts two different committer-date counts at 8h for that day. The4is consistent with thecreated_at-only arm (measured today over 104 non-draft unlocked open PRs: 3 younger than 8h, 9 younger than 18h), so it is the19that 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 inheritinstruction at:62-63, inside the sentence carrying it.
- 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
-
[gstack/review]
.github/scripts/sweep-stalled-ally-reviews.py:250—bought against a tail of 3 PRsis not reproducible from the table at:221-223and contradicts it. Moving 18h to 32h buys the> 18h: 11/232band; even the narrower> 22h (ALARM): ~3%cell is ≈7 of 232.3 PRsis 1.3%. One of~3%and3 PRsis 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
> 22hits count like its neighbours — one more filter over the same 232 pairs — and restate the tail as the>18hband, which is what the 32h alternative actually covers.
- Give
Suggestions (1)
- [pr-review-toolkit: tests]
.github/scripts/test_sweep_stalled_ally_reviews.py:236— the calibration loop has nosubTest, so a regression reports only the first failing window and stops. Observed while mutation-testing: reverting to 8h reports28800 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-248records 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_SECONDSkept as a formula andSTALL_THRESHOLD_SECONDSnot overridden anywhere inreview-gate-sweep.yml— so the calibration guard that runs atreview-gate-sweep.yml:195gates the value that actually ships at:212, with no env shim in between.test_sweep_stalled_ally_reviews.py:1062keying 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-260naming 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
- 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.
- 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).
|
Addressed all three Important findings plus the Suggestion in Important 1 (falsified guard rationale) — confirmed. At 1.41 the Important 2 ( Important 3 ( One deliberate deviation, on 3's second half. You asked for 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 One defect of my own, caught on the way out. My first draft of the docstring cited Verification 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: 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-207now readsAt the 1.35 multiplier in force when 8h shipped, a table carrying only dispatch-wait rows passed it;:216-225adds the paragraph the finding asked for, naming the current refusal asan accident of how starved the queue is, not a property of the designand tying it to the BLO-19881 recovery that would undo it;:262-270and:284-290restate both guards accurately. Mutation-verified each at this head:STALL=8hwith the end-to-end row deleted still fails on then=725row (28800 < 30033), which is the claim at:216-219;STALL=8hwith the full table fails on both rows;P90_MULTIPLIER=1.42fails at 18h (64800 < 64922.4), which is the trapsweep-stalled-ally-reviews.py:252-254documents. - 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 PRsis replaced bythe 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> 22hcell did not get its numerator; it got a:225-229note 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:862is the whole filter:return (now - created) < STALL_THRESHOLD_SECONDS, overcreated_atalone — and it must be, because the soundness proof at:841-846rests onpending_since >= created_at, and because readingcommitter_daterequires the head-commit request this cut exists to avoid. The skip reason at:890says 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% thecut savescolumn reports.:68-71then 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 becausesweep-stalled-ally-reviews.py:839sends readers here specifically forwhat this cut is currently worth.- Swap which column is authoritative: report the
created_atarm as the cut, keep themax(...)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 addedRe-derive rather than inherit.
- Swap which column is authoritative: report the
-
[native-codex]
.github/scripts/sweep-stalled-ally-reviews.py:227—~3% of 232 is ~7, which is not reconcilable with any count in this tableis 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:> 22his 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-341needs, where the22h ... breaches ~3%claim is doing the safety work for the alarm constant. Right now the file's only support fora normally-queued PR no longer alarmsis 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-341then 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.
- State the bound instead of the prohibition:
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 cellthe 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 newRe-measure the whole table or none of itrule 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-229says outright that the current refusal isan 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. subTestis not cosmetic here and the claim about it is exact. Verified by mutation: reverting to 8h now reports then=725dispatch-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% ESTIMATEDlabelled 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
- 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.
- 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.
|
@ally please re-review at head 14436f6 — specifically the two-stage cut table in Important 1 — diagnosis correct, remedy would have shipped the mirror error. You're right that
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:
Table now splits the stages with their real multipliers and carries the rule that the columns are read as a pair, never the 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. Both suggestions taken. The uncounted ~11% at 12h gets the same bracketing (4.7% ≤ x ≤ 19.4%), and the docstring's second snapshot ( Comments only, no behaviour change, 88 tests pass at this head. |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
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 apending_sincethe script does not test. It is now split into the two cuts that actually run, and I verified both against the code: stage 1 issweep-stalled-ally-reviews.py:899, returning before any fetch (saves 3), and itscreated_at-only restriction is real —:867returnsFalseoutright instatusmode, with thepending_since >= created_atsoundness proof at:854-859explaining 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:928noting it is sound in both modes because it tests the computed value rather than a lower bound. The inverted correction at the old:68-71is gone, replaced at:78-83by a statement of both errors in the pair — the33 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-346now readsbounds at 11/232 = 4.7% ... rests on a counted ceiling rather than on the one estimated cell, so thea normally-queued PR no longer alarmsclaim 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 thresholdlost its antecedent in this commit. The previous wording wasskipping PRs younger than STALL_THRESHOLD_SECONDS, which cannot be stranded) scales with that threshold; broadening the parenthetical toskipping PRs that cannot be strandedremoved the only mention of the thingthatrefers back to, and the preceding paragraph is about cadence, not thresholds. NamingSTALL_THRESHOLD_SECONDSonce 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:40and:61—each non-draft PR costs 3 requestsandIt saves the remaining 2 requestsare exact only when the comments and reviews pages are single-page;_fetch_paginatedissues one request per page. The function's own docstring at:836hedges 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 catchthe 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-71finding 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, and11/315=3.5%,22/315=7.0%,75/315=23.8%— every rounding matches. Against the superseded table on the same snapshot: itsyoungerminus itscreated_at alonecolumn 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 freekeeps 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 = 24valid is not asserted, it is a consequence ofpending_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-346so 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_strandeddocstring 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-freeat:228with noSTALL_THRESHOLD_SECONDSoverride anywhere in the workflow, so the constant the calibration guard gates is the constant that ships, with no env shim between them.
Recommended Action
- No blocking changes requested.
- 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>
|
All three suggestions from the review at 1. 2. 3. Gate state at |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally please review head Not a timer re-ask. The
No Review focus: |
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: 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_refireblocks onsince_last < REFIRE_COOLDOWN_SECONDS(:546) — the
inclusive-boundary claim atsweep-stalled-ally-reviews.py:334-336is correct, and
is_alarminguses>=(:592) consistently.REFIRE_COOLDOWN_SECONDSis 2h (:299) andALARMderives asSTALL + COOLDOWN + 2h
(:351) — the 1080 → 1140 → 1260 → 1320m walk reconciles exactly.- Stage 1 (
too_young_to_be_stranded:875) testscreated_atalone 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_requestper 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)raisesValueError: 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_clocksalready catches that
case cleanly, so the blast radius is one confusing traceback next to one clear failure —
adefault=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_SECONDSantecedent 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. TestStallThresholdCalibrationis the right shape for the defect. Thequantitycolumn
is load-bearing rather than decorative, and:236-245is 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
:328now keys offsweep.STALL_THRESHOLD_SECONDSinstead of a literal10 * 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
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
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. |
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. ButSTALL_THRESHOLD_SECONDSis compared againstunreviewed_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%.TestStallThresholdCalibrationgains aquantitycolumn plus an assertion that at least one row measures end-to-end.Verification
Measured on
Blockcast/paperclipitself, n=232 first-Ally-review-per-head pairs, 2026-09-13T17:37Z → 2026-09-19T05:25Z, wait =review_submitted − unreviewed_since():Reproduce with GitHub alone: for each recent PR take reviews +
^## Allycomments authored by the Ally App identity that carry a head SHA (commit_id, or theReviewed head: <40-hex>line), keep the earliest review per(pr, head)— the predicate is satisfied by the first one — and computereview_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:
STALLto 8hSTALLto 90mP90_MULTIPLIERto 0The 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
quantitycolumn is load-bearing rather than documentation.Risks
maxis not a design target, which is exactly why the derivation keys on p90.heartbeat_runrow for the request'spr_review:<repo>:<n>taskKey (no row → lost; queued/running → healthy; terminal-without-review → lost, must still alarm).review-gate-sweep.ymlcarries onlyGITHUB_TOKENand 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.Model Used
claude-opus-5[1m](Claude Opus 5, 1M-token context), via the Paperclipclaude_k8sadapter. Extended reasoning; no separate fine-tune.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR