-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add Codex CLI provider with local authentication #8051
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
- Add new provider 'codex-cli' extending BaseOpenAiCompatibleProvider - Implement local sign-in flow without API keys using session tokens - Store session tokens securely in VS Code Secret Storage - Add UI component with Sign In/Sign Out buttons and CLI path configuration - Add message handlers for codexCliSignIn, codexCliSignOut, and codexCliDetect - Include comprehensive test coverage for the new provider - Follow existing patterns from Roo Cloud and Claude Code providers This implementation allows users to use OpenAI-compatible models through Codex CLI without managing API keys directly, using a local authentication flow instead.
| {isSignedIn ? ( | ||
| <> | ||
| <div className="text-sm text-vscode-descriptionForeground"> | ||
| {t("settings:providers.codexCli.authenticatedMessage", { |
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.
Avoid using inline fallback/default English strings in translation calls (via defaultValue). Instead rely on the translation JSON files for the English string to keep consistency with translation guidelines.
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
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.
I wrote this code 5 minutes ago and I'm already confused by my own TODOs.
| loginProcess.on("close", async (code) => { | ||
| if (code === 0) { | ||
| // Login successful, extract the token from the output | ||
| const tokenMatch = output.match(/token:\s*([^\s]+)/) |
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 sign-in implementation appears to be placeholder logic. The token extraction pattern (line 2373) and base URL extraction (line 2381) won't work with the actual CLI output. Should we complete the ChatMock integration as mentioned in the issue?
|
|
||
| // Try to detect the CLI in PATH | ||
| try { | ||
| const { stdout } = await exec("which codex") |
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 CLI detection is hardcoded to 'codex' here, but shouldn't we use the configured CLI path from apiConfiguration.codexCliPath instead?
| } | ||
| } catch (error) { | ||
| // If ContextProxy is not initialized, continue with defaults | ||
| console.debug("ContextProxy not available, using default values for Codex CLI") |
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.
Is this silent error handling intentional? When ContextProxy isn't available, we're just logging to debug and continuing with defaults. This could mask configuration issues - perhaps we should at least log a warning?
| } | ||
|
|
||
| // Check if a custom CLI path is configured | ||
| const cliPath = options.codexCliPath |
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 cliPath variable is retrieved here but never actually used in the handler. Should this be passed to the base constructor or used elsewhere?
|
|
||
| export const CodexCli: React.FC<CodexCliProps> = ({ apiConfiguration, setApiConfigurationField }) => { | ||
| const { t } = useAppTranslation() | ||
| const [isSignedIn, setIsSignedIn] = useState(false) |
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 signed-in state management here relies on inference rather than explicit state from the backend. Could we improve this by having the backend explicitly communicate the authentication state?
|
What about the new Codex models? |
This PR implements the Codex CLI provider enhancement requested in Issue #8049.
Summary
Adds a new Codex CLI provider that extends the OpenAI-compatible base provider, enabling users to use OpenAI-compatible models through a local CLI without managing API keys directly.
Implementation Details
Provider Implementation
CodexCliHandlerclass extendingBaseOpenAiCompatibleProviderfor wire-level OpenAI compatibilityhttp://localhost:3000/v1Authentication Flow
codexCliSignIn,codexCliSignOut, andcodexCliDetectmessage handlersContextProxyGLOBAL_SECRET_KEYS)User Interface
Type Safety & Testing
Areas for Future Enhancement
Testing
Closes #8049
cc @hannesrudolph
Important
Adds Codex CLI provider with local authentication, UI integration, and comprehensive testing.
CodexCliHandlerinsrc/api/providers/codex-cli.ts, extendingBaseOpenAiCompatibleProvider.baseURLwith defaulthttp://localhost:3000/v1.codexCliSignIn,codexCliSignOut, andcodexCliDetectinwebviewMessageHandler.ts.ContextProxy.CodexClicomponent inCodexCli.tsxwith Sign In/Sign Out buttons.codex-cli.ts.codex-cli.spec.tsfor handler functionality.provider-settings.tsandglobal-settings.tsto include Codex CLI settings.ApiOptions.tsxandconstants.tsfor UI integration.This description was created by
for a5d010e. You can customize this summary. It will automatically update as commits are pushed.