Drizzle migration rewriter: correctness follow-ups to #823

[tobyhede]

Follow-ups to #823, which made the Drizzle EQL migration rewriter add-only and fail closed. Both items below survive on main (c8b1325a) and are silent: the command exits 0 and reports success while leaving the user in a wrong state. Neither is data loss — the rewrite never emits DROP COLUMN or RENAME COLUMN — but neither is visible either.

Shared file with #837 — sequence this after it. Both issues edit the same paragraph, skills/stash-drizzle/SKILL.md:64. #837 owns deleting the false "data-destroying / safe only on an empty table" claim, which is wrong today and independent of anything decided here. Item 2 below adds post-sweep reconciliation guidance to that same paragraph, and cannot be written until the decision in item 2 is made. Land #837's correction first; editing in the other order silently reverts it.

1. The corpus index is blind to dollar-quoted DDL, so the already-encrypted guard is still bypassed

This is the mechanism #811 reported. #823 closed #811 by removing the blast radius (add-only emission), not by closing the mechanism.

indexColumnDeclarations gates five of its six scans on isInsideCommentOrString, which skips a dollar-quoted body whole:

• packages/cli/src/commands/db/rewrite-migrations.ts:562, 588, 594, 600, 609
• mirrored at packages/wizard/src/lib/rewrite-migrations.ts:602

(The sixth scan, CREATE_TABLE_ENCRYPTED_COLUMN_RE at :583-585, is deliberately ungated; the asymmetry is documented at :575-582.)

An encrypted ADD COLUMN inside DO $$ … END $$; is executed SQL — the column really is encrypted in the database — but it never enters the encrypted set. The column then falls to "plaintext by residue" and the rewrite proceeds.

Result: the sweep adds an empty <column>_encrypted twin while the real ciphertext sits untouched in the source column. rewritten lists the file as handled, skipped is empty, exit code 0, no warning.

#823's own test codifies the wrong outcome — packages/cli/src/__tests__/rewrite-migrations.test.ts:977-998, inside describe('issue #811 dollar-quoted DDL regression'):

992:      const { rewritten } = await rewriteEncryptedAlterColumns(tmpDir)
993:
994:      expect(rewritten).toEqual([change])
995:      const updated = fs.readFileSync(change, 'utf-8')
996:      expect(updated).toContain('ADD COLUMN "email_encrypted"')

The setup at :981-987 declares email as eql_v3_text_search inside DO $$. Wizard parity test at packages/wizard/src/__tests__/rewrite-migrations.test.ts:918-939.

Decision needed: scan dollar-quoted bodies for the index pass only (correct for the index, still skipped for the rewrite pass), or fail closed on any table touched inside a dollar-quoted body. #811 suggested both; neither was implemented.

2. After a sweep, schema.ts, the drizzle-kit snapshot, and the database are three-way divergent

renderSafeAlter emits ALTER TABLE … ADD COLUMN "<col>_encrypted" "public"."<domain>"; and preserves the source column. Nothing reconciles the other two artefacts. Neither rewriter copy reads or writes a snapshot; meta/_journal.json is read only as a drizzle-output-directory detector (packages/wizard/src/lib/rewrite-migrations.ts:874). Both callers — packages/cli/src/commands/eql/migration.ts:291 and packages/wizard/src/lib/post-agent.ts:136 — only report.

So after a sweep:

• the database has email text + email_encrypted eql_v3_text_search
• schema.ts still declares email as the encrypted domain
• meta/*_snapshot.json still records email as the encrypted domain
• neither schema.ts nor the snapshot knows email_encrypted exists

drizzle-kit generate gives no signal. It diffs schema.ts against the snapshot — it never reads .sql and never introspects the database. Both inputs still agree, so the diff is empty and it emits nothing. It cannot propose dropping a column that appears in neither input; that behaviour belongs to drizzle-kit push, which does introspect.

Consequences through the ORM, all silent:

• reads of users.email push plaintext through a customType.fromDriver expecting an EQL envelope
• writes push an EQL envelope into a text column and succeed, storing ciphertext in a plaintext column
• email_encrypted is unreachable — it is in no Drizzle schema

It only fails loudly much later: if the user corrects schema.ts by hand, generate diffs against the stale snapshot and emits a duplicate ADD COLUMN, which errors at migrate time with "column already exists".

Not documented anywhere. skills/stash-cli/SKILL.md:379 describes the rewrite and never mentions reconciling schema.ts. skills/stash-drizzle/SKILL.md:64 links to "Migrating an Existing Column to Encrypted" but gives no post-sweep reconciliation instruction. The rewriter-never-drops-ciphertext changeset caveats database drift only, not artefact drift.

Decision needed: warn after a sweep, write the schema.ts edit, or detect the divergence in stash encrypt plan.

3. The wizard sweep's partial-result read is an unchecked cast, defeating per-directory fail-closed reporting

Moved here from #837: this is a fail-closed correctness defect in the same subsystem, not cleanup.

packages/wizard/src/lib/rewrite-migrations.ts:944-953:

944:    } catch (err) {
945:      const message = err instanceof Error ? err.message : String(err)
946:      const partial = err as Partial<RewriteSweepError>
947:      results.push({
948:        dir,
949:        rewritten: partial.rewritten ?? [],
950:        skipped: partial.skipped ?? [],

For a non-object throw (throw null, throw undefined), the property read at :949 raises a TypeError inside the catch block. That escapes sweepMigrationDirs entirely, so the per-directory error result is never pushed and the fail-closed reporting this catch exists to provide does not happen.

The CLI path was hardened against exactly this — packages/cli/src/commands/eql/migration.ts:42-49 narrows with isPartialRewriteResult, and the rationale is documented at :36-41:

The sweep can also fail with a non-Error throw — a string, null, anything — in which case there is no partial result to report. Narrow rather than cast so those cases fall through to the plain "could not sweep" message instead of crashing on a property read of a non-object.

The wizard has zero occurrences of isPartialRewriteResult.

Practical severity is low — fs/promises does not throw non-Errors — but this is the only unresolved review thread on merged #823 (posted by CodeRabbit at 2026-07-29T07:14:52Z, merged 07:17:15Z, 2m23s later), and an unresolved thread on a merged PR is invisible to triage. It exists nowhere else. Note also that :946 sits inside the #region wizard-only block, so scripts/__tests__/rewriter-copies-in-sync.test.mjs cannot police the CLI/wizard asymmetry either way.