diff --git a/docs/mcp.md b/docs/mcp.md index 2b973d3..e0d1806 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -126,13 +126,25 @@ - `Agent._apply_mcp_scope` (called at the top of `run_stream`) asks `McpManager.visible_servers(role, user_id)` and puts the answer in the - `current_allowed_mcp_servers` ContextVar (`navi/tools/_internal/base.py`). The token is - reset on every exit path. + `current_allowed_mcp_servers` ContextVar (`navi/tools/_internal/base.py`). It returns two + tokens and both are reset on the stop and max-iterations exit paths; the + normal-completion path returns without resetting, which is harmless because every run + sets the variables again before anything reads them. - `build_tool_list` (`navi/core/tool_utils.py`) skips any server outside that set. - `role == "admin"`, or no MCP layer at all → the ContextVar is `None`, meaning "no restriction", and nothing changes for the owner. - Fail closed: if `visible_servers` raises, the set is empty rather than unrestricted. +**Why a server is absent is part of the prompt.** Absence alone left the model knowing the +server existed, holding none of its tools, and having nothing to tell the user. +`_apply_mcp_scope` therefore also derives the withheld servers — `byok_servers() − allowed`, +the complement of what it kept, not a second read of the key store that could disagree with +the first — and puts them in `current_keyless_mcp_servers`. `ContextBuilder._mcp_context_msg` +names each of them in the `[MCP servers]` system message, followed by the reason and a +pointer at Settings → MCP keys, so the assistant can explain the missing key itself instead +of silently lacking the tools. The variable defaults to empty, so a run that never set it — +a peer run out of a fresh API task — claims nothing. + Saving a key in the panel takes effect on the next turn — the resolver's `on_change` callback clears the manager's visibility cache along with the affected clients. diff --git a/navi/core/agent.py b/navi/core/agent.py index 0477b55..f0bd575 100644 --- a/navi/core/agent.py +++ b/navi/core/agent.py @@ -387,7 +387,7 @@ raise SessionNotFound(session_id) profile = self._profiles.get(session.profile_id) - _scope_token = await self._apply_mcp_scope(session) + _scope_token, _keyless_token = await self._apply_mcp_scope(session) tools = self._tool_list(profile.get_agent_tools()) tool_schemas = [t.schema() for t in tools] llm = self._get_backend(profile.llm_backend) @@ -406,6 +406,7 @@ current_profile_id as _profile_var, current_working_directory as _cwd_var, current_allowed_mcp_servers as _mcp_scope_var, + current_keyless_mcp_servers as _keyless_var, ) _sid_token = _sid_var.set(session_id) @@ -761,6 +762,7 @@ _cwd_var.reset(_cwd_token) _sid_var.reset(_sid_token) _mcp_scope_var.reset(_scope_token) + _keyless_var.reset(_keyless_token) yield StreamStopped() return @@ -798,6 +800,7 @@ _cwd_var.reset(_cwd_token) _sid_var.reset(_sid_token) _mcp_scope_var.reset(_scope_token) + _keyless_var.reset(_keyless_token) raise MaxIterationsReached(profile.max_iterations) # ------------------------------------------------------------------ @@ -813,27 +816,48 @@ no-auth local user, whose role is ``admin``) get ``None`` — the shared credential in the config is theirs, so nothing is filtered. - Returns the ContextVar token; callers reset it where they already reset - the session and cwd vars. + Also records *which* servers were withheld for want of a key. The reason + is not stored anywhere else and is not derivable by the caller: it is the + complement of what ``visible_servers`` kept, so it is computed from that + same value rather than read a second time (a second read of the key store + could disagree with the first). + + Returns the pair of ContextVar tokens; callers reset them where they + already reset the session and cwd vars. """ from navi.tools._internal.base import ( current_allowed_mcp_servers, + current_keyless_mcp_servers, current_user_id, current_user_role, ) if self._mcp_manager is None: - return current_allowed_mcp_servers.set(None) - try: - allowed = await self._mcp_manager.visible_servers( - current_user_role.get(), current_user_id.get() or session.user_id - ) - except Exception: - # Fail closed: an unanswerable visibility question must not fall back - # to "everything is available". - log.warning("agent.mcp_scope_failed", exc_info=True) - return current_allowed_mcp_servers.set(frozenset()) - return current_allowed_mcp_servers.set(allowed) + allowed, keyless = None, frozenset() + else: + try: + allowed = await self._mcp_manager.visible_servers( + current_user_role.get(), current_user_id.get() or session.user_id + ) + # Withheld = gated servers the run did not get to keep. None means + # "no restriction", so nothing was withheld. + keyless = ( + frozenset() + if allowed is None + else frozenset(self._mcp_manager.byok_servers() - allowed) + ) + except Exception: + # Fail closed: an unanswerable visibility question must not fall + # back to "everything is available". Every gated server then counts + # as withheld, which matches the empty tool list the model gets. + log.warning("agent.mcp_scope_failed", exc_info=True) + allowed = frozenset() + keyless = frozenset(self._mcp_manager.byok_servers()) + + return ( + current_allowed_mcp_servers.set(allowed), + current_keyless_mcp_servers.set(keyless), + ) async def _run_workers( self, diff --git a/navi/core/context_builder.py b/navi/core/context_builder.py index b26a1fc..f423676 100644 --- a/navi/core/context_builder.py +++ b/navi/core/context_builder.py @@ -414,15 +414,23 @@ instructions themselves used to be inlined here, which cost ~8 KB of prose on *every* request of a heavy profile; they are now read on demand with ``tool_manual("")``. + + A server whose tools were withheld for want of a personal key is still + named, followed by the reason. Listing it without one was the worst of + both: the model knew the server existed, had none of its tools, and had + nothing to tell the user. """ if not self._mcp_manager: return None if profile is not None and not profile.get_agent_tools().mcp: return None + from navi.tools._internal.base import current_keyless_mcp_servers as _keyless_var + server_names = set(profile.get_agent_tools().mcp.keys()) if profile is not None else None summaries = self._mcp_manager.get_summaries(server_names) if not summaries: return None + keyless = _keyless_var.get() lines = [ "[MCP servers — external knowledge sources]", "Their tools are named mcp____. Before using a server's tools, " @@ -432,6 +440,12 @@ for name in sorted(summaries): lines.append("") lines.append(f"## {name} — {summaries[name]}") + if name in keyless: + lines.append( + " (tools withheld: this server runs on a personal MCP key and none " + "is saved for this account — do not retry it or call tool_manual for " + "it; tell the user to add the key in Settings → MCP keys.)" + ) return Message(role="system", content="\n".join(lines)) def _truncate_oversized(self, conv: list[Message]) -> list[Message]: diff --git a/navi/tools/_internal/base.py b/navi/tools/_internal/base.py index 2e9c91b..dd79646 100644 --- a/navi/tools/_internal/base.py +++ b/navi/tools/_internal/base.py @@ -64,6 +64,17 @@ "current_allowed_mcp_servers", default=None ) +# Set by run_stream() next to the variable above: the BYOK-gated servers whose +# tools were dropped from this run for want of a personal key. It is what lets +# the run explain itself — ContextBuilder._mcp_context_msg names each of them +# where the server is listed, so the model can tell the user about the missing +# key instead of silently lacking the tools. Empty when nothing was withheld, +# which is also the default for runs that never set it (a peer run out of a +# fresh task), so an unset variable never claims a key is missing. +current_keyless_mcp_servers: ContextVar[frozenset[str]] = ContextVar( + "current_keyless_mcp_servers", default=frozenset() +) + # Set by run_stream() / messages endpoint to expose the full user profile to # ContextBuilder so the LLM receives [User context] with name, email, locale, etc. current_user_info: ContextVar[dict | None] = ContextVar("current_user_info", default=None) diff --git a/navi/tools/mcp_status.py b/navi/tools/mcp_status.py index 0d1a184..9134c3b 100644 --- a/navi/tools/mcp_status.py +++ b/navi/tools/mcp_status.py @@ -57,8 +57,9 @@ if cfg is not None and cfg.user_key is not None: dest = cfg.user_key.header or cfg.user_key.env lines.append( - f" (accepts a per-user key via {dest} — without one this " - "server runs on the shared default)" + f" (accepts a per-user key via {dest} — without one an ordinary " + "account is refused and the server's tools are not offered at all; " + "the shared default is admin-only. Keys live in Settings → MCP keys.)" ) try: tools = await client.list_tools() diff --git a/tests/unit/core/test_agent.py b/tests/unit/core/test_agent.py index 8bcca30..d746451 100644 --- a/tests/unit/core/test_agent.py +++ b/tests/unit/core/test_agent.py @@ -1193,3 +1193,125 @@ assert "could not be saved to the session directory" in notice assert "image_view" not in notice + + +# ─── MCP scope ────────────────────────────────────────────────────────────── + + +class _ScopeSession: + """The only field _apply_mcp_scope reads off a session.""" + + user_id = "u1" + + +class _ScopeManager: + """Just enough McpManager for the scope: what may be used, and what is gated.""" + + def __init__(self, allowed, gated, raises=False): + self._allowed = allowed + self._gated = set(gated) + self._raises = raises + self.seen = None + + async def visible_servers(self, role, user_id): + self.seen = (role, user_id) + if self._raises: + raise RuntimeError("key store on fire") + return self._allowed + + def byok_servers(self): + return set(self._gated) + + +def _agent_with_scope(allowed, gated, raises=False): + profiles = ProfileRegistry() + profiles.register(make_profile("test")) + return Agent( + session_store=InMemorySessionStore(), + profile_registry=profiles, + tool_registry=make_registry_with_tools(), + backend_registry=BackendRegistry(), + mcp_manager=_ScopeManager(allowed, gated, raises=raises), + ) + + +class TestMcpScopeContextVars: + """A run records what it may use *and* what it withheld for want of a key. + + The withheld set is not stored anywhere: it is the complement of what + visible_servers kept, which is why it is derived from that same value rather + than read a second time. These tests pin both halves down. + """ + + @pytest.mark.asyncio + async def test_the_withheld_servers_are_recorded_then_cleared(self): + from navi.tools._internal.base import ( + current_allowed_mcp_servers, + current_keyless_mcp_servers, + ) + + agent = _agent_with_scope(frozenset({"navi-web"}), {"gntodo", "gnexus-book"}) + + allowed_token, keyless_token = await agent._apply_mcp_scope(_ScopeSession()) + try: + assert current_allowed_mcp_servers.get() == frozenset({"navi-web"}) + assert current_keyless_mcp_servers.get() == frozenset({"gntodo", "gnexus-book"}) + finally: + current_allowed_mcp_servers.reset(allowed_token) + current_keyless_mcp_servers.reset(keyless_token) + + # Both tokens were handed back, so a caller that resets them leaves nothing + # behind for the next run in this task. + assert current_allowed_mcp_servers.get() is None + assert current_keyless_mcp_servers.get() == frozenset() + + @pytest.mark.asyncio + async def test_an_unrestricted_run_withholds_nothing(self): + """visible_servers returns None for an admin: no restriction, no reason.""" + from navi.tools._internal.base import ( + current_allowed_mcp_servers, + current_keyless_mcp_servers, + ) + + agent = _agent_with_scope(None, {"gntodo"}) + + allowed_token, keyless_token = await agent._apply_mcp_scope(_ScopeSession()) + try: + assert current_keyless_mcp_servers.get() == frozenset() + finally: + current_allowed_mcp_servers.reset(allowed_token) + current_keyless_mcp_servers.reset(keyless_token) + + @pytest.mark.asyncio + async def test_a_failed_visibility_check_withholds_every_gated_server(self): + """Fail closed: nothing is available, so every gated server is blamed.""" + from navi.tools._internal.base import ( + current_allowed_mcp_servers, + current_keyless_mcp_servers, + ) + + agent = _agent_with_scope(None, {"gntodo", "gnexus-book"}, raises=True) + + allowed_token, keyless_token = await agent._apply_mcp_scope(_ScopeSession()) + try: + assert current_allowed_mcp_servers.get() == frozenset() + assert current_keyless_mcp_servers.get() == frozenset({"gntodo", "gnexus-book"}) + finally: + current_allowed_mcp_servers.reset(allowed_token) + current_keyless_mcp_servers.reset(keyless_token) + + @pytest.mark.asyncio + async def test_the_scope_asks_about_the_session_user(self): + from navi.tools._internal.base import ( + current_allowed_mcp_servers, + current_keyless_mcp_servers, + ) + + agent = _agent_with_scope(frozenset(), set()) + + allowed_token, keyless_token = await agent._apply_mcp_scope(_ScopeSession()) + try: + assert agent._mcp_manager.seen == ("user", "u1") + finally: + current_allowed_mcp_servers.reset(allowed_token) + current_keyless_mcp_servers.reset(keyless_token) diff --git a/tests/unit/core/test_context_builder.py b/tests/unit/core/test_context_builder.py index a8547fe..9066f45 100644 --- a/tests/unit/core/test_context_builder.py +++ b/tests/unit/core/test_context_builder.py @@ -518,3 +518,57 @@ current_reaction_instructions.reset(token) assert "[Reaction session]" not in result[0].content + + +class TestKeylessMcpServersBlock: + """A server whose tools were withheld says why, right where it is listed. + + Listing it without a reason was the worst of both: the model knew the server + existed, had none of its tools, and had nothing to tell the user. + """ + + def _block_with(self, mcp_servers, keyless): + from navi.tools._internal.base import current_keyless_mcp_servers + + builder = ContextBuilder( + profile_registry=make_profile_registry(), mcp_manager=FakeMcpManager() + ) + profile = make_profile("test", mcp_servers=mcp_servers) + + token = current_keyless_mcp_servers.set(frozenset(keyless)) + try: + result = builder.build([Message(role="user", content="hi")], profile, mem=None) + finally: + current_keyless_mcp_servers.reset(token) + + block = next(m.content for m in result if "MCP servers" in (m.content or "")) + return result, block + + def test_the_reason_sits_under_the_server_it_belongs_to(self): + result, block = self._block_with({"gntodo": ["read"], "navi-web": ["search"]}, {"gntodo"}) + + lines = block.splitlines() + withheld = next(i for i, line in enumerate(lines) if line.startswith("## gntodo")) + assert "Settings → MCP keys" in lines[withheld + 1] + + # The server that kept its tools is left alone. + kept = next(i for i, line in enumerate(lines) if line.startswith("## navi-web")) + assert "MCP keys" not in lines[kept + 1 : kept + 2] + + # …and it stays in the context, never in a message the user would read. + assert not any("MCP keys" in (m.content or "") for m in result if m.role != "system") + + def test_no_reason_when_no_key_is_missing(self): + _, block = self._block_with({"gntodo": ["read"]}, set()) + + assert "MCP keys" not in block + # Two prose lines, one heading line per server — the note must not scale + # with anything that is not withheld. + assert len(block.splitlines()) <= 4 + + def test_nothing_is_withheld_by_default(self): + from navi.tools._internal.base import current_keyless_mcp_servers + + # A run that never set it (a peer run out of a fresh task) must not claim + # a key is missing. + assert current_keyless_mcp_servers.get() == frozenset()