From 0c12e2a376b307dab495b04ce10b795ec6447e21 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 15:26:23 -0300 Subject: [PATCH 01/11] feat(mcp): expose request-scoped caller identity for middleware Add CallerIdentity (client id + scopes) and caller_identity(request), reading the validated caller off the request scope without re-decoding the bearer. Extract a shared _authenticated_user so caller_identity and require_request_bearer locate the validated user the same way; caller_identity is non-raising so it runs on every profile (anonymous under stdio/local). Subject is deferred until its consumer (per-user quotas) exists. --- .../src/pipefy_mcp/auth/request_identity.py | 58 +++++++++++++++++-- .../mcp/tests/auth/test_request_identity.py | 45 +++++++++++++- 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/packages/mcp/src/pipefy_mcp/auth/request_identity.py b/packages/mcp/src/pipefy_mcp/auth/request_identity.py index 06008435..3ae6e203 100644 --- a/packages/mcp/src/pipefy_mcp/auth/request_identity.py +++ b/packages/mcp/src/pipefy_mcp/auth/request_identity.py @@ -18,10 +18,59 @@ from __future__ import annotations +from dataclasses import dataclass, field + from mcp.server.auth.middleware.auth_context import AuthenticatedUser from starlette.requests import Request +@dataclass(frozen=True) +class CallerIdentity: + """The validated caller a tool-call middleware acts on behalf of. + + Sourced from the request's validated bearer, never re-decoded. ``client_id`` + is the OAuth client (``azp``/``client_id``, falling back to ``sub`` for a + user token) and ``scopes`` its granted scopes. Both default empty: under the + stdio/local profile there is no inbound bearer, so middleware sees an + anonymous identity rather than a failure. + + The end-user subject is intentionally absent: exposing it needs an + ``AccessToken`` subtype and its only consumer is per-user quotas, which are + not built yet. Add ``subject`` here when that lands. + """ + + client_id: str | None = None + scopes: tuple[str, ...] = field(default_factory=tuple) + + +def _authenticated_user(request: Request | None) -> AuthenticatedUser | None: + """Return the RS-validated user off the request, or None. + + Reads ``request.scope`` directly rather than ``request.user``: the latter + asserts ``AuthenticationMiddleware`` is installed, which holds only under the + ``remote`` profile, so touching the property would raise on a ``local`` HTTP + request. This is the one place both request-scoped readers agree on how the + validated caller is located (per-message request, never ``auth_context_var``, + which stateful Streamable HTTP freezes at the session's first bearer). + """ + user = request.scope.get("user") if request is not None else None + return user if isinstance(user, AuthenticatedUser) else None + + +def caller_identity(request: Request | None) -> CallerIdentity: + """Return the validated caller off the in-flight request, or an anonymous one. + + Runs on every profile (a tool-call middleware wraps both transports), so a + missing or non-authenticated user yields the empty :class:`CallerIdentity` + rather than an error. + """ + user = _authenticated_user(request) + if user is None: + return CallerIdentity() + token = user.access_token + return CallerIdentity(client_id=token.client_id, scopes=tuple(token.scopes)) + + def require_request_bearer(request: Request | None) -> str: """Return the validated bearer token off the in-flight request. @@ -34,13 +83,10 @@ def require_request_bearer(request: Request | None) -> str: Raises when no validated bearer is present (called outside the resource-server request scope, or the request bore no ``AuthenticatedUser``), so a missing - identity fails loudly instead of issuing an unauthenticated Pipefy call. The - ``isinstance`` guard keeps that fail-loud property on a typed contract: an - unauthenticated request carries a different user type, not an - ``AuthenticatedUser`` with an empty token. + identity fails loudly instead of issuing an unauthenticated Pipefy call. """ - user = request.user if request is not None else None - if not isinstance(user, AuthenticatedUser) or not user.access_token.token: + user = _authenticated_user(request) + if user is None or not user.access_token.token: raise RuntimeError( "No authenticated access token on the request; the resource-server " "profile must validate a bearer before a tool runs." diff --git a/packages/mcp/tests/auth/test_request_identity.py b/packages/mcp/tests/auth/test_request_identity.py index 744a4635..49b33fc6 100644 --- a/packages/mcp/tests/auth/test_request_identity.py +++ b/packages/mcp/tests/auth/test_request_identity.py @@ -12,8 +12,15 @@ import pytest from _rs_fixtures import authenticated_user, request_with_user +from mcp.server.auth.middleware.auth_context import AuthenticatedUser +from mcp.server.auth.provider import AccessToken +from starlette.requests import Request -from pipefy_mcp.auth.request_identity import require_request_bearer +from pipefy_mcp.auth.request_identity import ( + CallerIdentity, + caller_identity, + require_request_bearer, +) @pytest.mark.unit @@ -42,3 +49,39 @@ def test_raises_on_empty_token(): """An access token with an empty string is treated as absent.""" with pytest.raises(RuntimeError, match="No authenticated access token"): require_request_bearer(request_with_user(authenticated_user(""))) + + +@pytest.mark.unit +def test_caller_identity_reads_client_id_and_scopes(): + """The validated caller's client id and scopes come off the request's token.""" + user = AuthenticatedUser( + AccessToken(token="t", client_id="acting-client", scopes=["read", "write"]) + ) + identity = caller_identity(request_with_user(user)) + assert identity == CallerIdentity( + client_id="acting-client", scopes=("read", "write") + ) + + +@pytest.mark.unit +def test_caller_identity_is_anonymous_without_a_request(): + """No request (stdio, or outside the request scope) yields the empty identity.""" + assert caller_identity(None) == CallerIdentity() + + +@pytest.mark.unit +def test_caller_identity_is_anonymous_without_a_validated_user(): + """A request whose user is not authenticated yields the empty identity.""" + assert caller_identity(request_with_user(None)) == CallerIdentity() + + +@pytest.mark.unit +def test_caller_identity_survives_a_scope_without_a_user_key(): + """A local-over-HTTP request has no ``user`` in scope; reading it must not raise. + + ``AuthenticationMiddleware`` runs only under the remote profile, so a + ``local`` HTTP request never gets a ``user`` scope key. ``request.user`` would + assert; ``caller_identity`` reads ``scope`` directly and returns anonymous. + """ + request = Request({"type": "http", "headers": []}) + assert caller_identity(request) == CallerIdentity() From 53a4dc3f69e22b95d474b33cec3c13d22bf66d63 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 15:26:46 -0300 Subject: [PATCH 02/11] feat(mcp): add tool-call middleware seam Turn FastMCP's single CallToolRequest handler slot into an ordered, documented middleware chain so cross-cutting concerns (logging, quotas, rate limiting) layer without overwriting the private slot. Middleware register on McpRuntime and run outer-to-inner, may short-circuit with a canonical error result, and see the caller identity, tool name, bounded argument keys, and a request id. The chain wraps the handler once at build time (in server.py, the wiring root), is a no-op when nothing is registered, and is idempotent per app. A structured tool-log middleware ships as the first consumer, seeded only under the remote profile and emitting JSON to stderr so it never corrupts the stdio stream. --- packages/mcp/AGENTS.md | 54 ++++ packages/mcp/src/pipefy_mcp/core/runtime.py | 26 +- .../src/pipefy_mcp/core/tool_middleware.py | 244 +++++++++++++++++ .../src/pipefy_mcp/observability/__init__.py | 5 + .../observability/tool_log_middleware.py | 91 +++++++ packages/mcp/src/pipefy_mcp/server.py | 6 + packages/mcp/tests/core/test_runtime.py | 20 ++ .../mcp/tests/core/test_tool_middleware.py | 254 ++++++++++++++++++ packages/mcp/tests/observability/__init__.py | 0 .../observability/test_tool_log_middleware.py | 130 +++++++++ 10 files changed, 829 insertions(+), 1 deletion(-) create mode 100644 packages/mcp/src/pipefy_mcp/core/tool_middleware.py create mode 100644 packages/mcp/src/pipefy_mcp/observability/__init__.py create mode 100644 packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py create mode 100644 packages/mcp/tests/core/test_tool_middleware.py create mode 100644 packages/mcp/tests/observability/__init__.py create mode 100644 packages/mcp/tests/observability/test_tool_log_middleware.py diff --git a/packages/mcp/AGENTS.md b/packages/mcp/AGENTS.md index 70f169d0..7c3fb597 100644 --- a/packages/mcp/AGENTS.md +++ b/packages/mcp/AGENTS.md @@ -169,3 +169,57 @@ than a local `file_path`), enforce that in the tool body gated on `settings.mcp.profile == "remote"` at call time, not via the marker. A tool whose exclusion deserves a reason gets a plain code comment stating why; the exclusion itself needs no annotation. + +## Tool-call middleware + +Cross-cutting concerns that wrap a tool invocation (logging, per-user quotas, +rate limiting, cost weighting, downstream 429/circuit-breaking) register as +ordered middleware, not by overwriting the server's internal handler. The MCP SDK +dispatches every tool call through one `request_handlers[CallToolRequest]` slot; +`core/tool_middleware.py` wraps that slot once, at build time, and composes the +registered middleware around it. The private slot is no longer the extension +surface. + +Register through the runtime, never by touching FastMCP internals: + +```python +from pipefy_mcp.core.tool_middleware import ToolCallContext, CallNext, short_circuit_error + +async def quota(ctx: ToolCallContext, call_next: CallNext): + if over_quota(ctx.identity.client_id): + return short_circuit_error("quota exceeded", code="RATE_LIMITED") + return await call_next(ctx) + +runtime.register_tool_middleware(quota) # before install_tool_call_middleware(app) +``` + +- **Order**: registration order runs outer to inner around the tool. `[A, B]` + runs A, then B, then the tool, and unwinds in reverse. +- **Short-circuit**: a middleware that returns without awaiting `call_next` skips + the inner chain and the tool. Use `short_circuit_error`, which carries the + canonical `tool_error` envelope but sets `isError=True` deliberately: a + governance stop means the tool never ran, distinct from a tool that ran and + reported a business error (`isError=False`). +- **Identity** (`ctx.identity`): the validated caller's `client_id` and `scopes`, + read off the request's bearer, never re-decoded. Empty under stdio/local (no + inbound bearer). The end-user `subject` is intentionally absent until its + consumer (per-user quotas) exists. +- **`request_id`**: correlates a call to its HTTP request when available, else the + JSON-RPC message id, which is client-chosen and only unique within a session. +- **Raw arguments**: FastMCP registers the terminal with `validate_input=False` + and coerces arguments downstream, so middleware sees the un-coerced, client-sent + arguments. `ctx.argument_keys` is bounded (count and length caps) and values-free + for privacy-sensitive consumers; `ctx.arguments` values are passed unbounded to + any consumer that opts to read them. Never log a bearer or argument values. + +The chain installs on every profile (a pass-through when nothing is registered); +the built-in structured logger (`observability/tool_log_middleware.py`) is seeded +only under the `remote` profile. That logger emits through the `logging` module +(stderr), never stdout, which is the stdio transport's JSON-RPC stream. + +The wrap targets a FastMCP internal and is tested against `mcp==1.25.0`; the +install is idempotent per app (the sentinel is per handler, not a global). This is +a separate seam from the argument-validation envelope +(`tools/validation_envelope.py`), which patches `Tool.run` to reshape a pydantic +`ValidationError` inside FastMCP's executor: that error's structured detail exists +only there, below this chain, so the two are complementary, not interchangeable. diff --git a/packages/mcp/src/pipefy_mcp/core/runtime.py b/packages/mcp/src/pipefy_mcp/core/runtime.py index bdb9d193..366c0405 100644 --- a/packages/mcp/src/pipefy_mcp/core/runtime.py +++ b/packages/mcp/src/pipefy_mcp/core/runtime.py @@ -19,6 +19,8 @@ ResourceServerAuth, build_resource_server_auth, ) +from pipefy_mcp.core.tool_middleware import ToolCallMiddleware +from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.settings import Settings @@ -133,6 +135,7 @@ def __init__( self._identity = identity self.inbound_auth = inbound_auth self._engine = PipefyEngine.build(settings.pipefy, surface="mcp") + self._tool_middlewares: list[ToolCallMiddleware] = [] @classmethod def for_profile(cls, settings: Settings) -> McpRuntime: @@ -164,7 +167,13 @@ def for_profile(cls, settings: Settings) -> McpRuntime: "PIPEFY_MCP_RS_RESOURCE_SERVER_URL so the server validates a " "per-request bearer and acts on behalf of the caller." ) - return cls(settings, RequestScopedIdentity(), inbound_auth=inbound_auth) + runtime = cls(settings, RequestScopedIdentity(), inbound_auth=inbound_auth) + # Structured tool-call logging is a hosted-operations concern; seed it + # only under the remote profile. The chain itself is installed on every + # profile (see install_tool_call_middleware), but the built-in logger + # stays off the stdio/local path. + runtime.register_tool_middleware(tool_log_middleware) + return runtime return cls(settings, StartupIdentity.from_configured_credential(settings)) def session_for_request(self, request: Request | None) -> PipefyClient: @@ -178,6 +187,21 @@ def session_for_request(self, request: Request | None) -> PipefyClient: """ return self._engine.session(self._identity.resolve(request)) + def register_tool_middleware(self, middleware: ToolCallMiddleware) -> None: + """Register a tool-call middleware, run outer-to-inner in registration order. + + The public seam for cross-cutting concerns (logging, quotas, rate limiting): + callers register here and never touch the server's internal request handlers. + The wiring layer reads :attr:`tool_middlewares` and installs the chain onto + the app at build time. + """ + self._tool_middlewares.append(middleware) + + @property + def tool_middlewares(self) -> tuple[ToolCallMiddleware, ...]: + """The registered tool-call middleware, in registration order.""" + return tuple(self._tool_middlewares) + @property def settings(self) -> Settings: """The resolved settings this runtime was built from.""" diff --git a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py new file mode 100644 index 00000000..12ef9ecd --- /dev/null +++ b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py @@ -0,0 +1,244 @@ +"""Ordered middleware chain around MCP tool invocation. + +The hosted profile needs to run cross-cutting logic around each tool call with +the caller's validated identity in hand: logging, per-user quotas, rate limiting, +cost weighting, downstream 429/circuit-breaking. The MCP SDK offers no seam for +this: FastMCP dispatches every tool call through a single ``CallToolRequest`` +entry in the low-level server's ``request_handlers`` dict, and the only way to +observe or govern a call is to overwrite that one private slot, so the next +feature that needs it clobbers the previous. + +This module turns that single slot into an ordered chain. Middleware register on +:class:`~pipefy_mcp.core.runtime.McpRuntime` (the public seam) and this module +wraps FastMCP's handler exactly once, at build time, composing the registered +middleware around it. Middleware run outer-to-inner in registration order, may +short-circuit (return an error result without invoking the tool), and read the +validated caller from the per-message request context. + +The wrap targets ``app._mcp_server.request_handlers[CallToolRequest]`` and is +tested against ``mcp==1.25.0``. If that pin moves, re-verify the handler is still +a single ``async def handler(req: CallToolRequest) -> ServerResult`` populated by +``FastMCP._setup_handlers``. +""" + +from __future__ import annotations + +import json +from collections.abc import Awaitable, Callable, Sequence +from dataclasses import dataclass +from typing import TYPE_CHECKING, Any + +from mcp import types +from mcp.server.lowlevel.server import request_ctx + +from pipefy_mcp.auth.request_identity import CallerIdentity, caller_identity + +if TYPE_CHECKING: + from mcp.server.fastmcp import FastMCP + +__all__ = [ + "CallNext", + "CallerIdentity", + "ToolCallContext", + "ToolCallMiddleware", + "compose", + "context_from_request", + "install_tool_call_middleware", + "short_circuit_error", +] + +# The bounded, values-free view of a call's arguments. Keys come straight off the +# JSON-RPC body before any validation, so an authenticated caller controls their +# count and length; the caps keep one call from flooding a downstream log line. +_MAX_ARG_KEYS = 64 +_MAX_ARG_KEY_LENGTH = 200 + +# Marks the installed chain handler so a second install on the same app is a +# no-op. The sentinel lives on the wrapped function (per app), not on a module or +# class global: each FastMCP instance has its own handler, so a global would wrap +# the first app and silently skip every later one built in the same process. +_INSTALLED_MARKER = "__pipefy_tool_call_chain__" + + +@dataclass(frozen=True) +class ToolCallContext: + """What a tool-call middleware sees for one invocation. + + ``arguments`` is the raw JSON-RPC argument map: FastMCP registers the terminal + with ``validate_input=False`` and does its own coercion/defaulting downstream, + so middleware observes the un-coerced, client-sent arguments (defaults not + applied, types not coerced). ``argument_keys`` is the bounded, values-free view + a privacy-sensitive consumer (logging) should prefer; ``arguments`` values are + passed unbounded to any consumer that opts to read them. + + ``request_id`` correlates a call to its HTTP request when one is available; + otherwise it falls back to the JSON-RPC message id, which is client-chosen and + only unique within a session (see :func:`context_from_request`). ``req`` is the + untouched request the terminal handler is called with. + """ + + tool_name: str + arguments: dict[str, Any] | None + argument_keys: tuple[str, ...] + identity: CallerIdentity + request_id: str | None + req: types.CallToolRequest + + +# A middleware calls ``call_next(ctx)`` to run the inner chain (ending at the +# tool), or returns its own ServerResult to short-circuit without running it. +CallNext = Callable[[ToolCallContext], Awaitable[types.ServerResult]] +ToolCallMiddleware = Callable[ + [ToolCallContext, CallNext], Awaitable[types.ServerResult] +] + +# FastMCP's registered ``CallToolRequest`` handler: the innermost callable the +# chain wraps. +_TerminalHandler = Callable[[types.CallToolRequest], Awaitable[types.ServerResult]] + + +def _argument_keys(arguments: dict[str, Any] | None) -> tuple[str, ...]: + """Sorted, values-free argument key names, bounded so one call can't flood a log.""" + if not arguments: + return () + keys = sorted(str(key)[:_MAX_ARG_KEY_LENGTH] for key in arguments) + if len(keys) > _MAX_ARG_KEYS: + dropped = len(keys) - _MAX_ARG_KEYS + keys = keys[:_MAX_ARG_KEYS] + keys.append(f"...+{dropped} more") + return tuple(keys) + + +def _request_scope() -> tuple[Any | None, str | None]: + """Return ``(request, request_id)`` from the in-flight message context. + + The low-level server sets ``request_ctx`` per JSON-RPC message before calling + the handler, so both are available at chain entry. ``request`` is the Starlette + request under HTTP (``None`` over stdio). ``request_id`` prefers an HTTP-layer + correlation id stamped into ``scope["state"]`` (what a request-logging ASGI + middleware would set) and falls back to the JSON-RPC message id. + """ + try: + ctx = request_ctx.get() + except LookupError: + return None, None + + request = ctx.request + request_id: Any = None + if request is not None: + request_id = request.scope.get("state", {}).get("request_id") + if request_id is None: + request_id = ctx.request_id + return request, (str(request_id) if request_id is not None else None) + + +def context_from_request(req: types.CallToolRequest) -> ToolCallContext: + """Build the :class:`ToolCallContext` for the in-flight call.""" + request, request_id = _request_scope() + arguments = req.params.arguments + return ToolCallContext( + tool_name=req.params.name, + arguments=arguments, + argument_keys=_argument_keys(arguments), + identity=caller_identity(request), + request_id=request_id, + req=req, + ) + + +def compose( + middlewares: Sequence[ToolCallMiddleware], terminal: _TerminalHandler +) -> CallNext: + """Fold ``middlewares`` around ``terminal`` into one ``call_next``. + + Registration order is outer-to-inner: ``[A, B]`` runs ``A`` then ``B`` then the + tool, and unwinds in reverse. A middleware that returns without awaiting + ``call_next`` short-circuits the rest of the chain and the tool. + """ + + async def call_terminal(ctx: ToolCallContext) -> types.ServerResult: + return await terminal(ctx.req) + + call_next: CallNext = call_terminal + for middleware in reversed(list(middlewares)): + call_next = _bind(middleware, call_next) + return call_next + + +def _bind(middleware: ToolCallMiddleware, call_next: CallNext) -> CallNext: + """Bind one middleware ahead of ``call_next`` (own scope, no late binding).""" + + async def run(ctx: ToolCallContext) -> types.ServerResult: + return await middleware(ctx, call_next) + + return run + + +def short_circuit_error( + message: str, + *, + code: str | None = None, + details: dict[str, Any] | None = None, +) -> types.ServerResult: + """A short-circuit result a middleware returns instead of running the tool. + + Carries the canonical ``tool_error`` envelope so the body matches an in-tool + failure, but sets ``isError=True`` deliberately: a governance stop (quota, + rate limit) means the tool never ran, which is distinct from a tool that ran + and reported a business error (``isError=False``). Because this bypasses + FastMCP's terminal, it replicates FastMCP's dict normalization itself (the + same envelope in both ``structuredContent`` and serialized ``content``). + """ + # Imported lazily: pipefy_mcp.tools.tool_error_envelope pulls the tools + # package, which imports the runtime, which imports this module. A top-level + # import would form a cycle at load time. + from pipefy_mcp.tools.tool_error_envelope import tool_error + + envelope = tool_error(message, code=code, details=details) + return types.ServerResult( + types.CallToolResult( + content=[ + types.TextContent(type="text", text=json.dumps(envelope, indent=2)) + ], + structuredContent=envelope, + isError=True, + ) + ) + + +def install_tool_call_middleware( + app: FastMCP, middlewares: Sequence[ToolCallMiddleware] +) -> None: + """Wrap ``app``'s ``CallToolRequest`` handler with the composed chain, once. + + A no-op when ``middlewares`` is empty: FastMCP's handler is left untouched, so + the default (no middleware) path pays nothing per call rather than routing every + tool call through a pass-through wrapper that builds and discards a context. + + Idempotent per app: the sentinel lives on the installed handler, so a repeat + install on the same app is a no-op while a different app (built later in the + same process) wraps its own fresh handler. Raises if FastMCP has not registered + the handler, so an SDK-internal change fails loud rather than silently skipping + the chain. + """ + if not middlewares: + return + + handlers = app._mcp_server.request_handlers + if types.CallToolRequest not in handlers: + raise RuntimeError( + "CallToolRequest handler missing from request_handlers; the FastMCP " + f"internal contract changed. Present keys: {list(handlers.keys())!r}" + ) + + terminal = handlers[types.CallToolRequest] + if getattr(terminal, _INSTALLED_MARKER, False): + return + + chain = compose(middlewares, terminal) + + async def chained(req: types.CallToolRequest) -> types.ServerResult: + return await chain(context_from_request(req)) + + setattr(chained, _INSTALLED_MARKER, True) + handlers[types.CallToolRequest] = chained diff --git a/packages/mcp/src/pipefy_mcp/observability/__init__.py b/packages/mcp/src/pipefy_mcp/observability/__init__.py new file mode 100644 index 00000000..54208fd4 --- /dev/null +++ b/packages/mcp/src/pipefy_mcp/observability/__init__.py @@ -0,0 +1,5 @@ +"""Hosted-profile observability: middleware that ride the tool-call chain.""" + +from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware + +__all__ = ["tool_log_middleware"] diff --git a/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py new file mode 100644 index 00000000..0ea61fa8 --- /dev/null +++ b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py @@ -0,0 +1,91 @@ +"""Structured per-tool-call logging as a tool-call middleware. + +The first consumer of the tool-call middleware chain (see +:mod:`pipefy_mcp.core.tool_middleware`): it emits one JSON line per call with the +tool name, outcome, duration, the caller's client id, argument key names, and a +request id for correlation. It proves a real cross-cutting concern rides the seam +without touching FastMCP internals. + +Privacy: never logs the bearer, and never logs argument values, only their +(bounded) key names. + +Output goes through the standard ``logging`` module, which writes to stderr. It +must never write to stdout: the stdio transport frames JSON-RPC over stdout, so a +stray log line there corrupts the protocol. +""" + +from __future__ import annotations + +import json +import logging +import time +from typing import Literal + +from mcp import types + +from pipefy_mcp.core.tool_middleware import CallNext, ToolCallContext + +logger = logging.getLogger("pipefy_mcp.observability.tool_call") + +ToolCallOutcome = Literal["ok", "error"] + + +def _outcome(result: types.ServerResult) -> ToolCallOutcome: + """Map a terminal result to an outcome. + + Reads ``isError`` off the result's ``CallToolResult`` root. The attribute is + read defensively: an experimental ``CreateTaskResult`` root has no ``isError``, + which should read as ``ok`` rather than raise. + """ + root = getattr(result, "root", None) + return "error" if getattr(root, "isError", False) else "ok" + + +def _emit(event: dict[str, object]) -> None: + """Write one structured line via logging (stderr), never stdout. + + Serialization runs before ``logger.info``, so it would otherwise be paid even + when the line is discarded; the ``isEnabledFor`` guard skips it when the level + is off. + """ + if not logger.isEnabledFor(logging.INFO): + return + logger.info(json.dumps(event, separators=(",", ":"))) + + +async def tool_log_middleware( + ctx: ToolCallContext, call_next: CallNext +) -> types.ServerResult: + """Emit one structured log line around a tool call, then propagate the result. + + A tool body error surfaces as a result with ``isError=True`` (FastMCP's + terminal turns tool exceptions into an error result), so the common path logs + from the returned result. The exceptions that DO propagate through the chain, + ``CancelledError`` (client disconnect / ``notifications/cancelled``) and + ``UrlElicitationRequiredError``, are logged as errors and re-raised: swallowing + them would break cancellation and elicitation. ``BaseException`` (not + ``Exception``) is caught so a ``CancelledError`` cannot skip the handler and let + ``finally`` emit a stale ``ok``. + """ + started_at = time.perf_counter() + outcome: ToolCallOutcome = "ok" + try: + result = await call_next(ctx) + outcome = _outcome(result) + return result + except BaseException: + outcome = "error" + raise + finally: + duration_ms = round((time.perf_counter() - started_at) * 1000, 3) + _emit( + { + "event": "tool_call", + "tool": ctx.tool_name, + "outcome": outcome, + "duration_ms": duration_ms, + "arg_keys": list(ctx.argument_keys), + "client_id": ctx.identity.client_id, + "request_id": ctx.request_id, + } + ) diff --git a/packages/mcp/src/pipefy_mcp/server.py b/packages/mcp/src/pipefy_mcp/server.py index fe9095a7..3c15bd00 100644 --- a/packages/mcp/src/pipefy_mcp/server.py +++ b/packages/mcp/src/pipefy_mcp/server.py @@ -8,6 +8,7 @@ from mcp.server.fastmcp import FastMCP from pipefy_mcp.core.runtime import McpRuntime +from pipefy_mcp.core.tool_middleware import install_tool_call_middleware from pipefy_mcp.settings import Settings from pipefy_mcp.tools.registry import ToolRegistry from pipefy_mcp.tools.validation_envelope import install_pipefy_validation_envelope @@ -101,6 +102,11 @@ def build_pipefy_mcp_server(settings: Settings) -> FastMCP: auth=auth, ) _register_pipefy_tools(app, remote_mode=settings.mcp.profile == "remote") + # Wrap the tool-call handler with the runtime's registered middleware chain. + # Both transports serve this app, so tool calls over stdio and HTTP alike run + # through the chain; the built-in logger is seeded per profile (see + # McpRuntime.for_profile) and this is a no-op when nothing is registered. + install_tool_call_middleware(app, runtime.tool_middlewares) return app diff --git a/packages/mcp/tests/core/test_runtime.py b/packages/mcp/tests/core/test_runtime.py index 045d9ac7..82b0b699 100644 --- a/packages/mcp/tests/core/test_runtime.py +++ b/packages/mcp/tests/core/test_runtime.py @@ -19,6 +19,7 @@ RequestScopedIdentity, StartupIdentity, ) +from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.settings import McpSettings, Settings @@ -165,6 +166,25 @@ def test_local_static_token_binds_the_static_bearer(self, clear_auth_env): assert runtime.inbound_auth is None assert _bearer_of(runtime.session_for_request(None)) == "Bearer env-bearer" + @pytest.mark.unit + def test_remote_seeds_the_tool_log_middleware(self): + """The hosted profile seeds structured tool-call logging on the chain.""" + runtime = McpRuntime.for_profile(remote_rs_settings()) + + assert runtime._tool_middlewares == [tool_log_middleware] + + @pytest.mark.unit + def test_local_seeds_no_tool_middleware(self, clear_auth_env): + """The local profile leaves the chain empty (logging is a hosted concern).""" + settings = Settings( + pipefy=PipefySettings(base_url="https://api.pipefy.com"), + auth=AuthSettings(static_token="env-bearer"), + ) + + runtime = McpRuntime.for_profile(settings) + + assert runtime._tool_middlewares == [] + @pytest.mark.unit @patch("pipefy_auth.resolver.load_session", lambda **_: None) def test_local_without_credential_fails_fast(self, clear_auth_env): diff --git a/packages/mcp/tests/core/test_tool_middleware.py b/packages/mcp/tests/core/test_tool_middleware.py new file mode 100644 index 00000000..3c7c8a2e --- /dev/null +++ b/packages/mcp/tests/core/test_tool_middleware.py @@ -0,0 +1,254 @@ +"""Unit tests for the tool-call middleware chain. + +Covers the seam mechanics against a real FastMCP app: composition order, +short-circuit, exception propagation, the short-circuit envelope shape, the +per-app idempotent install, and the argument-context build. +""" + +from __future__ import annotations + +import asyncio +import json +from contextlib import contextmanager + +import pytest +from mcp import types +from mcp.server.fastmcp import FastMCP +from mcp.server.lowlevel.server import request_ctx +from mcp.shared.context import RequestContext + +from pipefy_mcp.core.tool_middleware import ( + ToolCallContext, + compose, + context_from_request, + install_tool_call_middleware, + short_circuit_error, +) + + +def _app() -> FastMCP: + app = FastMCP("test") + + @app.tool() + async def echo(x: int) -> str: + return f"got {x}" + + return app + + +def _call_tool_request(**arguments: object) -> types.CallToolRequest: + return types.CallToolRequest( + method="tools/call", + params=types.CallToolRequestParams(name="echo", arguments=arguments), + ) + + +def _bare_context(**arguments: object) -> ToolCallContext: + """A minimal context for exercising the chain without a request scope.""" + return ToolCallContext( + tool_name="echo", + arguments=arguments, + argument_keys=tuple(sorted(arguments)), + identity=None, # type: ignore[arg-type] + request_id=None, + req=_call_tool_request(**arguments), + ) + + +async def _noop(ctx: ToolCallContext, call_next): + """A pass-through middleware, so an install actually wraps the handler.""" + return await call_next(ctx) + + +@contextmanager +def _message_scope(request_id: str = "req-1"): + """Enter a request scope for one JSON-RPC message (stdio-like: no HTTP request).""" + token = request_ctx.set( + RequestContext( + request_id=request_id, + meta=None, + session=None, # type: ignore[arg-type] + lifespan_context=None, + request=None, + ) + ) + try: + yield + finally: + request_ctx.reset(token) + + +async def _invoke(app: FastMCP, req: types.CallToolRequest) -> types.ServerResult: + """Run the app's (wrapped) CallToolRequest handler inside a request scope.""" + handler = app._mcp_server.request_handlers[types.CallToolRequest] + with _message_scope(): + return await handler(req) + + +@pytest.mark.unit +def test_middleware_run_outer_to_inner_in_registration_order(): + """``[A, B]`` runs A, then B, then the tool, and unwinds in reverse.""" + trace: list[str] = [] + + def recorder(label: str): + async def mw(ctx: ToolCallContext, call_next): + trace.append(f"{label}:before") + result = await call_next(ctx) + trace.append(f"{label}:after") + return result + + return mw + + app = _app() + install_tool_call_middleware(app, [recorder("A"), recorder("B")]) + + result = asyncio.run(_invoke(app, _call_tool_request(x=5))) + + assert result.root.isError is False + assert "got 5" in result.root.content[0].text + assert trace == ["A:before", "B:before", "B:after", "A:after"] + + +@pytest.mark.unit +def test_middleware_short_circuits_without_running_the_tool(): + """Returning without awaiting ``call_next`` skips inner middleware and the tool.""" + reached_inner = False + + async def deny(ctx: ToolCallContext, call_next): + return short_circuit_error("denied", code="DENIED") + + async def inner(ctx: ToolCallContext, call_next): + nonlocal reached_inner + reached_inner = True + return await call_next(ctx) + + app = _app() + install_tool_call_middleware(app, [deny, inner]) + + result = asyncio.run(_invoke(app, _call_tool_request(x=5))) + + assert result.root.isError is True + assert reached_inner is False + assert json.loads(result.root.content[0].text)["error"]["code"] == "DENIED" + + +@pytest.mark.unit +def test_chain_propagates_cancellation(): + """A ``CancelledError`` from the terminal propagates, not swallowed by the chain.""" + + async def observer(ctx: ToolCallContext, call_next): + return await call_next(ctx) + + async def boom(req: types.CallToolRequest) -> types.ServerResult: + raise asyncio.CancelledError + + chain = compose([observer], boom) + + async def run() -> None: + await chain(_bare_context()) + + with pytest.raises(asyncio.CancelledError): + asyncio.run(run()) + + +@pytest.mark.unit +def test_chain_propagates_arbitrary_exceptions(): + """``compose`` adds no try/except, so a terminal exception surfaces unchanged. + + Covers the ``Exception`` path (``CancelledError`` above covers ``BaseException``) + that framework signals like ``UrlElicitationRequiredError`` travel: the chain + must not intercept them. + """ + + class Boom(Exception): + pass + + async def observer(ctx: ToolCallContext, call_next): + return await call_next(ctx) + + async def boom(req: types.CallToolRequest) -> types.ServerResult: + raise Boom("propagate me") + + chain = compose([observer], boom) + + async def run() -> None: + await chain(_bare_context()) + + with pytest.raises(Boom, match="propagate me"): + asyncio.run(run()) + + +@pytest.mark.unit +def test_short_circuit_error_shape(): + """The envelope is the canonical tool_error dict in both content and structured.""" + result = short_circuit_error("quota exceeded", code="RATE_LIMITED") + root = result.root + + envelope = json.loads(root.content[0].text) + assert root.isError is True + assert root.structuredContent == envelope + assert envelope == { + "success": False, + "error": {"message": "quota exceeded", "code": "RATE_LIMITED"}, + } + + +@pytest.mark.unit +def test_install_with_no_middleware_leaves_the_handler_untouched(): + """An empty chain is a no-op: FastMCP's own handler stays in place.""" + app = _app() + original = app._mcp_server.request_handlers[types.CallToolRequest] + install_tool_call_middleware(app, []) + assert app._mcp_server.request_handlers[types.CallToolRequest] is original + + +@pytest.mark.unit +def test_install_is_idempotent_per_app(): + """A second install on the same app is a no-op (same wrapped handler).""" + app = _app() + install_tool_call_middleware(app, [_noop]) + first = app._mcp_server.request_handlers[types.CallToolRequest] + + install_tool_call_middleware(app, [_noop]) + second = app._mcp_server.request_handlers[types.CallToolRequest] + + assert first is second + + +@pytest.mark.unit +def test_install_wraps_each_app_independently(): + """A second app built in the same process wraps its own handler. + + Guards against a module- or class-global sentinel, which would wrap the first + app and silently skip every later one. + """ + app_one = _app() + app_two = _app() + install_tool_call_middleware(app_one, [_noop]) + install_tool_call_middleware(app_two, [_noop]) + + handler_one = app_one._mcp_server.request_handlers[types.CallToolRequest] + handler_two = app_two._mcp_server.request_handlers[types.CallToolRequest] + assert handler_one is not handler_two + + +@pytest.mark.unit +def test_install_raises_when_handler_absent(): + """A missing CallToolRequest handler fails loud (SDK contract changed).""" + app = _app() + del app._mcp_server.request_handlers[types.CallToolRequest] + with pytest.raises(RuntimeError, match="CallToolRequest handler missing"): + install_tool_call_middleware(app, [_noop]) + + +@pytest.mark.unit +def test_context_exposes_bounded_argument_keys_without_values(): + """The context carries sorted argument keys, never the values.""" + req = _call_tool_request(zeta=1, alpha="secret-value") + with _message_scope(request_id="m-1"): + ctx = context_from_request(req) + + assert ctx.tool_name == "echo" + assert ctx.argument_keys == ("alpha", "zeta") + # No HTTP request in scope -> falls back to the JSON-RPC message id. + assert ctx.request_id == "m-1" diff --git a/packages/mcp/tests/observability/__init__.py b/packages/mcp/tests/observability/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/packages/mcp/tests/observability/test_tool_log_middleware.py b/packages/mcp/tests/observability/test_tool_log_middleware.py new file mode 100644 index 00000000..d31a40ba --- /dev/null +++ b/packages/mcp/tests/observability/test_tool_log_middleware.py @@ -0,0 +1,130 @@ +"""Unit tests for the structured tool-call logging middleware. + +Pins the log shape and the two protocol-safety invariants: the outcome derives +from the call result, and output goes to logging (stderr), never stdout, so it +cannot corrupt the stdio JSON-RPC stream. +""" + +from __future__ import annotations + +import asyncio +import json +import logging +import sys + +import pytest +from mcp import types + +from pipefy_mcp.auth.request_identity import CallerIdentity +from pipefy_mcp.core.tool_middleware import ToolCallContext +from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware + + +def _context(**arguments: object) -> ToolCallContext: + req = types.CallToolRequest( + method="tools/call", + params=types.CallToolRequestParams(name="get_card", arguments=arguments), + ) + return ToolCallContext( + tool_name="get_card", + arguments=arguments, + argument_keys=tuple(sorted(arguments)), + identity=CallerIdentity(client_id="acting-client", scopes=("read",)), + request_id="req-42", + req=req, + ) + + +def _ok_result() -> types.ServerResult: + return types.ServerResult( + types.CallToolResult(content=[types.TextContent(type="text", text="ok")]) + ) + + +def _log_line(caplog) -> dict: + records = [ + r for r in caplog.records if r.name == "pipefy_mcp.observability.tool_call" + ] + assert len(records) == 1 + return json.loads(records[0].getMessage()) + + +@pytest.mark.unit +def test_logs_one_line_with_the_documented_fields(caplog): + async def terminal(ctx): + return _ok_result() + + with caplog.at_level(logging.INFO, logger="pipefy_mcp.observability.tool_call"): + asyncio.run(tool_log_middleware(_context(card_id="1"), terminal)) + + event = _log_line(caplog) + assert event["tool"] == "get_card" + assert event["outcome"] == "ok" + assert event["arg_keys"] == ["card_id"] + assert event["client_id"] == "acting-client" + assert event["request_id"] == "req-42" + assert isinstance(event["duration_ms"], (int, float)) + + +@pytest.mark.unit +def test_never_logs_argument_values_or_a_bearer(caplog): + async def terminal(ctx): + return _ok_result() + + with caplog.at_level(logging.INFO, logger="pipefy_mcp.observability.tool_call"): + asyncio.run( + tool_log_middleware(_context(token="super-secret-bearer"), terminal) + ) + + raw = caplog.records[-1].getMessage() + assert "super-secret-bearer" not in raw + assert "token" in json.loads(raw)["arg_keys"] # the key is logged, not the value + + +@pytest.mark.unit +def test_error_result_logs_outcome_error(caplog): + async def terminal(ctx): + return types.ServerResult( + types.CallToolResult( + content=[types.TextContent(type="text", text="boom")], isError=True + ) + ) + + with caplog.at_level(logging.INFO, logger="pipefy_mcp.observability.tool_call"): + asyncio.run(tool_log_middleware(_context(card_id="1"), terminal)) + + assert _log_line(caplog)["outcome"] == "error" + + +@pytest.mark.unit +def test_propagated_exception_logs_error_and_re_raises(caplog): + async def terminal(ctx): + raise asyncio.CancelledError + + with caplog.at_level(logging.INFO, logger="pipefy_mcp.observability.tool_call"): + with pytest.raises(asyncio.CancelledError): + asyncio.run(tool_log_middleware(_context(card_id="1"), terminal)) + + assert _log_line(caplog)["outcome"] == "error" + + +@pytest.mark.unit +def test_writes_nothing_to_stdout(capsys): + """The logger must never touch stdout: it is the stdio transport's JSON-RPC stream.""" + + async def terminal(ctx): + return _ok_result() + + # Route logging to stderr explicitly, then assert stdout stays empty. + handler = logging.StreamHandler(sys.stderr) + log = logging.getLogger("pipefy_mcp.observability.tool_call") + log.addHandler(handler) + log.setLevel(logging.INFO) + try: + asyncio.run(tool_log_middleware(_context(card_id="1"), terminal)) + finally: + log.removeHandler(handler) + + captured = capsys.readouterr() + assert captured.out == "" + assert '"event":"tool_call"' in captured.err From daca78847d637e668c47c0b7b3562d5d5b01806f Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 15:54:53 -0300 Subject: [PATCH 03/11] docs(mcp): clarify subject deferral rationale in CallerIdentity The local static token is a JWT carrying the user subject, so subject is feasible on both profiles; it is deferred for want of a consumer, not because it cannot be sourced. --- packages/mcp/src/pipefy_mcp/auth/request_identity.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/mcp/src/pipefy_mcp/auth/request_identity.py b/packages/mcp/src/pipefy_mcp/auth/request_identity.py index 3ae6e203..e21f92d3 100644 --- a/packages/mcp/src/pipefy_mcp/auth/request_identity.py +++ b/packages/mcp/src/pipefy_mcp/auth/request_identity.py @@ -34,9 +34,10 @@ class CallerIdentity: stdio/local profile there is no inbound bearer, so middleware sees an anonymous identity rather than a failure. - The end-user subject is intentionally absent: exposing it needs an - ``AccessToken`` subtype and its only consumer is per-user quotas, which are - not built yet. Add ``subject`` here when that lands. + The end-user subject is intentionally deferred: the consumer that keys on it + (per-user quotas) is not built yet. Both profiles could supply it when it + lands: the remote profile from the validated ``sub`` claim, the local profile + by decoding the configured JWT credential once at startup. """ client_id: str | None = None From c5208e392575833d2c2e1bee615c7f08576e6442 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 16:16:09 -0300 Subject: [PATCH 04/11] docs(mcp): stop framing the tool-call seam as hosted-only The middleware chain is profile-agnostic; only per-user concerns (quotas, cost attribution) are hosted-specific, while observability and downstream protection apply to any deployment. Reframe the module docstring, the logger-seeding comment, and AGENTS.md so the remote-only logger default reads as a default, not a capability boundary. --- packages/mcp/AGENTS.md | 5 ++++- packages/mcp/src/pipefy_mcp/core/runtime.py | 10 ++++++---- .../mcp/src/pipefy_mcp/core/tool_middleware.py | 16 +++++++++------- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/packages/mcp/AGENTS.md b/packages/mcp/AGENTS.md index 7c3fb597..84e3b24f 100644 --- a/packages/mcp/AGENTS.md +++ b/packages/mcp/AGENTS.md @@ -214,7 +214,10 @@ runtime.register_tool_middleware(quota) # before install_tool_call_middleware( The chain installs on every profile (a pass-through when nothing is registered); the built-in structured logger (`observability/tool_log_middleware.py`) is seeded -only under the `remote` profile. That logger emits through the `logging` module +by default only under the `remote` profile. That is a default, not a capability +boundary: per-call concerns like observability and downstream protection apply to +any deployment (only per-user concerns are hosted-specific), so a local deployment +can register its own middleware. The logger emits through the `logging` module (stderr), never stdout, which is the stdio transport's JSON-RPC stream. The wrap targets a FastMCP internal and is tested against `mcp==1.25.0`; the diff --git a/packages/mcp/src/pipefy_mcp/core/runtime.py b/packages/mcp/src/pipefy_mcp/core/runtime.py index 366c0405..c88fc673 100644 --- a/packages/mcp/src/pipefy_mcp/core/runtime.py +++ b/packages/mcp/src/pipefy_mcp/core/runtime.py @@ -168,10 +168,12 @@ def for_profile(cls, settings: Settings) -> McpRuntime: "per-request bearer and acts on behalf of the caller." ) runtime = cls(settings, RequestScopedIdentity(), inbound_auth=inbound_auth) - # Structured tool-call logging is a hosted-operations concern; seed it - # only under the remote profile. The chain itself is installed on every - # profile (see install_tool_call_middleware), but the built-in logger - # stays off the stdio/local path. + # Default the built-in tool-call logger on under the hosted profile, + # where structured logs are how operators attribute activity. This is a + # default, not a capability boundary: the chain installs on every profile + # (see install_tool_call_middleware), so a local deployment can register + # its own middleware, and tool logging could later become a config toggle + # a local user opts into. runtime.register_tool_middleware(tool_log_middleware) return runtime return cls(settings, StartupIdentity.from_configured_credential(settings)) diff --git a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py index 12ef9ecd..5e39fe81 100644 --- a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py +++ b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py @@ -1,12 +1,14 @@ """Ordered middleware chain around MCP tool invocation. -The hosted profile needs to run cross-cutting logic around each tool call with -the caller's validated identity in hand: logging, per-user quotas, rate limiting, -cost weighting, downstream 429/circuit-breaking. The MCP SDK offers no seam for -this: FastMCP dispatches every tool call through a single ``CallToolRequest`` -entry in the low-level server's ``request_handlers`` dict, and the only way to -observe or govern a call is to overwrite that one private slot, so the next -feature that needs it clobbers the previous. +The server needs a seam to run cross-cutting logic around each tool call. Some of +it spans every deployment (observability, and downstream protection such as +honoring the API's 429s or circuit-breaking); some is specific to the multi-tenant +hosted profile (per-user quotas, rate limiting, and cost attribution keyed on the +validated caller). The MCP SDK offers no such seam: FastMCP dispatches every tool +call through a single ``CallToolRequest`` entry in the low-level server's +``request_handlers`` dict, and the only way to observe or govern a call is to +overwrite that one private slot, so the next feature that needs it clobbers the +previous. This module turns that single slot into an ordered chain. Middleware register on :class:`~pipefy_mcp.core.runtime.McpRuntime` (the public seam) and this module From c8302bd71acb35b3614ba379c701b8ff9426286f Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 16:36:30 -0300 Subject: [PATCH 05/11] docs(mcp): tidy tool-call middleware comments Drop editorial framing and restated rationale from the seam's docstrings, fix a number-agreement slip, and correct require_request_bearer to say the read goes through request.scope rather than the request.user property the refactor stopped using. --- .../mcp/src/pipefy_mcp/auth/request_identity.py | 16 ++++++++-------- .../mcp/src/pipefy_mcp/core/tool_middleware.py | 4 ++-- .../mcp/src/pipefy_mcp/observability/__init__.py | 2 +- .../observability/tool_log_middleware.py | 4 ++-- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/mcp/src/pipefy_mcp/auth/request_identity.py b/packages/mcp/src/pipefy_mcp/auth/request_identity.py index e21f92d3..25b91868 100644 --- a/packages/mcp/src/pipefy_mcp/auth/request_identity.py +++ b/packages/mcp/src/pipefy_mcp/auth/request_identity.py @@ -34,10 +34,10 @@ class CallerIdentity: stdio/local profile there is no inbound bearer, so middleware sees an anonymous identity rather than a failure. - The end-user subject is intentionally deferred: the consumer that keys on it - (per-user quotas) is not built yet. Both profiles could supply it when it - lands: the remote profile from the validated ``sub`` claim, the local profile - by decoding the configured JWT credential once at startup. + The end-user subject is intentionally deferred: its consumer (per-user + quotas) is not built yet. Both profiles can source it when it lands, the + remote profile from the validated ``sub`` claim and the local profile from + the configured JWT credential. """ client_id: str | None = None @@ -51,8 +51,8 @@ def _authenticated_user(request: Request | None) -> AuthenticatedUser | None: asserts ``AuthenticationMiddleware`` is installed, which holds only under the ``remote`` profile, so touching the property would raise on a ``local`` HTTP request. This is the one place both request-scoped readers agree on how the - validated caller is located (per-message request, never ``auth_context_var``, - which stateful Streamable HTTP freezes at the session's first bearer). + validated caller is located: off the per-message request, never + ``auth_context_var``. """ user = request.scope.get("user") if request is not None else None return user if isinstance(user, AuthenticatedUser) else None @@ -75,8 +75,8 @@ def caller_identity(request: Request | None) -> CallerIdentity: def require_request_bearer(request: Request | None) -> str: """Return the validated bearer token off the in-flight request. - The resource-server middleware sets ``request.user`` to the - ``AuthenticatedUser`` it validated; this reads its access token. Taking the + The resource-server middleware sets the validated ``AuthenticatedUser`` on + the request; this reads its access token. Taking the request as an argument (the runtime passes ``ctx.request_context.request`` from the tool handler) keeps the read on the current caller: it never touches ``auth_context_var``, which stateful Streamable HTTP freezes at the session's diff --git a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py index 5e39fe81..a264a202 100644 --- a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py +++ b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py @@ -68,8 +68,8 @@ class ToolCallContext: ``arguments`` is the raw JSON-RPC argument map: FastMCP registers the terminal with ``validate_input=False`` and does its own coercion/defaulting downstream, - so middleware observes the un-coerced, client-sent arguments (defaults not - applied, types not coerced). ``argument_keys`` is the bounded, values-free view + so middleware observes the un-coerced, client-sent arguments. ``argument_keys`` + is the bounded, values-free view a privacy-sensitive consumer (logging) should prefer; ``arguments`` values are passed unbounded to any consumer that opts to read them. diff --git a/packages/mcp/src/pipefy_mcp/observability/__init__.py b/packages/mcp/src/pipefy_mcp/observability/__init__.py index 54208fd4..1d9106fd 100644 --- a/packages/mcp/src/pipefy_mcp/observability/__init__.py +++ b/packages/mcp/src/pipefy_mcp/observability/__init__.py @@ -1,4 +1,4 @@ -"""Hosted-profile observability: middleware that ride the tool-call chain.""" +"""Hosted-profile observability: middleware for the tool-call chain.""" from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware diff --git a/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py index 0ea61fa8..309ebe1e 100644 --- a/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py +++ b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py @@ -3,8 +3,8 @@ The first consumer of the tool-call middleware chain (see :mod:`pipefy_mcp.core.tool_middleware`): it emits one JSON line per call with the tool name, outcome, duration, the caller's client id, argument key names, and a -request id for correlation. It proves a real cross-cutting concern rides the seam -without touching FastMCP internals. +request id for correlation. Being the first consumer, it also exercises the +chain end to end. Privacy: never logs the bearer, and never logs argument values, only their (bounded) key names. From 51b29746b471c570615c1c3a12e1737797ee62b1 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 17:26:51 -0300 Subject: [PATCH 06/11] fix(mcp): address tool-call middleware review findings Behavioral fixes: - log-middleware outcome is now ok/error/cancelled/elicitation, so client disconnects and URL-elicitation control flow no longer inflate error metrics. - a repeat install with a different middleware set raises instead of silently dropping the newcomers; the marker now snapshots the installed set. - harden the request_id read so a present non-dict scope["state"] cannot crash the chain entry. - require_request_bearer's error names both causes (no valid bearer, or auth middleware not wired), recovering the signal the old request.user assertion gave. Accompanying tidy-ups in the same files: - ToolCallContext.tool_name/arguments are read-only views over req.params, so the context cannot drift from the request it wraps. - guard the log emit at the call site so the event dict is not built when INFO is off; drop an over-defensive getattr in _outcome; drop a redundant list() in compose. - reword an as-is doc line in AGENTS.md that described the seam as a delta. --- packages/mcp/AGENTS.md | 4 +- .../src/pipefy_mcp/auth/request_identity.py | 6 +- .../src/pipefy_mcp/core/tool_middleware.py | 71 +++++++++++------- .../observability/tool_log_middleware.py | 74 ++++++++++--------- .../mcp/tests/core/test_tool_middleware.py | 15 +++- .../observability/test_tool_log_middleware.py | 39 +++++++++- 6 files changed, 141 insertions(+), 68 deletions(-) diff --git a/packages/mcp/AGENTS.md b/packages/mcp/AGENTS.md index 84e3b24f..d74f9746 100644 --- a/packages/mcp/AGENTS.md +++ b/packages/mcp/AGENTS.md @@ -177,8 +177,8 @@ rate limiting, cost weighting, downstream 429/circuit-breaking) register as ordered middleware, not by overwriting the server's internal handler. The MCP SDK dispatches every tool call through one `request_handlers[CallToolRequest]` slot; `core/tool_middleware.py` wraps that slot once, at build time, and composes the -registered middleware around it. The private slot is no longer the extension -surface. +registered middleware around it. The middleware chain is the extension surface; +the private slot is wrapped, not written to directly. Register through the runtime, never by touching FastMCP internals: diff --git a/packages/mcp/src/pipefy_mcp/auth/request_identity.py b/packages/mcp/src/pipefy_mcp/auth/request_identity.py index 25b91868..5570ac58 100644 --- a/packages/mcp/src/pipefy_mcp/auth/request_identity.py +++ b/packages/mcp/src/pipefy_mcp/auth/request_identity.py @@ -89,7 +89,9 @@ def require_request_bearer(request: Request | None) -> str: user = _authenticated_user(request) if user is None or not user.access_token.token: raise RuntimeError( - "No authenticated access token on the request; the resource-server " - "profile must validate a bearer before a tool runs." + "No authenticated access token on the request. Under the " + "resource-server profile this means either the caller sent no valid " + "bearer or the authentication middleware is not wired into the ASGI " + "stack; a tool must not run without a validated caller." ) return user.access_token.token diff --git a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py index a264a202..3a70f4eb 100644 --- a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py +++ b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py @@ -55,10 +55,12 @@ _MAX_ARG_KEYS = 64 _MAX_ARG_KEY_LENGTH = 200 -# Marks the installed chain handler so a second install on the same app is a -# no-op. The sentinel lives on the wrapped function (per app), not on a module or -# class global: each FastMCP instance has its own handler, so a global would wrap -# the first app and silently skip every later one built in the same process. +# Holds the middleware set the installed chain was built from, so a second install +# with the same set is a no-op and one with a different set fails loud rather than +# silently dropping the newcomers. The marker lives on the wrapped function (per +# app), not on a module or class global: each FastMCP instance has its own handler, +# so a global would wrap the first app and silently skip every later one built in +# the same process. _INSTALLED_MARKER = "__pipefy_tool_call_chain__" @@ -66,12 +68,14 @@ class ToolCallContext: """What a tool-call middleware sees for one invocation. - ``arguments`` is the raw JSON-RPC argument map: FastMCP registers the terminal - with ``validate_input=False`` and does its own coercion/defaulting downstream, - so middleware observes the un-coerced, client-sent arguments. ``argument_keys`` - is the bounded, values-free view - a privacy-sensitive consumer (logging) should prefer; ``arguments`` values are - passed unbounded to any consumer that opts to read them. + ``tool_name`` and ``arguments`` are read-only views over ``req.params``, so the + context cannot drift from the request it wraps. ``arguments`` is the raw + JSON-RPC argument map: FastMCP registers the terminal with + ``validate_input=False`` and does its own coercion/defaulting downstream, so + middleware observes the un-coerced, client-sent arguments. ``argument_keys`` is + the bounded, values-free view a privacy-sensitive consumer (logging) should + prefer; ``arguments`` values are passed unbounded to any consumer that opts to + read them. ``request_id`` correlates a call to its HTTP request when one is available; otherwise it falls back to the JSON-RPC message id, which is client-chosen and @@ -79,13 +83,19 @@ class ToolCallContext: untouched request the terminal handler is called with. """ - tool_name: str - arguments: dict[str, Any] | None argument_keys: tuple[str, ...] identity: CallerIdentity request_id: str | None req: types.CallToolRequest + @property + def tool_name(self) -> str: + return self.req.params.name + + @property + def arguments(self) -> dict[str, Any] | None: + return self.req.params.arguments + # A middleware calls ``call_next(ctx)`` to run the inner chain (ending at the # tool), or returns its own ServerResult to short-circuit without running it. @@ -128,7 +138,11 @@ def _request_scope() -> tuple[Any | None, str | None]: request = ctx.request request_id: Any = None if request is not None: - request_id = request.scope.get("state", {}).get("request_id") + # scope["state"] is a dict under Starlette, but guard the type: the default + # only covers an absent key, so a present non-dict would crash the read. + state = request.scope.get("state") + if isinstance(state, dict): + request_id = state.get("request_id") if request_id is None: request_id = ctx.request_id return request, (str(request_id) if request_id is not None else None) @@ -137,11 +151,8 @@ def _request_scope() -> tuple[Any | None, str | None]: def context_from_request(req: types.CallToolRequest) -> ToolCallContext: """Build the :class:`ToolCallContext` for the in-flight call.""" request, request_id = _request_scope() - arguments = req.params.arguments return ToolCallContext( - tool_name=req.params.name, - arguments=arguments, - argument_keys=_argument_keys(arguments), + argument_keys=_argument_keys(req.params.arguments), identity=caller_identity(request), request_id=request_id, req=req, @@ -162,7 +173,7 @@ async def call_terminal(ctx: ToolCallContext) -> types.ServerResult: return await terminal(ctx.req) call_next: CallNext = call_terminal - for middleware in reversed(list(middlewares)): + for middleware in reversed(middlewares): call_next = _bind(middleware, call_next) return call_next @@ -217,11 +228,14 @@ def install_tool_call_middleware( the default (no middleware) path pays nothing per call rather than routing every tool call through a pass-through wrapper that builds and discards a context. - Idempotent per app: the sentinel lives on the installed handler, so a repeat - install on the same app is a no-op while a different app (built later in the - same process) wraps its own fresh handler. Raises if FastMCP has not registered - the handler, so an SDK-internal change fails loud rather than silently skipping - the chain. + Idempotent per app: the marker lives on the installed handler, so a repeat + install on the same app with the same middleware set is a no-op while a + different app (built later in the same process) wraps its own fresh handler. A + repeat install with a *different* set raises rather than silently drop the + newcomers, because the marker snapshots the set at install time (the supported + pattern is to register everything, then install once). Raises too if FastMCP + has not registered the handler, so an SDK-internal change fails loud rather than + silently skipping the chain. """ if not middlewares: return @@ -234,7 +248,14 @@ def install_tool_call_middleware( ) terminal = handlers[types.CallToolRequest] - if getattr(terminal, _INSTALLED_MARKER, False): + installed = getattr(terminal, _INSTALLED_MARKER, None) + if installed is not None: + if tuple(middlewares) != installed: + raise RuntimeError( + "a tool-call chain is already installed on this app with a " + "different middleware set; register all middleware before calling " + "install_tool_call_middleware once" + ) return chain = compose(middlewares, terminal) @@ -242,5 +263,5 @@ def install_tool_call_middleware( async def chained(req: types.CallToolRequest) -> types.ServerResult: return await chain(context_from_request(req)) - setattr(chained, _INSTALLED_MARKER, True) + setattr(chained, _INSTALLED_MARKER, tuple(middlewares)) handlers[types.CallToolRequest] = chained diff --git a/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py index 309ebe1e..bfd70105 100644 --- a/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py +++ b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py @@ -16,40 +16,38 @@ from __future__ import annotations +import asyncio import json import logging import time from typing import Literal -from mcp import types +from mcp import UrlElicitationRequiredError, types from pipefy_mcp.core.tool_middleware import CallNext, ToolCallContext logger = logging.getLogger("pipefy_mcp.observability.tool_call") -ToolCallOutcome = Literal["ok", "error"] +# "cancelled" (client disconnect) and "elicitation" (the tool is asking the client +# to visit a URL) are normal control flow, kept out of "error" so an error-rate +# alert is not tripped by them. A governance short-circuit still reads as "error": +# it returns isError=True and is indistinguishable from a tool-reported failure at +# the result boundary. +ToolCallOutcome = Literal["ok", "error", "cancelled", "elicitation"] def _outcome(result: types.ServerResult) -> ToolCallOutcome: """Map a terminal result to an outcome. - Reads ``isError`` off the result's ``CallToolResult`` root. The attribute is - read defensively: an experimental ``CreateTaskResult`` root has no ``isError``, - which should read as ``ok`` rather than raise. + Reads ``isError`` off the result's root defensively: an experimental + ``CreateTaskResult`` root has no ``isError`` and reads as ``ok`` rather than + raising. """ - root = getattr(result, "root", None) - return "error" if getattr(root, "isError", False) else "ok" + return "error" if getattr(result.root, "isError", False) else "ok" def _emit(event: dict[str, object]) -> None: - """Write one structured line via logging (stderr), never stdout. - - Serialization runs before ``logger.info``, so it would otherwise be paid even - when the line is discarded; the ``isEnabledFor`` guard skips it when the level - is off. - """ - if not logger.isEnabledFor(logging.INFO): - return + """Write one structured line via logging (stderr), never stdout.""" logger.info(json.dumps(event, separators=(",", ":"))) @@ -60,12 +58,13 @@ async def tool_log_middleware( A tool body error surfaces as a result with ``isError=True`` (FastMCP's terminal turns tool exceptions into an error result), so the common path logs - from the returned result. The exceptions that DO propagate through the chain, - ``CancelledError`` (client disconnect / ``notifications/cancelled``) and - ``UrlElicitationRequiredError``, are logged as errors and re-raised: swallowing - them would break cancellation and elicitation. ``BaseException`` (not - ``Exception``) is caught so a ``CancelledError`` cannot skip the handler and let - ``finally`` emit a stale ``ok``. + from the returned result. The two exceptions that DO propagate through the + chain each get their own non-error outcome and are re-raised: ``CancelledError`` + (client disconnect / ``notifications/cancelled``) and + ``UrlElicitationRequiredError`` (the client must visit a URL to continue). + Swallowing either would break cancellation and elicitation. ``BaseException`` + (not ``Exception``) is caught so a ``CancelledError`` cannot skip the handler + and let ``finally`` emit a stale ``ok``. """ started_at = time.perf_counter() outcome: ToolCallOutcome = "ok" @@ -73,19 +72,28 @@ async def tool_log_middleware( result = await call_next(ctx) outcome = _outcome(result) return result + except asyncio.CancelledError: + outcome = "cancelled" + raise + except UrlElicitationRequiredError: + outcome = "elicitation" + raise except BaseException: outcome = "error" raise finally: - duration_ms = round((time.perf_counter() - started_at) * 1000, 3) - _emit( - { - "event": "tool_call", - "tool": ctx.tool_name, - "outcome": outcome, - "duration_ms": duration_ms, - "arg_keys": list(ctx.argument_keys), - "client_id": ctx.identity.client_id, - "request_id": ctx.request_id, - } - ) + # Guard here, not inside _emit, so the event dict is not built when the + # line would be discarded. + if logger.isEnabledFor(logging.INFO): + duration_ms = round((time.perf_counter() - started_at) * 1000, 3) + _emit( + { + "event": "tool_call", + "tool": ctx.tool_name, + "outcome": outcome, + "duration_ms": duration_ms, + "arg_keys": list(ctx.argument_keys), + "client_id": ctx.identity.client_id, + "request_id": ctx.request_id, + } + ) diff --git a/packages/mcp/tests/core/test_tool_middleware.py b/packages/mcp/tests/core/test_tool_middleware.py index 3c7c8a2e..75a950e1 100644 --- a/packages/mcp/tests/core/test_tool_middleware.py +++ b/packages/mcp/tests/core/test_tool_middleware.py @@ -46,8 +46,6 @@ def _call_tool_request(**arguments: object) -> types.CallToolRequest: def _bare_context(**arguments: object) -> ToolCallContext: """A minimal context for exercising the chain without a request scope.""" return ToolCallContext( - tool_name="echo", - arguments=arguments, argument_keys=tuple(sorted(arguments)), identity=None, # type: ignore[arg-type] request_id=None, @@ -215,6 +213,19 @@ def test_install_is_idempotent_per_app(): assert first is second +@pytest.mark.unit +def test_reinstall_with_a_different_middleware_set_raises(): + """A second install with a changed set fails loud instead of dropping it.""" + + async def other(ctx: ToolCallContext, call_next): + return await call_next(ctx) + + app = _app() + install_tool_call_middleware(app, [_noop]) + with pytest.raises(RuntimeError, match="different middleware set"): + install_tool_call_middleware(app, [_noop, other]) + + @pytest.mark.unit def test_install_wraps_each_app_independently(): """A second app built in the same process wraps its own handler. diff --git a/packages/mcp/tests/observability/test_tool_log_middleware.py b/packages/mcp/tests/observability/test_tool_log_middleware.py index d31a40ba..a340d486 100644 --- a/packages/mcp/tests/observability/test_tool_log_middleware.py +++ b/packages/mcp/tests/observability/test_tool_log_middleware.py @@ -13,7 +13,7 @@ import sys import pytest -from mcp import types +from mcp import UrlElicitationRequiredError, types from pipefy_mcp.auth.request_identity import CallerIdentity from pipefy_mcp.core.tool_middleware import ToolCallContext @@ -26,8 +26,6 @@ def _context(**arguments: object) -> ToolCallContext: params=types.CallToolRequestParams(name="get_card", arguments=arguments), ) return ToolCallContext( - tool_name="get_card", - arguments=arguments, argument_keys=tuple(sorted(arguments)), identity=CallerIdentity(client_id="acting-client", scopes=("read",)), request_id="req-42", @@ -97,7 +95,9 @@ async def terminal(ctx): @pytest.mark.unit -def test_propagated_exception_logs_error_and_re_raises(caplog): +def test_cancellation_logs_outcome_cancelled_and_re_raises(caplog): + """A client disconnect is control flow, not an error, and must propagate.""" + async def terminal(ctx): raise asyncio.CancelledError @@ -105,6 +105,37 @@ async def terminal(ctx): with pytest.raises(asyncio.CancelledError): asyncio.run(tool_log_middleware(_context(card_id="1"), terminal)) + assert _log_line(caplog)["outcome"] == "cancelled" + + +@pytest.mark.unit +def test_elicitation_logs_outcome_elicitation_and_re_raises(caplog): + """A URL-elicitation signal is a continuation, not an error, and must propagate.""" + + async def terminal(ctx): + raise UrlElicitationRequiredError([]) + + with caplog.at_level(logging.INFO, logger="pipefy_mcp.observability.tool_call"): + with pytest.raises(UrlElicitationRequiredError): + asyncio.run(tool_log_middleware(_context(card_id="1"), terminal)) + + assert _log_line(caplog)["outcome"] == "elicitation" + + +@pytest.mark.unit +def test_other_propagated_exception_logs_outcome_error_and_re_raises(caplog): + """Any other propagating exception is logged as an error and re-raised.""" + + class Boom(Exception): + pass + + async def terminal(ctx): + raise Boom("propagate me") + + with caplog.at_level(logging.INFO, logger="pipefy_mcp.observability.tool_call"): + with pytest.raises(Boom, match="propagate me"): + asyncio.run(tool_log_middleware(_context(card_id="1"), terminal)) + assert _log_line(caplog)["outcome"] == "error" From 7d34d06994dccc4b8e13b844ce7dd3b7a625a8d1 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 17:30:36 -0300 Subject: [PATCH 07/11] refactor(mcp): own the tool-call middleware list at the composition root The runtime seeded a profile-specific logger into a registration list that server.py then read back out and installed, which coupled core to a concrete observability middleware and made the list a pointless round-trip. Move the seeding decision to default_tool_middlewares(settings) in server.py and pass the list straight to install_tool_call_middleware. Drop the runtime's _tool_middlewares list, register_tool_middleware, and tool_middlewares property: the composition root builds the list and installs it, so the runtime owns only the engine, identity, and inbound auth again. The core -> observability import is gone; the dependency now runs one way. --- packages/mcp/AGENTS.md | 15 +++++---- packages/mcp/src/pipefy_mcp/core/runtime.py | 28 +---------------- .../src/pipefy_mcp/core/tool_middleware.py | 14 ++++----- packages/mcp/src/pipefy_mcp/server.py | 31 +++++++++++++++---- packages/mcp/tests/core/test_runtime.py | 20 ------------ packages/mcp/tests/test_server.py | 14 +++++++++ 6 files changed, 56 insertions(+), 66 deletions(-) diff --git a/packages/mcp/AGENTS.md b/packages/mcp/AGENTS.md index d74f9746..7dc5573c 100644 --- a/packages/mcp/AGENTS.md +++ b/packages/mcp/AGENTS.md @@ -180,7 +180,9 @@ dispatches every tool call through one `request_handlers[CallToolRequest]` slot; registered middleware around it. The middleware chain is the extension surface; the private slot is wrapped, not written to directly. -Register through the runtime, never by touching FastMCP internals: +A middleware is a plain async callable. Add it to the chain at the composition +root (`default_tool_middlewares` in `server.py`), never by touching FastMCP +internals: ```python from pipefy_mcp.core.tool_middleware import ToolCallContext, CallNext, short_circuit_error @@ -190,11 +192,12 @@ async def quota(ctx: ToolCallContext, call_next: CallNext): return short_circuit_error("quota exceeded", code="RATE_LIMITED") return await call_next(ctx) -runtime.register_tool_middleware(quota) # before install_tool_call_middleware(app) +# server.py builds the list per profile and hands it to install: +# install_tool_call_middleware(app, default_tool_middlewares(settings)) ``` -- **Order**: registration order runs outer to inner around the tool. `[A, B]` - runs A, then B, then the tool, and unwinds in reverse. +- **Order**: list order runs outer to inner around the tool. `[A, B]` runs A, + then B, then the tool, and unwinds in reverse. - **Short-circuit**: a middleware that returns without awaiting `call_next` skips the inner chain and the tool. Use `short_circuit_error`, which carries the canonical `tool_error` envelope but sets `isError=True` deliberately: a @@ -212,8 +215,8 @@ runtime.register_tool_middleware(quota) # before install_tool_call_middleware( for privacy-sensitive consumers; `ctx.arguments` values are passed unbounded to any consumer that opts to read them. Never log a bearer or argument values. -The chain installs on every profile (a pass-through when nothing is registered); -the built-in structured logger (`observability/tool_log_middleware.py`) is seeded +The chain installs on every profile (a no-op when the list is empty); the +built-in structured logger (`observability/tool_log_middleware.py`) is seeded by default only under the `remote` profile. That is a default, not a capability boundary: per-call concerns like observability and downstream protection apply to any deployment (only per-user concerns are hosted-specific), so a local deployment diff --git a/packages/mcp/src/pipefy_mcp/core/runtime.py b/packages/mcp/src/pipefy_mcp/core/runtime.py index c88fc673..bdb9d193 100644 --- a/packages/mcp/src/pipefy_mcp/core/runtime.py +++ b/packages/mcp/src/pipefy_mcp/core/runtime.py @@ -19,8 +19,6 @@ ResourceServerAuth, build_resource_server_auth, ) -from pipefy_mcp.core.tool_middleware import ToolCallMiddleware -from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.settings import Settings @@ -135,7 +133,6 @@ def __init__( self._identity = identity self.inbound_auth = inbound_auth self._engine = PipefyEngine.build(settings.pipefy, surface="mcp") - self._tool_middlewares: list[ToolCallMiddleware] = [] @classmethod def for_profile(cls, settings: Settings) -> McpRuntime: @@ -167,15 +164,7 @@ def for_profile(cls, settings: Settings) -> McpRuntime: "PIPEFY_MCP_RS_RESOURCE_SERVER_URL so the server validates a " "per-request bearer and acts on behalf of the caller." ) - runtime = cls(settings, RequestScopedIdentity(), inbound_auth=inbound_auth) - # Default the built-in tool-call logger on under the hosted profile, - # where structured logs are how operators attribute activity. This is a - # default, not a capability boundary: the chain installs on every profile - # (see install_tool_call_middleware), so a local deployment can register - # its own middleware, and tool logging could later become a config toggle - # a local user opts into. - runtime.register_tool_middleware(tool_log_middleware) - return runtime + return cls(settings, RequestScopedIdentity(), inbound_auth=inbound_auth) return cls(settings, StartupIdentity.from_configured_credential(settings)) def session_for_request(self, request: Request | None) -> PipefyClient: @@ -189,21 +178,6 @@ def session_for_request(self, request: Request | None) -> PipefyClient: """ return self._engine.session(self._identity.resolve(request)) - def register_tool_middleware(self, middleware: ToolCallMiddleware) -> None: - """Register a tool-call middleware, run outer-to-inner in registration order. - - The public seam for cross-cutting concerns (logging, quotas, rate limiting): - callers register here and never touch the server's internal request handlers. - The wiring layer reads :attr:`tool_middlewares` and installs the chain onto - the app at build time. - """ - self._tool_middlewares.append(middleware) - - @property - def tool_middlewares(self) -> tuple[ToolCallMiddleware, ...]: - """The registered tool-call middleware, in registration order.""" - return tuple(self._tool_middlewares) - @property def settings(self) -> Settings: """The resolved settings this runtime was built from.""" diff --git a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py index 3a70f4eb..adb98d79 100644 --- a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py +++ b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py @@ -10,12 +10,12 @@ overwrite that one private slot, so the next feature that needs it clobbers the previous. -This module turns that single slot into an ordered chain. Middleware register on -:class:`~pipefy_mcp.core.runtime.McpRuntime` (the public seam) and this module -wraps FastMCP's handler exactly once, at build time, composing the registered -middleware around it. Middleware run outer-to-inner in registration order, may -short-circuit (return an error result without invoking the tool), and read the -validated caller from the per-message request context. +This module turns that single slot into an ordered chain. The composition root +builds the middleware list per profile and passes it to +:func:`install_tool_call_middleware`, which wraps FastMCP's handler exactly once, +at build time, composing the middleware around it. Middleware run outer-to-inner +in list order, may short-circuit (return an error result without invoking the +tool), and read the validated caller from the per-message request context. The wrap targets ``app._mcp_server.request_handlers[CallToolRequest]`` and is tested against ``mcp==1.25.0``. If that pin moves, re-verify the handler is still @@ -233,7 +233,7 @@ def install_tool_call_middleware( different app (built later in the same process) wraps its own fresh handler. A repeat install with a *different* set raises rather than silently drop the newcomers, because the marker snapshots the set at install time (the supported - pattern is to register everything, then install once). Raises too if FastMCP + pattern is to build the full list, then install once). Raises too if FastMCP has not registered the handler, so an SDK-internal change fails loud rather than silently skipping the chain. """ diff --git a/packages/mcp/src/pipefy_mcp/server.py b/packages/mcp/src/pipefy_mcp/server.py index 3c15bd00..e979b44c 100644 --- a/packages/mcp/src/pipefy_mcp/server.py +++ b/packages/mcp/src/pipefy_mcp/server.py @@ -8,7 +8,11 @@ from mcp.server.fastmcp import FastMCP from pipefy_mcp.core.runtime import McpRuntime -from pipefy_mcp.core.tool_middleware import install_tool_call_middleware +from pipefy_mcp.core.tool_middleware import ( + ToolCallMiddleware, + install_tool_call_middleware, +) +from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.settings import Settings from pipefy_mcp.tools.registry import ToolRegistry from pipefy_mcp.tools.validation_envelope import install_pipefy_validation_envelope @@ -74,6 +78,22 @@ def _register_pipefy_tools(app: FastMCP, *, remote_mode: bool) -> None: registry.apply_remote_profile(remote_mode=remote_mode) +def default_tool_middlewares(settings: Settings) -> list[ToolCallMiddleware]: + """The built-in tool-call middleware to seed for the resolved profile. + + Structured tool-call logging is on by default under the hosted ``remote`` + profile, where operators rely on it to attribute activity. This is a default, + not a capability boundary: the chain installs on every profile, so any + deployment can register its own middleware, and this could later become a + config toggle a local deployment opts into. Deciding the seed here, at the + composition root, keeps the runtime free of any dependency on a concrete + middleware. + """ + if settings.mcp.profile == "remote": + return [tool_log_middleware] + return [] + + def build_pipefy_mcp_server(settings: Settings) -> FastMCP: """Build the FastMCP app with its tools registered once, before serving. @@ -102,11 +122,10 @@ def build_pipefy_mcp_server(settings: Settings) -> FastMCP: auth=auth, ) _register_pipefy_tools(app, remote_mode=settings.mcp.profile == "remote") - # Wrap the tool-call handler with the runtime's registered middleware chain. - # Both transports serve this app, so tool calls over stdio and HTTP alike run - # through the chain; the built-in logger is seeded per profile (see - # McpRuntime.for_profile) and this is a no-op when nothing is registered. - install_tool_call_middleware(app, runtime.tool_middlewares) + # Wrap the tool-call handler with the profile's built-in middleware chain. Both + # transports serve this app, so tool calls over stdio and HTTP alike run through + # the chain; the install is a no-op when the list is empty. + install_tool_call_middleware(app, default_tool_middlewares(settings)) return app diff --git a/packages/mcp/tests/core/test_runtime.py b/packages/mcp/tests/core/test_runtime.py index 82b0b699..045d9ac7 100644 --- a/packages/mcp/tests/core/test_runtime.py +++ b/packages/mcp/tests/core/test_runtime.py @@ -19,7 +19,6 @@ RequestScopedIdentity, StartupIdentity, ) -from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.settings import McpSettings, Settings @@ -166,25 +165,6 @@ def test_local_static_token_binds_the_static_bearer(self, clear_auth_env): assert runtime.inbound_auth is None assert _bearer_of(runtime.session_for_request(None)) == "Bearer env-bearer" - @pytest.mark.unit - def test_remote_seeds_the_tool_log_middleware(self): - """The hosted profile seeds structured tool-call logging on the chain.""" - runtime = McpRuntime.for_profile(remote_rs_settings()) - - assert runtime._tool_middlewares == [tool_log_middleware] - - @pytest.mark.unit - def test_local_seeds_no_tool_middleware(self, clear_auth_env): - """The local profile leaves the chain empty (logging is a hosted concern).""" - settings = Settings( - pipefy=PipefySettings(base_url="https://api.pipefy.com"), - auth=AuthSettings(static_token="env-bearer"), - ) - - runtime = McpRuntime.for_profile(settings) - - assert runtime._tool_middlewares == [] - @pytest.mark.unit @patch("pipefy_auth.resolver.load_session", lambda **_: None) def test_local_without_credential_fails_fast(self, clear_auth_env): diff --git a/packages/mcp/tests/test_server.py b/packages/mcp/tests/test_server.py index f3cc6848..43599934 100644 --- a/packages/mcp/tests/test_server.py +++ b/packages/mcp/tests/test_server.py @@ -11,11 +11,13 @@ from pipefy_auth import AuthSettings from pipefy_sdk import PipefySettings +from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.server import ( _assert_safe_http_bind, _make_lifespan, _register_pipefy_tools, build_pipefy_mcp_server, + default_tool_middlewares, run_server, ) from pipefy_mcp.settings import McpSettings, Settings, resolve_mcp_settings @@ -131,6 +133,18 @@ def test_build_server_remote_mode_exposes_only_the_remote_safe_seed(mocked_runti assert "execute_graphql" not in exposed +@pytest.mark.unit +def test_default_tool_middlewares_seeds_the_logger_under_remote(): + """The composition root seeds structured tool-call logging for the hosted profile.""" + assert default_tool_middlewares(_REMOTE_PROFILE_SETTINGS) == [tool_log_middleware] + + +@pytest.mark.unit +def test_default_tool_middlewares_seeds_nothing_under_local(): + """The local profile gets no default middleware; the chain stays empty.""" + assert default_tool_middlewares(_MINIMAL_PIPEFY_SETTINGS) == [] + + @pytest.mark.unit def test_second_registration_pass_is_rejected_by_collision_preflight(mocked_runtime): """A second registration pass on the same app is rejected by the preflight. From 2a5892d43b3f123f56f3171d70faf7e175bf91f7 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 17:48:15 -0300 Subject: [PATCH 08/11] refactor(mcp): simplify the tool-call install idempotency guard The install marker snapshotted the middleware tuple and compared it on re-install, buying a same-set-is-a-no-op tolerance no caller reaches (install runs once per app at the composition root) at the cost of object-identity equality semantics on callables. Replace it with a boolean marker that raises on any second install: simpler, and still fail-loud against silently stacking or dropping middleware. Also drop CallerIdentity from the module __all__; its home is auth.request_identity and nothing imports it through this module. --- .../src/pipefy_mcp/core/tool_middleware.py | 39 +++++++------------ .../mcp/tests/core/test_tool_middleware.py | 22 +++-------- 2 files changed, 20 insertions(+), 41 deletions(-) diff --git a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py index adb98d79..c285d1ca 100644 --- a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py +++ b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py @@ -40,7 +40,6 @@ __all__ = [ "CallNext", - "CallerIdentity", "ToolCallContext", "ToolCallMiddleware", "compose", @@ -55,12 +54,11 @@ _MAX_ARG_KEYS = 64 _MAX_ARG_KEY_LENGTH = 200 -# Holds the middleware set the installed chain was built from, so a second install -# with the same set is a no-op and one with a different set fails loud rather than -# silently dropping the newcomers. The marker lives on the wrapped function (per -# app), not on a module or class global: each FastMCP instance has its own handler, -# so a global would wrap the first app and silently skip every later one built in -# the same process. +# Marks the wrapped handler so a second install on the same app fails loud instead +# of silently stacking or dropping middleware. The marker lives on the wrapped +# function (per app), not on a module or class global: each FastMCP instance has +# its own handler, so a global would wrap the first app and silently skip every +# later one built in the same process. _INSTALLED_MARKER = "__pipefy_tool_call_chain__" @@ -228,13 +226,10 @@ def install_tool_call_middleware( the default (no middleware) path pays nothing per call rather than routing every tool call through a pass-through wrapper that builds and discards a context. - Idempotent per app: the marker lives on the installed handler, so a repeat - install on the same app with the same middleware set is a no-op while a - different app (built later in the same process) wraps its own fresh handler. A - repeat install with a *different* set raises rather than silently drop the - newcomers, because the marker snapshots the set at install time (the supported - pattern is to build the full list, then install once). Raises too if FastMCP - has not registered the handler, so an SDK-internal change fails loud rather than + Installed once per app: a second install on the same app raises (build the full + middleware list and install it once), while a different app built later in the + same process wraps its own fresh handler. Raises too if FastMCP has not + registered the handler, so an SDK-internal change fails loud rather than silently skipping the chain. """ if not middlewares: @@ -248,20 +243,16 @@ def install_tool_call_middleware( ) terminal = handlers[types.CallToolRequest] - installed = getattr(terminal, _INSTALLED_MARKER, None) - if installed is not None: - if tuple(middlewares) != installed: - raise RuntimeError( - "a tool-call chain is already installed on this app with a " - "different middleware set; register all middleware before calling " - "install_tool_call_middleware once" - ) - return + if getattr(terminal, _INSTALLED_MARKER, False): + raise RuntimeError( + "a tool-call chain is already installed on this app; build the full " + "middleware list and install it once" + ) chain = compose(middlewares, terminal) async def chained(req: types.CallToolRequest) -> types.ServerResult: return await chain(context_from_request(req)) - setattr(chained, _INSTALLED_MARKER, tuple(middlewares)) + setattr(chained, _INSTALLED_MARKER, True) handlers[types.CallToolRequest] = chained diff --git a/packages/mcp/tests/core/test_tool_middleware.py b/packages/mcp/tests/core/test_tool_middleware.py index 75a950e1..880fa1cb 100644 --- a/packages/mcp/tests/core/test_tool_middleware.py +++ b/packages/mcp/tests/core/test_tool_middleware.py @@ -2,7 +2,7 @@ Covers the seam mechanics against a real FastMCP app: composition order, short-circuit, exception propagation, the short-circuit envelope shape, the -per-app idempotent install, and the argument-context build. +once-per-app install, and the argument-context build. """ from __future__ import annotations @@ -201,28 +201,16 @@ def test_install_with_no_middleware_leaves_the_handler_untouched(): @pytest.mark.unit -def test_install_is_idempotent_per_app(): - """A second install on the same app is a no-op (same wrapped handler).""" - app = _app() - install_tool_call_middleware(app, [_noop]) - first = app._mcp_server.request_handlers[types.CallToolRequest] - - install_tool_call_middleware(app, [_noop]) - second = app._mcp_server.request_handlers[types.CallToolRequest] - - assert first is second - - -@pytest.mark.unit -def test_reinstall_with_a_different_middleware_set_raises(): - """A second install with a changed set fails loud instead of dropping it.""" +def test_reinstalling_on_the_same_app_raises(): + """Install is once-per-app: a second install fails loud instead of silently + stacking or dropping middleware. Build the full list, install once.""" async def other(ctx: ToolCallContext, call_next): return await call_next(ctx) app = _app() install_tool_call_middleware(app, [_noop]) - with pytest.raises(RuntimeError, match="different middleware set"): + with pytest.raises(RuntimeError, match="already installed"): install_tool_call_middleware(app, [_noop, other]) From c8edfdc44acd798ec551d3a2ceeccbe4b040cf59 Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 20:48:25 -0300 Subject: [PATCH 09/11] feat(mcp): expose tool-call middleware registration through the builder Add an optional extra_tool_middlewares to build_pipefy_mcp_server so a consumer of the builder (a hosted serving layer wanting per-tool metrics, say) folds its own middleware into the single install rather than reaching into the private request_handlers slot. The built-in defaults run outer to the consumer's, so the default observability layer records every call, including those a consumer's middleware short-circuits. --- packages/mcp/AGENTS.md | 12 ++++++---- packages/mcp/src/pipefy_mcp/server.py | 24 ++++++++++++++----- packages/mcp/tests/test_server.py | 33 +++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 11 deletions(-) diff --git a/packages/mcp/AGENTS.md b/packages/mcp/AGENTS.md index 7dc5573c..b5a590d3 100644 --- a/packages/mcp/AGENTS.md +++ b/packages/mcp/AGENTS.md @@ -180,9 +180,11 @@ dispatches every tool call through one `request_handlers[CallToolRequest]` slot; registered middleware around it. The middleware chain is the extension surface; the private slot is wrapped, not written to directly. -A middleware is a plain async callable. Add it to the chain at the composition -root (`default_tool_middlewares` in `server.py`), never by touching FastMCP -internals: +A middleware is a plain async callable. A built-in middleware joins the per-profile +defaults (`default_tool_middlewares` in `server.py`); a consumer of +`build_pipefy_mcp_server` passes its own through `extra_tool_middlewares`, which the +builder folds into the single install after the built-ins (so the default +observability layer stays outermost). Neither path touches FastMCP internals: ```python from pipefy_mcp.core.tool_middleware import ToolCallContext, CallNext, short_circuit_error @@ -192,8 +194,8 @@ async def quota(ctx: ToolCallContext, call_next: CallNext): return short_circuit_error("quota exceeded", code="RATE_LIMITED") return await call_next(ctx) -# server.py builds the list per profile and hands it to install: -# install_tool_call_middleware(app, default_tool_middlewares(settings)) +# a serving layer registers its own middleware through the public builder: +# app = build_pipefy_mcp_server(settings, extra_tool_middlewares=[quota]) ``` - **Order**: list order runs outer to inner around the tool. `[A, B]` runs A, diff --git a/packages/mcp/src/pipefy_mcp/server.py b/packages/mcp/src/pipefy_mcp/server.py index e979b44c..7a1a41bd 100644 --- a/packages/mcp/src/pipefy_mcp/server.py +++ b/packages/mcp/src/pipefy_mcp/server.py @@ -2,7 +2,7 @@ import logging import textwrap -from collections.abc import AsyncIterator, Callable +from collections.abc import AsyncIterator, Callable, Sequence from contextlib import AbstractAsyncContextManager, asynccontextmanager from mcp.server.fastmcp import FastMCP @@ -94,7 +94,10 @@ def default_tool_middlewares(settings: Settings) -> list[ToolCallMiddleware]: return [] -def build_pipefy_mcp_server(settings: Settings) -> FastMCP: +def build_pipefy_mcp_server( + settings: Settings, + extra_tool_middlewares: Sequence[ToolCallMiddleware] = (), +) -> FastMCP: """Build the FastMCP app with its tools registered once, before serving. Reads everything from the resolved ``settings`` the composition root @@ -102,6 +105,13 @@ def build_pipefy_mcp_server(settings: Settings) -> FastMCP: remote-safe tool surface, and ``settings.mcp.host`` / ``settings.mcp.port`` give the HTTP bind (they matter only for the HTTP transport; stdio ignores them). + ``extra_tool_middlewares`` is the public registration seam for a consumer of this + builder (a hosted serving layer that wants per-tool metrics, say): the chain + installs once, so a consumer folds its middleware in here rather than reaching + into the private ``request_handlers`` slot or re-wrapping the handler. The + built-in middleware runs outer to the consumer's, so the default observability + layer records every call including those a consumer's middleware short-circuits. + The one app-scoped :class:`McpRuntime` is built via :meth:`McpRuntime.for_profile`, which owns both the outbound identity and (under ``remote``) the inbound resource-server ``(verifier, auth)`` pair. When that pair @@ -122,10 +132,12 @@ def build_pipefy_mcp_server(settings: Settings) -> FastMCP: auth=auth, ) _register_pipefy_tools(app, remote_mode=settings.mcp.profile == "remote") - # Wrap the tool-call handler with the profile's built-in middleware chain. Both - # transports serve this app, so tool calls over stdio and HTTP alike run through - # the chain; the install is a no-op when the list is empty. - install_tool_call_middleware(app, default_tool_middlewares(settings)) + # Wrap the tool-call handler with the built-in chain plus any consumer middleware. + # Both transports serve this app, so tool calls over stdio and HTTP alike run + # through the chain; the install is a no-op when the combined list is empty. + install_tool_call_middleware( + app, [*default_tool_middlewares(settings), *extra_tool_middlewares] + ) return app diff --git a/packages/mcp/tests/test_server.py b/packages/mcp/tests/test_server.py index 43599934..d15bfc45 100644 --- a/packages/mcp/tests/test_server.py +++ b/packages/mcp/tests/test_server.py @@ -1,3 +1,4 @@ +import json from datetime import timedelta from unittest.mock import MagicMock, patch @@ -11,6 +12,7 @@ from pipefy_auth import AuthSettings from pipefy_sdk import PipefySettings +from pipefy_mcp.core.tool_middleware import ToolCallContext, short_circuit_error from pipefy_mcp.observability.tool_log_middleware import tool_log_middleware from pipefy_mcp.server import ( _assert_safe_http_bind, @@ -145,6 +147,37 @@ def test_default_tool_middlewares_seeds_nothing_under_local(): assert default_tool_middlewares(_MINIMAL_PIPEFY_SETTINGS) == [] +@pytest.mark.anyio +async def test_extra_tool_middlewares_register_through_the_public_builder( + mocked_runtime, +): + """A consumer's middleware installs via ``extra_tool_middlewares``, no private reach. + + Drives a real MCP session against an app built through the public builder: a + short-circuiting middleware passed as ``extra_tool_middlewares`` returns its + envelope, so the consumer's middleware demonstrably runs in the installed chain + (the tool never executes, so no live client is needed). The short-circuit + envelope's shape is pinned elsewhere; this asserts only that the seam wires it in. + """ + + async def deny(ctx: ToolCallContext, call_next): + return short_circuit_error("blocked by consumer", code="DENIED") + + app = build_pipefy_mcp_server( + _MINIMAL_PIPEFY_SETTINGS, extra_tool_middlewares=[deny] + ) + session = create_client_session( + app, + read_timeout_seconds=timedelta(seconds=10), + raise_exceptions=True, + ) + async with session as s: + result = await s.call_tool("get_organization", {}) + + assert result.isError is True + assert json.loads(result.content[0].text)["error"]["code"] == "DENIED" + + @pytest.mark.unit def test_second_registration_pass_is_rejected_by_collision_preflight(mocked_runtime): """A second registration pass on the same app is rejected by the preflight. From 1076f5d9172c76644590ee11cc6092c96dd47dab Mon Sep 17 00:00:00 2001 From: Gabriel Date: Wed, 8 Jul 2026 23:25:27 -0300 Subject: [PATCH 10/11] test(mcp): pin short_circuit_error parity with the in-tool error envelope Correct the short_circuit_error docstring: it does not replicate FastMCP's dict normalization byte-for-byte. A dict-returning tool with no output schema runs through FastMCP's own encoder (non-ASCII left raw, structuredContent unset), while short_circuit_error uses json.dumps and fills structuredContent. The match is on the decoded envelope, not the bytes. Add a contract test that drives a real dict-returning tool through the terminal and asserts the decoded envelope equals short_circuit_error's, with the intended isError divergence, so an SDK encoder change fails loud rather than drifting the two apart silently. --- .../src/pipefy_mcp/core/tool_middleware.py | 13 ++++--- .../mcp/tests/core/test_tool_middleware.py | 37 +++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py index c285d1ca..3ed8c1dc 100644 --- a/packages/mcp/src/pipefy_mcp/core/tool_middleware.py +++ b/packages/mcp/src/pipefy_mcp/core/tool_middleware.py @@ -193,12 +193,13 @@ def short_circuit_error( ) -> types.ServerResult: """A short-circuit result a middleware returns instead of running the tool. - Carries the canonical ``tool_error`` envelope so the body matches an in-tool - failure, but sets ``isError=True`` deliberately: a governance stop (quota, - rate limit) means the tool never ran, which is distinct from a tool that ran - and reported a business error (``isError=False``). Because this bypasses - FastMCP's terminal, it replicates FastMCP's dict normalization itself (the - same envelope in both ``structuredContent`` and serialized ``content``). + Carries the canonical ``tool_error`` envelope so a client reads a governance + stop (quota, rate limit) the same way it reads a tool's own failure, but sets + ``isError=True`` deliberately: the tool never ran, distinct from a tool that ran + and reported a business error (``isError=False``). Bypassing FastMCP's terminal, + it builds the result directly, putting the envelope in the serialized ``content`` + (where an in-tool error carries it too) and in ``structuredContent``. The match to + an in-tool error is on the decoded envelope, not the byte serialization. """ # Imported lazily: pipefy_mcp.tools.tool_error_envelope pulls the tools # package, which imports the runtime, which imports this module. A top-level diff --git a/packages/mcp/tests/core/test_tool_middleware.py b/packages/mcp/tests/core/test_tool_middleware.py index 880fa1cb..df691470 100644 --- a/packages/mcp/tests/core/test_tool_middleware.py +++ b/packages/mcp/tests/core/test_tool_middleware.py @@ -24,6 +24,7 @@ install_tool_call_middleware, short_circuit_error, ) +from pipefy_mcp.tools.tool_error_envelope import tool_error def _app() -> FastMCP: @@ -191,6 +192,42 @@ def test_short_circuit_error_shape(): } +@pytest.mark.unit +def test_short_circuit_matches_the_in_tool_error_envelope(): + """A short-circuit decodes to the same envelope a client reads from an in-tool error. + + A governance stop should carry the same ``tool_error`` payload an agent gets when a + tool runs and fails, so the client needs no special-casing. The two are compared on + the parsed envelope, not the bytes: FastMCP serializes a dict return through its own + encoder (non-ASCII left raw, ``structuredContent`` unset for a schema-less tool), + while ``short_circuit_error`` uses ``json.dumps`` and fills ``structuredContent``. + Comparing decoded JSON pins the contract that matters and fails loud if either + encoder or the envelope shape drifts, without false-alarming on cosmetics. The one + intended divergence: the short-circuit is ``isError=True`` (the tool never ran). The + non-ASCII value below crosses the encoders' escaping difference to prove the point. + """ + message, code, details = "blocked", "DENIED", {"reason": "quota (café)"} + + app = FastMCP("parity") + + @app.tool() + async def failing() -> dict: + return tool_error(message, code=code, details=details) + + request = types.CallToolRequest( + method="tools/call", + params=types.CallToolRequestParams(name="failing", arguments={}), + ) + in_tool = asyncio.run(_invoke(app, request)).root + short_circuit = short_circuit_error(message, code=code, details=details).root + + assert json.loads(in_tool.content[0].text) == json.loads( + short_circuit.content[0].text + ) + assert in_tool.isError is False + assert short_circuit.isError is True + + @pytest.mark.unit def test_install_with_no_middleware_leaves_the_handler_untouched(): """An empty chain is a no-op: FastMCP's own handler stays in place.""" From 09a598b4fd28871bb6839437ddb05f4bcaa45d9e Mon Sep 17 00:00:00 2001 From: Gabriel Date: Thu, 9 Jul 2026 12:25:54 -0300 Subject: [PATCH 11/11] fix(pr-review): document log-line integrity dependency on root handler --- .../src/pipefy_mcp/observability/tool_log_middleware.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py index bfd70105..766167d0 100644 --- a/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py +++ b/packages/mcp/src/pipefy_mcp/observability/tool_log_middleware.py @@ -12,6 +12,14 @@ Output goes through the standard ``logging`` module, which writes to stderr. It must never write to stdout: the stdio transport frames JSON-RPC over stdout, so a stray log line there corrupts the protocol. + +Line integrity depends on the process's root log handler, which this module does +not configure. FastMCP's default ``RichHandler`` wraps at width 80 in a non-TTY, +which would split an event across physical lines; and a deployment that +reconfigures root logging before construction can drop these INFO lines. The +hosted logging wiring (the structured-log emitter) owns installing a plain, +non-wrapping formatter at INFO so the one-line contract holds; until then a host +that cares must install one. """ from __future__ import annotations