feat(cli): add stash eql migration --supabase so a v3 install survives db reset (#613) - #856
Merged
Merged
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: 1f25971 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 |
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
📝 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 and status
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.
…push guidance Six fixes from the PR review plus a verification pass against the Supabase CLI source (supabase/cli v2.111.0). --out on a bare --supabase silently reintroduced #613. The Supabase CLI's migrations directory is not configurable — `MigrationsDir` is a hard-coded `filepath.Join("supabase", "migrations")` in the Go CLI and a literal `path.join(workdir, "supabase", "migrations")` in both the TS `db reset` and `db push` handlers, with no config.toml key and an open, unanswered request to add one. So `--out db/migrations` wrote a file Supabase would never replay: the exact failure this command exists to fix, relocated. It now warns (comparing resolved paths, and above the dry-run branch so a dry run predicts it), and the registry example that showcased it is gone. init reported `EQL migration generated` over a migration it only found on disk. `eqlMigrationAlreadyPresent` refines `eqlMigrationPending` rather than replacing it, so the verb is honest while the apply guidance and the completeness check are untouched. The confirm prompt asked about installing into the database and then wrote a file, and declining pointed a Supabase user at `stash eql install` — the command that reinstates #613. The route is now resolved as a value before the prompt, so the prompt, the non-interactive notice, and the decline hint all name what will actually happen. The --force warning told users to re-apply remotely with `supabase db push`, which does nothing: `FindPendingMigrations` is positional (`pending := localMigrations[len(remoteMigrations):]`) with no content hash, so a version already in the ledger is never re-run and push reports "up to date" while the remote keeps the old bundle. Replaced with the real recipe (`migration repair --status reverted <version>` then `db push --include-all`), and it now names the `DROP SCHEMA ... CASCADE` at the head of the bundle, which takes dependent indexes and RLS policies with it on a populated database. The timestamp-sorts-last rationale was greenfield-only. A project that ran `stash eql install`, wrote encrypted-column migrations against it, then hit #613 gets an install sorting after migrations that reference eql_v3 — and `db reset` replays those first and fails. `findEqlDependentMigrationsBefore` detects it and warns, naming the files and the remedy. Detection only: back- dating carries an `--include-all` consequence the user has to accept. Also corrected a comment that had the mechanism backwards: an out-of-order version does not cause `db push` to skip the file, it aborts the whole push with ErrMissingRemote before applying anything. That is the stronger argument for the current design. Fixed in the source, the test comment, and skills/stash-cli.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/eql/migration.ts (1)
167-193: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
--prisma --supabasebefore dispatch.Line 167 only rejects
--drizzle --prisma, so--prisma --supabasefalls through to theoptions.supabasebranch and creates a Supabase migration while ignoring--prisma. Add this target-combination rejection, or make an explicit Prisma-specific error the first dispatch outcome.🤖 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/eql/migration.ts` around lines 167 - 193, Update the target validation before dispatch so using options.prisma with options.supabase is rejected, rather than reaching generateSupabaseEqlMigration. Add this combination to the existing mutual-exclusion check or make the Prisma-specific rejection run first, while preserving the current single-target dispatch 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 `@skills/stash-cli/SKILL.md`:
- Line 40: Update the migration exception wording in the `stash init`
documentation to say “Drizzle and local Supabase flows” instead of grouping all
Supabase flows with Drizzle, matching the `resolveMigrationRoute` behavior and
the qualification documented near the Supabase setup instructions.
- Line 390: Update the backdated-install guidance near the migration-ordering
warning to distinguish already-applied installs from unapplied SQL. For remotes
where stash eql install has already run, instruct users to mark the backdated
migration as applied with supabase migration repair --status applied <version>
instead of rerunning the EQL bundle; retain supabase db push --include-all only
when the database still needs the migration SQL applied.
---
Outside diff comments:
In `@packages/cli/src/commands/eql/migration.ts`:
- Around line 167-193: Update the target validation before dispatch so using
options.prisma with options.supabase is rejected, rather than reaching
generateSupabaseEqlMigration. Add this combination to the existing
mutual-exclusion check or make the Prisma-specific rejection run first, while
preserving the current single-target dispatch behavior.
🪄 Autofix
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: 334e7116-0bbd-47fe-b618-ce8b1e3c79d1
📒 Files selected for processing (15)
.changeset/supabase-eql-migration-file.mdpackages/cli/README.mdpackages/cli/src/cli/registry.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/steps/__tests__/install-eql.test.tspackages/cli/src/commands/init/steps/install-eql.tspackages/cli/src/commands/init/types.tspackages/cli/src/messages.tsskills/stash-cli/SKILL.mdskills/stash-supabase/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/cli/src/commands/init/tests/init-command.test.ts
- packages/cli/src/commands/init/index.ts
- .changeset/supabase-eql-migration-file.md
- packages/cli/src/cli/registry.ts
- packages/cli/README.md
- packages/cli/src/commands/init/steps/tests/install-eql.test.ts
`stash eql migration --supabase` warns when a project already has EQL-referencing migrations that sort before the install it is about to write, and told everyone to reach for `supabase db push --include-all`. That warning only ever fires on a project that ran `stash eql install` directly — which is precisely the state where the remote already has the bundle and is missing only the ledger row. Pushing the file there re-runs a bundle opening with `DROP SCHEMA IF EXISTS eql_v3 CASCADE`, taking every dependent index, constraint, and RLS policy with it. The remedy is now split: `supabase migration repair --status applied <version>` for a remote that already has EQL (ledger row only, no SQL), with `--include-all` kept for one that genuinely still needs the SQL applied. Corrected in the runtime message and in the three docs that repeated it — `skills/stash-cli`, `skills/stash-supabase`, and the CLI README. Also in `skills/stash-cli`: the `stash init` overview claimed the Supabase flow always generates an EQL migration. `resolveMigrationRoute` gates that on `hasLocalSupabaseScaffolding()`, so a hosted project with no local `supabase/` directory installs directly — now "local Supabase flows". Adds two regression tests. The brownfield warning pins the split remedy. The second pins something invisible in the source: `--prisma --supabase` is rejected only by branch ordering (the `--prisma` exit sits above the `--supabase` dispatch), not by the `drizzle && prisma` mutual-exclusion check, so a reorder would silently route it into the Supabase emitter. The sibling cases' `expect(spawnMock).not.toHaveBeenCalled()` cannot catch that — the emitter writes files rather than spawning — so it stubs `cwd` at a tmpdir and asserts nothing was written.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@skills/stash-cli/SKILL.md`:
- Line 390: Require remote EQL-state verification before recommending
ledger-only repair: in skills/stash-cli/SKILL.md:390 and
skills/stash-supabase/SKILL.md:117-126, instruct users to confirm the remote EQL
schemas/types are installed before running supabase migration repair --status
applied; use ledger repair only when EQL is present, and direct remotes without
EQL to apply the install SQL via the appropriate include-all push path.
🪄 Autofix
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: df975b14-ffcb-4691-80de-26db6221b8ef
📒 Files selected for processing (6)
.changeset/precise-supabase-init-and-backdated-push-guidance.mdpackages/cli/README.mdpackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/messages.tsskills/stash-cli/SKILL.mdskills/stash-supabase/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/README.md
- packages/cli/src/messages.ts
- packages/cli/src/commands/eql/tests/migration.test.ts
The claims this command rests on were all verified by reading supabase/cli's Go source. That was enough to correct the guidance, and not enough to trust it: the `--force` remote recipe now shipping in two skills had never been run. `db push --db-url` needs neither Docker nor a linked project, so a bare Postgres cluster is enough to drive the real binary. The new suite covers the six things the filesystem tests cannot: the generated install applying with no `--include-all` (which is also the only proof the CLI's statement splitter survives 2.6 MB of dollar-quoted bundle); `anon` reaching `eql_v3` through `SET ROLE`, using the grants carried INSIDE the emitted file rather than the ones `eql install --direct` applies; an out-of-order version aborting the whole push rather than being skipped; a `--force`-replaced file never re-applying; and a leaked `.tmp` file being ignored but reported. It replaces the weakest test in the PR — one that sorted two filenames in a tmpdir and asserted nothing about the CLI. Running it corrected the guidance again. `--include-all` is NOT unconditionally required after `migration repair --status reverted`: reverting the newest version leaves it at the tail of remote history, where a plain `db push` applies it. Only a version with applied migrations above it is the "gap in the middle" that trips ErrMissingRemote — which is the usual shape, since encrypted-column migrations get written after the install, but it is a condition rather than a rule. Recommending the flag unconditionally was its own hazard: it applies every out-of-order migration the user has, not just this one. Corrected in the printed note, both skills, and the README. Gated on STASH_TEST_SUPABASE_DB_URL + STASH_TEST_SUPABASE_CLI, so the default suite is unchanged. Still out of reach: `supabase db reset` (it removes the container and volume, so it needs the full local stack) and the PostgREST HTTP round-trip, which the Docker integration job already covers for the direct installer.
freshtonic
approved these changes
Aug 6, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Re-review — APPROVE
Re-reviewed the full diff at HEAD 88fc1c40 (newer than the last CodeRabbit pass). Verified against a checkout of the branch:
pnpm run code:check— 0 errors (warnings/infos only, which CI allows)pnpm --filter stash test— 1078 passed, 16 skipped (the skipped ones are the env-gated live suites, including the newsupabase-push.live.test.ts)pnpm --filter stash build— success (ESM + DTS)stash manifest --json—eql migrationnow carries--supabaseand the new--force; skill/manifest agreement is guarded byskill-supabase-apply.test.ts
What changed since the last round — prior feedback all addressed
--outon a bare--supabasereintroducing #613 (the previousCHANGES_REQUESTED): fixed —migrationSupabaseOutNotReplayedwarning comparing resolved paths, registry--outdoc + example reworked, and a dedicated test block covering relative/absolute/default/dry-run/--drizzle --supabasecases.--prisma --supabasereaching the Supabase emitter: fixed — the--prismarejection is dispatched before the--supabasebranch, and the ordering is pinned by a test that asserts nothing is written.- Skill/README wording: the remote apply is now
supabase db push(not a baresupabase migration up, which is local), the back-dated-install remedy splits ledger-onlymigration repair --status appliedfromdb push --include-all, and the init overview says "Drizzle and local Supabase flows". Corrected in both skills, the README, and the changesets.
Blocking
None.
Should-fix (non-blocking)
- Combined-flag
provider.nameininit.resolveProviderjoins multiple flags asmatchedKeys.sort().join('-'), sostash init --drizzle --supabaseproducesprovider.name === 'drizzle-supabase', which never equals'drizzle'/'supabase'ininstall-eql.tsorindex.ts. Routing then relies solely onstate.integration; on a local Supabase+Drizzle project (integration === 'postgresql') bothsupabaseanddrizzlego false,resolveMigrationRoutereturns null, and init installs directly with no grants and no migration — the exact #613 failure, but only reachable through an undocumented flag combination. Low impact, cheap to harden (normalize to the first provider segment, or deriveisSupabase/isDrizzlefrommatchedKeys/flags). This is the one CodeRabbit nit still open.
Highlights
- Atomic write (dot-prefixed
.tmpsibling →rename, temp name failing the Supabase CLI filter twice over) exercised against the real fs, not a mock. - Suffix-based duplicate detection with an
isFileguard against the EISDIR path. - Timestamped version rationale (sorts into the pending tail, pushes without
--include-all) is sound and paired with a brownfield "sorts before install" detector + warning. - Dry-run predicts the real outcome — refuse / replace / write — rather than a blanket "would write".
- The guidance strings encode the Supabase CLI's actual Go behaviour and are pinned by a live push suite gated on env, so the default suite is untouched.
Changeset present (stash minor + a stash patch for the skill/README corrections); skills updated in-PR per AGENTS.md. Nice work.
…ger repair
Two findings, both reachable in normal use.
`stash init --drizzle --supabase` is an accepted invocation — parseArgs simply
sets both flags and nothing rejects the pair — but `resolveProvider` joined
matched flags into `provider.name` ('drizzle-supabase') for referrer tracking,
and every consumer compared that string by equality. All of them fell through:
- install-eql: on a local Supabase stack (127.0.0.1:54322 detects as
'postgresql') both signals went false, so init installed EQL directly with
no migration and no role grants — #613 exactly, via a flag pair we accept
- install-deps: integrationPackageFor('drizzle-supabase') returned null, so
NEITHER adapter was installed
- build-schema: `--prisma` with any second flag lost the Prisma Next branch
- resolve-database: no `supabase status`, the one lookup that finds a local
stack's URL
- index: the summary's apply step fell through to the drizzle-kit default
The fix splits the two things that were conflated. `name` stays the referrer,
still joined alphabetically and still what `authenticateStep` hands `login()`.
A new `provider.selected` carries the capability signal, and every step reads
that instead, so the concerns cannot drift back together. PROVIDER_KEYS is
derived from PROVIDER_MAP rather than restated, so a new provider cannot be
half-added. `matchedKeys.sort()` also mutated in place, which would have
reordered `selected` once both named the same array; it now sorts a copy.
Separately, the brownfield warning recommended `supabase migration repair
--status applied <version>` for a remote where `stash eql install` had already
run, on the user's unverified say-so. Marking applied is the one remedy here
with no self-correcting failure: if EQL is not actually present, it writes a
ledger row for SQL that never ran, so EQL is both absent and permanently
recorded as installed, and no later push will ever apply it. The guidance now
requires a check first:
psql "$REMOTE_DATABASE_URL" -Atc "select eql_v3.version()"
`eql_v3.version()` rather than a `pg_namespace` probe because of where each
object sits in the bundle: CREATE SCHEMA eql_v3 is line 43 of 59573, while
version() is the last object created, at 59455. Verified against a live
database — on a half-applied install the namespace probe returns 1 (a false
pass that sends the user to repair a broken remote) while version() reports
`function eql_v3.version() does not exist`. A remote that genuinely lacks EQL
needs the SQL applied, not a ledger row.
Fixed in all four copies (the printed message is the source of truth; both
skills and the README duplicate it), with a guard in skill-supabase-apply so a
future edit cannot reintroduce ledger-repair advice without a check above it.
Also documents in skills/stash-cli that the init integration flags combine —
and that this is init only, since `eql migration` still takes exactly one
target.
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