mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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`
This commit is contained in:
@@ -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<AgentCreateFormProps> = ({
|
||||
|
||||
const byConfigID = new Map<string, string>();
|
||||
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;
|
||||
}
|
||||
|
||||
@@ -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<string, string>();
|
||||
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;
|
||||
}
|
||||
|
||||
@@ -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<string, string>();
|
||||
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;
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<Parameters<typeof getModelOptionsFromCatalog>[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<Parameters<typeof getModelOptionsFromCatalog>[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,
|
||||
},
|
||||
]);
|
||||
});
|
||||
});
|
||||
@@ -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<TypesGen.ChatModel, "provider" | "model">
|
||||
| Pick<TypesGen.ChatModelConfig, "provider" | "model">
|
||||
| RuntimeModelRef;
|
||||
|
||||
type CatalogModelLike =
|
||||
| TypesGen.ChatModel
|
||||
| (RuntimeModelRef & {
|
||||
readonly id?: unknown;
|
||||
readonly display_name?: unknown;
|
||||
});
|
||||
|
||||
type CatalogProviderLike = Omit<TypesGen.ChatModelProvider, "models"> & {
|
||||
readonly models?: readonly CatalogModelLike[];
|
||||
};
|
||||
|
||||
type ModelCatalogLike = {
|
||||
readonly providers?: readonly CatalogProviderLike[];
|
||||
};
|
||||
|
||||
type ChatModelConfigLike =
|
||||
| Pick<TypesGen.ChatModelConfig, "provider" | "model" | "context_limit">
|
||||
| (RuntimeModelRef & Pick<TypesGen.ChatModelConfig, "context_limit">);
|
||||
|
||||
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<string, ModelSelectorOption>();
|
||||
|
||||
@@ -47,18 +85,24 @@ export const getModelOptionsFromCatalog = (
|
||||
const contextLimitByKey = new Map<string, number>();
|
||||
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),
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user