fix(cql): shape-check the definitions/subgraphs containers before reading them - #853
fix(cql): shape-check the definitions/subgraphs containers before reading them#853mattmillerai wants to merge 2 commits into
Conversation
…ding them
`(workflow.get("definitions") or {}).get("subgraphs") or []` only replaces
FALSY values, so a truthy wrong-typed container survived into code that
assumed the right shape: `"definitions": 5` raised `AttributeError` at
`.get`, `{"subgraphs": 5}` raised `TypeError` at the loop, and
`{"subgraphs": "abc"}` was walked character by character.
`comfy workflow slots/set-slot/vary` reach `_subgraph_defs_by_id` before
any of their own guards run, and `slots_cmd`'s `except (ValueError,
KeyError)` catches neither error, so a hand-edited or truncated save
exited the CLI on a rich traceback with NOTHING on stdout — a JSON caller
saw a hard crash instead of a parseable envelope.
Bind the containers and `isinstance`-check each one, mirroring
`_collect_subgraph_defs` in `workflow_to_api.py`. A container that is not
a dict/list now reads as "no definitions": the caller gets an empty index,
which every one of them already handles, rather than a traceback. Same
guard applied to `_count_instances` and `templates._workflow_node_types`,
the other two sites that read the block with the same `or` idiom.
No behavior change for well-formed workflows, and no new error code —
`_load_workflow_or_fail` is untouched.
mattmillerai
added
cursor-review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughChangesWorkflow container validation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Malformed workflow definitions and subgraphs now produce normal empty or domain-error results rather than tracebacks, while valid workflow traversal remains covered. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 2 |
Panel: 6/6 reviewers contributed findings.
| if not isinstance(definitions, dict): | ||
| return {} | ||
| defs = definitions.get("subgraphs") | ||
| if not isinstance(defs, list): |
There was a problem hiding this comment.
🟡 Medium — The new checks stop at the outer subgraphs list, so a definition like {"id": "u1", "nodes": 5} is still indexed; when slots resolves an instance of it, _extract_frontend_slots walks sg.get("nodes") or [] on the scalar and raises TypeError — the same traceback-with-no-envelope crash this PR is meant to eliminate. Either skip defs whose nodes isn't a list here, or guard the downstream walk with isinstance.
Raised by 3 of 6 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max edge-case).
| if isinstance(subgraphs, list): | ||
| for sg in subgraphs: | ||
| if isinstance(sg, dict): | ||
| for n in sg.get("nodes") or []: |
There was a problem hiding this comment.
🟡 Medium — sg.get("nodes") or [] only substitutes falsy values, so a definition carrying "nodes": 5 (or true) still raises TypeError: 'int' object is not iterable on this newly added line. Because _count_instances scans every definition, a single malformed sibling def crashes any interior/promoted set-slot write that reaches _isolate_shared_subgraph; use isinstance(sg.get("nodes"), list) the way _iter_workflow_nodes does.
Raised by 4 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).
| if isinstance(subgraphs, list): | ||
| for sg in subgraphs: | ||
| if isinstance(sg, dict): | ||
| node_lists.append(sg.get("nodes") or []) |
There was a problem hiding this comment.
🟡 Medium — sg.get("nodes") or [] appends a truthy non-list, which then reaches for node in nodes two lines below, so {"nodes": [], "definitions": {"subgraphs": [{"nodes": 5}]}} still raises TypeError — the same defect this hunk fixes, one level deeper — on remote-fetched (cache-poisonable) template JSON that run_template_cmd feeds to _detect_paid_nodes/_enforce_spend_gate without exception handling. _iter_workflow_nodes in this file already guards with isinstance(sg_nodes, list); note the new parametrized test only varies definitions/subgraphs, never a per-subgraph nodes, which is why the gap survives.
Raised by 4 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).
| defs = (workflow.get("definitions") or {}).get("subgraphs") or [] | ||
| definitions = workflow.get("definitions") | ||
| if not isinstance(definitions, dict): | ||
| return {} |
There was a problem hiding this comment.
🟢 Low — The degradation is silent: an unreadable definitions block is dropped with no signal, so slots returns ok: true with every subgraph slot missing, and set-slot reports workflow_slot_invalid ("the address doesn't resolve") as the new test documents — misattributing a corrupt file to a bad address. workflow_print.py already emits "workflow: ignoring non-object definitions block" for this case and the slots payload has a warnings channel, so the two paths should agree rather than one warning and the other hiding it.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
| per-character. | ||
| """ | ||
| defs = (workflow.get("definitions") or {}).get("subgraphs") or [] | ||
| definitions = workflow.get("definitions") |
There was a problem hiding this comment.
🟢 Low — The sweep for this idiom appears to miss CLI-reachable siblings: workflow_ops.capture_recipe still does (workflow.get("definitions") or {}).get("subgraphs") (AttributeError on a truthy non-dict) while comfy workflow capture catches only RecipeError, and workflow_print.render_py sanitizes a non-dict definitions but never checks subgraphs while comfy workflow print catches only PrintUnsupported. Applying the same shape checks there would make the degradation contract hold across commands instead of only for slots/set-slot.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max adversarial).
1 participant
ELI-5
A workflow file can say
"definitions": 5— nonsense, but valid JSON. The code that reads the subgraph definitions out of a workflow usedorto supply a default, andoronly kicks in for falsy values, so a garbage-but-truthy one sailed straight through into code that assumed a dict and a list.comfy workflow slotsthen died on a Python traceback with nothing on stdout, which for a--jsoncaller is indistinguishable from the CLI being broken. This swaps theorfor explicit type checks: a container that isn't the right shape now reads as "there are no definitions here", so the command returns an ordinary empty-result envelope instead of crashing.Description
_subgraph_defs_by_id(comfy_cli/cql/engine.py) read the block with:orreplaces only falsy values, so three wrong-typed shapes survived it:"definitions": 5AttributeError: 'int' object has no attribute 'get'"definitions": {"subgraphs": 5}TypeError: 'int' object is not iterable"definitions": {"subgraphs": "abc"}{}That helper is the first statement of
_extract_frontend_slots, so it runs before any of the calling commands' own validation._load_workflow_or_fail(comfy_cli/command/workflow.py) only checks thatnodesis a list,slots_cmd'sexcept (ValueError, KeyError)catches neither exception, andcmdline.main()has no catch-all — so the CLI exited on a rich traceback with nothing on stdout.The fix binds each container and
isinstance-checks it, mirroring_collect_subgraph_defsincomfy_cli/workflow_to_api.py, which already does exactly this. A container that is not adict/listreads as "no definitions" and the caller gets an empty index — the degradation every caller already handles, since a document with no readable definitions has no subgraph instances to resolve. The per-entryif not isinstance(sg, dict): continueand the id/name indexing are untouched.The same
oridiom appeared at two more sites reading the same block; both get the guard the repo already uses attemplates.py:1245/workflow.py:554:_count_instances(comfy_cli/cql/engine.py)_workflow_node_types(comfy_cli/command/templates.py)Deliberately not changed, per the ticket:
_load_workflow_or_failstays as-is and no new error code is added. Well-formed workflows are unaffected — the checks pass and the code proceeds exactly as before.How has this been tested?
Reproduced on the base commit first, then re-run after the fix, with the exact command from the report:
0343f505){"nodes": [], "links": [], "definitions": 5}AttributeError, no stdoutexit 0,ok: true,count: 0,slots: []{... "definitions": {"subgraphs": 5}}TypeError, no stdoutexit 0,ok: true,count: 0,slots: []{... "definitions": {"subgraphs": "abc"}}{}via a per-character walk{}via the isinstance branchI also exercised the other reachable commands the report names, on the same corrupt inputs.
set-slotandvarycrashed identically on the base commit (AttributeError/TypeError) and now return a normal error envelope for the real problem (workflow_slot_invalid: node 3 not found in workflow).notesandls-nodesalready tolerated these shapes and still do.New automated coverage:
tests/comfy_cli/cql/test_engine.py—TestSubgraphDefsByIdShapeChecks/TestCountInstancesShapeChecks: non-dictdefinitions(int/str/list/float/bool), non-listsubgraphs(int/dict/float/bool), the string case with its intent documented, a top-levelsubgraphswith nodefinitionswrapper, plus positive controls that a well-formed file still indexes by id and by unambiguous name and that non-dict entries beside a real def are still skipped.tests/comfy_cli/command/test_templates.py—_workflow_node_typesover the same corrupt shapes, plus a positive control that interior subgraph node types are still collected.tests/comfy_cli/command/test_workflow_slots.py—TestSlotsCorruptDefinitions: end-to-endworkflow slots, assertingresult.exception is None, exit 0, and a success envelope withcount == 0/slots == []; one test that a corruptdefinitionsblock does not suppress the top-level graph's own slots; and one thatset-slotreaches a domain error rather than a crash.These tests are not vacuous: reverting only the two source files leaves 16 of them failing.
Judgment calls
slots. That is the behavior the report specifies, and_load_workflow_or_failis explicitly out of scope, so no warning is emitted here.comfy workflow printalready surfacesworkflow: ignoring non-object definitions blockfor the same shape, so the "your definitions block is junk" signal does exist elsewhere in the CLI.test_workflow_edit.py"or whereverslots_cmdis exercised";test_workflow_slots.pyis that file, and its convention is a patched graph rather than--input, so the test follows the file. The literal--inputinvocation is covered by the manual run above._count_instancesline and that a rebase might be needed. As of writing, feat(workflow): add node mode overrides #740 is still open and its diff does not touch anysubgraphsline, so there is no overlap today; it does touch the same three files, so a textual conflict on merge order is still possible.isinstance(True, dict)isFalse, so a booldefinitionsreads as "no definitions" rather than as a mapping. Covered by a test; it is the same answer the reference implementation inworkflow_to_api.pygives.Residual
Two of the ten
definitionsreads incomfy_cli/are still unguarded, and both still crash on the same input. I swept everyget("definitions")in the package: 10 sites, 5 already using the explicitisinstancepattern (workflow_print.py,workflow_to_api.py,workflow_ops.py:1143,templates.py:1245,workflow.py:554), 5 using theoridiom. This PR fixes the 3 the report names. The remaining 2 are outside its scope and are not fixed here:comfy_cli/workflow_ops.py:1441incapture_recipe—if (workflow.get("definitions") or {}).get("subgraphs"):comfy_cli/workflow_ops.py:2347incanonical—defs = (w.get("definitions") or {}).get("subgraphs")Verified on this branch: both raise
AttributeError: 'int' object has no attribute 'get'on{"nodes": [], "links": [], "definitions": 5}. Neither is reached byslots/set-slot/vary, which is why they are not in the reported repro; each is a one-line application of the same guard plus one test, and they would leave zero unguarded readers of the block. I did not trace which CLI surfaces reachcapture_recipeandcanonicalwith attacker- or user-supplied JSON, so the severity of those two is unassessed rather than assessed-as-low.Also unexercised: the source investigation's full trace and its findings comment are not readable from this environment, so the crash characterization above rests on my own reproduction against
origin/mainrather than on that record. The 12 call sites inworkflow_ops.pyand the one incql/promoted.pythat the report lists as calling_subgraph_defs_by_idunguarded are fixed transitively by the helper returning{}instead of raising; I exercised that throughslots/set-slot/varyand the unit tests, not through each of the 13 individually.Documentation
None needed — no user-facing surface, flag, or output schema changes. The behavior contract is recorded in the
_subgraph_defs_by_iddocstring.Provenance
ruff check .— All checks passed;ruff format --check .— 447 files already formatted;pytest tests/comfy_cli/cql/test_engine.py tests/comfy_cli/command/test_workflow_slots.py tests/comfy_cli/command/test_templates.py— 387 passed, 0 failed (and 16 failures when the two source files are reverted, confirming the tests bite); fullpytest— 7420 passed, 38 skipped, 1 pre-existing failure unrelated to this change (test_http.py::test_an_unloadable_supplement_falls_through_to_the_platform_roots, which fails identically onorigin/mainwith this branch's source reverted); base-commit reproduction and post-fix re-run of the reported CLI command, and ofset-slot/vary/notes/ls-nodes, on all three corrupt shapes## Residual