Skip to content

Commit 20e5df9

Browse files
committed
fix(cli): honest Drizzle EQL outcome + close test gaps (review on #687)
Differential review (Codex) found the completion gate still reported a successful, complete setup for `stash init --drizzle` before EQL was in the database: the Drizzle path only *generates* a v2 migration (applied later with `drizzle-kit migrate`), yet `installEqlStep` returned `eqlInstalled: true` for every non-throwing outcome — the exact false-success this PR removes elsewhere. - `installCommand` now returns a structured `InstallOutcome` (`installed` | `already-installed` | `migration-generated` | `dry-run`). - `installEqlStep` maps `migration-generated` to a new `eqlMigrationPending` state (EQL not in the DB yet) instead of `eqlInstalled`. - The init summary reports it honestly — "○ EQL migration generated — apply it with `drizzle-kit migrate`" — and excuses it from the "Setup incomplete" hard-fail (exit 0), the same treatment Prisma Next already gets. - Changeset + stash-cli skill updated to match. Test-gap review: added the previously-uncovered branches — - init summary's three-way encryption-client checkmark (scaffolded / kept); - the Drizzle migration-pending gate (cross-layer: outcome → step → summary); - handoff-claude / handoff-codex launch-prompt skills clause (stripped build drops the skills dir; codex keeps AGENTS.md). Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
1 parent 1d245a9 commit 20e5df9

10 files changed

Lines changed: 272 additions & 18 deletions

File tree

.changeset/honest-noninteractive-init.md

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,13 @@ setup that didn't fully complete.
1313
Interactive runs still offer to align. A *newer* install stays a warn (the
1414
install is likely fine; update the CLI instead).
1515
- **No false "Setup complete".** If the EQL extension isn't installed at the
16-
end (and the integration isn't Prisma Next, which installs it via
17-
`migration apply`), the summary reads "Setup incomplete" and init exits
18-
non-zero, pointing at `stash eql install`.
16+
end — and the integration isn't one that installs it out-of-band — the
17+
summary reads "Setup incomplete" and init exits non-zero, pointing at
18+
`stash eql install`. Integrations that install EQL via a migration are
19+
reported honestly rather than as failures: Prisma Next (installs it via
20+
`migration apply`) and the Drizzle flow, which *generates* an EQL migration
21+
and now says "EQL migration generated — apply it with `drizzle-kit migrate`"
22+
instead of claiming the extension is already installed.
1923
- **Honest checkmarks.** The summary no longer claims "Database connection
2024
verified" (init resolves a URL but doesn't open a connection) — it now says
2125
"Database URL resolved" — and only shows "Encryption client scaffolded" when

packages/cli/src/commands/db/install.ts

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,28 @@ async function resolveInstallContext(
169169
return { databaseUrl, clientPath }
170170
}
171171

172-
export async function installCommand(options: InstallOptions) {
172+
/**
173+
* What `installCommand` actually did — so callers (notably `stash init`'s
174+
* completion gate) can tell "EQL is now in the database" from "a migration
175+
* file was written but nothing was applied yet".
176+
*
177+
* - `installed` / `already-installed`: EQL is present in the target database.
178+
* - `migration-generated`: a migration file was written (Drizzle, or the
179+
* Supabase `--migration` mode); the user still has to APPLY it
180+
* (`drizzle-kit migrate` / `supabase db push`) before EQL exists in the DB.
181+
* - `dry-run`: nothing was changed.
182+
*
183+
* Terminal error paths call `process.exit(1)` and never return an outcome.
184+
*/
185+
export type InstallOutcome =
186+
| 'installed'
187+
| 'already-installed'
188+
| 'migration-generated'
189+
| 'dry-run'
190+
191+
export async function installCommand(
192+
options: InstallOptions,
193+
): Promise<InstallOutcome> {
173194
p.intro(runnerCommand(detectPackageManager(), 'stash eql install'))
174195

175196
// Validate mutually-exclusive / supabase-required flags BEFORE doing any
@@ -238,7 +259,9 @@ export async function installCommand(options: InstallOptions) {
238259
supabase: resolved.supabase,
239260
excludeOperatorFamily: resolved.excludeOperatorFamily,
240261
})
241-
return
262+
// A Drizzle migration file was written — NOT applied. EQL only lands in
263+
// the database once the user runs `drizzle-kit migrate`.
264+
return 'migration-generated'
242265
}
243266

244267
// Supabase non-Drizzle path: pick between writing a migration file and
@@ -270,7 +293,9 @@ export async function installCommand(options: InstallOptions) {
270293
force: options.force,
271294
dryRun: options.dryRun,
272295
})
273-
return
296+
// Migration file written, not applied — the user runs it via their
297+
// Supabase migration workflow (`supabase db push`).
298+
return 'migration-generated'
274299
}
275300
// mode === 'direct' — fall through to existing direct-install behavior.
276301
}
@@ -282,7 +307,7 @@ export async function installCommand(options: InstallOptions) {
282307
: `Would use bundled EQL${eqlVersion === 3 ? ' v3' : ''} install script`
283308
p.note(`${source}\nWould execute the SQL against the database`, 'Dry Run')
284309
p.outro('Dry run complete.')
285-
return
310+
return 'dry-run'
286311
}
287312

288313
const installer = new EQLInstaller({
@@ -331,7 +356,7 @@ export async function installCommand(options: InstallOptions) {
331356
if (installed) {
332357
p.log.info('Use --force to re-run the install script.')
333358
p.outro('Nothing to do.')
334-
return
359+
return 'already-installed'
335360
}
336361
}
337362

@@ -370,6 +395,7 @@ export async function installCommand(options: InstallOptions) {
370395

371396
printNextSteps()
372397
p.outro('Done!')
398+
return 'installed'
373399
}
374400

375401
/**
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { beforeEach, expect, it, vi } from 'vitest'
2+
import type { InitState } from '../../../init/types.js'
3+
4+
// The launch prompt's skills clause is the unit under test. Mock the skill
5+
// installer so we control whether any skills were "copied", and stub the
6+
// artifact writer / agent spawner so no files are written and no process is
7+
// spawned. `impl.test.ts` mocks `howToProceedStep.run` out entirely, so
8+
// nothing there drives this prompt — this file is its dedicated coverage.
9+
const installSkills = vi.hoisted(() => vi.fn())
10+
vi.mock('../../../init/lib/install-skills.js', () => ({ installSkills }))
11+
vi.mock('../../../init/lib/handoff-helpers.js', () => ({
12+
writeArtifacts: vi.fn(),
13+
spawnAgent: vi.fn(async () => 0),
14+
}))
15+
vi.mock('@clack/prompts', () => ({
16+
note: vi.fn(),
17+
log: { success: vi.fn(), info: vi.fn(), warn: vi.fn() },
18+
}))
19+
20+
import * as p from '@clack/prompts'
21+
import { handoffClaudeStep } from '../handoff-claude.js'
22+
23+
// `agents` undefined → Claude "not installed" path, which routes the launch
24+
// prompt into a `p.note` (rather than spawning). That note's body is what we
25+
// assert on.
26+
const state = { integration: 'postgresql' } as unknown as InitState
27+
28+
beforeEach(() => vi.clearAllMocks())
29+
30+
it('launch prompt omits the skills dir when no skills were copied', async () => {
31+
installSkills.mockReturnValue([])
32+
await handoffClaudeStep.run(state)
33+
expect(vi.mocked(p.note).mock.calls[0][0]).not.toContain('.claude/skills/')
34+
})
35+
36+
it('launch prompt names the skills dir when skills were copied', async () => {
37+
installSkills.mockReturnValue(['stash-encryption'])
38+
await handoffClaudeStep.run(state)
39+
expect(vi.mocked(p.note).mock.calls[0][0]).toContain('.claude/skills/')
40+
})
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import { beforeEach, expect, it, vi } from 'vitest'
2+
import type { InitState } from '../../../init/types.js'
3+
4+
// Same seam as the handoff-claude test: the launch prompt's skills clause is
5+
// the unit under test. Mock the skill installer to control whether skills were
6+
// "copied". The Codex handoff also writes AGENTS.md to disk before printing
7+
// the prompt, so stub `node:fs` and the AGENTS.md builders to keep the test
8+
// hermetic (no file lands in the repo) — the assertion is purely on the
9+
// launch-prompt text.
10+
const installSkills = vi.hoisted(() => vi.fn())
11+
vi.mock('../../../init/lib/install-skills.js', () => ({ installSkills }))
12+
vi.mock('../../../init/lib/handoff-helpers.js', () => ({
13+
writeArtifacts: vi.fn(),
14+
spawnAgent: vi.fn(async () => 0),
15+
}))
16+
vi.mock('../../../init/lib/build-agents-md.js', () => ({
17+
buildAgentsMdBody: vi.fn(() => '# managed doctrine'),
18+
}))
19+
vi.mock('../../../init/lib/sentinel-upsert.js', () => ({
20+
upsertManagedBlock: vi.fn(() => '# AGENTS.md'),
21+
}))
22+
vi.mock('node:fs', () => ({
23+
existsSync: vi.fn(() => false),
24+
readFileSync: vi.fn(() => ''),
25+
writeFileSync: vi.fn(),
26+
}))
27+
vi.mock('@clack/prompts', () => ({
28+
note: vi.fn(),
29+
log: { success: vi.fn(), info: vi.fn(), warn: vi.fn() },
30+
}))
31+
32+
import * as p from '@clack/prompts'
33+
import { handoffCodexStep } from '../handoff-codex.js'
34+
35+
// `agents` undefined → Codex "not installed" path, which routes the launch
36+
// prompt into a `p.note`.
37+
const state = { integration: 'postgresql' } as unknown as InitState
38+
39+
beforeEach(() => vi.clearAllMocks())
40+
41+
it('launch prompt drops .codex/skills/ but keeps AGENTS.md when no skills copied', async () => {
42+
installSkills.mockReturnValue([])
43+
await handoffCodexStep.run(state)
44+
const body = vi.mocked(p.note).mock.calls[0][0]
45+
expect(body).not.toContain('.codex/skills/')
46+
// The durable rules live in AGENTS.md regardless, so the prompt still names it.
47+
expect(body).toContain('AGENTS.md')
48+
})
49+
50+
it('launch prompt names .codex/skills/ when skills were copied', async () => {
51+
installSkills.mockReturnValue(['stash-encryption'])
52+
await handoffCodexStep.run(state)
53+
expect(vi.mocked(p.note).mock.calls[0][0]).toContain('.codex/skills/')
54+
})

packages/cli/src/commands/init/__tests__/init-command.test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,4 +114,63 @@ describe('initCommand — honest summary', () => {
114114
'Setup complete',
115115
)
116116
})
117+
118+
it('reports a generated Drizzle migration honestly — not installed, not incomplete', async () => {
119+
// Differential review (PR #687): the Drizzle flow GENERATES an EQL
120+
// migration; `installEqlStep` returns eqlMigrationPending (not
121+
// eqlInstalled). The summary must neither claim "✓ EQL extension
122+
// installed" nor hard-fail as "Setup incomplete" — it should say a
123+
// migration was generated and point at `drizzle-kit migrate`, then exit 0.
124+
eqlRun.mockImplementationOnce(async (s: InitState) => ({
125+
...s,
126+
integration: 'drizzle',
127+
eqlInstalled: false,
128+
eqlMigrationPending: true,
129+
}))
130+
131+
await expect(initCommand({}, {})).resolves.toBeUndefined()
132+
133+
const summary = vi
134+
.mocked(p.note)
135+
.mock.calls.find(([, title]) => title === 'Setup complete')
136+
expect(summary).toBeDefined()
137+
const body = summary?.[0] as string
138+
expect(body).toContain('EQL migration generated')
139+
expect(body).toContain('drizzle-kit migrate')
140+
expect(body).not.toContain('✓ EQL extension installed')
141+
})
142+
143+
it('summary says "kept (existing file)" when an existing client is kept', async () => {
144+
// The three-way encryption-client checkmark fork was untested — the keep
145+
// path (`build-schema` sets clientFilePath + schemaGenerated: false) now
146+
// produces a different string with nothing locking it. This fails against
147+
// the pre-change code, which always claimed "scaffolded".
148+
eqlRun.mockImplementationOnce(async (s: InitState) => ({
149+
...s,
150+
eqlInstalled: true,
151+
clientFilePath: './src/encryption/index.ts',
152+
schemaGenerated: false,
153+
}))
154+
155+
await initCommand({}, {})
156+
expect(vi.mocked(p.note)).toHaveBeenCalledWith(
157+
expect.stringContaining('✓ Encryption client kept (existing file)'),
158+
'Setup complete',
159+
)
160+
})
161+
162+
it('summary says "scaffolded" when a placeholder was written', async () => {
163+
eqlRun.mockImplementationOnce(async (s: InitState) => ({
164+
...s,
165+
eqlInstalled: true,
166+
clientFilePath: './src/encryption/index.ts',
167+
schemaGenerated: true,
168+
}))
169+
170+
await initCommand({}, {})
171+
expect(vi.mocked(p.note)).toHaveBeenCalledWith(
172+
expect.stringContaining('✓ Encryption client scaffolded'),
173+
'Setup complete',
174+
)
175+
})
117176
})

packages/cli/src/commands/init/index.ts

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,15 +120,34 @@ export async function initCommand(
120120
checkmarks.push('✓ `@cipherstash/stack` installed')
121121
}
122122
if (state.cliInstalled) checkmarks.push('✓ `stash` CLI installed')
123-
if (state.eqlInstalled) checkmarks.push('✓ EQL extension installed')
123+
if (state.eqlInstalled) {
124+
checkmarks.push('✓ EQL extension installed')
125+
} else if (state.eqlMigrationPending) {
126+
// The Drizzle flow (and Supabase `--migration` mode) GENERATES an EQL
127+
// migration rather than applying it — EQL isn't in the database until
128+
// the user runs the migration. That's the intended, honest end state
129+
// for these flows (applying is the ORM/migration tool's job), so it's
130+
// NOT an incomplete setup — but we must not claim "installed" either.
131+
const applyCmd =
132+
state.integration === 'supabase'
133+
? 'supabase db push'
134+
: 'drizzle-kit migrate'
135+
checkmarks.push(
136+
`○ EQL migration generated — apply it with \`${applyCmd}\``,
137+
)
138+
}
124139

125-
// EQL is required for encryption. Prisma Next installs it via `migration
126-
// apply` (so `eqlInstalled` is false by design there); every other
127-
// integration needs it installed here. If it's missing, setup is NOT
128-
// complete — say so and exit non-zero so automation can't read a false
129-
// success from a run where encryption would fail at query time.
140+
// EQL is required for encryption. Some integrations install it out-of-band
141+
// and legitimately leave `eqlInstalled` false here: Prisma Next installs it
142+
// via `migration apply`, and the Drizzle flow generates a migration the
143+
// user applies with `drizzle-kit migrate` (`eqlMigrationPending`). Only a
144+
// run that neither installed EQL nor generated a migration to install it is
145+
// genuinely incomplete — say so and exit non-zero so automation can't read
146+
// a false success from a run where encryption would fail at query time.
130147
const eqlPending =
131-
!state.eqlInstalled && state.integration !== 'prisma-next'
148+
!state.eqlInstalled &&
149+
!state.eqlMigrationPending &&
150+
state.integration !== 'prisma-next'
132151
if (eqlPending) {
133152
checkmarks.push('✗ EQL extension NOT installed')
134153
p.note(checkmarks.join('\n'), messages.init.setupIncomplete)

packages/cli/src/commands/init/steps/__tests__/install-eql.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ describe('installEqlStep', () => {
6060
// prompt. init must proceed with the default (install) rather than abort,
6161
// and still scaffold stash.config.ts via the EQL install.
6262
vi.mocked(isInteractive).mockReturnValue(false)
63+
vi.mocked(installCommand).mockResolvedValueOnce('installed')
6364

6465
const result = await installEqlStep.run(baseState, provider)
6566

@@ -69,5 +70,37 @@ describe('installEqlStep', () => {
6970
'ensure',
7071
)
7172
expect(result.eqlInstalled).toBe(true)
73+
expect(result.eqlMigrationPending).toBeFalsy()
74+
})
75+
76+
it('treats an already-installed database as EQL installed', async () => {
77+
vi.mocked(installCommand).mockResolvedValueOnce('already-installed')
78+
79+
const result = await installEqlStep.run(baseState, provider)
80+
81+
expect(result.eqlInstalled).toBe(true)
82+
expect(result.eqlMigrationPending).toBeFalsy()
83+
})
84+
85+
it('maps a generated Drizzle migration to eqlMigrationPending, NOT eqlInstalled', async () => {
86+
// The Drizzle path only WRITES a v2 migration — EQL isn't in the DB until
87+
// the user runs `drizzle-kit migrate`. `installEqlStep` must carry that
88+
// distinction through so `initCommand` doesn't claim "EQL installed".
89+
// This is the seam the differential review flagged (PR #687): the step
90+
// used to return `eqlInstalled: true` for every non-throwing outcome.
91+
const drizzleState = {
92+
integration: 'drizzle',
93+
databaseUrl: 'postgresql://localhost:5432/app',
94+
} as unknown as InitState
95+
vi.mocked(installCommand).mockResolvedValueOnce('migration-generated')
96+
97+
const result = await installEqlStep.run(drizzleState, {
98+
name: 'drizzle',
99+
} as unknown as InitProvider)
100+
101+
// Pinned to v2 for the Drizzle migration path (v3 rejects --drizzle).
102+
expect(vi.mocked(installCommand).mock.calls[0][0].eqlVersion).toBe('2')
103+
expect(result.eqlInstalled).toBe(false)
104+
expect(result.eqlMigrationPending).toBe(true)
72105
})
73106
})

packages/cli/src/commands/init/steps/install-eql.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,9 @@ export const installEqlStep: InitStep = {
9090
return { ...state, eqlInstalled: false }
9191
}
9292

93+
let outcome: Awaited<ReturnType<typeof installCommand>>
9394
try {
94-
await installCommand({
95+
outcome = await installCommand({
9596
supabase: supabase || undefined,
9697
drizzle: drizzle || undefined,
9798
databaseUrl: state.databaseUrl,
@@ -115,6 +116,16 @@ export const installEqlStep: InitStep = {
115116
return { ...state, eqlInstalled: false }
116117
}
117118

119+
// The Drizzle path (and Supabase `--migration` mode) only WRITES a
120+
// migration file — EQL isn't in the database until the user applies it.
121+
// Report that honestly rather than claiming the extension is installed;
122+
// the init summary turns this into "migration generated, apply it".
123+
if (outcome === 'migration-generated') {
124+
return { ...state, eqlInstalled: false, eqlMigrationPending: true }
125+
}
126+
127+
// 'installed' | 'already-installed' — the extension is present in the DB.
128+
// ('dry-run' never happens from init; it doesn't pass dryRun.)
118129
return { ...state, eqlInstalled: true }
119130
},
120131
}

packages/cli/src/commands/init/types.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,16 @@ export interface InitState {
4545
stackInstalled?: boolean
4646
/** True when the `stash` CLI is in the project's devDependencies. */
4747
cliInstalled?: boolean
48-
/** True when EQL was installed (or already-installed) by install-eql. */
48+
/** True when EQL was installed (or already-installed) by install-eql —
49+
* i.e. the extension is actually present in the target database. */
4950
eqlInstalled?: boolean
51+
/** True when install-eql GENERATED a migration file but did not apply it
52+
* (the Drizzle v2 path, or Supabase `--migration` mode). EQL is not in the
53+
* database yet — the user applies it with `drizzle-kit migrate` (or their
54+
* Supabase migration workflow). Distinct from `eqlInstalled` so the init
55+
* summary reports "migration generated, apply it" instead of a false
56+
* "installed" or a spurious "setup incomplete". */
57+
eqlMigrationPending?: boolean
5058
/** Detected ORM / framework integration. Set by build-schema. */
5159
integration?: Integration
5260
/** Schema definitions written to the encryption client. Carries every

skills/stash-cli/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ npx stash init # PostgreSQL / Drizzle / Prisma
3636
npx stash init --supabase # Supabase
3737
```
3838

39-
`stash init` installs the CLI as a project dev dependency, so subsequent commands can drop the `npx`. The CLI is package-manager aware — before init, use whichever one-shot runner your project uses (`npx`, `pnpm dlx`, `bunx`, `yarn dlx`). Installs are **pinned to the exact `@cipherstash/*` versions this CLI release shipped with** (never bare dist-tags, which can lag behind a release), and init flags any already-installed `@cipherstash/*` package whose resolved version differs from the release's. The fix depends on direction, and init says which applies: an **older** install should be aligned to the release (init offers the exact command); a **newer** install must NOT be downgraded — update the `stash` CLI to the matching release instead (init prints that command too). **Non-interactively, an older ("behind") skew is fatal** — init refuses with a non-zero exit and the align command rather than scaffolding against mismatched packages and reporting a false success. Interactively it offers to align. Likewise, if the EQL extension isn't installed at the end (and the integration isn't Prisma Next, which installs it via `migration apply`), init reports **"Setup incomplete"** and exits non-zero — it never claims a setup is complete when encryption would fail at query time.
39+
`stash init` installs the CLI as a project dev dependency, so subsequent commands can drop the `npx`. The CLI is package-manager aware — before init, use whichever one-shot runner your project uses (`npx`, `pnpm dlx`, `bunx`, `yarn dlx`). Installs are **pinned to the exact `@cipherstash/*` versions this CLI release shipped with** (never bare dist-tags, which can lag behind a release), and init flags any already-installed `@cipherstash/*` package whose resolved version differs from the release's. The fix depends on direction, and init says which applies: an **older** install should be aligned to the release (init offers the exact command); a **newer** install must NOT be downgraded — update the `stash` CLI to the matching release instead (init prints that command too). **Non-interactively, an older ("behind") skew is fatal** — init refuses with a non-zero exit and the align command rather than scaffolding against mismatched packages and reporting a false success. Interactively it offers to align. Likewise, if the EQL extension isn't installed at the end, init reports **"Setup incomplete"** and exits non-zero — it never claims a setup is complete when encryption would fail at query time. Integrations that install EQL through a migration are the exception and exit 0: **Prisma Next** installs it via `migration apply`, and the **Drizzle** flow *generates* an EQL migration, which init reports honestly as "EQL migration generated — apply it with `drizzle-kit migrate`" rather than claiming the extension is already installed.
4040

4141
**If you are an agent, do this first:**
4242

0 commit comments

Comments
 (0)