terrymanu commented on PR #38836: URL: https://github.com/apache/shardingsphere/pull/38836#issuecomment-4662036326
### Summary - **Merge Decision: Mergeable** - **Reason:** The latest head keeps the MCP encrypt workflow rule-DistSQL-only, masks model-facing secret output while preserving raw execution, and has targeted coverage for the changed workflow contract. ### Evidence - Root-cause fix is in the planner: `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowPlanningService.java:145` plans drop as rule DistSQL only, `:171` rejects unsafe alter expansion, `:225` emits only encrypt rule DistSQL artifacts, and `:308` requires explicit rule inputs instead of deriving physical columns. - Secret handling is separated from execution: `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowExecutionService.java:229` uses display SQL for preview, `:323` masks failure messages, and `:372` still executes the raw artifact SQL. `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowArtifactBundle.java:124` keeps raw SQL plus masked display SQL for rule artifacts. - Descriptor and prompt contract match the new surface: `mcp/features/encrypt/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-encrypt.yaml:382` declares `distsql_artifacts`, `:387` declares `masked_property_preview`, and `mcp/features/encrypt/src/test/java/org/apache/shardingsphere/mcp/feature/encrypt/descriptor/EncryptToolDescriptorValidatorTest.java:97` verifies the response schema no longer exposes `ddl_artifacts`, `index_plan`, or `derived_column_plan`. - Runtime validation is rule-state based and masks properties: `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowValidationService.java:76` validates runtime encrypt rules, `:146` compares expected and actual rule state, `:214` masks property mismatches, and `:227` masks returned rule evidence. - Shared-layer blast-radius review did not find encrypt-specific leakage: `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowArtifactPayloadUtils.java:77` uses a generic rule-DistSQL-only workflow kind set for encrypt, mask, broadcast, readwrite, shadow, and sharding rule workflows. - E2E coverage exercises the key user-facing paths in `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/HttpProductionProxyEncryptWorkflowE2ETest.java`: plan/apply/validate without physical artifacts at `:80`, unsupported second encrypt column rejection at `:193`, manual-only mode at `:219`, unsupported alter expansion at `:265`, drop workflow at `:281`, and custom algorithm capability metadata at `:305`. ### Review Details - **Reviewed Scope:** Latest PR head `2bd9572efefba85b776b8cff89b86026b9c77935`; GitHub `/pulls/38836/files` matched the local triple-dot diff from merge base `32a929182f8d160d1779b2f7db9442ae7932a954`, covering 43 files. - **Not Reviewed Scope:** I did not use GitHub Actions status/check runs as merge evidence. I also did not review the full broad MCP roadmap in #35294 beyond this PR's encrypt workflow template slice. No parser, dialect grammar, routing precedence, dependency, or distribution manifest behavior is changed by this PR. - **Verification:** `git fetch apache master refs/pull/38836/head:refs/remotes/apache/pr/38836`: exit 0; `./mvnw -pl mcp/features/encrypt,mcp/core,mcp/support -am -DskipITs -Dspotless.skip=true -Dtest='WorkflowExecutionServiceTest,Encrypt*Test,WorkflowArtifactBundleTest,WorkflowArtifactPayloadUtilsTest' test -Dsurefire.failIfNoSpecifiedTests=false -B -ntp`: exit 0, 137 tests; `./mvnw spotless:check -Pcheck -T1C -B -ntp`: exit 0; `./mvnw checkstyle:check -Pcheck -T1C -B -ntp`: exit 0. Earlier on the same PR head, full MCP Docker E2E `*E2ETest` also passed: exit 0, 179 tests. - **Release Note / User Docs:** User/dev/test MCP docs are updated for the rule-DistSQL-only encrypt workflow. I do not see a separate `RELEASE-NOTES.md` blocker because this scoped template refinement introduces no new config key, migration step, dependency, or released API/SPI change. -- 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]
