From b28d6aedfaf649994e8db32cd509d8c291f0a54f Mon Sep 17 00:00:00 2001 From: Jahanzaib Tayyab Date: Sun, 26 Apr 2026 00:21:10 +0500 Subject: [PATCH 1/2] fix: resolve transitive local paths against declaring package (#857) Relative ``local_path`` dependencies declared inside a non-root package's apm.yml were resolved against the root consumer's project directory rather than the package's own source directory. This made it impossible to compose two local sibling packages where one declares the other as a dependency. Introduce ``APMPackage.source_path`` to track the on-disk directory holding each package's apm.yml, and thread the declaring package through the download callback so ``_copy_local_package`` can anchor relative paths on the right base directory. Aligns with the semantic every other package manager uses for relative paths in manifests. Prepared with assistance from Claude (Anthropic) under human review. --- src/apm_cli/deps/apm_resolver.py | 94 +++++++++++++--- src/apm_cli/install/phases/local_content.py | 9 +- src/apm_cli/install/phases/resolve.py | 20 +++- src/apm_cli/models/apm_package.py | 6 + tests/test_apm_resolver.py | 115 ++++++++++++++++++++ tests/unit/test_install_command.py | 3 +- 6 files changed, 224 insertions(+), 23 deletions(-) diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index 69aa48faa..f4376d648 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -11,10 +11,13 @@ ) # Type alias for the download callback. -# Takes (dep_ref, apm_modules_dir, parent_chain) and returns the install path -# if successful. ``parent_chain`` is a human-readable breadcrumb string like -# "root-pkg > mid-pkg > this-pkg" showing the full dependency path including -# the current node, or just the node's display name for direct (depth-1) deps. +# Takes (dep_ref, apm_modules_dir, parent_chain, parent_pkg) and returns the +# install path if successful. ``parent_chain`` is a human-readable breadcrumb +# string like "root-pkg > mid-pkg > this-pkg" showing the full dependency path +# including the current node, or just the node's display name for direct +# (depth-1) deps. ``parent_pkg`` is the APMPackage that declared this +# dependency (None for direct deps from the root); callers use its +# ``source_path`` to anchor relative ``local_path`` resolution (#857). @runtime_checkable class DownloadCallback(Protocol): def __call__( @@ -22,6 +25,7 @@ def __call__( dep_ref: 'DependencyReference', apm_modules_dir: Path, parent_chain: str = "", + parent_pkg: Optional['APMPackage'] = None, ) -> Optional[Path]: ... @@ -79,6 +83,8 @@ def resolve_dependencies(self, project_root: Path) -> DependencyGraph: try: root_package = APMPackage.from_apm_yml(apm_yml_path) + # Anchor for relative local_path dependencies declared at the root. + root_package.source_path = project_root.resolve() except (ValueError, FileNotFoundError) as e: # Create error graph empty_package = APMPackage(name="error", version="0.0.0", package_path=project_root) @@ -213,7 +219,9 @@ def build_dependency_tree(self, root_apm_yml: Path) -> DependencyTree: parent_chain = node.get_ancestor_chain() loaded_package = self._try_load_dependency_package( - dep_ref, parent_chain=parent_chain + dep_ref, + parent_chain=parent_chain, + parent_pkg=parent_node.package if parent_node else None, ) if loaded_package: # Update the node with the actual loaded package @@ -358,35 +366,43 @@ def _validate_dependency_reference(self, dep_ref: DependencyReference) -> bool: return True def _try_load_dependency_package( - self, dep_ref: DependencyReference, parent_chain: str = "" + self, + dep_ref: DependencyReference, + parent_chain: str = "", + parent_pkg: Optional[APMPackage] = None, ) -> Optional[APMPackage]: """ Try to load a dependency package from apm_modules/. - + This method scans apm_modules/ to find installed packages and loads their apm.yml to enable transitive dependency resolution. If a package is not installed and a download_callback is available, it will attempt to fetch the package first. - + Args: dep_ref: Reference to the dependency to load parent_chain: Human-readable breadcrumb of the dependency path that led here (e.g. "root-pkg > mid-pkg"). Forwarded to the download callback for contextual error messages. - + parent_pkg: APMPackage that declared *dep_ref* (None for direct + deps from the root project). Its ``source_path`` is forwarded + to the download callback so relative ``local_path`` deps + resolve against the directory containing the declaring + apm.yml rather than the root consumer (#857). + Returns: APMPackage: Loaded package if found, None otherwise - + Raises: ValueError: If package exists but has invalid format FileNotFoundError: If package cannot be found """ if self._apm_modules_dir is None: return None - + # Get the canonical install path for this dependency install_path = dep_ref.get_install_path(self._apm_modules_dir) - + # If package doesn't exist locally, try to download it if not install_path.exists(): if self._download_callback is not None: @@ -395,7 +411,10 @@ def _try_load_dependency_package( if unique_key not in self._downloaded_packages: try: downloaded_path = self._download_callback( - dep_ref, self._apm_modules_dir, parent_chain + dep_ref, + self._apm_modules_dir, + parent_chain, + parent_pkg, ) if downloaded_path and downloaded_path.exists(): self._downloaded_packages.add(unique_key) @@ -403,11 +422,18 @@ def _try_load_dependency_package( except Exception: # Download failed - continue without this dependency's sub-deps pass - + # Still doesn't exist after download attempt if not install_path.exists(): return None - + + # Anchor for resolving this package's own relative local_path deps: + # the *original* source dir for local deps (not the apm_modules copy), + # otherwise the install_path itself. + resolved_source_path = self._compute_dep_source_path( + dep_ref, install_path, parent_pkg + ) + # Look for apm.yml in the install path apm_yml_path = install_path / "apm.yml" if not apm_yml_path.exists(): @@ -420,22 +446,56 @@ def _try_load_dependency_package( name=dep_ref.get_display_name(), version="1.0.0", source=dep_ref.repo_url, - package_path=install_path + package_path=install_path, + source_path=resolved_source_path, ) # No manifest found return None - + # Load and return the package try: package = APMPackage.from_apm_yml(apm_yml_path) # Ensure source is set for tracking if not package.source: package.source = dep_ref.repo_url + # Set source_path so transitive resolution of THIS package's + # local-path deps anchors on the right directory. + package.source_path = resolved_source_path return package except (ValueError, FileNotFoundError) as e: # Package has invalid apm.yml - log warning but continue # In production, we might want to surface this to the user return None + + def _compute_dep_source_path( + self, + dep_ref: DependencyReference, + install_path: Path, + parent_pkg: Optional[APMPackage], + ) -> Path: + """Return the on-disk anchor for resolving *dep_ref*'s own local deps. + + For local dependencies this is the *original* directory the user + referenced (so relative paths inside the package's apm.yml mean what + a developer reading the file would expect). For remote deps it is + ``install_path`` (the clone location), which is also where the + package's apm.yml lives. + """ + if dep_ref.is_local and dep_ref.local_path: + local = Path(dep_ref.local_path).expanduser() + if local.is_absolute(): + return local.resolve() + base_dir = ( + parent_pkg.source_path + if parent_pkg is not None and parent_pkg.source_path is not None + else self._project_root + ) + if base_dir is None: + # Fallback: best-effort relative-to-cwd. Rare; only hit if + # the resolver was driven without a project root. + return local.resolve() + return (base_dir / local).resolve() + return install_path.resolve() def _create_resolution_summary(self, graph: DependencyGraph) -> str: """ diff --git a/src/apm_cli/install/phases/local_content.py b/src/apm_cli/install/phases/local_content.py index 1cf73cdf9..fdbe2acb1 100644 --- a/src/apm_cli/install/phases/local_content.py +++ b/src/apm_cli/install/phases/local_content.py @@ -75,13 +75,16 @@ 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, logger=None): """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 + 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). logger: Optional CommandLogger for structured output Returns: @@ -91,7 +94,7 @@ def _copy_local_package(dep_ref, install_path, project_root, logger=None): local = Path(dep_ref.local_path).expanduser() if not local.is_absolute(): - local = (project_root / local).resolve() + local = (base_dir / local).resolve() else: local = local.resolve() diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index f6118ca78..e8f35dd2c 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -119,7 +119,7 @@ def run(ctx: "InstallContext") -> None: logger = ctx.logger verbose = ctx.verbose - 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: @@ -127,6 +127,13 @@ def download_callback(dep_ref, modules_dir, parent_chain=""): 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: The APMPackage that declared *dep_ref* (None for + direct deps from the root project). For local deps, the + parent's ``source_path`` becomes the base directory for + resolving the relative path -- so a transitive dep like + ``../sibling`` declared inside a package nested several + directories deep resolves against the package's own + location, not the root consumer (#857). """ install_path = dep_ref.get_install_path(modules_dir) if install_path.exists(): @@ -140,8 +147,17 @@ 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 the source_path field. + 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, logger=logger ) if result_path: callback_downloaded[dep_ref.get_unique_key()] = None diff --git a/src/apm_cli/models/apm_package.py b/src/apm_cli/models/apm_package.py index 48e1ea4d7..9f2e10fb3 100644 --- a/src/apm_cli/models/apm_package.py +++ b/src/apm_cli/models/apm_package.py @@ -73,6 +73,12 @@ class APMPackage: dev_dependencies: Optional[Dict[str, List[Union[DependencyReference, str, dict]]]] = None scripts: Optional[Dict[str, str]] = None package_path: Optional[Path] = None # Local path to package + # Absolute on-disk directory holding this package's apm.yml. Used to + # resolve relative ``local_path`` dependencies declared in this package's + # apm.yml (#857). For local deps this is the *original* source directory, + # not the apm_modules/_local/ copy. For remote deps and the root project + # this matches package_path. + source_path: Optional[Path] = None target: Optional[Union[str, List[str]]] = None # Target agent(s): single string or list (applies to compile and install) type: Optional[PackageContentType] = None # Package content type: instructions, skill, hybrid, or prompts includes: Optional[Union[str, List[str]]] = None # Include-only manifest: 'auto' or list of repo paths diff --git a/tests/test_apm_resolver.py b/tests/test_apm_resolver.py index 214deaee4..3a1248894 100644 --- a/tests/test_apm_resolver.py +++ b/tests/test_apm_resolver.py @@ -493,5 +493,120 @@ def test_dependency_graph_error_handling(self): assert not summary["is_valid"] +class TestSourcePathField(unittest.TestCase): + """APMPackage.source_path field exists and is independent of package_path.""" + + def test_source_path_default_is_none(self): + pkg = APMPackage(name="x", version="1.0.0") + assert pkg.source_path is None + + def test_source_path_can_be_set(self): + pkg = APMPackage(name="x", version="1.0.0", source_path=Path("/some/dir")) + assert pkg.source_path == Path("/some/dir") + + +class TestComputeDepSourcePath(unittest.TestCase): + """``_compute_dep_source_path`` anchors local-relative deps correctly (#857).""" + + def setUp(self): + self.resolver = APMDependencyResolver() + self.resolver._project_root = Path("/proj") + + def _local_ref(self, local_path: str): + return DependencyReference.parse(local_path) + + def test_remote_dep_returns_install_path(self): + ref = DependencyReference(repo_url="user/repo", is_local=False) + install = Path("/proj/apm_modules/user/repo") + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=None) + assert result == install.resolve() + + def test_local_absolute_path_returns_resolved(self): + ref = self._local_ref("/abs/path/pkg") + install = Path("/proj/apm_modules/_local/pkg") + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=None) + assert result == Path("/abs/path/pkg").resolve() + + def test_local_relative_path_uses_parent_source_path(self): + # The bug: ``../editorial-pipeline`` declared inside a transitive + # package must resolve against that package's directory, not the + # root consumer. + ref = self._local_ref("../editorial-pipeline") + install = Path("/proj/apm_modules/_local/editorial-pipeline") + parent = APMPackage( + name="handbook-agents", + version="1.0.0", + source_path=Path("/proj/packages/handbook-agents"), + ) + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=parent) + assert result == Path("/proj/packages/editorial-pipeline").resolve() + + def test_local_relative_path_falls_back_to_project_root_when_no_parent(self): + # Direct deps from root: parent_pkg is None, anchor is project_root. + ref = self._local_ref("./packages/foo") + install = Path("/proj/apm_modules/_local/foo") + result = self.resolver._compute_dep_source_path(ref, install, parent_pkg=None) + assert result == Path("/proj/packages/foo").resolve() + + def test_local_relative_path_falls_back_to_project_root_when_parent_has_no_source_path(self): + # Backwards compat: a parent package created before source_path + # was added (still None) shouldn't break resolution -- fall back + # to the project root rather than crashing. + ref = self._local_ref("./packages/foo") + install = Path("/proj/apm_modules/_local/foo") + legacy_parent = APMPackage(name="legacy", version="1.0.0") + result = self.resolver._compute_dep_source_path( + ref, install, parent_pkg=legacy_parent + ) + assert result == Path("/proj/packages/foo").resolve() + + +class TestResolverSetsRootSourcePath(unittest.TestCase): + """``resolve_dependencies`` populates ``source_path`` on the root package.""" + + def test_root_package_has_source_path_after_resolve(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + ) + graph = APMDependencyResolver().resolve_dependencies(project_root) + assert graph.root_package.source_path == project_root.resolve() + + +class TestDownloadCallbackReceivesParent(unittest.TestCase): + """The download callback is invoked with ``parent_pkg`` for transitive deps.""" + + def test_callback_called_with_parent_package(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/dep1\n" + ) + + received_calls = [] + + def fake_download(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + received_calls.append((dep_ref.get_unique_key(), parent_pkg)) + return None # Simulate download miss; we only care about args + + resolver = APMDependencyResolver( + apm_modules_dir=apm_modules, + download_callback=fake_download, + ) + resolver.resolve_dependencies(project_root) + + # The first (and only) download attempt is for the direct dep. + # parent_pkg is None for direct deps -- the assert below pins + # the contract that the callback receives the parameter (and + # would receive a real parent for any transitive deps). + assert received_calls, "download_callback was never invoked" + _, parent_pkg = received_calls[0] + assert parent_pkg is None # direct dep from root + + if __name__ == '__main__': unittest.main() \ No newline at end of file diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 728d723a4..145f281ad 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -454,10 +454,11 @@ def test_download_callback_includes_chain_in_error(self, tmp_path): # Track what the callback receives callback_calls = [] - def tracking_callback(dep_ref, mods_dir, parent_chain=""): + def tracking_callback(dep_ref, mods_dir, parent_chain="", parent_pkg=None): callback_calls.append({ "dep": dep_ref.get_display_name(), "parent_chain": parent_chain, + "parent_pkg": parent_pkg, }) if "leaf-pkg" in dep_ref.get_display_name(): # Simulate what the real callback does: catch internal error, From c568b9d612b750093025575e13e771f1770b82c7 Mon Sep 17 00:00:00 2001 From: Jahanzaib Tayyab Date: Sun, 26 Apr 2026 11:34:29 +0500 Subject: [PATCH 2/2] fix(resolver): address Copilot review on transitive local path resolution (#940) - Detect whether download_callback accepts parent_pkg via signature inspection. Legacy 3-arg callbacks no longer raise a silent TypeError that would mask the dependency; the resolver gracefully falls back to the pre-#857 call shape and pre-#857 resolution semantic. - Disambiguate the per-resolution download dedup key for local deps: the literal local_path string can refer to different on-disk packages depending on the declaring parent's source_path, so include the resolved absolute source path in the cache key (`local::`). Remote deps are unchanged. - Update docs/guides/dependencies.md to reflect the new resolution semantic ("relative to this apm.yml's directory") and add a bullet explaining how relative paths in nested packages compose. - Tests: - Transitive case where parent_pkg is non-None and source_path is set (mid-pkg declares user/leaf-dep; assert callback receives mid-pkg). - Legacy callback compatibility: 3-arg callable still invoked. - Signature detection: modern, legacy, and **kwargs callables. - Local dedup key separation: same local_path under different parents produces distinct keys. --- docs/src/content/docs/guides/dependencies.md | 5 +- src/apm_cli/deps/apm_resolver.py | 86 +++++++++++- tests/test_apm_resolver.py | 140 +++++++++++++++++++ 3 files changed, 222 insertions(+), 9 deletions(-) diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index 2810bb47c..f6543aeb6 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -96,7 +96,7 @@ dependencies: - gitlab.com/acme/repo/prompts/code-review.prompt.md # Local path (for development / monorepo workflows) - - ./packages/my-shared-skills # relative to project root + - ./packages/my-shared-skills # relative to this apm.yml's directory - /home/user/repos/my-ai-package # absolute path # Object format: git URL + sub-path / ref / alias @@ -286,7 +286,7 @@ Or declare them in `apm.yml`: ```yaml dependencies: apm: - - ./packages/my-shared-skills # relative to project root + - ./packages/my-shared-skills # relative to this apm.yml's directory - /home/user/repos/my-ai-package # absolute path - microsoft/apm-sample-package # remote (can be mixed) ``` @@ -296,6 +296,7 @@ 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) +- **Relative paths anchor on the declaring `apm.yml`** -- a local dep listed inside a nested package (e.g. `../sibling-package` declared in `packages/foo/apm.yml`) resolves against that package's directory, not the root consumer. This matches every other package manager (npm, pip, cargo) and lets sibling packages compose. Use absolute paths if you want anchor-independent behavior. **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. diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index f4376d648..abbe09591 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -1,5 +1,6 @@ """APM dependency resolution engine with recursive resolution and conflict detection.""" +import inspect from pathlib import Path from typing import List, Set, Optional, Protocol, Tuple, runtime_checkable from collections import deque @@ -51,7 +52,33 @@ def __init__( self._apm_modules_dir: Optional[Path] = apm_modules_dir self._project_root: Optional[Path] = None self._download_callback = download_callback + # Whether ``download_callback`` accepts ``parent_pkg`` (added in #857). + # Detected once via signature inspection so legacy callbacks that + # predate the field still work without raising a silent TypeError + # that would mask the dependency. + self._callback_accepts_parent_pkg: bool = ( + self._signature_accepts_parent_pkg(download_callback) + if download_callback is not None + else False + ) self._downloaded_packages: Set[str] = set() # Track what we downloaded during this resolution + + @staticmethod + def _signature_accepts_parent_pkg(callback) -> bool: + """Return True if ``callback`` declares a ``parent_pkg`` parameter + (or accepts ``**kwargs``). Falls back to True if the signature can't + be introspected (e.g. C extensions) so the modern path is preferred. + """ + try: + sig = inspect.signature(callback) + except (TypeError, ValueError): + return True + for param in sig.parameters.values(): + if param.kind is inspect.Parameter.VAR_KEYWORD: + return True + if param.name == "parent_pkg": + return True + return False def resolve_dependencies(self, project_root: Path) -> DependencyGraph: """ @@ -406,16 +433,32 @@ def _try_load_dependency_package( # If package doesn't exist locally, try to download it if not install_path.exists(): if self._download_callback is not None: - unique_key = dep_ref.get_unique_key() + # For local deps, dedupe by the *resolved* on-disk path so that + # the same literal (e.g. ``../common``) declared by two + # different parents does not collapse onto a single graph + # node when each parent's source dir actually points to a + # different sibling (#857). + unique_key = self._download_dedup_key(dep_ref, parent_pkg) # Avoid re-downloading the same package in a single resolution if unique_key not in self._downloaded_packages: try: - downloaded_path = self._download_callback( - dep_ref, - self._apm_modules_dir, - parent_chain, - parent_pkg, - ) + if self._callback_accepts_parent_pkg: + downloaded_path = self._download_callback( + dep_ref, + self._apm_modules_dir, + parent_chain, + parent_pkg, + ) + else: + # Legacy callback predating #857 -- no parent_pkg. + # Local deps from non-root parents will fall back + # to project_root resolution; this matches behavior + # before the fix, so no surprise regression. + downloaded_path = self._download_callback( + dep_ref, + self._apm_modules_dir, + parent_chain, + ) if downloaded_path and downloaded_path.exists(): self._downloaded_packages.add(unique_key) install_path = downloaded_path @@ -467,6 +510,35 @@ def _try_load_dependency_package( # In production, we might want to surface this to the user return None + def _download_dedup_key( + self, + dep_ref: DependencyReference, + parent_pkg: Optional[APMPackage], + ) -> str: + """Return a context-aware key for the per-resolution download cache. + + For remote deps this is just ``dep_ref.get_unique_key()``. For local + deps the literal ``local_path`` (e.g. ``../common``) is ambiguous + once #857 anchors it on the declaring package's directory: the same + string can refer to different on-disk packages depending on which + parent declared it. We therefore prefix the key with the resolved + absolute source path so the dedup matches the actual filesystem + identity, not the textual identity. + """ + if dep_ref.is_local and dep_ref.local_path: + local = Path(dep_ref.local_path).expanduser() + if local.is_absolute(): + resolved = local.resolve() + else: + base_dir = ( + parent_pkg.source_path + if parent_pkg is not None and parent_pkg.source_path is not None + else self._project_root + ) + resolved = (base_dir / local).resolve() if base_dir else local.resolve() + return f"local::{resolved}" + return dep_ref.get_unique_key() + def _compute_dep_source_path( self, dep_ref: DependencyReference, diff --git a/tests/test_apm_resolver.py b/tests/test_apm_resolver.py index 3a1248894..c440adbaa 100644 --- a/tests/test_apm_resolver.py +++ b/tests/test_apm_resolver.py @@ -607,6 +607,146 @@ def fake_download(dep_ref, modules_dir, parent_chain="", parent_pkg=None): _, parent_pkg = received_calls[0] assert parent_pkg is None # direct dep from root + def test_callback_receives_parent_for_transitive_dep(self): + """Transitive deps invoke the callback with the *declaring* package + as ``parent_pkg`` so its ``source_path`` can anchor relative local + paths (#857).""" + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + + # Root depends on a local sibling that itself declares a + # transitive remote dep. We satisfy the local dep by hand so + # its apm.yml is loaded and its sub-deps get resolved (which + # is when the callback should fire with parent_pkg=mid-pkg). + mid_install = apm_modules / "_local" / "mid" + mid_install.mkdir(parents=True) + (mid_install / "apm.yml").write_text( + "name: mid-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/leaf-dep\n" + ) + + mid_src = project_root / "packages" / "mid" + mid_src.mkdir(parents=True) + (mid_src / "apm.yml").write_text( + "name: mid-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/leaf-dep\n" + ) + + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - ./packages/mid\n" + ) + + received_calls = [] + + def fake_download(dep_ref, apm_modules_dir, parent_chain="", parent_pkg=None): + received_calls.append( + (dep_ref.get_unique_key(), parent_pkg, parent_chain) + ) + return None + + resolver = APMDependencyResolver( + apm_modules_dir=apm_modules, + download_callback=fake_download, + ) + resolver.resolve_dependencies(project_root) + + # The transitive ``user/leaf-dep`` invocation must carry the + # mid-pkg APMPackage as parent_pkg. + leaf_calls = [c for c in received_calls if "leaf-dep" in c[0]] + assert leaf_calls, ( + f"expected a callback call for the transitive leaf-dep, " + f"got: {received_calls}" + ) + _, parent_pkg, _ = leaf_calls[0] + assert parent_pkg is not None, "transitive dep should have parent_pkg" + assert parent_pkg.name == "mid-pkg" + # source_path must be set so future relative resolution would work. + assert parent_pkg.source_path is not None + + +class TestLegacyDownloadCallbackCompatibility(unittest.TestCase): + """Callbacks that predate #857 (no ``parent_pkg`` parameter) still work.""" + + def test_legacy_callback_signature_is_called(self): + with TemporaryDirectory() as temp_dir: + project_root = Path(temp_dir) + apm_modules = project_root / "apm_modules" + apm_modules.mkdir() + (project_root / "apm.yml").write_text( + "name: root-pkg\nversion: 1.0.0\n" + "dependencies:\n apm:\n - user/dep1\n" + ) + + invocations = [] + + # Legacy 3-arg callback -- no ``parent_pkg``. Pre-#857 this is + # the only signature that exists; if the resolver tried to call + # with a 4th positional arg it would raise TypeError and the + # download would be silently skipped. + def legacy_download(dep_ref, apm_modules_dir, parent_chain=""): + invocations.append(dep_ref.get_unique_key()) + return None + + resolver = APMDependencyResolver( + apm_modules_dir=apm_modules, + download_callback=legacy_download, # type: ignore[arg-type] + ) + resolver.resolve_dependencies(project_root) + + assert invocations, ( + "legacy callback should still be invoked; resolver must " + "detect missing parent_pkg parameter via signature inspection" + ) + + def test_modern_callback_detected_as_parent_pkg_aware(self): + def modern(dep_ref, apm_modules_dir, parent_chain="", parent_pkg=None): + return None + assert APMDependencyResolver._signature_accepts_parent_pkg(modern) is True + + def test_legacy_callback_detected_as_not_parent_pkg_aware(self): + def legacy(dep_ref, apm_modules_dir, parent_chain=""): + return None + assert APMDependencyResolver._signature_accepts_parent_pkg(legacy) is False + + def test_kwargs_callback_detected_as_parent_pkg_aware(self): + def varargs(dep_ref, apm_modules_dir, parent_chain="", **kwargs): + return None + assert APMDependencyResolver._signature_accepts_parent_pkg(varargs) is True + + +class TestDownloadDedupKey(unittest.TestCase): + """Per-resolution download dedup key disambiguates identical local_path + literals declared by different parents (#857).""" + + def test_remote_dep_uses_get_unique_key(self): + resolver = APMDependencyResolver() + dep = DependencyReference(repo_url="user/repo") + assert resolver._download_dedup_key(dep, parent_pkg=None) == dep.get_unique_key() + + def test_local_dep_keys_include_resolved_path(self): + resolver = APMDependencyResolver() + dep = DependencyReference( + repo_url="../common", is_local=True, local_path="../common" + ) + parent_a = APMPackage( + name="a", version="1.0.0", source="local", + source_path=Path("/proj/packages/team-x/handbook").resolve(), + ) + parent_b = APMPackage( + name="b", version="1.0.0", source="local", + source_path=Path("/proj/packages/team-y/handbook").resolve(), + ) + key_a = resolver._download_dedup_key(dep, parent_pkg=parent_a) + key_b = resolver._download_dedup_key(dep, parent_pkg=parent_b) + assert key_a != key_b, ( + "same local_path literal under different parents must dedup separately" + ) + assert "team-x" in key_a + assert "team-y" in key_b + if __name__ == '__main__': unittest.main() \ No newline at end of file