-
Notifications
You must be signed in to change notification settings - Fork 0
Fix ZAI fallback message content normalization #643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
None
input (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
text
attribute but nomodel_dump
methodExample 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.py
tests/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 (
json
andSequence
) 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
getattr
usage is defensive coding that guards against edge cases wherehasattr
returns 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.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68ec2699ab208333aa457b9299c22d89
Summary by CodeRabbit
New Features
Bug Fixes
Tests