fix(db): refuse migrations whose ownership statements only pass outside production

scripts/check-migrations-safety.test.ts

@@ -1,24 +1,40 @@
 import { execFile } from 'node:child_process'
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
 import path from 'node:path'
 import { promisify } from 'node:util'
-import { describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it } from 'vitest'
 
 const execFileAsync = promisify(execFile)
 const ROOT = path.resolve(import.meta.dirname, '..')
 const SCRIPT = path.join(ROOT, 'scripts/check-migrations-safety.ts')
 
 async function runAudit(
-  baseRef: string
+  ...args: string[]
 ): Promise<{ code: number; stdout: string; stderr: string }> {
   try {
-    const { stdout, stderr } = await execFileAsync('bun', ['run', SCRIPT, baseRef], { cwd: ROOT })
+    const { stdout, stderr } = await execFileAsync('bun', ['run', SCRIPT, ...args], { cwd: ROOT })
     return { code: 0, stdout, stderr }
   } catch (error) {
     const failure = error as { code?: number; stdout?: string; stderr?: string }
     return { code: failure.code ?? 1, stdout: failure.stdout ?? '', stderr: failure.stderr ?? '' }
   }
 }
 
+const tempDirs: string[] = []
+
+/** A scratch migrations directory holding one file. */
+function migrationsDir(sql: string): string {
+  const dir = mkdtempSync(path.join(tmpdir(), 'migration-owner-'))
+  tempDirs.push(dir)
+  writeFileSync(path.join(dir, '0001_seq.sql'), sql)
+  return dir
+}
+
+afterEach(() => {
+  for (const dir of tempDirs.splice(0)) rmSync(dir, { recursive: true, force: true })
+})
+
 describe('migration safety audit', () => {
   /**
    * The regression this guards: an unresolvable base ref made `git diff` fail, the
@@ -34,6 +50,116 @@ describe('migration safety audit', () => {
     expect(stderr).not.toContain('No new migrations to check')
   }, 30_000)
 
+  /**
+   * Production keeps tables owned by an older role than the one that runs migrations, so linking a
+   * new sequence to an existing table passed CI and staging and failed only the production
+   * migration. Linking it to a table the same migration creates stays safe.
+   */
+  it('refuses ownership-dependent statements on existing tables', async () => {
+    const dir = migrationsDir(
+      [
+        'CREATE SEQUENCE IF NOT EXISTS "public"."calls_seq";--> statement-breakpoint',
+        'ALTER SEQUENCE "public"."calls_seq" OWNED BY "calls"."position";--> statement-breakpoint',
+        'ALTER TABLE "calls" OWNER TO "postgres";',
+      ].join('\n')
+    )
+    const { code, stdout, stderr } = await runAudit('--dir', dir)
+
+    expect(code).toBe(1)
+    expect(`${stdout}${stderr}`).toContain('sequence-owned-by-existing-table')
+    expect(`${stdout}${stderr}`).toContain('owner-to')
+  }, 30_000)
+
+  it.each(['"jobs"', '"public"."jobs"'])(
+    'allows a sequence owned by a table created in the same migration as %s',
+    async (table) => {
+      const dir = migrationsDir(
+        [
+          `CREATE TABLE ${table} ("id" text PRIMARY KEY, "position" bigint);--> statement-breakpoint`,
+          'CREATE SEQUENCE IF NOT EXISTS "public"."jobs_seq";--> statement-breakpoint',
+          'ALTER SEQUENCE "public"."jobs_seq" OWNED BY "jobs"."position";',
+        ].join('\n')
+      )
+      const { code, stdout, stderr } = await runAudit('--dir', dir)
+
+      expect(`${stdout}${stderr}`).not.toContain('sequence-owned-by-existing-table')
+      expect(code).toBe(0)
+    },
+    30_000
+  )
+
+  it('matches a schema-qualified OWNED BY to an unqualified create in public', async () => {
+    const dir = migrationsDir(
+      [
+        'CREATE TABLE "jobs" ("id" text PRIMARY KEY, "position" bigint);--> statement-breakpoint',
+        'CREATE SEQUENCE IF NOT EXISTS "public"."jobs_seq";--> statement-breakpoint',
+        'ALTER SEQUENCE "public"."jobs_seq" OWNED BY "public"."jobs"."position";',
+      ].join('\n')
+    )
+    const { code, stdout, stderr } = await runAudit('--dir', dir)
+
+    expect(`${stdout}${stderr}`).not.toContain('sequence-owned-by-existing-table')
+    expect(code).toBe(0)
+  }, 30_000)
+
+  it.each([
+    [
+      'a conditional create, which may find the table already there',
+      'CREATE TABLE IF NOT EXISTS "jobs" ("id" text PRIMARY KEY, "position" bigint);--> statement-breakpoint\nALTER SEQUENCE "jobs_seq" OWNED BY "jobs"."position";',
+    ],
+    [
+      'a same-named table in another schema',
+      'CREATE TABLE "archive"."jobs" ("id" text PRIMARY KEY, "position" bigint);--> statement-breakpoint\nALTER SEQUENCE "jobs_seq" OWNED BY "public"."jobs"."position";',
+    ],
+    [
+      'an ownership change wrapped in a DO block',
+      'DO $$ BEGIN\n  ALTER SEQUENCE "jobs_seq" OWNED BY "jobs"."position";\nEXCEPTION WHEN undefined_table THEN NULL;\nEND $$;',
+    ],
+  ])(
+    'refuses OWNED BY through %s',
+    async (_case, sql) => {
+      const { code, stdout, stderr } = await runAudit('--dir', migrationsDir(sql))
+
+      expect(code).toBe(1)
+      expect(`${stdout}${stderr}`).toContain('sequence-owned-by-existing-table')
+    },
+    30_000
+  )
+
+  it('reads past an escaped quote in an escape string to an ownership change after it', async () => {
+    const dir = migrationsDir(
+      "DO $$ BEGIN RAISE NOTICE E'it\\'s ready'; ALTER TABLE calls OWNER TO postgres; RAISE NOTICE 'done'; END $$;"
+    )
+    const { code, stdout, stderr } = await runAudit('--dir', dir)
+
+    expect(code).toBe(1)
+    expect(`${stdout}${stderr}`).toContain('owner-to')
+  }, 30_000)
+
+  it('ignores ownership words inside an escape string', async () => {
+    const dir = migrationsDir(
+      "COMMENT ON TABLE \"jobs\" IS E'Ops\\'s note: hand owner to the support team';"
+    )
+    const { code, stdout, stderr } = await runAudit('--dir', dir)
+
+    expect(`${stdout}${stderr}`).not.toContain('owner-to')
+    expect(code).toBe(0)
+  }, 30_000)
+
+  it('ignores ownership words inside string literals and comments', async () => {
+    const dir = migrationsDir(
+      [
+        '-- Hands owner to the support team in a later release.',
+        'COMMENT ON TABLE "jobs" IS \'Transfer owner to the support team, OWNED BY ops.jobs.id\';',
+      ].join('\n')
+    )
+    const { code, stdout, stderr } = await runAudit('--dir', dir)
+
+    expect(`${stdout}${stderr}`).not.toContain('owner-to')
+    expect(`${stdout}${stderr}`).not.toContain('sequence-owned-by-existing-table')
+    expect(code).toBe(0)
+  }, 30_000)
+
   it('passes against a real base ref with no new migrations', async () => {
     const { code, stdout } = await runAudit('HEAD')
 

scripts/check-migrations-safety.ts

@@ -48,6 +48,55 @@ interface Statement {
   startLine: number
 }
 
+/** `schema.table` with quotes stripped; an unqualified name is in `public`. */
+function qualifiedName(raw: string): string {
+  const parts = raw.replace(/"/g, '').toLowerCase().split('.')
+  return parts.length > 1 ? parts.slice(-2).join('.') : `public.${parts[0]}`
+}
+
+/**
+ * The statement with string literals and comments blanked, so only executable SQL is matched.
+ * An escape string (`E'…'`) is read with its backslash escapes, so `\'` does not end it early.
+ * Dollar-quoted bodies stay: a `DO` block runs them.
+ */
+function executableSql(sql: string): string {
+  return sql.replace(
+    /(?<![\w$])[Ee]'(?:[^'\\]|\\[\s\S]|'')*'|'(?:[^']|'')*'|--[^\n]*|\/\*[\s\S]*?\*\//g,
+    ' '
+  )
+}
+
+/**
+ * Statements that depend on who owns a table. Production keeps older tables owned by a
+ * different role than the one that runs migrations, while every table in CI and staging belongs
+ * to the migrator, so these pass everywhere but production. `newTables` holds the tables this
+ * migration certainly creates (`CREATE TABLE` without `IF NOT EXISTS`), which the migrator owns.
+ */
+function ownershipMatches(sql: string, newTables: Set<string>): RawMatch[] {
+  const s = executableSql(sql).replace(/\s+/g, ' ')
+  const matches: RawMatch[] = []
+  for (const ownedBy of s.matchAll(/\b(?:CREATE|ALTER) SEQUENCE\b[^;]*?\bOWNED BY ([^\s;]+)/gi)) {
+    if (/^NONE$/i.test(ownedBy[1])) continue
+    const column = ownedBy[1].replace(/"/g, '').split('.')
+    if (newTables.has(qualifiedName(column.slice(0, -1).join('.')))) continue
+    matches.push({
+      kind: 'error',
+      rule: 'sequence-owned-by-existing-table',
+      message:
+        'OWNED BY needs the sequence and its table to share an owner, and an existing table may belong to a different role than the one that created the sequence. Leave the sequence unowned, or link it only to a table this migration creates.',
+    })
+  }
+  if (/\bOWNER TO\b/i.test(s)) {
+    matches.push({
+      kind: 'error',
+      rule: 'owner-to',
+      message:
+        'Owner roles differ between environments, so OWNER TO is not portable. Ownership changes belong to an operator, not a migration.',
+    })
+  }
+  return matches
+}
+
 /** Strip quotes and any schema prefix so `"public"."user"` and `"user"` match. */
 function bareName(raw: string): string {
   const unquoted = raw.replace(/"/g, '')
@@ -334,15 +383,21 @@ function lintSql(content: string): Finding[] {
   const lines = content.split('\n')
   const statements = parseStatements(content)
   const createdTables = new Set<string>()
+  const newTables = new Set<string>()
   for (const { sql } of statements) {
-    const m = sql.match(/^CREATE TABLE (?:IF NOT EXISTS )?("?[.\w]+"?)/i)
-    if (m) createdTables.add(bareName(m[1]))
+    const m = sql.match(/^CREATE TABLE (IF NOT EXISTS )?((?:"[^"]+"|\w+)(?:\.(?:"[^"]+"|\w+))?)/i)
+    if (!m) continue
+    createdTables.add(bareName(m[2]))
+    if (!m[1]) newTables.add(qualifiedName(m[2]))
   }
 
   const findings: Finding[] = []
   let sawCommit = false
   for (const { sql, startLine } of statements) {
-    for (const match of classify(sql, createdTables, sawCommit)) {
+    for (const match of [
+      ...classify(sql, createdTables, sawCommit),
+      ...ownershipMatches(sql, newTables),
+    ]) {
       if (match.kind === 'error') {
         findings.push({
           line: startLine,