Don't flag parent dirs of subdirectory packages as orphaned

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

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 []
 

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,15 +172,42 @@ 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:
                 package = APMPackage.from_apm_yml(candidate / APM_YML_FILENAME)
                 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")

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")

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)