wenjin272 commented on code in PR #924:
URL: https://github.com/apache/flink-agents/pull/924#discussion_r3780670997
##########
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:
Could we align the explicit-null Event ID contract between Java and Python?
With this change, `Event.fromJson("{\"id\":null,\"type\":\"x\"}")` succeeds
in Java and silently assigns a new UUID. The equivalent Python `Event(id=None,
type="x")` fails validation because its UUID default is applied only when the
field is absent.
The repository guidelines require equivalent Java/Python public APIs to have
aligned null/None semantics. Could we choose one contract—either explicit null
means “generate an ID” or explicit null is invalid—and add matching contract
tests 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:
Could the execution metadata use the same default-path normalization as
`call()`?
When `path` is missing or explicitly null, `call()` loads the default
`SKILL.md`. This metadata path instead converts an explicit null with
`String.valueOf`, recording `"skillResourcePath": "null"`; Python similarly
records `"None"`. The Tool succeeds, but its Trace describes a resource that
was not actually loaded, and the Java/Python records also differ.
Could both `call()` and `getToolExecutionMetadata()` share the rule
`missing/null/None -> "SKILL.md"` and record that normalized path? Tests for
both an omitted path and an explicit null/None would keep execution and
observability aligned.
##########
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:
Could we avoid using truthiness to select the default agent name?
Here an explicitly supplied empty string falls back to the Python class
name, while the corresponding Java constructor falls back only for null and
preserves an empty string. The same public input can therefore produce
different `agentName` values in the serialized plan and Event Log.
Could we either use `agent_name if agent_name is not None else
agent.__class__.__name__` to match Java, or reject blank agent names
consistently on both sides?
##########
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:
Following up on [the earlier lifecycle
thread](https://github.com/apache/flink-agents/pull/924#discussion_r3697188385):
the Action-level path now catches `Throwable`, but this child-execution
boundary still catches only `Exception`.
`JavaFunction.call()` now unwraps an `InvocationTargetException` and
rethrows its target `Error` directly. Consequently, a Function Tool throwing
`AssertionError` or `NoClassDefFoundError` skips this catch: the Tool execution
has already emitted `started`, but never emits a terminal `failed` event. The
parent Action subsequently fails and clears the active child-execution map,
leaving the Tool execution permanently incomplete.
The LLM and parser boundaries in `ChatModelAction` have the same `catch
(Exception)` shape. Could all three boundaries catch `Throwable` long enough to
report `failed`, then preserve the original failure when rethrowing? An
operator-level regression test using a `LinkageError` would cover the complete
path.
--
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]
