fix(models): emit envelope/1 errors on every model download failure (BE-4217) - #581
Conversation
`comfy --json model download` predated the renderer.error() envelope infra:
its failure paths either exited 1 with no envelope (so the local MCP saw
`code=None` and a raw stderr tail) or, worse, exited 0 with no envelope at
all — which the MCP's plain_ok synthesizer turns into a synthesized SUCCESS
for a download that never happened.
Every failure now funnels through a single `_download_failure()` helper that
emits an `envelope/1` error and exits 1:
- file already exists -> model_file_exists (was exit 0)
- Hugging Face 401, no token -> hf_unauthorized (was exit 0)
- no resolvable filename -> missing_argument (was a bare Exit(1)
/ an unhandled DownloadException)
- transfer failure -> download_failed
- local write failure (OSError) -> download_failed (was a traceback)
- CivitAI metadata lookup fails -> download_failed, details.stage=resolve
(was a traceback)
- Hugging Face download / hub install fails -> download_failed (was a traceback)
Registers `model_file_exists` and `hf_unauthorized`, and widens the existing
`download_failed` entry to cover model downloads. The success path is
unchanged (prints + exit 0, no envelope) because the MCP synthesizer depends
on exit-0-no-envelope meaning success.
The ticket also proposed a `model_source_unknown` hard stop on the "Model
source is unknown" branch. That branch is NOT an error: it is how a plain
direct file URL is downloaded, verified live (a raw.githubusercontent.com URL
downloads 35823 bytes, exit 0) and covered by the pre-existing direct-URL
tests. Stopping there would break direct downloads and contradict the
ticket's own acceptance case (a 404 direct URL must yield download_failed),
so the branch keeps working and only its misleading log line was corrected.
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: 41 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 (3)
📝 WalkthroughWalkthroughChangesThe model download command now validates filenames and CivitAI metadata, scrubs sensitive URL data, and routes download failures through structured error envelopes. Hugging Face filename handling and authorization errors were updated, while HTTP content-length parsing now tolerates invalid values. Model download hardening
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
size:L
![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 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 4 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…(BE-4217) Follow-ups to the review panel on #581: - Honour `--filename` on the *gated* Hugging Face path. `hf_hub_download` names the file after the repo path, so `--filename` was silently ignored there while the public-HF path (via `download_file`) honoured it. That split made the `local_filepath.exists()` guard inspect a path the branch never writes, and made the new `model_file_exists` hint ("pass `--filename`") impossible to act on. The result is now moved to the requested name. - Reject path components that escape the workspace. `local_filename` (from `--filename`, or from the CivitAI API's `file["name"]`, which is the accepted default in non-interactive runs) and `basemodel` (`version["baseModel"]`) are joined into the destination without sanitisation; `pathlib` does not neutralise `..` or an absolute component. Both are now validated, with the hint pointing at `--relative-path` — the option that exists for choosing a directory, so the capability is redirected rather than removed. - Scrub credentials out of the URL before it is echoed. CivitAI links carry the token as `?token=`; `tracking._scrub_value` already strips it before telemetry, and an error envelope is a louder channel than telemetry. Query, fragment and userinfo are dropped from every `details.url` and log line. - Restore `escape()` on interpolated values. `print` is Rich's markup-parsing print, so a URL holding `[/]` (or an IPv6 literal like `http://[::1]/x`) raised MarkupError out of the *logging* — an uncaught crash with no envelope, which is the exact failure this command promises not to have. - Guard `int(Content-Length)` in `_download_file_httpx`: a non-numeric header from a broken proxy raised ValueError past both handlers. Root-caused there, plus a catch-all backstop at the call site so no downloader failure can end the command without an envelope. - Drop the ineffective `patch("comfy_cli.tracking.track_command", ...)` from the tests: `download` is decorated at import time, so patching the factory afterwards never rebinds the wrapper. The HF auto-install targeting the workspace interpreter (Low finding) is left as-is — it is pinned by tests/comfy_cli/test_models_python_resolution.py and changing it is a separate decision; deferred to a follow-up.
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
This PR currently conflicts with Please rebase (or merge |
Resolves a semantic conflict with main's `--background` download feature (#728d11c) and the version-switch invalid_argument hint tightening (#606): - comfy_cli/command/models/models.py: combine main's needs_hf_auth/background restructuring with this branch's error-envelope hardening (hf_hub_download install/download/rename failures now all raise _download_failure, same as before the refactor moved). - comfy_cli/error_codes.py: keep main's generalized path-segment hint (used by multiple commands, not just model download) while keeping this branch's detail about the CivitAI-sourced filename/basemodel case. - tests/comfy_cli/command/test_model_download_background.py: main's TestSubmitFailsFast tests asserted the pre-envelope failure shape (DownloadException / bare print+return) for paths this branch converts to typer.Exit + envelope; updated to match the new (intended) behavior. The fail-fast invariant they exist to guard — nothing detaches before resolution succeeds — is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/models/models.py`:
- Around line 568-572: Update the move condition in the download flow to use the
resolved local name/path, including names entered through the interactive
prompt, rather than checking only filename. Keep the existing local_filepath
comparison and move behavior, and ensure the existence guard and
model_file_exists hint refer to the same destination that the branch writes.
In `@tests/comfy_cli/command/models/test_model_download_json.py`:
- Around line 112-116: Update the test helper _invoke to accept
check_civitai_url and check_huggingface_url through its existing **patches
override mechanism, supplying the current mocked defaults and allowing
caller-provided patches to take precedence. Replace the hard-coded patches in
the with block, preserving existing behavior while enabling CivitAI and
HuggingFace tests to override these checks without rebuilding the patch stack.
In `@tests/comfy_cli/command/test_model_download_background.py`:
- Around line 515-556: Update all four failure-path tests in the background
download suite to capture the raised typer.Exit exception and assert its
exit_code is nonzero, rather than only asserting the exception type. Apply this
to the tests for unresolved filenames, existing destinations, unresolvable
filenames, and missing Hugging Face tokens while preserving their existing
output assertions.
🪄 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: e42eb063-a778-4053-879d-e7ca03922f3b
📒 Files selected for processing (6)
comfy_cli/command/models/models.pycomfy_cli/error_codes.pycomfy_cli/file_utils.pytests/comfy_cli/command/models/test_model_download_json.pytests/comfy_cli/command/test_model_download_background.pytests/test_file_utils_network.py
…name (BE-4217)
Three CodeRabbit findings from the last review round:
- `models.py`: gate the post-`hf_hub_download` move on the *resolved*
`local_filename` instead of `filename is not None`. The name can equally come
from the interactive prompt (whose default is only a suggestion), so keying on
the flag left the same split open via the prompt door: the artifact landed at
the repo path while the `local_filepath.exists()` guard and its
`model_file_exists` hint ("pass `--filename`") both described a path this
branch never wrote. New regression test covers the prompted-name case, and it
fails against the old condition.
- `test_model_download_json.py`: fold `check_civitai_url` /
`check_huggingface_url` into `_invoke`'s `**patches` mechanism (defaults
merged, caller wins) and convert the five tests that were rebuilding the whole
patch stack by hand.
- `test_model_download_background.py`: pin `exit_code == 1` on the four
`TestSubmitFailsFast` blocks. `typer.Exit` defaults to 0, so they passed even
if a regression started exiting successfully on these failure paths — the very
"exit 0 with no envelope" bug this PR exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Rebase done — Since your note I also cleared the three remaining CodeRabbit threads in fc4081e (HF result now moves to the resolved filename rather than only when Ready for your pass whenever it suits, @bigcat88. |
3 participants
ELI-5
If you ask the CLI to download a model and it fails, it should say so in a way a
program can understand — a JSON error with a code — and it should exit with a
failure code.
Two of the failure cases didn't do that. If the file was already there, or if
Hugging Face said "you need a token," the CLI printed a message and exited 0
— i.e. "success." The local MCP wrapper sees "exit 0, no JSON" and synthesizes
a success envelope, so an agent was told "download complete" for a download
that never happened. Other failures exited 1 but with no JSON at all, so the MCP
could only forward a raw stderr tail with
code=None.Now every failure emits one
envelope/1error object with a realerror.codeand exits 1. Success is untouched (prints + exit 0, no envelope) because the
MCP's synthesizer depends on that meaning success.
What changed
All failure paths in
comfy_cli/command/models/models.pydownloadfunnelthrough one
_download_failure()helper (emitrenderer.error(...), return thetyper.Exit(1)to raise):model_file_existshf_unauthorizedmissing_argumenttyper.Exit(1), no messagemissing_argumentDownloadExceptiontracebackDownloadException)download_failedOSError)download_faileddownload_failed+details.stage="resolve"huggingface_hubinstall faileddownload_failedRegistry (
comfy_cli/error_codes.py): addsmodel_file_existsandhf_unauthorized; widens the existingdownload_failedentry to cover modeldownloads (reused rather than adding a fourth code, per the ticket's
"reuse an exact fit" instruction).
escape()around theDownloadExceptionmessage is dropped:error_panelbuilds its body from
rich.text.Text, which is literal, so markupmetacharacters (
[/],[id]) survive without aMarkupError. The existingmarkup regression test still passes, and a new JSON-mode test asserts the raw
message reaches
error.messagebyte-for-byte.Acceptance criteria — verified live against the real CLI
Run in a temp workspace on this branch:
tests/comfy_cli/output/test_error_code_registry.pypasses (criterion 4 ✅),as does the full suite: 2780 passed, 37 skipped;
ruff check+ruff formatclean on the CI-pinned ruff 0.15.15.
Judgment call — the
model_source_unknownhard stop was NOT shippedThe ticket's plan item 2 asked for a
model_source_unknowncode on theelse: print("Model source is unknown")branch, made to stop (exit 1)instead of falling through.
That branch is not an error path — it is how a plain direct file URL is
downloaded. Before shipping a capability denial I went and attempted the
capability by every path the CLI offers:
comfy --json model download --url https://raw.githubusercontent.com/Comfy-Org/comfy-cli/main/LICENSE --relative-path models/checkpoints --filename direct.bin→ exit 0,35823bytes written to disk. This URL is neithercivitai.comnorhuggingface.co, so it takes exactly the branch the ticket wanted to turn into a dead end.main:TestDownloadCommandDownloaderOptionandTestDownloadCommandErrorHandlingintests/comfy_cli/command/models/test_models.pypatch both host checks toFalseand then assertdownload_fileis called and"Done in "is printed — the direct-URL path is already the tested contract.<404-url>(a direct URL) and requireserror.code == "download_failed". Amodel_source_unknownstop would have producedmodel_source_unknowninstead, so the plan item and the acceptance criterion are mutually exclusive.So the branch keeps working. What I did change is its misleading log line, which
is what made it read like an error path in the first place:
Model source is unknown→Model source is unknown; treating <url> as a direct file URL(stderr in
--jsonmode, so no result-shape change). Nomodel_source_unknowncode was registered — the registry test enforces both directions, so an
unraised code would fail CI anyway.
The underlying JSON-mode concern the plan item was reaching for — "there is no
interactive prompt to save it in
--jsonmode" — is real, and it is handled atthe place it actually bites: when no filename can be resolved (prompt skipped or
cancelled), the command now errors with
missing_argument+pass --filenameinstead of a bare exit or a traceback.
Other notes for review
not just
--json. The ticket offered keeping exit 0 for TTY users but calledexit-1-in-both "the simplest consistent choice," and acceptance criterion 2
states it unqualified. A script that re-ran a download to no-op on an existing
file will now see a failure exit — the friendly message is preserved (as the
red error panel with the same
File already exists: <path>text).because they violated the same invariant by crashing with a traceback and no
envelope: CivitAI metadata resolution, the Hugging Face download/
huggingface_hubinstall, and a local
OSErroron write. Each is a boundedtry/exceptthatre-raises as
typer.Exit(1)after emitting the envelope; the broadexcept Exceptionclauses cannot swallow Ctrl-C (cancellation.pyraisesKeyboardInterrupt, aBaseException).print("Error parsing CivitAI model URL")insidecheck_civitai_url'sexcept. It is a pure predicate returning a tuple, andevery outcome downstream of it now lands on a covered path (direct download →
download_failed, or CivitAI resolve →download_failed), so the invariantholds without changing that function's contract.
_download_failure'scodeis keyword-only on purpose: the registry testAST-scans for
code="..."keyword arguments, so a positional call site wouldsilently drop out of the both-ways registry enforcement.
tests/comfy_cli/command/models/test_model_download_json.py(12cases). They pin the renderer's machine/pretty streams to explicit buffers
rather than relying on
CliRunner'smix_stderr, which is a default inClick 8.1 and removed in 8.2 — so the "exactly one envelope on stdout, human
logs on stderr" assertion holds on either Click.