sangkyoonnam commented on code in PR #1185:
URL: https://github.com/apache/flink-agents/pull/1185#discussion_r4237896578
##########
api/src/main/java/org/apache/flink/agents/api/tools/ToolResponse.java:
##########
@@ -42,52 +59,59 @@ public class ToolResponse {
@JsonCreator
private ToolResponse(
- @JsonProperty("result") Object result,
+ @JsonProperty("blocks") List<? extends DataContentBlock> blocks,
+ @JsonProperty("metadata") Map<String, Object> metadata,
@JsonProperty("success") boolean success,
@JsonProperty("error") String error,
@JsonProperty("execution_time_ms") long executionTimeMs,
@JsonProperty("tool_name") String toolName) {
- this.result = result;
+ if (success != (error == null)) {
+ throw new IllegalArgumentException("ToolResponse success and error
disagree");
+ }
+ // Kryo restores collections by adding elements to the backing list.
+ this.blocks = blocks == null ? new ArrayList<>() : new
ArrayList<>(List.copyOf(blocks));
+ this.metadata = metadata == null ? Map.of() : metadata;
Review Comment:
On joeyutong's `Map.of()` point: this default hits every `ToolResponse`
built without metadata. Outside Flink on JDK 17, using plain Kryo 2.24.0 and
the `StdInstantiatorStrategy` fallback, the empty `Map.of()` comes back as a
broken `MapN` (its `toString()` throws an NPE), a one-entry `Map.of("id", "x")`
fails on read with `UnsupportedOperationException`, and a `HashMap`
round-trips. `ToolResultBlock` already defaults to `new HashMap<>()`:
```suggestion
this.metadata = metadata == null ? new HashMap<>() : metadata;
```
This also needs `import java.util.HashMap;`. With that change, the api suite
(609 tests, 13 skipped) and the focused plan and runtime suites (80 and 73)
pass here on JDK 21 and Flink 2.3.0. Caller-supplied maps are still stored as
given, so `Map.of(...)` metadata, including an explicitly passed empty map,
stays incompatible with Flink 1.20's default Kryo serializer. The two Java
metadata examples in `tool_use.md` use `Map.of(...)`; switching them to a
`HashMap`, with a note, would steer users away from it.
--
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]
