Skip to content

fix(codex): route account providers explicitly - #497

Merged
lucas77778 merged 1 commit into
masterfrom
chenyu/code-644
Aug 31, 2026
Merged

fix(codex): route account providers explicitly#497
lucas77778 merged 1 commit into
masterfrom
chenyu/code-644

Conversation

@lucas77778

Copy link
Copy Markdown
Member

Summary

  • select a thread-scoped Codex model provider for every account-backed custom endpoint instead of relying on the built-in OpenAI provider
  • route Responses over HTTPS with the account token and configured base URL, with WebSocket and OpenAI login auth disabled
  • keep CLI-backed 401s on the ChatGPT login flow while reporting account-backed 401s as provider authentication failures
  • document and regression-test the provider routing and auth distinction

CODE-644

Verification

  • pnpm check:ci
  • pnpm test — 3040 passed, 1 skipped
  • targeted Codex credential, routing, and auth tests — 55 passed
  • real Codex app-server probe observed POST /v1/responses with the expected bearer token; a provider 401 remained recoverable and did not emit authentication_failed

Checklist

  • pnpm check:ci and pnpm test both pass (plus cargo fmt / clippy / test for Rust changes)
  • I ran the affected surface and observed the change working
  • If a wire message changed: WIRE_PROTOCOL_VERSION is bumped (no wire message changed)
  • New code and assets are my own work, or their origin and license compatibility are noted above
  • Docs and comments are updated where behavior changed

Copilot AI lite review requested due to automatic review settings August 31, 2026 05:14
@linear-code

linear-code Bot commented Aug 31, 2026

Copy link
Copy Markdown

CODE-644

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@lucas77778
lucas77778 requested a review from xiaoland August 31, 2026 05:16
@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown

Greptile Summary

The PR explicitly routes account-backed Codex sessions through a thread-scoped Responses provider and distinguishes provider authentication failures from ChatGPT login failures.

  • Adds account-provider configuration using the configured base URL and credential environment.
  • Disables WebSocket transport and OpenAI login requirements for account-backed endpoints.
  • Preserves the ChatGPT login refresh flow for CLI-backed authentication failures.
  • Adds regression coverage and updates adapter documentation.

Confidence Score: 5/5

The PR appears safe to merge with no concrete blocking or non-blocking defects identified.

Account credentials, provider configuration, thread routing, and authentication-error classification remain internally consistent across the changed paths, with regression coverage for the primary behavior.

Important Files Changed

Filename Overview
packages/host/agent-adapter/src/native/codex/adapter.ts Adds consistent account-provider routing to both thread start and resume paths and separates account-provider 401 handling from CLI login failures.
packages/host/agent-adapter/src/tests/codex-auth.test.ts Verifies that account-backed 401 responses remain recoverable and do not request ChatGPT login.
packages/host/agent-adapter/src/tests/codex-shell.test.ts Verifies the generated provider identifier, Responses configuration, base URL, credential environment, and disabled WebSocket behavior.
packages/host/agent-adapter/AGENTS.md Documents the explicit provider-routing and authentication distinction introduced by the implementation.

Sequence Diagram

sequenceDiagram
  participant Host as Host session
  participant Adapter as Codex adapter
  participant Codex as Codex app-server
  participant Provider as Account endpoint
  Host->>Adapter: Start with account credential and base URL
  Adapter->>Codex: "thread/start or thread/resume<br/>modelProvider=linkcode-account<br/>Responses config"
  Codex->>Provider: "HTTPS POST /v1/responses<br/>Bearer CODEX_API_KEY"
  alt Provider returns 401
    Provider-->>Codex: Unauthorized
    Codex-->>Adapter: Error notification
    Adapter-->>Host: Recoverable provider authentication error
  else CLI-backed session returns 401
    Codex-->>Adapter: Authentication error
    Adapter-->>Host: Non-recoverable authentication_failed
  end
Loading

Reviews (1): Last reviewed commit: "fix(codex): route account providers expl..." | Re-trigger Greptile

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The provider routing itself checks out — I verified every config key and the modelProvider param against the pinned codex 0.144.6 binary. But the auth-classification half changes an account-backed 401 from a fail-fast signal into a recoverable, stop-less one, which can hang an unattended automation turn indefinitely.

Reviewed changes — the full diff of commit 2d311c8 (4 files), plus the surrounding Codex adapter, the engine's AUTH_FAILED_ERROR_CODE and automation consumers, and the installed @openai/codex 0.144.6 binary's own generated protocol types.

  • Private per-thread model provideraccountProviderOverrides() emits six model_providers.linkcode-account.* config keys and openThread() selects them via modelProvider, replacing the OPENAI_BASE_URL approach that never configured the built-in provider.
  • Shared start/resume params — the provider rides both thread/start and thread/resume, so the post-401 respawn keeps the account routing rather than silently falling back.
  • 401 classification split — a key-backed 401 now emits a provider error, while a CLI-backed one keeps authentication_failed so the daemon still re-probes ChatGPT login.
  • Docs and tests — the agent-adapter/AGENTS.md Auth and apiKey-injection bullets were rewritten, and two new tests cover routing and the auth split.

I verified the protocol claims directly rather than trusting the description: codex app-server generate-ts on the pinned binary shows modelProvider?: string | null on both ThreadStartParams and ThreadResumeParams and no provider field on TurnStartParams, so the selection is thread-scoped and per-turn model/effort overrides cannot reset it. ModelProviderInfo carries requires_openai_auth and supports_websockets, the model_providers map key is a free-form string (the hyphen is fine), and unknown keys are silently ignored rather than a config-load error. OPENAI_BASE_URL appears 0 times in that binary against 23 for OPENAI_API_KEY, which confirms the premise of the fix.

ℹ️ Three sites still describe codex as overriding the built-in provider's base URL

The PR establishes that codex does not read OPENAI_BASE_URL (confirmed — the string is absent from the 0.144.6 binary), which makes the surviving descriptions of the old mechanism wrong. None are in the diff, so they are reported here rather than inline. Only the first sentence of each is affected; the wire_api = "chat" reasoning that follows in resolve.ts remains correct and load-bearing.

Technical details
# Stale "codex overrides the built-in provider's base URL" references

## Affected sites
- `packages/foundation/providers/src/resolve.ts:17` — the doc comment that drives `preferredProtocols`/`resolveBinding`; states codex overrides the base URL of its *built-in* Responses provider, which is now a private `linkcode-account` provider instead.
- `packages/foundation/providers/src/__tests__/resolve.test.ts:92` — same claim as a test comment.
- `packages/host/engine/src/__tests__/provider-config.test.ts:131` — same claim, used to justify why codex carries no `knownProvider`.

## Required outcome
- The three comments describe the mechanism the adapter actually uses, so the next reader does not re-adopt `OPENAI_BASE_URL` as the routing knob. The *conclusions* (codex speaks only `openai-responses`; codex accounts carry no `knownProvider`) are unchanged and must survive the edit.

ℹ️ OPENAI_BASE_URL is now known-dead, and the new test pins it

codexEnv still sets OPENAI_BASE_URL (credential.ts:74), docs/ENVIRONMENT.md:45 still documents it as a credential handed to the agent subprocess, and codex-shell.test.ts:380 now asserts it — while this same PR documents that it does not configure anything. It may be deliberate cover for older detected user installs that did honor it, but the PR does not say so, and a test assertion reads as an intentional contract. Worth a decision either way: keep it with a one-line note about drifted detected binaries, or drop the env var, its doc row, and the assertion.

Technical details
# `OPENAI_BASE_URL` has no reader on the pinned Codex binary

## Evidence
- `grep -a -c OPENAI_BASE_URL node_modules/@openai/codex-linux-x64/vendor/x86_64-unknown-linux-musl/bin/codex``0` (`OPENAI_API_KEY` → 23, `CODEX_API_KEY` → 4). Binary self-reports `codex-cli 0.144.6`, matching the `^0.144.6` pin.

## Affected sites
- `packages/host/agent-adapter/src/credential.ts:74``if (cred.baseUrl) env.OPENAI_BASE_URL = cred.baseUrl;`
- `packages/host/agent-adapter/src/__tests__/codex-shell.test.ts:380` — new assertion that the var reaches the app-server env.
- `packages/host/agent-adapter/src/__tests__/credential.test.ts:79-82` — pre-existing assertion.
- `docs/ENVIRONMENT.md:45` — lists it as an account credential handed to the agent subprocess.

## Required outcome
- A reader can tell whether the var is live behavior or deliberate compatibility ballast for a drifted detected install. Either state the reason at the one site that sets it, or remove it together with its doc row and both assertions.

## Open questions for the human
- Do any Codex versions in the *detected* install window (the compat-manifest range, CODE-77) still read `OPENAI_BASE_URL`? If none do, it is dead in every reachable configuration.

ℹ️ Nitpicks

  • packages/host/agent-adapter/AGENTS.md still pins codex at 0.144.1 in the version table while package.json says ^0.144.6 and the new Auth bullet cites 0.144.6. Pre-existing drift, but this PR is the natural place to correct it.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread packages/host/agent-adapter/src/native/codex/adapter.ts
Comment thread packages/host/agent-adapter/AGENTS.md

@xiaoland xiaoland left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified, it's work:

Image

and the code LGTM.

@lucas77778
lucas77778 merged commit b18fdc4 into master Aug 31, 2026
13 checks passed
@lucas77778
lucas77778 deleted the chenyu/code-644 branch August 31, 2026 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants