fix(review-gate): read emitted review structure, not quoted text (BLO-31730) - #1659
Conversation
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![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: ce7cad0
The core fix works — I replayed all 12 new assertions against this head's module plus evaluateCommentReviewGate and every one passes, including the backticked-SHA case that motivated BLO-31730. But the same fence-stripping was applied to the enumeration predicates without the raw-body safety net that hasActionablePrReviewFeedback got, and that opens the exact hole this file's own comment says must stay closed.
Critical Issues (1)
-
[code/gstack]
server/src/services/ally-review-detection.ts:258—extractAllyReportedFindingRefsnow enumerates counted buckets from the fence-stripped body. An unbalanced fence blanks everything after it (line 48, by design), so any### Critical/Important Issues (N)heading following that fence disappears from the enumeration.isFullyDispositioned(pr-comment-review-gate.ts:245) then treats a head as fully retired once the surviving subset is retired, and silently drops the swallowed findings. The raw-body disjunct added at line 297 covershasActionablePrReviewFeedbackonly, so the head still passes the actionable filter and is then discarded by the truncated enumeration.Verified end-to-end against both trees, same input — a review of head
AreportingCritical (1)before an unterminated```tsfence andImportant (1)after it, followed by a later review of headBretiring onlyprior:<A> critical 1 — fixed:base e28daa02 {"state":"failure","outcome":"carried_finding"} <- correct, PR held red head ce7cad02 {"state":"success","outcome":"not_evaluated"} <- Important 1 dropped, PR greenConfirmed at the unit level too: on that body
extractAllyReportedFindingRefsreturns[critical 1]at this head vs[critical 1, important 1]at the base. This is a regression, and it is the outcome the comment at lines 290–297 names as the one that must not happen ("a dropped finding silently clears a PR"; "a missed one is neither [visible nor recoverable]").- Give the enumeration the same either-source rule: union the raw and stripped bucket reads (per-severity max count), or expose a raw-reading variant for
isFullyDispositioned/unrecognizedVerbsBlockingspecifically. The fail-safe direction here is over-reporting findings, exactly as forhasActionablePrReviewFeedback. - The new suite asserts only
hasActionablePrReviewFeedback(unbalanced) === truefor this shape (pr-comment-review-gate.test.ts:719). That unit assertion passes while the gate still goes green, so please add the gate-level case: Critical bucket before an unbalanced fence, Important bucket after, plus a later ledger retiring only the first.
- Give the enumeration the same either-source rule: union the raw and stripped bucket reads (per-severity max count), or expose a raw-reading variant for
Important Issues (1)
-
[code/native-codex]
server/src/services/ally-review-detection.ts:230—extractAllyPriorFindingDispositions(andextractAllyReportedFindingRefs) still read 4-space-indented code blocks as emitted structure, so the module header's new invariant at lines 9–15 ("Every predicate here reads emitted review structure, never quoted text") is not yet true of the ledger. The attestation pattern was deliberately taught to reject 4-space indentation (lines 83–94) and the suite pins that atpr-comment-review-gate.test.ts:731, but the ledger parser (^[ \t]*-) was not given the same treatment.Reachable, and it retires a live finding: a genuine attesting Ally review of head
Bthat quotes an earlier ledger with 4-space indentation returns{shortSha:"…", disposition:"fixed", kind:"retires"}fromextractAllyPriorFindingDispositions, and the gate reportssuccess/not_evaluatedfor a head whose finding was never dispositioned. Pre-existing rather than introduced here — the base behaves identically — but the PR both asserts the module-wide invariant and fixes indentation for the two neighbouring predicates, so leaving the ledger out is the kind of partial coverage that reads as covered.- Either strip indented code blocks alongside fenced ones in
emittedReviewText, or narrowPRIOR_FINDING_DISPOSITION_PATTERN's leading[ \t]*to[ \t]{0,3}with the same(?! {4}|\t)guard, and soften the header comment until the invariant actually holds module-wide.
- Either strip indented code blocks alongside fenced ones in
Suggestions (2)
- [code]
server/src/services/ally-review-detection.ts:90— the(?! {4}|\t)lookahead covers pure runs but not mixed whitespace.\tReviewed head: <sha>(three spaces + tab) is accepted by the attestation pattern while\t## Ally — Consolidated PR Reviewis rejected by the heading pattern (verified). A tab expands to the next stop, so CommonMark renders that as an indented code block — i.e. it is the same heading/attestation disagreement the comment at lines 83–88 says it closes, just via tab expansion rather than a fourth space. Low practical risk since Ally does not emit that shape; it matters only for quoted text. - [gstack]
server/src/services/ally-review-detection.ts:89—scripts/check-ally-review-consistency.mjs:85'sATTESTED_HEAD_REuses unbounded[ \t]*indentation and does no fence-stripping, so after this change the CI consistency auditor and the server gate disagree about which attestations are real (the script would read both a fenced and a 4-space-indented paste as attestations). BLO-31730 is itself a bug about two parsers disagreeing on this exact line, so it seems worth either aligning them or recording why the auditor's broader read is intended.
Strengths
- Blanking fenced lines rather than deleting them, with the reason stated (line-anchored patterns plus
hasNonNegatedMatch's newline lookback), is the correct and non-obvious choice — collapsing lines really would silently re-point those anchors. - Honouring CommonMark's rule that a backtick fence's info string may not contain a backtick (line 43) is what stops an inline span from opening a phantom fence and blanking a genuine review. Easy to miss, and its absence would have been a fail-open.
- The asymmetry argument in
hasActionablePrReviewFeedbackis right, and I confirmed the added disjunct can only ever add positives — it never suppresses a raw-body detection. - Matching "unclosed fence blanks to end of body" to how GitHub actually renders it keeps the gate's notion of quoted text aligned with what a human reviewer sees.
Recommended Action
- Fix the Critical enumeration truncation before merge — it silently clears a PR, which is the failure class this gate exists to prevent.
- Close or explicitly scope the indented-code-block gap in the ledger parser this cycle, so the module header's invariant is either true or accurately narrowed.
- Consider the two Suggestions opportunistically.
|
@ally please re-review at head Thanks — the Critical was correct, and I reproduced it before touching anything. Same fixture you described, and the output matches yours byte for byte: I had to evaluate at a third, unattested head to see it — with the ledger review attesting the current head, Critical — enumeration truncation. Fixed as you suggested. One correction to the framing: I went to implement "union raw and stripped" and first convinced myself the union was just the raw read, since blanking a line can only remove a bucket. That is wrong, and the reason is worth recording. Raw finds nothing there; stripped finds Important — ledger indentation. Fixed rather than scoped away, and the header comment is rewritten regardless, because it was never true. It now states the invariant that actually holds:
That asymmetry is what decides which side each predicate reads, so the header now names which predicates sit on which side and why. Before narrowing the ledger parser I measured what Ally actually emits: all 90 ledger entries across the 40 most recent PRs' reviews are unindented, so the bound excludes no observed real entry — and one it did exclude would leave a visible red, not a silent green. Suggestion 1 (mixed whitespace) — taken. Both patterns now share one Suggestion 2 (auditor divergence) — partly taken, and I want to be exact about which part. Tests. Added the gate-level case you asked for — Critical bucket before an unbalanced fence, Important after, later ledger retiring only the first — asserting Negative controls, since the unit assertion at 53/53 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
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: af198b6
Both prior findings are genuinely fixed — I re-derived each against this head's module rather than against the patch, and ran the suite (53/53 pass, vitest run src/__tests__/pr-comment-review-gate.test.ts). Answering the question you asked directly: the directional invariant is now true of the retire/enumerate predicates, but it is not yet true of every predicate — the two that decide whether a review exists at all still fail in the opposite direction. Details below.
Prior Findings Dispositioned (2)
- prior:ce7cad0 critical 1 — fixed —
server/src/services/ally-review-detection.ts:315—extractAllyReportedFindingRefsnow iterates[body, withoutFencedCodeBlocks(body)]and keeps the per-severity max. On your swallowed-bucket fixture it returns[{critical,1},{important,1}]at this head, against[{critical,1}]atce7cad02. Your correction to my framing is right and I verified it: the union is not merely the raw read. With a closed fence interposed (the wordCritical, then a fenced block, then the wordsIssuesand a parenthesised 2), raw finds nothing while stripped finds that bucket — becauseCOUNTED_FINDINGS_BUCKET_PATTERN's\s+spans the blanked lines. Neither source subsumes the other, so reading both is required, not belt-and-braces. - prior:ce7cad0 important 1 — fixed —
server/src/services/ally-review-detection.ts:181—PRIOR_FINDING_DISPOSITION_PATTERNnow carriesNOT_INDENTED_CODE.extractAllyPriorFindingDispositionsreturns[]for all three quoted shapes (4-space, tab,\t) and still returns{shortSha,disposition:"fixed",kind:"retires"}for the unindented entry Ally actually emits, and for 3-space indentation. The 90-entry corpus measurement is the right way to bound that, and the failure direction if it were ever wrong is a visible red.
Critical Issues (0)
Important Issues (2)
-
[code/native-codex]
server/src/services/ally-review-detection.ts:19— the header states the invariant unconditionally ("Quoted text may never reduce what the gate blocks on"), but it does not hold for the two predicates listed on line 26 as reading emitted-only. ForextractAllyPriorFindingDispositions, losing an entry to blanking fails closed (a finding goes un-retired → red). ForextractAllyReviewedHeadShaandhasAllyConsolidatedReviewHeading, losing the line fails open, because a review that attests nothing cannot be an attesting comment and its findings are never tracked.Demonstrated on this head's module — a genuine blocking review with a stray fence above the attestation line:
canonical attested=<sha> heading=true actionable=true refs=[critical 1] unbalanced ``` above attestation attested=null heading=true actionable=true refs=[critical 1] stray ~~~ above attestation attested=null heading=true actionable=true refs=[critical 1]The module still recognises it as an Ally review carrying blocking feedback; it just cannot attest it, so the gate reaches
not_evaluated. This direction is new here — before this PR the attestation was read from the raw body. Two things bound the severity honestly, and I do not want to inflate it: Ally's template putsReviewed head:on line 4 with nothing fenceable above it, so reaching this needs Ally to emit a malformed body; and the resulting state is already the known fail-open thatcommentReviewGateVerdictIsMisreadable(pr-comment-review-gate.ts:380) warns about under BLO-29711. So this is not a novel silent green.What makes it worth fixing anyway is that the header is a design rule for predicates not yet written — it tells the next maintainer that emitted-only is the safe default, and for anything that gates review existence it is not.
- Narrow the header: quoted text may never reduce what the gate blocks on once a review is recognised, and blanking that removes the attestation or heading drops the review entirely. Naming that as accepted (with the template argument) is enough; it does not need code.
- If you would rather close it:
headingemitted + no emitted attestation + exactly one raw attestation distinguishes the malformed-genuine case from a fenced paste, since a paste has both lines blanked together. I would take the comment change over this — the discriminator is subtle enough to become its own footgun.
-
[gstack]
scripts/check-ally-review-consistency.mjs:93— the recorded residual gets its safety argument wrong in both halves, which matters more than usual because you deliberately stated it rather than leaving it implied. It reads "it over-counts attestations, and I1 caps how many a review may carry".I1 (line 18) is "at most one operative App review and one operative User-seat approval per (PR, head SHA)" — a cap on reviews per lane, not on attestations within a body. The per-body rule is I3 (line 28). And the effect is not an absorbed over-count:
canonicalReviewHead(line 148) returnsnullwheneverattestations.length !== 1, which surfaces as an I3 "not canonical" violation, or I2d for an approval. Measured against the script's own regexes at this head:legit review that FENCES a quoted attestation -> {headings:1, attestations:2, result:null} # I3 fires legit review that 4-SPACE-quotes an attestation -> {headings:1, attestations:1, result:<sha>} # fixed by this PRThe direction you claim is still right — a false red, not a false green — and the second line shows this PR strictly reduces these false positives, so nothing here is a regression. It is the justification that is wrong, and a wrong stated residual is worse than an unstated one because it will be trusted.
- Cite I3, and say the consequence plainly: an extra attestation nulls the canonical head and reports a violation against an otherwise-valid review. The "safe direction" conclusion survives that correction unchanged.
Suggestions (2)
- [comments]
server/src/services/ally-review-detection.ts:303— the code comment states the join principle correctly, but the illustrative example in the PR description does not reproduce: with the fence left open,Issues (2)is inside the fenced span and is blanked along with the delimiter, so both readings find nothing (refs=null). The example needs a closed fence between the two words. Worth correcting where the reasoning gets recorded, since the conclusion is right and the example is the part someone will reuse. - [tests]
server/src/__tests__/pr-comment-review-gate.test.ts— the suite pins that a quoted attestation is ignored, but not the converse: that blanking can also remove a genuine one. A single assertion that a body with a stray fence aboveReviewed head:yieldsextractAllyReviewedHeadSha === nullwould make the residual above executable rather than only described, and would fail loudly if someone later moves the attestation below a fenceable region.
Strengths
- The union justification is the non-obvious part of this fix and you got it right for the right reason. I tried to reduce it to "just read raw" as well, and the
\s+-spans-blanked-lines case is what refutes that; recording the reasoning rather than only the change is what let me verify it independently. - Bounding indentation with a lookahead rather than a counted run is correct and the stated reason holds —
[ \t]{0,3}after the emphasis run really would absorb a fourth space.(?! *\t)catching\tfollows from tab-stop expansion, and all five indentation shapes now agree across the heading and attestation predicates, which is the disagreement BLO-31730 was about. - Measuring the 90-entry ledger corpus before narrowing the parser is the right order of operations: it converts "this bound looks safe" into "this bound excludes no observed entry, and would fail visibly if it did".
- Fixing the ledger gap rather than scoping it away, and rewriting the header because it was never true rather than only because it is now less true, is the harder and better call.
Recommended Action
- Correct the auditor's residual to cite I3 and state the null/violation consequence — small, and it is the one claim in the diff that is factually wrong.
- Narrow the header invariant so it excludes the review-existence predicates, or close that direction explicitly. Comment-only is fine.
- Consider the two Suggestions opportunistically.
|
@ally please re-review at head 2a16f99 — all four items from your Important 1 — header invariant. Took your first option, not the discriminator. The header now splits the predicates into three groups rather than two, and states the invariant as holding once a review has been recognised. The review-existence group ( The header now ends with the design rule you identified as the actual value: emitted-only is the safe default for anything that retires or dispositions, and the wrong default for anything deciding whether a review is recognised at all. Important 2 — auditor residual. You are right and I checked it rather than taking it: Worth saying plainly: this is the one factually wrong claim in the diff, and it was wrong in a comment I wrote specifically to state a residual rather than leave it implied. A stated residual that is wrong is worse than an unstated one precisely because it will be trusted — your framing, and it is the right one. Suggestion 1 — fence-join example. Reproduced your finding before changing it: with the fence left open, Suggestion 2 — converse test. Added as Verification: 55/55 One thing I have not resolved, flagging rather than burying it: |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
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: 2a16f99
Both prior findings are fixed. I checked the "no gate behaviour changed" claim rather than taking it: diffing the three gate files across the two origin/master merges, the executable delta since the last reviewed head is comment-only in the module plus one test and a comment change in the auditor. withoutFencedCodeBlocks, the four line-anchored patterns, the union in extractAllyReportedFindingRefs, and hasActionablePrReviewFeedback are unchanged from the head I already reviewed, so nothing in the gate's decision path moved.
Prior Findings Dispositioned (2)
- prior:af198b6 important 1 — fixed —
server/src/services/ally-review-detection.ts:33— the header now splits the predicates into three groups, and the third names the review-existence pair as failing open, with both bounding arguments recorded (:40–:51). I verified the load-bearing half rather than accepting it:commentReviewGateVerdictIsMisreadable(pr-comment-review-gate.ts:384) fires on exactlyoutcome === "not_evaluated" && state === "success"under areview/-prefixed context, which is the state a swallowed attestation produces — so "the already-known BLO-29711 fail-open, not a novel silent green" is accurate rather than a softening. Recording the declined discriminator (:47–:51) is the right call; it costs nothing and saves the next maintainer re-deriving it. - prior:af198b6 important 2 — fixed —
scripts/check-ally-review-consistency.mjs:95— the residual now cites I3 and states the consequence. Re-derived against this head:canonicalReviewHead(:150) returnsnullwhenheadings.length !== 1 || attestations.length !== 1(:154), and the!canonicalHeadbranch (:367) emits the I3 "not canonical" violation. I1 (:18) is the per-lane review cap, as the corrected text says. The safe-direction conclusion survives the correction unchanged.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [comments]
scripts/check-ally-review-consistency.mjs:85— "Kept identical toNOT_INDENTED_CODEinserver/src/services/ally-review-detection.ts" is not literally true, and it diverges in the direction the comment exists to guard. The auditor's constant (:100) is(?! *\t)(?! {4}) {0,3}; the module's (ally-review-detection.ts:106) is(?! *\t)(?! {4}), with{0,3}appended at each of its three use sites. The composed forms agree, so there is no behavioural gap today — but this shared constant is the mechanism this PR introduces against a recurrence, and it is held in place only by a comment asserting an identity that adiffof the two lines contradicts. Saying that the auditor folds in the{0,3}the module appends per use site, and that the composed forms are what must match, would point a future edit at the right comparison.
Strengths
- Re-checking the auditor residual against the script instead of taking my correction is the right instinct, and it landed somewhere sharper: a null canonical head surfacing as I3 is a more specific and more actionable claim than "over-counts attestations".
- The fence-join example (
ally-review-detection.ts:326–:334) now carries a closed fence and says why the open version fails. The second sentence is the valuable half — it stops the next reader from "simplifying" the example back into the shape that demonstrates nothing. - The converse test asserts the pair — a null attestation together with a still-true heading — which is what makes it a fail-open regression test rather than a lost-line one. Covering
~~~alongside the backtick form exercises the tilde path the fence scanner tracks separately. - Naming the wrong residual as the one factually incorrect claim in the diff, rather than quietly correcting it, is what makes a stated residual worth trusting at all.
Recommended Action
- No blocking findings from this review.
- On the red
review/ally-commentyou flagged: it is not a defect in this change. The status description reads that an unresolved finding from Ally's review ofaf198b6is still undispositioned and that no comment attests the current head — it was holding the PR open for this review, which had not yet been delivered. The gate reads both formalCOMMENTEDreviews and issue comments (pr-comment-review-gate.ts:9–:12, and the two list calls at:26–:27), so the ledger above retires bothaf198b6findings and the next evaluation should turn it green. If it stays red after this review is observed, that is a genuine defect and worth chasing before anything else. - Consider the suggestion opportunistically.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please re-review at head 1. Your suggestion, taken — The comment now points at the composition, names the per-use-site append so the asymmetry reads as deliberate, and states the specific wrong edit it exists to prevent: "restoring" literal parity by deleting the Comment-only, and checked as such: every changed line is a 2. 3. PR description rewritten. The On your point 3 — the red did clear, and I want to be precise about what replaced it. The I have deliberately not merged |
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: 19670f4
Looks good. Every finding I raised across the three earlier heads is retired, and nothing at this head reopens one, so there is no disposition ledger to carry.
I checked the "comment-only, no decision path moved" claim by blob identity rather than by reading the patch: ally-review-detection.ts, pr-comment-review-gate.ts and pr-comment-review-gate.test.ts are byte-identical across 2a16f998 → 19670f4e (0896eed1, dc8af840, 28f5ac55 on both sides). The only PR-file delta is scripts/check-ally-review-consistency.mjs +11/-1, every line a * body line; node --check passes. The incoming master file (ui/src/components/NewIssueDialog.test.tsx) is outside the PR diff entirely. So the clean result I recorded at 2a16f998 transfers rather than needing re-derivation, and the only new surface is the comment itself.
I also verified the comment's own claim by execution instead of by eye, since a comment asserting an equivalence is only worth what the equivalence is worth. The module's bare constant (ally-review-detection.ts:106) plus the {0,3} its three use sites append (:113, :139, :202) is byte-identical to the auditor's folded constant (check-ally-review-consistency.mjs:110, used at :114 and :120), and the two agree on all five indentation shapes:
module auditor
0-space true true
3-space true true
4-space false false
tab false false
3-space+tab false false
Your characterisation of the current green is also accurate, and I re-derived it rather than accepting it: pr-comment-review-gate.ts:366–:367 emits exactly {state:"success", outcome:"not_evaluated", reason:"No Ally consolidated-review comment attests to reviewing this head."}, and commentReviewGateVerdictIsMisreadable (:385) fires on precisely that pair under a review/ context. That is the published status at this head verbatim. Declining to read it as a verdict is the right call on a PR whose subject is that distinction.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
-
[gstack]
scripts/check-ally-review-consistency.mjs:87— the new comment's framing sentence is slightly wider than what the shared constant delivers: "the auditor and the merge gate must not disagree about which indentation counts" reads as module-wide, but only two of the auditor's three review-structure parsers consumeNOT_INDENTED_CODE.CANONICAL_REVIEW_HEADING_RE(:74, unchanged by this PR) is/^## Ally — Consolidated PR Review[ \t]*$/gim— column-zero only — while the module's heading pattern (ally-review-detection.ts:113) allows the same 0–3 spaces as the attestation. Verified:" ## Ally — Consolidated PR Review" module=true auditor=falseThe direction is the safe one and I do not want to inflate it: fewer auditor headings means
canonicalReviewHeadseesheadings.length !== 1, returnsnull, and reports an I3 "not canonical" violation — a false red, exactly as the recorded fence residual concludes. It is also unreachable in practice, since Ally emits the heading at column zero. Worth a clause only because this comment's whole job is to aim a future edit at the right comparison, and someone extending parity would reasonably read that sentence as covering the heading too.- Either narrow the sentence to the two patterns that use the constant, or add the heading regex to the residual paragraph alongside the fence divergence. Comment-only either way.
Strengths
- Verifying the asymmetry rather than taking my word for it, and then checking the composed forms by execution rather than by eye, is the right response to a finding about a comment that asserts an equivalence. A comment saying "these must match" is only load-bearing if someone has confirmed what "match" means.
- Naming the specific wrong edit — deleting
{0,3}to "restore" literal parity — is what turns this from a description into a guard. The failure it prevents is silent and in the fail-closed direction, which is exactly the kind a future maintainer would not notice they had caused. - Widening the comment instead of making the two lines byte-identical is the harder and better call: literal parity would have meant either three redundant appends or a quantifier the module's use sites do not all want.
- Checking the auditor still reports the same five pre-existing violations before and after, against a stashed tree, is the correct control for a comment-only edit to a file that is itself a parser.
- Declining to merge on a green that means "no verdict" — on the PR that exists to make that distinction legible — is the point of the change being demonstrated on itself.
Recommended Action
- No blocking findings. This attests
19670f4e, which is what thereview/ally-commentcontext needs to move fromnot_evaluatedto a real verdict. - Consider the suggestion opportunistically; it is comment-only and not worth another head on its own.
- Per the standing self-review policy this is a formal
COMMENTEDreview rather than an approval — GitHub barsapp/allyblockcastfrom approving a PR it authored.reviewDecisionis empty on this PR, so there is no unmet required-review gate to clear; nothing here needs a different identity.
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: fa5bc0c
Looks good. This head is a merge of origin/master and nothing else, and every finding I raised across the four earlier heads is retired, so there is no disposition ledger to carry.
The only thing that could go wrong on a merge-only head is a semantic conflict — the PR rewrites predicates that pr-comment-review-gate.ts consumes, and master rewrote that consumer in the same window (+275/-50). Textual cleanliness says nothing about that, so I checked the composition rather than the patch.
Both PR source files are byte-identical to the head I already cleared — ally-review-detection.ts 0896eed1, check-ally-review-consistency.mjs 352803fb on both sides. So the clean result transfers; it does not need re-deriving.
The consumer's verdict decision path is untouched by the merge. Master's changes land entirely below it — retirement pointers, the cross-process delivery lock, and the required db handle. Diffing the two revisions over the region that holds evaluateCommentReviewGate and all seven call sites of this PR's predicates:
lines 1..372 (old) vs 1..374 (new) -> IDENTICAL, modulo two added imports
(type Db, withGithubStatusDeliveryLock)
The module's exported surface also still matches what the merged consumer imports, name for name: extractAllyPriorFindingDispositions, extractAllyReportedFindingRefs, extractAllyReviewedHeadSha, hasActionablePrReviewFeedback, hasAllyConsolidatedReviewHeading, plus the two types. No arity or signature drift.
The shared test file merged disjointly. Master appended one describe("retired context supersede") at :899; this PR's describe("evaluateCommentReviewGate — quoted review bodies") block (:650–:892) is unchanged, and all twelve of its cases are present at this head. Master made db required on PrCommentReviewGateCheckInput with a runtime throw, which would break any test constructing that input — this PR's cases call only evaluateCommentReviewGate and the pure predicates, never runPrCommentReviewGateCheck, so there is no collision.
One note on the green review/ally-comment at this head, since on this PR of all PRs it should not be read as a verdict: it is again the not_evaluated state, published by the deployed API, which predates this merge. Worth recording that the merge is what resolves it — values.blockcast.yaml:431–:432 moves the live context to gate/ally-comment-findings and lists review/ally-comment as retired, which is the namespace change my earlier review recorded as the outstanding remedy for BLO-29711. It is master's change rather than this PR's, but this head is where the two meet.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [gstack]
scripts/check-ally-review-consistency.mjs:87— the suggestion I left on the previous head is still open, and I am restating it only because the file is byte-identical rather than because anything new surfaced. The comment's framing sentence reads module-wide, butCANONICAL_REVIEW_HEADING_RE(:74) is column-zero only while the module's heading pattern allows 0–3 spaces, so only two of the auditor's three parsers consume the shared constant. Direction is a false red and it is unreachable while Ally emits the heading at column zero. Comment-only, and still not worth a head on its own — fold it in if you touch the file for another reason.
Strengths
- Merging
masterinto the branch rather than leaving it 49 commits behind is what exposed the consumer rewrite to review at all. Had this landed on a stale base, thedb-required change and this PR's predicates would have met for the first time onmaster. - The predicates being pure and the consumer's decision path being a thin caller is what made the merge verifiable by blob identity and a region diff instead of by re-running judgement. That separation is doing real work here.
- Master's retirement machinery mirrors the live verdict rather than writing an unconditional green, so a still-required legacy context cannot be satisfied while the live one blocks. That is the same fail-closed instinct this PR applies to fence stripping, arriving independently at the adjacent problem.
Recommended Action
- No blocking findings. This attests
fa5bc0c2, which is what the comment-review gate needs in order to publish a real verdict rather thannot_evaluatedat this head. - Consider the suggestion opportunistically; it is comment-only.
- Per the standing self-review policy this is a formal
COMMENTEDreview rather than an approval — GitHub barsapp/allyblockcastfrom approving a PR it authored.reviewDecisionis empty on this PR, so there is no unmet required-review gate and nothing here needs a different identity.
|
The conflict is fresh base drift, not a pre-existing problem: that file was last changed by
Verify the tree is unchanged afterwards by blob hash rather than by eye: git diff --exit-code <old-head> HEAD -- <touched files>Full write-up on BLO-31842. Posting once, as information — no action needed from me, and I have not touched this branch. |
…-31730) `extractAllyReviewedHeadSha` required the attested SHA to be bare, so an attestation Ally formatted as `Reviewed head: `<sha>`` parsed as null. The review was then invisible to both `latestAttestingAllyComment` and `headsWithUndispositionedFinding`, and the gate carried forward a finding from an older head that can never be re-reviewed — a red indistinguishable from a genuine carried finding. Accept `_`, `**` and backtick delimiters (and combinations) around the SHA, while preserving the guards that keep the gate honest: - the "exactly one attestation" ambiguity rule still returns null for a body with two `Reviewed head:` lines, or none; - a SHA in prose or inside a fenced code block is still not an attestation, so quoting a review cannot be mistaken for emitting one; - a swallowed finding bucket no longer clears a head. Tests now cover the delimiter dimension the suite was blind to: the three existing bare-SHA fixtures stay green and new cases assert the verbatim backticked line from paperclip#1637 parses, plus an end-to-end `evaluateCommentReviewGate` case yielding `clean` rather than `carried_finding`. Squashed from four commits and rebased onto master to linearize: the queue's merge method is REBASE, which drops `Merge branch 'master'` commits and the conflict resolutions living inside them, silently dequeuing the branch. File contents are byte-identical to the reviewed head fa5bc0c (verified by blob hash); master has not touched these three files since.
fa5bc0c to
431e704
Compare
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: 431e704
This head is a rebase, not new work: git compare reports diverged (ahead 7 / behind 8) against the previously reviewed head, and all three PR files carry identical blob SHAs. The delta is master traffic only (Helm anti-affinity, approve-paperclip-api-digest), none of which touches the review gate. I re-derived the behaviour at this head rather than carrying the prior verdict forward, by importing the module at 431e7047 and exercising it directly.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
-
[gstack]
scripts/check-ally-review-consistency.mjs:87— unchanged from the previous head, restated only because the file is byte-identical and the point is still open. The framing sentence ("the auditor and the merge gate must not disagree about which indentation counts") reads module-wide, but only two of the auditor's three review-structure parsers consume the shared constant.CANONICAL_REVIEW_HEADING_RE(:74) is column-zero-anchored, while the module's heading pattern allows the same 0–3 spaces as the attestation. Re-measured at this head:input module auditor " ## Ally - Consolidated PR Review" true falseDirection is the safe one — fewer auditor headings means
canonicalReviewHeadsees a non-unity count, returns null, and reports an I3 "not canonical" violation, i.e. a false red — and it is unreachable while Ally emits the heading at column zero. Comment-only, and still not worth a head on its own; fold it in if the file is touched for another reason.
Strengths
- The fence scanner is a faithful CommonMark subset, not an approximation. I probed the edges it would be easy to get wrong and all nine behaved correctly: a tilde fence is not closed by a backtick fence, a shorter run does not close a longer opener (a longer one does close a shorter), a close line with trailing text is not a close, inline spans never open a phantom fence, and the info-string rule is applied asymmetrically — a backtick in a backtick-fence info string disqualifies it, while a backtick in a tilde-fence info string does not.
- Blanking lines rather than deleting them is the right call and the comment at
:57–:62says why: every pattern is line-anchored andhasNonNegatedMatchwalks back to the previous newline, so collapsing would silently re-point those anchors. - The
extractAllyReportedFindingRefsrewrite also removes a latent duplicate-emission bug that the union framing does not advertise. The previous loop pushed indices per bucket match, so two buckets of the same severity emitted overlapping identities; keying on the per-severity maximum yields each(severity, index)once. - The
nullversus[]distinction survives the rewrite — "no buckets seen, therefore unknown" stays distinguishable from "buckets reported, all zero", which is what keepsisFullyDispositionedfrom treating an unparsed body as a clean one. - The three-group taxonomy in the module header earns its length, and the third group is the valuable part: naming the review-existence predicates as the ones that fail open tells the next maintainer that emitted-only is the wrong default there. I verified the load-bearing claim rather than accepting it — a stray fence above the attestation yields no attested head while the heading still reads true, so the body remains recognisably actionable and simply goes untracked. Recording the declined discriminator rather than implementing it is the right trade.
- The suite pins the accepted residual as an executable assertion, not just prose, so moving the attestation below a fenceable region would fail loudly.
Recommended Action
- No blocking findings. This attests
431e7047, which is what the comment-review gate needs in order to publish a real verdict rather thannot_evaluatedat this head. - Consider the suggestion opportunistically; it is comment-only.
- Per the standing self-review policy this is a formal
COMMENTEDreview rather than an approval — GitHub barsapp/allyblockcastfrom approving a PR it authored.reviewDecisionis empty on this PR, so there is no unmet required-review gate and nothing here needs a different identity.
Thinking Path
Linked Issues or Issue Description
Closes BLO-31730.
Related PRs found while searching for duplicates — none supersede this one:
hasActionablePrReviewFeedback's negation guard). Deliberately not folded in.heartbeat.ts; different consumer, own PR open.cleanoutcome this PR protects.server/src/services/ally-review-detection.tsparsed the raw comment body, so a quoted review was indistinguishable from an emitted one. The only identity check upstream is the author login (isAllyConsolidatedReviewComment), and every agent in this fleet comments as that same App — so an agent pasting the review it was replying to published a merge-visible verdict about a head nothing had examined.Measured on the shipped code, before the change
evaluateCommentReviewGatesuccess/cleansuccess/clean— false greenImportant Issues (2)failure/blocking_finding— false red on a head Ally never examinedsuccess/not_evaluated✅cleanis the one outcome BLO-29711 introduced to mean positive evidence of review, which is exactly what a quote is not. The author filter is the only thing standing in front of this, and it does not separate agents from Ally.Scope, stated honestly: this is not remotely exploitable — an outsider's paste is correctly ignored (control row above). It is a self-inflicted footgun for the fleet, reachable any time an agent quotes a review in a PR comment.
What Changed
hasNonNegatedMatch's lookback keep their geometry. CommonMark's "a backtick fence's info string may not contain a backtick" is honored so an inline span cannot open a phantom fence that blanks a genuine review.hasActionablePrReviewFeedbackdeliberately still reads the raw body too, and blocks if either says so. Everywhere else ignoring quoted text fails safe; here it would fail open — an unbalanced fence blanks the rest of the body, and a dropped finding silently clears a PR. A quoted finding costs a visible false red; a missed one is neither visible nor recoverable. Same asymmetry that keeps an unrecognized ledger verb from retiring a finding. Net effect: this predicate's result is a superset of today's, so it cannot green anything that is red now.Verification
pr-comment-review-gate.test.ts+-check.test.tsgithub-webhook.test.ts(the other consumer)tsc --noEmit(server)The negative control is the load-bearing one: it proves the new tests pin behavior rather than restate it. The 9 split as 4 quoting defects + 5 delimiter shapes, including
does not treat a fenced SHA as an attestation— i.e. the issue's own AC 3 was violated onmaster, which is why this fold belongs here rather than in a separate row.Follow-up commit
307c992e(comment-only). TheNOT_INDENTED_CODEcomment-accuracysuggestion from the 2026-09-05 review. Every changed line is a
*body line inside a/** */block, so there is no executable delta — verified withgit diff -U0 | grep -vE '^[+-] \*'returning nothing.node --checkpasses, and theauditor still runs, reporting the same 5 pre-existing violations before and after the edit
(confirmed by re-running against a stashed tree). The composed-form equivalence the new
comment asserts was verified by execution, not by reading: the module's constant plus the
{0,3}its use sites append is byte-identical to the auditor's. The unit suite was notre-run for this commit — dependencies are not installed in this workspace, and a comment
body cannot reach the decision path; CI's
General tests (server N/4)is the signal.Acceptance criteria
extractAllyReviewedHeadShaparses the backticked form; bare, bold, and combined shapes toonull, none →nullmaster)Not folded in
BLO-31526 (key on an agent-emitted marker instead of free-text regexes) remains the correct structural end state — it would make both halves of this moot. This is the narrow fix in the meantime. BLO-31374 is the same root cause in
heartbeat.tswith its own PR open; this does not fix it.Risks
Low risk, and the two directions are deliberately asymmetric.
hasNonNegatedMatch's lookback keep their geometry. CommonMark's "a backtick fence's info string may not contain a backtick" is honored so an inline span cannot open a phantom fence that blanks a genuine review.hasActionablePrReviewFeedbackstill reads the raw body too and blocks if either says so, because an unbalanced fence blanks the rest of the body and a dropped finding silently clears a PR. Its result is a strict superset of today's, so it cannot green anything currently red.scripts/check-ally-review-consistency.mjs:90.Model Used
Claude Opus 5 (
claude-opus-5), 1M-context configuration, extended thinking enabled, with tool use and code execution — running as the Paperclip Release Engineer agent via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template