Compare commits
1 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
a1d536826e |
@@ -1302,9 +1302,9 @@ class TestConcurrentToolExecution:
|
||||
mock_con.assert_not_called()
|
||||
|
||||
def test_malformed_json_args_forces_sequential(self, agent):
|
||||
"""Non-dict tool arguments (e.g. JSON array) should fall back to sequential."""
|
||||
"""Unparseable tool arguments should fall back to sequential."""
|
||||
tc1 = _mock_tool_call(name="web_search", arguments='{}', call_id="c1")
|
||||
tc2 = _mock_tool_call(name="web_search", arguments='[1, 2, 3]', call_id="c2")
|
||||
tc2 = _mock_tool_call(name="web_search", arguments="NOT JSON {{{", call_id="c2")
|
||||
mock_msg = _mock_assistant_msg(content="", tool_calls=[tc1, tc2])
|
||||
messages = []
|
||||
with patch.object(agent, "_execute_tool_calls_sequential") as mock_seq:
|
||||
@@ -1384,9 +1384,10 @@ class TestConcurrentToolExecution:
|
||||
mock_msg = _mock_assistant_msg(content="", tool_calls=[tc1, tc2])
|
||||
messages = []
|
||||
|
||||
call_count = [0]
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
# Deterministic failure based on tool_call_id to avoid race conditions
|
||||
if kwargs.get("tool_call_id") == "c1":
|
||||
call_count[0] += 1
|
||||
if call_count[0] == 1:
|
||||
raise RuntimeError("boom")
|
||||
return "success"
|
||||
|
||||
|
||||
@@ -416,219 +416,3 @@ class TestEdgeCases:
|
||||
"""Verify max workers constant exists and is reasonable."""
|
||||
from run_agent import _MAX_TOOL_WORKERS
|
||||
assert 1 <= _MAX_TOOL_WORKERS <= 32
|
||||
|
||||
|
||||
# ── Integration Tests: AIAgent Concurrent Execution ───────────────────────────
|
||||
|
||||
class TestAIAgentConcurrentExecution:
|
||||
"""Exercise _execute_tool_calls_concurrent through an AIAgent instance."""
|
||||
|
||||
@pytest.fixture
|
||||
def agent(self):
|
||||
"""Minimal AIAgent with mocked OpenAI client and tool loading."""
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import patch
|
||||
from run_agent import AIAgent
|
||||
|
||||
def _make_tool_defs(*names):
|
||||
return [
|
||||
{
|
||||
"type": "function",
|
||||
"function": {
|
||||
"name": n,
|
||||
"description": f"{n} tool",
|
||||
"parameters": {"type": "object", "properties": {}},
|
||||
},
|
||||
}
|
||||
for n in names
|
||||
]
|
||||
|
||||
with (
|
||||
patch("run_agent.get_tool_definitions", return_value=_make_tool_defs("web_search", "read_file")),
|
||||
patch("run_agent.check_toolset_requirements", return_value={}),
|
||||
patch("run_agent.OpenAI"),
|
||||
):
|
||||
a = AIAgent(
|
||||
api_key="test-key-1234567890",
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
)
|
||||
a.client = MagicMock()
|
||||
return a
|
||||
|
||||
def _mock_assistant_msg(self, tool_calls=None):
|
||||
from types import SimpleNamespace
|
||||
return SimpleNamespace(content="", tool_calls=tool_calls)
|
||||
|
||||
def _mock_tool_call(self, name, arguments, call_id):
|
||||
from types import SimpleNamespace
|
||||
return SimpleNamespace(
|
||||
id=call_id,
|
||||
type="function",
|
||||
function=SimpleNamespace(name=name, arguments=json.dumps(arguments)),
|
||||
)
|
||||
|
||||
def test_two_tool_batch_executes_concurrently(self, agent):
|
||||
"""2-tool parallel batch: all execute, results ordered, 100% pass."""
|
||||
tc1 = self._mock_tool_call("read_file", {"path": "a.txt"}, "c1")
|
||||
tc2 = self._mock_tool_call("read_file", {"path": "b.txt"}, "c2")
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=[tc1, tc2])
|
||||
messages = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
return json.dumps({"file": args.get("path", ""), "content": f"content_of_{args.get('path', '')}"})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert len(messages) == 2
|
||||
assert messages[0]["tool_call_id"] == "c1"
|
||||
assert messages[1]["tool_call_id"] == "c2"
|
||||
assert "a.txt" in messages[0]["content"]
|
||||
assert "b.txt" in messages[1]["content"]
|
||||
|
||||
def test_three_tool_batch_executes_concurrently(self, agent):
|
||||
"""3-tool parallel batch: all execute, results ordered, 100% pass."""
|
||||
tcs = [
|
||||
self._mock_tool_call("web_search", {"query": f"q{i}"}, f"c{i}")
|
||||
for i in range(3)
|
||||
]
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=tcs)
|
||||
messages = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
return json.dumps({"query": args.get("query", ""), "results": [f"result_{args.get('query', '')}"]})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert len(messages) == 3
|
||||
for i, tc in enumerate(tcs):
|
||||
assert messages[i]["tool_call_id"] == tc.id
|
||||
assert f"q{i}" in messages[i]["content"]
|
||||
|
||||
def test_four_tool_batch_executes_concurrently(self, agent):
|
||||
"""4-tool parallel batch: all execute, results ordered, 100% pass."""
|
||||
tcs = [
|
||||
self._mock_tool_call("read_file", {"path": f"file{i}.txt"}, f"c{i}")
|
||||
for i in range(4)
|
||||
]
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=tcs)
|
||||
messages = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
return json.dumps({"path": args.get("path", ""), "size": 100})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert len(messages) == 4
|
||||
for i, tc in enumerate(tcs):
|
||||
assert messages[i]["tool_call_id"] == tc.id
|
||||
assert f"file{i}.txt" in messages[i]["content"]
|
||||
|
||||
def test_mixed_read_and_search_batch(self, agent):
|
||||
"""read_file + search_files: safe parallel, different scopes."""
|
||||
tc1 = self._mock_tool_call("read_file", {"path": "config.yaml"}, "c1")
|
||||
tc2 = self._mock_tool_call("web_search", {"query": "provider"}, "c2")
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=[tc1, tc2])
|
||||
messages = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
return json.dumps({"tool": name, "args": args})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert len(messages) == 2
|
||||
assert messages[0]["tool_call_id"] == "c1"
|
||||
assert messages[1]["tool_call_id"] == "c2"
|
||||
assert "config.yaml" in messages[0]["content"]
|
||||
assert "provider" in messages[1]["content"]
|
||||
|
||||
def test_concurrent_pass_rate_report(self, agent):
|
||||
"""Simulate 2/3/4-tool batches and report pass rate."""
|
||||
batch_sizes = [2, 3, 4]
|
||||
pass_rates = {}
|
||||
|
||||
for size in batch_sizes:
|
||||
tcs = [
|
||||
self._mock_tool_call("web_search", {"query": f"q{i}"}, f"c{i}")
|
||||
for i in range(size)
|
||||
]
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=tcs)
|
||||
messages = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
return json.dumps({"ok": True, "query": args.get("query", "")})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
passed = sum(1 for m in messages if "ok" in m.get("content", ""))
|
||||
pass_rates[size] = passed / size if size > 0 else 0.0
|
||||
|
||||
for size, rate in pass_rates.items():
|
||||
assert rate == 1.0, f"Expected 100% pass rate for {size}-tool batch, got {rate:.0%}"
|
||||
|
||||
def test_gemma4_style_two_read_files(self, agent):
|
||||
"""Gemma 4 may issue two reads simultaneously — verify both returned."""
|
||||
tc1 = self._mock_tool_call("read_file", {"path": "src/main.py"}, "c1")
|
||||
tc2 = self._mock_tool_call("read_file", {"path": "src/utils.py"}, "c2")
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=[tc1, tc2])
|
||||
messages = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
return json.dumps({"content": f"# {args['path']}\nprint('hello')"})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert len(messages) == 2
|
||||
assert "main.py" in messages[0]["content"]
|
||||
assert "utils.py" in messages[1]["content"]
|
||||
|
||||
def test_gemma4_style_three_reads(self, agent):
|
||||
"""Gemma 4 may issue 3 reads for different files — all returned."""
|
||||
tcs = [
|
||||
self._mock_tool_call("read_file", {"path": f"mod{i}.py"}, f"c{i}")
|
||||
for i in range(3)
|
||||
]
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=tcs)
|
||||
messages = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
return json.dumps({"content": f"# {args['path']}"})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert len(messages) == 3
|
||||
for i in range(3):
|
||||
assert f"mod{i}.py" in messages[i]["content"]
|
||||
|
||||
def test_mixed_safe_and_write_tools_parallel(self, agent):
|
||||
"""Mix of read (safe) and write (path-scoped) on different paths — parallel."""
|
||||
tc1 = self._mock_tool_call("read_file", {"path": "input.txt"}, "c1")
|
||||
tc2 = self._mock_tool_call("write_file", {"path": "output.txt", "content": "x"}, "c2")
|
||||
tc3 = self._mock_tool_call("read_file", {"path": "config.txt"}, "c3")
|
||||
mock_msg = self._mock_assistant_msg(tool_calls=[tc1, tc2, tc3])
|
||||
messages = []
|
||||
|
||||
call_order = []
|
||||
|
||||
def fake_handle(name, args, task_id, **kwargs):
|
||||
call_order.append(name)
|
||||
return json.dumps({"tool": name, "path": args.get("path", "")})
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=fake_handle):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert len(messages) == 3
|
||||
# Results ordered by tool call ID, not completion order
|
||||
assert messages[0]["tool_call_id"] == "c1"
|
||||
assert messages[1]["tool_call_id"] == "c2"
|
||||
assert messages[2]["tool_call_id"] == "c3"
|
||||
# All three should have executed
|
||||
assert len(call_order) == 3
|
||||
|
||||
@@ -308,12 +308,12 @@ word word
|
||||
content = """\
|
||||
---
|
||||
name: test-skill
|
||||
description: A test skill.
|
||||
description: A test skill with enough content to pass the minimum length validation check of one hundred characters.
|
||||
---
|
||||
|
||||
# Test
|
||||
|
||||
word word
|
||||
word word word word word word word word word word
|
||||
"""
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", content)
|
||||
@@ -484,3 +484,185 @@ class TestSkillManageDispatcher:
|
||||
raw = skill_manage(action="create", name="test-skill", content=VALID_SKILL_CONTENT)
|
||||
result = json.loads(raw)
|
||||
assert result["success"] is True
|
||||
|
||||
|
||||
|
||||
class TestPokaYokeValidation:
|
||||
"""Tests for poka-yoke auto-revert functionality (#837)."""
|
||||
|
||||
def test_short_skill_md_reverts(self, tmp_path):
|
||||
"""SKILL.md shorter than 100 chars should be reverted."""
|
||||
short_content = """---
|
||||
name: test-skill
|
||||
description: Test
|
||||
---
|
||||
|
||||
Short
|
||||
"""
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
result = _edit_skill("my-skill", short_content)
|
||||
|
||||
assert result["success"] is False
|
||||
assert "too short" in result["error"].lower()
|
||||
|
||||
# Verify the original file is preserved
|
||||
skill_md = tmp_path / "my-skill" / "SKILL.md"
|
||||
content = skill_md.read_text()
|
||||
assert "test-skill" in content # Original content preserved
|
||||
|
||||
def test_truncated_skill_reverts(self, tmp_path):
|
||||
"""Truncated YAML frontmatter should be reverted."""
|
||||
truncated = """---
|
||||
name: test-skill
|
||||
description: Test skill with enough content to pass minimum length validation check.
|
||||
---
|
||||
|
||||
# Test
|
||||
|
||||
This is a longer body section with plenty of text to ensure the content exceeds the minimum one hundred character requirement for SKILL.md files.
|
||||
"""
|
||||
# Chop it off to simulate truncation
|
||||
truncated = truncated[:80]
|
||||
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
result = _edit_skill("my-skill", truncated)
|
||||
|
||||
assert result["success"] is False
|
||||
|
||||
def test_linked_files_validation(self, tmp_path):
|
||||
"""Missing linked_files should cause revert."""
|
||||
content_with_links = """---
|
||||
name: test-skill
|
||||
description: Test skill with enough content to pass minimum length validation check.
|
||||
linked_files:
|
||||
- references/nonexistent.md
|
||||
---
|
||||
|
||||
# Test
|
||||
|
||||
This is a longer body section with plenty of text to ensure the content exceeds the minimum one hundred character requirement for SKILL.md files.
|
||||
"""
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
result = _edit_skill("my-skill", content_with_links)
|
||||
|
||||
assert result["success"] is False
|
||||
assert "linked files missing" in result["error"].lower()
|
||||
|
||||
def test_valid_linked_files_pass(self, tmp_path):
|
||||
"""Existing linked_files should pass validation."""
|
||||
content_with_links = """---
|
||||
name: test-skill
|
||||
description: Test skill with enough content to pass minimum length validation check.
|
||||
linked_files:
|
||||
- references/exists.md
|
||||
---
|
||||
|
||||
# Test
|
||||
|
||||
This is a longer body section with plenty of text to ensure the content exceeds the minimum one hundred character requirement for SKILL.md files.
|
||||
"""
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
# Create the linked file
|
||||
ref_dir = tmp_path / "my-skill" / "references"
|
||||
ref_dir.mkdir(parents=True, exist_ok=True)
|
||||
(ref_dir / "exists.md").write_text("# Reference")
|
||||
|
||||
result = _edit_skill("my-skill", content_with_links)
|
||||
|
||||
assert result["success"] is True
|
||||
|
||||
|
||||
class TestHistoryRegistry:
|
||||
"""Tests for history registry functionality (#837)."""
|
||||
|
||||
def test_history_saved_on_edit(self, tmp_path):
|
||||
"""Editing a skill should save the original to history."""
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
|
||||
# Make an edit
|
||||
new_content = """---
|
||||
name: test-skill
|
||||
description: Updated description that is longer than one hundred characters to pass validation.
|
||||
---
|
||||
|
||||
# Updated Test
|
||||
|
||||
This body has more content to ensure it passes the minimum length check of one hundred characters.
|
||||
"""
|
||||
result = _edit_skill("my-skill", new_content)
|
||||
assert result["success"] is True
|
||||
|
||||
# Check history was saved
|
||||
history_dir = tmp_path / ".history" / "my-skill"
|
||||
assert history_dir.exists()
|
||||
history_files = list(history_dir.glob("*.md"))
|
||||
assert len(history_files) == 1
|
||||
|
||||
def test_history_pruned_to_three(self, tmp_path):
|
||||
"""Only last 3 history versions should be kept."""
|
||||
from tools.skill_manager_tool import _save_to_history
|
||||
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
|
||||
# Save 5 versions to history
|
||||
for i in range(5):
|
||||
content = f"""---
|
||||
name: test-skill
|
||||
description: Version {i} that is long enough to pass minimum length validation check of one hundred characters.
|
||||
---
|
||||
|
||||
# Version {i}
|
||||
|
||||
This is the body content for version {i} that ensures we meet the minimum length requirement.
|
||||
"""
|
||||
_save_to_history("my-skill", content, timestamp=1000 + i)
|
||||
|
||||
# Check only 3 history files remain
|
||||
history_dir = tmp_path / ".history" / "my-skill"
|
||||
history_files = sorted(history_dir.glob("*.md"))
|
||||
assert len(history_files) == 3
|
||||
# Should be the last 3 (timestamps 1002, 1003, 1004)
|
||||
assert "1002" in str(history_files[0])
|
||||
|
||||
def test_revert_to_history(self, tmp_path):
|
||||
"""Should be able to revert to a history version."""
|
||||
from tools.skill_manager_tool import _revert_to_history, _get_history_versions
|
||||
|
||||
with _skill_dir(tmp_path):
|
||||
_create_skill("my-skill", VALID_SKILL_CONTENT)
|
||||
skill_md = tmp_path / "my-skill" / "SKILL.md"
|
||||
|
||||
# Save original to history
|
||||
original = skill_md.read_text()
|
||||
from tools.skill_manager_tool import _save_to_history
|
||||
_save_to_history("my-skill", original)
|
||||
|
||||
# Edit the skill
|
||||
new_content = """---
|
||||
name: test-skill
|
||||
description: Updated description that is longer than one hundred characters to pass validation.
|
||||
---
|
||||
|
||||
# Updated
|
||||
|
||||
This body has more content to ensure it passes the minimum length check of one hundred characters.
|
||||
"""
|
||||
_edit_skill("my-skill", new_content)
|
||||
|
||||
# Verify edit was applied
|
||||
assert "Updated" in skill_md.read_text()
|
||||
|
||||
# Revert to history
|
||||
error = _revert_to_history("my-skill", skill_md, version=0)
|
||||
assert error is None
|
||||
|
||||
# Verify revert worked
|
||||
content = skill_md.read_text()
|
||||
assert "test-skill" in content
|
||||
assert "A test skill" in content
|
||||
|
||||
@@ -322,12 +322,112 @@ def _cleanup_old_backups(file_path: Path, max_backups: int = MAX_BACKUPS_PER_FIL
|
||||
break
|
||||
|
||||
|
||||
|
||||
|
||||
# History registry for rollback (#837)
|
||||
MAX_HISTORY_VERSIONS = 3
|
||||
|
||||
|
||||
def _history_dir_for_skill(skill_name: str) -> Path:
|
||||
"""Return the history directory path for a skill."""
|
||||
return SKILLS_DIR / ".history" / skill_name
|
||||
|
||||
|
||||
def _save_to_history(skill_name: str, content: str, timestamp: Optional[int] = None) -> Optional[Path]:
|
||||
"""Save a version of the skill to the history registry.
|
||||
|
||||
History is stored in ~/.hermes/skills/.history/<skill-name>/<timestamp>.md
|
||||
Keeps the last MAX_HISTORY_VERSIONS versions.
|
||||
|
||||
Returns the path to the saved history file, or None if not saved.
|
||||
"""
|
||||
if timestamp is None:
|
||||
timestamp = int(time.time())
|
||||
|
||||
history_dir = _history_dir_for_skill(skill_name)
|
||||
history_dir.mkdir(parents=True, exist_ok=True)
|
||||
|
||||
history_file = history_dir / f"{timestamp}.md"
|
||||
_atomic_write_text(history_file, content)
|
||||
|
||||
# Clean up old history versions
|
||||
_cleanup_history(skill_name)
|
||||
|
||||
return history_file
|
||||
|
||||
|
||||
def _cleanup_history(skill_name: str, max_versions: int = MAX_HISTORY_VERSIONS) -> None:
|
||||
"""Prune old history versions, keeping only the most recent max_versions."""
|
||||
history_dir = _history_dir_for_skill(skill_name)
|
||||
if not history_dir.exists():
|
||||
return
|
||||
|
||||
try:
|
||||
# Get all history files sorted by modification time (oldest first)
|
||||
history_files = sorted(
|
||||
[f for f in history_dir.iterdir() if f.suffix == '.md' and f.is_file()],
|
||||
key=lambda p: p.stat().st_mtime,
|
||||
)
|
||||
except OSError:
|
||||
return
|
||||
|
||||
# Remove oldest files if we have more than max_versions
|
||||
while len(history_files) > max_versions:
|
||||
try:
|
||||
history_files.pop(0).unlink()
|
||||
except OSError:
|
||||
break
|
||||
|
||||
|
||||
def _get_history_versions(skill_name: str) -> List[Path]:
|
||||
"""Get list of history versions for a skill, newest first."""
|
||||
history_dir = _history_dir_for_skill(skill_name)
|
||||
if not history_dir.exists():
|
||||
return []
|
||||
|
||||
try:
|
||||
return sorted(
|
||||
[f for f in history_dir.iterdir() if f.suffix == '.md' and f.is_file()],
|
||||
key=lambda p: p.stat().st_mtime,
|
||||
reverse=True,
|
||||
)
|
||||
except OSError:
|
||||
return []
|
||||
|
||||
|
||||
def _revert_to_history(skill_name: str, skill_md_path: Path, version: int = 0) -> Optional[str]:
|
||||
"""Revert a skill to a previous history version.
|
||||
|
||||
Args:
|
||||
skill_name: Name of the skill
|
||||
skill_md_path: Path to the current SKILL.md
|
||||
version: Which history version to revert to (0 = most recent, 1 = second most recent, etc.)
|
||||
|
||||
Returns:
|
||||
Error message if revert failed, None if successful
|
||||
"""
|
||||
history_versions = _get_history_versions(skill_name)
|
||||
if not history_versions:
|
||||
return "No history versions available to revert to."
|
||||
|
||||
if version >= len(history_versions):
|
||||
return f"History version {version} not found (only {len(history_versions)} versions available)."
|
||||
|
||||
target_version = history_versions[version]
|
||||
|
||||
try:
|
||||
content = target_version.read_text(encoding="utf-8")
|
||||
_atomic_write_text(skill_md_path, content)
|
||||
return None
|
||||
except Exception as exc:
|
||||
return f"Failed to revert to history version: {exc}"
|
||||
|
||||
def _validate_written_file(file_path: Path, is_skill_md: bool = False) -> Optional[str]:
|
||||
"""Re-read a file from disk and validate it after writing.
|
||||
|
||||
Catches filesystem-level issues (truncation, encoding errors, empty
|
||||
writes) that pre-write validation cannot detect. For SKILL.md files
|
||||
the frontmatter is also re-validated.
|
||||
the frontmatter is also re-validated and linked_files are verified.
|
||||
|
||||
Returns an error message, or *None* if the file looks healthy.
|
||||
"""
|
||||
@@ -341,11 +441,69 @@ def _validate_written_file(file_path: Path, is_skill_md: bool = False) -> Option
|
||||
if len(content) == 0:
|
||||
return "File is empty after write (possible truncation)."
|
||||
|
||||
# Minimum content length check for SKILL.md only (#837)
|
||||
if is_skill_md and len(content) < 100:
|
||||
return f"SKILL.md is too short after write ({len(content)} chars, minimum 100)."
|
||||
|
||||
if is_skill_md:
|
||||
err = _validate_frontmatter(content)
|
||||
if err:
|
||||
return f"Post-write validation failed: {err}"
|
||||
|
||||
# Verify linked_files exist (#837)
|
||||
err = _validate_linked_files(content, file_path.parent)
|
||||
if err:
|
||||
return f"Post-write validation failed: {err}"
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def _validate_linked_files(content: str, skill_dir: Path) -> Optional[str]:
|
||||
"""Validate that all files referenced in linked_files exist.
|
||||
|
||||
Parses the SKILL.md frontmatter and checks that any linked_files
|
||||
entries point to files that actually exist in the skill directory.
|
||||
|
||||
Returns an error message, or *None* if all linked files exist.
|
||||
"""
|
||||
if not content.startswith("---"):
|
||||
return None
|
||||
|
||||
end_match = re.search(r'\n---\s*\n', content[3:])
|
||||
if not end_match:
|
||||
return None
|
||||
|
||||
yaml_content = content[3:end_match.start() + 3]
|
||||
try:
|
||||
parsed = yaml.safe_load(yaml_content)
|
||||
except yaml.YAMLError:
|
||||
return None
|
||||
|
||||
if not isinstance(parsed, dict):
|
||||
return None
|
||||
|
||||
linked_files = parsed.get("linked_files", [])
|
||||
if not linked_files:
|
||||
return None
|
||||
|
||||
missing = []
|
||||
for lf in linked_files:
|
||||
if isinstance(lf, dict):
|
||||
file_ref = lf.get("file") or lf.get("path", "")
|
||||
elif isinstance(lf, str):
|
||||
file_ref = lf
|
||||
else:
|
||||
continue
|
||||
|
||||
if file_ref:
|
||||
# Resolve relative to skill directory
|
||||
target = skill_dir / file_ref
|
||||
if not target.exists():
|
||||
missing.append(file_ref)
|
||||
|
||||
if missing:
|
||||
return f"Linked files missing: {', '.join(missing)}"
|
||||
|
||||
return None
|
||||
|
||||
|
||||
@@ -483,6 +641,13 @@ def _edit_skill(name: str, content: str) -> Dict[str, Any]:
|
||||
|
||||
skill_md = existing["path"] / "SKILL.md"
|
||||
|
||||
# Save original to history before modification (#837)
|
||||
try:
|
||||
original_content = skill_md.read_text(encoding="utf-8")
|
||||
_save_to_history(name, original_content)
|
||||
except (OSError, UnicodeDecodeError):
|
||||
pass # If we can't read original, proceed without history
|
||||
|
||||
# --- Transactional write-validate-commit-or-rollback ---
|
||||
backup_path = _backup_skill_file(skill_md)
|
||||
_atomic_write_text(skill_md, content)
|
||||
@@ -598,6 +763,14 @@ def _patch_skill(
|
||||
|
||||
is_skill_md = not file_path
|
||||
|
||||
# Save original to history when patching SKILL.md (#837)
|
||||
if is_skill_md:
|
||||
try:
|
||||
original_content = target.read_text(encoding="utf-8")
|
||||
_save_to_history(name, original_content)
|
||||
except (OSError, UnicodeDecodeError):
|
||||
pass
|
||||
|
||||
# --- Transactional write-validate-commit-or-rollback ---
|
||||
backup_path = _backup_skill_file(target)
|
||||
_atomic_write_text(target, new_content)
|
||||
|
||||
Reference in New Issue
Block a user