fix: resolve transitive local paths against declaring package (#857)#940
fix: resolve transitive local paths against declaring package (#857)#940JahanzaibTayyab wants to merge 5 commits intomicrosoft:mainfrom
Conversation
…oft#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.
JahanzaibTayyab
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
Fixes transitive local dependency resolution so relative local_path entries in a package's apm.yml resolve relative to the declaring package directory (not the root consumer), enabling sibling local package composition.
Changes:
- Add
APMPackage.source_pathto track the on-disk directory that owns a package'sapm.yml. - Extend the resolver download callback to receive
parent_pkgand compute the correct base directory for resolving transitive local paths. - Add unit tests covering
source_pathand source-path computation behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_install_command.py | Updates a resolver callback mock signature to accept the new parent_pkg argument. |
| tests/test_apm_resolver.py | Adds tests for APMPackage.source_path, _compute_dep_source_path, and callback argument threading. |
| src/apm_cli/models/apm_package.py | Introduces source_path field on APMPackage to anchor relative local path resolution. |
| src/apm_cli/install/phases/resolve.py | Uses parent_pkg.source_path (when available) to resolve transitive local paths against the declaring package. |
| src/apm_cli/install/phases/local_content.py | Renames _copy_local_package parameter project_root -> base_dir and updates resolution accordingly. |
| src/apm_cli/deps/apm_resolver.py | Threads parent_pkg through resolution, sets source_path, and adds _compute_dep_source_path. |
…tion (microsoft#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-microsoft#857 call shape and pre-microsoft#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::<resolved>`). 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.
|
@microsoft-github-policy-service agree |
|
Thanks for the careful Copilot review. Pushed 1. Silent TypeError on legacy callbacks ( 2. Docs ( 3. Transitive callback test ( 4. Local dedup key collision ( Also added a Full suite: Also signed the CLA above. The pre-existing concerns called out in the PR description (lockfile entries for transitive local deps, |
APM Review Panel VerdictDisposition: REQUEST_CHANGES (two pre-merge doc/admin actions; code and tests are solid) Per-persona findingsPython Architect: This is a routine PR -- bug fix in the dependency resolution engine with backward-compat introspection and a new optional field on a dataclass. Not a major architectural change; one class diagram + one flow diagram. 1. OO / class diagramclassDiagram
direction TB
class APMDependencyResolver {
<<CoreEngine>>
+_project_root: Path
+_download_callback: DownloadCallback
+_callback_accepts_parent_pkg: bool
+resolve_dependencies(project_root) DependencyGraph
+_try_load_dependency_package(dep_ref, parent_chain, parent_pkg) APMPackage
+_compute_dep_source_path(dep_ref, install_path, parent_pkg) Path
+_download_dedup_key(dep_ref, parent_pkg) str
+_signature_accepts_parent_pkg(callback) bool
}
class APMPackage {
<<ValueObject>>
+name: str
+version: str
+package_path: Optional[Path]
+source_path: Optional[Path]
+dependencies: Dict
+from_apm_yml(path) APMPackage
}
class DownloadCallback {
<<Protocol>>
+__call__(dep_ref, apm_modules_dir, parent_chain, parent_pkg) Optional[Path]
}
class DependencyReference {
+repo_url: str
+is_local: bool
+local_path: str
+get_install_path(apm_modules_dir) Path
+get_unique_key() str
}
class DependencyGraph {
+root_package: APMPackage
+dependencies: List
}
class DependencyNode {
+package: APMPackage
+get_ancestor_chain() str
}
APMDependencyResolver *-- DownloadCallback : invokes
APMDependencyResolver ..> APMPackage : creates/reads
APMDependencyResolver ..> DependencyReference : reads
APMDependencyResolver ..> DependencyGraph : produces
APMDependencyResolver ..> DependencyNode : traverses
DownloadCallback ..> APMPackage : receives parent_pkg
DependencyGraph *-- APMPackage : root_package
DependencyNode *-- APMPackage : package
note for APMDependencyResolver "Adapter: _signature_accepts_parent_pkg\ndetects legacy callbacks at init time"
class APMDependencyResolver:::touched
class APMPackage:::touched
class DownloadCallback:::touched
classDef touched fill:#fff3b0,stroke:#d47600
2. Execution flow diagramflowchart TD
A["apm install (CLI)"] --> B["resolve.run(ctx)\nsrc/apm_cli/install/phases/resolve.py"]
B --> C["APMDependencyResolver.resolve_dependencies(project_root)\nsrc/apm_cli/deps/apm_resolver.py"]
C --> D["[I/O] APMPackage.from_apm_yml(apm_yml_path)\nroot_package.source_path = project_root.resolve()"]
D --> E["build_dependency_tree: BFS deque of DependencyNodes"]
E --> F{"next dep_ref in queue?"}
F -->|yes| G["_try_load_dependency_package(dep_ref, parent_chain,\nparent_pkg=parent_node.package)"]
G --> H{"install_path.exists()?"}
H -->|no| I{"_callback_accepts_parent_pkg?"}
I -->|yes| J["download_callback(dep_ref, modules_dir,\nparent_chain, parent_pkg)\nresolve.py closure"]
I -->|no| K["legacy: download_callback(dep_ref, modules_dir, parent_chain)\nsilent fallback to project_root anchor"]
J --> L{"parent_pkg.source_path set?"}
L -->|yes| M["[FS] _copy_local_package(dep_ref, install_path,\nbase_dir=parent_pkg.source_path)\nlocal_content.py"]
L -->|no| N["[FS] _copy_local_package(dep_ref, install_path,\nbase_dir=project_root)\nlocal_content.py"]
H -->|yes| O["_compute_dep_source_path(dep_ref, install_path, parent_pkg)"]
M --> O
N --> O
O --> P{"dep_ref.is_local?"}
P -->|absolute| Q["return local.resolve()"]
P -->|relative + parent.source_path| R["return (parent.source_path / local).resolve()"]
P -->|relative + no parent| S["return (project_root / local).resolve()"]
P -->|remote dep| T["return install_path.resolve()"]
Q & R & S & T --> U["package.source_path = resolved_source_path\nrecurse into package's own deps"]
U --> F
F -->|empty| V["[LOCK] produce DependencyGraph"]
```
#### 3. Design patterns
**Design patterns**
- Used in this PR: Adapter -- `_signature_accepts_parent_pkg` (static method on `APMDependencyResolver`) detects legacy 3-arg callback signatures at construction time so the modern 4-arg path is preferred and the legacy path degrades gracefully without a TypeError surface. Dataclass-as-value-object -- `APMPackage.source_path` carries the resolved anchor immutably through the BFS traversal.
- Pragmatic suggestion: The base_dir computation `(parent_pkg.source_path if parent_pkg is not None and parent_pkg.source_path is not None else self._project_root)` is duplicated verbatim in `_download_dedup_key` and `_compute_dep_source_path`. Extract to a one-liner helper method `_effective_base_dir(parent_pkg)` to eliminate the two-site duplication. The refactor is four lines; the benefit is a single place to change if the fallback logic evolves.
**Assessment**: Architecture is clean and appropriately scoped. The BFS parent-node forwarding (`parent_node.package if parent_node else None`) is the idiomatic way to thread context through the tree walk. The `inspect.signature` detection is a well-understood backward-compat pattern. The `source_path` field is the right place on `APMPackage`. One minor concern: `_signature_accepts_parent_pkg` returns `True` for unresolvable signatures (C extensions), which means the resolver will attempt the 4-arg call and fall into the `except Exception: pass` in `_try_load_dependency_package`, silently dropping the dependency. This is pre-existing behavior for the exception-swallow; the True default is the right bias. No blocker.
---
**CLI Logging Expert**:
The PR adds no new user-facing log output, STATUS_SYMBOLS, CommandLogger methods, or diagnostic messages. The `_copy_local_package` function receives its `logger` parameter and passes it through unchanged -- correct.
One pre-existing note (not introduced by this PR): the `except Exception: pass` in `_try_load_dependency_package` swallows download failures silently. A verbose-mode log of the caught exception at `[!]` level would help users debug silent dep-miss failures. This is pre-existing technical debt; flagging for follow-up only. No output-path misuse, no direct `_rich_*` calls outside console code. CLEAN.
---
**DevX UX Expert**:
The fix aligns APM with npm, pip, cargo, and go module semantics for relative paths -- exactly the right call. A monorepo user who hit this bug will now find APM behaves as expected without reading docs.
**Positive**: The doc comment update (`"relative to this apm.yml's directory"`) and the bolded note in `dependencies.md` are clear and accurate. The `inspect.signature` mitigation means existing callback authors are not silently broken at runtime.
**Required (Rule 4)**: `packages/apm-guide/.apm/skills/apm-usage/dependencies.md` is NOT updated. The local-path section of that file currently describes paths as resolving relative to the project root. The behavioral change (anchor on declaring `apm.yml`) must be reflected there so agents that consume the skill resource give users correct guidance. The update is one sentence.
**Required (repo convention)**: `CHANGELOG.md` has no entry for this fix. Per repo changelog convention, every merged PR that changes behavior must have a "Fixed" entry. For a `DownloadCallback` Protocol signature change (even with compat shim), a brief note is owed to callback authors. Suggested entry:
```
### Fixed
- Relative `local_path` dependencies declared in nested packages now resolve
against the declaring package's directory instead of the project root,
matching npm/pip/cargo semantics; enables sibling-package composition in
monorepos (#857) -- by @JahanzaibTayyab (#940)
Supply Chain Security Expert: Path traversal surface: New exposure introduced by this PR: Dedup key: Backward-compat shim: Fails-closed posture: No change -- local path resolution fails closed (missing dir, missing apm.yml, or missing SKILL.md all return None or produce an error log). Auth Expert: Not activated -- PR modifies only local dependency path resolution in OSS Growth Hacker: Story angle: "APM now resolves local deps the same way npm, pip, and cargo do." This is a high-value one-liner for the release notes -- it directly addresses a pain point that would cause monorepo users to churn. The fix enables composable multi-agent pipelines (e.g. External contributor (@JahanzaibTayyab) deserves visible credit in the CHANGELOG. External contributor PRs are the most powerful OSS signal -- highlighting them in release notes compounds contributor acquisition. Side-channel note to CEO: This fix is worth a dedicated mention in the next release post. The before/after monorepo repro in the PR description is already release-post-ready. Consider it as evidence for a "monorepo-native" positioning beat. CEO arbitrationThe specialist panel is aligned. The code change is correct, clean, well-tested (9 new test cases covering the bug path, backward compat, and edge cases), and matches the semantic contract of every major package manager. The Two administrative gaps block merge: a missing Required actions before merge
Optional follow-ups
|
danielmeppiel
left a comment
danielmeppiel
left a comment
There was a problem hiding this comment.
Required actions before merge
Add a CHANGELOG.md entry under ## [Unreleased] > ### Fixed describing the transitive local-path anchor fix with credit to @JahanzaibTayyab. See the suggested entry in the DevX section above.
Update packages/apm-guide/.apm/skills/apm-usage/dependencies.md local-path section to note that relative paths anchor on the declaring apm.yml's directory, not the project root. One sentence aligned with the dependencies.md doc update already in this PR.
Extract _effective_base_dir(parent_pkg) helper method on APMDependencyResolver to eliminate the duplicated parent_pkg.source_path or self._project_root logic in _download_dedup_key and _compute_dep_source_path.
Open a follow-up security hardening issue to add path_security.validate_path_segments(dep_ref.local_path) and ensure_path_within(local, project_root) guards in _copy_local_package, covering both the pre-existing gap and the slightly widened surface from base_dir variability.
Add a verbose-mode log ([!] level) in the except Exception: pass block of _try_load_dependency_package to surface silent download failures when --verbose is active.
Consider explicitly rejecting local_path declarations found inside remotely-fetched packages (not user-controlled local deps), as they are semantically surprising and constitute the most realistic path-traversal vector.
…crosoft#940) Fold all required actions from review #4191366889 into this PR: - Extract _effective_base_dir(parent_pkg) helper on APMDependencyResolver so _download_dedup_key and _compute_dep_source_path share one rule for parent-or-project-root anchoring. - Reject relative local_path declared inside remotely-fetched packages (the most plausible path-traversal vector); direct deps from the root project and absolute paths are unaffected. New _is_remote_parent helper checks containment under apm_modules_dir. - Surface previously silent transitive download failures via the stdlib logger so --verbose makes the skip diagnosable; partial resolution still works. - Harden _copy_local_package: optional project_root parameter triggers ensure_path_within(local, project_root) so a transitive base_dir cannot widen the surface to paths outside the project. (We deliberately do not call validate_path_segments on local_path because it would reject the legitimate one-level-up form ../sibling that this PR enables; ensure_path_within is the actual security boundary.) - CHANGELOG: add Fixed entry under [Unreleased] crediting @JahanzaibTayyab (microsoft#857). - packages/apm-guide/.apm/skills/apm-usage/dependencies.md: document the declaring-apm.yml anchoring rule and the rejection of local_path in remote packages. - Tests: cover _effective_base_dir, the remote-parent rejection guard, the verbose-mode warning on download failure, and the ensure_path_within containment guard in _copy_local_package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added
panel-review
APM Review Panel Verdict: REJECT
Required before merge (7 items)
Nits (13 items, skip if you want)
CEO arbitrationFive active panelists reviewed PR #940 and collectively surfaced 7 required findings. All agree the core fix -- anchoring transitive Three findings carry the highest strategic weight and should be addressed together. First, python-architect identifies that The fix path is clear: make the cache key include Dissent resolved: No panelist disagreed on severity classification. The CHANGELOG migration finding was raised independently as REQUIRED by both devx-ux-expert and oss-growth-hacker, which reinforces rather than divides. The distinction between the two is framing: devx-ux-expert argues from user actionability, oss-growth-hacker from community retention. Both are right; the fix satisfies both. No CEO override needed. Growth/positioning note: oss-growth-hacker notes this fix delivers what every monorepo user has wanted: predictable sibling-package resolution matching npm, pip, and cargo semantics. Recommended release framing: "APM now plays by the same rules as every other package manager -- your monorepo just works." A 30-second GIF of a two-package monorepo resolving cleanly would make this repostable. The path-containment security hardening is a quiet trust signal worth a one-liner in the release post, especially for enterprise evaluators. Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class DownloadCallback {
<<Protocol / Strategy>>
+__call__(dep_ref, apm_modules_dir, parent_chain, parent_pkg) Optional~Path~
}
note for DownloadCallback "Strategy: injectable download\nbehavior; legacy callbacks\ndetected via signature inspection"
class APMPackage {
<<Dataclass / ValueObject>>
+name str
+version str
+source_path Optional~Path~
+package_path Optional~Path~
}
note for APMPackage "source_path set POST-CONSTRUCTION\non cached instance -- cache risk"
class _copy_local_package {
<<Pure / IOBoundary>>
+__call__(dep_ref, install_path, base_dir, logger, project_root) Optional~Path~
}
class APMDependencyResolver {
<<Strategy Host>>
+_download_callback Optional~DownloadCallback~
+_callback_accepts_parent_pkg bool
+resolve_dependencies(project_root) DependencyGraph
+build_dependency_tree(root_apm_yml) DependencyTree
+_try_load_dependency_package(dep_ref, parent_chain, parent_pkg) Optional~APMPackage~
+_is_remote_parent(parent_pkg) bool
+_effective_base_dir(parent_pkg) Optional~Path~
+_download_dedup_key(dep_ref, parent_pkg) str
+_compute_dep_source_path(dep_ref, install_path, parent_pkg) Path
+_signature_accepts_parent_pkg(callback) bool
}
note for APMDependencyResolver "Strategy Host: download_callback\ninjected at construction;\nAdapter: _signature_accepts_parent_pkg\nbridges legacy to new Protocol"
class DependencyReference {
<<ValueObject>>
+is_local bool
+local_path Optional~str~
+repo_url Optional~str~
+get_install_path(modules_dir) Path
+get_unique_key() str
}
class DependencyNode {
+package APMPackage
+dependency_ref DependencyReference
+depth int
+parent Optional~DependencyNode~
+children List~DependencyNode~
+get_ancestor_chain() str
}
class LocalDependencySource {
<<Strategy / ConcreteSource>>
+acquire() Optional~Materialization~
}
class DownloadCallback:::touched
class APMPackage:::touched
class _copy_local_package:::touched
class APMDependencyResolver:::touched
class LocalDependencySource:::touched
APMDependencyResolver *-- DownloadCallback : injects Strategy
APMDependencyResolver ..> APMPackage : produces and mutates source_path
APMDependencyResolver ..> DependencyReference : consumes
APMDependencyResolver o-- DependencyNode : traverses
DependencyNode *-- APMPackage : carries
DependencyNode *-- DependencyReference : carries
LocalDependencySource ..> _copy_local_package : delegates FS
LocalDependencySource ..> APMPackage : reads source_path
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A([apm install CLI]) --> B[phases/resolve.py run]
B --> C[APMDependencyResolver.__init__\n_signature_accepts_parent_pkg called once]
C --> D[resolve_dependencies\nproject_root captured]
D --> E[[FS] APMPackage.from_apm_yml\nroot apm.yml loaded and cached]
E --> F[[FS] root_package.source_path = project_root.resolve]
F --> G[build_dependency_tree BFS loop]
G --> H{dep at depth greater than 0?}
H -->|yes| I[_try_load_dependency_package\ndep_ref + parent_chain + parent_pkg]
H -->|no| G
I --> J{dep_ref.is_local AND\nnot absolute AND\n_is_remote_parent?}
J -->|yes -- REJECT| K[_logger.warning\nreturn None]
J -->|no -- ALLOW| L{install_path.exists?}
L -->|no| M{_download_callback set?}
M -->|yes| N{_callback_accepts_parent_pkg?}
N -->|True modern path| O[download_callback\ndep_ref modules_dir parent_chain parent_pkg]
N -->|False legacy path| P[download_callback\ndep_ref modules_dir parent_chain]
M -->|no| Q[return None]
O --> R{dep_ref.is_local?}
P --> R
R -->|yes| S[_effective_base_dir\nparent_pkg.source_path OR project_root]
S --> T[[FS] _copy_local_package\ndep_ref install_path base_dir project_root]
T --> T1{ensure_path_within\nlocal inside project_root?}
T1 -->|no -- PathTraversalError| U[log error return None]
T1 -->|yes| V[[FS] shutil.copytree to apm_modules/_local/name]
V --> W[_downloaded_packages key added]
R -->|no -- remote| X[[NET] GitHubPackageDownloader.download]
X --> W
L -->|yes| Y[_compute_dep_source_path\noriginal src dir for local -- install_path for remote]
W --> Y
Y --> Z[[FS] APMPackage.from_apm_yml install_path/apm.yml]
Z --> AA[package.source_path = resolved_source_path\nMUTATES cached APMPackage]
AA --> AB[node.package = loaded_package]
AB --> AC[enqueue sub-deps with parent_pkg=loaded_package]
AC --> G
Design patterns
Required:
Nits:
CLI Logging ExpertRequired:
Nits:
DevX UX ExpertRequired:
Nits:
Supply Chain Security ExpertRequired:
Nits:
Auth ExpertInactive -- No auth-specific files touched; changed files are apm_resolver.py, local_content.py, resolve.py, sources.py, apm_package.py, tests, docs -- none of which affect authentication, token management, credential resolution, or host classification. OSS Growth HackerRequired:
Nits:
Verdict computed deterministically: 7 required findings across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
added
panel-rejected
|
Superseded by #1111, which keeps your investigation, fix architecture, and tests intact (preserved via Closing in favor of #1111. |
…857) (#1111) * fix(install): anchor transitive local deps on declaring package (#857) A package's local_path references are now anchored on that package's own directory (npm/pip/cargo workspace parity), not on the consumer's project root. Previously a transitive '../sibling' declared inside packages/specialized/apm.yml resolved against the consumer's root, making mono-repos with sibling helper packages non-portable. Changes: - APMPackage.source_path tracks the on-disk dir each package was loaded from. Cache key is (apm_yml_path, source_path) so two loads with different anchors don't collide. - Resolver threads parent_pkg through the download_callback Protocol; legacy callbacks without the kwarg keep working via a signature introspection fallback (False on TypeError/ValueError). - Resolve phase persists a dep_key -> base_dir map to ctx so the integrate phase can copy transitive locals with the right anchor. - _copy_local_package now requires project_root (no silent skip) and enforces ensure_path_within with a red error + actionable hint on PathTraversalError. - Resolver dual-rejects local_path declared by a remote parent (relative AND absolute) at ERROR severity. - Download failures surface via _rich_warning with exc_info on a separate _logger.debug call (no more bare except: pass). - SECURITY comments on symlinks=True (preserved intentionally) and the TOCTOU race window. Tests: cache-key distinctness, source_path threading on from_apm_yml, _is_remote_parent _local/ exclusion, signature fallback, containment boundary on _copy_local_package. Closes #857. Supersedes #940. Co-authored-by: Jahanzaib Tayyab <JahanzaibTayyab@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(install): address PR #1111 review comments C1 (logger=None AttributeError): add NullCommandLogger fallback at the top of _copy_local_package; failed first attempt to default it pipeline- wide because NullCommandLogger lacks InstallLogger-specific methods like policy_discovery_miss used by the policy_gate phase. C2 (rejected remote-parent local_path still installed): track rejected dep keys on ApmDependencyResolver._rejected_remote_local_keys, then fold them into ctx.callback_failures in resolve phase so integrate.py's existing skip gate honors the rejection. C3 (dep_base_dirs key collision): detect divergent-anchor writes in the build loop, _logger.warning loudly, keep first-write semantics. Comment documents that collision is latent today (BFS dedupes by unique key) but the guard is defensive against future BFS changes. C4/C5 (false containment claim in docs): drop the 'must stay inside consuming project root' wording from both packages/apm-guide and docs/src/content/docs; clarify sibling layouts work and the security boundary is upstream remote-parent rejection. C6 (CHANGELOG entry too long, wrong PR number): condense the 5-clause Fixed entry to a single line ending '(#1111, closes #857) Thanks @JahanzaibTayyab.' Tests: TestRemoteParentLocalPathFailClosed (2 tests) covers C2; test_copy_local_package_logger_none_does_not_raise covers C1. Verified: ruff check + format silent; 7241 unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jahanzaib Tayyab <JahanzaibTayyab@users.noreply.github.com> Co-authored-by: Daniel Meppiel <copilot-rework@github.com>
Description
Fixes #857.
Relative
local_pathdependencies declared inside a non-root package'sapm.ymlwere 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.After this PR, relative paths in any
apm.ymlresolve against the directory containing thatapm.yml, matching the semantic every other package manager (npm, pip, cargo, go modules, Bicep) uses.Type of change
Repro
A monorepo with two local packages:
packages/handbook-agents/apm.yml:Before this PR:
apm installfails because APM resolves../editorial-pipelineagainst the root consumer (my-project/), looking formy-project/../editorial-pipeline.After this PR: APM resolves it against the directory holding the apm.yml that declared it (
packages/handbook-agents/), so it finds the sibling package.Implementation
The fix follows the design proposed in the issue:
APMPackagegainssource_path: Optional[Path] = None-- absolute on-disk directory holding this package's apm.yml. For the root project and for remote deps this matches the install path; for local deps it is the original source directory (not theapm_modules/_local/copy).DownloadCallbackProtocol gainsparent_pkg: Optional[APMPackage] = None-- the package that declared the dependency._try_load_dependency_packageacceptsparent_pkgand computes the loaded package'ssource_pathvia a small new helper_compute_dep_source_path. The caller inbuild_dependency_treealready has the parent node, so it just forwardsparent_node.package.download_callback(ininstall/phases/resolve.py) computesbase_dir = parent_pkg.source_path or project_rootand passes that to_copy_local_package._copy_local_package's third parameter renamedproject_root->base_dirto reflect the new semantic. Existing positional callers are unaffected.The root project's
source_pathis set toproject_rootimmediately afterAPMPackage.from_apm_yml()so direct deps work identically to today.Behavior matrix
./packages/fooproject_root✅project_root✅../sibling./internal-helperAreas needing careful review
Backwards compatibility on
DownloadCallback. The Protocol now has a fourth keyword-argparent_pkg. Any existing user-defined callback that doesn't accept it (and lacks**kwargs) will fail with aTypeErrorwhen called from_try_load_dependency_package. The resolver currently catches that as a generic download miss, so the failure is silent rather than loud. Two options if maintainers prefer different ergonomics:inspect.signaturein_try_load_dependency_packageto omitparent_pkgfor legacy callbacks (backwards-compatible, slightly hidden complexity).I went with the second since the issue explicitly proposed the Protocol change and the only in-tree caller is the one we control. Happy to switch.
Lockfile entries for transitive local deps.
lockfile.pyserialiseslocal_pathas a string. For transitive local deps, the string's anchor changes (relative-to-source vs. relative-to-root). A regenerate handles it; direct local deps are unaffected. I did not modify the lockfile schema, in line with the "Out of scope" section of the issue.Install-path collisions (
_local/<dirname>collision when two parents declare local deps with the same final folder name) is pre-existing and unchanged by this PR. The issue calls this out as orthogonal.Tests
7 new tests in
tests/test_apm_resolver.py:TestSourcePathField(2): default-None, settable.TestComputeDepSourcePath(5): remote dep returns install_path; local absolute resolved; local relative uses parent'ssource_path(the bug-fix path); local relative falls back toproject_rootwhen no parent; backwards-compat fallback when parent has nosource_path.TestResolverSetsRootSourcePath(1): rootsource_pathpopulated after resolve.TestDownloadCallbackReceivesParent(1):parent_pkgarg flows through to the callback.One existing test (
tests/unit/test_install_command.py::TestTransitiveDepParentChain::test_download_callback_includes_chain_in_error) was updated -- its mock callback signature gainedparent_pkg=Noneto match the extended Protocol.Testing
uv run pytest tests/unit tests/test_console.py: 5,466 passed, matches main minus the pre-existing f-string-in-test_audit_report.pycollection error which exists on main)Prepared with assistance from Claude (Anthropic) under human review.