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.
2 participants
Description
adds codex oauth multi-account support
Re-targeted from #137 (was against
main, now againstdev).Testing Done
Checklist