Add regression pins for TTL, DNS-rebind, bridge, schema#5977
Merged
Conversation
Continues the #5742 acceptance gate for the mcp-go -> go-sdk migration (toolhive-core v0.0.32) with the remaining in-repo test gaps plus one doc fix. Each test pins behavior fixed under the migration so a future regression fails loudly here rather than only in toolhive-core. - Sliding-session TTL (§14): activity through Manager.Validate renews the shared-store TTL (GETEX), so a session kept alive by traffic does not expire; an idle session past the TTL is reported terminated. The negative pairs with the positive so the survival case can't pass vacuously. - DNS-rebinding default (§6): a loopback-bound vMCP server 403s a POST whose Host is non-localhost, with a loopback-Host positive control proving the request is otherwise valid. Pins go-sdk's localhost protection default, which the shim exposes but ToolHive leaves on. - stdio bridge resource/prompt list_changed (§1): a backend resources/list_changed or prompts/list_changed triggers the bridge's additive re-sync and the new capability is reflected on the bridge's server. Complements the existing tools/list_changed test. - Auth-error classifier boundary (§13): IsAuthenticationError matches the intended phrase patterns but not a bare "401"/"unauthorized" substring, so a backend tool error merely mentioning 401 is not misclassified. Consolidated into the classifier's owning package (removing a duplicate that lived in pkg/vmcp/health). - Tool-schema ingestion fidelity (§12): a skipped gate (blocked on #5976) that pins backend -> vMCP ingestion preserving top-level oneOf/anyOf and not fabricating "type":"". Flips green on the toolhive-core bump. - Correct the ErrLegacySSEServer comment: that arm fires only for the sse transport; a streamable 403-on-initialize maps to ErrBackendUnavailable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
JAORMX
requested review from
ChrisJBurns,
amirejaz,
blkt,
jerm-dro,
jhrozek,
rdimitrov and
tgrunnagle
as code owners
July 24, 2026 16:51
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5977 +/- ##
==========================================
+ Coverage 71.96% 71.97% +0.01%
==========================================
Files 717 717
Lines 73970 73970
==========================================
+ Hits 53233 53243 +10
+ Misses 16935 16924 -11
- Partials 3802 3803 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ChrisJBurns
approved these changes
Jul 24, 2026
3 tasks
JAORMX
added a commit
that referenced
this pull request
Jul 25, 2026
The §12 tool-schema ingestion fidelity fix landed upstream in stacklok/toolhive-core#186 (ToolArgumentsSchema now preserves top-level JSON Schema keywords like oneOf/anyOf via an Extra catch-all and stops fabricating an empty "type"). Bump toolhive-core to pick it up and unskip the in-repo gate added in #5977, closing the ToolHive half of #5976. The dependency is pinned to the exact #186 commit rather than to toolhive-core main HEAD: HEAD also carries #185 (adopt go-sdk v1.7.0-pre.3), a 2026-07-28 groundwork change tracked separately under #5754 and out of scope here. Re-pin to a tagged release (v0.0.34+) once one is cut that includes #186. - go.mod/go.sum: toolhive-core v0.0.32 -> #186 commit (plus minor transitive aws-sdk patch bumps). - Remove the t.Skip on TestRegression_ToolSchemaFidelity_PreservesCompositors; it now passes, pinning backend->vMCP ingestion of a top-level oneOf and the absence of a fabricated "type":"". Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79 Co-authored-by: Claude Opus 4.8 (1M context) <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
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
Part of #5743. Continues #5742 (the acceptance gate for the
mcp-go→go-sdkmigration, toolhive-core v0.0.32), following #5974.Several behaviors fixed under the migration are currently pinned only in
toolhive-core; this PR adds the remaining ToolHive-side regression tests so a badgo.modbump or a shim behavior change fails loudly here. It closes the lower-severity gaps from the #5742 audit and lands the in-repo half of the §12 defect as a skipped gate.Manager.Validaterenews the shared-store TTL (RedisGETEX), so a session kept alive by traffic does not expire; an idle session past its TTL is reported terminated. The negative case pairs with the positive so the survival test cannot pass vacuously.Hostis non-localhost, with a loopback-Hostpositive control proving the request is otherwise valid. Pins go-sdk's localhost protection (the shim exposes the toggle; ToolHive leaves protection on).list_changed(§1): a backendresources/list_changed/prompts/list_changedtriggers the bridge's additive re-sync and the new capability is reflected on the bridge's server. Complements the existingtools/list_changedtest.IsAuthenticationErrormatches its intended phrase patterns but not a bare401/unauthorizedsubstring, so a backend tool error merely mentioning401is not misclassified. Consolidated into the classifier's owning package (removed a duplicate that lived inpkg/vmcp/health, a Test-Scope fix).t.Skip, blocked on Backend tool-schema ingestion drops top-level oneOf/anyOf/enum #5976) that pins backend→vMCP ingestion preserving top-leveloneOf/anyOfand not fabricating"type":"". It flips green on the toolhive-core bump.ErrLegacySSEServerarm fires only for thessetransport; a streamable 403-on-initialize maps toErrBackendUnavailable. Comment-only.Type of change
pkg/vmcp/client/client.go.Test plan
task test)task lint-fix)All new tests pass under
-race(A8 is intentionally skipped pending #5976). Fulltask testis green except three pre-existing, environment-only failures unrelated to this change (no container runtime / no keyring in the sandbox), none in the touched packages.Changes
pkg/vmcp/server/sessionmanager/sliding_ttl_regression_test.gopkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.gonewSharedRedisStorageWithTTL(default preserved)pkg/vmcp/server/dns_rebinding_regression_test.gopkg/transport/bridge_test.golist_changedre-sync tests +resourceNamesOnServer/promptNamesOnServerhelpers (§1)pkg/vmcp/errors_test.goIsAuthenticationErrorboundary table (§13)pkg/vmcp/health/checker_test.gopkg/vmcp/client/schema_ingestion_regression_test.gopkg/vmcp/client/client.goErrLegacySSEServerfallback note (§7)Does this introduce a user-facing change?
No.
Special notes for reviewers
os.Stdin(process-global) and are therefore serialized (//nolint:paralleltest), mirroring the existingTestBridge_ToolsListChanged_TriggersReSyncdiscipline; the deliberate scope decision was to assert the re-sync on the bridge's server rather than swapos.Stdoutfor a downstream-delivery check (that would only exercise go-sdk framing, not our package).🤖 Generated with Claude Code