fix: type registry API failures and surface machine-readable error codes (BE-3271) - #528
Conversation
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: 25 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)
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. |
…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.
…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>
55d620c to
ab54fba
Compare
…-api-typed-errors # Conflicts: # comfy_cli/error_codes.py
skishore23
left a comment
skishore23
left a comment
There was a problem hiding this comment.
Approving. The typed-error work is right and the hardening is better than the ticket asked for. One conflict to resolve before merge, plus a design follow-up worth filing.
Conflict: main grew its own typed registry exception
comfy_cli/registry/__init__.py conflicts because main added NodeFetchError after this branch opened:
HEAD: from .api import RegistryAPI, RegistryAPIError
origin/main: from .api import NodeFetchError, RegistryAPI
They're complementary, not duplicative — I checked which sites each covers on current main:
| site | main |
this PR |
|---|---|---|
get_node (:169) |
NodeFetchError |
untouched |
| publisher-id / project-name validation (:61,:64) | bare Exception |
RegistryAPIError |
| publish (:105) | bare Exception |
RegistryAPIError |
| list (:120) | bare Exception |
RegistryAPIError |
| install (:146) | bare Exception |
RegistryAPIError |
So together all six are typed. Resolution is a union export — keep both names in the import and both in __all__. I did that locally: 122 passed. (The one failure, test_restore_snapshot_fast_deps.py::test_default_omitted_does_not_force_uv_compile, reproduces on plain origin/main — pre-existing, unrelated.)
Follow-up worth filing: the two exceptions don't compose
After the union resolution the registry exposes two typed errors with different attribute names and no common base:
class NodeFetchError(Exception): def __init__(self, message, status_code=None)
class RegistryAPIError(Exception): def __init__(self, message, *, status=None, body=None)Consequences: except RegistryAPIError silently misses get_node failures, and a caller reading .status on a NodeFetchError gets AttributeError. Both classes exist for the same reason — "the registry said no, here's the HTTP status."
Cheapest fix is making NodeFetchError a subclass of RegistryAPIError that maps status_code → status (keeping status_code as a property for existing callers). Then except RegistryAPIError is the one true handler. Not a blocker for this PR — main created the divergence — but it should be closed before a third variant appears.
Verified
The sanitize_error_body hardening actually works — exercised it directly:
redacts PAT : error token=***REDACTED*** denied
escapes CRLF: 'line1\\r\\nFAKE LOG LINE'
bounds len : True (MAX_ERROR_BODY_CHARS = 2000)
The ordering is correct and non-obvious: redact → escape → truncate. Truncating first could split a secret and leave a partial token visible; escaping first wouldn't change the match but would shift offsets. Worth a comment saying the order is load-bearing, since a future refactor could reorder it harmlessly-looking.
Also good that the PAT is passed as a secret only at the publish site (api.py:133), where the token is actually in scope — the list/install sites (:153, :184) correctly pass no secrets rather than threading a token they don't have.
Everything else: 105 passed as-authored; registry-install exiting 1 on a registry failure is a correctness fix and the narrow scoping (only RegistryAPIError takes the new path) is the right amount of caution; catching RegistryAPIError before the existing broad except preserves all other behavior.
The "CI red is external" section is now stale in your favor — all checks on this PR are currently green, so the tomlkit/pydantic_core triage can come out of the squash message.
…-api-typed-errors # Conflicts: # comfy_cli/registry/__init__.py
…-api-typed-errors
Merging origin/main pulled in an `output.renderer` import for get_renderer alongside this branch's existing `output` import of the same re-exported name, tripping ruff's F811 redefinition check.
|
🤖 Reviews-loop pass: conflict resolved, CI green.
|
…-api-typed-errors # Conflicts: # tests/comfy_cli/command/nodes/test_node_install.py
…E-3271) main independently added an error_codes.py entry for `node_install_failed` (the `node install --exit-on-fail` / cm-cli path) while this branch was also using that code for `node registry-install`'s registry-fetch failures. The merge combined both entries under the same code, tripping test_no_duplicate_codes and colliding two semantically distinct failure modes. Rename the registry-install side to `registry_install_failed` so both stay distinguishable machine-readable codes.
|
🤖 Reviews-loop pass: merge conflict resolved, CI green.
All PR checks green ( |
4 participants
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.