Wire the tool optimizer onto the Serve path#5543
Conversation
Implements changes for issue #5538: - Build a per-session optimizer over core.ListTools on the Serve path and advertise only find_tool/call_tool (sourced from the core) - Route call_tool's inner invocation through core.CallTool by its real name, closing the deferred inner-target admission gap - Skip the session factory's optimizer decorator on the Serve path (FactoryConfig.AdvertiseFromCore) to avoid double-indexing the store - Surface the resolved optimizer factory via Manager.OptimizerFactory; store/cleanup ownership stays in sessionmanager Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5543 +/- ##
==========================================
+ Coverage 69.85% 69.88% +0.03%
==========================================
Files 647 649 +2
Lines 65796 66000 +204
==========================================
+ Hits 45960 46125 +165
- Misses 16491 16512 +21
- Partials 3345 3363 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-agent review summary
Strong, well-tested PR that delivers exactly what #5538 asks and closes the deferred call_tool inner-target authorization gap as a bonus. All 12 acceptance criteria are satisfied, and the four security invariants (optimizer-agnostic seam, real-inner-name authorization, generic no-leak denial, fail-closed identity binding) are correctly implemented and load-bearing-tested against the real coreToolHandler path. 0 HIGH findings — nothing blocks merge.
Two MEDIUM items worth attention (neither a live bug):
- F1
find_tooldropped the nil-output guard the legacy + siblingcall_toolhandlers keep, while a comment still claims it "mirrors the legacy handler." Latent today; 2-line fix recommended in this PR. - F2
AdvertiseFromCoreis an unenforced 3-point contract — enabling the optimizer but forgetting the flag silently double-indexes the FTS5 store. Acknowledged as unenforced until #5445; should be made fail-loud before that PR flips the switch.
Four LOW test/perf polish items are inline. Reviewed by 6 specialist agents (Security, Concurrency, Architecture, Go, Test-coverage, General). Codex cross-review skipped (CLI not installed).
Recommendation: COMMENT — land F1 here; track F2 as a #5445 prerequisite.
🤖 Generated with Claude Code
Addresses #5543 review comments: - MEDIUM serve_optimizer.go (3428801576): add the output == nil guard to optimizerFindToolHandler for parity with the legacy/sibling handlers, so a nil result cannot marshal to "null" and surface as a success - LOW serve_optimizer.go (3428801603): document that optimizerSessionTools re-upserts on every registration and cross-pod rehydration (idempotent by PK, repeated work not a leak; optimization deferred to #5445)
Addresses #5543 review comments: - LOW serve_optimizer_test.go (3428801613): assert find_tool returns exactly the core's advertised tool names by decoding into FindToolOutput, instead of substring-matching the whole marshalled body; note ranking is out of scope - LOW serve_optimizer.go (3428801607): add a test that optimizerToolHandler rejects an unknown meta-tool name, locking in the defensive default branch - LOW serve_optimizer_test.go (3428801618): assert the cross-pod rehydration re-aggregates via a fresh core.ListTools and rebuilds the optimizer (the half of AC5 the test did not previously prove)
Addresses #5543 review comments: - MEDIUM factory.go (3428801597): make AC6 structural instead of an unenforced contract. Manager surfaces the optimizer factory via OptimizerFactory() only when AdvertiseFromCore is true, so the session-factory decorator (used iff !AdvertiseFromCore) and the Serve-layer getter become mutually exclusive writers of the shared FTS5 store. A Serve composition root that enables the optimizer but forgets the flag now gets a nil factory (no Serve-layer optimizer) rather than a silent double-index. Adds a test locking in the gate in both directions.
Summary
The tool optimizer (
find_tool/call_tool) is currently a session-factory decorator with no equivalent on the Serve path. On the Serve path (s.core != nil), session registration sources the advertised capability set fromcore.ListTools, which bypasses the optimizer decorator entirely. This matters because #5445 (P3.2) makesServethe single live path for allserver.Newcallers — including the productionthv vmcp serve --enable-optimizerpath. The momentserver.Newroutes throughServe,tools/listwould advertise raw backend tools instead offind_tool/call_tool, silently breaking--enable-optimizer. This PR makes the Serve path optimizer-complete first so #5445 can flip the switch without regressing the optimizer.#5442deferred it;core/admission.godocuments the invariant). Routingserver.NewthroughServefor an optimizer-enabled config would otherwise silently break--enable-optimizer.core.ListToolsand routecall_tool's inner invocation throughcore.CallToolby the real inner tool name.core.CallToolauthorizes the inner tool through the core admission seam, this also closes the deferredcall_toolinner-target authorization gap documented inpkg/vmcp/core/admission.go— no separate optimizer-admission change needed.server.Newpath (s.core == nil) is unchanged.Closes #5538
Blocks #5445. Relates to #5442 (the original deferral). Epic #5419 — vMCP interface refactor (RFC THV-0076).
Type of change
Test plan
task test)task lint-fix)Added
pkg/vmcp/server/serve_optimizer_test.go(internalpackage server) drivingServewith a stubcore.VMCPand a fake optimizer factory:tools/listadvertises exactly{find_tool, call_tool}with raw tools hidden.find_toolreturns matches over the core's advertised set.call_toolroutes the inner invocation throughcore.CallToolwith the real inner tool name.lazyInjectSessionToolsre-injectsfind_tool/call_toolfor a rehydrated session.The existing
TestIntegration_SessionManagement_OptimizerModecontinues to exercise the legacyserver.Newpath (s.core == nil), unchanged until #5445 removes that path.Changes
pkg/vmcp/server/serve_optimizer.goserveSessionTools,optimizerSessionTools, and thefind_tool/call_toolSDK handlers (identity binding, search over core tools, inner dispatch throughcore.CallTool).pkg/vmcp/server/serve_optimizer_test.gopkg/vmcp/server/serve.go*Server; document the optimizer caller contract (AdvertiseFromCore).pkg/vmcp/server/serve_handlers.goinjectCoreSessionCapabilitiesnow callsserveSessionToolsinstead ofcoreSessionTools.pkg/vmcp/server/server.gooptimizerFactoryfield; cross-podlazyInjectSessionToolsnow usesserveSessionToolson the Serve path.pkg/vmcp/server/sessionmanager/factory.goFactoryConfig.AdvertiseFromCore; skip the factory's optimizer decorator when set to avoid double-indexing the shared store.pkg/vmcp/server/sessionmanager/session_manager.goManager; expose it viaOptimizerFactory().pkg/vmcp/session/optimizerdec/decorator.gofind_tool/call_tooldefinitions into the exportedOptimizerTools()shared by both consumers.Does this introduce a user-facing change?
No. This is test-only behavior until #5445 wires the Serve path into a production composition root. Once that lands,
thv vmcp serve --enable-optimizercontinues to advertise onlyfind_tool/call_toolwith no behavioral change for users.Special notes for reviewers
AdvertiseFromCoreis the single switch that prevents double-indexing: the session manager retains the factory's store/cleanup ownership but skips the decorator, and the Serve layer consumes the resolved factory directly.call_toolreturns the core handler's generic "call denied by authorization policy" result verbatim on a denied inner target, so no authorizer detail leaks.optimizerCallToolHandlerguards against a(nil, nil)result from a future optimizer implementation.Generated with Claude Code