refactor: unify OpenAI provider identity#88451
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed May 30, 2026, 7:19 PM ET / 23:19 UTC. Summary Reproducibility: yes. for the PR defects via source inspection: the affected upgrade/setup paths are visible in the PR head without needing live credentials. I did not run live OAuth or doctor migration because this review is read-only and the PR body does not provide that proof. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land a backward-preserving unification that preserves or migrates shipped config/auth/setup inputs through doctor-owned repair, keeps legacy fallback refresh safe until migration, and includes redacted real proof for fresh OpenAI auth plus a legacy upgrade path. Do we have a high-confidence way to reproduce the issue? Yes for the PR defects via source inspection: the affected upgrade/setup paths are visible in the PR head without needing live credentials. I did not run live OAuth or doctor migration because this review is read-only and the PR body does not provide that proof. Is this the best way to solve the issue? No. Canonicalizing the OpenAI identity may be the right direction, but this implementation still removes shipped compatibility paths and needs real upgrade/auth proof before it is the safest maintainable solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 8d6a6e9d0325. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
f4c1f67 to
f6e0e90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e0e90dff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!Object.hasOwn(providers, OPENAI_PROVIDER_ID)) { | ||
| providers[OPENAI_PROVIDER_ID] = normalizedConfig; | ||
| } | ||
| delete providers[providerId]; |
There was a problem hiding this comment.
Preserve legacy OpenAI provider config when both ids exist
When a user already has both models.providers.openai (for direct API-key/OpenAI Platform settings) and a legacy models.providers.openai-codex entry (often carrying ChatGPT/Codex proxy baseUrl, headers, or request options), this normalization deletes the legacy entry without merging or warning because openai is present. The validated runtime config then loses those Codex transport overrides, so upgraded configs with both providers silently route openai/* through the surviving direct OpenAI provider settings instead of the user's legacy ChatGPT/Codex transport.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06fe882e5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }), | ||
| readFallbackCredential: ({ profileId, credential }) => | ||
| credential.provider === "openai" || credential.provider === "openai-codex" | ||
| credential.provider === "openai" |
There was a problem hiding this comment.
Restore legacy Codex fallback refresh
When a stored OAuth profile still has the shipped legacy provider: "openai-codex", the refresh path can no longer recover it: the OpenAI OAuth provider/runtime now resolves under openai, and this predicate skips readExternalCliFallbackCredential for the retired id. In that upgrade state, an expired legacy Codex profile that used to fall back to local Codex CLI auth now resolves to no API key until the user runs doctor/migration, so keep accepting the retired provider id here or normalize it before the check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea4251e562
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "openai-completions", | ||
| "openai-responses", | ||
| "openai-codex-responses", | ||
| "openai-chatgpt-responses", |
There was a problem hiding this comment.
Preserve the legacy Codex catalog API alias
External or workspace plugin manifests that still publish the shipped api: "openai-codex-responses" now lose their API during normalizeModelCatalogApi() because this enum no longer accepts the old string, and normalizeModelCatalogProviderRows() only emits api when that lookup succeeds. In that upgrade case the row no longer matches the ChatGPT/Codex response hooks, so custom catalog entries silently stop using the intended transport; keep accepting/normalizing the legacy API value here as well as in config validation.
Useful? React with 👍 / 👎.
| @@ -19,8 +19,7 @@ import { uniqueStrings } from "../shared/string-normalization.js"; | |||
|
|
|||
| const CORE_PROVIDER_AUTH_ENV_VAR_CANDIDATES = { | |||
| anthropic: ["ANTHROPIC_OAUTH_TOKEN", "ANTHROPIC_API_KEY"], | |||
| openai: ["OPENAI_API_KEY"], | |||
| "openai-codex": ["CODEX_API_KEY", "OPENAI_API_KEY"], | |||
| openai: ["CODEX_API_KEY", "OPENAI_API_KEY"], | |||
There was a problem hiding this comment.
Do not let CODEX_API_KEY shadow direct OpenAI keys
For direct OpenAI Platform paths such as openai-responses or OpenAI image generation, resolveEnvApiKey("openai") iterates these candidates in order, so an environment that has both CODEX_API_KEY and OPENAI_API_KEY now sends the Codex app-server key to the direct OpenAI API and never reaches the valid OPENAI_API_KEY. Keep CODEX_API_KEY scoped to the ChatGPT/Codex transport or prefer OPENAI_API_KEY for the unified openai provider's direct API-key flows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88faddf0a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (profileIds.length === 0) { | ||
| return true; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Treat env OpenAI keys as direct image auth
When isConfigured() is checking a non-public OpenAI image endpoint (for example an Azure/OpenAI-compatible baseUrl) and the only credential is OPENAI_API_KEY from the environment, isProviderApiKeyConfigured({ provider: "openai" }) is true but this helper now returns false because there are no stored profiles. Since the configured check then requires hasDirectOpenAIImageApiKeyAuth() for non-public base URLs, the OpenAI image provider is reported unavailable even though generateImage() can resolve and use the env key; keep the no-profile case as direct API-key auth or pass through the env result explicitly.
Useful? React with 👍 / 👎.
|
Verification for head 88faddf:
Known proof gaps: none for the changed surface; live OAuth/API-key Crabbox evidence was already captured earlier in this PR path and this final push only fixes CI follow-through/test expectations plus the OpenAI high-signal filter guard. |
* refactor: unify OpenAI provider identity * refactor: move legacy oauth sidecar doctor helpers * test: align OpenAI fixtures after rebase * test: clean OpenAI provider unification * fix: finish OpenAI provider cleanup * fix: finish OpenAI cleanup follow-through * fix: finish OpenAI CI cleanup
* refactor: unify OpenAI provider identity * refactor: move legacy oauth sidecar doctor helpers * test: align OpenAI fixtures after rebase * test: clean OpenAI provider unification * fix: finish OpenAI provider cleanup * fix: finish OpenAI cleanup follow-through * fix: finish OpenAI CI cleanup
* refactor: unify OpenAI provider identity * refactor: move legacy oauth sidecar doctor helpers * test: align OpenAI fixtures after rebase * test: clean OpenAI provider unification * fix: finish OpenAI provider cleanup * fix: finish OpenAI cleanup follow-through * fix: finish OpenAI CI cleanup
Summary
openaiacross provider registration, auth/profile selection, model routing, usage polling, image routing, native/UI fixtures, and docs.openai-chatgpt-responses, while leaving legacy names only in doctor/migration compatibility paths.CODEX_API_KEY, API-key usage polling, sidecar OAuth preservation, image auth lookup, OAuth refresh guidance, TLS preflight visibility, and Hermes nested token-pool detection.Verification
OPENCLAW_VITEST_FS_MODULE_CACHE_PATH=.artifacts/vitest-cache-openai-unify node scripts/run-vitest.mjs extensions/migrate-hermes/files-and-skills.test.ts extensions/migrate-hermes/secrets.test.ts extensions/migrate-hermes/provider.secret-failure.test.ts src/commands/models/auth-list.test.ts src/commands/doctor-auth.hints.test.ts src/commands/doctor-auth.profile-health.test.tsOPENCLAW_VITEST_FS_MODULE_CACHE_PATH=.artifacts/vitest-cache-oauth-tls node scripts/run-vitest.mjs src/commands/oauth-tls-preflight.test.ts src/commands/oauth-tls-preflight.doctor.test.tspnpm tsgo:prodpnpm tsgo:testgit diff --checkrg -n 'openai-codex' . --glob '!node_modules/**' --glob '!.git/**' --glob '!CHANGELOG.md' --glob '!appcast.xml' --glob '!src/commands/doctor*' --glob '!src/commands/doctor/**' --glob '!docs/gateway/doctor.md' --glob '!docs/cli/doctor.md' --glob '!extensions/codex/doctor-contract-api.ts'; test $? -eq 1rg -n 'openai-codex-responses|openai-codex-' . --glob '!node_modules/**' --glob '!.git/**' --glob '!CHANGELOG.md' --glob '!appcast.xml' --glob '!src/commands/doctor*' --glob '!src/commands/doctor/**' --glob '!docs/gateway/doctor.md' --glob '!docs/cli/doctor.md' --glob '!extensions/codex/doctor-contract-api.ts'; test $? -eq 1/Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode localclean after fixing the accepted TLS-preflight finding.Real behavior proof
Behavior addressed: OpenAI and ChatGPT OAuth now share the canonical
openaiprovider identity; legacy OpenAI-Codex state is handled by doctor/migration paths instead of live provider ids.Real environment tested: Local OpenClaw source checkout on macOS with Node/Vitest/tsgo.
Exact steps or command run after this patch: The verification commands above, including focused auth/migration/provider tests, type checks, strict retired-id scans, and autoreview.
Evidence after fix: Focused Vitest shards passed;
tsgo:prodandtsgo:testpassed; strict scans found no non-doctor liveopenai-codexreferences; autoreview reported no accepted/actionable findings.Observed result after fix: The tree keeps
openai-codexonly in doctor/migration/history surfaces, preserves pre-doctor legacy profile visibility where migration diagnostics need it, and usesopenaifor live OpenAI provider/auth routing.What was not tested: Live OAuth browser sign-in, live OpenAI API-key calls, and Crabbox/WebVNC were not rerun in this closeout pass.