fix(heartbeat): retire stale same-PR review context (BLO-22229) - #1064
fix(heartbeat): retire stale same-PR review context (BLO-22229)#1064kkroo wants to merge 3 commits into
Conversation
…eview instance (BLO-22229)
mergeCoalescedContextSnapshot spread `{...existing, ...incoming}` over the
review-content keys (githubPrReviewState/Body/AuthorLogin/...). A new
review-instance wake that doesn't carry its own reviewState — every
issue_comment-sourced `github_pr_review_feedback` wake, since that branch
never sets a formal state — inherited whatever reviewState an EARLIER,
unrelated review instance on the same PR happened to leave behind. Observed:
a comment-shaped Ally review carrying Critical findings got composed with a
prior human APPROVED, producing a directive that told the PR author to merge
a PR carrying unresolved critical findings.
Treat the review-content key block as owned outright by whichever wake most
recently reported it: when the incoming wake is itself a review-instance wake
(github_pr_review_submitted or github_pr_review_feedback), any review-content
key it doesn't re-supply is cleared rather than inherited, mirroring the
existing BLO-19118 handling for cross-PR contamination.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![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: 66d6123
Critical Issues (0)
Important Issues (1)
-
[pr-review-toolkit, gstack/review, native-codex]
server/src/services/heartbeat.ts:6610— A same-PR review replacement does not clear stale comment routing when the superseded review-feedback comment was the only comment.preserveNonGithubPrReviewFeedbackWakeCommentIds(existing)removes that sole ID, somergedCommentIdsis empty; however, the cleanup branch runs only forisDifferentGithubPr. The spread-retainedwakeCommentIds,commentId,wakeCommentId, andpaperclipWaketherefore survive, allowing the superseded Critical/Important feedback to be reloaded alongside the replacement formal review.Clear the canonical comment fields and
paperclipWakewhen eitherisDifferentGithubProrisNewReviewInstanceyields an empty merged list. Add a regression case where the existing snapshot contains only the generated review-feedback comment and the incoming formal review has no comment IDs; the current test keepshuman-comment-1, so it never exercises this branch.
Suggestions (0)
Strengths
- Review-content fields are now replaced atomically, and the added tests cover mixed human-comment preservation and stale review-state inheritance clearly.
Recommended Action
- Address the Important issue this cycle and rerun the focused heartbeat-context tests.
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: 352489d
Looks good.
Prior Findings Dispositioned (1)
- prior:66d6123 important 1 — fixed —
server/src/services/heartbeat.ts:6610— The empty merged-comment set now clears canonical comment routing for a new same-PR review instance, andserver/src/__tests__/heartbeat-context-summary.test.ts:1196covers the sole-feedback-comment case.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Review content fields are now replaced as one unit, which prevents stale approval state from being paired with a later comment-shaped review.
- The tests cover both review-order directions, preservation of unrelated human comments, and complete cleanup when no comment survives.
Recommended Action
- Merge when the required CI checks are green.
allyblockcast
left a comment
allyblockcast
left a comment
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: 352489d
Looks good.
Prior Findings Dispositioned (1)
- prior:66d6123 important 1 — fixed —
server/src/services/heartbeat.ts:6610— The empty merged-comment set now clears canonical comment routing for a new same-PR review instance, andserver/src/__tests__/heartbeat-context-summary.test.ts:1196covers the sole-feedback-comment case.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Review content fields are now replaced as one unit, which prevents stale approval state from being paired with a later comment-shaped review.
- The tests cover both review-order directions, preservation of unrelated human comments, and complete cleanup when no comment survives.
Recommended Action
- Merge when the required CI checks are green.
3 participants
Thinking Path
Linked Issues or Issue Description
Refs: https://paperclip.blockcast.net/BLO/issues/BLO-22229
Refs: https://paperclip.blockcast.net/BLO/issues/BLO-19118
Supersedes: #1063
Bug: Same-PR review wake coalescing could compose context from two different review instances. A later comment-shaped review lacked its own formal
reviewStateand inherited an earlier approval; a later formal review could also keep an older actionable review-feedback comment inwakeCommentIds, causing the rebuiltpaperclipWake.commentspayload to reload superseded findings.What Changed
github_pr_review_submittedandgithub_pr_review_feedbackwakes as fresh review instances that own review-content fields outright.github_pr_review_feedbackcomment ids out of the existing wake-comment set when a same-PR review instance replaces the prior review context.Verification
PATH=/opt/homebrew/bin:$PATH pnpm install --frozen-lockfilePATH=/opt/homebrew/bin:$PATH pnpm exec vitest run server/src/__tests__/heartbeat-context-summary.test.ts server/src/__tests__/heartbeat-workspace-session.test.ts --no-file-parallelism --maxWorkers=1(283 passed)PATH=/opt/homebrew/bin:$PATH pnpm --filter @paperclipai/server typecheckPATH=/opt/homebrew/bin:$PATH pnpm exec vitest run server/src/__tests__/workspace-runtime.test.ts --testNamePattern "provisions worktree-local pnpm node_modules instead of reusing base-repo links" --no-file-parallelism --maxWorkers=1(2 passed, 116 skipped)PATH=/opt/homebrew/bin:$PATH pnpm exec vitest run server/src/__tests__/heartbeat-queued-backlog-convergence.test.ts --no-file-parallelism --maxWorkers=1(11 skipped locally)Risks
Low to moderate risk. The change is scoped to heartbeat coalescing for GitHub review-instance wakes. The main behavior shift is intentional: a new review instance no longer inherits stale review-content fields or stale review-feedback comments from a prior review instance. Non-review wakes still inherit existing review context.
Model Used
Codex GPT-5 coding agent with shell, GitHub CLI, and local test execution.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template