terrymanu commented on PR #38988:
URL: https://github.com/apache/shardingsphere/pull/38988#issuecomment-4880837155
### Summary
**Review Result: Mergeable**
Reason: Code-scope only review. I did not find a confirmed code-level
blocker in the latest PR scope; CI not reviewed by request.
### Evidence
- The latest PR scope is a responsibility-splitting refactor across MCP
descriptor validation, sharding workflow planning helpers, MCP
bootstrap/support tests, and MCP E2E harness helpers. I did not find
substantive unrelated behavior changes in the reviewed files.
- `MCPDescriptorCatalogValidator` still remains the catalog loader entry
point and now delegates to focused validators for resources, tools, prompts,
completion targets, and navigation. The moved validation checks preserve the
original validation path and have direct component tests.
- `ShardingWorkflowPlanningKernel` keeps the same public planning entry
points and lifecycle sequence; the new input validator and lifecycle spec
extract existing validation/specification responsibilities without changing the
workflow persistence or artifact planning path.
- The MCP E2E runner/readiness/runtime helper splits preserve the previous
client, transport, trace, and readiness behavior. The immediate completion
next-action path is covered by focused runner/planner tests.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: 31 files from GitHub `/pulls/38988/files`, covering
`mcp/support`, `mcp/features/sharding`, `mcp/bootstrap` tests, and
`test/e2e/mcp`. Latest PR head SHA: `d94c1b644ed6b7b88f78bc2b251b32fc5d0d0917`;
base ref: `master`; PR metadata base SHA:
`4004670ef7044e7bea76d4d3d29c425d4a8a0b37`; local merge-base used for
triple-dot review: `e4fd5c7784780b56bc8d3ead10436d94662ee326`. Local
changed-file list matched GitHub `/pulls/38988/files`.
- Not Reviewed Scope: GitHub Actions/check-run status and full Docker/LLM
E2E job execution were not reviewed in this code-correctness pass.
- Verification:
- `./mvnw -pl mcp/support,mcp/features/sharding,mcp/bootstrap -DskipITs
-Dspotless.skip=true -Dcheckstyle.skip=true
-Dsurefire.failIfNoSpecifiedTests=false -Dtest=... test` exit `0`; 132 focused
tests passed.
- `./mvnw -pl test/e2e/env -DskipTests -DskipITs -Dspotless.skip=true
-Dcheckstyle.skip=true install` exit `0`; used only to refresh the local E2E
test artifact before compiling `test/e2e/mcp`.
- `./mvnw -pl test/e2e/mcp -DskipITs -Dspotless.skip=true
-Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false
-Dtest=LLMMCPConversationTurnPlannerTest,LLMMCPConversationRunnerNextActionTest
test` exit `0`; 18 focused tests passed.
- Release Note / User Docs: Not required; the PR is an internal/test
refactor under the MCP umbrella issue and does not change user-facing
configuration, migration behavior, or documented MCP contracts.
--
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]
