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.