From 7361e8f06b23aec9bc6dbceb64baed6edd906c51 Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Wed, 9 Jul 2025 08:08:45 +1000 Subject: [PATCH 1/2] Add non-TTY handling to Confirm function - Confirm function now fails by default in non-TTY environments - Shows helpful error message suggesting to use -y/--yes flag - Maintains existing behavior for TTY environments - Adds comprehensive test coverage for new behavior Amp-Thread: https://ampcode.com/threads/T-96c672dc-7e75-4174-b20e-b6bff5e074d5 Co-authored-by: Amp --- internal/io/confirm.go | 16 +++++++- internal/io/confirm_test.go | 75 +++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 internal/io/confirm_test.go diff --git a/internal/io/confirm.go b/internal/io/confirm.go index 454cca02..d7f887c6 100644 --- a/internal/io/confirm.go +++ b/internal/io/confirm.go @@ -1,10 +1,24 @@ package io import ( + "fmt" + "os" + "github.com/charmbracelet/huh" + "github.com/mattn/go-isatty" ) func Confirm(confirmed *bool, title string) error { + // If already confirmed via flag, skip the prompt + if *confirmed { + return nil + } + + // In non-TTY environments, fail by default with yes flag message + if !isatty.IsTerminal(os.Stdout.Fd()) { + return fmt.Errorf("confirmation required but not running in a terminal; use -y or --yes to confirm") + } + form := huh.NewForm( huh.NewGroup( huh.NewConfirm(). @@ -12,7 +26,7 @@ func Confirm(confirmed *bool, title string) error { Affirmative("Yes"). Negative("No"). Value(confirmed), - ).WithHide(*confirmed), // user can bypass the prompt by passing the flag + ), ) err := form.Run() diff --git a/internal/io/confirm_test.go b/internal/io/confirm_test.go new file mode 100644 index 00000000..b6680e6a --- /dev/null +++ b/internal/io/confirm_test.go @@ -0,0 +1,75 @@ +package io + +import ( + "os" + "testing" +) + +func TestConfirm(t *testing.T) { + tests := []struct { + name string + confirmed bool + isTTY bool + expectError bool + errorMsg string + }{ + { + name: "already confirmed via flag", + confirmed: true, + isTTY: false, + expectError: false, + }, + { + name: "not confirmed and not TTY", + confirmed: false, + isTTY: false, + expectError: true, + errorMsg: "confirmation required but not running in a terminal; use -y or --yes to confirm", + }, + { + name: "not confirmed and TTY (interactive test skipped)", + confirmed: false, + isTTY: true, + expectError: false, // We'll skip this test as it requires user interaction + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.name == "not confirmed and TTY (interactive test skipped)" { + t.Skip("Skipping interactive test") + } + + // Mock TTY detection by temporarily redirecting stdout + if !tt.isTTY { + // Create a pipe to simulate non-TTY + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer r.Close() + defer w.Close() + + // Temporarily replace stdout + oldStdout := os.Stdout + os.Stdout = w + defer func() { + os.Stdout = oldStdout + }() + } + + confirmed := tt.confirmed + err := Confirm(&confirmed, "Test confirmation") + + if tt.expectError { + if err == nil { + t.Errorf("Expected error but got none") + } else if err.Error() != tt.errorMsg { + t.Errorf("Expected error message %q but got %q", tt.errorMsg, err.Error()) + } + } else if err != nil { + t.Errorf("Unexpected error: %v", err) + } + }) + } +} From 675f5439259e06bcad85f11027709571fc3d3929 Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Wed, 9 Jul 2025 08:03:26 +1000 Subject: [PATCH 2/2] Refactor TTY detection with internal wrapper - Add TTY wrapper functions in internal/io package - Replace direct isatty usage with IsTerminal() - Centralize TTY detection logic for easier testing and future changes - Update all tests to use the new wrapper functions - isatty package now only imported in one place Amp-Thread: https://ampcode.com/threads/T-96c672dc-7e75-4174-b20e-b6bff5e074d5 Co-authored-by: Amp --- internal/io/confirm.go | 4 +--- internal/io/spinner.go | 5 +---- internal/io/spinner_test.go | 5 +---- internal/io/tty.go | 12 ++++++++++++ 4 files changed, 15 insertions(+), 11 deletions(-) create mode 100644 internal/io/tty.go diff --git a/internal/io/confirm.go b/internal/io/confirm.go index d7f887c6..ee6c9620 100644 --- a/internal/io/confirm.go +++ b/internal/io/confirm.go @@ -2,10 +2,8 @@ package io import ( "fmt" - "os" "github.com/charmbracelet/huh" - "github.com/mattn/go-isatty" ) func Confirm(confirmed *bool, title string) error { @@ -15,7 +13,7 @@ func Confirm(confirmed *bool, title string) error { } // In non-TTY environments, fail by default with yes flag message - if !isatty.IsTerminal(os.Stdout.Fd()) { + if !IsTerminal() { return fmt.Errorf("confirmation required but not running in a terminal; use -y or --yes to confirm") } diff --git a/internal/io/spinner.go b/internal/io/spinner.go index 565d90cc..29999973 100644 --- a/internal/io/spinner.go +++ b/internal/io/spinner.go @@ -1,14 +1,11 @@ package io import ( - "os" - "github.com/charmbracelet/huh/spinner" - "github.com/mattn/go-isatty" ) func SpinWhile(name string, action func()) error { - if !isatty.IsTerminal(os.Stdout.Fd()) { + if !IsTerminal() { // No TTY available, just run the action without spinner action() return nil diff --git a/internal/io/spinner_test.go b/internal/io/spinner_test.go index 4a37613a..3ba3acaf 100644 --- a/internal/io/spinner_test.go +++ b/internal/io/spinner_test.go @@ -1,10 +1,7 @@ package io import ( - "os" "testing" - - "github.com/mattn/go-isatty" ) func TestSpinWhileWithoutTTY(t *testing.T) { @@ -59,7 +56,7 @@ func TestSpinWhileWithError(t *testing.T) { func TestSpinWhileTTYDetection(t *testing.T) { // Test that TTY detection works as expected // This test documents the behavior rather than forcing specific outcomes - isTTY := isatty.IsTerminal(os.Stdout.Fd()) + isTTY := IsTerminal() actionCalled := false err := SpinWhile("TTY detection test", func() { diff --git a/internal/io/tty.go b/internal/io/tty.go new file mode 100644 index 00000000..5b8c1fbe --- /dev/null +++ b/internal/io/tty.go @@ -0,0 +1,12 @@ +package io + +import ( + "os" + + "github.com/mattn/go-isatty" +) + +// IsTerminal returns true if stdout is a terminal +func IsTerminal() bool { + return isatty.IsTerminal(os.Stdout.Fd()) +}