terrymanu commented on PR #39044:
URL: https://github.com/apache/shardingsphere/pull/39044#issuecomment-4910693445
### Summary
**Review Result: Mergeable**
Reason: This is a code-scope result only; CI was not reviewed by request. I
did not find a code-level blocker in the latest PR scope. The production change
keeps the completion context ownership inside `MCPCompletionService`, and the
added tests directly cover the evaluation, completion, and redaction contracts
touched by this PR.
### Evidence
- The local triple-dot scope matches GitHub `/pulls/39044/files`: 5 changed
files under `mcp/bootstrap` and `test/e2e/mcp`.
- The `MCPCompletionSpecificationFactory` simplification is
behavior-preserving for context mutation ownership:
`MCPCompletionSpecificationFactory.java:67` now passes the request context map
directly, while `MCPCompletionService.java:75` still creates its own
`LinkedHashMap` before inference/merge, and
`MCPCompletionRequestContext.java:45` still copies the context for providers.
- The encrypt tool-result redaction path is now guarded by
`MCPCallToolResultFactoryTest.java:123-125`, using a fixture that contains raw
`aes-key-value` data at `MCPCallToolResultFactoryTest.java:256` and a raw
DistSQL artifact at `MCPCallToolResultFactoryTest.java:259`.
- The MCP builder evaluation artifact now has a direct contract check for
the required feature planning tools and `completion/complete` at
`MCPBuilderEvaluationArtifactTest.java:105-114`.
- The HTTP completion E2E coverage now includes encrypt algorithm completion
through `plan_encrypt_rule` at `HttpTransportCompletionE2ETest.java:80-81`,
alongside the existing readwrite-splitting, shadow, sharding, and key-generator
completion checks.
- The evaluation text now names the concrete feature planning tools and
plugin resources instead of relying on vague wildcard wording, reducing the
chance of a shallow or under-specified MCP builder evaluation answer.
### Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: latest PR head `8cd94a0f17679dabcbcedc72168fefede0fc8178`;
base ref `master` at `75a9c69502f71a62b1c5ef53c13884e326c23e0e`; local
merge-base `9848b5b59a6b3ef45484335ac78aa83ce869b0ee`. Local changed-file list
matched GitHub `/pulls/39044/files`.
- Reviewed files:
-
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/completion/MCPCompletionSpecificationFactory.java`
-
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPCallToolResultFactoryTest.java`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/suite/MCPBuilderEvaluationArtifactTest.java`
-
`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/programmatic/HttpTransportCompletionE2ETest.java`
-
`test/e2e/mcp/src/test/resources/llm/evaluation/mcp-builder-evaluation.xml`
- Not Reviewed Scope: GitHub Actions/check-runs and full MCP Docker/LLM E2E
runtime behavior were not reviewed because this was a code-correctness review,
not a mergeability or CI review.
- Verification:
- `./mvnw -pl mcp/bootstrap
-Dtest=MCPCallToolResultFactoryTest,MCPCompletionSpecificationFactoryTest test
-Pcheck -T1C` exited `0`; 24 tests passed.
- `./mvnw -pl test/e2e/mcp -Dtest=MCPBuilderEvaluationArtifactTest test
-Pcheck -T1C` exited `0`; 5 tests passed.
- `./mvnw -pl test/e2e/mcp -Dtest=HttpTransportCompletionE2ETest test
-Pcheck -T1C` exited `0`; 4 tests were skipped by the local E2E Docker
condition.
- GitHub PR metadata, files, commits, review comments, and reviews were
accessible through public/token-backed REST. No review comments or reviews were
present at the time of review.
- Release Note / User Docs: Not required. The PR changes internal MCP
evaluation/test coverage and a behavior-preserving implementation
simplification; it does not add or change a user-facing configuration, API,
protocol version, migration path, or troubleshooting behavior.
--
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]
