From b9b5c80ccc50f4ec928f6bc17174c82932d9bec0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 22 Aug 2026 01:43:52 -0700 Subject: [PATCH] Refuse to prompt when nothing can answer the prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- e2e/setup.bats | 193 +++++++++ internal/appctx/context.go | 9 +- internal/appctx/context_test.go | 33 +- internal/cli/root.go | 8 +- internal/cli/root_test.go | 29 +- internal/commands/chat.go | 30 +- internal/commands/chat_test.go | 42 ++ internal/commands/helpers.go | 26 ++ internal/commands/skill.go | 48 ++- internal/commands/skill_test.go | 77 ++++ internal/commands/wizard.go | 130 +++--- internal/commands/wizard_agents.go | 9 +- internal/commands/wizard_test.go | 441 ++++++++++++++++----- internal/stdinarg/stdinarg.go | 55 ++- internal/stdinarg/stdinarg_test.go | 108 +++-- internal/tui/forms.go | 144 ++++--- internal/tui/forms_test.go | 341 ++++++++++++++++ internal/tui/launchers_test.go | 564 +++++++++++++++++++++++++++ internal/tui/paginated_picker.go | 7 +- internal/tui/picker.go | 62 ++- internal/tui/resolve/resolve.go | 18 +- internal/tui/resolve/resolve_test.go | 56 ++- internal/tui/spinner.go | 4 + skills/basecamp-doctor/SKILL.md | 9 +- 24 files changed, 2130 insertions(+), 313 deletions(-) create mode 100644 e2e/setup.bats create mode 100644 internal/tui/forms_test.go create mode 100644 internal/tui/launchers_test.go diff --git a/e2e/setup.bats b/e2e/setup.bats new file mode 100644 index 000000000..7e7aba7b0 --- /dev/null +++ b/e2e/setup.bats @@ -0,0 +1,193 @@ +#!/usr/bin/env bats +# setup.bats - `basecamp setup` refuses to prompt when nothing can answer it. +# +# The wizard is prompts end to end, and huh runs them as a bubbletea program. +# Redirecting stdin does not make that program fail: bubbletea sees a +# non-terminal stdin and opens /dev/tty instead, so the prompt waits on the real +# terminal — `basecamp setup --json < /dev/null` hung forever. +# +# Every case runs under a timeout, and the timeout is the assertion: exit 124 is +# the bug reproducing. A unit test with a fake reader cannot catch this, because +# the hang lives in what the real os.Stdin makes bubbletea do — which is also +# why the PTY case at the bottom exists. Under bats alone, stdout is captured, +# so stdout is what fails the interactivity check and stdin is never the +# deciding factor. + +load test_helper + +# timeout_bin names GNU timeout, which stock macOS does not ship. Tests that +# need it skip rather than silently drop their only real assertion. +timeout_bin() { + if command -v timeout >/dev/null 2>&1; then + echo timeout + elif command -v gtimeout >/dev/null 2>&1; then + echo gtimeout + fi +} + +# run_guarded runs a shell snippet under a timeout. +# +# The timeout wraps the whole shell, not the snippet's first word. Prefixing it +# (`timeout 10 printf '' | basecamp setup`) times `printf` and leaves `basecamp` +# to hang the suite forever — the exact failure these tests exist to catch. +# pipefail so a refusal upstream of a pipe is not masked by the last stage. +run_guarded() { + local to + to="$(timeout_bin)" + if [[ -z "$to" ]]; then + skip "no GNU timeout available (install coreutils)" + fi + run "$to" 10 bash -c "set -o pipefail; $1" +} + +assert_not_timed_out() { + if [[ "$status" -eq 124 ]]; then + echo "Command hit the timeout (exit 124) — it hung instead of refusing" + echo "Output: $output" + return 1 + fi +} + +# assert_refused is the whole contract: exit 1, a usage envelope, and a hint +# naming something that works without a terminal. "Non-zero and not 124" is too +# weak — the pre-fix binary also exits non-zero where no controlling terminal +# exists, on huh's own TTY error, several steps in. +assert_refused() { + assert_not_timed_out + assert_exit_code 1 + assert_json_value '.code' 'usage' + assert_json_value '.hint | contains("basecamp setup agents")' 'true' +} + + +@test "setup --json with stdin closed refuses instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999}' + + run_guarded "basecamp setup --json < /dev/null" + assert_refused +} + +@test "setup without --json and stdin closed refuses instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999}' + + run_guarded "basecamp setup < /dev/null" + assert_refused +} + +@test "setup with piped stdin refuses instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999}' + + run_guarded "printf '' | basecamp setup --json" + assert_refused +} + +@test "setup with piped stdout refuses instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999}' + + run_guarded "basecamp setup --json < /dev/null | cat" + assert_refused +} + + +# The reported bug: a real terminal on stdout, nothing on stdin. bats cannot +# produce that on its own — it captures stdout — so borrow a pty from script(1). +# The exit code travels in a sentinel rather than through script, whose status +# propagation differs between the util-linux and BSD versions. + +# run_in_pty runs a shell snippet with stdout attached to a pseudo-terminal. +run_in_pty() { + local snippet="$1" + if ! command -v script >/dev/null 2>&1; then + skip "script(1) not available to allocate a pty" + fi + if script --version 2>&1 | grep -qi util-linux; then + run bash -c "script -qec '$snippet' /dev/null | tr -d '\r'" + else + run bash -c "script -q /dev/null /bin/sh -c '$snippet' | tr -d '\r'" + fi +} + +@test "setup --json on a terminal with stdin closed refuses instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999}' + + local to + to="$(timeout_bin)" + if [[ -z "$to" ]]; then + skip "no GNU timeout available (install coreutils)" + fi + + # stdout is a pty here, so /dev/null on stdin is the only disqualifier — the + # case a character-device check called interactive and bubbletea did not. + run_in_pty "$to 10 basecamp setup --json < /dev/null; echo EXIT:\$?" + + assert_output_not_contains "EXIT:124" + assert_output_contains "EXIT:1" + assert_output_contains "basecamp setup agents" +} + +@test "setup on a terminal with stdin closed refuses instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999}' + + local to + to="$(timeout_bin)" + if [[ -z "$to" ]]; then + skip "no GNU timeout available (install coreutils)" + fi + + run_in_pty "$to 10 basecamp setup < /dev/null; echo EXIT:\$?" + + assert_output_not_contains "EXIT:124" + assert_output_contains "EXIT:1" + assert_output_contains "basecamp setup agents" +} + + +# The gate is on the parent's RunE only. These three are the supported +# non-interactive paths and have to keep working — a persistent hook would have +# taken all of them out, which is the easiest thing to get wrong here. + +# hide_agent_binaries drops the developer's real claude/codex from PATH. Without +# it these tests shell out to whichever agent CLI happens to be installed, and +# those have prompts of their own — a hang in somebody else's tool, unrelated to +# the gate under test. What we are asserting is that the parent's gate does not +# reach the subcommands, and that holds with or without an agent installed. +hide_agent_binaries() { + export PATH="$BASECAMP_ROOT/bin:/usr/bin:/bin" +} + +@test "setup agents still runs without a terminal" { + create_credentials + create_global_config '{"account_id": 99999}' + hide_agent_binaries + export BASECAMP_SETUP_AGENT=none + + run_guarded "basecamp setup agents --json < /dev/null" + assert_not_timed_out + assert_success +} + +@test "setup claude still runs without a terminal" { + create_credentials + create_global_config '{"account_id": 99999}' + hide_agent_binaries + + run_guarded "basecamp setup claude --json < /dev/null" + assert_not_timed_out + assert_success +} + +@test "setup codex still runs without a terminal" { + create_credentials + create_global_config '{"account_id": 99999}' + hide_agent_binaries + + run_guarded "basecamp setup codex --json < /dev/null" + assert_not_timed_out + assert_success +} diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 54906af87..7f65992c5 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -375,10 +375,11 @@ func (a *App) IsInteractive() bool { return false } - // Both stdout and stdin must be character devices: a TUI draws to stdout - // and reads keystrokes from stdin, so a pipe on either end can never - // drive one — and when the command is consuming piped content (a "-" - // stdin input), a TUI would eat that content as key events. + // Both stdout and stdin must be terminals: a TUI draws to stdout and reads + // keystrokes from stdin, so a pipe on either end can never drive one — and + // when the command is consuming piped content (a "-" stdin input), a TUI + // would eat that content as key events. A character device is not enough; + // /dev/null is one and delivers nothing. See stdinarg.InteractiveStdio. return stdinarg.InteractiveStdio() } diff --git a/internal/appctx/context_test.go b/internal/appctx/context_test.go index 87d730e68..249e5e42e 100644 --- a/internal/appctx/context_test.go +++ b/internal/appctx/context_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "os" + "runtime" "sync/atomic" "testing" "time" @@ -205,30 +206,34 @@ func TestIsInteractiveWithCountMode(t *testing.T) { } func TestIsInteractiveWithNonInteractiveEnv(t *testing.T) { - // Swap os.Stdout to the null device — a char device that passes the - // ModeCharDevice guard — so IsInteractive() would otherwise return true. - // Without this, go test's piped stdout makes IsInteractive() false regardless - // of the env var, and the assertion would pass even if the short-circuit were - // removed. - devNull, err := os.Open(os.DevNull) + // Point both ends at a pseudo-terminal so IsInteractive() would otherwise + // return true. Without this, go test's piped stdout and /dev/null stdin make + // it false regardless of the env var, and the assertion would pass even if + // the short-circuit were removed. /dev/null will not stand in for a terminal + // here: it is a character device but not a terminal, which is precisely the + // distinction IsInteractive() now draws. + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) if err != nil { - t.Skip(os.DevNull + " not available") + t.Skipf("open /dev/ptmx: %v", err) } - origStdout := os.Stdout - os.Stdout = devNull + origStdout, origStdin := os.Stdout, os.Stdin + os.Stdout, os.Stdin = pty, pty t.Cleanup(func() { - os.Stdout = origStdout - devNull.Close() + os.Stdout, os.Stdin = origStdout, origStdin + pty.Close() }) cfg := &config.Config{} app := NewApp(cfg) - // Baseline: char-device stdout, no env/flags → interactive. + // Baseline: terminal stdio, no env/flags → interactive. t.Setenv("BASECAMP_NONINTERACTIVE", "") - require.True(t, app.IsInteractive(), "char-device stdout should be interactive without the escape hatch") + require.True(t, app.IsInteractive(), "terminal stdio should be interactive without the escape hatch") - // The env escape hatch forces non-interactive even with an interactive stdout. + // The env escape hatch forces non-interactive even with interactive stdio. t.Setenv("BASECAMP_NONINTERACTIVE", "1") assert.False(t, app.IsInteractive(), "BASECAMP_NONINTERACTIVE should force non-interactive") // Output format is untouched — the escape hatch only disables prompts. diff --git a/internal/cli/root.go b/internal/cli/root.go index 745791be5..28f4b8102 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -578,13 +578,15 @@ func profileNames(cfg *config.Config) string { } // isInteractiveTTY reports whether the profile picker may run: no -// noninteractive mode set, and both ends of stdio are character devices. +// noninteractive mode set, and both ends of stdio are terminals. // // Stdin counts because the picker is a TUI reading key events, and this runs // from PersistentPreRunE — before any command touches its own input. Gating on // stdout alone let "printf body | basecamp todos create -" open the picker on -// a terminal stdout and consume the piped body as keystrokes. Same predicate -// as App.IsInteractive and resolve.Resolver.IsInteractive. +// a terminal stdout and consume the piped body as keystrokes. A character +// device is not enough either: /dev/null is one, delivers no keystrokes, and +// Bubble Tea answers it by waiting on /dev/tty. Same predicate as +// App.IsInteractive and resolve.Resolver.IsInteractive. func isInteractiveTTY(flags appctx.GlobalFlags) bool { if config.NonInteractiveEnv() { return false diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index a68c88781..e6ec8140f 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "os" + "runtime" "testing" "github.com/spf13/cobra" @@ -166,7 +167,7 @@ func isolateRootTest(t *testing.T) { } func TestIsInteractiveTTYWithNonInteractiveEnv(t *testing.T) { - stubCharDeviceStdio(t) + stubTerminalStdio(t) t.Setenv("BASECAMP_NONINTERACTIVE", "") require.True(t, isInteractiveTTY(appctx.GlobalFlags{})) @@ -283,10 +284,10 @@ func TestVersionWithJQReturnsUsageError(t *testing.T) { // feeding a "-" content input the picker would eat that body as keystrokes — // so a terminal stdout is not on its own enough to open one. func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { - stubCharDeviceStdio(t) + stubTerminalStdio(t) t.Setenv("BASECAMP_NONINTERACTIVE", "") - require.True(t, isInteractiveTTY(appctx.GlobalFlags{}), "char-device stdio is interactive") + require.True(t, isInteractiveTTY(appctx.GlobalFlags{}), "terminal stdio is interactive") reader, writer, err := os.Pipe() require.NoError(t, err) @@ -306,13 +307,12 @@ func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { // must stay nil for cobra's unknown-command handling), so quick-start's own // interactive paths sit behind it. The e2e suite always has a piped stdout, // which takes the machine-output branch and never reaches them — this covers -// the other side: a character-device stdout, the terminal stand-in, with a -// piped stdin. The root carries a subcommand so InstallDashGuard takes the +// the other side: a terminal stdout with a piped stdin. The root carries a subcommand so InstallDashGuard takes the // pre-run branch production uses, not the Args branch for a childless root. func TestRootDashGuardWithTerminalStdout(t *testing.T) { isolateRootTest(t) - stubCharDeviceStdio(t) + stubTerminalStdio(t) t.Setenv("BASECAMP_NONINTERACTIVE", "") reader, writer, err := os.Pipe() @@ -350,16 +350,23 @@ func TestRootDashGuardWithTerminalStdout(t *testing.T) { // device, so interactivity assertions do not depend on how `go test` itself was // invoked — a piped stdin on the test runner would otherwise fail the // interactive baseline now that both streams are checked. -func stubCharDeviceStdio(t *testing.T) { +// stubTerminalStdio points stdio at a pseudo-terminal, the only stand-in +// isInteractiveTTY accepts. /dev/null used to serve here — it is a character +// device — but a character device is not a terminal, and treating it as one is +// how `cmd < /dev/null` ended up launching prompts that wait on /dev/tty. +func stubTerminalStdio(t *testing.T) { t.Helper() - devNull, err := os.Open(os.DevNull) + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) if err != nil { - t.Skip(os.DevNull + " not available") + t.Skipf("open /dev/ptmx: %v", err) } origStdout, origStdin := os.Stdout, os.Stdin - os.Stdout, os.Stdin = devNull, devNull + os.Stdout, os.Stdin = pty, pty t.Cleanup(func() { os.Stdout, os.Stdin = origStdout, origStdin - devNull.Close() + pty.Close() }) } diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 65e404657..af24e0f3a 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -1,6 +1,7 @@ package commands import ( + "errors" "fmt" "os" "path/filepath" @@ -1052,6 +1053,18 @@ You can pass either a line ID or a Basecamp line URL: Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + + // Refuse before any account, project or chat lookup. This command + // confirms interactively unless told otherwise, and + // isNonInteractiveCommand only knows about flags, the env var and + // stdout — it never looks at stdin. An agent in a PTY with stdin on + // /dev/null and no --json lands here, and a prompt reached there + // waits on /dev/tty instead of failing. Failing up front costs it + // nothing; reaching the prompt spent two round trips first. + if err := ensureDeleteConfirmable(cmd, force); err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -1100,13 +1113,20 @@ You can pass either a line ID or a Basecamp line URL: return output.ErrUsage("Invalid line ID") } - // Confirm destructive action in interactive mode + // Confirm destructive action in interactive mode. ensureDeleteConfirmable + // above already rejected the case where the prompt cannot be answered, + // so a failure here is the user canceling. if !force && !isNonInteractiveCommand(cmd) { confirmed, err := tui.ConfirmDangerous("Permanently delete this chat line?") - if err != nil { - return nil //nolint:nilerr // user canceled prompt - } - if !confirmed { + switch { + case errors.Is(err, tui.ErrCanceled): + return nil // the user answered, and the answer was no + case err != nil: + // Not a cancellation — a timeout or a bubbletea failure. + // Nothing is deleted either way, but say what happened + // rather than reporting a decision nobody made. + return fmt.Errorf("confirming the delete: %w", err) + case !confirmed: return nil } } diff --git a/internal/commands/chat_test.go b/internal/commands/chat_test.go index 8631cada2..663772a5c 100644 --- a/internal/commands/chat_test.go +++ b/internal/commands/chat_test.go @@ -1811,3 +1811,45 @@ func TestChatPostRejectsPositionalWithContentFlag(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "cannot combine") } + +// TestChatDeleteRefusesWhenStdinCannotConfirm covers the gap isMachineOutput +// cannot see: it checks flags, the env var and stdout, never stdin. An agent in +// a PTY with stdin on /dev/null and no --json reaches the confirmation prompt, +// which used to block on /dev/tty. It must now fail with a usage error naming +// --force, before it issues a single request. +func TestChatDeleteRefusesWhenStdinCannotConfirm(t *testing.T) { + for _, kind := range []string{"pipe", "devnull"} { + t.Run(kind, func(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + nonInteractiveStdin(t, kind) + + transport := &countingChatTransport{inner: &mockChatDeleteTransport{}} + app, _ := newChatDeleteTestApp(transport) + // No machine-output flag and a *bytes.Buffer stdout, so + // isNonInteractiveCommand is false and the confirm is reached. + + cmd := NewChatCmd() + err := executeChatCommand(cmd, app, "delete", "111") + require.Error(t, err, "delete must not silently succeed on a confirmation nobody can answer") + + outErr := output.AsError(err) + require.NotNil(t, outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Hint, "--force") + + assert.Zero(t, transport.requests, + "the refusal belongs before the account and project lookups, not after them") + }) + } +} + +// countingChatTransport counts every request that reaches the wire. +type countingChatTransport struct { + inner http.RoundTripper + requests int +} + +func (t *countingChatTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.requests++ + return t.inner.RoundTrip(req) +} diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 2854d1023..2600974e7 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -16,6 +16,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/names" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/urlarg" ) @@ -55,6 +56,31 @@ func noChanges(cmd *cobra.Command) error { return cmd.Help() } +// ensureDeleteConfirmable rejects an invocation that will reach a confirmation +// prompt it cannot drive. isNonInteractiveCommand decides whether to prompt at +// all, but it reads flags, the env var and stdout — never stdin. The gap is a +// PTY with stdin redirected, which is how an agent says "I have nothing to +// type": the prompt is not skipped, and bubbletea answers a non-terminal stdin +// by opening /dev/tty and waiting on the real terminal. It asks +// InteractivePrompt rather than InteractiveStdio because the confirmation is a +// huh form, which draws to stderr. +// +// Deliberately narrow: this does not change when a command prompts, only what +// happens when the prompt it already decided to show cannot be answered. +// Widening isNonInteractiveCommand itself would change missingArg and noChanges +// across many commands, which is a separate decision. +func ensureDeleteConfirmable(cmd *cobra.Command, force bool) error { + if force || isNonInteractiveCommand(cmd) || stdinarg.InteractivePrompt() { + return nil + } + // Name the requirement, not one end of it: this also fires when stdin is a + // terminal but stderr is redirected, where the confirmation would be drawn + // somewhere nobody can see it. + return output.ErrUsageHint( + "This deletion needs a confirmation that can't be shown or answered here", + "Confirming needs a terminal on both stdin and stderr. Pass --force to delete without confirming.") +} + // isNonInteractiveCommand returns true when command-level flows should avoid // human prompts or help screens, without implying a machine output format. func isNonInteractiveCommand(cmd *cobra.Command) bool { diff --git a/internal/commands/skill.go b/internal/commands/skill.go index 2ac89ea62..027f3c415 100644 --- a/internal/commands/skill.go +++ b/internal/commands/skill.go @@ -1,7 +1,9 @@ package commands import ( + "errors" "fmt" + "io" "os" "path/filepath" "strings" @@ -152,6 +154,35 @@ func installSkillFiles() (string, error) { } // runSkillWizard runs the interactive skill installation wizard. +// skillPromptFailed decides what a failed prompt means. Exactly one outcome is +// a success: the user was asked and said no. Everything else is a failure and +// has to be reported, because "Installation canceled." plus exit 0 claims an +// answer nobody gave and leaves the caller believing it declined an install it +// never saw. +// +// Three cases, and the default is deliberately *not* cancellation: +// +// - tui.ErrCanceled — the user dismissed the prompt. Report and exit 0. +// - tui.ErrNotInteractive — nothing could be asked. app.IsInteractive() checks +// stdin and stdout, but huh draws to stderr, so `basecamp skill 2>somewhere` +// enters the wizard and only then finds it has nowhere to draw. +// - anything else — a timeout, or a bubbletea or runtime failure. Propagate +// it; guessing that it meant "no" is how a real error disappears. +func skillPromptFailed(w io.Writer, styles *tui.Styles, err error) error { + switch { + case errors.Is(err, tui.ErrCanceled): + fmt.Fprintln(w, styles.Muted.Render(" Installation canceled.")) + return nil + case errors.Is(err, tui.ErrNotInteractive): + return output.ErrUsageHint( + "Can't show the installation prompts here", + "Installing interactively needs a terminal on both stdin and stderr. "+ + "Run basecamp skill install to install without prompts, or basecamp skill to print the file.") + default: + return fmt.Errorf("showing the installation prompts: %w", err) + } +} + func runSkillWizard(cmd *cobra.Command, app *appctx.App) error { w := cmd.OutOrStdout() styles := tui.NewStylesWithTheme(tui.ResolveTheme(tui.DetectDark())) @@ -175,16 +206,18 @@ func runSkillWizard(cmd *cobra.Command, app *appctx.App) error { selectedPath, err := tui.Select(" Where would you like to install the Basecamp skill?", options) if err != nil { - fmt.Fprintln(w, styles.Muted.Render(" Installation canceled.")) - return nil //nolint:nilerr // user canceled prompt + return skillPromptFailed(w, styles, err) } // Handle custom path if selectedPath == "other" { selectedPath, err = tui.Input(" Enter custom path", "/path/to/skills/basecamp/SKILL.md") - if err != nil || selectedPath == "" { + if err != nil { + return skillPromptFailed(w, styles, err) + } + if selectedPath == "" { fmt.Fprintln(w, styles.Muted.Render(" Installation canceled.")) - return nil //nolint:nilerr // user canceled prompt + return nil } selectedPath = normalizeSkillPath(selectedPath) } @@ -195,9 +228,12 @@ func runSkillWizard(cmd *cobra.Command, app *appctx.App) error { if _, statErr := os.Stat(expandedPath); statErr == nil { overwrite, confirmErr := tui.Confirm( fmt.Sprintf(" File already exists at %s. Overwrite?", selectedPath), false) - if confirmErr != nil || !overwrite { + if confirmErr != nil { + return skillPromptFailed(w, styles, confirmErr) + } + if !overwrite { fmt.Fprintln(w, styles.Muted.Render(" Installation canceled.")) - return nil //nolint:nilerr // user canceled or declined + return nil } } else if !os.IsNotExist(statErr) { return fmt.Errorf("checking existing file: %w", statErr) diff --git a/internal/commands/skill_test.go b/internal/commands/skill_test.go index 37de7153c..57c8a7771 100644 --- a/internal/commands/skill_test.go +++ b/internal/commands/skill_test.go @@ -3,6 +3,8 @@ package commands import ( "bytes" "context" + "errors" + "fmt" "os" "path/filepath" "strings" @@ -11,6 +13,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/tui" "github.com/basecamp/basecamp-cli/internal/version" "github.com/basecamp/basecamp-cli/skills" ) @@ -685,3 +689,76 @@ func TestRepairClaudeSkillLink_HealthySymlink(t *testing.T) { targetAfter, _ := os.Readlink(filepath.Join(symlinkDir, "basecamp")) assert.Equal(t, targetBefore, targetAfter, "healthy symlink should not be modified") } + +// TestSkillWizardReportsWhenItCannotPrompt covers the gap between "the user +// said no" and "nobody could be asked". app.IsInteractive() looks at stdin and +// stdout, but huh draws the form to stderr — so `basecamp skill 2>somewhere` +// gets past the interactivity check and only then finds it has nowhere to draw. +// +// Every prompt error used to print "Installation canceled." and exit 0, which +// claims an answer nobody gave, and leaves a caller believing it declined an +// install it never saw. A real cancellation still exits 0; this must not. +func TestSkillWizardReportsWhenItCannotPrompt(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + nonInteractiveStdin(t, "devnull") + + buf := &bytes.Buffer{} + styles := tui.NewStylesWithTheme(tui.ResolveTheme(false)) + + err := skillPromptFailed(buf, styles, tui.ErrNotInteractive) + + require.Error(t, err, "being unable to prompt is not a successful cancellation") + outErr := output.AsError(err) + require.NotNil(t, outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Hint, "basecamp skill install", + "the hint must name the non-interactive path that works") + assert.NotContains(t, buf.String(), "canceled", + "must not claim the user canceled when the user was never asked") +} + +// TestSkillWizardTreatsCancellationAsSuccess is the other half: an actual +// cancellation is an answer, and answering no is not an error. +func TestSkillWizardTreatsCancellationAsSuccess(t *testing.T) { + buf := &bytes.Buffer{} + styles := tui.NewStylesWithTheme(tui.ResolveTheme(false)) + + err := skillPromptFailed(buf, styles, tui.ErrCanceled) + + require.NoError(t, err) + assert.Contains(t, buf.String(), "canceled") +} + +// TestSkillWizardPropagatesRealPromptErrors is the case that makes the other +// two mean anything. Cancellation is the *only* prompt outcome that exits 0, so +// the default has to be propagation. +// +// huh signals a real dismissal with ErrUserAborted and nothing else; a timeout, +// or a bubbletea or runtime failure, arrives as an ordinary error. Treating +// every error as cancellation — which this did — turns any of those into +// "Installation canceled." and exit 0, reporting a decision nobody made. +func TestSkillWizardPropagatesRealPromptErrors(t *testing.T) { + for _, tc := range []struct { + name string + err error + }{ + // huh's own values, spelled out rather than imported: the launcher + // backstop keeps huh to a single import path, and these are plain + // sentinels (form.go:51-57). + {"timeout", errors.New("timeout")}, + {"a wrapped bubbletea failure", fmt.Errorf("huh: %w", errors.New("could not open a new TTY"))}, + {"anything unrecognized", errors.New("boom")}, + } { + t.Run(tc.name, func(t *testing.T) { + buf := &bytes.Buffer{} + styles := tui.NewStylesWithTheme(tui.ResolveTheme(false)) + + err := skillPromptFailed(buf, styles, tc.err) + + require.Error(t, err, "a failure that is not a cancellation must not exit 0") + assert.ErrorIs(t, err, tc.err, "the cause must survive so it can be diagnosed") + assert.NotContains(t, buf.String(), "canceled", + "must not claim the user canceled when the prompt failed") + }) + } +} diff --git a/internal/commands/wizard.go b/internal/commands/wizard.go index 066ec716e..617710494 100644 --- a/internal/commands/wizard.go +++ b/internal/commands/wizard.go @@ -2,6 +2,7 @@ package commands import ( "encoding/json" + "errors" "fmt" "io" "os" @@ -15,20 +16,23 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" "github.com/basecamp/basecamp-cli/internal/tui/resolve" "github.com/basecamp/basecamp-cli/internal/version" ) -// WizardResult holds the outcome of the first-run wizard. +// WizardResult holds the outcome of the first-run wizard, for showSuccess to +// render. It carries no json tags: the wizard only runs interactively, so the +// structured envelope it once emitted is unreachable and nothing serializes +// this. type WizardResult struct { - Version string `json:"version"` - Status string `json:"status"` // "complete" - AccountID string `json:"account_id,omitempty"` - AccountName string `json:"account_name,omitempty"` - ProjectID string `json:"project_id,omitempty"` - ProjectName string `json:"project_name,omitempty"` - ConfigScope string `json:"config_scope,omitempty"` // "global", "local", or "" if skipped + Status string // "complete" or "incomplete" + AccountID string + AccountName string + ProjectID string + ProjectName string + ConfigScope string // "global", "local", or "" if skipped } // NewSetupCmd creates the setup command (explicit wizard invocation). @@ -56,10 +60,22 @@ func runWizard(cmd *cobra.Command, app *appctx.App) error { return fmt.Errorf("app not initialized") } + // --jq keeps its own, more specific error; it is a usage error either way. if app.Flags.JQFilter != "" { return output.ErrJQNotSupported("the setup command") } + // Refuse rather than walk into a prompt nothing can answer. The user asked + // for the wizard by name here, so say so; isFirstRun asks the same question + // and answers it differently — see wizardCanRun. + // + // The gate belongs to this RunE alone: `setup claude`, `setup codex` and + // `setup agents` are the supported non-interactive paths and must keep + // working, which a persistent hook here would have broken. + if !wizardCanRun(app) { + return output.ErrUsageHint("basecamp setup needs an interactive terminal", wizardEscapeHint()) + } + styles := tui.NewStylesWithTheme(tui.ResolveTheme(tui.DetectDark())) // Step 1: Welcome @@ -72,7 +88,7 @@ func runWizard(cmd *cobra.Command, app *appctx.App) error { } // Step 3: Account selection - result := WizardResult{Version: version.Version, Status: "complete"} + result := WizardResult{Status: "complete"} accountID, err := wizardAccount(cmd, app, styles) if err != nil { return err @@ -115,18 +131,23 @@ func runWizard(cmd *cobra.Command, app *appctx.App) error { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to persist onboarding flag: %v\n", err) } - // Step 7: Summary with next steps - // Interactive mode shows the rich checklist directly; non-interactive - // or machine-output mode delegates to app.OK which renders the structured envelope. - if app.IsInteractive() && !app.IsMachineOutput() { - showSuccess(cmd.OutOrStdout(), styles, result, agentOutcome.Checks, agentOutcome.Issues, agentOutcome.Skipped) - return nil - } + // Step 7: Summary with next steps. The gate above already established + // interactive, non-machine output — the wizard cannot run any other way — + // so the rich checklist is the only summary this path renders. The + // structured-envelope branch that used to sit here was reachable only under + // machine output, which the gate now refuses; it and its two helpers went + // with it rather than being kept alive for nobody. + showSuccess(cmd.OutOrStdout(), styles, result, agentOutcome.Checks, agentOutcome.Issues, agentOutcome.Skipped) + return nil +} - return app.OK(result, - output.WithSummary(wizardSummaryLine(result)), - output.WithBreadcrumbs(wizardBreadcrumbs(result)...), - ) +// wizardEscapeHint names the non-interactive paths that cover what the wizard +// would have prompted for, rather than restating that a terminal is missing. +// Modeled on stdinEscapeHint: point at the real alternatives. +func wizardEscapeHint() string { + return "Agent setup runs without a terminal: basecamp setup agents (or basecamp setup claude / basecamp setup codex). " + + "Set the defaults the wizard would ask for with basecamp config set account_id (or basecamp accounts use ) and basecamp config set project_id . " + + "Check authentication with basecamp auth status." } // showWelcome displays the welcome screen with animated logo. @@ -240,13 +261,18 @@ func wizardProject(cmd *cobra.Command, app *appctx.App, styles *tui.Styles) (str fmt.Fprintln(w, styles.Heading.Render(" Step 3: Default Project (optional)")) fmt.Fprintln(w) + // Declining and failing look the same to a caller that only checks for a + // non-nil error, so separate them: a dismissal skips the step, anything else + // is a real failure and says so rather than reporting a choice nobody made. wantProject, err := tui.Confirm(" Set a default project?", true) - if err != nil { - return "", nil //nolint:nilerr // Treat confirm error as skip (user canceled) + if err != nil && !errors.Is(err, tui.ErrCanceled) { + return "", fmt.Errorf("asking about the default project: %w", err) } - if !wantProject { + if err != nil || !wantProject { fmt.Fprintln(w, styles.Muted.Render(" Skipped. Use --project or run: basecamp config project")) fmt.Fprintln(w) + //nolint:nilerr // err here can only be tui.ErrCanceled — the check above + // returned anything else — and a cancellation is an answer, not a failure. return "", nil } @@ -282,6 +308,11 @@ func wizardSaveConfig(w io.Writer, styles *tui.Styles, accountID, projectID stri {Value: "local", Label: "Local (.basecamp/config.json) - this directory only"}, {Value: "skip", Label: "Don't save - I'll use flags each time"}, }) + // No error return here, so a genuine failure is surfaced in place instead of + // being flattened into the same "Skipped." a deliberate choice produces. + if err != nil && !errors.Is(err, tui.ErrCanceled) { + fmt.Fprintln(w, styles.Warning.Render(fmt.Sprintf(" Could not ask where to save: %s", err))) + } if err != nil || scope == "skip" { fmt.Fprintln(w, styles.Muted.Render(" Skipped. Use --account and --project flags.")) fmt.Fprintln(w) @@ -361,7 +392,7 @@ func showSuccess(w io.Writer, styles *tui.Styles, result WizardResult, checks [] fmt.Fprintln(w, styles.RenderStatus(true, fmt.Sprintf("Config saved (%s)", result.ConfigScope))) } if skipped { - fmt.Fprintln(w, styles.Muted.Render(" Coding agent setup skipped — run: basecamp setup")) + fmt.Fprintln(w, styles.Muted.Render(" Coding agent setup skipped — run: basecamp setup agents")) } else { for _, check := range checks { fmt.Fprintln(w, styles.RenderStatus(check.Status == "pass", check.Name)) @@ -447,38 +478,29 @@ func fetchProjectName(cmd *cobra.Command, app *appctx.App, projectID string) str return project.Name } -// wizardSummaryLine builds a concise summary for the output envelope. -func wizardSummaryLine(result WizardResult) string { - headline := "Setup complete" - if result.Status == "incomplete" { - headline = "Setup finished with issues" - } - if result.AccountName != "" { - return fmt.Sprintf("%s - %s", headline, result.AccountName) - } - return headline -} - -// wizardBreadcrumbs returns next-step breadcrumbs based on wizard outcome. -func wizardBreadcrumbs(result WizardResult) []output.Breadcrumb { - crumbs := []output.Breadcrumb{ - {Action: "list_projects", Cmd: "basecamp projects list", Description: "List projects"}, - } - if result.ProjectID != "" { - crumbs = append(crumbs, - output.Breadcrumb{Action: "list_todos", Cmd: "basecamp todos list", Description: "List to-dos"}, - output.Breadcrumb{Action: "search", Cmd: "basecamp search \"query\"", Description: "Search Basecamp"}, - ) - } else { - crumbs = append(crumbs, - output.Breadcrumb{Action: "set_project", Cmd: "basecamp config project", Description: "Set default project"}, - ) - } - return crumbs +// wizardCanRun reports whether the interactive wizard can actually be shown. +// +// The wizard is prompts end to end, and redirecting stdin does not skip one — +// bubbletea falls back to /dev/tty and waits on the real terminal (see +// tui.ErrNotInteractive). Asking a caller who requested machine output to answer +// a question is the same mistake with a terminal attached. Three checks, because +// no one of them sees everything: IsInteractive covers non-terminal +// stdin/stdout, the machine-output flags and the BASECAMP_NONINTERACTIVE escape +// hatch; IsMachineOutput adds the config-driven json/quiet formats it does not +// look at; InteractivePrompt adds stderr, which is where huh actually draws. +// +// Two callers, deliberately different responses. `basecamp setup` was asked for +// by name, so it refuses out loud. Bare `basecamp` never asked for a wizard at +// all, so isFirstRun simply declines to start one and the caller falls through +// to help or the quick-start envelope — answering a question the user did not +// ask with an error about a command they did not type. +func wizardCanRun(app *appctx.App) bool { + return app.IsInteractive() && !app.IsMachineOutput() && stdinarg.InteractivePrompt() } // isFirstRun returns true if this appears to be a first-time run. -// Checks: onboarded flag, stored credentials, BASECAMP_TOKEN env, interactive TTY. +// Checks: onboarded flag, stored credentials, BASECAMP_TOKEN env, and whether +// the wizard could be shown at all. func isFirstRun(app *appctx.App) bool { if app.Config.Onboarded != nil && *app.Config.Onboarded { return false @@ -489,5 +511,5 @@ func isFirstRun(app *appctx.App) bool { if os.Getenv("BASECAMP_TOKEN") != "" { return false } - return app.IsInteractive() + return wizardCanRun(app) } diff --git a/internal/commands/wizard_agents.go b/internal/commands/wizard_agents.go index 27c63c239..d581ddd45 100644 --- a/internal/commands/wizard_agents.go +++ b/internal/commands/wizard_agents.go @@ -303,6 +303,9 @@ func wizardAgents(cmd *cobra.Command, styles *tui.Styles) (agentSetupOutcome, er fmt.Fprintln(w) install, confirmErr := tui.Confirm(" Set up Basecamp for your coding agents?", true) + if confirmErr != nil && !errors.Is(confirmErr, tui.ErrCanceled) { + return agentSetupOutcome{}, fmt.Errorf("asking about agent setup: %w", confirmErr) + } if confirmErr != nil || !install { fmt.Fprintln(w) fmt.Fprintln(w, styles.Muted.Render(" You can set up agents later:")) @@ -314,7 +317,7 @@ func wizardAgents(cmd *cobra.Command, styles *tui.Styles) (agentSetupOutcome, er fmt.Fprintln(w) // Skipped carries the current snapshot for the checklist but records no // issues, so a deliberate skip stays "complete". - return agentSetupOutcome{Skipped: true, Checks: preChecks}, nil //nolint:nilerr // Treat confirm error as skip (user canceled) + return agentSetupOutcome{Skipped: true, Checks: preChecks}, nil } fmt.Fprintln(w) @@ -324,7 +327,9 @@ func wizardAgents(cmd *cobra.Command, styles *tui.Styles) (agentSetupOutcome, er // Install baseline skill (always, for any agent) if _, err := installSkillFiles(); err != nil { fmt.Fprintln(w, styles.Warning.Render(fmt.Sprintf(" Skill install failed: %s", err))) - issues = append(issues, agentIssue{Check: "Agent skill", Hint: "Run: basecamp setup"}) + // Not "basecamp setup": this runs inside it, so that advice is circular. + // `setup agents` retries exactly the step that failed, and needs no terminal. + issues = append(issues, agentIssue{Check: "Agent skill", Hint: "Run: basecamp setup agents"}) } else { fmt.Fprintln(w, styles.RenderStatus(true, "Agent skill installed")) } diff --git a/internal/commands/wizard_test.go b/internal/commands/wizard_test.go index dc14c5669..0fc59accf 100644 --- a/internal/commands/wizard_test.go +++ b/internal/commands/wizard_test.go @@ -6,7 +6,9 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" "testing" + "time" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -46,64 +48,6 @@ func TestIsFirstRunAuthenticated(t *testing.T) { assert.False(t, isFirstRun(app), "isFirstRun should be false when authenticated") } -// TestWizardResultJSON verifies the WizardResult struct serializes correctly. -func TestWizardResultJSON(t *testing.T) { - app, buf := setupQuickstartTestApp(t, "", "") - - result := WizardResult{ - Version: "1.0.0", - Status: "complete", - AccountID: "12345", - AccountName: "Test Company", - ProjectID: "67890", - ProjectName: "My Project", - ConfigScope: "global", - } - - err := app.OK(result, output.WithSummary("Setup complete")) - require.NoError(t, err) - - out := buf.String() - assert.Contains(t, out, `"account_id": "12345"`) - assert.Contains(t, out, `"project_id": "67890"`) - assert.Contains(t, out, `"config_scope": "global"`) -} - -// TestWizardSummaryLine verifies summary generation. -func TestWizardSummaryLine(t *testing.T) { - tests := []struct { - name string - result WizardResult - expected string - }{ - { - name: "with account name", - result: WizardResult{AccountName: "Test Co"}, - expected: "Setup complete - Test Co", - }, - { - name: "without account name", - result: WizardResult{AccountID: "123"}, - expected: "Setup complete", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expected, wizardSummaryLine(tt.result)) - }) - } -} - -// TestWizardSummaryLineIncomplete verifies the summary reflects an unhealthy -// agent-setup outcome instead of claiming completion. -func TestWizardSummaryLineIncomplete(t *testing.T) { - assert.Equal(t, "Setup finished with issues", - wizardSummaryLine(WizardResult{Status: "incomplete"})) - assert.Equal(t, "Setup finished with issues - Test Co", - wizardSummaryLine(WizardResult{Status: "incomplete", AccountName: "Test Co"})) -} - // TestSuccessHeadline verifies the completion banner is honest when the // agent-setup step left unresolved issues. func TestSuccessHeadline(t *testing.T) { @@ -231,40 +175,6 @@ func TestShowSuccessComplete(t *testing.T) { assert.NotContains(t, out, "basecamp doctor") } -// TestWizardBreadcrumbs verifies breadcrumb generation based on wizard outcome. -func TestWizardBreadcrumbs(t *testing.T) { - t.Run("with project", func(t *testing.T) { - result := WizardResult{ProjectID: "123"} - crumbs := wizardBreadcrumbs(result) - - assert.True(t, len(crumbs) >= 2) - assert.Equal(t, "list_projects", crumbs[0].Action) - - // Should have todos breadcrumb when project is set - var hasTodos bool - for _, c := range crumbs { - if c.Action == "list_todos" { - hasTodos = true - } - } - assert.True(t, hasTodos, "expected list_todos breadcrumb when project is set") - }) - - t.Run("without project", func(t *testing.T) { - result := WizardResult{} - crumbs := wizardBreadcrumbs(result) - - // Should suggest setting a project - var hasSetProject bool - for _, c := range crumbs { - if c.Action == "set_project" { - hasSetProject = true - } - } - assert.True(t, hasSetProject, "expected set_project breadcrumb when no project") - }) -} - // TestIsFirstRunOnboarded verifies isFirstRun returns false when onboarded flag is set. func TestIsFirstRunOnboarded(t *testing.T) { app, _ := setupQuickstartTestApp(t, "", "") @@ -702,3 +612,350 @@ func TestJoinNames(t *testing.T) { assert.Equal(t, "Claude Code and Cursor", joinNames([]string{"Claude Code", "Cursor"})) assert.Equal(t, "A, B, and C", joinNames([]string{"A", "B", "C"})) } + +// terminalOutputs points both os.Stdout and os.Stderr at a pseudo-terminal so +// stdin is the only thing left disqualifying a prompt. Both are needed: the +// setup gate asks IsInteractive (stdin+stdout) and InteractivePrompt +// (stdin+stderr), and go test pipes both — so without this the assertions below +// would hold for any stdin at all and prove nothing. +// +// Best-effort: where no pty is available the test still runs, less specifically. +func terminalOutputs(t *testing.T) { + t.Helper() + + for _, stream := range []**os.File{&os.Stdout, &os.Stderr} { + if runtime.GOOS == "windows" { + return + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + return + } + + orig, target := *stream, stream + *stream = pty + t.Cleanup(func() { + *target = orig + _ = pty.Close() + }) + } +} + +// nonInteractiveStdin points os.Stdin at the named non-terminal for the +// duration of the test, so the assertions hold no matter how the test binary +// was invoked — running it straight from a terminal would otherwise leave stdin +// a TTY and prove nothing. "devnull" is the case that used to slip through: a +// character device that is not a terminal. +func nonInteractiveStdin(t *testing.T, kind string) { + t.Helper() + + terminalOutputs(t) + + var replacement *os.File + switch kind { + case "pipe": + r, w, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { _ = w.Close() }) + replacement = r + case "devnull": + f, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + require.NoError(t, err) + replacement = f + default: + t.Fatalf("unknown stdin kind %q", kind) + } + + orig := os.Stdin + os.Stdin = replacement + t.Cleanup(func() { + os.Stdin = orig + _ = replacement.Close() + }) +} + +// runSetupWithin executes the setup command and fails if it has not returned +// within the timeout. The timeout is the real assertion: an ungated wizard +// reaches a huh prompt, which blocks on /dev/tty rather than failing. +func runSetupWithin(t *testing.T, cmd *cobra.Command, timeout time.Duration) error { + t.Helper() + + done := make(chan error, 1) + go func() { done <- cmd.Execute() }() + + select { + case err := <-done: + return err + case <-time.After(timeout): + t.Fatal("setup blocked instead of returning; it reached a prompt it cannot drive") + return nil + } +} + +// TestSetupRefusesNonInteractiveStdio covers the hang this gate exists for: +// `basecamp setup` off a terminal used to walk into a huh prompt, which falls +// back to /dev/tty instead of failing. It must return a usage error instead, +// and the hint must name a path that actually works without a terminal. +func TestSetupRefusesNonInteractiveStdio(t *testing.T) { + for _, tc := range []struct { + name string + json bool + kind string + }{ + {"plain/pipe", false, "pipe"}, + {"plain/devnull", false, "devnull"}, + {"json/pipe", true, "pipe"}, + {"json/devnull", true, "devnull"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + nonInteractiveStdin(t, tc.kind) + + app, _ := setupQuickstartTestApp(t, "", "") + app.Flags.JSON = tc.json + + cmd := NewSetupCmd() + cmd.SetArgs(nil) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := runSetupWithin(t, cmd, 10*time.Second) + require.Error(t, err) + + outErr := output.AsError(err) + require.NotNil(t, outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Hint, "basecamp setup agents", + "the hint must name a non-interactive alternative, not just restate the problem") + }) + } +} + +// TestSetupRefusesUnderNonInteractiveEnv verifies BASECAMP_NONINTERACTIVE is +// honored even where stdio would pass: the wizard is prompts end to end, so +// the env var that means "never prompt me" has to reach it. +func TestSetupRefusesUnderNonInteractiveEnv(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("BASECAMP_NONINTERACTIVE", "1") + + app, _ := setupQuickstartTestApp(t, "", "") + + cmd := NewSetupCmd() + cmd.SetArgs(nil) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := runSetupWithin(t, cmd, 10*time.Second) + require.Error(t, err) + assert.Equal(t, output.CodeUsage, output.AsError(err).Code) +} + +// TestSetupRefusesMachineOutputOnATerminal covers the other half of the gate. +// Terminal stdio is not enough: a caller that asked for machine output has +// declared it is not there to answer questions, and the wizard is nothing but +// questions. Config-driven json/quiet counts too — app.IsInteractive() does not +// look at it, which is why the gate also asks IsMachineOutput(). +func TestSetupRefusesMachineOutputOnATerminal(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + t.Skipf("open /dev/ptmx: %v", err) + } + origOut, origIn, origErr := os.Stdout, os.Stdin, os.Stderr + os.Stdout, os.Stdin, os.Stderr = pty, pty, pty + t.Cleanup(func() { + os.Stdout, os.Stdin, os.Stderr = origOut, origIn, origErr + pty.Close() + }) + + for _, tc := range []struct { + name string + apply func(*appctx.App) + }{ + {"json flag", func(a *appctx.App) { a.Flags.JSON = true }}, + {"agent flag", func(a *appctx.App) { a.Flags.Agent = true }}, + {"quiet flag", func(a *appctx.App) { a.Flags.Quiet = true }}, + {"config format json", func(a *appctx.App) { a.Config.Format = "json" }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + app, _ := setupQuickstartTestApp(t, "", "") + tc.apply(app) + + cmd := NewSetupCmd() + cmd.SetArgs(nil) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := runSetupWithin(t, cmd, 10*time.Second) + require.Error(t, err) + assert.Equal(t, output.CodeUsage, output.AsError(err).Code) + assert.Contains(t, output.AsError(err).Hint, "basecamp setup agents") + }) + } +} + +// TestSetupSubcommandsSurviveTheGate is the other half of the gate: it belongs +// to the parent's RunE only. `setup agents`, `setup claude` and `setup codex` +// are the supported non-interactive paths and must keep working off a terminal +// — a persistent hook here would have broken all three. +func TestSetupSubcommandsSurviveTheGate(t *testing.T) { + for _, sub := range []string{"agents", "claude", "codex"} { + t.Run(sub, func(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("HOME", t.TempDir()) + t.Setenv("PATH", t.TempDir()) // no agent binaries + t.Setenv("BASECAMP_SETUP_AGENT", "none") + nonInteractiveStdin(t, "devnull") + + app, _ := setupQuickstartTestApp(t, "", "") + app.Flags.JSON = true + + cmd := NewSetupCmd() + cmd.SetArgs([]string{sub}) // --json is a root persistent flag; app.Flags carries it here + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := runSetupWithin(t, cmd, 10*time.Second) + require.NoError(t, err, "setup %s must still run without a terminal", sub) + }) + } +} + +// TestBareBasecampNeverReportsASetupError covers the difference between asking +// for the wizard and being handed one. `basecamp setup` refuses out loud when it +// cannot prompt — the user named that command. Bare `basecamp` never asked for a +// wizard, so an error about `basecamp setup` is an answer to a question nobody +// posed, and it replaces output the user was entitled to. +// +// Both rows reach runWizard through RunQuickStartDefault's first-run path and +// would have hit the gate, because isFirstRun's old check saw only stdin and +// stdout: +// +// - stderr redirected: stdin/stdout are terminals, so first-run fires, but +// huh draws to stderr and could not have shown anything. +// - config-driven json: IsInteractive does not read Config.Format, so +// first-run fires, while quickstart.go documents this shape as preserving +// the quick-start envelope. +func TestBareBasecampNeverReportsASetupError(t *testing.T) { + for _, tc := range []struct { + name string + apply func(t *testing.T, app *appctx.App) + }{ + { + name: "stderr redirected on a terminal", + apply: func(t *testing.T, _ *appctx.App) { + terminalStdio(t) // stdin+stdout+stderr all terminals... + redirectStderrToPipe(t) // ...then take away the one huh draws to + }, + }, + { + name: "config-driven json output", + apply: func(t *testing.T, app *appctx.App) { + terminalStdio(t) + app.Config.Format = "json" + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + app, _ := setupQuickstartTestApp(t, "", "") + tc.apply(t, app) + + // Without this the test proves nothing: isFirstRun bails on + // IsInteractive before it ever reaches the wizard, and every + // assertion below passes for the wrong reason. An earlier version + // of this test did exactly that — it set stdout and stderr but left + // stdin on go test's /dev/null, so it passed against the bug. + require.True(t, app.IsInteractive(), + "precondition: stdin and stdout must look interactive, or first-run never fires") + + cmd := NewQuickStartCmd() + cmd.SetArgs(nil) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + done := make(chan error, 1) + go func() { done <- RunQuickStartDefault(cmd, nil) }() + + select { + case err := <-done: + if err != nil { + assert.NotContains(t, err.Error(), "basecamp setup", + "bare basecamp must not fail with an error about a command the user never typed") + } + case <-time.After(10 * time.Second): + t.Fatal("bare basecamp blocked; it reached a prompt it cannot drive") + } + }) + } +} + +// TestExplicitSetupStillRefuses is the other side of the same predicate: naming +// the command still gets the usage error, in exactly the contexts where bare +// `basecamp` must not. +func TestExplicitSetupStillRefuses(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + terminalStdio(t) + redirectStderrToPipe(t) + + app, _ := setupQuickstartTestApp(t, "", "") + require.True(t, app.IsInteractive(), + "precondition: only stderr should be disqualifying here") + + cmd := NewSetupCmd() + cmd.SetArgs(nil) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := runSetupWithin(t, cmd, 10*time.Second) + require.Error(t, err) + assert.Equal(t, output.CodeUsage, output.AsError(err).Code) +} + +// terminalStdio points stdin, stdout and stderr at pseudo-terminals, so an +// invocation looks fully interactive before a test takes one of them away. +func terminalStdio(t *testing.T) { + t.Helper() + + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + t.Skipf("open /dev/ptmx: %v", err) + } + origIn, origOut, origErr := os.Stdin, os.Stdout, os.Stderr + os.Stdin, os.Stdout, os.Stderr = pty, pty, pty + t.Cleanup(func() { + os.Stdin, os.Stdout, os.Stderr = origIn, origOut, origErr + pty.Close() + }) +} + +// redirectStderrToPipe makes stderr a pipe — huh's render target, so a prompt +// could not be seen even though stdin and stdout are terminals. +func redirectStderrToPipe(t *testing.T) { + t.Helper() + + r, w, err := os.Pipe() + require.NoError(t, err) + + orig := os.Stderr + os.Stderr = w + t.Cleanup(func() { + os.Stderr = orig + _ = r.Close() + _ = w.Close() + }) +} diff --git a/internal/stdinarg/stdinarg.go b/internal/stdinarg/stdinarg.go index 193643bc8..2666f1310 100644 --- a/internal/stdinarg/stdinarg.go +++ b/internal/stdinarg/stdinarg.go @@ -12,6 +12,8 @@ import ( "os" "strconv" "strings" + + "github.com/charmbracelet/x/term" ) // AnnotationAllowDash is the cmd.Annotations key marking where a command @@ -92,18 +94,45 @@ func IsPiped(r io.Reader) bool { return fi.Mode()&os.ModeCharDevice == 0 } -// InteractiveStdio reports whether both stdout and stdin are character -// devices — the floor for launching anything that draws to the terminal and -// reads keystrokes. A TUI (picker, wizard) reads key events from stdin, so a -// pipe or redirected file can never drive one — and when the command is -// consuming piped content (a "-" stdin input), a TUI would eat that content -// as key events. +// InteractiveStdio reports whether both stdout and stdin are terminals — the +// floor for launching anything that draws to the terminal and reads +// keystrokes. A TUI (picker, wizard) reads key events from stdin, so a pipe or +// redirected file can never drive one — and when the command is consuming +// piped content (a "-" stdin input), a TUI would eat that content as key +// events. +// +// This asks term.IsTerminal, not whether the file is a character device. The +// two differ on exactly the case that matters: /dev/null is a character device +// that delivers no keystrokes, and `cmd < /dev/null` from a terminal session is +// how an agent says "I have nothing to type". Bubble Tea agrees — it tests +// the same term.IsTerminal, and when stdin fails that test it does not error, it opens +// /dev/tty and waits on the real terminal instead. Calling /dev/null +// interactive is therefore a hang, not a cosmetic mismatch. +// +// IsPiped above deliberately keeps the character-device test: it answers a +// different question (is there content on stdin to read?), and reading +// /dev/null correctly yields nothing. func InteractiveStdio() bool { - for _, f := range []*os.File{os.Stdout, os.Stdin} { - fi, err := f.Stat() - if err != nil || fi.Mode()&os.ModeCharDevice == 0 { - return false - } - } - return true + return isTerminal(os.Stdin) && isTerminal(os.Stdout) +} + +// InteractivePrompt reports whether stdin and stderr are terminals — the floor +// for a huh form specifically, because huh draws the form to stderr rather than +// stdout (huh form.go:112 passes tea.WithOutput(os.Stderr)), while a bare +// bubbletea program such as the picker draws to stdout. +// +// The distinction is not pedantry. Checking stdout for a form that renders to +// stderr means `cmd 2>somewhere` draws the prompt into the void while still +// reading /dev/tty: an invisible question blocking a terminal. Ask about the +// stream the launcher actually writes to. +func InteractivePrompt() bool { + return isTerminal(os.Stdin) && isTerminal(os.Stderr) +} + +// isTerminal uses charmbracelet/x/term, the same package Bubble Tea asks — +// both v1 (tea.go:25) and v2 (tea.go:34) import it — so this floor and the +// /dev/tty fallback it exists to prevent cannot disagree about what a terminal +// is. +func isTerminal(f *os.File) bool { + return term.IsTerminal(f.Fd()) } diff --git a/internal/stdinarg/stdinarg_test.go b/internal/stdinarg/stdinarg_test.go index d80c06579..78d935a7d 100644 --- a/internal/stdinarg/stdinarg_test.go +++ b/internal/stdinarg/stdinarg_test.go @@ -2,6 +2,7 @@ package stdinarg import ( "os" + "runtime" "strings" "testing" @@ -66,40 +67,97 @@ func TestIsPipedRegularFile(t *testing.T) { assert.True(t, IsPiped(f)) } -// TestInteractiveStdio proves TUIs are gated off when stdin is piped: a -// wizard or picker reads keystrokes from stdin, so piped stdin would be -// consumed as key events — including piped content meant for a "-" input. -func TestInteractiveStdio(t *testing.T) { - devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) +// openPTY returns the master side of a new pseudo-terminal — the only thing +// term.IsTerminal accepts. /dev/null will not stand in: it is a character +// device but not a terminal, and that gap is what these predicates exist to +// close. +func openPTY(t *testing.T) *os.File { + t.Helper() + + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + f, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) if err != nil { - t.Fatalf("open %s: %v", os.DevNull, err) + t.Skipf("open /dev/ptmx: %v", err) } + t.Cleanup(func() { _ = f.Close() }) + return f +} + +// stdioMatrix exercises a predicate over each of its two endpoints. The +// terminal/terminal row is the load-bearing one: without a passing positive +// baseline every other row would hold for a predicate hardwired to false, and +// the test would bless a floor that refuses everything. +// +// pipe and /dev/null are the two ways a stream arrives without a human. Both +// have to fail, and /dev/null is the one that used to pass: a character device +// that is not a terminal. Bubble Tea does not error on one, it opens /dev/tty +// and waits on the real terminal — so classifying it interactive is a hang. +func stdioMatrix(t *testing.T, name string, predicate func() bool, first, second **os.File) { + t.Helper() + + pty := openPTY(t) + + devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + require.NoError(t, err) defer devnull.Close() pipeR, pipeW, err := os.Pipe() - if err != nil { - t.Fatalf("os.Pipe: %v", err) - } + require.NoError(t, err) defer pipeR.Close() defer pipeW.Close() - origOut, origIn := os.Stdout, os.Stdin - t.Cleanup(func() { os.Stdout, os.Stdin = origOut, origIn }) - - // /dev/null is a character device, standing in for a terminal on both - // ends without needing a PTY. - os.Stdout, os.Stdin = devnull, devnull - if !InteractiveStdio() { - t.Fatal("expected interactive with char-device stdout and stdin") + origFirst, origSecond := *first, *second + t.Cleanup(func() { *first, *second = origFirst, origSecond }) + + for _, tc := range []struct { + label string + first *os.File + second *os.File + interactive bool + }{ + {"terminal/terminal", pty, pty, true}, + {"pipe/terminal", pipeR, pty, false}, + {"terminal/pipe", pty, pipeW, false}, + {"devnull/terminal", devnull, pty, false}, + {"terminal/devnull", pty, devnull, false}, + } { + *first, *second = tc.first, tc.second + assert.Equal(t, tc.interactive, predicate(), "%s with %s", name, tc.label) } +} - os.Stdin = pipeR - if InteractiveStdio() { - t.Fatal("expected non-interactive with piped stdin") - } +// TestInteractiveStdio covers the picker's pair: a bare bubbletea program reads +// keystrokes from stdin and draws to stdout. +func TestInteractiveStdio(t *testing.T) { + stdioMatrix(t, "InteractiveStdio", InteractiveStdio, &os.Stdin, &os.Stdout) +} - os.Stdout, os.Stdin = pipeW, devnull - if InteractiveStdio() { - t.Fatal("expected non-interactive with piped stdout") - } +// TestInteractivePrompt covers huh's pair. huh draws the form to stderr +// (form.go:112 passes tea.WithOutput(os.Stderr)), so asking about stdout 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. +func TestInteractivePrompt(t *testing.T) { + stdioMatrix(t, "InteractivePrompt", InteractivePrompt, &os.Stdin, &os.Stderr) +} + +// TestPredicatesDisagreeOnTheStreamTheyAskAbout pins the reason there are two: +// a terminal stdin with a piped stdout and a terminal stderr is exactly the +// shape where a form works and a picker does not. +func TestPredicatesDisagreeOnTheStreamTheyAskAbout(t *testing.T) { + pty := openPTY(t) + + pipeR, pipeW, err := os.Pipe() + require.NoError(t, err) + defer pipeR.Close() + defer pipeW.Close() + + origIn, origOut, origErr := os.Stdin, os.Stdout, os.Stderr + t.Cleanup(func() { os.Stdin, os.Stdout, os.Stderr = origIn, origOut, origErr }) + + os.Stdin, os.Stdout, os.Stderr = pty, pipeW, pty + + assert.False(t, InteractiveStdio(), "a piped stdout is no place for a picker") + assert.True(t, InteractivePrompt(), "but a form draws to stderr, which is still a terminal") } diff --git a/internal/tui/forms.go b/internal/tui/forms.go index 927c91ec6..6ff106e6f 100644 --- a/internal/tui/forms.go +++ b/internal/tui/forms.go @@ -2,11 +2,56 @@ package tui import ( "errors" + "os" "github.com/charmbracelet/bubbles/key" "github.com/charmbracelet/huh" + + "github.com/basecamp/basecamp-cli/internal/stdinarg" ) +// ErrNotInteractive is returned instead of launching a form when stdio cannot +// drive one. huh runs the form as a bubbletea program, and redirecting stdin +// does not make that program fail: bubbletea v1 sees a non-terminal stdin and +// silently opens /dev/tty instead (tea.go:590-613), so the prompt sits waiting +// on the real terminal — a hang for a caller that redirected stdin precisely +// because nobody is there to type. Where there is no controlling terminal it +// fails instead, but only partway through, after the earlier steps have run. +// Neither outcome is usable, so refuse rather than launch. +// +// The floor lives here rather than at the call sites because a call-site audit +// is exactly what missed `basecamp setup`: huh calls tea.NewProgram inside +// form.go, so grepping for the launcher cannot see these functions at all. +// Gating the constructor bounds where a prompt can be reached, and covers the +// prompts nobody has written yet. +var ErrNotInteractive = errors.New("not an interactive terminal") + +// ErrCanceled is returned when the user dismissed a prompt — Escape or Ctrl+C. +// It is an answer, and callers are right to treat it as "no" rather than as a +// failure. +// +// It exists so they can do that *precisely*. huh returns ErrUserAborted only +// for a real dismissal; a timeout, or a bubbletea or runtime failure, comes back +// as an ordinary error. Callers that treat every prompt error as cancellation +// therefore report an answer nobody gave, and exit 0 having done nothing — the +// same mistake as claiming an install was canceled when the prompt never +// appeared. Translate once here so no caller has to know huh's error values. +var ErrCanceled = errors.New("prompt canceled") + +// canPrompt reports whether stdio can drive a huh form: stdin must be a +// terminal to deliver keystrokes, and stderr must be one because that is where +// huh draws. A character device is not enough, and stdout is not the stream to +// ask about — see stdinarg.InteractivePrompt. +func canPrompt() bool { + return stdinarg.InteractivePrompt() +} + +// canPick reports whether stdio can drive a bare bubbletea program. The picker +// draws to stdout, not stderr, so it asks a different pair than canPrompt. +func canPick() bool { + return stdinarg.InteractiveStdio() +} + // escKeyMap returns a keymap where both Ctrl+C and Escape abort the form. func escKeyMap() *huh.KeyMap { km := huh.NewDefaultKeyMap() @@ -14,6 +59,42 @@ func escKeyMap() *huh.KeyMap { return km } +// runForm is the one place in this package a huh form is executed, and so the +// one place the floor has to hold. Every exported prompt below funnels through +// it. TestNoUnsanctionedLaunchers keeps it that way — it fails on a .Run() in +// this file outside runForm, and on a huh import or a tea.NewProgram anywhere +// outside the handful of sanctioned files — because widening a call-site audit +// is exactly what let `basecamp setup` slip through. +func runForm(form *huh.Form) error { + if !canPrompt() { + return ErrNotInteractive + } + // Pin the output stream rather than inheriting huh's, which is not one + // stream. NewForm defaults to stderr, but it silently switches the whole + // form to accessible mode when TERM=dumb (form.go:124), and that mode writes + // to f.output-or-stdout instead (form.go:670-673). A floor that asks about + // stderr while huh draws to stdout is wrong in both directions: it refuses a + // prompt that would have rendered fine, and permits one nobody can see. + // Setting it here makes the answer the same in both modes, so canPrompt and + // huh cannot disagree. + // + // Only the output. Passing WithInput would set bubbletea's inputType to + // customInput and so disable its /dev/tty fallback — which sounds desirable + // until you read tty.go:100, where a swallowed io.EOF means the program + // hangs instead of quitting. The floor keeps a non-terminal stdin from + // reaching here at all. + err := form.WithOutput(os.Stderr).WithKeyMap(escKeyMap()).Run() + if errors.Is(err, huh.ErrUserAborted) { + return ErrCanceled + } + return err +} + +// runFields runs a single-group form over the given fields. +func runFields(fields ...huh.Field) error { + return runForm(huh.NewForm(huh.NewGroup(fields...))) +} + // Confirm shows a yes/no confirmation prompt. Escape or Ctrl+C cancels. func Confirm(message string, defaultValue bool) (bool, error) { var result bool @@ -23,10 +104,7 @@ func Confirm(message string, defaultValue bool) (bool, error) { Negative("No"). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - if err != nil { + if err := runFields(field); err != nil { return defaultValue, err } return result, nil @@ -42,10 +120,7 @@ func ConfirmDangerous(message string) (bool, error) { Negative("Cancel"). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - if err != nil { + if err := runFields(field); err != nil { return false, err } return result, nil @@ -59,10 +134,7 @@ func Input(title, placeholder string) (string, error) { Placeholder(placeholder). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - return result, err + return result, runFields(field) } // InputRequired shows a required text input prompt. Escape or Ctrl+C cancels. @@ -79,10 +151,7 @@ func InputRequired(title, placeholder string) (string, error) { return nil }) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - return result, err + return result, runFields(field) } // TextArea shows a multiline text input prompt. Escape or Ctrl+C cancels. @@ -93,10 +162,7 @@ func TextArea(title, placeholder string) (string, error) { Placeholder(placeholder). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - return result, err + return result, runFields(field) } // SelectOption represents an option in a select prompt. @@ -118,10 +184,7 @@ func Select(title string, options []SelectOption) (string, error) { Options(huhOptions...). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - return result, err + return result, runFields(field) } // SelectWithDescription shows a select prompt with descriptions. Escape or Ctrl+C cancels. @@ -138,10 +201,7 @@ func SelectWithDescription(title, description string, options []SelectOption) (s Options(huhOptions...). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - return result, err + return result, runFields(field) } // MultiSelect shows a multi-select prompt. Escape or Ctrl+C cancels. @@ -157,10 +217,7 @@ func MultiSelect(title string, options []SelectOption) ([]string, error) { Options(huhOptions...). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - return result, err + return result, runFields(field) } // FormField represents a field in a form. @@ -199,11 +256,8 @@ func Form(title string, fields []FormField) (map[string]string, error) { huhFields[i] = input } - form := huh.NewForm( - huh.NewGroup(huhFields...).Title(title), - ).WithKeyMap(escKeyMap()) - - if err := form.Run(); err != nil { + form := huh.NewForm(huh.NewGroup(huhFields...).Title(title)) + if err := runForm(form); err != nil { return nil, err } @@ -214,12 +268,12 @@ func Form(title string, fields []FormField) (map[string]string, error) { return results, nil } -// Note shows an informational note (non-interactive). +// Note shows an informational note. It takes no input, but huh still runs it as +// a bubbletea program reading os.Stdin, so it needs the same floor. func Note(title, body string) error { - return huh.NewNote(). + return runFields(huh.NewNote(). Title(title). - Description(body). - Run() + Description(body)) } // ConfirmSetDefault asks the user if they want to save a value as the default. Escape or Ctrl+C cancels. @@ -232,16 +286,16 @@ func ConfirmSetDefault(valueName string) (bool, error) { Negative("No"). Value(&result) - err := huh.NewForm(huh.NewGroup(field)). - WithKeyMap(escKeyMap()). - Run() - if err != nil { + if err := runFields(field); err != nil { return false, err } return result, nil } // SelectScope shows a prompt for selecting the config scope (global or local). +// It inherits Select's floor, so it returns ErrNotInteractive off a terminal. +// +//nolint:gocritic // delegates deliberately; the floor lives in runForm func SelectScope() (string, error) { options := []SelectOption{ {Value: "local", Label: "Local (.basecamp/config.json)"}, diff --git a/internal/tui/forms_test.go b/internal/tui/forms_test.go new file mode 100644 index 000000000..1315c81ba --- /dev/null +++ b/internal/tui/forms_test.go @@ -0,0 +1,341 @@ +package tui + +import ( + "context" + "errors" + "go/ast" + "go/parser" + "go/token" + "os" + "runtime" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// stdinKinds are the two ways stdin arrives without a human behind it. Both +// have to refuse, and /dev/null is the one that used to slip through: it is a +// character device, so the older interactivity test called it a terminal. +var stdinKinds = []string{"pipe", "devnull"} + +// The two launchers in this package draw to different streams, so the tests +// have to hold different things constant. A huh form renders to stderr +// (huh form.go:112), a bare bubbletea program to stdout. Point the launcher's +// own output stream at a pty so stdin is the only thing left disqualifying it — +// otherwise go test's piped stdout and stderr fail the check on their own, and +// the stdin cases below prove nothing at all. +// +// Best-effort: where no pty is available the test still runs, less specifically. +func terminalStdout(t *testing.T) { + t.Helper() + swapForPTY(t, &os.Stdout) +} + +// terminalStderr is terminalStdout's counterpart for huh, which draws there. +func terminalStderr(t *testing.T) { + t.Helper() + swapForPTY(t, &os.Stderr) +} + +func swapForPTY(t *testing.T, stream **os.File) { + t.Helper() + + if runtime.GOOS == "windows" { + return + } + pty, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + return + } + + orig := *stream + *stream = pty + t.Cleanup(func() { + *stream = orig + _ = pty.Close() + }) +} + +// nonInteractiveStdin points os.Stdin at the named non-terminal for the +// duration of the test, so the assertions hold no matter how the test binary +// was invoked — running it straight from a terminal would otherwise leave +// stdin a TTY and prove nothing. The caller points the relevant output stream +// at a pty first. +func nonInteractiveStdin(t *testing.T, kind string) { + t.Helper() + + var replacement *os.File + switch kind { + case "pipe": + r, w, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { _ = w.Close() }) + replacement = r + case "devnull": + f, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + require.NoError(t, err) + replacement = f + default: + t.Fatalf("unknown stdin kind %q", kind) + } + + orig := os.Stdin + os.Stdin = replacement + t.Cleanup(func() { + os.Stdin = orig + _ = replacement.Close() + }) +} + +// promptFloor is one exported prompt in forms.go and a call that must refuse. +// The name has to match the function's declared name: TestPromptFloorCovers +// walks forms.go and fails if an exported function is missing from this table, +// so a prompt added without the floor cannot pass unnoticed. +type promptFloor struct { + name string + call func() error +} + +func promptFloors() []promptFloor { + opts := []SelectOption{{Value: "a", Label: "A"}} + + return []promptFloor{ + {"Confirm", func() error { _, err := Confirm("q", true); return err }}, + {"ConfirmDangerous", func() error { _, err := ConfirmDangerous("q"); return err }}, + {"Input", func() error { _, err := Input("t", "p"); return err }}, + {"InputRequired", func() error { _, err := InputRequired("t", "p"); return err }}, + {"TextArea", func() error { _, err := TextArea("t", "p"); return err }}, + {"Select", func() error { _, err := Select("t", opts); return err }}, + {"SelectWithDescription", func() error { _, err := SelectWithDescription("t", "d", opts); return err }}, + {"MultiSelect", func() error { _, err := MultiSelect("t", opts); return err }}, + {"Form", func() error { _, err := Form("t", []FormField{{Key: "k", Title: "T"}}); return err }}, + {"Note", func() error { return Note("t", "b") }}, + {"ConfirmSetDefault", func() error { _, err := ConfirmSetDefault("account_id"); return err }}, + {"SelectScope", func() error { _, err := SelectScope(); return err }}, + } +} + +// TestPromptFloorRefusesNonInteractiveStdio is the anti-hang assertion: every +// prompt returns ErrNotInteractive rather than launching a bubbletea program. +// A prompt that skips the floor fails one of the two assertions depending on +// the environment — it blocks on /dev/tty where there is a controlling +// terminal (the timeout catches that), and returns a huh TTY error where there +// is not (the errors.Is catches that). +func TestPromptFloorRefusesNonInteractiveStdio(t *testing.T) { + for _, floor := range promptFloors() { + for _, kind := range stdinKinds { + for _, term := range []string{"xterm-256color", "dumb"} { + t.Run(floor.name+"/"+kind+"/TERM="+term, func(t *testing.T) { + // TERM=dumb makes huh switch the form to accessible mode, + // which renders through a different path entirely and never + // starts a bubbletea program. The floor has to hold in both, + // so it is asserted in both. + t.Setenv("TERM", term) + terminalStderr(t) // huh draws here; stdin is the variable under test + nonInteractiveStdin(t, kind) + + done := make(chan error, 1) + go func() { done <- floor.call() }() + + select { + case err := <-done: + assert.True(t, errors.Is(err, ErrNotInteractive), + "%s should return ErrNotInteractive on %s stdin (TERM=%s), got %v", + floor.name, kind, term, err) + case <-time.After(5 * time.Second): + t.Fatalf("%s blocked on %s stdin (TERM=%s) instead of refusing", + floor.name, kind, term) + } + }) + } + } + } +} + +// TestPickerFloorRefusesNonInteractiveStdio covers the other bubbletea +// launcher in this package. Every current picker call site gates on +// resolve.Resolver.IsInteractive first; this makes the next one safe anyway. +// +// The loader case also asserts the loader never ran. A refusal that still +// fetched would have paid for a round trip nobody can act on, and would leave +// the caller unable to tell a refusal from a failed fetch. +func TestPickerFloorRefusesNonInteractiveStdio(t *testing.T) { + items := []PickerItem{{ID: "1", Title: "One"}} + + var loaderCalls int + loader := func() ([]PickerItem, error) { + loaderCalls++ + return items, nil + } + + for _, tc := range []struct { + name string + picker *Picker + }{ + {"items", NewPicker(items)}, + {"loader", NewPickerWithLoader(loader)}, + } { + for _, kind := range stdinKinds { + t.Run(tc.name+"/"+kind, func(t *testing.T) { + terminalStdout(t) // the picker draws here; stdin is the variable under test + nonInteractiveStdin(t, kind) + + done := make(chan error, 1) + go func() { + _, err := tc.picker.Run() + done <- err + }() + + select { + case err := <-done: + assert.True(t, errors.Is(err, ErrNotInteractive), + "picker should return ErrNotInteractive on %s stdin, got %v", kind, err) + case <-time.After(5 * time.Second): + t.Fatalf("picker blocked on %s stdin instead of refusing", kind) + } + }) + } + } + + assert.Zero(t, loaderCalls, "a refused picker must not fetch items it cannot show") +} + +// TestDeadLaunchersStillRefuse covers Spinner and PaginatedPicker. Both are +// exported, both have zero callers today, and both are slated for deletion — +// but "nobody calls it" is not a gate. Adding a caller would not add a +// tea.NewProgram, so the structural backstop would still pass while the hang +// came straight back. They hold the floor until they are gone. +func TestDeadLaunchersStillRefuse(t *testing.T) { + for _, kind := range stdinKinds { + t.Run("spinner/"+kind, func(t *testing.T) { + terminalStdout(t) + nonInteractiveStdin(t, kind) + + var ran bool + done := make(chan error, 1) + go func() { + _, err := NewSpinner("working").Run(func() (string, error) { + ran = true + return "", nil + }) + done <- err + }() + + select { + case err := <-done: + assert.True(t, errors.Is(err, ErrNotInteractive), + "spinner should return ErrNotInteractive on %s stdin, got %v", kind, err) + assert.False(t, ran, "a refused spinner must not run the work it was wrapping") + case <-time.After(5 * time.Second): + t.Fatalf("spinner blocked on %s stdin instead of refusing", kind) + } + }) + + t.Run("paginated_picker/"+kind, func(t *testing.T) { + terminalStdout(t) + nonInteractiveStdin(t, kind) + + var fetched bool + fetcher := func(context.Context, string) (*PageResult, error) { + fetched = true + return &PageResult{}, nil + } + + done := make(chan error, 1) + go func() { + _, err := NewPaginatedPicker(context.Background(), fetcher).Run() + done <- err + }() + + select { + case err := <-done: + assert.True(t, errors.Is(err, ErrNotInteractive), + "paginated picker should return ErrNotInteractive on %s stdin, got %v", kind, err) + assert.False(t, fetched, "a refused picker must not fetch a page it cannot show") + case <-time.After(5 * time.Second): + t.Fatalf("paginated picker blocked on %s stdin instead of refusing", kind) + } + }) + } +} + +// TestAutoSelectSingleWorksWithoutATerminal pins the one path through +// Picker.Run that is deliberately outside the floor. With WithAutoSelectSingle +// and exactly one static item, Run resolves without constructing a bubbletea +// program or reading a keystroke — there is nothing for non-terminal stdio to +// fail to drive, and WithAutoSelectSingle documents the behavior as +// unconditional. Gating it would have broken that for no safety gain. +// +// The second case is the boundary: two items need a real picker, so the floor +// applies again. +func TestAutoSelectSingleWorksWithoutATerminal(t *testing.T) { + for _, kind := range stdinKinds { + t.Run("single item resolves/"+kind, func(t *testing.T) { + nonInteractiveStdin(t, kind) + + items := []PickerItem{{ID: "42", Title: "Only one"}} + + done := make(chan *PickerItem, 1) + errs := make(chan error, 1) + go func() { + got, err := NewPicker(items, WithAutoSelectSingle()).Run() + done <- got + errs <- err + }() + + select { + case got := <-done: + require.NoError(t, <-errs) + require.NotNil(t, got, "the single item should be auto-selected") + assert.Equal(t, "42", got.ID) + case <-time.After(5 * time.Second): + t.Fatal("auto-select blocked; it should never reach a TUI") + } + }) + + t.Run("two items still refuse/"+kind, func(t *testing.T) { + terminalStdout(t) + nonInteractiveStdin(t, kind) + + items := []PickerItem{{ID: "1", Title: "One"}, {ID: "2", Title: "Two"}} + + done := make(chan error, 1) + go func() { + _, err := NewPicker(items, WithAutoSelectSingle()).Run() + done <- err + }() + + select { + case err := <-done: + assert.True(t, errors.Is(err, ErrNotInteractive), + "more than one item needs a real picker, got %v", err) + case <-time.After(5 * time.Second): + t.Fatal("picker blocked instead of refusing") + } + }) + } +} + +// TestPromptFloorCovers keeps the table honest. Every exported function in +// forms.go launches a huh form — directly or through one that does — so every +// one of them has to be exercised above. A new prompt fails here first. +func TestPromptFloorCovers(t *testing.T) { + covered := make(map[string]bool) + for _, floor := range promptFloors() { + covered[floor.name] = true + } + + file, err := parser.ParseFile(token.NewFileSet(), "forms.go", nil, 0) + require.NoError(t, err) + + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Recv != nil || !fn.Name.IsExported() { + continue + } + assert.True(t, covered[fn.Name.Name], + "forms.go exports %s but promptFloors() does not exercise its non-interactive floor", fn.Name.Name) + } +} diff --git a/internal/tui/launchers_test.go b/internal/tui/launchers_test.go new file mode 100644 index 000000000..7bd889344 --- /dev/null +++ b/internal/tui/launchers_test.go @@ -0,0 +1,564 @@ +package tui + +import ( + "errors" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A TUI launcher reads keystrokes, so every one of them needs the +// non-interactive floor. The floor is only worth anything if launchers cannot +// appear outside the places that apply it — which is the lesson of the +// `basecamp setup` hang: huh calls tea.NewProgram inside its own package, so an +// audit that greps for the launcher never saw the prompt at all. +// +// So bound where a launcher can be written rather than trying to recognize +// every spelling of one. Two layers: +// +// - a file may launch only if it is listed here, and +// - inside a listed file, the launch may sit only in the named runner, so a +// second unguarded one cannot ride in on the file's exemption. +// +// A file whose runner is "" is exempt wholesale, which needs its own argument. +// Both lists are deliberately short; adding to either is a decision to apply +// the floor by hand, and belongs in review rather than waved through. +var ( + // bubbleteaLaunchers maps a file that may call tea.NewProgram to the sole + // function within it that may do so. + bubbleteaLaunchers = map[string]string{ + "internal/tui/picker.go": "runPicker", + + // Zero callers today and slated for deletion, but "dead" is not a gate: + // adding a caller would not add a NewProgram, so the check would still + // pass while the hang came back. Both launch from their own Run, and + // both apply the floor there. + "internal/tui/spinner.go": "Run", + "internal/tui/paginated_picker.go": "Run", + + "internal/commands/tui.go": "", // dev build tag; not in a shipped binary + } + + // huhImporters may import huh at all. huh launches bubbletea internally, so + // importing it is importing a launcher. forms.go funnels every form through + // runForm — asserted separately by TestFormsRunsOnlyThroughRunForm, which + // looks for .Run() rather than NewProgram because the launch is inside huh. + huhImporters = map[string]string{ + "internal/tui/forms.go": "the sanctioned prompt layer", + } + + // launcherModules are the modules whose NewProgram starts a program, each + // paired with the package name it declares. Both of these live at + // .../bubbletea (or .../bubbletea/v2) and declare `package tea`, so the + // declared name is recorded here rather than derived from the path — an + // unaliased `import "charm.land/bubbletea/v2"` binds tea, not bubbletea, + // and a resolver that guessed from the path would look up the wrong + // identifier and wave the launcher through. + launcherModules = []struct{ path, pkg string }{ + {"charm.land/bubbletea", "tea"}, + {"github.com/charmbracelet/bubbletea", "tea"}, + } + + huhPaths = []string{ + "charm.land/huh", + "github.com/charmbracelet/huh", + } +) + +// matchesModule reports whether an import path is a module or one of its +// subpackages, under any major-version suffix. +func matchesModule(importPath string, modules []string) bool { + for _, m := range modules { + if importPath == m || strings.HasPrefix(importPath, m+"/") { + return true + } + } + return false +} + +// launcherIdents maps the identifiers a file binds to a launcher module back to +// that module's path. An explicit alias wins; otherwise the import binds the +// package name the module declares, which launcherModules records. Both forms +// are the same launcher and both have to be recognized: +// +// import tea "charm.land/bubbletea/v2" // alias +// import "charm.land/bubbletea/v2" // also binds tea +// +// Only the modules in launcherModules are resolved. That is the check's +// boundary: a new bubbletea-alike would need adding here, which go.mod makes +// visible in the same review. +func launcherIdents(file *ast.File) map[string]string { + idents := make(map[string]string, len(file.Imports)) + for _, spec := range file.Imports { + importPath, err := strconv.Unquote(spec.Path.Value) + if err != nil { + continue + } + for _, mod := range launcherModules { + if !matchesModule(importPath, []string{mod.path}) { + continue + } + name := mod.pkg + if spec.Name != nil { + name = spec.Name.Name + } + idents[name] = importPath + } + } + return idents +} + +// importPaths returns every path a file imports, for checks that do not care +// what the import is called locally. +func importPaths(file *ast.File) []string { + paths := make([]string, 0, len(file.Imports)) + for _, spec := range file.Imports { + importPath, err := strconv.Unquote(spec.Path.Value) + if err != nil { + continue + } + paths = append(paths, importPath) + } + return paths +} + +// repoRoot returns the module root, two levels up from internal/tui. +func repoRoot(t *testing.T) string { + t.Helper() + + wd, err := os.Getwd() + require.NoError(t, err) + + root := filepath.Join(wd, "..", "..") + _, err = os.Stat(filepath.Join(root, "go.mod")) + require.NoError(t, err, "expected the module root at %s", root) + return root +} + +// TestNoUnsanctionedLaunchers walks every non-test .go file in the repo and +// fails on a TUI launcher outside the lists above — or inside a listed file but +// outside its named runner. +func TestNoUnsanctionedLaunchers(t *testing.T) { + root := repoRoot(t) + fset := token.NewFileSet() + + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + switch d.Name() { + case ".git", "vendor", "node_modules", "bin", "dist": + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + + rel, relErr := filepath.Rel(root, p) + require.NoError(t, relErr) + rel = filepath.ToSlash(rel) + + // A file this walk cannot parse is not this check's business — the + // compiler and the linter both fail on it first. + file, parseErr := parser.ParseFile(fset, p, nil, 0) + if parseErr != nil { + return nil //nolint:nilerr // unparseable files are out of scope here + } + + for _, importPath := range importPaths(file) { + if matchesModule(importPath, huhPaths) { + _, allowed := huhImporters[rel] + assert.True(t, allowed, + "%s imports %s. huh launches a bubbletea program against the real stdin; "+ + "route prompts through internal/tui instead of importing it here", + rel, importPath) + } + } + + idents := launcherIdents(file) + + runner, fileAllowed := bubbleteaLaunchers[rel] + for _, ref := range referencesTo(file, "NewProgram", idents) { + if !fileAllowed { + assert.Fail(t, "unsanctioned bubbletea launcher", + "%s (%s) references NewProgram on %s. A bubbletea program reached without an "+ + "interactivity floor waits on /dev/tty rather than failing; gate it, "+ + "or route it through internal/tui", + rel, ref.where, ref.module) + continue + } + if runner != "" { + assert.Equal(t, runner, ref.enclosing, + "%s may reach bubbletea only in %s, where the floor is applied — %s "+ + "reaches it outside that runner", rel, runner, ref.where) + } + } + return nil + }) + require.NoError(t, err) +} + +// TestRunFormPinsItsOutputStream keeps the floor and huh talking about the same +// stream. huh has two render paths with two different defaults — stderr +// normally, stdout under accessible mode, which it enables on its own when +// TERM=dumb — so the stream is only knowable if we set it. canPrompt asks about +// stderr; runForm must therefore say stderr, not inherit a default that changes +// under an environment variable. +func TestRunFormPinsItsOutputStream(t *testing.T) { + file, err := parser.ParseFile(token.NewFileSet(), "forms.go", nil, 0) + require.NoError(t, err) + + var pinned bool + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name.Name != "runForm" { + continue + } + ast.Inspect(fn, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) != 1 { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "WithOutput" { + return true + } + arg, ok := call.Args[0].(*ast.SelectorExpr) + if !ok { + return true + } + pkg, ok := arg.X.(*ast.Ident) + if ok && pkg.Name == "os" && arg.Sel.Name == "Stderr" { + pinned = true + } + return true + }) + } + + assert.True(t, pinned, + "runForm must call WithOutput(os.Stderr): huh renders to stdout in accessible mode "+ + "(auto-enabled when TERM=dumb), which would disagree with canPrompt's stderr check") +} + +// launcherRef is one mention of a guarded symbol, and the top-level declaration +// it sits in. +type launcherRef struct { + enclosing string // the func's name, or "" for a package-level declaration + where string // human-readable, for the failure message + module string +} + +// referencesTo finds every mention of sel on a launcher package anywhere in the +// file, and attributes each to its enclosing top-level declaration. +// +// It walks the whole file rather than only *ast.FuncDecl bodies, because a +// launcher does not have to live in a function to run: +// +// var launch = func() { tea.NewProgram(m).Run() } // package-level func value +// var newProgram = tea.NewProgram // package-level alias +// +// Both are declarations, not FuncDecls, and a FuncDecl-only walk never sees +// them. Matching the mention rather than the call also catches the alias, where +// the call happens somewhere else entirely. +func referencesTo(file *ast.File, sel string, idents map[string]string) []launcherRef { + var refs []launcherRef + + for _, decl := range file.Decls { + enclosing, where := "", "package-level declaration" + if fn, ok := decl.(*ast.FuncDecl); ok { + enclosing = fn.Name.Name + where = "func " + fn.Name.Name + if fn.Recv != nil && len(fn.Recv.List) > 0 { + where = "method " + fn.Name.Name + } + } + + ast.Inspect(decl, func(n ast.Node) bool { + s, ok := n.(*ast.SelectorExpr) + if !ok || s.Sel.Name != sel { + return true + } + ident, ok := s.X.(*ast.Ident) + if !ok { + return true + } + if module, isLauncher := idents[ident.Name]; isLauncher { + refs = append(refs, launcherRef{enclosing: enclosing, where: where, module: module}) + } + return true + }) + } + return refs +} + +// formLaunchers are huh.Form's exported entry points. Both start the same +// bubbletea program, so both have to be funneled — checking only Run leaves +// RunWithContext as an open door. +var formLaunchers = []string{"Run", "RunWithContext"} + +// TestFormsRunsOnlyThroughRunForm keeps forms.go funneled. Every prompt there +// builds a form and hands it to runForm, which is where the floor lives — a +// launch anywhere else in the file is a prompt that skipped it. NewProgram is +// the wrong thing to look for here: huh makes that call inside its own package, +// which is exactly how this family stayed invisible. +func TestFormsRunsOnlyThroughRunForm(t *testing.T) { + assert.Equal(t, map[string]bool{"runForm": true}, + functionsReferencing(t, "forms.go", formLaunchers...), + "forms.go must launch forms only in runForm, which is where the non-interactive floor is applied") +} + +// functionsReferencing returns the enclosing top-level declaration of every +// mention of the named methods in a file. +// +// Two deliberate choices, each closing a way past an earlier version: +// +// - It matches a selector *reference*, not a call. `run := form.Run` is a +// method value: the launch happens wherever run is invoked, possibly in +// another file, and a CallExpr-only scan sees nothing here. +// - A package-level declaration reports as "", which no allowlist entry can +// match, so a launcher outside any function fails rather than slipping past +// a FuncDecl-only walk. +// +// It does not check the receiver's type — that would need type information this +// walk does not have. For a single-purpose file like forms.go that errs toward +// strictness, which is the right direction for a backstop: a false positive is +// a conversation, a false negative is the bug shipping. +func functionsReferencing(t *testing.T, filename string, names ...string) map[string]bool { + t.Helper() + + wanted := make(map[string]bool, len(names)) + for _, n := range names { + wanted[n] = true + } + + file, err := parser.ParseFile(token.NewFileSet(), filename, nil, 0) + require.NoError(t, err) + + callers := map[string]bool{} + for _, decl := range file.Decls { + enclosing := "" + if fn, ok := decl.(*ast.FuncDecl); ok { + enclosing = fn.Name.Name + } + ast.Inspect(decl, func(n ast.Node) bool { + if sel, ok := n.(*ast.SelectorExpr); ok && wanted[sel.Sel.Name] { + callers[enclosing] = true + } + return true + }) + } + return callers +} + +// TestLauncherIdentsResolvesEverySpelling guards the resolver the backstop +// depends on. It got this wrong once in a way nothing caught: it derived the +// identifier from the import path, so an unaliased bubbletea import registered +// as "bubbletea" while the file actually binds "tea", and every unaliased +// launcher walked past the check. Nothing in the repo noticed, because +// picker.go happens to alias its import. +// +// Both modules declare `package tea` at a path ending in bubbletea (or +// bubbletea/v2), so path and package name genuinely disagree — the exact shape +// a path-derived guess gets wrong. +func TestLauncherIdentsResolvesEverySpelling(t *testing.T) { + for _, tc := range []struct { + name string + source string + ident string + path string + }{ + { + name: "unaliased v2 binds tea, not bubbletea", + source: `package p; import "charm.land/bubbletea/v2"`, + ident: "tea", + path: "charm.land/bubbletea/v2", + }, + { + name: "unaliased v1 binds tea, not bubbletea", + source: `package p; import "github.com/charmbracelet/bubbletea"`, + ident: "tea", + path: "github.com/charmbracelet/bubbletea", + }, + { + name: "explicit alias wins", + source: `package p; import bt "charm.land/bubbletea/v2"`, + ident: "bt", + path: "charm.land/bubbletea/v2", + }, + { + name: "an alias may even be a misleading one", + source: `package p; import bubbletea "github.com/charmbracelet/bubbletea"`, + ident: "bubbletea", + path: "github.com/charmbracelet/bubbletea", + }, + } { + t.Run(tc.name, func(t *testing.T) { + file, err := parser.ParseFile(token.NewFileSet(), "p.go", tc.source, 0) + require.NoError(t, err) + + assert.Equal(t, map[string]string{tc.ident: tc.path}, launcherIdents(file)) + }) + } +} + +// TestLauncherIdentsIgnoresUnrelatedImports keeps the resolver from claiming +// identifiers it has no business claiming — a false positive here would block +// an unrelated NewProgram on some other package. +func TestLauncherIdentsIgnoresUnrelatedImports(t *testing.T) { + source := `package p + +import ( + "os" + + "github.com/charmbracelet/huh" + tea "example.com/not/bubbletea-alike" +)` + + file, err := parser.ParseFile(token.NewFileSet(), "p.go", source, 0) + require.NoError(t, err) + + assert.Empty(t, launcherIdents(file)) +} + +// TestReferencesToSeesPackageLevelLaunchers guards the walk itself. An earlier +// revision iterated only *ast.FuncDecl bodies, so a launcher that is not inside +// a function — a package-level func value, or an alias whose call happens +// elsewhere — was invisible to the backstop while looking perfectly normal in +// the diff. +// +// The enclosing name for a package-level declaration is "", which no allowlist +// entry can match, so such a launcher fails rather than slipping through. +func TestReferencesToSeesPackageLevelLaunchers(t *testing.T) { + const mod = "charm.land/bubbletea/v2" + + for _, tc := range []struct { + name string + source string + enclosing string + }{ + { + name: "inside a function", + source: `func run() { tea.NewProgram(nil) }`, + enclosing: "run", + }, + { + name: "inside a method", + source: `func (p *picker) run() { tea.NewProgram(nil) }`, + enclosing: "run", + }, + { + name: "package-level func value", + source: `var launch = func() { tea.NewProgram(nil) }`, + enclosing: "", + }, + { + name: "package-level alias, called elsewhere entirely", + source: `var newProgram = tea.NewProgram`, + enclosing: "", + }, + { + name: "inside a package-level composite literal", + source: `var launchers = []func(){func() { tea.NewProgram(nil) }}`, + enclosing: "", + }, + } { + t.Run(tc.name, func(t *testing.T) { + src := "package p\n\nimport \"" + mod + "\"\n\ntype picker struct{}\n\n" + tc.source + file, err := parser.ParseFile(token.NewFileSet(), "p.go", src, 0) + require.NoError(t, err) + + refs := referencesTo(file, "NewProgram", launcherIdents(file)) + require.Len(t, refs, 1, "the walk must see this launcher") + assert.Equal(t, tc.enclosing, refs[0].enclosing) + assert.Equal(t, mod, refs[0].module) + }) + } +} + +// TestReferencesToIgnoresUnrelatedSelectors keeps the walk from firing on +// something that merely shares a method name. +func TestReferencesToIgnoresUnrelatedSelectors(t *testing.T) { + src := `package p + +import "charm.land/bubbletea/v2" + +type other struct{} + +func (o other) NewProgram() {} + +func run() { + var o other + o.NewProgram() + _ = tea.Quit +}` + + file, err := parser.ParseFile(token.NewFileSet(), "p.go", src, 0) + require.NoError(t, err) + + assert.Empty(t, referencesTo(file, "NewProgram", launcherIdents(file)), + "only NewProgram on a launcher package counts") +} + +// TestRunFormTranslatesCancellation guards a chain that five call sites now +// depend on and none of them can see. +// +// huh reports a dismissal — Escape or Ctrl+C, both bound to Quit by escKeyMap — +// by setting f.aborted, which Run turns into ErrUserAborted (form.go:558, 690). +// Everything else (a timeout, a wrapped bubbletea failure) arrives as an +// ordinary error. runForm is the single place that knows this, translating the +// dismissal into ErrCanceled so callers can swallow exactly that one value. +// +// Drop the translation and every caller silently reverts to reporting a +// cancellation nobody performed — which is the bug this replaced, and it would +// come back without a single test failing anywhere else. +func TestRunFormTranslatesCancellation(t *testing.T) { + file, err := parser.ParseFile(token.NewFileSet(), "forms.go", nil, 0) + require.NoError(t, err) + + var sawAborted, sawCanceled bool + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name.Name != "runForm" { + continue + } + ast.Inspect(fn, func(n ast.Node) bool { + switch e := n.(type) { + case *ast.SelectorExpr: + if id, ok := e.X.(*ast.Ident); ok && id.Name == "huh" && e.Sel.Name == "ErrUserAborted" { + sawAborted = true + } + case *ast.Ident: + if e.Name == "ErrCanceled" { + sawCanceled = true + } + } + return true + }) + } + + assert.True(t, sawAborted && sawCanceled, + "runForm must translate huh.ErrUserAborted into ErrCanceled: it is the only place that "+ + "knows huh's error values, and its callers swallow ErrCanceled alone") +} + +// TestPromptSentinelsAreDistinct keeps the two outcomes from collapsing into +// each other. "Nobody could be asked" and "the user said no" lead to opposite +// handling — a usage error versus exit 0 — so a caller matching on one must +// never match the other. +func TestPromptSentinelsAreDistinct(t *testing.T) { + assert.False(t, errors.Is(ErrCanceled, ErrNotInteractive)) + assert.False(t, errors.Is(ErrNotInteractive, ErrCanceled)) + assert.NotEqual(t, ErrCanceled.Error(), ErrNotInteractive.Error()) +} diff --git a/internal/tui/paginated_picker.go b/internal/tui/paginated_picker.go index 11c47d074..bc288ceb7 100644 --- a/internal/tui/paginated_picker.go +++ b/internal/tui/paginated_picker.go @@ -387,8 +387,13 @@ func NewPaginatedPicker(ctx context.Context, fetcher PageFetcher, opts ...Pagina } // Run shows the picker and returns the selected item. -// Returns nil if the user canceled. +// Returns nil if the user canceled, and ErrNotInteractive when stdio cannot +// drive a TUI at all. func (p *PaginatedPicker) Run() (*PickerItem, error) { + if !canPick() { + return nil, ErrNotInteractive + } + m := newPaginatedPickerModel(p.ctx, p.fetcher, p.opts...) program := tea.NewProgram(m) diff --git a/internal/tui/picker.go b/internal/tui/picker.go index 210c8e3a9..3dad6fda2 100644 --- a/internal/tui/picker.go +++ b/internal/tui/picker.go @@ -488,7 +488,9 @@ func NewPickerWithLoader(loader ItemLoader, opts ...PickerOption) *Picker { } // Run shows the picker and returns the selected item. -// Returns nil if the user canceled. +// Returns nil if the user canceled, and ErrNotInteractive when stdio cannot +// drive a TUI at all — except for the single-item WithAutoSelectSingle fast +// path, which resolves without a TUI and so works anywhere. func (p *Picker) Run() (*PickerItem, error) { if p.loader != nil { return p.runWithLoader() @@ -496,20 +498,20 @@ func (p *Picker) Run() (*PickerItem, error) { m := newPickerModel(p.items, p.opts...) - // Auto-select if only one item and option is set + // Auto-select if only one item and option is set. Deliberately ahead of the + // floor: this path launches nothing and reads no input, so there is nothing + // for a non-terminal stdio to fail to drive, and refusing here would break + // WithAutoSelectSingle's documented unconditional behavior for no safety + // gain. Everything that does launch goes through runPicker, which holds the + // floor. if m.autoSelectSingle && len(m.items) == 1 { return m.getOriginalItem(m.items[0].ID), nil } - // Use alternate screen so picker disappears after selection - program := tea.NewProgram(m) - - finalModel, err := program.Run() + final, err := runPicker(m, nil) if err != nil { return nil, err } - - final := finalModel.(pickerModel) //nolint:errcheck // type assertion always succeeds here if final.quitting { return nil, nil } @@ -523,27 +525,49 @@ func (p *Picker) runWithLoader() (*PickerItem, error) { m.loading = true m.loadingMsg = "Loading…" } - // Use alternate screen so picker disappears after selection - program := tea.NewProgram(m) - - // Load items in background - go func() { - items, err := p.loader() - program.Send(PickerItemsLoadedMsg{Items: items, Err: err}) - }() - finalModel, err := program.Run() + // Load items in background, once the program exists to receive them. + final, err := runPicker(m, func(program *tea.Program) { + go func() { + items, loadErr := p.loader() + program.Send(PickerItemsLoadedMsg{Items: items, Err: loadErr}) + }() + }) if err != nil { return nil, err } - - final := finalModel.(pickerModel) //nolint:errcheck // type assertion always succeeds here if final.quitting { return nil, final.loadError // Return loader error if any (nil if user just canceled) } return final.selected, nil } +// runPicker is the one place this package starts a bubbletea program, and so +// the one place the floor has to hold. TestPickerRunsOnlyThroughRunPicker keeps +// it that way: a tea.NewProgram anywhere else in this file fails the test. It +// mirrors runForm in forms.go — a launcher nobody can reach without passing the +// floor beats a call-site audit, which is what missed `basecamp setup`. +// +// start, when non-nil, runs after the program is constructed and before it is +// started, for work that needs to Send into it. +func runPicker(m pickerModel, start func(*tea.Program)) (pickerModel, error) { + if !canPick() { + return pickerModel{}, ErrNotInteractive + } + + // Use alternate screen so picker disappears after selection + program := tea.NewProgram(m) + if start != nil { + start(program) + } + + finalModel, err := program.Run() + if err != nil { + return pickerModel{}, err + } + return finalModel.(pickerModel), nil //nolint:errcheck // type assertion always succeeds here +} + // Pick is a convenience function for simple picking. func Pick(title string, items []PickerItem) (*PickerItem, error) { return NewPicker(items, WithPickerTitle(title)).Run() diff --git a/internal/tui/resolve/resolve.go b/internal/tui/resolve/resolve.go index 65a9496dd..a39f9d89b 100644 --- a/internal/tui/resolve/resolve.go +++ b/internal/tui/resolve/resolve.go @@ -100,12 +100,16 @@ func (r *Resolver) Flags() *Flags { return r.flags } -// IsInteractive returns true if interactive prompts can be shown. +// IsInteractive returns true if a picker can be shown. // This checks stdout, stdin, and machine-output flags. // Returns false if BASECAMP_NONINTERACTIVE is set, if any machine-output flag is // set (--agent, --json, --quiet, --ids-only, --count), or if stdout or stdin is -// not a character device (the guard treats any char device — a terminal, -// /dev/null, etc. — as interactive-capable). +// not a terminal. /dev/null does not count: it is a character device, but it +// delivers no keystrokes, and Bubble Tea responds to a non-terminal stdin by +// opening /dev/tty and waiting on the real terminal. +// +// This is the picker's pair (stdin+stdout). A huh form draws to stderr instead, +// which is stdinarg.InteractivePrompt — see internal/tui. func (r *Resolver) IsInteractive() bool { // Explicit escape hatch: BASECAMP_NONINTERACTIVE forces non-interactive mode // even under a PTY, without changing the output format. @@ -120,10 +124,10 @@ func (r *Resolver) IsInteractive() bool { } } - // Both stdout and stdin must be character devices: pickers draw to - // stdout and read keystrokes from stdin, so a pipe on either end can - // never drive one — and when the command is consuming piped content - // (a "-" stdin input), a picker would eat that content as key events. + // Both stdout and stdin must be terminals: pickers draw to stdout and read + // keystrokes from stdin, so a pipe on either end can never drive one — and + // when the command is consuming piped content (a "-" stdin input), a picker + // would eat that content as key events. return stdinarg.InteractiveStdio() } diff --git a/internal/tui/resolve/resolve_test.go b/internal/tui/resolve/resolve_test.go index bdd1a0235..6b8feb016 100644 --- a/internal/tui/resolve/resolve_test.go +++ b/internal/tui/resolve/resolve_test.go @@ -2,16 +2,43 @@ package resolve import ( "os" + "runtime" "testing" ) -// TestIsInteractiveRequiresStdinCharDevice proves pickers are gated off when -// stdin is piped: a Bubble Tea picker reads keystrokes from stdin, so piped -// stdin can never drive one — and when a command is consuming piped content -// (a "-" stdin input), a picker would eat that content as key events. -func TestIsInteractiveRequiresStdinCharDevice(t *testing.T) { +// openPTY returns the master side of a new pseudo-terminal, which is what +// term.IsTerminal actually accepts. /dev/null will not do: it is a character +// device but not a terminal, and treating it as one is the bug this file +// guards. +func openPTY(t *testing.T) *os.File { + t.Helper() + + if runtime.GOOS == "windows" { + t.Skip("no /dev/ptmx on Windows") + } + f, err := os.OpenFile("/dev/ptmx", os.O_RDWR, 0) + if err != nil { + t.Skipf("open /dev/ptmx: %v", err) + } + t.Cleanup(func() { _ = f.Close() }) + return f +} + +// TestIsInteractiveRequiresTerminalStdio proves pickers are gated off unless +// both ends are real terminals. A Bubble Tea picker reads keystrokes from +// stdin, so piped stdin can never drive one — and when a command is consuming +// piped content (a "-" stdin input), a picker would eat that content as key +// events. +// +// The /dev/null case is the one worth stating out loud: it is a character +// device, so the older character-device test called it interactive. Bubble Tea +// disagrees — it sees a non-terminal stdin, opens /dev/tty and waits on the +// real terminal — so `basecamp ... < /dev/null` from a terminal session hung. +func TestIsInteractiveRequiresTerminalStdio(t *testing.T) { t.Setenv("BASECAMP_NONINTERACTIVE", "") + pty := openPTY(t) + devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) if err != nil { t.Fatalf("open %s: %v", os.DevNull, err) @@ -28,19 +55,28 @@ func TestIsInteractiveRequiresStdinCharDevice(t *testing.T) { origOut, origIn := os.Stdout, os.Stdin t.Cleanup(func() { os.Stdout, os.Stdin = origOut, origIn }) - // /dev/null is a character device, so it stands in for a terminal on - // both ends without needing a PTY. - os.Stdout = devnull + os.Stdout = pty r := New(nil, nil, nil) - os.Stdin = devnull + os.Stdin = pty if !r.IsInteractive() { - t.Fatal("expected interactive with char-device stdout and stdin") + t.Fatal("expected interactive with terminal stdout and stdin") } os.Stdin = pipeR if r.IsInteractive() { t.Fatal("expected non-interactive with piped stdin: a picker would consume the pipe as key events") } + + os.Stdin = devnull + if r.IsInteractive() { + t.Fatal("expected non-interactive with stdin on /dev/null: a picker there waits on /dev/tty forever") + } + + os.Stdin = pty + os.Stdout = devnull + if r.IsInteractive() { + t.Fatal("expected non-interactive with stdout on /dev/null") + } } diff --git a/internal/tui/spinner.go b/internal/tui/spinner.go index 802ba02c7..4c13eec9a 100644 --- a/internal/tui/spinner.go +++ b/internal/tui/spinner.go @@ -154,6 +154,10 @@ func NewSpinner(message string, opts ...SpinnerOption) *Spinner { // Run executes the given function while displaying a spinner. // Returns the result and any error from the function. func (s *Spinner) Run(fn func() (string, error)) (string, error) { + if !canPick() { + return "", ErrNotInteractive + } + m := newSpinnerModel(s.message, s.opts...) p := tea.NewProgram(m) diff --git a/skills/basecamp-doctor/SKILL.md b/skills/basecamp-doctor/SKILL.md index 17311a368..c815d44e6 100644 --- a/skills/basecamp-doctor/SKILL.md +++ b/skills/basecamp-doctor/SKILL.md @@ -21,9 +21,14 @@ Interpret every check by status: Report failures and warnings with their `hint` fields. Also inspect the top-level `breadcrumbs` array and preserve its structured `cmd` next steps, because a breadcrumb can provide a more specific action than a check hint. Use these common remediations when relevant: - Basecamp authentication: `basecamp auth login` -- Agent plugin installation or version: `basecamp setup` -- Skill + all detected agents, non-interactively: `basecamp setup agents` (honors `BASECAMP_SETUP_AGENT`) +- Agent plugin installation or version: `basecamp setup agents` (honors `BASECAMP_SETUP_AGENT`) - Codex plugin specifically: `basecamp setup codex` - Claude Code plugin specifically: `basecamp setup claude` +Every remediation above runs without a terminal. Bare `basecamp setup` is the +interactive first-run wizard and is **not** one of them: it is prompts end to +end, so it refuses with a usage error in machine-output modes or when stdin and +stderr are not both terminals. Suggest it to a human at a terminal if you like, +but never run it yourself — use the subcommands above. + Do not read, print, or request credential files. If every check passes, say that Basecamp and its agent integration are ready.