feat(cli): add stash eql migration --supabase so a v3 install survives db reset (#613) - #856
Open
tobyhede wants to merge 5 commits into
Open
feat(cli): add stash eql migration --supabase so a v3 install survives db reset (#613)#856tobyhede wants to merge 5 commits into
stash eql migration --supabase so a v3 install survives db reset (#613)#856tobyhede wants to merge 5 commits into
Conversation
…ves `db reset` (#613) Supabase projects had only `stash eql install --supabase`, which applies the SQL directly to a running database. `supabase db reset` — the ordinary local development loop — drops that database and replays `supabase/migrations/`, so the install was wiped and the next query failed with `type "eql_v3_encrypted" does not exist`. Nothing wrote EQL into the migrations directory. This was a regression, not merely unimplemented: `db/supabase-migration.ts` was a working migration-file writer, v2-only by its own comment, and #825 deleted it under the v2 umbrella (#772). `stash eql migration --supabase` now writes `supabase/migrations/<timestamp>_cipherstash_eql.sql`. The SQL body reuses `buildEqlV3MigrationSql({ supabase: true })` unchanged, so the file carries the v3 bundle, the role grants, and the `cs_migrations` tracking schema — one reset provisions everything `stash encrypt` needs. `--supabase` is a target when it stands alone and stays the grants modifier alongside `--drizzle`; only a bare `--supabase` selects the new emitter. The file is timestamped at generation time rather than carrying the all-zero prefix the retired v2 writer used. A version sorting below the highest applied one is out-of-order to the Supabase CLI, which `db push` skips without `--include-all`. Sorting last costs nothing: the only ordering that matters is EQL before the user's encrypted-column migrations, and those come later. A second run exits rather than adding a duplicate install. `--force` rewrites the existing migration in place, keeping its version — writing a new one would leave the first applied and undeletable (removing an applied migration desyncs `supabase_migrations.schema_migrations`), giving the user two EQL installs. Also fixes the three surfaces that advertised the removed flow: - `init/providers/supabase.ts` told every user to run `eql install --supabase` and then `supabase db reset` — the exact sequence that destroyed the install. - `db/install.ts` pointed every `--migration` user at `eql migration --drizzle`, which shells out to drizzle-kit. - `db/detect.ts` documented `hasMigrationsDir` as feeding a prompt that no longer exists; it now gates init's migration-vs-direct route and says so. `stash init --supabase` generates the migration when the project has local `supabase/` scaffolding, and still installs directly when it does not — a hosted project with no `supabase/` directory has nowhere to write. Two pty e2e assertions in smoke.e2e.test.ts pinned the literal phrase `eql migration --drizzle` inside the removal message, which is the misdirection being removed. Retargeted at both replacements, with an `unwrapped()` helper — clack hard-wraps to the pty's 100 columns, so long phrases were failing on formatting rather than content. Verified: 1003 unit tests, 97 pty e2e tests, `code:check` error-free, and the `eql migration` manifest matches skills/stash-cli. The install was replayed through four simulated resets against a local Postgres (drop, recreate, apply `supabase/migrations/` in order), with a dependent `eql_v3_text_search` column migration proving the ordering — the Supabase CLI itself is not installed here, so run it once against the real thing before merge.
🦋 Changeset detectedLatest commit: 76f55dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe CLI now generates Supabase EQL v3 migration files with grants and tracking-schema setup. It detects existing migrations, supports forced replacement, and updates ChangesSupabase EQL migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EqlMigration
participant SupabaseMigration
participant SupabaseCLI
User->>EqlMigration: run eql migration --supabase
EqlMigration->>SupabaseMigration: generate and write EQL migration
SupabaseMigration-->>EqlMigration: return migration path
EqlMigration-->>User: show application steps
User->>SupabaseCLI: run db reset or db push
SupabaseCLI-->>User: apply migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Five defects, each with a regression test written first. **Re-running `stash init --supabase` failed the whole run.** The second run called `eqlMigrationCommand` with no force, `writeSupabaseEqlMigration` threw "already exists", and `generateEqlMigration`'s catch treated the refusal as a write failure — returning no `eqlMigrationPending`, so `initCommand` printed "✗ EQL extension NOT installed", pointed the user at the direct `stash eql install` this route exists to avoid, and exited 1. Nothing was wrong: the migration was right there. Init now checks `findExistingEqlMigration` first and reports the existing file as pending. Passing `force: true` would also have unblocked it, but silently rewrites a file that may already be applied. **The init summary named the wrong apply command on the headline path.** The branch read `state.integration`, which `detectIntegration` derives from the DATABASE_URL host — and a local Supabase stack is `127.0.0.1:54322`, so integration lands on 'postgresql' while the provider is 'supabase'. `installEqlStep` routes on either signal, so it generated a Supabase migration and the summary then said `drizzle-kit migrate`, contradicting the provider's own next-steps block a few lines later. It now matches on both signals exactly as the step does, with Drizzle winning when both fire. **`--dry-run` did not predict the refusal.** It always reported "would write a new file", including in a directory where the real run exits 1. It now reports the refusal, or the in-place replacement under `--force`. **`findExistingEqlMigration` matched directories.** `readdirSync` returns both, so an entry named `…_cipherstash_eql.sql` became the write target and failed with a raw EISDIR. Filtered to files, mirroring `existsAsDirectory` in detect.ts. **The write was not atomic.** `supabase db reset` executes the migrations directory wholesale, so a truncated file from a failed write is not inert — it runs. Now writes to a dot-prefixed temp sibling and renames, cleaning up on failure. Also from the review: - `--name` is warned about rather than silently ignored on the Supabase path; the filename is load-bearing for duplicate detection. - The spinner no longer repeats the path the success line already reports, and the `--force` warning leads with `db reset` rather than the `eql install` the new guidance steers Supabase users away from. - `applyCmd` no longer smuggles backticks through its value. - Remote apply is `supabase db push`, not a bare `supabase migration up` — that form targets the LOCAL database, so the old wording meant a production database silently never got EQL. Corrected in the skill, README, provider next-steps, setup-prompt, and the command's own note, with a test pinning it. - skills/stash-cli no longer says "pass exactly one of --drizzle / --prisma". - `unwrapped()` in smoke.e2e.test.ts strips clack's `│` gutter, which is inserted at each wrap point — collapsing whitespace alone still left it embedded mid-phrase, the exact failure the helper exists to prevent. - The detect.js mock spreads importOriginal, so detectSupabase / detectDrizzle / detectPrismaNext stay defined. 1015 unit tests (up from 1003) and 97 pty e2e tests pass; `code:check` is error-free. Re-verified against the built CLI: all three dry-run predictions, the --name warning, no temp file left behind, and three more Postgres replay cycles with a dependent eql_v3_text_search migration.
Three findings, each with a regression test written first. **The `already present` init path skipped scaffolding.** `eql migration` writes SQL and nothing else — deliberately — so init supplies the `stash.config.ts` and encryption client every other route gets (#581). The previous commit's early return for an existing Supabase migration returned before any of that. A project whose migration came from a standalone `stash eql migration --supabase` has never had a config written, so init reported "Setup complete" over a project that cannot load one. The scaffolding is now its own function and every migration-first exit runs it, including that one. **A bare `supabase migration up` survived in the drop-plaintext step.** The previous commit corrected five sites and added a callout saying that form targets the LOCAL database, then left `skills/stash-supabase` line 783 presenting it as the remote apply — contradicting the callout in the same shipped file. Corrected to `db reset` local / `db push` remote. The guard is a new test over every `skills/*/SKILL.md`, following the version-pin guard in release-train.test.ts: any `supabase migration up` must carry `--linked` or be qualified as local within 80 characters. A nearby "locally" does not satisfy it — the exact wording being fixed here had one, attached to the other command, which is how a looser first version of this test passed over the bug. `setup-prompt.ts` also named `migration up` in the planning agent's do-not-run list; it now names `supabase db reset`, which is both the command an agent on a local project would reach for and the destructive one worth listing. **The `--name` warning rendered above the intro.** clack draws log lines into the frame the intro opens, so warning before it put the line above the banner, detached from the command. Moved below the intro and still above the dry-run branch, which ignores `--name` too. Verified against the built CLI. 1032 unit tests (up from 1015) and 97 pty e2e tests pass.
…ording The Supabase init provider now emits `eql migration --supabase (writes it into supabase/migrations/)`. The provider's unit test was updated with it; this cross-package copy of the same expectation was not, so all four package-manager cases failed in CI.
tobyhede
marked this pull request as ready for review
August 4, 2026 07:29
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/commands/init/index.ts (1)
151-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize
provider.namebefore provider flag checks.
resolveProviderjoins matched flags with-, so runs like--drizzle --supabaseproduce values such as'drizzle-supabase'. Directprovider.name === 'supabase'checks in init then treat Supabase hints as missing, including the post-migration apply hint,install-eql.tsrouting,build-schema.tsintegration selection (for Prisma), andresolve-database.tsSupabase resolver hint. Move the joined suffix into init/state once or normalize comparisons to the first provider segment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/index.ts` around lines 151 - 158, Normalize the provider value returned by resolveProvider before the provider checks in init, using the first provider segment (before “-”) consistently. Update the init/state provider value or comparison logic so isDrizzle, isSupabase, install-eql.ts routing, build-schema.ts integration selection, and resolve-database.ts Supabase hints recognize combined flags such as “drizzle-supabase” without changing single-provider behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/__tests__/skill-supabase-apply.test.ts`:
- Around line 49-56: Update the validation loop over `prose.matchAll` so the
qualifier only passes when `--linked` or the immediate phrase “applies to the
local database” follows `supabase migration up`; remove the broad 80-character
`local database` containment check while preserving the existing remote-command
failure message.
---
Nitpick comments:
In `@packages/cli/src/commands/init/index.ts`:
- Around line 151-158: Normalize the provider value returned by resolveProvider
before the provider checks in init, using the first provider segment (before
“-”) consistently. Update the init/state provider value or comparison logic so
isDrizzle, isSupabase, install-eql.ts routing, build-schema.ts integration
selection, and resolve-database.ts Supabase hints recognize combined flags such
as “drizzle-supabase” without changing single-provider behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3aa41a8-22ca-4c4f-ab70-f2c32caad442
📒 Files selected for processing (24)
.changeset/supabase-eql-migration-file.mde2e/tests/package-managers.e2e.test.tspackages/cli/README.mdpackages/cli/src/__tests__/skill-supabase-apply.test.tspackages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/db/detect.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/commands/eql/__tests__/supabase-migration.test.tspackages/cli/src/commands/eql/migration.tspackages/cli/src/commands/eql/supabase-migration.tspackages/cli/src/commands/init/__tests__/init-command.test.tspackages/cli/src/commands/init/index.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/init/providers/__tests__/supabase.test.tspackages/cli/src/commands/init/providers/supabase.tspackages/cli/src/commands/init/steps/__tests__/install-eql.test.tspackages/cli/src/commands/init/steps/install-eql.tspackages/cli/src/messages.tspackages/cli/tests/e2e/command-help.e2e.test.tspackages/cli/tests/e2e/smoke.e2e.test.tsskills/stash-cli/SKILL.mdskills/stash-supabase/SKILL.md
The 80-character containment window passed whenever "local database" or "--linked" appeared anywhere nearby — including when attached to a different command, which is the wording the guard exists to reject. Match only what immediately follows `supabase migration up`, as the file's own doc comment already described.
freshtonic
reviewed
Aug 5, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the full diff (comment-only). This is a thorough, careful fix and the reasoning is easy to follow throughout. Some things I specifically checked and liked:
- Atomic write (temp sibling →
rename, dot-prefixed +.tmpso a crash leaves nothing the Supabase CLI replays) — correct, and the failure test exercises it against the real fs rather than a mock. - Suffix-based duplicate detection with the
isFileguard against a directory named…_cipherstash_eql.sql— good catch on the EISDIR path. - Timestamped version so it sorts after applied migrations (vs the retired v2 all-zero prefix) — the
push-without---include-allrationale is sound and covered. - Dry-run predicts the real run's refusal/overwrite, not a blanket "would write".
- The
--supabase-is-target-alone / grants-modifier-with---drizzlesplit is consistent across the command,messages.ts,registry.ts, and both skills, and theinitsummary now routes on both signals (integrationandprovider.name) — the local-Supabase127.0.0.1:54322→integration:'postgresql'case that would otherwise printdrizzle-kit migrateis a real trap and it's nailed with a test.
Test coverage (new supabase-migration.test.ts, the eql migration — Supabase block, install-eql routing, and the shipped-skill supabase migration up guard) is excellent.
A few non-blocking observations:
1. --out on a bare --supabase can silently reintroduce #613. The file is written to the resolved --out, but supabase db reset / db push only replay supabase/migrations/. So eql migration --supabase --out db/migrations produces a file Supabase never applies — exactly the "EQL isn't in the replayed directory" failure this PR exists to fix, just relocated. The registry even showcases eql migration --supabase --out db/migrations --force as an example. Worth confirming Supabase actually supports a non-default migrations dir; if it doesn't, consider warning when the resolved dir isn't supabase/migrations, and dropping it from the example.
2. The "already present" init branch reports eqlMigrationPending: true, and initCommand then prints ○ EQL migration generated even though nothing was generated this run — it was already on disk. The apply guidance is right; only the verb is slightly off. Minor.
3. Pre-existing, now extended to Supabase: the Install the EQL extension into your database now? (required for encryption) confirm precedes a branch that writes a migration file rather than touching the DB. It read a little oddly for Drizzle already and now for Supabase too — optional wording tweak.
4. Verification gap (your own callout). The real supabase db reset / db push path wasn't exercised (no Supabase CLI/Docker on the machine); the throwaway-Postgres reset substitute is a good proxy, but the CLI's version-ordering + --include-all behaviour and the PostgREST role grants are the two things the substitute can't fully stand in for — worth one run against the real CLI before merge, as you noted.
Only #1 is one I'd genuinely consider addressing before merge; the rest are polish. Nice work.
freshtonic
self-requested a review
August 5, 2026 06:00
freshtonic
requested changes
Aug 5, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
See the main review comment. Finding 1 is worth addressing before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #613.
The bug
Supabase projects had only
stash eql install --supabase, which applies the SQL directly to a running database.supabase db reset— the ordinary local development loop — drops that database and replayssupabase/migrations/, so the install was wiped and the next query failed withtype "eql_v3_encrypted" does not exist. Nothing wrote EQL into the migrations directory.This regressed rather than merely being unimplemented.
packages/cli/src/commands/db/supabase-migration.tswas a working migration-file writer — v2-only by its own comment — and #825 deleted it under the v2 umbrella (#772). So "unimplemented for v3" became "removed entirely".Three surfaces still advertised the removed flow, and one of them actively broke databases:
init/providers/supabase.ts:11-12told everystash init --supabaseuser to runeql install --supabase (prompts for migration vs direct)and thensupabase db reset. The install prompts for nothing, and that reset destroys it.db/install.ts:43-45pointed every--migrationuser atstash eql migration --drizzle, which shells out todrizzle-kit— useless to a Supabase project without Drizzle.db/detect.ts:36-38documentedhasMigrationsDiras picking the default in a prompt that no longer exists.What this does
stash eql migration --supabasewritessupabase/migrations/<timestamp>_cipherstash_eql.sql. The SQL body reusesbuildEqlV3MigrationSql({ supabase: true })unchanged — same bundle, same grants, same trailingcs_migrationstracking schema — so onesupabase db resetprovisions everythingstash encryptneeds, with no out-of-band install.eql migration --drizzleeql migration --drizzle --supabaseeql migration --supabasesupabase/migrations/eql migration --prismaprisma-next migrate)--supabaseis a target when it stands alone and stays the grants modifier alongside--drizzle. Only a bare--supabaseselects the new emitter.stash init --supabasenow generates that migration when the project has localsupabase/scaffolding, and still installs directly when it does not — a hosted project with nosupabase/directory has nowhere to write and nosupabasebinary to apply one.Two decisions worth reviewing
Timestamped filename, not the all-zero prefix the retired v2 writer used. A version sorting below the highest applied one is out-of-order to the Supabase CLI:
supabase db pushskips it unless the user knows to pass--include-all. Sorting last costs nothing, because the only ordering that matters is EQL before the user's encrypted-column migrations — and those are written afterwards. Covered by a test.--forceoverwrites in place, keeping the original version. Writing a second, newer-versioned file would leave the first one applied and undeletable (removing an applied migration desyncssupabase_migrations.schema_migrations), so the user would end up with two EQL installs in their history. A--forcerun warns that any database which already applied the file now has the old bundle.Test changes that are not additive
Two pty e2e assertions in
smoke.e2e.test.tspinned the literal phraseeql migration --drizzleinside theeql install --migrationremoval message — the exact misdirection this PR removes, so they had to change. They now assert both replacements, via anunwrapped()helper: clack hard-wraps to the pty's 100 columns, so long assertion phrases were failing on formatting rather than content.Verification
pnpm run code:checkerror-free (192 warnings, unchanged from baseline, none in the new files).stash manifest --jsonforeql migrationmatches whatskills/stash-clidocuments.supabase db resetitself — neither the Supabase CLI nor Docker is installed on this machine. Substitute: a throwaway Postgres 14 with theanon/authenticated/service_roleroles, and a script reproducing what a reset does (drop database, recreate, replaysupabase/migrations/in lexical order). Across four cycles the EQL domains, the schema grants for all three roles, andcipherstash.cs_migrationscame back every time. A second migration declaring apublic.eql_v3_text_searchcolumn and aneql_v3.eq_termindex applied cleanly, confirming the install still sorts ahead of dependent user migrations. Someone should run this once against the real CLI before merge.stash init --supabaseis covered by unit tests only; exercising it end-to-end needs CipherStash credentials.Follow-up outside this repo
The Supabase Fundamentals docs page carries a warning that a v3 install does not survive
supabase db reset. That is obsolete once this ships and needs replacing with the migration flow.Summary by CodeRabbit
New Features
Bug Fixes
Documentation