fix(install): allow local packages at --global scope; fix broken tests

[stbenjam]

Summary

I spotted some failing tests that don't run in CI.

• Remove the validation that rejected local filesystem packages when using --global. There is no technical reason local paths cannot be installed at user scope -- primitives are copied to ~/.claude/, ~/.gemini/, etc. the same way as at project scope.
• Add test_global_scope_e2e.py to scripts/test-integration.sh so CI catches regressions in --global scope tests going forward. This file was never included in CI, so the broken test went undetected.
• Fix test_user_scope_skips_workspace_runtimes which was missing the MCPServerOperations mock. The test passed in CI by accident: no runtimes are installed in CI, so MCPIntegrator.install() returns early at the scope-filtering stage before reaching the registry validation. Locally, if codex is installed, target_runtimes survives scope filtering and hits the real registry, which fails on the fake test/server name. The sibling test (test_project_scope_includes_all_runtimes) already had the correct mock.

Test plan

• pytest tests/unit tests/test_console.py tests/integration/test_global_scope_e2e.py -- 5459 passed, 0 failed
• test_auto_bootstrap_creates_user_manifest now passes (previously broken since local-package rejection was added)
• test_user_scope_skips_workspace_runtimes now passes deterministically regardless of locally installed runtimes

Generated with Claude Code

[github-actions]

APM Review Panel Verdict

Disposition: REQUEST_CHANGES (incomplete implementation -- three guards, one removed)

---

Per-persona findings

Python Architect:

The PR removes one guard in _validate_and_add_packages_to_apm_yml (commands/install.py) but leaves two downstream guards intact. The pipeline has three independent enforcement points for the same policy, and only one was addressed.

OO / class diagram

classDiagram
    direction LR
    class _validate_and_add_packages_to_apm_yml {
        <<IOBoundary>>
        +packages list
        +scope InstallScope
        +logger InstallLogger
    }
    class LocalPackageSource {
        <<Strategy>>
        +acquire() Materialization
    }
    class download_callback {
        <<Pure>>
        +dep_ref DependencyReference
        +modules_dir Path
    }
    class DependencyReference {
        <<ValueObject>>
        +is_local bool
        +local_path str
        +parse(dep_str) DependencyReference
    }
    class InstallScope {
        <<Enum>>
        PROJECT
        USER
    }
    class _copy_local_package {
        <<IOBoundary>>
        +dep_ref DependencyReference
        +install_path Path
        +project_root Path
    }
    _validate_and_add_packages_to_apm_yml ..> DependencyReference : parses
    _validate_and_add_packages_to_apm_yml ..> InstallScope : reads (guard REMOVED here)
    LocalPackageSource ..> InstallScope : reads (guard STILL PRESENT)
    download_callback ..> InstallScope : reads (guard STILL PRESENT)
    LocalPackageSource ..> _copy_local_package : delegates
    download_callback ..> _copy_local_package : delegates
    class _validate_and_add_packages_to_apm_yml:::touched
    class LocalPackageSource:::untouched
    class download_callback:::untouched
    classDef touched fill:#fff3b0,stroke:#d47600
    classDef untouched fill:#ffcccc,stroke:#cc0000
    note for LocalPackageSource "sources.py:141-153: still\nrejects USER scope with\n'not supported' diagnostic"
    note for download_callback "resolve.py:136-142: still\nadds to callback_failures\nfor USER scope"

Loading

Execution flow diagram

flowchart TD
    A["apm install --global ./local-pkg"] --> B["_validate_and_add_packages_to_apm_yml\ncommands/install.py"]
    B --> C["DependencyReference.parse(package)"]
    C --> D{is_insecure?}
    D -- No --> E["[BEFORE PR] is_local AND USER scope?\n-> validation_fail + reject"]
    E -- "REMOVED by PR #937" --> F["[FS] Add to apm.yml\napm.yml updated with dep"]
    D -- No --> F
    F --> G["LocalPackageSource.acquire()\nsources.py:118"]
    G --> H{scope is USER?}
    H -- YES --> I["[STILL PRESENT] diagnostics.warn\n'local paths are not supported at user scope'\nreturn None -- package NOT deployed"]
    H -- NO --> J["_copy_local_package()\nlocal_content.py:78"]
    J --> K["[FS] shutil.copytree to apm_modules/\nIntegration pipeline runs"]
    I --> L["apm.yml has entry,\nprimitives NOT deployed,\nexit code may be 0"]
    style E fill:#ffcccc,stroke:#cc0000
    style I fill:#ffcccc,stroke:#cc0000
    style L fill:#ffeecc,stroke:#cc7700
```

**Design patterns**

- Used in this PR: none -- straight-line guard removal in a procedural validation function, appropriate for the scope.
- Pragmatic suggestion: none -- the current shape (sequential validation guards + collected outcomes) is the simplest correct design. The fix is to remove all three guards consistently, not to introduce a new pattern.

**Verdict**: The change is architecturally incomplete. `sources.py:141-153` and `resolve.py:136-142` enforce the same restriction that was removed from `commands/install.py`. The result is a broken half-state: the manifest records the dependency, the package is never deployed, and the user sees a contradictory diagnostic that still says "not supported at user scope."

---

**CLI Logging Expert**:

The removed validation correctly used `logger.validation_fail(package, reason)` -- the right pattern. No logging anti-patterns were introduced.

The live concern is `sources.py:141-153`, which still emits:

```
"Skipped local package '{dep_ref.local_path}' -- local paths are not supported at user scope (--global). Use a remote reference (owner/repo) instead."

Loading

This message will fire even after this PR merges, directly contradicting the PR's intent. If local packages are truly allowed, that diagnostic must be removed (or reworded) in the same PR.

The E2E tests check result.stdout + result.stderr with substring assertions like "user scope" in combined.lower(), which would pass even when the diagnostic is a warning rather than a success message -- they do not distinguish success from failure output.

---

DevX UX Expert:

The intent -- aligning with npm install -g ./my-pkg / pip install -e . ergonomics -- is correct. The original restriction was genuinely surprising to package-manager-savvy developers.

However, the current implementation is UX-negative compared to the before state:

• Before: apm install --global ./my-pkg fails immediately with a clear error: "not supported at user scope -- use owner/repo". The user knows exactly what to do.
• After (as submitted): Validation passes, apm.yml is updated, then a buried diagnostic warning fires ("not supported at user scope"). The user's apm.yml now lists a dependency that was never deployed. This is the worst of both worlds: no upfront guidance, and a corrupted manifest state.

The fix must be complete (remove all three guards and verify _copy_local_package path resolution at user scope) before this improves the UX.

One path-resolution note for when the fix is complete: _copy_local_package resolves relative paths against project_root. At user scope, ensure project_root is Path.cwd() (the directory where the user ran apm install), not Path.home(). If project_root is ~ at user scope, ./my-pkg resolves to ~/.my-pkg which likely does not exist.

---

Supply Chain Security Expert:

The original restriction comment cited a real correctness risk: "relative paths resolve against cwd during validation but against $HOME during copy." Inspecting _copy_local_package (local_content.py:92-96):

local = Path(dep_ref.local_path).expanduser()
if not local.is_absolute():
    local = (project_root / local).resolve()

At project scope, project_root = Path.cwd() -- correct. At user scope, project_root needs to be checked: if it resolves to Path.home(), then ./my-pkg would expand to ~/.my-pkg and silently fail the is_dir() check. This is a correctness hazard, not a traversal hazard (absolute paths are safe; relative paths may silently miss the source).

No new path traversal or token leakage surface is introduced. The path_security guards in the integrator layer remain intact and are not bypassed by this change. The risk is bounded: the user is explicitly specifying a local path they control, and _copy_local_package applies resolve() which prevents ..-style traversal.

Verdict: Low security risk, but the project_root resolution at user scope must be verified before the downstream guards are removed.

---

Auth Expert: Not activated -- no auth-relevant files changed; the PR removes a scope restriction (is_local and scope is USER) in install.py that has no bearing on AuthResolver, token precedence, or credential resolution.

---

OSS Growth Hacker:

The goal -- apm install --global ./my-pkg just working -- is a friction reducer that would resonate with developers familiar with npm link, pip install -e ., or cargo install --path .. It reinforces the "package manager for AI-native development" frame.

But a half-baked relaxation that produces a manifest with an undeployed entry is a silent failure mode that would generate confused bug reports and erode trust. Fix it completely, then it deserves a Fixed CHANGELOG entry and potentially a mention in the release notes as a quality-of-life improvement.

Side-channel to CEO: once complete, this is a good "APM behaves like mature package managers" signal for the CHANGELOG. Not a standalone launch beat, but worth a clean release note line.

---

CEO arbitration

The PR's core intent is right -- apm install --global ./my-pkg should work, and the original restriction was unnecessarily conservative. All specialists agree on the direction. The problem is implementation completeness: the early validation guard was the least important of the three enforcement points, and removing only it produces a worse user experience than the original restriction. The sources.py:141-153 guard is the one that actually prevents deployment, and it still fires with stale "not supported" language. Merging as-is would add a confusing manifest-corruption failure mode to the product. Request the author complete the change -- remove all three guards, verify project_root resolution at user scope in _copy_local_package, update the stale diagnostic message, and add a CHANGELOG entry. The E2E test suite in test_global_scope_e2e.py is well-structured and will provide good regression coverage once the pipeline is actually unblocked end-to-end.

---

Required actions before merge

1. Remove the guard in src/apm_cli/install/sources.py:141-153 (the LocalPackageSource.acquire() block that emits "local paths are not supported at user scope" and returns None). This is the guard that actually prevents deployment; leaving it makes the PR a no-op for real installs.

2. Remove the guard in src/apm_cli/install/phases/resolve.py:136-142 (the download_callback block that adds to callback_failures for transitive local deps at user scope). Without this, transitive local deps would silently fail even if direct deps work.

3. Verify project_root value at user scope in _copy_local_package (local_content.py:92-96): confirm that relative paths like ./my-pkg resolve against Path.cwd() (the directory where apm install was invoked), not Path.home(). Add a test case for a relative path with an explicit cwd different from fake_home to prevent this regression.

4. Add a CHANGELOG.md entry under [Unreleased] -> Fixed, e.g.: Local packages (./path, /abs/path) now install correctly at user scope (--global); the earlier restriction was overly conservative. (#937)

---

Optional follow-ups

• Add a unit test that directly calls LocalPackageSource.acquire() at InstallScope.USER with a local package, asserting it succeeds (not just that the manifest is written). This would have caught the incomplete guard removal.
• Consider a [i] info message when a local package is installed at user scope clarifying where the primitives were deployed (e.g., [i] Local package deployed from /abs/path to ~/.claude/commands/).
• The test_warns_about_unsupported_targets E2E test asserts "cursor" in combined.lower() -- this is a substring check against output that may contain URLs. Verify it is not flagging a URL fragment (CodeQL py/incomplete-url-substring-sanitization); if the word "cursor" only appears in prose, it is fine as-is.

Generated by PR Review Panel for issue #937 · ● 1.9M · ◷

[danielmeppiel]

Great one but need adjustment per panel findings