wenjin272 commented on code in PR #1060:
URL: https://github.com/apache/flink-agents/pull/1060#discussion_r3975339527
##########
api/src/main/java/org/apache/flink/agents/api/chat/model/routing/RoutingContext.java:
##########
@@ -97,7 +97,9 @@ private static List<ChatMessage> deepCopy(List<ChatMessage>
messages) {
toolCalls.add(call == null ? null : new HashMap<>(call));
}
}
- copy.add(new ChatMessage(m.getRole(), m.getContent(), toolCalls,
m.getExtraArgs()));
+ // The full constructor copies the block list; blocks themselves
are shared, matching
Review Comment:
This copies the block list but still shares every mutable `ContentBlock`
instance with the original request. For example, a strategy can obtain the
first `TextBlock` from `context.getMessages()`, call `setText("mutated")`, and
the original message later sent to the model will also return `"mutated"`. The
same aliasing exists for `MediaBlock.setData()` and `setUrl()`. This does not
require a hostile strategy; it is exactly the kind of accidental mutation that
the defensive copy is intended to isolate, and it is newly exposed because the
previous text content was an immutable `String`.
Rather than adding a type-specific deep-copy branch whenever a new block
type is introduced, could we make the concrete `ContentBlock` implementations
immutable—`final` fields, no setters, and validated `@JsonCreator`
constructors—and snapshot the block list in `ChatMessage`? `RoutingContext`
could then safely share block instances while continuing to copy the mutable
message/list structure. The routing isolation tests should also cover
block-level aliasing rather than only replacing the copied message's text/list.
##########
api/src/main/java/org/apache/flink/agents/api/chat/messages/MediaBlock.java:
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.agents.api.chat.messages;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+import javax.annotation.Nullable;
+
+import java.util.Objects;
+
+/**
+ * Shared shape for binary media blocks: modality is the concrete type,
encoding is the MIME type.
+ *
+ * <p>The payload is carried by exactly one of base64 {@code data} or an
externally managed {@code
+ * url} (enforced by the argument constructor and the per-type factories; the
no-arg bean path is
+ * lenient for deserialization). URL-backed content is externally managed:
URLs may expire, may not
+ * be reachable by the model provider, and may be invalid after recovery from
a checkpoint. The
+ * optional {@code name}/{@code sizeBytes}/{@code sha256} metadata also serves
the Event Log, which
Review Comment:
We need an explicit serialization contract for Event Log output here. The
regular `ChatMessage` JSON representation is also used by the Java/Python
bridge, event serialization, and state recovery, where `data` and the original
`url` must be preserved. Therefore, changing the global `ChatMessage`
serialization to remove these fields would break the normal wire format.
Reusing that same representation for Event Log output, however, cannot
guarantee metadata-only and sanitized logging.
Could each `ContentBlock` define its log-safe representation—for example
through `sanitize()`—and could the Event Log serialization module register a
dedicated `ChatMessage` serializer that uses it? A media block should
explicitly whitelist its loggable metadata, omit inline `data`, and sanitize
credentials from URLs. This keeps `EventLogRecordJsonSerializer` generic,
avoids hard-coding every block subtype into it, and lets future block types
define their own logging policy. Tests should verify both that normal JSON
serialization still preserves the complete payload and that Event Log
serialization never exposes it at either STANDARD or VERBOSE level.
##########
docs/content/docs/development/chat_models.md:
##########
@@ -97,7 +97,7 @@ class MyAgent(Agent):
@staticmethod
def process_response(event: Event, ctx: RunnerContext) -> None:
chat_response = ChatResponseEvent.from_event(event)
- response_content = chat_response.response.content
+ response_content = chat_response.response.text
Review Comment:
This response access was migrated, but the same documentation still contains
`ChatMessage(..., content=...)` examples and Java calls to the removed
`getContent()` method. The Python examples now fail because unknown fields are
forbidden, while the Java examples no longer compile. Similar remnants remain
in `prompts.md`, `react_agent.md`, `workflow_agent.md`, and the quickstarts.
Could we complete a repository-wide documentation migration to `blocks`/the
message factories and `text`/`getText()`?
##########
api/src/main/java/org/apache/flink/agents/api/chat/messages/MediaBlock.java:
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.agents.api.chat.messages;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+import javax.annotation.Nullable;
+
+import java.util.Objects;
+
+/**
+ * Shared shape for binary media blocks: modality is the concrete type,
encoding is the MIME type.
+ *
+ * <p>The payload is carried by exactly one of base64 {@code data} or an
externally managed {@code
+ * url} (enforced by the argument constructor and the per-type factories; the
no-arg bean path is
+ * lenient for deserialization). URL-backed content is externally managed:
URLs may expire, may not
+ * be reachable by the model provider, and may be invalid after recovery from
a checkpoint. The
+ * optional {@code name}/{@code sizeBytes}/{@code sha256} metadata also serves
the Event Log, which
+ * records media metadata instead of payload bytes.
+ */
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public abstract class MediaBlock extends ContentBlock {
+
+ @JsonProperty("mime_type")
+ private String mimeType;
+
+ @Nullable private String data;
+
+ @Nullable private String url;
+
+ @Nullable private String name;
+
+ @JsonProperty("size_bytes")
+ @Nullable
+ private Long sizeBytes;
+
+ @Nullable private String sha256;
+
+ protected MediaBlock() {}
Review Comment:
The constructor enforces exactly one of `data` and `url`, but the no-arg
Jackson path and the independent setters bypass that check. Java can therefore
accept a wire payload containing both or neither source while Python rejects
the same payload. Could we use validated immutable `@JsonCreator`
construction—or represent the source as `Base64Source | URLSource`—and freeze
or validate assignment on the Python side as well?
##########
plan/src/test/java/org/apache/flink/agents/plan/resource/python/PythonPromptTest.java:
##########
@@ -75,9 +75,9 @@ public void testFromSerializedMapWithMessageListTemplate() {
prompt.formatMessages(MessageRole.SYSTEM, new HashMap<>());
assertThat(formattedMessages).hasSize(2);
assertThat(formattedMessages.get(0).getRole()).isEqualTo(MessageRole.SYSTEM);
- assertThat(formattedMessages.get(0).getContent()).isEqualTo("You are a
helpful assistant.");
+ assertThat(formattedMessages.get(0).getText()).isEqualTo("You are a
helpful assistant.");
Review Comment:
This test still constructs the serialized Python messages with the old
`content` field, while the production Python resource now serializes them as
`blocks`. `PythonPrompt.parseChatMessage()` still reads `content` and defaults
it to an empty string, so a real message-based Python prompt loses all of its
blocks when restored in Java. Could we parse `blocks` through the existing
map/Jackson conversion and update this fixture to use the actual Python
`model_dump()` shape, including text and media blocks?
--
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]
