diff --git a/hermes_cli/plugins_cmd_catalog.py b/hermes_cli/plugins_cmd_catalog.py index 2692728b8f..4e49b38d43 100644 --- a/hermes_cli/plugins_cmd_catalog.py +++ b/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 --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) diff --git a/tests/hermes_cli/test_plugins_cmd_catalog.py b/tests/hermes_cli/test_plugins_cmd_catalog.py index f3ff7209eb..2f0ed49d2e 100644 --- a/tests/hermes_cli/test_plugins_cmd_catalog.py +++ b/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): diff --git a/website/docs/user-guide/features/plugin-catalog.md b/website/docs/user-guide/features/plugin-catalog.md index 86f6159e93..4d2229eba8 100644 --- a/website/docs/user-guide/features/plugin-catalog.md +++ b/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/-/` and the update warns you. If the new pin renames the plugin's manifest, the old directory is removed and