Skip to content
Open
131 changes: 131 additions & 0 deletions docs/pr36-review-verification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
# PR #36: HTTP factory review fixes and verification

## Baseline and dependency

The review was reproduced on PR #36 head `e458282b24ef81f05dd6577f336f0d018541c118`.
Complete collection failed on the four modules listed by the reviewer. Running
the importability and supported-version tests directly reproduced both additional
failures.

Refs recorded before implementation:

| Ref | Commit |
| --- | --- |
| PR #36 / `feature/issue-23-http-factory` | `e458282b24ef81f05dd6577f336f0d018541c118` |
| PR #34 head | `55512e0b8b5231b066b9adb6b2284bdfc0989160` |
| Upstream `develop` | `b3a2e13ae9f6af024f568b10d7c689927ae8ec2a` |

Upstream develop was integrated with normal merge commit `8df5fc4`. The merge
kept the sidecar files deleted. The supported-version test fix was already on
develop and arrived with that merge; no supported protocol date was removed.

PR #36 remains dependent on PR #34 and should land after it. PR #34 was not
modified, and its newer test exclusions were not imported. The separate
spillover branch and PR are not part of this change.

## Architecture and behavior

Official MCP 2.x remains the sole owner of `/mcp`: it parses JSON-RPC, dispatches
requests, and serializes responses. No removed ping helper, dispatch pipeline,
body-replay middleware, or wrapper was restored.

A small public `Server.middleware` hook applies era policy to the SDK's already
parsed request context. It rejects initialization on modern-only servers,
rejects explicit unsupported version claims, and checks that a supplied HTTP
version agrees with its envelope. It delegates valid calls to the official
runner. It does not read or buffer the HTTP request body, dispatch a second
handler, or construct JSON-RPC responses.

The official SDK uses an implicit `2025-03-26` version when compatible HTTP
requests omit a protocol header. That SDK fallback remains allowed for legacy
compatibility; it is not added as a newly advertised protocol date. Explicit
versions still follow the existing era policy.

| Era | Explicit supported dates | Resource subscribe capability |
| --- | --- | --- |
| legacy | 2025-06-18, 2025-11-25 | true; existing compatibility behavior |
| auto | 2025-06-18, 2025-11-25, 2026-07-28 | false; subscription requests rejected |
| modern | 2026-07-28 | false; subscription requests rejected |

The shared subscription predicate is used by discover, capabilities, and
subscribe/unsubscribe handling. Server instances receive the resolved era when
their handlers are configured, so capabilities are correct before HTTP startup,
for additional server instances, and during stdio initialization.

The existing single CORS layer, era-driven `/json/version`, official session
manager, health routes, slash compatibility, and public stdio runner remain.

## Test migration map

| Previous failure or test target | Replacement and retained coverage |
| --- | --- |
| Blueprint import of deleted dispatch classes | Real `server/discover` and tool calls through the mounted HTTP application; capabilities and no-session assertions retained |
| Deprecated-method tests using pipeline and replay doubles | Official HTTP dispatch and registered handlers; sessionless subscription rejection and legacy compatibility tested separately |
| Wire tests using deleted pipeline classes | Actual HTTP method/name/version rejection and successful supported-version requests; parsing, error mapping, identity, and pure validators retained |
| Stateless HTTP imports of deleted ping/dispatch helpers | Mounted-app tool/resource round trips, JSON and SSE responses, all eras, header-only malformed request rejection, unknown-method errors, CORS, slash/health/version checks, verified identity, parameter mirroring, and sessionless isolation |
| Test-module importability | Existing registry remains enabled and unchanged; all registered modules import |
| Supported versions per era | Existing additional legacy date included in test expectations through the develop merge |
| Private stdio dependency | AST checks of runner imports, real memory-stream exchanges, legacy initialization, modern calls, initialization rejection, notifications, and existing stdout safety tests |

The large test deletion reflects removal of tests of retired implementation
details and duplicate pipeline/replay entry points. It does not exclude files
from pytest. Pure header, CORS, parser, SSE, method-contract-table, and response
echo utility tests remain. Coverage of current behavior is exercised at the
official transport boundary instead of through a replacement sidecar.

MCP 2.2 maps modern method-not-found errors to HTTP 404; the legacy transport
maps those errors to HTTP 200 with a JSON-RPC error body. Tests assert those
official transport mappings rather than preserving a deleted sidecar's status.

## Verification

All tests used MCP **2.2.0**. Full runs used:

```text
python -m pytest tests --cov=nitrostack --cov-report=term-missing --timeout=60
```

| Platform | Python | Complete suite | Coverage |
| --- | --- | --- | --- |
| Windows | 3.12.14 | 807 passed | 86.02% |
| Ubuntu / WSL | 3.10.21 | 807 passed | 86.04% |
| Ubuntu / WSL | 3.11.16 | 807 passed | 86.02% |
| Ubuntu / WSL | 3.12.14 | 807 passed | 86.02% |

No tests were skipped, marked xfail, ignored, or deselected in these complete
runs. Coverage's separate "1 empty file skipped" message refers to an empty
source file in its report, not a skipped pytest test.

The final review restored two additional pre-existing pure utility checks:
the method-contract surface and supported response-version echo. Both passed
on all four runtimes after the complete runs above. Final collection contains
**809 tests**, with no collection errors. These two additions do not change
production behavior. The hosted workflow should run all 809 tests on the final
commit; local Linux matrix results above are not a claim about hosted CI status.

Other recorded checks:

- The six reviewer-reported test modules: **225 passed** before the two final
utility checks were restored.
- The original issue verification command: **34 passed**.
- The issue #22/#23 ownership/factory and related HTTP/stdio/deprecation set:
**76 passed**.
- AST validation rejects private `mcp.server.runner` imports.
- Production searches find no deleted sidecar wrappers or ping helpers.
- `git diff --check` passes.

The exact original issue verification command is:

```text
python -m pytest tests/test_transport_http.py tests/test_lifecycle_http.py tests/test_mcp20_stdio.py tests/test_transports.py -q
```

For a fresh environment, install the repository's development extra first:

```text
python -m pip install -e ".[dev]"
```

The existing GitHub workflow provides the Linux Python 3.10/3.11/3.12 matrix.
Its status must be checked separately on the pushed commit. No reviewer reply
was posted as part of the implementation; a response is prepared for user review.
3 changes: 0 additions & 3 deletions nitrostack/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@
from nitrostack.tasks import InMemoryTaskStore, TaskAccessContext, TaskStore
from nitrostack.tasks.authorization import check_task_access, extract_task_access_context
from nitrostack.runtime import StatelessInvariants, assert_stateless_headers
from nitrostack.transports import wrap_stateless_transport, StatelessIngressPipeline


__all__ = [
Expand Down Expand Up @@ -242,6 +241,4 @@
"extract_task_access_context",
"StatelessInvariants",
"assert_stateless_headers",
"wrap_stateless_transport",
"StatelessIngressPipeline",
]
45 changes: 13 additions & 32 deletions nitrostack/core/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
resolve_http_engine,
resolve_protocol_era_resolution,
wire_mode_for_era,
supports_resource_subscriptions,
)
from nitrostack.protocol.mrtr import InputRequiredResult, split_mrtr_from_arguments
from nitrostack.protocol.cache_hints import (
Expand Down Expand Up @@ -867,10 +868,14 @@ def handle_server_discover(self, protocol_version: Optional[str] = None) -> Dict
getattr(self, "protocol_era", None),
self.server_config.protocol_version,
)
# Same source of truth as `NitroStackMcpServer.get_capabilities`, so
# `server/discover` and real capability negotiation never disagree.
resources_subscribe = supports_resource_subscriptions(self.protocol_era)
return build_discover_result(
server_name=self.server_config.name,
server_version=self.server_config.version,
protocol_version=version,
resources_subscribe=resources_subscribe,
advertise_tasks=self._advertise_tasks_extension(),
advertise_app=has_widgets,
custom_extensions=self._custom_extensions(),
Expand Down Expand Up @@ -902,6 +907,7 @@ def _list_endpoint_cache_meta(self) -> Dict[str, Any]:
return build_list_endpoint_cache_hint_meta()

def _setup_handlers(self, server: NitroStackMcpServer) -> None:
server.protocol_era = self.protocol_era
async def _list_tools(
_ctx: ServerRequestContext,
_params: Optional[PaginatedRequestParams],
Expand Down Expand Up @@ -971,14 +977,16 @@ async def _read_resource(ctx: ServerRequestContext, params: ReadResourceRequestP
request_ctx.reset(token)

async def _subscribe_resource(_ctx: ServerRequestContext, params: SubscribeRequestParams):
if rejects_deprecated_method("resources/subscribe", self.protocol_era):
if not supports_resource_subscriptions(self.protocol_era):
message = deprecated_method_message("resources/subscribe")
raise MCPError(types.METHOD_NOT_FOUND, message or "Not supported")
if str(params.uri) not in self._resources:
raise ResourceNotFoundError(str(params.uri))
return types.EmptyResult()

async def _unsubscribe_resource(_ctx: ServerRequestContext, _params: UnsubscribeRequestParams):
if not supports_resource_subscriptions(self.protocol_era):
raise MCPError(types.METHOD_NOT_FOUND, "Resource subscriptions are not supported in this era")
return types.EmptyResult()

async def _list_prompts(_ctx: ServerRequestContext, _params: Optional[PaginatedRequestParams]):
Expand Down Expand Up @@ -1920,37 +1928,10 @@ def get_combined_app(
http_engine=http_engine,
)

if http_engine == "sessionless":
from nitrostack.transports.middleware import wrap_stateless_transport

def _discover_handler(_request):
return self.handle_server_discover()

def _initialize_handler(request):
params = getattr(request, "params", None) or {}
requested = params.get("protocolVersion") if isinstance(params, dict) else None
return self.handle_sessionless_initialize(
requested if isinstance(requested, str) else None
)

http_app = wrap_stateless_transport(
http_app,
server_name=self.server_config.name,
server_version=self.server_config.version,
protocol_version=protocol_version_for_era(era, self.server_config.protocol_version),
advertise_tasks=self._advertise_tasks_extension(),
advertise_app=any(
getattr(entry, "component", None) is not None
for entry in getattr(self, "_tools", {}).values()
),
custom_extensions=self._custom_extensions(),
wire_mode=wire_mode,
protocol_era=era,
enable_cors=enable_cors,
discover_handler=_discover_handler,
initialize_handler=_initialize_handler,
)

# The official MCP SDK owns the complete Streamable HTTP request lifecycle
# mounted by ``build_http_app``. Do not wrap it in a second JSON-RPC
# dispatcher: a sidecar would answer ping/initialize/tools/call itself and
# could diverge from the protocol and session behaviour of the SDK.
return http_app

async def _run_stdio(self) -> None:
Expand Down
41 changes: 40 additions & 1 deletion nitrostack/core/mcp_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,19 @@
from typing import Any, Callable, Dict, Optional

import mcp.types as types
from mcp import MCPError
from mcp.server.lowlevel import Server as LowLevelServer
from mcp.server.lowlevel.server import NotificationOptions
from mcp.server.subscriptions import InMemorySubscriptionBus, ListenHandler
from nitrostack.protocol.errors import JsonRpcErrorCode
from nitrostack.protocol.version import (
MODERN_PROTOCOL_VERSION,
ProtocolEra,
supports_resource_subscriptions,
supported_protocol_versions_for_era,
)
from nitrostack.protocol.meta import flatten_request_meta_object
from nitrostack.protocol.jsonrpc import HeaderBodyMismatchError, validate_protocol_version_header_meta


class NitroStackMcpServer(LowLevelServer):
Expand All @@ -33,12 +43,41 @@ def __init__(self, name: str, version: Optional[str] = None):
self.has_task_support: bool = False
self.http_engine: Optional[str] = None
self.sessionful: bool = False
self.protocol_era: Optional[ProtocolEra] = None
self.middleware.append(self._enforce_protocol_era)
self.discover_handler: Optional[Callable[[], dict[str, Any]]] = None
self.initialize_handler: Optional[Callable[[Optional[str]], dict[str, Any]]] = None
# In-process tests still look up handlers by request type.
self.request_handlers: Dict[Any, Callable[..., Any]] = {}
self.notification_handlers: Dict[Any, Callable[..., Any]] = {}

async def _enforce_protocol_era(self, context, call_next):
"""Apply era policy inside the official runner, after its JSON-RPC parsing."""
if self.protocol_era == "modern" and context.method in (
"initialize", "notifications/initialized",
):
raise MCPError(types.METHOD_NOT_FOUND, f"Method not found: {context.method}")
request_headers = getattr(getattr(context, "request", None), "headers", {})
meta = flatten_request_meta_object(context.meta)
header_version = request_headers.get("mcp-protocol-version")
meta_version = meta.get("io.modelcontextprotocol/protocolVersion")
try:
validate_protocol_version_header_meta(header_version, meta_version)
except HeaderBodyMismatchError as exc:
raise MCPError(int(exc.code), exc.message) from None
claimed_version = header_version or meta_version
# The official SDK uses a 2025-03-26 default when a legacy-compatible
# HTTP request omits the header. Do not reject that implicit fallback.
unsupported_claim = self.protocol_era is not None and claimed_version and (
claimed_version not in supported_protocol_versions_for_era(self.protocol_era)
)
modern_legacy_request = (
self.protocol_era == "modern" and context.protocol_version != MODERN_PROTOCOL_VERSION
)
if unsupported_claim or modern_legacy_request:
raise MCPError(int(JsonRpcErrorCode.UNSUPPORTED_PROTOCOL_VERSION), "Unsupported protocol version")
return await call_next(context)

def handle_server_discover(self) -> dict[str, Any]:
"""Answer ``server/discover`` from this server instance."""
if self.discover_handler is None:
Expand Down Expand Up @@ -67,7 +106,7 @@ def get_capabilities(
)

if caps.resources is not None:
caps.resources.subscribe = True
caps.resources.subscribe = supports_resource_subscriptions(self.protocol_era)

if self.has_task_support:
caps.tasks = types.ServerTasksCapability(
Expand Down
2 changes: 0 additions & 2 deletions nitrostack/protocol/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
from nitrostack.protocol.extensions import MCPExtensionId
from nitrostack.protocol.jsonrpc import (
JsonRpcWireError,
build_ping_response,
build_tool_error_result,
jsonrpc_error,
jsonrpc_success,
Expand Down Expand Up @@ -166,7 +165,6 @@
"build_sessionless_initialize_result",
"build_discover_result",
"parse_jsonrpc_request",
"build_ping_response",
"jsonrpc_success",
"jsonrpc_error",
"build_tool_error_result",
Expand Down
10 changes: 8 additions & 2 deletions nitrostack/protocol/discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,17 @@ def build_discover_result(
custom_extensions: Optional[dict[str, str]] = None,
tools_list_changed: bool = True,
resources_list_changed: bool = True,
resources_subscribe: bool = False,
prompts_list_changed: bool = True,
ttl_ms: int = DEFAULT_LIST_CACHE_TTL_MS,
cache_scope: Literal["public", "private"] = "private",
) -> dict[str, Any]:
"""Build the ``server/discover`` result for the mounted HTTP engine."""
"""Build the ``server/discover`` result for the mounted HTTP engine.

``resources_subscribe`` must match ``NitroStackMcpServer.get_capabilities``
for the same era, so ``server/discover`` and real capability negotiation
never disagree on ``resources.subscribe``.
"""
versions = list(supported_versions or SUPPORTED_PROTOCOL_VERSIONS)
extensions: dict[str, dict[str, str]] = {}
if advertise_app:
Expand All @@ -53,7 +59,7 @@ def build_discover_result(

capabilities: dict[str, Any] = {
"tools": {"listChanged": tools_list_changed},
"resources": {"subscribe": False, "listChanged": resources_list_changed},
"resources": {"subscribe": resources_subscribe, "listChanged": resources_list_changed},
"prompts": {"listChanged": prompts_list_changed},
}
if extensions:
Expand Down
5 changes: 0 additions & 5 deletions nitrostack/protocol/jsonrpc.py
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,6 @@ def jsonrpc_error(request_id: Any, code: int, message: str, data: Any = None) ->
return {"jsonrpc": JSONRPC_VERSION, "id": request_id, "error": error}


def build_ping_response(request_id: Any) -> dict[str, Any]:
"""Ping fast path."""
return jsonrpc_success(request_id, {})


def build_tool_error_result(message: str, *, text_type: str = "text") -> dict[str, Any]:
"""
Tool business failure — JSON-RPC success with isError: true.
Expand Down
5 changes: 5 additions & 0 deletions nitrostack/protocol/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,11 @@ def needs_sessionful_engine(era: ProtocolEra) -> bool:
return era == "legacy"


def supports_resource_subscriptions(era: Optional[ProtocolEra]) -> bool:
"""Only the legacy compatibility era serves resource subscriptions."""
return era == "legacy"


def http_engine_for_era(era: ProtocolEra) -> HttpEngine:
"""
Select the /mcp HTTP engine for an era.
Expand Down
Loading
Loading