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 is already Ready for Review. |
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.
|
|
|
@Wibias The branch moved to current head Two further upgrade-safety gaps were fixed:
Current validation: typecheck passed; focused shim suite 62 passed / 0 failed; privacy scan passed; docs production build passed (221 pages). All correct Codex and CodeRabbit threads are resolved, and the PR is Ready for Review. |
|
✅ Action performedReview finished.
|
|
@codex review |
|
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- HEAD and PR revisions ---'
git rev-parse HEAD
gh pr view 1441 --repo lidge-jun/opencodex --json baseRefOid,headRefOid,baseRefName,headRefName
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 structure ---'
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, launcher discovery, and transaction references ---'
rg -n -C 12 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|launcherShellPath|spawn\(|findCodexOnPath|--version|setCodexShimProbe' src/codex/shim.ts
echo '--- direct executable and probe test references ---'
rg -n -i -C 12 'native executable|direct executable|process\.execPath|symlink|--version|probe|installCodexShim|setCodexShimProbe' tests/codex-shim.test.ts
echo '--- relevant diff ---'
git diff --find-renames --unified=35 "${base_sha}...HEAD" -- src/codex/shim.ts tests/codex-shim.test.tsLength of output: 186190
P1 — Execute the saved launcher directly in the validation probe
spawn(launcherShellPath, [wrapperPath, "--version"], ...)
After the installation transaction moves that launcher to the backup path, the probe runs: The shell attempts to parse the executable. The executable does not receive Run the launcher directly: spawn(wrapperPath, ["--version"], {
detached: true,
env: process.env,
stdio: ["ignore", "ignore", "pipe", "pipe"],
});Remove Add a Unix regression test in
The I completed static inspection on the exact ready head. I could not execute the test suite in this environment. You are interacting with an AI system. |
![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: d5a1ce5b56
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/lifecycle.md`:
- Around line 290-295: The Unix launcher probe currently invokes the saved
launcher through /bin/sh, rejecting native executables and symlinks; update the
saved-launcher validation to execute it directly with --version, preserve
timeout/cleanup checks, and add regression coverage for concrete executables and
symlinks. In docs-site/src/content/docs/reference/cli/lifecycle.md:290-295,
retain the concrete-executable recovery guidance only with the corrected probe
behavior; synchronize the same behavior in
docs-site/src/content/docs/ja/reference/cli/lifecycle.md:184-184,
docs-site/src/content/docs/ko/reference/cli/lifecycle.md:238-243,
docs-site/src/content/docs/ru/reference/cli/lifecycle.md:256-262, and
docs-site/src/content/docs/zh-cn/reference/cli/lifecycle.md:181-181.
In `@src/codex/shim.ts`:
- Around line 1832-1842: The obsolete-shim upgrade path in autoRestoreCodexShim
currently maps both deferred races and terminal refusals to "ineligible". Add an
explicit deferred indicator to refreshObsoleteUnixShims results, use it to
return "deferred" while retaining "ineligible" for safety removal, and extend
the existing concurrent wrapper replacement test to call autoRestoreCodexShim
and assert the deferred status.
- Around line 1500-1509: Replace the field-by-field fingerprint checks in the
staged wrapper validation block with the existing sameFingerprint comparison,
matching the comparison used later in the same function. Preserve the current
error behavior when the staged wrapper is missing or fingerprints differ; only
retain a manual comparison if sameFingerprint includes a rename-sensitive
path-derived field, and document that field with a brief comment.
- Around line 123-153: Update finishAfterStderr so the stderr-drain timeout
marks both stderrEnded and leaseEnded before evaluating completion, allowing
shutdown to finish when no process group remains even if fd 3 is still open. Add
a regression test in tests/codex-shim.test.ts using a launcher that leaks fd 3
into a process exiting before the drain deadline, and assert installation
succeeds without reporting a timeout.
- Around line 747-790: Update rollbackFreshShimInstall to recognize ownership of
a partially written wrapper when writtenWrapperFingerprint is undefined, using
the existing SHIM_MARKER fallback ownership rule from rollbackGuardedRefresh and
rollbackObsoleteUnixShimRefresh. Ensure rollback removes the incomplete wrapper,
restores the original from backup, and does not raise the ownership-changed
error; add a regression test beside the Unix probe-infrastructure rollback test
that fails writeShim after creation and verifies the launcher is restored with
no backup or state file.
- Around line 731-737: In the process-group wait loop around the install probe
cleanup in src/codex/shim.ts, replace the temporary SharedArrayBuffer/Int32Array
and Atomics.wait delay with Bun.sleepSync(10), preserving the existing deadline
and unixProcessGroupAlive checks.
In `@tests/codex-shim.test.ts`:
- Around line 997-1006: Add a shared Unix test helper near the existing Unix
helpers that removes the exact UNIX_SHIM_REVISION_MARKER line and asserts the
resulting wrapper differs from the current content. Replace the inline
current.replace constructions in all four obsolete-shim tests, including the
test around withInstalledShim, with this helper so every test retains the drift
guard.
- Around line 23-36: Harden processState so it checks the spawnSync result
before reading stdout, returning a distinct non-empty failure state when /bin/ps
is unavailable or fails; update waitForProcessStop to treat that failure as
unresolved rather than interpreting an empty string as a stopped process, while
preserving normal process-state polling.
🪄 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: a05982f9-2e9d-4c0a-be4d-2eefc13b157c
📒 Files selected for processing (7)
docs-site/src/content/docs/ja/reference/cli/lifecycle.mddocs-site/src/content/docs/ko/reference/cli/lifecycle.mddocs-site/src/content/docs/reference/cli/lifecycle.mddocs-site/src/content/docs/ru/reference/cli/lifecycle.mddocs-site/src/content/docs/zh-cn/reference/cli/lifecycle.mdsrc/codex/shim.tstests/codex-shim.test.ts
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- HEAD and PR revisions ---'
git rev-parse HEAD
gh pr view 1441 --repo lidge-jun/opencodex --json baseRefOid,headRefOid,baseRefName,headRefName
printf '%s\n' '--- source structure ---'
ast-grep outline src/codex/shim.ts --items all --view expanded
printf '%s\n' '--- probe and launcher discovery ---'
rg -n -C 12 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|launcherShellPath|spawn\(|findCodexOnPath|setCodexShimProbeShellForTests|--version' src/codex/shim.ts
printf '%s\n' '--- direct-executable regression coverage ---'
rg -n -i -C 12 'native executable|direct executable|process\.execPath|symlink|--version|probe|installCodexShim|setCodexShimProbeShellForTests' tests/codex-shim.test.tsLength of output: 69744
P1 — Execute the saved launcher directly in the validation probe
spawn(launcherShellPath, [wrapperPath, "--version"], ...)
Update spawn(wrapperPath, ["--version"], {
detached: true,
env: process.env,
stdio: ["ignore", "ignore", "pipe", "pipe"],
});Remove Add a Unix regression test in The current I completed static inspection on the current head. I did not execute the test suite. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- PR head ---'
gh pr view 1441 --repo lidge-jun/opencodex --json headRefOid --jq .headRefOid
git rev-parse HEAD
echo '--- relevant declarations ---'
ast-grep outline src/codex/shim.ts --items all --view expanded | \
rg -n -C 3 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|findCodexOnPath|setCodexShimProbeShellForTests'
echo '--- probe and target discovery ---'
rg -n -C 12 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|launcherShellPath|spawn\(|findCodexOnPath|setCodexShimProbeShellForTests|--version' src/codex/shim.ts
echo '--- direct-executable coverage ---'
rg -n -i -C 12 'native executable|direct executable|process\.execPath|symlink|--version|probe|installCodexShim|setCodexShimProbeShellForTests' tests/codex-shim.test.tsLength of output: 61569
P1 — Execute the saved launcher directly in the validation probe
spawn(launcherShellPath, [wrapperPath, "--version"], ...)
After installation moves that launcher to the owned backup path, Update spawn(wrapperPath, ["--version"], {
detached: true,
env: process.env,
stdio: ["ignore", "ignore", "pipe", "pipe"],
});Remove Add a Unix regression test in The existing Static inspection completed on the exact ready head. I did not execute the test suite. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- HEAD and PR head ---'
git rev-parse HEAD
gh pr view 1441 --repo lidge-jun/opencodex --json headRefOid,baseRefOid,headRefName,baseRefName
echo '--- source structure ---'
ast-grep outline src/codex/shim.ts --items all --view expanded
echo '--- probe implementation and launcher discovery ---'
rg -n -C 12 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|launcherShellPath|spawn\(|findCodexOnPath|setCodexShimProbeShellForTests|--version' src/codex/shim.ts
echo '--- relevant source ranges ---'
nl -ba src/codex/shim.ts | sed -n '1,220p;400,470p;630,710p'
echo '--- direct-executable regression coverage ---'
rg -n -i -C 10 'native executable|direct executable|process\.execPath|symlink|--version|probe|installCodexShim|setCodexShimProbeShellForTests' tests/codex-shim.test.ts
echo '--- relevant PR diff ---'
base_sha="$(gh pr view 1441 --repo lidge-jun/opencodex --json baseRefOid --jq .baseRefOid)"
git diff --find-renames --unified=25 "${base_sha}...HEAD" -- src/codex/shim.ts tests/codex-shim.test.tsLength of output: 25402
P1 — Execute the saved launcher directly in the validation probe The exact PR head still contains the native-launcher compatibility defect. In
Run the saved launcher directly: spawn(wrapperPath, ["--version"], {
detached: true,
env: process.env,
stdio: ["ignore", "ignore", "pipe", "pipe"],
});Remove Add a Unix regression test in The inspection command ended early because You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36cae1b0c4
ℹ️ 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".
|
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 --jq '{headRefOid,baseRefOid}'
echo '--- checked-out revision ---'
git rev-parse HEAD
echo '--- shim symbols ---'
ast-grep outline src/codex/shim.ts --items all --view expanded | \
rg -n -C 2 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|findCodexOnPath|setCodexShimProbeShellForTests'
echo '--- probe and target discovery ---'
rg -n -C 12 'CODEX_SHIM_INSTALL_PROBE_SCRIPT|probeUnixShimInstall|launcherShellPath|spawn\(|findCodexOnPath|setCodexShimProbeShellForTests|--version' src/codex/shim.ts
echo '--- direct-executable regression coverage ---'
rg -n -i -C 10 'native executable|direct executable|process\.execPath|symlink|--version|probe|installCodexShim|setCodexShimProbeShellForTests' tests/codex-shim.test.ts || trueLength of output: 62877
The gate payload reports completed SHA In
The new regression tests in Update the probe to execute the saved launcher directly: spawn(wrapperPath, ["--version"], {
detached: true,
env: process.env,
stdio: ["ignore", "ignore", "pipe", "pipe"],
});Remove The pull request is not ready until this compatibility case is fixed and the native executable and symlink tests pass. You are interacting with an AI system. |
|
@codex review Focus only on the bounded Unix runtime re-entry contract changed by the preceding fix: one legitimate new-PID nested Codex invocation remains allowed, while repeated argument- or directory-dependent child-process shim redispatch must terminate instead of looping. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- revisions ---'
git rev-parse HEAD
gh pr view 1441 --repo lidge-jun/opencodex --json headRefOid,baseRefOid,headRefName,baseRefName
echo '--- shim structure ---'
ast-grep outline src/codex/shim.ts --items all --view expanded
echo '--- runtime re-entry references ---'
rg -n -C 14 -i 're-entr|reentr|recurs|redispatch|same.pid|same_pid|parent.pid|parent_pid|PPID|pid|directory|cwd|argument|argv|guard|sentinel' src/codex/shim.ts
echo '--- focused runtime re-entry test references ---'
rg -n -C 14 -i 're-entr|reentr|recurs|redispatch|same.pid|same_pid|parent.pid|parent_pid|PPID|pid|directory|cwd|argument|argv|guard|sentinel' tests/codex-shim.test.ts
echo '--- diff after the reported review head parent ---'
git show --stat --oneline --decorate HEAD
git show --format=fuller --find-renames --unified=50 HEAD -- src/codex/shim.ts tests/codex-shim.test.tsLength of output: 138613
The depth guard does not use command arguments or the working directory. Therefore, it bounds both argument-dependent and directory-dependent child-process redispatch.
I completed static inspection of You are interacting with an AI system. |
|
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 '--- changed files at PR scope ---'
base_sha="$(gh pr view 1441 --repo lidge-jun/opencodex --json baseRefOid --jq .baseRefOid)"
git diff --find-renames --name-only "${base_sha}...HEAD"
echo '--- implementation and test structure ---'
ast-grep outline src/codex/shim.ts --items all --view expanded
ast-grep outline tests/codex-shim.test.ts --items all --view expanded
echo '--- Unix runtime re-entry implementation ---'
rg -n -C 16 -i 're-entry|reentry|recurs|redispatch|same-pid|parent.*pid|process\.pid|CODEx.*SHIM|CODEX.*SHIM|argv|cwd|directory' \
src/codex/shim.ts
echo '--- Focused runtime re-entry tests ---'
rg -n -C 16 -i 're-entry|reentry|recurs|redispatch|same-pid|new-pid|nested|child invocation|argument|directory|cwd' \
tests/codex-shim.test.ts
echo '--- Diff for relevant files ---'
git diff --find-renames --unified=45 "${base_sha}...HEAD" -- \
src/codex/shim.ts tests/codex-shim.test.tsLength of output: 113618 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- exact head ---'
git rev-parse HEAD
git show -s --format='%H%n%P%n%s' HEAD
echo '--- runtime guard symbols with line numbers ---'
rg -n -i 'OCX_SHIM_ACTIVE_PID|OCX_SHIM_ACTIVE_DEPTH|REENTRY|re-entry|reentry|redispatch' \
src/codex/shim.ts tests/codex-shim.test.ts
echo '--- generated Unix wrapper implementation ---'
line="$(rg -n 'export function buildUnixCodexShim|function buildUnixCodexShim' src/codex/shim.ts | head -n1 | cut -d: -f1)"
sed -n "${line},$((line + 180))p" src/codex/shim.ts
echo '--- focused runtime tests ---'
for test_name in \
'Unix runtime guard stops argument-dependent child-process redispatch' \
'Unix shim permits a real Codex process to start a new child invocation'
do
line="$(rg -n -F "$test_name" tests/codex-shim.test.ts | head -n1 | cut -d: -f1)"
echo "--- ${test_name} at line ${line} ---"
sed -n "${line},$((line + 90))p" tests/codex-shim.test.ts
done
echo '--- changes introduced by the reviewed head ---'
parent="$(git rev-parse HEAD^)"
git diff --unified=50 "${parent}...HEAD" -- src/codex/shim.ts tests/codex-shim.test.tsLength of output: 35592
The implementation applies this check before argument parsing and does not depend on the working directory. Therefore, argument-dependent and directory-dependent redispatch inherit the same depth limit.
A non-blocking test gap remains: add a directory-dependent redispatch regression. The launcher can change directory and invoke I completed static inspection only. I could not execute the test suite in this environment. You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2bdba007c
ℹ️ 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".
|
Scope decision for the final focused review on The clean-environment child-spawn scenario is a real limitation of cooperative runtime guards, but solving it requires non-cooperative platform-specific containment or a persistent ancestry registry rather than another focused shim fix. That would add new runtime state, PID/concurrency semantics, and cross-platform architecture outside #1439. The concrete reported mise chain preserves inherited state and is covered by the current same-PID, bounded probe, and child-depth regressions. No further implementation or automated re-review is requested for that out-of-scope architecture line. The PR remains Ready for Review with current checks green and the thread resolved with this rationale. |
2 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/devat7779c05748485e45b0011c9e2f6375a1bd124286; the newest upstream-only changes are confined to PR-target workflow code and tests, with no shim overlap.Closes #1439
Verification
bun run prepushon the code-equivalent head before the final upstream-only CL-07/lab rebase: 10,833 passed, 7 skipped, 0 failed; typecheck and privacy scan passed.devhead:bun run typecheckpassed andbun test tests/codex-shim.test.tspassed (56 passed, 0 failed).cd docs-site && bun install --frozen-lockfile && bun run buildpassed and built 221 pages.bun run privacy:scanpassed on the current head.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