terrymanu commented on PR #39016:
URL: https://github.com/apache/shardingsphere/pull/39016#issuecomment-4888367589
### Summary
**Review Result: Mergeable**
Reason: Code-scope review only. The latest PR head adds model-facing
workflow continuation fields through the existing MCP workflow payload
builders, keeps the descriptor/schema contract synchronized, and includes
focused tests for the changed behavior. CI not reviewed by request.
### Evidence
- Planning responses now add a top-level `summary` through the existing
`WorkflowPlanPayloadBuilder.build(...)` path, before the existing workflow
fields and guidance are appended, and the summary text is derived from the
current workflow status without changing lifecycle behavior:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowPlanPayloadBuilder.java:57`.
- Apply responses now add `summary` in the common apply response builder,
and preview responses intentionally reuse the existing review summary as the
top-level model-facing summary while keeping `review_summary` for
compatibility:
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowApplyResponseBuilder.java:60`,
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowApplyResponseBuilder.java:99`,
`mcp/core/src/main/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowApplyResponseBuilder.java:125`.
- Validation responses now include `summary`, and rejected validation
preconditions now return structured `next_actions` with concrete required
inputs for session mismatch versus invalid workflow state:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowValidationSupport.java:129`,
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowValidationSupport.java:222`.
- Descriptor validation was tightened so workflow planning, apply, and
validate descriptors must declare `summary`, preventing future descriptor
drift:
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPToolDescriptorValidationUtils.java:36`,
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPToolDescriptorCatalogValidator.java:207`.
- The output schemas and examples were updated for workflow apply/validate
and feature planning tools, including the support descriptor and feature
descriptors such as encrypt:
`mcp/support/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-support.yaml:216`,
`mcp/features/encrypt/src/main/resources/META-INF/shardingsphere-mcp/mcp-descriptors/mcp-descriptor-encrypt.yaml:270`.
- Tests cover the new planning/apply/validation summaries, validation
recovery `next_actions`, descriptor loader expectations, descriptor validator
requirements, and feature descriptor contract:
`mcp/core/src/test/java/org/apache/shardingsphere/mcp/core/workflow/WorkflowApplyResponseBuilderTest.java:43`,
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/workflow/service/WorkflowValidationSupportTest.java:45`,
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/descriptor/MCPDescriptorCatalogLoaderTest.java:43`,
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/descriptor/MCPToolDescriptorValidationUtilsTest.java:40`,
`mcp/features/encrypt/src/test/java/org/apache/shardingsphere/mcp/feature/encrypt/EncryptDescriptorContractTest.java:39`.
- User-facing MCP docs were updated to describe completion `next_actions`
and workflow `summary` / `next_actions` continuation behavior:
`docs/document/content/reference/mcp/protocol-surface.en.md:200`,
`docs/document/content/user-manual/shardingsphere-mcp/features/plugin-workflow.en.md:33`.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: latest PR head `2eb941748164989aa8681f6862c25256f5b66b7a`;
base `master` at `112867535801ca378c308ed3ae768159b2c7ad3b`; local merge-base
`112867535801ca378c308ed3ae768159b2c7ad3b`. Reviewed all 28 changed files from
GitHub `/pulls/39016/files`; the local triple-dot file list matched GitHub.
- Not Reviewed Scope: GitHub Actions/check-runs and Actions logs were not
reviewed because this was a code correctness review, not a mergeability or CI
review. No Proxy runtime, Docker, or LLM E2E run was reviewed.
- Verification: ran on detached PR head
`2eb941748164989aa8681f6862c25256f5b66b7a`.
- `./mvnw spotless:check -Pcheck -T1C` exited `0`.
- `./mvnw checkstyle:check -Pcheck -T1C` exited `0`.
- `./mvnw -pl
mcp/support,mcp/core,mcp/features/encrypt,mcp/features/broadcast,mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding
-am test -DskipITs -Dspotless.skip=true
-Dsurefire.failIfNoSpecifiedTests=false -B -ntp` exited `0`.
- Public PR metadata, file list, issue comments, review comments, and
reviews were accessible through GitHub REST; no public comments or reviews were
returned.
- Release Note / User Docs: user docs are required because the MCP
model-facing response contract is user-visible, and the PR updates the relevant
English and Chinese MCP protocol/user-manual pages. No separate release note
blocker found.
--
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]
