Forward passthrough headers on the Serve path

[tgrunnagle]

Summary

The vMCP `Serve` path (introduced in #5556) routes all tool and resource calls through a shared backend client (`pkg/vmcp/client`). That client built its outgoing transport from `target.HeaderForward` only and never read `ForwardedHeadersFromContext(ctx)`, so allowlisted passthrough headers (e.g. `X-Api-Key`) were silently dropped on every backend call — a regression from the legacy `server.New` path.

Closes #5560

• Promote `MergeForwardedHeaders`: The merge logic that previously existed only inside `pkg/vmcp/session/internal/backend` (legacy per-session connector) is now the exported `headerforward.MergeForwardedHeaders`. It combines the static per-backend `HeaderForwardConfig` with the per-request forwarded headers from `ForwardedHeadersFromContext(ctx)`, applying the same restricted-header checks and collision detection as the legacy path.
• Wire it into the shared client: `pkg/vmcp/client.defaultClientFactory` now calls `MergeForwardedHeaders` before building the transport chain. `CaptureMiddleware` puts the current request's allowlisted headers into the context; the client reads them per-call, so each backend request carries the latest forwarded-header value.
• Telemetry on the `Serve` path: `core.New` now wraps the workflow engine with OTEL metrics (`toolhive_vmcp_workflow_*`) when a `TelemetryProvider` is present, matching the instrumentation on the session-factory path.
• Integration test helper migrated: `test/integration/vmcp/helpers/vmcp_server.go` uses `core.New` + `Serve` (the production path) instead of the legacy `server.New`.

Type of change

• Bug fix

Test plan

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

`TestVMCPServer_PassthroughHeaders` is re-enabled and verifies end-to-end that an allowlisted header reaches the backend and a non-allowlisted header is dropped. The second call asserts that a mid-request header change is reflected on the next call (per-request forwarding). New unit tests cover `MergeForwardedHeaders` and `telemetryComposer`.

Changes

File	Change
`pkg/vmcp/headerforward/transport.go`	Export `MergeForwardedHeaders` (promoted from `mcp_session.go`)
`pkg/vmcp/headerforward/transport_test.go`	Unit tests for `MergeForwardedHeaders`
`pkg/vmcp/client/client.go`	Call `MergeForwardedHeaders` before building the transport chain
`pkg/vmcp/core/core_vmcp.go`	Wire workflow telemetry via `telemetryComposer` when `TelemetryProvider` is set
`pkg/vmcp/core/core_telemetry.go`	New: `telemetryComposer` wrapper + `workflowInstruments`
`pkg/vmcp/core/core_telemetry_test.go`	Unit tests for `telemetryComposer`
`pkg/vmcp/session/internal/backend/mcp_session.go`	Replace local `mergeForwardedHeaders` body with a call to the exported function
`pkg/vmcp/session/internal/backend/mcp_session_test.go`	Remove duplicated merge tests (now in `transport_test.go`)
`test/integration/vmcp/helpers/vmcp_server.go`	Migrate test helper from `server.New` to `core.New` + `Serve`
`test/integration/vmcp/passthrough_headers_test.go`	Re-enable; assert per-request forwarding

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 90.59829% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.93%. Comparing base (4470503) to head (e7adb86).
⚠️ Report is 9 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 ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5561      +/-   ##
==========================================
- Coverage   70.05%   69.93%   -0.13%     
==========================================
  Files         650      653       +3     
  Lines       66167    66489     +322     
==========================================
+ Hits        46352    46496     +144     
- Misses      16459    16636     +177     
- Partials     3356     3357       +1     

☔ 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.

[tgrunnagle]

Good catch — agreed. f08f3d0 drops the capturedPassthroughHeaders sync.Map, injectCapturedHeaders, and the AddOnUnregisterSession hook. The MergeForwardedHeaders call in client.go already reads ForwardedHeadersFromContext(ctx) per-call, so per-request forwarding is natural. Rewrote TestVMCPServer_PassthroughHeaders to assert the changed value reaches the backend on the second call.

PR size has been reduced below the XL threshold. Thank you for splitting this up!

[github-actions]

✅ PR size has been reduced below the XL threshold. The size review has been dismissed and this PR can now proceed with normal review. Thank you for splitting this up!

[jerm-dro]

blocker: The title and description still describe the abandoned capture/sync.Map/OnUnregisterSession design — they say "session-stable" forwarding, but f08f3d0 switched to per-request (the opposite semantic). Since this squash-merges, the commit and any derived changelog would misdescribe the change. Please update the title and body to match the per-request implementation before/with the follow-up.

[tgrunnagle]

Multi-Agent Consensus Review

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

Consensus Summary

#	Finding	Consensus	Severity	Action
F1	Issue #5560 AC mismatch — intentional per-request vs. session-stable	9/10	MEDIUM	Update issue
F2	Stale client.go comment references removed session-stable machinery	7/10	MEDIUM	Push local fix 025ee28a
F3	Stale "session-stable" comment in NewVMCPServer	6/10	LOW	Fix
F4	Stale vmcpserver.Config type name in passthrough_headers_test.go	7/10	LOW	Fix

Overall

This PR correctly identifies and fixes the actual bug: passthrough headers were silently dropped on the Serve path because defaultClientFactory built its transport from target.HeaderForward only and never read ForwardedHeadersFromContext(ctx). The fix — calling MergeForwardedHeaders inside the factory — is targeted and correct. Session-stable forwarding is not restored; the user has confirmed this was always a coincidental side-effect of the per-session connection model, not a design requirement.

The implementation is substantially simpler than the previous iteration: no sync.Map, no session-capture hooks, no injectCapturedHeaders. The integration test correctly inverts the second-call assertion to match per-request semantics. All findings from the prior review have been addressed (telemetryComposer tests added, base-mutation guard added, doc comment updated for canonicalization behavior).

The remaining items are documentation: the most important is F2 — a local commit (025ee28a) already fixes the stale client.go comment but hasn't been pushed to the PR branch yet. F3 and F4 are one-line fixes. F1 asks for the linked issue's acceptance criteria to be updated so future contributors don't treat per-request forwarding as a regression.

---

Generated with Claude Code