terrymanu commented on PR #39078: URL: https://github.com/apache/shardingsphere/pull/39078#issuecomment-4951646035
### Summary **Review Result: Mergeable** **Reason:** The latest PR head has no confirmed code-level blockers. This result is code-scope only; CI not reviewed by request. ### Evidence - Head `ba97dc138255ad00eae52ce0f0c137d7c505515d` restores the pre-refactor handling of scalar JSON-RPC `error.data` while preserving object-valued recovery data in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/transport/MCPInteractionPayloads.java:230`. - The behavior matches the pinned MCP Java SDK 1.1.2, which emits string data for unknown tools in [McpAsyncServer.java](https://github.com/modelcontextprotocol/java-sdk/blob/v1.1.2/mcp-core/src/main/java/io/modelcontextprotocol/server/McpAsyncServer.java#L539-L543). - Direct tests cover both scalar SDK error data and structured recovery data in `MCPInteractionPayloadsTest.java:127` and `MCPInteractionPayloadsTest.java:134`. - Removed concurrency tests were replaced with focused behavior-owner coverage for same-session serialization, cross-session concurrency, and guarded session closure in `MCPSessionExecutionCoordinatorTest.java:55`. - The sole production change consolidates an existing single startup message without changing server construction, startup, shutdown, or message content in `MCPRuntimeLauncher.java:48`. - Reviewer verification passed: - 398 affected MCP tests, zero failures or errors. - `./mvnw spotless:check -Pcheck -T1C` - `./mvnw checkstyle:check -Pcheck -T1C` ### Review Details - **Review Focus:** Code Correctness Review. CI not reviewed by request. - **Reviewed Scope:** All 64 files at head `ba97dc138255ad00eae52ce0f0c137d7c505515d`, covering `mcp/core`, `mcp/bootstrap`, MCP feature modules, and `test/e2e/mcp`. The merge-base is `a945425ecb820a91501cea8a536b3bc7a33d9e47`; the local triple-dot file list matched GitHub `/pulls/39078/files`. - **Not Reviewed Scope:** GitHub Actions and environment-backed production E2E scenarios. - **Verification:** Public PR metadata, commits, files, comments, and reviews were inspected. Current-head scoped Maven tests, Spotless, and Checkstyle completed successfully. - **Release Note / User Docs:** Not required. The PR refactors tests and test support while preserving the startup-log, configuration, protocol, API, dependency, and distribution contracts. -- 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]
