chore: address coderabbit feedback

Author: calvinbrewer

packages/cli/src/commands/impl/steps/handoff-agents-md.ts

@@ -8,7 +8,7 @@ import {
   CONTEXT_REL_PATH,
   SETUP_PROMPT_REL_PATH,
 } from '../../init/lib/write-context.js'
-import type { InitProvider, InitState, InitStep } from '../../init/types.js'
+import type { HandoffStep, InitState } from '../../init/types.js'
 
 const AGENTS_MD_REL_PATH = 'AGENTS.md'
 
@@ -25,10 +25,10 @@ const AGENTS_MD_REL_PATH = 'AGENTS.md'
  * tools wouldn't know to look there. Re-runs replace only the sentinel
  * region in AGENTS.md.
  */
-export const handoffAgentsMdStep: InitStep = {
+export const handoffAgentsMdStep: HandoffStep = {
   id: 'handoff-agents-md',
   name: 'Write AGENTS.md',
-  async run(state: InitState, _provider?: InitProvider): Promise<InitState> {
+  async run(state: InitState): Promise<InitState> {
     const cwd = process.cwd()
     const integration = state.integration ?? 'postgresql'
 

packages/cli/src/commands/impl/steps/handoff-claude.ts

@@ -5,7 +5,7 @@ import {
   CONTEXT_REL_PATH,
   SETUP_PROMPT_REL_PATH,
 } from '../../init/lib/write-context.js'
-import type { InitProvider, InitState, InitStep } from '../../init/types.js'
+import type { HandoffStep, InitState } from '../../init/types.js'
 
 const CLAUDE_SKILLS_DIR = '.claude/skills'
 
@@ -18,10 +18,10 @@ const CLAUDE_INSTALL_URL = 'https://code.claude.com/docs/en/quickstart'
  * on PATH we still write the artifacts and print install + manual-launch
  * instructions.
  */
-export const handoffClaudeStep: InitStep = {
+export const handoffClaudeStep: HandoffStep = {
   id: 'handoff-claude',
   name: 'Hand off to Claude Code',
-  async run(state: InitState, _provider?: InitProvider): Promise<InitState> {
+  async run(state: InitState): Promise<InitState> {
     const cwd = process.cwd()
     const integration = state.integration ?? 'postgresql'
 

packages/cli/src/commands/impl/steps/handoff-codex.ts

@@ -9,7 +9,7 @@ import {
   CONTEXT_REL_PATH,
   SETUP_PROMPT_REL_PATH,
 } from '../../init/lib/write-context.js'
-import type { InitProvider, InitState, InitStep } from '../../init/types.js'
+import type { HandoffStep, InitState } from '../../init/types.js'
 
 const AGENTS_MD_REL_PATH = 'AGENTS.md'
 const CODEX_SKILLS_DIR = '.codex/skills'
@@ -25,10 +25,10 @@ const CODEX_INSTALL_URL = 'https://github.com/openai/codex'
  * AGENTS.md is sentinel-upserted so re-runs replace only our region and
  * any user content outside it survives.
  */
-export const handoffCodexStep: InitStep = {
+export const handoffCodexStep: HandoffStep = {
   id: 'handoff-codex',
   name: 'Hand off to Codex',
-  async run(state: InitState, _provider?: InitProvider): Promise<InitState> {
+  async run(state: InitState): Promise<InitState> {
     const cwd = process.cwd()
     const integration = state.integration ?? 'postgresql'
 

packages/cli/src/commands/impl/steps/handoff-wizard.ts

@@ -5,7 +5,7 @@ import {
   buildContextFile,
   writeContextFile,
 } from '../../init/lib/write-context.js'
-import type { InitProvider, InitState, InitStep } from '../../init/types.js'
+import type { HandoffStep, InitState } from '../../init/types.js'
 import { runWizardSpawn } from '../../wizard/index.js'
 
 /**
@@ -20,10 +20,10 @@ import { runWizardSpawn } from '../../wizard/index.js'
  * No skills are installed here. The wizard fetches its own agent-side
  * prompt from the gateway and runs its own `maybeInstallSkills` flow.
  */
-export const handoffWizardStep: InitStep = {
+export const handoffWizardStep: HandoffStep = {
   id: 'handoff-wizard',
   name: 'Use the CipherStash Agent',
-  async run(state: InitState, _provider?: InitProvider): Promise<InitState> {
+  async run(state: InitState): Promise<InitState> {
     const cwd = process.cwd()
     const envKeys = state.envKeys ?? []
 

packages/cli/src/commands/impl/steps/how-to-proceed.ts

@@ -2,10 +2,9 @@ import * as p from '@clack/prompts'
 import {
   CancelledError,
   type HandoffChoice,
+  type HandoffStep,
   type InitMode,
-  type InitProvider,
   type InitState,
-  type InitStep,
 } from '../../init/types.js'
 import { handoffAgentsMdStep } from './handoff-agents-md.js'
 import { handoffClaudeStep } from './handoff-claude.js'
@@ -79,10 +78,10 @@ export function buildOptions(
   return options
 }
 
-export const howToProceedStep: InitStep = {
+export const howToProceedStep: HandoffStep = {
   id: 'how-to-proceed',
   name: 'How to proceed',
-  async run(state: InitState, _provider?: InitProvider): Promise<InitState> {
+  async run(state: InitState): Promise<InitState> {
     const mode: InitMode = state.mode ?? 'implement'
     const message =
       mode === 'plan'

packages/cli/src/commands/init/lib/__tests__/parse-plan.test.ts

@@ -61,6 +61,17 @@ describe('parsePlanSummary', () => {
     expect(parsePlanSummary(empty)).toBeUndefined()
   })
 
+  it('rejects an empty columns array — falls back to soft summary', () => {
+    // An agent that writes `{"columns": []}` (genuinely empty plan,
+    // truncated write, or chat cutoff) would otherwise render as
+    // "0 columns across 0 tables — single-deploy", which is misleading.
+    // `stash impl` falls back to the "open in your editor" panel instead.
+    const md = `<!-- cipherstash:plan-summary
+{"columns": []}
+-->`
+    expect(parsePlanSummary(md)).toBeUndefined()
+  })
+
   it('tolerates extra unknown fields without dropping the parse', () => {
     // Future-proofing — agents may include estimated-deploys or other
     // ancillary keys. The parser should ignore them, not fail.

packages/cli/src/commands/init/lib/__tests__/read-context.test.ts

@@ -54,4 +54,40 @@ describe('readContextFile', () => {
     )
     expect(readContextFile(cwd)).toBeUndefined()
   })
+
+  it('returns undefined for an empty object — wrong shape, not "initialized"', () => {
+    // A `{}` file parses fine but doesn't carry `schemas`/`integration`/
+    // `packageManager`. Downstream code (status, plan summary, etc.)
+    // dereferences those without guarding, so accepting `{}` would mean
+    // a hand-edited or partial-write file crashes the CLI.
+    writeContext({})
+    expect(readContextFile(cwd)).toBeUndefined()
+  })
+
+  it('returns undefined when schemas is missing', () => {
+    writeContext({
+      integration: 'drizzle',
+      packageManager: 'pnpm',
+      // schemas absent
+    })
+    expect(readContextFile(cwd)).toBeUndefined()
+  })
+
+  it('returns undefined when integration is the wrong type', () => {
+    writeContext({
+      integration: 42,
+      packageManager: 'pnpm',
+      schemas: [],
+    })
+    expect(readContextFile(cwd)).toBeUndefined()
+  })
+
+  it('returns undefined when schemas is not an array', () => {
+    writeContext({
+      integration: 'drizzle',
+      packageManager: 'pnpm',
+      schemas: 'oops',
+    })
+    expect(readContextFile(cwd)).toBeUndefined()
+  })
 })

packages/cli/src/commands/init/lib/parse-plan.ts

@@ -49,7 +49,15 @@ function isPlanColumn(x: unknown): x is PlanColumn {
 function isPlanSummary(x: unknown): x is PlanSummary {
   if (!x || typeof x !== 'object') return false
   const obj = x as Record<string, unknown>
-  return Array.isArray(obj.columns) && obj.columns.every(isPlanColumn)
+  // Empty `columns` is rejected: downstream `renderPlanSummary` would
+  // produce "0 columns across 0 tables — single-deploy", which is
+  // misleading. Treating empty as invalid lets `stash impl` fall back
+  // to the soft "open it in your editor" panel.
+  return (
+    Array.isArray(obj.columns) &&
+    obj.columns.length > 0 &&
+    obj.columns.every(isPlanColumn)
+  )
 }
 
 /**

packages/cli/src/commands/init/lib/read-context.ts

@@ -2,19 +2,42 @@ import { existsSync, readFileSync } from 'node:fs'
 import { resolve } from 'node:path'
 import { CONTEXT_REL_PATH, type ContextFile } from './write-context.js'
 
+/**
+ * Validate that a parsed JSON value has the minimum shape callers rely
+ * on. We only check the fields downstream code dereferences without
+ * a guard — `integration`, `packageManager`, and `schemas`. Other
+ * fields (cliVersion, generatedAt, etc.) are informational and absent
+ * values won't crash anything.
+ *
+ * A wider schema check would belong in a runtime validator (zod, etc.);
+ * this is the minimum to keep `stash status`, `stash plan`, and `stash
+ * impl` from hard-failing on a hand-edited or partial-write file.
+ */
+function isContextFile(x: unknown): x is ContextFile {
+  if (!x || typeof x !== 'object') return false
+  const obj = x as Record<string, unknown>
+  return (
+    typeof obj.integration === 'string' &&
+    typeof obj.packageManager === 'string' &&
+    Array.isArray(obj.schemas)
+  )
+}
+
 /**
  * Read the `.cipherstash/context.json` file written by `stash init`.
- * Returns `undefined` when the file is missing or malformed — both `stash
- * plan` and `stash impl` use that signal to point the user back at
- * `stash init` rather than crashing.
+ * Returns `undefined` when the file is missing, unparseable, or doesn't
+ * have the expected shape — both `stash plan` and `stash impl` use that
+ * signal to point the user back at `stash init` rather than crashing.
  *
- * Never throws on bad input. Malformed JSON is treated as "no context."
+ * Never throws on bad input. Malformed JSON and wrong-shape objects are
+ * both treated as "no context."
  */
 export function readContextFile(cwd: string): ContextFile | undefined {
   const path = resolve(cwd, CONTEXT_REL_PATH)
   if (!existsSync(path)) return undefined
   try {
-    return JSON.parse(readFileSync(path, 'utf-8')) as ContextFile
+    const parsed = JSON.parse(readFileSync(path, 'utf-8')) as unknown
+    return isContextFile(parsed) ? parsed : undefined
   } catch {
     return undefined
   }

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

@@ -64,14 +64,32 @@ export interface InitState {
   mode?: InitMode
 }
 
+/**
+ * A step that runs as part of the `stash init` pipeline. The init
+ * pipeline owns the `InitProvider` (intro copy, provider-specific
+ * defaults) and threads it into every step. Some init steps consult it
+ * (e.g. `authenticateStep` reads `provider.name` for telemetry) so the
+ * argument is required at the type level — calling
+ * `authenticateStep.run(state)` without a provider would crash.
+ */
 export interface InitStep {
   id: string
   name: string
-  /** `provider` is optional. The init pipeline passes one (it owns
-   *  intro copy and provider-specific defaults); the post-init handoff
-   *  steps invoked by `stash plan` / `stash impl` don't have a provider
-   *  to give and don't use one. */
-  run(state: InitState, provider?: InitProvider): Promise<InitState>
+  run(state: InitState, provider: InitProvider): Promise<InitState>
+}
+
+/**
+ * A step that runs after init has finished — invoked by `stash plan` and
+ * `stash impl` to drive the agent handoff. These steps don't have an
+ * `InitProvider` available (init owns that abstraction) and don't need
+ * one, so the type intentionally omits it. Keeping `InitStep` and
+ * `HandoffStep` distinct prevents callers from accidentally invoking
+ * init-only steps from the post-init flow.
+ */
+export interface HandoffStep {
+  id: string
+  name: string
+  run(state: InitState): Promise<InitState>
 }
 
 export interface InitProvider {