fix: resolve transitive local paths against declaring package (#857)#940
fix: resolve transitive local paths against declaring package (#857)#940JahanzaibTayyab wants to merge 2 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
|
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.