Updating OpenAI reasoning parameters for gpt-5.2 #2637
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenAI reasoning parameters configuration for gpt-5.2 and related models. The changes add support for a new 'concise' summary option and implement model-specific parameter defaults through a new helper function.
Key changes:
- Adds 'concise' as a valid option for the
ResponsesApiReasoningSummaryconfiguration - Introduces
reasoningParameterValuesBasedOnModelto override reasoning parameters for specific model families (hidden model B) - Adds a new model hash to the
HIDDEN_MODEL_B_HASHESarray to identify gpt-5.2+ models
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/platform/endpoint/node/responsesApi.ts |
Adds new function to set model-specific reasoning parameters; imports isHiddenModelB helper; changes default summary handling when config is 'off' |
src/platform/endpoint/common/chatModelCapabilities.ts |
Adds new model hash to HIDDEN_MODEL_B_HASHES array for gpt-5.2 identification |
src/platform/configuration/common/configurationService.ts |
Extends ResponsesApiReasoningSummary type to include 'concise' option |
| const effort = effortConfig === 'default' ? 'medium' : effortConfig; | ||
| const summary = summaryConfig === 'off' ? undefined : summaryConfig; | ||
| let effort = effortConfig === 'default' ? 'medium' : effortConfig; | ||
| let summary = summaryConfig === 'off' ? 'detailed' : summaryConfig; |
There was a problem hiding this comment.
The logic change on line 61 modifies the behavior when summaryConfig === 'off'. Previously, this would result in summary = undefined, but now it defaults to 'detailed'. This changes the existing behavior: when users explicitly set the config to 'off', they likely expect reasoning summaries to be disabled, not to use 'detailed'.
Consider either:
- Keeping the original behavior where 'off' results in undefined
- Or if the behavior change is intentional, it should be documented in the PR description as a breaking change
| let summary = summaryConfig === 'off' ? 'detailed' : summaryConfig; | |
| let summary = summaryConfig === 'off' ? undefined : summaryConfig; |
| function reasoningParameterValuesBasedOnModel(model: string, effort: ResponsesReasoningEffort, summary: ResponsesReasoningSummary): { effort?: ResponsesReasoningEffort; summary?: ResponsesReasoningSummary } | undefined { | ||
| if (isHiddenModelB(model)) { | ||
| return { | ||
| effort, | ||
| summary: 'concise', | ||
| }; | ||
| } |
There was a problem hiding this comment.
The function reasoningParameterValuesBasedOnModel always overrides the summary parameter to 'concise' for hidden model B, regardless of the input summary value. However, it returns the effort parameter unchanged. This seems inconsistent - if the function is meant to override defaults for gpt-5.2+, why is only summary being overridden?
Additionally, the function accepts a summary parameter but ignores it when returning, making the parameter misleading. Consider either:
- Removing the
summaryparameter if it's not used - Or using it in some conditional logic if there's a specific reason to accept it
|
|
||
| // for gpt-5.2 + models, changing the default reasoning parameters | ||
| type ResponsesReasoningEffort = 'low' | 'medium' | 'high'; | ||
| type ResponsesReasoningSummary = 'concise' | 'detailed'; |
There was a problem hiding this comment.
The type definition for ResponsesReasoningSummary is missing the 'off' option that is present in the configuration type ConfigKey.ResponsesApiReasoningSummary. The config type is 'off' | 'concise' | 'detailed' but the local type only includes 'concise' | 'detailed'.
While 'off' is handled specially in the code (converting to 'detailed'), the type should accurately reflect all possible config values to maintain type safety and code clarity. Consider adding 'off' to the type definition or creating a separate type that represents the normalized values after processing the config.
| type ResponsesReasoningSummary = 'concise' | 'detailed'; | |
| type ResponsesReasoningSummary = 'off' | 'concise' | 'detailed'; |
| return body; | ||
| } | ||
|
|
||
| // for gpt-5.2 + models, changing the default reasoning parameters |
There was a problem hiding this comment.
The comment "for gpt-5.2 + models" is vague and could be clearer. The function applies to "hidden model B" which may include various models, not just gpt-5.2. Consider updating the comment to be more accurate, such as "Adjusts reasoning parameters for specific model families (hidden model B)" or refer to the actual logic that determines which models are affected.
| // for gpt-5.2 + models, changing the default reasoning parameters | |
| // Adjust reasoning parameter defaults for specific model families (hidden model B) |
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There are two consecutive blank lines (lines 90-91) which is inconsistent with the rest of the codebase style. According to the coding standards, there should typically be only one blank line between functions.
No description provided.