From 12e1b274852679806e0a4aaf3bdd4d7e703a9494 Mon Sep 17 00:00:00 2001
From: Ralf Anton Beier <ralf_beier@me.com>
Date: Sat, 2 May 2026 08:18:34 +0200
Subject: [PATCH] fix: idempotency guard on pull_request.opened (Bug #13)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Why: QA bug-hunter flagged that pull_request.opened had no
isProcessed/markProcessed gate. Probot retries the webhook ~5 times on
5xx/timeout, replaying enablePullRequestAutoMerge (idempotent on
GitHub's side, but spammy in logs), the AI review (only protected by
the 5-min _reviewTimestamps throttle inside reviewPullRequest), and
the dependabot generation enqueue (creates duplicate task-store
entries with the same key — wasteful and confusing).

What:
  - Add `if (deliveryId && isProcessed(deliveryId)) return;` at handler
    entry, mirroring the pattern already used by repository.created,
    issues.opened, issue_comment.created, and pull_request.closed.
  - Add `if (deliveryId) markProcessed(deliveryId);` at handler exit so
    subsequent retries hit the dedup gate.

Test plan:
  - 2 new tests:
    - duplicate delivery short-circuits before reviewPullRequest
    - successful delivery records markProcessed
  - npm test → 836 pass (was 834)
  - npm run lint → clean

Risk: low — same pattern as four sibling handlers.

Note: pull_request.synchronize and pull_request.reopened (added in
PR #50) will need the same guard. Will fold in once #50 lands.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 __tests__/integration/app.test.js | 48 +++++++++++++++++++++++++++++++
 src/app.js                        | 14 +++++++++
 2 files changed, 62 insertions(+)

diff --git a/__tests__/integration/app.test.js b/__tests__/integration/app.test.js
index da1cb5a..58333a1 100644
--- a/__tests__/integration/app.test.js
+++ b/__tests__/integration/app.test.js
@@ -695,6 +695,54 @@ describe('app', () => {
 
       expect(getLogger().error).toHaveBeenCalledWith({ err: error }, 'Probot error');
     });
+
+    // Bug #13: pull_request.opened must dedupe on deliveryId. Without
+    // this, Probot retries replay enablePullRequestAutoMerge, the AI
+    // review (only the AI side has its own 5-min throttle), and the
+    // dependabot enqueue.
+    it('pull_request.opened skips duplicate webhook deliveries', async () => {
+      isProcessed.mockReturnValue(true);
+      _setConfigForTesting({ ai_review: { enabled: true } });
+      reviewPullRequest.mockClear();
+      const { handlers } = setupApp();
+
+      const context = {
+        id: 'delivery-pr-opened-dup',
+        log: { info: jest.fn(), warn: jest.fn(), error: jest.fn() },
+        octokit: createMockOctokit(),
+        payload: {
+          pull_request: { number: 7, node_id: 'PR_7' },
+          repository: { name: 'rivet', owner: { login: 'pulseengine' }, default_branch: 'main' },
+          sender: { login: 'human' }
+        }
+      };
+
+      await handlers['pull_request.opened'](context);
+
+      expect(reviewPullRequest).not.toHaveBeenCalled();
+      expect(markProcessed).not.toHaveBeenCalled();
+    });
+
+    it('pull_request.opened marks delivery processed on completion', async () => {
+      _setConfigForTesting({ ai_review: { enabled: true } });
+      reviewPullRequest.mockResolvedValue({ success: true });
+      const { handlers } = setupApp();
+
+      const context = {
+        id: 'delivery-pr-opened-1',
+        log: { info: jest.fn(), warn: jest.fn(), error: jest.fn() },
+        octokit: createMockOctokit(),
+        payload: {
+          pull_request: { number: 8, node_id: 'PR_8' },
+          repository: { name: 'rivet', owner: { login: 'pulseengine' }, default_branch: 'main' },
+          sender: { login: 'human' }
+        }
+      };
+
+      await handlers['pull_request.opened'](context);
+
+      expect(markProcessed).toHaveBeenCalledWith('delivery-pr-opened-1');
+    });
   });
 
   // =========================================================================
diff --git a/src/app.js b/src/app.js
index db1446d..770bdae 100644
--- a/src/app.js
+++ b/src/app.js
@@ -839,6 +839,18 @@ function registerApp(app, options = {}) {
   // PR opened: auto-merge for bots, AI review, dependabot generation check
   app.on('pull_request.opened', async (context) => {
     if (context.log) setLogger(context.log);
+    // Bug #13: dedupe at handler entry. Without this guard, a Probot
+    // webhook redelivery (~5 retries on 5xx/timeout) re-runs auto-merge
+    // enable, the AI review, and the dependabot enqueue. The 5-min
+    // _reviewTimestamps throttle in reviewPullRequest only catches the
+    // AI review path; it doesn't protect enablePullRequestAutoMerge or
+    // the task-store enqueue.
+    const deliveryId = context.id;
+    if (deliveryId && isProcessed(deliveryId)) {
+      getLogger().info({ deliveryId }, 'Skipping duplicate pull_request.opened delivery');
+      return;
+    }
+
     const config = getConfig();
     const pr = context.payload.pull_request;
     const { repository } = context.payload;
@@ -926,6 +938,8 @@ function registerApp(app, options = {}) {
         }
       }
     }
+
+    if (deliveryId) markProcessed(deliveryId);
   });
 
   app.on('pull_request.closed', async (context) => {