diff --git a/tests/test_skill_manager_error_context.py b/tests/test_skill_manager_error_context.py index fd22aca1a..166f5dcc4 100644 --- a/tests/test_skill_manager_error_context.py +++ b/tests/test_skill_manager_error_context.py @@ -6,7 +6,7 @@ Verifies that error messages include file paths, context, and suggestions. import pytest from pathlib import Path from unittest.mock import patch, MagicMock -from tools.skill_manager_tool import _format_error, _edit_skill, _patch_skill +from tools.skill_manager_tool import _format_error, _edit_skill, _patch_skill, skill_manage class TestFormatError: @@ -79,6 +79,25 @@ Body content here. assert "nonexistent" in result["error"] assert "skills_list()" in result.get("suggestion", "") + @patch('tools.skill_manager_tool._find_skill') + def test_yaml_parse_error_includes_file_path_and_line_number(self, mock_find, tmp_path): + """Invalid YAML should report target file path and parser line information.""" + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("old", encoding="utf-8") + mock_find.return_value = {"path": skill_dir} + + bad_content = """--- +name: my-skill +description: [broken +--- +Body. +""" + result = _edit_skill("my-skill", bad_content) + assert result["success"] is False + assert str(skill_dir / "SKILL.md") in result["error"] + assert "line" in result["error"].lower() + class TestPatchSkillErrors: """Test improved error messages in _patch_skill.""" @@ -106,6 +125,28 @@ class TestPatchSkillErrors: assert "nonexistent" in result["error"] assert "skills_list()" in result.get("suggestion", "") + @patch('tools.skill_manager_tool._find_skill') + def test_pattern_match_error_includes_state_info(self, mock_find, tmp_path): + """Patch failures should include file path and patch state info.""" + skill_dir = tmp_path / "state-skill" + skill_dir.mkdir() + target = skill_dir / "SKILL.md" + target.write_text("---\nname: state-skill\ndescription: desc\n---\n\nBody content here.\n", encoding="utf-8") + mock_find.return_value = {"path": skill_dir} + + result = _patch_skill("state-skill", "missing pattern", "new text", replace_all=False) + assert result["success"] is False + assert str(target) in result["error"] + assert "replace_all" in result["error"] + assert "False" in result["error"] + + +class TestSkillManageEntryPoint: + def test_patch_missing_old_string_returns_json_error(self): + result = skill_manage(action="patch", name="demo-skill", old_string="", new_string="x") + assert isinstance(result, str) + assert "old_string is required" in result + if __name__ == "__main__": pytest.main([__file__, "-v"]) diff --git a/tools/registry.py b/tools/registry.py index 432e1f074..bcaa7427d 100644 --- a/tools/registry.py +++ b/tools/registry.py @@ -21,6 +21,18 @@ from typing import Callable, Dict, List, Optional, Set logger = logging.getLogger(__name__) +def tool_error(message: str, success: bool = False, **extra) -> str: + """Return a standardized JSON error payload for tool handlers. + + Many tools import this helper directly from the registry module. + Keeping it here avoids circular helper imports and ensures a consistent + error envelope across tools. + """ + payload = {"success": success, "error": message} + payload.update(extra) + return json.dumps(payload, ensure_ascii=False) + + class ToolEntry: """Metadata for a single registered tool.""" diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index f6886354c..32bfe82fc 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -186,43 +186,58 @@ def _validate_category(category: Optional[str]) -> Optional[str]: return None -def _validate_frontmatter(content: str) -> Optional[str]: - """ - Validate that SKILL.md content has proper frontmatter with required fields. - Returns error message or None if valid. +def _validate_frontmatter_details(content: str) -> Tuple[Optional[str], Optional[dict]]: + """Validate SKILL.md frontmatter and return optional structured context. + + Returns: + (error_message, context_dict) where both are None when valid. """ if not content.strip(): - return "Content cannot be empty. SKILL.md must have YAML frontmatter and body content." + return "Content cannot be empty.", None if not content.startswith("---"): - return "SKILL.md must start with YAML frontmatter delimiter (---). Expected format: ---\nname: skill-name\ndescription: ...\n---\n" + return "SKILL.md must start with YAML frontmatter (---). See existing skills for format.", None end_match = re.search(r'\n---\s*\n', content[3:]) if not end_match: - return "SKILL.md frontmatter is not closed. Ensure you have a closing '---' line." + return "SKILL.md frontmatter is not closed. Ensure you have a closing '---' line.", None yaml_content = content[3:end_match.start() + 3] try: parsed = yaml.safe_load(yaml_content) except yaml.YAMLError as e: - return f"YAML frontmatter parse error: {e}" + context = {} + problem_mark = getattr(e, "problem_mark", None) + if problem_mark is not None: + context["line"] = problem_mark.line + 1 + context["column"] = problem_mark.column + 1 + return f"YAML frontmatter parse error: {e}", (context or None) if not isinstance(parsed, dict): - return "Frontmatter must be a YAML mapping (key: value pairs). Check for syntax errors in the YAML." + return "Frontmatter must be a YAML mapping (key: value pairs). Check for syntax errors in the YAML.", None if "name" not in parsed: - return "Frontmatter must include 'name' field." + return "Frontmatter must include 'name' field.", None if "description" not in parsed: - return "Frontmatter must include 'description' field." + return "Frontmatter must include 'description' field.", None if len(str(parsed["description"])) > MAX_DESCRIPTION_LENGTH: - return f"Description exceeds {MAX_DESCRIPTION_LENGTH} characters." + return f"Description exceeds {MAX_DESCRIPTION_LENGTH} characters.", None body = content[end_match.end() + 3:].strip() if not body: - return "SKILL.md must have content after the frontmatter (instructions, procedures, etc.)." + return "SKILL.md must have content after the frontmatter (instructions, procedures, etc.).", None - return None + return None, None + + +def _validate_frontmatter(content: str) -> Optional[str]: + """ + Validate that SKILL.md content has proper frontmatter with required fields. + Returns error message or None if valid. + """ + err, _context = _validate_frontmatter_details(content) + return err def _validate_content_size(content: str, label: str = "SKILL.md") -> Optional[str]: @@ -256,7 +271,15 @@ def _find_skill(name: str) -> Optional[Dict[str, Any]]: {"path": Path} or None. """ from agent.skill_utils import get_all_skills_dirs - for skills_dir in get_all_skills_dirs(): + + candidate_dirs = [] + if isinstance(SKILLS_DIR, Path): + candidate_dirs.append(SKILLS_DIR) + for extra_dir in get_all_skills_dirs(): + if extra_dir not in candidate_dirs: + candidate_dirs.append(extra_dir) + + for skills_dir in candidate_dirs: if not skills_dir.exists(): continue for skill_md in skills_dir.rglob("SKILL.md"): @@ -351,10 +374,19 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An if err: return {"success": False, "error": err} + skill_dir = _resolve_skill_dir(name, category) + skill_md = skill_dir / "SKILL.md" + # Validate content - err = _validate_frontmatter(content) + err, context = _validate_frontmatter_details(content) if err: - return {"success": False, "error": err} + return _format_error( + err, + skill_name=name, + file_path=str(skill_md), + context=context, + suggestion="Fix the YAML frontmatter before creating the skill." + ) err = _validate_content_size(content) if err: @@ -371,11 +403,9 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An ) # Create the skill directory - skill_dir = _resolve_skill_dir(name, category) skill_dir.mkdir(parents=True, exist_ok=True) # Write SKILL.md atomically - skill_md = skill_dir / "SKILL.md" _atomic_write_text(skill_md, content) # Security scan — roll back on block @@ -406,14 +436,6 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An def _edit_skill(name: str, content: str) -> Dict[str, Any]: """Replace the SKILL.md of any existing skill (full rewrite).""" - err = _validate_frontmatter(content) - 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 _format_error( @@ -423,6 +445,21 @@ def _edit_skill(name: str, content: str) -> Dict[str, Any]: ) skill_md = existing["path"] / "SKILL.md" + + err, context = _validate_frontmatter_details(content) + if err: + return _format_error( + err, + skill_name=name, + file_path=str(skill_md), + context=context, + suggestion="Fix the YAML frontmatter before updating the skill." + ) + + err = _validate_content_size(content) + if err: + return {"success": False, "error": err} + # Back up original content for rollback original_content = skill_md.read_text(encoding="utf-8") if skill_md.exists() else None _atomic_write_text(skill_md, content) @@ -507,7 +544,7 @@ def _patch_skill( # from exact-match failures on minor formatting mismatches. from tools.fuzzy_match import fuzzy_find_and_replace - new_content, match_count, _strategy, match_error = fuzzy_find_and_replace( + new_content, match_count, match_error = fuzzy_find_and_replace( content, old_string, new_string, replace_all ) if match_error: @@ -517,7 +554,12 @@ def _patch_skill( f"Pattern match failed: {match_error}", skill_name=name, file_path=str(target), - context={"file_preview": preview[:200] + "..." if len(preview) > 200 else preview}, + context={ + "replace_all": replace_all, + "target_exists": target.exists(), + "content_chars": len(content), + "file_preview": preview[:200] + "..." if len(preview) > 200 else preview, + }, suggestion="Check for whitespace differences, indentation, or escaping issues in old_string" ) @@ -529,12 +571,13 @@ def _patch_skill( # If patching SKILL.md, validate frontmatter is still intact if not file_path: - err = _validate_frontmatter(new_content) + err, validation_context = _validate_frontmatter_details(new_content) if err: return _format_error( f"Patch would break SKILL.md structure: {err}", skill_name=name, file_path=str(target), + context=validation_context, suggestion="Ensure the patch doesn't corrupt YAML frontmatter (--- delimiters and key: value format)" )