fix(models): strip ANSI, not just Rich markup, from download-server error text (BE-5023)

[mattmillerai]

STACKED — merging lands on matt/be-4794-sanitize-ansi (owned by @mattmillerai, PR #614), NOT main. This PR is based on #614's branch because it uses comfy_cli/output/sanitize.py and the pretty / assert_inert() test harness, neither of which exists on main yet. Review the two-commit diff, or wait for #614 to merge and GitHub will retarget this to main automatically. Do not merge this before #614.

ELI-5

When a model download fails, the CLI prints the reason the server gave us. A hostile (or impersonated) model host gets to pick that text — and it can hide invisible terminal control codes in it that clear your screen, rename your terminal window, or scribble over lines the CLI already printed to make them say something else. We were already defusing one half of that (Rich markup), but not the escape codes themselves. Now we strip both, at the point the server's text enters the CLI and again where it is printed.

What was wrong

comfy_cli/command/models/models.py rendered download-server error text with rich.markup.escape(). The inline comment said what it was for: stopping a MarkupError from a stray [/]. It does that, but escape() does not touch raw ANSI/control bytes, so server-chosen escape sequences still reached the terminal live.

The remote text arrives via guess_status_code_reason() in comfy_cli/file_utils.py. Most branches return a canned per-status string and are safe; the 401 branch interpolates the server's own JSON message. That flows into DownloadException and out through the two print sites. Reproduced on origin/main:

body = json.dumps({"message": "\x1b[2J\x1b]0;PWNED\x07denied"}).encode()
reason = guess_status_code_reason(401, body)   # contains a raw ESC byte
# rendered through Rich as main does today:
#   escape()          -> CSI-2J reaches the terminal: True,  OSC-title: True
#   sanitize_markup() -> CSI-2J: False, OSC-title: False

Trigger: comfy model download --url <host> where that host answers 401 with a JSON body whose message carries escape bytes. A CivitAI-shaped mirror or a MITM'd model host is the realistic case.

What changed

Two layers, deliberately:

1. The boundary — guess_status_code_reason() (file_utils.py). The one branch that echoes server text now runs it through sanitize_value() before interpolating. Fixing it here means every consumer of the reason string benefits — the comfy model download error line, the persisted background-download state file, and comfy node install's ui.display_error_message() — rather than each print site having to remember. Markup escaping is deliberately not applied here: display_error_message renders with markup=False, so escaping backslashes would be visible on screen there.

2. The print sites — models.py:401 and :704. escape(str(x)) becomes sanitize_markup(x). sanitize_markup is sanitize_value + rich.markup.escape, so it is a strict superset of the old behavior: it keeps the existing MarkupError protection, adds ANSI/control-byte stripping, and does the str() coercion the old str(e) calls did (so those are dropped). Both layers earn their keep — the print sites still cover reasons this change does not sanitize at source, such as the message aria2 relays into a DownloadException, and the download-status / downloads row error, which is text read back from a state file a detached worker wrote.

sanitize_markup is a no-op on markup-free, escape-free text, so realistic messages render byte-identically. There is an explicit test for that.

Tests

Added to tests/comfy_cli/command/test_pretty_print_sanitize.py, reusing that file's pretty fixture (force-terminal renderer) and assert_inert() helper (no OSC 8 / OSC 0 / CSI 2J survives):

• test_guess_status_code_reason_401_strips_server_escapes — the boundary itself.
• test_guess_status_code_reason_leaves_benign_text_alone — the no-op guarantee on a realistic message.
• test_download_error_is_inert / ..._with_unbalanced_markup_does_not_crash — models.download() against a stubbed httpx.stream returning 401 with a hostile body. Nothing is faked between the response and the print site: the real guess_status_code_reason builds the reason, the real DownloadException carries it, the real except clause renders it.
• test_download_status_row_error_is_inert / ..._with_unbalanced_markup_does_not_crash — the per-row error line under the download-status / downloads table.

Verified failing before the fix: with the two source files reverted, the three ANSI assertions fail (AssertionError: escape byte survived: 'abc123: \x1b[2J\x1b]0;PWNED...'). The two unbalanced_markup tests pass both before and after by design — they pin the MarkupError protection escape() already provided, proving this change does not regress it.

Full suite: 3045 passed, 37 skipped. ruff check / ruff format --check clean on the touched files (the repo has 16 pre-existing ruff check findings and one unformatted file on the base branch under local ruff 0.12.7; this change adds none).

Judgment calls / not done

• Sanitizing at the file_utils boundary was the ticket's "also worth a look" suggestion, and I took it. It is the reason comfy node install is fixed too without touching custom_nodes/command.py. It is a behavior change to a shared function, so: it only ever removes C0/C1 control characters and ANSI sequences from an attacker-chosen substring, non-str values get the same str() coercion the f-string did, and the benign-text no-op is pinned by a test.
• Adjacent, deliberately out of scope — server-supplied CivitAI metadata is still rendered unsanitized through markup. request_civitai_model_version_api() returns download_url, model_name, and baseModel straight from the API response, and those reach markup-interpreting prints a few lines above the site fixed here (Start downloading URL: {url} into {local_filepath}, and File already exists: {local_filepath}). It is a real hazard of the same class, but a different one from this ticket's (error text), and it requires compromising the pinned civitai.com host rather than an arbitrary --url host, so it is materially weaker. Flagging rather than folding it in — happy to file it as a follow-up.
• file_utils.py's retry line (print(f"Download error (attempt ...)")) uses the builtin print, not the Rich one, and its text comes from _friendly_network_error(), which returns canned strings for HTTP statuses. No server-chosen text reaches a markup sink there, so it is untouched.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b70cc1bf-93de-46c5-aa4c-12f444ab09f1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

⚠️ Review failed

Judge call failed (status=parse_error): Could not parse JSON findings from output. First 500 chars:
I've verified all the findings against the actual code. Key confirmations:

- `sanitize_value`/`sanitize_markup` both coerce non-str via `str()` (sanitize.py:82) — so the three Gemini findings claiming a `TypeError` from passing `e`/`row['error']`/`msg_json["message"]` without `str()` are **false positives** (dropped).
- `guess_status_code_reason` is called at file_utils.py:335 *outside* any try; foreground `download()` (models.py:398) only catches `DownloadException` → non-dict `msg_json` and i

Re-trigger by removing and re-adding the cursor-review label.

[bigcat88]

This PR currently conflicts with main — GitHub reports mergeable: CONFLICTING, so it needs a rebase before I can review it and I'm skipping it in the current review sweep.

Please rebase (or merge main in) and I'll pick it up on the next pass. main moved a fair bit in the last day, including #614 (ANSI sanitisation across the pretty-print call sites) and #628 (the duplicate server_died error-code fix that had main red), so a refresh may also clear unrelated CI noise on this branch.