wenjin272 commented on code in PR #1185:
URL: https://github.com/apache/flink-agents/pull/1185#discussion_r4244152310
##########
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:
Thanks! `ToolResponse` now uses a `HashMap` for metadata, including a copy
of caller-supplied maps. Added a Kryo round-trip test, verified on Flink 1.20.5
and 2.3.0.
##########
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:
Thanks for verifying! The fix also copies explicitly supplied maps into a
`HashMap`, covering empty and non-empty `Map.of(...)`. The documentation
examples remain valid without a compatibility caveat.
--
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]
