terrymanu commented on PR #38759:
URL: https://github.com/apache/shardingsphere/pull/38759#issuecomment-4577705277
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** Latest PR head
`0577a9a79057f08a8447cbb82bdca380abf06274`; base ref `master` at
`bea8f9de7e5ba0c228687dc27388a8c65a1c939c`; local merge-base
`8bfd71e786ffc1379fca6353225b9ef134479cd1`; local PR file list matched GitHub
`/pulls/38759/files`. Per review request, detailed review focused on latest
commit `0577a9a79057f08a8447cbb82bdca380abf06274` only, and its local file list
matched GitHub commit API. Files reviewed:
`docs/document/content/user-manual/shardingsphere-mcp/{_index,capabilities,client-integration,workflow}.{cn,en}.md`
and
`docs/document/content/user-manual/shardingsphere-mcp/features/{_index,encrypt}.{cn,en}.md`.
- **Not Reviewed Scope:** Previous commit
`7b21819cb83731e5fd0457c10583f214ce078b5b` except as the parent boundary for
the latest-commit diff; GitHub Actions/CI; live MCP runtime against a real
ShardingSphere-Proxy or physical database.
- **Need Expert Review:** No dedicated security, concurrency, performance,
or protocol expert review is required for this docs-only change.
### Basis
- The latest commit directly addresses the documentation goal by clarifying
protocol discovery versus runtime availability, Proxy versus direct database
boundaries, plugin workflow phases, and encrypt plugin usage.
- The workflow docs match the descriptor contract for
`database_gateway_apply_workflow`, including `preview`, `review-then-execute`,
`manual-only`, and `approved_steps`
(`mcp/support/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-support.yaml:164`,
`mcp/support/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-support.yaml:173`).
- The encrypt docs match the feature descriptor's Proxy-visible resources,
prompt, and completion targets
(`mcp/features/encrypt/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-encrypt.yaml:18`,
`mcp/features/encrypt/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-encrypt.yaml:72`,
`mcp/features/encrypt/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-encrypt.yaml:126`).
- The documented encrypt drop behavior matches implementation and tests:
remaining sibling columns produce `ALTER ENCRYPT RULE`, while no remaining
columns produces `DROP ENCRYPT RULE`
(`mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptRuleDistSQLPlanningService.java:57`,
`mcp/features/encrypt/src/test/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptRuleDistSQLPlanningServiceTest.java:82`).
- Risk scan found no production code, parser/grammar, SQL dialect behavior,
shared execution path, SPI contract, dependency, config, or high-frequency
DML/DQL path change. The `ConcurrentHashMap#computeIfAbsent` hot-path rule is
not applicable.
- Adversarial pass: direct database usage is explicitly kept out of
DistSQL/plugin workflow claims; adjacent Mask workflow behavior is not changed;
the original docs ambiguity is addressed by clearer capability discovery and
plugin workflow separation.
### Verification
- `curl` checks against PR metadata, PR commits, PR files, and latest commit
files: exit 0.
- `git fetch apache refs/heads/master:refs/remotes/apache/master
refs/pull/38759/head:refs/remotes/apache/pr-38759`: exit 0.
- Local PR three-dot file list matched GitHub `/pulls/38759/files`; latest
commit file list matched GitHub commit API.
- `hugo --source docs/document --destination
/tmp/shardingsphere-doc-review-38759 --cleanDestinationDir`: exit 0. Only
existing Hugo theme deprecation warnings were emitted.
- `git diff --check
0577a9a79057f08a8447cbb82bdca380abf06274^..0577a9a79057f08a8447cbb82bdca380abf06274`:
exit 0.
- GitHub Actions and CI status were not inspected or used for this verdict.
--
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]
