diff --git a/forge-cli/internal/tui/components/egress_display.go b/forge-cli/internal/tui/components/egress_display.go index 5d03dcf3..5d812f62 100644 --- a/forge-cli/internal/tui/components/egress_display.go +++ b/forge-cli/internal/tui/components/egress_display.go @@ -32,8 +32,7 @@ func NewEgressDisplay(domains []EgressDomain, primaryStyle, dimStyle, borderStyl kbd := NewKbdHint(kbdKeyStyle, kbdDescStyle) kbd.Bindings = []KeyBinding{ {Key: "⏎", Desc: "accept"}, - {Key: "backspace", Desc: "back"}, - {Key: "esc", Desc: "quit"}, + {Key: "esc", Desc: "back"}, } return EgressDisplay{ diff --git a/forge-cli/internal/tui/components/kbd_hint.go b/forge-cli/internal/tui/components/kbd_hint.go index eff68481..c75a5390 100644 --- a/forge-cli/internal/tui/components/kbd_hint.go +++ b/forge-cli/internal/tui/components/kbd_hint.go @@ -42,7 +42,7 @@ func SelectHints() []KeyBinding { return []KeyBinding{ {Key: "↑↓", Desc: "navigate"}, {Key: "⏎", Desc: "select"}, - {Key: "esc", Desc: "quit"}, + {Key: "esc", Desc: "back"}, } } @@ -52,7 +52,7 @@ func MultiSelectHints() []KeyBinding { {Key: "↑↓", Desc: "navigate"}, {Key: "space", Desc: "toggle"}, {Key: "⏎", Desc: "confirm"}, - {Key: "esc", Desc: "quit"}, + {Key: "esc", Desc: "back"}, } } @@ -60,15 +60,17 @@ func MultiSelectHints() []KeyBinding { func InputHints() []KeyBinding { return []KeyBinding{ {Key: "⏎", Desc: "submit"}, - {Key: "esc", Desc: "quit"}, + {Key: "esc", Desc: "back"}, } } -// ReviewHints returns standard hints for the review step. +// ReviewHints returns standard hints for the review step. Only `esc` is +// advertised for back: wizard-level esc supersedes the step-local backspace +// (StepBackMsg) path, so showing both would imply two behaviors where there's +// one. See #264 review. func ReviewHints() []KeyBinding { return []KeyBinding{ {Key: "⏎", Desc: "confirm"}, - {Key: "backspace", Desc: "back"}, - {Key: "esc", Desc: "quit"}, + {Key: "esc", Desc: "back"}, } } diff --git a/forge-cli/internal/tui/components/multi_select.go b/forge-cli/internal/tui/components/multi_select.go index 15dd2031..f8626575 100644 --- a/forge-cli/internal/tui/components/multi_select.go +++ b/forge-cli/internal/tui/components/multi_select.go @@ -118,18 +118,11 @@ func (m MultiSelect) Update(msg tea.Msg) (MultiSelect, tea.Cmd) { case " ": m.Items[m.cursor].Checked = !m.Items[m.cursor].Checked case "enter": - // If nothing is checked, auto-check the cursor item so the user - // doesn't have to remember Space+Enter for a single selection. - anyChecked := false - for _, item := range m.Items { - if item.Checked { - anyChecked = true - break - } - } - if !anyChecked && len(m.Items) > 0 { - m.Items[m.cursor].Checked = true - } + // Enter confirms exactly what Space has toggled — including an + // empty selection. It must NOT auto-check the cursor item: the + // cursor is just the highlight, and steps like Skills / Fallback + // legitimately allow selecting nothing. (Previously Enter with + // nothing checked silently selected whatever row was highlighted.) m.done = true } } diff --git a/forge-cli/internal/tui/components/multi_select_test.go b/forge-cli/internal/tui/components/multi_select_test.go new file mode 100644 index 00000000..241ebcd2 --- /dev/null +++ b/forge-cli/internal/tui/components/multi_select_test.go @@ -0,0 +1,53 @@ +package components + +import ( + "testing" + + tea "github.com/charmbracelet/bubbletea" +) + +func twoItemSelect() MultiSelect { + return MultiSelect{Items: []MultiSelectItem{ + {Label: "alpha", Value: "alpha"}, + {Label: "beta", Value: "beta"}, + }} +} + +// TestMultiSelect_EnterConfirmsEmpty pins the fix: pressing Enter with +// nothing toggled confirms an EMPTY selection instead of silently +// auto-selecting the highlighted row. +func TestMultiSelect_EnterConfirmsEmpty(t *testing.T) { + m := twoItemSelect() + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + if !m.Done() { + t.Fatal("Enter should confirm the selection") + } + if vals := m.SelectedValues(); len(vals) != 0 { + t.Errorf("Enter with nothing toggled must select nothing, got %v", vals) + } +} + +// TestMultiSelect_SpaceThenEnterSelects confirms Space still toggles the +// cursor row and Enter confirms it. +func TestMultiSelect_SpaceThenEnterSelects(t *testing.T) { + m := twoItemSelect() + m, _ = m.Update(tea.KeyMsg{Type: tea.KeySpace}) // toggle cursor row (alpha) + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + vals := m.SelectedValues() + if len(vals) != 1 || vals[0] != "alpha" { + t.Errorf("Space+Enter should select 'alpha', got %v", vals) + } +} + +// TestMultiSelect_MoveThenSpaceSelectsCorrectRow guards that toggling +// follows the cursor, not row 0. +func TestMultiSelect_MoveThenSpaceSelectsCorrectRow(t *testing.T) { + m := twoItemSelect() + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyDown}) // cursor -> beta + m, _ = m.Update(tea.KeyMsg{Type: tea.KeySpace}) // toggle beta + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + vals := m.SelectedValues() + if len(vals) != 1 || vals[0] != "beta" { + t.Errorf("expected only 'beta', got %v", vals) + } +} diff --git a/forge-cli/internal/tui/steps/channel_step.go b/forge-cli/internal/tui/steps/channel_step.go index 0f025b16..3f35a2bb 100644 --- a/forge-cli/internal/tui/steps/channel_step.go +++ b/forge-cli/internal/tui/steps/channel_step.go @@ -104,6 +104,11 @@ func (s *ChannelStep) Title() string { return "Channel Connector" } func (s *ChannelStep) Icon() string { return "📡" } func (s *ChannelStep) Init() tea.Cmd { + // Reset on (re-)entry so BACK navigation doesn't land on a completed step + // whose Update short-circuits on `s.complete` and strands the wizard. + // Re-entry contract — SkillsStep/CompressionStep precedent (#264 review). + s.complete = false + s.phase = channelSelectPhase return s.selector.Init() } @@ -553,6 +558,11 @@ func (s *ChannelStep) Summary() string { func (s *ChannelStep) Apply(ctx *tui.WizardContext) { ctx.Channel = s.channel + // Write current state, not accumulate: back-navigation + choosing a + // different channel must not leave the prior channel's tokens behind. + // ChannelStep exclusively owns ChannelTokens, so a full reset is safe. + // See #264 review. + clear(ctx.ChannelTokens) for k, v := range s.tokens { ctx.ChannelTokens[k] = v } diff --git a/forge-cli/internal/tui/steps/fallback_step.go b/forge-cli/internal/tui/steps/fallback_step.go index 543cede1..57f59620 100644 --- a/forge-cli/internal/tui/steps/fallback_step.go +++ b/forge-cli/internal/tui/steps/fallback_step.go @@ -57,6 +57,17 @@ func (s *FallbackStep) Title() string { return "Fallback Providers" } func (s *FallbackStep) Icon() string { return "🔄" } func (s *FallbackStep) Init() tea.Cmd { + // Reset on (re-)entry. Prepare() resets these too, but the wizard only + // calls Prepare on FORWARD advance — BACK navigation calls Init() alone, + // so without this a completed step short-circuits Update and strands the + // wizard. Re-entry contract (#264 review). + s.complete = false + s.validating = false + s.phase = fallbackAskPhase + s.selected = nil + s.collected = nil + s.keyIndex = 0 + // Build the "Add fallback providers?" selector items := []components.SingleSelectItem{ {Label: "No", Value: "no", Description: "Use only the primary provider", Icon: "⏭️"}, diff --git a/forge-cli/internal/tui/steps/name_step.go b/forge-cli/internal/tui/steps/name_step.go index 957f7f1b..09e55fd8 100644 --- a/forge-cli/internal/tui/steps/name_step.go +++ b/forge-cli/internal/tui/steps/name_step.go @@ -60,6 +60,9 @@ func (s *NameStep) Init() tea.Cmd { s.name = s.prefill return func() tea.Msg { return tui.StepCompleteMsg{} } } + // Reset on BACK navigation so the completed step doesn't short-circuit + // Update and strand the wizard. Re-entry contract (#264 review). + s.complete = false return s.input.Init() } diff --git a/forge-cli/internal/tui/steps/provider_step.go b/forge-cli/internal/tui/steps/provider_step.go index 37914993..b978a35a 100644 --- a/forge-cli/internal/tui/steps/provider_step.go +++ b/forge-cli/internal/tui/steps/provider_step.go @@ -143,6 +143,12 @@ func (s *ProviderStep) Title() string { return "Model Provider" } func (s *ProviderStep) Icon() string { return "🤖" } func (s *ProviderStep) Init() tea.Cmd { + // Init also runs on BACK navigation into this step. Reset to the initial + // phase so a completed step doesn't short-circuit Update (which begins + // with `if s.complete`) and strand the wizard. Re-entry contract — + // SkillsStep/CompressionStep precedent (#264 review). + s.complete = false + s.phase = providerSelectPhase return s.selector.Init() } diff --git a/forge-cli/internal/tui/steps/reentry_test.go b/forge-cli/internal/tui/steps/reentry_test.go new file mode 100644 index 00000000..2dab6b81 --- /dev/null +++ b/forge-cli/internal/tui/steps/reentry_test.go @@ -0,0 +1,124 @@ +package steps + +import ( + "testing" + + "github.com/initializ/forge/forge-cli/internal/tui" +) + +// TestStepInitResetsComplete pins the re-entry contract from the #264 review: +// every step's Init() must reset `complete` to false, because the wizard calls +// Init() (not Prepare) on BACK navigation and every step's Update short- +// circuits on `if s.complete`. Without the reset, esc-back lands on an inert +// step that ignores all input and soft-locks the wizard. +// +// The test drives REAL steps (a stateless mock can't catch the soft-lock — +// see the wizard_nav_test note): construct the step, force it complete, call +// Init(), and assert it's usable again. +func TestStepInitResetsComplete(t *testing.T) { + styles := tui.NewStyleSet(tui.DarkTheme) + noKey := func(string, string) error { return nil } + + t.Run("name", func(t *testing.T) { + s := NewNameStep(styles, "") // no prefill + s.complete = true + s.Init() + if s.complete { + t.Error("NameStep.Init() must reset complete") + } + }) + t.Run("provider", func(t *testing.T) { + s := NewProviderStep(styles, noKey) + s.complete = true + s.Init() + if s.complete { + t.Error("ProviderStep.Init() must reset complete") + } + }) + t.Run("channel", func(t *testing.T) { + s := NewChannelStep(styles) + s.complete = true + s.Init() + if s.complete { + t.Error("ChannelStep.Init() must reset complete") + } + }) + t.Run("web_search", func(t *testing.T) { + s := NewWebSearchStep(styles, noKey) + s.complete = true + s.Init() + if s.complete { + t.Error("WebSearchStep.Init() must reset complete") + } + }) + t.Run("fallback", func(t *testing.T) { + s := NewFallbackStep(styles, noKey) + s.complete = true + s.Init() + if s.complete { + t.Error("FallbackStep.Init() must reset complete") + } + }) + t.Run("auth", func(t *testing.T) { + s := NewAuthStep(styles) + s.complete = true + s.Init() + if s.complete { + t.Error("AuthStep.Init() must reset complete") + } + }) + t.Run("compression", func(t *testing.T) { + s := NewCompressionStep(styles) + s.complete = true + s.Init() + if s.complete { + t.Error("CompressionStep.Init() must reset complete") + } + }) +} + +// TestWebSearchStepApply_ClearsStaleKeys pins finding #2: redoing the +// web-search step (after back-nav) with "No web search" chosen must not leak +// the previously-entered provider/key into the context. +func TestWebSearchStepApply_ClearsStaleKeys(t *testing.T) { + styles := tui.NewStyleSet(tui.DarkTheme) + ctx := tui.NewWizardContext() + + // First pass: Tavily chosen with a key. + s := NewWebSearchStep(styles, func(string, string) error { return nil }) + s.provider = "tavily" + s.keyName = "TAVILY_API_KEY" + s.key = "tvly-x" + s.Apply(ctx) + if ctx.EnvVars["TAVILY_API_KEY"] == "" || ctx.EnvVars["WEB_SEARCH_PROVIDER"] == "" { + t.Fatal("first Apply should have written the web-search env vars") + } + if !containsWebSearch(ctx.BuiltinTools) { + t.Fatal("first Apply should have recorded web_search") + } + + // Redo: user went back and chose "No web search". + s.provider = "" + s.keyName = "" + s.key = "" + s.Apply(ctx) + + if _, ok := ctx.EnvVars["TAVILY_API_KEY"]; ok { + t.Error("stale TAVILY_API_KEY leaked after choosing No web search") + } + if _, ok := ctx.EnvVars["WEB_SEARCH_PROVIDER"]; ok { + t.Error("stale WEB_SEARCH_PROVIDER leaked after choosing No web search") + } + if containsWebSearch(ctx.BuiltinTools) { + t.Error("stale web_search left in BuiltinTools after deselect") + } +} + +func containsWebSearch(s []string) bool { + for _, v := range s { + if v == "web_search" { + return true + } + } + return false +} diff --git a/forge-cli/internal/tui/steps/review_step.go b/forge-cli/internal/tui/steps/review_step.go index 080a882f..2b7e4467 100644 --- a/forge-cli/internal/tui/steps/review_step.go +++ b/forge-cli/internal/tui/steps/review_step.go @@ -86,6 +86,11 @@ func (s *ReviewStep) Title() string { return "Review & Generate" } func (s *ReviewStep) Icon() string { return "🚀" } func (s *ReviewStep) Init() tea.Cmd { + // Review is the terminal step (confirming it quits the wizard), so it is + // not normally re-entered via BACK — but reset `complete` anyway to honor + // the uniform re-entry contract (#264 review). `prepared` is left intact + // so the rendered summary survives. + s.complete = false return nil } diff --git a/forge-cli/internal/tui/steps/step_auth.go b/forge-cli/internal/tui/steps/step_auth.go index d7e2daa0..d32851a5 100644 --- a/forge-cli/internal/tui/steps/step_auth.go +++ b/forge-cli/internal/tui/steps/step_auth.go @@ -137,7 +137,14 @@ func NewAuthStep(styles *tui.StyleSet) *AuthStep { func (s *AuthStep) Title() string { return "Authentication" } func (s *AuthStep) Icon() string { return "🔐" } -func (s *AuthStep) Init() tea.Cmd { return s.selector.Init() } +func (s *AuthStep) Init() tea.Cmd { + // Reset on (re-)entry so BACK navigation restarts at the provider choice + // instead of short-circuiting on a stale `s.complete`. Re-entry contract + // (#264 review). + s.complete = false + s.phase = authSelectPhase + return s.selector.Init() +} func (s *AuthStep) Update(msg tea.Msg) (tui.Step, tea.Cmd) { if s.complete { diff --git a/forge-cli/internal/tui/steps/web_search_step.go b/forge-cli/internal/tui/steps/web_search_step.go index df48bf06..f8ccf19e 100644 --- a/forge-cli/internal/tui/steps/web_search_step.go +++ b/forge-cli/internal/tui/steps/web_search_step.go @@ -84,6 +84,12 @@ func (s *WebSearchStep) Title() string { return "Web Search" } func (s *WebSearchStep) Icon() string { return "🔍" } func (s *WebSearchStep) Init() tea.Cmd { + // Reset on (re-)entry so BACK navigation restarts at the provider choice + // instead of short-circuiting on a stale `s.complete`. Re-entry contract + // — SkillsStep/CompressionStep precedent (#264 review). + s.complete = false + s.validating = false + s.phase = webSearchChoosePhase return s.choose.Init() } @@ -246,6 +252,17 @@ func (s *WebSearchStep) Summary() string { } func (s *WebSearchStep) Apply(ctx *tui.WizardContext) { + // Apply must write CURRENT state, not accumulate. Back-navigation makes + // redo reachable: choosing a provider + key, then going back and picking + // "No web search", must not leave the stale key / provider in the context + // (they'd land in the generated .env for a tool the user dropped). Clear + // the keys this step exclusively owns first, then re-write the current + // selection. See #264 review. + delete(ctx.EnvVars, "WEB_SEARCH_PROVIDER") + delete(ctx.EnvVars, "TAVILY_API_KEY") + delete(ctx.EnvVars, "PERPLEXITY_API_KEY") + ctx.BuiltinTools = removeStr(ctx.BuiltinTools, "web_search") + if s.provider == "" { return } @@ -257,3 +274,15 @@ func (s *WebSearchStep) Apply(ctx *tui.WizardContext) { ctx.EnvVars[s.keyName] = s.key } } + +// removeStr returns slice with all occurrences of val removed, preserving +// order. Used so Apply can rewrite BuiltinTools idempotently on redo. +func removeStr(slice []string, val string) []string { + out := slice[:0:0] + for _, s := range slice { + if s != val { + out = append(out, s) + } + } + return out +} diff --git a/forge-cli/internal/tui/wizard.go b/forge-cli/internal/tui/wizard.go index 25e461f0..2af84942 100644 --- a/forge-cli/internal/tui/wizard.go +++ b/forge-cli/internal/tui/wizard.go @@ -144,7 +144,20 @@ func (w WizardModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return w, nil case tea.KeyMsg: - if msg.String() == "ctrl+c" || msg.String() == "esc" { + if msg.String() == "ctrl+c" { + w.err = fmt.Errorf("wizard cancelled") + return w, tea.Quit + } + // esc is global back-navigation: return to the previous step (re-Init + // so it starts fresh). Handled at the wizard level so it works from + // EVERY step regardless of the step's own sub-phase or text inputs — + // esc is never an editing key, so intercepting it here is safe. On + // the first step there's nowhere to go back to, so esc cancels. + if msg.String() == "esc" { + if w.current > 0 { + w.current-- + return w, w.steps[w.current].Init() + } w.err = fmt.Errorf("wizard cancelled") return w, tea.Quit } diff --git a/forge-cli/internal/tui/wizard_nav_test.go b/forge-cli/internal/tui/wizard_nav_test.go new file mode 100644 index 00000000..c4ebd38c --- /dev/null +++ b/forge-cli/internal/tui/wizard_nav_test.go @@ -0,0 +1,132 @@ +package tui + +import ( + "testing" + + tea "github.com/charmbracelet/bubbletea" +) + +// navMockStep is a STATEFUL Step for driving the wizard in tests. It mirrors +// the real steps' contract so back-navigation bugs are visible (a stateless +// mock that never completes would only exercise index arithmetic — see the +// #264 review): +// - Update short-circuits once complete (`if s.complete { return }`), the +// same guard every real step's Update opens with; +// - a space key completes the step and signals advancement; +// - Init() resets to the initial (incomplete) state — the re-entry contract +// the wizard relies on when it calls Init() on BACK navigation. +type navMockStep struct { + n string + complete bool +} + +func (s *navMockStep) Title() string { return s.n } +func (s *navMockStep) Icon() string { return "" } +func (s *navMockStep) Init() tea.Cmd { s.complete = false; return nil } +func (s *navMockStep) Update(msg tea.Msg) (Step, tea.Cmd) { + if s.complete { + return s, nil // inert once done — exactly the real-step guard + } + if k, ok := msg.(tea.KeyMsg); ok && k.Type == tea.KeySpace { + s.complete = true + return s, func() tea.Msg { return StepCompleteMsg{} } + } + return s, nil +} +func (s *navMockStep) View(int) string { return "" } +func (s *navMockStep) Complete() bool { return s.complete } +func (s *navMockStep) Summary() string { return "" } +func (s *navMockStep) Apply(*WizardContext) {} + +// TestWizard_EscGoesBack pins the global back-navigation: esc returns to +// the previous step from anywhere. +func TestWizard_EscGoesBack(t *testing.T) { + w := NewWizardModel(TermTheme{}, []Step{&navMockStep{n: "a"}, &navMockStep{n: "b"}, &navMockStep{n: "c"}}, "v") + + // Advance to the third step (index 2). + m, _ := w.Update(StepCompleteMsg{}) + w = m.(WizardModel) + m, _ = w.Update(StepCompleteMsg{}) + w = m.(WizardModel) + if w.current != 2 { + t.Fatalf("setup: current=%d want 2", w.current) + } + + // esc -> back to step 1. + m, _ = w.Update(tea.KeyMsg{Type: tea.KeyEsc}) + w = m.(WizardModel) + if w.current != 1 { + t.Errorf("esc should go back to step 1, got %d", w.current) + } + // esc again -> back to step 0. + m, _ = w.Update(tea.KeyMsg{Type: tea.KeyEsc}) + w = m.(WizardModel) + if w.current != 0 { + t.Errorf("esc should go back to step 0, got %d", w.current) + } + if w.err != nil { + t.Errorf("back-navigation must not cancel the wizard: %v", w.err) + } +} + +// TestWizard_EscAtFirstStepCancels — esc with nowhere to go back cancels. +func TestWizard_EscAtFirstStepCancels(t *testing.T) { + w := NewWizardModel(TermTheme{}, []Step{&navMockStep{n: "a"}, &navMockStep{n: "b"}}, "v") + m, _ := w.Update(tea.KeyMsg{Type: tea.KeyEsc}) + w = m.(WizardModel) + if w.err == nil { + t.Error("esc at the first step should cancel the wizard") + } +} + +// TestWizard_BackReEntersUsableStep is the #264 blocking-finding regression: +// esc-back must land on a USABLE step, not an inert completed one. It drives a +// real completion so the step's `complete` guard is live, then esc-backs into +// it and re-completes — the exact flow the feature exists for. +func TestWizard_BackReEntersUsableStep(t *testing.T) { + stepB := &navMockStep{n: "b"} + w := NewWizardModel(TermTheme{}, []Step{&navMockStep{n: "a"}, stepB, &navMockStep{n: "c"}}, "v") + + // Advance onto step b (index 1). + m, _ := w.Update(StepCompleteMsg{}) + w = m.(WizardModel) + if w.current != 1 { + t.Fatalf("setup: current=%d want 1", w.current) + } + + // Complete step b (space), then advance onto step c. + m, _ = w.Update(tea.KeyMsg{Type: tea.KeySpace}) + w = m.(WizardModel) + if !stepB.complete { + t.Fatal("space should complete step b") + } + m, _ = w.Update(StepCompleteMsg{}) + w = m.(WizardModel) + if w.current != 2 { + t.Fatalf("should be on step c (2), got %d", w.current) + } + + // esc back into step b. The wizard must call Init(), which resets the + // step so it's no longer inert. THIS is the soft-lock assertion: without + // the reset, stepB.complete stays true and every input is ignored. + m, _ = w.Update(tea.KeyMsg{Type: tea.KeyEsc}) + w = m.(WizardModel) + if w.current != 1 { + t.Fatalf("esc should return to step b (1), got %d", w.current) + } + if stepB.complete { + t.Fatal("SOFT-LOCK: esc-back re-entered step b without resetting complete") + } + + // And it accepts input again — re-completes and re-advances. + m, _ = w.Update(tea.KeyMsg{Type: tea.KeySpace}) + w = m.(WizardModel) + if !stepB.complete { + t.Error("step b should accept input after back-navigation") + } + m, _ = w.Update(StepCompleteMsg{}) + w = m.(WizardModel) + if w.current != 2 { + t.Errorf("re-completing step b should advance to c again, got %d", w.current) + } +}