Repository navigation
fix(canary): initialize MCP session before tools/call - #107
Conversation
The MCP Streamable HTTP transport requires a session initialization
handshake (method: initialize) before any tools/call request. Without
it the server returns HTTP 404 on every probe step.
Adds initMCPSession which POSTs the initialize request, reads the
Mcp-Session-Id response header, and returns it to the caller. The
session ID is then forwarded via Mcp-Session-Id on every subsequent
probeMCPTool call.
Also adds a mcp_init probe step so session-initialization failures are
surfaced in mctl_telegram_canary_step_failure_total{step=mcp_init}
rather than appearing as an unexplained list_dialogs failure.
Tests: three new unit tests for initMCPSession; integration test
newFakeServer updated to handle initialize automatically.
|
@claude review |
|
Claude finished @mashkovd's task in 1m 37s —— View job Review complete
SummaryThe fix is correct. The root cause (missing MCP initialize handshake) is properly addressed, the new P1: 0 | P2: 0 | P3: 3
No P1/P2 findings, good to merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return "", errors.New("server did not return Mcp-Session-Id header") | ||
| } | ||
|
|
||
| return sessionID, nil |
There was a problem hiding this comment.
P3 — The response body is never drained before defer resp.Body.Close() fires (including on both early-return error paths). net/http only reuses keep-alive connections when the body is fully consumed; skipping the drain forces a new TCP connection for every subsequent request in the same run. For a canary making ~3 requests this has no functional impact, but it's inconsistent with probeMCPTool (which calls io.ReadAll).
| return sessionID, nil | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| return sessionID, nil | |
| } |
Add _, _ = io.Copy(io.Discard, resp.Body) before the final return sessionID, nil, and a similar drain before each early return — or restructure to always drain once after the status check.
| return | ||
| } | ||
| // Re-inject the already-read body for the delegate handler. | ||
| r.Body = io.NopCloser(strings.NewReader(string(body))) |
There was a problem hiding this comment.
P3 — string(body) copies the byte slice unnecessarily; bytes.NewReader(body) avoids the allocation and is more idiomatic here.
| r.Body = io.NopCloser(strings.NewReader(string(body))) | |
| r.Body = io.NopCloser(bytes.NewReader(body)) |
| "result": map[string]any{"protocolVersion": "2024-11-05"}, | ||
| }) | ||
| })) | ||
| defer srv.Close() |
There was a problem hiding this comment.
P3 — The handler accepts any POST without verifying the request shape (method field, Authorization header, Content-Type). If the request body format regresses the test still passes as long as the server returns the header. A quick assertion on the method field and auth header would make the test catch more regressions at no real cost.
if r.Header.Get("Authorization") != "Bearer token" {
http.Error(w, "unauthorized", http.StatusUnauthorized)
return
}
var body jsonRPCRequest
_ = json.NewDecoder(r.Body).Decode(&body)
if body.Method != "initialize" {
http.Error(w, "unexpected method", http.StatusBadRequest)
return
}
|
Claude finished @mashkovd's task in 2m 29s —— View job Review complete
SummaryThe fix remains correct — the MCP initialize handshake is properly sequenced, The three P3 findings from the previous review pass are still present and unaddressed. P1: 0 | P2: 0 | P3: 3
No P1/P2 findings, good to merge. |
- drain response body in initMCPSession on all return paths to enable TCP connection reuse (consistent with probeMCPTool) - use bytes.NewReader instead of strings.NewReader in newFakeServer body re-injection (avoids string copy) - assert Authorization header and method field in TestInitMCPSessionSuccess so request format regressions are caught
|
Claude finished @mashkovd's task in 1m 41s —— View job Review complete
SummaryThe follow-up commit ("fix(canary): address P3 review findings") resolves all three findings from the prior review:
P1: 0 | P2: 0 | P3: 0 No new issues introduced. No P1/P2 findings, good to merge. |
| if sessionID == "" { | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
| return "", errors.New("server did not return Mcp-Session-Id header") | ||
| } |
There was a problem hiding this comment.
P3 — Response body is not drained before return sessionID, nil, and also not drained on the early-return error path at line 260. net/http only reuses keep-alive connections when the body is fully consumed; skipping the drain forces a new TCP connection on every subsequent call. Inconsistent with probeMCPTool (which calls io.ReadAll).
| } | |
| _, _ = io.Copy(io.Discard, resp.Body) | |
| return sessionID, nil | |
| } |
Also add a similar drain (or restructure) before each early return "", … after defer resp.Body.Close().
| Method string `json:"method"` | ||
| } | ||
| body, _ := io.ReadAll(io.LimitReader(r.Body, 1<<16)) | ||
| _ = json.Unmarshal(body, &req) |
There was a problem hiding this comment.
P3 — string(body) copies the byte slice into a new string just to wrap it in a strings.NewReader; bytes.NewReader avoids the allocation and is idiomatic here.
| _ = json.Unmarshal(body, &req) | |
| r.Body = io.NopCloser(bytes.NewReader(body)) |
| return | ||
| } | ||
| var req jsonRPCRequest | ||
| _ = json.NewDecoder(r.Body).Decode(&req) |
There was a problem hiding this comment.
P3 — The handler accepts any POST without verifying the request shape (method field, auth header, Content-Type). If the request body format or auth header regresses, the test still passes as long as the server returns the session-id header. A short assertion block would catch more regressions at no real cost:
if r.Header.Get("Authorization") != "Bearer token" {
http.Error(w, "unauthorized", http.StatusUnauthorized)
return
}
var body jsonRPCRequest
_ = json.NewDecoder(r.Body).Decode(&body)
if body.Method != "initialize" {
http.Error(w, "unexpected method", http.StatusBadRequest)
return
}
Summary
tools/callwithout the MCP initialize handshake, causing HTTP 404 on every probe stepinitMCPSessionthat POSTs{"method":"initialize"}, reads theMcp-Session-Idresponse header, and passes it to each subsequentprobeMCPToolcall via theMcp-Session-Idrequest headermcp_initas an explicit probe step — session failures now appear inmctl_telegram_canary_step_failure_total{step=mcp_init}rather than as unexplainedlist_dialogsfailuresinitMCPSession; integration test fake server updated to handleinitializeautomaticallyTest plan
go test ./cmd/canary/...passesmctl_telegram_canary_successgauge = 1 in Pushgateway🤖 Generated with Claude Code