mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(profiles): allowlist default-export paths + preserve symlinks (#58394)
`hermes profile export default` crashed with `shutil.Error` when HERMES_HOME pointed outside ~/.hermes (common in Docker deployments) and the workspace contained broken symlinks. Two root causes: 1. `copytree` defaults to `symlinks=False` and follows link targets; broken ones crash. #58397 (liuhao1024) drafted a minimal `symlinks=True` flag fix; this PR adopts that change. 2. `copytree` was invoked against the entire HERMES_HOME root (which doubles as cwd in Docker layouts). The post-hoc blacklist at `_DEFAULT_EXPORT_EXCLUDE_ROOT` is a fixed-length enumerate-and-pray list that can't anticipate every unrelated sibling directory (`x11-dev/`, etc.). Replaced with a positive allow-list at `_DEFAULT_EXPORT_INCLUDE_ROOT` enumerating the known Hermes profile artifacts (config, persona, skills, cron, scripts, sessions, plugins, memories, knowledge, preferences). Sensitive runtime surfaces (`state.db`, `logs/`, auth files, other profiles) are intentionally not in the allow-list so the export stays a portable, credential-free snapshot of the user-facing surface — which means the existing `test_export_default_excludes_infrastructure` regressions remain green. Adds two regression tests: * test_export_default_uses_allowlist_for_unrelated_dirs — >x11-dev< sibling directories must not leak into the archive. * test_export_default_handles_broken_symlinks — symlinks inside allowed artifacts survive instead of crashing the export. closing that PR as superseded once this lands. Closes #58394
This commit is contained in:
parent
b6b9bcd2a1
commit
8d9684c9da
2 changed files with 102 additions and 19 deletions
|
|
@ -224,6 +224,25 @@ _DEFAULT_EXPORT_EXCLUDE_ROOT = frozenset({
|
|||
"logs", # gateway logs
|
||||
})
|
||||
|
||||
# Allow-list for ``export_profile("default")``: when HERMES_HOME equals the
|
||||
# cwd (Docker/custom deployments), the default profile home is the working
|
||||
# directory and contains arbitrary user files that should NOT be bundled
|
||||
# into the export. The set below identifies the *known Hermes profile
|
||||
# artifacts* at the root of HERMES_HOME; everything else is excluded.
|
||||
# Sensitive runtime infrastructure (``state.db``, ``logs/``, ``auth.*``,
|
||||
# other profiles) is intentionally *not* in this list so the export stays
|
||||
# a portable, credential-free snapshot of the user-facing surface
|
||||
# (#58394). Add new artifacts here when introduced in ``hermes_constants``.
|
||||
_DEFAULT_EXPORT_INCLUDE_ROOT = frozenset({
|
||||
# Configuration / persona
|
||||
"config.yaml", "SOUL.md", "MEMORY.md", "USER.md", "todo.json",
|
||||
"system_prompt.md", "AGENTS.md", "CLAUDE.md", ".cursorrules",
|
||||
# User-facing skill, cron, and session artifacts
|
||||
"skills", "cron", "scripts", "sessions",
|
||||
# Plugin / memory surfaces (per-profile overrides live here)
|
||||
"plugins", "memories", "knowledge", "preferences",
|
||||
})
|
||||
|
||||
# Names that cannot be used as profile aliases
|
||||
_RESERVED_NAMES = frozenset({
|
||||
"hermes", "default", "test", "tmp", "root", "sudo",
|
||||
|
|
@ -1843,8 +1862,18 @@ def get_active_profile_name() -> str:
|
|||
def _default_export_ignore(root_dir: Path):
|
||||
"""Return an *ignore* callable for :func:`shutil.copytree`.
|
||||
|
||||
At the root level it excludes everything in ``_DEFAULT_EXPORT_EXCLUDE_ROOT``.
|
||||
At all levels it excludes ``__pycache__``, sockets, and temp files.
|
||||
Two-tier filtering:
|
||||
|
||||
* **Root-level allow-list** — only entries whose name appears in
|
||||
``_DEFAULT_EXPORT_INCLUDE_ROOT`` survive. Everything else (such as
|
||||
an unrelated ``x11-dev/`` directory in a Docker deployment where
|
||||
HERMES_HOME equals the cwd) is excluded. Blacklisting was tried
|
||||
first and proved unable to anticipate every non-Hermes file the
|
||||
user may have lying alongside HERMES_HOME (#58394).
|
||||
* **Universal exclusions at any depth** — ``__pycache__``, sockets,
|
||||
temp files; plus npm lockfiles, which may appear at the root.
|
||||
|
||||
All other profile artifacts are copied through untouched.
|
||||
"""
|
||||
|
||||
def _ignore(directory: str, contents: list) -> set:
|
||||
|
|
@ -1856,9 +1885,12 @@ def _default_export_ignore(root_dir: Path):
|
|||
# npm lockfiles can appear at root
|
||||
elif entry in {"package.json", "package-lock.json"}:
|
||||
ignored.add(entry)
|
||||
# Root-level exclusions
|
||||
# Root-level allow-list: drop everything that isn't a known
|
||||
# Hermes profile artifact.
|
||||
if Path(directory) == root_dir:
|
||||
ignored.update(c for c in contents if c in _DEFAULT_EXPORT_EXCLUDE_ROOT)
|
||||
ignored.update(
|
||||
entry for entry in contents if entry not in _DEFAULT_EXPORT_INCLUDE_ROOT
|
||||
)
|
||||
return ignored
|
||||
|
||||
return _ignore
|
||||
|
|
|
|||
|
|
@ -1385,19 +1385,63 @@ class TestExportImport:
|
|||
|
||||
assert not any("__pycache__" in n for n in names)
|
||||
|
||||
def test_export_default_handles_broken_symlinks(self, profile_env, tmp_path):
|
||||
"""Export succeeds when the profile directory contains broken symlinks.
|
||||
def test_export_default_uses_allowlist_for_unrelated_dirs(self, profile_env, tmp_path):
|
||||
"""Unrelated directories under HERMES_HOME are excluded by allow-list (#58394).
|
||||
|
||||
In Docker/custom HERMES_HOME deployments, unrelated directories may
|
||||
contain stale symlinks. copytree must not follow them.
|
||||
Docker/custom deployments often set HERMES_HOME to a working
|
||||
directory that also contains unrelated user projects (``x11-dev/``,
|
||||
etc.). The root-level allow-list filters those out so only known
|
||||
Hermes artifacts end up in the archive. Replaces the old
|
||||
exhaustive blacklist.
|
||||
"""
|
||||
default_dir = get_profile_dir("default")
|
||||
(default_dir / "config.yaml").write_text("ok")
|
||||
# Create a broken symlink (target does not exist)
|
||||
(default_dir / "broken_link").symlink_to("/nonexistent/path")
|
||||
# Create a valid symlink for comparison
|
||||
(default_dir / "valid_target.txt").write_text("real data")
|
||||
(default_dir / "valid_link").symlink_to(default_dir / "valid_target.txt")
|
||||
(default_dir / "SOUL.md").write_text("soul")
|
||||
# Allowed subdirectory with content
|
||||
(default_dir / "skills" / "demo").mkdir(parents=True)
|
||||
(default_dir / "skills" / "demo" / "SKILL.md").write_text("hi")
|
||||
# Unrelated directory — should NOT appear in the archive
|
||||
unrelated = default_dir / "x11-dev" / "usr" / "lib"
|
||||
unrelated.mkdir(parents=True)
|
||||
(unrelated / "libXi.so").write_text("data")
|
||||
|
||||
output = tmp_path / "export" / "default.tar.gz"
|
||||
output.parent.mkdir(parents=True, exist_ok=True)
|
||||
result = export_profile("default", str(output))
|
||||
|
||||
with tarfile.open(str(result), "r:gz") as tf:
|
||||
names = set(tf.getnames())
|
||||
|
||||
# Allowed artifacts present
|
||||
assert any(n.endswith("config.yaml") for n in names)
|
||||
assert any(n.endswith("SOUL.md") for n in names)
|
||||
assert any(n.endswith("skills/demo/SKILL.md") for n in names)
|
||||
# Unrelated artifact excluded
|
||||
assert not any("x11-dev" in n for n in names)
|
||||
assert not any("libXi.so" in n for n in names)
|
||||
|
||||
def test_export_default_handles_broken_symlinks(self, profile_env, tmp_path):
|
||||
"""Broken symlinks inside allowed artifacts are preserved, not crashed (#58394).
|
||||
|
||||
``shutil.copytree``'s default is ``symlinks=False``, which follows
|
||||
symlinks and crashes on broken ones. Use ``symlinks=True`` so stale
|
||||
symlinks inside *allowed* artifacts (e.g. ``skills/``) survive as
|
||||
symlinks; the link and its target are both retained.
|
||||
"""
|
||||
default_dir = get_profile_dir("default")
|
||||
(default_dir / "config.yaml").write_text("ok")
|
||||
# Place broken symlink *inside* the allowed ``skills/`` tree so the
|
||||
# root-level allow-list passes the directory through; the
|
||||
# symlinks=True flag must then preserve the link instead of
|
||||
# following and crashing.
|
||||
broken_dir = default_dir / "skills" / "with-broken-links"
|
||||
broken_dir.mkdir(parents=True)
|
||||
(broken_dir / "broken_link").symlink_to("/nonexistent/path")
|
||||
# Valid symlink for comparison
|
||||
(broken_dir / "valid_target.txt").write_text("real data")
|
||||
(broken_dir / "valid_link").symlink_to(
|
||||
broken_dir / "valid_target.txt"
|
||||
)
|
||||
|
||||
output = tmp_path / "export" / "default.tar.gz"
|
||||
output.parent.mkdir(parents=True, exist_ok=True)
|
||||
|
|
@ -1405,12 +1449,19 @@ class TestExportImport:
|
|||
|
||||
assert result.exists()
|
||||
with tarfile.open(str(result), "r:gz") as tf:
|
||||
names = tf.getnames()
|
||||
# Broken symlink is preserved as a symlink entry
|
||||
assert any("broken_link" in n for n in names)
|
||||
# Valid symlink and its target are both present
|
||||
assert any("valid_link" in n for n in names)
|
||||
assert any("valid_target.txt" in n for n in names)
|
||||
names = set(tf.getnames())
|
||||
# Allowed artifact survived
|
||||
assert any(n.endswith("config.yaml") for n in names)
|
||||
# Broken symlink inside an allowed dir was preserved as a symlink
|
||||
# (without crashing) — tar entry name recorded as the link path.
|
||||
assert any(
|
||||
"with-broken-links/broken_link" in n for n in names
|
||||
), (
|
||||
f"broken_link should survive; tarfile names: {sorted(names)[:30]}"
|
||||
)
|
||||
# Valid symlink + target also kept
|
||||
assert any("valid_link" in n for n in names)
|
||||
assert any("valid_target.txt" in n for n in names)
|
||||
|
||||
def test_import_default_without_name_raises(self, profile_env, tmp_path):
|
||||
"""Importing a default export without --name gives clear guidance."""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue