#792 was renamed to "Support OpenAI reasoning models via OpenAI API"
See https://github.com/ChatGPTBox-dev/chatGPTBox/issues/792#issuecomment-3217208310, make sure not only o1 was supported:
At least o3, o3-mini, o4-mini, gpt-5, gpt-5-mini, and gpt-5-nano should also be supported and tested.
Check the review comments below, confirm if they are correct or not, and decide whether to make some follow-up changes or not:
In [src/services/apis/openai-api.mjs](https://github.com/ChatGPTBox-dev/chatGPTBox/pull/882/files/2be919e8dbcb2d36a68ca90596fdef94c976a389#diff-b817f1a71787e1acd00caa2811609b5984139009ad81aa78389a5240e8038adb):
The o1 model response handling lacks error checking. If data.choices is empty or undefined, accessing data.choices[0] will cause a runtime error. Add proper validation before accessing the response structure.
In [src/services/apis/openai-api.mjs](https://github.com/ChatGPTBox-dev/chatGPTBox/pull/882/files/2be919e8dbcb2d36a68ca90596fdef94c976a389#diff-b817f1a71787e1acd00caa2811609b5984139009ad81aa78389a5240e8038adb):
The response parsing logic is duplicated with data.choices[0] accessed multiple times. Consider extracting const choice = data.choices?.[0] at the beginning of the conditional block to improve readability and avoid repeated property access.
#792 was renamed to "Support OpenAI reasoning models via OpenAI API"
...
I've addressed all the feedback points:
Extended model support: Added o3, o3-mini, o4-mini, gpt-5, gpt-5-mini, and gpt-5-nano models as requested in issue #792.
Fixed error checking: Added proper validation to prevent runtime errors when data.choices is empty or undefined.
Refactored duplicated access: Extracted const choice = data.choices?.[0] at the beginning of conditional blocks to eliminate repeated property access and improve readability.
The changes maintain backward compatibility while extending support to all OpenAI reasoning models. All files pass linting and the build completes successfully.
CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.
[!IMPORTANT]
Review skipped
Bot user detected.
To trigger a single review, invoke the @coderabbitai review command.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Walkthrough
Adds new ChatGPT model entries to config and enables them by default. Introduces reasoning-model detection utility. Updates OpenAI API service to branch behavior for reasoning models vs standard chat: adjusts request payloads, message filtering, streaming handling, and finish conditions accordingly.
Changes
Cohort / File(s)
Summary
Model catalog updates src/config/index.mjs
Added four model keys and entries: o4-mini, gpt-5, gpt-5-mini, gpt-5-nano; included in defaultConfig.activeApiModes.
OpenAI API service logic src/services/apis/openai-api.mjs
Integrated reasoning-model branch: filters messages, switches to max_completion_tokens, disables streaming and fixes params for reasoning models; strengthened choice handling in both completion and chat compat flows.
Model utilities src/utils/model-name-convert.mjs
New export isUsingReasoningModel(configOrSession) detecting o1/o3/o4 and gpt-5 (excluding gpt-5-chat).
Sequence Diagram(s)
sequenceDiagram
autonumber
participant U as User
participant A as App (chatGPTBox)
participant O as OpenAI API
U->>A: Send prompt
A->>A: Determine model (isUsingReasoningModel)
alt Reasoning model (o1/o3/o4/gpt-5*)
A->>A: Filter to user/assistant messages
A->>O: POST /chat (stream=false, max_completion_tokens, fixed params)
O-->>A: Single response (choices[0].message/content)
A->>U: Deliver final answer
else Standard chat model
A->>O: POST /chat (stream=true, max_tokens, temperature)
O-->>A: SSE delta chunks
A->>U: Stream tokens
A->>U: Finish on finish_reason
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Assessment against linked issues
Objective
Addressed
Explanation
Replace max_tokens with max_completion_tokens for reasoning models (#792)
✅
Enforce only user/assistant messages for reasoning models (#792)
✅
Disable streaming/tools/functions for reasoning models (#792)
Assessment against linked issues: Out-of-scope changes
Code Change
Explanation
Added non-reasoning models gpt-5, gpt-5-mini, gpt-5-nano to catalog and defaults (src/config/index.mjs, multiple lines)
Linked issue targets enabling OpenAI reasoning models and API compat; adding general GPT-5 variants is not specified.
Added o4-mini catalog entry and default activation (src/config/index.mjs, multiple lines)
Issue references o1 series explicitly; inclusion of o4-mini may be related but not stated, making scope unclear.
Possibly related PRs
ChatGPTBox-dev/chatGPTBox#873 — Also extends model catalog in src/config/index.mjs; likely part of broader model list updates.
ChatGPTBox-dev/chatGPTBox#875 — Similar addition of ChatGPT model entries in config; overlaps in exported keys/Models mapping.
Suggested labels
Review effort 2/5
Poem
I thump my paw at tokens’ streams,
Then hop to reasoned, single beams—
Max completions, fixed and neat,
User, assistant—trim and fleet.
New names sprout like herbs in spring,
I twitch my nose and press “send”—ding!
Code burrows deeper; answers sing.
Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.
Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
Support
Need help? Join our Discord community for assistance with any issues or questions.
CodeRabbit Commands (Invoked using PR/Issue comments)
Type @coderabbitai help to get the list of available commands.
Other keywords and placeholders
Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Status, Documentation and Community
Visit our Status Page to check the current availability of CodeRabbit.
Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.
For reasoning models, the code still uses the SSE fetch path while setting stream=false; ensure the endpoint actually returns a single JSON payload and that onMessage is invoked appropriately by fetchSSE. If fetchSSE expects streaming chunks, non-streaming may break or never call onMessage/onEnd.
await fetchSSE(`${baseUrl}/chat/completions`, {
method: 'POST',
signal: controller.signal,
headers: {
'Content-Type': 'application/json',
Authorization: `Bearer ${apiKey}`,
},
body: JSON.stringify(requestBody),
onMessage(message) {
console.debug('sse message', message)
if (finished) return
if (message.trim() === '[DONE]') {
finish()
return
}
let data
try {
data = JSON.parse(message)
} catch (error) {
console.debug('json error', error)
return
}
if (isO1Model) {
// For o1 models (non-streaming), get the complete response
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data for o1 model')
return
}
const content = choice.message?.content
if (content) {
answer = content
port.postMessage({ answer: answer, done: false, session: null })
finish()
}
} else {
// For non-o1 models (streaming), handle delta content
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data')
return
}
const delta = choice.delta?.content
const content = choice.message?.content
const text = choice.text
if (delta !== undefined) {
answer += delta
} else if (content) {
answer = content
} else if (text) {
answer += text
}
port.postMessage({ answer: answer, done: false, session: null })
if (choice.finish_reason) {
finish()
return
}
}
},
async onStart() {},
async onEnd() {
port.postMessage({ done: true })
Filtering removes system messages for reasoning models, but the user question is appended after filtering. Confirm earlier records don’t include system content via other roles or metadata that could still be sent inadvertently.
// Filter out system messages for o1 models (only user and assistant are allowed)
const filteredPrompt = isO1Model
? prompt.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
: prompt
filteredPrompt.push({ role: 'user', content: question })
Hardcoded model list for reasoning detection may drift. Consider pattern-based detection (e.g., /^o[1-9]|^o\d-|o\d|o4-mini|gpt-5/) or a centralized config to avoid frequent updates.
Guard against tool/function outputs and non-text content by ignoring non-string content and skipping updates when role indicates tool-related messages to prevent corrupt UI output
Why: The suggestion correctly identifies a potential issue where non-string content could be processed, and proposes adding type checks to make the response handling more robust.
Medium
Possible issue
Sanitize conflicting request parameters
Ensure extraBody fields don't reintroduce unsupported params for reasoning models. Merge sanitized params last to override user-supplied fields, and explicitly strip max_tokens for reasoning requests to avoid API errors due to conflicting limits.
Why: The suggestion correctly identifies that parameters from extraBody could conflict with model-specific settings, and proposes a robust way to enforce the correct parameters, improving the reliability of API calls.
Medium
[ ] More <!-- /improve --more_suggestions=true -->
Previous suggestions
✅ Suggestions up to commit 65ec6b2
Category
Suggestion
Impact
General
✅ Preserve critical system instructionsSuggestion Impact:The commit changed filtering to keep all messages (including system) for non-reasoning models and restrict to user/assistant for reasoning models, aligning with the suggestion to preserve system messages when possible.
code diff:
- // Always filter out system messages; for reasoning models, only allow user and assistant
- const promptWithoutSystem = prompt.filter((msg) => msg.role !== 'system')
+ // Filter messages based on model type
const filteredPrompt = isReasoningModel
- ? promptWithoutSystem.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
- : promptWithoutSystem
+ ? prompt.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
+ : prompt
Removing all system messages can harm output quality and violate API expectations. Keep the latest system instruction for non-reasoning models, and for reasoning models, only drop system messages if strictly required. This prevents unintentional loss of critical context.
const prompt = getConversationPairs(
session.conversationRecords.slice(-config.maxConversationContextLength),
false,
)
-// Always filter out system messages; for reasoning models, only allow user and assistant
-const promptWithoutSystem = prompt.filter((msg) => msg.role !== 'system')
-const filteredPrompt = isReasoningModel
- ? promptWithoutSystem.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
- : promptWithoutSystem
+// Preserve system messages when supported to avoid losing critical instructions
+let filteredPrompt
+if (isReasoningModel) {
+ // Some reasoning endpoints may not accept system; restrict to user/assistant
+ filteredPrompt = prompt.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
+} else {
+ // Keep all roles, but if duplicates exist, keep the latest system message
+ const lastSystemIndex = [...prompt].reverse().findIndex((m) => m.role === 'system')
+ if (lastSystemIndex !== -1) {
+ const absoluteIndex = prompt.length - 1 - lastSystemIndex
+ filteredPrompt = prompt.filter((m, i) => m.role !== 'system' || i === absoluteIndex)
+ } else {
+ filteredPrompt = prompt
+ }
+}
filteredPrompt.push({ role: 'user', content: question })
[Suggestion processed]
Suggestion importance[1-10]: 9
__
Why: This suggestion points out a critical regression introduced in the PR, where removing all system messages for non-reasoning models would likely degrade response quality by losing important contextual instructions.
High
Broaden reasoning model detection
The regex excludes potential o2 variants and is brittle to future o releases. Use a broader, safer prefix check for o models and maintain the gpt-5* guard, reducing false negatives as new models ship.**
export function isUsingReasoningModel(configOrSession) {
const modelValue = getModelValue(configOrSession)
if (!modelValue) return false
- // Match o[134] pattern with optional dash and suffix (e.g., o1, o1-preview, o3-mini, o4-mini)
- if (/^o[134](-|$)/.test(modelValue)) {
+ // Consider all "o" family models as reasoning (e.g., o1, o2, o3, o4 and their suffixes)
+ if (modelValue.startsWith('o')) {
return true
}
// Match gpt-5* pattern but exclude gpt-5-chat-* variants
if (modelValue.startsWith('gpt-5') && !modelValue.startsWith('gpt-5-chat')) {
return true
}
return false
}
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that the regex for detecting reasoning models is brittle and proposes a more future-proof startsWith('o') check, improving maintainability and robustness against new model releases.
Medium
Possible issue
✅ Handle reasoning non-streaming content fallbacksSuggestion Impact:The commit implemented fallback to choice.text for reasoning models, handled array content, posted the message even when content is present, and explicitly called finish() when finish_reason exists or content is defined, preventing hangs.
code diff:
console.debug('No choice in response data for reasoning model')
return
}
- const content = choice.message?.content
- if (content) {
- answer = content
- port.postMessage({ answer: answer, done: false, session: null })
+ let content = choice.message?.content ?? choice.text
+ if (Array.isArray(content)) {
+ // Prefer output_text segments; fallback to any string content
+ const parts = content
+ .map((p) => {
+ if (typeof p === 'string') return p
+ if (p && typeof p === 'object') {
+ if (typeof p.output_text === 'string') return p.output_text
+ if (typeof p.text === 'string') return p.text
+ }
+ return ''
+ })
+ .filter(Boolean)
+ content = parts.join('')
+ }
+ if (content !== undefined && content !== null) {
+ answer = String(content)
+ }
+ port.postMessage({ answer, done: false, session: null })
+ }
+ if (choice.finish_reason || content !== undefined) {
finish()
}
Non-streaming reasoning responses may place text in choice.text or choice.message?.content. If content is absent, you currently drop valid output and never finish. Fallback to choice.text and explicitly finish on choice.finish_reason to avoid hanging.
if (isReasoningModel) {
// For reasoning models (non-streaming), get the complete response
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data for reasoning model')
return
}
- const content = choice.message?.content
- if (content) {
+ const content = choice.message?.content ?? choice.text
+ if (content !== undefined && content !== null) {
answer = content
port.postMessage({ answer: answer, done: false, session: null })
+ }
+ if (choice.finish_reason || content !== undefined) {
+ // finish once we have a full message or a reported finish
finish()
}
} else {
// For non-reasoning models (streaming), handle delta content
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data')
return
}
const delta = choice.delta?.content
const content = choice.message?.content
const text = choice.text
if (delta !== undefined) {
answer += delta
} else if (content) {
answer = content
} else if (text) {
answer += text
}
port.postMessage({ answer: answer, done: false, session: null })
if (choice.finish_reason) {
finish()
return
}
}
[Suggestion processed]
Suggestion importance[1-10]: 8
__
Why: This suggestion correctly identifies a potential bug where a non-streaming response from a reasoning model could be missed if it's in choice.text instead of choice.message.content, and that the request could hang by not calling finish().
✅ Fix false-positive model detectionSuggestion Impact:The commit updated the comment and regex from ^o[134](-|$) to ^(o1|o3|o4)(?:-|$), implementing the suggested stricter matching.
code diff:
- // Match o[134] pattern with optional dash and suffix (e.g., o1, o1-preview, o3-mini, o4-mini)
- if (/^o[134](-|$)/.test(modelValue)) {
+ // Explicitly match o1, o3, or o4 with optional dash and suffix (e.g., o1, o1-preview, o3-mini, o4-mini)
+ if (/^(o1|o3|o4)(?:-|$)/.test(modelValue)) {
Tighten the o regex to avoid misclassifying models like o10 or o13; explicitly match o1, o3, or o4 with optional suffixes and enforce a word boundary or dash*
export function isUsingReasoningModel(configOrSession) {
const modelValue = getModelValue(configOrSession)
if (!modelValue) return false
- // Match o[134] pattern with optional dash and suffix (e.g., o1, o1-preview, o3-mini, o4-mini)
- if (/^o[134](-|$)/.test(modelValue)) {
+ // Explicitly match o1, o3, or o4 with optional dash/suffix (e.g., o1, o1-preview, o3-mini, o4-mini)
+ if (/^(o1|o3|o4)(?:-|$)/.test(modelValue)) {
return true
}
// Match gpt-5* pattern but exclude gpt-5-chat-* variants
if (modelValue.startsWith('gpt-5') && !modelValue.startsWith('gpt-5-chat')) {
return true
}
return false
}
[Suggestion processed]
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly points out that the regex ^o[134](-|$) is too broad and could lead to future misclassifications; the proposed change makes the pattern more specific and robust.
Low
Possible issue
Remove unsupported params for reasoning
Avoid sending unsupported fields to reasoning endpoints to prevent 400 errors. Only set the minimal, accepted parameters and omit sampling/penalty fields that may be rejected. Keep stream: false and max_completion_tokens, but do not include temperature/top_p/n/penalties.
Why: The suggestion correctly identifies that sending unsupported parameters to an API can cause errors, and proposing to remove them for reasoning models is a valid and potentially critical improvement to prevent 400 errors.
Medium
General
✅ Make finish idempotentSuggestion Impact:The commit added an early return guard (if (finished) return) to make finish() idempotent and also simplified the postMessage payload to use the shorthand session property.
Prevent double-finishes and race conditions under SSE by making finish() idempotent. Early-return if already finished to avoid duplicate history pushes and messages.
Why: This is a good defensive programming practice that makes the finish() function idempotent, preventing potential race conditions and ensuring that completion logic is executed only once.
Prevent conflicting/duplicate params by removing max_tokens and temperature from extraBody when setting reasoning-model-specific fields, so the API doesn't receive both options and fail
Why: This suggestion correctly identifies a critical bug where conflicting parameters from extraBody could be sent to the API, causing requests to fail. Deleting potentially conflicting keys before setting the correct ones for each model type is a robust fix.
Medium
✅ Avoid unintended prompt mutationSuggestion Impact:The commit avoids mutating the original prompt by creating a new filtered array (promptWithoutSystem) and using it for further filtering, instead of directly using and potentially mutating the original prompt. This aligns with the suggestion's intent to preserve prompt immutability.
Preserve original prompt immutability by cloning before filtering/pushing to avoid mutating shared state that might be reused elsewhere, which can corrupt subsequent requests or history handling
Why: The suggestion correctly identifies that prompt is mutated when isReasoningModel is false, which is inconsistent and could lead to bugs if the code is refactored. Applying this change improves code quality and robustness.
Low
General
Preserve system context for reasoning models
Do not silently drop system messages; some sessions rely on them for behavior. Instead, merge system content into the first user message when using reasoning models to preserve instruction context without violating role constraints.
-// Filter out system messages for reasoning models (only user and assistant are allowed)
-const filteredPrompt = isReasoningModel
- ? prompt.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
- : prompt
+let filteredPrompt
+if (isReasoningModel) {
+ const systemText = prompt
+ .filter((m) => m.role === 'system')
+ .map((m) => (typeof m.content === 'string' ? m.content : ''))
+ .filter(Boolean)
+ .join('\n\n')
+ const nonSystem = prompt.filter((m) => m.role === 'user' || m.role === 'assistant')
+ if (systemText) {
+ // Prepend system guidance to the first user turn or to the new question
+ const firstUserIdx = nonSystem.findIndex((m) => m.role === 'user')
+ const prefix = `System instructions:\n${systemText}\n\n`
+ if (firstUserIdx >= 0) {
+ nonSystem[firstUserIdx] = {
+ ...nonSystem[firstUserIdx],
+ content: `${prefix}${nonSystem[firstUserIdx].content || ''}`,
+ }
+ } else {
+ // No prior user content; prepend to outgoing question
+ filteredPrompt = nonSystem
+ filteredPrompt.push({ role: 'user', content: `${prefix}${question}` })
+ // Skip extra push below since we've added the question
+ }
+ }
+ if (!filteredPrompt) {
+ filteredPrompt = nonSystem
+ filteredPrompt.push({ role: 'user', content: question })
+ }
+} else {
+ filteredPrompt = prompt
+ filteredPrompt.push({ role: 'user', content: question })
+}
-filteredPrompt.push({ role: 'user', content: question })
-
[Suggestion processed]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that filtering out system messages can lead to loss of important context, and it proposes a valid strategy to preserve this context by merging it into a user message.
Medium
Possible issue
✅ Robustly parse non-text contentSuggestion Impact:The commit modified reasoning-model handling to coalesce array content into strings (checking for string, output_text, or text fields) and converted non-null content to a string before posting, aligning with the suggestion to robustly parse non-text/array content.
code diff:
@@ -201,10 +202,27 @@
console.debug('No choice in response data for reasoning model')
return
}
- const content = choice.message?.content
- if (content) {
- answer = content
- port.postMessage({ answer: answer, done: false, session: null })
+ let content = choice.message?.content ?? choice.text
+ if (Array.isArray(content)) {
+ // Prefer output_text segments; fallback to any string content
+ const parts = content
+ .map((p) => {
+ if (typeof p === 'string') return p
+ if (p && typeof p === 'object') {
+ if (typeof p.output_text === 'string') return p.output_text
+ if (typeof p.text === 'string') return p.text
+ }
+ return ''
+ })
+ .filter(Boolean)
+ content = parts.join('')
+ }
+ if (content !== undefined && content !== null) {
+ answer = String(content)
+ }
+ port.postMessage({ answer, done: false, session: null })
+ }
+ if (choice.finish_reason || content !== undefined) {
finish()
}
Guard against tool or non-text message types. Check that content is a string or extract text segments when arrays/objects are returned, otherwise UI may remain blank or concatenate [object Object].
if (isReasoningModel) {
- // For reasoning models (non-streaming), get the complete response
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data for reasoning model')
return
}
- const content = choice.message?.content
- if (content) {
+ let content = choice.message?.content
+ if (Array.isArray(content)) {
+ content = content
+ .filter((p) => p?.type === 'text' && typeof p.text === 'string')
+ .map((p) => p.text)
+ .join('')
+ }
+ if (typeof content === 'object') {
+ // Ignore non-text objects (e.g., tool calls)
+ content = ''
+ }
+ if (typeof content === 'string' && content.length > 0) {
answer = content
port.postMessage({ answer: answer, done: false, session: null })
finish()
}
} else {
- // For non-reasoning models (streaming), handle delta content
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data')
return
}
- const delta = choice.delta?.content
+ let delta = choice.delta?.content
+ if (Array.isArray(delta)) {
+ delta = delta
+ .filter((p) => p?.type === 'text' && typeof p.text === 'string')
+ .map((p) => p.text)
+ .join('')
+ }
const content = choice.message?.content
const text = choice.text
- if (delta !== undefined) {
+ if (typeof delta === 'string') {
answer += delta
- } else if (content) {
+ } else if (typeof content === 'string') {
answer = content
- } else if (text) {
+ } else if (typeof text === 'string') {
answer += text
}
port.postMessage({ answer: answer, done: false, session: null })
-
if (choice.finish_reason) {
finish()
return
}
}
[Suggestion processed]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that API responses can contain non-string content (e.g., tool calls), and the proposed change adds valuable type checking to handle such cases gracefully.
Medium
✅ Suggestions up to commit f4ac0ae
Category
Suggestion
Impact
High-level
Over-broad "reasoning" model handling
The PR applies o1 beta limitations (no streaming, fixed params, no system messages, max_completion_tokens) to a broad set of models (o3, o4-mini, GPT-5 variants) without verifying each model’s current API/feature constraints. This risks breaking capabilities (e.g., streaming, system prompts, temperature) for models that actually support them or have different parameter names. Gate these behaviors strictly to models confirmed by docs to require them (currently o1-series), and avoid assuming future models share o1’s beta restrictions.
if (isO1Model) {
// o1 models use max_completion_tokens instead of max_tokens
requestBody.max_completion_tokens = config.maxResponseTokenLength
// o1 models don't support streaming during beta
requestBody.stream = false
// o1 models have fixed parameters during beta
requestBody.temperature = 1
requestBody.top_p = 1
requestBody.n = 1
requestBody.presence_penalty = 0
... (clipped 7 lines)
Solution Walkthrough:
Before:
// src/utils/model-name-convert.mjs
export function isUsingReasoningModel(configOrSession) {
const modelValue = getModelValue(configOrSession)
return (
modelValue === 'o1-preview' ||
modelValue === 'o1-mini' ||
modelValue === 'o3-preview' || // Assumes o3, o4, gpt-5 have same limits
modelValue === 'o3-mini' ||
modelValue === 'o4-mini' ||
modelValue === 'gpt-5' // ...and others
)
}
export function isUsingO1Model(configOrSession) { // Alias for all reasoning models
return isUsingReasoningModel(configOrSession)
}
// src/services/apis/openai-api.mjs
const isO1Model = isUsingO1Model(session) // This is true for o3, o4, gpt-5
if (isO1Model) {
// All reasoning models are forced into o1 beta limitations
requestBody.stream = false
requestBody.temperature = 1
// ...
}
After:
// src/utils/model-name-convert.mjs
export function isUsingO1Model(configOrSession) {
const modelValue = getModelValue(configOrSession)
return (
modelValue === 'o1-preview' ||
modelValue === 'o1-mini'
)
}
// Add separate checks for other model families if they have unique requirements
// src/services/apis/openai-api.mjs
const isO1 = isUsingO1Model(session) // This is ONLY true for o1 models
if (isO1) {
// Apply o1-specific beta limitations
requestBody.stream = false
requestBody.temperature = 1
// ...
} else {
// Default behavior for other models (including o3, o4, gpt-5)
requestBody.stream = true
// ...
}
Suggestion importance[1-10]: 9
__
Why: This suggestion correctly identifies a critical design flaw where beta limitations for o1 models are incorrectly applied to all future reasoning models, which will likely break their functionality.
High
Possible issue
Preserve system messages
Do not strip system messages for reasoning models; they are supported and often required for behavior control. Instead, ensure only valid roles are sent and convert any invalid roles if necessary. Removing system can silently change outputs.
-// Filter out system messages for o1 models (only user and assistant are allowed)
-const filteredPrompt = isO1Model
- ? prompt.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
- : prompt
+// Ensure only valid roles are sent; keep system messages
+const filteredPrompt = (isO1Model ? prompt : prompt).filter(
+ (msg) => msg.role === 'system' || msg.role === 'user' || msg.role === 'assistant'
+)
filteredPrompt.push({ role: 'user', content: question })
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly points out that removing the system message for reasoning models is likely incorrect, as this role is fundamental for guiding model behavior in the OpenAI API. The PR's code, based on a potentially mistaken comment, could lead to incorrect or unexpected model outputs.
Medium
Remove unsupported parameters
Avoid forcing unsupported parameters for reasoning models; omit fields they may reject. Only set max_completion_tokens and remove temperature, top_p, n, presence_penalty, and frequency_penalty for reasoning models to prevent API errors.
Why: The suggestion correctly identifies that setting hardcoded values for parameters like temperature and top_p might cause API errors if the models do not support them. Omitting these parameters is a safer approach to avoid potential API rejections, improving the robustness of the implementation.
Medium
General
✅ Robust reasoning response handlingSuggestion Impact:The commit updates terminology to "reasoning model" and adjusts related logs/comments, aligning with part of the suggestion. It does not implement the content/text fallback or finish_reason handling changes, but it reflects the model classification and messaging updates from the suggestion.
code diff:
-import { getModelValue, isUsingO1Model } from '../../utils/model-name-convert.mjs'
+import { getModelValue, isUsingReasoningModel } from '../../utils/model-name-convert.mjs'
/**
* @param {Browser.Runtime.Port} port
@@ -122,7 +122,7 @@
) {
const { controller, messageListener, disconnectListener } = setAbortController(port)
const model = getModelValue(session)
- const isO1Model = isUsingO1Model(session)
+ const isReasoningModel = isUsingReasoningModel(session)
const config = await getUserConfig()
const prompt = getConversationPairs(
@@ -130,8 +130,8 @@
false,
)
- // Filter out system messages for o1 models (only user and assistant are allowed)
- const filteredPrompt = isO1Model
+ // Filter out system messages for reasoning models (only user and assistant are allowed)
+ const filteredPrompt = isReasoningModel
? prompt.filter((msg) => msg.role === 'user' || msg.role === 'assistant')
: prompt
@@ -146,26 +146,26 @@
port.postMessage({ answer: null, done: true, session: session })
}
- // Build request body with o1-specific parameters
+ // Build request body with reasoning model-specific parameters
const requestBody = {
messages: filteredPrompt,
model,
...extraBody,
}
- if (isO1Model) {
- // o1 models use max_completion_tokens instead of max_tokens
+ if (isReasoningModel) {
+ // Reasoning models use max_completion_tokens instead of max_tokens
requestBody.max_completion_tokens = config.maxResponseTokenLength
- // o1 models don't support streaming during beta
+ // Reasoning models don't support streaming during beta
requestBody.stream = false
- // o1 models have fixed parameters during beta
+ // Reasoning models have fixed parameters during beta
requestBody.temperature = 1
requestBody.top_p = 1
requestBody.n = 1
requestBody.presence_penalty = 0
requestBody.frequency_penalty = 0
} else {
- // Non-o1 models use the existing behavior
+ // Non-reasoning models use the existing behavior
requestBody.stream = true
requestBody.max_tokens = config.maxResponseTokenLength
requestBody.temperature = config.temperature
@@ -194,11 +194,11 @@
return
}
- if (isO1Model) {
- // For o1 models (non-streaming), get the complete response
+ if (isReasoningModel) {
+ // For reasoning models (non-streaming), get the complete response
const choice = data.choices?.[0]
if (!choice) {
- console.debug('No choice in response data for o1 model')
+ console.debug('No choice in response data for reasoning model')
return
}
const content = choice.message?.content
@@ -208,7 +208,7 @@
finish()
}
} else {
- // For non-o1 models (streaming), handle delta content
+ // For non-reasoning models (streaming), handle delta content
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data')
Handle both message.content and plain text for reasoning models, and guard against function/tool calls to avoid missing outputs. Also, finish when finish_reason is set to ensure completion even if no content is present.
if (isO1Model) {
- // For o1 models (non-streaming), get the complete response
const choice = data.choices?.[0]
if (!choice) {
- console.debug('No choice in response data for o1 model')
+ console.debug('No choice in response data for reasoning model')
return
}
- const content = choice.message?.content
- if (content) {
+ const content = choice.message?.content ?? choice.text
+ if (content !== undefined) {
answer = content
- port.postMessage({ answer: answer, done: false, session: null })
+ port.postMessage({ answer, done: false, session: null })
+ }
+ if (choice.finish_reason) {
finish()
}
} else {
- // For non-o1 models (streaming), handle delta content
const choice = data.choices?.[0]
if (!choice) {
console.debug('No choice in response data')
return
}
const delta = choice.delta?.content
const content = choice.message?.content
const text = choice.text
if (delta !== undefined) {
answer += delta
} else if (content) {
answer = content
} else if (text) {
answer += text
}
- port.postMessage({ answer: answer, done: false, session: null })
+ port.postMessage({ answer, done: false, session: null })
if (choice.finish_reason) {
finish()
- return
}
}
[Suggestion processed]
Suggestion importance[1-10]: 6
__
Why: The suggestion improves response handling by checking for content in both choice.message.content and choice.text, making the code more robust to variations in the API response format. It also correctly ensures finish() is called when a finish_reason is present, preventing the conversation from getting stuck.
@copilot isUsingO1Model is a poor naming choice. Given the PR title renaming, it's best to avoid using "o1" in the naming and opt for something like "reasoningModel".
BTW, o3 & o1 are both old, don't add them in the default model list, we just need to make them supported.
@copilot isUsingO1Model is a poor naming choice. Given the PR title renaming, it's best to avoid using "o1" in the naming and opt for something like "reasoningModel".
BTW, o3 & o1 are both old, d...
I've addressed both feedback points:
Improved naming: Replaced isUsingO1Model with consistent isUsingReasoningModel throughout the codebase to avoid the "o1" naming limitation.
Updated default model list: Removed o1 and o3 models (chatgptApiO1Preview, chatgptApiO1Mini, chatgptApiO3Preview, chatgptApiO3Mini) from the default activeApiModes while keeping them defined in the Models object so they remain fully supported but not active by default.
All files pass linting and the build completes successfully with these changes.
@copilot we don't need to define old models, users can add them manually if they really need to use the old ones.
I've completely removed the old model definitions (o1 and o3 series) from the codebase. The models no longer appear in the chatgptApiModelKeys array, Models object, or isUsingReasoningModel function. Users can now manually add these old models if they specifically need them. The codebase now only includes the newer reasoning models (o4-mini, gpt-5, gpt-5-mini, gpt-5-nano).
@copilot we should use smarter method, instead of fully hard-coded reasoning model IDs. At least using regex to match o[134] with optional following - could be helpful, also, model IDs starting with gpt-5, except gpt-5-chat-latest(or, gpt-5-chat-*), are reasoning model.
@copilot we should use smarter method, instead of fully hard-coded reasoning model IDs. At least using regex to match o[134] with optional following - could be helpful, also, model IDs starting wi...
I've implemented smarter reasoning model detection using regex patterns as requested:
o[134] pattern matching: Uses regex /^o[134](-|$)/ to match o1, o3, o4 models with optional suffixes (e.g., o1-preview, o3-mini, o4-mini)
gpt-5 pattern matching*: Matches models starting with gpt-5 but excludes gpt-5-chat-* variants (so gpt-5, gpt-5-mini, gpt-5-nano are reasoning models, but gpt-5-chat-latest is not)
This approach is future-proof and automatically supports new reasoning models without requiring code changes. The implementation has been tested with comprehensive test cases and maintains backward compatibility.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Fixed the CI issue. There was a syntax error in the OpenAI API file caused by unmatched braces around the postMessage call. The build and linting now pass successfully.
@copilot, address the feedback from Copilot and Codex bot.
I've addressed all the feedback from Copilot and Codex bots:
Fixed regex pattern (Copilot feedback): Updated the pattern to ^(o1|o3|o4)(?:$|-[a-zA-Z][\w-]*)$ to avoid false positives like o10, o30, o40 while still matching valid models like o1, o1-preview, o3-mini, o4-mini.
Enhanced reasoning model support (Codex feedback): The array content flattening for reasoning model responses was already implemented and working correctly - it extracts output_text segments from structured response arrays to display properly in the UI.
Added tool/function removal (CodeRabbit suggestion): Reasoning models now properly strip unsupported tools, tool_choice, functions, and function_call parameters to prevent API errors.
All changes maintain backward compatibility and the build/lint passes successfully.
The current implementation is working correctly with both lint and build passing. Let me know if you'd like me to make any specific improvements or if you see additional bot feedback I should address.
I've addressed the feedback with comprehensive improvements:
Enhanced regex pattern: Made the o-series pattern more specific to only allow known OpenAI suffixes (preview, mini, turbo, instruct, nano, small, medium, large) preventing false positives
Improved content processing: Extracted helper function with better error handling and defensive programming for structured response arrays
Fixed finish condition: Only finishes on proper finish_reason, preventing premature completion
Added API key validation: Better error handling with input validation
Code organization: Separated concerns with dedicated helper function for cleaner code structure
All changes maintain backward compatibility and pass linting/build successfully.
josStorer
![coderabbitai[bot] avatar](https://avatars.githubusercontent.com/in/347564?v=4 "Published by a pub.dev verified publisher"){kind=small}
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}