From 1d5121eaef6768d3f5ad6e7d4480f95de253084a Mon Sep 17 00:00:00 2001 From: Alon Weiss Date: Thu, 10 Sep 2026 23:48:18 +0300 Subject: [PATCH 1/2] fix: disambiguate colliding builtin model ids in configOptions Two enabled builtin coding plans (Z.ai Coding Plan + Start Plan) both ship GLM-5.3. Bare modelId encoding advertised the same value twice, which crashes ACP clients that key on uniqueness (Paseo Command Center). Prefix colliding builtins the same way third-party models already are. --- src/config/options.ts | 68 +++++++++++++++--- tests/model-select-options.test.ts | 111 +++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 tests/model-select-options.test.ts diff --git a/src/config/options.ts b/src/config/options.ts index 413c5f1..cebf9b4 100644 --- a/src/config/options.ts +++ b/src/config/options.ts @@ -174,6 +174,56 @@ export function formatModelValue(providerId: string, modelId: string): string { return `${providerId}\\${modelId}`; } +function collidingBareValues(models: ModelRef[]): Set { + const counts = new Map(); + for (const model of models) { + const encoded = formatModelValue(model.providerId, model.modelId); + counts.set(encoded, (counts.get(encoded) ?? 0) + 1); + } + const colliding = new Set(); + for (const [encoded, count] of counts) { + if (count > 1) colliding.add(encoded); + } + return colliding; +} + +function encodeModelOptionValue(model: ModelRef, collidingBare: Set): string { + const encoded = formatModelValue(model.providerId, model.modelId); + // Two enabled builtins can share a modelId (Z.ai Coding Plan + Start Plan + // both ship GLM-5.3). Bare encoding then collides; ACP clients that key on + // `value` reject the duplicate. Prefix those rows the same way third-party + // models already are. + if (collidingBare.has(encoded)) { + return `${model.providerId}\\${model.modelId}`; + } + return encoded; +} + +function buildModelSelectOptions(models: ModelRef[]): Array<{ value: string; name: string }> { + const collidingBare = collidingBareValues(models); + const options: Array<{ value: string; name: string }> = []; + const seen = new Set(); + for (const model of models) { + const encoded = formatModelValue(model.providerId, model.modelId); + const value = encodeModelOptionValue(model, collidingBare); + if (seen.has(value)) continue; + seen.add(value); + const qualify = collidingBare.has(encoded) || !isBuiltinProvider(model.providerId); + options.push({ + value, + name: qualify ? `${model.providerName} › ${model.modelId}` : model.modelId, + }); + } + return options; +} + +function encodeCurrentModelValue(models: ModelRef[], providerId: string, modelId: string): string { + return encodeModelOptionValue( + { providerId, providerName: "", modelId }, + collidingBareValues(models), + ); +} + /** * Parse a configOption `value` back into { providerId, modelId }. * @@ -335,19 +385,19 @@ 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, - ); + // when settings omits providerId (legacy sessions). Colliding builtins use + // the same prefixed encoding as the dropdown so currentValue is one of the + // advertised options rather than a duplicate bare modelId. + const allModels = loadAllModels(); + const currentProvider = currentProviderId || allModels[0]?.providerId || DEFAULT_PROVIDER_ID; + const currentModel = encodeCurrentModelValue(allModels, 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}`, - })); + // Two enabled builtins that share a modelId (Z.ai Coding Plan + Start Plan + // both shipping GLM-5.3) also get the prefixed form so values stay unique. + 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..0c861b5 --- /dev/null +++ b/tests/model-select-options.test.ts @@ -0,0 +1,111 @@ +/** + * Regression: two enabled builtin coding-plan providers ship the same GLM + * model ids. formatModelValue encodes builtins as the bare modelId, so + * session/new used to advertise GLM-5.3 twice. Paseo keys Command Center + * entries on that value and crashes on the duplicate. + * + * Colliding builtins must stay selectable — they are different endpoints — + * so the dropdown disambiguates with the provider-prefixed form already used + * for third-party models, rather than dropping the second plan. + */ + +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("keeps a single builtin encoded as the bare modelId", 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(["GLM-5.3", "GLM-5.3-Flash"]); + expect(model?.currentValue).toBe(formatModelValue("builtin:zai-coding-plan", "GLM-5.3")); + expect(loadAllModels()).toHaveLength(2); + }); +}); From bf93cb745d65d496475ba4e9d05c6a87c20cfcb3 Mon Sep 17 00:00:00 2001 From: Alon Weiss Date: Thu, 10 Sep 2026 23:55:54 +0300 Subject: [PATCH 2/2] fix: always encode ACP model ids as providerId\modelId Collision-only prefixes advertised different id shapes depending on how many builtin coding plans were enabled. Builtins now use the same encoding as third-party models. Legacy bare modelIds still parse as the first enabled builtin. --- src/config/options.ts | 64 ++++++++++-------------------- tests/model-select-options.test.ts | 27 ++++++++----- tests/runtime-model.test.ts | 9 ++--- 3 files changed, 42 insertions(+), 58 deletions(-) diff --git a/src/config/options.ts b/src/config/options.ts index cebf9b4..e2bdbda 100644 --- a/src/config/options.ts +++ b/src/config/options.ts @@ -164,51 +164,38 @@ 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 collidingBareValues(models: ModelRef[]): Set { +function collidingModelIds(models: ModelRef[]): Set { const counts = new Map(); for (const model of models) { - const encoded = formatModelValue(model.providerId, model.modelId); - counts.set(encoded, (counts.get(encoded) ?? 0) + 1); + counts.set(model.modelId, (counts.get(model.modelId) ?? 0) + 1); } const colliding = new Set(); - for (const [encoded, count] of counts) { - if (count > 1) colliding.add(encoded); + for (const [modelId, count] of counts) { + if (count > 1) colliding.add(modelId); } return colliding; } -function encodeModelOptionValue(model: ModelRef, collidingBare: Set): string { - const encoded = formatModelValue(model.providerId, model.modelId); - // Two enabled builtins can share a modelId (Z.ai Coding Plan + Start Plan - // both ship GLM-5.3). Bare encoding then collides; ACP clients that key on - // `value` reject the duplicate. Prefix those rows the same way third-party - // models already are. - if (collidingBare.has(encoded)) { - return `${model.providerId}\\${model.modelId}`; - } - return encoded; -} - function buildModelSelectOptions(models: ModelRef[]): Array<{ value: string; name: string }> { - const collidingBare = collidingBareValues(models); + const collidingIds = collidingModelIds(models); const options: Array<{ value: string; name: string }> = []; const seen = new Set(); for (const model of models) { - const encoded = formatModelValue(model.providerId, model.modelId); - const value = encodeModelOptionValue(model, collidingBare); + const value = formatModelValue(model.providerId, model.modelId); if (seen.has(value)) continue; seen.add(value); - const qualify = collidingBare.has(encoded) || !isBuiltinProvider(model.providerId); + const qualify = collidingIds.has(model.modelId) || !isBuiltinProvider(model.providerId); options.push({ value, name: qualify ? `${model.providerName} › ${model.modelId}` : model.modelId, @@ -217,18 +204,11 @@ function buildModelSelectOptions(models: ModelRef[]): Array<{ value: string; nam return options; } -function encodeCurrentModelValue(models: ModelRef[], providerId: string, modelId: string): string { - return encodeModelOptionValue( - { providerId, providerName: "", modelId }, - collidingBareValues(models), - ); -} - /** * 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("\\"); @@ -385,18 +365,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). Colliding builtins use - // the same prefixed encoding as the dropdown so currentValue is one of the - // advertised options rather than a duplicate bare modelId. + // when settings omits providerId (legacy sessions). const allModels = loadAllModels(); const currentProvider = currentProviderId || allModels[0]?.providerId || DEFAULT_PROVIDER_ID; - const currentModel = encodeCurrentModelValue(allModels, currentProvider, currentModelId); + 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. - // Two enabled builtins that share a modelId (Z.ai Coding Plan + Start Plan - // both shipping GLM-5.3) also get the prefixed form so values stay unique. + // 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 diff --git a/tests/model-select-options.test.ts b/tests/model-select-options.test.ts index 0c861b5..cd11574 100644 --- a/tests/model-select-options.test.ts +++ b/tests/model-select-options.test.ts @@ -1,12 +1,13 @@ /** - * Regression: two enabled builtin coding-plan providers ship the same GLM - * model ids. formatModelValue encodes builtins as the bare modelId, so - * session/new used to advertise GLM-5.3 twice. Paseo keys Command Center - * entries on that value and crashes on the duplicate. + * 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. * - * Colliding builtins must stay selectable — they are different endpoints — - * so the dropdown disambiguates with the provider-prefixed form already used - * for third-party models, rather than dropping the second plan. + * 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"; @@ -90,7 +91,7 @@ describe("model configOptions uniqueness", () => { ]); }); - it("keeps a single builtin encoded as the bare modelId", async () => { + it("encodes a single builtin as providerId\\modelId too", async () => { fakeConfig = { provider: { "builtin:zai-coding-plan": codingPlan( @@ -104,8 +105,16 @@ describe("model configOptions uniqueness", () => { const model = options.find((option) => option.id === "model"); const values = model?.options.map((option) => option.value) ?? []; - expect(values).toEqual(["GLM-5.3", "GLM-5.3-Flash"]); + 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",