core-lead/molecule-core
1
0
Fork 0
forked from molecule-ai/molecule-core
Code Pull Requests Activity

fix(canvas): tighten originalModel + pin store-flush failure-gating (review feedback)

Browse Source 
PR #2545 self-review findings.

(1) originalModel was set from wsMetadataModel alone. On a hermes/pre-#240
workspace where MODEL_PROVIDER was never written but YAML has
runtime_config.model: "something", originalModel="" while the form
rendered "something" — handleSave's diff fired /model PUT on every
unrelated save (tier change → workspace auto-restart). Snapshot from
the actual rendered model in BOTH loadConfig branches so the diff
stays scoped to user-initiated changes.

(2) The store-flush test asserted the call happened but didn't pin
success-gating. A future refactor wrapping the PATCH in try/catch and
unconditionally calling updateNodeData would have shipped green and
left the badge lying about server-rejected writes. New test pins the
PATCH-rejects-no-flush invariant.

(3) Hermes-edge regression test for (1).

All 1214 canvas tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-05-02 23:37:52 -07:00
parent 7f0c58d563
commit bdd1d09dfb
2 changed files with 121 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
canvas/src/components/tabs/ConfigTab.tsx
View File

@ -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<ReturnType<typeof setTimeout>>(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);
}

87
canvas/src/components/tabs/__tests__/ConfigTab.provider.test.tsx
View File

@ -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(<ConfigTab workspaceId="ws-test" />);
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(<ConfigTab workspaceId="ws-test" />);
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);
});
});