terrymanu commented on PR #39020:
URL: https://github.com/apache/shardingsphere/pull/39020#issuecomment-4891173120
### Summary
**Review Result: Mergeable**
Reason: Code-scope only. I reviewed the latest PR code, descriptor changes,
tests, behavior, and regression risk for PR #39020 and did not find a confirmed
code-level blocker. CI not reviewed by request.
### Evidence
- The tool argument contract now validates declared properties,
schema-valued `additionalProperties`, and integer bounds before dispatching
tool handlers, with typed recovery for out-of-range integer arguments:
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/MCPToolArgumentContract.java:114`.
- `database_gateway_execute_update` now applies execution argument
validation before both preview and execute paths, closing the direct
handler-call gap for invalid `max_rows` / `timeout_ms`:
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/ExecuteUpdateToolHandler.java:72`.
- Streamable HTTP GET now has an explicit unsupported active event stream
response instead of delegating to the SDK path that can hold the connection
open:
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/http/StreamableHttpMCPServlet.java:140`.
- Completion aliases for storage-unit-style prompt arguments are
canonicalized through the metadata completion provider, and the descriptor
completion target additions are covered by descriptor contract tests:
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/completion/provider/MetadataCompletionProvider.java:50`,
`mcp/features/readwrite-splitting/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-readwrite-splitting.yaml:219`,
`mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/completion/provider/MetadataCompletionProviderTest.java:70`.
- The E2E coverage exercises the client-visible transport and tool-error
behavior added by this PR: unsupported active GET returns 405, invalid integer
arguments return tool errors, schema-valued additional properties are enforced,
and per-session tool-call limits are verified:
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java:90`.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: Latest PR head `81683b5623c8e61ac1fa747a53412b8a313ff1a1`
against base `master` SHA `5c7fc962ee278016886d6c2ea0a961a66f019b3d`; local
merge-base `e34b3592b49cdf5fa2a7b7fdde373a122b864d5f`. Reviewed all 26
GitHub-listed changed files under `mcp/bootstrap`, `mcp/core`,
`mcp/features/{broadcast,encrypt,mask,readwrite-splitting,shadow,sharding}`,
and `test/e2e/mcp`. The local triple-dot file list matched GitHub
`/pulls/39020/files`.
- Not Reviewed Scope: GitHub Actions/check-runs, release packaging,
native-image behavior, and manual external MCP client smoke were not reviewed.
- Verification: Ran focused PR-head verification in a clean detached
worktree: `./mvnw -pl
mcp/api,mcp/support,mcp/core,mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding,mcp/bootstrap,test/e2e/mcp
-am -DskipITs -Dspotless.skip=true
-Dtest=MetadataCompletionProviderTest,ServerCapabilitiesHandlerTest,ToolDefinitionRegistryTest,ExecuteUpdateToolHandlerTest,SearchMetadataToolHandlerTest,StreamableHttpMCPServletTest,BroadcastFeatureDefinitionTest,EncryptDescriptorContractTest,MaskFeatureDefinitionTest,ReadwriteSplittingFeatureDefinitionTest,ShadowDescriptorContractTest,ShardingDescriptorContractTest,HttpTransportProtocolContractE2ETest
-Dsurefire.failIfNoSpecifiedTests=false -B -ntp test` with exit code 0; 116
focused tests passed. GitHub public PR metadata, file list, reviews, issue
comments, and inline review comments were accessible; no public review/comment
counter-evidence was present.
- Release Note / User Docs: Not required for this code-scope review. The PR
tightens MCP tool validation, completion metadata, and HTTP transport contract
behavior with focused tests, without adding a new configuration key, migration
requirement, or user-facing compatibility procedure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
