terrymanu commented on PR #38781: URL: https://github.com/apache/shardingsphere/pull/38781#issuecomment-4604729740
### Decision - **Merge Verdict: Mergeable** - **Reviewed Scope:** PR latest head `c103369ba9655421d398ef485a897390326807e5`; base `apache/master` / merge-base `b8c88a962bf292cd77368ce1a094985c98710de2`. Reviewed the 6 GitHub `/pulls/38781/files` docs changes under `docs/document/content/user-manual/shardingsphere-mcp/...`; the local triple-dot diff file list matched GitHub exactly. Also checked adjacent MCP workflow, identifier, runtime recovery, and descriptor code used to validate the documentation claims. - **Not Reviewed Scope:** GitHub Actions/check-runs by review boundary; full Hugo site rendering; unrelated MCP implementation areas beyond evidence needed for these documentation changes. - **Need Expert Review:** No required expert review. This is a docs-only MCP boundary refinement; no parser, security, concurrency, dependency, API, or SPI change is included. ### Basis - The change is scoped to MCP user documentation for Encrypt, Mask, and Troubleshooting, matching the PR goal for #35294 and containing no substantive unrelated changes. - The new SQL generation boundary text is consistent with current workflow code: `WorkflowSQLUtils` unwraps delimited identifiers, rejects backtick/NUL/CR/LF content, and quotes special DistSQL or database SQL identifiers as needed (`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtils.java:72`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtils.java:118`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtils.java:131`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtils.java:147`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtils.java:291`). - Existing tests cover the important counterexamples for the doc claim: whitespace identifiers, quoted identifiers, reserved DistSQL keywords, Unicode identifiers, line terminators, and backticks (`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtilsTest.java:49`, `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtilsTest.java:55`, `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtilsTest.java:61`, `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtilsTest.java:67`, `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtilsTest.java:101`, `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowSQLUtilsTest.java:126`). - The troubleshooting rewrite stays aligned with runtime recovery behavior: empty metadata responses expose `empty_state`, metadata resources append recovery guidance, and runtime connection failures use the documented safe categories (`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/tool/handler/metadata/SearchMetadataToolHandler.java:73`, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/resource/handler/metadata/MetadataResourceHandler.java:75`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/metadata/jdbc/RuntimeDatabaseConnectionException.java:33`). - Removing the feature-page “automatic rollback” bullet does not create a false rollback guarantee. Workflow docs still tell users to consider backup/rollback planning, while apply execution explicitly separates preview, manual-only, execution, and failure reporting without promising feature-level compensation (`docs/document/content/user-manual/shardingsphere-mcp/features/plugin-workflow.en.md:49`, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowExecutionService.java:89`, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowExecutionService.java:243`, `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowExecutionService.java:251`). - Fresh adversarial pass found no unresolved adjacent-feature, disabled/manual path, original-symptom, performance, compatibility, parser-family, or shared hot-path risk. No `ConcurrentHashMap#computeIfAbsent` high-frequency execution-path concern is introduced because the PR is documentation-only. ### Verification - Reviewed PR metadata, commits, issue comments, PR reviews, PR review comments, and GitHub `/pulls/38781/files`; there were no GitHub-visible previous review comments requiring multi-round comparison. - Verified local scope with `git diff --name-status b8c88a962bf292cd77368ce1a094985c98710de2..apache/pr-38781`; it matched GitHub’s 6 modified files. - Ran `./mvnw -pl mcp/support -am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dtest=WorkflowSQLUtilsTest -Dsurefire.failIfNoSpecifiedTests=false test` with exit code 0; `WorkflowSQLUtilsTest` ran 49 tests with 0 failures. - Did not inspect or use GitHub Actions / CI status. -- 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]
