Skip to content

fix(inference): tolerate duplicate reasoning_content keys#4235

Open
YOMXXX wants to merge 2 commits into
tinyhumansai:mainfrom
YOMXXX:fix/GH-4204-duplicate-reasoning-content
Open

fix(inference): tolerate duplicate reasoning_content keys#4235
YOMXXX wants to merge 2 commits into
tinyhumansai:mainfrom
YOMXXX:fix/GH-4204-duplicate-reasoning-content

Conversation

@YOMXXX

@YOMXXX YOMXXX commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes OpenAI-compatible response parsing when providers emit duplicate reasoning_content keys in one JSON object.
  • Applies the same duplicate-key tolerance to both buffered ResponseMessage parsing and streaming StreamDelta parsing.
  • Adds regression coverage for Sentry TAURI-RUST-85R on response and SSE delta paths.
  • Includes a minimal translated README link cleanup to unblock the required Markdown Link Check, which scans docs/**/*.md even for Rust-only PRs.

Problem

  • NVIDIA-compatible endpoints can emit reasoning_content twice in the same message or delta object.
  • The prior hand-written deserializer still used a derived shadow struct, so serde rejected exact duplicate keys with duplicate field reasoning_content and dropped the whole completion.
  • Existing coverage only handled the distinct-key case: reasoning plus reasoning_content.
  • The required Markdown Link Check currently also fails on pre-existing stale localized README links unrelated to this inference fix.

Solution

  • Deserialize message and delta objects through serde_json::Value so exact duplicate JSON keys fold naturally with the last value retained.
  • Keep the existing canonical-field behavior: reasoning_content still wins over the reasoning alias when both are present.
  • Preserve null handling for optional fields and keep unknown provider fields ignored as before.
  • Update only the lychee-reported localized README links: old native voice docs URL, Reddit 403 link, and Star History fragment URL.

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • Diff coverage ≥ 80% — changed lines are covered by new Rust unit regressions; remote PR CI diff-cover remains the merge gate.
  • Coverage matrix updated — N/A: provider parsing bug fix and docs link cleanup, no feature row added/removed/renamed.
  • All affected feature IDs from the matrix are listed in the PR description under ## Related — N/A: no matrix feature ID applies.
  • No new external network dependencies introduced (mock backend used per Testing Strategy)
  • Manual smoke checklist updated if this touches release-cut surfaces (docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release manual smoke surface changed.
  • Linked issue closed via Closes #NNN in the ## Related section

Impact

  • Runtime impact: Rust core inference provider parsing only.
  • Compatibility impact: more tolerant parsing for quirky OpenAI-compatible providers; existing reasoning alias and canonical reasoning_content behavior are preserved.
  • Docs impact: localized README links now avoid known lychee failures.
  • No migration, UI, security, or network dependency impact.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: fix/GH-4204-duplicate-reasoning-content
  • Commit SHA: 37900c0bac99d69e5f1c3685a41ae6ae11069bfb

Validation Run

  • pnpm --filter openhuman-app format:check — N/A: no app source changes.
  • pnpm typecheck — N/A: no TypeScript changes.
  • Focused tests: 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).
  • Rust fmt/check (if changed): cargo fmt --manifest-path Cargo.toml --check; git diff --check; GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml.
  • Tauri fmt/check (if changed): N/A: no app/src-tauri changes.

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: duplicate reasoning_content response keys no longer fail deserialization; the last duplicate value is retained.
  • User-visible effect: affected NVIDIA-compatible completions should return normally instead of being dropped by a parse error.

Parity Contract

  • Legacy behavior preserved: reasoning_content still wins over reasoning; null optional fields remain accepted; unknown provider fields remain ignored.
  • Guard/fallback/dispatch parity checks: covered by the existing compatible provider test module.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none found for this branch.
  • Canonical PR: this PR.
  • Resolution (closed/superseded/updated): N/A.

Summary by CodeRabbit

  • Bug Fixes
    • Improved OpenAI-compatible response parsing to avoid failures when reasoning_content appears multiple times.
    • Ensured the final reasoning_content value is preserved for both non-streaming and streaming responses.
  • Tests
    • Added regression tests covering duplicate-key handling for standard and streaming deserialization.
  • Documentation
    • Updated README social link/Star History embed URLs and adjusted “native tools” documentation links across multiple locales.

@YOMXXX YOMXXX requested a review from a team June 28, 2026 11:32
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9f166869-0645-4709-b9df-10ba43bd1110

📥 Commits

Reviewing files that changed from the base of the PR and between db20bd5 and 37900c0.

📒 Files selected for processing (5)
  • docs/README.de.md
  • docs/README.ja-JP.md
  • docs/README.ko.md
  • docs/README.ur-pk.md
  • docs/README.zh-CN.md
✅ Files skipped from review due to trivial changes (3)
  • docs/README.de.md
  • docs/README.zh-CN.md
  • docs/README.ko.md

📝 Walkthrough

Walkthrough

Rewrites the OpenAI-compatible provider deserializers to tolerate duplicate reasoning_content keys in both response and streaming paths, and updates localized README files with revised Reddit, native voice, and Star History links.

Duplicate reasoning_content parsing

Layer / File(s) Summary
Map-based deserialization
src/openhuman/inference/provider/compatible_types.rs
Expands serde imports and rewrites ResponseMessage and StreamDelta manual deserialization to extract fields from a map, handling reasoning_content and reasoning with last-wins canonicalization plus shared helper functions.
Duplicate-key regression tests
src/openhuman/inference/provider/compatible_tests.rs
Adds non-streaming and streaming regression tests that parse JSON with duplicate reasoning_content keys and assert the final value is retained.

Localized README link updates

Layer / File(s) Summary
Social and embed link updates
docs/README.de.md, docs/README.ja-JP.md, docs/README.ko.md, docs/README.ur-pk.md, docs/README.zh-CN.md
Updates Reddit social link markup, changes native voice documentation targets, and switches Star History embeds to the api.star-history.com SVG form across localized README files.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • tinyhumansai/openhuman#3552: Updates the same deserialization area in compatible_types.rs to handle the earlier reasoning/reasoning_content collision and adds related parsing tests.

Suggested labels

rust-core, bug

Poem

🐇 I nibbled the JSON, one key then two,
The map kept the last one, just as it should do.
No parse-time thump, no vanished light,
The stream still hops on, clean and right.
And README stars now shimmer bright ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The localized README link/embed updates are unrelated to the duplicate-key parsing fix requested in #4204. Split the documentation cleanup into a separate PR or remove it if it is not needed for this fix.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: tolerating duplicate reasoning_content keys in inference parsing.
Linked Issues check ✅ Passed The PR fixes duplicate reasoning_content parsing in both buffered and streaming paths and adds regression tests as required by #4204.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/openhuman/inference/provider/compatible_types.rs (1)

515-537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider relocating these generic helpers to compatible_helpers.rs.

deserialize_object_fields and deserialize_optional_field are format-generic utilities, not type definitions. compatible_helpers.rs already hosts related conversion helpers, and compatible_types.rs already 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5a41a4f and db20bd5.

📒 Files selected for processing (2)
  • src/openhuman/inference/provider/compatible_tests.rs
  • src/openhuman/inference/provider/compatible_types.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 28, 2026
@YOMXXX

YOMXXX commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

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.

@coderabbitai coderabbitai Bot added bug rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. labels Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nvidia parse: tolerate duplicate reasoning_content key (TAURI-RUST-85R)

1 participant