From 4591cb561c4a85ae51b69c975cc1879a870cc71e Mon Sep 17 00:00:00 2001 From: manish-wekan Date: Thu, 5 Nov 2026 17:57:13 +0530 Subject: [PATCH] docs: remove PR #37 review logs Point-in-time review notes (one records a local stash SHA) that came in with the #37 merge; the code they describe landed via #108. Co-Authored-By: Claude Opus 5.5 --- docs/issue24-wire-alignment.md | 89 ----------------- docs/pr37-review-verification.md | 165 ------------------------------- 2 files changed, 254 deletions(-) delete mode 100644 docs/issue24-wire-alignment.md delete mode 100644 docs/pr37-review-verification.md diff --git a/docs/issue24-wire-alignment.md b/docs/issue24-wire-alignment.md deleted file mode 100644 index bcc0235..0000000 --- a/docs/issue24-wire-alignment.md +++ /dev/null @@ -1,89 +0,0 @@ -# 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 deleted file mode 100644 index 792b3c9..0000000 --- a/docs/pr37-review-verification.md +++ /dev/null @@ -1,165 +0,0 @@ -# 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.