-
Notifications
You must be signed in to change notification settings - Fork 38
fix(codex): avoid runtime shadow reindex on TUI startup #639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2760,6 +2760,101 @@ describe("codex bin wrapper", () => { | |
| expect(marker).toContain("close\n"); | ||
| }); | ||
|
|
||
| it("uses the canonical Codex home for interactive TUI runtime routing", async () => { | ||
| const fixtureRoot = createWrapperFixture(); | ||
| createRuntimeRotationProxyFixtureModule(fixtureRoot); | ||
| const fakeBin = createCustomFakeCodexBin(fixtureRoot, [ | ||
| "#!/usr/bin/env node", | ||
| 'const { spawnSync } = require("node:child_process");', | ||
| 'const fs = require("node:fs");', | ||
| 'const path = require("node:path");', | ||
| 'const args = process.argv.slice(2);', | ||
| 'if (args[0] === "app-server") {', | ||
| ' console.log(`APP_SERVER_FORWARDED:${args.join(" ")}`);', | ||
| " process.exit(0);", | ||
| "}", | ||
| 'const statePath = path.join(process.env.CODEX_HOME ?? "", "state_5.sqlite");', | ||
| 'const originalState = path.join(process.env.ORIGINAL_CODEX_HOME ?? "", "state_5.sqlite");', | ||
| 'console.log(`TUI_HOME_IS_ORIGINAL:${process.env.CODEX_HOME === process.env.ORIGINAL_CODEX_HOME}`);', | ||
| 'console.log(`TUI_STATE_EXISTED:${fs.existsSync(statePath)}`);', | ||
| 'fs.writeFileSync(statePath, "first-run-state\\n", "utf8");', | ||
| 'console.log(`TUI_STATE_PERSISTED:${fs.readFileSync(originalState, "utf8").includes("first-run-state")}`);', | ||
| 'console.log(`TUI_HAS_BASE_URL_OVERRIDE:${args.some((arg) => arg.includes("model_providers.codex-multi-auth-runtime-proxy.base_url="))}`);', | ||
| 'console.log(`TUI_HAS_ENV_KEY_OVERRIDE:${args.includes(\'model_providers.codex-multi-auth-runtime-proxy.env_key="OPENAI_API_KEY"\')}`);', | ||
| 'console.log(`TUI_HAS_AUTH_OVERRIDE:${args.includes("model_providers.codex-multi-auth-runtime-proxy.requires_openai_auth=false")}`);', | ||
| 'console.log(`TUI_HAS_WIRE_OVERRIDE:${args.includes(\'model_providers.codex-multi-auth-runtime-proxy.wire_api="responses"\')}`);', | ||
| 'console.log(`TUI_HAS_STORAGE_OVERRIDE:${args.includes("disable_response_storage=false")}`);', | ||
| 'console.log(`TUI_KEY_IN_ARGS:${args.includes(process.env.OPENAI_API_KEY ?? "__missing__")}`);', | ||
| 'const shimExe = path.join(process.env.CODEX_CLI_PATH ?? "", process.platform === "win32" ? "codex.exe" : "codex");', | ||
| 'const shimResult = spawnSync(shimExe, ["app-server", "--canonical-shim"], { encoding: "utf8", env: process.env });', | ||
| 'console.log(`TUI_SHIM_STATUS:${shimResult.status}`);', | ||
| 'console.log(`TUI_SHIM_STDOUT:${(shimResult.stdout ?? "").trim()}`);', | ||
| "process.exit(0);", | ||
| ]); | ||
| const originalHome = join(fixtureRoot, "codex-home"); | ||
| const markerPath = join(fixtureRoot, "proxy-marker.txt"); | ||
| mkdirSync(originalHome, { recursive: true }); | ||
| writeFileSync( | ||
| join(originalHome, "config.toml"), | ||
| 'model_provider = "openai"\n', | ||
| "utf8", | ||
| ); | ||
|
|
||
| const result = runWrapper(fixtureRoot, [], { | ||
| CODEX_MULTI_AUTH_REAL_CODEX_BIN: fakeBin, | ||
| CODEX_HOME: originalHome, | ||
| ORIGINAL_CODEX_HOME: originalHome, | ||
| CODEX_MULTI_AUTH_RUNTIME_ROTATION_PROXY: "1", | ||
| CODEX_MULTI_AUTH_APP_ROTATION_IDLE_MS: "200", | ||
| CODEX_MULTI_AUTH_TEST_PROXY_MARKER: markerPath, | ||
| OPENAI_API_KEY: undefined, | ||
| }); | ||
|
|
||
| const output = combinedOutput(result); | ||
| if (result.status !== 0) { | ||
| throw new Error(output); | ||
| } | ||
| expect(output).toContain("TUI_HOME_IS_ORIGINAL:true"); | ||
| expect(output).toContain("TUI_STATE_EXISTED:false"); | ||
| expect(output).toContain("TUI_STATE_PERSISTED:true"); | ||
| expect(output).toContain("TUI_HAS_BASE_URL_OVERRIDE:true"); | ||
| expect(output).toContain("TUI_HAS_ENV_KEY_OVERRIDE:true"); | ||
| expect(output).toContain("TUI_HAS_AUTH_OVERRIDE:true"); | ||
| expect(output).toContain("TUI_HAS_WIRE_OVERRIDE:true"); | ||
| expect(output).toContain("TUI_HAS_STORAGE_OVERRIDE:true"); | ||
| expect(output).toContain("TUI_KEY_IN_ARGS:false"); | ||
| expect(output).toContain("TUI_SHIM_STATUS:0"); | ||
| expect(output).toContain( | ||
| "APP_SERVER_FORWARDED:app-server --canonical-shim", | ||
| ); | ||
| expect(output).toContain( | ||
| "model_providers.codex-multi-auth-runtime-proxy.base_url=", | ||
| ); | ||
| expect(output).toContain( | ||
| 'model_providers.codex-multi-auth-runtime-proxy.env_key="OPENAI_API_KEY"', | ||
| ); | ||
| expect(output).toContain( | ||
| "model_providers.codex-multi-auth-runtime-proxy.requires_openai_auth=false", | ||
| ); | ||
| expect(output).toContain( | ||
| 'model_providers.codex-multi-auth-runtime-proxy.wire_api="responses"', | ||
| ); | ||
| expect(output).toContain("disable_response_storage=false"); | ||
| expect(output).toContain( | ||
| 'model_provider="codex-multi-auth-runtime-proxy"', | ||
| ); | ||
| expect(readFileSync(join(originalHome, "state_5.sqlite"), "utf8")).toBe( | ||
| "first-run-state\n", | ||
| ); | ||
| expect(readFileSync(join(originalHome, "config.toml"), "utf8")).toBe( | ||
| 'model_provider = "openai"\n', | ||
| ); | ||
| await waitForFileText( | ||
| markerPath, | ||
| "start:http://127.0.0.1:4567\nclose\n", | ||
| ); | ||
| }); | ||
|
|
||
|
Comment on lines
+2763
to
+2857
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win solid single-session coverage, but missing a concurrency regression case for canonical-home sharing. see scripts/codex.js:3497-3528 for the root cause — canonical mode now shares the live codex home across processes with no isolation. as per path instructions, test/** should "demand regression cases that reproduce concurrency bugs" and this new test only exercises one TUI session at a time. 🧰 Tools🪛 ast-grep (0.44.1)[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) 🤖 Prompt for AI AgentsSource: Path instructions |
||
| it("stops detached TUI app helpers after failed launches", async () => { | ||
| const fixtureRoot = createWrapperFixture(); | ||
| createRuntimeRotationProxyFixtureModule(fixtureRoot); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: ndycode/codex-multi-auth
Length of output: 614
🏁 Script executed:
Repository: ndycode/codex-multi-auth
Length of output: 28523
🏁 Script executed:
Repository: ndycode/codex-multi-auth
Length of output: 41998
don’t give interactive TUI/app-helper modes unguarded access to the real codex home.
scripts/codex.js:3903-3938selectscreateRuntimeRotationProxyCanonicalCodexHomefor TUI andscripts/codex.js:4078-4124gives the helper the same env; it setsCODEX_HOMEtooriginalCodexHomewithcleanup: () => {}, whilescripts/codex.js:3419-3495previously used a shadow home and sync-back. no lock/single-instance guard is in play, so two concurrent canonicalcodex-multi-authsessions using the same Codex home can race on session/sqlite state. add a regression intest/codex-bin-wrapper.test.ts:2763-2857that launches two canonical sessions concurrently and asserts no state corruption/loss, or enforce a single-instance/flock/backoff design.📍 Affects 2 files
scripts/codex.js#L3497-L3528(this comment)test/codex-bin-wrapper.test.ts#L2763-L2857🤖 Prompt for AI Agents
Source: Path instructions