Timmy_Foundation/hermes-agent
15
0
Fork 0
Code Issues 64 Pull Requests 2 Actions 1 Packages Projects Releases Wiki Activity
Files
e49c8bbbbb1f06dbe34f71c9400f77639d16a781
hermes-agent/tests/tools/test_skill_manager_tool.py
Teknium caded0a5e7 fix: repair 57 failing CI tests across 14 files (#5823) 
* fix: repair 57 failing CI tests across 14 files

Categories of fixes:

**Test isolation under xdist (-n auto):**
- test_hermes_logging: Strip ALL RotatingFileHandlers before each test
  to prevent handlers leaked from other xdist workers from polluting counts
- test_code_execution: Force TERMINAL_ENV=local in setUp — prevents Modal
  AuthError when another test leaks TERMINAL_ENV=modal
- test_timezone: Same TERMINAL_ENV fix for execute_code timezone tests
- test_codex_execution_paths: Mock _resolve_turn_agent_config to ensure
  model resolution works regardless of xdist worker state

**Matrix adapter tests (nio not installed in CI):**
- Add _make_fake_nio() helper with real response classes for isinstance()
  checks in production code
- Replace MagicMock(spec=nio.XxxResponse) with fake_nio instances
- Wrap production method calls with patch.dict('sys.modules', {'nio': ...})
  so import nio succeeds in method bodies
- Use try/except instead of pytest.importorskip for nio.crypto imports
  (importorskip can be fooled by MagicMock in sys.modules)
- test_matrix_voice: Skip entire file if nio is a mock, not just missing

**Stale test expectations:**
- test_cli_provider_resolution: _prompt_provider_choice now takes **kwargs
  (default param added); mock getpass.getpass alongside input
- test_anthropic_oauth_flow: Mock getpass.getpass (code switched from input)
- test_gemini_provider: Mock models.dev + OpenRouter API lookups to test
  hardcoded defaults without external API variance
- test_code_execution: Add notify_on_complete to blocked terminal params
- test_setup_openclaw_migration: Mock prompt_choice to select 'Full setup'
  (new quick-setup path leads to _require_tty → sys.exit in CI)
- test_skill_manager_tool: Patch get_all_skills_dirs alongside SKILLS_DIR
  so _find_skill searches tmp_path, not real ~/.hermes/skills/

**Missing attributes in object.__new__ test runners:**
- test_platform_reconnect: Add session_store to _make_runner()
- test_session_race_guard: Add hooks, _running_agents_ts, session_store,
  delivery_router to _make_runner()

**Production bug fix (gateway/run.py):**
- Fix sentinel eviction race: _AGENT_PENDING_SENTINEL was immediately
  evicted by the stale-detection logic because sentinels have no
  get_activity_summary() method, causing _stale_idle=inf >= timeout.
  Guard _should_evict with 'is not _AGENT_PENDING_SENTINEL'.

* fix: address remaining CI failures

- test_setup_openclaw_migration: Also mock _offer_launch_chat (called at
  end of both quick and full setup paths)
- test_code_execution: Move TERMINAL_ENV=local to module level to protect
  ALL test classes (TestEnvVarFiltering, TestExecuteCodeEdgeCases,
  TestInterruptHandling, TestHeadTailTruncation) from xdist env leaks
- test_matrix: Use try/except for nio.crypto imports (importorskip can be
  fooled by MagicMock in sys.modules under xdist)
2026-04-07 09:58:45 -07:00

426 lines
15 KiB
Python
Raw Blame History

"""Tests for tools/skill_manager_tool.py — skill creation, editing, and deletion."""
import json
from contextlib import contextmanager
from pathlib import Path
from unittest.mock import patch
from tools.skill_manager_tool import (
_validate_name,
_validate_category,
_validate_frontmatter,
_validate_file_path,
_find_skill,
_resolve_skill_dir,
_create_skill,
_edit_skill,
_patch_skill,
_delete_skill,
_write_file,
_remove_file,
skill_manage,
VALID_NAME_RE,
ALLOWED_SUBDIRS,
MAX_NAME_LENGTH,
)
@contextmanager
def _skill_dir(tmp_path):
"""Patch both SKILLS_DIR and get_all_skills_dirs so _find_skill searches
only the temp directory — not the real ~/.hermes/skills/."""
with patch("tools.skill_manager_tool.SKILLS_DIR", tmp_path), \
patch("agent.skill_utils.get_all_skills_dirs", return_value=[tmp_path]):
yield
VALID_SKILL_CONTENT = """\
---
name: test-skill
description: A test skill for unit testing.
---
# Test Skill
Step 1: Do the thing.
"""
VALID_SKILL_CONTENT_2 = """\
---
name: test-skill
description: Updated description.
---
# Test Skill v2
Step 1: Do the new thing.
"""
# ---------------------------------------------------------------------------
# _validate_name
# ---------------------------------------------------------------------------
class TestValidateName:
def test_valid_names(self):
assert _validate_name("my-skill") is None
assert _validate_name("skill123") is None
assert _validate_name("my_skill.v2") is None
assert _validate_name("a") is None
def test_empty_name(self):
assert _validate_name("") == "Skill name is required."
def test_too_long(self):
err = _validate_name("a" * (MAX_NAME_LENGTH + 1))
assert err == f"Skill name exceeds {MAX_NAME_LENGTH} characters."
def test_uppercase_rejected(self):
err = _validate_name("MySkill")
assert "Invalid skill name 'MySkill'" in err
def test_starts_with_hyphen_rejected(self):
err = _validate_name("-invalid")
assert "Invalid skill name '-invalid'" in err
def test_special_chars_rejected(self):
err = _validate_name("skill/name")
assert "Invalid skill name 'skill/name'" in err
err = _validate_name("skill name")
assert "Invalid skill name 'skill name'" in err
err = _validate_name("skill@name")
assert "Invalid skill name 'skill@name'" in err
class TestValidateCategory:
def test_valid_categories(self):
assert _validate_category(None) is None
assert _validate_category("") is None
assert _validate_category("devops") is None
assert _validate_category("mlops-v2") is None
def test_path_traversal_rejected(self):
err = _validate_category("../escape")
assert "Invalid category '../escape'" in err
def test_absolute_path_rejected(self):
err = _validate_category("/tmp/escape")
assert "Invalid category '/tmp/escape'" in err
# ---------------------------------------------------------------------------
# _validate_frontmatter
# ---------------------------------------------------------------------------
class TestValidateFrontmatter:
def test_valid_content(self):
assert _validate_frontmatter(VALID_SKILL_CONTENT) is None
def test_empty_content(self):
assert _validate_frontmatter("") == "Content cannot be empty."
assert _validate_frontmatter(" ") == "Content cannot be empty."
def test_no_frontmatter(self):
err = _validate_frontmatter("# Just a heading\nSome content.\n")
assert err == "SKILL.md must start with YAML frontmatter (---). See existing skills for format."
def test_unclosed_frontmatter(self):
content = "---\nname: test\ndescription: desc\nBody content.\n"
assert _validate_frontmatter(content) == "SKILL.md frontmatter is not closed. Ensure you have a closing '---' line."
def test_missing_name_field(self):
content = "---\ndescription: desc\n---\n\nBody.\n"
assert _validate_frontmatter(content) == "Frontmatter must include 'name' field."
def test_missing_description_field(self):
content = "---\nname: test\n---\n\nBody.\n"
assert _validate_frontmatter(content) == "Frontmatter must include 'description' field."
def test_no_body_after_frontmatter(self):
content = "---\nname: test\ndescription: desc\n---\n"
assert _validate_frontmatter(content) == "SKILL.md must have content after the frontmatter (instructions, procedures, etc.)."
def test_invalid_yaml(self):
content = "---\n: invalid: yaml: {{{\n---\n\nBody.\n"
assert "YAML frontmatter parse error" in _validate_frontmatter(content)
# ---------------------------------------------------------------------------
# _validate_file_path — path traversal prevention
# ---------------------------------------------------------------------------
class TestValidateFilePath:
def test_valid_paths(self):
assert _validate_file_path("references/api.md") is None
assert _validate_file_path("templates/config.yaml") is None
assert _validate_file_path("scripts/train.py") is None
assert _validate_file_path("assets/image.png") is None
def test_empty_path(self):
assert _validate_file_path("") == "file_path is required."
def test_path_traversal_blocked(self):
err = _validate_file_path("references/../../../etc/passwd")
assert err == "Path traversal ('..') is not allowed."
def test_disallowed_subdirectory(self):
err = _validate_file_path("secret/hidden.txt")
assert "File must be under one of:" in err
assert "'secret/hidden.txt'" in err
def test_directory_only_rejected(self):
err = _validate_file_path("references")
assert "Provide a file path, not just a directory" in err
assert "'references/myfile.md'" in err
def test_root_level_file_rejected(self):
err = _validate_file_path("malicious.py")
assert "File must be under one of:" in err
assert "'malicious.py'" in err
# ---------------------------------------------------------------------------
# CRUD operations
# ---------------------------------------------------------------------------
class TestCreateSkill:
def test_create_skill(self, tmp_path):
with _skill_dir(tmp_path):
result = _create_skill("my-skill", VALID_SKILL_CONTENT)
assert result["success"] is True
assert (tmp_path / "my-skill" / "SKILL.md").exists()
def test_create_with_category(self, tmp_path):
with _skill_dir(tmp_path):
result = _create_skill("my-skill", VALID_SKILL_CONTENT, category="devops")
assert result["success"] is True
assert (tmp_path / "devops" / "my-skill" / "SKILL.md").exists()
assert result["category"] == "devops"
def test_create_duplicate_blocked(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _create_skill("my-skill", VALID_SKILL_CONTENT)
assert result["success"] is False
assert "already exists" in result["error"]
def test_create_invalid_name(self, tmp_path):
with _skill_dir(tmp_path):
result = _create_skill("Invalid Name!", VALID_SKILL_CONTENT)
assert result["success"] is False
def test_create_invalid_content(self, tmp_path):
with _skill_dir(tmp_path):
result = _create_skill("my-skill", "no frontmatter here")
assert result["success"] is False
def test_create_rejects_category_traversal(self, tmp_path):
skills_dir = tmp_path / "skills"
skills_dir.mkdir()
with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir), \
patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills_dir]):
result = _create_skill("my-skill", VALID_SKILL_CONTENT, category="../escape")
assert result["success"] is False
assert "Invalid category '../escape'" in result["error"]
assert not (tmp_path / "escape").exists()
def test_create_rejects_absolute_category(self, tmp_path):
skills_dir = tmp_path / "skills"
skills_dir.mkdir()
outside = tmp_path / "outside"
with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir), \
patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills_dir]):
result = _create_skill("my-skill", VALID_SKILL_CONTENT, category=str(outside))
assert result["success"] is False
assert f"Invalid category '{outside}'" in result["error"]
assert not (outside / "my-skill" / "SKILL.md").exists()
class TestEditSkill:
def test_edit_existing_skill(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _edit_skill("my-skill", VALID_SKILL_CONTENT_2)
assert result["success"] is True
content = (tmp_path / "my-skill" / "SKILL.md").read_text()
assert "Updated description" in content
def test_edit_nonexistent_skill(self, tmp_path):
with _skill_dir(tmp_path):
result = _edit_skill("nonexistent", VALID_SKILL_CONTENT)
assert result["success"] is False
assert "not found" in result["error"]
def test_edit_invalid_content_rejected(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _edit_skill("my-skill", "no frontmatter")
assert result["success"] is False
# Original content should be preserved
content = (tmp_path / "my-skill" / "SKILL.md").read_text()
assert "A test skill" in content
class TestPatchSkill:
def test_patch_unique_match(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _patch_skill("my-skill", "Do the thing.", "Do the new thing.")
assert result["success"] is True
content = (tmp_path / "my-skill" / "SKILL.md").read_text()
assert "Do the new thing." in content
def test_patch_nonexistent_string(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _patch_skill("my-skill", "this text does not exist", "replacement")
assert result["success"] is False
assert "not found" in result["error"].lower() or "could not find" in result["error"].lower()
def test_patch_ambiguous_match_rejected(self, tmp_path):
content = """\
---
name: test-skill
description: A test skill.
---
# Test
word word
"""
with _skill_dir(tmp_path):
_create_skill("my-skill", content)
result = _patch_skill("my-skill", "word", "replaced")
assert result["success"] is False
assert "match" in result["error"].lower()
def test_patch_replace_all(self, tmp_path):
content = """\
---
name: test-skill
description: A test skill.
---
# Test
word word
"""
with _skill_dir(tmp_path):
_create_skill("my-skill", content)
result = _patch_skill("my-skill", "word", "replaced", replace_all=True)
assert result["success"] is True
def test_patch_supporting_file(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
_write_file("my-skill", "references/api.md", "old text here")
result = _patch_skill("my-skill", "old text", "new text", file_path="references/api.md")
assert result["success"] is True
def test_patch_skill_not_found(self, tmp_path):
with _skill_dir(tmp_path):
result = _patch_skill("nonexistent", "old", "new")
assert result["success"] is False
class TestDeleteSkill:
def test_delete_existing(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _delete_skill("my-skill")
assert result["success"] is True
assert not (tmp_path / "my-skill").exists()
def test_delete_nonexistent(self, tmp_path):
with _skill_dir(tmp_path):
result = _delete_skill("nonexistent")
assert result["success"] is False
def test_delete_cleans_empty_category_dir(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT, category="devops")
_delete_skill("my-skill")
assert not (tmp_path / "devops").exists()
# ---------------------------------------------------------------------------
# write_file / remove_file
# ---------------------------------------------------------------------------
class TestWriteFile:
def test_write_reference_file(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _write_file("my-skill", "references/api.md", "# API\nEndpoint docs.")
assert result["success"] is True
assert (tmp_path / "my-skill" / "references" / "api.md").exists()
def test_write_to_nonexistent_skill(self, tmp_path):
with _skill_dir(tmp_path):
result = _write_file("nonexistent", "references/doc.md", "content")
assert result["success"] is False
def test_write_to_disallowed_path(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _write_file("my-skill", "secret/evil.py", "malicious")
assert result["success"] is False
class TestRemoveFile:
def test_remove_existing_file(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
_write_file("my-skill", "references/api.md", "content")
result = _remove_file("my-skill", "references/api.md")
assert result["success"] is True
assert not (tmp_path / "my-skill" / "references" / "api.md").exists()
def test_remove_nonexistent_file(self, tmp_path):
with _skill_dir(tmp_path):
_create_skill("my-skill", VALID_SKILL_CONTENT)
result = _remove_file("my-skill", "references/nope.md")
assert result["success"] is False
# ---------------------------------------------------------------------------
# skill_manage dispatcher
# ---------------------------------------------------------------------------
class TestSkillManageDispatcher:
def test_unknown_action(self, tmp_path):
with _skill_dir(tmp_path):
raw = skill_manage(action="explode", name="test")
result = json.loads(raw)
assert result["success"] is False
assert "Unknown action" in result["error"]
def test_create_without_content(self, tmp_path):
with _skill_dir(tmp_path):
raw = skill_manage(action="create", name="test")
result = json.loads(raw)
assert result["success"] is False
assert "content" in result["error"].lower()
def test_patch_without_old_string(self, tmp_path):
with _skill_dir(tmp_path):
raw = skill_manage(action="patch", name="test")
result = json.loads(raw)
assert result["success"] is False
def test_full_create_via_dispatcher(self, tmp_path):
with _skill_dir(tmp_path):
raw = skill_manage(action="create", name="test-skill", content=VALID_SKILL_CONTENT)
result = json.loads(raw)
assert result["success"] is True
Reference in New Issue View Git Blame Copy Permalink