Preserve model backend suffix in generated tasks#120
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #120 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 25 25
Lines 6325 6390 +65
=====================================
- Misses 6325 6390 +65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Warning Review limit reached
More reviews will be available in 14 minutes and 34 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a ChangesDynamic model filename resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@dpti/hti.py`:
- Around line 1510-1515: The _graph_link_command function constructs a shell
command by directly interpolating graph_relpath and model_file into an ln -s
command without proper quoting, which creates a security and stability issue if
these paths contain spaces or shell metacharacters. Fix this by using shell
quoting (such as shlex.quote) to properly escape both the graph_relpath source
argument and the model_file destination argument in the return statement, and
add the -f flag to the ln -s command to make it idempotent. Apply the same fix
to the similar code mentioned in the comment at lines 1547-1550.
In `@dpti/mti.py`:
- Around line 339-343: The model_file variable is being interpolated directly
into shell ln -s commands without proper quoting, creating a security and
robustness issue. In the shell command fragments where link_model is assigned
(around lines 339 and 341, and also in the similar block at lines 374-380), wrap
both instances of the model_file variable with double quotes in the ln -s
commands to properly escape shell-sensitive characters. Additionally, add the -f
flag to the ln -s command to make the linking idempotent, so it will overwrite
existing links without error. Apply these same quoting and -f flag changes to
all occurrences of link_model assignments that use model_file.
In `@dpti/ti.py`:
- Around line 1081-1085: The model_file variable is directly interpolated into
the shell command string without proper quoting, creating a vulnerability to
command injection or path parsing issues if the filename contains special
characters or spaces. In the command string that includes the ln -s symlink
creation, wrap both occurrences of model_file with appropriate shell quotes
(single or double quotes) to properly escape the path. This applies to the
conditional command assignment where model_file is used twice in the symlink
command.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ad7ce01-416a-4024-9b69-d9f051ce24f1
📒 Files selected for processing (18)
dpti/equi.pydpti/gdi.pydpti/hti.pydpti/hti_ice.pydpti/hti_liq.pydpti/hti_water.pydpti/lib/utils.pydpti/mti.pydpti/old_equi.pydpti/relax.pydpti/ti.pytests/test_equi_make_task.pytests/test_gdi_make_task.pytests/test_hti_make_task.pytests/test_lib_utils.pytests/test_ti_make_task.pyworkflow/DpFreeEnergy.pyworkflow/DpFreeEnergyWater.py
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dpti/ti.py (1)
1081-1084:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShell quoting is still bypassable for crafted filenames.
At Line 1082, wrapping
model_filein double quotes is not enough if the basename contains quotes; this can still break command parsing/injection safety. Please shell-escape both path operands before interpolation.Suggested patch
def run_task(task_name, machine_file): + import shlex @@ - command=( - f'ln -sf "../{model_file}" "{model_file}"; {mdata["command"]} -in in.lammps' - if model_file - else f"{mdata['command']} -in in.lammps" - ), + command=( + f"ln -sf {shlex.quote(f'../{model_file}')} {shlex.quote(model_file)}; {mdata['command']} -in in.lammps" + if model_file + else f"{mdata['command']} -in in.lammps" + ),🤖 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 `@dpti/ti.py` around lines 1081 - 1084, The command construction at the `ln -sf` statement does not properly shell-escape the model_file variable before interpolation, which allows shell injection if the filename contains special characters like quotes. Use a shell escaping function (such as shlex.quote() in Python) to properly escape both the symlink source path (the quoted "../{model_file}" part) and the symlink destination path (the quoted "{model_file}" part) before interpolating them into the f-string command. This ensures that any special characters in the filename cannot break out of the quoted context or inject arbitrary commands.
🤖 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.
Duplicate comments:
In `@dpti/ti.py`:
- Around line 1081-1084: The command construction at the `ln -sf` statement does
not properly shell-escape the model_file variable before interpolation, which
allows shell injection if the filename contains special characters like quotes.
Use a shell escaping function (such as shlex.quote() in Python) to properly
escape both the symlink source path (the quoted "../{model_file}" part) and the
symlink destination path (the quoted "{model_file}" part) before interpolating
them into the f-string command. This ensures that any special characters in the
filename cannot break out of the quoted context or inject arbitrary commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 305627ba-f5f6-43e1-820f-717d3362e822
📒 Files selected for processing (3)
dpti/hti.pydpti/mti.pydpti/ti.py
🚧 Files skipped from review as they are similar to previous changes (2)
- dpti/mti.py
- dpti/hti.py
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
1 participant
Summary
model.pth->graph.pthTests
PYTHONPATH=.. pytest -q test_lib_utils.py test_hti_make_task.py test_gdi_make_task.py test_equi_make_task.py test_ti_make_task.py test_hti_liq_make_task.py test_hti_liq_gen_lammps_input.py test_hti_water_gen_lammps_input.py test_ti_water_gen_lammps_input.py test_hti_gen_lammps_input.pypython -m ruff check --select I,F --ignore F841 dpti/lib/utils.py dpti/equi.py dpti/gdi.py dpti/hti.py dpti/hti_water.py dpti/hti_liq.py dpti/hti_ice.py dpti/ti.py dpti/mti.py dpti/relax.py dpti/old_equi.py tests/test_lib_utils.py tests/test_hti_make_task.py tests/test_gdi_make_task.py tests/test_ti_make_task.py tests/test_equi_make_task.py workflow/DpFreeEnergy.py workflow/DpFreeEnergyWater.pypython -m compileall -q dpti workflow tests/test_lib_utils.py tests/test_hti_make_task.py tests/test_gdi_make_task.py tests/test_ti_make_task.py tests/test_equi_make_task.pyFull
pytest -qcurrently reports116 passed, 1 error; the remaining error is an existing pytest collection issue intests/test_hti_ff_spring.py:54, where a module-level test function has aselfparameter.Summary by CodeRabbit
Release Notes
New Features
graph.pbextensions) and are correctly propagated through job/task setup, linking, and submission forwarding.Bug Fixes
graph.pb).Tests