gnodet-bot commented on code in PR #28295:
URL: https://github.com/apache/camel/pull/28295#discussion_r4349315783
##########
components/camel-ai/camel-langchain4j-agent/src/main/java/org/apache/camel/component/langchain4j/agent/LangChain4jAgentProducer.java:
##########
@@ -450,7 +450,8 @@ private void addToolToResult(
// not leak into the calling producer exchange, while the caller's
context (properties/variables) reaches
// the tool route. Shared across the route-tool runtimes so they
cannot drift (CAMEL-24832, CAMEL-23944).
Exchange toolExchange =
AiToolExecutor.createToolExchange(exchange);
- AiToolResult result = AiToolExecutor.execute(spec, arguments,
toolExchange);
+ AiToolResult result = AiToolExecutor.execute(spec, arguments,
toolExchange,
+ toolExecutionRequest.id(), memoryId != null ?
memoryId.toString() : null);
Review Comment:
⚠️ **Semantic leak: `"default"` as conversation ID for memory-less agents**
When no `@MemoryId` annotation is present on the AI service interface (i.e.
`AgentWithoutMemory`), LangChain4j's `DefaultAiServices` does not pass `null` —
it resolves `findMemoryId(method, args).orElse(ChatMemoryService.DEFAULT)`
where `ChatMemoryService.DEFAULT = "default"`. As a result,
`memoryId.toString()` here returns `"default"`, and `CamelAiToolConversationId`
is set to `"default"` instead of being omitted.
Tools would see a spurious, misleading conversation ID they never requested.
Consider checking for the sentinel value:
```suggestion
toolExecutionRequest.id(), memoryId != null &&
!"default".equals(memoryId.toString()) ? memoryId.toString() : null);
```
A cleaner approach would be to check `memoryId` against
`ChatMemoryService.DEFAULT` explicitly, or to thread a boolean
`hasExplicitMemory` flag through from the service interface introspection.
##########
components/camel-ai/camel-langchain4j-agent/src/test/java/org/apache/camel/component/langchain4j/agent/LangChain4jAgentExecuteToolsConcurrentlyTest.java:
##########
@@ -141,6 +159,12 @@ void managedExecutorWorksAfterContextStopAndRestart()
throws Exception {
}
private void recordConcurrentEntry(String label, Exchange exchange) throws
InterruptedException {
+ String callId =
exchange.getMessage().getHeader(AiToolConstants.TOOL_CALL_ID, String.class);
+ assertThat(callId).isEqualTo(label.toLowerCase());
Review Comment:
💡 **Missing `Locale.ROOT`**
`toLowerCase()` without a locale is locale-sensitive and could behave
differently on Turkish/Azerbaijani JVMs (where `'I'` lowercases to `'ı'`). The
production code in this same PR already uses `name.toLowerCase(Locale.ROOT)`
for the Camel-reserved prefix check. Be consistent:
```suggestion
assertThat(callId).isEqualTo(label.toLowerCase(Locale.ROOT));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
