fix: remove trailing whitespace from submitcmd#23
fix: remove trailing whitespace from submitcmd#23ning-y wants to merge 4 commits intosnakemake:mainfrom
Conversation
📝 WalkthroughWalkthroughTrim trailing whitespace from configured shell commands and shell-escape jobscript and jobids: submit, status, sidecar, and cancel command handling now apply Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor as ClusterGenericExecutor
participant Shell as OS Shell / subprocess
User->>Executor: run_job(submitcmd, jobscript, env)
Executor->>Executor: submitcmd_clean = submitcmd.rstrip()
Executor->>Executor: jobscript_quoted = shlex.quote(jobscript)
Executor->>Shell: execute submit (clean + quoted jobscript)
Shell-->>Executor: output (external job id)
Executor-->>User: record external_jobid
alt status check
User->>Executor: check_active_jobs(jobid)
Executor->>Executor: statuscmd_clean = statuscmd.rstrip()
Executor->>Executor: jobid_quoted = shlex.quote(jobid)
Executor->>Shell: execute status (clean + quoted jobid)
Shell-->>Executor: status output
end
alt cancel flow
User->>Executor: cancel_jobs([jobids])
Executor->>Executor: cancel_list = shlex.split(cancel_cmd)
Executor->>Executor: cancel_list.extend(jobids)
Executor->>Shell: run cancel_list
Shell-->>Executor: result
end
alt sidecar launch
User->>Executor: _launch_sidecar(sidecar_cmd)
Executor->>Executor: sidecar_list = shlex.split(sidecar_cmd.rstrip())
Executor->>Shell: Popen(sidecar_list)
Shell-->>Executor: sidecar process
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I've tested that this works using pipenv: Where the And the |
ning-y
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
snakemake_executor_plugin_cluster_generic/__init__.py (3)
201-206: Apply the same trailing-whitespace trim to status_cmd to avoid analogous failures.A trailing newline in status_cmd will also break the shell invocation (the jobid ends up on a new line and is not passed to the status command).
Apply this diff:
- ret = subprocess.check_output( - "{statuscmd} '{jobid}'".format( - jobid=job_info.external_jobid, - statuscmd=self.workflow.executor_settings.status_cmd, - ), + ret = subprocess.check_output( + "{statuscmd} '{jobid}'".format( + jobid=job_info.external_jobid, + statuscmd=self.workflow.executor_settings.status_cmd.rstrip(), + ),
301-305: Trim cancel_cmd as well to prevent command-not-found errors when trailing whitespace is present.With shell=False, a newline or trailing spaces become part of the executable name and will fail to execute.
Apply this diff:
- subprocess.check_call( - [self.workflow.executor_settings.cancel_cmd] + chunk, + subprocess.check_call( + [self.workflow.executor_settings.cancel_cmd.rstrip()] + chunk,
371-376: Trim sidecar_cmd to avoid failures when trailing whitespace is present.With shell=False and a string arg, trailing whitespace becomes part of the program path.
Apply this diff:
- process = subprocess.Popen( - self.workflow.executor_settings.sidecar_cmd, + process = subprocess.Popen( + self.workflow.executor_settings.sidecar_cmd.rstrip(), stdout=subprocess.PIPE, shell=False, encoding="utf-8", )
🧹 Nitpick comments (1)
snakemake_executor_plugin_cluster_generic/__init__.py (1)
153-156: Harden quoting of the jobscript argument to avoid shell expansions and edge cases.Double quotes allow shell expansions (e.g., $(), backticks) if they ever appear in the path. shlex.quote ensures the path is passed literally and also handles spaces safely.
Apply this diff:
- '{submitcmd} "{jobscript}"'.format( - submitcmd=submitcmd.rstrip(), jobscript=jobscript - ), + '{submitcmd} {jobscript}'.format( + submitcmd=submitcmd.rstrip(), jobscript=shlex.quote(jobscript) + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_cluster_generic/__init__.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
snakemake_executor_plugin_cluster_generic/__init__.py
🔇 Additional comments (1)
snakemake_executor_plugin_cluster_generic/__init__.py (1)
152-156: Good fix: trimming trailing whitespace from submitcmd avoids silent job submission failures.Using submitcmd.rstrip() directly where the shell command is built precisely addresses the issue described in the PR and prevents the shell from treating a trailing newline as command termination.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
snakemake_executor_plugin_cluster_generic/__init__.py (1)
151-158: Fix double-quoting of jobscript (breaks submission).You’re passing shlex.quote(jobscript) into a format string that already wraps it with double quotes, resulting in an argument that literally contains single quotes around the path. With shell=True, this produces a single argument value like '/path/script.sh' (quotes included), which will likely break submitters expecting a plain path. Drop the outer quotes and rely on shlex.quote.
Apply this diff:
- '{submitcmd} "{jobscript}"'.format( - submitcmd=submitcmd.rstrip(), jobscript=shlex.quote(jobscript) - ), + '{submitcmd} {jobscript}'.format( + submitcmd=submitcmd.rstrip(), jobscript=shlex.quote(jobscript) + ),
🧹 Nitpick comments (3)
snakemake_executor_plugin_cluster_generic/__init__.py (3)
201-206: Use shlex.quote for jobid instead of manual single quotes.Manual single quotes won’t escape embedded single quotes (rare but possible), and it’s inconsistent with the submit path. Prefer shlex.quote and remove the surrounding quotes.
Apply this diff:
- ret = subprocess.check_output( - "{statuscmd} '{jobid}'".format( - jobid=job_info.external_jobid, - statuscmd=self.workflow.executor_settings.status_cmd.rstrip(), - ), + ret = subprocess.check_output( + "{statuscmd} {jobid}".format( + jobid=shlex.quote(job_info.external_jobid), + statuscmd=self.workflow.executor_settings.status_cmd.rstrip(), + ),
300-305: Nit: split cancel_cmd into argv when shell=False.When shell=False, passing a single string with spaces as argv[0] will fail if cancel_cmd contains arguments (e.g., "scancel -p queue"). Using shlex.split is safer while preserving your rstrip() fix.
Apply this diff:
- subprocess.check_call( - [self.workflow.executor_settings.cancel_cmd.rstrip()] + chunk, + subprocess.check_call( + shlex.split(self.workflow.executor_settings.cancel_cmd.rstrip()) + chunk, shell=False, timeout=cancel_timeout, env=env, )
370-376: Nit: split sidecar_cmd into argv when shell=False.Similar to cancel_cmd: with shell=False, a string with spaces is treated as a single executable name. Split the command string to support arguments.
Apply this diff:
- process = subprocess.Popen( - self.workflow.executor_settings.sidecar_cmd.rstrip(), + process = subprocess.Popen( + shlex.split(self.workflow.executor_settings.sidecar_cmd.rstrip()), stdout=subprocess.PIPE, shell=False, encoding="utf-8", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
snakemake_executor_plugin_cluster_generic/__init__.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
snakemake_executor_plugin_cluster_generic/__init__.py
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snakemake_executor_plugin_cluster_generic/__init__.py (2)
153-155: Fix is correct: trimming submit_cmd and quoting jobscript prevents silent submits with trailing newlinesThis directly addresses issue #22. Using submitcmd.rstrip() and shlex.quote(jobscript) is the right mitigation for the reported failure mode.
One subtle edge case to consider: if users end their submit_cmd with a line-continuation backslash, rstrip() will remove the newline and the trailing backslash will escape the inserted space before {jobscript}, accidentally concatenating the jobscript path to the previous token. To guard against that, prefer inserting a newline when the cleaned submit_cmd ends with a backslash.
Proposed minimal change:
- '{submitcmd} {jobscript}'.format( - submitcmd=submitcmd.rstrip(), jobscript=shlex.quote(jobscript) - ), + ( + f"{submitcmd.rstrip()}{'\n' if submitcmd.rstrip().endswith('\\') else ' '}" + f"{shlex.quote(jobscript)}" + ),Follow-up:
- Please verify behavior with a submit_cmd that intentionally ends with a trailing backslash (line continuation) to ensure it still works as intended.
- If you’d prefer a clearer implementation, I can refactor this into local variables above the call site for readability. Want me to push that?
225-227: Bug: joining integers in status_cmd_kills causes a TypeError",".join(self.status_cmd_kills) will raise at runtime because list items are ints. Low-impact and unrelated to this PR, but an easy correctness fix.
Apply:
- ",".join(self.status_cmd_kills), + ",".join(map(str, self.status_cmd_kills)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
snakemake_executor_plugin_cluster_generic/__init__.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
snakemake_executor_plugin_cluster_generic/__init__.py
🔇 Additional comments (3)
snakemake_executor_plugin_cluster_generic/__init__.py (3)
202-205: Good hardening: rstrip() on status_cmd and quoting jobidApplying .rstrip() to status_cmd and shlex.quote() to the jobid is a solid improvement (avoids the same trailing-whitespace trap and closes injection holes via job IDs).
Please confirm that existing custom status commands do not rely on a trailing newline for behavior (unlikely), and that quoting the jobid does not break any status scripts that expect unquoted job IDs.
302-302: Safer invocation for cancel_cmd via shlex.split + shell=FalseThis is a good security and robustness improvement. It also naturally ignores trailing whitespace.
This changes semantics if users previously relied on shell features (pipes, redirections) in --cluster-cancel. If that’s not officially supported (it probably shouldn’t be), we’re fine. Otherwise, please document that cancel_cmd must be a single executable with arguments (no shell composition), or add a backward-compatibility note in the README.
372-372: Sidecar command: switch to argv form (shlex.split) is sensibleRunning the sidecar with shell=False and a tokenized argv is more predictable and avoids the newline issue.
As with cancel_cmd, this breaks configurations that used shell constructs in sidecar_cmd. If that’s unsupported, consider clarifying in help text/README that sidecar_cmd must be a plain command path plus args.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
snakemake_executor_plugin_cluster_generic/__init__.py (3)
159-161: Prefer splitlines() for platform-agnostic line splittingUsing .splitlines() handles both LF and CRLF without leaving stray carriage returns.
Apply this diff:
- .decode() - .split("\n") + .decode() + .splitlines()
302-303: Potential backward-compat concern: cancel_cmd now runs without a shellSwitching to shell=False with shlex.split() is safer, but will break configurations that rely on shell metacharacters (pipes, redirections, env var expansion, etc.) inside --cluster-cancel. If users have such commands configured, cancellations may silently fail to execute as intended.
Two options:
- Keep current behavior but document that cancel_cmd must be a plain executable + args (no shell metachars).
- Or add a small fallback: detect shell metachars and use shell=True only in that case (still quoting job IDs).
Proposed minimal fallback within this block:
- subprocess.check_call( - shlex.split(self.workflow.executor_settings.cancel_cmd) + chunk, - shell=False, - timeout=cancel_timeout, - env=env, - ) + cmd_str = self.workflow.executor_settings.cancel_cmd.rstrip() + # Use shell only if the command appears to need it (pipes, redirects, etc.). + if any(ch in cmd_str for ch in "|&;<>()$`\\\"' \n*?[]#~=%"): + cmd = "{} {}".format( + cmd_str, " ".join(shlex.quote(j) for j in chunk) + ) + subprocess.check_call( + cmd, shell=True, timeout=cancel_timeout, env=env + ) + else: + subprocess.check_call( + shlex.split(cmd_str) + chunk, + shell=False, + timeout=cancel_timeout, + env=env, + )
372-372: Sidecar invocation now uses shell=False — verify commands that rely on shell featuresshlex.split(...), shell=False is the right default for safety and correctness, but any sidecar_cmd that includes shell constructs (pipes, redirects, env expansion) will no longer work.
If you need to maintain compatibility with such configurations, add a similar conditional fallback:
- process = subprocess.Popen( - shlex.split(self.workflow.executor_settings.sidecar_cmd), - stdout=subprocess.PIPE, - shell=False, - encoding="utf-8", - ) + cmd_str = self.workflow.executor_settings.sidecar_cmd.rstrip() + use_shell = any(ch in cmd_str for ch in "|&;<>()$`\\\"' \n*?[]#~=%") + process = subprocess.Popen( + cmd_str if use_shell else shlex.split(cmd_str), + stdout=subprocess.PIPE, + shell=use_shell, + encoding="utf-8", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
snakemake_executor_plugin_cluster_generic/__init__.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
snakemake_executor_plugin_cluster_generic/__init__.py
🔇 Additional comments (3)
snakemake_executor_plugin_cluster_generic/__init__.py (3)
153-155: Trailing-whitespace fix and safer jobscript quoting — good changeUsing submitcmd.rstrip() directly addresses the reported failure with trailing newlines, and shlex.quote(jobscript) prevents issues with spaces/special chars in the jobscript path while preserving shell semantics for submitcmd.
202-205: Status command: trimming and quoting are correctstatus_cmd.rstrip() removes problematic trailing whitespace and shlex.quote() for jobid avoids injection/word-splitting issues while still allowing complex status_cmd via shell=True.
226-226: Fix join on integers in status_cmd_kills — good catch",".join(map(str, ...)) prevents TypeError and logs as intended.
|
Now that I look at it again, it is a bit strange that |
1 participant
This should fix #22: trailing newlines in the
submitcmd(provided by the user ascluster-generic-submit-cmd) cause the job submission to fail silently as the trailing newline terminates the command insubprocess.run(..., shell=True)before the job script.snakemake-executor-plugin-cluster-generic/snakemake_executor_plugin_cluster_generic/__init__.py
Lines 152 to 158 in ce5fc82
Summary by CodeRabbit