-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: enable prompt enhancement for GPT-5 and Codex Mini models #7335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Implement completePromptViaResponsesApi to collect streaming responses - Add SSE fallback for robustness when SDK fails - Support all GPT-5 variants (gpt-5, gpt-5-mini, gpt-5-nano) and Codex Mini - Add comprehensive test coverage for new functionality - Fixes #7334
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
* Handles non-streaming completion for models that use the Responses API (GPT-5 and Codex Mini) | ||
* by collecting the streaming response into a complete string. | ||
*/ | ||
private async completePromptViaResponsesApi(prompt: string): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two methods have significant duplication in their event handling logic. Consider extracting the common event processing into a shared helper method to reduce code duplication and improve maintainability. Both methods handle the same event types (response.text.delta, response.reasoning.delta, response.output_item.added, etc.) with identical logic.
|
||
if (typeof (stream as any)[Symbol.asyncIterator] !== "function") { | ||
// Fall back to SSE if SDK doesn't return an AsyncIterable | ||
return await this.completePromptViaResponsesApiSSE(requestBody) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the SDK doesn't return an AsyncIterable, we fall back to SSE silently. Consider adding a console.warn here to help with debugging:
return await this.completePromptViaResponsesApiSSE(requestBody) | |
// Fall back to SSE if SDK doesn't return an AsyncIterable | |
console.warn('[GPT-5] SDK did not return AsyncIterable, falling back to SSE implementation'); | |
return await this.completePromptViaResponsesApiSSE(requestBody) |
reader.releaseLock() | ||
} catch (error) { | ||
if (error instanceof Error) { | ||
throw new Error(`Failed to complete prompt via GPT-5 API: ${error.message}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling here wraps errors with a specific message, but in the main method (line 1393) the fallback happens silently. Consider consistent error handling - either log both fallbacks or wrap both errors consistently.
@@ -470,6 +470,182 @@ describe("OpenAiNativeHandler", () => { | |||
}) | |||
}) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test coverage! Consider adding a few edge case tests:
- Network timeout scenarios during streaming
- Partial response handling when stream is interrupted
- Mixed content types in a single response (text + reasoning together)
These would help ensure robustness in production edge cases.
@@ -1276,4 +1277,254 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio | |||
throw error | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding JSDoc comments to document the purpose and behavior of these new methods. This would help future maintainers understand the fallback strategy and the dual-path approach (SDK with SSE fallback).
Closing in favor of #7067 |
Related GitHub Issue
Closes: #7334
Roo Code Task Context (Optional)
This PR was created with assistance from Roo Code to fix the GPT-5 prompt enhancement issue.
Description
This PR fixes the "completePrompt is not supported" error that occurs when using the Enhance Prompt feature with GPT-5 models (gpt-5, gpt-5-mini, gpt-5-nano) and Codex Mini.
Key implementation details:
completePrompt
method in the OpenAI Native provider to support models that use the Responses APIcompletePromptViaResponsesApi
method that collects streaming responses into a complete stringcompletePromptViaResponsesApiSSE
) for robustness when the SDK failsDesign choices:
Test Procedure
How I tested:
npx vitest run api/providers/__tests__/openai-native.spec.ts
npx eslint api/providers/openai-native.ts --max-warnings 0
How reviewers can verify:
cd src && npx vitest run api/providers/__tests__/openai-native.spec.ts
Pre-Submission Checklist
Screenshots / Videos
N/A - This is a backend fix for API compatibility.
Documentation Updates
The fix is transparent to users - the Enhance Prompt feature will simply start working with GPT-5 models without any user-facing changes needed.
Additional Notes
This implementation follows the same pattern used for other streaming-to-completion conversions in the codebase. The SSE fallback ensures robustness even if the OpenAI SDK has issues with the Responses API.
Get in Touch
Available for any questions about this implementation.
Important
Fixes
completePrompt
for GPT-5 and Codex Mini models by supporting streaming responses and adding robust error handling.completePrompt
inopenai-native.ts
.completePromptViaResponsesApi
to handle streaming responses for these models.completePromptViaResponsesApiSSE
for robustness.openai-native.spec.ts
.This description was created by
for 6af1f2f. You can customize this summary. It will automatically update as commits are pushed.