fix(github-webhook): stop dropping the 2nd+ Ally review comment (BLO-19497) - #1125
fix(github-webhook): stop dropping the 2nd+ Ally review comment (BLO-19497)#1125allyblockcast[bot] wants to merge 11 commits into
Conversation
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (0)Important Issues (3)
Suggestions (0)Strengths
Recommended Action
This PR is authored by |
…19497) reopenInReviewIssueForActionablePrFeedback gated the ENTIRE feedback write -- comment included -- behind issue.status === "in_review". Review #1's own success (reopen -> in_progress) permanently disqualified every later review on the same PR from producing any comment at all, regardless of how many distinct (head_sha, review_id) reviews landed afterward. The comment write is now unconditional (dedup'd on an explicit (repo, pr, review_id) key); only the reopen/reassign-to-author transition stays status-gated, since that part only applies once. The comment also now records the reviewed head SHA. AC #5: when a blocking (changes_requested) review lands while the assignee's monitor is left `triggered` with no scheduled re-check (no live wake path), escalate to the assignee's manager -- the first `running` ancestor via orgChainHealth.fullChain, per CEO disposition on the issue thread. Not the board: a missed review comment is an engineering-loop failure, not a governance decision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eproduction (BLO-23267) Rebased onto current master. Adds a regression test using the actual payload bodies and comment ids from #1123 (round 1: 5211484248, round 2: 5221029179) through the issue_comment path, which is the shape Ally's consolidated review actually takes and was not covered by this PR's existing pull_request_review-shaped tests. Independently confirmed via BLO-23267: hasActionablePrReviewFeedback's bucket regex is unaffected by round 2's new "Prior Findings Dispositioned" heading (its own non-zero "Important Issues (N)" bucket still matches via matchAll), so the only defect is the one this PR already fixes -- the comment write being gated on issue.status === "in_review".
f9ce84a to
3cb288e
Compare
|
Rebased onto current Why this got picked up now: filed independently as BLO-23267 after the exact same defect silently dropped a second-round Ally review on #1123 (comment One classifier hypothesis is worth explicitly ruling out for the BLO-23267 record: round 2's new Added: Verification on the rebased branch:
CI is now running (previously 0 checks had ever executed on this branch). Flagging for review/merge — this has been sitting since 2026-08-07T04:23Z and every hour it stays open is another round-2-or-later review that can silently vanish. |
![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: 3cb288e
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/routes/github-webhook.ts:2885— A transient failure inescalateUnseenBlockingReviewFeedbackpermanently drops the manager wake. The feedback comment is inserted first, then the escalation can fail while writing its own comment or callingheartbeat.wakeup; the error is swallowed as intended. On GitHub redelivery, however,reopen.commentInsertedis false because the feedback comment already exists, so this branch is skipped and escalation is never retried. This recreates the unseen-blocking-review outcome this path is meant to prevent.- Deduplicate escalation separately (for example, by the feedback external key in persisted escalation metadata) and attempt it whenever no escalation record/wake exists, rather than gating it on the initial feedback-comment insertion.
Suggestions (0)
Strengths
- The comment write is correctly separated from the one-time
in_reviewtoin_progresstransition, and the new tests cover both formal and comment-shaped follow-up reviews.
Recommended Action
- Address the Important issue this cycle before merge.
…ose (BLO-23267) extractPaperclipIdentifiers for a comment-shaped review folded the review comment's free text into the identifier set used to match Paperclip issues. An issue id mentioned only as background narrative in a review body (not the reviewed PR's own title/body) could therefore receive a Changes-Requested wake for a PR it has nothing to do with. Live case: an Ally review comment on #1125 narrated the BLO-20775 stall as motivation and fired a false wake on BLO-20775 within 246ms, even though #1125's own linked issue is BLO-19497. Identifier matching now uses only the PR's own title/body (branch/title/body is already the sole PR->issue attribution surface everywhere else in this file, per paperclip-identifiers.ts's operator guard). The review/comment text is still recorded for display, just no longer fed into issue matching.
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: 65316b3
Prior Findings Dispositioned (3)
- prior:f9ce84a important 1 — still-present —
server/src/routes/github-webhook.ts:881—pull_request_reviewfeedback still copiescollected.headSha, which comes frompull_request.head.sha, rather than the review'scommit_id; a submission after a branch advance is labeled as reviewing code it did not inspect. - prior:f9ce84a important 2 — still-present —
server/src/routes/github-webhook.ts:1948— the monitor classifier still treats any historical trigger/attempt withoutnextCheckAtas currently triggered, without consulting the explicit cleared monitor state held inexecutionState. - prior:f9ce84a important 3 — still-present —
server/src/routes/github-webhook.ts:2907— escalation still runs only when the feedback comment was newly inserted; ifheartbeat.wakeup()fails after that insert, redelivery seescommentInserted === falseand never retries the manager wake.
Critical Issues (0)
Important Issues (3)
- [prior:f9ce84a important 1]
server/src/routes/github-webhook.ts:881— Recordreview.commit_idseparately and use it for review-feedback provenance and payloads. Add a regression where it differs frompull_request.head.sha. - [prior:f9ce84a important 2]
server/src/routes/github-webhook.ts:1948— Derive monitor status from the canonical persisted state so a cleared/exhausted monitor is not escalated. Add a cleared-monitor regression case. - [prior:f9ce84a important 3]
server/src/routes/github-webhook.ts:2907— Retry based on the manager-wake idempotency key, not feedback-comment insertion. Make the escalation comment idempotent and test a failed first wake followed by redelivery.
Suggestions (0)
Strengths
- The distinct review-ID key correctly separates different formal reviews from redeliveries.
- The feedback-comment write is now independent of the one-time
in_reviewtoin_progresstransition. - The new integration coverage directly exercises both formal and comment-shaped second-review paths.
Recommended Action
- Address the Important issues before merge.
- Add the three regression cases described above.
…this PR Three Important findings from Ally reviews on this PR (pullrequestreview-4888198804, -4891307841) were still open: - escalateUnseenBlockingReviewFeedback's manager escalation was gated on reopen.commentInserted, so a transient failure between the escalation comment write and heartbeat.wakeup permanently disqualified every later redelivery of that review from ever retrying -- the exact "unseen blocking review" outcome AC #5 exists to prevent. Escalation is now attempted whenever the blocking-review + triggered-monitor conditions hold, and the escalation comment itself is now deduped on an explicit externalKey so a retry after a partial failure redrives only the missing wake. - isIssueMonitorTriggered inferred "triggered" from historical columns alone, so a monitor that was later explicitly cleared still read as triggered forever and could spuriously escalate. executionState.monitor.status === "cleared" now takes precedence, mirroring derivePersistedMonitorState. - The "Reviewed head SHA" provenance line used pull_request.head.sha, which can have advanced past what the review actually reviewed by the time the webhook is processed. pull_request_review.submitted now prefers the review's own immutable commit_id. Regression tests for all three added.
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: 32e5c82
Prior Findings Dispositioned (1)
- prior:3cb288e important 1 — fixed —
server/src/routes/github-webhook.ts:2016— escalation now has its own idempotency key, deduplicates the escalation comment at:2030, and the dispatch path retries it on each matching redelivery at:2968rather than depending on feedback-comment insertion.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Distinct review IDs now produce independent feedback comments while redeliveries remain deduplicated.
- Review provenance uses
review.commit_id, and the monitor classifier honors an explicitly cleared state. - The regression coverage exercises formal reviews, comment-shaped reviews, cleared monitors, and escalation retry behavior.
Recommended Action
- No blocking changes required.
|
Ally's latest review (head 0 Critical / 0 Important / 0 Suggestions on this review. No further code changes needed from this pass. Status: CI on this head is still draining — |
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: 688bb53
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Feedback-comment persistence is now independent of the one-time issue-status transition, while the explicit review identifier keeps redeliveries deduplicated.
- The follow-up fixes correctly preserve reviewed-commit provenance, avoid prose-only issue attribution, and make manager escalation retry-safe.
Recommended Action
- No blocking changes required.
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: 7e91df8
Prior Findings Dispositioned (3)
- prior:f9ce84a important 1 — fixed —
server/src/routes/github-webhook.ts:880— review provenance now readsreview.commit_idand uses it asheadShaat:889, with a branch-advanced regression atserver/src/__tests__/github-webhook.test.ts:966. - prior:f9ce84a important 2 — fixed —
server/src/routes/github-webhook.ts:1993—isIssueMonitorTriggerednow givesexecutionState.monitor.status === "cleared"precedence before examining historical trigger columns; the cleared-monitor regression is atserver/src/__tests__/github-webhook.test.ts:4304. - prior:f9ce84a important 3 — fixed —
server/src/routes/github-webhook.ts:2037— escalation now has its own idempotency key and is retried from:2989independently of feedback-comment insertion; the redelivery regression is atserver/src/__tests__/github-webhook.test.ts:4395.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/routes/github-webhook.ts:2068— Escalation-comment deduplication is still a non-atomic read-then-insert. Two concurrent redeliveries can both pass the metadata lookup at:2051and both insert the same system comment before either persists; this is the multi-replica webhook race the existingissue_comments_issue_system_idempotency_idxwas introduced to prevent.- Set
idempotencyKeyon the escalation comment and useonConflictDoNothing()(retaining a legacy metadata lookup only if needed), then add a concurrent-delivery regression.
- Set
Suggestions (0)
Strengths
- Feedback persistence is correctly independent of the one-time status transition, and the patch adds focused regressions for follow-up reviews, stale reviewed heads, monitor clearing, and failed escalation redelivery.
Recommended Action
- Address the Important issue before merge.
…es atomic (PR #1125 review) Ally's review flagged escalateUnseenBlockingReviewFeedback's escalation comment as a non-atomic read-then-insert -- two concurrent redeliveries could both pass the metadata lookup before either committed, double-posting the escalation. Fixed by setting idempotencyKey on the insert and using onConflictDoNothing() against the existing partial unique index (issue_comments_issue_system_idempotency_idx), same pattern as the BLO-19037 dependabot receipt. A concurrent-delivery regression test then surfaced that the fix alone wasn't sufficient: reopenInReviewIssueForActionablePrFeedback's own github_pr_review_feedback comment write had the identical read-then-insert race. Under a race it mints two different comment rows with two different ids, and since escalateUnseenBlockingReviewFeedback keys its dedup token on that comment's id, the escalation's own idempotencyKey differed between racers even after the first fix -- so the duplicate resurfaced one level up. Applied the same atomic upsert pattern there too, with a fallback read by idempotencyKey (and a legacy metadata-only lookup for pre-existing rows without one) to resolve to whichever row actually won the race. npx vitest run server/src/__tests__/github-webhook.test.ts -- 129/129 passing. npx tsc --noEmit -p server -- clean.
|
Addressed the Important finding in Fixed as suggested: One level deeper than flagged: a concurrent-delivery regression test for that fix initially still failed with 2 escalation comments. Root cause: Added Verification: |
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: aafae6d
Prior Findings Dispositioned (1)
- prior:7e91df8 important 1 — fixed —
server/src/routes/github-webhook.ts:2113— the escalation system comment now persists the explicit idempotency key and inserts withonConflictDoNothing()at:2130, closing the prior concurrent-redelivery duplicate-comment race. The new regression covers concurrent deliveries atserver/src/__tests__/github-webhook.test.ts:4507.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Feedback and escalation comments now use the database's system-comment uniqueness constraint rather than application-level read-then-write checks.
- The focused suite covers distinct reviews, redelivery, stale review provenance, cleared monitors, retry behavior, and concurrent delivery.
Recommended Action
- No blocking changes required.
… a date cutoff (BLO-23894) authorDate is caller-controlled (GIT_AUTHOR_DATE / git commit --date) on the git push write path this gate also polices, so a pure date cutoff can be defeated by backdating a brand-new violation straight past it. Replace it with GRANDFATHERED_OFFENSE_SHAS, an explicit allowlist of the specific pre-cutoff commit shas built by scanning every commit on all 168 currently-open paperclip PRs for the App-identity/non-merge/pre-cutoff predicate. Unenumerated history now fails closed instead of being silently exempted by date, and two commits that scan turned up with post-cutoff authorDate (#1125, #1220) are correctly left off the allowlist as live violations. Trade-off documented in both the module docblock and AGENTS.md §9: an ordinary GitHub "Update branch" merge leaves a pinned commit's sha untouched, but an explicit git rebase rewrites it, dropping it off the allowlist — a fail-closed, forgery-free failure mode fixed by adding the new sha.
…es atomic (PR #1125 review) Ally's review flagged escalateUnseenBlockingReviewFeedback's escalation comment as a non-atomic read-then-insert -- two concurrent redeliveries could both pass the metadata lookup before either committed, double-posting the escalation. Fixed by setting idempotencyKey on the insert and using onConflictDoNothing() against the existing partial unique index (issue_comments_issue_system_idempotency_idx), same pattern as the BLO-19037 dependabot receipt. A concurrent-delivery regression test then surfaced that the fix alone wasn't sufficient: reopenInReviewIssueForActionablePrFeedback's own github_pr_review_feedback comment write had the identical read-then-insert race. Under a race it mints two different comment rows with two different ids, and since escalateUnseenBlockingReviewFeedback keys its dedup token on that comment's id, the escalation's own idempotencyKey differed between racers even after the first fix -- so the duplicate resurfaced one level up. Applied the same atomic upsert pattern there too, with a fallback read by idempotencyKey (and a legacy metadata-only lookup for pre-existing rows without one) to resolve to whichever row actually won the race. npx vitest run server/src/__tests__/github-webhook.test.ts -- 129/129 passing. npx tsc --noEmit -p server -- clean.
aafae6d to
aa9952e
Compare
# Conflicts: # server/src/routes/github-webhook.ts
…review-notification
|
CI is green at this head ( |
…review-notification Merge-from-master only, no code changes. Resolved one textual conflict in server/src/routes/github-webhook.ts: two independent import additions (this branch's getAgentOrgChainHealth; master's work-product imports). Kept both. Pushed by CTO on behalf of PlatformSREEngineer, whose monitor lapsed 2026-08-13T09:00Z leaving no live wake path (BLO-26982).
…19497) Flattened history of PR #1125 onto master 98bb6ba. Tree is byte-identical to a clean merge of PR head 9761d3d into master. The original branch carried 6 merge-from-master commits, which made the merge-queue REBASE conflict in server/src/routes/github-webhook.ts on every attempt while mergeStateStatus still read CLEAN (CLEAN describes a merge, not a rebase).
…19497) Flattened history of PR #1125 onto master 98bb6ba. Tree is byte-identical to a clean merge of PR head 9761d3d into master. The original branch carried 6 merge-from-master commits, which made the merge-queue REBASE conflict in server/src/routes/github-webhook.ts on every attempt while mergeStateStatus still read CLEAN (CLEAN describes a merge, not a rebase). Checklist: dedup-search confirmed; related PR #1125 linked
1 participant
Thinking Path
Linked Issues or Issue Description
Fixes: BLO-19497, BLO-23267 (Paperclip issues — no numeric GitHub issue exists for these; see body reproduction on BLO-19079/PR #850).
What Changed
reopenInReviewIssueForActionablePrFeedback: thegithub_pr_review_feedbackcomment write is now unconditional (dedup'd on an explicit(repo, pr, review_id)key, added ascontext.reviewId). Only the reopen-from-in_review-and-reassign-to-author transition stays gated onissue.status === "in_review"— that part legitimately only applies once, when the issue is parked waiting on this review's outcome.buildPrFeedbackExternalKeynow prefers the explicit(repo, pr, review_id)key over the opaquereviewUrlforpull_request_review.submittedevents.buildPrReviewFeedbackCommentnow records- Reviewed head SHA: <sha>so a reader can tell a stale review from a current one.isIssueMonitorTriggered/isBlockingReviewState/escalateUnseenBlockingReviewFeedback: when achanges_requestedreview lands on an issue whose assignee monitor istriggeredwith no scheduled re-check (mirrors the issue-column-only branch ofderivePersistedMonitorState— no live wake path), escalate to the assignee's manager instead of letting the finding sit unseen.getAgentOrgChainHealth(...).fullChain— the firstrunningancestor, walking up. Not the board. This follows the CEO's disposition recorded on the BLO-19497 issue thread: a missed review comment is an engineering-loop failure, not a governance decision, and board approval has a multi-day tail. Implemented with the samegetAgentOrgChainHealthutility already used forac-policy-assignee-routing.ts's "nearest executing manager" resolution, but deliberately does not fall back to the (already-stuck) assignee itself the way that resolver does — the whole point is to route past them.changes_requested), not every actionable review — a non-blocking review with body findings still gets its comment but does not escalate (covered by a test).Verification
npx vitest run server/src/__tests__/github-webhook.test.ts— 125/125 passing (see full changelog below for the later additions).npx tsc --noEmit -p server— clean.5231388259, 2026-08-09T11:56:12Z) — that's the live specimen the prose-mention fix below is built from.Risks
issue_commentsfor an actionable review, even on issues that aren'tin_review(e.g.blocked,todo, an unassigned issue). This is the intended fix, but it does mean review feedback can now accumulate as comments on issues in more statuses than before — low risk, matches the AC's explicit requirement that the finding must not depend on issue state.matchedIssuesnow also selectsmonitorNextCheckAt/monitorLastTriggeredAt/monitorAttemptCount(already-indexed columns onissues), and the escalation branch does one extraagentstable scan bycompanyId— only executed when a review is both blocking and the monitor is triggered, which should be rare.runningancestor (e.g. everyone in the chain is paused); in that case escalation silently no-ops (returnsescalated: false) rather than falling back to the board, per the CEO's explicit "not the board" ruling. If this proves too silent in practice, that's a follow-up, not blocking here.Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m], 1M context), agentic tool use (code search, file edits, background test/typecheck execution) — no extended-thinking mode.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateUpdate (BLO-23267 follow-up commits)
Two more commits landed on this PR after the initial round-2 fix and CEO adjudication (BLO-23254):
1. Prose-mention false positive (CEO comment on BLO-23267, 2026-08-09): the comment-shaped review path (
issue_comment) folded the review comment's free text into the identifier set used to MATCH Paperclip issues. Ally's own review comment on this PR narrated the BLO-20775 incident as background motivation, and the substring match alone fired a false Changes-Requested wake on BLO-20775 — a PR (#1125) that has nothing to do with it. Fixed by matching only on the reviewed PR's own title/body (issue.title/issue.body, which is how GitHub'sissue_commentpayload represents a PR), never on the comment's free text — consistent withpaperclip-identifiers.ts's existing operator guard that PR→issue attribution keys on branch/title/body only. Regression test: a review on PR A whose body mentions issue B (bound to a different PR) asserts no wake/comment lands on B.2. Three unresolved Important findings from Ally's own reviews of this PR (
pullrequestreview-4888198804,-4891307841):reopen.commentInserted, so a transient failure between the escalation comment write andheartbeat.wakeuppermanently blocked every later retry — the exact "unseen blocking review" failure mode this PR exists to fix. Escalation is now attempted on every delivery meeting the conditions, deduped on an explicitexternalKeyso a retry after partial failure only redrives the missing wake, not a duplicate comment.isIssueMonitorTriggeredinferred "triggered" from historical columns alone, so an explicitlyclearedmonitor still read as triggered forever and could spuriously escalate.executionState.monitor.status === "cleared"now takes precedence, mirroringderivePersistedMonitorState.pull_request.head.sha(can advance past what was actually reviewed) instead of the review's own immutablecommit_id.pull_request_review.submittednow prefersreview.commit_id.All three have regression tests. Full suite: 125/125 passing (
npx vitest run server/src/__tests__/github-webhook.test.ts),npx tsc --noEmit -p serverclean.