fix(db): address review — DROP INDEX lock symmetry, RENAME CONSTRAINT false-positive, alter-type literal match

- Non-concurrent DROP INDEX is now a hard error (ACCESS EXCLUSIVE lock), symmetric with CREATE INDEX; DROP INDEX CONCURRENTLY after a COMMIT passes clean. Removes the false-confidence annotate path.
- RENAME rule narrowed to RENAME COLUMN / table RENAME TO; RENAME CONSTRAINT and ALTER INDEX ... RENAME (metadata-only) no longer flagged.
- alter-type regex now requires TYPE to follow the column identifier, so it no longer matches TYPE inside a string default.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: TheodoreSpeaks

.agents/skills/db-migrate/SKILL.md

@@ -34,6 +34,7 @@ Never put expand and contract in the same PR. If this PR both removes the code t
 | Change a column type | add a new column of the new type; dual-write | backfill, swap reads, drop old |
 | Add FK / CHECK | `ADD CONSTRAINT ... NOT VALID` | `VALIDATE CONSTRAINT` separately |
 | Index an existing table | `COMMIT;` breakpoint → `SET lock_timeout = 0` → `CREATE INDEX CONCURRENTLY IF NOT EXISTS` (see `packages/db/scripts/migrate.ts`) | — |
+| Drop an index | `COMMIT;` breakpoint → `DROP INDEX CONCURRENTLY` — plain `DROP INDEX` takes ACCESS EXCLUSIVE on the table | — |
 | Backfill data | batched + idempotent `UPDATE` (keyset/`WHERE`, bounded) | — |
 
 A `CREATE INDEX`, `ADD COLUMN`, or `ADD CONSTRAINT` against a table **created in the same migration** is always safe (no rows, no live traffic) — the lint already suppresses those.

scripts/check-migrations-safety.test.ts

@@ -137,6 +137,45 @@ describe('warnings (non-blocking)', () => {
   })
 })
 
+describe('review fixes', () => {
+  test('RENAME CONSTRAINT is metadata-only — not flagged', () => {
+    expect(
+      lintSql('ALTER TABLE "permission_group" RENAME CONSTRAINT "old_fk" TO "new_fk";')
+    ).toEqual([])
+  })
+
+  test('ALTER INDEX ... RENAME is metadata-only — not flagged', () => {
+    expect(lintSql('ALTER INDEX "old_idx" RENAME TO "new_idx";')).toEqual([])
+  })
+
+  test('table RENAME TO is still a hard error', () => {
+    expect(rules('ALTER TABLE "marketplace" RENAME TO "listings";')).toEqual(['error:rename'])
+  })
+
+  test('plain DROP INDEX is a hard error (ACCESS EXCLUSIVE lock)', () => {
+    expect(rules('DROP INDEX "permission_group_workspace_name_unique";')).toEqual([
+      'error:drop-index-not-concurrent',
+    ])
+  })
+
+  test('DROP INDEX CONCURRENTLY after a COMMIT passes clean', () => {
+    const sql = `COMMIT;
+--> statement-breakpoint
+DROP INDEX CONCURRENTLY IF EXISTS "stale_idx";`
+    expect(lintSql(sql)).toEqual([])
+  })
+
+  test('DROP INDEX CONCURRENTLY without a preceding COMMIT errors', () => {
+    expect(rules('DROP INDEX CONCURRENTLY IF EXISTS "stale_idx";')).toEqual([
+      'error:concurrent-drop-index-no-commit',
+    ])
+  })
+
+  test('alter-type does not match TYPE inside a string default', () => {
+    expect(lintSql(`ALTER TABLE "x" ALTER COLUMN "y" SET DEFAULT 'change TYPE later';`)).toEqual([])
+  })
+})
+
 describe('parser robustness', () => {
   test('semicolon inside a string literal does not split', () => {
     expect(lintSql(`ALTER TABLE "x" ADD COLUMN "y" text DEFAULT 'a;b' NOT NULL;`)).toEqual([])

scripts/check-migrations-safety.ts

@@ -243,12 +243,12 @@ function classify(sql: string, createdTables: Set<string>, sawCommit: boolean):
     })
   }
 
-  if (/\bRENAME\b/i.test(s)) {
+  if (/\bRENAME COLUMN\b/i.test(s) || /^ALTER TABLE\b[^;]*\bRENAME TO\b/i.test(s)) {
     matches.push({
       kind: 'error',
       rule: 'rename',
       message:
-        'RENAME breaks old code reading the old name. Add the new column/table, dual-write in code, then drop the old one in a later deploy.',
+        'RENAME of a column/table breaks old code reading the old name. Add the new column/table, dual-write in code, then drop the old one in a later deploy.',
     })
   }
 
@@ -282,12 +282,26 @@ function classify(sql: string, createdTables: Set<string>, sawCommit: boolean):
     if (/\bSET NOT NULL\b/i.test(s)) {
       matches.push({ kind: 'annotate', rule: 'set-not-null', message: 'SET NOT NULL' })
     }
-    if (/\b(?:SET DATA TYPE|ALTER COLUMN [^;]*\bTYPE)\b/i.test(s)) {
+    if (/\bSET DATA TYPE\b/i.test(s) || /\bALTER COLUMN ("?[.\w]+"?) TYPE\b/i.test(s)) {
       matches.push({ kind: 'annotate', rule: 'alter-type', message: 'column type change' })
     }
   }
   if (/^DROP INDEX\b/i.test(s)) {
-    matches.push({ kind: 'annotate', rule: 'drop-index', message: 'DROP INDEX' })
+    if (!/\bCONCURRENTLY\b/i.test(s)) {
+      matches.push({
+        kind: 'error',
+        rule: 'drop-index-not-concurrent',
+        message:
+          'Plain DROP INDEX takes an ACCESS EXCLUSIVE lock on the table for the whole drop. Use DROP INDEX CONCURRENTLY after a COMMIT; breakpoint (see packages/db/scripts/migrate.ts).',
+      })
+    } else if (!sawCommit) {
+      matches.push({
+        kind: 'error',
+        rule: 'concurrent-drop-index-no-commit',
+        message:
+          'DROP INDEX CONCURRENTLY cannot run inside the migration transaction. Precede it with a COMMIT; breakpoint (see packages/db/scripts/migrate.ts).',
+      })
+    }
   }
 
   if (/^(UPDATE|DELETE)\b/i.test(s)) {