joeyutong commented on code in PR #924:
URL: https://github.com/apache/flink-agents/pull/924#discussion_r3795858383
##########
plan/src/main/java/org/apache/flink/agents/plan/actions/ToolCallAction.java:
##########
@@ -72,64 +83,168 @@ public static void processToolRequest(Event event,
RunnerContext ctx) {
}
Tool tool = null;
- String diagnosticError = null;
+ Exception preparationError = null;
try {
tool = (Tool) ctx.getResource(name, ResourceType.TOOL);
} catch (Exception e) {
- diagnosticError = e.getMessage();
+ preparationError = e;
}
if (tool != null) {
try {
// Framework-owned injected args must win over
model-provided values so hidden
// context such as tenant ids cannot be spoofed by a tool
call payload.
mergedArguments.putAll(resolveInjectedArguments(tool,
ctx));
- ToolResponse response;
- final Tool toolRef = tool;
- final Map<String, Object> callArguments = mergedArguments;
- DurableCallable<ToolResponse> callable =
- new DurableCallable<>() {
- @Override
- public String getId() {
- return "tool-call";
- }
-
- @Override
- public Class<ToolResponse> getResultClass() {
- return ToolResponse.class;
- }
-
- @Override
- public ToolResponse call() throws Exception {
- return toolRef.call(new
ToolParameters(callArguments));
- }
- };
- response =
- toolCallAsync
- ? ctx.durableExecuteAsync(callable)
- : ctx.durableExecute(callable);
- success.put(id, response.isSuccess());
- responses.put(id, response);
- if (!response.isSuccess() && response.getError() != null) {
- error.put(id, response.getError());
- }
} catch (Exception e) {
- success.put(id, false);
- responses.put(
- id, ToolResponse.error(String.format("Tool %s
execute failed.", name)));
- error.put(id, e.getMessage());
+ preparationError = e;
+ }
+ }
+
+ ToolParameters metadataParameters = new
ToolParameters(mergedArguments);
+ Map<String, Object> entityMetadata =
+ toolEntityMetadata(
+ toolRequest.getId(),
+ id,
+ externalIds.get(id),
+ name,
+ tool,
+ metadataParameters);
+ ExecutionReporters.started(
+ ctx, ExecutionReporter.EntityTypes.TOOL, name,
entityMetadata);
+
+ if (tool == null || preparationError != null) {
+ Exception failure =
+ preparationError != null
+ ? preparationError
+ : new IllegalArgumentException("Tool does not
exist.");
+ success.put(id, false);
+ responses.put(
+ id,
+ ToolResponse.error(
+ String.format(
+ tool == null
+ ? "Tool %s does not exist."
+ : "Tool %s execute failed.",
+ name)));
+ String failureMessage = failure.getMessage();
+ if (failureMessage == null && tool == null) {
+ failureMessage = "Tool does not exist.";
}
- } else {
+ error.put(id, failureMessage);
+ ExecutionReporters.failed(
+ ctx,
+ ExecutionReporter.EntityTypes.TOOL,
+ name,
+ entityMetadata,
+ failure,
+ ExecutionReporter.ProblemCategories.TOOL_CALL_FAILED);
+ continue;
+ }
+
+ ToolResponse response = null;
+ try {
+ final Tool toolRef = tool;
+ final Map<String, Object> callArguments = mergedArguments;
+ DurableCallable<ToolResponse> callable =
+ new DurableCallable<>() {
+ @Override
+ public String getId() {
+ return "tool-call";
+ }
+
+ @Override
+ public Class<ToolResponse> getResultClass() {
+ return ToolResponse.class;
+ }
+
+ @Override
+ public ToolResponse call() throws Exception {
+ return toolRef.call(new
ToolParameters(callArguments));
+ }
+ };
+ response =
+ toolCallAsync
+ ? ctx.durableExecuteAsync(callable)
+ : ctx.durableExecute(callable);
+ success.put(id, response.isSuccess());
+ responses.put(id, response);
+ if (!response.isSuccess() && response.getError() != null) {
+ error.put(id, response.getError());
+ }
+ } catch (Exception e) {
Review Comment:
Agreed. Child-execution boundaries now report `failed` for `Error` and
rethrow it, so IGNORE/RETRY still only see `Exception`. An operator test covers
a Function Tool `NoClassDefFoundError` through `JavaFunction` unwrap and checks
the Tool `started`/`failed` pair.
##########
api/src/main/java/org/apache/flink/agents/api/Event.java:
##########
@@ -83,7 +83,7 @@ public Event(
if (type == null || type.isEmpty()) {
throw new IllegalArgumentException("Event 'type' must not be null
or empty.");
}
- this.id = id;
+ this.id = id != null ? id : UUID.randomUUID();
Review Comment:
Agreed. Omitted and explicit-null Event IDs both generate a per-occurrence
UUID. Python now matches Java; contract tests cover explicit null on both sides.
##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/LoadSkillTool.java:
##########
@@ -61,6 +65,22 @@ public ToolType getToolType() {
return ToolType.FUNCTION;
}
+ @Override
+ public Map<String, Object> getToolExecutionMetadata(ToolParameters
parameters) {
+ Map<String, Object> metadata = new LinkedHashMap<>();
+ if (parameters.hasParameter("name")) {
+ metadata.put(
+ ToolExecutionMetadataKeys.SKILL_NAME,
+ String.valueOf(parameters.getParameter("name")));
+ }
+ if (parameters.hasParameter("path")) {
Review Comment:
Agreed. `call()` and execution metadata now share missing/null/None →
`SKILL.md`, and that normalized path is what gets recorded. Tests cover omitted
and explicit null on both sides.
##########
python/flink_agents/plan/agent_plan.py:
##########
@@ -155,6 +158,7 @@ def from_agent(agent: Agent, config: AgentConfiguration) ->
"AgentPlan":
return AgentPlan(
actions=actions,
resource_providers=resource_providers,
+ agent_name=agent_name or agent.__class__.__name__,
Review Comment:
Agreed. Python now falls back only for `None`, matching Java. An explicit
empty string is preserved on both sides.
--
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]
