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

alex-alecu · web-flow · commit 2fba9db37b1b · 2026-04-28T09:52:28.000+02:00
* 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.

* fix(code-review): drop github check on merge-commit head

* fix(code-review): set gitlab status on merge-commit head

* fix(code-review): create check run on base repo
diff --git a/apps/web/src/lib/code-reviews/db/code-reviews.ts b/apps/web/src/lib/code-reviews/db/code-reviews.ts
@@ -494,6 +494,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
diff --git a/apps/web/src/lib/integrations/platforms/github/webhook-handlers/pull-request-handler.ts b/apps/web/src/lib/integrations/platforms/github/webhook-handlers/pull-request-handler.ts
@@ -7,6 +7,7 @@ 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';
@@ -170,6 +171,25 @@ export async function handlePullRequestCodeReview(
         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,
+      });
+
       return NextResponse.json({ message: 'Skipped merge commit' }, { status: 200 });
     }
 
@@ -384,6 +404,88 @@ export async function shouldSkipSynchronizeForMergeCommit(args: {
   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;
+}) {
+  if (args.appType === 'lite') 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
  */
diff --git a/apps/web/src/lib/integrations/platforms/gitlab/webhook-handlers/merge-request-handler.ts b/apps/web/src/lib/integrations/platforms/gitlab/webhook-handlers/merge-request-handler.ts
@@ -16,6 +16,7 @@ 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';
@@ -177,6 +178,21 @@ export async function handleMergeRequestCodeReview(
         project: project.path_with_namespace,
         head_sha: headSha,
       });
+
+      // The preserved review is still pinned to the prior commit's status,
+      // so any required 'kilo/code-review' gate would stay stuck on that
+      // SHA while MR branch protection evaluates the new HEAD. Repoint the
+      // review at the merge-commit SHA and drop a fresh pending status so
+      // the eventual completion callback writes its final state to the
+      // commit GitLab actually evaluates.
+      await migrateInFlightReviewsToMergeCommitHead({
+        integrationId: integration.id,
+        projectId: project.id,
+        repoFullName: project.path_with_namespace,
+        mrIid: mr.iid,
+        newHeadSha: headSha,
+      });
+
       return NextResponse.json({ message: 'Skipped merge commit' }, { status: 200 });
     }
 
@@ -363,6 +379,68 @@ export async function shouldSkipUpdateForMergeCommit(args: {
   }
 }
 
+/**
+ * When a merge-commit update arrives and we bail out to preserve the
+ * in-flight review, the review row still points at the previous SHA, so
+ * the completion callback would post its final GitLab commit status on
+ * the abandoned commit. Repoint the review at the merge-commit SHA and
+ * drop a fresh pending status on it. Best-effort: any failure is logged
+ * but does not fail the webhook.
+ */
+async function migrateInFlightReviewsToMergeCommitHead(args: {
+  integrationId: string;
+  projectId: number;
+  repoFullName: string;
+  mrIid: number;
+  newHeadSha: string;
+}) {
+  try {
+    const activeReviewIds = await findActiveReviewsForPR(
+      args.repoFullName,
+      args.mrIid,
+      args.newHeadSha
+    );
+    if (activeReviewIds.length === 0) return;
+
+    const fullIntegration = await getIntegrationById(args.integrationId);
+    if (!fullIntegration) return;
+    const metadata = fullIntegration.metadata as { gitlab_instance_url?: string } | null;
+    const instanceUrl = metadata?.gitlab_instance_url || 'https://gitlab.com';
+
+    // In practice an MR has at most one active review; migrate the first.
+    const [reviewId] = activeReviewIds;
+
+    // Update the DB row first. If this fails we never touched the new SHA,
+    // so there's nothing to clean up. If it succeeds but setCommitStatus
+    // fails below, the eventual completion callback will still target the
+    // correct (new) SHA — we just miss the transient pending badge.
+    await updateReviewHeadShaAndCheckRun(reviewId, args.newHeadSha, null);
+
+    try {
+      const pratToken = await getOrCreateProjectAccessToken(fullIntegration, args.projectId);
+      const detailsUrl = `${APP_URL}/code-reviews/${reviewId}`;
+      await setCommitStatus(
+        pratToken,
+        args.projectId,
+        args.newHeadSha,
+        'pending',
+        {
+          targetUrl: detailsUrl,
+          description: 'Kilo Code Review continuing from previous commit',
+        },
+        instanceUrl
+      );
+      logExceptInTest(
+        `Migrated review ${reviewId} to merge-commit head ${args.newHeadSha} and set pending status`
+      );
+    } catch (statusError) {
+      logExceptInTest('Failed to set pending status on merge-commit head:', statusError);
+    }
+  } catch (migrateError) {
+    logExceptInTest('Failed to migrate in-flight review onto merge commit head:', migrateError);
+  }
+}
+
 /**
  * Main router for merge request events
  */
