terrymanu commented on PR #39042:
URL: https://github.com/apache/shardingsphere/pull/39042#issuecomment-4906855351
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. The latest PR head keeps the change bounded
to MCP protocol/registry contract coverage, does not alter runtime behavior,
and the focused tests pass on the PR head. CI not reviewed by request.
### Evidence
- The production change in
`mcp/registry/src/main/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommand.java:45`
and
`mcp/registry/src/main/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommand.java:158`
only renames the registry schema constant to `REGISTRY_SCHEMA_URL`; the
validation value and behavior are unchanged.
-
`mcp/registry/src/test/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommandTest.java:175`
verifies that `mcp/server.json` uses the MCP Registry schema while not
declaring a runtime `protocolVersion`, which matches the clarified ownership
boundary.
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java:59`,
`:135`, `:151`, `:167`, and `:220` add focused protocol contract coverage for
initialize capabilities, unsupported tool JSON-RPC error shape, tool input
schema constraints, list response cursor behavior, and missing follow-up
protocol header rejection.
- The new helper in
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java:282`
sends initialized session requests through the existing HTTP transport
helpers, so the assertions exercise the same local protocol path instead of a
detached fixture.
- Focused verification on PR head passed:
- `./mvnw -pl test/e2e/mcp -Dtest=HttpTransportProtocolContractE2ETest
-DskipITs -Pcheck test` exit 0
- `./mvnw -pl mcp/registry -Dtest=MCPRegistryMetadataCommandTest -DskipITs
-Pcheck test` exit 0
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope:
[apache/shardingsphere#39042](https://github.com/apache/shardingsphere/pull/39042),
latest PR head `d30a86028092771fc8cec0f7a2a94356f3135362`, base `master` at
`443d736dd5945cf7c0474034569fcc9a75b5e965`, local merge-base
`443d736dd5945cf7c0474034569fcc9a75b5e965`. Reviewed files:
`mcp/registry/src/main/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommand.java`,
`mcp/registry/src/test/java/org/apache/shardingsphere/mcp/registry/MCPRegistryMetadataCommandTest.java`,
and
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java`.
Local triple-dot file list matched GitHub `/pulls/39042/files`.
- Not Reviewed Scope: GitHub Actions/check-runs/logs, full MCP distribution
packaging, Docker image E2E, and live production database/container E2E were
not reviewed because this was a code correctness review and the PR only changes
contract tests plus a non-behavioral registry constant name.
- Verification: GitHub PR metadata, file list, commits, issue comments, PR
reviews, and review comments were accessible. No public review comments were
present. Local focused Maven verification was run on the latest PR head with
the two commands listed above, both exit 0.
- Release Note / User Docs: Not required; this PR adds/clarifies tests and a
constant name without changing user-facing runtime behavior, configuration,
wire protocol behavior, or release artifacts.
--
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]
