feat(validate): enum-check COMFY_DYNAMICCOMBO_V3 selections + presence-check dotted sub-inputs (BE-3358)

[mattmillerai]

ELI-5

Some cloud nodes (ClaudeNode, ReveImageCreateNode, …) have a "pick one" input like model where the pick brings its own extra settings (model.max_tokens, model.temperature). Until now comfy validate ignored all of that completely: you could pick a model that doesn't exist and pass garbage settings, and validate would say everything is fine — then the server would reject it. Now validate checks the pick is real, checks the picked option's required settings are present and in range, and warns about settings the server would ignore.

What

COMFY_DYNAMICCOMBO_V3 inputs validated end-to-end in comfy_cli/cql/engine.py, mirroring the server (_io.py DynamicCombo.Option.as_dict / _expand_schema_for_dynamic → the required_input_missing presence check in execution.py:884-900):

• Parse: each option's {"key", "inputs": {"required", "optional"}} sub-schema is parsed into new Port.selection_keys / Port.dynamic_options via the existing _parse_inputs machinery, so nested dynamic combos (model.mode.budget) recurse naturally. Malformed options (non-dict, missing/non-string key, non-dict inputs) are skipped.
• Validate (_check_dynamic_combo / _expand_dynamic_port):
  • selection not a known key → hard unknown_enum_value carrying the full valid_options list (same shape as the existing enum error);
  • required dotted sub-inputs of the selected option absent → required_input_missing per key (same shape as the phase-1 path);
  • present dotted sub-values run validate_shape / validate_catalog exactly like top-level ports (shape_mismatch, unknown_enum_value, below_min/above_max route to errors per BE-3357);
  • required dynamic port entirely absent → required_input_missing (phase 1 deliberately skips dynamic types, so this pass owns it);
  • dotted keys matching no sub-port of the selection → unknown_input warning (server ignores extra keys);
  • link-valued (2-list) selections and sub-values are skipped defensively — the generic loop already edge-checks them.
• Discovery: nodes show / describe output now includes selection_keys for dynamic-combo ports; choices stays [] (selection keys are not flat enum choices — _is_scalar_choice semantics untouched).

Both BE-3349 repros now fail correctly: {"model": "Opus 4.6"} → required_input_missing on model.max_tokens/model.mode; {"model": "NotARealModel", "model.bogus_key": 5} → unknown_enum_value on model.

Judgment calls

• Unknown-dotted-key warnings are suppressed when the base selection is absent/invalid/link-valued — the sub-keys of an unresolved selection can't be judged, and the primary error already tells the agent what to fix; warning on model.bogus_key while also erroring model itself would be pile-on noise. Once the selection is fixed, a re-validate flags the bogus key.
• Unparseable options degrade to the old lenient behavior (no selection check at all) rather than false-erroring on a schema we can't read — the validator only becomes strict where the option schema genuinely parsed. This bounds the false-positive risk of the new hard errors to cases where the server-side option expansion would also fail.
• Sub-port presence checks mirror _check_required_present's exclusions (autogrow, COMFY_DYNAMICSLOT) to avoid double-error shapes.

Not touched (pre-existing on main)

tests/comfy_cli/command/test_validate_command.py::test_api_format_unchanged and ::test_empty_dict_payload_unchanged fail on origin/main itself (verified on a pristine checkout) — a semantic conflict between #551 (BE-3357 prompt_no_outputs hard error) and #553 (BE-3359 tests that assume outputless workflows exit 0). #565 (BE-3406) is already working in exactly that area, so this PR leaves it alone.

Tests

New fixture tests/comfy_cli/fixtures/dynamic_combo_object_info.json (BE-3349-shaped synthetic node: two options with different required sub-inputs incl. INT min/max and an enum, one option nesting a second dynamic combo) + 15 tests in TestDynamicComboInputs covering every case above, both BE-3349 repros, nested selections, malformed options, link-valued selections, and describe output. Existing autogrow tests unchanged.

pytest: 2653 passed on this branch minus the 2 pre-existing main failures above; ruff check + ruff format --check clean.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 50 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 09788478-bfff-4832-a9a1-729005095fa8

📥 Commits

Reviewing files that changed from the base of the PR and between 98325c9 and ad7ed66.

📒 Files selected for processing (3)

• comfy_cli/cql/engine.py
• tests/comfy_cli/cql/test_engine.py
• tests/comfy_cli/fixtures/dynamic_combo_object_info.json

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3358-dynamic-combo-validation

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3358-dynamic-combo-validation

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 5 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	2
🟢 Low	1
⚪ Nit	1

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

[mattmillerai]

Pushed 153a5ab addressing all five cursor-review panel findings (autogrow sub-ports under dynamic options, recursion-depth cap, stale sub-key edge-check false positives, hint truncation, deepest-prefix stray-key attribution) — each thread has details and dedicated tests.

Note on the build CI failure: the two failing tests (test_api_format_unchanged, test_empty_dict_payload_unchanged) are red on main itself — a semantic conflict between #551 (BE-3357, prompt_no_outputs hard check) and #553 (BE-3359, validate tests that use output-less workflows), which merged independently. This push repairs them here since this PR touches the same validator: the API-format test gains a SaveImage output node, and the empty-dict test now expects the (correct) prompt_no_outputs rejection. Full suite: 2663 passed locally; ruff 0.15.15 (CI pin) clean.

[mattmillerai]

Heads-up: #576 just shipped the two test_validate_command.py fixture fixes standalone (they're what's breaking build CI repo-wide on main, tracked separately). Once it merges, this branch will need a small rebase in that file — keep #576's version of test_api_format_unchanged and the renamed test_empty_dict_payload_not_converted_and_rejected; the intent is identical.

[mattmillerai]

Self-review passed at accd96c (post-conflict-resolution HEAD): fresh-eyes review of the full diff confirms the 5 previously-flagged Cursor findings are genuinely fixed (not superficial), no regressions to phase-1 validation, recursion cap fails safe, fixture/test shapes match the real COMFY_DYNAMICCOMBO_V3 schema convention already used elsewhere in the file. All CI green, all review threads resolved. Ready for merge review.

[bigcat88]

This PR currently conflicts with main — GitHub reports mergeable: CONFLICTING, so it needs a rebase before I can review it and I'm skipping it in the current review sweep.

Please rebase (or merge main in) and I'll pick it up on the next pass. main moved a fair bit in the last day, including #614 (ANSI sanitisation across the pretty-print call sites) and #628 (the duplicate server_died error-code fix that had main red), so a refresh may also clear unrelated CI noise on this branch.

[mattmillerai]

Rebased and reconciled a semantic conflict: main independently shipped BE-3777 (#617, "expand dynamic-combo options so validate matches the server") after this branch diverged — both PRs implement the same feature (COMFY_DYNAMICCOMBO_V3 option expansion) with different architectures. Textually they conflicted in comfy_cli/cql/engine.py; semantically I had to pick one core and graft the other's genuinely-additive pieces on top rather than take either side wholesale.

Kept main's architecture (lazy Port.raw_spec resolution, _check_dynamic_combos/_check_dynamic_combo_input/_check_dynamic_combo_sub) as the base, since it's already shipped and — critically — its lenient treatment of an autogrow sub-input nested in a dynamic-combo option (no slot-count enforcement) is backed by a captured production fixture (ByteDance Seedream's model.images, declared required with an effective min: 0, legitimately emitting zero slot keys). This PR's original stricter check (autogrow_no_slots/autogrow_bare_input for that case) would have been a false positive on real traffic, so I did not port it forward — see the reply on the "High" Cursor finding below for the full rationale.

Ported this PR's genuinely additive pieces onto main's architecture:

• unknown_input warnings for a present dotted sub-key that matches no sub-input of the resolved selection (deepest-resolved-prefix attribution included), plus the pre-loop exemption so a stale/link-valued stray sub-key doesn't false-dangling_edge in the generic edge-check loop.
• nodes show / describe output now exposes selection_keys for dynamic-combo ports (derived from raw_spec via main's _dynamic_combo_options, since the eager Port.selection_keys field this PR added doesn't exist in the merged architecture).
• The required_input_missing hint truncation for combos with many options was already present on main's side.

Test suites from both PRs are reconciled in tests/comfy_cli/cql/test_engine.py: main's TestValidateDynamicCombo (real Seedream-fixture coverage) is untouched, and this PR's TestDynamicComboInputs/TestDynamicComboAutogrowSub are adapted — a few tests that asserted on the now-removed eager Port.selection_keys internal or the reverted strict autogrow-slot check were rewritten to assert through the public validate_workflow/morphism_to_dict API instead, matching the reconciled behavior.

Full suite: 3669 passed, 37 skipped, 0 failed. ruff check/ruff format --check clean on all touched files (pre-existing UP038 isinstance-tuple lint noise in unrelated/inherited code, unaffected by this change).

[skishore23]

Strong PR overall — both BE-3349 repros behave exactly as specified, and it merges cleanly with 195 passing. But I found a case where the code doesn't implement a guarantee the description makes, and it's a forward-compatibility hazard. Small fix.

The stated leniency guard isn't implemented

From the description:

Unparseable options degrade to the old lenient behavior (no selection check at all) rather than false-erroring on a schema we can't read — the validator only becomes strict where the option schema genuinely parsed.

That's the right design. But when zero options parse, the empty key set is treated as authoritative and the selection is hard-rejected. Run against the PR's own fixture with a valid selection ("Opus 4.6"):

options = "garbage"    -> unknown_enum_value: 'Opus 4.6' not in 0 known options for model
options = [1, 2, 3]    -> unknown_enum_value: 'Opus 4.6' not in 0 known options for model
options = []           -> unknown_enum_value: 'Opus 4.6' not in 0 known options for model

"not in 0 known options" is self-refuting — it's the validator saying it had no basis to judge and rejecting anyway. Contrast with a partially malformed schema, where leniency does work correctly:

options = [{"key": "A", "inputs": "not-a-dict"}], sel="A"   -> []   ✓ lenient

So the guard holds for a malformed individual option but not for a malformed options container.

Why it matters beyond the synthetic case: this guard exists for forward compatibility — a newer ComfyUI shipping a COMFY_DYNAMICCOMBO_V3 option shape this parser doesn't recognize. When that happens, options parses to zero entries and every workflow using that node hard-fails validation, blocking comfy run preflight on a graph the server would happily execute. That's precisely the failure mode the paragraph above promises to avoid, and it's the one that shows up on a server upgrade rather than in tests.

Fix — guard before the option is None branch (engine.py:1353):

if not keys:
    # No option schema parsed (absent/unreadable `options`) — we have no basis to
    # judge the selection, so stay lenient rather than hard-erroring on a schema we
    # can't read. Matches the "only strict where the schema genuinely parsed" rule.
    return errors, warnings, set(), {f"{name}."}

Worth a test pinning options=[] / options="garbage" → valid: True, since the current suite only covers malformed entries.

Secondary: valid_options can contain null

An option dict missing key is counted but contributes None:

options = [{"inputs": {}}]  ->  "not in 1 known options"
                                valid_options=[None]  suggestions=[None]

valid_options / suggestions are agent-facing "here's what to pick" lists, so a JSON null in them is worse than an empty list — an agent may well try to set the field to null. The count (1) also disagrees with the number of matchable keys (0). Filtering to k is not None when building keys fixes both, and folds into the guard above.

What's verified and good

• Clean merge with origin/main; 195 passed (cql/ + command/test_validate_command.py).
• Both BE-3349 repros are exactly right, run against the real validator:

  {"model": "Opus 4.6"}                          -> required_input_missing on model.max_tokens, model.mode
  {"model": "NotARealModel", "model.bogus_key":5} -> unknown_enum_value on model, and NO warning on model.bogus_key
  

  The second confirms your judgment call about suppressing sub-key noise when the base selection is unresolved — that's the right call, and it's genuinely implemented rather than just described.
• No malformed input crashes. I threw non-list options, non-dict entries, missing key, and non-dict inputs at it — every one returns a structured result rather than raising.
• The description's "2 pre-existing main failures" caveat is stale in your favour — test_api_format_unchanged and test_empty_dict_payload_unchanged both pass on the merged branch now (#565 landed in that area). Drop that section from the squash message.

Happy to approve once the empty-keys guard is in.

[mattmillerai]

Pushed 033e5fe addressing the changes-requested review: the empty-keys guard now returns lenient (no error) when the options container itself doesn't parse (absent/garbage/non-list options, or every entry missing key), matching the leniency already applied to a malformed individual option entry. Also filtered None out of the matchable key set so a missing key can no longer leak into the agent-facing valid_options/suggestions lists. Added test_unparseable_options_container_stays_lenient and test_option_missing_key_excluded_from_valid_options covering both.

Also merged main in (0fa5202) — main had moved since the earlier merge (notably #695, which touches the same cql/engine.py region for nodes path source-type constraints); auto-merged cleanly with no conflicts, and the full suite (4548 passed, 36 skipped) plus ruff check/ruff format --check are clean on the merged tree.

Ready for re-review.