fix(cql): expand dynamic-combo sub-inputs in slot extraction and set-slot/vary write path (BE-3371) - #574
fix(cql): expand dynamic-combo sub-inputs in slot extraction and set-slot/vary write path (BE-3371)#574mattmillerai wants to merge 2 commits into
Conversation
…slot/vary write path (BE-3371) COMFY_DYNAMICCOMBO_V3 ports were treated as connections and dropped from widget_order, so on a pristine Seedream template every slot after the selector was mislabeled (1.seed showed the model name) and set-slot/vary wrote into the wrong widgets_values positions (1.seed=42 silently overwrote the model selector). - parse dynamic combos as widget ports: enum = option keys, raw option dicts retained for expansion - add value-aware Graph.widget_order_for_node that expands the selected option's widget sub-inputs in place (model.size_preset, model.width, …), recursing for nested combos and skipping connection subs (AUTOGROW) - migrate _node_widget_slots, _resolve_proxy_value, and _write_widget to the value-aware order; slots now surface N.model + N.model.<sub> with types/enums/current values - set-slot N.model=<other key> rebuilds the sub-widget roster from schema defaults, preserving values before/after the combo, with a warning - an N.model.<sub> address not under the current selector warns (no write) and lists the valid addresses Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughWalkthroughThe engine now parses V3 dynamic-combo metadata, expands widget slots based on selected values, resolves proxy indices accordingly, and supports validated sub-input writes and selector-driven roster rebuilding. Tests cover dynamic-combo workflows, nested options, warnings, and Seedream fixtures. ChangesDynamic combo workflow editing
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Graph
participant Workflow
CLI->>Graph: resolve dynamic-combo widget slots
Graph->>Workflow: read selected widget values
Workflow-->>Graph: return current selector values
Graph-->>CLI: return expanded slot addresses
CLI->>Workflow: write selector or sub-input
Workflow-->>CLI: return updated values and warnings
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've verified the findings against the actual code. Key confirmations: `_dynamic_combo_sub_ports` returns `[]` on unknown selectors (line 1421); the repo's own `workflow_to_api._has_control_after_generate_companion` handles implicit `seed`/`noise_seed` markers that `engine.py`'s expansion ignores; the `extend` flag is not checked before padding on selector change; and the fixtures use explicit `control_after_generate`.
[
{
"file": "comfy_cli/cql/engine.py",
"line": 1421,
"side": "
Re-trigger by removing and re-adding the cursor-review label.
|
This PR currently conflicts with Please rebase (or merge |
…semantic conflict with #617 Both branches independently added dynamic-combo option expansion to Port (this branch: `dynamic_options`, main's #617: `raw_spec`) — kept both fields since main's validate-path (`_check_dynamic_combos`) and this branch's slot-extraction/write-path (`_expand_widget_entries`) are disjoint consumers. Fixed the resulting arity break in `_check_dynamic_combo_sub`, which still unpacked `_parse_input_spec`'s old 4-tuple after this branch changed it to a 5-tuple. Also fixes two real bugs the Cursor review panel flagged (its JSON output hit a parse error, but the underlying findings were valid): - `_expand_widget_entries`/`_dynamic_combo_default_values` only recognized the schema's explicit `control_after_generate` flag, missing the implicit seed/noise_seed companion marker that `workflow_to_api._has_control_after_generate_companion` already handles for the UI→API converter — a dynamic-combo sub-input named `seed`/`noise_seed` would misalign every widget slot after it. - `_write_dynamic_combo_selector`'s roster-rebuild branch padded the widgets array to reach a combo's index without checking `extend`, silently overriding a caller's request not to pad. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Rebased on `main` and resolved the conflict — thanks for flagging it, @bigcat88. The conflict was semantic, not just textual: #617 (`main`) and this branch independently added dynamic-combo option expansion to `Port` for different consumers — #617's validate path (`raw_spec`) vs. this branch's slot-extraction/write path (`dynamic_options`). Kept both fields since they're used by disjoint code, and fixed the resulting arity break in `_check_dynamic_combo_sub`, which still unpacked `_parse_input_spec`'s old 4-tuple after this branch changed it to 5. Also addressed the two real findings buried in the Cursor review's parse-error output:
Added regression tests for both; full suite (3665 passed) and ruff (pinned to the CI version, 0.15.15) are green. |
ELI-5
ComfyUI has "shape-shifter" dropdowns: pick a model in the dropdown and the node grows extra knobs (size, width, height…) that belong to that model. The CLI didn't know these dropdowns existed, so when it counted a node's knobs it skipped the dropdown and its extra knobs — every knob after it got the wrong label, and
set-slotturned the wrong knob (setting the seed actually overwrote the model choice, silently). Now the CLI reads which model is picked, counts that model's knobs in their real positions, and when you switch models it swaps the knob set for the new model's defaults and tells you it did.Problem
On the pristine
api_bytedance_seedream_5_0_pro_t2itemplate (reproduced onorigin/main):comfy workflow slotsshowed1.seed='seedream 5.0 pro',1.watermark/1.thinking=2048, and no1.model/1.model.*slots at allapply_slots({"1.seed": 42})wrote index 1 — silently clobbering the model selector — with zero warningsRoot cause:
_is_linktreatedCOMFY_DYNAMICCOMBO_V3as a connection (dropped fromwidget_order), andwidget_orderwas value-independent while the frontend inlines the selected option's widget sub-values into the flat positionalwidgets_valuesright after the selector.Fix
COMFY_*COMBO*type is a widget port. Its enum = the optionkeystrings; the raw option dicts are retained onPort.dynamic_optionsfor expansion._is_scalar_choicestill rejects dict options for ordinary combos.Graph.widget_order_for_node(class_name, widgets_values): value-aware order — at a dynamic combo it reads the selector from its own positional slot and expands the matching option's widget-like sub-inputs as<name>.<sub>(same widget-vs-link test as top level; nested dynamic combos recurse with a depth cap; connection subs likeCOMFY_AUTOGROW_V3contribute no slot;control_after_generatemarkers follow control-flagged inputs, sub-inputs included).widget_order(class_name)is kept for callers without node context._node_widget_slotsemitsN.model(typeCOMFY_DYNAMICCOMBO_V3,enum= option keys, current = selector) plusN.model.<sub>slots with correct types/enums/current values. Slots with enum choices now carry an additiveenumkey.set-slot/vary):N.model.<sub>=vwrites the correct position, validated against the sub-spec.N.model=<other key>rebuildswidgets_valuesfor that node: values before the combo kept, new selector written, the new option's widget sub-inputs filled from schema defaults (sub-specdefault, first enum option for combos), trailing values (seed/marker/watermark/thinking) re-aligned — and emits adynamic_combo_roster_rebuiltwarning describing the rebuild.N.model.<sub>that doesn't exist under the current selector →unknown_dynamic_sub_inputwarning, no write, listing the valid addresses and how to switch rosters. This denial is schema-grounded, not assumed: under theproselector the frontend serializes nomax_imagesslot (the vendored pristine template's 8-value layout confirms it), so the write has no position to land in — the warning redirects to the valid addresses instead of dead-ending.N.model=<unknown key>→ hard error listing valid options (an unknown selector has no roster to build; writing it would corrupt the positional alignment of everything after — deliberate deviation from ordinary combos, which warn-and-write)._resolve_proxy_value(curated subgraph proxy reads) migrated to the value-aware order as well.Judgment calls
workflow_to_api). I kept them separate — the converter operates on rawINPUT_TYPESspecs, the engine on parsedPorts, and a cross-module import would couple the CQL engine to the converter's internals. The rules are aligned (_is_dynamic_combo_typemirrors_is_widget_input'sCOMFY_*COMBO*test) and both sides are pinned by tests on the same fixtures.size_presetlist ≠ pro's), so defaults are the safe deterministic choice; the warning tells the agent what was reset.test_slot_address_with_dotted_input_name) to monkeypatchwidget_order_for_nodeinstead ofwidget_order— its intent (first-dot address split) is unchanged.Tests
tests/comfy_cli/command/test_workflow_slots.py: pristine-template slots (addresses/types/enums/currents,1.seedcurrent is0not the model name), seed write doesn't clobber the selector, sub-input write hits index 2, selector change to lite rebuilds the roster (+2 slots,max_images/fail_on_partialappear, prompt/seed/watermark preserved, warning emitted, new roster addressable), unknown sub-address warns without mutating, unknown selector rejected,varyover a sub-input, autogrow subs not exposed.tests/comfy_cli/cql/test_engine.py: dynamic combo parses as widget with option-key enum; value-independent vs value-aware order; nested-combo recursion; unknown selector expands nothing; nested + outer selector-change rebuilds.Verification
uv run pytest tests/comfy_cli/command/test_workflow_slots.py tests/comfy_cli/cql/ tests/comfy_cli/test_workflow_to_api.py— 285 passeduv run pytest tests/— 2653 passed, 2 failed:test_validate_command.py::test_api_format_unchanged+::test_empty_dict_payload_unchangedfail identically on cleanorigin/main(pre-existing: the BE-3357prompt_no_outputshard error now rejects those tests' output-node-less fixtures) — unrelated to this diffruff checkclean on all touched files;ruff formatapplied (the 15 repo-wide UP038 findings also pre-exist onorigin/mainunder the local ruff)