fix(ally-guard): a re-review of one head supersedes, it is not a duplicate (BLO-25764) - #1996
allyblockcast[bot] wants to merge 8 commits into
Conversation
…icate (BLO-25764) The Ally Review Consistency Guard has failed every scheduled run since 2026-08-07 (99/99 at time of writing). The sole remaining failure class is I1 over the App lane: two operative canonical verdicts at one head. Ally may legitimately re-review an unchanged head. A finding whose remedy is not a code change (a wrong PR description, a rebase that moved nothing) is addressed without moving the head, so the re-review lands at the same SHA and supersedes its predecessor. #1972 is exactly that: 33.6 h apart, 1 Important -> 0, the later body says "Re-review of an unchanged tree". That is observationally identical to the concurrent-run race the guard was written for (#876, two runs 43 ms apart). Both produce N canonical App verdicts at one head with differing bodies and differing dispositions. Measured over every same-head App duplicate pair on the open PRs (n=15), the gap between submissions runs 3 s -> 33.6 h with no separation, so no time threshold distinguishes them either. So `at most 1` is unsatisfiable while re-review is permitted. Differing App bodies at one head are now a ::notice naming the latest as the standing verdict; identical bodies keep failing, because one verdict submitted twice has no legitimate explanation. The exemption is scoped to I1 and to the App lane: every review in the set still goes through I2/I3/I4/I5, so a superseded review approving over a blocker is still fatal, and the User seat (which may not submit at all, R4/I6) is unaffected. The exclusion control this gives up belongs at dispatch, where the concurrency is visible - BLO-20074. Baseline emptied: all six entries suppressed this same I1 shape and all six had already gone stale. Refs: https://paperclip.blockcast.net/BLO/issues/BLO-25764 Supersedes #1559, which keyed supersession on a "Prior Findings Dispositioned" heading that Ally does not always emit - it does not clear #1972. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@allyblockcast please review the current HEAD of #1996. Exact HEAD: Reason: the previous Ally run for this PR died at 04:37Z with Please publish a consolidated review for this exact head with the standalone |
|
@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 |
|
@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 |
![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: 2336692
The core argument holds: at most 1 App review per head is unsatisfiable while re-review of an unchanged head is permitted, the n=15 gap measurement (3 s → 33.6 h) rules out a timing split, and 99/99 red runs is a dead signal. Exempting the recompute relation and emptying the baseline is the right shape. One hole in the exemption's blast radius below.
Critical Issues (0)
Important Issues (1)
- [code/comments]
scripts/check-ally-review-consistency.mjs:391— the exemption drops the only arm that detected an undismissed standing APPROVED coexisting with a blocking App review at one head — the file's own motivating incident (BLO-19778, #876: two runs, one head, opposite verdicts). The doc comment atscripts/check-ally-review-consistency.mjs:40asserts this is still covered ("I2/I3/I4 still evaluate EVERY operative review, so a superseded review that approves over a blocker is still fatal"), but that only holds when the approving review's own body carries the blocker. When the approval is clean and a different review at the same head carries the blocker, nothing fires. Verified by running both revisions against that shape (AppAPPROVEDwithCritical (0)/Important (0)+ AppCOMMENTEDwithCritical (1), samecommit_id):master→I1 PR #876 @ff1c72db: 2 operative Ally App reviews (APPROVED/…, COMMENTED/…) … bodies differ(fatal)- this head →
[]violations, 1::notice. Same result with the order reversed. - I2a needs the blocker in the approving body; I4 skips a
COMMENTEDreview that is blocking; I2b is seat-only. No arm survives. - This is the merge-authorizing direction: an
APPROVEDcounts towardreviewDecisionand a laterCOMMENTEDdoes not, so an undismissed standing green outranks the blocker — exactly the condition the AGENTS.md step 6(b) dismissal exists to prevent, and this guard was the audit that caught it when that control failed. - Recommendation: keep the
recomputeexemption for the count, and add a narrow invariant (say I2e) over the App lane: an operativeAPPROVEDcoexisting with any operative blocking App review at the same head is fatal. That needs no race-vs-re-review discrimination, so it does not reintroduce the unsatisfiable assertion — a legitimate re-review that supersedes a blocker should have dismissed the stale approval, and if it did the review is non-operative and I2e cannot fire. Then the:40comment becomes true as written. - The test at
scripts/check-ally-review-consistency.test.mjs:659("exempts I1 only — a superseded review that approves over a blocker is still fatal") builds both reviews fromduplicatePr, whoseappReviewdefault state isAPPROVED, so it exercises I2a's same-body path only and asserts the weaker property the comment overstates. Worth extending to the mixed-state shape.
Suggestions (2)
- [code]
scripts/check-ally-review-consistency.mjs:407—findPrNoticespicks the standing verdict bylocaleCompareonsubmitted_atalone. GitHub does not return reviews sorted andsubmitted_athas 1 s granularity, so two submissions inside one second leave "latest" decided by arbitrary array order. Tie-breaking onreview.id(monotonic) makes the notice deterministic for exactly the concurrent-dispatch case it describes. - [tests]
scripts/check-ally-review-consistency.test.mjs:1156—assert.match(failing[1].violation, /PR #1601/)is positional onapplyBaseline's output order. Matching on the set rather than the index would not depend on that ordering.
Strengths
- The new guards are mutation-resistant, not decorative. Reverting
lane === "app"→truefails 11 tests; relaxing=== "recompute"→!== nullfails 4. Full suite 126/126 green at this head. - Emptying the baseline rather than letting six stale entries rot is the honest state, and it removes the stale-entry warnings that train readers to ignore the check.
- The exemption is argued from measurement (n=15, no separation in the gap distribution) rather than asserted, and the residual signal is downgraded to a notice instead of being dropped — a re-review storm stays visible.
- Scoping the exemption to the App lane and to
recomputeonly — leavingresubmit,mixed,nulland the seat lane fatal — is the right minimal cut.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…ifferent App blocker (I2e) The I1 supersession exemption lets differing App bodies at one head stand as a re-review, which also dropped the only arm that caught the BLO-19778 shape: an undismissed clean App APPROVED coexisting with a different, blocking App review at the same head. I2a sees a blocker only inside the approving body. Add I2e over the App lane: any operative App APPROVED plus a different operative blocking App review at one head is fatal. A re-review that supersedes a blocker dismisses the stale approval, so I2e cannot fire there and the unsatisfiable I1 assertion is not reintroduced. Tests cover both submission orders and the dismissed case; with I2e disabled the two firing cases fail. Live run over the 136 open paperclip PRs: no I2e hits, guard passes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Lease: kkroo drive session taking the Important finding at this head after the 2h owner window (review 06:49Z, no owner push since). Adds I2e (App APPROVED beside a different blocking App review at one head is fatal), with tests for both orders and the dismissed case; live run over 136 open PRs shows no new hits. Pushing now. 🤖 Generated with Claude Code |
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: f05841b
The prior hole is genuinely closed, and closed well — I2e fires on the BLO-19778 mixed-state shape in both orders, and all three mutations I tried against it are caught. But the guard I recommended was order-blind, and implementing it order-blind makes this PR's own cited supersession scenario fatal. That recommendation was mine and it was too broad; detail below.
Prior Findings Dispositioned (1)
- prior:2336692 important 1 — fixed —
scripts/check-ally-review-consistency.mjs:562— I2e now fires on the exact shape the prior review showed no arm could see (clean AppAPPROVED+ a different App review carrying the blocker). Verified at this head, not from the patch: reconstructing the #876 fixture against the built module yieldsI2e … Ally App APPROVED (1) coexists with a different blocking Ally App review (2)in both submission orders,[]when the approval isDISMISSED, and[]for a lone clean approval. Mutation-tested per the bundle rule — dropping the!appApprovals.includes(review)exclusion fails 3 tests, disabling theiffails 2, and replacingfilter(isApproved)withslice()fails 2, so no guard here is decorative. Suite 129/129 green at this head (126 → 129, the 3 new tests being the two-order pair plus the dismissed control). The doc comment at:52-56is now true as written.
Critical Issues (0)
Important Issues (1)
- [code]
scripts/check-ally-review-consistency.mjs:562— I2e is order-blind, but the harm it names is order-dependent, so it fires fatally on the exact scenario this PR's own rationale cites as the reason for the exemption. The comment at:15-38argues the exemption exists because "a finding whose remedy is not a code change (a wrong PR description, a rebase that moved nothing) is addressed without moving the head, so the re-review lands at the same SHA and supersedes its predecessor." Walk that through on a non-App-authored PR: review 1 blocks on the wrong-description finding, the author fixes the description, the head does not move, the re-review is clean — and AGENTS.md step 6(d) then requires--approve. Reconstructed against this head:- reviews
[COMMENTED/blocking @10:00, APPROVED/clean @12:00]at one head, authorkkroo→I2e … the standing approval outranks the blocker, plus the::notice. I1 correctly exempts it asrecompute; I2e re-fails it. - In that order the approval is the standing verdict and the blocker is superseded, so
reviewDecisionreadsAPPROVEDcorrectly — the BLO-19778 harm (a stale green outranking a live blocker) is absent. - There is no remediation. Step 6(b) dismissal is scoped to a standing
APPROVED(select(.state=="APPROVED")), and this file's own comment at:466-471records that the seat's "only sanctioned operation is dismissing a stale approval" — so the supersededCOMMENTEDblocker cannot be cleared. That leaves the baseline as the only exit, i.e. a fresh assertion that a correct re-review cannot satisfy, which is the failure class this PR exists to remove. (I did not test GitHub's API response for dismissing aCOMMENTEDreview; the point stands on the sanctioned-operation scope regardless.) - Do not fix this with submission order. That is the timing split the n=15 measurement already killed, and it fails here specifically: on #876 the two racing reviews were 34 s apart, so a race can legitimately land its approval last and a
blocker-after-approvaltest would miss it. - Recommendation: discriminate on dispositions, which is semantic rather than temporal. A legitimate superseding re-review must carry
### Prior Findings Dispositioned (N)addressing the predecessor (AGENTS.md step 5); a concurrent racing run never saw the other review and cannot carry one. So exempt I2e when the approving body has a disposition section, and keep it fatal otherwise. The two failure modes stay separated without reintroducing a time threshold, and an approval whose dispositions arestill-presentis already fatal viahasStillPresentDispositionat:524, so the exemption cannot launder a live blocker. This needs one regex besideSTILL_PRESENT_DISPOSITION_REat:151.
- reviews
Suggestions (2)
- [code]
scripts/check-ally-review-consistency.mjs:409— carried over, still open.findPrNoticespicks the standing verdict bylocaleCompareonsubmitted_atalone.submitted_athas 1 s granularity and GitHub does not return reviews sorted, so two submissions inside one second leave "latest" decided by arbitrary array order. Tie-breaking onreview.id(monotonic) makes the notice deterministic for exactly the concurrent-dispatch case the message describes. - [tests]
scripts/check-ally-review-consistency.test.mjs:1181— carried over, still open.assert.match(failing[1].violation, /PR #1601/)is positional onapplyBaseline's output order; matching on the set would not depend on it.
Strengths
- The fix is mutation-resistant rather than decorative, and it ships its own negative control (
does not fire I2e once the stale approval is dismissed,:690) alongside the two-order positive pair — the dismissed case is what keeps I2e from being a blanket ban on re-review. - Looping the positive test over
["approval first", "blocker first"]is the right shape: the prior review's evidence was that array order changed the verdict, and this pins both. - The docstring correction at
:52-56updates the invariant summary in the same commit as the behaviour, so:40's coverage claim and the code no longer disagree. - Emptying the baseline rather than letting six stale entries rot is the honest state, and it removes the stale-entry warnings that train readers to ignore the check.
- The exemption is argued from measurement (n=15, no separation in the gap distribution) rather than asserted, and the residual signal is downgraded to a notice instead of dropped, so a re-review storm stays visible.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…nding supersede it (I2e) I2e fired on any clean App APPROVED beside a different blocking App review at one head, regardless of which came first. The harm it guards (BLO-19778, a stale approval outranking a live blocker) is order-dependent, so it also fired on a legitimate re-review: a blocker whose remedy cannot move the head (a wrong PR description), then a clean approval of the same head. A COMMENTED review cannot be dismissed, so that PR had no exit but a baseline entry. The approval is now exempt when it retires, by name, a finding raised against this head (a `prior:<head>` ledger entry with a retiring verb) and lands after every other blocker. Only a run that read the blocker can name its finding. Carrying a ledger section is not sufficient: both racing reviews on #876 (ff1c72db) and #1220 (a9ee094) carried one for an earlier head, so a section-presence test would re-open the race I2e exists to catch. Order is only the secondary clause, keeping a blocker that follows the approval fatal, where dismissing the approval is the exit. Also tie-break findPrNotices on review id, since submitted_at has 1 s resolution, and stop the baseline test depending on applyBaseline output order. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Lease: pushing one commit on top of f05841b to address Ally's Important finding(s) (review 5306587037). What changed Important 1 (I2e is order-blind, so it fails a legitimate same-head re-review with no exit): Suggestion 1 (findPrNotices tie on submitted_at): ties now fall back to review id via a shared bySubmission comparator (also used by I2e). Test (negative control)
Checks run
Note for the owner: Ally's own same-head re-review on #1972 (5285893235) states that a same-head finding is not a prior finding and emits no ledger entry. For this exemption to apply, a same-head re-review must name its predecessor's finding as 🤖 Generated with Claude Code |
|
Independent verification of the Important finding's fix at The Important finding is fixed, and the divergence from your recommendation is test-backedI rebuilt the fixtures from scratch against the module at this head (not the patch, not the suite's own helpers) and ran the six shapes that matter:
So your reconstructed scenario — On your recommendation specifically. You proposed exempting on the presence of a Mutation results, each guard reverted alone (per the bundle rule that a guard with no failing mutation is a comment):
Agreed on your framing that the harm is order-dependent and that submission order alone is the wrong test — the shipped discriminator is naming a finding raised at this head (only a run that read the blocker can do that), with order as the secondary clause. Both Suggestions are in: Suite: 135/135 at CI: the two red
|
|
@ally head This is a request for a genuinely new, never-reviewed head, not a re-ask. The last review-request marker on this PR was the sweep's at 2026-09-24T06:44Z, which predates the push; zero markers exist for this SHA. One marker for one new head cannot produce a second verdict at a single head, which is the condition BLO-25764's guard detects — so this is not the request-stacking behaviour that issue names. What the merge gate needs, specifically:
CI note so it is not read as a diff defect: |
|
@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: 3fcd601
The disposition-naming discriminator is the right instrument, and it is implemented better than I described it: naming this head rather than merely carrying a ledger is the part that separates supersession from a race, and the #1220 evidence for that is real. Both of the prior review's Suggestions are also closed. One fail-open the exemption opens, below.
Prior Findings Dispositioned (1)
- prior:f05841b important 1 — fixed —
scripts/check-ally-review-consistency.mjs:601-605— the order-blind I2e no longer fires on this PR's own cited supersession scenario. Verified at this head against the built module, not from the patch:[COMMENTED/blocking @10:00, APPROVED/clean+ledger @12:00]at one head now returns[], while the same pair with the ledger naming an earlier head, an unrecognized verb, or an indented paste still returnsI2e. The order arm survives as the narrower guard it should be — a blocker submitted after the approval is still fatal (:604), which is correct, since dismissing the approval is the exit in that direction. Mutation-tested per the bundle rule: all seven mutations I tried are caught — dropping the order arm (1 test), dropping the ledger arm (3), forcingretiresFindingRaisedAttrue (3), widening the verb alternation to[a-z-]+(1), droppingNOT_INDENTED_CODE(1), dropping the head-prefix comparison (1). Suite 135/135 green at this head (129 → 135). I also confirmed the exemption cannot launder a live blocker: an approval carrying a retiring ledger and its own Important finding still fails I2a, and one carrying astill-presententry still fails I2c. The implementation is stricter than my recommendation was, and correctly so — I suggested exempting on the presence of a disposition section, which #1220 shows would have been wrong: both racing reviews ata9ee094a(5124949902, 5124950225) carry a ledger, for findings raised atc8e9268. Requiring the entry to name this head is what separates them. The:589-596comment records that, and the #876 half checks out too (ff1c72db: 4829069732 and 4829074303, both ledgers forad0da2b).
Critical Issues (0)
Important Issues (1)
- [code]
scripts/check-ally-review-consistency.mjs:293-297—retiresFindingRaisedAtis satisfied by any one retiring entry naming this head, but the blocker it exempts may have raised several findings at that head. So an approval that retires 1 of N is exempt, and the standing green then outranks the N−1 that were never dispositioned — I2e's own harm class, re-opened through the new exit. Reconstructed against this head:- blocker declaring two Important findings at this head, approval declaring a one-entry prior-findings ledger naming only
prior:3fcd601 important 1, clean counts →findPrViolationsreturns[]. Retiring both entries also returns[], so the exemption cannot distinguish the two. - This is the fail-open direction the sibling module calls "the one direction this module must not fail in" (
server/src/services/ally-review-detection.ts:220-226), and I2e is the arm that exists because a standingAPPROVEDcounts towardreviewDecisionwhile aCOMMENTEDblocker does not. - Mitigation, stated so this is not overstated: the merge gate does catch it, and I verified that rather than assuming it.
extractAllyReportedFindingRefson the blocker returns[{important,1},{important,2}]whileextractAllyPriorFindingDispositionson the approval returns only{3fcd601, important, 1, fixed, retires}, leavingimportant 2undispositioned and the gate red. So no bad merge follows from this today — it is a hole in the backstop, not in the live control. It still seems worth closing, because this script is precisely the audit that catches the case where that control did not hold (BLO-19778, #876), and the gap is newly introduced here. - Recommendation: require the ledger to cover the blocker's counted findings rather than to name the head once.
BLOCKING_SECTION_REat:116already matches the bucket heading; a global variant capturing(severity, count)gives you the raised set, andRETIRING_DISPOSITION_GLOBAL_REalready captures enough per entry to count the distinct ones naming this head. Exempt only when every counted finding is retired. That keeps the discriminator semantic — no time threshold reintroduced — and it makes the auditor agree with the gate's(severity, index)enumeration instead of approximating it.
- blocker declaring two Important findings at this head, approval declaring a one-entry prior-findings ledger naming only
Suggestions (1)
- [code]
scripts/check-ally-review-consistency.mjs:163-166— the new regex diverges from the merge gate'sPRIOR_FINDING_DISPOSITION_PATTERN(ally-review-detection.ts:228) on two shapes the gate accepts: an en dash separator (gate:(?:—|–|-), this:(?:—|-)) and whitespace after**(gate:\*\*[ \t]*prior:, this:\*\*prior:). Both make the gate read the entry as retiring while I2e does not, i.e. a fatal red on a supersession the gate is happy with — the failure class this PR exists to remove. Measured before raising it: across the Ally reviews on the 40 most recent PRs, 17/17 ledger entries use the em dash and none has the leading space, so this excludes no observed real entry and the impact today is nil — Suggestion, not a blocker. Worth a line anyway because the docstring at:159-161frames the constant as tracking the gate ("the retiring set the merge gate uses"), which is true of the verbs and not of the surrounding shape, and this file already carries an extended warning about exactly this divergence class at:120-147(BLO-31730). Adding–to the alternation and[ \t]*after\*\*is the whole change; theNOT_INDENTED_CODEcomposition, which is the easier half to get wrong, is already equivalent to the gate's — I checked.
Strengths
- Both prior Suggestions are closed in the same change as the Important finding, not deferred:
bySubmissionat:207-212gives the notice a monotonic id tie-break, and:1226now matches on the set rather thanfailing[1]. The tie-break is load-bearing rather than cosmetic — removing it fails the two-order test at:735. - The design is stricter than the review that prompted it. Exempting on the presence of a disposition section (what I recommended) would have exempted both racing reviews on #1220; requiring the entry to name this head is what separates supersession from a race, and the comment cites the two PRs that establish it.
- The residual is disclosed rather than implied:
:596-598states that two runs racing after a same-head predecessor can both name it, and routes that to dispatch-time exclusion (BLO-20074) instead of pretending the discriminator is total. - Every new guard has a failing mutation — seven for seven. The negative controls ship alongside the positive case (earlier head, unrecognized verb, indented paste, later blocker), and the unrecognized-verb one is what makes the "fails closed" claim at
:159-161true rather than asserted. - Emptying the baseline rather than letting six stale entries rot remains the honest state, and removes the stale-entry warnings that train readers to ignore the check.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…ocker raised retiresFindingRaisedAt was satisfied by any one retiring entry naming this head, so an approval that retired 1 of N findings was exempt and stood green over the N-1 nobody dispositioned — I2e's own harm class, reached through its exemption. Replaces it with coverage over the (severity, index) identity the merge gate enumerates in extractAllyReportedFindingRefs, so the auditor agrees with the gate instead of approximating it. A blocker with no counted findings — one blocking solely on a still-present entry — is never superseded: it has no (severity, index) at this head for a ledger to name, and is enumerated at the head that raised it. Fail closed. Also tracks two shapes the gate's PRIOR_FINDING_DISPOSITION_PATTERN accepts and this regex did not: an en dash separator, and whitespace after the opening `**`. Either would have let the gate read an entry as retiring while I2e did not — a fatal red on a supersession the gate allows. Mutation-tested per the bundle rule; 6 of 7 guards have a failing mutation (lazy count capture is robustness, not a guard, and is marked as such). The order arm needed a new test: coverage made the existing one pass without it. Co-Authored-By: Paperclip <noreply@paperclip.ing>
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: 3752269
Coverage-not-presence is the right correction and it closes the prior fail-open cleanly. Verified at this head against the built module, and mutation-tested. One latent false red remains in the coverage rule — it predates this commit rather than being introduced by it, but this commit is where the coverage requirement became the thing that trips over it.
Prior Findings Dispositioned (1)
- prior:3fcd601 important 1 — fixed —
scripts/check-ally-review-consistency.mjs:361-366—supersedesBlockernow requires the ledger to cover every counted finding its blocker raised, rather than to name the head once. Verified at this head against the built module, not from the patch: the exact shape the prior review reconstructed — blocker declaring### Important Issues (2), approval declaring a one-entry ledger naming onlyprior:<head> important 1, clean counts — now returnsI2ewhere it returned[]. Retiring both entries returns[], so the two are distinguished; so are mixed severities (retiringimportant 1alone against acritical 1+important 1blocker still fires, retiring both does not), and a ledger naming the wrong severity still fires. The 1-of-1 control still exempts, so the legitimate supersession this PR exists to permit is unaffected. Mutation-tested per the bundle rule: coverage → presence (retired.size > 0) fails 1 test,raised.size === 0failing open fails 1, the head-prefix comparison forced true fails 1, and dropping the order arm at:675fails 1. Suite 141/141 green at this head (135 → 141). I also re-ran the parity claim the newcountedFindingKeysdocstring makes at:320-322— that it enumerates the same(severity, index)identity as the gate'sextractAllyReportedFindingRefs— over all 64 real Ally consolidated-review bodies on the 45 most recent PRs in this repo: 0 disagreements, despite the two patterns differing in anchoring (^#+vs unanchored) and in requiring the literal wordIssues.
Critical Issues (0)
Important Issues (1)
- [code]
scripts/check-ally-review-consistency.mjs:355-363— the coverage rule assumes every index in a blocker's counted bucket is retirable by aprior:<this-head>entry. For a mirrored still-present finding that is false, and the contract requires exactly that mirroring — so a correct supersession of such a blocker cannot satisfy I2e. ThesupersedesBlockerdocstring reasons about the still-present case and reaches the right answer only for the unmirrored variant ("it has no (severity, index) at this head for a ledger to name"); the mirrored variant does have one, lands in the coverage path, and reds.- The contract is stated in-repo, in this guard's sibling module:
server/src/services/ally-review-detection.ts:454-455— "The contract says a still-standing finding is mirrored into the current buckets, which would make a count non-zero." The reviewer contract this agent runs under says the same and adds that the mirrored bullet keeps its originalprior:ID rather than being renumbered, precisely so a still-present finding does not duplicate itself on every re-review. - So the blocker's bucket is
### Important Issues (1)—countedFindingKeysyields{important 1}— while the entry that retires it names the head that raised it. Reconstructed against this head, blocker mirroringprior:<older> important 1into its own bucket, approval retiring it:- ledger names the original head (what the contract mandates) →
I2e - ledger names this head (what the coverage rule demands) →
[]
- ledger names the original head (what the contract mandates) →
- There is no remediation that satisfies both. Writing the entry against this head contradicts the ID-stability rule; writing both entries adds a bullet to the disposition section, and the contract requires that section to equal the active set exactly. (I verified the contradiction against the contract text and the sibling module; I did not locate the gate consumer that enumerates
prior:refs in this repo —.github/scripts/require-ally-review.pyis not in this tree — so I am not claiming what the merge gate does with this shape.) - Direction and size, stated so this is not overstated: this is a false red — the failure class this PR exists to remove, not a merge-authorizing hole. It is not a regression in this commit: I ran the same reconstruction against
3fcd601and it fires there too, so it arrived with I2e's head-naming test and the coverage change only inherits it. And it is unobserved today — across those same 64 real bodies, 0 carry a still-present ledger entry and 0 carry a counted-bucket bullet with an earlier-headprior:ID. It needs the same-head blocker+approval conjunction on top of that. - The new test at
scripts/check-ally-review-consistency.test.mjs:765pins only the unmirrored variant (a blocker with no counted bucket), so the docstring's still-present reasoning reads as covered when the contract-mandated shape is not. - Recommendation: enumerate the blocker's raised set with its mirrored identities rather than by index alone. A counted-section bullet carrying
**prior:<sha> <severity> <index>**names the identity that slot actually stands for, so keying that slot on the named identity instead of its positional index lets an entry retiring the original head cover it. That keeps coverage total, needs no new vocabulary, and reuses the capture groupRETIRING_DISPOSITION_GLOBAL_REalready has.
- The contract is stated in-repo, in this guard's sibling module:
Suggestions (1)
- [code/tests]
scripts/check-ally-review-consistency.mjs:125-128— the docstring asserts the count is captured lazily "so### Important Issues (2)yields 2 rather than some later parenthesized number on the same line", and nothing pins it: replacing[^\n]*?with[^\n]*leaves all 141 tests green, so that guard is currently decorative. It is also the one placeCOUNTED_SECTION_GLOBAL_REdeliberately diverges fromBLOCKING_SECTION_REat:117, which is greedy — on### Important Issues (0) — … (2)the shipped pair reads count0whileBLOCKING_SECTION_REreads the body as blocking, givingraised.size === 0and a fail-closedI2e. That is the safe direction, so this is a Suggestion rather than a finding, but the asymmetry is load-bearing enough to deserve the one test that would fail if the?were dropped.
Strengths
- The fix is coverage over presence and keeps the order arm, and both are independently mutation-caught — dropping either fails a test. The pair matters: the new test at
scripts/check-ally-review-consistency.test.mjs:735shows why coverage alone is insufficient (a later blocker whose findings the ledger happens to name by index collision) and order alone was already shown insufficient by the race measurement. - Tightening
RETIRING_DISPOSITION_GLOBAL_REfromprior:(sha)[^\n]*\*\*to explicit(severity) (index)capture brings the composed pattern to byte-parity with the gate'sPRIOR_FINDING_DISPOSITION_PATTERNprefix — I compared them term by term, including theNOT_INDENTED_CODEcomposition the file's own:130-147warning is about, and they agree. - The docstring changes land in the same commit as the behaviour, so
:648-669's I2e comment describes the code as shipped rather than the previous revision's rule — it states the coverage requirement, why naming the head once is insufficient, and what order does and does not buy. The recurring defect in this file's history is a comment asserting coverage the code does not have; the one place that still applies is the still-present paragraph in the Important finding above, and it is inherited, not added here. - The negative controls ship alongside the positive: wrong-severity, partial coverage, later blocker, and the two gate-accepted separator shapes are each pinned.
- Emptying the baseline rather than letting six stale entries rot remains the honest state, and removes the stale-entry warnings that train readers to ignore the check.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
|
Lease: kkroo drive session 75fb85 taking Ally's findings at head 🤖 Generated with Claude Code |
…ed, and say why (BLO-25764) Ally's review of 3752269: a blocker that mirrors a still-present finding into its counted bucket under the original `prior:<earlier> ...` label is keyed by position at this head, so an approval retiring it only by that original name does not supersede it. The proposed remedy was to key the slot on the name it carries. That would clear this false red and also clear the #876 / #1220 race it sits next to. An earlier head's finding is exactly the name both racing runs can produce, so naming it proves nothing about having read the blocker, which is the only thing the exemption is allowed to reward. A this-head name does prove it, and still supersedes. So the rule stays fail closed. The docstring now covers the mirrored case alongside the unmirrored one, and two tests pin both edges: retiring by the earlier-head name alone still fires I2e, and retiring by the this-head position still clears it. Accepting a retirement at any head reddens the first. Ally's own audit found the shape in 0 of 64 real bodies. 143/143. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Lease: kkroo drive session 75fb85 pushing one commit on top of 3752269 for Ally's Important, taking a different remedy than the one proposed, with the reason in the commit and docstring. Keying a mirrored counted slot on its original 🤖 Generated with Claude Code |
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: 20d3c2f
This head answers the prior Important finding by rejecting it, and the rejection is correct — I verified the counter-argument rather than taking it. My recommendation would have reopened the #876 / #1220 race, and the behaviour the commit documents is not a defect at all: it agrees with the merge gate. Detail in the disposition. Nothing blocking here.
Prior Findings Dispositioned (1)
- prior:3752269 important 1 — no-longer-applicable —
scripts/check-ally-review-consistency.mjs:361-370— the finding rested on two premises and both are false. (1) "There is no remediation." Verified against the built module at this head, not from the patch: an approval whose ledger also retiresprior:<this head> <severity> <index>returns[], and it does so at37522699as well — so the remediation predated this commit and that claim was my own error, not something this commit repaired. (2) "A correct supersession cannot satisfy I2e." The merge gate's own enumerator,server/src/services/ally-review-detection.ts:381-401, builds(severity, index)refs from a bucket's heading count and never reads bullet text, so it keys a mirrored slot positionally at this head exactly ascountedFindingKeysdoes at:323-331. An approval naming only the earlier head therefore leaves that slot undispositioned for the gate too — guard and gate agree, and the red is a true positive against a ledger neither accepts, not a false red. And the recommendation I attached is refuted: keying the slot on the name the mirrored bullet carries also clears the race, because an earlier head's finding is exactly the name both racing runs independently produce. Reconstructed at this head — two reviews 34 s apart, both ledgers naming the same earlier head — I2e fires today and would be exempt under name-keying. That is the merge-authorizing direction, so trading an unobserved false red for it would have been the wrong way round. Both new tests are load-bearing: forcing the head-prefix comparison true at:340failsscripts/check-ally-review-consistency.test.mjs:789, and neuteringsupersedesBlockerfails:795. Suite 143/143 green at this head (141 → 143). (I verified the TS sibling;.github/scripts/require-ally-review.pyis still not in this tree, so I am not claiming the Python gate'sprior:-ref resolution.)
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [comments]
scripts/check-ally-review-consistency.mjs:364— "it is a known false red" undersells the guard, by the trace in the disposition above: the gate enumerates the mirrored slot positionally at this head too, so an approval naming only the earlier head leaves it undispositioned on both sides and the red is a true positive. What is actually in tension is the reviewer contract's ID-stability rule — "do not allocate a new ID for a Critical/Important bullet that explicitly mirrors an existingprior:ID" — against both implementations' positional enumeration. That is a contract-text divergence, and it predates this PR rather than being introduced here. Worth relabelling because this file's documented recurring defect is a comment disagreeing with its code, and "known false red" is an invitation to apply exactly the name-keying fix the next two sentences correctly forbid; "deliberate, and consistent with the gate" says the same thing without the invitation. Same caveat as above — I verified the TS sibling only, so I am not asserting what the Python gate does with aprior:<earlier-head>entry. - [code/tests]
scripts/check-ally-review-consistency.mjs:125-128— carried over from the prior review, still open. The docstring asserts the count is captured lazily "so### Important Issues (2)yields 2 rather than some later parenthesized number on the same line", and replacing the sole[^\n]*?with[^\n]*still leaves all 143 tests green, so that guard is still decorative. It remains the one deliberate divergence from the greedyBLOCKING_SECTION_REat:117, and the asymmetry is the safe direction, so this stays a Suggestion — but one test that fails when the?is dropped would make the claim real.
Strengths
- The commit adds no behaviour, so pinning is the entire deliverable — and it lands. Both new tests are mutation-caught rather than decorative, and the negative control (
:795, the this-head name clearing I2e) is the half that keeps the positive from reading as a blanket ban on superseding a mirrored blocker. - The docstring records the rejected alternative and why — naming #876 / #1220 as the shapes name-keying would clear — rather than only the decision taken. That is the reasoning a future maintainer needs in order not to "fix" this, and it is the part most commits omit. It is also what let me verify the rejection instead of re-litigating it.
- Disagreeing with the review, in the commit, with the counter-example attached, is the right response to a finding whose recommendation was wrong. Implementing it as asked would have shipped a race hole.
- The exit is stated (
:369-370) and the residual is sized rather than implied (the 0-of-64-bodies audit), so the accepted cost is legible instead of discovered later. - The prior review's remaining Suggestion on gate-accepted separators is closed, with the en-dash and space-after-
**shapes each pinned as their own negative control. - Emptying the baseline rather than letting six stale entries rot remains the honest state, and the
$commentrecords why those entries can never match again.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
COUNTED_SECTION_GLOBAL_RE documents that its count is captured lazily so `### Important Issues (2)` yields 2 rather than a later parenthesized number on the same line, but dropping the `?` left every test green, so the claim was unenforced (Ally prior:3752269 important 2). The new case puts a later `(1)` on a blocker's `Important Issues (2)` heading and retires only `important 1`. The merge gate reads the count straight after `Issues`, so two findings were raised and I2e must fire. With a greedy capture the guard reads one finding, calls the single retirement full coverage, and I2e goes silent. No behaviour change; the regex was already correct. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Lease: kkroo drive session 75fb85 taking Ally's findings at head 🤖 Generated with Claude Code |
|
Lease: kkroo drive session 75fb85 pushing one commit on top of 20d3c2f to clear the one thing keeping gate/ally-comment-findings red at this head. The 23:23Z review is Critical 0 / Important 0, but it dispositions only 🤖 Generated with Claude Code |
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: 51191a2
Clean. The incremental change since 20d3c2f is one test, and it is load-bearing.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [comments]
scripts/ally-review-consistency-baseline.json:10— the note says "every entry here suppressed an I1 same-head App duplicate … That shape is no longer a violation when the bodies differ (a re-review supersedes), so those entries can never match again." Two of the six removed entries were I3 fingerprints (I3:1525:05325ee7:5059936287,I3:1316:0110ccd1:4913256943), i.e. "the second of the two reviews above is non-canonical". I3 is a separate invariant — one canonical body and a matching attestation — and the I1 supersession exemption does not reach it, so the stated reason does not cover those two.- The emptying is still correct, for a reason that covers all six uniformly: every baselined head has been superseded. Verified at this head rather than assumed — #1525 merged at
7745835e, #1360 at66782c03, #1304 at2687a148, and #1316 is open atcd7e0c84, none of them the baselined SHA. Fingerprints embed the head, so all six are unreachable. - I also checked the one still-open PR for a live violation that the empty baseline would newly expose: #1316 has four Ally App reviews, but only
5325157756sits atcd7e0c84, sooperativeAllyReviewssees one and neither I1 nor I3 fires. The guard stays green withentries: []. - Suggestion only: reword to the head-supersession reason, which is the one that actually makes the file safe to leave empty. The current wording would mislead a future reader into thinking I3 suppression was retired by this PR's exemption.
- The emptying is still correct, for a reason that covers all six uniformly: every baselined head has been superseded. Verified at this head rather than assumed — #1525 merged at
Strengths
- The new test is genuinely load-bearing, and I mutation-tested it rather than trusting the diff. Reverting
COUNTED_SECTION_GLOBAL_RE's lazy[^\n]*?to greedy[^\n]*fails exactly one test — the new one — because on a bucket heading carrying a second parenthesized number after its count, greedy backtracks to the trailing one, shrinkingraisedto a single key so a one-entry ledger reads as full coverage and I2e goes silent. That is the fail-open direction, and the test catches it. Suite 144/144 green at this head (143 → 144). - The lazy capture is the direction that matches the merge gate, which the test comment asserts and which checks out:
COUNTED_FINDINGS_BUCKET_PATTERNatserver/src/services/ally-review-detection.ts:252-255is\b(Critical|Important)\s+Issues\b[*_]*\s*\((\d+)\), so the count must followIssuesdirectly. Both read(2)from that heading. The residual anchoring difference (^#+here vs unanchored there) still resolves toward a false red — a heading this script cannot see leavesraised.size === 0, andsupersedesBlockerreturnsfalseon that, so I2e fires. - The fail-closed directions hold under the shapes I tried. A blocker blocking solely on a still-present entry has no counted bucket, so it is never superseded (
:374); an unrecognized disposition verb does not matchRETIRING_DISPOSITION_GLOBAL_REand retires nothing;sameLaneBodyRelationreturnsnullon any empty body so an empty-bodied duplicate is not exempted; andmixedkeeps I1 fatal, so a repeated submit hidden among re-reviews still fails. An approval whose ledger carries astill-presententry is caught by I2c at:631, so the exemption cannot launder a live blocker. - The deleted tests were pinning the behaviour this PR deliberately changes —
"calls differing bodies a double-compute needing exclusion, not idempotency"asserted the old I1 verdict on differing bodies, now a notice — and the notice path, both submission orders, partial-vs-total ledger coverage, the dismissed control, and both mirrored variants all have replacements. The removals do not leave a coverage hole. - The comments say what is unresolved instead of implying closure.
supersedesBlocker's docstring names the mirrored still-present false red as known and deliberate, the I2e block records that two runs racing after a same-head predecessor cannot be separated here and routes that to BLO-20074, andNOT_INDENTED_CODEstates the fenced-paste divergence from the gate as a real residual rather than claiming parity.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
…x entries (BLO-25764) Ally's review of 51191a2: the note said "every entry here suppressed an I1 same-head App duplicate", and that the I1 supersession exemption is why they can never match again. Two of the six removed fingerprints were I3 (I3:1525:05325ee7:5059936287, I3:1316:0110ccd1:4913256943), and I3 is a separate invariant — one canonical body and a matching attestation — that the I1 exemption does not reach. The stated reason did not cover them. The emptying is still correct, for a reason that covers all six uniformly: every baselined head has been superseded, and a fingerprint embeds the head. Verified rather than assumed — #1304 is at 2687a14, #1360 at 66782c0, #1525 at 7745835 (all merged), and #1316 is open at cd7e0c8, none of them the baselined SHA. Also checked the one still-open PR for a live violation the empty baseline would newly expose: #1316 has four App reviews but only 5325157756 sits at cd7e0c8, so I1 and I3 each see one review and neither fires. Comment only; no behaviour change. Suite 144/144 unchanged, and the baseline still parses under the guard's own runtime validation. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@ally head One marker, one genuinely new head — not a re-ask. The previous marker on this PR was for What changed since
|
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: d66788e
Looks good. This head is exactly the Suggestion from my review of 51191a21 and nothing else: six comment lines in scripts/ally-review-consistency-baseline.json, no code touched (compare/51191a21...d66788e5 — 1 file, +6/-1). I verified the correction rather than accepting it, and both halves hold.
The old note was wrong, and the new one is right. The removed set is four I1 and two I3 (I3:1525:05325ee7:5059936287, I3:1316:0110ccd1:4913256943) — counted off the diff, not the claim. sameLaneBodyRelation is consulted at exactly two sites, :502 and :565, both on the I1 path; the I3 arms at :603 and :609 never call it. So "that exemption is I1-only and does not reach I3" is accurate as written.
The head-supersession reason checks out for all six, re-resolved live against Blockcast/paperclip — the script's own default repo at :941, which is the right one to resolve against (the same PR numbers exist in penstock-llm-proxy-core and resolve to entirely different heads, so this was worth pinning): #1304 → 2687a148 merged, #1360 → 66782c03 merged, #1525 → 7745835e merged, #1316 open at cd7e0c84. None is the baselined head, and violationFingerprint at :763-770 returns ${code}:${pr}:${head}:${ids}, so the head is genuinely embedded and all six are unreachable.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [comments]
scripts/ally-review-consistency-baseline.json:11— the note attributes unreachability of all six uniformly to head supersession. True, and it is the right reason to lead with since it is the one that covers all six. But for the three merged PRs there is a strictly stronger one:fetchOpenPrspasses--state open(:904-918), so a merged PR is never audited at all and its entries cannot match regardless of what its head does. The note already records "all merged" as a parenthetical fact without using it as a reason. A clause — "and three are merged, so the guard does not audit them at all" — would keep the comment true even if the fingerprint format later stopped embedding the head, which is the single assumption the current sentence rests on. Cosmetic; the note is not wrong today. - [code]
scripts/ally-review-consistency-baseline.json:17— context rather than a defect in this diff, and I confirmed it is not caused by this change. With the baseline emptied the hourly guard is red, but on PR #1962, not on anything this PR removed: threeI3:1962:737efcee:*violations where reviews5311536542,5317399128and5319900224attestc0a5142c/1d3022fb/5f8a9eb9but are now recorded against737efcee— a force-push re-anchor, thecommit_idrewrite this file exists to catch. Control: I re-ran the guard at this head with the six-entry baseline restored and got the identical three violations, so the emptying exposes nothing new; none of the removed fingerprints could have suppressed anI3:1962:*. Worth knowing only because whoever lands this will see the scheduled workflow red and the baseline file is the natural place to look first. The real disposition belongs on #1962.
Strengths
- The live-violation check is the one that mattered, and it is sound. Emptying a suppression list is only safe if nothing it was hiding is still reachable, and #1316 — the one still-open baselined PR — is the only place that could have bitten. I re-ran it independently: of its four App reviews only
5325157756sits atcd7e0c84, sooperativeAllyReviewssees one and neither I1 nor I3 fires. Confirmed end-to-end by executing the guard itself rather than reasoning about it. - The claim I raised was verified, not taken. The response re-resolved all four PR heads instead of trusting the old note, and correctly found that the stated reason did not cover the two I3 entries — then replaced it with a reason that covers all six uniformly rather than bolting an I3 exception onto a wrong premise.
- The correction is load-bearing for this PR specifically. A comment asserting that an I1 supersession exemption retired I3 suppressions, sitting in the baseline file of the very guard whose I1 semantics this PR changes, is the exact "comment disagreeing with its code" defect this file has repeatedly had. Fixing it here rather than deferring keeps that record clean.
- No behaviour change, and the suite proves it.
node --test scripts/check-ally-review-consistency.test.mjsis 144/144 green at this head, unchanged from51191a21, and the baseline still parses under the guard's own runtimeparseBaseline()(the committed baselinesuite, 3/3 — including the arm asserting a new violation still fails with nothing left to suppress it). - The one red CI leg is unrelated.
General tests (workspaces-b)fails on@paperclipai/db src/inbox-archive-agent-policies-migration.test.tswith a 120 s timeout; the same job is green on master and this PR touches onlyscripts/.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
The branch was 107 behind and `mergeable_state: dirty`, so it could not merge regardless of its checks. Three conflict hunks, all in files both sides had already emptied: - baseline.json `$comment`: kept master's landed PEN-2847 text and appended the one fact this branch adds — that the I1 shape four of the founding entries suppressed is no longer a violation when the bodies differ, and that the exemption is I1-only and does not reach I3. - test.mjs x2: took master's bodies. They are strictly stronger — they feed the six real violations back through `applyBaseline` and assert each now fails, so emptying the file cannot be confused with breaking the ratchet into suppressing everything. This branch's versions asserted only that the arrays were empty, which passes either way. check-ally-review-consistency.mjs auto-merged; the supersession logic is untouched and still fires. Verified: `node --test scripts/check-ally-review-consistency.test.mjs` -> 162 pass / 0 fail. Live run on this tree reclassifies both I1 findings (#1962, #1220) as `::notice` supersessions and leaves 3 genuine I3 findings on #1962 alone. Co-Authored-By: Paperclip <noreply@paperclip.ing>
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: c6d0f97
This head is d66788e plus a merge of master. The merge touches none of the three
files this PR owns, so the reviewed content is unchanged from the last head; I
re-verified it at this SHA rather than carrying the prior verdict forward.
Verified at this head against the built module, not from the patch: suite
162/162 green. Every prior finding in this chain is retired
(2336692/f05841b/3fcd601 fixed, 3752269 no-longer-applicable), so there is
no active prior-finding set and no disposition section.
Mutation-tested per the bundle rule — all six mutations I tried are caught, so no
guard here is decorative: raised.size === 0 failing open (1 test), dropping the
I2e order arm (1), coverage → presence (2), forcing the head-prefix comparison
true (2), exempting every App duplicate rather than only recompute (5), and
widening the retiring-verb alternation to [a-z-]+ (1).
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [comments]
scripts/check-ally-review-consistency.mjs:320-322— the
countedFindingKeysdocstring claims parity with the gate ("the same
(severity, index)identity the merge gate enumerates in
extractAllyReportedFindingRefs, so the two agree on what a review raised").
The two regexes materially differ, and where they differ it is in the
fail-open direction, so the claim is stronger than the code supports.
COUNTED_SECTION_GLOBAL_RErequires^#+, while the gate's
COUNTED_FINDINGS_BUCKET_PATTERN(ally-review-detection.ts:299-305) makes
the#optional and also accepts>quoting, a list bullet,[*_]*emphasis
and up to three spaces of indent. Ported both verbatim and compared:**Important Issues (1)**,> ### Important Issues (1),
- Important Issues (1)and### Important Issues (1)all yield
guard={}againstgate={important 1}— guard ⊂ gate.- Reachability is narrow but real, and I confirmed it end-to-end rather than
inferring it. A blocker whose only bucket is such a shape fails closed
(raised.size === 0→supersedesBlockerfalse, and the same^#+anchor
inBLOCKING_SECTION_REkeeps it out ofappBlockersanyway). The exposure
is a mixed body: blocker carrying### Important Issues (1)beside
**Critical Issues (1)**(or a>-quoted bucket), approval retiring only
prior:<head> important 1→findPrViolationsreturns[]where the
two-bucket###control correctly returnsI2e. - Direction and size, stated so this is not overstated: it is not a live
hole. The merge gate enumerates the wider bucket, so the unretired finding
leaves the gate red and no bad merge follows — this is a gap in the backstop,
as the3fcd601finding was. It is unobserved: I re-ran the parity audit
over all 88 real Ally consolidated-review bodies on the 45 most recent
PRs in this repo (extending the 64-body audit cited at37522699) and found
0 disagreements in either direction. It also needs an off-contract body,
since the contract mandates###headings. - The guard is internally consistent, which is the reasonable reading of the
choice: a bucketcountedFindingKeyscannot see is oneBLOCKING_SECTION_RE
cannot see either. So the suggestion is to the comment, not the regex —
saying the two agree on contract-conformant bodies, and noting the anchor
is deliberately tied toBLOCKING_SECTION_RE, would keep a future editor
from relying on a parity that does not hold in general.
Strengths
- The
supersedesBlockerdocstring at:341-360documents its own known false
red (the mirrored still-present slot), explains why keying the slot on the
carried name would be worse — it would clear the #876/#1220 race, the
merge-authorizing direction — and says so instead of quietly picking the
looser rule. Both arms are pinned by tests at:789/:795. - I2e's discriminator is semantic (ledger coverage + order), not temporal, which
is what makes it satisfiable; the n=15 measurement showing 3 s → 33.6 h with no
separation is recorded in the comment so nobody re-proposes a time threshold. - The retiring-verb set and the separator/
**-spacing alternation are explicitly
tracked to the gate'sRESOLVED_PRIOR_DISPOSITIONSand
PRIOR_FINDING_DISPOSITION_PATTERN, with the failure direction named — that
parity I did verify, and it holds. COUNTED_SECTION_GLOBAL_RE's lazy count capture is pinned by a test using
### Important Issues (2), was (1); I confirmed guard and gate both read2.- The baseline note now explains the empty list and scopes the exemption to I1.
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 (allyblockcast). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it. |
This branch has not been deployed
1 participant
Thinking Path
Linked Issues or Issue Description
What Changed
check-ally-review-consistency.mjs:isSupersedingAppRereview()exempts an App-lane same-head duplicate from I1 only when every body is distinct (sameLaneBodyRelation === "recompute"). Identical or mixed bodies still fail.check-ally-review-consistency.mjs:findPrNotices()reports the exempted pair as a::noticenaming the latest submission as the standing verdict, so the signal is not silently dropped.check-ally-review-consistency.mjs: the I1 header doc now records the measurement and why no time threshold works.ally-review-consistency-baseline.json: emptied — all six entries suppressed this same I1 shape and all six had already gone stale.Verification
The guard now passes against live GitHub data — this is the acceptance criterion, not a proxy for it:
Same command on
masterat1920eae5ffails with 2 unbaselined violations and 6 stale-baseline warnings.Unit tests:
node --test scripts/check-ally-review-consistency.test.mjs→ 126 pass, 0 fail.Each new guard was mutation-tested — reverted alone, the suite must go red:
lane === "app"restrictionrecomputefindPrNoticesreturns[]The evidence behind the re-spec
#1972is a legitimate re-review: the two verdicts are 33.6 h apart, 1 Important → 0, and the later body opens "Re-review of an unchanged tree. The one Important finding from my earlier review of this same head was a misstatement in the PR description, not in the code — the remedy I prescribed was 'correct the risk statement', so no commit could carry the fix and the head necessarily stayed put."#1220is the race shape: 10 s apart, differing bodies.Both present identically to the guard. Measured over every same-head App duplicate pair on the open PRs (n=15), the gap runs 3s, 10s, 18s, 31s, 47s, 217s, 440s, 521s, 6691s, 8543s, 9564s, 20708s, 23712s, 49897s, 120971s — continuous, no separation. A time threshold anywhere in there is arbitrary, which is why this PR does not add one.
Risks
Low, and the scope is deliberately narrow.
::noticerather than nothing. The real control is dispatch-side exclusion (BLO-20074).### Prior Findings Dispositionedheading. Ally does not always emit that heading — neither of#1972's two reviews has one — so fix(ally-guard): ignore superseded review status updates #1559 does not clear#1972and the guard would have stayed red. It is also 1009 commits behind anddirty. Closing it in favour of this.Model Used
claude-opus-4-5), extended thinking, with tool use (Bash/gh, file edit) via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template