From 979d3f692715e58c0ec8d72117b18b0ae16d4053 Mon Sep 17 00:00:00 2001 From: marwanvx Date: Mon, 14 Sep 2026 15:22:51 +0300 Subject: [PATCH 1/2] fix(tui): guard model parse and handle variant selection (#48957) - Harden parse in util/model.ts against non-string, undefined, and empty inputs to avoid crashes (U.split is not a function). - Parse #variant syntax from model identifiers so modelID remains clean and catalog lookups succeed. - Validate configured model documents in context/local.tsx so malformed or incomplete entries cleanly return undefined rather than leaking { modelID: undefined }. - Thread variant through fallbackModel and preferredSelection while keeping in-session user variant switches authoritative. - Guard switchLabel against incomplete model descriptors. - Add unit and regression tests for model parsing and local selection. Closes #48957 --- packages/tui/src/app.tsx | 5 ++- packages/tui/src/context/local.tsx | 37 ++++++++++++++----- packages/tui/src/util/model.ts | 17 +++++++-- .../tui/test/context/local-selection.test.tsx | 28 ++++++++++++++ packages/tui/test/util/model.test.ts | 31 ++++++++++++++++ 5 files changed, 104 insertions(+), 14 deletions(-) diff --git a/packages/tui/src/app.tsx b/packages/tui/src/app.tsx index 6b6db4718220..d1d35d504cec 100644 --- a/packages/tui/src/app.tsx +++ b/packages/tui/src/app.tsx @@ -640,8 +640,8 @@ function App(props: { pair?: DialogPairCredentials }) { onMount(() => { batch(() => { if (args.agent) local.agent.set(args.agent) - if (args.model) { - const { providerID, modelID } = Model.parse(args.model) + if (args.model && typeof args.model === "string") { + const { providerID, modelID, variant } = Model.parse(args.model) if (!providerID || !modelID) return toast.show({ variant: "warning", @@ -649,6 +649,7 @@ function App(props: { pair?: DialogPairCredentials }) { duration: 3000, }) local.model.set({ providerID, modelID }, { recent: true }) + if (variant) local.model.variant.set(variant) } if (args.sessionID && !args.fork) { route.navigate({ diff --git a/packages/tui/src/context/local.tsx b/packages/tui/src/context/local.tsx index c10baed3090e..8035bed0489b 100644 --- a/packages/tui/src/context/local.tsx +++ b/packages/tui/src/context/local.tsx @@ -54,7 +54,8 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const models = () => data.location.model.list(location.ref) const providers = () => data.location.provider.list(location.ref) - function isModelValid(model: ModelPreferenceModel) { + function isModelValid(model?: ModelPreferenceModel) { + if (!model?.providerID || !model?.modelID) return false return !!models()?.some((item) => item.providerID === model.providerID && item.id === model.modelID) } @@ -205,18 +206,35 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ ?.findLast((entry) => entry.type === "document" && entry.info.model !== undefined) const configured = entry?.type === "document" ? entry.info.model : undefined if (!configured) return - return typeof configured === "string" - ? { ...parse(configured), variant: undefined } - : { providerID: configured.providerID, modelID: configured.model, variant: configured.variant } + if (typeof configured === "string") { + const parsed = parse(configured) + if (!parsed.providerID || !parsed.modelID) return undefined + return parsed + } + if (typeof configured === "object" && configured !== null) { + const record = configured as Record + const providerID = typeof record.providerID === "string" ? record.providerID : undefined + const modelID = + typeof record.model === "string" + ? record.model + : typeof record.modelID === "string" + ? record.modelID + : undefined + const variant = typeof record.variant === "string" ? record.variant : undefined + if (!providerID || !modelID) return undefined + return { providerID, modelID, variant } + } + return undefined }) const fallbackModel = createMemo(() => { - if (args.model) { - const { providerID, modelID } = parse(args.model) - if (isModelValid({ providerID, modelID })) { + if (args.model && typeof args.model === "string") { + const { providerID, modelID, variant } = parse(args.model) + if (providerID && modelID && isModelValid({ providerID, modelID })) { return { providerID, modelID, + variant, } } } @@ -242,7 +260,7 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const a = agent.current() return getFirstValidModel( () => a && selectionState.newSessionModelByLocationAgent[locationAgentKey(a.id)], - () => a?.model && { providerID: a.model.providerID, modelID: a.model.id }, + () => a?.model && { providerID: a.model.providerID, modelID: a.model.id, variant: a.model.variant }, fallbackModel, ) }) @@ -265,12 +283,13 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ return `${JSON.stringify([ref.directory, ref.workspaceID])}:${agentID}` } - function preferredSelection(model: ModelPreferenceModel): ModelSelection { + function preferredSelection(model: ModelPreferenceModel & { variant?: string }): ModelSelection { const configured = agent.current()?.model const fallback = configuredModel() const preferred = preferences.variant[modelPreferenceKey(model)] const variant = normalizeModelVariant( preferred ?? + model.variant ?? (configured?.providerID === model.providerID && configured.id === model.modelID ? configured.variant : undefined) ?? diff --git a/packages/tui/src/util/model.ts b/packages/tui/src/util/model.ts index 215602845fd9..d6a0b399a1e1 100644 --- a/packages/tui/src/util/model.ts +++ b/packages/tui/src/util/model.ts @@ -1,6 +1,16 @@ -export function parse(value: string) { - const [providerID, ...modelID] = value.split("/") - return { providerID, modelID: modelID.join("/") } +export function parse(value?: string | null): { providerID: string; modelID: string; variant?: string } { + if (typeof value !== "string" || value.length === 0) { + return { providerID: "", modelID: "" } + } + const variantIndex = value.indexOf("#") + const rawRef = variantIndex === -1 ? value : value.slice(0, variantIndex) + const variant = variantIndex === -1 ? undefined : value.slice(variantIndex + 1) || undefined + const [providerID, ...rest] = rawRef.split("/") + return { + providerID: providerID ?? "", + modelID: rest.join("/"), + ...(variant ? { variant } : {}), + } } export function formatRef(model: { providerID: string; id: string; variant?: string }) { @@ -12,6 +22,7 @@ export function switchLabel( models?: readonly { providerID: string; id: string; name: string }[], previous?: { providerID: string; id: string; variant?: string }, ) { + if (!model?.providerID || !model?.id) return "" if (previous?.providerID === model.providerID && previous.id === model.id) return `Switched variant to ${model.variant ?? "default"}` const display = models?.find((item) => item.providerID === model.providerID && item.id === model.id)?.name diff --git a/packages/tui/test/context/local-selection.test.tsx b/packages/tui/test/context/local-selection.test.tsx index 4c7376476bae..5d1d89cab333 100644 --- a/packages/tui/test/context/local-selection.test.tsx +++ b/packages/tui/test/context/local-selection.test.tsx @@ -147,6 +147,34 @@ test("same-model agent switches clear drafts without a model acknowledgment", as expect(setup.local.model.current()?.modelID).toBe("second") }) +test("handles malformed config models and non-string CLI model args safely", async () => { + await using setup = await renderLocal({ + models: [model("first"), model("second", ["low", "high"])], + args: { model: true as any }, + fetch: (url) => { + if (url.pathname === "/api/config") + return json([ + { type: "document", info: { model: { providerID: "incomplete" } } }, + { type: "document", info: { model: null } }, + { type: "document", info: { model: 123 } }, + { type: "document", info: { model: "provider/second#high" } }, + ]) + }, + }) + expect(setup.local.model.selection()).toEqual({ providerID: "provider", modelID: "second", variant: "high" }) +}) + +test("preserves CLI model and variant ahead of recents and config and allows in-session variant switches", async () => { + await using setup = await renderLocal({ + models: [model("first"), model("second", ["low", "high"])], + args: { model: "provider/second#high" }, + preferences: { recent: [{ providerID: "provider", modelID: "first" }] }, + }) + expect(setup.local.model.selection()).toEqual({ providerID: "provider", modelID: "second", variant: "high" }) + setup.local.model.variant.set("low") + expect(setup.local.model.selection()).toEqual({ providerID: "provider", modelID: "second", variant: "low" }) +}) + async function publishSelection( setup: Awaited>, agent: string, diff --git a/packages/tui/test/util/model.test.ts b/packages/tui/test/util/model.test.ts index 888b102013b3..32a0b2d80887 100644 --- a/packages/tui/test/util/model.test.ts +++ b/packages/tui/test/util/model.test.ts @@ -7,6 +7,31 @@ describe("util.model", () => { expect(parse("invalid")).toEqual({ providerID: "invalid", modelID: "" }) }) + test("parses variant from model identifier if present", () => { + expect(parse("anthropic/claude-3-5-sonnet#thinking")).toEqual({ + providerID: "anthropic", + modelID: "claude-3-5-sonnet", + variant: "thinking", + }) + expect(parse("openrouter/anthropic/claude-3.5-sonnet#high")).toEqual({ + providerID: "openrouter", + modelID: "anthropic/claude-3.5-sonnet", + variant: "high", + }) + expect(parse("provider/model#")).toEqual({ + providerID: "provider", + modelID: "model", + }) + }) + + test("handles undefined, null, non-string, and empty model identifiers safely", () => { + expect(parse(undefined as any)).toEqual({ providerID: "", modelID: "" }) + expect(parse(null as any)).toEqual({ providerID: "", modelID: "" }) + expect(parse({} as any)).toEqual({ providerID: "", modelID: "" }) + expect(parse(true as any)).toEqual({ providerID: "", modelID: "" }) + expect(parse("")).toEqual({ providerID: "", modelID: "" }) + }) + test("includes the selected variant in model refs", () => { expect(formatRef({ providerID: "anthropic", id: "sonnet", variant: "thinking" })).toBe("anthropic/sonnet/thinking") expect(formatRef({ providerID: "anthropic", id: "sonnet" })).toBe("anthropic/sonnet") @@ -46,4 +71,10 @@ describe("util.model", () => { "Switched model to anthropic/sonnet/high", ) }) + + test("handles empty or invalid model in switchLabel safely", () => { + expect(switchLabel(undefined as any)).toBe("") + expect(switchLabel({} as any)).toBe("") + expect(switchLabel({ providerID: "anthropic" } as any)).toBe("") + }) }) From 48eeb3de7ef9f5a0ef21ec4cea8c686926a3e4bd Mon Sep 17 00:00:00 2001 From: marwanvx Date: Tue, 15 Sep 2026 17:23:58 +0300 Subject: [PATCH 2/2] fix(tui): transient CLI variant precedence and canonical model parsing --- packages/tui/src/app.tsx | 4 +- packages/tui/src/context/local.tsx | 45 +++++++++++++------ packages/tui/src/util/model.ts | 19 ++++---- .../tui/test/context/local-selection.test.tsx | 20 +++++++++ packages/tui/test/util/model.test.ts | 12 ++--- 5 files changed, 70 insertions(+), 30 deletions(-) diff --git a/packages/tui/src/app.tsx b/packages/tui/src/app.tsx index d1d35d504cec..c884d7fe6bdb 100644 --- a/packages/tui/src/app.tsx +++ b/packages/tui/src/app.tsx @@ -641,15 +641,13 @@ function App(props: { pair?: DialogPairCredentials }) { batch(() => { if (args.agent) local.agent.set(args.agent) if (args.model && typeof args.model === "string") { - const { providerID, modelID, variant } = Model.parse(args.model) + const { providerID, modelID } = Model.parse(args.model) if (!providerID || !modelID) return toast.show({ variant: "warning", message: `Invalid model format: ${args.model}`, duration: 3000, }) - local.model.set({ providerID, modelID }, { recent: true }) - if (variant) local.model.variant.set(variant) } if (args.sessionID && !args.fork) { route.navigate({ diff --git a/packages/tui/src/context/local.tsx b/packages/tui/src/context/local.tsx index 8035bed0489b..8669fe4a12c9 100644 --- a/packages/tui/src/context/local.tsx +++ b/packages/tui/src/context/local.tsx @@ -1,7 +1,7 @@ import { createStore } from "solid-js/store" import { dedupeWith } from "effect/Array" import { createSimpleContext } from "./helper" -import { batch, createMemo, onCleanup } from "solid-js" +import { batch, createEffect, createMemo, createSignal, onCleanup } from "solid-js" import { useEvent } from "./event" import path from "path" import { useTuiPaths } from "./runtime" @@ -166,6 +166,7 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const repository = createModelPreferenceRepository(path.join(paths.state, "model.json")) const pendingSelectionCommits = new Map() + const [cliSuperseded, setCliSuperseded] = createSignal(false) const selectionKey = (value: ModelSelection) => `${modelPreferenceKey(value)}:${normalizeModelVariant(value.variant) ?? "default"}` const saveState = { @@ -227,18 +228,15 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ return undefined }) - const fallbackModel = createMemo(() => { - if (args.model && typeof args.model === "string") { - const { providerID, modelID, variant } = parse(args.model) - if (providerID && modelID && isModelValid({ providerID, modelID })) { - return { - providerID, - modelID, - variant, - } - } - } + const cliModel = createMemo(() => { + if (!args.model || typeof args.model !== "string") return undefined + const parsed = parse(args.model) + if (!parsed.providerID || !parsed.modelID) return undefined + if (!isModelValid({ providerID: parsed.providerID, modelID: parsed.modelID })) return undefined + return parsed + }) + const fallbackModel = createMemo(() => { const configured = configuredModel() if (configured && isModelValid(configured)) return configured @@ -260,11 +258,23 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const a = agent.current() return getFirstValidModel( () => a && selectionState.newSessionModelByLocationAgent[locationAgentKey(a.id)], + cliModel, () => a?.model && { providerID: a.model.providerID, modelID: a.model.id, variant: a.model.variant }, fallbackModel, ) }) + createEffect(() => { + const cli = cliModel() + if (!cli) return + if (!models()) return + if (!preferences.ready) return + const key = modelPreferenceKey(cli) + if (preferences.recent.some((item) => modelPreferenceKey(item) === key)) return + setPreferences("recent", recentModels({ providerID: cli.providerID, modelID: cli.modelID }, preferences.recent)) + savePreferences() + }) + const currentSelection = createMemo(() => { if (route.data.type === "session") return sessionSelection(route.data.sessionID) const model = newSessionModel() @@ -283,12 +293,16 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ return `${JSON.stringify([ref.directory, ref.workspaceID])}:${agentID}` } - function preferredSelection(model: ModelPreferenceModel & { variant?: string }): ModelSelection { + function preferredSelection(model: ModelSelection): ModelSelection { const configured = agent.current()?.model const fallback = configuredModel() + const cli = cliSuperseded() ? undefined : cliModel() + const cliVariant = + cli && cli.providerID === model.providerID && cli.modelID === model.modelID ? cli.variant : undefined const preferred = preferences.variant[modelPreferenceKey(model)] const variant = normalizeModelVariant( - preferred ?? + cliVariant ?? + preferred ?? model.variant ?? (configured?.providerID === model.providerID && configured.id === model.modelID ? configured.variant @@ -357,6 +371,7 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ if (route.data.type === "session") { const sessionID = route.data.sessionID const current = sessionSelection(sessionID) + setCliSuperseded(true) setSessionDraft( sessionID, current?.providerID === model.providerID && current.modelID === model.modelID @@ -368,6 +383,7 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ const current = agent.current() if (!current) return false setSelectionState("newSessionModelByLocationAgent", locationAgentKey(current.id), model) + setCliSuperseded(true) return true } @@ -542,6 +558,7 @@ export const { use: useLocal, provider: LocalProvider } = createSimpleContext({ set(value: string | undefined) { const m = currentSelection() if (!m) return + setCliSuperseded(true) if (route.data.type === "session") { setSessionDraft(route.data.sessionID, { ...m, variant: normalizeModelVariant(value) }) } diff --git a/packages/tui/src/util/model.ts b/packages/tui/src/util/model.ts index d6a0b399a1e1..deeca2f09f9a 100644 --- a/packages/tui/src/util/model.ts +++ b/packages/tui/src/util/model.ts @@ -1,15 +1,18 @@ +import { Model } from "@opencode/schema/model" + export function parse(value?: string | null): { providerID: string; modelID: string; variant?: string } { if (typeof value !== "string" || value.length === 0) { return { providerID: "", modelID: "" } } - const variantIndex = value.indexOf("#") - const rawRef = variantIndex === -1 ? value : value.slice(0, variantIndex) - const variant = variantIndex === -1 ? undefined : value.slice(variantIndex + 1) || undefined - const [providerID, ...rest] = rawRef.split("/") - return { - providerID: providerID ?? "", - modelID: rest.join("/"), - ...(variant ? { variant } : {}), + try { + const ref = Model.Ref.parse(value) + return { + providerID: ref.providerID, + modelID: ref.id, + ...(ref.variant ? { variant: ref.variant } : {}), + } + } catch { + return { providerID: "", modelID: "" } } } diff --git a/packages/tui/test/context/local-selection.test.tsx b/packages/tui/test/context/local-selection.test.tsx index 5d1d89cab333..e4d8d34ba42b 100644 --- a/packages/tui/test/context/local-selection.test.tsx +++ b/packages/tui/test/context/local-selection.test.tsx @@ -1,4 +1,5 @@ import { expect, test } from "bun:test" +import path from "node:path" import { agent, model, renderLocal, session } from "../fixture/local" import { json } from "../fixture/tui-client" @@ -175,6 +176,25 @@ test("preserves CLI model and variant ahead of recents and config and allows in- expect(setup.local.model.selection()).toEqual({ providerID: "provider", modelID: "second", variant: "low" }) }) +test("CLI variant wins transiently over stored preference without persisting", async () => { + await using setup = await renderLocal({ + models: [model("first"), model("second", ["low", "high"])], + args: { model: "provider/second#high" }, + preferences: { variant: { "provider/second": "low" } }, + }) + expect(setup.local.model.selection()).toEqual({ providerID: "provider", modelID: "second", variant: "high" }) + await setup.waitFor(async () => { + await Bun.sleep(10) + return setup.local.model.recent().some((item) => item.providerID === "provider" && item.modelID === "second") + }) + const stored = (await Bun.file(path.join(setup.state, "model.json")) + .json() + .catch(() => ({}))) as { variant?: Record } + expect(stored.variant?.["provider/second"]).toBe("low") + setup.local.model.variant.set("low") + expect(setup.local.model.selection()).toEqual({ providerID: "provider", modelID: "second", variant: "low" }) +}) + async function publishSelection( setup: Awaited>, agent: string, diff --git a/packages/tui/test/util/model.test.ts b/packages/tui/test/util/model.test.ts index 32a0b2d80887..a20ff65e2f68 100644 --- a/packages/tui/test/util/model.test.ts +++ b/packages/tui/test/util/model.test.ts @@ -4,7 +4,7 @@ import { formatRef, parse, switchLabel } from "../../src/util/model" describe("util.model", () => { test("splits provider from a nested model identifier", () => { expect(parse("provider/org/model")).toEqual({ providerID: "provider", modelID: "org/model" }) - expect(parse("invalid")).toEqual({ providerID: "invalid", modelID: "" }) + expect(parse("invalid")).toEqual({ providerID: "", modelID: "" }) }) test("parses variant from model identifier if present", () => { @@ -18,10 +18,12 @@ describe("util.model", () => { modelID: "anthropic/claude-3.5-sonnet", variant: "high", }) - expect(parse("provider/model#")).toEqual({ - providerID: "provider", - modelID: "model", - }) + }) + + test("rejects empty variants and extra separators as invalid", () => { + expect(parse("provider/model#")).toEqual({ providerID: "", modelID: "" }) + expect(parse("provider/mo#de#l")).toEqual({ providerID: "", modelID: "" }) + expect(parse("openai/gpt-5#high#extra")).toEqual({ providerID: "", modelID: "" }) }) test("handles undefined, null, non-string, and empty model identifiers safely", () => {