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.
|
|
![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: 90cac9b42d
ℹ️ 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".
90cac9b to
67bb966
Compare
|
Tip For best results, initiate chat on the files or code changes.
The reported readiness checklist and deterministic hygiene checks are complete. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
The readiness checklist is complete. No additional review action was requested. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
The readiness checklist is complete. No additional review action was requested. You are interacting with an AI system. |
Inventory all seven bug-labelled issue/PR pairs against dev 87e3ff9, record each reviewer blocker and red-CI root cause, and write one decade doc per pair. Audit findings beyond the filed reviews: - #1462 (no review yet) still resurrects a deleted provider or custom model when the stale process also edited that row: a persisted MISSING_CONFIG_VALUE is not a plain record, so src/config.ts:2884-2896 skips recursion and live wins. Probed at the exact head, both entity types came back. - #1465 (no review yet) gets its two-phase ordering right, but rollback deletes the scheduler task by name without proving this attempt created it, so a concurrent registration can be destroyed by our cleanup. - #1441's three blockers are already addressed on current head aa256f6; the reviewer judged the older, replaced head a8087d3. - #1461 and #1441 red CI is infrastructure noise (Bun 1.3.14 segfault and epoll_ctl EEXIST, plus the #1302 shard hang), not a contributor regression. Refs #1459 #1453 #1273 #1454 #1449 #1439 #1429
An adversarial reviewer that wrote none of this code audited the integrated branch and returned FAIL on four findings. Each was re-checked against origin/dev before being accepted, because 'this branch has defect X' and 'this branch introduced defect X' are different claims. Three describe behavior this branch did not introduce: - stale config saves dropping a disk-only key and preferring live state on a same-leaf conflict reproduce identically on origin/dev 9ec2e2d, and the same-leaf half is documented policy at src/config.ts:2908; - the name-only scheduler rollback is upstream's rollbackElevatedSchedulerTask (origin/dev:src/service.ts:1001, from 0deda7c), still reached only by the legacy dashboard finalizer; our commit added a nonce-verified path and pointed the install transaction at it; - the shim marker-ownership race is contributor code from #1441 that this branch neither wrote nor corrected. The fourth is ours and is fixed here: the outcome table cited pre-rebase SHAs that now live only on backup/pre-rebase-260811, and the scheduler and shim documents claimed ownership guarantees the reviewer's probes disprove. The table carries the rebased SHAs, and both guarantees are restated as the narrowings they actually are — including the TOCTOU window that remains between the nonce query and the elevated delete. Full audit disposition in 004_independent_audit.md.
|
Landed on Worth saying clearly: the three blockers on this PR were filed against An independent audit of the merged tree then found one real hole underneath all of that, which is now fixed on Also: when the source path is occupied by a file we do not own, the moved-aside backup is now kept rather than deleted — it is the only remaining copy of the user's launcher — and the refusal message names it. Verified: 81 pass / 0 fail across the three shim suites, with the ownership regression red before the fix. Thank you for the depth of iteration here; this was the largest and most careful of the batch. |
4 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/devat316f6758be0bd47c5f8d183bbc797a6dcf2d4c9e, including the CLI root, command-registry, and normalized dispatch-exit integration; the shim patch rebased without conflict.Closes #1439
Verification
67bb96628b375fe202ab2ea4b975a502adefe30f.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 265 pages.OCX_TEST_NO_QUEUE=1 bun run prepushon the pre-rebase 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