fix(workflow_to_api): widget-aware dynamic-combo expansion + name-aligned control-marker filtering (BE-3370)#558
Conversation
…gned control-marker filtering (BE-3370) A pristine COMFY_DYNAMICCOMBO_V3 template (e.g. Seedream) converted to invalid API JSON: node 1 produced a phantom "model.images": 0 (stealing the seed value) and "seed": "randomize" (unfiltered control marker), which the server rejected (seed: expected INT, got str). Two walks disagreed. _get_widget_name_order expanded a dynamic combo's options into EVERY sub-input name — including connection-only subs like Seedream's COMFY_AUTOGROW_V3 images, which own no widgets_values slot — so every later widget shifted by one. _filter_control_values walked schema inputs positionally WITHOUT dynamic expansion, so when a dynamic combo preceded a control_after_generate seed it checked the companion at the wrong index and the marker survived. Unify both into a single ordered pass, _schema_widget_pairs: expand dynamic combos in place (applying _is_widget_input to each sub-input so connection subs are skipped; recursing for nested dynamic combos) and drop control markers inline as each widget is consumed, so name order and marker filtering can never disagree. Non-dynamic nodes keep identical behavior; unknown/ schema-less nodes fall back to the positional heuristic as before. Add a faithful /object_info fixture for ByteDanceSeedreamNodeV2 and vendor the pristine pro-t2i template; add regression tests for the pristine template and a batch-capable option case (both fail before the fix).
|
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 (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
the
size:L
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…w (BE-3370) Address three cursor-review findings on the widget-aware dynamic-combo walk: - Implicit control_after_generate companion for a seed/noise_seed *sub-input* now matches on the dotted name's leaf segment (model.seed), so its trailing "randomize"/"fixed" marker is stripped instead of shifting later sub-inputs. - An unresolved dynamic-combo selector (value names no option in the current schema — model renamed/removed server-side or version skew) now logs a warning instead of silently misaligning every following widget. - Bound the dynamic-combo recursion at depth 16 so a pathological nested chain from a third-party /object_info degrades to a warning rather than an uncaught RecursionError that aborts the whole conversion. Adds regression tests for each. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
ELI-5
When you run
comfy run --workflowon a fresh "Seedream" template, comfy-clirewrites the visual workflow into the API JSON the server wants. It got two
things wrong on nodes that use the new V3 dynamic combo widget (a dropdown
that reveals different sub-widgets depending on what you pick):
were a knob with a saved value. There's no saved value for it, so every knob
after it slid down one slot — the seed's value ended up in a made-up
model.imageskey.seed didn't account for the dropdown's hidden sub-widgets, so it looked at
the wrong slot and let
"randomize"through as the seed value.Result: the server rejected the job (
seed: expected INT, got str). This PRmakes both passes into one walk that expands the dropdown correctly and
strips the marker in the right place, so a pristine template converts cleanly.
What changed
_collect_widget_inputsnow uses a single_schema_widget_pairswalk when aschema is available. It:
_is_widget_inputtest to dynamic-combo sub-inputs as totop-level inputs, so connection-only subs (e.g. Seedream's
images,COMFY_AUTOGROW_V3) consume nowidgets_valuesslot;control_after_generatemarker inline as each widget is consumed(explicit flag, or an implicit INT
seed/noise_seed), so name-order andmarker-filtering can never disagree.
_get_widget_name_order+_dynamic_combo_sub_inputsare replaced by_schema_widget_pairs+_dynamic_combo_selected_subs(spec-returning)._filter_control_values+_fallback_widget_names.model.size_preset) on thenode's API
inputs; connection subs arrive via the node'sinputsarray asdotted entries (
model.images.image_1), not the widget map.Tests
object_info_bytedance_seedream_v2.json(faithful/object_infoentry built per ComfyUI's
nodes_bytedance.pydefine_schema/_seedream_model_inputsand thecomfy_apiV3 serialization) andseedream_5_0_pro_t2i_ui.json(vendored copy of the pristineapi_bytedance_seedream_5_0_pro_t2i.jsontemplate).TestSeedreamDynamicCombo:model == "seedream 5.0 pro",model.size_preset,model.width/height,seed == 0(int),watermark False,thinking True(schema default), and no
model.imageskey / no"randomize"."seedream 5.0 lite"option →model.max_images == 2,model.fail_on_partial is False,seed == 7,watermark True, drops"fixed", nomodel.images.change).
tests/comfy_cli/test_workflow_to_api.py, 94 tests,incl.
TestDynamicComboAfterControlMarker) andtests/comfy_cli/command/test_run.pypass.
ruff check+ruff format --checkclean under CI-pinned ruff 0.15.15.Notes for review
node, not just dynamic combos. It applies the exact same predicates
(
_is_widget_input,_has_control_after_generate_companion) in the sameschema-section order as the two functions it replaces; I traced it against the
existing control-marker, implicit-seed, force-input, and wildcard tests and it
reproduces identical output (whole suite green).
consumefor a widget advances the value index by≥1 before recursing, and non-widget sub-inputs don't recurse, so depth is
bounded by
len(widgets_values)."seedream 5.0 pro"selector(per the ticket's serialization spec). The local ComfyUI checkout is a newer
node revision where "pro" was renamed and all options share the batch shape;
the fixture is faithful to the version the vendored template was saved from.
change denies no capability; it makes previously-failing conversions succeed
and adds no throw/deny/"not supported" path.