diff --git a/.changeset/honest-noninteractive-init.md b/.changeset/honest-noninteractive-init.md new file mode 100644 index 000000000..b4ae98650 --- /dev/null +++ b/.changeset/honest-noninteractive-init.md @@ -0,0 +1,29 @@ +--- +'stash': minor +--- + +`stash init` is honest non-interactively — it no longer reports success for a +setup that didn't fully complete. + +- **Fails on version skew.** A non-interactive run can't reconcile an + already-installed `@cipherstash/*` package that's *older* than this CLI + expects (it won't mutate an install without consent), so instead of warning + and proceeding — scaffolding against mismatched packages and then claiming + success — it now refuses with a non-zero exit and the exact align command. + Interactive runs still offer to align. A *newer* install stays a warn (the + install is likely fine; update the CLI instead). +- **No false "Setup complete".** If the EQL extension isn't installed at the + end — and the integration isn't one that installs it out-of-band — the + summary reads "Setup incomplete" and init exits non-zero, pointing at + `stash eql install`. Integrations that install EQL via a migration are + reported honestly rather than as failures: Prisma Next (installs it via + `migration apply`) and the Drizzle flow, which *generates* an EQL migration + and now says "EQL migration generated — apply it with `drizzle-kit migrate`" + instead of claiming the extension is already installed. +- **Honest checkmarks.** The summary no longer claims "Database connection + verified" (init resolves a URL but doesn't open a connection) — it now says + "Database URL resolved" — and only shows "Encryption client scaffolded" when + a client was actually written (skipped for Prisma Next). +- **No false "skills loaded".** The agent handoff prompt only points at the + skills directory when skills were actually copied (a stripped build installs + none), instead of telling the agent to read files that aren't there. diff --git a/packages/cli/src/commands/db/install.ts b/packages/cli/src/commands/db/install.ts index 7ce8f6aa0..fd5098e9c 100644 --- a/packages/cli/src/commands/db/install.ts +++ b/packages/cli/src/commands/db/install.ts @@ -169,7 +169,28 @@ async function resolveInstallContext( return { databaseUrl, clientPath } } -export async function installCommand(options: InstallOptions) { +/** + * What `installCommand` actually did — so callers (notably `stash init`'s + * completion gate) can tell "EQL is now in the database" from "a migration + * file was written but nothing was applied yet". + * + * - `installed` / `already-installed`: EQL is present in the target database. + * - `migration-generated`: a migration file was written (Drizzle, or the + * Supabase `--migration` mode); the user still has to APPLY it + * (`drizzle-kit migrate` / `supabase db push`) before EQL exists in the DB. + * - `dry-run`: nothing was changed. + * + * Terminal error paths call `process.exit(1)` and never return an outcome. + */ +export type InstallOutcome = + | 'installed' + | 'already-installed' + | 'migration-generated' + | 'dry-run' + +export async function installCommand( + options: InstallOptions, +): Promise { p.intro(runnerCommand(detectPackageManager(), 'stash eql install')) // Validate mutually-exclusive / supabase-required flags BEFORE doing any @@ -238,7 +259,9 @@ export async function installCommand(options: InstallOptions) { supabase: resolved.supabase, excludeOperatorFamily: resolved.excludeOperatorFamily, }) - return + // A Drizzle migration file was written — NOT applied. EQL only lands in + // the database once the user runs `drizzle-kit migrate`. + return 'migration-generated' } // Supabase non-Drizzle path: pick between writing a migration file and @@ -270,7 +293,9 @@ export async function installCommand(options: InstallOptions) { force: options.force, dryRun: options.dryRun, }) - return + // Migration file written, not applied — the user runs it via their + // Supabase migration workflow (`supabase db push`). + return 'migration-generated' } // mode === 'direct' — fall through to existing direct-install behavior. } @@ -282,7 +307,7 @@ export async function installCommand(options: InstallOptions) { : `Would use bundled EQL${eqlVersion === 3 ? ' v3' : ''} install script` p.note(`${source}\nWould execute the SQL against the database`, 'Dry Run') p.outro('Dry run complete.') - return + return 'dry-run' } const installer = new EQLInstaller({ @@ -331,7 +356,7 @@ export async function installCommand(options: InstallOptions) { if (installed) { p.log.info('Use --force to re-run the install script.') p.outro('Nothing to do.') - return + return 'already-installed' } } @@ -370,6 +395,7 @@ export async function installCommand(options: InstallOptions) { printNextSteps() p.outro('Done!') + return 'installed' } /** diff --git a/packages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.ts b/packages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.ts new file mode 100644 index 000000000..f82103438 --- /dev/null +++ b/packages/cli/src/commands/impl/steps/__tests__/handoff-claude.test.ts @@ -0,0 +1,40 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import type { InitState } from '../../../init/types.js' + +// The launch prompt's skills clause is the unit under test. Mock the skill +// installer so we control whether any skills were "copied", and stub the +// artifact writer / agent spawner so no files are written and no process is +// spawned. `impl.test.ts` mocks `howToProceedStep.run` out entirely, so +// nothing there drives this prompt — this file is its dedicated coverage. +const installSkills = vi.hoisted(() => vi.fn()) +vi.mock('../../../init/lib/install-skills.js', () => ({ installSkills })) +vi.mock('../../../init/lib/handoff-helpers.js', () => ({ + writeArtifacts: vi.fn(), + spawnAgent: vi.fn(async () => 0), +})) +vi.mock('@clack/prompts', () => ({ + note: vi.fn(), + log: { success: vi.fn(), info: vi.fn(), warn: vi.fn() }, +})) + +import * as p from '@clack/prompts' +import { handoffClaudeStep } from '../handoff-claude.js' + +// `agents` undefined → Claude "not installed" path, which routes the launch +// prompt into a `p.note` (rather than spawning). That note's body is what we +// assert on. +const state = { integration: 'postgresql' } as unknown as InitState + +beforeEach(() => vi.clearAllMocks()) + +it('launch prompt omits the skills dir when no skills were copied', async () => { + installSkills.mockReturnValue([]) + await handoffClaudeStep.run(state) + expect(vi.mocked(p.note).mock.calls[0][0]).not.toContain('.claude/skills/') +}) + +it('launch prompt names the skills dir when skills were copied', async () => { + installSkills.mockReturnValue(['stash-encryption']) + await handoffClaudeStep.run(state) + expect(vi.mocked(p.note).mock.calls[0][0]).toContain('.claude/skills/') +}) diff --git a/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts b/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts new file mode 100644 index 000000000..b095fa981 --- /dev/null +++ b/packages/cli/src/commands/impl/steps/__tests__/handoff-codex.test.ts @@ -0,0 +1,54 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import type { InitState } from '../../../init/types.js' + +// Same seam as the handoff-claude test: the launch prompt's skills clause is +// the unit under test. Mock the skill installer to control whether skills were +// "copied". The Codex handoff also writes AGENTS.md to disk before printing +// the prompt, so stub `node:fs` and the AGENTS.md builders to keep the test +// hermetic (no file lands in the repo) — the assertion is purely on the +// launch-prompt text. +const installSkills = vi.hoisted(() => vi.fn()) +vi.mock('../../../init/lib/install-skills.js', () => ({ installSkills })) +vi.mock('../../../init/lib/handoff-helpers.js', () => ({ + writeArtifacts: vi.fn(), + spawnAgent: vi.fn(async () => 0), +})) +vi.mock('../../../init/lib/build-agents-md.js', () => ({ + buildAgentsMdBody: vi.fn(() => '# managed doctrine'), +})) +vi.mock('../../../init/lib/sentinel-upsert.js', () => ({ + upsertManagedBlock: vi.fn(() => '# AGENTS.md'), +})) +vi.mock('node:fs', () => ({ + existsSync: vi.fn(() => false), + readFileSync: vi.fn(() => ''), + writeFileSync: vi.fn(), +})) +vi.mock('@clack/prompts', () => ({ + note: vi.fn(), + log: { success: vi.fn(), info: vi.fn(), warn: vi.fn() }, +})) + +import * as p from '@clack/prompts' +import { handoffCodexStep } from '../handoff-codex.js' + +// `agents` undefined → Codex "not installed" path, which routes the launch +// prompt into a `p.note`. +const state = { integration: 'postgresql' } as unknown as InitState + +beforeEach(() => vi.clearAllMocks()) + +it('launch prompt drops .codex/skills/ but keeps AGENTS.md when no skills copied', async () => { + installSkills.mockReturnValue([]) + await handoffCodexStep.run(state) + const body = vi.mocked(p.note).mock.calls[0][0] + expect(body).not.toContain('.codex/skills/') + // The durable rules live in AGENTS.md regardless, so the prompt still names it. + expect(body).toContain('AGENTS.md') +}) + +it('launch prompt names .codex/skills/ when skills were copied', async () => { + installSkills.mockReturnValue(['stash-encryption']) + await handoffCodexStep.run(state) + expect(vi.mocked(p.note).mock.calls[0][0]).toContain('.codex/skills/') +}) diff --git a/packages/cli/src/commands/impl/steps/handoff-claude.ts b/packages/cli/src/commands/impl/steps/handoff-claude.ts index 41d2c4fba..07e1cf980 100644 --- a/packages/cli/src/commands/impl/steps/handoff-claude.ts +++ b/packages/cli/src/commands/impl/steps/handoff-claude.ts @@ -35,10 +35,17 @@ export const handoffClaudeStep: HandoffStep = { writeArtifacts(cwd, state, 'claude-code', installed) const mode = state.mode ?? 'implement' + // Only point the agent at the skills dir when skills were actually copied. + // A stripped CLI build (no bundled skills) returns [] — claiming they're + // there would send the agent to read files that don't exist. + const skillsClause = + installed.length > 0 + ? `The installed skills under ${CLAUDE_SKILLS_DIR}/ have the rules; ` + : '' const launchPrompt = mode === 'plan' - ? `Read ${SETUP_PROMPT_REL_PATH} and produce the planning deliverable it describes. The installed skills under ${CLAUDE_SKILLS_DIR}/ have the rules; ${CONTEXT_REL_PATH} has the project facts. Do not edit code or run mutating commands during this phase.` - : `Read ${SETUP_PROMPT_REL_PATH} and complete the setup steps. The installed skills under ${CLAUDE_SKILLS_DIR}/ have the rules; ${CONTEXT_REL_PATH} has the project facts.` + ? `Read ${SETUP_PROMPT_REL_PATH} and produce the planning deliverable it describes. ${skillsClause}${CONTEXT_REL_PATH} has the project facts. Do not edit code or run mutating commands during this phase.` + : `Read ${SETUP_PROMPT_REL_PATH} and complete the setup steps. ${skillsClause}${CONTEXT_REL_PATH} has the project facts.` if (!state.agents?.cli.claudeCode) { p.note( diff --git a/packages/cli/src/commands/impl/steps/handoff-codex.ts b/packages/cli/src/commands/impl/steps/handoff-codex.ts index 662fa7b2a..6ad7278a1 100644 --- a/packages/cli/src/commands/impl/steps/handoff-codex.ts +++ b/packages/cli/src/commands/impl/steps/handoff-codex.ts @@ -54,10 +54,17 @@ export const handoffCodexStep: HandoffStep = { writeArtifacts(cwd, state, 'codex', installed) const mode = state.mode ?? 'implement' + // Only reference the skills dir when skills were actually copied (a + // stripped build returns []); the durable rules live in AGENTS.md either + // way, so the prompt stays useful without them. + const skillsClause = + installed.length > 0 + ? `the skills under ${CODEX_SKILLS_DIR}/ have the API details; ` + : '' const launchPrompt = mode === 'plan' - ? `Read ${SETUP_PROMPT_REL_PATH} and produce the planning deliverable it describes. AGENTS.md has the durable rules; the skills under ${CODEX_SKILLS_DIR}/ have the API details; ${CONTEXT_REL_PATH} has the project facts. Do not edit code or run mutating commands during this phase.` - : `Read ${SETUP_PROMPT_REL_PATH} and complete the setup steps. AGENTS.md has the durable rules; the skills under ${CODEX_SKILLS_DIR}/ have the API details; ${CONTEXT_REL_PATH} has the project facts.` + ? `Read ${SETUP_PROMPT_REL_PATH} and produce the planning deliverable it describes. AGENTS.md has the durable rules; ${skillsClause}${CONTEXT_REL_PATH} has the project facts. Do not edit code or run mutating commands during this phase.` + : `Read ${SETUP_PROMPT_REL_PATH} and complete the setup steps. AGENTS.md has the durable rules; ${skillsClause}${CONTEXT_REL_PATH} has the project facts.` if (!state.agents?.cli.codex) { p.note( diff --git a/packages/cli/src/commands/init/__tests__/init-command.test.ts b/packages/cli/src/commands/init/__tests__/init-command.test.ts index 893ca718c..3064d93f5 100644 --- a/packages/cli/src/commands/init/__tests__/init-command.test.ts +++ b/packages/cli/src/commands/init/__tests__/init-command.test.ts @@ -1,4 +1,7 @@ +import * as p from '@clack/prompts' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { CliExit } from '../../../cli/exit.js' +import { messages } from '../../../messages.js' import type { InitState } from '../types.js' // `--region` is the non-interactive escape hatch for `stash init`; it must land @@ -9,6 +12,10 @@ import type { InitState } from '../types.js' // (`authenticate`, `install-deps`) out of the fast suite. const authRun = vi.hoisted(() => vi.fn(async (state: InitState) => state)) const passthrough = { run: async (s: InitState) => s } +// Controllable so the honest-summary tests can vary whether EQL installed. +const eqlRun = vi.hoisted(() => + vi.fn(async (s: InitState) => ({ ...s, eqlInstalled: true })), +) vi.mock('../steps/authenticate.js', () => ({ authenticateStep: { id: 'authenticate', name: 'Authenticate', run: authRun }, @@ -26,7 +33,10 @@ vi.mock('../steps/install-deps.js', () => ({ installDepsStep: { id: 'install-deps', ...passthrough }, })) vi.mock('../steps/install-eql.js', () => ({ - installEqlStep: { id: 'install-eql', ...passthrough }, + // A successful init installs EQL — the default mark keeps the honest-summary + // gate (`eqlPending` → exit 1) happy for the region-threading runs. The + // honest-summary tests override `eqlRun` per case. + installEqlStep: { id: 'install-eql', run: eqlRun }, })) vi.mock('../steps/gather-context.js', () => ({ gatherContextStep: { id: 'gather-context', ...passthrough }, @@ -70,3 +80,97 @@ describe('initCommand — region threading', () => { expect(stateArg.regionFlag).toBeUndefined() }) }) + +describe('initCommand — honest summary', () => { + it('exits non-zero and reports "Setup incomplete" when EQL was not installed', async () => { + eqlRun.mockImplementationOnce(async (s: InitState) => ({ + ...s, + eqlInstalled: false, + })) + + await expect(initCommand({}, {})).rejects.toBeInstanceOf(CliExit) + // The summary titles the run as incomplete, and the EQL fix is surfaced. + expect(vi.mocked(p.note)).toHaveBeenCalledWith( + expect.any(String), + messages.init.setupIncomplete, + ) + expect(vi.mocked(p.log.error)).toHaveBeenCalledWith( + expect.stringContaining(messages.init.eqlNotInstalled), + ) + }) + + it('completes (no throw) when EQL was not installed but the integration is prisma-next', async () => { + // Prisma Next installs EQL via `migration apply`, so eqlInstalled=false is + // expected there and must NOT be treated as an incomplete setup. + eqlRun.mockImplementationOnce(async (s: InitState) => ({ + ...s, + integration: 'prisma-next', + eqlInstalled: false, + })) + + await expect(initCommand({}, {})).resolves.toBeUndefined() + expect(vi.mocked(p.note)).toHaveBeenCalledWith( + expect.any(String), + 'Setup complete', + ) + }) + + it('reports a generated Drizzle migration honestly — not installed, not incomplete', async () => { + // Differential review (PR #687): the Drizzle flow GENERATES an EQL + // migration; `installEqlStep` returns eqlMigrationPending (not + // eqlInstalled). The summary must neither claim "✓ EQL extension + // installed" nor hard-fail as "Setup incomplete" — it should say a + // migration was generated and point at `drizzle-kit migrate`, then exit 0. + eqlRun.mockImplementationOnce(async (s: InitState) => ({ + ...s, + integration: 'drizzle', + eqlInstalled: false, + eqlMigrationPending: true, + })) + + await expect(initCommand({}, {})).resolves.toBeUndefined() + + const summary = vi + .mocked(p.note) + .mock.calls.find(([, title]) => title === 'Setup complete') + expect(summary).toBeDefined() + const body = summary?.[0] as string + expect(body).toContain('EQL migration generated') + expect(body).toContain('drizzle-kit migrate') + expect(body).not.toContain('✓ EQL extension installed') + }) + + it('summary says "kept (existing file)" when an existing client is kept', async () => { + // The three-way encryption-client checkmark fork was untested — the keep + // path (`build-schema` sets clientFilePath + schemaGenerated: false) now + // produces a different string with nothing locking it. This fails against + // the pre-change code, which always claimed "scaffolded". + eqlRun.mockImplementationOnce(async (s: InitState) => ({ + ...s, + eqlInstalled: true, + clientFilePath: './src/encryption/index.ts', + schemaGenerated: false, + })) + + await initCommand({}, {}) + expect(vi.mocked(p.note)).toHaveBeenCalledWith( + expect.stringContaining('✓ Encryption client kept (existing file)'), + 'Setup complete', + ) + }) + + it('summary says "scaffolded" when a placeholder was written', async () => { + eqlRun.mockImplementationOnce(async (s: InitState) => ({ + ...s, + eqlInstalled: true, + clientFilePath: './src/encryption/index.ts', + schemaGenerated: true, + })) + + await initCommand({}, {}) + expect(vi.mocked(p.note)).toHaveBeenCalledWith( + expect.stringContaining('✓ Encryption client scaffolded'), + 'Setup complete', + ) + }) +}) diff --git a/packages/cli/src/commands/init/index.ts b/packages/cli/src/commands/init/index.ts index 630ffdc06..517eaa65d 100644 --- a/packages/cli/src/commands/init/index.ts +++ b/packages/cli/src/commands/init/index.ts @@ -1,5 +1,6 @@ import * as p from '@clack/prompts' import { CliExit } from '../../cli/exit.js' +import { messages } from '../../messages.js' import { HANDOFF_CHOICES } from '../impl/steps/how-to-proceed.js' import { planCommand } from '../plan/index.js' import { createBaseProvider } from './providers/base.js' @@ -99,16 +100,62 @@ export async function initCommand( const pm = detectPackageManager() const cli = runnerCommand(pm, 'stash') + // Only claim what actually happened. Auth throws on failure (reaching here + // means it succeeded); the database step *resolves* a URL but never opens a + // connection, so don't claim "verified"; the client scaffold is skipped for + // Prisma Next (no `clientFilePath` on state). `schemaGenerated` is true only + // when a placeholder was actually written — when an existing client file is + // kept, `clientFilePath` is still set but nothing was scaffolded, so don't + // claim we did. const checkmarks: string[] = [ '✓ Authenticated to CipherStash', - '✓ Database connection verified', - '✓ Encryption client scaffolded', + '✓ Database URL resolved', ] + if (state.schemaGenerated) { + checkmarks.push('✓ Encryption client scaffolded') + } else if (state.clientFilePath) { + checkmarks.push('✓ Encryption client kept (existing file)') + } if (state.stackInstalled) { checkmarks.push('✓ `@cipherstash/stack` installed') } if (state.cliInstalled) checkmarks.push('✓ `stash` CLI installed') - if (state.eqlInstalled) checkmarks.push('✓ EQL extension installed') + if (state.eqlInstalled) { + checkmarks.push('✓ EQL extension installed') + } else if (state.eqlMigrationPending) { + // The Drizzle flow (and Supabase `--migration` mode) GENERATES an EQL + // migration rather than applying it — EQL isn't in the database until + // the user runs the migration. That's the intended, honest end state + // for these flows (applying is the ORM/migration tool's job), so it's + // NOT an incomplete setup — but we must not claim "installed" either. + const applyCmd = + state.integration === 'supabase' + ? 'supabase db push' + : 'drizzle-kit migrate' + checkmarks.push( + `○ EQL migration generated — apply it with \`${applyCmd}\``, + ) + } + + // EQL is required for encryption. Some integrations install it out-of-band + // and legitimately leave `eqlInstalled` false here: Prisma Next installs it + // via `migration apply`, and the Drizzle flow generates a migration the + // user applies with `drizzle-kit migrate` (`eqlMigrationPending`). Only a + // run that neither installed EQL nor generated a migration to install it is + // genuinely incomplete — say so and exit non-zero so automation can't read + // a false success from a run where encryption would fail at query time. + const eqlPending = + !state.eqlInstalled && + !state.eqlMigrationPending && + state.integration !== 'prisma-next' + if (eqlPending) { + checkmarks.push('✗ EQL extension NOT installed') + p.note(checkmarks.join('\n'), messages.init.setupIncomplete) + p.log.error( + `${messages.init.eqlNotInstalled} Run \`${cli} eql install\` before running any encryption.`, + ) + throw new CliExit(1) + } p.note(checkmarks.join('\n'), 'Setup complete') diff --git a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts index 9211804e2..c36bad2cc 100644 --- a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts @@ -409,3 +409,50 @@ describe('renderSetupPrompt — no db push recommendations', () => { } }) }) + +describe('renderSetupPrompt — honours what the handoff actually wrote', () => { + for (const mode of ['implement', 'plan'] as const) { + it(`claude-code with no skills points at neither a skills dir nor AGENTS.md (${mode})`, () => { + const out = renderSetupPrompt({ + ...baseCtx, + mode, + handoff: 'claude-code', + installedSkills: [], + }) + // Nothing was written, so don't send the agent to files that don't exist. + expect(out).not.toContain('.claude/skills/') + expect(out).not.toContain('Read the skills') + // It may NAME AGENTS.md to say it was NOT written, but must not point the + // agent at it as a rules source (this handoff never writes one). + expect(out).not.toMatch( + /(?:rules are in|doctrine in|[Rr]ead)[^\n]*AGENTS\.md/, + ) + expect(out).toContain('No skills or `AGENTS.md` were written') + expect(out).toContain('cipherstash.com/docs') + }) + + it(`codex with no skills points at AGENTS.md, not .codex/skills/ (${mode})`, () => { + const out = renderSetupPrompt({ + ...baseCtx, + mode, + handoff: 'codex', + installedSkills: [], + }) + expect(out).not.toContain('.codex/skills/') + expect(out).toContain('AGENTS.md') + }) + + it(`claude-code with skills does not claim the doctrine is in AGENTS.md (${mode})`, () => { + // The Claude handoff never writes AGENTS.md — the doctrine is in the + // installed skills. + const out = renderSetupPrompt({ + ...baseCtx, + mode, + handoff: 'claude-code', + installedSkills: ['stash-encryption'], + }) + expect(out).toContain('.claude/skills/') + expect(out).not.toContain('AGENTS.md') + }) + } +}) diff --git a/packages/cli/src/commands/init/lib/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index 550e0cff3..bfa178049 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -144,6 +144,45 @@ function renderSkillIndex(installedSkills: string[]): string { .join('\n') } +/** + * The "## Skills loaded" section, honouring what the handoff actually wrote — + * so the prompt never points the agent at files that don't exist: + * + * - No skills installed (a stripped CLI build): don't reference any skill + * directory; point only at whatever durable rules the handoff wrote + * (`AGENTS.md` for codex / agents-md; nothing for claude-code, so send the + * agent to the docs). + * - claude-code: the doctrine lives in the installed skills, not `AGENTS.md` + * (this handoff never writes one) — so don't name `AGENTS.md`. + */ +function skillsLoadedLines( + handoff: HandoffChoice, + installedSkills: string[], +): string[] { + const wroteAgentsMd = handoff === 'codex' || handoff === 'agents-md' + if (installedSkills.length === 0) { + return [ + '## Rules', + '', + wroteAgentsMd + ? 'No skills were installed (stripped build) — the durable rules are in `AGENTS.md`; read it before answering API or pattern questions.' + : 'No skills or `AGENTS.md` were written (stripped build) — consult https://cipherstash.com/docs for the encryption API, schema rules, and the rollout/cutover lifecycle.', + ] + } + const doctrine = wroteAgentsMd + ? 'Read the skills before answering API or pattern questions. The doctrine in `AGENTS.md` covers the invariants that apply regardless of which flow you take — never log plaintext, never `.notNull()` on creation, etc.' + : 'Read the skills before answering API or pattern questions — they carry the invariants that apply regardless of which flow you take: never log plaintext, never `.notNull()` on creation, etc.' + return [ + '## Skills loaded', + '', + `Reusable rules and worked examples live in ${rulesLocation(handoff)}:`, + '', + renderSkillIndex(installedSkills), + '', + doctrine, + ] +} + /** * Render the project-specific action prompt. Dispatches to the plan-mode or * implement-mode renderer based on `ctx.mode`. Both produce the same shape @@ -227,13 +266,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { '', ...setupChecklist(ctx), '', - '## Skills loaded', - '', - `Reusable rules and worked examples live in ${rulesLocation(ctx.handoff)}:`, - '', - renderSkillIndex(ctx.installedSkills), - '', - 'Read the skills before answering API or pattern questions. The doctrine in `AGENTS.md` (or its inlined equivalent) covers the invariants that apply regardless of which flow you take — never log plaintext, never `.notNull()` on creation, etc.', + ...skillsLoadedLines(ctx.handoff, ctx.installedSkills), '', '## The two options', '', @@ -283,7 +316,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { '', '## Your first response', '', - `Before any edits, send the user a short orientation message. Confirm setup is complete, list the skills loaded with one-line purposes, summarise the two options in your own words, and end with a clear question — *"Which would you like to do? You can name a specific table+column or describe what you're trying to protect."* Reference concrete tables/columns from \`.cipherstash/context.json\` when it helps. Mention that they can run \`${cli} status\` at any time to see where each rollout is.`, + `Before any edits, send the user a short orientation message. Confirm setup is complete, list the skills loaded (if any) with one-line purposes, summarise the two options in your own words, and end with a clear question — *"Which would you like to do? You can name a specific table+column or describe what you're trying to protect."* Reference concrete tables/columns from \`.cipherstash/context.json\` when it helps. Mention that they can run \`${cli} status\` at any time to see where each rollout is.`, '', 'Once the user answers, execute the relevant flow. Show diffs / generated SQL before applying. Pause for review at every database-mutating step.', '', @@ -359,13 +392,7 @@ function planSharedSetupBlock(ctx: SetupPromptContext): string[] { '', ...setupChecklist(ctx), '', - '## Skills loaded', - '', - `Reusable rules and worked examples live in ${rulesLocation(ctx.handoff)}:`, - '', - renderSkillIndex(ctx.installedSkills), - '', - 'Read the skills before answering API or pattern questions. The doctrine in `AGENTS.md` (or its inlined equivalent) covers the invariants that apply regardless of which flow you take — never log plaintext, never `.notNull()` on creation, etc.', + ...skillsLoadedLines(ctx.handoff, ctx.installedSkills), '', ] } @@ -497,7 +524,7 @@ function renderRolloutPlanPrompt(ctx: SetupPromptContext): string { ...planSharedNotDoBlock(ctx), '## Your first response', '', - `Send the user a short orientation message before writing anything. Confirm setup is complete, list the skills loaded with one-line purposes, explain what an encryption rollout is in your own words, and end with a clear question — *"Which table(s) and column(s) would you like the rollout plan to cover? You can name them or describe what you're trying to protect."* Reference concrete tables/columns from \`.cipherstash/context.json\` when it helps.`, + `Send the user a short orientation message before writing anything. Confirm setup is complete, list the skills loaded (if any) with one-line purposes, explain what an encryption rollout is in your own words, and end with a clear question — *"Which table(s) and column(s) would you like the rollout plan to cover? You can name them or describe what you're trying to protect."* Reference concrete tables/columns from \`.cipherstash/context.json\` when it helps.`, '', `Once the user answers, write \`${PLAN_REL_PATH}\`. Show the plan in chat as well so the user can react inline. After the plan is approved, tell the user to run \`${cli} impl\` to execute it.`, '', diff --git a/packages/cli/src/commands/init/steps/__tests__/install-deps.test.ts b/packages/cli/src/commands/init/steps/__tests__/install-deps.test.ts index 160c6fa79..0f069c5a0 100644 --- a/packages/cli/src/commands/init/steps/__tests__/install-deps.test.ts +++ b/packages/cli/src/commands/init/steps/__tests__/install-deps.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { CliExit } from '../../../../cli/exit.js' import type { InitProvider, InitState } from '../../types.js' const execSyncMock = vi.hoisted(() => vi.fn()) @@ -199,7 +200,7 @@ describe('installDepsStep', () => { ) }) - it('non-interactive: warns on skew, prints align commands, never mutates', async () => { + it('non-interactive: REFUSES on skew (exit 1), prints align commands, never mutates', async () => { vi.mocked(isInteractive).mockReturnValue(false) present('@cipherstash/stack', 'stash') resolvedVersions({ @@ -207,7 +208,12 @@ describe('installDepsStep', () => { stash: FIXTURE_VERSIONS.stash, }) - const result = await installDepsStep.run(baseState, provider) + // M4: a non-interactive run can't reconcile a `behind` skew (won't mutate + // without consent), so it refuses with a non-zero exit rather than + // proceeding and reporting a false success. + await expect( + installDepsStep.run(baseState, provider), + ).rejects.toBeInstanceOf(CliExit) expect(p.log.warn).toHaveBeenCalledWith( expect.stringContaining('@cipherstash/stack: installed 0.19.0'), @@ -217,9 +223,8 @@ describe('installDepsStep', () => { expect.stringContaining('npm install @cipherstash/stack@9.9.9-test.1'), 'Version skew', ) + // Never mutates — the #661/#666 principle survives the refusal. expect(execSyncMock).not.toHaveBeenCalled() - expect(result.stackInstalled).toBe(true) - expect(result.cliInstalled).toBe(true) }) it('a NEWER install gets an update-stash warning, never a downgrade command', async () => { @@ -284,7 +289,11 @@ describe('installDepsStep', () => { stash: FIXTURE_VERSIONS.stash, }) - await installDepsStep.run(baseState, provider) + // An unreadable manifest classifies as `behind` skew, so a non-interactive + // run refuses (exit 1) after warning — same as any other skew. + await expect( + installDepsStep.run(baseState, provider), + ).rejects.toBeInstanceOf(CliExit) expect(p.log.warn).toHaveBeenCalledWith( expect.stringContaining( diff --git a/packages/cli/src/commands/init/steps/__tests__/install-eql.test.ts b/packages/cli/src/commands/init/steps/__tests__/install-eql.test.ts index 470605cdd..dfa3efbfd 100644 --- a/packages/cli/src/commands/init/steps/__tests__/install-eql.test.ts +++ b/packages/cli/src/commands/init/steps/__tests__/install-eql.test.ts @@ -60,6 +60,7 @@ describe('installEqlStep', () => { // prompt. init must proceed with the default (install) rather than abort, // and still scaffold stash.config.ts via the EQL install. vi.mocked(isInteractive).mockReturnValue(false) + vi.mocked(installCommand).mockResolvedValueOnce('installed') const result = await installEqlStep.run(baseState, provider) @@ -69,5 +70,37 @@ describe('installEqlStep', () => { 'ensure', ) expect(result.eqlInstalled).toBe(true) + expect(result.eqlMigrationPending).toBeFalsy() + }) + + it('treats an already-installed database as EQL installed', async () => { + vi.mocked(installCommand).mockResolvedValueOnce('already-installed') + + const result = await installEqlStep.run(baseState, provider) + + expect(result.eqlInstalled).toBe(true) + expect(result.eqlMigrationPending).toBeFalsy() + }) + + it('maps a generated Drizzle migration to eqlMigrationPending, NOT eqlInstalled', async () => { + // The Drizzle path only WRITES a v2 migration — EQL isn't in the DB until + // the user runs `drizzle-kit migrate`. `installEqlStep` must carry that + // distinction through so `initCommand` doesn't claim "EQL installed". + // This is the seam the differential review flagged (PR #687): the step + // used to return `eqlInstalled: true` for every non-throwing outcome. + const drizzleState = { + integration: 'drizzle', + databaseUrl: 'postgresql://localhost:5432/app', + } as unknown as InitState + vi.mocked(installCommand).mockResolvedValueOnce('migration-generated') + + const result = await installEqlStep.run(drizzleState, { + name: 'drizzle', + } as unknown as InitProvider) + + // Pinned to v2 for the Drizzle migration path (v3 rejects --drizzle). + expect(vi.mocked(installCommand).mock.calls[0][0].eqlVersion).toBe('2') + expect(result.eqlInstalled).toBe(false) + expect(result.eqlMigrationPending).toBe(true) }) }) diff --git a/packages/cli/src/commands/init/steps/install-deps.ts b/packages/cli/src/commands/init/steps/install-deps.ts index 0853d73bc..8357f11ca 100644 --- a/packages/cli/src/commands/init/steps/install-deps.ts +++ b/packages/cli/src/commands/init/steps/install-deps.ts @@ -1,6 +1,8 @@ import { execSync } from 'node:child_process' import * as p from '@clack/prompts' +import { CliExit } from '../../../cli/exit.js' import { isInteractive } from '../../../config/tty.js' +import { messages } from '../../../messages.js' import { compareVersions, expectedVersion, @@ -146,9 +148,12 @@ function splitProdDev(packages: readonly string[]): { * before any prompt or early exit, so no path (decline, partial failure, * everything-already-present) proceeds silently on a stale or placeholder * install. Interactively, init offers to align the skewed packages to this - * release in the same confirm as the missing installs; non-interactively it - * NEVER mutates an existing install — it warns, prints the exact align - * commands, and proceeds. + * release in the same confirm as the missing installs. Non-interactively it + * still NEVER mutates an existing install without consent — but rather than + * proceeding on a `behind` skew (which would scaffold against packages older + * than this CLI expects and then report a false success), it REFUSES with a + * non-zero exit and the exact align commands (M4). An `ahead` skew is not + * fatal — the install is likely fine and the fix is updating the CLI. * * When everything is already present at matching versions this logs a * success line and moves on with no prompts. @@ -193,6 +198,22 @@ export const installDepsStep: InitStep = { p.log.warn(`Version skew detected:\n ${skewLines(skewed)}`) } + // A non-interactive run can't reconcile a `behind` skew: it won't mutate an + // existing install without consent (the #661/#666 rule), so proceeding + // would scaffold config/client against packages older than this CLI + // expects and then report a false success. Refuse with a non-zero exit and + // the exact align commands, instead of warning-and-continuing. (Interactive + // runs still offer to align — see below. `ahead` skew is handled + // separately: the install is likely fine, so it warns and proceeds.) + if (skewed.length > 0 && !isInteractive()) { + p.note( + `Align these packages, then re-run init:\n ${alignCommands.join('\n ')}`, + 'Version skew', + ) + p.log.error(messages.init.skewNonInteractive) + throw new CliExit(1) + } + // What's missing outright (pinned, prod/dev split). const missing: string[] = [] if (!stackPresent) missing.push(STACK_PACKAGE) diff --git a/packages/cli/src/commands/init/steps/install-eql.ts b/packages/cli/src/commands/init/steps/install-eql.ts index 5ebc682e9..2cf0d28ed 100644 --- a/packages/cli/src/commands/init/steps/install-eql.ts +++ b/packages/cli/src/commands/init/steps/install-eql.ts @@ -90,8 +90,9 @@ export const installEqlStep: InitStep = { return { ...state, eqlInstalled: false } } + let outcome: Awaited> try { - await installCommand({ + outcome = await installCommand({ supabase: supabase || undefined, drizzle: drizzle || undefined, databaseUrl: state.databaseUrl, @@ -115,6 +116,16 @@ export const installEqlStep: InitStep = { return { ...state, eqlInstalled: false } } + // The Drizzle path (and Supabase `--migration` mode) only WRITES a + // migration file — EQL isn't in the database until the user applies it. + // Report that honestly rather than claiming the extension is installed; + // the init summary turns this into "migration generated, apply it". + if (outcome === 'migration-generated') { + return { ...state, eqlInstalled: false, eqlMigrationPending: true } + } + + // 'installed' | 'already-installed' — the extension is present in the DB. + // ('dry-run' never happens from init; it doesn't pass dryRun.) return { ...state, eqlInstalled: true } }, } diff --git a/packages/cli/src/commands/init/types.ts b/packages/cli/src/commands/init/types.ts index cd15ab7af..333f9e4ff 100644 --- a/packages/cli/src/commands/init/types.ts +++ b/packages/cli/src/commands/init/types.ts @@ -45,8 +45,16 @@ export interface InitState { stackInstalled?: boolean /** True when the `stash` CLI is in the project's devDependencies. */ cliInstalled?: boolean - /** True when EQL was installed (or already-installed) by install-eql. */ + /** True when EQL was installed (or already-installed) by install-eql — + * i.e. the extension is actually present in the target database. */ eqlInstalled?: boolean + /** True when install-eql GENERATED a migration file but did not apply it + * (the Drizzle v2 path, or Supabase `--migration` mode). EQL is not in the + * database yet — the user applies it with `drizzle-kit migrate` (or their + * Supabase migration workflow). Distinct from `eqlInstalled` so the init + * summary reports "migration generated, apply it" instead of a false + * "installed" or a spurious "setup incomplete". */ + eqlMigrationPending?: boolean /** Detected ORM / framework integration. Set by build-schema. */ integration?: Integration /** Schema definitions written to the encryption client. Carries every diff --git a/packages/cli/src/messages.ts b/packages/cli/src/messages.ts index 90bc52bb3..270141d96 100644 --- a/packages/cli/src/messages.ts +++ b/packages/cli/src/messages.ts @@ -139,6 +139,17 @@ export const messages = { completeRolloutConfirmed: 'Proceeding with --yes: the production-deploy gate is skipped', }, + init: { + /** + * Honest non-interactive init. These exit non-zero so automation never + * reads a false success — the e2e suite asserts on the leaders. + */ + setupIncomplete: 'Setup incomplete', + eqlNotInstalled: 'EQL is not installed — encryption queries will fail.', + /** Shown when a non-interactive run hits version skew it won't reconcile. */ + skewNonInteractive: + 'Version skew on already-installed packages — refusing to proceed non-interactively. Align the packages (below) and re-run, or run init interactively.', + }, telemetry: { /** * The one-time first-run notice. Printed to stderr so it never pollutes diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index a075af6e8..a0e187cce 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -36,7 +36,7 @@ npx stash init # PostgreSQL / Drizzle / Prisma npx stash init --supabase # Supabase ``` -`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 warns if an already-installed package's resolved version differs from the release's. Treat that warning as a real problem — but 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). +`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. **If you are an agent, do this first:**