fix(code-review): skip merge commits before cancelling in-flight reviews

When a pull_request.synchronize / merge_request.update event arrives with a merge-commit head (e.g. clicking GitHub's 'Update branch' button), the review was being cancelled before the merge-commit skip check ran, leaving the PR with no active review. Reorder so the merge-commit check runs first and bails out without touching existing reviews. Extract the decision into small pure helpers for unit testing.

Author: alex-alecu

apps/web/src/lib/integrations/platforms/github/webhook-handlers/pull-request-handler.test.ts

@@ -1,4 +1,5 @@
 import { resolvePullRequestCheckoutRef } from '@/lib/integrations/platforms/github/webhook-handlers/pull-request-checkout-ref';
+import { shouldSkipSynchronizeForMergeCommit } from '@/lib/integrations/platforms/github/webhook-handlers/pull-request-handler';
 
 describe('resolvePullRequestCheckoutRef', () => {
   it('uses head.ref for same-repo PRs', () => {
@@ -63,3 +64,64 @@ describe('resolvePullRequestCheckoutRef', () => {
     });
   });
 });
+
+describe('shouldSkipSynchronizeForMergeCommit', () => {
+  const baseArgs = {
+    installationId: 'inst-1',
+    headOwner: 'acme',
+    headRepoName: 'widgets',
+    headSha: 'deadbeef',
+    appType: 'standard' as const,
+  };
+
+  it('returns false for non-synchronize actions without calling the check', async () => {
+    for (const action of ['opened', 'reopened', 'ready_for_review']) {
+      let called = false;
+      const result = await shouldSkipSynchronizeForMergeCommit({
+        ...baseArgs,
+        action,
+        isMergeCommitFn: async () => {
+          called = true;
+          return true;
+        },
+      });
+
+      expect(result).toBe(false);
+      expect(called).toBe(false);
+    }
+  });
+
+  it('returns true when synchronize head is a merge commit', async () => {
+    const result = await shouldSkipSynchronizeForMergeCommit({
+      ...baseArgs,
+      action: 'synchronize',
+      isMergeCommitFn: async () => true,
+    });
+
+    expect(result).toBe(true);
+  });
+
+  it('returns false when synchronize head is not a merge commit', async () => {
+    const result = await shouldSkipSynchronizeForMergeCommit({
+      ...baseArgs,
+      action: 'synchronize',
+      isMergeCommitFn: async () => false,
+    });
+
+    expect(result).toBe(false);
+  });
+
+  it('passes the expected arguments to the check function', async () => {
+    const calls: Array<[string, string, string, string, string]> = [];
+    await shouldSkipSynchronizeForMergeCommit({
+      ...baseArgs,
+      action: 'synchronize',
+      isMergeCommitFn: async (installationId, owner, repo, sha, appType) => {
+        calls.push([installationId, owner, repo, sha, appType]);
+        return false;
+      },
+    });
+
+    expect(calls).toEqual([['inst-1', 'acme', 'widgets', 'deadbeef', 'standard']]);
+  });
+});

apps/web/src/lib/integrations/platforms/github/webhook-handlers/pull-request-handler.ts

@@ -14,7 +14,13 @@ import type { PlatformIntegration } from '@kilocode/db/schema';
 import type { Owner } from '@/lib/code-reviews/core';
 import { getBotUserId } from '@/lib/bot-users/bot-user-service';
 import type { CodeReviewAgentConfig } from '@/lib/agent-config/core/types';
-import { addReactionToPR, createCheckRun, isMergeCommit, updateCheckRun } from '../adapter';
+import type { GitHubAppType } from '@/lib/integrations/platforms/github/adapter';
+import {
+  addReactionToPR,
+  createCheckRun,
+  isMergeCommit,
+  updateCheckRun,
+} from '@/lib/integrations/platforms/github/adapter';
 import { codeReviewWorkerClient } from '@/lib/code-reviews/client/code-review-worker-client';
 import { updateCheckRunId } from '@/lib/code-reviews/db/code-reviews';
 import { resolvePullRequestCheckoutRef } from './pull-request-checkout-ref';
@@ -141,7 +147,34 @@ export async function handlePullRequestCodeReview(
       );
     }
 
-    // 4. Cancel any existing reviews for this PR (different SHA)
+    const appType = integration.github_app_type ?? 'standard';
+    const headFullName = checkoutRef.headRepoFullName ?? repository.full_name;
+    const [headOwner, headRepoName] = headFullName.split('/');
+
+    // 4. Skip merge commits on synchronize (e.g. merging base branch into feature branch).
+    // Runs before cancellation so that an in-flight review at an earlier SHA is preserved:
+    // a merge commit introduces no new feature work and should not supersede the existing review.
+    if (
+      headOwner &&
+      headRepoName &&
+      (await shouldSkipSynchronizeForMergeCommit({
+        action: payload.action,
+        installationId: integration.platform_installation_id as string,
+        headOwner,
+        headRepoName,
+        headSha: pull_request.head.sha,
+        appType,
+      }))
+    ) {
+      logExceptInTest('Skipping merge commit:', {
+        pr_number: pull_request.number,
+        repo: repository.full_name,
+        head_sha: pull_request.head.sha,
+      });
+      return NextResponse.json({ message: 'Skipped merge commit' }, { status: 200 });
+    }
+
+    // 5. Cancel any existing reviews for this PR (different SHA)
     // This prevents spam when user pushes multiple commits quickly
     const oldReviewIds = await findActiveReviewsForPR(
       repository.full_name,
@@ -165,29 +198,7 @@ export async function handlePullRequestCodeReview(
       );
     }
 
-    // 4b. Skip merge commits on synchronize (e.g. merging base branch into feature branch)
-    // Placed after step 4 so old reviews are still cancelled before we bail out.
-    if (payload.action === GITHUB_ACTION.SYNCHRONIZE) {
-      const headRepo = checkoutRef.headRepoFullName ?? repository.full_name;
-      const [headOwner, headRepoName] = headRepo.split('/');
-      const mergeCommit = await isMergeCommit(
-        integration.platform_installation_id as string,
-        headOwner,
-        headRepoName,
-        pull_request.head.sha,
-        integration.github_app_type ?? 'standard'
-      );
-      if (mergeCommit) {
-        logExceptInTest('Skipping merge commit:', {
-          pr_number: pull_request.number,
-          repo: repository.full_name,
-          head_sha: pull_request.head.sha,
-        });
-        return NextResponse.json({ message: 'Skipped merge commit' }, { status: 200 });
-      }
-    }
-
-    // 5. Check for duplicate review (same repo, PR, SHA)
+    // 6. Check for duplicate review (same repo, PR, SHA)
     const existingReview = await findExistingReview(
       repository.full_name,
       pull_request.number,
@@ -208,7 +219,7 @@ export async function handlePullRequestCodeReview(
       );
     }
 
-    // 6. Create review record (session_id will be updated async)
+    // 7. Create review record (session_id will be updated async)
     const reviewId = await createCodeReview({
       owner,
       platformIntegrationId: integration.id,
@@ -230,8 +241,7 @@ export async function handlePullRequestCodeReview(
 
     const [repoOwner, repoName] = repository.full_name.split('/');
 
-    // 7. Create GitHub Check Run (PR gate) — skip for lite (read-only) app, skip when flag is off
-    const appType = integration.github_app_type ?? 'standard';
+    // 8. Create GitHub Check Run (PR gate) — skip for lite (read-only) app, skip when flag is off
     const isPrGateEnabled =
       process.env.NODE_ENV === 'development' ||
       (await isFeatureFlagEnabled('code-review-pr-gate', owner.userId));
@@ -283,7 +293,7 @@ export async function handlePullRequestCodeReview(
       }
     }
 
-    // 8. Post 👀 reaction to show Kilo is reviewing
+    // 9. Post 👀 reaction to show Kilo is reviewing
     try {
       await addReactionToPR(
         integration.platform_installation_id as string,
@@ -298,7 +308,7 @@ export async function handlePullRequestCodeReview(
       logExceptInTest('Failed to add eyes reaction:', reactionError);
     }
 
-    // 9. Try to dispatch pending reviews (including this new one)
+    // 10. Try to dispatch pending reviews (including this new one)
     // Review is created with status='pending' and dispatch will pick it up if slots available
     try {
       const dispatchResult = await tryDispatchPendingReviews(owner);
@@ -323,7 +333,7 @@ export async function handlePullRequestCodeReview(
       // Don't throw - review record created as pending, will be picked up later
     }
 
-    // 10. Return 202 Accepted (always succeeds, review queued as pending)
+    // 11. Return 202 Accepted (always succeeds, review queued as pending)
     return NextResponse.json(
       {
         message: 'Code review queued',
@@ -351,6 +361,34 @@ export async function handlePullRequestCodeReview(
   }
 }
 
+/**
+ * Decides whether a pull_request webhook should bail out because its head
+ * commit is a merge commit (e.g. produced by GitHub's "Update branch" button
+ * or a manual `git merge main`). Only applies to synchronize events.
+ *
+ * Extracted so tests can inject a fake `isMergeCommitFn` without mocking the
+ * GitHub adapter module.
+ */
+export async function shouldSkipSynchronizeForMergeCommit(args: {
+  action: string;
+  installationId: string;
+  headOwner: string;
+  headRepoName: string;
+  headSha: string;
+  appType: GitHubAppType;
+  isMergeCommitFn?: (
+    installationId: string,
+    owner: string,
+    repo: string,
+    sha: string,
+    appType: GitHubAppType
+  ) => Promise<boolean>;
+}): Promise<boolean> {
+  if (args.action !== GITHUB_ACTION.SYNCHRONIZE) return false;
+  const check = args.isMergeCommitFn ?? isMergeCommit;
+  return check(args.installationId, args.headOwner, args.headRepoName, args.headSha, args.appType);
+}
+
 /**
  * Main router for pull request events
  */

apps/web/src/lib/integrations/platforms/gitlab/webhook-handlers/merge-request-handler.test.ts

@@ -0,0 +1,49 @@
+import { shouldSkipUpdateForMergeCommit } from '@/lib/integrations/platforms/gitlab/webhook-handlers/merge-request-handler';
+
+describe('shouldSkipUpdateForMergeCommit', () => {
+  it('returns false for non-update actions without calling the check', async () => {
+    const actions: Array<string | undefined> = ['open', 'reopen', 'close', 'merge', undefined];
+    for (const action of actions) {
+      let called = false;
+      const result = await shouldSkipUpdateForMergeCommit({
+        action,
+        check: async () => {
+          called = true;
+          return true;
+        },
+      });
+
+      expect(result).toBe(false);
+      expect(called).toBe(false);
+    }
+  });
+
+  it('returns true when update head is a merge commit', async () => {
+    const result = await shouldSkipUpdateForMergeCommit({
+      action: 'update',
+      check: async () => true,
+    });
+
+    expect(result).toBe(true);
+  });
+
+  it('returns false when update head is not a merge commit', async () => {
+    const result = await shouldSkipUpdateForMergeCommit({
+      action: 'update',
+      check: async () => false,
+    });
+
+    expect(result).toBe(false);
+  });
+
+  it('fails open when the check throws (review proceeds)', async () => {
+    const result = await shouldSkipUpdateForMergeCommit({
+      action: 'update',
+      check: async () => {
+        throw new Error('GitLab API unreachable');
+      },
+    });
+
+    expect(result).toBe(false);
+  });
+});