From 6796b1ce165d462a2f05acea36dc182433de2c4e Mon Sep 17 00:00:00 2001 From: Joao Moura Date: Mon, 15 Jun 2026 01:36:24 -0700 Subject: [PATCH] fix(cli): address deploy zip review feedback --- lib/cli/src/crewai_cli/deploy/archive.py | 31 +++++++++++-------- lib/cli/src/crewai_cli/git.py | 14 ++++----- lib/cli/src/crewai_cli/run_crew.py | 7 ++++- lib/cli/tests/deploy/test_archive.py | 38 ++++++++++++++++++++++++ lib/cli/tests/test_git.py | 12 ++++++++ 5 files changed, 82 insertions(+), 20 deletions(-) diff --git a/lib/cli/src/crewai_cli/deploy/archive.py b/lib/cli/src/crewai_cli/deploy/archive.py index 5e76152e5..b64b7e162 100644 --- a/lib/cli/src/crewai_cli/deploy/archive.py +++ b/lib/cli/src/crewai_cli/deploy/archive.py @@ -73,25 +73,28 @@ def create_project_zip( def _project_files(root: Path, repository: git.Repository | None = None) -> list[Path]: if repository is not None: - try: - files = [Path(path) for path in repository.deployable_files()] - return [ - path - for path in files - if not _is_excluded(path) and (root / path).is_file() - ] - except Exception: # noqa: S110 - pass + files = [Path(path) for path in repository.deployable_files()] + return [ + path + for path in files + if not _is_excluded(path) and _is_regular_file(root / path) + ] return [ path for path in _walk_files(root) - if not _is_excluded(path) and (root / path).is_file() + if not _is_excluded(path) and _is_regular_file(root / path) ] def _walk_files(root: Path) -> list[Path]: - return [path.relative_to(root) for path in root.rglob("*") if path.is_file()] + return [ + path.relative_to(root) for path in root.rglob("*") if _is_regular_file(path) + ] + + +def _is_regular_file(path: Path) -> bool: + return path.is_file() and not path.is_symlink() def _is_excluded(path: Path) -> bool: @@ -112,9 +115,13 @@ def _stage_project(root: Path, files: list[Path]) -> Path: try: for relative_path in files: + source = root / relative_path + if not _is_regular_file(source): + continue + destination = staging_root / relative_path destination.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(root / relative_path, destination) + shutil.copy2(source, destination) if _is_json_crew_project(staging_root): _add_json_crew_deploy_wrapper(staging_root) diff --git a/lib/cli/src/crewai_cli/git.py b/lib/cli/src/crewai_cli/git.py index 58a08afe3..47f933573 100644 --- a/lib/cli/src/crewai_cli/git.py +++ b/lib/cli/src/crewai_cli/git.py @@ -51,8 +51,9 @@ class Repository: def fetch(self) -> None: """Fetch latest updates from the remote.""" - result = subprocess.run( - ["git", "fetch"], # noqa: S607 + command = ["git", "fetch"] + result = subprocess.run( # noqa: S603 + command, cwd=self.path, capture_output=True, text=True, @@ -61,11 +62,10 @@ class Repository: return if "No remote repository specified" in result.stderr: return - raise subprocess.CalledProcessError( - result.returncode, - ["git", "fetch"], - output=result.stdout, - stderr=result.stderr, + details = result.stderr.strip() or result.stdout.strip() or "no output" + raise ValueError( + f"Git fetch failed with exit code {result.returncode} " + f"for command {command!r}: {details}" ) @classmethod diff --git a/lib/cli/src/crewai_cli/run_crew.py b/lib/cli/src/crewai_cli/run_crew.py index 78a810a4f..1f828b970 100644 --- a/lib/cli/src/crewai_cli/run_crew.py +++ b/lib/cli/src/crewai_cli/run_crew.py @@ -36,7 +36,10 @@ class CrewType(Enum): # otherwise placeholders are interpolated at runtime but never prompted for. _INPUT_PLACEHOLDER_RE = re.compile(r"(? Any click.echo(f"An unexpected error occurred while running the JSON crew: {e}") raise SystemExit(1) from e + return None + def _chain_deploy() -> None: from rich.console import Console diff --git a/lib/cli/tests/deploy/test_archive.py b/lib/cli/tests/deploy/test_archive.py index 3ed3630f5..8ae915f9b 100644 --- a/lib/cli/tests/deploy/test_archive.py +++ b/lib/cli/tests/deploy/test_archive.py @@ -1,6 +1,8 @@ from pathlib import Path import zipfile +import pytest + from crewai_cli.deploy.archive import create_project_zip @@ -54,6 +56,42 @@ def test_create_project_zip_uses_repository_file_list(tmp_path: Path): assert names == {"pyproject.toml", "uv.lock"} +def test_create_project_zip_does_not_fallback_when_repository_listing_fails( + tmp_path: Path, +): + (tmp_path / "pyproject.toml").write_text("[project]\nname = 'demo'\n") + + class RepositoryStub: + def deployable_files(self) -> list[str]: + raise RuntimeError("git listing failed") + + with pytest.raises(RuntimeError, match="git listing failed"): + create_project_zip( + "demo", + project_dir=tmp_path, + repository=RepositoryStub(), # type: ignore[arg-type] + ) + + +def test_create_project_zip_excludes_symlinked_files(tmp_path: Path): + (tmp_path / "pyproject.toml").write_text("[project]\nname = 'demo'\n") + outside_file = tmp_path.parent / f"{tmp_path.name}-secret.txt" + outside_file.write_text("secret\n") + try: + (tmp_path / "external-secret.txt").symlink_to(outside_file) + except OSError as exc: + pytest.skip(f"symlinks are not supported in this environment: {exc}") + + archive_path = create_project_zip("demo", project_dir=tmp_path) + try: + with zipfile.ZipFile(archive_path) as archive: + names = set(archive.namelist()) + finally: + archive_path.unlink(missing_ok=True) + + assert names == {"pyproject.toml"} + + def test_create_project_zip_adds_json_project_wrapper(tmp_path: Path): (tmp_path / "pyproject.toml").write_text( """ diff --git a/lib/cli/tests/test_git.py b/lib/cli/tests/test_git.py index 3815113cf..41ffbb222 100644 --- a/lib/cli/tests/test_git.py +++ b/lib/cli/tests/test_git.py @@ -31,6 +31,18 @@ def test_is_git_not_installed(fp): Repository(path=".") +def test_fetch_failure_raises_value_error(fp): + fp.register(["git", "--version"], stdout="git version 2.30.0\n") + fp.register(["git", "rev-parse", "--is-inside-work-tree"], stdout="true\n") + fp.register(["git", "fetch"], returncode=128, stderr="remote unavailable\n") + + with pytest.raises( + ValueError, + match=r"Git fetch failed with exit code 128 for command \['git', 'fetch'\]: remote unavailable", + ): + Repository(path=".") + + def test_status(fp, repository): fp.register( ["git", "status", "--branch", "--porcelain"],