fix: Robust multipart content handling in Gemini CLI provider#150
fix: Robust multipart content handling in Gemini CLI provider#150mirrobot-agent[bot] wants to merge 2 commits intodevfrom
Conversation
- Add _parse_content_parts() helper method to properly parse both string and list content formats - Apply content parsing to system messages to fix Invalid JSON payload error (issue #149) - Apply content parsing to user and assistant messages for consistency - Resolve 'can only concatenate str (not list) to str' error (issue #148) - Ensure compatibility with OpenAI API multipart message format The fix handles content in both formats: - Plain string: "Hello" - Multipart list: [{"type": "text", "text": "Hello"}, {"type": "image_url", ...}] This matches the implementation in antigravity_provider.py for consistency across providers.
|
Starting review of the multipart content handling fix for Gemini CLI provider. Let's dive into the refactoring. |
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This PR addresses issues #149 and #148 by extracting multipart content parsing into a reusable _parse_content_parts method. The refactoring eliminates code duplication and improves maintainability. The core approach is sound.
Key Suggestions
-
Edge Case Handling: The method can return an empty list if content is empty. Consider what downstream code expects and whether validation is needed.
-
Data URL Validation: Malformed data URLs could cause
IndexErrorwhen splitting. Add format validation before attempting to parse the URL components. -
Error Handling Strategy: Invalid images are silently dropped. Clarify whether this is acceptable or if the request should fail fast.
-
Consistency: The
if content:optimization is applied to assistant messages but not system/user messages. Apply consistently.
Questions for the Author
- Are there existing tests covering the multipart content parsing edge cases (empty content, malformed data URLs, non-data URLs)?
- Is silent failure for invalid images the intended behavior?
This review was generated by an AI assistant.
| def _parse_content_parts( | ||
| self, content: Any, model: str = "" | ||
| ) -> List[Dict[str, Any]]: | ||
| """ | ||
| Parse content into Gemini parts format. | ||
|
|
||
| Handles both string content and multipart list content (text, images, etc.). | ||
| This ensures compatibility with OpenAI API format where content can be: | ||
| - A plain string: "Hello" | ||
| - A list of parts: [{"type": "text", "text": "Hello"}, {"type": "image_url", ...}] | ||
|
|
||
| Args: | ||
| content: The message content (string or list) | ||
| model: The model name (for context if needed) | ||
|
|
||
| Returns: | ||
| List of Gemini parts dictionaries | ||
| """ | ||
| parts = [] | ||
|
|
||
| if isinstance(content, str): | ||
| if content: | ||
| parts.append({"text": content}) | ||
| elif isinstance(content, list): | ||
| for item in content: | ||
| if item.get("type") == "text": | ||
| text = item.get("text", "") | ||
| if text: | ||
| parts.append({"text": text}) | ||
| elif item.get("type") == "image_url": | ||
| image_url = item.get("image_url", {}).get("url", "") | ||
| if image_url.startswith("data:"): | ||
| try: | ||
| header, data = image_url.split(",", 1) | ||
| mime_type = header.split(":")[1].split(";")[0] | ||
| parts.append( | ||
| { | ||
| "inlineData": { | ||
| "mimeType": mime_type, | ||
| "data": data, | ||
| } | ||
| } | ||
| ) | ||
| except Exception as e: | ||
| lib_logger.warning(f"Failed to parse image data URL: {e}") | ||
| else: | ||
| lib_logger.warning( | ||
| f"Non-data-URL images not supported: {image_url[:50]}..." | ||
| ) | ||
|
|
||
| return parts |
There was a problem hiding this comment.
What happens if this returns an empty list? Consider validating that parts is non-empty before returning, or document the expected behavior when content is empty.
| parts.append({"text": text}) | ||
| elif item.get("type") == "image_url": | ||
| image_url = item.get("image_url", {}).get("url", "") | ||
| if image_url.startswith("data:"): |
There was a problem hiding this comment.
A malformed data URL (e.g., data:image/png without the comma) could cause an IndexError on line 660. Consider adding validation before splitting:
| if image_url.startswith("data:"): | |
| if "," not in image_url or ";" not in header: | |
| lib_logger.warning(f"Invalid data URL format: {image_url[:50]}...") | |
| continue |
| } | ||
| } | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
The exception is caught and logged, but processing continues. This means invalid images are silently dropped. Is this the intended behavior, or should the entire request fail when an image is malformed?
| elif role == "assistant": | ||
| if isinstance(content, str): | ||
| parts.append({"text": content}) | ||
| if content: |
There was a problem hiding this comment.
The if content: check is good here, but system messages (line 700) and user messages don't have this guard. Consider applying the same optimization consistently across all three call sites.
|
| Filename | Overview |
|---|---|
| src/rotator_library/providers/gemini_cli_provider.py | Adds _parse_content_parts helper and updates system/user/assistant message transformation to correctly handle both string and list content, fixing the Proto "list-in-text-field" and TypeError bugs. One minor unused model parameter in the new helper. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_transform_messages(messages, model)"] --> B{First message\nis system role?}
B -- Yes --> C["pop system message\nsystem_prompt_content = msg['content']"]
C --> D["_parse_content_parts(system_prompt_content)"]
D --> E{system_parts\nnon-empty?}
E -- Yes --> F["system_instruction =\n{role: 'user', parts: system_parts}"]
E -- No --> G["system_instruction = None"]
B -- No --> H
F --> H["Loop remaining messages"]
G --> H
H --> I{role?}
I -- user --> J["parts = _parse_content_parts(content)"]
I -- assistant --> K{content\ntruthy?}
K -- Yes --> L["content_parts = _parse_content_parts(content)\nparts.extend(content_parts)"]
K -- No --> M["process tool_calls only"]
I -- tool --> N["accumulate pending_tool_parts\n(functionResponse)"]
J --> O["append {role, parts} to gemini_contents"]
L --> O
M --> O
subgraph _parse_content_parts
P{isinstance\ncontent?} -- str --> Q{non-empty?}
Q -- Yes --> R["[{'text': content}]"]
Q -- No --> S["[]"]
P -- list --> T["for item in content"]
T --> U{isinstance\nitem dict?}
U -- No --> V["log warning, skip"]
U -- Yes --> W{item type?}
W -- text --> X["append {'text': item.text}"]
W -- image_url --> Y{data URL?}
Y -- Yes --> Z["append {'inlineData': ...}"]
Y -- No --> AA["log warning, skip"]
P -- other --> BB["[]"]
end
Last reviewed commit: cf332c0
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@mirrobot-agent On opencode it works flawlessly. But through RooCode I get a response like this: Roo said Roo said Model Response Incomplete API Request Roo has a question I'm ready to help! What would you like to do with the openlane-vin-check project?code |
|
@Dayclone, I'm investigating the issue you're experiencing with RooCode. The problem appears to be that the model is responding with text instead of using tools, which suggests a tool configuration issue rather than a content parsing issue. Let me analyze the differences between how Opencode and RooCode are sending requests to identify the root cause. |
|
@Dayclone, I've completed my investigation. Here's what I found: Root Cause AnalysisThe issue you're experiencing with RooCode is NOT caused by the multipart content parsing changes in this PR. The The problem - where the model responds with JSON text containing "follow_up" suggestions instead of using tools - is typically caused by:
Recommended Next StepsTo debug this issue, we need to:
Action RequiredCould you:
This will help us identify whether this is a RooCode configuration issue or if there's a subtle bug in how we're handling the tool configuration. |
Update: Defensive Coding Fix AppliedI've made a small improvement to the Change: Added validation to skip non-dict items in content lists, preventing potential This addresses one of the review comments and makes the code more robust, though it's not related to the RooCode issue you're experiencing. Next Steps for RooCode Issue: To help debug the RooCode issue, could you check:
The fact that it works in Opencode but not RooCode strongly suggests a client-side configuration difference rather than a proxy issue. |
- Skip non-dict items in multipart content lists to prevent AttributeError - Add warning log when skipping invalid items - Addresses review comment about unhandled edge cases This is a defensive improvement unrelated to the RooCode issue.
SummaryI've investigated the issue you reported with RooCode and made a defensive coding improvement to the PR. Investigation FindingsThe RooCode issue is NOT caused by the multipart content parsing changes in this PR. The problem you're experiencing - where the model responds with JSON text containing "follow_up" suggestions instead of using tools - is likely due to:
Changes MadeI've added a defensive coding improvement to handle edge cases in content parsing:
This addresses a review comment and makes the code more robust, though it's unrelated to the RooCode issue. Next StepsTo resolve the RooCode issue, we need to:
Could you enable debug logging on the proxy and share the request payloads from both clients? This will help us pinpoint the root cause. Commit: cf332c0 - fix: Add defensive check for non-dict items in content parsing |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.Add a Ruff configuration file to your project to customize how CodeRabbit runs |
|
@mirrobot-agent Honestly it works and that's all I care about, grateful for that. You can push the PR and commit? Not sure what next steps are needed. |
|
@Dayclone, Perfect! I'll push the latest commit with the defensive coding fix and update the PR status now. |
|
@Dayclone, All done! The PR is up to date with the latest commit: Summary✅ Latest commit pushed: What Was Done
PR Status
The PR is ready for merge whenever you're ready! This update was generated by an AI assistant. |
|
@Dayclone does it resolve your problems? keep in mind agent is forbidden from using anything external(no web access tools) and can only fetch from git. |
2 participants
Description
This PR fixes both issue #149 (Invalid JSON payload error) and issue #148 (string/list concatenation error) by implementing robust multipart content handling in the Gemini CLI provider.
Related Issues
Closes #149
Closes #148
Root Cause Analysis
Both issues were caused by improper handling of message content in
gemini_cli_provider.py:Issue #149: When clients send system messages with content as a list (multipart format like
[{"type": "text", "text": "You are helpful"}]), the code directly wrapped it without parsing, creating:{"role": "user", "parts": [{"text": [{"type": "text", "text": "You are helpful"}]}]}This caused the Gemini API to receive a list for the
textfield instead of a string, triggering the Protocol Buffers error: "Proto field is not repeating, cannot start list."Issue #148: The same root cause affected user and assistant messages, causing TypeError when attempting string concatenation with list content.
Changes Made
Added Helper Method
_parse_content_parts(content, model)(lines 627-677): New helper method that properly parses content into Gemini parts format"Hello"→[{"text": "Hello"}][{"type": "text", "text": "Hello"}]→[{"text": "Hello"}]Updated Message Transformation
_parse_content_parts()instead of direct wrapping_parse_content_parts()_parse_content_parts()for consistencyImplementation Details
The implementation mirrors the approach used in
antigravity_provider.py(lines 2653-2673) to ensure consistency across Gemini providers. This provides:Testing
The fix should be tested with:
{"role": "system", "content": "You are helpful"}{"role": "system", "content": [{"type": "text", "text": "You are helpful"}]}Additional Notes
This pull request was automatically generated by mirrobot-agent in response to @Mirrowel's request.