terrymanu commented on PR #39055: URL: https://github.com/apache/shardingsphere/pull/39055#issuecomment-4914288377
### Summary **Review Result: Mergeable** Reason: Code-scope review only; CI not reviewed by request. The latest PR head repairs the root-cause path by classifying Rule DistSQL as `rule-metadata`, routing Rule DistSQL syntax failures into workflow-aware recovery, and updating the MCP guidance/descriptor contract so models are directed to the planning workflow before retrying raw SQL. ### Evidence - `ClassificationResult` now recognizes supported Rule DistSQL prefixes and reports `rule-metadata` through `getSideEffectScope()` instead of treating these statements as ordinary physical DDL: `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/ClassificationResult.java:34`, `:111`, `:129`. - `database_gateway_execute_update` preview uses that shared side-effect scope and adds workflow-first guidance only for Rule DistSQL: `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/ExecuteUpdateToolHandler.java:57`, `:121`, `:147`. - The JDBC execution path converts `SQLSyntaxErrorException` from classified Rule DistSQL into `RuleDistSQLExecutionException`, preserving ordinary SQL syntax handling for non-Rule DistSQL: `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/execute/MCPJdbcStatementExecutor.java:97`. - Error conversion maps the new exception before generic query recovery and returns structured recovery that reads guidance and database capabilities before asking for rewritten SQL: `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/protocol/error/MCPRecoveryPayloadFactory.java:53`, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/protocol/error/MCPSQLRecoveryPayloadFactory.java:58`, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/protocol/error/MCPRecoveryPayloadSupport.java:71`. - The model-facing contract is updated in descriptors/guidance: `database_gateway_execute_update` advertises workflow preference and `rule-metadata`, while global guidance adds a `rule_workflow` route and rule-change preference: `mcp/core/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-core.yaml:1390`, `:1614`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPGuidancePayloadBuilder.java:93`, `:258`, `:265`. - Tests cover the root-cause path through classifier, preview handler, JDBC executor, recovery payload conversion, and descriptor/guidance contracts: `StatementClassifierTest.java:91`, `ExecuteUpdateToolHandlerTest.java:145`, `MCPJdbcStatementExecutorTest.java:282`, `MCPErrorConverterTest.java:303`, `ShardingDescriptorContractTest.java:62`. ### Review Details - Review Focus: Code Correctness Review. CI not reviewed by request. - Reviewed Scope: PR #39055 latest head `1d7d353485669e40f83bfe4d12f0709baaf8d40f`, base `master` at `3b994c61c1fb4df214d92b1f976080fd7704c50d`, merge-base `3b994c61c1fb4df214d92b1f976080fd7704c50d`. GitHub `/pulls/39055/files` matched the local triple-dot diff: 20 files across `mcp/core`, `mcp/support`, and `mcp/features/sharding`. - Not Reviewed Scope: GitHub Actions/check-runs/logs and external live MCP/client E2E behavior. - Verification: latest-head identity and file-scope checks passed locally. Local verification on this same head also passed: focused MCP tests, scoped MCP reactor install, `spotless:check`, `checkstyle:check`, and `apache-rat:check` all exited 0. - Release Note / User Docs: Not required. This is MCP model-facing guidance, diagnostics, and descriptor behavior; no user migration, configuration key, dependency, or release artifact change was found. -- 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]
