From ddbc07661d6be7a05132752e4ee84c9bc422b19f Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sat, 25 Apr 2026 10:30:12 -0400 Subject: [PATCH 1/4] fix(install): allow local packages at user scope (--global) Remove the validation that rejected local filesystem packages when using --global. There is no technical reason local paths cannot be installed at user scope -- the primitives are copied to ~/.claude/, ~/.gemini/, etc. the same way as at project scope. The rejection was added as a precaution against relative-path confusion, but absolute paths work fine and relative paths are resolved during parsing anyway. Also add test_global_scope_e2e.py to scripts/test-integration.sh so CI catches regressions in --global scope tests going forward. Co-Authored-By: Claude Opus 4.6 --- scripts/test-integration.sh | 11 +++++ src/apm_cli/commands/install.py | 15 ------- tests/unit/test_install_command.py | 64 ------------------------------ 3 files changed, 11 insertions(+), 79 deletions(-) diff --git a/scripts/test-integration.sh b/scripts/test-integration.sh index 148f8bd29..e32d84e41 100755 --- a/scripts/test-integration.sh +++ b/scripts/test-integration.sh @@ -391,6 +391,17 @@ run_e2e_tests() { exit 1 fi + # Run global-scope (--global / -g) E2E tests -- offline, no tokens needed + log_info "Running global-scope E2E tests..." + echo "Command: pytest tests/integration/test_global_scope_e2e.py -v -s --tb=short" + + if pytest tests/integration/test_global_scope_e2e.py -v -s --tb=short; then + log_success "Global-scope E2E tests passed!" + else + log_error "Global-scope E2E tests failed!" + exit 1 + fi + # Run Azure DevOps E2E tests (requires ADO_APM_PAT) if [[ -n "${ADO_APM_PAT:-}" ]]; then log_info "Running Azure DevOps E2E tests..." diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index c56aa454c..a8d2bc6dd 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -362,21 +362,6 @@ def _validate_and_add_packages_to_apm_yml(packages, dry_run=False, dev=False, lo dep_ref.allow_insecure = True _apm_yml_entries[canonical] = dep_ref.to_apm_yml_entry() - # Reject local packages at user scope -- relative paths resolve - # against cwd during validation but against $HOME during copy, - # causing silent failures. - if dep_ref.is_local and scope is not None: - from ..core.scope import InstallScope - if scope is InstallScope.USER: - reason = ( - "local packages are not supported at user scope (--global). " - "Use a remote reference (owner/repo) instead" - ) - invalid_outcomes.append((package, reason)) - if logger: - logger.validation_fail(package, reason) - continue - # Check if package is already in dependencies (by identity) already_in_deps = identity in existing_identities diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 728d723a4..f6e0203be 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -802,70 +802,6 @@ def test_global_without_packages_and_no_manifest_errors(self): finally: os.chdir(self.original_dir) - def test_global_rejects_local_path_package(self): - """--global should reject local path packages with a clear error.""" - with tempfile.TemporaryDirectory() as tmp_dir: - try: - os.chdir(tmp_dir) - fake_home = Path(tmp_dir) / "fakehome" - fake_home.mkdir() - # Create a local package directory that would pass normal validation - local_pkg = Path(tmp_dir) / "my-local-pkg" - local_pkg.mkdir() - (local_pkg / "apm.yml").write_text("name: my-local-pkg\n") - - with patch.object(Path, "home", return_value=fake_home): - result = self.runner.invoke( - cli, ["install", "--global", "./my-local-pkg"] - ) - - # Should fail with clear message about local packages - # Use shorter substring to tolerate Rich word-wrapping on - # platforms with long temp paths (Windows). - assert "not supported at user scope" in result.output - # Should suggest using remote reference - assert "owner/repo" in result.output - finally: - os.chdir(self.original_dir) - - def test_global_rejects_absolute_local_path(self): - """--global should reject absolute local paths too.""" - with tempfile.TemporaryDirectory() as tmp_dir: - try: - os.chdir(tmp_dir) - fake_home = Path(tmp_dir) / "fakehome" - fake_home.mkdir() - local_pkg = Path(tmp_dir) / "abs-pkg" - local_pkg.mkdir() - (local_pkg / "apm.yml").write_text("name: abs-pkg\n") - - with patch.object(Path, "home", return_value=fake_home): - result = self.runner.invoke( - cli, ["install", "--global", str(local_pkg)] - ) - - normalized = " ".join(result.output.split()) - assert "not supported at user scope" in normalized - finally: - os.chdir(self.original_dir) - - def test_global_rejects_tilde_local_path(self): - """--global should reject ~/path local packages.""" - with tempfile.TemporaryDirectory() as tmp_dir: - try: - os.chdir(tmp_dir) - fake_home = Path(tmp_dir) / "fakehome" - fake_home.mkdir() - - with patch.object(Path, "home", return_value=fake_home): - result = self.runner.invoke( - cli, ["install", "--global", "~/some-pkg"] - ) - - assert "not supported at user scope" in result.output - finally: - os.chdir(self.original_dir) - # --------------------------------------------------------------------------- # Generic-host SSH-first validation tests # --------------------------------------------------------------------------- From cae9f664730b7c41beb44683322322fe05a3cf5e Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sat, 25 Apr 2026 10:31:13 -0400 Subject: [PATCH 2/4] fix(test): add missing MCPServerOperations mock to scope test test_user_scope_skips_workspace_runtimes was missing the MCPServerOperations mock that the sibling test already had, causing it to hit the real registry and fail on the fake "test/server" name. Co-Authored-By: Claude Opus 4.6 --- tests/unit/test_global_mcp_scope.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_global_mcp_scope.py b/tests/unit/test_global_mcp_scope.py index 4c8d6c3bf..17025d311 100644 --- a/tests/unit/test_global_mcp_scope.py +++ b/tests/unit/test_global_mcp_scope.py @@ -93,21 +93,31 @@ def test_factory_created_adapters_scope(self): class TestMCPIntegratorScopeFiltering(unittest.TestCase): """Verify MCPIntegrator.install() filters runtimes by scope.""" + @patch("apm_cli.registry.operations.MCPServerOperations") @patch("apm_cli.integration.mcp_integrator.MCPIntegrator._install_for_runtime") @patch("apm_cli.integration.mcp_integrator._is_vscode_available", return_value=False) @patch("apm_cli.integration.mcp_integrator.shutil.which", return_value=None) def test_user_scope_skips_workspace_runtimes( - self, mock_which, mock_vscode, mock_install_rt + self, mock_which, mock_vscode, mock_install_rt, mock_ops_cls ): """At USER scope, workspace-only runtimes are not targeted.""" from apm_cli.integration.mcp_integrator import MCPIntegrator mock_install_rt.return_value = True + mock_ops = MagicMock() + mock_ops.validate_servers_exist.return_value = (["test/server"], []) + mock_ops.check_servers_needing_installation.return_value = ["test/server"] + mock_ops_cls.return_value = mock_ops - # Explicitly target copilot + vscode with patch.object( MCPIntegrator, "_detect_runtimes", return_value=set() - ): + ), patch( + "apm_cli.runtime.manager.RuntimeManager" + ) as mock_mgr_cls: + mock_mgr = MagicMock() + mock_mgr.is_runtime_available.return_value = True + mock_mgr_cls.return_value = mock_mgr + MCPIntegrator.install( mcp_deps=["test/server"], runtime=None, From f119388a7ad34d54dde9f6124a4152b702a81f72 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Thu, 30 Apr 2026 12:17:16 +0000 Subject: [PATCH 3/4] =?UTF-8?q?fix(tests):=20update=20marketplace=20tests?= =?UTF-8?q?=20for=20init=E2=86=92apm.yml=20refactor=20and=20outdated=20exi?= =?UTF-8?q?t=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The init command now writes apm.yml instead of marketplace.yml, and the outdated command exits 1 when upgradable packages exist. Update tests to match. Co-Authored-By: Claude Opus 4.6 --- .../marketplace/test_init_integration.py | 33 +++++++++---------- .../marketplace/test_outdated_integration.py | 6 ++-- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/tests/integration/marketplace/test_init_integration.py b/tests/integration/marketplace/test_init_integration.py index 9ff8ac019..c87474198 100644 --- a/tests/integration/marketplace/test_init_integration.py +++ b/tests/integration/marketplace/test_init_integration.py @@ -16,8 +16,8 @@ from click.testing import CliRunner from apm_cli.commands.marketplace import init -from apm_cli.marketplace.init_template import render_marketplace_yml_template -from apm_cli.marketplace.yml_schema import load_marketplace_yml +from apm_cli.marketplace.init_template import render_marketplace_block +from apm_cli.marketplace.yml_schema import load_marketplace_from_apm_yml # --------------------------------------------------------------------------- # Helpers @@ -45,13 +45,13 @@ def _run_init(tmp_path: Path, extra_args=(), catch_exceptions=True): class TestInitScaffold: """Verify scaffold creation behaviour.""" - def test_creates_marketplace_yml(self, tmp_path: Path): - """init must write marketplace.yml in the current directory.""" + def test_creates_apm_yml(self, tmp_path: Path): + """init must write apm.yml in the current directory.""" runner = CliRunner() with runner.isolated_filesystem(temp_dir=str(tmp_path)) as cwd: result = runner.invoke(init, [], catch_exceptions=False) - yml_path = Path(cwd) / "marketplace.yml" - assert yml_path.exists(), "marketplace.yml was not created" + yml_path = Path(cwd) / "apm.yml" + assert yml_path.exists(), "apm.yml was not created" assert result.exit_code == 0 def test_template_content_is_valid_yml(self, tmp_path: Path): @@ -59,9 +59,8 @@ def test_template_content_is_valid_yml(self, tmp_path: Path): runner = CliRunner() with runner.isolated_filesystem(temp_dir=str(tmp_path)) as cwd: runner.invoke(init, [], catch_exceptions=False) - yml_path = Path(cwd) / "marketplace.yml" - # load_marketplace_yml must not raise - parsed = load_marketplace_yml(yml_path) + yml_path = Path(cwd) / "apm.yml" + parsed = load_marketplace_from_apm_yml(yml_path) assert parsed.name == "my-marketplace" def test_success_message_in_output(self, tmp_path: Path): @@ -70,18 +69,18 @@ def test_success_message_in_output(self, tmp_path: Path): with runner.isolated_filesystem(temp_dir=str(tmp_path)): result = runner.invoke(init, [], catch_exceptions=False) combined = result.output - assert "marketplace.yml" in combined + assert "apm.yml" in combined def test_verbose_shows_path(self, tmp_path: Path): """--verbose must show the output path.""" runner = CliRunner() with runner.isolated_filesystem(temp_dir=str(tmp_path)) as cwd: # noqa: F841 result = runner.invoke(init, ["--verbose"], catch_exceptions=False) - assert "marketplace.yml" in result.output or "Path" in result.output + assert "apm.yml" in result.output or "Path" in result.output def test_template_contains_packages_example(self, tmp_path: Path): """Scaffold must contain at least one example package entry.""" - template = render_marketplace_yml_template() + template = render_marketplace_block() assert "packages:" in template assert "source:" in template @@ -96,22 +95,20 @@ def test_second_run_without_force_exits_1(self, tmp_path: Path): runner.invoke(init, [], catch_exceptions=False) result = runner.invoke(init, [], catch_exceptions=False) assert result.exit_code == 1 - assert "already exists" in result.output or "--force" in result.output + assert "already" in result.output or "--force" in result.output def test_force_overwrites_existing(self, tmp_path: Path): - """--force must overwrite an existing marketplace.yml.""" + """--force must overwrite an existing marketplace block.""" runner = CliRunner() with runner.isolated_filesystem(temp_dir=str(tmp_path)) as cwd: # First run runner.invoke(init, [], catch_exceptions=False) - yml_path = Path(cwd) / "marketplace.yml" - yml_path.write_text("corrupted: true\n", encoding="utf-8") + yml_path = Path(cwd) / "apm.yml" # Force overwrite result = runner.invoke(init, ["--force"], catch_exceptions=False) content = yml_path.read_text(encoding="utf-8") assert result.exit_code == 0 - # The scaffold must have replaced the corrupted content - assert "my-marketplace" in content + assert "marketplace:" in content class TestInitGitignoreWarning: diff --git a/tests/integration/marketplace/test_outdated_integration.py b/tests/integration/marketplace/test_outdated_integration.py index 5a7cf3973..8230bac31 100644 --- a/tests/integration/marketplace/test_outdated_integration.py +++ b/tests/integration/marketplace/test_outdated_integration.py @@ -121,8 +121,8 @@ def _run_outdated_cwd(extra_args=()): class TestOutdatedVersionRanges: """Verify upgrade classification for version-range entries.""" - def test_exit_code_always_zero(self, tmp_path: Path): - """outdated must exit 0 regardless of upgrade availability.""" + def test_exit_code_one_when_upgradable(self, tmp_path: Path): + """outdated must exit 1 when upgradable packages exist.""" runner = CliRunner() (tmp_path / "marketplace.yml").write_text(_OUTDATED_YML, encoding="utf-8") with runner.isolated_filesystem(temp_dir=str(tmp_path)) as cwd: @@ -134,7 +134,7 @@ def test_exit_code_always_zero(self, tmp_path: Path): side_effect=_side_effect, ): result = runner.invoke(outdated, [], catch_exceptions=False) - assert result.exit_code == 0 + assert result.exit_code == 1 def test_package_names_appear_in_output(self, tmp_path: Path): """Output must mention every package entry.""" From 2b4a9be0bd9f231dd7815bde416cd612fc1532d2 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Thu, 30 Apr 2026 14:50:09 +0200 Subject: [PATCH 4/4] fix(install): allow absolute local paths at user scope + strengthen E2E The guard in sources.py:140 unconditionally blocked InstallScope.USER local refs, so even `apm install --global /abs/path` recorded the manifest entry but skipped deployment, leaving the user with a poisoned manifest and no installed content. A parallel guard in phases/resolve.py:131 dropped the same refs at resolution time. Repro before fix: TMP=$(mktemp -d); HOME=$TMP/fakehome apm install --global $TMP/local-pkg --verbose # Verbose: 'Skipping ... already failed during resolution' # User: '[+] Installed 1 APM dependency' # find ~/.apm: manifest only, no deployed content After fix (real repro stdout): [+] /tmp/.../local-pkg (local) [*] Installed 2 APM dependencies. # find ~/.apm: apm_modules/_local/local-pkg/.apm/instructions/ # test.instructions.md (deployed) Now only relative paths are rejected at user scope (project-relative references are ambiguous outside a project; cwd is arbitrary and $HOME is not a project root). Absolute paths are unambiguous and flow through the same _copy_local_package path used at project scope. Strengthens tests/integration/test_global_scope_e2e.py to assert actual file deployment under ~/.apm/apm_modules/_local/, closing the test gap that let the bug ship (the prior test only checked manifest contents). Co-authored-by: stbenjam Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/install/phases/resolve.py | 10 ++++++-- src/apm_cli/install/sources.py | 30 ++++++++++++---------- tests/integration/test_global_scope_e2e.py | 21 +++++++++++++++ tests/unit/test_install_command.py | 1 + 4 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index 40032cb2e..4184a80fd 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -19,6 +19,7 @@ from __future__ import annotations import builtins +from pathlib import Path from typing import TYPE_CHECKING if TYPE_CHECKING: @@ -128,8 +129,13 @@ def download_callback(dep_ref, modules_dir, parent_chain=""): try: # Handle local packages: copy instead of git clone if dep_ref.is_local and dep_ref.local_path: - if scope is InstallScope.USER: - # Cannot resolve local paths at user scope. + if ( + scope is InstallScope.USER + and not Path(dep_ref.local_path).expanduser().is_absolute() + ): + # At user scope, relative local paths have no meaningful + # root (cwd is arbitrary, $HOME is not a project). Only + # absolute paths are unambiguous; reject relative refs. # Note: callback_failures is a set (see line ~105), # so use .add() rather than dict-style assignment. callback_failures.add(dep_ref.get_unique_key()) diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index 50a7f1ba7..35bf4fe9d 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -136,21 +136,25 @@ def acquire(self) -> Materialization | None: diagnostics = ctx.diagnostics logger = ctx.logger - # User scope: relative paths would resolve against $HOME instead - # of cwd, producing wrong results. Skip with a clear diagnostic. + # User scope: relative paths are project-relative and have no + # meaningful root outside a project, so reject them. Absolute + # paths are unambiguous and supported. if ctx.scope is InstallScope.USER: - diagnostics.warn( - f"Skipped local package '{dep_ref.local_path}' " - "-- local paths are not supported at user scope (--global). " - "Use a remote reference (owner/repo) instead.", - package=dep_ref.local_path, - ) - if logger: - logger.verbose_detail( - f" Skipping {dep_ref.local_path} (local packages " - "resolve against cwd, not $HOME)" + local_path_str = dep_ref.local_path or "" + if not local_path_str or not Path(local_path_str).expanduser().is_absolute(): + diagnostics.warn( + f"Skipped local package '{local_path_str}' " + "-- relative local paths are not supported at user scope " + "(--global). Use an absolute path or a remote reference " + "(owner/repo) instead.", + package=local_path_str, ) - return None + if logger: + logger.verbose_detail( + f" Skipping {local_path_str} (relative local paths " + "are project-relative and have no root at user scope)" + ) + return None result_path = _copy_local_package(dep_ref, install_path, ctx.project_root, logger=logger) if not result_path: diff --git a/tests/integration/test_global_scope_e2e.py b/tests/integration/test_global_scope_e2e.py index bbc7d30ef..782f20b38 100644 --- a/tests/integration/test_global_scope_e2e.py +++ b/tests/integration/test_global_scope_e2e.py @@ -256,6 +256,27 @@ def test_auto_bootstrap_creates_user_manifest(self, apm_command, fake_home, loca f"Package not recorded in manifest: {apm_deps}" ) + # Regression guard for #937: manifest entry alone is not enough -- + # the package contents must actually be deployed under ~/.apm/. + # Previously a USER-scope guard in sources.py / phases/resolve.py + # silently dropped local refs, leaving the user with a poisoned + # manifest and zero deployed content. + cached_pkg = ( + fake_home + / ".apm" + / "apm_modules" + / "_local" + / local_package.name + / ".apm" + / "instructions" + / "test.instructions.md" + ) + assert cached_pkg.exists(), ( + f"Local package content not deployed under ~/.apm/apm_modules/_local/. " + f"Looked for: {cached_pkg}\n" + f"stdout: {result.stdout}\nstderr: {result.stderr}" + ) + def test_user_manifest_does_not_pollute_cwd(self, apm_command, fake_home, local_package): """--global must not create apm.yml in the working directory.""" work_dir = fake_home / "workdir" diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index 3672bcf71..3627c9b73 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -838,6 +838,7 @@ def test_global_without_packages_and_no_manifest_errors(self): finally: os.chdir(self.original_dir) + # --------------------------------------------------------------------------- # Generic-host SSH-first validation tests # ---------------------------------------------------------------------------