fix(review-gate-sweep): re-check ally_has_reviewed_head before the write (BLO-32044) - #1667
Conversation
…ite (BLO-32044) The pre-write guard added in BLO-31908 re-read the PR's comments and re-applied only the cooldown. The scan has a second precondition -- _consider_pr skips a PR when ally_has_reviewed_head() is true -- and that read was left at its scan-time value, so the guard could withhold a write for a concurrent re-ask while happily re-asking for a review that had just landed. That interleaving needs no concurrent sweep at all, which arguably makes it the likelier one: a single run whose scan and write straddle Ally answering is enough. Measured request->response latency is 5m-74m against a ~2min run, so the window is small but not exotic. Both surfaces are re-checked, because either alone yields false negatives (verified live 2026-08-04: #952 carried 4 comment-shaped reviews with an empty pulls/952/reviews; #937 carried 4 formal review objects and no comment-shaped one). Ordered by cost -- the cooldown and the comment surface are free, since the guard already holds the comments, and the one paid read is reached only when both free checks pass. Every branch that declines is a branch that will not write, so may_refire=False and dry_run still pay for nothing. The check is head-exact, not "has Ally reviewed at all". A report against a superseded head must NOT block: reconciling that case is the entire purpose of the sweep, and a coarser test would silently disable the reconciler while still passing the two positive tests. The stale-head negative control is the load-bearing one and was confirmed against two mutants -- with the check removed, 5 tests fail; with head-exactness dropped for a naive "any Ally report -> skip", the positive tests still pass and only that control fails. Skips carry a distinct REVIEWED_SKIP_REASON_PREFIX so an operator can tell "Ally answered mid-run" from "re-asked too recently" off the log line, and a guard skip still returns refire=False, so it consumes no MAX_REFIRES_PER_RUN slot. BLO-31908's residual is left intact: this narrows a different window with the same mechanism and closes neither race.
…a negative age (BLO-32044) Two non-blocking items Ally raised reviewing #1661, deliberately deferred there so a clean review at head bc73d71 was not staled for changes with no correctness gain. Both are observability, and both sit in the code this issue already touches. 1. The guard's skips were the least visible of the run's outcomes, which inverts the priority. `failed`, `deferred` and `alarming` each get a GITHUB_STEP_SUMMARY section; the guard's reached an operator only through the per-PR stdout line. The contended count is the only direct evidence that carrying no concurrency group (BLO-31818) has a live cost -- i.e. that sweeps genuinely overlap and the residual on refire_still_permitted is real rather than theoretical -- so it was the one signal you had to grep logs to find. It now gets a summary block, counting the two kinds separately: contended means a concurrent sweep beat us to it, answered means Ally replied mid-run and the PR is simply no longer stranded. The block is conditional, so a clean run does not carry a permanent "0 withheld" line that nobody reads. 2. The cooldown reason rendered a negative age in exactly the case the guard exists for. `now` is sampled once at the top of the run, so a marker posted by a concurrent sweep mid-run is genuinely newer than this run's clock and the message read "re-asked -3s ago < cooldown 7200s". The decision was right, but it reads as an arithmetic bug on the one line operators use as evidence. Reworded to name the skew rather than clamp it with max(0, ...), which would print "re-asked 0s ago" and hide the informative part -- that the marker POSTDATES this run. The clamp is kept out of the comparison, pinned by a test.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
The red
|
check-pr-test-coverage.mjs revision |
result |
|---|---|
current (master / this base) |
passed=false — "No test files detected" |
| with #1666 | passed=true |
So the gate goes green on this PR once #1666 lands; no change is needed here.
Retitling to refactor: would be the wrong fix and is worth naming explicitly, because it is what the gate's own message pushes you toward: it would silence a correct-in-intent gate by mislabelling a behavioural bug fix that ships with tests. BLO-19152 recorded that same inversion for the .mjs case.
Ordering
- fix(ci): let the test-coverage gate see Python test files (BLO-31908) #1666 → unblocks this PR's
reviewgate (targetsmaster, currentlybehind). - fix(review-gate-sweep): re-read markers before the write so concurrent sweeps cannot both re-fire (BLO-31908) #1661 → this PR is stacked on it, because
refire_still_permitted()does not exist onmasteryet. Base here isBLO-31908-sweep-refire-reread-guard; GitHub will retarget tomasterautomatically when fix(review-gate-sweep): re-read markers before the write so concurrent sweeps cannot both re-fire (BLO-31908) #1661 merges. This PR cannot merge before fix(review-gate-sweep): re-read markers before the write so concurrent sweeps cannot both re-fire (BLO-31908) #1661.
Tracking on BLO-32044; both prerequisites belong to BLO-31908.
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f392d08
The core change is right and the negative control (test_a_report_against_a_stale_head_does_not_block_the_refire) is exactly the test this class of guard needs. All 118 tests pass locally at this head. One finding, in the summary/exit surface rather than the guard itself.
Critical Issues (0)
Important Issues (1)
-
[native-codex / gstack-review]
.github/scripts/sweep-stalled-ally-reviews.py:998— theREVIEWED_SKIP_REASON_PREFIXpath returnspending_sinceunchanged, so a PR the guard just proved is not stranded still tripsis_alarmingand fails the run red.The scan path handles the identical discovery differently. At
sweep-stalled-ally-reviews.py:962,ally_has_reviewed_head()being true setspending_since = None, with the comment "Either way treat as not-pending soshould_refire/is_alarmingboth skip it." The new guard discovers the same fact minutes later and skips that normalization, somain()'salarminglist at:1119-1127(which readsr[2]) counts it.Reproduced at this head, using the guard's own fixtures (PR pending 10h, Ally report served only to the pre-write re-read):
refire: False reason: skip: Ally reviewed this head between scan and write -- consolidated report on the comment surface pending_since is None? False ALARM_THRESHOLD_SECONDS: 19800 = 5.5 h is_alarming -> TrueConsequence: one
GITHUB_STEP_SUMMARYreports the same PR twice, in contradictory terms — the new section says "1 answered … the PR is no longer stranded. This one is healthy", while the:rotating_light:section below it says "still pending past 5.5h despite an automated re-fire cycle" — andsys.exit(EXIT_ALARM)turns the scheduled job red on the healthiest outcome the guard can produce. Thecontendedpath is different and should not be normalized: there the PR genuinely is still stranded.Why the new tests don't catch it:
test_sweep_stalled_ally_reviews.py:1634's_skip()helper hardcodesNonein thepending_sinceslot, soTestGuardSkipsAreVisibleInTheStepSummarynever exercises the real tuple_consider_prreturns.TestPreWriteAllyReviewedGuardbuilds the real tuple but only asserts onrefireandreason.- Return
Noneforpending_sinceon theREVIEWED_SKIP_REASON_PREFIXbranch only, mirroring:962— e.g. haverefire_still_permittedsignal which precondition declined (it already returnsprefix) and normalize at:998whenprefix is REVIEWED_SKIP_REASON_PREFIX. Then pin it with an assertion that anansweredoutcome yieldsis_alarming(...) is False, and add a_skip()variant carrying a real stalepending_sinceso the summary test can distinguish the two.
- Return
Suggestions (2)
-
[pr-review-toolkit/code]
.github/scripts/sweep-stalled-ally-reviews.py:694— the stated cost bound isn't exact. "only the reviews surface costs a request, bounded atMAX_REFIRES_PER_RUNper run" holds for the write path, butsweep()at:1051decrementsrefires_leftonly onoutcome[3], andtest_an_already_reviewed_skip_does_not_consume_a_refire_budget_slot(correctly) pins that a withheld write leaves the slot free. So each PR withheld on the reviews surface pays a request without consuming budget, and the true bound is the eligible-PR count. The practical exposure is small — cooldown-blocked PRs short-circuit before the paid read, and theanswered-via-reviews case is rare — but given how carefully the rest of this file states its residuals rather than claiming them away, the bound is worth rewording to match. -
[pr-review-toolkit/code]
.github/scripts/sweep-stalled-ally-reviews.py:432—int(-since_last)truncates, so a sub-second skew renders asre-asked 0s AFTER this run's scan clock. TheAFTER … (concurrent writer)wording still carries the informative part, so this is cosmetic, but it's the same "reads as an arithmetic bug" failure the change set out to fix. Rounding, or a<1sfloor, would close it.
Strengths
test_a_report_against_a_stale_head_does_not_block_the_refireis the load-bearing control, and its docstring names the exact failure a coarserany Ally review present -> skipwould cause (silently reinstating BLO-22892). It serves the stale report to both the scan and the re-read, which is what makes it a real control rather than a restatement.- The two-surface check is grounded in measured evidence (
#952comment-shaped with emptypulls/reviews;#937the inverse) rather than asserted, andtest_the_guard_reads_the_reviews_surface_again_rather_than_trusting_the_scanpins the extra read so a refactor can't reopen half the race while the other tests still pass. - Keeping
REVIEWED_SKIP_REASON_PREFIXdistinct fromREREAD_SKIP_REASON_PREFIX, withtest_the_two_skip_reasons_are_distinguishableasserting neither is a prefix of the other, is the right call forstartswith()-based classification — and the summary text explicitly tells the operator that one is contention evidence and the other is healthy. - The residual is inherited explicitly ("BLO-32044 narrows a DIFFERENT window with the same mechanism and inherits the same residual") instead of the guard being described as closing the race.
Recommended Action
- Normalize
pending_sinceon theansweredbranch before merge — as written, the guard's success case fails the job red. - Consider the two suggestions opportunistically.
Formal COMMENTED review: this PR is authored by app/allyblockcast, so GitHub bars the App from APPROVE on it. The consolidated result is delivered as a formal review (not a plain comment) so it carries a commit_id and is visible to the exact-head gate.
Thinking Path
Linked Issues or Issue Description
Fixes: BLO-32044
Refs BLO-31908 / #1661 — this PR is stacked on that branch, because
refire_still_permitted()does not exist onmasteryet. Base isBLO-31908-sweep-refire-reread-guard; GitHub will re-target tomasterwhen #1661 merges.Refs BLO-31818 — the dropped concurrency group whose live cost the new summary block makes measurable.
Raised by Ally reviewing #1661 (
native-codexlens) and verified independently against the script on that branch. Explicitly out of scope for BLO-31908 and not a defect it introduced; the guard is simply the natural home for it.What Changed
The fix (commit 1)
refire_still_permitted()now re-appliesally_has_reviewed_head()as well as the cooldown, and withholds the DELETE, the POST and the marker comment when Ally has reviewed the current head.#952carried 4 comment-shaped reviews with an emptypulls/952/reviews, while#937carried 4 formal review objects and no comment-shaped one.GET .../reviewsis reached only when both free checks pass. Every branch that declines is a branch that will not write, somay_refire=Falseanddry_runstill pay for nothing.REVIEWED_SKIP_REASON_PREFIX, held distinct from the cooldown prefix: one says "somebody re-asked too recently", the other says "the review we were about to ask for has already landed".MAX_REFIRES_PER_RUNsemantics unchanged — the guard returnsrefire=Falseandsweep()decrements on that flag, so a guard skip consumes no slot.Observability (commit 2) — two non-blocking items Ally raised on #1661, deferred there rather than staling a clean review at head
bc73d71e:GITHUB_STEP_SUMMARYblock, counting contended and answered separately.failed/deferred/alarmingeach already had one; the contended count is the only direct evidence that dropping the concurrency group (BLO-31818) has a live cost, and it was the one signal you had to grep logs to find. The block is conditional, so a clean run carries no permanent "0 withheld" line.nowis sampled once per run, so a marker posted mid-run by a concurrent sweep is genuinely newer than the run's clock and the message readre-asked -3s ago < cooldown 7200s. Reworded to name the skew rather than clamp it withmax(0, ...), which would printre-asked 0s agoand hide the informative part — that the marker postdates this run. The clamp is kept out of the comparison, pinned by a test.Verification
python3 -m unittest discover -s .github/scripts -p 'test_sweep_*.py'— 118 passed (99 before this PR, +19). This is the workflow's ownUnit tests for the sweep's pure decision logicstep (review-gate-sweep.yml:184,pr.yml:314).The interleaving needs Ally to answer inside a ~2min window against a measured 5m–74m response latency, so it is not reproducible on demand. The unit tests stand in for it, and they verify the guard rather than the absence of the race. They simulate the straddle the same way #1661's do — by serving the scan and the pre-write re-read different pages of the same surface.
Because "these tests pass" is weak evidence for a guard, each new behaviour was confirmed against a deliberately broken copy:
The second row is the load-bearing one. An over-broad check passes both positive tests while disabling the reconciler outright — a PR whose head has moved past an older review carries an Ally report on both surfaces permanently, and that is precisely the state this sweep exists to re-fire. Suppressing it would reinstate BLO-22892 silently: no re-fire, no alarm.
test_a_report_against_a_stale_head_does_not_block_the_refireis the single test separating a correct guard from that, and it is the only one the over-broad mutant fails.Risks
Low, and the risk is asymmetric in the direction this change is careful about.
ally_has_reviewed_headrequires the consolidated envelope plus an exact head attestation, and ignores the mutablereview.commit_id. That bias is pre-existing and inherited deliberately, not loosened here.GET .../pulls/{n}/reviewsper re-firing PR — at mostMAX_REFIRES_PER_RUN(5) per run against 1,000/hour/repository, negligible beside the ~270 reads the scan already issues, and short-circuited on every non-writing path.TestRereadGuardResidualIsStated).Model Used
Claude Opus 4.5 (
claude-opus-4-5, 1M context) via Claude Code, with extended thinking and tool use (file editing,git,gh, local test execution).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template