diff --git a/forge-cli/cmd/init.go b/forge-cli/cmd/init.go index 6956820d..5500ce06 100644 --- a/forge-cli/cmd/init.go +++ b/forge-cli/cmd/init.go @@ -241,16 +241,6 @@ func collectInteractive(opts *initOptions) error { theme := tui.DetectTheme(themeOverride) styles := tui.NewStyleSet(theme) - // Load tool info for the tools step - allTools := builtins.All() - var toolInfos []steps.ToolInfo - for _, t := range allTools { - toolInfos = append(toolInfos, steps.ToolInfo{ - Name: t.Name(), - Description: t.Description(), - }) - } - // Load skill info for the skills step var skillInfos []steps.SkillInfo reg, regErr := local.NewEmbeddedRegistry() @@ -325,7 +315,7 @@ func collectInteractive(opts *initOptions) error { steps.NewProviderStep(styles, validateKeyFn, oauthFlowFn), steps.NewFallbackStep(styles, validateKeyFn), steps.NewChannelStep(styles), - steps.NewToolsStep(styles, toolInfos, validateWebSearchKeyFn), + steps.NewWebSearchStep(styles, validateWebSearchKeyFn), steps.NewSkillsStep(styles, skillInfos), steps.NewCompressionStep(styles), steps.NewAuthStep(styles), @@ -375,7 +365,13 @@ func collectInteractive(opts *initOptions) error { opts.Channels = []string{ctx.Channel} } - opts.BuiltinTools = ctx.BuiltinTools + // MERGE the wizard's builtin-tool selection into any --tools flag rather + // than overwriting it. The wizard only records web_search today; a bare + // overwrite would drop the flag's other entries from both forge.yaml and + // the init_egress DefaultToolDomains derivation whenever the user picked a + // provider. Merge + dedupe covers all three paths (flag+skip, flag+ + // provider, no-flag+provider). See #263 review. + opts.BuiltinTools = mergeBuiltinTools(opts.BuiltinTools, ctx.BuiltinTools) opts.Skills = ctx.Skills opts.Compression = ctx.Compression @@ -1417,6 +1413,27 @@ func containsStr(slice []string, val string) bool { return false } +// mergeBuiltinTools returns the union of a --tools flag value and the wizard's +// recorded builtin-tool selection, preserving first-seen order and dropping +// duplicates and empty entries. Merging (not overwriting) keeps a flag entry +// like http_request when the wizard also records web_search โ€” both must reach +// forge.yaml and the init_egress domain derivation. See #263 review. +func mergeBuiltinTools(flag, fromWizard []string) []string { + seen := make(map[string]struct{}, len(flag)+len(fromWizard)) + out := make([]string, 0, len(flag)+len(fromWizard)) + for _, name := range append(append([]string{}, flag...), fromWizard...) { + if name == "" { + continue + } + if _, dup := seen[name]; dup { + continue + } + seen[name] = struct{}{} + out = append(out, name) + } + return out +} + // runOAuthFlow executes the OAuth browser flow for a provider and returns the access token. func runOAuthFlow(provider string) (string, error) { var config oauth.ProviderConfig diff --git a/forge-cli/cmd/init_test.go b/forge-cli/cmd/init_test.go index f6463131..50ba3a08 100644 --- a/forge-cli/cmd/init_test.go +++ b/forge-cli/cmd/init_test.go @@ -951,3 +951,35 @@ func writeTempFile(t *testing.T, name, content string) string { } return path } + +// TestMergeBuiltinTools pins the three paths the #263 review called out: +// a --tools flag merges with the wizard's web_search selection instead of +// being clobbered, empties are dropped, and duplicates are deduped. +func TestMergeBuiltinTools(t *testing.T) { + cases := []struct { + name string + flag []string + fromWizard []string + want []string + }{ + {"flag+skip (wizard records nothing)", []string{"http_request"}, nil, []string{"http_request"}}, + {"flag+provider (must not clobber the flag)", []string{"http_request"}, []string{"web_search"}, []string{"http_request", "web_search"}}, + {"no-flag+provider", nil, []string{"web_search"}, []string{"web_search"}}, + {"dedupe overlap", []string{"web_search", "http_request"}, []string{"web_search"}, []string{"web_search", "http_request"}}, + {"drops empties", []string{"", "http_request"}, []string{""}, []string{"http_request"}}, + {"both empty", nil, nil, []string{}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := mergeBuiltinTools(tc.flag, tc.fromWizard) + if len(got) != len(tc.want) { + t.Fatalf("got %v, want %v", got, tc.want) + } + for i := range tc.want { + if got[i] != tc.want[i] { + t.Fatalf("got %v, want %v (order matters)", got, tc.want) + } + } + }) + } +} diff --git a/forge-cli/internal/tui/steps/tools_step.go b/forge-cli/internal/tui/steps/tools_step.go deleted file mode 100644 index 5ab4b784..00000000 --- a/forge-cli/internal/tui/steps/tools_step.go +++ /dev/null @@ -1,320 +0,0 @@ -package steps - -import ( - "fmt" - "os" - "strings" - - tea "github.com/charmbracelet/bubbletea" - - "github.com/initializ/forge/forge-cli/internal/tui" - "github.com/initializ/forge/forge-cli/internal/tui/components" -) - -// ToolInfo represents a builtin tool for the tools step. -type ToolInfo struct { - Name string - Description string -} - -// ValidateWebSearchKeyFunc validates a web search API key for a given provider. -type ValidateWebSearchKeyFunc func(provider, key string) error - -type toolsPhase int - -const ( - toolsSelectPhase toolsPhase = iota - toolsWebSearchProviderPhase - toolsWebSearchKeyPhase - toolsWebSearchValidatingPhase - toolsDonePhase -) - -// ToolsStep handles builtin tool selection. -type ToolsStep struct { - styles *tui.StyleSet - phase toolsPhase - multiSelect components.MultiSelect - providerSelect components.SingleSelect - keyInput components.SecretInput - complete bool - selected []string - webSearchKey string - webSearchKeyName string // "TAVILY_API_KEY" or "PERPLEXITY_API_KEY" - webSearchProvider string // "tavily" or "perplexity" - validateFn ValidateWebSearchKeyFunc - validating bool -} - -// NewToolsStep creates a new tools selection step. -func NewToolsStep(styles *tui.StyleSet, tools []ToolInfo, validateFn ValidateWebSearchKeyFunc) *ToolsStep { - var items []components.MultiSelectItem - for _, t := range tools { - icon := toolIcon(t.Name) - items = append(items, components.MultiSelectItem{ - Label: t.Name, - Value: t.Name, - Description: t.Description, - Icon: icon, - }) - } - - ms := components.NewMultiSelect( - items, - styles.Theme.Accent, - styles.Theme.AccentDim, - styles.Theme.Primary, - styles.Theme.Secondary, - styles.Theme.Dim, - styles.ActiveBorder, - styles.InactiveBorder, - styles.KbdKey, - styles.KbdDesc, - ) - - return &ToolsStep{ - styles: styles, - multiSelect: ms, - validateFn: validateFn, - } -} - -func (s *ToolsStep) Title() string { return "Built-in Tools" } -func (s *ToolsStep) Icon() string { return "๐Ÿ”ง" } - -func (s *ToolsStep) Init() tea.Cmd { - return s.multiSelect.Init() -} - -func (s *ToolsStep) Update(msg tea.Msg) (tui.Step, tea.Cmd) { - if s.complete { - return s, nil - } - - if wsm, ok := msg.(tea.WindowSizeMsg); ok { - switch s.phase { - case toolsSelectPhase: - updated, cmd := s.multiSelect.Update(wsm) - s.multiSelect = updated - return s, cmd - case toolsWebSearchProviderPhase: - updated, cmd := s.providerSelect.Update(wsm) - s.providerSelect = updated - return s, cmd - } - return s, nil - } - - switch s.phase { - case toolsSelectPhase: - updated, cmd := s.multiSelect.Update(msg) - s.multiSelect = updated - - if s.multiSelect.Done() { - s.selected = s.multiSelect.SelectedValues() - - // Check if web_search selected and no key is already set - if containsStr(s.selected, "web_search") && - os.Getenv("TAVILY_API_KEY") == "" && - os.Getenv("PERPLEXITY_API_KEY") == "" { - // Show provider selection - s.phase = toolsWebSearchProviderPhase - s.providerSelect = components.NewSingleSelect( - []components.SingleSelectItem{ - {Label: "Tavily (Recommended)", Value: "tavily", Description: "LLM-optimized search with structured results", Icon: "๐Ÿ”"}, - {Label: "Perplexity", Value: "perplexity", Description: "AI-powered search with citations", Icon: "๐ŸŒ"}, - }, - s.styles.Theme.Accent, - s.styles.Theme.Primary, - s.styles.Theme.Secondary, - s.styles.Theme.Dim, - s.styles.Theme.Border, - s.styles.Theme.Accent, - s.styles.Theme.AccentDim, - s.styles.KbdKey, - s.styles.KbdDesc, - ) - return s, s.providerSelect.Init() - } - - // If a key is already set in env, detect the provider - if containsStr(s.selected, "web_search") { - if os.Getenv("TAVILY_API_KEY") != "" { - s.webSearchProvider = "tavily" - } else if os.Getenv("PERPLEXITY_API_KEY") != "" { - s.webSearchProvider = "perplexity" - } - } - - s.complete = true - return s, func() tea.Msg { return tui.StepCompleteMsg{} } - } - - return s, cmd - - case toolsWebSearchProviderPhase: - updated, cmd := s.providerSelect.Update(msg) - s.providerSelect = updated - - if s.providerSelect.Done() { - _, s.webSearchProvider = s.providerSelect.Selected() - s.initKeyInput("") - return s, s.keyInput.Init() - } - - return s, cmd - - case toolsWebSearchKeyPhase: - updated, cmd := s.keyInput.Update(msg) - s.keyInput = updated - - if s.keyInput.Done() { - s.webSearchKey = s.keyInput.Value() - - // Run validation if we have a key and a validateFn - if s.webSearchKey != "" && s.validateFn != nil { - s.phase = toolsWebSearchValidatingPhase - s.validating = true - return s, s.runValidation() - } - - s.complete = true - return s, func() tea.Msg { return tui.StepCompleteMsg{} } - } - - return s, cmd - - case toolsWebSearchValidatingPhase: - if msg, ok := msg.(tui.ValidationResultMsg); ok { - s.validating = false - if msg.Err != nil { - // Validation failed โ€” go back to key input with error - s.initKeyInput(fmt.Sprintf("retry โ€” %s", msg.Err)) - s.keyInput.SetState(components.SecretInputFailed, msg.Err.Error()) - return s, s.keyInput.Init() - } - // Success - s.complete = true - return s, func() tea.Msg { return tui.StepCompleteMsg{} } - } - - return s, nil - } - - return s, nil -} - -// initKeyInput creates a fresh SecretInput for the web search API key. -func (s *ToolsStep) initKeyInput(suffix string) { - keyLabel := "Tavily API key for web_search" - s.webSearchKeyName = "TAVILY_API_KEY" - if s.webSearchProvider == "perplexity" { - keyLabel = "Perplexity API key for web_search" - s.webSearchKeyName = "PERPLEXITY_API_KEY" - } - if suffix != "" { - keyLabel = fmt.Sprintf("%s (%s)", keyLabel, suffix) - } - - s.phase = toolsWebSearchKeyPhase - s.keyInput = components.NewSecretInput( - keyLabel, - false, true, // required โ€” cannot skip; masked - s.styles.Theme.Accent, - s.styles.Theme.Success, - s.styles.Theme.Error, - s.styles.Theme.Border, - s.styles.AccentTxt, - s.styles.InactiveBorder, - s.styles.SuccessTxt, - s.styles.ErrorTxt, - s.styles.DimTxt, - s.styles.KbdKey, - s.styles.KbdDesc, - ) -} - -// runValidation runs the web search key validation asynchronously. -func (s *ToolsStep) runValidation() tea.Cmd { - provider := s.webSearchProvider - key := s.webSearchKey - validateFn := s.validateFn - return func() tea.Msg { - if validateFn == nil { - return tui.ValidationResultMsg{Err: nil} - } - err := validateFn(provider, key) - return tui.ValidationResultMsg{Err: err} - } -} - -func (s *ToolsStep) View(width int) string { - switch s.phase { - case toolsSelectPhase: - return s.multiSelect.View(width) - case toolsWebSearchProviderPhase: - return s.providerSelect.View(width) - case toolsWebSearchKeyPhase: - return s.keyInput.View(width) - case toolsWebSearchValidatingPhase: - if s.validating { - return " " + s.styles.AccentTxt.Render("โฃพ Validating...") + "\n" - } - return s.keyInput.View(width) - } - return "" -} - -func (s *ToolsStep) Complete() bool { - return s.complete -} - -func (s *ToolsStep) Summary() string { - if len(s.selected) == 0 { - return "none" - } - var parts []string - for _, name := range s.selected { - if name == "web_search" && s.webSearchProvider != "" { - parts = append(parts, fmt.Sprintf("web_search [%s]", s.webSearchProvider)) - } else { - parts = append(parts, name) - } - } - return strings.Join(parts, ", ") -} - -func (s *ToolsStep) Apply(ctx *tui.WizardContext) { - ctx.BuiltinTools = s.selected - if s.webSearchKey != "" && s.webSearchKeyName != "" { - ctx.EnvVars[s.webSearchKeyName] = s.webSearchKey - } - if s.webSearchProvider != "" { - ctx.EnvVars["WEB_SEARCH_PROVIDER"] = s.webSearchProvider - } -} - -func toolIcon(name string) string { - icons := map[string]string{ - "http_request": "๐ŸŒ", - "json_parse": "๐Ÿ“‹", - "csv_parse": "๐Ÿ“Š", - "datetime_now": "๐Ÿ•", - "uuid_generate": "๐Ÿ”‘", - "math_calculate": "๐Ÿ”ข", - "web_search": "๐Ÿ”", - } - if icon, ok := icons[name]; ok { - return icon - } - return "๐Ÿ”ง" -} - -func containsStr(slice []string, val string) bool { - for _, s := range slice { - if s == val { - return true - } - } - return false -} diff --git a/forge-cli/internal/tui/steps/web_search_step.go b/forge-cli/internal/tui/steps/web_search_step.go new file mode 100644 index 00000000..df48bf06 --- /dev/null +++ b/forge-cli/internal/tui/steps/web_search_step.go @@ -0,0 +1,259 @@ +package steps + +import ( + "fmt" + "os" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/initializ/forge/forge-cli/internal/tui" + "github.com/initializ/forge/forge-cli/internal/tui/components" +) + +// ValidateWebSearchKeyFunc validates a web search API key for a given provider. +type ValidateWebSearchKeyFunc func(provider, key string) error + +type webSearchPhase int + +const ( + webSearchChoosePhase webSearchPhase = iota + webSearchKeyPhase + webSearchValidatingPhase +) + +// WebSearchStep configures the built-in web_search tool: pick a provider +// (Tavily / Perplexity) or skip, and capture + validate its API key. Every +// builtin tool is auto-registered at runtime; web_search is the only one that +// needs operator input, so it gets a focused step of its own rather than being +// buried in a bulk tool checklist. +type WebSearchStep struct { + styles *tui.StyleSet + phase webSearchPhase + choose components.SingleSelect + keyInput components.SecretInput + complete bool + validating bool + + provider string // "tavily" or "perplexity"; "" when web search is disabled + keyName string // "TAVILY_API_KEY" or "PERPLEXITY_API_KEY" + key string + keyFromEnv bool + + validateFn ValidateWebSearchKeyFunc +} + +// NewWebSearchStep creates a new web search configuration step. +func NewWebSearchStep(styles *tui.StyleSet, validateFn ValidateWebSearchKeyFunc) *WebSearchStep { + // Annotate a provider whose key is already in the environment so the user + // knows selecting it will silently adopt that key (and picking the other + // provider forgoes an already-available key). See #263 review. + tavilyLabel := "Tavily (Recommended)" + if os.Getenv("TAVILY_API_KEY") != "" { + tavilyLabel += " ยท key detected in env" + } + perplexityLabel := "Perplexity" + if os.Getenv("PERPLEXITY_API_KEY") != "" { + perplexityLabel += " ยท key detected in env" + } + + choose := components.NewSingleSelect( + []components.SingleSelectItem{ + {Label: tavilyLabel, Value: "tavily", Description: "LLM-optimized search with structured results", Icon: "๐Ÿ”"}, + {Label: perplexityLabel, Value: "perplexity", Description: "AI-powered search with citations", Icon: "๐ŸŒ"}, + {Label: "No web search", Value: "", Description: "Skip โ€” agents have no live web access", Icon: "๐Ÿšซ"}, + }, + styles.Theme.Accent, + styles.Theme.Primary, + styles.Theme.Secondary, + styles.Theme.Dim, + styles.Theme.Border, + styles.Theme.Accent, + styles.Theme.AccentDim, + styles.KbdKey, + styles.KbdDesc, + ) + + return &WebSearchStep{ + styles: styles, + choose: choose, + validateFn: validateFn, + } +} + +func (s *WebSearchStep) Title() string { return "Web Search" } +func (s *WebSearchStep) Icon() string { return "๐Ÿ”" } + +func (s *WebSearchStep) Init() tea.Cmd { + return s.choose.Init() +} + +func (s *WebSearchStep) Update(msg tea.Msg) (tui.Step, tea.Cmd) { + if s.complete { + return s, nil + } + + if wsm, ok := msg.(tea.WindowSizeMsg); ok { + switch s.phase { + case webSearchChoosePhase: + updated, cmd := s.choose.Update(wsm) + s.choose = updated + return s, cmd + case webSearchKeyPhase: + updated, cmd := s.keyInput.Update(wsm) + s.keyInput = updated + return s, cmd + } + return s, nil + } + + switch s.phase { + case webSearchChoosePhase: + updated, cmd := s.choose.Update(msg) + s.choose = updated + + if s.choose.Done() { + _, s.provider = s.choose.Selected() + + // "No web search" โ€” nothing more to configure. + if s.provider == "" { + return s.finish() + } + + s.keyName = "TAVILY_API_KEY" + if s.provider == "perplexity" { + s.keyName = "PERPLEXITY_API_KEY" + } + + // If the provider key is already in the environment, adopt it + // without prompting. + if os.Getenv(s.keyName) != "" { + s.keyFromEnv = true + return s.finish() + } + + s.initKeyInput("") + return s, s.keyInput.Init() + } + + return s, cmd + + case webSearchKeyPhase: + updated, cmd := s.keyInput.Update(msg) + s.keyInput = updated + + if s.keyInput.Done() { + s.key = s.keyInput.Value() + + if s.key != "" && s.validateFn != nil { + s.phase = webSearchValidatingPhase + s.validating = true + return s, s.runValidation() + } + + return s.finish() + } + + return s, cmd + + case webSearchValidatingPhase: + if msg, ok := msg.(tui.ValidationResultMsg); ok { + s.validating = false + if msg.Err != nil { + // Validation failed โ€” return to key input with the error. + s.initKeyInput(fmt.Sprintf("retry โ€” %s", msg.Err)) + s.keyInput.SetState(components.SecretInputFailed, msg.Err.Error()) + return s, s.keyInput.Init() + } + return s.finish() + } + return s, nil + } + + return s, nil +} + +func (s *WebSearchStep) finish() (tui.Step, tea.Cmd) { + s.complete = true + return s, func() tea.Msg { return tui.StepCompleteMsg{} } +} + +// initKeyInput creates a fresh SecretInput for the provider API key. +func (s *WebSearchStep) initKeyInput(suffix string) { + keyLabel := "Tavily API key for web_search" + if s.provider == "perplexity" { + keyLabel = "Perplexity API key for web_search" + } + if suffix != "" { + keyLabel = fmt.Sprintf("%s (%s)", keyLabel, suffix) + } + + s.phase = webSearchKeyPhase + s.keyInput = components.NewSecretInput( + keyLabel, + false, true, // required โ€” cannot skip; masked + s.styles.Theme.Accent, + s.styles.Theme.Success, + s.styles.Theme.Error, + s.styles.Theme.Border, + s.styles.AccentTxt, + s.styles.InactiveBorder, + s.styles.SuccessTxt, + s.styles.ErrorTxt, + s.styles.DimTxt, + s.styles.KbdKey, + s.styles.KbdDesc, + ) +} + +// runValidation runs the web search key validation asynchronously. +func (s *WebSearchStep) runValidation() tea.Cmd { + provider := s.provider + key := s.key + validateFn := s.validateFn + return func() tea.Msg { + if validateFn == nil { + return tui.ValidationResultMsg{Err: nil} + } + return tui.ValidationResultMsg{Err: validateFn(provider, key)} + } +} + +func (s *WebSearchStep) View(width int) string { + switch s.phase { + case webSearchChoosePhase: + return s.choose.View(width) + case webSearchKeyPhase: + return s.keyInput.View(width) + case webSearchValidatingPhase: + if s.validating { + return " " + s.styles.AccentTxt.Render("โฃพ Validating...") + "\n" + } + return s.keyInput.View(width) + } + return "" +} + +func (s *WebSearchStep) Complete() bool { return s.complete } + +func (s *WebSearchStep) Summary() string { + if s.provider == "" { + return "disabled" + } + if s.keyFromEnv { + return fmt.Sprintf("%s [key from env]", s.provider) + } + return s.provider +} + +func (s *WebSearchStep) Apply(ctx *tui.WizardContext) { + if s.provider == "" { + return + } + // web_search is auto-registered; recording it in BuiltinTools keeps the + // review/summary and any policy interaction consistent with the choice. + ctx.BuiltinTools = append(ctx.BuiltinTools, "web_search") + ctx.EnvVars["WEB_SEARCH_PROVIDER"] = s.provider + if s.key != "" && s.keyName != "" { + ctx.EnvVars[s.keyName] = s.key + } +} diff --git a/forge-cli/internal/tui/steps/web_search_step_test.go b/forge-cli/internal/tui/steps/web_search_step_test.go new file mode 100644 index 00000000..f3e7d2f9 --- /dev/null +++ b/forge-cli/internal/tui/steps/web_search_step_test.go @@ -0,0 +1,55 @@ +package steps + +import ( + "slices" + "testing" + + "github.com/initializ/forge/forge-cli/internal/tui" +) + +// TestWebSearchStep_Apply pins the three meaningful Apply behaviors the #263 +// review flagged: skip writes nothing; a chosen provider writes the provider +// env var + BuiltinTools + key; the env-key path writes the provider but no +// key (it was already in the environment). +func TestWebSearchStep_Apply(t *testing.T) { + t.Run("skip writes nothing", func(t *testing.T) { + ctx := tui.NewWizardContext() + s := &WebSearchStep{provider: ""} // "No web search" + s.Apply(ctx) + if len(ctx.BuiltinTools) != 0 { + t.Errorf("skip must not record a builtin tool, got %v", ctx.BuiltinTools) + } + if len(ctx.EnvVars) != 0 { + t.Errorf("skip must not write env vars, got %v", ctx.EnvVars) + } + }) + + t.Run("chosen provider writes provider + tool + key", func(t *testing.T) { + ctx := tui.NewWizardContext() + s := &WebSearchStep{provider: "tavily", keyName: "TAVILY_API_KEY", key: "tvly-secret"} + s.Apply(ctx) + if !slices.Contains(ctx.BuiltinTools, "web_search") { + t.Errorf("expected web_search recorded, got %v", ctx.BuiltinTools) + } + if ctx.EnvVars["WEB_SEARCH_PROVIDER"] != "tavily" { + t.Errorf("WEB_SEARCH_PROVIDER = %q", ctx.EnvVars["WEB_SEARCH_PROVIDER"]) + } + if ctx.EnvVars["TAVILY_API_KEY"] != "tvly-secret" { + t.Errorf("expected the key written, got %q", ctx.EnvVars["TAVILY_API_KEY"]) + } + }) + + t.Run("env-key path writes provider but no key", func(t *testing.T) { + ctx := tui.NewWizardContext() + // keyFromEnv: the key is already in the environment, so Apply records + // the provider + tool but must NOT re-write the key value (it's ""). + s := &WebSearchStep{provider: "perplexity", keyName: "PERPLEXITY_API_KEY", key: "", keyFromEnv: true} + s.Apply(ctx) + if ctx.EnvVars["WEB_SEARCH_PROVIDER"] != "perplexity" { + t.Errorf("WEB_SEARCH_PROVIDER = %q", ctx.EnvVars["WEB_SEARCH_PROVIDER"]) + } + if _, wrote := ctx.EnvVars["PERPLEXITY_API_KEY"]; wrote { + t.Errorf("env-key path must not write the key, got %v", ctx.EnvVars) + } + }) +}