Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a public Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/snakemake_software_deployment_plugin_container/__init__.py`:
- Around line 49-55: The dataclass field "mountpoints: List[str] = field(...)"
uses a mutable default list (default=[]), which triggers dataclasses import-time
errors; change the field to use default_factory=list instead of default so
replace the default=[] with default_factory=list in the field() call for
mountpoints (preserving metadata like "nargs" and "help") to ensure safe
initialization of List[str] instances per dataclass instance.
- Around line 121-126: Replace the use of Python repr(!r) when building the
mountpoints string with shlex.quote to ensure POSIX shell-safe quoting for
getcwd(), SNAKEMAKE_MOUNTPOINT, self.source_cache and containercache (e.g., use
shlex.quote(getcwd()) and shlex.quote(SNAKEMAKE_MOUNTPOINT) instead of !r); for
the loop over self.settings.mountpoints, call shlex.quote on each mountpoint but
first validate the user-provided string contains a ':' separator (raise
ValueError with a clear message if missing) to avoid generating malformed -v
arguments; keep using the existing shlex import.
| mountpoints = ( | ||
| f" -v {getcwd()!r}:{SNAKEMAKE_MOUNTPOINT!r}" # Mount host directory to container | ||
| f" -v {str(self.source_cache)!r}:{containercache!r}" # Mount host cache to container | ||
| ) | ||
| for mountpoint in self.settings.mountpoints: | ||
| mountpoints += f" -v {mountpoint!r}" |
There was a problem hiding this comment.
Use shlex.quote() instead of !r for shell-safe quoting.
!r produces Python repr() output, not POSIX shell quoting. For paths containing both quote types (e.g., /home/it's "here"), repr can emit backslash-escaped single-quoted strings which are invalid in POSIX shell. Since shlex is already imported and used on line 136, use it consistently here as well.
Additionally, user-provided mountpoints on line 126 are interpolated without any format validation — a value missing the : separator will produce a confusing container-runtime error.
🔒 Proposed fix
mountpoints = (
- f" -v {getcwd()!r}:{SNAKEMAKE_MOUNTPOINT!r}" # Mount host directory to container
- f" -v {str(self.source_cache)!r}:{containercache!r}" # Mount host cache to container
+ f" -v {shlex.quote(getcwd())}:{shlex.quote(SNAKEMAKE_MOUNTPOINT)}" # Mount host directory to container
+ f" -v {shlex.quote(str(self.source_cache))}:{shlex.quote(containercache)}" # Mount host cache to container
)
for mountpoint in self.settings.mountpoints:
- mountpoints += f" -v {mountpoint!r}"
+ if ":" not in mountpoint:
+ raise WorkflowError(
+ f"Invalid mountpoint format {mountpoint!r}, expected hostpath:containerpath"
+ )
+ mountpoints += f" -v {shlex.quote(mountpoint)}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mountpoints = ( | |
| f" -v {getcwd()!r}:{SNAKEMAKE_MOUNTPOINT!r}" # Mount host directory to container | |
| f" -v {str(self.source_cache)!r}:{containercache!r}" # Mount host cache to container | |
| ) | |
| for mountpoint in self.settings.mountpoints: | |
| mountpoints += f" -v {mountpoint!r}" | |
| mountpoints = ( | |
| f" -v {shlex.quote(getcwd())}:{shlex.quote(SNAKEMAKE_MOUNTPOINT)}" # Mount host directory to container | |
| f" -v {shlex.quote(str(self.source_cache))}:{shlex.quote(containercache)}" # Mount host cache to container | |
| ) | |
| for mountpoint in self.settings.mountpoints: | |
| if ":" not in mountpoint: | |
| raise WorkflowError( | |
| f"Invalid mountpoint format {mountpoint!r}, expected hostpath:containerpath" | |
| ) | |
| mountpoints += f" -v {shlex.quote(mountpoint)}" |
🤖 Prompt for AI Agents
In `@src/snakemake_software_deployment_plugin_container/__init__.py` around lines
121 - 126, Replace the use of Python repr(!r) when building the mountpoints
string with shlex.quote to ensure POSIX shell-safe quoting for getcwd(),
SNAKEMAKE_MOUNTPOINT, self.source_cache and containercache (e.g., use
shlex.quote(getcwd()) and shlex.quote(SNAKEMAKE_MOUNTPOINT) instead of !r); for
the loop over self.settings.mountpoints, call shlex.quote on each mountpoint but
first validate the user-provided string contains a ':' separator (raise
ValueError with a clear message if missing) to avoid generating malformed -v
arguments; keep using the existing shlex import.
🤖 I have created a release *beep* *boop* --- ## [0.2.0](v0.1.0...v0.2.0) (2026-02-17) ### Features * additional mountpoints ([#5](#5)) ([29ba897](29ba897)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary by CodeRabbit
New Features
Improvements