terrymanu commented on PR #38995:
URL: https://github.com/apache/shardingsphere/pull/38995#issuecomment-4882252303
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. I reviewed the latest PR head for
implementation correctness, scope, regression risk, and focused local
verification; no code-level blocker was found. CI not reviewed by request.
### Evidence
- The PR consistently moves resource descriptor naming from `uriTemplate` to
`uriOrTemplate` across the MCP resource handler contract, resource descriptors,
registry, bootstrap resource specifications, and feature resource handlers.
- The descriptor catalog split is internally consistent: YAML loading now
builds protocol descriptors and ShardingSphere descriptors separately, while
`MCPDescriptorCatalogIndex`, payload builders, validators, and registry paths
consume the split catalog through one validated load path.
- The new `shardingsphere://guidance` resource is covered end to end: it is
declared in
`mcp/core/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-core.yaml`,
registered through `CoreResourceHandlers`, exposed through
`ResourceDefinitionRegistry`, and exercised by the capability/resource
specification tests.
- The removed `MCPToolDescriptorValidator` SPI path is replaced by
centralized descriptor validation in `MCPToolDescriptorCatalogValidator`; the
previous core, workflow, feature planning, secret workflow, runtime,
related-resource, and encrypt DistSQL checks are covered in the consolidated
validator and related tests.
- Focused verification passed:
- MCP selected tests: exit 0; support 71 tests, core 69 tests, bootstrap 5
tests; build success.
- Root Checkstyle: exit 0; build success.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: 108 changed files from GitHub `/pulls/38995/files`,
covering `mcp/api`, `mcp/support`, `mcp/core`, `mcp/features/*`, and
`mcp/bootstrap`. Latest PR head SHA:
`ca7258f2b843346fd0bc882a9688ce29a713763b`. Base ref: `master`; base SHA:
`68b7bab93f92a5c942af31f272cbde62c45bfbc8`. Local merge-base SHA:
`68b7bab93f92a5c942af31f272cbde62c45bfbc8`. Local triple-dot file list matched
GitHub `/pulls/38995/files`.
- Not Reviewed Scope: GitHub Actions/check-runs, Actions logs, full MCP
E2E/proxy runtime smoke, and broader product documentation beyond the changed
PR scope.
- Verification: GitHub REST PR metadata/files/comments/reviews were read
successfully. Focused MCP Maven test command exited 0. Root `./mvnw
checkstyle:check -Pcheck -T1C` exited 0.
- Release Note / User Docs: Not required for this code-correctness review;
the changed scope is an MCP descriptor/catalog/guidance refactor and no
upgrade, migration, rollback, dependency, or released-user configuration impact
was identified.
--
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]
