terrymanu commented on PR #38828: URL: https://github.com/apache/shardingsphere/pull/38828#issuecomment-4645570687
### Summary - **Merge Decision: Mergeable** - **Reason:** The latest PR head fixes the workflow response-builder duplication while preserving the feature-specific mask/encrypt payload behavior, and the added MCP completion/runtime fixes have targeted regression coverage. ### Evidence - The removed broadcast/readwrite-splitting/shadow/sharding `WorkflowToolResponseBuilder` classes were pure delegation wrappers around `WorkflowPlanPayloadBuilder.buildRuleDistSQLOnly(...)`; the handlers now call the shared builder directly, for example `mcp/features/broadcast/src/main/java/org/apache/shardingsphere/mcp/feature/broadcast/tool/handler/PlanBroadcastRuleToolHandler.java:64` and `mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/handler/AbstractShardingPlanningToolHandler.java:58`. - The remaining builders are correctly feature-prefixed and still own feature-specific behavior: `EncryptWorkflowToolResponseBuilder` appends `masked_property_preview` at `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/handler/EncryptWorkflowToolResponseBuilder.java:38`, and `MaskWorkflowToolResponseBuilder` does the same at `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/handler/MaskWorkflowToolResponseBuilder.java:36`. - The original completion/runtime failure path is covered at the descriptor and factory levels: encrypt completion targets are declared in `mcp/features/encrypt/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-encrypt.yaml:127`, mask completion targets in `mcp/features/mask/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-mask.yaml:119`, and real descriptor-backed plan payload schema coverage is added in `mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolSpecificationFactoryTest.java:177`. - The DistSQL fallback is scoped to workflow inspection and preserves non-DistSQL failure propagation: unsupported syntax is classified in `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowDistSQLQueryUtils.java:40`, while mask/encrypt rule inspection still rethrows non-syntax query failures in `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskRuleInspectionService.java:74` and `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptRuleInspectionService.java:90`. - Secret-handling regression coverage is present: encrypt clarifying responses are checked so `aes-key-value` is masked and the original secret is absent in `mcp/features/encrypt/src/test/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/handler/EncryptToolHandlerTest.java:112`. - The shared-path blast radius is acceptable: non-mask/encrypt workflow tools only move from local delegating wrappers to the existing shared payload builder, and mask/encrypt keep feature-owned response customization. No parser, routing, dependency, POM, distribution, LICENSE, or NOTICE files are changed. - The linked issue `#35294` is an MCP umbrella discussion rather than a precise close/fix ticket; this PR is a scoped MCP workflow cleanup/fix slice and does not appear to over-claim full issue completion. ### Review Details - **Reviewed Scope:** PR `apache/shardingsphere#38828`, latest head `6f4dde8a75169f6ac430a6c3f1993adc53e70c23`, base and local merge-base `11bfda8fd2a2645f81459ce13fd170170a26cd74`. Reviewed all 36 changed files reported by GitHub `/pulls/38828/files`; local triple-dot file list matched GitHub exactly. - **Not Reviewed Scope:** Full repository behavior outside the PR diff, live GitHub Actions status, performance benchmarking, and full LLM usability suite behavior beyond the programmatic MCP runtime subset. - **Verification:** `./mvnw -pl mcp/support,mcp/features/mask,mcp/features/encrypt,mcp/bootstrap -am -DskipITs -Dspotless.skip=true -Dtest=WorkflowDistSQLQueryUtilsTest,MaskRuleInspectionServiceTest,EncryptRuleInspectionServiceTest,MaskAlgorithmPropertyTemplateServiceTest,EncryptAlgorithmPropertyTemplateServiceTest,MaskToolDescriptorValidatorTest,EncryptToolDescriptorValidatorTest,MaskToolHandlerTest,EncryptToolHandlerTest,MaskAlgorithmCompletionProviderTest,EncryptAlgorithmCompletionProviderTest,MCPToolSpecificationFactoryTest,MCPCompletionSpecificationFactoryTest test -Dsurefire.failIfNoSpecifiedTests=false -B -ntp` passed with exit code 0. `./mvnw checkstyle:check -Pcheck -T1C` passed with exit code 0. Programmatic MCP runtime E2E was also run locally on the same head with `-pl test/e2e/mcp -am`, covering 52 tests with 0 failures and 0 errors. - **Release Note / User Docs:** Not required for this PR because the change is an internal MCP workflow response-builder cleanup plus descriptor/runtime contract correction, with no user migration, configuration change, or released dependency/distribution impact.； - -- 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]
