diff --git a/browser_tests/unit/asset-staleness.test.mjs b/browser_tests/unit/asset-staleness.test.mjs index b1749531..5212cf85 100644 --- a/browser_tests/unit/asset-staleness.test.mjs +++ b/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"); +}); diff --git a/browser_tests/unit/stale-placeholders.test.mjs b/browser_tests/unit/stale-placeholders.test.mjs index a283ccb8..a2308b1b 100644 --- a/browser_tests/unit/stale-placeholders.test.mjs +++ b/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()"))), diff --git a/web/js/comfyui-mcp-panel.js b/web/js/comfyui-mcp-panel.js index 2827791f..badd5ef5 100644 --- a/web/js/comfyui-mcp-panel.js +++ b/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, diff --git a/web/js/lib/asset-staleness.js b/web/js/lib/asset-staleness.js index f4de7ebd..b4021bc9 100644 --- a/web/js/lib/asset-staleness.js +++ b/web/js/lib/asset-staleness.js @@ -761,14 +761,18 @@ export function collectAllGraphs(rootGraph) { * map supplies the concrete-def key to look its options up under (#458×#366). Widgets * not in the map fall back to their own name. */ -export function refreshComboOptionsFromDefs(node, defsByType, defTypeKey, widgetNameMap) { +export function refreshComboOptionsFromDefs(node, defsByType, defTypeKey, widgetNameMap, mergedInputs) { let refreshed = 0; if (!node || !defsByType) return refreshed; try { const type = defTypeKey ?? node.type ?? node.comfyClass; const def = type ? defsByType[type] : null; if (!def) return refreshed; - const inputs = { ...(def.input?.required ?? {}), ...(def.input?.optional ?? {}) }; + // #1193 — the merged map may be supplied by a caller sweeping MANY nodes, so a graph of + // N nodes of the same type spreads its inputs once rather than N times. This function is + // O(inputs) per call and #1193 measured the register/reapply phase at 3.97s of a 9s run; + // rebuilding combos there must not add a fresh spread per node. + const inputs = mergedInputs ?? { ...(def.input?.required ?? {}), ...(def.input?.optional ?? {}) }; for (const w of node.widgets ?? []) { if (w?.name == null) continue; const defKey = (widgetNameMap && widgetNameMap[w.name]) ?? w.name; @@ -790,15 +794,126 @@ export function refreshComboOptionsFromDefs(node, defsByType, defTypeKey, widget return refreshed; } +/** Merged `{...required, ...optional}` per TYPE, built at most once per sweep (#1193). */ +function mergedInputsFor(def, cache, type) { + if (cache.has(type)) return cache.get(type); + const merged = { ...(def.input?.required ?? {}), ...(def.input?.optional ?? {}) }; + cache.set(type, merged); + return merged; +} + +/** + * Is this input spec a combo whose authoritative option list came back EMPTY? + * + * A combo's spec is `[[opt, ...], config?]`. An empty first element is the backend saying + * "this widget has no valid values" — which is a real answer (#507/#1133: a server with zero + * checkpoints), not a panel failure. It is worth DISCLOSING and never worth refusing over. + */ +function isEmptyComboSpec(spec) { + const first = Array.isArray(spec) ? spec[0] : undefined; + return Array.isArray(first) && first.length === 0; +} + +/** + * #1172 — the combos whose AUTHORITATIVE list is empty, for the types actually on the graph. + * + * Computed from the `/object_info` payload the caller already holds, deliberately NOT by + * re-reading widgets after `app.refreshComboInNodes()` resolves. #1193 wants to stop waiting + * on that call; a disclosure that depended on it would tie this answer to the one await it is + * trying to drop, and would report nothing at all if the call were ever abandoned mid-flight. + * + * Scoped to types PRESENT on the graph. The backend may publish dozens of empty combos for + * packs the user is not using, and reporting those would bury the one node that matters — + * the same "say only what was observed to matter" rule `describeUploadFailure` follows. + * + * @returns {Array<{type: string, widget: string}>} sorted, de-duplicated + */ +export function emptyComboListsOnGraph(rootGraph, defsByType) { + const found = new Map(); + if (!defsByType) return []; + try { + const cache = new Map(); + for (const graph of collectAllGraphs(rootGraph)) { + for (const node of graph._nodes ?? []) { + const type = node?.type ?? node?.comfyClass; + const def = type ? defsByType[type] : null; + if (!def) continue; + const inputs = mergedInputsFor(def, cache, type); + for (const [widget, spec] of Object.entries(inputs)) { + if (!isEmptyComboSpec(spec)) continue; + found.set(`${type}::${widget}`, { type, widget }); + } + } + } + } catch { + /* best-effort — a malformed def means we disclose less, never that we refuse */ + } + return [...found.values()].sort((a, b) => a.type.localeCompare(b.type) || a.widget.localeCompare(b.widget)); +} + +/** + * #1172 — what to tell the agent when the backend's own list came back empty. + * + * WHAT THIS DELIBERATELY DOES NOT DO: name a cause, or predict what another command will do. + * The same rule `describeUploadFailure` states for #756, and a first version of this note + * broke it twice. It said "the backend is not finding it: check that server's model paths, + * then restart ComfyUI" — an INFERENCE. An empty list is equally a node whose combo the + * CLIENT populates, a pack that publishes an empty enum deliberately, or a server with + * genuinely zero of that asset. And it said "setting one of these widgets will be refused", + * which is simply FALSE: `set-widget.js` treats an authoritative empty list as unknowable + * and PERFORMS the write, reporting `empty_option_list: true` (#507/#1133). That sentence + * would have talked an agent out of a write that succeeds. + * + * So it reports the observation and the one thing that genuinely follows from it — that a + * second refresh returns the same payload, because the refresh is not what is empty. + */ +export function emptyComboNote(empties) { + if (!Array.isArray(empties) || !empties.length) return ""; + const shown = empties.slice(0, 6).map((e) => `${e.type}.${e.widget}`).join(", "); + const more = empties.length > 6 ? `, and ${empties.length - 6} more` : ""; + return ( + `${empties.length} combo widget${empties.length === 1 ? "" : "s"} on this graph ` + + `${empties.length === 1 ? "has" : "have"} an EMPTY list of valid values in the definitions ` + + `the backend just published: ${shown}${more}. The refresh itself succeeded — this empty ` + + `list is what /object_info answered. Why the list is empty ` + + `is NOT established here: it may be an asset the server has none of, a combo this node ` + + `populates client-side, or a pack that publishes an empty list deliberately. A write to ` + + `one of these is still permitted and reports empty_option_list, but nothing can verify ` + + `the value against a list the backend did not provide.` + ); +} + export function reapplyDefsToLiveNodes(rootGraph, defsByType) { let repaired = 0; if (!defsByType) return repaired; try { + // #1193 — one merged input map per TYPE for the whole sweep, not one per node. This + // removes the per-node spread; it does NOT make the sweep free. Rebuilding still walks + // every widget and copies each authoritative array, which is work the sweep did not do + // before. Stated rather than claimed safe: it has not been measured against #1193's + // 3.97s register/reapply figure. + const mergedCache = new Map(); for (const graph of collectAllGraphs(rootGraph)) { for (const node of graph._nodes ?? []) { const type = node?.type ?? node?.comfyClass; const def = type ? defsByType[type] : null; if (!def) continue; + // #1172 — REBUILD THE COMBO OPTION ARRAYS HERE. + // + // This sweep stamped `ctor.nodeData` and reconciled UNKNOWN widget names but never + // touched `options.values`, even though `refreshComboOptionsFromDefs` — which does + // exactly that — sits thirty lines above and was called only from the set_widget + // path. Rebuilding was delegated entirely to `app.refreshComboInNodes()`, whose + // per-node effect the panel never observes and never verifies, so a newly added + // CheckpointLoaderSimple kept an empty `ckpt_name` list while `refresh_nodes` + // answered `refreshed: true`. Doing it here also makes the panel's combo state + // correct independently of whether that frontend call is waited on at all, which is + // a partial answer to #1193. + // + // It is safe to run alongside the frontend call: `refreshComboOptionsFromDefs` + // skips a dynamic (function) option source and writes the same values + // `/object_info` just published. + refreshComboOptionsFromDefs(node, defsByType, type, undefined, mergedInputsFor(def, mergedCache, type)); // Stamp only onto a TYPE-SPECIFIC constructor (already carries nodeData // for this type) — never onto a shared generic/unknown fallback class, // which would corrupt every other unknown node.