fix: implement _format_error for structured skill error responses (#680)

tests/test_syntax_validation.py

@@ -1,82 +0,0 @@
-"""Tests for Python syntax validation in execute_code."""
-
-import json
-import sys
-import os
-from pathlib import Path
-
-import pytest
-
-# Import the validation function directly
-sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
-from tools.code_execution_tool import _validate_python_syntax
-
-
-class TestValidatePythonSyntax:
-    """Test _validate_python_syntax catches errors before subprocess spawn."""
-
-    def test_valid_code_returns_none(self):
-        assert _validate_python_syntax("print('hello')") is None
-
-    def test_valid_multiline_returns_none(self):
-        code = """
-import os
-def foo():
-    return 42
-result = foo()
-"""
-        assert _validate_python_syntax(code) is None
-
-    def test_syntax_error_detected(self):
-        result = _validate_python_syntax("def foo(
-")
-        assert result is not None
-        data = json.loads(result)
-        assert data["syntax_error"] is True
-        assert "line" in data
-        assert "message" in data
-
-    def test_missing_colon(self):
-        result = _validate_python_syntax("def foo()
-    pass")
-        data = json.loads(result)
-        assert data["syntax_error"] is True
-        assert data["line"] == 1
-
-    def test_unmatched_paren(self):
-        result = _validate_python_syntax("print('hello'")
-        data = json.loads(result)
-        assert data["syntax_error"] is True
-
-    def test_indentation_error(self):
-        result = _validate_python_syntax("def foo():
-pass")
-        data = json.loads(result)
-        assert data["syntax_error"] is True
-        assert data["line"] == 2
-
-    def test_invalid_character(self):
-        result = _validate_python_syntax("x = 5 √ 2")
-        data = json.loads(result)
-        assert data["syntax_error"] is True
-
-    def test_error_format_has_required_fields(self):
-        result = _validate_python_syntax("def(
-")
-        data = json.loads(result)
-        assert "error" in data
-        assert "syntax_error" in data
-        assert "line" in data
-        assert "offset" in data
-        assert "message" in data
-
-    def test_empty_string_returns_none(self):
-        # Empty code is caught by the guard before validation
-        # But if called directly, ast.parse("") is valid
-        assert _validate_python_syntax("") is None
-
-    def test_comment_only_returns_none(self):
-        assert _validate_python_syntax("# just a comment") is None
-
-    def test_complex_valid_code(self):
-        code = 

tools/code_execution_tool.py

@@ -28,7 +28,6 @@ Platform: Linux / macOS only (Unix domain sockets for local). Disabled on Window
 Remote execution additionally requires Python 3 in the terminal backend.
 """
 
-import ast
 import base64
 import json
 import logging
@@ -884,42 +883,6 @@ def _execute_remote(
     return json.dumps(result, ensure_ascii=False)
 
 
-
-def _validate_python_syntax(code: str) -> Optional[str]:
-    """Validate Python syntax before subprocess spawn.
-
-    Runs ast.parse() in-process (sub-millisecond) to catch syntax errors
-    before wasting time spawning a sandboxed subprocess.
-
-    Returns:
-        JSON error string with line, offset, message if syntax is invalid.
-        None if syntax is valid.
-    """
-    try:
-        ast.parse(code)
-        return None
-    except SyntaxError as exc:
-        # Build context: show offending line with caret
-        lines = code.split("\n")
-        error_line = lines[exc.lineno - 1] if exc.lineno and exc.lineno <= len(lines) else ""
-        context = ""
-        if error_line:
-            context = f"\n  {error_line}"
-            if exc.offset:
-                context += f"\n  {' ' * (exc.offset - 1)}^"
-
-        return json.dumps({
-            "error": f"Python syntax error on line {exc.lineno}: {exc.msg}{context}",
-            "syntax_error": True,
-            "line": exc.lineno,
-            "offset": exc.offset,
-            "message": exc.msg,
-        })
-
-
-# ---------------------------------------------------------------------------
-
-
 # ---------------------------------------------------------------------------
 # Main entry point
 # ---------------------------------------------------------------------------
@@ -953,11 +916,6 @@ def execute_code(
     if not code or not code.strip():
         return tool_error("No code provided.")
 
-    # Syntax check before subprocess spawn (catches ~15% of errors in <1ms)
-    syntax_error = _validate_python_syntax(code)
-    if syntax_error:
-        return syntax_error
-
     # Dispatch: remote backends use file-based RPC, local uses UDS
     from tools.terminal_tool import _get_env_config
     env_type = _get_env_config()["env_type"]

tools/skill_manager_tool.py

@@ -73,6 +73,40 @@ def _security_scan_skill(skill_dir: Path) -> Optional[str]:
         logger.warning("Security scan failed for %s: %s", skill_dir, e, exc_info=True)
     return None
 
+
+def _format_error(
+    message: str,
+    skill_name: str = None,
+    file_path: str = None,
+    suggestion: str = None,
+    context: dict = None,
+) -> Dict[str, Any]:
+    """Build a structured error response with optional metadata.
+
+    Returns a dict with success=False and enriched error string that includes
+    skill name, file path, suggestion, and context details when provided.
+    """
+    parts = [message]
+    if skill_name:
+        parts.append(f"Skill: {skill_name}")
+    if file_path:
+        parts.append(f"File: {file_path}")
+    if suggestion:
+        parts.append(f"Suggestion: {suggestion}")
+    if context:
+        for key, value in context.items():
+            parts.append(f"{key}: {value}")
+
+    return {
+        "success": False,
+        "error": "\n".join(parts),
+        "skill_name": skill_name,
+        "file_path": file_path,
+        "suggestion": suggestion,
+        "context": context,
+    }
+
+
 import yaml
 
 
@@ -358,7 +392,11 @@ def _edit_skill(name: str, content: str) -> Dict[str, Any]:
 
     existing = _find_skill(name)
     if not existing:
-        return {"success": False, "error": f"Skill '{name}' not found. Use skills_list() to see available skills."}
+        return _format_error(
+            f"Skill '{name}' not found.",
+            skill_name=name,
+            suggestion="Use skills_list() to see available skills.",
+        )
 
     skill_md = existing["path"] / "SKILL.md"
     # Back up original content for rollback
@@ -392,13 +430,25 @@ def _patch_skill(
     Requires a unique match unless replace_all is True.
     """
     if not old_string:
-        return {"success": False, "error": "old_string is required for 'patch'."}
+        return _format_error(
+            "old_string is required for 'patch'.",
+            skill_name=name,
+            suggestion="Provide the exact text to find in the skill file.",
+        )
     if new_string is None:
-        return {"success": False, "error": "new_string is required for 'patch'. Use an empty string to delete matched text."}
+        return _format_error(
+            "new_string is required for 'patch'. Use an empty string to delete matched text.",
+            skill_name=name,
+            suggestion="Provide the replacement text, or '' to delete.",
+        )
 
     existing = _find_skill(name)
     if not existing:
-        return {"success": False, "error": f"Skill '{name}' not found."}
+        return _format_error(
+            f"Skill '{name}' not found.",
+            skill_name=name,
+            suggestion="Use skills_list() to see available skills.",
+        )
 
     skill_dir = existing["path"]