fix(python-execute): rename the timeout parameter to survive CLI client kwargs (#587)

benchmark.yaml's python_execute job advertised a `timeout` parameter, but
`timeout` is one of the CLI's client-selection kwargs (reme/reme.py pops it
before the payload is sent), so `reme python_execute code=... timeout=300`
never delivered the timeout to the job. The value silently became the HTTP
client's request timeout while the subprocess kept the 60s default, so
benchmark code that needs longer was killed at 60s with no hint why. HTTP
and MCP callers were unaffected.

Rename the parameter to `python_timeout`, mirroring how the shell job
already avoids the same collision with `shell_timeout`. The step prefers
the new name and still accepts the legacy `timeout` context key so
existing benchmark configs keep working. A new guard test fails if any
job in reme/config/*.yaml or a plugin plugin.yaml declares a
client-reserved parameter name again.

reme/config/benchmark.yaml

@@ -139,7 +139,7 @@ jobs:
         code:
           type: string
           description: "Python code to execute. Print the final result to stdout."
-        timeout:
+        python_timeout:
           type: number
           description: "Execution timeout in seconds; defaults to 60."
       required:

reme/steps/common/python_execute.py

@@ -27,7 +27,9 @@ class PythonExecuteStep(BaseStep):
         assert self.context is not None
 
         code = self.context.get("code", "")
-        timeout, timeout_error = self._parse_timeout(self.context.get("timeout", DEFAULT_TIMEOUT))
+        timeout, timeout_error = self._parse_timeout(
+            self.context.get("python_timeout", self.context.get("timeout", DEFAULT_TIMEOUT)),
+        )
         if not isinstance(code, str) or not code.strip():
             self.context.response.success = False
             self.context.response.answer = "code is required"
@@ -45,7 +47,7 @@ class PythonExecuteStep(BaseStep):
                 {
                     "returncode": result.returncode,
                     "stderr": result.stderr,
-                    "timeout": timeout,
+                    "python_timeout": timeout,
                 },
             )
             return self.context.response
@@ -58,7 +60,7 @@ class PythonExecuteStep(BaseStep):
             {
                 "returncode": result.returncode,
                 "stderr": stderr,
-                "timeout": timeout,
+                "python_timeout": timeout,
             },
         )
         return self.context.response
@@ -94,7 +96,7 @@ class PythonExecuteStep(BaseStep):
         try:
             timeout = float(raw)
         except (TypeError, ValueError):
-            return DEFAULT_TIMEOUT, "timeout must be a positive number"
+            return DEFAULT_TIMEOUT, "python_timeout must be a positive number"
         if timeout <= 0:
-            return DEFAULT_TIMEOUT, "timeout must be a positive number"
+            return DEFAULT_TIMEOUT, "python_timeout must be a positive number"
         return timeout, ""

tests/unit/test_common_steps.py

@@ -134,13 +134,40 @@ def test_python_execute_step_reports_stderr_on_failure():
 def test_python_execute_step_times_out():
     """python_execute converts subprocess timeout into a failed response."""
 
+    async def run():
+        step = PythonExecuteStep()
+        resp = await step(code="import time\ntime.sleep(1)", python_timeout=0.01)
+        assert resp.success is False
+        assert resp.answer == "Python execution timed out after 0.01s"
+        assert resp.metadata["python_timeout"] == 0.01
+        print("✓ test_python_execute_step_times_out passed")
+
+    _run(run())
+
+
+def test_python_execute_step_accepts_legacy_timeout_name():
+    """Configs written before the rename keep their timeout through the legacy key."""
+
     async def run():
         step = PythonExecuteStep()
         resp = await step(code="import time\ntime.sleep(1)", timeout=0.01)
         assert resp.success is False
         assert resp.answer == "Python execution timed out after 0.01s"
-        assert resp.metadata["timeout"] == 0.01
-        print("✓ test_python_execute_step_times_out passed")
+        assert resp.metadata["python_timeout"] == 0.01
+        print("✓ test_python_execute_step_accepts_legacy_timeout_name passed")
+
+    _run(run())
+
+
+def test_python_execute_step_rejects_invalid_timeout():
+    """A non-positive python_timeout fails the call instead of running unbounded."""
+
+    async def run():
+        step = PythonExecuteStep()
+        resp = await step(code="print(1)", python_timeout=0)
+        assert resp.success is False
+        assert resp.answer == "python_timeout must be a positive number"
+        print("✓ test_python_execute_step_rejects_invalid_timeout passed")
 
     _run(run())
 

tests/unit/test_reme_cli.py

@@ -7,6 +7,7 @@ import sys
 from types import SimpleNamespace
 
 import pytest
+import yaml
 
 from reme.components.service import cli_service
 from reme.components.service.cli_service import CliService
@@ -331,6 +332,46 @@ def test_call_server_passes_shell_parameters_as_payload(monkeypatch, capsys):
     assert capsys.readouterr().out == "ok\n"
 
 
+def test_call_server_passes_python_execute_parameters_as_payload(monkeypatch, capsys):
+    """python_execute's timeout parameter avoids the client-side timeout option."""
+    seen = {}
+    _set_client_backend(monkeypatch, _recording_client(seen))
+    monkeypatch.setattr(reme_module, "running_app_config", lambda: None)
+
+    async def run():
+        await reme_module.call_server(
+            "python_execute",
+            backend="http",
+            code="print(1)",
+            python_timeout=5,
+            timeout=1.5,
+        )
+
+    asyncio.run(run())
+
+    assert seen["action"] == "python_execute"
+    assert seen["payload"] == {"code": "print(1)", "python_timeout": 5}
+    assert seen["client_kwargs"] == {"timeout": 1.5}
+    assert capsys.readouterr().out == "ok\n"
+
+
+def test_no_job_parameter_collides_with_client_kwargs():
+    """A job parameter named like a client option could never be set through the CLI."""
+    reserved = set(reme_module._CLIENT_KWARGS) | {"backend"}  # pylint: disable=protected-access
+    repo_root = Path(reme_module.__file__).resolve().parents[1]
+    configs = sorted((repo_root / "reme" / "config").glob("*.yaml"))
+    configs += sorted(repo_root.glob("plugins/*/src/*/plugin.yaml"))
+    for config_path in configs:
+        jobs = yaml.safe_load(config_path.read_text(encoding="utf-8")).get("jobs") or {}
+        for job_name, job in jobs.items():
+            properties = ((job or {}).get("parameters") or {}).get("properties") or {}
+            clash = sorted(set(properties) & reserved)
+            assert not clash, (
+                f"{config_path.relative_to(repo_root)} job '{job_name}' declares client-reserved "
+                f"parameters: {clash}"
+            )
+
+
 def test_call_server_uses_running_plugins_and_their_service_defaults(monkeypatch, capsys):
     """A bare client call can load the Client backend enabled by the running app."""
     seen = {}