diff --git a/CHANGELOG.md b/CHANGELOG.md index b6567e2be..e23450c15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,6 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - ## [Unreleased] ### Changed @@ -22,6 +21,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Plugin manifest schema-conformance tests.** `tests/unit/test_plugin_exporter_schema.py` validates every shape of `plugin.json` produced by `apm pack` (synthesized, authored, and authored-with-stale-keys) against the vendored official schema. Companion marketplace conformance lives in `tests/unit/marketplace/test_schema_conformance.py`. (#1061) - Slash commands installed from APM packages now surface argument hints in Claude Code -- `apm install` automatically maps prompt `input:` to Claude's `arguments:` front-matter, rewrites `${input:name}` references to `$name`, and auto-generates `argument-hint`. Argument names are validated against an allowlist to prevent YAML injection from third-party packages, and the mapping is reported at install time. (#1039) +### Fixed + +- **`apm prune` no longer flags directories it put there itself** -- skills installed from a subdirectory path (e.g., `owner/repo/.apm/skills/skill-name`) no longer cause the parent `owner/repo/` clone to appear as an orphan. Fixes spurious removal prompts in multi-skill and monorepo-style setups. The same fix applies to `apm deps list` and `apm compile`. A genuinely orphaned `owner/repo` package is still flagged even when a sibling subdirectory dep shares the same `owner/repo` root. No changes to `apm.yml` or the lockfile are required to benefit from this fix. (#1050) + ## [0.11.0] - 2026-04-29 ### Added diff --git a/src/apm_cli/commands/_helpers.py b/src/apm_cli/commands/_helpers.py index 4cd8b4f9c..9a441a400 100644 --- a/src/apm_cli/commands/_helpers.py +++ b/src/apm_cli/commands/_helpers.py @@ -6,6 +6,7 @@ import builtins import os import sys +from collections.abc import Iterable from pathlib import Path import click @@ -23,6 +24,7 @@ from ..update_policy import get_update_hint_message, is_self_update_enabled from ..utils.atomic_io import atomic_write_text as _atomic_write # noqa: F401 from ..utils.console import _rich_echo, _rich_info, _rich_warning +from ..utils.path_security import PathTraversalError, validate_path_segments from ..utils.version_checker import check_for_updates from ..version import get_build_sha, get_version @@ -135,7 +137,7 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path (depth > 1 from ``apm.lock``), using ``get_install_path()`` for consistency with how packages are actually installed. """ - expected = builtins.set() + expected = set() for dep in declared_deps: install_path = dep.get_install_path(apm_modules_dir) try: @@ -157,6 +159,85 @@ def _build_expected_install_paths(declared_deps, lockfile, apm_modules_dir: Path return expected +def _expand_with_ancestors( + paths: Iterable[str], installed: Iterable[str] | None = None +) -> set[str]: + """Expand a set of expected paths to include ancestor prefixes. + + Given ``{"owner/repo/.apm/skills/my-skill"}``, returns a set containing + the original path plus all intermediate path prefixes with 2+ segments + (e.g., ``"owner/repo"``, ``"owner/repo/.apm"``, + ``"owner/repo/.apm/skills"``, plus the original + ``"owner/repo/.apm/skills/my-skill"``). + This allows O(1) membership checks when determining whether a scanned + directory is an ancestor of an expected package path. + + Ancestor expansion exists because a subdirectory dependency + (``git: owner/repo, path: .apm/skills/x``) is installed by cloning the + entire repo to ``apm_modules/owner/repo/``. Intermediate filesystem + directories created by that clone are required parts of the install -- + not stale leftovers. + + Real-orphan safety: when *installed* is supplied, an ancestor that + matches one of the installed paths is NOT added to the expansion + unless that path is also directly declared in *paths*. Callers should + pass only the subset of installed paths that look like *real + standalone packages* (i.e., directories that ship their own + ``apm.yml``) -- not filesystem intermediaries (which typically have + only a ``.apm/`` subtree from a cloned subdir dep). This preserves + orphan detection for the case where a user has a genuinely orphaned + ``owner/repo`` package on disk alongside a declared sibling + subdirectory dep (``owner/repo/.apm/skills/foo``): only filesystem + intermediaries are suppressed, never real installed packages. + + Security contract -- ancestor depth cap: ``get_install_path()`` + anchors installs at the 2-segment repo root (GitHub) or 3-segment + root (ADO). Anything deeper is a filesystem-intermediary path + (``.apm/``, ``skills/``, ...) that ``_scan_installed_packages`` + skips, so emitting ancestors past depth 3 would only widen the + orphan-suppression surface without serving any real lookup. The + loop is therefore capped at depth 3 (``min(4, len(parts))``), which + bounds the number of paths an attacker-influenced ``apm.yml`` dep + declaration can hide from orphan detection. If the install strategy + ever grows deeper roots, lift this cap and document the new + invariant here. + + Traversal guard: any input path that fails + :func:`apm_cli.utils.path_security.validate_path_segments` (which + rejects both ``.`` and ``..`` segments after backslash + normalisation) is kept in the result as-is (membership check) but + produces no ancestors. Routing through the canonical guard -- + rather than a hand-rolled ``".." in parts`` check -- ensures + single-dot segments (``owner/./repo``) are also caught and keeps + the project's path-validation contract centralised. + """ + materialized = list(paths) + materialized_set = set(materialized) + expanded = set(materialized) + installed_set = set(installed) if installed is not None else set() + for p in materialized: + try: + validate_path_segments(p, context="ancestor expansion") + except PathTraversalError: + continue + # Normalise backslashes so Windows-style tokens split into the + # same parts as POSIX inputs for the depth-capped loop below. + normalised = p.replace("\\", "/") + parts = normalised.split("/") + # Cap at depth 3 -- the ADO install-root depth -- to bound the + # ancestor-suppression surface (see security contract above). + for i in range(2, min(4, len(parts))): + ancestor = "/".join(parts[:i]) + # Do not mask a real installed package via ancestor expansion; + # only filesystem intermediaries should be added. A real + # installed package that is also directly declared remains in + # expanded via materialized_set. + if ancestor in installed_set and ancestor not in materialized_set: + continue + expanded.add(ancestor) + return expanded + + def _scan_installed_packages(apm_modules_dir: Path) -> list: """Scan *apm_modules_dir* for installed package paths. @@ -180,6 +261,52 @@ def _scan_installed_packages(apm_modules_dir: Path) -> list: return installed +def _standalone_installed_packages( + installed: Iterable[str], apm_modules_dir: Path, lockfile=None +) -> list: + """Filter *installed* to entries that look like real standalone packages. + + Determination order (tamper-evident first): + + 1. Path appears as a dependency key in *lockfile* -- the canonical + record of what APM installed. The lockfile is integrity-checked + and not forgeable by dropping/omitting files in ``apm_modules/``. + 2. Fallback: path has its own ``apm.yml``. Used when the lockfile + is absent (older installs / fresh checkouts) or does not list + the key. A directory with only a ``.apm/`` marker is treated as + a filesystem intermediary, not a standalone package. + + Combining both signals closes the suppression-via-absence gap + (panel finding: forgeable ``apm.yml`` heuristic) while preserving + behaviour for projects that pre-date the lockfile or have not yet + re-installed. + + Failure mode: only narrowly-typed shape errors against + ``lockfile.dependencies`` (``AttributeError`` / ``TypeError`` / + ``KeyError``) are absorbed and degrade to the ``apm.yml``-only + fallback. Any other exception (e.g. lockfile parse / I/O failure) + propagates so the outer caller can decide whether to log or fail + closed -- preventing a corrupted or attacker-crafted lockfile from + silently disabling the tamper-evident standalone check. + """ + lockfile_keys: set[str] = set() + if lockfile is not None: + try: + for dep_key in lockfile.dependencies: + if dep_key: + lockfile_keys.add(dep_key) + except (AttributeError, TypeError, KeyError): + lockfile_keys = set() + standalone: list = [] + for p in installed: + if p in lockfile_keys: + standalone.append(p) + continue + if (apm_modules_dir / p / APM_YML_FILENAME).exists(): + standalone.append(p) + return standalone + + def _check_orphaned_packages(): """Check for packages in apm_modules/ that are not declared in apm.yml or apm.lock. @@ -210,7 +337,19 @@ def _check_orphaned_packages(): return [] installed = _scan_installed_packages(apm_modules_dir) - return [p for p in installed if p not in expected] + # Combined lockfile-membership + apm.yml fallback determines + # which installed paths are real standalone packages (and so + # must NOT be masked by ancestor expansion). The lockfile is + # the canonical, tamper-evident record; apm.yml-existence is + # the fallback for projects without a lockfile yet. + # See _expand_with_ancestors for the user-safety rationale. + standalone_installed = _standalone_installed_packages( + installed, apm_modules_dir, lockfile=lockfile + ) + expected_with_ancestors = _expand_with_ancestors(expected, standalone_installed) + # Sort for deterministic, diffable output across runs (rglob + # traversal order is filesystem-dependent). + return sorted(p for p in installed if p not in expected_with_ancestors) except Exception: return [] diff --git a/src/apm_cli/commands/deps/cli.py b/src/apm_cli/commands/deps/cli.py index 99fc338de..49a7197b9 100644 --- a/src/apm_cli/commands/deps/cli.py +++ b/src/apm_cli/commands/deps/cli.py @@ -12,6 +12,7 @@ from ...core.command_logger import CommandLogger from ...core.target_detection import TargetParamType from ...models.apm_package import APMPackage, ValidationResult, validate_apm_package # noqa: F401 +from .._helpers import _expand_with_ancestors, _standalone_installed_packages from ._utils import ( _count_package_files, # noqa: F401 _count_primitives, @@ -147,8 +148,8 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): # Scan for installed packages in org-namespaced structure # Walks the tree to find directories containing apm.yml or SKILL.md, # handling GitHub (2-level), ADO (3-level), and subdirectory (4+ level) packages. - installed_packages = [] - orphaned_packages = [] + # First pass: collect valid candidate paths for ancestor-aware orphan check. + scanned_candidates = [] for candidate in apm_modules_path.rglob("*"): if not candidate.is_dir() or candidate.name.startswith("."): continue @@ -159,8 +160,6 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): rel_parts = candidate.relative_to(apm_modules_path).parts if len(rel_parts) < 2: continue - org_repo_name = "/".join(rel_parts) - # Skip sub-skills inside .apm/ directories -- they belong to the parent package if ".apm" in rel_parts: continue @@ -173,7 +172,34 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): and _is_nested_under_package(candidate, apm_modules_path) ): continue + scanned_candidates.append((candidate, "/".join(rel_parts), has_apm_yml, has_skill_md)) + # Precompute expected paths + ancestors for O(1) orphan checks. + # Mirror prune.py / _check_orphaned_packages: pass the standalone + # installed paths (lockfile-membership + apm.yml fallback) so a + # genuinely orphaned ``owner/repo`` package is not masked when a + # sibling subdirectory dep shares the same install root. + try: + try: + lockfile_path_for_check = get_lockfile_path(apm_dir) + lockfile_for_check = ( + LockFile.read(lockfile_path_for_check) if lockfile_path_for_check.exists() else None + ) + except Exception: + lockfile_for_check = None + scanned_names = [name for _c, name, _h, _s in scanned_candidates] + standalone_installed_for_check = _standalone_installed_packages( + scanned_names, apm_modules_path, lockfile=lockfile_for_check + ) + except Exception: + standalone_installed_for_check = [] + declared_with_ancestors = _expand_with_ancestors( + declared_sources.keys(), standalone_installed_for_check + ) + + installed_packages = [] + orphaned_packages = [] + for candidate, org_repo_name, has_apm_yml, _has_skill_md in scanned_candidates: try: version = "unknown" if has_apm_yml: @@ -181,7 +207,7 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): version = package.version or "unknown" primitives = _count_primitives(candidate) - is_orphaned = org_repo_name not in declared_sources + is_orphaned = org_repo_name not in declared_with_ancestors if is_orphaned: orphaned_packages.append(org_repo_name) @@ -210,7 +236,7 @@ def _resolve_scope_deps(apm_dir, logger, insecure_only=False): if insecure_only: installed_packages = [pkg for pkg in installed_packages if pkg["is_insecure"]] - return installed_packages, orphaned_packages + return installed_packages, sorted(orphaned_packages) @click.group(help="Manage APM package dependencies") @@ -277,15 +303,15 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o console.print(table) - # Show orphaned packages warning + # Show orphaned packages warning -- routed through CommandLogger + # so output goes through the central STATUS_SYMBOLS prefix path + # (no raw `[!]` literal that Rich would parse as markup) and so + # behaviour is consistent with prune.py. if orphaned_packages: - console.print( - f"\n[!] {len(orphaned_packages)} orphaned package(s) found (not in apm.yml):", - style="yellow", - ) + logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") for pkg in orphaned_packages: - console.print(f" * {pkg}", style="dim yellow") - console.print("\n Run 'apm prune' to remove orphaned packages", style="cyan") + logger.warning(f" - {pkg}") + logger.info("Run 'apm prune' to remove orphaned packages") else: # Fallback text table if insecure_only: @@ -323,14 +349,13 @@ def _show_scope_deps(scope_label, apm_dir, logger, console, has_rich, insecure_o f"{name:<30} {version:<10} {source:<12} {prompts:>7} {instructions:>7} {agents:>7} {skills:>7} {hooks:>7}" ) - # Show orphaned packages warning + # Show orphaned packages warning -- route through CommandLogger + # for consistency with the rich branch above and with prune.py. if orphaned_packages: - click.echo( - f"\n[!] {len(orphaned_packages)} orphaned package(s) found (not in apm.yml):" - ) + logger.warning(f"{len(orphaned_packages)} orphaned package(s) found (not in apm.yml):") for pkg in orphaned_packages: - click.echo(f" * {pkg}") - click.echo("\n Run 'apm prune' to remove orphaned packages") + logger.warning(f" - {pkg}") + logger.info("Run 'apm prune' to remove orphaned packages") @deps.command(name="list", help="List installed APM dependencies") diff --git a/src/apm_cli/commands/prune.py b/src/apm_cli/commands/prune.py index 817cd9c17..063170cc2 100644 --- a/src/apm_cli/commands/prune.py +++ b/src/apm_cli/commands/prune.py @@ -13,7 +13,12 @@ from ..deps.lockfile import LockFile, get_lockfile_path from ..models.apm_package import APMPackage from ..utils.path_security import PathTraversalError, safe_rmtree # noqa: F401 -from ._helpers import _build_expected_install_paths, _scan_installed_packages +from ._helpers import ( + _build_expected_install_paths, + _expand_with_ancestors, + _scan_installed_packages, + _standalone_installed_packages, +) @click.command(help="Remove APM packages not listed in apm.yml") @@ -57,19 +62,32 @@ def prune(ctx, dry_run): sys.exit(1) installed_packages = _scan_installed_packages(apm_modules_dir) - orphaned_packages = [p for p in installed_packages if p not in expected_installed] + # Mirror _check_orphaned_packages: filter installed paths to + # real standalone packages (lockfile-membership + apm.yml + # fallback) so ancestor expansion does NOT silently mask a + # genuinely orphaned ``owner/repo`` package when a sibling + # subdirectory dep shares the same install root. + # ``apm prune`` is a destructive command -- it MUST behave + # identically to its advisory display path. + standalone_installed = _standalone_installed_packages( + installed_packages, apm_modules_dir, lockfile=lockfile + ) + expected_with_ancestors = _expand_with_ancestors(expected_installed, standalone_installed) + orphaned_packages = sorted( + p for p in installed_packages if p not in expected_with_ancestors + ) if not orphaned_packages: logger.success("No orphaned packages found. apm_modules/ is clean.", symbol="check") return # Show what will be removed - logger.progress(f"Found {len(orphaned_packages)} orphaned package(s):") + logger.warning(f"Found {len(orphaned_packages)} orphaned package(s):") for pkg_name in orphaned_packages: if dry_run: - logger.progress(f" - {pkg_name} (would be removed)") + logger.warning(f" - {pkg_name} (would be removed)") else: - logger.progress(f" - {pkg_name}") + logger.warning(f" - {pkg_name}") if dry_run: logger.success("Dry run complete - no changes made") diff --git a/src/apm_cli/core/command_logger.py b/src/apm_cli/core/command_logger.py index 8c0351e11..506db7683 100644 --- a/src/apm_cli/core/command_logger.py +++ b/src/apm_cli/core/command_logger.py @@ -86,6 +86,19 @@ def progress(self, message: str, symbol: str = "info"): """Log progress during an operation.""" _rich_info(message, symbol=symbol) + def info(self, message: str, symbol: str = "info"): + """Log static advisory / informational context. + + Distinct from :meth:`progress` only at the semantic level: + ``progress`` narrates an in-flight step (may be suppressed in + ``--quiet``/CI), while ``info`` carries persistent advisory + context such as recovery hints that must survive quiet-mode + suppression. Both currently delegate to ``_rich_info``; the + split exists so future quiet-mode policy can drop ``progress`` + without dropping advisory context. + """ + _rich_info(message, symbol=symbol) + def success(self, message: str, symbol: str = "sparkles"): """Log successful completion.""" _rich_success(message, symbol=symbol) diff --git a/tests/unit/test_command_helpers.py b/tests/unit/test_command_helpers.py index b3a1585e0..9a41f988a 100644 --- a/tests/unit/test_command_helpers.py +++ b/tests/unit/test_command_helpers.py @@ -16,6 +16,8 @@ from apm_cli.commands._helpers import ( _atomic_write, _check_and_notify_updates, + _check_orphaned_packages, + _expand_with_ancestors, _get_default_script, _list_available_scripts, _load_apm_config, @@ -249,6 +251,279 @@ def test_returns_empty_for_empty_dir(self, tmp_path): assert result == [] +# --------------------------------------------------------------------------- +# _expand_with_ancestors +# --------------------------------------------------------------------------- + + +class TestExpandWithAncestors: + """Tests for _expand_with_ancestors.""" + + def test_adds_intermediate_ancestors(self): + """Subdirectory path produces install-root ancestors only. + + Depth-cap security contract: ancestors past depth 3 are NOT + emitted (see _expand_with_ancestors docstring). For a path + like ``owner/repo/.apm/skills/my-skill`` only the 2-segment + and 3-segment prefixes are added. + """ + paths = {"owner/repo/.apm/skills/my-skill"} + result = _expand_with_ancestors(paths) + assert "owner/repo" in result + assert "owner/repo/.apm" in result + assert "owner/repo/.apm/skills/my-skill" in result + # Depth cap: deeper ancestors are intentionally not emitted. + assert "owner/repo/.apm/skills" not in result + + def test_two_segment_path_unchanged(self): + """A 2-segment path has no intermediate ancestors to add.""" + paths = {"owner/repo"} + result = _expand_with_ancestors(paths) + assert result == {"owner/repo"} + + def test_empty_input(self): + """Empty set returns empty set.""" + assert _expand_with_ancestors(set()) == set() + + def test_three_segment_ado_path(self): + """ADO-style org/project/repo produces org/project as ancestor.""" + paths = {"org/project/repo"} + result = _expand_with_ancestors(paths) + assert "org/project/repo" in result + assert "org/project" in result + + def test_no_false_prefix_overlap(self): + """owner/repo-extra does not collide with owner/repo.""" + paths = {"owner/repo-extra/.apm/skills/x"} + result = _expand_with_ancestors(paths) + assert "owner/repo-extra" in result + assert "owner/repo" not in result + + def test_skips_path_traversal(self): + """Paths containing '..' are skipped during expansion.""" + paths = {"owner/../etc/passwd"} + result = _expand_with_ancestors(paths) + # Original path is kept (it's in the input set), but no ancestors are generated + assert "owner/../etc/passwd" in result + assert "owner/.." not in result + + def test_skips_backslash_traversal(self): + """Backslash-encoded traversal cannot bypass the '..' guard. + + Regression for the supply-chain finding: prior implementation + called ``p.split('/')`` directly, so a token like + ``owner\\..\\evil/sub`` parsed as a single segment containing + ``..`` and slipped past the guard. The fix normalises ``\\`` + -> ``/`` before splitting. + """ + paths = {"owner\\..\\evil/sub"} + result = _expand_with_ancestors(paths) + # Original kept (membership semantics), but NO ancestor must + # leak into the expansion. + assert "owner\\..\\evil/sub" in result + assert "owner" not in result + assert "owner/.." not in result + assert "owner\\..\\evil" not in result + + def test_installed_guard_protects_real_orphan(self): + """When ``installed`` lists a real standalone package that is + also an ancestor of an expected subdir dep, the ancestor is + NOT added to the expansion -- so the real package can still + be detected as an orphan. + """ + paths = {"owner/repo/.apm/skills/foo"} + result = _expand_with_ancestors(paths, installed={"owner/repo"}) + assert "owner/repo" not in result, ( + "Real installed package must not be masked by ancestor expansion" + ) + + def test_depth_cap_bounds_ancestor_emission(self): + """Ancestors past depth 3 are not emitted (security cap). + + ``_scan_installed_packages`` skips dotted dirs and doesn't see + intermediates past depth 3, so emitting them would only widen + the orphan-suppression surface. + """ + paths = {"owner/repo/.apm/skills/foo/extra/deeper"} + result = _expand_with_ancestors(paths) + assert "owner/repo" in result + # Cap stops emission at depth 3 (exclusive index 4). + assert "owner/repo/.apm/skills" not in result + assert "owner/repo/.apm/skills/foo" not in result + + +# --------------------------------------------------------------------------- +# _check_orphaned_packages -- subdirectory ancestor false-positive fix +# --------------------------------------------------------------------------- + + +class TestCheckOrphanedPackagesSubdirectoryAncestor: + """Tests that parent directories of subdirectory virtual packages are not + falsely flagged as orphaned. + + When a subdirectory package is installed at owner/repo/.apm/skills/name, + the intermediate owner/repo/ directory contains .apm/ and gets picked up + by _scan_installed_packages. The orphan check must recognise it as an + ancestor of an expected path rather than an orphaned package. + """ + + def test_parent_of_subdirectory_package_not_orphaned(self, tmp_path, monkeypatch): + """owner/repo is not orphaned when owner/repo/.apm/skills/x is expected.""" + monkeypatch.chdir(tmp_path) + + # Set up apm.yml with a dict-form dependency using git: + path: + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/my-skill\n", + encoding="utf-8", + ) + + # Simulate the on-disk layout: owner/repo/.apm/skills/my-skill/SKILL.md + apm_modules = tmp_path / "apm_modules" + skill_dir = apm_modules / "owner" / "repo" / ".apm" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") + + orphaned = _check_orphaned_packages() + assert orphaned == [], f"Parent dir should not be orphaned; got: {orphaned}" + + def test_actual_orphan_still_detected(self, tmp_path, monkeypatch): + """A truly orphaned package is still reported even with the ancestor fix.""" + monkeypatch.chdir(tmp_path) + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/my-skill\n", + encoding="utf-8", + ) + + # Simulate an additional unrelated package that IS orphaned + apm_modules = tmp_path / "apm_modules" + skill_dir = apm_modules / "owner" / "repo" / ".apm" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") + + orphan_dir = apm_modules / "other" / "stale-pkg" + orphan_dir.mkdir(parents=True) + (orphan_dir / "apm.yml").write_text("name: stale-pkg", encoding="utf-8") + + orphaned = _check_orphaned_packages() + assert "other/stale-pkg" in orphaned + + def test_whole_repo_dependency_not_orphaned(self, tmp_path, monkeypatch): + """A whole-repo dependency (owner/repo shorthand) is not flagged as orphaned.""" + monkeypatch.chdir(tmp_path) + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - github.example.com/org/my-package\n", + encoding="utf-8", + ) + + # Simulate the installed whole-repo package with .apm content + apm_modules = tmp_path / "apm_modules" + pkg_dir = apm_modules / "org" / "my-package" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0", encoding="utf-8") + apm_dir = pkg_dir / ".apm" + apm_dir.mkdir() + instr_dir = apm_dir / "instructions" + instr_dir.mkdir() + (instr_dir / "example.instructions.md").write_text("# Instructions", encoding="utf-8") + + orphaned = _check_orphaned_packages() + assert orphaned == [], f"Whole-repo package should not be orphaned; got: {orphaned}" + + def test_whole_repo_with_unrelated_orphan(self, tmp_path, monkeypatch): + """Whole-repo dep is fine; an unrelated stale package IS orphaned.""" + monkeypatch.chdir(tmp_path) + + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - github.example.com/org/my-package\n", + encoding="utf-8", + ) + + apm_modules = tmp_path / "apm_modules" + # Declared package + pkg_dir = apm_modules / "org" / "my-package" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: my-package\nversion: 1.0.0", encoding="utf-8") + + # Stale package not in apm.yml + stale_dir = apm_modules / "org" / "old-package" + stale_dir.mkdir(parents=True) + (stale_dir / "apm.yml").write_text("name: old-package\nversion: 0.1.0", encoding="utf-8") + + orphaned = _check_orphaned_packages() + assert "org/my-package" not in orphaned + assert "org/old-package" in orphaned + + def test_real_orphan_at_owner_repo_with_sibling_subdir_dep(self, tmp_path, monkeypatch): + """Regression: a real installed ``owner/repo`` package on disk MUST + still be flagged as orphaned even when a sibling subdirectory dep + ``owner/repo/.apm/skills/foo`` is declared in apm.yml. + + Previously, ancestor expansion blindly added ``owner/repo`` to the + expected set whenever a subdir dep referenced it, silently + suppressing detection of a genuinely orphaned standalone package + that shared the same ``owner/repo`` filesystem root. ``apm prune`` + is a safety command -- it must NEVER silently miss a real orphan. + """ + monkeypatch.chdir(tmp_path) + + # Declare ONLY the subdirectory dep. The standalone owner/repo + # package on disk is NOT declared anywhere. + apm_yml = tmp_path / "apm.yml" + apm_yml.write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/foo\n", + encoding="utf-8", + ) + + apm_modules = tmp_path / "apm_modules" + # Real installed standalone package at owner/repo (with apm.yml AND + # .apm marker). This is a genuine orphan -- nothing in apm.yml + # declares the whole repo as a dep. + pkg_dir = apm_modules / "owner" / "repo" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: repo\nversion: 1.0.0", encoding="utf-8") + # Subdirectory dep content (legitimately installed) shares the + # same ``owner/repo`` root. + skill_dir = pkg_dir / ".apm" / "skills" / "foo" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") + + orphaned = _check_orphaned_packages() + assert "owner/repo" in orphaned, ( + "Real orphan at owner/repo must be flagged even when a " + "sibling subdirectory dep shares the same root; got: " + f"{orphaned}" + ) + + # --------------------------------------------------------------------------- # _check_and_notify_updates # --------------------------------------------------------------------------- @@ -313,3 +588,90 @@ def test_silently_ignores_check_exception(self): ): # Should not raise _check_and_notify_updates() + + +# --------------------------------------------------------------------------- +# Round-3 panel regressions: +# - traversal guard must route through canonical validate_path_segments +# - _standalone_installed_packages must NOT swallow corruption errors +# --------------------------------------------------------------------------- + + +class TestExpandWithAncestorsRoutesThroughCanonicalGuard: + """Round-3 supply-chain finding: ad-hoc ``..`` check must be replaced + by ``apm_cli.utils.path_security.validate_path_segments`` so the + project keeps a single chokepoint for path-segment validation and + also catches single-dot (``.``) traversal segments. + """ + + def test_helpers_traversal_uses_validate_path_segments(self): + """``_expand_with_ancestors`` calls the canonical guard once per + input path. Mocking the guard and asserting it was called proves + the hand-rolled ``".." in parts`` check is gone. + """ + with patch("apm_cli.commands._helpers.validate_path_segments") as mock_guard: + _expand_with_ancestors({"owner/repo/.apm/skills/foo"}) + assert mock_guard.called, ( + "Ancestor expansion must route every input through " + "validate_path_segments rather than a hand-rolled '..' check" + ) + called_paths = {call.args[0] for call in mock_guard.call_args_list} + assert "owner/repo/.apm/skills/foo" in called_paths + + def test_single_dot_segment_now_rejected(self): + """Single-dot segments are rejected by the canonical guard + (which the prior ad-hoc ``".." in parts`` check missed). The + path is kept in the result (membership semantics) but no + ancestors are emitted. + """ + result = _expand_with_ancestors({"owner/./repo"}) + assert "owner/./repo" in result + assert "owner" not in result + assert "owner/." not in result + + +class TestStandaloneInstalledDoesNotSwallowCorruption: + """Round-3 supply-chain finding: bare ``except Exception`` in + ``_standalone_installed_packages`` silently destroyed + ``lockfile_keys`` and failed open on lockfile corruption. The + narrowed ``except (AttributeError, TypeError, KeyError)`` clause + must let unexpected exceptions propagate. + """ + + def test_standalone_installed_does_not_swallow_lockfile_corruption(self, tmp_path): + """A lockfile object whose ``dependencies`` attribute raises an + unexpected exception (e.g. ``RuntimeError`` from a corrupted / + attacker-crafted backing store) must propagate, not silently + return an empty list. + """ + from apm_cli.commands._helpers import _standalone_installed_packages + + class CorruptLockfile: + @property + def dependencies(self): + raise RuntimeError("simulated lockfile corruption") + + with pytest.raises(RuntimeError, match="simulated lockfile corruption"): + _standalone_installed_packages(["owner/repo"], tmp_path, lockfile=CorruptLockfile()) + + def test_standalone_installed_absorbs_narrow_shape_errors(self, tmp_path): + """Narrow shape errors (TypeError when ``dependencies`` is e.g. + an ``int`` due to YAML coercion) are intentionally absorbed and + degrade to the ``apm.yml``-only fallback. + """ + from apm_cli.commands._helpers import _standalone_installed_packages + + class BadShapeLockfile: + dependencies = 42 # not iterable -> TypeError on for-loop + + # Create owner/repo with apm.yml so fallback marks it standalone. + (tmp_path / "owner" / "repo").mkdir(parents=True) + (tmp_path / "owner" / "repo" / "apm.yml").write_text( + "name: r\nversion: 1.0", encoding="utf-8" + ) + result = _standalone_installed_packages( + ["owner/repo"], tmp_path, lockfile=BadShapeLockfile() + ) + assert result == ["owner/repo"], ( + "Narrow shape errors must degrade to apm.yml fallback, not propagate to caller" + ) diff --git a/tests/unit/test_deps_list_tree_info.py b/tests/unit/test_deps_list_tree_info.py index a4ab1df09..d71de4195 100644 --- a/tests/unit/test_deps_list_tree_info.py +++ b/tests/unit/test_deps_list_tree_info.py @@ -169,7 +169,9 @@ def test_list_shows_orphaned_warning(self): with patch("apm_cli.core.scope.get_apm_dir", return_value=tmp), _force_rich_fallback(): result = self.runner.invoke(cli, ["deps", "list"]) assert result.exit_code == 0 - assert "orphaned" in result.output.lower() + # Match the specific orphan-warning header (avoids false-positive + # match against unrelated 'orphan' substrings in future output). + assert "orphaned package(s) found" in result.output def test_list_version_shown(self): """Version from apm.yml should appear in fallback text output.""" @@ -279,6 +281,48 @@ def test_list_insecure_reports_clean_when_no_http_locked_deps(self): assert result.exit_code == 0 assert "No insecure APM dependencies installed" in result.output + def test_list_subdirectory_parent_not_orphaned(self): + """Parent dir of a subdirectory virtual package is not flagged orphaned. + + Per panel feedback (test fixture nit): the simpler -- and more + accurate -- representation of the subdir-dep bug uses NO + ``apm.yml`` at the ``owner/repo`` parent (only the ``.apm/`` + marker created by the clone). With ``apm.yml`` present at + ``owner/repo`` the directory is a real standalone package and + the new ``standalone_installed`` guard correctly flags it as + orphaned (it is not declared anywhere). Drop the conflated + ``apm.yml`` to test the intended scenario. + """ + with self._chdir_tmp() as tmp: + # Declare a dict-form dependency with path: pointing into .apm/skills/ + (tmp / "apm.yml").write_text( + "name: test-project\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/my-skill\n", + encoding="utf-8", + ) + + # Simulate installed layout: owner/repo/ with NO apm.yml -- + # only the cloned .apm/ subtree. ``owner/repo`` is a pure + # filesystem intermediary, not a standalone package. + repo_dir = tmp / "apm_modules" / "owner" / "repo" + repo_dir.mkdir(parents=True) + skill_dir = repo_dir / ".apm" / "skills" / "my-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill", encoding="utf-8") + + with patch("apm_cli.core.scope.get_apm_dir", return_value=tmp), _force_rich_fallback(): + result = self.runner.invoke(cli, ["deps", "list"]) + + assert result.exit_code == 0 + # Assertion targets the deps-specific orphan header + # (deps says "orphaned package(s) found", prune says + # "Found N orphaned package(s)"). + assert "orphaned package(s) found" not in result.output + class TestDepsTreeCommand(_DepsCmdBase): """Tests for apm deps tree.""" diff --git a/tests/unit/test_orphan_announce_parity.py b/tests/unit/test_orphan_announce_parity.py new file mode 100644 index 000000000..cf4b2513f --- /dev/null +++ b/tests/unit/test_orphan_announce_parity.py @@ -0,0 +1,160 @@ +"""Round-3 panel regressions: orphan-announce channel parity. + +Both `apm prune` and `apm deps list` surface the same semantic event +("orphans found") and must therefore use the same logger channel: + + * Orphan-found header + per-package bullets -> ``logger.warning`` + (a destructive command must be at least as loud as an advisory + display command, and bullets are subordinate context of that + warning -- not transient progress narration). + * Recovery hint ("Run 'apm prune'...") -> ``logger.info`` + (advisory remediation, not the problem statement; using + ``logger.progress`` risks suppression in ``--quiet`` / CI mode and + silently drops the actionable hint). + +These tests pin the channel mapping so a future refactor cannot +silently regress to ``logger.progress``. +""" + +from __future__ import annotations + +import contextlib +import os +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from click.testing import CliRunner + +from apm_cli.cli import cli + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +_APM_YML_NO_DEPS = "name: t\nversion: 1.0.0\ndependencies:\n apm: []\n" + + +def _make_orphan_dir(tmp: Path, owner: str, repo: str) -> Path: + pkg = tmp / "apm_modules" / owner / repo + pkg.mkdir(parents=True) + (pkg / "apm.yml").write_text("name: r\nversion: 1.0.0", encoding="utf-8") + return pkg + + +@contextlib.contextmanager +def _chdir_tmp(): + with tempfile.TemporaryDirectory() as td: + prev = Path.cwd() + os.chdir(td) + try: + yield Path(td) + finally: + os.chdir(prev) + + +def _capture_logger_calls(): + """Return a (logger_factory, calls) tuple. + + The factory is suitable for patching CommandLogger; ``calls`` is a + list of ``(method_name, message)`` tuples in invocation order. + """ + calls: list[tuple[str, str]] = [] + + def _make(*_args, **_kwargs): + logger = MagicMock() + + def _record(name): + def _inner(msg, *a, **k): + calls.append((name, str(msg))) + + return _inner + + for method in ( + "info", + "warning", + "error", + "success", + "progress", + "start", + "debug", + ): + setattr(logger, method, _record(method)) + return logger + + return _make, calls + + +# --------------------------------------------------------------------------- +# Parity: orphan-found header + bullets routed via logger.warning +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "command, patch_target", + [ + (["prune", "--dry-run"], "apm_cli.commands.prune.CommandLogger"), + (["deps", "list"], "apm_cli.commands.deps.cli.CommandLogger"), + ], +) +def test_orphan_announce_level_parity_prune_vs_deps_cli(command, patch_target): + """Both surfaces emit the orphan-found header AND each per-package + bullet through ``logger.warning`` -- never ``logger.progress``. + """ + runner = CliRunner() + factory, calls = _capture_logger_calls() + with _chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_APM_YML_NO_DEPS) + _make_orphan_dir(tmp, "orphan-org", "orphan-repo") + with patch(patch_target, side_effect=factory): + result = runner.invoke(cli, command) + assert result.exit_code == 0, result.output + + warning_msgs = [m for (lvl, m) in calls if lvl == "warning"] + progress_msgs = [m for (lvl, m) in calls if lvl == "progress"] + + # Header (substring match -- exact text differs slightly between commands) + assert any("orphan" in m.lower() and "package" in m.lower() for m in warning_msgs), ( + f"Orphan-found header must be at warning level. " + f"warnings={warning_msgs!r} progress={progress_msgs!r}" + ) + # Per-package bullet for the orphan must be at warning level too. + assert any("orphan-org/orphan-repo" in m for m in warning_msgs), ( + f"Per-package orphan bullet must be at warning level. " + f"warnings={warning_msgs!r} progress={progress_msgs!r}" + ) + # And must NOT appear at progress level (the regression we are + # pinning closed). + assert not any("orphan-org/orphan-repo" in m for m in progress_msgs), ( + f"Per-package orphan bullet must NOT be emitted at progress " + f"level. progress={progress_msgs!r}" + ) + + +def test_orphan_recovery_hint_uses_info_not_progress(): + """The ``Run 'apm prune' to remove orphaned packages`` hint emitted + by ``apm deps list`` is advisory remediation context. It must use + ``logger.info`` so it survives quiet/CI suppression of the + in-flight ``progress`` channel. + """ + runner = CliRunner() + factory, calls = _capture_logger_calls() + with _chdir_tmp() as tmp: + (tmp / "apm.yml").write_text(_APM_YML_NO_DEPS) + _make_orphan_dir(tmp, "orphan-org", "orphan-repo") + with patch("apm_cli.commands.deps.cli.CommandLogger", side_effect=factory): + result = runner.invoke(cli, ["deps", "list"]) + assert result.exit_code == 0, result.output + + info_msgs = [m for (lvl, m) in calls if lvl == "info"] + progress_msgs = [m for (lvl, m) in calls if lvl == "progress"] + + assert any("apm prune" in m for m in info_msgs), ( + f"Recovery hint must be emitted at info level. " + f"info={info_msgs!r} progress={progress_msgs!r}" + ) + assert not any("apm prune" in m for m in progress_msgs), ( + f"Recovery hint must NOT be emitted at progress level. progress={progress_msgs!r}" + ) diff --git a/tests/unit/test_prune_command.py b/tests/unit/test_prune_command.py index f7f868bb2..713d66cd4 100644 --- a/tests/unit/test_prune_command.py +++ b/tests/unit/test_prune_command.py @@ -225,6 +225,79 @@ def test_prune_removes_multiple_orphans(self): assert not dir1.exists() assert not dir2.exists() + def test_prune_removes_real_orphan_with_sibling_subdir_dep(self): + """Regression: the destructive ``apm prune`` command must + delete a genuinely orphaned ``owner/repo`` package even when + a sibling subdirectory dep ``owner/repo/.apm/skills/foo`` is + declared in apm.yml. + + Previously, ``prune.py`` called ``_expand_with_ancestors`` + without the ``standalone_installed`` guard, so ``owner/repo`` + was added to the expected set as an ancestor of the subdir + dep -- silently suppressing deletion of a real orphan and + diverging from the advisory display path. ``apm prune`` is a + safety command; missing a real orphan is a correctness bug. + """ + with self._chdir_tmp() as tmp: + # Declare ONLY the subdirectory dep. The standalone + # owner/repo package is not declared anywhere. + (tmp / "apm.yml").write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/foo\n" + ) + # Real installed standalone package (apm.yml + .apm marker). + pkg_dir = tmp / "apm_modules" / "owner" / "repo" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: repo\nversion: 1.0\n") + # Subdirectory dep content cohabits the same install root. + skill_dir = pkg_dir / ".apm" / "skills" / "foo" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill\n") + + result = self.runner.invoke(cli, ["prune"]) + assert result.exit_code == 0, result.output + # Real orphan MUST be deleted -- this is the security + # invariant the panel flagged as a required fix. + assert not (pkg_dir / "apm.yml").exists(), ( + "Real orphan owner/repo (apm.yml) must be removed even " + "when a sibling subdir dep shares the same root" + ) + # Subdir dep content collateral-damages because the whole + # owner/repo tree is the orphan's filesystem footprint; + # the user is expected to re-install. This matches the + # advisory display path in deps/cli.py. + assert not skill_dir.exists() + + def test_prune_dry_run_lists_real_orphan_with_sibling_subdir_dep(self): + """Dry-run path must also surface the real orphan (display + parity with the advisory check). + """ + with self._chdir_tmp() as tmp: + (tmp / "apm.yml").write_text( + "name: test\n" + "version: 1.0.0\n" + "dependencies:\n" + " apm:\n" + " - git: github.example.com/owner/repo\n" + " path: .apm/skills/foo\n" + ) + pkg_dir = tmp / "apm_modules" / "owner" / "repo" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: repo\nversion: 1.0\n") + skill_dir = pkg_dir / ".apm" / "skills" / "foo" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text("# Skill\n") + + result = self.runner.invoke(cli, ["prune", "--dry-run"]) + assert result.exit_code == 0, result.output + assert "owner/repo" in result.output + # No deletion occurred. + assert (pkg_dir / "apm.yml").exists() + # ------------------------------------------------------------------ # Parse error in apm.yml # ------------------------------------------------------------------