fix: make stash impl and stash plan not hang in non-TTY contexts#446
fix: make stash impl and stash plan not hang in non-TTY contexts#446auxesis wants to merge 1 commit into
stash impl and stash plan not hang in non-TTY contexts#446Conversation
The agent-target picker previously read from `/dev/tty` and waited forever. This caused problems when running `impl` or `plan` in CI, pipes, and automation harnesses. This fix allows users to pass `--target <claude-code|codex|agents-md|wizard>` to select a handoff target non-interactively. When neither `--target` nor a TTY is available the command prints a hint and exits cleanly instead of blocking. Fixes #445.
🦋 Changeset detectedLatest commit: e01d576 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 |
📝 WalkthroughWalkthroughThis PR fixes hangs in non-TTY environments by adding a ChangesNon-TTY safety via --target flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/commands/impl/index.ts (1)
170-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign TTY detection with stdin checks used elsewhere in the codebase.
The current stdout-only check works because line 271 short-circuits before reaching the picker, but other commands check both
process.stdin.isTTY && process.stdout.isTTY. Use the same pattern here for consistency and robustness:Proposed fix
- const isTTY = process.stdout.isTTY + const isTTY = Boolean(process.stdin.isTTY && process.stdout.isTTY)Also applies to: 203-224, 268-277
🤖 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 170 - 171, The TTY detection currently sets const isTTY = process.stdout.isTTY; change this to check both streams (process.stdin.isTTY && process.stdout.isTTY) so it aligns with other commands; update the other occurrences of the same pattern in this file (the other isTTY declarations/uses around the blocks that control the interactive picker) to use the combined check so interactive-only code gates consistently.
🧹 Nitpick comments (2)
packages/cli/src/commands/impl/__tests__/impl.test.ts (1)
59-61: ⚡ Quick winReduce coupling to
howToProceedStep.runinternals in these tests.On Line 59, Line 73, and Line 87, the tests spy on an internal step object. Prefer asserting
implCommandthrough public behavior (observable output/exit behavior and command result), so harmless step refactors don’t break tests.As per coding guidelines, “Prefer testing via public API; avoid reaching into private internals in tests”.
Also applies to: 73-75, 87-88
🤖 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/__tests__/impl.test.ts` around lines 59 - 61, Tests currently spy on the internal howToProceedStep.run implementation; instead remove the vi.spyOn(howToProceedStep, 'run') usages and assert implCommand via its public behavior: invoke implCommand (the CLI entry function under test) and assert on its observable outputs (returned result, process exit code, stdout/stderr, or files produced) or mock public dependencies it calls (e.g., any injected services or modules) rather than reaching into the step object; update the three places referencing howToProceedStep.run (the spies at lines ~59, ~73, ~87) to either stub the public dependency that produces the step results or assert the final command outcome so refactoring the internal step implementation won’t break the tests.packages/cli/src/commands/init/index.ts (1)
114-116: ⚡ Quick winImport
HANDOFF_CHOICESand format the target list dynamically to eliminate duplication.The target list
claude-code|codex|agents-md|wizardis hardcoded in the outro message. This duplicates the canonicalHANDOFF_CHOICESfromhow-to-proceed.ts. If targets are added or removed fromHANDOFF_CHOICESin the future, this message will become stale without an update here, creating a maintenance burden and potential user confusion.Suggested refactor to eliminate duplication
Import
HANDOFF_CHOICESat the top of the file:import * as p from '@clack/prompts' import { planCommand } from '../plan/index.js' +import { HANDOFF_CHOICES } from '../impl/steps/how-to-proceed.js' import { createBaseProvider } from './providers/base.js'Then format the target list dynamically:
} else { // Non-TTY users (CI, agent Bash tools, pipes) will hit the same // agent-target picker in `stash plan`, which only reads from // /dev/tty. Steer them at `--target` up front so the next command // doesn't surprise them. + const targets = HANDOFF_CHOICES.map(c => c.value).join('|') p.outro( - `Next: run \`${cli} plan --target <claude-code|codex|agents-md|wizard>\` to draft your encryption plan. The \`--target\` flag is required when running non-interactively (skips the agent-target picker).`, + `Next: run \`${cli} plan --target <${targets}>\` to draft your encryption plan. The \`--target\` flag is required when running non-interactively (skips the agent-target picker).`, ) }🤖 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/index.ts` around lines 114 - 116, The outro message hardcodes the target list instead of using the canonical HANDOFF_CHOICES, causing duplication; import HANDOFF_CHOICES from how-to-proceed.ts and build the inline target string dynamically (e.g., join HANDOFF_CHOICES with '|' or similar) when calling p.outro so the message stays in sync with the source of truth; update the call site in the function that calls p.outro in packages/cli/src/commands/init/index.ts to use the generated string from HANDOFF_CHOICES.
🤖 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/plan/index.ts`:
- Around line 179-187: Replace the brittle process.stdout.isTTY check with the
stdin+CI pattern used elsewhere: use Boolean(process.stdin.isTTY) &&
process.env.CI !== 'true' to decide whether interactive prompts are allowed;
update the early-exit block that checks target (currently using
process.stdout.isTTY) and the subsequent interactive p.confirm() gating to use
this new check, and keep existing messages (p.log.info, p.outro) and
HANDOFF_CHOICES unchanged so non-interactive runs exit cleanly while
stdin-redirected or CI runs do not hang.
In `@packages/cli/tests/e2e/impl-non-tty.e2e.test.ts`:
- Around line 52-54: The test file impl-non-tty.e2e.test.ts contains hard-coded
user-facing strings; extract those strings into src/messages.ts as exported
constants (e.g., NO_AGENT_SELECTED_HINT, UNKNOWN_TARGET_ERROR,
HELP_IMPL_FLAGS_HEADER, VALID_TARGETS) and update any implementation files that
emit those messages to use the same constants, then import the constants into
the test (from ../../src/messages.js) and replace the hard-coded literals in the
expect(...) assertions with the imported symbols so the test and implementation
share a single source of truth.
---
Outside diff comments:
In `@packages/cli/src/commands/impl/index.ts`:
- Around line 170-171: The TTY detection currently sets const isTTY =
process.stdout.isTTY; change this to check both streams (process.stdin.isTTY &&
process.stdout.isTTY) so it aligns with other commands; update the other
occurrences of the same pattern in this file (the other isTTY declarations/uses
around the blocks that control the interactive picker) to use the combined check
so interactive-only code gates consistently.
---
Nitpick comments:
In `@packages/cli/src/commands/impl/__tests__/impl.test.ts`:
- Around line 59-61: Tests currently spy on the internal howToProceedStep.run
implementation; instead remove the vi.spyOn(howToProceedStep, 'run') usages and
assert implCommand via its public behavior: invoke implCommand (the CLI entry
function under test) and assert on its observable outputs (returned result,
process exit code, stdout/stderr, or files produced) or mock public dependencies
it calls (e.g., any injected services or modules) rather than reaching into the
step object; update the three places referencing howToProceedStep.run (the spies
at lines ~59, ~73, ~87) to either stub the public dependency that produces the
step results or assert the final command outcome so refactoring the internal
step implementation won’t break the tests.
In `@packages/cli/src/commands/init/index.ts`:
- Around line 114-116: The outro message hardcodes the target list instead of
using the canonical HANDOFF_CHOICES, causing duplication; import HANDOFF_CHOICES
from how-to-proceed.ts and build the inline target string dynamically (e.g.,
join HANDOFF_CHOICES with '|' or similar) when calling p.outro so the message
stays in sync with the source of truth; update the call site in the function
that calls p.outro in packages/cli/src/commands/init/index.ts to use the
generated string from HANDOFF_CHOICES.
🪄 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: 479e6513-0951-431e-a76a-7af7e109afb4
📒 Files selected for processing (11)
.changeset/handle-stdin-not-tty.mdpackages/cli/src/bin/stash.tspackages/cli/src/commands/impl/__tests__/how-to-proceed.test.tspackages/cli/src/commands/impl/__tests__/impl.test.tspackages/cli/src/commands/impl/index.tspackages/cli/src/commands/impl/steps/how-to-proceed.tspackages/cli/src/commands/init/index.tspackages/cli/src/commands/plan/index.tspackages/cli/tests/e2e/impl-non-tty.e2e.test.tspackages/cli/tests/helpers/spawn-piped.tsskills/stash-cli/SKILL.md
| // Non-TTY without --target would hang on the agent-target picker. | ||
| // Exit cleanly with a hint so automation users discover the flag. | ||
| if (!target && !process.stdout.isTTY) { | ||
| p.log.info( | ||
| `No agent selected. Pass --target <${HANDOFF_CHOICES.join('|')}> to run the handoff non-interactively.`, | ||
| ) | ||
| p.outro('No handoff performed.') | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific lines in plan/index.ts
cat -n packages/cli/src/commands/plan/index.ts | sed -n '170,220p'Repository: cipherstash/stack
Length of output: 2338
🏁 Script executed:
# Search for the impl command to find the stdin-aware pattern being referenced
find packages/cli/src/commands -name "*impl*" -type fRepository: cipherstash/stack
Length of output: 115
🏁 Script executed:
# Also search for any stdin-aware interactive checks in the codebase
rg -A 5 -B 2 "isTTY|stdin|process\.stdin" packages/cli/src/commands --max-count=20Repository: cipherstash/stack
Length of output: 11094
🏁 Script executed:
# Verify the actual pattern in db/install.ts more clearly
cat -n packages/cli/src/commands/db/install.ts | sed -n '10,30p'Repository: cipherstash/stack
Length of output: 820
🏁 Script executed:
# Check if there are any tests or comments explaining the isTTY choice in plan/impl
rg -B 5 -A 2 "isTTY|stdin.*TTY" packages/cli/src/commands/plan/index.ts packages/cli/src/commands/impl/index.tsRepository: cipherstash/stack
Length of output: 4007
Use stdin+TTY checks for robust non-interactive gating.
The code relies on process.stdout.isTTY only, which can miss stdin-redirection cases (e.g., ./cli plan < input.txt). This risks hanging on p.confirm() at line 191-212 when stdout appears TTY but stdin is not. Use the pattern from packages/cli/src/commands/db/install.ts instead: Boolean(process.stdin.isTTY) && process.env.CI !== 'true'.
Also applies to: 191-212
🤖 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 179 - 187, Replace the
brittle process.stdout.isTTY check with the stdin+CI pattern used elsewhere: use
Boolean(process.stdin.isTTY) && process.env.CI !== 'true' to decide whether
interactive prompts are allowed; update the early-exit block that checks target
(currently using process.stdout.isTTY) and the subsequent interactive
p.confirm() gating to use this new check, and keep existing messages
(p.log.info, p.outro) and HANDOFF_CHOICES unchanged so non-interactive runs exit
cleanly while stdin-redirected or CI runs do not hang.
| expect(r.stdout).toContain('No agent selected') | ||
| expect(r.stdout).toContain('--target') | ||
| expect(r.stdout).toContain('claude-code|codex|agents-md|wizard') |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract hard-coded user-facing strings to src/messages.ts.
The test assertions contain hard-coded user-facing strings ('No agent selected', 'Unknown --target', 'Impl Flags:', '--target', 'claude-code|codex|agents-md|wizard', etc.) that should be extracted as constants. As per coding guidelines, E2E tests should import these strings from src/messages.ts rather than hard-coding them, ensuring a single source of truth and preventing drift between implementation and test expectations.
Recommended approach
- Define constants in
src/messages.ts:
export const NO_AGENT_SELECTED_HINT = 'No agent selected'
export const UNKNOWN_TARGET_ERROR = 'Unknown --target'
export const HELP_IMPL_FLAGS_HEADER = 'Impl Flags:'
export const VALID_TARGETS = 'claude-code|codex|agents-md|wizard'
// ... etc-
Update implementation files to use these constants
-
Import and use in this test:
import { NO_AGENT_SELECTED_HINT, UNKNOWN_TARGET_ERROR, ... } from '../../src/messages.js'
// Then in assertions:
expect(r.stdout).toContain(NO_AGENT_SELECTED_HINT)As per coding guidelines, E2E tests under tests/e2e/**/*.e2e.test.ts must extract user-facing strings that E2E tests assert on into src/messages.ts as constants rather than hard-coding them in tests.
Also applies to: 66-69, 77-79
🤖 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/tests/e2e/impl-non-tty.e2e.test.ts` around lines 52 - 54, The
test file impl-non-tty.e2e.test.ts contains hard-coded user-facing strings;
extract those strings into src/messages.ts as exported constants (e.g.,
NO_AGENT_SELECTED_HINT, UNKNOWN_TARGET_ERROR, HELP_IMPL_FLAGS_HEADER,
VALID_TARGETS) and update any implementation files that emit those messages to
use the same constants, then import the constants into the test (from
../../src/messages.js) and replace the hard-coded literals in the expect(...)
assertions with the imported symbols so the test and implementation share a
single source of truth.
2 participants
The agent-target picker previously read from
/dev/ttyand waited forever. This caused problems when runningimplorplanin CI, pipes, and automation harnesses.This fix allows users to pass
--target <claude-code|codex|agents-md|wizard>to select a handoff target non-interactively.When neither
--targetnor a TTY is available the command prints a hint and exits cleanly instead of blocking.Fixes #445.
Summary by CodeRabbit
New Features
--targetflag tostash planandstash implfor non-interactive agent selection in automation environments (supports: claude-code, codex, agents-md, wizard).Bug Fixes
--targetis not provided.Documentation
--targetusage.