diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ef4721095..344c61b762 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Preserve marketplace discovery provenance across dependency updates so `plugin@marketplace` uninstall aliases keep working in project and global scope. -- by @mfroembgen (#2949) + ### Added - gh-aw's shared APM import now supports `token-source: github-token`; after consumers re-vendor the workflow, its read-only current-repository identity can fetch same-repository private packages, while `cascade` remains the default and cross-repository packages still require a dedicated token or GitHub App. (#2706) diff --git a/src/apm_cli/install/phases/lockfile.py b/src/apm_cli/install/phases/lockfile.py index d0b6a0d376..c0d5f14a71 100644 --- a/src/apm_cli/install/phases/lockfile.py +++ b/src/apm_cli/install/phases/lockfile.py @@ -380,6 +380,20 @@ def _attach_declared_licenses(self, lockfile: LockFile) -> None: lockfile.dependencies[dep_key].declared_license = declared def _attach_marketplace_provenance(self, lockfile: LockFile) -> None: + # Canonical manifest entries do not rediscover their marketplace on + # update. Keep the original discovery snapshot for surviving identities + # without carrying forward stale commits, hashes, or removed entries. + if self.ctx.existing_lockfile: + for dep_key, dep in lockfile.dependencies.items(): + previous = self.ctx.existing_lockfile.dependencies.get(dep_key) + # Ports are not part of the dependency key. + if previous is not None and previous.port == dep.port: + dep.discovered_via = previous.discovered_via + dep.marketplace_plugin_name = previous.marketplace_plugin_name + dep.source_url = previous.source_url + dep.source_digest = previous.source_digest + + # Fresh discovery replaces the entire snapshot, including absent fields. if self.ctx.marketplace_provenance: for dep_key, prov in self.ctx.marketplace_provenance.items(): if dep_key in lockfile.dependencies: diff --git a/tests/integration/test_marketplace_update_provenance_lifecycle.py b/tests/integration/test_marketplace_update_provenance_lifecycle.py new file mode 100644 index 0000000000..a6e1ea847b --- /dev/null +++ b/tests/integration/test_marketplace_update_provenance_lifecycle.py @@ -0,0 +1,169 @@ +"""Real-CLI marketplace alias ownership across a moving Git branch update.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path + +import pytest + +from apm_cli.utils.yaml_io import load_yaml +from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.local_git_repository import LocalGitRepositoryFactory +from tests.utils.local_package import LocalPackageFactory + +pytestmark = [ + pytest.mark.integration, + pytest.mark.e2e, + pytest.mark.lifecycle_smoke, + pytest.mark.requires_apm_binary, + pytest.mark.requires_e2e_mode, +] + +_HOST = "gitlab.example.invalid" +_REPO = "team/platform/agent-catalog" +_REMOTE = f"https://{_HOST}/{_REPO}" +_MARKETPLACE = "example-marketplace" +_PLUGIN = "example-plugin" +_SUBDIR = f"plugins/{_PLUGIN}" +_ALIAS = f"{_PLUGIN}@{_MARKETPLACE}" + + +def _skill_document(revision: str) -> str: + return ( + "---\n" + f"name: {_PLUGIN}\n" + "description: Marketplace update lifecycle fixture\n" + "---\n" + f"# {revision}\n" + ) + + +def _locked_dependency(lock_path: Path) -> dict[str, object]: + dependencies = load_yaml(lock_path)["dependencies"] + assert len(dependencies) == 1 + return dependencies[0] + + +@pytest.mark.parametrize("global_scope", [False, True], ids=["project", "global"]) +def test_branch_update_retains_marketplace_alias_for_offline_uninstall( + tmp_path: Path, + apm_binary_path: Path, + global_scope: bool, +) -> None: + """A real branch advance preserves host-qualified provenance and removal.""" + # Canonical HOME keeps macOS /var and /private/var paths identical for ownership. + isolated = IsolatedApmEnvironment.create( + tmp_path.resolve() / "isolated", base_env=dict(os.environ) + ) + environment = isolated.subprocess_env() + repositories = LocalGitRepositoryFactory(isolated.repository_root, env=environment) + repository = repositories.create("agent-catalog") + packages = LocalPackageFactory(repository.worktree / "plugins") + package = packages.create(_PLUGIN, targets=("codex",)) + skill_source = packages.add_skill(package, _PLUGIN, _skill_document("before update")) + marketplace_dir = repository.worktree / ".claude-plugin" + marketplace_dir.mkdir() + (marketplace_dir / "marketplace.json").write_text( + json.dumps( + { + "name": _MARKETPLACE, + "owner": {"name": "APM Test"}, + "plugins": [ + { + "name": _PLUGIN, + "source": { + "source": "git-subdir", + "url": _REMOTE, + "path": _SUBDIR, + "ref": "main", + }, + } + ], + } + ), + encoding="utf-8", + ) + initial_commit = repositories.commit(repository, message="publish initial plugin") + repositories.install_url_rewrite(repository, _REMOTE) + (isolated.home / ".gitconfig").write_bytes(Path(environment["GIT_CONFIG_GLOBAL"]).read_bytes()) + environment = repositories.url_rewrite_subprocess_env(repository, _REMOTE) + consumer = LocalPackageFactory(isolated.work_root).create("consumer", targets=("codex",)) + runner = ApmLifecycleRunner((str(apm_binary_path),), timeout_seconds=120) + scope_args = ("--global",) if global_scope else () + workspace = isolated.config_root if global_scope else consumer.root + deploy_root = isolated.home if global_scope else consumer.root + lock_path = workspace / "apm.lock.yaml" + deployed_skill = deploy_root / ".agents" / "skills" / _PLUGIN / "SKILL.md" + unrelated_skill = deploy_root / ".agents" / "skills" / "user-owned" / "SKILL.md" + unrelated_skill.parent.mkdir(parents=True) + unrelated_skill.write_text("User-owned content\n", encoding="utf-8") + + runner.run_sequence( + ( + ("marketplace", "add", _REMOTE, "--name", _MARKETPLACE, "--ref", "main"), + ( + "install", + _ALIAS, + *scope_args, + "--target", + "codex", + "--no-policy", + "--parallel-downloads", + "0", + ), + ), + expected_returncodes=(0, 0), + scenario_id="marketplace-alias-install", + cwd=consumer.root, + env=environment, + ) + before = _locked_dependency(lock_path) + assert before["resolved_commit"] == initial_commit.sha + assert before["discovered_via"] == _MARKETPLACE + assert before["marketplace_plugin_name"] == _PLUGIN + assert before["host"] == _HOST + assert before["repo_url"] == _REPO + assert before["virtual_path"] == _SUBDIR + assert deployed_skill.read_text(encoding="utf-8") == _skill_document("before update") + runner.run_sequence( + (("uninstall", _ALIAS, *scope_args, "--dry-run"),), + expected_returncodes=(0,), + scenario_id="marketplace-alias-preview-before-update", + cwd=consumer.root, + env=environment, + ) + + # Advance only the producer's actual branch; consumer state is CLI-owned. + skill_source.write_text(_skill_document("after update"), encoding="utf-8") + updated_commit = repositories.commit(repository, message="advance plugin branch") + runner.run_sequence( + (("update", *scope_args, "--yes", "--target", "codex", "--parallel-downloads", "0"),), + expected_returncodes=(0,), + scenario_id="marketplace-alias-update", + cwd=consumer.root, + env=environment, + ) + after = _locked_dependency(lock_path) + assert after["resolved_commit"] == updated_commit.sha + assert deployed_skill.read_text(encoding="utf-8") == _skill_document("after update") + for field in ("discovered_via", "marketplace_plugin_name", "host", "repo_url", "virtual_path"): + assert after.get(field) == before[field], field + + # Unregister the catalog so the offline lock lookup is the only alias source. + runner.run_sequence( + ( + ("marketplace", "remove", _MARKETPLACE, "--yes"), + ("uninstall", _ALIAS, *scope_args), + ), + expected_returncodes=(0, 0), + scenario_id="marketplace-alias-uninstall-offline", + cwd=consumer.root, + env=environment, + ) + assert not deployed_skill.exists() + assert unrelated_skill.read_text(encoding="utf-8") == "User-owned content\n" + assert not lock_path.exists() + assert not load_yaml(workspace / "apm.yml").get("dependencies", {}).get("apm") diff --git a/tests/unit/marketplace/test_lockfile_provenance.py b/tests/unit/marketplace/test_lockfile_provenance.py index 9f95305bcf..3a44b09aba 100644 --- a/tests/unit/marketplace/test_lockfile_provenance.py +++ b/tests/unit/marketplace/test_lockfile_provenance.py @@ -3,7 +3,7 @@ from types import SimpleNamespace from urllib.parse import urlparse -import pytest # noqa: F401 +import pytest from apm_cli.deps.lockfile import LockedDependency, LockFile from apm_cli.install.phases.lockfile import LockfileBuilder @@ -118,6 +118,7 @@ def test_lockfile_builder_attaches_marketplace_source_provenance(self): } ) ctx = SimpleNamespace( + existing_lockfile=None, marketplace_provenance={ "owner/repo": { "discovered_via": "catalog", @@ -125,7 +126,7 @@ def test_lockfile_builder_attaches_marketplace_source_provenance(self): "source_url": "https://catalog.example.com/marketplace.json", "source_digest": "sha256:" + "f" * 64, } - } + }, ) builder = LockfileBuilder(ctx) @@ -141,3 +142,121 @@ def test_lockfile_builder_attaches_marketplace_source_provenance(self): "/marketplace.json", ) assert dep.source_digest == "sha256:" + "f" * 64 + + +class TestRebuiltMarketplaceProvenance: + """Rebuilding a lock entry retains discovery, never stale package state.""" + + @staticmethod + def _discovered(**identity: object) -> LockedDependency: + return LockedDependency( + repo_url="owner/repo", + resolved_commit="a" * 40, + content_hash="sha256:old-content", + discovered_via="catalog", + marketplace_plugin_name="tool", + source_url="https://catalog.example.com/marketplace.json", + source_digest="sha256:" + "b" * 64, + **identity, + ) + + def test_retains_discovery_when_commit_changes(self) -> None: + previous = self._discovered( + host="git.example.com", host_type="gitlab", virtual_path="plugins/tool", is_virtual=True + ) + current = LockedDependency( + repo_url=previous.repo_url, + host=previous.host, + host_type=previous.host_type, + virtual_path=previous.virtual_path, + is_virtual=True, + resolved_commit="c" * 40, + content_hash="sha256:new-content", + ) + lockfile = LockFile(dependencies={current.get_unique_key(): current}) + ctx = SimpleNamespace( + existing_lockfile=LockFile(dependencies={previous.get_unique_key(): previous}), + marketplace_provenance=None, + ) + + LockfileBuilder(ctx)._attach_marketplace_provenance(lockfile) + + assert current.discovered_via == previous.discovered_via + assert current.marketplace_plugin_name == previous.marketplace_plugin_name + assert urlparse(current.source_url) == urlparse(previous.source_url) + assert current.source_digest == previous.source_digest + assert current.resolved_commit == "c" * 40 + assert current.content_hash == "sha256:new-content" + assert current.get_unique_key() == previous.get_unique_key() + assert previous.resolved_commit == "a" * 40 + + @pytest.mark.parametrize( + "changed_identity", + [ + {"host": "other.example.com"}, + {"port": 8443}, + {"virtual_path": "plugins/other"}, + {"repo_url": "owner/other"}, + ], + ) + def test_does_not_transfer_discovery_to_another_identity( + self, changed_identity: dict[str, object] + ) -> None: + previous = self._discovered( + host="git.example.com", virtual_path="plugins/tool", is_virtual=True + ) + identity = { + "repo_url": previous.repo_url, + "host": previous.host, + "virtual_path": previous.virtual_path, + "is_virtual": True, + **changed_identity, + } + current = LockedDependency(**identity) + lockfile = LockFile(dependencies={current.get_unique_key(): current}) + ctx = SimpleNamespace( + existing_lockfile=LockFile(dependencies={previous.get_unique_key(): previous}), + marketplace_provenance=None, + ) + + LockfileBuilder(ctx)._attach_marketplace_provenance(lockfile) + + assert current.discovered_via is None + assert current.marketplace_plugin_name is None + assert current.source_url is None + assert current.source_digest is None + assert set(lockfile.dependencies) == {current.get_unique_key()} + + def test_fresh_discovery_replaces_entire_previous_tuple(self) -> None: + previous = self._discovered() + current = LockedDependency(repo_url=previous.repo_url) + lockfile = LockFile(dependencies={current.get_unique_key(): current}) + ctx = SimpleNamespace( + existing_lockfile=LockFile(dependencies={previous.get_unique_key(): previous}), + marketplace_provenance={ + current.get_unique_key(): { + "discovered_via": "new-catalog", + "marketplace_plugin_name": "new-tool", + } + }, + ) + + LockfileBuilder(ctx)._attach_marketplace_provenance(lockfile) + + assert current.discovered_via == "new-catalog" + assert current.marketplace_plugin_name == "new-tool" + assert current.source_url is None + assert current.source_digest is None + assert previous.discovered_via == "catalog" + + def test_does_not_restore_removed_dependency(self) -> None: + previous = self._discovered() + lockfile = LockFile() + ctx = SimpleNamespace( + existing_lockfile=LockFile(dependencies={previous.get_unique_key(): previous}), + marketplace_provenance=None, + ) + + LockfileBuilder(ctx)._attach_marketplace_provenance(lockfile) + + assert lockfile.dependencies == {}