fix(cli): route stash impl and the two stash init prompts through the shared TTY gate#704
Merged
Conversation
`stash impl` gated prompting on an inline `Boolean(process.stdin.isTTY) && process.env.CI !== 'true'`, which only recognises the exact lowercase spelling of CI. On a runner that sets `CI=1` or `CI=TRUE` and allocates a TTY, `stash plan` correctly took the non-interactive path but `stash impl` believed it was interactive and blocked on the plan-summary confirm, or on the agent-target picker (clack `select` reads /dev/tty). A hang, not an error. Use `isInteractive()` from `config/tty.ts` — the same helper `stash plan` already uses, whose `isCiEnv()` accepts `1`/`true` in any case. Unit test pins the behaviour across CI=1, CI=TRUE, CI=true and CI unset.
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.
🦋 Changeset detectedLatest commit: e5eb696 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
📝 WalkthroughWalkthroughThe CLI now uses shared TTY and CI detection for ChangesTTY and CI interactivity
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
The flag-consent path was the one prompt on `stash impl` that never went through `isInteractive()`. With no plan on disk and `--continue-without-plan` passed, a CI runner still reached `p.confirm`, which reads /dev/tty — the same hang the rest of this branch closes. Worse if the prompt did resolve: the confirm is `initialValue: false`, so a non-interactive run would cancel work the flag had explicitly authorised. The flag is the consent. Take it as given when non-interactive; keep the interactive confirmation unchanged.
auxesis
reviewed
Jul 20, 2026
auxesis
left a comment
auxesis
left a comment
Contributor
There was a problem hiding this comment.
Verdict
The fix itself is right and well-argued: four ad-hoc prompt gates collapse onto the shared isInteractive() helper, which corrects both defects at once — the CI spelling (isCiEnv() accepts 1/true in any case, where the inline CI !== 'true' did not) and the stream (stdin, not stdout). The comments left at each call site explain why rather than what, and the new suites force both streams to TTY so the old process.stdout.isTTY gate can't pass for the wrong reason. That's the right paranoia.
The consolidated concern is coverage shape, not correctness. Nearly every new test asserts the non-interactive arm (prompt not reached, handoff not run). Two things follow:
- The stream half of the fix is unverified. Both
setTty()and theinit-command.test.tsbeforeEachmovestdinandstdouttogether, so no test expressesstdin=false, stdout=true— the exact configuration (stash init < /dev/nullfrom a terminal) that distinguishes the new gate from the old one. Right now only theCIaxis actually discriminates old from new;process.stdout.isTTYcould be reinstated in either file and these suites stay green. - The interactive arms the gate change made reachable are largely unlocked — the
--continue-without-planconfirm, the chain-offer accept path, and the newCI+ TTY route intoexit(1).
None of this blocks merge. 8 gaps inline, ordered by confidence.
Additional findings not posted inline
init-command.test.ts: the non-interactive-by-stdin path (stdin not a TTY,CIunset) never asserts theplan --targetoutro — only theCI-with-TTY loop does. Both reach the sameelse, but a regression that madeisInteractive()CI-only would go unnoticed.resolve-proxy-choice.test.ts:87: the interactive prompt is only exercised withselectresolvingtrue. Resolvingfalse(theinitialValue, and by far the common answer) has no test — andfalseis the same value the non-interactive default produces, so nothing distinguishes "user chose SDK" from "prompt was skipped".impl.test.ts: the plan-exists + non-interactive branch logsPlan at `.cipherstash/plan.md` — agent will execute it as the source of truth.Nothing asserts that message, so a non-interactive run could go silent about which plan it's about to execute.
Out of scope, noted only
Not a coverage point: --continue-without-plan is now the sole consent under CI, a deliberate loosening of a default-no safety confirm. Worth a sanity check that the flag can only arrive from argv — not from .cipherstash/context.json or an env var — since the prompt that used to backstop it is gone in automation.
Review stats
| Source | Model | Type | Raw | Kept |
|---|---|---|---|---|
| claude | claude-opus-4-8 | test-gap | 7 | 7 |
| codex | gpt-5.5 | test-gap | 3 | 3 |
- Unique findings after de-duplication: 8 (all 8 posted inline; nothing dropped for the cap — the 3 bullets above are the sources' own secondary lists).
- Cross-model overlap: 3 of 8 kept findings were corroborated by 2+ models — the redirected-stdin gap in
resolve-proxy-choice, the same gap in theinitchain offer, and the untested non-interactive handoff success path. - Dropped as unverifiable: 0. Every finding from both sources was substantiated against the diff.
Adds the tests auxesis flagged as missing on #704, each verified against the implementation: - resolve-proxy-choice: stdin-piped/stdout-TTY split (the "wrong stream" claim) and the --no-proxy (usesProxy:false) short-circuit. - init chain offer: redirected-stdin skip, and the accept arm that chains into `stash plan` (planCommand + early return). - impl: still runs under CI=1 with --target (plus a --continue-without-plan variant), the interactive confirm + decline-aborts path, and the no-plan/no-flag reroute into the exit(1) branch. - impl PTY e2e: real-hang reproducer under a genuine TTY with CI=1/TRUE (the headline changeset claim; the unit tests only lock the boolean). Test-only change; no changeset required.
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
2 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.
Closes #727
Closes #727
Three commands could hang forever on a CI runner that allocates a TTY. Each reimplemented the interactivity check inline instead of using
config/tty.ts, and each got a different subset right.isInteractive()isBoolean(process.stdin.isTTY) && !isCiEnv(), whereisCiEnv()matches/^(1|true)$/iagainst a trimmedCI.The three sites
commands/impl/index.tsprocess.stdin.isTTY && process.env.CI !== 'true'CI=1/CI=TRUEpass the check → blocks onp.confirmor the agent-target pickercommands/init/index.tsprocess.stdout.isTTYCIcheck at all → blocks on the "chain intostash plan?" confirmcommands/init/steps/resolve-proxy-choice.tsprocess.stdout.isTTYCIcheck → blocks on the proxy-vs-SDK select (unless--proxy/--no-proxyshort-circuits it)plan/index.tsalready used the shared helper, and carries a comment explaining precisely why the inline form is wrong —implsat a few files away doing exactly that.Two distinct bugs are being fixed.
implhad the right stream and the wrongCItest. Both init sites had the wrong stream and noCItest — strictly more exposed, since neither caught evenCI=true.It hangs rather than errors because clack's prompts open
/dev/ttydirectly rather than readingprocess.stdin, so there's nothing to fail — the prompt just waits until the job hits its wall-clock limit.Behaviour under CI
Both init sites already had correct non-interactive branches in their
else; only the gate selecting them was wrong.init/index.ts— skips the offer and prints the existingplan --targetsteering outro.resolve-proxy-choice.ts— returnsusesProxy: falsewith an info log. That default is unchanged, not new: it is the prompt's owninitialValueand what cancelling already produced. The--proxy/--no-proxyshort-circuit is untouched.Verification
Reverting each gate fails the corresponding tests: 6 for the init sites (all three
CIspellings × 2 sites), 2 forimpl(CI=1,CI=TRUE— that gate did catchCI=true). Both counts are what the respective bugs predict.Worth noting: the first version of the init tests stubbed only
process.stdin.isTTYand passed against the reverted code, because vitest's ownprocess.stdout.isTTYis already false — so the old stdout gate skipped the prompt for an unrelated reason. A real CI runner has both streams as TTYs. Both test files now force both, with a comment explaining why.Full suite: 52 files, 598 tests passed (up from 587), no pre-existing test modified.
code:checkexits 0 at the existing baseline.Note
resolve-proxy-choiceis the only one of the three where the non-interactive path makes a behavioural choice rather than skipping a nicety — a CIstash initwithout--proxynow reliably gets SDK-only config with only an info log. Already true for piped runs; this extends it to CI-with-TTY. Left as-is: the Proxy is a v2-era concern and SDK-direct is the v3 path, so erroring would break CI runs that have no opinion about it.No
skills/*update — commands, flags and prompt text are unchanged; only the condition under which an existing prompt renders.🤖 Generated with Claude Code
Summary by CodeRabbit
stash implandstash inithanging on CI runners when a TTY is attached.CIvalue casing/format.