Handle OAuth denial and deduplicate login output (#260)

## Summary

OAuth consent denial returns `error=access_denied` without a code. The
CLI previously treated that valid error response as a malformed success
callback, showing “missing authorization code” in the browser. Native
login also emitted duplicate terminal statuses and reported success
before credentials were persisted.

- Handle OAuth errors after state validation and before requiring a
code. Denial returns a distinct error without entering token exchange.
- Reuse the callback page layout for a clear branded denial page without
a success checkmark. Provider-supplied error descriptions are not
reflected.
- Stop and erase the waiting spinner without printing a second status.
The root renderer now emits the single meaningful denial, cancellation,
or authentication error with a nonzero exit.
- Classify cancellation from the authentication error itself so a racing
context cancellation cannot mask an unrelated failure.
- Print one success only after credentials are saved. Credential-save
failures retain the storage error and reauthentication guidance and now
exit nonzero.
- Preserve existing stored credentials on denial.
- Extract the callback and root error handlers for focused
rendered-output regression coverage.

## Validation

- `go test ./pkg/auth ./cmd -count=1`
- `go test -race ./pkg/auth -count=1`
- `go vet ./pkg/auth ./cmd`
- `go build ./...`
- Focused output tests cover success, consent denial, generic
authentication failure, cancellation races, credential-save failure, and
the real spinner erase boundary.
- Earlier built-binary loopback fixture: simulated browser launch and
denial callback produced one clear terminal error, exit code 1, zero
token requests, and unchanged fixture credentials. This used isolated
fixture configuration with keyring access disabled, not a live provider
login.
- Chromium rendering at desktop and mobile widths: denial text, embedded
favicon, no success checkmark, and no horizontal overflow. Existing
callback layout and colors are retained.
- Build, test, Semgrep, Socket Security, and BugBot checks pass on
`d99e4da9b708e119211d76dec6e5047305d8858f`.

This is not released; a live denial retest is still pending after
release.

---------

Co-authored-by: rgarcia <72655+rgarcia@users.noreply.github.com>

Author: rgarcia

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