From 7fcfb8fafcf4e95ca11615bcfe0f84c9161dafa9 Mon Sep 17 00:00:00 2001 From: enyst Date: Sat, 13 Jun 2026 01:45:35 +0000 Subject: [PATCH] settings: trust public OpenHands provider models Co-authored-by: openhands --- .../model-selector-openhands.test.tsx | 22 +-- .../llm-settings-local-view.test.tsx | 49 +++++- .../utils/normalize-display-model.test.ts | 39 ----- __tests__/utils/openhands-llm.test.ts | 28 ---- .../shared/modals/settings/model-selector.tsx | 22 +-- .../shared/modals/settings/settings-form.tsx | 4 - src/routes/llm-settings.tsx | 10 -- src/utils/normalize-display-model.ts | 24 --- src/utils/openhands-llm.ts | 22 --- .../mock-llm-profile-management.spec.ts | 150 ++++++++++++++++++ 10 files changed, 208 insertions(+), 162 deletions(-) delete mode 100644 __tests__/utils/normalize-display-model.test.ts delete mode 100644 __tests__/utils/openhands-llm.test.ts delete mode 100644 src/utils/normalize-display-model.ts delete mode 100644 src/utils/openhands-llm.ts diff --git a/__tests__/components/modals/settings/model-selector-openhands.test.tsx b/__tests__/components/modals/settings/model-selector-openhands.test.tsx index 6d8a4cf42..ef7c9e417 100644 --- a/__tests__/components/modals/settings/model-selector-openhands.test.tsx +++ b/__tests__/components/modals/settings/model-selector-openhands.test.tsx @@ -6,7 +6,7 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { ModelSelector } from "#/components/shared/modals/settings/model-selector"; import { server } from "#/mocks/node"; -describe("ModelSelector — OpenHands round-trip", () => { +describe("ModelSelector — OpenHands provider", () => { let providersCount = 0; let verifiedCount = 0; let modelsCount = 0; @@ -20,7 +20,9 @@ describe("ModelSelector — OpenHands round-trip", () => { server.use( http.get("*/api/llm/providers", () => { providersCount += 1; - return HttpResponse.json({ providers: ["anthropic", "openai"] }); + return HttpResponse.json({ + providers: ["openhands", "anthropic", "openai"], + }); }), http.get("*/api/llm/models/verified", () => { verifiedCount += 1; @@ -47,25 +49,13 @@ describe("ModelSelector — OpenHands round-trip", () => { ); } - it("shows OpenHands as the provider for a persisted litellm_proxy/ + All-Hands proxy base URL, fetching each LLM endpoint exactly once", async () => { - // Arrange — mirrors the post-save state: the SDK rewrote openhands/ - // to litellm_proxy/ on disk and pinned the All-Hands proxy base URL. - renderWithQuery( - , - ); + it("shows OpenHands as the provider for a public openhands/ model", async () => { + renderWithQuery(); - // Act / Assert — the bootstrap effect must wait for the openhands - // verified list to load before resolving the displayed provider. await waitFor(() => { expect(screen.getByLabelText("LLM$PROVIDER")).toHaveValue("OpenHands"); }); - // Assert — the three queries that all need verified-models data share - // a single cache entry, and the bootstrap effect picks the right - // provider on its first run, so /models is not fetched twice. expect(providersCount).toBe(1); expect(verifiedCount).toBe(1); expect(modelsCount).toBe(1); 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 ff6852d94..89a2d062f 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 @@ -593,7 +593,10 @@ describe("LlmSettingsLocalView", () => { }); describe("Basic tab save", () => { - it("drops hidden base_url values for OpenHands models", async () => { + 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. const user = userEvent.setup(); vi.mocked(ProfilesService.getProfile).mockResolvedValue({ name: "gpt-4-profile", @@ -608,6 +611,7 @@ describe("LlmSettingsLocalView", () => { renderWithProviders(); + // Act — open the profile in edit mode, force the Basic tab, and save. await user.click(screen.getAllByTestId("profile-menu-trigger")[0]); await user.click(screen.getByTestId("profile-edit")); await waitFor(() => { @@ -621,11 +625,54 @@ 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. 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"); }); + + 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. + const user = userEvent.setup(); + vi.mocked(ProfilesService.getProfile).mockResolvedValue({ + name: "gpt-4-profile", + api_key_set: true, + config: { + model: "litellm_proxy/claude-opus-4-8", + api_key: "gAAAA_encrypted_key", + base_url: "https://llm-proxy.app.all-hands.dev/", + }, + }); + mockSaveMutateAsync.mockResolvedValueOnce({ success: true }); + + renderWithProviders(); + + // Act — open the profile in edit mode, force the Basic tab, and save + // without touching the model dropdown. + 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")); + await waitFor(() => { + expect(screen.getByTestId("save-profile-btn")).not.toBeDisabled(); + }); + 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. + 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).not.toHaveProperty("base_url"); + }); }); describe("All tab save", () => { diff --git a/__tests__/utils/normalize-display-model.test.ts b/__tests__/utils/normalize-display-model.test.ts deleted file mode 100644 index 7b19fe79d..000000000 --- a/__tests__/utils/normalize-display-model.test.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { normalizeDisplayModel } from "#/utils/normalize-display-model"; - -const OPENHANDS_VERIFIED = ["claude-opus-4-7", "gpt-5.5"]; -const OPENHANDS_PROXY_BASE_URL = "https://llm-proxy.app.all-hands.dev/"; - -describe("normalizeDisplayModel", () => { - it("rewrites litellm_proxy/ to openhands/ when the base URL is the All-Hands proxy and the model is in the openhands verified list", () => { - // Arrange — mirrors the SDK round-trip: openhands/ on save becomes - // litellm_proxy/ on disk plus the All-Hands proxy base URL. - const persistedModel = "litellm_proxy/claude-opus-4-7"; - - // Act - const result = normalizeDisplayModel( - persistedModel, - OPENHANDS_PROXY_BASE_URL, - OPENHANDS_VERIFIED, - ); - - // Assert - expect(result).toBe("openhands/claude-opus-4-7"); - }); - - it("leaves litellm_proxy/ untouched when the base URL is not the All-Hands proxy", () => { - // Arrange — a user-configured litellm_proxy pointed at a non-OpenHands - // gateway must not be re-labelled as OpenHands. - const persistedModel = "litellm_proxy/claude-opus-4-7"; - - // Act - const result = normalizeDisplayModel( - persistedModel, - "https://other-proxy.example.com/", - OPENHANDS_VERIFIED, - ); - - // Assert - expect(result).toBe("litellm_proxy/claude-opus-4-7"); - }); -}); diff --git a/__tests__/utils/openhands-llm.test.ts b/__tests__/utils/openhands-llm.test.ts deleted file mode 100644 index 4f387d002..000000000 --- a/__tests__/utils/openhands-llm.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - OPENHANDS_LLM_PROXY_BASE_URL, - isOpenHandsProxyBaseUrl, -} from "#/utils/openhands-llm"; - -describe("openhands LLM helpers", () => { - it("exports the All-Hands LiteLLM proxy base URL", () => { - expect(OPENHANDS_LLM_PROXY_BASE_URL).toBe( - "https://llm-proxy.app.all-hands.dev/", - ); - }); - - it("recognizes the All-Hands proxy base URL regardless of trailing slash or /v1", () => { - expect(isOpenHandsProxyBaseUrl(OPENHANDS_LLM_PROXY_BASE_URL)).toBe(true); - expect(isOpenHandsProxyBaseUrl("https://llm-proxy.app.all-hands.dev")).toBe( - true, - ); - expect( - isOpenHandsProxyBaseUrl("https://llm-proxy.app.all-hands.dev/v1"), - ).toBe(true); - expect(isOpenHandsProxyBaseUrl("https://other-proxy.example.com")).toBe( - false, - ); - expect(isOpenHandsProxyBaseUrl(null)).toBe(false); - expect(isOpenHandsProxyBaseUrl(undefined)).toBe(false); - }); -}); diff --git a/src/components/shared/modals/settings/model-selector.tsx b/src/components/shared/modals/settings/model-selector.tsx index e6ef01dc3..bc2361e21 100644 --- a/src/components/shared/modals/settings/model-selector.tsx +++ b/src/components/shared/modals/settings/model-selector.tsx @@ -15,13 +15,10 @@ import { HelpLink } from "#/ui/help-link"; import { PRODUCT_URL } from "#/utils/constants"; import { useSearchProviders } from "#/hooks/query/use-search-providers"; import { useProviderModels } from "#/hooks/query/use-provider-models"; -import { useOpenhandsVerifiedModels } from "#/hooks/query/use-openhands-verified-models"; -import { normalizeDisplayModel } from "#/utils/normalize-display-model"; interface ModelSelectorProps { isDisabled?: boolean; currentModel?: string; - currentBaseUrl?: string; onChange?: (provider: string | null, model: string | null) => void; onDefaultValuesChanged?: ( provider: string | null, @@ -34,7 +31,6 @@ interface ModelSelectorProps { export function ModelSelector({ isDisabled, currentModel, - currentBaseUrl, onChange, onDefaultValuesChanged, wrapperClassName, @@ -47,7 +43,6 @@ export function ModelSelector({ const [selectedModel, setSelectedModel] = React.useState(null); const { data: providers = [] } = useSearchProviders(); - const { data: openhandsVerifiedModels } = useOpenhandsVerifiedModels(); const { data: providerModels = [], isLoading: isLoadingModels, @@ -73,24 +68,15 @@ export function ModelSelector({ ); React.useEffect(() => { - // Wait for the openhands verified list before initializing — otherwise a - // persisted `litellm_proxy/` model would first land as `litellm_proxy` - // and only later flip to `openhands`, triggering a redundant /api/llm/models - // fetch for the throwaway provider value. - if (currentModel && openhandsVerifiedModels !== undefined) { - const displayModel = normalizeDisplayModel( - currentModel, - currentBaseUrl, - openhandsVerifiedModels, - ); - const { provider, model } = extractModelAndProvider(displayModel); + if (currentModel) { + const { provider, model } = extractModelAndProvider(currentModel); - setLitellmId(displayModel); + setLitellmId(currentModel); setSelectedProvider(provider || null); setSelectedModel(model); onDefaultValuesChanged?.(provider || null, model); } - }, [currentModel, currentBaseUrl, openhandsVerifiedModels]); + }, [currentModel]); const handleChangeProvider = (provider: string) => { setSelectedProvider(provider); diff --git a/src/components/shared/modals/settings/settings-form.tsx b/src/components/shared/modals/settings/settings-form.tsx index 3445e03c2..ca3b85409 100644 --- a/src/components/shared/modals/settings/settings-form.tsx +++ b/src/components/shared/modals/settings/settings-form.tsx @@ -70,7 +70,6 @@ export function SettingsForm({ settings, onClose }: SettingsFormProps) { const isLLMKeySet = settings.llm_api_key_set; const currentModel = getAgentSettingValue(settings, "llm.model"); - const currentBaseUrl = getAgentSettingValue(settings, "llm.base_url"); return (
@@ -85,9 +84,6 @@ export function SettingsForm({ settings, onClose }: SettingsFormProps) { currentModel={ typeof currentModel === "string" ? currentModel : undefined } - currentBaseUrl={ - typeof currentBaseUrl === "string" ? currentBaseUrl : undefined - } wrapperClassName="!flex-col !gap-[17px]" labelClassName={SETTINGS_FORM.LABEL_CLASSNAME} /> diff --git a/src/routes/llm-settings.tsx b/src/routes/llm-settings.tsx index 37b96582e..9b66a489e 100644 --- a/src/routes/llm-settings.tsx +++ b/src/routes/llm-settings.tsx @@ -33,7 +33,6 @@ import { resolveLlmAuthType, } from "#/constants/llm-subscription"; import { useOpenAISubscriptionModels } from "#/hooks/query/use-llm-subscription-models"; -import { OPENHANDS_LLM_PROXY_BASE_URL } from "#/utils/openhands-llm"; const LLM_EXCLUDED_KEYS = new Set([ "llm.model", @@ -58,14 +57,6 @@ const getSchemaFieldDefaultValue = ( const KNOWN_PROVIDER_DEFAULT_BASE_URLS: Partial>> = { openai: new Set(["https://api.openai.com", "https://api.openai.com/v1"]), - openhands: new Set([ - OPENHANDS_LLM_PROXY_BASE_URL, - "https://llm-proxy.app.all-hands.dev/v1", - ]), - litellm_proxy: new Set([ - OPENHANDS_LLM_PROXY_BASE_URL, - "https://llm-proxy.app.all-hands.dev/v1", - ]), }; const normalizeBaseUrl = (baseUrl: string) => { @@ -350,7 +341,6 @@ export function LlmSettingsScreen({ <> { const nextModel = buildModelId(provider, model); if (nextModel) { diff --git a/src/utils/normalize-display-model.ts b/src/utils/normalize-display-model.ts deleted file mode 100644 index 0ae6504e2..000000000 --- a/src/utils/normalize-display-model.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { isOpenHandsProxyBaseUrl } from "#/utils/openhands-llm"; - -const LITELLM_PROXY_PREFIX = "litellm_proxy/"; - -/** - * Reverse the SDK's `openhands/* -> litellm_proxy/*` rewrite for display. - * - * The agent-server's LLM validator stores curated OpenHands models as - * `litellm_proxy/` against the All-Hands proxy base URL. Without - * normalization the GUI re-loads such settings as `litellm_proxy`, so the - * provider dropdown silently switches off "OpenHands" after every save. - */ -export function normalizeDisplayModel( - rawModel: string | null | undefined, - baseUrl: string | null | undefined, - openhandsVerifiedModels: readonly string[], -): string { - if (!rawModel) return ""; - if (!rawModel.startsWith(LITELLM_PROXY_PREFIX)) return rawModel; - if (!isOpenHandsProxyBaseUrl(baseUrl)) return rawModel; - const modelName = rawModel.slice(LITELLM_PROXY_PREFIX.length); - if (!openhandsVerifiedModels.includes(modelName)) return rawModel; - return `openhands/${modelName}`; -} diff --git a/src/utils/openhands-llm.ts b/src/utils/openhands-llm.ts deleted file mode 100644 index 6431841c7..000000000 --- a/src/utils/openhands-llm.ts +++ /dev/null @@ -1,22 +0,0 @@ -export const OPENHANDS_LLM_PROXY_BASE_URL = - "https://llm-proxy.app.all-hands.dev/"; - -// Accepted spellings of the All-Hands LiteLLM proxy base URL, normalized -// without a trailing slash. The agent-server stores curated OpenHands models -// against one of these once its validator rewrites `openhands/*` to -// `litellm_proxy/*`. -const OPENHANDS_LLM_PROXY_BASE_URLS = new Set([ - "https://llm-proxy.app.all-hands.dev", - "https://llm-proxy.app.all-hands.dev/v1", -]); - -/** - * True when `baseUrl` points at the All-Hands LiteLLM proxy, ignoring any - * trailing slash. - */ -export function isOpenHandsProxyBaseUrl(baseUrl: unknown): baseUrl is string { - return ( - typeof baseUrl === "string" && - OPENHANDS_LLM_PROXY_BASE_URLS.has(baseUrl.trim().replace(/\/+$/, "")) - ); -} 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 e18e7fe20..78a4929e5 100644 --- a/tests/e2e/mock-llm/mock-llm-profile-management.spec.ts +++ b/tests/e2e/mock-llm/mock-llm-profile-management.spec.ts @@ -16,10 +16,18 @@ * user selected, not the first alphabetical match. The fix * 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. */ import { test, expect } from "@playwright/test"; import { + BACKEND_URL, + SESSION_API_KEY, seedLocalStorage, routeSessionApiKey, dismissAnalyticsModal, @@ -39,6 +47,28 @@ import { const MOCK_MODEL = "openai/mock-test-model"; +/** + * Read-only helper: fetch a profile's persisted config via the API. + * Used to verify that UI-driven saves persisted the expected values. + */ +async function getProfileConfig( + request: import("@playwright/test").APIRequestContext, + name: string, +): Promise> { + const resp = await request.get( + `${BACKEND_URL}/api/profiles/${encodeURIComponent(name)}`, + { + headers: { + "X-Session-API-Key": SESSION_API_KEY, + "X-Expose-Secrets": "encrypted", + }, + }, + ); + expect(resp.ok(), `GET /api/profiles/${name}: ${resp.status()}`).toBe(true); + const data = await resp.json(); + return (data.config ?? {}) as Record; +} + test.describe.configure({ mode: "serial" }); // ═══════════════════════════════════════════════════════════════════════ @@ -328,3 +358,123 @@ test.describe("same-model profile identity", () => { }); }); }); + +// ═══════════════════════════════════════════════════════════════════════ +// Test 3 — OpenHands provider base_url normalization +// ═══════════════════════════════════════════════════════════════════════ + +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. + 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/"; + + test.beforeEach(async ({ page }) => { + await seedLocalStorage(page); + }); + + test.afterAll(async ({ browser }) => { + const page = await browser.newPage(); + try { + await seedLocalStorage(page); + await routeSessionApiKey(page); + await page.goto("/settings/llm", { waitUntil: "domcontentloaded" }); + await dismissAnalyticsModal(page); + await waitForTestId(page, "add-llm-profile"); + await deleteProfileIfExists(page, OPENHANDS_PROFILE); + } catch { + // best-effort + } finally { + await page.close(); + } + }); + + test("re-saving an OpenHands profile from Basic view drops proxy 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. ── + await routeSessionApiKey(page); + await page.goto("/settings/llm", { waitUntil: "domcontentloaded" }); + await dismissAnalyticsModal(page); + await waitForTestId(page, "add-llm-profile"); + + await deleteProfileIfExists(page, OPENHANDS_PROFILE); + await createProfileViaUI(page, { + profileName: OPENHANDS_PROFILE, + model: OPENHANDS_MODEL, + baseUrl: OPENHANDS_PROXY_BASE_URL, + }); + + await test.step("open profile in edit mode", async () => { + const profileRows = page.getByTestId("profile-row"); + const rowCount = await profileRows.count(); + let targetRow: ReturnType | null = null; + + for (let i = 0; i < rowCount; i++) { + const row = profileRows.nth(i); + const text = await row.textContent(); + if (text?.includes(OPENHANDS_PROFILE)) { + targetRow = row; + break; + } + } + expect( + targetRow, + `Could not find profile row for "${OPENHANDS_PROFILE}"`, + ).not.toBeNull(); + + await targetRow!.getByTestId("profile-menu-trigger").click(); + await waitForTestId(page, "profile-actions-menu"); + await page.getByTestId("profile-edit").click(); + + await expect(page.getByTestId("profile-name-input")).toHaveValue( + OPENHANDS_PROFILE, + { timeout: 10_000 }, + ); + }); + + await test.step("switch to Basic view and save", async () => { + // Click the Basic toggle explicitly so the save path exercises the view + // that hides base_url and therefore drops stale provider transport data. + const basicToggle = page.getByTestId("sdk-section-basic-toggle"); + if (await basicToggle.isVisible().catch(() => false)) { + await basicToggle.click(); + } + + const saveButton = page.getByTestId("save-profile-btn"); + await expect(saveButton).toBeEnabled({ timeout: 10_000 }); + await saveButton.click(); + + 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 () => { + 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(); + expect(EXPECTED_OPENHANDS_MODELS).toContain(config.model); + }); + + await test.step("profile survives page reload", async () => { + await page.reload({ waitUntil: "domcontentloaded" }); + await waitForTestId(page, "add-llm-profile"); + + // Re-read via API to confirm persistence is durable + const config = await getProfileConfig(request, OPENHANDS_PROFILE); + expect(config.base_url ?? null).toBeNull(); + expect(EXPECTED_OPENHANDS_MODELS).toContain(config.model); + }); + }); +});