From 0c8fd32fc77e614dd0beb0973255241d0e7f0149 Mon Sep 17 00:00:00 2001 From: Darien Kindlund Date: Wed, 18 Mar 2026 00:48:19 -0700 Subject: [PATCH] fix(ui): stop injecting empty vector_store_ids, guardrails, and $0 cost into model litellm_params on save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Admin UI's model edit form unconditionally includes several fields in the update payload even when they are empty/untouched, overwriting the existing model config: - vector_store_ids: [] injected, causing Anthropic 400 errors - guardrails: [] injected, clearing configured guardrails - input_cost_per_token: 0.0 injected, overriding built-in pricing with $0 - output_cost_per_token: 0.0 injected, same issue The $0 cost injection is particularly dangerous: it silently disables spend tracking for the affected model, causing budget enforcement to be bypassed and cloud provider billing to diverge from LiteLLM's reported spend. The $0 custom cost is written to both litellm_params AND model_info, and both must be cleared to restore correct pricing. Fix: apply the same guard pattern used for litellm_credential_name — only include these fields when explicitly set, otherwise delete them. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../src/components/model_info_view.test.tsx | 424 +++++++++++++++++- .../src/components/model_info_view.tsx | 32 +- 2 files changed, 444 insertions(+), 12 deletions(-) diff --git a/ui/litellm-dashboard/src/components/model_info_view.test.tsx b/ui/litellm-dashboard/src/components/model_info_view.test.tsx index b2cbbda34aa..0281fcf64db 100644 --- a/ui/litellm-dashboard/src/components/model_info_view.test.tsx +++ b/ui/litellm-dashboard/src/components/model_info_view.test.tsx @@ -1,5 +1,5 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; -import { render, screen, waitFor } from "@testing-library/react"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import React, { ReactNode } from "react"; import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -20,6 +20,25 @@ vi.mock("./molecules/notifications_manager", () => ({ }, })); +// Mock VectorStoreSelector as a controlled component that integrates with Ant Design Form.Item. +// Form.Item passes value/onChange props to its child; this mock renders a hidden input so the +// form value is properly tracked and can be manipulated in tests. +vi.mock("./vector_store_management/VectorStoreSelector", () => ({ + default: ({ value, onChange, ...rest }: any) => + React.createElement("input", { + "data-testid": "vector-store-selector", + type: "hidden", + value: JSON.stringify(value ?? []), + onChange: (e: any) => { + try { + onChange?.(JSON.parse(e.target.value)); + } catch { + onChange?.([]); + } + }, + }), +})); + vi.mock("./networking", () => ({ modelInfoV1Call: vi.fn(), credentialGetCall: vi.fn(), @@ -636,4 +655,407 @@ describe("ModelInfoView", () => { expect(screen.getByText(/Created By/)).toBeInTheDocument(); }); }); + + it("should not inject empty vector_store_ids into litellm_params on save", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model has NO vector_store_ids in litellm_params + const modelWithoutVectorStores = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + // no vector_store_ids + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithoutVectorStores] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + expect(updatePayload.litellm_params.vector_store_ids).toBeUndefined(); + }); + + it("should preserve non-empty vector_store_ids in litellm_params on save", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model HAS vector_store_ids configured + const modelWithVectorStores = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + vector_store_ids: ["vs_abc123", "vs_def456"], + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithVectorStores] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + expect(updatePayload.litellm_params.vector_store_ids).toEqual(["vs_abc123", "vs_def456"]); + }); + + it("should not inject empty vector_store_ids when model previously had empty array", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model has vector_store_ids set to empty array (the bug condition) + const modelWithEmptyVectorStores = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + vector_store_ids: [], + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithEmptyVectorStores] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + expect(updatePayload.litellm_params.vector_store_ids).toBeUndefined(); + }); + + it("should remove vector_store_ids when user clears all vector stores from a model that had them", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model has vector_store_ids configured — simulates a model where user will remove them + const modelWithVectorStores = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + vector_store_ids: ["vs_abc123"], + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithVectorStores] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + // Simulate user clearing all vector stores by setting the form field to [] + const vectorStoreInput = screen.getByTestId("vector-store-selector"); + // Simulate user clearing all vector stores via the selector + fireEvent.change(vectorStoreInput, { target: { value: "[]" } }); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + // vector_store_ids should be fully removed via the else { delete } branch, + // not re-injected via parsedExtraParams from the litellm_extra_params textarea + expect(updatePayload.litellm_params.vector_store_ids).toBeUndefined(); + }); + + // --- guardrails tests (same pattern as vector_store_ids) --- + + it("should not inject empty guardrails into litellm_params on save", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model has NO guardrails in litellm_params + const modelWithoutGuardrails = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + // no guardrails + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithoutGuardrails] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + expect(updatePayload.litellm_params.guardrails).toBeUndefined(); + }); + + it("should preserve non-empty guardrails in litellm_params on save", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model HAS guardrails configured + const modelWithGuardrails = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + guardrails: ["content_filter", "toxicity_filter"], + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithGuardrails] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + expect(updatePayload.litellm_params.guardrails).toEqual(["content_filter", "toxicity_filter"]); + }); + + it("should not inject empty guardrails when model previously had empty array", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model has guardrails set to empty array (same bug pattern as vector_store_ids) + const modelWithEmptyGuardrails = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + guardrails: [], + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithEmptyGuardrails] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + expect(updatePayload.litellm_params.guardrails).toBeUndefined(); + }); + + it("should remove guardrails when model previously had them but they are cleared", async () => { + const user = userEvent.setup(); + const mockOnModelUpdate = vi.fn(); + + // Model has guardrails configured — the else { delete } branch must scrub + // the value that leaks through parsedExtraParams when the form field is empty + const modelWithGuardrails = { + ...defaultModelData, + litellm_params: { + ...defaultModelData.litellm_params, + guardrails: ["content_filter"], + }, + }; + + mockUseModelsInfo.mockReturnValue({ + data: { data: [modelWithGuardrails] }, + isLoading: false, + error: null, + }); + + render(, { wrapper }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /edit settings/i })).toBeInTheDocument(); + }); + + await user.click(screen.getByRole("button", { name: /edit settings/i })); + + await waitFor(() => { + expect(screen.getByRole("button", { name: /save changes/i })).toBeInTheDocument(); + }); + + // Clear the guardrails select by removing all selected tags + // The Ant Design Select with mode="tags" renders selected items as removable tags + const removeButtons = screen.queryAllByLabelText("close"); + for (const btn of removeButtons) { + await user.click(btn); + } + + await user.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + // guardrails should be fully removed via the else { delete } branch + expect(updatePayload.litellm_params.guardrails).toBeUndefined(); + }); + + it("should not inject input_cost_per_token: 0 when cost fields are empty on save", async () => { + const mockModelPatchUpdateCall = vi.spyOn(networking, "modelPatchUpdateCall").mockResolvedValue({}); + + // Model has NO custom cost set + const modelWithNoCost = { + ...mockModelData, + litellm_params: { + ...mockModelData.litellm_params, + // no input_cost_per_token or output_cost_per_token + }, + }; + + renderWithQueryClient(); + + // Enter edit mode and save without touching cost fields + await userEvent.click(screen.getByRole("button", { name: /edit settings/i })); + await userEvent.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + // Cost fields should NOT be present (not injected as 0) + expect(updatePayload.litellm_params.input_cost_per_token).toBeUndefined(); + expect(updatePayload.litellm_params.output_cost_per_token).toBeUndefined(); + }); + + it("should preserve explicitly set cost values on save", async () => { + const mockModelPatchUpdateCall = vi.spyOn(networking, "modelPatchUpdateCall").mockResolvedValue({}); + + // Model HAS custom cost set + const modelWithCost = { + ...mockModelData, + litellm_params: { + ...mockModelData.litellm_params, + input_cost_per_token: 0.000003, + output_cost_per_token: 0.000015, + }, + }; + + renderWithQueryClient(); + + // Enter edit mode and save without changing cost fields + await userEvent.click(screen.getByRole("button", { name: /edit settings/i })); + await userEvent.click(screen.getByRole("button", { name: /save changes/i })); + + await waitFor(() => { + expect(mockModelPatchUpdateCall).toHaveBeenCalled(); + }); + + const updatePayload = mockModelPatchUpdateCall.mock.calls[0][1]; + // Cost fields should be preserved with their original values + expect(updatePayload.litellm_params.input_cost_per_token).toBe(0.000003); + expect(updatePayload.litellm_params.output_cost_per_token).toBe(0.000015); + }); }); diff --git a/ui/litellm-dashboard/src/components/model_info_view.tsx b/ui/litellm-dashboard/src/components/model_info_view.tsx index 9e846c83ff3..bf7525d118b 100644 --- a/ui/litellm-dashboard/src/components/model_info_view.tsx +++ b/ui/litellm-dashboard/src/components/model_info_view.tsx @@ -253,22 +253,32 @@ export default function ModelInfoView({ max_retries: values.max_retries, timeout: values.timeout, stream_timeout: values.stream_timeout, - input_cost_per_token: values.input_cost / 1_000_000, - output_cost_per_token: values.output_cost / 1_000_000, tags: values.tags, }; + if (values.input_cost != null && values.input_cost !== "") { + updatedLitellmParams.input_cost_per_token = values.input_cost / 1_000_000; + } else { + delete updatedLitellmParams.input_cost_per_token; + } + if (values.output_cost != null && values.output_cost !== "") { + updatedLitellmParams.output_cost_per_token = values.output_cost / 1_000_000; + } else { + delete updatedLitellmParams.output_cost_per_token; + } if (values.litellm_credential_name) { updatedLitellmParams.litellm_credential_name = values.litellm_credential_name; } else { delete updatedLitellmParams.litellm_credential_name; } - if (values.guardrails) { + if (Array.isArray(values.guardrails) && values.guardrails.length > 0) { updatedLitellmParams.guardrails = values.guardrails; + } else { + delete updatedLitellmParams.guardrails; } - if (values.vector_store_ids !== undefined) { - updatedLitellmParams.vector_store_ids = Array.isArray(values.vector_store_ids) - ? values.vector_store_ids - : []; + if (Array.isArray(values.vector_store_ids) && values.vector_store_ids.length > 0) { + updatedLitellmParams.vector_store_ids = values.vector_store_ids; + } else { + delete updatedLitellmParams.vector_store_ids; } // Handle cache control settings @@ -628,12 +638,12 @@ export default function ModelInfoView({ model_access_group: Array.isArray(localModelData.model_info?.access_groups) ? localModelData.model_info.access_groups : [], - guardrails: Array.isArray(localModelData.litellm_params?.guardrails) + guardrails: Array.isArray(localModelData.litellm_params?.guardrails) && localModelData.litellm_params.guardrails.length > 0 ? localModelData.litellm_params.guardrails - : [], - vector_store_ids: Array.isArray(localModelData.litellm_params?.vector_store_ids) + : undefined, + vector_store_ids: Array.isArray(localModelData.litellm_params?.vector_store_ids) && localModelData.litellm_params.vector_store_ids.length > 0 ? localModelData.litellm_params.vector_store_ids - : [], + : undefined, tags: Array.isArray(localModelData.litellm_params?.tags) ? localModelData.litellm_params.tags : [], health_check_model: isWildcardModel ? localModelData.model_info?.health_check_model : null, litellm_credential_name: localModelData.litellm_params?.litellm_credential_name || "",