terrymanu commented on PR #39015:
URL: https://github.com/apache/shardingsphere/pull/39015#issuecomment-4886708673
### Summary
**Review Result: Mergeable**
Reason: This is a code-scope review only, and CI was not reviewed by
request. The latest PR head keeps the change within the MCP model-facing
guidance contract in the current base-to-head net diff, adds direct
payload/schema/test coverage for the new `summary`, `self_resource`,
`first_call_routes`, transport guidance, and top-level recovery `next_actions`,
and I did not find a code-level blocker in the reviewed scope.
### Evidence
- The MCP payload changes are additive and compatibility-preserving:
`self_uri` remains present while `self_resource` is added as a typed resource
hint, and response summaries are added without removing existing payload fields.
- Recovery handling now keeps existing nested `recovery.next_actions` and
also exposes the same actions at top level for model execution; focused tests
cover both `MCPErrorResponse` and runtime database validation payloads.
- Descriptor and model-facing formatter coverage were updated with the new
fields, including guidance baseline updates and formatter tests for `summary`
and `self_resource`.
- GitHub listed two `database/connector/core` files in the PR three-dot file
list, but against the current base ref they have no net diff; I reviewed that
as branch-history noise rather than current PR merge content.
- No high-frequency `computeIfAbsent`, dependency/distribution, public API
removal, protocol lifecycle, SQL parser, or release-note-required change was
found in the current net diff.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: PR #39015, latest head
`69adb82997a3d4d862346c2be56288cb83ae0732`, base ref `master` at
`6281889d27e70e8526db2d949a282d3488e73749`, local merge-base
`1f47e5ccf21b775c3aa18d57172882754a5a4955`. GitHub `/pulls/39015/files` listed
27 files and matched the local three-dot file list; current base-to-head net
diff contains 25 MCP/MCP-E2E files because the two database connector files are
already equivalent on current base.
- Not Reviewed Scope: GitHub Actions/check-runs/workflow logs, full release
packaging, and runtime production smoke tests were not reviewed because this
was not a mergeability or CI review.
- Verification:
- `./mvnw -pl
database/connector/core,mcp/support,mcp/core,mcp/bootstrap,test/e2e/mcp -am
-Dtest=DatabaseTypeFactoryTest,MCPCallToolResultFactoryTest,RuntimeStatusHandlerTest,MetadataResourceHandlerTest,MCPErrorResponseTest,RuntimeDatabaseValidationResultTest,SQLExecutionResponseTest,SearchMetadataToolHandlerTest,ExecuteUpdateToolHandlerTest,MCPGuidancePayloadBuilderTest,ServerCapabilitiesHandlerTest,MCPModelFacingPayloadContractTest,MCPModelContractAssertionsTest,LLMMCPModelFacingToolResponseFormatterTest
-Dsurefire.failIfNoSpecifiedTests=false test -Pcheck -T1C`: exit 0.
- `./mvnw checkstyle:check -Pcheck -T1C`: exit 0.
- Release Note / User Docs: Not required for this PR; the reviewed changes
are additive MCP model-facing payload/schema guidance improvements and do not
introduce user configuration, migration, compatibility, or operational behavior
changes requiring separate documentation.
--
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]
