[vMCP] Consolidate duplicate identity-propagation round-trippers into shared internal transport package
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 2.2k
- Forks
- 300
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 184
Description
Context
After the fix in #5323 (commits 2298d349 + 109a8a15), the identity-propagation round-tripper exists in two parallel implementations:
pkg/vmcp/client/client.go—identityPropagatingRoundTripper(withisHealthCheckfield; used by the per-callhttpBackendClient).pkg/vmcp/session/internal/backend/mcp_session.go—identityRoundTripper(withoutisHealthCheck; used by the persistent-session connector).
The two types now have:
- Near-identical struct definitions (
base,fallbackIdentity, with one carrying an extraisHealthCheckbool). - Near-identical
RoundTripimplementations, both gating fallback injection on!hasIdentity. - Near-identical doc comments (each citing #5323).
- Near-identical unit test suites (both following the
*_PerRequestIdentity_*/*_FallbackIdentity_*grouping).
The asymmetric health-check handling is documented in the identityRoundTripper doc comment, but the structural duplication remains a long-term maintenance hazard: any future change to the identity invariant (e.g. logging on fallback injection, metrics counters, additional checks) must be repeated in both files with no compile-time link between them.
Proposal
Extract both round-trippers into a small internal helper package — e.g. pkg/vmcp/internal/transport — and have both pkg/vmcp/client/client.go and pkg/vmcp/session/internal/backend/mcp_session.go consume the same type. Parameterise the health-check marker so the same implementation serves both call sites:
- Constructor option pattern (
transport.WithHealthCheckMarker()) or - Two thin wrapper types in the same package sharing a common implementation.
The consolidation should also unify the unit test suites in one package, removing the lock-step maintenance requirement.
Acceptance criteria
- Single round-tripper type (or family) lives in
pkg/vmcp/internal/transport(or similar) and is consumed by bothpkg/vmcp/client/client.goandpkg/vmcp/session/internal/backend/mcp_session.go. - Health-check marker propagation is opt-in via a constructor option, not a separate type that the session-backend variant lacks.
- Unit test suite for the round-tripper(s) lives alongside the shared implementation; the consumer packages keep only integration-level tests that exercise the full transport chain.
- No behavioural change: existing
pkg/vmcp/client/client_test.go,pkg/vmcp/session/internal/backend/roundtripper_test.go, andpkg/vmcp/session/internal/backend/mcp_session_identity_refresh_test.gocontinue to pass (or are migrated alongside the move).
Notes
This was raised as a MEDIUM finding on the iteration 2 review of #5323. The reviewer explicitly allowed deferring it from the bug-fix PR; this issue tracks the cleanup so the duplication is not silently forgotten.
Related: #5323
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by comparing the round-tripper implementations in pkg/vmcp/client/client.go and pkg/vmcp/session/internal/backend/mcp_session.go, then review the related tests named in the acceptance criteria. Extract the shared implementation under pkg/vmcp/internal/transport with opt-in health-check marking, move its unit tests alongside it, and run the existing consumer tests to confirm no behavioral change.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100