gnodet commented on code in PR #25289:
URL: https://github.com/apache/camel/pull/25289#discussion_r3708109284
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIProducer.java:
##########
@@ -633,6 +671,89 @@ private void processNonStreamingAgentic(
"Max tool iterations (%d) exceeded. Tools called:
%s".formatted(maxIterations, toolCallsLog));
Review Comment:
**Code duplication:** `discoverCamelRouteTools()`,
`executeCamelRouteTool()`, and the result-handling logic are duplicated nearly
verbatim between `OpenAIProducer` and `OpenAIToolExecutionProducer` (~80 lines
each). The duplication is slightly inconsistent — this class extracts result
handling into `handleCamelToolResult()`, while `OpenAIToolExecutionProducer`
inlines the same logic.
Consider extracting the shared code into a package-private helper class
(similar to how `AiToolSpecToOpenAI` is already a shared utility).
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIProducer.java:
##########
@@ -633,6 +671,89 @@ private void processNonStreamingAgentic(
"Max tool iterations (%d) exceeded. Tools called:
%s".formatted(maxIterations, toolCallsLog));
}
+ /**
+ * Executes a Camel route tool via {@link AiToolExecutor}, handling the
exchange lifecycle and error strategies.
+ */
+ private String executeCamelRouteTool(
+ AiToolSpec spec, String argsJson, Exchange exchange,
OpenAIConfiguration config)
+ throws Exception {
+ LOG.debug("Executing Camel route tool '{}' with args: {}",
spec.getName(), argsJson);
+
+ Map<String, Object> argsMap;
+ try {
+ if (argsJson == null || argsJson.trim().isEmpty()) {
+ argsMap = Map.of();
+ } else {
+ argsMap = OBJECT_MAPPER.readValue(argsJson, Map.class);
+ }
+ } catch (JsonProcessingException e) {
+ if (config.getToolExecutionErrorStrategy() ==
ToolExecutionErrorStrategy.FAIL_EXCHANGE) {
+ throw e;
+ }
+ LOG.warn("Invalid tool arguments for Camel route tool '{}': {}",
spec.getName(), argsJson, e);
+ return "Error: invalid tool arguments: " + e.getMessage();
+ }
+
+ // Isolate tool execution in its own exchange copy
+ Exchange toolExchange = ExchangeHelper.createCopy(exchange, true);
+ try {
+ AiToolResult result = AiToolExecutor.execute(spec, argsMap,
toolExchange);
+ return handleCamelToolResult(spec.getName(), result, config);
+ } catch (Exception e) {
+ if (config.getToolExecutionErrorStrategy() ==
ToolExecutionErrorStrategy.FAIL_EXCHANGE) {
+ throw e;
+ }
+ LOG.warn("Camel route tool '{}' execution failed: {}",
spec.getName(), e.getMessage(), e);
+ return "Error: Tool execution failed: " + e.getMessage();
Review Comment:
**Security nit:** The `AiToolResult` Javadoc warns: "Framework adapters MUST
NOT return [ExecutionError.message()] verbatim to the LLM without
sanitization." The `handleCamelToolResult()` method correctly returns the
generic `"Error: Tool execution failed"` for `ExecutionError`, but this outer
catch returns `e.getMessage()` which could include internal details. Consider
using the same sanitized message:
```suggestion
return "Error: Tool execution failed";
```
The same pattern applies to `OpenAIToolExecutionProducer` at line 291.
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIProducer.java:
##########
@@ -474,12 +496,17 @@ private void processNonStreamingSimple(
}
private void processNonStreamingAgentic(
- Exchange exchange, ChatCompletionCreateParams params,
OpenAIConfiguration config)
+ Exchange exchange, ChatCompletionCreateParams params,
OpenAIConfiguration config,
+ Map<String, AiToolSpec> camelRouteTools)
throws Exception {
int maxIterations = config.getMaxToolIterations();
+
+ Set<String> availableToolNames = new java.util.LinkedHashSet<>();
Review Comment:
**FQCN violation:** `new java.util.LinkedHashSet<>()` should use the simple
class name with an import. The project convention states: "Do NOT use fully
qualified class names in Java code." Note that `OpenAIToolExecutionProducer` in
this same PR correctly imports `java.util.LinkedHashSet`.
```suggestion
Set<String> availableToolNames = new LinkedHashSet<>();
```
(Also add `import java.util.LinkedHashSet;` alongside the existing `import
java.util.LinkedHashMap;`)
--
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]
