Refuse to prompt when nothing can answer the prompt - #652
Conversation
There was a problem hiding this comment.
Pull request overview
Prevents TUI prompts from hanging when stdin or output streams are non-terminal.
Changes:
- Uses terminal detection and centralized TUI launcher guards.
- Rejects non-interactive setup and unconfirmable chat deletion.
- Adds unit, structural, PTY, and e2e regression tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/tui/resolve/resolve_test.go |
Tests resolver behavior with real PTYs. |
internal/tui/picker.go |
Adds guarded Bubble Tea execution. |
internal/tui/launchers_test.go |
Adds structural launcher checks. |
internal/tui/forms.go |
Centralizes guarded huh form execution. |
internal/tui/forms_test.go |
Tests prompt and picker floors. |
internal/stdinarg/stdinarg.go |
Uses terminal-aware predicates. |
internal/stdinarg/stdinarg_test.go |
Adds terminal predicate matrices. |
internal/commands/wizard.go |
Rejects non-interactive setup. |
internal/commands/wizard_test.go |
Tests setup refusal and subcommands. |
internal/commands/helpers.go |
Adds deletion confirmability guard. |
internal/commands/chat.go |
Guards chat deletion before lookups. |
internal/commands/chat_test.go |
Tests early chat deletion refusal. |
internal/cli/root_test.go |
Replaces character-device test stubs. |
internal/appctx/context_test.go |
Tests interactivity using a PTY. |
e2e/setup.bats |
Adds setup anti-hang scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
751f1df to
9d571b9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d571b9cec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9d571b9 to
dcd04d6
Compare
dcd04d6 to
dd5b89b
Compare
dd5b89b to
9fca545
Compare
9fca545 to
c29832f
Compare
c29832f to
2005e9c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2005e9cf1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2005e9c to
5d9dffa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d9dffa830
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`basecamp setup --json < /dev/null` hung forever. #646 removed `timeline --watch` as "the only reachable tea.NewProgram without a gate", but that claim came from `rg 'tea.NewProgram'`, and the instrument could not see the larger family: internal/tui/forms.go wraps huh, and huh calls tea.NewProgram inside its own package. Every tui.Confirm/Select/Input was a bubbletea launcher the grep never showed, and `NewSetupCmd` reached one with no interactivity check at all. Two things make a reached prompt hang rather than fail. Bubble Tea does not error on a non-terminal stdin — it opens /dev/tty and waits on the real terminal (tea.go:590-613). And InteractiveStdio() tested ModeCharDevice, so /dev/null passed: `cmd < /dev/null`, which is exactly how an agent says it has nothing to type, was classified interactive. So fix the instrument rather than adding another call-site check: - InteractiveStdio() now asks term.IsTerminal, from the same charmbracelet/x/term that Bubble Tea itself imports, so this floor and the /dev/tty fallback it prevents cannot disagree about what a terminal is. IsPiped keeps the character-device test; it answers a different question. Adds InteractivePrompt() for stdin+stderr, because huh draws forms to stderr while the picker draws to stdout — asking about the wrong stream would let `cmd 2>somewhere` render an invisible question that still blocks a terminal, and would refuse `cmd | less` where the prompt would have worked fine. - internal/tui grows a floor with one guarded runner per launcher: every huh form goes through runForm, every bubbletea program through runPicker. Both return a typed ErrNotInteractive. This covers prompts nobody has written yet, which a call-site audit cannot — the audit is what missed setup. - `basecamp setup` refuses non-interactive stdio and machine-output modes with a usage error naming what does work without a terminal. The gate is on the parent's RunE only: `setup claude`, `setup codex` and `setup agents` stay non-interactive and keep working. Refusing machine output leaves the wizard's structured envelope unreachable, so that branch, its two helpers and WizardResult's json tags go with it. - `chat delete` refuses before any lookup when it would reach a confirmation stdin cannot answer, naming --force. isNonInteractiveCommand is left alone; it reads stdout, never stdin, and widening it would change missingArg and noChanges across many commands. Tests. A structural check that fails on a bubbletea launcher or a huh import outside a short allowlist, and — inside an allowlisted file — on a launch outside its named runner, so a second unguarded program cannot ride in on the file's exemption. Its import resolver is itself tested, because getting it wrong is silent. Both bubbletea modules sit at a path ending in bubbletea (or bubbletea/v2) and declare `package tea`, so path and package name disagree: deriving the identifier from the path registers "bubbletea" while the file binds "tea", and every unaliased launcher walks straight past. Nothing in the repo would have noticed, since picker.go happens to alias its import. The resolver now records each module's declared package name and honors an explicit alias over it, with all four spellings pinned by test. Also: a table over every exported prompt, AST-checked for completeness, that fails if a new prompt skips the floor. A predicate matrix whose terminal/terminal row is the baseline that keeps the negative rows from holding vacuously. And e2e cases that put a real pty on stdout with /dev/null on stdin, the only shape that reproduces the original hang. Tests that used /dev/null as a terminal stand-in now open /dev/ptmx, and each floor test points the launcher's own output stream at a pty so stdin is the variable actually under test.
5d9dffa to
b9b5c80
Compare
basecamp setup --json < /dev/nullhangs forever. This is the same defect class as #646, wearinghuhinstead oftea, on a shipped code path.The instrument was wrong, twice
#646 removed
timeline --watchas "the only reachabletea.NewProgramwithout a gate." That claim came fromrg 'tea.NewProgram', and the grep could not see the family:internal/tui/forms.gowrapshuh, and huh callstea.NewPrograminside its own package. Everytui.Confirm/Select/Inputis a bubbletea launcher the grep never showed.NewSetupCmd'sRunEcalledrunWizardwith no interactivity check at all, reachingtui.Confirmatwizard.go:243.Two things turn a reached prompt into a hang rather than a failure:
/dev/ttyand waits on the real terminal (tea.go:590-613). Redirecting stdin cannot escape a prompt.InteractiveStdio()testedModeCharDevice, and/dev/nullis a character device. Socmd < /dev/null— exactly how an agent says "I have nothing to type" — was classified interactive.Point 2 means a call-site gate using the old predicate would not have fixed the reported bug at all. The fix is the instrument, not another selector.
What changed
internal/stdinarg—InteractiveStdio()now asksterm.IsTerminal, from the samecharmbracelet/x/termthat Bubble Tea itself imports (v1tea.go:25, v2tea.go:34), so this floor and the/dev/ttyfallback it prevents cannot disagree about what a terminal is.IsPipedkeeps the character-device test — it answers a different question ("is there content on stdin?"), for which/dev/nullcorrectly reads as nothing.New
InteractivePrompt()checks stdin + stderr, because huh draws forms to stderr (form.go:112) while the picker draws to stdout. Asking about the wrong stream would letcmd 2>somewhererender an invisible question that still blocks a terminal, and would refusecmd | lesswhere the prompt would have worked.internal/tui— a floor with one guarded runner per launcher: every huh form throughrunForm, every bubbletea program throughrunPicker. Both return a typedErrNotInteractive. This covers prompts nobody has written yet, which a call-site audit cannot — the audit is what missedsetup.basecamp setup— refuses non-interactive stdio and machine-output modes with a usage error naming what does work without a terminal (setup agents,setup claude/codex,config set account_id). The gate is on the parent'sRunEonly:setup claude,setup codexandsetup agentsstay non-interactive and keep working, asserted by unit and e2e tests. Refusing machine output leaves the wizard's structured-envelope branch unreachable, so it, its two helpers, andWizardResult's json tags go with it.chat delete— refuses before any lookup when it would reach a confirmation stdin cannot answer, naming--force.isNonInteractiveCommandis deliberately not widened: it reads stdout, never stdin, and its other callers (missingArg,noChanges) use it to choose between help and a structured error.Tests
A structural check fails on a bubbletea launcher or huh import outside a short allowlist, and — inside an allowlisted file — on a launch outside its named runner, so a second unguarded program cannot ride in on the file's exemption.
Its import resolver is itself tested, because getting it wrong is silent. Both bubbletea modules sit at a path ending
bubbletea(orbubbletea/v2) and declarepackage tea, so path and package name disagree. An earlier revision derived the identifier from the path, registeringbubbleteawhile the file bindstea— every unaliased launcher walked straight past, in both modules. Nothing in the repo would have noticed, becausepicker.gohappens to alias its import. The resolver now records each module's declared package name and honors an explicit alias over it, with all four spellings pinned.Also: a table over every exported prompt, AST-checked for completeness, that fails if a new prompt skips the floor. A predicate matrix whose terminal/terminal row is the baseline that keeps the negative rows from holding vacuously. And e2e cases that put a real pty on stdout with
/dev/nullon stdin — the only shape that reproduces the original hang, and one bats cannot produce on its own since it captures stdout. Tests that used/dev/nullas a terminal stand-in now open/dev/ptmx, and each floor test points the launcher's own output stream at a pty so stdin is the variable actually under test.Verification
Every guard was checked by reverting it and confirming the test fails. Anti-hang smoke, where exit 124 would be the bug reproducing:
bin/cigreen.go.modis unchanged (charmbracelet/x/termwas already a direct dependency).Stack
Two follow-ups are stacked on this branch: the dead-TUI cleanup this makes obvious, and a
--forcesafety change forchat delete.Summary by cubic
Refuses to launch TUIs when nothing can answer them, preventing hangs with redirected stdio. Old behavior:
huh/bubbleteaopened/dev/ttyand blocked; new behavior: commands require real terminals and return usage errors with hints for non-interactive alternatives.internal/stdinarg.InteractiveStdio()now usesgithub.com/charmbracelet/x/term; addsInteractivePrompt()(stdin+stderr) forhuhforms;IsPiped()unchanged.huhforms,bubbleteapickers, and the spinner run through guarded runners and returntui.ErrNotInteractive;WithAutoSelectSinglestill bypasses TUIs.basecamp setupgates in the parentRunE, refuses non-interactive stdio and machine-output modes, and removes the wizard’s JSON envelope/helpers.chat deletefails early when stdin/stderr can’t confirm and requires--forcefor non-interactive runs.skillreportstui.ErrNotInteractiveas a usage error with a hint; real cancellations still exit 0 and stderr redirection is handled correctly.appctx, CLI, and resolver now require terminals (not just character devices).huhimports andbubbletealaunchers;skills/basecamp-doctor/SKILL.mddirects non-interactive setups to agent-specific commands.Migration
basecamp setupin non-interactive contexts: usebasecamp setup agents(orbasecamp setup claude/basecamp setup codex), or set defaults withbasecamp config set account_id <id>andbasecamp config set project_id <id>.--forcetobasecamp chat delete.basecamp skill installorbasecamp skillto print the file.Written for commit b9b5c80. Summary will update on new commits.