Repository navigation
fix(codex): retain selected subscription auth across turns - #243
Conversation
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — 190c6add
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T22:35:59Z
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 190c6add7b
ℹ️ 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".
| ): AsyncIterable<ChatDelta> { | ||
| // Codex's refresh token rotates once. Keep one bridge turn per selected | ||
| // account in flight so two private jail copies cannot spend it together. | ||
| const releaseAuthTurn = await this.acquireAuthTurn(signal) |
There was a problem hiding this comment.
Acquire this global lease only when turns actually share a rotating ChatGPT credential. As written, every Codex request is serialized, including API-key auth and Docker executors configured with oauthMode: 'per-slot'; I checked the Docker wiring in src/server.ts, where each pool slot receives an independent credential volume specifically to support parallel execution. Consequently, a long turn blocks otherwise independent sessions, leaves all but one configured pool slot idle, and can cause queued requests' execution deadlines to expire before they spawn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed by f0ddbf5 and b9a575a: only unjailed shared-host file-backed ChatGPT turns hold the full-turn lease. Jailed, API-key, Docker, and per-slot turns remain parallel. Two real concurrent jailed subscription turns on b9a575a overlapped in 68/75 sampled native process intervals, both completed with separate outputs and no residual child. Artifact: /tmp/cli-bridge-concurrency-proof-20260927-final/overlap.json.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — f0ddbf5e
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T22:44:59Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — b9a575a7
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T22:46:59Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — c199aa92
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T22:53:59Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — e4de6740
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T22:58:59Z
|
@codex review\n\nPlease review current head e4de674. The prior P1 global queue is addressed by selective shared-host subscription leasing; isolated jailed and per-slot turns retain parallel execution. Latest auth compatibility follows installed Codex 0.156.1 modes and preserves last_refresh in access-only jail seeds. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4de67405e
ℹ️ 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".
| try { | ||
| const auth = readFileMaybe(authSourcePath) | ||
| if (auth !== null) writeFileSync(join(baseDir, 'auth.json'), auth) | ||
| if (existsSync(authSourcePath)) symlinkSync(resolve(authSourcePath), join(baseDir, 'auth.json')) |
There was a problem hiding this comment.
When Codex uses the Docker executor and a synthetic/retained CODEX_HOME is exposed through the configured workspace mount, this absolute symlink points to the bridge host's auth path, which is not mounted at that location inside the container; the per-slot credential volume is mounted separately under the container home. Because codex.ts also exports the synthetic directory as CODEX_HOME, retained-session and MCP launches see a dangling auth.json and fail authentication. The previous byte copy remained readable in the mounted synthetic home, so Docker materialization must continue copying auth or avoid overriding the container's per-slot home.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on 6c78567: Docker Codex no longer exports a host synthetic CODEX_HOME. The container keeps its mounted account home; request MCP config enters that home through stdin, with base config restored before slot reuse. Actual two-slot per-slot and shared-mount no-model proofs, including different MCP secrets, auth readability, no secret in argv, forced restore failure and next-turn recovery, are retained under /tmp/cli-bridge-docker-auth-proof-20260927/.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — 6c78567d
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T23:29:59Z
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c78567dc8
ℹ️ 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".
| const server = /^mcp_servers\.(?:"([^"]+)"|'([^']+)'|([A-Za-z0-9_-]+))(?:\.|$)/.exec(section) | ||
| skipSection = !!server && names.has(server[1] ?? server[2] ?? server[3]!) |
There was a problem hiding this comment.
When the mounted Docker config.toml already declares an MCP server that is absent from the current request, this predicate removes only aliases present in names, so every other ambient [mcp_servers.*] table survives into the per-turn config. A Docker Codex turn can therefore access tools and credentials not declared by its exact profile, contrary to the repository's sole-MCP-authority contract; remove all existing MCP tables/keys before appending the requested set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on 9c3d761: every Docker Codex turn installs a per-turn config, including zero-MCP turns; all ambient MCP tables and keys are removed. Actual native Docker codex mcp list with both unquoted and quoted ambient tables saw no undeclared server; base config restored. Artifacts: /tmp/cli-bridge-docker-auth-proof-20260927/affinity-zero-mcp-unquoted.json and affinity-zero-mcp-quoted.json.
| ...(dockerExecution ? { | ||
| CODEX_HOME: undefined, | ||
| ...(this.spawner.dockerOauthMode === 'per-slot' ? { |
There was a problem hiding this comment.
With Docker oauthMode: 'per-slot', clearing CODEX_HOME stores Codex rollouts and indexes in the acquired slot's credential volume. I checked ContainerPool.acquire: affinity is only a preference, and when the previous slot is busy it falls back to any free slot. A later codex exec resume <thread_id> can therefore run in a home without that thread and lose or fail retained-session continuity; use a session-persistent home visible across slots or enforce durable slot affinity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on 9c3d761: per-slot Docker Codex sessions use deterministic hard slot affinity for a fixed pool size, including when their slot is busy. Actual two-slot disposable Docker proof returned a typed 300 ms capacity refusal with the other slot free, then resumed on the original slot; /tmp/cli-bridge-docker-auth-proof-20260927/affinity-zero-mcp.json. README states that pool size changes require session migration.
| const releaseShared = opts.backend === 'codex' && (opts.oauthMode ?? 'share') === 'share' | ||
| ? await acquireSharedCodex(spawnOpts.signal) : undefined |
There was a problem hiding this comment.
For Docker Codex with a shared OAuth mount, a request waits on this lease before calling pool.acquire, but the lease receives only the overall execution signal and never spawnOpts.acquireDeadlineMs. If another shared-home turn is long-running, an execution.acquireTimeoutMs request can wait far beyond its configured queue deadline and eventually report an execution timeout instead of the expected capacity refusal; apply the same bounded acquire deadline to this preliminary lease.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on 9c3d761: the shared-home lease and subsequent slot acquire share one bounded pool acquire deadline, and shared lease waiters respect the configured pool queue cap. Actual disposable Docker proof returned executor_saturated after 301 ms and refused an excess waiter immediately; /tmp/cli-bridge-docker-auth-proof-20260927/shared-deadline.json and shared-queue.json.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — 9c3d7615
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T23:43:59Z
|
@codex review |
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — e288147c
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T23:47:00Z
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c3d761541
ℹ️ 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".
| codexHome ? join(codexHome.homePath, 'config.toml') | ||
| : authSourcePath ? join(dirname(authSourcePath), 'config.toml') : undefined, |
There was a problem hiding this comment.
For a one-shot host jail with no usable request MCP servers, codexHome is null, so this fallback copies the operator's entire persistent config.toml into the agent-writable jail. Any ambient [mcp_servers.*] entries—including headers or commands containing credentials—then become available to a request that did not declare them; the same happens for env-authenticated turns because only the auth path argument is suppressed. Generate an empty/request-scoped config instead of falling back to the account home's config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on ec607ea: jailed host turns now seed a config with all ambient MCP declarations removed while keeping unrelated account settings; declared request MCP config stays scoped. Actual installed Codex 0.156.1 mcp list with a fake key saw no ambient server, and a declared server remained available. Artifact: /tmp/cli-bridge-docker-auth-proof-20260927/jail-mcp.json (sourceHead ec607ea).
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — a68dcfde
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T23:51:01Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — ec607eab
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-27T23:53:59Z
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec607eabe4
ℹ️ 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 (section === '' && !header && trimmed && !trimmed.startsWith('#') | ||
| && trimmed.includes('=') && tomlRootKey(trimmed) === 'mcp_servers') continue |
There was a problem hiding this comment.
When an existing Codex config uses a valid dotted-key declaration with a multiline value, such as mcp_servers.foo.args = [ followed by array elements, this removes only the assignment’s first line and retains its continuation lines. The merged Docker or jailed config is then invalid TOML, so affected Codex turns fail during configuration loading instead of merely stripping the ambient MCP server; parse the complete TOML value or explicitly reject this form before writing the replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on 2ec1c97: a dotted MCP or project key is removed only if its complete TOML value ends on that line. Open arrays, inline tables, strings, and multiline strings now cause a clear configuration error before a partial scoped config is written. A direct parser probe with multiline mcp_servers.foo.args = [ produced the expected refusal. Native Docker and jailed no-model probes on this exact head pass; /tmp/cli-bridge-docker-auth-proof-20260927/project-mcp-docker.json and jail-project-mcp.json.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — 2ec1c97d
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-28T00:06:00Z
|
@codex review Please review exact head 2ec1c97. Scoped Codex config now removes saved project trust, so trusted project MCP does not enter zero-MCP Docker or jailed turns. It also rejects multiline dotted MCP/project values before writing a partial config. Native no-model Docker and installed Codex jail probes are retained under /tmp/cli-bridge-docker-auth-proof-20260927/project-mcp-docker.json and jail-project-mcp.json. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — c3f66477
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-28T00:11:59Z
|
@codex review Please review exact head c3f6647. This corrects a valid multiline account setting that contains a table-looking line: scoped config now recognizes TOML multiline strings before removing ambient MCP and saved project trust. Installed Codex 0.156.1 accepts the scoped native fixture; /tmp/cli-bridge-docker-auth-proof-20260927/multiline-scope.json. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3f66477e6
ℹ️ 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".
| const paths = `d=${dir}; p="$d/config.toml"; b="$d/.cli-bridge-config.backup"; m="$d/.cli-bridge-config.absent"` | ||
| const base = await readDockerCodexConfig(containerId, configDir) | ||
| const merged = scopeCodexTurnConfig(base, config) | ||
| const prepare = `set -eu; ${paths}; if [ -e "$p" ]; then cp -p "$p" "$b"; chmod 600 "$b"; else umask 077; : > "$m"; fi; rm -f "$p"; umask 077; cat > "$p"` |
There was a problem hiding this comment.
When the mounted Codex config.toml is a symlink—for example, a relative link into a dotfiles directory within the mounted config volume—cp -p "$p" "$b" dereferences it, and recovery later replaces the original link with a regular file. This silently breaks the operator's configuration management after the first Docker turn. The runtime's cp --help documents -p as preserving mode, ownership, and timestamps, while -P is required not to dereference links; back up and restore the link itself rather than only its target contents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed on 2b6e10c: Docker now backs up config.toml with cp -Pp, including relative and dangling symlinks, and does not chmod through a link. Actual disposable Docker Codex 0.146 turn excluded ambient MCP, then restored the original relative link and left its target bytes unchanged; inFlight 0. Artifact /tmp/cli-bridge-docker-auth-proof-20260927/symlink-config.json pinned 2b6e10c.
There was a problem hiding this comment.
Follow-up cb3303f closes the dangling-link recovery gap found independently after 2b6e10c. Recovery recognizes symlink backups even when the target is missing; native Docker Codex proof retains a requested MCP only during the turn and restores the original dangling link with backup absent and inFlight 0. /tmp/cli-bridge-docker-auth-proof-20260927/symlink-config.json is pinned cb3303f.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — 2b6e10cd
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-28T00:21:00Z
|
@codex review Please review exact head 2b6e10c. The previous Docker symlink finding is fixed with a link-preserving backup; a native disposable Docker Codex turn proved ambient MCP removed during the turn, original relative symlink and target bytes restored afterward, and inFlight 0. Artifact: /tmp/cli-bridge-docker-auth-proof-20260927/symlink-config.json. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6e10cd26
ℹ️ 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".
| const paths = `d=${dir}; p="$d/config.toml"; b="$d/.cli-bridge-config.backup"; m="$d/.cli-bridge-config.absent"` | ||
| const base = await readDockerCodexConfig(containerId, configDir) | ||
| const merged = scopeCodexTurnConfig(base, config) | ||
| const prepare = `set -eu; ${paths}; if [ -e "$p" ] || [ -L "$p" ]; then cp -Pp "$p" "$b"; if [ ! -L "$b" ]; then chmod 600 "$b"; fi; else umask 077; : > "$m"; fi; rm -f "$p"; umask 077; cat > "$p"` |
There was a problem hiding this comment.
Fresh evidence at the exact head is that this new -L branch backs up a dangling config.toml as a dangling symlink, but recoverDockerCodexConfig later restores only when [ -f "$b" ]; that test follows the link and is false when its target was already missing or is deleted during the turn. Recovery therefore leaves the per-turn regular config.toml installed and strands .cli-bridge-config.backup instead of restoring the original link, so the recovery check also needs to recognize a symbolic-link backup.
Useful? React with 👍 / 👎.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved PR — cb3303fd
Blanket team auto-approval is intentional. The merge gates are CI and codex-p1.
No automated review runs on this PR. This approval rests on the rule above alone.
tangletools · auto-approval · reason: blanket_auto_approve · 2026-09-28T00:27:00Z
|
@codex review Please review exact head cb3303f. Docker recovery now recognizes a dangling config backup symlink and restores it instead of leaving request config in the slot. A native disposable Docker Codex turn proved the declared MCP was present during the turn, then the original dangling link was restored with no backup and inFlight 0; the readable relative-link case still passes. Artifact: /tmp/cli-bridge-docker-auth-proof-20260927/symlink-config.json. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Merged source
All four turns completed with retained native text and usage. In this recording, 8899 reported input/output tokens 31,009/45 and 42,458/60; 8900 reported 34,855/45 and 50,150/60. Native session mappings persisted across both rounds. Dollar cost remained unknown ( The attached uncut 1× video records the entire real command from uncut-1x.mp4fast-4x.mp4
|
2 participants
Change
Keep one selected Codex account per bridge process and serialize its turns. Host native homes link
auth.jsonto the selected account, so the installed CLI persists normal refreshes. Linux fs-jail subscription turns receive allowlisted access credentials and cannot read or promote the host refresh token. The bridge asks the installed app-server to refresh the file-backed account before a bounded jailed turn when needed, with a six-minute margin for native proactive refresh. Subscription turns fail closed where read confinement cannot be enforced.The existing jailed Codex test now owns a temporary auth fixture and checks that ambient Codex auth overrides and extra credentials do not reach the child.
Proof
pnpm typecheckpassed.pnpm testpassed: 1,214 active Vitest tests, 12 skipped, and one Runtime consumer check.HOMEand noCODEX_HOME: 15 tests.account/readwithrefreshToken:truethrough a temporary auth symlink updated the verified hello team account file, kept the symlink intact, and preserved account identity. The file-store-pinned installed argv and read-only RPC also passed.streamAgentTurnthrough an isolated enforced fs-jail bridge returnedRUNTIME_CODEX_REFRESH_OK, 10,032 input and 10 output tokens; USD is unknown. Retained artifact:/tmp/cli-bridge-refresh-proof-20260927-v3/runtime-turn.json.cancelledvia bridge cancellation with no matching Codex child. Artifact:/tmp/cli-bridge-refresh-proof-20260927-v3/cancel.json. The temporary bridge stopped and its port closed.Scope and limit
Service routes 8899 and 8900 are held for independent recovery review. The in-process turn queue does not coordinate external Codex processes sharing the same account home; intended service mapping gives each bridge a distinct team account. A jailed access-only turn must fit inside the token lifetime after the proactive-refresh margin.