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

@zjshen14

Copy link
Copy Markdown
Owner Author

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

What's good: The fix is correct and precisely scoped. lacksTemperatureSupport() with /^o1(-|$)/ correctly matches the original o1 family (o1, o1-mini, o1-preview) while leaving o3-mini, o4-mini, and future models unaffected. The function comment explains the non-obvious server-side constraint (o1 temperature is fixed at 1) — justified per project conventions. Test coverage is thorough: 6 tests covering all three o1 variants, o3-mini positive case, gpt-4o positive case, and the no-temperature-configured baseline. This closes a real 400 Invalid parameter failure for anyone running opencli run "..." --model o1-mini --temperature 0.5.

Notes

1. src/providers/openai.ts is outside the bot's auto-merge scope — provider files require manual maintainer review. No objection to the change itself; the implementation is clean.

2. Merge order dependency with #252 — once config.temperature is wired through (PR #252), o1 users who have temperature: 0.7 in ~/.opencli/config.json will hit this bug silently even without passing --temperature. Landing this PR before #252 is the correct order (which the second comment on #252 already notes).

Recommendation: Merge. Suggested order: this PR (#230) first, then #252.


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