fix(grep): keep missing roots as tool errors
This commit is contained in:
@@ -820,7 +820,15 @@ async def run_query(
|
||||
# Single tool: sequential (stream events immediately)
|
||||
tc = tool_calls[0]
|
||||
yield ToolExecutionStarted(tool_name=tc.name, tool_input=tc.input), None
|
||||
result = await _execute_tool_call(context, tc.name, tc.id, tc.input)
|
||||
try:
|
||||
result = await _execute_tool_call(context, tc.name, tc.id, tc.input)
|
||||
except Exception as exc:
|
||||
log.exception("tool execution raised: name=%s id=%s", tc.name, tc.id)
|
||||
result = ToolResultBlock(
|
||||
tool_use_id=tc.id,
|
||||
content=f"Tool {tc.name} failed: {type(exc).__name__}: {exc}",
|
||||
is_error=True,
|
||||
)
|
||||
yield ToolExecutionCompleted(
|
||||
tool_name=tc.name,
|
||||
output=result.content,
|
||||
|
||||
@@ -16,7 +16,10 @@ class GrepToolInput(BaseModel):
|
||||
"""Arguments for the grep tool."""
|
||||
|
||||
pattern: str = Field(description="Regular expression to search for")
|
||||
root: str | None = Field(default=None, description="Search root directory")
|
||||
root: str | None = Field(
|
||||
default=None,
|
||||
description="Search root directory or file. For multiple roots, call grep separately per root.",
|
||||
)
|
||||
file_glob: str = Field(default="**/*")
|
||||
case_sensitive: bool = Field(default=True)
|
||||
limit: int = Field(default=200, ge=1, le=2000)
|
||||
@@ -36,6 +39,14 @@ class GrepTool(BaseTool):
|
||||
|
||||
async def execute(self, arguments: GrepToolInput, context: ToolExecutionContext) -> ToolResult:
|
||||
root = _resolve_path(context.cwd, arguments.root) if arguments.root else context.cwd
|
||||
if not root.exists():
|
||||
return ToolResult(
|
||||
output=(
|
||||
f"Search root does not exist: {root}\n"
|
||||
"If you intended multiple roots, call grep separately for each root."
|
||||
),
|
||||
is_error=True,
|
||||
)
|
||||
if root.is_file():
|
||||
display_base = _display_base(root, context.cwd)
|
||||
matches = await _rg_grep_file(
|
||||
|
||||
@@ -1198,6 +1198,64 @@ class _BoomTool(BaseTool):
|
||||
raise RuntimeError("boom")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_query_engine_synthesizes_tool_result_when_single_tool_raises(tmp_path: Path):
|
||||
registry = ToolRegistry()
|
||||
registry.register(_BoomTool())
|
||||
|
||||
engine = QueryEngine(
|
||||
api_client=FakeApiClient(
|
||||
[
|
||||
_FakeResponse(
|
||||
message=ConversationMessage(
|
||||
role="assistant",
|
||||
content=[
|
||||
TextBlock(text="Running one tool."),
|
||||
ToolUseBlock(id="toolu_boom", name="boom_tool", input={}),
|
||||
],
|
||||
),
|
||||
usage=UsageSnapshot(input_tokens=1, output_tokens=1),
|
||||
),
|
||||
_FakeResponse(
|
||||
message=ConversationMessage(
|
||||
role="assistant",
|
||||
content=[TextBlock(text="Recovered from the failure.")],
|
||||
),
|
||||
usage=UsageSnapshot(input_tokens=1, output_tokens=1),
|
||||
),
|
||||
]
|
||||
),
|
||||
tool_registry=registry,
|
||||
permission_checker=PermissionChecker(PermissionSettings(mode=PermissionMode.FULL_AUTO)),
|
||||
cwd=tmp_path,
|
||||
model="claude-test",
|
||||
system_prompt="system",
|
||||
)
|
||||
|
||||
events = [event async for event in engine.submit_message("run one tool")]
|
||||
|
||||
completed = [event for event in events if isinstance(event, ToolExecutionCompleted)]
|
||||
assert len(completed) == 1
|
||||
assert completed[0].tool_name == "boom_tool"
|
||||
assert completed[0].is_error is True
|
||||
assert "RuntimeError" in completed[0].output
|
||||
assert "boom" in completed[0].output
|
||||
|
||||
user_tool_messages = [
|
||||
msg
|
||||
for msg in engine.messages
|
||||
if msg.role == "user" and any(isinstance(block, ToolResultBlock) for block in msg.content)
|
||||
]
|
||||
assert len(user_tool_messages) == 1
|
||||
result_blocks = [
|
||||
block for block in user_tool_messages[0].content if isinstance(block, ToolResultBlock)
|
||||
]
|
||||
assert result_blocks[0].tool_use_id == "toolu_boom"
|
||||
|
||||
assert isinstance(events[-1], AssistantTurnComplete)
|
||||
assert events[-1].message.text == "Recovered from the failure."
|
||||
|
||||
|
||||
class _LargeOutputTool(BaseTool):
|
||||
name = "mcp__playwright__browser_snapshot"
|
||||
description = "Returns a large browser snapshot."
|
||||
|
||||
@@ -163,3 +163,31 @@ async def test_grep_tool_python_fallback_reports_invalid_regex(monkeypatch, tmp_
|
||||
assert result.is_error is False
|
||||
assert "invalid regex pattern 'hello('" in result.output
|
||||
assert "unterminated subpattern" in result.output
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_grep_tool_reports_missing_root_before_spawning_rg(monkeypatch, tmp_path: Path):
|
||||
tool = GrepTool()
|
||||
src_root = tmp_path / "src"
|
||||
tests_root = tmp_path / "tests"
|
||||
src_root.mkdir()
|
||||
tests_root.mkdir()
|
||||
monkeypatch.setattr("openharness.tools.grep_tool.shutil.which", lambda _: "/usr/bin/rg")
|
||||
|
||||
async def fail_create_subprocess_exec(*args, **kwargs):
|
||||
del args, kwargs
|
||||
raise AssertionError("rg should not be spawned for a missing root")
|
||||
|
||||
monkeypatch.setattr(
|
||||
"openharness.tools.grep_tool.asyncio.create_subprocess_exec",
|
||||
fail_create_subprocess_exec,
|
||||
)
|
||||
|
||||
result = await tool.execute(
|
||||
GrepToolInput(pattern="continue", root=f"{src_root} {tests_root}"),
|
||||
type("Ctx", (), {"cwd": tmp_path})(),
|
||||
)
|
||||
|
||||
assert result.is_error is True
|
||||
assert "Search root does not exist" in result.output
|
||||
assert "call grep separately for each root" in result.output
|
||||
|
||||
Reference in New Issue
Block a user