Skip to content

feat: add prefix to job uuid #330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add prefix to job uuid #330

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ccd2b44
feat: prefix for uuid
sebhoerl Jul 8, 2025
81b9c80
update name
sebhoerl Jul 8, 2025
4879d6f
Merge branch 'main' into feat/uuid-prefix
cmeesters Jul 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion snakemake_executor_plugin_slurm/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,14 @@ class ExecutorSettings(ExecutorSettingsBase):
"required": False,
},
)
prefix: str = field(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"prefix" is a bit too generic. How about naming the purpose, e.g. "prefix_uuid"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the type should be Optional[str], right?

default="",
metadata={
"help": "Prefix that is added to the job names",
"env_var": False,
"required": False
}
)
Comment on lines +151 to +158
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation for the prefix field.

The prefix field lacks validation for SLURM-safe characters and length limits. Since this prefix will be used in job names and shell commands (e.g., sacct --name), invalid characters could cause command failures.

Consider adding validation in the field definition or in the __post_init__ method:

 prefix: str = field(
     default="",
     metadata={
-        "help": "Prefix that is added to the job names",
+        "help": "Prefix that is added to the job names. Must contain only alphanumeric characters, hyphens, and underscores. Maximum length recommended is 50 characters.",
         "env_var": False,
         "required": False
     }
 )

Additionally, implement validation logic to ensure the prefix contains only SLURM-safe characters (alphanumeric, hyphens, underscores) and doesn't exceed reasonable length limits.

📝 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.

Suggested change
prefix: str = field(
default="",
metadata={
"help": "Prefix that is added to the job names",
"env_var": False,
"required": False
}
)
prefix: str = field(
default="",
metadata={
"help": "Prefix that is added to the job names. Must contain only alphanumeric characters, hyphens, and underscores. Maximum length recommended is 50 characters.",
"env_var": False,
"required": False
}
)
🤖 Prompt for AI Agents 
In snakemake_executor_plugin_slurm/__init__.py around lines 151 to 158, the
prefix field lacks validation for allowed SLURM-safe characters and length
constraints. Add validation logic either in the field definition or in the
class's __post_init__ method to check that the prefix contains only alphanumeric
characters, hyphens, and underscores, and that its length does not exceed a
reasonable limit. Raise a ValueError if the validation fails to prevent invalid
prefixes from being used in job names and shell commands.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the AI, here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmeesters Thank you for confirming! You're absolutely right that input validation for the prefix field is important to prevent potential issues with SLURM job submission and status checking.

Would you like me to implement the validation logic for the prefix field? I can add proper validation that:

  1. Ensures the prefix contains only alphanumeric characters, hyphens, and underscores
  2. Enforces a reasonable length limit (e.g., 50 characters)
  3. Raises a ValueError with a clear message if validation fails

This validation could be added either in the ExecutorSettings.__post_init__() method or in the Executor.__post_init__() method. Let me know if you'd like me to proceed with implementing this validation logic!

🧠 Learnings used 
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.

Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.

Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.

Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.

Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#183
File: .github/workflows/post_to_mastodon.sh:6-17
Timestamp: 2025-01-15T14:23:31.720Z
Learning: The Mastodon instance used by the Snakemake project has a higher character limit than the standard 500 characters, making strict character limit validation unnecessary in the post_to_mastodon.sh script.

Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.

Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.

Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.



# Required:
Expand Down Expand Up @@ -181,7 +189,7 @@ def __post_init__(self, test_mode: bool = False):
# run check whether we are running in a SLURM job context
self.warn_on_jobcontext()
self.test_mode = test_mode
self.run_uuid = str(uuid.uuid4())
self.run_uuid = self.workflow.executor_settings.prefix + str(uuid.uuid4())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Validate prefix before concatenation to prevent command injection.

The direct concatenation of prefix and UUID could introduce security and functionality issues if the prefix contains special characters that break shell commands or exceed SLURM naming constraints.

The prefixed UUID is used in shell commands (line 467: sacct --name {self.run_uuid}). Verify this doesn't break with special characters:

Apply this diff to add basic validation:

 def __post_init__(self, test_mode: bool = False):
     # run check whether we are running in a SLURM job context
     self.warn_on_jobcontext()
     self.test_mode = test_mode
+    # Validate prefix for SLURM-safe characters
+    if self.workflow.executor_settings.prefix:
+        import re
+        if not re.match(r'^[a-zA-Z0-9_-]*$', self.workflow.executor_settings.prefix):
+            raise WorkflowError(
+                f"Invalid prefix '{self.workflow.executor_settings.prefix}'. "
+                "Prefix must contain only alphanumeric characters, hyphens, and underscores."
+            )
+        if len(self.workflow.executor_settings.prefix) > 50:
+            raise WorkflowError(
+                f"Prefix '{self.workflow.executor_settings.prefix}' is too long. "
+                "Maximum length is 50 characters."
+            )
     self.run_uuid = self.workflow.executor_settings.prefix + str(uuid.uuid4())

🏁 Script executed:

#!/bin/bash
# Check for potential issues with special characters in SLURM job names
# Test common problematic characters that could break shell commands

echo "Testing potential problematic characters in SLURM job names:"
echo "Characters that could cause issues: \$, \`, \", ', |, ;, &, >, <, (, ), {, }, [, ], *, ?, \\, space"

# Check SLURM documentation for job name constraints
echo "Checking SLURM job name constraints..."
scontrol show config | grep -i name || echo "Could not retrieve SLURM config"

# Test if there are any length limits
echo "Checking typical SLURM job name length limits..."
man sbatch | grep -A 5 -B 5 "job-name" || echo "Could not retrieve sbatch manual"

Length of output: 948

🏁 Script executed:

#!/bin/bash
set -e

echo "== Searching for ‘sacct’ usage =="
rg -n "sacct" .

echo -e "\n== Searching for ‘run_uuid’ usage =="
rg -n "run_uuid" .

Length of output: 9846

🏁 Script executed:

#!/bin/bash
set -e

echo "== Context around `--job-name` usage in submit_string.py =="
rg -n "--job-name" -A3 -B3 snakemake_executor_plugin_slurm/submit_string.py

Length of output: 717

🏁 Script executed:

#!/bin/bash
set -e

echo "== Searching for `executor_settings` definition =="
rg -n "class .*Settings" -g "snakemake_executor_plugin_slurm/*.py"

echo -e "\n== Searching for prefix usage in executor_settings =="
rg -n "prefix" -g "snakemake_executor_plugin_slurm/*.py"

echo -e "\n== Searching for where executor_settings.prefix is set =="
rg -n "prefix" -n "executor_settings" -g "snakemake_executor_plugin_slurm/*.py"

Length of output: 1023

🏁 Script executed:

#!/bin/bash
set -e

echo "== Searching for subprocess usage =="
rg -n "subprocess\." -n 5 snakemake_executor_plugin_slurm/__init__.py

Length of output: 2623

🏁 Script executed:

#!/bin/bash
set -e

echo "== First 200 lines of submit_string.py =="
sed -n '1,200p' snakemake_executor_plugin_slurm/submit_string.py

Length of output: 2649

🏁 Script executed:

#!/bin/bash
set -e

echo "== Displaying lines 350-430 of __init__.py for sbatch invocation context =="
sed -n '350,430p' snakemake_executor_plugin_slurm/__init__.py

Length of output: 3396

Enforce SLURM-safe prefix in ExecutorSettings to prevent shell injection

The run_uuid (built as prefix + UUID) is interpolated unquoted into shell commands—both in the sbatch --job-name call (submit_string.py) and in sacct --name={run_uuid} (efficiency_report.py and status checks). If a malicious or malformed prefix contains characters like `;|&`` it can break the command or lead to injection.

Please add validation in ExecutorSettings.__post_init__ (snakemake_executor_plugin_slurm/init.py around line 192):

 def __post_init__(self, test_mode: bool = False):
     # run check whether we are running in a SLURM job context
     self.warn_on_jobcontext()
     self.test_mode = test_mode
+    # Validate prefix: only allow alphanumeric, hyphens, underscores; cap length
+    prefix = self.workflow.executor_settings.prefix
+    if prefix:
+        import re
+        if not re.fullmatch(r'[A-Za-z0-9_-]+', prefix):
+            raise WorkflowError(
+                f"Invalid prefix '{prefix}'. "
+                "Allowed characters are letters, digits, hyphens, and underscores."
+            )
+        if len(prefix) > 50:
+            raise WorkflowError(
+                f"Prefix '{prefix}' is too long ({len(prefix)} > 50)."
+            )
     self.run_uuid = prefix + str(uuid.uuid4())
  • File: snakemake_executor_plugin_slurm/__init__.py
  • Lines: ~190–195 (around __post_init__)

This ensures run_uuid is safe wherever it’s passed to the shell.

📝 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.

Suggested change
self.run_uuid = self.workflow.executor_settings.prefix + str(uuid.uuid4())
def __post_init__(self, test_mode: bool = False):
# run check whether we are running in a SLURM job context
self.warn_on_jobcontext()
self.test_mode = test_mode
# Validate prefix: only allow alphanumeric, hyphens, underscores; cap length
prefix = self.workflow.executor_settings.prefix
if prefix:
import re
if not re.fullmatch(r'[A-Za-z0-9_-]+', prefix):
raise WorkflowError(
f"Invalid prefix '{prefix}'. "
"Allowed characters are letters, digits, hyphens, and underscores."
)
if len(prefix) > 50:
raise WorkflowError(
f"Prefix '{prefix}' is too long ({len(prefix)} > 50)."
)
self.run_uuid = prefix + str(uuid.uuid4())
🤖 Prompt for AI Agents 
In snakemake_executor_plugin_slurm/__init__.py around lines 190 to 195, add
validation in the ExecutorSettings.__post_init__ method to ensure the prefix
used for run_uuid contains only safe characters (e.g., alphanumeric,
underscores, and hyphens) to prevent shell injection. Implement a check that
raises an error or sanitizes the prefix if it contains any unsafe characters
like ;|&` and ensure this validation runs before run_uuid is constructed and
used in shell commands.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no separator - even if the default is "", we might want to see a separator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, there is no validation.

self.logger.info(f"SLURM run ID: {self.run_uuid}")
self._fallback_account_arg = None
self._fallback_partition = None
Expand Down