refactor(cql): remove vestigial load_graph fetch stack from loader.py (BE-4364)

[mattmillerai]

ELI-5

comfy_cli/cql/loader.py had 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 loads object_info through resilient_load_object_info, which goes to the engine's loaders. This PR deletes that dead code and its tests, and makes resilient_load_object_info the public entry point instead of load_graph. No behavior changes.

What changed

• comfy_cli/cql/loader.py: deleted load_graph, the loader's own _load_from_file, and _load_from_server (the whole block above the # ---- normalization ---- divider), plus the now-orphaned _LOADER_OPENER and MAX_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-export resilient_load_object_info as the public entry point in place of load_graph.
• tests/comfy_cli/cql/test_loader.py: removed the tests covering the deleted stack (test_load_graph_*, test_load_from_server_*, _FakeResp); kept all normalize tests and fixtures.
• tests/comfy_cli/test_http_only_openers.py: dropped the ("cql.loader._LOADER_OPENER", loader._LOADER_OPENER) entry from the module-level OPENERS inventory (and the now-unused loader import). 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 for build_http_only_opener/build_opener across comfy_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) and resilient_load_object_info are 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 in loader.py, eliminating the security-policy fork (256 MiB vs 64 MiB byte caps, duplicate SSRF guards) flagged in BE-4352. normalize and 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_info is 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 --check clean on the touched files (the pre-existing default_workflow.py:139 UP038 lint is unrelated and left untouched).

[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: 57 minutes

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: b73b0a5c-bdd0-477d-8541-bda525c1dd0e

📥 Commits

Reviewing files that changed from the base of the PR and between a6a62cf and a052d1e.

📒 Files selected for processing (4)

• comfy_cli/cql/__init__.py
• comfy_cli/cql/loader.py
• tests/comfy_cli/cql/test_loader.py
• tests/comfy_cli/test_http_only_openers.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-4364-remove-loader-fetch-stack

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-4364-remove-loader-fetch-stack

---

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

[github-actions]

🔍 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)

[bigcat88]

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.

[skishore23]

Approving — @bigcat88's blocker is fixed. But the merge conflict with main is a trap; please read that section before resolving, because the obvious resolution breaks the cache-write path.

@bigcat88's blocker is resolved

tests/comfy_cli/test_http_only_openers.py is now in the diff and the cql.loader._LOADER_OPENER entry is gone, so collection succeeds — 195 passed as-authored (test_http_only_openers.py + cql/), no collection error.

I also independently checked the "opener inventory intentionally shrank by one" claim, since that list is deliberately exhaustive. It names exactly seven, and they're the seven that exist:

http._AUTHED_OPENER      comfy_client._OPENER     cql.engine._opener
cloud.oauth._OAUTH_OPENER   transfer._TRANSFER_OPENER
transfer._DOWNLOAD_OPENER   http._PLAIN_OPENER

Cross-checked against every module-level … = build_http_only_opener(…) / build_opener(…) in comfy_cli/ — same seven, nothing lost coverage. Good call flagging it explicitly.

⚠️ The conflict: do NOT resolve by taking this PR's side

comfy_cli/cql/loader.py conflicts because main changed the region this PR deletes. The HEAD side is empty (you removed the whole block), so "keep ours" looks obviously right — and it's wrong.

main added from comfy_cli.file_utils import atomic_write_text to that region, and write_object_info_cache (which survives) now uses it at loader.py:315. Your branch predates that switch — your copy of write_object_info_cache has zero atomic_write_text references. So taking the empty HEAD side drops the import while keeping the caller → NameError on every cache write, i.e. resilient_load_object_info breaks at loader.py:391 and :411 on the success path.

Correct resolution — replace the whole conflict with exactly this one line:

from comfy_cli.file_utils import atomic_write_text

Everything else on main's side of the hunk (MAX_INPUT_BYTES, _LOADER_OPENER, the NoRedirectHandler/build_http_only_opener imports, load_graph, _load_from_file, _load_from_server) is exactly the dead stack this PR removes.

I applied that locally and confirmed:

dead symbols remaining (load_graph|_load_from_server|_LOADER_OPENER|MAX_INPUT_BYTES): 0
ruff check: All checks passed
pytest tests/comfy_cli/cql tests/comfy_cli/test_http_only_openers.py: 211 passed

The security rationale holds

Removing the duplicate stack does eliminate a real policy fork — loader.py's own 256 MiB MAX_INPUT_BYTES vs the engine's cap, plus a second SSRF/loopback guard maintained in parallel. Two independently-maintained copies of a security check is how they drift. The live path (engine._load_from_file / _load_from_target + resilient_load_object_info) is untouched and keeps its guards.

Note

comfy_cli.cql.load_graph was in __all__, so this is a public API removal — you've flagged it for release notes, which is right. Worth a line in the changelog naming resilient_load_object_info as the replacement.

My approval won't clear @bigcat88's outstanding CHANGES_REQUESTED — flagging @bigcat88 that the _LOADER_OPENER reference is now handled.

[bigcat88]

Addressed — approving and merging. 36b8ec4 drops the _LOADER_OPENER entry and the now-unused loader import, which is exactly what I asked for. Apologies this sat 11 days: you fixed it 14 minutes after my review and I was filtering my queue on reviewDecision, which never clears on its own.

Re-verified against today's main, not the tree you wrote it on

A deletion PR's risk is that main grows a new reference while it waits, so I re-ran the sweep rather than trusting the old result. AST walk over comfy_cli/ + tests/ for every symbol this removes (load_graph, MAX_INPUT_BYTES, _LOADER_OPENER, _load_from_file, _load_from_server), plus a raw grep as a backstop for string/getattr access:

• Zero dangling references. The three _load_from_file hits that remain all resolve to comfy_cli.cql.engine._load_from_file — the surviving copy this consolidates onto, not the one you delete.
• test_http_only_openers.py now imports comfy_client, http, oauth, transfer, engine — no loader. Collection is clean.

Behaviour through the real CLI, against a live 792-class object_info dump, branch vs main:

nodes ls              ok, 792 rows   (identical)
nodes show KSampler   ok             (identical)
nodes path MODEL IMAGE ok, 10 paths  (identical)

• Full suite: 4773 passed, 31 skipped. The one failure is test_non_fast_deps_uses_global_python, which fails identically on plain main here.
• ruff check + ruff format --diff at the CI-pinned 0.15.15 — clean.

One stale comment this creates (nit — please fix, but I'm not blocking on it)

comfy_cli/http.py:237-239 justifies MAX_RESPONSE_BYTES by pointing at the cap you're deleting:

# It also matches the cap the already-bounded call sites converged on
# independently (``models/search``, both ``workflow`` readers), so the CLI has
# one number rather than five. ``cql.loader`` keeps its own, larger
# ``MAX_INPUT_BYTES`` — that one bounds a user-supplied object_info dump, a
# different thing being measured.

After this PR both halves are false: cql.loader has no MAX_INPUT_BYTES, and the surviving object_info cap is engine._MAX_OBJECT_INFO_BYTES = 64 MiB — equal to MAX_RESPONSE_BYTES, not larger. It's a comment in a file you don't touch, so it's easy to miss, but it documents a security-relevant number and a reader who goes looking for the symbol will find nothing and may conclude the cap was lost. Suggested:

# one number rather than five. The object_info dump has its own cap,
# ``cql.engine._MAX_OBJECT_INFO_BYTES``, which happens to be the same size.

Worth stating explicitly in the description

The effective ceiling on a user-supplied object_info dump drops 256 MiB → 64 MiB as a side effect of collapsing onto the engine's loader. I think that's right — and I measured the headroom rather than guessing: a real 792-class catalog off a live ComfyUI is 1.3 MiB, so 64 MiB is ~48× a realistic worst case. Still, it's a user-visible tightening hiding inside a "remove vestigial code" PR, and the body should say so.

Also worth a line: cql/__init__.py's __all__ loses load_graph and gains resilient_load_object_info. Nothing in this repo imported it, but that is a public name on a published package — if comfy-mcp or anything else reaches for comfy_cli.cql.load_graph, it breaks at import. You'd know better than I would whether anything does.