terrymanu commented on PR #39019:
URL: https://github.com/apache/shardingsphere/pull/39019#issuecomment-4890171189
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. The PR keeps the change focused on MCP
prompt completion discoverability and model-facing SQL-tool wording, with no
code-level blocker found. CI not reviewed by request.
### Evidence
-
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/completion/provider/MetadataCompletionProvider.java:50`
adds the storage-unit argument aliases, and `:62-79` canonicalizes those
aliases before `supports` and `complete`, so prompt arguments such as
`write_storage_unit`, `source_storage_unit`, and `shadow_storage_unit` reuse
the existing `storageUnit` completion path instead of creating a parallel
provider contract.
-
`mcp/features/readwrite-splitting/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-readwrite-splitting.yaml:219-233`,
`mcp/features/shadow/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-shadow.yaml:288-310`,
and
`mcp/features/sharding/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-sharding.yaml:527-566`
expose completion targets only for arguments that are already backed by
metadata, workflow-plan, or feature algorithm completion providers.
-
`mcp/core/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-core.yaml:1385-1390`
and `:1603-1604` rename `database_gateway_execute_update` to “Preview or
Execute Side-Effecting SQL”, which matches the existing preview/execute
behavior without changing execution semantics.
- Tests cover the new public contract:
`mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/completion/provider/MetadataCompletionProviderTest.java:69-75`
checks alias support, `:199-206` checks alias completion behavior, and feature
descriptor tests assert the new prompt completion argument declarations.
### Review Details
- Review Focus: Code Correctness Review; CI not reviewed by request.
- Reviewed Scope: PR #39019 latest head
`d8ad01685cbbd017c54d4e2382c7bd9c923449af`, base `master` at
`e34b3592b49cdf5fa2a7b7fdde373a122b864d5f`, merge-base
`e34b3592b49cdf5fa2a7b7fdde373a122b864d5f`. Reviewed all 17 files reported by
GitHub `/pulls/39019/files`; the local `apache/master...HEAD` file list matched
the GitHub file list.
- Not Reviewed Scope: GitHub Actions/check-runs and MCP E2E runtime behavior
were not reviewed because this is Code Correctness Review.
- Verification:
- `./mvnw -pl mcp/core -DskipITs -Dspotless.check.skip=true
-Dcheckstyle.skip=true
-Dtest=MetadataCompletionProviderTest,ServerCapabilitiesHandlerTest,SearchMetadataToolHandlerTest
test` exited 0.
- `./mvnw -pl
mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding
-DskipITs -Dspotless.check.skip=true -Dcheckstyle.skip=true
-Dsurefire.failIfNoSpecifiedTests=false
-Dtest='*FeatureDefinitionTest,*DescriptorContractTest' test` exited 0.
- `./mvnw -pl mcp/core -DskipITs -Dspotless.check.skip=true
-Dcheckstyle.skip=true
-Dtest=MCPCompletionServiceTest,MetadataCompletionProviderTest,WorkflowPlanIdCompletionProviderTest
test` exited 0.
- Release Note / User Docs: Not required; this is an MCP
descriptor/completion usability improvement and a clearer tool title, with no
user migration, configuration, or compatibility action needed.
--
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]
