diff --git a/docs/issue24-wire-alignment.md b/docs/issue24-wire-alignment.md new file mode 100644 index 0000000..bcc0235 --- /dev/null +++ b/docs/issue24-wire-alignment.md @@ -0,0 +1,89 @@ +# Issue #24: modern MCP wire alignment + +PR #37 extends the official-transport foundation from PRs #34 and #36. The +official MCP implementation owns JSON-RPC parsing, dispatch, negotiation, SSE and +the modern method/name header checks. NitroStack supplies its handlers and era +policy; it does not recreate the deleted JSON-RPC sidecar. + +## Behavior + +| Area | Result | +|---|---| +| Ping | Available only in the legacy era through MCP's public `on_ping` hook. Auto and modern reject it. | +| Retired methods | Modern requests reject retired methods even when the application is configured as auto. Legacy-compatible auto requests retain their existing supported behavior. | +| Discovery | The shared builder supplies `_meta.serverInfo`, preserving cache hints and its top-level compatibility copy. MCP may omit legacy-only top-level fields when serializing the modern wire. | +| Ordinary results | Tool, resource and prompt handlers use native top-level `resultType`; existing metadata and specialized result types are preserved. | +| Capability error | `JsonRpcErrorCode.MISSING_REQUIRED_CLIENT_CAPABILITY` defines `-32021`, with its default message and public protocol export. NitroStack-specific live task-capability enforcement is deferred to the tasks issue. | +| Tasks extension | The application does not advertise the unimplemented 2026 tasks extension, including through custom discovery extensions. Existing legacy task functionality remains available. | + +## Why a small HTTP header check is still necessary + +MCP 2.2.0 selects its strict modern HTTP entry using `MCP-Protocol-Version`. On +that entry, MCP validates `Mcp-Method`, `Mcp-Name`, protocol metadata, duplicates +and unsupported versions. A missing version header instead selects the SDK's +legacy-compatible HTTP entry, so the strict path cannot enforce its own entry +condition. + +The existing mount's `HeaderCompatMiddleware` therefore rejects a modern-era +POST with a missing or blank version header using HTTP 400 / error `-32020`. +It does not read the body, infer a method, replay input or dispatch a request. +The response uses a null ID because the JSON-RPC body was not inspected. Health +and other routes remain outside this mount-specific check. + +Auto preserves genuine 2025 requests. If a request's body explicitly claims the +modern protocol but lacks the HTTP version header, the existing server middleware +rejects the inconsistency. Stdio has no HTTP headers and is exempt from HTTP-only +requirements. Version-header normalization retains duplicate entries so the +official validator can reject them. + +The retirement middleware also evaluates the request's negotiated protocol, not +only the application's era. This prevents auto's legacy task handlers from +answering methods retired on a modern request. Handler registration still uses +MCP's public APIs. + +## Result fields and compatibility + +The SDK's models provide a native `result_type` field serialized as `resultType`. +The handlers use that field rather than inventing `_meta.resultType`. Model +updates retain application metadata and any already-selected result type. + +Discovery's requested `_meta.serverInfo` is distinct from MCP 2.2.0's reserved +`io.modelcontextprotocol/serverInfo` metadata stamp on ordinary modern results. +Tests check the correct location for each. Existing MRTR and legacy task handling +is retained; this PR does not implement a new tasks or MRTR protocol. + +## Removed machinery and replacement coverage + +Removed code includes the unused sessionless initialize payload builder and +application/server aliases, the test-only modern method-contract table, and the +duplicate optional/required method/name validators. Initialization remains owned +by MCP. + +| Previous coverage | Replacement | +|---|---| +| Direct checks of deleted dispatch/pipeline objects | PR #36's migrated HTTP probes retained through the dependency merge. | +| Required/optional method and name helper tests | HTTP requests with missing and mismatched headers for tools, resources and prompts in auto/modern. | +| Static method-contract membership assertion | Successful real HTTP operations covering the requested tool/resource/prompt surface. | +| Sessionless initialize helper inspection | Actual auto initialization response through the official transport. | +| Ping as an HTTP success probe | Tool listing as the success probe, plus explicit legacy success and modern/auto retirement tests for ping. | + +Pure JSON-RPC parsing/error mapping, protocol-version consistency, CORS utilities, +SSE formatting and response-header utility tests remain. No new skips, xfail +markers or exclusions are used. + +## Dependency merge details + +The updated PR #34 contained the new custom CORS middleware and older versions of +the four migrated test modules. The conflict resolution retains the corrected +PR #36 tests and the single Starlette `CORSMiddleware`. Browser-defined parameter +headers are supported through Starlette's request-header mirroring, while allowed +origins remain governed by `MCP_CORS_ALLOWED_ORIGINS`. + +CORS tests assert Starlette's HTTP 200 preflight response and inspect exposed +headers on actual responses. The dual HTTP/stdio test now includes the required +HTTP version header with its modern body metadata. + +The stash was inspected selectively and remains intact. Generated bytecode, +scaffolded applications, OAuth client changes and spillover changes were not +ported into this implementation. See `pr37-review-verification.md` for exact +commit IDs, reproduction commands and validation results. diff --git a/docs/pr37-review-verification.md b/docs/pr37-review-verification.md new file mode 100644 index 0000000..792b3c9 --- /dev/null +++ b/docs/pr37-review-verification.md @@ -0,0 +1,165 @@ +# PR #37 / issue #24 verification + +Validation date: 2026-10-28. Branch: `feature/issue-24-wire-alignment`. +Implementation commit: `0ec5fe265c46d6cbcf35900eca54dcded8865de0`. + +## Recorded inputs and dependency history + +| Input | Commit | +|---|---| +| Published PR #37 head | `e458282b24ef81f05dd6577f336f0d018541c118` | +| Current upstream develop | `e9cf1dbd8127aaa0154d9734586a0245f1bd2e84` | +| PR #34 head | `680c722837f2fa86da03a8aca9b1f10e97dc239f` | +| Corrected PR #36 head | `71e47f10577bb255e18ec745f0ece18c669fedd0` | +| Preserved local stash | `4163a6583b2fc2d3b95f471f64e05d046bd7c318` | + +PRs #34 and #36 were open and unmerged when fetched. The issue branch received +normal merges of PR #36 (`d0391ef`) and the updated PR #34 (`749d4d6`), followed +by the CORS conflict-resolution correction (`e81d34c`). Develop is already an +ancestor through the updated dependency; a subsequent develop merge reported +already up to date. No history was rewritten or force-pushed. + +The issue implementation selectively ports relevant ideas from the stash without +applying or dropping it. Its generated bytecode and scaffolded application were +excluded. OAuth and spillover feature commits are not included. + +## Reviewer failure reproduction + +Collection against an isolated archive of the published head reproduced all four +reviewer-reported errors: **566 tests collected, four collection errors**. + +| Module | Original failure | Correction | +|---|---|---| +| `test_mcp20_blueprint.py` | Deleted dispatch module import | Retain PR #36's actual HTTP discovery/statelessness tests. | +| `test_mcp20_deprecated.py` | Deleted dispatch module import | Retain official HTTP and registered-handler retirement tests. | +| `test_mcp20_jsonrpc_wire.py` | Deleted dispatch module import | Retain pure parsing/error tests and official HTTP validation probes. | +| `test_mcp20_stateless_http.py` | Removed ping helper import | Retain migrated HTTP, malformed-body, CORS and SSE coverage. | + +After dependency integration, collection succeeded with **826 tests**. The final +branch collects **914 tests**, including **99 issue #24 wire cases**. Obsolete +method/name-validator and static method-table checks were replaced with real HTTP +header and successful-operation tests. See `issue24-wire-alignment.md` for the +coverage mapping and architecture explanation. + +## Final results + +All complete runs use **MCP 2.2.0**, the development extra and a 60-second test +timeout. No ignore flags, deselection expressions, new skips or xfail markers +were used. The coverage tool's empty-file message does not indicate a skipped +test. + +| Platform | Python | Complete suite | Coverage | +|---|---|---|---| +| Ubuntu / WSL | 3.10.21 | 914 passed | 86.15% | +| Ubuntu / WSL | 3.11.16 | 914 passed | 86.11% | +| Ubuntu / WSL | 3.12.14 | 914 passed | 86.11% | +| Windows | 3.12.14 | 914 passed | 86.13% | + +The Linux matrix runs an exact `git archive` of implementation commit `0ec5fe2` +on WSL's native filesystem in isolated environments. This avoids the mounted +Windows filesystem's substantial test/build overhead. Logs are copied to the +worktree's ignored `.pytest_cache/linux-final-3.10.log`, +`linux-final-3.11.log` and `linux-final-3.12.log`. The complete final Windows log +is `windows-final.tmp.log`, also ignored. These are local matrix runs, not hosted +GitHub Actions results. + +The combined reviewer/protocol/transport verification set passed **379 tests**: + +```powershell +python -m pytest tests/test_mcp24_wire_alignment.py tests/test_mcp20_blueprint.py tests/test_mcp20_deprecated.py tests/test_mcp20_jsonrpc_wire.py tests/test_mcp20_stateless_http.py tests/test_mcp20_acceptance.py tests/test_mcp20_foundation.py tests/test_mcp22_official_http_owner.py tests/test_mcp23_http_factory.py tests/test_transport_http.py tests/test_lifecycle_http.py tests/test_mcp20_stdio.py tests/test_transports.py -o addopts= -q --timeout=60 --tb=short +``` + +Complete verification commands: + +```powershell +python -m pytest tests --collect-only -o addopts= -q +python -m pytest tests --cov=nitrostack --cov-report=term-missing --timeout=60 --tb=short -o addopts= -q +git diff --check +``` + +## Earlier failures and Windows comparison + +An initial full Windows run reported 912 passed and two failures. One was the +dual HTTP/stdio test sending modern envelope metadata without the required HTTP +version header. Its request now supplies that header and still verifies that +both transports expose the same tools. An earlier Linux 3.10 run also executed +that pre-correction request; the exact-commit final matrix above supersedes it. + +The other Windows failure was the unchanged live OAuth localhost socket test, +`test_discovery_dcr_404_when_disabled_or_missing_client_id`, with WinError 10053. +An archive of the unchanged merged foundation (`e81d34c`), using the same Python +3.12.14 environment and MCP version, reproduced WinError 10053 in the companion +`test_discovery_server_idempotent_start_and_dcr_http` test: **34 passed, 1 failed**. + +Baseline comparison command: + +```powershell +python -m pytest tests/test_oauth.py tests/test_mcp20_stdio.py::test_dual_http_and_stdio_share_the_same_tools -o addopts= -q --timeout=60 --tb=short +``` + +The final complete Windows run passed all tests. The earlier baseline-reproducible +socket failure remains documented as intermittent; its OS-level cause is not +claimed resolved. No OAuth server or socket test was changed or excluded. + +## Static and publication checks + +- `git diff --check` passes and no merge-conflict markers remain. +- Production does not reference the deleted sidecar wrappers or ping helpers. +- Duplicate optional/required method/name validators and the obsolete static + method-contract table are gone; their coverage moved to the official transport. +- The existing stdio AST/public-runner and behavioral regressions pass. +- Dependency files and release version are unchanged by the issue implementation. +- Develop and both dependency heads are ancestors of this branch. +- The saved stash remains intact, and other feature worktrees are unchanged. + +The branch remains stacked on #34 and #36 until they land. PR #37 must target +develop when its updates are published. No push, PR-base edit, reviewer reply or +merge is included in this implementation step. Hosted CI must be checked on the +eventual pushed head. + +## Rebase after PR #34 merged (2026-10-28) + +PR #34 merged normally as `805db05`. Both dependent branches now contain +develop `63ecd9240d98bbe06c879089dd887494c7e99293`, including upstream SecretValue +changes. PR #36 was rebased first; PR #37's issue-specific commits were replayed +onto that rebased foundation. The original local PR #37 head is preserved as +`backup/pr37-before-develop-rebase-20261028`. The original published head was +still `e458282`; publishing uses an explicit force-with-lease against that head. + +PR #37 now targets develop. It still includes PR #36 until that PR merges, but +the already-merged PR #34 commits are no longer outstanding dependency changes. +Tree comparison against the previously validated implementation plus new develop +shows no functional changes lost during the rebase. + +Complete collection succeeds with **954 tests**, including the 99 wire cases. +Exact-source runs at rebased head `a577fe7` use MCP **2.2.0**: + +| Platform | Python | Complete suite | Coverage | +|---|---|---|---| +| Ubuntu / WSL | 3.10.21 | 954 passed | 86.32% | +| Ubuntu / WSL | 3.11.16 | 954 passed | 86.30% | +| Ubuntu / WSL | 3.12.14 | 954 passed | 86.30% | +| Windows | 3.12.14 | 953 passed, 1 socket failure on first run | 86.30% | + +The Windows failure is `test_discovery_dcr_404_when_disabled_or_missing_client_id` +with WinError 10053, matching the intermittent baseline-reproduced failure +documented above. A complete repeat is recorded separately below. No exclusions, +skips or xfails were added. Logs are retained in +`.pytest_cache/rebase-linux-3.10.log`, `rebase-linux-3.11.log`, +`rebase-linux-3.12.log`, `rebase-windows-full.tmp.log` and +`rebase-windows-repeat.tmp.log`. Hosted CI must be checked on the new published +head; the old 914-test results above are historical. + +The complete Windows repeat reported **952 passed, 2 failed** (86.30% coverage), +both unchanged live OAuth discovery socket tests: +`test_discovery_server_idempotent_start_and_dcr_http` and +`test_discovery_dcr_404_when_disabled_or_missing_client_id`, with WinError 10053. +These Windows full suites are not claimed green. No OAuth source or test changes +were introduced by this rebase, and none of these tests were excluded. + +A fresh archive of current develop `63ecd92`, tested with the same Windows +Python/MCP environment, reproduced **both** failures: **72 passed, 2 failed**. +The command was `python -m pytest tests/test_oauth.py tests/test_secret_value.py +-o addopts= -q --timeout=60 --tb=short`. The log is +`.pytest_cache/rebase-develop-windows.log`. Import-path inspection confirmed the +archive's package was loaded rather than the editable feature checkout.