feat(cli): plan-or-implement choice in stash init#412
Merged
calvinbrewer merged 8 commits intomainfrom May 6, 2026
Merged
Conversation
Adds a step before the handoff target picker that asks the user whether the agent should produce a reviewable plan first or go straight to implementation. Plan-first is the default — for migrate-existing-column work the wrong order of operations is hard to recover from, so a plan checkpoint is the safer default. Plan mode currently routes only to Claude Code or Codex (AGENTS.md and the CipherStash Agent / wizard don't yet have planning prompt templates). The implementation prompt now reads `.cipherstash/plan.md` as the source of truth for routing if it exists, rather than re-asking which path applies. Closes #408
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR introduces a plan-first workflow to the CipherStash CLI by separating initialization into three commands: ChangesType System & Mode Support
Init Command Refactoring & Plan Integration
Impl Command & Handoff Refactoring
Status Command
CLI Wiring & Tests
Documentation (Changesets)
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Context
participant Agent
participant FS as Filesystem
User->>CLI: stash init
CLI->>CLI: Scaffold (auth, db, encrypt, EQL, context)
CLI->>Context: Save context to context.json
CLI->>User: Setup complete! Ready to plan?
alt User chooses Plan
User->>CLI: stash plan
CLI->>Context: Load context.json
CLI->>Agent: Invoke with plan-mode prompt
Agent->>FS: Write .cipherstash/plan.md
CLI->>User: Plan drafted! Ready to implement?
User->>CLI: stash impl
CLI->>FS: Read plan.md
CLI->>Agent: Invoke with implement-mode prompt
Agent->>FS: Execute plan (code changes)
else User chooses Direct Implement
User->>CLI: stash impl --continue-without-plan
CLI->>Agent: Invoke with implement-mode prompt
Agent->>FS: Execute directly (code changes)
end
Agent->>User: Implementation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🦋 Changeset detectedLatest commit: 8266859 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
We split and added to gamify the encryption setup with a save-point between scaffolding and the agent handoff, plus an opt-in prompt at end of init to chain straight into plan mode.
calvinbrewer
approved these changes
May 6, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/init/lib/setup-prompt.ts (1)
177-423: ⚡ Quick winConsider extracting shared section builders to reduce drift between the two renderers.
renderImplementPromptandrenderPlanPromptshare several near-identical sections that will need to be kept in sync as the prompt copy evolves:
- The
donechecklist construction (lines 181–200 vs 308–327) is byte-identical except for the variable name.- The "What
stash initalready did" + "Skills loaded" intro paragraph (lines 215–225 vs 336–346).- The first two "Stop and ask" bullets (the in-place-conversion and post-cutover-re-encryption cases).
- The "Your first response" orientation framing.
Hoisting these into small helpers (e.g.
renderDoneChecklist(ctx),renderSkillsSection(ctx),renderCommonStopAndAsk()) would keep both prompts aligned by construction and shrink each renderer to just the parts that genuinely differ between plan and implement modes.♻️ Sketch of the extraction
+function renderDoneChecklist(ctx: SetupPromptContext): string[] { + const done: string[] = [ + checked('Authenticated to CipherStash and selected a workspace'), + checked(`Detected integration: \`${ctx.integration}\``), + checked( + `Wrote a placeholder encryption client at \`${ctx.encryptionClientPath}\` (a small file showing the encryption-client patterns; the user's real Drizzle/Supabase schema files remain authoritative)`, + ), + ] + if (ctx.stackInstalled) done.push(checked('Installed `@cipherstash/stack` (runtime)')) + if (ctx.cliInstalled) done.push(checked('Installed `stash` (CLI, dev dep)')) + if (ctx.eqlInstalled) { + done.push( + checked('Installed the EQL extension and `cipherstash.cs_migrations` into the database'), + ) + } + return done +} + +function renderSkillsSection(ctx: SetupPromptContext): string[] { + return [ + '## 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.', + '', + ] +}Both renderers can then splice those arrays in instead of repeating the strings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/lib/setup-prompt.ts` around lines 177 - 423, Both renderImplementPrompt and renderPlanPrompt duplicate large blocks (the done checklist, the skills/intro paragraph, shared "stop and ask" bullets, and the "Your first response" framing); extract these into small helpers (e.g. renderDoneChecklist(ctx), renderSkillsSection(ctx), renderCommonStopAndAsk(), renderOrientationPrompt(ctx)) and replace the duplicated arrays/strings in renderImplementPrompt and renderPlanPrompt with calls that splice in the returned arrays/strings, preserving existing symbols like renderImplementPrompt, renderPlanPrompt, PLAN_REL_PATH, runnerCommand and migrationCommands so callers remain unchanged; keep helper names unique and colocate them near these functions to minimise drift and ensure tests/consumers still join the sections with sections.join('\n').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/commands/init/lib/setup-prompt.ts`:
- Around line 177-423: Both renderImplementPrompt and renderPlanPrompt duplicate
large blocks (the done checklist, the skills/intro paragraph, shared "stop and
ask" bullets, and the "Your first response" framing); extract these into small
helpers (e.g. renderDoneChecklist(ctx), renderSkillsSection(ctx),
renderCommonStopAndAsk(), renderOrientationPrompt(ctx)) and replace the
duplicated arrays/strings in renderImplementPrompt and renderPlanPrompt with
calls that splice in the returned arrays/strings, preserving existing symbols
like renderImplementPrompt, renderPlanPrompt, PLAN_REL_PATH, runnerCommand and
migrationCommands so callers remain unchanged; keep helper names unique and
colocate them near these functions to minimise drift and ensure tests/consumers
still join the sections with sections.join('\n').
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b63cdbe0-4ddc-4fb7-8313-0cd7622c08d8
📒 Files selected for processing (12)
.changeset/init-plan-or-implement.mdpackages/cli/src/commands/init/__tests__/how-to-proceed.test.tspackages/cli/src/commands/init/index.tspackages/cli/src/commands/init/lib/__tests__/setup-prompt.test.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/init/lib/write-context.tspackages/cli/src/commands/init/steps/choose-mode.tspackages/cli/src/commands/init/steps/handoff-claude.tspackages/cli/src/commands/init/steps/handoff-codex.tspackages/cli/src/commands/init/steps/how-to-proceed.tspackages/cli/src/commands/init/steps/next-steps.tspackages/cli/src/commands/init/types.ts
Dan/init plan or implement 2
Dan/init plan or implement 1
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/bin/stash.ts (1)
380-422:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an E2E test for the new
plan/impl/statusargv routing.This change extends the top-level switch with three new commands and introduces the
--continue-without-planflag whose parsing happens here. Per the repo guideline, that warrants an E2E test alongside the unit suite — at minimum to lock in that:
stash statusexits 0 in a virgin tree and referencesinitin its output.stash plandispatchesplanCommand(e.g. via env-gated stub or stdout assertion).stash impl --continue-without-planis parsed as a flag, not swallowed as a positional.- Unknown commands still exit 1.
These are exactly the regressions that pure unit tests on
planCommand/implCommand/statusCommandwon't catch, because the bug surface is the argv→dispatch wiring in this file.As per coding guidelines: "Add an E2E test when touching
src/bin/stash.tsargv parsing, exit codes, or top-level error handling".Want me to draft the E2E test scaffolding (spawning the built CLI with
STASH_DISABLE_TELEMETRY=1à la the wizard pattern) for these four cases?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/bin/stash.ts` around lines 380 - 422, Add an E2E test suite that spawns the built CLI binary (with STASH_DISABLE_TELEMETRY=1) to verify argv→dispatch in the top-level switch: assert `stash status` exits 0 and its stdout references "init"; assert `stash plan` dispatches planCommand (e.g., detect expected planCommand output or use an env-gated stub); assert `stash impl --continue-without-plan` treats `--continue-without-plan` as a flag (not a positional) by observing implCommand behavior/output when that flag is present; and assert an unknown command exits 1 and prints the help/unknown message; implement these tests by spawning the CLI, capturing stdout/stderr and exit codes and failing on mismatches.
🧹 Nitpick comments (10)
packages/cli/src/commands/init/lib/setup-prompt.ts (2)
308-346: ⚡ Quick winSignificant duplication between
renderPlanPromptandrenderImplementPrompt.The "What
stash initalready did"donelist (Line 308-327 vs Line 181-200), the integration/package-manager header, and the "Skills loaded" block (Line 340-346 vs Line 219-224) are identical between the two renderers. Future edits to e.g. the EQL bullet, or a new skill purpose, will need to be applied in both places — exactly the failure mode the existing test suite cannot catch (each renderer is tested in isolation).Consider extracting:
renderDoneList(ctx)returning thestring[]of checklist linesrenderHeader(ctx)for the integration/pm + intro paragraph (parameterized by intent string)renderSkillsSection(ctx)for the rules-location + skill-index linesBoth renderers then assemble a tailored body around the shared building blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/lib/setup-prompt.ts` around lines 308 - 346, Both renderPlanPrompt and renderImplementPrompt duplicate the same header, "What `stash init` already did" checklist, and "Skills loaded" block; extract these repeated parts into reusable helpers (e.g., add functions renderDoneList(ctx) that returns the done string[] used in the sections array, renderHeader(ctx, intent) that returns the integration/package-manager line and the intent paragraph, and renderSkillsSection(ctx) that returns the rulesLocation and renderSkillIndex lines) and have renderPlanPrompt and renderImplementPrompt call these helpers to assemble their final sections instead of duplicating the arrays and strings.
357-398: 💤 Low valuePlan deliverable instructions are thorough, but the docs/plans copy step competes with the plan being authoritative at
.cipherstash/plan.md.The instruction at Line 398 tells the agent to offer copying the plan into
docs/plans/cipherstash-encryption.md.stash implreads.cipherstash/plan.md(perrenderImplementPromptLine 211), so a copy indocs/plans/becomes a second source that can drift after edits. Either:
- spell out which file is canonical and that
docs/plans/cipherstash-encryption.mdis a snapshot for review only, or- have the agent write
docs/plans/cipherstash-encryption.mdfirst and symlink/copy to.cipherstash/plan.md(riskier, fs-dependent).The first option is the simpler clarification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/lib/setup-prompt.ts` around lines 357 - 398, The copy-to-docs step introduces a second authoritative plan that can drift from the canonical `.cipherstash/plan.md`; update the prose around PLAN_REL_PATH so it explicitly declares `.cipherstash/plan.md` as the canonical source and that `docs/plans/cipherstash-encryption.md` (if offered) is only a snapshot for review, not the source of truth, and mention that `renderImplementPrompt` reads `.cipherstash/plan.md`; alter the sentence offering to copy so it asks permission and states the copied file is a review snapshot only (do not create automatic symlinks or bidirectional copy logic).packages/cli/src/commands/init/types.ts (1)
61-64: 💤 Low valueDoc says
modeis set by the command itself, butmode?is optional.The comment claims
stash planandstash implalways set the mode, which suggests it's effectively required at the handoff step. Marking it required (or at least narrowing on the handoff-step types) would let the rendering layer drop its fallback path. Low priority sincemode='plan'is set inbuildStateFromContextand'implement'is presumably set in impl's equivalent — but a non-optional field makes intent explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/types.ts` around lines 61 - 64, The comment and types disagree: change the optional mode?: InitMode in the handoff type to a required mode: InitMode (or create a narrower HandoffInitState that requires mode: InitMode) so the handoff step always has a concrete InitMode; update any constructors/builders such as buildStateFromContext (and the impl equivalent) to continue setting 'plan' or 'implement' and adjust any callers that relied on an undefined mode to use the required field instead.packages/cli/src/commands/plan/index.ts (2)
74-89: 💤 Low valueChain prompt asserts the plan was drafted without checking the file.
The handoff step launches an external agent; whether the agent actually wrote
.cipherstash/plan.mdis not known to the CLI when it reaches Line 76. If the user dismisses the agent without producing a plan and accepts the chain prompt,stash implwill fail (or require--continue-without-plan) right after a message that claims the plan exists.♻️ Suggested adjustment
- if (process.stdout.isTTY) { - const proceed = await p.confirm({ - message: `Plan drafted at \`${PLAN_REL_PATH}\`. Continue to \`${cli} impl\` now?`, - initialValue: true, - }) - if (!p.isCancel(proceed) && proceed) { - p.outro('Plan complete — handing off to `stash impl`.') - const { implCommand } = await import('../impl/index.js') - await implCommand({}) - return - } - } - - p.outro( - `Plan drafted at \`${PLAN_REL_PATH}\`. Review it, then run \`${cli} impl\` to implement.`, - ) + const planExists = existsSync(resolve(cwd, PLAN_REL_PATH)) + if (process.stdout.isTTY && planExists) { + const proceed = await p.confirm({ + message: `Plan drafted at \`${PLAN_REL_PATH}\`. Continue to \`${cli} impl\` now?`, + initialValue: true, + }) + if (!p.isCancel(proceed) && proceed) { + p.outro('Plan complete — handing off to `stash impl`.') + const { implCommand } = await import('../impl/index.js') + await implCommand({}) + return + } + } + + p.outro( + planExists + ? `Plan drafted at \`${PLAN_REL_PATH}\`. Review it, then run \`${cli} impl\` to implement.` + : `When the agent has written \`${PLAN_REL_PATH}\`, run \`${cli} impl\` to implement.`, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/plan/index.ts` around lines 74 - 89, The prompt and outro incorrectly assume the plan file at PLAN_REL_PATH was written by the external agent; update the flow in the command handler so you check for the file's existence and non-emptiness before prompting or claiming the plan exists: after the external agent returns and before calling p.confirm or p.outro, stat the file at PLAN_REL_PATH (or read it) and if missing/empty surface a clear warning and offer a different prompt (e.g., "No plan found, continue to impl without a plan?") and only call implCommand({}) and show the "Plan drafted…" message when the file is present; keep references to PLAN_REL_PATH, p.confirm, p.outro and implCommand so the change is easy to locate.
44-55: 💤 Low valueExtract the missing-context pre-flight check into a reusable function.
The identical
readContextFile+ error pattern appears in bothplanandimplcommands. ExtractrequireContext(cwd, cli)intoinit/lib/read-context.tsto keep the error message and behavior synchronized across both commands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/plan/index.ts` around lines 44 - 55, Extract the duplicated pre-flight check into a reusable function by adding requireContext(cwd, cli) in init/lib/read-context.ts that calls readContextFile(cwd) and, if it returns falsy, calls p.log.error with the same message using CONTEXT_REL_PATH and the provided cli then process.exit(1); replace the existing readContextFile + error block in planCommand (and the impl command) with a single call to requireContext(process.cwd(), runnerCommand(detectPackageManager(), 'stash')) so both commands share identical behavior and message.packages/cli/src/commands/init/lib/read-context.ts (1)
13-21: ⚡ Quick winConsider validating the parsed JSON shape, not just JSON validity.
JSON.parse(...) as ContextFileaccepts any well-formed JSON — including a stalecontext.jsonfrom an older CLI version that's missing required fields. The same pattern inparse-plan.tsuses anisPlanSummarytype guard before returning; doing the same here would prevent downstream code (buildStateFromContext, handoff steps) from readingundefinedproperties off an unexpected shape and would letstash plan/stash implroute the user back tostash initwith a clear error rather than failing later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/lib/read-context.ts` around lines 13 - 21, The readContextFile function currently returns any parsed JSON cast as ContextFile; add a runtime shape validation (a type guard like isContextFile similar to isPlanSummary in parse-plan.ts) after JSON.parse and before returning to ensure required fields from ContextFile/CONTEXT_REL_PATH exist; if validation fails, return undefined (so buildStateFromContext / stash plan/stash impl can route users to stash init) and keep the try/catch around JSON.parse to still handle malformed JSON.packages/cli/src/commands/init/lib/parse-plan.ts (1)
90-113: 💤 Low valueLong
table.columnlabels silently break alignment.
padEnd(COLUMN_LABEL_WIDTH)is a no-op when${table}.${column}exceeds 20 characters — the row still works, but the description column drifts and the output looks ragged. Also worth noting:COLUMN_LABEL_WIDTH = 20is a fairly tight ceiling for typical table+column names (e.g.subscriptions.activated_atis 25). Consider sizing the column to the longest label in the summary, with a reasonable minimum:♻️ Suggested adjustment
-export function renderPlanSummary(summary: PlanSummary): string { +export function renderPlanSummary(summary: PlanSummary): string { const tables = new Set(summary.columns.map((c) => c.table)) const migrateCount = summary.columns.filter( (c) => c.path === 'migrate', ).length const colCount = summary.columns.length const tableCount = tables.size const header = `${colCount} column${colCount === 1 ? '' : 's'} across ${tableCount} table${tableCount === 1 ? '' : 's'}` - const rows = summary.columns.map((c) => { + const labelWidth = Math.max( + COLUMN_LABEL_WIDTH, + ...summary.columns.map((c) => `${c.table}.${c.column}`.length), + ) + const rows = summary.columns.map((c) => { const desc = c.path === 'new' ? 'add new encrypted column' : 'migrate existing column' - return `◇ ${`${c.table}.${c.column}`.padEnd(COLUMN_LABEL_WIDTH)} ${desc}` + return `◇ ${`${c.table}.${c.column}`.padEnd(labelWidth)} ${desc}` })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/init/lib/parse-plan.ts` around lines 90 - 113, renderPlanSummary currently uses a fixed COLUMN_LABEL_WIDTH which lets long `${c.table}.${c.column}` labels overflow and misalign descriptions; change the function to compute a dynamic labelWidth = Math.max(COLUMN_LABEL_WIDTH, ...summary.columns.map(c => `${c.table}.${c.column}`.length)) (ensuring a sensible minimum) and use that labelWidth in the padEnd call so the description column stays aligned; update references in renderPlanSummary to use this computed width instead of the constant.packages/cli/src/commands/status/__tests__/status.test.ts (1)
86-95: ⚡ Quick winConsider adding a wrong-shape companion to the malformed-JSON test.
This test guards the
JSON.parsethrow path. It does not cover the case wherecontext.jsonparses but is missing keys (e.g.{}or an older format withoutschemas) — which currently slips pastreadProjectStatusand crashesbuildStagesline 51 (raised as a separate comment onstatus/index.ts). If you add shape validation there, mirror it here:it('treats wrong-shape context.json as not-initialized', () => { mkdirSync(join(cwd, '.cipherstash'), { recursive: true }) writeFileSync(join(cwd, '.cipherstash', 'context.json'), '{}', 'utf-8') expect(readProjectStatus(cwd).initialized).toBe(false) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/status/__tests__/status.test.ts` around lines 86 - 95, Add a companion test covering the "wrong-shape" (parses but missing keys) case in the same test file by creating .cipherstash/context.json with '{}' and asserting readProjectStatus(cwd).initialized is false; also ensure the implementation of readProjectStatus validates the parsed shape (e.g., presence of expected keys like schemas/project identifiers used by buildStages) and returns initialized: false instead of allowing later code (e.g., buildStages) to crash when required keys are absent.packages/cli/src/commands/impl/index.ts (2)
78-78: ⚡ Quick winClose the clack frame before
process.exit(1).Line 78 opens an intro frame; line 119 hard-exits with a
p.log.errorbut no closingp.outro/p.cancel. Clack relies on a paired closer to leave the terminal in a clean state — this matches theCancelledErrorpath at line 162 which correctly callsp.cancel('Cancelled.')before exiting.♻️ Suggested fix
} else if (!isTTY) { - p.log.error( + p.cancel( `No plan at \`${PLAN_REL_PATH}\`. Run \`${cli} plan\` first, or pass --continue-without-plan to skip planning.`, ) process.exit(1) } else {Also applies to: 115-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/impl/index.ts` at line 78, The clack intro frame opened by p.intro('CipherStash Implementation') is not closed before hard exiting in the error paths; update the error handling in the blocks that call p.log.error(...) followed by process.exit(1) (the error path around where p.log.error and process.exit(1) are used, and the similar block at the 115–119 region) to first close the frame—call p.outro(...) or p.cancel('Cancelled.') as appropriate—then log the error and call process.exit(1) so the terminal state is restored; ensure you reference and update the handlers that invoke p.log.error, process.exit, and the clack helpers p.outro/p.cancel around those exits.
16-31: 💤 Low value
buildStateFromContextoverstates installation state.
stackInstalled,cliInstalled, andeqlInstalledare set totrueunconditionally on the assumption that a successful priorstash initleft them true. That's a reasonable optimistic default, but nothing on disk is being checked — a user who manually uninstalled@cipherstash/stackor dropped the EQL extension betweeninitandimplwill get a state object that lies about the world.Today only
howToProceedStepconsumes this state on the impl path and it doesn't gate on these flags, so the bug is latent. If you later add an "is everything still installed?" check downstream, this will mask it. At minimum, a comment pinning the assumption helps the next reader; even better, drop the fields you don't actually consume.♻️ Option: only set what the impl flow actually relies on
function buildStateFromContext( ctx: ContextFile, agents: AgentEnvironment, ): InitState { return { integration: ctx.integration, clientFilePath: ctx.encryptionClientPath, schemas: ctx.schemas, envKeys: ctx.envKeys, - stackInstalled: true, - cliInstalled: true, - eqlInstalled: true, agents, mode: 'implement', } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/impl/index.ts` around lines 16 - 31, buildStateFromContext currently sets stackInstalled, cliInstalled, and eqlInstalled to true unconditionally which can misrepresent actual disk state; remove these three fields from the returned InitState (or set them via real checks) and update the InitState type and any callers accordingly (notably howToProceedStep) — if you must keep the fields instead, replace the hardcoded true values with real presence checks (e.g., checkStackInstalled/checkCliInstalled/checkEqlInstalled) or add a clear comment documenting the optimistic assumption.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/init/lib/parse-plan.ts`:
- Around line 49-53: isPlanSummary currently treats an object with an empty
columns array as valid, which causes renderPlanSummary to show "0 columns..."
instead of falling back; update isPlanSummary (the function named isPlanSummary)
to require obj.columns be a non-empty array (e.g., check
Array.isArray(obj.columns) && obj.columns.length > 0 &&
obj.columns.every(isPlanColumn)) so empty plans are rejected and downstream
logic can fall back to the editor/soft prompt.
In `@packages/cli/src/commands/init/types.ts`:
- Around line 67-75: The current InitStep.run signature makes provider optional
which allows callers to omit it and causes runtime crashes in steps that
dereference provider.name (see resolveDatabaseStep, installEqlStep,
authenticateStep); split the step types instead: keep InitStep.run(state:
InitState, provider: InitProvider) with provider required for the init pipeline,
add a new HandoffStep.run(state: InitState) without a provider for post-init
flows (used by howToProceedStep and other plan/impl steps), update the pipeline
that invokes init steps to use InitStep and the handoff flow to use HandoffStep,
and adjust types/usages accordingly so steps that need provider remain
type-safe.
In `@packages/cli/src/commands/status/index.ts`:
- Around line 31-47: readProjectStatus currently only catches JSON.parse errors
so a parsed-but-wrong-shaped context (e.g., missing
schemas/integration/packageManager) sets initialized=true and later makes
buildStages crash on status.context.schemas.length; modify readProjectStatus to
validate the parsed object conforms to the ContextFile shape (ensure schemas is
an array, integration and packageManager exist and are correct types) and if
validation fails treat it like a malformed file (leave context undefined and
initialized false). Update the parsing block in readProjectStatus (the
JSON.parse try) to perform these checks and only assign to context when the
shape matches, and add unit tests for "{}" and for a context missing schemas to
cover the regression.
---
Outside diff comments:
In `@packages/cli/src/bin/stash.ts`:
- Around line 380-422: Add an E2E test suite that spawns the built CLI binary
(with STASH_DISABLE_TELEMETRY=1) to verify argv→dispatch in the top-level
switch: assert `stash status` exits 0 and its stdout references "init"; assert
`stash plan` dispatches planCommand (e.g., detect expected planCommand output or
use an env-gated stub); assert `stash impl --continue-without-plan` treats
`--continue-without-plan` as a flag (not a positional) by observing implCommand
behavior/output when that flag is present; and assert an unknown command exits 1
and prints the help/unknown message; implement these tests by spawning the CLI,
capturing stdout/stderr and exit codes and failing on mismatches.
---
Nitpick comments:
In `@packages/cli/src/commands/impl/index.ts`:
- Line 78: The clack intro frame opened by p.intro('CipherStash Implementation')
is not closed before hard exiting in the error paths; update the error handling
in the blocks that call p.log.error(...) followed by process.exit(1) (the error
path around where p.log.error and process.exit(1) are used, and the similar
block at the 115–119 region) to first close the frame—call p.outro(...) or
p.cancel('Cancelled.') as appropriate—then log the error and call
process.exit(1) so the terminal state is restored; ensure you reference and
update the handlers that invoke p.log.error, process.exit, and the clack helpers
p.outro/p.cancel around those exits.
- Around line 16-31: buildStateFromContext currently sets stackInstalled,
cliInstalled, and eqlInstalled to true unconditionally which can misrepresent
actual disk state; remove these three fields from the returned InitState (or set
them via real checks) and update the InitState type and any callers accordingly
(notably howToProceedStep) — if you must keep the fields instead, replace the
hardcoded true values with real presence checks (e.g.,
checkStackInstalled/checkCliInstalled/checkEqlInstalled) or add a clear comment
documenting the optimistic assumption.
In `@packages/cli/src/commands/init/lib/parse-plan.ts`:
- Around line 90-113: renderPlanSummary currently uses a fixed
COLUMN_LABEL_WIDTH which lets long `${c.table}.${c.column}` labels overflow and
misalign descriptions; change the function to compute a dynamic labelWidth =
Math.max(COLUMN_LABEL_WIDTH, ...summary.columns.map(c =>
`${c.table}.${c.column}`.length)) (ensuring a sensible minimum) and use that
labelWidth in the padEnd call so the description column stays aligned; update
references in renderPlanSummary to use this computed width instead of the
constant.
In `@packages/cli/src/commands/init/lib/read-context.ts`:
- Around line 13-21: The readContextFile function currently returns any parsed
JSON cast as ContextFile; add a runtime shape validation (a type guard like
isContextFile similar to isPlanSummary in parse-plan.ts) after JSON.parse and
before returning to ensure required fields from ContextFile/CONTEXT_REL_PATH
exist; if validation fails, return undefined (so buildStateFromContext / stash
plan/stash impl can route users to stash init) and keep the try/catch around
JSON.parse to still handle malformed JSON.
In `@packages/cli/src/commands/init/lib/setup-prompt.ts`:
- Around line 308-346: Both renderPlanPrompt and renderImplementPrompt duplicate
the same header, "What `stash init` already did" checklist, and "Skills loaded"
block; extract these repeated parts into reusable helpers (e.g., add functions
renderDoneList(ctx) that returns the done string[] used in the sections array,
renderHeader(ctx, intent) that returns the integration/package-manager line and
the intent paragraph, and renderSkillsSection(ctx) that returns the
rulesLocation and renderSkillIndex lines) and have renderPlanPrompt and
renderImplementPrompt call these helpers to assemble their final sections
instead of duplicating the arrays and strings.
- Around line 357-398: The copy-to-docs step introduces a second authoritative
plan that can drift from the canonical `.cipherstash/plan.md`; update the prose
around PLAN_REL_PATH so it explicitly declares `.cipherstash/plan.md` as the
canonical source and that `docs/plans/cipherstash-encryption.md` (if offered) is
only a snapshot for review, not the source of truth, and mention that
`renderImplementPrompt` reads `.cipherstash/plan.md`; alter the sentence
offering to copy so it asks permission and states the copied file is a review
snapshot only (do not create automatic symlinks or bidirectional copy logic).
In `@packages/cli/src/commands/init/types.ts`:
- Around line 61-64: The comment and types disagree: change the optional mode?:
InitMode in the handoff type to a required mode: InitMode (or create a narrower
HandoffInitState that requires mode: InitMode) so the handoff step always has a
concrete InitMode; update any constructors/builders such as
buildStateFromContext (and the impl equivalent) to continue setting 'plan' or
'implement' and adjust any callers that relied on an undefined mode to use the
required field instead.
In `@packages/cli/src/commands/plan/index.ts`:
- Around line 74-89: The prompt and outro incorrectly assume the plan file at
PLAN_REL_PATH was written by the external agent; update the flow in the command
handler so you check for the file's existence and non-emptiness before prompting
or claiming the plan exists: after the external agent returns and before calling
p.confirm or p.outro, stat the file at PLAN_REL_PATH (or read it) and if
missing/empty surface a clear warning and offer a different prompt (e.g., "No
plan found, continue to impl without a plan?") and only call implCommand({}) and
show the "Plan drafted…" message when the file is present; keep references to
PLAN_REL_PATH, p.confirm, p.outro and implCommand so the change is easy to
locate.
- Around line 44-55: Extract the duplicated pre-flight check into a reusable
function by adding requireContext(cwd, cli) in init/lib/read-context.ts that
calls readContextFile(cwd) and, if it returns falsy, calls p.log.error with the
same message using CONTEXT_REL_PATH and the provided cli then process.exit(1);
replace the existing readContextFile + error block in planCommand (and the impl
command) with a single call to requireContext(process.cwd(),
runnerCommand(detectPackageManager(), 'stash')) so both commands share identical
behavior and message.
In `@packages/cli/src/commands/status/__tests__/status.test.ts`:
- Around line 86-95: Add a companion test covering the "wrong-shape" (parses but
missing keys) case in the same test file by creating .cipherstash/context.json
with '{}' and asserting readProjectStatus(cwd).initialized is false; also ensure
the implementation of readProjectStatus validates the parsed shape (e.g.,
presence of expected keys like schemas/project identifiers used by buildStages)
and returns initialized: false instead of allowing later code (e.g.,
buildStages) to crash when required keys are absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b3af236-b5ad-4873-9df4-6f1bdc51c9d2
📒 Files selected for processing (25)
.changeset/stash-impl-command.md.changeset/stash-impl-plan-summary.md.changeset/stash-plan-command.md.changeset/stash-status-command.mdpackages/cli/src/bin/stash.tspackages/cli/src/commands/impl/__tests__/how-to-proceed.test.tspackages/cli/src/commands/impl/index.tspackages/cli/src/commands/impl/steps/handoff-agents-md.tspackages/cli/src/commands/impl/steps/handoff-claude.tspackages/cli/src/commands/impl/steps/handoff-codex.tspackages/cli/src/commands/impl/steps/handoff-wizard.tspackages/cli/src/commands/impl/steps/how-to-proceed.tspackages/cli/src/commands/index.tspackages/cli/src/commands/init/index.tspackages/cli/src/commands/init/lib/__tests__/parse-plan.test.tspackages/cli/src/commands/init/lib/__tests__/read-context.test.tspackages/cli/src/commands/init/lib/__tests__/setup-prompt.test.tspackages/cli/src/commands/init/lib/parse-plan.tspackages/cli/src/commands/init/lib/read-context.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/cli/src/commands/init/steps/next-steps.tspackages/cli/src/commands/init/types.tspackages/cli/src/commands/plan/index.tspackages/cli/src/commands/status/__tests__/status.test.tspackages/cli/src/commands/status/index.ts
💤 Files with no reviewable changes (1)
- packages/cli/src/commands/init/steps/next-steps.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/stash-impl-plan-summary.md
auxesis
approved these changes
May 6, 2026
Contributor
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
Nice one, and good call @calvinbrewer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.cipherstash/plan.mdas the source of truth for routing if it exists, rather than re-asking which path applies.Closes #408
What changes
chooseModeStepruns aftergather-context(default:plan).howToProceedStepfilters its option list and default by mode.setup-prompt.tssplits intorenderPlanPrompt(writes.cipherstash/plan.md, forbids mutations, allows read-only inspection, offers to copy intodocs/plans/if it exists) andrenderImplementPrompt(existing orient-and-route prompt, plus a "read the plan first if it exists" preamble).next-stepsprints a plan-mode-specific closing message.Test plan
pnpm --filter stash test— 189/189 pass (15 new tests across plan-mode rendering and the mode-aware option/default helpers).pnpm --filter stash build— clean.pnpm --filter stash lint— clean for changed files.stash initin a fresh project, accept plan-first default, pick Claude → verify only Claude/Codex offered, plan handoff prompt produced, no schema edits.stash initagain, pick "Go straight to implementation" → verify all four targets offered, implement prompt detects.cipherstash/plan.mdand skips re-routing.stash initwith no plan file present → verify implement prompt falls through to the orient-and-route flow..cipherstash/plan.mdis offered for copy intodocs/plans/when that directory exists.Summary by CodeRabbit
Release Notes
stash plancommand for drafting project plans with machine-readable summaries.stash implcommand for executing plans or continuing without one via--continue-without-planflag.stash statuscommand showing initialization, plan, and implementation progress.stash initnow scaffolds only, then prompts to runstash plan..cipherstash/plan.mdare now the source of truth for implementation.