feat(providers): add OpenAI Codex provider with Responses API support#128
feat(providers): add OpenAI Codex provider with Responses API support#128b3nw wants to merge 9 commits intoMirrowel:devfrom
Conversation
|
@mirrobot-agent gimme a review ya lazy bot |
|
@b3nw, I'm on it! I'll dive into the Codex provider implementation and give you a thorough review. 🕵️♂️ |
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
The OpenAI Codex provider implementation is exceptionally thorough and well-integrated into the existing rotator architecture. The move to the Responses API allows for advanced features like reasoning output and streaming tool calls, which are critical for agentic workflows. The OAuth implementation with PKCE is robust and handles headless environments elegantly.
Architectural Feedback
- Separation of Concerns: The use of mixins (
OpenAIOAuthBase,CodexQuotaTracker) keeps the mainCodexProviderclass focused on request/response translation. - Resilience: The token refresh logic and re-auth coordinator handle credential expiration gracefully.
- Quota Tracking: Using virtual models (
_5h_window,_weekly_window) is a clever way to expose Codex's unique rate limit windows to the TUI.
Key Suggestions
- Error Handling: Refine the generic exceptions in the streaming loop to include more context (status codes, etc.).
- Fragile Parsing: Move away from regex-based retry-after extraction if the API provides structured alternatives.
- Backend API usage: Double-check the stability/longevity of the
chatgpt.com/backend-apiendpoints as they appear to be internal.
Questions for the Author
- GPT-5 Models: Are the
gpt-5model names intended as placeholders or are they targeting a specific internal preview? - System Message Conversion: Is the conversion of system messages to user messages a strict requirement of the Responses API, or a strategy to bypass the fixed
instructionsfield?
This review was generated by an AI assistant.
| error_message = error.get("message", "Response failed") | ||
|
|
||
| if error_message: | ||
| raise Exception(f"Codex response failed: {error_message}") |
There was a problem hiding this comment.
Consider using a more specific exception type here, or including the HTTP status code if available, to help with debugging and error handling downstream.
|
|
||
| # Try to extract from message | ||
| import re | ||
| match = re.search(r"try again in (\d+)s", message) |
There was a problem hiding this comment.
Parsing rate limit retry times from strings can be fragile. If the API provides a Retry-After header or a structured field in the JSON error response, those would be safer to use.
| # - If base contains /backend-api: use /wham/usage (ChatGptApi style) | ||
| # - Otherwise: use /api/codex/usage (CodexApi style) | ||
| # Since we use chatgpt.com/backend-api, we need /wham/usage | ||
| CODEX_USAGE_URL = "https://chatgpt.com/backend-api/wham/usage" |
There was a problem hiding this comment.
This looks like a internal backend API endpoint. Ensure that the proxy is intended to hit this directly and that there are no public-facing alternatives that might be more stable.
| if item.get("type") == "function_call": | ||
| call_id = item.get("call_id") or item.get("id", "") | ||
| name = item.get("name", "") | ||
| arguments = item.get("arguments", "") |
There was a problem hiding this comment.
User-provided system messages are converted to user messages here. While this ensures compatibility with the Responses API's strict instruction field, it might affect models that rely heavily on the system role for personality/constraints. It's worth confirming if this is the intended behavior for all Codex-style models.
|
@b3nw, I've finished the review! Overall, it's a very solid implementation. I've left some detailed feedback and a few questions in the formal review. Let me know if you want me to look into anything else! 🚀 |
Mirrowel
added
enhancement
|
I'm starting my review of the OpenAI Codex provider integration. I see some new retry logic for handling garbled tool calls in the Responses API - checking the implementation details now! |
There was a problem hiding this comment.
Follow-up Review: Codex Provider & Responses API
The implementation of the OpenAI Codex provider is very thorough and the integration with the Responses API is well-handled. The latest additions introducing retry logic for garbled tool calls are a welcome step toward production-level reliability.
Assessment of New Changes:
- Retry Logic: The introduction of
_stream_with_retryand_non_stream_with_retrycorrectly addresses the intermittent "ChatGPT internal format" reversion issue. - Marker Detection: The use of the
GARBLED_TOOL_CALL_MARKERis a practical heuristic for detecting when the model has entered an invalid state.
Key Suggestions:
- Streaming Duplication (High Priority): As noted in the inline comment, retrying a stream after yielding chunks will cause duplication of the initial content (like reasoning tokens) in the client's view. This should be addressed by buffering the start of the stream or implementing a "reset" signal for the client if possible.
- Fragmented Marker Detection: The current check might miss markers split across two network packets/chunks.
- Cleanup: There is a minor unreachable code path in the non-streaming retry wrapper.
Note on Previous Feedback:
The previous suggestions regarding specific exception types (line 1186), fragile regex-based retry extraction (line 1242), and the stability of internal backend API endpoints still apply and should be considered for future hardening.
This review was generated by an AI assistant.
| for attempt in range(GARBLED_TOOL_CALL_MAX_RETRIES): | ||
| garbled_detected = False | ||
|
|
||
| try: | ||
| async for chunk in self._stream_response( | ||
| client, headers, payload, model, reasoning_compat, credential_path | ||
| ): | ||
| # Inspect content deltas for garbled tool call marker | ||
| if hasattr(chunk, "choices") and chunk.choices: | ||
| choice = chunk.choices[0] | ||
| delta = getattr(choice, "delta", None) | ||
| if delta: | ||
| content = getattr(delta, "content", None) or "" | ||
| if GARBLED_TOOL_CALL_MARKER in content: | ||
| garbled_detected = True | ||
| lib_logger.warning( | ||
| f"[Codex] Garbled tool call detected in stream for {model}, " | ||
| f"attempt {attempt + 1}/{GARBLED_TOOL_CALL_MAX_RETRIES}. " | ||
| f"Content snippet: {content[:100]!r}" | ||
| ) | ||
| break # Stop consuming this stream | ||
|
|
||
| yield chunk | ||
|
|
||
| if not garbled_detected: | ||
| return # Stream completed successfully | ||
|
|
||
| except Exception: | ||
| if garbled_detected: | ||
| # Exception during stream teardown after garble detected - continue to retry | ||
| pass | ||
| else: | ||
| raise # Non-garble exception - propagate | ||
|
|
||
| # Garbled stream detected - retry if we have attempts left | ||
| if attempt < GARBLED_TOOL_CALL_MAX_RETRIES - 1: | ||
| lib_logger.info( | ||
| f"[Codex] Retrying request for {model} after garbled tool call " | ||
| f"(attempt {attempt + 2}/{GARBLED_TOOL_CALL_MAX_RETRIES})" | ||
| ) | ||
| await asyncio.sleep(GARBLED_TOOL_CALL_RETRY_DELAY) | ||
| else: | ||
| lib_logger.error( | ||
| f"[Codex] Garbled tool call persisted after {GARBLED_TOOL_CALL_MAX_RETRIES} " | ||
| f"attempts for {model}. Giving up." | ||
| ) | ||
| # Final attempt already yielded whatever chunks it had | ||
| return |
There was a problem hiding this comment.
There is a significant duplication risk in the streaming retry logic. Since yield chunk occurs inside the same loop that detects the garbled marker, any chunks yielded before the marker is found (e.g., reasoning tokens or early valid text) will be sent to the client. When the request is retried, the client will receive those same chunks again from the new stream, leading to duplicated content in the UI.
Consider buffering the early chunks or only retrying if no content has been yielded yet. If retrying mid-stream is unavoidable, the client should be notified to clear the previous partial response.
| delta = getattr(choice, "delta", None) | ||
| if delta: | ||
| content = getattr(delta, "content", None) or "" | ||
| if GARBLED_TOOL_CALL_MARKER in content: |
There was a problem hiding this comment.
Checking for GARBLED_TOOL_CALL_MARKER in individual chunks may fail if the marker (+#+#) is split across two consecutive chunks (e.g., +# followed by +#).
Consider maintaining a small rolling buffer of the last few characters to ensure the marker is detected even when fragmented.
| return response | ||
|
|
||
| # Should not reach here, but return the last response as fallback | ||
| return response |
There was a problem hiding this comment.
This line is unreachable because the return response at line 805 is inside the loop and covers the final iteration fallback.
b3nw
force-pushed
the
feature/codex-all
branch
from
41085fe to
f3739c2
Compare
There was a problem hiding this comment.
Follow-up Review: Anthropic & Cursor Integration
The PR continues to expand with robust implementations for Anthropic (OAuth + API key) and Cursor providers. The new Log Viewer TUI is also a great addition for observability.
Assessment of New Changes:
- Anthropic Provider: The dual routing (OAuth/API key) is well-handled. The interleaved thinking support is correctly implemented.
- Cursor Provider: Solid integration with the sidecar and clever usage of a virtual model for quota tracking.
- Log Viewer: A very functional addition. The lazy loading of metadata and prompts is a good choice for performance.
- Request Transforms: Centralized transforms for Kimi and GLM-5/4 help with model-specific quirks.
Note on Previous Feedback:
The critical issues raised in the previous review for src/rotator_library/providers/codex_provider.py (specifically the Streaming Duplication risk on line 760 and the Fragmented Marker detection on line 726) still apply as the file has not been modified in the latest commits. These should be addressed to ensure reliability in production scenarios.
This review was generated by an AI assistant.
| ANTHROPIC_MESSAGES_ENDPOINT = f"{ANTHROPIC_API_BASE}/v1/messages" | ||
|
|
||
| # Required headers for OAuth requests | ||
| ANTHROPIC_BETA_HEADER = "oauth-2025-04-20,interleaved-thinking-2025-05-14" |
There was a problem hiding this comment.
The date 2025-05-14 in the ANTHROPIC_BETA_HEADER seems very far in the future. Is this a placeholder or a requirement for a specific private beta?
| ANTHROPIC_USER_AGENT = "claude-cli/2.1.2 (external, cli)" | ||
|
|
||
| # Tool name prefix for OAuth path | ||
| TOOL_PREFIX = "mcp_" |
There was a problem hiding this comment.
Why is the TOOL_PREFIX set to mcp_? If this refers to "Model Context Protocol", it might be worth adding a comment explaining the choice, as it might affect client expectations for tool naming.
src/proxy_app/log_viewer.py
Outdated
| from rich.syntax import Syntax | ||
|
|
||
|
|
||
| def _get_logs_dir() -> Path: |
There was a problem hiding this comment.
This function re-implements logic already available in src/rotator_library/utils/paths.py. It is better to import get_logs_dir from there to ensure consistency and avoid duplication.
b3nw
force-pushed
the
feature/codex-all
branch
from
f3739c2 to
8b094e3
Compare
There was a problem hiding this comment.
Follow-up Review: Codex Garble Handling Refinements
The latest updates to the Codex provider significantly harden the garbled tool call detection and retry logic. The move to a buffer-then-flush strategy for streaming is a particularly robust fix for the duplication risks previously identified.
Assessment of New Changes:
- Robust Detection: Expanding
GARBLED_TOOL_CALL_MARKERSand implementingaccumulated_texttracking ensures that garbled output is caught even when markers are split across chunks. - Streaming Reliability: The buffer-then-flush approach in
_stream_with_retryis a great solution. While it adds latency to the initial chunk, it guarantees that no "corrupted" or duplicated content reaches the client. - Diagnostics: The new logging for
response.completedwill be very helpful for tracking down why the model sometimes reverts to ChatML-style tool calls.
Note on Previous Feedback:
- Resolved: The high-priority issue regarding Streaming Duplication and the concern about Fragmented Markers have been successfully addressed.
- Still Applies: The minor unreachable code path in
_non_stream_with_retry(line 863) still exists. Additionally, the previous suggestions regarding specific exception types (line 1255 ofcodex_provider.pyin the full file) and regex-based retry extraction in the quota tracker remain relevant for future hardening.
This review was generated by an AI assistant.
| return response | ||
|
|
||
| # Should not reach here, but return the last response as fallback | ||
| return response |
There was a problem hiding this comment.
This line remains unreachable because the return response at line 860 covers all exit paths from the loop. While minor, removing it would clean up the implementation.
| Uses a buffer-then-flush approach: all chunks are collected first, | ||
| then checked for the garbled marker. Only if the stream is clean | ||
| are chunks yielded to the caller. This allows true retry since | ||
| no chunks have been sent to the HTTP client yet. | ||
|
|
||
| Detection is done both per-chunk (for early abort) AND on the | ||
| accumulated text after stream completion (to catch markers that | ||
| are split across multiple SSE chunks). | ||
| """ | ||
| for attempt in range(GARBLED_TOOL_CALL_MAX_RETRIES): | ||
| garbled_detected = False | ||
| buffered_chunks: list = [] | ||
| accumulated_text = "" # Track all text content across chunks | ||
|
|
||
| try: | ||
| async for chunk in self._stream_response( |
There was a problem hiding this comment.
The buffer-then-flush strategy effectively addresses the streaming duplication risk identified in the previous review. Note that this does mean real-time streaming is disabled for the client as they must wait for the entire response to be generated before any chunks are yielded. Given the intermittent nature of the Responses API garbling, this seems like the correct trade-off for reliability.
| buffered_chunks.append(chunk) | ||
|
|
||
| # Post-stream check: inspect accumulated text for markers split across chunks | ||
| if not garbled_detected and _is_garbled_tool_call(accumulated_text): |
There was a problem hiding this comment.
Using accumulated_text for a post-stream check is a great addition to catch garble markers that might be split across network packets. This addresses the fragmented marker concern from the previous review.
b3nw
force-pushed
the
feature/codex-all
branch
from
8b094e3 to
64c0a37
Compare
There was a problem hiding this comment.
Follow-up Review
This update focuses on cleaning up diagnostic logging within the Codex provider's streaming logic.
Assessment of New Changes:
- Log Cleanup: The removal of the verbose
lib_logger.infoblock in theresponse.completedhandler is a sensible refinement now that the Responses API integration is stable.
Overall Status:
The implementation remains robust. The core logic for handling garbled tool calls and the buffer-then-flush streaming strategy are well-executed.
Note on Previous Feedback:
The following items from previous reviews remain unaddressed:
codex_provider.py: The unreachable code path at line 863 and the use of generic exceptions at lines 1123 and 1242.codex_quota_tracker.py: The recommendation to favor structured retry-after data over regex parsing where the API allows.anthropic_provider.py: The questions regarding the future-dated beta header (2025-05-14) and the choice of themcp_tool prefix.log_viewer.py: The suggestion to use the centralizedget_logs_dirutility for path consistency.
Great work on the cleanup!
This review was generated by an AI assistant.
b3nw
force-pushed
the
feature/codex-all
branch
from
64c0a37 to
73b27bf
Compare
|
@mirrobot-agent major cleanup and fixed a stall issue around instructions. |
There was a problem hiding this comment.
Follow-up Review: Codex Implementation Refined
The latest changes significantly harden the OpenAI Codex provider. The refactoring of instruction handling and the introduction of a robust streaming retry strategy address the most critical stability concerns raised in previous reviews.
Assessment of New Changes:
- Retry Logic: The
buffer-then-flushapproach in_stream_with_retrysuccessfully eliminates the risk of yielding duplicated or garbled content to the client. - Instruction Handling: Utilizing the
instructionsfield of the Responses API directly (with proper fallbacks) is a much cleaner approach than converting system messages to user messages and appears to have resolved the stall issues. - Error Handling: Replacing generic
Exceptioncalls withStreamedAPIErrorallows the rotator library to correctly classify and handle model-generated errors. - Cleanup: Unreachable code paths have been removed, and the overall provider logic is more concise.
Key Suggestions:
- Regex Parsing (Minor): The
parse_quota_errormethod still relies on regex forretry-afterextraction. While functional, favoring headers (if the Responses API eventually provides them) would be more robust. - Scope Clarification: I noticed that the Anthropic and Cursor providers, along with the Log Viewer, were deleted in this update. I assume this is a deliberate move to narrow the PR scope to Codex only, but please confirm if these were intended to be removed.
Overall Status:
The Codex implementation is now very stable and follows project conventions well. The addition of the bot management infrastructure also looks like a great step forward for the repository's maintenance.
This review was generated by an AI assistant.
| accumulated text after stream completion (to catch markers that | ||
| are split across multiple SSE chunks). | ||
| """ | ||
| for attempt in range(GARBLED_TOOL_CALL_MAX_RETRIES): |
There was a problem hiding this comment.
The buffer-then-flush strategy in _stream_with_retry is a robust solution to the streaming duplication risk. It ensures that no garbled content is yielded to the client before detection is complete.
| # The Codex Responses API requires the 'instructions' field — it's non-optional. | ||
| # Always include it; fall back to the Codex system instruction if nothing else. | ||
| if not instructions: | ||
| instructions = CODEX_SYSTEM_INSTRUCTION |
There was a problem hiding this comment.
The refactored instructions handling properly utilizes the Responses API field and includes a fallback to CODEX_SYSTEM_INSTRUCTION. This should effectively address the stall issues previously identified.
| error = evt.get("response", {}).get("error", {}) | ||
| error_msg = error.get("message", "Response failed") | ||
| lib_logger.error(f"Codex response failed: {error_msg}") | ||
| raise StreamedAPIError(f"Codex response failed: {error_msg}") |
There was a problem hiding this comment.
Thanks for replacing generic exceptions with StreamedAPIError. This allows for more granular error classification in the rotator.
|
|
||
| # Try to extract from message | ||
| import re | ||
| match = re.search(r"try again in (\d+)s", message) |
There was a problem hiding this comment.
This regex-based extraction still exists. While acceptable if the API doesn't provide structured retry data, consider using headers if available in the future.
b3nw
force-pushed
the
feature/codex-all
branch
from
b5e5d63 to
aaa59f3
Compare
Replace hardcoded BASE_MODELS and REASONING_MODEL_EFFORTS with dynamic fetching from the Codex GitHub repo's models.json at runtime. - Fetch models from raw.githubusercontent.com/openai/codex on first access - Cache for 1 hour (configurable via CODEX_MODELS_CACHE_TTL env var) - Fall back to stale cache on fetch failure, then to hardcoded defaults - Dynamic _allowed_efforts_for_model() with prefix matching for aliases - Adds gpt-5.4, gpt-5-codex-mini, gpt-oss-120b, gpt-oss-20b automatically - URL configurable via CODEX_MODELS_JSON_URL env var
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dynamically discovered models from GitHub models.json (like gpt-5.4) were not in the static AVAILABLE_MODELS list, causing get_model_quota_group() to return None. This made the executor's quota display show '?/?' instead of actual values. Changes: - Override get_model_quota_group() to return 'codex-global' for any model, since all Codex models share the same per-account rate limits (same pattern as FirmwareProvider) - Push primary window quota data to 'codex-global' group in addition to '5h-limit' so the executor's _get_quota_display() can find limits when looking up the model's quota group
There was a problem hiding this comment.
Follow-up Review: Codex Provider Cleanup
Assessment of New Changes:
The latest changes focus on cleanup and minor refinements to the Codex provider implementation. The addition of get_model_quota_group properly integrates dynamic model discovery with the quota tracking system.
Key Observations:
- Quota Group Integration: The new
get_model_quota_groupmethod ensures that dynamically discovered Codex models (from GitHub's models.json) are properly tracked under the 'codex-global' quota group. - Workflow Cleanup: Several legacy workflow files and prompts have been removed, streamlining the repository.
- Build Output Simplification: The release notes format has been simplified for better readability.
Overall Status:
The implementation remains stable. The quota group method is a sensible addition for supporting dynamic model discovery.
Note on Previous Feedback:
All critical issues from previous reviews (streaming duplication, fragmented marker detection, instruction handling) have been successfully addressed. The codebase is in good shape.
This review was generated by an AI assistant.
| """This provider uses custom logic (Responses API instead of litellm).""" | ||
| return True | ||
|
|
||
| def get_model_quota_group(self, model: str) -> Optional[str]: |
There was a problem hiding this comment.
This get_model_quota_group method returns a hardcoded string for all models. Consider adding a comment explaining why this is safe (e.g., "All Codex models share the same per-account rate limits"), or add validation if this method might be called with non-Codex models.
- Add build_env_lines() and export_credential_to_env() to OpenAIOAuthBase with Codex-specific fields (api_key, account_id, id_token) - Add build_env_lines() and export_credential_to_env() to AnthropicOAuthBase with proper claudeAiOauth nested format handling - Add delete_credential() to OpenAIOAuthBase for parity with other providers - Add export_codex_to_env() and export_anthropic_to_env() functions to credential_tool.py - Update export_credentials_submenu() to include Codex and Anthropic in individual, bulk, and combine export options - Update combine_all_credentials() to include codex and anthropic providers - Update credential_manager.py to discover Codex API_KEY-only credentials (CODEX_N_API_KEY without requiring REFRESH_TOKEN) - Fix anthropic list_credentials() to properly parse claudeAiOauth format
There was a problem hiding this comment.
Follow-up Review: Credential Export & Management Enhancements
Assessment of New Changes:
The latest commit adds comprehensive credential export functionality for Codex and Anthropic OAuth providers, along with improved credential detection logic. The changes enable users to export OAuth credentials to .env format and combine multiple credentials, which is a valuable addition for deployment scenarios.
Key Observations:
- Credential Detection: The
credential_manager.pychanges properly handle Codex's unique ability to work with API_KEY-only credentials (exchanged from OAuth tokens) - Export Functions: The new
export_codex_to_env()andexport_anthropic_to_env()functions follow the established pattern from other providers - Base Class Methods: The
build_env_lines()andexport_credential_to_env()methods are well-implemented and handle both flat and nested credential formats - Consistency: The implementation maintains consistency with existing OAuth providers (Gemini CLI, Qwen Code, iFlow, Antigravity)
Overall Status:
Solid implementation that brings Codex and Anthropic credential management in line with other OAuth providers. The code is clean and follows project conventions.
Minor Suggestions:
- Adding brief clarifying comments about Codex's unique API_KEY-only credential support would help future maintainers (see inline comments)
This review was generated by an AI assistant.
| if refresh_key in self.env_vars and self.env_vars[refresh_key]: | ||
| found_indices.add(index) | ||
|
|
||
| # For Codex provider, also check for API_KEY-only credentials |
There was a problem hiding this comment.
Consider expanding this comment slightly to explain why Codex is unique in supporting API_KEY-only credentials. A note like 'Codex exchanges OAuth tokens for persistent API keys, unlike other OAuth providers' would help future maintainers understand this is Codex-specific behavior.
| # Use "0" as the index for legacy single credential | ||
| found_indices.add("0") | ||
|
|
||
| # For Codex, also accept legacy API_KEY-only format |
There was a problem hiding this comment.
Same suggestion: adding a brief comment explaining that this legacy API_KEY-only format is specific to Codex's token exchange mechanism would clarify why other OAuth providers don't have this fallback.
There was a problem hiding this comment.
Overall Assessment
This is a focused, high-impact fix that addresses a real-world scenario: the same email authenticating to multiple OpenAI workspaces/organizations. The addition of account_id matching prevents credential file collisions and the improved logging makes the credential lifecycle much clearer.
Architectural Feedback
The approach is sound and fits well within the existing OAuth credential management pattern. The change is minimal and targeted, which reduces the risk of introducing regressions.
Key Suggestions
- Edge Case Handling: Consider the scenario where
account_idmight be missing from some existing credential files. The current strict matching could lead to unexpected behavior if one file hasaccount_idand another doesn't (both for the same email). - Code Duplication: The
account_idextraction appears in multiple places. Consolidating this would improve maintainability.
Questions for the Author
- Missing account_id: What should happen when the OAuth response doesn't include an
account_id? Should the system fall back to email-only matching, or treat it as a separate account?
This review was generated by an AI assistant.
| def _find_existing_credential_by_email( | ||
| self, | ||
| email: str, | ||
| base_dir: Optional[Path] = None, | ||
| account_id: Optional[str] = None, | ||
| ) -> Optional[Path]: | ||
| """Find an existing credential file for the given email and account. | ||
|
|
||
| When ``account_id`` is provided the match requires **both** the email | ||
| and the account_id to be equal. This prevents credentials for the same | ||
| email on different OpenAI workspaces / organisations from colliding. | ||
| """ | ||
| if base_dir is None: | ||
| base_dir = self._get_oauth_base_dir() | ||
|
|
||
| prefix = self._get_provider_file_prefix() | ||
| pattern = str(base_dir / f"{prefix}_oauth_*.json") | ||
|
|
||
| for cred_file in glob(pattern): | ||
| try: | ||
| with open(cred_file, "r") as f: | ||
| creds = json.load(f) | ||
| metadata = creds.get("_proxy_metadata", {}) | ||
| existing_email = metadata.get("email") | ||
| if existing_email != email: | ||
| continue | ||
|
|
||
| # If an account_id was supplied, require it to match as well | ||
| if account_id is not None: | ||
| existing_account_id = ( | ||
| creds.get("account_id") | ||
| or metadata.get("account_id") | ||
| ) | ||
| if existing_account_id != account_id: | ||
| continue | ||
|
|
||
| return Path(cred_file) | ||
| except Exception: | ||
| continue | ||
|
|
||
| return None | ||
|
|
||
| def _get_next_credential_number(self, base_dir: Optional[Path] = None) -> int: |
There was a problem hiding this comment.
The addition of account_id matching is a solid fix for preventing credential collisions across workspaces. However, consider what happens when account_id is None for one credential file but present for another with the same email - the current logic would treat them as different credentials. Is this the intended behavior, or should there be a fallback to match by email only when account_id is unavailable?
| success=False, error="Could not retrieve email from OAuth response" | ||
| ) | ||
|
|
||
| # Extract account_id so we can scope the duplicate check to the | ||
| # specific workspace / organisation. Without this, the same email | ||
| # authenticating to two different workspaces would overwrite the | ||
| # first credential file. | ||
| account_id = new_creds.get("account_id") or new_creds.get( | ||
| "_proxy_metadata", {} | ||
| ).get("account_id") | ||
|
|
||
| existing_path = self._find_existing_credential_by_email( | ||
| email, base_dir, account_id=account_id | ||
| ) | ||
| is_update = existing_path is not None | ||
|
|
||
| if is_update: | ||
| file_path = existing_path | ||
| lib_logger.info( | ||
| f"Updating existing credential at '{Path(file_path).name}' " | ||
| f"for {email} (account {account_id})" | ||
| ) | ||
| else: | ||
| file_path = self._build_credential_path(base_dir) | ||
| lib_logger.info( | ||
| f"Creating new credential at '{Path(file_path).name}' " | ||
| f"for {email} (account {account_id})" | ||
| ) | ||
|
|
||
| await self._save_credentials(str(file_path), new_creds) | ||
|
|
||
| return CredentialSetupResult( | ||
| success=True, | ||
| file_path=str(file_path), |
There was a problem hiding this comment.
The logging improvements are helpful, but the account_id extraction logic is duplicated. Consider extracting this to a helper method or computing it once before the conditional branch to avoid redundancy.
There was a problem hiding this comment.
Follow-up Review: Workspace & Account Display Enhancements
Assessment of New Changes:
The additions to display workspace title, plan type, and account ID in the credential setup flow are valuable improvements for user transparency. The enhanced organization extraction logic from JWT claims is well-implemented and handles the common case of finding the default organization correctly.
Key Suggestions:
- Edge Case Handling: The organization fallback logic could be more defensive against malformed JWT claims (see inline comment).
Overall Status:
Solid incremental improvements that enhance the user experience when managing multiple OpenAI workspaces. The changes follow existing patterns and integrate cleanly with the Codex credential flow.
Note on Previous Feedback:
All critical issues from previous reviews have been addressed. This PR is in good shape.
This review was generated by an AI assistant.
| api_key = None | ||
| if org_id and project_id: | ||
| try: | ||
| api_key = await self._exchange_for_api_key( | ||
| client, token_data.get("id_token", "") | ||
| ) | ||
| new_creds["api_key"] = api_key | ||
| except Exception as e: | ||
| lib_logger.warning(f"API key exchange failed: {e}") | ||
|
|
||
| new_creds["_proxy_metadata"] = { | ||
| "email": email, | ||
| "account_id": account_id, | ||
| "org_id": org_id, | ||
| "project_id": project_id, | ||
| "plan_type": plan_type, | ||
| "workspace_title": workspace_title, |
There was a problem hiding this comment.
The logic for finding the default organization is good, but consider adding a safeguard for edge cases. If organizations exists but none have is_default=True and the first item is not a dict, line 855 could raise an IndexError or TypeError.
| api_key = None | |
| if org_id and project_id: | |
| try: | |
| api_key = await self._exchange_for_api_key( | |
| client, token_data.get("id_token", "") | |
| ) | |
| new_creds["api_key"] = api_key | |
| except Exception as e: | |
| lib_logger.warning(f"API key exchange failed: {e}") | |
| new_creds["_proxy_metadata"] = { | |
| "email": email, | |
| "account_id": account_id, | |
| "org_id": org_id, | |
| "project_id": project_id, | |
| "plan_type": plan_type, | |
| "workspace_title": workspace_title, | |
| if not workspace_title and organizations and isinstance(organizations[0], dict): | |
| workspace_title = organizations[0].get("title", "") |
There was a problem hiding this comment.
Follow-up Review: Workspace & Plan Display Enhancement
Assessment of New Changes:
- TUI Enhancement: The addition of workspace, plan, and account ID columns to the Codex credential display is a valuable UX improvement. Users can now quickly identify which workspace/organization a credential belongs to.
- Metadata Completeness: Including
plan_typeandworkspace_titlein the credential metadata ensures these fields are available for display and future features. - Display Truncation: The account ID truncation logic (showing first 8 chars + "...") is a sensible choice for readability.
Overall Status:
This is a solid incremental improvement that enhances transparency in the credential management flow. The implementation follows existing patterns from other providers (gemini_cli, antigravity).
Note on Previous Feedback:
- Resolved: The workspace/plan display functionality has been successfully implemented.
- Still Applies: The minor safeguard suggestion for the organization fallback logic (line 836) remains unaddressed. While the edge case is unlikely (malformed JWT claims), adding the defensive check would improve robustness.
The PR continues to be in good shape and the Codex implementation is stable.
This review was generated by an AI assistant.
| break | ||
| if not workspace_title and isinstance(organizations[0], dict): | ||
| workspace_title = organizations[0].get("title", "") | ||
|
|
There was a problem hiding this comment.
The previous suggestion about adding a safeguard for edge cases still applies here. If organizations exists but the first item is not a dict, this could raise a TypeError.
| if not workspace_title and organizations and isinstance(organizations[0], dict): | |
| workspace_title = organizations[0].get("title", "") |
|
I'm starting my follow-up review of the Codex credential display enhancements. Checking the TUI changes now! |
There was a problem hiding this comment.
Follow-up Review: Codex Credential Display Enhancement
Assessment of New Changes:
The addition of workspace, plan, and account ID columns to the Codex credential detail view is a solid UX improvement. Users can now quickly distinguish between credentials from different OpenAI workspaces/organizations, which is especially valuable given the multi-tenant nature of Codex access.
Key Observations:
- Implementation Quality: The changes follow the established pattern from other OAuth providers (gemini_cli, antigravity)
- Backward Compatibility: The use of
.get()with fallbacks handles older credential files correctly - Display Truncation: Account ID truncation (8 chars + "...") maintains readability
Minor Suggestions:
- Add brief comments noting backward compatibility for future maintainers (see inline comments)
Overall Status:
This is a clean, focused improvement that enhances transparency in credential management. The implementation is sound and follows project conventions.
Note on Previous Feedback:
All critical issues from previous reviews have been addressed. The one remaining minor suggestion from the last review (organization fallback safeguard in openai_oauth_base.py:836) is unrelated to these changes and can be addressed separately.
This review was generated by an AI assistant.
| table.add_column("Plan", style="magenta") | ||
| table.add_column("Account ID", style="dim") | ||
| # Add type column for iFlow (OAuth vs Cookie) | ||
| elif provider_name == "iflow": |
There was a problem hiding this comment.
The workspace/plan/account columns are added here for Codex. Consider adding a comment noting backward compatibility - older credential files created before these fields were added will show '-' which is fine, but future maintainers might wonder why.
| if project and len(project) > 25: | ||
| project = project[:22] + "..." | ||
| table.add_row(str(i), file_name, email, tier, project or "-") | ||
| elif provider_name == "codex": |
There was a problem hiding this comment.
Same here - the .get() fallbacks handle missing fields correctly. A brief comment like # Backward compatible: older credentials may lack workspace/plan fields would help future maintainers.
| elif provider_name == "codex": | |
| elif provider_name == "codex": | |
| # Backward compatible: older credentials may lack workspace/plan fields | |
| workspace = cred.get("workspace_title", "-") or "-" |
|
I'm starting my follow-up review of the Qwen Code provider model additions. Let me check what's changed! |
There was a problem hiding this comment.
Follow-up Review: Qwen Code Model Addition
Assessment of New Changes:
- Model Addition: The addition of
coder-model(Qwen 3.5 Plus) is a simple, low-risk change that expands available model options. - Documentation: The expanded inline comment helpfully clarifies what each model represents.
Overall Status:
This is a minor, focused change that follows existing patterns. The implementation is sound.
This review was generated by an AI assistant.
|
Minor suggestion for (line 30): Adding to the hardcoded list is straightforward, but consider adding a comment clarifying that this is an alias for Qwen 3.5 Plus, since the naming convention differs from the other models (not format). |
|
Minor suggestion for Adding |
…fferent workspaces The _find_existing_credential_by_email() method was matching credentials solely by email address. When the same email authenticates to two different OpenAI workspaces/accounts (each with a different account_id), the second authentication would overwrite the first credential file instead of creating a new one. Changes: - _find_existing_credential_by_email() now accepts an optional account_id parameter. When provided, both email AND account_id must match for a credential to be considered a duplicate. - setup_credential() extracts the account_id from the new OAuth response and passes it to the duplicate check. - Extract workspace/organization title from JWT claims and persist in credential metadata. - Display workspace title, plan type, and account_id in the credential setup success panel and credential listing tables.
|
@b3nw will this get merge, or you're all out of mana by now? |
3 participants
Add full Codex provider implementation with OAuth-based authentication, Responses API integration, and quota tracking.
New files:
Integration points:
Important
Adds OpenAI Codex provider with OAuth authentication, Responses API support, and quota tracking.
CodexProviderincodex_provider.pyfor OpenAI Codex models with OAuth authentication and Responses API.CodexQuotaTracker.openai_oauth_base.pyfor token management.codex_quota_tracker.pyfor tracking rate limits and credits.UsageManager.provider_factory.pyand updates credential management.quota_viewer.pyto skip limitless quota groups in views.This description was created by
for 993793c. You can customize this summary. It will automatically update as commits are pushed.