test(codex): centralize shim recursion-guard sanitization - #1961
Merged
Conversation
The previous fix deleted both guard variables at one spawn site. A reviewer pointed out the class of bug survives: the nearby re-entry tests assert status 126, so an inherited depth offset leaves them passing for entirely the wrong reason, and the next test to spawn a shim starts the cycle again. shimChildEnv strips PID, DEPTH and PROBE_ACTIVE in one place, mirroring what probeUnixShimInstall already does before spawning its probe. The second spawn site at line 291 had the same gap - it deleted only the pid - so it was quietly depth-sensitive too. What this buys is that a green run means what it says. Before, these tests measured whatever shim ancestry the developer's shell happened to carry, and were green in CI only because CI has no shimmed ancestor.
Contributor
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
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 (1)
📝 WalkthroughWalkthroughThe test suite adds a shared ChangesShim environment tests
Estimated code review effort: 1 (Trivial) | ~5 minutes 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.
Follow-up to #1960. The previous fix sanitized one spawn site; this centralizes it.
A reviewer noted the class of bug survives a single-site fix: the nearby re-entry tests assert
status === 126, so an inherited depth offset leaves them passing for the wrong reason, and the second spawn site deleted only the pid.shimChildEnvstripsOCX_SHIM_ACTIVE_PID,OCX_SHIM_ACTIVE_DEPTHandOCX_SHIM_PROBE_ACTIVEin one place, mirroring whatprobeUnixShimInstallalready does before spawning its probe.What it buys: a green run means what it says. Before, these tests measured whatever shim ancestry the developer shell carried, and were green in CI only because CI has no shimmed ancestor.
Verification
bun run typecheckpassed.bun test tests/codex-shim.test.ts69 pass, 0 fail.Checklist
Summary by CodeRabbit