terrymanu commented on PR #38740:
URL: https://github.com/apache/shardingsphere/pull/38740#issuecomment-4560921121
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** Latest PR head `5d9029d` against merge-base `3ae97b6`;
`AGENTS.md` plus MCP bootstrap elicitation production/test changes under
`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool`
and
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool`.
- **Not Reviewed Scope:** GitHub Actions/check-run status; real MCP client
end-to-end interop beyond the covered unit/workflow tests.
- **Need Expert Review:** No mandatory expert review found. Optional MCP
protocol maintainer review may still be useful for capability interpretation.
### Basis
- The PR scope is focused: `git diff --name-status 3ae97b6..5d9029d` shows
12 files, limited to `AGENTS.md` and MCP elicitation code/tests.
- The refactor keeps the original behavior shape while reducing handler
responsibility:
- Client capability detection moved from the handler into
`MCPClientElicitationCapabilities.from(...)` with the same form/url detection
logic
(`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPClientElicitationCapabilities.java:43`).
- Fallback reason strings and selected interaction values are centralized
in `MCPToolElicitationFallbackReason` without changing payload values
(`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolElicitationFallbackReason.java:30`).
- Fallback payload creation, sensitive-question sanitization, and
`elicitation_support` construction are extracted into
`MCPToolElicitationFallbackResponseFactory`
(`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolElicitationFallbackResponseFactory.java:55`).
- The small behavior simplification in `MCPToolClarificationPolicy` is safe:
empty question lists are already rejected before form construction
(`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolClarificationPolicy.java:67`),
and form field names are generated from monotonically increasing question
indexes
(`mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolClarificationPolicy.java:110`).
- The extracted public types have direct focused tests, and the real
specification factory test now covers the end-to-end handler path instead of a
non-existent target class:
-
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPClientElicitationCapabilitiesTest.java:37`
-
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolElicitationFallbackReasonTest.java:37`
-
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolElicitationFallbackResponseFactoryTest.java:41`
-
`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolSpecificationFactoryTest.java:293`
- Regression scan did not find unresolved risk:
- Original interactive STDIO elicitation path is covered
(`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolSpecificationFactoryTest.java:294`).
- Adjacent disabled/unsupported paths are covered: sensitive input,
URL-only client, HTTP transport, missing capabilities, malformed/invalid/stale
results
(`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolSpecificationFactoryTest.java:331`).
- Non-planning tools and tools without runtime descriptors still bypass
elicitation
(`mcp/bootstrap/src/test/java/org/apache/shardingsphere/mcp/bootstrap/transport/capability/tool/MCPToolSpecificationFactoryTest.java:461`).
### Verification
- Reviewer-run local verification:
- `./mvnw -pl mcp/bootstrap -am -DskipITs -Dspotless.skip=true
-Dtest=MCPClientElicitationCapabilitiesTest,MCPToolElicitationFallbackReasonTest,MCPToolElicitationFallbackResponseFactoryTest,MCPToolClarificationPolicyTest,MCPToolSpecificationFactoryTest
test -Dsurefire.failIfNoSpecifiedTests=false` passed; 72 tests run, 0
failures, 0 errors.
- `./mvnw spotless:check -Pcheck -T1C` passed.
- `./mvnw checkstyle:check -Pcheck -T1C` passed.
- No GitHub Actions, CI status, or check-run completion was used for this
verdict.
--
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]
