terrymanu commented on PR #38873:
URL: https://github.com/apache/shardingsphere/pull/38873#issuecomment-4751719033
### Summary
- Merge Decision: Mergeable
- I found no blocking issues in the latest PR head
`a5de45c39112a66eddb2518f07430d6c23ac41eb`.
- The GitHub file list matches the local triple-dot diff from merge base
`242c11f73af82310bc9f85d71045dff7c7fe46eb`: 96 files, all within `mcp/**` and
`test/e2e/mcp/**`.
### Issues
No blocking issues found.
### Review Details
- Root-cause repair looks complete for the reviewed scope:
- The hidden `database_gateway_validate_proxy_connectivity` alias is
removed from `ToolDefinitionRegistry`; tests now assert the legacy name is
unsupported.
- `user_overrides` is removed from public descriptors and guarded by
`MCPDescriptorCatalogValidator` / `WorkflowRequestBinder`.
- MCP completion next-action payloads now use canonical `ref`, `argument`,
and `context` fields; legacy completion arguments are rejected by E2E runner
tests.
- The LLM E2E side-effect preview path now handles both next-turn and
same-completion read-only SQL misrouting without executing
`database_gateway_execute_update` for the SELECT verification path.
- Side-effect and adjacent-feature scan:
- Shared workflow binding changes are reflected across MCP feature
descriptors and handlers.
- Readwrite-splitting status no longer exposes `operation_type` as a
status alias and rewrites model-facing status artifacts to `target_status`.
- I did not find substantive unrelated changes outside the PR goal.
- Public review context:
- No PR review comments or issue comments were present at review time.
- GitHub Actions status was not used for this merge decision.
### Verification
Ran against the latest PR head:
- `./mvnw -pl mcp/core,mcp/support,mcp/features/readwrite-splitting -am
-DskipITs -Dspotless.skip=true
-Dtest=ToolDefinitionRegistryTest,MCPToolArgumentsTest,WorkflowRequestBinderTest,MCPNextActionUtilsTest,MCPModelFirstContractPayloadBuilderTest,ReadwriteSplittingToolHandlerTest,ReadwriteSplittingDistSQLPlanningServiceTest,ReadwriteSplittingWorkflowPlanningServiceTest
test -Dsurefire.failIfNoSpecifiedTests=false`
- Exit code: 0
- Result: `BUILD SUCCESS`
- `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true
-Dtest=LLMMCPActionExecutorTest,LLMMCPToolDefinitionFactoryTest,LLMMCPConversationRunnerNextActionTest,LLMMCPConversationRunnerTest,LLMMCPInteractionCoverageTest
test -Dsurefire.failIfNoSpecifiedTests=false`
- Exit code: 0
- Result: `BUILD SUCCESS`
- `./mvnw -pl mcp -am -DskipITs -Dspotless.skip=true -DskipTests package`
- Exit code: 0
- Result: `BUILD SUCCESS`
### Release Notes / Docs
No release-note or user-doc blocker found for this review. The PR updates
the in-repo MCP descriptors, model-contract baseline, and tests that define the
reviewed MCP surface.
--
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]
