fix(gate): recheck migration collisions at merge

Author: JSONbored

src/queue/processors.ts

@@ -247,8 +247,7 @@ import {
 } from "../settings/agent-actions";
 import { isAutoCloseExempt } from "../settings/auto-close-exempt";
 import { resolveGlobalContributorOpenItemCap } from "../settings/global-contributor-cap";
-import { detectMigrationCollisions, extractMigrationNumber, KNOWN_MIGRATION_DUPLICATES } from "../db/migration-collisions";
-import { listMigrationFilenamesAtRef } from "../github/migration-tree";
+import { migrationFilenamesForLiveRecheck, resolveLiveMigrationCollisionHold } from "../services/migration-collision-recheck";
 import {
   executeAgentMaintenanceActions,
   executeIssueMaintenanceActions,
@@ -1603,59 +1602,6 @@ export function changedPathsForGuardrail(
   return [...paths];
 }
 
-/**
- * Live premerge migrations/** collision recheck (#2550). `check-migrations.mjs` (CI) only validates against
- * THIS PR's own branch snapshot at the time CI ran — it can never see a sibling PR that merged a
- * same-numbered migration file to `baseRef` in the meantime. This does the live check right before the
- * merge-decision moment: fetch the base branch's CURRENT migration filenames, drop any filename THIS PR's
- * own diff removes from the base (an outright deletion, or a rename's pre-rename name — otherwise renaming
- * an existing base migration self-collides with its own old name, which is still live on `baseRef` until
- * this PR merges), union what's left with THIS PR's own new migration filenames (the live tree never
- * contains this PR's own not-yet-merged files, so checking main alone could never detect a collision from
- * this PR's perspective — the union is load-bearing, not optional), then run the SAME collision-detection
- * function scripts/check-migrations.mjs uses.
- *
- * Deliberately scoped to a collision involving THIS PR's own migration number(s) only (via `prNumbers`) — a
- * pre-existing collision between two OTHER already-merged files (which would mean `main` itself is already
- * broken, a separate problem CI already surfaces loudly) must not hold an unrelated third PR whose own
- * migration number doesn't collide with anything.
- *
- * Fail-OPEN throughout: a missing baseRef or a failed live fetch returns undefined (no hold) rather than
- * risking a false hold on inconclusive data — this is a safety net, not a new way to get PRs stuck.
- */
-// Deliberately UNCACHED: this is the safety check the whole feature exists to provide, so it must always
-// read the live tree fresh. A cache keyed by repo+baseRef (even a short-TTL one) can serve a snapshot taken
-// BEFORE a sibling PR merged its own colliding migration — defeating the exact race this function exists to
-// catch (PR A merges 0099, a still-cached pre-merge tree lets a later-processed PR B also merge its own 0099
-// within the cache window). The existing GitHub rate-limit admission/backoff mechanism (the same
-// `admissionKey` every other live call in this function already uses) already bounds the cost; correctness
-// here matters far more than shaving a redundant API call.
-async function resolveLiveMigrationCollisionHold(
-  args: {
-    repoFullName: string;
-    baseRef: string | null | undefined;
-    token: string | undefined;
-    admissionKey: GitHubRateLimitAdmissionKey | undefined;
-    prMigrationFilenames: string[];
-    prRemovedMigrationFilenames: string[];
-  },
-): Promise<{ reason: string; comment: string } | undefined> {
-  if (!args.baseRef) return undefined;
-  const liveFilenames = await listMigrationFilenamesAtRef(args.repoFullName, args.baseRef, args.token, args.admissionKey);
-  if (liveFilenames === null) return undefined;
-  const removedFromBase = new Set(args.prRemovedMigrationFilenames);
-  const effectiveLiveFilenames = liveFilenames.filter((f) => !removedFromBase.has(f));
-  const union = [...new Set([...effectiveLiveFilenames, ...args.prMigrationFilenames])];
-  const prNumbers = new Set(args.prMigrationFilenames.map((f) => extractMigrationNumber(f)).filter((n): n is number => n !== null));
-  const collisions = detectMigrationCollisions(union, KNOWN_MIGRATION_DUPLICATES).filter((c) => prNumbers.has(c.number));
-  if (collisions.length === 0) return undefined;
-  const detail = collisions.map((c) => `${c.paddedNumber}: ${c.files.join(", ")}`).join("; ");
-  return {
-    reason: `live migrations/** collision on ${args.baseRef} (${detail})`,
-    comment: `Gittensory: a live check of \`migrations/**\` on \`${args.baseRef}\` found a migration-number collision that isn't visible from this PR's own diff — another PR merged a same-numbered migration file since this PR's CI last ran (**${detail}**). This PR is held for manual review — please rebase onto the latest \`${args.baseRef}\` and renumber your migration to the next free number before this can merge.`,
-  };
-}
-
 /**
  * Chain the two INDEPENDENT precision circuit-breakers over a planned action set (the merge-side and close-side
  * downgrades), in order. PURE — the live flag reads happen at the call site (each fail-open), so this composes
@@ -1920,25 +1866,7 @@ async function runAgentMaintenancePlanAndExecute(
   // producing a false hold that can never clear (a later rename still carries the stale old name forever, on
   // every subsequent maintenance pass). Only `.path` (the file's CURRENT name) and only non-removed files
   // reflect what will actually exist in this PR's tree once merged.
-  const prMigrationFilenames = changedFiles
-    .filter((f) => f.status !== "removed" && f.path.startsWith("migrations/") && f.path.endsWith(".sql"))
-    .map((f) => f.path.slice("migrations/".length));
-  // Base filenames this PR's diff removes from `migrations/**` — an outright deletion's own `.path`, or a
-  // rename's pre-rename `.previousFilename` — so a filename that won't exist once this PR merges isn't still
-  // counted from the live base fetch below. Without this, renaming an EXISTING base migration within the same
-  // number (e.g. `migrations/0099_old.sql` -> `migrations/0099_new.sql`, fixing a typo on an already-merged
-  // file) unions both the old (still live) and new (this PR's) name and self-collides, even though the merged
-  // tree would only ever contain the new file.
-  const prRemovedMigrationFilenames = changedFiles.flatMap((f) => {
-    const removed: string[] = [];
-    if (f.status === "removed" && f.path.startsWith("migrations/") && f.path.endsWith(".sql")) {
-      removed.push(f.path.slice("migrations/".length));
-    }
-    if (f.previousFilename && f.previousFilename.startsWith("migrations/") && f.previousFilename.endsWith(".sql")) {
-      removed.push(f.previousFilename.slice("migrations/".length));
-    }
-    return removed;
-  });
+  const { prMigrationFilenames, prRemovedMigrationFilenames } = migrationFilenamesForLiveRecheck(changedFiles);
   const migrationCollisionHold =
     settings.premergeContentRecheck === true && prMigrationFilenames.length > 0
       ? await resolveLiveMigrationCollisionHold({

src/services/agent-action-executor.ts

@@ -1,4 +1,4 @@
-import { bumpPullRequestMergeAttempt, createPendingAgentActionIfAbsent, insertNotificationDeliveryIfAbsent, isGlobalAgentFrozen, markPullRequestApproved, markPullRequestMergeBlocked, recordAuditEvent } from "../db/repositories";
+import { bumpPullRequestMergeAttempt, createPendingAgentActionIfAbsent, getPullRequest, insertNotificationDeliveryIfAbsent, isGlobalAgentFrozen, listPullRequestFiles, markPullRequestApproved, markPullRequestMergeBlocked, recordAuditEvent } from "../db/repositories";
 import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure";
 import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord";
 import { createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app";
@@ -9,10 +9,12 @@ import { closeIssue, closePullRequest, createIssueComment, createPullRequestRevi
 import { fetchPullRequestFreshness, pullRequestFreshnessDetail } from "../github/pr-freshness";
 import { isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy";
 import { buildAgentActionAudit, isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness } from "../settings/agent-execution";
-import type { PlannedAgentAction } from "../settings/agent-actions";
+import { AGENT_LABEL_MIGRATION_COLLISION, type PlannedAgentAction } from "../settings/agent-actions";
 import type { AgentActionClass, AgentPendingActionParams, AutonomyLevel, AutonomyPolicy } from "../types";
 import { errorMessage } from "../utils/json";
 import { AGENT_LABEL_PENDING_CLOSURE } from "../review/linked-issue-hard-rules";
+import { migrationFilenamesForLiveRecheck, resolveLiveMigrationCollisionHold } from "./migration-collision-recheck";
+import { resolveRepositorySettings } from "../settings/repository-settings";
 
 // The agent actor name on every audit record — the App acts on the maintainer's behalf per their configured
 // autonomy (the config IS the authorization; there is no human commenter to authorize, unlike #824).
@@ -168,12 +170,44 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
         continue;
       }
     }
-    // 7) Write-permission readiness: a PR-write action needs `pull_requests: write` granted.
+    // 7) Actuation-time migrations/** recheck (#2550): planning already checks this, but a sibling PR can merge
+    //    a same-numbered migration while a merge waits in auto_with_approval or between planning and actuation.
+    //    Re-read the live base tree at the final mutation boundary so the checked invariant is fresh.
+    if (action.actionClass === "merge") {
+      const changedFiles = await listPullRequestFiles(env, ctx.repoFullName, ctx.pullNumber);
+      const { prMigrationFilenames, prRemovedMigrationFilenames } = migrationFilenamesForLiveRecheck(changedFiles);
+      if (prMigrationFilenames.length > 0) {
+        const settings = await resolveRepositorySettings(env, ctx.repoFullName);
+        if (settings.premergeContentRecheck === true) {
+          const [pr, migrationToken] = await Promise.all([
+            getPullRequest(env, ctx.repoFullName, ctx.pullNumber),
+            createInstallationToken(env, ctx.installationId).catch(() => undefined),
+          ]);
+          const migrationAdmissionKey = githubRateLimitAdmissionKeyForToken(env, migrationToken, ctx.installationId);
+          const migrationCollisionHold = await resolveLiveMigrationCollisionHold({
+            repoFullName: ctx.repoFullName,
+            baseRef: pr?.baseRef,
+            token: migrationToken,
+            admissionKey: migrationAdmissionKey,
+            prMigrationFilenames,
+            prRemovedMigrationFilenames,
+          });
+          if (migrationCollisionHold !== undefined) {
+            await ensurePullRequestLabel(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, AGENT_LABEL_MIGRATION_COLLISION, { createMissingLabel: true, mode }).catch(() => undefined);
+            await createIssueComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, migrationCollisionHold.comment).catch(() => undefined);
+            await audit("denied", `${migrationCollisionHold.reason} — action not executed`);
+            continue;
+          }
+        }
+      }
+    }
+
+    // 8) Write-permission readiness: a PR-write action needs `pull_requests: write` granted.
     if (PR_WRITE_CLASSES.has(action.actionClass) && resolveAgentPermissionReadiness({ autonomy: ctx.autonomy, installationPermissions: ctx.installationPermissions }) !== "ready") {
       await audit("denied", "pull_requests: write not granted — maintainer must re-consent");
       continue;
     }
-    // 8) live — perform the real mutation, recording success or the error.
+    // 9) live — perform the real mutation, recording success or the error.
     try {
       await performAction(env, ctx, action);
       await audit("completed", action.reason);

src/services/migration-collision-recheck.ts

@@ -0,0 +1,52 @@
+import { detectMigrationCollisions, extractMigrationNumber, KNOWN_MIGRATION_DUPLICATES } from "../db/migration-collisions";
+import { listMigrationFilenamesAtRef } from "../github/migration-tree";
+import type { GitHubRateLimitAdmissionKey } from "../github/client";
+import type { PullRequestFileRecord } from "../types";
+
+export type MigrationCollisionHold = { reason: string; comment: string };
+
+export function migrationFilenamesForLiveRecheck(changedFiles: readonly Pick<PullRequestFileRecord, "path" | "status" | "previousFilename">[]): { prMigrationFilenames: string[]; prRemovedMigrationFilenames: string[] } {
+  const prMigrationFilenames = changedFiles
+    .filter((f) => f.status !== "removed" && f.path.startsWith("migrations/") && f.path.endsWith(".sql"))
+    .map((f) => f.path.slice("migrations/".length));
+  const prRemovedMigrationFilenames = changedFiles.flatMap((f) => {
+    const removed: string[] = [];
+    if (f.status === "removed" && f.path.startsWith("migrations/") && f.path.endsWith(".sql")) {
+      removed.push(f.path.slice("migrations/".length));
+    }
+    if (f.previousFilename && f.previousFilename.startsWith("migrations/") && f.previousFilename.endsWith(".sql")) {
+      removed.push(f.previousFilename.slice("migrations/".length));
+    }
+    return removed;
+  });
+  return { prMigrationFilenames, prRemovedMigrationFilenames };
+}
+
+/**
+ * Live premerge migrations/** collision recheck (#2550). Always reads the base tree fresh; callers invoke this
+ * both while planning and again at merge actuation so an approval-queue wait or concurrent sibling merge cannot
+ * reuse a stale no-collision decision.
+ */
+export async function resolveLiveMigrationCollisionHold(args: {
+  repoFullName: string;
+  baseRef: string | null | undefined;
+  token: string | undefined;
+  admissionKey: GitHubRateLimitAdmissionKey | undefined;
+  prMigrationFilenames: string[];
+  prRemovedMigrationFilenames: string[];
+}): Promise<MigrationCollisionHold | undefined> {
+  if (!args.baseRef) return undefined;
+  const liveFilenames = await listMigrationFilenamesAtRef(args.repoFullName, args.baseRef, args.token, args.admissionKey);
+  if (liveFilenames === null) return undefined;
+  const removedFromBase = new Set(args.prRemovedMigrationFilenames);
+  const effectiveLiveFilenames = liveFilenames.filter((f) => !removedFromBase.has(f));
+  const union = [...new Set([...effectiveLiveFilenames, ...args.prMigrationFilenames])];
+  const prNumbers = new Set(args.prMigrationFilenames.map((f) => extractMigrationNumber(f)).filter((n): n is number => n !== null));
+  const collisions = detectMigrationCollisions(union, KNOWN_MIGRATION_DUPLICATES).filter((c) => prNumbers.has(c.number));
+  if (collisions.length === 0) return undefined;
+  const detail = collisions.map((c) => `${c.paddedNumber}: ${c.files.join(", ")}`).join("; ");
+  return {
+    reason: `live migrations/** collision on ${args.baseRef} (${detail})`,
+    comment: `Gittensory: a live check of \`migrations/**\` on \`${args.baseRef}\` found a migration-number collision that isn't visible from this PR's own diff — another PR merged a same-numbered migration file since this PR's CI last ran (**${detail}**). This PR is held for manual review — please rebase onto the latest \`${args.baseRef}\` and renumber your migration to the next free number before this can merge.`,
+  };
+}