Skip to content

fix: openai reasoning and params fixes - #7239

Merged
akshaydeo merged 1 commit into
mainfrom
09-16-fix_openai_reasoning_and_params_fixes
Sep 24, 2026
Merged

akshaydeo merged 1 commit into
mainfrom
09-16-fix_openai_reasoning_and_params_fixes

Conversation

@TejasGhatte

@TejasGhatte TejasGhatte commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Extends the sampling-parameter stripping logic to cover temperature, top_logprobs, and logprobs in addition to the existing top_p gate, and ensures that models which cannot disable reasoning (o-series, GPT-5 base/pro, GPT-6) have a "none" effort remapped to their lowest accepted level rather than forwarded to the API.

Changes

  • Renamed topPUnsupported to samplingParamUnsupported, generalizing it to accept any field name (top_p, temperature, top_logprobs, logprobs) and applying the same datasheet-driven + name-fallback logic to all four.
  • Added stripUnsupportedSamplingParams on OpenAIChatRequest and wired it into ToOpenAIChatRequest for OpenAI and Azure providers.
  • Extended ToOpenAIResponsesRequest to strip temperature, top_logprobs, and logprobs (in addition to top_p) for OpenAI/Azure hosts only; third-party OpenAI-compatible hosts (e.g. Groq) are left untouched.
  • For the Responses API, when logprobs is unsupported, the "message.output_text.logprobs" entry is removed from Include by cloning the slice first to avoid mutating the caller's params.
  • Added defaultCanDisableReasoning and omittedEffortReasons helpers to utils.go to correctly model which models default to "none" effort when omitted (GPT-5.1–5.4) versus which always reason (GPT-5-pro, GPT-6, o-series).
  • Corrected the omitted-effort semantics: an absent reasoning.effort now counts as "none" only for models whose default is "none" (GPT-5.1–5.4); for GPT-5.5+ and GPT-6, an omitted effort is treated as active reasoning.
  • Removed the -codex special-case from the name fallback; codex variants now follow their version's default.
  • Registered FieldTemperature, FieldTopLogprobs, and FieldLogprobs as typed constants in modelcaps.go.
  • Added comprehensive tests covering always-reasoning models, effort-conditional stripping, third-party host passthrough, and caller-params immutability.

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

go test ./core/providers/openai/... ./core/schemas/...

Key scenarios to validate:

  • gpt-6-astra with effort: "none" → effort remapped to "low", all four sampling fields stripped.
  • gpt-5.4 with no effort set → sampling fields preserved (defaults to "none").
  • gpt-5.5 with no effort set → sampling fields stripped (defaults to "medium").
  • gpt-5.6 with effort: "none" → sampling fields preserved.
  • o3 with effort: "high" → sampling fields stripped.
  • Groq-hosted openai/gpt-oss-120b → temperature preserved regardless of effort.
  • Caller's Params.Include slice is not mutated when logprobs is stripped from the Responses request.

Breaking changes

  • No

Security considerations

None. Changes are limited to request shaping before forwarding to upstream APIs; no auth, secrets, or PII are involved.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: maximhq/bifrost/.coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 6b880bc2-e601-48be-b2a1-5e249e97d3d6

📥 Commits

Reviewing files that changed from the base of the PR and between e53579a and 12b8b3b.

📒 Files selected for processing (1)
  • core/schemas/modelcaps.go

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved compatibility with OpenAI and Azure models by automatically handling unsupported sampling options.
    • Reasoning settings now normalize correctly for models that cannot disable reasoning.
    • Unsupported log-probability options are omitted when necessary.
    • Sampling options remain available for models that support them, including compatible third-party models.
    • Request settings are preserved without unexpectedly modifying caller-provided parameters.
    • Model-specific defaults now provide more consistent reasoning behavior.

Walkthrough

OpenAI and Azure chat and responses requests now normalize reasoning effort and filter sampling parameters by model capabilities. Tests cover model defaults, conditional support, caller-parameter preservation, and logprob include filtering.

Changes

OpenAI parameter handling

Layer / File(s) Summary
Reasoning capability defaults
core/providers/openai/utils.go, core/schemas/modelcaps.go
Model-name defaults now determine whether reasoning can be disabled and how omitted effort behaves. Sampling field constants were added.
Chat request shaping
core/providers/openai/chat.go, core/providers/openai/chat_test.go
Chat requests normalize non-disableable none effort and remove unsupported sampling fields. Tests cover normalization, filtering, preservation, and caller-parameter immutability.
Responses request shaping
core/providers/openai/responses.go, core/providers/openai/responses_test.go
Responses requests use generalized sampling checks, normalize reasoning effort, filter unsupported fields and logprob includes, and apply updated GPT-5.x fallback rules. Tests cover datasheet overrides, conditional support, and request conversion.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant RequestConversion
  participant ModelCapabilities
  participant OpenAIOrAzureAPI
  Client->>RequestConversion: provide reasoning and sampling parameters
  RequestConversion->>ModelCapabilities: resolve effort and field support
  ModelCapabilities-->>RequestConversion: return normalization and filtering decisions
  RequestConversion->>OpenAIOrAzureAPI: send normalized request
Loading

Merge Risk: ⚪ Minimal · up to 12b8b

The reviewed change is mergeable; no current-head defects were identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title identifies the main areas of change: OpenAI reasoning and request parameters. It is concise and relevant, though “fixes” is broad.
Description check ✅ Passed The description follows the template and explains the purpose, changes, affected areas, testing steps, breaking-change status, and security considerations. It also reports added tests. The unchecked c…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

TejasGhatte commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

coderabbitai[bot]
coderabbitai Bot previously approved these changes Sep 17, 2026
@TejasGhatte
TejasGhatte force-pushed the 09-16-fix_openai_reasoning_and_params_fixes branch 3 times, most recently from 12b8b3b to 9c0b34c Compare September 24, 2026 07:10
@TejasGhatte
TejasGhatte changed the base branch from dev to graphite-base/7239 September 24, 2026 08:07
@TejasGhatte
TejasGhatte force-pushed the 09-16-fix_openai_reasoning_and_params_fixes branch from 9c0b34c to a038118 Compare September 24, 2026 08:07
@TejasGhatte
TejasGhatte changed the base branch from graphite-base/7239 to main September 24, 2026 08:07
@TejasGhatte
TejasGhatte dismissed coderabbitai[bot]’s stale review September 24, 2026 08:07

The base branch was changed.

akshaydeo commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Merge activity

  • Sep 24, 10:06 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Sep 24, 10:07 AM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo
akshaydeo merged commit b304e56 into main Sep 24, 2026
11 of 12 checks passed
@akshaydeo
akshaydeo deleted the 09-16-fix_openai_reasoning_and_params_fixes branch September 24, 2026 10:07
@akshaydeo akshaydeo mentioned this pull request Sep 24, 2026
7 of 18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants