fix(workflow): surface unparseable cloud 200 body as an error instead of empty/success (BE-3334)#549
fix(workflow): surface unparseable cloud 200 body as an error instead of empty/success (BE-3334)#549mattmillerai wants to merge 3 commits into
Conversation
… of empty/success (BE-3334)
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: 23 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 (2)
📝 WalkthroughWalkthroughCloud workflow requests now classify non-empty, non-JSON responses as ChangesCloud workflow error handling
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
the
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/command/workflow.py`:
- Around line 628-634: Update the response parsing logic around the JSON load
and _ResponseUnparseable handling to explicitly decode non-empty cloud response
bodies as UTF-8 before calling json.loads. Ensure UTF-16/32 or other non-UTF-8
bytes raise UnicodeDecodeError and are mapped to _ResponseUnparseable,
preserving the existing malformed-response error path.
In `@tests/comfy_cli/command/test_workflow_saved.py`:
- Around line 550-567: Extend TestUnparseableResponse to assert
error.details.operation for list and save using their respective operation
values, then add malformed-response tests covering the changed get and delete
command paths. Each new case should assert ok is false, error.code is
workflow_unparseable, and the envelope’s operation detail matches the invoked
command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c637a549-84be-4f38-bd2c-cbb7dd7e47b1
📒 Files selected for processing (3)
comfy_cli/command/workflow.pycomfy_cli/error_codes.pytests/comfy_cli/command/test_workflow_saved.py
Handed raw bytes, json.loads auto-detects UTF-16/32 (RFC 4627) and would silently accept a non-UTF-8 body the workflow_unparseable contract treats as malformed. Decode UTF-8 explicitly first so such bodies raise UnicodeDecodeError -> _ResponseUnparseable, matching the contract. Also expand TestUnparseableResponse per review: add a UTF-16-JSON regression param, assert details.operation on the envelope, and cover the get/delete catch sites (were untested). Addresses CodeRabbit review on PR #549. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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 |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 2 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
Address adversarial review-panel findings on the unparseable-200 handling:
- _http_request: catch the ValueError base (+ RecursionError) instead of only
(JSONDecodeError, UnicodeDecodeError). Both were already ValueError subclasses;
the base also maps json.loads' bare ValueError from an integer past CPython's
4300-digit int/str limit, which previously escaped as a raw traceback.
- list_cmd: guard the response shape like get/save do. A valid-JSON non-dict 200
(an array or scalar) hit (body).get("data") -> raw AttributeError; coercing it to
[] would masquerade a malformed shape as a genuinely-empty listing. Now mapped to
cloud_http_error.
- _handle_cloud_http_error: bound the HTTP error-body read with e.read(1000) rather
than read-all-then-slice, so a large/malicious error page can't force an unbounded
allocation on the error path.
- tests: add a bigint bare-ValueError body param and non-dict-shape list cases.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
ELI-5
When you run
comfy workflow listorcomfy workflow saveagainst Comfy Cloud, the CLI reads the server's reply and parses it as JSON. If the server answered200 OKbut the body was garbage (a non-JSON HTML error page from a proxy, or non-UTF-8 bytes), the CLI used to quietly pretend it got nothing — solistsaidcount: 0, ok: true(looks like "you have no workflows") andsavesaidok: true, changed: true, workflow_id: nulland printed✓ saved 'x' → None(looks like it worked). Both lied: the request actually failed, you just couldn't tell.This PR makes that case loud: a non-empty-but-unparseable 200 body now raises a new
_ResponseUnparseableand surfaces as a real error envelope (ok: false, codeworkflow_unparseable) instead of a misleading empty/success. A genuinely empty body is still treated as legitimate "no data" — that behavior is unchanged.What changed
_http_request(the shared cloud helper): now distinguishes the two conditions. An empty body still returns(status, None); a non-empty body that fails to decode raises_ResponseUnparseable(a sibling to the existing_ResponseTooLarge). It now also catchesUnicodeDecodeError—json.loadsdecodes bytes itself and raises that (notJSONDecodeError) on non-UTF-8 input, which previously would have escaped uncaught._handle_cloud_http_error: maps_ResponseUnparseable→ error codeworkflow_unparseable, mirroring theworkflow_too_largebranch (message + hint +details.operation).list/get/save/delete) add_ResponseUnparseableto theirexcepttuple so the new raise is caught and mapped rather than crashing.error_codes.py: registeredworkflow_unparseable(the repo enforces "every raised code is registered" viatest_error_code_registry.py).TestUnparseableResponsefeeds both a non-JSON (<html>…) and a non-UTF-8 (\xff\xfe…) 200 body throughlistandsave, assertingok: false+workflow_unparseable(i.e. no silent empty/null-id success).Scope / judgment calls
None-on-unparseable path consistent; distinguishing "empty" from "unparseable" was explicitly left as separate work.get/deletewere already non-misleading, but_http_requestnow raises where it used to returnNone, so they must include_ResponseUnparseablein theirexcepttuple or they'd crash on a non-empty junk body. Net effect:getnow reportsworkflow_unparseable(was a generic "unexpected response shape") anddeletereports it too (was a silentdeleted: trueon a junk body) — both strictly clearer, and the empty-body/204 delete path is unchanged.Testing
pytest tests/comfy_cli/command/test_workflow_saved.py— 41 passed (incl. 4 new cases)pytest tests/comfy_cli/output/test_error_code_registry.py— passed (confirms the new code is registered + raised)pytest tests/comfy_cli/command/— 1161 passed, 2 skippedruff check+ruff format --check— clean