Restore session-stable passthrough-header forwarding on the Serve path

[tgrunnagle]

Summary

The vMCP Serve path (introduced in #5556) routes all tool and resource calls through a shared backend client rather than a per-session connection. This meant passthrough headers were read per-request from the live incoming context, so a mid-session header change would reach the backend — a behavioral regression versus the legacy server.New path and a security concern when the forwarded headers are credentials (e.g. API keys).

This PR restores session-stable passthrough-header forwarding on the Serve path by capturing the allowlisted headers once at session creation and re-injecting that snapshot into every subsequent backend call for that session's lifetime. The captured state is held node-local in a sync.Map and is never written to Redis or any shared state store.

Closes #5560

• Capture at session creation: handleSessionRegistrationImpl reads headerforward.ForwardedHeadersFromContext once and stores the result in Server.capturedPassthroughHeaders (keyed by session ID).
• Inject before each backend call: coreToolHandler and coreResourceHandler call the new injectCapturedHeaders helper before delegating to core.CallTool / core.ReadResource, replacing the per-request context value with the session-stable snapshot.
• Merge in the shared client: pkg/vmcp/client now calls headerforward.MergeForwardedHeaders to combine the session-captured headers with the static per-backend HeaderForwardConfig before building the transport chain.
• MergeForwardedHeaders promoted: The merge logic that previously lived only in pkg/vmcp/session/internal/backend is now the exported headerforward.MergeForwardedHeaders, shared by both the legacy per-session connector and the new shared client path.
• Cleanup via OnUnregisterSession hook: A new SDK hook deletes the node-local entry when a session is unregistered (both explicit client DELETE and idle-TTL expiry).
• Telemetry wired onto the Serve path: core.New now wraps the workflow engine with OTEL metrics (toolhive_vmcp_workflow_*) when a TelemetryProvider is present, matching the instrumentation already present on the session-factory path.
• Integration test helper migrated: test/integration/vmcp/helpers/vmcp_server.go is updated to use core.New + Serve (the production path) instead of the legacy server.New, enabling TestVMCPServer_PassthroughHeaders to exercise the session-stable guarantee.

Type of change

• Bug fix

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

New unit tests in pkg/vmcp/headerforward/transport_test.go cover MergeForwardedHeaders: empty-forwarded no-op, static-only, forwarded-only, merged output, header canonicalization, restricted-name silently dropped, and collision detection. The TestVMCPServer_PassthroughHeaders integration test (previously skipped with a reference to this issue) is re-enabled.

Changes

File	Change
pkg/vmcp/headerforward/transport.go	Export MergeForwardedHeaders (moved from mcp_session.go)
pkg/vmcp/headerforward/transport_test.go	New unit tests for MergeForwardedHeaders
pkg/vmcp/server/server.go	Add capturedPassthroughHeaders sync.Map field; capture headers in handleSessionRegistrationImpl
pkg/vmcp/server/serve.go	Register OnUnregisterSession hook to clean up node-local header state
pkg/vmcp/server/serve_handlers.go	Call injectCapturedHeaders in coreToolHandler and coreResourceHandler; add injectCapturedHeaders helper
pkg/vmcp/client/client.go	Call MergeForwardedHeaders before building transport chain
pkg/vmcp/core/core_vmcp.go	Wire workflow telemetry onto the Serve path via telemetryComposer
pkg/vmcp/core/core_telemetry.go	New file: telemetryComposer and workflowInstruments
pkg/vmcp/session/internal/backend/mcp_session.go	Replace local mergeForwardedHeaders body with a call to headerforward.MergeForwardedHeaders
pkg/vmcp/session/internal/backend/mcp_session_test.go	Remove duplicated merge tests (now live in transport_test.go)
test/integration/vmcp/helpers/vmcp_server.go	Migrate test helper from server.New to core.New + Serve

Does this introduce a user-facing change?

No. The fix restores behavior that was already guaranteed by the legacy server.New path. From a user's perspective, passthrough headers (e.g. X-Api-Key) forwarded to backends continue to work, and the security property that a mid-session header change does not reach the backend is preserved.

Special notes for reviewers

Cross-pod behavior: A session restored on another pod will have no captured headers (the sync.Map is node-local). This matches the legacy path's behavior — the legacy per-session connection also could not be serialized across pods. Persisting the captured headers to Redis would require storing credentials in shared state, which is explicitly forbidden by the security constraint in .claude/rules/security.md.

Abandoned sessions (client crashes, sessionIdleTTL=0): Their entries persist in capturedPassthroughHeaders until server shutdown. This is acceptable because the map is bounded by the number of live sessions and each entry is small. The SDK's idle-TTL sweeper fires OnUnregisterSession for expired sessions when sessionIdleTTL > 0.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 90.76923% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.95%. Comparing base (4470503) to head (4ae5e52).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/vmcp/core/core_telemetry.go	88.46%	3 Missing and 3 partials ⚠️
pkg/vmcp/core/core_vmcp.go	62.50%	2 Missing and 1 partial ⚠️
pkg/vmcp/client/client.go	66.66%	1 Missing and 1 partial ⚠️
pkg/vmcp/server/server.go	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5561      +/-   ##
==========================================
- Coverage   70.05%   69.95%   -0.11%     
==========================================
  Files         650      652       +2     
  Lines       66167    66445     +278     
==========================================
+ Hits        46352    46479     +127     
- Misses      16459    16594     +135     
- Partials     3356     3372      +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: security-reviewer, concurrency-reviewer, architecture-reviewer, test-coverage-reviewer, general-quality-reviewer

Consensus Summary

#	Finding	Consensus	Severity	Action
F1	Defensive copy missing for captured headers	8/10	MEDIUM	Fix
F2	Anti-pattern #1: context value used for domain data	8/10	MEDIUM	Fix (add comment)
F3	Double-delete of session header entry undocumented	8/10	LOW	Fix (add comment)
F4	"Base not mutated" test assertion missing	8/10	LOW	Fix
F5	Static key canonicalization change undocumented	7/10	LOW	Fix (add comment)
F6	No unit tests for injectCapturedHeaders	6/10	LOW	Fix
F7	No unit tests for OnUnregisterSession cleanup hook	6/10	LOW	Fix
F8	No unit tests for telemetryComposer	6/10	LOW	Fix

Overall

This PR correctly fixes a security-relevant behavioral regression: the core.New + Serve path introduced in #5556 used a shared backend client that re-read forwarded headers per-request, breaking the session-stable guarantee that the legacy server.New path provided. The fix is targeted and sound — headers are captured once at session creation into a node-local sync.Map, injected before every backend call via injectCapturedHeaders, and cleaned up on session teardown. The PR also promotes the header-merge logic to a shared exported function (MergeForwardedHeaders), wires workflow telemetry onto the Serve path consistently with the session-factory path, and migrates the integration test helper to exercise the production code path. The re-enabled TestVMCPServer_PassthroughHeaders integration test directly verifies the acceptance criteria from #5560.

The two MEDIUM findings (F1, F2) are worth addressing but do not block merge — F1 is not a current bug and the fix is a one-line maps.Clone; F2 is a documentation comment. The LOW findings are unit test gaps for new production code (F6–F8) and documentation clarifications (F3–F5) that would improve defensibility of a credential-handling path.

---

Generated with Claude Code

[github-actions]

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 transformation

Alternative:

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.

[jerm-dro]

I think this bundles a real fix with a re-implementation of incidental legacy behavior — flagging the latter before it lands.

Real fix: on the Serve path, defaultClientFactory built the tripper from target.HeaderForward only and never read ForwardedHeadersFromContext(ctx) — so passthrough headers were dropped entirely. The MergeForwardedHeaders call fixes that, and since the factory runs per-call, it forwards them live on every request.

The rest isn't needed: the capture/sync.Map/cleanup machinery exists to freeze the first request's header value for the session. But "session-stable" was a side effect of the legacy persistent connection, not a designed guarantee — #5466 describes the feature as per-request, and session-stability simply isn't a requirement.

Proposal: keep the client.go merge, drop the capture machinery. TestVMCPServer_PassthroughHeaders encodes the freeze, so rewrite it to assert per-request forwarding.