mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-05-20 08:32:25 +00:00
refactor(tools): remove GlobTool
GlobTool is redundant — GrepTool already supports glob-based file filtering via its `glob` parameter, making a standalone glob-only tool unnecessary. Removing it simplifies the tool surface and reduces LLM confusion between glob and grep.
This commit is contained in:
parent
45d999ae70
commit
fe90edd71f
@ -47,7 +47,7 @@ _SNIP_SAFETY_BUFFER = 1024
|
|||||||
_MICROCOMPACT_KEEP_RECENT = 10
|
_MICROCOMPACT_KEEP_RECENT = 10
|
||||||
_MICROCOMPACT_MIN_CHARS = 500
|
_MICROCOMPACT_MIN_CHARS = 500
|
||||||
_COMPACTABLE_TOOLS = frozenset({
|
_COMPACTABLE_TOOLS = frozenset({
|
||||||
"read_file", "exec", "grep", "glob",
|
"read_file", "exec", "grep",
|
||||||
"web_search", "web_fetch", "list_dir",
|
"web_search", "web_fetch", "list_dir",
|
||||||
})
|
})
|
||||||
_BACKFILL_CONTENT = "[Tool result unavailable — call was interrupted or lost]"
|
_BACKFILL_CONTENT = "[Tool result unavailable — call was interrupted or lost]"
|
||||||
|
|||||||
@ -1,4 +1,4 @@
|
|||||||
"""Search tools: grep and glob."""
|
"""Search tools: grep."""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
@ -108,146 +108,6 @@ class _SearchTool(_FsTool):
|
|||||||
for filename in sorted(filenames):
|
for filename in sorted(filenames):
|
||||||
yield current / filename
|
yield current / filename
|
||||||
|
|
||||||
def _iter_entries(
|
|
||||||
self,
|
|
||||||
root: Path,
|
|
||||||
*,
|
|
||||||
include_files: bool,
|
|
||||||
include_dirs: bool,
|
|
||||||
) -> Iterable[Path]:
|
|
||||||
if root.is_file():
|
|
||||||
if include_files:
|
|
||||||
yield root
|
|
||||||
return
|
|
||||||
|
|
||||||
for dirpath, dirnames, filenames in os.walk(root):
|
|
||||||
dirnames[:] = sorted(d for d in dirnames if d not in self._IGNORE_DIRS)
|
|
||||||
current = Path(dirpath)
|
|
||||||
if include_dirs:
|
|
||||||
for dirname in dirnames:
|
|
||||||
yield current / dirname
|
|
||||||
if include_files:
|
|
||||||
for filename in sorted(filenames):
|
|
||||||
yield current / filename
|
|
||||||
|
|
||||||
|
|
||||||
class GlobTool(_SearchTool):
|
|
||||||
"""Find files matching a glob pattern."""
|
|
||||||
_scopes = {"core", "subagent"}
|
|
||||||
|
|
||||||
@property
|
|
||||||
def name(self) -> str:
|
|
||||||
return "glob"
|
|
||||||
|
|
||||||
@property
|
|
||||||
def description(self) -> str:
|
|
||||||
return (
|
|
||||||
"Find files matching a glob pattern (e.g. '*.py', 'tests/**/test_*.py'). "
|
|
||||||
"Results are sorted by modification time (newest first). "
|
|
||||||
"Skips .git, node_modules, __pycache__, and other noise directories."
|
|
||||||
)
|
|
||||||
|
|
||||||
@property
|
|
||||||
def read_only(self) -> bool:
|
|
||||||
return True
|
|
||||||
|
|
||||||
@property
|
|
||||||
def parameters(self) -> dict[str, Any]:
|
|
||||||
return {
|
|
||||||
"type": "object",
|
|
||||||
"properties": {
|
|
||||||
"pattern": {
|
|
||||||
"type": "string",
|
|
||||||
"description": "Glob pattern to match, e.g. '*.py' or 'tests/**/test_*.py'",
|
|
||||||
"minLength": 1,
|
|
||||||
},
|
|
||||||
"path": {
|
|
||||||
"type": "string",
|
|
||||||
"description": "Directory to search from (default '.')",
|
|
||||||
},
|
|
||||||
"max_results": {
|
|
||||||
"type": "integer",
|
|
||||||
"description": "Legacy alias for head_limit",
|
|
||||||
"minimum": 1,
|
|
||||||
"maximum": 1000,
|
|
||||||
},
|
|
||||||
"head_limit": {
|
|
||||||
"type": "integer",
|
|
||||||
"description": "Maximum number of matches to return (default 250)",
|
|
||||||
"minimum": 0,
|
|
||||||
"maximum": 1000,
|
|
||||||
},
|
|
||||||
"offset": {
|
|
||||||
"type": "integer",
|
|
||||||
"description": "Skip the first N matching entries before returning results",
|
|
||||||
"minimum": 0,
|
|
||||||
"maximum": 100000,
|
|
||||||
},
|
|
||||||
"entry_type": {
|
|
||||||
"type": "string",
|
|
||||||
"enum": ["files", "dirs", "both"],
|
|
||||||
"description": "Whether to match files, directories, or both (default files)",
|
|
||||||
},
|
|
||||||
},
|
|
||||||
"required": ["pattern"],
|
|
||||||
}
|
|
||||||
|
|
||||||
async def execute(
|
|
||||||
self,
|
|
||||||
pattern: str,
|
|
||||||
path: str = ".",
|
|
||||||
max_results: int | None = None,
|
|
||||||
head_limit: int | None = None,
|
|
||||||
offset: int = 0,
|
|
||||||
entry_type: str = "files",
|
|
||||||
**kwargs: Any,
|
|
||||||
) -> str:
|
|
||||||
try:
|
|
||||||
root = self._resolve(path or ".")
|
|
||||||
if not root.exists():
|
|
||||||
return f"Error: Path not found: {path}"
|
|
||||||
if not root.is_dir():
|
|
||||||
return f"Error: Not a directory: {path}"
|
|
||||||
|
|
||||||
if head_limit is not None:
|
|
||||||
limit = None if head_limit == 0 else head_limit
|
|
||||||
elif max_results is not None:
|
|
||||||
limit = max_results
|
|
||||||
else:
|
|
||||||
limit = _DEFAULT_HEAD_LIMIT
|
|
||||||
include_files = entry_type in {"files", "both"}
|
|
||||||
include_dirs = entry_type in {"dirs", "both"}
|
|
||||||
matches: list[tuple[str, float]] = []
|
|
||||||
for entry in self._iter_entries(
|
|
||||||
root,
|
|
||||||
include_files=include_files,
|
|
||||||
include_dirs=include_dirs,
|
|
||||||
):
|
|
||||||
rel_path = entry.relative_to(root).as_posix()
|
|
||||||
if _match_glob(rel_path, entry.name, pattern):
|
|
||||||
display = self._display_path(entry, root)
|
|
||||||
if entry.is_dir():
|
|
||||||
display += "/"
|
|
||||||
try:
|
|
||||||
mtime = entry.stat().st_mtime
|
|
||||||
except OSError:
|
|
||||||
mtime = 0.0
|
|
||||||
matches.append((display, mtime))
|
|
||||||
|
|
||||||
if not matches:
|
|
||||||
return f"No paths matched pattern '{pattern}' in {path}"
|
|
||||||
|
|
||||||
matches.sort(key=lambda item: (-item[1], item[0]))
|
|
||||||
ordered = [name for name, _ in matches]
|
|
||||||
paged, truncated = _paginate(ordered, limit, offset)
|
|
||||||
result = "\n".join(paged)
|
|
||||||
if note := _pagination_note(limit, offset, truncated):
|
|
||||||
result += f"\n\n{note}"
|
|
||||||
return result
|
|
||||||
except PermissionError as e:
|
|
||||||
return f"Error: {e}"
|
|
||||||
except Exception as e:
|
|
||||||
return f"Error finding files: {e}"
|
|
||||||
|
|
||||||
|
|
||||||
class GrepTool(_SearchTool):
|
class GrepTool(_SearchTool):
|
||||||
|
|||||||
@ -11,7 +11,6 @@ _TOOL_FORMATS: dict[str, tuple[list[str], str, bool, bool]] = {
|
|||||||
"read_file": (["path", "file_path"], "read {}", True, False),
|
"read_file": (["path", "file_path"], "read {}", True, False),
|
||||||
"write_file": (["path", "file_path"], "write {}", True, False),
|
"write_file": (["path", "file_path"], "write {}", True, False),
|
||||||
"edit": (["file_path", "path"], "edit {}", True, False),
|
"edit": (["file_path", "path"], "edit {}", True, False),
|
||||||
"glob": (["pattern"], 'glob "{}"', False, False),
|
|
||||||
"grep": (["pattern"], 'grep "{}"', False, False),
|
"grep": (["pattern"], 'grep "{}"', False, False),
|
||||||
"exec": (["command"], "$ {}", False, True),
|
"exec": (["command"], "$ {}", False, True),
|
||||||
"web_search": (["query"], 'search "{}"', False, False),
|
"web_search": (["query"], 'search "{}"', False, False),
|
||||||
|
|||||||
@ -25,7 +25,6 @@ async def test_subagent_uses_tool_loader():
|
|||||||
tools = sm._build_tools()
|
tools = sm._build_tools()
|
||||||
assert tools.has("read_file")
|
assert tools.has("read_file")
|
||||||
assert tools.has("write_file")
|
assert tools.has("write_file")
|
||||||
assert tools.has("glob")
|
|
||||||
assert not tools.has("message")
|
assert not tools.has("message")
|
||||||
assert not tools.has("spawn")
|
assert not tools.has("spawn")
|
||||||
|
|
||||||
|
|||||||
@ -34,10 +34,6 @@ class TestToolHintKnownTools:
|
|||||||
assert "main.py" in result
|
assert "main.py" in result
|
||||||
assert "edit " in result
|
assert "edit " in result
|
||||||
|
|
||||||
def test_glob_shows_pattern(self):
|
|
||||||
result = _hint([_tc("glob", {"pattern": "**/*.py", "path": "src"})])
|
|
||||||
assert result == 'glob "**/*.py"'
|
|
||||||
|
|
||||||
def test_grep_shows_pattern(self):
|
def test_grep_shows_pattern(self):
|
||||||
result = _hint([_tc("grep", {"pattern": "TODO|FIXME", "path": "src"})])
|
result = _hint([_tc("grep", {"pattern": "TODO|FIXME", "path": "src"})])
|
||||||
assert result == 'grep "TODO|FIXME"'
|
assert result == 'grep "TODO|FIXME"'
|
||||||
|
|||||||
@ -190,7 +190,7 @@ async def test_run_populates_tools_used_across_iterations(tmp_path):
|
|||||||
ctx1 = AgentHookContext(iteration=0, messages=messages)
|
ctx1 = AgentHookContext(iteration=0, messages=messages)
|
||||||
ctx1.tool_calls = [
|
ctx1.tool_calls = [
|
||||||
ToolCallRequest(id="c1", name="read_file", arguments={}),
|
ToolCallRequest(id="c1", name="read_file", arguments={}),
|
||||||
ToolCallRequest(id="c2", name="glob", arguments={}),
|
ToolCallRequest(id="c2", name="grep", arguments={}),
|
||||||
]
|
]
|
||||||
for h in extras:
|
for h in extras:
|
||||||
await h.after_iteration(ctx1)
|
await h.after_iteration(ctx1)
|
||||||
@ -204,7 +204,7 @@ async def test_run_populates_tools_used_across_iterations(tmp_path):
|
|||||||
bot._loop.process_direct = fake_process_direct
|
bot._loop.process_direct = fake_process_direct
|
||||||
result = await bot.run("do stuff")
|
result = await bot.run("do stuff")
|
||||||
assert result.content == "final"
|
assert result.content == "final"
|
||||||
assert result.tools_used == ["read_file", "glob", "web_fetch"]
|
assert result.tools_used == ["read_file", "grep", "web_fetch"]
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
|
|||||||
@ -1,4 +1,4 @@
|
|||||||
"""Tests for grep/glob search tools."""
|
"""Tests for grep search tools."""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
@ -12,7 +12,7 @@ import pytest
|
|||||||
|
|
||||||
from nanobot.agent.loop import AgentLoop
|
from nanobot.agent.loop import AgentLoop
|
||||||
from nanobot.agent.subagent import SubagentManager, SubagentStatus
|
from nanobot.agent.subagent import SubagentManager, SubagentStatus
|
||||||
from nanobot.agent.tools.search import GlobTool, GrepTool
|
from nanobot.agent.tools.search import GrepTool
|
||||||
from nanobot.agent.tools.web import WebSearchTool
|
from nanobot.agent.tools.web import WebSearchTool
|
||||||
from nanobot.bus.queue import MessageBus
|
from nanobot.bus.queue import MessageBus
|
||||||
from nanobot.config.schema import WebSearchConfig
|
from nanobot.config.schema import WebSearchConfig
|
||||||
@ -33,39 +33,6 @@ async def test_web_search_tool_refreshes_dynamic_config_loader(monkeypatch) -> N
|
|||||||
assert await tool.execute("nanobot") == "duckduckgo:nanobot:3"
|
assert await tool.execute("nanobot") == "duckduckgo:nanobot:3"
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
|
||||||
async def test_glob_matches_recursively_and_skips_noise_dirs(tmp_path: Path) -> None:
|
|
||||||
(tmp_path / "src").mkdir()
|
|
||||||
(tmp_path / "nested").mkdir()
|
|
||||||
(tmp_path / "node_modules").mkdir()
|
|
||||||
(tmp_path / "src" / "app.py").write_text("print('ok')\n", encoding="utf-8")
|
|
||||||
(tmp_path / "nested" / "util.py").write_text("print('ok')\n", encoding="utf-8")
|
|
||||||
(tmp_path / "node_modules" / "skip.py").write_text("print('skip')\n", encoding="utf-8")
|
|
||||||
|
|
||||||
tool = GlobTool(workspace=tmp_path, allowed_dir=tmp_path)
|
|
||||||
result = await tool.execute(pattern="*.py", path=".")
|
|
||||||
|
|
||||||
assert "src/app.py" in result
|
|
||||||
assert "nested/util.py" in result
|
|
||||||
assert "node_modules/skip.py" not in result
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
|
||||||
async def test_glob_can_return_directories_only(tmp_path: Path) -> None:
|
|
||||||
(tmp_path / "src").mkdir()
|
|
||||||
(tmp_path / "src" / "api").mkdir(parents=True)
|
|
||||||
(tmp_path / "src" / "api" / "handlers.py").write_text("ok\n", encoding="utf-8")
|
|
||||||
|
|
||||||
tool = GlobTool(workspace=tmp_path, allowed_dir=tmp_path)
|
|
||||||
result = await tool.execute(
|
|
||||||
pattern="api",
|
|
||||||
path="src",
|
|
||||||
entry_type="dirs",
|
|
||||||
)
|
|
||||||
|
|
||||||
assert result.splitlines() == ["src/api/"]
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_grep_respects_glob_filter_and_context(tmp_path: Path) -> None:
|
async def test_grep_respects_glob_filter_and_context(tmp_path: Path) -> None:
|
||||||
(tmp_path / "src").mkdir()
|
(tmp_path / "src").mkdir()
|
||||||
@ -246,33 +213,6 @@ async def test_grep_files_with_matches_mode_respects_max_results(tmp_path: Path)
|
|||||||
assert "pagination: limit=2, offset=0" in result
|
assert "pagination: limit=2, offset=0" in result
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
|
||||||
async def test_glob_supports_head_limit_offset_and_recent_first(tmp_path: Path) -> None:
|
|
||||||
(tmp_path / "src").mkdir()
|
|
||||||
a = tmp_path / "src" / "a.py"
|
|
||||||
b = tmp_path / "src" / "b.py"
|
|
||||||
c = tmp_path / "src" / "c.py"
|
|
||||||
a.write_text("a\n", encoding="utf-8")
|
|
||||||
b.write_text("b\n", encoding="utf-8")
|
|
||||||
c.write_text("c\n", encoding="utf-8")
|
|
||||||
|
|
||||||
os.utime(a, (1, 1))
|
|
||||||
os.utime(b, (2, 2))
|
|
||||||
os.utime(c, (3, 3))
|
|
||||||
|
|
||||||
tool = GlobTool(workspace=tmp_path, allowed_dir=tmp_path)
|
|
||||||
result = await tool.execute(
|
|
||||||
pattern="*.py",
|
|
||||||
path="src",
|
|
||||||
head_limit=1,
|
|
||||||
offset=1,
|
|
||||||
)
|
|
||||||
|
|
||||||
lines = result.splitlines()
|
|
||||||
assert lines[0] == "src/b.py"
|
|
||||||
assert "pagination: limit=1, offset=1" in result
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_grep_reports_skipped_binary_and_large_files(
|
async def test_grep_reports_skipped_binary_and_large_files(
|
||||||
tmp_path: Path,
|
tmp_path: Path,
|
||||||
@ -296,16 +236,13 @@ async def test_search_tools_reject_paths_outside_workspace(tmp_path: Path) -> No
|
|||||||
outside.write_text("secret\n", encoding="utf-8")
|
outside.write_text("secret\n", encoding="utf-8")
|
||||||
|
|
||||||
grep_tool = GrepTool(workspace=tmp_path, allowed_dir=tmp_path)
|
grep_tool = GrepTool(workspace=tmp_path, allowed_dir=tmp_path)
|
||||||
glob_tool = GlobTool(workspace=tmp_path, allowed_dir=tmp_path)
|
|
||||||
|
|
||||||
grep_result = await grep_tool.execute(pattern="secret", path=str(outside))
|
grep_result = await grep_tool.execute(pattern="secret", path=str(outside))
|
||||||
glob_result = await glob_tool.execute(pattern="*.txt", path=str(outside.parent))
|
|
||||||
|
|
||||||
assert grep_result.startswith("Error:")
|
assert grep_result.startswith("Error:")
|
||||||
assert glob_result.startswith("Error:")
|
|
||||||
|
|
||||||
|
|
||||||
def test_agent_loop_registers_grep_and_glob(tmp_path: Path) -> None:
|
def test_agent_loop_registers_grep(tmp_path: Path) -> None:
|
||||||
bus = MessageBus()
|
bus = MessageBus()
|
||||||
provider = MagicMock()
|
provider = MagicMock()
|
||||||
provider.get_default_model.return_value = "test-model"
|
provider.get_default_model.return_value = "test-model"
|
||||||
@ -313,11 +250,10 @@ def test_agent_loop_registers_grep_and_glob(tmp_path: Path) -> None:
|
|||||||
loop = AgentLoop(bus=bus, provider=provider, workspace=tmp_path, model="test-model")
|
loop = AgentLoop(bus=bus, provider=provider, workspace=tmp_path, model="test-model")
|
||||||
|
|
||||||
assert "grep" in loop.tools.tool_names
|
assert "grep" in loop.tools.tool_names
|
||||||
assert "glob" in loop.tools.tool_names
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_subagent_registers_grep_and_glob(tmp_path: Path) -> None:
|
async def test_subagent_registers_grep(tmp_path: Path) -> None:
|
||||||
bus = MessageBus()
|
bus = MessageBus()
|
||||||
provider = MagicMock()
|
provider = MagicMock()
|
||||||
provider.get_default_model.return_value = "test-model"
|
provider.get_default_model.return_value = "test-model"
|
||||||
@ -345,7 +281,6 @@ async def test_subagent_registers_grep_and_glob(tmp_path: Path) -> None:
|
|||||||
await mgr._run_subagent("sub-1", "search task", "label", {"channel": "cli", "chat_id": "direct"}, status)
|
await mgr._run_subagent("sub-1", "search task", "label", {"channel": "cli", "chat_id": "direct"}, status)
|
||||||
|
|
||||||
assert "grep" in captured["tool_names"]
|
assert "grep" in captured["tool_names"]
|
||||||
assert "glob" in captured["tool_names"]
|
|
||||||
|
|
||||||
|
|
||||||
def test_subagent_prompt_respects_disabled_skills(tmp_path: Path) -> None:
|
def test_subagent_prompt_respects_disabled_skills(tmp_path: Path) -> None:
|
||||||
|
|||||||
@ -406,7 +406,7 @@ def test_loader_registers_same_tools_as_old_hardcoded():
|
|||||||
|
|
||||||
expected = {
|
expected = {
|
||||||
"read_file", "write_file", "edit_file", "list_dir",
|
"read_file", "write_file", "edit_file", "list_dir",
|
||||||
"glob", "grep", "notebook_edit", "exec", "web_search", "web_fetch",
|
"grep", "notebook_edit", "exec", "web_search", "web_fetch",
|
||||||
"message", "spawn", "cron",
|
"message", "spawn", "cron",
|
||||||
}
|
}
|
||||||
actual = set(registered)
|
actual = set(registered)
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user