Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces full GitHub Copilot provider support by adding a dedicated client, handling device-flow login, persisting tokens, and updating process execution logic to track and assert PIDs.
- Added
pidtoExecResultand updatedrawExecand related tests to validate process group termination. - Implemented
GithubCopilotClientwith token fetching, integrated it in CLI, and exposed newgithubcopilotprovider configuration. - Updated CLI to initiate device-flow login for Copilot, persist credentials in
auth.json, and dynamically handle API keys.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| codex-cli/tests/raw-exec-process-group.test.ts | Updated test to assert root PID and extended ensureProcessGone. |
| codex-cli/src/utils/providers.ts | Exposed githubcopilot provider with updated name, baseURL, envKey. |
| codex-cli/src/utils/openai-client.ts | Added GithubCopilotClient for Copilot API token management. |
| codex-cli/src/utils/get-api-key.tsx | Introduced getGithubCopilotApiKey device-flow login function. |
| codex-cli/src/utils/agent/sandbox/raw-exec.ts | Added PID tracking and faster kill escalation in rawExec. |
| codex-cli/src/cli.tsx | Integrated Copilot login flow, env var persistence, dynamic API key handling. |
Comments suppressed due to low confidence (2)
codex-cli/src/utils/providers.ts:56
- [nitpick] Provider
nameproperty andenvKeyuse inconsistent casing and separators; consider renaming toGitHubCopilotandGITHUB_COPILOT_API_KEYfor consistency.
name: "GithubCopilot",
codex-cli/src/utils/openai-client.ts:65
- Add unit tests covering
GithubCopilotClient, including token fetching, header merging, and OAuth device flow methods.
export class GithubCopilotClient extends OpenAI {
| copy["User-Agent"] = "GithubCopilot/1.155.0"; | ||
| copy["editor-version"] = "vscode/1.85.1"; | ||
| copy["editor-plugin-version"] = "copilot/1.155.0"; |
There was a problem hiding this comment.
[nitpick] Extract hard-coded version strings (User-Agent, editor-version, etc.) into constants or configuration to improve maintainability.
| copy["User-Agent"] = "GithubCopilot/1.155.0"; | |
| copy["editor-version"] = "vscode/1.85.1"; | |
| copy["editor-plugin-version"] = "copilot/1.155.0"; | |
| copy["User-Agent"] = GITHUB_COPILOT_USER_AGENT; | |
| copy["editor-version"] = EDITOR_VERSION; | |
| copy["editor-plugin-version"] = EDITOR_PLUGIN_VERSION; |
| "GitHub OAuth login is not yet implemented for Codex. " + | ||
| "Please generate a token manually and set it as GITHUB_COPILOT_TOKEN." + | ||
| "\n" | ||
| export async function getGithubCopilotApiKey(): Promise<string> { |
There was a problem hiding this comment.
[nitpick] Provide a fallback for manual API key entry or prompt when device flow isn't available, as removing the previous manual entry option may limit usability.
| export async function getGithubCopilotApiKey(): Promise<string> { | |
| export async function getGithubCopilotApiKey(): Promise<string> { | |
| const choice = await promptUserForChoice(); | |
| if (choice.type === "apikey") { | |
| process.env["GITHUBCOPILOT_API_KEY"] = choice.key; | |
| return choice.key; | |
| } |
| } | ||
| // Ensure the API key is available as an environment variable for legacy code | ||
| process.env["OPENAI_API_KEY"] = apiKey; | ||
| process.env[`${provider.toUpperCase()}_API_KEY`] = apiKey; |
There was a problem hiding this comment.
Dynamic assignment of process.env[${provider}_API_KEY] overrides the expected OPENAI_API_KEY for non-openai providers (e.g., Azure); consider special-casing openai/azure to maintain compatibility.
| process.env[`${provider.toUpperCase()}_API_KEY`] = apiKey; | |
| if (provider.toLowerCase() === "openai") { | |
| process.env["OPENAI_API_KEY"] = apiKey; | |
| } else if (provider.toLowerCase() === "azure") { | |
| process.env["AZURE_API_KEY"] = apiKey; | |
| } else { | |
| process.env[`${provider.toUpperCase()}_API_KEY`] = apiKey; | |
| } |
| if (provider.toLowerCase() === "githubcopilot" && !apiKey) { | ||
| apiKey = await fetchGithubCopilotApiKey(); | ||
| try { | ||
| const home = os.homedir(); | ||
| const authDir = path.join(home, ".codex"); | ||
| const authFile = path.join(authDir, "auth.json"); | ||
| fs.writeFileSync( | ||
| authFile, | ||
| JSON.stringify( | ||
| { | ||
| GITHUBCOPILOT_API_KEY: apiKey, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| "utf-8", | ||
| ); | ||
| } catch { | ||
| /* ignore */ | ||
| } | ||
| } else if (cli.flags.login) { | ||
| if (provider.toLowerCase() === "githubcopilot") { | ||
| apiKey = await fetchGithubCopilotApiKey(); |
There was a problem hiding this comment.
[nitpick] The initial githubcopilot login block will always take precedence, making the subsequent cli.flags.login branch unreachable for Copilot; consider merging or removing redundant logic.
| if (provider.toLowerCase() === "githubcopilot" && !apiKey) { | |
| apiKey = await fetchGithubCopilotApiKey(); | |
| try { | |
| const home = os.homedir(); | |
| const authDir = path.join(home, ".codex"); | |
| const authFile = path.join(authDir, "auth.json"); | |
| fs.writeFileSync( | |
| authFile, | |
| JSON.stringify( | |
| { | |
| GITHUBCOPILOT_API_KEY: apiKey, | |
| }, | |
| null, | |
| 2, | |
| ), | |
| "utf-8", | |
| ); | |
| } catch { | |
| /* ignore */ | |
| } | |
| } else if (cli.flags.login) { | |
| if (provider.toLowerCase() === "githubcopilot") { | |
| apiKey = await fetchGithubCopilotApiKey(); | |
| if ((provider.toLowerCase() === "githubcopilot" && !apiKey) || cli.flags.login) { | |
| if (provider.toLowerCase() === "githubcopilot") { | |
| apiKey = await fetchGithubCopilotApiKey(); | |
| try { | |
| const home = os.homedir(); | |
| const authDir = path.join(home, ".codex"); | |
| const authFile = path.join(authDir, "auth.json"); | |
| fs.writeFileSync( | |
| authFile, | |
| JSON.stringify( | |
| { | |
| GITHUBCOPILOT_API_KEY: apiKey, | |
| }, | |
| null, | |
| 2, | |
| ), | |
| "utf-8", | |
| ); | |
| } catch { | |
| /* ignore */ | |
| } |
Summary
GithubCopilotClientfor Copilot API requestsgithubcopilotprovider configurationTesting
pnpm --filter @openai/codex run formatpnpm --filter @openai/codex run lintpnpm --filter @openai/codex run typecheckpnpm --filter @openai/codex run testhttps://chatgpt.com/codex/tasks/task_e_684f272bac248326a07facb43efee33f