From 2489064be469d68dbb902eacc0415a9f9100a0a4 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 30 Mar 2026 21:40:56 +0200 Subject: [PATCH 1/2] fix: apm deps update was a no-op -- delegate to install engine `apm deps update` called download_package() but never regenerated the lockfile, re-integrated deployed primitives, or handled transitive dependencies. This made the command silently report success while doing nothing useful. Rewrite the command to call _install_apm_dependencies() directly with update_refs=True. This ensures the lockfile, deployed files, and integration state are all refreshed correctly through the battle-tested install pipeline. Changes: - Rewrite deps update to delegate to install engine (not ctx.invoke) - Add --verbose, --force, --target, --parallel-downloads flags - Support multi-package update: apm deps update org/a org/b - Show SHA diff summary (old -> new per changed package) - Validate requested packages exist in manifest before engine call - Delete broken _update_single_package/_update_all_packages (~100 lines) - Clean dead exports from __init__.py - Add 17 behavioral tests (mock engine, not source code inspection) - Update CLI reference and dependency guide docs Co-authored-by: Maxim Salnikov <1560278+webmaxru@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 + docs/src/content/docs/guides/dependencies.md | 5 +- .../content/docs/reference/cli-commands.md | 26 +- src/apm_cli/commands/deps/__init__.py | 4 - src/apm_cli/commands/deps/_utils.py | 108 +--- src/apm_cli/commands/deps/cli.py | 202 ++++++-- tests/unit/test_deps_update_command.py | 487 ++++++++++++++++++ 7 files changed, 682 insertions(+), 154 deletions(-) create mode 100644 tests/unit/test_deps_update_command.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c38126a..e78b6e0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm deps update` was a no-op -- rewrote to delegate to the install engine so lockfile, deployed files, and integration state are all refreshed correctly (#485) + ## [0.8.6] - 2026-03-27 ### Added diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index 7134dc01..d5bce0c8 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -554,12 +554,15 @@ dependencies: ### Updating Dependencies ```bash -# Update all dependencies to latest versions +# Update all dependencies to latest refs apm deps update # Update specific dependency apm deps update apm-sample-package +# Update with verbose output +apm deps update --verbose + # Install with updates (equivalent to update) apm install --update ``` diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index 7a695e20..b499dd48 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -717,22 +717,38 @@ apm deps clean --yes #### `apm deps update` - Update APM dependencies -Update installed APM dependencies to their latest versions. +Re-resolve git references to their latest commits, download updated content, +re-integrate primitives, and regenerate the lockfile. ```bash -apm deps update [PACKAGE] +apm deps update [PACKAGES...] [OPTIONS] ``` **Arguments:** -- `PACKAGE` - Optional. Update specific package only +- `PACKAGES` - Optional. One or more packages to update. Omit to update all. + +**Options:** +- `--verbose, -v` - Show detailed update information +- `--force` - Overwrite locally-authored files on collision +- `--target, -t` - Force deployment to a specific target (copilot, claude, cursor, opencode, vscode, agents, all) +- `--parallel-downloads` - Max concurrent downloads (default: 4) **Examples:** ```bash -# Update all APM dependencies to latest versions +# Update all APM dependencies to latest refs apm deps update -# Update specific package to latest version +# Update a specific package apm deps update compliance-rules + +# Update multiple packages +apm deps update org/pkg-a org/pkg-b + +# Update with verbose output +apm deps update --verbose + +# Force overwrite local files on collision +apm deps update --force ``` ### `apm mcp` - Browse MCP server registry diff --git a/src/apm_cli/commands/deps/__init__.py b/src/apm_cli/commands/deps/__init__.py index e341d327..89e750db 100644 --- a/src/apm_cli/commands/deps/__init__.py +++ b/src/apm_cli/commands/deps/__init__.py @@ -9,8 +9,6 @@ _get_detailed_context_counts, _get_package_display_info, _get_detailed_package_info, - _update_single_package, - _update_all_packages, ) __all__ = [ @@ -29,6 +27,4 @@ "_get_detailed_context_counts", "_get_package_display_info", "_get_detailed_package_info", - "_update_single_package", - "_update_all_packages", ] diff --git a/src/apm_cli/commands/deps/_utils.py b/src/apm_cli/commands/deps/_utils.py index 16383cec..01c70127 100644 --- a/src/apm_cli/commands/deps/_utils.py +++ b/src/apm_cli/commands/deps/_utils.py @@ -1,11 +1,10 @@ """Utility helpers for APM dependency commands.""" from pathlib import Path -from typing import Any, Dict, List, Optional +from typing import Any, Dict -from ...constants import APM_DIR, APM_MODULES_DIR, APM_YML_FILENAME, SKILL_MD_FILENAME +from ...constants import APM_DIR, APM_YML_FILENAME, SKILL_MD_FILENAME from ...models.apm_package import APMPackage -from ...deps.github_downloader import GitHubPackageDownloader def _scan_installed_packages(apm_modules_dir: Path) -> list: @@ -221,106 +220,3 @@ def _get_detailed_package_info(package_path: Path) -> Dict[str, Any]: 'workflows': 0, 'hooks': 0 } - - -def _update_single_package(package_name: str, project_deps: List, apm_modules_path: Path, logger=None): - """Update a specific package.""" - if logger is None: - from ...core.command_logger import CommandLogger - logger = CommandLogger("deps-update") - - # Find the dependency reference for this package - target_dep = None - for dep in project_deps: - if dep.get_display_name() == package_name or dep.repo_url.split('/')[-1] == package_name: - target_dep = dep - break - - if not target_dep: - logger.error(f"Package '{package_name}' not found in apm.yml dependencies") - return - - # Find the installed package directory using namespaced structure - # GitHub: owner/repo (2 parts) - # Azure DevOps: org/project/repo (3 parts) - package_dir = None - if target_dep.alias: - package_dir = apm_modules_path / target_dep.alias - else: - # Parse path from repo_url - repo_parts = target_dep.repo_url.split('/') - if target_dep.is_azure_devops() and len(repo_parts) >= 3: - # ADO structure: apm_modules/org/project/repo - package_dir = apm_modules_path / repo_parts[0] / repo_parts[1] / repo_parts[2] - elif len(repo_parts) >= 2: - package_dir = apm_modules_path / repo_parts[0] / repo_parts[1] - else: - # Fallback to simple name matching - package_dir = apm_modules_path / package_name - - if not package_dir.exists(): - logger.error(f"Package '{package_name}' not installed in apm_modules/") - logger.progress(f"Run 'apm install' to install it first") - return - - try: - downloader = GitHubPackageDownloader() - logger.progress(f"Updating {target_dep.repo_url}...") - - # Download latest version - package_info = downloader.download_package(target_dep, package_dir) - - logger.success(f"Updated {target_dep.repo_url}") - - except Exception as e: - logger.error(f"Failed to update {package_name}: {e}") - - -def _update_all_packages(project_deps: List, apm_modules_path: Path, logger=None): - """Update all packages.""" - if logger is None: - from ...core.command_logger import CommandLogger - logger = CommandLogger("deps-update") - - if not project_deps: - logger.progress("No APM dependencies to update") - return - - logger.start(f"Updating {len(project_deps)} APM dependencies...") - - downloader = GitHubPackageDownloader() - updated_count = 0 - - for dep in project_deps: - # Determine package directory using namespaced structure - # GitHub: apm_modules/owner/repo (2 parts) - # Azure DevOps: apm_modules/org/project/repo (3 parts) - if dep.alias: - package_dir = apm_modules_path / dep.alias - else: - # Parse path from repo_url - repo_parts = dep.repo_url.split('/') - if dep.is_azure_devops() and len(repo_parts) >= 3: - # ADO structure - package_dir = apm_modules_path / repo_parts[0] / repo_parts[1] / repo_parts[2] - elif len(repo_parts) >= 2: - package_dir = apm_modules_path / repo_parts[0] / repo_parts[1] - else: - # Fallback to simple repo name (shouldn't happen) - package_dir = apm_modules_path / dep.repo_url - - if not package_dir.exists(): - logger.warning(f"{dep.repo_url} not installed - skipping") - continue - - try: - logger.verbose_detail(f" Updating {dep.repo_url}...") - package_info = downloader.download_package(dep, package_dir) - updated_count += 1 - logger.success(f" {dep.repo_url}") - - except Exception as e: - logger.error(f" Failed to update {dep.repo_url}: {e}") - continue - - logger.success(f"Updated {updated_count} of {len(project_deps)} packages") diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index ac115ba4..a734b4db 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -11,10 +11,6 @@ from ...models.apm_package import APMPackage, ValidationResult, validate_apm_package from ...core.command_logger import CommandLogger -# Import APM dependency system components (with fallback) -from ...deps.github_downloader import GitHubPackageDownloader -from ...deps.apm_resolver import APMDependencyResolver - from ._utils import ( _is_nested_under_package, _count_primitives, @@ -23,8 +19,6 @@ _get_detailed_context_counts, _get_package_display_info, _get_detailed_package_info, - _update_single_package, - _update_all_packages, ) @@ -443,43 +437,175 @@ def clean(dry_run: bool, yes: bool): sys.exit(1) -@deps.command(help="Update APM dependencies") -@click.argument('package', required=False) -def update(package: Optional[str]): - """Update specific package or all if no package specified.""" - logger = CommandLogger("deps-update") +@deps.command(help="Update APM dependencies to latest refs") +@click.argument("packages", nargs=-1) +@click.option("--verbose", "-v", is_flag=True, help="Show detailed update information") +@click.option( + "--force", is_flag=True, + help="Overwrite locally-authored files on collision", +) +@click.option( + "--target", "-t", + type=click.Choice( + ["copilot", "claude", "cursor", "opencode", "vscode", "agents", "all"], + case_sensitive=False, + ), + default=None, + help="Force deployment to a specific target (overrides auto-detection)", +) +@click.option( + "--parallel-downloads", + type=int, + default=4, + show_default=True, + help="Max concurrent package downloads (0 to disable parallelism)", +) +def update(packages, verbose, force, target, parallel_downloads): + """Update APM dependencies to latest git refs. + + Re-resolves git references (branches/tags) to their current SHAs, + downloads updated content, re-integrates primitives, and regenerates + the lockfile. + + \b + Examples: + apm deps update # Update all packages + apm deps update org/repo # Update one package + apm deps update org/a org/b # Update specific packages + apm deps update --verbose # Show detailed progress + """ + from ..install import ( + _install_apm_dependencies, + APM_DEPS_AVAILABLE, + _APM_IMPORT_ERROR, + ) + from ...core.command_logger import InstallLogger + from ...core.auth import AuthResolver + + logger = InstallLogger(verbose=verbose, partial=bool(packages)) + + if not APM_DEPS_AVAILABLE: + logger.error("APM dependency system not available") + if _APM_IMPORT_ERROR: + logger.progress(f"Import error: {_APM_IMPORT_ERROR}") + sys.exit(1) + + project_root = Path.cwd() + apm_yml_path = project_root / APM_YML_FILENAME + + if not apm_yml_path.exists(): + logger.error(f"No {APM_YML_FILENAME} found in current directory") + sys.exit(1) - project_root = Path(".") - apm_modules_path = project_root / APM_MODULES_DIR - - if not apm_modules_path.exists(): - logger.progress("No apm_modules/ directory found - no packages to update") - return - - # Get project dependencies to validate updates try: - apm_yml_path = project_root / APM_YML_FILENAME - if not apm_yml_path.exists(): - logger.error(f"No {APM_YML_FILENAME} found in current directory") - return - - project_package = APMPackage.from_apm_yml(apm_yml_path) - project_deps = project_package.get_apm_dependencies() - - if not project_deps: - logger.progress("No APM dependencies defined in apm.yml") - return - + apm_package = APMPackage.from_apm_yml(apm_yml_path) except Exception as e: - logger.error(f"Error reading {APM_YML_FILENAME}: {e}") + logger.error(f"Failed to parse {APM_YML_FILENAME}: {e}") + sys.exit(1) + + all_deps = apm_package.get_apm_dependencies() + apm_package.get_dev_apm_dependencies() + if not all_deps: + logger.progress("No APM dependencies defined in apm.yml") return - - if package: - # Update specific package - _update_single_package(package, project_deps, apm_modules_path, logger=logger) + + # Validate requested packages exist in manifest + only_pkgs = None + if packages: + only_pkgs = list(packages) + known_keys = set() + for dep in all_deps: + known_keys.add(dep.get_unique_key()) + known_keys.add(dep.get_display_name()) + known_keys.add(dep.repo_url) + if hasattr(dep, "alias") and dep.alias: + known_keys.add(dep.alias) + parts = dep.repo_url.split("/") + if len(parts) >= 2: + known_keys.add(parts[-1]) + + for pkg in only_pkgs: + if pkg not in known_keys: + available = ", ".join(dep.get_display_name() for dep in all_deps) + logger.error(f"Package '{pkg}' not found in {APM_YML_FILENAME}") + logger.progress(f"Available: {available}") + sys.exit(1) + + # Snapshot old lockfile SHAs for before/after diff + from ...deps.lockfile import LockFile, get_lockfile_path, migrate_lockfile_if_needed + + lockfile_path = get_lockfile_path(project_root) + old_lockfile = LockFile.read(lockfile_path) + old_shas: dict = {} + if old_lockfile: + for key, dep in old_lockfile.dependencies.items(): + old_shas[key] = dep.resolved_commit + + migrate_lockfile_if_needed(project_root) + + auth_resolver = AuthResolver() + + if packages: + noun = f"{len(packages)} package(s)" + else: + noun = f"all {len(all_deps)} dependencies" + logger.start(f"Updating {noun}...") + + try: + install_result = _install_apm_dependencies( + apm_package, + update_refs=True, + verbose=verbose, + only_packages=only_pkgs, + force=force, + parallel_downloads=parallel_downloads, + logger=logger, + auth_resolver=auth_resolver, + target=target, + ) + except Exception as e: + logger.error(f"Update failed: {e}") + if not verbose: + logger.progress("Run with --verbose for detailed diagnostics") + sys.exit(1) + + # Show diagnostics if any + if install_result.diagnostics and install_result.diagnostics.has_diagnostics: + install_result.diagnostics.render_summary() + + # Compare old vs new lockfile SHAs to show what changed + new_lockfile = LockFile.read(lockfile_path) + changed: list = [] + if new_lockfile: + for key, dep in new_lockfile.dependencies.items(): + old_sha = old_shas.get(key) + new_sha = dep.resolved_commit + if old_sha and new_sha and old_sha != new_sha: + changed.append( + (key, old_sha[:8], new_sha[:8], dep.resolved_ref or "") + ) + + error_count = 0 + if install_result.diagnostics: + try: + error_count = int(install_result.diagnostics.error_count) + except (TypeError, ValueError): + error_count = 0 + + if changed: + pkg_noun = "package" if len(changed) == 1 else "packages" + if error_count > 0: + logger.warning( + f"Updated {len(changed)} {pkg_noun} with {error_count} error(s)." + ) + else: + logger.success(f"Updated {len(changed)} {pkg_noun}:") + for key, old_sha, new_sha, ref in changed: + ref_str = f" ({ref})" if ref else "" + click.echo(f" {key}{ref_str}: {old_sha} -> {new_sha}") + elif error_count > 0: + logger.error(f"Update failed with {error_count} error(s).") else: - # Update all packages - _update_all_packages(project_deps, apm_modules_path, logger=logger) + logger.success("All packages already at latest refs.") @deps.command(help="Show detailed package information") diff --git a/tests/unit/test_deps_update_command.py b/tests/unit/test_deps_update_command.py new file mode 100644 index 00000000..e5a44408 --- /dev/null +++ b/tests/unit/test_deps_update_command.py @@ -0,0 +1,487 @@ +"""Tests for `apm deps update` command. + +Validates CLI wiring, flag propagation to the install engine, error handling, +and update-specific output (SHA diffs). The install engine itself is mocked -- +these are CLI-layer tests, not integration tests. +""" + +import contextlib +import os +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +import yaml +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.models.results import InstallResult + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _minimal_apm_yml(deps=None, dev_deps=None): + """Return a minimal apm.yml dict with the given APM deps.""" + data = {"name": "test-project", "version": "1.0.0"} + if deps: + data["dependencies"] = {d: "main" for d in deps} + if dev_deps: + data["devDependencies"] = {d: "main" for d in dev_deps} + return yaml.dump(data, default_flow_style=False) + + +def _mock_dep(repo_url, alias=None): + """Create a mock DependencyReference with required methods.""" + dep = MagicMock() + dep.repo_url = repo_url + dep.alias = alias + dep.get_unique_key.return_value = repo_url + dep.get_display_name.return_value = alias or repo_url + dep.reference = "main" + dep.is_local = False + dep.local_path = None + dep.is_virtual = False + dep.virtual_path = None + return dep + + +def _mock_locked_dep(repo_url, sha, ref="main"): + """Create a mock LockedDependency.""" + dep = MagicMock() + dep.repo_url = repo_url + dep.resolved_commit = sha + dep.resolved_ref = ref + dep.get_unique_key.return_value = repo_url + return dep + + +# Common patch targets -- the update() function uses lazy imports. +_PATCH_ENGINE = "apm_cli.commands.install._install_apm_dependencies" +_PATCH_DEPS_AVAILABLE = "apm_cli.commands.install.APM_DEPS_AVAILABLE" +_PATCH_APM_PACKAGE = "apm_cli.commands.deps.cli.APMPackage" +_PATCH_LOCKFILE = "apm_cli.deps.lockfile.LockFile" +_PATCH_GET_LOCKFILE_PATH = "apm_cli.deps.lockfile.get_lockfile_path" +_PATCH_MIGRATE = "apm_cli.deps.lockfile.migrate_lockfile_if_needed" +_PATCH_AUTH = "apm_cli.core.auth.AuthResolver" + + +class TestDepsUpdateCommand: + """Test `apm deps update` CLI wiring and output.""" + + def setup_method(self): + self.runner = CliRunner() + try: + self.original_dir = os.getcwd() + except FileNotFoundError: + self.original_dir = str(Path(__file__).parent.parent.parent) + os.chdir(self.original_dir) + + def teardown_method(self): + try: + os.chdir(self.original_dir) + except (FileNotFoundError, OSError): + repo_root = Path(__file__).parent.parent.parent + os.chdir(str(repo_root)) + + @contextlib.contextmanager + def _chdir_tmp(self): + """Create a temp dir, chdir into it, restore CWD on exit.""" + with tempfile.TemporaryDirectory() as tmp_dir: + try: + os.chdir(tmp_dir) + yield Path(tmp_dir) + finally: + os.chdir(self.original_dir) + + # ------------------------------------------------------------------ + # Pre-flight validation + # ------------------------------------------------------------------ + + def test_no_apm_yml_exits_1(self): + """Exit 1 when no apm.yml exists.""" + with self._chdir_tmp(): + result = self.runner.invoke(cli, ["deps", "update"]) + assert result.exit_code == 1 + assert "No apm.yml found" in result.output + + @patch(_PATCH_APM_PACKAGE) + def test_no_deps_exits_0(self, mock_pkg_cls): + """Exit 0 with informational message when apm.yml has no APM deps.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml()) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + result = self.runner.invoke(cli, ["deps", "update"]) + assert result.exit_code == 0 + assert "No APM dependencies" in result.output + + @patch(_PATCH_APM_PACKAGE) + def test_unknown_package_exits_1(self, mock_pkg_cls): + """Exit 1 when requested package isn't in apm.yml.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/real-pkg"])) + mock_pkg = MagicMock() + dep = _mock_dep("org/real-pkg") + mock_pkg.get_apm_dependencies.return_value = [dep] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + result = self.runner.invoke(cli, ["deps", "update", "org/nonexistent"]) + assert result.exit_code == 1 + assert "not found in apm.yml" in result.output + + @patch(_PATCH_APM_PACKAGE) + def test_unknown_package_shows_available(self, mock_pkg_cls): + """Error message lists available packages.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/real-pkg"])) + mock_pkg = MagicMock() + dep = _mock_dep("org/real-pkg") + mock_pkg.get_apm_dependencies.return_value = [dep] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + result = self.runner.invoke(cli, ["deps", "update", "org/nonexistent"]) + assert "Available:" in result.output + assert "org/real-pkg" in result.output + + # ------------------------------------------------------------------ + # Engine delegation + # ------------------------------------------------------------------ + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_update_all_passes_update_refs_true( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """update_refs=True passed when no packages specified.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update"]) + assert result.exit_code == 0 + mock_engine.assert_called_once() + _, kwargs = mock_engine.call_args + assert kwargs["update_refs"] is True + assert kwargs["only_packages"] is None + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_update_single_passes_only_packages( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """only_packages=['org/pkg'] passed when package arg given.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update", "org/pkg"]) + assert result.exit_code == 0 + _, kwargs = mock_engine.call_args + assert kwargs["only_packages"] == ["org/pkg"] + assert kwargs["update_refs"] is True + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_force_flag_propagates( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """--force propagates to engine.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update", "--force"]) + assert result.exit_code == 0 + _, kwargs = mock_engine.call_args + assert kwargs["force"] is True + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_target_flag_propagates( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """--target propagates to engine.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update", "--target", "claude"]) + assert result.exit_code == 0 + _, kwargs = mock_engine.call_args + assert kwargs["target"] == "claude" + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_logger_passed_to_engine( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """An InstallLogger is passed to the engine for verbose output.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update"]) + assert result.exit_code == 0 + _, kwargs = mock_engine.call_args + assert kwargs["logger"] is not None + logger = kwargs["logger"] + assert hasattr(logger, "download_complete") + assert hasattr(logger, "lockfile_entry") + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_verbose_flag_propagates( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """--verbose propagates to engine and to the logger.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update", "--verbose"]) + assert result.exit_code == 0 + _, kwargs = mock_engine.call_args + assert kwargs["verbose"] is True + assert kwargs["logger"].verbose is True + + # ------------------------------------------------------------------ + # Output tests + # ------------------------------------------------------------------ + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_sha_diff_shown_when_changed( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """Output contains 'old_sha -> new_sha' when packages change.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + + # Old lockfile: SHA aaa... + old_locked = _mock_locked_dep("org/pkg", "aaa11111222233334444555566667777") + old_lockfile = MagicMock() + old_lockfile.dependencies = {"org/pkg": old_locked} + + # New lockfile: SHA bbb... + new_locked = _mock_locked_dep("org/pkg", "bbb11111222233334444555566667777") + new_lockfile = MagicMock() + new_lockfile.dependencies = {"org/pkg": new_locked} + + # First read returns old, second read (after engine) returns new + mock_lockfile_cls.read.side_effect = [old_lockfile, new_lockfile] + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update"]) + assert result.exit_code == 0 + assert "aaa11111" in result.output + assert "bbb11111" in result.output + assert "->" in result.output + assert "Updated 1 package" in result.output + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_already_latest_message( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """Shows 'already at latest refs' when SHAs unchanged.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + + # Same SHA before and after + same_sha = "aaa11111222233334444555566667777" + locked = _mock_locked_dep("org/pkg", same_sha) + lockfile = MagicMock() + lockfile.dependencies = {"org/pkg": locked} + mock_lockfile_cls.read.return_value = lockfile + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update"]) + assert result.exit_code == 0 + assert "already at latest refs" in result.output + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_engine_failure_exits_1( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """sys.exit(1) when engine raises an exception.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["org/pkg"])) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [_mock_dep("org/pkg")] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.side_effect = RuntimeError("Network timeout") + + result = self.runner.invoke(cli, ["deps", "update"]) + assert result.exit_code == 1 + assert "Update failed" in result.output + + # ------------------------------------------------------------------ + # Multi-package support + # ------------------------------------------------------------------ + + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_multiple_packages_propagate( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """Multiple package args propagate as only_packages list.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text( + _minimal_apm_yml(deps=["org/pkg-a", "org/pkg-b"]) + ) + mock_pkg = MagicMock() + mock_pkg.get_apm_dependencies.return_value = [ + _mock_dep("org/pkg-a"), + _mock_dep("org/pkg-b"), + ] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke( + cli, ["deps", "update", "org/pkg-a", "org/pkg-b"] + ) + assert result.exit_code == 0 + _, kwargs = mock_engine.call_args + assert kwargs["only_packages"] == ["org/pkg-a", "org/pkg-b"] + + +class TestDeadCodeRemoval: + """Verify broken update helpers have been removed.""" + + def test_update_single_package_removed(self): + """_update_single_package deleted from _utils.py.""" + from apm_cli.commands.deps import _utils as utils + assert not hasattr(utils, "_update_single_package") + + def test_update_all_packages_removed(self): + """_update_all_packages deleted from _utils.py.""" + from apm_cli.commands.deps import _utils as utils + assert not hasattr(utils, "_update_all_packages") + + def test_not_in_package_exports(self): + """Dead functions not exported from __init__.py.""" + from apm_cli.commands.deps import __init__ as init_mod + all_names = getattr(init_mod, "__all__", []) + assert "_update_single_package" not in all_names + assert "_update_all_packages" not in all_names From e439f6a9ac507bdf57bec632f32637d000eace07 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Mon, 30 Mar 2026 21:54:25 +0200 Subject: [PATCH 2/2] Address PR review: normalize short names, fix lockfile baseline, fix test import - Normalize user-supplied package names (short names, aliases) to canonical owner/repo keys before passing to install engine - Migrate legacy lockfile before reading baseline SHAs (fixes false 'already current' when no baseline exists) - Fix __init__ import in dead-code-removal test - Add test for short-name-to-canonical normalization - Update doc examples to use canonical owner/repo form - Fix CHANGELOG to reference #493 with @webmaxru credit Co-authored-by: Maxim Salnikov <1560278+webmaxru@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 2 +- docs/src/content/docs/guides/dependencies.md | 4 +- .../content/docs/reference/cli-commands.md | 4 +- src/apm_cli/commands/deps/cli.py | 41 ++++++++++++------- tests/unit/test_deps_update_command.py | 33 ++++++++++++++- 5 files changed, 63 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e78b6e0a..9f16e3b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- `apm deps update` was a no-op -- rewrote to delegate to the install engine so lockfile, deployed files, and integration state are all refreshed correctly (#485) +- `apm deps update` was a no-op -- rewrote to delegate to the install engine so lockfile, deployed files, and integration state are all refreshed correctly -- by @webmaxru (#493) ## [0.8.6] - 2026-03-27 diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index d5bce0c8..6064fd33 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -557,8 +557,8 @@ dependencies: # Update all dependencies to latest refs apm deps update -# Update specific dependency -apm deps update apm-sample-package +# Update specific dependency (use the owner/repo form from apm.yml) +apm deps update owner/apm-sample-package # Update with verbose output apm deps update --verbose diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index b499dd48..c84f0809 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -738,8 +738,8 @@ apm deps update [PACKAGES...] [OPTIONS] # Update all APM dependencies to latest refs apm deps update -# Update a specific package -apm deps update compliance-rules +# Update a specific package (short name or full owner/repo) +apm deps update owner/compliance-rules # Update multiple packages apm deps update org/pkg-a org/pkg-b diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index a734b4db..d5c1de86 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -508,40 +508,51 @@ def update(packages, verbose, force, target, parallel_downloads): logger.progress("No APM dependencies defined in apm.yml") return - # Validate requested packages exist in manifest + # Validate and normalize requested packages to canonical dependency keys. + # The install engine matches only_packages by DependencyReference identity + # (e.g. "owner/repo"), so short names like "compliance-rules" must be + # mapped to their canonical form before calling the engine. only_pkgs = None if packages: - only_pkgs = list(packages) - known_keys = set() + token_to_canonical: Dict[str, str] = {} for dep in all_deps: - known_keys.add(dep.get_unique_key()) - known_keys.add(dep.get_display_name()) - known_keys.add(dep.repo_url) + canonical_key = dep.get_unique_key() or dep.repo_url or dep.get_display_name() + tokens = {canonical_key, dep.get_display_name(), dep.repo_url} if hasattr(dep, "alias") and dep.alias: - known_keys.add(dep.alias) + tokens.add(dep.alias) parts = dep.repo_url.split("/") if len(parts) >= 2: - known_keys.add(parts[-1]) - - for pkg in only_pkgs: - if pkg not in known_keys: + tokens.add(parts[-1]) + for token in tokens: + if token and token not in token_to_canonical: + token_to_canonical[token] = canonical_key + + only_pkgs = [] + seen: Dict[str, bool] = {} + for pkg in packages: + canonical = token_to_canonical.get(pkg) + if not canonical: available = ", ".join(dep.get_display_name() for dep in all_deps) logger.error(f"Package '{pkg}' not found in {APM_YML_FILENAME}") logger.progress(f"Available: {available}") sys.exit(1) + if canonical not in seen: + seen[canonical] = True + only_pkgs.append(canonical) - # Snapshot old lockfile SHAs for before/after diff + # Migrate legacy lockfile first, then snapshot SHAs for before/after diff from ...deps.lockfile import LockFile, get_lockfile_path, migrate_lockfile_if_needed lockfile_path = get_lockfile_path(project_root) + migrate_lockfile_if_needed(project_root) + old_lockfile = LockFile.read(lockfile_path) + had_baseline = old_lockfile is not None old_shas: dict = {} if old_lockfile: for key, dep in old_lockfile.dependencies.items(): old_shas[key] = dep.resolved_commit - migrate_lockfile_if_needed(project_root) - auth_resolver = AuthResolver() if packages: @@ -604,6 +615,8 @@ def update(packages, verbose, force, target, parallel_downloads): click.echo(f" {key}{ref_str}: {old_sha} -> {new_sha}") elif error_count > 0: logger.error(f"Update failed with {error_count} error(s).") + elif not had_baseline: + logger.success("Update complete.") else: logger.success("All packages already at latest refs.") diff --git a/tests/unit/test_deps_update_command.py b/tests/unit/test_deps_update_command.py index e5a44408..88660eb2 100644 --- a/tests/unit/test_deps_update_command.py +++ b/tests/unit/test_deps_update_command.py @@ -212,6 +212,35 @@ def test_update_single_passes_only_packages( assert kwargs["only_packages"] == ["org/pkg"] assert kwargs["update_refs"] is True + @patch(_PATCH_AUTH) + @patch(_PATCH_MIGRATE) + @patch(_PATCH_GET_LOCKFILE_PATH) + @patch(_PATCH_LOCKFILE) + @patch(_PATCH_ENGINE) + @patch(_PATCH_APM_PACKAGE) + def test_short_name_normalized_to_canonical_key( + self, mock_pkg_cls, mock_engine, mock_lockfile_cls, + mock_get_path, mock_migrate, mock_auth, + ): + """Short repo basename is normalized to canonical owner/repo key.""" + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_minimal_apm_yml(deps=["owner/compliance-rules"])) + mock_pkg = MagicMock() + dep = _mock_dep("owner/compliance-rules") + mock_pkg.get_apm_dependencies.return_value = [dep] + mock_pkg.get_dev_apm_dependencies.return_value = [] + mock_pkg_cls.from_apm_yml.return_value = mock_pkg + + mock_get_path.return_value = tmp / "apm.lock.yaml" + mock_lockfile_cls.read.return_value = None + mock_engine.return_value = InstallResult() + + result = self.runner.invoke(cli, ["deps", "update", "compliance-rules"]) + assert result.exit_code == 0 + _, kwargs = mock_engine.call_args + assert kwargs["only_packages"] == ["owner/compliance-rules"] + assert kwargs["update_refs"] is True + @patch(_PATCH_AUTH) @patch(_PATCH_MIGRATE) @patch(_PATCH_GET_LOCKFILE_PATH) @@ -481,7 +510,7 @@ def test_update_all_packages_removed(self): def test_not_in_package_exports(self): """Dead functions not exported from __init__.py.""" - from apm_cli.commands.deps import __init__ as init_mod - all_names = getattr(init_mod, "__all__", []) + import apm_cli.commands.deps as deps_mod + all_names = getattr(deps_mod, "__all__", []) assert "_update_single_package" not in all_names assert "_update_all_packages" not in all_names