Repository navigation
CAMEL-25985: Expose AI tool JSON arguments and invocation identifiers - #28295
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The required-argument validation runs before the camel-prefix strip, which can silently discard a declared and required parameter.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Federico, three points need a change before merge. The rest is optional.
-
Blocking - LangChain4j: an agent without memory gets
CamelAiToolConversationId: default. LangChain4j 1.22 falls back toChatMemoryService.DEFAULT("default") as the chat memory id when the service method has no@MemoryId, andAiAgentWithoutMemoryServicehas none. SomemoryIdis never null here. The tool route then receives a made-up id, and every caller of a memory-less agent shares the same "conversation". This goes against the JIRA and the new docs ("An unavailable identifier is omitted rather than generated"). Pass the memory id only when the agent has memory (inline comment). The concurrency test now always configures a chat memory provider, so the memory-less path is no longer covered. Please add a test for it that expects no conversation header. -
Blocking - The body keeps undeclared arguments. The JIRA asks for "the complete accepted arguments" built "from arguments accepted by the executor's existing filtering". The adoc also says "Camel always allowlists top-level property names when invoking the tool route, even if the schema sets
additionalProperties: true" (line 199), and the generated schema publishesadditionalProperties: false. With this PR, an argument the schema forbids still reaches the route through the body. A route that forwards the body to a back end (an obvious use ofargumentsAsBody) then passes on fields the model invented, for example apriceorrolefield added after a prompt injection. Declared arguments are untrusted too, but the allowlist exists so the route only gets the fields the tool declares. Please build the body from the filtered set (the same keys as the headers). If you do want to keep extra fields when the schema allowsadditionalProperties, make it a separate decision and update line 199 to match. The testpreservesUndeclaredArgumentsOnlyInTheBody(zero-parameter tool, see CAMEL-24241) would then expect{}. -
Blocking - The catalog copy of the doc is not regenerated.
catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/ai-tool-component.adocdoes not have the two new sections, so the uncommitted-changes check will fail. Regenerate the catalog and commit it. -
Not blocking - OpenAI Responses API:
OpenAIResponsesProducerresolves a conversation id (CamelOpenAIConversationId/conversationId), butMcpToolCallExecutoralways passesnull. The docs only mention the chat-completion callback. Either pass the Responses conversation id through, or say in the docs that the Responses API does not supply one either. -
Not blocking - My PR #28250 (still open) adds
@BodyTypestoAiToolEndpointwithconsumerOut= no body. Whichever PR merges second has to describe the JSON String body that depends onargumentsAsBody. -
Optional -
McpServerBridgestill calls the 3-argexecute. The MCP request id or session id could fill these headers later. The docs already say MCP does not propagate them, so this can be a follow-up. -
Optional - In
LangChain4jAgentExecuteToolsConcurrentlyTest,label.toLowerCase()should useLocale.ROOT.
Checked, no change needed:
- Both headers are
Camel-prefixed and documented with@MetadatathroughheadersClass, so they appear in the catalog and the endpoint DSL. - The default header filter keeps them away from http and other outbound producers.
- Model-supplied
CamelAiTool*argument names cannot override them, and stale values are removed. - Nothing evaluates the arguments or the JSON body.
- Existing headers are unchanged, and the new option defaults to
false, so no upgrade-guide entry is needed. - Invalid JSON is still rejected in each adapter before the executor runs.
- Tests use AssertJ, like the rest of these modules, and do not use
Thread.sleep. - CI: the build is still pending.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Missing catalog adoc sync (CI-breaking)
The source adoc at components/camel-ai/camel-ai-tool/src/main/docs/ai-tool-component.adoc has been updated with new sections (Tool Arguments in the Body, new headers, etc.), but the generated copy at catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/ai-tool-component.adoc is not included in this PR. The PrepareCatalogMojo regenerates this file during build, so CI's "Fail if there are uncommitted changes" step will detect the desync and fail. Previous commits in this area (e.g. cc762d8) always include both files together. Please run mvn process-resources -pl catalog/camel-catalog -am (or equivalent) and commit the regenerated file.
Addressed: the required-argument validation ordering concern from the previous review is resolved — bodyArguments.keySet().removeIf(camelReserved) now runs before the required-check, so a Camel-reserved name cannot satisfy a required parameter.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier reviews — all prior findings have been addressed.
Previous findings resolved:
- ✅ Required-argument ordering: Camel-prefix filter now runs before the required-argument check — a reserved-name arg can no longer pass the required check and then be silently discarded.
- ✅ Memory-less agent conversation ID:
hasMemory && memoryId != nullguard correctly suppressesCamelAiToolConversationIdfor agents without memory. The new test confirms memory-less agents propagate no conversation ID. - ✅ Body uses filtered arguments only: body is serialized from the post-filter
argsCopy— undeclared and reserved-name arguments are excluded from both headers and body. - ✅ Catalog adoc sync:
catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/ai-tool-component.adocis included in the PR. - ✅ OpenAI Responses API conversation ID:
OpenAIResponsesProducernow resolves and propagatesconversationIdthroughMcpToolCallExecutortoAiToolExecutor. - ✅
Locale.ROOTadded tolabel.toLowerCase()in the concurrency test.
The test suite is comprehensive: body serialization, header/body filtering, invocation identifier propagation, stale identifier cleanup, reserved-name injection prevention, and parallel concurrency. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Federico, the three blocking points are fixed, and so is the optional Responses API one. I resolved the threads, including the gnodet-bot ones that the new commits cover.
- Memory-less agents:
hasMemory && memoryId != nullomits the conversation header, and the concurrency test covers both the memory-less path and a memory agent that picks"default"on purpose. - The body is built from the filtered
argsCopy, so it has the same keys as the headers, and the docs and option description now say so. - The catalog copy of the doc is regenerated.
- The Responses API passes its resolved conversation id (header or option) through
McpToolCallExecutor, with tests for both the header and the option. - Reserved names are now removed before the required check, so a required
camel*argument rejects the call.
Two small things, neither blocking:
camel-4x-upgrade-guide-4_22.adocgets an entry under "Upgrading from 4.22.2 to 4.22.3". That only fits if the required-argument fix inAiToolExecutoris backported tocamel-4.22.x(the gap exists there too). CAMEL-25985 has no fix version yet. Please either plan that backport (only the filter ordering, not the new option and headers) or drop the 4.22 entry.agent instanceof AgentWithMemorymeans a customAgentimplementation that does use memory gets no conversation header. Omitting it is safe and matches "omitted rather than generated", so this is fine. The docs could say "anAgentWithMemory" instead of "when the agent uses memory".
Point 5 from my earlier review still applies: #28250 is still open, so whichever of the two merges second has to describe the JSON String body.
CI: Build and test is still queued.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 14 of 707 tested, 23 compile-only — current: 14 all testedMaveniverse Scalpel detected 14 affected modules (current approach: 14). Skip-tests mode would test 14 modules (8 direct + 10 downstream), skip tests for 23 (generated code, meta-modules) Modules Scalpel would test (14)
Modules with tests skipped (23)
All tested modules (41 modules, 8m 38s total)Total reactor time: 8m 38s
Top 20 slowest modules:
|
Description
Tool routes currently receive arguments only as individual headers, requiring users to reconstruct JSON for tools such as Kaoto Data Mapper. Add
argumentsAsBody(defaultfalse) to populate the body with the complete tool arguments as a JSON string, preserving nested objects, arrays and null values.The body and individual headers use the same declared-argument allowlist. Undeclared arguments are filtered out with a warning. Reserved argument names are excluded from both before required-argument validation, so a required reserved argument rejects the call before the route runs. Missing required arguments also reject the call.
Propagate
CamelAiToolCallIdandCamelAiToolConversationIdindependently of the body option wherever adapters supply them. LangChain4j supplies the call ID and propagates the memory ID only for agents with memory, including an intentional ID ofdefault. OpenAI supplies the call ID and, for the Responses API, the resolved conversation ID from configuration or headers. Spring AI supplies the conversation ID. Unavailable identifiers are omitted.Update the component documentation and regenerate its catalog copy, component metadata, configurers, and Java DSLs.
Target
main.Tracking
CAMEL-25985
Validation
Latest main validation: all 201 selected tests passed (145 AI Tool tests, 9 LangChain4j concurrency/filter tests, and 47 OpenAI executor/Responses tests). Coverage includes body allowlisting, required reserved arguments, memory-less agents, an intentional
defaultmemory ID, Responses configuration/header precedence, and parallel tool calls. The two Spring AI invocation tests passed earlier.The applicable changes were also tested on the local 4.22.x backport: 113 AI Tool tests and 9 LangChain4j tests passed. The newer Responses tool loop is not part of that branch.
Formatting and import sorting ran through the Maven lifecycle. Catalog generation and DSL generation succeeded for both branches; the catalog's AI Tool documentation matches its source exactly.
AI-assisted contributions
AI-assisted with Codex on behalf of @Croway.