core: prevent shell_snapshot from inheriting stdin #9735
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@etraut-openai ping — all required checks are green, and Codex review came back clean. When you have a moment, could you take a look? (For context: Bazel is marked SKIPPED on fork PRs since |
|
@swordfish444, we're pretty far behind on code reviews at the moment. The entire Codex team is very busy on higher-priority work. This is in the queue, and we'll get to this eventually. |
.github/workflows/bazel.yml
Outdated
| cancel-in-progress: ${{ github.ref_name != 'main' }} | ||
| jobs: | ||
| test: | ||
| # This workflow relies on `secrets.BUILDBUDDY_API_KEY`, which is not available for PRs |
There was a problem hiding this comment.
Bazel is not required for now in the CI so please drop this
This reverts commit 69eef17.
|
@jif-oai Done — dropped the Bazel workflow change (reverted). PR now only contains the |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@etraut-openai Totally understand the review backlog. I’ve been doing some issue triage (closing obvious dupes/empty reports + consolidating threads) to help reduce noise. If there are any specific issue clusters you’d like support on (e.g. identify confirmed dupes / gather repros), point me at them and I’ll focus there. |
|
@swordfish444, thanks I appreciate your contributions! |
4 participants
Fixes #9559.
When
shell_snapshotruns, it may execute user startup files (e.g..bashrc). If those files read from stdin (or if stdin is an interactive TTY under job control), the snapshot subprocess can block or receiveSIGTTIN(as reported over SSH).This change explicitly sets
stdintoStdio::null()for the snapshot subprocess, so it can't read from the terminal.Regression test added that would hang/timeout without this change.
Tests:
ulimit -n 4096 && cargo test -p codex-core.cc @dongdongbh @etraut-openai