fix(1172): rebuild combo options during the reapply sweep, and disclose an empty authoritative list - #1218
Merged
Merged
Conversation
…s stay empty Claiming #1172. Diagnosis: reapplyDefsToLiveNodes never rebuilds combo option arrays, and describeNodeDefRefresh's inputs are all STRUCTURAL — nothing inspects the content it just registered. Bulk lands in lib/asset-staleness.js.
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…n empty list TWO HALVES, because the report contains two distinct failures. REBUILD. reapplyDefsToLiveNodes stamped ctor.nodeData and reconciled UNKNOWN widget names but never touched options.values — even though refreshComboOptionsFromDefs, which does exactly that, sits thirty lines above it in the same module 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, and the agent found out at queue time via "Value not in list (… not in [])". Doing it in-panel also makes the combo state correct independently of whether that frontend call is waited on, which is a partial answer to #1193. DISCLOSE. Every input describeNodeDefRefresh takes is STRUCTURAL — app present, defs obtained, register ran, combo API present, combo resolved. refreshed:true was a claim about API calls RESOLVING, not about the definitions being usable. The payload said ckpt_name: [[], {…}], the panel held it at register and at reapply, and discarded it. refreshed STAYS TRUE (#1133). A server with zero checkpoints is a real answer, and #507/#1133 establish that empty lists are sometimes legitimate; flipping the verdict would re-refuse via the verdict exactly what #1133 permits via the write path. Disclosure, not failure. Read from the defs payload already in hand, NOT by re-reading widgets after app.refreshComboInNodes() resolves (#1193) — a disclosure that depended on that await would report nothing if the call were ever abandoned mid-flight. The note points at the BACKEND: the panel did refresh, the server itself published an empty list, so refreshing again returns the same thing and the remedy is model paths or a restart. Naming the refresh as its own remedy is the wrong-remedy failure this repo keeps removing. SCOPED to types present on the graph. The backend may publish dozens of empty combos for packs the user is not using; reporting those buries the one node that matters. PERF (#1193 measured register/reapply at 3.97s of a 9s run): the merged {...required, ...optional} map is hoisted per TYPE for the whole sweep instead of being rebuilt per node, so a graph of N same-type nodes spreads once. #981's HOLE, EXTENDED RATHER THAN RE-DUG: the refreshed:true 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 at this same line. The guard test now names every field that must survive it. Two of my own errors, both caught by the repo's own guards rather than by inspection: a NUL byte written into asset-staleness.js (caught by the stray-control-character test), and the #981 source guard pinning the literal I extended. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex review, P1 — and it is #756's rule broken in the note that cites #756's rule. The first version said "the backend is not finding it: check that server's model paths, then restart ComfyUI". That is an INFERENCE. An empty list is equally a combo this node populates CLIENT-side, a pack that publishes an empty enum deliberately, or a server with genuinely zero of that asset. Naming one is the same defect as the workflow fence asserting "the workflow was switched" for every mismatch (#750). Worse, it said "setting one of these widgets will be refused until the backend lists it", 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 — a fabricated constraint, in a disclosure whose whole purpose is to stop the panel over-claiming. The note now reports the observation, states plainly that the cause is NOT established here, and says the true thing about writes so the agent is not left guessing. Also from the review: - the source guard pinned the spread and the note but not the list mapping itself, so dropping `empty_combo_lists` from the forwarded object would have gone unnoticed. Both fields are now pinned. - the perf comment claimed safety it had not measured. Hoisting the merged input map removes the per-node spread; it does not make the sweep free, since rebuilding still walks every widget and copies each authoritative array. Stated, not claimed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…either Third pass on the same sentence, and the same rule each time. It said "refreshing again returns the same thing". That is a PREDICTION about another command — the exact thing the comment two lines above forbids. A later refresh does a fresh /object_info read and can observe changed server state: models added, a restart, a pack installed. The clause was a guess wearing an observation's clothes. What survives is only what this refresh established: the empty list is what /object_info answered, not a step the panel skipped. The agent can decide for itself whether retrying is worthwhile; the note's job is to say what was seen. Three versions of one sentence, rejected for: inferring a cause, asserting a refusal that #1133 disproves, and now predicting another command's result. Worth recording because the sentence that keeps failing is the one explaining that the panel must not over-claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth and final pass on one sentence. "not a step the panel skipped" is a claim about the PANEL's internals, and this disclosure is built from the /object_info payload alone — it can say what the server answered, not what the panel did or did not do. Deleted rather than re-argued: the surrounding sentence already carries everything it was there to convey. The full tally on this one sentence, because the pattern is the point: it inferred a cause, then asserted a refusal that #1133 disproves, then predicted what a later refresh would return, then claimed a panel-internal fact. Four rounds, each caught by review rather than by me, in the note whose entire subject is the panel not over-claiming. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
artokun
marked this pull request as ready for review
August 14, 2026 06:30
artokun
added a commit
that referenced
this pull request
Aug 14, 2026
Ships two fixes, and is the first release cut through the repaired changelog path. - #1218 / #1172 — panel_refresh_nodes no longer answers `refreshed: true` while a newly added CheckpointLoaderSimple keeps an empty ckpt_name list. The reapply sweep now rebuilds combo option arrays from the /object_info payload it just fetched instead of relying on a frontend call whose per-node effect the panel never observed, and an authoritative list that comes back EMPTY is disclosed rather than hidden behind a structural "refreshed" verdict. - #1219 — every release since 0.14.31 had written TWO changelog sections for one version. Cause: notes hand-written under a pre-numbered heading, then the generator adding its own; the hand edit landed after the script exited, so no in-script guard could see it. The three duplicated releases are merged (losslessly — verified entry by entry against the original release commits), `today()` now has one UTC source instead of local calendar fields, and a committed test asserts the shipped file's shape so the next occurrence fails on the release PR rather than months later. VERIFIED BY BEING THE FIRST USER OF THE FIX: notes went under [Unreleased], the generator promoted them into exactly ONE dated section, and changelog-integrity.test.mjs passes. Under the old path this commit would have been the fourth duplicate pair. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Works #1172.
panel_refresh_nodesanswersrefreshed: truewhile a freshly addedCheckpointLoaderSimplestill holds an emptyckpt_namelist, so the nextpanel_runfails validation withValue not in list (… not in []).Root cause — two halves, both real
1. Nothing rebuilds the combo option arrays.
reapplyDefsToLiveNodes(lib/asset-staleness.js) stampsctor.nodeDataand reconcilesUNKNOWNwidget names, but never touchesoptions.values— even thoughrefreshComboOptionsFromDefs, which does exactly that, sits ~30 lines above it in the same module and is currently called only from theset_widgetpath. Rebuilding is delegated entirely toapp.refreshComboInNodes(), whose per-node effect the panel never observes, never verifies, and (per #1193) may abandon mid-flight.2.
refreshed: trueis a claim about API calls resolving, not about the definitions being usable. Every inputdescribeNodeDefRefreshtakes is structural — app present, defs obtained, register ran, combo API present, combo resolved.defs.CheckpointLoaderSimple.input.required.ckpt_name[0] === []is in hand at the register and reapply call sites and is discarded. The panel had the evidence and threw it away.Approach
refreshComboOptionsFromDefsfromreapplyDefsToLiveNodes's sweep, so the panel repopulates array-backed combo lists itself from the payload it just fetched — including nodes in subgraphs, whichcollectAllGraphsalready reaches — instead of depending on a frontend call whose effect it cannot see.{type, widget}pairs whose authoritative list came back empty, thread it intodescribeNodeDefRefresh, and surface it as anempty_combo_listsnote alongside the existingrequires_reload/stale_placeholderspair.Two constraints this must not break
refreshedmust staytrueon an empty list. A server with zero checkpoints is a real answer, and panel_set_widget can never write a dynamic combo whose server-side option list is empty (StarNodes Ollama "model") #507/fix(1126): a combo write is decided by whether the panel could READ the option list #1133 show empty lists are sometimes legitimate. Flipping the verdict tofalsewould re-refuse via the verdict what fix(1126): a combo write is decided by whether the panel could READ the option list #1133 deliberately permits via the write path. Disclosure, not failure.defspayload already in hand plus the panel's own synchronous rebuild — never by re-reading widgets afterapp.refreshComboInNodes()resolves. That would make the disclosure depend on the one await The refresh run budget is thin: refreshComboInNodes alone measures ~4.8s of a 9s run #1193 wants to stop waiting for. Doing the rebuild in-panel is in fact a partial answer to The refresh run budget is thin: refreshComboInNodes alone measures ~4.8s of a 9s run #1193.The note must point at the backend — the panel refreshed from
/object_infoand the server itself published an empty list, so the remedy is model paths or a ComfyUI restart — rather than at anotherpanel_refresh_nodes, which is the wrong-remedy failure this repo keeps removing.Work in progress
refreshedrefresh_nodesforwarding whitelist — the panel_refresh_nodes does not rehydrate missing subgraph nodes after classes are registered #981 holeBulk lands in
lib/asset-staleness.js, which no open PR touches; the monolith hunks are two small ones far from every open PR's edit region. Confirmed not to touchlib/object-info-cache.js, so it is parallel-safe with #1178.🤖 Generated with Claude Code