fix(registry): exit non-zero on non-RegistryAPIError registry-install failures#538
Conversation
mattmillerai
added
agent-coded
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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.
✅ No high-signal findings.
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:S
55d620c to
ab54fba
Compare
… failures Only RegistryAPIError produced a structured error envelope and exit 1. Every other install_node failure — connection error, DNS failure, timeout, JSON decode error — fell into the broad except, which logged, printed a friendly message and returned, exiting 0. A network outage during `comfy node registry-install` was therefore reported to automation/CI as success, and JSON-mode consumers saw no error output at all. The `download_url` missing branch was the same silent-success shape. Both paths now emit the node_install_failed envelope and exit 1. The download_url check moves below the try: typer.Exit subclasses Exception, so raising it inside would be swallowed by the broad handler and emit a second envelope. install_node either returns a NodeVersion or raises, and both except branches now raise, so the check is only reached on success.
7f9906e to
24f901e
Compare
|
STACKED — merging lands on Resolved a merge conflict: this branch had stale copies of the BE-3271 commits from before #528 was rebased onto a newer main. Rebased CI: no pytest/ruff/build workflows run here since they're gated to |
2 participants
ELI-5
If your wifi dropped in the middle of
comfy node registry-install, the command printed a friendly "Failed to download..." message and then told your shell everything was fine (exit code 0). A CI job or an agent watching the exit code would happily carry on as if the node had installed. This makes the command admit it failed.What this changes
registry_installhad two silent-success paths:except Exception— every non-RegistryAPIErrorfailure (connection error, DNS failure, timeout, JSON decode error) logged, printed a message, andreturned → exit 0, with no error output at all in JSON mode.if not node_version.download_url:branch — same shape, same silent exit 0.Both now emit the
node_install_failedstructured envelope andraise typer.Exit(code=1), mirroring the existingRegistryAPIErrorbranch.node_install_failedwas already registered, so no new error code.This is a deliberate, behavioral change: connection errors and other non-registry failures now exit 1 where they previously exited 0. That is the point of the change.
Stacked PR
Based on
matt/be-3271-registry-api-typed-errors(#528), notmain— theRegistryAPIErrorbranch,get_renderer()in this module, and thenode_install_failederror code all come from that PR and are not onmainyet. GitHub will retarget this tomainonce #528 merges. Review only the net diff (one commit).The one non-obvious line
The
download_urlcheck moves below thetryblock.typer.ExitsubclassesException, so raising it inside thetrywould be caught by the broadexcept Exception— logging a nonsenseerror: 1and emitting a second envelope. Moving the check out is safe becauseinstall_nodeeither returns aNodeVersionor raises, and bothexceptbranches now raise, so the check is only reached on the success path with a non-Nonenode_version. (Response-mapping errors like a missingid/versionstill raise insideinstall_node, i.e. still inside thetry.)Testing
New
TestRegistryInstallNonApiFailurecovers the two cases the ticket asked for — arequests.ConnectionErrorfrominstall_node, and a response with nodownload_url— each asserting exit 1 plus thenode_install_failedenvelope withdetails={"node_id": ...}.Beyond the mocked tests, I drove the real CLI with the real renderer in JSON mode (the unit tests mock
get_renderer, so they can't prove the envelope is well-formed). Both paths produce exit 1 and exactly one valid envelope, confirming there's no double-emit:{"schema": "envelope/1", "ok": false, "command": "node registry-install", "error": {"code": "node_install_failed", "message": "Failed to download the custom node test-node.", "details": {"node_id": "test-node"}}}Full suite green: 2584 passed, 37 skipped.
ruff format --checkclean;ruff checkclean on both touched files.Judgment calls
assert result.exit_code == 0cases intest_node_install.py, per the ticket's guardrail. All of them exercise other commands (install,uv-sync,show, …) against a mockedexecute_cm_clisuccess — none were asserting the silent-success bug fixed here, so none needed changing.ruff check .reports 14 pre-existingUP038errors in files this PR doesn't touch. They reproduce with my changes stashed (a local ruff newer than the pinned one), so I left them alone rather than take a drive-by refactor into this diff.node_install_failedends with "checkdetails.body", but the two new non-API paths have nobodyindetails. The hint comes from fix: type registry API failures and surface machine-readable error codes (BE-3271) #528's registry and is shared with the API path, so retuning it is out of scope here — flagging it as a cosmetic wart rather than silently widening this PR.raise/exit-1 paths, but nothing that previously worked now fails: it only changes how already-failed installs report themselves. There's no working alternate path a user should be redirected to instead.