Skip to content

Drizzle sweep: partial-write-then-throw is reported as unverified, not destructive #786

Description

@tobyhede

The A-2 fail-closed sweep (#783, #785) made the Drizzle migration rewriter refuse to
rewrite anything it cannot prove is plaintext. One path is still fail-open: when the
sweep throws after it has already rewritten at least one file, the wizard reports
that directory as unverified rather than destructive — the user gets no
data-destruction warning while a live DROP COLUMN sits on disk.

What happens

rewriteEncryptedAlterColumns writes files one at a time inside its per-file loop
(packages/wizard/src/lib/rewrite-migrations.ts:672-675):

if (updated !== original) {
  await writeFile(filePath, updated, 'utf-8')
  rewritten.push(filePath)
}

If writeFile (or the read of a later file) throws on the second file after the first
was rewritten, the whole call rejects and the locally accumulated rewritten array is
discarded. sweepMigrationDirs catches and reports zeros
(packages/wizard/src/lib/rewrite-migrations.ts:737-743):

try {
  const { rewritten, skipped } = await rewriteEncryptedAlterColumns(abs)
  results.push({ dir, rewritten, skipped })
} catch (err) {
  const message = err instanceof Error ? err.message : String(err)
  results.push({ dir, rewritten: [], skipped: [], error: message })   // <- partial work lost
}

rewriteEncryptedMigrations then continues past the reporting block entirely on
error !== undefined (packages/wizard/src/lib/post-agent.ts:181-187), so even the
per-file log.step list of what was rewritten never prints. destructive stays
false, unverified becomes true, and the prompt takes the wrong arm
(post-agent.ts:118-126):

Run the migration now? … the sweep could not check 1 directory (drizzle/); review
those migrations before migrating, or you may apply broken/unsafe SQL

instead of the destructive wording. The comment on that arm says the softer wording is
deliberate because "nothing is known about that directory, so claiming it destroys data
would be a guess" — which is right for a directory that threw on the first file, and
wrong for one that threw after writing an ADD+DROP+RENAME.

The CLI copies (packages/cli/src/commands/eql/migration.ts:258,
packages/cli/src/commands/db/install.ts) have the milder form of the same bug: they
set sweepIncomplete and warn, but never name the files they already rewrote, so the
user is told to review the directory without being told which files now hold a live
DROP COLUMN.

Why it matters

This is the exact fail-open inversion the A-2 branch exists to close, in a path that
branch did not touch. The consequence is the worst one in this subsystem: a
data-destroying migration presented to the user under an initialValue-driven prompt
that does not mention data destruction. The wizard runs this unattended across three
candidate directories.

Realistic triggers: a read-only file or directory partway through the sweep, ENOSPC, a
permissions change, an editor or drizzle-kit holding a lock. Not common, but the
failure mode is silent and destructive.

Suggested fix

  1. Have rewriteEncryptedAlterColumns surface the work it already did when it later
    throws — e.g. attach rewritten/skipped to the thrown error, or return a result
    carrying error instead of rejecting.
  2. Have sweepMigrationDirs propagate those partial arrays on the error result rather
    than emptying them.
  3. In post-agent.ts, report a directory that has both an error and rewritten files
    as destructive: print the rewritten-file list and the data-destruction warning
    before the unverified warning, and let destructive win the prompt arm.
  4. Do the same in the CLI callers — on catch, still list any files the sweep reports as
    rewritten.

Tests

Both existing failure tests use the EISDIR-on-read trick, which throws before any write,
so this is invisible today. Add a mid-directory write-failure case: two rewritable files,
the second made unwritable, asserting the first is reported as rewritten and the prompt
takes the destructive arm.


Deferred from #785 with reasons; documented as item 7 in
docs/superpowers/specs/2026-07-25-a2-coverage-followup.md. It is a production
behaviour change rather than test coverage, which is why it was not folded into that PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    SDKbugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions