-
Notifications
You must be signed in to change notification settings - Fork 7.4k
fix(shell-snapshot): detach snapshot from tty #9388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Prevent snapshot shells from inheriting the interactive TTY by - routing stdin to /dev/null - detaching from the controlling terminal (setsid) - tagging snapshots with CODEX_SHELL_SNAPSHOT Refs: openai#9383
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review |
|
Thanks for the contribution. Please fix the CI failure — a simple formatting issue. Just ask codex to run formatting tool locally. You can ignore the Bazel failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ab0ff888a
ℹ️ 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".
Unset CODEX_SHELL_SNAPSHOT after sourcing rc files so it does not get captured by export -p in snapshot output.
|
@codex review |
|
Note To use Codex here, create a Codex account and connect to github. |
|
@etraut-openai Feel free to run the formatting CI and the review again |
|
@etraut-openai Do you know if there's a plan to merge it soon? |
Thanks to the investigations made by * @frantic-openai #9403 * @kfiramar #9388
|
@kfiramar, it's a national holiday in the U.S. today (MLK Day), so most Codex team members are on vacation. We'll try to get it merged later this week. |
|
@kfiramar, thanks for the contribution. We've made other changes recently that address this issue, so this PR is no longer needed. |
Summary
Bug Details
When shell_snapshot is enabled, Codex spawns a login shell (zsh -lc) and sources ~/.zshrc to capture aliases/functions/env. On some setups (e.g., Terminal.app + a .zshrc that runs stty/tput or emits escape sequences), the snapshot shell writes to the live TTY and changes terminal modes. While the Codex TUI is in raw mode, this breaks input handling: Enter/backspace stop working and raw escape sequences like ^[[O appear (often after alt-tab). Disabling shell_snapshot avoids the issue.
Repro Steps (macOS)
Enable
shell_snapshot(TUI/experimental→ “Shell snapshot”, or set[features].shell_snapshot = truein~/.codex/config.tomland restart).Create a minimal zshrc that touches the tty:
Launch Codex with that rc:
When the TUI appears, input is broken (Enter/Backspace don’t work). After alt‑tabbing, you may see raw escape sequences like
^[[O.Confirm that disabling snapshot restores input:
Why It Failed Before
The snapshot subprocess inherited the controlling TTY and ran
.zshrcwhile the TUI was in raw mode. Any tty mode changes or escape output from the snapshot shell hit the live terminal, which flips the terminal state out of raw mode and effectively “breaks” input handling for the running TUI. The failure is config-dependent, so it only shows up when shell_snapshot is enabled and.zshrc(or a plugin) touches the TTY.Fix
Detach the snapshot subprocess from the controlling terminal and avoid inheriting stdin:
This keeps snapshot output capture intact while preventing tty side effects.
Testing
justtool in environment)Issue