terrymanu commented on PR #38771: URL: https://github.com/apache/shardingsphere/pull/38771#issuecomment-4600077739
## PR Review: apache/shardingsphere#38771 Merge Verdict: Mergeable ### Reviewed Head - PR head: `160445a46236325888f643526004a61f46cff7fb` - Base: `a9ac265ad4adcc247a8af27f1febca026e6cc8d4` - Scope: MCP preflight validation tool, MCP descriptor/model contract, documentation, unit tests, and MCP E2E contract/distribution coverage. ### Root-Cause Review The original blocker was not only the missing MCP E2E baseline update. The new preflight tool also needed a safe contract boundary. The latest PR head now keeps the MCP tool model-facing surface constrained to a configured runtime `database` name. It no longer exposes raw JDBC inputs such as `jdbcUrl`, `username`, `password`, or `driverClassName` through tool arguments. Runtime connection details remain administrator-owned configuration and are resolved through the MCP runtime context before any JDBC validation runs. This directly addresses the root risk instead of only making the E2E tool list pass. ### Validation Review The MCP contract and runtime surface have been synchronized: - The tool descriptor requires only `database`. - Descriptor validation rejects re-exposing raw JDBC connection fields. - Unit tests cover request parsing, runtime database lookup, missing/unknown database handling, failure recovery, and descriptor constraints. - HTTP contract, MySQL HTTP/STDIO, and packaged distribution E2E expectations include the new tool. - Model contract baseline and fingerprints were updated consistently. Observed checks for the reviewed head: - Check-runs: `15 success`, `6 skipped`, `0 failed`, `0 active` - Local HTTP contract/baseline verification: `11 tests`, `0 failures` - Local working tree: clean ### Risk Review - Design: The tool now follows the existing MCP runtime database ownership model instead of accepting caller-supplied connection details. - Security: The change avoids expanding MCP callers into arbitrary JDBC endpoint validation. - Compatibility: This is a new tool contract before merge, so tightening the schema does not break an established released API. - Performance: The validation path is explicit and on-demand; it is not on the Proxy/JDBC high-frequency SQL execution path. - Regression surface: Existing MCP tools remain registered, and the new official tool is covered across descriptor, model contract, runtime, and distribution tests. ### Final Assessment No remaining code-level blocker was found for the reviewed head. The earlier temporary inability to merge was caused by pending GitHub checks, not by an additional code defect in the latest reviewed code. -- 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]
