fix(codex): auto-restore the codex shim after external npm updates (#320)#356
Conversation
📝 WalkthroughWalkthroughThe CLI now restores stable external Codex shim replacements before eligible commands. The implementation adds fingerprint probing, guarded transactional rollback, configuration and environment opt-outs, CLI policy handling, tests, and multilingual documentation. ChangesCodex shim auto-restore
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant AutoRestore
participant ShimStore
User->>CLI: run eligible ocx command
CLI->>AutoRestore: check command and configuration
AutoRestore->>ShimStore: probe launcher fingerprints
ShimStore-->>AutoRestore: stable replacement detected
AutoRestore->>ShimStore: backup and guarded restore
ShimStore-->>AutoRestore: restore result
AutoRestore-->>CLI: warning if restored or failed
CLI-->>User: continue requested command
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: ec1418b670
ℹ️ 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".
| process.exit(0); | ||
| } | ||
|
|
||
| maybeAutoRestoreCodexShim(command, args); |
There was a problem hiding this comment.
In src/cli/index.ts, ocx --help, ocx -h, bare ocx help, and invocation without arguments do not match any preceding early exit, so this call can rename Codex launchers and write shim state before merely printing usage. Move these top-level help/empty-argument cases ahead of auto-restore or include them in the skip policy so help remains side-effect free.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
| function fingerprintIsOldEnough(fingerprint: ShimPathFingerprint, nowMs: number): boolean { | ||
| const timestamps = [fingerprint.mtimeMs, fingerprint.ctimeMs]; | ||
| if (fingerprint.target) timestamps.push(fingerprint.target.mtimeMs, fingerprint.target.ctimeMs); | ||
| return nowMs - Math.max(...timestamps) >= CODEX_SHIM_REPLACEMENT_STABLE_MS; |
There was a problem hiding this comment.
In fingerprintIsOldEnough, an old mtime/ctime only proves that no recorded metadata change happened recently; it does not prove that this fingerprint remained unchanged for 100 ms. If npm pauses for more than 100 ms midway through writing a launcher and another ocx command runs, the partial launcher is accepted and replaces the known-good backup; a subsequently failed update then leaves the shim executing that partial file. Record two matching fingerprints separated by the interval, or persist the first observation and defer until a later probe confirms it.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
| if (probe.prefix.includes(SHIM_MARKER)) { | ||
| if (!isHealthyShimProbe(probe, state.platform)) return { status: "ineligible" }; | ||
| continue; |
There was a problem hiding this comment.
On Windows, when npm has replaced one tracked launcher but another sibling is still the old healthy shim, this continue treats the mixed set as eligible and the transaction repairs only the replaced path. A slow sequential npm update can therefore be modified while it is still processing the remaining .cmd, .ps1, or extensionless launchers, defeating the documented mixed-sibling safety guard. Defer whenever some non-preserve siblings are external replacements and others are still shims, then repair only after the launcher set is consistent.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
| if (existsSync(entry.stagedOldBackupPath)) unlinkSync(entry.stagedOldBackupPath); | ||
| renameSync(operation.file.backupPath, entry.stagedOldBackupPath); | ||
| } | ||
| renameSync(operation.sourcePath, operation.file.backupPath); |
There was a problem hiding this comment.
If two ordinary ocx processes detect the same replacement concurrently, both can pass the fingerprint check before either reaches this rename. After one process stages the old backup and installs its shim, the other can rename that newly written shim over the replacement backup on POSIX; the resulting wrapper then executes a backup that is itself a shim, causing recursive launches and losing the updated Codex launcher. Protect the complete guarded transaction with an interprocess lock or use conditional no-replace operations that abort when either source or destination has changed.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
| /** Narrow deterministic race seam for the guarded transaction tests. */ | ||
| beforeGuardedRefresh?: (wrapperPath: string, index: number) => void; | ||
| }): CodexShimAutoRestoreResult { | ||
| const state = readState(); |
There was a problem hiding this comment.
Every ordinary command now enters autoRestoreCodexShim, whose first operation calls readState; that helper uses an unbounded readFileSync followed by JSON.parse. If codex-shim.json becomes oversized or is replaced with a non-regular file, every ocx command can consume unbounded memory or block before dispatch instead of taking the documented bounded probe path. Validate that the state path is a small regular file and cap the bytes read before parsing it.
AGENTS.md reference: AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/ru/reference/cli.md`:
- Line 401: Исправьте опечатку в пользовательской документации: в описании
поведения `ocx` замените термин «лончер» на стандартный вариант «лаунчер», не
изменяя остальной текст.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb5f8b6b-de03-47da-ae68-fc5dbc2cbf1f
📒 Files selected for processing (24)
README.ja.mdREADME.ko.mdREADME.mdREADME.ru.mdREADME.zh-CN.mddocs-site/src/content/docs/ja/reference/cli.mddocs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/cli.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/cli.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdsrc/cli/codex-shim-autorestore.tssrc/cli/index.tssrc/codex/shim.tssrc/config.tssrc/types.tsstructure/01_runtime.mdtests/codex-shim-autorestore.test.tstests/codex-shim.test.tstests/config.test.ts
| Если обновление Codex перезаписало обёртку, shim автоматически восстанавливается при следующем | ||
| вызове `install` — новый бинарный файл резервируется, и записывается свежая обёртка. | ||
| Если завершённое внешнее обновление Codex перезаписало установленный shim, следующая обычная команда | ||
| `ocx` до запуска сохранит стабильный новый лончер в резервную копию и восстановит shim. Лончер, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Исправьте опечатку в пользовательской документации.
Используйте стандартную транслитерацию лаунчер вместо лончер, чтобы термин совпадал с остальным русским текстом.
Предлагаемое исправление
- до запуска сохранит стабильный новый лончер в резервную копию и восстановит shim. Лончер,
+ до запуска сохранит стабильный новый лаунчер в резервную копию и восстановит shim. Лаунчер,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `ocx` до запуска сохранит стабильный новый лончер в резервную копию и восстановит shim. Лончер, | |
| `ocx` до запуска сохранит стабильный новый лаунчер в резервную копию и восстановит shim. Лаунчер, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/ru/reference/cli.md` at line 401, Исправьте
опечатку в пользовательской документации: в описании поведения `ocx` замените
термин «лончер» на стандартный вариант «лаунчер», не изменяя остальной текст.
Dual review (security + bugs/edge cases)Bot review threads are not cleared yet (6 unresolved: 5 Codex P2 + 1 CodeRabbit docs typo). Below is an independent second look. This PR is credential-path-adjacent (mutates Codex launchers that load Security categories checkedNo evidence of malware, backdoor, hidden remote C2, local admission bypass, classic XSS, or credential logging/serialization. Writes stay on shim wrapper / backup paths plus Fail-safe shape looks intentional and largely sound: Blockers
Should-fix before merge
Non-blocking / already covered well
Aligns with open bot threadsFindings 1–5 match the still-open Codex P2 threads (concurrency lock, stability observation, mixed Windows set, help side effects, bounded state read). Please resolve those after fixing, or reply why a finding does not apply. Suggested fix order
|
|
Maintainer triage review (2026-07-24, post-#359 re-review). Direction is valid and this does NOT fight #359's persisted runtime selection ( High — concurrent restore can lose the real launcher / create shim recursion. Two processes both pass the fingerprint check; process A writes the shim while process B renames that shim to backup (src/codex/shim.ts:691-713). There is no interprocess lock or conditional rename between verify and rename. Result: shim -> backup-shim recursion and the updated launcher is lost. High — the "stable for 100ms" guarantee is only an mtime/ctime age check (src/codex/shim.ts:217-220, :858-865). An updater that stalls >100ms after a partial write gets its partial file promoted over the known-good backup. Medium — Windows mixed sibling sets are restored piecemeal (.cmd replaced while .ps1/extensionless are still shims), contradicting the doc's own "mixed siblings defer" note (src/codex/shim.ts:638-646, :850-865). Medium — bare Medium — every normal invocation synchronously reads+parses all of codex-shim.json with no size/type bound (src/codex/shim.ts:493-516, :842-843); not a "bounded startup probe" for large or non-regular files. Also note: the green CI on ec1418b predates the #359 merge, so exact-SHA cross-platform CI is needed after rework. Suggested path: add an interprocess lock (or compare-and-rename) around the verify->rename transaction, observe real stability (two consecutive identical fingerprints across an interval, not just age), defer mixed sibling sets, move auto-restore behind the help early-exit, and bound the shim-state read. Happy to re-review promptly after a rebuild on current dev. |
ec1418b to
ce65a75
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/reference/cli.md`:
- Around line 375-379: Clarify that `ocx codex-shim status` may perform
auto-restore and rewrite launcher files before reporting state, while preserving
the existing opt-out and repair behavior. Apply the same side-effect warning to
docs-site/src/content/docs/reference/cli.md:375-379,
docs-site/src/content/docs/ko/reference/cli.md:379-383, and
docs-site/src/content/docs/ru/reference/cli.md:400-405, translating it
appropriately in the Korean and Russian pages.
In `@src/codex/shim.ts`:
- Around line 691-703: The reclaimStaleRestoreLock function must atomically
detach the verified stale lock instead of deleting it by path. Replace
unlinkSync(path) with a renameSync operation to a unique temporary target, and
return true only when that rename succeeds; handle rename failures consistently,
including ENOENT, so a concurrent replacement cannot be removed.
In `@tests/codex-shim-autorestore.test.ts`:
- Around line 142-145: Update the autorestore verification around the wrapper
write and Bun.sleep flow to require two matching launcher-content fingerprints
separated by the stability interval, rather than relying only on elapsed
modification time. Add a regression test that changes the launcher between
observations and asserts the operation is deferred without mutating the launcher
or related state.
- Around line 127-129: Add a Windows-specific test fixture alongside “shim
replaced -> next ocx command auto-restores and warns” that creates mixed
shim/replacement launcher siblings, then asserts restoration is deferred and
every sibling remains unchanged. Exercise the tracked sibling set atomically,
including the .cmd and other launcher paths, rather than restoring any
individual sibling during a partial update.
- Around line 127-155: Serialize shim repair transactions across processes using
an interprocess exclusive lock or equivalent conditional-rename protocol in the
repair flow exercised by installCodexShim and the codex-shim status command.
After acquiring the lock, re-fingerprint the wrapper before modifying it so
stale classifications cannot overwrite the real launcher or create recursive
shims. Extend the existing integration coverage with two concurrent processes
and assert the backup codex.opencodex-real remains the external replacement.
- Around line 103-119: The auto-restore state-loading flow must preflight the
state path before reading contents: require a regular file and reject files
exceeding CODEX_SHIM_STATE_MAX_BYTES without invoking the content reader. Update
autoRestoreCodexShim and its state-read helper to validate size/type before
opening or reading, preferably via an opened descriptor to reduce races, and add
coverage proving oversized and non-regular paths never invoke the content
reader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72ac1cb3-97df-4030-b536-71cd611137de
📒 Files selected for processing (25)
README.ja.mdREADME.ko.mdREADME.mdREADME.ru.mdREADME.zh-CN.mddocs-site/src/content/docs/ja/reference/cli.mddocs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/cli.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/cli.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/cli.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/cli.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdsrc/cli/codex-shim-autorestore.tssrc/cli/index.tssrc/codex/shim.tssrc/config.tssrc/types.tsstructure/01_runtime.mdtests/cli-help.test.tstests/codex-shim-autorestore.test.tstests/codex-shim.test.tstests/config.test.ts
| If a completed external Codex update overwrites an installed shim, the next ordinary `ocx` command | ||
| backs up the stable new launcher and restores the shim before dispatch. A launcher that is still | ||
| changing is left untouched and retried later. Repair failures warn without failing the requested | ||
| command; manual fallback: `ocx codex-shim install`. Set `codexShimAutoRestore` to `false`, or set | ||
| `OPENCODEX_CODEX_SHIM_AUTO_RESTORE=0` for a process-level opt-out. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the status side effect consistently across locales.
The CLI leaves ocx codex-shim status eligible for auto-restore, so it can rewrite launcher files before reporting state. Update each page to state this explicitly, or change the policy if status must remain read-only.
docs-site/src/content/docs/reference/cli.md#L375-L379: document that Englishstatusinvocations may perform repair.docs-site/src/content/docs/ko/reference/cli.md#L379-L383: add the same side-effect warning to the Korean flow.docs-site/src/content/docs/ru/reference/cli.md#L400-L405: add the same side-effect warning to the Russian flow.
As per path instructions, translated docs must remain synchronized with actual CLI behavior.
📍 Affects 3 files
docs-site/src/content/docs/reference/cli.md#L375-L379(this comment)docs-site/src/content/docs/ko/reference/cli.md#L379-L383docs-site/src/content/docs/ru/reference/cli.md#L400-L405
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/reference/cli.md` around lines 375 - 379, Clarify
that `ocx codex-shim status` may perform auto-restore and rewrite launcher files
before reporting state, while preserving the existing opt-out and repair
behavior. Apply the same side-effect warning to
docs-site/src/content/docs/reference/cli.md:375-379,
docs-site/src/content/docs/ko/reference/cli.md:379-383, and
docs-site/src/content/docs/ru/reference/cli.md:400-405, translating it
appropriately in the Korean and Russian pages.
Source: Path instructions
| function reclaimStaleRestoreLock(path: string): boolean { | ||
| const before = statFingerprint(path, false); | ||
| if (!before || before.kind !== "file" || Date.now() - before.mtimeMs <= CODEX_SHIM_RESTORE_LOCK_STALE_MS) return false; | ||
| const after = statFingerprint(path, false); | ||
| if (!after || !sameFingerprint(before, after)) return false; | ||
| try { | ||
| unlinkSync(path); | ||
| return true; | ||
| } catch (error) { | ||
| if (fileErrorCode(error) === "ENOENT") return true; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm reclaimStaleRestoreLock is the only site that removes a *live* restore lock by path,
# and inspect every unlink of the restore lock file.
rg -nP -C3 'restoreLockPath|autorestore\.lock|reclaimStaleRestoreLock' src/codex/shim.ts
rg -nP -C2 'unlinkSync' src/codex/shim.tsRepository: lidge-jun/opencodex
Length of output: 4130
🏁 Script executed:
sed -n '705,737p' src/codex/shim.ts
sed -n '718,734p' src/codex/shim.tsRepository: lidge-jun/opencodex
Length of output: 2279
Make stale-lock reclamation atomic src/codex/shim.ts:691-703
reclaimStaleRestoreLock() deletes by unlinkSync(path) after two matching stats. If two processes decide the lock is stale before either unlinks it, the first can remove the old inode, reacquire the lock, and the second can then unlink that fresh lock by path. Both processes can then enter restore work at the same time, so this no longer guarantees mutual exclusion.
Use an atomic reclaim instead: move the stale lock aside with a unique renameSync(...) target and only continue if that rename succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/shim.ts` around lines 691 - 703, The reclaimStaleRestoreLock
function must atomically detach the verified stale lock instead of deleting it
by path. Replace unlinkSync(path) with a renameSync operation to a unique
temporary target, and return true only when that rename succeeds; handle rename
failures consistently, including ENOENT, so a concurrent replacement cannot be
removed.
| test("oversized shim state is bounded, skipped, and warned without loading config", () => { | ||
| const home = mkdtempSync(join(tmpdir(), "ocx-shim-oversized-state-")); | ||
| const oldHome = process.env.OPENCODEX_HOME; | ||
| try { | ||
| process.env.OPENCODEX_HOME = home; | ||
| const statePath = join(home, "codex-shim.json"); | ||
| writeFileSync(statePath, Buffer.alloc(CODEX_SHIM_STATE_MAX_BYTES + 1, 0x20)); | ||
| const before = readFileSync(statePath); | ||
| const { deps, warnings, readConfigCalls } = cliDeps({ status: "healthy" }, { | ||
| restore: autoRestoreCodexShim, | ||
| }); | ||
|
|
||
| maybeAutoRestoreCodexShim("status", ["status"], deps); | ||
|
|
||
| expect(warnings).toEqual([expect.stringContaining("exceeds the 1 MiB startup limit")]); | ||
| expect(readConfigCalls()).toBe(0); | ||
| expect(readFileSync(statePath)).toEqual(before); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject unsafe state files before reading their contents.
This test passes even if autoRestoreCodexShim reads the entire oversized file and only then checks its length. Per the PR objectives, state reads are synchronous and currently lack file-type/size preflight, so an arbitrary-size file can allocate memory and a FIFO can block CLI startup. Validate a regular file and its size before opening/reading it (preferably from the opened descriptor to reduce races), and add a test proving the content reader is never invoked for oversized and non-regular state paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-shim-autorestore.test.ts` around lines 103 - 119, The
auto-restore state-loading flow must preflight the state path before reading
contents: require a regular file and reject files exceeding
CODEX_SHIM_STATE_MAX_BYTES without invoking the content reader. Update
autoRestoreCodexShim and its state-read helper to validate size/type before
opening or reading, preferably via an opened descriptor to reduce races, and add
coverage proving oversized and non-regular paths never invoke the content
reader.
Source: Path instructions
| test("shim replaced -> next ocx command auto-restores and warns", async () => { | ||
| if (process.platform === "win32") return; | ||
| const binDir = mkdtempSync(join(tmpdir(), "ocx-shim-activation-bin-")); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Add Windows sibling-set restoration coverage.
Returning on Windows leaves the .cmd/other launcher-sibling path untested, despite the identified risk of repairing one sibling while another is mid-update. Add a Windows fixture that creates mixed shim/replacement siblings and assert restoration defers without changing any sibling; the implementation must treat the tracked sibling set atomically.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-shim-autorestore.test.ts` around lines 127 - 129, Add a
Windows-specific test fixture alongside “shim replaced -> next ocx command
auto-restores and warns” that creates mixed shim/replacement launcher siblings,
then asserts restoration is deferred and every sibling remains unchanged.
Exercise the tracked sibling set atomically, including the .cmd and other
launcher paths, rather than restoring any individual sibling during a partial
update.
Source: Path instructions
| test("shim replaced -> next ocx command auto-restores and warns", async () => { | ||
| if (process.platform === "win32") return; | ||
| const binDir = mkdtempSync(join(tmpdir(), "ocx-shim-activation-bin-")); | ||
| const home = mkdtempSync(join(tmpdir(), "ocx-shim-activation-home-")); | ||
| const wrapper = join(binDir, "codex"); | ||
| const backup = join(binDir, "codex.opencodex-real"); | ||
| const replacement = "#!/bin/sh\necho externally updated codex\n"; | ||
| const oldPath = process.env.PATH; | ||
| const oldHome = process.env.OPENCODEX_HOME; | ||
| try { | ||
| process.env.PATH = binDir; | ||
| process.env.OPENCODEX_HOME = home; | ||
| writeFileSync(wrapper, "#!/bin/sh\necho original codex\n", "utf8"); | ||
| chmodSync(wrapper, 0o755); | ||
| expect(installCodexShim().installed).toBe(true); | ||
| writeFileSync(wrapper, replacement, "utf8"); | ||
| chmodSync(wrapper, 0o755); | ||
| await Bun.sleep(120); | ||
|
|
||
| const result = spawnSync(process.execPath, [join(import.meta.dir, "..", "src", "cli", "index.ts"), "codex-shim", "status"], { | ||
| encoding: "utf8", | ||
| env: { ...process.env, PATH: binDir, OPENCODEX_HOME: home }, | ||
| }); | ||
|
|
||
| expect(result.status).toBe(0); | ||
| expect(result.stderr).toContain("automatic repair after Codex update"); | ||
| expect(result.stdout).toContain("wrapper shim present"); | ||
| expect(readFileSync(wrapper, "utf8")).toContain(SHIM_MARKER); | ||
| expect(readFileSync(backup, "utf8")).toBe(replacement); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Serialize concurrent repair transactions.
This integration test exercises only one ocx process. The unresolved concurrent-repair path allows two processes to classify the same replacement, then overwrite the backup with a shim; that loses the real launcher and can produce recursive shims. Add an interprocess exclusive lock (or an equivalent conditional rename protocol), re-fingerprint after acquiring it, and add a two-process regression test asserting the backup remains the external replacement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-shim-autorestore.test.ts` around lines 127 - 155, Serialize shim
repair transactions across processes using an interprocess exclusive lock or
equivalent conditional-rename protocol in the repair flow exercised by
installCodexShim and the codex-shim status command. After acquiring the lock,
re-fingerprint the wrapper before modifying it so stale classifications cannot
overwrite the real launcher or create recursive shims. Extend the existing
integration coverage with two concurrent processes and assert the backup
codex.opencodex-real remains the external replacement.
Source: Path instructions
| writeFileSync(wrapper, replacement, "utf8"); | ||
| chmodSync(wrapper, 0o755); | ||
| await Bun.sleep(120); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify stable content twice, not just elapsed mtime.
Sleeping after one static write only validates timestamp-age gating. A launcher can still be actively written after its timestamp becomes old enough. Require two matching fingerprints separated by the stability interval, and add a regression that changes launcher content between observations and asserts a deferred, non-mutating result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-shim-autorestore.test.ts` around lines 142 - 145, Update the
autorestore verification around the wrapper write and Bun.sleep flow to require
two matching launcher-content fingerprints separated by the stability interval,
rather than relying only on elapsed modification time. Add a regression test
that changes the launcher between observations and asserts the operation is
deferred without mutating the launcher or related state.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/shim.ts`:
- Around line 748-767: Update reclaimStaleRestoreLock to handle lock directories
where readShimRestoreLockSnapshot returns no valid snapshot: use the directory
mtime, verify it exceeds CODEX_SHIM_RESTORE_LOCK_STALE_MS, confirm the lock
owner is not active when applicable, then remove the abandoned directory safely.
Preserve the existing snapshot comparison and token-specific unlink path for
valid lock records, and ensure tryAcquireShimRestoreLock can recover from empty
or malformed directories.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e3e9cd7-69d3-4419-92d1-1ea9d6078a8c
📒 Files selected for processing (3)
src/codex/shim.tsstructure/01_runtime.mdtests/codex-shim.test.ts
| function reclaimStaleRestoreLock(path: string, beforeDelete?: () => void): boolean { | ||
| const observed = readShimRestoreLockSnapshot(path); | ||
| if (!observed) return false; | ||
| const createdAt = Math.max(observed.record.createdAt, observed.fingerprint.mtimeMs); | ||
| if (Date.now() - createdAt <= CODEX_SHIM_RESTORE_LOCK_STALE_MS) return false; | ||
| if (isProcessAlive(observed.record.pid)) return false; | ||
| const current = readShimRestoreLockSnapshot(path); | ||
| if (!current || !sameShimRestoreLock(observed, current)) return false; | ||
| beforeDelete?.(); | ||
| try { | ||
| // The token is part of the owner filename. Even if the lock directory is | ||
| // replaced after the comparison, this unlink cannot target a successor's | ||
| // differently named owner record. | ||
| unlinkSync(observed.ownerPath); | ||
| rmdirSync(path); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the only lock-dir cleanup paths, and that no age/empty-dir reclamation already exists.
rg -nP -C3 'restoreLockPath|reclaimStaleRestoreLock|rmdirSync|readShimRestoreLockSnapshot' src/codex/shim.ts
# Inspect the omitted guarded-refresh range for any hidden lock cleanup.
sed -n '828,967p' src/codex/shim.tsRepository: lidge-jun/opencodex
Length of output: 8765
Reclaim malformed restore lock directories
src/codex/shim.ts:748-823 can permanently wedge auto-restore after a crash between mkdirSync(path) and owner-file creation at openSync(ownerPath, "wx"): readShimRestoreLockSnapshot() only succeeds when the lock dir contains exactly one *.json owner file, so an empty or malformed directory makes reclaimStaleRestoreLock() return false and tryAcquireShimRestoreLock() defer forever on every later run.
Add a fallback for non-snapshot lock dirs older than CODEX_SHIM_RESTORE_LOCK_STALE_MS (using the directory mtime) so abandoned/garbled lock dirs self-heal instead of blocking restore indefinitely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/shim.ts` around lines 748 - 767, Update reclaimStaleRestoreLock to
handle lock directories where readShimRestoreLockSnapshot returns no valid
snapshot: use the directory mtime, verify it exceeds
CODEX_SHIM_RESTORE_LOCK_STALE_MS, confirm the lock owner is not active when
applicable, then remove the abandoned directory safely. Preserve the existing
snapshot comparison and token-specific unlink path for valid lock records, and
ensure tryAcquireShimRestoreLock can recover from empty or malformed
directories.
2 participants
Summary
Addresses the actionable half of #320: when
@openai/codexis updated via external npm, the replaced launcher silently loses the OpenCodex shim until the user manually runsocx codex-shim install. The CLI now detects the replaced launcher on ordinaryocxstartup and restores the shim automatically.(Part 1 of #320 — the native login gate firing before proxy traffic — remains an upstream Codex CLI limitation and is intentionally out of scope.)
Design
ocx codex-shim install; no duplicated logic.npm install.Tests
+382 test lines across
tests/codex-shim.test.ts,tests/codex-shim-autorestore.test.ts,tests/config.test.ts— named activations for: replaced→restore+warn, intact→zero-overhead, failure→warn-only, opt-out, sibling-drift rollback, npm-in-progress defer, and never-fresh-install guards.Security review
Credential-path-adjacent (shim wraps the codex launcher): explicit maintainer security review requested before merge per MAINTAINERS.md. No auth flow changes; writes are confined to shim wrapper paths and their backups.
Design/audit trail:
devlog/_plan/260724_bugfix_train/030_shim_autorestore.md.Refs #320.
Summary by CodeRabbit
codexShimAutoRestore(default on) andOPENCODEX_CODEX_SHIM_AUTO_RESTORE=0to disable auto-restore; skips during uninstall/remove flows.ocx codex-shim install.