fix: deploy root project .apm/ primitives without a sub-package stub#715
fix: deploy root project .apm/ primitives without a sub-package stub#715edenfunf wants to merge 4 commits intomicrosoft:mainfrom
Conversation
apm install silently ignored the project's own root-level .apm/ directory. Users with both external dependencies and local rules in root .apm/ were forced to create a dummy ./agent/apm.yml stub and reference it as a local dependency -- unnecessary boilerplate that the tool should handle natively. Root cause: _install_apm_dependencies() had two early-return guards that fired before target detection and integrator setup, so the project root's own .apm/ was never processed. The integration pipeline only ever ran against paths inside apm_modules/. Fix: detect root .apm/ subdirectories before the early returns so setup proceeds. After the dependency download loop, create a synthetic PackageInfo with install_path=project_root and run it through the same _integrate_package_primitives pipeline used for all other packages. Fixes microsoft#714
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Enables apm install to deploy primitives from the project root’s own .apm/ directory (and SKILL.md) even when there are no declared external APM dependencies, addressing #714.
Changes:
- Enter APM install flow when root-local primitives exist, skipping previous early returns that prevented integration.
- Integrate root project primitives via the existing
_integrate_package_primitivespipeline after dependency processing. - Add integration tests covering root
.apm/deployment, mixed root+external case, regression for the stub workaround, and idempotency.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/test_local_install.py | Adds integration tests asserting root-level .apm/ primitives are deployed (with/without deps) and behavior is idempotent. |
| src/apm_cli/commands/install.py | Updates install flow to detect root primitives, avoid early exits, and integrate root primitives as an implicit local package. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 3
Address review feedback on microsoft#715: - Remove _LOCAL_PRIM_SUBDIRS allowlist from both install() and _install_apm_dependencies(). The check now treats .apm/ as a package itself (consistent with how all other packages are handled), letting each integrator decide whether it has content to deploy. This avoids drift if new primitive types are added in the future. - Pass full traceback via diagnostics.error(detail=...) so root integration failures are actionable in the diagnostic report. When root integration is the only action (no external deps), also emit logger.error() so the failure is visible to the user rather than silently swallowed. - Add two integration tests: test_root_apm_hooks_deployed (guards that a hooks-only .apm/ dir does not hit the early-return path) and test_root_skill_md_detected (guards the SKILL.md detection branch). All 6 TestRootProjectPrimitives tests pass.
|
@danielmeppiel Thank you for taking the time to review this — your feedback was really helpful. I've pushed a follow-up commit that addresses all three points raised in the review:
Let me know if there's anything else you'd like me to adjust. Thanks again! |
| _cli_root_apm = _cli_project_root / ".apm" | ||
| _cli_has_root_primitives = ( | ||
| _cli_root_apm.exists() and _cli_root_apm.is_dir() | ||
| ) or (_cli_project_root / "SKILL.md").exists() |
There was a problem hiding this comment.
Why would we be processing it in case the project contains a Skill.md in root? I think that would be confusing UX
| _root_apm_dir = project_root / ".apm" | ||
| _root_has_local_primitives = ( | ||
| _root_apm_dir.exists() and _root_apm_dir.is_dir() | ||
| ) or (project_root / "SKILL.md").exists() |
There was a problem hiding this comment.
Second time the check appears in the file, seems error prone. Could extract to function.
What
apm installnow deploys primitives from the project root's own.apm/directory, alongside any declared external dependencies.Before: running
apm installon a project with root.apm/instructions/rules either exited silently ("All dependencies are up to date") or deployed only the external packages' rules -- the root-level.apm/was never touched.After:
Why
Users with both external dependencies and local rules in root
.apm/were forced to create a dummy./agent/apm.ymlstub and add it as a local-path dependency just to get their own rules deployed. This is the issue raised in Discussion #595 and tracked in #714.The root cause is two early-return guards inside
_install_apm_dependencies()that check whether the resolved dependency list is empty and return before target detection and integrator setup ever run. The integration pipeline only operates on paths insideapm_modules/, so the project root itself was never a candidate.How
Three changes to
src/apm_cli/commands/install.py:install()entry point -- detect root.apm/subdirectories and enter the APM install path even whenhas_any_apm_depsis False.project_rootcomputation before the guard and skip the return when root primitives exist.deps_to_install == []guard.PackageInfo(install_path=project_root)and run it through the existing_integrate_package_primitivespipeline. No new integration logic; uses the same path all other packages take.The existing
./agent/apm.ymlworkaround continues to work without changes (verified by regression test).Test
Added
TestRootProjectPrimitivesintests/integration/test_local_install.py:.apm/+ no external deps: rules deployed.apm/: both rule sets deployed./agent/apm.ymlworkaround still worksapm installtwice produces the same resultAll four new tests pass. Full test suite: 4309 passed, 0 new failures.
Fixes #714
Refs Discussion #595