From 4cf696fa42bf952d953c18e7e713c101ab95daa6 Mon Sep 17 00:00:00 2001 From: IceRhymers Date: Mon, 27 Jul 2026 12:27:02 -0700 Subject: [PATCH] app: FastMCP server instructions + tool annotations (#121) Agents did not reach for the code-search tools when asked about a repo that is not checked out locally -- they ran local Glob/Grep, reported not-found, and never consulted the index. A server's `instructions` string is returned in the `initialize` result and reaches the client unconditionally, whereas tool schemas may be deferred behind a tool-search step when many servers are connected. The server passed no `instructions`, so it contributed zero always-on context: only the bare tool names survived, and `search_code` next to a built-in grep reads like a worse grep. Extract `build_mcp() -> FastMCP` from `create_app()`. The Starlette app does not expose the FastMCP object it was built from, so tool metadata was unreachable without standing up a full HTTP session -- which would have put the tests in the e2e file that never executes. Each call still yields a fresh instance, preserving the single-use session-manager constraint. Add `title=` and read-only/open-world annotations to all six tools on semantic-accuracy grounds only; no routing or approval-friction credit is claimed. `_READ_ONLY` is a module constant because inlining pushes all six registration lines past line-length 100. Tool names, query semantics, and payload shapes are unchanged. The six tool docstrings are untouched -- that is #122, deferred behind #123. Co-Authored-By: Claude Opus 5 --- README.md | 6 ++ app/main.py | 63 ++++++++++++++++----- tests/unit/test_mcp_descriptions.py | 85 +++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 14 deletions(-) create mode 100644 tests/unit/test_mcp_descriptions.py diff --git a/README.md b/README.md index e553810..aaa2113 100644 --- a/README.md +++ b/README.md @@ -596,6 +596,12 @@ Note that auto-login fires **only** for OAuth (`databricks-cli`) profiles. On PA Azure profiles the proxy reports the failure rather than re-running login, so it cannot overwrite credentials you did not ask it to touch. +Registering the server is also all it takes to deliver its guidance to the client: the MCP +`initialize` handshake delivers the server's `instructions` to the client automatically, with +no per-client or per-repo configuration needed. Any MCP client speaking the protocol receives +it — Claude Code, Cursor, Windsurf, VS Code — not just the one shown above. What a client then +does with it is client behavior, not something the protocol guarantees. + ### Option B: native OAuth app connection Use this if you want the client to hold its own OAuth registration rather than ride on your diff --git a/app/main.py b/app/main.py index eec9bc2..819d3c2 100644 --- a/app/main.py +++ b/app/main.py @@ -44,6 +44,7 @@ import anyio from mcp.server.fastmcp import Context, FastMCP +from mcp.types import ToolAnnotations from sqlalchemy import select from sqlalchemy.engine import Engine from starlette.applications import Starlette @@ -677,7 +678,7 @@ async def lifespan(server: FastMCP) -> AsyncIterator[dict[str, Any]]: # ---------------------------------------------------------------------------------- tools # # Tools and routes are plain module functions registered onto a fresh ``FastMCP`` by -# ``create_app()`` below (NOT decorated onto one module-global instance). A +# ``build_mcp()`` below (NOT decorated onto one module-global instance). A # ``streamable_http_app`` caches a single ``StreamableHTTPSessionManager`` whose ``run()`` may # be entered only once per instance, so a per-instance factory is what lets each test (and any # future multi-mount) get its own session manager instead of reusing a spent one. @@ -1047,21 +1048,55 @@ def probe() -> None: # --------------------------------------------------------------------------- ASGI export - -def create_app() -> Starlette: - """Build a fresh MCP ASGI app: a new ``FastMCP`` with tools/routes registered and its own - single-use ``StreamableHTTPSessionManager``. Production uses the module ``app`` below; tests - call this per test so each gets an unspent session manager (see the tools comment above).""" - mcp = FastMCP("code-search", lifespan=lifespan) - mcp.tool()(search_code) - mcp.tool()(semantic_search) - mcp.tool()(list_repos) - mcp.tool()(get_file) - mcp.tool()(find_references) - mcp.tool()(list_imports) +# Server-level ``instructions`` land verbatim in every client's system prompt (via +# ``initialize``), so this is the always-on channel: it answers "should I engage this server at +# all" and routes the four entry-level tools. It deliberately does NOT enumerate +# ``find_references``/``list_imports`` -- those are discoverable from the tool list once an +# agent has engaged -- and it routes discovery through ``list_repos`` rather than asserting a +# corpus exists, so it stays true against a freshly forked template with an empty index. +SERVER_INSTRUCTIONS = """ +Indexed code search across many repositories -- including repositories that are NOT +checked out in the current working directory. + +Use these tools whenever a request names a repository, project, file, or symbol you +cannot find locally. Local file tools only see the working directory; do not conclude +that something does not exist based on a local search alone -- check list_repos first +to see what is indexed. + +Then: search_code for exact text, regex, or symbol matches; semantic_search for +natural-language questions about behavior; get_file to read a known path in full. +""".strip() + +# All six tools are genuinely read-only against an external, open-world index (semantic-accuracy +# grounds only -- Claude Code does not auto-approve on ``readOnlyHint``, so no routing credit is +# claimed). Module-level constant because inlining ``ToolAnnotations(...)`` six times pushes +# every registration line past ``line-length = 100`` (pyproject.toml). +_READ_ONLY = ToolAnnotations(readOnlyHint=True, openWorldHint=True) + + +def build_mcp() -> FastMCP: + """Build a fresh ``FastMCP`` with tools/routes registered and its own single-use + ``StreamableHTTPSessionManager``. Split out of ``create_app()`` so tool metadata + (``instructions``, ``list_tools()``) is reachable and unit-testable without standing up a + full HTTP session -- the Starlette app returned by ``streamable_http_app()`` does not expose + the ``FastMCP`` object it was built from. Each call still yields a fresh instance, preserving + the single-use session-manager constraint described in the tools comment above.""" + mcp = FastMCP("code-search", instructions=SERVER_INSTRUCTIONS, lifespan=lifespan) + mcp.tool(title="Search Code", annotations=_READ_ONLY)(search_code) + mcp.tool(title="Semantic Search", annotations=_READ_ONLY)(semantic_search) + mcp.tool(title="List Indexed Repositories", annotations=_READ_ONLY)(list_repos) + mcp.tool(title="Get File", annotations=_READ_ONLY)(get_file) + mcp.tool(title="Find References", annotations=_READ_ONLY)(find_references) + mcp.tool(title="List Imports", annotations=_READ_ONLY)(list_imports) mcp.custom_route("/health", methods=["GET"])(health) mcp.custom_route("/ready", methods=["GET"])(ready) - return mcp.streamable_http_app() + return mcp + + +def create_app() -> Starlette: + """Build a fresh MCP ASGI app. Production uses the module ``app`` below; tests call this + per test so each gets an unspent session manager (see the tools comment above).""" + return build_mcp().streamable_http_app() app = create_app() diff --git a/tests/unit/test_mcp_descriptions.py b/tests/unit/test_mcp_descriptions.py new file mode 100644 index 0000000..1a7cbe4 --- /dev/null +++ b/tests/unit/test_mcp_descriptions.py @@ -0,0 +1,85 @@ +"""Unit tests for the MCP discoverability surface added in app/main.py (issue #121): +``build_mcp()``'s server-level ``instructions=`` and the per-tool ``title=``/``annotations=``. + +No DB, no SDK session: ``FastMCP.instructions`` is a plain property and ``list_tools()`` is +in-process metadata introspection over the registered tool functions, so these run under +``tests/unit/`` (unlike ``tests/integration/test_mcp_server.py``, which is ``@pytest.mark.e2e`` +and never executes under ``make test``'s ``-m "unit or observability"`` selection). +""" + +from __future__ import annotations + +import re + +import pytest + +from app.main import SERVER_INSTRUCTIONS, build_mcp +from app.query.parser import _SUPPORTED + +# --------------------------------------------------------------------------------- instructions + + +@pytest.mark.unit +def test_instructions_present_and_routes_discovery() -> None: + instr = build_mcp().instructions + assert instr is not None + assert instr.strip() != "" + assert "list_repos" in instr + # Without these, the AC is unenforced: the pre-change baseline is ``instructions=None``, so + # *any* non-empty string would pass. These pin the not-found-locally trigger specifically. + assert re.search(r"working directory", instr, re.I) + assert re.search(r"cannot find locally", instr, re.I) + # Binds this test's subject to the constant the other test checks. Without this, a different + # string passed to ``FastMCP(instructions=...)`` in ``build_mcp()`` could evade the denylist + # and length cap in ``test_instructions_boundary_denylist_and_length_cap`` below while still + # passing this test. + assert instr == SERVER_INSTRUCTIONS + + +@pytest.mark.unit +def test_instructions_boundary_denylist_and_length_cap() -> None: + """Denylist is PRIMARY, the 650-char cap is secondary. An earlier draft smuggled the entire + query grammar into ``instructions`` at 731 chars under a 900-char cap while hitting zero + denylist tokens -- a length-only check does not catch grammar creep, so the denylist is the + real boundary and the cap is only a backstop against unbounded growth. + + The field-name half of the denylist is DERIVED from ``_SUPPORTED`` rather than hand-copied + (the repo convention stated at app/query/semantic_filters.py:26-27: field names are recovered + by inverting the parser's own map, "never a second hand-written table") so a future field + added to the query grammar is denylisted automatically. + """ + instr = SERVER_INSTRUCTIONS + grammar = {f"{f}:" for f in _SUPPORTED} | { + "/regex/", + "next_cursor", + "max_bytes", + "truncation_reason", + } + for token in grammar: + assert token not in instr, f"instructions leaked query-grammar token: {token!r}" + assert len(instr) <= 650 + + +# --------------------------------------------------------------------------------- tool metadata + +_EXPECTED_TOOLS = { + "search_code", + "semantic_search", + "list_repos", + "get_file", + "find_references", + "list_imports", +} + + +@pytest.mark.unit +async def test_tool_metadata_titles_and_annotations() -> None: + tools = await build_mcp().list_tools() + names = {tool.name for tool in tools} + assert names == _EXPECTED_TOOLS + for tool in tools: + assert tool.description + assert tool.title + assert tool.annotations is not None + assert tool.annotations.readOnlyHint is True + assert tool.annotations.openWorldHint is True