Repository navigation
Conversation
ApprovabilityVerdict: Needs Human Approval Macroscope's review found this PR not approvable — The change replaces static managed Codex bearer injection with a new server-owned IPC broker and native auth.command flow, spanning token renewal, credential persistence, process lifetime, and profile binding. It also adds production static-analysis suppressions, so the authentication-sensitive and cross-process behavior warrants human review. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
|
Clarifying the Windows security-boundary note in the CodeRabbit summary for The intended boundary is the server's OS user, not an individually authenticated managed child. Environment/instance/profile identifiers select and bind credentials; they are not caller-authentication secrets. The implementation does not claim a literally owner-only pipe ACL or use an unpredictable pipe name as authorization. The disposable Windows checks described in the PR's Verification section exercised the actual bundled broker on both Node and Electron. The observed security descriptor granted full access to the owner, Administrators and SYSTEM, and read access to Everyone/Anonymous, consistent with the documented Windows default. An Everyone-restricted token produced: Read access is therefore not described as denied. The broker sends nothing on connection and requires a complete valid request before returning a token. The observed write restriction and request-before-response behavior work together; profile binding alone is not the peer boundary. These are the earlier startup-corrected-source probes, not newly claimed executions following this comment. I rechecked that the published broker, protocol and transport helper are byte-identical to those probe sources, and all 21 published source hashes still match the approved snapshot. The final subsequent correction only preserved a runtime setup error's cause and added its assertions. A separately launched unrelated-user process, Windows service hosting, and remote-SMB identities were not exercised. Those remain explicit verification limits for human review; the restricted-token result is not presented as coverage of every Windows identity/deployment. No ACL/account changes or new code changes were made in response to this inferred concern. Macroscope's human-approval requirement and the upstream fork-workflow approval gates remain pending. |
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
|
Taking over this one as maintainer for batch codex-09. I will rebuild the token-renewal fix from current main as reviewable broker and runtime-integration layers, trim redundant tests, and retain author credit. Nothing will be merged by this batch. |
|
Thanks @Timpan4. You identified the stale launch-time bearer correctly and established that native auth.command can recover active turns. The server-owned refresh lifetime and reuse of the existing credential lock/store are retained. I rebuilt this from current architecture as two reviewable layers:
I compared your complete original diff with the replacement stack. No required renewal, persistence, listener-failure containment, environment/profile binding, catalog-race, or service-fixture behavior is intentionally omitted. The standalone broker/protocol/helper permutation suites were reduced to seven added behavior cases in existing files. Independent review found that profile-specific provider names break native saved-thread resume after upgrade or sign-in; the replacement keeps the stable provider name and retains binding in the helper/broker. The helper also avoids loading the contracts graph. The replacement passed 173 focused tests and scoped static checks. Native Codex 0.156.1 completed a synthetic 401→renewal→200 in the same process/thread/turn, and a legacy thread preserved its history across upgrade, broker/runtime restart, and controlled sign-in replacement. Opus 5.5 and GPT-6 Sol reviewed the full stack in two rounds; round two found no code blockers. Closing this in favor of those unmerged layers. Their bodies preserve attribution and state the pending human auth/IPC, current-head CI, and platform-verification gates; model review does not waive those gates. |
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
Managed ChatGPT Codex passed its access token to the shared app-server once, at launch. T3 renewed the saved token, but the running process kept the old one, so long turns (and every turn after the hour was up) failed with 401 "Provided authentication token is expired". Backport of pingdotgg#19895 (unmerged, head 7241aee): Codex now fetches the bearer through its native auth.command, which asks a private T3 socket for a current token, so renewal reaches turns already running. Fork fixup: the broker tests spied on ESM namespace exports, which vitest rejects; they now mock node:fs/promises and node:os instead.
Problem
T3 can renew saved managed ChatGPT credentials while an already-running Codex app-server keeps its launch-time bearer. Subsequent conversation requests can continue to fail with an expired token even though Settings has valid saved credentials. Restarting the app-server is not necessary when the installed Codex already supports native
auth.commandrecovery.Change
auth.command, using T3's existing Node/Electron/asar/SEA self-invocation conventions. No owned bearer is placed in launch arguments or the child environment.Scope and approval
Addresses the stale-launch-bearer mechanism in the maintainer triage for #16132 and the expiry/401 reports in #17445. It configures native
auth.command, already supported by the pinned Codex 0.156.1, for the existing T3-owned ChatGPT connection. It does not change terminal CLI authentication or claim to fix failed OAuth refresh grants, every 401, or unrelated historical hourly failures.The SQLite shadow-home fix is independent: #19861. Neither PR depends on the other.
The private broker is necessary because actual native Stop and process-tree teardown can kill a direct credential-writing helper after OAuth accepts a rotation but before the atomic rename. Moving persistence into the existing server fixes that lifetime boundary without adding a daemon, duplicate credential store, public TCP/RPC/MCP endpoint, periodic refresh timer, dependency, installer-pin change or desktop termination-policy change. The four live-provider test edits only supply the required typed service fixture.
The channel has bounded requests, responses, connections and admitted jobs, with immediate overload rejection and no waiting queue. These are local resource-policy choices, not Codex protocol requirements or a promise of recovery under arbitrary load. Windows protection relies on default pipe WRITE/duplex access, not a literally owner-only ACL: read-only peers receive no token bytes. Unix uses a trusted parent, an owned non-symlink
0700directory and a0600socket.Both requested independent reviewers, GPT-6.1 Sol xhigh and Astra xhigh, approved the complete 21-file feature at
b4e4b0dcagainst maind8e9fd26through the normal Codex provider. Each inspected every changed hunk, current callers/interfaces and installed dependency semantics, with no necessary findings or omitted files. Beginning and ending integrity checks verified all 21 disk/index/committed hashes and the complete 100,115-byte diff (SHA256c953e05e584a9437900c9032c852a78a40b3e639f8cc306138f619fe32141392). This is a fresh full-feature review, including all six added files.The current normal merge integrates main
d8e9fd26atb4e4b0dc. Its only manual conflict resolution retains both the upstream startup-failure fixture and the profile-change catalog regression. Automatic merges preserve upstream workspace-skill scan error propagation, full-CLI stdin handling and server workspace-service wiring. This branch remains independent of the SQLite PR.Verification
./node_modules/.bin/vp test run \ apps/server/src/provider/CodexChatGptAuth.test.ts \ apps/server/src/provider/CodexManagedRuntime.test.ts \ apps/server/src/provider/CodexManagedTokenBroker.test.ts \ apps/server/src/provider/CodexManagedTokenProtocol.test.ts \ apps/server/src/cli/managedChatGptToken.test.ts \ apps/server/src/provider/Drivers/CodexDriver.test.ts \ apps/server/src/provider/ProviderInstanceRegistry.test.ts \ apps/server/src/provider/ProviderRegistry.test.ts \ apps/server/src/orchestration-v2/AcpRegistryOrchestratorV2.live.test.ts \ apps/server/src/orchestration-v2/CursorOrchestratorV2.live.test.ts \ apps/server/src/orchestration-v2/GrokOrchestratorV2.live.test.ts \ apps/server/src/orchestration-v2/OpenCode2OrchestratorV2.live.test.tsCurrent integrated source
b4e4b0dc, with frozen upstream dependencies including Effect/platform-node 4.0.5: 176 passed, 30 skipped, one failed across the 12 selected files. The failure is the existing default-Cursor registry fixture's Windows temporary-directory cleanup (rmdir EBUSY), reproduced in isolation and in the independent SQLite checkout where the registry source and test are byte-identical to main and the managed-token feature is absent. The skip count includes platform and live-test exclusions. Server typecheck, all 21 changed-file lint/format checks, scoped Knip exports and feature diff checks passed. This is not a full-suite-green claim.The earlier
7241aeeaintegration passed its separate 11-file selection, 145 passed and 17 platform skips, plus server pack. That earlier selection included store, session-lock and self-invocation regressions; it was not rerun solely for this merge. Both listener-failure cases preserve exact diagnostic causes while keeping public message/detail safe.Coverage includes concurrent rotation, disconnect during OAuth and before rename, scope drain, logout without credential resurrection, reconnect/cancel/controller replacement, profile-switch contention, catalog replacement races, request framing/admission/deadlines and failed listener setup before rotation.
At the earlier
7241aeeaintegration, the actual-source bundles and packed helper were rebuilt and these disposable proofs with mock OAuth/model endpoints passed. They were not rerun atb4e4b0dc; the current merge is covered by the focused tests and static checks above:/models404 diagnostics are not catalog-success evidence; catalog binding is covered by targeted regressions.TMPDIRstartup failure, then verified protected short fallback, unavailable-parent containment and partial acquisition cleanup. This is not a full Linux Vitest or Linux-native Codex run.Retained verification limits: the broader settings suite produced 59 passed / one failed dangling-settings-link watcher test, also reproduced with both settings implementation and test byte-identical to main
86534db4in the independent checkout. A separate earlier registry run has an unchanged WindowsEBUSYfixture-cleanup failure reproduced on its original baseline. An unchanged hard-link-alias test failed once in an earlier run, then passed in isolated and full retries; its transient cause is unproven. These failures were not silently fixed or counted as passing runs. This is not a whole-suite-green claim.Human IPC/auth review and genuinely executed upstream CI are still required. Fork workflow runs with
action_requiredand zero jobs are maintainer approval gates, not passing or failing test executions. Model approvals and automated review success do not establish merge readiness.No real OAuth, live settings/database changes, UI, macOS or built-SEA execution was performed. The durability guarantee is ordinary consumer cancellation while the server lives. Whole-server kill, power loss and the desktop's unchanged five-second forced termination are outside that guarantee; graceful drain is not unconditional app-exit durability.
Model: GPT-6.1 Sol. Harness: Pi via T3 Code for the initial implementation; Codex via T3 Code for the current main integration. Current independent review models: GPT-6.1 Sol (
gpt-6.1-sol) and Astra (gpt-6-astra), both xhigh through the normal Codex provider.