diff --git a/comfy_cli/command/custom_nodes/command.py b/comfy_cli/command/custom_nodes/command.py index bbeb4ac5..7de9fbc2 100644 --- a/comfy_cli/command/custom_nodes/command.py +++ b/comfy_cli/command/custom_nodes/command.py @@ -23,9 +23,11 @@ upload_file_to_signed_url, zip_files, ) +from comfy_cli.output import get_renderer from comfy_cli.output import rprint as print # context-aware: stderr in JSON mode from comfy_cli.registry import ( RegistryAPI, + RegistryAPIError, extract_node_configuration, initialize_project_config, ) @@ -1119,8 +1121,15 @@ def publish( # Upload the zip file to the signed URL typer.echo("Uploading zip file...") upload_file_to_signed_url(signed_url, zip_filename) + except RegistryAPIError as e: + get_renderer().error( + code="node_publish_failed", + message=str(e), + details={"status": e.status, "body": e.body}, + ) + raise typer.Exit(code=1) from e except Exception as e: - ui.display_error_message({str(e)}) + ui.display_error_message(str(e)) raise typer.Exit(code=1) @@ -1210,15 +1219,33 @@ def registry_install( try: # Call the API to install the node node_version = registry_api.install_node(node_id, version) - if not node_version.download_url: - logging.error("Download URL not provided from the registry.") - ui.display_error_message(f"Failed to download the custom node {node_id}.") - return - + except RegistryAPIError as e: + logging.error(f"Encountered an error while installing the node. error: {str(e)}") + get_renderer().error( + code="node_install_failed", + message=f"Failed to download the custom node {node_id}.", + details={"node_id": node_id, "status": e.status, "body": e.body}, + ) + raise typer.Exit(code=1) from e except Exception as e: logging.error(f"Encountered an error while installing the node. error: {str(e)}") - ui.display_error_message(f"Failed to download the custom node {node_id}.") - return + get_renderer().error( + code="node_install_failed", + message=f"Failed to download the custom node {node_id}.", + details={"node_id": node_id}, + ) + raise typer.Exit(code=1) from e + + # Checked outside the try: typer.Exit subclasses Exception, so raising it + # above would be swallowed by the broad handler and emit a second envelope. + if not node_version.download_url: + logging.error("Download URL not provided from the registry.") + get_renderer().error( + code="node_install_failed", + message=f"Failed to download the custom node {node_id}.", + details={"node_id": node_id}, + ) + raise typer.Exit(code=1) # Download the node archive custom_nodes_path = pathlib.Path(workspace_manager.workspace_path) / "custom_nodes" diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index f0729744..ae35fe76 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -577,6 +577,21 @@ class ErrorCode: "`generate --emit-workflow` could not build the partner-node workflow.", "check the model name and that all required inputs are provided", ), + # --- custom node registry ------------------------------------------------ + ErrorCode( + "node_publish_failed", + "Publishing a custom-node version to the registry failed: either a " + "client-side validation gap (missing publisher id / project name in " + "pyproject.toml) or a non-2xx from the registry. `details.status` and " + "`details.body` carry the response when it was an HTTP failure.", + "check `details.body`; ensure `[tool.comfy] publisher_id` and `[project] name` are set, and the token is valid", + ), + ErrorCode( + "node_install_failed", + "Fetching a custom node from the registry for install failed with a non-2xx. " + "`details.status` and `details.body` carry the response.", + "check the node id and version exist in the registry (`comfy node registry-list`); check `details.body`", + ), ErrorCode( "spend_consent_required", "A credit-spending command hit its spend gate with no consent, so it failed closed — " diff --git a/comfy_cli/registry/__init__.py b/comfy_cli/registry/__init__.py index 912438a9..398818f8 100644 --- a/comfy_cli/registry/__init__.py +++ b/comfy_cli/registry/__init__.py @@ -1,9 +1,10 @@ -from .api import RegistryAPI +from .api import RegistryAPI, RegistryAPIError from .config_parser import extract_node_configuration, initialize_project_config from .types import Node, NodeVersion, PublishNodeVersionResponse, PyProjectConfig __all__ = [ "RegistryAPI", + "RegistryAPIError", "extract_node_configuration", "PyProjectConfig", "PublishNodeVersionResponse", diff --git a/comfy_cli/registry/api.py b/comfy_cli/registry/api.py index eb1c65a7..eefca655 100644 --- a/comfy_cli/registry/api.py +++ b/comfy_cli/registry/api.py @@ -13,6 +13,46 @@ PyProjectConfig, ) +MAX_ERROR_BODY_CHARS = 2000 + + +def sanitize_error_body(text: str, *, secrets: tuple[str | None, ...] = ()) -> str: + """Make an untrusted registry response body safe to log and render. + + The registry is upstream of us: its response body can echo back what we + sent (including the publish PAT), embed newlines that forge extra log + lines, or be arbitrarily large. Redact known secrets, flatten CR/LF to + escapes, and bound the length before the body reaches a log record or a + ``renderer.error`` envelope. + """ + for secret in secrets: + if secret: + text = text.replace(secret, "***REDACTED***") + + text = text.replace("\r", "\\r").replace("\n", "\\n") + + if len(text) > MAX_ERROR_BODY_CHARS: + text = f"{text[:MAX_ERROR_BODY_CHARS]}... [truncated, {len(text)} chars total]" + + return text + + +class RegistryAPIError(Exception): + """Raised when a Registry API call fails. + + Carries the HTTP ``status`` and response ``body`` when the failure came + from a non-2xx response, so the command boundary can surface a + machine-readable ``renderer.error(code=..., details={status, body})`` + instead of a bare traceback. Client-side validation failures (missing + publisher id / project name) carry no status/body. + """ + + def __init__(self, message: str, *, status: int | None = None, body: str | None = None): + super().__init__(message) + self.message = message + self.status = status + self.body = body + class RegistryAPI: def __init__(self): @@ -44,10 +84,10 @@ def publish_node_version( """ # Local import to prevent circular dependency if not node_config.tool_comfy.publisher_id: - raise Exception("Publisher ID is required in pyproject.toml to publish a node version") + raise RegistryAPIError("Publisher ID is required in pyproject.toml to publish a node version") if not node_config.project.name: - raise Exception("Project name is required in pyproject.toml to publish a node version") + raise RegistryAPIError("Project name is required in pyproject.toml to publish a node version") license_json = serialize_license(node_config.project.license) request_body = { "personal_access_token": token, @@ -88,7 +128,14 @@ def publish_node_version( signedUrl=data["signedUrl"], ) else: - raise Exception(f"Failed to publish node version: {response.status_code} {response.text}") + # The publish request body carries the PAT, so a registry error that + # echoes the payload back would otherwise leak it into logs. + safe_body = sanitize_error_body(response.text, secrets=(token,)) + raise RegistryAPIError( + f"Failed to publish node version: {response.status_code} {safe_body}", + status=response.status_code, + body=safe_body, + ) def list_all_nodes(self): """ @@ -103,7 +150,12 @@ def list_all_nodes(self): raw_nodes = response.json()["nodes"] return [map_node_to_node_class(node) for node in raw_nodes] else: - raise Exception(f"Failed to retrieve nodes: {response.status_code} - {response.text}") + safe_body = sanitize_error_body(response.text) + raise RegistryAPIError( + f"Failed to retrieve nodes: {response.status_code} - {safe_body}", + status=response.status_code, + body=safe_body, + ) def install_node(self, node_id, version=None): """ @@ -129,7 +181,12 @@ def install_node(self, node_id, version=None): logging.debug(f"RegistryAPI install_node response: {response.json()}") return map_node_version(response.json()) else: - raise Exception(f"Failed to install node: {response.status_code} - {response.text}") + safe_body = sanitize_error_body(response.text) + raise RegistryAPIError( + f"Failed to install node: {response.status_code} - {safe_body}", + status=response.status_code, + body=safe_body, + ) def get_node(self, node_id): """ diff --git a/tests/comfy_cli/command/nodes/test_node_install.py b/tests/comfy_cli/command/nodes/test_node_install.py index 93fdbe54..e2e7cf1c 100644 --- a/tests/comfy_cli/command/nodes/test_node_install.py +++ b/tests/comfy_cli/command/nodes/test_node_install.py @@ -2,10 +2,12 @@ import subprocess from unittest.mock import MagicMock, patch +import requests from typer.testing import CliRunner from comfy_cli.command.custom_nodes.command import app from comfy_cli.file_utils import DownloadException +from comfy_cli.registry import RegistryAPIError runner = CliRunner() @@ -383,3 +385,79 @@ def test_no_traceback_in_output(self, tmp_path): assert "Traceback" not in result.output assert "DownloadException" not in result.output + + +class TestRegistryInstallApiError: + """A RegistryAPIError from install_node must surface a machine-readable + renderer.error(code="node_install_failed", details={status, body}) and + exit non-zero — not a bare traceback and not a silent exit 0.""" + + def test_api_error_surfaced_with_code_and_exit_1(self, tmp_path): + with ( + patch("comfy_cli.command.custom_nodes.command.registry_api") as mock_api, + patch("comfy_cli.command.custom_nodes.command.workspace_manager") as mock_ws, + patch("comfy_cli.command.custom_nodes.command.get_renderer") as mock_get_renderer, + ): + mock_ws.workspace_path = str(tmp_path) + mock_api.install_node.side_effect = RegistryAPIError( + "Failed to install node: 404 - Not Found", status=404, body="Not Found" + ) + + result = runner.invoke(app, ["registry-install", "test-node"]) + + assert result.exit_code == 1 + assert "Traceback" not in result.output + mock_get_renderer.return_value.error.assert_called_once() + _, kwargs = mock_get_renderer.return_value.error.call_args + assert kwargs["code"] == "node_install_failed" + assert kwargs["details"] == {"node_id": "test-node", "status": 404, "body": "Not Found"} + + +class TestRegistryInstallNonApiFailure: + """Failures that aren't RegistryAPIError — a connection error, a DNS failure, + a timeout, a JSON decode error, or a registry response carrying no download + URL — must also emit the node_install_failed envelope and exit non-zero. + Exiting 0 here reports a network outage to automation / CI as success.""" + + def _invoke(self, tmp_path, *, install_node_side_effect=None, install_node_return=None): + with ( + patch("comfy_cli.command.custom_nodes.command.registry_api") as mock_api, + patch("comfy_cli.command.custom_nodes.command.workspace_manager") as mock_ws, + patch("comfy_cli.command.custom_nodes.command.get_renderer") as mock_get_renderer, + patch("comfy_cli.command.custom_nodes.command.download_file") as mock_dl, + ): + mock_ws.workspace_path = str(tmp_path) + if install_node_side_effect is not None: + mock_api.install_node.side_effect = install_node_side_effect + else: + mock_api.install_node.return_value = install_node_return + + result = runner.invoke(app, ["registry-install", "test-node"]) + return result, mock_get_renderer, mock_dl + + def test_connection_error_exits_1_with_envelope(self, tmp_path): + result, mock_get_renderer, mock_dl = self._invoke( + tmp_path, install_node_side_effect=requests.ConnectionError("Name or service not known") + ) + + # Must exit non-zero so automation / CI can detect the failure. + assert result.exit_code == 1 + assert "Traceback" not in result.output + mock_dl.assert_not_called() + mock_get_renderer.return_value.error.assert_called_once() + _, kwargs = mock_get_renderer.return_value.error.call_args + assert kwargs["code"] == "node_install_failed" + assert kwargs["details"] == {"node_id": "test-node"} + + def test_missing_download_url_exits_1_with_envelope(self, tmp_path): + result, mock_get_renderer, mock_dl = self._invoke( + tmp_path, install_node_return=MagicMock(download_url="", version="1.0.0") + ) + + assert result.exit_code == 1 + assert "Traceback" not in result.output + mock_dl.assert_not_called() + mock_get_renderer.return_value.error.assert_called_once() + _, kwargs = mock_get_renderer.return_value.error.call_args + assert kwargs["code"] == "node_install_failed" + assert kwargs["details"] == {"node_id": "test-node"} diff --git a/tests/comfy_cli/command/nodes/test_publish.py b/tests/comfy_cli/command/nodes/test_publish.py index 42905a0e..d5df7559 100644 --- a/tests/comfy_cli/command/nodes/test_publish.py +++ b/tests/comfy_cli/command/nodes/test_publish.py @@ -5,6 +5,7 @@ from typer.testing import CliRunner from comfy_cli.command.custom_nodes.command import app +from comfy_cli.registry import RegistryAPIError from comfy_cli.registry.types import ComfyConfig, ProjectConfig, PyProjectConfig runner = CliRunner() @@ -148,6 +149,38 @@ def test_publish_exits_on_upload_failure(): assert mock_upload.called +def test_publish_surfaces_registry_api_error_with_code(): + # A RegistryAPIError from the publish call must be surfaced as a + # machine-readable renderer.error(code=..., details={status, body}) and + # exit 1 — not a bare traceback. + mock_result = MagicMock() + mock_result.returncode = 0 + mock_result.stdout = "" + + with ( + patch("subprocess.run", return_value=mock_result), + patch("comfy_cli.command.custom_nodes.command.extract_node_configuration") as mock_extract, + patch("comfy_cli.command.custom_nodes.command.registry_api.publish_node_version") as mock_publish, + patch("comfy_cli.command.custom_nodes.command.zip_files") as mock_zip, + patch("comfy_cli.command.custom_nodes.command.upload_file_to_signed_url") as mock_upload, + patch("comfy_cli.command.custom_nodes.command.get_renderer") as mock_get_renderer, + ): + mock_extract.return_value = create_mock_config() + mock_publish.side_effect = RegistryAPIError( + "Failed to publish node version: 400 Bad Request", status=400, body="Bad Request" + ) + + result = runner.invoke(app, ["publish", "--token", "test-token"]) + + assert result.exit_code == 1 + assert not mock_zip.called + assert not mock_upload.called + mock_get_renderer.return_value.error.assert_called_once() + _, kwargs = mock_get_renderer.return_value.error.call_args + assert kwargs["code"] == "node_publish_failed" + assert kwargs["details"] == {"status": 400, "body": "Bad Request"} + + def test_publish_fails_when_config_is_none(): # extract_node_configuration returns None when pyproject.toml is missing; # validate_node_for_publishing must exit 1 (not crash on the subsequent diff --git a/tests/comfy_cli/registry/test_api.py b/tests/comfy_cli/registry/test_api.py index b8095b66..824e77ab 100644 --- a/tests/comfy_cli/registry/test_api.py +++ b/tests/comfy_cli/registry/test_api.py @@ -5,7 +5,12 @@ from unittest.mock import MagicMock, patch from comfy_cli.registry import PyProjectConfig -from comfy_cli.registry.api import RegistryAPI +from comfy_cli.registry.api import ( + MAX_ERROR_BODY_CHARS, + RegistryAPI, + RegistryAPIError, + sanitize_error_body, +) from comfy_cli.registry.types import ComfyConfig, License, ProjectConfig, URLs @@ -69,9 +74,36 @@ def test_publish_node_version_failure(self, mock_post): mock_response.text = "Bad Request" mock_post.return_value = mock_response - with self.assertRaises(Exception) as context: + with self.assertRaises(RegistryAPIError) as context: self.registry_api.publish_node_version(self.node_config, self.token) self.assertIn("Failed to publish node version", str(context.exception)) + self.assertEqual(context.exception.status, 400) + self.assertEqual(context.exception.body, "Bad Request") + + def test_publish_node_version_requires_publisher_id(self): + # Client-side validation failure: typed error, no HTTP status/body. + self.node_config.tool_comfy.publisher_id = "" + with self.assertRaises(RegistryAPIError) as context: + self.registry_api.publish_node_version(self.node_config, self.token) + self.assertIn("Publisher ID is required", str(context.exception)) + self.assertIsNone(context.exception.status) + self.assertIsNone(context.exception.body) + + @patch("requests.post") + def test_publish_node_version_failure_redacts_token_echoed_by_registry(self, mock_post): + # The publish payload carries the PAT; a registry error that echoes the + # payload back must not leak it into the message or the error body. + mock_response = MagicMock() + mock_response.status_code = 400 + mock_response.text = f'{{"error":"bad request","sent":{{"personal_access_token":"{self.token}"}}}}' + mock_post.return_value = mock_response + + with self.assertRaises(RegistryAPIError) as context: + self.registry_api.publish_node_version(self.node_config, self.token) + + self.assertNotIn(self.token, str(context.exception)) + self.assertNotIn(self.token, context.exception.body) + self.assertIn("***REDACTED***", context.exception.body) def _mock_publish_response(self, changelog=""): mock_response = MagicMock() @@ -168,9 +200,11 @@ def test_list_all_nodes_failure(self, mock_get): mock_response.text = "Internal Server Error" mock_get.return_value = mock_response - with self.assertRaises(Exception) as context: + with self.assertRaises(RegistryAPIError) as context: self.registry_api.list_all_nodes() self.assertIn("Failed to retrieve nodes", str(context.exception)) + self.assertEqual(context.exception.status, 500) + self.assertEqual(context.exception.body, "Internal Server Error") @patch("requests.get") def test_install_node_success(self, mock_get): @@ -197,9 +231,11 @@ def test_install_node_failure(self, mock_get): mock_response.text = "Not Found" mock_get.return_value = mock_response - with self.assertRaises(Exception) as context: + with self.assertRaises(RegistryAPIError) as context: self.registry_api.install_node("node1") self.assertIn("Failed to install node", str(context.exception)) + self.assertEqual(context.exception.status, 404) + self.assertEqual(context.exception.body, "Not Found") @patch("requests.get") def test_get_node_success(self, mock_get): @@ -233,3 +269,25 @@ def test_get_node_failure(self, mock_get): with self.assertRaises(Exception) as context: self.registry_api.get_node("node1") self.assertIn("Failed to retrieve node", str(context.exception)) + + +class TestSanitizeErrorBody(unittest.TestCase): + def test_leaves_ordinary_body_untouched(self): + self.assertEqual(sanitize_error_body("Not Found"), "Not Found") + + def test_redacts_secrets(self): + body = '{"personal_access_token":"s3cr3t"}' + self.assertEqual(sanitize_error_body(body, secrets=("s3cr3t",)), '{"personal_access_token":"***REDACTED***"}') + + def test_ignores_empty_secrets(self): + # A falsy token must not turn every empty string into a redaction marker. + self.assertEqual(sanitize_error_body("plain", secrets=(None, "")), "plain") + + def test_escapes_newlines_to_prevent_log_forgery(self): + forged = "error\nINFO: everything is fine\r\n" + self.assertEqual(sanitize_error_body(forged), "error\\nINFO: everything is fine\\r\\n") + + def test_truncates_oversized_body(self): + result = sanitize_error_body("x" * (MAX_ERROR_BODY_CHARS + 500)) + self.assertTrue(result.startswith("x" * MAX_ERROR_BODY_CHARS)) + self.assertIn(f"truncated, {MAX_ERROR_BODY_CHARS + 500} chars total", result)