terrymanu commented on PR #39022:
URL: https://github.com/apache/shardingsphere/pull/39022#issuecomment-4892792035
### Correction
Previous Finding: I previously treated the missing `RELEASE-NOTES.md` entry
for restricting MCP protocol support to `2025-11-25` as a merge blocker.
Current Status: Withdrawn
Reason: The PR is tightening the protocol surface of ShardingSphere-MCP
before the standalone MCP server is released in the same release line.
`RELEASE-NOTES.md` already contains the user-facing new feature entry for the
standalone ShardingSphere-MCP Server via #38541, so requiring another
release-note line for this pre-release protocol-scope refinement is not
necessary for code-scope merge safety.
### Summary
**Review Result: Mergeable**
Reason: Code-scope only: the latest PR head consistently restricts server
and production E2E client protocol support to `2025-11-25`, keeps `2025-06-18`
only as a negative test sentinel, and includes focused validation for both HTTP
and stdio production client transports. CI was not reviewed by request.
### Evidence
-
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/MCPTransportConstants.java:32`
sets the current protocol to `ProtocolVersions.MCP_2025_11_25`, and `:34`
exposes `SUPPORTED_PROTOCOL_VERSIONS` as `List.of(PROTOCOL_VERSION)`.
-
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/http/StreamableHttpMCPServlet.java:118-120`
negotiates unsupported initialize requests back to the single server protocol,
and `:275-278` writes the current protocol version into initialize responses
without keeping legacy per-session protocol state.
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/ProductionMCPClientTransportFactory.java:54`
defines the single production E2E client protocol list, `:70-72` passes it
into the HTTP SDK transport builder, and `:114-116` uses the same list for
stdio.
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/ProductionMCPClientTransportFactoryTest.java:36-53`
directly verifies that both production HTTP and stdio client transports expose
only `ProtocolVersions.MCP_2025_11_25`.
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportProtocolContractE2ETest.java:148-167`
keeps `2025-06-18` as a legacy negative-contract sentinel: initialize is
negotiated to the current protocol, while follow-up requests with the legacy
protocol header are rejected.
- `RELEASE-NOTES.md` is not part of this PR, which is acceptable for this
code review because the standalone MCP server is already represented as a new
feature in the same release line.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: Latest PR head `eb5132f16e26984678de71153187406006a30b2e`,
base `master` at `d4d615e3de4702ff04be250eb5b6590b10793a0a`, local merge-base
`d4d615e3de4702ff04be250eb5b6590b10793a0a`. Reviewed all 7 files reported by
GitHub `/pulls/39022/files`; the local triple-dot file list matched GitHub.
- Not Reviewed Scope: GitHub Actions/check-runs/logs, full Docker/release
MCP E2E, and MCP features outside the changed protocol-version paths.
- Verification:
- `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true
-Dcheckstyle.skip=true -Dlicense.skip=true -Djacoco.skip=true
-Dsurefire.failIfNoSpecifiedTests=false
-Dtest=ProductionMCPClientTransportFactoryTest,HttpTransportProtocolContractE2ETest
test` passed with exit code 0.
- `./mvnw -pl mcp/bootstrap -am -DskipITs -Dspotless.skip=true
-Dcheckstyle.skip=true -Dlicense.skip=true -Djacoco.skip=true
-Dsurefire.failIfNoSpecifiedTests=false
-Dtest=StreamableHttpMCPServletTest,ProtocolVersionHeaderConstraintTest,SessionManagedStdioTransportProviderTest
test` passed with exit code 0.
- Release Note / User Docs: Separate release-note entry is not required for
this PR because ShardingSphere-MCP is being introduced in the same release line
and the release notes already include the standalone MCP server feature entry.
Existing MCP docs already target protocol revision `2025-11-25`.
--
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]
