implement openaichat images for APIs that support them (using content parts) #2849
Conversation
WalkthroughRemoved automatic injection of a system prompt into initial messages. Introduced multimodal support: new ChatContentPart and ChatImageUrl types, added ContentParts to ChatRequestMessage, and implemented MarshalJSON/UnmarshalJSON to allow either a text Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/aiusechat/openaichat/openaichat-convertmessage.go`:
- Around line 238-307: convertAIMessageMultimodal can return a StoredChatMessage
with empty ContentParts when all parts are skipped; update
convertAIMessageMultimodal to detect when contentParts is empty after processing
(variable contentParts in function convertAIMessageMultimodal) and return a
non-nil error (or a clear sentinel) instead of returning a StoredChatMessage
with empty Message.ContentParts, so callers can avoid persisting/using empty
messages; ensure the error text clearly mentions "no content parts" and
references aiMsg.MessageId for debugging.
🧹 Nitpick comments (2)
pkg/aiusechat/openaichat/openaichat-types.go (2)
92-119: UnmarshalJSON: consider handling JSONnullcontent explicitly.When
contentis explicitlynullin JSON (not absent),len(raw.Content)is 4 (the bytesn,u,l,l), so the early return on line 103 won't fire. The code then falls into the array unmarshal path, which succeeds silently (settingpartstonil). While the end result is correct, the intent isn't obvious. A brief comment or an explicitnullcheck (e.g.,bytes.Equal(raw.Content, []byte("null"))) would make the behavior clearer.Suggested clarification
if len(raw.Content) == 0 { return nil } + // explicit null → no content + if string(raw.Content) == "null" { + return nil + } // try array first
261-264: Shallow copy ofContentPartsdoesn't deep-copy the*ChatImageUrlpointer field.
copy()on line 263 performs a shallow element copy. EachChatContentPartcontains a*ChatImageUrlpointer, so the original and the copied message will share the sameChatImageUrlinstances. This is inconsistent with theToolCallsdeep-copy pattern (lines 251-259) where theToolUseDatapointer is explicitly cloned. IfChatImageUrlis ever mutated afterCopy(), both the original and copy would be affected.Deep-copy ChatImageUrl to match ToolCalls pattern
if len(m.Message.ContentParts) > 0 { copied.Message.ContentParts = make([]ChatContentPart, len(m.Message.ContentParts)) - copy(copied.Message.ContentParts, m.Message.ContentParts) + for i, cp := range m.Message.ContentParts { + copied.Message.ContentParts[i] = cp + if cp.ImageUrl != nil { + imgCopy := *cp.ImageUrl + copied.Message.ContentParts[i].ImageUrl = &imgCopy + } + } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/aiusechat/openaichat/openaichat-types.go`:
- Around line 262-265: The current shallow copy of m.Message.ContentParts
(copying into copied.Message.ContentParts) leaves ChatContentPart.ImageUrl
pointers shared; update the clone so each ChatContentPart is deep-copied: after
allocating copied.Message.ContentParts and copying the slice, loop over each
index and if m.Message.ContentParts[i].ImageUrl != nil, allocate a new
ChatImageUrl, copy its fields (e.g., Detail, any other fields) into it and
assign to copied.Message.ContentParts[i].ImageUrl; mirror the pattern used in
the ToolCalls/ToolUseData deep-copy to ensure no shared pointers remain.
🧹 Nitpick comments (1)
pkg/aiusechat/openaichat/openaichat-types.go (1)
70-91: MarshalJSON: empty content omits the"content"key entirely.When both
ContentandContentPartsare empty/nil,raw.Contentremains a niljson.RawMessage, and theomitemptytag on the wire struct causes the"content"key to be dropped from the JSON output. Some OpenAI-compatible APIs expect"content": nullto be present (e.g., for assistant messages with tool calls). If this is intentional, a brief comment would help; otherwise, consider emittingnullexplicitly.
| if len(m.Message.ContentParts) > 0 { | ||
| copied.Message.ContentParts = make([]ChatContentPart, len(m.Message.ContentParts)) | ||
| copy(copied.Message.ContentParts, m.Message.ContentParts) | ||
| } |
There was a problem hiding this comment.
Shallow copy of ContentParts doesn't deep-copy the ImageUrl pointer.
copy() copies struct values, but ChatContentPart.ImageUrl is a *ChatImageUrl. After copy(...), the original and the clone share the same ChatImageUrl pointer. If the copy is later mutated (e.g., ImageUrl.Detail changed), it will corrupt the original.
Compare with the ToolCalls deep-copy on lines 252–260 which explicitly clones ToolUseData.
Proposed fix
if len(m.Message.ContentParts) > 0 {
copied.Message.ContentParts = make([]ChatContentPart, len(m.Message.ContentParts))
- copy(copied.Message.ContentParts, m.Message.ContentParts)
+ for i, cp := range m.Message.ContentParts {
+ copied.Message.ContentParts[i] = cp
+ if cp.ImageUrl != nil {
+ imgCopy := *cp.ImageUrl
+ copied.Message.ContentParts[i].ImageUrl = &imgCopy
+ }
+ }
}🤖 Prompt for AI Agents
In `@pkg/aiusechat/openaichat/openaichat-types.go` around lines 262 - 265, The
current shallow copy of m.Message.ContentParts (copying into
copied.Message.ContentParts) leaves ChatContentPart.ImageUrl pointers shared;
update the clone so each ChatContentPart is deep-copied: after allocating
copied.Message.ContentParts and copying the slice, loop over each index and if
m.Message.ContentParts[i].ImageUrl != nil, allocate a new ChatImageUrl, copy its
fields (e.g., Detail, any other fields) into it and assign to
copied.Message.ContentParts[i].ImageUrl; mirror the pattern used in the
ToolCalls/ToolUseData deep-copy to ensure no shared pointers remain.
1 participant
No description provided.