Skip to content

fix(binding-mcp-openapi): resolve exit per route instead of one binding-wide value - #2565

Merged
jfallows merged 3 commits into
developfrom
claude/kind-brown-j38mac
Sep 10, 2026
Merged

fix(binding-mcp-openapi): resolve exit per route instead of one binding-wide value#2565
jfallows merged 3 commits into
developfrom
claude/kind-brown-j38mac

Conversation

@jfallows

@jfallows jfallows commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Description

Repurposed from the original "lock down the schema" approach — see discussion below for why.

Follow-up to #2563. That PR made the kind: proxy exit requirement internally consistent, but the underlying implementation still only ever resolved a single binding-wide exit, silently discarding any other value declared on a route (findFirst() over all routes, applied uniformly to every generated mcp-http0 route via McpOpenapiCompositeGenerator.injectRoutes()).

The original fix for this (commit dee960ff, since reverted here) tightened the schema to forbid routes[].exit entirely and require exit only at the binding level — mirroring binding-mcp-kafka-connect/binding-mcp-schema-registry. That approach was rejected: it's a backward-incompatible schema change — any existing config using route-level exit (a shape #2563 explicitly allows and requires in the no-binding-level-exit case) would fail validation outright. Comparing against binding-mcp-kafka, which genuinely supports different routes exiting to different downstream bindings (some tools bypass its local cache and go to a live Kafka client, others don't — a real structural need, not just permissiveness), it's more correct to fix mcp-openapi's implementation to actually deliver on what its schema already promises, rather than restricting the schema to match a buggy implementation.

This PR instead implements genuine per-route exit resolution, keeping the schema exactly as merged in #2563:

  • Fixed a live regression along the way: McpOpenapiBindingConfig.routes filtered on route.exit == null to exclude the engine-synthesized catch-all route (for the top-level exit: shorthand). But since real routes can also carry their own exit (per #2563's schema), that filter also excluded any real route with its own exit — silently dropping its with/when from the generated composite. A proxy config using only per-route exits (no binding-level exit) currently produces an empty mcp-http0 binding with zero tools/resources. Fixed by identifying the synthetic route via with == null instead (every real mcp-openapi route always has with; the synthetic one never does).
  • McpOpenapiRouteConfig now carries its own resolved exit.
  • McpOpenapiCompositeGenerator resolves each RoutedOperation's exit individually — the originating route's own exit, falling back to binding.exit — and applies it per generated route, instead of broadcasting one binding-wide value onto every route regardless of origin.

Test plan

Test-first: added shouldGenerateProxyKindMcpHttpFromRouteExit first and confirmed it failed against the unfixed code (mcpHttp.routes came back empty — proving the live regression), then implemented the fix.

  • ./mvnw clean verify -pl runtime/binding-mcp-openapi: 61/61 unit tests (including three new cases: a route with its own exit is no longer dropped; two routes with different exits produce two generated routes with correspondingly different exits; a route with no exit falls back to the binding-level one) + 26/26 ITs (McpOpenapiProxyIT, McpOpenapiClientIT) pass
  • ./mvnw clean verify -pl specs/binding-mcp-openapi.spec: 14/14 SchemaTest cases (unchanged from fix(binding-mcp-openapi): correct proxy exit schema and route filtering #2563) + 17/17 spec ITs pass
  • Checkstyle clean, JaCoCo coverage checks met, NOTICE up to date on both modules

🤖 Generated with Claude Code

https://claude.ai/code/session_01SBDndP49e5oZjtxYNmzH5h

… proxy kind

The proxy-kind exit rule previously accepted either a binding-level
exit or a per-route exit on every route, resolved at runtime by
picking the first non-null route.exit found -- silently discarding
any other declared value. Nothing differentiated per generated route
in the composite (McpOpenapiCompositeGenerator broadcasts one
resolved exit onto every generated mcp-http0 route), so per-route
exit could never express real per-route routing; it only invited
configs that looked like it could.

Mirror the mcp-kafka-connect / mcp-schema-registry pattern instead:
exit is unconditionally required at the binding level and forbidden
on every route. This matches what the implementation actually
supports and removes the ambiguity entirely, rather than papering
over it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SBDndP49e5oZjtxYNmzH5h
@jfallows
jfallows marked this pull request as draft September 10, 2026 20:50
…ng-wide value

McpOpenapiBindingConfig.routes previously filtered on route.exit == null
to exclude the engine-synthesized catch-all route for the top-level
exit: shorthand. Since the proxy-kind schema already permits (and, when
binding.exit is absent, requires) a real route to declare its own exit,
that filter also excluded any real route with its own exit -- silently
dropping its with/when from the generated composite entirely. A proxy
config using only per-route exits (no binding-level exit) produced an
empty mcp-http0 binding with zero tools or resources.

The synthetic route is now identified by with == null (every real
mcp-openapi route always has with; the synthetic route never does),
so a real route's own exit is preserved.

McpOpenapiRouteConfig now carries its own resolved exit. Given that,
McpOpenapiCompositeGenerator resolves each RoutedOperation's exit
individually (the originating route's own exit, falling back to
binding.exit) and applies it per generated mcp-http0 route, instead of
broadcasting one binding-wide value onto every route regardless of
origin. This makes different routes' declared exits actually take
effect -- previously only the first non-null one found was ever
honored, with the rest silently discarded.

This keeps the schema exactly as merged in #2563 (no compatibility
break for existing configs using route-level exit) and fixes the
implementation to deliver what that schema already promised.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SBDndP49e5oZjtxYNmzH5h
@jfallows jfallows changed the title fix(binding-mcp-openapi): require binding exit, forbid route exit for proxy kind fix(binding-mcp-openapi): resolve exit per route instead of one binding-wide value Sep 10, 2026
@jfallows
jfallows marked this pull request as ready for review September 10, 2026 20:59
@jfallows
jfallows merged commit 9cf4ebc into develop Sep 10, 2026
43 checks passed
jfallows pushed a commit that referenced this pull request Sep 10, 2026
…yQName(route.id)

McpOpenapiRouteConfig (added by #2565's per-route exit resolution) called
context.supplyQName(route.id) to resolve a route's own exit qname, re-deriving
what EngineManager already resolves and stores on RouteConfig.qname. Read
route.qname() directly instead, consistent with how McpOpenapiBindingConfig's
own top-level exit resolution already works.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WF7FVoCe2rovFmkWB8sbtt
jfallows added a commit that referenced this pull request Sep 11, 2026
…fig.Reference (#2567)

* refactor(engine.conf): resolve qname once at engine load time via Config.Reference

RouteConfig and the NamedConfig family (GuardedConfig, CatalogedConfig,
OverlayConfig, VaultedConfig, StoredConfig, EmbeddedConfig) now expose a
shared Config.Reference/Config.Resolver contract so EngineManager resolves
each ref's id/qname once during config processing. Composite MCP bindings
(openapi, kafka-connect, schema-registry) read route.qname()/guard.qname()
directly instead of recomputing it via context.supplyQName(id) at generation
time. BindingConfig, GuardConfig, VaultConfig, CatalogConfig, StoreConfig,
and ExporterConfig now extend NamedConfig, removing duplicated id/name/qname
fields.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WF7FVoCe2rovFmkWB8sbtt

* fix(binding-mcp-openapi): read route.qname() instead of context.supplyQName(route.id)

McpOpenapiRouteConfig (added by #2565's per-route exit resolution) called
context.supplyQName(route.id) to resolve a route's own exit qname, re-deriving
what EngineManager already resolves and stores on RouteConfig.qname. Read
route.qname() directly instead, consistent with how McpOpenapiBindingConfig's
own top-level exit resolution already works.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WF7FVoCe2rovFmkWB8sbtt

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants