fix(cli): stash init --drizzle installs EQL v3, not v2#705
Conversation
The Drizzle init flow pinned `eqlVersion: '2'` when calling `installCommand`, because `eql install --drizzle` — the only migration-generating install path at the time — was v2-only and rejects `--drizzle` under the v3 default. That made `stash init --drizzle` the single flow that provisions a v2 database. A bare `stash eql install`, and init for every other integration, already default to v3 (`resolveEqlVersion`, registry `default: '3'`). So every new Drizzle customer was silently added to the v2 install base the team plans to retire. It was also internally inconsistent: init copies the `stash-drizzle` skill into the same project, and that skill documents the v3 surface (`@cipherstash/stack-drizzle/v3`, `types.*` domains, `EncryptionV3`, `public.eql_v3_*` column types). The user's agent would author v3 code against the v2 database init had just provisioned. `stash eql migration --drizzle` (#691) closes the gap that forced the pin: v3 SQL, still migration-first. Init's Drizzle flow now routes through it, so the install keeps landing in the project's Drizzle migration history while emitting v3. The generated migration also carries the `cs_migrations` tracking schema, so one `drizzle-kit migrate` covers everything `stash encrypt …` needs. `eql install`'s config/client scaffolding isn't part of `eql migration`, so the branch does it directly (`offerStashConfig({ ensure: true })` + `ensureEncryptionClient`) to keep init's contract identical — without it, init would finish with no `stash.config.ts` and every downstream command would dead-end (#581). A missing or misconfigured `drizzle-kit` now degrades to "EQL not installed" with a pointer to `stash eql migration --drizzle`, rather than the old v2 path's `process.exit(1)` mid-init. Also: - `eqlMigrationCommand` gains `embedded` to suppress its intro/outro and `printNextSteps()` when run as an init step, so the user doesn't get two competing "what next" blocks. - `eql install --drizzle`'s rejection message now points at the v3 alternative instead of only suggesting `--eql-version 2`. - Updated `skills/stash-cli` and `skills/stash-drizzle`, which both documented the v2 pin as current behaviour.
🦋 Changeset detectedLatest commit: 30deef1 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: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 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 |
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
Verdict
The change itself is sound and the installEqlStep Drizzle branch is well covered — install-eql.test.ts guards the v2-pin regression, the eqlMigrationPending mapping, the scaffolding takeover, the --supabase forward, the failure degrade, and the credential-leak axis.
The gaps are in the other two files the diff touches, where new behaviour landed with no new tests: the embedded suppression in eql/migration.ts and the v3Alternative pointer in db/install.ts. Both are pure/unit-shaped and fit the existing Vitest suites. Three inline comments below, all verified against the diff.
No security issues observed. install-eql.ts:129 deliberately swallows the error object rather than echoing it — correct, given state.databaseUrl flows through this step, and the diff already ships a test for that axis.
Review stats
| Source | Raw findings | Survived |
|---|---|---|
| claude (claude-opus-4-8) [test-gap] | 3 | 3 |
| codex (gpt-5.5) [test-gap] | 2 | 2 |
- Merged/de-duplicated total: 3 kept findings (5 raw).
- Cross-model overlap: 2 of 3 kept findings were raised independently by both models (the
embeddedsuppression gap and thev3Alternativegap). 1 is single-source. - Dropped as unverifiable: 0.
Additional findings not posted inline
packages/cli/src/commands/init/steps/install-eql.ts:126— lopsided negative case: the newforwards --supabase so a Drizzle-on-Supabase project gets the role grantstest coverssupabase === true, but nothing asserts a plain Drizzle project passessupabase: undefined. One extraexpect(vi.mocked(eqlMigrationCommand).mock.calls[0][0].supabase).toBeUndefined()againstdrizzleState/drizzleProvidercloses it.packages/cli/src/commands/eql/migration.ts:165— theembedded+dryRuncombination is unreachable frominittoday (init never setsdryRun), so it's only worth a test ifEqlMigrationOptionsis treated as a stable programmatic surface. Noted, not recommended.
…l-clientPath re-run Close the three test gaps auxesis flagged on #705 — all callee-side or negative-case coverage the diff's new behaviour landed without: - eql/migration.ts: assert the six `if (!embedded)` suppression sites and the `?? false` default at the callee (banners emitted by default; suppressed + SQL still written when embedded; abort outro suppressed but CliExit still propagates). - db/install.ts: assert the `v3Alternative` pointer via its distinctive `stash eql migration --drizzle` substring (the pre-diff `/--drizzle/` match was unguarded), plus the lopsided-negative that it stays absent for --latest / --migration / --migrations-dir. - init/steps/install-eql.ts: cover the null-clientPath re-run path (config already on disk) and assert a plain Drizzle project passes supabase: undefined. Tests-only; no production change.
|
Tracked retroactively as #732 (filed after merge; closed as completed). |
2 participants
stash init --drizzlewas the only flow still provisioning an EQL v2 database. Bareeql installand init for every other integration already default to v3.The pin's stated reason — "the Drizzle migration path is v2-only" — stopped being true when
stash eql migration --drizzleshipped in 0811330.skills/stash-drizzle/SKILL.mddidn't merely document the pin, it justified it with the same now-false claim.The pin was causing the mismatch it appeared to prevent
Init installs
stash-encryption,stash-drizzleandstash-cliinto the customer's repo, and the two content skills are fully v3 — they documentEncryptionV3,types.*andpublic.eql_v3_*. Soinit --drizzlehanded the customer's coding agent v3 guidance against a v2 database it had just provisioned.Why this isn't just dropping the flag
routeInstallPathForEqlVersionforcesdrizzle: falseunder v3, so a bare flag flip would have silently converted Drizzle projects to a direct install and lost their migration history.Init's Drizzle branch now routes through
eqlMigrationCommand({ drizzle: true, embedded: true })— v3 SQL, still migration-first. Three things that needed handling:offerStashConfig({ ensure: true })+ensureEncryptionClient, which aren't part ofeql migrationbut are load-bearing for stash CLI:eql installshouldn't require a stash.config.ts; support standalone --database-url + document URL precedence #581.embeddedsuppresses the duplicate intro/outro/printNextSteps()that would otherwise render mid-init.drizzle-kitnow degrades to "not installed" rather than the old path's mid-initprocess.exit(1).Also fixed:
eql install --drizzle's rejection message, which was actively steering users to--eql-version 2.On the risk that didn't materialise
The concern going in was that flipping the pin would yield a v3 database with v2 authoring code, since
init/utils.tsemitsencryptedColumn().equality()throughout. Tracing callers showed those generators are dead on the init path:build-schema.ts:129only callsgeneratePlaceholderClient()generateClientFromSchema{,s}has one caller, and it'scommands/schema/build.ts— a different commandSearchOp/ColumnDefininit/types.tsare vestigial, and init always setsschemas: []Verification
install-eql.test.tsgoes from 5 tests to 11, including a regression guard on the v2 pin, config scaffolding,--supabaseforwarding, drizzle-kit-missing degradation, and no credential leakage on the failure path.Full suite: 51 files, 589 tests passed. Build clean,
code:checkexits 0, manifest check confirms every command named in the skill edits resolves.Follow-up, not in this PR
generatePlaceholderClientemits v2 pattern comments andEncryption({ schemas: [] })for all integrations, not just Drizzle — so Supabase and plain-Postgres init already ship a v3 database with a v2 placeholder client. Pre-existing and broader than this fix; self-contained (two string constants plusschema/build.ts's generators). Worth its own issue.🤖 Generated with Claude Code