diff --git a/plugins/home/README.md b/plugins/home/README.md index 61325120..a8f61ebb 100644 --- a/plugins/home/README.md +++ b/plugins/home/README.md @@ -15,6 +15,7 @@ contributes the `auto_bash` job, which can be run with The job defaults to read-only command validation and a 60-second timeout. Set `is_read_only=false` to disable command validation. Supplying `read_only_paths` or `writable_paths` enables a file sandbox (Bubblewrap on Linux, Seatbelt on -macOS), with sandbox paths identical to their host paths. Linux defaults to a -4 GiB cgroup memory limit; macOS has no memory limit and warns when a nonzero -`memory_limit_bytes` value is supplied. +macOS), with sandbox paths identical to their host paths. On Linux, the +`memory_limit_mib` cgroup limit defaults to 4096 MiB; 0 disables it. Supplying +the limit requires a path policy. macOS has no memory limit and warns when a +nonzero value is supplied. diff --git a/plugins/home/src/reme_home/auto_bash.py b/plugins/home/src/reme_home/auto_bash.py index 1b3b2ff6..e68b41a9 100644 --- a/plugins/home/src/reme_home/auto_bash.py +++ b/plugins/home/src/reme_home/auto_bash.py @@ -1,11 +1,13 @@ """Run allowlisted shell commands without an approval prompt.""" +import asyncio + from agentscope.message import TextBlock, ToolResultState from agentscope.permission import PermissionBehavior, PermissionContext, PermissionDecision from agentscope.tool import Bash, ToolChunk -from reme.utils.file_sandbox import create_file_sandbox_backend from reme.steps.base_step import BaseStep +from reme.steps.common.file_sandbox import file_sandbox_for_step _READ_COMMANDS = { ":": {}, @@ -53,7 +55,9 @@ unset GIT_EXTERNAL_DIFF GIT_TRACE GIT_TRACE_SETUP GIT_TRACE_PERFORMANCE unset GIT_TRACE_PACK_ACCESS GIT_TRACE_PACKET GIT_TRACE_CURL GIT_TRACE2 GIT_TRACE2_EVENT GIT_TRACE2_PERF export GIT_OPTIONAL_LOCKS=0 GIT_PAGER=/bin/cat GIT_NO_LAZY_FETCH=1 GIT_TERMINAL_PROMPT=0 git() { - if command git -c trace2.normalTarget=0 -c trace2.eventTarget=0 -c trace2.perfTarget=0 \ + # These metadata queries do not read file contents through Git filters. + if [ "$1" != rev-parse ] && [ "$1" != ls-files ] && \ + command git -c trace2.normalTarget=0 -c trace2.eventTarget=0 -c trace2.perfTarget=0 \ config --get-regexp '^filter\..*\.(clean|smudge|process)$' > /dev/null; then echo 'Permission denied: Git repositories with external filters require unrestricted mode.' >&2 return 1 @@ -158,19 +162,26 @@ class AutoBash(Bash): if "$" in command or "`" in command: return False try: - from safecmd import CmdSpec, validate # pylint: disable=import-outside-toplevel - from safecmd.bashxtract import extract_commands, parse_bash # pylint: disable=import-outside-toplevel - from safecmd.core import normalize_dest # pylint: disable=import-outside-toplevel + from safecmd import CmdSpec # pylint: disable=import-outside-toplevel + from safecmd.bashxtract import ( # pylint: disable=import-outside-toplevel + check_types, + collect_ops, + collect_redirects, + parse_bash, + visit_stmts, + ) + from safecmd.core import normalize_dest, validate_cmd # pylint: disable=import-outside-toplevel - if not _literal_shell(parse_bash(command), command): + ast = parse_bash(command) + check_types(ast) + if not _literal_shell(ast, command): return False specs = {CmdSpec(name, denied=options.get("denied")) for name, options in _READ_COMMANDS.items()} - validate(command, cmds=specs, dests=_SAFE_DESTINATIONS) - commands, operators, redirects = extract_commands(command) - if any(not _allowed_arguments(tokens) for tokens in commands): + commands = visit_stmts(ast.get("Stmts", []), command) + if any(not validate_cmd(tokens, specs) or not _allowed_arguments(tokens) for tokens in commands): return False - return "=" not in operators and all( - normalize_dest(destination) == "/dev/null" for _, destination in redirects + return "=" not in collect_ops(ast) and all( + normalize_dest(destination) in _SAFE_DESTINATIONS for _, destination in collect_redirects(ast, command) ) except Exception: # pylint: disable=broad-exception-caught # Parser failures and missing shfmt must never allow execution. @@ -178,7 +189,7 @@ class AutoBash(Bash): async def check_read_only(self, tool_input: dict) -> bool: """Report the same policy used by the execution guard.""" - return self.is_read_only and self._is_read_only(tool_input.get("command", "")) + return self.is_read_only and await asyncio.to_thread(self._is_read_only, tool_input.get("command", "")) async def check_permissions(self, tool_input: dict, _context: PermissionContext) -> PermissionDecision: """Resolve permission automatically, without an ASK decision.""" @@ -194,7 +205,9 @@ class AutoBash(Bash): async def call(self, command: str, description: str = "", timeout: int = 60000): """Apply the read-only guard when configured, then run the command.""" - if self.is_read_only and (not isinstance(command, str) or not self._is_read_only(command)): + if self.is_read_only and ( + not isinstance(command, str) or not await asyncio.to_thread(self._is_read_only, command) + ): yield ToolChunk( content=[TextBlock(text="Permission denied: only read-only shell commands are allowed.")], state=ToolResultState.ERROR, @@ -215,9 +228,6 @@ class AutoBashStep(BaseStep): command = self.context.get("command", "") timeout = self.context.get("timeout", self.kwargs.get("timeout", 60000)) is_read_only = self.context.get("is_read_only", self.kwargs.get("is_read_only", True)) - read_only_paths = self.context.get("read_only_paths", self.kwargs.get("read_only_paths")) - writable_paths = self.context.get("writable_paths", self.kwargs.get("writable_paths")) - memory_limit_bytes = self.context.get("memory_limit_bytes", self.kwargs.get("memory_limit_bytes")) if not isinstance(command, str) or not command.strip(): self.context.response.success = False self.context.response.answer = "command is required" @@ -232,16 +242,7 @@ class AutoBashStep(BaseStep): return self.context.response try: - backend = ( - create_file_sandbox_backend( - cwd=self.workspace_path, - read_only_paths=[] if read_only_paths is None else read_only_paths, - writable_paths=[] if writable_paths is None else writable_paths, - memory_limit_bytes=memory_limit_bytes, - ) - if read_only_paths is not None or writable_paths is not None - else None - ) + backend = file_sandbox_for_step(self) except (TypeError, ValueError, RuntimeError) as exc: self.context.response.success = False self.context.response.answer = str(exc) diff --git a/plugins/home/src/reme_home/plugin.yaml b/plugins/home/src/reme_home/plugin.yaml index df5159b8..77ad1584 100644 --- a/plugins/home/src/reme_home/plugin.yaml +++ b/plugins/home/src/reme_home/plugin.yaml @@ -30,10 +30,10 @@ application_defaults: type: array items: { type: string } description: "Writable host paths; sandbox paths match host paths" - memory_limit_bytes: + memory_limit_mib: type: integer minimum: 0 - description: "Linux cgroup memory limit in bytes (default 4 GiB); macOS warns and ignores nonzero values" + description: "Linux cgroup memory limit in MiB (default 4096; 0 disables it); requires a path policy; macOS warns and ignores nonzero values" required: - command steps: diff --git a/plugins/home/tests/test_auto_bash.py b/plugins/home/tests/test_auto_bash.py index 0b254c99..21d6c42c 100644 --- a/plugins/home/tests/test_auto_bash.py +++ b/plugins/home/tests/test_auto_bash.py @@ -1,7 +1,9 @@ """Shell job and its optional read-only execution guard.""" +import asyncio import inspect import subprocess +import threading from importlib.metadata import EntryPoint from pathlib import Path @@ -139,6 +141,16 @@ async def test_unsupported_platform_runs_without_paths_but_rejects_path_policy(t assert restricted.answer == "No file sandbox backend for unsupported" +@pytest.mark.asyncio +@pytest.mark.parametrize("limit", [0, 1, -1]) +async def test_step_rejects_memory_limit_without_path_policy(tmp_path, limit): + """An explicitly supplied limit cannot be ignored by host execution.""" + step = AutoBashStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) + response = await step(command="echo should-not-run", is_read_only=False, memory_limit_mib=limit) + assert response.success is False + assert response.answer == "memory_limit_mib requires read_only_paths or writable_paths to enable the sandbox" + + @pytest.mark.asyncio async def test_step_configuration_can_disable_read_only(tmp_path): """Step configuration applies when the job does not supply the flag.""" @@ -174,6 +186,28 @@ async def test_permission_decisions_never_ask(tmp_path): assert denied.behavior == PermissionBehavior.DENY +@pytest.mark.asyncio +async def test_read_only_parsing_does_not_block_event_loop(tmp_path, monkeypatch): + """A slow synchronous parser runs in a worker thread during permission checks.""" + started = threading.Event() + release = threading.Event() + + def slow_parse(_command): + started.set() + release.wait(timeout=2) + return True + + tool = AutoBash(cwd=tmp_path) + monkeypatch.setattr(tool, "_is_read_only", slow_parse) + task = asyncio.create_task(tool.check_read_only({"command": "echo hello"})) + try: + assert await asyncio.wait_for(asyncio.to_thread(started.wait), timeout=1) + assert not task.done() + finally: + release.set() + assert await asyncio.wait_for(task, timeout=1) + + @pytest.mark.asyncio async def test_unrestricted_tool_allows_commands_without_read_only_check(tmp_path, monkeypatch): """The unrestricted tool never invokes the read-only parser.""" @@ -244,6 +278,7 @@ def test_home_manifest_declares_step_and_job(): assert job["steps"] == [{"backend": "auto_bash_step"}] assert job["parameters"]["properties"]["read_only_paths"]["type"] == "array" assert job["parameters"]["properties"]["writable_paths"]["type"] == "array" + assert job["parameters"]["properties"]["memory_limit_mib"]["minimum"] == 0 def test_home_plugin_registers_job_and_step(monkeypatch): @@ -340,6 +375,36 @@ async def test_git_configured_helpers_cannot_write(tmp_path, helper): assert all(chunk.state != ToolResultState.ERROR for chunk in chunks) +@pytest.mark.asyncio +async def test_global_git_lfs_config_allows_metadata_queries(tmp_path, monkeypatch): + """A global filter alone must not block Git queries that never use it.""" + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", str(repo)], check=True, capture_output=True) + (repo / "note.txt").write_text("hello\n", encoding="utf-8") + subprocess.run(["git", "add", "note.txt"], cwd=repo, check=True, capture_output=True) + + global_config = tmp_path / "global.gitconfig" + subprocess.run( + ["git", "config", "--file", str(global_config), "filter.lfs.process", "/bin/false"], + check=True, + capture_output=True, + ) + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(global_config)) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + tool = AutoBash(cwd=repo) + + for command, expected in (("git rev-parse --git-dir", ".git"), ("git ls-files", "note.txt")): + chunks = [chunk async for chunk in tool.call(command)] + assert chunks + assert all(chunk.state != ToolResultState.ERROR for chunk in chunks) + assert expected in "".join(block.text for chunk in chunks for block in chunk.content) + + chunks = [chunk async for chunk in tool.call("git diff")] + assert chunks[-1].state == ToolResultState.ERROR + assert "external filters" in chunks[-1].content[0].text + + @pytest.mark.asyncio async def test_allow_and_denied_command_options_are_combined(tmp_path, monkeypatch): """Allow constrains the first argument while denied still checks later flags.""" diff --git a/reme/config/benchmark.yaml b/reme/config/benchmark.yaml index 6fda30a9..46d61455 100644 --- a/reme/config/benchmark.yaml +++ b/reme/config/benchmark.yaml @@ -142,10 +142,10 @@ jobs: python_timeout: type: number description: "Execution timeout in seconds; defaults to 60." - memory_limit_bytes: + memory_limit_mib: type: integer minimum: 0 - description: "Linux memory limit in bytes (default 4 GiB); macOS ignores nonzero values" + description: "Linux memory limit in MiB (default 4096; 0 disables it); requires a path policy; macOS ignores nonzero values" required: - code steps: diff --git a/reme/steps/common/file_sandbox.py b/reme/steps/common/file_sandbox.py new file mode 100644 index 00000000..fd516899 --- /dev/null +++ b/reme/steps/common/file_sandbox.py @@ -0,0 +1,23 @@ +"""Resolve an invocation's optional file sandbox from injected job parameters.""" + +from ...utils.file_sandbox import create_file_sandbox_backend +from ...utils.file_sandbox.file_sandbox_backend import FileSandboxBackend +from ..base_step import BaseStep + + +def file_sandbox_for_step(step: BaseStep) -> FileSandboxBackend | None: + """Keep path and memory policy selection identical across executor steps.""" + assert step.context is not None + read_only_paths = step.context.get("read_only_paths", step.kwargs.get("read_only_paths")) + writable_paths = step.context.get("writable_paths", step.kwargs.get("writable_paths")) + memory_limit_mib = step.context.get("memory_limit_mib", step.kwargs.get("memory_limit_mib")) + if read_only_paths is None and writable_paths is None: + if memory_limit_mib is not None: + raise ValueError("memory_limit_mib requires read_only_paths or writable_paths to enable the sandbox") + return None + return create_file_sandbox_backend( + cwd=step.workspace_path, + read_only_paths=[] if read_only_paths is None else read_only_paths, + writable_paths=[] if writable_paths is None else writable_paths, + memory_limit_mib=memory_limit_mib, + ) diff --git a/reme/steps/common/python_execute.py b/reme/steps/common/python_execute.py index d3a1794f..19e85bb0 100644 --- a/reme/steps/common/python_execute.py +++ b/reme/steps/common/python_execute.py @@ -5,10 +5,10 @@ import sys from dataclasses import dataclass from typing import Any -from ...utils.file_sandbox import create_file_sandbox_backend from ...utils.file_sandbox.file_sandbox_backend import FileSandboxBackend from ..base_step import BaseStep from ...components import R +from .file_sandbox import file_sandbox_for_step DEFAULT_TIMEOUT = 60.0 @@ -41,20 +41,8 @@ class PythonExecuteStep(BaseStep): self.context.response.answer = timeout_error return self.context.response - read_only_paths = self.context.get("read_only_paths", self.kwargs.get("read_only_paths")) - writable_paths = self.context.get("writable_paths", self.kwargs.get("writable_paths")) - memory_limit_bytes = self.context.get("memory_limit_bytes", self.kwargs.get("memory_limit_bytes")) try: - backend = ( - create_file_sandbox_backend( - cwd=self.workspace_path, - read_only_paths=[] if read_only_paths is None else read_only_paths, - writable_paths=[] if writable_paths is None else writable_paths, - memory_limit_bytes=memory_limit_bytes, - ) - if read_only_paths is not None or writable_paths is not None - else None - ) + backend = file_sandbox_for_step(self) except (TypeError, ValueError, RuntimeError) as exc: self.context.response.success = False self.context.response.answer = str(exc) diff --git a/reme/utils/file_sandbox/__init__.py b/reme/utils/file_sandbox/__init__.py index 4be0d28a..898d31aa 100644 --- a/reme/utils/file_sandbox/__init__.py +++ b/reme/utils/file_sandbox/__init__.py @@ -7,12 +7,12 @@ from .path_bubble_backend import PathBubbleBackend from .path_seatbelt_backend import PathSeatbeltBackend -def create_file_sandbox_backend(*, memory_limit_bytes: int | None = None, **kwargs) -> FileSandboxBackend: +def create_file_sandbox_backend(*, memory_limit_mib: int | None = None, **kwargs) -> FileSandboxBackend: """Construct the platform backend; never silently execute without isolation.""" if sys.platform.startswith("linux"): - limit = 4 * 1024**3 if memory_limit_bytes is None else memory_limit_bytes - return PathBubbleBackend(memory_limit_bytes=limit, **kwargs) + limit = 4096 if memory_limit_mib is None else memory_limit_mib + return PathBubbleBackend(memory_limit_mib=limit, **kwargs) if sys.platform == "darwin": - limit = 0 if memory_limit_bytes is None else memory_limit_bytes - return PathSeatbeltBackend(memory_limit_bytes=limit, **kwargs) + limit = 0 if memory_limit_mib is None else memory_limit_mib + return PathSeatbeltBackend(memory_limit_mib=limit, **kwargs) raise RuntimeError(f"No file sandbox backend for {sys.platform}") diff --git a/reme/utils/file_sandbox/file_sandbox_backend.py b/reme/utils/file_sandbox/file_sandbox_backend.py index c1aa31e4..4810856c 100644 --- a/reme/utils/file_sandbox/file_sandbox_backend.py +++ b/reme/utils/file_sandbox/file_sandbox_backend.py @@ -88,7 +88,17 @@ class FileSandboxBackend(BackendBase): ) -> ExecResult: """Execute an argv list with a timeout inside the platform sandbox.""" process = await self._start_process(command, cwd or self._cwd) - communication = asyncio.create_task(process.communicate()) + return await self._communicate(process, timeout=timeout) + + async def _communicate( + self, + process: asyncio.subprocess.Process, + *, + data: bytes | None = None, + timeout: float | None = None, + ) -> ExecResult: + """Drain output and release process resources on success, timeout, or cancellation.""" + communication = asyncio.create_task(process.communicate() if data is None else process.communicate(data)) try: stdout, stderr = await asyncio.wait_for(asyncio.shield(communication), timeout=timeout) except TimeoutError: @@ -152,13 +162,6 @@ class FileSandboxBackend(BackendBase): self._cwd, stdin=asyncio.subprocess.PIPE, ) - communication = asyncio.create_task(process.communicate(data)) - try: - _stdout, stderr = await asyncio.shield(communication) - except BaseException: - await self._stop_process(process, communication) - raise - finally: - self._release_process(process) - if process.returncode != 0: - raise OSError(stderr.decode(errors="replace")) + result = await self._communicate(process, data=data) + if not result.ok(): + raise OSError(result.stderr.decode(errors="replace")) diff --git a/reme/utils/file_sandbox/path_bubble_backend.py b/reme/utils/file_sandbox/path_bubble_backend.py index 676365dd..418b99c7 100644 --- a/reme/utils/file_sandbox/path_bubble_backend.py +++ b/reme/utils/file_sandbox/path_bubble_backend.py @@ -30,11 +30,11 @@ class PathBubbleBackend(FileSandboxBackend): cwd: str | os.PathLike[str], read_only_paths: list[str], writable_paths: list[str], - memory_limit_bytes: int = 4 * 1024**3, + memory_limit_mib: int = 4096, ) -> None: - if isinstance(memory_limit_bytes, bool) or not isinstance(memory_limit_bytes, int) or memory_limit_bytes < 0: - raise ValueError("memory_limit_bytes must be a non-negative integer") - self._memory_limit_bytes = memory_limit_bytes + if isinstance(memory_limit_mib, bool) or not isinstance(memory_limit_mib, int) or memory_limit_mib < 0: + raise ValueError("memory_limit_mib must be a non-negative integer") + self._memory_limit_mib = memory_limit_mib self._process_cgroups: dict[int, Path] = {} super().__init__(cwd=cwd, read_only_paths=read_only_paths, writable_paths=writable_paths) @@ -73,7 +73,7 @@ class PathBubbleBackend(FileSandboxBackend): if shutil.which("bwrap") is None: raise RuntimeError("PathBubbleBackend requires bwrap on PATH") argv = self._bwrap_argv(command, cwd) - group = self._create_cgroup() if self._memory_limit_bytes else None + group = self._create_cgroup() if self._memory_limit_mib else None try: if group is not None: argv = [sys.executable, "-I", "-S", "-c", _CGROUP_EXEC, str(group / "cgroup.procs"), *argv] @@ -104,7 +104,7 @@ class PathBubbleBackend(FileSandboxBackend): group = parent / f"reme-{uuid.uuid4().hex}" group.mkdir() try: - (group / "memory.max").write_text(str(self._memory_limit_bytes), encoding="ascii") + (group / "memory.max").write_text(str(self._memory_limit_mib * 1024**2), encoding="ascii") except BaseException: group.rmdir() raise @@ -125,6 +125,3 @@ class PathBubbleBackend(FileSandboxBackend): group.rmdir() except OSError as cleanup_error: raise RuntimeError(f"could not remove sandbox cgroup {group}: {cleanup_error}") from exc - - # Keep the existing helper name for callers that manage a process directly. - _release_cgroup = _release_process diff --git a/reme/utils/file_sandbox/path_seatbelt_backend.py b/reme/utils/file_sandbox/path_seatbelt_backend.py index 9417562d..21ae4036 100644 --- a/reme/utils/file_sandbox/path_seatbelt_backend.py +++ b/reme/utils/file_sandbox/path_seatbelt_backend.py @@ -20,13 +20,13 @@ class PathSeatbeltBackend(FileSandboxBackend): cwd: str | os.PathLike[str], read_only_paths: list[str], writable_paths: list[str], - memory_limit_bytes: int = 0, + memory_limit_mib: int = 0, ) -> None: - if isinstance(memory_limit_bytes, bool) or not isinstance(memory_limit_bytes, int) or memory_limit_bytes < 0: - raise ValueError("memory_limit_bytes must be a non-negative integer") - if memory_limit_bytes: + if isinstance(memory_limit_mib, bool) or not isinstance(memory_limit_mib, int) or memory_limit_mib < 0: + raise ValueError("memory_limit_mib must be a non-negative integer") + if memory_limit_mib: warnings.warn( - "PathSeatbeltBackend does not support memory_limit_bytes; the limit is ignored on macOS", + "PathSeatbeltBackend does not support memory_limit_mib; the limit is ignored on macOS", RuntimeWarning, stacklevel=2, ) diff --git a/tests/unit/test_file_sandbox_backend.py b/tests/unit/test_file_sandbox_backend.py index d9d560e8..c12ee845 100644 --- a/tests/unit/test_file_sandbox_backend.py +++ b/tests/unit/test_file_sandbox_backend.py @@ -18,7 +18,7 @@ def test_platform_selection_and_memory_defaults(tmp_path, monkeypatch): monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", "linux") linux = create_file_sandbox_backend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) assert isinstance(linux, PathBubbleBackend) - assert linux._memory_limit_bytes == 4 * 1024**3 # pylint: disable=protected-access + assert linux._memory_limit_mib == 4096 # pylint: disable=protected-access monkeypatch.setattr("reme.utils.file_sandbox.sys.platform", "darwin") mac = create_file_sandbox_backend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) @@ -28,7 +28,7 @@ def test_platform_selection_and_memory_defaults(tmp_path, monkeypatch): cwd=tmp_path, read_only_paths=[], writable_paths=[], - memory_limit_bytes=1024, + memory_limit_mib=1024, ) assert isinstance(limited, PathSeatbeltBackend) diff --git a/tests/unit/test_path_bubble_backend.py b/tests/unit/test_path_bubble_backend.py index 89162a63..d7fd495d 100644 --- a/tests/unit/test_path_bubble_backend.py +++ b/tests/unit/test_path_bubble_backend.py @@ -56,15 +56,36 @@ def test_path_bubble_backend_adds_workspace_read_only(tmp_path): @pytest.mark.parametrize("limit", [-1, True, 1.5, "4096"]) def test_path_bubble_backend_rejects_invalid_memory_limit(tmp_path, limit): - """Memory limits must be non-negative whole bytes.""" - with pytest.raises(ValueError, match="memory_limit_bytes"): - PathBubbleBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[], memory_limit_bytes=limit) + """Memory limits must be non-negative whole MiB.""" + with pytest.raises(ValueError, match="memory_limit_mib"): + PathBubbleBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[], memory_limit_mib=limit) + + +def test_one_mib_is_written_as_bytes_to_cgroup(tmp_path, monkeypatch): + """The public MiB limit is converted only at the Linux cgroup boundary.""" + proc_file = tmp_path / "proc-self-cgroup" + proc_file.write_text("0::/\n", encoding="ascii") + cgroup_root = tmp_path / "cgroup" + cgroup_root.mkdir() + original_path = Path + + def sandbox_path(value): + if value == "/proc/self/cgroup": + return proc_file + if value == "/sys/fs/cgroup": + return cgroup_root + return original_path(value) + + monkeypatch.setattr("reme.utils.file_sandbox.path_bubble_backend.Path", sandbox_path) + backend = PathBubbleBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[], memory_limit_mib=1) + group = backend._create_cgroup() # pylint: disable=protected-access + assert (group / "memory.max").read_text(encoding="ascii") == str(1024**2) @pytest.mark.asyncio async def test_each_execution_gets_separate_memory_cgroup(tmp_path, monkeypatch): """Each process receives its own cgroup before bubblewrap is executed.""" - backend = PathBubbleBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[], memory_limit_bytes=1024) + backend = PathBubbleBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[], memory_limit_mib=1024) groups = [] subprocess_options = [] subprocess_commands = [] @@ -92,15 +113,15 @@ async def test_each_execution_gets_separate_memory_cgroup(tmp_path, monkeypatch) for argv, group in zip(subprocess_commands, groups): assert argv[:4] == (sys.executable, "-I", "-S", "-c") assert argv[5:7] == (str(group / "cgroup.procs"), "bwrap") - backend._release_cgroup(first) # pylint: disable=protected-access - backend._release_cgroup(second) # pylint: disable=protected-access + backend._release_process(first) # pylint: disable=protected-access + backend._release_process(second) # pylint: disable=protected-access assert not any(group.exists() for group in groups) @pytest.mark.asyncio async def test_zero_memory_limit_skips_cgroup(tmp_path, monkeypatch): """A zero limit bypasses cgroup creation and child enrollment.""" - backend = PathBubbleBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[], memory_limit_bytes=0) + backend = PathBubbleBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[], memory_limit_mib=0) options = {} async def create_process(*_argv, **kwargs): @@ -164,7 +185,7 @@ async def test_python_execute_uses_path_backend_when_paths_supplied(tmp_path, mo assert seen["command"] == [sys.executable, "-c", "print('ok')"] assert seen["cwd"] == str(tmp_path) assert seen["timeout"] == 60 - assert seen["backend"]._memory_limit_bytes == 4 * 1024**3 # pylint: disable=protected-access + assert seen["backend"]._memory_limit_mib == 4096 # pylint: disable=protected-access @pytest.mark.asyncio @@ -173,16 +194,26 @@ async def test_python_execute_passes_custom_memory_limit(tmp_path, monkeypatch): seen = {} async def fake_exec(self, _command, *, cwd=None, timeout=None): # pylint: disable=unused-argument - seen["limit"] = self._memory_limit_bytes # pylint: disable=protected-access + seen["limit"] = self._memory_limit_mib # pylint: disable=protected-access return ExecResult(exit_code=0, stdout=b"", stderr=b"") monkeypatch.setattr(PathBubbleBackend, "exec_shell", fake_exec) step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) - response = await step(code="pass", read_only_paths=[], memory_limit_bytes=0) + response = await step(code="pass", read_only_paths=[], memory_limit_mib=0) assert response.success is True assert seen["limit"] == 0 +@pytest.mark.asyncio +@pytest.mark.parametrize("limit", [0, 1, -1]) +async def test_python_execute_rejects_memory_limit_without_path_policy(tmp_path, limit): + """A requested memory policy must never be silently ignored.""" + step = PythonExecuteStep(app_context=ApplicationContext(workspace_dir=str(tmp_path))) + response = await step(code="print('should not run')", memory_limit_mib=limit) + assert response.success is False + assert response.answer == "memory_limit_mib requires read_only_paths or writable_paths to enable the sandbox" + + @pytest.mark.asyncio async def test_step_rejects_invalid_mount_parameters(tmp_path): """Malformed path injection cannot silently disable sandboxing.""" @@ -214,4 +245,4 @@ def test_job_hides_injected_path_policy(): parameters = yaml.safe_load(config_path.read_text(encoding="utf-8"))["jobs"]["python_execute"]["parameters"] assert "read_only_paths" not in parameters["properties"] assert "writable_paths" not in parameters["properties"] - assert "default" not in parameters["properties"]["memory_limit_bytes"] + assert "default" not in parameters["properties"]["memory_limit_mib"]