terrymanu commented on PR #39011:
URL: https://github.com/apache/shardingsphere/pull/39011#issuecomment-4886271667
### Summary
**Review Result: Mergeable**
Reason: Code-scope review of the latest commit only
(`e08f9b8bb4ab8194e6d8350d616748fcd704f5a1`) found no code-level blocker. CI
not reviewed by request.
### Evidence
- The new LLM discovery bridge actions are wired through the model-facing
schema, executor, trace record, and turn planner paths:
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/conversation/LLMMCPToolDefinitionFactory.java:41`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/conversation/LLMMCPActionExecutor.java:44`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/conversation/LLMMCPTraceRecordFactory.java:31`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/conversation/LLMMCPConversationTurnPlanner.java:47`
- JSON-RPC error normalization now preserves `error.data` while retaining
the stable normalized `error_code` and top-level error message:
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/support/transport/MCPInteractionPayloads.java:168`
- covered by
`MCPInteractionPayloadsTest.assertGetJsonRpcErrorPayloadPreservesErrorData`.
- The URL elicitation fallback contract is internally consistent:
URL-capable sensitive fallback still reports `url_mode_not_implemented`, but
the selected interaction is `structured_fallback`, and the encrypt/mask
descriptors no longer advertise `url_fallback`.
- The extended MCP discovery scenario now requires `tools/list`,
`resources/list`, `resources/templates/list`, `resources/read`, and the query
tool in both allowed and required tool lists:
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/suite/usability/scenario/LLMUsabilityScenarioCatalog.java:155`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/suite/usability/scenario/LLMUsabilityScenarioCatalogTest.java:80`
- Public GitHub issue comments, inline review comments, and reviews for this
PR were empty at review time, so no previous public feedback needed multi-round
comparison.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: latest commit only,
`e08f9b8bb4ab8194e6d8350d616748fcd704f5a1` (`Improve MCP discovery bridge and
fallback contracts`), parent `8d2f14c013b9016f5cf7da299b1c40cb80981dbf`. The
local last-commit file list matched the GitHub commit API file list: 18
modified files under `mcp/bootstrap`, `mcp/features/encrypt`,
`mcp/features/mask`, and `test/e2e/mcp`.
- Not Reviewed Scope: earlier PR commits, full PR cumulative diff outside
the last commit, GitHub Actions/check-runs, Docker/native LLM E2E runtime
execution.
- Verification:
- `./mvnw -pl mcp/bootstrap
-Dtest=MCPToolElicitationFallbackReasonTest,MCPToolElicitationFallbackResponseFactoryTest,MCPToolElicitationFlowTest
-DfailIfNoTests=false -DfailIfNoSpecifiedTests=false -Dcheckstyle.skip=true
-Dspotless.check.skip=true -Dlicense.skip=true test -Pcheck -T1C` exit 0.
- `./mvnw -pl test/e2e/mcp
-Dtest=LLMMCPToolDefinitionFactoryTest,LLMMCPActionExecutorTest,LLMMCPConversationRunnerTest,LLMMCPConversationTurnPlannerTest,MCPInteractionPayloadsTest,LLMUsabilityScenarioCatalogTest
-DfailIfNoTests=false -DfailIfNoSpecifiedTests=false -Dcheckstyle.skip=true
-Dspotless.check.skip=true -Dlicense.skip=true test -Pcheck -T1C` exit 0.
- Release Note / User Docs: not required for this last-commit scope; it
adjusts MCP E2E bridge coverage and aligns descriptor/runtime fallback contract
without introducing a user-facing configuration or migration requirement.
--
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]
