mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-09-21 02:16:27 +00:00
fix(cli): classify deploy create failures by status and by stage
Review fixes on the failure span. Check the HTTP class before the body so a gateway's HTML page counts as api_4xx / api_5xx with its code. Treat a 2xx whose body is not a JSON object carrying uuid and status as invalid_response and exit cleanly, instead of emitting a success span and crashing in the display step. Recognise archive failures by a dedicated ArchiveError (a ValueError) raised from create_project_zip, so the git helpers' own ValueErrors no longer read as zip_error; a failed ZIP write is wrapped and its partial file removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
@@ -36,6 +36,15 @@ _EXCLUDED_SUFFIXES = {
|
||||
}
|
||||
|
||||
|
||||
class ArchiveError(ValueError):
|
||||
"""The project ZIP could not be built.
|
||||
|
||||
A ``ValueError`` so existing callers keep working; a distinct type so the
|
||||
deploy command can tell an archive failure from the git helpers' own
|
||||
``ValueError``s when it classifies why a create failed.
|
||||
"""
|
||||
|
||||
|
||||
def create_project_zip(
|
||||
project_name: str,
|
||||
*,
|
||||
@@ -46,7 +55,7 @@ def create_project_zip(
|
||||
root = (project_dir or Path.cwd()).resolve()
|
||||
files = _project_files(root, repository)
|
||||
if not files:
|
||||
raise ValueError("No deployable project files were found.")
|
||||
raise ArchiveError("No deployable project files were found.")
|
||||
|
||||
staged_root = _stage_project(root, files)
|
||||
archive_handle = tempfile.NamedTemporaryFile(
|
||||
@@ -62,6 +71,9 @@ def create_project_zip(
|
||||
for relative_path in _walk_files(staged_root):
|
||||
absolute_path = staged_root / relative_path
|
||||
zip_file.write(absolute_path, relative_path.as_posix())
|
||||
except (OSError, zipfile.BadZipFile) as exc:
|
||||
archive_path.unlink(missing_ok=True)
|
||||
raise ArchiveError(f"Could not build the project ZIP: {exc}") from exc
|
||||
finally:
|
||||
shutil.rmtree(staged_root, ignore_errors=True)
|
||||
|
||||
|
||||
@@ -4,7 +4,6 @@ import subprocess
|
||||
from typing import Any
|
||||
from urllib.parse import quote
|
||||
import webbrowser
|
||||
import zipfile
|
||||
|
||||
from crewai_core.plus_api import CreateCrewPayload
|
||||
from crewai_core.telemetry import DeployFailureReason, DeploySource
|
||||
@@ -14,7 +13,7 @@ from rich.console import Console
|
||||
from crewai_cli import git
|
||||
from crewai_cli.command import BaseCommand, PlusAPIMixin
|
||||
from crewai_cli.constants import DEFAULT_CREWAI_ENTERPRISE_URL
|
||||
from crewai_cli.deploy.archive import create_project_zip
|
||||
from crewai_cli.deploy.archive import ArchiveError, create_project_zip
|
||||
from crewai_cli.deploy.validate import DeployValidator, Severity, render_report
|
||||
from crewai_cli.utils import fetch_and_json_env_file, get_project_name
|
||||
|
||||
@@ -132,31 +131,40 @@ def _deployment_page_url(base_url: str, json_response: dict[str, Any]) -> str |
|
||||
def _creation_failure_reason(exc: BaseException) -> DeployFailureReason:
|
||||
"""Classify an exception raised while requesting a deployment, for telemetry.
|
||||
|
||||
Only the archive step raises ``ValueError`` / ``OSError`` inside that request
|
||||
(the project name is validated at construction), so those read as zip errors.
|
||||
Archive failures are recognised by type (``ArchiveError``), not by base
|
||||
class: the git helpers raise plain ``ValueError`` too, and those are not
|
||||
ZIP problems.
|
||||
"""
|
||||
if isinstance(exc, (KeyboardInterrupt, EOFError)):
|
||||
return "user_declined"
|
||||
if isinstance(exc, httpx.HTTPError):
|
||||
return "network_error"
|
||||
if isinstance(exc, (ValueError, OSError, zipfile.BadZipFile)):
|
||||
if isinstance(exc, ArchiveError):
|
||||
return "zip_error"
|
||||
return "unexpected"
|
||||
|
||||
|
||||
def _response_failure_reason(response: httpx.Response) -> DeployFailureReason | None:
|
||||
"""Classify a create response that ``_validate_response`` will reject.
|
||||
"""Classify a create response that cannot become a created deployment.
|
||||
|
||||
Mirrors its checks in the same order; ``None`` means the response will pass.
|
||||
Status first, so a gateway's HTML error page counts as the API class it is;
|
||||
then the body, which must be a JSON object carrying ``uuid`` and ``status``
|
||||
for the success path to use. ``None`` means the response is a creation.
|
||||
"""
|
||||
try:
|
||||
response.json()
|
||||
except (json.JSONDecodeError, ValueError):
|
||||
return "invalid_response"
|
||||
if response.status_code >= 500:
|
||||
return "api_5xx"
|
||||
if not response.is_success:
|
||||
return "api_4xx"
|
||||
try:
|
||||
payload = response.json()
|
||||
except (json.JSONDecodeError, ValueError):
|
||||
return "invalid_response"
|
||||
if (
|
||||
not isinstance(payload, dict)
|
||||
or not payload.get("uuid")
|
||||
or "status" not in payload
|
||||
):
|
||||
return "invalid_response"
|
||||
return None
|
||||
|
||||
|
||||
@@ -492,15 +500,22 @@ class DeployCommand(BaseCommand, PlusAPIMixin):
|
||||
self._telemetry.crew_deployment_failed_span(
|
||||
failure_reason, source=source, status_code=response.status_code
|
||||
)
|
||||
self._validate_response(response)
|
||||
# Prints the API's own details and exits for every non-2xx and for
|
||||
# a body that is not JSON. Only a 2xx that parsed but is not a
|
||||
# creation payload gets past it.
|
||||
self._validate_response(response)
|
||||
console.print(
|
||||
"Unexpected response from the Enterprise API: no deployment uuid was returned.",
|
||||
style="bold red",
|
||||
)
|
||||
raise SystemExit(1)
|
||||
|
||||
json_response = response.json()
|
||||
# After _validate_response, not before: it raises SystemExit on a failed
|
||||
# create, so the span cannot fire for a deployment that was not made. This
|
||||
# is the first point at which the uuid exists -- the pre-flight span at the
|
||||
# top of this method counts the attempt and cannot carry it.
|
||||
created_uuid = json_response.get("uuid")
|
||||
# Only here, after the response has been classified as a creation: this
|
||||
# is the first point at which the uuid exists -- the pre-flight span at
|
||||
# the top of this method counts the attempt and cannot carry it.
|
||||
self._telemetry.crew_deployment_created_span(
|
||||
uuid=str(created_uuid) if created_uuid else None, source=source
|
||||
uuid=str(json_response["uuid"]), source=source
|
||||
)
|
||||
self._display_creation_success(json_response)
|
||||
|
||||
|
||||
@@ -4,7 +4,7 @@ import zipfile
|
||||
|
||||
import pytest
|
||||
|
||||
from crewai_cli.deploy.archive import create_project_zip
|
||||
from crewai_cli.deploy.archive import ArchiveError, create_project_zip
|
||||
|
||||
|
||||
def test_create_project_zip_excludes_local_artifacts(tmp_path: Path):
|
||||
@@ -305,3 +305,31 @@ type = "crew"
|
||||
assert "run_crew" not in pyproject
|
||||
assert "json_crew =" not in pyproject
|
||||
assert "[project.scripts]" not in pyproject
|
||||
|
||||
|
||||
def test_create_project_zip_with_nothing_to_deploy_raises_archive_error(
|
||||
tmp_path: Path,
|
||||
):
|
||||
"""Still a ValueError for existing callers, and a distinct type for the deploy command."""
|
||||
with pytest.raises(ArchiveError, match="No deployable project files were found"):
|
||||
create_project_zip("demo", project_dir=tmp_path)
|
||||
assert issubclass(ArchiveError, ValueError)
|
||||
|
||||
|
||||
def test_create_project_zip_wraps_a_write_failure_and_removes_the_partial_file(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
):
|
||||
(tmp_path / "pyproject.toml").write_text("[project]\nname = 'demo'\n")
|
||||
(tmp_path / "uv.lock").write_text("# lock\n")
|
||||
created: list[Path] = []
|
||||
|
||||
def failing_zipfile(path, *args, **kwargs):
|
||||
created.append(Path(path))
|
||||
raise OSError("disk full")
|
||||
|
||||
monkeypatch.setattr("crewai_cli.deploy.archive.zipfile.ZipFile", failing_zipfile)
|
||||
|
||||
with pytest.raises(ArchiveError, match="Could not build the project ZIP: disk full"):
|
||||
create_project_zip("demo", project_dir=tmp_path)
|
||||
|
||||
assert created and not created[0].exists()
|
||||
|
||||
@@ -9,6 +9,7 @@ import pytest
|
||||
import json
|
||||
|
||||
import crewai_cli.deploy.main as deploy_main
|
||||
from crewai_cli.deploy.archive import ArchiveError
|
||||
import httpx
|
||||
from crewai_cli.deploy.validate import Severity, ValidationResult
|
||||
from crewai_cli.utils import parse_toml
|
||||
@@ -942,13 +943,13 @@ class TestDeployCommand(unittest.TestCase):
|
||||
initialized_repository = MagicMock()
|
||||
initialized_repository.origin_url.return_value = None
|
||||
mock_repository.initialize.return_value = initialized_repository
|
||||
mock_create_project_zip.side_effect = ValueError(
|
||||
mock_create_project_zip.side_effect = ArchiveError(
|
||||
"No deployable project files were found."
|
||||
)
|
||||
|
||||
with patch.object(self.deploy_command, "_telemetry") as telemetry:
|
||||
with patch("sys.stdout", new=StringIO()):
|
||||
with pytest.raises(ValueError, match="No deployable project files"):
|
||||
with pytest.raises(ArchiveError, match="No deployable project files"):
|
||||
self.deploy_command.create_crew(skip_validate=True, confirm=True)
|
||||
|
||||
telemetry.crew_deployment_failed_span.assert_called_once_with(
|
||||
@@ -956,6 +957,73 @@ class TestDeployCommand(unittest.TestCase):
|
||||
)
|
||||
self.mock_client.create_crew_from_zip.assert_not_called()
|
||||
|
||||
@patch("crewai_cli.deploy.main.fetch_and_json_env_file")
|
||||
@patch("crewai_cli.deploy.main.git.Repository")
|
||||
@patch("builtins.input")
|
||||
def test_a_git_helper_error_is_not_a_zip_error(
|
||||
self, mock_input, mock_repository, mock_fetch_env
|
||||
):
|
||||
"""The git helpers raise plain ValueError; only ArchiveError is a ZIP problem."""
|
||||
self._git_path(mock_input, mock_repository, mock_fetch_env)
|
||||
mock_repository.return_value.origin_url.side_effect = ValueError(
|
||||
"Git remote lookup failed"
|
||||
)
|
||||
|
||||
with patch.object(self.deploy_command, "_telemetry") as telemetry:
|
||||
with patch("sys.stdout", new=StringIO()):
|
||||
with pytest.raises(ValueError, match="Git remote lookup failed"):
|
||||
self.deploy_command.create_crew(skip_validate=True)
|
||||
|
||||
telemetry.crew_deployment_failed_span.assert_called_once_with(
|
||||
"unexpected", source="cli"
|
||||
)
|
||||
|
||||
@patch("crewai_cli.deploy.main.fetch_and_json_env_file")
|
||||
@patch("crewai_cli.deploy.main.git.Repository")
|
||||
@patch("builtins.input")
|
||||
def test_a_non_json_error_page_keeps_its_api_class(
|
||||
self, mock_input, mock_repository, mock_fetch_env
|
||||
):
|
||||
"""A gateway's HTML 502 is an api_5xx, not an invalid response."""
|
||||
self._git_path(mock_input, mock_repository, mock_fetch_env)
|
||||
self.mock_client.create_crew.return_value = self._api_response(
|
||||
502, ValueError("not json")
|
||||
)
|
||||
|
||||
with patch.object(self.deploy_command, "_telemetry") as telemetry:
|
||||
with patch("sys.stdout", new=StringIO()):
|
||||
with self.assertRaises(SystemExit):
|
||||
self.deploy_command.create_crew(skip_validate=True)
|
||||
|
||||
telemetry.crew_deployment_failed_span.assert_called_once_with(
|
||||
"api_5xx", source="cli", status_code=502
|
||||
)
|
||||
|
||||
@patch("crewai_cli.deploy.main.fetch_and_json_env_file")
|
||||
@patch("crewai_cli.deploy.main.git.Repository")
|
||||
@patch("builtins.input")
|
||||
def test_a_success_body_that_is_not_a_creation_reports_invalid_response(
|
||||
self, mock_input, mock_repository, mock_fetch_env
|
||||
):
|
||||
"""A 2xx without a deployment uuid is not a success and must not crash."""
|
||||
self._git_path(mock_input, mock_repository, mock_fetch_env)
|
||||
for body in ([{"uuid": "in-a-list"}], {"status": "created"}, {}):
|
||||
with self.subTest(body=body):
|
||||
self.mock_client.create_crew.return_value = self._api_response(
|
||||
200, body
|
||||
)
|
||||
with patch.object(self.deploy_command, "_telemetry") as telemetry:
|
||||
with patch("sys.stdout", new=StringIO()) as fake_out:
|
||||
with self.assertRaises(SystemExit) as exit_info:
|
||||
self.deploy_command.create_crew(skip_validate=True)
|
||||
|
||||
assert exit_info.exception.code == 1
|
||||
assert "no deployment uuid was returned" in fake_out.getvalue()
|
||||
telemetry.crew_deployment_failed_span.assert_called_once_with(
|
||||
"invalid_response", source="cli", status_code=200
|
||||
)
|
||||
telemetry.crew_deployment_created_span.assert_not_called()
|
||||
|
||||
@patch("crewai_cli.deploy.main.fetch_and_json_env_file")
|
||||
@patch("crewai_cli.deploy.main.git.Repository")
|
||||
@patch("builtins.input")
|
||||
|
||||
Reference in New Issue
Block a user