Skip to content

fix(openai): omit temperature for o1/o1-mini/o1-preview reasoning models#230

Open
zjshen14 wants to merge 1 commit into
mainfrom
fix/openai-o1-temperature
Open

fix(openai): omit temperature for o1/o1-mini/o1-preview reasoning models#230
zjshen14 wants to merge 1 commit into
mainfrom
fix/openai-o1-temperature

Conversation

@zjshen14

@zjshen14 zjshen14 commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • The o1/o1-mini/o1-preview family of OpenAI reasoning models does not accept a temperature parameter (it is fixed at 1 server-side). Passing temperature to these models causes a 400 Invalid parameter API error.
  • Added a lacksTemperatureSupport() helper that matches the o1 family specifically; o3 and o4 variants do support temperature and are unaffected.
  • The bug is triggered today when a user runs opencli run "..." --model o1-mini --temperature 0.5.

Related issue

Part of #43

Test plan

  • npm run typecheck && npm run lint && npm run format:check && npm test — all pass (699 tests)
  • New tests verify temperature is omitted for o1, o1-mini, o1-preview
  • New tests verify temperature IS passed for gpt-4o and o3-mini
  • Existing test for developer role on o-series models still passes

🤖 Generated with Claude Code


Generated by Claude Code

The o1 family of OpenAI reasoning models does not accept a temperature
parameter (it is fixed at 1). Passing temperature to these models causes
a 400 API error, which surfaces when a user runs:

  opencli run "..." --model o1-mini --temperature 0.5

o3 and o4 variants do support temperature and are unaffected.

Adds a lacksTemperatureSupport() helper and 6 new tests covering:
- temperature is passed for gpt-4o and o3-mini
- temperature is omitted for o1, o1-mini, and o1-preview
- temperature is omitted when not configured at all

Part of #43
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@zjshen14

zjshen14 commented Jun 8, 2026

Copy link
Copy Markdown
Owner Author

Review: fix(openai): omit temperature for o1/o1-mini/o1-preview reasoning models

What's good: Correct fix, correct regex (^o1(-|$) — matches o1, o1-mini, o1-preview, and nothing else). The tests are comprehensive: gpt-4o and o3-mini still receive temperature, o1/o1-mini/o1-preview don't, and the no-temperature-configured case is also covered. CI is green.

One minor style note: several new test cases re-mock OpenAI at the top of the test body via vi.mocked(OpenAI).mockImplementation(...) even though the same mock is already set by the outer beforeEach. The re-mocks in the o3-mini, o1, o1-mini, and o1-preview cases are no-ops. Not a correctness issue, just noise — the gpt-4o test doesn't need the re-mock and neither do these.


I'm not merging this directly because src/providers/openai.ts falls under src/providers/*, which the review-bot policy reserves for human sign-off even for well-contained fixes. The code is solid and the restriction is positional, not substantive.

Recommendation: Merge — the change is correct and the tests are thorough.


Generated by Claude Code

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.

2 participants