fix: deterministic Trino multi-connection routing with mandatory selection#146
Merged
fix: deterministic Trino multi-connection routing with mandatory selection#146
Conversation
…ager Replace N separate single-client Trino toolkits with a single toolkit backed by multiserver.Manager, eliminating non-deterministic connection routing caused by last-write-wins tool registration and Go map iteration. - Add AggregateToolkitFactory type for toolkit kinds that handle multi-connection routing internally - Add NewMulti constructor using mcp-trino's multiserver.Manager - Switch RegisterBuiltinFactories to use TrinoAggregateFactory for trino - Extract connection arg from tool call request for accurate audit logging - Refactor MCPToolCallMiddleware and LoadFromMap to stay within complexity limits
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
+ Coverage 90.47% 90.65% +0.18%
==========================================
Files 112 113 +1
Lines 10800 11032 +232
==========================================
+ Hits 9771 10001 +230
- Misses 742 743 +1
- Partials 287 288 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ons multi-connection support When multiple Trino connections are configured, tool calls that omit the connection parameter now return an error listing all available connections with their descriptions, so the LLM can choose explicitly instead of silently routing to the default. The list_connections tool now expands multi-connection toolkits into individual entries with descriptions and default markers via the new ConnectionLister interface. - Add Description field to Trino Config, parsed from platform YAML - Add pkg/toolkit shared types package (ConnectionDetail, ConnectionLister) - Implement ConnectionLister on Trino toolkit for single and multi mode - Add ConnectionRequiredMiddleware rejecting empty connection when >1 exist - Update list_connections handler to use ConnectionLister for expansion - Add example description fields to configs/platform.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes non-deterministic Trino connection routing when multiple instances are configured (e.g., cassandra, elasticsearch, warehouse) and adds three follow-up improvements: connection descriptions, mandatory connection selection, and correct
list_connectionsexpansion.Commit 1: Deterministic routing via multiserver.Manager
Previously, the platform created N separate single-client
Toolkitinstances that each registered the same tool names viaRegisterTools(server). Due to the MCP SDK's last-write-wins semantics and Go map iteration order, one random toolkit "owned" all Trino tools per process start — causing queries intended for one catalog to hit a different one entirely.Replaced N separate toolkits with a single toolkit backed by
multiserver.Managerfrommcp-trino. The manager handles connection routing internally based on theconnectionparameter in each tool call request.Commit 2: Connection descriptions, mandatory selection, list_connections fix
Three design gaps remained after the routing fix:
No connection descriptions — LLMs had no way to know what each connection is for. Added a
descriptionfield to Trino instance config so operators can document each connection's purpose (e.g., "Elasticsearch for transactional sales data").Connection silently defaults — When multiple connections exist and none is specified, the query silently routed to the default. Now a
ConnectionRequiredMiddlewarerejects the call with an error listing all available connections and their descriptions, so the LLM can choose explicitly.list_connectionsonly showed 1 entry — After the multi-connection change, the platform creates 1 Trino toolkit (not N), but the handler iteratedtoolkitRegistry.All()creating 1 entry per toolkit. Now toolkits implementing theConnectionListerinterface are expanded into individual entries with descriptions and default markers.Changes
Registry & loader (
pkg/registry/)toolkit.go: AddedAggregateToolkitFactorytype for factories that receive all instance configs and produce a single toolkitregistry.go: AddedaggregateFactoriesmap,RegisterAggregateFactory(),GetAggregateFactory()loader.go: Check for aggregate factories before per-instance creation. ExtractedloadAggregate(),mergeInstanceConfigs(),loadKindFromMap(),mergeMapInstances()factories.go: AddedTrinoAggregateFactoryusingmultiserver.ManagerShared types (
pkg/toolkit/)connection.go: New dependency-free package withConnectionDetailandConnectionListerinterface (avoids import cycle betweenregistry↔toolkits/trino)Trino toolkit (
pkg/toolkits/trino/)config.go: AddedMultiConfig,ParseMultiConfig(), parsedescriptionfieldtoolkit.go: AddedNewMulti()withmultiserver.Manager,ListConnections()implementingConnectionLister,buildConnectionRequired()helper,connectionDescriptionsmapconnection_required.go: NewConnectionRequiredMiddleware— rejects tool calls with emptyconnectionwhen multiple connections exist; error message lists all connections with descriptions sorted alphabetically. Uses reflection (FieldByName("Connection")) to extract the connection from any Trino tool input typeAudit accuracy (
pkg/middleware/)mcp.go: AddedextractConnectionArg()to parse theconnectionfield from tool call JSON arguments. Audit logs now reflect the actual target connection, not the toolkit defaultPlatform tools (
pkg/platform/)connections_tool.go: AddedDescription/IsDefaultfields toconnectionEntry. Handler checks forConnectionListerinterface to expand multi-connection toolkits into individual entriesConfig (
configs/platform.yaml)descriptionfields to Trino instancesHow it works
Config format
Test plan
make verifypasses (fmt, test, lint, security, coverage, dead-code, mutation, release-check)NewMulti100%,ListConnections100%,ConnectionRequiredMiddleware.Before100%,extractConnectionFromInput100%,buildConnectionRequired100%,ParseConfig(description) 100%,handleListConnections91.7%pc.Connectionuses request arg, not toolkit defaultConnectionRequiredMiddlewaretests: passes with connection set, rejects without connection when >1 exist, skipslist_connectionstool, error includes all connection names and descriptionsListConnectionstests: single mode returns 1 entry, multi mode returns all with descriptions, satisfiesConnectionListerinterfacehandleListConnectionstests:ConnectionListertoolkit expanded into multiple entries with description/is_default; non-lister toolkit falls through to legacy pathlist_connections→ all 3 Trino connections appear with descriptionstrino_querywithoutconnection→ error lists available connections with descriptionstrino_querywithconnection: "warehouse"→ routes correctly