diff --git a/.changeset/init-installs-agent-skills.md b/.changeset/init-installs-agent-skills.md new file mode 100644 index 000000000..d5b571ed1 --- /dev/null +++ b/.changeset/init-installs-agent-skills.md @@ -0,0 +1,45 @@ +--- +'stash': patch +--- + +`stash init` installs the agent skills again, and does it first. + +Since 1.0.0-rc.4 the only callers of the skills installer were the `plan` and +`impl` handoff steps, which `stash init` never reaches — so `stash@1.1.0` +installed no `stash-*` skills for anyone, in any mode. The most common flow, a +coding agent running `npx stash init --supabase` inside a project, completed +with a green summary, a plausible-looking `.cipherstash/context.json`, and zero +guidance: the skills sat unread in `node_modules/stash/dist/skills/` unless the +agent thought to go digging. Fixes #923. + +Init now copies the per-integration skills into `.claude/skills/` (Claude Code +detected via the `claude` binary or a `.claude/` directory) and `.codex/skills/` +(Codex), installing to both when both are detected, and records them in +`context.json`. + +It runs as init's **first** step, ahead of authentication. Installing skills +needs no network, no credentials and no database, while authenticate, +resolve-database and install-eql each need one and each can exit non-zero — +so the guidance now survives a run that fails partway, which is when it is +needed most. One behaviour change falls out of that: a run cancelled at the +first prompt leaves the skills directory behind where previously it wrote +nothing. + +Also: + +- **New optional `stash init --target `** names the skills + destination and skips detection. Unlike `plan --target` / `impl --target` it + selects the destination only — `init` still performs no handoff. Existing + invocations are unaffected. +- **The summary reports the outcome either way.** A run that installs nothing + now says so, and prints the command that will install them, instead of a + silent `installedSkills: []`. +- **`--target` is validated properly on `init`, `plan` and `impl`.** A + trailing `--target` with no value, and `--target=`, were both treated as + "flag absent" — so the command silently did whatever it does with no flag at + all, rather than telling you the value was missing. All three commands share + one validator now. +- **A later handoff no longer erases the record.** `stash plan --target + agents-md` installs no skill directories of its own and used to overwrite + `installedSkills` with an empty list, dropping skills that were on disk. + Deliveries are merged across hops now. diff --git a/packages/cli/src/cli/registry.ts b/packages/cli/src/cli/registry.ts index 2770c3a8a..c40874ee8 100644 --- a/packages/cli/src/cli/registry.ts +++ b/packages/cli/src/cli/registry.ts @@ -111,6 +111,7 @@ export const registry: CommandGroup[] = [ 'init --supabase', 'init --prisma', 'init --region us-east-1', + 'init --target claude-code', ], flags: [ { @@ -131,6 +132,12 @@ export const registry: CommandGroup[] = [ description: 'Region to authenticate against (e.g. us-east-1). Skips the interactive region picker. Required for non-interactive init when not already logged in.', }, + { + name: '--target', + value: '', + description: + 'Which agent to install the bundled skills for: claude-code (.claude/skills) or codex (.codex/skills). Skips agent detection. Unlike `plan --target` and `impl --target`, this selects the skills destination only — init performs no handoff. agents-md, lovable and wizard install no skill directories (those handoffs inline the skills instead), so passing one here installs nothing.', + }, ], }, { diff --git a/packages/cli/src/commands/impl/__tests__/how-to-proceed.test.ts b/packages/cli/src/commands/impl/__tests__/how-to-proceed.test.ts index f763ec727..0b56e0993 100644 --- a/packages/cli/src/commands/impl/__tests__/how-to-proceed.test.ts +++ b/packages/cli/src/commands/impl/__tests__/how-to-proceed.test.ts @@ -6,6 +6,7 @@ import { defaultChoice, HANDOFF_CHOICES, resolveTarget, + resolveTargetFlag, } from '../steps/how-to-proceed.js' function makeAgents(claudeCode: boolean, codex: boolean): AgentEnvironment { @@ -15,6 +16,7 @@ function makeAgents(claudeCode: boolean, codex: boolean): AgentEnvironment { claudeDir: false, claudeMd: false, claudeSkillsDir: false, + codexDir: false, agentsMd: false, }, editor: 'unknown', @@ -100,3 +102,64 @@ describe('howToProceed — resolveTarget', () => { expect(resolveTarget(undefined)).toBeNull() }) }) + +/** + * `--target` is accepted by three commands, and the validation had been + * hand-copied into each — which is how `plan` and `impl` kept this bug after + * `init` was fixed. Testing the shared helper covers all three. + * + * The distinction that matters is "absent" versus "present but unusable". + * `parseArgs` files a trailing `--target` (nothing followed it) under `flags` + * as `true`, and `--target=` under `values` as an empty string. Testing the + * value for truthiness alone reads both as absent, so the command silently + * does whatever it does with no flag at all — for `init`, writing skills to an + * auto-detected directory the user had just declined by naming another. + */ +describe('howToProceed — resolveTargetFlag', () => { + it('passes a valid target through', () => { + expect(resolveTargetFlag({}, { target: 'codex' })).toEqual({ + target: 'codex', + error: null, + }) + }) + + it('treats an absent flag as neither a target nor an error', () => { + expect(resolveTargetFlag({}, {})).toEqual({ target: null, error: null }) + }) + + it('rejects a trailing `--target`, which parseArgs files under flags', () => { + const { target, error } = resolveTargetFlag({ target: true }, {}) + expect(target).toBeNull() + expect(error).toContain('needs a value') + }) + + it('rejects an empty `--target=`', () => { + const { target, error } = resolveTargetFlag({}, { target: '' }) + expect(target).toBeNull() + expect(error).toContain('needs a value') + }) + + it('reports an unknown value differently from a missing one', () => { + const { target, error } = resolveTargetFlag({}, { target: 'emacs' }) + expect(target).toBeNull() + expect(error).toContain('Unknown --target `emacs`') + expect(error).not.toContain('needs a value') + }) + + it.each([ + ['a trailing flag', { target: true }, {}], + ['an empty value', {}, { target: '' }], + ['an unknown value', {}, { target: 'emacs' }], + ])('lists the valid values when rejecting %s', (_label, flags, values) => { + const { error } = resolveTargetFlag(flags, values) + for (const choice of HANDOFF_CHOICES) expect(error).toContain(choice) + }) + + // An unrelated boolean flag must not be mistaken for the target flag. + it('ignores other flags', () => { + expect(resolveTargetFlag({ yes: true }, {})).toEqual({ + target: null, + error: null, + }) + }) +}) diff --git a/packages/cli/src/commands/impl/index.ts b/packages/cli/src/commands/impl/index.ts index 44fa65f28..33f8fe236 100644 --- a/packages/cli/src/commands/impl/index.ts +++ b/packages/cli/src/commands/impl/index.ts @@ -23,7 +23,7 @@ import { detectPackageManager, runnerCommand } from '../init/utils.js' import { HANDOFF_CHOICES, howToProceedStep, - resolveTarget, + resolveTargetFlag, } from './steps/how-to-proceed.js' function buildStateFromContext( @@ -35,6 +35,15 @@ function buildStateFromContext( clientFilePath: ctx.encryptionClientPath, schemas: ctx.schemas, envKeys: ctx.envKeys, + // Carry the skills already on disk so the handoff's `writeArtifacts` + // merges into them instead of overwriting the record with just its own + // delivery — an `agents-md` handoff installs no directories, and used to + // reset `installedSkills` to `[]` on a project that had them (#923). + skills: { + installed: ctx.installedSkills ?? [], + inlined: ctx.inlinedSkills ?? [], + failed: [], + }, stackInstalled: true, cliInstalled: true, eqlInstalled: true, @@ -152,12 +161,9 @@ export async function implCommand( // Validate `--target` before printing the intro so the error sits at // the top of the output instead of after a half-rendered prompt frame. - const targetFlag = values.target - const target = resolveTarget(targetFlag) - if (targetFlag && !target) { - p.log.error( - `Unknown --target \`${targetFlag}\`. Valid values: ${HANDOFF_CHOICES.join(', ')}.`, - ) + const { target, error: targetError } = resolveTargetFlag(flags, values) + if (targetError) { + p.log.error(targetError) process.exit(1) } diff --git a/packages/cli/src/commands/impl/steps/how-to-proceed.ts b/packages/cli/src/commands/impl/steps/how-to-proceed.ts index 39cd9ebc5..eb2b41459 100644 --- a/packages/cli/src/commands/impl/steps/how-to-proceed.ts +++ b/packages/cli/src/commands/impl/steps/how-to-proceed.ts @@ -40,6 +40,44 @@ export function resolveTarget( : null } +/** + * Resolve a `--target` from raw parsed argv, distinguishing "absent" from + * "present but unusable". + * + * The distinction is the whole point, and it needs both halves of `parseArgs` + * to see. A trailing `--target` (nothing followed it) lands in `flags` as + * `true`; `--target=` lands in `values` as an empty string. Each command used + * to test `values.target` for truthiness alone, so both forms read as "flag + * absent" and fell through to whatever the no-flag path does — for `init`, + * writing skills to an auto-detected directory the user had just declined to + * accept by naming a different one. + * + * Returns the validated target, or an `error` message the caller prints + * before exiting. Exit MECHANICS stay with the caller: `init` unwinds through + * `CliExit` so telemetry flushes, while `plan` and `impl` call `process.exit` + * directly. + * + * Lives here beside {@link HANDOFF_CHOICES} and {@link resolveTarget} because + * three commands accept this flag and the validation had already been + * hand-copied into each — which is exactly how two of them kept the bug after + * the third was fixed. + */ +export function resolveTargetFlag( + flags: Record, + values: Record, +): { target: HandoffChoice | null; error: string | null } { + const provided = flags.target === true || Object.hasOwn(values, 'target') + const raw = values.target + const target = resolveTarget(raw) + if (!provided || target) return { target, error: null } + return { + target: null, + error: raw + ? `Unknown --target \`${raw}\`. Valid values: ${HANDOFF_CHOICES.join(', ')}.` + : `\`--target\` needs a value. Valid values: ${HANDOFF_CHOICES.join(', ')}.`, + } +} + /** * Pick the default option in the menu. * 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 5a7ce68d6..8995a6700 100644 --- a/packages/cli/src/commands/init/__tests__/init-command.test.ts +++ b/packages/cli/src/commands/init/__tests__/init-command.test.ts @@ -17,11 +17,24 @@ const authRun = vi.hoisted(() => vi.fn(async (state: InitState, _provider: InitProvider) => state), ) const passthrough = { run: async (s: InitState) => s } +// Controllable so the skills-summary tests can vary what the first step +// delivered. Mocked like every other step — the REAL one copies files into +// `process.cwd()`, which in this suite is the package root, so leaving it +// unmocked writes `.claude/skills/` into the repo on every test run. +const skillsRun = vi.hoisted(() => + vi.fn(async (s: InitState) => ({ + ...s, + skills: { installed: ['stash-cli'], inlined: [], failed: [] }, + })), +) // 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/install-skills.js', () => ({ + installSkillsStep: { id: 'install-skills', name: 'Skills', run: skillsRun }, +})) vi.mock('../steps/authenticate.js', () => ({ authenticateStep: { id: 'authenticate', name: 'Authenticate', run: authRun }, })) @@ -478,3 +491,79 @@ describe('initCommand — CI detection on the `stash plan` chain offer', () => { ) }) }) + +describe('initCommand — skills summary and --target', () => { + const summaryBody = () => + vi + .mocked(p.note) + .mock.calls.find(([, title]) => title === 'Setup complete')?.[0] as + | string + | undefined + + it('reports how many skills were installed', async () => { + await initCommand({}, {}) + expect(summaryBody()).toContain('✓ 1 agent skill installed') + }) + + /** + * The visible half of #923. Init printed an unqualified "Setup complete" + * while delivering no guidance at all, and `context.json` recorded an + * `installedSkills: []` that looked like a normal empty field. Three of + * four skilltester runs against 1.1.0 ended exactly here, with each agent + * left to find the bundled skills in `node_modules` on its own. + */ + it('says so loudly, with a remedy, when nothing was installed', async () => { + skillsRun.mockImplementationOnce(async (s: InitState) => ({ + ...s, + skills: { installed: [], inlined: [], failed: [] }, + })) + + await initCommand({}, {}) + + const body = summaryBody() + expect(body).toContain('No agent skills installed') + expect(body).toContain('plan --target claude-code') + }) + + it('threads a valid --target onto state for the skills step', async () => { + await initCommand({}, { target: 'codex' }) + expect(skillsRun.mock.calls[0]?.[0].targetFlag).toBe('codex') + }) + + it('rejects an unknown --target before doing any work', async () => { + await expect(initCommand({}, { target: 'emacs' })).rejects.toBeInstanceOf( + CliExit, + ) + expect(skillsRun).not.toHaveBeenCalled() + }) + + /** + * `parseArgs` files a trailing `--target` (nothing followed it) under + * `flags` and `--target=` under `values` as an empty string. A bare + * truthiness test on the value treats both as "flag absent", so init would + * fall through to auto-detection and could write skills to a directory the + * user never chose — silently, having been asked for something specific. + */ + it('rejects a valueless `--target`', async () => { + await expect(initCommand({ target: true }, {})).rejects.toBeInstanceOf( + CliExit, + ) + expect(skillsRun).not.toHaveBeenCalled() + }) + + it('rejects an empty `--target=`', async () => { + await expect(initCommand({}, { target: '' })).rejects.toBeInstanceOf( + CliExit, + ) + expect(skillsRun).not.toHaveBeenCalled() + }) + + it('names the problem when the value is missing rather than unknown', async () => { + await expect(initCommand({ target: true }, {})).rejects.toBeInstanceOf( + CliExit, + ) + const message = vi.mocked(p.log.error).mock.calls.map(String).join('\n') + expect(message).toContain('needs a value') + expect(message).not.toContain('Unknown') + }) +}) diff --git a/packages/cli/src/commands/init/__tests__/steps-wiring.test.ts b/packages/cli/src/commands/init/__tests__/steps-wiring.test.ts new file mode 100644 index 000000000..5487252d4 --- /dev/null +++ b/packages/cli/src/commands/init/__tests__/steps-wiring.test.ts @@ -0,0 +1,65 @@ +import { readFileSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' + +/** + * Guards the `STEPS` pipeline in `init/index.ts` against the failure mode + * that produced #923. + * + * `installSkills()` was never broken. What broke was its REACHABILITY: the + * init/plan/impl restructure left the handoff steps as its only callers, and + * `stash init` reaches no handoff — so `stash@1.1.0` shipped a CLI that + * installed zero skills for anyone, in any mode, for an entire release. + * Nothing caught it because every unit test of the module still passed. A + * step that nothing invokes reads exactly like a step that works. + * + * Scanning the source rather than importing it is deliberate, and matches + * `lintWiring.test.ts` / `integrationSuiteCi.test.ts` elsewhere in the repo: + * `init/index.ts` transitively imports the plan command and the whole + * provider graph, so an import-based assertion would trade a precise check + * for a fragile one. + */ +const INIT_INDEX = join( + dirname(fileURLToPath(import.meta.url)), + '..', + 'index.ts', +) + +function stepsArray(): string { + const source = readFileSync(INIT_INDEX, 'utf-8') + const match = source.match(/const STEPS = \[([\s\S]*?)\n\]/) + if (!match) throw new Error('Could not find the STEPS array in init/index.ts') + return match[1] +} + +/** Step identifiers in pipeline order, comments stripped. */ +function stepOrder(): string[] { + return stepsArray() + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.endsWith('Step,')) + .map((line) => line.slice(0, -1)) +} + +describe('init STEPS wiring', () => { + it('runs installSkillsStep', () => { + expect(stepOrder()).toContain('installSkillsStep') + }) + + /** + * Ordering is load-bearing, not cosmetic. Installing skills needs no + * network, no credentials and no database; authenticate, resolve-database + * and install-eql each need one and each can exit non-zero. Running first + * is what makes the guidance survive those failures — and `stash-cli`, + * which covers recovering from them, is in every skill set. + */ + it('runs it first, ahead of every fallible step', () => { + expect(stepOrder()[0]).toBe('installSkillsStep') + }) + + it('still imports the step it names', () => { + const source = readFileSync(INIT_INDEX, 'utf-8') + expect(source).toContain("from './steps/install-skills.js'") + }) +}) diff --git a/packages/cli/src/commands/init/detect-agents.ts b/packages/cli/src/commands/init/detect-agents.ts index a33f316f0..bff2f78a6 100644 --- a/packages/cli/src/commands/init/detect-agents.ts +++ b/packages/cli/src/commands/init/detect-agents.ts @@ -18,6 +18,8 @@ export interface AgentEnvironment { claudeMd: boolean /** A `.claude/skills/` directory exists at the project root. */ claudeSkillsDir: boolean + /** A `.codex/` directory exists at the project root. */ + codexDir: boolean /** An `AGENTS.md` file exists at the project root. */ agentsMd: boolean } @@ -86,6 +88,7 @@ export function detectAgents( claudeDir: isDirectory(resolve(cwd, '.claude')), claudeMd: existsSync(resolve(cwd, 'CLAUDE.md')), claudeSkillsDir: isDirectory(resolve(cwd, '.claude', 'skills')), + codexDir: isDirectory(resolve(cwd, '.codex')), agentsMd: existsSync(resolve(cwd, 'AGENTS.md')), }, editor: detectEditor(env), diff --git a/packages/cli/src/commands/init/index.ts b/packages/cli/src/commands/init/index.ts index ea45285f1..137850148 100644 --- a/packages/cli/src/commands/init/index.ts +++ b/packages/cli/src/commands/init/index.ts @@ -2,7 +2,10 @@ import * as p from '@clack/prompts' import { CliExit } from '../../cli/exit.js' import { isInteractive } from '../../config/tty.js' import { messages } from '../../messages.js' -import { HANDOFF_CHOICES } from '../impl/steps/how-to-proceed.js' +import { + HANDOFF_CHOICES, + resolveTargetFlag, +} from '../impl/steps/how-to-proceed.js' import { planCommand } from '../plan/index.js' import { createBaseProvider } from './providers/base.js' import { createDrizzleProvider } from './providers/drizzle.js' @@ -13,6 +16,7 @@ import { buildSchemaStep } from './steps/build-schema.js' import { gatherContextStep } from './steps/gather-context.js' import { installDepsStep } from './steps/install-deps.js' import { installEqlStep } from './steps/install-eql.js' +import { installSkillsStep } from './steps/install-skills.js' import { resolveDatabaseStep } from './steps/resolve-database.js' import type { InitProvider, InitState, ProviderKey } from './types.js' import { CancelledError } from './types.js' @@ -47,6 +51,16 @@ const PROVIDER_KEYS = Object.keys(PROVIDER_MAP) as ProviderKey[] * to the longer agent-driven phase. */ const STEPS = [ + // `install-skills` runs FIRST, and deliberately. It is the only step that + // needs neither network, credentials, nor a database, and the guidance it + // writes is what an agent needs most when a LATER step fails — auth, the + // database URL and EQL all exit non-zero, and `stash-cli` is the skill that + // covers recovering from each. Ordering it last is how #923 happened: the + // only callers of `installSkills` were the handoff steps, which `stash init` + // never reaches, so init shipped zero skills for an entire release. + // Guarded by `__tests__/steps-wiring.test.ts` — a unit test of the step + // alone would not have caught a pipeline that stopped calling it. + installSkillsStep, authenticateStep, resolveDatabaseStep, buildSchemaStep, @@ -123,10 +137,22 @@ export async function initCommand( const provider = resolveProvider(flags) + // `--target` on `init` selects the SKILLS DESTINATION and nothing else — it + // does not perform a handoff the way `plan --target` / `impl --target` do. + // Shares `resolveTargetFlag` with those two so the three never drift on what + // a target name means, or on which malformed forms are rejected. + // Absent means "auto-detect". + const { target, error: targetError } = resolveTargetFlag(flags, values) + if (targetError) { + p.log.error(targetError) + throw new CliExit(1) + } + p.intro('CipherStash Stack Setup') p.log.info(provider.introMessage) let state: InitState = {} + if (target) state.targetFlag = target // Thread `--region ` through to the authenticate step so init can run // non-interactively (STASH_REGION works even without this, via the env @@ -206,6 +232,29 @@ export async function initCommand( checkmarks.push(`○ EQL migration ${verb} — ${applyStep}`) } + // Report the skills outcome in the summary, both ways. A silent + // `installedSkills: []` is what let #923 hide for a release: init printed + // success, the context file looked plausible, and the agent driving the + // setup was never told the guidance it needed was sitting unread in + // `node_modules`. Absent skills are degraded guidance, not a broken + // setup, so this never changes the exit code — unlike `eqlPending` below. + // + // Pushed BEFORE the EQL check so it appears on the failing summary too. + // That run is the one where it matters most: the agent is about to be + // told setup is incomplete, and `stash-cli` is the skill that covers + // `stash eql install`. + const installedSkills = state.skills?.installed ?? [] + if (installedSkills.length > 0) { + checkmarks.push( + `✓ ${installedSkills.length} agent skill${installedSkills.length !== 1 ? 's' : ''} installed`, + ) + } else { + const suggestion = state.targetFlag ?? 'claude-code' + checkmarks.push( + `○ No agent skills installed — no coding agent detected. Run \`${cli} plan --target ${suggestion}\` to install them.`, + ) + } + // EQL is required for encryption. Some integrations install it out-of-band // and legitimately leave `eqlInstalled` false here: Prisma Next installs it // via `prisma-next migrate`, and the Drizzle and Supabase flows generate a @@ -247,7 +296,10 @@ export async function initCommand( }) if (!p.isCancel(proceed) && proceed) { p.outro('Setup complete — handing off to `stash plan`.') - await planCommand() + // Forward an explicit `--target`: the user already named their agent + // once, so re-asking with the picker would be asking the same + // question twice. Without the flag, `plan` prompts as before. + await planCommand({}, target ? { target } : {}) return } p.outro(`Next: run \`${cli} plan\` to draft your encryption plan.`) @@ -257,7 +309,9 @@ export async function initCommand( // /dev/tty. Steer them at `--target` up front so the next command // doesn't surprise them. p.outro( - `Next: run \`${cli} plan --target <${HANDOFF_CHOICES.join('|')}>\` to draft your encryption plan. The \`--target\` flag is required when running non-interactively (skips the agent-target picker).`, + target + ? `Next: run \`${cli} plan --target ${target}\` to draft your encryption plan.` + : `Next: run \`${cli} plan --target <${HANDOFF_CHOICES.join('|')}>\` to draft your encryption plan. The \`--target\` flag is required when running non-interactively (skips the agent-target picker).`, ) } } catch (err) { diff --git a/packages/cli/src/commands/init/lib/__tests__/handoff-helpers.test.ts b/packages/cli/src/commands/init/lib/__tests__/handoff-helpers.test.ts new file mode 100644 index 000000000..504910c11 --- /dev/null +++ b/packages/cli/src/commands/init/lib/__tests__/handoff-helpers.test.ts @@ -0,0 +1,143 @@ +import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mergeSkillsDelivery } from '../../types.js' + +vi.mock('@clack/prompts', () => ({ + log: { warn: vi.fn(), success: vi.fn(), info: vi.fn() }, +})) + +import { writeArtifacts } from '../handoff-helpers.js' +import { + CONTEXT_REL_PATH, + type ContextFile, + SETUP_PROMPT_REL_PATH, +} from '../write-context.js' + +let cwd: string + +beforeEach(() => { + cwd = mkdtempSync(join(tmpdir(), 'stash-handoff-')) +}) + +afterEach(() => { + vi.restoreAllMocks() + rmSync(cwd, { recursive: true, force: true }) +}) + +function readContext(): ContextFile { + return JSON.parse(readFileSync(join(cwd, CONTEXT_REL_PATH), 'utf-8')) +} + +function readPrompt(): string { + return readFileSync(join(cwd, SETUP_PROMPT_REL_PATH), 'utf-8') +} + +describe('mergeSkillsDelivery', () => { + it('unions both sides and de-duplicates', () => { + expect( + mergeSkillsDelivery( + { installed: ['a', 'b'], inlined: [], failed: [] }, + { installed: ['b', 'c'], inlined: ['d'], failed: [] }, + ), + ).toEqual({ installed: ['a', 'b', 'c'], inlined: ['d'], failed: [] }) + }) + + // A skill that failed one hop and landed in another is delivered. Leaving it + // in `failed` would have the record report it as both. + it('drops a failure that a later hop delivered', () => { + expect( + mergeSkillsDelivery( + { installed: [], inlined: [], failed: ['stash-cli'] }, + { installed: [], inlined: ['stash-cli'], failed: [] }, + ), + ).toEqual({ installed: [], inlined: ['stash-cli'], failed: [] }) + }) + + it('treats an absent left side as empty', () => { + expect( + mergeSkillsDelivery(undefined, { + installed: ['a'], + inlined: [], + failed: [], + }), + ).toEqual({ installed: ['a'], inlined: [], failed: [] }) + }) +}) + +describe('writeArtifacts', () => { + /** + * The #923 regression, one command later. `stash init` installs skills into + * `.claude/skills/` up front; a subsequent `stash plan --target agents-md` + * installs no directories of its own and used to overwrite + * `installedSkills` with its own empty list — erasing from the record + * skills that are sitting on disk. + */ + it('keeps skills a previous hop installed', () => { + writeArtifacts( + cwd, + { + integration: 'supabase', + skills: { + installed: ['stash-encryption', 'stash-cli'], + inlined: [], + failed: [], + }, + }, + 'agents-md', + { installed: [], inlined: ['stash-supabase'], failed: [] }, + ) + + const ctx = readContext() + expect(ctx.installedSkills).toEqual(['stash-encryption', 'stash-cli']) + expect(ctx.inlinedSkills).toEqual(['stash-supabase']) + }) + + /** + * The context file and the setup prompt answer different questions, so they + * take different views of the same delivery. + * + * Here `stash init` installed skills into `.claude/skills/`, and a later + * Codex handoff failed to write `.codex/skills/` and could not fall back to + * inlining either. The project genuinely has those skills — `context.json` + * should keep saying so. But the prompt is launching Codex, and + * `rulesLocation` derives its directory from the handoff choice: rendering + * it from the merged view let the earlier `.claude/skills/` install satisfy + * the "installed" test and point Codex at a directory that was never + * written. + */ + it('does not let an earlier install vouch for this handoff’s directory', () => { + writeArtifacts( + cwd, + { + integration: 'supabase', + skills: { + installed: ['stash-encryption', 'stash-cli'], + inlined: [], + failed: [], + }, + }, + 'codex', + { installed: [], inlined: [], failed: ['stash-encryption', 'stash-cli'] }, + ) + + // The project still has them — from the earlier hop. + expect(readContext().installedSkills).toEqual([ + 'stash-encryption', + 'stash-cli', + ]) + // But the prompt must not send Codex to `.codex/skills/`. + expect(readPrompt()).not.toContain('.codex/skills/') + }) + + it('records this hop when there is nothing to merge with', () => { + writeArtifacts(cwd, { integration: 'drizzle' }, 'claude-code', { + installed: ['stash-drizzle'], + inlined: [], + failed: [], + }) + + expect(readContext().installedSkills).toEqual(['stash-drizzle']) + }) +}) diff --git a/packages/cli/src/commands/init/lib/__tests__/write-context.test.ts b/packages/cli/src/commands/init/lib/__tests__/write-context.test.ts new file mode 100644 index 000000000..f134a0ef9 --- /dev/null +++ b/packages/cli/src/commands/init/lib/__tests__/write-context.test.ts @@ -0,0 +1,56 @@ +import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + CONTEXT_REL_PATH, + type ContextFile, + writeBaselineContextFile, +} from '../write-context.js' + +let cwd: string + +beforeEach(() => { + cwd = mkdtempSync(join(tmpdir(), 'stash-write-context-')) +}) + +afterEach(() => { + rmSync(cwd, { recursive: true, force: true }) +}) + +function readContext(): ContextFile { + return JSON.parse(readFileSync(join(cwd, CONTEXT_REL_PATH), 'utf-8')) +} + +describe('writeBaselineContextFile', () => { + /** + * `installedSkills` used to be hardcoded `[]` in `buildContextFile`, set + * only by the handoff steps — so every `context.json` written by `stash + * init` reported no skills regardless of what was on disk. That empty + * array is the artifact #923 was diagnosed from, and the reason the bug + * survived a release: the file looked plausible. + */ + it('reports the skills the init step installed', () => { + writeBaselineContextFile( + { + integration: 'supabase', + skills: { + installed: ['stash-supabase', 'stash-cli'], + inlined: [], + failed: [], + }, + }, + cwd, + ['DATABASE_URL'], + ) + + const ctx = readContext() + expect(ctx.installedSkills).toEqual(['stash-supabase', 'stash-cli']) + expect(ctx.envKeys).toEqual(['DATABASE_URL']) + }) + + it('reports an empty list when nothing was installed', () => { + writeBaselineContextFile({ integration: 'drizzle' }, cwd, []) + expect(readContext().installedSkills).toEqual([]) + }) +}) diff --git a/packages/cli/src/commands/init/lib/handoff-helpers.ts b/packages/cli/src/commands/init/lib/handoff-helpers.ts index 5f09ddd55..64c3271bc 100644 --- a/packages/cli/src/commands/init/lib/handoff-helpers.ts +++ b/packages/cli/src/commands/init/lib/handoff-helpers.ts @@ -2,19 +2,19 @@ import { spawn } from 'node:child_process' import { existsSync, readFileSync, writeFileSync } from 'node:fs' import { resolve } from 'node:path' import * as p from '@clack/prompts' -import type { HandoffChoice, InitState } from '../types.js' +import type { HandoffChoice, InitState, SkillsDelivery } from '../types.js' +import { mergeSkillsDelivery } from '../types.js' import { upsertManagedBlock } from './sentinel-upsert.js' import { buildContextFile, buildSetupPromptContext, CONTEXT_REL_PATH, SETUP_PROMPT_REL_PATH, - type SkillsDelivery, writeContextFile, writeSetupPrompt, } from './write-context.js' -export type { SkillsDelivery } from './write-context.js' +export type { SkillsDelivery } from '../types.js' export const AGENTS_MD_REL_PATH = 'AGENTS.md' @@ -78,9 +78,29 @@ export function writeAgentsMd(cwd: string, managed: string): boolean { * paths, which all need the same artifacts with handoff-specific values * threaded into the setup prompt. * - * `skills` records where the skills actually ended up (installed as - * directories, inlined into AGENTS.md, or failed) so the generated prompt - * never mislabels an unwritable destination as a stripped build. + * `skills` records where THIS handoff put them (installed as directories, + * inlined into AGENTS.md, or failed) so the generated prompt never mislabels + * an unwritable destination as a stripped build. + * + * The two outputs deliberately take DIFFERENT views of it: + * + * - `context.json` gets the MERGE of `state.skills` and this handoff. + * `stash init` installs skills up front now, and a handoff that installs + * none of its own — `agents-md`, `lovable` — used to overwrite + * `installedSkills` with `[]` and erase from the record skills that are + * sitting on disk (#923, one command later). The field is a flat list + * with no destination attached, so a union across hops is the honest + * reading of "which skills does this project have". + * + * - The setup prompt gets THIS handoff's delivery only. It answers a + * different question — where should the agent I am launching right now + * go to read the rules — and that answer is per-destination. + * `rulesLocation` derives the directory from the handoff choice, so + * feeding it the merged view lets skills installed under `.claude/skills` + * by an earlier `stash init` satisfy the "installed" test for a Codex + * handoff whose own copy into `.codex/skills` failed. The prompt would + * then send Codex to a directory that was never written, and the merge's + * failure-filtering would have hidden the failure that caused it. */ export function writeArtifacts( cwd: string, @@ -88,13 +108,14 @@ export function writeArtifacts( handoff: HandoffChoice, skills: SkillsDelivery, ): void { - const ctx = buildContextFile(state) + const merged = mergeSkillsDelivery(state.skills, skills) + const ctx = buildContextFile({ ...state, skills: merged }) ctx.envKeys = state.envKeys ?? [] - ctx.installedSkills = skills.installed - ctx.inlinedSkills = skills.inlined writeContextFile(resolve(cwd, CONTEXT_REL_PATH), ctx) p.log.success(`Wrote ${CONTEXT_REL_PATH}`) + // `skills`, not `merged` — see the note above. The prompt is about this + // handoff's destination; the context file is about the project. const promptCtx = buildSetupPromptContext(state, handoff, skills) if (promptCtx) { writeSetupPrompt(resolve(cwd, SETUP_PROMPT_REL_PATH), promptCtx) diff --git a/packages/cli/src/commands/init/lib/install-skills.ts b/packages/cli/src/commands/init/lib/install-skills.ts index 223dea04f..ac5d43eee 100644 --- a/packages/cli/src/commands/init/lib/install-skills.ts +++ b/packages/cli/src/commands/init/lib/install-skills.ts @@ -66,8 +66,19 @@ export const SKILL_MAP: Record = { ], } -/** The skills every integration gets — the safe fallback for an unmapped one. */ -const BASE_SKILLS: readonly string[] = [ +/** + * The skills every integration gets — the safe fallback for an unmapped one, + * and the set installed before the integration is known. + * + * `stash init` installs skills as its FIRST step, so the guidance survives a + * run that dies at auth / database / EQL — which is exactly when an agent + * needs `stash-cli` most. At that point the integration is only known when a + * provider flag or a cwd signal (drizzle config, prisma-next config) supplies + * it; a bare `stash init` against a Supabase-hosted URL cannot be classified + * until `resolve-database` has run. Those runs get this set, then a top-up in + * `build-schema` once `detectIntegration` has answered. + */ +export const BASE_SKILLS: readonly string[] = [ 'stash-encryption', 'stash-indexing', 'stash-deployment', @@ -77,7 +88,10 @@ const BASE_SKILLS: readonly string[] = [ ] /** - * Skills for an integration, resilient to an unmapped one. `SKILL_MAP` is + * Skills for an integration, resilient to an unmapped one — and to not + * knowing the integration yet: `undefined` yields {@link BASE_SKILLS}, which + * is what the first-step install uses before `build-schema` has classified + * the project. `SKILL_MAP` is * typed `Record`, but the build (`tsup`) transpiles without * type-checking — so a new `Integration` variant added without a `SKILL_MAP` * entry would ship as `undefined` and crash both consumers (`installSkills`, @@ -86,7 +100,10 @@ const BASE_SKILLS: readonly string[] = [ * stack trace. (Regression-guarded by a test asserting SKILL_MAP has a * non-empty entry for every value in a maintained `ALL_INTEGRATIONS` list.) */ -export function skillsFor(integration: Integration): readonly string[] { +export function skillsFor( + integration: Integration | undefined, +): readonly string[] { + if (integration === undefined) return BASE_SKILLS return SKILL_MAP[integration] ?? BASE_SKILLS } @@ -100,7 +117,9 @@ export function skillsFor(integration: Integration): readonly string[] { * is what keeps "inlining N skills" claims honest (#714 / #687 removed * exactly this kind of false success elsewhere in init). */ -export function availableSkills(integration: Integration): string[] { +export function availableSkills( + integration: Integration | undefined, +): string[] { const bundledRoot = findBundledDir('skills') if (!bundledRoot) return [] return skillsFor(integration).filter((name) => @@ -151,7 +170,7 @@ export interface SkillsInstallResult { export function installSkills( cwd: string, destDir: string, - integration: Integration, + integration: Integration | undefined, ): SkillsInstallResult { const bundledRoot = findBundledDir('skills') if (!bundledRoot) { diff --git a/packages/cli/src/commands/init/lib/write-context.ts b/packages/cli/src/commands/init/lib/write-context.ts index 25514ad2d..6fbfa9b2f 100644 --- a/packages/cli/src/commands/init/lib/write-context.ts +++ b/packages/cli/src/commands/init/lib/write-context.ts @@ -7,6 +7,7 @@ import type { InitState, Integration, SchemaDef, + SkillsDelivery, } from '../types.js' import { detectPackageManager, @@ -19,25 +20,11 @@ import { renderSetupPrompt, type SetupPromptContext } from './setup-prompt.js' export const CONTEXT_REL_PATH = '.cipherstash/context.json' export const SETUP_PROMPT_REL_PATH = '.cipherstash/setup-prompt.md' -/** - * How the per-integration skills reached (or failed to reach) the project. - * Threaded into `context.json` and the setup prompt so both describe what - * actually happened, not what the handoff hoped for: - * - * installed — copied into a skills directory (`.claude/skills`, - * `.codex/skills`) - * inlined — bodies written into AGENTS.md under "## Skill references" - * (the editor-agent handoff, and the Codex fallback for an - * unwritable `.codex/` — #736) - * failed — bundled skills that ended up nowhere (destination - * unwritable with no inline fallback, or AGENTS.md itself - * unwritable) - */ -export interface SkillsDelivery { - installed: string[] - inlined: string[] - failed: string[] -} +// `SkillsDelivery` moved to `../types.js` so `InitState` can carry one +// without types.ts importing this module back. Re-exported here because +// every existing consumer (setup-prompt, handoff-helpers) imports it from +// this path. +export type { SkillsDelivery } from '../types.js' export interface ContextFile { cliVersion: string @@ -116,6 +103,12 @@ function ensureDir(path: string): void { * which columns to encrypt, so there are no inferred schemas at init time. * The agent (or `stash encrypt` commands later) populates this when real * encrypted tables exist. + * + * The skill lists come from `state.skills`, which accumulates across every + * hop that delivers skills (the init step, then any later handoff). They + * used to be hardcoded empty here and set only by `writeArtifacts`, so a + * `context.json` written by `stash init` always reported no skills — the + * visible half of #923. */ export function buildContextFile(state: InitState): ContextFile { const integration = state.integration ?? 'postgresql' @@ -129,8 +122,8 @@ export function buildContextFile(state: InitState): ContextFile { installCommand: prodInstallCommand(pm, pinnedSpec('@cipherstash/stack')), envKeys: [], schemas: state.schemas ?? [], - installedSkills: [], - inlinedSkills: [], + installedSkills: state.skills?.installed ?? [], + inlinedSkills: state.skills?.inlined ?? [], planStep: state.planStep, generatedAt: new Date().toISOString(), } diff --git a/packages/cli/src/commands/init/steps/__tests__/install-skills.test.ts b/packages/cli/src/commands/init/steps/__tests__/install-skills.test.ts new file mode 100644 index 000000000..751f84e05 --- /dev/null +++ b/packages/cli/src/commands/init/steps/__tests__/install-skills.test.ts @@ -0,0 +1,247 @@ +import { existsSync, mkdirSync, mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { AgentEnvironment } from '../../detect-agents.js' +import type { InitProvider, InitState } from '../../types.js' + +vi.mock('@clack/prompts', () => ({ + log: { warn: vi.fn(), success: vi.fn(), info: vi.fn() }, +})) + +import { + CLAUDE_SKILLS_DIR, + CODEX_SKILLS_DIR, + guessIntegration, + installSkillsStep, + skillDestinations, + topUpSkills, +} from '../install-skills.js' + +/** An environment with nothing detected; tests switch on what they need. */ +function env(overrides: Partial = {}): AgentEnvironment { + return { + cli: { claudeCode: false, codex: false }, + project: { + claudeDir: false, + claudeMd: false, + claudeSkillsDir: false, + codexDir: false, + agentsMd: false, + }, + editor: 'unknown', + ...overrides, + } +} + +function provider(selected: InitProvider['selected'] = []): InitProvider { + return { + name: 'test', + selected, + introMessage: '', + getNextSteps: () => [], + } +} + +let cwd: string + +beforeEach(() => { + cwd = mkdtempSync(join(tmpdir(), 'stash-init-skills-')) + vi.spyOn(process, 'cwd').mockReturnValue(cwd) + // `installSkillsStep` calls the real `detectAgents`, which walks the real + // PATH — so on a developer machine with `claude` installed every + // "nothing detected" case would install anyway, and pass or fail by + // accident of who ran it. Blank the PATH and drive detection from the + // project-level signals the temp cwd controls. + vi.stubEnv('PATH', '') +}) + +afterEach(() => { + vi.restoreAllMocks() + vi.unstubAllEnvs() + rmSync(cwd, { recursive: true, force: true }) +}) + +describe('skillDestinations', () => { + it('installs nowhere when no agent is detected', () => { + expect(skillDestinations(env(), undefined)).toEqual([]) + }) + + it('follows the Claude CLI', () => { + expect( + skillDestinations( + env({ cli: { claudeCode: true, codex: false } }), + undefined, + ), + ).toEqual([CLAUDE_SKILLS_DIR]) + }) + + // The #923 flow: an agent running `npx stash init` inside a repo that + // already has `.claude/`. The binary need not be on the PATH this process + // inherited for the project to be a Claude Code project. + it('follows a project-level .claude/ with no CLI on PATH', () => { + const agents = env() + agents.project.claudeDir = true + expect(skillDestinations(agents, undefined)).toEqual([CLAUDE_SKILLS_DIR]) + }) + + it('follows a project-level .codex/', () => { + const agents = env() + agents.project.codexDir = true + expect(skillDestinations(agents, undefined)).toEqual([CODEX_SKILLS_DIR]) + }) + + it('installs both when both are detected', () => { + expect( + skillDestinations( + env({ cli: { claudeCode: true, codex: true } }), + undefined, + ), + ).toEqual([CLAUDE_SKILLS_DIR, CODEX_SKILLS_DIR]) + }) + + // An explicit --target is the user naming their agent. Detection must not + // add a second directory they did not ask for. + it('honours --target over detection, without adding the detected one', () => { + expect( + skillDestinations( + env({ cli: { claudeCode: true, codex: false } }), + 'codex', + ), + ).toEqual([CODEX_SKILLS_DIR]) + }) + + // These handoffs inline the skill bodies into AGENTS.md instead of copying + // directories, and `init` performs no handoff — so there is nothing to write. + it.each([ + 'agents-md', + 'lovable', + 'wizard', + ] as const)('installs no directories for --target %s', (target) => { + expect( + skillDestinations( + env({ cli: { claudeCode: true, codex: false } }), + target, + ), + ).toEqual([]) + }) +}) + +describe('guessIntegration', () => { + it('is undefined when nothing is conclusive', () => { + expect(guessIntegration(cwd, provider())).toBeUndefined() + }) + + it('reads the --supabase flag', () => { + expect(guessIntegration(cwd, provider(['supabase']))).toBe('supabase') + }) + + // `build-schema`'s detectIntegration consults only `--prisma`, so a + // `--drizzle` run in a project with no Drizzle config classifies as + // postgresql there. For the encryption client that is right; for skills it + // is not — the flag is the user naming the integration to teach the agent, + // and ignoring it hands them the base set instead of the Drizzle one. + it('reads the --drizzle flag even with no drizzle config on disk', () => { + expect(guessIntegration(cwd, provider(['drizzle']))).toBe('drizzle') + }) + + // Same precedence init uses when routing the EQL migration: Drizzle owns + // the migration history, `--supabase` is the grants modifier. + it('prefers drizzle over supabase on a combined run', () => { + expect(guessIntegration(cwd, provider(['supabase', 'drizzle']))).toBe( + 'drizzle', + ) + }) + + // Same precedence as build-schema's detectIntegration: Prisma Next owns the + // migration framework even when a Supabase signal also fires. + it('prefers prisma-next over supabase on a combined run', () => { + expect(guessIntegration(cwd, provider(['supabase', 'prisma']))).toBe( + 'prisma-next', + ) + }) +}) + +describe('installSkillsStep', () => { + it('copies skills into .claude/skills and records them on state', async () => { + mkdirSync(join(cwd, '.claude'), { recursive: true }) + + const next = await installSkillsStep.run({}, provider(['supabase'])) + + expect(next.skills?.installed).toContain('stash-supabase') + expect(next.skills?.installed).toContain('stash-cli') + expect(next.skills?.failed).toEqual([]) + expect( + existsSync(join(cwd, CLAUDE_SKILLS_DIR, 'stash-supabase', 'SKILL.md')), + ).toBe(true) + }) + + // The base set is what an unclassifiable run gets — a bare `stash init` + // whose Supabase-ness lives in a DATABASE_URL that has not been resolved + // yet. It must still carry `stash-cli`, the skill covering recovery from + // the auth / database / EQL failures this step now runs ahead of. + it('falls back to the base skill set when the integration is unknown', async () => { + mkdirSync(join(cwd, '.claude'), { recursive: true }) + + const next = await installSkillsStep.run({}, provider()) + + expect(next.skills?.installed).toContain('stash-cli') + expect(next.skills?.installed).not.toContain('stash-supabase') + }) + + it('installs the flagged integration set, not just the base set', async () => { + mkdirSync(join(cwd, '.claude'), { recursive: true }) + + const next = await installSkillsStep.run({}, provider(['drizzle'])) + + expect(next.skills?.installed).toContain('stash-drizzle') + }) + + it('installs nothing and stays non-fatal when no agent is detected', async () => { + const next = await installSkillsStep.run({}, provider(['supabase'])) + + expect(next.skills?.installed).toEqual([]) + expect(existsSync(join(cwd, '.claude'))).toBe(false) + expect(existsSync(join(cwd, '.codex'))).toBe(false) + }) + + it('stores the detected environment so later steps do not re-walk PATH', async () => { + const next = await installSkillsStep.run({}, provider()) + expect(next.agents).toBeDefined() + }) +}) + +describe('topUpSkills', () => { + // The correction path for a bare `stash init` against a Supabase-hosted + // URL: the first step could not classify it, `build-schema` can. + it('adds the integration skills the base set was missing', async () => { + mkdirSync(join(cwd, '.claude'), { recursive: true }) + const state = (await installSkillsStep.run({}, provider())) as InitState + + expect(state.skills?.installed).not.toContain('stash-supabase') + + const topped = topUpSkills(cwd, state, 'supabase') + + expect(topped?.installed).toContain('stash-supabase') + expect(topped?.installed).toContain('stash-cli') + // Merged, not replaced — every name appears once. + expect(new Set(topped?.installed).size).toBe(topped?.installed.length) + }) + + it('is a no-op when the first-step guess already matched', async () => { + mkdirSync(join(cwd, '.claude'), { recursive: true }) + const state = (await installSkillsStep.run( + {}, + provider(['supabase']), + )) as InitState + + expect(topUpSkills(cwd, state, 'supabase')?.installed).toEqual( + state.skills?.installed, + ) + }) + + it('installs nothing when no agent was detected', async () => { + const state = (await installSkillsStep.run({}, provider())) as InitState + expect(topUpSkills(cwd, state, 'supabase')?.installed).toEqual([]) + }) +}) diff --git a/packages/cli/src/commands/init/steps/build-schema.ts b/packages/cli/src/commands/init/steps/build-schema.ts index b43dbb926..a0d037418 100644 --- a/packages/cli/src/commands/init/steps/build-schema.ts +++ b/packages/cli/src/commands/init/steps/build-schema.ts @@ -18,6 +18,7 @@ import type { } from '../types.js' import { CancelledError } from '../types.js' import { generatePlaceholderClient } from '../utils.js' +import { topUpSkills } from './install-skills.js' /** * Pick the integration template by reading the same signals `eql install` @@ -89,6 +90,7 @@ export const buildSchemaStep: InitStep = { schemas: [], schemaFromIntrospection: false, envKeys, + skills: topUpSkills(cwd, state, integration), } writeBaselineContextFile(nextState, cwd, envKeys) return nextState @@ -153,12 +155,19 @@ export const buildSchemaStep: InitStep = { // state for now to avoid a wider type change; always false. schemaFromIntrospection: false, envKeys, + // The install-skills step ran before `DATABASE_URL` was resolved, so a + // bare `stash init` against a Supabase-hosted database got the base + // skill set. Now that `integration` is known, top it up — idempotent, + // and a no-op when the first-step guess already matched. + skills: topUpSkills(cwd, state, integration), } // Write a baseline `.cipherstash/context.json` immediately so it tracks - // the placeholder we just wrote. Handoff steps refresh it later with - // the list of installed skills; this baseline guarantees the file - // exists even if init aborts before the handoff fires. + // the placeholder we just wrote — including the skills installed by the + // first step, so the file never claims `installedSkills: []` over a + // project that has them (#923). Handoff steps merge their own delivery + // in later; this baseline guarantees the file exists, and is honest, + // even if init aborts before any handoff fires. writeBaselineContextFile(nextState, cwd, envKeys) return nextState diff --git a/packages/cli/src/commands/init/steps/gather-context.ts b/packages/cli/src/commands/init/steps/gather-context.ts index f190d25d1..e3101d8ea 100644 --- a/packages/cli/src/commands/init/steps/gather-context.ts +++ b/packages/cli/src/commands/init/steps/gather-context.ts @@ -15,7 +15,10 @@ export const gatherContextStep: InitStep = { name: 'Gather setup context', async run(state: InitState, _provider: InitProvider): Promise { const cwd = process.cwd() - const agents = detectAgents(cwd, process.env) + // `install-skills` (the first step) already walked PATH and stat'd the + // project. Reuse its answer; only re-detect if this step is called + // outside the init pipeline. + const agents = state.agents ?? detectAgents(cwd, process.env) const pm = detectPackageManager() const envKeyCount = state.envKeys?.length ?? 0 @@ -25,6 +28,8 @@ export const gatherContextStep: InitStep = { detectedBits.push(`package manager: ${pm}`) if (agents.cli.claudeCode) detectedBits.push('Claude Code CLI: yes') if (agents.cli.codex) detectedBits.push('Codex CLI: yes') + const skillCount = state.skills?.installed.length ?? 0 + if (skillCount > 0) detectedBits.push(`agent skills: ${skillCount}`) if (envKeyCount > 0) { detectedBits.push(`env keys: ${envKeyCount} found`) } diff --git a/packages/cli/src/commands/init/steps/install-skills.ts b/packages/cli/src/commands/init/steps/install-skills.ts new file mode 100644 index 000000000..f9b5f113d --- /dev/null +++ b/packages/cli/src/commands/init/steps/install-skills.ts @@ -0,0 +1,180 @@ +import * as p from '@clack/prompts' +import { detectDrizzle, detectPrismaNext } from '../../db/detect.js' +import { type AgentEnvironment, detectAgents } from '../detect-agents.js' +import { installSkills } from '../lib/install-skills.js' +import type { + HandoffChoice, + InitProvider, + InitState, + InitStep, + Integration, + SkillsDelivery, +} from '../types.js' +import { mergeSkillsDelivery } from '../types.js' + +export const CLAUDE_SKILLS_DIR = '.claude/skills' +export const CODEX_SKILLS_DIR = '.codex/skills' + +/** + * Which skills directories this run should write, in install order. + * + * An explicit `--target` wins outright — it is the user saying which agent + * they are setting up for, and detection must not add a second directory + * they did not ask for. The three non-directory targets (`agents-md`, + * `lovable`, `wizard`) deliberately return nothing: those handoffs inline + * skill bodies into AGENTS.md (or, for the wizard, install their own), and + * `stash init` performs no handoff, so there is nothing here for it to write. + * + * Without a flag, both CLIs are honoured independently — a machine with + * `claude` and `codex` both on PATH gets both, because either might be the + * one that picks the project up. Project-level artifacts count as evidence + * alongside the CLI: an agent running inside a repo that already has + * `.claude/` is the exact flow #923 is about, and it does not require the + * binary to be on the PATH this process inherited. + */ +export function skillDestinations( + agents: AgentEnvironment, + target: HandoffChoice | undefined, +): string[] { + if (target === 'claude-code') return [CLAUDE_SKILLS_DIR] + if (target === 'codex') return [CODEX_SKILLS_DIR] + if (target !== undefined) return [] + + const dests: string[] = [] + if (agents.cli.claudeCode || agents.project.claudeDir) { + dests.push(CLAUDE_SKILLS_DIR) + } + if (agents.cli.codex || agents.project.codexDir) { + dests.push(CODEX_SKILLS_DIR) + } + return dests +} + +/** + * Best guess at the integration BEFORE `resolve-database` and `build-schema` + * have run, used only to pick a skill set. + * + * Returns `undefined` rather than guessing when nothing is conclusive, so + * the caller falls back to `BASE_SKILLS` instead of shipping a wrong + * integration's skills. + * + * The cwd signals and their precedence come from `build-schema`'s + * `detectIntegration`, minus the one it cannot answer yet: `detectSupabase` + * reads the resolved `DATABASE_URL`, which does not exist this early. On top + * of those, the `--drizzle` and `--supabase` FLAGS count as conclusive here, + * which `detectIntegration` does not do — it consults only `--prisma`. That + * asymmetry is right for the encryption client (a project with no Drizzle + * config should not get a Drizzle-shaped one just because a flag was passed) + * and wrong for skills: a user who typed `--drizzle` is telling us which + * integration to teach the agent about, and answering `undefined` here hands + * them the base six instead of the Drizzle seven. + * + * Drizzle outranks Supabase on a combined `--drizzle --supabase` run, the + * same way it does when init routes the EQL migration — it owns the + * migration history there, and `--supabase` is the grants modifier. + * + * A bare `stash init` against a Supabase-hosted URL still falls through to + * `undefined` and is corrected by the top-up in `build-schema`. + * + * Reads `provider.selected`, never `provider.name`: a combined + * `--prisma --supabase` run names itself `'prisma-supabase'`, which equals + * no single flag. + */ +export function guessIntegration( + cwd: string, + provider: InitProvider, +): Integration | undefined { + if (provider.selected.includes('prisma')) return 'prisma-next' + if (detectPrismaNext(cwd)) return 'prisma-next' + if (detectDrizzle(cwd) || provider.selected.includes('drizzle')) { + return 'drizzle' + } + if (provider.selected.includes('supabase')) return 'supabase' + return undefined +} + +/** + * Copy the agent skills into the project — the FIRST thing `stash init` does. + * + * Ordering is the whole point of this step. `installSkills` needs no network, + * no credentials and no database; every other init step needs at least one of + * those and can fail. Running last (as the handoff steps effectively did) meant + * a run that died at auth, at the database URL, or at EQL delivered no guidance + * at all — and those failures are precisely when an agent needs `stash-cli`. + * Running first, the skills survive any later exit, including Ctrl+C. + * + * Never fatal: `installSkills` degrades every filesystem error to a warning + * (#736), and a run with no agent to install for is a normal outcome, not an + * error. The init summary reports whichever happened — a silent + * `installedSkills: []` is what made #923 invisible for a whole release. + */ +export const installSkillsStep: InitStep = { + id: 'install-skills', + name: 'Install agent skills', + async run(state: InitState, provider: InitProvider): Promise { + const cwd = process.cwd() + const agents = detectAgents(cwd, process.env) + const integration = guessIntegration(cwd, provider) + const dests = skillDestinations(agents, state.targetFlag) + + let skills: SkillsDelivery = { installed: [], inlined: [], failed: [] } + for (const dest of dests) { + const { copied, failed } = installSkills(cwd, dest, integration) + if (copied.length > 0) { + p.log.success( + `Installed ${copied.length} skill${copied.length !== 1 ? 's' : ''} into ${dest}/: ${copied.join(', ')}`, + ) + } + if (failed.length > 0) { + p.log.warn( + `${failed.length} skill${failed.length !== 1 ? 's' : ''} could not be installed to ${dest}/: ${failed.join(', ')}.`, + ) + } + skills = mergeSkillsDelivery(skills, { + installed: copied, + inlined: [], + failed, + }) + } + + // `agents` is stored for `gather-context` (and the handoff steps, when + // `plan`/`impl` reuse this state) so the PATH walk happens once per run. + return { ...state, agents, skills } + }, +} + +/** + * Top up the installed skills once `build-schema` has resolved the real + * integration. + * + * Only does anything for a run the first-step install could not classify — + * in practice a bare `stash init` whose Supabase-ness lives in the + * `DATABASE_URL` host. `installSkills` is idempotent (`cpSync` with + * `force`), the per-integration set is a superset of `BASE_SKILLS`, and the + * destinations are re-derived from the same inputs, so a re-run of the same + * classification copies the same files again and reports the same names. + * + * Silent on success: the first step already announced the install, and a + * second "installed N skills" line for four extra files reads like the work + * happened twice. + */ +export function topUpSkills( + cwd: string, + state: InitState, + integration: Integration, +): SkillsDelivery | undefined { + if (!state.agents) return state.skills + const dests = skillDestinations(state.agents, state.targetFlag) + if (dests.length === 0) return state.skills + + let skills = state.skills + for (const dest of dests) { + const { copied, failed } = installSkills(cwd, dest, integration) + skills = mergeSkillsDelivery(skills, { + installed: copied, + inlined: [], + failed, + }) + } + return skills +} diff --git a/packages/cli/src/commands/init/types.ts b/packages/cli/src/commands/init/types.ts index 1488e13e6..fefcbcc87 100644 --- a/packages/cli/src/commands/init/types.ts +++ b/packages/cli/src/commands/init/types.ts @@ -74,6 +74,57 @@ export type HandoffChoice = */ export type InitMode = 'plan' | 'implement' +/** + * How the per-integration skills reached (or failed to reach) the project. + * Threaded into `context.json` and the setup prompt so both describe what + * actually happened, not what the handoff hoped for: + * + * installed — copied into a skills directory (`.claude/skills`, + * `.codex/skills`) + * inlined — bodies written into AGENTS.md under "## Skill references" + * (the editor-agent handoff, and the Codex fallback for an + * unwritable `.codex/` — #736) + * failed — bundled skills that ended up nowhere (destination + * unwritable with no inline fallback, or AGENTS.md itself + * unwritable) + * + * Lives here rather than in `lib/write-context.ts` (where it started) so + * `InitState` can carry one without a circular import. + */ +export interface SkillsDelivery { + installed: string[] + inlined: string[] + failed: string[] +} + +/** Merge two deliveries, de-duplicating and keeping a stable order. + * + * Exists because skills now reach a project in more than one hop: `stash + * init` installs them first thing, and a later `stash plan` / `stash impl` + * handoff installs or inlines its own set. `writeArtifacts` used to + * OVERWRITE `context.json.installedSkills` with just the current hop's + * result, so `stash plan --target agents-md` (which installs no + * directories) reset the field to `[]` on a project that had them — the + * same false-empty state #923 is about, reached one command later. + * + * A name that failed in one hop and succeeded in another counts as + * delivered: `failed` is filtered against the merged successes so the + * record never reports a skill as both. + */ +export function mergeSkillsDelivery( + a: SkillsDelivery | undefined, + b: SkillsDelivery, +): SkillsDelivery { + const dedupe = (xs: string[]) => [...new Set(xs)] + const installed = dedupe([...(a?.installed ?? []), ...b.installed]) + const inlined = dedupe([...(a?.inlined ?? []), ...b.inlined]) + const delivered = new Set([...installed, ...inlined]) + const failed = dedupe([...(a?.failed ?? []), ...b.failed]).filter( + (name) => !delivered.has(name), + ) + return { installed, inlined, failed } +} + export interface InitState { authenticated?: boolean /** Region passed via `--region` / `STASH_REGION`. Consumed by the @@ -120,8 +171,19 @@ export interface InitState { * values. Set by build-schema (so the baseline context.json has them); * read by the handoff steps without re-scanning. */ envKeys?: string[] - /** Available coding agents in the user's environment. Set by detect-agents. */ + /** Available coding agents in the user's environment. Set by the + * install-skills step (the first thing `stash init` runs) and reused by + * gather-context rather than re-walking `PATH`. */ agents?: AgentEnvironment + /** Where the per-integration skills ended up. Accumulated across every hop + * that delivers them — the init step, then any later handoff — and read by + * `buildContextFile` so `context.json` reports the union rather than the + * most recent hop. */ + skills?: SkillsDelivery + /** Validated `--target` from `stash init`. On `init` this selects the + * skills destination ONLY; it does not perform a handoff the way + * `plan --target` / `impl --target` do. Absent means "auto-detect". */ + targetFlag?: HandoffChoice /** What the user picked at the "how to proceed" step. */ handoff?: HandoffChoice /** True when the handoff step actually launched an agent process diff --git a/packages/cli/src/commands/plan/index.ts b/packages/cli/src/commands/plan/index.ts index c303a8e03..3f435d765 100644 --- a/packages/cli/src/commands/plan/index.ts +++ b/packages/cli/src/commands/plan/index.ts @@ -8,7 +8,7 @@ import { messages } from '../../messages.js' import { HANDOFF_CHOICES, howToProceedStep, - resolveTarget, + resolveTargetFlag, } from '../impl/steps/how-to-proceed.js' import { type AgentEnvironment, detectAgents } from '../init/detect-agents.js' import type { PlanStep } from '../init/lib/parse-plan.js' @@ -62,6 +62,15 @@ function buildStateFromContext( clientFilePath: ctx.encryptionClientPath, schemas: ctx.schemas, envKeys: ctx.envKeys, + // Carry the skills already on disk so the handoff's `writeArtifacts` + // merges into them instead of overwriting the record with just its own + // delivery — an `agents-md` handoff installs no directories, and used to + // reset `installedSkills` to `[]` on a project that had them (#923). + skills: { + installed: ctx.installedSkills ?? [], + inlined: ctx.inlinedSkills ?? [], + failed: [], + }, stackInstalled: true, cliInstalled: true, eqlInstalled: true, @@ -201,12 +210,9 @@ export async function planCommand( process.exit(1) } - const targetFlag = values.target - const target = resolveTarget(targetFlag) - if (targetFlag && !target) { - p.log.error( - `Unknown --target \`${targetFlag}\`. Valid values: ${HANDOFF_CHOICES.join(', ')}.`, - ) + const { target, error: targetError } = resolveTargetFlag(flags, values) + if (targetError) { + p.log.error(targetError) process.exit(1) } diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index a63e998a7..8f44f6aeb 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -38,13 +38,13 @@ npx stash init --prisma # Prisma Next npx stash init --drizzle --supabase # Drizzle on Supabase (the flags combine) ``` -`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 the top-level `prisma-next migrate`, and the **Drizzle** and **local Supabase** flows (a Supabase project with a local `supabase/` directory — a hosted one with no CLI scaffolding installs directly) *generate* an EQL migration, which init reports honestly as "EQL migration generated — apply it with `drizzle-kit migrate`" (Supabase: `supabase db reset` locally, `supabase db push` remotely) rather than claiming the extension is already installed. Re-running init over a project whose install migration is already on disk reports "EQL migration **already present**" — same apply step, same zero exit, no claim that this run generated anything. +`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. **Init installs the agent skills first, before anything else.** The per-integration set — the `stash-*` skills bundled in the `stash` tarball, chosen by integration — is copied into `.claude/skills/` when Claude Code is detected — the `claude` binary on `PATH`, or a `.claude/` directory in the project — and into `.codex/skills/` for Codex, both when both are detected. `--target claude-code|codex` names the destination explicitly and skips detection. Skills install ahead of authentication deliberately: it is the only step needing neither network nor credentials, so the guidance survives a run that later fails at auth, at the database URL, or at EQL — which is when it is needed most. The summary reports the outcome either way, and `.cipherstash/context.json` records the names under `installedSkills`. When no agent is detected nothing is written and the summary says so, with the command that will install them. 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 the top-level `prisma-next migrate`, and the **Drizzle** and **local Supabase** flows (a Supabase project with a local `supabase/` directory — a hosted one with no CLI scaffolding installs directly) *generate* an EQL migration, which init reports honestly as "EQL migration generated — apply it with `drizzle-kit migrate`" (Supabase: `supabase db reset` locally, `supabase db push` remotely) rather than claiming the extension is already installed. Re-running init over a project whose install migration is already on disk reports "EQL migration **already present**" — same apply step, same zero exit, no claim that this run generated anything. **If you are an agent, do this first:** 1. **`npx stash manifest --json`** — the structured, version-stamped command surface. Read it before running anything else. 2. **`npx stash auth login --json --region `** — only if not already authenticated. Surface the URL to the human (see [Authentication](#authentication)). Do this *before* `init`. -3. **`npx stash init`** — now finds the token and proceeds without prompting. +3. **`npx stash init`** — now finds the token and proceeds without prompting. It installs the per-integration skills into `.claude/skills/` or `.codex/skills/` as its **first** step, before authenticating, so read them from there once init has run — pass `--target` if you want to name the destination yourself. If the summary says `No agent skills installed`, no agent was detected: run `stash plan --target ` to install them. 4. **`stash plan` → `stash impl` → `stash status`** — pass `--target` when non-interactive. ### Ask the CLI, don't trust this file @@ -132,11 +132,14 @@ There is **no global `--non-interactive` or `--json` flag** (and no global `--ye | Region (`auth login`, `init`) | `--region ` or `STASH_REGION` | | Database URL (all `db` / `eql` / `schema` commands) | `--database-url ` or `DATABASE_URL` | | Agent target (`plan`, `impl`) | `--target ` | +| Skills destination (`init`) | `--target ` — selects the destination only; `init` performs no handoff | | Dual-write confirmation (`encrypt backfill`) | `--confirm-dual-writes-deployed` | | Machine-readable output | `--json` on `status`, `manifest`, `auth login`, `auth regions` | When a required value is missing in a non-TTY context, the command exits non-zero with an actionable message naming the flag and env var — it never hangs. +**`init --target` is not the same flag as `plan --target` / `impl --target`.** On `plan` and `impl`, `--target` selects the agent to hand off to. On `init` it selects only where the bundled skills are copied — `claude-code` → `.claude/skills/`, `codex` → `.codex/skills/` — and skips agent detection; `init` never performs a handoff. `agents-md`, `lovable` and `wizard` are accepted but install no skill directories, because those handoffs inline the skill bodies into `AGENTS.md` instead. The flag is optional everywhere: without it, `init` detects the agent itself. + **`plan` and `impl` need `--target` in a non-TTY.** Their agent-target picker reads from `/dev/tty`. Without `--target` they print a "no agent selected" hint and exit 0 *without performing the handoff*. `init` and `status` adapt automatically and are safe anywhere. **Exit codes.** `1` on failure; `0` when a user cancels a prompt. In `--json` mode an `{ "status": "error", "code", "message" }` line is emitted before exiting 1. @@ -229,23 +232,25 @@ Four explicit save-points. Each runs standalone; chain prompts make first-time s ### `init` — scaffold -Six mechanical steps, no agent handoff. It prompts only when it can't pick a sensible default. +Seven mechanical steps, no agent handoff. It prompts only when it can't pick a sensible default. -1. **Authenticate** — silent when a valid token exists. -2. **Resolve database** — per the resolution order above; verifies the connection. -3. **Build schema** — auto-detects Drizzle, Supabase, and Prisma Next and writes the placeholder encryption client. **Prisma Next is the exception:** it derives schemas from `contract.json`, so no encryption-client file is written and none is needed. -4. **Install dependencies** — one combined prompt for `@cipherstash/stack` and `stash`. -5. **Install EQL** — always EQL v3, migration-first wherever there is a migration history to land in. Drizzle generates `eql migration --drizzle`; a Supabase project with a local `supabase/` directory generates `eql migration --supabase`; Prisma Next installs through `prisma-next migrate`; everything else (including a hosted Supabase project with no CLI scaffolding) installs directly. The migration routes leave EQL **generated, not applied** — the summary says so, and you run the migrate step yourself. -6. **Gather context** — detects available coding agents and writes `.cipherstash/context.json`. +1. **Install agent skills** — copies the per-integration `stash-*` skills into `.claude/skills/` and/or `.codex/skills/`, per detection or `--target`. First deliberately: it needs no network and no credentials, so the guidance survives a failure in any step below. +2. **Authenticate** — silent when a valid token exists. +3. **Resolve database** — per the resolution order above; verifies the connection. +4. **Build schema** — auto-detects Drizzle, Supabase, and Prisma Next, writes the placeholder encryption client, and writes `.cipherstash/context.json`. **Prisma Next is the exception:** it derives schemas from `contract.json`, so no encryption-client file is written and none is needed. +5. **Install dependencies** — one combined prompt for `@cipherstash/stack` and `stash`. +6. **Install EQL** — always EQL v3, migration-first wherever there is a migration history to land in. Drizzle generates `eql migration --drizzle`; a Supabase project with a local `supabase/` directory generates `eql migration --supabase`; Prisma Next installs through `prisma-next migrate`; everything else (including a hosted Supabase project with no CLI scaffolding) installs directly. The migration routes leave EQL **generated, not applied** — the summary says so, and you run the migrate step yourself. +7. **Gather context** — summarises what was detected. -Flags: `--supabase`, `--drizzle`, `--prisma`, `--region `. +Flags: `--supabase`, `--drizzle`, `--prisma`, `--region `, `--target `. **The integration flags combine.** `stash init --drizzle --supabase` is a Drizzle project on Supabase: the EQL migration goes into your Drizzle migrations folder (drizzle-kit owns the history there) with the Supabase role grants appended, both adapter packages are installed, and the database-URL resolver may use `supabase status` to find a local stack. `--prisma` combined with another flag still takes the Prisma Next route. Combined flags are recorded together as the referrer (`drizzle-supabase`), exactly as `stash auth login --drizzle --supabase` does. This is `init` only — `eql migration` takes exactly one target (see below). | Generated file | Purpose | |---|---| | `./src/encryption/index.ts` | Placeholder encryption client — declare encrypted columns here, or let `plan`/`impl` do it. **Not written for Prisma Next** (`--prisma`), which derives schemas from `contract.json` | -| `.cipherstash/context.json` | Detected facts: integration, package manager, schemas, env key names, and agents. CLI-owned; never hand-edit | +| `.cipherstash/context.json` | Detected facts: integration, package manager, schemas, env key names, and the skills installed (`installedSkills`). CLI-owned; never hand-edit | +| `.claude/skills/` or `.codex/skills/` | The per-integration `stash-*` skills, when an agent is detected or `--target` names one | | `stash.config.ts` | Scaffolded if missing | ### `plan` — draft for review