Skip to content

fix: give the local /userdata workflow_too_large error per-operation hints#544

Open
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-3311-local-too-large-hints
Open

fix: give the local /userdata workflow_too_large error per-operation hints#544
mattmillerai wants to merge 2 commits into
mainfrom
matt/be-3311-local-too-large-hints

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

When comfy workflow ... --where local gets 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 for get, 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_error is called with operation= of list, get, save, and delete (5 call sites), but hardcoded one hint:

  • list — an oversize /userdata listing means too many workflows, not one large workflow. "The saved workflow" named a thing that doesn't exist in this context.
  • save / delete — these have already sent their request by the time the response body is read, so an oversize response does not mean the write was rejected. The old hint implied a clean failure, which invites the user to retry a mutation that may have already landed.

What changed

  • New _LOCAL_TOO_LARGE_HINTS table keyed by operation, plus a neutral fallback ("the local response was unexpectedly large") at the call site for any future operation.
  • Tests: TestLocalTooLargeHints parametrizes over all four verbs against the local surface with _USERDATA_MAX_BYTES monkeypatched tiny, asserting the envelope hint matches the table; plus targeted assertions that the save/delete hints don't imply the write was rejected, that the list hint doesn't name a single workflow, and that the unknown-operation fallback fires.

This mirrors the _TOO_LARGE_HINTS fix made on the cloud _handle_cloud_http_error surface 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 local in the confirm command.

Judgment call — the list hint deviates from the proposed wording

The plan proposed list -> "narrow the result set with --limit or --name". I did not use that, because it's not true on the local path. ComfyUI's /userdata listing has no server-side sort/limit/filter (see the existing comment on _LOCAL_SORT_KEYS): _local_list sends only dir/recurse/split/full_info, buffers the whole listing, and only then applies --name/--limit client-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 builds params = {"dir", "recurse", "split", "full_info"} and nothing else, and there is no other userdata listing path or server-side limit/name parameter anywhere in comfy_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:

too many saved workflows on the server; prune the ComfyUI workflows/ userdata directory (--limit/--name filter client-side, so they cannot shrink the response)

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_HINTS table. That table doesn't exist on main — it arrives with #540, which is still open — so the new table sits with the other local /userdata constants (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.
  • Non-vacuity check (the shape the cloud-side test showed): with the new table present but the call site reverted to the old hardcoded hint, test_hint_is_per_operation fails for list/save/delete and passes for get. Simply deleting the source change instead fails all four on AttributeError, which proves nothing — hence the isolated check.
  • Call-site coverage confirmed exhaustive: grep shows exactly 5 _handle_local_http_error calls 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, and details are untouched.

…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.
@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

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: 6 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: 8844eafc-a96a-4217-9525-3353748dd936

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and 87b5c3e.

📒 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-3311-local-too-large-hints
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-3311-local-too-large-hints

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mattmillerai
mattmillerai marked this pull request as ready for review July 17, 2026 10:30
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Jul 17, 2026
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>
@mattmillerai

Copy link
Copy Markdown
Collaborator Author

Review pass — scope cleanup + CI triage

Removed unrelated uv.lock churn (87b5c3e)

The commit had swept in a 285-line uv.lock refresh (adding anthropic/pydantic/pydantic-core for the bench optional extra). That was accidental: pyproject.toml on main already declares optional-dependencies.bench, but main's lock never got re-synced, so every local uv run regenerates the lock and dirties the tree — and a commit -a picks it up. I reproduced this during review (the lock re-dirtied itself right after I reverted it).

It's now reverted, so this PR is back to exactly its stated scope: comfy_cli/command/workflow.py + its tests. The re-lock itself is legitimate but already owned by dedicated PRs (#534 / #541 / #543 / #545), so it doesn't belong here.

The two red checks are pre-existing and not from this diff

Verified on a clean main worktree, not assumed:

Both are repo-wide and block every PR touching comfy_cli/**. I deliberately did not fix them here — that would duplicate the PRs above and mix an unrelated production fix into a hint-string change. This PR should go green once those land.

Self-review

  • Call-site coverage is exhaustive: exactly 5 _handle_local_http_error calls passing exactly {list, get, save, delete}; all four keyed, .get() fallback covers any verb added later.
  • The list hint's negative claim ("--limit/--name filter client-side, so they cannot shrink the response") checks out against the code: _local_list sends only {dir, recurse, split, full_info}, and both --name filtering and --limit slicing happen after _userdata_request has already read the whole body — which is where _ResponseTooLarge is raised. So those flags genuinely cannot prevent the cap from tripping, and the hint redirects to the one thing that does (pruning the userdata dir) rather than dead-ending.
  • Verified the hints' suggested command actually parses: comfy --json --where local workflow list → exits 0.
  • pytest tests/comfy_cli/command/test_workflow_saved.py45 passed; ruff format --check / ruff check → clean.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

@mattmillerai Thanks for the thorough triage. I’ll review the current scoped diff and validate the local operation-specific error hints independently—no lockfile hare-raising. 🐇

✅ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded PR authored by the agent-work loop bug Something isn't working cursor-review Request Cursor bot review size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant