Replace discovery-into-context with direct VMCP calls

[tgrunnagle]

Summary

P2.4 of the vMCP core-extraction epic (parent story #5431, epic #5419). On the Serve
path the core VMCP is now the single authoritative, admission-filtered source of truth
for the advertised capability set and call routing. Capability aggregation must therefore
stop flowing through the discovery-into-context middleware, which applies no admission and
would bypass authorization now that authz lives in the core admission seam (#5438).

This PR wires the Serve path through the core's explicit-identity methods and guards the
legacy discovery path to server.New only:

• Store the injected core on *Server; the s.core == nil / s.core != nil value is the
  branch selector for legacy vs. Serve throughout the server. The discovery middleware is
  guarded to the legacy path (s.core == nil), left intact rather than deleted — physical
  removal is Phase 3 (P3.2 Reduce server.New body to the wrapper #5445).
• Source session capabilities from core.ListTools / core.ListResources once at
  OnRegisterSession; route tool/resource invocations through core.CallTool /
  core.ReadResource with an explicit *auth.Identity (new serve_handlers.go).
• Enforce identity binding on the core call path via a new exported
  session.ValidateCaller seam (audited check stays in
  pkg/vmcp/session/internal/security). Requests reach the core directly, bypassing the
  BindSession decorator, so this is the sole binding-enforcement point on the Serve path.
• Resolve Serve-path audit backend enrichment from Tool.BackendID → backend.Name via
  core.LookupTool + the registry, for parity with the legacy path's WorkloadName. The
  legacy context-based enrichment path is unchanged.

Closes #5442

Type of change

• Refactoring (no behavior change)

Test plan

• Unit tests (task test)
• Linting (task lint-fix)
• Manual testing (describe below)

task build passes. Changed-package unit tests pass under the race detector:
go test -race ./pkg/vmcp/server/... ./pkg/vmcp/session/..., including the new Serve-path
tests: discovery-guard branch coverage, tools/call routed through the core, the
once-per-session core call, identity-binding enforcement on the call path, audit-enrichment
parity (BackendName resolved from BackendID), and ValidateCaller fail-closed cases.

task lint-fix is clean except a pre-existing, unrelated gosec G115 in
cmd/thv/app/upgrade.go (untouched by this PR).

Note on task test: two unrelated environment failures, neither touched by this PR —
Docker-dependent tests ("no available runtime") and the flaky TestTransparentProxy_*
tests in pkg/transport/proxy/transparent (pass in isolation). The packages this PR
changes pass.

Changes

File	Change
pkg/vmcp/server/serve_handlers.go	New. Serve-path capability wiring: injectCoreSessionCapabilities sources tools/resources from the core once per session; coreToolHandler / coreResourceHandler route through core.CallTool / core.ReadResource; enforceSessionBinding is the sole identity-binding check on the Serve call path.
pkg/vmcp/server/server.go	Store core on *Server; guard the discovery middleware application to s.core == nil; branch handleSessionRegistrationImpl and lazyInjectSessionTools (cross-pod re-injection) onto the core on the Serve path.
pkg/vmcp/server/backend_enrichment.go	Serve-path audit enrichment resolves the backend via core.LookupTool / LookupResource / LookupPrompt and Tool.BackendID, then maps to backend.Name via the registry. Legacy context-based resolution unchanged.
pkg/vmcp/server/serve.go	Assign the injected core to srv.core; update the Serve contract docs (the "/" MCP route is now serveable because the shared Handler skips discovery when s.core != nil).
pkg/vmcp/session/session.go	New exported ValidateCaller seam delegating to the internal security package.
pkg/vmcp/session/internal/security/security.go	Extract validateCallerBinding free function so the same audited check backs both the decorator and the exported ValidateCaller.

Does this introduce a user-facing change?

No. The legacy server.New path is unchanged; the Serve path is not yet routed in
production (no composition root until a later phase).

Implementation plan

Approved implementation plan

The issue's literal AC2 prescribes feeding core.ListTools into the session factory as
the ProcessPreQueriedCapabilities input. Planning analysis found this infeasible:

• ProcessPreQueriedCapabilities takes per-backend raw tools, while core.ListTools
  returns a flat, already-aggregated slice.
• The flat slice drops BackendTarget.OriginalCapabilityName, which the factory needs for
  backend-name translation.

The approved alternative ("C1"), discussed with and approved by the issue author/owner
before implementation:

1. Core-sourced advertising. Use the core's output directly as the SDK session's
   fixed-at-initialize capability set; the factory does not re-aggregate on the Serve path.
   core.ListTools / core.ListResources are called once per session at
   OnRegisterSession.
2. Core-routed calls. Install SDK handlers that route tools/call / resources/read
   through core.CallTool / core.ReadResource with an explicit *auth.Identity read at
   the transport boundary (never from context inside the core).
3. Discovery guard. Wrap the discovery-middleware application in if s.core == nil,
   leaving the middleware, handleSubsequentRequest, and the context seam in place for the
   legacy path. Physical removal is deferred to Phase 3 (P3.2 Reduce server.New body to the wrapper #5445).
4. Audit enrichment via LookupTool. Derive the backend from Tool.BackendID (via the
   core's admission-filtered Lookup*) → backend.Name via the registry, matching the
   legacy WorkloadName. This re-aggregates per audited request; documented and deferred
   per anti-pattern Use an actual logger #9 (no production composition root yet).
5. ValidateCaller binding seam. Add an exported session.ValidateCaller so the Serve
   path can enforce identity binding without the BindSession decorator; the audited check
   stays in pkg/vmcp/session/internal/security.

Special notes for reviewers

• AC2 was implemented via an approved alternative, not literally. The issue's AC2 asks
  for core.ListTools to feed the factory as the ProcessPreQueriedCapabilities input.
  That is infeasible: ProcessPreQueriedCapabilities consumes per-backend raw tools, but
  core.ListTools returns a flat aggregated slice, and that flat slice drops
  BackendTarget.OriginalCapabilityName needed for backend-name translation. So the core's
  output is used directly as the SDK session's fixed-at-initialize set and the factory does
  not re-aggregate on the Serve path. This was discussed with and approved by the issue
  author/owner before implementation.
• Audit-enrichment re-aggregation is a known, deferred cost. LookupTool re-aggregates
  per audited request (the core is intentionally stateless). This is documented inline and
  deferred per vMCP anti-pattern Use an actual logger #9 (optimize with evidence): the Serve path has no
  production composition root yet. The fix when the path goes live is a per-session
  advertised-set cache ("Serve caches, core is stateless").
• enforceSessionBinding fails closed in two ways — a missing session record and an
  empty/unparsable binding both reject the caller, and any rejection terminates the session.
  This is intentionally stricter than the legacy GetAdaptedTools handler.
• Scope: the discovery middleware, handleSubsequentRequest injection, and the
  DiscoveredCapabilities context seam are deliberately left in place for the legacy path;
  physical removal is Phase 3 (P3.2 Reduce server.New body to the wrapper #5445). server.New's signature and observable behavior are
  unchanged.
• This PR went through two review rounds (1 HIGH + 2 MEDIUM, then 2 MEDIUM + 2 LOW), all
  addressed; the final review pass found 0 critical/high/medium findings.

Large PR Justification

The production change is small and cohesive: a single logical refactor (route the Serve path through the core VMCP), ~255 lines of non-comment code across 6 source files — within the ≤400 LOC / ≤10 file guideline.

The ~1085-line total is dominated by:

• Tests (594 lines): Serve-path handler coverage (tool/resource handlers, binding enforcement, fail-closed cases, prompts omission, audit enrichment) added per code-review feedback. Splitting these from the code they cover would reduce reviewability.
• Architectural doc comments: this is the Phase 2 join point of the core-extraction epic; the comments capture non-obvious contracts (AC2 single-aggregation, the C1 deviation from the literal spec, the binding seam) for future maintainers.

Per CONTRIBUTING/CLAUDE.md, large PRs are acceptable for test-heavy changes. This is one atomic refactor that should not be split from its tests.

[codecov]

Codecov Report

❌ Patch coverage is 75.42857% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.50%. Comparing base (ad127f7) to head (f428746).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/vmcp/server/serve_handlers.go	70.90%	21 Missing and 11 partials ⚠️
pkg/vmcp/server/backend_enrichment.go	70.58%	9 Missing and 1 partial ⚠️
pkg/vmcp/server/server.go	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5491      +/-   ##
==========================================
+ Coverage   69.48%   69.50%   +0.02%     
==========================================
  Files         638      640       +2     
  Lines       65042    65193     +151     
==========================================
+ Hits        45192    45311     +119     
- Misses      16529    16551      +22     
- Partials     3321     3331      +10     

☔ 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 review — Replace discovery-into-context with direct VMCP calls

A refactor (P2.4 of the core-extraction epic) that adds a guarded Serve path beside the unchanged legacy server.New path. The approach is sound and well-executed: the legacy path is genuinely untouched, dependency direction (server→core) is clean, identity is read once at the transport boundary and passed explicitly (not ctx-coupling), and the security-critical behaviors — fail-closed binding via the reused validateCallerBinding, generic authz-denial messages, audit name-resolution only for caller-reachable capabilities — are correct. The C1 deviation from the issue's literal AC2 is justified and documented (the literal wiring is infeasible because core.ListTools returns a flat aggregated slice that has dropped the per-backend data the factory needs).

No blocking correctness or security regressions. The substantive items are test-coverage gaps on the new handlers and one latent architectural contract; neither is a runtime regression today (the Serve path is test-only, with no production composition root).

Consensus summary

#	Finding	Severity	Action
F1	Serve path double-aggregates: factory CreateSession aggregates in addition to core.ListTools; AC2 "factory does not aggregate" is an unenforced/untested contract	MEDIUM	Fix (doc + test)
F2	coreResourceHandler has zero test coverage	MEDIUM	Fix
F3	coreToolHandler error/denial paths untested; fakeCore.callErr is a dead field	MEDIUM	Fix
F4	terminate-on-binding-failure side effect never asserted	MEDIUM	Fix
F5	fakeCore doesn't model admission filtering; "denied" subtest only exercises not-found	MEDIUM	Fix
F6	No-arg tools/call fails the arguments assertion — parity-preserving, pre-existing in all 4 adapters (not a regression)	MEDIUM	Follow-up
F7	Resource handler forwards raw core error to the client (info-disclosure asymmetry); parity with legacy	MEDIUM	Follow-up
F8	Cross-pod re-injection lists tools under request identity, not bound identity (specialist severity conflict; adjudicated)	LOW	Adjudicate
F9	Audit LookupTool re-aggregates per audited request (documented deferral, anti-pattern #9)	LOW	Track
F10	Prompts omitted — confirm SDK doesn't advertise a prompts capability; lock omission with a test	LOW	Fix (cheap)

Recommendation: COMMENT. 0 HIGH after verification — both agent-flagged HIGHs are non-blocking on inspection (F6 is parity-preserving pre-existing behavior; F8 is bounded by the call-time binding check). The MEDIUM test-coverage gaps (F2–F5) and the double-aggregation contract (F1) are worth addressing before the Serve path is wired into production; F6/F7 belong in separate follow-ups. Inline comments below.

_{Reviewed against 688e4573. 6 specialist agents (security, concurrency, architecture, MCP/SDK, test, general); codex cross-review skipped (not installed).}

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

[github-actions]

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

Large PR justification has been provided. Thank you!

[jerm-dro]

Re: `pkg/vmcp/session/factory.go:147` (WithAggregator) — leaving as a top-level note since this file is outside the PR diff.

suggestion: Once the legacy path is gone (Phase 3, #5445), the factory should never aggregate — that responsibility belongs entirely to the core on the Serve path. Consider dropping a TODO here tying WithAggregator (and the aggregator field) to that removal, so the "factory MUST be built without an aggregator" contract (AC2) becomes a deletion that's already on the books rather than a comment people have to remember.

// TODO(#5445): Remove WithAggregator and the aggregator field once server.New is
// routed through Serve. On the Serve path the core is the single aggregator; a
// factory that also aggregates double-aggregates and drifts (AC2). Deleting the
// option makes that contract structurally impossible to violate.