diff --git a/comfy_cli/command/custom_nodes/command.py b/comfy_cli/command/custom_nodes/command.py index 7f4b1c80..d5fe48a5 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) @@ -1215,6 +1224,14 @@ def registry_install( 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}.") diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 4cba21d3..cfe20ff1 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`", + ), # --- feedback ------------------------------------------------------------ ErrorCode( "feedback_message_required", 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 928b3606..1c57d5fc 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): """ @@ -127,7 +179,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 map_node_version(api_node_version): diff --git a/tests/comfy_cli/command/nodes/test_node_install.py b/tests/comfy_cli/command/nodes/test_node_install.py index 93fdbe54..2acfb295 100644 --- a/tests/comfy_cli/command/nodes/test_node_install.py +++ b/tests/comfy_cli/command/nodes/test_node_install.py @@ -6,6 +6,7 @@ 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 +384,29 @@ 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"} 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 9521b65e..03fb0bea 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,6 +231,30 @@ 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") + + +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)