CAMEL-24309: camel-ai-tool - AiToolRegistry listener SPI for tool registration changes - #25301
CAMEL-24309: camel-ai-tool - AiToolRegistry listener SPI for tool registration changes#25301Croway wants to merge 1 commit into
Conversation
…istration changes Add AiToolRegistryListener with toolRegistered/toolDeregistered callbacks fired on ai-tool consumer lifecycle events (route start/resume registers, stop/suspend deregisters). Callbacks fire outside the registry lock, only on actual state changes, and a failing listener cannot break registration. Prerequisite for MCP tools/list_changed notifications (CAMEL-24308). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
This review was generated by Claude Code, an AI assistant, on behalf of @gnodet.
This is a well-crafted SPI addition. The implementation is clean and the test coverage is thorough. Approving with a few minor suggestions below.
What works well
- Lock discipline: Notifications fire outside the
ReentrantLock, so listeners can safely re-read the registry (e.g., callgetTools()) without deadlocking. This is documented in theAiToolRegistryListenerJavadoc. - Error isolation: A throwing listener is caught and logged without affecting the registration itself or other listeners. This is verified by
testListenerExceptionDoesNotBreakRegistrationOrOtherListeners. - Idempotent no-ops: Re-adding the same spec instance fires no duplicate event; removing an absent spec fires nothing. Both are tested.
- Test quality: Tests follow all Camel conventions -- package-private classes, AssertJ, no
Thread.sleep(), no JUnitpublic. The lifecycle test usesCopyOnWriteArrayListfor thread safety while the unit test correctly uses plainArrayList. @since 4.22on the newAiToolRegistryListenerinterface.
Minor suggestions
-
removeListenermissing Javadoc — For consistency withaddListener(which has a doc block), consider adding a brief Javadoc comment, e.g.:/** * Removes a previously added listener. No-op if the listener was not registered. */ public void removeListener(AiToolRegistryListener listener) {
-
No null guard on
addListener— Ifnullis passed,CopyOnWriteArrayListaccepts it silently. Later, during notification,listener.toolRegistered(...)throws NPE which is caught and logged as a generic warning — making the root cause hard to diagnose. An earlyObjects.requireNonNullwould fail fast:public void addListener(AiToolRegistryListener listener) { Objects.requireNonNull(listener, "listener"); listeners.add(listener); }
-
Concurrency ordering caveat — Since notifications fire outside the lock, two concurrent threads performing
putandremoveon the same spec could theoretically deliverderegisteredbeforeregistered. In practice Camel route lifecycle is sequential per route, so this is unlikely. But since the Javadoc already documents the "subscribe-then-snapshot" idiom (showing attention to concurrency semantics), consider adding a note: "Notification ordering is guaranteed within a single thread but not across concurrent threads."
All minor — none are blockers. Nice work!
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 29 compile-only — current: 11 all testedMaveniverse Scalpel detected 40 affected modules (current approach: 11).
|
|
LGTM! @JiriOndrusek please review if there's an impact for Camel quarkus ? |
4 participants
CAMEL-24309: AiToolRegistry listener SPI for tool registration changes
Sub-task of CAMEL-24308 (camel-mcp-server). JIRA: CAMEL-24309
Changes
Adds a listener SPI to the
camel-ai-toolregistry so adapters can react to tools appearing/disappearing instead of polling — the prerequisite for the MCP server'stools/list_changednotification.AiToolRegistryListenerinterface withtoolRegistered(String tag, AiToolSpec spec)/toolDeregistered(String tag, AiToolSpec spec);tag == nulldenotes the default (untagged) pool, and a multi-tag endpoint fires one event per tag.AiToolRegistry.addListener/removeListener; callbacks fire outside the registry lock (so a listener can safely re-read the registry), only on actual state changes (idempotent re-put of the same spec and removal of an absent spec fire nothing, a rejected duplicate-name registration fires nothing), and a throwing listener is logged without affecting the registration or other listeners.Existing adapters (langchain4j-agent, spring-ai-chat) re-query the registry per exchange and are unaffected.
Testing
AiToolRegistryListenerTest— registry-level semantics (8 tests: event on put/remove, default pool null tag, no event on no-op or rejected mutations, listener isolation, removeListener).AiToolRegistryListenerLifecycleTest— events driven by realai-toolroute lifecycle (4 tests: start/stop, suspend/resume, multi-tag, untagged default pool).All 12 new tests pass; full
camel-ai-toolmodule test run is green.This PR was written by Claude Code on behalf of Federico Mariani (@Croway).
🤖 Generated with Claude Code