You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "terrymanu (via GitHub)" <gi...@apache.org> on 2026/05/28 04:00:15 UTC
[PR] Simplify tool elicitation handler [shardingsphere]
terrymanu opened a new pull request, #38740:
URL: https://github.com/apache/shardingsphere/pull/38740
Refactor MCP tool elicitation handling by extracting client capability detection and fallback response construction into dedicated public types. Restore ArgumentBinding semantic helper usage, merge elicitation workflow coverage into the real specification factory test, and add focused tests for new public contracts.
Also document helper extraction boundaries in AGENTS.md.
For #35294.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify tool elicitation handler [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify tool elicitation handler [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu merged PR #38740:
URL: https://github.com/apache/shardingsphere/pull/38740
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org