feat(gate): re-check migration-file collisions against live main before merge (#2550)

.gittensory.yml.example

@@ -153,6 +153,15 @@ gate:
   # use. Bool. Default: false.
   firstTimeContributorGrace: false
 
+  # Live premerge migrations/** collision recheck (#2550, anti-abuse-adjacent safety net). When true, a PR
+  # touching migrations/** gets a fresh GitHub read of the base branch's CURRENT migration filenames
+  # immediately before an agent-driven merge (not just at CI time against this PR's own branch snapshot) —
+  # catching the case where a DIFFERENT PR merged a same-numbered migration file in the meantime. A live
+  # collision holds the PR (rebase-needed label + comment) instead of merging blind. Config-as-code only (no
+  # dashboard/DB equivalent). Bool. Default: false. Costs one extra GitHub API call per migrations/**-touching
+  # PR, so it is opt-in rather than a new default.
+  premergeContentRecheck: false
+
   # AI maintainer review. Opt-in; the AI capabilities are switched on at the
   # deployment level.
   aiReview:

package.json

@@ -27,7 +27,7 @@
     "rees:metadata": "npm --prefix review-enrichment run metadata",
     "rees:metadata:check": "npm --prefix review-enrichment run metadata:check",
     "rees:validate-sourcemaps": "npm --prefix review-enrichment run validate:sourcemaps",
-    "db:migrations:check": "node scripts/check-migrations.mjs",
+    "db:migrations:check": "tsx scripts/check-migrations.mjs",
     "actionlint": "node scripts/actionlint.mjs",
     "ui:dev": "npm run ui:preview",
     "extension:build": "node scripts/build-extension.mjs",

scripts/check-migrations.mjs

@@ -1,18 +1,24 @@
-#!/usr/bin/env node
+#!/usr/bin/env -S npx --no-install tsx
 // Guards the D1 migration set against the silent failure modes that git can't catch:
 //   • two PRs that each grab the same next number (e.g. `0038_foo.sql` + `0038_bar.sql`) are DIFFERENT
 //     files, so git reports no conflict and both merge — then `wrangler d1 migrations apply` runs both
 //     in filename order and, if they touch the same column, errors mid-deploy.
 //   • a skipped number (gap) or a stray non-conforming filename.
 // Migration-only PRs trigger this via the `migrations/**` path filter in .github/workflows/ci.yml.
 //
-// KNOWN_DUPLICATES: pairs already merged AND applied in production before the collision was noticed — D1
-// records applied migrations by filename, so renaming either now would make wrangler (and the self-host
-// migrator) try to RE-APPLY it. For a non-idempotent statement (e.g. `ALTER TABLE … ADD COLUMN`, which SQLite
-// cannot guard with IF NOT EXISTS) the re-apply ERRORS and breaks the deploy, so renumbering is unsafe once the
-// dup has shipped — those are grandfathered here. Do NOT add a NEW (not-yet-merged) duplicate to this list:
-// renumber its branch to the next free number BEFORE merge. Only an already-shipped, can't-be-renumbered dup
-// belongs here.
+// Run via `tsx` (not plain `node`), not for style but because this script's number/duplicate-detection logic
+// is imported from src/db/migration-collisions.ts (#2550) — the SAME pure module the live premerge recheck
+// uses inside the Worker, so CI and the Worker can never disagree about what counts as a collision. Plain
+// `node` cannot resolve a `.ts` import without a flag CI's pinned Node version isn't guaranteed to support;
+// `tsx` is already an established devDependency for exactly this (see ui:openapi/selfhost:postgres:migrate).
+//
+// KNOWN_MIGRATION_DUPLICATES (src/db/migration-collisions.ts): pairs already merged AND applied in production
+// before the collision was noticed — D1 records applied migrations by filename, so renaming either now would
+// make wrangler (and the self-host migrator) try to RE-APPLY it. For a non-idempotent statement (e.g.
+// `ALTER TABLE … ADD COLUMN`, which SQLite cannot guard with IF NOT EXISTS) the re-apply ERRORS and breaks the
+// deploy, so renumbering is unsafe once the dup has shipped — those are grandfathered there. Do NOT add a NEW
+// (not-yet-merged) duplicate: renumber its branch to the next free number BEFORE merge. Only an
+// already-shipped, can't-be-renumbered dup belongs there.
 //   • 0015 / 0017 — predate the guard.
 //   • 0074 — both 0074_ai_review_cache (#1462) and 0074_orb_self_enrollment_disabled (#1465, a bare ADD COLUMN)
 //     merged + deployed before the collision surfaced; the column already exists in prod, so a rename would
@@ -21,15 +27,11 @@
 //     merged with bare ADD COLUMN statements. Preserve both filenames so already-applied databases never
 //     replay either ALTER under a new migration name.
 import { readdirSync, readFileSync } from "node:fs";
+import { detectMigrationCollisions, extractMigrationNumber, KNOWN_MIGRATION_DUPLICATES, MIGRATION_FILENAME_PATTERN } from "../src/db/migration-collisions.ts";
 
 const DIR = process.env.CHECK_MIGRATIONS_DIR || "migrations";
-const NAME = /^(\d{4})_[a-z0-9]+(?:_[a-z0-9]+)*\.sql$/;
-const KNOWN_DUPLICATES = new Map([
-  [15, new Set(["0015_github_agent_command_feedback.sql", "0015_product_usage_events.sql"])],
-  [17, new Set(["0017_agent_recommendation_outcomes.sql", "0017_product_usage_role_retention_rollups.sql"])],
-  [74, new Set(["0074_ai_review_cache.sql", "0074_orb_self_enrollment_disabled.sql"])],
-  [90, new Set(["0090_contributor_cap_label.sql", "0090_pull_request_detail_sync_head_sha.sql"])],
-]);
+const NAME = MIGRATION_FILENAME_PATTERN;
+const KNOWN_DUPLICATES = KNOWN_MIGRATION_DUPLICATES;
 
 const fail = (message) => {
   process.stderr.write(`check-migrations: ${message}\n`);
@@ -137,7 +139,7 @@ if (malformed.length > 0) {
 
 const filesByNumber = new Map();
 for (const file of files) {
-  const number = Number(NAME.exec(file)[1]);
+  const number = extractMigrationNumber(file);
   if (!filesByNumber.has(number)) filesByNumber.set(number, []);
   filesByNumber.get(number).push(file);
 }
@@ -148,14 +150,13 @@ const nextFree = () => {
   return String(n).padStart(4, "0");
 };
 
-for (const [number, group] of filesByNumber) {
-  if (group.length === 1) continue;
-  const padded = String(number).padStart(4, "0");
-  const allowed = KNOWN_DUPLICATES.get(number);
-  const grandfathered = allowed && group.length === allowed.size && group.every((f) => allowed.has(f));
-  if (!grandfathered) {
-    fail(`duplicate migration number ${padded}: ${group.map((f) => `"${f}"`).join(", ")}. Two PRs grabbed the same number — renumber the newest to the next free number (${nextFree()}).`);
-  }
+// #2550: the actual duplicate-vs-grandfathered decision runs through detectMigrationCollisions, the SAME
+// pure function the live premerge recheck uses — this is not just the equivalent logic re-derived here, it
+// is the identical import, so CI and the Worker can never silently disagree about what counts as a collision.
+const collisions = detectMigrationCollisions(files, KNOWN_DUPLICATES);
+if (collisions.length > 0) {
+  const { paddedNumber, files: group } = collisions[0];
+  fail(`duplicate migration number ${paddedNumber}: ${group.map((f) => `"${f}"`).join(", ")}. Two PRs grabbed the same number — renumber the newest to the next free number (${nextFree()}).`);
 }
 
 const numbers = [...filesByNumber.keys()].sort((a, b) => a - b);

src/db/migration-collisions.ts

@@ -0,0 +1,57 @@
+// Pure, fs-free migration-collision detection (#2550), shared by scripts/check-migrations.mjs (CI, reads the
+// local filesystem) and the live premerge recheck (src/queue/processors.ts, reads a GitHub-API-fetched
+// filename list) — a single source of truth so the two never drift apart.
+
+/** Matches scripts/check-migrations.mjs's NAME regex exactly. */
+export const MIGRATION_FILENAME_PATTERN = /^(\d{4})_[a-z0-9]+(?:_[a-z0-9]+)*\.sql$/;
+
+export type MigrationCollision = {
+  number: number;
+  paddedNumber: string;
+  files: string[];
+};
+
+/** Extract the 4-digit migration number from a conforming filename, or null if it doesn't match
+ *  MIGRATION_FILENAME_PATTERN — a malformed filename is a separate concern (the CI script's own malformed-name
+ *  check), not something this function flags. */
+export function extractMigrationNumber(filename: string): number | null {
+  const match = MIGRATION_FILENAME_PATTERN.exec(filename);
+  return match ? Number(match[1]) : null;
+}
+
+/** The pairs already merged AND applied in production before the collision was noticed (see
+ *  scripts/check-migrations.mjs's own header comment for why these can never be renumbered). Kept in lockstep
+ *  with that script's KNOWN_DUPLICATES — both must list the exact same grandfathered sets. */
+export const KNOWN_MIGRATION_DUPLICATES: ReadonlyMap<number, ReadonlySet<string>> = new Map([
+  [15, new Set(["0015_github_agent_command_feedback.sql", "0015_product_usage_events.sql"])],
+  [17, new Set(["0017_agent_recommendation_outcomes.sql", "0017_product_usage_role_retention_rollups.sql"])],
+  [74, new Set(["0074_ai_review_cache.sql", "0074_orb_self_enrollment_disabled.sql"])],
+  [90, new Set(["0090_contributor_cap_label.sql", "0090_pull_request_detail_sync_head_sha.sql"])],
+]);
+
+/**
+ * Group filenames by their migration number and return every number with more than one file, minus any
+ * EXACT-set match against `knownDuplicates` (same grandfather semantics as scripts/check-migrations.mjs:
+ * the group must be the identical size and every file in it must be in the allowed set — a third file at an
+ * already-grandfathered number, or a substitution, is still flagged). Non-conforming filenames are ignored —
+ * malformed-filename detection is a separate, CI-only concern. Pure, no I/O.
+ */
+export function detectMigrationCollisions(filenames: readonly string[], knownDuplicates: ReadonlyMap<number, ReadonlySet<string>> = new Map()): MigrationCollision[] {
+  const byNumber = new Map<number, string[]>();
+  for (const file of filenames) {
+    const number = extractMigrationNumber(file);
+    if (number === null) continue;
+    const group = byNumber.get(number);
+    if (group) group.push(file);
+    else byNumber.set(number, [file]);
+  }
+  const collisions: MigrationCollision[] = [];
+  for (const [number, files] of byNumber) {
+    if (files.length === 1) continue;
+    const allowed = knownDuplicates.get(number);
+    const grandfathered = allowed !== undefined && files.length === allowed.size && files.every((f) => allowed.has(f));
+    if (grandfathered) continue;
+    collisions.push({ number, paddedNumber: String(number).padStart(4, "0"), files: [...files].sort() });
+  }
+  return collisions.sort((a, b) => a.number - b.number);
+}

src/github/migration-tree.ts

@@ -0,0 +1,57 @@
+import { timeoutFetch, type GitHubRateLimitAdmissionKey } from "./client";
+import { repoParts } from "../utils/json";
+
+const GITHUB_FETCH_TIMEOUT_MS = 10_000;
+const MIGRATIONS_PREFIX = "migrations/";
+
+/** Shared GitHub headers for a read call. */
+function ghHeaders(token: string | undefined): Record<string, string> {
+  return {
+    accept: "application/vnd.github+json",
+    "user-agent": "gittensory/0.1",
+    "x-github-api-version": "2022-11-28",
+    ...(token ? { authorization: `Bearer ${token}` } : {}),
+  };
+}
+
+/**
+ * List the `.sql` filenames directly under `migrations/` at `ref` (typically the live tip of the base branch)
+ * via the recursive Git Trees API — the same primitive src/review/rag-index.ts's fetchRepoTree uses, but
+ * scoped to a single path prefix so a caller doesn't need to re-derive the migrations/-filter logic (#2550).
+ * Kept as its own small, single-purpose helper rather than exporting/reusing fetchRepoTree directly — that
+ * function is rag-index.ts's own indexing concern (returns the WHOLE tree, unfiltered); coupling this feature
+ * to it for one extra call site isn't worth the cross-module dependency.
+ *
+ * Fail-safe: any non-OK response, network error, or malformed body returns null (never throws). Callers MUST
+ * treat null as "recheck inconclusive" and never treat it as evidence of (or absence of) a collision — a live
+ * pre-merge safety check must fail OPEN on a read failure, not silently hold every PR whenever GitHub hiccups.
+ */
+export async function listMigrationFilenamesAtRef(repoFullName: string, ref: string, token: string | undefined, admissionKey: GitHubRateLimitAdmissionKey | undefined): Promise<string[] | null> {
+  try {
+    const { owner, name } = repoParts(repoFullName);
+    const url = `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/git/trees/${encodeURIComponent(ref)}?recursive=1`;
+    const response = await timeoutFetch(url, {
+      headers: ghHeaders(token),
+      signal: AbortSignal.timeout(GITHUB_FETCH_TIMEOUT_MS),
+      githubRateLimitAdmission: admissionKey !== undefined,
+      ...(admissionKey ? { githubRateLimitAdmissionKey: admissionKey } : {}),
+    });
+    if (!response.ok) return null;
+    const body = (await response.json()) as { tree?: Array<{ path?: string; type?: string }>; truncated?: boolean } | null;
+    // A truncated tree (repo exceeds GitHub's ~100k-entry/7MB response cap) can silently omit migrations/
+    // entries — treat exactly like a fetch failure (null, fail-open) rather than trusting a possibly-incomplete
+    // list, since an incomplete live snapshot is inconclusive, not evidence of "no collision."
+    if (body?.truncated === true) return null;
+    const filenames: string[] = [];
+    for (const node of body?.tree ?? []) {
+      if (node.type !== "blob" || typeof node.path !== "string") continue;
+      if (!node.path.startsWith(MIGRATIONS_PREFIX)) continue;
+      const rest = node.path.slice(MIGRATIONS_PREFIX.length);
+      if (rest.length === 0 || rest.includes("/")) continue; // skip nested dirs, defensively — migrations/ is flat
+      if (rest.endsWith(".sql")) filenames.push(rest);
+    }
+    return filenames;
+  } catch {
+    return null;
+  }
+}