Fix login-shell probe cleanup#2124
Conversation
![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: 1d480a59e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let override = environment["CODEX_CLI_PATH"], | ||
| FileManager.default.isExecutableFile(atPath: override) | ||
| { | ||
| return override |
There was a problem hiding this comment.
When CODEX_CLI_PATH points at an executable Codex shim installed by npm/fnm/mise and the app or CLI starts with a minimal GUI PATH, this early return skips currentOrCapture, but CodexRPCClient later launches the override through /usr/bin/env with PathBuilder.effectivePATH built from the uncaptured cache. The override itself is executable, yet its shebang such as /usr/bin/env node can no longer find node, so explicit overrides that previously depended on the login-shell PATH fail to start. Consider capturing the login PATH for override launches without using it to locate the Codex binary, or pass the captured PATH through to the launch environment.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: sufficient
|
Codex review: found issues before merge. Reviewed July 13, 2026, 8:05 PM ET / July 14, 2026, 00:05 UTC. Summary Reproducibility: yes. The linked isolated runtime proof and process-leak report provide a high-confidence reproduction of session-escaped helpers surviving cleanup and exhausting process capacity. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Make Darwin descriptor creation and inheritance safety global to the process-spawn boundary, or redesign output-holder cleanup so a transiently inherited probe descriptor can never authorize terminating an unrelated process, then add a concurrent unrelated-spawn regression test. Do we have a high-confidence way to reproduce the issue? Yes. The linked isolated runtime proof and process-leak report provide a high-confidence reproduction of session-escaped helpers surviving cleanup and exhausting process capacity. Is this the best way to solve the issue? No, not yet. Adopting probes into shared process cleanup is focused, but the Darwin pipe-creation window must be eliminated or cleanup must be unable to target unrelated descriptor holders. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 315ff65f7b2a. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (3 earlier review cycles) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc40e4bf37
ℹ️ 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".
| guard withUnsafeMutablePointer(to: &fds, { | ||
| $0.withMemoryRebound(to: Int32.self, capacity: 2) { pipe($0) == 0 } | ||
| }) else { return nil } | ||
|
|
||
| for fd in [fds.read, fds.write] { | ||
| let flags = fcntl(fd, F_GETFD) | ||
| guard flags >= 0, fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0 else { |
There was a problem hiding this comment.
When another subprocess is launched outside ShellCommandLocator during the small window between pipe() and the later fcntl(FD_CLOEXEC), that child can inherit these probe descriptors because shellSpawnLock is local to this helper and does not cover other Process.run()/spawn sites. The new output-holder cleanup then scans for any process holding these pipes and can SIGTERM/SIGKILL an unrelated live child, so this still regresses concurrent app activity on Darwin despite serializing overlapping shell probes.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
2 participants
Summary
#!/usr/bin/env node.posix_spawnPATH probes intoSpawnedProcessGroup, cleaning helpers that escape the original session on timeout and normal exit.POSIX_SPAWN_CLOEXEC_DEFAULT.Root cause
The PATH runner only killed the original process group. Shell startup helpers could call
setsid(), retain a probe output pipe, and survive cleanup. Moving PATH capture later avoided unnecessary work for native overrides, but treating every explicit override alike broke script launchers that depend on login-shell PATH. Pipe descriptors also had a small creation-to-fcntlinheritance window when probes overlapped.Validation
make checkmake testthrough autoreview on fixup commitffe09726cbc40e4bf3make check: cleanpipe2(O_CLOEXEC)through a direct glibc/musl symbol declaration after Swift's Linux modules omitted the function declaration.Screenshots are not applicable because this is subprocess lifecycle behavior.
Out of scope
The separately observed Codex
auth.jsondeletion is unrelated. This patch does not read, write, refresh, or remove Codex authentication state. Validation used isolated homes and test doubles without real account, browser-cookie, or Keychain access.Related
Authored with Claude; maintainer fixups by @steipete.