CAMEL-23943: Fix three defects in camel-langchain4j-tools producer - #24991
Conversation
1. Unbounded tool-calling loop: the do/while(true) loop in toolsChat had no iteration cap, so a misbehaving LLM could loop forever. Added maxToolCallingRoundTrips endpoint option (default 10) that throws RuntimeCamelException when exceeded. 2. Crash on hallucinated tool names: .findFirst().get() threw NoSuchElementException when the LLM requested a non-existent tool. Changed to .orElse(null) and sends an error message back to the LLM listing available tools, allowing graceful recovery. 3. Tool errors swallowed silently: the catch block set the exception on toolExchange but never propagated it to the LLM, and subsequent logic overwrote the exchange state. Now catches both thrown exceptions and processor-set exceptions, sends error details back to the LLM as a ToolExecutionResultMessage, and does not pollute the main exchange. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
- Regenerated catalog JSON and endpoint DSL factory for the new maxToolCallingRoundTrips option (fixes CI "uncommitted changes" check) - Improved defect 1 test to use a custom mock that always returns tool calls, creating a true infinite loop scenario. Added @timeout safety net so the test fails cleanly (timeout) if the fix is reverted, instead of hanging forever. - Verified all three tests fail before the fix and pass after: - Test 1: times out (infinite loop) → RuntimeCamelException (fix works) - Test 2: NoSuchElementException → graceful recovery - Test 3: exception on exchange → error sent to LLM Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 12 tested, 26 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Review Summary
Focused, well-crafted fix for three genuine defects in the LangChain4jToolsProducer.toolsChat() loop. Each fix is minimal, targeted, and backed by a dedicated test. The changes are production-ready.
What I Love
-
The
ExchangeHelper.copyResultsrelocation is subtle but important. Moving it inside the try block means a failed tool no longer pollutes the main exchange with a stale exception. In the original code,copyResultswas called unconditionally after the catch block, which copied the exception fromtoolExchangeto the mainexchange— yet the loop continued anyway, leaving the exchange in an inconsistent state. This fix resolves that silently. -
Sending errors back to the LLM as
ToolExecutionResultMessageis the right pattern. This gives the LLM a chance to self-correct (e.g., try a different tool or answer directly) rather than crashing the entire interaction. The hallucinated-tool error message even lists available tools, which is a nice touch for LLM self-correction. -
The test design using
OpenAIMockwith targeted scenarios is excellent. Each test reproduces exactly one defect, the@Timeout(10)on the infinite-loop test is a proper safety net, and the assertions verify both the positive (got a result) and negative (no exception) conditions.
Findings
Critical
None — nice work!
Important
None.
Suggestions & Nits
-
[Suggestion] Upgrade guide entry for
maxToolCallingRoundTripsdefault. The previous behavior was effectively unlimited round trips (thedo/while(true)ran forever). With the new default of 10, any existing setup where the LLM legitimately needs 11+ round trips before converging would now throw aRuntimeCamelException. While that scenario is unlikely — and was a latent bug anyway — an upgrade guide entry mentioning the new option and its default would help users who encounter the exception after upgrading. Something along the lines of:The
langchain4j-toolsproducer now limits tool-calling round trips to 10 by default (previously unlimited). If your LLM interaction legitimately requires more iterations, set themaxToolCallingRoundTripsendpoint option to a higher value. Set to 0 for unlimited (not recommended). -
[Nit]
e.getMessage()in the tool-error result could be null. If the caught exception has a null message (rare but possible, e.g.,new RuntimeException()), the tool result sent to the LLM would read"Error executing tool 'ToolName': null". A minor defensive check would make this slightly cleaner:String errorDetail = e.getMessage() != null ? e.getMessage() : e.getClass().getName(); toolResult = "Error executing tool '" + toolName + "': " + errorDetail;
This is purely cosmetic — the current behavior is functional.
Overall
This is a thorough, well-reasoned fix. The three defects were real, the fixes are clean, and the test coverage is solid. The interaction between all three fixes is sound: hallucinated-tool errors and tool-execution errors feed back into the LLM gracefully, and the max round trips cap provides a safety net across all scenarios. The check for toolExchange.getException() after process() is particularly important — without it, Camel's throwException DSL (which sets the exception on the exchange rather than throwing it) would be silently swallowed.
Note: This review would be an APPROVE, but GitHub does not allow approving your own PR. Requesting review from relevant committers is recommended.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
LGTM — all three fixes are correct, well-tested, and follow project conventions.
Summary
The PR fixes three genuine defects in LangChain4jToolsProducer:
- Unbounded tool-calling loop — bounded by new
maxToolCallingRoundTripsoption (default 10) - Crash on hallucinated tool names —
.findFirst().get()→.orElse(null)with graceful error back to LLM - Tool errors swallowed silently — errors now sent back to the LLM as
ToolExecutionResultMessage
Conventions
- ✅ Test class and methods are package-private
- ✅ AssertJ assertions used throughout
- ✅ No
Thread.sleep() - ✅ Generated files regenerated
- ✅
@Timeoutsafety net on infinite loop test
Suggestion
- Upgrade guide entry: Adding
maxToolCallingRoundTripswith default 10 changes behavior from unbounded to bounded loops. Existing routes exceeding 10 round trips would now throwRuntimeCamelException. Consider documenting this incamel-4x-upgrade-guide-4_22.adoc.
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.
Reviewed all three fixes. Since this producer sits on the prompt-injection path, I want to lead with the security check: the declaredParams allowlist is preserved verbatim and is not weakened by this PR.
I verified that mechanically rather than by eye. The allowlist derivation and its guard are outside every hunk in the diff — hunk 2 ends at old line 222 and hunk 3 starts at old line 261, so the whole block survives untouched. On the PR head the guard still reads if (!allowedParams.contains(name)) with the "Skipping undeclared tool argument" warning, and the single call that turns a model-supplied name into a header still sits inside it. Both new code paths — the hallucinated-tool branch and the error branch — set zero headers. Good.
The round-trip cap and the hallucinated-tool handling both look right to me, and the test class is well-formed (AssertJ, @Timeout(10), no Thread.sleep, package-private).
The one thing I'd like addressed: tool failures no longer reach the route
This is the change I think deserves more attention than the round-trip default that earlier reviews focused on.
ExchangeHelper.copyResults moved inside the try, so it now runs only on the success path. The consequence chain:
ExchangeHelper.doCopyResultsends withresult.setException(source.getException())(core/camel-support/src/main/java/org/apache/camel/support/ExchangeHelper.java:400).- Before,
copyResultswas unconditional, so a tool failure propagated onto the caller's exchange and the route failed /onExceptionfired. - After, on the error path the calling exchange is never touched. The producer returns normally, the route succeeds, and the failure is visible only as a string handed back to the LLM.
So a route author who wrapped langchain4j-tools: in onException(...) expecting to catch tool failures now gets silence.
I want to be fair about the starting point: the old behaviour was already unreliable, because a later successful tool's copyResults would call setException(null) and wipe the exception. So this isn't a clean regression — it replaces a flaky signal with no signal. And feeding the error back to the LLM so it can recover is a defensible design; it may well be what you intend.
But it is a user-visible behaviour change with no opt-out and no documentation, and I don't think users will discover it until a tool silently fails in production. At minimum it needs an upgrade-guide note; a failOnToolError-style switch would be better if you want to keep both audiences.
Missing upgrade-guide entry
Related: docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_22.adoc has no === camel-langchain4j-tools section at all. Two things here are user-visible — the round-trip cap going from unbounded to 10 and now throwing, and the error-propagation change above. For calibration, CAMEL-23382 added a 37-line entry to that same file for a comparable change in the adjacent component.
Smaller points
- Defect 2's fix closes the crash but the underlying feature stays dead. The
exposed=false+ tool-search combination cannot work by construction:CamelToolExecutorCachekeepstoolsandsearchableToolsin two disjoint maps,computeCandidatesbuildscallableToolsfrom the exposed map only, andToolSearchToolsearches the searchable map. So anything the search tool returns is guaranteed absent fromcallableTools. Previously that was aNoSuchElementException; now it's"Tool 'X' not found. Available tools: <exposed only>", which directly contradicts what the search tool just told the model — so the model will likely retry and burn round trips until the new cap throws. Better than crashing, but worth a follow-up ticket rather than leaving it looking fixed. - Negative values silently mean unlimited. The guard is
maxRoundTrips > 0while the description says "Set to 0 for unlimited", so-1also disables the cap with no warning. e.getMessage()can be null, giving"Error executing tool 'X': null"— you flagged this yourself in the self-review.- The option is endpoint-only;
LangChain4jToolsConfigurationholds justchatModel, so it can't be set viacamel.component.langchain4j-tools.*. The agent side routes the equivalent throughAgentConfiguration. Not necessarily wrong, just an asymmetry.
CI is green and the branch is clean. Nothing here blocks from my side except the doc gap, and I'd genuinely like your read on whether the error-swallowing is intended.
Reviewed with Claude Code (Opus 4.8) on behalf of Andrea Cosentino (@oscerd). This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
- Add upgrade guide entry documenting the three behavior changes: maxToolCallingRoundTrips default, hallucinated tool handling, and tool error propagation change - Fix null e.getMessage() in tool error result sent to LLM - Reject negative maxToolCallingRoundTrips values with IllegalArgumentException Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude Code on behalf of gnodet @oscerd Thank you for the thorough security-focused review. All actionable items addressed in c82475d: Addressed
Acknowledged (not in scope for this PR)
On the error propagation designThe previous behavior was indeed unreliable as you noted — a later successful tool's |
3 participants
Summary
Fixes three defects in the
camel-langchain4j-toolsproducer as reported in CAMEL-23943:Unbounded tool-calling loop — The
do/while(true)loop intoolsChat()has no iteration cap. If the LLM keeps requesting tools indefinitely, it never terminates. Fix: AddedmaxToolCallingRoundTripsendpoint option (default 10, producer-only) that bounds the loop and throwsRuntimeCamelExceptionwhen exceeded.Crash on hallucinated tool names — When the LLM hallucinates a tool name,
.findFirst().get()throwsNoSuchElementException. Fix: Changed to.orElse(null)with a graceful error message sent back to the LLM listing available tools.Tool errors swallowed silently — When a tool route throws an exception, the error is set on
toolExchangebut never propagated to the LLM; subsequentExchangeHelper.copyResultsoverwrites exchange state. Fix: Errors are now sent back to the LLM asToolExecutionResultMessage, movedcopyResultsinside the try block.Changes
LangChain4jToolsProducer.java— All three fixes intoolsChat()/invokeTools(), with null-safee.getMessage()handlingLangChain4jToolsEndpoint.java— NewmaxToolCallingRoundTrips@UriParam(producer-only, default 10), with validation rejecting negative valuesLangChain4jToolDefectsTest.java— Three targeted reproducer tests (one per defect), verified to fail before fix and pass aftercamel-4x-upgrade-guide-4_22.adoc— Documents all three behavior changesTest plan
Review feedback addressed
e.getMessage()null check — uses class name as fallback (flagged by self-review, @oscerd)maxToolCallingRoundTripswithIllegalArgumentException(flagged by @oscerd)exposed=false+ToolSearchTooldead path as pre-existing issue (flagged by @oscerd) — will create JIRA follow-up🤖 Generated with Claude Code on behalf of gnodet