fix(plugins): contain user-file carry within staged tree

(cherry picked from commit 97be7ac043a95f20268628e1a3c2e6d22c9c74d1)

hermes_cli/plugins_cmd_catalog.py

@@ -282,7 +282,22 @@ def refuse_if_installed_removed(name: str, plugin_dir) -> None:
             "or reinstall with `hermes plugins install <source> --force --allow-removed` if you trust it.")
 
 
-_PRESERVE_SKIP = ("__pycache__", CATALOG_SIDECAR)
+_PRESERVE_SKIP = ("__pycache__", ".git", CATALOG_SIDECAR)
+_NO_GIT_REVISION_FILES = frozenset({
+    "plugin.yaml", "plugin.yml", "plugin.json", "mcp.json",
+    "pyproject.toml", "package.json", "package-lock.json", "uv.lock",
+})
+_NO_GIT_REVISION_DIRS = frozenset({"desktop", "skills", "sidecar", "node_modules"})
+
+
+def _revision_owned_without_git(rel: Path) -> bool:
+    """True for plugin code/control surfaces an update must never resurrect from the old tree."""
+    return (
+        rel.suffix == ".py"
+        or rel.as_posix() in _NO_GIT_REVISION_FILES
+        or bool(rel.parts and rel.parts[0] in _NO_GIT_REVISION_DIRS)
+    )
+
 
 
 def _local_changes(target: Path) -> Optional[tuple[list[str], list[str]]]:
@@ -290,12 +305,21 @@ def _local_changes(target: Path) -> Optional[tuple[list[str], list[str]]]:
     cannot classify the installed tree (notably subdirectory installs, which carry no ``.git``)."""
     from hermes_cli.plugins_cmd import _resolve_git_executable, _run_plugin_git
     git_exe = _resolve_git_executable()
-    if not git_exe or not (target / ".git").exists():
+    if not (target / ".git").exists():
         return None
+    if not git_exe:
+        from hermes_cli.plugins_cmd import PluginOperationError
+        raise PluginOperationError(
+            f"Could not inspect local changes for '{target.name}': git executable is unavailable."
+        )
     status = _run_plugin_git(git_exe, target, "status", "--porcelain", "--ignored", "-z", "--untracked-files=all",
                              "--ignored=matching", timeout=30)
     if status.returncode != 0:
-        return None
+        from hermes_cli.plugins_cmd import PluginOperationError
+        detail = (status.stderr or status.stdout or "git status failed").strip()
+        raise PluginOperationError(
+            f"Could not inspect local changes for '{target.name}': {detail}"
+        )
     local, modified = [], []
     for item in status.stdout.split("\0"):
         if len(item) < 4:
@@ -321,11 +345,18 @@ def _carry_user_files(old: Path, new: Path, local: Optional[list[str]]) -> None:
 
     For a git checkout, *local* is the ``??``/``!!`` set and may contain a directory entry
     such as ``data/``; descendants of those entries are copied and win over same-path files in
-    the new tree.  ``None`` means git cannot classify the tree, so only non-Python files that
-    the new tree does not already ship are carried.  Path-type conflicts are owned by the new tree.
+    the new tree. ``None`` means there is no git checkout, so user-state files absent from the new
+    tree are carried while executable/declarative plugin surfaces remain revision-owned. If a
+    user-owned path cannot be represented safely in the new tree, fail before publication rather
+    than silently dropping it.
     """
     keep = {Path(rel) for rel in local or ()}
-    for dirpath, dirnames, filenames in os.walk(old):
+
+    def _walk_error(exc: OSError) -> None:
+        from hermes_cli.plugins_cmd import PluginOperationError
+        raise PluginOperationError(f"Could not preserve user files from '{old}': {exc}") from exc
+
+    for dirpath, dirnames, filenames in os.walk(old, onerror=_walk_error):
         here = Path(dirpath)
         links = [name for name in dirnames if (here / name).is_symlink()]
         dirnames[:] = [
@@ -337,23 +368,54 @@ def _carry_user_files(old: Path, new: Path, local: Optional[list[str]]) -> None:
             rel = src.relative_to(old)
             if any(part in _PRESERVE_SKIP or part.endswith(".pyc") for part in rel.parts):
                 continue
+            dst = new / rel
             if local is None:
                 # A no-git subdir install cannot distinguish removed upstream code from user files.
-                # Never resurrect an old Python module/package into a new plugin revision.
-                if rel.suffix == ".py" or os.path.lexists(new / rel):
+                # Never resurrect old executable/control surfaces. This path may run from
+                # _install_plugin_core's post-scan before_swap hook, so do not inject an unscanned
+                # symlink either.
+                if src.is_symlink() or _revision_owned_without_git(rel):
+                    continue
+                if os.path.lexists(dst):
+                    # A same-shape path belongs to the new revision when git cannot prove otherwise.
+                    # A file -> directory clash is different: silently skipping it would delete a
+                    # user-state file, so keep the live install intact and make the user resolve it.
+                    if dst.is_dir() and not dst.is_symlink():
+                        from hermes_cli.plugins_cmd import PluginOperationError
+                        raise PluginOperationError(
+                            f"Cannot preserve user file '{rel}': the updated plugin now has a directory "
+                            "at that path. The installed plugin was left unchanged."
+                        )
                     continue
             elif keep.isdisjoint((rel, *rel.parents)):
                 continue
 
-            dst = new / rel
-            # The replacement tree owns file/dir shape changes. Carrying across a type clash can
-            # either copy into the wrong directory or make parent mkdir fail and abort the update.
             if dst.is_dir() and not dst.is_symlink():
-                continue
-            try:
-                dst.parent.mkdir(parents=True, exist_ok=True)
-            except (FileExistsError, NotADirectoryError):
-                continue
+                from hermes_cli.plugins_cmd import PluginOperationError
+                raise PluginOperationError(
+                    f"Cannot preserve user file '{rel}': the updated plugin now has a directory "
+                    "at that path. The installed plugin was left unchanged."
+                )
+
+            parent = new
+            for part in rel.parent.parts:
+                parent /= part
+                if os.path.lexists(parent):
+                    if parent.is_symlink() or not parent.is_dir():
+                        from hermes_cli.plugins_cmd import PluginOperationError
+                        raise PluginOperationError(
+                            f"Cannot preserve user file '{rel}': its destination conflicts with the "
+                            "updated plugin. The installed plugin was left unchanged."
+                        )
+                    continue
+                try:
+                    parent.mkdir()
+                except OSError as exc:
+                    from hermes_cli.plugins_cmd import PluginOperationError
+                    raise PluginOperationError(
+                        f"Cannot preserve user file '{rel}': its destination could not be prepared. "
+                        "The installed plugin was left unchanged."
+                    ) from exc
             if dst.is_symlink() or dst.is_file():
                 dst.unlink()
             shutil.copy2(src, dst, follow_symlinks=False)

tests/hermes_cli/test_plugins_cmd_catalog.py

@@ -204,8 +204,8 @@ def test_repin_keeps_local_files_backs_up_edits_and_follows_manifest_rename(worl
     assert any("plugins-backup" in w for w in result["warnings"]) and any("renamed" in w for w in result["warnings"])
 
 
-def test_carry_user_files_without_git_preserves_data_but_not_old_code_or_type_clashes(tmp_path):
-    """No-git fallback keeps user state without resurrecting removed code or fighting new tree shapes."""
+def test_carry_user_files_without_git_preserves_data_but_not_old_code(tmp_path):
+    """No-git fallback keeps user state without resurrecting removed executable/control surfaces."""
     old = tmp_path / "old"
     new = tmp_path / "new"
     old.mkdir()
@@ -215,25 +215,101 @@ def test_carry_user_files_without_git_preserves_data_but_not_old_code_or_type_cl
     (old / "data").mkdir()
     (old / "data" / "state.db").write_text("user data")
     (old / "legacy.py").write_text("OLD = True\n")
-
-    # old=file/new=dir: the new directory owns the path.
-    (old / "file-to-dir").write_text("old user file")
-    (new / "file-to-dir").mkdir()
-    (new / "file-to-dir" / "current.txt").write_text("new tree")
-
-    # old=dir/new=file: carrying a descendant must not make mkdir abort the update.
-    (old / "dir-to-file").mkdir()
-    (old / "dir-to-file" / "state.db").write_text("old nested data")
-    (new / "dir-to-file").write_text("new tree file")
+    (old / ".git").write_text("gitdir: /tmp/foreign-worktree\\n")
+    (old / "desktop").mkdir()
+    (old / "desktop" / "plugin.js").write_text('export default { id: "stale" }\n')
+    (old / "skills").mkdir()
+    (old / "skills" / "stale").mkdir()
+    (old / "skills" / "stale" / "SKILL.md").write_text("# stale\n")
+    (old / "mcp.json").write_text('{"mcpServers":{"stale":{"type":"stdio","command":"./stale"}}}\n')
+    (old / "pyproject.toml").write_text('[project]\nname="stale"\nversion="1"\n')
+    (old / "package.json").write_text('{"name":"stale"}\n')
 
     cat._carry_user_files(old, new, None)
 
     assert (new / "config.yaml").read_text() == "endpoint: mine\n"
     assert (new / "data" / "state.db").read_text() == "user data"
     assert not (new / "legacy.py").exists()
-    assert (new / "file-to-dir" / "current.txt").read_text() == "new tree"
-    assert sorted(path.name for path in (new / "file-to-dir").iterdir()) == ["current.txt"]
-    assert (new / "dir-to-file").read_text() == "new tree file"
+    assert not (new / ".git").exists()
+    assert not (new / "desktop").exists()
+    assert not (new / "skills").exists()
+    assert not (new / "mcp.json").exists()
+    assert not (new / "pyproject.toml").exists()
+    assert not (new / "package.json").exists()
+
+
+@pytest.mark.parametrize("shape", ["old-file-new-dir", "old-dir-new-file"])
+def test_carry_user_files_fails_closed_on_type_clashes(tmp_path, shape):
+    """An update never drops user state just because the new revision changed a path's type."""
+    old, new = tmp_path / "old", tmp_path / "new"
+    old.mkdir()
+    new.mkdir()
+    if shape == "old-file-new-dir":
+        (old / "data").write_text("user data")
+        (new / "data").mkdir()
+    else:
+        (old / "data" / "db").mkdir(parents=True)
+        (old / "data" / "db" / "index.db").write_text("user data")
+        (new / "data").write_text("new upstream file")
+
+    with pytest.raises(pc.PluginOperationError, match="Cannot preserve user file"):
+        cat._carry_user_files(old, new, None)
+
+    if shape == "old-file-new-dir":
+        assert (old / "data").read_text() == "user data"
+        assert (new / "data").is_dir()
+    else:
+        assert (old / "data" / "db" / "index.db").read_text() == "user data"
+        assert (new / "data").read_text() == "new upstream file"
+
+
+def test_carry_user_files_fails_closed_on_staged_symlink_parent(tmp_path):
+    """A staged symlink cannot redirect carried user data outside the replacement transaction."""
+    old, new, outside = tmp_path / "old", tmp_path / "new", tmp_path / "outside"
+    (old / "data").mkdir(parents=True)
+    new.mkdir()
+    outside.mkdir()
+    (old / "data" / "index.db").write_text("user data")
+    try:
+        (new / "data").symlink_to(outside, target_is_directory=True)
+    except OSError:
+        pytest.skip("symlinks unavailable on this platform")
+
+    with pytest.raises(pc.PluginOperationError, match="Cannot preserve user file"):
+        cat._carry_user_files(old, new, None)
+
+    assert not (outside / "index.db").exists()
+    assert (old / "data" / "index.db").read_text() == "user data"
+
+
+def test_carry_user_files_fails_closed_when_source_tree_cannot_be_walked(tmp_path, monkeypatch):
+    """Unreadable user state aborts replacement instead of being silently omitted."""
+    old = tmp_path / "old"
+    new = tmp_path / "new"
+    old.mkdir()
+    new.mkdir()
+
+    def denied_walk(_path, *, onerror=None, **_kwargs):
+        assert onerror is not None
+        onerror(PermissionError("denied"))
+        return ()
+
+    monkeypatch.setattr(cat.os, "walk", denied_walk)
+    with pytest.raises(pc.PluginOperationError, match="Could not preserve user files.*denied"):
+        cat._carry_user_files(old, new, None)
+
+
+def test_git_checkout_update_fails_closed_when_local_changes_cannot_be_inspected(world, monkeypatch):
+    """A destructive re-pin must not guess ownership when a real git checkout cannot be inspected."""
+    target = cat.install_catalog_entry(pc_cat.get_live_catalog_entry("cat-plugin"), force=False)[0]
+    assert (target / ".git").exists()
+    monkeypatch.setattr(pc, "_resolve_git_executable", lambda: None)
+    world["state"]["pin"] = world["sha2"]
+
+    with pytest.raises(pc.PluginOperationError, match="git executable is unavailable"):
+        cat.repin_catalog_plugin(target, cat.read_catalog_sidecar(target))
+
+    assert _head(target) == world["sha1"]
 
 
 @pytest.mark.parametrize("via", ["url", "catalog"])
@@ -245,6 +321,8 @@ def test_update_of_a_subdir_install_keeps_files_the_user_created_or_edited(world
     (src / "plugin.yaml").write_text("name: sub-plugin\nversion: 1.0.0\ndescription: d\n")
     (src / "__init__.py").write_text("def register(ctx):\n    pass\n")
     (src / "config.yaml.example").write_text("endpoint: default\n")
+    (src / "desktop").mkdir()
+    (src / "desktop" / "plugin.js").write_text("export default { id: \"v1\" }\n")
     sp.run(["git", "init", "-q"], cwd=mono, check=True, env=_GIT_ENV)
     pin = {"sha": _commit(mono, "v1")}
 
@@ -271,12 +349,14 @@ def test_update_of_a_subdir_install_keeps_files_the_user_created_or_edited(world
 
     (src / "plugin.yaml").write_text("name: sub-plugin\nversion: 2.0.0\ndescription: d\n")
     (src / "config.yaml.example").write_text("endpoint: new-default\n")
+    shutil.rmtree(src / "desktop")
     pin["sha"] = _commit(mono, "v2")
     assert pc.dashboard_update_user_plugin("sub-plugin")["ok"] is True
 
     assert "version: 2.0.0" in (target / "plugin.yaml").read_text()
     assert (target / "config.yaml").read_text() == "endpoint: mine\n"
     assert (target / "data" / "state.json").read_text() == "{}"
+    assert not (target / "desktop").exists()
 
 
 def test_repin_keeps_a_wholly_ignored_data_dir_in_a_git_checkout(world):

website/docs/user-guide/features/plugin-catalog.md

@@ -197,8 +197,11 @@ before publishing it. Your
 enabled/disabled state is preserved, and so are files the plugin's repo does
 not track (the `config.yaml` created from its `.example`, data files, `.env`).
 For monorepo/subdirectory installs, which do not carry a local Git checkout,
-update preserves non-Python files the new revision does not ship; removed Python
-code is not carried forward because it can shadow the new plugin layout.
+update preserves user-state files the new revision does not ship. Plugin code and
+control surfaces (Python, Desktop/skills, manifests/MCP and dependency metadata)
+remain revision-owned and are not resurrected from the old install. If a user-state
+path conflicts with the new tree's file/directory layout, the update stops before
+publication so the installed copy — and the user's data — remain intact.
 Edits you made to *tracked* files are not carried onto the new code; copies are
 saved under `~/.hermes/plugins-backup/<name>-<sha>/` and the update warns you.
 If the new pin renames the plugin's manifest, the old directory is removed and