Skip to content

Commit 6920402

Browse files
committed
fix(gate): exclude base-tree renames from the migration collision recheck and harden the tsx shebang
Renaming an already-merged base migration (same number, e.g. a typo fix) unioned both the old name (still live on main pre-merge) and the new name, self-colliding even though the merged tree would only ever have one file at that number. Now the PR's own removed/renamed-away base filenames are subtracted from the live tree before the union. Also harden scripts/check-migrations.mjs's shebang with --no-install so a missing local tsx fails closed instead of npx silently fetching an unverified package from the registry, per the security scanner finding.
1 parent dce0fad commit 6920402

3 files changed

Lines changed: 85 additions & 7 deletions

File tree

scripts/check-migrations.mjs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#!/usr/bin/env -S npx tsx
1+
#!/usr/bin/env -S npx --no-install tsx
22
// Guards the D1 migration set against the silent failure modes that git can't catch:
33
// • two PRs that each grab the same next number (e.g. `0038_foo.sql` + `0038_bar.sql`) are DIFFERENT
44
// files, so git reports no conflict and both merge — then `wrangler d1 migrations apply` runs both

src/queue/processors.ts

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1578,10 +1578,13 @@ export function changedPathsForGuardrail(
15781578
* Live premerge migrations/** collision recheck (#2550). `check-migrations.mjs` (CI) only validates against
15791579
* THIS PR's own branch snapshot at the time CI ran — it can never see a sibling PR that merged a
15801580
* same-numbered migration file to `baseRef` in the meantime. This does the live check right before the
1581-
* merge-decision moment: fetch the base branch's CURRENT migration filenames, union them with THIS PR's own
1582-
* new migration filenames (the live tree never contains this PR's own not-yet-merged files, so checking main
1583-
* alone could never detect a collision from this PR's perspective — the union is load-bearing, not optional),
1584-
* then run the SAME collision-detection function scripts/check-migrations.mjs uses.
1581+
* merge-decision moment: fetch the base branch's CURRENT migration filenames, drop any filename THIS PR's
1582+
* own diff removes from the base (an outright deletion, or a rename's pre-rename name — otherwise renaming
1583+
* an existing base migration self-collides with its own old name, which is still live on `baseRef` until
1584+
* this PR merges), union what's left with THIS PR's own new migration filenames (the live tree never
1585+
* contains this PR's own not-yet-merged files, so checking main alone could never detect a collision from
1586+
* this PR's perspective — the union is load-bearing, not optional), then run the SAME collision-detection
1587+
* function scripts/check-migrations.mjs uses.
15851588
*
15861589
* Deliberately scoped to a collision involving THIS PR's own migration number(s) only (via `prNumbers`) — a
15871590
* pre-existing collision between two OTHER already-merged files (which would mean `main` itself is already
@@ -1605,12 +1608,15 @@ async function resolveLiveMigrationCollisionHold(
16051608
token: string | undefined;
16061609
admissionKey: GitHubRateLimitAdmissionKey | undefined;
16071610
prMigrationFilenames: string[];
1611+
prRemovedMigrationFilenames: string[];
16081612
},
16091613
): Promise<{ reason: string; comment: string } | undefined> {
16101614
if (!args.baseRef) return undefined;
16111615
const liveFilenames = await listMigrationFilenamesAtRef(args.repoFullName, args.baseRef, args.token, args.admissionKey);
16121616
if (liveFilenames === null) return undefined;
1613-
const union = [...new Set([...liveFilenames, ...args.prMigrationFilenames])];
1617+
const removedFromBase = new Set(args.prRemovedMigrationFilenames);
1618+
const effectiveLiveFilenames = liveFilenames.filter((f) => !removedFromBase.has(f));
1619+
const union = [...new Set([...effectiveLiveFilenames, ...args.prMigrationFilenames])];
16141620
const prNumbers = new Set(args.prMigrationFilenames.map((f) => extractMigrationNumber(f)).filter((n): n is number => n !== null));
16151621
const collisions = detectMigrationCollisions(union, KNOWN_MIGRATION_DUPLICATES).filter((c) => prNumbers.has(c.number));
16161622
if (collisions.length === 0) return undefined;
@@ -1888,9 +1894,32 @@ async function runAgentMaintenancePlanAndExecute(
18881894
const prMigrationFilenames = changedFiles
18891895
.filter((f) => f.status !== "removed" && f.path.startsWith("migrations/") && f.path.endsWith(".sql"))
18901896
.map((f) => f.path.slice("migrations/".length));
1897+
// Base filenames this PR's diff removes from `migrations/**` — an outright deletion's own `.path`, or a
1898+
// rename's pre-rename `.previousFilename` — so a filename that won't exist once this PR merges isn't still
1899+
// counted from the live base fetch below. Without this, renaming an EXISTING base migration within the same
1900+
// number (e.g. `migrations/0099_old.sql` -> `migrations/0099_new.sql`, fixing a typo on an already-merged
1901+
// file) unions both the old (still live) and new (this PR's) name and self-collides, even though the merged
1902+
// tree would only ever contain the new file.
1903+
const prRemovedMigrationFilenames = changedFiles.flatMap((f) => {
1904+
const removed: string[] = [];
1905+
if (f.status === "removed" && f.path.startsWith("migrations/") && f.path.endsWith(".sql")) {
1906+
removed.push(f.path.slice("migrations/".length));
1907+
}
1908+
if (f.previousFilename && f.previousFilename.startsWith("migrations/") && f.previousFilename.endsWith(".sql")) {
1909+
removed.push(f.previousFilename.slice("migrations/".length));
1910+
}
1911+
return removed;
1912+
});
18911913
const migrationCollisionHold =
18921914
settings.premergeContentRecheck === true && prMigrationFilenames.length > 0
1893-
? await resolveLiveMigrationCollisionHold({ repoFullName, baseRef, token, admissionKey, prMigrationFilenames })
1915+
? await resolveLiveMigrationCollisionHold({
1916+
repoFullName,
1917+
baseRef,
1918+
token,
1919+
admissionKey,
1920+
prMigrationFilenames,
1921+
prRemovedMigrationFilenames,
1922+
})
18941923
: undefined;
18951924
const repoOwner = repoFullName.includes("/")
18961925
? repoFullName.slice(0, repoFullName.indexOf("/"))

test/unit/queue.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5979,6 +5979,55 @@ describe("queue processors", () => {
59795979
expect(seen.labels).not.toContain("gittensory:migration-collision");
59805980
});
59815981

5982+
it("REGRESSION: renaming an EXISTING base migration (same number) does NOT self-collide with its own old name still live on main", async () => {
5983+
// Before the fix, liveFilenames (fetched from main, which still has the pre-rename name until this PR
5984+
// merges) was unioned as-is with prMigrationFilenames (the new name only) — so a same-number typo-fix
5985+
// rename of an ALREADY-MERGED base migration counted as two distinct files at one number and
5986+
// self-collided, even though the merged tree would only ever contain the renamed file.
5987+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
5988+
await seedMigrationRecheckRepo(env, 73, { premergeContentRecheck: true });
5989+
const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 };
5990+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
5991+
const url = input.toString();
5992+
const method = (init?.method ?? "GET").toUpperCase();
5993+
if (url === "https://api.gittensor.io/miners") return Response.json([]);
5994+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
5995+
if (url.includes("/check-runs/") && method === "PATCH") return Response.json({ id: 901 });
5996+
if (url.includes("/git/trees/main")) {
5997+
seen.treeCalls += 1;
5998+
// main still has the PRE-rename name — this PR's rename hasn't merged yet.
5999+
return Response.json({ tree: [{ type: "blob", path: "migrations/0099_old.sql" }] });
6000+
}
6001+
if (/\/pulls\/\d+(?:\?|$)/.test(url) && method === "GET" && !url.includes("/pulls/73/")) {
6002+
return Response.json({ number: 73, state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main", sha: "base" }, mergeable_state: "clean", labels: [] });
6003+
}
6004+
// Renames an EXISTING base migration (same number 0099), not a file this PR itself added.
6005+
if (url.includes("/pulls/73/files")) return Response.json([{ filename: "migrations/0099_new.sql", previous_filename: "migrations/0099_old.sql", status: "renamed", additions: 1, deletions: 1, changes: 2, patch: "@@\n rename" }]);
6006+
if (url.includes("/commits/sha1/check-runs")) return Response.json({ total_count: 0, check_runs: [] });
6007+
if (url.includes("/commits/sha1/status")) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] });
6008+
if (url.includes("/commits/sha1/check-suites")) return Response.json({ check_suites: [] });
6009+
if (url.includes("/branches/")) return Response.json({ contexts: [] });
6010+
if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } });
6011+
if (url.includes("/pulls/73/merge") && method === "PUT") {
6012+
seen.merged = true;
6013+
return Response.json({ merged: true, sha: "merged-sha1" });
6014+
}
6015+
if (url.includes("/issues/73/labels") && method === "GET") return Response.json([]);
6016+
if (url.includes("/issues/73/labels") && method === "POST") {
6017+
seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[]));
6018+
return Response.json([]);
6019+
}
6020+
if (url.endsWith("/labels") && method === "POST") return Response.json({ name: "x" }, { status: 201 });
6021+
if (url.includes("/issues/73/comments")) return Response.json([]);
6022+
return Response.json({});
6023+
});
6024+
6025+
await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-rename-existing-base", repoFullName: "owner/repo", prNumber: 73, installationId: 123 });
6026+
6027+
expect(seen.merged).toBe(true); // the pre-rename name still live on main must not count against this PR
6028+
expect(seen.labels).not.toContain("gittensory:migration-collision");
6029+
});
6030+
59826031
it("REGRESSION: renumbering (renaming) this PR's migration to resolve a real collision does not leave a stale hold from the old filename", async () => {
59836032
// Before the fix, the stale previousFilename (the OLD number) stayed in prMigrationFilenames forever,
59846033
// colliding with an unrelated already-merged file at that old number and permanently re-holding a PR

0 commit comments

Comments
 (0)