Add regression tests gating the mcp-go to go-sdk migration#5772
Merged
Conversation
Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.
Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.
Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
a bare {type:object}, dropping properties and required fields
(toolhive-core#156 U6)
Refs #5742
Pin current mcpcompat shim behavior for auth error classification and local proxy Host-header rewriting. Add configurable HeartbeatInterval to ServerConfig for testable SSE keep-alive. Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to legacy SSE fallback, tool errors with auth-looking messages not being misclassified as auth failures, and Host-header rewriting in the transparent proxy. Key finding: the streamable-HTTP transport does not return transport.ErrLegacySSEServer for 403 responses (only the SSE transport does), so the sentinel is not in the error chain. The error is still classified as ErrBackendUnavailable via string matching. Pinned with documented assertions. Refs #5742
Pin current mcpcompat shim behavior for SSE keep-alive cadence, tool pagination, per-session tool projection with Cedar authz, and sliding session TTL so the go-sdk migration can be validated. Tests cover keep-alive bytes on idle SSE streams, >1000 tool listing behavior (skipped — known pagination gap), two sessions with different authz outcomes seeing different tools, filtered tools being uncallable, session tool sets being fixed at initialize, and active sessions not expiring despite TTL elapsing. Add WithSessionTTL helper option for integration tests. Refs #5742
Pin the current mcpcompat shim behavior for the stdio bridge's list_changed re-fetch fix and document known shim limitations. Test covers the forwardAll re-sync when a tools/list_changed notification arrives from the upstream backend, verifying the bridge's local server picks up the updated tool set. Skipped test documents that progress and logging notifications are dropped by the mcpcompat client shim (no ProgressNotificationHandler or LoggingMessageHandler installed) and that list_changed notifications cannot be forwarded downstream (SendNotificationToAllClients only handles progress and message). Serves as regression anchor for when the shim is fixed. Refs #5742
JAORMX
requested review from
ChrisJBurns,
amirejaz,
blkt,
jerm-dro,
jhrozek,
rdimitrov and
tgrunnagle
as code owners
July 9, 2026 15:13
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/remove-mcp-go #5772 +/- ##
======================================================
+ Coverage 70.77% 70.84% +0.07%
======================================================
Files 683 683
Lines 69194 69208 +14
======================================================
+ Hits 48970 49030 +60
+ Misses 16625 16591 -34
+ Partials 3599 3587 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Open
4 tasks
Contributor
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
github-actions
Bot
dismissed
their stale review
July 13, 2026 06:52
Large PR justification has been provided. Thank you!
Codespell flagged rIn as a potential misspelling (ring/rink/rain). Rename to pipeR/pipeW to avoid the false positive. Refs #5742
github-actions
Bot
added
size/XL
rdimitrov
approved these changes
Jul 13, 2026
JAORMX
added a commit
that referenced
this pull request
Jul 16, 2026
* Add Serve-path regression tests and bridge list_changed re-fetch
Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.
Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.
Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
a bare {type:object}, dropping properties and required fields
(toolhive-core#156 U6)
Refs #5742
* Add auth-error mapping, Host-header, and HeartbeatInterval config
Pin current mcpcompat shim behavior for auth error classification
and local proxy Host-header rewriting. Add configurable
HeartbeatInterval to ServerConfig for testable SSE keep-alive.
Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to
legacy SSE fallback, tool errors with auth-looking messages not
being misclassified as auth failures, and Host-header rewriting in
the transparent proxy.
Key finding: the streamable-HTTP transport does not return
transport.ErrLegacySSEServer for 403 responses (only the SSE
transport does), so the sentinel is not in the error chain. The
error is still classified as ErrBackendUnavailable via string
matching. Pinned with documented assertions.
Refs #5742
* Add SSE keep-alive, pagination, projection, and sliding TTL tests
Pin current mcpcompat shim behavior for SSE keep-alive cadence,
tool pagination, per-session tool projection with Cedar authz,
and sliding session TTL so the go-sdk migration can be validated.
Tests cover keep-alive bytes on idle SSE streams, >1000 tool
listing behavior (skipped — known pagination gap), two sessions
with different authz outcomes seeing different tools, filtered
tools being uncallable, session tool sets being fixed at
initialize, and active sessions not expiring despite TTL elapsing.
Add WithSessionTTL helper option for integration tests.
Refs #5742
* Add stdio bridge notification forwarding regression tests
Pin the current mcpcompat shim behavior for the stdio bridge's
list_changed re-fetch fix and document known shim limitations.
Test covers the forwardAll re-sync when a tools/list_changed
notification arrives from the upstream backend, verifying the
bridge's local server picks up the updated tool set.
Skipped test documents that progress and logging notifications are
dropped by the mcpcompat client shim (no ProgressNotificationHandler
or LoggingMessageHandler installed) and that list_changed
notifications cannot be forwarded downstream (SendNotificationToAllClients
only handles progress and message). Serves as regression anchor
for when the shim is fixed.
Refs #5742
* Fix codespell: rename rIn/wIn to pipeR/pipeW in bridge test
Codespell flagged rIn as a potential misspelling (ring/rink/rain).
Rename to pipeR/pipeW to avoid the false positive.
Refs #5742
JAORMX
added a commit
that referenced
this pull request
Jul 16, 2026
Rebasing onto main advanced toolhive-core to a release where three mcpcompat behaviors the #5772 gating tests pinned have changed: - Schema fidelity: a tool schema without a top-level "type":"object" is now projected intact (properties/required preserved, type supplied) rather than normalized to the empty object schema. - initialize capabilities: tools and resources are now advertised in the initialize response alongside logging. - SSE keep-alive: an idle GET stream now emits periodic keep-alive comments; the test asserts the configured heartbeat by passing WithHeartbeatInterval to the streamable server it builds. Flip the first two per their own documented instructions and wire the heartbeat interval into the keep-alive test so it asserts the real keep-alive rather than an incidental notification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe
JAORMX
added a commit
that referenced
this pull request
Jul 16, 2026
* Add Serve-path regression tests and bridge list_changed re-fetch
Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.
Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.
Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
a bare {type:object}, dropping properties and required fields
(toolhive-core#156 U6)
Refs #5742
* Add auth-error mapping, Host-header, and HeartbeatInterval config
Pin current mcpcompat shim behavior for auth error classification
and local proxy Host-header rewriting. Add configurable
HeartbeatInterval to ServerConfig for testable SSE keep-alive.
Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to
legacy SSE fallback, tool errors with auth-looking messages not
being misclassified as auth failures, and Host-header rewriting in
the transparent proxy.
Key finding: the streamable-HTTP transport does not return
transport.ErrLegacySSEServer for 403 responses (only the SSE
transport does), so the sentinel is not in the error chain. The
error is still classified as ErrBackendUnavailable via string
matching. Pinned with documented assertions.
Refs #5742
* Add SSE keep-alive, pagination, projection, and sliding TTL tests
Pin current mcpcompat shim behavior for SSE keep-alive cadence,
tool pagination, per-session tool projection with Cedar authz,
and sliding session TTL so the go-sdk migration can be validated.
Tests cover keep-alive bytes on idle SSE streams, >1000 tool
listing behavior (skipped — known pagination gap), two sessions
with different authz outcomes seeing different tools, filtered
tools being uncallable, session tool sets being fixed at
initialize, and active sessions not expiring despite TTL elapsing.
Add WithSessionTTL helper option for integration tests.
Refs #5742
* Add stdio bridge notification forwarding regression tests
Pin the current mcpcompat shim behavior for the stdio bridge's
list_changed re-fetch fix and document known shim limitations.
Test covers the forwardAll re-sync when a tools/list_changed
notification arrives from the upstream backend, verifying the
bridge's local server picks up the updated tool set.
Skipped test documents that progress and logging notifications are
dropped by the mcpcompat client shim (no ProgressNotificationHandler
or LoggingMessageHandler installed) and that list_changed
notifications cannot be forwarded downstream (SendNotificationToAllClients
only handles progress and message). Serves as regression anchor
for when the shim is fixed.
Refs #5742
* Fix codespell: rename rIn/wIn to pipeR/pipeW in bridge test
Codespell flagged rIn as a potential misspelling (ring/rink/rain).
Rename to pipeR/pipeW to avoid the false positive.
Refs #5742
JAORMX
added a commit
that referenced
this pull request
Jul 16, 2026
Rebasing onto main advanced toolhive-core to a release where three mcpcompat behaviors the #5772 gating tests pinned have changed: - Schema fidelity: a tool schema without a top-level "type":"object" is now projected intact (properties/required preserved, type supplied) rather than normalized to the empty object schema. - initialize capabilities: tools and resources are now advertised in the initialize response alongside logging. - SSE keep-alive: an idle GET stream now emits periodic keep-alive comments; the test asserts the configured heartbeat by passing WithHeartbeatInterval to the streamable server it builds. Flip the first two per their own documented instructions and wire the heartbeat interval into the keep-alive test so it asserts the real keep-alive rather than an incidental notification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe
JAORMX
added a commit
that referenced
this pull request
Jul 17, 2026
…k) (#5729) * feat: migrate from mark3labs/mcp-go to toolhive-core/mcpcompat Swaps all github.com/mark3labs/mcp-go/{mcp,server,client,client/transport} imports to the go-sdk-backed compatibility shim in toolhive-core (github.com/stacklok/toolhive-core/mcpcompat/*). No call-site logic changes — the shim mirrors mcp-go's API. This moves ToolHive's MCP protocol handling onto the official modelcontextprotocol/go-sdk. Depends on stacklok/toolhive-core#150; the toolhive-core dependency is pinned to that branch commit and should be bumped to the released version once #150 merges. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: bump toolhive-core shim + fix gci import grouping Bump github.com/stacklok/toolhive-core to the mcpcompat shim commit that closes the server/client compatibility gaps exercised by the vMCP and MCP test suites (pulls the required cel-go v0.29.1). Also fix gci import grouping in three files flagged by lint (toolhive-core shares the github.com/stacklok/toolhive prefix, so its imports group with toolhive's). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style: fix gci import grouping after mcp-go->mcpcompat swap Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Add regression tests gating the mcp-go to go-sdk migration (#5772) * Add Serve-path regression tests and bridge list_changed re-fetch Pin current mcpcompat shim behavior for session lifecycle, capability advertisement, tool schema fidelity, and concurrent context isolation so the go-sdk migration can be validated. Fix bridge to re-fetch tools/resources/prompts when a list_changed notification arrives, keeping the local stdio server's advertised set in sync with the upstream backend. Two real regressions found and pinned with documented assertions: - Initialize response advertises only {logging:{}} instead of capabilities.tools and capabilities.resources (toolhive-core#156 U1) - normalizeObjectSchema replaces schemas without type:object with a bare {type:object}, dropping properties and required fields (toolhive-core#156 U6) Refs #5742 * Add auth-error mapping, Host-header, and HeartbeatInterval config Pin current mcpcompat shim behavior for auth error classification and local proxy Host-header rewriting. Add configurable HeartbeatInterval to ServerConfig for testable SSE keep-alive. Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to legacy SSE fallback, tool errors with auth-looking messages not being misclassified as auth failures, and Host-header rewriting in the transparent proxy. Key finding: the streamable-HTTP transport does not return transport.ErrLegacySSEServer for 403 responses (only the SSE transport does), so the sentinel is not in the error chain. The error is still classified as ErrBackendUnavailable via string matching. Pinned with documented assertions. Refs #5742 * Add SSE keep-alive, pagination, projection, and sliding TTL tests Pin current mcpcompat shim behavior for SSE keep-alive cadence, tool pagination, per-session tool projection with Cedar authz, and sliding session TTL so the go-sdk migration can be validated. Tests cover keep-alive bytes on idle SSE streams, >1000 tool listing behavior (skipped — known pagination gap), two sessions with different authz outcomes seeing different tools, filtered tools being uncallable, session tool sets being fixed at initialize, and active sessions not expiring despite TTL elapsing. Add WithSessionTTL helper option for integration tests. Refs #5742 * Add stdio bridge notification forwarding regression tests Pin the current mcpcompat shim behavior for the stdio bridge's list_changed re-fetch fix and document known shim limitations. Test covers the forwardAll re-sync when a tools/list_changed notification arrives from the upstream backend, verifying the bridge's local server picks up the updated tool set. Skipped test documents that progress and logging notifications are dropped by the mcpcompat client shim (no ProgressNotificationHandler or LoggingMessageHandler installed) and that list_changed notifications cannot be forwarded downstream (SendNotificationToAllClients only handles progress and message). Serves as regression anchor for when the shim is fixed. Refs #5742 * Fix codespell: rename rIn/wIn to pipeR/pipeW in bridge test Codespell flagged rIn as a potential misspelling (ring/rink/rain). Rename to pipeR/pipeW to avoid the false positive. Refs #5742 * Update migration gating tests for new toolhive-core behavior Rebasing onto main advanced toolhive-core to a release where three mcpcompat behaviors the #5772 gating tests pinned have changed: - Schema fidelity: a tool schema without a top-level "type":"object" is now projected intact (properties/required preserved, type supplied) rather than normalized to the empty object schema. - initialize capabilities: tools and resources are now advertised in the initialize response alongside logging. - SSE keep-alive: an idle GET stream now emits periodic keep-alive comments; the test asserts the configured heartbeat by passing WithHeartbeatInterval to the streamable server it builds. Flip the first two per their own documented instructions and wire the heartbeat interval into the keep-alive test so it asserts the real keep-alive rather than an incidental notification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe * Bump toolhive-core to v0.0.29 Pick up the CallGate seam and the passive SSE keep-alive so the mcpcompat migration includes the keep-alive fix the gating test asserts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe * Reject a terminated vMCP session with 404, not 503 Under the mcpcompat transport, a request for a session that was terminated on another replica came back HTTP 503 instead of 404, so the client retried the dead session forever instead of re-initializing — a cross-replica lazy-eviction regression surfaced by the SDK migration (mcp-go answered a blanket 404 on any validation failure). Manager.Terminate deletes the storage key (unchanged — deliberate, to avoid the Phase 1->2 resurrection race). The bug was that Manager.Validate then reported the absent key as (isTerminated=false, error), which the transport maps to a retryable 503. A genuinely-gone session (deleted, TTL-expired, or never existed) is not transient: report it as (isTerminated=true, nil) so the transport answers a definitive 404 and the client re-initializes. A real, transient storage error still returns (false, error) -> 503, so a store blip does not drop live session state. Terminate is untouched, so the resurrection-race guarantee is preserved; only the way an absent key is reported by Validate changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe * Verify shim forwards progress/logging notifications Replace a stale skipped regression anchor that claimed the mcpcompat client dropped upstream progress/logging notifications. The client wires both the go-sdk ProgressNotificationHandler and LoggingMessageHandler (toolhive-core issue #156), and the bridge relays every notification downstream, so the premise no longer holds. Add a real test driving a live backend: notifications/progress forwards to the client's OnNotification handler unconditionally, and notifications/message forwards once the client subscribes via SetLoggingLevel (level-gated per the MCP spec, matching mcp-go). The test fails if either handler regresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe * Address review: strengthen migration tests, fix stale comments Reviewer feedback on the mcp-go -> mcpcompat migration: Test quality (false-green gates): - sse_keepalive: serve srv.Handler(ctx) through httptest so the configured interval flows Config -> Handler -> WithHeartbeatInterval; the prior form built its own server and never gated that wiring. - sliding_ttl: active half drives tools/call (through enforceSessionBinding -> storage.Load) instead of tools/list, which returns the cached set even after eviction and couldn't distinguish a sliding TTL from a fixed one. - host_header: assert tracingTransport.RoundTrip directly with a request whose Host differs from URL.Host, so the test fails if the Host-rewrite is removed. - context_isolation: collect goroutine results and assert after wg.Wait() (require/FailNow is unsafe off the test goroutine); rename the second block to what it actually asserts. Coverage: - auth_error_mapping: pin wrapBackendError's 5xx, timeout, EOF/refused, and transport.ErrAuthorizationRequired -> ErrAuthenticationFailed branches (the migration-sensitive sentinel). Hygiene: - bridge.go: soften the forwardAll comment (additive-only; does not prune capabilities removed upstream). - server.go: heartbeat doc says "zero or negative" (matches the d<=0 guard); note HeartbeatInterval is plumbed ahead of CLI/CRD wiring. - Update stale "mark3labs" doc comments to "mcpcompat". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a comprehensive regression test suite that pins the current
mcpcompatshim behavior so the mcp-go → go-sdk migration (#5729) can be validated against it. The tests are written againsttoolhive-core/mcpcompatimports (notmark3labs/mcp-go) so they compile on this branch and survive the import swap.Closes #5742.
Large PR Justification
This PR is ~2000 lines, almost entirely test code (18 files changed, only 4 contain production changes totaling ~35 lines). Per the repo's Contributing Guidelines and
CLAUDE.md, test-only changes are exempt from the 400-line/10-file limit.The tests cannot be meaningfully split because they form a single coherent regression gate for issue #5742's 14-group test matrix — they must land together so the migration PR (#5729) can be validated against the complete set. Splitting into per-group PRs would delay the migration gate and create partial coverage in the meantime.
The 4 production changes (bridge list_changed re-fetch ~12 lines, HeartbeatInterval config ~20 lines, WithSessionTTL helper ~13 lines, derive/serve field threading ~3 lines) are minimal and directly enable the tests.
What's tested
pkg/transport/bridge_test.gopkg/transport/bridge_test.gopkg/transport/bridge_test.gopkg/vmcp/server/session_lifecycle_regression_test.gotest/integration/vmcp/per_session_projection_regression_test.gopkg/vmcp/server/sse_keepalive_regression_test.gotest/integration/vmcp/pagination_regression_test.gopkg/transport/proxy/transparent/host_header_regression_test.gopkg/vmcp/client/auth_error_mapping_regression_test.gopkg/vmcp/server/capability_regression_test.go{logging:{}}advertised (#5769)pkg/vmcp/server/context_isolation_regression_test.gopkg/vmcp/server/schema_fidelity_regression_test.gotype:objectare normalized to{"type":"object"}(#5770)pkg/vmcp/client/auth_error_mapping_regression_test.gotest/integration/vmcp/sliding_ttl_regression_test.goCodebase changes
pkg/transport/bridge.go): Re-fetch tools/resources/prompts when a*_list_changednotification arrives (previously only fetched once at startup)pkg/vmcp/server/server.go,serve.go,derive.go): Make SSE keep-alive interval configurable for testing (defaults to 30s in production, 100ms in tests)test/integration/vmcp/helpers/vmcp_server.go): Allow integration tests to configure short session TTLsReal regressions found and documented
{logging:{}}instead ofcapabilities.toolsandcapabilities.resources(CRITICAL — breaks spec-compliant clients)type:objectis absentVerification
Notes
github.com/stacklok/toolhive-core/mcpcompat/*— no directmark3labs/mcp-goimportsencoding/json) and toolhive's own types (pkg/vmcp) where possible to minimize SDK couplingRelated