From e0b1bab7f53b345a612ac253f793cc9648b04eaa Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 14:23:26 +1000 Subject: [PATCH 1/5] feat(cli): add `stash eql repair --drizzle` with applied-migration awareness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #710. Repairing a migration drizzle-kit generated with an un-runnable in-place `ALTER COLUMN ... SET DATA TYPE ` meant running `stash eql migration --drizzle` a second time — generating a redundant EQL install migration purely to trigger the sweep, because the sweep runs before `drizzle-kit generate` has emitted the broken statement. `eql repair` runs the same rewriter standalone. Both commands now report through one helper (`eql/sweep-report.ts`), so the two surfaces render the rewritten list, the per-reason skip guidance, the staged reconciliation notice and the partial-sweep report identically and cannot drift. New capability the generate-time sweep cannot offer: applied-migration awareness. An ALTER to an EQL domain is un-runnable, so its migration failed and is safe to rewrite — except for a `jsonb` column changed to an EQL domain on an empty table, which applies. Rewriting that afterwards leaves the .sql describing a shape the database never got from it, so a fresh CI or staging database replaying it diverges silently. Repair reads `meta/_journal.json` offline and, given `--database-url`/`DATABASE_URL`, the latest `created_at` in `drizzle.__drizzle_migrations`; a migration is applied when its journal `when` is at or below that watermark — the same timestamp comparison `drizzle-kit migrate` makes (drizzle-orm 0.45.2 pg-core/dialect.js:62; hashes are written, never compared). Applied migrations are reported as their own outcome, left untouched, and the command exits non-zero. Offline the repair proceeds and warns that applied state is unverified: the journal proves a migration exists, not that it ran, and refusing on that ambiguity would make the command useless in exactly the flow it exists for. A check that is requested but cannot run is a hard failure — nothing is rewritten. `rewriteEncryptedAlterColumns` gains `dryRun` and a multi-path `skip`; the wizard's copy carries the identical change (scripts/__tests__/ rewriter-copies-in-sync.test.mjs). Also updates skills/stash-cli and skills/stash-drizzle (both ship in the tarball) and the CLI README, and adds a changeset. --- .changeset/eql-repair-command.md | 42 ++ packages/cli/README.md | 20 + packages/cli/src/bin/main.ts | 11 + packages/cli/src/cli/__tests__/help.test.ts | 13 + packages/cli/src/cli/registry.ts | 39 ++ .../cli/src/commands/db/rewrite-migrations.ts | 18 +- .../src/commands/eql/__tests__/repair.test.ts | 586 ++++++++++++++++++ packages/cli/src/commands/eql/applied.ts | 72 +++ packages/cli/src/commands/eql/journal.ts | 74 +++ packages/cli/src/commands/eql/migration.ts | 67 +- packages/cli/src/commands/eql/repair.ts | 211 +++++++ packages/cli/src/commands/eql/sweep-report.ts | 105 ++++ packages/cli/src/messages.ts | 70 +++ .../cli/tests/e2e/command-help.e2e.test.ts | 12 + packages/cli/tests/e2e/eql-repair.e2e.test.ts | 75 +++ packages/cli/tests/e2e/smoke.e2e.test.ts | 1 + packages/wizard/src/lib/rewrite-migrations.ts | 16 +- skills/stash-cli/SKILL.md | 29 +- skills/stash-drizzle/SKILL.md | 13 +- 19 files changed, 1406 insertions(+), 68 deletions(-) create mode 100644 .changeset/eql-repair-command.md create mode 100644 packages/cli/src/commands/eql/__tests__/repair.test.ts create mode 100644 packages/cli/src/commands/eql/applied.ts create mode 100644 packages/cli/src/commands/eql/journal.ts create mode 100644 packages/cli/src/commands/eql/repair.ts create mode 100644 packages/cli/src/commands/eql/sweep-report.ts create mode 100644 packages/cli/tests/e2e/eql-repair.e2e.test.ts diff --git a/.changeset/eql-repair-command.md b/.changeset/eql-repair-command.md new file mode 100644 index 000000000..b1e2a4a1c --- /dev/null +++ b/.changeset/eql-repair-command.md @@ -0,0 +1,42 @@ +--- +'stash': minor +'@cipherstash/wizard': patch +--- + +Add `stash eql repair --drizzle` — repair a migration directory that `drizzle-kit +generate` filled with an un-runnable in-place `ALTER COLUMN … SET DATA TYPE +`, without generating anything (cipherstash/stack#710). + +```bash +stash eql repair --drizzle # sweep drizzle/ +stash eql repair --drizzle --dry-run # preview; writes nothing +stash eql repair --drizzle --database-url … # leave applied migrations alone +``` + +Until now the only way to run that sweep was `stash eql migration --drizzle`, +which generates a redundant EQL install migration as a side effect purely to +trigger it — the sweep runs before `drizzle-kit generate` has emitted the broken +statement, so recovery meant creating a migration you did not want. `eql repair` +runs the same rewriter and prints the same report (both commands now share one +reporting path, so the two surfaces cannot drift). + +**New: applied-migration awareness.** The sweep has always been unfiltered. That +is harmless for almost every match, because an ALTER to an EQL domain cannot run +— so the migration failed and was never applied. The exception is a `jsonb` +column changed to an EQL domain on an empty table, which applies successfully; +rewriting it afterwards leaves the `.sql` describing a shape the database never +got from it, and a fresh CI or staging database replaying the rewritten file +diverges from the original, silently. + +`eql repair` therefore reads `meta/_journal.json` offline and, given +`--database-url` (or `DATABASE_URL`), the latest `created_at` in +`drizzle.__drizzle_migrations`. A migration is applied when its journal `when` is +at or below that watermark — the same timestamp comparison `drizzle-kit migrate` +makes, hashes being written but never compared. Applied migrations are reported +as their own outcome, left untouched, and the command exits non-zero. Without a +database URL the repair proceeds and warns that applied state could not be +verified; if the check is requested but cannot run, nothing is rewritten. + +`rewriteEncryptedAlterColumns` gained `dryRun`, and its `skip` option now accepts +several paths as well as one. The wizard's copy of the rewriter carries the same +change so the two stay in sync; its own sweep is unaffected. diff --git a/packages/cli/README.md b/packages/cli/README.md index 7bb470e2b..a34d07111 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -308,6 +308,26 @@ npx drizzle-kit migrate --- +### `npx stash eql repair --drizzle` + +Repairs migrations `drizzle-kit generate` emitted with an in-place `ALTER COLUMN … SET DATA TYPE `, which Postgres cannot run (there is no cast from `text`/`numeric` to an EQL domain). Each is rewritten into an additive `ADD COLUMN "_encrypted"` that preserves the source column. + +```bash +npx stash eql repair --drizzle +npx drizzle-kit migrate +``` + +| Flag | Description | +|------|-------------| +| `--drizzle` | Required. Repair a Drizzle migration directory | +| `--out ` | Directory to sweep. Default `drizzle` | +| `--dry-run` | Report what would be rewritten without writing anything | +| `--database-url ` | Leave migrations the database has already applied untouched | + +This is the same sweep `eql migration --drizzle` performs, without generating an install migration you do not need. With `--database-url` it reads `drizzle.__drizzle_migrations` and refuses to rewrite an already-applied migration — doing so would leave the file describing a shape that database never got from it, and a fresh CI or staging database replaying it would silently diverge. Without a URL it proceeds and warns that applied state could not be verified. + +--- + ## Required database permissions Before installing EQL, the CLI verifies that the connected role has: diff --git a/packages/cli/src/bin/main.ts b/packages/cli/src/bin/main.ts index 773e41c3a..73789d494 100644 --- a/packages/cli/src/bin/main.ts +++ b/packages/cli/src/bin/main.ts @@ -110,6 +110,7 @@ Commands: eql install Scaffold stash.config.ts (if missing) and install EQL extensions eql migration Generate an EQL v3 install migration for your ORM (Drizzle) + eql repair Repair migrations with an un-runnable ALTER COLUMN to an encrypted type eql upgrade Upgrade EQL extensions to the latest version eql status Show EQL installation status @@ -264,6 +265,16 @@ async function runEqlCommand( }) break } + case 'repair': { + const { eqlRepairCommand } = await import('../commands/eql/repair.js') + await eqlRepairCommand({ + drizzle: flags.drizzle, + out: values.out, + dryRun: flags['dry-run'], + databaseUrl: values['database-url'], + }) + break + } case 'upgrade': await runUpgrade(flags, values) break diff --git a/packages/cli/src/cli/__tests__/help.test.ts b/packages/cli/src/cli/__tests__/help.test.ts index de0c48906..d3d9df596 100644 --- a/packages/cli/src/cli/__tests__/help.test.ts +++ b/packages/cli/src/cli/__tests__/help.test.ts @@ -53,6 +53,19 @@ describe('renderCommandHelp', () => { expect(out).toContain('auth regions') }) + // `eql repair` is the only command that can consult applied state, so its + // --database-url is load-bearing rather than decorative — help must show it + // alongside the flags that shape the sweep. + it('renders every flag of the standalone repair command', () => { + const out = renderCommandHelp('eql repair', RUNNER) + expect(out).not.toBeNull() + expect(out).toContain('Usage: npx stash eql repair [options]') + expect(out).toContain('--drizzle') + expect(out).toContain('--out ') + expect(out).toContain('--dry-run') + expect(out).toContain('--database-url ') + }) + it('renders a summary-only command without empty Options/Examples', () => { const out = renderCommandHelp('wizard', RUNNER) expect(out).toContain('Usage: npx stash wizard [options]') diff --git a/packages/cli/src/cli/registry.ts b/packages/cli/src/cli/registry.ts index 6eb715609..e3549329c 100644 --- a/packages/cli/src/cli/registry.ts +++ b/packages/cli/src/cli/registry.ts @@ -360,6 +360,45 @@ export const registry: CommandGroup[] = [ DRY_RUN_FLAG, ], }, + { + name: 'eql repair', + summary: + 'Repair migrations drizzle-kit generated with an un-runnable ALTER COLUMN to an encrypted type', + long: [ + 'Sweep an existing Drizzle output directory for in-place', + '`ALTER COLUMN ... SET DATA TYPE ` statements — which cannot run,', + 'because Postgres has no cast from text/numeric to an EQL domain — and rewrite', + 'each into an additive encrypted column that preserves the source column.', + '', + 'This is the same sweep `eql migration --drizzle` runs, without having to', + 'generate an EQL install migration you do not want just to trigger it.', + '', + 'Migrations the database has already applied are reported and left alone:', + 'rewriting one would leave its .sql describing a shape that database never got', + 'from it, so a fresh CI or staging database replaying the file would silently', + 'diverge. Pass --database-url so that check can run; without it the repair', + 'proceeds and warns that applied state could not be verified.', + ].join('\n'), + examples: [ + 'eql repair --drizzle', + 'eql repair --drizzle --dry-run', + 'eql repair --drizzle --out db/migrations --database-url postgres://…', + ], + flags: [ + { + name: '--drizzle', + description: 'Repair a Drizzle migration directory.', + }, + { + name: '--out', + value: '', + description: + 'Directory holding the migrations to sweep. Defaults to `drizzle`; set it to match your drizzle.config.ts.', + }, + DRY_RUN_FLAG, + DATABASE_URL_FLAG, + ], + }, { name: 'eql upgrade', summary: 'Upgrade EQL extensions to the latest version', diff --git a/packages/cli/src/commands/db/rewrite-migrations.ts b/packages/cli/src/commands/db/rewrite-migrations.ts index c8c60ca1f..adac6487f 100644 --- a/packages/cli/src/commands/db/rewrite-migrations.ts +++ b/packages/cli/src/commands/db/rewrite-migrations.ts @@ -1038,10 +1038,19 @@ interface RewriteSweepError extends Error, PartialRewriteResult {} * shipping broken SQL. Statements sitting inside a * SQL comment — or inside a single-quoted string literal, where they are data * rather than SQL — are inert and are neither rewritten nor reported. + * + * `options.skip` names files to leave on disk — they are still READ, because a + * column's current type comes from the whole corpus, but never written. One + * path or many: `eql migration` passes the install migration it just generated, + * `eql repair` passes every migration already applied to the database. + * + * `options.dryRun` computes the identical result without writing anything, so a + * caller can preview a repair — or ask which files WOULD be rewritten — before + * touching migrations that are about to be applied. */ export async function rewriteEncryptedAlterColumns( outDir: string, - options: { skip?: string } = {}, + options: { skip?: string | readonly string[]; dryRun?: boolean } = {}, ): Promise { const entries = await readdir(outDir).catch( (error: NodeJS.ErrnoException) => { @@ -1057,6 +1066,9 @@ export async function rewriteEncryptedAlterColumns( const staged: StagedColumn[] = [] const seen = new Set() const stagedTargets = new Set() + const skipFiles = new Set( + typeof options.skip === 'string' ? [options.skip] : (options.skip ?? []), + ) /** Record a skip once — the strict pass and the broad scan can both find it. */ const skip = (file: string, statement: string, reason: SkipReason): void => { @@ -1084,7 +1096,7 @@ export async function rewriteEncryptedAlterColumns( try { for (const [filePath, original] of contents) { - if (options.skip && filePath === options.skip) continue + if (skipFiles.has(filePath)) continue // Reset the regex's lastIndex — it's stateful on /g ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 @@ -1165,7 +1177,7 @@ export async function rewriteEncryptedAlterColumns( ) if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') + if (!options.dryRun) await writeFile(filePath, updated, 'utf-8') rewritten.push(filePath) staged.push(...fileStaged) } diff --git a/packages/cli/src/commands/eql/__tests__/repair.test.ts b/packages/cli/src/commands/eql/__tests__/repair.test.ts new file mode 100644 index 000000000..fba499f71 --- /dev/null +++ b/packages/cli/src/commands/eql/__tests__/repair.test.ts @@ -0,0 +1,586 @@ +import { + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { CliExit } from '../../../cli/exit.js' +import { messages } from '../../../messages.js' +import { eqlRepairCommand } from '../repair.js' + +// clack is chrome — silence it and spy on the channels the command reports +// through. `step` is on the real clack `log`; omitting it makes the +// per-statement sweep report throw. +const clack = vi.hoisted(() => ({ + spinnerInstance: { start: vi.fn(), stop: vi.fn() }, + log: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + step: vi.fn(), + }, + intro: vi.fn(), + note: vi.fn(), + outro: vi.fn(), +})) +vi.mock('@clack/prompts', () => ({ + spinner: vi.fn(() => clack.spinnerInstance), + log: clack.log, + intro: clack.intro, + note: clack.note, + outro: clack.outro, +})) + +// The applied-state probe is the one part of the command that talks to a +// database. Fake the driver rather than the module that uses it, so the real +// query text and the real timestamp comparison stay under test. +const pgMock = vi.hoisted(() => ({ + connect: vi.fn(), + query: vi.fn(), + end: vi.fn(), + connectionStrings: [] as (string | undefined)[], +})) +vi.mock('pg', () => ({ + default: { + Client: vi.fn((config: { connectionString?: string }) => { + pgMock.connectionStrings.push(config?.connectionString) + return { + connect: pgMock.connect, + query: pgMock.query, + end: pgMock.end, + } + }), + }, +})) + +// The sweep stays REAL by default — every other test drives it through actual +// SQL on disk. The spy exists only to reach the "sweep threw" branch, with a +// failure the sweep cannot be made to produce from a fixture (EACCES). +const rewriteMock = vi.hoisted(() => ({ + real: (() => { + throw new Error( + 'rewriteMock.real not initialised: rewrite-migrations mock factory did not run', + ) + }) as typeof import('../../db/rewrite-migrations.js').rewriteEncryptedAlterColumns, + spy: vi.fn(), +})) +vi.mock('../../db/rewrite-migrations.js', async (importOriginal) => { + const actual = + await importOriginal() + rewriteMock.real = actual.rewriteEncryptedAlterColumns + return { ...actual, rewriteEncryptedAlterColumns: rewriteMock.spy } +}) + +/** Stand in for `select max(created_at) from drizzle.__drizzle_migrations`. */ +function mockLatestAppliedMillis(millis: number | null): void { + // node-postgres returns bigint columns as strings; the command must cope. + pgMock.query.mockResolvedValue({ + rows: [{ max_created_at: millis === null ? null : String(millis) }], + }) +} + +beforeEach(() => { + // A stray DATABASE_URL in the developer's shell would silently turn the + // offline tests into online ones. + vi.stubEnv('DATABASE_URL', '') + pgMock.connectionStrings.length = 0 + rewriteMock.spy.mockImplementation(rewriteMock.real) +}) +afterEach(() => { + vi.unstubAllEnvs() + vi.clearAllMocks() +}) + +/** drizzle-kit's journal, with one entry per tag at the given `when`. */ +function writeJournal( + outDir: string, + entries: { tag: string; when: number }[], +): void { + mkdirSync(join(outDir, 'meta'), { recursive: true }) + writeFileSync( + join(outDir, 'meta', '_journal.json'), + JSON.stringify({ + version: '7', + dialect: 'postgresql', + entries: entries.map(({ tag, when }, idx) => ({ + idx, + version: '7', + when, + tag, + breakpoints: true, + })), + }), + ) +} + +const BROKEN_ALTER = + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n' + +/** + * The corpus a real project has when drizzle-kit emits the broken ALTER: an + * earlier migration declaring the plaintext column (the sweep is fail-closed + * and rewrites nothing it cannot see declared), then the ALTER itself. + */ +function writeBrokenCorpus( + outDir: string, + opts: { declaredWhen?: number; brokenWhen?: number } = {}, +): string { + mkdirSync(outDir, { recursive: true }) + writeFileSync( + join(outDir, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + const broken = join(outDir, '0001_encrypt-email.sql') + writeFileSync(broken, BROKEN_ALTER) + writeJournal(outDir, [ + { tag: '0000_declare', when: opts.declaredWhen ?? 1_000 }, + { tag: '0001_encrypt-email', when: opts.brokenWhen ?? 2_000 }, + ]) + return broken +} + +describe('eqlRepairCommand — target selection', () => { + it('exits 1 with an actionable message when no target is given', async () => { + await expect(eqlRepairCommand({})).rejects.toBeInstanceOf(CliExit) + expect(clack.log.error).toHaveBeenCalledWith(messages.eql.repairNeedsTarget) + }) +}) + +describe('eqlRepairCommand — Drizzle', () => { + let tmp: string + beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), 'stash-eql-repair-')) + }) + afterEach(() => { + rmSync(tmp, { recursive: true, force: true }) + }) + + // Fail closed rather than treat "nothing there" as "nothing to repair": a + // mistyped --out (or a drizzle.config.ts writing elsewhere) would otherwise + // report a clean sweep over a directory the command never looked at. + it('exits 1 naming --out when the output directory does not exist', async () => { + const out = join(tmp, 'nope') + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.error).toHaveBeenCalledWith(expect.stringContaining(out)) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('--out'), + ) + }) + + // The journal is the only offline record of which migrations exist and when + // they were generated — without it the applied-state check has no input at + // all, so repairing would be guessing. Refuse rather than sweep blind. + it('exits 1 when meta/_journal.json is missing', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + writeFileSync(join(out, '0000_x.sql'), 'SELECT 1;\n') + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('meta/_journal.json'), + ) + }) + + // Same refusal for a journal that is present but unusable — truncated by a + // botched merge, or carrying no `entries` array. Both leave the applied-state + // check with no input, and neither should surface as a raw SyntaxError. + it.each([ + ['unparseable JSON', '{ "entries": ['], + ['no entries array', '{ "version": "7" }'], + ])('exits 1 when the journal is malformed (%s)', async (_label, body) => { + const out = join(tmp, 'drizzle') + mkdirSync(join(out, 'meta'), { recursive: true }) + writeFileSync(join(out, 'meta', '_journal.json'), body) + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('meta/_journal.json'), + ) + }) + + // The reason the command exists: repair a broken migration without having to + // generate a redundant EQL install migration to trigger the sweep. + it('rewrites an unapplied broken ALTER COLUMN and reports the file', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).resolves.toBeUndefined() + + const rewritten = readFileSync(broken, 'utf-8') + expect(rewritten).toContain( + 'ALTER TABLE "users" ADD COLUMN "email_encrypted" "public"."eql_v3_text_search";', + ) + expect(rewritten).not.toContain('SET DATA TYPE') + // Add-only: the source column survives for the staged backfill. + expect(rewritten).not.toMatch(/\b(?:DROP|RENAME)\s+COLUMN\b/i) + + const info = clack.log.info.mock.calls.map((c) => String(c[0])) + expect(info.some((msg) => msg.includes('Rewrote 1 migration file'))).toBe( + true, + ) + expect(clack.log.error).not.toHaveBeenCalled() + }) + + // A clean directory must say so out loud. Silence reads as "the command did + // not run", and sends the user back to `eql migration --drizzle` — the + // redundant-install-migration workaround this command exists to remove. + it('reports success and changes nothing when there is nothing to repair', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + const clean = join(out, '0000_clean.sql') + const contents = 'CREATE TABLE "users" ("email" text);\n' + writeFileSync(clean, contents) + writeJournal(out, [{ tag: '0000_clean', when: 1_000 }]) + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).resolves.toBeUndefined() + + expect(readFileSync(clean, 'utf-8')).toBe(contents) + expect(clack.log.success).toHaveBeenCalledWith( + messages.eql.repairNothingToDo, + ) + expect(clack.log.error).not.toHaveBeenCalled() + }) + + // Same fail-closed posture as `eql migration --drizzle`: a statement the + // sweep could not rewrite still fails at migrate time, so exiting 0 would + // tell CI the repair had succeeded. + it('exits 1 and reports the guidance when the sweep leaves a statement behind', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + // Hand-authored `SET DATA TYPE ... USING`: outside the strict matcher, but + // the broad scan flags it as a near-miss. + const nearMiss = join(out, '0000_nearmiss.sql') + writeFileSync( + nearMiss, + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE eql_v3_text_search USING (email)::eql_v3_text_search;\n', + ) + writeJournal(out, [{ tag: '0000_nearmiss', when: 1_000 }]) + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + + expect(readFileSync(nearMiss, 'utf-8')).toContain('SET DATA TYPE') + const stepped = clack.log.step.mock.calls.map((c) => String(c[0])) + expect( + stepped.some((msg) => msg.includes('falls outside the strict matcher')), + ).toBe(true) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('unsafe or unverified SQL'), + ) + }) + + // --dry-run must be a true preview: the whole point is inspecting the repair + // before letting it touch migrations that are about to be applied. + it('writes nothing under --dry-run but names the file it would rewrite', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + + await expect( + eqlRepairCommand({ drizzle: true, out, dryRun: true }), + ).resolves.toBeUndefined() + + expect(readFileSync(broken, 'utf-8')).toBe(BROKEN_ALTER) + const info = clack.log.info.mock.calls.map((c) => String(c[0])) + expect(info.some((msg) => msg.includes('Would rewrite 1 migration'))).toBe( + true, + ) + const stepped = clack.log.step.mock.calls.map((c) => String(c[0])) + expect(stepped.some((msg) => msg.includes(broken))).toBe(true) + }) + + /** + * A sweep that throws part way through has already written some rewrites, so + * the user needs the same partial-work report `eql migration --drizzle` gives + * (#786) — not an unhandled rejection. Both commands render it through the + * same helper. + */ + it('reports a sweep that threw, including the work it had already done', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + writeJournal(out, [{ tag: '0000_x', when: 1_000 }]) + rewriteMock.spy.mockRejectedValueOnce( + Object.assign(new Error('EACCES: permission denied'), { + rewritten: [join(out, '0000_x.sql')], + skipped: [], + staged: [], + }), + ) + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + + const info = clack.log.info.mock.calls.map((c) => String(c[0])).join('\n') + expect(info).toContain('before the sweep stopped') + expect(clack.log.warn).toHaveBeenCalledWith( + expect.stringContaining('Could not sweep'), + ) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('unsafe or unverified SQL'), + ) + }) + + // `describeStagedReconciliation` is written in the past tense — "the database + // now has" — because a real sweep has already written the twin. Under + // --dry-run nothing was written, so printing it verbatim would send the user + // reconciling a schema against a column that does not exist. + it('does not claim a twin exists under --dry-run', async () => { + const out = join(tmp, 'drizzle') + writeBrokenCorpus(out) + + await eqlRepairCommand({ drizzle: true, out, dryRun: true }) + + const warnings = clack.log.warn.mock.calls + .map((c) => String(c[0])) + .join('\n') + expect(warnings).not.toContain('the database now has') + const info = clack.log.info.mock.calls.map((c) => String(c[0])).join('\n') + expect(info).toContain(messages.eql.repairDryRunStaged(1)) + }) +}) + +/** + * The capability `eql migration --drizzle` cannot offer, and the reason this + * command exists as its own entry point. + * + * A matching statement is un-runnable by construction, so the migration almost + * always failed on apply and is safe to rewrite. The exception is a `jsonb` + * column changed to a v3 domain on an EMPTY table: the base types are + * compatible and the envelope CHECK has nothing to reject, so it applies. Once + * it has, rewriting the .sql leaves it describing a shape the original database + * never got from it — a fresh CI or staging database replaying the rewritten + * file diverges from dev, silently. + */ +describe('eqlRepairCommand — applied migrations', () => { + let tmp: string + beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), 'stash-eql-repair-')) + }) + afterEach(() => { + rmSync(tmp, { recursive: true, force: true }) + }) + + it('refuses to rewrite a migration the database has already applied', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out, { brokenWhen: 2_000 }) + // Drizzle's applied-check is `folderMillis <= max(created_at)`, so a + // watermark at the broken migration's own `when` means it ran. + mockLatestAppliedMillis(2_000) + + await expect( + eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }), + ).rejects.toBeInstanceOf(CliExit) + + // Untouched on disk — the whole point. + expect(readFileSync(broken, 'utf-8')).toBe(BROKEN_ALTER) + + const reported = [ + ...clack.log.warn.mock.calls, + ...clack.log.error.mock.calls, + ...clack.log.step.mock.calls, + ] + .map((c) => String(c[0])) + .join('\n') + // A distinct outcome, not a silent skip: names the file, says it is + // applied, names the hazard, and points at the staged lifecycle. + expect(reported).toContain(broken) + expect(reported.toLowerCase()).toContain('already applied') + expect(reported).toContain('stash encrypt') + expect(reported).toContain(messages.eql.repairAppliedHazard) + }) + + // The discriminating half. Without this, an implementation that refused every + // migration whenever a database was reachable would pass the test above — and + // would be useless in the flow the command exists for. + it('still repairs a migration generated after the applied watermark', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out, { + declaredWhen: 1_000, + brokenWhen: 2_000, + }) + // Only the declaring migration ran. + mockLatestAppliedMillis(1_000) + + await expect( + eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }), + ).resolves.toBeUndefined() + + expect(readFileSync(broken, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + }) + + /** + * The offline default, stated as a test so it cannot drift: proceed, but say + * plainly that applied state was NOT verified. + * + * Refusing without a URL would be the fail-closed reading, and it is wrong + * here — the overwhelmingly common case is a broken migration that could not + * have applied, in a project whose database may not be reachable from where + * the repair runs, so a refusal would make the command useless in its own + * intended flow. The warning names the one genuinely unsafe case and how to + * get the check. + */ + it('warns that applied state is unverified without a database URL, and repairs anyway', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).resolves.toBeUndefined() + + expect(readFileSync(broken, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + expect(clack.log.warn).toHaveBeenCalledWith( + messages.eql.repairAppliedUnverified, + ) + // No connection attempt at all — the command stays offline. + expect(pgMock.connect).not.toHaveBeenCalled() + }) + + // DATABASE_URL is the documented second tier of the URL resolver, so the + // check must run off it too — a user with it exported gets the safety without + // repeating it on the command line. + it('runs the applied check from DATABASE_URL when no flag is passed', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out, { brokenWhen: 2_000 }) + mockLatestAppliedMillis(2_000) + vi.stubEnv('DATABASE_URL', 'postgres://user:pw@localhost:5432/from-env') + + await expect( + eqlRepairCommand({ drizzle: true, out }), + ).rejects.toBeInstanceOf(CliExit) + + expect(readFileSync(broken, 'utf-8')).toBe(BROKEN_ALTER) + expect(pgMock.connectionStrings).toEqual([ + 'postgres://user:pw@localhost:5432/from-env', + ]) + }) + + // Pins the mechanism, not just the outcome: drizzle decides on the timestamp + // watermark in its own ledger. A hash comparison would be modelling something + // drizzle does not do (`pg-core/dialect.js:62` writes the hash and never + // reads it), and would re-run migrations drizzle considers done. + it('reads the watermark from drizzle.__drizzle_migrations', async () => { + const out = join(tmp, 'drizzle') + writeBrokenCorpus(out) + mockLatestAppliedMillis(1_000) + + await eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }) + + const sql = String(pgMock.query.mock.calls[0]?.[0]) + expect(sql).toContain('drizzle.__drizzle_migrations') + expect(sql).toContain('created_at') + expect(sql).not.toContain('hash') + }) + + // An empty (or absent-but-readable) ledger means nothing has run yet, so + // everything is repairable. Reported rather than silent, because "no rows" + // and "check did not run" must not look the same to the user. + it('treats an empty ledger as nothing applied', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + mockLatestAppliedMillis(null) + + await expect( + eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }), + ).resolves.toBeUndefined() + + expect(readFileSync(broken, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + expect(clack.log.info).toHaveBeenCalledWith( + messages.eql.repairNothingApplied, + ) + }) + + // `drizzle.__drizzle_migrations` only exists once `drizzle-kit migrate` has + // run. Its absence is an ANSWER — nothing applied — not a failed check. + it.each([ + ['undefined_table', '42P01'], + ['invalid_schema_name', '3F000'], + ])('treats a missing ledger (%s) as nothing applied', async (_l, code) => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + pgMock.query.mockRejectedValue( + Object.assign(new Error('relation does not exist'), { code }), + ) + + await expect( + eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }), + ).resolves.toBeUndefined() + + expect(readFileSync(broken, 'utf-8')).toContain( + 'ADD COLUMN "email_encrypted"', + ) + expect(clack.log.info).toHaveBeenCalledWith( + messages.eql.repairNothingApplied, + ) + }) + + /** + * The asymmetry that matters: passing --database-url is a request to be + * protected from rewriting applied migrations. If the probe cannot answer, + * carrying on as if nothing were applied would hand the user the exact drift + * they asked to avoid, having told them the check was on. + */ + it('exits 1 without rewriting anything when the applied check itself fails', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + pgMock.connect.mockRejectedValue( + Object.assign(new Error('connection refused'), { code: 'ECONNREFUSED' }), + ) + + await expect( + eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }), + ).rejects.toBeInstanceOf(CliExit) + + expect(readFileSync(broken, 'utf-8')).toBe(BROKEN_ALTER) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('connection refused'), + ) + // Every path out of the command closes the banner it opened. + expect(clack.outro).toHaveBeenCalledWith('Repair incomplete.') + }) +}) diff --git a/packages/cli/src/commands/eql/applied.ts b/packages/cli/src/commands/eql/applied.ts new file mode 100644 index 000000000..08953c191 --- /dev/null +++ b/packages/cli/src/commands/eql/applied.ts @@ -0,0 +1,72 @@ +/** + * drizzle's own migration ledger: schema `drizzle`, table + * `__drizzle_migrations`, one row per applied migration carrying `hash` and + * `created_at`. These are drizzle-kit's defaults (`migrationsSchema` / + * `migrationsTable`); a project that overrides them is out of scope for the + * check, which then reports "nothing applied" and is handled like the offline + * case. + */ +const LATEST_APPLIED_SQL = + 'select max(created_at) as max_created_at from drizzle.__drizzle_migrations' + +/** Postgres SQLSTATEs meaning "the ledger isn't there", i.e. nothing applied. */ +const NO_LEDGER = new Set([ + '42P01', // undefined_table + '3F000', // invalid_schema_name +]) + +/** The ledger is absent — `drizzle-kit migrate` has never run against this DB. */ +export const NOTHING_APPLIED = Symbol('nothing-applied') + +/** + * The `created_at` of the most recently applied migration, in epoch + * milliseconds, or {@link NOTHING_APPLIED} when the ledger does not exist or is + * empty. + * + * **Why a single watermark rather than a per-migration lookup.** drizzle's + * applied-check is timestamp-based, not hash-based: `drizzle-orm@0.45.2` + * `pg-core/dialect.js:62` is `if (!lastDbMigration || Number( + * lastDbMigration.created_at) < migration.folderMillis)`, with the hash written + * on insert at `:67` and never read for the decision, and `folderMillis` coming + * from the journal's `when` (`migrator.js:22`). `drizzle-kit migrate` delegates + * to that same code for every Postgres driver. So a migration is applied iff + * its `when` is at or below this watermark, and comparing hashes would be + * modelling a mechanism drizzle does not have. + * + * `pg` is imported lazily: repair is an offline command in every run that does + * not pass a database URL, and it should not pay for the driver to find that + * out. + */ +export async function latestAppliedMillis( + databaseUrl: string, +): Promise { + const { default: pg } = await import('pg') + const client = new pg.Client({ connectionString: databaseUrl }) + try { + await client.connect() + const result = await client.query<{ max_created_at: string | null }>( + LATEST_APPLIED_SQL, + ) + // `created_at` is a bigint, which node-postgres returns as a string to + // avoid a lossy Number conversion. The values are epoch milliseconds, far + // inside Number.MAX_SAFE_INTEGER, so converting here is safe. + const raw = result.rows[0]?.max_created_at + if (raw === null || raw === undefined) return NOTHING_APPLIED + return Number(raw) + } catch (error) { + // A missing ledger is an answer, not a failure: `drizzle-kit migrate` has + // never run here, so nothing is applied. Every other error (auth, network, + // permissions) is a check that did NOT happen, and must not be quietly + // downgraded to "all clear" — it propagates to the caller. + if ( + typeof error === 'object' && + error !== null && + NO_LEDGER.has(String((error as { code?: unknown }).code)) + ) { + return NOTHING_APPLIED + } + throw error + } finally { + await client.end() + } +} diff --git a/packages/cli/src/commands/eql/journal.ts b/packages/cli/src/commands/eql/journal.ts new file mode 100644 index 000000000..8d8f5f1d8 --- /dev/null +++ b/packages/cli/src/commands/eql/journal.ts @@ -0,0 +1,74 @@ +import { readFile } from 'node:fs/promises' +import { join } from 'node:path' + +/** + * One migration as drizzle-kit records it in `meta/_journal.json`. + * + * Only the two fields the applied-state check needs are modelled. `when` is the + * generation timestamp in epoch milliseconds — drizzle names it `folderMillis` + * once it reaches the migrator, and it is the value written into + * `drizzle.__drizzle_migrations.created_at` on a successful apply. + */ +export interface JournalEntry { + /** Migration tag, e.g. `0001_encrypt-email`. The `.sql` file is `.sql`. */ + tag: string + /** Generation timestamp in epoch milliseconds (`folderMillis`). */ + when: number +} + +/** Thrown when the journal is absent or unreadable as a drizzle journal. */ +export class JournalError extends Error {} + +/** `/meta/_journal.json` — drizzle-kit's fixed location. */ +export function journalPath(outDir: string): string { + return join(outDir, 'meta', '_journal.json') +} + +/** + * Read drizzle-kit's migration journal. + * + * Throws a {@link JournalError} when the file is missing, unparseable, or does + * not carry an `entries` array. Every one of those means the caller cannot tell + * which migrations exist or when they were generated, and a repair that cannot + * answer that must refuse rather than guess. + */ +export async function readJournal(outDir: string): Promise { + const path = journalPath(outDir) + let raw: string + try { + raw = await readFile(path, 'utf-8') + } catch (error) { + throw new JournalError( + `Could not read ${path}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + + let parsed: unknown + try { + parsed = JSON.parse(raw) + } catch (error) { + throw new JournalError( + `Could not parse ${path}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + + if ( + typeof parsed !== 'object' || + parsed === null || + !Array.isArray((parsed as { entries?: unknown }).entries) + ) { + throw new JournalError(`${path} has no "entries" array.`) + } + + const entries: JournalEntry[] = [] + for (const entry of (parsed as { entries: unknown[] }).entries) { + if (typeof entry !== 'object' || entry === null) continue + const { tag, when } = entry as { tag?: unknown; when?: unknown } + // A malformed individual entry is not a malformed journal: skip it rather + // than abort, so one bad row cannot block repairing the rest. It simply + // never matches a file, and an unmatched file is treated as unapplied. + if (typeof tag !== 'string' || typeof when !== 'number') continue + entries.push({ tag, when }) + } + return entries +} diff --git a/packages/cli/src/commands/eql/migration.ts b/packages/cli/src/commands/eql/migration.ts index e7df41439..1a003c4b6 100644 --- a/packages/cli/src/commands/eql/migration.ts +++ b/packages/cli/src/commands/eql/migration.ts @@ -6,13 +6,11 @@ import { MIGRATIONS_SCHEMA_SQL } from '@cipherstash/migrate' import * as p from '@clack/prompts' import { CliExit } from '@/cli/exit.js' import { printNextSteps, SAFE_MIGRATION_NAME } from '@/commands/db/install.js' +import { rewriteEncryptedAlterColumns } from '@/commands/db/rewrite-migrations.js' import { - describeSkipReason, - describeStagedReconciliation, - isPartialRewriteResult, - type PartialRewriteResult, - rewriteEncryptedAlterColumns, -} from '@/commands/db/rewrite-migrations.js' + reportSweepFailure, + reportSweepResult, +} from '@/commands/eql/sweep-report.js' import { detectPackageManager, execArgv, @@ -267,64 +265,13 @@ async function generateDrizzleEqlMigration( // it again at the closing note (below) — not just inline here. let sweepIncomplete = false try { - const { rewritten, skipped, staged } = await rewriteEncryptedAlterColumns( - outDir, - { skip: migrationPath }, + sweepIncomplete = reportSweepResult( + await rewriteEncryptedAlterColumns(outDir, { skip: migrationPath }), ) - if (rewritten.length > 0) { - p.log.info( - `Rewrote ${rewritten.length} migration file(s) to add staged encrypted columns while preserving the source columns:`, - ) - for (const file of rewritten) p.log.step(` - ${file}`) - } - // The rewrite repaired SQL only, so schema.ts and the drizzle-kit snapshot - // now disagree with the database — and `drizzle-kit generate` cannot see it - // (#836, item 2). Warn, rather than exit non-zero: the swept SQL is valid - // and additive, and the reconciliation is the user's editorial call. - if (staged.length > 0) { - p.log.warn(describeStagedReconciliation(staged).join('\n')) - } - if (skipped.length > 0) { - sweepIncomplete = true - p.log.warn( - `Found ${skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, - ) - for (const { file, statement, reason } of skipped) { - p.log.step(` - ${file}: ${statement}`) - p.log.step(` ${describeSkipReason(reason)}`) - } - } } catch (error) { // Advisory: the install migration itself is already written and valid. sweepIncomplete = true - const partial: PartialRewriteResult = isPartialRewriteResult(error) - ? error - : {} - if (partial.rewritten && partial.rewritten.length > 0) { - p.log.info( - `Rewrote ${partial.rewritten.length} migration file(s) before the sweep stopped:`, - ) - for (const file of partial.rewritten) p.log.step(` - ${file}`) - } - // A partial sweep still staged real twins, so the same three-way divergence - // already exists for them. - if (partial.staged && partial.staged.length > 0) { - p.log.warn(describeStagedReconciliation(partial.staged).join('\n')) - } - if (partial.skipped && partial.skipped.length > 0) { - p.log.warn( - `Found ${partial.skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, - ) - for (const { file, statement, reason } of partial.skipped) { - p.log.step(` - ${file}: ${statement}`) - p.log.step(` ${describeSkipReason(reason)}`) - } - } - p.log.warn( - `Could not sweep ${outDir} for unsafe ALTER COLUMN statements: ${ - error instanceof Error ? error.message : String(error) - }`, - ) + reportSweepFailure(outDir, error) } if (sweepIncomplete) { diff --git a/packages/cli/src/commands/eql/repair.ts b/packages/cli/src/commands/eql/repair.ts new file mode 100644 index 000000000..564c51e1d --- /dev/null +++ b/packages/cli/src/commands/eql/repair.ts @@ -0,0 +1,211 @@ +import { existsSync } from 'node:fs' +import { join, resolve } from 'node:path' +import * as p from '@clack/prompts' +import { CliExit } from '@/cli/exit.js' +import { + type RewriteResult, + rewriteEncryptedAlterColumns, +} from '@/commands/db/rewrite-migrations.js' +import { latestAppliedMillis, NOTHING_APPLIED } from '@/commands/eql/applied.js' +import { + type JournalEntry, + JournalError, + readJournal, +} from '@/commands/eql/journal.js' +import { + reportSweepFailure, + reportSweepResult, +} from '@/commands/eql/sweep-report.js' +import { resolveDatabaseUrl } from '@/config/database-url.js' +import { messages } from '@/messages.js' + +/** Same default as `eql migration --drizzle`, and as drizzle-kit's own `out`. */ +const DEFAULT_DRIZZLE_OUT = 'drizzle' + +export interface EqlRepairOptions { + /** Repair a Drizzle output directory. Required — mirrors `eql migration`. */ + drizzle?: boolean + /** Directory holding the migrations. Defaults to `drizzle`. */ + out?: string + /** Report what would be rewritten without writing anything. */ + dryRun?: boolean + /** + * Database to check applied state against. Without it (and without + * `DATABASE_URL`) the repair proceeds, warning that it could not verify. + */ + databaseUrl?: string +} + +/** + * `stash eql repair` — sweep an existing migration directory for the un-runnable + * in-place `ALTER COLUMN ... SET DATA TYPE ` statements + * drizzle-kit emits, and rewrite them into a staged encrypted-column addition. + */ +export async function eqlRepairCommand( + options: EqlRepairOptions, +): Promise { + if (!options.drizzle) { + p.log.error(messages.eql.repairNeedsTarget) + throw new CliExit(1) + } + + const outDir = resolve(options.out ?? DEFAULT_DRIZZLE_OUT) + + // Fail closed on a directory that isn't there. `rewriteEncryptedAlterColumns` + // treats ENOENT as an empty sweep, which is right for a generate-time sweep + // but wrong here: a repair that reports "nothing to repair" for a mistyped + // --out sends the user to `drizzle-kit migrate` with the broken SQL intact. + if (!existsSync(outDir)) { + p.log.error(messages.eql.repairOutMissing(outDir)) + throw new CliExit(1) + } + + let journal: JournalEntry[] + try { + journal = await readJournal(outDir) + } catch (error) { + if (error instanceof JournalError) { + p.log.error(messages.eql.repairJournalUnreadable(error.message)) + throw new CliExit(1) + } + throw error + } + + p.intro('CipherStash EQL repair') + + const dryRun = options.dryRun ?? false + const applied = await appliedFiles(outDir, journal, options.databaseUrl) + + let refused: string[] + let result: RewriteResult + let sweepIncomplete: boolean + try { + refused = await refusedAppliedFiles(outDir, applied) + result = await rewriteEncryptedAlterColumns(outDir, { + dryRun, + skip: [...applied], + }) + sweepIncomplete = reportSweepResult(result, { dryRun }) + } catch (error) { + // Unlike `eql migration`, where the sweep is advisory work after a + // migration was already written, here it IS the command — so report the + // partial work (#786) and fail, rather than surfacing a raw stack trace. + reportSweepFailure(outDir, error) + p.log.error(messages.eql.repairSweepIncomplete(outDir)) + p.outro('Repair incomplete.') + throw new CliExit(1) + } + + if (refused.length > 0) { + p.log.warn(messages.eql.repairAppliedRefused(refused.length)) + for (const file of refused) p.log.step(` - ${file}`) + p.log.step(` ${messages.eql.repairAppliedHazard}`) + } + + if (sweepIncomplete || refused.length > 0) { + p.log.error(messages.eql.repairSweepIncomplete(outDir)) + p.outro('Repair incomplete.') + throw new CliExit(1) + } + if (result.rewritten.length === 0) { + p.log.success(messages.eql.repairNothingToDo) + } + p.outro(dryRun ? 'Dry run complete.' : 'Done!') +} + +/** + * The applied migrations that carry an ALTER-to-encrypted statement — the ones + * to report and refuse. + * + * Answered with a second, WRITE-FREE sweep over the whole directory rather than + * a private matcher: "does this file carry a statement the repair would act on" + * is exactly the question the rewriter answers, and a hand-rolled regex here + * would be a second definition of it, free to drift. The real sweep that follows + * holds these files back, so it cannot answer this itself. + */ +async function refusedAppliedFiles( + outDir: string, + applied: ReadonlySet, +): Promise { + // The intersection is empty by construction when nothing is applied — which + // is every offline run — so don't pay for the extra pass. + if (applied.size === 0) return [] + const preview = await rewriteEncryptedAlterColumns(outDir, { dryRun: true }) + return [ + ...new Set([ + ...preview.rewritten, + ...preview.skipped.map(({ file }) => file), + ]), + ] + .filter((file) => applied.has(file)) + .sort() +} + +/** + * The migration files this database has already run, as absolute paths. + * + * Offline (no database URL resolvable) the answer is "none": see + * {@link resolveRepairDatabaseUrl} for why that is the default rather than a + * refusal. + */ +async function appliedFiles( + outDir: string, + journal: readonly JournalEntry[], + databaseUrlFlag: string | undefined, +): Promise> { + const databaseUrl = await resolveRepairDatabaseUrl(databaseUrlFlag) + if (databaseUrl === undefined) { + p.log.warn(messages.eql.repairAppliedUnverified) + return new Set() + } + + let watermark: number | typeof NOTHING_APPLIED + try { + watermark = await latestAppliedMillis(databaseUrl) + } catch (error) { + // Asking for the check and not getting it is a hard failure. Proceeding + // here would rewrite applied migrations while having told the user the + // check was on — the precise drift they passed --database-url to avoid. + p.log.error( + messages.eql.repairAppliedCheckFailed( + error instanceof Error ? error.message : String(error), + ), + ) + p.outro('Repair incomplete.') + throw new CliExit(1) + } + if (watermark === NOTHING_APPLIED) { + p.log.info(messages.eql.repairNothingApplied) + return new Set() + } + + // `folderMillis <= max(created_at)` — drizzle's own applied-check, mirrored. + const applied = new Set( + journal + .filter((entry) => entry.when <= watermark) + .map((entry) => join(outDir, `${entry.tag}.sql`)), + ) + p.log.info(messages.eql.repairAppliedCount(applied.size)) + return applied +} + +/** + * The database URL to check applied state against, or `undefined` to skip the + * check. + * + * Deliberately only the first two tiers of `resolveDatabaseUrl` — the explicit + * flag and `DATABASE_URL` — never the interactive prompt or the hard failure + * below them. The applied-state check is an enhancement, not the command's + * purpose: prompting for a connection string in the middle of an offline repair + * would be surprising, and failing without one would make the command useless + * in the flow it exists for (a broken, never-applied migration in a project + * whose database may not even be reachable). + */ +async function resolveRepairDatabaseUrl( + flag: string | undefined, +): Promise { + if (flag === undefined && !process.env.DATABASE_URL?.trim()) return undefined + // Routed through the shared resolver so a malformed --database-url is + // rejected the same way every other command rejects it. + return await resolveDatabaseUrl({ databaseUrlFlag: flag }) +} diff --git a/packages/cli/src/commands/eql/sweep-report.ts b/packages/cli/src/commands/eql/sweep-report.ts new file mode 100644 index 000000000..4b76c6fb1 --- /dev/null +++ b/packages/cli/src/commands/eql/sweep-report.ts @@ -0,0 +1,105 @@ +import * as p from '@clack/prompts' +import { + describeSkipReason, + describeStagedReconciliation, + isPartialRewriteResult, + type PartialRewriteResult, + type RewriteResult, +} from '@/commands/db/rewrite-migrations.js' +import { messages } from '@/messages.js' + +/** + * Shared rendering for a {@link RewriteResult}. + * + * `stash eql migration --drizzle` and `stash eql repair --drizzle` run the same + * sweep over the same directory, so they must say the same things about it — + * a divergence between the two surfaces is a defect, not a style difference. + * Both call these helpers rather than each formatting the result themselves. + */ + +/** Render the rewritten-file list under `lead`. */ +function reportRewritten(files: readonly string[], lead: string): void { + if (files.length === 0) return + p.log.info(lead) + for (const file of files) p.log.step(` - ${file}`) +} + +/** + * Render the near-misses the sweep left alone, each with the guidance its + * {@link describeSkipReason} carries — the reasons need different action from + * the user, so a single generic line would hide that. + */ +function reportSkipped(skipped: PartialRewriteResult['skipped']): void { + if (!skipped || skipped.length === 0) return + p.log.warn( + `Found ${skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, + ) + for (const { file, statement, reason } of skipped) { + p.log.step(` - ${file}: ${statement}`) + p.log.step(` ${describeSkipReason(reason)}`) + } +} + +/** Render the schema/snapshot reconciliation a staged twin leaves behind. */ +function reportStaged(staged: PartialRewriteResult['staged']): void { + if (!staged || staged.length === 0) return + p.log.warn(describeStagedReconciliation(staged).join('\n')) +} + +/** + * Report a completed sweep. Returns `true` when it left statements behind, + * which every caller turns into a non-zero exit: the remaining SQL still fails + * at migrate time, and a zero exit would tell CI the sweep had succeeded. + */ +export function reportSweepResult( + result: RewriteResult, + options: { dryRun?: boolean } = {}, +): boolean { + reportRewritten( + result.rewritten, + options.dryRun + ? `Would rewrite ${result.rewritten.length} migration file(s) to add staged encrypted columns while preserving the source columns:` + : `Rewrote ${result.rewritten.length} migration file(s) to add staged encrypted columns while preserving the source columns:`, + ) + // The rewrite repaired SQL only, so schema.ts and the drizzle-kit snapshot now + // disagree with the database — and `drizzle-kit generate` cannot see it (#836, + // item 2). A warning rather than a failure: the swept SQL is valid and + // additive, and the reconciliation is the user's editorial call. + // + // Under --dry-run none of that has happened yet. `describeStagedReconciliation` + // is written in the past tense ("the database now has"), so printing it here + // would send the user reconciling against a column no migration adds. Count + // the twins instead, and let the real run print the reconciliation. + if (options.dryRun) { + if (result.staged.length > 0) { + p.log.info(messages.eql.repairDryRunStaged(result.staged.length)) + } + } else { + reportStaged(result.staged) + } + reportSkipped(result.skipped) + return result.skipped.length > 0 +} + +/** + * Report a sweep that threw partway through: the work it had already done + * (#786), then the failure itself. Callers always treat this as incomplete. + */ +export function reportSweepFailure(outDir: string, error: unknown): void { + const partial: PartialRewriteResult = isPartialRewriteResult(error) + ? error + : {} + reportRewritten( + partial.rewritten ?? [], + `Rewrote ${partial.rewritten?.length ?? 0} migration file(s) before the sweep stopped:`, + ) + // A partial sweep still staged real twins, so the same three-way divergence + // already exists for them. + reportStaged(partial.staged) + reportSkipped(partial.skipped) + p.log.warn( + `Could not sweep ${outDir} for unsafe ALTER COLUMN statements: ${ + error instanceof Error ? error.message : String(error) + }`, + ) +} diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index d3b2f9c73..26ba4b941 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -84,6 +84,76 @@ export const messages = { /** `--name` carried characters outside `[A-Za-z0-9_-]`. */ migrationBadName: 'Migration name must contain only letters, numbers, dashes, and underscores.', + /** `stash eql repair` with no `--drizzle` target. */ + repairNeedsTarget: 'Specify a target: `stash eql repair --drizzle`.', + /** `--out` (or its `drizzle` default) points at a directory that isn't there. */ + repairOutMissing: (outDir: string) => + `Drizzle output directory not found: ${outDir}\nPass --out so it matches your drizzle.config.ts.`, + /** + * The drizzle journal is missing or unparseable. `detail` names the file and + * the underlying reason. Fail closed: the journal is the only offline record + * of which migrations exist and when each was generated, so without it the + * applied-state check cannot run and a rewrite would be blind. + */ + /** + * The sweep left statements it could not rewrite. Fail closed, exactly as + * `eql migration --drizzle` does: the remaining SQL still fails at migrate + * time, so a zero exit would tell CI the repair had succeeded. + */ + repairSweepIncomplete: (outDir: string) => + `The ALTER COLUMN sweep found unsafe or unverified SQL in ${outDir}. Review the statements above and use the staged stash encrypt lifecycle before running drizzle-kit migrate.`, + /** + * The dry-run stand-in for `describeStagedReconciliation`, which is written + * in the past tense and would otherwise claim a column exists that no + * migration has added yet. + */ + repairDryRunStaged: (count: number) => + `Would stage ${count} encrypted column(s). Re-run without --dry-run to apply the repair — the reconciliation your Drizzle schema and drizzle-kit snapshot then need is printed at that point.`, + /** + * Lead line for migrations left alone because the database has already run + * them. A distinct outcome from the sweep's `skipped` near-misses: those + * are statements the rewriter could not understand, this is one it + * understood perfectly and must not act on. + */ + repairAppliedRefused: (count: number) => + `Left ${count} ALREADY APPLIED migration(s) untouched. They carry ALTER-to-encrypted statements, but rewriting an applied migration is not a repair:`, + /** + * Why an applied migration is refused, and what to do instead. Named + * separately so the e2e/unit assertions pin the hazard, not the phrasing + * around it. + */ + repairAppliedHazard: + 'the database already has whatever this migration did, so rewriting its .sql leaves the file describing a shape that database never got from it — a fresh CI or staging database replaying the rewritten file would silently diverge from this one. Reconcile the environments by hand instead, and move the column onto the encrypted twin through the staged `stash encrypt` lifecycle.', + /** + * The applied-state probe could not run. Fail closed: the user asked to be + * protected from rewriting applied migrations, and a silent fallback to + * "nothing is applied" would hand them the drift they were avoiding. + */ + repairAppliedCheckFailed: (detail: string) => + `Could not check which migrations have been applied: ${detail}\nNothing was rewritten. Fix the connection and re-run, or re-run without --database-url to repair unverified (see the warning that prints in that mode).`, + /** Nothing in `drizzle.__drizzle_migrations` — every migration is fair game. */ + repairNothingApplied: + 'No applied migrations found in drizzle.__drizzle_migrations — every migration in this directory can be repaired.', + /** How many of the journal's migrations the database has already run. */ + repairAppliedCount: (count: number) => + `${count} migration(s) already applied to this database; they will not be rewritten.`, + /** + * No database URL, so the applied-state check did not run. + * + * The default is to proceed. The journal proves a migration EXISTS, not + * that it ran, and refusing everything on that ambiguity would make the + * command useless in exactly the flow it exists for — a broken migration + * that failed on apply, in a project whose database may not be reachable + * from where the repair is being run. So: warn loudly, name the one case + * that is genuinely unsafe, and tell the user how to get the check. + */ + repairAppliedUnverified: + 'Could not verify which migrations have been applied: no --database-url and no DATABASE_URL. The journal shows that a migration EXISTS, not that it ran. Almost every ALTER-to-encrypted statement is un-runnable and so cannot have been applied — the exception is a jsonb column changed to an EQL domain on an empty table, which applies successfully. If you have run drizzle-kit migrate since generating these migrations, re-run with --database-url so applied migrations are left alone.', + /** A clean sweep — said out loud so silence never reads as "did not run". */ + repairNothingToDo: + 'Nothing to repair: no unsafe ALTER-to-encrypted statements found.', + repairJournalUnreadable: (detail: string) => + `${detail}\nstash eql repair needs drizzle-kit's meta/_journal.json to tell which migrations have already been applied. Check that --out points at your drizzle-kit output directory.`, }, db: { unknownSubcommand: 'Unknown db subcommand', diff --git a/packages/cli/tests/e2e/command-help.e2e.test.ts b/packages/cli/tests/e2e/command-help.e2e.test.ts index 411dcf3e9..1dc33110a 100644 --- a/packages/cli/tests/e2e/command-help.e2e.test.ts +++ b/packages/cli/tests/e2e/command-help.e2e.test.ts @@ -18,6 +18,7 @@ describe('per-command --help', () => { expect(r.output).toContain('Usage: npx stash eql [options]') expect(r.output).toContain('eql install') expect(r.output).toContain('eql migration') + expect(r.output).toContain('eql repair') expect(r.output).toContain('eql upgrade') expect(r.output).toContain('eql status') // A group listing must NOT be the global banner. @@ -45,6 +46,17 @@ describe('per-command --help', () => { expect(r.output).toContain('Also settable via DATABASE_URL.') }) + it('renders full command help for `eql repair --help`', async () => { + const r = await run(['eql', 'repair', '--help'], { + env: { npm_config_user_agent: '' }, + }) + expect(r.exitCode).toBe(0) + expect(r.output).toContain('Usage: npx stash eql repair [options]') + expect(r.output).toContain('--drizzle') + expect(r.output).toContain('--dry-run') + expect(r.output).toContain('--database-url') + }) + it('honours the `-h` short flag after a command', async () => { const r = await run(['eql', 'install', '-h'], { env: { npm_config_user_agent: '' }, diff --git a/packages/cli/tests/e2e/eql-repair.e2e.test.ts b/packages/cli/tests/e2e/eql-repair.e2e.test.ts new file mode 100644 index 000000000..50d0dbb27 --- /dev/null +++ b/packages/cli/tests/e2e/eql-repair.e2e.test.ts @@ -0,0 +1,75 @@ +import { + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { messages } from '../../src/messages.js' +import { run } from '../helpers/run.js' + +/** + * `stash eql repair --drizzle` end to end: argv parsing, dispatch, the sweep, + * and the exit code — the wiring the unit tests deliberately do not cover + * (they call `eqlRepairCommand` directly). + * + * The applied-state check is not exercised here: it needs a live Postgres, and + * its logic has unit coverage against a faked driver. What matters at this + * level is that the command exists, routes, repairs, and exits correctly. + */ +describe('stash eql repair', () => { + let tmp: string + beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), 'stash-repair-e2e-')) + }) + afterEach(() => { + rmSync(tmp, { recursive: true, force: true }) + }) + + it('exits 1 with the target message when --drizzle is missing', async () => { + const r = await run(['eql', 'repair'], { cwd: tmp }) + expect(r.exitCode).toBe(1) + expect(r.output).toContain(messages.eql.repairNeedsTarget) + }) + + it('rewrites a broken ALTER COLUMN in the --out directory', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(join(out, 'meta'), { recursive: true }) + writeFileSync( + join(out, '0000_declare.sql'), + 'CREATE TABLE "users" ("email" text);\n', + ) + writeFileSync( + join(out, '0001_encrypt-email.sql'), + 'ALTER TABLE "users" ALTER COLUMN "email" SET DATA TYPE "undefined"."eql_v3_text_search";\n', + ) + writeFileSync( + join(out, 'meta', '_journal.json'), + JSON.stringify({ + version: '7', + dialect: 'postgresql', + entries: [ + { idx: 0, version: '7', when: 1, tag: '0000_declare' }, + { idx: 1, version: '7', when: 2, tag: '0001_encrypt-email' }, + ], + }), + ) + + const r = await run(['eql', 'repair', '--drizzle', '--out', out], { + cwd: tmp, + }) + + expect(r.exitCode).toBe(0) + // Framed like every other top-level command, so the output reads as one + // run rather than loose log lines. + expect(r.output).toContain('CipherStash EQL repair') + const rewritten = readFileSync(join(out, '0001_encrypt-email.sql'), 'utf-8') + expect(rewritten).toContain( + 'ADD COLUMN "email_encrypted" "public"."eql_v3_text_search"', + ) + expect(rewritten).not.toContain('SET DATA TYPE') + }) +}) diff --git a/packages/cli/tests/e2e/smoke.e2e.test.ts b/packages/cli/tests/e2e/smoke.e2e.test.ts index febcf3b56..72a007d4d 100644 --- a/packages/cli/tests/e2e/smoke.e2e.test.ts +++ b/packages/cli/tests/e2e/smoke.e2e.test.ts @@ -24,6 +24,7 @@ describe('stash CLI — non-interactive smoke', () => { expect(r.output).toContain('init') expect(r.output).toContain('eql install') expect(r.output).toContain('eql migration') + expect(r.output).toContain('eql repair') expect(r.output).toContain('eql upgrade') expect(r.output).toContain('eql status') // The dotenv "injected env" banner regression guard lives in the diff --git a/packages/wizard/src/lib/rewrite-migrations.ts b/packages/wizard/src/lib/rewrite-migrations.ts index 4d8e673aa..0a152f3be 100644 --- a/packages/wizard/src/lib/rewrite-migrations.ts +++ b/packages/wizard/src/lib/rewrite-migrations.ts @@ -1046,10 +1046,17 @@ interface RewriteSweepError extends Error, PartialRewriteResult {} * shipping broken SQL. Statements sitting inside a * SQL comment — or inside a single-quoted string literal, where they are data * rather than SQL — are inert and are neither rewritten nor reported. + * + * `options.skip` names files to leave on disk — they are still READ, because a + * column's current type comes from the whole corpus, but never written. One + * path or many. `options.dryRun` computes the identical result without writing + * anything. Neither is used by this package's own sweep (which rewrites every + * candidate directory in place); they exist because the CLI's `eql repair` + * needs them, and the two copies of this file are kept in sync. */ export async function rewriteEncryptedAlterColumns( outDir: string, - options: { skip?: string } = {}, + options: { skip?: string | readonly string[]; dryRun?: boolean } = {}, ): Promise { const entries = await readdir(outDir).catch( (error: NodeJS.ErrnoException) => { @@ -1065,6 +1072,9 @@ export async function rewriteEncryptedAlterColumns( const staged: StagedColumn[] = [] const seen = new Set() const stagedTargets = new Set() + const skipFiles = new Set( + typeof options.skip === 'string' ? [options.skip] : (options.skip ?? []), + ) /** Record a skip once — the strict pass and the broad scan can both find it. */ const skip = (file: string, statement: string, reason: SkipReason): void => { @@ -1092,7 +1102,7 @@ export async function rewriteEncryptedAlterColumns( try { for (const [filePath, original] of contents) { - if (options.skip && filePath === options.skip) continue + if (skipFiles.has(filePath)) continue // Reset the regex's lastIndex — it's stateful on /g ALTER_COLUMN_TO_ENCRYPTED_RE.lastIndex = 0 @@ -1173,7 +1183,7 @@ export async function rewriteEncryptedAlterColumns( ) if (updated !== original) { - await writeFile(filePath, updated, 'utf-8') + if (!options.dryRun) await writeFile(filePath, updated, 'utf-8') rewritten.push(filePath) staged.push(...fileStaged) } diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 0b8e3f567..0436ce719 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -1,6 +1,6 @@ --- name: stash-cli -description: Drive CipherStash setup and encryption migrations through the `stash` CLI — `init`, `plan`, `impl`, `status`, `auth login`, `eql install/upgrade/status`, `db validate`, `encrypt backfill/drop`, `schema build`, and `manifest --json`. Covers the agent / non-interactive interface, credential rules, and the staged EQL v3 rollout lifecycle. +description: Drive CipherStash setup and encryption migrations through the `stash` CLI — `init`, `plan`, `impl`, `status`, `auth login`, `eql install/migration/repair/upgrade/status`, `db validate`, `encrypt backfill/drop`, `schema build`, and `manifest --json`. Covers the agent / non-interactive interface, credential rules, and the staged EQL v3 rollout lifecycle. --- # CipherStash CLI (`stash`) @@ -335,6 +335,7 @@ Flags below are the decision-relevant ones. Run `stash --help` for the ```bash stash eql install stash eql migration --drizzle +stash eql repair --drizzle stash eql upgrade stash eql status ``` @@ -382,6 +383,32 @@ The sweep is fail-closed: it rewrites a statement only when the same directory a **After a successful sweep, reconcile your ORM schema.** The rewrite repairs SQL only, so the database ends up with both `email` (unchanged, still plaintext) and `email_encrypted`, while your `schema.ts` and drizzle-kit's `meta/*_snapshot.json` both still declare `email` as the encrypted domain and know nothing about the twin. `drizzle-kit generate` will **not** warn you: it diffs your schema against its snapshot, reads neither the `.sql` nor the database, and those two still agree. The command prints the divergence per column, naming the table, both columns and the domain. Until you reconcile it, reads of the source column hand plaintext to a decrypt path expecting an EQL envelope, writes store an EQL envelope in a plaintext column and *succeed*, and the new encrypted column is unreachable through the ORM. See `skills/stash-drizzle` for the reconciliation, including the step that is easy to miss: because the snapshot has never seen the encrypted twin, `drizzle-kit generate` **always** emits an `ADD COLUMN` for it, and the swept migration already adds that column — so the regenerated statement has to be deleted (or made `IF NOT EXISTS`) or migrate fails with `column already exists`. +The sweep runs *after* the install migration is written, so it only sees migrations that already exist. The broken `ALTER COLUMN` is normally generated later, by your next `drizzle-kit generate` — use `eql repair` for that, rather than regenerating an install migration you do not need. + +#### `eql repair` + +Runs the same sweep as `eql migration --drizzle`, standalone: it repairs an existing migration directory without generating anything. This is the command for the usual failure — `drizzle-kit generate` emitted an in-place `ALTER COLUMN … SET DATA TYPE ` after EQL was installed, and `drizzle-kit migrate` fails on it. + +```bash +stash eql repair --drizzle # sweep drizzle/ +stash eql repair --drizzle --dry-run # preview; writes nothing +stash eql repair --drizzle --out db/migrations \ + --database-url postgres://… # skip already-applied migrations +``` + +| Flag | Description | +|---|---| +| `--drizzle` | Required. Repair a Drizzle migration directory. | +| `--out ` | Directory to sweep. Default `drizzle`; set it to match your `drizzle.config.ts`. | +| `--dry-run` | Report what would be rewritten without writing anything. | +| `--database-url ` | Check which migrations the database has already applied, and leave those alone. Also read from `DATABASE_URL`. | + +What it rewrites, what it refuses, and the schema reconciliation afterwards are identical to the `eql migration --drizzle` sweep above — both call the same rewriter and print the same report. It exits non-zero if any statement is left unrepaired. + +**Already-applied migrations are refused, not repaired.** Nearly every ALTER-to-encrypted statement is un-runnable, so its migration failed and is safe to rewrite. The exception is a `jsonb` column changed to an EQL domain on an empty table: base types are compatible, so it applies. Rewriting that migration afterwards leaves its `.sql` describing a shape the database never got from it, and a fresh CI or staging database replaying the rewritten file diverges from the one you developed on — silently, since nothing records that they should differ. With `--database-url`, repair reads `drizzle.__drizzle_migrations` and treats a migration as applied when its journal `when` is at or below the latest `created_at` — the same timestamp comparison `drizzle-kit migrate` itself makes (hashes are written but never compared). Applied migrations are listed and left untouched, and the command exits non-zero. + +Without a database URL the repair proceeds and warns that applied state could not be verified: the journal proves a migration exists, not that it ran. Pass `--database-url` whenever you have run `drizzle-kit migrate` since generating the migrations. If the check is requested but cannot run (connection refused, bad credentials), nothing is rewritten and the command exits non-zero rather than silently repairing unverified. + #### `eql upgrade` The install SQL is safe to re-run — columns and data survive — but it cascade-drops functional indexes that depend on `eql_v3`; recreate them afterward. `upgrade` is v3-only and accepts `--supabase`, `--dry-run`, and `--database-url`. diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index 779aa8f0a..4b35955f7 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -61,7 +61,18 @@ stash eql migration --drizzle --supabase # also grants eql_v3 to anon/authenti The generated migration also installs the `cs_migrations` tracking schema, so a single `drizzle-kit migrate` covers everything `stash encrypt …` needs — no out-of-band `stash eql install`. EQL v3 ships one SQL bundle for every target including Supabase; `--supabase` only adds the PostgREST/RLS role grants (harmless when you connect directly as `postgres`). Requires `drizzle-kit` installed and configured. -**Changing an existing plaintext column to an encrypted one.** `drizzle-kit generate` emits an in-place `ALTER TABLE … ALTER COLUMN … SET DATA TYPE eql_v3_`, which Postgres rejects — there is no cast from `text`/`numeric` to an EQL domain. (On drizzle-kit 0.31.0 and later the emitted type is also mangled to `"undefined"."eql_v3_"`, since a `customType` has no `typeSchema`.) The `stash eql migration --drizzle` sweep repairs the invalid statement — the `stash-cli` skill covers what it rewrites. The repair is **add-only**: it adds a staged `_encrypted` column and leaves the source column in place, so it never emits `DROP COLUMN` or `RENAME COLUMN` and is safe to apply on a populated table. It repairs only what it can prove — the swept directory must also contain the migration that declared the column, so the sweep can see the source type. A statement it cannot place, one whose column is already encrypted, one whose encrypted twin already exists, or one outside the strict matcher (a hand-authored `SET DATA TYPE … USING …`) is left untouched, and the command exits non-zero so you review the directory before running `drizzle-kit migrate`. Applying the swept migration only *adds* the column — encrypting the data is the staged flow in **Migrating an Existing Column to Encrypted** below. +**Changing an existing plaintext column to an encrypted one.** `drizzle-kit generate` emits an in-place `ALTER TABLE … ALTER COLUMN … SET DATA TYPE eql_v3_`, which Postgres rejects — there is no cast from `text`/`numeric` to an EQL domain. (On drizzle-kit 0.31.0 and later the emitted type is also mangled to `"undefined"."eql_v3_"`, since a `customType` has no `typeSchema`.) Repair it with: + +```bash +stash eql repair --drizzle # sweep drizzle/, rewrite the invalid statements +stash eql repair --drizzle --dry-run # preview first; writes nothing +``` + +The repair is **add-only**: it adds a staged `_encrypted` column and leaves the source column in place, so it never emits `DROP COLUMN` or `RENAME COLUMN` and is safe to apply on a populated table. It repairs only what it can prove — the swept directory must also contain the migration that declared the column, so the sweep can see the source type. A statement it cannot place, one whose column is already encrypted, one whose encrypted twin already exists, or one outside the strict matcher (a hand-authored `SET DATA TYPE … USING …`) is left untouched, and the command exits non-zero so you review the directory before running `drizzle-kit migrate`. Applying the swept migration only *adds* the column — encrypting the data is the staged flow in **Migrating an Existing Column to Encrypted** below. + +`stash eql migration --drizzle` runs the identical sweep over its output directory, but only sees migrations that already exist when it runs — and this broken `ALTER COLUMN` is normally generated afterwards. Reach for `eql repair`; there is no reason to generate a second EQL install migration to trigger a repair. + +**If you have already run `drizzle-kit migrate`, pass `--database-url`.** Repair then reads `drizzle.__drizzle_migrations` and leaves applied migrations alone, listing them and exiting non-zero. Rewriting a migration the database has already run would leave its `.sql` describing a shape that database never got from it, so a fresh CI or staging database replaying the file diverges from yours — silently. This is rare but real: an ALTER to an EQL domain is normally un-runnable, so its migration failed and is safe to rewrite, but a `jsonb` column changed to an EQL domain on an **empty** table applies successfully. Without a URL (and without `DATABASE_URL`) the repair proceeds and warns that it could not verify applied state — the journal proves a migration exists, not that it ran. **Reconcile your schema after a sweep — `drizzle-kit generate` will not tell you to.** The sweep repairs SQL and nothing else, so once you apply the swept migration three artefacts disagree: From 37f58a5dd72f88797bb888f43fbd2b653ab98cef Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 15:28:29 +1000 Subject: [PATCH 2/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/cli/src/messages.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index 26ba4b941..2950bcce8 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -89,12 +89,7 @@ export const messages = { /** `--out` (or its `drizzle` default) points at a directory that isn't there. */ repairOutMissing: (outDir: string) => `Drizzle output directory not found: ${outDir}\nPass --out so it matches your drizzle.config.ts.`, - /** - * The drizzle journal is missing or unparseable. `detail` names the file and - * the underlying reason. Fail closed: the journal is the only offline record - * of which migrations exist and when each was generated, so without it the - * applied-state check cannot run and a rewrite would be blind. - */ + // The drizzle journal is missing or unparseable (see repairJournalUnreadable below). /** * The sweep left statements it could not rewrite. Fail closed, exactly as * `eql migration --drizzle` does: the remaining SQL still fails at migrate From 36c9ee2079007fb0215938ffbc785e11d920759d Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 15:28:37 +1000 Subject: [PATCH 3/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/cli/src/commands/eql/applied.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/commands/eql/applied.ts b/packages/cli/src/commands/eql/applied.ts index 08953c191..8d1f51875 100644 --- a/packages/cli/src/commands/eql/applied.ts +++ b/packages/cli/src/commands/eql/applied.ts @@ -67,6 +67,6 @@ export async function latestAppliedMillis( } throw error } finally { - await client.end() + await client.end().catch(() => undefined) } } From 8818aa25fa1f4026e50e46186b90b5c81491c89d Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 15:38:54 +1000 Subject: [PATCH 4/5] fix(cli): make the pg test double return promises from connect/end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The autofix for the review finding on `latestAppliedMillis` chained `.catch()` off `client.end()` to stop a teardown failure masking the classified error. That is correct against the real driver — pg's `connect`/`end` both return promises — but the test double's `end` was a bare `vi.fn()` returning undefined, so every test exercising the applied check died on "Cannot read properties of undefined (reading 'catch')" and surfaced as `repairAppliedCheckFailed`. Resolve both from the double, matching the driver's real shape. Also move the `repairJournalUnreadable` doc comment onto its member instead of leaving a pointer comment where the orphan was. --- packages/cli/src/commands/eql/__tests__/repair.test.ts | 6 ++++++ packages/cli/src/messages.ts | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/eql/__tests__/repair.test.ts b/packages/cli/src/commands/eql/__tests__/repair.test.ts index fba499f71..7ce4820c7 100644 --- a/packages/cli/src/commands/eql/__tests__/repair.test.ts +++ b/packages/cli/src/commands/eql/__tests__/repair.test.ts @@ -90,6 +90,12 @@ beforeEach(() => { vi.stubEnv('DATABASE_URL', '') pgMock.connectionStrings.length = 0 rewriteMock.spy.mockImplementation(rewriteMock.real) + // `pg`'s Client.connect/end both return promises, and the probe chains off + // end() to swallow a teardown failure. A bare vi.fn() returns undefined, so + // the double has to resolve or that chain throws on a shape the real driver + // never produces. + pgMock.connect.mockResolvedValue(undefined) + pgMock.end.mockResolvedValue(undefined) }) afterEach(() => { vi.unstubAllEnvs() diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index 2950bcce8..d75fc5127 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -89,7 +89,6 @@ export const messages = { /** `--out` (or its `drizzle` default) points at a directory that isn't there. */ repairOutMissing: (outDir: string) => `Drizzle output directory not found: ${outDir}\nPass --out so it matches your drizzle.config.ts.`, - // The drizzle journal is missing or unparseable (see repairJournalUnreadable below). /** * The sweep left statements it could not rewrite. Fail closed, exactly as * `eql migration --drizzle` does: the remaining SQL still fails at migrate @@ -147,6 +146,12 @@ export const messages = { /** A clean sweep — said out loud so silence never reads as "did not run". */ repairNothingToDo: 'Nothing to repair: no unsafe ALTER-to-encrypted statements found.', + /** + * The drizzle journal is missing or unparseable. `detail` names the file and + * the underlying reason. Fail closed: the journal is the only offline record + * of which migrations exist and when each was generated, so without it the + * applied-state check cannot run and a rewrite would be blind. + */ repairJournalUnreadable: (detail: string) => `${detail}\nstash eql repair needs drizzle-kit's meta/_journal.json to tell which migrations have already been applied. Check that --out points at your drizzle-kit output directory.`, }, From 59246a9969bfa88b59258304b341cb6df0b567cd Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Thu, 30 Jul 2026 15:57:40 +1000 Subject: [PATCH 5/5] fix(cli): stop eql repair failing open on a renamed drizzle ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on `stash eql repair --drizzle`, both about the command saying something it could not support. 1. An absent ledger was reported as a verified "nothing applied". The probe queries `drizzle.__drizzle_migrations`, so a project that sets `migrations.table` / `migrations.schema` in drizzle.config.ts got SQLSTATE 42P01, which was classified as an answer — and was then told "every migration in this directory can be repaired" while its database had applied plenty. That is the fail-open case the whole flag exists to prevent, delivered as reassurance. An absent relation and an empty one are now different results. LEDGER_ABSENT is ambiguous (never migrated, or we looked in the wrong place) and reports applied state as UNVERIFIED, naming the relation it tried. NOTHING_APPLIED still means the ledger exists and is empty, which is a real answer and keeps its confident wording. `--migrations-table <[schema.]table>` makes the warning actionable — it is the only way to point the check at a renamed ledger, since the probe cannot discover one. The relation is quoted into the query, and the shape is validated before connecting: a malformed value would otherwise query a relation that cannot exist, and its undefined_table would read back as an absent ledger, silently downgrading the requested check. 2. Applied files reported near-misses under the applied-migration hazard. `refusedAppliedFiles` unioned the preview's rewritten and skipped files, so a statement the sweep would have left alone regardless — an undeclared source column, an existing twin — was explained as "rewriting an applied migration is not a repair", which is not why it was skipped. The user lost the actionable skip reason. The two are now separate outcomes; near-misses render through the shared skip reporter and still exit non-zero. Tests: RED-first for each behaviour. The two superseded tests that pinned "missing ledger = nothing applied" are replaced by ones asserting the unverified report, plus a regression test that an absent ledger still repairs. 946 unit / 97 e2e / 366 wizard / 178 scripts, code:check clean, `eql repair` resolves --migrations-table in `manifest --json`. --- .changeset/eql-repair-command.md | 11 ++ packages/cli/src/bin/main.ts | 1 + packages/cli/src/cli/registry.ts | 11 +- .../src/commands/eql/__tests__/repair.test.ts | 161 +++++++++++++++++- packages/cli/src/commands/eql/applied.ts | 75 ++++++-- packages/cli/src/commands/eql/repair.ts | 92 +++++++--- packages/cli/src/commands/eql/sweep-report.ts | 2 +- packages/cli/src/messages.ts | 19 +++ skills/stash-cli/SKILL.md | 5 + skills/stash-drizzle/SKILL.md | 2 +- 10 files changed, 332 insertions(+), 47 deletions(-) diff --git a/.changeset/eql-repair-command.md b/.changeset/eql-repair-command.md index b1e2a4a1c..a47ddd497 100644 --- a/.changeset/eql-repair-command.md +++ b/.changeset/eql-repair-command.md @@ -37,6 +37,17 @@ as their own outcome, left untouched, and the command exits non-zero. Without a database URL the repair proceeds and warns that applied state could not be verified; if the check is requested but cannot run, nothing is rewritten. +A ledger that is not where the probe looked is reported as **unverified**, not as +"nothing applied" — it means either `drizzle-kit migrate` never ran, or +`drizzle.config.ts` overrode `migrations.table` / `migrations.schema` and the +query went to the wrong relation. `--migrations-table <[schema.]table>` names the +ledger for that case; the value must be a plain (optionally schema-qualified) +identifier and is rejected before connecting otherwise. + +An applied migration whose statement the sweep would have skipped regardless — an +undeclared source column, an already-encrypted one, an existing twin — is +reported with that skip reason rather than as an applied-migration refusal. + `rewriteEncryptedAlterColumns` gained `dryRun`, and its `skip` option now accepts several paths as well as one. The wizard's copy of the rewriter carries the same change so the two stay in sync; its own sweep is unaffected. diff --git a/packages/cli/src/bin/main.ts b/packages/cli/src/bin/main.ts index 73789d494..16f26f639 100644 --- a/packages/cli/src/bin/main.ts +++ b/packages/cli/src/bin/main.ts @@ -272,6 +272,7 @@ async function runEqlCommand( out: values.out, dryRun: flags['dry-run'], databaseUrl: values['database-url'], + migrationsTable: values['migrations-table'], }) break } diff --git a/packages/cli/src/cli/registry.ts b/packages/cli/src/cli/registry.ts index e3549329c..0e9e52a8a 100644 --- a/packages/cli/src/cli/registry.ts +++ b/packages/cli/src/cli/registry.ts @@ -377,7 +377,10 @@ export const registry: CommandGroup[] = [ 'rewriting one would leave its .sql describing a shape that database never got', 'from it, so a fresh CI or staging database replaying the file would silently', 'diverge. Pass --database-url so that check can run; without it the repair', - 'proceeds and warns that applied state could not be verified.', + 'proceeds and warns that applied state could not be verified. If your', + 'drizzle.config.ts overrides `migrations.table` / `migrations.schema`, name', + 'the ledger with --migrations-table — otherwise the check queries the default', + 'relation, finds nothing, and reports applied state as unverified.', ].join('\n'), examples: [ 'eql repair --drizzle', @@ -395,6 +398,12 @@ export const registry: CommandGroup[] = [ description: 'Directory holding the migrations to sweep. Defaults to `drizzle`; set it to match your drizzle.config.ts.', }, + { + name: '--migrations-table', + value: '<[schema.]table>', + description: + "Drizzle's migration ledger, when drizzle.config.ts overrides `migrations.table` / `migrations.schema`. Defaults to `drizzle.__drizzle_migrations`. Only read with --database-url.", + }, DRY_RUN_FLAG, DATABASE_URL_FLAG, ], diff --git a/packages/cli/src/commands/eql/__tests__/repair.test.ts b/packages/cli/src/commands/eql/__tests__/repair.test.ts index 7ce4820c7..a3eda6313 100644 --- a/packages/cli/src/commands/eql/__tests__/repair.test.ts +++ b/packages/cli/src/commands/eql/__tests__/repair.test.ts @@ -10,6 +10,7 @@ import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { CliExit } from '../../../cli/exit.js' import { messages } from '../../../messages.js' +import { describeSkipReason } from '../../db/rewrite-migrations.js' import { eqlRepairCommand } from '../repair.js' // clack is chrome — silence it and spy on the channels the command reports @@ -504,7 +505,9 @@ describe('eqlRepairCommand — applied migrations', () => { }) const sql = String(pgMock.query.mock.calls[0]?.[0]) - expect(sql).toContain('drizzle.__drizzle_migrations') + // Quoted, because the relation is now parameterised by --migrations-table + // and reaches the query as text. + expect(sql).toContain('"drizzle"."__drizzle_migrations"') expect(sql).toContain('created_at') expect(sql).not.toContain('hash') }) @@ -533,18 +536,163 @@ describe('eqlRepairCommand — applied migrations', () => { ) }) - // `drizzle.__drizzle_migrations` only exists once `drizzle-kit migrate` has - // run. Its absence is an ANSWER — nothing applied — not a failed check. + /** + * An ABSENT ledger is ambiguous in a way an empty one is not. It means either + * `drizzle-kit migrate` never ran here, or the project set `migrations.table` + * / `migrations.schema` in drizzle.config.ts and the probe looked in the + * wrong place. The command cannot tell those apart, so it must not report the + * confident "nothing applied" — that is the fail-open case: a project with a + * custom ledger gets told every migration is repairable while its database + * has applied plenty. + */ it.each([ ['undefined_table', '42P01'], ['invalid_schema_name', '3F000'], - ])('treats a missing ledger (%s) as nothing applied', async (_l, code) => { + ])('does not claim nothing is applied when the ledger (%s) is absent', async (_l, code) => { const out = join(tmp, 'drizzle') - const broken = writeBrokenCorpus(out) + writeBrokenCorpus(out) pgMock.query.mockRejectedValue( Object.assign(new Error('relation does not exist'), { code }), ) + await eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }) + + expect(clack.log.info).not.toHaveBeenCalledWith( + messages.eql.repairNothingApplied, + ) + const warned = clack.log.warn.mock.calls.map((c) => String(c[0])).join('\n') + // Names where it looked, and the config key that would move it — a + // warning the user cannot act on is not much better than silence. + expect(warned).toContain('drizzle.__drizzle_migrations') + expect(warned).toContain('--migrations-table') + }) + + /** + * Applied files carry TWO kinds of statement, and they need different words. + * + * One the rewriter would have repaired — refusing it is about the applied + * hazard, and `repairAppliedHazard` is the right explanation. One it would + * have left alone regardless (here `source-unknown`: nothing in the corpus + * declares the column) — its applied-ness is irrelevant, and the actionable + * thing is the skip reason. Reporting the second under the hazard banner + * swaps guidance the user can act on for an explanation that does not apply. + */ + it('gives an applied near-miss its skip guidance, not the rewrite hazard', async () => { + const out = join(tmp, 'drizzle') + mkdirSync(out, { recursive: true }) + // No CREATE TABLE anywhere, so the sweep cannot prove the source type. + const undeclared = join(out, '0000_encrypt-orphan.sql') + writeFileSync( + undeclared, + 'ALTER TABLE "ghosts" ALTER COLUMN "note" SET DATA TYPE "undefined"."eql_v3_text_search";\n', + ) + writeJournal(out, [{ tag: '0000_encrypt-orphan', when: 1_000 }]) + mockLatestAppliedMillis(1_000) + + await expect( + eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + }), + ).rejects.toBeInstanceOf(CliExit) + + const reported = [ + ...clack.log.warn.mock.calls, + ...clack.log.error.mock.calls, + ...clack.log.step.mock.calls, + ] + .map((c) => String(c[0])) + .join('\n') + expect(reported).toContain(describeSkipReason('source-unknown')) + expect(reported).not.toContain(messages.eql.repairAppliedHazard) + }) + + /** + * The actionable half of the absent-ledger warning. Without this the warning + * names a flag that does nothing, and a project with a custom ledger has no + * way to get the check at all. + */ + it('queries the ledger named by --migrations-table', async () => { + const out = join(tmp, 'drizzle') + writeBrokenCorpus(out) + mockLatestAppliedMillis(1_000) + + await eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + migrationsTable: 'audit.applied_migrations', + }) + + const sql = String(pgMock.query.mock.calls[0]?.[0]) + expect(sql).toContain('"audit"."applied_migrations"') + expect(sql).not.toContain('__drizzle_migrations') + }) + + /** + * The relation is the one part of the query built from user text. It is + * quoted, so it cannot break out — but a value that is not a plain + * `[schema.]table` becomes a quoted identifier no database has, and the + * resulting "relation does not exist" would be reported as an ABSENT ledger: + * a typo would silently downgrade the check the flag was passed to get. + * Reject it before connecting instead. + */ + it.each([ + ['a statement terminator', '__drizzle_migrations; drop table users --'], + ['too many parts', 'db.drizzle.__drizzle_migrations'], + ['an empty part', 'drizzle.'], + ['a quoted identifier', '"drizzle"."__drizzle_migrations"'], + ])('rejects a --migrations-table containing %s', async (_label, migrationsTable) => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + + await expect( + eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + migrationsTable, + }), + ).rejects.toBeInstanceOf(CliExit) + + // Never reached the database, and never touched the migration. + expect(pgMock.query).not.toHaveBeenCalled() + expect(readFileSync(broken, 'utf-8')).toBe(BROKEN_ALTER) + }) + + // A bare table name is the common case — drizzle's `migrations.table` without + // a `migrations.schema`. It must resolve via search_path, not be forced into + // the `drizzle` schema. + it('accepts an unqualified --migrations-table', async () => { + const out = join(tmp, 'drizzle') + writeBrokenCorpus(out) + mockLatestAppliedMillis(1_000) + + await eqlRepairCommand({ + drizzle: true, + out, + databaseUrl: 'postgres://user:pw@localhost:5432/app', + migrationsTable: 'my_migrations', + }) + + expect(String(pgMock.query.mock.calls[0]?.[0])).toContain('"my_migrations"') + }) + + // An absent ledger still REPAIRS — the overwhelmingly common cause is that + // drizzle-kit migrate never ran, and refusing there would make the command + // useless in the flow it exists for. Only the claim of certainty is dropped. + it('still repairs when the ledger is absent', async () => { + const out = join(tmp, 'drizzle') + const broken = writeBrokenCorpus(out) + pgMock.query.mockRejectedValue( + Object.assign(new Error('relation does not exist'), { code: '42P01' }), + ) + await expect( eqlRepairCommand({ drizzle: true, @@ -556,9 +704,6 @@ describe('eqlRepairCommand — applied migrations', () => { expect(readFileSync(broken, 'utf-8')).toContain( 'ADD COLUMN "email_encrypted"', ) - expect(clack.log.info).toHaveBeenCalledWith( - messages.eql.repairNothingApplied, - ) }) /** diff --git a/packages/cli/src/commands/eql/applied.ts b/packages/cli/src/commands/eql/applied.ts index 8d1f51875..0cddf729e 100644 --- a/packages/cli/src/commands/eql/applied.ts +++ b/packages/cli/src/commands/eql/applied.ts @@ -1,27 +1,67 @@ /** * drizzle's own migration ledger: schema `drizzle`, table * `__drizzle_migrations`, one row per applied migration carrying `hash` and - * `created_at`. These are drizzle-kit's defaults (`migrationsSchema` / - * `migrationsTable`); a project that overrides them is out of scope for the - * check, which then reports "nothing applied" and is handled like the offline - * case. + * `created_at`. These are drizzle-kit's defaults (`migrations.schema` / + * `migrations.table` in drizzle.config.ts); a project that overrides them must + * say so, because the probe cannot discover it — see {@link LEDGER_ABSENT}. */ -const LATEST_APPLIED_SQL = - 'select max(created_at) as max_created_at from drizzle.__drizzle_migrations' +export const DEFAULT_MIGRATIONS_RELATION = 'drizzle.__drizzle_migrations' -/** Postgres SQLSTATEs meaning "the ledger isn't there", i.e. nothing applied. */ +/** Postgres SQLSTATEs meaning "the relation isn't there". */ const NO_LEDGER = new Set([ '42P01', // undefined_table '3F000', // invalid_schema_name ]) -/** The ledger is absent — `drizzle-kit migrate` has never run against this DB. */ +/** The ledger exists and is empty — nothing has been applied. A real answer. */ export const NOTHING_APPLIED = Symbol('nothing-applied') +/** + * The ledger is not where we looked. NOT the same answer as {@link + * NOTHING_APPLIED}: it means either `drizzle-kit migrate` never ran here, or + * the project overrode `migrations.table` / `migrations.schema` and the probe + * queried the wrong relation. Those are indistinguishable from the SQLSTATE, so + * the caller must report the state as unverified rather than claim nothing has + * been applied — the second case would otherwise rewrite applied migrations + * while telling the user they were safe. + */ +export const LEDGER_ABSENT = Symbol('ledger-absent') + +/** + * Is `relation` a plain `table` or `schema.table` of unquoted identifiers? + * + * The relation is quoted before it reaches SQL, so this is not what stops + * injection — {@link quoteRelation} does. It stops something quieter: a value + * that is not a plain identifier pair becomes a quoted relation no database + * has, whose `undefined_table` would be read as {@link LEDGER_ABSENT}. A typo + * would then silently downgrade the very check the caller asked for. + */ +export function isValidRelation(relation: string): boolean { + const parts = relation.split('.') + if (parts.length < 1 || parts.length > 2) return false + return parts.every((part) => /^[A-Za-z_][A-Za-z0-9_$]*$/.test(part)) +} + +/** + * Render `[schema.]table` as quoted SQL identifiers. + * + * The relation reaches the query as text, so it is quoted rather than + * interpolated bare: each dot-separated part is wrapped in double quotes with + * any embedded quote doubled, which is injection-safe by construction. The + * caller validates the shape separately, so a malformed value is rejected with + * a useful message instead of becoming a quoted identifier that cannot exist. + */ +function quoteRelation(relation: string): string { + return relation + .split('.') + .map((part) => `"${part.replace(/"/g, '""')}"`) + .join('.') +} + /** * The `created_at` of the most recently applied migration, in epoch - * milliseconds, or {@link NOTHING_APPLIED} when the ledger does not exist or is - * empty. + * milliseconds; {@link NOTHING_APPLIED} when the ledger exists and is empty; or + * {@link LEDGER_ABSENT} when it is not where we looked. * * **Why a single watermark rather than a per-migration lookup.** drizzle's * applied-check is timestamp-based, not hash-based: `drizzle-orm@0.45.2` @@ -39,13 +79,14 @@ export const NOTHING_APPLIED = Symbol('nothing-applied') */ export async function latestAppliedMillis( databaseUrl: string, -): Promise { + relation: string = DEFAULT_MIGRATIONS_RELATION, +): Promise { const { default: pg } = await import('pg') const client = new pg.Client({ connectionString: databaseUrl }) try { await client.connect() const result = await client.query<{ max_created_at: string | null }>( - LATEST_APPLIED_SQL, + `select max(created_at) as max_created_at from ${quoteRelation(relation)}`, ) // `created_at` is a bigint, which node-postgres returns as a string to // avoid a lossy Number conversion. The values are epoch milliseconds, far @@ -54,16 +95,16 @@ export async function latestAppliedMillis( if (raw === null || raw === undefined) return NOTHING_APPLIED return Number(raw) } catch (error) { - // A missing ledger is an answer, not a failure: `drizzle-kit migrate` has - // never run here, so nothing is applied. Every other error (auth, network, - // permissions) is a check that did NOT happen, and must not be quietly - // downgraded to "all clear" — it propagates to the caller. + // A missing relation is a partial answer — see LEDGER_ABSENT for why it is + // not "nothing applied". Every other error (auth, network, permissions) is + // a check that did NOT happen, and must not be quietly downgraded to "all + // clear" — it propagates to the caller. if ( typeof error === 'object' && error !== null && NO_LEDGER.has(String((error as { code?: unknown }).code)) ) { - return NOTHING_APPLIED + return LEDGER_ABSENT } throw error } finally { diff --git a/packages/cli/src/commands/eql/repair.ts b/packages/cli/src/commands/eql/repair.ts index 564c51e1d..29d8cee25 100644 --- a/packages/cli/src/commands/eql/repair.ts +++ b/packages/cli/src/commands/eql/repair.ts @@ -6,13 +6,20 @@ import { type RewriteResult, rewriteEncryptedAlterColumns, } from '@/commands/db/rewrite-migrations.js' -import { latestAppliedMillis, NOTHING_APPLIED } from '@/commands/eql/applied.js' +import { + DEFAULT_MIGRATIONS_RELATION, + isValidRelation, + LEDGER_ABSENT, + latestAppliedMillis, + NOTHING_APPLIED, +} from '@/commands/eql/applied.js' import { type JournalEntry, JournalError, readJournal, } from '@/commands/eql/journal.js' import { + reportSkipped, reportSweepFailure, reportSweepResult, } from '@/commands/eql/sweep-report.js' @@ -34,6 +41,13 @@ export interface EqlRepairOptions { * `DATABASE_URL`) the repair proceeds, warning that it could not verify. */ databaseUrl?: string + /** + * The drizzle migration ledger to read applied state from, as + * `[schema.]table`. Defaults to drizzle-kit's own + * `drizzle.__drizzle_migrations`; set it to match `migrations.table` / + * `migrations.schema` in drizzle.config.ts when the project overrides them. + */ + migrationsTable?: string } /** @@ -49,6 +63,14 @@ export async function eqlRepairCommand( throw new CliExit(1) } + // Before anything else: a malformed ledger name must not reach the probe, + // where its "relation does not exist" would masquerade as an absent ledger. + const relation = options.migrationsTable ?? DEFAULT_MIGRATIONS_RELATION + if (!isValidRelation(relation)) { + p.log.error(messages.eql.repairMigrationsTableInvalid(relation)) + throw new CliExit(1) + } + const outDir = resolve(options.out ?? DEFAULT_DRIZZLE_OUT) // Fail closed on a directory that isn't there. `rewriteEncryptedAlterColumns` @@ -74,13 +96,18 @@ export async function eqlRepairCommand( p.intro('CipherStash EQL repair') const dryRun = options.dryRun ?? false - const applied = await appliedFiles(outDir, journal, options.databaseUrl) + const applied = await appliedFiles( + outDir, + journal, + options.databaseUrl, + relation, + ) - let refused: string[] + let appliedFindings: AppliedFindings let result: RewriteResult let sweepIncomplete: boolean try { - refused = await refusedAppliedFiles(outDir, applied) + appliedFindings = await refusedAppliedFiles(outDir, applied) result = await rewriteEncryptedAlterColumns(outDir, { dryRun, skip: [...applied], @@ -96,13 +123,17 @@ export async function eqlRepairCommand( throw new CliExit(1) } + const { refused, nearMisses } = appliedFindings if (refused.length > 0) { p.log.warn(messages.eql.repairAppliedRefused(refused.length)) for (const file of refused) p.log.step(` - ${file}`) p.log.step(` ${messages.eql.repairAppliedHazard}`) } + // Held back with the rest of their file, so the real sweep never saw them — + // report them here with the same guidance an unapplied near-miss gets. + reportSkipped(nearMisses) - if (sweepIncomplete || refused.length > 0) { + if (sweepIncomplete || refused.length > 0 || nearMisses.length > 0) { p.log.error(messages.eql.repairSweepIncomplete(outDir)) p.outro('Repair incomplete.') throw new CliExit(1) @@ -114,8 +145,28 @@ export async function eqlRepairCommand( } /** - * The applied migrations that carry an ALTER-to-encrypted statement — the ones - * to report and refuse. + * What an applied migration carries, split by what the rewriter would have done + * with it — because the two need different words from us. + * + * `refused` are files the sweep WOULD have rewritten. Holding them back is the + * applied-migration hazard, and {@link messages.eql.repairAppliedHazard} + * explains it. + * + * `nearMisses` are statements the sweep would have left alone anyway — an + * undeclared source column, an already-encrypted one, an existing twin. Their + * applied-ness is beside the point: the actionable thing is the skip reason, so + * they get the same guidance an unapplied near-miss gets. Reporting them under + * the hazard banner would swap advice the user can act on for an explanation + * that does not apply to them. + */ +interface AppliedFindings { + refused: string[] + nearMisses: RewriteResult['skipped'] +} + +/** + * The applied migrations that carry an ALTER-to-encrypted statement, split into + * {@link AppliedFindings}. * * Answered with a second, WRITE-FREE sweep over the whole directory rather than * a private matcher: "does this file carry a statement the repair would act on" @@ -126,19 +177,15 @@ export async function eqlRepairCommand( async function refusedAppliedFiles( outDir: string, applied: ReadonlySet, -): Promise { +): Promise { // The intersection is empty by construction when nothing is applied — which // is every offline run — so don't pay for the extra pass. - if (applied.size === 0) return [] + if (applied.size === 0) return { refused: [], nearMisses: [] } const preview = await rewriteEncryptedAlterColumns(outDir, { dryRun: true }) - return [ - ...new Set([ - ...preview.rewritten, - ...preview.skipped.map(({ file }) => file), - ]), - ] - .filter((file) => applied.has(file)) - .sort() + return { + refused: preview.rewritten.filter((file) => applied.has(file)).sort(), + nearMisses: preview.skipped.filter(({ file }) => applied.has(file)), + } } /** @@ -152,6 +199,7 @@ async function appliedFiles( outDir: string, journal: readonly JournalEntry[], databaseUrlFlag: string | undefined, + relation: string = DEFAULT_MIGRATIONS_RELATION, ): Promise> { const databaseUrl = await resolveRepairDatabaseUrl(databaseUrlFlag) if (databaseUrl === undefined) { @@ -159,9 +207,9 @@ async function appliedFiles( return new Set() } - let watermark: number | typeof NOTHING_APPLIED + let watermark: number | typeof NOTHING_APPLIED | typeof LEDGER_ABSENT try { - watermark = await latestAppliedMillis(databaseUrl) + watermark = await latestAppliedMillis(databaseUrl, relation) } catch (error) { // Asking for the check and not getting it is a hard failure. Proceeding // here would rewrite applied migrations while having told the user the @@ -174,6 +222,12 @@ async function appliedFiles( p.outro('Repair incomplete.') throw new CliExit(1) } + // Absent relation: ambiguous, so warn rather than claim a clean check. See + // LEDGER_ABSENT. Still proceeds, for the same reason the offline path does. + if (watermark === LEDGER_ABSENT) { + p.log.warn(messages.eql.repairLedgerMissing(relation)) + return new Set() + } if (watermark === NOTHING_APPLIED) { p.log.info(messages.eql.repairNothingApplied) return new Set() diff --git a/packages/cli/src/commands/eql/sweep-report.ts b/packages/cli/src/commands/eql/sweep-report.ts index 4b76c6fb1..5bcc8be4d 100644 --- a/packages/cli/src/commands/eql/sweep-report.ts +++ b/packages/cli/src/commands/eql/sweep-report.ts @@ -29,7 +29,7 @@ function reportRewritten(files: readonly string[], lead: string): void { * {@link describeSkipReason} carries — the reasons need different action from * the user, so a single generic line would hide that. */ -function reportSkipped(skipped: PartialRewriteResult['skipped']): void { +export function reportSkipped(skipped: PartialRewriteResult['skipped']): void { if (!skipped || skipped.length === 0) return p.log.warn( `Found ${skipped.length} ALTER-to-encrypted statement(s) the sweep left alone. Review and fix them before running your migrations:`, diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index d75fc5127..78e1d4a3c 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -125,6 +125,25 @@ export const messages = { */ repairAppliedCheckFailed: (detail: string) => `Could not check which migrations have been applied: ${detail}\nNothing was rewritten. Fix the connection and re-run, or re-run without --database-url to repair unverified (see the warning that prints in that mode).`, + /** + * `--migrations-table` is not a plain `[schema.]table`. Rejected before + * connecting: the value is quoted before it reaches SQL, so it cannot break + * out, but a malformed one would query a relation that cannot exist and the + * resulting `undefined_table` would be read as an absent ledger — silently + * downgrading the check the flag was passed to get. + */ + repairMigrationsTableInvalid: (value: string) => + `--migrations-table must be a table name, optionally schema-qualified (e.g. \`my_migrations\` or \`audit.my_migrations\`). Got: ${value}`, + /** + * The ledger relation is not there. Ambiguous, and deliberately NOT the + * confident `repairNothingApplied`: either `drizzle-kit migrate` never ran + * against this database, or the project overrode `migrations.table` / + * `migrations.schema` in drizzle.config.ts and the probe queried the wrong + * relation. Claiming "nothing applied" for the second case would rewrite + * applied migrations while reporting the check as clean. + */ + repairLedgerMissing: (relation: string) => + `Could not verify which migrations have been applied: ${relation} does not exist. Either drizzle-kit migrate has never run against this database — in which case there is nothing applied and this repair is safe — or your drizzle.config.ts sets migrations.table / migrations.schema, and the check looked in the wrong place. If it does, re-run with --migrations-table <[schema.]table> naming your ledger. Repairing anyway.`, /** Nothing in `drizzle.__drizzle_migrations` — every migration is fair game. */ repairNothingApplied: 'No applied migrations found in drizzle.__drizzle_migrations — every migration in this directory can be repaired.', diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 0436ce719..b086af393 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -402,6 +402,7 @@ stash eql repair --drizzle --out db/migrations \ | `--out ` | Directory to sweep. Default `drizzle`; set it to match your `drizzle.config.ts`. | | `--dry-run` | Report what would be rewritten without writing anything. | | `--database-url ` | Check which migrations the database has already applied, and leave those alone. Also read from `DATABASE_URL`. | +| `--migrations-table <[schema.]table>` | Drizzle's migration ledger, when `drizzle.config.ts` overrides `migrations.table` / `migrations.schema`. Defaults to `drizzle.__drizzle_migrations`. Only read alongside `--database-url`. | What it rewrites, what it refuses, and the schema reconciliation afterwards are identical to the `eql migration --drizzle` sweep above — both call the same rewriter and print the same report. It exits non-zero if any statement is left unrepaired. @@ -409,6 +410,10 @@ What it rewrites, what it refuses, and the schema reconciliation afterwards are Without a database URL the repair proceeds and warns that applied state could not be verified: the journal proves a migration exists, not that it ran. Pass `--database-url` whenever you have run `drizzle-kit migrate` since generating the migrations. If the check is requested but cannot run (connection refused, bad credentials), nothing is rewritten and the command exits non-zero rather than silently repairing unverified. +**If `drizzle.config.ts` overrides `migrations.table` / `migrations.schema`, pass `--migrations-table`.** The probe cannot discover a renamed ledger, and a missing one is ambiguous — it means either `drizzle-kit migrate` never ran here, or the check looked in the wrong place. Repair reports that state as *unverified* rather than as "nothing applied" and says which relation it tried, so a custom-ledger project is never told its applied migrations are safe to rewrite. Naming the ledger turns the check back on. The value must be a plain table name, optionally schema-qualified; anything else is rejected before connecting. + +An applied migration carrying a statement the sweep would have skipped anyway — an undeclared source column, an already-encrypted one, an existing twin — is reported with that skip reason, not as an applied-migration refusal. Its applied-ness is not why it was left alone. + #### `eql upgrade` The install SQL is safe to re-run — columns and data survive — but it cascade-drops functional indexes that depend on `eql_v3`; recreate them afterward. `upgrade` is v3-only and accepts `--supabase`, `--dry-run`, and `--database-url`. diff --git a/skills/stash-drizzle/SKILL.md b/skills/stash-drizzle/SKILL.md index 4b35955f7..8fd4cd6f7 100644 --- a/skills/stash-drizzle/SKILL.md +++ b/skills/stash-drizzle/SKILL.md @@ -72,7 +72,7 @@ The repair is **add-only**: it adds a staged `_encrypted` column and lea `stash eql migration --drizzle` runs the identical sweep over its output directory, but only sees migrations that already exist when it runs — and this broken `ALTER COLUMN` is normally generated afterwards. Reach for `eql repair`; there is no reason to generate a second EQL install migration to trigger a repair. -**If you have already run `drizzle-kit migrate`, pass `--database-url`.** Repair then reads `drizzle.__drizzle_migrations` and leaves applied migrations alone, listing them and exiting non-zero. Rewriting a migration the database has already run would leave its `.sql` describing a shape that database never got from it, so a fresh CI or staging database replaying the file diverges from yours — silently. This is rare but real: an ALTER to an EQL domain is normally un-runnable, so its migration failed and is safe to rewrite, but a `jsonb` column changed to an EQL domain on an **empty** table applies successfully. Without a URL (and without `DATABASE_URL`) the repair proceeds and warns that it could not verify applied state — the journal proves a migration exists, not that it ran. +**If you have already run `drizzle-kit migrate`, pass `--database-url`.** Repair then reads `drizzle.__drizzle_migrations` and leaves applied migrations alone, listing them and exiting non-zero. Rewriting a migration the database has already run would leave its `.sql` describing a shape that database never got from it, so a fresh CI or staging database replaying the file diverges from yours — silently. This is rare but real: an ALTER to an EQL domain is normally un-runnable, so its migration failed and is safe to rewrite, but a `jsonb` column changed to an EQL domain on an **empty** table applies successfully. Without a URL (and without `DATABASE_URL`) the repair proceeds and warns that it could not verify applied state — the journal proves a migration exists, not that it ran. If your `drizzle.config.ts` sets `migrations.table` or `migrations.schema`, add `--migrations-table <[schema.]table>`: the probe cannot discover a renamed ledger, and without it the check finds nothing at the default relation and reports applied state as unverified rather than claiming nothing is applied. **Reconcile your schema after a sweep — `drizzle-kit generate` will not tell you to.** The sweep repairs SQL and nothing else, so once you apply the swept migration three artefacts disagree: