From 7413f3f1fee5850684ef6eab13abd9da7521dd73 Mon Sep 17 00:00:00 2001 From: Dan Morosanu Date: Fri, 7 Aug 2026 18:19:39 +0300 Subject: [PATCH] feat(protected-mock): add subset fixture matching and harden mockd startup --- docs/TASK_DEFINITION_GUIDE.md | 9 +- src/coder_eval/protected_mock/runtime.py | 67 +++++++- src/coder_eval/protected_mock/server.py | 57 +++++-- tests/test_protected_mock.py | 204 ++++++++++++++++++++++- 4 files changed, 313 insertions(+), 24 deletions(-) diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 0da730f7..e85b0418 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -558,7 +558,7 @@ sandbox: The agent receives a thin `cli_mocks/uip` wrapper. The fixture is copied into a per-run staging directory, mounted below the private `mockd` filesystem parent, and read only by the `mockd` UID. The client speaks a bounded Unix-socket protocol and has no file-read, path, glob, search, dump, or debug operation. Calls use the existing `cli_mocks/calls.jsonl` schema. -Fixture files map exact argument lists to responses: +Fixture files map argument lists to responses: ```json { @@ -578,7 +578,12 @@ Fixture files map exact argument lists to responses: } ``` -Matching defaults to exact argv equality. A response may opt into `"match_mode": "normalized"`; this still selects from a finite command map but ignores `--output `, treats `--flag=value` like `--flag value`, and permits token reordering. It never performs subset or substring matching. Duplicate keys, malformed responses, oversized output, request-budget exhaustion, and service startup failures all fail loudly. +Matching defaults to exact argv equality. Two further modes exist, selected per response via `match_mode`: + +- `"normalized"` still selects from a finite command map but ignores `--output `, treats `--flag=value` like `--flag value`, and permits token reordering. Duplicate keys are rejected at load for both finite modes. +- `"subset"` matches when every rule token appears in the invocation's normalized token set, regardless of order or extra arguments. Subset rules are evaluated in fixture-file order and the first match wins; exact and normalized matches always take precedence over subset scanning. Duplicate subset rules are allowed (an earlier rule shadows a later one); an empty subset `argv` is rejected at load. + +Malformed responses, oversized output, request-budget exhaustion, and service startup failures all fail loudly. `passthrough_argv_prefixes` is for deliberately public live operations such as `uip docsai ask`. `mockd` invokes the real tool only when argv begins with one of these typed prefixes, caches the response in memory for the run, and never reveals the executable path to the agent. Do not use a broad prefix such as `[or]` or `[auth]`. `protected_mocks` and `record_cli` cannot claim the same tool name. diff --git a/src/coder_eval/protected_mock/runtime.py b/src/coder_eval/protected_mock/runtime.py index f67c850b..c1fc1aaf 100644 --- a/src/coder_eval/protected_mock/runtime.py +++ b/src/coder_eval/protected_mock/runtime.py @@ -6,6 +6,7 @@ import os import subprocess import sys +import tempfile import time from collections.abc import Iterator from pathlib import Path @@ -13,35 +14,83 @@ from .protocol import SERVER_LAUNCHER, SOCKET_PATH +# Generous on purpose: normal startup is well under a second, but a loaded box +# -- parallel workers, cold caches -- can stall interpreter startup and bind +# well past a tight deadline. The common case is unaffected: the poll returns as +# soon as the socket appears. +STARTUP_TIMEOUT_SECONDS = 30.0 + + +def _server_stderr_suffix(stderr_path: Path) -> str: + """Tail of the child's captured stderr, formatted for an error message.""" + try: + text = stderr_path.read_text(encoding="utf-8", errors="replace").strip() + except OSError: + return "" + if not text: + return "" + return f"; server stderr (tail): {text[-2000:]}" + + @contextlib.contextmanager def running_mock_server(config_path: Path | None) -> Iterator[None]: if config_path is None: yield return - process = subprocess.Popen( - [SERVER_LAUNCHER, sys.executable, "-m", "coder_eval.protected_mock.server", "--config", str(config_path)], - stdin=subprocess.DEVNULL, - ) + # Captured to a file rather than a pipe: nothing drains a pipe here, and a + # full one would deadlock the child. The temp file is created 0600 and owned + # by the spawning process (root inside the container), so the agent uid + # cannot read it. It deliberately lives outside the socket directory, which + # mockd creates for itself. + with tempfile.NamedTemporaryFile(prefix="coder-eval-mockd-", suffix=".stderr", delete=False) as stderr_sink: + stderr_path = Path(stderr_sink.name) + try: + process = subprocess.Popen( + [ + SERVER_LAUNCHER, + sys.executable, + "-m", + "coder_eval.protected_mock.server", + "--config", + str(config_path), + ], + stdin=subprocess.DEVNULL, + stderr=stderr_sink, + ) + except OSError: + stderr_path.unlink(missing_ok=True) + raise + socket_path = Path(SOCKET_PATH) try: - deadline = time.monotonic() + 5 + started = time.monotonic() + deadline = started + STARTUP_TIMEOUT_SECONDS while time.monotonic() < deadline: if process.poll() is not None: - raise RuntimeError(f"protected mockd exited during startup with code {process.returncode}") + raise RuntimeError( + f"protected mockd exited during startup with code {process.returncode}" + + _server_stderr_suffix(stderr_path) + ) if socket_path.exists(): break time.sleep(0.02) else: - raise RuntimeError("protected mockd did not create its socket within 5 seconds") + waited = time.monotonic() - started + deadline_note = f"within {waited:.1f}s (deadline {STARTUP_TIMEOUT_SECONDS}s)" + raise RuntimeError( + f"protected mockd did not create its socket {deadline_note}" + _server_stderr_suffix(stderr_path) + ) yield finally: if process.poll() is None: process.terminate() try: - process.wait(timeout=3) + process.wait(timeout=5) except subprocess.TimeoutExpired: process.kill() - process.wait(timeout=3) + process.wait(timeout=5) with contextlib.suppress(OSError): os.unlink(socket_path) + with contextlib.suppress(OSError): + os.unlink(stderr_path) diff --git a/src/coder_eval/protected_mock/server.py b/src/coder_eval/protected_mock/server.py index 19ef6a42..2a92dcdd 100644 --- a/src/coder_eval/protected_mock/server.py +++ b/src/coder_eval/protected_mock/server.py @@ -31,6 +31,7 @@ class CommandResponse: class ToolState: responses: dict[tuple[str, ...], CommandResponse] normalized_responses: dict[tuple[str, ...], CommandResponse] + subset_responses: list[tuple[tuple[str, ...], CommandResponse]] default: CommandResponse remaining: int passthrough_prefixes: tuple[tuple[str, ...], ...] @@ -42,13 +43,17 @@ class ToolState: _NOISE_VALUE_FLAGS = frozenset({"--output"}) -def _normalized_argv(argv: list[str]) -> tuple[str, ...]: - """Canonical finite-command key: flag form/order agnostic, never subset matching.""" +def _expand_argv_tokens(argv: list[str]) -> list[str]: + """Flag-form-agnostic token stream: ``--flag=value`` split, noise flags dropped.""" expanded: list[str] = [] for raw in argv: if raw.startswith("-") and "=" in raw: flag, value = raw.split("=", 1) + if flag in _NOISE_VALUE_FLAGS: + # Inline form is dropped whole (value included, even an empty + # one) so a bare noise flag never re-enters the skip logic below. + continue expanded.append(flag) if value: expanded.append(value) @@ -56,16 +61,24 @@ def _normalized_argv(argv: list[str]) -> tuple[str, ...]: expanded.append(raw) cleaned: list[str] = [] - skip_next = False - for token in expanded: - if skip_next: - skip_next = False - continue + index = 0 + while index < len(expanded): + token = expanded[index] + index += 1 if token in _NOISE_VALUE_FLAGS: - skip_next = True + # Split form: swallow the following token only when it is actually a + # value, so a trailing noise flag cannot eat the next flag. + if index < len(expanded) and not expanded[index].startswith("-"): + index += 1 continue cleaned.append(token) - return tuple(sorted(cleaned)) + return cleaned + + +def _normalized_argv(argv: list[str]) -> tuple[str, ...]: + """Canonical finite-command key: flag form/order agnostic, never subset matching.""" + + return tuple(sorted(_expand_argv_tokens(argv))) def _response(raw: object, *, context: str) -> CommandResponse: @@ -98,16 +111,26 @@ def _load_tool( raise ValueError(f"fixture {fixture_path} responses must be a list") responses: dict[tuple[str, ...], CommandResponse] = {} normalized_responses: dict[tuple[str, ...], CommandResponse] = {} + # Ordered on purpose: subset rules are scanned in fixture-file order and the + # first match wins, so duplicates are legal (an earlier rule shadows a later + # one) -- the duplicate-key error applies to the finite match modes only. + subset_responses: list[tuple[tuple[str, ...], CommandResponse]] = [] for index, entry in enumerate(entries): if not isinstance(entry, dict): raise ValueError(f"fixture {fixture_path} response {index} must be an object") argv = entry.get("argv") if not isinstance(argv, list) or not all(isinstance(item, str) for item in argv): raise ValueError(f"fixture {fixture_path} response {index}.argv must be a string list") - key = tuple(argv) match_mode = entry.get("match_mode", "exact") - if match_mode not in {"exact", "normalized"}: - raise ValueError(f"fixture {fixture_path} response {index}.match_mode must be exact or normalized") + if match_mode not in {"exact", "normalized", "subset"}: + raise ValueError(f"fixture {fixture_path} response {index}.match_mode must be exact, normalized, or subset") + if match_mode == "subset": + rule_tokens = tuple(_expand_argv_tokens(argv)) + if not argv or not rule_tokens: + raise ValueError(f"fixture {fixture_path} response {index}.argv must be non-empty for subset matching") + subset_responses.append((rule_tokens, _response(entry, context=f"response {index}"))) + continue + key = tuple(argv) destination = responses if match_mode == "exact" else normalized_responses command_key = key if match_mode == "exact" else _normalized_argv(argv) if command_key in destination: @@ -126,6 +149,7 @@ def _load_tool( return ToolState( responses=responses, normalized_responses=normalized_responses, + subset_responses=subset_responses, default=default, remaining=max_requests, passthrough_prefixes=tuple(tuple(prefix) for prefix in passthrough_prefixes), @@ -187,6 +211,15 @@ def dispatch(self, tool: str, argv: list[str]) -> CommandResponse: response = state.responses.get(tuple(argv)) if response is None: response = state.normalized_responses.get(_normalized_argv(argv)) + if response is None and state.subset_responses: + # Finite matches take precedence; subset rules scan in fixture-file + # order and the first whose tokens all appear in the invocation's + # normalized token set wins. + invocation_tokens = set(_expand_argv_tokens(argv)) + for rule_tokens, candidate in state.subset_responses: + if all(token in invocation_tokens for token in rule_tokens): + response = candidate + break if response is not None: return response if any(tuple(argv[: len(prefix)]) == prefix for prefix in state.passthrough_prefixes): diff --git a/tests/test_protected_mock.py b/tests/test_protected_mock.py index 318a489a..da51a25a 100644 --- a/tests/test_protected_mock.py +++ b/tests/test_protected_mock.py @@ -4,8 +4,12 @@ import json import subprocess +import sys +import tempfile import threading +import time from pathlib import Path +from typing import Any from unittest.mock import MagicMock import pytest @@ -22,7 +26,8 @@ TaskDefinition, ) from coder_eval.protected_mock.protocol import CLIENT_EXECUTABLE -from coder_eval.protected_mock.server import ProtectedMockServer, load_config +from coder_eval.protected_mock.runtime import running_mock_server +from coder_eval.protected_mock.server import ProtectedMockServer, ToolState, _normalized_argv, load_config from coder_eval.sandbox import Sandbox @@ -46,6 +51,27 @@ def _fixture(path: Path) -> Path: return path +def _write_config(config: Path, fixture: Path, max_requests: int = 10) -> Path: + config.write_text( + json.dumps( + { + "version": 1, + "tools": [{"tool": "uip", "fixture": str(fixture), "max_requests": max_requests}], + } + ), + encoding="utf-8", + ) + return config + + +def _fake_server(tools: dict[str, ToolState]) -> MagicMock: + fake = MagicMock() + fake.tools = tools + fake.budget_lock = threading.Lock() + fake.passthrough_lock = threading.Lock() + return fake + + def test_protected_mocks_require_docker_driver(tmp_path: Path) -> None: fixture = _fixture(tmp_path / "uip.json") with pytest.raises(ValidationError, match="requires driver: docker"): @@ -157,6 +183,182 @@ def test_normalized_fixture_matching_remains_finite(tmp_path: Path) -> None: assert extra_argument.exit_code == 2 +def test_noise_flag_tokenizer_only_swallows_real_values() -> None: + # An --output pair is still stripped, in either flag form. + assert _normalized_argv(["rpa", "get-errors", "--output", "json"]) == ("get-errors", "rpa") + assert _normalized_argv(["rpa", "get-errors", "--output=json"]) == ("get-errors", "rpa") + + # A valueless --output must not swallow the flag that follows it. + assert _normalized_argv(["deploy", "--output", "--delete-all"]) == ("--delete-all", "deploy") + assert _normalized_argv(["deploy", "--output=", "--delete-all"]) == ("--delete-all", "deploy") + + # A trailing --output is simply dropped. + assert _normalized_argv(["deploy", "--output"]) == ("deploy",) + + +def _subset_fixture(path: Path, responses: list[dict[str, Any]]) -> Path: + path.write_text( + json.dumps( + { + "version": 1, + "responses": responses, + "default": {"exit_code": 2, "stderr": "not configured\n"}, + } + ), + encoding="utf-8", + ) + return path + + +def test_subset_matching_is_order_independent_and_tolerates_extra_tokens(tmp_path: Path) -> None: + fixture = _subset_fixture( + tmp_path / "subset.json", + [{"argv": ["rpa", "get-errors"], "match_mode": "subset", "exit_code": 0, "stdout": "subset\n"}], + ) + fake_server = _fake_server(load_config(_write_config(tmp_path / "config.json", fixture))) + + # Extra tokens, reordering, and --flag=value form are all tolerated. + matched = ProtectedMockServer.dispatch(fake_server, "uip", ["get-errors", "--job-id=42", "rpa"]) + assert matched.stdout == "subset\n" + + # A rule token missing from the invocation is not a match. + missing = ProtectedMockServer.dispatch(fake_server, "uip", ["rpa", "list-jobs"]) + assert missing.exit_code == 2 + + +def test_subset_rules_scan_in_fixture_order_first_match_wins(tmp_path: Path) -> None: + fixture = _subset_fixture( + tmp_path / "subset.json", + [ + {"argv": ["rpa"], "match_mode": "subset", "exit_code": 0, "stdout": "broad\n"}, + {"argv": ["rpa", "get-errors"], "match_mode": "subset", "exit_code": 0, "stdout": "narrow\n"}, + ], + ) + fake_server = _fake_server(load_config(_write_config(tmp_path / "config.json", fixture))) + + # Both rules match; the earlier (broader) one wins because order decides. + response = ProtectedMockServer.dispatch(fake_server, "uip", ["rpa", "get-errors"]) + assert response.stdout == "broad\n" + + +def test_exact_and_normalized_take_precedence_over_subset(tmp_path: Path) -> None: + fixture = _subset_fixture( + tmp_path / "subset.json", + [ + {"argv": ["rpa", "get-errors"], "match_mode": "subset", "exit_code": 0, "stdout": "subset\n"}, + {"argv": ["rpa", "get-errors"], "exit_code": 0, "stdout": "exact\n"}, + { + "argv": ["rpa", "list-jobs", "--job-id", "42"], + "match_mode": "normalized", + "exit_code": 0, + "stdout": "normalized\n", + }, + {"argv": ["rpa", "list-jobs"], "match_mode": "subset", "exit_code": 0, "stdout": "subset-jobs\n"}, + ], + ) + fake_server = _fake_server(load_config(_write_config(tmp_path / "config.json", fixture))) + + assert ProtectedMockServer.dispatch(fake_server, "uip", ["rpa", "get-errors"]).stdout == "exact\n" + assert ( + ProtectedMockServer.dispatch(fake_server, "uip", ["--job-id=42", "list-jobs", "rpa"]).stdout == "normalized\n" + ) + # No exact/normalized hit -> the subset rule catches the variant. + assert ProtectedMockServer.dispatch(fake_server, "uip", ["rpa", "list-jobs", "--all"]).stdout == "subset-jobs\n" + + +def test_subset_duplicates_are_allowed_and_empty_argv_is_rejected(tmp_path: Path) -> None: + duplicate = {"argv": ["rpa"], "match_mode": "subset", "exit_code": 0, "stdout": "first\n"} + fixture = _subset_fixture(tmp_path / "dup.json", [duplicate, {**duplicate, "stdout": "second\n"}]) + fake_server = _fake_server(load_config(_write_config(tmp_path / "config.json", fixture))) + assert ProtectedMockServer.dispatch(fake_server, "uip", ["rpa"]).stdout == "first\n" + + empty = _subset_fixture(tmp_path / "empty.json", [{"argv": [], "match_mode": "subset", "exit_code": 0}]) + with pytest.raises(ValueError, match="non-empty for subset"): + load_config(_write_config(tmp_path / "config2.json", empty)) + + # Noise-flag-only argv normalizes to an empty token set: also rejected. + noise = _subset_fixture( + tmp_path / "noise.json", [{"argv": ["--output", "json"], "match_mode": "subset", "exit_code": 0}] + ) + with pytest.raises(ValueError, match="non-empty for subset"): + load_config(_write_config(tmp_path / "config3.json", noise)) + + +def _stub_mockd_child(monkeypatch: pytest.MonkeyPatch, script: str, tmp_path: Path) -> list[Path]: + """Run ``script`` in place of mockd; returns the stderr temp files it created. + + ``script`` is formatted with a ``{ready}`` marker path it must create once its + stderr is flushed. The fake ``Popen`` blocks on that marker (or on the child + exiting), so the readiness deadline only starts ticking after the child has + actually said something -- interpreter startup cost stays out of the clock. + """ + monkeypatch.setattr("coder_eval.protected_mock.runtime.SOCKET_PATH", str(tmp_path / "uip.sock")) + ready = tmp_path / "child-ready" + source = script.format(ready=str(ready)) + real_popen = subprocess.Popen + real_named_temp = tempfile.NamedTemporaryFile + created: list[Path] = [] + + def fake_popen(_argv: list[str], **kwargs: Any) -> Any: + child = real_popen([sys.executable, "-c", source], **kwargs) + while not ready.exists() and child.poll() is None: + time.sleep(0.01) + return child + + def recording_named_temp(**kwargs: Any) -> Any: + handle = real_named_temp(**kwargs) + created.append(Path(handle.name)) + return handle + + monkeypatch.setattr("coder_eval.protected_mock.runtime.subprocess.Popen", fake_popen) + monkeypatch.setattr("coder_eval.protected_mock.runtime.tempfile.NamedTemporaryFile", recording_named_temp) + return created + + +def test_mockd_startup_exit_reports_child_stderr(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + created = _stub_mockd_child( + monkeypatch, + "import sys; sys.stderr.write('mockd fixture load failed\\n'); raise SystemExit(3)", + tmp_path, + ) + monkeypatch.setattr("coder_eval.protected_mock.runtime.STARTUP_TIMEOUT_SECONDS", 10.0) + + with ( + pytest.raises(RuntimeError, match="exited during startup") as excinfo, + running_mock_server(tmp_path / "config.json"), + ): + pass + + assert "mockd fixture load failed" in str(excinfo.value) + assert created and not created[0].exists() + + +def test_mockd_startup_timeout_reports_wait_time_and_child_stderr( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + created = _stub_mockd_child( + monkeypatch, + "import sys, time, pathlib; " + "sys.stderr.write('mockd still binding\\n'); sys.stderr.flush(); " + "pathlib.Path(r'{ready}').write_text('1'); time.sleep(30)", + tmp_path, + ) + monkeypatch.setattr("coder_eval.protected_mock.runtime.STARTUP_TIMEOUT_SECONDS", 0.3) + + with ( + pytest.raises(RuntimeError, match="did not create its socket within") as excinfo, + running_mock_server(tmp_path / "config.json"), + ): + pass + + assert "mockd still binding" in str(excinfo.value) + if sys.platform != "win32": + # Windows releases a just-terminated child's inherited handle + # asynchronously, so the best-effort unlink only lands reliably on the + # platform mockd actually runs on. + assert created and not created[0].exists() + + def test_passthrough_is_prefix_limited_and_cached(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: fixture = _fixture(tmp_path / "uip.json") config = tmp_path / "config.json"