refactor(cql): remove vestigial load_graph fetch stack from loader.py (BE-4364) - #598
refactor(cql): remove vestigial load_graph fetch stack from loader.py (BE-4364)#598mattmillerai wants to merge 3 commits into
Conversation
…_server stack (BE-4364) The loader's private fetch stack (load_graph, its own _load_from_file, and _load_from_server) is dead: every production caller (command/nodes.py, command/workflow.py, command/workflow_fragments.py) reaches object_info via resilient_load_object_info, which delegates to the engine's loaders. The loader's stack was reached only by its own tests and the cql/__init__.py export. Delete that block (through the # ---- normalization ---- divider), the now-orphaned _LOADER_OPENER and MAX_INPUT_BYTES, and the imports they used (NoRedirectHandler, is_loopback_host, urllib.*). Rewrite the module docstring to describe what remains: normalize and resilient_load_object_info. Re-export resilient_load_object_info as the public cql entry point in place of load_graph. Drop the tests that covered the removed stack; keep all normalize tests. The engine's live loaders and resilient_load_object_info are untouched — they keep their own loopback guard, no-redirect opener, byte cap, and cloud HTTPS+auth. This removes the security-policy fork (256 MiB vs 64 MiB byte caps, duplicate SSRF guards) flagged in BE-4352.
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 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.
✅ No high-signal findings.
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…oader-fetch-stack # Conflicts: # comfy_cli/cql/loader.py
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
Requesting changes — one missed reference, and CI is already showing it.
Blocking: deleting _LOADER_OPENER breaks a test file this PR doesn't touch
build is red, and it's not flaky infrastructure — it fails at collection, so the entire suite is skipped:
ERROR collecting tests/comfy_cli/test_http_only_openers.py
("cql.loader._LOADER_OPENER", loader._LOADER_OPENER),
E AttributeError: module 'comfy_cli.cql.loader' has no attribute '_LOADER_OPENER'
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
tests/comfy_cli/test_http_only_openers.py builds a module-level OPENERS list naming every opener the CLI constructs, and line 31 is ("cql.loader._LOADER_OPENER", loader._LOADER_OPENER). Because it's evaluated at import time, a missing attribute takes the whole file down rather than failing one test.
This isn't your mistake so much as a sequencing one. That test file didn't exist when you wrote this — it arrived with #530 (route authed urllib paths through a shared NoRedirectHandler opener), which merged on 2026-07-30. I confirmed the timeline: git log --diff-filter=A -- tests/comfy_cli/test_http_only_openers.py points at #530, and your branch head has zero occurrences of _LOADER_OPENER while origin/main has the reference.
Fix: drop the ("cql.loader._LOADER_OPENER", loader._LOADER_OPENER) entry from OPENERS, and remove the now-unused loader import from that test if nothing else there uses it. Worth a line in the description noting the opener inventory shrank by one, since that list is deliberately exhaustive — a reader should see the removal was intentional rather than an oversight.
The rest looks right
An AST sweep over the whole repo for the symbols you delete confirms the removal is otherwise clean — load_graph is referenced only by its own def, the cql/__init__.py re-export you update, and the loader tests you delete; MAX_INPUT_BYTES only within loader.py itself. No live caller anywhere.
Collapsing the security-policy fork is the valuable part: two byte caps (256 MiB vs 64 MiB) and two SSRF guards for the same job is exactly the kind of duplication that drifts into a real hole, and the engine's loaders — which keep the loopback guard, no-redirect opener, byte cap, and cloud HTTPS+auth — are untouched.
Re-run once that reference is gone and I'll take another look; I expect this to be ready immediately after.
… (BE-4364) tests/comfy_cli/test_http_only_openers.py arrived with #530 after this branch was written, and its module-level OPENERS list named cql.loader._LOADER_OPENER — which this PR deletes along with the rest of the vestigial load_graph fetch stack. The list is evaluated at import, so the missing attribute failed collection and took the whole suite down rather than one test. Drop that one entry and the now-unused loader import. The inventory stays exhaustive: a sweep for build_http_only_opener/build_opener across comfy_cli/ finds exactly the seven openers still listed, so no opener is left uncovered — only the entry for a symbol that no longer exists is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
ELI-5
comfy_cli/cql/loader.pyhad an old, private way to fetch a workflow graph (load_graph→_load_from_file/_load_from_server). Nothing in the product uses it anymore — every real command loadsobject_infothroughresilient_load_object_info, which goes to the engine's loaders. This PR deletes that dead code and its tests, and makesresilient_load_object_infothe public entry point instead ofload_graph. No behavior changes.What changed
comfy_cli/cql/loader.py: deletedload_graph, the loader's own_load_from_file, and_load_from_server(the whole block above the# ---- normalization ----divider), plus the now-orphaned_LOADER_OPENERandMAX_INPUT_BYTES. Pruned the imports they used (NoRedirectHandler,is_loopback_host,urllib.error/parse/request). Rewrote the module docstring to describe what remains (normalize+resilient_load_object_info).comfy_cli/cql/__init__.py: re-exportresilient_load_object_infoas the public entry point in place ofload_graph.tests/comfy_cli/cql/test_loader.py: removed the tests covering the deleted stack (test_load_graph_*,test_load_from_server_*,_FakeResp); kept allnormalizetests and fixtures.tests/comfy_cli/test_http_only_openers.py: dropped the("cql.loader._LOADER_OPENER", loader._LOADER_OPENER)entry from the module-levelOPENERSinventory (and the now-unusedloaderimport). The opener inventory intentionally shrank by one — that list is deliberately exhaustive, so calling this out explicitly: the entry was removed because the opener it named no longer exists, not as an oversight. A sweep forbuild_http_only_opener/build_openeracrosscomfy_cli/finds exactly the seven openers still listed, so no live opener lost coverage.Why it's safe (security-flagged removal)
The engine's loaders (
comfy_cli/cql/engine.py_load_from_file/_load_from_target) andresilient_load_object_infoare the live path and are untouched — they keep their own loopback guard, no-redirect opener, byte cap, and cloud HTTPS+auth. This change only removes the vestigial duplicate stack inloader.py, eliminating the security-policy fork (256 MiB vs 64 MiB byte caps, duplicate SSRF guards) flagged in BE-4352.normalizeand its tests are intentionally left in place (a separate follow-up).Public API note (for release notes)
comfy_cli.cql.load_graph— which was exported via__all__— is removed.resilient_load_object_infois the replacement entry point. Flagging so reviewers can note it in release notes.Verification
grep -rn "load_graph\|_load_from_server\|_LOADER_OPENER\|MAX_INPUT_BYTES" --include='*.py' .→ zero hits (excluding.egg-info).uv run pytest tests/comfy_cli/cql/ tests/comfy_cli/command/→ 1409 passed, 2 skipped.ruff check/ruff format --checkclean on the touched files (the pre-existingdefault_workflow.py:139UP038 lint is unrelated and left untouched).