Conversation
- Add _extract_explicit_email_from_payload to prefer email claim over sub - Prefer id_token explicit email over access_token sub for metadata - Harden _find_existing_credential_by_identity to require both email and account_id to match when both are available (prevents workspace collisions) - Fall back to single-field matching only when one side is missing - Add tests for identity matching edge cases and setup_credential flows
gpt-5.3-codex returns 400 for max_output_tokens parameter. Omit it and let the API use its default.
|
Starting my review of this comprehensive OpenAI Codex OAuth multi-account support PR. With 27 files changed and over 5,500 lines added, this is a substantial feature addition. I'll be diving into the auth base implementation, provider integration, credential manager updates, and the new test suite. Will report back with detailed findings shortly. |
Mirrowel
added
enhancement
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This is a comprehensive and well-structured PR that adds OpenAI Codex OAuth multi-account support to the LLM-API-Key-Proxy. The implementation follows existing patterns in the codebase and includes:
- Complete OAuth Authorization Code + PKCE flow implementation
- File-based and environment variable credential support
- First-run import from Codex CLI credentials
- SSE translation for streaming responses
- Comprehensive test suite (994 lines of tests across 5 test files)
- Updated documentation (README, DOCUMENTATION.md, .env.example)
The code quality is generally high with good error handling, logging, and follows the existing architecture. The PR demonstrates solid engineering practices with proper separation of concerns between auth base, provider, and credential management.
Key Strengths
- Protocol Implementation: The SSE event translation (
CodexSSETranslator) is thorough and handles multiple event types with proper fallback chains - Testing: Good test coverage with 379 lines dedicated to auth tests alone, including JWT decoding, expiry logic, and env loading
- Documentation: All user-facing documentation has been updated consistently
- Integration: Clean integration with existing credential tool, settings UI, and launcher TUI
Suggestions for Improvement
-
Security: The hardcoded OAuth client ID
app_EMoamEEZ73f0CkXaXp7hranninopenai_codex_auth_base.pyshould be confirmed as intentionally public (it's a public client ID by design for OAuth flows, but should be documented) -
Async Task Handling: The
_refresh_tokenmethod creates fire-and-forget tasks withasyncio.create_task()for re-auth queueing. These tasks are not awaited or tracked, which could lead to unhandled exceptions being silently dropped. -
Resource Management: Consider reusing
httpx.AsyncClientinstances rather than creating new ones per refresh attempt for better performance. -
Code Duplication: JWT decoding logic is duplicated between
credential_manager.pyandopenai_codex_auth_base.py- consider extracting to a shared utility. -
Error Parsing: The
parse_quota_errormethod uses substring matching that could have false positives - consider more specific patterns. -
TODOs: Two TODOs remain regarding quota window tuning (lines 376, 382 in provider) - acceptable for initial implementation but should be tracked.
Minor Notes
- Architecture: The nested async generators in
acompletioncould be flattened for better readability - The deduplication logic in import methods could be refactored to reduce duplication
- Consider adding stress tests for queue management concurrency paths
This PR is ready for merge. The suggestions above are improvements rather than blockers. Great work on this substantial feature addition!
This review was generated by an AI assistant.
Remove non-existent gpt-4.1-codex and add all current models: gpt-5.3-codex, gpt-5.3-codex-spark, gpt-5.2-codex, gpt-5.2, gpt-5.1-codex-max, gpt-5.1-codex-mini
There was a problem hiding this comment.
Follow-up Review
Assessment of New Changes:
The model list update in openai_codex_provider.py looks good. Adding the newer GPT-5.x model variants (5.3, 5.2, 5.1 codex flavors) while removing the older gpt-4.1-codex keeps the hardcoded fallback list current and relevant.
Verification of Previous Feedback:
From my initial review, the following items were noted:
- ✅ Hardcoded OAuth client ID - confirmed as intentionally public
⚠️ Async task handling with fire-and-forget tasks - still applies⚠️ httpx.AsyncClient reuse - still applies⚠️ JWT decoding logic duplication - still applies⚠️ Error parsing substring matching - still applies⚠️ TODOs for quota window tuning - still present
These previous suggestions remain as potential improvements but don't block merging.
Overall Status:
This incremental update to the model list is clean and appropriate. The PR continues to be ready for merge.
This review was generated by an AI assistant.
Not generally available — requires special account access. Can still be used via OPENAI_CODEX_MODELS env or dynamic discovery.
There was a problem hiding this comment.
Follow-up Review
Assessment of New Changes:
The removal of gpt-5.3-codex-spark from the hardcoded model list is a clean cleanup. This keeps the model list focused on actively supported variants.
Verification of Previous Feedback:
The following items from my initial review remain applicable:
- Async task handling with fire-and-forget tasks - still applies
- httpx.AsyncClient reuse opportunity - still applies
- JWT decoding logic duplication - still applies
- Error parsing substring matching - still applies
- TODOs for quota window tuning - still present
These are non-blocking improvement suggestions for future consideration.
Overall Status:
This incremental cleanup is appropriate. The PR remains ready for merge.
This review was generated by an AI assistant.
There was a problem hiding this comment.
Follow-up Review
Excellent work addressing the previous feedback! This follow-up commit demonstrates solid engineering practices and responsiveness to review comments.
Verification of Previous Feedback:
| Item | Status | Notes |
|---|---|---|
| OAuth client ID documentation | ✅ Fixed | Added clear comments explaining it is public/non-secret in both code and docs |
| Fire-and-forget async tasks | ✅ Fixed | Now tracked with _background_tasks set and exceptions logged via _track_background_task() |
| JWT decoding duplication | ✅ Fixed | Extracted to new openai_codex_jwt.py shared utility module |
| Error parsing substring matching | ✅ Fixed | Replaced with precise regex patterns (INVALID_GRANT_PATTERN, rate limit patterns) |
| Deduplication logic | ✅ Fixed | Extracted _dedupe_openai_codex_records() to eliminate duplication |
| httpx.AsyncClient reuse | Still creates new clients per request (non-blocking) | |
| TODOs for quota window tuning | Now have better comments referencing tracking doc (acceptable for MVP) |
Assessment of New Changes:
The new openai_codex_jwt.py utility module is well-structured with clear documentation. The test additions (test_queue_refresh_deduplicates_under_concurrency, test_refresh_invalid_grant_queues_reauth_sync, test_parse_quota_error_does_not_match_generic_quota_substrings) provide good coverage for the fixed issues.
Overall Status:
This PR is in excellent shape and ready for merge. The addressed items significantly improve code quality and maintainability. The remaining open items are minor optimizations that don't block functionality.
This review was generated by an AI assistant.
| account_id = self._extract_account_id_from_payload( | ||
| access_payload | ||
| ) or self._extract_account_id_from_payload(id_payload) | ||
|
|
||
| # Prefer explicit email claim from id_token first (most user-specific), | ||
| # then explicit access-token email, then fall back to sub-based extraction. |
There was a problem hiding this comment.
Good improvement tracking background tasks with _background_tasks set and surfacing exceptions via _track_background_task(). This addresses the previous fire-and-forget concern. Consider also adding a cleanup method to cancel pending tasks on shutdown to prevent warnings about unfinished tasks.
| @@ -0,0 +1,105 @@ | |||
| # SPDX-License-Identifier: LGPL-3.0-only | |||
There was a problem hiding this comment.
Excellent extraction of JWT helpers into a shared utility module. The clear module docstring explaining the intentional lack of signature verification is appreciated. This eliminates the duplication between credential_manager.py and openai_codex_auth_base.py.
|
|
||
| # Keep top-level expiry_date synchronized from token exp as fallback | ||
| if not creds.get("expiry_date"): | ||
| expiry_ms = self._extract_expiry_ms_from_payload(access_payload) or self._extract_expiry_ms_from_payload( | ||
| id_payload |
There was a problem hiding this comment.
The INVALID_GRANT_PATTERN regex is much more precise than the previous substring matching. Good fix for avoiding false positives.
|
|
||
| if isinstance(content, str): | ||
| return content | ||
|
|
||
| if isinstance(content, list): | ||
| parts: List[str] = [] | ||
| for item in content: | ||
| if isinstance(item, dict): | ||
| # OpenAI chat content blocks | ||
| if item.get("type") == "text" and isinstance(item.get("text"), str): | ||
| parts.append(item["text"]) | ||
| elif item.get("type") in {"input_text", "output_text"} and isinstance( | ||
| item.get("text"), str |
There was a problem hiding this comment.
The rate limit regex patterns are well-designed and the _looks_like_rate_limit() method properly avoids false positives from generic "quota" substrings (as verified by the new test). Clean implementation.
| access_payload = decode_jwt_unverified(access_token) | ||
| id_payload = decode_jwt_unverified(id_token) if id_token else None | ||
|
|
||
| account_id = extract_account_id_from_payload(access_payload) or extract_account_id_from_payload( | ||
| id_payload | ||
| ) | ||
| email = extract_email_from_payload(id_payload) or extract_email_from_payload(access_payload) | ||
| exp_ms = extract_expiry_ms_from_payload(access_payload) or extract_expiry_ms_from_payload( | ||
| id_payload | ||
| ) | ||
|
|
||
| return account_id, email, exp_ms | ||
|
|
||
| def _normalize_openai_codex_auth_json_record(self, auth_data: Dict[str, Any]) -> Optional[Dict[str, Any]]: | ||
| """Normalize ~/.codex/auth.json format to proxy schema.""" | ||
| tokens = auth_data.get("tokens") | ||
| if not isinstance(tokens, dict): | ||
| return None | ||
|
|
||
| access_token = tokens.get("access_token") | ||
| refresh_token = tokens.get("refresh_token") | ||
| id_token = tokens.get("id_token") | ||
|
|
||
| if not isinstance(access_token, str) or not isinstance(refresh_token, str): | ||
| return None | ||
|
|
||
| account_id, email, exp_ms = self._extract_codex_identity(access_token, id_token) | ||
|
|
There was a problem hiding this comment.
Nice deduplication refactor extracting _dedupe_openai_codex_records(). This eliminates the duplicate dedupe logic between the two import paths.
2 participants
Description
adds codex oauth multi-account support
Re-targeted from #137 (was against
main, now againstdev).Testing Done
Checklist
Follow-up pushed (c3e244d)
Addressed review notes:
Ran
pytest -q(35 passed).