Repository navigation
Conversation
If ^C arrives while pexpect is still spawning the child, `child` is never bound and the KeyboardInterrupt handler in ProcessHandler.system() crashed with UnboundLocalError. Initialise `child` to None and, when there is no child yet, print ^C and return -SIGINT like an interrupted command. Closes ipython#12164
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: e1661fd353
ℹ️ 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".
| # Interrupted while spawning: there is no child process to | ||
| # forward ^C to, so report it as killed by SIGINT. | ||
| print("^C", file=sys.stderr, end="") | ||
| return -signal.SIGINT |
There was a problem hiding this comment.
When Ctrl-C lands after pexpect's internal fork has created the shell but before pexpect.spawn() returns, child is still None; this new return skips termination/reaping, records -SIGINT, and lets the rest of the cell proceed while the child can remain running. Delay or guard the interrupt through spawn (or otherwise retain the child handle), and make the regression test interrupt after process creation rather than having the stub raise before one.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
| monkeypatch.delattr(pexpect, "spawnb", raising=False) | ||
| monkeypatch.setattr(pexpect, "spawn", interrupted_spawn) | ||
|
|
||
| status = ProcessHandler().system("true") |
There was a problem hiding this comment.
With the production hunk removed, interrupted_spawn enters the old handler, which raises UnboundLocalError while dereferencing child before the final assertion is reached; the regression therefore errors rather than failing on an assertion. Catch and record that old exception and let the status assertion encode the failure, as required for bug-fix tests.
AGENTS.md reference: AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| @dec.skip_win32 | ||
| def test_system_interrupt_during_spawn(monkeypatch): |
There was a problem hiding this comment.
This test is introduced in the same commit as the implementation, so neither the parent nor this revision provides the required checkout where the new test fails before the fix. Separate the regression and implementation in the mandated order; because this history is already pushed, preserve it and use follow-up commits rather than rebasing it.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
The interrupt-during-spawn test now records the old UnboundLocalError so it fails on its assertion without the fix. Add a test where ^C arrives after pexpect has created the child but before spawn() returns; the child must not be left running. It fails on this commit.
A ^C that arrived after pexpect had forked the shell but before spawn() returned left system() without a handle on the child, so it kept running. Defer SIGINT until spawn() returns and raise it then, so the usual handler forwards ^C to the child and terminates it. Also cope with child.before still being None in that handler.
|
Thanks for the review notes. I pushed two follow-up commits (no rebase, per AGENTS.md):
|
Requested by winshyon
Description
🤖🤖 This PR was prepared with an AI coding agent (see disclosure below).
ProcessHandler.system()inIPython/utils/_process_posix.pybindschildinside thetry:block (child = pexpect.spawn(...)). If theKeyboardInterruptarrives while pexpect is still spawning the child (as seen on the conda-forge aarch64/ppc64le builds intest_system_interrupt),childis never assigned and theexcept KeyboardInterrupt:handler fails with:Fix:
child = Nonebefore thetry:;except KeyboardInterrupt:handler, if there is no child yet there is nothing to forward^Cto or terminate, so print^C(same asgetoutput()does) and return-signal.SIGINT. That keeps the existing convention ofsystem()(a negative number for "terminated by signal N"), so callers such as%sx/!cmdsee a normal interrupted status instead of a crash.The normal path and the "interrupted after spawn" path are unchanged.
A regression test,
tests/test_process.py::test_system_interrupt_during_spawn, monkeypatchespexpect.spawnto raiseKeyboardInterrupt, so the race is reproduced deterministically. It fails with theUnboundLocalErrorabove without the fix.Motivation / disclosure: I had some spare AI-assistant quota (Claude Code) and am using it to try to help projects with open good-first-issues. The change was prepared with Claude Code and reviewed by me. If it doesn't fit, isn't up to your bar, or you'd simply rather not take it — please feel free to close it, no hard feelings at all 🙂
Related issue
Closes #12164
Checklist
tests/test_process.py::test_system_interrupt_during_spawn)CHANGELOG.md/ whatsnew entry — n/a (bug fix;docs/source/whatsnew/pr/is for new features and incompatible changes only)🤖 Generated with Claude Code