Repository navigation
FEAT: program execution scorer in an isolated sandbox (scorer phase 10b) - #3218
WatchTree-19 wants to merge 3 commits into
Conversation
Adds ProgramBehaves, ProgramExecutionScorable, ProgramExecutionSource, the SandboxClient protocol with a DockerSandboxClient, and ProgramExecutionScorer. The scorer sends the source only the runtime profile and invocations; expected results stay with the scorer. Program failure is observed evidence, a sandbox failure is undetermined, and a stored execution can be judged again without running the program.
- ProgramExecutionSource shares one in-flight run between concurrent callers of the same attempt and cancels it only when every caller has gone. - DockerSandboxClient reports an exit code only for an exited container; a client that exits while the container is still running is a sandbox error. - Docker management commands are killed and reaped on cancellation as well as on timeout. - A failed container removal is reported as a warning with the container name, kept in the observation metadata, and stops further cases. - The read-only program file is made writable before its directory is removed, so cleanup also works on Windows. - ContentEntryScorable is accepted as a program reference, so a saved score anchor can be scored again.
|
Thanks Roman, all six addressed in the latest commit, each with a regression test:
|
…ations - DockerSandboxClient uses one deadline for the whole invocation, including the attached client's exit. A client still running after its streams close is ended at the deadline, and the container's own state then decides the result, so a stalled client is neither unbounded nor mistaken for a program timeout. - ProgramExecutionSource waits for the sandbox's cleanup when the last caller of an attempt is cancelled, so cancellation still means the environment is gone. - Memory links a new observation whose ID is already stored when the stored row is identical, checked under the observation row locks; different evidence under the same ID is still rejected. ProgramExecutionScorer drops its separate pre-check, so concurrent scoring calls that share one acquisition both persist their scores.
|
Thanks Roman, all three are addressed in the latest commit.
|
| "/tmp:rw,nosuid,nodev,size=16m", | ||
| "--workdir", | ||
| "/tmp", | ||
| "--mount", |
There was a problem hiding this comment.
🔴 Must fix: bound Docker's own output logs, not only the retained streams.
max_output_bytes caps the bytes this client keeps, but this docker run command inherits the daemon's logging configuration. Docker's default json-file driver has no log rotation, so container output can still consume host disk space. The read-only root filesystem and memory limit do not constrain these daemon-side files, and the timeout limits duration rather than output volume.
I checked this with a bounded mocked Docker lifecycle and a real local Python producer that wrote exactly 8,192 bytes. With max_output_bytes=128, the stored event kept 128 bytes and set stdout_truncated=True, while the simulated daemon log kept all 8,192 bytes. The captured command had neither --log-driver nor --log-opt. This was not a live-daemon disk-exhaustion test; the mock models the separately documented log routing. Docker documents the default disk-exhaustion risk here: https://docs.docker.com/engine/logging/configure/.
Please disable daemon-side logging for these attached runs with --log-driver none, or enforce a bounded logging configuration. Add a regression assertion on the generated Docker arguments so the output limit does not depend on the host's logging defaults.
AI-assisted review; reproduction run with GitHub Copilot.
| def _remember(self, *, scorable: ProgramExecutionScorable, observation: Observation) -> None: | ||
| self._observed[scorable] = observation | ||
| while len(self._observed) > self._MAX_REUSED_OBSERVATIONS: | ||
| self._observed.popitem(last=False) |
There was a problem hiding this comment.
🟡 Should fix: preserve execution identity after cache eviction.
Evicting an observation makes a later acquisition of the same ProgramExecutionScorable execute the program again under the same execution_id. That contradicts the documented contract that reusing the scorable reuses one observed execution, and that a fresh run requires a new ID. This is separate from the concurrent-acquisition issue already fixed.
I reproduced this through public score_async calls with real in-memory SQLite persistence and a fake sandbox whose stdout changes on each call:
- Score one explicit execution:
True. - Score it again immediately:
True, with the same observation and no second sandbox call. - Acquire 256 other distinct successful executions, using the unchanged cache limit.
- Score the original execution again:
False, with a new observation.
Both final scores persisted successfully. Their execution anchors, including the UUID, were identical, but the sandbox ran the original program twice and the scores referenced different observations. A nondeterministic program can therefore give contradictory results for what the API says is one attempt.
Please make reuse independent of LRU eviction, or fail explicitly when an attempt's evidence is no longer available instead of silently running it again. Keep the memory bound; simply removing eviction would introduce a different problem. Add a regression covering reuse beyond the 256-entry limit.
AI-assisted review; reproduction run with GitHub Copilot.
| "--mount", | ||
| f"type=bind,source={os.fspath(program_dir)},target=/program,readonly", | ||
| profile.image, | ||
| *resolved_command, |
There was a problem hiding this comment.
🟡 Should fix: make the runtime profile control the effective entrypoint too.
Putting resolved_command after the image overrides Docker's CMD, but does not override the image's ENTRYPOINT. For an image with ENTRYPOINT ["python3"], the accepted profile command=("python3", "-I") therefore runs python3 python3 -I /program/main.py, not the command recorded in the execution event. Correct submitted code can be scored as a program failure because of the runtime configuration.
I reproduced the result with a controlled Docker-lifecycle mock that applies Docker's documented ENTRYPOINT-plus-command composition and runs the resulting arguments through a real local Python interpreter. For the same print('ok') program and expectation:
- No image entrypoint: exit 0, verdict
True. - Image entrypoint
("python3",): exit 2 because Python tries to openpython3as a script, verdictFalse. - Both events recorded the same
resolved_command:("python3", "-I", "/program/main.py"), although the effective commands differed.
The Docker daemon was unavailable, so this was not a live-container test. Docker documents the argument composition here: https://docs.docker.com/engine/containers/run/#default-command-and-options.
Please set --entrypoint explicitly from the trusted profile, or reject incompatible image entrypoints before scoring. Add a profile-with-ENTRYPOINT regression and ensure the recorded command matches what actually runs.
AI-assisted review; reproduction run with GitHub Copilot.
| program_dir = Path(tempfile.mkdtemp(prefix="pyrit-program-")) | ||
| program_path = program_dir / profile.program_filename | ||
| try: | ||
| program_path.write_text(program, encoding="utf-8") |
There was a problem hiding this comment.
🟡 Should fix: move filesystem preparation and removal off the event loop.
execute_async directly calls synchronous mkdtemp, write_text, and chmod, and its finally reaches synchronous shutil.rmtree. A slow filesystem operation blocks unrelated scoring tasks and prevents their timers or cancellation handlers from running. These operations are on the async execution path, not constructor-time setup.
I reproduced the impact without Docker: I mocked only _run_invocation_async, injected a bounded 300 ms delay into the real Path.write_text call, and ran an independent 5 ms asyncio heartbeat on the same loop. The largest heartbeat gap increased from 19.0 ms in the control run to 306.5 ms with the delayed write. The program file was still written and cleaned up normally; the delay shows that filesystem latency stalls the shared event loop rather than only this execution.
Please put preparation and removal in synchronous helpers awaited through asyncio.to_thread, or use the existing async file-I/O pattern where appropriate. Preserve the cleanup ordering on cancellation. A slow-file/heartbeat regression would cover this independently of the Docker process tests.
AI-assisted review; reproduction run with GitHub Copilot.
| if program_dir.exists(): | ||
| logger.warning("Could not remove the program directory %s", program_dir) | ||
|
|
||
| async def _run_invocation_async( |
There was a problem hiding this comment.
Simplicity: separate command construction from the process lifecycle.
The overall scorer -> source -> sandbox responsibility split is good. Inside _run_invocation_async, however, one function constructs all Docker arguments, starts the process, coordinates three stream tasks, handles deadlines and two different client-stall paths, inspects container state, removes the container, and builds the event. The nested try/except/finally blocks make it difficult to check which failures are program evidence and which are infrastructure failures.
Please extract a pure run-argument builder and a focused helper that waits for the streams/process against one deadline. Keep resource ownership and cleanup visible in _run_invocation_async; this does not need another class hierarchy. That would make the command contract and timeout policy easier to understand and test independently.
AI-assisted review with GitHub Copilot.
Description
This is phase 10b from the scorer roadmap, the executable program behaviour prototype. It adds a scorer that answers "does this program behave as intended for these inputs?" by running the program in an isolated sandbox and judging what it did, rather than what a response says it would do.
It follows the same shape as the file write scorer from phase 10: a scorable for the evidence, a condition for what counts, a source that acquires the evidence, and a pure matcher. Nothing in the existing scorers changes.
What it adds
ProgramBehavescondition (runtime_profile,cases), where eachProgramCasepairs aProgramInvocation(arguments and stdin) with aProgramResult(exit code, optionalstdoutandstderrtext matchers).ProgramExecutionScorable(scorable_type="program_execution") with anexecution_id, the program (aMessageScorablewith one text piece, or aContentScorable), the runtime profile and the invocations. It holds no expected output.ProgramExecutionEventandProgramExecutionObservationPayload(kind="program_execution"): one event per invocation with exit code, bounded stdout and stderr with a truncation flag on each, timeout and resource-limit flags, the resolved command, the image digest and the duration, plus the sha256 of the program that ran.SandboxClientprotocol,SandboxRunandSandboxExecutionError. The client owns the disposable environment; the source only submits the attempt and waits for it.DockerSandboxClientwithDockerRuntimeProfile: each invocation runs in a fresh container with no network, a read-only root, all capabilities dropped,no-new-privileges, an unprivileged user, memory, CPU and pids limits, and the program mounted read-only. Images must be pinned by digest and are never pulled during scoring (--pull never).ProgramExecutionSourceandProgramExecutionScorer(CONDITION_TYPE = ProgramBehaves)._ConditionTupleSchema: a condition that uses one nested model in two fields (ProgramResult.stdoutand.stderrboth take aTextMatcher) is emitted by Pydantic as a$ref, so theconditionsJSON schema now resolves the reference and every entry keeps its fields inline. The existing schema tests cover it.Stimuli and assertions are split before acquisition
The scorer takes only the runtime profile and each invocation from the condition to build the
ProgramExecutionScorable, and keeps each expected result for the matcher. The source and the sandbox never see which output passes. A test asserts the expected value never reaches the sandbox.How the verdict works
ERROR), when it stopped before every case ran (PARTIAL), or when output cut at the retention limit cannot settle anEqualsorRegexcheck. A cut stream can still prove aContainsmatch. Broken infrastructure therefore never looks like a program that failed its tests.On the open decision in the roadmap
Direct code content only. The program is the scored text used verbatim; code is not extracted from surrounding prose, so a response that wraps code in explanation fails its cases rather than being guessed at. A message must name exactly one text piece, and a blocked or failed response has no program to run, so the evidence is unavailable.
Reuse and replay
Every scoring call mints a new
execution_id, so it is a new attempt. Scoring an explicitProgramExecutionScorableagain reuses that attempt's observation (the source keeps a bounded cache of completed and partial runs; errors are not reused), and the new score links to the stored observation.score_observation_asyncjudges stored output against new expected results without running the program again. Replay with different invocations or a different runtime profile raisesNonReplayableObservationError.Tests and Documentation
tests/unit/models/test_program.py(27 tests): validation, round trips through the condition and scorable registries, and the acquisition invariants onObservation.tests/unit/models/test_scorable.py: the union coverage test gains aProgramExecutionScorablecase.tests/unit/score/test_program_execution_scorer.py(48 tests): the matching table, verdicts for pass, fail, timeout, resource limit, sandbox error and early stop, output bounding, the stimuli split, reuse, replay, stored messages and blocked responses, and the inverter keeping undetermined.tests/unit/score/test_docker_sandbox_client.py(18 tests): runs the real client against a fake Docker CLI script, covering the container flags, arguments and stdin, timeout kill, OOM, output caps, a container that never starts, a failure after some cases ran, and container removal.python:3.12-slim: correct program true, wrong output false, syntax error false, infinite loop false (timed out), network access false, write to the root false, OOM false (resource limit), missing image undetermined, and no containers left behind.doc/code/scoring/7_program_execution_scorer(paired .py and .ipynb, executed), linked from the scoring overview,myst.ymlandframework.md. It needs Docker and the pinned image, and no model or credentials.