Skip to content

Commit bb0a90f

Browse files
committed
fix(cli): route the two stash init prompts through the shared TTY gate
Both `stash init` prompts gated on `process.stdout.isTTY` and consulted CI not at all, so a runner with an allocated TTY rendered the prompt and blocked on /dev/tty forever — strictly more exposed than the `stash impl` bug, since neither even caught `CI=true`: - init/index.ts: the confirm offering to chain into `stash plan` - steps/resolve-proxy-choice.ts: the Proxy-vs-SDK select Gating on stdout was also the wrong stream: a redirected stdin still hangs a prompt. Both now use `isInteractive()` from config/tty.ts. Both files already had correct non-interactive branches, so this is a condition swap only — no reshaping of the init flow. Non-interactive init skips the chain offer and prints the `plan --target` hint; the proxy question defaults to SDK-only, which is what the interactive prompt defaults to and what cancelling it already produced. The tests force isTTY on BOTH streams: a CI runner that allocates a TTY has stdin and stdout both TTYs, and stubbing stdin alone would let the old stdout gate pass for the wrong reason, since vitest's own stdout is not a TTY. Verified by reverting both gates — all six CI cases fail.
1 parent 0e2ce93 commit bb0a90f

5 files changed

Lines changed: 217 additions & 16 deletions

File tree

.changeset/hungry-spoons-shake.md

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,23 @@
22
'stash': patch
33
---
44

5-
Fix `stash impl` hanging on CI runners that set `CI=1` or `CI=TRUE`.
5+
Fix `stash impl` and `stash init` hanging on CI runners that allocate a TTY.
66

7-
`stash impl` decided whether to prompt with an inline `process.env.CI !== 'true'`
8-
check, which only recognised the exact lowercase spelling. On a CI runner that
9-
sets `CI=1` or `CI=TRUE` and allocates a TTY, the command believed it was
10-
interactive and blocked forever on the plan-summary confirmation or the
11-
agent-target picker — a silent hang with no error and no timeout.
7+
Three prompts decided whether to run interactively without going through the
8+
shared TTY helper, so on a CI runner with an allocated TTY they rendered a clack
9+
prompt and blocked forever on `/dev/tty` — a silent hang with no error and no
10+
timeout:
1211

13-
The gate now uses the shared `isInteractive()` helper, the same one `stash plan`
14-
already used, which treats `1`/`true` in any case as CI. `stash impl` now takes
15-
the non-interactive path on those runners, matching `stash plan`.
12+
- `stash impl` gated on an inline `process.env.CI !== 'true'`, which only
13+
recognised the exact lowercase spelling. Runners that set `CI=1` or `CI=TRUE`
14+
blocked on the plan-summary confirmation or the agent-target picker.
15+
- `stash init`'s offer to chain into `stash plan`, and its Proxy-vs-SDK
16+
question, gated on `process.stdout.isTTY` and did not consult `CI` at all —
17+
so they hung on any CI runner with a TTY, whatever the spelling. Gating on
18+
stdout was also the wrong stream: a redirected stdin still hangs a prompt.
19+
20+
All three now use the shared `isInteractive()` helper (stdin is a TTY and `CI`
21+
is not set to `1`/`true` in any case), matching `stash plan`. Non-interactive
22+
runs take the path they always should have: `stash init` skips the chain offer
23+
and prints the `plan --target` hint, the Proxy-vs-SDK question defaults to
24+
SDK-only, and `stash impl` proceeds without prompting.

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

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as p from '@clack/prompts'
2-
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
33
import { CliExit } from '../../../cli/exit.js'
44
import { messages } from '../../../messages.js'
55
import type { InitState } from '../types.js'
@@ -174,3 +174,64 @@ describe('initCommand — honest summary', () => {
174174
)
175175
})
176176
})
177+
178+
describe('initCommand — CI detection on the `stash plan` chain offer', () => {
179+
// Regression: this gate was `process.stdout.isTTY`, which consulted CI not
180+
// at all. A CI runner with an allocated TTY reached the confirm and blocked
181+
// on /dev/tty forever — a hang, not an error. It also asked about the wrong
182+
// stream: a redirected stdin still hangs the prompt. Now `isInteractive()`
183+
// (stdin + isCiEnv, which accepts 1/true in any case).
184+
const originalStdinIsTTY = process.stdin.isTTY
185+
const originalStdoutIsTTY = process.stdout.isTTY
186+
187+
// Force BOTH streams. A CI runner that allocates a TTY has stdin *and*
188+
// stdout as TTYs — that is the configuration this gate used to hang in, and
189+
// setting stdin alone would let the old `process.stdout.isTTY` gate pass
190+
// these tests for the wrong reason (vitest's own stdout is not a TTY).
191+
beforeEach(() => {
192+
Object.defineProperty(process.stdin, 'isTTY', {
193+
value: true,
194+
configurable: true,
195+
})
196+
Object.defineProperty(process.stdout, 'isTTY', {
197+
value: true,
198+
configurable: true,
199+
})
200+
})
201+
202+
afterEach(() => {
203+
Object.defineProperty(process.stdin, 'isTTY', {
204+
value: originalStdinIsTTY,
205+
configurable: true,
206+
})
207+
Object.defineProperty(process.stdout, 'isTTY', {
208+
value: originalStdoutIsTTY,
209+
configurable: true,
210+
})
211+
vi.unstubAllEnvs()
212+
})
213+
214+
for (const ciValue of ['1', 'TRUE', 'true']) {
215+
it(`skips the chain offer under CI=${ciValue} even with a TTY`, async () => {
216+
vi.stubEnv('CI', ciValue)
217+
218+
await expect(initCommand({}, {})).resolves.toBeUndefined()
219+
220+
// Non-interactive must skip the offer, not fail: init still completes,
221+
// and steers the user at `plan --target` instead of blocking.
222+
expect(vi.mocked(p.confirm)).not.toHaveBeenCalled()
223+
expect(vi.mocked(p.outro)).toHaveBeenCalledWith(
224+
expect.stringContaining('--target'),
225+
)
226+
})
227+
}
228+
229+
it('offers the chain when CI is unset and stdin is a TTY', async () => {
230+
vi.stubEnv('CI', '')
231+
vi.mocked(p.confirm).mockResolvedValueOnce(false)
232+
233+
await expect(initCommand({}, {})).resolves.toBeUndefined()
234+
235+
expect(vi.mocked(p.confirm)).toHaveBeenCalledTimes(1)
236+
})
237+
})

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import * as p from '@clack/prompts'
22
import { CliExit } from '../../cli/exit.js'
3+
import { isInteractive } from '../../config/tty.js'
34
import { messages } from '../../messages.js'
45
import { HANDOFF_CHOICES } from '../impl/steps/how-to-proceed.js'
56
import { planCommand } from '../plan/index.js'
@@ -165,7 +166,13 @@ export async function initCommand(
165166
// multi-command flow. Drafting a plan is fast (~1–3 min of agent
166167
// thinking) and produces a reviewable artifact — `stash impl` is the
167168
// separate, slower verb that actually mutates code.
168-
if (process.stdout.isTTY) {
169+
//
170+
// Gated on the shared `isInteractive()` (config/tty.ts), the same helper
171+
// every other prompt uses. `process.stdout.isTTY` was wrong on both
172+
// counts: it ignored CI entirely (a runner with an allocated TTY blocked
173+
// here forever, since clack `confirm` reads /dev/tty), and it asked about
174+
// the wrong stream — a redirected stdin still hangs the prompt.
175+
if (isInteractive()) {
169176
const proceed = await p.confirm({
170177
message: `Continue to \`${cli} plan\` now to draft your encryption plan?`,
171178
initialValue: true,
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
import * as p from '@clack/prompts'
2+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
3+
import type { InitProvider, InitState } from '../../types.js'
4+
import { resolveProxyChoiceStep } from '../resolve-proxy-choice.js'
5+
6+
vi.mock('@clack/prompts', () => ({
7+
select: vi.fn(),
8+
isCancel: vi.fn(() => false),
9+
log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn() },
10+
}))
11+
12+
const provider = {} as InitProvider
13+
const baseState = {} as InitState
14+
15+
const originalStdinIsTTY = process.stdin.isTTY
16+
const originalStdoutIsTTY = process.stdout.isTTY
17+
18+
/**
19+
* Force BOTH streams. A CI runner that allocates a TTY has stdin *and* stdout
20+
* as TTYs — that is the configuration this gate used to hang in, and setting
21+
* stdin alone would let the old `process.stdout.isTTY` gate pass these tests
22+
* for the wrong reason (vitest's own stdout is not a TTY).
23+
*/
24+
function setTty(value: boolean) {
25+
Object.defineProperty(process.stdin, 'isTTY', { value, configurable: true })
26+
Object.defineProperty(process.stdout, 'isTTY', { value, configurable: true })
27+
}
28+
29+
function restoreTty() {
30+
Object.defineProperty(process.stdin, 'isTTY', {
31+
value: originalStdinIsTTY,
32+
configurable: true,
33+
})
34+
Object.defineProperty(process.stdout, 'isTTY', {
35+
value: originalStdoutIsTTY,
36+
configurable: true,
37+
})
38+
}
39+
40+
beforeEach(() => {
41+
vi.clearAllMocks()
42+
})
43+
44+
afterEach(() => {
45+
restoreTty()
46+
vi.unstubAllEnvs()
47+
})
48+
49+
describe('resolveProxyChoiceStep — flag short-circuit', () => {
50+
it('returns state untouched when --proxy/--no-proxy already set it', async () => {
51+
setTty(true)
52+
vi.stubEnv('CI', '')
53+
54+
const state = { ...baseState, usesProxy: true }
55+
await expect(resolveProxyChoiceStep.run(state, provider)).resolves.toEqual(
56+
state,
57+
)
58+
expect(vi.mocked(p.select)).not.toHaveBeenCalled()
59+
})
60+
})
61+
62+
describe('resolveProxyChoiceStep — CI detection with a TTY attached', () => {
63+
// Regression: this gate was `process.stdout.isTTY`, which consulted CI not
64+
// at all. On a CI runner with an allocated TTY the clack `select` rendered
65+
// and blocked on /dev/tty forever — a hang, not an error. `isInteractive()`
66+
// gates on stdin and treats CI=1/TRUE/true alike.
67+
beforeEach(() => {
68+
setTty(true)
69+
})
70+
71+
for (const ciValue of ['1', 'TRUE', 'true']) {
72+
it(`defaults to SDK-only under CI=${ciValue} without prompting`, async () => {
73+
vi.stubEnv('CI', ciValue)
74+
75+
const result = await resolveProxyChoiceStep.run(baseState, provider)
76+
77+
// Non-interactive proceeds with the safe default rather than failing:
78+
// SDK-only is what the flagless interactive prompt defaults to anyway.
79+
expect(vi.mocked(p.select)).not.toHaveBeenCalled()
80+
expect(result.usesProxy).toBe(false)
81+
expect(vi.mocked(p.log.info)).toHaveBeenCalledWith(
82+
expect.stringContaining('defaulting to SDK-only mode'),
83+
)
84+
})
85+
}
86+
87+
it('prompts when CI is unset and stdin is a TTY', async () => {
88+
vi.stubEnv('CI', '')
89+
vi.mocked(p.select).mockResolvedValueOnce(true)
90+
91+
const result = await resolveProxyChoiceStep.run(baseState, provider)
92+
93+
expect(vi.mocked(p.select)).toHaveBeenCalledTimes(1)
94+
expect(result.usesProxy).toBe(true)
95+
})
96+
97+
it('falls back to SDK-only when the interactive prompt is cancelled', async () => {
98+
vi.stubEnv('CI', '')
99+
vi.mocked(p.select).mockResolvedValueOnce(Symbol('cancel'))
100+
vi.mocked(p.isCancel).mockReturnValueOnce(true)
101+
102+
const result = await resolveProxyChoiceStep.run(baseState, provider)
103+
104+
expect(result.usesProxy).toBe(false)
105+
})
106+
})
107+
108+
describe('resolveProxyChoiceStep — no TTY', () => {
109+
it('defaults to SDK-only when stdin is not a TTY, CI unset', async () => {
110+
setTty(false)
111+
vi.stubEnv('CI', '')
112+
113+
const result = await resolveProxyChoiceStep.run(baseState, provider)
114+
115+
expect(vi.mocked(p.select)).not.toHaveBeenCalled()
116+
expect(result.usesProxy).toBe(false)
117+
})
118+
})

packages/cli/src/commands/init/steps/resolve-proxy-choice.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import * as p from '@clack/prompts'
2+
import { isInteractive } from '../../../config/tty.js'
23
import type { InitProvider, InitState, InitStep } from '../types.js'
34

45
/**
@@ -7,8 +8,8 @@ import type { InitProvider, InitState, InitStep } from '../types.js'
78
* interactive prompt, and stored on state.usesProxy.
89
*
910
* The prompt is non-blocking: cancellation falls back to false (SDK-only,
10-
* the default). In non-TTY contexts without a flag, defaults to false and
11-
* logs an info message.
11+
* the default). In non-interactive contexts without a flag, defaults to false
12+
* and logs an info message.
1213
*/
1314
export const resolveProxyChoiceStep: InitStep = {
1415
id: 'resolve-proxy-choice',
@@ -19,8 +20,13 @@ export const resolveProxyChoiceStep: InitStep = {
1920
return state
2021
}
2122

22-
// In TTY mode, prompt the user
23-
if (process.stdout.isTTY) {
23+
// Prompt only when it's safe to: stdin is a real TTY and we're not in CI.
24+
// Via the shared `isInteractive()` (config/tty.ts) so this gate matches
25+
// every other prompt gate. Keying off `process.stdout.isTTY` was wrong on
26+
// both counts — it ignored CI entirely (a runner with an allocated TTY
27+
// blocked here forever), and a redirected stdin still hangs clack `select`,
28+
// which reads from /dev/tty.
29+
if (isInteractive()) {
2430
const choice = await p.select({
2531
message:
2632
'Are you planning to query encrypted data via CipherStash Proxy, or directly via the SDK?',
@@ -48,7 +54,7 @@ export const resolveProxyChoiceStep: InitStep = {
4854
return { ...state, usesProxy: choice }
4955
}
5056

51-
// Non-TTY: default to false and log
57+
// Non-interactive: default to false and log
5258
p.log.info(
5359
'No --proxy flag set; defaulting to SDK-only mode (no `stash db push` in default flows).',
5460
)

0 commit comments

Comments
 (0)