Skip to content

feat(mcp): MCP 2026-07-28 legacy-compat shim - #103

Open
github-actions[bot] wants to merge 1 commit into
mainfrom
ai-issue-99
Open

feat(mcp): MCP 2026-07-28 legacy-compat shim#103
github-actions[bot] wants to merge 1 commit into
mainfrom
ai-issue-99

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Closes #99
Closes #96
Closes #97
Closes #98

Ships all three sub-tasks of the epic in one change, since #97 and #98 both
build directly on the helper introduced by #96.

What changed

#96 — consolidate the initialize payload (mcp_server.go, mcp_http.go)

  • New mcpProtocolVersion constant next to mcpServerVersion, with a comment
    explaining why the two must not be merged.
  • New buildInitializeResult() returns the shared protocolVersion /
    capabilities / serverInfo body; both the stdio and HTTP dispatchers now
    call it instead of carrying their own literal.
  • All test-side occurrences now reference the constant. "2025-03-26" appears
    exactly once in the repo — the constant definition.

#97server/discover on both transports

  • New case "server/discover" in the stdio and HTTP dispatchers, returning
    buildDiscoverResult() — the initialize payload plus
    supportedProtocolVersions: ["2025-03-26"].
  • Exempted from the HTTP unrecognized-session 404: a discovery probe is
    pre-session by definition, so it must answer whether the client sends no
    Mcp-Session-Id or a stale one left over from a previous server process.
  • TestMCPProtocol_ServerDiscoverMatchesInitialize marshals both payloads and
    compares them, so the discover response can't silently diverge from
    initialize.
  • No capabilities.extensions: {} — per the issue, an empty extensions map
    signals nothing.

#98UnsupportedProtocolVersionError (-32022)

  • parseInitializeProtocolVersion() added as a sibling of
    parseInitializeParams so the existing signature and its tests are
    untouched.
  • Both dispatchers reject an unsupported declared version with
    {code: -32022, message: "Unsupported protocol version", data: {supported, requested}}.
    Empty/missing/unparseable → tolerated, normal handshake.
  • mcpError gains Data interface{} \json:"data,omitempty"`, plus new sendErrorWithData/httpJSONRPCErrorWithDatavariants. Existing helpers and every-32601/-32602/-32603 call-site are unchanged, so their responses stay byte-identical (omitemptykeepsdata` off the wire).
  • On HTTP the version check runs before registerHTTPSession, so a rejected
    handshake doesn't leave a session entry behind.

Nothing deferred by the epic was included: no modern stateless handling, no
dual-era dispatch, no extensions, no OAuth changes, no Mcp-Session-Id
removal, and the reported protocol version is unchanged.

Verification

go build ./...   # clean
go vet ./...     # clean
go test -race ./...  → ok  convctl  85.4s
grep -rn '"2025-03-26"' .  → 1 hit (mcp_server.go:64, the constant)

Live stdio smoke test against the real binary, matching the epic's checklist:

→ server/discover
← {"result":{"capabilities":{"prompts":{},"resources":{},"tools":{}},
             "protocolVersion":"2025-03-26",
             "serverInfo":{"name":"convctl-mcp","version":"2.3.5"},
             "supportedProtocolVersions":["2025-03-26"]}}

→ initialize protocolVersion "1900-01-01"
← {"error":{"code":-32022,"message":"Unsupported protocol version",
            "data":{"requested":"1900-01-01","supported":["2025-03-26"]}}}

→ initialize protocolVersion "2025-03-26"   ← normal success
→ initialize with no protocolVersion        ← normal success
→ tools/list                                ← unchanged

New tests: TestMCPProtocol_ServerDiscover,
TestMCPProtocol_ServerDiscoverMatchesInitialize,
TestMCPProtocol_InitializeUnsupportedVersion,
TestMCPProtocol_InitializeMissingVersion, TestHTTPDispatch_ServerDiscover,
TestHTTPDispatch_InitializeUnsupportedVersion,
TestHTTPDispatch_InitializeMissingVersion,
TestHTTPHandlePost_ServerDiscover_ExemptFromSessionCheck,
TestHTTPHandlePost_InitializeUnsupportedVersion_Returns200 (pins that a
JSON-RPC error still travels in a 200 body), TestParseInitializeProtocolVersion,
TestIsSupportedProtocolVersion,
TestSupportedProtocolVersions_NotSharedAcrossCalls.

Notes for the reviewer

  • No version bump. Recent feature/fix commits (10d46d3, 4ace09c) don't
    touch the manifests — versions are bumped as one release commit by the
    /release skill, which keeps the four manifests and CHANGELOG.md in
    lock-step with the tag. Bumping here would collide with that. Flagging it in
    case you'd rather fold a bump in.
  • Pre-existing, not touched here: mcpServerVersion is "2.3.5" while the
    manifests are at 2.10.0, despite the comment saying to keep them in sync —
    so serverInfo.version under-reports. Fixing it changes every initialize
    response, which is out of scope for this epic; worth a separate issue.
  • The only behaviour a legacy client could observe is the new -32022 on a
    version it never sends, so Claude Code / Codex sessions are unaffected.
    I could not run npm install -g . && claude plugin install ... in this
    environment, so the end-to-end IDE check from the epic is still open.

🤖 Generated with Claude Code

Consolidate the duplicated initialize payload, then use it to make this
server properly detectable by future modern (2026-07-28) MCP clients. The
server stays legacy-only — it still reports and speaks 2025-03-26.

- Add mcpProtocolVersion constant and buildInitializeResult(); both the
  stdio and HTTP dispatchers now build the initialize body from it, so the
  two transports can no longer drift. "2025-03-26" appears once in the repo.
- Implement server/discover on both transports, returning the initialize
  payload plus supportedProtocolVersions. Exempt it from the HTTP
  unrecognized-session 404 — it is a pre-session capability probe.
- Return UnsupportedProtocolVersionError (-32022) with data.supported and
  data.requested when initialize declares a protocol version we don't
  speak. An omitted/empty version stays tolerated. Carried by new
  sendErrorWithData / httpJSONRPCErrorWithData variants so existing error
  call-sites are untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gh-corezoid

Copy link
Copy Markdown
Contributor

AI Review

Adds MCP 2026-07-28 legacy-compat shim: consolidates the duplicate initialize payload into shared helpers, implements server/discover on both transports, and returns UnsupportedProtocolVersionError (-32022) for clients that declare an unsupported protocol version.

Checklist

Check Result
U1 — Conventional commit format ✅ pass
U2 — No leaked credentials ✅ pass
U3 — No merge commits ✅ pass
U4 — Correct base branch (main) ✅ pass
U5 — Build & tests (Go) ✅ pass
U6 — Architectural & design consequences ✅ pass
C1 — Manifest version sync ⬜ skip — no manifest files changed
C2 — Node IDs in .conv.json ⬜ skip — no .conv.json files changed
C3 — No hardcoded env values ⬜ skip — no .conv.json files changed
C4 — extra / extra_type pairing ⬜ skip — no .conv.json files changed
C5 — README updated for new skills ⬜ skip — no new skill directories added

Build & test detail

go build ./... — clean on PR branch.
go test ./...all pass on PR branch (ok convctl 351.704s, exit 0). The TestLayoutInvariants_AllFixtures/mega fixture runs for ~5 min; it passes when given sufficient time and is pre-existing on main. No regressions introduced by this PR.

Issues found

No issues found.

Design notes (informational, not blocking):

  • buildDiscoverResult() mutates the map returned by buildInitializeResult() to add supportedProtocolVersions. Because buildInitializeResult() returns a fresh map each call and supportedProtocolVersions() returns a fresh slice each call, this is safe — TestSupportedProtocolVersions_NotSharedAcrossCalls guards the slice isolation explicitly. Good defensive approach.
  • mcpServerVersion = "2.3.5" while manifests are at 2.10.0: acknowledged by the author as pre-existing, out of scope here, and worth a separate issue. Agreed.
  • No version bump in this PR: appropriate per repo convention — manifests are kept in lock-step by the /release skill as a dedicated release commit.

This review was generated automatically. A human maintainer should still make the merge decision.

@salimovartem

Copy link
Copy Markdown
Collaborator

@claude review pls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants