fix(inference): tolerate duplicate reasoning_content keys#4235
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughRewrites the OpenAI-compatible provider deserializers to tolerate duplicate Duplicate
Localized README link updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/inference/provider/compatible_types.rs (1)
515-537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider relocating these generic helpers to
compatible_helpers.rs.
deserialize_object_fieldsanddeserialize_optional_fieldare format-generic utilities, not type definitions.compatible_helpers.rsalready hosts related conversion helpers, andcompatible_types.rsalready exceeds the ~500-line module budget; moving them improves cohesion and keeps the types module from growing further.As per coding guidelines, Rust modules must be ≤ ~500 lines in size.
🤖 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 `@src/openhuman/inference/provider/compatible_types.rs` around lines 515 - 537, Move the format-generic deserialization helpers out of compatible_types.rs and into compatible_helpers.rs: relocate deserialize_object_fields and deserialize_optional_field, then update any callers in the compatible types path to use the new location. Keep compatible_types.rs focused on type definitions and serde implementations, and preserve the existing behavior and signatures while doing the move.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.
Nitpick comments:
In `@src/openhuman/inference/provider/compatible_types.rs`:
- Around line 515-537: Move the format-generic deserialization helpers out of
compatible_types.rs and into compatible_helpers.rs: relocate
deserialize_object_fields and deserialize_optional_field, then update any
callers in the compatible types path to use the new location. Keep
compatible_types.rs focused on type definitions and serde implementations, and
preserve the existing behavior and signatures while doing the move.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32f2b1f2-2b37-4b24-b06d-ea484af86ea4
📒 Files selected for processing (2)
src/openhuman/inference/provider/compatible_tests.rssrc/openhuman/inference/provider/compatible_types.rs
|
Markdown Link Check failed on this Rust-only PR because the workflow scans docs/**/*.md and hit pre-existing stale links in five localized READMEs. I added the same minimal cleanup used on #4234: update the old native voice docs URL, remove the Reddit hyperlink that returns 403 to lychee, and replace the Star History fragment URL with the SVG endpoint. The docs change is limited to the exact files/link categories reported by lychee so the inference parser fix can stay mergeable. |
Summary
reasoning_contentkeys in one JSON object.ResponseMessageparsing and streamingStreamDeltaparsing.docs/**/*.mdeven for Rust-only PRs.Problem
reasoning_contenttwice in the same message or delta object.duplicate field reasoning_contentand dropped the whole completion.reasoningplusreasoning_content.Solution
serde_json::Valueso exact duplicate JSON keys fold naturally with the last value retained.reasoning_contentstill wins over thereasoningalias when both are present.nullhandling for optional fields and keep unknown provider fields ignored as before.Submission Checklist
## Related— N/A: no matrix feature ID applies.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release manual smoke surface changed.Closes #NNNin the## RelatedsectionImpact
reasoningalias and canonicalreasoning_contentbehavior are preserved.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/GH-4204-duplicate-reasoning-content37900c0bac99d69e5f1c3685a41ae6ae11069bfbValidation Run
pnpm --filter openhuman-app format:check— N/A: no app source changes.pnpm typecheck— N/A: no TypeScript changes.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib duplicate_reasoning_content_keys(2 passed);GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib openhuman::inference::provider::compatible::tests(199 passed).cargo fmt --manifest-path Cargo.toml --check;git diff --check;GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml.app/src-taurichanges.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
reasoning_contentresponse keys no longer fail deserialization; the last duplicate value is retained.Parity Contract
reasoning_contentstill wins overreasoning;nulloptional fields remain accepted; unknown provider fields remain ignored.Duplicate / Superseded PR Handling
Summary by CodeRabbit
reasoning_contentappears multiple times.reasoning_contentvalue is preserved for both non-streaming and streaming responses.