refactor(http): shared capped-read request_json; migrate workflow + models search, fixing search's uncaught oversize/non-UTF-8 crashes (BE-4363)

[mattmillerai]

ELI-5

Two places in the CLI did the exact same thing: "call an authenticated URL, refuse to read more than N bytes, hand back the parsed JSON." They had drifted into two separate copies. This merges them into one shared helper.

The interesting part is what the second copy got wrong. comfy models … capped its reads, but when a response blew past the cap it raised a plain ValueError — and none of its four error handlers catch ValueError. Same story for a response body that isn't valid UTF-8: json.loads raises UnicodeDecodeError, which is a ValueError but not a JSONDecodeError, so it slipped past those handlers too. Either one crashed the CLI with a Python traceback and no --json envelope at all. Both now come out as a normal ok:false envelope.

What changed

comfy_cli/http.py — new ResponseTooLarge exception and request_json(url, target, *, method, body, timeout, max_bytes). It reads one byte past the cap so a complete body is distinguishable from a truncated one, raises urllib errors verbatim so callers keep mapping them to their own envelope codes, and returns None for an empty or unparseable body. max_bytes is keyword-required with no default, so each caller keeps owning its own cap constant rather than inheriting a shared one by accident.

comfy_cli/command/workflow.py — _http_request is now a thin wrapper that delegates. Its name, signature, and exception surface are unchanged, and it still reads the module-global _HTTP_MAX_BYTES at call time (so the existing tests that monkeypatch that constant keep working). The local _ResponseTooLarge class is now a top-level alias of the shared one, which keeps all ten except sites and the two isinstance checks working untouched. A class used in an except clause at module scope can't be lazy-imported, so this is a real top-level import — comfy_cli.http is stdlib-only, and search.py/jobs.py already pull urllib.request in at import time, so it doesn't change the import-cost picture.

comfy_cli/command/models/search.py — _http_get_json wraps request_json and preserves its contract (returns the parsed body, raises decode errors verbatim). ResponseTooLarge joins (URLError, OSError, JSONDecodeError) in all four caller except-tuples, which routes the oversize case to the existing cloud_http_error / server_not_running path with the exception message interpolated — so the helper raises a descriptive message (response from <url> exceeds <n> byte cap) rather than a bare exception.

_emit_http_error and every error mapper are untouched, as are comfy_client.py, jobs.py, and workflow._userdata_request (raw bytes, not JSON — out of scope). Redirect behavior is unchanged: request_json uses the default opener exactly as both helpers it replaces did. Deliberately not attaching NoRedirectHandler here — that would be a behavior change to these call sites and belongs in its own ticket.

Judgment calls

One intentional behavior change. request_json cannot distinguish a literal JSON null body from an unparseable one — both parse to None. For workflow this is a no-op (its old code returned (status, None) for null too). For search it means a null body now raises JSONDecodeError → error envelope. Previously models list-folders reported ok:true, count:0 for a null body, which silently presented a malformed response as "this backend has no folders"; models search / models show crashed outright with an AttributeError on body.get(). An error envelope is the right answer in all three cases. Pinned by test_literal_null_body_yields_envelope.

Deleted search._authed_request (and its now-unused import urllib.request). The ticket said "delete nothing else," but that helper existed solely to serve _http_get_json and had no other caller — leaving it would be dead code, and leaving the import would fail ruff F401. workflow._authed_request is kept, since _userdata_request still uses it.

Widened _http_request's return annotation from tuple[int, dict | None] to tuple[int, dict | list | None]. The old annotation was already inaccurate — json.loads has always been able to return a list there — so this is a truthfulness fix, not a contract change.

Testing

tests/comfy_cli/test_http.py — 12 new tests for request_json: oversize raises ResponseTooLarge with a descriptive message, body exactly at cap parses, empty body → (status, None), non-UTF-8 → (status, None), unparseable → (status, None), list body parses, GET sends no body/Content-Type, POST attaches both, cloud target gets auth headers, local target's header bag is empty even with stray credentials set, urllib errors propagate verbatim, max_bytes is required.

tests/comfy_cli/command/models/test_search.py — a new TestMalformedResponseEnvelopes class covering the two regressions end-to-end, plus a parametrized case asserting all four _http_get_json call sites (list-folders, list-folder, search, show) route oversize to an envelope. I verified these are genuine regression tests by running them against the pre-change search.py: 7 of 9 fail there with no envelope on stdout (the other 2 are preservation tests for paths that already worked). _patch_urlopen gained bytes-payload support so a test can serve a body that isn't valid JSON at all.

Full suite: 3095 passed, 37 skipped, 1 failed. The one failure is test_error_code_registry.py::test_no_duplicate_codes (duplicate server_died code in the registry) — I confirmed it fails identically on a clean origin/main worktree, so it is pre-existing and unrelated to this change. ruff check and ruff format --diff are clean under the CI-pinned ruff==0.15.15.

[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: 17 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: 6b8c4df7-17a4-41b0-91dc-665db3e8e135

📥 Commits

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

📒 Files selected for processing (6)

• comfy_cli/comfy_client.py
• comfy_cli/command/models/search.py
• comfy_cli/command/workflow.py
• comfy_cli/http.py
• tests/comfy_cli/command/models/test_search.py
• tests/comfy_cli/test_http.py

---

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

[dosubot]

📄 Knowledge review

Dosu skipped reviewing this PR because your organization has used its 200 included credits for the month. Your usage will reset on 2026-08-01. To have Dosu review this PR before then, ask your organization admin to upgrade to a pro account.

---

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 8 finding(s).

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

Panel: 7/8 reviewers contributed findings.

Reviewers that did not contribute: gemini-3.1-pro:adversarial (error)

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 6 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	2
🟢 Low	3

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

• BE-5090 — Distinguish empty/null/malformed JSON bodies in comfy_cli.http.request_json instead of collapsing all to None
• BE-5091 — comfy_cli.http.request_json's timeout bounds each socket read, not the whole response, letting a slow-drip server hang the CLI

[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 — merged main in (98325c9, including #614 and #628) and resolved the conflicts in http.py, comfy_client.py, error_codes.py, search.py, and workflow.py: kept main's http(s)-only opener infra and its already-shipped ResponseUnparseable/workflow_unparseable distinction, and layered this branch's request_json/ResponseTooLarge helper on top of main's shared _AUTHED_OPENER for search.py's migration. All 3676 local tests pass on the merged tree; CI is re-running now. All prior Cursor review threads were already resolved (BE-5090/BE-5091 filed for the deferred items). @bigcat88 ready for another pass whenever convenient.

[bigcat88]

Approving. Every failure mode in the description reproduces on main and is fixed here — I drove the real CLI against a stub serving each malformed body shape, including a genuinely oversize one (71.5 MB, past the 64 MiB cap).

models — all three crashes are real

                main                                              this branch
null      ->    ok, exit 0, count=0   (list-folders)              cloud_http_error
          ->    AttributeError: 'NoneType' has no attribute 'get' (search, show)
nonutf8   ->    UnicodeDecodeError, no JSON on stdout             cloud_http_error
oversize  ->    ValueError: response from …, no JSON on stdout    cloud_http_error

Confirmed on list-folders, search --text foo, and show independently. The two tracebacks exit non-zero with nothing on stdout, so a --json caller gets neither a result nor an error object — the worst shape for an agent.

The null row is the intentional behavior change, and your read of it is right: list-folders reporting ok, count=0 for a null body silently presents a malformed response as "this backend has no folders", which is a wrong answer rather than a crash. Turning all three into one envelope is the correct call.

The workflow side is byte-identical — the refactor didn't regress it

This is what I most wanted to check, since _http_request keeps its name and ten except sites:

body        main                    this branch
good        ok                      ok
null        ok                      ok
empty       ok                      ok
html        workflow_unparseable    workflow_unparseable
nonutf8     workflow_unparseable    workflow_unparseable
oversize    workflow_too_large      workflow_too_large

Identical across all six. Aliasing _ResponseTooLarge to the shared class rather than replacing it is what keeps those except sites and the two isinstance checks working untouched, and reading _HTTP_MAX_BYTES at call time is what keeps the existing monkeypatching tests honest.

Guards check out

assert_safe_url:  ALLOW https://… , http://127.0.0.1 , http://localhost , http://[::1]
                  REJECT http://evil.example.com , http://10.0.0.5

And end to end: a cloud Target carrying an api_key against http://evil.example.com is refused before the request is sent. Gating on if headers: is the right shape — an unauthenticated local call over plain http stays legal, which is exactly the local-ComfyUI case.

max_bytes rejects 0 and negatives rather than silently reading nothing. Making it keyword-required with no default is the right call: a shared default would quietly re-unify the two caps this PR is careful to keep separate.

Reading max_bytes + 1 so a complete body is distinguishable from a truncated one is the small detail that makes ResponseTooLarge trustworthy rather than a guess.

Notes

Deleting search._authed_request is correct — it had no other caller, and leaving it plus its import would trip ruff F401. Keeping workflow._authed_request because _userdata_request still uses it is the right asymmetry.

Widening the return annotation to tuple[int, dict | list | None] is a truthfulness fix; json.loads could always return a list there.

Good call leaving NoRedirectHandler attachment out of scope — that would change behavior at these call sites rather than dedupe them.

Full suite green on this branch merged with current main; ruff check + ruff format --diff clean under the CI-pinned 0.15.15.