From 5b9a9e5bdfd81a58fe5239dbde5c79f9e8fd7bb6 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 19 Mar 2026 12:27:24 +0100 Subject: [PATCH] fix(site): guard malformed agent model refs (#23252) ## Summary - guard Agent pages against malformed model provider/model values before trimming - reuse a shared model-ref normalizer across Agent detail, sidebar, list, and create flows - add regression coverage for malformed catalog and config entries ## Validation - `cd site && pnpm exec vitest run src/pages/AgentsPage/modelOptions.test.ts src/pages/AgentsPage/AgentDetail.test.ts` - `cd site && pnpm lint:types` --- site/src/pages/AgentsPage/AgentCreateForm.tsx | 4 +- site/src/pages/AgentsPage/AgentDetail.tsx | 4 +- site/src/pages/AgentsPage/AgentsPage.tsx | 8 +- site/src/pages/AgentsPage/AgentsSidebar.tsx | 12 ++- .../src/pages/AgentsPage/modelOptions.test.ts | 88 +++++++++++++++++ site/src/pages/AgentsPage/modelOptions.ts | 95 +++++++++++++------ 6 files changed, 172 insertions(+), 39 deletions(-) create mode 100644 site/src/pages/AgentsPage/modelOptions.test.ts diff --git a/site/src/pages/AgentsPage/AgentCreateForm.tsx b/site/src/pages/AgentsPage/AgentCreateForm.tsx index aa11d7d773..a8be2c980c 100644 --- a/site/src/pages/AgentsPage/AgentCreateForm.tsx +++ b/site/src/pages/AgentsPage/AgentCreateForm.tsx @@ -35,6 +35,7 @@ import { AgentChatInput } from "./AgentChatInput"; import { getModelCatalogStatusMessage, getModelSelectorPlaceholder, + getNormalizedModelRef, hasConfiguredModelsInCatalog, } from "./modelOptions"; import { formatUsageLimitMessage, isUsageLimitData } from "./usageLimitMessage"; @@ -158,8 +159,7 @@ export const AgentCreateForm: FC = ({ const byConfigID = new Map(); for (const config of modelConfigs) { - const provider = config.provider.trim().toLowerCase(); - const model = config.model.trim(); + const { provider, model } = getNormalizedModelRef(config); if (!provider || !model) { continue; } diff --git a/site/src/pages/AgentsPage/AgentDetail.tsx b/site/src/pages/AgentsPage/AgentDetail.tsx index 226914eb68..8c0ce3fba4 100644 --- a/site/src/pages/AgentsPage/AgentDetail.tsx +++ b/site/src/pages/AgentsPage/AgentDetail.tsx @@ -57,6 +57,7 @@ import { getModelCatalogStatusMessage, getModelOptionsFromCatalog, getModelSelectorPlaceholder, + getNormalizedModelRef, hasConfiguredModelsInCatalog, } from "./modelOptions"; import { parsePullRequestUrl } from "./pullRequest"; @@ -395,8 +396,7 @@ const AgentDetail: FC = () => { const modelConfigIDByModelID = useMemo(() => { const byModelID = new Map(); for (const config of chatModelConfigsQuery.data ?? []) { - const provider = config.provider.trim().toLowerCase(); - const model = config.model.trim(); + const { provider, model } = getNormalizedModelRef(config); if (!provider || !model) { continue; } diff --git a/site/src/pages/AgentsPage/AgentsPage.tsx b/site/src/pages/AgentsPage/AgentsPage.tsx index 682ed6045c..5e3d297b2d 100644 --- a/site/src/pages/AgentsPage/AgentsPage.tsx +++ b/site/src/pages/AgentsPage/AgentsPage.tsx @@ -41,7 +41,10 @@ import { import { maybePlayChime } from "./AgentDetail/useAgentChime"; import type { AgentsOutletContext } from "./AgentsPageView"; import { AgentsPageView } from "./AgentsPageView"; -import { getModelOptionsFromCatalog } from "./modelOptions"; +import { + getModelOptionsFromCatalog, + getNormalizedModelRef, +} from "./modelOptions"; import type { ChatDetailError } from "./usageLimitMessage"; import { useAgentsPageKeybindings } from "./useAgentsPageKeybindings"; import { useAgentsPWA } from "./useAgentsPWA"; @@ -187,8 +190,7 @@ const AgentsPage: FC = () => { const modelConfigIDByModelID = useMemo(() => { const byModelID = new Map(); for (const config of chatModelConfigsQuery.data ?? []) { - const provider = config.provider.trim().toLowerCase(); - const model = config.model.trim(); + const { provider, model } = getNormalizedModelRef(config); if (!provider || !model) { continue; } diff --git a/site/src/pages/AgentsPage/AgentsSidebar.tsx b/site/src/pages/AgentsPage/AgentsSidebar.tsx index 3616e81b5e..b2c582862d 100644 --- a/site/src/pages/AgentsPage/AgentsSidebar.tsx +++ b/site/src/pages/AgentsPage/AgentsSidebar.tsx @@ -7,6 +7,7 @@ import type { import { ErrorAlert } from "components/Alert/ErrorAlert"; import { Avatar } from "components/Avatar/Avatar"; import type { ModelSelectorOption } from "components/ai-elements"; +import { asString } from "components/ai-elements/runtimeTypeUtils"; import { Button } from "components/Button/Button"; import { DropdownMenu, @@ -69,6 +70,7 @@ import { import { Link, NavLink, useLocation, useParams } from "react-router"; import { cn } from "utils/cn"; import { shortRelativeTime } from "utils/time"; +import { getNormalizedModelRef } from "./modelOptions"; import { getTimeGroup, TIME_GROUPS } from "./timeGroups"; type SidebarView = @@ -187,10 +189,10 @@ const getModelDisplayName = ( if (!modelConfig) { return "Default model"; } - const provider = modelConfig.provider.trim().toLowerCase(); - const model = modelConfig.model.trim(); + const { provider, model } = getNormalizedModelRef(modelConfig); + const displayName = asString(modelConfig.display_name).trim(); if (!provider || !model) { - return modelConfig.display_name.trim() || "Default model"; + return displayName || "Default model"; } // Try to find a matching option with a display name. @@ -203,8 +205,8 @@ const getModelDisplayName = ( return match.displayName; } - if (modelConfig.display_name.trim()) { - return modelConfig.display_name.trim(); + if (displayName) { + return displayName; } return model; diff --git a/site/src/pages/AgentsPage/modelOptions.test.ts b/site/src/pages/AgentsPage/modelOptions.test.ts new file mode 100644 index 0000000000..78f7ecb190 --- /dev/null +++ b/site/src/pages/AgentsPage/modelOptions.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it } from "vitest"; +import { + getModelOptionsFromCatalog, + getNormalizedModelRef, +} from "./modelOptions"; + +describe("getNormalizedModelRef", () => { + it("returns empty strings for malformed values", () => { + expect(getNormalizedModelRef({ provider: undefined, model: null })).toEqual( + { provider: "", model: "" }, + ); + }); + + it("trims and normalizes provider values", () => { + expect( + getNormalizedModelRef({ provider: " OpenAI ", model: " gpt-4o " }), + ).toEqual({ provider: "openai", model: "gpt-4o" }); + }); +}); + +describe("getModelOptionsFromCatalog", () => { + it("skips malformed configs and catalog models without crashing", () => { + const catalog = { + providers: [ + { + provider: "openai", + available: true, + models: [ + { + id: " valid-model ", + provider: " OpenAI ", + model: " gpt-4o ", + display_name: " GPT‑4o ", + }, + { + id: "broken-model", + provider: undefined, + model: " gpt-4.1 ", + display_name: "Broken", + }, + { + id: " fallback-model ", + provider: " OpenAI ", + model: " zz-model ", + display_name: undefined, + }, + ], + }, + ], + } satisfies NonNullable[0]>; + + const configs = [ + { + provider: undefined, + model: " gpt-4o ", + context_limit: 123, + }, + { + provider: " openai ", + model: " gpt-4o ", + context_limit: 456, + }, + { + provider: " openai ", + model: " zz-model ", + context_limit: 789, + }, + ] satisfies NonNullable[1]>; + + expect(() => getModelOptionsFromCatalog(catalog, configs)).not.toThrow(); + expect(getModelOptionsFromCatalog(catalog, configs)).toEqual([ + { + id: "valid-model", + provider: "openai", + model: "gpt-4o", + displayName: "GPT‑4o", + contextLimit: 456, + }, + { + id: "fallback-model", + provider: "openai", + model: "zz-model", + displayName: "zz-model", + contextLimit: 789, + }, + ]); + }); +}); diff --git a/site/src/pages/AgentsPage/modelOptions.ts b/site/src/pages/AgentsPage/modelOptions.ts index 279da40381..b2ce754db9 100644 --- a/site/src/pages/AgentsPage/modelOptions.ts +++ b/site/src/pages/AgentsPage/modelOptions.ts @@ -1,44 +1,82 @@ import type * as TypesGen from "api/typesGenerated"; import type { ModelSelectorOption } from "components/ai-elements"; +import { asNumber, asString } from "components/ai-elements/runtimeTypeUtils"; -type CatalogProvider = TypesGen.ChatModelsResponse["providers"][number]; +type RuntimeModelRef = { + readonly provider?: unknown; + readonly model?: unknown; +}; + +type ModelRefLike = + | Pick + | Pick + | RuntimeModelRef; + +type CatalogModelLike = + | TypesGen.ChatModel + | (RuntimeModelRef & { + readonly id?: unknown; + readonly display_name?: unknown; + }); + +type CatalogProviderLike = Omit & { + readonly models?: readonly CatalogModelLike[]; +}; + +type ModelCatalogLike = { + readonly providers?: readonly CatalogProviderLike[]; +}; + +type ChatModelConfigLike = + | Pick + | (RuntimeModelRef & Pick); + +export const getNormalizedModelRef = ( + value: ModelRefLike, +): { readonly provider: string; readonly model: string } => { + const modelRef = value ?? {}; + return { + provider: asString(modelRef.provider).trim().toLowerCase(), + model: asString(modelRef.model).trim(), + }; +}; const getCatalogProviders = ( - catalog: TypesGen.ChatModelsResponse | null | undefined, -): readonly CatalogProvider[] => { + catalog: ModelCatalogLike | null | undefined, +): readonly CatalogProviderLike[] => { const providers = catalog?.providers; return Array.isArray(providers) ? providers : []; }; const getProviderModels = ( - provider: CatalogProvider, -): readonly CatalogProvider["models"][number][] => { + provider: CatalogProviderLike, +): readonly CatalogModelLike[] => { const models = provider.models; return Array.isArray(models) ? models : []; }; -const isProviderConfiguredInCatalog = (provider: CatalogProvider): boolean => { +const isProviderConfiguredInCatalog = ( + provider: CatalogProviderLike, +): boolean => { if (getProviderModels(provider).length > 0) { return true; } - if (provider.available) { + if (provider.available === true) { return true; } - return ( - Boolean(provider.unavailable_reason) && - provider.unavailable_reason !== "missing_api_key" - ); + const unavailableReason = asString(provider.unavailable_reason).trim(); + return unavailableReason !== "" && unavailableReason !== "missing_api_key"; }; export const hasConfiguredModelsInCatalog = ( - catalog: TypesGen.ChatModelsResponse | null | undefined, + catalog: ModelCatalogLike | null | undefined, ): boolean => { return getCatalogProviders(catalog).some(isProviderConfiguredInCatalog); }; export const getModelOptionsFromCatalog = ( - catalog: TypesGen.ChatModelsResponse | null | undefined, - configs?: readonly TypesGen.ChatModelConfig[], + catalog: ModelCatalogLike | null | undefined, + configs?: readonly ChatModelConfigLike[], ): readonly ModelSelectorOption[] => { const optionsByID = new Map(); @@ -47,18 +85,24 @@ export const getModelOptionsFromCatalog = ( const contextLimitByKey = new Map(); if (configs) { for (const config of configs) { - if (config.context_limit > 0) { - const key = `${config.provider.trim().toLowerCase()}:${config.model.trim()}`; - if (!contextLimitByKey.has(key)) { - contextLimitByKey.set(key, config.context_limit); - } + const contextLimit = asNumber(config.context_limit); + if (contextLimit === undefined || contextLimit <= 0) { + continue; + } + const { provider, model } = getNormalizedModelRef(config); + if (!provider || !model) { + continue; + } + const key = `${provider}:${model}`; + if (!contextLimitByKey.has(key)) { + contextLimitByKey.set(key, contextLimit); } } } for (const provider of getCatalogProviders(catalog)) { const models = getProviderModels(provider); - if (!provider.available || models.length === 0) { + if (provider.available !== true || models.length === 0) { continue; } for (const model of models) { @@ -66,9 +110,9 @@ export const getModelOptionsFromCatalog = ( continue; } - const modelID = model.id.trim(); - const modelProvider = model.provider.trim(); - const modelRef = model.model.trim(); + const modelID = asString(model.id).trim(); + const { provider: modelProvider, model: modelRef } = + getNormalizedModelRef(model); if (!modelID || !modelProvider || !modelRef) { continue; } @@ -82,10 +126,7 @@ export const getModelOptionsFromCatalog = ( id: modelID, provider: modelProvider, model: modelRef, - displayName: - (typeof model.display_name === "string" && - model.display_name.trim()) || - modelRef, + displayName: asString(model.display_name).trim() || modelRef, contextLimit: contextLimitByKey.get(configKey), }); }