fix(cql): type-check autogrow slot keys against their template input - #849
fix(cql): type-check autogrow slot keys against their template input#849mattmillerai wants to merge 1 commit into
Conversation
An autogrow group's grown slot keys (`images.image0`, `model.images.image_1`, …) never entered `port_by_name` — `/object_info` declares the GROUP and never its slots — so the driver loop had no type to check them against: * a link-valued slot key got `dangling_edge` / `output_index_out_of_range` but skipped the `edge_type_mismatch` comparison, because `port_by_name.get(input_name)` was `None`; * a non-link slot key fell out at `if port is None: continue`, before `validate_shape` or `_validate_catalog_value` ever ran. So an `INT` output wired into `BatchImagesNode.images.image0` (template inner type `IMAGE`) validated clean here and was then hard-rejected by the server as `return_type_mismatch` — `execution.validate_inputs` → `validate_node_input` runs against the expanded `template_input`. Synthesize one `Port` per present, template-declared slot key from the group's own template input (`Port.autogrow_slot_port`), at both nesting depths, and consult it where `port_by_name` misses. The slot inherits everything the inner spec declares, so a COMBO template is membership-checked and a ranged INT template is range-checked, and it inherits the existing severities too — `edge_type_mismatch` stays advisory. Keys the template does not declare stay out: `images.bogus` is not an input of the node at all, so it draws no synthesized diagnostic (and nested, keeps reporting `unknown_input`). A template with no inner input declares no type to check against, so it synthesizes nothing — the real shape of `ByteDanceSeedreamNodeV2.model.images` in the captured catalog. The presence-based slot COUNT check is untouched.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![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 7 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 4 |
Panel: 6/6 reviewers contributed findings.
| to check a slot against, so it keeps the pre-existing "no port, no | ||
| diagnostic" behaviour rather than guessing one. | ||
| """ | ||
| names = port.autogrow_required_slot_names(port.autogrow_effective_min) |
There was a problem hiding this comment.
🟡 Medium — autogrow_required_slot_names(autogrow_effective_min) is materialized before anything looks at slots, and autogrow_effective_min is only clamped when autogrow_limits[1] is not None — so a template with a prefix, a huge min and no max/names (e.g. "min": 1_000_000_000) makes this build that many f"{prefix}{i}" strings and hang or OOM validate_workflow. The pre-existing caller _autogrow_below_min_error reached the same expansion only for an output-reachable node with wired slots, whereas this runs once per autogrow group per node from the driver loop, on catalog data fetched from a possibly untrusted /object_info. Add an early if not slots: return {} and clamp the expansion (e.g. to the server's _MaxNames).
Raised by 5 of 6 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-high edge-case).
| # Where the catalog names no slots there is nothing to filter | ||
| # against and the historical prefix match stands. | ||
| declared = agp.autogrow_declared_slot_keys(matched) | ||
| slot_ports.update(_autogrow_slot_ports(base, agp, matched if declared is None else declared)) |
There was a problem hiding this comment.
🟡 Medium — The matched if declared is None else declared fallback synthesizes a Port for every dotted key under the group base when the catalog names no slots — including typos (images.bogus) and deeper keys (images.image0.sub) that the server never places in the expanded schema. For an INT or COMBO template those keys now produce hard shape_mismatch/unknown_enum_value errors that set valid: False, where previously they hit if port is None: continue and were skipped; this is reachable with a template that has an input block but no names/prefix (the case test_declared_slot_keys_are_none_without_a_naming_template pins), and inverts the "don't reject what you cannot read" leniency the sibling autogrow accessors document. The nested twin at line 2850 inherits the same fallback via valid.
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| @@ -249,9 +250,48 @@ def autogrow_element_type(self) -> str | None: | |||
| for spec in section_def.values(): | |||
| type_id = spec[0] if isinstance(spec, (list, tuple)) and spec else spec | |||
| if isinstance(type_id, str) and type_id: | |||
There was a problem hiding this comment.
🟡 Medium — The isinstance(type_id, str) gate skips classic list-form COMBO specs ([["a","b"], {}], where spec[0] is a list) even though _parse_input_spec handles that shape fully, so such a template either yields None (slots silently lose the enum/shape checks this change adds) or — when the section declares more than one inner input — the loop keeps scanning and returns a later input's spec, typing every slot off the wrong input and producing false edge_type_mismatch/shape_mismatch findings. The server's Autogrow._expand_schema_for_dynamic and the sibling autogrow_template_required both take the first value of the first non-empty section; matching that (and accepting any spec _parse_input_spec can read) would keep the accessors consistent.
Raised by 4 of 6 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max edge-case, kimi-k3-high edge-case).
| spec = self.autogrow_template_input_spec | ||
| if spec is None: | ||
| return None | ||
| return _port_from_spec(slot_key, spec, required) |
There was a problem hiding this comment.
🟢 Low — autogrow_template_input_spec accepts any non-empty string type id, including COMFY_AUTOGROW_V3/COMFY_DYNAMICCOMBO_V3, so a template whose inner input is itself a dynamic spec produces a slot Port typed with the internal marker: the driver loop then emits input 'images.image0' expects COMFY_AUTOGROW_V3 but X[0] produces IMAGE (a type no author can wire), and the autogrow_bare_input guard cannot catch it because that branch only tests top-level names. A synthesized dynamic port also never goes through the recursive _check_dynamic_combo_input expansion, so its selector and dotted sub-inputs go unvalidated. Returning None for such a spec keeps the malformed-catalog case on the existing "nothing to check against" path.
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max edge-case).
| spec = self.autogrow_template_input_spec | ||
| if spec is None: | ||
| return None | ||
| return _port_from_spec(slot_key, spec, required) |
There was a problem hiding this comment.
🟢 Low — autogrow_template_input_spec's guard accepts tuple specs (isinstance(spec, (list, tuple))) but _parse_input_spec only understands str and list, so a tuple inner spec falls into the not isinstance(spec, list) branch and yields a Port typed "UNKNOWN". Since UNKNOWN is not a wildcard, every correctly-wired slot of such a group gets a false edge_type_mismatch while autogrow_element_type reports the real type for the same template — the two accessors disagree. Only reachable when object_info is built in Python rather than JSON-decoded, but the fix is small: normalize the tuple to a list here, or teach _parse_input_spec about tuples.
Raised by 2 of 6 reviewers (kimi-k3-high adversarial, claude-opus-5-thinking-max edge-case).
| required_keys = set() if names is None else {f"{field}.{name}" for name in names} | ||
| out: dict[str, Port] = {} | ||
| for key in slots: | ||
| slot_port = port.autogrow_slot_port(key, required=key in required_keys) |
There was a problem hiding this comment.
🟢 Low — One Port is now built per slot key, and each _port_from_spec -> _parse_input_spec call copies the template's whole enum (list(options)) into that Port; a resulting unknown_enum_value finding then embeds another full copy plus a suggest_combo difflib pass over the option list. The slot-key count is bounded only by the workflow file (and, with no max in the template, by the 6-digit index filter), so many bogus slot values against a COMBO-template group turn O(n) input into O(n x options) time and memory where before these keys produced no Port at all. Parse and share the immutable template metadata once, and/or cap the ports/findings synthesized per group.
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).
| warnings: list[dict] = [] | ||
| present = node_inputs | ||
| dynamic_ports = [p for p in m.inputs if p.is_dynamic_combo] | ||
| if not dynamic_ports: |
There was a problem hiding this comment.
🟢 Low — _check_dynamic_combos now returns a 5-tuple (both this early return and the final one, and the docstring was updated), but its return annotation still reads tuple[list[dict], list[dict], set[str], set[str]]. A type checker will therefore flag the caller's 5-way unpack at line 1824 instead of the stale signature.
Raised by 3 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-high edge-case).
1 participant
STACKED — merging lands on
matt/be-10633-autogrow-min-slots(owned by @mattmillerai, PR #842), NOTmain. Review/merge #842 first; GitHub will retarget this PR tomainonce it lands.ELI-5
A node like
BatchImagesNodesays it has one input calledimages, but you actually wireimages.image0,images.image1, … one key per connection. The validator only knew about the group, never the individual slots — so it had no idea what type a slot expects. Wire a number into an image slot andcomfysaid "looks fine", then the server rejected the whole run. This teaches the validator to look up the slot's type in the group's own template, so a slot gets checked exactly like any other input.What was wrong
Autogrow slot keys never entered
port_by_nameinGraph.validate_workflow(port_by_name = {p.name: p for p in m.inputs}—/object_infodeclares the GROUP and never its grown slots), so both port lookups in the driver loop missed:dangling_edge/output_index_out_of_range, but skipped theedge_type_mismatchcomparison entirely, becauseport = port_by_name.get(input_name)wasNone;if port is None: continue, beforevalidate_shapeor_validate_catalog_valueever ran.Reproduced against the captured production catalog (
tests/comfy_cli/fixtures/object_info_nested_autogrow.json), on this PR's base and after it —LoadImage[1]isMASK,BatchImagesNode.images' template input isIMAGE:The server does not agree with the "before" line:
execution.validate_inputs→validate_node_inputruns against the expandedtemplate_input, so it hard-rejects the prompt asreturn_type_mismatch. Validation was giving false confidence right before a paid run.What this does
Port.autogrow_template_input_spec— the single innerINPUT_TYPESspec the group's schematemplatedeclares, factored out ofautogrow_element_typeso both read the template exactly the same way (declaration order, mirroringAutogrow._expand_schema_for_dynamic'sinput.items()walk).autogrow_element_typekeeps its behaviour verbatim; it now derives the type id from that spec instead of re-walking.Port.autogrow_slot_port(slot_key, *, required)— aPortfor one grown slot, parsed from that inner spec via the same_port_from_specevery other input goes through. The slot inheritsenum_values/enum_declared(so a COMBO template is membership-checked) andoptions(so a rangedINTtemplate is range-checked).Nonefor a non-autogrow port or a template with no inner input._autogrow_slot_ports(field, port, slots)— one synthesized port per present slot key, shared by the top-level driver and the nested path so a slot is typed identically at every depth.requiredper slot is the server's own answer — the names it places in the expanded schema'srequiredsection (autogrow_required_slot_namesoverautogrow_effective_min), not the firstminkeys the prompt happens to wire — andFalsewhere the catalog names no slots.slot_portsper node (top level: present keys underf"{base}.", filtered throughPort.autogrow_declared_slot_keyswith the historical prefix match as the fallback; nested: merged from the dynamic-combo walk, the only pass that knows which option expanded them), and bothport_by_name.get(input_name)lookups becomeport_by_name.get(input_name) or slot_ports.get(input_name).edge_type_mismatchstays advisory,unknown_enum_value/below_min/shape_mismatchare the same hard errors any declared input gets, andbelow_minstays gated on output-reachability like every other range check.images.bogusis not an input of the node — the server never places it in the expanded schema — so no port is synthesized and it draws no diagnostic; nested,model.images.boguskeeps reportingunknown_input(fix(cql): enforce the server's autogrow minimum slot count in validation #842's behaviour, preserved)._autogrow_slot_errors/_check_autogrow_requiredare unchanged; presence-based counting stays the server-parity behaviour.Chesterton's-fence note on the two
orlookupsBoth
port_by_name.get(...)calls are broadened with an||, which is the shape that hides regressions, so:port_by_namereturnsNoneexactly for names/object_infodoes not declare, and slot keys are precisely that set.Portis a plain dataclass with no__bool__/__len__, so a real port is always truthy and theorcan never fall through toslot_portswhen a declared port exists —port_by_namekeeps precedence unconditionally.slot_portsis only ever keyed by<autogrow base>.<a name the template declares>.Corpus sweep — the half this does NOT change
Swept every
object_infocapture undertests/for autogrow groups (top-level and dynamic-combo-nested) and tabulated what their templates declare, since that is what decides whether a group gains a new check at all:IMAGEVIDEOAUDIOSTRING36 groups over 6 files, 7 distinct nodes carrying an
IMAGEtemplate. The 4 with no inner input are allByteDanceSeedreamNodeV2.model.images, the real captured shape behind the "template with no inner input" test. Zero of the 36 declare an inner input while carrying neithernamesnorprefix— the one shape where theautogrow_declared_slot_keysfallback would synthesize a port for a key the group may not actually grow — so that path has no live false-positive in the captured corpus.Consequence worth stating plainly: no captured catalog declares a
COMBOor ranged-INTautogrow template, so the new catalog/range checks on slot values are exercised only by the synthetic fixtures inTestAutogrowSlotPorts. They are forward-looking against a template shape the server supports but the current partner catalogs do not use.Tests
tests/comfy_cli/cql/test_engine.py::TestAutogrowSlotPorts(synthetic, 14 tests) — INT source into an IMAGE slot → oneedge_type_mismatchonimages.image0; correctly wired IMAGE → clean;images.bogus→ no synthesized diagnostic, no crash; dangling slot edge keeps its existing error; COMBO template +"zzz"→unknown_enum_value; COMBO template +"a"→ clean; ranged INT template +0→below_min; INT template +"five"→shape_mismatch; template with no inner input → nothing synthesized; plus unit tests forautogrow_slot_port, the inherited enum/options, and therequiredflag following the server's required slot NAMES.tests/comfy_cli/cql/test_nested_autogrow.py(real captured catalog, 6 tests) — the same top-level and nested cases againstBatchImagesNode.images.image0andGeminiNanoBanana2V2.model.images.image_1, includingmodel.images.bogusstill reportingunknown_input.Residual
object_info_nested_autogrow.jsonis documented as the production catalog verbatim and contains no INT-producing node, so adding one would break that contract. The captured-catalog tests wireLoadImage[1](MASK) into theIMAGEslot instead; the literal INT→IMAGE case is covered on the synthetic fixture inTestAutogrowSlotPorts. Both exercise the identical code path (_edge_types_compatible(src_type, dst_type)with no union overlap).test_dotted_slots_validate_cleanandtest_wired_slot_keys_are_validwere not extended. Both drive fixtures whose autogrow groups are deliberately template-LESS (["COMFY_AUTOGROW_V3", {}]— thetest_engine.pyBatchImagesNodecarries a comment pinning that on purpose), so no port is synthesized for their slot keys and an added assertion would assert nothing about this change. The correctly-wired-clean criterion is covered bytest_correctly_wired_slot_stays_cleanandtest_top_level_correctly_wired_slot_stays_clean/test_nested_correctly_wired_slot_stays_cleaninstead. Both original tests still pass unmodified, which is the regression evidence that a template-less group's behaviour is unchanged.Port.validate_shapestill has no branch for link-typed ports, so{"images.image0": "not-a-link"}continues to validate clean after this change — a plain string against anIMAGEslot falls through every shape branch. This is the known follow-up "cql: report a non-link value on a link-typed input (link_expected)" and is deliberately out of scope here; it is not introduced by this PR (the same hole exists today for every declared link input).edge_type_mismatchon a slot key is advisory, not an error, by design — slot keys inherit the existing severities and the promotion of edge mismatches to hard errors is tracked separately. So the exact case in the repro above still reportsvalid: True; what changes is that the user now sees the warning instead of nothing.execution.validate_inputs→validate_node_inputon the expandedtemplate_inputrejecting asreturn_type_mismatch) is carried from the originating investigation and verified only against the captured/object_infocatalogs in this repo, not against a running server from this environment.tests/comfy_cli/test_http.py::test_an_unloadable_supplement_falls_through_to_the_platform_rootsfails, and fails identically on this PR's base branch with the change stashed — pre-existing and unrelated to CQL.Provenance
uv run pytest tests/comfy_cli/cql -q: 580 passed, 0 failed;uv run pytest -q(full suite): 7447 passed, 38 skipped, 1 failed (test_http.py::test_an_unloadable_supplement_falls_through_to_the_platform_roots, pre-existing on the base branch — confirmed by re-running it with the change stashed);uv run ruff check .: all checks passed;uv run ruff format --check .: 446 files already formatted; before/after repro againstobject_info_nested_autogrow.jsonas quoted above.main, the first option the plan allows while fix(cql): enforce the server's autogrow minimum slot count in validation #842 is open — the plan'sPort.autogrow_declared_slot_keys/autogrow_required_slot_names/autogrow_effective_minall originate there. (2) The plan saysautogrow_slot_portshould take "the first spec inrequiredthenoptional"; it takes the first spec in declaration order instead, which is what the plan's own qualifier ("the same walkautogrow_element_typedoes") resolves to on fix(cql): enforce the server's autogrow minimum slot count in validation #842's branch, and what the server does. (3)_check_dynamic_combo_input/_check_dynamic_combo_subsurface the nested slot ports through a mutated accumulator argument rather than a fifth tuple element, matching theresolvedaccumulator already threaded through those exact functions;_check_dynamic_combosdoes return it as a fifth element, since the driver is its only caller. (4) The two named clean-wiring tests were not extended — see Residual.