terrymanu commented on PR #39176:
URL: https://github.com/apache/shardingsphere/pull/39176#issuecomment-5010115277
### Summary
**Review Result: Mergeable**
Reason: This is a code-scope-only result. The latest head consistently
simplifies the transport enum and YAML value while preserving existing
model-facing and diagnostic output; CI was not reviewed by request.
### Evidence
-
`mcp/api/src/main/java/org/apache/shardingsphere/mcp/api/transport/MCPTransportType.java:23`
now models the two supported runtime choices directly as `HTTP` and `STDIO`,
removing the redundant `value` field and Lombok-generated API.
-
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/MCPRuntimeLauncher.java:77`
and
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/resource/handler/capability/RuntimeStatusHandler.java:64`
derive lowercase names explicitly, preserving the existing `http` / `stdio`
startup-log and runtime-resource contracts.
- HTTP and STDIO routing remains explicit at `MCPRuntimeLauncher.java:86`,
and the transport-specific security summary remains aligned at
`RuntimeStatusHandler.java:93`.
- Distribution configuration, validation diagnostics, E2E fixtures,
evaluation artifacts, and the English and Chinese user documentation
consistently use `transport.type: HTTP`.
- `MCPRuntimeLauncherTest` directly covers both server-selection branches
and their complete startup messages. `RuntimeStatusHandlerTest` directly
verifies `http` / `stdio` payload compatibility. Configuration loader, swapper,
validation, packaged-distribution, and evaluation-artifact tests cover the
changed YAML boundary.
- Release history shows that the MCP module was introduced by `c4490ad`
after the 5.5.3 release; the 5.5.3 tag contains neither this MCP configuration
nor the public transport enum. No released configuration or API compatibility
shim is therefore required.
- The remaining `STREAMABLE_HTTP_*` identifiers in
`MCPRegistryMetadataCommand` represent the separate MCP Registry
`streamable-http` protocol/package identifier and are not stale YAML enum
references.
### Review Details
- Review Focus: Code Correctness Review — CI not reviewed by request.
- Reviewed Scope: All 34 files reported by GitHub `/pulls/39176/files`,
latest head `0847a10fb2f9473846214ff9f0b5910a07158c6d`, base and local
merge-base `e8bba8cde0ee38bf162e0a8df0008ea06a840f1b`. The local triple-dot
file list matched GitHub exactly.
- Not Reviewed Scope: GitHub Actions/check-runs and live
Docker/native-client smoke were not reviewed. Direct configuration, runtime,
distribution-support, and payload tests sufficiently cover this refactor’s code
paths.
- Verification:
- `./mvnw spotless:apply -Pcheck -T1C` — exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C` — exit 0, zero violations.
- Focused MCP Core/Bootstrap reactor tests — exit 0, 47 tests passed.
- `MCPBuilderEvaluationArtifactTest` and
`PackagedDistributionTestSupportTest` — exit 0, 13 tests passed.
- Reactor freshness was required because the changed public API type is
consumed by Core, Bootstrap, and E2E support modules.
- GitHub Evidence Access: PR metadata, all changed-file pages, commits,
comments, reviews, and review threads were available through authenticated
REST/GraphQL access. No public-evidence gap remains.
- Release Note / User Docs: English and Chinese configuration/deployment
documentation is updated. A release note is not required because the MCP
configuration and enum have not appeared in a released ShardingSphere version.
--
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]
