diff --git a/src/config/options.ts b/src/config/options.ts index 413c5f1..e2bdbda 100644 --- a/src/config/options.ts +++ b/src/config/options.ts @@ -164,21 +164,51 @@ export function isBuiltinProvider(providerId: string): boolean { /** * Encode a provider+model pair into a configOption `value` string. * - * Builtin providers encode as the bare modelId (legacy form, keeps the dropdown - * clean for the common case). Third-party providers encode as - * `providerId\modelId` — `\` is unambiguous because providerIds (UUIDs) and - * modelIds (`/`-separated) never contain it. + * Always `providerId\modelId` — builtins included. A collision-only prefix + * would advertise different id shapes depending on how many coding plans the + * user has enabled. `\` is unambiguous because providerIds (UUIDs / builtin: + * slugs) and modelIds (`/`-separated) never contain it. + * + * Inbound, `parseModelValue` still accepts a legacy bare modelId. */ export function formatModelValue(providerId: string, modelId: string): string { - if (isBuiltinProvider(providerId)) return modelId; return `${providerId}\\${modelId}`; } +function collidingModelIds(models: ModelRef[]): Set { + const counts = new Map(); + for (const model of models) { + counts.set(model.modelId, (counts.get(model.modelId) ?? 0) + 1); + } + const colliding = new Set(); + for (const [modelId, count] of counts) { + if (count > 1) colliding.add(modelId); + } + return colliding; +} + +function buildModelSelectOptions(models: ModelRef[]): Array<{ value: string; name: string }> { + const collidingIds = collidingModelIds(models); + const options: Array<{ value: string; name: string }> = []; + const seen = new Set(); + for (const model of models) { + const value = formatModelValue(model.providerId, model.modelId); + if (seen.has(value)) continue; + seen.add(value); + const qualify = collidingIds.has(model.modelId) || !isBuiltinProvider(model.providerId); + options.push({ + value, + name: qualify ? `${model.providerName} › ${model.modelId}` : model.modelId, + }); + } + return options; +} + /** * Parse a configOption `value` back into { providerId, modelId }. * - * A value without `\` is a builtin modelId (legacy form) → resolve to the first - * enabled builtin provider. A value with `\` is a third-party provider+model. + * A value without `\` is a legacy bare modelId → resolve to the first enabled + * builtin provider. A value with `\` is the current provider+model encoding. */ export function parseModelValue(value: string): { providerId: string; modelId: string } { const idx = value.indexOf("\\"); @@ -336,18 +366,14 @@ export async function buildConfigOptions( // currentValue encodes provider+model so the switch handler can locate the // right provider (and its apiKey). Fall back to the first enabled provider // when settings omits providerId (legacy sessions). - const currentModel = formatModelValue( - currentProviderId || loadAllModels()[0]?.providerId || DEFAULT_PROVIDER_ID, - currentModelId, - ); + const allModels = loadAllModels(); + const currentProvider = currentProviderId || allModels[0]?.providerId || DEFAULT_PROVIDER_ID; + const currentModel = formatModelValue(currentProvider, currentModelId); - // Model options: config.json enabled providers are authoritative. Builtin - // models show as the bare modelId (clean dropdown for the common case); - // third-party models prefix the provider name so they're distinguishable. - let modelOptions = loadAllModels().map((m) => ({ - value: formatModelValue(m.providerId, m.modelId), - name: isBuiltinProvider(m.providerId) ? m.modelId : `${m.providerName} › ${m.modelId}`, - })); + // Model options: config.json enabled providers are authoritative. Values + // are always providerId\modelId. Builtin labels stay the bare modelId + // unless two providers ship the same id; third-party labels always qualify. + let modelOptions = buildModelSelectOptions(allModels); if (!modelOptions.some((o) => o.value === currentModel)) { // The current model isn't from an enabled provider (e.g. the session was // created with a now-disabled provider). Append it so the dropdown still diff --git a/tests/model-select-options.test.ts b/tests/model-select-options.test.ts new file mode 100644 index 0000000..cd11574 --- /dev/null +++ b/tests/model-select-options.test.ts @@ -0,0 +1,120 @@ +/** + * ACP model option values always encode as providerId\modelId, including + * builtins. Bare GLM-5.3 collided when two coding plans shipped the same id + * and crashed clients that key on uniqueness (Paseo Command Center). A + * collision-only prefix would have advertised different id shapes depending + * on the user's enabled-provider set. + * + * parseModelValue still accepts a legacy bare modelId (first enabled builtin). + * Dropdown labels stay the bare modelId for a single builtin; colliding + * modelIds are qualified with the provider name. + */ + +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { ZCODE_CREDS_PATH } from "../src/utils.js"; +import { ZcodeAcpServer } from "../src/server.js"; + +function codingPlan(name: string, baseURL: string) { + return { + name, + kind: "anthropic", + enabled: true, + options: { apiKey: "plan-token", apiKeyRequired: true, baseURL }, + models: { + "GLM-5.3": { limit: { context: 200000 } }, + "GLM-5.3-Flash": { limit: { context: 200000 } }, + }, + }; +} + +function collidingPlansConfig() { + return { + provider: { + "builtin:zai-coding-plan": codingPlan("Z.ai - Coding Plan", "https://api.z.ai/api/anthropic"), + "builtin:zai-start-plan": codingPlan( + "Z.ai - Start Plan", + "https://zcode.z.ai/api/v1/zcode-plan/anthropic", + ), + }, + }; +} + +let fakeConfig: unknown = collidingPlansConfig(); + +vi.mock("node:fs", async () => { + const actual = await vi.importActual("node:fs"); + return { + ...actual, + readFileSync: (p: string) => { + if (p === ZCODE_CREDS_PATH) return JSON.stringify(fakeConfig); + return actual.readFileSync(p); + }, + }; +}); + +const { buildConfigOptions, formatModelValue, loadAllModels, parseModelValue } = + await import("../src/config/options.js"); + +afterEach(() => { + fakeConfig = collidingPlansConfig(); +}); + +describe("model configOptions uniqueness", () => { + it("does not advertise duplicate values when two builtins share GLM-5.3", async () => { + const options = await buildConfigOptions(new ZcodeAcpServer(), null); + const model = options.find((option) => option.id === "model"); + const values = model?.options.map((option) => option.value) ?? []; + + expect(values).toEqual([...new Set(values)]); + expect(values).toHaveLength(4); + expect(model?.currentValue).toBeDefined(); + expect(values).toContain(model?.currentValue); + }); + + it("keeps both colliding coding plans selectable", async () => { + const options = await buildConfigOptions(new ZcodeAcpServer(), null); + const model = options.find((option) => option.id === "model"); + const parsed = (model?.options ?? []).map((option) => parseModelValue(option.value)); + + expect(parsed).toEqual([ + { providerId: "builtin:zai-coding-plan", modelId: "GLM-5.3" }, + { providerId: "builtin:zai-coding-plan", modelId: "GLM-5.3-Flash" }, + { providerId: "builtin:zai-start-plan", modelId: "GLM-5.3" }, + { providerId: "builtin:zai-start-plan", modelId: "GLM-5.3-Flash" }, + ]); + expect(model?.options.map((option) => option.name)).toEqual([ + "Z.ai - Coding Plan › GLM-5.3", + "Z.ai - Coding Plan › GLM-5.3-Flash", + "Z.ai - Start Plan › GLM-5.3", + "Z.ai - Start Plan › GLM-5.3-Flash", + ]); + }); + + it("encodes a single builtin as providerId\\modelId too", async () => { + fakeConfig = { + provider: { + "builtin:zai-coding-plan": codingPlan( + "Z.ai - Coding Plan", + "https://api.z.ai/api/anthropic", + ), + }, + }; + + const options = await buildConfigOptions(new ZcodeAcpServer(), null); + const model = options.find((option) => option.id === "model"); + const values = model?.options.map((option) => option.value) ?? []; + + expect(values).toEqual([ + "builtin:zai-coding-plan\\GLM-5.3", + "builtin:zai-coding-plan\\GLM-5.3-Flash", + ]); + expect(model?.options.map((option) => option.name)).toEqual(["GLM-5.3", "GLM-5.3-Flash"]); + expect(model?.currentValue).toBe(formatModelValue("builtin:zai-coding-plan", "GLM-5.3")); + expect(parseModelValue("GLM-5.3")).toEqual({ + providerId: "builtin:zai-coding-plan", + modelId: "GLM-5.3", + }); + expect(loadAllModels()).toHaveLength(2); + }); +}); diff --git a/tests/runtime-model.test.ts b/tests/runtime-model.test.ts index 84901fe..609e788 100644 --- a/tests/runtime-model.test.ts +++ b/tests/runtime-model.test.ts @@ -10,8 +10,8 @@ * buildRuntimeModel() inlines apiKey as {source:"inline",value} * for third-party providers (the backend resolves model-call auth from the * overlay itself; omitting it yields HTTP 401) but omits it for builtins. - * Builtin models encode as bare modelIds, and third-party models carry their - * providerId prefix. + * Every model encodes as providerId\modelId. A legacy bare modelId still + * parses as the first enabled builtin. */ import { describe, expect, it, vi } from "vitest"; @@ -205,10 +205,9 @@ describe("modelContextWindow", () => { }); describe("parseModelValue / formatModelValue", () => { - it("builtin providers encode as bare modelId (no prefix)", () => { - // The common case stays clean — builtin models show just the modelId. + it("builtin providers encode as providerId\\modelId", () => { const value = formatModelValue("builtin:primary", "model-a"); - expect(value).toBe("model-a"); + expect(value).toBe("builtin:primary\\model-a"); expect(parseModelValue(value)).toEqual({ providerId: "builtin:primary", modelId: "model-a",