fix(ally-guard): classify a same-lane duplicate review by body, not by timing - #1738
Merged
Merged
Conversation
…y timing
I1 reports that one PR head carries two operative Ally reviews. It did not
report which of two different defects produced them, and the two need
opposite fixes, so every finding had to be hand-diffed to be actionable.
Add sameLaneBodyRelation() and name the mechanism in the I1 message:
resubmit every body identical -> one computed verdict reached GitHub more
than once; the submit step is at-least-once.
recompute bodies differ -> two full reviews were computed for one head and
both submitted; the missing control is reviewer exclusion, not
submit idempotency.
mixed >2 reviews showing both shapes at once.
A body is left unclassified when any of them is empty: that is an attestation
defect (I3), and inferring a mechanism from it would assert more than the
evidence carries.
Timing was the previous discriminator and it is wrong. On paperclip#1220 two
reviews 10 s apart carry different bodies (8513 vs 6564 bytes) -- a genuine
double-compute inside the window a timing rule reserves for retries. Measured
against live GitHub, the four current violations split 2 resubmit
(#1731, #1730) / 2 recompute (#1721, #1220), where the timing rule scores
three of them as retries.
The classification clause is deliberately free of any 6-digit-or-longer
number. violationFingerprint() harvests every such token out of the message
text, so a count or an account id here would change an I1 fingerprint and
silently void the matching baseline suppression. A test pins that, and it was
mutation-checked: injecting an account id into the clause fails it.
Refs PEN-2865, BLO-19778.
Signed-off-by: Cto <cto@paperclip.blockcast.net>
Author
1 similar comment
Author
Author
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
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: 4b1bc3a
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The body-based classifier cleanly separates identical resubmissions, substantive recomputations, and mixed duplicate sets without relying on timing.
- Empty-body handling fails closed rather than inferring a mechanism from incomplete evidence.
- The tests cover whitespace normalization, all relation classes, fingerprint stability, and the committed baseline; the targeted suite passes 124/124.
Recommended Action
- No Critical or Important review findings. The implementation is suitable for merge once the repository's required checks complete.
Merged
13 tasks
13 tasks
13 tasks
Merged
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs PEN-2865, BLO-19778. Related (not superseded by this PR): #1687, #1141.
Searched open PRs and
gh search prsforcheck-ally-review-consistency/ ally / duplicate / idempotency. Closest neighbours, all complementary rather than overlapping:allyReviewAlreadyAttestsHead()check. See "Relationship to fix(webhook): don't wake the PR reviewer for a head it already reviewed (BLO-32198) #1687" below; it fixes a strict subset of what I1 reports, and this PR is what tells you which subset.fix(ally-guard): ignore superseded review status updates. Touches the same script, no overlap with the I1 message path.Thinking Path
Linked Issues or Issue Description
Refs PEN-2865— the duplicate-review defect this diagnoses (Paperclip issue, not a GitHub issue)Refs BLO-19778— the incident class the guard exists to preventWhat Changed
scripts/check-ally-review-consistency.mjs— new exportedsameLaneBodyRelation(operative)returningresubmit(all bodies identical) /recompute(bodies differ) /mixed(>2 reviews, both shapes) /null(not a duplicate, or any body empty). Reuses the existingnormalizedBody()so the trim semantics stay shared withduplicateBodyAcrossIdentities.scripts/check-ally-review-consistency.mjs— theI1message now appends the mechanism the relation implies (SAME_LANE_RELATION_NOTES). Nothing else aboutI1changes: same trigger, same count, same fingerprint.scripts/check-ally-review-consistency.test.mjs— 11 new tests across the classifier, theI1wording, the empty-body abstention, and fingerprint stability.Verification
The live run classified all four then-current violations exactly as they had been independently hand-diffed from
GET /pulls/{n}/reviews:772616cbresubmita17fba0dresubmita9ee094arecomputea8096107recompute#1220 is the counterexample that motivates the whole change: 10 seconds apart with two independently composed reviews (one opens
### Prior Findings Dispositioned (2), the other with a rebase/mergeStateStatushead-context paragraph). A timing threshold files that as a retry; submit idempotency keys on the payload and these are two different payloads, so it would not fix it.Mutation-checked both new invariants, i.e. I confirmed the tests fail when the behaviour is wrong:
resubmit→ 3 tests failRelationship to #1687
#1687 adds prevention (a pre-wake
allyReviewAlreadyAttestsHead()check) and is the right shape for it. This PR is diagnosis, and the two compose: the classification says which instances #1687 closes.A pre-wake check fires before any review is computed or submitted, so on the four above it closes
#1721(2.4 h — by the second wake the first review already attests the head) but structurally cannot close:#1731/#1730— one wake, one computed body, submitted twice. The duplicate happens after the wake decision.#1220— two wakes both arriving before either review posts. Both legitimately readnot_attested; a full review takes minutes, so a 10-second submission gap means both were already in flight. Same shape as the 43 ms#876pair in this script's docblock.So the remaining gap after #1687 is a once-only submit, and
resubmitin the guard output is what names it. Nothing here changes or duplicates #1687.Risks
Low risk, and deliberately bounded:
I1string grows.violationFingerprint()derives the baseline key from the message text by harvesting every 6-digit-or-longer token, so a byte count or an account id in the new clause would change everyI1fingerprint and silently void the matchingscripts/ally-review-consistency-baseline.jsonsuppressions. The clause is digit-free by construction, a comment on the helper says why, and a test pins the fingerprint across all three relations.pr.ymlalready runs this test file, so the new tests gate on the existing job.Separately noted, not changed here: all six baseline entries are currently stale (the guard warns on each) because the violations they pinned rotated out. Pruning them changes no verdict; they belong to PEN-2847 and I left them to that row's owner rather than mixing an unrelated edit into this diff.
Model Used
Claude Opus 4.5 (
claude-opus-5[1m]as configured in this agent's adapter), 1M context, extended thinking, with tool use (shell, file edit, GitHub API) via the Claude Agent SDK.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template