test(vmcp): cover client response posts - #5885
Conversation
Signed-off-by: King Star <mcxin.y@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5885 +/- ##
==========================================
- Coverage 71.48% 71.45% -0.03%
==========================================
Files 694 694
Lines 70971 70971
==========================================
- Hits 50735 50714 -21
- Misses 16577 16598 +21
Partials 3659 3659 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ChrisJBurns
left a comment
There was a problem hiding this comment.
Test-only PR with genuine regression value for the client-response → 202 path. I applied it to current main and it passes under -race. A few small things would make it clearer/stronger:
Scope of Closes #5009: the issue's "where to fix" points at pkg/transport/proxy/streamable/ and its impact section mentions non-vMCP proxy pings too. This test only covers the vMCP server path. Can you confirm the general thv run / thv proxy streamable transport also returns 202 for client-response POSTs? If not, this closes only half of #5009 — worth either adding coverage there or noting the remaining gap on the issue before it's auto-closed.
Housekeeping: the branch is ~84 commits behind main and CI hasn't reported. Mind rebasing so the parity/lint suites run green before merge? (It still applies cleanly and passes locally, so no real conflict expected.)
| // these two layers together. The request-only authz layer treated a JSON-RPC | ||
| // response as malformed because the parser intentionally returned no request. | ||
| srv.config.AuthMiddleware = mcpparser.ParsingMiddleware | ||
| srv.config.AuthzMiddleware = legacyRequestAuthorization |
There was a problem hiding this comment.
This is inert by construction, not an active gate. The production Handler (server.go:634-637) no longer reads config.AuthzMiddleware at all — the whole HTTP authz block was deleted in #5556, not just guarded on nil. So this assignment does nothing today; the assertion passes because the field is ignored. That's still a useful canary (it would fail if someone re-added an if config.AuthzMiddleware != nil { apply } block to Handler), but the preceding comment frames it as if the layer is live in the chain. Suggest making the intent explicit, e.g.:
// Regression canary: Handler no longer reads config.AuthzMiddleware (the HTTP
// authz block was removed in #5556). Setting it here proves that re-introducing
// an `if config.AuthzMiddleware != nil { apply }` block would break response POSTs.| } | ||
| } | ||
|
|
||
| func TestHandler_AcceptsClientResponsePostWithLegacyAuthzConfigured(t *testing.T) { |
There was a problem hiding this comment.
The name ...WithLegacyAuthzConfigured reads as if the legacy middleware is live in the request chain, but it's structurally unreachable (see comment below). Consider a name/doc that says it guards against re-adding the authz block rather than running "with … configured", or add a short top-of-function comment stating the two distinct things this proves: (1) a client-response POST returns 202, and (2) a configured AuthzMiddleware is not reinstalled by the Serve-path Handler.
| readErr: readErr, | ||
| } | ||
| select { | ||
| case t.observed <- observation: |
There was a problem hiding this comment.
Minor: the default silently drops the observation if this buffered (size 1) channel is already full. Fine for the single-response happy path, but if a retry ever POSTs a second response, the test could hang on <-observed waiting for one that was dropped. A one-line comment noting "only one response POST is expected; extras are intentionally discarded" would save a future debugger some time.
Summary
Valid JSON-RPC responses sent by Streamable HTTP clients used to pass through a
request-only authorization layer, which rejected them as malformed and broke
server-initiated MCP requests. The production vMCP handler no longer installs
that legacy layer after #5556, but the client-response path had no end-to-end
regression coverage.
This adds a production-handler integration test that opens a real Streamable
HTTP client, triggers server-initiated elicitation during a tool call, and sends
the correlated client response back through the
/mcpendpoint.Changes
The test verifies that:
202 Acceptedwith an empty body;back into the production handler chain.
The change is test-only and does not modify runtime behavior.
Verification
Closes #5009