From d6c9f08a597673f3c95e1ec0168719eb9db0e699 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 17 Aug 2026 12:54:58 -0400 Subject: [PATCH] fix(mcp): reject undeclared tool arguments instead of silently dropping them (#2709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FastMCP validates tool arguments with a pydantic model whose extra-field policy is 'ignore', so create_note(content=...) — a plausible near-miss for body=, primed by add_task_log's content — ran successfully, stored body: '', and left a record embedding/search cannot see. Two real notes were persisted body-less before the pattern was noticed; create_task only 'worked' because those calls happened to use the right name. StrictArgsFastMCP rejects any tool call carrying arguments the tool does not declare, before dispatch, with a did-you-mean hint when one is close and the declared list when none is. Applied at the dispatch seam so every tool gets the guarantee — an error the caller sees once beats data half-written forever. Co-Authored-By: Claude Fable 5 --- src/scribe/mcp/server.py | 44 +++++++++++++++++++- tests/test_mcp_strict_args.py | 75 +++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 tests/test_mcp_strict_args.py diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index be0653b..8dc5045 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -1,6 +1,8 @@ """FastMCP instance + Quart mount-point. Tools are registered in mcp/tools/.""" from __future__ import annotations +import difflib + from mcp.server.fastmcp import FastMCP from mcp.server.transport_security import TransportSecuritySettings from quart import Quart @@ -165,6 +167,46 @@ def _body_calls_write_tool(body: bytes) -> bool: return False +class StrictArgsFastMCP(FastMCP): + """A FastMCP that REJECTS tool calls carrying undeclared arguments. + + FastMCP validates arguments with a pydantic model built from the tool + signature, and pydantic's default extra-field policy is "ignore" — so a + misnamed argument simply vanishes and the tool runs with that field's + default. On the create/update tools the default is "", which turns a + plausible near-miss (`content=` for `body=`, primed by add_task_log's + `content`) into SILENT DATA LOSS: the call reports success and stores an + empty body, leaving a record search cannot see (#2709). Two notes were + persisted body-less that way before anyone noticed. + + An error the caller sees once is strictly better than data half-written + forever, so the policy is applied to every tool, not just the two that + bit: nothing here knows tool semantics, only that an argument nobody + declared cannot have been meant to be dropped. + """ + + async def call_tool(self, name, arguments): + try: + tool = self._tool_manager.get_tool(name) + except Exception: + tool = None # unknown tool → let upstream produce its own error + if tool is not None: + declared = set((tool.parameters or {}).get("properties", {})) + unknown = sorted(set(arguments or {}) - declared) + if unknown: + hints = [] + for arg in unknown: + close = difflib.get_close_matches(arg, sorted(declared), n=1) + suggestion = f" (did you mean '{close[0]}'?)" if close else "" + hints.append(f"'{arg}'{suggestion}") + raise ValueError( + f"{name} does not accept argument(s) {', '.join(hints)}. " + f"It accepts: {', '.join(sorted(declared))}. Nothing was " + "created or changed — retry with the declared names." + ) + return await super().call_tool(name, arguments) + + def build_mcp_server() -> FastMCP: """Build the FastMCP instance with all tools registered. @@ -185,7 +227,7 @@ def build_mcp_server() -> FastMCP: # every request self-contained (bearer-auth only), so a post-deploy # reconnect just works. Trade-off: no server-pushed list_changed stream, # which we don't use — tools are re-fetched on reconnect anyway. - mcp = FastMCP( + mcp = StrictArgsFastMCP( "scribe", instructions=_INSTRUCTIONS.strip(), stateless_http=True, diff --git a/tests/test_mcp_strict_args.py b/tests/test_mcp_strict_args.py new file mode 100644 index 0000000..3333f9f --- /dev/null +++ b/tests/test_mcp_strict_args.py @@ -0,0 +1,75 @@ +"""Unknown tool arguments are rejected, never silently dropped (#2709). + +The failure this pins: FastMCP validates tool arguments with a pydantic model +whose extra-field policy is "ignore", so `create_note(content=...)` — a +plausible near-miss for `body=`, primed by add_task_log's `content` — ran +successfully, stored `body: ""`, and left a record embedding/search cannot +see. The call REPORTED SUCCESS. Two real notes were persisted body-less +before the pattern was noticed. + +The fix is at the dispatch seam, not per-tool: StrictArgsFastMCP rejects any +call carrying arguments the tool does not declare, with a did-you-mean when +one is close. An error the caller sees once beats data half-written forever. +""" +import pytest + +from scribe.mcp.server import StrictArgsFastMCP + + +def _echo_server() -> StrictArgsFastMCP: + mcp = StrictArgsFastMCP("strict-test") + + @mcp.tool() + def echo(text: str = "") -> str: + return text + + return mcp + + +async def test_declared_arguments_still_dispatch(): + mcp = _echo_server() + result = await mcp.call_tool("echo", {"text": "hi"}) + assert "hi" in str(result) + + +async def test_unknown_argument_is_an_error_not_a_silent_drop(): + """The load-bearing property: the call must FAIL, because succeeding is + what turned a typo into data loss.""" + mcp = _echo_server() + with pytest.raises(ValueError) as exc: + await mcp.call_tool("echo", {"txt": "hi"}) + msg = str(exc.value) + assert "'txt'" in msg + assert "did you mean 'text'?" in msg # difflib near-miss hint + assert "Nothing was created or changed" in msg + + +async def test_empty_arguments_pass(): + mcp = _echo_server() + assert await mcp.call_tool("echo", {}) is not None + + +async def test_unknown_tool_keeps_the_upstream_error(): + """The gate must not swallow or reshape 'no such tool' — that error path + belongs to FastMCP and clients already understand it.""" + mcp = _echo_server() + with pytest.raises(Exception) as exc: + await mcp.call_tool("no_such_tool", {"text": "hi"}) + assert "no_such_tool" in str(exc.value) + + +async def test_the_original_regression_create_note_with_content(): + """Pin #2709 itself against the REAL server: `content=` on create_note + must raise before dispatch — naming the bad argument and listing `body` + among the accepted ones — instead of creating a body-less note.""" + from scribe.mcp.server import build_mcp_server + + mcp = build_mcp_server() + assert isinstance(mcp, StrictArgsFastMCP) # the guard is actually mounted + with pytest.raises(ValueError) as exc: + await mcp.call_tool( + "create_note", {"title": "t", "content": "the body text"} + ) + msg = str(exc.value) + assert "'content'" in msg + assert "body" in msg