fix(workflow): detect oversize responses in _http_request instead of silently truncating (BE-3297)

[mattmillerai]

ELI-5

When the CLI asks Comfy Cloud for a workflow, it reads at most 64 MiB of the reply. If the reply was bigger, the CLI quietly chopped it off. The chopped-off JSON was then broken, so parsing failed — and that failure was caught and turned into "no body at all." The user got the same result as an empty response, with no clue their workflow had been truncated.

Now the CLI reads one byte past the limit. If that extra byte exists, the body was too big, and it says so loudly with a workflow_too_large error instead of pretending nothing was there.

What changed

• Added a module-level _HTTP_MAX_BYTES = 64 * 1024 * 1024, replacing the inline 64 * 1024 * 1024 magic number in _http_request.
• _http_request now reads _HTTP_MAX_BYTES + 1 and raises _ResponseTooLarge when the body exceeds the cap — mirroring the sibling _userdata_request, which has done exactly this since it was written.
• _handle_cloud_http_error gained a _ResponseTooLarge branch emitting a workflow_too_large envelope with details.limit_bytes.
• All four _http_request call sites (list, get, save, delete) added _ResponseTooLarge to their except tuple, so the new exception routes to an envelope rather than an uncaught traceback.
• Generalized the _ResponseTooLarge docstring, which named /userdata specifically and now covers both surfaces.

Judgment calls / deviations from the ticket

The ticket's premise about _handle_cloud_http_error was wrong, and I fixed it rather than working around it. The ticket states the cloud handler "already knows how to map _ResponseTooLarge to a limit_bytes envelope error." It does not — that is _handle_local_http_error, the local /userdata sibling. The cloud handler would have dropped _ResponseTooLarge into its generic else branch and emitted a vague cloud_http_error: get failed: with an empty exception string and a misleading "check network" hint. So step 3 of the plan required adding the branch, not just wiring the except tuples. This is the one substantive addition beyond the ticket's four steps.

Separate constant rather than reusing _USERDATA_MAX_BYTES. Both are 64 MiB and could share one name. I kept them distinct because they cap two different surfaces (local ComfyUI /userdata vs. the cloud API) that may reasonably diverge later; collapsing them would couple the two. Same value today, so no behavior difference either way.

details includes workflow_id in the cloud too-large envelope (the local one omits it). It is None for list/save, which is accurate and matches the other cloud error branches.

Verification

uv run -p 3.10 --extra dev pytest tests/comfy_cli/command/ -q → 1165 passed, 2 skipped. ruff format --check and ruff check clean on both touched files.

Six new tests:

• TestGet::test_response_over_cap_refuses_to_truncate — end-to-end envelope assertion, including details.limit_bytes.
• TestHttpRequestCap::test_oversize_body_raises_rather_than_returning_none — the exact regression the ticket names: oversize must raise, not return (status, None).
• test_body_exactly_at_cap_still_parses — boundary guard: len(raw) == cap is a complete body, not a truncated one, and must still parse.
• test_empty_body_still_returns_none — guards the unchanged (status, None) contract for a genuinely empty body.
• test_every_call_site_routes_oversize_to_envelope[list|get|save|delete] — parametrized over all four call sites, so an untouched except tuple fails the suite rather than surfacing a traceback to users.

The two regression tests were confirmed to fail against pre-fix behavior, not just pass against the new code: reverting _http_request to the truncating read (resp.read(_HTTP_MAX_BYTES), no overflow check) reproduces DID NOT RAISE _ResponseTooLarge and a red test_response_over_cap_refuses_to_truncate. The three boundary/contract tests pass in both directions by design — they guard against over-correction.

On the new error path

This adds a failure path, so worth stating plainly: it does not deny a capability that works today. Fetching an oversize workflow is already broken on main — I ran the pre-fix _http_request against a truncated body and it returns (200, None), which for get then surfaces as a misleading "unexpected response shape" error naming the wrong cause. The change converts an already-failing silent path into a loud, correctly-coded one; it does not remove a working path. Behavior for every in-cap response (the entire real-world case) is byte-for-byte unchanged.

Scope

_http_request has exactly four call sites, all in workflow.py; neither it nor _ResponseTooLarge is imported anywhere else in the repo (verified by grep), so the new exception cannot escape to an unprepared caller.

Deferred from a review thread on #530.

[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: 35 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: 403ce4db-7bfb-47bc-a81f-9f10c17ee17c

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and 812274e.

📒 Files selected for processing (2)

• comfy_cli/command/workflow.py
• tests/comfy_cli/command/test_workflow_saved.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3297-http-oversize

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3297-http-oversize

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

⚠️ Review failed

Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've verified the code. Confirmed:
- Line 647: `body = (e.read() or b"")[:1000]...` reads the full error body before slicing, bypassing the cap.
- Lines 629–632: `json.loads(raw)` on bytes, `except json.JSONDecodeError` only — `UnicodeDecodeError` (a `ValueError`) escapes; call sites don't catch it either.
- Line 643: hint hardcoded to "saved workflow" but reused for `list`/`get`/`delete`.
- Line 1120: `save` POST can succeed server-side yet report `workflow_too_large`.

I dropped nothing beyond

Re-trigger by removing and re-adding the cursor-review label.

[mattmillerai]

Review resolution (f91cab6)

The cursor-review panel's judge crashed with a parse_error before it could post findings, but its truncated output named four concrete issues. I treated those as real review signal and worked each one. Three held up; details below.

Fixed

1. The workflow_too_large hint was hardcoded to the get wording. The panel was right, and this was on code this PR introduced. _handle_cloud_http_error serves all four verbs, but the hint read "the saved workflow is unexpectedly large" — meaningless for list, where an oversize response means too many workflows, not one large one. Replaced with a per-operation _TOO_LARGE_HINTS table.

2. save can succeed server-side yet report workflow_too_large. Also correct, and a better catch than it first looks: save/delete have already sent their request by the time the response is read, so an oversize response says nothing about whether the write landed. The old hint implied a clean failure, which would invite a user to retry a mutation that already succeeded. Their hints now say the change may still have landed and point at workflow list to confirm.

3. UnicodeDecodeError escapes _http_request. Confirmed empirically — json.loads() on non-UTF-8 bytes raises UnicodeDecodeError, which is a ValueError but not a JSONDecodeError, so it escaped the except and no call site caught it. A non-UTF-8 body surfaced as a traceback, not an envelope. Now named alongside JSONDecodeError.

4. The HTTPError body read was unbounded. (e.read() or b"")[:1000] pulled an arbitrarily large error body into memory before slicing. e.read(1000) is byte-for-byte equivalent, verified including the fp=None edge case.

3 and 4 are pre-existing rather than introduced here, but both sit in the two functions this PR rewrites and are squarely on its theme (don't read unbounded; don't let a malformed body produce a confusing failure), so they're fixed rather than deferred.

Tests

Four new tests, each confirmed to fail against the pre-fix code — the hint test fails for list/save/delete and passes for get, which is exactly the shape of the bug. 1169 passed, 2 skipped; ruff clean.

Deferred

_handle_local_http_error (workflow.py:548) has the identical hardcoded-hint bug on the local /userdata surface. Not fixed here: this PR is scoped to the cloud _http_request, the local path is untouched pre-existing code no reviewer looked at on this PR, and its hints need different wording ("on the server" vs "cloud UI"), so it's a separate table rather than a reuse. Filed as a follow-up with a full plan.

On the two red checks

Both are pre-existing repo-wide breakage, unrelated to this diff (which touches only workflow.py):

• build — 9 failures in registry/test_config_parser.py + nodes/test_node_init.py, all tomlkit "Comment cannot contain line breaks" (tomlkit 0.15). Fix in flight: fix(registry): emit pyproject hint blocks as single-line tomlkit comments #533 / fix(registry): emit pyproject hints as standalone comments for tomlkit 0.15 #536.
• test — dependency install fails on pydantic_core import. Fix in flight: fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289) #535.

Neither touches any file in this PR, and both pass locally. This PR's own suite is green.

Note: CodeRabbit never actually reviewed this PR — it hit a Fair-Usage rate limit and posted only the limit notice, though its check reports "pass". Re-triggering below.

[mattmillerai]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[mattmillerai]

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

• https://linear.app/comfyorg/issue/BE-3311|BE-3311 — Give the local /userdata workflow_too_large error per-operation hints

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

⚠️ Review failed

Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've validated each finding against the actual code. Key confirmations:
- Line 642 (`except ... return status, None`): `list_cmd` (L972 `rows = (body or {}).get("data") or []`) reports `count: 0, ok: true` and `save_cmd` (L1138) emits `workflow_id: null, ok: true` when the body is undecodable. `get_cmd` (L1043) is safe (shape check), and `delete_cmd` ignores the body — so the accurate framing is list+save, not delete.
- Line 662 is where `e.read(1000)` actually lives (the reviewer's "663" points

Re-trigger by removing and re-adding the cursor-review label.

[mattmillerai]

Second-pass review resolution (812274e)

All review threads on this PR were already resolved by the previous pass (f91cab6). This pass found one thing that pass introduced, and re-verified the rest.

Fixed: dropped an unrelated 287-line uv.lock regeneration

f91cab6 accidentally committed a full lockfile regen — adding anthropic, pydantic, jiter and friends to a PR that only touches _http_request. The PR description never mentions it, which is what flagged it.

The drift is real but pre-existing, not contamination: #490 added optional-dependencies.bench = [ "anthropic>=0.40" ] to pyproject.toml without re-locking, so uv lock --check fails on main today. I verified this directly — main's lock reports "needs to be updated", this branch's passes.

The trap is that a bare uv run --extra dev pytest ... (this repo's own documented test command) silently re-locks as a side effect. That's how it got picked up, and it re-dirtied the file on me mid-pass too. Reverted to main's copy and committed without touching uv in between.

Behavior-neutral: no CI job consumes uv.lock (only ci-cursor-review.yml mentions it, and only to exclude it from the diff-size gate). Carrying it here would have invited conflicts with #533/#535/#536, all in flight on dependency code. Diff is now 2 files / 120 lines.

Deferred to a follow-up with a full re-lock plan → tracked by the fleet.

Re-verified the substantive diff

• _TOO_LARGE_HINTS["list"] points at --limit and --name — confirmed both flags actually exist on list_cmd, so the hint is actionable rather than inventing options.
• _ResponseTooLarge is raised after the with urllib.request.urlopen(...) block exits, so the response is closed before unwinding.
• All four call sites (list/get/save/delete) carry _ResponseTooLarge in their except tuple; the parametrized test fails the suite if one is missed.
• Negative-claim check: this adds a failure path, so — it denies nothing that works today. Oversize fetch is already broken on main (returns (200, None), surfacing as a misleading "unexpected response shape"). This converts an already-failing silent path into a correctly-coded loud one. Every in-cap response is byte-for-byte unchanged.

1169 passed, 2 skipped; ruff format --check and ruff check clean.

The one red check is not this PR

test (windows) fails in Install Dependencies, before any test runs:

error: failed to remove file `...\venv\Lib\site-packages\pydantic_core\_pydantic_core.cp312-win_amd64.pyd`: Access is denied. (os error 5)
ImportError: cannot import name '__version__' from 'pydantic_core' (unknown location)

That's the pre-existing repo-wide breakage #535 (BE-3289) is fixing — a locked-DLL upgrade during comfy install, on a code path this PR does not touch. This PR's own suite is green.

[mattmillerai]

Third-pass review resolution — no code change

No unresolved review threads, no merge conflict (MERGEABLE, branch is current with main). The two prior passes (f91cab6, 812274e) resolved every finding the cursor panel surfaced. This pass re-verified the diff independently and diagnosed the newly-red build check, which is not this PR.

The build red is a repo-wide dependency break, already fixed by #537

build fails on every open PR (#540, #541, #543, #544) with the same 9 failures in test_config_parser.py / test_node_init.py — files this PR does not touch:

ValueError: Comment cannot contain line breaks

Root cause, reproduced locally: tomlkit 0.15.1 shipped today and rejects comments containing line breaks. uv.lock pins tomlkit==0.13.3, but pytest.yml installs via pip install -e ., which re-resolves from scratch and never sees the pin. Confirmed both directions:

• uv run --frozen --extra dev pytest tests/comfy_cli/registry/test_config_parser.py → 81 passed (locked 0.13.3)
• uv run --extra dev --with 'tomlkit>0.13.3' pytest ... → 8 failed (0.15.1) — the exact CI signature

#537 (BE-3292) already fixes this by installing from uv.lock; its own build job is green. Nothing to do here — this PR goes green once #537 lands. The test (windows) red is the pre-existing pydantic_core locked-DLL failure tracked by #535 (BE-3289), also unrelated.

Re-verified the diff independently

• Every branch of _handle_cloud_http_error reaches return typer.Exit(code=1) — the new _ResponseTooLarge branch is an if ahead of the HTTPError elif, so it cannot fall through and return None into a raise ... from.
• _ResponseTooLarge is not an HTTPError, so ordering the isinstance check first is required, not stylistic.
• raise _ResponseTooLarge() sits outside the with urlopen(...) block — the response is closed before unwinding.
• All four call sites carry _ResponseTooLarge in their except tuple; the parametrized test fails the suite if one is dropped.
• e.read(1000) is equivalent to the old (e.read() or b"")[:1000] without the unbounded read.

1169 passed, 2 skipped; ruff format --check and ruff check clean. Working tree clean — uv.lock untouched (used uv run --frozen throughout to avoid the silent re-lock trap #812274e documented).

No new follow-ups: the deferrals from earlier passes are already filed as BE-3311 and BE-3312.