fix(update): preserve marketplace uninstall aliases

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)

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:

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")

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,14 +118,15 @@ def test_lockfile_builder_attaches_marketplace_source_provenance(self):
             }
         )
         ctx = SimpleNamespace(
+            existing_lockfile=None,
             marketplace_provenance={
                 "owner/repo": {
                     "discovered_via": "catalog",
                     "marketplace_plugin_name": "tool",
                     "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 == {}