From 7a3ba6aa3066f4dab35baff78e9b2c57af36c374 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Sat, 13 Jun 2026 18:56:36 -0700 Subject: [PATCH 1/2] 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/2] 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