-
Notifications
You must be signed in to change notification settings - Fork 3
feat(cli): stash wizard thin-wrapper subcommand #394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| --- | ||
| 'stash': minor | ||
| --- | ||
|
|
||
| Add `stash wizard` as a thin wrapper subcommand around `@cipherstash/wizard`. | ||
|
|
||
| The wizard ships as a separate npm package so the heavy agent SDK stays out of the `stash` CLI bundle. Until now, users had to remember a second tool name (`npx @cipherstash/wizard`); the wrapper exposes the same capability under the existing `stash` surface so the user only has to think about one CLI. | ||
|
|
||
| `stash wizard` detects the project's package manager and spawns the wizard via the matching one-shot runner — `npx`, `pnpm dlx`, `yarn dlx`, or `bunx` — with `stdio: 'inherit'` so the wizard owns the terminal cleanly. Any flags after `wizard` are forwarded verbatim, so `stash wizard --debug` works. | ||
|
|
||
| On a cold cache (the wizard package isn't installed in the project) the runner downloads it before launching — a few seconds. The wrapper prints an explicit "first run downloads ~5s" line in that case so the CLI doesn't appear hung. On a warm cache, just a "Launching the CipherStash wizard…" line, then the wizard takes over. | ||
|
|
||
| Existing copy that pointed at `npx @cipherstash/wizard` (init's next-steps for base / Drizzle / Supabase, `db install`'s post-install note) now uses `stash wizard`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { splitRunner } from '../index.js' | ||
|
|
||
| describe('splitRunner', () => { | ||
| it('splits a single-token runner like bunx', () => { | ||
| expect(splitRunner('bunx @cipherstash/wizard')).toEqual({ | ||
| bin: 'bunx', | ||
| preArgs: ['@cipherstash/wizard'], | ||
| }) | ||
| }) | ||
|
|
||
| it('splits a multi-token runner like pnpm dlx', () => { | ||
| expect(splitRunner('pnpm dlx @cipherstash/wizard')).toEqual({ | ||
| bin: 'pnpm', | ||
| preArgs: ['dlx', '@cipherstash/wizard'], | ||
| }) | ||
| }) | ||
|
|
||
| it('splits yarn dlx', () => { | ||
| expect(splitRunner('yarn dlx @cipherstash/wizard')).toEqual({ | ||
| bin: 'yarn', | ||
| preArgs: ['dlx', '@cipherstash/wizard'], | ||
| }) | ||
| }) | ||
|
|
||
| it('splits npx', () => { | ||
| expect(splitRunner('npx @cipherstash/wizard')).toEqual({ | ||
| bin: 'npx', | ||
| preArgs: ['@cipherstash/wizard'], | ||
| }) | ||
| }) | ||
|
|
||
| it('collapses consecutive whitespace', () => { | ||
| expect(splitRunner('pnpm dlx @cipherstash/wizard')).toEqual({ | ||
| bin: 'pnpm', | ||
| preArgs: ['dlx', '@cipherstash/wizard'], | ||
| }) | ||
| }) | ||
|
|
||
| it('throws on an empty runner', () => { | ||
| expect(() => splitRunner('')).toThrow(/Empty runner command/i) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,74 @@ | ||||||||||||||||||||||||||||||||||||
| import { spawn } from 'node:child_process' | ||||||||||||||||||||||||||||||||||||
| import * as p from '@clack/prompts' | ||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||
| detectPackageManager, | ||||||||||||||||||||||||||||||||||||
| isPackageInstalled, | ||||||||||||||||||||||||||||||||||||
| runnerCommand, | ||||||||||||||||||||||||||||||||||||
| } from '../init/utils.js' | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const WIZARD_PACKAGE = '@cipherstash/wizard' | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Resolve the runner invocation into argv-style tokens for `spawn`. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * `runnerCommand` returns strings like `'pnpm dlx @cipherstash/wizard'` or | ||||||||||||||||||||||||||||||||||||
| * `'npx @cipherstash/wizard'`. Splitting on whitespace is safe here because | ||||||||||||||||||||||||||||||||||||
| * every token is constructed from a closed enum (the package manager name | ||||||||||||||||||||||||||||||||||||
| * and the literal package). We avoid `shell: true` so we don't have to | ||||||||||||||||||||||||||||||||||||
| * worry about quoting user-passed flags downstream. | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| function splitRunner(cmd: string): { bin: string; preArgs: string[] } { | ||||||||||||||||||||||||||||||||||||
| const tokens = cmd.split(/\s+/).filter(Boolean) | ||||||||||||||||||||||||||||||||||||
| const [bin, ...preArgs] = tokens | ||||||||||||||||||||||||||||||||||||
| if (!bin) { | ||||||||||||||||||||||||||||||||||||
| // Should be unreachable — runnerCommand always returns at least one token. | ||||||||||||||||||||||||||||||||||||
| throw new Error(`Empty runner command: "${cmd}"`) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return { bin, preArgs } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||
| * Thin wrapper around `@cipherstash/wizard`. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * The wizard ships as its own package so the heavy agent SDK stays out of the | ||||||||||||||||||||||||||||||||||||
| * `stash` CLI bundle. This wrapper exists so users see one CLI surface | ||||||||||||||||||||||||||||||||||||
| * (`stash wizard`) instead of being told to remember a second tool name. | ||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||
| * On a cold cache (the wizard package isn't installed in the project) the | ||||||||||||||||||||||||||||||||||||
| * package manager will download it before running — that can take a few | ||||||||||||||||||||||||||||||||||||
| * seconds. We surface that explicitly so the user doesn't think the CLI is | ||||||||||||||||||||||||||||||||||||
| * hung. We don't show a spinner because the wizard itself uses clack and | ||||||||||||||||||||||||||||||||||||
| * needs an inherited TTY; intercepting child stdout would break the wizard's | ||||||||||||||||||||||||||||||||||||
| * own UI. | ||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||
| export async function wizardCommand(passthroughArgs: string[]): Promise<void> { | ||||||||||||||||||||||||||||||||||||
| const pm = detectPackageManager() | ||||||||||||||||||||||||||||||||||||
| const runner = runnerCommand(pm, WIZARD_PACKAGE) | ||||||||||||||||||||||||||||||||||||
| const cached = isPackageInstalled(WIZARD_PACKAGE) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (cached) { | ||||||||||||||||||||||||||||||||||||
| p.log.info('Launching the CipherStash wizard...') | ||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||
| p.log.info( | ||||||||||||||||||||||||||||||||||||
| `Launching the CipherStash wizard... first run downloads ${WIZARD_PACKAGE} (~5s).`, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const { bin, preArgs } = splitRunner(runner) | ||||||||||||||||||||||||||||||||||||
| const args = [...preArgs, ...passthroughArgs] | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const exitCode = await new Promise<number>((resolvePromise) => { | ||||||||||||||||||||||||||||||||||||
| const child = spawn(bin, args, { stdio: 'inherit', shell: false }) | ||||||||||||||||||||||||||||||||||||
| child.on('close', (code) => resolvePromise(code ?? 0)) | ||||||||||||||||||||||||||||||||||||
| child.on('error', (err) => { | ||||||||||||||||||||||||||||||||||||
| p.log.error(`Failed to launch wizard: ${err.message}`) | ||||||||||||||||||||||||||||||||||||
| resolvePromise(127) | ||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+60
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check package.json engines field and any CI matrix for Windows runners
rg -n '"engines"' packages/cli/package.json
fd -e yml -e yaml . .github/workflows --exec grep -l 'windows'Repository: cipherstash/stack Length of output: 78 🏁 Script executed: cat packages/cli/package.json | head -100Repository: cipherstash/stack Length of output: 1741 🏁 Script executed: rg -A 5 '"engines"' packages/cli/package.jsonRepository: cipherstash/stack Length of output: 97 🏁 Script executed: cat -n packages/cli/src/commands/wizard/index.ts | sed -n '50,75p'Repository: cipherstash/stack Length of output: 925 🏁 Script executed: rg -B 5 -A 15 'function splitRunner|export.*splitRunner|const splitRunner' packages/cli/src/commands/wizard/Repository: cipherstash/stack Length of output: 2181 🏁 Script executed: rg -n "splitRunner" packages/cli/src/commands/wizard/index.tsRepository: cipherstash/stack Length of output: 212 🏁 Script executed: rg -B 10 "const runner = " packages/cli/src/commands/wizard/index.tsRepository: cipherstash/stack Length of output: 622 🏁 Script executed: rg -B 20 "function splitRunner" packages/cli/src/commands/wizard/index.ts | head -40Repository: cipherstash/stack Length of output: 790 🏁 Script executed: rg -A 30 "function runnerCommand|export.*runnerCommand" packages/cli/src/commands/init/utils.tsRepository: cipherstash/stack Length of output: 745 🏁 Script executed: rg -A 20 "function detectPackageManager|export.*detectPackageManager" packages/cli/src/commands/init/utils.tsRepository: cipherstash/stack Length of output: 734 🏁 Script executed: rg -n "windows|Windows|WIN32|win32" packages/cli/README.md packages/cli/CHANGELOG.md 2>/dev/nullRepository: cipherstash/stack Length of output: 43 🏁 Script executed: find packages/cli -name "*.md" -type f | xargs grep -l "Windows\|windows\|platform" 2>/dev/nullRepository: cipherstash/stack Length of output: 45 🏁 Script executed: rg "spawn.*bin.*shell.*false" packages/cli/src/ -A 5 -B 5Repository: cipherstash/stack Length of output: 944
These package managers are distributed as While Windows support is not explicitly declared in the 🐛 Proposed fix using
|
||||||||||||||||||||||||||||||||||||
| const exitCode = await new Promise<number>((resolvePromise) => { | |
| const child = spawn(bin, args, { stdio: 'inherit', shell: false }) | |
| child.on('close', (code) => resolvePromise(code ?? 0)) | |
| child.on('error', (err) => { | |
| p.log.error(`Failed to launch wizard: ${err.message}`) | |
| resolvePromise(127) | |
| }) | |
| }) | |
| const exitCode = await new Promise<number>((resolvePromise) => { | |
| const winBin = process.platform === 'win32' ? `${bin}.cmd` : bin | |
| const child = spawn(winBin, args, { stdio: 'inherit', shell: false }) | |
| child.on('close', (code) => resolvePromise(code ?? 0)) | |
| child.on('error', (err) => { | |
| p.log.error(`Failed to launch wizard: ${err.message}`) | |
| resolvePromise(127) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/index.ts` around lines 60 - 67, The Windows
failure is caused by using node's child_process.spawn (spawn) with shell: false
which cannot execute package manager .cmd shims (npm/pnpm/yarn); replace the use
of spawn in this block with cross-spawn's default export (import spawn from
'cross-spawn') and call it with the same signature (spawn(bin, args, { stdio:
'inherit' })) so .cmd is appended on Windows; keep the child.on('close', ...)
and child.on('error', ...) handlers and the exitCode Promise logic unchanged and
ensure TypeScript types/imports are updated to reference cross-spawn instead of
child_process.spawn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
E2E test required by coding guidelines for this routing change.
The
wizardcase introduces a new exit-code propagation path (wizardCommandcallsprocess.exit(exitCode)) routed throughstash.ts. The coding guidelines explicitly require an E2E test when touchingsrc/bin/stash.ts"argv parsing, exit codes, or top-level error handling". The PR's test plan covers only unit tests forsplitRunner; no E2E test forstash wizardrouting is listed.At minimum, an E2E test should verify that
stash wizard --help(or equivalent) exits 0 and that an unknown/failing wizard invocation propagates the expected non-zero exit code.As per coding guidelines: "Add an E2E test when touching
src/bin/stash.tsargv parsing, exit codes, or top-level error handling."🤖 Prompt for AI Agents