From 2888a5ca4e62543f4eb249e32a3b066fcbe2d3ec Mon Sep 17 00:00:00 2001 From: Engel Nyst Date: Sat, 13 Jun 2026 05:05:25 +0200 Subject: [PATCH] fix: preserve hidden LLM base URL on basic saves (#1347) Co-authored-by: openhands --- .../llm-settings-local-view.test.tsx | 91 +++++++++++++++---- __tests__/routes/llm-settings.test.tsx | 37 ++++++++ .../llm-profiles/llm-settings-local-view.tsx | 11 ++- src/routes/llm-settings.tsx | 2 +- .../mock-llm-profile-management.spec.ts | 49 +++++----- 5 files changed, 143 insertions(+), 47 deletions(-) diff --git a/__tests__/components/settings/llm-profiles/llm-settings-local-view.test.tsx b/__tests__/components/settings/llm-profiles/llm-settings-local-view.test.tsx index 89a2d062fe..6f2bec2644 100644 --- a/__tests__/components/settings/llm-profiles/llm-settings-local-view.test.tsx +++ b/__tests__/components/settings/llm-profiles/llm-settings-local-view.test.tsx @@ -30,14 +30,29 @@ vi.mock("#/routes/llm-settings", async () => { }) => void; }) => { const initialValueOverridesRef = React.useRef(initialValueOverrides); + const initialValuesRef = React.useRef({ + "llm.model": "openai/gpt-4o", + "llm.api_key": "test-api-key", + "llm.base_url": "", + ...(initialValueOverrides ?? {}), + }); const [view, setView] = React.useState<"basic" | "all">("basic"); + const [model, setModel] = React.useState( + String(initialValuesRef.current["llm.model"] ?? ""), + ); + const [apiKey] = React.useState( + String(initialValuesRef.current["llm.api_key"] ?? ""), + ); + const [baseUrl] = React.useState( + String(initialValuesRef.current["llm.base_url"] ?? ""), + ); const [temperature, setTemperature] = React.useState("0.2"); React.useEffect(() => { const values = { - "llm.model": "openai/gpt-4o", - "llm.api_key": "test-api-key", - "llm.base_url": "", ...(initialValueOverridesRef.current ?? {}), + "llm.model": model, + "llm.api_key": apiKey, + "llm.base_url": baseUrl, }; onSaveControlChange?.({ save: vi.fn(), @@ -58,7 +73,7 @@ vi.mock("#/routes/llm-settings", async () => { }; }, }); - }, [onSaveControlChange, temperature, view]); + }, [apiKey, baseUrl, model, onSaveControlChange, temperature, view]); return (
@@ -76,6 +91,13 @@ vi.mock("#/routes/llm-settings", async () => { > All + {view === "basic" ? ( + setModel(event.currentTarget.value)} + /> + ) : null} {view === "all" ? ( { }); describe("Basic tab save", () => { - it("drops stale base_url for OpenHands models in Basic mode", async () => { - // Arrange — a profile whose stored config pairs an OpenHands model with a - // stale base_url. In Basic mode the public provider prefix is enough; the - // SDK derives provider transport details when making LLM calls. + it("preserves hidden base_url for OpenHands models without a model change", async () => { + // Arrange — a profile has an actual advanced base_url value. Switching to + // Basic hides it, but saving without changing the model must not wipe it. const user = userEvent.setup(); vi.mocked(ProfilesService.getProfile).mockResolvedValue({ name: "gpt-4-profile", @@ -625,18 +646,16 @@ describe("LlmSettingsLocalView", () => { }); await user.click(screen.getByTestId("save-profile-btn")); - // Assert — the saved LLM config keeps the OpenHands model and drops the - // stale base_url instead of re-stamping a LiteLLM proxy detail. + // Assert — the hidden base_url survives because the model did not change. await waitFor(() => expect(mockSaveMutateAsync).toHaveBeenCalled()); const savedLlm = mockSaveMutateAsync.mock.calls[0][0].request.llm; expect(savedLlm.model).toBe("openhands/claude-opus-4-5-20251101"); - expect(savedLlm).not.toHaveProperty("base_url"); + expect(savedLlm.base_url).toBe("https://stale.example.com/v1"); }); - it("drops base_url for stored litellm_proxy profiles in Basic mode", async () => { - // Arrange — legacy profiles may still contain a LiteLLM proxy model and - // proxy base_url. The SDK migration owns that compatibility; the Basic tab - // should not keep re-stamping transport details. + it("preserves hidden base_url for stored litellm_proxy profiles without a model change", async () => { + // Arrange — legacy/custom proxy profiles may still have a base_url. Basic + // view must not erase that invisible value on a same-model save. const user = userEvent.setup(); vi.mocked(ProfilesService.getProfile).mockResolvedValue({ name: "gpt-4-profile", @@ -666,11 +685,49 @@ describe("LlmSettingsLocalView", () => { }); await user.click(screen.getByTestId("save-profile-btn")); - // Assert — the legacy model is preserved, but the Basic tab drops the - // base_url instead of reverse-mapping it in the frontend. + // Assert — the legacy model and hidden base_url are both preserved. await waitFor(() => expect(mockSaveMutateAsync).toHaveBeenCalled()); const savedLlm = mockSaveMutateAsync.mock.calls[0][0].request.llm; expect(savedLlm.model).toBe("litellm_proxy/claude-opus-4-8"); + expect(savedLlm.base_url).toBe("https://llm-proxy.app.all-hands.dev/"); + }); + + it("drops hidden base_url when the Basic view model changes", async () => { + // Arrange — the existing base_url belongs to the old model/provider. + const user = userEvent.setup(); + vi.mocked(ProfilesService.getProfile).mockResolvedValue({ + name: "gpt-4-profile", + api_key_set: true, + config: { + model: "openhands/claude-opus-4-5-20251101", + api_key: "gAAAA_encrypted_key", + base_url: "https://stale.example.com/v1", + }, + }); + mockSaveMutateAsync.mockResolvedValueOnce({ success: true }); + + renderWithProviders(); + + await user.click(screen.getAllByTestId("profile-menu-trigger")[0]); + await user.click(screen.getByTestId("profile-edit")); + await waitFor(() => { + expect(screen.getByTestId("profile-name-input")).toHaveValue( + "gpt-4-profile", + ); + }); + await user.click(await screen.findByTestId("sdk-section-basic-toggle")); + const modelInput = await screen.findByTestId("mock-basic-model-input"); + await user.clear(modelInput); + await user.type(modelInput, "openhands/claude-sonnet-4-20250514"); + await waitFor(() => { + expect(screen.getByTestId("save-profile-btn")).not.toBeDisabled(); + }); + await user.click(screen.getByTestId("save-profile-btn")); + + // Assert — changing the Basic model clears the old hidden base_url. + await waitFor(() => expect(mockSaveMutateAsync).toHaveBeenCalled()); + const savedLlm = mockSaveMutateAsync.mock.calls[0][0].request.llm; + expect(savedLlm.model).toBe("openhands/claude-sonnet-4-20250514"); expect(savedLlm).not.toHaveProperty("base_url"); }); }); diff --git a/__tests__/routes/llm-settings.test.tsx b/__tests__/routes/llm-settings.test.tsx index a60d9c4869..86a615a8a8 100644 --- a/__tests__/routes/llm-settings.test.tsx +++ b/__tests__/routes/llm-settings.test.tsx @@ -141,6 +141,43 @@ describe("LlmSettingsScreen", () => { expect(screen.getByTestId("llm-api-key-input")).toHaveValue(""); }); + it("does not clear an existing base URL on Basic save without a model change", async () => { + const saveSettingsSpy = vi + .spyOn(SettingsService, "saveSettings") + .mockResolvedValue(true); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue( + buildSettings({ + llm_model: "openai/gpt-4o", + llm_base_url: "https://custom.example/v1", + agent_settings: { + ...MOCK_DEFAULT_USER_SETTINGS.agent_settings, + llm: { + model: "openai/gpt-4o", + api_key: null, + base_url: "https://custom.example/v1", + }, + }, + }), + ); + + renderLlmSettingsScreen(); + + await screen.findByTestId("llm-settings-screen"); + fireEvent.click(screen.getByTestId("sdk-section-basic-toggle")); + fireEvent.change(screen.getByTestId("llm-api-key-input"), { + target: { value: "test-api-key" }, + }); + fireEvent.click(screen.getByTestId("save-button")); + + await waitFor(() => expect(saveSettingsSpy).toHaveBeenCalled()); + const payload = saveSettingsSpy.mock.calls[0][0] as Record; + const llmPayload = ( + payload.agent_settings_diff as Record + ).llm as Record; + expect(llmPayload.api_key).toBe("test-api-key"); + expect(llmPayload).not.toHaveProperty("base_url"); + }); + it("does not show a 'key set' indicator for a brand-new embedded profile even when a global key exists (bug #640)", async () => { // A global key exists, but a fresh profile form must look unset so the user // knows they have to enter one — otherwise the profile saves with no key. diff --git a/src/components/features/settings/llm-profiles/llm-settings-local-view.tsx b/src/components/features/settings/llm-profiles/llm-settings-local-view.tsx index caeaa3872c..b91eab40c2 100644 --- a/src/components/features/settings/llm-profiles/llm-settings-local-view.tsx +++ b/src/components/features/settings/llm-profiles/llm-settings-local-view.tsx @@ -262,6 +262,10 @@ export function LlmSettingsLocalView() { viewMode === "edit" && editingProfile?.baseConfig ? { ...editingProfile.baseConfig } : {}; + const didChangeModelInBasic = + saveControl.view === "basic" && + Object.prototype.hasOwnProperty.call(dirtyLlm, "model") && + dirtyLlm.model !== baseConfig.model; const llmConfig: Record = { ...baseConfig, ...dirtyLlm }; const authType = resolveLlmAuthType(llmConfig.auth_type); @@ -274,9 +278,10 @@ export function LlmSettingsLocalView() { llmConfig.auth_type = LLM_AUTH_TYPE_API_KEY; llmConfig.subscription_vendor = null; - // The Basic tab has no base_url field. Provider defaults are handled by - // the backend, so drop stale custom values for every provider. - if (saveControl.view === "basic") { + // The Basic tab has no base_url field. Preserve an existing hidden value + // when the model did not actually change; if the user chooses a new model, + // drop the old base URL so provider defaults can apply to that model. + if (didChangeModelInBasic) { delete llmConfig.base_url; } diff --git a/src/routes/llm-settings.tsx b/src/routes/llm-settings.tsx index 9b66a489e9..eefacf2df0 100644 --- a/src/routes/llm-settings.tsx +++ b/src/routes/llm-settings.tsx @@ -462,7 +462,7 @@ export function LlmSettingsScreen({ llm.auth_type = LLM_AUTH_TYPE_API_KEY; llm.subscription_vendor = null; } - if (context.view === "basic") { + if (context.view === "basic" && llm.model !== undefined) { llm.base_url = getSchemaFieldDefaultValue(schema, "llm.base_url"); } } diff --git a/tests/e2e/mock-llm/mock-llm-profile-management.spec.ts b/tests/e2e/mock-llm/mock-llm-profile-management.spec.ts index 78a4929e5a..521d95c204 100644 --- a/tests/e2e/mock-llm/mock-llm-profile-management.spec.ts +++ b/tests/e2e/mock-llm/mock-llm-profile-management.spec.ts @@ -17,11 +17,10 @@ * stamps the active profile name on client-side conversation * metadata at creation and on per-conversation switches. * - * 3. OpenHands provider base_url normalization: - * The public `openhands/` model namespace is enough to preserve - * profile identity. Re-saving an OpenHands profile from the Basic tab - * must drop stale LiteLLM proxy base_url details so the SDK owns - * transport-time mapping. + * 3. OpenHands provider hidden base_url preservation: + * A base_url typed in Advanced view is still real profile data after + * switching to Basic. Re-saving from Basic without changing the model + * must preserve that hidden value. */ import { test, expect } from "@playwright/test"; @@ -360,19 +359,18 @@ test.describe("same-model profile identity", () => { }); // ═══════════════════════════════════════════════════════════════════════ -// Test 3 — OpenHands provider base_url normalization +// Test 3 — OpenHands provider hidden base_url preservation // ═══════════════════════════════════════════════════════════════════════ -test.describe("OpenHands provider base_url normalization", () => { - // The public OpenHands provider model is the target persisted identity. Older - // agent-server releases may still report the transitional litellm_proxy form, - // but Basic-mode saves should not continue stamping proxy transport details - // in either case. +test.describe("OpenHands provider hidden base_url preservation", () => { + // The public OpenHands provider model remains the profile identity, but a + // custom Advanced base_url is still user data. A same-model Basic save must + // not wipe it just because the field is invisible in that view. const OPENHANDS_PROFILE = "openhands-basic-save-test"; const OPENHANDS_MODEL = "openhands/claude-opus-4-5-20251101"; const LEGACY_TRANSPORT_MODEL = "litellm_proxy/claude-opus-4-5-20251101"; const EXPECTED_OPENHANDS_MODELS = [OPENHANDS_MODEL, LEGACY_TRANSPORT_MODEL]; - const OPENHANDS_PROXY_BASE_URL = "https://llm-proxy.app.all-hands.dev/"; + const CUSTOM_BASE_URL = "https://custom-openhands-proxy.example/v1"; test.beforeEach(async ({ page }) => { await seedLocalStorage(page); @@ -394,13 +392,13 @@ test.describe("OpenHands provider base_url normalization", () => { } }); - test("re-saving an OpenHands profile from Basic view drops proxy base_url", async ({ + test("re-saving an OpenHands profile from Basic view preserves hidden base_url", async ({ page, request, }) => { - // ── Setup: create a public OpenHands profile with a stale proxy base_url - // through the Settings UI. This mirrors legacy/manual profile state while - // keeping the public openhands/* model namespace that this PR trusts. ── + // ── Setup: create a public OpenHands profile with a custom base_url through + // Advanced view. The value becomes hidden after switching to Basic, but it + // is still part of the profile unless the model changes. ── await routeSessionApiKey(page); await page.goto("/settings/llm", { waitUntil: "domcontentloaded" }); await dismissAnalyticsModal(page); @@ -410,7 +408,7 @@ test.describe("OpenHands provider base_url normalization", () => { await createProfileViaUI(page, { profileName: OPENHANDS_PROFILE, model: OPENHANDS_MODEL, - baseUrl: OPENHANDS_PROXY_BASE_URL, + baseUrl: CUSTOM_BASE_URL, }); await test.step("open profile in edit mode", async () => { @@ -441,9 +439,9 @@ test.describe("OpenHands provider base_url normalization", () => { ); }); - await test.step("switch to Basic view and save", async () => { + await test.step("switch to Basic view and save without changing model", async () => { // Click the Basic toggle explicitly so the save path exercises the view - // that hides base_url and therefore drops stale provider transport data. + // that hides base_url without changing the selected model. const basicToggle = page.getByTestId("sdk-section-basic-toggle"); if (await basicToggle.isVisible().catch(() => false)) { await basicToggle.click(); @@ -456,14 +454,13 @@ test.describe("OpenHands provider base_url normalization", () => { await waitForTestId(page, "add-llm-profile"); }); - // ── Verify: Basic-tab save removed the stale proxy base_url ── - await test.step("verify base_url is dropped after save", async () => { + // ── Verify: Basic-tab save preserved the hidden custom base_url ── + await test.step("verify base_url is preserved after save", async () => { const config = await getProfileConfig(request, OPENHANDS_PROFILE); expect( - config.base_url ?? null, - "base_url should not be persisted for public OpenHands models after " + - "a Basic-tab re-save; the SDK owns transport-time proxy mapping", - ).toBeNull(); + config.base_url, + "Basic-tab re-save without a model change must not clear hidden base_url", + ).toBe(CUSTOM_BASE_URL); expect(EXPECTED_OPENHANDS_MODELS).toContain(config.model); }); @@ -473,7 +470,7 @@ test.describe("OpenHands provider base_url normalization", () => { // Re-read via API to confirm persistence is durable const config = await getProfileConfig(request, OPENHANDS_PROFILE); - expect(config.base_url ?? null).toBeNull(); + expect(config.base_url).toBe(CUSTOM_BASE_URL); expect(EXPECTED_OPENHANDS_MODELS).toContain(config.model); }); });