feat(cli): add stash eql migration --drizzle (v3, migration-first EQL install)#691
feat(cli): add stash eql migration --drizzle (v3, migration-first EQL install)#691coderdan wants to merge 2 commits into
stash eql migration --drizzle (v3, migration-first EQL install)#691Conversation
…QL install) First of the three PRs in #690: make migration-first, ORM-agnostic EQL v3 install the front door, so every integration sources install SQL from one place (the CLI's bundled variants) instead of vendoring its own. This is the pattern that keeps Drizzle Supabase-safe; prisma-next's superuser-on-Supabase failure is fixed by the stacked follow-ups (`--prisma` emitter + removing prisma-next's baked baseline), which land on top of the prisma-next EQL v3 work (#655). `stash eql migration --drizzle [--supabase] [--name] [--out] [--dry-run]`: - Generates a Drizzle custom migration (via `drizzle-kit generate --custom`, then injects the SQL) carrying the bundled EQL **v3** install script + the `cs_migrations` tracking schema — one `drizzle-kit migrate` does everything `stash encrypt …` needs. - `--supabase` appends the v3 role grants (`eql_v3` + `eql_v3_internal` → anon/authenticated/service_role), matching `stash eql install --supabase`. - v3 only — no `--eql-version` (prisma-next never shipped v2). - `--prisma` is registered but fails with a pointer to #690 until the stacked PR. The SQL assembly (`buildEqlV3MigrationSql`) is a pure, unit-tested function; the drizzle-kit scaffold/inject orchestration reuses the proven helpers from the v2 `install --drizzle` path (`findGeneratedMigration`, `cleanupMigrationFile`, now exported). Registry + dispatch + help + `stash-cli`/`stash-drizzle` skills updated. Tests: 4 new unit (bundle present, grants gated on --supabase, tracking schema), 544 CLI unit green, manifest resolves the command, biome clean. Changeset: stash minor. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
🦋 Changeset detectedLatest commit: fd0af48 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 |
📝 WalkthroughWalkthroughAdds ChangesEQL migration generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
packages/cli/src/commands/eql/migration.ts (1)
132-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
drizzle-kitinstallation hint package-manager aware.Since
detectPackageManager()is already imported and available in this file, you can dynamically construct the installation command instead of hardcodingnpm. This provides a better experience for users onpnpm,yarn, orbun.✨ Proposed refactor
- p.log.info('Make sure drizzle-kit is installed: npm install -D drizzle-kit') + const pm = detectPackageManager() + const installCmd = pm === 'npm' ? 'npm install -D' : `${pm} add -D` + p.log.info(`Make sure drizzle-kit is installed: ${installCmd} drizzle-kit`)🤖 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` at line 132, Update the installation hint in the migration command to use the package manager returned by detectPackageManager() instead of hardcoding npm, while preserving the existing drizzle-kit development-dependency install wording for pnpm, yarn, bun, and other supported managers.
🤖 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/eql/migration.ts`:
- Around line 96-100: Validate the user-derived migrationName in the migration
command flow before constructing drizzleCmd, allowing only alphanumeric
characters, dashes, and underscores. Reject invalid names with the command’s
established error-handling behavior, and only interpolate the validated value
into the execSync command.
---
Nitpick comments:
In `@packages/cli/src/commands/eql/migration.ts`:
- Line 132: Update the installation hint in the migration command to use the
package manager returned by detectPackageManager() instead of hardcoding npm,
while preserving the existing drizzle-kit development-dependency install wording
for pnpm, yarn, bun, and other supported managers.
🪄 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: dc6bc24f-3a47-4690-b339-ba275d91484e
📒 Files selected for processing (9)
.changeset/eql-migration-command.mdpackages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/commands/eql/migration.tspackages/cli/src/messages.tsskills/stash-cli/SKILL.mdskills/stash-drizzle/SKILL.md
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Test coverage review — stash eql migration --drizzle
The added migration.test.ts covers buildEqlV3MigrationSql well (4 tests, both --supabase arms). The gap is everything around it: eqlMigrationCommand and generateDrizzleEqlMigration — the actual command — have zero tests. That's 3 validation exits, the --dry-run short-circuit, the --name/--out threading, and the cleanup-on-write-failure path, none exercised. The file header calls the orchestration "thin and I/O-bound", but packages/cli already has the idiom for exactly this shape — auth/__tests__/login.test.ts (spyExit() + hoisted clack mock) and env/__tests__/env.test.ts — so the cost here is low.
Six inline comments below, ordered by value. All sketches are drop-in for the fast unit suite (pnpm --filter stash test) — I verified the module graph loads without native deps and that the ordering assertion in the third comment passes as written.
Additional coverage gaps not posted inline
execSyncfailure fallback (migration.ts:120-136) — the stderr extraction has a 3-way fallback (error.stderr→error.message→'Unknown error.') and none of the three arms is covered. Cheap to pin once thenode:child_processmock from comment #2 is in place:execMock.mockImplementation(() => { throw Object.assign(new Error('boom'), { stderr: 'drizzle-kit: not found' }) })→ assertsp.log.errorgot the trimmed stderr, not'boom'.cleanupMigrationFile's warn-on-failure arm (db/install.ts:886) — thecatch→p.log.warnbranch is unreachable in any current test.- No test guards registry ↔ HELP drift. Not a gap in the diff's own logic, but worth flagging: the new command is in
registry.tsbuteql migrationwas not added to theHELPbanner inbin/main.ts:110-112, which still lists onlyeql install/eql upgrade/eql status.stash --helptherefore won't mention the command this PR ships.AGENTS.mdtreats the registry as the source of truth, andmanifest.test.tsasserts registry completeness — but nothing asserts the hand-maintainedHELPstring stays in sync with it, so this drifted silently. Worth a one-line fix plus (optionally) a unit test that every non-hidden registry command name appears inHELP.
Out of scope, noted only
Nothing security- or crypto-relevant. One functional observation surfaced while writing comment #4: --out is resolved for the lookup but never passed to drizzle-kit, so --out only works when it happens to match drizzle.config.ts. Same shape as the pre-existing db/install.ts path, so this PR inherits rather than introduces it — but the registry advertises --out as "Directory to write the generated migration into", which is not what it does today.
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Reviewed at extra-high effort — recall-oriented, so some of these are minor. Take them as surfaced, not all as blockers. 10 findings: 9 inline, 1 below.
The two that would bite users immediately
- The command runs the wrong drizzle-kit on pnpm and yarn (
migration.ts:98) —runnerCommandis thepnpm dlx/yarn dlxdownload-and-run form, not the run-a-local-binary form.execCommandinsetup-prompt.ts:78already exists for exactly this and is already used fordrizzle-kit generate. --outdoesn't write anywhere (migration.ts:97) — it only changes where we search. Any project whose drizzle out dir isn't./drizzlefails at step 2.
Both are inherited from the v2 install --drizzle path this reuses, so the fix likely belongs in both places.
Not inline (the line isn't in the diff)
eql migration is missing from the global HELP banner — packages/cli/src/bin/main.ts:110-112.
registry.ts:8-13 calls this out specifically:
⚠️ The one remaining hand-maintained surface is the globalHELPbanner inbin/main.ts(barestash/stash --help) — it duplicates only the command list (names + summaries), not their flags. A new command, or a summary edit, still belongs in both places or the banner andstash manifestwill diverge.
stash --help, bare stash, and the eql unknown-subcommand fallback (main.ts:258) all print HELP, which lists eql install / eql upgrade / eql status but not eql migration — so the command this PR positions as the preferred v3 install path is invisible in the CLI's primary help. Per-command help (stash eql migration --help) is fine; it reads the registry.
The SQL assembly split — buildEqlV3MigrationSql as a pure, unit-tested function — reads well, and the v3-only / exactly-one-target framing is clean.
Addresses the review on #691 — correctness, security, telemetry, and coverage: - Run the PROJECT-LOCAL drizzle-kit (`pnpm exec` / `npx --no-install`), not the `pnpm dlx`/`yarn dlx` download-and-run form, so it resolves the project's own drizzle.config.ts + schema. New shared `execCommand`/`execArgv` in init/utils (setup-prompt's private copy folded in). - Invoke via `spawnSync` with an argv array instead of `execSync` on a shell string — a `--name` with spaces or shell metacharacters is now one inert token (no word-splitting, no command injection). Plus a `[\w-]+` name guard. - Pass `--out` through to `drizzle-kit generate` (always) so the flag actually steers where the migration is written, and our lookup can't miss it. - Validation exits and every abort throw `CliExit(1)` instead of `process.exit`, so the telemetry `finally` runs and the branches are unit-testable. - Guard `loadBundledEqlSql` (corrupt bundle → clean error, not a fatal trace), add the missing `p.intro`, and call `printNextSteps()` so the now-preferred install path isn't less helpful than `eql install`. - HELP banner lists `eql migration`. Tests: 14 migration unit (target selection incl. `--supabase`-alone, name guard, dry-run, argv threading, non-zero drizzle-kit, write-failure cleanup, SQL order), 3 `findGeneratedMigration` unit (now public API), e2e smoke + `eql migration --help`. 557 unit / 65 e2e green, code:check clean. Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
|
Addressed the full review in fd0af48 — all inline threads replied to + resolved. Summary of the substantive changes: Correctness / security
Telemetry / testability
Coverage (the big gap you flagged)
557 unit / 65 e2e green, One I only mitigated, not fully fixed (called out on its thread): a step-2 "couldn't locate the generated file" failure can't clean a path it never learned. Passing |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/eql/__tests__/migration.test.ts`:
- Line 43: Update the mock declaration containing real so it does not use the
type-erasing undefined as unknown assertion: make real optional and narrow it
before use, or add a targeted Biome suppression with a reason documenting the
hoisted mock initialization. Preserve the existing writeFileSync mock behavior.
In `@packages/cli/src/commands/init/utils.ts`:
- Around line 257-258: Update the `bun` case in the command selection logic to
include `--no-install` in `prefixArgs` alongside `x`, preventing Bun from
installing missing binaries while preserving project-local execution.
🪄 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: 498c1f60-11dc-4ae3-8a05-2ecc6cd2823f
📒 Files selected for processing (11)
packages/cli/src/bin/main.tspackages/cli/src/cli/registry.tspackages/cli/src/commands/db/__tests__/find-generated-migration.test.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/commands/eql/migration.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/init/utils.tspackages/cli/src/messages.tspackages/cli/tests/e2e/command-help.e2e.test.tspackages/cli/tests/e2e/smoke.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli/src/messages.ts
- packages/cli/src/bin/main.ts
- packages/cli/src/cli/registry.ts
| // throw without touching everything else. `beforeEach` restores the delegating | ||
| // default after `clearAllMocks`. | ||
| const fsWrite = vi.hoisted(() => ({ | ||
| real: undefined as unknown as typeof import('node:fs').writeFileSync, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove or explicitly suppress the type-erasing assertion.
undefined as unknown as ... violates the TypeScript policy. Model real as optional and narrow it before use, or add a deliberate Biome suppression explaining the hoisted mock initialization.
As per coding guidelines, “source code must not use type-erasing assertions such as as unknown unless a deliberate case is suppressed with a reason.”
🤖 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/__tests__/migration.test.ts` at line 43, Update
the mock declaration containing real so it does not use the type-erasing
undefined as unknown assertion: make real optional and narrow it before use, or
add a targeted Biome suppression with a reason documenting the hoisted mock
initialization. Preserve the existing writeFileSync mock behavior.
Source: Coding guidelines
| case 'bun': | ||
| return { command: 'bun', prefixArgs: ['x'] } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant section of the file first.
ast-grep outline packages/cli/src/commands/init/utils.ts --view expanded
# Read the surrounding lines for the binary-resolution helper.
sed -n '220,290p' packages/cli/src/commands/init/utils.ts
# Look for any comments or tests that describe the intended semantics.
rg -n "local|project-local|bun x|bunx|--no-install|prefixArgs" packages/cli/src/commands/init -S
# Check whether the repository mentions Bun download/fallback behavior elsewhere.
rg -n "bun x|bunx|no-install|downloads from npm|install from npm" -S .Repository: cipherstash/stack
Length of output: 29075
🌐 Web query:
Bun documentation bun x no-install local binary fallback installs from npm when missing
💡 Result:
The bun x (or bunx) command is designed to first look for a locally installed package (e.g., in node_modules) and, if not found, fall back to auto-installing the package from the npm registry [1][2]. Regarding your query about the --no-install flag and its behavior: 1. Purpose of --no-install: According to the Bun documentation, the --no-install flag is intended to skip automatic installation if the requested package is not already installed [1]. 2. Known Limitations: Users have reported issues where bun x --no-install does not always behave as expected when a binary is missing from the local project, sometimes failing to strictly prevent fallback behavior or behaving inconsistently compared to other tools like npx [3]. 3. Current Technical Status: While the documented behavior is that bun x should skip installation with this flag, this functionality has been the subject of active bug reports and community discussion [3]. If bun x fails to find a local binary, it may attempt to resolve the package via auto-install unless the environment or configuration successfully restricts it. If you are encountering issues where bun x still attempts to download packages despite using --no-install, it is likely due to the ongoing issues with the implementation of this flag in the Bun CLI [3]. For strictly reproducible or secure environments, some users have opted for alternative approaches, such as checking for binary existence manually or using dedicated local-only execution scripts [3].
Citations:
- 1: https://bun.com/docs/pm/bunx
- 2: https://github.com/oven-sh/bun/blob/main/docs/pm/bunx.mdx
- 3:
--no-installflag isn't working forbun xoven-sh/bun#23673
Add --no-install to Bun’s exec path. bun x can fall back to installing a missing binary from npm, so this should stay project-local like the other package managers.
Proposed fix
case 'bun':
- return { command: 'bun', prefixArgs: ['x'] }
+ return { command: 'bun', prefixArgs: ['x', '--no-install'] }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'bun': | |
| return { command: 'bun', prefixArgs: ['x'] } | |
| case 'bun': | |
| return { command: 'bun', prefixArgs: ['x', '--no-install'] } |
🤖 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/utils.ts` around lines 257 - 258, Update the
`bun` case in the command selection logic to include `--no-install` in
`prefixArgs` alongside `x`, preventing Bun from installing missing binaries
while preserving project-local execution.
1 participant
PR 1 of 3 in the plan documented at #690 — migration-first, ORM-agnostic EQL v3 install.
Why
EQL install differs per integration, and prisma-next's approach breaks on Supabase (its baked-in full v2 bundle needs superuser for
CREATE OPERATOR CLASS/FAMILY). Drizzle avoids this by delegating install to the CLI. This PR makes that delegation a first-class, migration-emitting command — one source of install SQL (the CLI's bundled variants), reusable by every ORM.What
stash eql migration --drizzle [--supabase] [--name] [--out] [--dry-run]. Generates a Drizzle custom migration (drizzle-kit generate --custom, then injects SQL) carrying the bundled EQL v3 install script + thecs_migrationstracking schema — so onedrizzle-kit migrateinstalls everythingstash encrypt …needs.--supabaseappends the v3 role grants (eql_v3+eql_v3_internal→ anon/authenticated/service_role), matchingstash eql install --supabase. Harmless when connecting directly aspostgres; needed for PostgREST/RLS.--eql-version(prisma-next never shipped v2).--prismais registered but fails with a pointer to stash eql migration: migration-first, ORM-agnostic EQL v3 install (fixes prisma-next superuser-on-Supabase) #690 until PR3 (it needs prisma-next's v3 surface from Feat/prisma next eql v3 #655 to install against).Design
buildEqlV3MigrationSql) is a pure, unit-tested function; the drizzle-kit scaffold/inject orchestration reuses the proven helpers from the v2install --drizzlepath (findGeneratedMigration,cleanupMigrationFile, now exported — the only change toinstall.ts).--drizzle/--prismarequired; both/neither error cleanly (exit 1).Sequencing (see #690)
--drizzle, offmain. Independent; Drizzle v3 is already on main.feat/prisma-next-eql-v3) on this.--prismaemitter + remove prisma-next's baked baseline, stacked on Feat/prisma next eql v3 #655 — that's the change that makes prisma-next Supabase-safe.Tests
buildEqlV3MigrationSql: v3 bundle present, grants gated on--supabase, tracking schema, additive superset).--prisma→ pointer + exit 1; both → exit 1;--drizzle --dry-run→ dry-run note, exit 0).eql migration, biome clean.stash-cli+stash-drizzleskills updated; changeset (stashminor).Note: the pty e2e suite wasn't run (this command has no interactive flow); its behaviour is covered by the unit + smoke checks above.
https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
Summary by CodeRabbit
stash eql migrationto generate EQL v3 install migrations for Drizzle.Summary by CodeRabbit
stash eql migrationto generate EQL v3 installation migrations for Drizzle.