Handle OAuth denial and deduplicate login output

cmd/login.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"os/signal"
@@ -55,32 +56,41 @@ func runLogin(cmd *cobra.Command, args []string) error {
 
 	pterm.Debug.Printf("Starting local callback server on %s\n", oauthConfig.Config.RedirectURL)
 
-	// Start OAuth flow
-	spinner, _ := pterm.DefaultSpinner.Start("Waiting for authentication...")
-	tokens, err := oauthConfig.StartOAuthFlow(ctx)
-	if err != nil {
-		spinner.Fail("Authentication failed")
+	spinner, _ := newLoginSpinner().Start("Waiting for authentication...")
+	return completeLogin(ctx, spinner, oauthConfig.StartOAuthFlow, auth.SaveTokens)
+}
 
-		// Handle common error cases with helpful messages
-		if ctx.Err() == context.Canceled {
-			pterm.Info.Println("Authentication cancelled by user")
-			return nil
-		}
+func newLoginSpinner() *pterm.SpinnerPrinter {
+	return pterm.DefaultSpinner.WithRemoveWhenDone()
+}
+
+type spinnerStopper interface {
+	Stop() error
+}
 
+func completeLogin(
+	ctx context.Context,
+	spinner spinnerStopper,
+	authenticate func(context.Context) (*auth.TokenStorage, error),
+	saveTokens func(*auth.TokenStorage) error,
+) error {
+	tokens, err := authenticate(ctx)
+	_ = spinner.Stop()
+	if err != nil {
+		if errors.Is(err, auth.ErrAuthorizationDenied) {
+			return err
+		}
+		if errors.Is(err, context.Canceled) {
+			return errors.New("authentication cancelled by user")
+		}
 		return fmt.Errorf("authentication failed: %w", err)
 	}
 
-	spinner.Success("Authentication successful!")
-
-	// Save tokens securely
-	if err := auth.SaveTokens(tokens); err != nil {
-		pterm.Warning.Printf("Authentication succeeded but failed to save credentials: %v\n", err)
-		pterm.Warning.Println("You may need to re-authenticate on your next CLI usage")
-		return nil
+	if err := saveTokens(tokens); err != nil {
+		return fmt.Errorf("OAuth authorization completed, but credentials could not be saved: %w; fix credential storage and run 'kernel login' again", err)
 	}
 
-	pterm.Success.Println("✓ Successfully authenticated with Kernel!")
+	pterm.Success.Println("Successfully authenticated with Kernel!")
 	pterm.Info.Println("You can now use other Kernel CLI commands without setting KERNEL_API_KEY")
-
 	return nil
 }

cmd/login_test.go

@@ -0,0 +1,197 @@
+package cmd
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/charmbracelet/fang"
+	"github.com/charmbracelet/lipgloss/v2"
+	"github.com/kernel/cli/pkg/auth"
+	"github.com/pterm/pterm"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type testSpinner struct {
+	stops int
+}
+
+func (s *testSpinner) Stop() error {
+	s.stops++
+	return nil
+}
+
+func TestCompleteLoginOutput(t *testing.T) {
+	tests := []struct {
+		name         string
+		authErr      error
+		saveErr      error
+		cancel       bool
+		wantErr      bool
+		wantSave     bool
+		wantSuccess  int
+		wantErrors   int
+		wantOutput   []string
+		absentOutput []string
+	}{
+		{
+			name:        "success",
+			wantSave:    true,
+			wantSuccess: 1,
+			wantOutput: []string{
+				"Successfully authenticated with Kernel!",
+				"You can now use other Kernel CLI commands without setting KERNEL_API_KEY",
+			},
+			absentOutput: []string{"Authentication successful!", "ERROR"},
+		},
+		{
+			name:       "consent denial",
+			authErr:    auth.ErrAuthorizationDenied,
+			wantErr:    true,
+			wantErrors: 1,
+			wantOutput: []string{"authorization denied; no new credentials were saved"},
+			absentOutput: []string{
+				"Authentication failed",
+				"Successfully authenticated",
+			},
+		},
+		{
+			name:       "generic authentication failure",
+			authErr:    errors.New("callback server failed"),
+			wantErr:    true,
+			wantErrors: 1,
+			wantOutput: []string{"authentication failed: callback server failed"},
+			absentOutput: []string{
+				"Successfully authenticated",
+			},
+		},
+		{
+			name:       "credential save failure",
+			saveErr:    errors.New("credential store unavailable"),
+			wantErr:    true,
+			wantSave:   true,
+			wantErrors: 1,
+			wantOutput: []string{
+				"OAuth authorization completed, but credentials could not be saved: credential store unavailable",
+				"fix credential storage and run 'kernel login' again",
+			},
+			absentOutput: []string{"SUCCESS", "Successfully authenticated"},
+		},
+		{
+			name:       "cancellation",
+			authErr:    context.Canceled,
+			cancel:     true,
+			wantErr:    true,
+			wantErrors: 1,
+			wantOutput: []string{"authentication cancelled by user"},
+			absentOutput: []string{
+				"authentication failed",
+				"Successfully authenticated",
+			},
+		},
+		{
+			name:       "ambient cancellation does not mask authentication failure",
+			authErr:    errors.New("callback server failed"),
+			cancel:     true,
+			wantErr:    true,
+			wantErrors: 1,
+			wantOutput: []string{"authentication failed: callback server failed"},
+			absentOutput: []string{
+				"authentication cancelled",
+				"Successfully authenticated",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			output := capturePtermOutput(t)
+			ctx, cancel := context.WithCancel(context.Background())
+			if tt.cancel {
+				cancel()
+			} else {
+				defer cancel()
+			}
+
+			spinner := &testSpinner{}
+			saveCalled := false
+			err := completeLogin(
+				ctx,
+				spinner,
+				func(context.Context) (*auth.TokenStorage, error) {
+					return &auth.TokenStorage{}, tt.authErr
+				},
+				func(*auth.TokenStorage) error {
+					saveCalled = true
+					return tt.saveErr
+				},
+			)
+
+			if tt.wantErr {
+				require.Error(t, err)
+				renderCommandError(output, fang.Styles{
+					ErrorText: lipgloss.NewStyle(),
+					Program:   fang.Program{Flag: lipgloss.NewStyle()},
+				}, err)
+			} else {
+				require.NoError(t, err)
+			}
+
+			rendered := ansiEscapes.ReplaceAllString(output.String(), "")
+			assert.Equal(t, 1, spinner.stops)
+			assert.Equal(t, tt.wantSave, saveCalled)
+			assert.Equal(t, tt.wantSuccess, strings.Count(rendered, "SUCCESS"), rendered)
+			assert.Equal(t, tt.wantErrors, strings.Count(rendered, "ERROR"), rendered)
+			for _, want := range tt.wantOutput {
+				assert.Contains(t, rendered, want)
+			}
+			for _, absent := range tt.absentOutput {
+				assert.NotContains(t, rendered, absent)
+			}
+		})
+	}
+}
+
+func TestLoginSpinnerClearsWaitingOutput(t *testing.T) {
+	rawOutput := pterm.RawOutput
+	pterm.RawOutput = false
+	t.Cleanup(func() { pterm.RawOutput = rawOutput })
+
+	var output bytes.Buffer
+	spinner := newLoginSpinner().WithWriter(&output)
+	spinner.IsActive = true
+	spinner.UpdateText("Waiting for authentication...")
+	require.Contains(t, output.String(), "Waiting for authentication...")
+
+	require.NoError(t, spinner.Stop())
+	assert.NotContains(t, visibleTerminalOutput(output.String()), "Waiting for authentication...")
+}
+
+func visibleTerminalOutput(output string) string {
+	var visible strings.Builder
+	line := make([]rune, 0, len(output))
+	cursor := 0
+	for _, r := range output {
+		switch r {
+		case '\r':
+			cursor = 0
+		case '\n':
+			visible.WriteString(string(line))
+			visible.WriteRune('\n')
+			line = line[:0]
+			cursor = 0
+		default:
+			if cursor == len(line) {
+				line = append(line, r)
+			} else {
+				line[cursor] = r
+			}
+			cursor++
+		}
+	}
+	visible.WriteString(string(line))
+	return visible.String()
+}

cmd/root.go

@@ -221,57 +221,59 @@ func Execute(m Metadata) {
 	if err := fang.Execute(context.Background(), rootCmd,
 		fang.WithVersion(metadata.Version),
 		fang.WithCommit(metadata.Commit),
-		fang.WithErrorHandler(func(w io.Writer, styles fang.Styles, err error) {
-			err = util.CleanedUpSdkError{Err: err}
-
-			// Some subcommands intentionally suppress diagnostics for curl-like
-			// quiet modes while still returning a non-zero exit status.
-			var silent interface{ Silent() bool }
-			if errors.As(err, &silent) && silent.Silent() {
-				return
-			}
-
-			// remove margins so that it matches other pterm.error "style"
-			// we should add them back later as it looks cleaner
-			errorTextStyle := styles.ErrorText.UnsetMargins()
-
-			// Keep command errors on fang's error stream, normally stderr. This
-			// gives curl-like commands a quiet stdout for response bodies and
-			// scripts while preserving the existing pterm error styling.
-			oldErrorWriter := pterm.Error.Writer
-			pterm.Error.Writer = w
-			defer func() {
-				pterm.Error.Writer = oldErrorWriter
-			}()
-			// Fail-fast interactivity errors render one problem per line.
-			// The default ErrorText style must not apply: its width-based
-			// word-wrap splits flag tokens like --template across lines, and
-			// its transform (strings.Fields + Join) collapses the newlines
-			// between problems.
-			msg := strings.TrimSpace(err.Error())
-			style := errorTextStyle
-			var promptErr *interactive.PromptError
-			if errors.As(err, &promptErr) {
-				msg = capitalizeFirst(promptErr.Display())
-				style = style.UnsetWidth().UnsetTransform()
-			}
-			pterm.Error.Println(style.Render(msg))
-			if isUsageError(err) {
-				fmt.Fprintln(w)
-				fmt.Fprintln(w, lipgloss.JoinHorizontal(
-					lipgloss.Left,
-					errorTextStyle.UnsetWidth().Render("Try"),
-					styles.Program.Flag.Render("--help"),
-					errorTextStyle.UnsetWidth().UnsetTransform().PaddingLeft(1).Render("for usage."),
-				))
-			}
-		}),
+		fang.WithErrorHandler(renderCommandError),
 	); err != nil {
 		// fang takes care of printing the error
 		os.Exit(1)
 	}
 }
 
+func renderCommandError(w io.Writer, styles fang.Styles, err error) {
+	err = util.CleanedUpSdkError{Err: err}
+
+	// Some subcommands intentionally suppress diagnostics for curl-like
+	// quiet modes while still returning a non-zero exit status.
+	var silent interface{ Silent() bool }
+	if errors.As(err, &silent) && silent.Silent() {
+		return
+	}
+
+	// remove margins so that it matches other pterm.error "style"
+	// we should add them back later as it looks cleaner
+	errorTextStyle := styles.ErrorText.UnsetMargins()
+
+	// Keep command errors on fang's error stream, normally stderr. This
+	// gives curl-like commands a quiet stdout for response bodies and
+	// scripts while preserving the existing pterm error styling.
+	oldErrorWriter := pterm.Error.Writer
+	pterm.Error.Writer = w
+	defer func() {
+		pterm.Error.Writer = oldErrorWriter
+	}()
+	// Fail-fast interactivity errors render one problem per line.
+	// The default ErrorText style must not apply: its width-based
+	// word-wrap splits flag tokens like --template across lines, and
+	// its transform (strings.Fields + Join) collapses the newlines
+	// between problems.
+	msg := strings.TrimSpace(err.Error())
+	style := errorTextStyle
+	var promptErr *interactive.PromptError
+	if errors.As(err, &promptErr) {
+		msg = capitalizeFirst(promptErr.Display())
+		style = style.UnsetWidth().UnsetTransform()
+	}
+	pterm.Error.Println(style.Render(msg))
+	if isUsageError(err) {
+		fmt.Fprintln(w)
+		fmt.Fprintln(w, lipgloss.JoinHorizontal(
+			lipgloss.Left,
+			errorTextStyle.UnsetWidth().Render("Try"),
+			styles.Program.Flag.Render("--help"),
+			errorTextStyle.UnsetWidth().UnsetTransform().PaddingLeft(1).Render("for usage."),
+		))
+	}
+}
+
 // isUsageError is a hack to detect usage errors.
 // See: https://github.com/spf13/cobra/pull/2266
 // from github.com/charmbracelet/fang/help.go