diff --git a/navi/core/registry.py b/navi/core/registry.py index 511891c..201d228 100644 --- a/navi/core/registry.py +++ b/navi/core/registry.py @@ -70,6 +70,18 @@ self._external_names.discard(name) self._tools.pop(name, None) + def clear_external_tools(self) -> list[str]: + """Drop every external tool and return the names removed. + + External registrations are rebuilt from scratch on reload: an MCP + server removed from the config (or a tool it stopped exposing) has no + other way to leave the registry. + """ + removed = sorted(self._external_names) + for name in removed: + self.unregister_external(name) + return removed + def add_middleware(self, middleware) -> None: """Add a ToolMiddleware instance.""" self._middlewares.append(middleware) diff --git a/navi/core/reload.py b/navi/core/reload.py new file mode 100644 index 0000000..ddea2b9 --- /dev/null +++ b/navi/core/reload.py @@ -0,0 +1,148 @@ +"""Hot-reload of user tools, context providers and MCP servers. + +One implementation serves both surfaces — the ``reload_tools`` built-in and the +admin endpoint behind the settings button — so the two can never drift apart. +The MCP half especially: ``register_mcp_tools`` only ever *adds* registrations, +so reconnecting without dropping the old ones left ghost tools for every server +removed from the config or every tool the server stopped exposing. +""" + +from dataclasses import dataclass, field + +import structlog + +from navi.config import settings + +log = structlog.get_logger() + + +@dataclass +class ReloadReport: + """What a reload loaded, and everything that went wrong on the way.""" + + tools: list[str] = field(default_factory=list) + total_tools: int = 0 + tool_errors: dict[str, str] = field(default_factory=dict) + missing_enabled: list[str] = field(default_factory=list) + providers: list[str] = field(default_factory=list) + provider_errors: dict[str, str] = field(default_factory=dict) + mcp_tools: list[str] = field(default_factory=list) + mcp_error: str | None = None + + @property + def ok(self) -> bool: + """True when nothing failed to load. ``missing_enabled`` is a warning: + the name in enabled.json is wrong, but the reload itself worked.""" + return not self.tool_errors and not self.provider_errors and self.mcp_error is None + + def as_dict(self) -> dict: + return { + "ok": self.ok, + "tools": self.tools, + "total_tools": self.total_tools, + "tool_errors": self.tool_errors, + "missing_enabled": self.missing_enabled, + "providers": self.providers, + "provider_errors": self.provider_errors, + "mcp_tools": self.mcp_tools, + "mcp_error": self.mcp_error, + } + + def lines(self) -> list[str]: + """Human/model-readable report, one line per fact.""" + lines = [ + f"Tools ({len(self.tools)}): {', '.join(self.tools)}." + if self.tools + else "Tools: none.", + ] + if self.tool_errors: + lines.append(f"Tool errors ({len(self.tool_errors)}):") + lines.extend(f" {f}: {e}" for f, e in self.tool_errors.items()) + if self.missing_enabled: + lines.append( + f"Warning: enabled.json names {len(self.missing_enabled)} tool(s) that do not " + f"exist and are silently dropped from every profile: " + f"{', '.join(self.missing_enabled)}." + ) + lines.append( + f"Context providers ({len(self.providers)}): {', '.join(self.providers)}." + if self.providers + else "Context providers: none." + ) + if self.provider_errors: + lines.append(f"Context provider errors ({len(self.provider_errors)}):") + lines.extend(f" {f}: {e}" for f, e in self.provider_errors.items()) + if self.mcp_error is not None: + lines.append(f"MCP reload error: {self.mcp_error}") + else: + lines.append( + f"MCP tools ({len(self.mcp_tools)}): {', '.join(self.mcp_tools)}." + if self.mcp_tools + else "MCP tools: none." + ) + return lines + + +def _missing_enabled(registry) -> list[str]: + """Names in tools/enabled.json that no tool in the registry answers to.""" + from navi.core.tool_utils import load_user_enabled_tools + + missing = [] + for name in load_user_enabled_tools(): + try: + registry.get(name) + except Exception: + missing.append(name) + return missing + + +async def _reload_mcp(registry, mcp_manager) -> tuple[list[str], str | None]: + """Reconnect every MCP server and rebuild its tool registrations. + + Every external registration is dropped first — see the module docstring. + Servers that are down right now lose their tools until the health-check + reconnects them, which re-registers them through the manager's + on_server_connected callback. + """ + from navi.api.deps import register_mcp_tools + from navi.mcp.tools import is_mcp_tool + + registry.clear_external_tools() + await mcp_manager.reload_all() + await register_mcp_tools(registry, mcp_manager) + return sorted(t.name for t in registry.all() if is_mcp_tool(t.name)), None + + +async def reload_all(registry, cp_registry=None, mcp_manager=None) -> ReloadReport: + """Reload user tools, context providers and MCP servers in place.""" + report = ReloadReport() + + if registry is not None: + tool_result = registry.reload_user_tools(settings.tools_dir) + report.tools = [t.name for t in tool_result.loaded] + report.tool_errors = dict(tool_result.errors) + report.total_tools = len(registry.all()) + report.missing_enabled = _missing_enabled(registry) + + if cp_registry is not None: + cp_result = cp_registry.reload_user_providers(settings.context_providers_dir) + report.providers = list(cp_result.loaded) + report.provider_errors = dict(cp_result.errors) + + if mcp_manager is not None and registry is not None: + try: + report.mcp_tools, report.mcp_error = await _reload_mcp(registry, mcp_manager) + except Exception as exc: # noqa: BLE001 — a failed MCP pass must not lose the rest + log.warning("reload.mcp_failed", error=str(exc)) + report.mcp_error = f"{type(exc).__name__}: {exc}" + + log.info( + "reload.done", + tools=len(report.tools), + tool_errors=len(report.tool_errors), + missing_enabled=len(report.missing_enabled), + providers=len(report.providers), + mcp_tools=len(report.mcp_tools), + ok=report.ok, + ) + return report diff --git a/navi/tools/_internal/loader.py b/navi/tools/_internal/loader.py index 8d554ab..0faa546 100644 --- a/navi/tools/_internal/loader.py +++ b/navi/tools/_internal/loader.py @@ -69,7 +69,13 @@ module = importlib.util.module_from_spec(spec) try: - spec.loader.exec_module(module) # type: ignore[union-attr] + # Compile the source here rather than letting the loader use + # __pycache__: the bytecode cache is keyed on (mtime in whole seconds, + # file size), so an edit of the same length made in the same second as + # the previous load re-ran the OLD code — reload_tools then reported a + # reload while the previous version of the tool stayed live. + source = py_file.read_text(encoding="utf-8") + exec(compile(source, str(py_file), "exec"), module.__dict__) except Exception as e: return [], f"{type(e).__name__}: {e}" @@ -136,11 +142,14 @@ f"Add them as class-level variables (e.g. name = 'my_tool')." ) + # Every built-in is written as execute(self, params, ctx=None), so a + # user copying one got "wrong signature" and a tool that never loaded. sig = inspect.signature(obj.execute) - if list(sig.parameters.keys()) != ["self", "params"]: + if list(sig.parameters.keys()) not in (["self", "params"], ["self", "params", "ctx"]): return [], ( f"{obj.__name__}.execute has wrong signature: {sig}. " - f"Required: async def execute(self, params: dict) -> ToolResult" + f"Required: async def execute(self, params: dict) -> ToolResult " + f"(an optional third `ctx` parameter is allowed)" ) try: diff --git a/navi/tools/list_tools.py b/navi/tools/list_tools.py index 3fd49f6..29d4f16 100644 --- a/navi/tools/list_tools.py +++ b/navi/tools/list_tools.py @@ -2,22 +2,10 @@ from __future__ import annotations -import json -from pathlib import Path - from navi.mcp.tools import build_mcp_name from ._internal.base import Tool, ToolContext, ToolResult -_USER_ENABLED_FILE = Path("tools/enabled.json") - - -def _load_user_enabled_tools() -> list[str]: - try: - return json.loads(_USER_ENABLED_FILE.read_text()) - except Exception: - return [] - class ListToolsTool(Tool): name = "list_tools" @@ -59,7 +47,11 @@ scope = profile.get_agent_tools() names = list(scope.native) - extra = _load_user_enabled_tools() + # 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 + + extra = load_user_enabled_tools() for name in extra: if name not in names: names.append(name) diff --git a/navi/tools/reload_tools.py b/navi/tools/reload_tools.py index afb335b..91d1db7 100644 --- a/navi/tools/reload_tools.py +++ b/navi/tools/reload_tools.py @@ -1,7 +1,5 @@ """Built-in tool to hot-reload user tools and context providers without restarting.""" -from navi.config import settings - from ._internal.base import Tool, ToolContext, ToolResult @@ -29,43 +27,7 @@ if self._registry is None: return ToolResult(success=False, output="Tool registry not available.", error="no_registry") - lines = [] - has_errors = False + from navi.core.reload import reload_all - tool_result = self._registry.reload_user_tools(settings.tools_dir) - if tool_result.loaded: - lines.append(f"Tools ({len(tool_result.loaded)}): {', '.join(t.name for t in tool_result.loaded)}") - else: - lines.append("Tools: none.") - if tool_result.errors: - has_errors = True - lines.append(f"Tool errors ({len(tool_result.errors)}):") - for filename, error in tool_result.errors.items(): - lines.append(f" {filename}: {error}") - - if self._cp_registry is not None: - cp_result = self._cp_registry.reload_user_providers(settings.context_providers_dir) - if cp_result.loaded: - lines.append(f"Context providers ({len(cp_result.loaded)}): {', '.join(cp_result.loaded)}") - else: - lines.append("Context providers: none.") - if cp_result.errors: - has_errors = True - lines.append(f"Context provider errors ({len(cp_result.errors)}):") - for filename, error in cp_result.errors.items(): - lines.append(f" {filename}: {error}") - - # Reconnect MCP servers - if self._mcp_manager is not None: - try: - await self._mcp_manager.reload_all() - from navi.api.deps import register_mcp_tools - await register_mcp_tools(self._registry, self._mcp_manager) - from navi.mcp.tools import is_mcp_tool - mcp_tools = [t.name for t in self._registry.all() if is_mcp_tool(t.name)] - lines.append(f"MCP tools ({len(mcp_tools)}): {', '.join(mcp_tools) or 'none'}") - except Exception as exc: - has_errors = True - lines.append(f"MCP reload error: {exc}") - - return ToolResult(success=not has_errors, output="\n".join(lines)) + report = await reload_all(self._registry, self._cp_registry, self._mcp_manager) + return ToolResult(success=report.ok, output="\n".join(report.lines())) diff --git a/tests/unit/core/test_reload.py b/tests/unit/core/test_reload.py new file mode 100644 index 0000000..91c1875 --- /dev/null +++ b/tests/unit/core/test_reload.py @@ -0,0 +1,212 @@ +"""reload_all() — the single implementation behind reload_tools and the admin button. + +The MCP half is the delicate one: register_mcp_tools only adds registrations, so +a reload that reconnects without dropping the old ones leaves ghost tools — for +every server taken out of the config, and for every tool a server stopped +exposing. They stay visible to the model and fail only when called. +""" + +import textwrap + +import pytest + +from navi.context_providers._loader import ContextProviderRegistry +from navi.core.registry import ToolRegistry +from navi.core.reload import ReloadReport, reload_all + + +def _write(path, name, version="v1", broken=False): + if broken: + path.write_text("not python (") + return + path.write_text( + textwrap.dedent( + f''' + name = "{name}" + description = "{version}" + parameters = {{"type": "object", "properties": {{}}}} + + async def execute(params): + return "{version}" + ''' + ).lstrip() + ) + + +class FakeClient: + def __init__(self, server, tools): + self.server = server + self.connected = True + self._tools = tools + + async def list_tools(self): + return [ + type("T", (), {"name": n, "description": "d", "inputSchema": {}}) + for n in self._tools + ] + + +class FakeMcpManager: + """Reconnects to whatever servers the test put in ``servers``.""" + + def __init__(self, servers: dict[str, list[str]]): + self.servers = servers + self.reloads = 0 + self.fail = False + + @property + def clients(self): + return {s: FakeClient(s, t) for s, t in self.servers.items()} + + async def reload_all(self): + self.reloads += 1 + if self.fail: + raise RuntimeError("mcp is down") + + +@pytest.fixture +def isolated_settings(tmp_path, monkeypatch): + """Point tools_dir at a scratch directory and control enabled.json.""" + import navi.core.reload as reload_mod + + # Settings is frozen — replace the object the module sees, not the field. + monkeypatch.setattr( + reload_mod, "settings", reload_mod.settings.model_copy(update={"tools_dir": str(tmp_path)}) + ) + # enabled.json is read through tool_utils by both reload and the agent. + monkeypatch.setattr("navi.core.tool_utils.load_user_enabled_tools", list) + return tmp_path + + +class FakeBuiltin: + name = "reload_tools" + description = "d" + parameters = {} + + +async def test_reports_what_was_loaded(isolated_settings): + _write(isolated_settings / "alpha.py", "alpha") + _write(isolated_settings / "beta.py", "beta") + registry = ToolRegistry() + registry.register(FakeBuiltin(), builtin=True) + + report = await reload_all(registry, None, None) + + assert report.ok is True + assert sorted(report.tools) == ["alpha", "beta"] + assert report.total_tools == 3 + assert report.mcp_error is None + assert "Tools (2): alpha, beta." in report.lines() + + +async def test_enabled_json_naming_a_tool_that_does_not_exist_is_reported(isolated_settings, monkeypatch): + """The gmail case: the name stayed in enabled.json after its file stopped + loading, and nothing said so — the tool just vanished from every profile.""" + _write(isolated_settings / "alpha.py", "alpha") + monkeypatch.setattr("navi.core.tool_utils.load_user_enabled_tools", lambda: ["ghost"]) + registry = ToolRegistry() + + report = await reload_all(registry, None, None) + + assert report.missing_enabled == ["ghost"] + assert report.ok is True # the reload itself worked; the config is wrong + assert any("ghost" in line for line in report.lines()) + + +async def test_one_broken_file_does_not_hide_the_rest(isolated_settings): + _write(isolated_settings / "alpha.py", "alpha") + _write(isolated_settings / "broken.py", "broken", broken=True) + registry = ToolRegistry() + + report = await reload_all(registry, None, None) + + assert report.ok is False + assert report.tools == ["alpha"] + assert list(report.tool_errors) == ["broken.py"] + assert registry.get("alpha") is not None + + +async def test_context_providers_are_reloaded(isolated_settings, tmp_path, monkeypatch): + import navi.core.reload as reload_mod + + providers = tmp_path / "providers" + providers.mkdir() + (providers / "clock.py").write_text( + textwrap.dedent( + ''' + name = "clock" + description = "the time" + global_provider = False + + async def get_context(): + return "12:00" + ''' + ).lstrip() + ) + monkeypatch.setattr( + reload_mod, + "settings", + reload_mod.settings.model_copy(update={"context_providers_dir": str(providers)}), + ) + cp_registry = ContextProviderRegistry() + + report = await reload_all(ToolRegistry(), cp_registry, None) + + assert report.providers == ["clock"] + assert cp_registry.get("clock") is not None + + +async def test_mcp_tools_of_a_removed_server_do_not_survive(isolated_settings): + registry = ToolRegistry() + manager = FakeMcpManager({"alpha": ["ping", "pong"]}) + + first = await reload_all(registry, None, manager) + assert first.mcp_tools == ["mcp__alpha__ping", "mcp__alpha__pong"] + + # The server now exposes only one tool... + manager.servers = {"alpha": ["ping"]} + second = await reload_all(registry, None, manager) + assert second.mcp_tools == ["mcp__alpha__ping"] + assert "mcp__alpha__pong" not in [t.name for t in registry.all()] + + # ...and then leaves the config entirely. + manager.servers = {} + third = await reload_all(registry, None, manager) + assert third.mcp_tools == [] + assert registry.all() == [] + + +async def test_builtin_tools_survive_an_mcp_reload(isolated_settings): + registry = ToolRegistry() + registry.register(FakeBuiltin(), builtin=True) + + await reload_all(registry, None, FakeMcpManager({"alpha": ["ping"]})) + + assert [t.name for t in registry.all()] == ["reload_tools", "mcp__alpha__ping"] + + +async def test_an_mcp_failure_is_isolated_and_reported(isolated_settings): + """A dead MCP server must not erase what the tool pass already loaded.""" + _write(isolated_settings / "alpha.py", "alpha") + registry = ToolRegistry() + manager = FakeMcpManager({}) + manager.fail = True + + report = await reload_all(registry, None, manager) + + assert report.ok is False + assert report.tools == ["alpha"] + assert "mcp is down" in report.mcp_error + assert registry.get("alpha") is not None + + +def test_report_serialises_for_the_api(): + report = ReloadReport(tools=["a"], total_tools=2, tool_errors={"x.py": "boom"}) + data = report.as_dict() + assert data["ok"] is False + assert data["tools"] == ["a"] and data["total_tools"] == 2 + assert data["tool_errors"] == {"x.py": "boom"} + assert set(data) == { + "ok", "tools", "total_tools", "tool_errors", "missing_enabled", + "providers", "provider_errors", "mcp_tools", "mcp_error", + } diff --git a/tests/unit/tools/test_loader.py b/tests/unit/tools/test_loader.py new file mode 100644 index 0000000..fb82ffc --- /dev/null +++ b/tests/unit/tools/test_loader.py @@ -0,0 +1,127 @@ +"""Loading user tools from tools/ — the reload must see the file, not a cache. + +The loader used to run the file through importlib's source loader, so the +bytecode cache in __pycache__ decided the result. Its key is (mtime in whole +seconds, file size): an edit of the same length written in the same second as +the previous load re-ran the OLD code, and reload_tools reported a successful +reload while the previous version of the tool stayed live. +""" + +import textwrap +from pathlib import Path + +from navi.core.registry import ToolRegistry +from navi.tools._internal.loader import load_tools_from_dir + + +def _write(path: Path, version: str) -> None: + """Write a module-level tool whose source length does not depend on *version*.""" + path.write_text( + textwrap.dedent( + f''' + name = "probe" + description = "{version}" + parameters = {{"type": "object", "properties": {{}}}} + + async def execute(params): + return "{version}" + ''' + ).lstrip() + ) + + +def test_reload_sees_an_edit_written_in_the_same_second(tmp_path): + registry = ToolRegistry() + tool_file = tmp_path / "probe.py" + + _write(tool_file, "v1") + assert [t.name for t in registry.reload_user_tools(str(tmp_path)).loaded] == ["probe"] + assert registry.get("probe").description == "v1" + + # Same length, written the same second — the stale-bytecode window. + _write(tool_file, "v2") + registry.reload_user_tools(str(tmp_path)) + assert registry.get("probe").description == "v2" + + +def test_reload_replaces_the_module_level_execute(tmp_path): + """The new body must run, not just the new description show.""" + import asyncio + + registry = ToolRegistry() + tool_file = tmp_path / "probe.py" + + _write(tool_file, "v1") + registry.reload_user_tools(str(tmp_path)) + assert asyncio.run(registry.get("probe").execute({})).output == "v1" + + _write(tool_file, "v2") + registry.reload_user_tools(str(tmp_path)) + assert asyncio.run(registry.get("probe").execute({})).output == "v2" + + +def test_class_based_tools_still_load(tmp_path): + (tmp_path / "cls_tool.py").write_text( + textwrap.dedent( + ''' + from navi.tools._internal.base import Tool, ToolResult + + + class ClsTool(Tool): + name = "cls_tool" + description = "class-based" + parameters = {"type": "object", "properties": {}} + + async def execute(self, params): + return ToolResult(success=True, output="cls") + ''' + ).lstrip() + ) + result = load_tools_from_dir(str(tmp_path)) + assert [t.name for t in result.loaded] == ["cls_tool"] + assert result.errors == {} + + +def test_class_based_tool_may_take_ctx(tmp_path): + """The signature every built-in uses — it must not read as "wrong signature".""" + (tmp_path / "ctx_tool.py").write_text( + textwrap.dedent( + ''' + from navi.tools._internal.base import Tool, ToolResult + + + class CtxTool(Tool): + name = "ctx_tool" + description = "takes ctx" + parameters = {"type": "object", "properties": {}} + + async def execute(self, params, ctx=None): + return ToolResult(success=True, output="ctx") + ''' + ).lstrip() + ) + result = load_tools_from_dir(str(tmp_path)) + assert [t.name for t in result.loaded] == ["ctx_tool"] + assert result.errors == {} + + +def test_a_broken_file_does_not_hide_the_others(tmp_path): + (tmp_path / "broken.py").write_text("this is not python (") + _write(tmp_path / "probe.py", "v1") + + result = load_tools_from_dir(str(tmp_path)) + assert [t.name for t in result.loaded] == ["probe"] + assert list(result.errors) == ["broken.py"] + assert "SyntaxError" in result.errors["broken.py"] + + +def test_a_missing_directory_is_not_an_error(tmp_path): + result = load_tools_from_dir(str(tmp_path / "nope")) + assert result.loaded == [] + assert result.errors == {} + + +def test_underscored_files_are_ignored(tmp_path): + _write(tmp_path / "_private.py", "v1") + result = load_tools_from_dir(str(tmp_path)) + assert result.loaded == [] and result.errors == {} diff --git a/tests/unit/tools/test_reload_tools.py b/tests/unit/tools/test_reload_tools.py new file mode 100644 index 0000000..b28f62e --- /dev/null +++ b/tests/unit/tools/test_reload_tools.py @@ -0,0 +1,60 @@ +"""reload_tools is a thin wrapper: it reports what reload_all() found. + +The tool used to carry its own copy of the reload work, which is how the MCP +half drifted behind the admin reconnect route. +""" + +from navi.core.reload import ReloadReport +from navi.core.registry import ToolRegistry +from navi.tools.reload_tools import ReloadToolsTool + + +async def test_success_follows_the_report(monkeypatch): + async def fake_reload_all(registry, cp_registry=None, mcp_manager=None): + return ReloadReport(tools=["alpha"], total_tools=4) + + monkeypatch.setattr("navi.core.reload.reload_all", fake_reload_all) + + result = await ReloadToolsTool(registry=ToolRegistry()).execute({}) + + assert result.success is True + assert "Tools (1): alpha." in result.output + + +async def test_errors_are_visible_in_the_output(monkeypatch): + async def fake_reload_all(registry, cp_registry=None, mcp_manager=None): + return ReloadReport( + tools=["alpha"], + tool_errors={"broken.py": "SyntaxError: bad"}, + missing_enabled=["ghost"], + ) + + monkeypatch.setattr("navi.core.reload.reload_all", fake_reload_all) + + result = await ReloadToolsTool(registry=ToolRegistry()).execute({}) + + assert result.success is False + assert "broken.py: SyntaxError: bad" in result.output + assert "ghost" in result.output + + +async def test_forwards_the_registries_it_was_built_with(monkeypatch): + seen = {} + + async def fake_reload_all(registry, cp_registry=None, mcp_manager=None): + seen.update(registry=registry, cp=cp_registry, mcp=mcp_manager) + return ReloadReport() + + monkeypatch.setattr("navi.core.reload.reload_all", fake_reload_all) + registry, cps, mcp = ToolRegistry(), object(), object() + + await ReloadToolsTool(registry=registry, cp_registry=cps, mcp_manager=mcp).execute({}) + + assert seen == {"registry": registry, "cp": cps, "mcp": mcp} + + +async def test_no_registry_is_an_error_not_a_crash(): + result = await ReloadToolsTool().execute({}) + + assert result.success is False + assert result.error == "no_registry"