terrymanu commented on PR #38827: URL: https://github.com/apache/shardingsphere/pull/38827#issuecomment-4639103486
### Summary No blocking issues found in the latest PR head. The PR now fixes the workflow-governance root cause instead of only adjusting downstream guidance: automatic workflow execution is gated by a persisted preview state and explicit `approved_steps`, rule planning is metadata-gated before artifact generation, encrypt/mask validation compares the planned full table rule state, and sensitive rule-property evidence is masked. **Merge Decision: Mergeable** ### Evidence - `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowExecutionService.java:125` now rejects invalid apply lifecycle states, requires `previewed` status before `review-then-execute`, rejects empty `approved_steps`, and rejects approval steps that were not visible in the latest preview. The previous implicit “empty approved steps means apply all” behavior is gone at `WorkflowExecutionService.java:358`. - `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowExecutionService.java:186` persists preview state as `previewed`; `WorkflowExecutionService.java:215` exposes explicit approval values; `WorkflowExecutionService.java:244` stops cleanly when preview has no artifacts. - Model-facing guidance is aligned with the new gate: recovery now tells the model to preview and copy visible approval steps at `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/protocol/error/MCPWorkflowRecoveryPayloadFactory.java:48`, the model-first common flow uses explicit `approved_steps` at `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPModelFirstContractPayloadBuilder.java:143`, and the descriptor says automatic execution requires a previewed plan plus explicit `approved_steps` at `mcp/support/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-support.yaml:167`. - Previewed plans remain discoverable through completion because `previewed` is included in eligible workflow statuses at `mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/completion/provider/WorkflowPlanIdCompletionProvider.java:38`. - Encrypt and mask planning now use metadata-gated planning context before rule artifact generation at `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowPlanningService.java:94` and `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskWorkflowPlanningService.java:91`. - Full expected rule state is captured during planning for drop and non-drop paths at `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowPlanningService.java:135`, `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowPlanningService.java:151`, and `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskWorkflowPlanningService.java:155`. - Validation now compares the expected complete table rule state and reports missing, extra, and property mismatches for encrypt and mask at `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowValidationService.java:95`, `mcp/features/encrypt/src/main/java/org/apache/shardingsphere/mcp/feature/encrypt/tool/service/EncryptWorkflowValidationService.java:142`, `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskWorkflowValidationService.java:90`, and `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskWorkflowValidationService.java:123`. - Sensitive algorithm property values are masked in validation evidence via `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowArtifactMaskUtils.java:83`; encrypt and mask validation use it at `EncryptWorkflowValidationService.java:217` and `MaskWorkflowValidationService.java:179`. - The old silent mask fallback path is removed. `mcp/features/mask/src/main/java/org/apache/shardingsphere/mcp/feature/mask/tool/service/MaskRuleInspectionService.java:45` and `MaskRuleInspectionService.java:68` now surface unavailable DistSQL instead of inventing fallback state. - Regression tests cover the original safety path and adjacent cases: missing approval, execution before preview, invisible approval steps, preview persistence, approved-only execution, skipped unapproved artifacts, metadata-missing planning, full expected-state validation, masked secret mismatch evidence, non-target rule loss, and removed mask fallback behavior. ### Review Details Reviewed Scope: - PR: https://github.com/apache/shardingsphere/pull/38827 - Latest head SHA reviewed: `b032a16af51cff5c1422303ac6f2ab11503f95f5` - Base SHA: `1e890783241fe4de1b781d3feb945b687d526205` - Merge-base used for local diff: `1e890783241fe4de1b781d3feb945b687d526205` - GitHub `/pulls/38827/files` returned 34 files, and the list matched local `git diff --name-only <merge-base>..refs/remotes/apache/pr/38827` exactly. - Scope reviewed: `.github/workflows/e2e-mcp.yml`, `mcp/core`, `mcp/support`, `mcp/features/encrypt`, and `mcp/features/mask` files changed by the PR. Not Reviewed Scope: - GitHub Actions/check-run status was intentionally not used for this merge decision. - Full repository `clean install` and live MCP E2E runtime were not rerun; the reviewed behavior is covered by scoped module tests and local code evidence. - No SQL parser, JDBC/Proxy high-frequency DML/DQL path, dependency manifest, distribution package, LICENSE, or NOTICE behavior was changed in this PR. Verification: - `curl` PR metadata/files plus local file-list diff boundary check: exit 0. - `./mvnw -pl "mcp/api,mcp/support,mcp/core,mcp/features/encrypt,mcp/features/mask" -am test -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false -B -ntp`: exit 0, build success, total time 33.389 s. - `./mvnw -pl "mcp/support,mcp/core,mcp/features/encrypt,mcp/features/mask" -am spotless:check -Pcheck -T1C -B -ntp`: exit 0, build success. - `./mvnw -pl "mcp/support,mcp/core,mcp/features/encrypt,mcp/features/mask" -am checkstyle:check -Pcheck -T1C -B -ntp`: exit 0, build success. Release Note/User Docs: - No additional release note is required for this scoped PR. The existing MCP user docs already describe preview-before-apply and user confirmation for rule changes; this PR tightens that existing safety contract and updates the model-facing descriptor, prompt, capability, and recovery payloads that drive MCP clients. -- 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]
