diff --git a/reme/config/benchmark.yaml b/reme/config/benchmark.yaml index 99cb05ef..9212092c 100644 --- a/reme/config/benchmark.yaml +++ b/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: diff --git a/reme/steps/common/python_execute.py b/reme/steps/common/python_execute.py index b98b48db..6da5d441 100644 --- a/reme/steps/common/python_execute.py +++ b/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, "" diff --git a/tests/unit/test_common_steps.py b/tests/unit/test_common_steps.py index 1c6aa63d..b9ec3aa3 100644 --- a/tests/unit/test_common_steps.py +++ b/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()) diff --git a/tests/unit/test_reme_cli.py b/tests/unit/test_reme_cli.py index 13d74cbd..2fbc73fd 100644 --- a/tests/unit/test_reme_cli.py +++ b/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 = {}