terrymanu commented on PR #39080: URL: https://github.com/apache/shardingsphere/pull/39080#issuecomment-4954199618
### Summary **Review Result: Mergeable** **Reason:** The latest version is mergeable from a code-correctness perspective. It removes duplicated model-facing prose while preserving the machine-readable EXPLAIN capability and authoritative tool constraints. This result applies only to the reviewed code scope; final CI merge gates were not evaluated. ### Evidence - `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/response/MCPDatabaseCapabilityResponse.java:40` retains `supportsExplain` and `supportedStatementClasses`, removing only `explainExecutionSemantics`, which was derived from the boolean capability. - The constraints for a single SELECT, database-native EXPLAIN syntax, and rejection of `EXPLAIN ANALYZE` and `EXPLAIN PLAN FOR` remain enforced by `mcp/core/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-core.yaml:1386`, the guidance resources, and the runtime validator. No model-facing or execution-safety semantics are lost. - `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/MCPSQLExecutionFacade.java:126` verifies that the target database supports `SupportedMCPStatement.EXPLAIN` before execution. Unsupported paths therefore cannot enter the modified syntax-recovery branch. - For actual EXPLAIN syntax failures, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/protocol/error/MCPSQLRecoveryPayloadFactory.java:60` reads the retained `databaseType` from the capability resource before regenerating database-native EXPLAIN SQL. The recovery flow remains consistent with the error classification. - `MCPDatabaseCapabilityResponseTest` verifies the complete payload for both `supportsExplain=true` and `supportsExplain=false`. `MCPErrorConverterTest` covers the updated resource-read reason and the subsequent retry tool. - The removed field was introduced by commit [25d65170b19](https://github.com/apache/shardingsphere/commit/25d65170b191a1ee56dd8268dfca227985d59878) and has not appeared in any tag. There are also no other repository consumers, baselines, or documentation references, so no compatibility shim or migration guidance is required. - The final adversarial review covered adjacent consumers, unsupported and disabled paths, repeated recovery, public payload compatibility, documentation, and release impact. It found no new independent issues. ### Review Details - **Review Focus:** Code Correctness Review. CI not reviewed by request. - **Reviewed Scope:** PR head `b6b81d4c41cf2aff3cbfe9ceda2d8c38ff4312be`; base and local merge-base `e3bff1a84847082fe67cf095a60392784f11daf6`. All four changed files in `mcp/support` and `mcp/core` were reviewed. The local triple-dot file list exactly matched GitHub `/pulls/39080/files`. - **Not Reviewed Scope:** GitHub Actions and final repository merge gates. Unchanged database-dialect capability implementations and runtime MCP E2E tests were inspected only as adjacent context. - **Verification:** Focused latest-head tests passed with exit code `0`: 2 `MCPDatabaseCapabilityResponseTest` cases and 46 `MCPErrorConverterTest` cases. `./mvnw spotless:check -Pcheck -T1C` and `./mvnw checkstyle:check -Pcheck -T1C` both passed with exit code `0`. - **Release Note / User Docs:** Not required. The removed payload field was unreleased and undocumented, while its authoritative EXPLAIN semantics remain available through the existing tool metadata and guidance. -- 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]
