Skip to content

fix: disable storage for Codex responses requests - #3192

Merged
graycyrus merged 1 commit into
tinyhumansai:mainfrom
honor2030:fix/codex-responses-store-false
Jun 2, 2026
Merged

graycyrus merged 1 commit into
tinyhumansai:mainfrom
honor2030:fix/codex-responses-store-false

Conversation

@honor2030

@honor2030 honor2030 commented Jun 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Send store: false on OpenAI/Codex Responses API requests.
  • Extend the existing Codex Responses API test to assert the outbound JSON body includes store: false.

Why

Codex OAuth routes through the ChatGPT backend Responses API. That path expects requests not to create stored responses, so explicitly setting store: false keeps the request shape aligned with Codex/ChatGPT account usage.

Test Plan

  • RED: strengthened responses_api_primary_posts_directly_to_responses to require store: false; before the implementation change, the mock did not match and the focused test failed with openai Responses API error.
  • GREEN: RUSTC="$(rustup which --toolchain 1.93.0 rustc)" rustup run 1.93.0 cargo test responses_api_primary_posts_directly_to_responses --manifest-path Cargo.toml --lib -- --nocapture → 1 passed, 0 failed.
  • RUSTC="$(rustup which --toolchain 1.93.0 rustc)" rustup run 1.93.0 cargo test openai_oauth --manifest-path Cargo.toml --lib -- --nocapture → 32 passed, 0 failed.
  • rustup run 1.93.0 cargo fmt --check
  • git diff --check

Summary by CodeRabbit

  • New Features

    • Extended API request configuration capabilities to support explicit control of server-side storage behavior for OpenAI-compatible provider endpoints, enabling management of data retention preferences
  • Tests

    • Updated test suite to validate that storage configuration settings are correctly included and serialized in outbound request payloads

@honor2030
honor2030 requested a review from a team June 2, 2026 07:47
@coderabbitai

coderabbitai Bot commented Jun 2, 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: a1bc81a7-7052-47bc-bc5c-4983cf54ddce

📥 Commits

Reviewing files that changed from the base of the PR and between 1e03dd2 and f68cb6f.

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

📝 Walkthrough

Walkthrough

The PR adds an optional store field to the OpenAI Responses API request type and sets it to false when the chat_via_responses fallback path constructs outgoing requests. Tests are updated to validate the new field in request body assertions.

Changes

Responses API store field

Layer / File(s) Summary
ResponsesRequest store field definition
src/openhuman/inference/provider/compatible_types.rs
ResponsesRequest adds an optional store: Option<bool> field with conditional serialization to control server-side behavior.
chat_via_responses usage and test validation
src/openhuman/inference/provider/compatible.rs, src/openhuman/inference/provider/compatible_tests.rs
The method now sets store: Some(false) in outgoing requests. Test matchers are tightened with body_json to validate the complete request payload including the new store field.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • tinyhumansai/openhuman#2902: Changes whether the /v1/responses fallback is attempted for specific providers, whereas this PR modifies the request payload sent when that fallback path is used.

Suggested labels

rust-core, bug

Poem

🐰 A store field appears so neat,
Set to false, the request's complete,
From types to tests, the change runs true,
Small but vital, through and through!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ 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 directly and clearly describes the main change: adding store: false to Responses API requests for Codex, which is the primary purpose of the pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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 and usage tips.

@coderabbitai coderabbitai Bot added rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. bug labels Jun 2, 2026
graycyrus
graycyrus previously approved these changes Jun 2, 2026

@graycyrus graycyrus 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.

Clean, well-scoped fix. Adding store: Option<bool> to ResponsesRequest with skip_serializing_if = "Option::is_none" is the right approach — it keeps the field absent for providers that don't use it while letting the Codex path set it explicitly to false.

The test tightening is solid: switching to body_json makes the mock an exact-match assertion on the full outbound payload, so any future regression on store will fail the test rather than silently slip through. The matcher aligns with the actual serialized body (instructions is None and thus skipped, stream and store are both present as false).

CI is fully green including the coverage gate. No concerns.

@graycyrus
graycyrus dismissed their stale review June 2, 2026 14:14

Auto-corrected: Rule violation — clean PRs must use COMMENT, not APPROVE

@graycyrus graycyrus 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.

Clean, well-scoped fix. Adding store: Option<bool> to ResponsesRequest with skip_serializing_if = "Option::is_none" is the right approach — it keeps the field absent for providers that don't use it while letting the Codex path set it explicitly to false.

The test tightening is solid: switching to body_json makes the mock an exact-match assertion on the full outbound payload, so any future regression on store will fail the test rather than silently slip through. The matcher aligns with the actual serialized body (instructions is None and thus skipped, stream and store are both present as false).

CI is fully green including the coverage gate. No concerns.

@graycyrus
graycyrus merged commit c10d747 into tinyhumansai:main Jun 2, 2026
24 of 27 checks passed
senamakel pushed a commit to senamakel/openhuman that referenced this pull request Jun 6, 2026
Co-authored-by: honor2030 <honor2030@users.noreply.github.com>
senamakel pushed a commit to nocstah/openhuman that referenced this pull request Sep 11, 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.

2 participants