diff --git a/docs/tools.md b/docs/tools.md index 8738658..9110cb9 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -42,7 +42,7 @@ | `TodoTool` | `todo` | Per-session task checklist (set/update/read) | | `ScratchpadTool` | `scratchpad` | Per-session named working notes (write/append/read/clear) | | `ReloadToolsTool` | `reload_tools` | Hot-reload user tools and context providers without server restart | -| `ListToolsTool` | `list_tools` | Return the live tool list from registry | +| `ListToolsTool` | `list_tools` | Tools a profile can actually use, grouped by source (`native`, then one section per `mcp____`). Names only by default; `query` narrows by substring over names and descriptions, `verbose` adds descriptions. Declared-but-unregistered tools (server not connected) are listed separately as "Not registered" instead of being advertised | | `ToolManualTool` | `tool_manual` | Return manuals/{name}.md or auto-generate from schema | | `MemoryTool` | `memory` | Unified memory tool: save, search, and forget facts | | `SpawnAgentTool` | `spawn_agent` | Spawn an isolated subagent (blocking). Optional `profile_id` selects another profile; omitted means parent profile. `inherit_system_prompt=true` prepends the parent profile's full system prompt as a base layer for the subagent | diff --git a/navi/tools/list_tools.py b/navi/tools/list_tools.py index 29d4f16..f280eb4 100644 --- a/navi/tools/list_tools.py +++ b/navi/tools/list_tools.py @@ -1,4 +1,4 @@ -"""Built-in tool that returns the current list of available tools.""" +"""Built-in tool that returns the list of tools available to a profile.""" from __future__ import annotations @@ -6,12 +6,20 @@ from ._internal.base import Tool, ToolContext, ToolResult +NATIVE_SOURCE = "native" +_MISSING_HINT = ( + "Declared for this profile but not registered — their MCP server is not " + "connected (or the tool is gone). Do not call these; pick another tool." +) + class ListToolsTool(Tool): name = "list_tools" description = ( - "Returns the list of tools available to a specific profile. " - "Requires a profile_id — no default output." + "Returns the tools a profile can actually use, grouped by source: its built-in " + "tools as 'native' and each MCP server as 'mcp____'. Names only by " + "default — narrow it with query (substring over names and descriptions) or add " + "verbose for descriptions. For one tool's full docs use tool_manual." ) parameters = { "type": "object", @@ -19,7 +27,22 @@ "profile_id": { "type": "string", "description": "Profile ID. Returns only the tools enabled for this profile.", - } + }, + "query": { + "type": "string", + "description": ( + "Optional substring filter over tool names and descriptions, " + "e.g. 'ssh' or 'notify'. The full catalogue of an MCP-heavy profile " + "is tens of thousands of characters — filter instead of pulling it all." + ), + }, + "verbose": { + "type": "boolean", + "description": ( + "Add each tool's description. The output grows ~10x, so use it with a " + "narrow query, not to list everything." + ), + }, }, "required": ["profile_id"], } @@ -39,52 +62,124 @@ return ToolResult(success=False, output="Registry not available.", error="no_registry") profile_id = params.get("profile_id") - if profile_id and self._profile_registry is not None: - try: - profile = self._profile_registry.get(profile_id) - except Exception: - return ToolResult(success=False, output=f"Profile '{profile_id}' not found.", error="profile_not_found") + if not profile_id: + return ToolResult( + success=False, + output="Missing required argument: profile_id. Specify a profile ID to see its enabled tools.", + error="missing_profile_id", + ) - scope = profile.get_agent_tools() - names = list(scope.native) - # Same reader the agent itself uses — a second, cwd-relative copy of - # this path made list_tools disagree with the real toolset. - from navi.core.tool_utils import load_user_enabled_tools + if self._profile_registry is None: + return ToolResult( + success=False, output="Profile registry not available.", error="no_profile_registry" + ) - extra = load_user_enabled_tools() - for name in extra: - if name not in names: - names.append(name) + try: + profile = self._profile_registry.get(profile_id) + except Exception: + return ToolResult( + success=False, output=f"Profile '{profile_id}' not found.", error="profile_not_found" + ) - # Expand MCP server groups into concrete tool names - if scope.mcp and self._mcp_manager: - for server_name, groups in scope.mcp.items(): - if "*" in groups: - prefix = build_mcp_name(server_name, "") - for tool in self._registry.all(): - if tool.name.startswith(prefix) and tool.name not in names: - names.append(tool.name) - else: - for group_name in groups: - for tool_name in self._mcp_manager.resolve_group(server_name, group_name): - full_name = build_mcp_name(server_name, tool_name) - if full_name not in names: - names.append(full_name) + query = (params.get("query") or "").strip().lower() + verbose = bool(params.get("verbose")) - tools = [] + declared = self._declared_by_source(profile) + declared_total = sum(len(names) for names in declared.values()) + + # Resolve against the LIVE registry, not the config: a tool the config + # declares but nothing registered (server down, tool removed) must not be + # advertised — the agent would call it and get "tool not found". + available: dict[str, list[str]] = {} + descriptions: dict[str, str] = {} + missing: list[str] = [] + matched = 0 + for source, names in declared.items(): + kept: list[str] = [] for name in names: - try: - tools.append(self._registry.get(name)) - except Exception: - pass + tool = self._lookup(name) + if tool is None: + if not query or query in name.lower(): + missing.append(name) + continue + description = tool.description or "" + if query and query not in name.lower() and query not in description.lower(): + continue + kept.append(name) + descriptions[name] = description + matched += 1 + if kept: + available[source] = kept - lines = [f"Tools for profile '{profile_id}' ({len(tools)} total):"] - for tool in tools: - lines.append(f"• {tool.name}: {tool.description}") - return ToolResult(success=True, output="\n".join(lines)) + lines = [self._header(profile_id, matched, declared_total, query)] + for source in self._ordered_sources(available): + names = available[source] + if verbose: + lines.append(f"\n{source} ({len(names)})") + lines.extend(f" • {n}: {descriptions.get(n, '')}" for n in names) + else: + lines.append(f"{source} ({len(names)}): {', '.join(names)}") - return ToolResult( - success=False, - output="Missing required argument: profile_id. Specify a profile ID to see its enabled tools.", - error="missing_profile_id", - ) + if missing: + lines.append("") + lines.append(f"Not registered ({len(missing)}): {', '.join(missing)}") + lines.append(_MISSING_HINT) + + if not query and not verbose: + lines.append("") + lines.append("Pass query=… to filter, verbose=true for descriptions, tool_manual for one tool's docs.") + + return ToolResult(success=True, output="\n".join(lines)) + + # ── helpers ────────────────────────────────────────────────────────── + + def _declared_by_source(self, profile) -> dict[str, list[str]]: + """Names the profile enables, bucketed by source ('native', 'mcp____').""" + scope = profile.get_agent_tools() + groups: dict[str, list[str]] = {NATIVE_SOURCE: list(scope.native)} + + # Same reader the agent itself uses — a second, cwd-relative copy of + # this path made list_tools disagree with the real toolset. + from navi.core.tool_utils import load_user_enabled_tools + + for name in load_user_enabled_tools(): + if name not in groups[NATIVE_SOURCE]: + groups[NATIVE_SOURCE].append(name) + + if scope.mcp and self._mcp_manager: + for server_name, group_names in scope.mcp.items(): + source = build_mcp_name(server_name, "") + bucket = groups.setdefault(source, []) + if "*" in group_names: + for tool in self._registry.all(): + if tool.name.startswith(source) and tool.name not in bucket: + bucket.append(tool.name) + else: + for group_name in group_names: + for tool_name in self._mcp_manager.resolve_group(server_name, group_name): + full_name = build_mcp_name(server_name, tool_name) + if full_name not in bucket: + bucket.append(full_name) + + return {source: names for source, names in groups.items() if names} + + def _lookup(self, name: str): + try: + return self._registry.get(name) + except Exception: + return None + + @staticmethod + def _ordered_sources(available: dict[str, list[str]]) -> list[str]: + """Native tools first, then MCP servers alphabetically.""" + servers = sorted(source for source in available if source != NATIVE_SOURCE) + return ([NATIVE_SOURCE] if NATIVE_SOURCE in available else []) + servers + + @staticmethod + def _header(profile_id: str, matched: int, declared_total: int, query: str) -> str: + if query: + return ( + f"Tools for profile '{profile_id}' matching '{query}': " + f"{matched} of {declared_total} declared." + ) + return f"Tools for profile '{profile_id}' ({matched} available):" diff --git a/tests/unit/tools/test_list_tools.py b/tests/unit/tools/test_list_tools.py new file mode 100644 index 0000000..3515cd2 --- /dev/null +++ b/tests/unit/tools/test_list_tools.py @@ -0,0 +1,209 @@ +"""Tests for list_tools. + +Two failures this tool must not have. It must not advertise a tool the agent +cannot call — the list is built from the profile config, but the call goes to +the live registry, and an MCP server that never connected leaves a phantom +entry the agent then calls into a "tool not found". And it must not dump the +whole catalogue: an MCP-heavy profile carries ~125 tools / ~35 KB, which is +9k tokens spent to answer "do I have anything for ssh". +""" + +import pytest + +from navi.core.registry import ProfileRegistry, ToolRegistry +from navi.profiles.base import ToolConfig, ToolScopeConfig +from navi.tools.list_tools import ListToolsTool +from tests.conftest_factory import FakeTool, make_profile + + +@pytest.fixture(autouse=True) +def no_user_enabled_tools(monkeypatch): + """Every profile also picks up tools/enabled.json, a real file on the machine + running the tests — tests must see exactly what they declare.""" + monkeypatch.setattr("navi.core.tool_utils.load_user_enabled_tools", lambda: []) + + +class FakeMcpManager: + """Stand-in for McpManager — group name to tool names, as the server config holds them.""" + + def __init__(self, groups: dict[str, dict[str, list[str]]] | None = None) -> None: + self._groups = groups or {} + + def resolve_group(self, server_name: str, group_name: str) -> list[str]: + return list(self._groups.get(server_name, {}).get(group_name, [])) + + +def make_profile_with(profile_id: str, native: list[str], mcp: dict | None = None): + """A profile holding exactly the declared tools — no enabled_tools migration on top.""" + return make_profile( + profile_id, + enabled_tools=[], + tools=ToolConfig( + agent=ToolScopeConfig(native=list(native), mcp=dict(mcp or {})), + ), + ) + + +def make_profiles(*profiles) -> ProfileRegistry: + registry = ProfileRegistry() + for profile in profiles: + registry.register(profile) + return registry + + +def make_registry(*specs: tuple[str, str]) -> ToolRegistry: + """specs: (name, description) pairs — the live, registered tools.""" + registry = ToolRegistry() + for name, description in specs: + registry.register(FakeTool(name, description=description), builtin=True) + return registry + + +def make_tool(registry, profiles, groups=None) -> ListToolsTool: + return ListToolsTool( + registry=registry, + profile_registry=profiles, + mcp_manager=FakeMcpManager(groups), + ) + + +@pytest.mark.asyncio +async def test_lists_only_the_requested_profiles_tools(): + profiles = make_profiles( + make_profile_with("secretary", ["todo"]), + make_profile_with("developer", ["code_exec"]), + ) + registry = make_registry(("todo", "notes"), ("code_exec", "runs"), ("weather", "forecast")) + + result = await make_tool(registry, profiles).execute({"profile_id": "secretary"}) + + assert result.success is True + assert "todo" in result.output + assert "code_exec" not in result.output + assert "weather" not in result.output + + +@pytest.mark.asyncio +async def test_compact_output_groups_names_by_source(): + groups = {"tgclient": {"read": ["dialogs_list", "messages_history"]}} + profiles = make_profiles( + make_profile_with("secretary", ["todo"], {"tgclient": ["read"]}) + ) + registry = make_registry( + ("todo", "notes and tasks"), + ("mcp__tgclient__dialogs_list", "list dialogs"), + ("mcp__tgclient__messages_history", "read history"), + ) + + result = await make_tool(registry, profiles, groups).execute({"profile_id": "secretary"}) + + assert "native (1): todo" in result.output + assert ( + "mcp__tgclient__ (2): mcp__tgclient__dialogs_list, mcp__tgclient__messages_history" + in result.output + ) + # Descriptions are what makes the output huge — they stay out unless asked for. + assert "notes and tasks" not in result.output + + +@pytest.mark.asyncio +async def test_verbose_is_an_order_of_magnitude_larger(): + """Descriptions are what costs the context — 125 tools of them is ~35 KB.""" + names = [f"tool_{i}" for i in range(20)] + profiles = make_profiles(make_profile_with("secretary", names)) + registry = make_registry(*[(name, "d" * 200) for name in names]) + tool = make_tool(registry, profiles) + + compact = (await tool.execute({"profile_id": "secretary"})).output + verbose = (await tool.execute({"profile_id": "secretary", "verbose": True})).output + + assert "d" * 200 not in compact + assert "d" * 200 in verbose + assert len(verbose) > len(compact) * 5 + + +@pytest.mark.asyncio +async def test_query_filters_by_name_and_by_description(): + profiles = make_profiles(make_profile_with("secretary", ["todo", "sync_files"])) + registry = make_registry(("todo", "notes and tasks"), ("sync_files", "copy files")) + tool = make_tool(registry, profiles) + + by_name = (await tool.execute({"profile_id": "secretary", "query": "files"})).output + assert "mcp__" not in by_name + assert "sync_files" in by_name + assert "todo" not in by_name + assert "matching 'files'" in by_name + + by_description = (await tool.execute({"profile_id": "secretary", "query": "tasks"})).output + assert "todo" in by_description + assert "sync_files" not in by_description + + +@pytest.mark.asyncio +async def test_declared_but_unregistered_tools_are_not_advertised(): + """The navi_ui case: the profile config declares it, the server never connected.""" + groups = {"navi_ui": {"ui": ["render_component"]}} + profiles = make_profiles( + make_profile_with("secretary", ["todo"], {"navi_ui": ["ui"]}) + ) + registry = make_registry(("todo", "notes")) # render_component was never registered + + result = await make_tool(registry, profiles, groups).execute({"profile_id": "secretary"}) + + assert "Not registered (1): mcp__navi_ui__render_component" in result.output + # Counted as neither available nor part of a source section. + assert "(1 available)" in result.output + assert "mcp__navi_ui__ (1)" not in result.output + + +@pytest.mark.asyncio +async def test_query_still_reports_a_matching_unregistered_tool(): + """"Do I have anything for render" must answer "declared, but not callable".""" + groups = {"navi_ui": {"ui": ["render_component"]}} + profiles = make_profiles( + make_profile_with("secretary", ["todo"], {"navi_ui": ["ui"]}) + ) + registry = make_registry(("todo", "notes")) + + result = await make_tool(registry, profiles, groups).execute( + {"profile_id": "secretary", "query": "render"} + ) + + assert "Not registered (1): mcp__navi_ui__render_component" in result.output + assert "0 of 2 declared" in result.output + + +@pytest.mark.asyncio +async def test_wildcard_group_takes_every_registered_tool_of_that_server(): + profiles = make_profiles( + make_profile_with("secretary", ["todo"], {"tgclient": ["*"]}) + ) + registry = make_registry( + ("todo", "notes"), + ("mcp__tgclient__dialogs_list", "dialogs"), + ("mcp__other__whatever", "not this server"), + ) + + result = await make_tool(registry, profiles).execute({"profile_id": "secretary"}) + + assert "mcp__tgclient__dialogs_list" in result.output + assert "mcp__other__whatever" not in result.output + + +@pytest.mark.asyncio +async def test_requires_a_profile_id(): + result = await make_tool(make_registry(), make_profiles()).execute({}) + + assert result.success is False + assert result.error == "missing_profile_id" + + +@pytest.mark.asyncio +async def test_unknown_profile_is_rejected(): + registry = make_registry(("todo", "notes")) + profiles = make_profiles(make_profile_with("secretary", ["todo"])) + + result = await make_tool(registry, profiles).execute({"profile_id": "nope"}) + + assert result.success is False + assert result.error == "profile_not_found"