I think this is ready. handleInitialize now follows MCP's version negotiation: if a client asks for a revision the server doesn't support, the server answers with one it does support (2025-06-18) and lets the client decide whether to go on. Before, it sent back -32602, and that's what was breaking clients that requested 2025-11-25. Since only one revision is supported, "echo it back if it matches, otherwise offer ours" comes down to always returning MCPProtocolVersion. That's correct, and the comment where the result is built says so.
I walked each row of TestMCPServerInitializeInvalidParams through the handler to make sure every rejection comes from the check it's meant to test:
null params, {}, an empty version and a null version all decode to an empty ProtocolVersion. They hit the new nonempty check and get "Invalid params".capabilities, and string clientInfo all cause a json.Unmarshal type error. That goes down the existing -32602 "Invalid params" path.So the suite covers both the type-checking rejections and the new empty-string check. The negotiation test sends real wire input. It checks the returned revision and the initialized flag across matching, newer, older and arbitrary version strings, which is the behaviour that matters here.
Changing the old "wrong protocol version" case to "missing protocol version" is the right call. A test asserting the old rejection would now be asserting behaviour that breaks the spec. The new case still covers the remaining -32602 path in handleInitialize. No other test or doc in the repo depended on the old rejection.
Why I'm recommending approval: the change is one small, focused handler edit. It matches the protocol's documented negotiation rules. It only touches initialize, and both the success and rejection paths have wire-level tests that I traced by hand. I don't see anything a human reviewer would need to weigh in on beyond that. As the description says, Portal will still need its pinned dependency bumped after this merges.
Verdict: ready
{
"owner": "mirendev",
"repo": "mflags",
"number": 18,
"verdict": "ready",
"approve": true,
"event": "approve",
"summary": "I think this is ready. `handleInitialize` now follows MCP's version negotiation: if a client asks for a revision the server doesn't support, the server answers with one it does support (`2025-06-18`) and lets the client decide whether to go on. Before, it sent back `-32602`, and that's what was breaking clients that requested `2025-11-25`. Since only one revision is supported, \"echo it back if it matches, otherwise offer ours\" comes down to always returning `MCPProtocolVersion`. That's correct, and the comment where the result is built says so.\n\nI walked each row of `TestMCPServerInitializeInvalidParams` through the handler to make sure every rejection comes from the check it's meant to test:\n- **Missing params, `null` params, `{}`, an empty version and a `null` version** all decode to an empty `ProtocolVersion`. They hit the new nonempty check and get `\"Invalid params\"`.\n- **Array or string params, a numeric version, array `capabilities`, and string `clientInfo`** all cause a `json.Unmarshal` type error. That goes down the existing `-32602 \"Invalid params\"` path.\n\nSo the suite covers both the type-checking rejections and the new empty-string check. The negotiation test sends real wire input. It checks the returned revision and the `initialized` flag across matching, newer, older and arbitrary version strings, which is the behaviour that matters here.\n\nChanging the old `\"wrong protocol version\"` case to `\"missing protocol version\"` is the right call. A test asserting the old rejection would now be asserting behaviour that breaks the spec. The new case still covers the remaining `-32602` path in `handleInitialize`. No other test or doc in the repo depended on the old rejection.\n\nWhy I'm recommending approval: the change is one small, focused handler edit. It matches the protocol's documented negotiation rules. It only touches `initialize`, and both the success and rejection paths have wire-level tests that I traced by hand. I don't see anything a human reviewer would need to weigh in on beyond that. As the description says, Portal will still need its pinned dependency bumped after this merges.",
"posted_to_pr": true,
"draft": true,
"head_sha": "2c18f3c616d1dc4af4608d450d34781658223f1f"
}