diff --git a/canvas/src/components/ProviderModelSelector.tsx b/canvas/src/components/ProviderModelSelector.tsx index bca8cc1e..ce576679 100644 --- a/canvas/src/components/ProviderModelSelector.tsx +++ b/canvas/src/components/ProviderModelSelector.tsx @@ -310,12 +310,26 @@ export function ProviderModelSelector({ onChange({ providerId: "", model: "", envVars: [] }); return; } - // When switching providers, default the model to the first concrete - // entry in that provider (or empty if wildcard). Avoids showing a - // stale model id from the previous provider. + // When switching providers: + // - wildcard provider → empty (free-text input takes over) + // - exactly 1 concrete model → auto-pick (no choice to make) + // - 2+ concrete models → leave empty so the operator MUST pick + // + // Background: previously this defaulted to `next.models[0]` for any + // non-wildcard provider, which silently set the alphabetically-first + // model in the bucket. Bit a real user on 2026-05-03 — they picked + // the MiniMax provider intending `MiniMax-M2.7` but the form silently + // set `MiniMax-M2` (first in the list). They never saw the model + // dropdown change because the provider+model widgets are visually + // distinct, and the workspace deployed with the wrong model. Caller + // already disables Deploy/Save while `model.trim() === ""`, so the + // empty default forces an explicit pick without loosening any other + // gate. const defaultModel = next.wildcard ? "" - : next.models[0]?.id ?? ""; + : next.models.length === 1 + ? next.models[0]?.id ?? "" + : ""; onChange({ providerId: next.id, model: defaultModel, diff --git a/canvas/src/components/__tests__/ProviderModelSelector.test.tsx b/canvas/src/components/__tests__/ProviderModelSelector.test.tsx index f5746dd4..c98a4dbe 100644 --- a/canvas/src/components/__tests__/ProviderModelSelector.test.tsx +++ b/canvas/src/components/__tests__/ProviderModelSelector.test.tsx @@ -182,15 +182,38 @@ describe("", () => { expect(modelSelect.disabled).toBe(true); }); - it("picking provider emits onChange with default model + envVars", () => { + it("picking a multi-model provider emits onChange with empty model (forces explicit pick)", () => { const { onChange } = setup(); const providerSelect = screen.getByTestId("provider-select"); const catalog = buildProviderCatalog(CLAUDE_CODE_MODELS); const minimax = catalog.find((p) => p.vendor === "minimax")!; + // MiniMax bucket holds 2 models (MiniMax-M2 + MiniMax-M2.7). Auto- + // picking the first one used to bite a real user (2026-05-03): + // they wanted M2.7 but the silent default put M2 in the deploy + // payload. Now the model field must come back empty so the next + // dropdown is required-empty and Save/Deploy stay disabled until + // the user picks. fireEvent.change(providerSelect, { target: { value: minimax.id } }); expect(onChange).toHaveBeenCalledWith({ providerId: minimax.id, - model: "MiniMax-M2", + model: "", + envVars: ["ANTHROPIC_AUTH_TOKEN"], + }); + }); + + it("picking a single-model provider auto-fills the model (no choice to make)", () => { + const { onChange } = setup(); + const providerSelect = screen.getByTestId("provider-select"); + const catalog = buildProviderCatalog(CLAUDE_CODE_MODELS); + // GLM-4.6 is the only model under the zai vendor in the fixture — + // a "0 vs many" boundary check. With only one option, forcing the + // user to re-pick adds friction without preventing any error. + const zai = catalog.find((p) => p.vendor === "zai")!; + expect(zai.models.length).toBe(1); + fireEvent.change(providerSelect, { target: { value: zai.id } }); + expect(onChange).toHaveBeenCalledWith({ + providerId: zai.id, + model: "GLM-4.6", envVars: ["ANTHROPIC_AUTH_TOKEN"], }); }); @@ -250,7 +273,7 @@ describe("", () => { expect(screen.getByText(/requires:/).textContent).toMatch(/CLAUDE_CODE_OAUTH_TOKEN/); }); - it("switching provider resets model to first concrete option", () => { + it("switching to a multi-model provider clears the stale model id", () => { const catalog = buildProviderCatalog(CLAUDE_CODE_MODELS); const oauth = catalog.find((p) => p.vendor === "anthropic-oauth")!; const minimax = catalog.find((p) => p.vendor === "minimax")!; @@ -260,9 +283,11 @@ describe("", () => { onChange, }); fireEvent.change(screen.getByTestId("provider-select"), { target: { value: minimax.id } }); + // Empty rather than auto-picked — see "picking a multi-model + // provider …" test above for the user-facing rationale. expect(onChange).toHaveBeenCalledWith({ providerId: minimax.id, - model: "MiniMax-M2", + model: "", envVars: ["ANTHROPIC_AUTH_TOKEN"], }); }); diff --git a/canvas/src/components/tabs/ConfigTab.tsx b/canvas/src/components/tabs/ConfigTab.tsx index f75700ed..89a12c32 100644 --- a/canvas/src/components/tabs/ConfigTab.tsx +++ b/canvas/src/components/tabs/ConfigTab.tsx @@ -191,6 +191,16 @@ export function ConfigTab({ workspaceId }: Props) { // data, written into /configs/config.yaml on next provision too). const [provider, setProvider] = useState(""); const [originalProvider, setOriginalProvider] = useState(""); + // Track the model that loaded from the DB (workspace_secrets.MODEL_PROVIDER + // via /workspaces/:id/model) separately from the YAML's runtime_config.model. + // handleSave's diff used to compare nextModel against the YAML's value; + // after the loadConfig fix mirrors wsMetadataModel into runtime_config.model + // for display, that diff would always be non-zero (YAML default vs. + // overridden value) and trigger a /model PUT — which auto-restarts — + // on every Save. Comparing against the loaded MODEL_PROVIDER instead + // keeps unrelated saves (tier change, skill edit) from rebooting the + // workspace just because the template's YAML default differs. + const [originalModel, setOriginalModel] = useState(""); const successTimerRef = useRef>(undefined); useEffect(() => { @@ -220,6 +230,7 @@ export function ConfigTab({ workspaceId }: Props) { const m = await api.get<{ model?: string }>(`/workspaces/${workspaceId}/model`); wsMetadataModel = (m.model || "").trim(); } catch { /* non-fatal */ } + setOriginalModel(wsMetadataModel); // Load explicit provider override (Option B PR-5). Endpoint returns // {provider: "", source: "default"} when no override is set, so the @@ -249,7 +260,17 @@ export function ConfigTab({ workspaceId }: Props) { // form doesn't contradict the node badge (issue: badge=T3, form=T2). const merged = { ...DEFAULT_CONFIG, ...parsed } as ConfigData; if (wsMetadataRuntime) merged.runtime = wsMetadataRuntime; - if (wsMetadataModel) merged.model = wsMetadataModel; + if (wsMetadataModel) { + // Single source of truth: MODEL_PROVIDER (DB) is the live runtime + // value. Override BOTH top-level + nested runtime_config.model so + // currentModelId (which reads runtime_config.model first) doesn't + // silently fall through to the template default. Without the + // nested override, a workspace deployed with `MiniMax-M2` shows + // the template's `runtime_config.model: sonnet` in the UI even + // though the container env (and chat) actually use MiniMax-M2. + merged.model = wsMetadataModel; + merged.runtime_config = { ...(merged.runtime_config ?? {}), model: wsMetadataModel }; + } if (wsMetadataTier !== null) merged.tier = wsMetadataTier; setConfig(merged); } catch { @@ -265,6 +286,10 @@ export function ConfigTab({ workspaceId }: Props) { ...DEFAULT_CONFIG, runtime: wsMetadataRuntime, model: wsMetadataModel, + // Mirror the merged-path fix above — keep top-level + nested in + // sync so currentModelId picks up wsMetadataModel even when the + // form falls into the no-config.yaml branch (hermes/external). + ...(wsMetadataModel ? { runtime_config: { model: wsMetadataModel } } : {}), ...(wsMetadataTier !== null ? { tier: wsMetadataTier } : {}), } as ConfigData); } finally { @@ -415,6 +440,15 @@ export function ConfigTab({ workspaceId }: Props) { } if (Object.keys(dbPatch).length > 0) { await api.patch(`/workspaces/${workspaceId}`, dbPatch); + // Mirror the DB write into the canvas store node data so the + // header pill (TIER T2/T3, RUNTIME claude-code/hermes) and the + // node card update immediately. Without this push, the workspace + // row reflects the new tier but every UI surface that reads from + // useCanvasStore.nodes (header badge, ContextMenu, etc.) keeps + // showing the stale value until the next full hydrate. Bug + // surfaced 2026-05-03 — user picked T3, hit Save & Restart, + // database said tier=3, badge still said T2. + useCanvasStore.getState().updateNodeData(workspaceId, dbPatch); } // Model has its own endpoint (separate from the general workspace @@ -436,21 +470,26 @@ export function ConfigTab({ workspaceId }: Props) { // configured" error in the chat. Caught 2026-04-30 on hongmingwang // hermes workspace 32993ee7-…cb9d75d112a5. const nextModelRaw = (nextSource.runtime_config as Record | undefined)?.model; - const oldModelRaw = (oldParsed.runtime_config as Record | undefined)?.model; const nextModel = typeof nextModelRaw === "string" && nextModelRaw ? nextModelRaw : typeof nextSource.model === "string" ? nextSource.model : ""; - const oldModel = - typeof oldModelRaw === "string" && oldModelRaw - ? oldModelRaw - : (oldParsed.model as string) || ""; + // Diff against the loaded MODEL_PROVIDER (the runtime source of + // truth), not the YAML's runtime_config.model. After loadConfig + // mirrors wsMetadataModel into runtime_config.model for display, + // nextModel always equals the loaded value on a no-op save — + // diffing against oldModelRaw (the unmirrored YAML default) would + // make every Save fire a /model PUT and trigger an auto-restart, + // even when the user only changed an unrelated field. Comparing + // against `originalModel` keeps the PUT scoped to actual user + // intent. let modelSaveError: string | null = null; - if (nextModel && nextModel !== oldModel) { + if (nextModel && nextModel !== originalModel) { try { await api.put(`/workspaces/${workspaceId}/model`, { model: nextModel }); + setOriginalModel(nextModel); } catch (e) { modelSaveError = e instanceof Error ? e.message : "Model update was rejected"; } diff --git a/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx b/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx index 2714cba8..c2c2b4af 100644 --- a/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx +++ b/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx @@ -38,10 +38,15 @@ vi.mock("@/lib/api", () => ({ }, })); +// Shared store stub — `updateNodeData` is exposed so a test can assert the +// node-data flush happens after a successful PATCH (regression: previously +// the DB updated but the canvas badge stayed stale until full hydrate). +const storeUpdateNodeData = vi.fn(); +const storeRestartWorkspace = vi.fn(); vi.mock("@/store/canvas", () => ({ useCanvasStore: Object.assign( - (selector: (s: unknown) => unknown) => selector({ restartWorkspace: vi.fn(), updateNodeData: vi.fn() }), - { getState: () => ({ restartWorkspace: vi.fn(), updateNodeData: vi.fn() }) }, + (selector: (s: unknown) => unknown) => selector({ restartWorkspace: storeRestartWorkspace, updateNodeData: storeUpdateNodeData }), + { getState: () => ({ restartWorkspace: storeRestartWorkspace, updateNodeData: storeUpdateNodeData }) }, ), })); @@ -90,6 +95,8 @@ beforeEach(() => { apiGet.mockReset(); apiPatch.mockReset(); apiPut.mockReset(); + storeUpdateNodeData.mockReset(); + storeRestartWorkspace.mockReset(); }); describe("ConfigTab — Provider override (Option B PR-5)", () => { @@ -333,4 +340,148 @@ describe("ConfigTab — Provider override (Option B PR-5)", () => { expect(providerCalls[0][1]).toEqual({ provider: "" }); }); }); + + // Display-vs-storage drift regression (2026-05-03 incident, workspace + // e13aebd8…). User deployed claude-code with MiniMax-M2 stored in + // MODEL_PROVIDER. The container env (MODEL=MiniMax-M2) and chat + // worked correctly, but the Config tab showed "Claude Code + // subscription / Claude Sonnet (OAuth)" — i.e. the template's + // runtime_config.model: sonnet default — because currentModelId + // reads runtime_config.model first and loadConfig was overriding + // only the top-level config.model field. The merged shape was: + // { model: "MiniMax-M2", runtime_config: { model: "sonnet" } } + // and currentModelId picked "sonnet". Fix: loadConfig propagates + // wsMetadataModel into BOTH places so the form is a single source + // of truth (DB-backed MODEL_PROVIDER). Pinning the merged-path + // branch with the exact reproducing shape: claude-code template + // YAML has runtime_config.model: sonnet; live workspace's + // MODEL_PROVIDER is MiniMax-M2; tab must show the latter. + it("prefers MODEL_PROVIDER over the template's runtime_config.model on load", async () => { + wireApi({ + workspaceRuntime: "claude-code", + workspaceModel: "MiniMax-M2", + configYamlContent: "name: ws\nruntime: claude-code\nruntime_config:\n model: sonnet\n", + providerValue: "", + templates: [ + { + id: "claude-code-default", + name: "Claude Code", + runtime: "claude-code", + models: [ + { id: "sonnet", name: "Claude Sonnet (OAuth)", required_env: ["CLAUDE_CODE_OAUTH_TOKEN"] }, + { id: "MiniMax-M2", name: "MiniMax M2", required_env: ["MINIMAX_API_KEY"] }, + { id: "MiniMax-M2.7", name: "MiniMax M2.7", required_env: ["MINIMAX_API_KEY"] }, + ], + }, + ], + }); + + render(); + const modelSelect = (await screen.findByTestId("model-select")) as HTMLSelectElement; + await waitFor(() => expect(modelSelect.value).toBe("MiniMax-M2")); + + // Provider dropdown should also reflect MiniMax (back-derived from + // the model slug since LLM_PROVIDER is unset). Without the fix, + // the selector falls back to the first catalog entry whose first + // model matches "sonnet" → anthropic-oauth bucket → "Claude Code + // subscription". + const providerSelect = screen.getByTestId("provider-select") as HTMLSelectElement; + const selectedOption = providerSelect.options[providerSelect.selectedIndex]; + expect(selectedOption.textContent ?? "").toMatch(/MiniMax/); + }); + + // Sibling pin to the display-fix above. The display fix mirrors + // wsMetadataModel into runtime_config.model so the selector renders + // the live value; that mirror means handleSave's old YAML-vs-form + // diff would always be non-zero on a no-op save (YAML default + // "sonnet" vs. mirrored "MiniMax-M2") and PUT /model — which + // server-side SetModel chains into an auto-restart. handleSave now + // diffs against the loaded MODEL_PROVIDER instead. Pin: an + // unrelated edit (tier change) must NOT touch /model when the + // model itself didn't change. + it("does not PUT /model on a no-op save when only an unrelated field changed", async () => { + wireApi({ + workspaceRuntime: "claude-code", + workspaceModel: "MiniMax-M2", + configYamlContent: "name: ws\nruntime: claude-code\ntier: 2\nruntime_config:\n model: sonnet\n", + providerValue: "", + templates: [ + { + id: "claude-code-default", + name: "Claude Code", + runtime: "claude-code", + models: [ + { id: "sonnet", name: "Claude Sonnet", required_env: ["CLAUDE_CODE_OAUTH_TOKEN"] }, + { id: "MiniMax-M2", name: "MiniMax M2", required_env: ["MINIMAX_API_KEY"] }, + ], + }, + ], + }); + apiPut.mockResolvedValue({}); + apiPatch.mockResolvedValue({}); + + render(); + const tierSelect = (await screen.findByLabelText(/tier/i)) as HTMLSelectElement; + fireEvent.change(tierSelect, { target: { value: "3" } }); + + const saveBtn = screen.getByRole("button", { name: /^save$/i }); + fireEvent.click(saveBtn); + + await waitFor(() => { + const tierPatches = apiPatch.mock.calls.filter(([path, body]) => + path === "/workspaces/ws-test" && (body as { tier?: number }).tier === 3, + ); + expect(tierPatches.length).toBe(1); + }); + // Spurious /model PUT would fire here without the originalModel + // diff baseline. The model itself didn't change, so /model must + // stay untouched (otherwise SetModel auto-restarts). + const modelPuts = apiPut.mock.calls.filter(([path]) => path === "/workspaces/ws-test/model"); + expect(modelPuts.length).toBe(0); + }); + + // Save-then-stale-badge regression (2026-05-03 incident). User + // selected T3 in the Tier dropdown, hit Save & Restart, the workspace + // PATCH succeeded (`tier: 3` in DB), but the canvas header pill kept + // showing "TIER T2" until a full hydrate. Root cause: handleSave + // sent the PATCH to workspace-server but never pushed the same + // change into useCanvasStore.updateNodeData, so every UI surface + // reading from the store kept its stale value. Pin: a successful + // tier PATCH must mirror into the store so the badge updates + // synchronously with the response. + it("flushes the dbPatch into useCanvasStore.updateNodeData after a successful PATCH", async () => { + wireApi({ + workspaceRuntime: "claude-code", + workspaceModel: "MiniMax-M2", + configYamlContent: "name: ws\nruntime: claude-code\ntier: 2\nruntime_config:\n model: sonnet\n", + providerValue: "", + templates: [ + { + id: "claude-code-default", + name: "Claude Code", + runtime: "claude-code", + models: [{ id: "sonnet", name: "Sonnet", required_env: ["CLAUDE_CODE_OAUTH_TOKEN"] }], + }, + ], + }); + apiPatch.mockResolvedValue({ status: "updated" }); + + render(); + const tierSelect = (await screen.findByLabelText(/tier/i)) as HTMLSelectElement; + fireEvent.change(tierSelect, { target: { value: "3" } }); + + const saveBtn = screen.getByRole("button", { name: /^save$/i }); + fireEvent.click(saveBtn); + + await waitFor(() => { + expect(apiPatch.mock.calls.some(([p]) => p === "/workspaces/ws-test")).toBe(true); + }); + // Without the store flush, the badge would keep reading tier=2 + // from useCanvasStore.nodes until a full hydrate. Pin: handleSave + // pushes the same fields it PATCHed. + expect(storeUpdateNodeData).toHaveBeenCalledWith( + "ws-test", + expect.objectContaining({ tier: 3 }), + ); + }); });