diff --git a/tests/tools/test_skill_improvements.py b/tests/tools/test_skill_improvements.py new file mode 100644 index 000000000..6e781309f --- /dev/null +++ b/tests/tools/test_skill_improvements.py @@ -0,0 +1,174 @@ +"""Tests for skill fuzzy patching via tools.fuzzy_match.""" + +import json +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +from tools.skill_manager_tool import ( + _create_skill, + _patch_skill, + _write_file, + skill_manage, +) + + +SKILL_CONTENT = """\ +--- +name: test-skill +description: A test skill for unit testing. +--- + +# Test Skill + +Step 1: Do the thing. +Step 2: Do another thing. +Step 3: Final step. +""" + + +# --------------------------------------------------------------------------- +# Fuzzy patching +# --------------------------------------------------------------------------- + + +class TestFuzzyPatchSkill: + @pytest.fixture(autouse=True) + def setup_skills(self, tmp_path, monkeypatch): + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + monkeypatch.setattr("tools.skill_manager_tool.SKILLS_DIR", skills_dir) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + self.skills_dir = skills_dir + + def test_exact_match_still_works(self): + _create_skill("test-skill", SKILL_CONTENT) + result = _patch_skill("test-skill", "Step 1: Do the thing.", "Step 1: Done!") + assert result["success"] is True + content = (self.skills_dir / "test-skill" / "SKILL.md").read_text() + assert "Step 1: Done!" in content + + def test_whitespace_trimmed_match(self): + """Patch with extra leading whitespace should still find the target.""" + skill = """\ +--- +name: ws-skill +description: Whitespace test +--- + +# Commands + + def hello(): + print("hi") +""" + _create_skill("ws-skill", skill) + # Agent sends patch with no leading whitespace (common LLM behaviour) + result = _patch_skill("ws-skill", "def hello():\n print(\"hi\")", "def hello():\n print(\"hello world\")") + assert result["success"] is True + content = (self.skills_dir / "ws-skill" / "SKILL.md").read_text() + assert 'print("hello world")' in content + + def test_indentation_flexible_match(self): + """Patch where only indentation differs should succeed.""" + skill = """\ +--- +name: indent-skill +description: Indentation test +--- + +# Steps + + 1. First step + 2. Second step + 3. Third step +""" + _create_skill("indent-skill", skill) + # Agent sends with different indentation + result = _patch_skill( + "indent-skill", + "1. First step\n2. Second step", + "1. Updated first\n2. Updated second" + ) + assert result["success"] is True + content = (self.skills_dir / "indent-skill" / "SKILL.md").read_text() + assert "Updated first" in content + + def test_multiple_matches_blocked_without_replace_all(self): + """Multiple fuzzy matches should return an error without replace_all.""" + skill = """\ +--- +name: dup-skill +description: Duplicate test +--- + +# Steps + +word word word +""" + _create_skill("dup-skill", skill) + result = _patch_skill("dup-skill", "word", "replaced") + assert result["success"] is False + assert "match" in result["error"].lower() + + def test_replace_all_with_fuzzy(self): + skill = """\ +--- +name: dup-skill +description: Duplicate test +--- + +# Steps + +word word word +""" + _create_skill("dup-skill", skill) + result = _patch_skill("dup-skill", "word", "replaced", replace_all=True) + assert result["success"] is True + content = (self.skills_dir / "dup-skill" / "SKILL.md").read_text() + assert "word" not in content + assert "replaced" in content + + def test_no_match_returns_preview(self): + _create_skill("test-skill", SKILL_CONTENT) + result = _patch_skill("test-skill", "this does not exist anywhere", "replacement") + assert result["success"] is False + assert "file_preview" in result + + def test_fuzzy_patch_on_supporting_file(self): + """Fuzzy matching should also work on supporting files.""" + _create_skill("test-skill", SKILL_CONTENT) + ref_content = " function hello() {\n console.log('hi');\n }" + _write_file("test-skill", "references/code.js", ref_content) + # Patch with stripped indentation + result = _patch_skill( + "test-skill", + "function hello() {\nconsole.log('hi');\n}", + "function hello() {\nconsole.log('hello world');\n}", + file_path="references/code.js" + ) + assert result["success"] is True + content = (self.skills_dir / "test-skill" / "references" / "code.js").read_text() + assert "hello world" in content + + def test_patch_preserves_frontmatter_validation(self): + """Fuzzy matching should still run frontmatter validation on SKILL.md.""" + _create_skill("test-skill", SKILL_CONTENT) + # Try to destroy the frontmatter via patch + result = _patch_skill("test-skill", "---\nname: test-skill", "BROKEN") + assert result["success"] is False + assert "structure" in result["error"].lower() or "frontmatter" in result["error"].lower() + + def test_skill_manage_patch_uses_fuzzy(self): + """The dispatcher should route to the fuzzy-matching patch.""" + _create_skill("test-skill", SKILL_CONTENT) + raw = skill_manage( + action="patch", + name="test-skill", + old_string=" Step 1: Do the thing.", # extra leading space + new_string="Step 1: Updated.", + ) + result = json.loads(raw) + # Should succeed via line-trimmed or indentation-flexible matching + assert result["success"] is True diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 06a2f88ae..a20d23fcb 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -271,7 +271,7 @@ class TestPatchSkill: _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"] + 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 = """\ @@ -288,7 +288,7 @@ word word _create_skill("my-skill", content) result = _patch_skill("my-skill", "word", "replaced") assert result["success"] is False - assert "matched" in result["error"] + assert "match" in result["error"].lower() def test_patch_replace_all(self, tmp_path): content = """\ diff --git a/tests/tools/test_skill_size_limits.py b/tests/tools/test_skill_size_limits.py new file mode 100644 index 000000000..c94ba02e8 --- /dev/null +++ b/tests/tools/test_skill_size_limits.py @@ -0,0 +1,215 @@ +"""Tests for skill content size limits. + +Agent writes (create/edit/patch/write_file) are constrained to +MAX_SKILL_CONTENT_CHARS (100k) and MAX_SKILL_FILE_BYTES (1 MiB). +Hand-placed and hub-installed skills have no hard limit. +""" + +import json +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +from tools.skill_manager_tool import ( + MAX_SKILL_CONTENT_CHARS, + MAX_SKILL_FILE_BYTES, + _validate_content_size, + skill_manage, +) + + +@pytest.fixture(autouse=True) +def isolate_skills(tmp_path, monkeypatch): + """Redirect SKILLS_DIR to a temp directory.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + monkeypatch.setattr("tools.skill_manager_tool.SKILLS_DIR", skills_dir) + monkeypatch.setattr("tools.skills_tool.SKILLS_DIR", skills_dir) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + return skills_dir + + +def _make_skill_content(body_chars: int) -> str: + """Generate valid SKILL.md content with a body of the given character count.""" + frontmatter = ( + "---\n" + "name: test-skill\n" + "description: A test skill\n" + "---\n" + ) + body = "# Test Skill\n\n" + ("x" * max(0, body_chars - 15)) + return frontmatter + body + + +class TestValidateContentSize: + """Unit tests for _validate_content_size.""" + + def test_within_limit(self): + assert _validate_content_size("a" * 1000) is None + + def test_at_limit(self): + assert _validate_content_size("a" * MAX_SKILL_CONTENT_CHARS) is None + + def test_over_limit(self): + err = _validate_content_size("a" * (MAX_SKILL_CONTENT_CHARS + 1)) + assert err is not None + assert "100,001" in err + assert "100,000" in err + + def test_custom_label(self): + err = _validate_content_size("a" * (MAX_SKILL_CONTENT_CHARS + 1), label="references/api.md") + assert "references/api.md" in err + + +class TestCreateSkillSizeLimit: + """create action rejects oversized content.""" + + def test_create_within_limit(self, isolate_skills): + content = _make_skill_content(5000) + result = json.loads(skill_manage(action="create", name="small-skill", content=content)) + assert result["success"] is True + + def test_create_over_limit(self, isolate_skills): + content = _make_skill_content(MAX_SKILL_CONTENT_CHARS + 100) + result = json.loads(skill_manage(action="create", name="huge-skill", content=content)) + assert result["success"] is False + assert "100,000" in result["error"] + + def test_create_at_limit(self, isolate_skills): + # Content at exactly the limit should succeed + frontmatter = "---\nname: edge-skill\ndescription: Edge case\n---\n# Edge\n\n" + body_budget = MAX_SKILL_CONTENT_CHARS - len(frontmatter) + content = frontmatter + ("x" * body_budget) + assert len(content) == MAX_SKILL_CONTENT_CHARS + result = json.loads(skill_manage(action="create", name="edge-skill", content=content)) + assert result["success"] is True + + +class TestEditSkillSizeLimit: + """edit action rejects oversized content.""" + + def test_edit_over_limit(self, isolate_skills): + # Create a small skill first + small = _make_skill_content(1000) + json.loads(skill_manage(action="create", name="grow-me", content=small)) + + # Try to edit it to be oversized + big = _make_skill_content(MAX_SKILL_CONTENT_CHARS + 100) + # Fix the name in frontmatter + big = big.replace("name: test-skill", "name: grow-me") + result = json.loads(skill_manage(action="edit", name="grow-me", content=big)) + assert result["success"] is False + assert "100,000" in result["error"] + + +class TestPatchSkillSizeLimit: + """patch action checks resulting size, not just the new_string.""" + + def test_patch_that_would_exceed_limit(self, isolate_skills): + # Create a skill near the limit + near_limit = _make_skill_content(MAX_SKILL_CONTENT_CHARS - 50) + json.loads(skill_manage(action="create", name="near-limit", content=near_limit)) + + # Patch that adds enough to go over + result = json.loads(skill_manage( + action="patch", + name="near-limit", + old_string="# Test Skill", + new_string="# Test Skill\n" + ("y" * 200), + )) + assert result["success"] is False + assert "100,000" in result["error"] + + def test_patch_that_reduces_size_on_oversized_skill(self, isolate_skills, tmp_path): + """Patches that shrink an already-oversized skill should succeed.""" + # Manually create an oversized skill (simulating hand-placed) + skill_dir = tmp_path / "skills" / "bloated" + skill_dir.mkdir(parents=True) + oversized = _make_skill_content(MAX_SKILL_CONTENT_CHARS + 5000) + oversized = oversized.replace("name: test-skill", "name: bloated") + (skill_dir / "SKILL.md").write_text(oversized, encoding="utf-8") + assert len(oversized) > MAX_SKILL_CONTENT_CHARS + + # Patch that removes content to bring it under the limit. + # Use replace_all to replace the repeated x's with a shorter string. + result = json.loads(skill_manage( + action="patch", + name="bloated", + old_string="x" * 100, + new_string="y", + replace_all=True, + )) + # Should succeed because the result is well within limits + assert result["success"] is True + + def test_patch_supporting_file_size_limit(self, isolate_skills): + """Patch on a supporting file also checks size.""" + small = _make_skill_content(1000) + json.loads(skill_manage(action="create", name="with-ref", content=small)) + # Create a supporting file + json.loads(skill_manage( + action="write_file", + name="with-ref", + file_path="references/data.md", + file_content="# Data\n\nSmall content.", + )) + # Try to patch it to be oversized + result = json.loads(skill_manage( + action="patch", + name="with-ref", + old_string="Small content.", + new_string="x" * (MAX_SKILL_CONTENT_CHARS + 100), + file_path="references/data.md", + )) + assert result["success"] is False + assert "references/data.md" in result["error"] + + +class TestWriteFileSizeLimit: + """write_file action enforces both char and byte limits.""" + + def test_write_file_over_char_limit(self, isolate_skills): + small = _make_skill_content(1000) + json.loads(skill_manage(action="create", name="file-test", content=small)) + + result = json.loads(skill_manage( + action="write_file", + name="file-test", + file_path="references/huge.md", + file_content="x" * (MAX_SKILL_CONTENT_CHARS + 1), + )) + assert result["success"] is False + assert "100,000" in result["error"] + + def test_write_file_within_limit(self, isolate_skills): + small = _make_skill_content(1000) + json.loads(skill_manage(action="create", name="file-ok", content=small)) + + result = json.loads(skill_manage( + action="write_file", + name="file-ok", + file_path="references/normal.md", + file_content="# Normal\n\n" + ("x" * 5000), + )) + assert result["success"] is True + + +class TestHandPlacedSkillsNoLimit: + """Skills dropped directly on disk are not constrained.""" + + def test_oversized_handplaced_skill_loads(self, isolate_skills, tmp_path): + """A hand-placed 200k skill can still be read via skill_view.""" + from tools.skills_tool import skill_view + + skill_dir = tmp_path / "skills" / "manual-giant" + skill_dir.mkdir(parents=True) + huge = _make_skill_content(200_000) + huge = huge.replace("name: test-skill", "name: manual-giant") + (skill_dir / "SKILL.md").write_text(huge, encoding="utf-8") + + result = json.loads(skill_view("manual-giant")) + assert "content" in result + # The full content is returned — no truncation at the storage layer + assert len(result["content"]) > MAX_SKILL_CONTENT_CHARS diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 8507a6d13..d6d2f6f78 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -82,6 +82,8 @@ SKILLS_DIR = HERMES_HOME / "skills" MAX_NAME_LENGTH = 64 MAX_DESCRIPTION_LENGTH = 1024 +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 # Characters allowed in skill names (filesystem-safe, URL-friendly) VALID_NAME_RE = re.compile(r'^[a-z0-9][a-z0-9._-]*$') @@ -177,6 +179,21 @@ def _validate_frontmatter(content: str) -> Optional[str]: return None +def _validate_content_size(content: str, label: str = "SKILL.md") -> Optional[str]: + """Check that content doesn't exceed the character limit for agent writes. + + Returns an error message or None if within bounds. + """ + if len(content) > MAX_SKILL_CONTENT_CHARS: + return ( + f"{label} content is {len(content):,} characters " + f"(limit: {MAX_SKILL_CONTENT_CHARS:,}). " + f"Consider splitting into a smaller SKILL.md with supporting files " + f"in references/ or templates/." + ) + return None + + def _resolve_skill_dir(name: str, category: str = None) -> Path: """Build the directory path for a new skill, optionally under a category.""" if category: @@ -275,6 +292,10 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An if err: return {"success": False, "error": err} + err = _validate_content_size(content) + if err: + return {"success": False, "error": err} + # Check for name collisions across all directories existing = _find_skill(name) if existing: @@ -318,6 +339,10 @@ def _edit_skill(name: str, content: str) -> Dict[str, Any]: if err: return {"success": False, "error": err} + err = _validate_content_size(content) + if err: + return {"success": False, "error": err} + existing = _find_skill(name) if not existing: return {"success": False, "error": f"Skill '{name}' not found. Use skills_list() to see available skills."} @@ -379,27 +404,29 @@ def _patch_skill( content = target.read_text(encoding="utf-8") - count = content.count(old_string) - if count == 0: + # Use the same fuzzy matching engine as the file patch tool. + # This handles whitespace normalization, indentation differences, + # escape sequences, and block-anchor matching — saving the agent + # from exact-match failures on minor formatting mismatches. + from tools.fuzzy_match import fuzzy_find_and_replace + + new_content, match_count, match_error = fuzzy_find_and_replace( + content, old_string, new_string, replace_all + ) + if match_error: # Show a short preview of the file so the model can self-correct preview = content[:500] + ("..." if len(content) > 500 else "") return { "success": False, - "error": "old_string not found in the file.", + "error": match_error, "file_preview": preview, } - if count > 1 and not replace_all: - return { - "success": False, - "error": ( - f"old_string matched {count} times. Provide more surrounding context " - f"to make the match unique, or set replace_all=true to replace all occurrences." - ), - "match_count": count, - } - - new_content = content.replace(old_string, new_string) if replace_all else content.replace(old_string, new_string, 1) + # Check size limit on the result + target_label = "SKILL.md" if not file_path else file_path + err = _validate_content_size(new_content, label=target_label) + if err: + return {"success": False, "error": err} # If patching SKILL.md, validate frontmatter is still intact if not file_path: @@ -419,10 +446,9 @@ def _patch_skill( _atomic_write_text(target, original_content) return {"success": False, "error": scan_error} - replacements = count if replace_all else 1 return { "success": True, - "message": f"Patched {'SKILL.md' if not file_path else file_path} in skill '{name}' ({replacements} replacement{'s' if replacements > 1 else ''}).", + "message": f"Patched {'SKILL.md' if not file_path else file_path} in skill '{name}' ({match_count} replacement{'s' if match_count > 1 else ''}).", } @@ -455,6 +481,21 @@ def _write_file(name: str, file_path: str, file_content: str) -> Dict[str, Any]: if not file_content and file_content != "": return {"success": False, "error": "file_content is required."} + # Check size limits + content_bytes = len(file_content.encode("utf-8")) + if content_bytes > MAX_SKILL_FILE_BYTES: + return { + "success": False, + "error": ( + f"File content is {content_bytes:,} bytes " + f"(limit: {MAX_SKILL_FILE_BYTES:,} bytes / 1 MiB). " + f"Consider splitting into smaller files." + ), + } + err = _validate_content_size(file_content, label=file_path) + if err: + return {"success": False, "error": err} + existing = _find_skill(name) if not existing: return {"success": False, "error": f"Skill '{name}' not found. Create it first with action='create'."} diff --git a/tools/skills_hub.py b/tools/skills_hub.py index c818261d7..56c89ba71 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -2525,6 +2525,22 @@ def install_from_quarantine( if install_dir.exists(): shutil.rmtree(install_dir) + # Warn (but don't block) if SKILL.md is very large + skill_md = quarantine_path / "SKILL.md" + if skill_md.exists(): + try: + skill_size = skill_md.stat().st_size + if skill_size > 100_000: + logger.warning( + "Skill '%s' has a large SKILL.md (%s chars). " + "Large skills consume significant context when loaded. " + "Consider asking the author to split it into smaller files.", + safe_skill_name, + f"{skill_size:,}", + ) + except OSError: + pass + install_dir.parent.mkdir(parents=True, exist_ok=True) shutil.move(str(quarantine_path), str(install_dir))