Skip to content

Conversation

matdev83
Copy link
Owner

@matdev83 matdev83 commented Oct 12, 2025

Summary

  • add a helper in the chat controller to normalize multi-part message content before creating Anthropic fallbacks for ZAI requests
  • update the ZAI fallback to use the helper so structured messages keep their text content
  • add unit coverage for the new normalization helper to prevent regressions

Testing

  • python -m pytest --override-ini addopts='' tests/unit/core/app/controllers/test_chat_controller_content.py
  • python -m pytest --override-ini addopts='' # fails: missing optional test dependencies such as pytest_asyncio/respx/pytest_httpx

https://chatgpt.com/codex/tasks/task_e_68ec2699ab208333aa457b9299c22d89

Summary by CodeRabbit

  • New Features

    • Improved chat message handling to seamlessly interpret various content formats (plain text, byte data, structured parts, and sequences), producing consistent plain-text responses.
  • Bug Fixes

    • Reduced errors when sending mixed or non-string message content, improving reliability and response quality in chat.
  • Tests

    • Added comprehensive tests to validate content coercion across multiple formats and edge cases.

@coderabbitai
Copy link

coderabbitai bot commented Oct 12, 2025

Walkthrough

Introduces 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

Cohort / File(s) Summary
Chat content coercion
src/core/app/controllers/chat_controller.py
Added _coerce_message_content_to_text to normalize content (None, str, bytes, dicts, sequences, model objects) into text; integrated into handle_chat_completion for Anthropic message construction; minor structural/docstring updates.
Unit tests
tests/unit/core/app/controllers/test_chat_controller_content.py
New tests covering sequence flattening, bytes decoding, and domain model text extraction behavior of _coerce_message_content_to_text.

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()
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibble on bytes, then strings I inspect,
From parts into paragraphs, neatly correct.
I hop through dicts, models, and queues,
Coaxing out text from binary hues.
Tests burrow deep—content now tight—
Carrots up! The chats read right. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly states that the pull request fixes the normalization of fallback message content for ZAI, which directly reflects the core change of adding a helper method and updating the fallback logic; it is concise, specific, and accurately summarizes the main update.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/fix-identified-frontend-bug-qn6wal

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 no model_dump method

Example 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

📥 Commits

Reviewing files that changed from the base of the PR and between dd216cf and 0b36685.

📒 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 and Sequence) 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 where hasattr 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant