Multimodal content blocks - #1060
Conversation
…nditions, providers
|
Thanks for taking this on. Apologies for the delayed review due to our recent team outing. I will complete the review by the end of this week. |
wenjin272
left a comment
There was a problem hiding this comment.
Thanks for the work on this comprehensive multimodal message refactor! I left a few comments on the Event Log serialization contract, cross-language migration and invariants, routing isolation, and documentation updates.
| * 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 |
There was a problem hiding this 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.
| 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."); |
There was a problem hiding this 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?
| } | ||
| } | ||
| copy.add(new ChatMessage(m.getRole(), m.getContent(), toolCalls, m.getExtraArgs())); | ||
| // The full constructor copies the block list; blocks themselves are shared, matching |
There was a problem hiding this 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.
|
|
||
| @Nullable private String sha256; | ||
|
|
||
| protected MediaBlock() {} |
There was a problem hiding this 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?
| 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 |
There was a problem hiding this 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()?
| public abstract class MediaBlock extends ContentBlock { | ||
|
|
||
| @JsonProperty("mime_type") | ||
| private String mimeType; |
There was a problem hiding this comment.
Would mediaType be a better name than mimeType here? Media Type is the terminology used by the current standards, while MIME type is more of a commonly used historical name. Since this field is part of the public multimodal ContentBlock API rather than a MIME-specific protocol implementation, mediaType may better describe its semantics and also reads naturally alongside MediaBlock.
This is mostly an API naming suggestion, but since these block types are being introduced in this PR, it may be worth settling the terminology before the API becomes public.
Linked issue: #1059 (Phase 1 — the tracking issue stays open for the provider phase)
Purpose of change
Implements the framework part of the multimodal design agreed in #1031.
ChatMessage.content: Stringis replaced byblocks: List<ContentBlock>in both Java and Python. The initial block types areTextBlock,ImageBlock,AudioBlock,VideoBlock, andDocumentBlock. Media blocks share a MIME-typed shape withmime_type, exactly one of base64dataor externalurl, and optional metadata such asname,size_bytes, andsha256.Existing text constructors/factories keep their signatures and create a single
TextBlock.getText()/.textreturns the ordered text projection.Java and Python use the same
type-discriminated JSON format, covered by the regenerated cross-language snapshots. The pemja bridge is updated to carry blocks across runtimes as well.This PR also updates prompt handling so image-only messages are preserved, and
Prompt.formatMessagesapplies substitutions only to text blocks while passing media blocks through unchanged.Provider-specific multimodal conversion is intentionally left to the next #1059 phase. Existing providers continue using the text projection in this PR.
Behavior changes:
response.contentneed to migrate toresponse.blocks.ChatMessagenow rejects unknown fields, so the removedcontent=argument fails explicitly instead of silently creating an empty message.toString()/__str__, equality, and hashing now use the block representation.dataandurlare mutually exclusive for media blocks.Tests
ChatMessageserialization tests covering the wire format, mixed block ordering, media fields, round trips, source validation, and rejection of the removedcontentargument.ChatMessage.API
Yes. This is a Beta breaking change discussed in #1031.
ChatMessage.contentis replaced byblocksin both Java and Python. Java removesgetContent()/setContent; Python removes.content. Text-only access remains available throughgetText()/.textandsetText()/set_text().New public types in both languages are
ContentBlock,TextBlock,MediaBlock,ImageBlock,AudioBlock,VideoBlock, andDocumentBlock.ChatMessagealso addsgetBlocksAsMaps()/setBlocksFromMaps()for the pemja bridge, where blocks cross as plain maps.CEL expressions referencing
response.contentmust migrate toresponse.blocks.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated by: Claude Code Fable 5