From 81539317c25480bfe570400f12745b41ac6ba5ea Mon Sep 17 00:00:00 2001 From: xp880906 <4232190+xp880906@users.noreply.github.com> Date: Thu, 6 Aug 2026 16:17:36 +0800 Subject: [PATCH 1/3] feat(provider): add editable Base URL step to official provider wizard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The official-provider tab in `ocr config provider` only captured API key and model, with no way to override a preset provider's Base URL. The resolver already honored `entry.URL` over `preset.BaseURL`, but the TUI never exposed it — litellm (a self-hosted gateway rarely at http://localhost:4000/v1) was the canonical pain point. Add a Base URL step to the official-tab flow (stepModel -> stepBaseURL -> stepAPIKey), pre-filled with the effective URL (configured override or preset default). Persist `providers..url` only when the entered value differs from the preset default, so the preset remains the fallback and configs without an explicit url are unchanged. Custom/manual tabs are unaffected. Add resolver regression tests (litellm override + default fallback) and TUI tests (pre-fill with preset/override, Esc navigation, persistence of override vs. clearing on preset default). Update the four official-tab tests that assumed stepModel -> stepAPIKey to traverse the new step. --- cmd/opencodereview/provider_cmd.go | 9 ++ cmd/opencodereview/provider_cmd_test.go | 60 ++++++++++++ cmd/opencodereview/provider_tui.go | 122 ++++++++++++++++++++++++ cmd/opencodereview/provider_tui_test.go | 110 ++++++++++++++++++--- internal/llm/resolver_test.go | 58 +++++++++++ 5 files changed, 347 insertions(+), 12 deletions(-) diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index f67da930f..3ab951359 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" tea "charm.land/bubbletea/v2" @@ -260,6 +261,14 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider // Confirmed empty key: clear saved api_key so resolver falls back to $ENV_VAR. entry.APIKey = "" } + // Persist a Base URL override only when it differs from the preset default. + // An empty/unchanged value clears any prior override so the preset BaseURL + // remains the default, matching the "preset is the fallback" contract. + if isPreset && strings.TrimSpace(result.url) != "" && result.url != preset.BaseURL { + entry.URL = result.url + } else { + entry.URL = "" + } cfg.Providers[result.provider] = entry if cfg.Provider != result.provider { diff --git a/cmd/opencodereview/provider_cmd_test.go b/cmd/opencodereview/provider_cmd_test.go index d1f03bba8..86cca1c38 100644 --- a/cmd/opencodereview/provider_cmd_test.go +++ b/cmd/opencodereview/provider_cmd_test.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "testing" + + "github.com/alibaba/open-code-review/internal/llm" ) func TestMaskKey(t *testing.T) { @@ -376,3 +378,61 @@ func TestPrintWizardCancelled(t *testing.T) { }) } } + +// TestApplyOfficialProviderConfig_PersistsURLOverride verifies that a custom +// Base URL entered in the wizard is persisted to providers..url, while a +// value equal to the preset default is cleared so the preset remains the default. +func TestApplyOfficialProviderConfig_PersistsURLOverride(t *testing.T) { + t.Setenv("LITELLM_API_KEY", "sk-litellm") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + cfg := &Config{} + + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "litellm", + model: "openai/gpt-5.4", + apiKey: "sk-litellm", + url: "https://gateway.internal:8000/v1", + }) + if err != nil { + t.Fatalf("applyOfficialProviderConfig: %v", err) + } + if got := cfg.Providers["litellm"].URL; got != "https://gateway.internal:8000/v1" { + t.Errorf("persisted URL = %q, want https://gateway.internal:8000/v1", got) + } + diskCfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("load config: %v", err) + } + if got := diskCfg.Providers["litellm"].URL; got != "https://gateway.internal:8000/v1" { + t.Errorf("disk URL = %q, want https://gateway.internal:8000/v1", got) + } +} + +// TestApplyOfficialProviderConfig_ClearsURLWhenPresetDefault verifies that +// submitting the preset default Base URL writes no url field, so the preset +// BaseURL remains the resolver default. +func TestApplyOfficialProviderConfig_ClearsURLWhenPresetDefault(t *testing.T) { + t.Setenv("LITELLM_API_KEY", "sk-litellm") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + cfg := &Config{ + Providers: map[string]ProviderEntry{ + "litellm": {URL: "https://old-gateway.internal:9000/v1"}, + }, + } + + preset, _ := llm.LookupProvider("litellm") + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "litellm", + model: "openai/gpt-5.4", + apiKey: "sk-litellm", + url: preset.BaseURL, + }) + if err != nil { + t.Fatalf("applyOfficialProviderConfig: %v", err) + } + if got := cfg.Providers["litellm"].URL; got != "" { + t.Errorf("persisted URL = %q, want empty (preset default should not persist a url)", got) + } +} diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 31fa9e55b..daab18875 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -18,6 +18,7 @@ type tuiStep int const ( stepProvider tuiStep = iota stepModel + stepBaseURL // official-tab only: edit the provider Base URL (defaults to preset) stepAPIKey ) @@ -135,6 +136,9 @@ type providerTUIModel struct { cpURLInput textinput.Model cpAuthInput textinput.Model + // --- tab: official (Base URL override) --- + officialURLInput textinput.Model + // --- tab: manual --- inManualForm bool manualStep manualStep @@ -260,6 +264,10 @@ func newProviderTUI(cfg *Config, configPath string) providerTUIModel { cpAuth.Placeholder = "optional, leave empty for default (Authorization)" cpAuth.SetWidth(55) + officialURL := textinput.New() + officialURL.Placeholder = "leave empty for provider default" + officialURL.SetWidth(50) + manualURL := textinput.New() manualURL.Placeholder = "enter your API base URL" manualURL.SetWidth(50) @@ -286,6 +294,7 @@ func newProviderTUI(cfg *Config, configPath string) providerTUIModel { cpNameInput: cpName, cpURLInput: cpURL, cpAuthInput: cpAuth, + officialURLInput: officialURL, manualURLInput: manualURL, manualModelInput: manualModel, manualAuthHeaderInput: manualAuthHeader, @@ -340,6 +349,16 @@ func newProviderTUI(cfg *Config, configPath string) providerTUIModel { m.apiKeyOriginal = entry.APIKey m.apiKeyMasked = true } + + // Pre-fill the official Base URL input with the effective URL: a + // configured override (entry.URL) if present, else the preset default. + // Editing this value later overrides preset.BaseURL in the resolver. + selected := providers[m.officialIdx] + effectiveURL := selected.BaseURL + if entry, ok := cfg.Providers[cfg.Provider]; ok && entry.URL != "" { + effectiveURL = entry.URL + } + m.officialURLInput.SetValue(effectiveURL) } if cfg.Provider == "" && cfg.Llm.URL != "" { @@ -617,6 +636,10 @@ func (m providerTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.updateAPIKeyInput(key, msg) } + if m.step == stepBaseURL { + return m.updateOfficialURLInput(key, msg) + } + if m.step == stepProvider && (m.creatingCustom || m.editingCustom) { return m.updateCustomProviderForm(key, msg) } @@ -705,6 +728,11 @@ func (m providerTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } default: + if m.step == stepBaseURL { + var cmd tea.Cmd + m.officialURLInput, cmd = m.officialURLInput.Update(msg) + return m, cmd + } if m.step == stepProvider && (m.creatingCustom || m.editingCustom) { return m.passThroughCPInput(msg) } @@ -928,10 +956,43 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { return false, "API key is required" } +// updateOfficialURLInput handles the official-tab Base URL step. Enter advances +// to the API key step (regardless of whether the field was edited); Esc/Back +// returns to the model step. +func (m providerTUIModel) updateOfficialURLInput(key string, msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { + switch key { + case "esc": + m.officialURLInput.Blur() + m.step = stepModel + m.formError = "" + return m, nil + case "enter": + m.officialURLInput.Blur() + m.step = stepAPIKey + m.formError = "" + m.loadExistingAPIKey() + return m, m.apiKeyInput.Focus() + case "ctrl+c": + m.cancelled = true + return m, tea.Quit + default: + var cmd tea.Cmd + m.officialURLInput, cmd = m.officialURLInput.Update(msg) + m.formError = "" + return m, cmd + } +} + func (m providerTUIModel) updateAPIKeyInput(key string, msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { switch key { case "esc": m.apiKeyInput.Blur() + // Official providers have an intermediate Base URL step; custom and + // manual providers go straight back to model selection. + if m.activeTab == tabOfficial { + m.step = stepBaseURL + return m, m.officialURLInput.Focus() + } m.step = stepModel m.formError = "" return m, nil @@ -1731,6 +1792,20 @@ func (m providerTUIModel) handleEnter() (tea.Model, tea.Cmd) { m.formError = err.Error() return m, nil } + // Official providers offer an editable Base URL step (defaults to the + // preset); custom providers already capture a URL at creation time, so + // they advance straight to the API key. + if m.activeTab == tabOfficial { + m.step = stepBaseURL + m.formError = "" + m.loadOfficialURL() + return m, m.officialURLInput.Focus() + } + m.step = stepAPIKey + m.formError = "" + m.loadExistingAPIKey() + return m, m.apiKeyInput.Focus() + case stepBaseURL: m.step = stepAPIKey m.formError = "" m.loadExistingAPIKey() @@ -1816,6 +1891,21 @@ func (m *providerTUIModel) loadExistingAPIKey() { } } +// loadOfficialURL pre-fills the official Base URL input with the effective URL +// for the currently selected provider: a configured override (entry.URL) when +// set, otherwise the preset default (preset.BaseURL). Leaving the field at the +// preset default persists no url field, so the preset remains the default. +func (m *providerTUIModel) loadOfficialURL() { + p := m.currentProvider() + effectiveURL := p.BaseURL + if m.existingCfg != nil { + if entry, ok := m.existingCfg.Providers[p.Name]; ok && entry.URL != "" { + effectiveURL = entry.URL + } + } + m.officialURLInput.SetValue(effectiveURL) +} + func (m providerTUIModel) selectedModelFromState() string { if m.modelInput.Value() != "" && (m.customModel || m.isCustomModelItem(m.modelIdx)) { return m.modelInput.Value() @@ -1847,6 +1937,7 @@ func (m providerTUIModel) result() providerTUIResult { provider: p.Name, model: model, apiKey: apiKey, + url: strings.TrimSpace(m.officialURLInput.Value()), sessionModelPick: m.sessionModelPickSnapshot(), } @@ -1968,6 +2059,8 @@ func (m providerTUIModel) View() tea.View { m.viewProvider(&s) case stepModel: m.viewModel(&s) + case stepBaseURL: + m.viewBaseURL(&s) case stepAPIKey: m.viewAPIKey(&s) } @@ -2291,6 +2384,35 @@ func (m providerTUIModel) viewModel(s *strings.Builder) { s.WriteString("\n") } +// viewBaseURL renders the official-tab Base URL step. The field is pre-filled +// with the effective URL (override or preset default); editing it overrides +// preset.BaseURL when persisted, leaving it unchanged keeps the preset default. +func (m providerTUIModel) viewBaseURL(s *strings.Builder) { + provider := m.currentProvider() + title := fmt.Sprintf(" Edit Base URL (%s)", provider.DisplayName) + s.WriteString(tuiTitleStyle.Render(title)) + s.WriteString("\n\n") + s.WriteString(" " + m.officialURLInput.View()) + s.WriteString("\n") + + preset, isPreset := llm.LookupProvider(provider.Name) + if isPreset && preset.BaseURL != "" { + s.WriteString("\n") + s.WriteString(tuiDimStyle.Render(fmt.Sprintf(" Default: %s (leave unchanged to keep the preset default)", preset.BaseURL))) + s.WriteString("\n") + } + + if m.formError != "" { + s.WriteString("\n") + s.WriteString(tuiErrorStyle.Render(" " + m.formError)) + s.WriteString("\n") + } + + s.WriteString("\n") + s.WriteString(tuiHelpStyle.Render(" Enter Confirm Esc Back")) + s.WriteString("\n") +} + func (m providerTUIModel) viewAPIKey(s *strings.Builder) { var title string if m.activeTab == tabCustom && m.customIdx < len(m.customProviders) { diff --git a/cmd/opencodereview/provider_tui_test.go b/cmd/opencodereview/provider_tui_test.go index e15dd4d36..4b300be3d 100644 --- a/cmd/opencodereview/provider_tui_test.go +++ b/cmd/opencodereview/provider_tui_test.go @@ -176,7 +176,7 @@ func TestProviderTUI_EscFromModelGoesBackToProvider(t *testing.T) { } } -func TestProviderTUI_EscFromAPIKeyGoesBackToModel(t *testing.T) { +func TestProviderTUI_EscFromAPIKeyGoesBackToBaseURL(t *testing.T) { m := newProviderTUI(&Config{}, "") result, _ := m.Update(enterKey()) @@ -184,14 +184,20 @@ func TestProviderTUI_EscFromAPIKeyGoesBackToModel(t *testing.T) { result, _ = m2.Update(enterKey()) m3 := result.(providerTUIModel) - if m3.step != stepAPIKey { - t.Fatalf("after 2x Enter, step = %d, want %d (stepAPIKey)", m3.step, stepAPIKey) + if m3.step != stepBaseURL { + t.Fatalf("after 2x Enter, step = %d, want %d (stepBaseURL)", m3.step, stepBaseURL) } - result, _ = m3.Update(escKey()) + result, _ = m3.Update(enterKey()) m4 := result.(providerTUIModel) - if m4.step != stepModel { - t.Errorf("after Esc on stepAPIKey, step = %d, want %d (stepModel)", m4.step, stepModel) + if m4.step != stepAPIKey { + t.Fatalf("after 3x Enter, step = %d, want %d (stepAPIKey)", m4.step, stepAPIKey) + } + + result, _ = m4.Update(escKey()) + m5 := result.(providerTUIModel) + if m5.step != stepBaseURL { + t.Errorf("after Esc on stepAPIKey, step = %d, want %d (stepBaseURL)", m5.step, stepBaseURL) } } @@ -2489,8 +2495,8 @@ func TestProviderTUI_CancelIncompleteOfficialProviderSwitch_NoPersistedChanges(t if m2.savedInSession { t.Error("savedInSession should be false for cross-provider navigation") } - if m2.step != stepAPIKey { - t.Fatalf("step = %d, want stepAPIKey", m2.step) + if m2.step != stepBaseURL { + t.Fatalf("step = %d, want stepBaseURL", m2.step) } result, _ = m2.Update(escKey()) @@ -2542,8 +2548,8 @@ func TestProviderTUI_SameOfficialProviderModelChange_DefersPersistUntilConfirm(t if m2.savedInSession { t.Error("savedInSession should be false before API key confirm") } - if m2.step != stepAPIKey { - t.Fatalf("step = %d, want stepAPIKey", m2.step) + if m2.step != stepBaseURL { + t.Fatalf("step = %d, want stepBaseURL", m2.step) } if _, err := os.Stat(configPath); err == nil { t.Fatal("config should not be written before wizard confirm") @@ -2580,9 +2586,20 @@ func TestProviderTUI_OfficialModelChangeBlockedAtAPIKey_KeepsGlobalModel(t *test result, _ := m.Update(enterKey()) m2 := result.(providerTUIModel) - m2.beginAPIKeyReplace() + if m2.step != stepBaseURL { + t.Fatalf("step = %d, want stepBaseURL", m2.step) + } - result, cmd := m2.Update(enterKey()) + // Advance from the Base URL step to the API key step, then attempt to + // confirm without a key (should be blocked). + result, _ = m2.Update(enterKey()) + m2b := result.(providerTUIModel) + if m2b.step != stepAPIKey { + t.Fatalf("step = %d, want stepAPIKey", m2b.step) + } + m2b.beginAPIKeyReplace() + + result, cmd := m2b.Update(enterKey()) m3 := result.(providerTUIModel) if cmd != nil { t.Error("Enter without key or env should not quit") @@ -2976,3 +2993,72 @@ func TestProviderTUIResult_ManualProtocolIsCanonical(t *testing.T) { } } } + +// TestProviderTUI_OfficialBaseURLPrefilledWithPreset verifies that entering the +// official Base URL step pre-fills the input with the preset default when no +// override is configured. +func TestProviderTUI_OfficialBaseURLPrefilledWithPreset(t *testing.T) { + cfg := &Config{ + Provider: "litellm", + Providers: map[string]ProviderEntry{ + "litellm": {APIKey: "sk-test", Model: "openai/gpt-5.4"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "litellm" { + m.officialIdx = i + break + } + } + m.step = stepModel + m.modelIdx = modelIdxForName(t, m, "openai/gpt-5.4") + + result, _ := m.Update(enterKey()) + m2 := result.(providerTUIModel) + if m2.step != stepBaseURL { + t.Fatalf("step = %d, want stepBaseURL", m2.step) + } + preset, _ := llm.LookupProvider("litellm") + if got := m2.officialURLInput.Value(); got != preset.BaseURL { + t.Errorf("officialURLInput = %q, want preset default %q", got, preset.BaseURL) + } +} + +// TestProviderTUI_OfficialBaseURLPrefilledWithOverride verifies that a +// configured providers..url is shown in the Base URL step. +func TestProviderTUI_OfficialBaseURLPrefilledWithOverride(t *testing.T) { + cfg := &Config{ + Provider: "litellm", + Providers: map[string]ProviderEntry{ + "litellm": {APIKey: "sk-test", Model: "openai/gpt-5.4", URL: "https://gateway.internal:8000/v1"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "litellm" { + m.officialIdx = i + break + } + } + m.step = stepModel + m.modelIdx = modelIdxForName(t, m, "openai/gpt-5.4") + + result, _ := m.Update(enterKey()) + m2 := result.(providerTUIModel) + if m2.step != stepBaseURL { + t.Fatalf("step = %d, want stepBaseURL", m2.step) + } + if got := m2.officialURLInput.Value(); got != "https://gateway.internal:8000/v1" { + t.Errorf("officialURLInput = %q, want configured override", got) + } + + // Esc from the Base URL step returns to model selection. + result, _ = m2.Update(escKey()) + m3 := result.(providerTUIModel) + if m3.step != stepModel { + t.Errorf("after Esc on stepBaseURL, step = %d, want stepModel", m3.step) + } +} diff --git a/internal/llm/resolver_test.go b/internal/llm/resolver_test.go index a81e0f1dc..48cc7b783 100644 --- a/internal/llm/resolver_test.go +++ b/internal/llm/resolver_test.go @@ -2144,3 +2144,61 @@ func TestEnsureMessagesSuffix(t *testing.T) { }) } } + +// TestResolveEndpoint_PresetProviderURLOverride verifies that a configured +// providers..url overrides the preset BaseURL for a built-in provider, +// while the same provider without a url field falls back to preset.BaseURL. +// litellm is the canonical case: a self-hosted gateway whose URL is rarely the +// preset default (http://localhost:4000/v1). +func TestResolveEndpoint_PresetProviderURLOverride(t *testing.T) { + clearAllEnv(t) + + cfg := configFile{ + Provider: "litellm", + Providers: map[string]providerEntryConfig{ + "litellm": {APIKey: "sk-litellm-test", Model: "openai/gpt-5.4", URL: "https://gateway.internal:8000/v1"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.URL != "https://gateway.internal:8000/v1" { + t.Errorf("URL = %q, want %q (configured url should override preset default)", ep.URL, "https://gateway.internal:8000/v1") + } + if ep.Protocol != ProtocolOpenAIChatCompletions { + t.Errorf("Protocol = %q, want %q", ep.Protocol, ProtocolOpenAIChatCompletions) + } +} + +// TestResolveEndpoint_PresetProviderURLDefaultsToPreset verifies that a +// built-in provider without a configured url resolves to preset.BaseURL. +func TestResolveEndpoint_PresetProviderURLDefaultsToPreset(t *testing.T) { + clearAllEnv(t) + + cfg := configFile{ + Provider: "litellm", + Providers: map[string]providerEntryConfig{ + "litellm": {APIKey: "sk-litellm-test", Model: "openai/gpt-5.4"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.URL != "http://localhost:4000/v1" { + t.Errorf("URL = %q, want %q (preset default should be used when no url configured)", ep.URL, "http://localhost:4000/v1") + } +} From 73f72b1e097e2581fe5904bd1921303f0396796d Mon Sep 17 00:00:00 2001 From: xp880906 <4232190+xp880906@users.noreply.github.com> Date: Thu, 6 Aug 2026 16:17:36 +0800 Subject: [PATCH 2/3] feat(provider): surface override Base URL in model picker and document it With the wizard now able to set a Base URL override for built-in providers, make the override visible and discoverable. - `ocr config model` shows the effective Base URL for a preset provider (the configured `providers..url` override, or the preset default when none is set) so users can confirm their gateway is in use. - The provider-wizard model-selection step shows the same effective URL via a tab-aware `effectiveBaseURL()` helper (official override/preset, or custom provider URL). - Document `providers..url` as a built-in provider override in the configuration docs, with a litellm example and the preset-as-default semantics; note the wizard's editable Base URL step. Add tests covering the model-selector display (override vs preset default) and the wizard's effectiveBaseURL resolution. --- cmd/opencodereview/provider_cmd.go | 6 + cmd/opencodereview/provider_tui.go | 37 +++++- cmd/opencodereview/provider_tui_funcs_test.go | 107 ++++++++++++++++++ pages/src/content/docs/en/configuration.md | 21 ++++ 4 files changed, 169 insertions(+), 2 deletions(-) diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index 3ab951359..067b79c65 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -320,6 +320,12 @@ func runConfigModel() error { if entry, ok := cfg.Providers[cfg.Provider]; ok { currentModel = activeModelForProvider(cfg, cfg.Provider, entry) provider.Models = mergeModelLists(provider.Models, entry.Models) + // Surface the effective Base URL: a configured override takes + // precedence over the preset default so users can confirm their + // gateway is in use from the model picker. + if entry.URL != "" { + provider.BaseURL = entry.URL + } } } else { isCustom = true diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index daab18875..de5bf4a5c 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -2329,9 +2329,37 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) { } } +// effectiveBaseURL returns the URL to display in the model-selection step: +// the official preset's configured override (entry.URL) or preset.BaseURL, or +// the custom provider's URL. Empty when no provider is selected. +func (m providerTUIModel) effectiveBaseURL() string { + if m.activeTab == tabOfficial { + p := m.currentProvider() + // A configured override (entry.URL) takes precedence over the preset + // default so the model-selection step reflects the gateway in use. + if m.existingCfg != nil { + if entry, ok := m.existingCfg.Providers[p.Name]; ok && entry.URL != "" { + return entry.URL + } + } + if p.BaseURL != "" { + return p.BaseURL + } + } + if cp, ok := m.selectedCustomProvider(); ok && cp.entry.URL != "" { + return cp.entry.URL + } + return "" +} + func (m providerTUIModel) viewModel(s *strings.Builder) { s.WriteString(tuiTitleStyle.Render(fmt.Sprintf(" Select a model (%s)", m.modelProviderName()))) - s.WriteString("\n\n") + s.WriteString("\n") + if url := m.effectiveBaseURL(); url != "" { + s.WriteString(tuiDimStyle.Render(fmt.Sprintf(" Base URL: %s", url))) + s.WriteString("\n") + } + s.WriteString("\n") models := m.models() @@ -3017,7 +3045,12 @@ func (m modelTUIModel) View() tea.View { var s strings.Builder s.WriteString("\n") s.WriteString(tuiTitleStyle.Render(fmt.Sprintf(" Select a model (%s)", m.provider.DisplayName))) - s.WriteString("\n\n") + s.WriteString("\n") + if m.provider.BaseURL != "" { + s.WriteString(tuiDimStyle.Render(fmt.Sprintf(" Base URL: %s", m.provider.BaseURL))) + s.WriteString("\n") + } + s.WriteString("\n") models := m.displayModels() for i, model := range models { diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 1462b3a73..f723babe8 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -1945,3 +1945,110 @@ func TestProviderTUIView_StepModel_CustomTabDeleteHelp(t *testing.T) { t.Errorf("custom model row should show d Delete hint; got:\n%s", got) } } + +// TestModelTUI_ShowsEffectiveBaseURL verifies that the standalone model +// selector (ocr config model) renders the effective Base URL — the configured +// override rather than the preset default — so users can confirm their gateway. +func TestModelTUI_ShowsEffectiveBaseURL(t *testing.T) { + preset, ok := llm.LookupProvider("litellm") + if !ok { + t.Skip("litellm provider not in registry") + } + cfg := &Config{ + Provider: "litellm", + Providers: map[string]ProviderEntry{ + "litellm": {APIKey: "sk-test", Model: "openai/gpt-5.4", URL: "https://gateway.internal:8000/v1"}, + }, + } + provider := preset + provider.Models = mergeModelLists(preset.Models, cfg.Providers["litellm"].Models) + // Mirror runConfigModel's effective-URL resolution. + provider.BaseURL = cfg.Providers["litellm"].URL + + m := newModelTUIConfig(modelTUIConfig{ + Provider: provider, + CurrentModel: "openai/gpt-5.4", + RegistryModels: preset.Models, + ExistingCfg: cfg, + ProviderName: "litellm", + }) + got := stripANSI(m.View().Content) + if !strings.Contains(got, "https://gateway.internal:8000/v1") { + t.Errorf("model view should show the override Base URL; got:\n%s", got) + } + if strings.Contains(got, preset.BaseURL) { + t.Errorf("model view should not show the preset default %q when an override is set; got:\n%s", preset.BaseURL, got) + } +} + +// TestModelTUI_ShowsPresetBaseURLWhenNoOverride verifies the preset default is +// shown when no override is configured. +func TestModelTUI_ShowsPresetBaseURLWhenNoOverride(t *testing.T) { + preset, ok := llm.LookupProvider("litellm") + if !ok { + t.Skip("litellm provider not in registry") + } + cfg := &Config{ + Provider: "litellm", + Providers: map[string]ProviderEntry{ + "litellm": {APIKey: "sk-test", Model: "openai/gpt-5.4"}, + }, + } + provider := preset + provider.Models = mergeModelLists(preset.Models, cfg.Providers["litellm"].Models) + + m := newModelTUIConfig(modelTUIConfig{ + Provider: provider, + CurrentModel: "openai/gpt-5.4", + RegistryModels: preset.Models, + ExistingCfg: cfg, + ProviderName: "litellm", + }) + got := stripANSI(m.View().Content) + if !strings.Contains(got, preset.BaseURL) { + t.Errorf("model view should show the preset default Base URL %q; got:\n%s", preset.BaseURL, got) + } +} + +// TestProviderTUI_EffectiveBaseURL verifies the wizard's model-selection step +// resolves the effective Base URL: override when set, preset default otherwise. +func TestProviderTUI_EffectiveBaseURL(t *testing.T) { + // Override configured. + cfg := &Config{ + Provider: "litellm", + Providers: map[string]ProviderEntry{ + "litellm": {APIKey: "sk-test", Model: "openai/gpt-5.4", URL: "https://gateway.internal:8000/v1"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "litellm" { + m.officialIdx = i + break + } + } + if got := m.effectiveBaseURL(); got != "https://gateway.internal:8000/v1" { + t.Errorf("effectiveBaseURL() = %q, want override", got) + } + + // No override: preset default. + cfg2 := &Config{ + Provider: "litellm", + Providers: map[string]ProviderEntry{ + "litellm": {APIKey: "sk-test", Model: "openai/gpt-5.4"}, + }, + } + m2 := newProviderTUI(cfg2, "") + m2.activeTab = tabOfficial + for i, p := range m2.providers { + if p.Name == "litellm" { + m2.officialIdx = i + break + } + } + preset, _ := llm.LookupProvider("litellm") + if got := m2.effectiveBaseURL(); got != preset.BaseURL { + t.Errorf("effectiveBaseURL() = %q, want preset default %q", got, preset.BaseURL) + } +} diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 52e01b0da..6832f5b72 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -59,6 +59,27 @@ environment variable. | `minimax` | openai | `https://api.minimaxi.com/v1` | `MINIMAX_API_KEY` | | `baidu-qianfan` | openai | `https://qianfan.baidubce.com/v2` | `QIANFAN_API_KEY` | +### Overriding a built-in provider's Base URL + +Every built-in provider has a preset Base URL (shown in the table above). +To point a built-in provider at a different endpoint — for example a +self-hosted LiteLLM gateway that is rarely at the preset default +`http://localhost:4000/v1` — set `providers..url`: + +```bash +ocr config set provider litellm +ocr config set model openai/gpt-5.4 +ocr config set providers.litellm.api_key "$LITELLM_API_KEY" +ocr config set providers.litellm.url https://gateway.internal:8000/v1 +``` + +The configured `url` takes precedence over the preset Base URL. When +`providers..url` is unset (or cleared), OCR falls back to the +preset default — so you only need to set it when your endpoint differs. +The interactive wizard (`ocr config provider`) exposes this as an +editable **Base URL** step for built-in providers, pre-filled with the +preset default. + ### Custom providers Any provider name not in the table above is treated as custom and must From 2f85c192413b94bc80104b8a379fcaef394a911e Mon Sep 17 00:00:00 2001 From: xp880906 <4232190+xp880906@users.noreply.github.com> Date: Thu, 6 Aug 2026 16:17:37 +0800 Subject: [PATCH 3/3] =?UTF-8?q?fix(provider):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20URL=20trim,=20validation,=20dead=20code,=20Esc=20di?= =?UTF-8?q?splay?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address 4 of 5 code review findings on PR #729: 1. URL trim consistency (provider_cmd.go): trim the Base URL once and use the trimmed value for both comparison and persistence, preventing whitespace-polluted URLs from being written to config. 2. URL format validation (provider_cmd.go): validate that the Base URL has an http/https scheme and non-empty host before persisting, giving immediate feedback instead of a runtime failure. Rejects malformed values like bare hosts or ftp:// schemes. 3. Dead code removal (provider_tui.go): remove the init-time pre-fill of officialURLInput that is always overwritten by loadOfficialURL() when the user enters the Base URL step. Pre-fill logic now lives in a single place. 4. effectiveBaseURL reflects pending edit (provider_tui.go): when the user edits the Base URL and presses Esc back to model selection, effectiveBaseURL() now returns the in-progress value from officialURLInput instead of the stale on-disk config. The SSRF/private-IP finding (#2 in review) is not addressed — it is a false positive for a local CLI tool where localhost and private network endpoints are the primary use case (the litellm preset default is http://localhost:4000/v1). --- cmd/opencodereview/provider_cmd.go | 26 +++++++- cmd/opencodereview/provider_cmd_test.go | 84 +++++++++++++++++++++++++ cmd/opencodereview/provider_tui.go | 16 ++--- cmd/opencodereview/provider_tui_test.go | 43 +++++++++++++ 4 files changed, 157 insertions(+), 12 deletions(-) diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index 067b79c65..a52732471 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "fmt" + "net/url" "os" "path/filepath" "strings" @@ -264,8 +265,12 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider // Persist a Base URL override only when it differs from the preset default. // An empty/unchanged value clears any prior override so the preset BaseURL // remains the default, matching the "preset is the fallback" contract. - if isPreset && strings.TrimSpace(result.url) != "" && result.url != preset.BaseURL { - entry.URL = result.url + trimmedURL := strings.TrimSpace(result.url) + if isPreset && trimmedURL != "" && trimmedURL != preset.BaseURL { + if err := validateBaseURL(trimmedURL); err != nil { + return err + } + entry.URL = trimmedURL } else { entry.URL = "" } @@ -424,3 +429,20 @@ func maskKey(key string) string { } return key[:4] + "***" + key[len(key)-4:] } + +// validateBaseURL checks that a provider Base URL has an http or https scheme +// and a non-empty host, giving the user immediate feedback in the TUI rather +// than a runtime failure when the LLM client tries to use it. +func validateBaseURL(raw string) error { + parsed, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("invalid Base URL %q: %w", raw, err) + } + if parsed.Scheme != "http" && parsed.Scheme != "https" { + return fmt.Errorf("Base URL must use http or https scheme, got %q", parsed.Scheme) + } + if parsed.Host == "" { + return fmt.Errorf("Base URL %q must include a host", raw) + } + return nil +} diff --git a/cmd/opencodereview/provider_cmd_test.go b/cmd/opencodereview/provider_cmd_test.go index 86cca1c38..caaea1830 100644 --- a/cmd/opencodereview/provider_cmd_test.go +++ b/cmd/opencodereview/provider_cmd_test.go @@ -436,3 +436,87 @@ func TestApplyOfficialProviderConfig_ClearsURLWhenPresetDefault(t *testing.T) { t.Errorf("persisted URL = %q, want empty (preset default should not persist a url)", got) } } + +// TestApplyOfficialProviderConfig_TrimsURLWhitespace verifies that a Base URL +// with surrounding whitespace is trimmed before comparison and persistence, so +// whitespace-polluted values are never written to the config file. +func TestApplyOfficialProviderConfig_TrimsURLWhitespace(t *testing.T) { + t.Setenv("LITELLM_API_KEY", "sk-litellm") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + cfg := &Config{} + + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "litellm", + model: "openai/gpt-5.4", + apiKey: "sk-litellm", + url: " https://gateway.internal:8000/v1 ", + }) + if err != nil { + t.Fatalf("applyOfficialProviderConfig: %v", err) + } + if got := cfg.Providers["litellm"].URL; got != "https://gateway.internal:8000/v1" { + t.Errorf("persisted URL = %q, want trimmed value", got) + } +} + +// TestApplyOfficialProviderConfig_RejectsInvalidScheme verifies that a Base URL +// without an http/https scheme is rejected with a clear error at config time +// rather than failing later at runtime. +func TestApplyOfficialProviderConfig_RejectsInvalidScheme(t *testing.T) { + t.Setenv("LITELLM_API_KEY", "sk-litellm") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + cfg := &Config{} + + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "litellm", + model: "openai/gpt-5.4", + apiKey: "sk-litellm", + url: "ftp://example.com/v1", + }) + if err == nil { + t.Fatal("expected error for non-http scheme, got nil") + } +} + +// TestApplyOfficialProviderConfig_RejectsMissingScheme verifies that a Base URL +// lacking a scheme (e.g. a bare host) is rejected at config time. +func TestApplyOfficialProviderConfig_RejectsMissingScheme(t *testing.T) { + t.Setenv("LITELLM_API_KEY", "sk-litellm") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + cfg := &Config{} + + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "litellm", + model: "openai/gpt-5.4", + apiKey: "sk-litellm", + url: "api.example.com/v1", + }) + if err == nil { + t.Fatal("expected error for URL missing scheme, got nil") + } +} + +// TestApplyOfficialProviderConfig_AcceptsHTTP verifies that plain http:// URLs +// are accepted (the default litellm preset is http://localhost:4000/v1). +func TestApplyOfficialProviderConfig_AcceptsHTTP(t *testing.T) { + t.Setenv("LITELLM_API_KEY", "sk-litellm") + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + cfg := &Config{} + + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "litellm", + model: "openai/gpt-5.4", + apiKey: "sk-litellm", + url: "http://my-litellm.local:4000/v1", + }) + if err != nil { + t.Fatalf("applyOfficialProviderConfig: %v", err) + } + if got := cfg.Providers["litellm"].URL; got != "http://my-litellm.local:4000/v1" { + t.Errorf("persisted URL = %q, want http URL", got) + } +} diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index de5bf4a5c..0795fd843 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -349,16 +349,6 @@ func newProviderTUI(cfg *Config, configPath string) providerTUIModel { m.apiKeyOriginal = entry.APIKey m.apiKeyMasked = true } - - // Pre-fill the official Base URL input with the effective URL: a - // configured override (entry.URL) if present, else the preset default. - // Editing this value later overrides preset.BaseURL in the resolver. - selected := providers[m.officialIdx] - effectiveURL := selected.BaseURL - if entry, ok := cfg.Providers[cfg.Provider]; ok && entry.URL != "" { - effectiveURL = entry.URL - } - m.officialURLInput.SetValue(effectiveURL) } if cfg.Provider == "" && cfg.Llm.URL != "" { @@ -2335,6 +2325,12 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) { func (m providerTUIModel) effectiveBaseURL() string { if m.activeTab == tabOfficial { p := m.currentProvider() + // When the user has an in-progress edit in the Base URL step (e.g. they + // typed a new URL and pressed Esc back to model selection), reflect that + // pending value rather than the stale on-disk config. + if v := strings.TrimSpace(m.officialURLInput.Value()); v != "" { + return v + } // A configured override (entry.URL) takes precedence over the preset // default so the model-selection step reflects the gateway in use. if m.existingCfg != nil { diff --git a/cmd/opencodereview/provider_tui_test.go b/cmd/opencodereview/provider_tui_test.go index 4b300be3d..46fddf33a 100644 --- a/cmd/opencodereview/provider_tui_test.go +++ b/cmd/opencodereview/provider_tui_test.go @@ -3062,3 +3062,46 @@ func TestProviderTUI_OfficialBaseURLPrefilledWithOverride(t *testing.T) { t.Errorf("after Esc on stepBaseURL, step = %d, want stepModel", m3.step) } } + +// TestProviderTUI_EffectiveBaseURLReflectsPendingEdit verifies that after the +// user edits the Base URL in the stepBaseURL step and returns to model +// selection (Esc), effectiveBaseURL() reflects the in-progress value rather +// than the stale on-disk config. +func TestProviderTUI_EffectiveBaseURLReflectsPendingEdit(t *testing.T) { + cfg := &Config{ + Provider: "litellm", + Providers: map[string]ProviderEntry{ + "litellm": {APIKey: "sk-test", Model: "openai/gpt-5.4"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "litellm" { + m.officialIdx = i + break + } + } + m.step = stepModel + m.modelIdx = modelIdxForName(t, m, "openai/gpt-5.4") + + // Enter the Base URL step. + result, _ := m.Update(enterKey()) + m2 := result.(providerTUIModel) + if m2.step != stepBaseURL { + t.Fatalf("step = %d, want stepBaseURL", m2.step) + } + + // Simulate the user typing a new URL. + m2.officialURLInput.SetValue("https://my-new-gateway.internal:9000/v1") + + // Esc back to model selection — the model step should show the pending edit. + result, _ = m2.Update(escKey()) + m3 := result.(providerTUIModel) + if m3.step != stepModel { + t.Fatalf("after Esc on stepBaseURL, step = %d, want stepModel", m3.step) + } + if got := m3.effectiveBaseURL(); got != "https://my-new-gateway.internal:9000/v1" { + t.Errorf("effectiveBaseURL() = %q, want the pending edit value", got) + } +}