fix(code-review): preserve in-flight review on merge commit push

apps/web/src/lib/code-reviews/db/code-reviews.ts

@@ -493,6 +493,39 @@ export async function updateCheckRunId(reviewId: string, checkRunId: number): Pr
   }
 }
 
+/**
+ * Repoints an in-flight review at a new head SHA (and optionally a new check
+ * run). Used when a merge commit arrives for a PR with a preserved review:
+ * the review keeps running on the prior feature-branch content, but its
+ * eventual completion needs to update the gate on the new HEAD (which is
+ * what branch-protection evaluates) rather than the abandoned prior SHA.
+ *
+ * Pass `checkRunId = null` for GitLab, whose commit statuses are keyed by
+ * (sha, name) rather than by an opaque ID.
+ */
+export async function updateReviewHeadShaAndCheckRun(
+  reviewId: string,
+  headSha: string,
+  checkRunId: number | null
+): Promise<void> {
+  try {
+    await db
+      .update(cloud_agent_code_reviews)
+      .set({
+        head_sha: headSha,
+        check_run_id: checkRunId,
+        updated_at: new Date().toISOString(),
+      })
+      .where(eq(cloud_agent_code_reviews.id, reviewId));
+  } catch (error) {
+    captureException(error, {
+      tags: { operation: 'updateReviewHeadShaAndCheckRun' },
+      extra: { reviewId, headSha, checkRunId },
+    });
+    throw error;
+  }
+}
+
 /**
  * Verifies that a user owns (or is a member of the org that owns) a code review
  * Returns true if the user has access, false otherwise

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

@@ -7,14 +7,21 @@ import {
   createCodeReview,
   findExistingReview,
   findActiveReviewsForPR,
+  updateReviewHeadShaAndCheckRun,
 } from '@/lib/code-reviews/db/code-reviews';
 import { tryDispatchPendingReviews } from '@/lib/code-reviews/dispatch/dispatch-pending-reviews';
 import { getAgentConfigForOwner } from '@/lib/agent-config/db/agent-configs';
 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 +148,54 @@ 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,
+      });
+
+      // The preserved review's check run still sits on the prior SHA, so
+      // branch protection that requires the Kilo check would stay blocked
+      // on the merge-commit head. Drop a fresh check run on the new HEAD
+      // and repoint the review at it so its eventual completion callback
+      // updates the gate on the commit GitHub actually evaluates. Note
+      // the check run goes on the *base* repo (where branch protection
+      // lives and where the app is installed), not the head/fork repo.
+      const [baseOwner, baseRepoName] = repository.full_name.split('/');
+      await migrateInFlightReviewsToMergeCommitHead({
+        repoFullName: repository.full_name,
+        prNumber: pull_request.number,
+        newHeadSha: pull_request.head.sha,
+        installationId: integration.platform_installation_id as string,
+        baseOwner,
+        baseRepoName,
+        appType,
+        userIdForFlag: owner.userId,
+      });
+
+      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 +219,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 +240,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 +262,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 +314,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 +329,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 +354,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 +382,122 @@ 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);
+}
+
+/**
+ * When a merge-commit synchronize arrives and we bail out to preserve the
+ * in-flight review, the review is still pinned to the previous SHA's check
+ * run. GitHub branch protection evaluates against the current HEAD, so the
+ * required Kilo check would never appear on the merge commit. This helper
+ * creates a fresh check run on the new HEAD and moves the review record
+ * onto it, so the completion callback updates the visible gate. Best-effort:
+ * any failure is logged but does not fail the webhook.
+ */
+async function migrateInFlightReviewsToMergeCommitHead(args: {
+  repoFullName: string;
+  prNumber: number;
+  newHeadSha: string;
+  installationId: string;
+  // Owner/name of the *base* repo (where branch protection lives and the
+  // GitHub App is installed). Fork PRs must not create check runs in the
+  // contributor's fork — that repo often has no app installation, and
+  // branch protection on the base wouldn't see the run anyway.
+  baseOwner: string;
+  baseRepoName: string;
+  appType: GitHubAppType;
+  userIdForFlag: string;
+}) {
+  if (args.appType === 'lite') return;
+
+  const isPrGateEnabled =
+    process.env.NODE_ENV === 'development' ||
+    (await isFeatureFlagEnabled('code-review-pr-gate', args.userIdForFlag));
+  if (!isPrGateEnabled) return;
+
+  try {
+    const activeReviewIds = await findActiveReviewsForPR(
+      args.repoFullName,
+      args.prNumber,
+      args.newHeadSha
+    );
+    if (activeReviewIds.length === 0) return;
+
+    // In practice a PR has at most one active review at a time; migrate the
+    // first one to the new SHA. Any extras stay pinned to their old SHAs
+    // and will be cancelled on the next non-merge push.
+    const [reviewId] = activeReviewIds;
+    const detailsUrl = `${APP_URL}/code-reviews/${reviewId}`;
+
+    let newCheckRunId: number | undefined;
+    try {
+      newCheckRunId = await createCheckRun(
+        args.installationId,
+        args.baseOwner,
+        args.baseRepoName,
+        args.newHeadSha,
+        {
+          detailsUrl,
+          output: {
+            title: 'Kilo Code Review in progress',
+            summary: 'Continuing the review from the previous commit.',
+          },
+        },
+        args.appType
+      );
+      await updateReviewHeadShaAndCheckRun(reviewId, args.newHeadSha, newCheckRunId);
+      logExceptInTest(
+        `Migrated review ${reviewId} to merge-commit head ${args.newHeadSha} (check run ${newCheckRunId})`
+      );
+    } catch (migrateError) {
+      logExceptInTest('Failed to migrate in-flight review onto merge commit head:', migrateError);
+      // If we created the new check run on GitHub but could not persist
+      // the migration, cancel the new run so it does not stay 'queued'
+      // forever and block branch-protection gating.
+      if (newCheckRunId !== undefined) {
+        try {
+          await updateCheckRun(
+            args.installationId,
+            args.baseOwner,
+            args.baseRepoName,
+            newCheckRunId,
+            { status: 'completed', conclusion: 'cancelled' }
+          );
+        } catch (cancelError) {
+          logExceptInTest('Failed to cancel orphaned merge-commit check run:', cancelError);
+        }
+      }
+    }
+  } catch (lookupError) {
+    logExceptInTest('Failed to find active reviews for merge-commit migration:', lookupError);
+  }
+}
+
 /**
  * Main router for pull request events
  */