CAMEL-23382: camel-ai-tool - create unified AI tool component#24473
CAMEL-23382: camel-ai-tool - create unified AI tool component#24473zbendhiba wants to merge 5 commits into
Conversation
|
🌟 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: 651 tested, 23 compile-only — current: 76 all testedMaveniverse Scalpel detected 674 affected modules (current approach: 76).
|
zbendhiba
force-pushed
the
CAMEL-23382-step1-camel-ai-tool
branch
4 times, most recently
from
157b333 to
78fab84
Compare
Step 1: Create the new camel-ai-tool module with AiToolSpec, AiToolRegistry, consumer endpoint, and full tests. No changes to existing modules. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zbendhiba
force-pushed
the
CAMEL-23382-step1-camel-ai-tool
branch
from
78fab84 to
4a45e3e
Compare
Discussion: passing tool arguments as an exchange variable instead of headersThis PR changes how tool arguments reach the route compared to Behavioral changes
Why variables are safer than headers
Open question: keep the single wrapper variable (most conservative), or additionally set one variable per declared argument ( Claude Code on behalf of @Croway @zbendhiba @orpiske @davsclaus @oscerd this is something worth discussing, from a user perspective the header approach is more elegant, |
Croway
left a comment
Croway
left a comment
There was a problem hiding this comment.
Reviewed the new module in depth — the per-context registry and the security posture around argument passing are solid (see separate discussion comment). A handful of findings below, two of which I'd consider pre-merge: the broken SQL doc example and the executor's uncaught body-conversion path. Also one structural question best settled now: the package is org.apache.camel.component.ai.tools (plural) while the artifact/scheme are ai-tool (singular) — once steps 2–6 wire other modules against these classes, renaming becomes a breaking change.
Claude Code on behalf of @Croway
|
|
||
| from("ai-tool:queryUser?tags=users&description=Query database by user ID" + | ||
| "¶meter.id=integer¶meter.id.description=The user ID¶meter.id.required=true") | ||
| .to("sql:SELECT name FROM users WHERE id = :#id"); |
There was a problem hiding this comment.
This example won't work: :#id binds from headers/body, but tool arguments are in the AiToolArguments exchange variable. Either use :#${variable.AiToolArguments.parameters[id]} or add a processor step that extracts args.getInteger("id") into a header first. Worth fixing since it's the pattern users will copy (the XML and YAML tabs have the same issue).
There was a problem hiding this comment.
so I guess your call on headers comes from this. which is a valid thing.
I guess this one should be revised.
There was a problem hiding this comment.
Let me review that. I've deleted the headers for security when we move those to the Executor, however this adds issues for execution
| } | ||
|
|
||
| String result = exchange.getMessage().getBody(String.class); | ||
| LOG.debug("Tool '{}' execution completed successfully", toolName); |
There was a problem hiding this comment.
This sits outside the try/catch, so a TypeConversionException (route returns a non-convertible body) propagates to the adapter — contradicting the documented contract that all errors come back as typed AiToolResult variants. Suggest wrapping it and returning an ExecutionError.
| "A toolName must be provided: ai-tool:<toolName>?description=<desc>"); | ||
| } | ||
|
|
||
| final String toolName = StringHelper.before(remaining, "/", remaining); |
There was a problem hiding this comment.
ai-tool:myTool/anything silently drops /anything. Since the syntax is ai-tool:toolName with no path segments, better to fail on a / (or use remaining as-is) so typos surface instead of registering a truncated tool name.
| } | ||
|
|
||
| @Override | ||
| protected void doStop() throws Exception { |
There was a problem hiding this comment.
Route suspend doesn't deregister the tool (no doSuspend override), so a suspended route stays in the registry and the executor will still dispatch into its processor. Either deregister on suspend or have AiToolExecutor check the consumer's state before dispatching — and document whichever is chosen.
| synchronized (set) { | ||
| for (AiToolSpec existing : set) { | ||
| if (existing.getName().equals(spec.getName()) && existing != spec) { | ||
| LOG.warn("Duplicate toolName '{}' under tag '{}' -- the LLM adapter will see both " |
There was a problem hiding this comment.
Warn-only on duplicate tool names means both specs stay registered, and most LLM APIs (OpenAI included) reject duplicate function names — so the failure surfaces later and far from the cause. Since this registry is the foundation for steps 2–6, consider failing fast at registration, or documenting last-wins semantics.
| @UriParam(description = "Comma-separated list of tags used to group tools. " | ||
| + "Producers filter the registry by these tags to select which tools to expose to the LLM. " | ||
| + "When omitted, the tool goes into a default pool available to all producers.") | ||
| private String tags; |
There was a problem hiding this comment.
Missing @Metadata(label = "consumer") — the generated catalog JSON shows "label": "" for tags while description/parameters have "consumer".
| * | ||
| * @since 4.22 | ||
| */ | ||
| public final class AiToolRegistry { |
There was a problem hiding this comment.
This class relies on synchronized throughout — synchronized (context) in getOrCreate, Collections.synchronizedSet, and synchronized (set) blocks in put/getToolsByTag/getAllTools/getTools/getDefaultTools. Camel supports virtual threads, and the project has been systematically replacing synchronized with ReentrantLock for virtual-thread compatibility (CAMEL-20199, e.g. #21703) — new code shouldn't reintroduce it. A ReentrantLock-guarded LinkedHashSet keeps the insertion ordering, or CopyOnWriteArraySet would fit this read-heavy registry with no locking at all.
I assume this change. Keep in mind that this is internal to components using the camel ai tool. I've already started implementing for camel langchain4j agent and camel spring ai chat components. There is 0 impact for users. there's no reason to propagate headers as those are the internal LLM calls. I didn't spent time on langchain4j-tools internal. I marked in the spec this component as to deprecate. |
revised, let me look at this again |
|
ACK ... This one is on my TODO list for tomorrow morning |
orpiske
left a comment
orpiske
left a comment
There was a problem hiding this comment.
High level review from the perspective of downstream consumers
Claude Code on behalf of Otavio Rodolfo Piske
Overall Assessment: Approve (Step 1)
This PR introduces a clean, well-scoped abstraction layer. Step 1 is purely additive — no existing modules are modified. The analysis below focuses on how this new module interacts with downstream bean-factory systems like Forage.
Compatibility
The AiToolRegistry and AiToolSpec abstractions operate at a different layer than bean-based agent creation. Agent beans (ChatModel + Memory + RAG + Guardrails) are created and bound to the CamelContext registry via registry.bind(). Tools are registered in the separate AiToolRegistry plugin. These are complementary — the agent is the "brain," the tools are the "hands." The ?bean= URI syntax for agent lookup is unaffected.
Beans still work. No conflicts.
Extension Points (positive)
The module exposes useful hooks for downstream consumers:
AiToolRegistry.getOrCreate(CamelContext)— clean programmatic access for registering tools outside of route DSLAiToolSpecconstructor is public — third parties can build and register tool descriptors- Tag-based discovery maps well to multi-agent configurations
Concerns for Steps 2-5
-
Argument passing change: The shift from exchange headers (
exchange.getMessage().setHeader(name, value)) to exchange variables (AiToolArguments) in Step 2 will be a breaking change for existing tool routes that read parameters via${header.city}. This deserves a clear upgrade guide entry when it lands. -
Missing SPI for third-party tool sources: There is no
AiToolProviderSPI for external tool catalogs (e.g., MCP server catalogs, property-driven tool definitions). All tools must come through eitherAiToolConsumerroutes or programmaticAiToolRegistry.put(). Consider whether a provider SPI would be valuable for Step 6 or beyond. -
URI migration path: When
langchain4j-tools:consumer is deprecated (Step 5), users will need to migrate toai-tool:. A smooth deprecation period where both work simultaneously would reduce friction.
Summary
Step 1 is safe to merge. The architecture is sound and the separation of concerns is correct. The risks noted above are all in future steps and are flagged early so they can be addressed as the roadmap progresses.
| /** | ||
| * Exchange variable name under which {@link AiToolArguments} is set by the executor. | ||
| */ | ||
| public static final String TOOL_ARGUMENTS = "AiToolArguments"; |
There was a problem hiding this comment.
We should probably use CamelXxx format. The proper variable name should be CamelAiToolArguments.
I am OK with this change, although:
Whether to keep a single wrapper or per variable ... I don't have a strong opinion. I think the one variable per argument approach would solve one of my concerns above, but I am unsure how much more complicated it is to implement. |
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Nice work, Zineb — reviewed mainly from a security and integration-correctness angle.
The security posture is the part I want to call out as well-designed: untrusted LLM tool-call arguments are wrapped under a single constant key into an exchange variable (exchange.setVariable(AiTool.TOOL_ARGUMENTS, ...)) rather than spread into Exchange headers. That deliberately avoids the raw-name→header mapping behind the camel-langchain4j-tools header-injection family (CVE-2025-27636 and follow-ons) — argument names can't pollute the header namespace, and variables don't auto-propagate or drive dispatch. AiToolResult also carries the note about not returning raw exception text to the LLM. Good hardening. MojoHelper registration, generated files and conventions all check out, and CI is green.
Concurring with @Croway's two pre-merge items:
- The executor's body conversion
exchange.getMessage().getBody(String.class)sits after the try/catch, so aTypeConversionExceptionwould escape the "all errors returned as a typedAiToolResult" contract instead of becoming anExecutionError. - The flagship doc example binds
:#idin the SQL, but arguments now live in theAiToolArgumentsvariable, so the copy-paste example won't bind at runtime (functional only — it stays parameterized, so no injection concern).
Thanks for taking the hardened approach here.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
oscerd
left a comment
oscerd
left a comment
There was a problem hiding this comment.
Step 1 review — conventions & quality pass
Really nice groundwork: the design doc (design/aiTool.adoc) plus the sequential-PR plan make this easy to review, and the Step 1 scoping (no changes to existing modules) is exactly right. Checked against the repo conventions:
- ✅ MojoHelper registration,
camel-aimodule list, parent/BOM/allcomponents, catalog + component/endpoint DSL regeneration, docs symlinks andnav.adoc— all present and consistent - ✅ Only
camel-supportas runtime dependency; no new external dependencies - ✅ Comprehensive unit tests (including a registry concurrency test); no
Thread.sleep - ✅ Passing tool arguments as a single typed exchange variable instead of per-name headers avoids header-namespace collisions entirely — good call, and in line with the committer security checklist for consumers
- ✅
additionalProperties: falsein the generated schema and the sanitization note onAiToolResultare thoughtful touches
I left 5 inline comments: 3 small correctness/consistency items (undeclared-argument handling vs. its log message, a sql doc example that won't bind with variable-based arguments, a blank-tags edge case) and 2 questions.
One module-wide naming question that doesn't fit a single line: the Java package is org.apache.camel.component.ai.tools (plural) while the artifact and scheme are camel-ai-tool / ai-tool (singular). Since Steps 2–4 will make other modules depend on these classes, and package renames after a release are breaking, it may be worth aligning on one form now (e.g. org.apache.camel.component.ai.tool).
None of this is blocking — looking forward to the follow-up steps.
This review is a conventions/quality pass and does not replace dedicated review tooling or static analysis (e.g. SonarCloud).
Claude Code on behalf of oscerd
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| } | ||
|
|
||
| // Defensive copy so callers cannot mutate arguments during route execution | ||
| Map<String, Object> argsCopy = arguments != null ? new HashMap<>(arguments) : Map.of(); |
There was a problem hiding this comment.
The warn above says the undeclared argument is being ignored, but argsCopy copies all entries — undeclared arguments included — so they remain accessible to the route via AiToolArguments. The generated schema also advertises additionalProperties: false to the model.
Two ways to make this consistent:
- Actually filter undeclared arguments out of
argsCopywhen the tool declares parameters (defense-in-depth: extra parameters an LLM hallucinates — or is prompted into sending — never reach the route), or - Keep the pass-through behavior and reword the log message/Javadoc (the design doc says "warned about but not rejected").
Option 1 seems preferable given the schema already tells the model additionalProperties: false. Either way, a test asserting the chosen behavior would pin it down — testExecuteIgnoresUndeclaredArguments currently passes under both semantics (it never checks whether extraParam is present in AiToolArguments).
|
|
||
| from("ai-tool:queryUser?tags=users&description=Query database by user ID" + | ||
| "¶meter.id=integer¶meter.id.description=The user ID¶meter.id.required=true") | ||
| .to("sql:SELECT name FROM users WHERE id = :#id"); |
There was a problem hiding this comment.
This example won't bind as written: camel-sql resolves :#id from the body-as-Map, the headers, or an exchange variable named id (see SqlHelper.lookupParameter), but with this design the arguments live only inside the single AiToolArguments variable — there is no id variable or header, and the body is not the arguments map. (The pattern worked with the old header-based tools components, which is probably where the example stems from.)
Options: use a simple-expression binding (e.g. :#${variable.AiToolArguments.parameters[id]} — worth a quick verification), switch the example to a processor/bean that reads AiToolArguments, or — if :#name ergonomics are a goal for tool routes — consider having the executor additionally expose declared arguments individually. The same applies to the XML tab (line 188) and the YAML tab (line 226).
|
|
||
| AiToolRegistry registry = AiToolRegistry.getOrCreate(getEndpoint().getCamelContext()); | ||
| String tags = configuration.getTags(); | ||
| if (tags != null && !tags.isBlank()) { |
There was a problem hiding this comment.
Edge case: a tags value that is non-blank but contains only blank items (e.g. tags=,) passes this check, but splitTags then returns an empty array — so the loop below registers the tool nowhere (neither under a tag nor in the default pool), silently. Similarly, tags=,research registers the tool under the empty-string tag "".
Suggestion: filter blank entries in splitTags, and if nothing remains fall back to the default pool here, matching the documented "when omitted" semantics.
| "A toolName must be provided: ai-tool:<toolName>?description=<desc>"); | ||
| } | ||
|
|
||
| final String toolName = StringHelper.before(remaining, "/", remaining); |
There was a problem hiding this comment.
Question: this silently drops anything after the first / (ai-tool:foo/bar → tool name foo). Since the syntax is just ai-tool:toolName, silently truncating user input seems surprising — is the / handling intentional (future-proofing?), or would rejecting / keeping the full remaining be clearer? A short comment would help either way.
| // LangChain4j agent uses only weather-tagged tools | ||
| from("direct:chat") | ||
| .to("langchain4j-agent:assistant?agent=#myAgent&tags=weather"); | ||
| ---- |
There was a problem hiding this comment.
Minor: this section shows the langchain4j-agent producer consuming registry tools, but that wiring only lands in Step 2 — a reader of the nightly docs could reasonably expect it to work today. The See Also section already says "future release"; a short NOTE admonition here as well would avoid confusion until Steps 2–4 land.
EIPs and Camel components should frankly use exchange properties. We made variables only for end users to have full namespace for their own variables that dont get mixed up with internal camel state. |
zbendhiba
force-pushed
the
CAMEL-23382-step1-camel-ai-tool
branch
from
57894da to
f9bfe30
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest commits (f9bfe30, adebbe7). The follow-up changes substantively address prior review feedback:
- Package rename from
ai.tools(plural) toai.tool(singular) synchronizedreplaced withReentrantLock- Body conversion moved inside try/catch
/rejection in tool names- Suspend/resume lifecycle support
- Blank-tag edge case handled
- Duplicate tool names now fail-fast
- Doc examples fixed to use bean/header pattern
AiToolArgumentsexchange variable removed in favor of per-header arguments with Camel-prefix filtering
Security posture is sound: Camel-prefixed header names are rejected (CVE-2025-27636 family), undeclared arguments are filtered out, and AiToolResult documents sanitization requirements. Test coverage is thorough (lifecycle, executor edge cases, registry concurrency, parameter parsing, Camel-prefix rejection).
LGTM.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
Thanks everyone for the reviews and the feedback ! |
Step 1: Create the new camel-ai-tool module with AiToolSpec, AiToolRegistry, consumer endpoint, and full tests. No changes to existing modules.
Description
Summary
Create the new
camel-ai-toolmodule with the unified AI tool abstraction layer:AiToolSpec— vendor-neutral tool descriptor (name, description, parameters, tags)AiToolRegistry— central registry for discovering and managing AI toolsAiToolConsumer— endpoint that registers Camel routes as AI-callable toolsNo changes to existing modules in this PR.
Roadmap
Technical design details : design/aiTool.adoc file
This work is split into several sequential PRs to keep reviews focused:
camel-ai-toolmodule withAiToolSpec,AiToolRegistry, consumer endpoint, and full tests. No changes to existing modules.camel-langchain4j-agentproducer to read tools fromAiToolRegistryinstead of the old cache, with upgrade guide entry.camel-spring-ai-chatproducer to discover tools fromAiToolRegistry, with upgrade guide entry.camel-langchain4j-toolsproducer to read fromAiToolRegistry, keeping the agentic loop intact.camel-langchain4j-toolsconsumer side and removecamel-spring-ai-toolsentirely, with upgrade guide entries for both.camel-openai. Resolve tools fromAiToolRegistryand implement the dispatch loop(separate Jira issue).