From 4ec3075ed7ff5ccc0e550c351912020b52916434 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Thu, 19 Mar 2026 17:41:20 -0700 Subject: [PATCH 1/4] Add RFC-0058: Inline aggregator into session factory and extract filter decorator The vMCP Aggregator interface is a monolithic middleman between middleware and session objects. This RFC proposes inlining its pipeline into the session factory and extracting the advertising filter as a session decorator. This fixes a bug where composite tool type coercion fails for non-advertised backend tools (toolhive#4287). Co-Authored-By: Claude Opus 4.6 --- ...0058-inline-aggregator-filter-decorator.md | 305 ++++++++++++++++++ 1 file changed, 305 insertions(+) create mode 100644 rfcs/THV-0058-inline-aggregator-filter-decorator.md diff --git a/rfcs/THV-0058-inline-aggregator-filter-decorator.md b/rfcs/THV-0058-inline-aggregator-filter-decorator.md new file mode 100644 index 0000000..fce33e0 --- /dev/null +++ b/rfcs/THV-0058-inline-aggregator-filter-decorator.md @@ -0,0 +1,305 @@ +# RFC-0058: Inline aggregator into session factory and extract advertising filter as session decorator + +- **Status**: Draft +- **Author(s)**: Jeremy Drouillard (@jerm-dro) +- **Created**: 2026-03-19 +- **Last Updated**: 2026-03-19 +- **Target Repository**: toolhive +- **Related Issues**: [toolhive#4287](https://github.com/stacklok/toolhive/issues/4287) + +## Summary + +Decompose the monolithic `Aggregator` interface by inlining its pipeline (per-backend overrides, conflict resolution, routing table construction) into the session factory, and extracting the advertising filter into a session decorator. This fixes a bug where composite tool workflows fail type coercion for non-advertised backend tools, and simplifies the vMCP architecture by removing a layer that sits awkwardly between middleware and session. + +## Problem Statement + +### Bug: composite tool type coercion fails for non-advertised tools + +When backend tools are excluded from advertising via `excludeAll` or `filter`, composite tool workflows fail. The workflow engine's `getToolInputSchema()` searches the session's tool list for `InputSchema` to coerce string arguments to their schema types. Non-advertised tools are absent from this list, so coercion is skipped and backends reject string-typed arguments (e.g. `pullNumber` as `"42"` instead of `float64(42)`). + +The root cause: the advertising filter runs inside the aggregator **before** session creation. The session's `tools` field only contains advertised tools. The composite tools decorator builds its workflow engine from `sess.Tools()`, so it never sees schemas for non-advertised tools. + +### Architectural issue: the aggregator is a monolithic middleman + +The `Aggregator` interface has 7 responsibilities: backend capability querying, per-backend overrides, conflict resolution, advertising filtering, routing table construction, name reversal, and discovery pipeline orchestration. It sits between the HTTP middleware and the session object — operating on data that logically belongs to session construction. + +The discovery path (`AggregateCapabilities`) is dead code — `WithSessionScopedRouting()` bypasses it entirely. Only `ProcessPreQueriedCapabilities` is called in production, during session creation. + +As the aggregator's responsibilities shift (e.g. removing the filter), it becomes increasingly confusing to reason about what it does and when. + +## Goals + +- Fix composite tool type coercion for non-advertised backend tools +- Eliminate the `Aggregator` interface and `defaultAggregator` struct +- Move advertising filtering to a session decorator in the correct position in the decorator stack +- Keep conflict resolvers and per-backend override utilities as a reusable library +- No breaking changes to configuration or client-visible behavior + +## Non-Goals + +- Moving renaming/conflict-resolution into a session decorator (not feasible — routing requires private session state) +- Changing the `MultiSession` interface +- Refactoring Cedar authorization from HTTP middleware to a session decorator +- Removing the conflict resolver implementations or changing their behavior + +## Proposed Solution + +### High-Level Design + +``` +Decorator stack (top → bottom): + + optimizer — replaces tool list with find_tool/call_tool + filter — NEW: advertising filter decorator + composite tools — workflow engine sees ALL renamed tools (fix!) + base session — routing table built by factory with resolved names +``` + +The session factory directly orchestrates: per-backend overrides → conflict resolution → routing table construction → advertising set computation. No `Aggregator` object. The advertising filter becomes a session decorator that sits above composite tools, so composite tools see all tool schemas while clients only see advertised tools. + +### Detailed Design + +#### Session factory inlines aggregator pipeline + +`pkg/vmcp/session/factory.go` + +Merge `buildRoutingTable` and `buildRoutingTableWithAggregator` into a single function: + +```go +func buildRoutingTable( + ctx context.Context, + results []initResult, + conflictResolver aggregator.ConflictResolver, + toolConfigMap map[string]*config.WorkloadToolConfig, + excludeAllTools bool, +) (*vmcp.RoutingTable, []vmcp.Tool, []vmcp.Resource, []vmcp.Prompt, map[string]bool, error) +``` + +When `conflictResolver` or `toolConfigMap` is non-nil: + +1. Group tools by `BackendID` +2. Apply `aggregator.ProcessBackendTools()` per backend (overrides) +3. Call `conflictResolver.ResolveToolConflicts()` (conflict resolution) +4. Build routing table keyed by resolved names, with `OriginalCapabilityName` set via `aggregator.ActualBackendCapabilityName()` +5. Build `allTools` from ALL resolved tools (no filtering) +6. Compute `advertisedSet map[string]bool` keyed by resolved name — evaluates the filter logic inline + +When both are nil: current first-writer-wins logic, `advertisedSet = nil`. + +The `advertisedSet` is stored on `defaultMultiSession` as a private field. + +#### Filter decorator + +`pkg/vmcp/session/filterdec/decorator.go` (new) + +```go +type filterDecorator struct { + sessiontypes.MultiSession + advertisedSet map[string]bool +} + +func (d *filterDecorator) Tools() []vmcp.Tool { + all := d.MultiSession.Tools() + result := make([]vmcp.Tool, 0, len(all)) + for _, t := range all { + // Composite tools (empty BackendID) always pass through + if t.BackendID == "" || d.advertisedSet[t.Name] { + result = append(result, t) + } + } + return result +} +``` + +The decorator extracts `advertisedSet` from the base session via a private interface: + +```go +type advertisedSetProvider interface { + AdvertisedSet() map[string]bool +} +``` + +`CallTool`, `ReadResource`, `GetPrompt` pass through unchanged — the filter only affects what's advertised via `Tools()`, not what's callable. + +#### Decorator wiring + +`pkg/vmcp/server/sessionmanager/factory.go` (assumes PR #4231 landed) + +Insert filter decorator between composite tools and optimizer: + +```go +decorators = append(decorators, compositeToolsDecorator(...)) +decorators = append(decorators, filterDecoratorFn()) // extracts advertisedSet from session +decorators = append(decorators, optimizerDecoratorFn(...)) +``` + +#### Aggregator package becomes a utility library + +Keep: +- `ConflictResolver` interface + 3 implementations (prefix, priority, manual) +- `ProcessBackendTools` (exported) — applies per-backend overrides +- `ActualBackendCapabilityName` (exported) — reverses overrides for backend forwarding +- `ResolvedTool`, `BackendCapabilities`, `ResolvedCapabilities` types + +Delete: +- `Aggregator` interface, `defaultAggregator` struct +- `ProcessPreQueriedCapabilities`, `AggregateCapabilities`, `QueryCapabilities`, `QueryAllCapabilities`, `MergeCapabilities` +- `shouldAdvertiseTool` (logic inlined into factory) +- `AggregatedCapabilities`, `AggregationMetadata`, `BackendDiscoverer` types + +#### Why renaming can't be a decorator + +`defaultMultiSession.CallTool` uses the private `connections map[string]backend.Session` field for routing. A decorator wrapping `MultiSession` can't access this. If two backends both expose a tool named `fetch`, the base session's routing table (keyed by name) can only store one entry. A renaming decorator couldn't route `backend-a_fetch` to the correct backend by delegating `CallTool("fetch")` — the base session would route to whichever backend won first-writer-wins. The routing table must be built with conflict-resolved names at factory time. + +### API Changes + +**Removed**: `WithAggregator(agg aggregator.Aggregator)` session factory option + +**Added**: +- `WithConflictResolver(cr aggregator.ConflictResolver)` — optional conflict resolver +- `WithToolConfig(cfg *config.AggregationConfig)` — optional aggregation config (overrides, filters) + +Or a single `WithAggregationConfig` option that carries both. + +**Exported**: +- `aggregator.ProcessBackendTools` (was `processBackendTools`) +- `aggregator.ActualBackendCapabilityName` (was `actualBackendCapabilityName`) + +### Configuration Changes + +None. All existing configuration fields (`aggregation.excludeAllTools`, `aggregation.tools[].excludeAll`, `aggregation.tools[].filter`, `aggregation.tools[].overrides`) continue to work identically. The implementation moves from the aggregator to the factory + decorator. + +### Data Model Changes + +`defaultMultiSession` gains a private `advertisedSet map[string]bool` field. Not exposed on any public interface. + +## Security Considerations + +### Threat Model + +This change does not introduce new attack surface. The advertising filter continues to hide backend tools from clients — the mechanism changes (decorator vs aggregator) but the client-visible behavior is identical. + +### Authentication and Authorization + +No changes. Cedar authorization remains as HTTP middleware. The filter decorator operates below the auth layer. + +### Data Security + +No sensitive data handling changes. + +### Input Validation + +No new user input paths. + +### Secrets Management + +No changes. + +### Audit and Logging + +No changes. Filter decisions are evaluated at session creation time and are deterministic from the configuration. + +### Mitigations + +The `advertisedSet` is computed once at session creation and is immutable. The filter decorator is a simple set-membership check with no complex logic. + +## Alternatives Considered + +### Alternative 1: Add `AllRoutableTools()` to MultiSession interface + +- Description: Add a new method that returns all tools including non-advertised ones +- Pros: Minimal change, composite tools call the new method +- Cons: Bloats the `MultiSession` interface with a method that exists only to work around the filter's position. Rejected during design review. + +### Alternative 2: Store tool definitions on RoutingTable + +- Description: Add `ToolDefinitions map[string]vmcp.Tool` to `RoutingTable` +- Pros: Avoids interface changes +- Cons: Duplicates data already on `vmcp.Tool`. Creates redundant state that must be kept in sync. Rejected during design review. + +### Alternative 3: Filter decorator only (keep aggregator) + +- Description: Move only the advertising filter to a decorator, keep the aggregator for everything else +- Pros: Smallest change, fixes the bug +- Cons: The aggregator continues to exist as a confusing middleman with shrinking responsibilities. Its remaining role (overrides + conflict resolution + routing table) is pure session factory logic. + +### Alternative 4: Renaming as a session decorator + +- Description: Move all aggregator responsibilities including renaming into decorators +- Pros: Full decomposition, no aggregator at all +- Cons: Not feasible — `CallTool` routing requires private session state (`connections` map). A renaming decorator can't route calls to the correct backend when multiple backends share a tool name. + +## Compatibility + +### Backward Compatibility + +Fully backward compatible. All configuration fields work identically. Client-visible `tools/list` output is unchanged. The `Aggregator` interface is removed but it's an internal interface — no external consumers. + +### Forward Compatibility + +The decorator-based architecture makes it straightforward to add new session-level transforms (e.g. moving Cedar authorization from HTTP middleware to a decorator in the future). + +## Implementation Plan + +### Dependencies + +- PR #4231 (`issue-3872-v3`) must land first — it introduces `DecoratingFactory` and `buildDecoratingFactory` in `sessionmanager/factory.go` + +### Phase 1: Core changes + +1. Inline aggregator pipeline into `buildRoutingTable` in `pkg/vmcp/session/factory.go` +2. Export `ProcessBackendTools` and `ActualBackendCapabilityName` from aggregator package +3. Create filter decorator at `pkg/vmcp/session/filterdec/decorator.go` +4. Wire filter decorator in `pkg/vmcp/server/sessionmanager/factory.go` +5. Update server wiring in `cmd/vmcp/app/commands.go` and `pkg/vmcp/server/server.go` + +### Phase 2: Cleanup + +6. Delete `Aggregator` interface, `defaultAggregator`, and associated methods +7. Delete dead discovery types (`AggregatedCapabilities`, `BackendDiscoverer`, etc.) +8. Update tests + +## Testing Strategy + +- **Unit tests**: Filter decorator (`filterdec/decorator_test.go`) — `Tools()` filtering, composite tool pass-through, `CallTool` pass-through +- **Unit tests**: Unified `buildRoutingTable` — with/without conflict resolver, `advertisedSet` computation +- **Regression test**: Workflow engine coercion — non-advertised tool receives `float64(42)` not `"42"` (`composer/workflow_engine_test.go`) +- **Existing tests**: Conflict resolver tests remain unchanged. Aggregator tests for deleted methods are removed. + +```bash +go vet ./pkg/vmcp/... ./cmd/vmcp/... +go test ./pkg/vmcp/... ./cmd/vmcp/... +task lint-fix +``` + +## Documentation + +- Update `docs/arch/` if aggregator is referenced in architecture docs +- No user-facing documentation changes (configuration is unchanged) + +## Open Questions + +1. Should the `shouldAdvertiseTool` comment (incorrectly says "before overrides" but actually receives post-override names) be fixed as part of this work, or is it moot since the method is deleted? +2. Should we add a Filter+Override combined test before deleting the aggregator, to document the actual behavior? + +## References + +- [toolhive#4287](https://github.com/stacklok/toolhive/issues/4287) — Bug: composite tool type coercion fails for non-advertised tools +- [toolhive#4231](https://github.com/stacklok/toolhive/pull/4231) — PR: Move decoration into DecoratingFactory (dependency) +- [THV-0008](THV-0008-virtual-mcp-server.md) — Virtual MCP Server RFC + +--- + +## RFC Lifecycle + +### Review History + +| Date | Reviewer | Decision | Notes | +|------|----------|----------|-------| +| 2026-03-19 | @jerm-dro | Draft | Initial submission | + +### Implementation Tracking + +| Repository | PR | Status | +|------------|-----|--------| +| toolhive | TBD | Pending | From 5645d9e11de5305018b7c72df08f546ffe3843c0 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Thu, 19 Mar 2026 18:11:27 -0700 Subject: [PATCH 2/4] Address review comments on RFC-0058 - Remove nil conflict resolver path; always use default prefix strategy - Filter decorator evaluates directly on sess.Tools(), no advertisedSetProvider - WithConflictResolver always required in factory options - Move processBackendTools and actualBackendCapabilityName to session package - Expand backward compatibility table with all config fields - Remove moot open question about filter name matching - Rewrite testing strategy as feature combination matrix covering filter, renames, conflict resolution, composite tools, optimizer, and authn interactions across 11 E2E test scenarios Co-Authored-By: Claude Opus 4.6 --- ...0058-inline-aggregator-filter-decorator.md | 199 ++++++++++++++---- 1 file changed, 154 insertions(+), 45 deletions(-) diff --git a/rfcs/THV-0058-inline-aggregator-filter-decorator.md b/rfcs/THV-0058-inline-aggregator-filter-decorator.md index fce33e0..c3fd995 100644 --- a/rfcs/THV-0058-inline-aggregator-filter-decorator.md +++ b/rfcs/THV-0058-inline-aggregator-filter-decorator.md @@ -3,7 +3,7 @@ - **Status**: Draft - **Author(s)**: Jeremy Drouillard (@jerm-dro) - **Created**: 2026-03-19 -- **Last Updated**: 2026-03-19 +- **Last Updated**: 2026-03-20 - **Target Repository**: toolhive - **Related Issues**: [toolhive#4287](https://github.com/stacklok/toolhive/issues/4287) @@ -69,33 +69,33 @@ Merge `buildRoutingTable` and `buildRoutingTableWithAggregator` into a single fu func buildRoutingTable( ctx context.Context, results []initResult, - conflictResolver aggregator.ConflictResolver, + conflictResolver ConflictResolver, toolConfigMap map[string]*config.WorkloadToolConfig, excludeAllTools bool, -) (*vmcp.RoutingTable, []vmcp.Tool, []vmcp.Resource, []vmcp.Prompt, map[string]bool, error) +) (*vmcp.RoutingTable, []vmcp.Tool, []vmcp.Resource, []vmcp.Prompt, error) ``` -When `conflictResolver` or `toolConfigMap` is non-nil: +A conflict resolver is always provided — `NewConflictResolver(nil)` defaults to prefix strategy with `{workload}_` format, matching current behavior. The pipeline: 1. Group tools by `BackendID` -2. Apply `aggregator.ProcessBackendTools()` per backend (overrides) +2. Apply `processBackendTools()` per backend (overrides) 3. Call `conflictResolver.ResolveToolConflicts()` (conflict resolution) -4. Build routing table keyed by resolved names, with `OriginalCapabilityName` set via `aggregator.ActualBackendCapabilityName()` +4. Build routing table keyed by resolved names, with `OriginalCapabilityName` set via `actualBackendCapabilityName()` 5. Build `allTools` from ALL resolved tools (no filtering) -6. Compute `advertisedSet map[string]bool` keyed by resolved name — evaluates the filter logic inline -When both are nil: current first-writer-wins logic, `advertisedSet = nil`. - -The `advertisedSet` is stored on `defaultMultiSession` as a private field. +The old `buildRoutingTable` (first-writer-wins, no conflict resolution) is removed entirely — the conflict resolver always handles name conflicts. #### Filter decorator `pkg/vmcp/session/filterdec/decorator.go` (new) +The filter decorator evaluates the advertising filter directly on `sess.Tools()` using the tool config. Each tool in the session has `BackendID` set, which is sufficient to evaluate per-workload `excludeAll` and `filter` rules. The filter config references post-override tool names, which match `tool.Name` after conflict resolution when no prefix was applied, or can be matched by stripping the known prefix. + ```go type filterDecorator struct { sessiontypes.MultiSession - advertisedSet map[string]bool + toolConfigMap map[string]*config.WorkloadToolConfig + excludeAllTools bool } func (d *filterDecorator) Tools() []vmcp.Tool { @@ -103,7 +103,7 @@ func (d *filterDecorator) Tools() []vmcp.Tool { result := make([]vmcp.Tool, 0, len(all)) for _, t := range all { // Composite tools (empty BackendID) always pass through - if t.BackendID == "" || d.advertisedSet[t.Name] { + if t.BackendID == "" || d.shouldAdvertise(t) { result = append(result, t) } } @@ -111,40 +111,35 @@ func (d *filterDecorator) Tools() []vmcp.Tool { } ``` -The decorator extracts `advertisedSet` from the base session via a private interface: - -```go -type advertisedSetProvider interface { - AdvertisedSet() map[string]bool -} -``` - `CallTool`, `ReadResource`, `GetPrompt` pass through unchanged — the filter only affects what's advertised via `Tools()`, not what's callable. #### Decorator wiring `pkg/vmcp/server/sessionmanager/factory.go` (assumes PR #4231 landed) -Insert filter decorator between composite tools and optimizer: +Insert filter decorator between composite tools and optimizer. The filter decorator is always applied — it receives the tool config and evaluates advertising decisions against `sess.Tools()` at decoration time: ```go decorators = append(decorators, compositeToolsDecorator(...)) -decorators = append(decorators, filterDecoratorFn()) // extracts advertisedSet from session +decorators = append(decorators, filterDecoratorFn(toolConfigMap, excludeAllTools)) decorators = append(decorators, optimizerDecoratorFn(...)) ``` #### Aggregator package becomes a utility library -Keep: +Keep in aggregator package: - `ConflictResolver` interface + 3 implementations (prefix, priority, manual) -- `ProcessBackendTools` (exported) — applies per-backend overrides -- `ActualBackendCapabilityName` (exported) — reverses overrides for backend forwarding +- `NewConflictResolver` factory function - `ResolvedTool`, `BackendCapabilities`, `ResolvedCapabilities` types +Move to session package (implementation detail): +- `processBackendTools` — used only by the session factory +- `actualBackendCapabilityName` — used only by the session factory + Delete: - `Aggregator` interface, `defaultAggregator` struct - `ProcessPreQueriedCapabilities`, `AggregateCapabilities`, `QueryCapabilities`, `QueryAllCapabilities`, `MergeCapabilities` -- `shouldAdvertiseTool` (logic inlined into factory) +- `shouldAdvertiseTool` (logic moves to filter decorator) - `AggregatedCapabilities`, `AggregationMetadata`, `BackendDiscoverer` types #### Why renaming can't be a decorator @@ -156,14 +151,11 @@ Delete: **Removed**: `WithAggregator(agg aggregator.Aggregator)` session factory option **Added**: -- `WithConflictResolver(cr aggregator.ConflictResolver)` — optional conflict resolver -- `WithToolConfig(cfg *config.AggregationConfig)` — optional aggregation config (overrides, filters) - -Or a single `WithAggregationConfig` option that carries both. +- `WithAggregationConfig(cfg *config.AggregationConfig, cr ConflictResolver)` — required conflict resolver and aggregation config. The conflict resolver always exists (`NewConflictResolver` defaults to prefix strategy). -**Exported**: -- `aggregator.ProcessBackendTools` (was `processBackendTools`) -- `aggregator.ActualBackendCapabilityName` (was `actualBackendCapabilityName`) +**Moved to session package** (implementation detail, not exported): +- `processBackendTools` — moves from `aggregator` to `session` package +- `actualBackendCapabilityName` — moves from `aggregator` to `session` package ### Configuration Changes @@ -171,7 +163,7 @@ None. All existing configuration fields (`aggregation.excludeAllTools`, `aggrega ### Data Model Changes -`defaultMultiSession` gains a private `advertisedSet map[string]bool` field. Not exposed on any public interface. +None. The filter decorator holds the tool config directly and evaluates advertising decisions at decoration time against `sess.Tools()`. ## Security Considerations @@ -233,7 +225,17 @@ The `advertisedSet` is computed once at session creation and is immutable. The f ### Backward Compatibility -Fully backward compatible. All configuration fields work identically. Client-visible `tools/list` output is unchanged. The `Aggregator` interface is removed but it's an internal interface — no external consumers. +All configuration fields work identically. Client-visible behavior is unchanged. The `Aggregator` interface is removed but it's internal — no external consumers. + +**Feature-by-feature analysis of the new decorator ordering:** + +| Feature | Current flow | New flow | Impact | +|---|---|---|---| +| **Authentication (Cedar)** | HTTP middleware intercepts `tools/list` and `tools/call` at the JSON-RPC level, evaluates Cedar policies against resolved tool names | Unchanged — Cedar middleware sits above the entire session decorator stack, sees the same resolved tool names in requests/responses | None | +| **Composite tool calling** | Workflow engine receives `sess.Tools()` (advertised only) and `sess.GetRoutingTable()`. Calls `backendClient.CallTool()` directly (bypasses session decorators). `getToolInputSchema()` fails for non-advertised tools | Workflow engine receives `sess.Tools()` (ALL tools, since composite tools decorator sits below the filter). `backendClient.CallTool()` still bypasses decorators. `getToolInputSchema()` now finds schemas for all tools | **Bug fix** — coercion works for non-advertised tools | +| **Advertising filter** | Aggregator applies `shouldAdvertiseTool` before session creation. `sess.Tools()` returns only advertised tools | Filter decorator applies after composite tools. `sess.Tools()` at the filter level returns only advertised tools + composite tools. Below the filter (where composite tools operate), `sess.Tools()` returns all tools | Same client-visible result; composite tools see more | +| **Optimizer** | Wraps session after composite tools, indexes `sess.Tools()` (advertised + composite) into find_tool/call_tool | Wraps session after filter, indexes `sess.Tools()` (advertised + composite) — same tools as before | None | +| **Tool call routing** | `defaultMultiSession.CallTool` looks up routing table → `GetBackendCapabilityName()` → backend HTTP call | Unchanged — routing table is built with the same resolved names and `OriginalCapabilityName` values, just by the factory instead of the aggregator | None | ### Forward Compatibility @@ -247,8 +249,8 @@ The decorator-based architecture makes it straightforward to add new session-lev ### Phase 1: Core changes -1. Inline aggregator pipeline into `buildRoutingTable` in `pkg/vmcp/session/factory.go` -2. Export `ProcessBackendTools` and `ActualBackendCapabilityName` from aggregator package +1. Move `processBackendTools` and `actualBackendCapabilityName` from aggregator to session package +2. Inline aggregator pipeline into `buildRoutingTable` in `pkg/vmcp/session/factory.go` — always use conflict resolver 3. Create filter decorator at `pkg/vmcp/session/filterdec/decorator.go` 4. Wire filter decorator in `pkg/vmcp/server/sessionmanager/factory.go` 5. Update server wiring in `cmd/vmcp/app/commands.go` and `pkg/vmcp/server/server.go` @@ -256,20 +258,128 @@ The decorator-based architecture makes it straightforward to add new session-lev ### Phase 2: Cleanup 6. Delete `Aggregator` interface, `defaultAggregator`, and associated methods -7. Delete dead discovery types (`AggregatedCapabilities`, `BackendDiscoverer`, etc.) -8. Update tests +7. Delete dead types (`AggregatedCapabilities`, `BackendDiscoverer`, etc.) +8. Delete aggregator tests for removed methods; add integration tests on MultiSession construction/decoration ## Testing Strategy -- **Unit tests**: Filter decorator (`filterdec/decorator_test.go`) — `Tools()` filtering, composite tool pass-through, `CallTool` pass-through -- **Unit tests**: Unified `buildRoutingTable` — with/without conflict resolver, `advertisedSet` computation -- **Regression test**: Workflow engine coercion — non-advertised tool receives `float64(42)` not `"42"` (`composer/workflow_engine_test.go`) -- **Existing tests**: Conflict resolver tests remain unchanged. Aggregator tests for deleted methods are removed. +All validation is at the K8s/Ginkgo E2E level (`test/e2e/thv-operator/virtualmcp/`). Tests deploy real MCPServers + VirtualMCPServers to a Kind cluster and exercise the full stack via MCP clients. + +### Feature combination matrix + +The features being modified interact with each other. The matrix below maps every relevant combination to an E2E test — either an existing one that provides coverage or a new one that must be written. + +**Features**: **F** = Filter (per-workload), **EA** = ExcludeAll (per-workload or global), **R** = Renames/Overrides, **CR** = Conflict Resolution, **CT** = Composite Tools, **O** = Optimizer, **A** = Authn (non-anonymous) + +| # | F | EA | R | CR | CT | O | A | E2E test | Status | +|---|---|---|---|---|---|---|---|----------|--------| +| 1 | | G | | prefix | | | anon | `virtualmcp_excludeall_global_test.go` | Existing | +| 2 | Y | Y | | prefix | | | anon | `virtualmcp_aggregation_filtering_test.go` | Existing | +| 3 | Y | Y | | prefix | Y | | anon | `virtualmcp_composite_hidden_tools_test.go` | Existing | +| 4 | | | | prefix/priority/manual | | | anon | `virtualmcp_conflict_resolution_test.go` | Existing | +| 5 | | | Y | prefix | | | anon | `virtualmcp_aggregation_overrides_test.go` | Existing | +| 6 | Y | | Y | prefix | | | anon | `virtualmcp_toolconfig_test.go` (MCPToolConfig CRD) | Existing | +| 7 | | | Y | prefix | Y | Y | anon | `virtualmcp_optimizer_composite_test.go` | Existing | +| 8 | Y | Y | | prefix | Y | | anon | **`virtualmcp_composite_coercion_test.go`** | **New** | +| 9 | Y | | Y | prefix | | | anon | **`virtualmcp_overrides_filter_test.go`** | **New** | +| 10 | Y | Y | Y | prefix | Y | Y | anon | **`virtualmcp_full_stack_test.go`** | **New** | +| 11 | | | | prefix | | | token | `virtualmcp_external_auth_test.go` | Existing | + +**Legend**: G = global ExcludeAllTools, Y = feature active in test, blank = not exercised. + +### Existing tests (regression gates) + +All existing tests in the matrix must pass without modification after the refactor. Any failure is a regression. + +- **Row 1**: Global `ExcludeAllTools: true` hides all backend tools; MCP server still responds. +- **Row 2**: Per-workload `Filter` + `ExcludeAll` applied to different backends; only filtered tools exposed. +- **Row 3**: Composite tool calls hidden backend tools (ExcludeAll + Filter). Validates #3636 fix. Only tests string params — does NOT exercise type coercion. +- **Row 4**: Prefix, priority, and manual conflict resolution with two backends sharing tool names. +- **Row 5**: Tool name/description overrides; overridden tool callable by new name. +- **Row 6**: MCPToolConfig CRD drives filtering + overrides simultaneously. +- **Row 7**: Optimizer wraps composite tools + overridden backend tools through `find_tool`/`call_tool`. +- **Row 11**: Token-based incoming auth with backend routing. + +### New E2E tests + +#### Row 8: Composite tool type coercion with hidden tools (regression test for #4287) + +**File**: `test/e2e/thv-operator/virtualmcp/virtualmcp_composite_coercion_test.go` + +The specific bug this RFC fixes. When a composite tool workflow step calls a hidden backend tool with a numeric parameter, `getToolInputSchema()` fails to find the schema, skips coercion, and the backend rejects `"42"` (string) instead of `float64(42)`. + +**Setup**: +- Backend with a tool accepting an integer parameter (requires adding an `add` tool to yardstick: `{"a": integer, "b": integer}` → returns sum) +- `ExcludeAll: true` for the backend +- Composite tool whose workflow step calls the hidden tool via template expansion (`"{{ .params.a }}"`) + +**Assertions**: +- `tools/list` returns only the composite tool +- Calling the composite tool succeeds — backend receives coerced numeric args +- Response contains correct output (the sum), proving type coercion worked + +**Must fail before the refactor, pass after.** + +#### Row 9: Overrides + filter on the same backend + +**File**: `test/e2e/thv-operator/virtualmcp/virtualmcp_overrides_filter_test.go` + +No existing test exercises overrides and filter together. The `shouldAdvertiseTool` comment bug (says "before overrides" but receives post-override name) was never caught because this combination is untested. + +**Setup**: +- Backend with tool `echo`, overridden to `custom_echo` +- Filter configured with `["echo"]` (pre-override name) + +**Assertions**: +- Document whether the filter matches pre- or post-override names +- Verify the overridden tool is callable by its new name +- Lock in the actual behavior so the refactor preserves it + +#### Row 10: Full stack — all features combined + +**File**: `test/e2e/thv-operator/virtualmcp/virtualmcp_full_stack_test.go` + +Exercises the entire decorator stack with all features active simultaneously. + +**Setup**: +- Two backends with conflicting tool names (prefix conflict resolution) +- Backend A: tool overridden (`echo` → `custom_echo`), plus filter hiding some tools +- Backend B: `ExcludeAll: true` +- Composite tool calling tools from both backends (including hidden + overridden) +- Optimizer enabled + +**Assertions**: +- `tools/list` via optimizer returns only `find_tool`/`call_tool` +- `find_tool` returns the composite tool + filtered backend A tools (not excluded backend B tools, not hidden backend A tools) +- `call_tool` with composite tool succeeds — reaches both backends with correct original names +- Overridden tool callable by its new name through the optimizer +- Backend B tools reachable via composite tool despite `ExcludeAll` + +### Unit tests for the filter decorator + +**File**: `pkg/vmcp/session/filterdec/decorator_test.go` + +Lightweight unit tests for the new filter decorator in isolation: +- `Tools()` with `excludeAll` → only composite tools returned +- `Tools()` with `filter` → only matching backend tools + composite tools +- `CallTool` passes through for both advertised and non-advertised tools +- No filter config → all tools pass through + +### Cleanup + +- Delete aggregator unit tests for removed methods (`ProcessPreQueriedCapabilities`, `MergeCapabilities`, `shouldAdvertiseTool`) +- Conflict resolver unit tests remain unchanged + +### Verification ```bash +# Unit tests go vet ./pkg/vmcp/... ./cmd/vmcp/... go test ./pkg/vmcp/... ./cmd/vmcp/... task lint-fix + +# E2E tests (Kind cluster) +task test-e2e ``` ## Documentation @@ -279,8 +389,7 @@ task lint-fix ## Open Questions -1. Should the `shouldAdvertiseTool` comment (incorrectly says "before overrides" but actually receives post-override names) be fixed as part of this work, or is it moot since the method is deleted? -2. Should we add a Filter+Override combined test before deleting the aggregator, to document the actual behavior? +None — all design questions resolved during review. ## References From 1e3dacdeee332d022df026444b8fc90661f1baa0 Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Thu, 19 Mar 2026 20:47:38 -0700 Subject: [PATCH 3/4] Simplify workflow engine by routing through session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workflow engine directly uses Router + BackendClient, bypassing the session's CallTool entirely. Replace with sess.CallTool() and sess.Tools() — the session already handles routing and backend dispatch. This eliminates the abstraction leak and naturally fixes #4287 since sess.Tools() below the filter decorator includes all tool schemas. Co-Authored-By: Claude Opus 4.6 --- ...0058-inline-aggregator-filter-decorator.md | 47 ++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/rfcs/THV-0058-inline-aggregator-filter-decorator.md b/rfcs/THV-0058-inline-aggregator-filter-decorator.md index c3fd995..7027ed7 100644 --- a/rfcs/THV-0058-inline-aggregator-filter-decorator.md +++ b/rfcs/THV-0058-inline-aggregator-filter-decorator.md @@ -19,6 +19,10 @@ When backend tools are excluded from advertising via `excludeAll` or `filter`, c The root cause: the advertising filter runs inside the aggregator **before** session creation. The session's `tools` field only contains advertised tools. The composite tools decorator builds its workflow engine from `sess.Tools()`, so it never sees schemas for non-advertised tools. +### Abstraction leak: workflow engine bypasses the session + +The workflow engine directly uses `Router.RouteTool()` to resolve a `BackendTarget`, then calls `BackendClient.CallTool()` with that target — completely bypassing the session's `CallTool` method and every decorator in the stack. The session already encapsulates routing + backend dispatch in its own `CallTool`, making the engine's direct use of `Router` and `BackendClient` redundant. This leak is why `getToolInputSchema()` must maintain its own tool list rather than relying on the session — the engine doesn't call through the session at all. + ### Architectural issue: the aggregator is a monolithic middleman The `Aggregator` interface has 7 responsibilities: backend capability querying, per-backend overrides, conflict resolution, advertising filtering, routing table construction, name reversal, and discovery pipeline orchestration. It sits between the HTTP middleware and the session object — operating on data that logically belongs to session construction. @@ -32,6 +36,7 @@ As the aggregator's responsibilities shift (e.g. removing the filter), it become - Fix composite tool type coercion for non-advertised backend tools - Eliminate the `Aggregator` interface and `defaultAggregator` struct - Move advertising filtering to a session decorator in the correct position in the decorator stack +- Simplify the workflow engine by routing tool calls through the session instead of directly using `Router` + `BackendClient` - Keep conflict resolvers and per-backend override utilities as a reusable library - No breaking changes to configuration or client-visible behavior @@ -57,6 +62,8 @@ Decorator stack (top → bottom): The session factory directly orchestrates: per-backend overrides → conflict resolution → routing table construction → advertising set computation. No `Aggregator` object. The advertising filter becomes a session decorator that sits above composite tools, so composite tools see all tool schemas while clients only see advertised tools. +The workflow engine is simplified: instead of directly using `Router` and `BackendClient` to dispatch tool calls, it calls `sess.CallTool()`. The session already handles routing and backend dispatch. This eliminates the abstraction leak and means the engine's `getToolInputSchema()` can use `sess.Tools()` — which, at the composite tools layer (below the filter), includes all tools. + ### Detailed Design #### Session factory inlines aggregator pipeline @@ -113,6 +120,31 @@ func (d *filterDecorator) Tools() []vmcp.Tool { `CallTool`, `ReadResource`, `GetPrompt` pass through unchanged — the filter only affects what's advertised via `Tools()`, not what's callable. +#### Workflow engine calls through the session + +`pkg/vmcp/composer/workflow_engine.go` + +The workflow engine currently takes a `Router` and `BackendClient` and dispatches tool calls directly: + +```go +// Current: bypasses the session entirely +target, err := e.router.RouteTool(ctx, step.Tool) +result, err := e.backendClient.CallTool(ctx, target, step.Tool, args, nil) +``` + +This is replaced with a call through the session: + +```go +// New: routes through the session's CallTool +result, err := e.session.CallTool(ctx, step.Tool, args, nil) +``` + +The session already handles routing table lookup, backend capability name reversal, and backend dispatch. The engine no longer needs `Router` or `BackendClient` interfaces — it only needs a reference to the `MultiSession` it decorates. + +Similarly, `getToolInputSchema()` currently maintains its own `e.tools` list. It can instead call `e.session.Tools()`, which at the composite tools layer (below the filter) returns all tools including non-advertised ones. This naturally fixes the #4287 bug: the schema for every backend tool is available regardless of advertising. + +The `ResolveToolName` call in `getToolInputSchema` (for dot-convention support) moves to the session's `CallTool` implementation, which already handles this via the session router. + #### Decorator wiring `pkg/vmcp/server/sessionmanager/factory.go` (assumes PR #4231 landed) @@ -148,7 +180,9 @@ Delete: ### API Changes -**Removed**: `WithAggregator(agg aggregator.Aggregator)` session factory option +**Removed**: +- `WithAggregator(agg aggregator.Aggregator)` session factory option +- `Router` and `BackendClient` dependencies from the workflow engine (tool calls route through the session) **Added**: - `WithAggregationConfig(cfg *config.AggregationConfig, cr ConflictResolver)` — required conflict resolver and aggregation config. The conflict resolver always exists (`NewConflictResolver` defaults to prefix strategy). @@ -232,7 +266,7 @@ All configuration fields work identically. Client-visible behavior is unchanged. | Feature | Current flow | New flow | Impact | |---|---|---|---| | **Authentication (Cedar)** | HTTP middleware intercepts `tools/list` and `tools/call` at the JSON-RPC level, evaluates Cedar policies against resolved tool names | Unchanged — Cedar middleware sits above the entire session decorator stack, sees the same resolved tool names in requests/responses | None | -| **Composite tool calling** | Workflow engine receives `sess.Tools()` (advertised only) and `sess.GetRoutingTable()`. Calls `backendClient.CallTool()` directly (bypasses session decorators). `getToolInputSchema()` fails for non-advertised tools | Workflow engine receives `sess.Tools()` (ALL tools, since composite tools decorator sits below the filter). `backendClient.CallTool()` still bypasses decorators. `getToolInputSchema()` now finds schemas for all tools | **Bug fix** — coercion works for non-advertised tools | +| **Composite tool calling** | Workflow engine receives `sess.Tools()` (advertised only) and `sess.GetRoutingTable()`. Calls `backendClient.CallTool()` directly (bypasses session decorators). `getToolInputSchema()` fails for non-advertised tools | Workflow engine calls `sess.CallTool()` — routes through the session instead of bypassing it. `getToolInputSchema()` uses `sess.Tools()` which, below the filter decorator, includes all tools | **Bug fix** — coercion works for non-advertised tools. **Simplification** — engine no longer depends on `Router` or `BackendClient` | | **Advertising filter** | Aggregator applies `shouldAdvertiseTool` before session creation. `sess.Tools()` returns only advertised tools | Filter decorator applies after composite tools. `sess.Tools()` at the filter level returns only advertised tools + composite tools. Below the filter (where composite tools operate), `sess.Tools()` returns all tools | Same client-visible result; composite tools see more | | **Optimizer** | Wraps session after composite tools, indexes `sess.Tools()` (advertised + composite) into find_tool/call_tool | Wraps session after filter, indexes `sess.Tools()` (advertised + composite) — same tools as before | None | | **Tool call routing** | `defaultMultiSession.CallTool` looks up routing table → `GetBackendCapabilityName()` → backend HTTP call | Unchanged — routing table is built with the same resolved names and `OriginalCapabilityName` values, just by the factory instead of the aggregator | None | @@ -253,13 +287,14 @@ The decorator-based architecture makes it straightforward to add new session-lev 2. Inline aggregator pipeline into `buildRoutingTable` in `pkg/vmcp/session/factory.go` — always use conflict resolver 3. Create filter decorator at `pkg/vmcp/session/filterdec/decorator.go` 4. Wire filter decorator in `pkg/vmcp/server/sessionmanager/factory.go` -5. Update server wiring in `cmd/vmcp/app/commands.go` and `pkg/vmcp/server/server.go` +5. Simplify workflow engine: replace `Router` + `BackendClient` with `sess.CallTool()`; replace `e.tools` with `sess.Tools()` +6. Update server wiring in `cmd/vmcp/app/commands.go` and `pkg/vmcp/server/server.go` ### Phase 2: Cleanup -6. Delete `Aggregator` interface, `defaultAggregator`, and associated methods -7. Delete dead types (`AggregatedCapabilities`, `BackendDiscoverer`, etc.) -8. Delete aggregator tests for removed methods; add integration tests on MultiSession construction/decoration +7. Delete `Aggregator` interface, `defaultAggregator`, and associated methods +8. Delete dead types (`AggregatedCapabilities`, `BackendDiscoverer`, etc.) +9. Delete aggregator tests for removed methods; remove `Router` and `BackendClient` interfaces from workflow engine ## Testing Strategy From ccaa02ce0ad259e2535fc176ffd1781ebdeaa47a Mon Sep 17 00:00:00 2001 From: Jeremy Drouillard Date: Fri, 20 Mar 2026 11:07:17 -0700 Subject: [PATCH 4/4] Move new tests to in-process MultiSession level New tests that don't involve auth use real in-process MCP backends via httptest.NewServer() instead of K8s. This constructs the full decorator stack (factory, conflict resolution, filter, composite tools, optimizer) without containers or a Kind cluster. Follows the existing pattern in connector_integration_test.go. K8s E2E tests remain as regression gates for auth-dependent flows and CRD-driven configuration. Co-Authored-By: Claude Opus 4.6 --- ...0058-inline-aggregator-filter-decorator.md | 78 +++++++++++-------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/rfcs/THV-0058-inline-aggregator-filter-decorator.md b/rfcs/THV-0058-inline-aggregator-filter-decorator.md index 7027ed7..9f5a39b 100644 --- a/rfcs/THV-0058-inline-aggregator-filter-decorator.md +++ b/rfcs/THV-0058-inline-aggregator-filter-decorator.md @@ -298,33 +298,35 @@ The decorator-based architecture makes it straightforward to add new session-lev ## Testing Strategy -All validation is at the K8s/Ginkgo E2E level (`test/e2e/thv-operator/virtualmcp/`). Tests deploy real MCPServers + VirtualMCPServers to a Kind cluster and exercise the full stack via MCP clients. +New tests that don't involve auth are written as in-process integration tests at the `MultiSession` level, using real MCP backends hosted via `httptest.NewServer()`. This constructs the full vMCP stack (factory → conflict resolution → decorators → tool calls) without a K8s dependency. The existing pattern lives in `pkg/vmcp/session/connector_integration_test.go` — `startInProcessMCPServer()` creates real MCP servers with registered tools, and the session factory connects to them with full capability discovery. + +K8s/Ginkgo E2E tests (`test/e2e/thv-operator/virtualmcp/`) remain the regression gates for auth-dependent flows and CRD-driven configuration. ### Feature combination matrix -The features being modified interact with each other. The matrix below maps every relevant combination to an E2E test — either an existing one that provides coverage or a new one that must be written. +The features being modified interact with each other. The matrix below maps every relevant combination to a test — either an existing one that provides coverage or a new one that must be written. **Features**: **F** = Filter (per-workload), **EA** = ExcludeAll (per-workload or global), **R** = Renames/Overrides, **CR** = Conflict Resolution, **CT** = Composite Tools, **O** = Optimizer, **A** = Authn (non-anonymous) -| # | F | EA | R | CR | CT | O | A | E2E test | Status | -|---|---|---|---|---|---|---|---|----------|--------| -| 1 | | G | | prefix | | | anon | `virtualmcp_excludeall_global_test.go` | Existing | -| 2 | Y | Y | | prefix | | | anon | `virtualmcp_aggregation_filtering_test.go` | Existing | -| 3 | Y | Y | | prefix | Y | | anon | `virtualmcp_composite_hidden_tools_test.go` | Existing | -| 4 | | | | prefix/priority/manual | | | anon | `virtualmcp_conflict_resolution_test.go` | Existing | -| 5 | | | Y | prefix | | | anon | `virtualmcp_aggregation_overrides_test.go` | Existing | -| 6 | Y | | Y | prefix | | | anon | `virtualmcp_toolconfig_test.go` (MCPToolConfig CRD) | Existing | -| 7 | | | Y | prefix | Y | Y | anon | `virtualmcp_optimizer_composite_test.go` | Existing | -| 8 | Y | Y | | prefix | Y | | anon | **`virtualmcp_composite_coercion_test.go`** | **New** | -| 9 | Y | | Y | prefix | | | anon | **`virtualmcp_overrides_filter_test.go`** | **New** | -| 10 | Y | Y | Y | prefix | Y | Y | anon | **`virtualmcp_full_stack_test.go`** | **New** | -| 11 | | | | prefix | | | token | `virtualmcp_external_auth_test.go` | Existing | +| # | F | EA | R | CR | CT | O | A | Test | Level | Status | +|---|---|---|---|---|---|---|---|------|-------|--------| +| 1 | | G | | prefix | | | anon | `virtualmcp_excludeall_global_test.go` | K8s E2E | Existing | +| 2 | Y | Y | | prefix | | | anon | `virtualmcp_aggregation_filtering_test.go` | K8s E2E | Existing | +| 3 | Y | Y | | prefix | Y | | anon | `virtualmcp_composite_hidden_tools_test.go` | K8s E2E | Existing | +| 4 | | | | prefix/priority/manual | | | anon | `virtualmcp_conflict_resolution_test.go` | K8s E2E | Existing | +| 5 | | | Y | prefix | | | anon | `virtualmcp_aggregation_overrides_test.go` | K8s E2E | Existing | +| 6 | Y | | Y | prefix | | | anon | `virtualmcp_toolconfig_test.go` (MCPToolConfig CRD) | K8s E2E | Existing | +| 7 | | | Y | prefix | Y | Y | anon | `virtualmcp_optimizer_composite_test.go` | K8s E2E | Existing | +| 8 | Y | Y | | prefix | Y | | | **composite_coercion_test.go** | **In-process** | **New** | +| 9 | Y | | Y | prefix | | | | **overrides_filter_test.go** | **In-process** | **New** | +| 10 | Y | Y | Y | prefix | Y | Y | | **full_stack_test.go** | **In-process** | **New** | +| 11 | | | | prefix | | | token | `virtualmcp_external_auth_test.go` | K8s E2E | Existing | **Legend**: G = global ExcludeAllTools, Y = feature active in test, blank = not exercised. -### Existing tests (regression gates) +### Existing K8s E2E tests (regression gates) -All existing tests in the matrix must pass without modification after the refactor. Any failure is a regression. +All existing tests must pass without modification after the refactor. Any failure is a regression. - **Row 1**: Global `ExcludeAllTools: true` hides all backend tools; MCP server still responds. - **Row 2**: Per-workload `Filter` + `ExcludeAll` applied to different backends; only filtered tools exposed. @@ -335,58 +337,66 @@ All existing tests in the matrix must pass without modification after the refact - **Row 7**: Optimizer wraps composite tools + overridden backend tools through `find_tool`/`call_tool`. - **Row 11**: Token-based incoming auth with backend routing. -### New E2E tests +### New in-process integration tests + +These tests construct the full decorator stack against real in-process MCP backends (via `httptest.NewServer()` + `mcp-go`). No K8s, no containers — the session factory connects to backends over HTTP, discovers capabilities, applies conflict resolution + overrides, and wires the decorator chain. Tests call through the decorated `MultiSession` interface. + +**Location**: `pkg/vmcp/session/` (alongside existing `connector_integration_test.go`) #### Row 8: Composite tool type coercion with hidden tools (regression test for #4287) -**File**: `test/e2e/thv-operator/virtualmcp/virtualmcp_composite_coercion_test.go` +**File**: `pkg/vmcp/session/composite_coercion_integration_test.go` The specific bug this RFC fixes. When a composite tool workflow step calls a hidden backend tool with a numeric parameter, `getToolInputSchema()` fails to find the schema, skips coercion, and the backend rejects `"42"` (string) instead of `float64(42)`. **Setup**: -- Backend with a tool accepting an integer parameter (requires adding an `add` tool to yardstick: `{"a": integer, "b": integer}` → returns sum) -- `ExcludeAll: true` for the backend -- Composite tool whose workflow step calls the hidden tool via template expansion (`"{{ .params.a }}"`) +- In-process MCP backend with a tool accepting integer parameters (e.g., `add` tool: `{"a": integer, "b": integer}` → returns sum) +- Session factory with `ExcludeAll: true` for the backend +- Composite tools decorator applied with a workflow step that calls the hidden tool via template expansion (`"{{ .params.a }}"`) +- Filter decorator applied above composite tools **Assertions**: -- `tools/list` returns only the composite tool -- Calling the composite tool succeeds — backend receives coerced numeric args +- `sess.Tools()` at top of stack returns only the composite tool +- `sess.CallTool()` with the composite tool succeeds — backend receives coerced numeric args - Response contains correct output (the sum), proving type coercion worked **Must fail before the refactor, pass after.** #### Row 9: Overrides + filter on the same backend -**File**: `test/e2e/thv-operator/virtualmcp/virtualmcp_overrides_filter_test.go` +**File**: `pkg/vmcp/session/overrides_filter_integration_test.go` No existing test exercises overrides and filter together. The `shouldAdvertiseTool` comment bug (says "before overrides" but receives post-override name) was never caught because this combination is untested. **Setup**: -- Backend with tool `echo`, overridden to `custom_echo` +- In-process MCP backend with tool `echo` +- Override config: `echo` → `custom_echo` - Filter configured with `["echo"]` (pre-override name) +- Session constructed with full decorator chain **Assertions**: - Document whether the filter matches pre- or post-override names -- Verify the overridden tool is callable by its new name +- Verify the overridden tool is callable by its new name via `sess.CallTool()` - Lock in the actual behavior so the refactor preserves it #### Row 10: Full stack — all features combined -**File**: `test/e2e/thv-operator/virtualmcp/virtualmcp_full_stack_test.go` +**File**: `pkg/vmcp/session/full_stack_integration_test.go` Exercises the entire decorator stack with all features active simultaneously. **Setup**: -- Two backends with conflicting tool names (prefix conflict resolution) +- Two in-process MCP backends with conflicting tool names (prefix conflict resolution) - Backend A: tool overridden (`echo` → `custom_echo`), plus filter hiding some tools - Backend B: `ExcludeAll: true` - Composite tool calling tools from both backends (including hidden + overridden) - Optimizer enabled +- Full decorator chain: base session → composite tools → filter → optimizer **Assertions**: -- `tools/list` via optimizer returns only `find_tool`/`call_tool` -- `find_tool` returns the composite tool + filtered backend A tools (not excluded backend B tools, not hidden backend A tools) -- `call_tool` with composite tool succeeds — reaches both backends with correct original names +- `sess.Tools()` via optimizer returns only `find_tool`/`call_tool` +- `sess.CallTool("find_tool", ...)` returns the composite tool + filtered backend A tools (not excluded backend B tools) +- `sess.CallTool("call_tool", {tool_name: compositeToolName, ...})` succeeds — reaches both backends with correct original names - Overridden tool callable by its new name through the optimizer - Backend B tools reachable via composite tool despite `ExcludeAll` @@ -408,12 +418,12 @@ Lightweight unit tests for the new filter decorator in isolation: ### Verification ```bash -# Unit tests +# Unit + in-process integration tests go vet ./pkg/vmcp/... ./cmd/vmcp/... go test ./pkg/vmcp/... ./cmd/vmcp/... task lint-fix -# E2E tests (Kind cluster) +# K8s E2E tests (Kind cluster — regression gates) task test-e2e ```