From c1302b770b235c5373dad6824d526ec041e038cb Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Thu, 26 Feb 2026 18:49:43 +0800 Subject: [PATCH 1/2] 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 --- browser_flow.go | 37 +++++++++++++--- tui/browser_model.go | 6 +-- tui/manager_test.go | 102 +++++++++++++++++++++++++++++++++++++++++++ tui/messages.go | 24 +++++++--- 4 files changed, 154 insertions(+), 15 deletions(-) diff --git a/browser_flow.go b/browser_flow.go index a7f1a78..53fb387 100644 --- a/browser_flow.go +++ b/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) } diff --git a/tui/browser_model.go b/tui/browser_model.go index 510ec30..ee5b21e 100644 --- a/tui/browser_model.go +++ b/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} } diff --git a/tui/manager_test.go b/tui/manager_test.go index 8c41661..29f3899 100644 --- a/tui/manager_test.go +++ b/tui/manager_test.go @@ -105,6 +105,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 diff --git a/tui/messages.go b/tui/messages.go index d66ea97..28e5d62 100644 --- a/tui/messages.go +++ b/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. From 9cd5468b0e3d440222334830e398d212773ac0a3 Mon Sep 17 00:00:00 2001 From: appleboy Date: Thu, 26 Feb 2026 20:33:33 +0800 Subject: [PATCH 2/2] 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 --- tui/browser_view.go | 2 +- tui/manager_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/tui/browser_view.go b/tui/browser_view.go index 42428c4..7ac8482 100644 --- a/tui/browser_view.go +++ b/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 diff --git a/tui/manager_test.go b/tui/manager_test.go index 29f3899..303add4 100644 --- a/tui/manager_test.go +++ b/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,