From 93bdd27b32eeaa419922a5570169ae31dca52261 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Tue, 8 Sep 2026 10:54:28 +0300 Subject: [PATCH] app, core: a node's outputs are visible, and model family detection Every node's own OUTPUTS were invisible on its own panel in the pipeline editor -- wiring an input already let you pick a source, but opening the node that PRODUCES something told you nothing about what it makes or whether anything reads it. Found live, by a real user just looking at a node, after two full review rounds that only ever checked the consuming side. Fixed with a read-only "Outputs" section per node, naming each output, its type, and who (if anyone) currently wires from it. Also: model family detection. core/probe.py reads a safetensors header (tensor names only, no weights) and asks every installed model module, through the same capability-lookup mechanism already used for empty_latent/decode, whether the file is its own -- core never names a family itself. A real, adversarially-found bug in the ported signature: one of MiniMax H3's three candidate prefixes doesn't correspond to any prefix ComfyUI's own loader actually resolves to, traced through comfy/model_detection.py and comfy/sd.py -- the probe could have called a file H3 that ComfyUI's real loader would refuse. Removed, and pinned with a dedicated test module. A second finding narrowed the folder search to only the folders FunPack's own loaders actually read from. Co-Authored-By: Claude Sonnet 5 --- app/shell/pipeline_window.js | 34 ++++ app/shell/tests/pipeline_window.test.js | 52 +++++ app/tests-layout/pipeline.spec.js | 18 ++ core/probe.py | 114 +++++++++++ core/routes.py | 25 ++- core/tests/test_probe.py | 186 ++++++++++++++++++ core/tests/test_routes_probe.py | 124 ++++++++++++ modules/models/minimax_h3/__init__.py | 37 +++- .../models/minimax_h3/tests/test_detect.py | 81 ++++++++ 9 files changed, 669 insertions(+), 2 deletions(-) create mode 100644 core/probe.py create mode 100644 core/tests/test_probe.py create mode 100644 core/tests/test_routes_probe.py create mode 100644 modules/models/minimax_h3/tests/test_detect.py diff --git a/app/shell/pipeline_window.js b/app/shell/pipeline_window.js index 34697c6a..d39aa9ad 100644 --- a/app/shell/pipeline_window.js +++ b/app/shell/pipeline_window.js @@ -467,9 +467,43 @@ export function mount({ load, describe, check, search, onApply, })); } + // Outputs are never wired from HERE -- wiring is always asked for by the + // input that wants one, through its own Wire button, so the same link + // is never described two different ways depending on which end you + // opened. But an output that is never SHOWN anywhere on its own node is + // a real blind spot, not just a different workflow: open a node that + // exists to produce things and there was nothing here saying what it + // produces, or whether anything downstream already reads it. + if (described.outputs && described.outputs.length) { + rows.push(composer.label.section({ text: "Outputs" })); + described.outputs.forEach((type, index) => { + const name = (described.output_names || [])[index] || type; + const consumers = consumersOf(slot.id, index); + rows.push(composer.settingsRow.default({ + label: name, + hint: consumers.length + ? `${type} — feeds ${consumers.join(", ")}` + : `${type} — nothing reads this yet`, + })); + }); + } + return rows; } + /** Every (consumerId.inputName) currently wired to this slot's output. */ + function consumersOf(slotId, index) { + const found = []; + for (const other of slots) { + for (const [name, value] of Object.entries(other.inputs || {})) { + if (isLink(value) && value[0] === slotId && value[1] === index) { + found.push(`${other.id}.${name}`); + } + } + } + return found; + } + /** Every OTHER slot's output that could legally feed this socket. */ function sourcesFor(slot, socket) { const found = []; diff --git a/app/shell/tests/pipeline_window.test.js b/app/shell/tests/pipeline_window.test.js index e5a08d71..cc948610 100644 --- a/app/shell/tests/pipeline_window.test.js +++ b/app/shell/tests/pipeline_window.test.js @@ -523,6 +523,58 @@ test("an unfed socket has no Unwire button to press", async () => { win.close(); }); +// --- a node's own outputs, shown on its own panel --------------------------- +// +// Wiring is always asked for by the CONSUMING input, never from the output's +// own side -- but an output that is never shown anywhere on its own node's +// panel is a real blind spot: open the node that PRODUCES something and +// there was nothing here saying what it produces, or whether anything reads +// it yet. Missed by two full review rounds because every one of them +// reasoned from "does wiring an input work", never "can you see what a node +// makes just by looking at it". + +test("a node's own outputs are listed on its panel, named and typed", async () => { + const { win } = await opened(); + win.enter("Loaders"); + const rows = [...win.node.querySelectorAll(".cx-settings-row")] + .filter((r) => r.querySelector(".cx-settings-label")?.textContent === "model"); + // "model" is both an INPUT widget on other nodes and this node's OWN + // output name -- the output row is the one with no input control, only + // the read-only hint. + const outputRow = rows.find((r) => !r.querySelector("input, select, textarea, button")); + assert.ok(outputRow, "the Loader's own 'model' output is not shown at all"); + assert.match(outputRow.querySelector(".cx-hint")?.textContent ?? "", /MODEL/); + win.close(); +}); + +test("an output already consumed says who reads it", async () => { + const { win } = await opened(); + win.enter("Loaders"); + const rows = [...win.node.querySelectorAll(".cx-settings-row")] + .filter((r) => r.querySelector(".cx-settings-label")?.textContent === "model"); + const outputRow = rows.find((r) => !r.querySelector("input, select, textarea, button")); + assert.match(outputRow.querySelector(".cx-hint")?.textContent ?? "", /feeds sampler\.model/); + win.close(); +}); + +test("an output nothing reads yet says so", async () => { + const custom = server({ slots: [{ id: "s", group: "Sampling", node: "Sampler", inputs: {} }] }); + const win = openWindow(custom); + await win.ready; + win.enter("Sampling"); + const outputRow = rowFor(win, "LATENT"); + assert.match(outputRow.querySelector(".cx-hint")?.textContent ?? "", /nothing reads this yet/); + win.close(); +}); + +test("a node with no outputs shows no Outputs section", async () => { + const { win } = await opened(); + win.enter("Other"); // "save" has no group of its own + assert.equal([...win.node.querySelectorAll(".cx-eyebrow")] + .some((el) => el.textContent === "Outputs"), false); + win.close(); +}); + // --- wiring a WIDGET input -- "linked inputs" ------------------------------- // // A widget-typed input (a number, a string, a combo) can hold a link too -- diff --git a/app/tests-layout/pipeline.spec.js b/app/tests-layout/pipeline.spec.js index 93a44458..fcb8fd73 100644 --- a/app/tests-layout/pipeline.spec.js +++ b/app/tests-layout/pipeline.spec.js @@ -161,6 +161,24 @@ test("a widget input -- a linked input -- can be wired to another node's output, await expect(textRow.locator("textarea, input")).toBeVisible(); }); +test("a node's own outputs are shown on its own panel, named and consumed", async ({ page }) => { + // Wiring is always asked for by the consuming input; that does not mean an + // output should be invisible on the node that PRODUCES it. Missed by two + // review rounds, both reasoning only from the consuming side -- found + // live, by someone just looking at a node and not seeing what it makes. + await openWindow(page); + await page.locator(".cx-card", { hasText: "Loaders" }).click(); + await page.locator('[aria-label="Nodes"] .cx-filter-row', { hasText: "model" }).click(); + + const outputsHeading = page.locator(".cx-eyebrow", { hasText: "Outputs" }); + await expect(outputsHeading).toBeVisible(); + const modelOutputRow = page.locator(".cx-settings-row") + .filter({ has: page.locator(".cx-settings-label", { hasText: /^model$/ }) }) + .filter({ hasNot: page.locator("input, select, textarea, button") }); + await expect(modelOutputRow).toContainText("MODEL"); + await expect(modelOutputRow).toContainText(/feeds/); +}); + test("the Add-node picker has one search box, not two disagreeing ones", async ({ page }) => { // filterList draws its own "Filter these results" search box; this picker // already has an outer one that re-queries the server on every keystroke. diff --git a/core/probe.py b/core/probe.py new file mode 100644 index 00000000..b04653ec --- /dev/null +++ b/core/probe.py @@ -0,0 +1,114 @@ +"""Which model family a checkpoint file is, without loading it. + +Core reads the file -- a safetensors header is a length-prefixed JSON object +whose keys are tensor names, readable in kilobytes regardless of whether the +file itself is 2GB or 40GB -- and that is ALL it knows how to do. Whether a +set of tensor names belongs to MiniMax H3, or anything else, is a model +module's own business: asking one to identify itself is the same "detect" +capability every model module can already offer, the same shape as the +`empty_latent` and `decode` capabilities modules/models/minimax_h3 provides. +Core aggregates whichever module claims a file and says plainly when none +does -- it is never the one deciding what a family IS called. + +Only .safetensors today: no loader in this pack can read a GGUF file yet, so +detecting one accurately would be groundwork for a format nothing here can +use. +""" + +from __future__ import annotations + +import json +import struct +from pathlib import Path +from typing import List, Optional + +from . import registry as registry_mod + +#: A header past this size is not a safetensors file -- reading a wild length +#: prefix as a real one would try to allocate however many gigabytes a +#: corrupt or unrelated file's first eight bytes happen to spell out. +MAX_HEADER_BYTES = 256 * 1024 * 1024 + + +def read_safetensors_keys(path: Path) -> Optional[List[str]]: + """Tensor names out of a safetensors header, or None when this is not one. + + Layout: an 8-byte little-endian length, then that many bytes of JSON whose + top-level keys are the tensor names (plus `__metadata__`, dropped here). + Nothing past the header is read. + """ + try: + with open(path, "rb") as f: + raw = f.read(8) + if len(raw) < 8: + return None + (length,) = struct.unpack(" MAX_HEADER_BYTES: + return None + header = json.loads(f.read(length).decode("utf-8", errors="replace")) + except (OSError, ValueError, json.JSONDecodeError, struct.error): + return None + if not isinstance(header, dict): + return None + return [k for k in header if k != "__metadata__"] + + +def resolve_diffusion_model(filename: str) -> Optional[Path]: + """Where `filename` lives, across the folders a FunPack loader can + actually read a diffusion model from -- `modules/loaders/diffusion_model` + offers files from "diffusion_models", `modules/loaders/checkpoint` from + "checkpoints"; nothing in this app reads a model from a "unet" folder, + so probing one would risk answering for a file no loader here would ever + actually load under that name. None when it is nowhere findable -- not + this function's job to say why. + """ + try: + import folder_paths + except ImportError: # not inside ComfyUI + return None + for folder in ("diffusion_models", "checkpoints"): + try: + found = folder_paths.get_full_path(folder, filename) + except Exception: # noqa: BLE001 + found = None + if found: + return Path(found) + return None + + +def detect(path: Path, registry=None) -> dict: + """Inspect one checkpoint file. + + Returns ``{"module", "title", "detected", "reason"}``. `module` is the id + of whichever model module claimed it, or None -- and None is a real + answer, not a failure to try hard enough: it means this install has no + module that recognises the file, and the caller should say so rather + than guess a family. `reason` is plain text for a person, always + populated. + """ + p = Path(path) + if not p.is_file(): + return {"module": None, "title": None, "detected": False, + "reason": f"{p.name}: file not found"} + if p.suffix.lower() != ".safetensors": + return {"module": None, "title": None, "detected": False, + "reason": f"{p.name}: only .safetensors can be inspected without loading it"} + keys = read_safetensors_keys(p) + if keys is None: + return {"module": None, "title": None, "detected": False, + "reason": f"{p.name}: not a readable safetensors header"} + + keyset = set(keys) + reg = registry or registry_mod.current() + for spec, claims in reg.providers("detect"): + try: + matched = claims(keyset) + except Exception as exc: # noqa: BLE001 + from . import log + log.failed(f"{spec.id}.detect", exc) + continue + if matched: + return {"module": spec.id, "title": spec.title, "detected": True, + "reason": f"{p.name}: {spec.title}"} + return {"module": None, "title": None, "detected": False, + "reason": f"{p.name}: no installed model module recognises this file"} diff --git a/core/routes.py b/core/routes.py index 506fa434..ac01eca6 100644 --- a/core/routes.py +++ b/core/routes.py @@ -8,7 +8,8 @@ thin adapters over pure functions in `serve`. import asyncio import json -from . import (backend_log, config, graph as graph_mod, log, media, nodes_manager, projects, +from . import (backend_log, config, graph as graph_mod, log, media, nodes_manager, probe as probe_mod, + projects, sysinfo, temp_files, update as update_mod, @@ -447,6 +448,28 @@ def register(routes, prefix=None): payload = await asyncio.to_thread(sysinfo.collect) return web.json_response(payload) + @routes.get(P + "/api/probe") + async def _probe(req): + """Which model family a checkpoint file is, before anything loads it. + + Reads a safetensors header off disk -- to_thread because that is + real (if small) file I/O -- and asks every installed model module + whether the tensor names are its own. Nothing here names a family: + that answer comes back from whichever module claimed it. + """ + filename = (req.query.get("file") or "").strip() + if not filename: + return web.json_response( + {"problems": ["which file is named by ?file="]}, status=400) + path = probe_mod.resolve_diffusion_model(filename) + if path is None: + return web.json_response({ + "module": None, "title": None, "detected": False, + "reason": f"{filename}: not found in the models folders", + }) + payload = await asyncio.to_thread(probe_mod.detect, path) + return web.json_response(payload) + @routes.post(P + "/api/git/update") async def _git_pull(req): body = await req.json() if req.can_read_body else {} diff --git a/core/tests/test_probe.py b/core/tests/test_probe.py new file mode 100644 index 00000000..c874c32f --- /dev/null +++ b/core/tests/test_probe.py @@ -0,0 +1,186 @@ +"""Model family detection: a safetensors header read cold, then handed to +whichever model module claims it. Core never names a family -- these tests +prove that split holds, not any one architecture's signature (that belongs +to modules/models/minimax_h3's own tests). +""" + +import json +import struct +import sys +import types +from pathlib import Path + +import pytest + +from core import probe +from core.contract import ModuleSpec +from core.registry import Registry + + +def _write_safetensors(path, keys): + header = {k: {"dtype": "F16", "shape": [1], "data_offsets": [0, 0]} for k in keys} + header["__metadata__"] = {"format": "pt"} + body = json.dumps(header).encode("utf-8") + path.write_bytes(struct.pack(" bool: return has_block(model, MODEL_CLASS) +def detect(keys) -> bool: + """Whether a safetensors file's tensor names are MiniMax H3's own. + + Read before anything is loaded: this is what lets core refuse a + mismatched checkpoint (or say what a file actually is) before queueing + a run that would otherwise fail deep inside sampling, far from the + mistake. + """ + keyset = set(keys) + return any(all(f"{prefix}{key}" in keyset for key in _SIGNATURE_KEYS) + for prefix in _PREFIXES) + + def traits(model): """What core cannot read off the model for itself.""" if not is_h3(model): @@ -112,4 +147,4 @@ def empty_latent(model, width, height, length, batch_size=1): TRAITS = traits -PROVIDES = {"empty_latent": empty_latent, "decode": decode} +PROVIDES = {"empty_latent": empty_latent, "decode": decode, "detect": detect} diff --git a/modules/models/minimax_h3/tests/test_detect.py b/modules/models/minimax_h3/tests/test_detect.py new file mode 100644 index 00000000..28d7d96a --- /dev/null +++ b/modules/models/minimax_h3/tests/test_detect.py @@ -0,0 +1,81 @@ +"""detect(): whether a set of tensor names is MiniMax H3's own. + +Pinned against ComfyUI's real prefix-resolution algorithm +(comfy/model_detection.py's unet_prefix_from_state_dict, and what +comfy/sd.py's load_diffusion_model_state_dict does with its answer) rather +than only against this module's own idea of what a "reasonable" prefix +looks like -- a probe that says yes to a file ComfyUI's real loader would +refuse is worse than one that says "not detected", which is exactly the +failure a prior review round found and this file exists to pin shut. +""" + +import pytest + + +@pytest.fixture(autouse=True) +def _needs_comfy(comfyui): + """Only needed to import the module under test, which imports comfy.* at + module level -- detect() itself touches no comfy API.""" + + +def _module(): + from modules.models import minimax_h3 + return minimax_h3 + + +def _keys(prefix, *, both=True): + names = ["video_patch_proj.weight", "audio_patch_proj.weight"] + if not both: + names = names[:1] + return {f"{prefix}{n}" for n in names} + + +def test_bare_keys_are_recognised(): + """A standalone unet-only file: ComfyUI's own prefix guess strips + nothing for this layout (no key starts with "model."), so the ORIGINAL + unprefixed keys are what its real loader ends up reading.""" + m = _module() + assert m.detect(_keys("")) + + +def test_model_diffusion_model_prefix_is_recognised(): + """The first, and only H3-relevant, candidate + unet_prefix_from_state_dict actually tries for a full-checkpoint + layout.""" + m = _module() + assert m.detect(_keys("model.diffusion_model.")) + + +def test_bare_diffusion_model_prefix_is_not_recognised(): + """Found by adversarial review: this prefix is not one of the three real + candidates ComfyUI's own unet_prefix_from_state_dict tries, and tracing + what its fallback guess ("model.") does with a state dict keyed this way + shows ComfyUI's real loader would never find these tensors either -- so + this probe must not claim it can.""" + m = _module() + assert not m.detect(_keys("diffusion_model.")) + + +def test_only_one_of_the_two_signature_keys_is_not_enough(): + m = _module() + assert not m.detect(_keys("", both=False)) + assert not m.detect(_keys("model.diffusion_model.", both=False)) + + +def test_the_two_signature_keys_split_across_different_prefixes_do_not_match(): + """One tensor under one prefix, the other under a different one -- not a + real file's shape, and must not be read as one by accident.""" + m = _module() + mixed = {"video_patch_proj.weight", + "model.diffusion_model.audio_patch_proj.weight"} + assert not m.detect(mixed) + + +def test_unrelated_keys_are_not_recognised(): + m = _module() + assert not m.detect({"model.diffusion_model.unrelated.weight", "vae.decoder.weight"}) + + +def test_an_empty_key_set_is_not_recognised(): + m = _module() + assert not m.detect(set())