feat(cli): default EQL to v3, stop recommending db push (v2/Proxy-only)#586
Open
coderdan wants to merge 3 commits into
Open
feat(cli): default EQL to v3, stop recommending db push (v2/Proxy-only)#586coderdan wants to merge 3 commits into
coderdan wants to merge 3 commits into
Conversation
Closes #585. EQL v3 is now the default for `stash eql install` / `eql upgrade` — no `--eql-version 3` required. The v2-only paths (--drizzle, --migration, --migrations-dir, --latest) require an explicit `--eql-version 2` and error with clear guidance otherwise; `stash init` pins v2 for the Drizzle flow. A single DEFAULT_EQL_VERSION constant drives the installer defaults. `stash db push` writes `public.eql_v2_configuration`, a v2 + CipherStash Proxy artifact that EQL v3 neither creates nor reads (v3 config lives in each column's `eql_v3.*` type). The CLI no longer recommends it: - `eql status` is v3-aware — reports config-in-column-types on a v3-only DB instead of the "table not found → run db push" dead-end. - `db push` guards a v3-only DB with a clean "not needed under v3" message. - Removed push recommendations from the help banner, init/plan/cutover setup-prompt guidance (dropping the now-unused usesProxy branch), and the encrypt-status / quest next-step hints. `db push` / `db activate` remain for v2 + Proxy users and are labelled as such. Tests: reworked validateInstallFlags coverage for the v3 default + explicit-v2 gating; made the v2-specific installer tests explicit about eqlVersion: 2; replaced the usesProxy setup-prompt tests with no-db-push assertions. 396 unit + 55 e2e green.
🦋 Changeset detectedLatest commit: 5d13d2b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
📝 WalkthroughWalkthroughThis PR switches EQL install and upgrade defaults to v3, requires explicit v2 selection for v2-only flags, adds v3-aware guards in db push and db status, and rewrites CLI guidance and init prompts to remove db push recommendations. ChangesEQL v3 Default and db push Removal
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![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
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/installer/index.ts (1)
471-492: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
eqlVersion: 2into the Supabase migration helper.
loadBundledEqlSql()now defaults to v3, sowriteSupabaseEqlMigration()will emit the wrong bundle for the v2-only--migrationpath unless it threads the version through. The direct install path can keep the default.🤖 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/installer/index.ts` around lines 471 - 492, The Supabase migration path is using the default Eql bundle version instead of v2, so update writeSupabaseEqlMigration() to pass eqlVersion: 2 into loadBundledEqlSql() when generating the --migration script. Keep the direct install flow unchanged, and use the existing loadBundledEqlSql() and writeSupabaseEqlMigration() symbols to wire the version through explicitly.
🤖 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/commands/db/status.ts`:
- Around line 94-109: The v2/v3 guard in statusCommand is using the wrong
install check, so `installedV2` and `versionV2` are actually reading the default
v3 schema. Update the v2 probe calls in `statusCommand` to explicitly pass `{
eqlVersion: 2 }` when calling `installer.isInstalled()` and
`installer.getInstalledVersion()`, then keep the `if (!installedV2)` early
return so v3-only databases skip the `public.eql_v2_configuration` query path.
---
Outside diff comments:
In `@packages/cli/src/installer/index.ts`:
- Around line 471-492: The Supabase migration path is using the default Eql
bundle version instead of v2, so update writeSupabaseEqlMigration() to pass
eqlVersion: 2 into loadBundledEqlSql() when generating the --migration script.
Keep the direct install flow unchanged, and use the existing loadBundledEqlSql()
and writeSupabaseEqlMigration() symbols to wire the version through explicitly.
🪄 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
Run ID: b22cc4ac-dee4-47ae-aa6f-d98e3efe4b12
📒 Files selected for processing (20)
.changeset/stash-cli-eql-v3-default.mdpackages/cli/src/__tests__/installer.test.tspackages/cli/src/__tests__/supabase-migration.test.tspackages/cli/src/bin/main.tspackages/cli/src/cli/__tests__/help.test.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/db/activate.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/db/push.tspackages/cli/src/commands/db/status.tspackages/cli/src/commands/db/upgrade.tspackages/cli/src/commands/encrypt/backfill.tspackages/cli/src/commands/encrypt/cutover.tspackages/cli/src/commands/encrypt/status.tspackages/cli/src/commands/init/lib/__tests__/setup-prompt.test.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/init/lib/write-context.tspackages/cli/src/commands/init/steps/install-eql.tspackages/cli/src/commands/status/quest.tspackages/cli/src/installer/index.ts
💤 Files with no reviewable changes (1)
- packages/cli/src/commands/init/lib/write-context.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the stash CLI’s EQL workflows to default installs/upgrades to EQL v3 and removes outdated guidance that recommended stash db push (a v2 + Proxy-only step) in general CLI output. It also adds version-aware gating so v2-only installer paths require an explicit --eql-version 2, and adjusts status/prompt messaging to align with v3’s “config-in-column-types” model.
Changes:
- Default EQL targeting to v3 across installer logic and CLI flags/help, while requiring explicit
--eql-version 2for v2-only install/upgrade paths. - Remove broad CLI recommendations for
db push, and relabeldb push/db activateas (EQL v2 + Proxy). - Make
eql status/db pushbehave sensibly on v3-only databases (avoid v2 config-table dead ends).
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/installer/index.ts | Introduces DEFAULT_EQL_VERSION = 3 and threads defaulting through installer helpers. |
| packages/cli/src/commands/status/quest.ts | Removes db push from quest “next step” guidance. |
| packages/cli/src/commands/init/steps/install-eql.ts | Pins v2 for Drizzle init flow while leaving v3 as default elsewhere. |
| packages/cli/src/commands/init/lib/write-context.ts | Removes usesProxy from setup-prompt context building. |
| packages/cli/src/commands/init/lib/setup-prompt.ts | Removes Proxy-conditional branches and strips db push recommendations from prompts/plans. |
| packages/cli/src/commands/init/lib/tests/setup-prompt.test.ts | Updates setup prompt tests to assert no db push mentions. |
| packages/cli/src/commands/encrypt/status.ts | Rewords “no encrypted columns” guidance to avoid db push. |
| packages/cli/src/commands/encrypt/cutover.ts | Rewords “no pending config” error to remove db push imperative. |
| packages/cli/src/commands/encrypt/backfill.ts | Updates unsupported cast_as warning to avoid db push guidance. |
| packages/cli/src/commands/db/upgrade.ts | Defaults upgrade targeting to v3 and improves --latest + default-version messaging. |
| packages/cli/src/commands/db/status.ts | Makes config-table probing conditional on v2 installation, to avoid v3-only dead ends. |
| packages/cli/src/commands/db/push.ts | Adds v3-only DB guard so db push exits cleanly when not applicable. |
| packages/cli/src/commands/db/install.ts | Makes v3 the default install target; adds explicit v2-only flag validation. |
| packages/cli/src/commands/db/activate.ts | Rewords “no pending config” error to remove db push imperative. |
| packages/cli/src/cli/registry.ts | Updates flag descriptions and defaults to show v3 default + v2-only flag gating. |
| packages/cli/src/cli/tests/help.test.ts | Updates help output expectation for default eql version = 3. |
| packages/cli/src/bin/main.ts | Relabels db push / db activate as (EQL v2 + Proxy) and removes example db push. |
| packages/cli/src/tests/supabase-migration.test.ts | Updates tests around v3-default + explicit-v2 gating behavior. |
| packages/cli/src/tests/installer.test.ts | Makes v2 installer tests pass explicit eqlVersion: 2 where needed. |
| .changeset/stash-cli-eql-v3-default.md | Adds release notes for v3 defaulting + messaging changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- status.ts (critical, self-inflicted regression): `installedV2` used bare
`isInstalled()` / `getInstalledVersion()`, which now default to v3 after the
DEFAULT_EQL_VERSION flip — so on a v2-only DB status wrongly reported "EQL is
not installed", and on a v3 DB it mislabelled the version and re-hit the
eql_v2_configuration dead-end. Pass `{ eqlVersion: 2 }` explicitly.
- push.ts: replace the two `isInstalled` probes (3 connections, missed the
fresh-DB case, diverged when the schema exists without the table, and
swallowed transient errors via `.catch(() => false)`) with a single
`to_regclass('public.eql_v2_configuration')` check on the connection push
already opens. One connection; covers v3-only, fresh, and partial installs.
- Centralize the default-version resolution in `resolveEqlVersion()` next to
DEFAULT_EQL_VERSION and use it in install.ts (both sites) + upgrade.ts, so the
default lives in one place instead of three hand-inlined `=== '2' ? 2 : 3`.
- Document the Supabase-init behavior change (now a v3 direct install rather
than the v2 migration-file flow) in the changeset.
- Add resolveEqlVersion unit coverage.
398 unit + 55 e2e green.
…review) writeSupabaseEqlMigration called loadBundledEqlSql without an eqlVersion, so after the DEFAULT_EQL_VERSION flip to v3 it emitted cipherstash-encrypt-v3- supabase.sql into a migration that still appends the v2 SUPABASE_PERMISSIONS_SQL — a mismatched v3-body/v2-perms file. The Supabase migration-file flow is v2-only (v3 has no migration-file path), so pin eqlVersion: 2 explicitly. The existing test passed despite the bug because the header + permissions text mention eql_v2 regardless of the body; strengthened it to assert `eql_v2_encrypted` is present and no `eql_v3` leaks in. Caught by Copilot on the PR.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/db/supabase-migration.ts (1)
102-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEQL v2 pin is correct and well-documented.
The explicit
eqlVersion: 2correctly prevents the v3 default from leaking into the Supabase migration-file flow. The comment clearly explains the rationale and the downstream test assertseql_v2_encryptedis present andeql_v3is absent, confirming the contract holds.One minor observation on line 112:
excludeOperatorFamily: excludeOperatorFamily || truealways evaluates totrueregardless of the option value (sincefalse || true === true), making theexcludeOperatorFamilyoption fromWriteSupabaseEqlMigrationOptionseffectively dead. If the intent is "always exclude operator family for Supabase," passingtruedirectly would be clearer and avoid implying the option has an effect.🤖 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/db/supabase-migration.ts` around lines 102 - 113, The Supabase migration flow in supabase-migration.ts correctly pins eqlVersion through loadBundledEqlSql, but excludeOperatorFamily is effectively ignored because excludeOperatorFamily || true always resolves to true. Update the loadBundledEqlSql call to reflect the intended behavior explicitly: either pass a literal true if Supabase must always exclude the operator family, or wire the WriteSupabaseEqlMigrationOptions value through without the unconditional fallback if the option should remain configurable.
🤖 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.
Nitpick comments:
In `@packages/cli/src/commands/db/supabase-migration.ts`:
- Around line 102-113: The Supabase migration flow in supabase-migration.ts
correctly pins eqlVersion through loadBundledEqlSql, but excludeOperatorFamily
is effectively ignored because excludeOperatorFamily || true always resolves to
true. Update the loadBundledEqlSql call to reflect the intended behavior
explicitly: either pass a literal true if Supabase must always exclude the
operator family, or wire the WriteSupabaseEqlMigrationOptions value through
without the unconditional fallback if the option should remain configurable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b13aa794-6d38-4536-bf0e-3ca7a919e297
📒 Files selected for processing (2)
packages/cli/src/__tests__/supabase-migration.test.tspackages/cli/src/commands/db/supabase-migration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/tests/supabase-migration.test.ts
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
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 #585. Follow-up to the config-scaffold DX work (#580).
What & why
Two things you asked for after testing
eql statuson a v3-only database:1. EQL v3 is now the default
stash eql install/stash eql upgradetarget v3 (nativeeql_v3.*domain schema) with no--eql-version 3needed. A singleDEFAULT_EQL_VERSIONconstant drives the installer.The v2-only paths —
--drizzle,--migration,--migrations-dir,--latest— require an explicit--eql-version 2and error with actionable guidance otherwise (v3 installs via the direct path only). The "requires--supabase" check runs first, so a bare--migrationstill points at its more fundamental missing prerequisite.stash initpins v2 automatically when it drives the Drizzle migration flow.2. The CLI no longer recommends
db pushdb pushwritespublic.eql_v2_configuration— a v2 + CipherStash Proxy artifact. EQL v3 has no configuration table (config lives in each column'seql_v3.*type) and nothing in the v3 stack reads it (verified: v3 SQL has zeroCREATE TABLE/config functions; the SDK never touches the table; Proxy'sreload_configdoesn't exist in v3). CipherStash's own v3 spec calls it "now-redundant".eql statusis v3-aware: on a v3-only DB it reports config-in-column-types instead of the old "table not found → rundb push" dead-end (which never created that table and doesn't apply to v3).db pushguards a v3-only DB with a clean "not needed under EQL v3" message instead of a rawrelation "public.eql_v2_configuration" does not exist.usesProxybranch), and theencrypt status/ quest next-step hints.db push/db activateremain available for EQL v2 + Proxy users and are now labelled(EQL v2 + Proxy).Decisions worth a look
db activate/encrypt cutoverreactive errors: I kept these commands' own "no pending config" error messages coherent (describing the v2/Proxy config lifecycle) rather than stripping the guidance entirely, since a v2/Proxy user running those commands still needs it. I dropped the explicitdb pushimperative from them. Tell me if you want them gutted too.usesProxyis now vestigial in the setup prompt (its only consumer). I removed it fromSetupPromptContextbut left the broader init/context.json plumbing intact. Retiring the "do you use Proxy?" init question is a natural follow-up.SKILL.md) shipped bystash initstill referencedb push— those are docs, out of scope here; happy to sweep them in a follow-up.alpha.2("version: DEV"). This makes it the default install for everyone, includingstash init.Tests
Reworked
validateInstallFlagscoverage (v3 default + explicit-v2 gating), made the v2-specific installer tests explicit abouteqlVersion: 2, replaced theusesProxysetup-prompt tests with no-db pushassertions. 396 unit + 55 e2e green; Biome clean.Summary by CodeRabbit
eql installandeql upgrade.eql statusis v3-aware, including how v3 encryption config is discovered.--eql-version 2), with improved guidance.stash db pushno longer fails on v3-only databases, anddb activate/status/cutover messaging is clearer.stash dbhelp and init/setup/quest prompts to remove outdateddb pushrecommendations.