[PR #174] [MERGED] fix(errors): sanitise the string `detail` and `error.message` forms #174

New issue

Closed

opened 2026-10-31 22:02:03 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:02:03 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/174
Author: @mattmillerai
Created: 9/19/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15951-sanitise-string-detail

📝 Commits (7)

2096320 fix(errors): sanitise and unify the Router detail[] validation summary
9525758 merge main, and drop the detail[] reroute the panel flagged
4c451b3 fix(errors): sanitise the string detail and error.message forms
c491678 Merge branch 'main' into matt/be-15951-sanitise-string-detail
e87d842 fix(errors): do not charge the summary budget for detail entries that sanitise away
bafc17c test(router-spec): pin credits_used to the contract header the spec now declares
40332dc Merge remote-tracking branch 'origin/main' into matt/be-15951-sanitise-string-detail

📊 Changes

7 files changed (+387 additions, -31 deletions)

View changed files

📝 CHANGELOG.md (+16 -0)
📝 src/comfy_low/errors.py (+87 -27)
📝 src/comfy_sdk/router_exceptions.py (+21 -4)
📝 tests/conftest.py (+8 -0)
📝 tests/test_error_mapping.py (+114 -0)
📝 tests/test_models_run.py (+36 -0)
📝 tests/test_router_exceptions.py (+105 -0)

📄 Description

STACKED — merging lands on matt/be-15633-run-422-sanitise-detail (owned by @mattmillerai, PR #167), NOT main. Do not merge this before #167. Its hunks in error_from_envelope and error_from_response sit directly against the lines this PR changes, so basing on main would have conflicted; GitHub will retarget this PR to main when #167 merges and its branch is deleted.

ELI-5

When a request fails, the SDK builds an exception out of whatever the server said. Some of that text was already being cleaned up before it was shown — control characters turned into spaces, everything squashed onto one line, cut off at 256 characters — because an error message is printed into a terminal and a log, and a server (or a proxy, or a model provider standing behind one) can put anything at all in it. But only some of it. Three of the ways the same human-readable string arrives were still passed through untouched, so a detail full of terminal escape codes, a right-to-left override and ten thousand xes landed in your traceback exactly as sent. Now all of them get the same clean-up.

What changed

Four builders handed a server-, proxy- or provider-supplied string to str(exc) with at most a .strip():

file	function	site
`src/comfy_low/errors.py`	`error_from_envelope`	the v2 envelope's `error.message`
`src/comfy_low/errors.py`	`error_from_envelope`	Router's request-level string `detail`
`src/comfy_sdk/router_exceptions.py`	`error_from_response`	`detail = raw_detail or None`
`src/comfy_sdk/router_exceptions.py`	`error_from_completion`	`detail = raw_detail or None`

All four now go through comfy_low.errors.clean_body_excerpt — the same function the detail[] array summary (#167) and every body excerpt already use: Cc/Cf/Co/Cs characters replaced with spaces, whitespace collapsed to a single line, and a 256-character cap, with only a bounded head of the input examined so the cost of describing a body does not scale with the body.

Deliberately not changed:

The CODE fields stay on _clean — error.code, the Router bucket (error_type, header and body), a cancel refusal's status. Those are wire tokens that get compared and branched on; clean_body_excerpt's whitespace collapse and 256-char cap would change a value rather than make it printable.
ValidationErrorDetail.msg and RouterError.errors / ApiError.validation_errors — those are data, raw by design. See Residual.
clean_body_excerpt is imported from the module (from comfy_low.errors import ...) exactly as clean_request_id and summarise_detail already are. No new public re-export.

Docs: the error_from_envelope docstring now states that both detail spellings and error.message are reduced; _clean's docstring says what is left to it and why; the RouterError.detail attribute comment says the value is sanitised and capped at 256 when it came off the wire (and that constructing the class by hand bypasses that); one Fixed entry under ## [Unreleased].

Behaviour change

clean_body_excerpt returns None for a non-string or a blank input, exactly as _clean did, so the detail[] list guard, the HTTP <status> fallback and the message-found body_excerpt gate all behave as before. One difference is real and intended: a whitespace-only detail now reads as absent on both router builders and falls to the existing fallback (HTTP 502, or the request completed with error_type '<bucket>') instead of surfacing as a blank description. That matches what error_from_envelope already did for the same input. A detail made entirely of unprintable characters ("\x00\x00") reduces to nothing the same way, and the body excerpt is then kept rather than dropped — strictly more information than before.

How this was tested

Red-then-green, verified by reverting only the two source files and re-running: 10 of the 14 new tests fail without the change (the other 4 assert pre-existing invariants the change must not break).

tests/test_error_mapping.py — error_from_envelope parametrised over the Router string detail and the v2 error.message, with HOSTILE = "\x1b[31mBAD\x1b[0m\n‮reversed\x00" + "x" * 10_000: no newline, no \x1b, no ‮, every character outside {Cc, Cf, Co, Cs}, len(err.message) == _BODY_EXCERPT_LIMIT, err.body_excerpt is None. Plus: the words survive the reduction (no healthy\x00upstream\n → no healthy upstream), a blank/zero-width-only detail falls back to HTTP 502, and a hostile message does not disturb error.code.
tests/test_router_exceptions.py — the same assertions parametrised over error_from_response(502, …) and error_from_completion(…, request_id="req_1"), on .detail and on str(exc); a " " detail yielding each builder's fallback; and .errors[0].msg confirmed still raw while .detail is bounded.
tests/test_models_run.py — one end-to-end case through the stubbed server, proving the whole models.run → transport.parse_or_raise → error_from_envelope → to_sdk_error chain (the only path the SDK itself takes to a string detail; error_from_response has no in-tree caller). Needed one new knob on the fake server, ServerState.model_run_error_detail, so a test can choose the failure's human-readable string.

Every pre-existing message-equality assertion passes unchanged — no expected string was edited and the sanitiser was not weakened.

Provenance

Authored by: agent-work loop
Verified: After merging main and the two review-round commits: uv run --extra dev pytest -q: 1029 passed, 10 skipped; ruff check .: all checks passed; ruff format --check .: 57 files already formatted; mypy src: no issues in 21 source files. All 12 PR checks green, including Test (py3.10/3.11/3.12/3.13), comfy_low codegen drift and public-repo-hygiene. Red-then-green confirmed twice: reverting the two original source files gives 10 failed, 5 passed; reverting the summarise_detail budget fix alone fails its new regression test.
Deviations: The original task was implemented as written. Two changes were added in the review round, both beyond the original scope:
- summarise_detail charged its length budget for detail[] entries measured before sanitisation, so one entry of control characters could exhaust the budget, reduce to nothing, and cost the caller every readable entry behind it — a bare HTTP 422. Reported by CodeRabbit, reproduced end to end, fixed with a regression test.
- main was already red at c0c4a33 on its own, independently of this branch: #166 added the credits_used lift while its header was undeclared and #177 synced a spec that declares it, tripping the deliberate tripwire in test_router_spec_contract.py. Merging main brings that failure here, so it is reconciled the way the tripwire's own message prescribes — the lift is moved into _CONTRACT_HEADER_LIFTS, which strengthens the pin rather than removing it. No base-branch assertion was deleted or weakened.

Residual

Not fixed here, and worth its own look.

ValidationErrorDetail.msg and RouterError.errors / ApiError.validation_errors still carry the server's bytes verbatim, and nothing bounds them. This is deliberate — they are typed data a caller branches on, not display text, and the task explicitly scoped them out — but it is a real remaining exposure on the same class of input this PR is about: a caller who prints exc.errors[0].msg or logs exc.validation_errors directly gets the unreduced string, including escape sequences and unbounded length. summarise_detail protects only the summary it builds. If that is to be closed, the answer is probably a documented display accessor rather than sanitising the data, since sanitising it would destroy the field's value as data.
RouterError(detail=...) constructed by hand is not sanitised. The reduction lives in the three builders, not in __init__, so anything outside this module that constructs the class directly with server text is still exposed. I documented that on the attribute rather than moving the reduction into the constructor (which would also reduce the SDK's own status-derived fallback sentences). No in-tree caller does this today.
A whitespace-only detail is a silent behaviour change for anyone who was reading it. It is in the changelog, but it is the one thing in this PR a downstream assertion could notice.

Not exercised.

No live Router or v2 deployment was contacted. Everything is verified against the repo's stdlib fake server and against the builders directly, per this repo's no-network test rule (AGENTS.md). The claim that a real intermediary sends such a detail is inference from the shapes the code already handles, not something I observed on live traffic.
The upstream investigation note this task was written from was not reachable from this environment. The four sites and the reproducer string were taken from the task text, but I did not take them on trust: reverting the source change makes the hostile-input tests fail on every one of the four, which is the same evidence independently obtained.
Negative-claim falsification: not triggered. The diff denies no capability — it adds no not supported / unavailable / STOP string, no throw or deny dead-end, and flips no test to assert one. The only reachability change is a whitespace-only detail routing to an existing fallback that already produced a useful message.
Sweep of the half not fixed. I swept src/comfy_low/ and src/comfy_sdk/ for every read of a server-supplied string that reaches a user-visible message. Four were display text and all four are fixed here; zero unsanitised display-text sites remain. Nine _clean-only reads remain and are all code tokens kept on purpose (error.code, the bucket in four places, a cancel status, the two error_type/code reads in retry.py and model_requests.py). Every other .strip() in the two packages validates caller input, not wire text.

Summary by CodeRabbit

Bug Fixes
- Error messages from Router responses and model runs are now displayed as printable, single-line text, with control and formatting characters removed and long messages limited to 256 characters.
- Validation summaries include field locations, skip entries that contain no readable text, and prioritize readable entries within the display limit. Original validation details and error codes remain unchanged.
- Blank or unreadable messages fall back to the HTTP status or completion error type.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-python-sdk/pull/174 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15951-sanitise-string-detail` --- ### 📝 Commits (7) - [`2096320`](https://github.com/Comfy-Org/comfy-python-sdk/commit/2096320b057367f1cd038c38d3787ec4f707aa44) fix(errors): sanitise and unify the Router detail[] validation summary - [`9525758`](https://github.com/Comfy-Org/comfy-python-sdk/commit/9525758506a914a54e12416feafe5a8d7c096b3d) merge main, and drop the detail[] reroute the panel flagged - [`4c451b3`](https://github.com/Comfy-Org/comfy-python-sdk/commit/4c451b3881f38b17825ee193e77f71de8b2def8c) fix(errors): sanitise the string `detail` and `error.message` forms - [`c491678`](https://github.com/Comfy-Org/comfy-python-sdk/commit/c49167898bf961244a09909ff3e50909e0e08a3f) Merge branch 'main' into matt/be-15951-sanitise-string-detail - [`e87d842`](https://github.com/Comfy-Org/comfy-python-sdk/commit/e87d842e2d2423a338c22d2b130e1232a168e322) fix(errors): do not charge the summary budget for detail entries that sanitise away - [`bafc17c`](https://github.com/Comfy-Org/comfy-python-sdk/commit/bafc17c47f5063ce8df37794cead1b5a82085302) test(router-spec): pin credits_used to the contract header the spec now declares - [`40332dc`](https://github.com/Comfy-Org/comfy-python-sdk/commit/40332dc94ea15f2e8733128b59522eff77a03ef5) Merge remote-tracking branch 'origin/main' into matt/be-15951-sanitise-string-detail ### 📊 Changes **7 files changed** (+387 additions, -31 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+16 -0) 📝 `src/comfy_low/errors.py` (+87 -27) 📝 `src/comfy_sdk/router_exceptions.py` (+21 -4) 📝 `tests/conftest.py` (+8 -0) 📝 `tests/test_error_mapping.py` (+114 -0) 📝 `tests/test_models_run.py` (+36 -0) 📝 `tests/test_router_exceptions.py` (+105 -0) </details> ### 📄 Description **STACKED — merging lands on `matt/be-15633-run-422-sanitise-detail` (owned by @mattmillerai, PR #167), NOT `main`.** Do not merge this before #167. Its hunks in `error_from_envelope` and `error_from_response` sit directly against the lines this PR changes, so basing on `main` would have conflicted; GitHub will retarget this PR to `main` when #167 merges and its branch is deleted. ## ELI-5 When a request fails, the SDK builds an exception out of whatever the server said. Some of that text was already being cleaned up before it was shown — control characters turned into spaces, everything squashed onto one line, cut off at 256 characters — because an error message is printed into a terminal and a log, and a server (or a proxy, or a model provider standing behind one) can put anything at all in it. But only *some* of it. Three of the ways the same human-readable string arrives were still passed through untouched, so a `detail` full of terminal escape codes, a right-to-left override and ten thousand `x`es landed in your traceback exactly as sent. Now all of them get the same clean-up. ## What changed Four builders handed a server-, proxy- or provider-supplied string to `str(exc)` with at most a `.strip()`: | file | function | site | |---|---|---| | `src/comfy_low/errors.py` | `error_from_envelope` | the v2 envelope's `error.message` | | `src/comfy_low/errors.py` | `error_from_envelope` | Router's request-level string `detail` | | `src/comfy_sdk/router_exceptions.py` | `error_from_response` | `detail = raw_detail or None` | | `src/comfy_sdk/router_exceptions.py` | `error_from_completion` | `detail = raw_detail or None` | All four now go through `comfy_low.errors.clean_body_excerpt` — the same function the `detail[]` array summary (#167) and every body excerpt already use: `Cc`/`Cf`/`Co`/`Cs` characters replaced with spaces, whitespace collapsed to a single line, and a 256-character cap, with only a bounded head of the input examined so the cost of describing a body does not scale with the body. Deliberately **not** changed: - **The CODE fields stay on `_clean`** — `error.code`, the Router bucket (`error_type`, header and body), a cancel refusal's `status`. Those are wire tokens that get compared and branched on; `clean_body_excerpt`'s whitespace collapse and 256-char cap would change a value rather than make it printable. - **`ValidationErrorDetail.msg` and `RouterError.errors` / `ApiError.validation_errors`** — those are data, raw by design. See Residual. - `clean_body_excerpt` is imported from the module (`from comfy_low.errors import ...`) exactly as `clean_request_id` and `summarise_detail` already are. No new public re-export. Docs: the `error_from_envelope` docstring now states that *both* `detail` spellings and `error.message` are reduced; `_clean`'s docstring says what is left to it and why; the `RouterError.detail` attribute comment says the value is sanitised and capped at 256 when it came off the wire (and that constructing the class by hand bypasses that); one Fixed entry under `## [Unreleased]`. ## Behaviour change `clean_body_excerpt` returns `None` for a non-string or a blank input, exactly as `_clean` did, so the `detail[]` list guard, the `HTTP <status>` fallback and the message-found `body_excerpt` gate all behave as before. One difference is real and intended: **a whitespace-only `detail` now reads as absent** on both router builders and falls to the existing fallback (`HTTP 502`, or `the request completed with error_type '<bucket>'`) instead of surfacing as a blank description. That matches what `error_from_envelope` already did for the same input. A `detail` made entirely of unprintable characters (`"\x00\x00"`) reduces to nothing the same way, and the body excerpt is then kept rather than dropped — strictly more information than before. ## How this was tested Red-then-green, verified by reverting only the two source files and re-running: 10 of the 14 new tests fail without the change (the other 4 assert pre-existing invariants the change must not break). - `tests/test_error_mapping.py` — `error_from_envelope` parametrised over the Router string `detail` and the v2 `error.message`, with `HOSTILE = "\x1b[31mBAD\x1b[0m\n‮reversed\x00" + "x" * 10_000`: no newline, no `\x1b`, no `‮`, every character outside `{Cc, Cf, Co, Cs}`, `len(err.message) == _BODY_EXCERPT_LIMIT`, `err.body_excerpt is None`. Plus: the words survive the reduction (`no healthy\x00upstream\n` → `no healthy upstream`), a blank/zero-width-only `detail` falls back to `HTTP 502`, and a hostile `message` does not disturb `error.code`. - `tests/test_router_exceptions.py` — the same assertions parametrised over `error_from_response(502, …)` and `error_from_completion(…, request_id="req_1")`, on `.detail` and on `str(exc)`; a `" "` detail yielding each builder's fallback; and `.errors[0].msg` confirmed still raw while `.detail` is bounded. - `tests/test_models_run.py` — one end-to-end case through the stubbed server, proving the whole `models.run` → `transport.parse_or_raise` → `error_from_envelope` → `to_sdk_error` chain (the only path the SDK itself takes to a string `detail`; `error_from_response` has no in-tree caller). Needed one new knob on the fake server, `ServerState.model_run_error_detail`, so a test can choose the failure's human-readable string. Every pre-existing message-equality assertion passes unchanged — no expected string was edited and the sanitiser was not weakened. ## Provenance - **Authored by:** agent-work loop - **Verified:** After merging `main` and the two review-round commits: `uv run --extra dev pytest -q`: 1029 passed, 10 skipped; `ruff check .`: all checks passed; `ruff format --check .`: 57 files already formatted; `mypy src`: no issues in 21 source files. All 12 PR checks green, including `Test (py3.10/3.11/3.12/3.13)`, `comfy_low codegen drift` and `public-repo-hygiene`. Red-then-green confirmed twice: reverting the two original source files gives 10 failed, 5 passed; reverting the `summarise_detail` budget fix alone fails its new regression test. - **Deviations:** The original task was implemented as written. Two changes were added in the review round, both beyond the original scope: - `summarise_detail` charged its length budget for `detail[]` entries measured *before* sanitisation, so one entry of control characters could exhaust the budget, reduce to nothing, and cost the caller every readable entry behind it — a bare `HTTP 422`. Reported by CodeRabbit, reproduced end to end, fixed with a regression test. - `main` was already red at `c0c4a33` on its own, independently of this branch: #166 added the `credits_used` lift while its header was undeclared and #177 synced a spec that declares it, tripping the deliberate tripwire in `test_router_spec_contract.py`. Merging `main` brings that failure here, so it is reconciled the way the tripwire's own message prescribes — the lift is moved into `_CONTRACT_HEADER_LIFTS`, which *strengthens* the pin rather than removing it. No base-branch assertion was deleted or weakened. ## Residual **Not fixed here, and worth its own look.** - **`ValidationErrorDetail.msg` and `RouterError.errors` / `ApiError.validation_errors` still carry the server's bytes verbatim, and nothing bounds them.** This is deliberate — they are typed data a caller branches on, not display text, and the task explicitly scoped them out — but it is a real remaining exposure on the *same* class of input this PR is about: a caller who prints `exc.errors[0].msg` or logs `exc.validation_errors` directly gets the unreduced string, including escape sequences and unbounded length. `summarise_detail` protects only the summary it builds. If that is to be closed, the answer is probably a documented display accessor rather than sanitising the data, since sanitising it would destroy the field's value as data. - **`RouterError(detail=...)` constructed by hand is not sanitised.** The reduction lives in the three builders, not in `__init__`, so anything outside this module that constructs the class directly with server text is still exposed. I documented that on the attribute rather than moving the reduction into the constructor (which would also reduce the SDK's own status-derived fallback sentences). No in-tree caller does this today. - **A whitespace-only `detail` is a silent behaviour change for anyone who was reading it.** It is in the changelog, but it is the one thing in this PR a downstream assertion could notice. **Not exercised.** - **No live Router or v2 deployment was contacted.** Everything is verified against the repo's stdlib fake server and against the builders directly, per this repo's no-network test rule (`AGENTS.md`). The claim that a real intermediary sends such a `detail` is inference from the shapes the code already handles, not something I observed on live traffic. - **The upstream investigation note this task was written from was not reachable from this environment.** The four sites and the reproducer string were taken from the task text, but I did not take them on trust: reverting the source change makes the hostile-input tests fail on every one of the four, which is the same evidence independently obtained. - **Negative-claim falsification: not triggered.** The diff denies no capability — it adds no `not supported` / `unavailable` / `STOP` string, no throw or deny dead-end, and flips no test to assert one. The only reachability change is a whitespace-only `detail` routing to an *existing* fallback that already produced a useful message. - **Sweep of the half not fixed.** I swept `src/comfy_low/` and `src/comfy_sdk/` for every read of a server-supplied string that reaches a user-visible message. Four were display text and all four are fixed here; **zero** unsanitised display-text sites remain. Nine `_clean`-only reads remain and are all code tokens kept on purpose (`error.code`, the bucket in four places, a cancel `status`, the two `error_type`/`code` reads in `retry.py` and `model_requests.py`). Every other `.strip()` in the two packages validates caller input, not wire text.  ## Summary by CodeRabbit * **Bug Fixes** * Error messages from Router responses and model runs are now displayed as printable, single-line text, with control and formatting characters removed and long messages limited to 256 characters. * Validation summaries include field locations, skip entries that contain no readable text, and prioritize readable entries within the display limit. Original validation details and error codes remain unchanged. * Blank or unreadable messages fall back to the HTTP status or completion error type.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:02:03 +00:00