Move HeaderForward helpers to pkg/vmcp/headerforward#5302
Conversation
Lift BuildHeaderForwardTripper and its supporting types out of pkg/vmcp/client/ into the new sibling package pkg/vmcp/headerforward/. The helper now has two consumers — pkg/vmcp/client (capability-discovery HTTP client) and pkg/vmcp/session/internal/backend (per-session client) — and is not logically part of either. A pkg/vmcp/headerforward/wirefmt subpackage already exists, so the namespace is established. Pure mechanical relocation: no behavior change, no signature change, existing tests still cover the helper. This makes future vMCP anti-pattern #8 cleanup (consolidation of the two parallel HTTP-client builders) more straightforward. Related to #5289 (the bug whose fix in PR #5301 motivated the export).
lorr1
left a comment
There was a problem hiding this comment.
Automated review loop opened per user request.
Reviewed the mechanical move of header-forward helpers from pkg/vmcp/client/ to the new sibling package pkg/vmcp/headerforward/. No findings. The diff is a clean rename: package declarations updated, buildHeaderForwardTripper exported as BuildHeaderForwardTripper, single call site in pkg/vmcp/client/client.go updated with a 3-line wrap necessary to stay under the 130-char lll limit (141 chars unwrapped). Verified:
- File-for-file diff vs
origin/mainshows onlypackagerename, function-name capitalization, and an added package doc comment — no behavior change. - No leftover references to the old location/name (
rgis clean). pkg/vmcp/client/header_forward_integration_test.gocorrectly stayed inpkg/vmcp/client/: its subject is theNewHTTPBackendClientfactory wiring and it only references the exportedvmcpclient.NewHTTPBackendClient, no moved unexported types.- Test coverage parity:
pkg/vmcp/headerforward/transport_test.gois byte-identical to the oldheader_forward_test.goafter the package and function-name rewrites. - New file has a proper
// Package headerforward ...doc comment naming both consumer packages. - Import grouping in
client.gofollows the standard 3-group convention. - SPDX headers preserved on both moved files.
pkg/vmcp/headerforward/wirefmt/subpackage untouched.- Local
go build ./pkg/vmcp/...andgo test ./pkg/vmcp/headerforward/... ./pkg/vmcp/client/...both pass.
Note on CI: the Tests / Test Go Code (ubuntu-8cores-32gb) check failed due to a runner shutdown (task: Signal received: "terminated", The runner has received a shutdown signal) — infrastructure flake, not a real test failure. Will retrigger and confirm green.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5302 +/- ##
==========================================
- Coverage 68.46% 68.46% -0.01%
==========================================
Files 620 620
Lines 63358 63360 +2
==========================================
Hits 43379 43379
- Misses 16741 16746 +5
+ Partials 3238 3235 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
BuildHeaderForwardTripperfrompkg/vmcp/client/so the per-session HTTP client atpkg/vmcp/session/internal/backend/could reuse it without duplicating ~160 LOC of transport-chain construction. That export was the minimum-viable fix for MCPServerEntry.headerForward.addPlaintextHeaders is silently ignored — never reaches upstream #5289, butpkg/vmcp/client/is also a consumer of the helper — not its logical home. Two consumers + an awkwardly placed shared helper is one of the shapes vMCP anti-pattern Start simple hardcoded registry #8 calls out.pkg/vmcp/headerforward/. The companion subpackagepkg/vmcp/headerforward/wirefmt/already exists, so the namespace is established. Pure mechanical move — no signature change, no behavior change, existing tests still cover the helper end-to-end.Related to #5289.
Type of change
Test plan
task test) — existingBuildHeaderForwardTripperandresolveHeaderForwardtests moved alongside the helper, still pass against the new location.task lint-fix) — clean.task test-e2e)API Compatibility
v1beta1API.No CRD changes. The only API surface delta is the package path of
BuildHeaderForwardTripper. The helper was exported only by PR #5301; this PR moves that export to its final home before broader use.Changes
pkg/vmcp/headerforward/transport.gopkg/vmcp/client/header_forward.go, package renamed.pkg/vmcp/headerforward/transport_test.gopkg/vmcp/client/header_forward_test.go.pkg/vmcp/client/header_forward.gopkg/vmcp/client/header_forward_test.gopkg/vmcp/client/client.goheaderforward.BuildHeaderForwardTripper.Does this introduce a user-facing change?
No.
Special notes for reviewers
main, not offgithub_mcp_header(PR Wire HeaderForward into vMCP per-session HTTP client #5301). The two PRs are independent. Whichever merges first, the other's rebase is a one-line import-path update inmcp_session.go(PR Wire HeaderForward into vMCP per-session HTTP client #5301) orclient.go(this PR).pkg/vmcp/headerforward/wirefmt/is unchanged — it owns the env-var wire format used by the operator to pass header-forward config into the vmcp pod; the newtransport.gois the runtime that consumes that config.Generated with Claude Code