fix: add shell mode to run_shell_script for pipe and redirect support#21130
fix: add shell mode to run_shell_script for pipe and redirect support#21130ihabshea wants to merge 3 commits intoPrefectHQ:mainfrom
Conversation
Adds a shell parameter to run_shell_script so that commands using shell operators (pipes, redirects, logical operators) work as expected. When shell=True, commands run through asyncio.create_subprocess_shell instead of being tokenized with shlex.split and passed as a list. Defaults to False to preserve existing behavior. Fixes PrefectHQ#11287
ihabshea
requested review from
chrisguidry,
cicdw,
desertaxle and
zzstoatzz
as code owners
![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: 70722058d9
ℹ️ 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".
| process = await asyncio.create_subprocess_shell( | ||
| command, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| cwd=directory, | ||
| env=current_env, | ||
| ) |
There was a problem hiding this comment.
When shell=True, this path launches a subprocess with asyncio.create_subprocess_shell but does not wrap it in a cleanup context or finally block, so cancellation of run_shell_script (for example, flow/deployment cancellation while a long command is running) can leave the child process running after the coroutine exits. The non-shell branch avoids this via open_process, which is explicitly designed to terminate and close processes on exceptions/cancellation, so shell mode currently regresses process lifecycle handling.
Useful? React with 👍 / 👎.
| split_command = shlex.split(command, posix=sys.platform != "win32") | ||
| if not split_command: | ||
| continue | ||
| async with open_process( |
There was a problem hiding this comment.
Please update this to use asyncio.create_subprocess_exec to be consistent with the asyncio.create_subprocess_shell usage.
…, add process cleanup Switches the non-shell code path from open_process to asyncio.create_subprocess_exec for consistency with the shell path. Both paths now share a try/finally block that kills the child process on cancellation or unexpected errors.
2 participants
Currently
run_shell_scriptsplits commands withshlex.splitand passes the result as a list to the subprocess. This means shell operators like pipes (|), redirects (>), and logical operators (&&,||) get treated as literal arguments instead of being interpreted by the shell.This adds a
shellparameter (defaults toFalse) that, when set toTrue, runs the command throughasyncio.create_subprocess_shellinstead. This matches thesubprocess.run(shell=True)interface that was suggested in #21021.Example usage in
prefect.yaml:Default behavior is unchanged. Existing scripts that don't use
shell: truework exactly as before.Tests added for pipes, redirects, logical operators, env vars, directory support, multiline scripts, error propagation, and backward compatibility.
Fixes #11287