Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions browser_flow.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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),
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This StepError update message includes the prefix "Authentication failed:", but the browser completion view also prefixes errors with "Authentication failed:". This can lead to duplicated text in the TUI (e.g., "Authentication failed: Authentication failed: …"). Prefer sending the raw error string here, or adjust the rendering to avoid double-prefixing.

Suggested change
Message: fmt.Sprintf("Authentication failed: %v", err),
Message: err.Error(),

Copilot uses AI. Check for mistakes.
}
return nil, false, fmt.Errorf("authentication failed: %w", err)
}

Expand Down
6 changes: 3 additions & 3 deletions tui/browser_model.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
Comment on lines 156 to 167
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the StepError branch you set m.err before checking update.Fallback. For a soft error (Fallback=true) this leaves m.err non-nil, so renderBrowserComplete() will take the error path and display an authentication failure instead of the fallback-to-device-flow message. Set m.err only for hard errors, or explicitly clear it when handling a fallback StepError.

Copilot uses AI. Check for mistakes.
Expand Down
2 changes: 1 addition & 1 deletion tui/browser_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
138 changes: 138 additions & 0 deletions tui/manager_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package tui

import (
"context"
"os"
"testing"
)
Expand Down Expand Up @@ -68,6 +69,41 @@
})
}

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,
Expand Down Expand Up @@ -105,6 +141,108 @@
}
}

func TestFlowUpdate_FallbackField(t *testing.T) {
t.Run("Fallback defaults to false", func(t *testing.T) {
update := FlowUpdate{Type: StepError, Message: "some hard error"}

Check failure on line 146 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Type (govet)

Check failure on line 146 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Type (govet)
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,

Check failure on line 154 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Type (govet)

Check failure on line 154 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Type (govet)
Fallback: true,
Message: "Browser authorization timed out",

Check failure on line 156 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Message (govet)

Check failure on line 156 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Message (govet)
}
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}

Check failure on line 164 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Type (govet)

Check failure on line 164 in tui/manager_test.go

View workflow job for this annotation

GitHub Actions / ubuntu-latest @ Go stable

unusedwrite: unused write to field Type (govet)
if update.Fallback {
t.Error("Fallback should not be set on non-error updates")
}
})
Comment on lines +162 to +168
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subtest "Fallback is ignored on non-error updates" is currently tautological (it sets Fallback: false and asserts it is false) and doesn’t verify any behavior. If the intent is to ensure non-error updates don’t trigger fallback handling, set Fallback: true and assert the consumer logic (e.g., handleFlowUpdate for StepStart) does not treat it as a fallback signal, or remove/rename this test.

Suggested change
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")
}
})

Copilot uses AI. Check for mistakes.
}

// 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
Expand Down
24 changes: 19 additions & 5 deletions tui/messages.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading