fix: give the local /userdata workflow_too_large error per-operation hints#544
fix: give the local /userdata workflow_too_large error per-operation hints#544mattmillerai wants to merge 2 commits into
Conversation
…hints
_handle_local_http_error is called with operation= of list, get, save and
delete, but hardcoded a hint that is only correct for get:
- list: an oversize /userdata listing means too many workflows, not one
large workflow, so 'the saved workflow' named a thing that does not
exist in that context.
- save/delete: the request is already on the wire by the time the body is
read, so an oversize response does not mean the write was rejected. The
old hint implied a clean failure, inviting a retry of a mutation that
may have already landed.
Add a _LOCAL_TOO_LARGE_HINTS table keyed by operation, with a neutral
fallback for any future call site.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 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)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
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)
The lock refresh (adding anthropic/pydantic for the `bench` optional extra) was swept in accidentally by `uv run` regenerating the stale lock during local test runs. It has nothing to do with the per-operation `workflow_too_large` hints this PR is about, and the same re-lock is already covered by its own dedicated PRs (#534/#541/#543/#545). Keeps this PR scoped to comfy_cli/command/workflow.py + its tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review pass — scope cleanup + CI triageRemoved unrelated
|
|
✅ Action performedReview finished.
|
ELI-5
When
comfy workflow ... --where localgets a huge response back from ComfyUI, the CLI refuses to truncate it and prints an error with a hint. That hint was hardcoded to say "the saved workflow is unexpectedly large; inspect it directly on the server" — which is good advice forget, and wrong (or actively misleading) for the other three verbs that share the same handler. This gives each verb the hint that's actually true for it.What was wrong
_handle_local_http_erroris called withoperation=oflist,get,save, anddelete(5 call sites), but hardcoded one hint:/userdatalisting means too many workflows, not one large workflow. "The saved workflow" named a thing that doesn't exist in this context.What changed
_LOCAL_TOO_LARGE_HINTStable keyed by operation, plus a neutral fallback ("the local response was unexpectedly large") at the call site for any future operation.TestLocalTooLargeHintsparametrizes over all four verbs against the local surface with_USERDATA_MAX_BYTESmonkeypatched tiny, asserting the envelope hint matches the table; plus targeted assertions that thesave/deletehints don't imply the write was rejected, that thelisthint doesn't name a single workflow, and that the unknown-operation fallback fires.This mirrors the
_TOO_LARGE_HINTSfix made on the cloud_handle_cloud_http_errorsurface in #540 (commit f91cab6). It's a separate table rather than reuse: the local hints need different wording ("inspect it directly on the server" vs "in the cloud UI") and reference--where localin the confirm command.Judgment call — the
listhint deviates from the proposed wordingThe plan proposed
list-> "narrow the result set with--limitor--name". I did not use that, because it's not true on the local path. ComfyUI's/userdatalisting has no server-side sort/limit/filter (see the existing comment on_LOCAL_SORT_KEYS):_local_listsends onlydir/recurse/split/full_info, buffers the whole listing, and only then applies--name/--limitclient-side. So those flags cannot prevent the cap from tripping — suggesting them would send the user in a circle. Verified against the code rather than assumed: the request buildsparams = {"dir", "recurse", "split", "full_info"}and nothing else, and there is no other userdata listing path or server-sidelimit/nameparameter anywhere incomfy_cli.The hint instead points at the only thing that actually helps (fewer files on the server) and says explicitly why the filter flags won't:
Happy to trim the wording if it reads long — the parenthetical is there because "use
--limit" is the obvious wrong guess.Placement note: the plan said to add this next to the existing
_TOO_LARGE_HINTStable. That table doesn't exist onmain— it arrives with #540, which is still open — so the new table sits with the other local/userdataconstants (next to_ResponseTooLarge/_LOCAL_SORT_KEYS), which is where it belongs anyway. No conflict with #540: different name, different section.Verification
uv run -p 3.10 --extra dev pytest tests/comfy_cli/command/ -q-> 1165 passed, 2 skipped.ruff format/ruff check-> clean.test_hint_is_per_operationfails forlist/save/deleteand passes forget. Simply deleting the source change instead fails all four onAttributeError, which proves nothing — hence the isolated check.grepshows exactly 5_handle_local_http_errorcalls passing exactly{list, get, save, delete}, all four keyed in the table, with the fallback covering anything added later.No behavior changes beyond the hint string; the error
code,message, anddetailsare untouched.