-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: OpenAI provider/types/UI updates; provider state persistence #6921
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
feat: OpenAI provider/types/UI updates; provider state persistence #6921
Conversation
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.
Thank you for your contribution! I've reviewed the changes and found several issues that need attention. The PR successfully updates the OpenAI provider to support new models and state persistence, but there are some critical issues around duplicate model definitions and error handling that should be addressed.
| systemPromptRole: "developer", | ||
| supportsTemperature: false, | ||
| }, | ||
| "gpt-5": { |
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.
Critical Issue: There are duplicate model entries here - both versioned (e.g., "gpt-5-2025-08-07") and unversioned (e.g., "gpt-5") variants with identical configurations. This creates redundancy and potential confusion.
Consider using a single source of truth with aliases, or document why both variants are necessary.
| if (this._postCondenseFirstCallInFlight && retryAttempt === 0) { | ||
| // Duplicate external trigger detected - no-op for this call | ||
| try { | ||
| this.providerRef |
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.
Missing error handling: The optional chaining here could silently fail if this.providerRef.deref() returns undefined. Consider adding proper error handling to avoid silent failures.
| yield* this.handleStreamResponse(response, model) | ||
| } | ||
| // Per-call override: allow metadata to force stateless operation and suppression of continuity. | ||
| const forceStateless = metadata?.forceStateless === true || metadata?.suppressPreviousResponseId === true |
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.
Complex state management: The interaction between forceStateless, suppressPreviousResponseId, and skipPrevResponseIdOnce flags is complex and could benefit from clearer documentation or refactoring into a state machine pattern. Consider adding a comment block explaining the different scenarios and their expected behavior.
| const role = message.role === "user" ? "User" : "Assistant" | ||
| // Defensive guard: if prev-id is present, we should never send more than one input item. | ||
| if (Array.isArray(newMessages) && newMessages.length !== 1) { | ||
| } |
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.
This commented console.log should be removed before merging. If debugging output is needed, consider using a proper logging framework or removing it entirely.
| let resolvedEffort = (model as any).reasoningEffort as any | undefined | ||
| const isOSeries = typeof model.id === "string" && model.id.startsWith("o") | ||
| const supportsSummary = (model.info as any)?.supportsReasoningSummary === true | ||
| const reasoningCfg: any = {} |
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.
Type safety concern: Multiple as any type assertions reduce type safety. Consider defining proper types for the reasoning configuration to improve maintainability and catch potential errors at compile time.
| * Read provider state persisted for a specific task. | ||
| * Returns undefined if no state exists yet. | ||
| */ | ||
| export async function readOpenAiNativeState({ |
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.
Missing test coverage: This new provider state persistence functionality lacks test coverage. Consider adding unit tests to verify the read/write operations and error handling.
| // Prefetch encrypted reasoning artifacts for reasoning-capable models so we can fall back to stateless if needed. | ||
| // This does NOT change statefulness: we only send conversationHistory as input when stateless (store === false). | ||
| const id = String(model.id || "") | ||
| const supportsEncrypted = id.startsWith("gpt-5") || id.startsWith("o") |
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.
Magic string pattern: Consider extracting the "gpt-5" prefix check into a constant or utility function for better maintainability. This would make it easier to update model detection logic in the future.
| // - If GPT‑5 (OpenAI Native), include "minimal" | ||
| // - Otherwise hide "minimal" | ||
| // Also de‑dupe in case the source list already includes "minimal" | ||
| const baseEfforts = [...reasoningEfforts] as ReasoningEffortWithMinimal[] |
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.
Potential duplication: The logic for building availableReasoningEfforts could be simplified. The current implementation might include "minimal" twice if it's already in the source array. Consider using a Set to ensure uniqueness.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
b41762e to
ee87256
Compare
|
@roomote-agent Use switch_mode to switch to pr-fixer mode and solve the issues on this PR. Do not create a new one. |
|
Hi @hannesrudolph, I see your request. I will now work on fixing the issues in this PR:
I will update you once the fixes are complete. |
- Removed unused src/core/task-persistence/providerState.ts file (fixes knip issue) - Removed openAiNativeState entry from globalFileNames.ts - Provider state is already handled internally in OpenAiNativeHandler
- Fixed Codex Mini model tests to use mocked OpenAI SDK instead of fetch - Added proper beforeEach hooks to clear mocks between tests - Updated error handling test to match provider's error hygiene behavior
…ons and add error handling
b58ea3b to
1f5a182
Compare
|
Hi @hannesrudolph, I have successfully fixed all the issues on this PR! ✅ Summary of changes:
The PR is now ready for review and merging! |
Related GitHub Issue
Closes: #0
Roo Code Task Context (Optional)
No Roo Code task context for this PR
Description
This PR prepares a fork-based contribution and introduces updates aligned with OpenAI Node SDK changes, while wiring UI controls for reasoning budget and persisting provider state.
Key changes:
Touched files (high level):
Rationale and references:
Test Procedure
Run focused tests per workspace rules:
Backend:
Types package:
UI checks (manual):
Pre-Submission Checklist
Screenshots / Videos
No UI changes in this PR
Documentation Updates
Additional Notes
The following references from temp_docs.md motivated these updates (selection):
Get in Touch
@hannesrudolph
Important
This PR updates the OpenAI provider to support new models, state persistence, and UI enhancements, with expanded tests for these features.
openai-native.tsto support new OpenAI models and state persistence.providerState.tsfor persisting provider state per task.ApiOptions.tsxandThinkingBudget.tsxto expose new configuration options.openai.tswith new models and options.openai.models.spec.tsfor model invariants.openai-native.spec.tsfor new provider behavior.Task.spec.tsfor stateless override behavior.globalFileNames.tsto includeopenai_native_state.json.ChatRow.tsxandReasoningBlock.tsx.This description was created by
for b41762e. You can customize this summary. It will automatically update as commits are pushed.