-
Notifications
You must be signed in to change notification settings - Fork 214
feat(mcp): add notification handlers support for async client #568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(mcp): add notification handlers support for async client #568
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for server-side notification handlers to the MCP async client wrapper. The implementation enables the MCP client to receive and process logging notifications from the server and dynamically update the cached tools list when the server sends a tools/list_changed notification.
Changes:
- Implemented a two-phase build pattern in
McpClientBuilder.buildAsync()to support notification handlers that reference the wrapper - Changed
McpAsyncClientandMcpSyncClientfields toAtomicReferencefor thread-safe client replacement - Added notification consumer handlers for logging and tools list changes
- Enhanced close() methods for idempotency using getAndSet(null)
- Added 9 new test cases for the new functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
McpAsyncClientWrapper.java |
Changed client field to AtomicReference; added setClient() and updateCachedTools() methods; enhanced close() for idempotency |
McpSyncClientWrapper.java |
Changed client field to AtomicReference with null checks; enhanced close() for idempotency |
McpClientBuilder.java |
Implemented two-phase build pattern in buildAsync(); added logging and tools change notification consumers |
McpAsyncClientWrapperTest.java |
Added 9 new test cases for updateCachedTools(), setClient(), and close() idempotency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Log to SLF4J by level | ||
| switch (level.toLowerCase()) { | ||
| case "error" -> | ||
| logger.error( | ||
| "[MCP-{}] [{}] {}", | ||
| name, | ||
| loggerName, | ||
| data); | ||
| case "warning" -> | ||
| logger.warn( | ||
| "[MCP-{}] [{}] {}", | ||
| name, | ||
| loggerName, | ||
| data); | ||
| case "debug" -> | ||
| logger.debug( | ||
| "[MCP-{}] [{}] {}", | ||
| name, | ||
| loggerName, | ||
| data); | ||
| default -> | ||
| logger.info( | ||
| "[MCP-{}] [{}] {}", | ||
| name, |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buildSync() method does not implement notification handlers like buildAsync() does. For consistency and feature parity, synchronous clients should also support logging and tools change notifications. Consider implementing a similar two-phase build pattern for buildSync() with notification consumers.
| void updateCachedTools(List<McpSchema.Tool> tools) { | ||
| if (tools != null) { | ||
| // Clear and rebuild cache | ||
| cachedTools.clear(); | ||
| tools.forEach(tool -> cachedTools.put(tool.name(), tool)); | ||
| logger.info("[MCP-{}] Updated cached tools, total: {}", name, tools.size()); | ||
| } | ||
| } |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updateCachedTools() method performs a non-atomic clear-and-rebuild operation on cachedTools. Since this can be called asynchronously via notification handlers while other threads may be reading from the cache (e.g., via getCachedTool() or during initialization), this creates a race condition. While ConcurrentHashMap is thread-safe for individual operations, the clear-then-add pattern is not atomic. Consider synchronizing this method or using other concurrency patterns to ensure atomicity.
| * Invokes the package-private updateCachedTools method using reflection. | ||
| * | ||
| * @param tools the list of tools to update | ||
| */ | ||
| private void invokeUpdateCachedTools(List<McpSchema.Tool> tools) { | ||
| try { | ||
| java.lang.reflect.Method method = | ||
| McpAsyncClientWrapper.class.getDeclaredMethod("updateCachedTools", List.class); | ||
| method.setAccessible(true); | ||
| method.invoke(wrapper, tools); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to invoke updateCachedTools", e); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Invokes the package-private setClient method using reflection. | ||
| * | ||
| * @param client the MCP async client to set | ||
| */ | ||
| private void invokeSetClient(McpAsyncClient client) { | ||
| try { | ||
| java.lang.reflect.Method method = | ||
| McpAsyncClientWrapper.class.getDeclaredMethod( | ||
| "setClient", McpAsyncClient.class); | ||
| method.setAccessible(true); | ||
| method.invoke(wrapper, client); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to invoke setClient", e); | ||
| } |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using reflection to test package-private methods is fragile and makes tests harder to maintain. Consider making updateCachedTools() protected instead of package-private, or refactor the tests to verify the behavior indirectly through public methods (e.g., by triggering actual notifications if possible).
| * Invokes the package-private updateCachedTools method using reflection. | |
| * | |
| * @param tools the list of tools to update | |
| */ | |
| private void invokeUpdateCachedTools(List<McpSchema.Tool> tools) { | |
| try { | |
| java.lang.reflect.Method method = | |
| McpAsyncClientWrapper.class.getDeclaredMethod("updateCachedTools", List.class); | |
| method.setAccessible(true); | |
| method.invoke(wrapper, tools); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to invoke updateCachedTools", e); | |
| } | |
| } | |
| /** | |
| * Invokes the package-private setClient method using reflection. | |
| * | |
| * @param client the MCP async client to set | |
| */ | |
| private void invokeSetClient(McpAsyncClient client) { | |
| try { | |
| java.lang.reflect.Method method = | |
| McpAsyncClientWrapper.class.getDeclaredMethod( | |
| "setClient", McpAsyncClient.class); | |
| method.setAccessible(true); | |
| method.invoke(wrapper, client); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to invoke setClient", e); | |
| } | |
| * Invokes the package-private {@code updateCachedTools} method on the wrapper. | |
| * | |
| * @param tools the list of tools to update | |
| */ | |
| private void invokeUpdateCachedTools(List<McpSchema.Tool> tools) { | |
| wrapper.updateCachedTools(tools); | |
| } | |
| /** | |
| * Invokes the package-private {@code setClient} method on the wrapper. | |
| * | |
| * @param client the MCP async client to set | |
| */ | |
| private void invokeSetClient(McpAsyncClient client) { | |
| wrapper.setClient(client); |
| return Mono.error( | ||
| new IllegalStateException("McpAsyncClient not set. Call setClient() first.")); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message "McpAsyncClient not set. Call setClient() first." exposes internal implementation details about the two-phase build pattern. Since setClient() is package-private and not part of the public API, users should never see this message in normal usage. Consider using a more user-friendly message like "MCP client not available" or ensure this state is impossible in production code.
| return Mono.error( | |
| new IllegalStateException("McpAsyncClient not set. Call setClient() first.")); | |
| return Mono.error(new IllegalStateException("MCP client not available")); |
| throw new IllegalStateException( | ||
| "McpSyncClient not set. Call setClient() first."); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the async wrapper, this error message exposes internal implementation details. However, McpSyncClientWrapper doesn't have a setClient() method, making this message confusing and potentially misleading. Consider using a consistent message like "MCP client not available" across both wrappers.
| throw new IllegalStateException( | |
| "McpSyncClient not set. Call setClient() first."); | |
| throw new IllegalStateException("MCP client not available"); |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…upport-for-async-client' into feat/add-notification-handlers-support-for-async-client
close #115
AgentScope-Java Version
1.0.8-SNAPSHOT
Description
Background
The MCP (Model Context Protocol) specification includes server-side notifications such as:
Previously, the MCP client wrapper did not handle these notifications.
Purpose
Enable MCP client wrappers to:
Changes Made
How to Test
Run the unit tests:
mvn test -Dtest=McpAsyncClientWrapperTest -pl agentscope-core
mvn test -Dtest=McpSyncClientWrapperTest -pl agentscope-core
Checklist
Please check the following items before code is ready to be reviewed.