Enable ruff ASYNC rules - #7441
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRuff async checks are enabled. Targeted suppressions are added to existing polling loops and an asynchronous SSH subprocess call. Runtime and test control flow remain unchanged. ChangesAsync lint configuration and suppressions
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7441 +/- ##
==========================================
- Coverage 80.68% 80.67% -0.00%
==========================================
Files 581 581
Lines 47068 47068
==========================================
- Hits 37972 37968 -4
- Misses 9096 9100 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| if self.auth_script != 'None': | ||
| result = subprocess.run(self.auth_script, shell=True, capture_output=True, text=True, check=False) | ||
| result = subprocess.run(self.auth_script, shell=True, capture_output=True, text=True, check=False) # noqa: ASYNC221 |
There was a problem hiding this comment.
This looks like a real issue that should be fixed. Quoting from ruff:
ASYNC221
Checks that async functions do not run processes with blocking methods.
Why is this bad?
Blocking an async function via a blocking call will block the entire
event loop, preventing it from executing other tasks while waiting for the
call to complete, negating the benefits of asynchronous programming.
Instead of making a blocking call, use an equivalent asynchronous library or function, like trio.run_process() or anyio.run_process().
Example
import subprocess
async def foo():
subprocess.run(cmd)Use instead:
import asyncio
async def foo():
asyncio.create_subprocess_shell(cmd)
There was a problem hiding this comment.
I think in general to asynchronize this function is good direction for further performance but I don't think it is safe to suspend within this open coroutine without any further changes. In this code we are already pass the if self._is_open: check, so the next coroutine could be another open and open another connection. So we would need a variable expressing that a connection is in the progress of being opened that need to be checked. In 2.10 we anyway wanted to make more functions async, so I think about it after release.
|
|
||
| [tool.ruff.lint] | ||
| ignore = [ | ||
| 'ASYNC240', # blocking-path-method-in-async-function |
There was a problem hiding this comment.
What it does
Checks that async functions do not call blocking os.path or pathlib.Path
methods.
Why is this bad?
Calling some os.path or pathlib.Path methods in an async function will block
the entire event loop, preventing it from executing other tasks while waiting
for the operation. This negates the benefits of asynchronous programming.
Instead, use the methods' async equivalents from
trio.Pathoranyio.Path.
There was a problem hiding this comment.
In order to resolve ASYNC230 and ASYNC240 we would need to take a dependency on a third party library that enables async file IO.
| async def do_pause(): | ||
| calc_node = self.runner.submit(test_processes.WaitProcess) | ||
| while calc_node.process_state != ProcessState.WAITING: | ||
| while calc_node.process_state != ProcessState.WAITING: # noqa: ASYNC110 |
There was a problem hiding this comment.
I suspect that in order to avoid sleeping these tests could be refactored to use the plumpy.process_listener.ProcessListener callback based solution? But I haven't gone deep enough to see if that is viable here.
https://plumpy.readthedocs.io/en/latest/apidoc/plumpy.process_listener.html
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiida/transports/plugins/ssh_async.py`:
- Line 171: Update the subprocess invocation in the authentication flow to pass
auth_script as a single executable argument rather than using shell=True.
Preserve the existing capture_output, text, and check behavior, and rely on
validate_script’s path-only validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f37815f-219e-429c-a747-72cd4389d95c
📒 Files selected for processing (6)
pyproject.tomlsrc/aiida/engine/processes/futures.pysrc/aiida/transports/plugins/ssh_async.pytests/engine/test_daemon.pytests/engine/test_rmq.pytests/engine/test_zeromq.py
|
|
||
| if self.auth_script != 'None': | ||
| result = subprocess.run(self.auth_script, shell=True, capture_output=True, text=True, check=False) | ||
| result = subprocess.run(self.auth_script, shell=True, capture_output=True, text=True, check=False) # noqa: ASYNC221 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A10 -B10 'def validate_script|authentication_script|auth_script' src testsRepository: aiidateam/aiida-core
Length of output: 12873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== surrounding code =="
sed -n '1,220p' src/aiida/transports/plugins/ssh_async.py
echo
echo "== similar subprocess usage =="
rg -n "subprocess\.run\(" src/aiida/transports -g '*.py'Repository: aiidateam/aiida-core
Length of output: 9775
Avoid shell=True for authentication_script.
validate_script only checks that this is an absolute, executable path; it still passes the raw string to a shell, so a path containing shell metacharacters can run extra commands. Use subprocess.run([self.auth_script], ...) if this option is path-only.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 171-171: Dynamic command passed to subprocess with shell=True. Use a command list without shell=True, or use shlex.quote() to sanitize input.
(coderabbit.command-injection.python-shell-true)
🪛 Ruff (0.15.21)
[error] 171-171: subprocess call with shell=True identified, security issue
(S602)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiida/transports/plugins/ssh_async.py` at line 171, Update the subprocess
invocation in the authentication flow to pass auth_script as a single executable
argument rather than using shell=True. Preserve the existing capture_output,
text, and check behavior, and rely on validate_script’s path-only validation.
Source: Linters/SAST tools
2 participants
What it says on the tin, we've got a lot of async code so let's enable linter rules for it!