terrymanu commented on PR #38798: URL: https://github.com/apache/shardingsphere/pull/38798#issuecomment-4619591911
## Reviewed Scope PR: apache/shardingsphere#38798 Head: 81595ead2ceed4d3c27cd53e5dbe6ade4afe667a Merge base: e503e17d49baab635b63aeca403c11c14d69025a GitHub `/pulls/38798/files` matches local triple-dot diff. Reviewed files: - `docs/document/content/user-manual/shardingsphere-mcp/troubleshooting.cn.md` - `docs/document/content/user-manual/shardingsphere-mcp/troubleshooting.en.md` ## Decision Merge Verdict: Mergeable The PR is scoped to MCP troubleshooting documentation and improves the page from a loose troubleshooting index into a user-facing issue list with clearer connection categories, runtime protection guidance, and known follow-up code improvement items. I did not find blocking factual mismatches or substantive unrelated changes. ## Basis - The new configuration note for required `username` and `driverClassName` is consistent with `YamlRuntimeDatabaseConfigurationsValidator`, which requires `databaseType`, `jdbcUrl`, `username`, and `driverClassName`. - The documented connection categories match `RuntimeDatabaseConnectionException`, including `missing_jdbc_driver`, `authentication_failed`, `authorization_failed`, `connection_timeout`, `invalid_configuration`, `database_unavailable`, `connection_failed`, and `database_not_visible`. - The runtime protection values for query row limits, timeout limits, and tool-call quota are consistent with `MCPRuntimeProtectionPolicy` and `MCPToolCallLimitExceededException`. - The follow-up note about `database_not_visible` not being aligned in runtime status is accurate: `RuntimeStatusHandler#createSafeRuntimeCategories` does not currently include that category. - The HTTP security follow-up is also framed as future work, not current behavior. Current code mainly reports Origin/session attribution failures through HTTP status/messages and logs. - Chinese and English pages are kept structurally aligned. ## Verification Commands run locally: - `curl -sL https://api.github.com/repos/apache/shardingsphere/pulls/38798` exited 0. - `curl -sL https://api.github.com/repos/apache/shardingsphere/pulls/38798/files?per_page=100` exited 0. - `git fetch origin dev:refs/remotes/origin/pr/38798 --force` exited 0. - `git fetch apache master:refs/remotes/apache/master --force` exited 0. - `git diff --name-status $(git merge-base apache/master origin/pr/38798)..origin/pr/38798` exited 0 and matched GitHub file scope. - `git diff --check $(git merge-base apache/master origin/pr/38798)..origin/pr/38798 -- docs/document/content/user-manual/shardingsphere-mcp/troubleshooting.cn.md docs/document/content/user-manual/shardingsphere-mcp/troubleshooting.en.md` exited 0. - Source/documentation cross-checks with `rg`, `git show`, and `nl` exited 0. No Maven build or unit tests were run because this PR changes only user documentation and no production/test code. ## Not Reviewed Scope - GitHub Actions / CI status was intentionally not used for this merge verdict. - Earlier PR revisions were not reviewed; this review is based only on the latest head. ## Need Expert Review None required from my side. -- 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]
