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 correctly resolves transitive `local_path` dependencies (npm/pip/cargo parity).** A package's `local_path` references are now anchored on **that package's own directory**, matching how every other package manager handles workspace-style relative paths. Previously a transitive `../sibling` declared inside `packages/specialized/apm.yml` was anchored on the **consumer**'s project root, which made shared workspace layouts (mono-repos, side-by-side helper packages) impossible to express portably -- the same `apm.yml` worked or broke depending on where the consumer lived in their tree. **Security tightening:** remote-cloned packages can no longer declare a local-filesystem `local_path` dependency (`ERROR` severity, fail-closed at resolve time, both relative and absolute paths rejected). Authors of remote packages who previously relied on `local_path` should switch the dependency to a published reference (`owner/repo` or marketplace handle) before upgrading. Sibling and monorepo layouts authored by the consumer (e.g. `apm install ../pkg-a`) continue to work unchanged. (#940, 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. The resolved path must still stay inside the consuming project root -- a `local_path` that escapes the project (for example via too many `..` segments) is rejected with a red error and an actionable hint.
+
+  ```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,12 @@ 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). The resolved path must stay inside the consuming project root --
+escaping it (e.g. via too many `..`) is rejected with a red error. Remote
+packages cannot declare `local_path` deps at all.
+
 ### 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,64 @@ 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
+
     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 +150,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 +164,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

@@ -114,14 +114,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 +145,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
@@ -303,6 +320,34 @@ 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.
+    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
+            )
+            dep_base_dirs[node.dependency_ref.get_unique_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
     # ------------------------------------------------------------------

src/apm_cli/install/sources.py

@@ -156,7 +156,19 @@ def acquire(self) -> Materialization | None:
                     )
                 return None
 
-        result_path = _copy_local_package(dep_ref, install_path, ctx.project_root, logger=logger)
+        # Determine the anchor for relative ``local_path`` (#857). For direct
+        # deps from the root project this is project_root. For transitive
+        # deps declared inside another local package, it is the parent
+        # package's source directory -- captured during resolve via
+        # ``ctx.dep_base_dirs``.
+        base_dir = getattr(ctx, "dep_base_dirs", {}).get(dep_key) or ctx.project_root
+        result_path = _copy_local_package(
+            dep_ref,
+            install_path,
+            base_dir,
+            project_root=ctx.project_root,
+            logger=logger,
+        )
         if not result_path:
             diagnostics.error(
                 f"Failed to copy local package: {dep_ref.local_path}",
@@ -167,10 +179,27 @@ def acquire(self) -> Materialization | None:
         if logger:
             logger.download_complete(dep_ref.local_path, ref_suffix="local")
 
-        # Build minimal PackageInfo for integration
+        # Build minimal PackageInfo for integration. Anchor source_path on
+        # the *original* user source directory (not the apm_modules copy) so
+        # any transitive ``../sibling`` dep declared inside this package
+        # resolves against where the developer wrote the path (#857).
         local_apm_yml = install_path / "apm.yml"
         if local_apm_yml.exists():
-            local_pkg = APMPackage.from_apm_yml(local_apm_yml)
+            original_src = Path(dep_ref.local_path).expanduser()
+            if not original_src.is_absolute():
+                # For TRANSITIVE local deps the relative path is anchored on
+                # the parent package's directory (base_dir above), not on
+                # the consumer's project root. Reusing base_dir here keeps
+                # the source_path stamped on the loaded APMPackage in lock-
+                # step with where _copy_local_package actually copied from.
+                original_src = (base_dir / original_src).resolve()
+            else:
+                original_src = original_src.resolve()
+            local_pkg = APMPackage.from_apm_yml(local_apm_yml, source_path=original_src)
+            # TODO(#940): post-construction mutation of .source has the same
+            # cache-poisoning shape as the bug fixed in this PR. Today the
+            # cache key is (apm.yml, source_path) so mutating .source is
+            # safe, but keep this in mind when reworking the source field.
             if not local_pkg.source:
                 local_pkg.source = dep_ref.local_path
         else:
@@ -297,10 +326,14 @@ def acquire(self) -> Materialization | None:
                 deltas=deltas,
             )
 
-        # Load package from apm.yml
+        # Load package from apm.yml. Anchor source_path on the clone location
+        # so transitive ``local_path`` deps inside this remote package resolve
+        # from there (#857).
         apm_yml_path = install_path / APM_YML_FILENAME
         if apm_yml_path.exists():
-            cached_package = APMPackage.from_apm_yml(apm_yml_path)
+            cached_package = APMPackage.from_apm_yml(apm_yml_path, source_path=install_path)
+            # TODO(#940): see note in _materialize_local for the same caveat
+            # about post-construction mutation of .source.
             if not cached_package.source:
                 cached_package.source = dep_ref.repo_url
         else: