diff --git a/canvas/src/components/tabs/ConfigTab.tsx b/canvas/src/components/tabs/ConfigTab.tsx index 89a12c32..5e7a0133 100644 --- a/canvas/src/components/tabs/ConfigTab.tsx +++ b/canvas/src/components/tabs/ConfigTab.tsx @@ -191,15 +191,24 @@ 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. + // Track the model the form first rendered, so handleSave can detect + // whether the user actually changed it (vs. only edited tier/skills/etc). + // Two field sources contribute: + // 1. wsMetadataModel — workspace_secrets.MODEL_PROVIDER (DB) + // 2. parsed.runtime_config.model — the template's YAML default + // Whichever was the live runtime value at load time is what currentModelId + // will display, and it's the value Save must diff against. + // + // Why not just diff the YAML directly: after loadConfig mirrors + // wsMetadataModel into runtime_config.model for display, the YAML diff + // is always non-zero on a no-op save, fires PUT /model, and triggers + // an auto-restart for unrelated edits. Why not diff against + // wsMetadataModel alone: on a hermes workspace where MODEL_PROVIDER + // was never set (pre-#240 workspaces, or workspaces created via direct + // API without going through the picker), wsMetadataModel="" but the + // form shows the YAML default — diffing against "" makes any first + // save propagate-and-restart even when the user didn't touch the model. + // Capturing the actual rendered value covers both. const [originalModel, setOriginalModel] = useState(""); const successTimerRef = useRef>(undefined); @@ -230,7 +239,11 @@ export function ConfigTab({ workspaceId }: Props) { const m = await api.get<{ model?: string }>(`/workspaces/${workspaceId}/model`); wsMetadataModel = (m.model || "").trim(); } catch { /* non-fatal */ } - setOriginalModel(wsMetadataModel); + // originalModel is set further down once the YAML has been parsed — + // we want it to reflect what the form ACTUALLY rendered, which may + // be the YAML's runtime_config.model fallback when MODEL_PROVIDER + // is empty. Setting it here from wsMetadataModel alone would be + // wrong for hermes/pre-#240 workspaces. // Load explicit provider override (Option B PR-5). Endpoint returns // {provider: "", source: "default"} when no override is set, so the @@ -272,6 +285,12 @@ export function ConfigTab({ workspaceId }: Props) { merged.runtime_config = { ...(merged.runtime_config ?? {}), model: wsMetadataModel }; } if (wsMetadataTier !== null) merged.tier = wsMetadataTier; + // Snapshot the rendered model so handleSave's diff stays scoped to + // user-initiated changes. mirrors the read precedence in + // currentModelId so an unrelated save (tier change) doesn't fire + // a /model PUT just because MODEL_PROVIDER was empty and the form + // showed the YAML default. + setOriginalModel(merged.runtime_config?.model || merged.model || ""); setConfig(merged); } catch { // No platform-managed config.yaml. Some runtimes (hermes, external) @@ -292,6 +311,11 @@ export function ConfigTab({ workspaceId }: Props) { ...(wsMetadataModel ? { runtime_config: { model: wsMetadataModel } } : {}), ...(wsMetadataTier !== null ? { tier: wsMetadataTier } : {}), } as ConfigData); + // Same snapshot as the merged-path branch above. Falls back to + // empty string when neither MODEL_PROVIDER nor a YAML model was + // present; handleSave's `nextModel && ...` guard then skips the + // PUT correctly. + setOriginalModel(wsMetadataModel); } finally { setLoading(false); } diff --git a/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx b/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx index c2c2b4af..d07d4806 100644 --- a/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx +++ b/canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx @@ -484,4 +484,91 @@ describe("ConfigTab — Provider override (Option B PR-5)", () => { expect.objectContaining({ tier: 3 }), ); }); + + // Failure-gating sibling pin to the store-flush test above. The + // production code places `updateNodeData` AFTER `await api.patch(...)` + // inside the same `if (Object.keys(dbPatch).length > 0)` block, so a + // PATCH rejection should throw before the store call. Without this + // pin, a future refactor that wraps the PATCH in try/catch and + // unconditionally calls updateNodeData would ship green — and then + // the badge would lie when the server actually rejected the change. + // Codified review feedback from PR #2545 (Agent 2). + it("does NOT flush into useCanvasStore.updateNodeData when the PATCH rejects", 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.mockRejectedValue(new Error("500 from workspace-server")); + + 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); + + // Wait for handleSave to settle (succeeds-or-fails). PATCH must + // have been attempted; the error swallow inside handleSave keeps + // saving=false in finally. + await waitFor(() => { + expect(apiPatch.mock.calls.some(([p]) => p === "/workspaces/ws-test")).toBe(true); + }); + // Critically: the store must NOT have been told about the failed + // change. Otherwise the badge would lie about a write the server + // rejected. + const tierFlushes = storeUpdateNodeData.mock.calls.filter(([, body]) => + typeof (body as { tier?: number }).tier === "number", + ); + expect(tierFlushes.length).toBe(0); + }); + + // Pin the hermes/pre-#240 edge case: workspace where MODEL_PROVIDER + // was never written but YAML has runtime_config.model: "something". + // originalModel must reflect the rendered baseline (the YAML value), + // not the empty MODEL_PROVIDER, so an unrelated save (tier change) + // doesn't fire a /model PUT and trigger an auto-restart. Codified + // review feedback from PR #2545 (Agent 1, "Important"). + it("does not PUT /model when MODEL_PROVIDER is empty and the user only edited an unrelated field", async () => { + wireApi({ + workspaceRuntime: "hermes", + workspaceModel: "", // legacy workspace — never went through the picker + configYamlContent: + "name: ws\nruntime: hermes\ntier: 2\nruntime_config:\n model: nousresearch/hermes-4-70b\n", + providerValue: "", + templates: [ + { + id: "hermes", + name: "Hermes", + runtime: "hermes", + models: [{ id: "nousresearch/hermes-4-70b", name: "Hermes 4 70B", required_env: ["HERMES_API_KEY"] }], + providers: ["nous"], + }, + ], + }); + 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(() => { + expect(apiPatch.mock.calls.some(([p]) => p === "/workspaces/ws-test")).toBe(true); + }); + const modelPuts = apiPut.mock.calls.filter(([path]) => path === "/workspaces/ws-test/model"); + expect(modelPuts.length).toBe(0); + }); });