Public error contract is untested, and two defects hide behind it: translating() skipped on all download paths, AsyncOutput.to_stream missing

[christian-byrne]

Summary

A test-coverage audit of origin/main (c96eb09) turned up two verified defects that share one
root cause: the guarantees the README and docstrings make about the public surface have no test
behind them. Everything else in the audit was clean — including codegen drift, which is guarded
and enforced as a required check (details at the bottom).

Both defects trace to the same code and the same guarantee, so they are batched here rather than
filed per-module.

---

Defect 1 — translating() is applied inconsistently; 10 public entry points leak the raw comfy_low.ApiError

comfy_sdk/exceptions.py::translating documents the contract:

Wrap the SDK-level operations that call comfy_low with this so integrators only ever catch
comfy_sdk exceptions (MissingAsset, HashMismatch, NotFound, ...), never the raw protocol error.

The README's "Typed errors" section makes the same promise: every error is a ComfyError subclass.

These entry points do not wrap, and raise comfy_low.errors.ApiError straight through:

Entry point	File
Output.to_file / to_stream / to_bytes / get_download_url	src/comfy_sdk/outputs.py — the module never imports translating at all
AsyncOutput.to_file / to_bytes / get_download_url	same
AssetFactory.get / AsyncAssetFactory.get	src/comfy_sdk/assets.py:271, :322
JobFactory.get / AsyncJobFactory.get	src/comfy_sdk/jobs.py:301, :309
Job.events() / AsyncJob.events() — the non-501 raise	src/comfy_sdk/jobs.py:516, :628

Why this bites harder than a normal type mismatch: comfy_low.errors and
comfy_sdk.exceptions both export a class named NotFound, and they are unrelated
(ApiError vs ComfyError). So a consumer writing the documented

from comfy_sdk import NotFound
try:
    out.to_file("result.png")
except NotFound:
    ...

silently never catches. And for the entity-specific server codes (job_not_found,
asset_not_found), comfy_low._BY_CODE has no entry either, so what escapes is a bare
comfy_low.errors.ApiError — not even the protocol-level NotFound.

This is the output-download path, i.e. the most-used consumer call after run().

Reproduction

Driven by this repo's own stub server (tests/conftest.py), no mocks. Controls first — two
entry points that are wrapped, on the same code paths, to show the harness detects correct
behaviour:

import os, sys
sys.path.insert(0, "tests")
import conftest as C

srv = C._start_server()
os.environ[C.BASE_URL_ENV_VAR] = srv.base_url

from comfy_sdk import Comfy
import comfy_sdk.exceptions as sdkx
import comfy_low.errors as lowx

client = Comfy()

def probe(label, fn):
    try:
        fn(); print(f"  {label:46s} no error")
    except sdkx.ComfyError as e:
        print(f"  {label:46s} comfy_sdk.{type(e).__name__}  OK")
    except lowx.ApiError as e:
        print(f"  {label:46s} comfy_low.{type(e).__name__}  *** LEAK ***")

wf = client.workflows.from_json({"3": {"class_type": "KSampler", "inputs": {}}})

print("controls (expected: comfy_sdk.*):")
srv.state.job_error = (422, "invalid_workflow")
probe("client.submit(wf)               [wrapped]", lambda: client.submit(wf))
srv.state.job_error = None
srv.state.job_workflow_not_found = True
job = client.submit(wf)
probe("job.get_workflow()              [wrapped]", lambda: job.get_workflow())
srv.state.job_workflow_not_found = False

print("suspects:")
client.assets.delete("asset_out_01")          # stub 404s a deleted asset thereafter
probe("client.assets.get(deleted id)", lambda: client.assets.get("asset_out_01"))

out = client.submit(wf).result().get_outputs("13")[0]
import io
probe("output.to_bytes()   (deleted asset)", lambda: out.to_bytes())
probe("output.to_file(...)  (deleted asset)", lambda: out.to_file("/dev/null"))
probe("output.to_stream(...) (deleted asset)", lambda: out.to_stream(io.BytesIO()))
probe("output.get_download_url() (deleted)", lambda: out.get_download_url())

client.close(); C._stop_server(srv)

Output:

controls (expected: comfy_sdk.*):
  client.submit(wf)               [wrapped]      comfy_sdk.InvalidWorkflow  OK
  job.get_workflow()              [wrapped]      comfy_sdk.NotFound  OK
suspects:
  client.assets.get(deleted id)                  comfy_low.NotFound  *** LEAK ***
  output.to_bytes()   (deleted asset)            comfy_low.NotFound  *** LEAK ***
  output.to_file(...)  (deleted asset)           comfy_low.NotFound  *** LEAK ***
  output.to_stream(...) (deleted asset)          comfy_low.NotFound  *** LEAK ***
  output.get_download_url() (deleted)            comfy_low.NotFound  *** LEAK ***

JobFactory.get needs a server that 404s a job (the stub returns a job for any id); against a
minimal 404-everything handler it leaks the same way:

  client.jobs.get(id)                        -> comfy_low.ApiError    *** LEAKED ***
  client.assets.get(id)                      -> comfy_low.ApiError    *** LEAKED ***

Suggested fix

Wrap the four outputs.py download methods (sync + async) and both factories' get() in
with translating():, and translate before the raise in events(). Then add the test that
would have caught it — one that asserts no comfy_low.ApiError escapes any public entry point.

---

Defect 2 — Output.to_stream has no AsyncOutput counterpart, and no test

src/comfy_sdk/outputs.py:89 defines Output.to_stream(...). AsyncOutput has no to_stream.

The README states:

Comfy and AsyncComfy expose the identical surface — swap the import and add await / async for.

and closes the downloads section with "(AsyncOutput mirrors all of the above with await.)".

A public-surface diff over every sync/async pair — 6 of 7 are identical, one is not:

Comfy           /AsyncComfy         sync-only=[] async-only=[]
Asset           /AsyncAsset         sync-only=[] async-only=[]
AssetFactory    /AsyncAssetFactory  sync-only=[] async-only=[]
Job             /AsyncJob           sync-only=[] async-only=[]
JobFactory      /AsyncJobFactory    sync-only=[] async-only=[]
Output          /AsyncOutput        sync-only=['to_stream'] async-only=[]  <-- ASYMMETRY
ComfyLow        /AsyncComfyLow      sync-only=[] async-only=[]

(the check normalises the intentional close/aclose rename)

to_stream is also the only download method with zero test references anywhere in tests/ —
which is why the asymmetry went unnoticed. Fix is async def to_stream on AsyncOutput plus a
parity test asserting the sync and async public method sets match.

---

The coverage checklist behind them

Ordered by consumer impact. These are gaps, not known bugs.

• A test asserting the translating() guarantee across every public entry point — the one that catches Defect 1 and any future regression of it.
• A sync/async parity test over the 7 class pairs — the one that catches Defect 2.
• AssetFactory.from_url / AsyncAssetFactory.from_url — zero tests. It is one of four README-documented asset constructors and the only one that opens its own httpx client, so it silently ignores the client's timeout= and client_info.
• Preview.to_pil() — zero tests. README shows show(pv.to_pil()) and the repo ships a pil extra for it; Pillow is already in [dev].
• comfy_low.errors.error_from_envelope — zero direct tests, and the _CODE_BY_STATUS status-derived fallback is entirely dark. tests/conftest.py::_err always writes a well-formed JSON envelope, so the "bare 401/404/429 with no JSON body" branch that the docstring exists for never runs. That is exactly the branch that fires when an nginx/LB/proxy answers instead of the API — the self-hosted comfy-api-proxy deployment the README targets.
• to_sdk_error — 3 of 13 code mappings tested. insufficient_credits, forbidden, blob_not_found appear nowhere in tests/. The table is currently complete, so this is a guard gap rather than a live bug — worth closing the same way test_spec_coverage.py already closes it for operationIds: assert comfy_low.errors._BY_CODE's code set is fully mapped by comfy_sdk.exceptions.
• The four escape hatches the README's architecture section promises — raw_request, open, all-headers, per-request timeout/abort — have no contract test. Only SSE timeout=None is exercised.
• _core.backoff_schedule — zero direct tests; wait() never polls long enough to reach cap.
• head_asset_by_hash's non-200/404 branch — zero tests; the stub only answers 200/404.

What was checked and is clean

Recording these so nobody re-runs them:

• Codegen drift is guarded and enforced. scripts/check_drift.py regenerates into a tempdir and compares byte-for-byte, so both a spec edit without a regen and a hand-edit of _generated.py are caught. The comfy_low codegen drift job runs it on every PR and push, and it is a required status check on main (verified at repos/.../branches/main/protection, not inferred from the workflow file). The generator is pinned and both ruff and mypy exclude the generated file, so a formatter cannot perturb the byte comparison. Worth noting for whoever checks next: the ruleset list contains only "CLA Check" — the real gate is classic branch protection, so checking rulesets alone yields a false "unguarded" reading.
• Auth is unusually well covered — test_auth_headers.py asserts the header actually sent rather than the 401/200 outcome, and test_transport_security.py / test_follow_up_links.py / test_content_redirect_security.py cover cross-origin bearer-token non-leak.
• Retry: queue_full → Retry-After → retry → success, plus budget exhaustion, sync and async.
• Pagination: N/A, not a gap — the vendored contract has 11 operations and no list endpoint.
• SSE: decoder unit tests cover comment/keepalive, stray blank line, malformed JSON, non-object JSON, multi-line data:; integration-level covers reconnect-without-replay, the zombie stalled connection, and the 501 no-SSE surface.
• No mock-only assertions. The suite drives a real stdlib HTTP stub; the only two monkeypatch.setattr calls target module constants, not the code under test.
• Assertion density: all 22 test files have assertions; no file reports as covered while asserting nothing. tests/integration/ is correctly skipif-gated.

[christian-byrne]

Third defect behind the same untested contract: submit() gives up on a retryable 429

Found while auditing comfy-typescript-sdk against this issue's findings — the same gap is in both
SDKs, so I'm adding it here rather than opening a second issue.

submit() retries by error code, but the contract disambiguates 429 by status + Retry-After.
The SDK's own generated model says so:

deployment_not_ready (429 + Retry-After — the deployment can still reach ready; retry) […]
A 429 is disambiguated by error.code alone; clients should treat any 429 + Retry-After as
"back off and retry".

— src/comfy_low/models/_generated.py:176-182

The retry loop gates on the translated type:

# src/comfy_sdk/client.py:170
if isinstance(err, QueueFull) and time.monotonic() < deadline:
    time.sleep(err.retry_after or _DEFAULT_RETRY_AFTER)
    continue
raise err from exc

and QueueFull is produced for exactly one code:

# src/comfy_sdk/exceptions.py:124
if exc.code == "queue_full":

Why this is easy to dismiss, and isn't

_CODE_BY_STATUS maps 429: "queue_full" (errors.py:140), so a first read suggests every 429
retries. It doesn't — that table is a fallback, reached only when the envelope carries no code:

# src/comfy_low/errors.py:113-118
code = (err or {}).get("code") if isinstance(err, dict) else None
...
if code is None:
    code = _CODE_BY_STATUS.get(http_status, "error")

So the behaviour splits on whether the server bothered to name the code:

429 response	resulting code	submit()
bare, no JSON envelope	falls back to queue_full	retries ✅
{"error": {"code": "deployment_not_ready"}}	deployment_not_ready → ComfyError	raises on attempt 1 ❌

A deployment that is merely warming up — the one case the contract explicitly calls out as
retryable — is the case that fails, and it fails on a serverless cold start, which is exactly when a
caller least expects a hard error. _QUEUE_RETRY_BUDGET is never consulted.

Fix

retry_after is already parsed off the header (transport.py:76) and carried on the ApiError, so
the information needed is present at the branch — it's the predicate that's wrong. Gate on the
status and the header rather than the code:

if err.http_status == 429 and err.retry_after is not None and time.monotonic() < deadline:

Both paths need it — sync client.py:171 and async client.py:254.

That also makes the behaviour forward-compatible: any future 429 code the server adds becomes
retryable without an SDK release, which is what "disambiguated by error.code alone" is asking for.

Test

Fits the contract-test table this issue already proposes. The case that matters is a 429 whose
envelope names a code other than queue_full — deployment_not_ready is the one in the spec.
A test that only exercises a bare 429 passes today and proves nothing, which is presumably how this
survived.

Same defect exists in comfy-typescript-sdk (filed as Comfy-Org/comfy-typescript-sdk#62) — worth
fixing in one pass so the two SDKs don't drift further.