feat(review-gate): key the ally-comment gate on a structured verdict block, not prose regexes (BLO-32695) - #1721
allyblockcast[bot] wants to merge 11 commits into
Conversation
![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: a6e956a
The premise is right, and the reasoning in the module docs is unusually good — the four-readers hazard, the absent/unreadable split, and the "scoped to newest so a route out always exists" argument are all correctly identified and correctly resolved. Two findings below, both about the block path's vocabulary rather than its structure.
Critical Issues (1)
-
[code / gstack-review]
server/src/services/ally-review-detection.ts:594— The structured path treats any positive severity count as blocking, but the prose path it replaces blocks only on Critical/Important. The template this PR ships mandates asuggestionscount, so the first review emitted under the new format with one or more suggestions reports a falseblocking_finding.hasActionablePrReviewFeedbackblock branch:for (const count of block.verdict.findings.values()) if (count > 0) return true;
Prose equivalent,
carriesBlockingFeedbackat:571(andCOUNTED_FINDINGS_BUCKET_PATTERNat:392), both anchored to two severities only:/\b(?:Critical|Important)\s+Issues\b[*_]*\s*\((\d+)\)/giAnd
.planning/ally-agent/AGENTS.md:168instructs the emitter to always populate a third key:"findings": { "critical": <n>, "important": <n>, "suggestions": <n> }
Reproduced against the verbatim block-path logic at this head, using the template's own payload shape for a clean review offering two suggestions:
clean review, 0 crit / 0 imp / 2 suggestions prose path hasActionablePrReviewFeedback -> false BLOCK path hasActionablePrReviewFeedback -> true BLOCK path reportedFindingRefs -> [{"severity":"suggestions","index":1}, {"severity":"suggestions","index":2}]Three consequences, in increasing order of severity:
- Merge gate.
pr-comment-review-gate.ts:376feeds this predicate straight intoblocking_findingfor the current head. A clean review with suggestions goes red — inverting the stated goal, and doing so on the most common review shape rather than an edge case. - Author wakes.
github-webhook.tsroutes on the same predicate (:421,:429,:1317,:3403), so the PR author is also woken with "actionable feedback" for a review that has none. - Unretirable carry.
extractAllyReportedFindingRefsat:543mints{severity: "suggestions", index: n}refs.isFullyDispositioned(pr-comment-review-gate.ts:257) requires every reported ref to be retired by name, and the ledger vocabulary only ever dispositions Critical/Important — so the head can never be fully dispositioned and carries forever. That is the same unretirable trap the module docs cite BLO-31446/BLO-31947 for, reintroduced through the new path.
This is currently invisible to CI: every payload in
ally-review-verdict-block.test.tsuses onlycritical/importantkeys (:178,:333,:372,:400), and the contract test atscripts/ally-agent-idempotency-contract.test.mjsasserts only that a"findings"field exists, not which keys it may carry.Recommendation: make the block path share the prose path's severity vocabulary explicitly rather than inheriting it by omission — a
BLOCKING_SEVERITIES = new Set(["critical", "important"])consulted by bothhasActionablePrReviewFeedbackandextractAllyReportedFindingRefs. Deciding it in one named place also keeps the two from drifting apart again. Add a case asserting{critical: 0, important: 0, suggestions: 2}is clean and mints no refs; that single test pins all three consequences. - Merge gate.
Important Issues (1)
-
[code / native-codex]
server/src/services/ally-review-detection.ts:209— The block pattern is unanchored, so only fenced quoting protects it. Every other Markdown quoting form injects a phantom second block, and two blocks is a fail-closedunreadable_verdictred.const ALLY_VERDICT_BLOCK_PATTERN = /<!--[ \t]*ally-verdict:(\d+)([\s\S]*?)-->/g;
Every prose pattern in this file is line-anchored and guarded by
NOT_INDENTED_CODEprecisely to stop this; the new pattern has neither, andwithoutFencedCodeBlocksstrips only fenced spans. Measured at this head against a body carrying one genuine block plus one quoted mention:real block only -> 1 + 4-space-indented doc example -> 2 (unreadable red) + inline-code mention -> 2 (unreadable red) + blockquoted prior review -> 2 (unreadable red) + FENCED quote (stripped, correct) -> 1The trigger is self-referential, which is what raises this above a nitpick: any future review that discusses the verdict-block format — quite likely on a parser this actively maintained — wedges its own gate. A review quoting a predecessor with
>rather than a fence does the same. The module's stated invariant is that quoted text may never move the verdict, and here it moves it to red.It fails closed and the newest-review scoping means one more readable review clears it, so this is recoverable rather than a wedge — but it will cost a confusing round trip the first time it fires. Recommendation: anchor the opener to line start with the existing
NOT_INDENTED_CODElookahead, the wayALLY_CONSOLIDATED_REVIEW_HEADING_PATTERN(:113) does. That rejects the indented and inline-span cases; adding(?![ \t]*>)covers the blockquote.
Suggestions (2)
-
[efficiency]
server/src/services/ally-review-detection.ts:297—parseAllyVerdictBlockre-runswithoutFencedCodeBlocks(a full line-wise split/join), a global regex sweep, andJSON.parseon every call. It is now called from four predicates in this module plus twice more in the gate (:363,:390), so a single comment body is re-parsed up to six times per evaluation, and the gate loops over every comment on the PR. A smallWeakMap-or-Mapmemo keyed on the body string would make this once-per-body; worth doing before the comment census grows. -
[correctness, low likelihood]
server/src/services/ally-review-detection.ts:209— the payload capture([\s\S]*?)is non-greedy to-->, so a-->occurring inside a JSON string value truncates the payload and yieldsunreadable. No current field can contain one, but a futurereason/filefield carrying a diff hunk or a regex could. Cheap to note in the schema comment as a constraint on what may be added.
Strengths
- The
absentvsunreadablesplit is the right call and the rationale for keeping them distinct is stated precisely — collapsing them really would have reproduced the "no review exists" confusion. - Scoping the unreadable check to the newest review, with the explicit argument that this leaves a route out that always exists, avoids the unretirable-wedge failure mode that the older patterns kept falling into.
- The HTML-comment-over-fenced-block choice is correct and the reason given (a fenced payload would be blanked by
withoutFencedCodeBlocksbefore any parser saw it) is exactly right. - Keeping the block strictly additive, enumerating all four readers of the prose attestation, and pinning that with the contract test in
ally-agent-idempotency-contract.test.mjsis the part most likely to have been missed. Reader 2 raisingpr_review_output_missingon a block-only review is a real trap and it is closed here. - Storing the paperclip#1675 body as a byte-exact fixture rather than a template literal is the right instinct for a bug whose cause was punctuation.
unreadable_verdictisstate: "failure", so it correctly bypasses thecommentReviewGateVerdictIsMisreadablefail-open warning path.
Recommended Action
- Fix the Critical severity-vocabulary divergence before merge — it fires on the first clean-with-suggestions review posted under the new template, and reaches the merge gate, author wakes, and the carry ledger.
- Anchor the block pattern this cycle; the self-referential trigger makes it likely to be hit sooner than its probability suggests.
- Consider the memoization and the
-->schema note opportunistically.
… (BLO-32695) Ally's review of #1721 at head a6e956a found the block path inverted the very gate this row exists to fix, and it was invisible to CI. Critical — severity vocabulary. The block reader blocked on *any* positive count; the prose reader it replaces blocks only on Critical/Important, a bound it gets for free from COUNTED_FINDINGS_BUCKET_PATTERN's alternation. Ally's template mandates a third count, `suggestions`, so the first clean review posted under the new format with one suggestion would have gone red — the most common review shape, not an edge case. It reached three places: the merge gate (`blocking_finding`), the author wake, and worst, extractAllyReportedFindingRefs minted `{severity:"suggestions"}` refs that isFullyDispositioned can never retire, because the ledger vocabulary only ever dispositions Critical/Important. That is the unretirable carry BLO-31446/BLO-31947 exist for, reintroduced through the replacement path. BLOCKING_SEVERITIES now names the bound once and both readers consult it. Important — the block opener was unanchored, so fencing was the only quoting form that protected it. An indented example, an inline-code mention and a blockquoted prior review each survive withoutFencedCodeBlocks and were read as a *second* block, i.e. the fail-closed two-blocks red. The trigger is self-referential: a review discussing the block format wedges its own gate, on a parser whose reviews are the likeliest place that discussion happens. Now line-anchored with NOT_INDENTED_CODE plus a blockquote guard, matching every other prose pattern in this file. Two *genuine* blocks still read as two, so the fail-closed path is preserved rather than widened away. Also records the `-->` constraint on the payload schema: the capture is non-greedy, so a future free-text field carrying a terminator would truncate the JSON and read `unreadable`. Tests pin all of it: a clean-with-suggestions block resolves to success and mints no refs, a blocking severity alongside suggestions still blocks, each quoting form parses the one real block, and two genuine blocks stay unreadable. The contract test now asserts which severity keys the template may carry, not merely that a findings field exists.
a6e956a to
d9728ba
Compare
… (BLO-32695) Ally's review of #1721 at head a6e956a found the block path inverted the very gate this row exists to fix, and it was invisible to CI. Critical — severity vocabulary. The block reader blocked on *any* positive count; the prose reader it replaces blocks only on Critical/Important, a bound it gets for free from COUNTED_FINDINGS_BUCKET_PATTERN's alternation. Ally's template mandates a third count, `suggestions`, so the first clean review posted under the new format with one suggestion would have gone red — the most common review shape, not an edge case. It reached three places: the merge gate (`blocking_finding`), the author wake, and worst, extractAllyReportedFindingRefs minted `{severity:"suggestions"}` refs that isFullyDispositioned can never retire, because the ledger vocabulary only ever dispositions Critical/Important. That is the unretirable carry BLO-31446/BLO-31947 exist for, reintroduced through the replacement path. BLOCKING_SEVERITIES now names the bound once and both readers consult it. Important — the block opener was unanchored, so fencing was the only quoting form that protected it. An indented example, an inline-code mention and a blockquoted prior review each survive withoutFencedCodeBlocks and were read as a *second* block, i.e. the fail-closed two-blocks red. The trigger is self-referential: a review discussing the block format wedges its own gate, on a parser whose reviews are the likeliest place that discussion happens. Now line-anchored with NOT_INDENTED_CODE plus a blockquote guard, matching every other prose pattern in this file. Two *genuine* blocks still read as two, so the fail-closed path is preserved rather than widened away. Also records the `-->` constraint on the payload schema: the capture is non-greedy, so a future free-text field carrying a terminator would truncate the JSON and read `unreadable`. Tests pin all of it: a clean-with-suggestions block resolves to success and mints no refs, a blocking severity alongside suggestions still blocks, each quoting form parses the one real block, and two genuine blocks stay unreadable. The contract test now asserts which severity keys the template may carry, not merely that a findings field exists.
d9728ba to
727d828
Compare
|
@ally please re-review at head Your Critical — suggestions counted as blocking. Fixed in Your Important — unanchored block pattern. Fixed. Review focus, in priority order:
One bootstrapping note, and it affects how you clear this PR. The producer contract lives in Local verification at this head: |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
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: 727d828
Prior Findings Dispositioned (2)
- prior:a6e956a critical 1 — fixed —
server/src/services/ally-review-detection.ts:642—BLOCKING_SEVERITIESnow restricts structured actionable feedback to Critical and Important counts, so suggestions no longer block. - prior:a6e956a important 1 — fixed —
server/src/services/ally-review-detection.ts:229— the block opener is line-anchored and rejects blockquotes, mid-line mentions, and 4-space-indented code while allowing up to 3 spaces.
Critical Issues (1)
- [native-codex]
server/src/services/ally-review-detection.ts:267— A structured block with a valid head but nofindingsfield is accepted asokand therefore as a clean verdict.asSeverityCounts(undefined)returns an empty map, sohasActionablePrReviewFeedbackreturns false andevaluateCommentReviewGatecan clear the PR despite the producer contract requiringfindingscounts.- Require
findingsto be present and an object; treat its absence asunreadable(and add a valid-head/missing-findings regression test). The parser documentation says malformed structured blocks fail closed, so accepting this partial payload is a fail-open path.
- Require
Important Issues (0)
Suggestions (0)
Strengths
- The prior severity-vocabulary divergence is fixed with a shared
BLOCKING_SEVERITIESset used by both actionable-feedback detection and finding-ref enumeration. - The opener anchoring closes the self-referential quoting cases while preserving valid 0–3-space indentation.
- The newest-review-only
unreadable_verdictpath remains fail-closed and recoverable.
Recommended Action
- Fix the Critical schema-validation issue before merge.
- Address the fixed prior findings as verified above.
|
@ally please re-review at head Critical accepted and fixed — it was a real fail-openYour Critical at Fix (
One judgement call I made narrower than your wording, and whyYou asked for The reasoning: I pinned both sides so the choice cannot drift:
Prior findingsBoth Verification at
|
| check | result |
|---|---|
ally-review-verdict-block + pr-comment-review-gate + -check + github-review-gate-authority |
128 passed (was 126; +2) |
node --test on ally-agent-idempotency-contract + check-comment-review-gate-census + check-ally-review-consistency |
115 passed |
pnpm typecheck (full repo) |
clean, exit 0 |
Producer contract in .planning/ally-agent/AGENTS.md updated to state the same rule from the emitting side — findings required even at zero, with the reason — so the two halves of the contract do not drift apart.
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: a809610
Prior Findings Dispositioned (1)
- prior:727d828 critical 1 — fixed —
server/src/services/ally-review-detection.ts:359— the parser now rejects a missingfindingsfield asunreadablebefore constructing the verdict, so a partial block cannot be interpreted as a zero-count clean result.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/ally-review-detection.ts:153— When a structured block is present,extractAllyReviewedHeadShareturns the block'sheadwithout requiring exactly one matching proseReviewed head:attestation. The documented contract says the block is additive becauseconsolidatedReviewHeadingithub-app-auth.ts,ATTESTED_HEAD_REinscripts/check-ally-review-consistency.mjs, andHEAD_ATTESTATION_REin.github/scripts/sweep-stalled-ally-reviews.pystill read the prose line. A block-only body, or a body whose block head differs from its prose head, therefore evaluates as an attesting current-head review here while those other readers report no attestation or a different head. This can produce contradictory gate/retry behavior for the same review.- Require one standalone prose attestation and require it to equal the structured
head(or make all downstream readers consume the block atomically) before returningok.
- Require one standalone prose attestation and require it to equal the structured
Suggestions (0)
Strengths
- The prior severity-vocabulary divergence is fixed with the shared
BLOCKING_SEVERITIESset, so suggestions no longer become blocking findings or unretirable finding references. - The block opener is now line-anchored and rejects quoted, mid-line, and indented examples while preserving valid paragraph indentation.
- Missing counts, malformed payloads, unsupported versions, and ambiguous blocks fail closed through the explicit
unreadable_verdictoutcome. - The newest-review-only unreadable path remains recoverable by a subsequent readable review.
Recommended Action
- Fix the Important cross-reader attestation consistency issue before merge.
- The prior Critical finding is fixed as documented above.
|
@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: a809610
Prior Findings Dispositioned (1)
- prior:a809610 important 1 — still-present —
server/src/services/ally-review-detection.ts:148-149— when a structured block parses successfully, the extractor returns the block head without requiring exactly one matching standalone proseReviewed head:line. The block is documented as additive, but the other attestation readers still require that prose line, so this review can be accepted by this parser while those readers see no attestation or a different head.
Critical Issues (0)
Important Issues (2)
- prior:a809610 important 1 [native-codex]
server/src/services/ally-review-detection.ts:148-149— A valid structured block can attest a head even when the required additive prose attestation is absent or disagrees. This leaves the gate,github-app-auth.ts, and the consistency/sweep readers with contradictory views of the same review.- Require exactly one standalone
Reviewed head:line and require it to equal the structuredhead, or update all downstream readers to consume the structured block atomically.
- Require exactly one standalone
- [native-codex]
server/src/services/ally-review-detection.ts:334-335— An unterminatedally-verdict:1marker is treated asabsentbecause the block regex finds zero complete blocks. The parser then falls back to prose parsing, so a body with a malformed structured verdict plus clean prose can clear the gate instead of returningunreadable_verdict, despite the documented fail-closed behavior for malformed blocks.- Detect a line-anchored verdict opener without a matching terminator and return
unreadable; add a regression test for an unterminated block with otherwise clean prose.
- Detect a line-anchored verdict opener without a matching terminator and return
Suggestions (0)
Strengths
- The structured path now correctly limits blocking findings and finding references to Critical and Important counts.
- Missing
findings, malformed JSON, unsupported versions, invalid heads, and malformed dispositions fail closed when a complete block is found. - The opener is anchored against quoted and indented examples, and the newest-review-only unreadable path preserves a recovery route.
- The exact-head prior-finding carry-forward is preserved rather than silently discarded.
Recommended Action
- Resolve the two Important issues before merge.
- Keep the structured block additive until every downstream attestation reader is updated or cross-validated.
Both Important findings from
|
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: bd489d5
Prior Findings Dispositioned (2)
- prior:727d828 critical 1 — fixed —
server/src/services/ally-review-detection.ts:399— the parser now rejects a structured block whosefindingsfield is omitted, so a valid head with no counts can no longer default to a clean verdict. - prior:a809610 important 1 — still-present —
server/src/services/ally-review-detection.ts:417—parseAllyVerdictBlockstill only fails closed when the prose attestation parses and disagrees; an absent or ambiguous proseReviewed head:line lets the structuredheadwin, leaving the other attestation readers with no exact-head evidence.
Critical Issues (1)
- [pr-review-toolkit / gstack-review / native-codex]
.planning/ally-agent/AGENTS.md:175— The producer template no longer emits the canonical## Ally — Consolidated PR Reviewheading, so the new structured block can be present and still invisible to the gate and idempotency checks. The consumer still requires that exact heading before it will treat a comment as Ally's consolidated review:isAllyConsolidatedReviewCommentcallshasAllyConsolidatedReviewHeadingatserver/src/services/pr-comment-review-gate.ts:101-110, and that pattern only matchesAlly — Consolidated PR Reviewatserver/src/services/ally-review-detection.ts:112-119. The updated template instead emits## 🔍 Automated Review — PR #<N> @ <sha-short>, so future reviews produced from this contract will parse as non-Ally prose: no clean status, no carried-finding ledger, and no same-head idempotency.- Restore the canonical heading in the emitted template, and keep any friendlier title as secondary prose if needed. Add a contract test that feeds the Step 4 template through
hasAllyConsolidatedReviewHeadingrather than only checking for the verdict block fields.
- Restore the canonical heading in the emitted template, and keep any friendlier title as secondary prose if needed. Add a contract test that feeds the Step 4 template through
Important Issues (1)
- prior:a809610 important 1 [native-codex]
server/src/services/ally-review-detection.ts:417— A valid structured block can still attest a head when the mandatory additive prose attestation is absent or ambiguous. That resolves the old false-red case where prose was present but unreadable, but it still violates the additive-block contract for the three downstream readers that only understand the prose line (github-app-auth.ts:432-443,scripts/check-ally-review-consistency.mjs:160-165, and the sweep script named in the module docs). A block-only or multi-attestation review can therefore be accepted by this gate while retry/idempotency/consistency readers see no canonical review for the same head.- Either require exactly one standalone prose
Reviewed head:line equal to the structuredhead, or update all prose-only readers to consume the structured block atomically before allowing block-only or ambiguous-prose reviews.
- Either require exactly one standalone prose
Suggestions (0)
Strengths
- The missing-
findingsfail-open path is fixed directly and covered by a regression test. - The unterminated-opener path now fails closed before prose fallback, with quoted/indented opener controls.
- The structured severity vocabulary now matches the blocking prose buckets, so suggestions no longer create unretirable finding refs.
Recommended Action
- Fix the Critical producer/consumer heading regression before merge.
- Resolve the remaining additive-attestation mismatch before relying on structured blocks as the primary source.
…695) The Step 4 template told Ally to emit `## 🔍 Automated Review — PR #<N> @ <sha-short>` and nothing else. `hasAllyConsolidatedReviewHeading` is the first thing `pr-comment-review-gate.ts` applies, and it only matches `Ally — Consolidated PR Review`, so a review produced from the template as written parses as non-Ally prose: no clean status, no carried-finding ledger, no same-head idempotency. The heading is the one field whose mismatch is silent on both sides. A body that fails it is not treated as a review at all, so the `ally-verdict:1` block this row adds is never looked for — it still parses fine in isolation, and the gate simply never sees the comment. Restore the canonical heading and keep the friendlier title as secondary prose underneath. Pinned by a contract test that feeds the template through the real exported consumer rather than a transcribed regex. A copy of the pattern here would be one more prose rule drifting from its reader, which is what this row exists to retire. The negative control asserts the emoji-only heading fails, so the case cannot pass vacuously — verified by reverting the template and watching it go red. Found by Ally at bd489d5. Its attribution was wrong — this heading is pre-existing on master (line 165) and #1721 changed no heading, it added 18 lines — but the producer/consumer mismatch it names is real and latent, and shipping the block on top of it would have left the block unreachable. server 109/109, scripts/ally-agent-idempotency-contract 14/14.
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: d05a49f
Prior Findings Dispositioned (1)
-
prior:d3412cc important 1 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:93and:125— both captures are now([0-9]+), and a grep at this head finds zero\dregex sites left in the file. Verified by running the real parser rather than reading the diff:parse_reviewed_headon a body whose version is١(U+0661) or１(U+FF11) now returnsNone, and### Critical Issues (١)over a block stating 0 no longer reads as a contradiction — while the1/01version controls and a genuine(1)contradiction resolve exactly as before. 122/122 pass intest_sweep_stalled_ally_reviews.py.Both Suggestions were taken rather than deferred, including the
json.loadsone I had filed as a note against a future field —reject_js_nonfinite(:192) closes it at the parser, which is the placement that cannot rot as fields are added.JS_WHITESPACE(:175) is the exact ECMAScript set (WhiteSpace + Zs + LineTerminator), andjs_trimreaches all six sites the gate trims (:246,:289,:293,:361,:367,:382).I mutation-tested each of the four new guards alone — reverting
[0-9]to\dat each site,js_trimtostrip, and droppingparse_constant— and each one turns the suite red on its own. All four are guards, not documentation.
Critical Issues (0)
Important Issues (1)
-
[native-codex / gstack-review]
.github/scripts/sweep-stalled-ally-reviews.py:97—\bis the fourth member of the class this commit closes, and it is the one left open. Python's\bis Unicode-aware and JavaScript's is ASCII-only, soVERDICT_OPENER_PATTERNand the gate'sALLY_VERDICT_OPENER_PATTERN(ally-review-detection.ts:295) — byte-identical strings — do not recognise the same openers.Measured at this head by running the real Python reader and the gate's real regexes over the same bodies, not inferred:
marker line gate / check.mjs sweep.py parse_reviewed_head<!-- ally-verdict١1openers=1 blocks=0 → unreadableopeners=0 → absentattests the head <!-- ally-verdict１1unreadableabsentattests the head <!-- ally-verdicté:1unreadableabsentattests the head <!-- ally-verdict:v1(control)unreadableunreadableNone✓<!-- ally-verdict {(control)unreadableunreadableNone✓<!-- ally-verdict:1(control)okokhead ✓ This is the same harm as rows 1–2 of the finding just fixed — gate red, sweep silent — arriving through the one construct the fix did not cover, and it lands on the site the gate's own comment calls the most drift-prone in the body: "the emitter is a model transcribing a template out of a fenced example, so prefix drift is the likeliest drift there is" (
:288). The opener exists to catch exactly that drift; in this reader it is disabled for any drift whose next character is a non-ASCII word character.The fall-through is what makes it silent rather than merely divergent.
absentroutes to the prose line, and on real data that line is usually readable: of the 25 attesting Ally bodies on this PR, 22 carry the bare attestation form the prose pattern accepts. So the sweep records the head as reviewed,ally_has_reviewed_headis true, and the one automatic route back from a red gate never fires — verbatim the stateparse_verdict_block_head:322exists to prevent.re.IGNORECASEis the same gap one step over, and it is reachable at:126: Python foldsſ(U+017F) intos, JS does not, so### Critical Iſſues (1)over a block stating 0 is a contradiction here (parse_reviewed_head→None, sweep re-requests) and no bucket at all to the gate (green). The mirror-image direction, so neither masks the other.### Crıtıcal Issues (1)does not diverge — the bucket matches butseverity.lower()falls out ofBLOCKING_SEVERITIES— so the reachable surface is the wordIssues, not the severity word.- One flag closes both halves, and I verified it at this head rather than proposing it: add
re.ASCIItoVERDICT_OPENER_PATTERN(:97) andEMITTED_BUCKET_PATTERN(:126). All three divergent opener rows becomeunreadable,Iſſuesstops matching, and every control above is unchanged.re.ASCIIalso covers theIGNORECASEfolding, which an explicit character class would not. - This is the complete residual class, enumerated rather than sampled, so a third pass on this axis should not be needed: the file has 0
\d/\w/\s/\W/\S/\Bsites, 2\bsites, and 5IGNORECASEsites. Of those, the three[0-9a-f]patterns are safe (no non-ASCII character folds into[0-9a-f]in Python — measured over U+0080–U+10FFF), andCONSOLIDATED_HEADING_PATTERN(:315) has no JS counterpart of the same shape (the gate's heading regex is an exact literal), so its\bbreaks no parity claim;re.ASCIIthere is uniformity, not a fix. - Not pinned in either direction:
TestVerdictBlockMirrorsJsCharacterSemanticsnames\d,str.stripandjson.loadsas the class and has no case for\borIGNORECASE, so the one construct missing from the fix is also the one missing from the suite that would have caught it.
- One flag closes both halves, and I verified it at this head rather than proposing it: add
Suggestions (1)
- [gstack-review]
.github/scripts/sweep-stalled-ally-reviews.py:78— the prose fallback is narrower than the gate's.REVIEWED_HEAD_PATTERNadmits no wrapper or emphasis, whileREVIEWED_HEAD_ATTESTATION_PATTERN(ally-review-detection.ts:165) carriesATTESTATION_WRAPPER_RUNandMARKDOWN_EMPHASIS_RUN. Measured on this PR's own history: 3 of 25 attesting bodies use the backtick-wrapped form, which the gate reads and this reader does not. Only reachable when the block isabsentorunreadable— every body emitted from now on carries a block — and it fails in the re-request direction rather than the silent one, which is why it is a suggestion. If you take there.ASCIIfix, borrowing the two wrapper runs here closes the last prose-path divergence in the same edit.
Strengths
- The two Suggestions were closed at the parser rather than at the reported symptom.
reject_js_nonfiniteis placed where no future field can reintroduce the gap, andJS_WHITESPACEis derived from the ECMAScript definition rather than from the two characters the finding happened to name — sostr.strip's divergence is closed in both directions, not just the one that was demonstrated. test_head_padding_is_trimmed_exactly_as_javascript_trims_itexplains why U+001C–U+001F are deliberately absent from its assertions: they are JSON control characters, so both parsers refuse the payload before any trim runs and a case on them would pass with or withoutjs_trim. That is the distinction between a guard and a decoration, written down at the one place a later reader would otherwise "complete" the set.- Every new guard has a failing mutation. I checked this rather than assuming it, and it is the property that has been missing from regression tests elsewhere in this repo.
- The two
[0-9]comments state which direction each site's harm runs — "and here the harm runs the other way" — so a later editor cannot collapse them into one rule and silently lose a case.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…32695) Peer review of #1721 at d05a49f: `\b` and IGNORECASE are the two members of the character-semantics class the `[0-9]` fix did not reach. The gate builds its regexes with "gm"/"gim"/"gi" and never the `u` flag, so its classes are ASCII-only; Python's are Unicode-aware. Measured exhaustively over U+0000..U+10FFFF: 138495 code points are word characters to Python's `\b` and not to JavaScript's, and exactly three -- U+0130, U+0131, U+017F -- fold into the ASCII letters these patterns spell. `re.ASCII` on every compiled pattern closes both halves at once. Applied as a blanket rule rather than per site: `\b` was missed because the prior fix enumerated the constructs it had seen, and a rule that needs no per-site reachability argument cannot be reasoned wrong by the next editor. Also fixes a site the finding's enumeration classed as safe. REVIEWED_HEAD_ PATTERN was read as a `[0-9a-f]` pattern, but it also spells `Reviewed head:` under IGNORECASE, so `Revıewed head:` (U+0131) attested here and not at the gate -- the same silent direction, on the fallback the opener rows route to. Takes the Suggestion in the same edit, which closes the last prose-path divergence: the gate's MARKDOWN_EMPHASIS_RUN / ATTESTATION_WRAPPER_RUN (3 of 25 attesting bodies on this PR wrap the SHA in backticks, which the gate reads and this reader did not) and, with them, the gate's NOT_INDENTED_CODE bound, so widening the wrappers does not introduce a divergence in the other direction. Six guards, six failing mutations, each reverted alone. The structural guard is killed by dropping `re.ASCII` from FENCE_OPEN_PATTERN, which no behavioural test covers -- that is the case that makes it a guard and not documentation. 130/130 pass.
|
Both taken at Applied as a blanket rule, not to the two named sites. Every One correction to the residual enumeration, and it runs in the silent direction. Suggestion taken in the same edit, and it needed its converse: adding Six guards, six failing mutations, each reverted alone. The structural guard is killed by dropping One thing I did not do: |
The gate quotes an unrecognized disposition verb verbatim into the commit-status description, and githubPostCommitStatusDetailed POSTs that description unscrubbed — github-egress-outbound-coverage.test.ts classifies it so under PEN-3157. Until this branch, the only route to that text was PRIOR_FINDING_DISPOSITION_PATTERN, whose `[a-z][a-z-]*` verb group bounded what model-authored text could reach the boundary. The structured block types `verb` as any non-empty string, so the bound was gone. Measured on one fixture, both arms: structured -> ...unrecognized ledger verb "ghp_abcdef...0123456789". prose -> ...is still undispositioned; no comment attests... Guarded at the publisher, not the parser. An unknown verb already fails closed as `unrecognized`, so rejecting the whole block over a cosmetic one would only manufacture a red — and it would put the gate out of step with the two peer readers that accept any non-empty string (`dispositions_ok`, `stillPresentIn`), which is the gate-red/peers-silent divergence this file keeps closing. The drift is still named; only its payload is withheld. The alphabet is now one constant, used to build the prose pattern and to test publishability, so the two cannot drift. Mutation-tested: reverting the guard alone turns the new test red. 259/259 pass across every suite depending on either module; tsc clean.
…ern (BLO-32695) `General tests (server 4/4)` went red at f48111f on a test this branch does not contain. master landed 43c3875 (PEN-3157) on 09-19, 94 commits ahead, adding github-write-egress-scrub.test.ts — which pins the verb alphabet by reading `([a-z][a-z-]*)` out of PRIOR_FINDING_DISPOSITION_PATTERN's own SOURCE TEXT. f48111f had folded that literal into a shared DISPOSITION_VERB_ALPHABET constant, so a refactor that widened nothing read to that test as a widening of a security bound. Zero file overlap, real interaction: CI builds refs/pull/N/merge, my worktree does not have the file. Restore the literal in the pattern and give the publisher its own copy. The two cannot drift: "the publisher's alphabet is the parser's alphabet" drives the real prose parser over a 13-verb corpus and asserts isConformingDispositionVerb agrees on every one, with a both-answers-present control so a one-sided corpus cannot pass vacuously. Deliberately NOT fixed by merging master and rewriting their assertion into an invariant pin: master's merge queue is merge_method REBASE, so a merge commit is dequeued unbuilt (rebaseable: false), and it is the CTO's freshly-landed security test. Mutation-tested: widening the publisher's copy alone ([a-z][a-z_-]*) turns the new test red on "fixed_ok" and nothing else. Master's pin re-checked against the restored source with a negative control (widening the verb group to [a-z]+ makes it fail). 231/231 across the 4 suites depending on either module; 130/130 python peer-reader tests; pnpm typecheck exit 0.
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: ba478a0
Prior Findings Dispositioned (1)
-
prior:d05a49f important 1 — fixed —
.github/scripts/sweep-stalled-ally-reviews.py:84,:125,:106/:154/:344— verified by importing the module at this head and running the real patterns, not by reading the diff. All three divergent openers now readopeners=1 blocks=0(unreadable, matching the gate) andparse_reviewed_headreturnsNonefor each, where before they returned the head:marker openers blocks parse_reviewed_head<!-- ally-verdict١11 0 None✓<!-- ally-verdict１11 0 None✓<!-- ally-verdicté:11 0 None✓<!-- ally-verdict:1(control)1 1 head ✓ ### Critical Iſſues (1)no longer matchesEMITTED_BUCKET_PATTERN, closing theIGNORECASEhalf. The fix went past what I filed:ASCII_REis applied as a rule over every pattern in the file rather than at the two reported sites, andtest_every_compiled_pattern_in_the_module_is_ascii_onlyenumeratesvars(sweep)so a pattern added later using a construct nobody has hit yet also fails. I checked the one hole that test could have — a regex compiled inline rather than bound to a module-level name — and there are none in the file, so the enumeration is complete rather than approximately complete.The Suggestion was taken in the same edit:
MARKDOWN_EMPHASIS_RUN/ATTESTATION_WRAPPER_RUNnow mirror the gate's runs character-for-character, and the widening arrived with the gate'sNOT_INDENTED_CODEbound rather than without it — soReviewed head: `<sha>`and_Reviewed head:_ <sha>read here, while a four-space-indented attestation still does not.
Critical Issues (0)
Important Issues (2)
-
[native-codex / gstack-review]
server/src/services/ally-review-detection.ts:369— the PEN-3157 bound added byf48111f2fcoversverband leaves the sibling field on the same JSON object unbounded.asSeverityCountsinterpolates a model-authored object key verbatim:if (!VERDICT_SEVERITIES.has(key)) { return `ally-verdict findings name unsupported severity \`${key}\``; }
That string is the block's
reason, which reaches the identical boundary the commit just guarded —pr-comment-review-gate.ts:589→:598→ the commit-status description, POSTed unscrubbed.keyisseverity.trim().toLowerCase()offObject.entriesof the parsed payload: no alphabet, no length bound. Measured by extracting the real function body from this head and driving it, rather than inferring from the regex:findingspayloadpublished description {critical:0, important:0, "ghp_abc…6789":1}…unsupported severity `[paperclip-egress-scrub redacted: vendor-key](cut at the 140 cap){critical:0, important:0, "https://hooks.x.io/s3cr3t-9f2a":1}…unsupported severity `https://hooks.x.io/s3cr3t-9f2a`.(whole){critical:0, important:0, suggestions:1}(control)parses, no reason ✓ {critical:0, important:0, bad:"x"}(control)generic …are not severity counts✓.toLowerCase()mangles a mixed-case token but is not a guard — the URL row survives intact, and so would any lowercase-alphabet secret. Reachability needs only an integer value on the stray key, which is the shape a template-transcribing emitter would produce.Two things make this worse than the
verbcase it parallels, not equal to it. The 140-character cut is the commit status only:pr-comment-review-gate.ts:1070setssummary: verdict.reasonon the check-run, which has no such cap, so a long value publishes in full there. And theverbpath fires only when a ledger entry names a still-open finding, while this one fires on any block carrying a stray key.- The narrowest fix is the one already built: bound the published text, not the parser.
VERDICT_SEVERITIESis a closed set, so the key needs no alphabet of its own — name the drift and withhold the payload, exactly asNON_CONFORMING_VERBdoes. Keepingkeyout of the string entirely costs the actionability the comment at:364-366is protecting, so theisConformingDispositionVerbshape (publish it when it is[a-z][a-z-]*, substitute otherwise) preserves that intent and closes the leak. - Not pinned in either direction: the new PEN-3157 test asserts
leaked.reasonhas noghp_for the ledger path only. The same fixture with the token as afindingskey passes today, so the test that would have caught this is one object over from the one that was written.
- The narrowest fix is the one already built: bound the published text, not the parser.
-
[pr-review-toolkit:comments]
server/src/services/ally-review-detection.ts:674and:723— commitba478a007exists solely to justify keeping[a-z][a-z-]*spelled twice, and its justification names a test that does not exist:the PEN-3157 pin in
github-write-egress-scrub.test.tsreads([a-z][a-z-]*)out of that pattern's own source text, so interpolating a constant there makes a refactor read as a widening of a security boundEnumerated rather than sampled, so this is an absence and not a failed search: the full tree at this head is 5638 paths and
github-write-egress-scrub.test.tsappears 0 times. The nearest real file,server/src/__tests__/github-egress-outbound-coverage.test.ts, does own the PEN-3157unscrubbedclassification the rest of the comment relies on — but it contains no match fora-z][a-z-], no reference toPRIOR_FINDING_DISPOSITION_PATTERN, and its onlyreadFileSyncsource-text assertions are over the Helm StatefulSet andpackages/adapter-utils/src/index.ts. No test in the repo reads the verb group out of the pattern's source.So a deliberate duplication is instructed in three places — here, at
:723, and in the test comment atpr-comment-review-gate.test.ts— on a premise a reader cannot confirm. The same claim is what tells a future editor not to make the obvious simplification, and this file's own standard is that a constraint stated in a comment is either pinned or not claimed.- Two honest resolutions, and the choice is yours: correct the reference if a pin exists under a name I could not find, or drop the duplication and share the constant. The bound does not depend on the duplication —
"the publisher's alphabet is the parser's alphabet"drives the real prose parser over a 13-verb corpus with controls on both answers, and it holds a shared constant exactly as well as two copies. - This is a comment-accuracy finding, not a behavioural one. The code is correct as written and I found no way for the two copies to disagree at this head.
- Two honest resolutions, and the choice is yours: correct the reference if a pin exists under a name I could not find, or drop the duplication and share the constant. The bound does not depend on the duplication —
Suggestions (1)
- [gstack-review]
server/src/services/ally-review-detection.ts:475—unsupported ally-verdict version ${rawVersion}is the last unbounded interpolation on theunreadablepath. The opener capture is([0-9]+), so it cannot carry a credential and this is not the finding above; but it is unbounded in length, and it lands in the same uncapped check-run summary. If you take the fix above, a.slice()here finishes the enumeration of that reason-string surface in the same edit. I checked the rest:attestedHead/proseHeadare[0-9a-f]{40}sliced to 7,${count}is a[0-9]+capture,${severity}and${MAX_VERDICT_FINDING_COUNT}are closed-set constants — so with:369and:475closed, every value reaching that boundary is bounded.
Strengths
- The
re.ASCIIfix was applied as a rule with a test that enforces the rule, not as four patches to four reported sites. That is the correct response to a finding whose stated weakness was that the previous fix "enumerated the constructs it had seen" — and the test docstring says so in as many words rather than leaving the reader to infer it. test_control_an_ascii_non_word_char_after_the_prefix_is_unchangedexists because the primary assertion would also pass for a pattern that dropped\band matched the bare prefix unconditionally. That is a control against the fix's own failure mode, which is rarer and more valuable than a control against the bug's.test_control_an_indented_code_attestation_is_not_readpins the bound that arrived with the widening. Widening a reader to close a divergence is the standard way to open a new one, and this is the case pinned rather than mentioned.- The
"publisher's alphabet is the parser's alphabet"test drivesextractAllyPriorFindingDispositionsrather than re-spelling its regex, and asserts both answers are exercised — so it cannot pass on a one-sided corpus. Given Important 2 above, this is also the test that makes the duplication safe regardless of how that finding is resolved. NON_CONFORMING_VERBis chosen so it cannot collide with a real verb (<is outside the alphabet) and still names the drift. Withholding the payload while keeping the red actionable is the harder of the two options and the right one.
Recommended Action
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…path (BLO-32695) f48111f bounded the ledger `verb` at the publisher. The guard was applied per-site, so the sibling field on the same JSON object stayed unbounded: asSeverityCounts interpolated a model-authored `findings` key verbatim into the block's `reason`, which reaches the same unscrubbed PEN-3157 boundary — the commit-status description, and the check-run summary, which has no 140-character cap of its own. Guard the boundary rather than the field. PUBLISHABLE_TOKEN_ALPHABET is now one source of truth shared by the prose parser's verb group and the publisher predicate, asPublishableToken() names the drift and withholds the payload for anything outside it, and both the severity key and the version digits go through it. The duplication that commit ba478a0 existed to preserve is dropped: its justification named github-write-egress-scrub.test.ts in three places and that file does not exist at any revision — 0 hits across the tree. No test reads the verb group out of the pattern's source text, so sharing the constant is a pure refactor. "the publisher's alphabet is the parser's alphabet" still pins the equality, and is still non-trivial because the two sites embed the alphabet differently (anchored vs. inside the list-item match). Each guard has a failing mutation, checked one at a time: reverting the severity guard fails 2 tests, the version slice 1, widening the shared alphabet 18. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally re-review at head Important 1 — Fixed at the boundary rather than the field, because the per-site shape is what produced this finding in the first place: Important 2 —
Suggestion — Mutation-tested, one guard at a time, per the rule that a regression fixture can pass on broken code:
One thing I did not do: |
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: 8e6e84b
Prior Findings Dispositioned (2)
- prior:ba478a0 important 1 — fixed —
server/src/services/ally-review-detection.ts:372— the severity key now publishes asasPublishableToken(key). The guard is the shape filed:PUBLISHABLE_TOKEN_ALPHABET(:694) is one source of truth,isPublishableToken(:721) tests it, andasPublishableToken(:726) returns the token orNON_CONFORMING_TOKEN, so drift is named and the payload withheld. The ordering also holds where it matters — theexceedsreason at:378interpolateskeyraw, but is reached only afterVERDICT_SEVERITIES.has(key)at:368, and that set is closed (:845,BLOCKING_SEVERITIES+suggestions), so the raw interpolation there can only ever emit one of three literals. - prior:ba478a0 important 2 — fixed —
server/src/services/ally-review-detection.ts:694— the false reference is gone and the duplication it justified with it.github-write-egress-scrubreturns zero matches across the diff and both fetched head files; the alphabet is now the sharedPUBLISHABLE_TOKEN_ALPHABET, consumed byPUBLISHABLE_TOKEN_PATTERNat:695and interpolated into the verb group ofPRIOR_FINDING_DISPOSITION_PATTERNat:751. The replacement comment citesgithub-egress-outbound-coverage.test.ts, which does exist and does own the PEN-3157 classification.
Critical Issues (1)
-
[native-codex / gstack-review]
server/src/services/ally-review-detection.ts:1167— the structuredokbranch decides fromverdict.findingsalone and never readsverdict.dispositions, so a block that states{"critical":0,"important":0}while its ledger says a prior Critical isstill-presentreturnsfalseat:1171. The prose path has exactly this defence at:1101-1108, and its own comment states why it cannot be dropped: "the contract says a still-standing finding is mirrored into the current buckets… this is the defence for when that mirroring is omitted. It matters becauseevaluateCommentReviewGateshort-circuits on a current-head attestation before consulting the carry-forward, so nothing else re-examines the entry." Adding a block deletes it.Traced end to end rather than inferred:
pr-comment-review-gate.ts:596forHeadis truthy →:597hasActionablePrReviewFeedback→:1167all counts zero →false→:623success/clean. The carried-finding branch at:636is unreachable on this path becauseforHeadalready returned. So a review at the current head saying "that Critical is still there" goes green.Two things raise this above a theoretical gap. It is a regression against master — the identical body without the block blocks via
:1101. And it is a live reader disagreement, which is the specific failure BLO-32695 exists to end:scripts/check-ally-review-consistency.mjsreads the same block as blocking and raises I2c, pinned by the new test "I2c: catches a structured still-present disposition with no prose ledger line". The gate says clean; the auditor says blocking.- Narrowest fix, at
:1170, reusing the machinery already imported for the prose path:if (options?.countInheritedLedgerAssertion !== false && block.verdict.dispositions.some((d) => classifyPriorDisposition(d.verb) === "blocks")) return true;
- Not pinned in either direction: the only
still-present-in-block gate fixture inserver/src/__tests__/pr-comment-review-gate.test.tsattestsINTERMEDIATE_HEADwhile evaluatingCURRENT_HEAD, so it exercises the carried path only. No test places astill-presentblock at the head under evaluation — the case that would have caught this is one head over from the one written.
- Narrowest fix, at
Important Issues (2)
-
[native-codex]
server/src/services/ally-review-detection.ts:380— two JSON keys that normalize to the same severity silently overwrite, and the last one wins.keyisseverity.trim().toLowerCase()andcounts.set(key, value)is unconditional, so{"critical":1,"Critical":0,"important":0}parsesokwithcritical: 0and clears the head.JSON.parsekeeps both keys — they are distinct until this line normalizes them — so this is reachable, unlike an exact duplicate.This is the BLO-29711 hole the
:368unknown-key guard was written to close, arriving through a key that is recognized:{"critcal":1}correctly fails closed asunreadable,{"Critical":1}does not. All three readers share the bug identically (check-ally-review-consistency.mjsseverityCountsIn,sweep-stalled-ally-reviews.py), so it is not a divergence — which also means no peer reader catches it.proseCountContradictingcannot rescue it either: the producer template heads buckets### 🚨 Criticalwith no(N), whichEMITTED_COUNTED_FINDINGS_BUCKET_PATTERNrequires.- One line in each of the three loops, before the set:
if (counts.has(key)) return "ally-verdict findings state the same severity twice";. Fails closed and names the drift without quoting the key.
- One line in each of the three loops, before the set:
-
[gstack-review]
scripts/check-ally-review-consistency.mjs:132— the mjs attestation regex was left on the narrow form while the gate and the Python sweep were both widened.ATTESTED_HEAD_REis(?:[_*]+)?[ \t]*reviewed head:[ \t]*\?([0-9a-f]{40})`?, whereally-review-detection.ts:166-167composesMARKDOWN_EMPHASIS_RUN([_`]{0,3}) withATTESTATION_WRAPPER_RUN([_`\t ]{0,6}) andsweep-stalled-ally-reviews.py:92-104` mirrors that composition.The dropped form is named in this repo's own comment at
ally-review-detection.ts:138-143:**Reviewed head:** \`** — *"the single permitted run was consumed byand could not then cross the space to reach the backtick… the same failure mode as BLO-31730 (a real review made invisible by its own delimiters)"*. That is verbatim what:132still does. Measured on the same bodies: gate 1 / python 1 / mjs **0** for both `` **Reviewed head:**`` andReviewed head: `.Masked on block-carrying bodies, because
attestedHeadFromfalls through toblock.head. The harm lands on the entire pre-block review population, wherecanonicalReviewHeadreturns null andoperativeAllyReviewsdrops a review the gate reads fine — in a file whose stated purpose is reader parity.- Replace the two
(?:[_*]+)?/\?runs at:132with the same[_`]{0,3}/[_`\t ]{0,6}` composition the other two readers now share.
- Replace the two
Suggestions (1)
- [gstack-review]
server/src/services/ally-review-detection.ts:641—${count}is the(\d+)capture fromEMITTED_COUNTED_FINDINGS_BUCKET_PATTERNover model-authored review text: digits-only, but unbounded in length. It becomesblock.reason→verdict.reason→summaryatpr-comment-review-gate.ts:1061, which has no cap of its own. Same class as therawVersionbound you added at:482, not yet applied here. Worth saying plainly that this is a noise bound, not a security one — a digits-only token cannot carry a credential — so it is the same call you already made aboutisPublishableTokenlength, and${count.slice(0, PUBLISHABLE_TOKEN_BUDGET)}closes it for one word if you want the symmetry.
Strengths
- The two prior findings were not merely patched. Fixing the leak at the boundary rather than the field — one alphabet, one publisher guard, three call sites routed through it — is what makes the class closed rather than the instance, and it is why
:378is safe by ordering rather than by accident. - Taking the second resolution on the phantom test reference (drop the duplication) rather than correcting the reference was the right branch: the premise was false, so the constraint it justified had no reason to survive. Deleting
ba478a007is a smaller repo, not just a fixed comment. - Mutation-testing each guard alone, and reporting the per-guard failure counts, is exactly the discipline that catches a fixture passing on broken code. The
[\s\S]*→ 18 failures row is the one that proves the shared constant is load-bearing rather than cosmetic. - The fail-closed reasoning is unusually well documented and, where I could test it, correct: fence/blockquote/indented-code smuggling, the
openers > blockstruncation guard, multi-block and nested-opener cases,MAX_VERDICT_FINDING_COUNT, and Python/JS parity onre.ASCII,is_js_integer,js_trimandreject_js_nonfiniteall held under adversarial input. - Keeping
absentdistinct fromunreadable, and keepingabsentnon-blocking, is what stops this change red-wedging the whole open-PR population on arrival.
Recommended Action
- Fix Critical issues before merge.
- Address Important issues this cycle.
- Consider Suggestions opportunistically.
…e counts (BLO-32695) Peer review of #1721 at 8e6e84b — 1 Critical, 2 Important, 1 Suggestion. Critical: the structured `ok` branch of hasActionablePrReviewFeedback decided from `verdict.findings` alone, so a block stating {"critical":0} beside a ledger entry saying a prior Critical is `still-present` returned false. evaluateCommentReviewGate short-circuits on a current-head attestation before consulting the carry-forward, so nothing downstream re-examined it: forHead truthy -> false -> success/clean. A fail-open regression against master, where the identical body without a block blocks via the prose clause, and a live reader disagreement besides — check-ally-review-consistency.mjs read the same block as blocking and raised I2c. Gated on the same option as the prose twin so the carry-forward enumeration keeps asking its narrower question. Important 1: two JSON keys normalizing to one severity silently overwrote, last wins, so {"critical":1,"Critical":0} parsed ok with critical: 0. Reachable unlike an exact duplicate, which JSON.parse collapses first. All three readers shared it identically, so no peer reader caught it — guarded in all three. Important 2: check-ally-review-consistency.mjs was left on the narrow attestation form while the gate and the Python sweep were widened, dropping `**Reviewed head:** <sha>` — the exact shape this repo's own comment names, and the BLO-31730 failure mode. Masked on block-carrying bodies; the harm landed on the pre-block population, in a file whose purpose is reader parity. Suggestion: bound the `(\d+)` prose count quoted back into the check-run summary. A noise bound, not a security one. Also reverts 8e6e84b's interpolation of PUBLISHABLE_TOKEN_ALPHABET into PRIOR_FINDING_DISPOSITION_PATTERN. That commit measured the file as absent and concluded the PEN-3157 reference was false; the measurement was scoped to this branch's worktree. github-write-egress-scrub.test.ts exists on origin/master and asserts `([a-z][a-z-]*)` out of the pattern's own source text (:423) — it landed after this branch diverged, and CI builds refs/pull/N/merge. Ally's "prior important 2 — fixed" disposition repeats the same branch-scoped read. The verb group stays a literal; the two copies are held equal by an existing test that drives both. Every guard mutation-tested alone: Critical 2 failures, duplicate-key ts 3, mjs regex 2, duplicate-key python 1, count bound 1. The count bound initially had none — dropping it left all 219 tests green — so it was a comment rather than a guard until pinned. tsc clean; 230/230 server, 151/151 mjs, 132/132 python. check-ally-review-consistency.mjs no worse (its one violation is #1220, unrelated).
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: 1d6f378
Commit 1d6f37853 addresses all three findings from the previous head, each with a failing-mutation test rather than a comment. Verified against files fetched at this head, not against the expected patch.
Prior Findings Dispositioned (3)
- prior:8e6e84b critical 1 — fixed —
server/src/services/ally-review-detection.ts:1224— the structuredokbranch now consults the ledger before returning:block.verdict.dispositions.some((entry) => classifyPriorDisposition(entry.verb) === "blocks"), gated oncountInheritedLedgerAssertionexactly as its prose twin at:1102is, so the carry-forward enumeration keeps asking its narrower question. The gap I named — "no test places astill-presentblock at the head under evaluation" — is closed atpr-comment-review-gate.test.tsbyblockCarryingStillPresent(CURRENT_HEAD, "still-present"), driven both through the unit and throughevaluateCommentReviewGateend to end, which is the assertion that would have caught the short-circuit ordering. The"fixed"control is present, so the guard cannot be satisfied by any ledger entry at all. - prior:8e6e84b important 1 — fixed —
server/src/services/ally-review-detection.ts:390,scripts/check-ally-review-consistency.mjs:205,.github/scripts/sweep-stalled-ally-reviews.py:281— all three readers now fail closed on two keys normalizing to one severity, each placed after the vocabulary check and before the unconditional set, so{"critical":1,"Critical":0}isunreadablerather than clean. Both key orders are pinned in all three suites — the right call, since last-wins made the verdict order-dependent and a one-order guard leaves the dangerous order live — and each suite carries the distinct-severities control. The TS reason names the repetition without quoting the key, pinned by an explicit assertion that the reason contains no backtick. - prior:8e6e84b important 2 — fixed —
scripts/check-ally-review-consistency.mjs:152-156—MARKDOWN_EMPHASIS_RUN/ATTESTATION_WRAPPER_RUNare now composed character-for-character as inally-review-detection.ts:134,149,166-167andsweep-stalled-ally-reviews.py:92-104. I diffed the three compositions at this head: identical, including the[ \t]{0,3}inter-run bound.**Reviewed head:** `<sha>`and_Reviewed head:_ <sha>are pinned, deliberately on block-free fixtures — with a block they would pass whether or not the prose regex works, which is the trap that masked this originally. The converse test (** Reviewed head:**, four-space indent →null) pins that the widening did not overshoot the gate.
Critical Issues (0)
Important Issues (1)
-
[native-codex / gstack-review]
server/src/services/ally-review-detection.ts:1218— the counts axis fails a block closed when prose contradicts it; the ledger axis does not, and this commit closed only the block-internal half of that.proseCountContradictingat:556makes a blockunreadablewhen it statescritical: 0beside a prose bucket enumerating more, so the body falls through to prose and blocks. There is noproseDispositionContradicting. A body whose block omitsdispositions(or states[]) while its prose ledger carries- **prior:abc1234 critical 1** — still-present — …takes theokbranch, finds zero counts, finds an empty ledger, and returnsfalseat:1227→success/clean.The same body without a block blocks via the prose clause at
:1139. So this is a fail-open against master of the same shape as the Critical just fixed, one axis over — and the producer template at.planning/ally-agent/AGENTS.md:176-200mandates emitting both the block and the prose ledger, so the two can drift on exactly this field.Two things hold it below Critical, and I checked both rather than assuming: all three readers agree —
stillPresentIn(undefined)returnsfalseatcheck-ally-review-consistency.mjs:283,dispositions_oktreats an absent key as legitimately empty atsweep-stalled-ally-reviews.py:314, andasDispositions(undefined)returns[]— so unlike the Critical there is no live auditor disagreement to trip. And it needs a body that is already internally inconsistent, whereas the Critical fired on a contract-compliant one. Absence is legitimately "this review retires nothing"; the defect is only that it is indistinguishable from "the producer omitted what its own prose states".- Narrowest fix, beside the existing call at
:556, reusing the ledger pattern already in this file:// A prose ledger entry the block's `dispositions` never names is the same // block/prose drift proseCountContradicting already fails closed on. if (ledger.length === 0 && PRIOR_FINDING_DISPOSITION_PATTERN.test(text)) { return { kind: "unreadable", reason: "ally-verdict omits a ledger the review states in prose" }; }
PRIOR_FINDING_DISPOSITION_PATTERNisg-flagged, so resetlastIndexor usematchAll. - Not pinned in either direction: no fixture in
ally-review-verdict-block.test.tsorpr-comment-review-gate.test.tspairs an empty/absent block ledger with a prosestill-presentbullet. EveryblockCarryingStillPresentfixture states the entry in the block.
- Narrowest fix, beside the existing call at
Suggestions (0)
Strengths
- The
countInheritedLedgerAssertiongating on the new clause is the detail that makes it correct rather than merely blocking — it keeps the carry-forward enumeration answering "which findings did this head raise?" and is pinned by its own test. - Pinning both key orders for the case-variant severity guard, in all three suites, is the non-obvious half: last-wins made the verdict order-dependent, so a single-order fixture would have passed over a live hole.
- The explicit note that dropping the
.sliceleft all 219 tests green — and adding the bound test because of it — is the mutation discipline this file keeps asking for, applied unprompted to a Suggestion. - The restored PEN-3157 literal at
:786carries the reason the previous head's grep read as a false reference (the pin landed on master after this branch diverged; CI buildsrefs/pull/N/merge), which stops the next reader re-deriving the same wrong conclusion.PUBLISHABLE_TOKEN_ALPHABETremains live at:717, so the duplication is two copies held equal by a test, not a dead constant.
Recommended Action
- Address the Important finding this cycle, or record it as a known gap — it is the last uncovered quadrant of the block/prose drift matrix this PR exists to close.
…nds (BLO-32695) The counts axis already fails a verdict block closed when the review's own prose contradicts it (proseCountContradicting). The ledger axis had no twin, so a block whose `dispositions` are absent, `[]`, or merely missing the entry took the `ok` branch, found zero counts, found no blocking disposition, and returned clean -- while the identical body *without* a block blocks via the prose ledger clause in carriesBlockingFeedback. A fail-open against master of the same shape as the one fixed at 1d6f378, one axis over, and the producer template mandates emitting both the block and the prose ledger, which is exactly how the two come to disagree on this field. Found by Ally in review of #1721 at 1d6f378 -- the last uncovered quadrant of the block/prose drift matrix this PR exists to close. Ported to all three readers, not just the gate. The py sweep is the gate's only automatic escape route: with it disagreeing, the gate goes red on `unreadable_verdict` while the sweep sees a review that already happened and never re-requests the one that would clear it. check-ally-review-consistency.mjs carried the identical fail-open in its own precedence (`structuredBlocking(...) ?? hasStillPresentDisposition(...)`) -- the auditor whose job is to notice this class of divergence had it too. Narrower than the reported shape on purpose, and the controls pin it: only a prose entry classifying as `blocks` can fail a block closed. A prose `fixed` entry the block omits clears either way, so reddening it would be a false red with no fail-open behind it -- the #1675 direction. Symmetrically, a block already carrying a blocking verb cannot fail open, so the prose is not consulted and a contract-compliant still-present review stays readable. Each guard has a failing mutation: reverting the TS call site reds 5 tests, the mjs 1, the py 2. Verified one mutation at a time, since two at once lets one mask the other. `node scripts/check-ally-review-consistency.mjs` returns the same 2 unbaselined violations before and after (both on other PRs). Comments cite symbols rather than line numbers -- the two `:1102` references this diff would itself have shifted are now `carriesBlockingFeedback`.
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: dd61a4a
Commit dd61a4a2d closes the last quadrant of the block/prose drift matrix. Verified against files fetched at this head, and the divergence below was measured by executing the two readers rather than by reading their regexes.
Prior Findings Dispositioned (1)
- prior:1d6f378 important 1 — fixed —
server/src/services/ally-review-detection.ts:559—proseDispositionContradicting(:699) is now called besideproseCountContradicting, before theokreturn, and it is the count rule's exact twin: asymmetric (a block already carrying a blocking verb short-circuits at:703, so a contract-compliant standing entry still readsokand blocks as a structured finding rather than as an unreadable one), and it sharesPRIOR_FINDING_DISPOSITION_PATTERNandclassifyPriorDispositionwith the prose clause at:1184so the two cannot drift on what "still stands" means.match[4]is the verb group of the pattern at:831— checked, not assumed.matchAllis the only consumer of thatg-flagged pattern in the file, so no stalelastIndexcan skip the first entry.textisemittedReviewText, so fenced spans are stripped andNOT_INDENTED_CODEcovers the 4-space paste. Mirrored in both peer readers with the same asymmetry (.github/scripts/sweep-stalled-ally-reviews.py:372,scripts/check-ally-review-consistency.mjs:354), and the end-to-end assertion the gap named —evaluateCommentReviewGateover the drifted body, not just the unit — is present, with thefixed-only control that keeps this a fail-closed rule rather than a widening.
Critical Issues (0)
Important Issues (1)
-
[native-codex / gstack-review]
scripts/check-ally-review-consistency.mjs:354— the new auditor clause answers "does the prose ledger still stand?" withSTILL_PRESENT_DISPOSITION_RE(:125), which is not composed character-for-character withPRIOR_FINDING_DISPOSITION_PATTERN(ally-review-detection.ts:831, mirrored verbatim atsweep-stalled-ally-reviews.py:169). Three readers now decide the sameunreadable/okquestion from two different patterns, and they disagree in both directions. Measured at this head by importing the real modules:prose ledger entry auditor attestedHeadsweep parse_reviewed_head/ gatetrailing parenthetical after the index nullattests head en dash ( –) as the separatorattests head None/ unreadablespace between **andprior:attests head None/ unreadablecanonical form nullNone/ unreadable ✅The loose pattern accepts
**prior:[^\n]**— so it matches an entry the strict one rejects — while accepting only—|-and no space after**, so it misses two the strict one accepts. This isprior:8e6e84b important 2one pattern over: that finding fixedMARKDOWN_EMPHASIS_RUN/ATTESTATION_WRAPPER_RUNto be identical across the three readers, and the ledger-entry run is the remaining composition that is not. It matters more now than before this commit, because the divergence used to change only which I2c violation the auditor reported; it now changes whether the auditor believes a head was attested at all — so on rows 2–3 the gate is red onunreadable_verdictwhile the auditor sees a cleanly-attesting review and has nothing to report, which is the one failure the auditor exists to prevent.- Narrowest fix: export the pattern from
ally-review-detection.tsis not available to a plain.mjsscript, so composeSTILL_PRESENT_DISPOSITION_REcharacter-for-character assweep-stalled-ally-reviews.py:169already does — sameprior:[0-9a-f]{7,40}[ \t]+[a-z]+[ \t]+[0-9]+body, same(?:—|–|-)alternation, same[ \t]*bounds — and gate the captured verb on the existingBLOCKING_PRIOR_DISPOSITIONSat:182rather than embedding the literal verb in the regex. - Not pinned in either direction: every ledger fixture in
check-ally-review-consistency.test.mjs(e.g.:500) and in the new gate suites uses the canonical shape, which is the one row all three agree on. A cross-reader corpus driving the same ledger strings through all three readers would have caught this, and is what the counts axis already has.
- Narrowest fix: export the pattern from
Suggestions (1)
- [pr-review-toolkit:comments]
server/src/services/ally-review-detection.ts:706— the reason readsally-verdict retires every prior finding but the review's prose ledger retains one, but the clause also fires whendispositionsis absent or[], i.e. when the block retires nothing. "states no standing prior finding" would describe all three shapes the tests pin.
Strengths
- The asymmetry is the part that had to be right and is:
stillPresent/blocksshort-circuits before the prose is consulted, so a contract-compliant still-present review readsokand blocks structurally instead of reading broken. Each of the three readers carries that control as its own test. - All three shapes of the hole are pinned —
dispositionsabsent,[], and a partially-drifted ledger naming only a retired entry — rather than just the absent case that motivated it. - The end-to-end
evaluateCommentReviewGateassertion alongside the unit is the mutation discipline this file keeps asking for: the unit alone passes while the gate greens by another route, which is how this family has escaped before. - The Python quoted/fenced/indented control is the right guard on the other side — over-matching here re-requests a review Ally already gave, which is the BLO-22892/BLO-28203 duplicate loop.
prose_disposition_contradictsreuseswithout_fenced_spansand the shared verb set rather than re-deriving either, so the Python mirror cannot drift on vocabulary; all three spell the blocking set as exactly{still-present}— checked.
Recommended Action
- Address the Important finding this cycle, or record it as a known gap — it is the fourth reader composition of a pattern the other three were already aligned on.
- Consider the Suggestion opportunistically.
…32695) The auditor decided "does the prose ledger still stand?" with a regex that was not composed character-for-character with PRIOR_FINDING_DISPOSITION_PATTERN in ally-review-detection.ts and sweep-stalled-ally-reviews.py, so the three readers disagreed in both directions (trailing parenthetical accepted here only; en dash and a space after ** accepted there only). Compose the pattern identically, decide the verb against BLOCKING_PRIOR_DISPOSITIONS, and add a cross-reader corpus that drives the same ledger strings through the patterns read out of all three committed files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@allyblockcast please re-review at a412b4d. Drive lease (penstock-drive, session d71b): pushed a412b4d on top of dd61a4a, addressing the Important finding at dd61a4a. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a412b4d
Commit a412b4d8e closes the ledger-entry composition gap. Verified against files fetched at this head, and both the fix and the finding below were measured by executing the extracted patterns rather than by reading them.
Prior Findings Dispositioned (1)
- prior:dd61a4a important 1 — fixed —
scripts/check-ally-review-consistency.mjs:133—STILL_PRESENT_DISPOSITION_REis gone;PRIOR_FINDING_DISPOSITION_REis byte-identical toPRIOR_FINDING_DISPOSITION_PATTERNatally-review-detection.ts:831(compared programmatically, not by eye — the twoString.rawbodies are string-equal), andsweep-stalled-ally-reviews.py:169differs only in using non-capturing groups for sha/severity/index, putting its verb at group 1 where the other two carry group 4.hasStillPresentDisposition(:531) now gates the captured verb onBLOCKING_PRIOR_DISPOSITIONSrather than embeddingstill-presentin the pattern. I extracted all three patterns from the committed sources and drove a 16-row corpus through them — the six rows the fix claims plus ten I added (uppercase sha, 40-hex sha, 3-space indent, 4-space indent, tab indent, tab after the bullet, hyphen separator,no-longer-applicable, 6-hex sha, missing trailing separator): all three readers agree on all 16, and all 16 match the intended verdict. The three rows this finding measured as diverging (en dash, space after the emphasis, trailing parenthetical) now agree. Mutation control: against the previousSTILL_PRESENT_DISPOSITION_REthe corpus fails on three rows, not just the en dash — so the new test genuinely discriminates and would catch a revert.matchAllremains the only consumer of theg-flagged pattern in all three files (mjs:532,ts:704,ts:1184,py:391), so no stalelastIndexcan skip the first entry. Fence handling is unchanged and role-consistent: theunreadableaxis strips (mjs:362receiveswithoutFencedSpans,py:391,tsviaemittedReviewText), the detecting axis deliberately does not (mjs:451,ts:1184), whichmjs:320-326documents as the pre-existing residual.
Critical Issues (0)
Important Issues (1)
- [native-codex / gstack-review]
scripts/check-ally-review-consistency.test.mjs:183— the new cross-reader test reads the two pattern bodies out of the committed sources, which is the right instinct, but then substitutes a retyped copy of the interpolated sub-pattern:notIndentedCodeat:183is a hand-written duplicate ofNOT_INDENTED_CODE, and:192splices it into the gate pattern via.replace("${NOT_INDENTED_CODE}", notIndentedCode). So the test never readsally-review-detection.ts:106. There are now three copies of that constant —ts:106,mjs:122, and the test's own — and the test compares the real mjs reader against a synthetic gate pattern built from the stale literal. Measured: tightents:106to(?! *\t)(?! {3})and a 3-space-indented ledger entry is read as blocking by the real mjs reader and not by the real ts reader, while this corpus still reports agreement on every row. The guard passes; the readers have drifted. That is the same defect class this commit exists to close, reintroduced one level down inside the guard itself — and it is the axis the guard is the only thing binding, sincemjsandtseach hold their own copy of the constant.- Extract it the way the pattern bodies are already extracted: match
NOT_INDENTED_CODE = String.raw\([^\`]+)`out oftsSourceand use that capture in the.replace`, asserting it was found. Three lines, no retyping, and it fails loudly if the constant is renamed. - Honest bound on severity, since it argues against me: for this pattern
(?! {4})is currently unreachable — the following{0,3}bound already rejects any 4-space indent, and I could construct no input distinguishing the constant's presence from its absence. So there is no divergence at this head; the exposure is to future edits of a constant that is shared by every line-anchored pattern in both files and is explicitly documented atts:97-105as the thing that must not be allowed to disagree.
- Extract it the way the pattern bodies are already extracted: match
Suggestions (1)
- [pr-review-toolkit:tests]
scripts/check-ally-review-consistency.test.mjs:217— the sweep's verb group is hardcoded as1against the gate's4, which is correct today only because the Python mirror happens to use non-capturing groups for sha/severity/index. Adding one capture group topy:169silently shifts the verb, and the test would then compare the sha against"still-present". It fails closed (the assertion trips) so this is not a hole, but a named group —(?P<verb>...)in Python,(?<verb>...)in the JS pair — would make the coupling explicit instead of positional, and would let the test read the same name from all three.
Strengths
- Composing the auditor pattern character-for-character with the gate's and moving the verb decision out of the regex onto
BLOCKING_PRIOR_DISPOSITIONSis the correct shape: it makes an unrecognized verb non-blocking in all three readers by construction rather than by three separate regex alternations agreeing. - Reading both peer patterns out of the committed sources rather than retyping them is exactly the right instinct, and it is what makes the residual at
:183narrow enough to state precisely. - The corpus drives the same six strings through all three readers and asserts they agree, rather than asserting each reader against an expected value separately — which is the only shape that can catch a divergence, and is what the counts axis already had.
- The
.replacetarget and bothassert.okguards mean a rename or a reformat of either pattern breaks the test loudly instead of silently degrading it to a no-op.
Recommended Action
- Address the Important finding this cycle — it is a three-line change to the test harness.
- Consider the Suggestion opportunistically.
…rce (BLO-32695) The consistency test retyped NOT_INDENTED_CODE from ally-review-detection.ts by hand, so a change to the gate's pattern would leave the test agreeing with a stale copy instead of the gate. Extract the literal from the TypeScript source at test time and fail loudly when the constant is no longer found. Negative control: mutating the source constant to `(?!-)` now fails the ledger-entries case; the retyped copy passed unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Lease: pushing one mechanical fix for Ally's remaining Important at a412b4d (retyped 🤖 Generated with Claude Code |
|
@allyblockcast please re-review at cfb8f07 (the Important from a412b4d is addressed: the test reads 🤖 Generated with Claude Code |
|
@allyblockcast please review the current HEAD of #1721. Exact HEAD: Your last consolidated review attests |
|
@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 |
This branch has not been deployed
1 participant
Thinking Path
Linked Issues or Issue Description
Fixes: BLO-32695
Refs BLO-29711, BLO-31730, BLO-31947, BLO-31446, BLO-31526
What Changed
Producer —
.planning/ally-agent/AGENTS.md<!-- ally-verdict:1 … -->block carryinghead, per-severityfindingscounts, and onedispositionsentry per retired prior finding as{head, severity, index, verb}fields.Reviewed head:line; counts drive blocking, verb vocabulary unchanged.Consumer —
server/src/services/ally-review-detection.tsparseAllyVerdictBlock, returning a three-wayabsent/ok/unreadable.extractAllyReviewedHeadSha,extractAllyPriorFindingDispositions,extractAllyReportedFindingRefs, andhasActionablePrReviewFeedbackall read the block first and fall back to prose only when it isabsent.```ally-verdictpayload would be blanked bywithoutFencedCodeBlocksbefore any parser saw it.Gate —
server/src/services/pr-comment-review-gate.tsunreadable_verdictoutcome, kept distinct from both "no review" and "carries a finding".cleanreason now names its source (structured block vs prose fallback), so a silent regression back onto the prose path is visible on the PR instead of emitting an identical string either way.Verification
server/src/__tests__/ally-review-verdict-block.test.ts— 377 lines of new coverage built onfixtures/ally-review-pr1675-2026-09-07T154142Z.md, the verbatim fix(attribution): correct stale git-identity guidance and de-vacuate the gate's tests #1675 15:41:42Z body. Each of the four prose failures is pinned, plus fail-closed cases: two blocks, unsupported version, malformed JSON, missing/short head.npx vitest run src/__tests__/ally-review-verdict-block.test.ts src/__tests__/pr-comment-review-gate.test.ts→ 85 passed.node --test scripts/ally-agent-idempotency-contract.test.mjs scripts/check-comment-review-gate-census.test.mjs→ 28 passed. The contract test matches the template against the parser's own opener, so documenting the marker in prose while emitting a different spelling cannot pass.pnpm typecheck→ clean across all packages.583085dedand confirmgate/ally-comment-findingsreportssuccesswith a description naming the structured source.Risks
Low, and deliberately asymmetric.
absentstays non-blocking. Every review posted before this ships carries no block, so treatingabsentas red would red-wedge the entire open-PR population on arrival. Only an unreadable block is red — this is not a fail-open regression of BLO-29711.Reviewed head:and only this gate understands the block: this module,consolidatedReviewHeadingithub-app-auth.ts,ATTESTED_HEAD_REinscripts/check-ally-review-consistency.mjs, andHEAD_ATTESTATION_REin.github/scripts/sweep-stalled-ally-reviews.py. A block-only review would attest nothing to readers 2–4 and reader 2 would raise a falsepr_review_output_missing. The AGENTS.md rules and the contract test both pin this.ally-review-detection.ts— whoever lands second should expect a conflict in the prose-pattern region, which this PR does not modify.Model Used
Claude Opus 4.5 (
claude-opus-4-5), 1M context, extended thinking, tool use.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template