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 (with isHealthCheck field; used by the per-call httpBackendClient).
pkg/vmcp/session/internal/backend/mcp_session.go — identityRoundTripper (without isHealthCheck; used by the persistent-session connector).
The two types now have:
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
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
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:
base,fallbackIdentity, with one carrying an extraisHealthCheckbool).RoundTripimplementations, both gating fallback injection on!hasIdentity.*_PerRequestIdentity_*/*_FallbackIdentity_*grouping).The asymmetric health-check handling is documented in the
identityRoundTripperdoc 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 bothpkg/vmcp/client/client.goandpkg/vmcp/session/internal/backend/mcp_session.goconsume the same type. Parameterise the health-check marker so the same implementation serves both call sites:transport.WithHealthCheckMarker()) orThe consolidation should also unify the unit test suites in one package, removing the lock-step maintenance requirement.
Acceptance criteria
pkg/vmcp/internal/transport(or similar) and is consumed by bothpkg/vmcp/client/client.goandpkg/vmcp/session/internal/backend/mcp_session.go.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