| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
AI-assisted review: the new integration assertion appears to check the wrong failure mode. listRoots() returns a Mono<ListRootsResult>, and the new guards return Mono.error(...); they do not throw synchronously when exchange.listRoots() is called. assertThatThrownBy(exchange::listRoots) only invokes the method reference and receives the Mono, so it should not observe the IllegalStateException unless the publisher is subscribed/blocked. Could this assertion consume the publisher (for example assertThatThrownBy(() -> exchange.listRoots().block())...) or use StepVerifier as the unit tests do? Otherwise the integration test does not actually pin the new fail-fast behavior.
Sorry, something went wrong.
|
@soyeladice-svg Thanks for checking this. That concern would apply to the async exchange, but this test uses SyncToolSpecification. Its handler receives McpSyncServerExchange, whose listRoots() already calls .block(). So assertThatThrownBy(exchange::listRoots) subscribes through that wrapper and observes the IllegalStateException. I reran the 41 async exchange tests and both HTTP/SSE testRootsWithoutCapability tests. All 43 passed with no failures or errors. The async unit tests use StepVerifier and also check that the session is never contacted. I’ve kept the integration assertion as it is because the synchronous wrapper already consumes the publisher. |
Sorry, something went wrong.
|
You're right — I traced the handler type again and missed that this integration path receives McpSyncServerExchange, whose listRoots() blocks the underlying Mono. In that context assertThatThrownBy(exchange::listRoots) does exercise the intended failure, while the async unit coverage correctly uses StepVerifier. Thanks for the precise correction and the 43-test rerun; my concern is resolved. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
A server calling listRoots against an uninitialized client or one without the roots capability currently sends an unsupported roots/list request. Add the same fail-fast checks used by sampling and elicitation, returning an IllegalStateException before contacting the session.
Fixes #1067.
Both listRoots overloads are covered by regression tests that assert no session interaction before or after subscription. Roots support with listChanged=false remains valid. The shared client/server integration test now asserts the local exception and a successful tool response instead of allowing an unasserted call to pass.
Validation on Windows with Temurin 17.0.19 and embedded Tomcat 11.0.2:
This is an AI-assisted contribution requiring human review. The repository's AI submission threshold is not met; the exact required disclosure is included in disclosure.txt.