feat(tui): drop builtin-tool selection screen; give web search its own step#263
Conversation
…n step Every builtin tool is auto-registered at runtime (builtins.RegisterAll), so the init wizard's bulk 'select which built-ins' screen served no purpose — builtin_tools only ever affected a narrow platform-policy-deny interaction, never registration. Remove ToolsStep. The one builtin that genuinely needs operator input — web_search (provider + API key) — gets its own focused WebSearchStep (Tavily / Perplexity / skip; validates the key; auto-skips the prompt when the provider key is already in the environment). The --tools flag still populates builtin_tools for non-interactive use; the now-always-empty wizard context no longer clobbers it.
…o/NewToolsStep) The tools-drop commit only landed the tools_step.go deletion; the replacement WebSearchStep and the init.go rewire that references it were never committed, so the package failed to compile (undefined steps.ToolInfo / steps.NewToolsStep). This adds the missing pieces: - new WebSearchStep: Tavily / Perplexity / skip, validates the key, and auto-adopts a provider key already present in the environment. - init.go: drop the toolInfos build, slot NewWebSearchStep in ChannelStep's place, and guard opts.BuiltinTools so an empty wizard context can't clobber a --tools flag.
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: drop builtin-tool selection screen; give web search its own step
Good cleanup with sound justification. I verified the load-bearing claims against main rather than taking them on faith, and they hold:
builtin_toolsreally is not a registration allowlist. Thetool_filter_stage.go/tool_filter.gohits that look alarming on a grep are just category annotation + dev-tool filtering keyed on tool names in the spec — nothing treatsbuiltin_toolsas "only these," and runtime usesbuiltins.RegisterAllunconditionally.- The egress interaction is correctly preserved.
init_egress.goderives allowlist domains fromopts.BuiltinTools(web_search→ provider-filteredapi.tavily.com/api.perplexity.ai; others →DefaultToolDomains). Among the builtins the old multi-select actually offered, onlyweb_searchcarries egress domains (http_requestis dynamic/empty; the domain-bearing names inDefaultToolDomainsare adapter tools that were never inbuiltins.All()). The new step'sApplyrecordingweb_search+WEB_SEARCH_PROVIDERkeeps that path working — the comment inApplyexplaining why is accurate and appreciated.
One real fix needed before merge (inline on init.go): the --tools clobber guard only protects the skip path. Remaining notes are minors/nits, inline and below.
Notes without an inline anchor
- Dead defensive branch:
runValidation'svalidateFn == nilcheck is unreachable (guarded at the call site). Harmless. - Semantics drift worth a docs glance:
builtin_toolsnow means "wizard-recorded selection for egress/policy purposes" — empty for skip-path agents,[web_search]otherwise — whiledocs/security/platform-policy.mdand theforge.yaml.tmplcomment date from the multi-select era. Adjacent to open issue #281 (thetools:-doesn't-register confusion), which this PR's direction actually helps clarify.
Verdict
The removal is safe (verified against the filter/egress/runtime consumers), the new step is a faithful, tighter rewrite of the old web-search sub-flow, and CI is green. Fix the merge-instead-of-overwrite in init.go (ideally with the small table test suggested inline) and this is good to go.
| // Only overwrite from the wizard context when it actually collected | ||
| // something (e.g. web_search). An empty context must not clobber a | ||
| // --tools flag set on the command line. | ||
| if len(ctx.BuiltinTools) > 0 { |
There was a problem hiding this comment.
Important: this guard only protects the skip path — choosing a provider still clobbers --tools.
When the user picks a web-search provider interactively, ctx.BuiltinTools is ["web_search"] and this REPLACES a flag-provided value. forge init --tools web_search,http_request + choosing Tavily in the wizard → builtin_tools: [web_search]: the flag's other entries are dropped from the yaml and from init_egress's DefaultToolDomains derivation. The PR body's "no longer clobbers a flag-provided value" holds only when the user selects "No web search."
Fix: merge and dedupe instead of overwrite, e.g. opts.BuiltinTools = dedupe(append(opts.BuiltinTools, ctx.BuiltinTools...)). A small table test (flag+skip / flag+provider / no-flag+provider) would pin all three paths — this is plain non-TUI code, so it's cheap to cover.
| return s.provider | ||
| } | ||
|
|
||
| func (s *WebSearchStep) Apply(ctx *tui.WizardContext) { |
There was a problem hiding this comment.
Minor: Apply is a pure function with three meaningful behaviors (skip writes nothing; provider writes WEB_SEARCH_PROVIDER + key; env-key path writes provider but no key) — worth a 10-line test even though the steps package is otherwise test-light. It's also the piece a future refactor is most likely to silently break.
| func NewWebSearchStep(styles *tui.StyleSet, validateFn ValidateWebSearchKeyFunc) *WebSearchStep { | ||
| choose := components.NewSingleSelect( | ||
| []components.SingleSelectItem{ | ||
| {Label: "Tavily (Recommended)", Value: "tavily", Description: "LLM-optimized search with structured results", Icon: "🔍"}, |
There was a problem hiding this comment.
Nit (UX): the old flow auto-detected an already-set TAVILY_API_KEY/PERPLEXITY_API_KEY before any prompt; the new flow makes the user pick a provider first and only then auto-skips the key. Explicit choice is a defensible improvement — but consider annotating the option that already has a key in env (e.g. "Tavily (key detected in env)") so users don't pick Perplexity not knowing their Tavily key would have been adopted silently.
| } | ||
|
|
||
| // initKeyInput creates a fresh SecretInput for the provider API key. | ||
| func (s *WebSearchStep) initKeyInput(suffix string) { |
There was a problem hiding this comment.
Nit (pre-existing, carried over from the old step): this fresh SecretInput never receives a WindowSizeMsg until the next terminal resize — the validating phase swallows them — so after a validation failure the retry input renders at zero/stale width. Not a regression, just noting it survives the rewrite.
…sts (#263 review) Blocking fix from the #263 review: the clobber guard only protected the SKIP path. When the user picked a web-search provider interactively, ctx.BuiltinTools was [web_search] and REPLACED a --tools flag value — dropping the flag's other entries from forge.yaml and the init_egress DefaultToolDomains derivation. - Extract mergeBuiltinTools(flag, fromWizard): union, first-seen order, dedupe, drop empties — used in collectInteractive instead of the overwrite. Table test pins all three paths (flag+skip, flag+provider, no-flag+provider) plus dedupe/empty edges. - web_search_step: annotate a provider whose key is already in the environment ("· key detected in env") so the user knows selecting it adopts that key and picking the other provider forgoes it (review UX nit). - Add WebSearchStep.Apply test covering the three behaviors: skip writes nothing; chosen provider writes WEB_SEARCH_PROVIDER + web_search + key; env-key path writes the provider but not the key.
|
Thanks — all addressed in the latest commit. Blocking:
UX nit — env-key annotation. Implemented: a provider whose key is already set gets Minors left as-is (per your "harmless"/"not a regression" notes):
Docs semantics drift ( Tests + lint green. |
1 participant
Summary
The
forge initwizard had a bulk "Built-in Tools" selection screen (checkbox list of every builtin). But every builtin is auto-registered at runtime (builtins.RegisterAll) regardless ofbuiltin_tools— that field only ever influenced a narrow platform-policy-deny interaction, never registration. So the selection screen was busywork.This removes it, and gives the one builtin that genuinely needs operator input — web search — its own focused step.
Changes
ToolsStep(the builtin multi-select).builtins.RegisterAllalready wires every builtin, so interactive init now leavesbuiltin_toolsempty → all builtins on.WebSearchStep— pick Tavily / Perplexity / skip, then enter + validate the API key. Auto-skips the key prompt when the provider's key is already in the environment. WritesWEB_SEARCH_PROVIDER+TAVILY_API_KEY/PERPLEXITY_API_KEYto the agent.env(same behavior the tools step used to bury).--toolsflag preserved — the non-interactive path still populatesbuiltin_tools; the now-always-empty wizard context no longer clobbers a flag-provided value (init.go).Notes
web_searchwithout a provider/key stays inert (as before) — operators can add the key later via.env.Build +
golangci-lintclean;forge-cli/internal/tui/...tests green.Follow-up (from the same feedback, separate PR): wizard back-navigation consistency and the multi-select Enter auto-select behavior.