fix(codex): stop recursive dynamic-launcher shims - #1441
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Unix Codex shim now embeds revision and recursion guards, validates launchers with bounded ChangesUnix shim safety
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant GeneratedShim
participant SavedLauncher
participant ProcessGroup
participant ShimState
Installer->>GeneratedShim: run bounded --version probe
GeneratedShim->>SavedLauncher: execute saved launcher
SavedLauncher-->>GeneratedShim: return or recurse
GeneratedShim->>ProcessGroup: terminate descendants
GeneratedShim-->>Installer: return probe classification
Installer->>SavedLauncher: restore launcher on failure
Installer->>ShimState: commit state after successful validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. Hygiene✅ Deterministic PR hygiene checks passed. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes on the current head (90ad67a37061cd9a5451f2cd2887f011d259a80d). I validated these against the current implementation and #1439 acceptance criteria.
- Medium — fresh install still commits a recursive launcher; the PID guard only covers same-process
execrecursion.
The new OCX_SHIM_ACTIVE_PID=$$ guard correctly stops the reported mise exec -- codex loop because that chain preserves the PID. However, installCodexShimInternal() still accepts any non-shim launcher from PATH, renames it to .opencodex-real, writes the OpenCodex shim, writes state, and performs no bounded behavioural validation of the saved launcher before committing the installation.
That means #1439's install-safety invariant is still unmet: installation can leave a codex -> saved launcher -> codex cycle behind. It also leaves equivalent recursion forms possible when the dynamic launcher starts a new child process rather than exec-replacing itself, because each child gets a new PID and passes the guard. A launcher that sanitises OCX_SHIM_ACTIVE_PID before redispatch likewise bypasses this guard.
Please keep the runtime fail-fast guard, but also make the install transaction validate the saved launcher before committing. A bounded probe should detect re-entry/timeout and roll back to the original launcher without overwriting an existing backup or persisting shim state. Add install-level regression coverage for same-PID redispatch, child-process redispatch, failed validation rollback, and existing-backup preservation.
- Medium — the fail-fast recovery instruction points to the wrong command.
The new diagnostic tells users to run ocx restore, but ocx restore restores Codex routing/config integration; it does not remove the autostart shim or rename .opencodex-real back into place. The actual shim recovery path is ocx codex-shim uninstall / remove, which calls uninstallCodexShim() and is already excluded from shim auto-restore.
When this guard fires, plain codex is already unusable, so the recovery message needs to direct users to the actual shim removal path. Please update the diagnostic and add a regression assertion for the supported recovery command.
The exact reported mise exec reproduction is fixed, and I found no credential, auth, shell-injection, Windows, or secret-disclosure regression in the patch. The remaining blockers are the incomplete install-safety contract and the incorrect recovery guidance.
|
|
|
✅ Action performedReview finished.
|
![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 `@src/codex/shim.ts`:
- Around line 442-456: The probe in probeUnixShimInstall must isolate the
launcher in its own process group, terminate the entire group on timeout, and
wait until the group has exited before rollbackFreshShimInstall runs. Apply the
probe change at src/codex/shim.ts lines 442-456 and ensure the rollback flow at
src/codex/shim.ts lines 1114-1126 occurs only after termination. Update
tests/codex-shim.test.ts lines 299-330 to fork a child process and assert that
no descendant remains, replacing the direct /bin/sleep-only fixture.
🪄 Autofix
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: 1534a890-481b-4c9e-a353-200ba87e0a78
📒 Files selected for processing (2)
src/codex/shim.tstests/codex-shim.test.ts
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex/shim.ts (1)
1195-1209: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProbe both refresh paths before committing a new shim.
refreshShimFileinsrc/codex/shim.ts:805-826moves a non-shim launcher tobackupPathand writes a shim without callingprobeUnixShimInstall.src/update/index.ts:289-295invokes this refresh automatically after an update. A dynamic launcher such asmise ... codexcan therefore makecodexhit the same-PID guard and exit 126.The
expectedReplacementspath used byautoRestoreCodexShimhas the same gap. It checks stable metadata, then commits the replacement without executing--version. Its rollback handles transaction errors, not an unsafe launcher. Reuse the probe-and-rollback logic in both paths, or defer the refresh and restore the launcher when the probe reports an unsafe 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 `@src/codex/shim.ts` around lines 1195 - 1209, Reuse the existing Unix safety probe and rollback behavior in both refresh paths: update refreshShimFile and the expectedReplacements flow used by autoRestoreCodexShim to probe the saved/original launcher with --version before committing a generated shim. If the probe reports recursive resolution, timeout, or lingering descendants, restore the original launcher and return the same failed-install result/message instead of committing the shim; preserve normal replacement behavior for safe launchers.
🤖 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 498-515: Update probeUnixShimInstall() to catch failures from
terminateUnixProcessGroup(groupId) and return "descendants" or another handled
probe result instead of propagating the exception. Preserve the existing timeout
and marker classification while ensuring rollbackFreshShimInstall() and
writeState() remain reachable after any filesystem mutation.
- Around line 42-73: Update CODEX_SHIM_INSTALL_PROBE_SCRIPT to resolve sleep
through PATH instead of assuming /bin/sleep, and make the watchdog exit without
writing the timeout marker or killing the launcher when sleep cannot start.
Preserve the outer spawnSync timeout as the fallback, and update the related
/bin/sleep and /bin/ps assumptions in the codex shim tests.
In `@tests/codex-shim.test.ts`:
- Line 252: Update the stderr assertion in the relevant codex shim test to
compare against the exported CODEX_SHIM_REENTRY_DIAGNOSTIC constant rather than
a shortened literal. Export or test-only re-export that constant from
src/codex/shim.ts, preserving the existing production classifier and ensuring
the test validates the complete diagnostic generated by buildUnixCodexShim.
- Around line 330-339: Strengthen the installCodexShim regression test to record
the grandchild /bin/sleep PID and the launcher’s process-group ID from the
fixture. Wait for the launcher’s descendant-check synchronization, then assert
both the grandchild is no longer running and process.kill with the negative
group ID fails. Preserve the existing child, shim-file, and install-result
assertions, following the coverage pattern used by the timeout test.
- Line 385: Update the process-group probe assertion near the existing
process.kill call to catch the thrown error and assert its
NodeJS.ErrnoException.code is "ESRCH", rather than using a broad toThrow()
check. Preserve the probe’s intended behavior of confirming the group no longer
exists, consistent with the process-group handling in the referenced shim logic.
---
Outside diff comments:
In `@src/codex/shim.ts`:
- Around line 1195-1209: Reuse the existing Unix safety probe and rollback
behavior in both refresh paths: update refreshShimFile and the
expectedReplacements flow used by autoRestoreCodexShim to probe the
saved/original launcher with --version before committing a generated shim. If
the probe reports recursive resolution, timeout, or lingering descendants,
restore the original launcher and return the same failed-install result/message
instead of committing the shim; preserve normal replacement behavior for safe
launchers.
🪄 Autofix
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: 3d3fb9e9-bef9-4bcd-8fc1-af077bbdb5fe
📒 Files selected for processing (2)
src/codex/shim.tstests/codex-shim.test.ts
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
@Wibias I addressed both requested changes on the current head
Local |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 476: Extend the UnixShimProbeResult type and the --version probe logic
around the recursive classification to return a distinct failure result whenever
result.status is non-zero. Update the rollback handling in the saved-launcher
installation flow to restore the original launcher, remove shim state, and
report that the saved launcher failed its --version probe. Add a regression test
covering a non-zero-exit launcher and asserting both restoration and state
removal.
In `@tests/codex-shim.test.ts`:
- Line 40: Update the cross-platform PATH assignments in withInstalledShim,
including both occurrences, to use the platform delimiter imported from
node:path instead of a hardcoded colon, ensuring installCodexShim can find the
temporary codex wrapper on Windows.
🪄 Autofix
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: ca0cdd58-9ba7-4fab-8f35-70a158f9d970
📒 Files selected for processing (2)
src/codex/shim.tstests/codex-shim.test.ts
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
e2bdba0 to
45734d0
Compare
|
Tip For best results, initiate chat on the files or code changes.
The readiness checklist and deterministic hygiene checks passed. No additional review action was requested. You are interacting with an AI system. |
45734d0 to
aa256f6
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR revision ---'
gh pr view 1441 --repo lidge-jun/opencodex --json headRefOid,baseRefOid,headRefName,baseRefName
echo '--- checked-out revision ---'
git rev-parse HEAD
echo '--- relevant symbols ---'
ast-grep outline src/codex/shim.ts --items all --view expanded | \
rg -n -C 3 'PROBE|probeUnixShimInstall|findCodexOnPath|buildUnixCodexShim|setCodexShimProbe'
echo '--- launcher probe and runtime re-entry implementation ---'
rg -n -C 14 -i 'launcherShellPath|spawn\(|probeUnixShimInstall|findCodexOnPath|OCX_SHIM_ACTIVE_(PID|DEPTH)|re-entry|reentry|redispatch|--version' \
src/codex/shim.ts
echo '--- direct-launcher and re-entry regression coverage ---'
rg -n -C 14 -i 'native executable|direct executable|process\.execPath|symlink|active.depth|re-entry|reentry|redispatch|nested invocation|--version' \
tests/codex-shim.test.ts
echo '--- PR changes in reviewed files ---'
base_sha="$(gh pr view 1441 --repo lidge-jun/opencodex --json baseRefOid --jq .baseRefOid)"
git diff --find-renames --unified=35 "${base_sha}...HEAD" -- \
src/codex/shim.ts tests/codex-shim.test.tsLength of output: 180009
P1 — The validation probe still parses native launchers as shell scripts In spawn(launcherShellPath, [wrapperPath, "--version"], ...)In This executes:
Run the saved launcher directly: spawn(wrapperPath, ["--version"], {
detached: true,
env: process.env,
stdio: ["ignore", "ignore", "pipe", "pipe"],
});Then remove
Static inspection completed. I did not execute the test suite. You are interacting with an AI system. |
|
@Wibias @Ingwannu exact-head refresh is ready for maintainer verification.
The exact-head Cross-platform CI run is blocked at Could a maintainer approve that exact run, and could Wibias re-review the current head once the run is available? |
|
@codex review Please independently review the complete Unix shim fix for #1439, limited to the requested acceptance matrix: direct executables and symlinks, dynamic/mise-style redispatch, transaction rollback and concurrent replacement, bounded timeout behavior, and descendant-process cleanup. Please distinguish blocking correctness defects from broader process-supervisor architecture outside this issue. |
![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: aa256f6d33
ℹ️ 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".
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The latest head resolves the previously opened review threads, and the underlying #1439 bug is real. I also inspected the exact-head Actions failure instead of treating the red aggregate gate as a product regression: Linux shard 3 died while loading unrelated cli-ready and desktop-3p-removal tests with Bun 1.3.14 EEXIST: file already exists, epoll_ctl, followed by Cannot call test()/describe() after the test run has completed. There was no shim assertion failure; macOS, the other three Linux shards, packaging, keyring, and npm-global jobs passed. Do not change the shim implementation to accommodate that runner failure.
Two merge blockers remain on current head aa256f6d:
- The head is not actually on current
dev: the GitHub compare is 3 commits ahead / 15 behinddev@87e3ff9f, despite the PR text naming the oldere2f7f2babase. Rebase or rebuild this focused patch on the latestdev, resolve any overlap with the newly landed CLI-root work, and rerun the focused shim suites plus exact-head cross-platform CI. The current merge-ref run validates the older base and cannot be used as evidence for the integration result. - This PR introduces a substantial process-supervision and rollback architecture but updates only implementation/tests/public lifecycle pages. Add a repository architecture decision in the applicable
structure/document using the required[Decision Log]format. Record why a bounded behavioral probe was chosen over launcher-content detection, why process-group termination plus sentinel/lease evidence is the selected containment model, the five-second cooperative boundary and escaped-descendant limitation, and the rollback/concurrent-replacement invariants. That decision is essential maintenance context for the 1,800-line shim transaction change.
After the rebase, decision record, focused checks, and a rerun of the Bun-infrastructure failure, this remains a high-value bug-fix candidate. Keep the issue and PR open; do not merge the current historical head.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- exact revisions ---'
git rev-parse HEAD
gh pr view 1441 --repo lidge-jun/opencodex --json headRefOid,baseRefOid,headRefName,baseRefName
echo '--- changed files ---'
base_sha="$(gh pr view 1441 --repo lidge-jun/opencodex --json baseRefOid --jq .baseRefOid)"
git diff --find-renames --name-only "${base_sha}...HEAD"
echo '--- source and test symbols ---'
ast-grep outline src/codex/shim.ts --items all --view expanded
ast-grep outline tests/codex-shim.test.ts --items all --view expanded
echo '--- probe, discovery, transaction, and runtime-guard references ---'
rg -n -C 14 -i \
'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|launcherShellPath|spawn\(|findCodexOnPath|rollback|fingerprint|concurrent|timeout|descendant|process group|OCX_SHIM_ACTIVE_(PID|DEPTH)|redispatch|--version' \
src/codex/shim.ts
echo '--- acceptance-matrix regression tests ---'
rg -n -C 12 -i \
'native executable|direct executable|process\.execPath|symlink|mise|redispatch|recursive|rollback|concurrent|timeout|descendant|child.*cleanup|nested invocation|--version' \
tests/codex-shim.test.ts
echo '--- full relevant diff ---'
git diff --find-renames --unified=40 "${base_sha}...HEAD" -- \
src/codex/shim.ts tests/codex-shim.test.tsLength of output: 113782 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- exact head ---'
git rev-parse HEAD
gh pr view 1441 --repo lidge-jun/opencodex --json headRefOid --jq .headRefOid
echo '--- probe script declaration and direct invocation ---'
rg -n -C 30 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|spawn\(launcherShellPath|spawn\(wrapperPath|launcherShellPath|probeLease|lease' src/codex/shim.ts
echo '--- bounded probe implementation ---'
probe_line="$(rg -n '^function probeUnixShimInstall\(' src/codex/shim.ts | head -n1 | cut -d: -f1)"
sed -n "$((probe_line - 15)),$((probe_line + 140))p" src/codex/shim.ts
echo '--- launcher discovery contract ---'
find_line="$(rg -n '^export function findCodexOnPath\(' src/codex/shim.ts | head -n1 | cut -d: -f1)"
sed -n "$((find_line - 15)),$((find_line + 85))p" src/codex/shim.ts
echo '--- native-launcher test blocks ---'
rg -n -A55 -B8 \
'probes a concrete native executable|probes a symlinked native executable' \
tests/codex-shim.test.ts
echo '--- repository diff after the requested review head ---'
git status --shortLength of output: 32937
P1 — Execute the saved launcher directly during the validation probe In launcher = spawn(launcherShellPath, [wrapperPath, "--version"], {In codexShimProbeShellForTests ?? "/bin/sh"The probe therefore runs:
During installation, the launcher moves to the backup path. For a native launcher, such as the Run the saved launcher directly: launcher = spawn(wrapperPath, ["--version"], {
detached: true,
env: process.env,
stdio: ["ignore", "ignore", "pipe", "pipe"],
});Then remove The direct-native and symbolic-link tests already specify the required acceptance cases. The current production code cannot satisfy them. The PR is not ready until these tests pass with direct launcher execution. I completed static inspection only. I could not run the test suite in this environment. You are interacting with an AI system. |
3 participants
Summary
codexback to the wrapper, as observed withmise exec -- codex.--versionprobe before committing state. Unsafe results and unexpected probe exceptions roll back the new launcher, prior owned backup, wrapper, and state transaction.ESRCHhandling.ocx codex-shim uninstallrecovery command.upstream/devate2f7f2ba7f265fe599cb757a345daedaf1f9c829; the final upstream-only Daybreak provider-alias commit has no shim overlap.Closes #1439
Verification
aa256f6d33d589a95b9c82bc30b1de600e544fc1.bun run typecheckpassed on the exact current head.bun test tests/codex-shim.test.ts tests/codex-shim-readiness.test.ts tests/codex-shim-autorestore.test.tspassed on the exact current head: 80 passed, 0 failed.bun run privacy:scanpassed on the exact current head.cd docs-site && bun run buildpassed on the exact current head and built 221 pages.OCX_TEST_NO_QUEUE=1 bun run prepushon the immediately preceding shim-equivalent head ran 10,978 tests: 10,969 passed, 8 skipped, and one static SQLite fixture collided with the pre-existing concurrent test runner. That exact failed case passed immediately in isolation: 1 passed, 0 failed. The typecheck phase passed; privacy passed separately after the test phase.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests