diff --git a/scripts/test-integration.sh b/scripts/test-integration.sh index dc338f395..c1b7b5a46 100755 --- a/scripts/test-integration.sh +++ b/scripts/test-integration.sh @@ -402,6 +402,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 e756898e5..c0228b6f0 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -401,22 +401,6 @@ def _resolve_package_references( 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/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/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.""" 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 55f4fa17b..3627c9b73 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -838,64 +838,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