From f9e4240c9b8a0707052c6d21321fcbab7a147a07 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Feb 2026 23:08:02 +0000 Subject: [PATCH 1/8] Initial plan From f7420ea153dc6b9c8116e8b80b8fb26ee997e6dc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Feb 2026 23:21:14 +0000 Subject: [PATCH 2/8] Fix transitive dependency handling in compile and orphan detection Read from apm.lock (which tracks transitive deps) in addition to apm.yml: - get_dependency_declaration_order() now includes transitive deps for compile - _check_orphaned_packages() no longer flags transitive deps as orphaned - prune command accounts for transitive deps in expected_installed set - Added get_lockfile_installed_paths() helper to lockfile module - Added 17 unit tests covering all transitive dependency scenarios Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> --- src/apm_cli/cli.py | 24 ++- src/apm_cli/deps/__init__.py | 3 +- src/apm_cli/deps/lockfile.py | 41 +++++ src/apm_cli/primitives/discovery.py | 21 ++- tests/test_enhanced_discovery.py | 110 ++++++++++++++ tests/unit/test_transitive_deps.py | 223 ++++++++++++++++++++++++++++ 6 files changed, 418 insertions(+), 4 deletions(-) create mode 100644 tests/unit/test_transitive_deps.py diff --git a/src/apm_cli/cli.py b/src/apm_cli/cli.py index 5f5fbc352..032f6e48f 100644 --- a/src/apm_cli/cli.py +++ b/src/apm_cli/cli.py @@ -130,7 +130,11 @@ def _lazy_confirm(): def _check_orphaned_packages(): - """Check for packages in apm_modules/ that are not declared in apm.yml. + """Check for packages in apm_modules/ that are not declared in apm.yml or apm.lock. + + Considers both direct dependencies (from apm.yml) and transitive dependencies + (from apm.lock) as expected packages, so transitive deps are not falsely + flagged as orphaned. Returns: List[str]: List of orphaned package names in org/repo or org/project/repo format @@ -164,6 +168,14 @@ def _check_orphaned_packages(): except ValueError: # If path is not relative to apm_modules_dir, use as-is expected_installed.add(str(install_path)) + + # Also include transitive dependencies from apm.lock + try: + from apm_cli.deps.lockfile import get_lockfile_installed_paths + lockfile_paths = get_lockfile_installed_paths(Path.cwd()) + expected_installed.update(lockfile_paths) + except Exception: + pass # If lockfile can't be read, proceed with direct deps only except Exception: return [] # If can't parse apm.yml, assume no orphans @@ -194,7 +206,7 @@ def _check_orphaned_packages(): path_key = f"{level1_dir.name}/{level2_dir.name}/{level3_dir.name}" installed_packages.append(path_key) - # Find orphaned packages (installed but not declared) + # Find orphaned packages (installed but not declared or locked) orphaned_packages = [] for org_repo_name in installed_packages: if org_repo_name not in expected_installed: @@ -789,6 +801,14 @@ def prune(ctx, dry_run): ) elif len(repo_parts) >= 2: expected_installed.add(f"{repo_parts[0]}/{repo_parts[1]}") + + # Also include transitive dependencies from apm.lock + try: + from apm_cli.deps.lockfile import get_lockfile_installed_paths + lockfile_paths = get_lockfile_installed_paths(Path.cwd()) + expected_installed.update(lockfile_paths) + except Exception: + pass # If lockfile can't be read, proceed with direct deps only except Exception as e: _rich_error(f"Failed to parse apm.yml: {e}") sys.exit(1) diff --git a/src/apm_cli/deps/__init__.py b/src/apm_cli/deps/__init__.py index c57662c37..eda2f0988 100644 --- a/src/apm_cli/deps/__init__.py +++ b/src/apm_cli/deps/__init__.py @@ -9,7 +9,7 @@ from .verifier import verify_dependencies, install_missing_dependencies, load_apm_config from .github_downloader import GitHubPackageDownloader from .package_validator import PackageValidator -from .lockfile import LockFile, LockedDependency, get_lockfile_path +from .lockfile import LockFile, LockedDependency, get_lockfile_path, get_lockfile_installed_paths __all__ = [ 'sync_workflow_dependencies', @@ -29,4 +29,5 @@ 'LockFile', 'LockedDependency', 'get_lockfile_path', + 'get_lockfile_installed_paths', ] diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 05201e998..43b66a7b3 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -208,3 +208,44 @@ def save(self, path: Path) -> None: def get_lockfile_path(project_root: Path) -> Path: """Get the path to the lock file for a project.""" return project_root / "apm.lock" + + +def get_lockfile_installed_paths(project_root: Path) -> List[str]: + """Get installed paths for all dependencies recorded in apm.lock. + + Reads the lockfile and computes expected installed paths for all + dependencies, including transitive ones. Used by: + - Primitive discovery to find all dependency primitives + - Orphan detection to avoid false positives for transitive deps + + Args: + project_root: Path to project root containing apm.lock. + + Returns: + List[str]: Relative installed paths (e.g., ['owner/repo']), + ordered by depth then repo_url (no duplicates). + """ + lockfile_path = get_lockfile_path(project_root) + lockfile = LockFile.read(lockfile_path) + if not lockfile: + return [] + + apm_modules_dir = project_root / "apm_modules" + paths: List[str] = [] + seen: set = set() + for dep in lockfile.get_all_dependencies(): + dep_ref = DependencyReference( + repo_url=dep.repo_url, + host=dep.host, + virtual_path=dep.virtual_path, + is_virtual=dep.is_virtual, + ) + install_path = dep_ref.get_install_path(apm_modules_dir) + try: + rel_path = str(install_path.relative_to(apm_modules_dir)) + except ValueError: + rel_path = str(install_path) + if rel_path not in seen: + seen.add(rel_path) + paths.append(rel_path) + return paths diff --git a/src/apm_cli/primitives/discovery.py b/src/apm_cli/primitives/discovery.py index 77c10fa27..062743d21 100644 --- a/src/apm_cli/primitives/discovery.py +++ b/src/apm_cli/primitives/discovery.py @@ -183,7 +183,14 @@ def scan_dependency_primitives(base_dir: str, collection: PrimitiveCollection) - def get_dependency_declaration_order(base_dir: str) -> List[str]: - """Get APM dependency installed paths in their declaration order from apm.yml. + """Get APM dependency installed paths in their declaration order. + + Returns the actual installed paths for each dependency, combining: + 1. Direct dependencies from apm.yml (highest priority, declaration order) + 2. Transitive dependencies from apm.lock (appended after direct deps) + + This ensures transitive dependencies are included in primitive discovery + and compilation, not just direct dependencies. Returns the actual installed paths for each dependency, which differs for: - Regular packages: owner/repo (GitHub) or org/project/repo (ADO) @@ -243,6 +250,18 @@ def get_dependency_declaration_order(base_dir: str) -> List[str]: # This matches our org-namespaced directory structure dependency_names.append(dep.repo_url) + # Include transitive dependencies from apm.lock + # Direct deps from apm.yml have priority; transitive deps are appended + try: + from ..deps.lockfile import get_lockfile_installed_paths + lockfile_paths = get_lockfile_installed_paths(Path(base_dir)) + direct_set = set(dependency_names) + for path in lockfile_paths: + if path not in direct_set: + dependency_names.append(path) + except Exception: + pass # If lockfile can't be read, proceed with direct deps only + return dependency_names except Exception as e: diff --git a/tests/test_enhanced_discovery.py b/tests/test_enhanced_discovery.py index 3bf06f28e..bfe0c2807 100644 --- a/tests/test_enhanced_discovery.py +++ b/tests/test_enhanced_discovery.py @@ -412,6 +412,116 @@ def test_collection_methods(self): chatmode_conflicts = collection.get_conflicts_by_type("chatmode") self.assertEqual(len(chatmode_conflicts), 0) # No conflicts yet + def test_dependency_order_includes_transitive_from_lockfile(self): + """Test that transitive dependencies from apm.lock are included in declaration order.""" + from apm_cli.deps.lockfile import LockFile, LockedDependency + + # Create apm.yml with only one direct dependency + dependencies = {"apm": ["rieraj/team-cot-agent-instructions"]} + self._create_apm_yml(dependencies) + + # Create apm.lock with transitive dependencies + lockfile = LockFile() + lockfile.add_dependency(LockedDependency( + repo_url="rieraj/team-cot-agent-instructions", + depth=1, + )) + lockfile.add_dependency(LockedDependency( + repo_url="rieraj/division-ime-agent-instructions", + depth=2, + resolved_by="rieraj/team-cot-agent-instructions", + )) + lockfile.add_dependency(LockedDependency( + repo_url="rieraj/autodesk-agent-instructions", + depth=3, + resolved_by="rieraj/division-ime-agent-instructions", + )) + lockfile.write(self.temp_dir_path / "apm.lock") + + order = get_dependency_declaration_order(str(self.temp_dir_path)) + + # Direct dep should come first, then transitive deps from lockfile + self.assertIn("rieraj/team-cot-agent-instructions", order) + self.assertIn("rieraj/division-ime-agent-instructions", order) + self.assertIn("rieraj/autodesk-agent-instructions", order) + # Direct dep first + self.assertEqual(order[0], "rieraj/team-cot-agent-instructions") + self.assertEqual(len(order), 3) + + def test_dependency_order_no_lockfile(self): + """Test that dependency order works without a lockfile (backward compat).""" + # Create apm.yml with dependencies but no lockfile + dependencies = {"apm": ["company/standards", "team/workflows"]} + self._create_apm_yml(dependencies) + + order = get_dependency_declaration_order(str(self.temp_dir_path)) + self.assertEqual(order, ["company/standards", "team/workflows"]) + + def test_dependency_order_lockfile_no_duplicates(self): + """Test that direct deps already in apm.yml are not duplicated from lockfile.""" + from apm_cli.deps.lockfile import LockFile, LockedDependency + + # Create apm.yml with all deps listed directly + dependencies = {"apm": [ + "rieraj/team-cot", + "rieraj/division-ime", + "rieraj/autodesk", + ]} + self._create_apm_yml(dependencies) + + # Create apm.lock that also contains all deps + lockfile = LockFile() + lockfile.add_dependency(LockedDependency(repo_url="rieraj/team-cot", depth=1)) + lockfile.add_dependency(LockedDependency(repo_url="rieraj/division-ime", depth=1)) + lockfile.add_dependency(LockedDependency(repo_url="rieraj/autodesk", depth=1)) + lockfile.write(self.temp_dir_path / "apm.lock") + + order = get_dependency_declaration_order(str(self.temp_dir_path)) + # No duplicates + self.assertEqual(len(order), 3) + self.assertEqual(len(set(order)), 3) + + def test_scan_dependency_primitives_with_transitive(self): + """Test that scan_dependency_primitives finds transitive dep primitives.""" + from apm_cli.deps.lockfile import LockFile, LockedDependency + + # Create apm.yml with only one direct dependency + dependencies = {"apm": ["owner/direct-dep"]} + self._create_apm_yml(dependencies) + + # Create apm.lock with a transitive dependency + lockfile = LockFile() + lockfile.add_dependency(LockedDependency(repo_url="owner/direct-dep", depth=1)) + lockfile.add_dependency(LockedDependency( + repo_url="owner/transitive-dep", depth=2, + resolved_by="owner/direct-dep", + )) + lockfile.write(self.temp_dir_path / "apm.lock") + + # Create dependency directories with primitives + direct_dep_dir = self.temp_dir_path / "apm_modules" / "owner" / "direct-dep" / ".apm" / "instructions" + direct_dep_dir.mkdir(parents=True, exist_ok=True) + self._create_primitive_file( + direct_dep_dir / "direct.instructions.md", + "instruction", "direct" + ) + + transitive_dep_dir = self.temp_dir_path / "apm_modules" / "owner" / "transitive-dep" / ".apm" / "instructions" + transitive_dep_dir.mkdir(parents=True, exist_ok=True) + self._create_primitive_file( + transitive_dep_dir / "transitive.instructions.md", + "instruction", "transitive" + ) + + collection = PrimitiveCollection() + scan_dependency_primitives(str(self.temp_dir_path), collection) + + # Both direct and transitive deps should be found + self.assertEqual(len(collection.instructions), 2) + instruction_names = {i.name for i in collection.instructions} + self.assertIn("direct", instruction_names) + self.assertIn("transitive", instruction_names) + if __name__ == '__main__': unittest.main() \ No newline at end of file diff --git a/tests/unit/test_transitive_deps.py b/tests/unit/test_transitive_deps.py new file mode 100644 index 000000000..4ef806cc7 --- /dev/null +++ b/tests/unit/test_transitive_deps.py @@ -0,0 +1,223 @@ +"""Unit tests for transitive dependency handling. + +Tests that: +- get_lockfile_installed_paths() correctly returns paths for all locked deps +- _check_orphaned_packages() does not flag transitive deps as orphaned +- get_dependency_declaration_order() includes transitive deps from lockfile +""" + +import tempfile +from pathlib import Path + +import pytest +import yaml + +from apm_cli.deps.lockfile import ( + LockFile, + LockedDependency, + get_lockfile_installed_paths, +) +from apm_cli.primitives.discovery import get_dependency_declaration_order + + +class TestGetLockfileInstalledPaths: + """Tests for get_lockfile_installed_paths helper.""" + + def test_returns_empty_when_no_lockfile(self, tmp_path): + """Returns empty list when no apm.lock exists.""" + assert get_lockfile_installed_paths(tmp_path) == [] + + def test_returns_paths_for_regular_packages(self, tmp_path): + lockfile = LockFile() + lockfile.add_dependency(LockedDependency(repo_url="owner/repo-a", depth=1)) + lockfile.add_dependency(LockedDependency(repo_url="owner/repo-b", depth=2)) + lockfile.write(tmp_path / "apm.lock") + + paths = get_lockfile_installed_paths(tmp_path) + assert "owner/repo-a" in paths + assert "owner/repo-b" in paths + + def test_no_duplicates(self, tmp_path): + lockfile = LockFile() + lockfile.add_dependency(LockedDependency(repo_url="owner/repo", depth=1)) + lockfile.write(tmp_path / "apm.lock") + + paths = get_lockfile_installed_paths(tmp_path) + assert paths.count("owner/repo") == 1 + + def test_ordered_by_depth_then_repo(self, tmp_path): + lockfile = LockFile() + lockfile.add_dependency(LockedDependency(repo_url="z/deep", depth=3)) + lockfile.add_dependency(LockedDependency(repo_url="a/direct", depth=1)) + lockfile.add_dependency(LockedDependency(repo_url="m/mid", depth=2)) + lockfile.write(tmp_path / "apm.lock") + + paths = get_lockfile_installed_paths(tmp_path) + assert paths == ["a/direct", "m/mid", "z/deep"] + + def test_virtual_file_package_path(self, tmp_path): + """Virtual file packages should use the flattened virtual package name.""" + lockfile = LockFile() + lockfile.add_dependency(LockedDependency( + repo_url="owner/repo", + is_virtual=True, + virtual_path="prompts/code-review.prompt.md", + depth=1, + )) + lockfile.write(tmp_path / "apm.lock") + + paths = get_lockfile_installed_paths(tmp_path) + # Virtual file: owner/- → owner/repo-code-review + assert "owner/repo-code-review" in paths + + def test_corrupt_lockfile(self, tmp_path): + """Corrupt lockfile should return empty list.""" + (tmp_path / "apm.lock").write_text("not: valid: yaml: [") + assert get_lockfile_installed_paths(tmp_path) == [] + + +class TestTransitiveDependencyDiscovery: + """Test that transitive deps from lockfile appear in discovery order.""" + + def _write_apm_yml(self, path: Path, deps: list): + content = { + "name": "test-project", + "version": "1.0.0", + "description": "test", + "dependencies": {"apm": deps}, + } + (path / "apm.yml").write_text(yaml.dump(content)) + + def test_transitive_deps_appended_after_direct(self, tmp_path): + self._write_apm_yml(tmp_path, ["owner/direct"]) + + lockfile = LockFile() + lockfile.add_dependency(LockedDependency(repo_url="owner/direct", depth=1)) + lockfile.add_dependency(LockedDependency( + repo_url="owner/transitive", depth=2, resolved_by="owner/direct", + )) + lockfile.write(tmp_path / "apm.lock") + + order = get_dependency_declaration_order(str(tmp_path)) + assert order == ["owner/direct", "owner/transitive"] + + def test_direct_deps_not_duplicated(self, tmp_path): + self._write_apm_yml(tmp_path, ["owner/a", "owner/b"]) + + lockfile = LockFile() + lockfile.add_dependency(LockedDependency(repo_url="owner/a", depth=1)) + lockfile.add_dependency(LockedDependency(repo_url="owner/b", depth=1)) + lockfile.write(tmp_path / "apm.lock") + + order = get_dependency_declaration_order(str(tmp_path)) + assert order == ["owner/a", "owner/b"] + + def test_multiple_transitive_levels(self, tmp_path): + """Mirrors the exact scenario from the bug report.""" + self._write_apm_yml(tmp_path, ["rieraj/team-cot-agent-instructions"]) + + lockfile = LockFile() + lockfile.add_dependency(LockedDependency( + repo_url="rieraj/team-cot-agent-instructions", depth=1, + )) + lockfile.add_dependency(LockedDependency( + repo_url="rieraj/division-ime-agent-instructions", depth=2, + resolved_by="rieraj/team-cot-agent-instructions", + )) + lockfile.add_dependency(LockedDependency( + repo_url="rieraj/autodesk-agent-instructions", depth=3, + resolved_by="rieraj/division-ime-agent-instructions", + )) + lockfile.write(tmp_path / "apm.lock") + + order = get_dependency_declaration_order(str(tmp_path)) + assert len(order) == 3 + assert order[0] == "rieraj/team-cot-agent-instructions" + assert "rieraj/division-ime-agent-instructions" in order + assert "rieraj/autodesk-agent-instructions" in order + + def test_no_lockfile_falls_back_to_direct_only(self, tmp_path): + self._write_apm_yml(tmp_path, ["owner/only-direct"]) + # No lockfile created + + order = get_dependency_declaration_order(str(tmp_path)) + assert order == ["owner/only-direct"] + + +class TestOrphanDetectionWithTransitiveDeps: + """Test _check_orphaned_packages accounts for transitive deps.""" + + def _setup_project(self, tmp_path, direct_deps, lockfile_deps, installed_pkgs): + """Set up a project directory with apm.yml, apm.lock, and apm_modules.""" + # apm.yml + content = { + "name": "test-project", + "version": "1.0.0", + "description": "test", + "dependencies": {"apm": direct_deps}, + } + (tmp_path / "apm.yml").write_text(yaml.dump(content)) + + # apm.lock + if lockfile_deps: + lockfile = LockFile() + for dep in lockfile_deps: + lockfile.add_dependency(dep) + lockfile.write(tmp_path / "apm.lock") + + # apm_modules directories + for pkg in installed_pkgs: + pkg_dir = tmp_path / "apm_modules" / pkg + pkg_dir.mkdir(parents=True, exist_ok=True) + (pkg_dir / "apm.yml").write_text(f"name: {pkg.split('/')[-1]}\nversion: 1.0.0\n") + + def test_transitive_dep_not_flagged_as_orphan(self, tmp_path, monkeypatch): + """Transitive deps in apm.lock should NOT be flagged as orphaned.""" + self._setup_project( + tmp_path, + direct_deps=["rieraj/team-cot"], + lockfile_deps=[ + LockedDependency(repo_url="rieraj/team-cot", depth=1), + LockedDependency(repo_url="rieraj/division-ime", depth=2, resolved_by="rieraj/team-cot"), + LockedDependency(repo_url="rieraj/autodesk", depth=3, resolved_by="rieraj/division-ime"), + ], + installed_pkgs=["rieraj/team-cot", "rieraj/division-ime", "rieraj/autodesk"], + ) + + monkeypatch.chdir(tmp_path) + + from apm_cli.cli import _check_orphaned_packages + orphans = _check_orphaned_packages() + assert orphans == [], f"Transitive deps should not be orphaned, got: {orphans}" + + def test_truly_orphaned_package_still_detected(self, tmp_path, monkeypatch): + """Packages not in apm.yml or apm.lock should still be flagged.""" + self._setup_project( + tmp_path, + direct_deps=["owner/kept"], + lockfile_deps=[ + LockedDependency(repo_url="owner/kept", depth=1), + ], + installed_pkgs=["owner/kept", "owner/stale"], + ) + + monkeypatch.chdir(tmp_path) + + from apm_cli.cli import _check_orphaned_packages + orphans = _check_orphaned_packages() + assert "owner/stale" in orphans + + def test_no_lockfile_still_works(self, tmp_path, monkeypatch): + """Without a lockfile, orphan detection should still work (direct deps only).""" + self._setup_project( + tmp_path, + direct_deps=["owner/kept"], + lockfile_deps=None, + installed_pkgs=["owner/kept", "owner/stale"], + ) + + monkeypatch.chdir(tmp_path) + + from apm_cli.cli import _check_orphaned_packages + orphans = _check_orphaned_packages() + assert "owner/stale" in orphans From 968021b860d9f32418f4b9bfd48038e6f6727a63 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Feb 2026 23:24:27 +0000 Subject: [PATCH 3/8] Improve error handling in lockfile path resolution - Wrap get_lockfile_installed_paths() in try/except to handle all errors - Use ImportError instead of broad Exception in callers - Update docs for transitive dep discovery Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> --- docs/enhanced-primitive-discovery.md | 4 +-- src/apm_cli/cli.py | 8 ++--- src/apm_cli/deps/lockfile.py | 51 +++++++++++++++------------- src/apm_cli/primitives/discovery.py | 4 +-- 4 files changed, 36 insertions(+), 31 deletions(-) diff --git a/docs/enhanced-primitive-discovery.md b/docs/enhanced-primitive-discovery.md index d7948ed5c..cea1f0631 100644 --- a/docs/enhanced-primitive-discovery.md +++ b/docs/enhanced-primitive-discovery.md @@ -116,7 +116,7 @@ if collection.has_conflicts(): ### Dependency Declaration Order -The system reads `apm.yml` to determine the order in which dependencies should be processed: +The system reads `apm.yml` to determine the order in which direct dependencies should be processed. Transitive dependencies (resolved automatically via dependency chains) are read from `apm.lock` and appended after direct dependencies: ```yaml # apm.yml @@ -129,7 +129,7 @@ dependencies: - user/utilities ``` -Dependencies are processed in this exact order. If multiple dependencies provide primitives with the same name, the first one declared wins. +Direct dependencies are processed first, in declaration order. Transitive dependencies from `apm.lock` are appended after. If multiple dependencies provide primitives with the same name, the first one declared wins. ## Directory Structure diff --git a/src/apm_cli/cli.py b/src/apm_cli/cli.py index 032f6e48f..ca9c09eb7 100644 --- a/src/apm_cli/cli.py +++ b/src/apm_cli/cli.py @@ -174,8 +174,8 @@ def _check_orphaned_packages(): from apm_cli.deps.lockfile import get_lockfile_installed_paths lockfile_paths = get_lockfile_installed_paths(Path.cwd()) expected_installed.update(lockfile_paths) - except Exception: - pass # If lockfile can't be read, proceed with direct deps only + except ImportError: + pass # Lockfile module not available except Exception: return [] # If can't parse apm.yml, assume no orphans @@ -807,8 +807,8 @@ def prune(ctx, dry_run): from apm_cli.deps.lockfile import get_lockfile_installed_paths lockfile_paths = get_lockfile_installed_paths(Path.cwd()) expected_installed.update(lockfile_paths) - except Exception: - pass # If lockfile can't be read, proceed with direct deps only + except ImportError: + pass # Lockfile module not available except Exception as e: _rich_error(f"Failed to parse apm.yml: {e}") sys.exit(1) diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 43b66a7b3..ca826d202 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -218,6 +218,8 @@ def get_lockfile_installed_paths(project_root: Path) -> List[str]: - Primitive discovery to find all dependency primitives - Orphan detection to avoid false positives for transitive deps + Returns an empty list if the lockfile is missing, corrupt, or unreadable. + Args: project_root: Path to project root containing apm.lock. @@ -225,27 +227,30 @@ def get_lockfile_installed_paths(project_root: Path) -> List[str]: List[str]: Relative installed paths (e.g., ['owner/repo']), ordered by depth then repo_url (no duplicates). """ - lockfile_path = get_lockfile_path(project_root) - lockfile = LockFile.read(lockfile_path) - if not lockfile: + try: + lockfile_path = get_lockfile_path(project_root) + lockfile = LockFile.read(lockfile_path) + if not lockfile: + return [] + + apm_modules_dir = project_root / "apm_modules" + paths: List[str] = [] + seen: set = set() + for dep in lockfile.get_all_dependencies(): + dep_ref = DependencyReference( + repo_url=dep.repo_url, + host=dep.host, + virtual_path=dep.virtual_path, + is_virtual=dep.is_virtual, + ) + install_path = dep_ref.get_install_path(apm_modules_dir) + try: + rel_path = str(install_path.relative_to(apm_modules_dir)) + except ValueError: + rel_path = str(install_path) + if rel_path not in seen: + seen.add(rel_path) + paths.append(rel_path) + return paths + except Exception: return [] - - apm_modules_dir = project_root / "apm_modules" - paths: List[str] = [] - seen: set = set() - for dep in lockfile.get_all_dependencies(): - dep_ref = DependencyReference( - repo_url=dep.repo_url, - host=dep.host, - virtual_path=dep.virtual_path, - is_virtual=dep.is_virtual, - ) - install_path = dep_ref.get_install_path(apm_modules_dir) - try: - rel_path = str(install_path.relative_to(apm_modules_dir)) - except ValueError: - rel_path = str(install_path) - if rel_path not in seen: - seen.add(rel_path) - paths.append(rel_path) - return paths diff --git a/src/apm_cli/primitives/discovery.py b/src/apm_cli/primitives/discovery.py index 062743d21..181d27663 100644 --- a/src/apm_cli/primitives/discovery.py +++ b/src/apm_cli/primitives/discovery.py @@ -259,8 +259,8 @@ def get_dependency_declaration_order(base_dir: str) -> List[str]: for path in lockfile_paths: if path not in direct_set: dependency_names.append(path) - except Exception: - pass # If lockfile can't be read, proceed with direct deps only + except ImportError: + pass # Lockfile module not available return dependency_names From 2ac2a507cfd482e83371b573e7c78304247a5ca0 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel <51440732+danielmeppiel@users.noreply.github.com> Date: Thu, 26 Feb 2026 00:58:17 +0100 Subject: [PATCH 4/8] Update src/apm_cli/primitives/discovery.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/apm_cli/primitives/discovery.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/apm_cli/primitives/discovery.py b/src/apm_cli/primitives/discovery.py index 181d27663..678e499f5 100644 --- a/src/apm_cli/primitives/discovery.py +++ b/src/apm_cli/primitives/discovery.py @@ -185,14 +185,13 @@ def scan_dependency_primitives(base_dir: str, collection: PrimitiveCollection) - def get_dependency_declaration_order(base_dir: str) -> List[str]: """Get APM dependency installed paths in their declaration order. - Returns the actual installed paths for each dependency, combining: + The returned list contains the actual installed path for each dependency, + combining: 1. Direct dependencies from apm.yml (highest priority, declaration order) 2. Transitive dependencies from apm.lock (appended after direct deps) This ensures transitive dependencies are included in primitive discovery - and compilation, not just direct dependencies. - - Returns the actual installed paths for each dependency, which differs for: + and compilation, not just direct dependencies. The installed path differs for: - Regular packages: owner/repo (GitHub) or org/project/repo (ADO) - Virtual packages: owner/virtual-pkg-name (GitHub) or org/project/virtual-pkg-name (ADO) From f3ce99b0d4b4f8f01cb98e06985a72059ab54749 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 26 Feb 2026 18:26:33 +0100 Subject: [PATCH 5/8] refactor: address PR review comments for transitive deps fix - Move get_lockfile_installed_paths logic into LockFile.get_installed_paths() method (foutoucour review) - Use .as_posix() for Windows-safe path normalization (copilot-reviewer) - Narrow except Exception to specific error types (copilot-reviewer) - Move inline imports to top-level in discovery.py and cli.py (foutoucour) - Remove unused imports in test file (tempfile, pytest) --- src/apm_cli/cli.py | 19 +++------ src/apm_cli/deps/lockfile.py | 66 +++++++++++++++++------------ src/apm_cli/primitives/discovery.py | 15 +++---- tests/unit/test_transitive_deps.py | 2 - 4 files changed, 50 insertions(+), 52 deletions(-) diff --git a/src/apm_cli/cli.py b/src/apm_cli/cli.py index ca9c09eb7..75cc06e7d 100644 --- a/src/apm_cli/cli.py +++ b/src/apm_cli/cli.py @@ -41,6 +41,7 @@ try: from apm_cli.deps.apm_resolver import APMDependencyResolver from apm_cli.deps.github_downloader import GitHubPackageDownloader + from apm_cli.deps.lockfile import get_lockfile_installed_paths from apm_cli.models.apm_package import APMPackage, DependencyReference from apm_cli.integration import PromptIntegrator, AgentIntegrator @@ -140,8 +141,6 @@ def _check_orphaned_packages(): List[str]: List of orphaned package names in org/repo or org/project/repo format """ try: - from pathlib import Path - # Check if apm.yml exists if not Path("apm.yml").exists(): return [] @@ -170,12 +169,8 @@ def _check_orphaned_packages(): expected_installed.add(str(install_path)) # Also include transitive dependencies from apm.lock - try: - from apm_cli.deps.lockfile import get_lockfile_installed_paths - lockfile_paths = get_lockfile_installed_paths(Path.cwd()) - expected_installed.update(lockfile_paths) - except ImportError: - pass # Lockfile module not available + lockfile_paths = get_lockfile_installed_paths(Path.cwd()) + expected_installed.update(lockfile_paths) except Exception: return [] # If can't parse apm.yml, assume no orphans @@ -803,12 +798,8 @@ def prune(ctx, dry_run): expected_installed.add(f"{repo_parts[0]}/{repo_parts[1]}") # Also include transitive dependencies from apm.lock - try: - from apm_cli.deps.lockfile import get_lockfile_installed_paths - lockfile_paths = get_lockfile_installed_paths(Path.cwd()) - expected_installed.update(lockfile_paths) - except ImportError: - pass # Lockfile module not available + lockfile_paths = get_lockfile_installed_paths(Path.cwd()) + expected_installed.update(lockfile_paths) except Exception as e: _rich_error(f"Failed to parse apm.yml: {e}") sys.exit(1) diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index ca826d202..4aa9a9cb8 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -200,6 +200,40 @@ def from_installed_packages( return lock + def get_installed_paths(self, apm_modules_dir: Path) -> List[str]: + """Get relative installed paths for all dependencies in this lockfile. + + Computes expected installed paths for all dependencies, including + transitive ones. Used by: + - Primitive discovery to find all dependency primitives + - Orphan detection to avoid false positives for transitive deps + + Args: + apm_modules_dir: Path to the apm_modules directory. + + Returns: + List[str]: POSIX-style relative installed paths (e.g., ['owner/repo']), + ordered by depth then repo_url (no duplicates). + """ + seen: set = set() + paths: List[str] = [] + for dep in self.get_all_dependencies(): + dep_ref = DependencyReference( + repo_url=dep.repo_url, + host=dep.host, + virtual_path=dep.virtual_path, + is_virtual=dep.is_virtual, + ) + install_path = dep_ref.get_install_path(apm_modules_dir) + try: + rel_path = install_path.relative_to(apm_modules_dir).as_posix() + except ValueError: + rel_path = Path(install_path).as_posix() + if rel_path not in seen: + seen.add(rel_path) + paths.append(rel_path) + return paths + def save(self, path: Path) -> None: """Save lock file to disk (alias for write).""" self.write(path) @@ -213,12 +247,9 @@ def get_lockfile_path(project_root: Path) -> Path: def get_lockfile_installed_paths(project_root: Path) -> List[str]: """Get installed paths for all dependencies recorded in apm.lock. - Reads the lockfile and computes expected installed paths for all - dependencies, including transitive ones. Used by: - - Primitive discovery to find all dependency primitives - - Orphan detection to avoid false positives for transitive deps - - Returns an empty list if the lockfile is missing, corrupt, or unreadable. + Convenience wrapper that loads the lockfile and delegates to + LockFile.get_installed_paths(). Returns an empty list if the + lockfile is missing, corrupt, or unreadable. Args: project_root: Path to project root containing apm.lock. @@ -232,25 +263,6 @@ def get_lockfile_installed_paths(project_root: Path) -> List[str]: lockfile = LockFile.read(lockfile_path) if not lockfile: return [] - - apm_modules_dir = project_root / "apm_modules" - paths: List[str] = [] - seen: set = set() - for dep in lockfile.get_all_dependencies(): - dep_ref = DependencyReference( - repo_url=dep.repo_url, - host=dep.host, - virtual_path=dep.virtual_path, - is_virtual=dep.is_virtual, - ) - install_path = dep_ref.get_install_path(apm_modules_dir) - try: - rel_path = str(install_path.relative_to(apm_modules_dir)) - except ValueError: - rel_path = str(install_path) - if rel_path not in seen: - seen.add(rel_path) - paths.append(rel_path) - return paths - except Exception: + return lockfile.get_installed_paths(project_root / "apm_modules") + except (FileNotFoundError, yaml.YAMLError, ValueError, KeyError): return [] diff --git a/src/apm_cli/primitives/discovery.py b/src/apm_cli/primitives/discovery.py index 678e499f5..9fc272a65 100644 --- a/src/apm_cli/primitives/discovery.py +++ b/src/apm_cli/primitives/discovery.py @@ -8,6 +8,7 @@ from .models import PrimitiveCollection from .parser import parse_primitive_file, parse_skill_file from ..models.apm_package import APMPackage +from ..deps.lockfile import get_lockfile_installed_paths # Common primitive patterns for local discovery (with recursive search) @@ -251,15 +252,11 @@ def get_dependency_declaration_order(base_dir: str) -> List[str]: # Include transitive dependencies from apm.lock # Direct deps from apm.yml have priority; transitive deps are appended - try: - from ..deps.lockfile import get_lockfile_installed_paths - lockfile_paths = get_lockfile_installed_paths(Path(base_dir)) - direct_set = set(dependency_names) - for path in lockfile_paths: - if path not in direct_set: - dependency_names.append(path) - except ImportError: - pass # Lockfile module not available + lockfile_paths = get_lockfile_installed_paths(Path(base_dir)) + direct_set = set(dependency_names) + for path in lockfile_paths: + if path not in direct_set: + dependency_names.append(path) return dependency_names diff --git a/tests/unit/test_transitive_deps.py b/tests/unit/test_transitive_deps.py index 4ef806cc7..ab6f33c28 100644 --- a/tests/unit/test_transitive_deps.py +++ b/tests/unit/test_transitive_deps.py @@ -6,10 +6,8 @@ - get_dependency_declaration_order() includes transitive deps from lockfile """ -import tempfile from pathlib import Path -import pytest import yaml from apm_cli.deps.lockfile import ( From 48d87c05c9824689b38cdcce129cf063108a3a26 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 26 Feb 2026 18:32:11 +0100 Subject: [PATCH 6/8] fix: set minimum console width for deps list to prevent truncation in CI --- src/apm_cli/commands/deps.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/apm_cli/commands/deps.py b/src/apm_cli/commands/deps.py index a75f33b3d..8968c4ae0 100644 --- a/src/apm_cli/commands/deps.py +++ b/src/apm_cli/commands/deps.py @@ -29,7 +29,9 @@ def list_packages(): # Import Rich components with fallback from rich.table import Table from rich.console import Console - console = Console() + import shutil + term_width = shutil.get_terminal_size((120, 24)).columns + console = Console(width=max(120, term_width)) has_rich = True except ImportError: has_rich = False From bd8794cb040e3e0f0f8c24af193eac915b7627c2 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 26 Feb 2026 19:31:47 +0100 Subject: [PATCH 7/8] refactor: move get_lockfile_installed_paths into LockFile classmethod --- src/apm_cli/cli.py | 6 ++-- src/apm_cli/deps/__init__.py | 4 +-- src/apm_cli/deps/lockfile.py | 45 +++++++++++++++-------------- src/apm_cli/primitives/discovery.py | 4 +-- tests/unit/test_transitive_deps.py | 15 +++++----- 5 files changed, 38 insertions(+), 36 deletions(-) diff --git a/src/apm_cli/cli.py b/src/apm_cli/cli.py index 05c35c3ac..efc3b33d4 100644 --- a/src/apm_cli/cli.py +++ b/src/apm_cli/cli.py @@ -41,7 +41,7 @@ try: from apm_cli.deps.apm_resolver import APMDependencyResolver from apm_cli.deps.github_downloader import GitHubPackageDownloader - from apm_cli.deps.lockfile import get_lockfile_installed_paths + from apm_cli.deps.lockfile import LockFile from apm_cli.models.apm_package import APMPackage, DependencyReference from apm_cli.integration import PromptIntegrator, AgentIntegrator @@ -169,7 +169,7 @@ def _check_orphaned_packages(): expected_installed.add(str(install_path)) # Also include transitive dependencies from apm.lock - lockfile_paths = get_lockfile_installed_paths(Path.cwd()) + lockfile_paths = LockFile.installed_paths_for_project(Path.cwd()) expected_installed.update(lockfile_paths) except Exception: return [] # If can't parse apm.yml, assume no orphans @@ -798,7 +798,7 @@ def prune(ctx, dry_run): expected_installed.add(f"{repo_parts[0]}/{repo_parts[1]}") # Also include transitive dependencies from apm.lock - lockfile_paths = get_lockfile_installed_paths(Path.cwd()) + lockfile_paths = LockFile.installed_paths_for_project(Path.cwd()) expected_installed.update(lockfile_paths) except Exception as e: _rich_error(f"Failed to parse apm.yml: {e}") diff --git a/src/apm_cli/deps/__init__.py b/src/apm_cli/deps/__init__.py index eda2f0988..d22856e7b 100644 --- a/src/apm_cli/deps/__init__.py +++ b/src/apm_cli/deps/__init__.py @@ -9,7 +9,7 @@ from .verifier import verify_dependencies, install_missing_dependencies, load_apm_config from .github_downloader import GitHubPackageDownloader from .package_validator import PackageValidator -from .lockfile import LockFile, LockedDependency, get_lockfile_path, get_lockfile_installed_paths +from .lockfile import LockFile, LockedDependency, get_lockfile_path __all__ = [ 'sync_workflow_dependencies', @@ -29,5 +29,5 @@ 'LockFile', 'LockedDependency', 'get_lockfile_path', - 'get_lockfile_installed_paths', + ] diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 4aa9a9cb8..6ee155f3c 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -238,6 +238,28 @@ def save(self, path: Path) -> None: """Save lock file to disk (alias for write).""" self.write(path) + @classmethod + def installed_paths_for_project(cls, project_root: Path) -> List[str]: + """Load apm.lock from project_root and return installed paths. + + Returns an empty list if the lockfile is missing, corrupt, or + unreadable. + + Args: + project_root: Path to project root containing apm.lock. + + Returns: + List[str]: Relative installed paths (e.g., ['owner/repo']), + ordered by depth then repo_url (no duplicates). + """ + try: + lockfile = cls.read(project_root / "apm.lock") + if not lockfile: + return [] + return lockfile.get_installed_paths(project_root / "apm_modules") + except (FileNotFoundError, yaml.YAMLError, ValueError, KeyError): + return [] + def get_lockfile_path(project_root: Path) -> Path: """Get the path to the lock file for a project.""" @@ -245,24 +267,5 @@ def get_lockfile_path(project_root: Path) -> Path: def get_lockfile_installed_paths(project_root: Path) -> List[str]: - """Get installed paths for all dependencies recorded in apm.lock. - - Convenience wrapper that loads the lockfile and delegates to - LockFile.get_installed_paths(). Returns an empty list if the - lockfile is missing, corrupt, or unreadable. - - Args: - project_root: Path to project root containing apm.lock. - - Returns: - List[str]: Relative installed paths (e.g., ['owner/repo']), - ordered by depth then repo_url (no duplicates). - """ - try: - lockfile_path = get_lockfile_path(project_root) - lockfile = LockFile.read(lockfile_path) - if not lockfile: - return [] - return lockfile.get_installed_paths(project_root / "apm_modules") - except (FileNotFoundError, yaml.YAMLError, ValueError, KeyError): - return [] + """Deprecated: use LockFile.installed_paths_for_project() instead.""" + return LockFile.installed_paths_for_project(project_root) diff --git a/src/apm_cli/primitives/discovery.py b/src/apm_cli/primitives/discovery.py index 9fc272a65..45ccd8cfe 100644 --- a/src/apm_cli/primitives/discovery.py +++ b/src/apm_cli/primitives/discovery.py @@ -8,7 +8,7 @@ from .models import PrimitiveCollection from .parser import parse_primitive_file, parse_skill_file from ..models.apm_package import APMPackage -from ..deps.lockfile import get_lockfile_installed_paths +from ..deps.lockfile import LockFile # Common primitive patterns for local discovery (with recursive search) @@ -252,7 +252,7 @@ def get_dependency_declaration_order(base_dir: str) -> List[str]: # Include transitive dependencies from apm.lock # Direct deps from apm.yml have priority; transitive deps are appended - lockfile_paths = get_lockfile_installed_paths(Path(base_dir)) + lockfile_paths = LockFile.installed_paths_for_project(Path(base_dir)) direct_set = set(dependency_names) for path in lockfile_paths: if path not in direct_set: diff --git a/tests/unit/test_transitive_deps.py b/tests/unit/test_transitive_deps.py index ab6f33c28..88397cff5 100644 --- a/tests/unit/test_transitive_deps.py +++ b/tests/unit/test_transitive_deps.py @@ -1,7 +1,7 @@ """Unit tests for transitive dependency handling. Tests that: -- get_lockfile_installed_paths() correctly returns paths for all locked deps +- LockFile.installed_paths_for_project() correctly returns paths for all locked deps - _check_orphaned_packages() does not flag transitive deps as orphaned - get_dependency_declaration_order() includes transitive deps from lockfile """ @@ -13,7 +13,6 @@ from apm_cli.deps.lockfile import ( LockFile, LockedDependency, - get_lockfile_installed_paths, ) from apm_cli.primitives.discovery import get_dependency_declaration_order @@ -23,7 +22,7 @@ class TestGetLockfileInstalledPaths: def test_returns_empty_when_no_lockfile(self, tmp_path): """Returns empty list when no apm.lock exists.""" - assert get_lockfile_installed_paths(tmp_path) == [] + assert LockFile.installed_paths_for_project(tmp_path) == [] def test_returns_paths_for_regular_packages(self, tmp_path): lockfile = LockFile() @@ -31,7 +30,7 @@ def test_returns_paths_for_regular_packages(self, tmp_path): lockfile.add_dependency(LockedDependency(repo_url="owner/repo-b", depth=2)) lockfile.write(tmp_path / "apm.lock") - paths = get_lockfile_installed_paths(tmp_path) + paths = LockFile.installed_paths_for_project(tmp_path) assert "owner/repo-a" in paths assert "owner/repo-b" in paths @@ -40,7 +39,7 @@ def test_no_duplicates(self, tmp_path): lockfile.add_dependency(LockedDependency(repo_url="owner/repo", depth=1)) lockfile.write(tmp_path / "apm.lock") - paths = get_lockfile_installed_paths(tmp_path) + paths = LockFile.installed_paths_for_project(tmp_path) assert paths.count("owner/repo") == 1 def test_ordered_by_depth_then_repo(self, tmp_path): @@ -50,7 +49,7 @@ def test_ordered_by_depth_then_repo(self, tmp_path): lockfile.add_dependency(LockedDependency(repo_url="m/mid", depth=2)) lockfile.write(tmp_path / "apm.lock") - paths = get_lockfile_installed_paths(tmp_path) + paths = LockFile.installed_paths_for_project(tmp_path) assert paths == ["a/direct", "m/mid", "z/deep"] def test_virtual_file_package_path(self, tmp_path): @@ -64,14 +63,14 @@ def test_virtual_file_package_path(self, tmp_path): )) lockfile.write(tmp_path / "apm.lock") - paths = get_lockfile_installed_paths(tmp_path) + paths = LockFile.installed_paths_for_project(tmp_path) # Virtual file: owner/- → owner/repo-code-review assert "owner/repo-code-review" in paths def test_corrupt_lockfile(self, tmp_path): """Corrupt lockfile should return empty list.""" (tmp_path / "apm.lock").write_text("not: valid: yaml: [") - assert get_lockfile_installed_paths(tmp_path) == [] + assert LockFile.installed_paths_for_project(tmp_path) == [] class TestTransitiveDependencyDiscovery: From 640ebbb6dcd210851cd201bac7b531c3fdacd333 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 26 Feb 2026 20:38:17 +0100 Subject: [PATCH 8/8] fix: update skill detection test to match current CLI output --- tests/integration/test_skill_install.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_skill_install.py b/tests/integration/test_skill_install.py index e963156d7..99b7c042a 100644 --- a/tests/integration/test_skill_install.py +++ b/tests/integration/test_skill_install.py @@ -105,7 +105,7 @@ def test_install_skill_updates_apm_yml(self, temp_project, apm_command): assert "anthropics/skills/skills/brand-guidelines" in content def test_skill_detection_in_output(self, temp_project, apm_command): - """Verify CLI output shows 'Claude Skill (SKILL.md detected)'.""" + """Verify CLI output shows skill integration message.""" result = subprocess.run( [apm_command, "install", "anthropics/skills/skills/brand-guidelines", "--verbose"], cwd=temp_project, @@ -114,8 +114,8 @@ def test_skill_detection_in_output(self, temp_project, apm_command): timeout=120 ) - # Check for skill detection message - assert "Claude Skill" in result.stdout or "SKILL.md detected" in result.stdout + # Check for skill detection/integration message + assert "Skill integrated" in result.stdout or "Claude Skill" in result.stdout or "SKILL.md detected" in result.stdout class TestClaudeSkillWithResources: