fix(ally): retain the newest duplicate review, never dismiss it (BLO-32837) by allyblockcast[bot] · Pull Request #2105 · Blockcast/paperclip

allyblockcast · 2026-09-29T07:22:53Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

One of those agents is Ally, the code reviewer, and scripts/check-ally-review-consistency.mjs is the guard that keeps its GitHub review attestations honest

That guard's invariant I1 caps operative App-lane reviews at one per (PR, head) — but it constrains only the end state, and says nothing about which review survives when a duplicate must be cleaned up

So I1 is satisfiable by a remedy that breaks the PR: GitHub derives reviewDecision from each reviewer's latest review, so dismissing the newer of two duplicate approvals leaves DISMISSED as the latest state and makes the retained approval inert

That is not hypothetical — Blockcast/onprem-k8s#3281 hit it, went mergeStateStatus=BLOCKED, and was merged with --admin despite carrying a genuine Ally approval

This pull request adds a tested selector that makes the choice mechanically instead of by hand, and refuses to act on the two shapes where acting would destroy information

The benefit is that a duplicate wake stops costing an admin merge, and the correct choice stops depending on whoever is cleaning up remembering which direction is safe

Linked Issues or Issue Description

Fixes: BLO-32837
Refs BLO-19778 — the concurrent-run race that produces duplicates in the first place (feat(sweep): human-gated ageing escalation on a clock agents cannot move (BLO-19130) #876, two runs 43 ms apart)
Refs BLO-34581 — GitHub re-anchors commit_id forward on APPROVED reviews, which is why selection here keys on the body attestation
Refs BLO-32198 — the server-side head-attestation gate that already prevents most duplicate submissions

Related open PRs (searched; no file overlap with this one):

fix(ally-guard): a re-review of one head supersedes, it is not a duplicate (BLO-25764) #1996 fix(ally-guard): a re-review of one head supersedes, it is not a duplicate (BLO-25764) — closest neighbour and complementary. It works at the reporting layer, reclassifying a re-review so the guard stops calling it a duplicate; this PR works at the remedy layer, constraining which review a cleanup may dismiss. It touches check-ally-review-consistency.{mjs,test.mjs}; this PR only imports two unchanged helpers from that module (attestedHead, allyReviewLane) and adds new files, so the two land independently.
fix(review-gate): refuse a re-anchored review whose body attests a different head (PEN-3413) #1984 fix(review-gate): refuse a re-anchored review whose body attests a different head (PEN-3413) — same underlying commit_id-mutability fact, applied in server/src/services/. No overlap.

What Changed

scripts/ally-review-de-dupe.mjs (new). selectDuplicateDismissals(reviews, headSha) → {reason, retain, dismiss}. Retains the newest candidate by submitted_at (id tiebreak); dismiss never contains it. Also exports exactHeadAppReviews and provides a stdin CLI for manual inspection.
Refuses rather than guesses on three shapes. conflicting-verdicts (two runs disagreeing at one head is a blocker-driven supersession, not a duplicate — picking a winner by clock order would discard a real finding), unorderable (a submitted_at that will not parse, which is exactly the condition that produced the incident), and none.
Matches on the body Reviewed head: attestation, never commit_id. GitHub re-anchors commit_id forward on APPROVED reviews when the branch is updated, and APPROVED is the state being de-duplicated, so commit_id both admits reviews that never read this head and hides ones that did.
Scoped to the App lane. R4 (BLO-24056) bars the allyblockcast User seat from submitting a review at all, so a seat review is an I6 violation to report, not a duplicate to tidy away.
scripts/ally-review-de-dupe.test.mjs (new). 16 tests, including the feat: skill folders and hide skills paperclipai/paperclip#3281 shape asserted in both input orderings.
.github/workflows/pr.yml. New Test Ally duplicate-review selection (BLO-32837) step beside the existing consistency-guard step.
.planning/ally-agent/AGENTS.md. New Step 6 documenting the rule and the remedy command (old Step 6 renumbered to 7).

Verification

End-to-end against the live incident. Fetched onprem-k8s#3281's real reviews, reset both to APPROVED to reconstruct the 20:10:36Z state, piped through the CLI:

$ gh api repos/Blockcast/onprem-k8s/pulls/3281/reviews --paginate \
    | node ./scripts/ally-review-de-dupe.mjs b108db25bdfa394bc6c2371120917538abe025c6
{ "reason": "duplicate",
  "retain":  { "id": 5146534396, "submitted_at": "2026-09-08T20:10:36Z" },
  "dismiss": [{ "id": 5146530564, "submitted_at": "2026-09-08T20:10:12Z" }] }

The exact inverse of what happened. (On that PR today, 5146534396 — the newer — is the one sitting DISMISSED.)

Tests.

$ node --test scripts/ally-agent-idempotency-contract.test.mjs \
               scripts/check-ally-review-consistency.test.mjs \
               scripts/ally-review-de-dupe.test.mjs
ℹ tests 167   ℹ pass 167   ℹ fail 0

Mutation-tested. Each guard reverted individually, one per run, confirming the suite goes red — a guard with no failing mutation is a comment, not a test:

mutation	result
retain oldest (the BLO-32837 defect itself)	killed (6 failing)
id tiebreak removed	killed (3)
conflicting-verdict refusal removed	killed (3)
unorderable refusal removed	killed (3)
key on `commit_id` instead of the attestation	killed (13)
operative-state (DISMISSED/PENDING) exclusion removed	killed (3)
app-lane restriction widened to the seat	killed (3)
head lower-casing removed	killed (3)
single-candidate short-circuit removed	killed (3)
bare `isMainModule()` (the silent-CLI bug)	killed (4)

A tenth guard — a ^[0-9a-f]{40}$ shape check on the head argument — survived. Comparing against attestedHead's output (only ever 40-hex or null) already rejects every malformed head, so it could not change an outcome. Removed rather than shipped as decoration.

That discipline also caught a real bug before merge: the first cut called a bare isMainModule(), whose default import.meta.url is evaluated in the module that defines it — so it compared argv[1] against check-ally-review-consistency's URL, never matched, and the CLI exited 0 having printed nothing. Invisible to every library-level test; now covered by a spawn test.

Risks

Low. The new module is additive and nothing calls it yet — it is a decision aid, so a wrong answer cannot dismiss anything on its own. It imports two helpers from check-ally-review-consistency.mjs without modifying them.

Two limits worth stating explicitly rather than implying:

This does not by itself change Ally's runtime behaviour. .planning/ally-agent/AGENTS.md is not Ally's live instruction source (its own contract test records this). The live managed bundle has no clean-duplicate cleanup rule at all — which is why feat: skill folders and hide skills paperclipai/paperclip#3281's choice was ad-hoc — and syncing it is a separate CTO action. This PR makes the correct choice available and tested; it does not guarantee a future cleanup consults it.
A COMMENTED review cannot be dismissed through the GitHub API, so a COMMENTED/COMMENTED duplicate has no cleanup path. Deliberately left for I1 to report rather than papered over.

Not core feature work, so no ROADMAP.md overlap — the roadmap's review/approval entries concern Paperclip's own workflow stages, not the Ally GitHub-review guard scripts.

Model Used

Claude Opus 4.5 (claude-opus-5[1m] as configured for this agent), 1M context, extended thinking, with tool use and code execution. Ran as the Paperclip agent Ally in a claude_k8s pod.

Checklist

…32837) check-ally-review-consistency.mjs invariant I1 caps operative App-lane reviews at one per (PR, head) but says nothing about how a violation is remedied, and the obvious remedy is wrong. GitHub derives reviewDecision from each reviewer's latest review, so dismissing the newer of two duplicate approvals leaves DISMISSED as the reviewer's latest state and makes the retained approval inert: I1 satisfied, PR unmergeable. Measured on Blockcast/onprem-k8s#3281 @ b108db25 — reviews 5146530564 (20:10:12Z) and 5146534396 (20:10:36Z), both APPROVED. The cleanup dismissed 5146534396, the newer one. The PR went reviewDecision=REVIEW_REQUIRED / mergeStateStatus=BLOCKED with latestOpinionatedReviews=[] and needed `gh pr merge --admin` despite carrying a real Ally approval. That review is still DISMISSED on the PR today; replaying the pre-cleanup state through the new selector retains 5146534396 and dismisses 5146530564 instead. scripts/ally-review-de-dupe.mjs makes the choice mechanically rather than by hand. It matches candidates on the body's `Reviewed head:` attestation, never commit_id — GitHub re-anchors commit_id forward on APPROVED reviews when the branch is updated (BLO-34581), and APPROVED is the state being de-duplicated, so commit_id both admits reviews that never read this head and hides ones that did. It refuses outright on conflicting verdicts (a blocker-driven supersession, BLO-19778, not a duplicate) and on unorderable input, since an unestablished "newest" is the exact condition that produced the incident. All nine guards in the selector carry a failing mutation. A tenth, a 40-hex shape check on the head argument, survived mutation testing: comparing against attestedHead's output already rejects every malformed head, so it could not change an outcome and was removed rather than shipped as a comment. The CLI entrypoint is covered by a spawn test because the first cut called a bare isMainModule(); its default import.meta.url is evaluated in the module that defines it, so it compared argv[1] against check-ally-review-consistency's URL, never matched, and exited 0 having printed nothing.

allyblockcast · 2026-09-29T07:23:05Z

🔗 Paperclip issue: BLO-19778
🔗 Paperclip issue: BLO-34581
🔗 Paperclip issue: BLO-32837

allyblockcast · 2026-09-29T07:27:57Z

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched under opencode_k8s).
Reviewed head: 97af625

The core fix is right and the regression test pins it: selectDuplicateDismissals retains the newest and dismiss never contains it. My findings are about the guard around that choice — the conflict refusal keys on a field that cannot discriminate on this repo's dominant PR class.

Critical Issues (1)

[gstack/review + native-codex] scripts/ally-review-de-dupe.mjs:98 — the conflicting-verdicts refusal keys on state alone, so it is blind on App-authored PRs, where every Ally self-review is COMMENTED. Two runs reaching opposite conclusions at one head are classified duplicate and the winner is picked by clock order — the exact outcome stated property 2 (line 29) says it refuses, on the exact example it cites (BLO-19778, #876).
- Reproduced against the module at this head: older COMMENTED with ### Critical Issues (2), newer COMMENTED clean → reason: "duplicate", retain: 222, dismiss: [111]. The dismissal target is the review carrying the blocking findings.
- Why App-authored PRs are the whole problem: GitHub bars a PR's author from APPROVE, so clean and blocking self-reviews are both COMMENTED and states.size === 1. On a human-authored PR the same disagreement is APPROVED vs COMMENTED and is caught. This PR is itself App-authored, so it is in the blind class.
- The only thing standing between this and a discarded blocker is the claim at .planning/ally-agent/AGENTS.md:279 that a COMMENTED review "cannot be dismissed through the API at all". I could not confirm that: GitHub's dismissals endpoint documents no state restriction, and the docs do state one explicitly for the adjacent delete-pending-review endpoint. A safety property resting on an unmeasured API behavior is the failure shape this module was written to prevent.
- Recommendation: make the conflict test substantive, not state-shaped — hasBlockingFindings is already exported from check-ally-review-consistency.mjs and is the discriminator I1/I2 use. Refuse when candidates disagree on state or on blocking findings. Separately, return a distinct non-actionable reason for an all-COMMENTED candidate set so the code says what line 279 says, instead of emitting a dismiss list the documented procedure ("Dismiss exactly the ids it lists") would feed to the API.

Important Issues (1)

[pr-review-toolkit: code] scripts/ally-review-de-dupe.mjs:75 — exactHeadAppReviews matches on attestedHead, which takes the first match and requires neither the consolidated-review heading nor attestation uniqueness. canonicalReviewHead is exported and is the helper that encodes this repo's actual canonicality contract (exactly one heading + exactly one attestation); the guard reports anything else as an I3 "not canonical" violation at check-ally-review-consistency.mjs:427.
- Reproduced at this head: a body with two attestations, and a body with an attestation but no heading, are both canonicalReviewHead === null yet both admitted as candidates. Pairing a genuine canonical review with a newer non-canonical one yields retain: 444 (non-canonical) and dismiss: [555] — the genuine review is the dismissal target.
- This is the hazard the comment at check-ally-review-consistency.mjs:116-123 says is absorbed by canonicalReviewHead ("a fenced paste is still read here as an attestation ... canonicalReviewHead requires exactly one, so it returns null"). Bypassing it re-opens the divergence that comment exists to close.
- Recommendation: filter candidates on canonicalReviewHead(review.body) === head. It keeps the deliberate body-over-commit_id property intact (property 1 is unaffected) while narrowing the set to reviews the rest of the system agrees are Ally reviews.

Suggestions (2)

[pr-review-toolkit: types] scripts/ally-review-de-dupe.mjs:46,52 — OPERATIVE_EXCLUDED_STATES and reviewState re-implement isDismissedOrPending/reviewState, which are private in check-ally-review-consistency.mjs:154-160. Given how much care that module takes over two parsers agreeing (the NOT_INDENTED_CODE comment), exporting and reusing the existing predicate would stop these two drifting apart later.
[pr-review-toolkit: tests] scripts/ally-review-de-dupe.test.mjs — the conflict test establishes its conflict via differing state (APPROVED vs COMMENTED), so it passes without exercising the blind spot above. A two-COMMENTED-differing-findings case would have failed on this head, and is the fixture to add alongside the fix.

Strengths

The regression test asserts both input orderings and asserts the negative (!dismiss.some(r => r.id === NEWER.id)) rather than only the positive — that is the property that actually protects reviewDecision, and order-independence is where a clock-ordered selection would quietly regress.
Commit-id avoidance is enforced by fixture design, not by comment: every fixture carries a deliberately wrong commit_id, so a regression to commit_id matching fails loudly.
The isMainModule(process.argv[1], import.meta.url) comment names a real, non-obvious default-parameter trap and the CLI test covers the silent-no-op it produced — a failure library tests structurally cannot see.
unorderable fails closed rather than guessing, and the incident evidence (ids, timestamps, the resulting latestOpinionatedReviews: []) is recorded precisely enough to re-verify.

Recommended Action

Fix Critical issues before merge.
Address Important issues this cycle.
Consider Suggestions opportunistically.

kkroo · 2026-09-29T12:25:05Z

Lease: kkroo drive session (Omar, 637c9c), about 60 min, at 97af6257. Taking Ally's Critical/Important at this head.

🤖 Generated with Claude Code

…LO-32837) Addresses Ally's review of 97af625 (Critical 1, Important 1). Critical: the conflicting-verdicts refusal keyed on `state` alone, so on App-authored PRs (where GitHub bars the author from APPROVE and every Ally self-review is COMMENTED) an older blocking review and a newer clean one were classified `duplicate` and the blocking review was the dismissal target. The verdict key is now state plus the guard's blocking-verdict test (hasBlockingFindings || hasStillPresentDisposition, both already exported by check-ally-review-consistency.mjs, which is left unmodified). A same-verdict all-COMMENTED set now returns a distinct, non-actionable `commented-only` reason with an empty dismiss list, so the safety property no longer rests on the unmeasured claim that the dismissals API rejects COMMENTED reviews. AGENTS.md Step 6 drops that claim and documents the new reason. Important: candidates were matched with attestedHead (first match, no heading or uniqueness check), which admitted bodies I3 reports as "not canonical" and let a newer non-canonical review be retained while the genuine one was dismissed. Candidates are now matched with canonicalReviewHead; the body-over-commit_id property is unchanged. Verification: node --test on ally-review-de-dupe, check-ally-review- consistency and ally-agent-idempotency-contract: 172/172 pass (5 new tests, including Ally's two reproductions in both input orderings). Mutation-checked, each reverted individually: state-only verdict key (2 red), still-present disposition dropped (1 red), commented-only refusal dropped (1 red), canonicalReviewHead -> attestedHead (2 red), whole fix reverted to 97af625 (5 red). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

kkroo · 2026-09-29T12:32:01Z

@ally please re-review head d02d505.

Critical (scripts/ally-review-de-dupe.mjs:98, state-only conflict test blind on App-authored PRs): the verdict key is now state plus the guard's blocking-verdict test (hasBlockingFindings || hasStillPresentDisposition, both already exported; check-ally-review-consistency.mjs is unmodified). A same-verdict all-COMMENTED set returns a distinct non-actionable commented-only reason with an empty dismiss, so safety no longer rests on the dismissals API rejecting COMMENTED; AGENTS.md Step 6 drops that unconfirmed claim. Verified by your reproduction (older COMMENTED Critical 2 / newer clean -> conflicting-verdicts, dismiss: [], both orderings), a still-present-disposition case, and an all-COMMENTED clean pair -> commented-only. Mutants killed: state-only key (2 red), still-present dropped (1 red), commented-only dropped (1 red).
Important (scripts/ally-review-de-dupe.mjs:75, attestedHead admits non-canonical bodies): candidates are matched with canonicalReviewHead; the body-over-commit_id property is unchanged. Verified by your reproduction (genuine 555 + newer double-attested 444 -> no-duplicate, retain 555, dismiss: []) and an exclusion test for double-attestation and no-heading bodies. Mutant canonicalReviewHead -> attestedHead killed (2 red).

node --test scripts/ally-agent-idempotency-contract.test.mjs scripts/check-ally-review-consistency.test.mjs scripts/ally-review-de-dupe.test.mjs: 172/172 pass. Reverting the whole fix to 97af625: 5 red.

🤖 Generated with Claude Code

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched under opencode_k8s).
Reviewed head: d02d505

Both prior findings are fixed, and I verified the fix rather than the claim: node --test scripts/ally-review-de-dupe.test.mjs against this head is 21/21 pass, including the two new blind-spot fixtures. The remaining finding is in the layer neither pipeline had reached before — the CLI is the entry point AGENTS.md documents, and on realistic input it returns a different answer from the library those 21 tests exercise.

Prior Findings Dispositioned (2)

prior:97af625 critical 1 — fixed — scripts/ally-review-de-dupe.mjs:124 — the verdict key is now ${reviewState(review)}/${isBlocking(review)}, with isBlocking at :62-64 composing the guard's own hasBlockingFindings || hasStillPresentDisposition. Confirmed on the module at this head: two COMMENTED reviews differing only in body (### Critical Issues (2) vs clean) now return conflicting-verdicts with dismiss: [] in both orderings — the case that previously returned duplicate and targeted the blocker-carrying review. The commented-only branch at :128-130 closes the second half: the all-COMMENTED set now returns an empty dismiss instead of resting on an unconfirmed claim about the dismissals API, and AGENTS.md drops that claim.
prior:97af625 important 1 — fixed — scripts/ally-review-de-dupe.mjs:94 — candidates match on canonicalReviewHead(review?.body) === head. Confirmed at this head: a double-attested body and a no-heading body are both excluded, and the genuine-canonical + newer-non-canonical pair returns no-duplicate retaining the genuine review, where it previously retained the non-canonical one and dismissed the genuine one.

Critical Issues (1)

[native-codex + pr-review-toolkit: errors] scripts/ally-review-de-dupe.mjs:152 — chunks.join("") decodes each stdin chunk independently. process.stdin async-iterates Buffers, so a multi-byte UTF-8 sequence split across a chunk boundary becomes replacement characters — and every Ally body carries em-dashes in the mandatory ## Ally — Consolidated PR Review heading and in — still-present — disposition markers. gh api .../reviews --paginate on a PR with a few Ally reviews clears the 64 KiB boundary easily, so this is reachable on the exact input the documented invocation pipes in.
- Reproduced against this head, both failure directions. Same JSON, once through the CLI and once in-process:
  - Fails closed — heading em-dash at byte 65534, two APPROVED duplicates: CLI → no-duplicate, retain 111 (the older); library → duplicate, retain 222, dismiss [111]. The newer review is silently dropped from the candidate set, so the real duplicate is never remedied.
  - Fails open — — still-present — em-dash at byte 65534, older APPROVED carrying the disposition, newer clean: CLI → duplicate, dismiss: [111]; library → conflicting-verdicts, dismiss: []. The corrupted marker stops matching STILL_PRESENT_DISPOSITION_RE, isBlocking collapses to false, and the two verdicts merge. AGENTS.md says "Dismiss exactly the ids it lists" — so this hands the operator the review carrying the blocker. That is precisely the outcome property 2 (:29-36) exists to refuse, defeated on the documented path.
- The commented-only branch absorbs the all-COMMENTED variant of this (verified: conflicting-verdicts → commented-only, dismiss still []), which is a genuine second line of defence — but it does not cover the APPROVED case above, which is the state this module was written for.
- Recommendation: process.stdin.setEncoding("utf8") before the loop, or Buffer.concat(chunks).toString("utf8") at :152. One line either way.
- The test cannot currently see it: scripts/ally-review-de-dupe.test.mjs:210 feeds JSON.stringify([OLDER, NEWER]) — well under one chunk, so the CLI is only ever exercised single-chunk. A fixture >64 KiB with a multi-byte character at the boundary is the regression test, and it is the same class of gap as the isMainModule silent no-op this file already calls out at :167-170.

Important Issues (0)

Suggestions (2)

[gstack/review] scripts/ally-review-de-dupe.mjs:109-113 and the AGENTS.md Step 6 prose — "dismissing one repairs nothing" is not quite right, and the gap it leaves is worth naming. isDismissedOrPending excludes DISMISSED from operativeAllyReviews (check-ally-review-consistency.mjs:158-161), and I1 fires on reviews.length > 1 with no exemption for clean App self-reviews — so dismissing a COMMENTED duplicate would clear the I1 violation. What is true is the narrower claim: it repairs nothing about reviewDecision, and the hazard that motivates the module therefore does not apply either way. I asked for this branch in the prior review and the fail-closed direction is right; the note is only that on this repo's dominant PR class (App-authored ⇒ every Ally self-review COMMENTED) a duplicate now has no tool-supported remedy at all and rests on the baseline file until the head moves. Saying that out loud in the docstring would stop the next reader re-deriving it.
[pr-review-toolkit: types] scripts/ally-review-de-dupe.mjs:55-59 — OPERATIVE_EXCLUDED_STATES and reviewState still re-implement the private isDismissedOrPending/reviewState in check-ally-review-consistency.mjs:154-161. Carried forward unchanged from the prior review; now that four other helpers are imported from that module, exporting these two as well would leave nothing for the two copies to drift on.

Strengths

The two new fixtures test the property, not the implementation: "refuses when two COMMENTED reviews disagree on blocking findings" asserts both orderings and asserts dismiss is empty with a comment naming why ("the review carrying the blockers must survive"). That is the assertion that would have caught the original defect.
exactHeadAppReviews:84-88 documents a guard that was removed because mutation-testing showed it could not change an outcome, and says so. Recording a negative result where a plausible-looking check would have sat is rarer and more useful than adding one.
The head-shape fixtures ("b108db2", "", null, HEAD + "00") pin the "never guess which full SHA an abbreviation meant" property directly, rather than trusting the comment that asserts it.
Every fixture still carries a deliberately wrong commit_id, so the body-over-commit_id property is enforced by construction — a regression to commit_id matching fails loudly rather than passing for the wrong reason.

Recommended Action

Fix Critical issues before merge.
Consider Suggestions opportunistically.

kkroo · 2026-09-29T13:17:30Z

Lease: kkroo drive session (Omar, 637c9c), about 60 min, at d02d505a. Taking Ally's Critical/Important at this head.

🤖 Generated with Claude Code

Addresses Ally's Critical at d02d505 (scripts/ally-review-de-dupe.mjs:152): the CLI joined stdin Buffers with chunks.join(""), which decodes each 64 KiB chunk on its own. An em-dash split across a chunk boundary became U+FFFD, so on the documented `gh api .../reviews --paginate` input the CLI disagreed with the library it wraps: - fail open: a corrupted "-- still-present --" marker (em-dash in the real body) made isBlocking false, merged two APPROVED verdicts, and listed the review carrying the blocker for dismissal; - fail closed: a corrupted consolidated-review heading dropped the newer review from the candidates, so the real duplicate was never remedied. Fix: Buffer.concat(chunks).toString("utf8"), decoding once. Verified: new CLI test pads a >64 KiB review list so a 3-byte em-dash straddles the first chunk boundary, for both directions. Against the old chunks.join("") it fails (fail-open case: actual duplicate, expected conflicting-verdicts; fail-closed case run alone: actual no-duplicate, expected duplicate), matching Ally's reproduction. With the fix: node --test scripts/ally-agent-idempotency-contract.test.mjs scripts/check-ally-review-consistency.test.mjs scripts/ally-review-de-dupe.test.mjs -> 173/173 pass; the new test passed 5/5 reruns. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

kkroo · 2026-09-29T13:24:58Z

@ally please re-review head 66b4db9.

Critical (scripts/ally-review-de-dupe.mjs:152, per-chunk stdin decode): the CLI now decodes once with Buffer.concat(chunks).toString("utf8") instead of chunks.join(""). Regression test CLI > decodes stdin as one UTF-8 stream, not per chunk pads a >64 KiB review list (a non-Ally filler review) so a 3-byte em-dash starts at byte 65534, straddling the first chunk boundary, in both directions you reproduced: fail-open (still-present marker, expects conflicting-verdicts with dismiss: []) and fail-closed (newer review's heading, expects duplicate retaining the newer). Mutation check: with the old chunks.join("") it fails (actual duplicate vs expected conflicting-verdicts; fail-closed case run alone: actual no-duplicate vs expected duplicate). With the fix, node --test scripts/ally-agent-idempotency-contract.test.mjs scripts/check-ally-review-consistency.test.mjs scripts/ally-review-de-dupe.test.mjs is 173/173 pass; the new test passed 5/5 reruns.

Your review listed one Critical and zero Important; the two Suggestions are left as-is.

🤖 Generated with Claude Code

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; nested CLI not launched under opencode_k8s).
Reviewed head: 66b4db9

The prior Critical is fixed, and I verified the guard rather than the claim: I checked out this head, reverted the one-line fix, and confirmed the new test fails — in both directions independently, which is the check that separates a regression test from documentation.

Prior Findings Dispositioned (1)

prior:d02d505 critical 1 — fixed — scripts/ally-review-de-dupe.mjs:155 — the CLI now decodes once, after concatenating: JSON.parse(Buffer.concat(chunks).toString("utf8") || "[]"). process.stdin async-iterates Buffers, so this is boundary-agnostic by construction — a multi-byte sequence split across any chunk boundary is reassembled before decoding, rather than each chunk being decoded alone into U+FFFD.
- Mutation-verified at this head, one mutation per run. Reverting only that expression to chunks.join(""):
  - fail-open case — node --test scripts/ally-review-de-dupe.test.mjs → actual: 'duplicate' vs expected: 'conflicting-verdicts'. That is the direction that mattered: the corrupted — still-present — marker stops matching STILL_PRESENT_DISPOSITION_RE (check-ally-review-consistency.mjs:128-131), isBlocking collapses to false, the two verdicts merge, and the emitted dismiss targets the review carrying the blocker.
  - fail-closed case — both cases share one it() and a for loop, so the second never runs once the first fails. I re-ran with the fail-open case removed to check it guards independently: actual: 'no-duplicate' vs expected: 'duplicate'. It does.
- Restored the tree and re-ran the three suites you cite: node --test scripts/ally-agent-idempotency-contract.test.mjs scripts/check-ally-review-consistency.test.mjs scripts/ally-review-de-dupe.test.mjs → 173 pass, 0 fail, matching your count exactly. The de-dupe suite alone is 22/22, and the new test passed 3/3 reruns here.
- The padding arithmetic is pinned rather than assumed: assert.equal(offsetOf(input), boundary - 2) at :247 fails loudly if the filler ever stops landing the em-dash on the boundary, so this cannot decay into a test that passes on input it was not measuring.

Critical Issues (0)

Important Issues (0)

Suggestions (3)

[pr-review-toolkit: tests] scripts/ally-review-de-dupe.test.mjs:225 — const boundary = 64 * 1024 encodes an unasserted assumption about Node's pipe read size, and the test's detection power rests on it entirely. With the fix in place the assertions hold under any chunking, so if that internal ever changes the test keeps passing with and without the fix — it stops guarding silently, which is the one failure mode this repo mutation-tests to avoid. Nothing observable from outside the child process can assert "stdin arrived in more than one chunk", so this may simply be the cost of testing the real CLI path; worth a line in the comment at :221-224 saying the 64 KiB figure is load-bearing for detection, not just for construction.
[pr-review-toolkit: types] scripts/ally-review-de-dupe.mjs:55-59 — carried forward unchanged: OPERATIVE_EXCLUDED_STATES and reviewState still re-implement the private isDismissedOrPending/reviewState in check-ally-review-consistency.mjs:154-161. Five helpers are now imported from that module; exporting these two as well would leave nothing for the copies to drift on. Noted as deliberately deferred.
[gstack/review] scripts/ally-review-de-dupe.mjs:109-113 — carried forward unchanged: the commented-only docstring still says "dismissing one repairs nothing". The narrower claim is the true one — it repairs nothing about reviewDecision, though it would clear the I1 violation, since isDismissedOrPending excludes DISMISSED from operativeAllyReviews. Noted as deliberately deferred.

Strengths

The fix is the boundary-agnostic one rather than the boundary-aware one. setEncoding("utf8") would also have worked, but Buffer.concat makes the property structural — there is no chunk size at which it can be wrong — and the comment at :152-154 names the mechanism instead of restating the diff.
The fixture pads with a non-Ally review (user: {login: "octocat"}) rather than inflating a candidate's body, so the padding cannot perturb the candidate set the assertion is about. The test measures the decode, not a side effect of its own setup.
Both failure directions are fixtures, not prose. The fail-open case is the one that hands an operator the blocker-carrying review under the documented "Dismiss exactly the ids it lists" procedure, and it is pinned with dismiss: [] — asserting the negative, which is the property that actually protects the blocker.
Edge paths fail in the safe direction, which I checked directly at this head: empty stdin → reason: "none" with an empty dismiss; malformed stdin → non-zero exit and a stack trace. Neither can emit a dismissal list, so a failed upstream gh api degrades to "dismiss nothing" rather than to a wrong dismissal.
.github/workflows/pr.yml:571-581 wires the suite into CI in the same PR, with a comment recording why I1 satisfaction was not sufficient on paperclipai#3281. The guard and the reason it exists land together.

Recommended Action

No blocking changes requested.
Merge once the remaining required CI checks finish green.

allyblockcast · 2026-09-30T02:17:06Z

This PR is clean at its current head but still has an outstanding code-owner review request (kkroo). GitHub does not enforce CODEOWNERS on this repository, so the landing routine holds it here rather than enqueuing it.

allyblockcast Bot requested a review from kkroo as a code owner September 29, 2026 07:22

allyblockcast Bot commented Sep 29, 2026

View reviewed changes

kkroo added this pull request to the merge queue Sep 29, 2026
Any commits made after this event will not be merged.

Navigation Menu

fix(ally): retain the newest duplicate review, never dismiss it (BLO-32837) - #2105

fix(ally): retain the newest duplicate review, never dismiss it (BLO-32837)#2105
allyblockcast[bot] wants to merge 3 commits into
masterfrom
ally/blo-32837-dedupe-retain-newest

allyblockcast Bot commented Sep 29, 2026 •

edited

Loading

Uh oh!

allyblockcast Bot commented Sep 29, 2026

Uh oh!

allyblockcast Bot commented Sep 29, 2026 •

edited

Loading

Uh oh!

allyblockcast Bot left a comment

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

allyblockcast Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

What Changed

Verification

Risks

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Sep 29, 2026

Uh oh!

allyblockcast Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (1)

Important Issues (1)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (2)

Critical Issues (1)

Important Issues (0)

Suggestions (2)

Strengths

Recommended Action

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

kkroo commented Sep 29, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (0)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

allyblockcast Bot commented Sep 29, 2026 •

edited

Loading

allyblockcast Bot commented Sep 29, 2026 •

edited

Loading