feat: distinguish soft and hard errors in flow updates (#7)

* feat: distinguish soft and hard errors in flow updates

- Introduce a Fallback field to FlowUpdate to distinguish soft errors (recoverable, trigger Device Code Flow) from hard errors (non-recoverable, surface to user).
- Update error handling logic to use the Fallback field instead of matching error messages.
- Add detailed documentation and comments clarifying error classes and flow control.
- Implement tests to verify correct handling and routing of soft and hard errors using the Fallback field.

Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>

* test: enhance error handling and test browser flow robustness

- Improve error message rendering by using only the error string.
- Add tests for SimpleManager browser flow, ensuring correct fallback signaling and update handling.

Signed-off-by: appleboy <appleboy.tw@gmail.com>

---------

Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Signed-off-by: appleboy <appleboy.tw@gmail.com>

Author: appleboy

browser_flow.go

@@ -96,10 +96,16 @@ func exchangeCode(ctx context.Context, code, codeVerifier string) (*TokenStorage
 // performBrowserFlowWithUpdates runs the Authorization Code Flow with PKCE
 // and sends progress updates through the provided channel.
 //
+// Every exit path — success, soft error, and hard error — sends at least one
+// update before returning, so the Bubble Tea TUI always receives a quit signal
+// and never hangs.
+//
 // Returns:
 //   - (storage, true, nil)  on success
-//   - (nil, false, nil)     when openBrowser() fails — caller should fall back to Device Code Flow
-//   - (nil, false, err)     on a hard error (CSRF mismatch, token exchange failure, etc.)
+//   - (nil, false, nil)     on a soft error (browser unavailable, timeout) —
+//     caller should fall back to Device Code Flow
+//   - (nil, false, err)     on a hard error (CSRF mismatch, token exchange
+//     failure, OAuth server rejection, etc.)
 func performBrowserFlowWithUpdates(
 	ctx context.Context,
 	updates chan<- tui.FlowUpdate,
@@ -113,11 +119,19 @@ func performBrowserFlowWithUpdates(
 
 	state, err := generateState()
 	if err != nil {
+		updates <- tui.FlowUpdate{
+			Type:    tui.StepError,
+			Message: fmt.Sprintf("Failed to generate state: %v", err),
+		}
 		return nil, false, fmt.Errorf("failed to generate state: %w", err)
 	}
 
 	pkce, err := GeneratePKCE()
 	if err != nil {
+		updates <- tui.FlowUpdate{
+			Type:    tui.StepError,
+			Message: fmt.Sprintf("Failed to generate PKCE parameters: %v", err),
+		}
 		return nil, false, fmt.Errorf("failed to generate PKCE: %w", err)
 	}
 
@@ -133,10 +147,11 @@ func performBrowserFlowWithUpdates(
 	}
 
 	if err := openBrowser(ctx, authURL); err != nil {
-		// Browser failed to open — signal the caller to fall back immediately.
+		// Browser failed to open — soft error, signal the caller to fall back.
 		updates <- tui.FlowUpdate{
-			Type:    tui.StepError,
-			Message: fmt.Sprintf("Could not open browser: %v", err),
+			Type:     tui.StepError,
+			Fallback: true,
+			Message:  fmt.Sprintf("Could not open browser: %v", err),
 		}
 		return nil, false, nil
 	}
@@ -195,12 +210,20 @@ func performBrowserFlowWithUpdates(
 		})
 	if err != nil {
 		if errors.Is(err, ErrCallbackTimeout) {
+			// Soft error — fall back to Device Code Flow silently.
 			updates <- tui.FlowUpdate{
-				Type:    tui.StepError,
-				Message: "Browser authorization timed out",
+				Type:     tui.StepError,
+				Fallback: true,
+				Message:  "Browser authorization timed out",
 			}
 			return nil, false, nil
 		}
+		// Hard error (CSRF mismatch, token exchange failure, OAuth rejection,
+		// etc.) — surface it to the user.
+		updates <- tui.FlowUpdate{
+			Type:    tui.StepError,
+			Message: fmt.Sprintf("Authentication failed: %v", err),
+		}
 		return nil, false, fmt.Errorf("authentication failed: %w", err)
 	}
 

tui/browser_model.go

@@ -155,13 +155,13 @@ func (m *BrowserModel) handleFlowUpdate(update FlowUpdate) tea.Cmd {
 
 	case StepError:
 		m.err = fmt.Errorf("%s", update.Message)
-		if update.Message == "Browser authorization timed out" ||
-			update.Message == "Could not open browser: exit status 1" {
-			// Fallback to device flow
+		if update.Fallback {
+			// Soft error — caller requested fall back to Device Code Flow.
 			return func() tea.Msg {
 				return successMsg{storage: nil, ok: false}
 			}
 		}
+		// Hard error — surface it to the user.
 		return func() tea.Msg {
 			return errorMsg{err: m.err}
 		}

tui/browser_view.go

@@ -72,7 +72,7 @@ func renderBrowserComplete(m *BrowserModel) string {
 	case m.err != nil:
 		// Error state
 		b.WriteString("\n")
-		b.WriteString(RenderError(fmt.Sprintf("Authentication failed: %v", m.err)))
+		b.WriteString(RenderError(m.err.Error()))
 		b.WriteString("\n\n")
 	case !m.ok:
 		// Fallback to device flow

tui/manager_test.go

@@ -1,6 +1,7 @@
 package tui
 
 import (
+	"context"
 	"os"
 	"testing"
 )
@@ -68,6 +69,41 @@ func TestSelectManager(t *testing.T) {
 	})
 }
 
+func TestSimpleManagerRunBrowserFlow(t *testing.T) {
+	t.Run("Non-error updates do not trigger fallback", func(t *testing.T) {
+		m := NewSimpleManager()
+		perform := func(ctx context.Context, updates chan<- FlowUpdate) (*TokenStorage, bool, error) {
+			updates <- FlowUpdate{Type: StepStart, Step: 1}
+			updates <- FlowUpdate{Type: BrowserOpened}
+			updates <- FlowUpdate{Type: StepStart, Step: 2}
+			updates <- FlowUpdate{Type: CallbackReceived}
+			return &TokenStorage{AccessToken: "test-token"}, true, nil
+		}
+		_, ok, err := m.RunBrowserFlow(context.Background(), perform)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if !ok {
+			t.Error("non-error updates must not trigger fallback: expected ok=true")
+		}
+	})
+
+	t.Run("perform returning ok=false is propagated as fallback signal", func(t *testing.T) {
+		m := NewSimpleManager()
+		perform := func(ctx context.Context, updates chan<- FlowUpdate) (*TokenStorage, bool, error) {
+			updates <- FlowUpdate{Type: StepError, Message: "browser failed"}
+			return nil, false, nil
+		}
+		_, ok, err := m.RunBrowserFlow(context.Background(), perform)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if ok {
+			t.Error("expected ok=false when perform signals fallback needed")
+		}
+	})
+}
+
 func TestFlowUpdateHelpers(t *testing.T) {
 	update := FlowUpdate{
 		Type:    StepStart,
@@ -105,6 +141,108 @@ func TestFlowUpdateHelpers(t *testing.T) {
 	}
 }
 
+func TestFlowUpdate_FallbackField(t *testing.T) {
+	t.Run("Fallback defaults to false", func(t *testing.T) {
+		update := FlowUpdate{Type: StepError, Message: "some hard error"}
+		if update.Fallback {
+			t.Error("Fallback should default to false for hard errors")
+		}
+	})
+
+	t.Run("Fallback can be set to true for soft errors", func(t *testing.T) {
+		update := FlowUpdate{
+			Type:     StepError,
+			Fallback: true,
+			Message:  "Browser authorization timed out",
+		}
+		if !update.Fallback {
+			t.Error("Fallback should be true for soft (recoverable) errors")
+		}
+	})
+
+	t.Run("Fallback is ignored on non-error updates", func(t *testing.T) {
+		update := FlowUpdate{Type: StepStart, Step: 1, Fallback: false}
+		if update.Fallback {
+			t.Error("Fallback should not be set on non-error updates")
+		}
+	})
+}
+
+// TestBrowserModel_HandleFlowUpdate_StepError verifies that StepError updates
+// are routed to the correct internal message type based on the Fallback field,
+// so the Bubble Tea program always receives a quit signal (no hanging).
+func TestBrowserModel_HandleFlowUpdate_StepError(t *testing.T) {
+	t.Run("hard error returns errorMsg", func(t *testing.T) {
+		updates := make(chan FlowUpdate, 1)
+		model := NewBrowserModel(updates, nil)
+
+		cmd := model.handleFlowUpdate(FlowUpdate{
+			Type:    StepError,
+			Message: "Authentication failed: state_mismatch: state parameter mismatch",
+		})
+
+		if cmd == nil {
+			t.Fatal("expected non-nil tea.Cmd for hard error")
+		}
+		msg := cmd()
+		if _, ok := msg.(errorMsg); !ok {
+			t.Errorf("expected errorMsg for hard error, got %T", msg)
+		}
+	})
+
+	t.Run("soft error with Fallback=true returns successMsg{ok:false}", func(t *testing.T) {
+		updates := make(chan FlowUpdate, 1)
+		model := NewBrowserModel(updates, nil)
+
+		cmd := model.handleFlowUpdate(FlowUpdate{
+			Type:     StepError,
+			Fallback: true,
+			Message:  "Browser authorization timed out",
+		})
+
+		if cmd == nil {
+			t.Fatal("expected non-nil tea.Cmd for soft error")
+		}
+		msg := cmd()
+		sm, ok := msg.(successMsg)
+		if !ok {
+			t.Errorf("expected successMsg for soft error, got %T", msg)
+		}
+		if sm.ok {
+			t.Error("successMsg.ok should be false for fallback (device flow signal)")
+		}
+		if sm.storage != nil {
+			t.Error("successMsg.storage should be nil for fallback")
+		}
+	})
+
+	t.Run(
+		"browser open failure with Fallback=true returns successMsg{ok:false}",
+		func(t *testing.T) {
+			updates := make(chan FlowUpdate, 1)
+			model := NewBrowserModel(updates, nil)
+
+			cmd := model.handleFlowUpdate(FlowUpdate{
+				Type:     StepError,
+				Fallback: true,
+				Message:  "Could not open browser: exec: no command",
+			})
+
+			if cmd == nil {
+				t.Fatal("expected non-nil tea.Cmd")
+			}
+			msg := cmd()
+			sm, ok := msg.(successMsg)
+			if !ok {
+				t.Errorf("expected successMsg, got %T", msg)
+			}
+			if sm.ok {
+				t.Error("successMsg.ok should be false for fallback")
+			}
+		},
+	)
+}
+
 func TestFlowUpdateTypeString(t *testing.T) {
 	tests := []struct {
 		updateType FlowUpdateType

tui/messages.go

@@ -19,13 +19,27 @@ const (
 )
 
 // FlowUpdate represents a progress update message from an OAuth flow.
+//
+// For StepError updates, the Fallback field distinguishes between two
+// classes of failure:
+//
+//   - Fallback == true  → soft error (browser unavailable, user timeout).
+//     The caller should silently retry with the Device Code Flow.
+//
+//   - Fallback == false → hard error (CSRF mismatch, token exchange failure,
+//     OAuth server rejection, etc.).
+//     The caller should surface the error to the user and exit.
 type FlowUpdate struct {
 	Type       FlowUpdateType
-	Step       int            // Current step number (1-indexed)
-	TotalSteps int            // Total number of steps
-	Message    string         // Human-readable message
-	Progress   float64        // Progress percentage (0.0 to 1.0)
-	Data       map[string]any // Additional data for specific update types
+	Step       int     // Current step number (1-indexed)
+	TotalSteps int     // Total number of steps
+	Message    string  // Human-readable message
+	Progress   float64 // Progress percentage (0.0 to 1.0)
+	// Fallback is only meaningful when Type == StepError.
+	// When true, the error is recoverable and the caller should fall back
+	// to the Device Code Flow instead of reporting a failure.
+	Fallback bool
+	Data     map[string]any // Additional data for specific update types
 }
 
 // String returns a human-readable representation of the FlowUpdateType.