Skip to content

feat(providers): add BoxdProvider for boxd machines - #1603

Open
MichielMAnalytics wants to merge 2 commits into
openenv:mainfrom
MichielMAnalytics:feat/boxd-provider
Open

MichielMAnalytics wants to merge 2 commits into
openenv:mainfrom
MichielMAnalytics:feat/boxd-provider

Conversation

@MichielMAnalytics

@MichielMAnalytics MichielMAnalytics commented Nov 12, 2026 •

Copy link
Copy Markdown

boxd × OpenEnv

Summary

Adds BoxdProvider, a ContainerProvider that runs an environment server in a boxd machine. I work on boxd. A boxd machine is a small VM with Docker installed, so the provider creates one machine per start_container, runs the environment image with docker run, and returns the machine's https:// URL. EnvClient connects to it over wss://. It follows ModalProvider in shape: registry images or image_from_dockerfile, the provider-owned flow, one machine per provider, secure-by-default errors.

  • src/openenv/core/containers/runtime/boxd_provider.py: the provider
  • pyproject.toml: openenv[boxd] extra (boxd>=0.3.1), imported only when the provider builds its client
  • tests/test_core/test_boxd_provider.py: 48 tests against a fake client, no account needed
  • examples/boxd_echo_env.py: the echo env end to end
  • docs: runtime providers guide (table row, setup section, provider-owned list), getting started, API reference

Turn it on with pip install "openenv[boxd]" and BOXD_API_KEY:

from echo_env import EchoEnv
from openenv.core.containers.runtime.boxd_provider import BoxdProvider

with BoxdProvider() as provider:
    base_url = provider.start_container("ghcr.io/huggingface/openenv-echo-env:latest")
    provider.wait_for_ready(base_url)
    with EchoEnv(base_url=base_url).sync() as env:
        env.reset()

How it maps onto ContainerProvider:

ContainerProvider boxd
start_container(image) machines.create(isolated=True, delete_after=3600), wait_until_ready, docker run -p 8000:8000 <image> in the machine
image_from_dockerfile(path) uploads the build context, docker build in the machine
env_vars uploaded as an env file into a directory only its owner can read, passed with docker run --env-file, removed after the run, so values never appear on a command line
wait_for_ready(url) polls /health; raises at once if the container exited
stop_container() / close() machines.delete(id)

Choices worth a look:

  1. The machine is always created isolated (no private network, no boxd credentials inside), because environments run untrusted code. A unit test pins it.
  2. The machine's URL is public. The name gets a random 12-hex suffix, and the docs say the URL is reachable from the internet and should be kept like a secret. The provider refuses a non-HTTPS URL and keeps the URL out of its errors (RFC 002 S1, S2). There is no signed URL like Daytona's.
  3. Only port 8000 is accepted, like Modal and Daytona. Other start_container options raise TypeError instead of being ignored.
  4. Container and docker build output stay out of errors unless surface_server_logs=True, which adds a redacted tail, as in ModalProvider.
  5. Every machine gets a delete-after timer, one hour by default (delete_after=), so boxd deletes it even when stop_container() never runs: a crashed script, or a Ctrl-C during the create call itself, before the provider has the machine id. delete_after=None turns it off. Modal's timeout and Daytona's auto_stop_interval play the same role.
  6. Hugging Face ecosystem: nothing here wraps Hub behaviour, so there is nothing to match against huggingface_hub.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • New environment
  • Refactoring

Alignment Checklist

Before submitting, verify:

  • I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
  • I have checked .claude/docs/INVARIANTS.md and no invariants are violated
  • I have run /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues

RFC Status

  • Not required (bug fix, docs, minor refactoring)
  • RFC exists: #___
  • RFC needed (will create before merge)

It implements the existing ContainerProvider contract and adds no new core API, like the Novita and Kubernetes providers.

Test Plan

Live against boxd (region nyc, boxd 0.3.1, 2026-11-12). Every machine was deleted by the provider and machines.get(id) returned not found afterwards:

Check Result
registry image (ghcr.io/huggingface/openenv-echo-env:latest), EchoEnv reset, list_tools, echo_message works; URL returned after 14 s (most of it the image pull), /health 200 after 20 s
image_from_dockerfile("envs/echo_env/server/Dockerfile"), built in the machine works; healthy after 43 s
provider-owned flow, CodingEnv(provider=BoxdProvider(image=...)), CodeAction("print(40 + 2)") stdout 42; machine deleted on client close
3 environments in parallel with asyncio.gather all healthy in 20 to 21 s
env_vars value reaches the container; not on any command line; env file in a 700 directory, removed after docker run
delete_after default machine is set to be deleted 1 h after create; delete_after=None sets no timer; the docs snippet (4 * 3600) sets 4 h
machine settings isolated, labelled managed-by=openenv
container that exits at once (busybox) wait_for_ready raises RuntimeError within 0.1 s
image that does not exist RuntimeError with Docker's error; machine deleted
second start_container RuntimeError, no second machine
wrong API key AuthenticationError: API key not recognized
docker build that fails (RUN ... && false) RuntimeError without the build output by default, with it under surface_server_logs=True; machine deleted
examples/boxd_echo_env.py as written (registry, and OPENENV_IMAGE=dockerfile) works, 24 s and 51 s end to end
the docs snippets as written healthy

Repo checks:

Command Result
PYTHONPATH=src:envs uv run pytest tests/test_core/test_boxd_provider.py 48 passed
CI test command (pytest tests/ ... -m "not integration and not network and not docker") 5808 passed, 112 skipped
uv run ruff check src/ tests/ examples/boxd_echo_env.py all checks passed
uv run ruff format src/ tests/ --check the new files are formatted; src/openenv/core/harness/README.md is flagged on main too
uv run usort check src/ tests/ only tests/envs/test_grid_world.py and tests/envs/test_julia_env.py, same on main

Claude Code Review

Output of /alignment-review on the branch. An earlier run also flagged the https check, the URL in a timeout error and in the example's log, build output in errors, and the order of the docs section; those are fixed. I left the egress and idle-timer points open for your view.

Alignment Review Report

Automated Checks

  • Lint: FAIL on the repo, but not because of this change. bash .claude/hooks/lint.sh reports "47 files would be reformatted", and the base commit 9c29b6f has the same 47. None of them is a file this diff touches: boxd_provider.py, test_boxd_provider.py and examples/boxd_echo_env.py pass usort check, ruff format --check and ruff check. tests/test_core/test_boxd_provider.py passes: 48 passed.
  • Debug code: CLEAN for this diff. check-debug.sh finds print/console.print and TODOs only in files that were already there (cli/commands/push.py, harbor/*, harness/rollout.py, test_local_docker_provider.py). Nothing in boxd_provider.py or the example, and no debugger statements.

Open RFCs Context

  • RFC 002 (env spec), In Review. Its cloud-provider section is the relevant one: "Adding a provider", design points 6 (automatic transitions) and 7 (implementation hygiene), and security invariants S1–S6, which every new ContainerProvider is expected to meet.
  • RFC 005 (agentic harnesses), In Review. Open Question 4 (network isolation through sandbox providers).
  • RFC 008 (environment auto-validation), In Review. Its Providers section covers network-policy capabilities and states "Core provider ABCs are unchanged". This diff leaves the ABCs alone, so there is no conflict.
  • RFCs 000, 001, 003, 006, 012 and 014 (In Review) and 010 and 011 (Draft) don't touch this area.

Tier 1: Fixes Required

None. No lint, debug, syntax, import or type errors were introduced, and the calls to the boxd SDK match the boxd 0.3.1 API.

Tier 2: Alignment Discussion

Principle Conflicts

ALIGNMENT FLAG: Egress from the environment container is not configured

  • Principle/RFC at stake: INVARIANTS.md Security Invariant 2, Container isolation ("Network access must be explicitly configured")
  • The concern: The machine is created isolated=True, which keeps it off private networks and away from boxd credentials. Outbound internet access from the machine and its container is still unrestricted. The boxd SDK has an egress allowlist (MachineSpecBuilder.egress, machines.set_egress_allow), but BoxdProvider exposes no option for it.
  • Suggested reviewer: darktex
RFC Conflicts

ALIGNMENT FLAG: Default-deny egress is not offered and the egress posture is not documented

  • Principle/RFC at stake: RFC 002 S3 ("Default-deny egress ... recommended" for untrusted code) and design point 7 ("Network posture (ingress/egress) is documented with default-deny egress recommended for untrusted code")
  • The concern: The new ### BoxdProvider docs describe ingress (a public URL to treat as a secret, bot check off) but say nothing about egress. The provider has no way to request default-deny or an allowlist. Bot protection is also turned off unconditionally, with no option to restrict ingress, which touches S2's "network-restricted where possible".
  • Suggested reviewer: m2gavin (wrote the RFC 002 security-invariants text); also darktex, zkwentz (RFC 002 authors)

ALIGNMENT FLAG: Automatic idle transitions are left to org defaults and not documented

  • Principle/RFC at stake: RFC 002 design point 6 ("Automatic transitions (auto-suspend, scale-to-zero, idle timeouts) can drop a live transport mid-episode, so providers choose conservative defaults for RL rollouts and document the failure mode")
  • The concern: machines.create is called without auto_suspend_timeout or idle_ignore_outbound, so the machine takes its org's defaults, which may include auto-suspend and auto-hibernate. The docs cover the delete_after hard timer (default 1h) but not standby or hibernation and how they affect a live WebSocket session. The 1h delete_after default also kills sessions that run longer than an hour. That is documented, but it is a non-conservative default for long RL rollouts and is worth discussing.
  • Suggested reviewer: darktex; m2gavin

Summary

  • 0 mechanical issues to fix
  • 1 alignment point for human review (container egress not configured)
  • 2 RFC conflicts to discuss (RFC 002 S3 and point 7, egress posture; RFC 002 point 6, idle-transition defaults)

Note

Medium Risk
New runtime path exposes internet-reachable HTTPS endpoints and runs untrusted code in cloud VMs; egress/idle defaults are org-dependent and worth review for long RL sessions.

Overview
Adds BoxdProvider, a new cloud ContainerProvider that provisions an isolated boxd microVM per start_container, runs the environment image with docker run on port 8000, and returns a public HTTPS base_url for EnvClient. It supports registry images and image_from_dockerfile (context uploaded and built on the machine), the provider-owned client flow, configurable delete_after cleanup, and secret handling via --env-file with optional redacted logs.

pip install openenv[boxd] (boxd>=0.3.1) is wired in pyproject.toml. Docs list boxd in getting started and the runtime providers guide (including security notes on public URLs). Reference autodoc, examples/boxd_echo_env.py, and 48 unit tests with a fake boxd client round out the change.

Reviewed by Cursor Bugbot for commit 38033b3. Bugbot is set up for automated code reviews on this repo. Configure here.

BoxdProvider creates one isolated boxd machine per start_container, runs
the environment image in it with docker run, and returns the machine's
https URL. image_from_dockerfile uploads the build context and builds it
in the machine. stop_container() deletes the machine.

The boxd SDK is an optional extra (openenv[boxd]) and is imported only
when the provider builds its client.
- Give every machine a delete-after timer (delete_after, one hour by
  default, None turns it off), so a machine is deleted even when
  stop_container() never runs, including a Ctrl-C during create.
- Write the env file into a directory only its owner can read.
- Refuse a non-HTTPS base_url and keep the URL out of errors and the
  example's log.
- Show docker build output only with surface_server_logs=True.
- Document delete_after and that the URL should be kept like a secret.
@MichielMAnalytics
MichielMAnalytics marked this pull request as ready for review November 12, 2026 21:44

@burtenshaw burtenshaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head: 38033b3ef86e25b83e1a7f97a19d92fee76cc433. The branch merges into main at ca6df26a6 without conflicts. No boxd account was used and no call reached boxd's API; every check below runs offline.

Verdict: NEEDS AUTHOR CHANGES

The provider is in good shape. Every boxd SDK call it makes exists with the arguments it passes in both boxd 0.3.1 and 0.3.2. Cleanup after a failed start is careful, secrets never reach a command line, and the URL stays out of the provider's own errors.

Three things block admission under the criteria proposed in #1608. Public ingress has no explicit opt-in (finding 1). The guide does not state the egress and metadata posture (finding 3). The owner line and the live round trip at the PR head are missing (criteria 5 and 6 in the table). Finding 2 is a lifecycle bug of the same class fixed in #1555 and #1556. Findings 4 to 7 are smaller. A patch for findings 1, 2, 4 and 6 is at the end. With it, the PR's tests (after the small updates listed in the test table) and the review checks below pass offline.

Whether core takes another vendor provider is a maintainer decision under #1608, whose objection window closes on 19 November. None of the items below depend on that decision.

Findings

1. [P1] Public ingress needs an explicit opt-in

src/openenv/core/containers/runtime/boxd_provider.py:88-103 and :339-355; docs/source/guides/runtime-providers.md:189-193.

BoxdProvider(image=...).start_container() returns https://<name>.<domain>, and anyone on the internet who has that URL can drive the environment. There is no login in front of it, and the provider turns the browser check off at line 355. Nothing in the call shows that the caller chose public ingress. The SDK builds access.url on the client from the machine name, so the only secret is the 48-bit random suffix in the hostname, which also travels in DNS lookups and in the TLS SNI field.

The PR description is upfront that the URL is public. Criterion 4 in #1608 turns that into a required opt-in: public ingress only behind an explicit flag such as public=True. Earlier providers follow the same rule. TensorlakeProvider in #1555 requires public=True, ACASandboxProvider requires anonymous_port=True, and the CoreWeave and W&B providers in #890 require ingress_mode="public".

Fix: add public: bool = False to __init__ and raise ValueError unless it is True, before any SDK call (patch below). Pass public=True in the class docstring example, the three guide snippets, examples/boxd_echo_env.py and the PR description. Add a test that a provider built without it raises and creates no machine. test_required_public_ingress_is_an_explicit_opt_in below fails at the PR head with DID NOT RAISE.

2. [P2] A machine created by a failed create call is never deleted

boxd_provider.py:339-348.

machines.create can raise after boxd has scheduled the machine. The SDK gives each call a 60 s deadline and does not retry DEADLINE_EXCEEDED, because the server may already have applied the mutation (its own reason, in boxd/errors.py). A Ctrl-C while the call is in flight has the same effect. In both cases the exception escapes before line 348, so _machine_id stays None. stop_container() and close() then delete nothing, and a second start_container() is allowed.

With the default delete_after=3600, boxd deletes the machine after an hour. With delete_after=None, which the guide documents, it runs until someone deletes it by hand. This is the same class as the allocation leaks fixed in #1555 (finding 1) and #1556 (finding 2). In #1556 a one-hour TTL was judged to bound such a leak but not to replace cleanup.

Fix: the provider generates the name before the call, and machines.delete accepts a name as well as an id. Record the name as the handle, then move create inside the existing try (patch below). The existing cleanup then deletes by name. A NotFoundError means nothing was created and counts as deleted, and a failed delete keeps the handle so close() can retry. Keep the name out of the cleanup warning, because it is the secret part of the URL. The class docstring (lines 71-74) and the guide (lines 211-213) should then stop saying that the timer is what covers a Ctrl-C during create. test_required_machine_allocated_by_a_failed_create_is_deleted below fails at the PR head with left running with no timer: {'vm-1': 'openenv-...'}.

3. [P2] The egress and metadata posture is not documented, and egress cannot be restricted

docs/source/guides/runtime-providers.md:182-224; boxd_provider.py:310-311 and :339-347.

Criterion 4 requires the guide to state the provider's egress and cloud metadata posture, and S3 recommends default-deny egress for untrusted code. The guide covers ingress only.

The SDK documents an isolated machine as having no metadata endpoint, no in-machine boxd CLI and no integrations, so code inside it has no boxd credential. That part is good. The same docstring says outbound internet is unaffected. The provider never calls machines.set_egress_allow, and start_container rejects every extra option, so a user cannot restrict egress through the provider. The SDK does not say whether 169.254.169.254 answers from inside a machine. Criterion 5 asks hosted backends to show that it is denied.

Fix: add a paragraph to the BoxdProvider guide section. It should say what isolated=True removes, that outbound internet is open by default, what the metadata check in the live run showed, and how to restrict egress. The smallest way needs no new provider code:

from boxd import Boxd

base_url = provider.start_container()
Boxd(region="fra").machines.set_egress_allow(provider.machine_id, ["api.example.com"])

Use the same region as the provider. ["none"] blocks all egress. Whether the provider should take an egress_allow= argument, applied after the image is pulled and before docker run, is a maintainer call. For comparison, the Tensorlake provider in #1555 takes allow_out and deny_out and always denies 169.254.0.0/16.

4. [P3] wait_for_ready can run about 30 s past its budget

boxd_provider.py:466-484.

Each loop iteration runs a 5 s HTTP probe and then a docker inspect through exec with the fixed 30 s timeout of _exec, and only then checks the deadline. When the exec stalls, for example because the machine's agent is slow to answer or the network drops, wait_for_ready(url, timeout_s=5) raises at about t=30 s. The HTTP probe can also return 200 up to 5 s after the deadline and still count as ready. test_required_readiness_probe_stays_inside_the_budget below returns at t=30 s for a 5 s budget. This is the same class as #1555 finding 3, bounded here by the 30 s exec timeout.

Fix: use time.monotonic() and bound both probes by what remains of the budget (patch below). Then test_wait_for_ready_times_out patches time.monotonic instead of time.time. test_wait_for_ready_returns_on_healthy should call wait_for_ready(url) with the default budget, so the HTTP timeout it asserts is still exactly 5.0.

5. [P3] The machine's default route is not pinned to port 8000

boxd_provider.py:351-355; docstring :288-290.

The provider and its docs say the machine's HTTPS URL routes to port 8000, but the provider never sets that. According to the SDK README, a route in auto port mode follows whichever of 80, 443, 8080, 8000, 3000, 5000 or 5173 the app on the machine binds, for the machine's whole life, and switches within seconds when a different one appears. Your live runs show the route reaching port 8000 for the echo image. If boxd's detection also sees listeners inside containers, an environment that starts a helper server on one of those ports would move the route away from the OpenEnv server mid-episode. I could not check the route's initial mode or the scope of detection without an account.

Fix: pin the route once the machine is ready, with self._client.machines.proxies.set_port(machine.id, _PORT). It exists in 0.3.1 and 0.3.2. Record machines.proxies.list(machine_id)[0].port_mode in the live evidence.

6. [P3] surface_server_logs=True does not redact the machine's URL

boxd_provider.py:337 and :436-447.

The redaction set holds only the env-var values. The server sees its public hostname in the Host header of every request through boxd's proxy, including the provider's own /health probes. A server that logs request headers before it crashes therefore puts the hostname in the RuntimeError. test_required_debug_output_redacts_the_machine_url below shows it. The impact is low, because this message is built after the container has exited and the URL serves nothing at that point. It is the same check as test_debug_output_redacts_known_ingress_url in the #1555 review.

Fix: add url.removeprefix("https://") to _redact_values once the URL is known, and replace longer values first (patch below).

7. [P3] Idle suspend and hibernation follow the org defaults, and the guide does not say so

boxd_provider.py:339-347; guide :211-219.

machines.create is called without auto_suspend_timeout or idle_ignore_outbound, so the org's or the platform's idle timers apply. RFC 002 design point 6 asks providers to choose conservative defaults for RL rollouts and to document how automatic transitions can drop a live session. EnvClient sends a WebSocket ping every 20 s by default, which should count as inbound activity while a client is connected. The gaps are the time between start_container() and the first connect, and clients that set websocket_ping_interval_s=None.

Fix: say in the guide what an idle machine does and what a suspended machine does to an open session. Alternatively, pass auto_suspend_timeout=0, which the SDK documents as disabled, call set_auto_hibernate_timeout(machine.id, 0), and rely on delete_after to bound the lifetime.

Nits

  • boxd_provider.py:226-229: _exec takes an env argument that nothing passes. The SDK turns exec(env=...) into a KEY=value prefix on the command line, which is what the env file exists to avoid. Drop the parameter so no later caller uses it.
  • boxd_provider.py:116 and :157, guide :187: the boxd SDK does not read BOXD_ORG (0.3.2 has no reference to it), so the provider defines a new variable. Take org= only, or name the boxd tool that defines BOXD_ORG.
  • boxd_provider.py:53, guide :184, examples/boxd_echo_env.py:10: drop ?ref=OpenEnv. Other providers link plain URLs, and core docs should not carry referral parameters.
  • boxd_provider.py:60, :175 and :286: CLAUDE.md asks for full paths in references, such as [~openenv.core.containers.runtime.boxd_provider.BoxdProvider.image_from_dockerfile].
  • boxd_provider.py:128: None has its own meaning for delete_after, so document the type as (int or None, optional, defaults to 3600).
  • tests/test_core/test_boxd_provider.py:45: _FakeMachines.create(self, name, **kwargs) accepts any keyword. The assertions pin the names you pass today, but criterion 5 asks the fakes to pin the SDK's names and arguments. Copy the real keyword-only signature into the fake, or add one test behind pytest.importorskip("boxd") that binds each call against the real signatures, as the contract check below does. CI installs all extras, so that test would run there.
  • tests/test_core/test_boxd_provider.py:398-404: the SDK never builds an http:// URL. It returns "" when the domain is not known yet, so use "" to cover the case that can happen.
  • Optional, boxd_provider.py:365-380: the install -d -m 700 step, its error path and the chmod 600 protect the env file from other users of a single-tenant VM, and the container cannot read the VM's filesystem. Deleting the file after docker run already removes it from disk. CLAUDE.md prefers less defensive code; your call.

What holds up

  • Every SDK call binds against the real signatures in boxd 0.3.1 and 0.3.2: Boxd(api_key=, region=), machines.create(name, org=, isolated=, labels=, vcpu=, memory=, delete_after=), wait_until_ready, set_bot_protection(mode="off"), exec(..., timeout=), files.upload with bytes and with str, delete, close, boxd.errors.NotFoundError, Machine.access.url and ExecResult.exit_code, stdout and stderr. Constructing Boxd() makes no network call.
  • Through the real SDK request builders, create sends isolated=true, the managed-by label, delete_after_secs=3600, and 16 GiB for memory="16G".
  • isolated=True keeps boxd credentials out of the machine. The SDK README says Boxd() inside an isolated machine raises AuthenticationError.
  • Env values reach the container only through the uploaded file. No exec command line carries them, and names and newlines are checked before any machine exists.
  • Commands run in the machine are built from constants, with shlex.quote on the image (S5).
  • Every failure after create returns deletes the machine. A failed delete keeps the id and blocks a second start. The real NotFoundError counts as deleted. A retry after a failed close() works, because the SDK's transport rebuilds its channel on the next call.
  • The URL stays out of the provider's errors and logs. The cleanup warning logs machine.id, which is the vm_id, not the name in the URL.
  • delete_after=0 is rejected. The SDK would read 0 as no timer.

Admission criteria (#1608)

# Criterion Status Evidence, and what to add
1 Contract only Met One new module that subclasses ContainerProvider. env_client.py, the server and runtime/__init__.py are unchanged.
2 Optional extra that resolves Met boxd is MIT-licensed and on PyPI. The provider imports it only in __init__ and stop_container. uv sync --all-extras resolves boxd 0.3.2 with no Python marker (it needs Python 3.10 or later, like core) and installs it on 3.13. CI runs 3.11 and 3.12.
3 Generally available backend Unknown boxd is a hosted service. Please confirm in the PR that anyone can sign up and create an API key without contacting sales.
4 Security invariants Not met S1 met: _require_secure_url, and the SDK only builds https:// URLs or an empty string. S2 not met: public by default (finding 1). S3 not met: egress is open and undocumented (finding 3). S4 met apart from finding 6. S5 met. S6 met by default: the VM size bounds CPU and memory, and delete_after defaults to one hour. Idle timers follow org defaults (finding 7).
5 Evidence Partly met The fakes and their assertions pin the method names and arguments, and the contract check below binds every call against 0.3.1 and 0.3.2. The live run in the description covers reset, step and cleanup with boxd 0.3.1, but names no commit and shows neither the /ws 101 nor a metadata check. After the changes, record one run at the new head with the SHA, the 101 from the /ws upgrade, reset() and step(), machines.get(id) raising NotFoundError after stop, and a request to http://169.254.169.254/ from inside the container failing.
6 Named owner Not met Add a line to the ### BoxdProvider guide section that names who fixes SDK breakage.
7 When to use which Met No other provider reaches boxd.

Tests and commands

Command Result
uv sync --all-extras (Python 3.13.14, macOS) Resolved. Installed boxd 0.3.2, grpcio 1.85.1, protobuf 6.33.6.
PYTHONPATH=src:envs uv run pytest tests/test_core/test_boxd_provider.py -q 48 passed
The same, with Python sockets and DNS blocked 48 passed
PYTHONPATH=src:envs uv run pytest tests/test_core/test_aca_provider.py tests/test_core/test_daytona_provider.py tests/test_core/test_hf_sandbox_provider.py tests/test_core/test_kubernetes_provider.py tests/test_core/test_modal_provider.py tests/test_core/test_novita_provider.py tests/test_core/test_uv_provider.py tests/test_core/test_local_docker_provider.py tests/test_core/test_client_provider_cleanup.py tests/test_core/test_boxd_provider.py -q -m "not integration and not network and not docker" 505 passed, 4 skipped
PYTHONPATH=src:envs uv run pytest tests/test_core -q -m "not integration and not network and not docker" 655 passed, 4 skipped, 23 subtests passed
uv run ruff check, uv run ruff format --check and uv run usort check on the provider, its tests and the example Clean
SDK contract check (below) with boxd 0.3.1 and with 0.3.2, sockets blocked Every call binds. PASS on both.
Review harness (below) at 38033b3e: real SDK, gRPC router stubbed, sockets blocked 2 passed, 4 failed (the four test_required_* tests)
Review harness and the PR's tests against the suggested patch 54 passed. The PR's tests needed public=True in two places, the cleanup-warning assertion updated, time.monotonic patched in test_wait_for_ready_times_out, and the default budget in test_wait_for_ready_returns_on_healthy.
git merge-tree --write-tree origin/main 38033b3e (main at ca6df26a) Clean
Review harness: test_pr1603_review.py

Run from the repository root after uv sync --all-extras. The real Boxd client, request builders, mappers and exec and upload code run. Only SyncRouter.call and SyncRouter.bidi are replaced by a fake control plane, and Python sockets and DNS are blocked. The test_contract_* tests pass at the PR head. The test_required_* tests describe the behavior asked for above and fail at the PR head. The PUBLIC switch lets the same file run before and after the provider gains public=.

"""Offline review checks for PR 1603 (BoxdProvider) against the real boxd SDK.

The real `Boxd` client, request builders, mappers and exec/upload code run.
Only `SyncRouter.call`/`bidi` are replaced by a fake control plane, and Python
sockets and DNS are blocked. `test_contract_*` pass on the PR head.
`test_required_*` describe the behavior asked for in the review and fail there.

    PYTHONPATH=src:envs uv run pytest test_pr1603_review.py -q
"""

import inspect
import socket
from types import SimpleNamespace
from unittest.mock import patch

import pytest
from boxd._generated import api_pb2
from boxd._regions import SyncRouter
from boxd.errors import APIStatusError, NotFoundError
from openenv.core.containers.runtime.boxd_provider import BoxdProvider

ECHO = "ghcr.io/huggingface/openenv-echo-env:latest"
DOMAIN = "fra.boxd.sh"
SECRET = "s3cret-review-value"


class FakeControlPlane:
    def __init__(self):
        self.calls = []  # (method, request)
        self.exec_timeouts = []
        self.machines = {}  # vm_id -> name
        self.uploads = {}
        self.create_error = None  # raised after the machine is allocated
        self.container_state = "running"
        self.logs = ""
        self.clock = None  # advanced by each exec's timeout when set

    def call(self, router, method, request, *, region=None, **options):
        if method == "UploadFileStream":
            chunks = list(request)
            self.uploads[chunks[0].path] = b"".join(c.data for c in chunks)
            self.calls.append((method, chunks[0]))
            return api_pb2.UploadFileResponse(bytes_written=chunks[0].total_size)
        self.calls.append((method, request))
        if method == "CreateVm":
            vm_id = f"vm-{len(self.calls)}"
            self.machines[vm_id] = request.name
            if self.create_error is not None:
                raise self.create_error
            vm = api_pb2.GetVmResponse(
                vm_id=vm_id,
                name=request.name,
                status="pending",
                access_domain=DOMAIN,
                delete_at_ms=1 if request.delete_after_secs else 0,
            )
            return api_pb2.CreateVmResponse(vm_id=vm_id, name=request.name, machine=vm)
        if method == "GetVm":
            vm_id = self._resolve(request.vm_id)
            return api_pb2.GetVmResponse(
                vm_id=vm_id, name=self.machines[vm_id], status="running", access_domain=DOMAIN
            )
        if method == "SetBotProtection":
            return api_pb2.SetBotProtectionResponse()
        if method == "DestroyVm":
            vm_id = self._resolve(request.vm_id)
            if vm_id is None:
                raise NotFoundError("machine not found", code="not_found")
            del self.machines[vm_id]
            return api_pb2.DestroyVmResponse()
        raise AssertionError(f"unexpected RPC {method}")

    def _resolve(self, ref):
        # Like the real API, machine calls take an id or a name.
        if ref in self.machines:
            return ref
        return next((i for i, n in self.machines.items() if n == ref), None)

    def bidi(self, router, method, requests, *, timeout=None, region=None, machine=None):
        init = next(iter(requests))
        self.calls.append((method, init))
        self.exec_timeouts.append(timeout)
        if self.clock is not None and timeout is not None:
            self.clock.now += timeout
        out = b""
        if init.command.startswith("docker inspect"):
            out = self.container_state.encode()
        elif init.command.startswith("docker logs"):
            out = self.logs.encode()
        return iter([api_pb2.ExecChunk(data=out, exit_code=0)])

    def commands(self):
        return [r.command for m, r in self.calls if m == "Exec"]

    def requests(self, method):
        return [r for m, r in self.calls if m == method]


@pytest.fixture
def plane(monkeypatch):
    def offline(*args, **kwargs):
        raise AssertionError("unexpected network call")

    monkeypatch.setattr(socket.socket, "connect", offline)
    monkeypatch.setattr(socket, "create_connection", offline)
    monkeypatch.setattr(socket, "getaddrinfo", offline)
    fake = FakeControlPlane()
    monkeypatch.setattr(SyncRouter, "call", lambda self, *a, **k: fake.call(self, *a, **k))
    monkeypatch.setattr(SyncRouter, "bidi", lambda self, *a, **k: fake.bidi(self, *a, **k))
    monkeypatch.setattr(SyncRouter, "console_origin_for", lambda self, region: "")
    return fake


# Lets the same file run before and after the provider gains `public=`.
PUBLIC = {"public": True} if "public" in inspect.signature(BoxdProvider).parameters else {}


def _provider(**kwargs):
    return BoxdProvider(api_key="bxd_review_dummy", **{**PUBLIC, **kwargs})


def test_contract_real_sdk_round_trip(plane):
    provider = _provider(vcpu=4, memory="16G")
    url = provider.start_container(ECHO, env_vars={"TOKEN": SECRET})
    (vm_id, name), = plane.machines.items()
    assert url == f"https://{name}.{DOMAIN}"

    (create,) = plane.requests("CreateVm")
    assert create.isolated and dict(create.labels) == {"managed-by": "openenv"}
    assert create.delete_after_secs == 3600
    assert (create.config.vcpu, create.config.memory_bytes) == (4, 16 * 1024**3)
    # Idle timers and egress are left to the org's defaults.
    assert not create.config.HasField("srf")
    assert not plane.requests("SetVmEgressAllow")
    assert plane.requests("SetBotProtection")[0].mode == "off"
    # The secret travels only in the uploaded env file.
    assert plane.uploads["/home/boxd/openenv-env/server.env"] == f"TOKEN={SECRET}\n".encode()
    assert not any(SECRET in c for c in plane.commands())

    provider.close()
    assert plane.machines == {}


def test_contract_real_not_found_counts_as_deleted(plane):
    provider = _provider()
    provider.start_container(ECHO)
    plane.machines.clear()  # deleted by its delete_after timer
    provider.stop_container()
    with pytest.raises(RuntimeError, match="no active machine"):
        provider.machine_id


def test_required_public_ingress_is_an_explicit_opt_in(plane):
    # Criterion 4 of #1608: public ingress only behind e.g. public=True.
    with pytest.raises((TypeError, ValueError, RuntimeError)):
        BoxdProvider(api_key="bxd_review_dummy").start_container(ECHO)
    assert not plane.requests("CreateVm")


def test_required_machine_allocated_by_a_failed_create_is_deleted(plane):
    # CreateVm is not retried on DEADLINE_EXCEEDED because the server may
    # already have created the machine. The provider generated its name.
    plane.create_error = APIStatusError("Deadline Exceeded", code="deadline_exceeded")
    provider = _provider(delete_after=None)
    with pytest.raises(APIStatusError):
        provider.start_container(ECHO)
    provider.close()
    assert plane.machines == {}, f"left running with no timer: {plane.machines}"


def test_required_readiness_probe_stays_inside_the_budget(plane):
    provider = _provider()
    url = provider.start_container(ECHO)
    plane.clock = SimpleNamespace(now=0.0)
    plane.exec_timeouts.clear()
    with (
        patch("time.time", side_effect=lambda: plane.clock.now),
        patch("time.monotonic", side_effect=lambda: plane.clock.now),
        patch("time.sleep"),
        patch("requests.get", return_value=SimpleNamespace(status_code=502)),
    ):
        with pytest.raises(TimeoutError):
            provider.wait_for_ready(url, timeout_s=5)
    assert plane.clock.now <= 6, f"returned at t={plane.clock.now}s, budget 5s"


def test_required_debug_output_redacts_the_machine_url(plane):
    provider = _provider(surface_server_logs=True)
    url = provider.start_container(ECHO)
    host = url.removeprefix("https://")
    plane.container_state = "exited"
    plane.logs = f"GET /health Host: {host}\nTraceback: boom\n"
    with patch("requests.get", return_value=SimpleNamespace(status_code=502)):
        with pytest.raises(RuntimeError) as err:
            provider.wait_for_ready(url, timeout_s=5)
    assert "boom" in str(err.value)
    assert host not in str(err.value)

Result at 38033b3e:

test_required_public_ingress_is_an_explicit_opt_in
E   Failed: DID NOT RAISE any of (TypeError, ValueError, RuntimeError)
test_required_machine_allocated_by_a_failed_create_is_deleted
E   AssertionError: left running with no timer: {'vm-1': 'openenv-f9f0e59653b2'}
test_required_readiness_probe_stays_inside_the_budget
E   AssertionError: returned at t=30.0s, budget 5s
test_required_debug_output_redacts_the_machine_url
E   AssertionError: assert 'openenv-7eb....fra.boxd.sh' not in 'The environ...back: boom\n'
4 failed, 2 passed
SDK contract check: sdk_contract.py

Run it with any interpreter that has boxd installed. It binds each call the provider makes against the installed SDK's signatures and blocks the network. Both 0.3.1 and 0.3.2 print RESULT PASS [].

"""Bind every boxd SDK call BoxdProvider makes against the installed SDK."""

import inspect
import socket
import typing
from importlib.metadata import version


def _no_network(*args, **kwargs):
    raise AssertionError("unexpected network call")


socket.socket.connect = _no_network
socket.create_connection = _no_network
socket.getaddrinfo = _no_network

import boxd  # noqa: E402
from boxd import errors, models  # noqa: E402
from boxd.resources.machines import Files, Machines  # noqa: E402

print("boxd", version("boxd"))
failures = []


def bind(label, func, *args, **kwargs):
    try:
        inspect.signature(func).bind(*args, **kwargs)
        print(f"OK   {label}")
    except TypeError as exc:
        failures.append(label)
        print(f"FAIL {label}: {exc}")


SELF = object()
bind("Boxd.__init__", boxd.Boxd.__init__, SELF, api_key="k", region="nyc")
bind(
    "Machines.create",
    Machines.create,
    SELF,
    "openenv-0123456789ab",
    org=None,
    isolated=True,
    labels={"managed-by": "openenv"},
    vcpu=4,
    memory="16G",
    delete_after=3600,
)
bind("Machines.wait_until_ready", Machines.wait_until_ready, SELF, "m-1")
bind("Machines.set_bot_protection", Machines.set_bot_protection, SELF, "m-1", mode="off")
bind("Machines.exec", Machines.exec, SELF, "m-1", "true", env=None, timeout=30.0)
bind("Files.upload (bytes)", Files.upload, SELF, "m-1", "/p", b"x")
bind("Files.upload (str)", Files.upload, SELF, "m-1", "/p", "x")
bind("Machines.delete", Machines.delete, SELF, "m-1")
bind("Boxd.close", boxd.Boxd.close, SELF)

checks = {
    "errors.NotFoundError subclasses BoxdError": issubclass(
        errors.NotFoundError, errors.BoxdError
    ),
    "Machine.id": "id" in models.Machine.model_fields,
    "Machine.access": "access" in models.Machine.model_fields,
    "MachineAccess.url": "url" in models.MachineAccess.model_fields,
    "ExecResult.exit_code/stdout/stderr": {"exit_code", "stdout", "stderr"}
    <= set(models.ExecResult.model_fields),
    "BotProtectionMode has 'off'": "off" in typing.get_args(models.BotProtectionMode),
}
client = boxd.Boxd(api_key="bxd_review_dummy", region="nyc")  # no network call
checks["Boxd() has .machines.files"] = isinstance(client.machines.files, Files)
client.close()
for label, ok in checks.items():
    print(("OK   " if ok else "FAIL ") + label)
    if not ok:
        failures.append(label)
print("RESULT", "FAIL" if failures else "PASS", failures)
Suggested patch for findings 1, 2, 4 and 6 (verified offline)

With this patch, the 48 PR tests (with the updates listed in the test table) and the six harness tests pass with the network blocked: 54 passed. It does not cover the docs, the example, finding 3, 5 or 7, or the nits.

--- a/src/openenv/core/containers/runtime/boxd_provider.py
+++ b/src/openenv/core/containers/runtime/boxd_provider.py
@@ -70,8 +70,7 @@
 
     As a backstop, boxd deletes the machine ``delete_after`` seconds after it
     was created (one hour by default), even if ``stop_container()`` never
-    runs. This also covers a Ctrl-C during the create call itself, before the
-    provider knows the machine id and can delete it.
+    runs, for example when the process is killed.
 
     Examples:
 
@@ -88,6 +87,7 @@
     def __init__(
         self,
         *,
+        public: bool = False,
         image: str | None = None,
         env_vars: dict[str, str] | None = None,
         api_key: str | None = None,
@@ -103,6 +103,10 @@
     ):
         """
         Args:
+            public (`bool`, *optional*, defaults to `False`):
+                Must be `True`. Confirms that the machine's URL is public:
+                anyone who has it can drive the environment while the machine
+                runs.
             image (`str`, *optional*):
                 Registry image or ``"dockerfile:<path>"`` source to use when
                 ``start_container()`` is called without an image.
@@ -138,6 +142,11 @@
                 its output, and startup-crash errors include the tail of the
                 container's output with injected env-var values redacted.
         """
+        if not public:
+            raise ValueError(
+                "BoxdProvider requires public=True: the machine's URL is "
+                "reachable from the internet with no login in front of it."
+            )
         if delete_after is not None and (
             isinstance(delete_after, bool)
             or not isinstance(delete_after, int)
@@ -335,21 +344,25 @@
                     "and values must be single lines."
                 )
         self._redact_values = {v for v in effective_env_vars.values() if v}
-
-        machine = self._client.machines.create(
-            f"{self._name_prefix}-{uuid.uuid4().hex[:12]}",
-            org=self._org,
-            isolated=True,
-            labels={"managed-by": "openenv"},
-            vcpu=self._vcpu,
-            memory=self._memory,
-            delete_after=self._delete_after,
-        )
-        self._machine_id = machine.id
 
+        name = f"{self._name_prefix}-{uuid.uuid4().hex[:12]}"
+        # Until create answers, the name is the only handle on the machine.
+        self._machine_id = name
         try:
+            machine = self._client.machines.create(
+                name,
+                org=self._org,
+                isolated=True,
+                labels={"managed-by": "openenv"},
+                vcpu=self._vcpu,
+                memory=self._memory,
+                delete_after=self._delete_after,
+            )
+            self._machine_id = machine.id
             ready = self._client.machines.wait_until_ready(machine.id)
             url = _require_secure_url(ready.access.url)
+            # The host is the secret part of the URL: keep it out of debug logs.
+            self._redact_values.add(url.removeprefix("https://"))
             # EnvClient is not a browser, so the browser check on the
             # machine's URL is turned off.
             self._client.machines.set_bot_protection(machine.id, mode="off")
@@ -391,9 +404,8 @@
                 self.stop_container()
             except Exception:
                 logger.warning(
-                    "Could not delete boxd machine %s after a failed start; "
-                    "delete it with stop_container() or in the boxd console.",
-                    machine.id,
+                    "Could not delete the boxd machine after a failed start; "
+                    "retry stop_container() or close()."
                 )
             raise
 
@@ -442,7 +454,7 @@
                 "to include a redacted excerpt."
             )
         log = self._exec(f"docker logs --tail 100 {_CONTAINER} 2>&1").stdout
-        for value in self._redact_values:
+        for value in sorted(self._redact_values, key=len, reverse=True):
             log = log.replace(value, "***")
         return f"{base}\nLog (redacted):\n{log[-2000:]}"
 
@@ -463,23 +475,25 @@
         import requests
 
         health_url = f"{base_url}/health"
-        deadline = time.time() + timeout_s
-        while time.time() < deadline:
+        deadline = time.monotonic() + timeout_s
+        while (remaining := deadline - time.monotonic()) > 0:
             try:
-                response = requests.get(health_url, timeout=5.0)
+                response = requests.get(health_url, timeout=min(5.0, remaining))
                 if response.status_code == 200:
                     return
             except requests.RequestException:
                 pass
 
-            if self._machine_id is not None:
+            remaining = deadline - time.monotonic()
+            if self._machine_id is not None and remaining > 0:
                 state = self._exec(
-                    f"docker inspect -f '{{{{.State.Status}}}}' {_CONTAINER}"
+                    f"docker inspect -f '{{{{.State.Status}}}}' {_CONTAINER}",
+                    timeout=min(30.0, remaining),
                 ).stdout.strip()
                 if state in ("exited", "dead"):
                     raise RuntimeError(self._server_died_message())
 
-            time.sleep(1.0)
+            time.sleep(min(1.0, max(0.0, deadline - time.monotonic())))
 
         raise TimeoutError(f"The boxd machine did not become ready within {timeout_s}s")

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants