diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 637c01691a..00396ff35c 100644 --- a/.gittensory.yml.example +++ b/.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: diff --git a/package.json b/package.json index 6643cd9a7d..079d6afea3 100644 --- a/package.json +++ b/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", diff --git a/scripts/check-migrations.mjs b/scripts/check-migrations.mjs index 6e0a911b17..cb3c2be963 100644 --- a/scripts/check-migrations.mjs +++ b/scripts/check-migrations.mjs @@ -1,4 +1,4 @@ -#!/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 @@ -6,13 +6,19 @@ // • 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); diff --git a/src/db/migration-collisions.ts b/src/db/migration-collisions.ts new file mode 100644 index 0000000000..1e32371371 --- /dev/null +++ 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> = 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> = new Map()): MigrationCollision[] { + const byNumber = new Map(); + 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); +} diff --git a/src/github/migration-tree.ts b/src/github/migration-tree.ts new file mode 100644 index 0000000000..1c728c0cdf --- /dev/null +++ b/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 { + 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 { + 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; + } +} diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ae9c2c1fc9..65cf515d63 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -234,6 +234,8 @@ import { type PlannedAgentAction, } from "../settings/agent-actions"; import { isAutoCloseExempt } from "../settings/auto-close-exempt"; +import { detectMigrationCollisions, extractMigrationNumber, KNOWN_MIGRATION_DUPLICATES } from "../db/migration-collisions"; +import { listMigrationFilenamesAtRef } from "../github/migration-tree"; import { executeAgentMaintenanceActions, executeIssueMaintenanceActions, @@ -1572,6 +1574,59 @@ 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 @@ -1823,6 +1878,49 @@ async function runAgentMaintenancePlanAndExecute( ); } const changedPaths = changedPathsForGuardrail(changedFiles); + // #2550: live migrations/** collision recheck — config-gated (off by default) AND path-gated (only a PR + // that actually touches migrations/** pays the extra GitHub API call), so a non-migrations PR sees zero + // added latency: the whole block short-circuits on the boolean+array checks before any network call. + // + // Deliberately built from `changedFiles` directly, NOT from `changedPaths` (changedPathsForGuardrail's + // output) — that helper unions BOTH `file.path` (current name) and `file.previousFilename` (pre-rename + // name) into one flat set for its own, unrelated guardrail-path-matching purpose. Reusing it here would + // mean a PR that simply RENAMES its own not-yet-merged migration file (e.g. fixing a typo, or renumbering + // to resolve a collision — the exact remediation this feature's own hold comment recommends) counts BOTH + // the old and new filenames as "this PR's own migration files", numerically colliding with itself and + // 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 migrationCollisionHold = + settings.premergeContentRecheck === true && prMigrationFilenames.length > 0 + ? await resolveLiveMigrationCollisionHold({ + repoFullName, + baseRef, + token, + admissionKey, + prMigrationFilenames, + prRemovedMigrationFilenames, + }) + : undefined; const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")) : ""; @@ -1935,6 +2033,7 @@ async function runAgentMaintenancePlanAndExecute( verifyBeforeClose: linkedIssueRulesConfig.verifyBeforeClose, closeDelaySeconds: linkedIssueRulesConfig.closeDelaySeconds, }, + ...(migrationCollisionHold !== undefined ? { migrationCollisionHold } : {}), pr: { mergeableState: liveMergeState ?? pr.mergeableState, reviewDecision: liveReviewDecision ?? pr.reviewDecision, diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 2710fb2138..34a12b9871 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -36,6 +36,11 @@ export const DEFAULT_REVIEW_NAG_LABEL = "review-nag-cooldown"; // would be misleading (the label promises an auto-merge that never happens), so a guarded passing PR gets // this distinct "needs a human" label instead. Blocking verdicts keep AGENT_LABEL_CHANGES. export const AGENT_LABEL_NEEDS_REVIEW = "gittensory:needs-human-review"; +// A PR that touches migrations/** and would otherwise auto-merge, but a LIVE recheck against the current tip +// of the base branch found a migration-number collision with a sibling PR merged since this PR's CI last ran +// (#2550). Distinct from AGENT_LABEL_NEEDS_REVIEW so an operator can filter/alert on this specific, proven- +// recurring failure mode separately from an ordinary guardrail hold. +export const AGENT_LABEL_MIGRATION_COLLISION = "gittensory:migration-collision"; // Maintainer-managed automation accounts whose PRs are never auto-closed. A recurring accumulator (e.g. // github-actions[bot] opening automation/readme-refresh) or a dependency PR must not be killed by a duplicate @@ -185,6 +190,14 @@ export type AgentActionPlanInput = { // pass it). `closeDelaySeconds` is surfaced in the flag comment so the contributor knows the verification // window. The presence of the label is read from `input.pr.labels`. linkedIssueVerify?: { verifyBeforeClose: boolean; closeDelaySeconds: number } | undefined; + // Live premerge migrations/** collision recheck (#2550). The trigger (runAgentMaintenancePlanAndExecute) has + // already fetched the base branch's LIVE migration filenames, unioned them with this PR's own new migration + // additions, and run collision detection — this input is already the resolved "yes, hold this merge" + // verdict (or absent, meaning no live collision was found). When present, this SUPPRESSES the merge exactly + // like a hard-guardrail hold (folded into `heldForManualReview`), and its `comment` is attached to the + // emitted `gittensory:migration-collision` label so the contributor knows why. Never causes a CLOSE — only + // ever downgrades a would-merge into a held-for-review state, same risk profile as the guardrail hold. + migrationCollisionHold?: { reason: string; comment: string } | undefined; pr: { mergeableState?: string | null | undefined; reviewDecision?: string | null | undefined; @@ -420,12 +433,14 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // never auto-merge, auto-approve, or auto-close a PR whose diff we don't know. Repos with no guardrails // configured stay permissive. const guardrailHit = isGuardrailHit(input.changedPaths, input.hardGuardrailGlobs); - // Manual review is the RARE exception (the operator's minimize-manual goal): the ONLY thing that holds a PR for - // a human instead of merge/close is an auto-merge-ready PR that touches a hard-guardrail path. (An owner PR that - // is not review-good is held separately, via the owner close-exemption below — never auto-closed.) Submission - // volume is NOT a hold reason: a high-volume author's clean PR still merges and their bad PR still closes — the - // quality gate, not a submission count, is the defense (anti-farming-by-manual-hold removed). - const heldForManualReview = guardrailHit; + // Manual review is the RARE exception (the operator's minimize-manual goal): the ONLY things that hold a PR + // for a human instead of merge/close are an auto-merge-ready PR that touches a hard-guardrail path, or a + // live migration-number collision detected against the CURRENT tip of the base branch (#2550 — a sibling PR + // merged a same-numbered migration file since this PR's CI last ran). (An owner PR that is not review-good + // is held separately, via the owner close-exemption below — never auto-closed.) Submission volume is NOT a + // hold reason: a high-volume author's clean PR still merges and their bad PR still closes — the quality + // gate, not a submission count, is the defense (anti-farming-by-manual-hold removed). + const heldForManualReview = guardrailHit || input.migrationCollisionHold !== undefined; // Canonical (reviewbot non-content-gate) policy, tuned to the operator's minimize-manual goal: merge-or-close // with high accuracy; manual review is the RARE exception. A PR is "review-good" when the gate passes AND CI is // green — that's the only thing that earns an auto-merge or an approve. Everything else, for a CONTRIBUTOR, is a @@ -503,16 +518,29 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // linked-issue hard-rule close (flag OR close pass) forces the changes-requested label regardless of the gate // verdict (the PR is about to be closed for an ineligible linked issue). Idempotent. if (acting("label")) { - const label = linkedIssueCloseInFlight || !reviewGood ? AGENT_LABEL_CHANGES : heldForManualReview ? AGENT_LABEL_NEEDS_REVIEW : AGENT_LABEL_READY; + // A live migration-collision hold takes priority over a plain guardrail hold when both are true — it is + // the more specific, actionable signal (tells the contributor exactly what to do: rebase), and gets its + // own distinct label (#2550) so an operator can filter/alert on it separately from an ordinary guardrail. + const label = linkedIssueCloseInFlight || !reviewGood ? AGENT_LABEL_CHANGES : input.migrationCollisionHold !== undefined ? AGENT_LABEL_MIGRATION_COLLISION : heldForManualReview ? AGENT_LABEL_NEEDS_REVIEW : AGENT_LABEL_READY; const reason = linkedIssueCloseInFlight ? `linked-issue hard rule: ${linkedIssueHardRule?.reason ?? "ineligible linked issue"}` : !reviewGood ? `verdict=${conclusion}${ciReason ? `; ${ciReason}` : ""}` - : heldForManualReview - ? `verdict=${conclusion}; guarded path → manual review` - : `verdict=${conclusion}; CI green`; + : input.migrationCollisionHold !== undefined + ? `verdict=${conclusion}; ${input.migrationCollisionHold.reason}` + : heldForManualReview + ? `verdict=${conclusion}; guarded path → manual review` + : `verdict=${conclusion}; CI green`; if (!hasLabel(input.pr.labels, label)) { - actions.push({ actionClass: "label", requiresApproval: approval("label"), reason, label }); + actions.push({ + actionClass: "label", + requiresApproval: approval("label"), + reason, + label, + // Only the migration-collision hold carries a comment here — the guardrail/ready/changes labels never + // did and still don't (comment stays undefined, matching the pre-#2550 shape exactly). + ...(!linkedIssueCloseInFlight && reviewGood && input.migrationCollisionHold !== undefined ? { comment: sanitizePublicComment(input.migrationCollisionHold.comment) } : {}), + }); } // Flag-then-close double-check, Pass 1: add the pending-closure label + a warning comment citing the specific // rule and the verification window. The label's presence is the state that, persisting to the next pass with diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 80c102524d..81c20e1ae3 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -45,6 +45,14 @@ export type FocusManifestGateConfig = { selfAuthoredLinkedIssue: GateRuleMode | null; dryRun: boolean | null; firstTimeContributorGrace: boolean | null; + /** `gate.premergeContentRecheck` (#2550): for a PR touching `migrations/**`, re-verify against a live, + * freshly-fetched tip of the base branch — unioned with this PR's own new migration filenames — for a + * migration-number collision immediately before an agent-driven merge, not just at CI time against the + * PR's own stale branch snapshot. On a live collision, the merge is suppressed and the PR is held with a + * rebase-needed comment instead of merging blind. null (unset) ⇒ off (byte-identical to today) — this + * costs one extra, uncached GitHub Trees-API call for any PR that touches migrations/**, so it is opt-in + * rather than a new default. */ + premergeContentRecheck: boolean | null; }; // The converged per-PR review features a self-host operator toggles PER-REPO under `features:` in the private @@ -292,6 +300,7 @@ const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { selfAuthoredLinkedIssue: null, dryRun: null, firstTimeContributorGrace: null, + premergeContentRecheck: null, }; const EMPTY_FEATURES_CONFIG: FocusManifestFeaturesConfig = { @@ -510,6 +519,7 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu selfAuthoredLinkedIssue: normalizeOptionalGateMode(record.selfAuthoredLinkedIssue, "gate.selfAuthoredLinkedIssue", warnings), dryRun: normalizeOptionalBoolean(record.dryRun, "gate.dryRun", warnings), firstTimeContributorGrace: normalizeOptionalBoolean(record.firstTimeContributorGrace, "gate.firstTimeContributorGrace", warnings), + premergeContentRecheck: normalizeOptionalBoolean(record.premergeContentRecheck, "gate.premergeContentRecheck", warnings), }; // #2266: the flag is parsed, clamped, and threaded end-to-end, but the gate evaluator never reads it — a // maintainer who sets it to true believing it softens a blocker for newcomers gets no such effect. Surface @@ -539,7 +549,8 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.manifestPolicy !== null || gate.selfAuthoredLinkedIssue !== null || gate.dryRun !== null || - gate.firstTimeContributorGrace !== null; + gate.firstTimeContributorGrace !== null || + gate.premergeContentRecheck !== null; return gate; } @@ -583,6 +594,7 @@ export function gateConfigToJson(gate: FocusManifestGateConfig): JsonValue { if (gate.selfAuthoredLinkedIssue !== null) out.selfAuthoredLinkedIssue = gate.selfAuthoredLinkedIssue; if (gate.dryRun !== null) out.dryRun = gate.dryRun; if (gate.firstTimeContributorGrace !== null) out.firstTimeContributorGrace = gate.firstTimeContributorGrace; + if (gate.premergeContentRecheck !== null) out.premergeContentRecheck = gate.premergeContentRecheck; return out; } @@ -1194,6 +1206,7 @@ export function resolveEffectiveSettings( if (gate.selfAuthoredLinkedIssue !== null) effective.selfAuthoredLinkedIssueGateMode = gate.selfAuthoredLinkedIssue; if (gate.dryRun !== null) effective.gateDryRun = gate.dryRun; if (gate.firstTimeContributorGrace !== null) effective.firstTimeContributorGrace = gate.firstTimeContributorGrace; + if (gate.premergeContentRecheck !== null) effective.premergeContentRecheck = gate.premergeContentRecheck; // The dashboard "Require linked issue" toggle must not silently diverge from gate blocking: when the // boolean is on but linkedIssueGateMode is still off, treat it as a block requirement (#797). if (effective.requireLinkedIssue && effective.linkedIssueGateMode === "off") { diff --git a/src/types.ts b/src/types.ts index 608c0e6a4e..21a2007a58 100644 --- a/src/types.ts +++ b/src/types.ts @@ -536,6 +536,14 @@ export type RepositorySettings = { * advisory sub-gate promoted to block) WITHOUT enforcing — the posted check stays non-blocking. Lets advisory mode * preview exactly what it would do before the maintainer flips to real enforcement. Default off. */ gateDryRun?: boolean | undefined; + /** Live premerge migrations/** collision recheck (#2550). When true, an agent-driven merge of a PR that + * touches migrations/** is preceded by a fresh GitHub Trees-API read of the base branch's CURRENT migration + * filenames — unioned with this PR's own new migration filenames — checked for a live numeric collision. + * A collision suppresses the merge and holds the PR with a rebase-needed label + comment instead of merging + * blind. Config-as-code only (no DB column, mirrors gateDryRun) — set via `.gittensory.yml` + * `gate.premergeContentRecheck`. Default off/undefined — opt-in, since it costs one extra, uncached + * GitHub API call for any PR that touches migrations/**. */ + premergeContentRecheck?: boolean | undefined; /** Merge-readiness gate (#merge-readiness). `off`/`advisory`/`block`. No min-score. Default `off`. */ mergeReadinessGateMode: GateRuleMode; /** Focus-manifest policy gate (#555). When `block`, the focus manifest's declared policy (blocked paths, diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index a786f9d154..1b6ffda6e8 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { AGENT_LABEL_CHANGES, AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, DEFAULT_BLACKLIST_LABEL, DEFAULT_CONTRIBUTOR_CAP_LABEL, DEFAULT_REVIEW_NAG_LABEL, downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, planAgentMaintenanceActions, type AgentActionPlanInput, type PlannedAgentAction } from "../../src/settings/agent-actions"; +import { AGENT_LABEL_CHANGES, AGENT_LABEL_MIGRATION_COLLISION, AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, DEFAULT_BLACKLIST_LABEL, DEFAULT_CONTRIBUTOR_CAP_LABEL, DEFAULT_REVIEW_NAG_LABEL, downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, planAgentMaintenanceActions, type AgentActionPlanInput, type PlannedAgentAction } from "../../src/settings/agent-actions"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; import type { GateCheckConclusion } from "../../src/rules/advisory"; @@ -327,6 +327,61 @@ describe("planAgentMaintenanceActions (#778)", () => { }); }); + describe("live migration-collision hold: a live premerge recheck found a same-numbered sibling on the base branch (#2550)", () => { + const collided = { migrationCollisionHold: { reason: "live migrations/** collision on main (0090: 0090_a.sql, 0090_b.sql)", comment: "Gittensory: a live check found a migration-number collision. Please rebase." } }; + + it("does NOT auto-merge a clean+approved+passing PR when a live migration collision is found", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, ...collided, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); + expect(plan).not.toContain("merge"); + }); + + it("labels the PR gittensory:migration-collision (NOT needs-human-review or ready-to-merge) with the live-collision reason", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto", merge: "auto" }, ...collided, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); + const label = plan.find((a) => a.actionClass === "label"); + expect(label?.label).toBe(AGENT_LABEL_MIGRATION_COLLISION); + expect(label?.label).not.toBe(AGENT_LABEL_NEEDS_REVIEW); + expect(label?.label).not.toBe(AGENT_LABEL_READY); + expect(label?.reason).toContain("live migrations/** collision"); + expect(classes(plan)).not.toContain("merge"); + }); + + it("attaches the rebase-needed comment to the migration-collision label action", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto" }, ...collided, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); + const label = plan.find((a) => a.actionClass === "label"); + expect(label?.comment).toContain("Please rebase"); + }); + + it("does not attach a comment for the ordinary guardrail hold (only migration-collision carries one)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto" }, changedPaths: ["src/scoring/model.ts"], hardGuardrailGlobs: ["src/scoring/**"], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); + const label = plan.find((a) => a.actionClass === "label"); + expect(label?.label).toBe(AGENT_LABEL_NEEDS_REVIEW); + expect(label?.comment).toBeUndefined(); + }); + + it("does not re-plan the migration-collision label when the PR already carries it (idempotent)", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto" }, ...collided, pr: { labels: [AGENT_LABEL_MIGRATION_COLLISION] } }))); + expect(plan).not.toContain("label"); + }); + + it("a BLOCKING PR keeps the changes-requested label even with a migration collision present (blocker wins)", () => { + const label = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { label: "auto" }, blockerTitles: ["x"], ...collided, pr: { labels: [] } })).find((a) => a.actionClass === "label"); + expect(label?.label).toBe(AGENT_LABEL_CHANGES); + expect(label?.comment).toBeUndefined(); + }); + + it("takes priority over a plain guardrail hold when both are true simultaneously — distinct label, not the generic one", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto" }, changedPaths: ["src/scoring/model.ts"], hardGuardrailGlobs: ["src/scoring/**"], ...collided, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); + const label = plan.find((a) => a.actionClass === "label"); + expect(label?.label).toBe(AGENT_LABEL_MIGRATION_COLLISION); + }); + + it("still auto-merges when no migration collision is present (absent input, byte-identical to today)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto", merge: "auto" }, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); + expect(classes(plan)).toContain("merge"); + expect(plan.find((a) => a.actionClass === "label")?.label).toBe(AGENT_LABEL_READY); + }); + }); + describe("submission volume is NOT a manual-hold reason — only guardrail paths hold (#minimize-manual)", () => { it("a high-volume author's clean+green+approved PR MERGES (the quality gate, not a submission count, is the defense)", () => { const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto", approve: "auto", close: "auto", label: "auto" }, pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); diff --git a/test/unit/check-migrations-script.test.ts b/test/unit/check-migrations-script.test.ts index eb345ddcce..0946b7f98c 100644 --- a/test/unit/check-migrations-script.test.ts +++ b/test/unit/check-migrations-script.test.ts @@ -9,6 +9,12 @@ afterEach(() => { for (const dir of tmpDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); +// #2550: the script imports src/db/migration-collisions.ts (a .ts module), so it must be run via `tsx` (the +// same binary package.json's db:migrations:check uses) rather than plain `node` — a bare `node.execPath` +// invocation can't resolve the .ts import without an experimental flag CI's pinned Node isn't guaranteed to +// support. +const TSX_BIN = join(process.cwd(), "node_modules", ".bin", "tsx"); + // Run scripts/check-migrations.mjs over a throwaway fixture dir (via CHECK_MIGRATIONS_DIR) and normalize the // pass/fail into { status, out }. On a non-zero exit execFileSync throws; the violation text is on stderr. function runCheck(files: Record): { status: number; out: string } { @@ -16,7 +22,7 @@ function runCheck(files: Record): { status: number; out: string tmpDirs.push(dir); for (const [name, body] of Object.entries(files)) writeFileSync(join(dir, name), body); try { - const stdout = execFileSync(process.execPath, ["scripts/check-migrations.mjs"], { + const stdout = execFileSync(TSX_BIN, ["scripts/check-migrations.mjs"], { encoding: "utf8", env: { ...process.env, CHECK_MIGRATIONS_DIR: dir }, }); @@ -29,7 +35,7 @@ function runCheck(files: Record): { status: number; out: string describe("check-migrations script", () => { it("reports every grandfathered duplicate migration number in the success summary", () => { - const output = execFileSync(process.execPath, ["scripts/check-migrations.mjs"], { encoding: "utf8" }); + const output = execFileSync(TSX_BIN, ["scripts/check-migrations.mjs"], { encoding: "utf8" }); expect(output).toContain("(4 grandfathered duplicates: 0015, 0017, 0074, 0090)"); }); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index b8f8c757aa..61d81bbd50 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -479,7 +479,7 @@ describe("compileFocusManifestPolicy", () => { issueDiscoveryPolicy: "neutral", maintainerNotes: [], publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], - gate: { present: false, enabled: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null }, + gate: { present: false, enabled: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, @@ -787,7 +787,7 @@ describe("parseFocusManifest gate config", () => { // the block→advisory deprecation-downgrade behavior itself is covered separately below. const m = parseFocusManifest({ gate: { linkedIssue: "block", duplicates: "advisory", readiness: { mode: "advisory", minScore: 70 } } }); expect(m.present).toBe(true); - expect(m.gate).toEqual({ present: true, enabled: null, pack: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null }); + expect(m.gate).toEqual({ present: true, enabled: null, pack: null, linkedIssue: "block", duplicates: "advisory", readinessMode: "advisory", readinessMinScore: 70, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null }); }); it("parses gate.mergeReadiness + gate.firstTimeContributorGrace, round-trips them, and warns on bad values (#822)", () => { @@ -1783,3 +1783,27 @@ describe("gate.dryRun dry-run disposition config (#gate-dryrun)", () => { expect(gateConfigToJson(m.gate)).toMatchObject({ dryRun: true }); }); }); + +describe("gate.premergeContentRecheck live migration-collision recheck config (#2550)", () => { + it("parses gate.premergeContentRecheck, sets present, round-trips, and resolves into effective settings", () => { + const m = parseFocusManifest({ gate: { premergeContentRecheck: true } }); + expect(m.gate.premergeContentRecheck).toBe(true); + expect(m.gate.present).toBe(true); + expect(gateConfigToJson(m.gate)).toMatchObject({ premergeContentRecheck: true }); + const eff = resolveEffectiveSettings({} as unknown as RepositorySettings, m); + expect(eff.premergeContentRecheck).toBe(true); + }); + + it("defaults to unset/undefined when omitted — byte-identical to today", () => { + const m = parseFocusManifest({}); + expect(m.gate.premergeContentRecheck).toBeNull(); + const eff = resolveEffectiveSettings({} as unknown as RepositorySettings, m); + expect(eff.premergeContentRecheck).toBeUndefined(); + }); + + it("warns and drops an invalid (non-boolean) value rather than silently coercing it", () => { + const m = parseFocusManifest({ gate: { premergeContentRecheck: "yes" as never } }); + expect(m.gate.premergeContentRecheck).toBeNull(); + expect(m.warnings.some((w) => /gate\.premergeContentRecheck/i.test(w))).toBe(true); + }); +}); diff --git a/test/unit/migration-collisions.test.ts b/test/unit/migration-collisions.test.ts new file mode 100644 index 0000000000..f45dd52c08 --- /dev/null +++ b/test/unit/migration-collisions.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vitest"; +import { detectMigrationCollisions, extractMigrationNumber, KNOWN_MIGRATION_DUPLICATES, MIGRATION_FILENAME_PATTERN } from "../../src/db/migration-collisions"; + +describe("extractMigrationNumber (#2550)", () => { + it("extracts the 4-digit number from a conforming filename", () => { + expect(extractMigrationNumber("0001_initial.sql")).toBe(1); + expect(extractMigrationNumber("0090_contributor_cap_label.sql")).toBe(90); + expect(extractMigrationNumber("1234_some_migration.sql")).toBe(1234); + }); + + it("returns null for a non-conforming filename", () => { + expect(extractMigrationNumber("not_a_migration.sql")).toBeNull(); + expect(extractMigrationNumber("001_too_few_digits.sql")).toBeNull(); + expect(extractMigrationNumber("0001.sql")).toBeNull(); + expect(extractMigrationNumber("0001_UPPER.sql")).toBeNull(); + }); + + it("MIGRATION_FILENAME_PATTERN matches the same conforming/non-conforming shapes", () => { + expect(MIGRATION_FILENAME_PATTERN.test("0001_initial.sql")).toBe(true); + expect(MIGRATION_FILENAME_PATTERN.test("not_a_migration.sql")).toBe(false); + }); +}); + +describe("detectMigrationCollisions (#2550)", () => { + it("returns [] for a filename list with no duplicate numbers", () => { + expect(detectMigrationCollisions(["0001_a.sql", "0002_b.sql", "0003_c.sql"])).toEqual([]); + }); + + it("returns [] for an empty list", () => { + expect(detectMigrationCollisions([])).toEqual([]); + }); + + it("flags a genuine collision (two files, same number, no grandfather list)", () => { + const collisions = detectMigrationCollisions(["0090_a.sql", "0090_b.sql"]); + expect(collisions).toEqual([{ number: 90, paddedNumber: "0090", files: ["0090_a.sql", "0090_b.sql"] }]); + }); + + it("sorts a collision's files alphabetically regardless of input order", () => { + const collisions = detectMigrationCollisions(["0090_z.sql", "0090_a.sql"]); + expect(collisions[0]?.files).toEqual(["0090_a.sql", "0090_z.sql"]); + }); + + it("sorts multiple simultaneous collisions numerically", () => { + const collisions = detectMigrationCollisions(["0090_a.sql", "0090_b.sql", "0010_a.sql", "0010_b.sql"]); + expect(collisions.map((c) => c.number)).toEqual([10, 90]); + }); + + it("ignores non-conforming filenames entirely (no crash, no false collision)", () => { + expect(detectMigrationCollisions(["README.md", "not_a_migration.sql", "0001_a.sql"])).toEqual([]); + }); + + it("does not flag an EXACT grandfathered set as a collision", () => { + const known = new Map([[90, new Set(["0090_a.sql", "0090_b.sql"])]]); + expect(detectMigrationCollisions(["0090_a.sql", "0090_b.sql"], known)).toEqual([]); + }); + + it("STILL flags a collision when a third file joins an already-grandfathered number", () => { + const known = new Map([[90, new Set(["0090_a.sql", "0090_b.sql"])]]); + const collisions = detectMigrationCollisions(["0090_a.sql", "0090_b.sql", "0090_c.sql"], known); + expect(collisions).toEqual([{ number: 90, paddedNumber: "0090", files: ["0090_a.sql", "0090_b.sql", "0090_c.sql"] }]); + }); + + it("STILL flags a collision when the grandfathered set doesn't exactly match (substitution)", () => { + const known = new Map([[90, new Set(["0090_a.sql", "0090_b.sql"])]]); + const collisions = detectMigrationCollisions(["0090_a.sql", "0090_c.sql"], known); + expect(collisions).toEqual([{ number: 90, paddedNumber: "0090", files: ["0090_a.sql", "0090_c.sql"] }]); + }); + + it("defaults knownDuplicates to an empty map when omitted", () => { + expect(detectMigrationCollisions(["0090_a.sql", "0090_b.sql"])).toHaveLength(1); + }); +}); + +describe("KNOWN_MIGRATION_DUPLICATES (#2550)", () => { + it("stays byte-identical to scripts/check-migrations.mjs's grandfathered list", () => { + // A drift here would mean the CI script and the live premerge recheck disagree about what's grandfathered + // — this pins the exact set so a future addition to one side without the other is caught immediately. + expect([...KNOWN_MIGRATION_DUPLICATES.keys()].sort((a, b) => a - b)).toEqual([15, 17, 74, 90]); + expect(KNOWN_MIGRATION_DUPLICATES.get(90)).toEqual(new Set(["0090_contributor_cap_label.sql", "0090_pull_request_detail_sync_head_sha.sql"])); + }); +}); diff --git a/test/unit/migration-tree.test.ts b/test/unit/migration-tree.test.ts new file mode 100644 index 0000000000..61f7512455 --- /dev/null +++ b/test/unit/migration-tree.test.ts @@ -0,0 +1,63 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { listMigrationFilenamesAtRef } from "../../src/github/migration-tree"; + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("listMigrationFilenamesAtRef (#2550)", () => { + it("returns the .sql filenames directly under migrations/, filtering out non-matching entries", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + expect(input.toString()).toContain("/git/trees/main?recursive=1"); + return Response.json({ + tree: [ + { type: "blob", path: "migrations/0001_initial.sql" }, + { type: "blob", path: "migrations/0002_second.sql" }, + { type: "blob", path: "src/index.ts" }, // outside migrations/ — excluded + { type: "tree", path: "migrations" }, // a directory node itself — excluded (not a blob) + { type: "blob", path: "migrations/nested/0003_nested.sql" }, // nested — excluded (migrations/ is flat) + { type: "blob", path: "migrations/README.md" }, // non-.sql — excluded + ], + }); + }); + + const result = await listMigrationFilenamesAtRef("owner/repo", "main", "token", undefined); + expect(result).toEqual(["0001_initial.sql", "0002_second.sql"]); + }); + + it("returns [] when the tree is empty or the tree field is absent", async () => { + vi.stubGlobal("fetch", async () => Response.json({})); + expect(await listMigrationFilenamesAtRef("owner/repo", "main", "token", undefined)).toEqual([]); + }); + + it("returns null (fail-safe) on a non-OK response", async () => { + vi.stubGlobal("fetch", async () => new Response("not found", { status: 404 })); + expect(await listMigrationFilenamesAtRef("owner/repo", "main", "token", undefined)).toBeNull(); + }); + + it("returns null (fail-safe) when the tree response is truncated — an incomplete list is inconclusive, not evidence of no collision", async () => { + vi.stubGlobal("fetch", async () => Response.json({ truncated: true, tree: [{ type: "blob", path: "migrations/0001_initial.sql" }] })); + expect(await listMigrationFilenamesAtRef("owner/repo", "main", "token", undefined)).toBeNull(); + }); + + it("returns the full list when truncated is explicitly false or absent", async () => { + vi.stubGlobal("fetch", async () => Response.json({ truncated: false, tree: [{ type: "blob", path: "migrations/0001_initial.sql" }] })); + expect(await listMigrationFilenamesAtRef("owner/repo", "main", "token", undefined)).toEqual(["0001_initial.sql"]); + }); + + it("returns null (fail-safe) when the fetch throws", async () => { + vi.stubGlobal("fetch", async () => { + throw new Error("network down"); + }); + expect(await listMigrationFilenamesAtRef("owner/repo", "main", "token", undefined)).toBeNull(); + }); + + it("works without a token (unauthenticated/public read)", async () => { + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + const headers = init?.headers as Record | undefined; + expect(headers?.authorization).toBeUndefined(); + return Response.json({ tree: [{ type: "blob", path: "migrations/0001_initial.sql" }] }); + }); + expect(await listMigrationFilenamesAtRef("owner/repo", "main", undefined, undefined)).toEqual(["0001_initial.sql"]); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index ea4d616b84..9368d78cba 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -5669,6 +5669,461 @@ describe("queue processors", () => { expect(seen.comments.some((c) => c.includes("blocked from contributing"))).toBe(true); }); + describe("live migrations/** collision recheck (#2550)", () => { + // Full merge-eligible stub set (clean + green + approved), reused across scenarios — a positive test proves + // the collision hold actually suppresses what would otherwise merge; a negative test proves the check + // correctly stays out of the way. `liveTree` is the live git/trees response for `main` (the collision + // source of truth); `seen.treeCalls` counts how many times it was fetched, so the "no latency for a + // non-migrations PR" and "off by default" requirements are directly assertable, not just inferred. + function stubMigrationRecheckFetch(prNumber: number, changedFile: { filename: string; status: string } | Array<{ filename: string; status: string }>, liveTree: Array<{ type: string; path: string }> | "error", seen: { closed: boolean; merged: boolean; labels: string[]; comments: string[]; treeCalls: number }) { + const changedFiles = Array.isArray(changedFile) ? changedFile : [changedFile]; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/check-runs/") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes(`/git/trees/main`)) { + seen.treeCalls += 1; + if (liveTree === "error") return new Response("not found", { status: 404 }); + return Response.json({ tree: liveTree }); + } + if (/\/pulls\/\d+(?:\?|$)/.test(url) && method === "GET" && !url.includes(`/pulls/${prNumber}/`)) { + return Response.json({ number: prNumber, state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main", sha: "base" }, mergeable_state: "clean", labels: [] }); + } + if (url.includes(`/pulls/${prNumber}/files`)) return Response.json(changedFiles.map((f) => ({ ...f, additions: 5, deletions: 0, changes: 5, patch: "@@\n+ALTER TABLE t ADD COLUMN c TEXT;" }))); + if (url.includes(`/commits/sha1/check-runs`)) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes(`/commits/sha1/status`)) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] }); + if (url.includes(`/commits/sha1/check-suites`)) return Response.json({ check_suites: [] }); + if (url.includes("/branches/")) return Response.json({ contexts: [] }); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + if (url.includes(`/pulls/${prNumber}/merge`) && method === "PUT") { + seen.merged = true; + return Response.json({ merged: true, sha: "merged-sha1" }); + } + if (url.includes(`/pulls/${prNumber}`) && method === "PATCH") { + seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; + return Response.json({ number: prNumber, state: "closed" }); + } + if (url.includes(`/issues/${prNumber}/labels`) && method === "GET") return Response.json([]); + if (url.includes(`/issues/${prNumber}/labels`) && method === "POST") { + seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); + return Response.json([]); + } + if (url.endsWith("/labels") && method === "POST") return Response.json({ name: "x" }, { status: 201 }); // repo-level label creation (createMissingLabel probe) + if (url.includes(`/issues/${prNumber}/comments`) && method === "POST") { + seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes(`/issues/${prNumber}/comments`)) return Response.json([]); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 901 }, { status: 201 }); + return Response.json({}); + }); + } + + async function seedMigrationRecheckRepo(env: Env, prNumber: number, opts: { premergeContentRecheck?: boolean } = {}) { + await upsertInstallation(env, { + installation: { id: 123, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { pull_requests: "write", issues: "write" }, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto", label: "auto" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + if (opts.premergeContentRecheck !== undefined) { + await upsertRepoFocusManifest(env, "owner/repo", { gate: { premergeContentRecheck: opts.premergeContentRecheck } }); + } + await upsertPullRequestFromGitHub(env, "owner/repo", { number: prNumber, title: "Migration PR", state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main" }, labels: [], body: "" }); + } + + it("holds a would-otherwise-merge PR when the live base has a colliding migration number, with the distinct label + rebase comment", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 60, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch(60, { filename: "migrations/0099_a.sql", status: "added" }, [{ type: "blob", path: "migrations/0099_b.sql" }], seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-collision-hold", repoFullName: "owner/repo", prNumber: 60, installationId: 123 }); + + expect(seen.merged).toBe(false); + expect(seen.closed).toBe(false); // held, never closed — this is a hold, not a close + expect(seen.labels).toContain("gittensory:migration-collision"); + expect(seen.comments.some((c) => c.includes("rebase") && c.includes("0099"))).toBe(true); + expect(seen.treeCalls).toBe(1); + }); + + it("does not hold when the base has no colliding number — merges normally", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 61, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch(61, { filename: "migrations/0099_a.sql", status: "added" }, [{ type: "blob", path: "migrations/0050_unrelated.sql" }], seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-no-collision", repoFullName: "owner/repo", prNumber: 61, installationId: 123 }); + + expect(seen.merged).toBe(true); + expect(seen.labels).not.toContain("gittensory:migration-collision"); + expect(seen.treeCalls).toBe(1); + }); + + it("pays zero latency (never fetches the live tree) for a PR that does not touch migrations/**, even with the recheck enabled", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 62, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch(62, { filename: "src/index.ts", status: "modified" }, [{ type: "blob", path: "migrations/0099_b.sql" }], seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-not-touched", repoFullName: "owner/repo", prNumber: 62, installationId: 123 }); + + expect(seen.treeCalls).toBe(0); // path-gated — never even attempted the live fetch + expect(seen.merged).toBe(true); + }); + + it("is off by default — never fetches the live tree even for a migrations/**-touching PR when unconfigured", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 63); // premergeContentRecheck left unset — defaults off + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch(63, { filename: "migrations/0099_a.sql", status: "added" }, [{ type: "blob", path: "migrations/0099_b.sql" }], seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-recheck-off", repoFullName: "owner/repo", prNumber: 63, installationId: 123 }); + + expect(seen.treeCalls).toBe(0); + expect(seen.merged).toBe(true); // a live collision exists but the feature is off — merges anyway (opt-in) + }); + + it("fails OPEN (merges normally) when the live tree fetch errors — never holds a PR on inconclusive data", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 64, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch(64, { filename: "migrations/0099_a.sql", status: "added" }, "error", seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-fetch-error", repoFullName: "owner/repo", prNumber: 64, installationId: 123 }); + + expect(seen.treeCalls).toBe(1); + expect(seen.merged).toBe(true); + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + + it("fails OPEN (never fetches the live tree) when the PR has no resolvable base ref", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { pull_requests: "write", issues: "write" }, events: [] }, + }); + // No default_branch on the repo record AND no base.ref on the PR record — baseRef resolves to undefined. + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto", label: "auto" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertRepoFocusManifest(env, "owner/repo", { gate: { premergeContentRecheck: true } }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 65, title: "No base ref", state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, labels: [], body: "" }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch(65, { filename: "migrations/0099_a.sql", status: "added" }, [{ type: "blob", path: "migrations/0099_b.sql" }], seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-no-base-ref", repoFullName: "owner/repo", prNumber: 65, installationId: 123 }); + + expect(seen.treeCalls).toBe(0); // no live target to compare against — never even attempted the fetch + expect(seen.merged).toBe(true); + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + + it("REGRESSION: is deliberately UNCACHED — a live tree that changes between two consecutive maintenance passes is picked up fresh, never served stale", async () => { + // The exact race a caching layer would reintroduce: a sibling PR (not modeled directly here — simulated + // by the live tree response CHANGING between the two fetches, the same effect a sibling merge has) adds + // a colliding migration file in the window between two maintenance passes on the SAME PR. A cache keyed + // by repo+baseRef would serve the first (pre-collision) snapshot on the second pass and miss the + // collision entirely — this asserts both passes fetch fresh and the second one correctly detects it. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 66, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + let liveTree: Array<{ type: string; path: string }> = []; // pass 1: main has nothing colliding yet + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/check-runs/") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes("/git/trees/main")) { + seen.treeCalls += 1; + return Response.json({ tree: liveTree }); + } + if (/\/pulls\/\d+(?:\?|$)/.test(url) && method === "GET" && !url.includes("/pulls/66/")) { + return Response.json({ number: 66, state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main", sha: "base" }, mergeable_state: "clean", labels: [] }); + } + if (url.includes("/pulls/66/files")) return Response.json([{ filename: "migrations/0099_a.sql", status: "added", additions: 5, deletions: 0, changes: 5, patch: "@@\n+ALTER TABLE t ADD COLUMN c TEXT;" }]); + if (url.includes("/commits/sha1/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/sha1/status")) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] }); + if (url.includes("/commits/sha1/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/branches/")) return Response.json({ contexts: [] }); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + if (url.includes("/pulls/66/merge") && method === "PUT") { + seen.merged = true; + return Response.json({ merged: true, sha: "merged-sha1" }); + } + if (url.includes("/issues/66/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/66/labels") && method === "POST") { + seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); + return Response.json([]); + } + if (url.endsWith("/labels") && method === "POST") return Response.json({ name: "x" }, { status: 201 }); + if (url.includes("/issues/66/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-fresh-pass-1", repoFullName: "owner/repo", prNumber: 66, installationId: 123 }); + expect(seen.merged).toBe(true); // pass 1: no collision yet — merges + + // Between passes, a sibling PR merges its own colliding 0099 file — main's live tree now has it. + liveTree = [{ type: "blob", path: "migrations/0099_b.sql" }]; + seen.merged = false; + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-fresh-pass-2", repoFullName: "owner/repo", prNumber: 66, installationId: 123 }); + + expect(seen.treeCalls).toBe(2); // every pass fetches fresh — no cache could ever mask the change + expect(seen.labels).toContain("gittensory:migration-collision"); + expect(seen.merged).toBe(false); // pass 2 correctly catches the now-live collision, never stale-served + }); + + it("does NOT hold for a pre-existing collision between two OTHER files unrelated to this PR's own migration number", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 67, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + // main already has a real collision at 0050 (two unrelated, already-merged files) — nothing to do with + // this PR's own migration at 0099. The prNumbers scoping must exclude it: main is already broken by + // someone else's mistake, but that must not hold an unrelated third PR. + stubMigrationRecheckFetch(67, { filename: "migrations/0099_a.sql", status: "added" }, [ + { type: "blob", path: "migrations/0050_x.sql" }, + { type: "blob", path: "migrations/0050_y.sql" }, + ], seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-unrelated-collision", repoFullName: "owner/repo", prNumber: 67, installationId: 123 }); + + expect(seen.merged).toBe(true); + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + + it("holds and reports every colliding number when a PR touches two migration files that each independently collide", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 68, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + stubMigrationRecheckFetch( + 68, + [ + { filename: "migrations/0098_a.sql", status: "added" }, + { filename: "migrations/0099_a.sql", status: "added" }, + ], + [ + { type: "blob", path: "migrations/0098_b.sql" }, + { type: "blob", path: "migrations/0099_b.sql" }, + ], + seen, + ); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-multi-collision", repoFullName: "owner/repo", prNumber: 68, installationId: 123 }); + + expect(seen.merged).toBe(false); + expect(seen.labels).toContain("gittensory:migration-collision"); + expect(seen.comments.some((c) => c.includes("0098") && c.includes("0099"))).toBe(true); + }); + + it("does not hold when the live base already contains one of the real grandfathered duplicate pairs, unrelated to this PR's own migration number", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 69, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + // The real, already-shipped 0090 grandfathered pair (see KNOWN_MIGRATION_DUPLICATES) is present on main — + // this must never trigger a hold for an unrelated PR touching a different number. + stubMigrationRecheckFetch(69, { filename: "migrations/0099_a.sql", status: "added" }, [ + { type: "blob", path: "migrations/0090_contributor_cap_label.sql" }, + { type: "blob", path: "migrations/0090_pull_request_detail_sync_head_sha.sql" }, + ], seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-grandfathered", repoFullName: "owner/repo", prNumber: 69, installationId: 123 }); + + expect(seen.merged).toBe(true); + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + + it("REGRESSION: renaming this PR's own not-yet-merged migration file (e.g. a typo fix, same number) does NOT self-collide", async () => { + // Before the fix, prMigrationFilenames was derived from changedPathsForGuardrail's collapsed set, which + // includes BOTH a renamed file's old and new name — counting one logical file as two and self-colliding. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 70, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/check-runs/") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes("/git/trees/main")) { + seen.treeCalls += 1; + return Response.json({ tree: [] }); // empty live base — nothing else to collide with + } + if (/\/pulls\/\d+(?:\?|$)/.test(url) && method === "GET" && !url.includes("/pulls/70/")) { + return Response.json({ number: 70, state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main", sha: "base" }, mergeable_state: "clean", labels: [] }); + } + // A single GitHub PR-files entry for a rename: status="renamed", filename=new name, previous_filename=old name. + if (url.includes("/pulls/70/files")) return Response.json([{ filename: "migrations/0099_add_column.sql", previous_filename: "migrations/0099_add_colum.sql", status: "renamed", additions: 1, deletions: 1, changes: 2, patch: "@@\n rename" }]); + if (url.includes("/commits/sha1/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/sha1/status")) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] }); + if (url.includes("/commits/sha1/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/branches/")) return Response.json({ contexts: [] }); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + if (url.includes("/pulls/70/merge") && method === "PUT") { + seen.merged = true; + return Response.json({ merged: true, sha: "merged-sha1" }); + } + if (url.includes("/issues/70/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/70/labels") && method === "POST") { + seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); + return Response.json([]); + } + if (url.endsWith("/labels") && method === "POST") return Response.json({ name: "x" }, { status: 201 }); + if (url.includes("/issues/70/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-rename-self", repoFullName: "owner/repo", prNumber: 70, installationId: 123 }); + + expect(seen.merged).toBe(true); // no false self-collision from counting the old+new rename names as two files + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + + it("REGRESSION: renaming an EXISTING base migration (same number) does NOT self-collide with its own old name still live on main", async () => { + // Before the fix, liveFilenames (fetched from main, which still has the pre-rename name until this PR + // merges) was unioned as-is with prMigrationFilenames (the new name only) — so a same-number typo-fix + // rename of an ALREADY-MERGED base migration counted as two distinct files at one number and + // self-collided, even though the merged tree would only ever contain the renamed file. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 73, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/check-runs/") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes("/git/trees/main")) { + seen.treeCalls += 1; + // main still has the PRE-rename name — this PR's rename hasn't merged yet. + return Response.json({ tree: [{ type: "blob", path: "migrations/0099_old.sql" }] }); + } + if (/\/pulls\/\d+(?:\?|$)/.test(url) && method === "GET" && !url.includes("/pulls/73/")) { + return Response.json({ number: 73, state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main", sha: "base" }, mergeable_state: "clean", labels: [] }); + } + // Renames an EXISTING base migration (same number 0099), not a file this PR itself added. + 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" }]); + if (url.includes("/commits/sha1/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/sha1/status")) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] }); + if (url.includes("/commits/sha1/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/branches/")) return Response.json({ contexts: [] }); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + if (url.includes("/pulls/73/merge") && method === "PUT") { + seen.merged = true; + return Response.json({ merged: true, sha: "merged-sha1" }); + } + if (url.includes("/issues/73/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/73/labels") && method === "POST") { + seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); + return Response.json([]); + } + if (url.endsWith("/labels") && method === "POST") return Response.json({ name: "x" }, { status: 201 }); + if (url.includes("/issues/73/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-rename-existing-base", repoFullName: "owner/repo", prNumber: 73, installationId: 123 }); + + expect(seen.merged).toBe(true); // the pre-rename name still live on main must not count against this PR + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + + it("REGRESSION: renumbering (renaming) this PR's migration to resolve a real collision does not leave a stale hold from the old filename", async () => { + // Before the fix, the stale previousFilename (the OLD number) stayed in prMigrationFilenames forever, + // colliding with an unrelated already-merged file at that old number and permanently re-holding a PR + // that had already fixed itself — exactly the remediation this feature's own comment recommends. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 71, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/check-runs/") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes("/git/trees/main")) { + seen.treeCalls += 1; + // main already has an unrelated, already-merged file at the OLD number (0099) — nothing to do with + // this PR anymore, since it renumbered away from 0099 to 0100. + return Response.json({ tree: [{ type: "blob", path: "migrations/0099_other_already_merged.sql" }] }); + } + if (/\/pulls\/\d+(?:\?|$)/.test(url) && method === "GET" && !url.includes("/pulls/71/")) { + return Response.json({ number: 71, state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main", sha: "base" }, mergeable_state: "clean", labels: [] }); + } + if (url.includes("/pulls/71/files")) return Response.json([{ filename: "migrations/0100_mine.sql", previous_filename: "migrations/0099_mine.sql", status: "renamed", additions: 1, deletions: 1, changes: 2, patch: "@@\n rename" }]); + if (url.includes("/commits/sha1/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/sha1/status")) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] }); + if (url.includes("/commits/sha1/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/branches/")) return Response.json({ contexts: [] }); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + if (url.includes("/pulls/71/merge") && method === "PUT") { + seen.merged = true; + return Response.json({ merged: true, sha: "merged-sha1" }); + } + if (url.includes("/issues/71/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/71/labels") && method === "POST") { + seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); + return Response.json([]); + } + if (url.endsWith("/labels") && method === "POST") return Response.json({ name: "x" }, { status: 201 }); + if (url.includes("/issues/71/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-renumber-remediation", repoFullName: "owner/repo", prNumber: 71, installationId: 123 }); + + expect(seen.merged).toBe(true); // the stale old-number previousFilename must not re-trigger a hold + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + + it("REGRESSION: deleting this PR's own colliding migration file does not still count it as one of the PR's own filenames", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedMigrationRecheckRepo(env, 72, { premergeContentRecheck: true }); + const seen = { closed: false, merged: false, labels: [] as string[], comments: [] as string[], treeCalls: 0 }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = (init?.method ?? "GET").toUpperCase(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/check-runs/") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes("/git/trees/main")) { + seen.treeCalls += 1; + return Response.json({ tree: [{ type: "blob", path: "migrations/0099_other_already_merged.sql" }] }); + } + if (/\/pulls\/\d+(?:\?|$)/.test(url) && method === "GET" && !url.includes("/pulls/72/")) { + return Response.json({ number: 72, state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main", sha: "base" }, mergeable_state: "clean", labels: [] }); + } + // The PR deletes its own migration file (status="removed") — it no longer exists in the PR's tree. + if (url.includes("/pulls/72/files")) return Response.json([{ filename: "migrations/0099_mine.sql", status: "removed", additions: 0, deletions: 5, changes: 5, patch: "@@\n-removed" }]); + if (url.includes("/commits/sha1/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/sha1/status")) return Response.json({ state: "success", statuses: [{ context: "ci/build", state: "success", description: "ok" }] }); + if (url.includes("/commits/sha1/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/branches/")) return Response.json({ contexts: [] }); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + if (url.includes("/pulls/72/merge") && method === "PUT") { + seen.merged = true; + return Response.json({ merged: true, sha: "merged-sha1" }); + } + if (url.includes("/issues/72/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/72/labels") && method === "POST") { + seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); + return Response.json([]); + } + if (url.endsWith("/labels") && method === "POST") return Response.json({ name: "x" }, { status: 201 }); + if (url.includes("/issues/72/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "migration-removed-file", repoFullName: "owner/repo", prNumber: 72, installationId: 123 }); + + // With no migrations/**-touching file left in the PR's own set (the only entry is `status: "removed"`), + // prMigrationFilenames is empty — the whole recheck is path-gated off, so it never even fetches the tree. + expect(seen.treeCalls).toBe(0); + expect(seen.merged).toBe(true); + expect(seen.labels).not.toContain("gittensory:migration-collision"); + }); + }); + it("contributor open-PR cap (#2270): a contributor's 3rd open PR (over a cap of 2) is labeled + closed deterministically with no merit merge", async () => { let aiCalls = 0; const env = createTestEnv({