fix(install): anchor transitive local_path deps on declaring package (#857)

CHANGELOG.md

@@ -40,6 +40,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **`apm install` now anchors transitive `local_path` deps on the declaring package's directory (npm/pip/cargo parity).** Sibling/monorepo layouts (e.g. `../base` declared inside `packages/specialized/apm.yml`) now resolve relative to the declaring package, not the consumer's project root. **Security tightening:** remote-cloned packages can no longer declare `local_path` deps -- both relative and absolute paths are rejected at `ERROR` severity at resolve time. (#1111, closes #857) Thanks @JahanzaibTayyab.
 - `apm compile` no longer silently drops instructions without an `applyTo` pattern from generated `AGENTS.md` and `CLAUDE.md`; globals now render under a `## Global Instructions` section, matching the optimizer's existing `(global)` placement (#1088, closes #1072)
 - `apm install` no longer masks local-bundle install failures with `UnboundLocalError`. (#PR_NUMBER)
 - **`apm install <pkg>@<marketplace>` no longer fails for all marketplace packages.** The install resolver now accepts both legacy and current marketplace key names: `repository`/`repo` for github sources, `url`/`repo` for git-subdir sources, and `type`/`source` as the source-type discriminator. A scheme guard rejects full URLs passed through the `url` fallback. (#1106, closes #1105)

docs/src/content/docs/guides/dependencies.md

@@ -298,6 +298,21 @@ dependencies:
 - Local packages are validated the same as remote packages (must have `apm.yml` or `SKILL.md`)
 - `apm compile` works identically regardless of dependency source
 - Transitive dependencies are resolved recursively (local packages can depend on remote packages)
+- **Anchor rule:** a `local_path` declared **inside another local package** is resolved relative to **that package's own directory**, not the consumer's project root. This matches npm/pip/cargo workspace behaviour and is what makes mono-repos with sibling helper packages portable across consumers. Sibling layouts that resolve **outside** the consuming project root (e.g. `../sibling-pkg` from a local dep at the project edge) are supported -- the consuming developer authored the manifest chain and trusts the layout. The security boundary lives upstream: see the next bullet.
+
+  ```yaml
+  # apm.yml at /repo/apm.yml
+  dependencies:
+    apm:
+      - ./packages/specialized
+
+  # apm.yml at /repo/packages/specialized/apm.yml
+  dependencies:
+    apm:
+      - ../base   # resolves to /repo/packages/base, NOT /repo/base
+  ```
+
+- **Remote packages may not declare local dependencies.** A package fetched from `owner/repo` cannot depend on a `local_path` -- such an entry would reach into the consumer's filesystem in unpredictable ways. Both relative and absolute local paths are rejected at `ERROR` severity. Authors of remote packages must publish their dependencies (or vendor them via subdirectory packages).
 
 **Re-install behavior:** Local deps are always re-copied on `apm install` since there is no commit SHA to cache against. This ensures you always get the latest local changes.
 

packages/apm-guide/.apm/skills/apm-usage/dependencies.md

@@ -35,6 +35,15 @@ dependencies:
     - ../sibling-repo/my-package
 ```
 
+**Local-path anchor rule:** a `local_path` declared INSIDE another local
+package is resolved relative to THAT package's own directory (npm/pip/cargo
+parity). Sibling layouts that resolve outside the consuming project root
+(e.g. `../sibling-pkg` from a local dep at the project edge) are
+supported -- the consuming developer authored the manifest chain and
+already trusts the layout. The actual security boundary is upstream:
+**remote-cloned packages cannot declare `local_path` deps at all**, since
+they have no business reaching into the consumer's filesystem.
+
 ### Custom git ports
 
 Non-default git ports are preserved on `https://`, `http://`, and `ssh://` URLs

src/apm_cli/install/phases/local_content.py

@@ -30,8 +30,9 @@
 
 from pathlib import Path
 
-from apm_cli.utils.console import _rich_error
-from apm_cli.utils.path_security import safe_rmtree
+from apm_cli.utils.path_security import (
+    safe_rmtree,
+)
 
 # ---------------------------------------------------------------------------
 # Root primitive detection helpers
@@ -83,32 +84,77 @@ def _has_local_apm_content(project_root):
 # ---------------------------------------------------------------------------
 
 
-def _copy_local_package(dep_ref, install_path, project_root, logger=None):
+def _copy_local_package(dep_ref, install_path, base_dir, *, project_root, logger):
     """Copy a local package to apm_modules/.
 
     Args:
-        dep_ref: DependencyReference with is_local=True
-        install_path: Target path under apm_modules/
-        project_root: Project root for resolving relative paths
-        logger: Optional CommandLogger for structured output
+        dep_ref: DependencyReference with is_local=True.
+        install_path: Target path under apm_modules/.
+        base_dir: Directory used to resolve a relative ``dep_ref.local_path``.
+            For direct deps from the root project this is the project root;
+            for transitive deps it is the source directory of the package
+            whose apm.yml declared *dep_ref* (#857). Must NOT be confused
+            with ``project_root`` -- the anchoring base and the security
+            containment boundary are deliberately distinct concerns.
+        project_root: Project root, threaded through for symmetry with the
+            anchoring story but NOT used as a hard containment boundary
+            here. The actual untrusted-source boundary lives upstream in
+            :mod:`apm_cli.deps.apm_resolver` (``_try_load_dependency_package``
+            dual-rejects any local_path declared by a remote parent before
+            this function ever runs). Enforcing project-root containment
+            *here* would also block legitimate sibling layouts -- e.g.
+            ``apm install ../pkg-a`` from a monorepo workspace -- which
+            users explicitly opt into. Kept on the signature so callers
+            keep the security model in mind and so a future tightening
+            (e.g. opt-in strict mode) has a hook.
+        logger: Required CommandLogger for structured output. Callers must
+            thread one in; we no longer fall back to a bare console helper
+            (#940 SR4) because doing so masked logger threading bugs in
+            transitive call stacks.
 
     Returns:
-        install_path on success, None on failure
+        install_path on success, None on failure.
+
+    Notes:
+        We deliberately do NOT call ``validate_path_segments`` on
+        ``dep_ref.local_path``: that helper rejects ``..`` segments, which
+        would break the legitimate ``../sibling`` pattern this PR enables.
+        The untrusted-source boundary is the resolver-level dual-reject
+        of remote-parent local_paths; everything reaching this function
+        comes from a parent the user already trusts (their own manifest,
+        a CLI arg, or another local package they explicitly added).
     """
     import shutil
 
+    # project_root retained on signature for future strict-mode hook (see
+    # docstring); not consumed in the current copy path.
+    _ = project_root
+
+    # PR #1111 review C1: ``ctx.logger`` is allowed to be None
+    # (``run_install_pipeline(logger=None)`` is a public, documented entry
+    # point). Without this guard the unconditional ``logger.error(...)``
+    # calls below would AttributeError for any local dep when a caller
+    # does not thread an InstallLogger through. Defaulting to the rich-
+    # console-backed ``NullCommandLogger`` keeps the error visible to the
+    # user while preserving the documented "logger is required" contract
+    # for callers that DO thread one in (their logger wins).
+    if logger is None:
+        from apm_cli.core.null_logger import NullCommandLogger
+
+        logger = NullCommandLogger()
+
     local = Path(dep_ref.local_path).expanduser()
+    # Anchor on the *declaring* package's directory (#857). For direct deps
+    # from the root, ``base_dir`` IS ``project_root`` so behavior is
+    # unchanged. For transitive deps, ``base_dir`` is the parent package's
+    # source dir. Absolute paths bypass anchoring.
     if not local.is_absolute():  # noqa: SIM108
-        local = (project_root / local).resolve()
+        local = (base_dir / local).resolve()
     else:
         local = local.resolve()
 
     if not local.is_dir():
-        msg = f"Local package path does not exist: {dep_ref.local_path}"
-        if logger:
-            logger.error(msg)
-        else:
-            _rich_error(msg)
+        logger.error(f"Local package path does not exist: {dep_ref.local_path}")
         return None
     from apm_cli.utils.helpers import find_plugin_json
 
@@ -117,14 +163,10 @@ def _copy_local_package(dep_ref, install_path, project_root, logger=None):
         and not (local / "SKILL.md").exists()
         and find_plugin_json(local) is None
     ):
-        msg = (
-            f"Local package is not a valid APM package "
+        logger.error(
+            "Local package is not a valid APM package "
             f"(no apm.yml, SKILL.md, or plugin.json): {dep_ref.local_path}"
         )
-        if logger:
-            logger.error(msg)
-        else:
-            _rich_error(msg)
         return None
 
     # Ensure parent exists and clean target (always re-copy for local deps)
@@ -135,5 +177,15 @@ def _copy_local_package(dep_ref, install_path, project_root, logger=None):
         apm_modules_dir = install_path.parent.parent  # _local/<name> -> apm_modules
         safe_rmtree(install_path, apm_modules_dir)
 
+    # SECURITY: symlinks=True preserves in-package symlinks rather than
+    # dereferencing them. This is INTENTIONAL: a package author who ships a
+    # symlink owns the consequences. The link is inert in apm_modules; any
+    # consumer tool that follows it is responsible for its own sandboxing.
+    # SECURITY: TOCTOU window between local.resolve() above and copytree
+    # here. An attacker with write access to the source tree could swap the
+    # directory for a symlink in this gap; but such an attacker can already
+    # modify deployed files directly, so the mitigation cost (atomic dir
+    # operations) outweighs the marginal risk. Future hardening should land
+    # at this site.
     shutil.copytree(local, install_path, dirs_exist_ok=False, symlinks=True)
     return install_path

src/apm_cli/install/phases/resolve.py

@@ -19,12 +19,15 @@
 from __future__ import annotations
 
 import builtins
+import logging
 from pathlib import Path
 from typing import TYPE_CHECKING
 
 if TYPE_CHECKING:
     from apm_cli.install.context import InstallContext
 
+_logger = logging.getLogger(__name__)
+
 
 def run(ctx: InstallContext) -> None:
     """Execute the resolve phase.
@@ -114,14 +117,19 @@ def run(ctx: InstallContext) -> None:
     logger = ctx.logger
     verbose = ctx.verbose  # noqa: F841
 
-    def download_callback(dep_ref, modules_dir, parent_chain=""):
+    def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None):
         """Download a package during dependency resolution.
 
         Args:
             dep_ref: The dependency to download.
             modules_dir: Target apm_modules directory.
             parent_chain: Human-readable breadcrumb (e.g. "root > mid")
                 showing which dependency path led to this transitive dep.
+            parent_pkg: APMPackage that declared *dep_ref*, or None for direct
+                deps from the root project. For local deps we use its
+                ``source_path`` as the anchor for relative paths so a
+                transitive ``../sibling`` resolves against the declaring
+                package's directory rather than the root consumer (#857).
         """
         install_path = dep_ref.get_install_path(modules_dir)
         if install_path.exists():
@@ -140,8 +148,20 @@ def download_callback(dep_ref, modules_dir, parent_chain=""):
                     # so use .add() rather than dict-style assignment.
                     callback_failures.add(dep_ref.get_unique_key())
                     return None
+                # Anchor relative paths on the *declaring* package's source
+                # directory when available (#857). Falls back to project_root
+                # for direct deps and for parents that predate source_path.
+                base_dir = (
+                    parent_pkg.source_path
+                    if parent_pkg is not None and parent_pkg.source_path is not None
+                    else project_root
+                )
                 result_path = _copy_local_package(
-                    dep_ref, install_path, project_root, logger=logger
+                    dep_ref,
+                    install_path,
+                    base_dir,
+                    project_root=project_root,
+                    logger=logger,
                 )
                 if result_path:
                     callback_downloaded[dep_ref.get_unique_key()] = None
@@ -209,6 +229,14 @@ def download_callback(dep_ref, modules_dir, parent_chain=""):
     dependency_graph = resolver.resolve_dependencies(ctx.apm_dir)
     ctx.dependency_graph = dependency_graph
 
+    # Fold remote-parent local_path rejections into ``callback_failures`` so
+    # the integrate phase skips them via the same gate used for download
+    # failures (PR #1111 review C2). The resolver has already emitted the
+    # red ERROR notice; here we just propagate the dep_key.
+    rejected_remote_local = getattr(resolver, "_rejected_remote_local_keys", set())
+    if rejected_remote_local:
+        callback_failures.update(rejected_remote_local)
+
     # Verbose: show resolved tree summary
     if ctx.logger:
         tree = dependency_graph.dependency_tree
@@ -303,6 +331,67 @@ def _collect_descendants(node, visited=None):
 
     ctx.deps_to_install = deps_to_install
 
+    # ------------------------------------------------------------------
+    # 7.5 Build dep_key -> parent source_path map for transitive locals
+    # ------------------------------------------------------------------
+    # Local deps declared by a transitive parent must be anchored on the
+    # parent's source dir, not on the consumer's project root (#857). We
+    # walk the dependency tree once here and stash the per-dep base_dir
+    # for the integrate phase to consume.
+    #
+    # Keying caveat (PR #1111 review C3): the map is keyed by
+    # ``dep_ref.get_unique_key()``, which for local deps is the raw
+    # ``local_path`` string. Two different parents that both declare the
+    # same relative ``local_path`` (e.g. both write ``../base``) collapse
+    # to the same key. In the current architecture this collision is
+    # latent: the BFS walk in ``APMDependencyResolver`` already dedupes
+    # by ``get_unique_key()`` so only one node ever exists for that key,
+    # and ``DependencyReference.get_install_path`` shares the same
+    # ``apm_modules/_local/<basename>`` slot regardless of the parent.
+    # That means today the "second parent wins" question never actually
+    # fires -- the second occurrence is dropped at queue-time. We still
+    # detect divergent-anchor writes here and warn loudly, both because
+    # silent first-wins behaviour would mask a real bug if BFS dedup ever
+    # changes, and because the warning gives the user a path to diagnose
+    # surprising layouts (e.g. ``../base`` from two parents resolving to
+    # different absolute directories).
+    dep_base_dirs: builtins.dict[str, Path] = {}
+    try:
+        tree = dependency_graph.dependency_tree
+        for node in tree.nodes.values():
+            parent_node = node.parent
+            if parent_node is None or parent_node.package is None:
+                continue
+            anchor = (
+                parent_node.package.source_path
+                if parent_node.package.source_path is not None
+                else project_root
+            )
+            key = node.dependency_ref.get_unique_key()
+            existing = dep_base_dirs.get(key)
+            if existing is not None and existing != anchor:
+                # Divergent anchors for the same dep key. Keep the first
+                # (deterministic) and surface the conflict so the user can
+                # rename one of the colliding refs or use absolute paths.
+                _logger.warning(
+                    "Local dep %r is referenced from two parents with "
+                    "different anchors (%s vs %s). Using the first; "
+                    "rename one of the local_path values or use absolute "
+                    "paths to disambiguate.",
+                    key,
+                    existing,
+                    anchor,
+                )
+                continue
+            dep_base_dirs[key] = anchor
+    except (AttributeError, KeyError):
+        # Tree shape may differ across releases; fall back to empty map
+        # (callers default to project_root anchoring, matching legacy).
+        # Narrow set: real bugs (TypeError/NameError) should surface, not
+        # silently degrade to legacy anchoring.
+        dep_base_dirs = {}
+    ctx.dep_base_dirs = dep_base_dirs
+
     # ------------------------------------------------------------------
     # 8. Orphan detection: intended_dep_keys
     # ------------------------------------------------------------------