fix(runtime): preserve Codex OAuth env isolation#125
Conversation
|
/code-review |
Code reviewThis PR narrows Codex OAuth/API-key separation for SDK profile resolution and adds focused regression tests. The direction is right, but the fix is incomplete for the local Codex transports called out in the PR body: SDK is isolated, while CLI/App Server can still inherit the ambient Must fix
Should fix
Nits
Context gates
Verdict: REQUEST_CHANGES until CLI/App Server stop implicitly consuming |
Code reviewThis PR aims to keep local Codex OAuth / Current head: e549b76. This is still not safe to merge as-is: the earlier blocker for non-SDK local Codex transports remains, and the branch is now 32 commits behind current Response pass
What looks good
Must fix
Must fix / merge-order requirement
Should fix
NitsNone. Context gates
Verification
VerdictVerdict: REQUEST_CHANGES until Codex CLI/App Server stop implicitly consuming ambient |
ichinya
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES until Codex CLI/App Server stop implicitly consuming ambient OPENAI_API_KEY, the branch is rebased onto current main, and CI passes on the rebased head.
Local Codex transports (sdk, cli, app-server) no longer implicitly consume an ambient OPENAI_API_KEY / OPENAI_BASE_URL, so a placeholder or unrelated key in .env (e.g. OPENAI_API_KEY=sk-000) can no longer hijack an OAuth-backed `codex login` session. - sdk: block OPENAI_API_KEY/OPENAI_BASE_URL from the SDK child env and stop inferring apiKey/baseUrl from them; forward only when a profile apiKeyEnvVar opts into API-key auth. - cli: gate OPENAI_API_KEY (and any custom key var) behind explicit opt-in; inject an explicit literal apiKey directly. - app-server: drop the implicit process.env.OPENAI_API_KEY fallback and apply the same opt-in gate (model discovery reuses this builder). - resolution: return apiKeyEnvVar/apiKey null for local Codex transports unless a profile sets them; infer CODEX_BASE_URL (not OPENAI_BASE_URL) for local transports. API transport behaviour is unchanged. Docs and .env.example updated to describe the OAuth-by-default contract and the explicit API-key/CODEX_BASE_URL opt-in. Adds regression tests for every transport plus profile resolution.
|
Since this PR has been inactive, we picked it up and addressed @ichinya's Review items resolved[P1] Local Codex CLI/App Server still consumed ambient
[P1] Rebase onto current [P2] Docs / Verification
Adapter parity: the hijack is Codex-specific (Claude/OpenRouter/opencode use their own auth and never consume an ambient |
e549b76 to
a10b678
Compare
|
Requesting changes. The SDK fix is good, but the PR still does not protect all local Codex transports as claimed. Please make CLI/App Server match SDK behavior:
Docs / Verdict: REQUEST_CHANGES. |
|
The CLI/App Server blockers were already addressed in
The latest review text matches the 2026-05-13 review almost verbatim and describes the pre-fix code paths. Could you re-review against head |
Move OPENAI_API_KEY into the CLI and App Server BLOCKED_ENV_KEYS sets and forward the explicitly opted-in key var before the blocklist, matching the SDK transport's structure. Behavior is unchanged — ambient OPENAI_API_KEY / OPENAI_BASE_URL are still blocked by default and only honored when a profile opts into API-key auth via apiKeyEnvVar/apiKey — but all three local Codex transports now isolate ambient OpenAI auth env through one identical pattern, satisfying the runtime adapter-parity checklist rule and removing the ad-hoc per-key conditionals that obscured the isolation guarantee.
|
Pushed The CLI and App Server env isolation was already behaviorally correct at
This also satisfies the runtime adapter-parity checklist rule — all three local Codex transports ( Verification: |
|
Re-checking the PR: verdict remains REQUEST_CHANGES. The SDK path is fixed, but the local transport contract is still incomplete. The PR says local Codex transports ( CLI still allows Please make CLI/App Server mirror SDK behavior:
Verdict: REQUEST_CHANGES. |
|
All five items in this review are already implemented at the current PR head
Could you re-run the review against head |
|
Note for maintainers: the automated The current head already implements every requested item — ambient Requesting a fresh re-review against |
mifogeshi15617-commits
left a comment
There was a problem hiding this comment.
Debi has thoroughly reviewed the PR. The isolation of ambient OpenAI env defaults (OPENAI_API_KEY/OPENAI_BASE_URL) is perfectly implemented across all three local transports (SDK, CLI, App Server). The regression tests cover all scenarios and verify complete parity. Prettier/ESLint are green, tests are green. Merging is highly recommended!
|
Re-checking this: the original PR #125 / branch is no longer accessible from the current GitHub connector state, so I cannot verify a new head SHA. However, current Verdict remains REQUEST_CHANGES. The required behavior is:
Current |
|
@ichinya This review appears to be stale. PR #125 is accessible and its current head is The requested behavior is implemented at that head:
So the latest REQUEST_CHANGES comment seems to have reviewed |
|
Reviewed PR #125. The PR addresses the Codex OAuth/env-isolation issue consistently across the runtime resolution layer and all local Codex transports. What changed:
I checked the relevant implementation paths:
The test coverage added in the PR is good and covers the important regression cases:
I did not find blocking issues in the PR diff. Validation note: in this Windows workspace, full |
Summary
OPENAI_API_KEY,OPENAI_BASE_URL) when the user intends to run throughcodex login/ ChatGPT OAuth.apitransport and for explicitly configured SDK profiles viaapiKeyEnvVar, while preserving explicitbaseUrl/CODEX_BASE_URLoverrides.OPENAI_API_KEY=sk-000cannot hijack OAuth-backed chat again.PROJECTS_DIR.Problem
A Codex runtime profile using
transport: sdkis expected to use the local Codex login state (codex login, e.g. ChatGPT OAuth). However, the SDK adapter currently forwards allOPENAI_*environment variables and also infersapiKey/baseUrlfromOPENAI_API_KEYandOPENAI_BASE_URLby default.That means a normal local/Docker setup can be logged in successfully with
codex login status, but chat still fails if.envcontains a placeholder or unrelated OpenAI API config such as:OPENAI_API_KEY=sk-000OPENAI_BASE_URL=http://host.docker.internal:8317/v1In that case Codex SDK is instantiated as an API-key client and sends requests to
/v1/responses, producing errors like401 Incorrect API key providedorstream disconnected before completion, instead of using the OAuth-backed Codex session.Fix
For local Codex transports (
sdk,cli,app-server), runtime resolution no longer treatsOPENAI_API_KEY/OPENAI_BASE_URLas implicit defaults. The SDK child environment also blocksOPENAI_API_KEYandOPENAI_BASE_URLunless API-key auth is explicitly requested through profile options.The
apitransport is unchanged: it still requires API key + base URL. Explicit SDK overrides are also preserved, so users who really want API-key-backed SDK runs can setapiKeyEnvVarorapiKey, and users can still opt into a custom Codex endpoint via profilebaseUrlorCODEX_BASE_URL.Test plan
npm run format:checknpm run lint --workspace=@aif/runtimenpm test -- --run src/__tests__/resolution.test.ts src/__tests__/codexSdk.test.tsfrompackages/runtimenpm test --workspace=@aif/runtime(59files,786tests)npm run build --workspace=@aif/runtimenpm run ai:validateattempted; on this Windows machine it reaches the perf phase and then fails in the existing web perf harness withspawn EINVALfrompackages/web/scripts/run-perf.mjs, unrelated to the runtime changes.