fix(models): emit envelope/1 errors on every model download failure (BE-4217)

[mattmillerai]

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/1 error object with a real error.code
and 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.py download funnel
through one _download_failure() helper (emit renderer.error(...), return the
typer.Exit(1) to raise):

Failure	Code	Before
Target file already exists	model_file_exists	red print, exit 0
Hugging Face 401 and no token	hf_unauthorized	print advice, exit 0
No resolvable filename	missing_argument	bare typer.Exit(1), no message
Empty filename	missing_argument	unhandled DownloadException traceback
Transfer failed (DownloadException)	download_failed	red print + exit 1, no envelope
Local write failed (OSError)	download_failed	traceback
CivitAI metadata lookup failed / no primary file	download_failed + details.stage="resolve"	traceback
Hugging Face download or huggingface_hub install failed	download_failed	traceback

Registry (comfy_cli/error_codes.py): adds model_file_exists and
hf_unauthorized; widens the existing download_failed entry to cover model
downloads (reused rather than adding a fourth code, per the ticket's
"reuse an exact fit" instruction).

escape() around the DownloadException message is dropped: error_panel
builds its body from rich.text.Text, which is literal, so markup
metacharacters ([/], [id]) survive without a MarkupError. The existing
markup regression test still passes, and a new JSON-mode test asserts the raw
message reaches error.message byte-for-byte.

Acceptance criteria — verified live against the real CLI

Run in a temp workspace on this branch:

$ comfy --json model download --url .../NO_SUCH_FILE_404.bin \
    --relative-path models/checkpoints --filename x.safetensors
{"schema":"envelope/1","ok":false,"command":"model download",...,
 "error":{"code":"download_failed","message":"Failed to download file.\nFile not found on server (404)",...}}
exit=1                                                              # criterion 1 ✅

$ comfy --json model download --url <same file, already on disk> ...
 "error":{"code":"model_file_exists",...,"details":{"path":"…/direct.bin"}}
exit=1                                                              # criterion 2a ✅

$ comfy --json model download --url https://huggingface.co/meta-llama/Llama-2-7b-hf/resolve/main/config.json ...
 "error":{"code":"hf_unauthorized",...,"details":{"repo_id":"meta-llama/Llama-2-7b-hf"}}
exit=1                                                              # criterion 2b ✅

$ comfy --json model download --url <a real direct file URL> ...
exit=0, stdout empty, 35823 bytes on disk                           # criterion 3 ✅

tests/comfy_cli/output/test_error_code_registry.py passes (criterion 4 ✅),
as does the full suite: 2780 passed, 37 skipped; ruff check + ruff format
clean on the CI-pinned ruff 0.15.15.

Judgment call — the model_source_unknown hard stop was NOT shipped

The ticket's plan item 2 asked for a model_source_unknown code on the
else: 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:

• Live run: comfy --json model download --url https://raw.githubusercontent.com/Comfy-Org/comfy-cli/main/LICENSE --relative-path models/checkpoints --filename direct.bin → exit 0, 35823 bytes written to disk. This URL is neither civitai.com nor huggingface.co, so it takes exactly the branch the ticket wanted to turn into a dead end.
• Existing tests on main: TestDownloadCommandDownloaderOption and TestDownloadCommandErrorHandling in tests/comfy_cli/command/models/test_models.py patch both host checks to False and then assert download_file is called and "Done in " is printed — the direct-URL path is already the tested contract.
• The ticket's own acceptance criterion 1 passes a <404-url> (a direct URL) and requires error.code == "download_failed". A model_source_unknown stop would have produced model_source_unknown instead, so the plan item and the acceptance criterion are mutually exclusive.
• The proposed hint text itself — "pass a direct file URL, a huggingface.co URL, or a civitai.com URL" — names direct file URLs as valid, i.e. the denial would have been denying a path it simultaneously recommended.

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 --json mode, so no result-shape change). No model_source_unknown
code 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 --json mode" — is real, and it is handled at
the place it actually bites: when no filename can be resolved (prompt skipped or
cancelled), the command now errors with missing_argument + pass --filename
instead of a bare exit or a traceback.

Other notes for review

• Intentional behavior change: file-exists now exits 1 in pretty mode too,
  not just --json. The ticket offered keeping exit 0 for TTY users but called
  exit-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).
• Beyond the ticket's six paths, three more failure branches were converted
  because they violated the same invariant by crashing with a traceback and no
  envelope: CivitAI metadata resolution, the Hugging Face download/huggingface_hub
  install, and a local OSError on write. Each is a bounded try/except that
  re-raises as typer.Exit(1) after emitting the envelope; the broad
  except Exception clauses cannot swallow Ctrl-C (cancellation.py raises
  KeyboardInterrupt, a BaseException).
• Not converted: the print("Error parsing CivitAI model URL") inside
  check_civitai_url's except. It is a pure predicate returning a tuple, and
  every outcome downstream of it now lands on a covered path (direct download →
  download_failed, or CivitAI resolve → download_failed), so the invariant
  holds without changing that function's contract.
• _download_failure's code is keyword-only on purpose: the registry test
  AST-scans for code="..." keyword arguments, so a positional call site would
  silently drop out of the both-ways registry enforcement.
• New tests: tests/comfy_cli/command/models/test_model_download_json.py (12
  cases). They pin the renderer's machine/pretty streams to explicit buffers
  rather than relying on CliRunner's mix_stderr, which is a default in
  Click 8.1 and removed in 8.2 — so the "exactly one envelope on stdout, human
  logs on stderr" assertion holds on either Click.

[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: 1 minute

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: af0d6a21-7d86-47a5-a02d-b30c0b908319

📥 Commits

Reviewing files that changed from the base of the PR and between 85b62da and c843355.

📒 Files selected for processing (5)

• comfy_cli/command/models/models.py
• comfy_cli/error_codes.py
• comfy_cli/file_utils.py
• tests/comfy_cli/command/models/test_model_download_json.py
• tests/test_file_utils_network.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-4217-model-download-error-envelopes

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-4217-model-download-error-envelopes

---

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

[github-actions]

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

[mattmillerai]

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

• BE-4272 — Make the huggingface_hub auto-install target the interpreter that will import it