src/snakemake/workflow.py (1)

1691-1704: Bind the exported module hooks positionally, not by log= keyword.

The runtime path invokes lifecycle hooks as func(logfile) positionally, but these wrappers prefill log by name. That makes module.onstart() / module.onsuccess() / module.onerror() stricter than normal hook execution and can break any hook registered with a differently named first parameter. Prefer positional binding here so the exported callable matches the existing contract.

Suggested change

     def onstart(self, func):
         """Register onstart function."""
         self._onstart = func
-        self.globals["onstart"] = partial(func, log=self.logger_manager.get_logfile())
+        self.globals["onstart"] = partial(func, self.logger_manager.get_logfile())

     def onsuccess(self, func):
         """Register onsuccess function."""
         self._onsuccess = func
-        self.globals["onsuccess"] = partial(func, log=self.logger_manager.get_logfile())
+        self.globals["onsuccess"] = partial(func, self.logger_manager.get_logfile())

     def onerror(self, func):
         """Register onerror function."""
         self._onerror = func
-        self.globals["onerror"] = partial(func, log=self.logger_manager.get_logfile())
+        self.globals["onerror"] = partial(func, self.logger_manager.get_logfile())

🤖 Prompt for AI Agents

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

In `@src/snakemake/workflow.py` around lines 1691 - 1704, The exported hook
wrappers (onstart, onsuccess, onerror) bind the logfile using a keyword argument
(`log=`) which breaks positional calling by the runtime; change the partial
bindings stored in self.globals["onstart"]/["onsuccess"]/["onerror"] to bind the
logfile positionally (e.g., partial(func, self.logger_manager.get_logfile())) so
the callable signature matches the runtime contract while still storing the
original function in self._onstart/_onsuccess/_onerror.

tests/tests.py (1)

3120-3132: ⚠️ Potential issue | 🟡 Minor

Retained tmpdir never gets cleaned up.

Line 3122 disables cleanup so this test can inspect the generated files, but path is never removed afterward. That leaves temp dirs behind across repeated runs; please wrap the assertions in a cleanup path as done in similar tests in this file.

🤖 Prompt for AI Agents

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

In `@tests/tests.py` around lines 3120 - 3132, The temporary directory returned by
run(...) in test_module_onstart_not_in_main_snakefile is left behind because
cleanup=False and no teardown is performed; wrap the assertions in a try/finally
(or context-style) block so that after asserting the two
Path(path)/"onstart_moduleX.log" files do not exist you remove the temp dir
(e.g., with shutil.rmtree or Path(path).rmdir equivalents) in the finally
clause; reference the test function test_module_onstart_not_in_main_snakefile,
the run(...) call that returns path, and the Path(path) usage to locate where to
add the cleanup.

tests/test_module_onstart_not_in_main_snakefile/module1/Snakefile (1)

3-9: Make the lifecycle fixtures hermetic.

This rule never reads its HTTP input, so the new hook tests can fail on GitHub/network availability before they ever exercise the lifecycle behavior. A local dummy input—or no input at all—would make the failure mode deterministic. The same pattern is repeated in the other new module hook fixtures.

♻️ One simple local-only variant

 rule a:
-    input:
-        storage.http("https://raw.githubusercontent.com/snakemake/snakemake/main/LICENSE.md"),
     output:
         "results/test.out",
     shell:
         "echo {config[test]} > {output[0]}"

🤖 Prompt for AI Agents

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

In `@tests/test_module_onstart_not_in_main_snakefile/module1/Snakefile` around
lines 3 - 9, The rule "a" currently declares an external HTTP input
(storage.http(...)) which isn't actually read and introduces network flakiness;
update the rule to be hermetic by replacing the HTTP input with a local dummy
file (e.g., create or reference a small local fixture file like
"inputs/dummy.txt") or remove the input entirely if not needed, and ensure the
shell command still writes to {output[0]}; apply the same change to the other
module hook fixture Snakefiles that use storage.http so all lifecycle fixtures
are local-only and deterministic.

tests/tests.py (1)

3142-3156: ⚠️ Potential issue | 🔴 Critical

path is referenced before assignment – test will raise NameError.

On line 3144, path is passed to prepare_tmpdir() but isn't defined until line 3145. The static analysis tool also flagged this (F821). Looking at similar tests (e.g., test_github_issue1158 at line 2116), prepare_tmpdir() should receive the test directory path directly.

🐛 Proposed fix

 def test_module_onstart_not_in_main_snakefile():
     # check that onstart is not executed, if not in the main snakefile
-    with prepare_tmpdir(path) as tmpdir:
+    with prepare_tmpdir(dpath("test_module_onstart_not_in_main_snakefile")) as tmpdir:
         path = run(
             dpath("test_module_onstart_not_in_main_snakefile"),
             check_results=True,
             cleanup=False,
             tmpdir=tmpdir,
         )
         assert not (
             Path(path) / "onstart_module1.log"
         ).exists(), "onstart should not be executed for module1"
         assert not (
             Path(path) / "onstart_module2.log"
         ).exists(), "onstart should not be executed for module2"

🤖 Prompt for AI Agents

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

In `@tests/tests.py` around lines 3142 - 3156, The test
test_module_onstart_not_in_main_snakefile references path before assignment;
change the prepare_tmpdir call to receive the test directory spec directly and
keep the run(...) assignment inside the context. Concretely, call
prepare_tmpdir(dpath("test_module_onstart_not_in_main_snakefile")) instead of
prepare_tmpdir(path) and then set path =
run(dpath("test_module_onstart_not_in_main_snakefile"), check_results=True,
cleanup=False, tmpdir=tmpdir) inside the with-block so prepare_tmpdir and run
use the correct directory values.