fix(test): sanitize both shim recursion-guard variables - #1960
Merged
Conversation
#1891's hold expired four minutes after I wrote it - the author rebased and ticked all four boxes at 02:10:50Z - so it merged as 5c66ad2. Wave 5D is now down to #1889 alone, blocked on maintainer sponsorship. The full suite on the merged tree is 12805 pass, 10 skip, 1 fail. The failure is the Unix shim autostart test failing with status 126, permission denied on exec, and it is environmental rather than a regression: it reproduces solo, it fails identically at the pre-campaign baseline 1208bd2, and all four test shards passed in dev CI for 9eb3a10. The test writes a shim to a temp dir and spawnSyncs it; this sandbox blocks execution from that path. Recorded rather than skipped - the fix is an environment note, not a test change.
The autostart shim test deleted OCX_SHIM_ACTIVE_PID from the child environment but inherited OCX_SHIM_ACTIVE_DEPTH from the parent. A developer running the suite from a shell that was itself launched through an installed Codex shim carries DEPTH=1, so the outer shim started at depth 1 rather than 0, the child re-entry landed on depth 2, and the guard exited 126 with its launcher-loop message. The shim was behaving exactly as designed; the test was starting from a slate it believed was clean. CI never sees this because CI has no shimmed ancestor, which is precisely what made it look like a machine quirk. I had recorded the cause as the sandbox blocking execution from a temp path. That was wrong - a chmod 755 script in mktemp -d runs fine here and /var/folders is not mounted noexec - and a reviewer traced the real mechanism. The devlog now carries the correction, because a plausible-sounding wrong explanation in a durable record is worse than no explanation.
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe child-invocation Codex shim test now clears inherited recursion-guard variables. The execution log records the corrected cause and the final suite result: 12,806 passing tests, 10 skipped tests, and no failures. ChangesCodex shim recursion-guard correction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This was referenced Aug 18, 2026
olddonkey
pushed a commit
to olddonkey/opencodex
that referenced
this pull request
Aug 18, 2026
Gate on dev at 87f7f97: 12807 pass, 10 skip, 0 fail across 826 files, with typecheck and privacy scan green. Promoted 107 commits to preview (a43150c) and main (7979903), both verified by ancestry rather than by the merge reporting success. Recording which PRs did not exist when the campaign started - lidge-jun#1951, lidge-jun#1953, lidge-jun#1955, lidge-jun#1960 and lidge-jun#1961 all came out of auditing the plan rather than executing it. Two of them fix defects I introduced myself, which is the part of this campaign most worth remembering. Every remaining item carries its reason in the table rather than sitting unexplained.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One test fix plus the Wave 5D record.
The test fix.
tests/codex-shim.test.tsdeletedOCX_SHIM_ACTIVE_PIDfrom the childenvironment but inherited
OCX_SHIM_ACTIVE_DEPTHfrom the parent. Anyone running the suite froma shell that was itself launched through an installed Codex shim carries
DEPTH=1, so the outershim starts at depth 1 rather than 0, the child re-entry lands on depth 2, and the recursion
guard exits 126 with its launcher-loop message.
The shim was behaving exactly as designed. The test was starting from a slate it believed was
clean. CI never sees it because CI has no shimmed ancestor — which is precisely what made it look
like a machine quirk rather than an under-sanitized test.
Production code already does the right thing here:
probeUnixShimInstalldeletesPID,DEPTHand
PROBE_ACTIVEbefore spawning its probe. This brings the test in line with the runtime's ownpractice.
I first recorded the cause as "the sandbox blocks execution from a temp path." That was wrong —
a
chmod 755script inmktemp -druns fine, and/var/foldersis notnoexec— and a reviewertraced the real mechanism. The devlog carries the correction, because a plausible-sounding wrong
explanation in a durable record is worse than none.
Wave 5D record. #1897 had already merged and #1836 was already closed before the phase ran.
#1891 landed (
5c66ad205) once its author rebased and ticked its checklist. #1889 remains blockedon maintainer sponsorship of
src/oauth/— a label that records a security review, so not one anagent should apply to unblock a merge.
Verification
bun test tests/codex-shim.test.ts— 69 pass, 0 fail (was 68/1).1208bd25c, and all four shards pass in dev CI run32090176020.Checklist
devSummary by CodeRabbit
Bug Fixes
Tests