stacklok / stacklok/toolhive

[vMCP] Consolidate duplicate identity-propagation round-trippers into shared internal transport package

Open
#5,333 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

go refactor vmcp
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.goidentityPropagatingRoundTripper (with isHealthCheck field; used by the per-call httpBackendClient).
  • pkg/vmcp/session/internal/backend/mcp_session.goidentityRoundTripper (without isHealthCheck; used by the persistent-session connector).

The two types now have:

  • Near-identical struct definitions (base, fallbackIdentity, with one carrying an extra isHealthCheck bool).
  • Near-identical RoundTrip implementations, 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 both pkg/vmcp/client/client.go and pkg/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, and pkg/vmcp/session/internal/backend/mcp_session_identity_refresh_test.go continue 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.