feat: add experimental Codex session supervisor#207
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| process.env.CODEX_AUTH_REFRESH_TOKEN = "refresh-secret"; | ||
| process.env.CODEX_AUTH_CLI_SESSION_SUPERVISOR = "1"; | ||
| process.env.CODEX_HOME = "/tmp/codex-home"; | ||
| process.env.PATH = "C:\\Windows\\System32"; |
There was a problem hiding this comment.
process.env.PATH mutation leaks across test suite
process.env.PATH is set to "C:\\Windows\\System32" here but PATH is not in the envKeys list that afterEach restores. on every platform except win32 this permanently replaces PATH for all subsequent tests in the same vitest worker, breaking any test that later needs to fork a real process (npm, node, etc.).
fix: either save/restore PATH manually or include it in envKeys:
| process.env.PATH = "C:\\Windows\\System32"; | |
| const originalPath = process.env.PATH; | |
| process.env.PATH = "C:\\Windows\\System32"; | |
| try { | |
| const env = supervisorTestApi?.buildCodexChildEnv?.(); | |
| expect(env).toBeTruthy(); | |
| expect(env?.CODEX_AUTH_ACCESS_TOKEN).toBeUndefined(); | |
| expect(env?.CODEX_AUTH_REFRESH_TOKEN).toBeUndefined(); |
and close with } finally { process.env.PATH = originalPath; }. alternatively just add "PATH" to the envKeys tuple at the top of the file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/codex-supervisor.test.ts
Line: 574
Comment:
**`process.env.PATH` mutation leaks across test suite**
`process.env.PATH` is set to `"C:\\Windows\\System32"` here but `PATH` is not in the `envKeys` list that `afterEach` restores. on every platform except win32 this permanently replaces `PATH` for all subsequent tests in the same vitest worker, breaking any test that later needs to fork a real process (npm, node, etc.).
fix: either save/restore `PATH` manually or include it in `envKeys`:
```suggestion
const originalPath = process.env.PATH;
process.env.PATH = "C:\\Windows\\System32";
try {
const env = supervisorTestApi?.buildCodexChildEnv?.();
expect(env).toBeTruthy();
expect(env?.CODEX_AUTH_ACCESS_TOKEN).toBeUndefined();
expect(env?.CODEX_AUTH_REFRESH_TOKEN).toBeUndefined();
```
and close with `} finally { process.env.PATH = originalPath; }`. alternatively just add `"PATH"` to the `envKeys` tuple at the top of the file.
How can I resolve this? If you propose a fix, please make it concise.
ndycode
added
the
needs-human-review
1 participant
Summary
codexCliSessionSupervisorValidation
npm run test:session-supervisor:smokenpm run typechecknpm run buildnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr introduces the
codexCliSessionSupervisorfeature: a newscripts/codex-supervisor.jsruntime (~2400 lines) that wraps interactive codex cli sessions, monitors per-account quota snapshots, pre-warms a successor account when the current account nears its 5h/7d quota threshold, and transparently restarts sessions viacodex resume <session-id>with a freshly selected account. the feature is disabled by default and gated by thecodexCliSessionSupervisorconfig field /CODEX_AUTH_CLI_SESSION_SUPERVISORenv var.key design highlights:
EPERM/EBUSYretry, and stale-lock eviction — concurrency is explicitly handled and testedQuotaProbeUnavailableErrorto non-aborting co-waiters instead of cancelling their probebuildCodexChildEnv()strips allCODEX_AUTH_*andCODEX_MULTI_AUTH_*env keys from the child process environment so token material is never inherited by the real codex binarysafeUnlinkretries onEPERM/EBUSYexclusively on win32one P1 issue found in
test/codex-supervisor.test.ts:process.env.PATHis overwritten globally in a test without being restored byafterEach, which can corrupt the test process PATH on linux/mac runners. two minor P2 issues: a redundant double abort-check inlib/quota-probe.tsand thesafeUnlinkretry timer not calling.unref().Confidence Score: 4/5
process.env.PATHmutation at line 574 needs restorationImportant Files Changed
snapshotProbeCache,sessionRolloutPathById) are shared across concurrent supervisor calls by design. one minor issue:safeUnlinkretry timer not.unref()'d.strips CODEX_AUTH env keys) mutatesprocess.env.PATHglobally without restoring it inafterEach, which can corrupt the PATH for the rest of the suite on non-windows runners.signal?: AbortSignaltoProbeCodexQuotaOptionswith correct abort forwarding to the internalAbortControllerand event-listener cleanup; one redundant consecutivethrowIfQuotaProbeAbortedcall before and inside the sametry {}block.findPrimaryCodexCommand,hasTopLevelHelpOrVersionFlag, andsplitCodexCommandArgshelpers with proper--config=inline flag,--stop-parsing, and case-normalization. clean and well-scoped.syncBeforeSupervisorLaunchso it runs exactly once per forwarded or interactive session. abort sentinel (130) skips post-run sync correctly.codexCliSessionSupervisor: falsedefault andgetCodexCliSessionSupervisoraccessor via existingresolveBooleanSettingpattern; synchronous, side-effect free, documented correctly.codex-supervisor.jsinto fixture dir.Sequence Diagram
sequenceDiagram participant CX as codex.js participant SUP as codex-supervisor.js participant LK as StorageLock participant AM as AccountManager participant QP as quota-probe participant CH as Codex child CX->>SUP: runCodexSupervisorIfEnabled() SUP->>SUP: loadSupervisorRuntime() (dynamic dist/ import) SUP->>LK: withSupervisorStorageLock() LK->>AM: loadFromDisk() AM-->>LK: manager LK->>QP: probeAccountSnapshot(account, signal) QP-->>LK: quota snapshot (cached or live) LK-->>SUP: ready account SUP->>CH: spawnRealCodex(codexBin, args) [env stripped of CODEX_AUTH_*] loop monitor loop (pollMs) SUP->>QP: probeAccountSnapshot(currentAccount) QP-->>SUP: pressure alt prewarm threshold crossed SUP-->>SUP: prepareResumeSelection() [background] end alt rotate threshold + idle SUP->>CH: requestChildRestart (SIGINT→SIGTERM→SIGKILL) SUP->>LK: markCurrentAccountForRestart() SUP->>SUP: commitPreparedSelection() or ensureLaunchableAccount() SUP->>CH: spawnRealCodex(codexBin, resume args) end end CH-->>SUP: exit code SUP-->>CX: supervisedExitCode (null = not handled) CX->>CX: autoSyncManagerActiveSelectionIfEnabled() if neededPrompt To Fix All With AI
Last reviewed commit: "feat: add experiment..."