terrymanu commented on PR #38783: URL: https://github.com/apache/shardingsphere/pull/38783#issuecomment-4609008185
### Decision - **Merge Verdict: Not Mergeable** - **Reviewed Scope:** Latest [PR #38783](https://github.com/apache/shardingsphere/pull/38783) head `962f6fd36c3d88eac616436ded44d1eeb73edbae`; base and local merge-base `260022e756f88b8b423c7efd3447b4738689c787`; local triple-dot file list matched GitHub `/pulls/38783/files` with 24 files. Reviewed all changed ShardingSphere-MCP user-manual docs under `docs/document/content/user-manual/shardingsphere-mcp/`, plus MCP reference/runtime evidence for the HTTP lifecycle. - **Not Reviewed Scope:** GitHub Actions / CI status, full Hugo site rendering, and live MCP runtime startup were not reviewed. SQL parser, database dialect, dependency, and production execution paths are not touched by this PR. - **Need Expert Review:** No security, concurrency, or performance expert review is required for this docs-only change. A MCP protocol/docs maintainer should re-check the HTTP examples after the lifecycle fix. ### Positive Feedback - The reorganization is directionally good: moving protocol-level material into a developer appendix makes the main user path much easier to follow, and the English/Chinese pages remain in sync at the section level. ### Major Issues - **[Blocker] HTTP debugging examples skip the required initialized notification** (`docs/document/content/user-manual/shardingsphere-mcp/developer-appendix.en.md:107`, `docs/document/content/user-manual/shardingsphere-mcp/developer-appendix.cn.md:107`) - **Symptom:** Both new developer appendices show `initialize`, then immediately show `resources/read` and `tools/call` examples. The removed quick-start flow previously included a `notifications/initialized` request before normal MCP operations, but the new appendix no longer documents that step. - **Risk:** Developers copying the appendix get a protocol-incomplete HTTP flow. The MCP 2025-11-25 lifecycle requires the client to send `notifications/initialized` after successful initialization before normal operation, and ShardingSphere's own HTTP E2E helper follows that flow in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/AbstractHttpProgrammaticRuntimeE2ETest.java:63`. See the official lifecycle requirement: https://modelcontextprotocol.io/specification/2025-11-25/basic/lifecycle - **Action:** Please add the missing `notifications/initialized` HTTP request in both `developer-appendix.en.md` and `developer-appendix.cn.md` after the initialize example, and state that later resource/tool requests must include the returned `MCP-Session-Id` and `MCP-Protocol-Version` headers. ### Next Steps - Add the initialized-notification curl example to both language appendices. - Keep the existing `resources/read` and `tools/call` examples after that notification step. - Recheck the changed docs after the fix; the current local diff boundary and `git diff --check` were clean, but the appendix examples need the lifecycle correction before merge. -- 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]
