From 7a3ba6aa3066f4dab35baff78e9b2c57af36c374 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Sat, 13 Jun 2026 18:56:36 -0700 Subject: [PATCH 1/6] feat(db): zero-downtime migration safety lint + db-migrate skill Add scripts/check-migrations-safety.ts (check:migrations), a CI gate that classifies statements in newly-added migrations into hard errors (rewrite), annotate-to-acknowledge contract ops (`-- migration-safe: `), and backfill warnings. Wire it into test-build.yml. Add the /db-migrate skill as the judgment half (expand/contract phasing, app-code cross-ref, annotation authoring). Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/db-migrate/SKILL.md | 66 ++++ .github/workflows/test-build.yml | 10 + package.json | 1 + scripts/check-migrations-safety.test.ts | 154 ++++++++ scripts/check-migrations-safety.ts | 470 ++++++++++++++++++++++++ 5 files changed, 701 insertions(+) create mode 100644 .agents/skills/db-migrate/SKILL.md create mode 100644 scripts/check-migrations-safety.test.ts create mode 100644 scripts/check-migrations-safety.ts diff --git a/.agents/skills/db-migrate/SKILL.md b/.agents/skills/db-migrate/SKILL.md new file mode 100644 index 00000000000..3903eaaf32c --- /dev/null +++ b/.agents/skills/db-migrate/SKILL.md @@ -0,0 +1,66 @@ +--- +name: db-migrate +description: Author or review a Drizzle DB migration for zero-downtime safety — expand/contract phasing, backward-compatibility with the deployed app version, and writing the `-- migration-safe` acknowledgment the check:migrations lint requires. Use when adding/editing files under `packages/db/migrations/` or changing `packages/db/schema.ts`. +--- + +# DB Migrate Skill + +You make schema changes that survive a deploy without downtime. The `check:migrations` lint (`scripts/check-migrations-safety.ts`) is the deterministic gate; you are the judgment that decides whether a flagged change is actually safe and writes the annotation that satisfies it. + +## The window (why this matters) + +A deploy runs the migration, then rolls out the new app image via blue/green. The two are **not atomic and cannot be** — during cutover the old task set keeps serving against the **already-migrated** schema. So: + +> Every migration must be backward-compatible with the app version that is *already deployed*. + +If a migration drops a column the old code still reads, renames one, or adds a `NOT NULL` the old inserts don't populate, the old code throws until traffic fully shifts — the downtime we're guarding against. You can't fix this by reordering the pipeline; the only fix is discipline. + +## Expand / contract + +Split every breaking change across **two deploys**: + +1. **Expand** (this PR): additive, backward-compatible schema + code that tolerates *both* the old and new shape. +2. **Contract** (a later PR, after expand is fully deployed): remove the old thing, now that nothing reads it. + +Never put expand and contract in the same PR. If this PR both removes the code that used a column *and* drops the column, the old code is still live during cutover — split it. + +### Per-operation playbook + +| You want to | Do (deploy 1 = expand) | Do (deploy 2 = contract) | +|---|---|---| +| Add a required column | `ADD COLUMN` nullable or `DEFAULT`; code writes it | backfill, then `SET NOT NULL` | +| Rename a column/table | add the new name; code dual-writes / reads new-then-old | drop the old name | +| Drop a column/table | stop all reads/writes in code; ship it | `DROP` (annotate) | +| 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`) | — | +| 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. + +## The judgment the lint can't do + +The lint flags risky *shapes*; it cannot know whether a given drop is *safe right now*. For each flagged statement, do the work it can't: + +1. **Is the dependency gone?** Grep the app for the table/column: search `apps/sim` and `packages` for the column name, the Drizzle field (camelCase), and the table object. If any live read/write remains, it is **not** safe — fix the code first. +2. **Did the expand already ship?** The removal of that read/write must be in a deploy that is *already out*, not this same PR. If it's in this PR, split: land the code change now, do the destructive migration in a follow-up after it deploys. +3. **Backfills:** confirm the `UPDATE`/`DELETE` is batched (bounded `WHERE`/keyset, not a single whole-table statement), idempotent (safe to replay — a failed migration re-runs unjournaled files from the top), and safe under concurrent writes from the still-live old app. + +## Workflow + +1. Edit `packages/db/schema.ts`, then `cd packages/db && bunx drizzle-kit generate` to produce the SQL. +2. Hand-edit the generated SQL where the playbook requires it: `CONCURRENTLY` + `COMMIT;` breakpoint for indexes on existing tables, `NOT VALID` for constraints, batching for backfills. +3. Run `bun run check:migrations` (or `bun run scripts/check-migrations-safety.ts main` locally). + - **Hard errors** (`add-not-null-no-default`, `rename`, `index-not-concurrent`, `constraint-not-valid`, …): rewrite into expand/contract. Do **not** try to annotate them away — the lint won't accept it. + - **Annotate tier** (`drop-table`, `drop-column`, `drop-default`, `set-not-null`, `alter-type`, `drop-index`): only after you've confirmed steps 1–3 above, add a comment on the line directly above the statement: + ```sql + -- migration-safe: `secret` read removed in v0.6.1 (#1234), shipped two deploys ago + ALTER TABLE "webhook" DROP COLUMN "secret"; + ``` + The reason must be specific and name the PR/version that removed the dependency. An empty reason fails the lint. + - **Warnings** (`data-backfill`): non-blocking, but confirm the batching/idempotency before merging. +4. Verify locally: `cd packages/db && bun run db:migrate` against a dev DB. + +## Hard rule + +Never annotate a destructive statement just to make the lint pass. The annotation is a claim that you verified the old code no longer depends on it. If you can't make that claim truthfully, the change belongs in a later deploy — tell the user to split it. diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index e59102ebd58..6ec3ae6a73e 100644 --- a/.github/workflows/test-build.yml +++ b/.github/workflows/test-build.yml @@ -115,6 +115,16 @@ jobs: - name: Verify realtime prune graph run: bun run check:realtime-prune + - name: Migration safety (zero-downtime) audit + run: | + if [ "${{ github.event_name }}" = "pull_request" ]; then + BASE_REF="origin/${{ github.base_ref }}" + git fetch --depth=1 origin "${{ github.base_ref }}" 2>/dev/null || true + else + BASE_REF="HEAD~1" + fi + bun run check:migrations "$BASE_REF" + - name: Type-check realtime server run: bunx turbo run type-check --filter=@sim/realtime diff --git a/package.json b/package.json index 0dcaf676f30..ba51c3b0670 100644 --- a/package.json +++ b/package.json @@ -27,6 +27,7 @@ "check:realtime-prune": "bun run scripts/check-realtime-prune-graph.ts", "check:zustand-v5": "bun run scripts/check-zustand-v5-selectors.ts", "check:utils": "bun run scripts/check-utils-enforcement.ts", + "check:migrations": "bun run scripts/check-migrations-safety.ts", "mship-contracts:generate": "bun run scripts/sync-mothership-stream-contract.ts", "mship-contracts:check": "bun run scripts/sync-mothership-stream-contract.ts --check", "mship-tools:generate": "bun run scripts/sync-tool-catalog.ts", diff --git a/scripts/check-migrations-safety.test.ts b/scripts/check-migrations-safety.test.ts new file mode 100644 index 00000000000..568f207c3be --- /dev/null +++ b/scripts/check-migrations-safety.test.ts @@ -0,0 +1,154 @@ +/** + * Run with: bun test scripts/check-migrations-safety.test.ts + * (Root scripts are bun-native and not part of the turbo/vitest workspaces.) + */ +import { describe, expect, test } from 'bun:test' +import { lintSql } from './check-migrations-safety.ts' + +const rules = (sql: string) => lintSql(sql).map((f) => `${f.tier}:${f.rule}`) + +describe('additive / safe', () => { + test('nullable add column passes', () => { + expect(lintSql('ALTER TABLE "webhook" ADD COLUMN "provider_config" json;')).toEqual([]) + }) + + test('NOT NULL with DEFAULT passes', () => { + expect(lintSql('ALTER TABLE "user" ADD COLUMN "flag" boolean DEFAULT false NOT NULL;')).toEqual( + [] + ) + }) + + test('CREATE TABLE plus index and FK on that new table passes', () => { + const sql = `CREATE TABLE "kb" ("id" text PRIMARY KEY NOT NULL, "user_id" text NOT NULL); +--> statement-breakpoint +CREATE INDEX "kb_user_id_idx" ON "kb" USING btree ("user_id"); +--> statement-breakpoint +ALTER TABLE "kb" ADD CONSTRAINT "kb_user_fk" FOREIGN KEY ("user_id") REFERENCES "user"("id");` + expect(lintSql(sql)).toEqual([]) + }) + + test('CONCURRENTLY index after a COMMIT breakpoint passes', () => { + const sql = `COMMIT; +--> statement-breakpoint +SET lock_timeout = 0; +--> statement-breakpoint +CREATE INDEX CONCURRENTLY IF NOT EXISTS "idx_x" ON "embedding" ("kb_id");` + expect(lintSql(sql)).toEqual([]) + }) +}) + +describe('hard errors', () => { + test('ADD COLUMN NOT NULL without default', () => { + expect(rules('ALTER TABLE "user" ADD COLUMN "email" text NOT NULL;')).toEqual([ + 'error:add-not-null-no-default', + ]) + }) + + test('RENAME column', () => { + expect(rules('ALTER TABLE "marketplace" RENAME COLUMN "executions" TO "views";')).toEqual([ + 'error:rename', + ]) + }) + + test('CREATE INDEX on existing table without CONCURRENTLY', () => { + expect(rules('CREATE INDEX "idx_y" ON "embedding" ("kb_id");')).toEqual([ + 'error:index-not-concurrent', + ]) + }) + + test('CONCURRENTLY index without IF NOT EXISTS', () => { + const sql = `COMMIT; +--> statement-breakpoint +CREATE INDEX CONCURRENTLY "idx_z" ON "embedding" ("kb_id");` + expect(rules(sql)).toEqual(['error:concurrent-index-not-idempotent']) + }) + + test('CONCURRENTLY index without a preceding COMMIT', () => { + expect( + rules('CREATE INDEX CONCURRENTLY IF NOT EXISTS "idx_z" ON "embedding" ("kb_id");') + ).toEqual(['error:concurrent-index-no-commit']) + }) + + test('ADD FOREIGN KEY on existing table without NOT VALID', () => { + expect( + rules( + 'ALTER TABLE "session" ADD CONSTRAINT "s_fk" FOREIGN KEY ("uid") REFERENCES "user"("id");' + ) + ).toEqual(['error:constraint-not-valid']) + }) +}) + +describe('annotate tier', () => { + const drop = 'ALTER TABLE "webhook" DROP COLUMN "secret";' + + test('DROP COLUMN unannotated fails', () => { + expect(rules(drop)).toEqual(['error:drop-column']) + }) + + test('DROP COLUMN annotated passes', () => { + const sql = `-- migration-safe: secret read removed in v0.6.1 (#1234), shipped two deploys ago\n${drop}` + expect(lintSql(sql)).toEqual([]) + }) + + test('annotation tolerates an intervening statement-breakpoint line', () => { + const sql = `ALTER TABLE "webhook" ADD COLUMN "provider_config" json; +--> statement-breakpoint +-- migration-safe: secret read removed in v0.6.1 (#1234) +${drop}` + expect(lintSql(sql)).toEqual([]) + }) + + test('dangling annotation with empty reason fails', () => { + const sql = `-- migration-safe:\n${drop}` + const found = lintSql(sql) + expect(found).toHaveLength(1) + expect(found[0].tier).toBe('error') + expect(found[0].message).toContain('no reason') + }) + + test('annotation on the wrong statement does not bleed', () => { + const sql = `-- migration-safe: removing secret +ALTER TABLE "webhook" ADD COLUMN "x" json; +--> statement-breakpoint +${drop}` + expect(rules(sql)).toEqual(['error:drop-column']) + }) + + test('type change and DROP TABLE are annotate-tier', () => { + expect( + rules( + 'ALTER TABLE "user_table_rows" ALTER COLUMN "order_key" SET DATA TYPE text COLLATE "C";' + ) + ).toEqual(['error:alter-type']) + expect(rules('DROP TABLE "marketplace_execution" CASCADE;')).toEqual(['error:drop-table']) + }) +}) + +describe('warnings (non-blocking)', () => { + test('UPDATE backfill warns but does not error', () => { + const found = lintSql('UPDATE "user_table_definitions" SET "schema" = \'{}\' WHERE id = \'1\';') + expect(found.map((f) => f.tier)).toEqual(['warn']) + }) + + test('UPDATE without WHERE flags the whole-table note', () => { + const found = lintSql('UPDATE "user" SET "active" = true;') + expect(found[0].tier).toBe('warn') + expect(found[0].message).toContain('no WHERE') + }) +}) + +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([]) + }) + + test('dollar-quoted DO block is one statement; FK on a new table is suppressed', () => { + const sql = `CREATE TABLE "jobs" ("id" text PRIMARY KEY NOT NULL, "wid" text NOT NULL); +--> statement-breakpoint +DO $$ BEGIN + ALTER TABLE "jobs" ADD CONSTRAINT "jobs_fk" FOREIGN KEY ("wid") REFERENCES "workspace"("id"); +EXCEPTION WHEN duplicate_object THEN null; +END $$;` + expect(lintSql(sql)).toEqual([]) + }) +}) diff --git a/scripts/check-migrations-safety.ts b/scripts/check-migrations-safety.ts new file mode 100644 index 00000000000..a0fb89df478 --- /dev/null +++ b/scripts/check-migrations-safety.ts @@ -0,0 +1,470 @@ +#!/usr/bin/env bun +/** + * Guards new Drizzle migrations against deploy-window downtime. + * + * During a deploy the previously-deployed app code keeps serving against the + * freshly-migrated schema (blue/green keeps both versions live). A migration + * that is backward-incompatible with that older code — drops a column it still + * reads, renames, adds a NOT NULL its inserts don't populate — throws until the + * new code takes over. The fix is the expand/contract discipline: additive now, + * destructive only after the dependent code is gone. + * + * This lint is the deterministic half of that guard (the `/db-migrate` skill is + * the judgment half). It classifies every statement in migrations added on this + * branch: + * - HARD ERROR: ops that are essentially never one-deploy-safe. Rewrite them. + * - ANNOTATE: legitimate contract-phase ops. Acknowledge each with a + * `-- migration-safe: ` comment on the preceding line(s), + * only after confirming the dependent code already shipped out. + * - WARN: data backfills — surfaced for review, never block. + * + * Scope is new migration files only (git diff vs base); the existing corpus is + * grandfathered. Usage: + * bun run scripts/check-migrations-safety.ts [baseRef] + * bun run scripts/check-migrations-safety.ts --all # whole corpus + * bun run scripts/check-migrations-safety.ts --dir # a directory + */ +import { execFileSync } from 'node:child_process' +import { readdir, readFile } from 'node:fs/promises' +import path from 'node:path' + +const ROOT = path.resolve(import.meta.dir, '..') +const MIGRATIONS_DIR = 'packages/db/migrations' +const ANNOTATION_PREFIX = '-- migration-safe:' + +type Tier = 'error' | 'warn' + +interface Finding { + line: number + statement: string + tier: Tier + rule: string + message: string +} + +interface Statement { + sql: string + startLine: number +} + +/** Strip quotes and any schema prefix so `"public"."user"` and `"user"` match. */ +function bareName(raw: string): string { + const unquoted = raw.replace(/"/g, '') + const parts = unquoted.split('.') + return (parts[parts.length - 1] ?? unquoted).toLowerCase() +} + +/** + * Split SQL into statements with their 1-based start line, respecting line + * comments (`--`), block comments, and single-quoted strings so a `;` inside + * any of them does not terminate a statement. + */ +function parseStatements(content: string): Statement[] { + const statements: Statement[] = [] + let buf = '' + let startOffset = -1 + let inLine = false + let inBlock = false + let inStr = false + let dollarTag: string | null = null + + const lineAt = (offset: number): number => { + let line = 1 + for (let i = 0; i < offset; i++) if (content[i] === '\n') line++ + return line + } + const flush = () => { + const sql = buf.trim() + if (sql.length > 0 && startOffset >= 0) statements.push({ sql, startLine: lineAt(startOffset) }) + buf = '' + startOffset = -1 + } + + for (let i = 0; i < content.length; i++) { + const c = content[i] + const next = content[i + 1] + + if (inLine) { + if (c === '\n') inLine = false + continue + } + if (inBlock) { + if (c === '*' && next === '/') { + inBlock = false + i++ + } + continue + } + if (inStr) { + buf += c + if (c === "'") { + if (next === "'") { + buf += "'" + i++ + } else { + inStr = false + } + } + continue + } + if (dollarTag) { + if (c === '$' && content.startsWith(dollarTag, i)) { + buf += dollarTag + i += dollarTag.length - 1 + dollarTag = null + } else { + buf += c + } + continue + } + if (c === '$') { + const tag = /^\$[A-Za-z_]*\$/.exec(content.slice(i))?.[0] + if (tag) { + if (startOffset < 0) startOffset = i + dollarTag = tag + buf += tag + i += tag.length - 1 + continue + } + } + if (c === '-' && next === '-') { + inLine = true + i++ + continue + } + if (c === '/' && next === '*') { + inBlock = true + i++ + continue + } + if (c === "'") { + inStr = true + if (startOffset < 0) startOffset = i + buf += c + continue + } + if (c === ';') { + flush() + continue + } + if (startOffset < 0 && !/\s/.test(c)) startOffset = i + buf += c + } + flush() + return statements +} + +/** + * Mirror of the api-validation annotation reader, for `--` SQL comments: + * scans up to three consecutive non-empty preceding lines for the prefix. + * `allowed` when a non-empty reason follows; `missingReason` flags a dangling + * annotation so it fails rather than silently passing. + */ +function readAnnotation( + lines: string[], + startLine: number +): { allowed: boolean; missingReason: boolean } { + let inspected = 0 + for (let i = startLine - 2; i >= 0 && inspected < 3; i--) { + const trimmed = lines[i]?.trim() ?? '' + if (trimmed.length === 0) continue + inspected++ + if (!trimmed.startsWith('--')) return { allowed: false, missingReason: false } + const idx = trimmed.indexOf(ANNOTATION_PREFIX) + if (idx === -1) continue + const reason = trimmed.slice(idx + ANNOTATION_PREFIX.length).trim() + if (reason.length === 0) return { allowed: false, missingReason: true } + return { allowed: true, missingReason: false } + } + return { allowed: false, missingReason: false } +} + +interface RawMatch { + kind: 'error' | 'annotate' | 'warn' + rule: string + message: string +} + +/** + * Classify one statement. `createdTables` holds tables created in the same + * migration — ops against a brand-new table have no old rows and no live + * traffic, so they are always safe and skipped. `sawCommit` tracks whether a + * `COMMIT;` breakpoint preceded a CONCURRENTLY index (see migrate.ts). + */ +function classify(sql: string, createdTables: Set, sawCommit: boolean): RawMatch[] { + const s = sql.replace(/\s+/g, ' ').trim() + const matches: RawMatch[] = [] + + const alterTable = s.match(/\bALTER TABLE (?:IF EXISTS )?(?:ONLY )?("?[.\w]+"?)/i) + const targetTable = alterTable ? bareName(alterTable[1]) : null + const onNewTable = targetTable !== null && createdTables.has(targetTable) + + if (/^CREATE (?:UNIQUE )?INDEX\b/i.test(s)) { + const on = s.match(/\bON ("?[.\w]+"?)/i) + const indexTable = on ? bareName(on[1]) : null + const concurrent = /\bCONCURRENTLY\b/i.test(s) + if (!(indexTable && createdTables.has(indexTable))) { + if (!concurrent) { + matches.push({ + kind: 'error', + rule: 'index-not-concurrent', + message: + 'CREATE INDEX on an existing table write-locks it for the whole build. Use CREATE INDEX CONCURRENTLY IF NOT EXISTS after a COMMIT; breakpoint (see packages/db/scripts/migrate.ts).', + }) + } else if (!/\bIF NOT EXISTS\b/i.test(s)) { + matches.push({ + kind: 'error', + rule: 'concurrent-index-not-idempotent', + message: + 'CREATE INDEX CONCURRENTLY must be IF NOT EXISTS — a failed build replays from the top and a partial INVALID index would be skipped forever.', + }) + } else if (!sawCommit) { + matches.push({ + kind: 'error', + rule: 'concurrent-index-no-commit', + message: + 'CREATE INDEX CONCURRENTLY cannot run inside the migration transaction. Precede it with a COMMIT; breakpoint and SET lock_timeout = 0 (see packages/db/scripts/migrate.ts).', + }) + } + } + } + + if ( + !onNewTable && + /\bADD COLUMN\b/i.test(s) && + /\bNOT NULL\b/i.test(s) && + !/\bDEFAULT\b/i.test(s) + ) { + matches.push({ + kind: 'error', + rule: 'add-not-null-no-default', + message: + 'ADD COLUMN NOT NULL with no DEFAULT breaks old inserts (and fails on existing rows). Add it nullable or with a DEFAULT, backfill, then SET NOT NULL in a later migration once code populates it.', + }) + } + + if (/\bRENAME\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.', + }) + } + + if ( + !onNewTable && + /\bADD CONSTRAINT\b/i.test(s) && + /\b(FOREIGN KEY|CHECK)\b/i.test(s) && + !/\bNOT VALID\b/i.test(s) + ) { + matches.push({ + kind: 'error', + rule: 'constraint-not-valid', + message: + 'ADD CONSTRAINT FOREIGN KEY/CHECK on an existing table locks it and rejects old writes that violate it. Add it NOT VALID, then VALIDATE CONSTRAINT in a separate step.', + }) + } + + if (!onNewTable) { + if (/^DROP TABLE\b/i.test(s)) { + matches.push({ kind: 'annotate', rule: 'drop-table', message: 'DROP TABLE' }) + } + if (/\bDROP COLUMN\b/i.test(s)) { + matches.push({ kind: 'annotate', rule: 'drop-column', message: 'DROP COLUMN' }) + } + if (/\bDROP CONSTRAINT\b/i.test(s)) { + matches.push({ kind: 'annotate', rule: 'drop-constraint', message: 'DROP CONSTRAINT' }) + } + if (/\bDROP DEFAULT\b/i.test(s)) { + matches.push({ kind: 'annotate', rule: 'drop-default', message: 'DROP DEFAULT' }) + } + 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)) { + 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 (/^(UPDATE|DELETE)\b/i.test(s)) { + const noWhere = !/\bWHERE\b/i.test(s) + matches.push({ + kind: 'warn', + rule: 'data-backfill', + message: noWhere + ? 'data backfill with no WHERE rewrites/locks the whole table. Confirm it is batched, idempotent, and safe under concurrent writes.' + : 'data backfill. Confirm it is batched, idempotent, and safe under concurrent writes.', + }) + } + + return matches +} + +const ANNOTATE_GUIDANCE = + 'is a contract-phase op. Confirm the old code no longer reads/writes it (it must have shipped in an earlier deploy — not this same PR), then acknowledge with a `-- migration-safe: ` comment on the line above.' + +/** Lint a single migration's SQL. Returns only actionable findings. */ +export function lintSql(content: string): Finding[] { + const lines = content.split('\n') + const statements = parseStatements(content) + const createdTables = new Set() + for (const { sql } of statements) { + const m = sql.match(/^CREATE TABLE (?:IF NOT EXISTS )?("?[.\w]+"?)/i) + if (m) createdTables.add(bareName(m[1])) + } + + const findings: Finding[] = [] + let sawCommit = false + for (const { sql, startLine } of statements) { + for (const match of classify(sql, createdTables, sawCommit)) { + if (match.kind === 'error') { + findings.push({ + line: startLine, + statement: sql, + tier: 'error', + rule: match.rule, + message: match.message, + }) + } else if (match.kind === 'warn') { + findings.push({ + line: startLine, + statement: sql, + tier: 'warn', + rule: match.rule, + message: match.message, + }) + } else { + const ann = readAnnotation(lines, startLine) + if (ann.allowed) continue + findings.push({ + line: startLine, + statement: sql, + tier: 'error', + rule: match.rule, + message: ann.missingReason + ? `${match.message}: \`-- migration-safe:\` annotation has no reason. Give it a real justification.` + : `${match.message} ${ANNOTATE_GUIDANCE}`, + }) + } + } + if (/^COMMIT\b/i.test(sql.trim())) sawCommit = true + } + return findings +} + +function git(args: string[]): string | null { + try { + return execFileSync('git', args, { + cwd: ROOT, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + }).trim() + } catch { + return null + } +} + +/** New migration files on this branch vs base, plus uncommitted ones locally. */ +function changedMigrationFiles(baseRef: string): string[] { + const files = new Set() + const inDir = (p: string) => p.startsWith(`${MIGRATIONS_DIR}/`) && p.endsWith('.sql') + + const mergeBase = git(['merge-base', baseRef, 'HEAD']) ?? baseRef + const committed = git([ + 'diff', + '--name-only', + '--diff-filter=AM', + mergeBase, + 'HEAD', + '--', + MIGRATIONS_DIR, + ]) + if (committed === null) return [] // git unavailable → fail open (handled by caller) + for (const f of committed.split('\n')) if (inDir(f)) files.add(f) + + const status = git(['status', '--porcelain', '--', MIGRATIONS_DIR]) + if (status) { + for (const raw of status.split('\n')) { + const p = raw.slice(3).trim() + if (inDir(p)) files.add(p) + } + } + return [...files] +} + +async function listSqlFiles(dir: string): Promise { + const out: string[] = [] + const entries = await readdir(dir, { withFileTypes: true }) + for (const e of entries) { + const full = path.join(dir, e.name) + if (e.isDirectory()) out.push(...(await listSqlFiles(full))) + else if (e.name.endsWith('.sql')) out.push(full) + } + return out +} + +async function resolveFiles(argv: string[]): Promise { + if (argv.includes('--all')) { + return (await listSqlFiles(path.join(ROOT, MIGRATIONS_DIR))).map((f) => path.relative(ROOT, f)) + } + const dirIdx = argv.indexOf('--dir') + if (dirIdx !== -1) { + const dir = argv[dirIdx + 1] + if (!dir) throw new Error('--dir requires a path') + return (await listSqlFiles(path.resolve(dir))).map((f) => path.relative(ROOT, f)) + } + const baseRef = argv.find((a) => !a.startsWith('--')) ?? 'origin/main' + const files = changedMigrationFiles(baseRef) + if (files.length === 0 && git(['rev-parse', 'HEAD']) === null) { + console.warn('⚠ git unavailable — skipping migration safety check.') + return null + } + return files +} + +async function main() { + const files = await resolveFiles(process.argv.slice(2)) + if (files === null) process.exit(0) + + if (files.length === 0) { + console.log('✓ No new migrations to check.') + process.exit(0) + } + + let errors = 0 + let warnings = 0 + for (const rel of files) { + const content = await readFile(path.join(ROOT, rel), 'utf8') + const findings = lintSql(content) + if (findings.length === 0) continue + + console.error(`\n${rel}`) + for (const f of findings.sort((a, b) => a.line - b.line)) { + const icon = f.tier === 'error' ? '✗' : '⚠' + if (f.tier === 'error') errors++ + else warnings++ + console.error(` ${icon} ${rel}:${f.line} [${f.rule}]`) + console.error(` ${f.statement.replace(/\s+/g, ' ').slice(0, 120)}`) + console.error(` → ${f.message}`) + } + } + + if (errors === 0) { + console.log( + warnings > 0 + ? `\n✓ No blocking migration issues (${warnings} warning(s) to review).` + : '\n✓ Migrations are backward-compatible.' + ) + process.exit(0) + } + console.error( + `\nFound ${errors} blocking migration issue(s). Rewrite hard errors into expand/contract, or annotate contract ops once safe. See the /db-migrate skill.` + ) + process.exit(1) +} + +if (import.meta.main) main() From 0b278d36d8b61d8ea1c944d0294137d05a6aef2d Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Sat, 13 Jun 2026 19:07:43 -0700 Subject: [PATCH 2/6] feat(skills): run cleanup and db-migrate safety checks in /ship --- .agents/skills/ship/SKILL.md | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/.agents/skills/ship/SKILL.md b/.agents/skills/ship/SKILL.md index 85fefb30ea6..9b827fe528c 100644 --- a/.agents/skills/ship/SKILL.md +++ b/.agents/skills/ship/SKILL.md @@ -1,6 +1,6 @@ --- name: ship -description: Commit, push, and open a PR to staging in one shot +description: Commit, push, and open a PR to staging in one shot — runs the cleanup pass and, when migrations changed, the db-migrate safety review first --- # Ship Command @@ -16,12 +16,17 @@ When the user runs `/ship`: - Types: `fix`, `feat`, `improvement`, `chore` - Scope: short identifier (e.g., `undo-redo`, `api`, `ui`) - Keep it concise -3. **Run pre-ship checks** from the repo root before staging: +3. **Run the cleanup pass** on the current changes before anything else: `/cleanup` + - This runs the six code-quality skills (effects, memo, callbacks, state, React Query, emcn) and applies fixes, so they land in this commit. +4. **Run migration safety** — only if the diff touches `packages/db/migrations/**` or `packages/db/schema.ts`: + - Run `/db-migrate` to review the migration for zero-downtime safety (expand/contract phasing, backward-compatibility with the deployed app version). + - `bun run check:migrations` must pass. Do not silence a flagged statement with a `-- migration-safe:` annotation unless `/db-migrate` confirmed the old code no longer depends on it; otherwise split the destructive change into a later deploy. +5. **Run pre-ship checks** from the repo root before staging: - `bun run lint` to fix formatting issues - `bun run check:api-validation:strict` to catch boundary contract failures before CI -4. **Stage and commit** the changes with the generated message -5. **Push to origin** using the current branch name -6. **Create a PR** to staging with a description in the user's voice +6. **Stage and commit** the changes with the generated message +7. **Push to origin** using the current branch name +8. **Create a PR** to staging with a description in the user's voice ## Commit Message Format @@ -77,7 +82,7 @@ gh pr create --base staging --title "COMMIT_MESSAGE" --body "PR_BODY" - Short, direct bullet points - No unnecessary explanation -- "Tested manually" is acceptable for testing section; include lint and boundary validation results when run +- "Tested manually" is acceptable for testing section; include lint, boundary validation, and (when migrations changed) `check:migrations` results when run - Checkboxes filled in appropriately - No screenshots section unless UI changes From 84093a242939ca61dd6077cd0dfd9777ae4b30e1 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Mon, 15 Jun 2026 11:12:18 -0700 Subject: [PATCH 3/6] =?UTF-8?q?fix(db):=20address=20review=20=E2=80=94=20D?= =?UTF-8?q?ROP=20INDEX=20lock=20symmetry,=20RENAME=20CONSTRAINT=20false-po?= =?UTF-8?q?sitive,=20alter-type=20literal=20match?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- .agents/skills/db-migrate/SKILL.md | 1 + scripts/check-migrations-safety.test.ts | 39 +++++++++++++++++++++++++ scripts/check-migrations-safety.ts | 22 +++++++++++--- 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/.agents/skills/db-migrate/SKILL.md b/.agents/skills/db-migrate/SKILL.md index 3903eaaf32c..c05b3895eee 100644 --- a/.agents/skills/db-migrate/SKILL.md +++ b/.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. diff --git a/scripts/check-migrations-safety.test.ts b/scripts/check-migrations-safety.test.ts index 568f207c3be..bec47a39b90 100644 --- a/scripts/check-migrations-safety.test.ts +++ b/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([]) diff --git a/scripts/check-migrations-safety.ts b/scripts/check-migrations-safety.ts index a0fb89df478..b5fab846fb1 100644 --- a/scripts/check-migrations-safety.ts +++ b/scripts/check-migrations-safety.ts @@ -243,12 +243,12 @@ function classify(sql: string, createdTables: Set, 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, 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)) { From be6d0bcbb3bbfbc12b8147b5e77f948fe521b353 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Mon, 15 Jun 2026 11:18:47 -0700 Subject: [PATCH 4/6] fix(db): enforce IF EXISTS on DROP INDEX CONCURRENTLY for replay idempotency Symmetric with the CREATE INDEX CONCURRENTLY rule: a post-COMMIT DROP INDEX CONCURRENTLY replays from the top on failure, so without IF EXISTS it aborts re-dropping an already-gone index. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/check-migrations-safety.test.ts | 7 +++++++ scripts/check-migrations-safety.ts | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/scripts/check-migrations-safety.test.ts b/scripts/check-migrations-safety.test.ts index bec47a39b90..af966d6e8dd 100644 --- a/scripts/check-migrations-safety.test.ts +++ b/scripts/check-migrations-safety.test.ts @@ -165,6 +165,13 @@ DROP INDEX CONCURRENTLY IF EXISTS "stale_idx";` expect(lintSql(sql)).toEqual([]) }) + test('DROP INDEX CONCURRENTLY without IF EXISTS is not idempotent', () => { + const sql = `COMMIT; +--> statement-breakpoint +DROP INDEX CONCURRENTLY "stale_idx";` + expect(rules(sql)).toEqual(['error:concurrent-drop-index-not-idempotent']) + }) + 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', diff --git a/scripts/check-migrations-safety.ts b/scripts/check-migrations-safety.ts index b5fab846fb1..5dbbc2ca479 100644 --- a/scripts/check-migrations-safety.ts +++ b/scripts/check-migrations-safety.ts @@ -294,6 +294,13 @@ function classify(sql: string, createdTables: Set, sawCommit: boolean): 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 (!/\bIF EXISTS\b/i.test(s)) { + matches.push({ + kind: 'error', + rule: 'concurrent-drop-index-not-idempotent', + message: + 'DROP INDEX CONCURRENTLY must be IF EXISTS — a failed run replays from the top and would abort re-dropping an already-gone index.', + }) } else if (!sawCommit) { matches.push({ kind: 'error', From a986f588ed46e2afb91f8cf5bf5907504caa9c76 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Mon, 15 Jun 2026 12:35:53 -0700 Subject: [PATCH 5/6] improvement(skills): gate /ship cleanup on UI changes; default migration base to staging - /ship runs /cleanup only when the diff touches UI code (.tsx or apps/sim/components|hooks|stores); the six passes are React-only. - /ship runs check:migrations against origin/staging (the PR base). - check:migrations default baseRef is now origin/staging instead of origin/main. Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/ship/SKILL.md | 6 +++--- scripts/check-migrations-safety.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.agents/skills/ship/SKILL.md b/.agents/skills/ship/SKILL.md index 9b827fe528c..db61568d6c0 100644 --- a/.agents/skills/ship/SKILL.md +++ b/.agents/skills/ship/SKILL.md @@ -16,11 +16,11 @@ When the user runs `/ship`: - Types: `fix`, `feat`, `improvement`, `chore` - Scope: short identifier (e.g., `undo-redo`, `api`, `ui`) - Keep it concise -3. **Run the cleanup pass** on the current changes before anything else: `/cleanup` - - This runs the six code-quality skills (effects, memo, callbacks, state, React Query, emcn) and applies fixes, so they land in this commit. +3. **Run the cleanup pass** — only if the diff modifies UI code (any `.tsx` file, or anything under `apps/sim/components/`, `apps/sim/hooks/`, or `apps/sim/stores/`): `/cleanup` + - The six code-quality skills (effects, memo, callbacks, state, React Query, emcn) only apply to React code, so skip this step entirely when no UI was touched. When it runs, it applies fixes so they land in this commit. 4. **Run migration safety** — only if the diff touches `packages/db/migrations/**` or `packages/db/schema.ts`: - Run `/db-migrate` to review the migration for zero-downtime safety (expand/contract phasing, backward-compatibility with the deployed app version). - - `bun run check:migrations` must pass. Do not silence a flagged statement with a `-- migration-safe:` annotation unless `/db-migrate` confirmed the old code no longer depends on it; otherwise split the destructive change into a later deploy. + - `bun run check:migrations origin/staging` must pass (staging is the PR base). Do not silence a flagged statement with a `-- migration-safe:` annotation unless `/db-migrate` confirmed the old code no longer depends on it; otherwise split the destructive change into a later deploy. 5. **Run pre-ship checks** from the repo root before staging: - `bun run lint` to fix formatting issues - `bun run check:api-validation:strict` to catch boundary contract failures before CI diff --git a/scripts/check-migrations-safety.ts b/scripts/check-migrations-safety.ts index 5dbbc2ca479..5c4e299af23 100644 --- a/scripts/check-migrations-safety.ts +++ b/scripts/check-migrations-safety.ts @@ -20,7 +20,7 @@ * * Scope is new migration files only (git diff vs base); the existing corpus is * grandfathered. Usage: - * bun run scripts/check-migrations-safety.ts [baseRef] + * bun run scripts/check-migrations-safety.ts [baseRef] # base defaults to origin/staging * bun run scripts/check-migrations-safety.ts --all # whole corpus * bun run scripts/check-migrations-safety.ts --dir # a directory */ @@ -438,7 +438,7 @@ async function resolveFiles(argv: string[]): Promise { if (!dir) throw new Error('--dir requires a path') return (await listSqlFiles(path.resolve(dir))).map((f) => path.relative(ROOT, f)) } - const baseRef = argv.find((a) => !a.startsWith('--')) ?? 'origin/main' + const baseRef = argv.find((a) => !a.startsWith('--')) ?? 'origin/staging' const files = changedMigrationFiles(baseRef) if (files.length === 0 && git(['rev-parse', 'HEAD']) === null) { console.warn('⚠ git unavailable — skipping migration safety check.') From 4c3f3faf764f1decf0ac4e28b7a677ac6610d35f Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Mon, 15 Jun 2026 16:39:23 -0700 Subject: [PATCH 6/6] docs(db-migrate): add contract-pending TODO convention for deferred drops Establishes a durable, greppable marker (`contract-pending(): ...`) left on the legacy column in schema.ts when an expand defers a drop, so the contract phase doesn't rot. The outstanding-work list is `grep -rn contract-pending`; the contract PR's `-- migration-safe:` annotation references the expand and deletes the marker. Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/db-migrate/SKILL.md | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/.agents/skills/db-migrate/SKILL.md b/.agents/skills/db-migrate/SKILL.md index c05b3895eee..d00f375f9ab 100644 --- a/.agents/skills/db-migrate/SKILL.md +++ b/.agents/skills/db-migrate/SKILL.md @@ -39,6 +39,28 @@ Never put expand and contract in the same PR. If this PR both removes the code t 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. +## Tracking the contract (don't let it rot) + +The contract half is deferred to a later deploy — and that is exactly when it gets forgotten, leaving dead columns, orphaned tables, and `NOT NULL`s that never land. Every deferred contract must become a durable, greppable TODO. + +When an expand defers a drop, leave a **`contract-pending`** marker on the legacy column/table in `packages/db/schema.ts` — that is the file you will be editing when you finally do the drop, so the reminder lives where the work happens: + +```ts +// contract-pending(after #5035 is fully deployed): drop once permission-check.ts stops reading it +workspaceId: text('workspace_id'), +``` + +Format: `contract-pending(): — `. The precondition names the PR/release that removes the last reader and **must be fully deployed** before the contract ships. + +- **The TODO list is a grep** — always accurate, never drifts: `grep -rn "contract-pending" packages/db apps/sim`. Run it when starting migration work to see what is owed. +- For anything with a real owner or schedule, also open a tracking issue and put its number in the marker. +- **Close the loop in the contract PR:** the contract migration's `-- migration-safe:` annotation references the expand, and you **delete the `contract-pending` marker** in the same PR: + ```sql + -- migration-safe: contract of #5035 — workspace_id readers removed there, deployed 2026-06-10 + ALTER TABLE "permission_group" DROP COLUMN "workspace_id"; + ``` +- An expand merged **without** a marker for the drop it defers, or a contract merged **without** removing its marker, is a bug — flag it in review. + ## The judgment the lint can't do The lint flags risky *shapes*; it cannot know whether a given drop is *safe right now*. For each flagged statement, do the work it can't: @@ -49,9 +71,9 @@ The lint flags risky *shapes*; it cannot know whether a given drop is *safe righ ## Workflow -1. Edit `packages/db/schema.ts`, then `cd packages/db && bunx drizzle-kit generate` to produce the SQL. +1. Edit `packages/db/schema.ts`, then `cd packages/db && bunx drizzle-kit generate` to produce the SQL. If this is an expand that defers a drop, leave a `contract-pending` marker on the legacy column (see "Tracking the contract"). If this is the contract, delete the marker it resolves. 2. Hand-edit the generated SQL where the playbook requires it: `CONCURRENTLY` + `COMMIT;` breakpoint for indexes on existing tables, `NOT VALID` for constraints, batching for backfills. -3. Run `bun run check:migrations` (or `bun run scripts/check-migrations-safety.ts main` locally). +3. Run `bun run check:migrations` (base defaults to `origin/staging`). - **Hard errors** (`add-not-null-no-default`, `rename`, `index-not-concurrent`, `constraint-not-valid`, …): rewrite into expand/contract. Do **not** try to annotate them away — the lint won't accept it. - **Annotate tier** (`drop-table`, `drop-column`, `drop-default`, `set-not-null`, `alter-type`, `drop-index`): only after you've confirmed steps 1–3 above, add a comment on the line directly above the statement: ```sql