Skip to content

fix(install): scope local content scan to ~/.apm/ at user scope (#830)#850

Merged
danielmeppiel merged 9 commits intomicrosoft:mainfrom
guwenqing:fix/830-global-install-home-glob
Apr 24, 2026
Merged

fix(install): scope local content scan to ~/.apm/ at user scope (#830)#850
danielmeppiel merged 9 commits intomicrosoft:mainfrom
guwenqing:fix/830-global-install-home-glob

Conversation

@guwenqing
Copy link
Copy Markdown
Contributor

@guwenqing guwenqing commented Apr 22, 2026

fix: scope link-resolver discovery to ~/.apm/ at user scope (#830)

Fixes #830

At user scope (--global), project_root is Path.home(). integrate_local_content() passes this as install_path, and BaseIntegrator.init_link_resolver() calls discover_primitives(install_path) — which recursive-globs the entire home directory. This causes multi-minute hangs and triggers macOS privacy dialogs (Downloads, Desktop, etc.).

Root cause: Feature #626 (local .apm/ auto-discovery, v0.8.12) was not tested with -g where project_root = ~/ (feature #452, v0.8.6).

Fix: In BaseIntegrator.init_link_resolver(), when install_path == Path.home(), scope discover_primitives() to ~/.apm/ instead of ~/. This keeps PackageInfo.install_path as project_root so integrators still find primitives at <project_root>/.apm// correctly.

Before: apm install -g hangs indefinitely After: apm install -g completes in ~4 seconds

Changes

  • src/apm_cli/integration/base_integrator.py — scope discover_primitives() scan root in init_link_resolver() when install_path is $HOME
  • tests/unit/integration/test_base_integrator.py — add tests for home-directory scoping
  • tests/unit/test_local_content_install.py — verify install_path stays project_root at user scope

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass (93 passed)
  • Added tests for new functionality

…osoft#830)

At user scope (`--global`), `project_root` is `Path.home()`.
`integrate_local_content()` passed this directly as `install_path`,
causing `discover_primitives()` to recursive-glob the entire home
directory.  This caused multi-minute hangs and triggered macOS privacy
dialogs (Downloads, Desktop, etc.).

Root cause: feature microsoft#626 (local `.apm/` auto-discovery, v0.8.12) was
not tested with `-g` where `project_root = ~/` (feature microsoft#452, v0.8.6).

Fix: when `scope is InstallScope.USER`, set `install_path` to
`project_root / ".apm"` so only `~/.apm/` is scanned.

Before: `apm install -g` hangs indefinitely
After:  `apm install -g` completes in ~4 seconds

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 22, 2026 18:51
@guwenqing
Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a user-scope (apm install -g/--global) performance hang where local primitive discovery could recursively scan the entire home directory by changing how the synthetic “local” package’s scan root is chosen during local-content integration.

Changes:

  • Adjust integrate_local_content() to set the synthetic local package install_path/package_path to ~/.apm when scope=InstallScope.USER.
  • Add a unit test asserting the user-scope synthetic PackageInfo.install_path is <project_root>/.apm rather than <project_root>.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/apm_cli/install/services.py Changes synthetic local package roots at user scope to avoid scanning $HOME.
tests/unit/test_local_content_install.py Adds coverage asserting the new user-scope install_path behavior.

Comment thread src/apm_cli/install/services.py Outdated
Comment thread tests/unit/test_local_content_install.py
…not install_path

Address Copilot review feedback: changing install_path to ~/.apm/
broke integrator file discovery (they look under <install_path>/.apm/<type>/).

Move the fix to BaseIntegrator.init_link_resolver() instead: when
install_path == $HOME, scope discover_primitives() to ~/.apm/ to
avoid recursive-globbing the entire home directory.

- Revert services.py install_path change (keep project_root)
- Fix base_integrator.py init_link_resolver to scope scan root
- Update test to verify install_path stays project_root at user scope
- Add tests for init_link_resolver home-directory scoping

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@guwenqing guwenqing force-pushed the fix/830-global-install-home-glob branch from 8307ff8 to de46eb2 Compare April 22, 2026 19:03
@danielmeppiel danielmeppiel added the panel-review Trigger the apm-review-panel gh-aw workflow label Apr 23, 2026
@github-actions
Copy link
Copy Markdown

APM Review Panel Verdict

Disposition: APPROVE (one required pre-merge action -- see below)


PR Summary

Fix for issue #830: when a user-scope package has install_path == Path.home(),
init_link_resolver was passing ~/ to discover_primitives, triggering a
recursive glob across the entire home directory. This PR redirects the scan
to ~/.apm/ in that case. Three files changed (+63/-2): one production fix
and two test additions.


Per-persona findings

Python Architect: The fix is surgical and correctly placed. Kudos for
self-correcting from the initial _integrate_local_content approach to the
init_link_resolver chokepoint -- that is exactly the right layer. The condition
scan_root == Path.home() is simple and readable. It relies on the silent
invariant that only user-scope packages land at install_path == Path.home();
this invariant is not enforced by type or parameter signature, just by runtime
convention. This is acceptable under the "simplicity first" philosophy, but a
future refactor could make the coupling explicit by threading scope: InstallScope
into init_link_resolver. Pre-existing except Exception: self.link_resolver = None
silently swallows discovery errors -- not introduced here, but worth a follow-up
issue. Test coverage is solid: home-path case, non-home-path case, and a regression
guard in the local-content test.

CLI Logging Expert: No output or logging changes. Pass -- nothing to review.

DevX UX Expert: No CLI surface changes; fix is fully internal. The performance
improvement (no more hanging on large home directories during user-scope install)
is a silent UX win. No docs update needed. The updated docstring in
test_local_content_install.py ("at project scope") correctly signals the
intended invariant.

Supply Chain Security Expert: Constraining the scan scope from $HOME to
$HOME/.apm is security-positive -- it shrinks the attack surface for primitive
discovery. Path.home() equality is safe; no traversal risk. The hardcoded
".apm" suffix is consistent with the rest of the codebase convention. No new
auth, lockfile, token, or file-write surfaces introduced. No concerns.

Auth Expert: Not applicable -- no auth code touched.

OSS Growth Hacker: This removes a silent "APM freezes my terminal on global
install" failure mode. That kind of complaint travels fast in developer communities;
fixing it before it becomes a meme is high-leverage. No conversion surface is
touched. Observation (side-channel to CEO): a Fixed bullet in CHANGELOG under
[Unreleased] makes this visible to the community on next release -- right now it
would ship silently.


CEO arbitration

Specialists are aligned -- no disagreement to arbitrate. This is a well-scoped,
well-tested bug fix that improves performance and security posture simultaneously.
The self-correction in the commit history (moving the guard from
_integrate_local_content to init_link_resolver) demonstrates good judgment
about abstraction boundaries. The single blocking item is the missing CHANGELOG
entry: project convention requires every code change to have one, and the user
impact here ("global install no longer hangs on large home directories") is
worth communicating to the community on next release. Once that entry is added,
this is ready to merge.


Required actions before merge

  1. Add a Fixed entry in CHANGELOG.md under [Unreleased], for example:
    ### Fixed
    - `apm install` (user scope): `init_link_resolver` now scopes `discover_primitives`
      to `~/.apm/` instead of `~/`, preventing recursive-glob across the entire home
      directory. Fixes #830 (#850)
    

Optional follow-ups

  • Open a follow-up issue to thread scope: InstallScope into init_link_resolver
    to make the Path.home() invariant explicit rather than implicit (low priority --
    the current approach is stable for now).
  • Open a follow-up issue for the pre-existing except Exception: self.link_resolver = None
    swallow in init_link_resolver -- silent failure makes debugging discovery errors
    harder.

Generated by PR Review Panel for issue #850 · ● 555.4K ·

guwenqing and others added 2 commits April 23, 2026 16:15
…ob fix

Add the required CHANGELOG entry under [Unreleased] per the APM Review
Panel verdict on PR microsoft#850.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@stbenjam
Copy link
Copy Markdown
Contributor

Thanks for fixing, I ran into this problem as well.

@danielmeppiel danielmeppiel enabled auto-merge April 24, 2026 21:54
@danielmeppiel danielmeppiel added this pull request to the merge queue Apr 24, 2026
@danielmeppiel danielmeppiel removed this pull request from the merge queue due to a manual request Apr 24, 2026
@danielmeppiel danielmeppiel merged commit 16e5b25 into microsoft:main Apr 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

panel-review Trigger the apm-review-panel gh-aw workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User-scope install (--global) unexpectedly enters local .apm integration and can spend a long time scanning $HOME

4 participants