fix(openai-compat): stop emitting [non_text_content] for non-text parts (#4644) - #4680
Conversation
github-actions
Bot
added
size: M
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
3691655 to
919ec99
Compare
383f72d to
aecd2ad
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new ChangesContent-part normalization extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
919ec99 to
10a6165
Compare
…ts (#4644) The Chat Completions and Responses inbound paths both collapsed any non-text content part (image_url / input_audio / file) to the opaque literal "[non_text_content]", which then reached the model as user text - the canary the issue calls out. They were also byte-identical duplicate parsers (the duplicate-pipeline smell in architecture.md). - New shared crate-private module content_parts owns the one copy of sanitize_product_text_fragment, content_array_item_text, and a new non_text_part_marker. Both chat_workflow.rs and responses_workflow.rs now delegate to it; their duplicate local copies are deleted. - non_text_part_marker maps a part type to a bounded, static marker ([image omitted] / [audio omitted] / [file omitted] / [unsupported content omitted]). It returns &'static str and never echoes the attacker-controlled part `type` string, so a crafted type cannot inject transcript content - and the legacy [non_text_content] token is gone from both paths. This route surface still cannot carry image/audio bytes to the model (the product envelope is bytes-free by design); the marker is the honest signal that a non-text part was provided but omitted. Multimodal delivery is a separate follow-up. Tests: content_parts unit tests cover text-part sanitization, every non-text marker, the no-echo guarantee for crafted types, and non-object items dropped; existing chat/responses workflow tests still pass.
aecd2ad to
b68eaa3
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy [non_text_content] literal from the OpenAI-compat Chat Completions and Responses inbound parsing paths by centralizing content-part normalization in a shared module and emitting bounded, static non-text markers instead.
Changes:
- Introduced
content_partsmodule to share sanitization + content-part-to-text normalization logic. - Replaced
[non_text_content]with static markers like[image omitted],[audio omitted],[file omitted], or[unsupported content omitted]. - Deleted duplicate per-workflow implementations and routed both workflows through the shared helpers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs | Switches inbound content parsing to shared helpers and removes the legacy placeholder. |
| crates/ironclaw_reborn_openai_compat/src/lib.rs | Adds the new content_parts module behind the openai-compat-beta feature gate. |
| crates/ironclaw_reborn_openai_compat/src/content_parts.rs | New shared parsing + sanitization module with unit tests and bounded non-text markers. |
| crates/ironclaw_reborn_openai_compat/src/chat_workflow.rs | Switches inbound content parsing to shared helpers and removes the legacy placeholder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .collect::<Vec<_>>() | ||
| .join(" "), | ||
| Some(value) if !value.is_null() => "[non_text_content]".to_string(), | ||
| Some(value) if !value.is_null() => non_text_part_marker(None).to_string(), |
There was a problem hiding this comment.
Fixed in 4d465f869. A bare object-form top-level content now routes through the same per-part normalizer (content_array_item_text) as array items, so a typed object like {"type":"image_url",...} renders its specific marker ([image omitted]) — or, for a text-typed object, its text — instead of discarding the type for the generic marker. The type is still never echoed (the marker is a fixed &'static str).
| .collect::<Vec<_>>() | ||
| .join(" "), | ||
| value if !value.is_null() => "[non_text_content]".to_string(), | ||
| value if !value.is_null() => non_text_part_marker(None).to_string(), |
There was a problem hiding this comment.
Fixed in 4d465f869. A bare object-form top-level content now routes through the same per-part normalizer (content_array_item_text) as array items, so a typed object like {"type":"image_url",...} renders its specific marker ([image omitted]) — or, for a text-typed object, its text — instead of discarding the type for the generic marker. The type is still never echoed (the marker is a fixed &'static str).
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs (1)
6-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale module header after the shared-module migration.
Line 6-7 says mirroring continues “until” a shared normalization module exists, but this file now imports and uses
crate::content_parts(Line 13-15). Please update the header to match current behavior.As per coding guidelines, “When changing behavior in a function, re-read its docstring and adjacent comments; update or delete them in the same change to keep documentation in sync with code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs` around lines 6 - 7, The module header comment at lines 6-7 contains outdated documentation. It states that the ack and text helpers mirror the chat slice "until" a shared normalization module exists, but the code now imports and uses crate::content_parts (visible in lines 13-15). Update the header comment to accurately reflect that the code is currently using the shared crate::content_parts module instead of implying that mirroring continues until such a module is created.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_openai_compat/src/content_parts.rs`:
- Around line 37-41: The match arm for text types ("text" | "input_text" |
"output_text") in content_array_item_text returns None when the "text" field is
missing or not a string, causing these malformed parts to be silently dropped by
downstream filter_map. Instead, return a marker indicating unsupported content
(using non_text_part_marker) to flag the issue loudly. Additionally, add a
regression test using #[test] that verifies malformed text parts where "text" is
missing or non-string are marked with "[unsupported content omitted]" rather
than silently filtered out.
---
Outside diff comments:
In `@crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs`:
- Around line 6-7: The module header comment at lines 6-7 contains outdated
documentation. It states that the ack and text helpers mirror the chat slice
"until" a shared normalization module exists, but the code now imports and uses
crate::content_parts (visible in lines 13-15). Update the header comment to
accurately reflect that the code is currently using the shared
crate::content_parts module instead of implying that mirroring continues until
such a module is created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6eb7fb80-f318-41f0-9585-141075eded42
📒 Files selected for processing (4)
crates/ironclaw_reborn_openai_compat/src/chat_workflow.rscrates/ironclaw_reborn_openai_compat/src/content_parts.rscrates/ironclaw_reborn_openai_compat/src/lib.rscrates/ironclaw_reborn_openai_compat/src/responses_workflow.rs
…ping them
Address review on the shared content-part normalizer:
- A text-typed array part (`text`/`input_text`/`output_text`) whose `text` is
missing or non-string was returning None and getting silently dropped by the
downstream filter_map. It now emits a bounded marker so the part is observable
rather than vanishing (and the doc's "None only when not an object" holds).
- Bare object-form top-level `content` (non-standard but tolerated) was routed
to the generic marker, discarding any `type`. It now runs through the same
per-part logic, so `{"type":"image_url"}` renders `[image omitted]` etc.
Regression test for the malformed-text-part case.
…ts (nearai#4644) (nearai#4680) * fix(openai-compat): stop emitting [non_text_content] for non-text parts (nearai#4644) The Chat Completions and Responses inbound paths both collapsed any non-text content part (image_url / input_audio / file) to the opaque literal "[non_text_content]", which then reached the model as user text - the canary the issue calls out. They were also byte-identical duplicate parsers (the duplicate-pipeline smell in architecture.md). - New shared crate-private module content_parts owns the one copy of sanitize_product_text_fragment, content_array_item_text, and a new non_text_part_marker. Both chat_workflow.rs and responses_workflow.rs now delegate to it; their duplicate local copies are deleted. - non_text_part_marker maps a part type to a bounded, static marker ([image omitted] / [audio omitted] / [file omitted] / [unsupported content omitted]). It returns &'static str and never echoes the attacker-controlled part `type` string, so a crafted type cannot inject transcript content - and the legacy [non_text_content] token is gone from both paths. This route surface still cannot carry image/audio bytes to the model (the product envelope is bytes-free by design); the marker is the honest signal that a non-text part was provided but omitted. Multimodal delivery is a separate follow-up. Tests: content_parts unit tests cover text-part sanitization, every non-text marker, the no-echo guarantee for crafted types, and non-object items dropped; existing chat/responses workflow tests still pass. * fix(openai-compat): render typed/object content parts instead of dropping them Address review on the shared content-part normalizer: - A text-typed array part (`text`/`input_text`/`output_text`) whose `text` is missing or non-string was returning None and getting silently dropped by the downstream filter_map. It now emits a bounded marker so the part is observable rather than vanishing (and the doc's "None only when not an object" holds). - Bare object-form top-level `content` (non-standard but tolerated) was routed to the generic marker, discarding any `type`. It now runs through the same per-part logic, so `{"type":"image_url"}` renders `[image omitted]` etc. Regression test for the malformed-text-part case.
Summary
Kills the
[non_text_content]canary the issue calls out: the Chat Completions and Responses inbound paths both collapsed any non-text content part (image_url/input_audio/file) to that opaque literal, which then reached the model as user text. They were also byte-identical duplicate parsers (the duplicate-pipeline smell inarchitecture.md).What changed
content_partsowns the single copy ofsanitize_product_text_fragment,content_array_item_text, and a newnon_text_part_marker. Bothchat_workflow.rsandresponses_workflow.rsdelegate to it; their duplicate local copies are deleted.non_text_part_markermaps a parttypeto a bounded, static marker ([image omitted]/[audio omitted]/[file omitted]/[unsupported content omitted]). It returns&'static strand never echoes the attacker-controlledtypestring, so a crafted type cannot inject transcript content — and the legacy[non_text_content]token is gone from both paths.This route surface still cannot carry image/audio bytes to the model (the product envelope is bytes-free by design); the marker is the honest signal that a non-text part was provided but omitted. Multimodal delivery remains a separate follow-up (needs the byte-readback path).
Tests
content_partsunit tests: text-part sanitization, every non-text marker, the no-echo guarantee for crafted types, non-object items dropped. Existing chat/responses workflow tests still pass.Stacking
Based on
fix/4644-model-visible-attachments(#4677).Summary by CodeRabbit