davsclaus commented on code in PR #27264:
URL: https://github.com/apache/camel/pull/27264#discussion_r4163912436
##########
components/camel-ai/camel-ai-tool/src/main/java/org/apache/camel/component/ai/tool/AiToolExecutor.java:
##########
@@ -175,4 +175,19 @@ private static AiToolResult buildSuccessResult(AiToolSpec
spec, Exchange exchang
String.format("Error executing tool '%s': %s",
spec.getName(), e.getMessage()), e);
}
}
+
+ /**
+ * Builds the exchange used to invoke a route tool, as an isolated copy of
the calling (agent) exchange. Context set
+ * before the agent ran - most importantly the authenticated caller's
identity kept as an exchange property -
+ * reaches the tool route, while header, body and exception changes on the
tool exchange do not leak back into the
+ * calling exchange. Every route-tool runtime (langchain4j-agent, openai,
spring-ai-chat) builds the tool exchange
+ * this way, so an authorization check on an exchange property behaves
identically across them (CAMEL-24832,
+ * CAMEL-23944).
+ *
+ * @param callingExchange the exchange driving the agent
+ * @return an isolated copy to pass to {@link
#execute(AiToolSpec, Map, Exchange)}
+ */
+ public static Exchange createToolExchange(Exchange callingExchange) {
+ return ExchangeHelper.createCopy(callingExchange, true);
Review Comment:
`ExchangeHelper` is not imported, so `camel-ai-tool` fails to compile
(`cannot find symbol: variable ExchangeHelper`). The import block needs:
```java
import org.apache.camel.support.ExchangeHelper;
```
Also, `createCopy` copies the caller's whole message (body + headers) along
with properties and variables. See point 2 in the review summary for why
clearing the copied message here may be preferable.
##########
components/camel-ai/camel-ai-tool/src/main/docs/ai-tool-component.adoc:
##########
@@ -353,6 +353,23 @@ YAML::
----
====
+== The calling exchange
+
+Each tool invocation runs on an isolated copy of the *calling exchange* — the
exchange that drives the agent
+(the `langchain4j-agent`, `openai` or `spring-ai-chat` producer). The context
the caller set before the agent ran
+is carried onto the tool route:
+
+* exchange *properties* — most importantly an authenticated caller's identity
kept as a property, so a tool route
+ can be guarded on it (for example `exchangeProperty.subject`) and the model
cannot forge it;
+* exchange *variables*.
+
+The tool's own *headers*, *body* and any *exception* are isolated to the copy
and do not leak back into the calling
+exchange. The tool arguments supplied by the model are placed on this copy
before the route runs.
Review Comment:
This reads as if the tool route starts without the caller's headers and
body, but with `exchange.copy()` it gets both: the caller's body (often the
user prompt) and every caller header, including inbound HTTP headers. Isolation
only holds in the other direction. Please either clear them in
`createToolExchange`, or say here exactly what the tool route receives.
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/McpToolCallExecutor.java:
##########
@@ -279,30 +282,29 @@ private ToolResult executeRouteTool(
try {
Map<String, Object> argsMap = OBJECT_MAPPER.readValue(argsJson,
Map.class);
- Exchange toolExchange = spec.getConsumer().createExchange(false);
- try {
- AiToolResult result = AiToolExecutor.execute(spec, argsMap,
toolExchange);
- if (result instanceof AiToolResult.Success success) {
- LOG.debug("Route tool '{}' result: {}", toolName,
success.value());
- return new ToolResult(
- toolCall.asFunction().id(), toolName,
success.value(),
- toolState.returnDirectTools().contains(toolName),
0, true);
- } else if (result instanceof AiToolResult.ArgumentError error)
{
- LOG.warn("Route tool '{}' argument error: {}", toolName,
error.message());
- return errorResult(toolCall, "Error: invalid tool
arguments: " + error.message());
- } else {
- AiToolResult.ExecutionError error =
(AiToolResult.ExecutionError) result;
- if (config.getToolExecutionErrorStrategy() ==
ToolExecutionErrorStrategy.FAIL_EXCHANGE) {
- if (error.cause() != null) {
- throw error.cause();
- }
- throw new IllegalStateException(error.message());
+ // isolated copy of the calling exchange so the caller's context
(e.g. an authenticated subject kept as an
+ // exchange property) reaches the tool route; this is not a pooled
consumer exchange, so it is not released
+ // here (CAMEL-24832)
+ Exchange toolExchange =
AiToolExecutor.createToolExchange(callingExchange);
Review Comment:
Before this change the tool exchange started empty. Now a route tool that
never sets a body returns the caller's body (the user prompt) to the model
instead of `No result`, and sees all caller headers. A test here that asserts
the tool route reads `${exchangeProperty.subject}` would also guard against
regressing back to a fresh exchange. Nothing tests this wiring today.
--
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]
