fix(review-gate): honor Ally's prior-findings disposition ledger - #1568
Conversation
|
Hey @kkroo! 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: 6b5b6e8
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/pr-comment-review-gate.ts:187-193— the prior-findings ledger is matched only by abbreviated head SHA, so onefixedledger entry clears every blocking finding raised on that head. A review can report bothprior:<head> critical 1andprior:<head> important 1, then disposition only the critical item;isDispositionedstill removes the entire head fromheadsWithUndispositionedFinding, allowing the important finding to disappear from the gate.- Preserve and match the complete stable finding identity (
head, severity, and 1-based index), and only retire the specific finding named by the ledger. Add a regression test with two findings on one prior head where only one is markedfixed.
- Preserve and match the complete stable finding identity (
Suggestions (0)
Strengths
- The parser fails closed for unrecognized disposition verbs and uses prefix matching for abbreviated SHAs.
- The implementation preserves ordering and timestamp safeguards for later ledgers.
Recommended Action
- Fix the Important issue before merge.
- Re-run the review-gate and consistency suites.
6b5b6e8 to
4659ebf
Compare
|
Addressed the Important finding from the review of The finding was correct: matching the ledger on the abbreviated head SHA alone meant a single Ally already emits a stable per-finding identity — severity plus a 1-based index within that bucket — and the counted One case worth calling out beyond the report: a body whose blocking feedback came from prose or an uncounted heading yields no identities to match against. Three tests added, including the two-findings/one- |
|
Hey @kkroo! 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. Toolkit and gstack skill bodies were unavailable in this runtime; their required review categories were applied directly to the exact PR diff.
Reviewed head: 4659ebf
Prior Findings Dispositioned (1)
- prior:6b5b6e8 important 1 — fixed —
server/src/services/pr-comment-review-gate.ts:196-203now matches the complete prior-finding identity by head prefix, severity, and index, so one ledger entry cannot retire unrelated findings from the same head.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/ally-review-detection.ts:83-88— the ledger only treatsfixedas a resolving disposition. The review contract also definesno-longer-applicableas retiring a prior finding, and the regex accepts that verb, so such a valid ledger entry is silently ignored and the finding remains blocking indefinitely.- Include
no-longer-applicableinRESOLVED_PRIOR_DISPOSITIONSand add a regression test proving that disposition retires the named prior finding without allowing unrelated findings to clear.
- Include
Suggestions (0)
Strengths
- The ledger now preserves severity and 1-based finding index, preventing partial-ledger entries from clearing an entire prior head.
- Prefix-anchored SHA matching and timestamp ordering provide conservative identity and chronology checks.
- The added tests cover partial retirement, unknown verbs, stale ordering, prefix matching, and unenumerated findings.
Recommended Action
- Fix the Important issue before merge.
- Re-run the review-gate and consistency suites.
The comment-review gate carried a finding forward from an earlier head until a later clean review of that *same* head cleared it. Ally never does that — it moves to the new head and closes the old finding in its next review, under a "Prior Findings Dispositioned" heading naming the head the finding was raised against. The gate never read that block, so a finding Ally had already closed kept the PR red once the head moved on again. Seen on Blockcast/libmmt#362: Ally's review of c9a1765 recorded `prior:731ced5 critical 1 — fixed`, and review/ally-comment still reported "An unresolved finding from Ally's review of 731ced5 is still undispositioned" against the following head. The existing docstring names exactly what was missing — a clean review of another head can't disposition H because nothing establishes that the other head contains the fix. The ledger is that evidence: it's a direct assertion about a named prior tree, not an inference from comment order. Only `fixed` clears. `still-present` asserts the opposite, and the sibling consistency guard already treats it as blocking (I2c). An unrecognized verb doesn't clear either, so a new word in Ally's vocabulary can't silently unblock a merge before someone decides it should. A ledger entry also has to be at least as new as the attestation it clears and come from a review of a different head, so a review can't disposition its own finding. Parsing lives in ally-review-detection.ts alongside the other shared predicates, matching the entry shape check-ally-review-consistency.mjs already parses.
Ally's review of 6b5b6e8 caught this: matching the ledger on the abbreviated head SHA alone meant one `fixed` entry cleared every finding raised on that head. A review reporting both `prior:<head> critical 1` and `prior:<head> important 1` could disposition only the critical item and the important one would vanish from the gate — the exact class of silent drop this gate exists to prevent. Ally already emits a stable identity per finding (severity plus its 1-based index within that bucket), and its counted `Critical/Important Issues (N)` headings enumerate what a head raised. So a head is retired only once every finding it reported is named by a later ledger. A body whose blocking feedback came from prose or an uncounted heading yields no identities to match, so it can never be fully retired and stays carried rather than being cleared by an unrelated entry.
Ally's review of 4659ebf caught a third disposition verb this allowlist was missing. The vocabulary is three words, not two: `fixed` and `no-longer-applicable` retire a prior finding, `still-present` asserts it stands. A ledger entry using the missing verb parsed fine and was then dropped, so the finding stayed blocking with nothing left that could retire it. `no-longer-applicable` means the finding does not apply to this code — often that it was incorrect as filed — so it retires without implying anything changed. Blockcast/onprem-k8s#2881 states the contract; it is also in use on #1126 and Blockcast/go-amt#93. Worth recording that the verb appears nowhere in this repo and nowhere in `gh search code`; it is only observable in real review bodies, which is why the first pass concluded there were two verbs. The allowlist stays an allowlist. The failure modes are asymmetric: an unrecognized verb leaves a PR visibly red until someone widens this set, whereas a denylist would silently clear a live finding. This very omission surfaced as a red gate rather than as a merged regression. Retiring stays per-finding, so `no-longer-applicable` gets the same severity+index identity matching as `fixed` and cannot clear a sibling finding the ledger never named.
4659ebf to
0a09fe4
Compare
|
Addressed in
Kept it an allowlist rather than flipping to "retire unless Retiring stays per-finding, so the new verb gets the same severity+index matching as One thing I did not fold in, to keep this diff reviewable: when a ledger entry uses an unrecognized verb the gate stays red with no indication why, which is the "silently ignored" part of the report. That's an observability gap rather than a correctness one — the verdict is right either way — so it seems better as a follow-up than as more surface on a merge-gate change that has already grown twice. Happy to do it here instead if you'd rather. Suites: 304 across eight review-gate/webhook files. |
|
Hey @kkroo! 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: 0a09fe4
Prior Findings Dispositioned (1)
- prior:4659ebf important 1 — fixed —
server/src/services/ally-review-detection.ts:96now includesno-longer-applicablein the resolved disposition vocabulary, and the added test atserver/src/__tests__/pr-comment-review-gate.test.ts:379-396verifies that it retires the named prior finding.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Prior-finding retirement is now matched per head prefix, severity, and 1-based finding index, so partial ledgers cannot clear unrelated findings.
- The parser fails closed for unrecognized disposition verbs and safely supports both resolving dispositions defined by the review contract.
- The regression coverage exercises chronology, prefix matching, partial retirement, unenumerated findings, and
no-longer-applicablehandling.
Recommended Action
- Merge when the remaining repository checks are green.
|
Hey @kkroo! 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 participant
Thinking Path
Linked Issues or Issue Description
No existing issue — describing inline as a bug report.
What happened. On Blockcast/libmmt#362, Ally reviewed head
c9a1765and recorded:After one more push,
review/ally-commentreported:Expected. The finding was closed by its own reviewer, so the gate should not still be blocking on it.
Actual.
headsWithUndispositionedFindingkeyed comments by the head each attests. A finding on head H could only be cleared by another review of H, which Ally does not do, so the PR stayed red until a human merged past it.Regression range. Introduced with the carry behavior in #1464 (BLO-29711), which added the per-head carry without a ledger reader.
What Changed
ally-review-detection.ts: parse### Prior Findings Dispositionedentries into{shortSha, severity, index}, and enumerate a review's own findings from its countedCritical/Important Issues (N)headings.pr-comment-review-gate.ts: retire a head only when every finding it raised is named by a later ledger entry. Matching is per finding, not per head.fixedandno-longer-applicableretire,still-presentblocks, anything unrecognized does not retire.Verification
pnpm exec vitest runinserver/:pr-comment-review-gate.test.ts— 25 passed (8 new)github-webhook.test.ts— 304 passed across 8 filespnpm exec tsc --noEmit— cleanEvery new test was confirmed to discriminate by mutating the implementation and checking a different test goes red each time. This matters because five of the eight assert that dispositioning does not happen, so they pass against a no-op and prove nothing on their own:
still-present+ unknown-verbno-longer-applicablefrom the allowlistno-longer-applicableretirementRisks
Moderate — this is merge-gating logic, so the failure that matters is a false green.
(head, severity, index)identity: onefixedentry cannot retire a sibling finding the ledger never named. Ally caught exactly this on an earlier revision of this PR.extractAllyReportedFindingRefsreturnsnullrather than[]there, because[].every(...)istrueand "nothing to enumerate" would otherwise have read as "nothing left to retire".no-longer-applicableomission surfaced that way during review — as a red gate, not a merged regression.Known follow-up, deliberately not in this diff: when a ledger uses an unrecognized verb the gate stays red with no indication why. That is observability, not correctness — the verdict is right either way — and this is already a twice-grown merge-gate change. Happy to fold it in here if reviewers prefer.
Dedup finding worth flagging: #1471 (open, BLO-29711) also touches
server/src/__tests__/pr-comment-review-gate.test.ts. No functional overlap — it moves the status context out of thereview/namespace, this changes disposition parsing — but whichever lands second will likely need a merge in that test file.Model Used
Anthropic
claude-opus-5(1M context), via Claude Code.