Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for GitHub Copilot as a provider in the CLI, SDK, and documentation, and enriches process‐execution utilities with PID tracking.
- Document GitHub Copilot in README and CLI help, and implement OAuth login flow
- Introduce
pidinExecResultand propagate it throughrawExecandexecApplyPatch - Implement
GithubCopilotClient, wire it intocreateOpenAIClientandAgentLoop
Reviewed Changes
Copilot reviewed 12 out of 12 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 | Add pid assertions and handle zombie processes in tests |
| codex-cli/tests/invalid-command-handling.test.ts | Assert pid is zero on spawn failure |
| codex-cli/tests/cancel-exec.test.ts | Verify pid is set for canceled and normal executions |
| codex-cli/src/utils/providers.ts | Add githubcopilot entry with name, baseURL, and envKey |
| codex-cli/src/utils/openai-client.ts | New GithubCopilotClient subclass and selection logic |
| codex-cli/src/utils/get-api-key.tsx | OAuth device-flow for GitHub Copilot API key retrieval |
| codex-cli/src/utils/agent/sandbox/raw-exec.ts | Include pid in results and shorten kill escalation timeout |
| codex-cli/src/utils/agent/sandbox/interface.ts | Add pid to ExecResult type |
| codex-cli/src/utils/agent/exec.ts | Populate pid in execApplyPatch result |
| codex-cli/src/utils/agent/agent-loop.ts | Instantiate GithubCopilotClient when provider is githubcopilot |
| codex-cli/src/cli.tsx | Update help text, login flow, and set provider‐specific env var |
| README.md | Document githubcopilot under supported providers and env vars |
Comments suppressed due to low confidence (5)
codex-cli/src/utils/providers.ts:56
- Use official branding casing: rename "GithubCopilot" to "GitHubCopilot" for consistency.
name: "GithubCopilot",
codex-cli/src/utils/providers.ts:58
- Add an underscore between words in the env var key (e.g. "GITHUB_COPILOT_API_KEY") to align with other providers.
envKey: "GITHUBCOPILOT_API_KEY",
codex-cli/src/utils/get-api-key.tsx:768
- Consider adding unit or integration tests for the GitHub Copilot OAuth flow (
getLoginURL/pollForAccessToken).
export async function getGithubCopilotApiKey(): Promise<string> {
README.md:109
- Document how to obtain and set the GitHub Copilot token, including any login commands or prerequisites.
> - githubcopilot
codex-cli/src/utils/agent/sandbox/raw-exec.ts:128
- [nitpick] Reducing the escalation timeout to 250ms may lead to premature SIGKILL and flaky tests; verify this lower delay is sufficient on all platforms.
}, 250).unref();
| } | ||
| // 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.
Deriving the env var name from provider.toUpperCase() will produce GITHUBCOPILOT_API_KEY (no underscore). Consider mapping Copilot explicitly to the correct key.
| process.env[`${provider.toUpperCase()}_API_KEY`] = apiKey; | |
| const envVarName = provider.toLowerCase() === "githubcopilot" | |
| ? "GITHUB_COPILOT_API_KEY" | |
| : `${provider.toUpperCase()}_API_KEY`; | |
| process.env[envVarName] = apiKey; |
| const home = os.homedir(); | ||
| const authDir = path.join(home, ".codex"); | ||
| const authFile = path.join(authDir, "auth.json"); | ||
| fs.writeFileSync( |
There was a problem hiding this comment.
Avoid blocking the event loop in async code—use fs.promises.writeFile instead of writeFileSync.
| fs.writeFileSync( | |
| await fs.promises.writeFile( |
| protected override authHeaders( | ||
| _opts: Core.FinalRequestOptions, | ||
| ): Core.Headers { | ||
| return {}; |
There was a problem hiding this comment.
authHeaders override returns an empty object, discarding default headers. Consider merging or calling super.authHeaders.
| return {}; | |
| const defaultHeaders = super.authHeaders(_opts); | |
| return { | |
| ...defaultHeaders, | |
| // Add or override custom headers here if needed | |
| }; |
| } catch { | ||
| /* ignore */ | ||
| } | ||
| } else if (cli.flags.login) { |
There was a problem hiding this comment.
[nitpick] The GitHub Copilot login block duplicates logic in getGithubCopilotApiKey. Consider consolidating into a shared helper.
Summary
Testing
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