Tighten comments and docstrings to load-bearing facts only

Author: maxisbey

src/mcp/server/_streamable_http_modern.py

@@ -219,9 +219,7 @@ async def _write(
 _MCP_PARAM_PREFIX_LOWER: Final = MCP_PARAM_HEADER_PREFIX.lower()
 
 _MCP_PARAM_LIST_PAGE_CAP: Final = 100
-"""Upper bound on `tools/list` pages walked while resolving a tool schema for
-`Mcp-Param-*` validation. A paginator bug (non-terminating or cycling cursor
-chain) must degrade to a logged validation skip, never a request-path hang."""
+"""Page cap for the schema-resolving tools/list walk: a buggy paginator degrades to a logged skip, not a hang."""
 
 
 async def _tool_input_schema(
@@ -234,14 +232,9 @@ async def _tool_input_schema(
 ) -> Any | None:
     """Resolve `name`'s inputSchema from the server's own registered `tools/list` handler.
 
-    The synthetic listing runs through the normal `serve_one` path — the
-    caller's envelope (rebuilt from the classifier verdict, so caller-side
-    `_meta` extras like a progress token are not replayed), the middleware
-    chain, a fresh per-page `Connection`, and an absorbing dispatch context
-    (no SSE sink, so request-scoped notifications drop instead of raising) —
-    so a visibility-scoped catalog yields exactly what *this* caller was
-    advertised. Returns `None` (caller skips validation) when the listing
-    raises, paginates past the cap or cycles, or never advertises the tool.
+    The listing runs through the normal `serve_one` path, so a visibility-scoped
+    catalog yields exactly what *this* caller was advertised. Returns None
+    (caller skips validation) when the listing fails or never advertises the tool.
     """
     meta = {
         PROTOCOL_VERSION_META_KEY: verdict.protocol_version,
@@ -258,9 +251,7 @@ async def _tool_input_schema(
         message_metadata=ServerMessageMetadata(request_context=request),
     )
     for _ in range(_MCP_PARAM_LIST_PAGE_CAP):
-        # A fresh per-page Connection is load-bearing: serve_one tears down
-        # the connection's exit stack on the way out. The dispatch context is
-        # page-independent and reused.
+        # Fresh Connection per page: serve_one tears down the connection's exit stack on the way out.
         connection = Connection.from_envelope(verdict.protocol_version, client_info, client_capabilities)
         try:
             result = await serve_one(
@@ -271,26 +262,17 @@ async def _tool_input_schema(
                     return tool.get("inputSchema")
             cursor = result.get("nextCursor")
         except ValidationError:
-            # Client fault: a mis-shaped envelope value the classifier admits
-            # on key presence alone fails the kernel's surface validation here
-            # first. The real dispatch produces the INVALID_PARAMS reply, so
-            # this is not worth more than a debug line (an exception-level log
-            # would let any client flood the log blaming the server).
+            # Client-fault envelope: the real dispatch produces the INVALID_PARAMS
+            # reply, and anything above a debug line would let clients flood the log.
             logger.debug("Mcp-Param header validation skipped: the request envelope fails tools/list validation")
             return None
         except Exception:
-            # Boundary by design: header validation must never break a working
-            # call path, so a failing listing — the handler raising, or a
-            # middleware short-circuit returning a mis-shaped result — skips
-            # validation for this request. Loudly, because the skip is
-            # fail-open. (A server broken here is broken for real discovery
-            # too.)
+            # Fail-open boundary by design: header validation must never break a
+            # working call path. Loud, precisely because the skip is fail-open.
             logger.exception("Mcp-Param header validation skipped: the tools/list listing failed")
             return None
         if not isinstance(cursor, str):
-            # Listing exhausted without advertising `name`: nothing was
-            # declared to this caller, so there is nothing to validate —
-            # dispatch owns rejecting a genuinely unknown tool.
+            # Listing exhausted without advertising `name`; dispatch owns rejecting an unknown tool.
             return None
         if cursor in seen_cursors:
             logger.warning("Mcp-Param header validation skipped: the tools/list handler returned a cursor cycle")
@@ -313,13 +295,9 @@ async def _mcp_param_rejection(
 ) -> InboundLadderRejection | None:
     """Validate a `tools/call` request's `Mcp-Param-*` headers against the called tool's schema.
 
-    Runs post-classification, pre-dispatch — and before any SSE machinery, so
-    a rejection is always a plain `application/json` 400 (the spec's MUST for
-    header-validation failures). The schema source is the registered
-    `tools/list` handler; with none registered the catalog is undiscoverable,
-    no client can have been told about an `x-mcp-header` annotation, and there
-    is no recognized header to validate. A mis-shaped `name`/`arguments` is
-    left to params validation at dispatch.
+    Runs pre-dispatch, before any SSE machinery, so a rejection is always a
+    plain `application/json` 400 (the spec's MUST). With no `tools/list` handler
+    the catalog is undiscoverable and there is no recognized header to validate.
     """
     if req.method != "tools/call" or app.get_request_handler("tools/list") is None:
         return None
@@ -331,11 +309,9 @@ async def _mcp_param_rejection(
     if raw_arguments is not None and not isinstance(raw_arguments, Mapping):
         return None
     arguments: Mapping[str, Any] = cast("Mapping[str, Any]", raw_arguments) if raw_arguments is not None else {}
-    # ASGI guarantees lowercase header names, same invariant the classifier
-    # leans on; the pure validator re-folds for arbitrary carriers.
+    # ASGI guarantees lowercase header names, so no case-folding here.
     if not arguments and not any(header.startswith(_MCP_PARAM_PREFIX_LOWER) for header in request.headers):
-        # With no argument values and no `Mcp-Param-*` headers, no declaration
-        # could be violated in either direction — skip the listing outright.
+        # No argument values and no `Mcp-Param-*` headers: no declaration can be violated either way.
         return None
     input_schema = await _tool_input_schema(app, request, req.id, verdict, lifespan_state, name)
     if input_schema is None:
@@ -382,9 +358,7 @@ async def handle_modern_request(
     try:
         decoded = json.loads(body)
     except ValueError:
-        # ValueError, not its JSONDecodeError subclass: an integer literal
-        # beyond CPython's int-conversion digit limit raises the bare parent
-        # and is just as unparseable.
+        # Not just JSONDecodeError: integer literals beyond CPython's digit limit raise bare ValueError.
         rej = JSONRPCError(jsonrpc="2.0", id=None, error=ErrorData(code=PARSE_ERROR, message="Parse error"))
         await _write(rej, scope, receive, send)
         return
@@ -408,10 +382,7 @@ async def handle_modern_request(
 
     duplicated = find_duplicated_routing_header(request.headers.items())
     if duplicated is not None:
-        # The classifier receives a folded mapping that can no longer show
-        # duplicates, so the raw-carrier check lives here: a routing header
-        # supplied twice is unverifiable (a consumer reading one copy and the
-        # validator the other would disagree).
+        # The raw carrier is the only place duplicates are visible; the classifier sees a folded mapping.
         rejection = InboundLadderRejection(code=HEADER_MISMATCH, message=f"{duplicated} header appears more than once")
         await _write_rejection(rejection, req.id, scope, receive, send)
         return

src/mcp/shared/inbound.py

@@ -177,10 +177,8 @@ def decode_header_value(value: str | None) -> str | None:
         decoded = base64.b64decode(payload, validate=True)
     except binascii.Error:
         return None
-    # Canonical-form gate: `validate=True` still tolerates payloads whose
-    # trailing bits are non-zero (e.g. `SGVsbG9=` for `Hello`); re-encoding
-    # must reproduce the payload exactly. The encoder only emits canonical
-    # base64, so no conforming peer is affected.
+    # Reject non-canonical base64 (e.g. non-zero trailing bits), which
+    # `validate=True` tolerates; the encoder only ever emits canonical form.
     if base64.b64encode(decoded).decode("ascii") != payload:
         return None
     try:
@@ -251,27 +249,18 @@ def x_mcp_header_map(input_schema: Any) -> dict[tuple[str, ...], str]:
 def _annotated_positions(input_schema: Any) -> Iterator[tuple[tuple[str, ...], str, dict[str, Any]]]:
     """Yield `(path, token, schema)` for every statically-reachable `x-mcp-header` annotation.
 
-    The single source of the "what counts as a declared header" predicate,
-    consumed by both the client-emit side (:func:`x_mcp_header_map`) and the
-    server-validate side (:func:`validate_mcp_param_headers`) so the two ends
-    of the mirror contract cannot drift.
+    Shared by client emit and server validate so both ends agree on what counts as a declared header.
     """
     for path, schema in _walk_schema_positions(input_schema):
         if path and isinstance(token := schema.get(X_MCP_HEADER_KEY), str):
             yield path, token, schema
 
 
 def _render_header_scalar(value: Any) -> str | None:
-    """Render an argument value the way the client mirrors it into a header, or `None`
-    when no header rendering exists.
-
-    The single source of the "mirrorable value" fact, shared by emit
-    (:func:`mcp_param_headers`, which omits the header) and validate
-    (:func:`validate_mcp_param_headers`, for which a header claiming an
-    unmirrorable value can never match) so the two sides agree by
-    construction. Unmirrorable: non-primitive values — the spec defines
-    rendering for string/integer/boolean only — and integers beyond CPython's
-    int-to-str conversion limit, which no JSON peer can express anyway.
+    """Render `value` the way the client mirrors it into a header, or `None` when no rendering exists.
+
+    Shared by emit and validate so both sides agree on what is mirrorable:
+    non-primitives and ints beyond CPython's int-to-str digit limit are not.
     """
     if isinstance(value, bool):
         return "true" if value else "false"
@@ -291,8 +280,8 @@ def mcp_param_headers(header_map: Mapping[tuple[str, ...], str], arguments: Mapp
     `Mcp-Param-<token>` carrying it: `bool` as `true`/`false`, other scalars via
     `str`, each passed through :func:`encode_header_value` so a non-token value
     is base64-wrapped. A path that hits a missing key or a non-mapping node is
-    skipped, matching the spec's "omit the header when no value is present" —
-    as is a value with no header rendering (see :func:`_render_header_scalar`).
+    skipped, matching the spec's "omit the header when no value is present",
+    as is a value with no header rendering.
     """
     headers: dict[str, str] = {}
     for path, token in header_map.items():
@@ -363,16 +352,9 @@ class InboundLadderRejection:
 def find_duplicated_routing_header(headers: Iterable[tuple[str, str]]) -> str | None:
     """Name of a routing header supplied more than once in raw header lines, or `None`.
 
-    `headers` is the carrier's raw `(name, value)` pairs (e.g.
-    `request.headers.items()`), which — unlike a folded mapping — still shows
-    duplicates. The routing headers (`MCP-Protocol-Version`, `Mcp-Method`,
-    `Mcp-Name`) are always recognized, so a duplicate is always unverifiable:
-    a consumer reading one copy and a validator reading the other would
-    disagree, the divergence header validation exists to prevent. Callers
-    reject with `HEADER_MISMATCH` before running the validation ladder.
-    `Mcp-Param-*` duplicates are not this function's job:
-    :func:`validate_mcp_param_headers` rejects recognized ones and the spec's
-    forward-and-ignore rule covers unrecognized ones.
+    Takes raw `(name, value)` pairs — a folded mapping hides duplicates. A
+    duplicate is rejected because first-copy and last-copy readers would
+    disagree. `Mcp-Param-*` duplicates are :func:`validate_mcp_param_headers`'s job.
     """
     seen: set[str] = set()
     for name, _ in headers:
@@ -468,27 +450,18 @@ def classify_inbound_request(
     )
 
 
-# An integer header value the spec's numeric-comparison SHOULD applies to:
-# optional sign, digits, optional pure-decimal fraction. Scientific notation
-# (`1e2`) and anything looser never compares numerically — the same canonical-
-# decimal gate the typescript-sdk applies on the header side.
+# Header values eligible for the spec's numeric-comparison SHOULD; scientific
+# notation never compares numerically (matching the typescript-sdk's gate).
 _CANONICAL_DECIMAL = re.compile(r"^-?[0-9]+(\.[0-9]+)?$")
 
 
 def _mcp_param_value_matches(prop_type: Any, value: Any, rendered: str, decoded: str) -> bool:
     """True when a decoded `Mcp-Param-*` header value agrees with the body argument.
 
-    `rendered` is the argument's own header rendering, so the fallback
-    comparison is the exact inverse of the emit side by construction. An
-    integer-typed declaration with an integral body value (`int`, or a `float`
-    like `42.0` that JSON Schema also admits as an integer; `bool` excluded —
-    it renders `true`/`false`) compares numerically (`42` matches `42.0`, the
-    spec's SHOULD) but only for canonical-decimal headers, and exactly — no
-    float round-trip, so values outside the IEEE754 safe range still compare
-    correctly. A non-canonical header (e.g. scientific notation) falls back to
-    the rendered comparison, so the client's own rendering of any value always
-    matches; a header whose integer part overflows CPython's int-conversion
-    digit limit can never name a JSON-expressible value, so it does not match.
+    Integer-typed declarations with an integral body value compare numerically
+    (`42` matches `42.0`, the spec's SHOULD) for canonical-decimal headers —
+    exact, no float round-trip, so values beyond the IEEE754 safe range still
+    compare. Anything else compares against `rendered`, the emit-side rendering.
     """
     if (
         prop_type == "integer"
@@ -513,29 +486,15 @@ def validate_mcp_param_headers(
 ) -> InboundLadderRejection | None:
     """Compare a `tools/call` request's `Mcp-Param-*` headers against its body arguments.
 
-    The server half of the `x-mcp-header` contract: for every annotated
-    property in `input_schema`, the body argument and its mirroring header
-    must agree — present together and equal after sentinel decoding, or absent
-    together (`null` counts as absent). Returns the first failure as an
-    :class:`InboundLadderRejection` carrying `HEADER_MISMATCH` (callers map it
-    to HTTP 400 via :data:`ERROR_CODE_HTTP_STATUS`), or `None` when every
-    declaration is satisfied.
-
-    Header names are matched case-insensitively regardless of the carrier's
-    canonicalization, and a recognized header supplied more than once (the
-    carrier's `items()` yielding duplicate names) is rejected — consumers that
-    read the first copy and a validator that read the last would otherwise
-    disagree. Headers with no matching declaration are ignored — the spec's
-    forward-and-ignore rule for unrecognized `Mcp-Param-*` headers. A header
-    present for an absent/null argument is rejected, as is one for an
-    argument no scalar rendering can match (a non-primitive value at an
-    annotated path): the spec's purpose clause is exactly the case of an
-    intermediary routing on a header value the body never carried
-    (streamable-http.mdx §Server Validation). Without the header, a
-    non-primitive argument is left to params validation at dispatch. An
-    `input_schema` that :func:`find_invalid_x_mcp_header` rejects validates
-    nothing: the spec assigns definition rejection to clients, which drop the
-    tool from `tools/list`, so no conforming client emitted headers for it.
+    Each annotated property's header and argument must agree: present together
+    and equal after sentinel decoding, or absent together (`null` counts as
+    absent). Returns the first failure as a `HEADER_MISMATCH` rejection, else `None`.
+
+    A header whose argument is absent or unrenderable is deliberately rejected:
+    the spec's purpose clause is exactly an intermediary routing on a value the
+    body never carried. A duplicated recognized header is rejected — first-copy
+    and last-copy readers would disagree. A schema :func:`find_invalid_x_mcp_header`
+    rejects validates nothing: conforming clients drop the tool and emit no headers.
     """
     if find_invalid_x_mcp_header(input_schema) is not None:
         return None
@@ -566,8 +525,7 @@ def validate_mcp_param_headers(
             continue
         rendered = _render_header_scalar(value)
         if rendered is None:
-            # No header rendering exists for this value, so a conforming
-            # client omitted the header; one claiming it can never match.
+            # Unrenderable value: a conforming client omitted the header, so one claiming it can never match.
             if raw is not None:
                 return InboundLadderRejection(
                     code=HEADER_MISMATCH,

tests/interaction/transports/test_hosting_http_modern.py

@@ -474,11 +474,9 @@ async def test_modern_client_emits_no_param_headers_for_an_unlisted_tool() -> No
     The spec lets a client that lacks the tool's `inputSchema` send the request without custom headers.
     The call is made with no prior `list_tools`, so the first `tools/call` POST -- captured before the
     implicit output-schema `list_tools` runs -- has no cached annotations and emits no `Mcp-Param-*` header.
-    The server, which does validate `Mcp-Param-*` headers against its own catalog, rejects exactly as the
-    spec's scenario table requires for an omitted header whose value is in the body (the client-side
-    relist-and-retry recovery is a spec SHOULD the client does not implement yet).
+    The server validates `Mcp-Param-*` against its own catalog and rejects as the spec's scenario table
+    requires for an omitted header (the relist-and-retry recovery is a SHOULD the client does not implement yet).
     """
-    # The rejected call is the only request: no implicit output-schema list follows a failure.
     requests: list[httpx.Request] = []
 
     async def on_request(request: httpx.Request) -> None:
@@ -519,10 +517,8 @@ async def test_modern_client_stops_mirroring_after_a_re_list_drops_the_tool() ->
     bad_schema = {"type": "object", "properties": {"a": {"type": "string", "x-mcp-header": "bad name"}}}
     valid = Tool(name="run", input_schema=schema)
     invalid = Tool(name="run", input_schema=bad_schema)
-    # First listing valid, every later one invalid. Listings are not counted: beyond the client's own
-    # calls (initial list, the re-list that drops the tool, the implicit re-list before the second call,
-    # since the prune also cleared `run`'s schema entry), the server reads its own catalog to validate
-    # `Mcp-Param-*` headers on each tools/call.
+    # First listing valid, every later one invalid; the count is not pinned because the server also
+    # reads its own catalog on each tools/call.
     listings: list[None] = []
 
     async def list_tools(ctx: ServerRequestContext, params: PaginatedRequestParams | None) -> ListToolsResult: