fix: prevent duplicate reviews on open-then-force-push #19

Merged

rcsheets merged 1 commit from fix/duplicate-review-on-force-push into main

2026-03-26 21:38:46 +00:00

rcsheets commented

2026-03-26 21:36:30 +00:00

Owner

When a PR is opened and immediately force-pushed (e.g. UI-prompted
rebase), two webhook events fire in quick succession. Both would
independently queue reviews, producing duplicate safety checks.

Two changes fix this:

Add a has_review_in_progress check in _handle_pr_event so the
redundant "synchronized" event is dropped when a review is already
queued.
Move the diff fetch to AFTER the settle delay in _do_review. The
diff was previously fetched before settling, so the review would
use the stale pre-rebase diff even though the settle delay was
designed for exactly this scenario. Now the settle absorbs the
rapid push and the diff fetch picks up the final state.

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

When a PR is opened and immediately force-pushed (e.g. UI-prompted rebase), two webhook events fire in quick succession. Both would independently queue reviews, producing duplicate safety checks. Two changes fix this: 1. Add a has_review_in_progress check in _handle_pr_event so the redundant "synchronized" event is dropped when a review is already queued. 2. Move the diff fetch to AFTER the settle delay in _do_review. The diff was previously fetched before settling, so the review would use the stale pre-rebase diff even though the settle delay was designed for exactly this scenario. Now the settle absorbs the rapid push and the diff fetch picks up the final state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

rcsheets added 1 commit

2026-03-26 21:36:30 +00:00

fix: prevent duplicate reviews on open-then-force-push b186dbccbf

When a PR is opened and immediately force-pushed (e.g. UI-prompted
rebase), two webhook events fire in quick succession. Both would
independently queue reviews, producing duplicate safety checks.

Two changes fix this:

1. Add a has_review_in_progress check in _handle_pr_event so the
   redundant "synchronized" event is dropped when a review is already
   queued.

2. Move the diff fetch to AFTER the settle delay in _do_review. The
   diff was previously fetched before settling, so the review would
   use the stale pre-rebase diff even though the settle delay was
   designed for exactly this scenario. Now the settle absorbs the
   rapid push and the diff fetch picks up the final state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

pr-reviewer-bot commented

2026-03-26 21:36:31 +00:00

Collaborator

Automated review by pr-reviewer v0.15.0 | Safety Check | anthropic | tracking id r-c5a6de-268671
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: LGTM — The changes implement a reasonable race condition fix by checking for in-progress reviews and moving diff fetching after the settle delay.
Complexity: moderate

This change addresses a race condition where rapid webhook events could create duplicate reviews. The solution is straightforward:

Duplicate prevention: Adds has_review_in_progress() check early in the event handler to skip redundant events
Timing fix: Moves diff fetching after the settle delay to ensure reviewing the final state rather than stale pre-rebase diffs

The logic flow is sound - check for duplicates first, settle to absorb rapid changes, then fetch the current diff. Error handling is preserved (empty diff case, comment update failures). No obvious bugs or security issues.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Safety Check | anthropic | tracking id `r-c5a6de-268671`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: LGTM** — The changes implement a reasonable race condition fix by checking for in-progress reviews and moving diff fetching after the settle delay. **Complexity:** moderate This change addresses a race condition where rapid webhook events could create duplicate reviews. The solution is straightforward: 1. **Duplicate prevention**: Adds `has_review_in_progress()` check early in the event handler to skip redundant events 2. **Timing fix**: Moves diff fetching after the settle delay to ensure reviewing the final state rather than stale pre-rebase diffs The logic flow is sound - check for duplicates first, settle to absorb rapid changes, then fetch the current diff. Error handling is preserved (empty diff case, comment update failures). No obvious bugs or security issues.