OSPP2: Add a stateless MCP tools entry to Agent Gateway - #7424

Draft review note: the overall MCP entry boundary is promising, but the preflight path currently appears to require both JSON and SSE Accept values regardless of response mode. Please confirm JSON-only clients are intentionally rejected, and keep this in draft until the protocol contract and failing K8s check are resolved.

Draft review note: the JSON-plus-SSE Accept requirement is now explicit and covered by tests, so the earlier ambiguity is resolved. Please keep the PR in draft until that protocol choice and the broader Agent Gateway MCP contract are ready for stabilization.

Reviewed current head 3882f857feda.

The main code paths have strong bounds, ownership, and authorization checks, and the earlier Accept-contract concern is resolved. I am leaving COMMENT because this is a broad new protocol entry with documented remaining MCP scope gaps rather than a small final-approval change.

Architecture note: No blocking defect found in the new MCP entry, request parser, dispatcher, context handling, registry, and starter wiring. This broad new protocol surface still needs live integration coverage beyond the reviewed unit tests.

Validation: exact-head targeted Maven reactor tests passed on Java 17.

./mvnw -pl shenyu-admin,shenyu-common,shenyu-plugin/shenyu-plugin-agent-gateway,shenyu-spring-boot-starter/shenyu-spring-boot-starter-plugin/shenyu-spring-boot-starter-plugin-agent-gateway -am test -Dtest=RuleServiceTest,AgentGatewayRuleHandleValidatorTest,AgentGatewayMcpConfigTest,AgentGatewayPluginTest,AgentGatewayRuleHandleParserTest,AgentMcpDispatcherTest,AgentMcpHttpHandlerTest,AgentMcpRequestParserTest,AgentMcpPrincipalSecurityResolverTest,AgentToolCapabilityTest,AgentToolDefinitionTest,AgentToolRegistryTest,AgentGatewayPluginConfigurationTest -Dsurefire.failIfNoSpecifiedTests=false -Drat.skip=true -Djacoco.skip=true -DskipRemoteResources=true -Dmaven.javadoc.skip=true

Supplementary example-module tests passed (28 tests, no failures).

./mvnw -f shenyu-examples/pom.xml -pl shenyu-examples-plugin -am test -Dtest=AgentGatewayExampleConfigurationTest,ExampleMcpAuthenticationFilterTest,OrderStatusToolTest -Dsurefire.failIfNoSpecifiedTests=false -Drat.skip=true -Djacoco.skip=true -DskipRemoteResources=true -Dmaven.javadoc.skip=true

Review: OSPP2 stateless MCP tools entry for Agent Gateway

I read the full diff (49 files, +4811/−33) and the surrounding contract in the existing AgentGatewayPlugin. This is a careful, security-first implementation. I am approving it; all comments below are non-blocking.

What is done well (specifics)

• Identity is never trusted from the wire. AgentMcpSecurityResolver is a deployment-supplied adapter; the default Starter wiring uses exchange -> Mono.empty(), so an MCP rule with no trusted adapter fails closed with 401 (AgentGatewayPluginConfigurationTest.shouldDenyMcpWhenNoSecurityAdapterIsRegistered). Forged _meta.subject / _meta.allowedTools and X-Subject / Mcp-Session-Id headers do not grant anything — verified by AgentMcpDispatcherTest.shouldIgnoreForgedMetadata… and AgentMcpHttpHandlerTest.shouldDenyMissingIdentityDespiteForgedHeadersAndMetadata.
• Permission intersection is correct and frozen per request. AgentMcpExecutionContext computes allowedTools = ruleTools ∩ securityGrants once, and AgentToolRegistry re-checks that same predicate at invoke time. An empty intersection yields an empty tools/list (200) but a 403 tools/call — the documented "unknown and invisible names have the same public denial" contract.
• Strict, layered input validation. AgentMcpRequestParser enforces STRICT_DUPLICATE_DETECTION, maxNestingDepth(64), FAIL_ON_TRAILING_TOKENS, mandatory UTF-8, single JSON-RPC request (no batch/notification/response hybrids), string-or-integral id, typed _meta, and exact mirrored-header matching (MCP-Protocol-Version, Mcp-Method, Mcp-Name, including base64 sentinel decode). This header/body cross-check is the right defense against a client smuggling the method/name through an unchecked field.
• Error sanitization is enforced by tests, not just claimed. Business/tool diagnostics (api-key=secret, private order exists, provider-private-diagnostic) are never forwarded; AgentToolArgumentException is mapped to a generic isError=true result, and unexpected exceptions become a 500 with no detail. Multiple tests assert the secret string is absent from the wire.
• Bounded, cancellable execution. One timeout budget covers identity → body read → parse → provider → encode → write; Instant deadline plus Reactor .timeout both apply; cancellation propagates to the provider subscription and is isolated per call even when 32 concurrent calls share an identical client RPC id (AgentMcpDispatcherTest.shouldKeep32ConcurrentCallsWithSameRpcIdIndependent, shouldCancelOneCallWithoutAffectingSurvivorWithTheSameRpcId). Netty buffers are released on normal completion, overflow and cancellation.
• Immutability / snapshot isolation. Every parsed request, definition, invocation argument and result is deep-copied; AgentToolRegistry and AgentGatewayMcpConfig are unmodifiable; config is captured at subscription, not assembly (AgentGatewayPluginTest.shouldSelectConfigurationAtSubscriptionNotAssembly). In-flight requests keep their snapshot while new requests see a revocation.
• Tests are thorough and meaningful. Beyond the 26 two-mode Accept cases and the regression suite you cite, the unit tests deliberately exercise the failure branches (missing identity, forged metadata, hidden/unknown tools, cursor rejection, empty completion, capability gating, deadline cancellation) rather than only the happy path. CI is fully green across all 44 checks.

Non-blocking suggestions

1. Hardcoded server version (minor). "2.7.2-SNAPSHOT" is duplicated in AgentGatewayPlugin.java:54, AgentGatewayPluginConfiguration.java:53 and AgentMcpDispatcherTest.java:3812, and is advertised as serverInfo.version in server/discover. After a release this silently lies. Prefer a single named constant or the project version. (Left as an inline nit.)
2. Redundant clientCapabilities re-parse (minor). AgentMcpDispatcher.java:126 re-serializes and re-parses clientCapabilities even though AgentMcpRequestParser already validated it as an object; reuse the existing JsonObject copy. (Left as an inline nit.)
3. RELEASE-NOTES. This adds a new trafficType: "mcp" value to the agentGateway rule handle and a new opt-in Starter assembly whose default behavior is "MCP denied until a trusted AgentMcpSecurityResolver bean is supplied." Please add a short ## Unreleased → ### Behavior Changes entry so downstream users are not surprised that enabling the plugin no longer auto-allows MCP. (Additive public API in shenyu-common is source-compatible, so this is documentation only.)
4. PR description vs state. The body still says "The PR remains draft pending the protocol-contract discussion." The actual PR is open (not draft) and CI is green. The 406 Accept policy is a deliberate, documented, defensible choice and is the kind of contract decision best finalized in the issue/ML thread rather than as a code blocker — but please update the description so reviewers are not misled about readiness.

Conclusion

The implementation is correct, well-tested and follows the existing module's defensive style. I have no blocking concerns. Approved. Please address the two nits and the RELEASE-NOTES point when convenient; the protocol-contract discussion can continue in parallel.

Re-review after rebase (head defa9ef)

I re-read the incremental diff against my prior reviewed head (3882f85). The large OLD..NEW diff was rebase noise from merging master (unrelated changes: admin lock removal, e2e logging-rabbitmq, k8s controller, registry-eureka, websocket sync, etc.). The PR's true content (base...head) is unchanged from the feature I already approved, plus the following improvements that resolve my earlier comments:

Nits resolved

1. Hardcoded server version — AgentGatewayPlugin.java / AgentGatewayPluginConfiguration.java / AgentMcpDispatcherTest no longer hardcode "2.7.2-SNAPSHOT". Version is now externalized to META-INF/shenyu/agent-gateway-version.properties (version=${project.version}, with scoped Maven resource filtering in pom.xml) and read via AgentGatewayConstants.MCP_SERVER_VERSION, which falls back to VersionUtils.getVersion(...). server/discover will now advertise the real build version. Test asserts the advertised version equals MCP_SERVER_VERSION.
2. Redundant clientCapabilities re-parse — AgentMcpDispatcher.java now calls request.getClientCapabilities() instead of re-toString()-ing and re-parsing. AgentMcpRequest stores a typed Gson snapshot built once in the constructor (toGson). A new test (shouldSnapshotCapabilitiesWithoutLosingNestedTypesOrLargeIntegers) verifies nested types, null, and large integers beyond Long.MAX (92233720368547758081234) are preserved without precision loss — strictly better than the previous text round-trip.

Non-blocking suggestions resolved

• RELEASE-NOTES.md now documents the trafficType: "mcp" opt-in and the fail-closed 401 behavior when no trusted AgentMcpSecurityResolver is supplied.
• README wording no longer claims "the PR remains draft"; it frames protocol-version/compatibility decisions as open for community discussion.

Verification

• CI: no failures across recent runs (license-header and others pass).
• mergeStateStatus: BLOCKED is only because the prior approval was dismissed on the new push; mergeable: MERGEABLE (no conflicts).
• Security invariants I originally verified remain intact: identity never derives from wire metadata, allowedTools = ruleTools ∩ grants frozen per request, strict layered parsing, error desensitization, single timeout budget with cancellation propagation.

The capability-snapshot change is a clean refactor with a stronger test than before; no regressions introduced. Re-approving.

(Note: the two original inline COMMENTED nits are now resolved by these changes and can be considered closed.)