mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-12 23:01:41 +00:00
fix(ui): stop injecting empty vector_store_ids, guardrails, and $0 cost into model litellm_params on save
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) <noreply@anthropic.com>
This commit is contained in:
Darien Kindlund
parent
cec3e9e7d4
commit
0c8fd32fc7
2 changed files with 444 additions and 12 deletions
|
|
@ -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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView {...DEFAULT_ADMIN_PROPS} onModelUpdate={mockOnModelUpdate} />, { 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(<ModelInfoView modelId="test-model-id" onClose={vi.fn()} modelData={modelWithNoCost} isLoading={false} userRole="proxy_admin" userID="admin-user" />);
|
||||
|
||||
// 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(<ModelInfoView modelId="test-model-id" onClose={vi.fn()} modelData={modelWithCost} isLoading={false} userRole="proxy_admin" userID="admin-user" />);
|
||||
|
||||
// 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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 || "",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue