Compare commits
2 Commits
fix/syntax
...
fix/memory
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
95bde9d3cb | ||
| 3b89bfbab2 |
@@ -144,7 +144,8 @@ class TestMemoryStoreReplace:
|
|||||||
def test_replace_no_match(self, store):
|
def test_replace_no_match(self, store):
|
||||||
store.add("memory", "fact A")
|
store.add("memory", "fact A")
|
||||||
result = store.replace("memory", "nonexistent", "new")
|
result = store.replace("memory", "nonexistent", "new")
|
||||||
assert result["success"] is False
|
assert result["success"] is True
|
||||||
|
assert result["result"] == "no_match"
|
||||||
|
|
||||||
def test_replace_ambiguous_match(self, store):
|
def test_replace_ambiguous_match(self, store):
|
||||||
store.add("memory", "server A runs nginx")
|
store.add("memory", "server A runs nginx")
|
||||||
@@ -177,7 +178,8 @@ class TestMemoryStoreRemove:
|
|||||||
|
|
||||||
def test_remove_no_match(self, store):
|
def test_remove_no_match(self, store):
|
||||||
result = store.remove("memory", "nonexistent")
|
result = store.remove("memory", "nonexistent")
|
||||||
assert result["success"] is False
|
assert result["success"] is True
|
||||||
|
assert result["result"] == "no_match"
|
||||||
|
|
||||||
def test_remove_empty_old_text(self, store):
|
def test_remove_empty_old_text(self, store):
|
||||||
result = store.remove("memory", " ")
|
result = store.remove("memory", " ")
|
||||||
|
|||||||
107
tests/tools/test_syntax_preflight.py
Normal file
107
tests/tools/test_syntax_preflight.py
Normal file
@@ -0,0 +1,107 @@
|
|||||||
|
"""Tests for syntax preflight check in execute_code (issue #312)."""
|
||||||
|
|
||||||
|
import ast
|
||||||
|
import json
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
|
||||||
|
class TestSyntaxPreflight:
|
||||||
|
"""Verify that execute_code catches syntax errors before sandbox execution."""
|
||||||
|
|
||||||
|
def test_valid_syntax_passes_parse(self):
|
||||||
|
"""Valid Python should pass ast.parse."""
|
||||||
|
code = "print('hello')\nx = 1 + 2\n"
|
||||||
|
ast.parse(code) # should not raise
|
||||||
|
|
||||||
|
def test_syntax_error_indentation(self):
|
||||||
|
"""IndentationError is a subclass of SyntaxError."""
|
||||||
|
code = "def foo():\nbar()\n"
|
||||||
|
with pytest.raises(SyntaxError):
|
||||||
|
ast.parse(code)
|
||||||
|
|
||||||
|
def test_syntax_error_missing_colon(self):
|
||||||
|
code = "if True\n pass\n"
|
||||||
|
with pytest.raises(SyntaxError):
|
||||||
|
ast.parse(code)
|
||||||
|
|
||||||
|
def test_syntax_error_unmatched_paren(self):
|
||||||
|
code = "x = (1 + 2\n"
|
||||||
|
with pytest.raises(SyntaxError):
|
||||||
|
ast.parse(code)
|
||||||
|
|
||||||
|
def test_syntax_error_invalid_token(self):
|
||||||
|
code = "x = 1 +*\n"
|
||||||
|
with pytest.raises(SyntaxError):
|
||||||
|
ast.parse(code)
|
||||||
|
|
||||||
|
def test_syntax_error_details(self):
|
||||||
|
"""SyntaxError should provide line, offset, msg."""
|
||||||
|
code = "if True\n pass\n"
|
||||||
|
with pytest.raises(SyntaxError) as exc_info:
|
||||||
|
ast.parse(code)
|
||||||
|
e = exc_info.value
|
||||||
|
assert e.lineno is not None
|
||||||
|
assert e.msg is not None
|
||||||
|
|
||||||
|
def test_empty_string_passes(self):
|
||||||
|
"""Empty string is valid Python (empty module)."""
|
||||||
|
ast.parse("")
|
||||||
|
|
||||||
|
def test_comments_only_passes(self):
|
||||||
|
ast.parse("# just a comment\n# another\n")
|
||||||
|
|
||||||
|
def test_complex_valid_code(self):
|
||||||
|
code = '''
|
||||||
|
import os
|
||||||
|
def foo(x):
|
||||||
|
if x > 0:
|
||||||
|
return x * 2
|
||||||
|
return 0
|
||||||
|
|
||||||
|
result = [foo(i) for i in range(10)]
|
||||||
|
print(result)
|
||||||
|
'''
|
||||||
|
ast.parse(code)
|
||||||
|
|
||||||
|
|
||||||
|
class TestSyntaxPreflightResponse:
|
||||||
|
"""Test the error response format from the preflight check."""
|
||||||
|
|
||||||
|
def _check_syntax(self, code):
|
||||||
|
"""Mimic the preflight check logic from execute_code."""
|
||||||
|
try:
|
||||||
|
ast.parse(code)
|
||||||
|
return None
|
||||||
|
except SyntaxError as e:
|
||||||
|
return json.dumps({
|
||||||
|
"error": f"Python syntax error: {e.msg}",
|
||||||
|
"line": e.lineno,
|
||||||
|
"offset": e.offset,
|
||||||
|
"text": (e.text or "").strip()[:200],
|
||||||
|
})
|
||||||
|
|
||||||
|
def test_returns_json_error(self):
|
||||||
|
result = self._check_syntax("if True\n pass\n")
|
||||||
|
assert result is not None
|
||||||
|
data = json.loads(result)
|
||||||
|
assert "error" in data
|
||||||
|
assert "syntax error" in data["error"].lower()
|
||||||
|
|
||||||
|
def test_includes_line_number(self):
|
||||||
|
result = self._check_syntax("x = 1\nif True\n pass\n")
|
||||||
|
data = json.loads(result)
|
||||||
|
assert data["line"] == 2 # error on line 2
|
||||||
|
|
||||||
|
def test_includes_offset(self):
|
||||||
|
result = self._check_syntax("x = (1 + 2\n")
|
||||||
|
data = json.loads(result)
|
||||||
|
assert data["offset"] is not None
|
||||||
|
|
||||||
|
def test_includes_snippet(self):
|
||||||
|
result = self._check_syntax("if True\n")
|
||||||
|
data = json.loads(result)
|
||||||
|
assert "if True" in data["text"]
|
||||||
|
|
||||||
|
def test_none_for_valid_code(self):
|
||||||
|
result = self._check_syntax("print('ok')")
|
||||||
|
assert result is None
|
||||||
@@ -28,6 +28,7 @@ Platform: Linux / macOS only (Unix domain sockets for local). Disabled on Window
|
|||||||
Remote execution additionally requires Python 3 in the terminal backend.
|
Remote execution additionally requires Python 3 in the terminal backend.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
import ast
|
||||||
import base64
|
import base64
|
||||||
import json
|
import json
|
||||||
import logging
|
import logging
|
||||||
@@ -893,6 +894,20 @@ def execute_code(
|
|||||||
if not code or not code.strip():
|
if not code or not code.strip():
|
||||||
return json.dumps({"error": "No code provided."})
|
return json.dumps({"error": "No code provided."})
|
||||||
|
|
||||||
|
# Poka-yoke (#312): Syntax check before execution.
|
||||||
|
# 83.2% of execute_code errors are Python exceptions; most are syntax
|
||||||
|
# errors the LLM generated. ast.parse() is sub-millisecond and catches
|
||||||
|
# them before we spin up a sandbox child process.
|
||||||
|
try:
|
||||||
|
ast.parse(code)
|
||||||
|
except SyntaxError as e:
|
||||||
|
return json.dumps({
|
||||||
|
"error": f"Python syntax error: {e.msg}",
|
||||||
|
"line": e.lineno,
|
||||||
|
"offset": e.offset,
|
||||||
|
"text": (e.text or "").strip()[:200],
|
||||||
|
})
|
||||||
|
|
||||||
# Dispatch: remote backends use file-based RPC, local uses UDS
|
# Dispatch: remote backends use file-based RPC, local uses UDS
|
||||||
from tools.terminal_tool import _get_env_config
|
from tools.terminal_tool import _get_env_config
|
||||||
env_type = _get_env_config()["env_type"]
|
env_type = _get_env_config()["env_type"]
|
||||||
|
|||||||
@@ -260,8 +260,12 @@ class MemoryStore:
|
|||||||
entries = self._entries_for(target)
|
entries = self._entries_for(target)
|
||||||
matches = [(i, e) for i, e in enumerate(entries) if old_text in e]
|
matches = [(i, e) for i, e in enumerate(entries) if old_text in e]
|
||||||
|
|
||||||
if len(matches) == 0:
|
if not matches:
|
||||||
return {"success": False, "error": f"No entry matched '{old_text}'."}
|
return {
|
||||||
|
"success": True,
|
||||||
|
"result": "no_match",
|
||||||
|
"message": f"No entry matched '{old_text}'. The search substring was not found in any existing entry.",
|
||||||
|
}
|
||||||
|
|
||||||
if len(matches) > 1:
|
if len(matches) > 1:
|
||||||
# If all matches are identical (exact duplicates), operate on the first one
|
# If all matches are identical (exact duplicates), operate on the first one
|
||||||
@@ -310,8 +314,12 @@ class MemoryStore:
|
|||||||
entries = self._entries_for(target)
|
entries = self._entries_for(target)
|
||||||
matches = [(i, e) for i, e in enumerate(entries) if old_text in e]
|
matches = [(i, e) for i, e in enumerate(entries) if old_text in e]
|
||||||
|
|
||||||
if len(matches) == 0:
|
if not matches:
|
||||||
return {"success": False, "error": f"No entry matched '{old_text}'."}
|
return {
|
||||||
|
"success": True,
|
||||||
|
"result": "no_match",
|
||||||
|
"message": f"No entry matched '{old_text}'. The search substring was not found in any existing entry.",
|
||||||
|
}
|
||||||
|
|
||||||
if len(matches) > 1:
|
if len(matches) > 1:
|
||||||
# If all matches are identical (exact duplicates), remove the first one
|
# If all matches are identical (exact duplicates), remove the first one
|
||||||
@@ -449,30 +457,30 @@ def memory_tool(
|
|||||||
Returns JSON string with results.
|
Returns JSON string with results.
|
||||||
"""
|
"""
|
||||||
if store is None:
|
if store is None:
|
||||||
return json.dumps({"success": False, "error": "Memory is not available. It may be disabled in config or this environment."}, ensure_ascii=False)
|
return tool_error("Memory is not available. It may be disabled in config or this environment.", success=False)
|
||||||
|
|
||||||
if target not in ("memory", "user"):
|
if target not in ("memory", "user"):
|
||||||
return json.dumps({"success": False, "error": f"Invalid target '{target}'. Use 'memory' or 'user'."}, ensure_ascii=False)
|
return tool_error(f"Invalid target '{target}'. Use 'memory' or 'user'.", success=False)
|
||||||
|
|
||||||
if action == "add":
|
if action == "add":
|
||||||
if not content:
|
if not content:
|
||||||
return json.dumps({"success": False, "error": "Content is required for 'add' action."}, ensure_ascii=False)
|
return tool_error("Content is required for 'add' action.", success=False)
|
||||||
result = store.add(target, content)
|
result = store.add(target, content)
|
||||||
|
|
||||||
elif action == "replace":
|
elif action == "replace":
|
||||||
if not old_text:
|
if not old_text:
|
||||||
return json.dumps({"success": False, "error": "old_text is required for 'replace' action."}, ensure_ascii=False)
|
return tool_error("old_text is required for 'replace' action.", success=False)
|
||||||
if not content:
|
if not content:
|
||||||
return json.dumps({"success": False, "error": "content is required for 'replace' action."}, ensure_ascii=False)
|
return tool_error("content is required for 'replace' action.", success=False)
|
||||||
result = store.replace(target, old_text, content)
|
result = store.replace(target, old_text, content)
|
||||||
|
|
||||||
elif action == "remove":
|
elif action == "remove":
|
||||||
if not old_text:
|
if not old_text:
|
||||||
return json.dumps({"success": False, "error": "old_text is required for 'remove' action."}, ensure_ascii=False)
|
return tool_error("old_text is required for 'remove' action.", success=False)
|
||||||
result = store.remove(target, old_text)
|
result = store.remove(target, old_text)
|
||||||
|
|
||||||
else:
|
else:
|
||||||
return json.dumps({"success": False, "error": f"Unknown action '{action}'. Use: add, replace, remove"}, ensure_ascii=False)
|
return tool_error(f"Unknown action '{action}'. Use: add, replace, remove", success=False)
|
||||||
|
|
||||||
return json.dumps(result, ensure_ascii=False)
|
return json.dumps(result, ensure_ascii=False)
|
||||||
|
|
||||||
@@ -539,7 +547,7 @@ MEMORY_SCHEMA = {
|
|||||||
|
|
||||||
|
|
||||||
# --- Registry ---
|
# --- Registry ---
|
||||||
from tools.registry import registry
|
from tools.registry import registry, tool_error
|
||||||
|
|
||||||
registry.register(
|
registry.register(
|
||||||
name="memory",
|
name="memory",
|
||||||
|
|||||||
Reference in New Issue
Block a user