Merge pull request #704 from cipherstash/fix/impl-tty-detection

fix(cli): route stash impl and the two stash init prompts through the shared TTY gate

Author: tobyhede

.changeset/hungry-spoons-shake.md

@@ -0,0 +1,28 @@
+---
+'stash': patch
+---
+
+Fix `stash impl` and `stash init` hanging on CI runners that allocate a TTY.
+
+Four prompts decided whether to run interactively without going through the
+shared TTY helper, so on a CI runner with an allocated TTY they rendered a clack
+prompt and blocked forever on `/dev/tty` — a silent hang with no error and no
+timeout:
+
+- `stash impl` gated on an inline `process.env.CI !== 'true'`, which only
+  recognised the exact lowercase spelling. Runners that set `CI=1` or `CI=TRUE`
+  blocked on the plan-summary confirmation or the agent-target picker.
+- `stash init`'s offer to chain into `stash plan`, and its Proxy-vs-SDK
+  question, gated on `process.stdout.isTTY` and did not consult `CI` at all —
+  so they hung on any CI runner with a TTY, whatever the spelling. Gating on
+  stdout was also the wrong stream: a redirected stdin still hangs a prompt.
+- `stash impl --continue-without-plan` confirmed the flag with a second prompt
+  that was not gated at all, so a CI run with no plan on disk blocked there even
+  though the flag had already granted consent. The flag is now taken as consent
+  in non-interactive runs and only re-confirmed interactively.
+
+All four now use the shared `isInteractive()` helper (stdin is a TTY and `CI`
+is not set to `1`/`true` in any case), matching `stash plan`. Non-interactive
+runs take the path they always should have: `stash init` skips the chain offer
+and prints the `plan --target` hint, the Proxy-vs-SDK question defaults to
+SDK-only, and `stash impl` proceeds without prompting.

packages/cli/src/commands/impl/__tests__/impl.test.ts

@@ -2,9 +2,20 @@ import fs from 'node:fs'
 import os from 'node:os'
 import path from 'node:path'
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { CliExit } from '../../../cli/exit.js'
 import { implCommand } from '../index.js'
 import { howToProceedStep } from '../steps/how-to-proceed.js'
 
+// `implCommand` reaches the plan-summary checkpoint via `p.confirm`, which
+// reads /dev/tty. Stub just that export (the rest of @clack/prompts stays
+// real) so the CI-detection cases can assert whether the prompt was reached.
+const confirmMock = vi.hoisted(() => vi.fn())
+vi.mock('@clack/prompts', async () => {
+  const actual =
+    await vi.importActual<typeof import('@clack/prompts')>('@clack/prompts')
+  return { ...actual, confirm: confirmMock }
+})
+
 let originalIsTTY: boolean | undefined
 let originalCwd: string
 let tmpDir: string
@@ -99,3 +110,138 @@ describe('implCommand — TTY handling', () => {
     expect(runSpy).not.toHaveBeenCalled()
   })
 })
+
+describe('implCommand — CI detection with a TTY attached', () => {
+  // Regression: the gate used to be an inline `process.env.CI !== 'true'`,
+  // which only recognised the exact lowercase spelling. A CI runner that sets
+  // CI=1 or CI=TRUE *and* allocates a TTY made `stash impl` believe it was
+  // interactive, so it blocked on the plan-summary confirm (or the
+  // agent-target picker) forever — a hang, not an error. The gate now goes
+  // through `isInteractive()` in config/tty.ts, whose `isCiEnv()` accepts
+  // 1/true in any case.
+  beforeEach(() => {
+    setIsTTY(true)
+    confirmMock.mockReset()
+    confirmMock.mockResolvedValue(true)
+  })
+
+  afterEach(() => {
+    vi.unstubAllEnvs()
+  })
+
+  for (const ciValue of ['1', 'TRUE', 'true']) {
+    it(`treats CI=${ciValue} as non-interactive even with a TTY`, async () => {
+      vi.stubEnv('CI', ciValue)
+      const runSpy = vi
+        .spyOn(howToProceedStep, 'run')
+        .mockResolvedValue({} as never)
+
+      await expect(implCommand({}, {})).resolves.toBeUndefined()
+
+      // Neither blocking prompt may be reached.
+      expect(confirmMock).not.toHaveBeenCalled()
+      expect(runSpy).not.toHaveBeenCalled()
+    })
+  }
+
+  it('does not re-confirm --continue-without-plan under CI when no plan exists', async () => {
+    // The flag is the consent. Prompting again under CI blocks on /dev/tty,
+    // and the confirm is default-no, so a resolved prompt would cancel a run
+    // the user explicitly asked for.
+    vi.stubEnv('CI', '1')
+    fs.rmSync(path.join(tmpDir, '.cipherstash', 'plan.md'))
+    const runSpy = vi
+      .spyOn(howToProceedStep, 'run')
+      .mockResolvedValue({} as never)
+
+    await expect(
+      implCommand({ 'continue-without-plan': true }, {}),
+    ).resolves.toBeUndefined()
+
+    expect(confirmMock).not.toHaveBeenCalled()
+    expect(runSpy).not.toHaveBeenCalled()
+  })
+
+  it('is interactive when CI is unset and stdin is a TTY', async () => {
+    vi.stubEnv('CI', '')
+    const runSpy = vi
+      .spyOn(howToProceedStep, 'run')
+      .mockResolvedValue({} as never)
+
+    await implCommand({}, {})
+
+    expect(confirmMock).toHaveBeenCalledTimes(1)
+    expect(runSpy).toHaveBeenCalledTimes(1)
+  })
+
+  it('performs the handoff under CI=1 when --target is given', async () => {
+    // Non-interactive means "don't prompt", not "don't run". A regression
+    // that made the command bail early under CI would keep the not-called
+    // cases above green — this locks the automation happy path.
+    vi.stubEnv('CI', '1')
+    const runSpy = vi
+      .spyOn(howToProceedStep, 'run')
+      .mockResolvedValue({} as never)
+
+    await implCommand({}, { target: 'agents-md' })
+
+    expect(confirmMock).not.toHaveBeenCalled()
+    expect(runSpy).toHaveBeenCalledTimes(1)
+    expect(runSpy.mock.calls[0][0].handoff).toBe('agents-md')
+  })
+
+  it('runs the selected handoff under CI with --continue-without-plan and no plan', async () => {
+    // The specific path the `if (isTTY)` gate at index.ts opened up: flag +
+    // target, no plan on disk — it must run the handoff without re-confirming.
+    vi.stubEnv('CI', '1')
+    fs.rmSync(path.join(tmpDir, '.cipherstash', 'plan.md'))
+    const runSpy = vi
+      .spyOn(howToProceedStep, 'run')
+      .mockResolvedValue({} as never)
+
+    await implCommand(
+      { 'continue-without-plan': true },
+      { target: 'agents-md' },
+    )
+
+    expect(confirmMock).not.toHaveBeenCalled()
+    expect(runSpy).toHaveBeenCalledTimes(1)
+    expect(runSpy.mock.calls[0][0].handoff).toBe('agents-md')
+  })
+
+  it('exits 1 with the plan hint under CI=1 when no plan and no --continue-without-plan', async () => {
+    // CI=1 flips isTTY to false, rerouting a no-plan / no-flag run out of the
+    // interactive `select` and into the `else if (!isTTY)` error + exit(1). It
+    // must fail loudly, not render the "No plan found" select and block.
+    vi.stubEnv('CI', '1')
+    fs.rmSync(path.join(tmpDir, '.cipherstash', 'plan.md'))
+    const exitSpy = vi.spyOn(process, 'exit').mockImplementation((() => {
+      throw new Error('process.exit')
+    }) as never)
+
+    await expect(implCommand({}, {})).rejects.toThrow('process.exit')
+
+    expect(exitSpy).toHaveBeenCalledWith(1)
+    expect(confirmMock).not.toHaveBeenCalled()
+  })
+
+  it('still confirms --continue-without-plan interactively, and declining aborts', async () => {
+    // The honour-the-prompt half of the `if (isTTY)` gate: with a TTY and CI
+    // unset the default-no confirm must still fire, and declining it must abort
+    // (CancelledError → CliExit) rather than fall through to the handoff.
+    vi.stubEnv('CI', '')
+    fs.rmSync(path.join(tmpDir, '.cipherstash', 'plan.md'))
+    confirmMock.mockReset()
+    confirmMock.mockResolvedValue(false)
+    const runSpy = vi
+      .spyOn(howToProceedStep, 'run')
+      .mockResolvedValue({} as never)
+
+    await expect(
+      implCommand({ 'continue-without-plan': true }, {}),
+    ).rejects.toBeInstanceOf(CliExit)
+
+    expect(confirmMock).toHaveBeenCalledTimes(1)
+    expect(runSpy).not.toHaveBeenCalled()
+  })
+})

packages/cli/src/commands/impl/index.ts

@@ -2,6 +2,7 @@ import { existsSync, readFileSync } from 'node:fs'
 import { resolve } from 'node:path'
 import * as p from '@clack/prompts'
 import { CliExit } from '../../cli/exit.js'
+import { isInteractive as isInteractiveTty } from '../../config/tty.js'
 import { type AgentEnvironment, detectAgents } from '../init/detect-agents.js'
 import {
   effectiveStep,
@@ -166,11 +167,13 @@ export async function implCommand(
   const planPath = resolve(cwd, PLAN_REL_PATH)
   const planExists = existsSync(planPath)
   const continueWithoutPlan = flags['continue-without-plan'] === true
-  // Interactive only when stdin is a real TTY and we're not in CI — the
-  // same gate the encrypt commands use. `process.stdout.isTTY` alone is
-  // wrong: a redirected stdin still hangs the agent-target picker (clack
-  // `select` reads from /dev/tty).
-  const isTTY = Boolean(process.stdin.isTTY) && process.env.CI !== 'true'
+  // Interactive only when stdin is a real TTY and we're not in CI — via the
+  // shared `isInteractive()` (config/tty.ts) so this gate stays identical to
+  // every other prompt gate (its `isCiEnv()` treats `CI=1`/`CI=TRUE` as CI too,
+  // which a bare `CI !== 'true'` inline would miss). `process.stdout.isTTY`
+  // alone is wrong: a redirected stdin still hangs the agent-target picker
+  // (clack `select` reads from /dev/tty).
+  const isTTY = isInteractiveTty()
 
   let planStep: PlanStep | undefined
 
@@ -227,7 +230,13 @@ export async function implCommand(
     } else {
       // No plan on disk. Branch on flag / TTY / interactive.
       if (continueWithoutPlan) {
-        await confirmContinueWithoutPlan()
+        // The flag *is* the consent. Re-prompting for it non-interactively
+        // is the same hang this gate exists to close (clack `confirm` reads
+        // /dev/tty), and `confirmContinueWithoutPlan` is default-no, so a
+        // prompt that did resolve would cancel a run the user asked for.
+        if (isTTY) {
+          await confirmContinueWithoutPlan()
+        }
       } else if (!isTTY) {
         p.log.error(
           `No plan at \`${PLAN_REL_PATH}\`. Run \`${cli} plan\` first, or pass --continue-without-plan to skip planning.`,

packages/cli/src/commands/init/__tests__/init-command.test.ts

@@ -1,5 +1,5 @@
 import * as p from '@clack/prompts'
-import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 import { CliExit } from '../../../cli/exit.js'
 import { messages } from '../../../messages.js'
 import type { InitState } from '../types.js'
@@ -174,3 +174,106 @@ describe('initCommand — honest summary', () => {
     )
   })
 })
+
+describe('initCommand — CI detection on the `stash plan` chain offer', () => {
+  // Regression: this gate was `process.stdout.isTTY`, which consulted CI not
+  // at all. A CI runner with an allocated TTY reached the confirm and blocked
+  // on /dev/tty forever — a hang, not an error. It also asked about the wrong
+  // stream: a redirected stdin still hangs the prompt. Now `isInteractive()`
+  // (stdin + isCiEnv, which accepts 1/true in any case).
+  const originalStdinIsTTY = process.stdin.isTTY
+  const originalStdoutIsTTY = process.stdout.isTTY
+
+  // Force BOTH streams. A CI runner that allocates a TTY has stdin *and*
+  // stdout as TTYs — that is the configuration this gate used to hang in, and
+  // setting stdin alone would let the old `process.stdout.isTTY` gate pass
+  // these tests for the wrong reason (vitest's own stdout is not a TTY).
+  beforeEach(() => {
+    Object.defineProperty(process.stdin, 'isTTY', {
+      value: true,
+      configurable: true,
+    })
+    Object.defineProperty(process.stdout, 'isTTY', {
+      value: true,
+      configurable: true,
+    })
+  })
+
+  afterEach(() => {
+    Object.defineProperty(process.stdin, 'isTTY', {
+      value: originalStdinIsTTY,
+      configurable: true,
+    })
+    Object.defineProperty(process.stdout, 'isTTY', {
+      value: originalStdoutIsTTY,
+      configurable: true,
+    })
+    vi.unstubAllEnvs()
+  })
+
+  for (const ciValue of ['1', 'TRUE', 'true']) {
+    it(`skips the chain offer under CI=${ciValue} even with a TTY`, async () => {
+      vi.stubEnv('CI', ciValue)
+
+      await expect(initCommand({}, {})).resolves.toBeUndefined()
+
+      // Non-interactive must skip the offer, not fail: init still completes,
+      // and steers the user at `plan --target` instead of blocking.
+      expect(vi.mocked(p.confirm)).not.toHaveBeenCalled()
+      expect(vi.mocked(p.outro)).toHaveBeenCalledWith(
+        expect.stringContaining('--target'),
+      )
+    })
+  }
+
+  it('offers the chain when CI is unset and stdin is a TTY', async () => {
+    vi.stubEnv('CI', '')
+    vi.mocked(p.confirm).mockResolvedValueOnce(false)
+
+    await expect(initCommand({}, {})).resolves.toBeUndefined()
+
+    expect(vi.mocked(p.confirm)).toHaveBeenCalledTimes(1)
+  })
+
+  it('skips the chain offer when stdin is redirected even if stdout is a TTY', async () => {
+    // The gate keys off stdin, not stdout: `stash init < /dev/null` from a
+    // terminal (stdin piped, stdout still a TTY) must not reach the confirm.
+    // Reinstating `process.stdout.isTTY` would keep the CI loop above green,
+    // so only this stdin/stdout split pins the correct stream.
+    vi.stubEnv('CI', '')
+    Object.defineProperty(process.stdin, 'isTTY', {
+      value: false,
+      configurable: true,
+    })
+    Object.defineProperty(process.stdout, 'isTTY', {
+      value: true,
+      configurable: true,
+    })
+
+    await expect(initCommand({}, {})).resolves.toBeUndefined()
+
+    expect(vi.mocked(p.confirm)).not.toHaveBeenCalled()
+    expect(vi.mocked(p.outro)).toHaveBeenCalledWith(
+      expect.stringContaining('--target'),
+    )
+  })
+
+  it('chains into `stash plan` when the offer is accepted', async () => {
+    // The accept arm (outro + planCommand() + early return) is what the gate
+    // change made reachable; the only other confirm test resolves `false`.
+    vi.stubEnv('CI', '')
+    vi.mocked(p.confirm).mockResolvedValueOnce(true)
+    const { planCommand } = await import('../../plan/index.js')
+
+    await expect(initCommand({}, {})).resolves.toBeUndefined()
+
+    expect(vi.mocked(planCommand)).toHaveBeenCalledTimes(1)
+    expect(vi.mocked(p.outro)).toHaveBeenCalledWith(
+      expect.stringContaining('handing off to `stash plan`'),
+    )
+    // The accept path returns early — it must not also print the --target hint.
+    expect(vi.mocked(p.outro)).not.toHaveBeenCalledWith(
+      expect.stringContaining('--target'),
+    )
+  })
+})

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

@@ -1,5 +1,6 @@
 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 { planCommand } from '../plan/index.js'
@@ -165,7 +166,13 @@ export async function initCommand(
     // multi-command flow. Drafting a plan is fast (~1–3 min of agent
     // thinking) and produces a reviewable artifact — `stash impl` is the
     // separate, slower verb that actually mutates code.
-    if (process.stdout.isTTY) {
+    //
+    // Gated on the shared `isInteractive()` (config/tty.ts), the same helper
+    // every other prompt uses. `process.stdout.isTTY` was wrong on both
+    // counts: it ignored CI entirely (a runner with an allocated TTY blocked
+    // here forever, since clack `confirm` reads /dev/tty), and it asked about
+    // the wrong stream — a redirected stdin still hangs the prompt.
+    if (isInteractive()) {
       const proceed = await p.confirm({
         message: `Continue to \`${cli} plan\` now to draft your encryption plan?`,
         initialValue: true,