Add Phase 6: write tools, shell, and edit tools with reasoning-only fix
Implement 6 new agent tools — write_file, make_dir, delete_file, str_replace, patch_apply, run_command — bringing the agent from read-only observer to active code modifier. All write/shell operations are gated through the existing permissions service. Also fix a bug where qwen3.5 thinking mode produces reasoning tokens but no content after tool results, causing the agent to silently exit. The loop now detects reasoning-only responses, retries twice, then injects a nudge message to break the model out of its thinking loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -60,6 +60,7 @@ def client() -> MagicMock:
|
||||
def handler() -> MagicMock:
|
||||
mock = MagicMock(spec=StreamHandler)
|
||||
mock.usage = None
|
||||
mock.had_reasoning_only = False
|
||||
mock.reset = MagicMock()
|
||||
return mock
|
||||
|
||||
|
||||
145
tests/unit/test_edit.py
Normal file
145
tests/unit/test_edit.py
Normal file
@@ -0,0 +1,145 @@
|
||||
"""Tests for edit tools: str_replace and patch_apply (Phase 6)."""
|
||||
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch as mock_patch
|
||||
|
||||
import pytest
|
||||
|
||||
from app.models.config import AppConfig, load_config
|
||||
from app.models.tool_call import ToolResultStatus
|
||||
from app.tools.edit import PatchApplyTool, StrReplaceTool
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def config() -> AppConfig:
|
||||
return load_config()
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tmp_workspace(tmp_path: Path, config: AppConfig) -> tuple[Path, AppConfig]:
|
||||
"""Create a temporary workspace for edit tests."""
|
||||
config.agent.workspace_root = tmp_path
|
||||
return tmp_path, config
|
||||
|
||||
|
||||
# --- StrReplaceTool ---
|
||||
|
||||
|
||||
class TestStrReplaceTool:
|
||||
def test_replace_unique_match(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "test.py").write_text("def hello():\n return 'hello'\n")
|
||||
tool = StrReplaceTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-1",
|
||||
{"file_path": "test.py", "old_str": "return 'hello'", "new_str": "return 'world'"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert "1 occurrence" in result.output
|
||||
assert (ws / "test.py").read_text() == "def hello():\n return 'world'\n"
|
||||
|
||||
def test_replace_no_match(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "test.py").write_text("some content")
|
||||
tool = StrReplaceTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-2",
|
||||
{"file_path": "test.py", "old_str": "nonexistent", "new_str": "replacement"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "not found" in (result.error or "").lower()
|
||||
|
||||
def test_replace_multiple_matches_fails(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "test.py").write_text("foo bar foo baz foo")
|
||||
tool = StrReplaceTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-3",
|
||||
{"file_path": "test.py", "old_str": "foo", "new_str": "qux"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "3 times" in (result.error or "")
|
||||
|
||||
def test_replace_nonexistent_file(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = StrReplaceTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-4",
|
||||
{"file_path": "missing.py", "old_str": "a", "new_str": "b"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "not found" in (result.error or "").lower()
|
||||
|
||||
def test_replace_path_traversal_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = StrReplaceTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-5",
|
||||
{"file_path": "../../etc/passwd", "old_str": "a", "new_str": "b"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "outside" in (result.error or "").lower()
|
||||
|
||||
|
||||
# --- PatchApplyTool ---
|
||||
|
||||
|
||||
class TestPatchApplyTool:
|
||||
def test_apply_valid_patch(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "target.txt").write_text("line1\nline2\nline3\n")
|
||||
patch_text = (
|
||||
"--- a/target.txt\n"
|
||||
"+++ b/target.txt\n"
|
||||
"@@ -1,3 +1,3 @@\n"
|
||||
" line1\n"
|
||||
"-line2\n"
|
||||
"+line2_modified\n"
|
||||
" line3\n"
|
||||
)
|
||||
tool = PatchApplyTool(ws, cfg)
|
||||
result = tool.run("tc-1", {"file_path": "target.txt", "patch": patch_text})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert "line2_modified" in (ws / "target.txt").read_text()
|
||||
|
||||
def test_apply_bad_patch(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "target.txt").write_text("line1\nline2\n")
|
||||
tool = PatchApplyTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-2",
|
||||
{"file_path": "target.txt", "patch": "this is not a valid patch"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
|
||||
def test_apply_nonexistent_file(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = PatchApplyTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-3",
|
||||
{"file_path": "missing.txt", "patch": "--- a\n+++ b\n"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "not found" in (result.error or "").lower()
|
||||
|
||||
def test_apply_path_traversal_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = PatchApplyTool(ws, cfg)
|
||||
result = tool.run(
|
||||
"tc-4",
|
||||
{"file_path": "../../etc/passwd", "patch": "--- a\n+++ b\n"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "outside" in (result.error or "").lower()
|
||||
|
||||
def test_apply_timeout(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "target.txt").write_text("content\n")
|
||||
tool = PatchApplyTool(ws, cfg)
|
||||
with mock_patch("app.tools.edit.subprocess.run", side_effect=__import__("subprocess").TimeoutExpired("patch", 30)):
|
||||
result = tool.run(
|
||||
"tc-5",
|
||||
{"file_path": "target.txt", "patch": "--- a\n+++ b\n"},
|
||||
)
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "timed out" in (result.error or "").lower()
|
||||
138
tests/unit/test_filesystem_write.py
Normal file
138
tests/unit/test_filesystem_write.py
Normal file
@@ -0,0 +1,138 @@
|
||||
"""Tests for write/mkdir/delete filesystem tools (Phase 6)."""
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from app.models.config import AppConfig, load_config
|
||||
from app.models.tool_call import ToolResultStatus
|
||||
from app.tools.filesystem import DeleteFileTool, MakeDirTool, WriteFileTool
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def config() -> AppConfig:
|
||||
return load_config()
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def workspace(config: AppConfig) -> Path:
|
||||
return config.agent.workspace_root
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tmp_workspace(tmp_path: Path, config: AppConfig) -> tuple[Path, AppConfig]:
|
||||
"""Create a temporary workspace for write tests."""
|
||||
# Override workspace_root to tmp_path for isolation
|
||||
config.agent.workspace_root = tmp_path
|
||||
return tmp_path, config
|
||||
|
||||
|
||||
# --- WriteFileTool ---
|
||||
|
||||
|
||||
class TestWriteFileTool:
|
||||
def test_write_new_file(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = WriteFileTool(ws, cfg)
|
||||
result = tool.run("tc-1", {"file_path": "hello.txt", "content": "Hello, world!"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert "13 characters" in result.output
|
||||
assert (ws / "hello.txt").read_text() == "Hello, world!"
|
||||
|
||||
def test_write_creates_parent_dirs(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = WriteFileTool(ws, cfg)
|
||||
result = tool.run("tc-2", {"file_path": "a/b/c/deep.txt", "content": "deep"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert (ws / "a" / "b" / "c" / "deep.txt").read_text() == "deep"
|
||||
|
||||
def test_write_overwrites_existing(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "existing.txt").write_text("old content")
|
||||
tool = WriteFileTool(ws, cfg)
|
||||
result = tool.run("tc-3", {"file_path": "existing.txt", "content": "new content"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert (ws / "existing.txt").read_text() == "new content"
|
||||
|
||||
def test_write_path_traversal_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = WriteFileTool(ws, cfg)
|
||||
result = tool.run("tc-4", {"file_path": "../../etc/evil.txt", "content": "bad"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "outside" in (result.error or "").lower()
|
||||
|
||||
|
||||
# --- MakeDirTool ---
|
||||
|
||||
|
||||
class TestMakeDirTool:
|
||||
def test_make_new_dir(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = MakeDirTool(ws, cfg)
|
||||
result = tool.run("tc-1", {"directory_path": "newdir"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert (ws / "newdir").is_dir()
|
||||
|
||||
def test_make_nested_dirs(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = MakeDirTool(ws, cfg)
|
||||
result = tool.run("tc-2", {"directory_path": "a/b/c"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert (ws / "a" / "b" / "c").is_dir()
|
||||
|
||||
def test_make_existing_dir_ok(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "existing_dir").mkdir()
|
||||
tool = MakeDirTool(ws, cfg)
|
||||
result = tool.run("tc-3", {"directory_path": "existing_dir"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
|
||||
def test_make_dir_over_file_fails(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "afile").write_text("content")
|
||||
tool = MakeDirTool(ws, cfg)
|
||||
result = tool.run("tc-4", {"directory_path": "afile"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "not a directory" in (result.error or "").lower()
|
||||
|
||||
def test_make_dir_path_traversal_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = MakeDirTool(ws, cfg)
|
||||
result = tool.run("tc-5", {"directory_path": "../../outside"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "outside" in (result.error or "").lower()
|
||||
|
||||
|
||||
# --- DeleteFileTool ---
|
||||
|
||||
|
||||
class TestDeleteFileTool:
|
||||
def test_delete_existing_file(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "doomed.txt").write_text("bye")
|
||||
tool = DeleteFileTool(ws, cfg)
|
||||
result = tool.run("tc-1", {"file_path": "doomed.txt"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert not (ws / "doomed.txt").exists()
|
||||
|
||||
def test_delete_nonexistent_file(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = DeleteFileTool(ws, cfg)
|
||||
result = tool.run("tc-2", {"file_path": "ghost.txt"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "not found" in (result.error or "").lower()
|
||||
|
||||
def test_delete_directory_refused(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
(ws / "adir").mkdir()
|
||||
tool = DeleteFileTool(ws, cfg)
|
||||
result = tool.run("tc-3", {"file_path": "adir"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "directory" in (result.error or "").lower()
|
||||
|
||||
def test_delete_path_traversal_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = DeleteFileTool(ws, cfg)
|
||||
result = tool.run("tc-4", {"file_path": "../../etc/passwd"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "outside" in (result.error or "").lower()
|
||||
95
tests/unit/test_shell.py
Normal file
95
tests/unit/test_shell.py
Normal file
@@ -0,0 +1,95 @@
|
||||
"""Tests for the run_command shell tool (Phase 6)."""
|
||||
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch as mock_patch
|
||||
|
||||
import pytest
|
||||
|
||||
from app.models.config import AppConfig, load_config
|
||||
from app.models.tool_call import ToolResultStatus
|
||||
from app.tools.shell import RunCommandTool
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def config() -> AppConfig:
|
||||
return load_config()
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tmp_workspace(tmp_path: Path, config: AppConfig) -> tuple[Path, AppConfig]:
|
||||
"""Create a temporary workspace for shell tests."""
|
||||
config.agent.workspace_root = tmp_path
|
||||
return tmp_path, config
|
||||
|
||||
|
||||
class TestRunCommandTool:
|
||||
def test_allowed_command_runs(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
result = tool.run("tc-1", {"command": "echo hello"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert "hello" in result.output
|
||||
|
||||
def test_denied_command_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
result = tool.run("tc-2", {"command": "sudo rm -rf /"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "denied" in (result.error or "").lower()
|
||||
|
||||
def test_disallowed_command_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
result = tool.run("tc-3", {"command": "curl http://example.com"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "denied" in (result.error or "").lower()
|
||||
|
||||
def test_unlisted_command_blocked(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
result = tool.run("tc-4", {"command": "nc -l 1234"})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "not in the allowed" in (result.error or "").lower()
|
||||
|
||||
def test_nonzero_exit_code_reported(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
# ls on a nonexistent path returns non-zero
|
||||
result = tool.run("tc-5", {"command": "ls /nonexistent_path_xyz"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert "Exit code:" in result.output
|
||||
|
||||
def test_timeout(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
with mock_patch(
|
||||
"app.tools.shell.subprocess.run",
|
||||
side_effect=subprocess.TimeoutExpired("cmd", 1),
|
||||
):
|
||||
result = tool.run("tc-6", {"command": "echo test", "timeout": 1})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "timed out" in (result.error or "").lower()
|
||||
|
||||
def test_empty_command(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
result = tool.run("tc-7", {"command": ""})
|
||||
assert result.status == ToolResultStatus.ERROR
|
||||
assert "empty" in (result.error or "").lower()
|
||||
|
||||
def test_custom_timeout(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
result = tool.run("tc-8", {"command": "echo fast", "timeout": 60})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert "fast" in result.output
|
||||
|
||||
def test_runs_in_workspace_dir(self, tmp_workspace: tuple[Path, AppConfig]) -> None:
|
||||
ws, cfg = tmp_workspace
|
||||
# Create a file in the workspace to verify cwd
|
||||
(ws / "marker.txt").write_text("found")
|
||||
tool = RunCommandTool(ws, cfg)
|
||||
result = tool.run("tc-9", {"command": "cat marker.txt"})
|
||||
assert result.status == ToolResultStatus.SUCCESS
|
||||
assert "found" in result.output
|
||||
@@ -96,12 +96,18 @@ class TestToolRegistry:
|
||||
def test_create_default_registry(self, workspace: Path, config: AppConfig) -> None:
|
||||
registry = create_default_registry(workspace, config)
|
||||
names = set(registry.get_all().keys())
|
||||
assert names == {"read_file", "list_dir", "grep_files", "find_files", "finish"}
|
||||
assert names == {
|
||||
"read_file", "list_dir", "grep_files", "find_files",
|
||||
"write_file", "make_dir", "delete_file",
|
||||
"str_replace", "patch_apply",
|
||||
"run_command",
|
||||
"finish",
|
||||
}
|
||||
|
||||
def test_schema_export(self, workspace: Path, config: AppConfig) -> None:
|
||||
registry = create_default_registry(workspace, config)
|
||||
schemas = registry.get_openai_tools_schema()
|
||||
assert len(schemas) == 5
|
||||
assert len(schemas) == 11
|
||||
assert all(s["type"] == "function" for s in schemas)
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user