github/OpenHands
1
0
Fork 0
mirror of https://github.com/OpenHands/OpenHands.git synced 2026-10-07 13:18:19 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix: preserve hidden LLM base URL on basic saves (#1347)

Browse Source 
Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Engel Nyst
2026-06-13 05:05:25 +02:00
committed by GitHub
co-authored by openhands
parent af9c653f3c
commit 2888a5ca4e
5 changed files with 143 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
__tests__/components/settings/llm-profiles/llm-settings-local-view.test.tsx
+74 -17
View File
@@ -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 (
<div data-testid="mock-llm-settings-screen">
@@ -76,6 +91,13 @@ vi.mock("#/routes/llm-settings", async () => {
>
All
</button>
{view === "basic" ? (
<input
data-testid="mock-basic-model-input"
value={model}
onChange={(event) => setModel(event.currentTarget.value)}
/>
) : null}
{view === "all" ? (
<input
data-testid="sdk-settings-llm.temperature"
@@ -593,10 +615,9 @@ describe("LlmSettingsLocalView", () => {
});
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(<LlmSettingsLocalView />);
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");
});
});
__tests__/routes/llm-settings.test.tsx
+37
View File
@@ -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<string, unknown>;
const llmPayload = (
payload.agent_settings_diff as Record<string, unknown>
).llm as Record<string, unknown>;
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.
src/components/features/settings/llm-profiles/llm-settings-local-view.tsx
+8 -3
View File
@@ -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<string, unknown> = { ...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;
}
src/routes/llm-settings.tsx
+1 -1
View File
@@ -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");
}
}
tests/e2e/mock-llm/mock-llm-profile-management.spec.ts
+23 -26
View File
@@ -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);
});
});