fix(1172): rebuild combo options during the reapply sweep, and disclose an empty authoritative list

browser_tests/unit/asset-staleness.test.mjs

@@ -8,6 +8,11 @@
 import test from "node:test";
 import assert from "node:assert/strict";
 import { readFileSync } from "node:fs";
+import { fileURLToPath } from "node:url";
+
+/** #1172 — the wiring assertion reads the shipped monolith, so the disclosure cannot be
+ *  added to the verdict while the forwarding whitelist silently drops it. */
+const PANEL_JS = fileURLToPath(new URL("../../web/js/comfyui-mcp-panel.js", import.meta.url));
 
 import {
   findNodeByScopedId,
@@ -22,6 +27,8 @@ import {
   reconcileUnknownWidgetNames,
   collectAllGraphs,
   reapplyDefsToLiveNodes,
+  emptyComboListsOnGraph,
+  emptyComboNote,
   collectMissingNodeTypeReasons,
   collectUnexplainedRedOutlines,
   combineNodeErrorMaps,
@@ -1207,3 +1214,121 @@ test("resolveMissingModelDirectory: NON-ultralytics directories never regress",
   assert.equal(resolveMissingModelDirectory("ultralytics_extra", "segm/x.pt"), "ultralytics_extra");
   assert.equal(resolveMissingModelDirectory(null, "segm/x.pt"), null);
 });
+
+// ── #1172: the reapply sweep must REBUILD combo options, and empty lists must be disclosed ──
+
+const CKPT_DEF = (values) => ({ input: { required: { ckpt_name: [values, {}] } } });
+
+test("#1172 the reapply sweep repopulates a combo whose option list is empty", () => {
+  // The reported bug. panel_add_node builds the widget from the REGISTERED nodeData, so a
+  // newly added CheckpointLoaderSimple starts with `values: []`. The sweep stamped nodeData
+  // and reconciled UNKNOWN names but never touched options.values, leaving the node unusable
+  // while refresh_nodes answered `refreshed: true`.
+  const node = {
+    id: 1,
+    type: "CheckpointLoaderSimple",
+    widgets: [{ name: "ckpt_name", value: "", options: { values: [] } }],
+    constructor: {},
+  };
+  reapplyDefsToLiveNodes(graphOf([node]), { CheckpointLoaderSimple: CKPT_DEF(["anime.safetensors", "sd15.ckpt"]) });
+  assert.deepEqual(node.widgets[0].options.values, ["anime.safetensors", "sd15.ckpt"]);
+});
+
+test("#1172 the rebuild reaches nodes inside SUBGRAPHS", () => {
+  // collectAllGraphs already walks them; the rebuild rides the same sweep, so a promoted
+  // inner node must be repaired too rather than silently skipped.
+  const inner = { id: 2, type: "CheckpointLoaderSimple", widgets: [{ name: "ckpt_name", options: { values: [] } }], constructor: {} };
+  const root = graphOf([{ id: 1, type: "Host", subgraph: { _nodes: [inner] } }]);
+  reapplyDefsToLiveNodes(root, { CheckpointLoaderSimple: CKPT_DEF(["a.safetensors"]) });
+  assert.deepEqual(inner.widgets[0].options.values, ["a.safetensors"]);
+});
+
+test("#1172 a DYNAMIC (function) option source is never clobbered", () => {
+  // #507/#1133 hazard: a client-populated combo derives its own list. Overwriting it with the
+  // backend's array would break exactly the nodes #1133 is making writable.
+  const dynamic = () => ["computed"];
+  const node = { id: 3, type: "CheckpointLoaderSimple", widgets: [{ name: "ckpt_name", options: { values: dynamic } }], constructor: {} };
+  reapplyDefsToLiveNodes(graphOf([node]), { CheckpointLoaderSimple: CKPT_DEF(["a.safetensors"]) });
+  assert.equal(node.widgets[0].options.values, dynamic, "a function source must survive the sweep");
+});
+
+test("#1172 emptyComboListsOnGraph reports only EMPTY lists, and only for types on the graph", () => {
+  const node = { id: 1, type: "CheckpointLoaderSimple", widgets: [], constructor: {} };
+  const defs = {
+    CheckpointLoaderSimple: CKPT_DEF([]),
+    // present in the payload but NOT on the graph — the backend may publish dozens of empty
+    // combos for packs the user is not using, and reporting those buries the one that matters.
+    SomeOtherLoader: { input: { required: { other_name: [[], {}] } } },
+  };
+  assert.deepEqual(emptyComboListsOnGraph(graphOf([node]), defs), [
+    { type: "CheckpointLoaderSimple", widget: "ckpt_name" },
+  ]);
+});
+
+test("#1172 FALSE-POSITIVE FLOOR: a populated list discloses nothing", () => {
+  // If this ever goes red the disclosure fires on every refresh and is worthless.
+  const node = { id: 1, type: "CheckpointLoaderSimple", widgets: [], constructor: {} };
+  assert.deepEqual(emptyComboListsOnGraph(graphOf([node]), { CheckpointLoaderSimple: CKPT_DEF(["a.safetensors"]) }), []);
+  // A non-combo input (a type string, not an option array) is not an empty combo either.
+  assert.deepEqual(
+    emptyComboListsOnGraph(graphOf([node]), { CheckpointLoaderSimple: { input: { required: { steps: ["INT", { default: 20 }] } } } }),
+    [],
+  );
+  assert.deepEqual(emptyComboListsOnGraph(graphOf([node]), null), []);
+  assert.deepEqual(emptyComboListsOnGraph(null, { CheckpointLoaderSimple: CKPT_DEF([]) }), []);
+});
+
+test("#1172 the note points at the BACKEND, never at another refresh", () => {
+  // The wrong-remedy failure this repo keeps removing: telling the agent to re-run the very
+  // command that just answered. The refresh worked; the server's answer is what is empty.
+  const note = emptyComboNote([{ type: "CheckpointLoaderSimple", widget: "ckpt_name" }]);
+  assert.match(note, /CheckpointLoaderSimple\.ckpt_name/);
+  assert.match(note, /this empty list is what \/object_info answered/i);
+  // …and nothing about the PANEL's internals: the disclosure is built from the payload alone,
+  // so a clause about what the panel did or did not skip overstates what it can establish.
+  assert.doesNotMatch(note, /panel (skipped|failed|missed)/i, "no claim about panel internals");
+  // …and it must NOT predict what a later refresh will return: a second /object_info read can
+  // observe changed server state, so that clause was a prediction dressed as an observation.
+  assert.doesNotMatch(note, /refresh(ing)? again (returns|will return)/i, "no prediction about another command");
+  assert.doesNotMatch(note, /panel_refresh_nodes|try refreshing|refresh the nodes again/i);
+  assert.equal(emptyComboNote([]), "");
+  assert.equal(emptyComboNote(null), "");
+});
+
+test("#1172 the note NAMES NO CAUSE and does not predict another command's outcome", () => {
+  // #756's rule, applied to this note. A first version broke it twice: it inferred "the
+  // backend is not finding it — check model paths, then restart", and it asserted "setting
+  // one of these widgets will be refused". The second is FALSE — set-widget.js treats an
+  // authoritative empty list as unknowable and PERFORMS the write with empty_option_list
+  // (#507/#1133) — and would have talked an agent out of a write that succeeds.
+  const note = emptyComboNote([{ type: "CheckpointLoaderSimple", widget: "ckpt_name" }]);
+  assert.doesNotMatch(note, /model path|restart ComfyUI|not finding|missing/i, "no inferred cause");
+  assert.doesNotMatch(note, /will be refused|cannot be set|must not be set/i, "no false refusal claim");
+  // …and it must say the true thing about writes, so the agent is not left guessing.
+  assert.match(note, /still permitted/i);
+  assert.match(note, /empty_option_list/);
+  // The one inference that IS supportable: the refresh is not what is empty.
+  assert.match(note, /NOT established here/);
+});
+
+test("#1172 WIRING: the disclosure survives the `refreshed: true` branch (#981's hole)", () => {
+  // That branch returns a FIXED object literal, so a field the verdict carries but the
+  // whitelist does not name is dropped on exactly the successful path where it matters.
+  // #981 fell into this hole at this same line; a verdict-only change would look correct in
+  // every unit test and report nothing to the agent.
+  const src = readFileSync(PANEL_JS, "utf8");
+  const code = src.split("\n").filter((l) => !l.trim().startsWith("//")).join("\n");
+  assert.match(code, /verdict\.empty_combo_lists = empties;/, "the verdict must carry the field");
+  assert.match(
+    code,
+    /if \(refreshed\) return \{ ok: true, refreshed: true, \.\.\.stale, \.\.\.emptyCombos \};/,
+    "…and the refreshed:true branch must forward it",
+  );
+  // The spread alone is not enough: `emptyCombos` could still be built without the list
+  // itself, forwarding only the note. Pin BOTH fields of the mapping.
+  assert.match(code, /empty_combo_lists: verdict\.empty_combo_lists,/, "the list must be mapped");
+  assert.match(code, /empty_combo_lists_note: verdict\.empty_combo_lists_note,/, "…and the note");
+  // #1133: an empty list must never flip the verdict to failed — that would re-refuse via the
+  // verdict what #1133 deliberately permits via the write path.
+  assert.doesNotMatch(code, /empty_combo_lists[\s\S]{0,200}?refreshed = false/, "disclosure, not failure");
+});

browser_tests/unit/stale-placeholders.test.mjs

@@ -259,8 +259,16 @@ test("#981 (codex r2) source guard: the disclosure survives the SUCCESS path of
   // success path the warning existed and no caller could ever see it. Found by tracing
   // the consumers of the verdict, not by reading the producer.
   const src = readFileSync(new URL("../../web/js/comfyui-mcp-panel.js", import.meta.url), "utf8");
-  assert.match(src, /if \(refreshed\) return \{ ok: true, refreshed: true, \.\.\.stale \};/, "forwarded on success");
+  // #1172 added a SECOND disclosure that rides the same branch. The hole is the branch's
+  // fixed object literal, so the guard now names every field that must survive it — adding a
+  // third disclosure without extending this line is the same bug again.
+  assert.match(
+    src,
+    /if \(refreshed\) return \{ ok: true, refreshed: true, \.\.\.stale, \.\.\.emptyCombos \};/,
+    "forwarded on success",
+  );
   assert.match(src, /stale_placeholders_note: verdict\.stale_placeholders_note/, "and the note with it");
+  assert.match(src, /empty_combo_lists_note: verdict\.empty_combo_lists_note/, "…and #1172's note too");
   // `ok` must stay true: the refresh did what it claims, and the reload flag is about
   // the canvas, not about the refresh having failed.
   assert.ok(!/ok: false/.test(src.slice(src.indexOf("async refresh_nodes()"), src.indexOf("graph_serialize()"))),

web/js/comfyui-mcp-panel.js

@@ -149,6 +149,8 @@ import { openSidePanel } from "./cmcp-sidepanel-ui.js";
 import {
   isStaleAssetCandidate as isStaleAssetCandidateLib,
   reapplyDefsToLiveNodes,
+  emptyComboListsOnGraph,
+  emptyComboNote,
   refreshComboOptionsFromDefs,
   collectAllGraphs,
   collectMissingNodeTypeReasons,
@@ -1342,6 +1344,28 @@ async function registerComfyNodeDefs(preloadedDefs) {
       verdict.stale_placeholders = stale;
       verdict.stale_placeholders_note = stalePlaceholderNote(stale);
     }
+    // #1172 — DISCLOSE an authoritative list that came back empty.
+    //
+    // Every input `describeNodeDefRefresh` takes is STRUCTURAL — app present, defs obtained,
+    // register ran, combo API present, combo resolved — so `refreshed: true` was a claim
+    // about API calls resolving, not about the definitions being usable. The payload said
+    // `ckpt_name: [[], {…}]` and the panel had it in hand at register and reapply, and
+    // discarded it; the agent then found out at queue time via `Value not in list (… not
+    // in [])`.
+    //
+    // `refreshed` stays TRUE. A server with zero checkpoints is a real answer, and #507/#1133
+    // establish that empty lists are sometimes legitimate — flipping the verdict to false
+    // would re-refuse via the verdict exactly what #1133 deliberately permits via the write
+    // path. Disclosure, not failure.
+    //
+    // Read from `defs`, which is already in hand, and NOT by re-reading widgets after
+    // `app.refreshComboInNodes()` resolves: #1193 wants to stop waiting on that call, and a
+    // disclosure that depended on it would report nothing if it were ever abandoned.
+    const empties = emptyComboListsOnGraph(getGraphCtx().rootGraph, defs);
+    if (empties.length) {
+      verdict.empty_combo_lists = empties;
+      verdict.empty_combo_lists_note = emptyComboNote(empties);
+    }
   } catch {
     /* a diagnosis must never turn a successful refresh into a failure */
   }
@@ -9225,7 +9249,18 @@ const GRAPH_TOOL_EXECUTORS = {
           stale_placeholders_note: verdict.stale_placeholders_note,
         }
       : {};
-    if (refreshed) return { ok: true, refreshed: true, ...stale };
+    // #1172 — forwarded through the SAME hole #981 fell into. The `refreshed: true` branch
+    // below returns a fixed object literal, so a field the verdict carries but this whitelist
+    // does not name is silently dropped on exactly the successful path where the disclosure
+    // matters most. Adding the field to the verdict without adding it here would look correct
+    // in every unit test of the verdict and report nothing to the agent.
+    const emptyCombos = verdict != null && typeof verdict === "object" && verdict.empty_combo_lists?.length
+      ? {
+          empty_combo_lists: verdict.empty_combo_lists,
+          empty_combo_lists_note: verdict.empty_combo_lists_note,
+        }
+      : {};
+    if (refreshed) return { ok: true, refreshed: true, ...stale, ...emptyCombos };
     return {
       ok: true,
       refreshed: false,