Fix ZAI fallback message content normalization #643
Conversation
WalkthroughIntroduces ChatController._coerce_message_content_to_text to normalize diverse message content types into text and updates handle_chat_completion to use it when preparing Anthropic messages. Adds unit tests validating sequence flattening, bytes decoding, and domain model text extraction. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client/UI
participant CC as ChatController
participant Coerce as _coerce_message_content_to_text
participant Anth as Anthropic API
UI->>CC: handle_chat_completion(messages)
loop For each message.content
CC->>Coerce: Normalize content to text
Coerce-->>CC: text
end
CC->>Anth: Create completion request with coerced texts
Anth-->>CC: Completion response
CC-->>UI: Response
note over Coerce,CC: Handles None, str, bytes, dicts, sequences, models, fallback to str()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
src/core/app/controllers/chat_controller.py (1)
109-163: Consider adding recursion depth limit.The method correctly handles various content types and provides good defensive coding. However, recursive calls in lines 128 and 151 could theoretically lead to stack overflow if there are deeply nested or circular data structures.
Consider adding a depth parameter to prevent unbounded recursion:
@staticmethod -def _coerce_message_content_to_text(content: Any) -> str: +def _coerce_message_content_to_text(content: Any, _depth: int = 0) -> str: """Flatten ChatMessage content into a plain text payload for Anthropic.""" + + if _depth > 20: # Prevent stack overflow from circular references + return str(content) if content is None: return "" if isinstance(content, str): return content if isinstance(content, bytes | bytearray): return content.decode("utf-8", errors="ignore") if hasattr(content, "model_dump"): try: dumped = content.model_dump() except Exception: # pragma: no cover - defensive dumped = None if dumped is not None: - return ChatController._coerce_message_content_to_text(dumped) + return ChatController._coerce_message_content_to_text(dumped, _depth + 1) if isinstance(content, dict): text_value = content.get("text") if isinstance(text_value, str): return text_value if isinstance(text_value, (bytes, bytearray)): return text_value.decode("utf-8", errors="ignore") if content.get("type") == "image_url": image_payload = content.get("image_url") if isinstance(image_payload, dict): url_value = image_payload.get("url") if isinstance(url_value, str): return url_value return json.dumps(content, ensure_ascii=False) if isinstance(content, Sequence) and not isinstance( content, (str, bytes, bytearray) ): parts: list[str] = [] for part in content: - text_part = ChatController._coerce_message_content_to_text(part) + text_part = ChatController._coerce_message_content_to_text(part, _depth + 1) if text_part: parts.append(text_part) return "\n\n".join(parts) if hasattr(content, "text"): text_attr = getattr(content, "text") if isinstance(text_attr, str): return text_attr if isinstance(text_attr, (bytes, bytearray)): return text_attr.decode("utf-8", errors="ignore") return str(content)tests/unit/core/app/controllers/test_chat_controller_content.py (1)
1-36: Expand test coverage for edge cases.The current tests cover the main happy paths well. Consider adding tests for:
Noneinput (should return empty string)- Empty sequence (should return empty string)
- Dict with
type: "image_url"(should extract URL)- Fallback to
str()for unknown object types- Objects with
textattribute but nomodel_dumpmethodExample additional tests:
def test_coerce_message_content_to_text_handles_none() -> None: """None input should return empty string.""" result = ChatController._coerce_message_content_to_text(None) assert result == "" def test_coerce_message_content_to_text_handles_empty_sequence() -> None: """Empty sequences should return empty string.""" result = ChatController._coerce_message_content_to_text([]) assert result == "" def test_coerce_message_content_to_text_extracts_image_url() -> None: """Image URL content should extract the URL string.""" content = { "type": "image_url", "image_url": {"url": "https://example.com/image.png"} } result = ChatController._coerce_message_content_to_text(content) assert result == "https://example.com/image.png" def test_coerce_message_content_to_text_handles_object_with_text_attr() -> None: """Objects with text attribute should return the text value.""" class CustomObject: text = "custom content" result = ChatController._coerce_message_content_to_text(CustomObject()) assert result == "custom content"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/app/controllers/chat_controller.py(3 hunks)tests/unit/core/app/controllers/test_chat_controller_content.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/core/app/controllers/chat_controller.pytests/unit/core/app/controllers/test_chat_controller_content.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/core/app/controllers/chat_controller.py
🧬 Code graph analysis (2)
src/core/app/controllers/chat_controller.py (5)
tests/unit/connectors/test_precision_payload_mapping.py (1)
json(124-129)tests/unit/openai_connector_tests/test_streaming_response.py (1)
content(57-58)src/core/config/app_config.py (3)
model_dump(680-689)get(608-610)get(1640-1654)src/core/domain/responses_api.py (3)
model_dump(52-55)model_dump(86-89)model_dump(149-152)src/core/domain/configuration/backend_config.py (1)
model_dump(59-72)
tests/unit/core/app/controllers/test_chat_controller_content.py (2)
src/core/app/controllers/chat_controller.py (2)
ChatController(36-629)_coerce_message_content_to_text(110-163)src/core/domain/chat.py (1)
MessageContentPartText(14-18)
🪛 Ruff (0.13.3)
src/core/app/controllers/chat_controller.py
125-125: Do not catch blind exception: Exception
(BLE001)
157-157: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.10)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
src/core/app/controllers/chat_controller.py (4)
1-6: LGTM!The new imports (
jsonandSequence) are correctly added to support the message content normalization functionality.
125-125: Static analysis hint can be safely ignored.The broad exception catch is intentional defensive code to prevent
model_dump()failures from propagating. The pragma comment correctly documents this.As per coding guidelines
157-157: Static analysis hint can be safely ignored.The
getattrusage is defensive coding that guards against edge cases wherehasattrreturns True but attribute access might still fail (e.g., properties with side effects or descriptors).
217-219: LGTM!The integration correctly uses the new helper to normalize message content before creating Anthropic fallbacks, addressing the PR objective.
|
PR #643 - Fix ZAI fallback message content normalization - ALREADY IMPLEMENTED Analysis SummaryThis PR addressed the issue of normalizing multi-part message content when creating Anthropic fallbacks for ZAI requests. Current Status: ALREADY IMPLEMENTEDAfter thorough analysis of the current codebase, all functionality described in this PR is already implemented and working correctly: ✅ Implementation Already Present
✅ Comprehensive Test CoverageThe est_chat_controller_content.py file contains 17 comprehensive tests covering:
✅ Code Quality
✅ Test ResultsAll related tests pass successfully:
Reviewer Comments Addressed✅ Recursion Depth Limit: Already implemented with _depth parameter and 20-level limit ConclusionThe functionality described in PR #643 has already been implemented and enhanced in the current codebase. The project now properly normalizes diverse message content types for ZAI fallback to Anthropic API, with comprehensive test coverage and robust error handling. No further action is needed as this improvement is already available and working correctly in the main development branch. |
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ec2699ab208333aa457b9299c22d89
Summary by CodeRabbit
New Features
Bug Fixes
Tests