terrymanu commented on PR #38833: URL: https://github.com/apache/shardingsphere/pull/38833#issuecomment-4655389937
### Summary - **Merge Decision: Mergeable** - **Reason:** The latest PR head directly fixes the MCP feature-completion regression, keeps the shared completion contract generic, and has targeted validation for prompt completions, fallback catalogs, descriptor validation, and scoped Checkstyle. ### Evidence - The root cause is addressed by moving feature plugin completions away from static resource completion targets and validating that resource completion targets are templates only (`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPDescriptorCatalogValidator.java:330`). - The updated descriptors expose prompt completion targets for feature plugin arguments, for example readwrite-splitting at `mcp/features/readwrite-splitting/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-readwrite-splitting.yaml:209` and sharding at `mcp/features/sharding/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-sharding.yaml:474`. - Readwrite-splitting, shadow, and sharding plugin fallback paths query live DistSQL first and fall back only for unsupported/syntax failures; ordinary query failures are rethrown (`mcp/features/readwrite-splitting/src/main/java/org/apache/shardingsphere/mcp/feature/readwritesplitting/tool/service/ReadwriteSplittingInspectionService.java:104`, `mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingInspectionService.java:55`). - Regression tests cover successful plugin queries, unsupported-DistSQL fallback, and connection-error propagation (`mcp/features/readwrite-splitting/src/test/java/org/apache/shardingsphere/mcp/feature/readwritesplitting/tool/service/ReadwriteSplittingInspectionServiceTest.java:63`, `mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingInspectionServiceTest.java:61`). - The runtime-facing E2E contract now exercises prompt references for readwrite-splitting, shadow, sharding, and key-generator completions (`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportCompletionE2ETest.java:84`). - Shared-layer blast radius was checked across MCP metadata/resource completions and feature completions; no parser, dependency, distribution, public SQL dialect, high-frequency `computeIfAbsent`, or target-specific leakage issue was found. - Review standards were checked against Code of Conduct readability, consistency, simplicity, build, and unit-test expectations (`CODE_OF_CONDUCT.md:8`, `CODE_OF_CONDUCT.md:10`, `CODE_OF_CONDUCT.md:11`, `CODE_OF_CONDUCT.md:18`, `CODE_OF_CONDUCT.md:86`). ### Review Details - **Reviewed Scope:** Latest PR head `58badfadf222a35b3c460b043c9e1a82133eb2d4`; base/merge-base `2cdb01f3599f8e5edf0710868c9ba7047621a418`; all 27 GitHub `/pulls/38833/files` entries under `mcp/features/*`, `mcp/support`, and `test/e2e/mcp`; local triple-dot file list matched GitHub exactly. - **Not Reviewed Scope:** GitHub Actions/check-run status by request/skill boundary; full `./mvnw clean install -B -T1C -Pcheck`; Docker-backed HTTP MCP E2E execution, because `HttpTransportCompletionE2ETest` was skipped by its Docker condition locally. - **Verification:** `git fetch apache master:refs/remotes/apache/master pull/38833/head:refs/remotes/apache/pr/38833` exit 0; GitHub file-list cross-check exit 0; `git diff --check` exit 0; targeted Maven test run with `-am` exit 0, with Docker-gated MCP E2E skipped; completion-provider/bootstrap Maven test run with `-am` exit 0; scoped `./mvnw -pl mcp/support,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding,test/e2e/mcp -am -DskipTests checkstyle:check -Pcheck -T1C` exit 0. - **Release Note / User Docs:** Not required for this PR; the change fixes and standardizes internal MCP descriptor/completion behavior without new user configuration, migration, SQL syntax, API/SPI, or 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]
