mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-01 01:51:44 +00:00
fix(skills): let skill_manage patch/edit/delete skills in external_dirs in place (#17512)
Closes #4759, closes #4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
This commit is contained in:
Teknium
GitHub
parent
e120cd5941
commit
8c8fc6c1ec
2 changed files with 171 additions and 25 deletions
|
|
@ -566,3 +566,151 @@ class TestSecurityScanGate:
|
|||
|
||||
with patch("hermes_cli.config.load_config", side_effect=RuntimeError("boom")):
|
||||
assert _guard_agent_created_enabled() is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# External skills directories (skills.external_dirs) — mutations in place
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@contextmanager
|
||||
def _two_roots(local_dir: Path, external_dir: Path):
|
||||
"""Patch the skill manager so local SKILLS_DIR = local_dir and
|
||||
get_all_skills_dirs() returns [local_dir, external_dir] in order."""
|
||||
with patch("tools.skill_manager_tool.SKILLS_DIR", local_dir), \
|
||||
patch("agent.skill_utils.get_all_skills_dirs",
|
||||
return_value=[local_dir, external_dir]):
|
||||
yield
|
||||
|
||||
|
||||
def _write_external_skill(external_dir: Path, name: str = "ext-skill") -> Path:
|
||||
skill_dir = external_dir / name
|
||||
skill_dir.mkdir(parents=True)
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
f"---\nname: {name}\ndescription: An external skill.\n---\n\n"
|
||||
"# External\n\nBody with OLD_MARKER here.\n"
|
||||
)
|
||||
return skill_dir
|
||||
|
||||
|
||||
class TestExternalSkillMutations:
|
||||
"""Verify skill_manage can patch/edit/write/remove/delete skills that live
|
||||
under skills.external_dirs — in place, without duplicating to local.
|
||||
|
||||
Regression for issues #4759 and #4381: the read-only gate used to refuse
|
||||
with 'Skill X is in an external directory and cannot be modified', which
|
||||
caused agents to create duplicate copies in ~/.hermes/skills/ as a
|
||||
workaround.
|
||||
"""
|
||||
|
||||
def test_patch_external_skill_writes_in_place(self, tmp_path):
|
||||
local = tmp_path / "local"
|
||||
external = tmp_path / "vault"
|
||||
local.mkdir(); external.mkdir()
|
||||
skill_dir = _write_external_skill(external)
|
||||
|
||||
with _two_roots(local, external):
|
||||
result = _patch_skill("ext-skill", "OLD_MARKER", "NEW_MARKER")
|
||||
|
||||
assert result["success"] is True, result
|
||||
assert "NEW_MARKER" in (skill_dir / "SKILL.md").read_text()
|
||||
# No duplicate in local
|
||||
assert not (local / "ext-skill").exists()
|
||||
|
||||
def test_edit_external_skill_writes_in_place(self, tmp_path):
|
||||
local = tmp_path / "local"
|
||||
external = tmp_path / "vault"
|
||||
local.mkdir(); external.mkdir()
|
||||
skill_dir = _write_external_skill(external)
|
||||
|
||||
new_content = (
|
||||
"---\nname: ext-skill\ndescription: Rewritten.\n---\n\n"
|
||||
"# Rewritten\n\nBrand new body.\n"
|
||||
)
|
||||
with _two_roots(local, external):
|
||||
result = _edit_skill("ext-skill", new_content)
|
||||
|
||||
assert result["success"] is True, result
|
||||
assert "Brand new body" in (skill_dir / "SKILL.md").read_text()
|
||||
assert not (local / "ext-skill").exists()
|
||||
|
||||
def test_write_file_on_external_skill(self, tmp_path):
|
||||
local = tmp_path / "local"
|
||||
external = tmp_path / "vault"
|
||||
local.mkdir(); external.mkdir()
|
||||
skill_dir = _write_external_skill(external)
|
||||
|
||||
with _two_roots(local, external):
|
||||
result = _write_file("ext-skill", "references/notes.md", "# Notes\n")
|
||||
|
||||
assert result["success"] is True, result
|
||||
assert (skill_dir / "references" / "notes.md").read_text() == "# Notes\n"
|
||||
assert not (local / "ext-skill").exists()
|
||||
|
||||
def test_remove_file_on_external_skill(self, tmp_path):
|
||||
local = tmp_path / "local"
|
||||
external = tmp_path / "vault"
|
||||
local.mkdir(); external.mkdir()
|
||||
skill_dir = _write_external_skill(external)
|
||||
(skill_dir / "references").mkdir()
|
||||
(skill_dir / "references" / "notes.md").write_text("# Notes\n")
|
||||
|
||||
with _two_roots(local, external):
|
||||
result = _remove_file("ext-skill", "references/notes.md")
|
||||
|
||||
assert result["success"] is True, result
|
||||
assert not (skill_dir / "references" / "notes.md").exists()
|
||||
|
||||
def test_delete_external_skill_removes_skill_not_root(self, tmp_path):
|
||||
local = tmp_path / "local"
|
||||
external = tmp_path / "vault"
|
||||
local.mkdir(); external.mkdir()
|
||||
skill_dir = _write_external_skill(external)
|
||||
|
||||
with _two_roots(local, external):
|
||||
result = _delete_skill("ext-skill")
|
||||
|
||||
assert result["success"] is True, result
|
||||
assert not skill_dir.exists()
|
||||
# The external root must NOT be rmdir'd, even when empty after deletion
|
||||
assert external.exists() and external.is_dir()
|
||||
|
||||
def test_delete_external_skill_cleans_empty_category(self, tmp_path):
|
||||
"""When a skill lives under external/<category>/<name>, deleting the
|
||||
last skill in the category should rmdir the empty category dir but
|
||||
stop at the external root."""
|
||||
local = tmp_path / "local"
|
||||
external = tmp_path / "vault"
|
||||
local.mkdir(); external.mkdir()
|
||||
cat_dir = external / "team"
|
||||
cat_dir.mkdir()
|
||||
skill_dir = cat_dir / "ext-skill"
|
||||
skill_dir.mkdir()
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: ext-skill\ndescription: An external skill.\n---\n\n"
|
||||
"# External\n\nBody.\n"
|
||||
)
|
||||
|
||||
with _two_roots(local, external):
|
||||
result = _delete_skill("ext-skill")
|
||||
|
||||
assert result["success"] is True, result
|
||||
assert not skill_dir.exists()
|
||||
assert not cat_dir.exists() # empty category cleaned up
|
||||
assert external.exists() # but never the external root
|
||||
|
||||
def test_create_still_writes_to_local_root(self, tmp_path):
|
||||
"""Creating a new skill always lands in local SKILLS_DIR, never
|
||||
external_dirs — create is unchanged by this PR."""
|
||||
local = tmp_path / "local"
|
||||
external = tmp_path / "vault"
|
||||
local.mkdir(); external.mkdir()
|
||||
|
||||
with _two_roots(local, external):
|
||||
result = _create_skill("fresh-skill", VALID_SKILL_CONTENT.replace(
|
||||
"name: test-skill", "name: fresh-skill"))
|
||||
|
||||
assert result["success"] is True, result
|
||||
assert (local / "fresh-skill" / "SKILL.md").exists()
|
||||
assert not (external / "fresh-skill").exists()
|
||||
|
||||
|
|
|
|||
|
|
@ -109,16 +109,28 @@ MAX_NAME_LENGTH = 64
|
|||
MAX_DESCRIPTION_LENGTH = 1024
|
||||
|
||||
|
||||
def _is_local_skill(skill_path: Path) -> bool:
|
||||
"""Check if a skill path is within the local SKILLS_DIR.
|
||||
|
||||
Skills found in external_dirs are read-only from the agent's perspective.
|
||||
def _containing_skills_root(skill_path: Path) -> Path:
|
||||
"""Return the skills root directory (local or external_dirs entry) that
|
||||
contains ``skill_path``. Falls back to the local ``SKILLS_DIR`` if no
|
||||
match is found (defensive — callers should have located the skill via
|
||||
``_find_skill`` first).
|
||||
"""
|
||||
from agent.skill_utils import get_all_skills_dirs
|
||||
|
||||
try:
|
||||
skill_path.resolve().relative_to(SKILLS_DIR.resolve())
|
||||
return True
|
||||
except ValueError:
|
||||
return False
|
||||
resolved = skill_path.resolve()
|
||||
except OSError:
|
||||
resolved = skill_path
|
||||
|
||||
for root in get_all_skills_dirs():
|
||||
try:
|
||||
resolved.relative_to(root.resolve())
|
||||
return root
|
||||
except (ValueError, OSError):
|
||||
continue
|
||||
return SKILLS_DIR
|
||||
|
||||
|
||||
MAX_SKILL_CONTENT_CHARS = 100_000 # ~36k tokens at 2.75 chars/token
|
||||
MAX_SKILL_FILE_BYTES = 1_048_576 # 1 MiB per supporting file
|
||||
|
||||
|
|
@ -397,9 +409,6 @@ def _edit_skill(name: str, content: str) -> Dict[str, Any]:
|
|||
if not existing:
|
||||
return {"success": False, "error": f"Skill '{name}' not found. Use skills_list() to see available skills."}
|
||||
|
||||
if not _is_local_skill(existing["path"]):
|
||||
return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified. Copy it to your local skills directory first."}
|
||||
|
||||
skill_md = existing["path"] / "SKILL.md"
|
||||
# Back up original content for rollback
|
||||
original_content = skill_md.read_text(encoding="utf-8") if skill_md.exists() else None
|
||||
|
|
@ -440,9 +449,6 @@ def _patch_skill(
|
|||
if not existing:
|
||||
return {"success": False, "error": f"Skill '{name}' not found."}
|
||||
|
||||
if not _is_local_skill(existing["path"]):
|
||||
return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified. Copy it to your local skills directory first."}
|
||||
|
||||
skill_dir = existing["path"]
|
||||
|
||||
if file_path:
|
||||
|
|
@ -522,15 +528,13 @@ def _delete_skill(name: str) -> Dict[str, Any]:
|
|||
if not existing:
|
||||
return {"success": False, "error": f"Skill '{name}' not found."}
|
||||
|
||||
if not _is_local_skill(existing["path"]):
|
||||
return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be deleted."}
|
||||
|
||||
skill_dir = existing["path"]
|
||||
skills_root = _containing_skills_root(skill_dir)
|
||||
shutil.rmtree(skill_dir)
|
||||
|
||||
# Clean up empty category directories (don't remove SKILLS_DIR itself)
|
||||
# Clean up empty category directories (don't remove the skills root itself)
|
||||
parent = skill_dir.parent
|
||||
if parent != SKILLS_DIR and parent.exists() and not any(parent.iterdir()):
|
||||
if parent != skills_root and parent.exists() and not any(parent.iterdir()):
|
||||
parent.rmdir()
|
||||
|
||||
return {
|
||||
|
|
@ -567,9 +571,6 @@ def _write_file(name: str, file_path: str, file_content: str) -> Dict[str, Any]:
|
|||
if not existing:
|
||||
return {"success": False, "error": f"Skill '{name}' not found. Create it first with action='create'."}
|
||||
|
||||
if not _is_local_skill(existing["path"]):
|
||||
return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified. Copy it to your local skills directory first."}
|
||||
|
||||
target, err = _resolve_skill_target(existing["path"], file_path)
|
||||
if err:
|
||||
return {"success": False, "error": err}
|
||||
|
|
@ -604,9 +605,6 @@ def _remove_file(name: str, file_path: str) -> Dict[str, Any]:
|
|||
if not existing:
|
||||
return {"success": False, "error": f"Skill '{name}' not found."}
|
||||
|
||||
if not _is_local_skill(existing["path"]):
|
||||
return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified."}
|
||||
|
||||
skill_dir = existing["path"]
|
||||
|
||||
target, err = _resolve_skill_target(skill_dir, file_path)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue