Skip to content

Commit 549a2a2

Browse files
sj26ampcode-com
andcommitted
Fix CLI TTY dependency issues for CI/script usage
- Fix SpinWhile logic to properly handle non-TTY environments - Add TTY detection to Confirm and PromptForOne functions - Replace all spinner.New() calls with TTY-aware SpinWhile() calls - Add non-TTY fallback for agent stop command using direct API calls - Add comprehensive test coverage for TTY/non-TTY behavior - All spinner operations now gracefully degrade when no TTY is available Fixes #496 Amp-Thread: https://ampcode.com/threads/T-04774b89-5ae4-4839-830d-67c16e65acb6 Co-authored-by: Amp <[email protected]>
1 parent f23dec3 commit 549a2a2

File tree

21 files changed

+322
-203
lines changed

21 files changed

+322
-203
lines changed

internal/agent/stoppable.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,3 +109,11 @@ func (agent StoppableAgent) View() string {
109109
func (agent StoppableAgent) Errored() bool {
110110
return agent.err != nil
111111
}
112+
113+
func (agent StoppableAgent) Error() error {
114+
return agent.err
115+
}
116+
117+
func (agent StoppableAgent) ID() string {
118+
return agent.id
119+
}

internal/io/confirm.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,24 @@
11
package io
22

33
import (
4+
"os"
5+
46
"github.com/charmbracelet/huh"
7+
"github.com/mattn/go-isatty"
58
)
69

710
func Confirm(confirmed *bool, title string) error {
11+
// If already confirmed via flag, skip the prompt
12+
if *confirmed {
13+
return nil
14+
}
15+
16+
// If no TTY is available, default to confirmed=true (non-interactive mode)
17+
if !isatty.IsTerminal(os.Stdout.Fd()) {
18+
*confirmed = true
19+
return nil
20+
}
21+
822
form := huh.NewForm(
923
huh.NewGroup(
1024
huh.NewConfirm().

internal/io/confirm_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package io
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func TestConfirmWithoutTTY(t *testing.T) {
8+
// Test that Confirm works without TTY and defaults to true
9+
confirmed := false
10+
err := Confirm(&confirmed, "Test confirmation")
11+
12+
if err != nil {
13+
t.Errorf("Confirm should not return error: %v", err)
14+
}
15+
16+
if !confirmed {
17+
t.Error("Confirm should default to true when no TTY is available")
18+
}
19+
}
20+
21+
func TestConfirmWithFlag(t *testing.T) {
22+
// Test that Confirm respects pre-set flag
23+
confirmed := true
24+
err := Confirm(&confirmed, "Test confirmation")
25+
26+
if err != nil {
27+
t.Errorf("Confirm should not return error: %v", err)
28+
}
29+
30+
if !confirmed {
31+
t.Error("Confirm should respect pre-set flag")
32+
}
33+
}

internal/io/prompt.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
package io
22

3-
import "github.com/charmbracelet/huh"
3+
import (
4+
"errors"
5+
"os"
6+
7+
"github.com/charmbracelet/huh"
8+
"github.com/mattn/go-isatty"
9+
)
410

511
const (
612
typeOrganizationMessage = "Pick an organization"
@@ -10,6 +16,11 @@ const (
1016
// PromptForOne will show the list of options to the user, allowing them to select one to return.
1117
// It's possible for them to choose none or cancel the selection, resulting in an error.
1218
func PromptForOne(resource string, options []string) (string, error) {
19+
// If no TTY is available, cannot prompt user for selection
20+
if !isatty.IsTerminal(os.Stdout.Fd()) {
21+
return "", errors.New("cannot prompt for selection: no TTY available (use appropriate flags to specify the selection)")
22+
}
23+
1324
var message string
1425
switch resource {
1526
case "pipeline":

internal/io/prompt_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package io
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func TestPromptForOneWithoutTTY(t *testing.T) {
8+
// Test that PromptForOne fails gracefully without TTY
9+
_, err := PromptForOne("pipeline", []string{"option1", "option2"})
10+
11+
if err == nil {
12+
t.Error("PromptForOne should return error when no TTY is available")
13+
}
14+
15+
expectedError := "cannot prompt for selection: no TTY available (use appropriate flags to specify the selection)"
16+
if err.Error() != expectedError {
17+
t.Errorf("Expected error message %q, got %q", expectedError, err.Error())
18+
}
19+
}

internal/io/spinner.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,13 @@ import (
88
)
99

1010
func SpinWhile(name string, action func()) error {
11-
if isatty.IsTerminal(os.Stdout.Fd()) {
11+
if !isatty.IsTerminal(os.Stdout.Fd()) {
12+
// No TTY available, just run the action without spinner
1213
action()
13-
1414
return nil
1515
}
1616

17+
// TTY is available, use the spinner
1718
return spinner.New().
1819
Title(name).
1920
Action(action).

internal/io/spinner_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
package io
2+
3+
import (
4+
"testing"
5+
)
6+
7+
func TestSpinWhileWithoutTTY(t *testing.T) {
8+
// Test that SpinWhile works without TTY
9+
actionCalled := false
10+
err := SpinWhile("Test action", func() {
11+
actionCalled = true
12+
})
13+
14+
if err != nil {
15+
t.Errorf("SpinWhile should not return error: %v", err)
16+
}
17+
18+
if !actionCalled {
19+
t.Error("Action should have been called")
20+
}
21+
}

internal/pipeline/resolver/repository.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@ import (
44
"context"
55
"strings"
66

7+
bk_io "github.com/buildkite/cli/v3/internal/io"
78
"github.com/buildkite/cli/v3/internal/pipeline"
89
"github.com/buildkite/cli/v3/pkg/cmd/factory"
910
buildkite "github.com/buildkite/go-buildkite/v4"
10-
"github.com/charmbracelet/huh/spinner"
1111
git "github.com/go-git/go-git/v5"
1212
)
1313

@@ -19,12 +19,9 @@ func ResolveFromRepository(f *factory.Factory, picker PipelinePicker) PipelineRe
1919
return func(ctx context.Context) (*pipeline.Pipeline, error) {
2020
var err error
2121
var pipelines []pipeline.Pipeline
22-
spinErr := spinner.New().
23-
Title("Resolving pipeline").
24-
Action(func() {
25-
pipelines, err = resolveFromRepository(ctx, f)
26-
}).
27-
Run()
22+
spinErr := bk_io.SpinWhile("Resolving pipeline", func() {
23+
pipelines, err = resolveFromRepository(ctx, f)
24+
})
2825
if spinErr != nil {
2926
return nil, spinErr
3027
}

pkg/cmd/agent/stop.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"bufio"
55
"context"
66
"errors"
7+
"fmt"
8+
"os"
79
"strings"
810
"sync"
911

@@ -12,6 +14,7 @@ import (
1214
bk_io "github.com/buildkite/cli/v3/internal/io"
1315
"github.com/buildkite/cli/v3/pkg/cmd/factory"
1416
tea "github.com/charmbracelet/bubbletea"
17+
"github.com/mattn/go-isatty"
1518
"github.com/spf13/cobra"
1619
"golang.org/x/sync/semaphore"
1720
)
@@ -88,6 +91,54 @@ func RunStop(cmd *cobra.Command, args []string, opts *AgentStopOptions) error {
8891
return errors.New("must supply agents to stop")
8992
}
9093

94+
// If no TTY is available, run in non-interactive mode
95+
if !isatty.IsTerminal(os.Stdout.Fd()) {
96+
// Directly execute the stop operations synchronously
97+
var failed []string
98+
var agentIDs []string
99+
100+
// Use the same logic as the original: either stdin OR args
101+
if bk_io.HasDataAvailable(cmd.InOrStdin()) {
102+
scanner := bufio.NewScanner(cmd.InOrStdin())
103+
scanner.Split(bufio.ScanLines)
104+
for scanner.Scan() {
105+
agentID := scanner.Text()
106+
if strings.TrimSpace(agentID) != "" {
107+
agentIDs = append(agentIDs, agentID)
108+
}
109+
}
110+
if scanner.Err() != nil {
111+
return scanner.Err()
112+
}
113+
} else if len(args) > 0 {
114+
agentIDs = args
115+
} else {
116+
return errors.New("must supply agents to stop")
117+
}
118+
119+
// Process all agent IDs
120+
for _, agentID := range agentIDs {
121+
if strings.TrimSpace(agentID) != "" {
122+
org, id := parseAgentArg(agentID, opts.f.Config)
123+
_, err := opts.f.RestAPIClient.Agents.Stop(cmd.Context(), org, id, opts.force)
124+
if err != nil {
125+
failed = append(failed, fmt.Sprintf("Failed to stop agent %s: %v", agentID, err))
126+
} else {
127+
fmt.Fprintf(cmd.OutOrStdout(), "Stopped agent %s\n", agentID)
128+
}
129+
}
130+
}
131+
132+
if len(failed) > 0 {
133+
for _, failure := range failed {
134+
fmt.Fprintf(cmd.ErrOrStderr(), "%s\n", failure)
135+
}
136+
return errors.New("at least one agent failed to stop")
137+
}
138+
return nil
139+
}
140+
141+
// TTY is available, use interactive mode with Bubble Tea
91142
bulkAgent := agent.BulkAgent{
92143
Agents: agents,
93144
}

pkg/cmd/agent/view.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ import (
55

66
"github.com/MakeNowJust/heredoc"
77
"github.com/buildkite/cli/v3/internal/agent"
8+
bk_io "github.com/buildkite/cli/v3/internal/io"
89
"github.com/buildkite/cli/v3/pkg/cmd/factory"
910
buildkite "github.com/buildkite/go-buildkite/v4"
10-
"github.com/charmbracelet/huh/spinner"
1111
"github.com/pkg/browser"
1212
"github.com/spf13/cobra"
1313
)
@@ -37,12 +37,9 @@ func NewCmdAgentView(f *factory.Factory) *cobra.Command {
3737

3838
var err error
3939
var agentData buildkite.Agent
40-
spinErr := spinner.New().
41-
Title("Loading agent").
42-
Action(func() {
43-
agentData, _, err = f.RestAPIClient.Agents.Get(cmd.Context(), org, id)
44-
}).
45-
Run()
40+
spinErr := bk_io.SpinWhile("Loading agent", func() {
41+
agentData, _, err = f.RestAPIClient.Agents.Get(cmd.Context(), org, id)
42+
})
4643
if spinErr != nil {
4744
return spinErr
4845
}

0 commit comments

Comments
 (0)