fix(llm): default missing OpenAI image detail to auto - #1940
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces standardized normalization for OpenAI-style image input 'detail' fields across multiple providers, including GitHub Copilot, NearAI, and the Rig adapter, ensuring they default to 'auto' when missing or invalid. The changes include new utility functions for normalization, updated message conversion logic, and comprehensive unit tests. Review feedback suggests adopting a more functional, iterator-based approach in the NearAI implementation to avoid in-place mutation and optimize for WASM environments. Additionally, it is recommended to explicitly use ImageDetail::Auto instead of unwrap_or_default() in the Rig adapter to ensure robustness against future changes in the underlying library.
| parts.extend(msg.content_parts.into_iter().map(|part| match part { | ||
| crate::llm::ContentPart::ImageUrl { mut image_url } => { | ||
| image_url.detail = Some(image_url.normalized_openai_detail()); | ||
| crate::llm::ContentPart::ImageUrl { image_url } | ||
| } | ||
| other => other, | ||
| })); |
There was a problem hiding this comment.
The current implementation of From<ChatMessage> for ChatCompletionMessage performs an in-place mutation of image_url.detail. To maintain consistency with other providers in this PR and optimize for WASM by avoiding unnecessary heap allocations, consider using a functional approach with iterators for the transformation. This avoids side effects and aligns with repository guidelines on performance and consistency.
References
- To improve performance in WASM, avoid unnecessary heap allocations by using iterators directly instead of collecting them into a Vec.
- Prioritize consistency with existing code patterns over refactoring, especially when adding new components like providers.
| ImageDetail::from_str(&image_url.normalized_openai_detail()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Using unwrap_or_default() on ImageDetail::from_str is safe but potentially fragile. If the rig crate's ImageDetail enum changes its default variant in a future version, it could lead to silent behavior changes. Using unwrap_or(ImageDetail::Auto) is more robust and follows the principle of avoiding implicit defaults for critical fields to prevent silent failures.
| ImageDetail::from_str(&image_url.normalized_openai_detail()) | |
| .unwrap_or_default(); | |
| ImageDetail::from_str(&image_url.normalized_openai_detail()) | |
| .unwrap_or(ImageDetail::Auto); |
References
- For critical fields, avoid using default values to prevent silent failures; it is safer for logic to be explicit or fail if data is missing.
serrrfirat
left a comment
There was a problem hiding this comment.
Findings:
-
The fix is incomplete: the ChatGPT Responses-provider path still sends image inputs without a
detailfield. Insrc/llm/codex_chatgpt.rs,input_imageis serialized with onlyimage_url, so image-bearing requests throughCodexChatGptProviderstill bypass the new normalization added elsewhere. That path is reachable from the normal attachment flow insrc/agent/session.rs, which builds image turns withuser_with_parts(..., turn.image_content_parts.clone()). If the bug here is “missing OpenAI image detail breaks requests”, this provider remains exposed. -
The added coverage misses the live attachment message shape and would not catch the gap above. The existing
codex_chatgpttest builds a synthetic message with embeddedContentPart::Textand nomsg.content, while the runtime path usesmsg.contentplus image-onlycontent_parts. A regression test for that real shape is still missing.
Residual risks:
- Validation is still mostly serializer-level unit coverage; there is no end-to-end/provider-request test for the actual attachment flow across the affected providers.
- I did not complete a local
cargo testrun during review, so this review is source-based rather than backed by a finished test pass.
|
Reviewed. Clean, correct, and well-tested. Approving with one minor architectural observation. What it does right
Minor
No Critical/High/Medium issues. LGTM. |
Resolve conflicts in two files: - src/bridge/effect_adapter.rs: keep both HEAD's engine_store / skill_registry fields (for v1→v2 skill sync) and staging's new workspace_mounts field (per-project sandbox from #2211). Merge the ironclaw_engine import list accordingly. - src/llm/rig_adapter.rs: adopt staging's detail-normalized image handling (#1940) — the upstream `ImageDetail::from_str(... normalized_openai_detail())` helper already defaults missing values to auto, so drop the HEAD-only parse_image_detail helper and its two call sites. Keep staging's expanded test coverage (data-url auto + explicit low/high preservation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Edward Ji <26658037+edwardji@users.noreply.github.com> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Summary
image_url.detailis normalized to"auto"instead of failing before request sendRigAdapterboundary used by the generic OpenAI-compatible provider, which was passingdetail: Noneinto rig-core fordata:image/...inputslowandhighdetail values and keep missingimage_url.urlrejectedFEATURE_PARITY.mdnote for OpenAI-compatible provider behaviorChange Type
Linked Issue
None
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featurescargo test --manifest-path /Users/1954d/projects/IronClaw/Cargo.toml image_detail,cargo test --manifest-path /Users/1954d/projects/IronClaw/Cargo.toml without_detail,cargo test --manifest-path /Users/1954d/projects/IronClaw/Cargo.toml without_urlIncomingAttachment -> ContentPart::ImageUrl -> RigAdapter/OpenAI-compatible payloadand verified missing image detail now defaults toautoSecurity Impact
None
Database Impact
None
Blast Radius
Touches OpenAI-style image message conversion in
RigAdapter,nearai_chat, andgithub_copilot. Main risk would be provider-specific image payload regressions, covered by focused conversion tests.Rollback Plan
Revert commit
a6a2c63cto restore previous behavior.Review track: B