diff --git a/docs/enhanced-primitive-discovery.md b/docs/enhanced-primitive-discovery.md index d7948ed5..cea1f063 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 ea8dbb5c..efc3b33d 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 LockFile from apm_cli.models.apm_package import APMPackage, DependencyReference from apm_cli.integration import PromptIntegrator, AgentIntegrator @@ -130,14 +131,16 @@ 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 """ try: - from pathlib import Path - # Check if apm.yml exists if not Path("apm.yml").exists(): return [] @@ -164,6 +167,10 @@ 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 + 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 @@ -194,7 +201,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 +796,10 @@ 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 + 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}") sys.exit(1) diff --git a/src/apm_cli/commands/deps.py b/src/apm_cli/commands/deps.py index a75f33b3..8968c4ae 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 diff --git a/src/apm_cli/deps/__init__.py b/src/apm_cli/deps/__init__.py index c57662c3..d22856e7 100644 --- a/src/apm_cli/deps/__init__.py +++ b/src/apm_cli/deps/__init__.py @@ -29,4 +29,5 @@ 'LockFile', 'LockedDependency', 'get_lockfile_path', + ] diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 05201e99..6ee155f3 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -200,11 +200,72 @@ 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) + @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.""" return project_root / "apm.lock" + + +def get_lockfile_installed_paths(project_root: Path) -> List[str]: + """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 77c10fa2..45ccd8cf 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 LockFile # Common primitive patterns for local discovery (with recursive search) @@ -183,9 +184,15 @@ 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, which differs for: + 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. 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) @@ -243,6 +250,14 @@ 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 + 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: + dependency_names.append(path) + return dependency_names except Exception as e: diff --git a/tests/integration/test_skill_install.py b/tests/integration/test_skill_install.py index e963156d..99b7c042 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: diff --git a/tests/test_enhanced_discovery.py b/tests/test_enhanced_discovery.py index 3bf06f28..bfe0c280 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 00000000..88397cff --- /dev/null +++ b/tests/unit/test_transitive_deps.py @@ -0,0 +1,220 @@ +"""Unit tests for transitive dependency handling. + +Tests that: +- 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 +""" + +from pathlib import Path + +import yaml + +from apm_cli.deps.lockfile import ( + LockFile, + LockedDependency, +) +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 LockFile.installed_paths_for_project(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 = LockFile.installed_paths_for_project(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 = LockFile.installed_paths_for_project(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 = LockFile.installed_paths_for_project(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 = 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 LockFile.installed_paths_for_project(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