src/snakemake/cli.py (1)

1020-1026: Add a real CLI regression for --containerize.

This changed the option from a boolean flag to nargs="?" plus const="dockerfile". The existing containerize coverage in tests/tests_using_conda.py goes through the helper/API path (containerize=True), so it won’t catch parser regressions for bare --containerize versus --containerize apptainer.

Also applies to: 2144-2145

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/cli.py` around lines 1020 - 1026, The argparse change for
"--containerize" (nargs="?" with const="dockerfile") can break bare-flag
parsing; add CLI-level regression tests that exercise both forms. Update
tests/tests_using_conda.py (or add a new test file) to invoke the CLI parser or
run the CLI entry with args ["--containerize"] and ["--containerize",
"apptainer"] and assert the parsed/observed value is "dockerfile" for the bare
flag and "apptainer" for the explicit value; also keep the existing API-path
test that passes containerize=True to ensure that path remains covered. Use the
same parser function or entry-point used by src/snakemake/cli.py so the test
detects parser regressions.

src/snakemake/deployment/containerize.py (2)

115-116: ⚠️ Potential issue | 🟡 Minor

Make Apptainer %files sources location-independent.

The src path reaches this method as a cwd-relative path from relfile(). Apptainer's %files section consumes host paths during build, so the generated .def will fail if written or built from a different directory. Normalize to an absolute path.

Proposed fix

     def copy_file(self, src, dest):
-        self._files.append((src, dest))
+        self._files.append((os.path.abspath(src), dest))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` around lines 115 - 116, The
copy_file method stores cwd-relative sources which makes Apptainer %files
entries location-dependent; update copy_file (and the call site expecting
relfile() inputs) to normalize src to an absolute path before appending (e.g.,
use os.path.abspath or Path(src).resolve()) so the tuple stored in self._files
contains an absolute host path and the generated .def is portable across working
directories.

---

118-121: ⚠️ Potential issue | 🟠 Major

Shell-quote remote downloads in %post to prevent injection.

The url and dest parameters are interpolated directly into shell commands. A URL with & in the query string (or other shell metacharacters) will break or behave unexpectedly. Use shlex.quote() for both operands.

Proposed fix

Add the import at the top of the file:

import shlex

Then update the method:

     def add_remote_file(self, url, dest):
         # Apptainer doesn't have ADD, so we download in %post
-        self._post.append(f"mkdir -p $(dirname {dest})")
-        self._post.append(f"wget -O {dest} {url}")
+        dest_quoted = shlex.quote(str(dest))
+        url_quoted = shlex.quote(str(url))
+        self._post.append(f"mkdir -p $(dirname {dest_quoted})")
+        self._post.append(f"wget -O {dest_quoted} {url_quoted}")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` around lines 118 - 121, The
add_remote_file method currently interpolates url and dest directly into shell
lines, risking shell injection; import shlex at top and wrap both url and dest
with shlex.quote() when building the _post entries in add_remote_file (e.g.,
quote url and dest before embedding into the mkdir and wget commands) so the
generated %post commands are safely shell-escaped.

src/snakemake/deployment/containerize.py (2)

85-87: Consider using spread syntax for list construction.

Static analysis suggests using [*commands, final_cmd] instead of concatenation.

Proposed fix

     def run_combined(self, commands, final_cmd):
-        all_cmds = commands + [final_cmd]
+        all_cmds = [*commands, final_cmd]
         print("\nRUN", " \\\n    ".join(all_cmds))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` around lines 85 - 87, The list
concatenation in run_combined uses commands + [final_cmd] to build all_cmds;
replace this with the spread-style construction to create all_cmds as
[*commands, final_cmd] and then continue using all_cmds (or inline it into the
join) so the behavior is identical but uses the suggested list construction;
update the expression where all_cmds is assigned in the run_combined function.

---

132-148: Add finish() to the ABC with a default no-op.

The finish() method exists only in ApptainerFormat, and the caller uses hasattr(formatter, "finish") to conditionally invoke it. This breaks the abstraction—all formatters should expose the same interface. Adding finish() to ContainerFormat with a default no-op implementation improves type safety and consistency.

Proposed fix

Add to the ABC (around line 55):

     `@abstractmethod`
     def run_combined(self, commands, final_cmd):
         """Print multiple commands combined together (e.g., conda env creation)."""
         pass
+
+    def finish(self):
+        """Finalize output. Override in subclasses that need post-processing."""
+        pass

Then update the caller (line 267-268):

-    # For Apptainer, print the collected sections at the end
-    if hasattr(formatter, "finish"):
-        formatter.finish()
+    # Finalize output (no-op for Docker, prints sections for Apptainer)
+    formatter.finish()

Also applies to: 267-268

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` around lines 132 - 148, Add a
default no-op finish() method to the ContainerFormat abstract base class so all
formatters implement the same interface (mirror the existing
ApptainerFormat.finish behavior), and then change callers that currently use
hasattr(formatter, "finish") to call formatter.finish() directly (remove the
hasattr conditional). Reference the ContainerFormat class to add the method and
the ApptainerFormat.finish implementation to copy semantics from, and update the
call site that currently checks for hasattr(formatter, "finish") to invoke
finish() unconditionally on the formatter instance.

src/snakemake/deployment/containerize.py (2)

242-242: ⚠️ Potential issue | 🟡 Minor

Remove extraneous f prefix from string literal.

This f-string has no placeholders.

Proposed fix

-        formatter.comment(f"Conda environment:")
+        formatter.comment("Conda environment:")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` at line 242, The call to
formatter.comment uses an unnecessary f-string: change formatter.comment(f"Conda
environment:") to a regular string by removing the f prefix so it becomes
formatter.comment("Conda environment:"); update the occurrence in
containerize.py where formatter.comment is invoked to avoid the redundant
f-string.

---

118-121: ⚠️ Potential issue | 🟠 Major

Shell quoting remains incomplete for URLs with single quotes.

The current implementation wraps url and dest in single quotes, which handles & characters but doesn't escape single quotes within the URL itself. If a URL contains a single quote, the shell command will break or potentially allow injection.

Proposed fix using shlex.quote

Add import shlex at the top, then:

     def add_remote_file(self, url, dest):
         # Apptainer doesn't have ADD, so we download in %post
-        self._post.append(f"mkdir -p $(dirname {dest})")
-        self._post.append(f"curl -L '{url}' > '{dest}'")
+        self._post.append(f"mkdir -p $(dirname {shlex.quote(str(dest))})")
+        self._post.append(f"curl -L {shlex.quote(str(url))} -o {shlex.quote(str(dest))}")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` around lines 118 - 121, The
add_remote_file method builds shell lines using raw url and dest which breaks if
either contains single quotes; import shlex and use shlex.quote to safely
shell-escape both values before inserting into the commands in add_remote_file
(e.g., q_url = shlex.quote(url); q_dest = shlex.quote(dest)), then append
f"mkdir -p $(dirname {q_dest})" and f"curl -L {q_url} > {q_dest}" to self._post
so all embedded quotes and special chars are properly escaped.

src/snakemake/deployment/containerize.py (2)

266-268: Consider defining finish() in the ABC with a default no-op.

Using hasattr(formatter, "finish") to conditionally call the method is a code smell. A cleaner approach is to define finish() as a concrete method in ContainerFormat with an empty body, allowing all formatters to call it unconditionally.

Proposed changes

In the ABC:

class ContainerFormat(ABC):
    # ... existing abstract methods ...
    
    def finish(self):
        """Finalize output. Override if the format needs deferred printing."""
        pass

Then at the end of containerize():

-    # For Apptainer, print the collected sections at the end
-    if hasattr(formatter, "finish"):
-        formatter.finish()
+    # Finalize output (e.g., print collected sections for Apptainer)
+    formatter.finish()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` around lines 266 - 268, The current
conditional hasattr check around formatter.finish() is a smell; add a concrete
no-op finish(self) method to the ContainerFormat ABC (so implementations can
override if needed) and then remove the hasattr guard in containerize() so you
call formatter.finish() unconditionally; update the ContainerFormat class to
include a finish method and ensure containerize() uses formatter.finish()
directly (referencing ContainerFormat, finish, and containerize to locate the
changes).

---

85-87: Consider using spread syntax for list concatenation.

Static analysis suggests using [*commands, final_cmd] instead of commands + [final_cmd] for slightly better performance and idiomatic style.

Suggested change

     def run_combined(self, commands, final_cmd):
-        all_cmds = commands + [final_cmd]
+        all_cmds = [*commands, final_cmd]
         print("\nRUN", " \\\n    ".join(all_cmds))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/snakemake/deployment/containerize.py` around lines 85 - 87, In
run_combined, replace the list concatenation expression commands + [final_cmd]
with the spread-style literal [*commands, final_cmd] to be more idiomatic and
slightly faster; update the all_cmds assignment in the run_combined method to
use all_cmds = [*commands, final_cmd] and keep the rest of the function behavior
(the print call) unchanged.