fix: type registry API failures and surface machine-readable error codes (BE-3271)#528
fix: type registry API failures and surface machine-readable error codes (BE-3271)#528mattmillerai wants to merge 2 commits into
Conversation
…des (BE-3271)
Replace the 5 bare `raise Exception(...)` in comfy_cli/registry/api.py with a
typed RegistryAPIError carrying the HTTP status/body, and map it at the
publish/install command boundary to renderer.error(code=..., details={status,
body}) so node publish/install failures surface a machine-readable code instead
of a raw traceback or generic message. Messages are kept equivalent.
dosubot
Bot
added
the
size:M
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 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 (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![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 4 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 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)
…rors (BE-3271)
Addresses the cursor-review panel findings on the new
renderer.error(details={status, body}) surface:
- Add sanitize_error_body(): redacts known secrets, escapes CR/LF, and
truncates to MAX_ERROR_BODY_CHARS before the untrusted registry
response body reaches a log record or an error envelope.
- publish_node_version() passes the PAT as a secret, so a registry error
that echoes the request payload back can no longer leak the token into
terminal output or JSON logs.
- Apply the same bounding/escaping to list_all_nodes() and install_node(),
which log the full str(e) — an attacker-controlled registry could
otherwise forge log lines or flood CI logs.
- Fix the adjacent set-literal quirk in the publish fallback:
display_error_message({str(e)}) rendered as {'...'} instead of a plain
string.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
size:L
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
🤖 Reviews-loop pass: no code changes needed — both red checks are external to this PR. Review threads: all 4 Cursor findings were already addressed in 55d620c (PAT redaction + body bounding at the raise sites, and the
This PR on its own is green: 101/101 tests in |
|
🤖 Reviews-loop re-check: no code changes needed — this PR is ready; both red checks are still external. Independently re-verified rather than trusting the earlier pass:
Also refreshed the PR description, which had gone stale after 55d620c: it still claimed error messages were "byte-for-byte equivalent" (no longer true — bodies are now redacted/escaped/truncated, which reviewers should know) and listed the set-literal quirk as "left alone" when it was actually fixed. Goes green on a rebase once the tomlkit fix lands on main. Not duplicating that work here to avoid conflicting with those PRs. |
1 participant
ELI-5
When you
comfy node publishorcomfy node registry-installand the registrysays "no" (bad token, node not found, server error), the CLI used to blow up
with a raw
Exception— a generic message or a Python traceback with nomachine-readable error code. An agent driving the CLI couldn't tell what
went wrong programmatically.
This PR gives those failures a name and a code: the registry client now raises a
typed
RegistryAPIErrorthat carries the HTTPstatusandbody, and thepublish/install commands turn that into the standard
renderer.error(code=..., details={status, body})envelope — the samestructured-error contract the rest of the CLI already uses.
What changed
comfy_cli/registry/api.py— newRegistryAPIError(Exception)carryingoptional
status/body. The 5 bareraise Exception(...)(publisher-id/ project-name validation, and the publish / list / install HTTP-failure
branches) now raise
RegistryAPIError. Any existing broadexcept Exceptionkeeps working —
RegistryAPIErrorsubclassesException.sanitize_error_body()redacts the publish PAT (the publish request bodycarries it, and the registry can echo it back), escapes CR/LF so a body can't
forge extra log lines, and bounds the body to 2000 chars. This means the
message text is no longer byte-for-byte identical to the old
raise Exception(...)for HTTP-failure branches — bodies may be redacted,escaped, or truncated. The two client-side validation messages are unchanged.
comfy_cli/command/custom_nodes/command.py— thepublishandregistry-installhandlers gain a typedexcept RegistryAPIError(before theexisting broad
except) that emitsrenderer.error(code="node_publish_failed" | "node_install_failed", details={status, body})and exits 1.comfy_cli/error_codes.py— registers the two new codes with navigationhints (the registry test enforces every raised code is registered and every
registered code carries a hint).
test_api.pynow asserts the typed exception + thatstatus/bodyare carried (plus a client-side-validation case with
status=None); newcommand-boundary tests assert publish/install surface the right code + details
and exit 1.
Scope / judgment calls (this is the narrowed BE-3271, a DOWNGRADE from a
broader idiom sweep)
The
list_all_nodesAPI call is converted to the typed exception (1 of the 5),but its hidden
registry-listcommand boundary (display_all_nodes) is leftuntouched — it still catches
RegistryAPIErrorasException, logs, showsthe friendly message, and returns exit 0, exactly as before. Zero behavioral
change there.
typer.echo/ui.display_error_messagemigrations acrosscustom_nodes/models were intentionally NOT done (the ticket explicitly defers
those until those commands join the JSON-envelope contract).
registry-installnow exits 1 instead of silently returning 0. This isa correctness fix — the sibling download-path test already documents the intent
("Must exit non-zero so automation / CI can detect the failure") — and it is
narrowly scoped: only
RegistryAPIErrortakes the new path; connection errorsand everything else still hit the unchanged broad
except(exit 0).ui.display_error_message({str(e)})set-literal quirk in the publish broad-
exceptfallback — it wrapped themessage in a one-element
set. Now passesstr(e). One-character fix flaggedin review; taken rather than deferred.
registry-installexit non-zero for non-RegistryAPIErrorfailures (connection error, timeout, JSON decode) is tracked as
BE-3294 / PR fix(registry): exit non-zero on non-RegistryAPIError registry-install failures #538.
Verification
ruff format --check: clean.ruff check: no errors in any file this PR touches.tests/comfy_cli/registry/test_api.py+tests/comfy_cli/command/nodes/).CI red is external to this PR (verified, not assumed)
build(9 failures) —ValueError: Comment cannot contain line breaksintest_config_parser.py/test_node_init.py. This PR touches neither file.Caused by tomlkit 0.15.1; bisected locally:
tomlkit==0.15.1→ the same 9failures,
tomlkit==0.13.3(whatuv.lockpins) → 83 passed. Thebuildjobresolves deps fresh instead of from the lock. Same red on unrelated PRs
build(deps): re-sync uv.lock with the pyproject bench extra (BE-3291) #534/fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289) #535; fixes in flight as fix(registry): emit pyproject hint blocks as single-line tomlkit comments #533/fix(registry): emit pyproject hints as standalone comments for tomlkit 0.15 #536/ci: install test dependencies from uv.lock instead of resolving fresh #537.
test(Windows) —ImportError: cannot import name __version__ from pydantic_coreduring dep install, via mixpanel. Covered by ci: set UV_LINK_MODE=copy in the Windows workflow #529/fix: build telemetry providers lazily so comfy install can upgrade pydantic_core (BE-3289) #535.Both go green on a rebase once those land.