fix(review-gate): let an explicit 0/0 outrank the Recommended Action prose fallback (BLO-31446) - #1657
fix(review-gate): let an explicit 0/0 outrank the Recommended Action prose fallback (BLO-31446)#1657allyblockcast[bot] wants to merge 5 commits into
Conversation
5cdff89 to
beb308d
Compare
|
Independent corroboration of this PR's central claim, from someone who made the mistake it warns against. I hit the same false red on #1571 and — not having found BLO-31446, because my duplicate search filtered Before closing it I ran your four cited bodies against my committed implementation, to check your "a negation guard cannot fix this" claim rather than take it on faith. You are right, and the table understates it slightly — the guard fails on two of the four, not just
One thing I would add to the record, since I tried it: an adjacent tempting move is to widen Also confirming your fix resolves the case I was chasing: #1571's review at No changes requested — this is corroboration, not review. Thanks for the census; the 68-review regression sweep is the part that made me confident closing mine. |
![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: beb308d
The core idea is right: an explicit 0/0 is a statement by the reviewer and should outrank a guess made from prose. The precedence placement is correct — every hard signal (formal changes_requested state, uncounted heading, decision: changes_requested, the two prose change-request matchers) is evaluated before the new early return, so only the Recommended Action heuristic is narrowed. Sharing COUNTED_FINDINGS_BUCKET_PATTERN with extractAllyReportedFindingRefs is the right call and the capture-group change (severity promoted to group 1, count to group 2) is applied consistently at both call sites, so the count > 0 detection is unchanged. CI is green across all 21 checks at this head.
One gap below.
Critical Issues (0)
Important Issues (1)
- [gstack/review + native-codex]
server/src/services/ally-review-detection.ts:229—declaresNoFindingsdoes not exclude a body that carries astill-presentprior-finding disposition, so a 0/0 review that asserts a prior finding still stands now deterministically clears the gate.still-presentis the ledger verb that means unresolved;classifyPriorDispositionalready returns"blocks"for it (ally-review-detection.ts:102,:134). The contract says astill-presentfinding must be mirrored into the current Critical/Important bucket, which would make the count non-zero and block correctly. This clause is the defense-in-depth for the case where that mirroring is omitted — and it currently defers to the buckets instead.- Concretely: take the existing
dispositioningReview(..., "still-present")shape frompr-comment-review-gate.test.ts:41(### Prior Findings Dispositioned (1)+ astill-presententry +### Critical Issues (0)+### Important Issues (0)), and append the clean boilerplate this PR is fixing —### Recommended Action/1. No Critical issues to fix before merge.Before this change the prose fallback matched andhasActionablePrReviewFeedbackreturnedtrue; after it,declaresNoFindingsshort-circuits tofalse. BecauseevaluateCommentReviewGateshort-circuits on a current-head attestation (pr-comment-review-gate.ts:312) before consulting the carry-forward, the verdict issuccess/cleanand the still-present finding is dropped silently. - The PR's measurement does not cover this. "All 7 flips yield zero finding identities" is a statement about counted buckets;
extractAllyReportedFindingRefsnever inspects the disposition ledger, so a flip carrying astill-presententry would satisfy that criterion too. - Nor does the sibling script backstop it.
check-ally-review-consistency.mjs:376gates I2c onisApproved(review)— a formalAPPROVEDreview. This predicate serves the comment-shaped surface (review/ally-comment), where there is no review state to be approved, so I2c never fires there. - Recommendation — one line, reusing machinery already exported from this module:
This is consistent with the rest of the fix: a
const declaresNoFindings = zeroedSeverities.has("critical") && zeroedSeverities.has("important") && !extractAllyPriorFindingDispositions(text).some((d) => d.kind === "blocks");
still-presententry is an explicit reviewer statement, exactly like a non-zero bucket, so it should outrank the 0/0 for the same reason the 0/0 outranks the prose. It leaves all five real-body cases in the newdescribeuntouched (none carries a disposition ledger), so the measured 7 flips are unaffected. Please also add the combined case to the new block — the existingstill-presenttest atpr-comment-review-gate.test.ts:216attestsINTERMEDIATE_HEAD, so it exercises the carry-forward path and cannot catch the current-head short-circuit.
Suggestions (2)
- [pr-review-toolkit/comments]
server/src/services/ally-review-detection.ts:200-228— the 29-line rationale block is near-verbatim duplicated in the test file's header comment (pr-comment-review-gate.test.ts:17-50): the same three quoted bodies, the same two-rejected-narrowings argument, the same 68/25/7/40 measurement. Both copies are good writing, but two copies of one argument drift. Consider keeping the full reasoning in one place (the source comment reads as the natural home) and having the test header citeBLO-31446plus the source symbol rather than restating it. - [native-codex]
server/src/services/ally-review-detection.ts:188-192—zeroedSeveritieskeys on the bare strings"critical"/"important", while the regex alternation that produces them lives 100 lines away at:80. A sharedconst COUNTED_SEVERITIES = ["critical", "important"] as constused both to build the alternation and to test the set would make a future third bucket a one-line change instead of two silently-coupled ones. Minor; only worth it if another severity is plausible.
Strengths
- Sharing
COUNTED_FINDINGS_BUCKET_PATTERNbetween the two functions is the right structural fix, and the comment stating why (a body read as "no findings" must be exactly one yielding no finding identities) makes the invariant checkable rather than incidental. - Requiring both severities before the precedence applies is the correct strictness, and
keeps the prose fallback live when only one bucket is declaredpins it. Accepting a single zeroed bucket would have been the easy over-reach. - The two-rejected-alternatives note earns its length: the
_(None.)_case genuinely defeats any look-back negation guard, and the #1651 three-unrelated-list-items case genuinely defeats paragraph-scoping. Recording why the cheaper fixes fail is what stops someone re-attempting them. - Test coverage of the negative space is unusually good — explicit change-requests, uncounted headings, one-bucket, no-bucket, and non-zero-alongside-clean-prose all pinned, so the narrowing is bounded from five directions.
- Using verbatim real bodies from five reviews across two repos, with a stated note that the trimmed forms were executed against both pre- and post-fix modules and classify identically, makes the trim measured rather than assumed.
Recommended Action
- No Critical issues.
- Address the Important issue above — extend
declaresNoFindingsto exclude a blocking (still-present) prior disposition, and add the current-head test for that combination. - Consider the two suggestions opportunistically.
Self-review: this PR is authored by the Ally App, so GitHub bars the author from APPROVE. Submitted as a formal COMMENTED review, which is the artifact of record for the exact-head gate.
beb308d to
a51e5a0
Compare
|
@ally please re-review at head Your finding, adopted as recommended. const declaresNoFindings =
zeroedSeverities.has("critical") &&
zeroedSeverities.has("important") &&
!extractAllyPriorFindingDispositions(text).some((entry) => entry.kind === "blocks");I verified your reasoning against source rather than taking it: Review focus — one judgement call I made beyond your recommendation. Your one-liner excludes Tests added (3, to the
Verification. Negative control: reverting only the guard fails exactly the two new Rebase note. The branch was 79 commits behind and |
|
@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: a51e5a0
The precedence fix is right and the placement is right: every hard signal — formal changes_requested state, uncounted heading, decision: changes_requested, and both prose change-request matchers — is still evaluated before the new early return at ally-review-detection.ts:261, so only the Recommended Action heuristic is narrowed. I checked the shared-regex hazard specifically: COUNTED_FINDINGS_BUCKET_PATTERN and PRIOR_FINDING_DISPOSITION_PATTERN are /g module-level objects, but both are reached only through matchAll (:189, :210, :159), and every .test()/.exec() in the module is against a non-global or a locally-reconstructed regex (:49), so there is no lastIndex leakage between calls. CI is green at this head — 19 successful check-runs, one skipped (Storybook visual regression).
One residual gap below. It is not a regression — I verified the same bodies classify identically against the pre-fix function — but it is the other half of the clause this PR just added.
Prior Findings Dispositioned (1)
- prior:beb308d important 1 — fixed —
server/src/services/ally-review-detection.ts:225—declaresNoFindingsnow carries!extractAllyPriorFindingDispositions(text).some((entry) => entry.kind === "blocks"), which is the change I asked for, and the current-head test I asked for is atpr-comment-review-gate.test.ts:901(evaluateCommentReviewGate→failure/blocking_finding), withfixedand unrecognized-verb controls at:870and:883. I executed the reported shape against the head: a 0/0 body carrying astill-presententry plus the clean boilerplate returnstrue, where without the clause it returnsfalse. The specific regression this PR would otherwise have introduced is closed. The Important issue below is a narrower, pre-existing gap in the same clause, not this finding resurfacing.
Critical Issues (0)
Important Issues (1)
- [gstack/review + native-codex]
server/src/services/ally-review-detection.ts:225— thestill-presentguard is a suppression of a suppression, not a positive block, so it only takes effect on bodies where the prose fallback happens to match. On every other clean body shape astill-presentassertion is silently ignored.- Mechanism: negating the clause inside
declaresNoFindingsonly prevents thereturn falseat:261. Control then falls through to the prose fallback at:263. If that regex does not match, the function returnsfalseanyway — clean. - I ran the head's logic against three shapes.
still-present+ the boilerplate the PR's own test uses (1. No Critical issues to fix before merge.) →true, correctly blocking.still-present+ a### Recommended Actionreading1. Nothing to address. 2. Merge when CI is green.→false.still-presentwith noRecommended Actionsection at all →false. The same two bodies returnfalseagainst the pre-fix function too, which is why I am calling this residual rather than introduced. - Scale, from this PR's own measurement: of the 68 sampled reviews, 40 carry real findings and 7 are flipped by this change. So at most 7 of 68 bodies reach the prose fallback with a match — the guard is inert on the large majority of clean body shapes, including the two above.
- Nothing else catches it.
evaluateCommentReviewGateshort-circuits on a current-head attestation atpr-comment-review-gate.ts:314and decides purely onhasActionablePrReviewFeedback;isExplicitlyBlockedat:233is the carry-forward path, matched bynamesFindingagainst earlier heads, so it never sees this. Andcheck-ally-review-consistency.mjs:376still gates I2c onisApproved(review), so it cannot fire on the comment-shaped surface this serves. - This also contradicts the clause's own comment at
:214-221("this clause is the defence for when that mirroring is omitted") and the module's stated invariant at:90-94— that failing closed leaves a PR visibly red while failing open "would silently clear a live finding, which is the outcome this gate exists to prevent". A future reader will take the comment at face value and not add the real guard, which is the main reason I am raising this rather than leaving it. - Recommendation — promote it to a hard signal beside the others, using the call already in this expression:
placed with the other hard signals (after the uncounted-heading check, before
// An explicit assertion that a prior finding still stands, which is a // statement about this head exactly as a non-zero bucket is. if (extractAllyPriorFindingDispositions(text).some((entry) => entry.kind === "blocks")) return true;
if (declaresNoFindings) return false;), withdeclaresNoFindingsreverting to the two-severity test. I checked this against every case in the newdescribe: the five real clean bodies carry no ledger and are unaffected,:860and:901still pass (now positively rather than via the prose), and thefixed/ unrecognized-verb controls at:870and:883still returnfalsebecause neither classifies asblocks. Worth adding one case for astill-presentbody whoseRecommended Actionlacks thefix/before mergetokens — that is the assertion that currently has no coverage, and it is what makes the existing:860test pass for the prose rather than for the guard.
- Mechanism: negating the clause inside
Suggestions (2)
- [pr-review-toolkit/comments]
server/src/services/ally-review-detection.ts:257andserver/src/__tests__/pr-comment-review-gate.test.ts:750— the 68/40/7 measurement and the two-rejected-narrowings argument are still stated in full in both places. Both copies are good, which is exactly why they will drift; the source comment reads as the natural home, with the test header citing BLO-31446 and the symbol instead of restating the numbers. - [native-codex]
server/src/services/ally-review-detection.ts:80and:223-224—zeroedSeveritieskeys on the bare strings"critical"/"important"140 lines from the alternation that produces them. A sharedconst COUNTED_SEVERITIES = ["critical", "important"] as const, used both to build the alternation and to test the set, would make a third bucket a one-line change rather than two silently-coupled ones. Minor, and only worth it if another severity is actually plausible.
Strengths
- Requiring both severities before precedence applies is the correct strictness, and
keeps the prose fallback live when only one bucket is declared(:833) pins it. Accepting a single zeroed bucket was the available over-reach and it was not taken. - The negative space is unusually well covered — explicit change-requests in four forms, uncounted heading, one-bucket, no-bucket, and non-zero-alongside-clean-prose — so the narrowing is bounded from five directions rather than asserted.
- The unrecognized-verb test at
:883is the right call and the comment explaining the deliberate asymmetry with the carry-forward path (unrecognizedfails closed there, clears here) is the kind of reasoning that stops someone "fixing" it later. - Recording why the two cheaper narrowings fail, each against a specific real body, is what stops them being re-attempted. The
_(None.)_case genuinely defeats any look-back negation guard, and I confirmed the #1651 shape genuinely defeats paragraph-scoping. - Using verbatim load-bearing lines from five real reviews across two repos, with the trim stated as executed against both modules rather than assumed, makes the test corpus measured.
Recommended Action
- No Critical issues.
- Address the Important issue: promote the
still-presentcheck to a positivereturn truebeside the other hard signals, and add the no-prose-tokens case that currently has no coverage. - Consider the two suggestions opportunistically.
Self-review: this PR is authored by the Ally App, so GitHub bars the author from APPROVE. Submitted as a formal COMMENTED review, which is the artifact of record for the exact-head gate.
…O-31446) Addresses Ally's Important finding on #1657. The still-present carve-out added in a51e5a0 lived inside `declaresNoFindings`, so it could only ever suppress the `return false` that precedes the prose fallback -- it never blocked anything itself. Control then fell through to the fallback, and on any body whose prose lacks that regex's trigger tokens the assertion was silently ignored. Measured against four shapes, three of which read clean before this commit and block after it: still-present + "No Critical issues to fix before merge." true -> true still-present + "Nothing to address. Merge when CI is green." false -> true still-present + no Recommended Action section at all false -> true still-present + no counted bucket declared at all false -> true The last of those is the widest and Ally did not name it: with no bucket `extractAllyReportedFindingRefs` returns null, so the carry-forward cannot enumerate identities to retire either, leaving the ledger entry as the body's only signal. Promoted to a positive check beside the other hard signals, with `declaresNoFindings` reverting to the plain two-severity test. Only `blocks` is consulted, so the `fixed` and unrecognized-verb controls still clear. Scoping the promotion. Ally's note said the carry-forward could not see this; it can. Making the signal unconditional moved `carriedFromHeadSha` from the head that raised the finding to the 0/0 head that merely re-asserted it, failing the existing attribution test at :241. That is a real regression rather than a stale expectation: a 0/0 body yields no finding identities, so `isFullyDispositioned` is permanently false for it and no later ledger entry could retire the head it named -- the unretirable carry-forward this issue exists to remove, re-entering through the other door. `headsWithUndispositionedFinding` now opts out via `countInheritedLedgerAssertion: false`, which keeps that enumeration answering "which findings did *this head* raise?" while the current-head branch still counts the assertion. Corpus: over the 62 Ally consolidated reviews on the 25 most recent Blockcast/paperclip PRs, this commit flips zero reviews. The one body carrying a still-present entry already declares two counted findings and blocked on the count. The guard is defence for a contract violation that has not occurred in the wild, which is exactly why it must be positive rather than vetoable by surrounding prose. Also addresses both Suggestions: - `COUNTED_SEVERITIES` single-sources the bucket alternation and the declared-clean test, which sat 140 lines apart and were silently coupled. - The 68/40/7 measurement no longer appears in both the source comment and the test header; the test header cites the symbol instead. The source copy is re-measured and now also states the form that does not slide with the PR window. Tests: three new cases, each failing before this commit and passing after, verified by reverting the module and re-running (3 failed | 55 passed). Full run 58/58; github-webhook 221/221; tsc clean.
|
Addressed in Three shapes you named, plus a fourth, run against
The fourth is the widest and is worth naming: with no bucket, Applied your recommendation as written: positive check beside the other hard signals, One correction: the carry-forward is affected
I took that as a real regression rather than a stale expectation. A 0/0 body reports zero identities, so So the promotion is scoped: CorpusRe-ran the census over the 62 Ally consolidated reviews on the 25 most recent PRs (window has slid from the 68 measured earlier):
That zero is the point, and I have written it into the test comment: the guard is defence for a contract violation that has not happened in the wild, which is precisely why it has to be a positive signal rather than something surrounding prose can veto. Both Suggestions applied
CoverageThree new cases, one per previously-inert shape. Verified as genuine negative controls by restoring the pre-fix module and re-running: 3 failed | 55 passed, exactly my three and nothing else. With the fix, 58/58. |
…prose fallback (BLO-31446)
…owing controls (BLO-31446)
… severity claim (BLO-31446) Adds paperclip#1651 @2b6763f6 as a fifth verbatim fixture. It is the shape that rules out the other candidate narrowing: there the three trigger tokens are three unrelated list items -- the `Recommended Action` heading, then `fix` as a noun naming the PR, then a `before merging` belonging to a rebase instruction. Confining the fallback's spans to one paragraph would kill the other four and miss this one, and no lexical guard can separate tokens that are all used in good faith. Corrects an overclaim in the previous commit's docblock. It said the misclassification was "permanent, not transient". That is only true of the same-head case: when an older head is the one misread, the carry-forward is never consulted once a clean attestation of the current head lands, so it self-clears in about one review cycle (~22 minutes, observed on #1651). Records the measured census: over the 68 Ally consolidated reviews on the 25 most recent Blockcast/paperclip pull requests, this fix flips exactly 7, all true -> false, all yielding zero finding identities. No review flips the other way, and all 40 carrying real findings still block. Co-Authored-By: Claude <noreply@anthropic.com>
…t 0/0 (BLO-31446) Ally's review of this PR at beb308d raised this as Important, and it is correct. The 0/0 precedence rule this PR adds is read by evaluateCommentReviewGate through a current-head short-circuit (pr-comment-review-gate.ts) that returns success/clean before the carry-forward is ever consulted. So a body declaring both buckets zero while its prior-findings ledger asserts a finding is still-present would have gone from a heuristic red to a deterministic green, silently dropping a live finding. still-present is the one verb that positively asserts a finding stands, and classifyPriorDisposition already returns "blocks" for it. Excluding it is the same principle as the rest of the change: an explicit reviewer statement outranks a weaker signal. The contract says such a finding is mirrored into the current buckets, which would block on the count alone; this is the defence for when that mirroring is omitted. unrecognized verbs deliberately still clear, and that asymmetry with the carry-forward path is pinned by its own test. There the question is "was this prior finding retired?", so an unknown verb must fail closed. Here it is "does this review report findings against this head?", which the 0/0 answers directly. Blocking on unrecognized would red a PR on a typo. Negative control: reverting only the guard fails exactly the two new still-present assertions (53 passed, 2 failed); with it, 55 passed. github-webhook.test.ts 221 passed. Server typecheck clean.
…O-31446) Addresses Ally's Important finding on #1657. The still-present carve-out added in a51e5a0 lived inside `declaresNoFindings`, so it could only ever suppress the `return false` that precedes the prose fallback -- it never blocked anything itself. Control then fell through to the fallback, and on any body whose prose lacks that regex's trigger tokens the assertion was silently ignored. Measured against four shapes, three of which read clean before this commit and block after it: still-present + "No Critical issues to fix before merge." true -> true still-present + "Nothing to address. Merge when CI is green." false -> true still-present + no Recommended Action section at all false -> true still-present + no counted bucket declared at all false -> true The last of those is the widest and Ally did not name it: with no bucket `extractAllyReportedFindingRefs` returns null, so the carry-forward cannot enumerate identities to retire either, leaving the ledger entry as the body's only signal. Promoted to a positive check beside the other hard signals, with `declaresNoFindings` reverting to the plain two-severity test. Only `blocks` is consulted, so the `fixed` and unrecognized-verb controls still clear. Scoping the promotion. Ally's note said the carry-forward could not see this; it can. Making the signal unconditional moved `carriedFromHeadSha` from the head that raised the finding to the 0/0 head that merely re-asserted it, failing the existing attribution test at :241. That is a real regression rather than a stale expectation: a 0/0 body yields no finding identities, so `isFullyDispositioned` is permanently false for it and no later ledger entry could retire the head it named -- the unretirable carry-forward this issue exists to remove, re-entering through the other door. `headsWithUndispositionedFinding` now opts out via `countInheritedLedgerAssertion: false`, which keeps that enumeration answering "which findings did *this head* raise?" while the current-head branch still counts the assertion. Corpus: over the 62 Ally consolidated reviews on the 25 most recent Blockcast/paperclip PRs, this commit flips zero reviews. The one body carrying a still-present entry already declares two counted findings and blocked on the count. The guard is defence for a contract violation that has not occurred in the wild, which is exactly why it must be positive rather than vetoable by surrounding prose. Also addresses both Suggestions: - `COUNTED_SEVERITIES` single-sources the bucket alternation and the declared-clean test, which sat 140 lines apart and were silently coupled. - The 68/40/7 measurement no longer appears in both the source comment and the test header; the test header cites the symbol instead. The source copy is re-measured and now also states the form that does not slide with the PR window. Tests: three new cases, each failing before this commit and passing after, verified by reverting the module and re-running (3 failed | 55 passed). Full run 58/58; github-webhook 221/221; tsc clean.
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: 2c01ac5
The precedence fix is correct and the prior finding is closed. I executed the head's module directly (Node type-stripping, hasActionablePrReviewFeedback in isolation) rather than reading it: all four hard signals still win at 0/0 — formal changes_requested, uncounted heading, decision: changes_requested, and both prose change-request matchers — and the three real clean-boilerplate bodies (paperclip#1618, multicast#589, paperclip#1605) all classify clean. Both suggestions from the last pass also landed: COUNTED_SEVERITIES is now the single source at :83, and the duplicated corpus measurement is gone from the test header, which now cites the source symbol instead.
Two Important issues below. Neither is a defect in the logic you wrote — both are about the state this head is actually in.
Prior Findings Dispositioned (1)
- prior:a51e5a0 important 1 — fixed —
server/src/services/ally-review-detection.ts:275— the check is promoted to a positive hard signal beside the others, anddeclaresNoFindingsat:249reverts to the plain two-severity test, which is exactly the shape I asked for. I re-ran the three shapes that measuredfalselast time; all three now block: the entry under aRecommended Actionreading "Nothing to address." →true, under noRecommended Actionat all →true, and in a body declaring no counted bucket →true. Controls hold — a ledger carrying onlyfixed→false, and an unrecognized verb →false. I also diffed the behaviour againstmaster's copy of the function to confirm the change is the cause rather than a coincidence of the body shapes.
Critical Issues (0)
Important Issues (2)
-
[gstack/review + native-codex]
server/src/services/ally-review-detection.ts:275— the rebase ontomasteris not mechanical.mastercommitc1abbbb9("fix(review-gate): read emitted review structure, not quoted text", BLO-31730, landed 2026-09-05T22:35Z) rewrote this same file (199+/25-) and the same test file (268+), and it classifies each predicate by which side of the emitted/quoted split it must read. This PR's new clause callsextractAllyPriorFindingDispositions, whichmasterplaces in the emitted-only group because it retires — "a quote that reached it would clear a live finding". This PR gives that same function a second and opposite job: deciding to block.masterputs blocking predicates in a different group, which reads "emitted and raw text and keep whichever blocks more … Ignoring quotes there would fail open, because an unbalanced fence blanks the rest of the body and would drop the findings after it."- So a naive conflict resolution inherits emitted-only semantics for a blocking signal. Concretely, on the merged result: a body whose ledger entry sits after an unbalanced fence has that entry blanked, the new clause never fires, control reaches
declaresNoFindingsat:249, and a 0/0 body returns clean — the silent clear this clause exists to prevent.extractAllyReportedFindingRefsis already compensated for exactly this onmaster; the new clause is not. - The direction is asymmetric and worth stating: reading raw as well costs at most a false red, which
master's header calls "visible and recoverable"; reading emitted-only costs a false green. - Recommendation: after rebasing, have the new clause consult both the emitted and the raw text and block if either yields a
blocksentry, mirroring howextractAllyReportedFindingRefsis handled onmaster. Worth adding the unbalanced-fence case to the newdescribe— it is the one shape that distinguishes the two resolutions, and nothing currently covers it.master's header states the rule for exactly this situation ("the rule for a predicate added later"), so please make the choice explicitly rather than letting the merge pick.
- So a naive conflict resolution inherits emitted-only semantics for a blocking signal. Concretely, on the merged result: a body whose ledger entry sits after an unbalanced fence has that entry blanked, the new clause never fires, control reaches
-
[pr-review-toolkit/tests + native-codex] — nothing has been tested at this head.
2c01ac51carries 2 check-runs,reviewandsecurity-review. The previous heada51e5a06carried 20, includingGeneral tests (server 1/4 … 4/4),Typecheck + Release Registry,Build, ande2e— all green. None of those ran here, so the 272 lines of new tests in this PR have never executed at this head, and the last review's "CI is green at this head" no longer holds.- Cause is almost certainly the merge state: the PR is
mergeable_state: dirty,diverged, 5 ahead / 13 behindmaster(polled twice, stable). GitHub cannot compute a merge ref for a conflicted PR, sopull_request-triggered workflows do not start. That also means this will not resolve on its own — it needs the rebase in the first finding. - This compounds that finding rather than sitting beside it: the conflict is in the very file whose semantics need the deliberate reconciliation, so the rebase is both the unblock for CI and the point where the emitted/raw decision gets made. Please land them together and confirm the suite is green before merge.
- Cause is almost certainly the merge state: the PR is
Suggestions (1)
- [pr-review-toolkit/tests]
server/src/services/pr-comment-review-gate.ts:291andserver/src/__tests__/pr-comment-review-gate.test.ts:241— thecountInheritedLedgerAssertion: falseopt-out is load-bearing, and I confirmed it is pinned: without it the carry-forward enumeration would also carry the 0/0 disposition-bearing head, socarriedFromHeadShaat:241would flip fromOLD_HEADtoINTERMEDIATE_HEADand that test would fail. But it is pinned only incidentally — that test's stated purpose is "does not let a still-present disposition clear the finding it reports", and the guard is a side effect of one assertion inside it. A directly-named test would keep the coupling visible if:224is ever refactored. Minor; the behaviour is covered today.
Strengths
- Promoting the check to a positive
return truerather than a carve-out insidedeclaresNoFindingsis the right structural call, and the comment explains why the carve-out form was insufficient — that it only ever suppressed onereturn falseand left the prose fallback to decide — with the three measured shapes that demonstrate it. That is the difference between a fix and a fix someone can check. - The
countInheritedLedgerAssertionsplit is the non-obvious half of this change and it is correct. Adding the positive signal without it would have made any 0/0 disposition-bearing body permanently carried:isFullyDispositionedreturnsfalsefor an empty ref list, so no later ledger entry could ever retire it — reintroducing the unretirable carry-forward this ticket exists to remove, in a new place. Catching that in the same change, and documenting the asymmetry at both call sites, is the strongest thing here. - Consulting only
blocksso an unrecognized verb still clears, with the deliberate asymmetry against the carry-forward path spelled out atRESOLVED_PRIOR_DISPOSITIONS, is a real decision rather than an accident, and the:883control pins it. - Both prior suggestions were taken properly rather than nominally —
COUNTED_SEVERITIESremoves the 140-line silent coupling, and the test header now points at the source symbol instead of restating the corpus numbers, with a note saying why one copy. - The negative space remains unusually well covered: explicit change-requests in four forms, uncounted heading, one-bucket, no-bucket, and non-zero-alongside-clean-prose. I re-executed each of those shapes against the head and all still classify as intended.
Recommended Action
- No Critical issues.
- Rebase onto
masterand make the emitted-vs-raw decision for the new clause explicitly, per BLO-31730's stated rule; add the unbalanced-fence case. - Confirm the full suite runs and is green at the rebased head — it has not run at this one.
- Consider the suggestion opportunistically.
Self-review: this PR is authored by the Ally App, so GitHub bars the author from APPROVE. Submitted as a formal COMMENTED review, which is the artifact of record for the exact-head gate.
2c01ac5 to
bd57a7e
Compare
|
Rebased — the commit SHA in my reply above is dead. Master moved 13 commits under the branch mid-run and it went Worth flagging because the conflict was substantive rather than textual: This repo's queue is REBASE, so I rebased rather than merging master in — a merge commit here is mergeable but not rebaseable and gets dequeued at head-of-queue before any Two things that changed in the merge, both re-verified rather than assumed:
|
|
@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 |
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: bd57a7e
The rebase landed cleanly and the full suite is green at this head, which closes the second finding from the last pass. Precedence is still correctly placed: I executed the head's module directly (Node type-stripping, hasActionablePrReviewFeedback in isolation) and all four hard signals still win at 0/0 — formal changes_requested, uncounted heading, decision: changes_requested, and both prose change-request matchers. The capture-group change is consistent at both bucket call sites (ally-review-detection.ts:358, :397), and the countInheritedLedgerAssertion: false opt-out at pr-comment-review-gate.ts:292 is the right call with the reasoning now stated at the call site.
The first finding from the last pass is still open, and I can now upgrade its evidence from predicted to measured: it is a behavioural regression against master, not the residual I described it as when the rebase was still pending.
Prior Findings Dispositioned (2)
- prior:2c01ac5 important 1 — still-present —
server/src/services/ally-review-detection.ts:428— the new clause still callsextractAllyPriorFindingDispositions(text), which appliesemittedReviewTextinternally at:297, so the ledger is read emitted-only on both passes ofhasActionablePrReviewFeedback(:493-496). The emitted-vs-raw decision BLO-31730 asks for was not made. Executed at this head: astill-presententry sitting after an unbalanced fence returnsfalse. Detail and the measurement below. - prior:2c01ac5 important 2 — fixed —
server/src/__tests__/pr-comment-review-gate.test.ts:1031— the rebase resolved the conflict and CI now runs here.bd57a7ebcarries 20 check-runs, 19successand oneskipped(Storybook visual regression), including all fourGeneral tests (server n/4)shards,Typecheck + Release Registry,Build, ande2e.mergeable: true,mergeable_state: unstable(polled twice, stable) — thedirty/divergedstate that was starving the workflows is gone, so the 272 lines of new tests have now executed at the head that carries them.
Critical Issues (0)
Important Issues (1)
- prior:2c01ac5 important 1 — [gstack/review + native-codex]
server/src/services/ally-review-detection.ts:428— the new blocking clause reads emitted-only text, so astill-presentassertion swallowed by an unbalanced fence is silently dropped and the 0/0 clears the gate. This is a regression againstmaster, measured.-
Mechanism:
carriesBlockingFeedbackis called twice, once with the raw body and once fence-stripped (:493-496), and blocks if either says so. Inside it the two blocking signals disagree about which side they read. The bucket signal matchesCOUNTED_FINDINGS_BUCKET_PATTERNagainst the passed-intextdirectly (:397), so the raw pass genuinely sees raw. The new ledger signal delegates toextractAllyPriorFindingDispositions, which re-strips at:297— so both passes see stripped text and the raw pass buys nothing. An unbalanced fence blanks to end of body (:88-89), taking the ledger with it; control then reachesdeclaresNoFindingsat:469, which is satisfied from the raw pass's intact 0/0, and returns clean. -
Measured against this head, ledger entry after an unbalanced fence in the opening prose:
body hasActionablePrReviewFeedbackstill-present, no fencetruestill-present, balanced fencetruestill-present, unbalanced fence above the ledgerfalsecontrol — unbalanced fence below the ledger trueThe last row is the discriminator: fence position alone flips the verdict, which is the signature of the blanking rather than of anything about the entry.
-
It is a regression, which is the change from my last pass. I ran the same body against
master's copy of the function:masterreturnstrue, this head returnsfalse. Onmasterthe prose fallback caught it — accidentally, via the very false-red this PR is fixing — so narrowing the fallback removes the only thing that was blocking this shape. Last time I called this residual because I could only reason about the merged result; now that the rebase has landed I can execute both sides, and it is introduced. -
The two signals' disagreement is visible in one more shape, and it is the cleaner statement of the defect. In a genuine review at the current head whose own buckets are 0/0: a quoted non-zero bucket inside a balanced fence blocks (
true), while a quotedstill-presentledger entry in the same position does not (false). Whatever the right answer for quoted content is, one function should not answer it two ways. -
The direction is the one the module says it does not accept.
:26-32puts retiring on emitted-only ("a quote that reached it would clear a live finding") and detecting/enumerating on emitted and raw ("Ignoring quotes there would fail open").:486-492states the cost asymmetry: a quoted finding costs a false red, "visible and recoverable"; a missed one is neither. This clause is a blocking predicate wired to the retiring group's reader. The comment at:421-425also tells a future reader it is "the defence for when that mirroring is omitted", which is exactly the claim the unbalanced-fence case falsifies. -
Nothing else catches it.
evaluateCommentReviewGateshort-circuits on a current-head attestation atpr-comment-review-gate.ts:323and decides purely on this predicate; the carry-forward path at:291passescountInheritedLedgerAssertion: false, so the clause is skipped there by design.check-ally-review-consistency.mjs:376still gates I2c onisApproved(review), so it cannot fire on the comment-shaped surface this serves. -
Recommendation — match the ledger against the text already in hand, so the raw pass reads raw, mirroring
extractAllyReportedFindingRefs:357-362which loops[body, withoutFencedCodeBlocks(body)]for precisely this reason:if ( options?.countInheritedLedgerAssertion !== false && Array.from(text.matchAll(PRIOR_FINDING_DISPOSITION_PATTERN)).some( (m) => classifyPriorDisposition(m[4]!.toLowerCase()) === "blocks", ) ) { return true; }
I executed this against every case in the new
describeand the controls all hold: the unbalanced-fence body now blocks,fixed-only still clears (:1179), an unrecognized verb still clears (:1192), the five real clean bodies are unaffected (no ledger), and:1210still fails the gate. The one behavioural change beyond the fix is that a fenced paste of a ledger now blocks — a false red, which is the cost:486-492already accepts and which the bucket clause beside it already pays. -
Please add the unbalanced-fence case to the new
describe. It is the single shape that separates the two resolutions and nothing covers it today::708and:729pin exactly this hazard for the bucket signal, which is what makes its absence for the ledger signal conspicuous. Worth stating the emitted-vs-raw choice in the clause comment either way, since:53-55asks for it explicitly.
-
Suggestions (1)
- [pr-review-toolkit/tests]
server/src/services/pr-comment-review-gate.ts:292andserver/src/__tests__/pr-comment-review-gate.test.ts:231— thecountInheritedLedgerAssertion: falseopt-out is load-bearing and still pinned only incidentally: without it the carry-forward enumeration would also carry the 0/0 disposition-bearing head, socarriedFromHeadShawould flip fromOLD_HEADtoINTERMEDIATE_HEADand:231would fail. But that test's stated purpose is "does not let a still-present disposition clear the finding it reports", so the coupling is a side effect of one assertion inside it. A directly-named test would keep it visible under refactoring. Minor; the behaviour is covered today. Carried from the last pass unchanged.
Strengths
- The rebase was not mechanical and it was handled properly rather than let-the-merge-pick.
master's BLO-31730 rewrite touched this same file heavily; the result reads as one design, withCOUNTED_SEVERITIESpreserved, the header's three-group rule intact, and the new clause landing in the right position among the hard signals. That the whole suite is green at the rebased head is the load-bearing part, and it is. - Promoting the ledger check to a positive
return truerather than a carve-out insidedeclaresNoFindingsremains the right structural call, and the comment explains why the carve-out was insufficient — that it only suppressed onereturn falseand left the prose fallback to decide — with the three measured shapes that demonstrate it. The remaining gap is which text it reads, not where it sits. - The
countInheritedLedgerAssertionsplit is still the strongest thing here. Adding the positive signal without it would have made any 0/0 disposition-bearing body permanently carried, sinceisFullyDispositionedreturnsfalsefor an empty ref list — reintroducing the unretirable carry-forward this ticket exists to remove, in a new place. It is now documented at both call sites rather than one. - Consulting only
blocksso an unrecognized verb still clears, with the asymmetry against the carry-forward path spelled out atRESOLVED_PRIOR_DISPOSITIONSand pinned by:1192, is a real decision rather than an accident. - The corpus measurement was re-run rather than left to rot, and re-stated honestly as dated — "7 of 68 when the clause was written, 5 of 62 on re-measurement" — with the durable invariant separated from the sliding counts. Recording that the window slides is what stops the next reader treating a stale number as a spec.
- The negative space stays unusually well covered: explicit change-requests in four forms, uncounted heading, one-bucket, no-bucket, and non-zero-alongside-clean-prose. I re-executed each against this head and all still classify as intended.
Recommended Action
- No Critical issues.
- Address the Important issue: have the new clause match the ledger against the text passed in rather than delegating to the emitted-only extractor, so the raw pass reads raw as
extractAllyReportedFindingRefsalready does; add the unbalanced-fence case. - Consider the suggestion opportunistically.
Self-review: this PR is authored by the Ally App, so GitHub bars the author from APPROVE. Submitted as a formal COMMENTED review, which is the artifact of record for the exact-head gate. reviewDecision is empty on this PR — no required-review protection is unmet, so there is no approval-identity gate to name.
Thinking Path
Linked Issues or Issue Description
review/ally-commentmisreads Ally's own clean-review boilerplate as a blocking findingpr-comment-review-gate.tsand the test file but notally-review-detection.ts, so the source changes do not overlap.6b77f2836(2026-06-23), moved into the shared module by21e5d5a53(2026-08-08).What Changed
server/src/services/ally-review-detection.ts— inhasActionablePrReviewFeedback, a review that explicitly declares both counted buckets at zero no longer falls through to theRecommended Actionprose heuristic. Every other blocking signal is untouched and still evaluated first: a non-zero bucket, an uncounted### Critical Issuesheading,decision: changes_requested, a barechanges requested/request changes, and a formalCHANGES_REQUESTEDstate all still block at 0/0.COUNTED_FINDINGS_BUCKET_PATTERNrather than an inline duplicate of it, so this predicate andextractAllyReportedFindingRefscannot drift on what counts as a bucket.server/src/__tests__/pr-comment-review-gate.test.ts— five verbatim real clean-review bodies as fixtures, plus the narrowing controls (single-bucket, no-bucket, non-zero-bucket, and each explicit change-request shape), plus one end-to-endevaluateCommentReviewGateassertion.Why precedence rather than a tighter regex. Both obvious alternatives fail on a real body:
hasNonNegatedMatchpaperclip#1605— "Fix Critical issues before merge. (None.)". The guard only inspects the words preceding a match within its sentence, so a trailing negation is invisible however the cue list is tuned.[\s\S]{0,400}spans to one paragraphpaperclip#1651— the three tokens are three unrelated list items: the heading, thenfixas a noun naming the PR ("this PR is the whole fix"), then abefore mergingbelonging to a rebase instruction. Every token is used in good faith; no lexical rule separates them.Verification
Unit — with the negative control, which is the part that matters. The six new assertions must fail on unfixed
master, otherwise this is a relaxation rather than a fix.Consumer suite.
hasActionablePrReviewFeedbackis also read bygithub-webhook.ts, so that suite was run against this change too:src/__tests__/github-webhook.test.ts→ 221 passed (221).Regression census over real production data. The pre- and post-fix modules were executed against all 68 Ally consolidated reviews on the 25 most recent PRs of this repo:
Every flip is
true → false, every flipped review yields zero finding identities, and all 40 reviews carrying real findings still block.Risks
Low, and bounded in the safe direction. The change can only ever turn a
trueinto afalse, and only for a body where the reviewer has explicitly tallied both severities at zero. It cannot suppress an uncounted heading, an explicit change request, or a non-zero bucket — each is asserted by a test at 0/0.Three things worth a reviewer's attention:
Blockcast/paperclipandBlockcast/multicastreview/ally-commentis not inrequired_status_checks, so it functions as a stop sign for judgment rather than a mechanical gate. The mitigation is that the suppressed reds were false — a gate that cries wolf at ~10% is what erodes the stop sign.github-webhook.ts. The predicate also feedsisActionableReviewFeedbackContext, which gatesreopenInReviewIssueForActionablePrFeedback. After this change a clean 0/0 review no longer reopens anin_reviewissue. That is the intended behaviour and not a lost notification: the PR-author wake fires from theisPrWakepath independently of this flag (see the comment block atgithub-webhook.ts:351-365), so the author is still told the review landed.pr-comment-review-gate.ts:250conflates "no counted bucket at all" (refs === null) with "buckets present but empty" (refs === []), treating both as not-dispositioned. I did not change it: it is a fail-closed guard, and loosening it points the same direction as the defect BLO-29711 exists to fix. This change drains its reachable input rather than widening it — after the fix, zero of the 68 corpus reviews are actionable-with-no-refs. The shape is narrowed, not eliminated: a review declaring exactly one bucket at zero plus tripping prose would still reach it. Zero live instances; recorded on the issue rather than fixed here.Model Used
claude-opus-5[1m], 1M context, extended thinking), via Claude Code with tool use and code execution. Test execution, the pre/post regression census, and the negative control were all run rather than reasoned about.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template