Repository navigation
Add sandbox providers for CoreWeave and Wandb - #890
matthoare117-wandb wants to merge 11 commits into
Conversation
matthoare117-wandb
changed the title
|
Cool pull request. Thanks. Reviewing now. |
sergiopaniego
left a comment
sergiopaniego
left a comment
There was a problem hiding this comment.
Thanks for the PR! Some comments from a first pass:
Transport security (S1)
Both ModalProvider and ACASandboxProvider enforce https/wss transport via _require_secure_url() (RFC 002 invariant S1), because EnvClient derives the WebSocket URL from base_url (http:// becomes cleartext ws:// in convert_to_ws_url). CWSandboxProvider defaults to url_scheme="http" and returns http://{service_address} when the address has no scheme. Combined with ingress_mode="public" by default, client traffic can travel in cleartext against a public endpoint. EnvClient does not block this on its own (it only skips the proxy for localhost, no cleartext warning for remote hosts). Could we add the same _require_secure_url guard (or force/validate https) before returning the URL?
Public ingress opt-in (S2)
ACASandboxProvider requires an explicit anonymous_port=True (RFC 002 invariant S2, "public exposure is never implicit"). Here ingress_mode="public" is the default with no explicit opt-in or warning. Could we require an explicit opt-in (or at least warn) to match the ACA pattern?
Docs
The provider table and "Per-provider setup" section in docs/source/guides/runtime-providers.md, plus the container-providers section in docs/source/reference/core.md, are not updated, so these two providers are currently undocumented. Adding table rows and setup blocks for CWSandbox and W&B (with a "Full example" link, like Daytona/Modal have) would keep the providers doc complete.
Examples
The PR adds an example for W&B (the subclass) but not for CWSandboxProvider, which is the base provider holding all the logic. It would be good to add a CWSandbox example (or generalize the current one to cover both). A couple of style nits to align the W&B example with modal_echo_env.py:
- The
try/except Exception: provider.stop_container(); raisearoundfrom_docker_imageplus a separateasync with envmixes two cleanup styles. Modal uses the cleanerwith Provider() as provider: start_container / wait_for_ready / stoppattern. - Prefer
loggingoverprintto match the other examples. - The example references
registry.hf.space/openenv-echo-env:latestwith a hardcodedSERVER_CMD, whereas Modal/Daytona build the image withProvider.image_from_dockerfile(...). A short note on where that image comes from would help.
RFC status
The PR marks RFC as "Not required", but Modal and ACA landed under the proposed "Cloud Sandbox Providers" amendment to RFC 002, which is where the S1/S2 invariants come from. A new cloud provider with public ingress should reference and satisfy those invariants rather than opt out (ties into the two points above).
Minor
wait_for_readydocstring says "respond to /health and /ws" but only polls/health.- The two new provider files and the two test files are missing the BSD copyright header that all other providers (and
__init__.py) carry. - If S1/S2 get implemented, a test asserting the resulting URL is secure and that implicit public ingress is rejected would be worth adding.
|
Adding one stack-compatibility note, and +1 to the S1/S2 feedback above.
|
|
🙏 Thanks so much for the thoughtful review. I really appreciate you taking the time, and I’ll address the feedback. |
|
Hey Matt, how's this one going? |
|
Hey! Thanks for following up. We’re currently implementing HTTPS endpoint support in CoreWeave Sandboxes so it’ll work with this integration. We’re hoping to have it ready soon, and once it lands we’ll update this PR. Looking forward to getting this landed! |
|
Fixed in ebad7d7. Retain the sandbox handle, base URL and redaction values when stop/delete fails, so close can retry and start_container cannot orphan the old sandbox by replacing it. Clear ownership after successful cleanup. Three failure/retry regressions failed before the fix; all 29 CoreWeave/W&B mocked-provider tests now pass. No live SDK/provider validation. Public ingress and HTTP defaults (S1/S2), startup contract and documentation still require review; this is not merge signoff. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f50057f. Configure here.
burtenshaw
left a comment
burtenshaw
left a comment
There was a problem hiding this comment.
NEEDS AUTHOR CHANGES
Reviewed 2df6b809b5f91d5a6295d174952079b3161a157c. Pushed the mechanical URL normalization fix 6716be606e46904fcfe7e0a4de7ce79b333e75f2; the three blockers below remain at that head.
2df6b809 addressed the earlier default transport, ingress and image startup findings on the author's behalf:
src/openenv/core/containers/runtime/cwsandbox_provider.py:47now declaresurl_scheme: str = "https"; line 53 rejects other schemes. At line 366,parsed.scheme != "https"rejects plaintext and bare service addresses before server commands run. Both providers reject HTTP, WS, bare host:port and credential-bearing addresses and clean up the sandbox. Synthetic HTTPS endpoints reachEnvClientas HTTPS and its connection function aswss://.../ws, through both provider-owned startup andfrom_docker_image.- At
cwsandbox_provider.py:36,ingress_mode: str | None = None; line 142 rejects omitted/empty ingress beforeSandbox.run. Public ingress requires an explicitingress_mode="public".wandb_sandbox_provider.py:10inherits this behavior. This resolves the implicit-public finding, but full S2/S4 capability hygiene and S6 lifetime requirements still have the failures below. - The startup-contract finding was concrete:
EnvClient(provider=...)callsstart_container()without an image. Constructorimageat line 34 and fallback at line 136 now support that call for both providers. An explicit start image overrides it; missing images fail before provisioning. Constructorenv_varsparity remains optional follow-up, not a startup blocker. - Cleanup regressions confirm ownership survives failed deletion and blocks replacement, while
if stop_error is None or deleted:at line 239 releases ownership after successful deletion. Failed SDK wait, command discovery, command launch and client readiness clean up; failed cleanup retains the handle for retry.
One additional S1 case failed at 2df6b809: the accepted address HTTPS://sandbox.example.test:8000 reached the client unchanged and became ws://HTTPS://sandbox.example.test:8000/ws. Commit 6716be60 returns parsed.geturl() at cwsandbox_provider.py:377, normalizing the scheme. The regression at tests/test_core/test_cwsandbox_provider.py:212 failed before this fix and passes afterward for CoreWeave and W&B. The checked transport cases now remain HTTPS/WSS.
Bugbot reconciliation: “Public ingress is the silent default”, “Provider-owned startup is unsupported”, and “Stop error retains ownership after delete” are stale and were already resolved when this review started. The unresolved “Remote URLs default to cleartext HTTP” claim is also stale: the current default is HTTPS and insecure addresses are rejected. Its October 26 inline maintainer reply explicitly kept the hosted TLS integration question open. That question remains in blocker 1; it is separate from the fixed cleartext-default bug.
Blocking findings:
- [P1] Make a supported SDK and HTTPS endpoint path work.
pyproject.toml:54andpyproject.toml:57admit the PyPI releases selected by a freshuv sync --all-extras:cwsandbox 1.25.0andwandb 0.31.0. For either provider,Provider(image="echo:stored", ingress_mode="public").start_container()fails atcwsandbox_provider.py:161withTypeError: NetworkOptions.__init__() got an unexpected keyword argument 'ingress_mode', before provisioning. The real SDK has also replacedservice_addresswithservice_urls. The minimum releases,cwsandbox 0.23.3andwandb 0.27.0, exist and satisfy the Python markers, but their documented bare-address path is deliberately rejected at line 366. Pinning to those minima alone does not establish a working hosted HTTPS deployment. The current SDK supplies HTTPS endpoint configuration throughService/Endpoint; this adapter does not request it. Smallest complete fix: map one supported SDK generation to an explicit HTTPS endpoint and its returned URL, constrain both extras to the verified compatible range, and add strict SDK contract coverage plus evidence that the exposed/wsupgrades successfully. Keep the insecure-address rejection. Sources: PyPI cwsandbox, PyPI W&B, current endpoint types, minimum service-address contract. - [P1] Remove capability URLs and raw SDK errors from diagnostics.
cwsandbox_provider.py:276inserts the ingress URL into every readiness timeout; line 274 also includes raw request errors. Callingwait_for_ready("https://sensitive-capability.example.test", timeout_s=0)reproduces the URL disclosure for both providers. Authentication errors containing a synthetic token are interpolated atcwsandbox_provider.py:92andwandb_sandbox_provider.py:31. A failed-start cleanup warning atcwsandbox_provider.py:205logs the raw exception traceback throughexc_info=cleanup_exc. Tests reproduce all three disclosures for both providers. Smallest fix: use fixed or sanitized messages at these boundaries, omit raw exception chaining/tracebacks, and keep regression assertions on both exception text and captured logs. Default server-output suppression already passes; it does not cover these paths. - [P2] Require or set a finite sandbox lifetime.
cwsandbox_provider.py:40defaultsmax_lifetime_secondstoNone, and line 184 removes that argument fromSandbox.run. Both providers' default calls omit it. Constructing the real minimum SDK with the captured kwargs leaves its effective lifetimeNone; W&B 0.27.0 re-exports the same SDK. The configured request timeout does not impose a sandbox lifetime. This leaves time boxing to an unspecified runner policy. Smallest fix: require a finite positivemax_lifetime_secondsbefore provisioning, or supply and document a finite default. Sources: minimum SDK lifetime defaults, W&B minimum SDK wrapper.
Documentation was a concrete request for provider table rows, per-provider install/auth/setup blocks, API-reference entries and an example link. docs/source/guides/runtime-providers.md:322 now explains image ownership, explicit ingress and the unsupported bare-address path; examples/wandb_sandbox_echo_env.py:1 now states its TLS prerequisite. The table at guide line 56, setup section at line 159 and reference at docs/source/reference/core.md:273 still omit these providers. Completing those entries and a CoreWeave/shared example is a nonblocking suggestion. Also correct the /ws readiness claim at cwsandbox_provider.py:252, document URL lifetime/reconnect behavior and the egress_mode="internet" posture, and state the required runner resource/IMDS restrictions. No live metadata access or runner-policy violation was demonstrated, so those remain suggestions. The proposed RFC 002 amendment covers these providers; a separate protocol RFC is not needed for the existing adapter contract.
Validation:
- Provider suites plus
tests/test_core/test_client_provider_cleanup.py: 131 passed, 4 skipped at the original head; 132 passed, 4 skipped after the pushed regression/fix. Changed-file usort, Ruff formatting, Ruff lint andgit diff --checkpass. - Additional offline audit: 32 passed and 10 intentional failing requirement tests covering the three blockers for both providers. The audit tests are in the collapsed block below. Save them as
tests/test_core/test_pr890_audit.pyand runPYTHONPATH=src:envs uv run pytest tests/test_core/test_pr890_audit.py -q --tb=short. No live sandbox was provisioned. - Both provider modules import with SDK imports blocked. Construction without the optional SDK raises the existing actionable installation/auth error. Core installation remains free of these optional SDK imports. The existing provider tests are offline, but their permissive fakes missed the installed SDK incompatibility.
PYTHONPATH=src:envs uv run pytest tests/core -q: 873 passed, 9 skipped, 5 failed in unchanged harness-process tests with macOSos.killpg(..., 0)PermissionError. Three of those failures reproduce at merge-basef26cb132; the other two pass in isolation. They are not provider findings. Repository Ruff lint passes; repository formatting checks flag unchanged files, including the two known usort failures.- The original head's recorded CI checks passed, with Bugbot neutral. The PR is behind current main.
Audit tests (tests/test_core/test_pr890_audit.py)
"""Offline PR 890 review checks. Synthetic credentials only; never provision."""
import asyncio
import builtins
import importlib
import json
import logging
import sys
import traceback
from pathlib import Path
from types import SimpleNamespace
from unittest.mock import AsyncMock, patch
import pytest
import requests
sys.path.insert(0, str(Path.cwd() / 'tests/test_core'))
import test_cwsandbox_provider as cw_tests
import test_wandb_sandbox_provider as wb_tests
from openenv.core.containers.runtime.cwsandbox_provider import CWSandboxProvider
from openenv.core.containers.runtime.wandb_sandbox_provider import WandbSandboxProvider
from openenv.core.generic_client import GenericEnvClient
@pytest.fixture(params=['coreweave', 'wandb'])
def provider_case(request):
if request.param == 'coreweave':
return CWSandboxProvider, cw_tests.fake_sdk.__wrapped__()
return WandbSandboxProvider, wb_tests.fake_wandb_sdk.__wrapped__()
@pytest.mark.parametrize('mode', ['omitted', None, ''])
def test_ingress_opt_in_precedes_provisioning(provider_case, mode):
provider_cls, sdk = provider_case
kwargs = {} if mode == 'omitted' else {'ingress_mode': mode}
provider = provider_cls(image='echo:stored', sdk=sdk, **kwargs)
with pytest.raises(ValueError, match='explicit ingress_mode'):
provider.start_container()
assert sdk.Sandbox.run_count == 0
@pytest.mark.parametrize('scheme', ['http', 'ws', 'wss'])
def test_constructor_rejects_insecure_or_unsupported_scheme(provider_case, scheme):
provider_cls, sdk = provider_case
with pytest.raises(ValueError, match='HTTPS'):
provider_cls(sdk=sdk, url_scheme=scheme)
assert sdk.Sandbox.run_count == 0
@pytest.mark.parametrize('address', [
'http://sandbox.example.test:8000',
'ws://sandbox.example.test:8000',
'sandbox.example.test:443',
'https://user:dummy-secret@sandbox.example.test',
])
def test_unacceptable_endpoint_is_cleaned_before_commands(provider_case, address):
provider_cls, sdk = provider_case
sdk.Sandbox.instance.service_address = address
provider = provider_cls(sdk=sdk, ingress_mode='public')
with pytest.raises(RuntimeError, match='HTTPS service_address') as error:
provider.start_container('echo:latest')
assert address not in str(error.value)
assert sdk.Sandbox.instance.stopped
assert len(sdk.Sandbox.deleted) == 1
assert provider.sandbox_id is None
assert not sdk.Sandbox.instance.exec_calls
@pytest.mark.parametrize('flow', ['provider_owned', 'from_docker_image'])
def test_envclient_receives_https_and_opens_wss(provider_case, flow):
provider_cls, sdk = provider_case
provider = provider_cls(sdk=sdk, image='echo:stored', ingress_mode='public')
address = sdk.Sandbox.instance.service_address
with patch.object(provider, 'wait_for_ready'):
if flow == 'provider_owned':
client = GenericEnvClient(provider=provider)
else:
client = GenericEnvClient.from_docker_image(
'echo:explicit', provider=provider
)._consume()
ws = SimpleNamespace(state=None, send=AsyncMock(), close=AsyncMock())
with patch('openenv.core.env_client.ws_connect', new_callable=AsyncMock,
return_value=ws) as connect:
async def exercise():
await client.connect()
assert client.base_url == address
assert connect.call_args.args[0] == address.replace('https://', 'wss://', 1) + '/ws'
assert sdk.Sandbox.last_run_kwargs['container_image'] == (
'echo:stored' if flow == 'provider_owned' else 'echo:explicit'
)
await client.close()
asyncio.run(exercise())
assert sdk.Sandbox.instance.stopped
assert len(sdk.Sandbox.deleted) == 1
@pytest.mark.parametrize('failure', ['wait', 'discovery', 'exec', 'readiness'])
def test_client_failed_start_releases_sandbox(provider_case, failure):
provider_cls, sdk = provider_case
provider = provider_cls(sdk=sdk, image='echo:stored', ingress_mode='public')
client = GenericEnvClient(provider=provider)
owner, name = {
'wait': (sdk.Sandbox.instance, 'wait'),
'discovery': (provider, '_discover_server_cmd'),
'exec': (provider, '_start_server'),
'readiness': (provider, 'wait_for_ready'),
}[failure]
with patch.object(owner, name, side_effect=ValueError('synthetic startup failure')):
async def exercise():
with pytest.raises(ValueError, match='synthetic startup failure'):
await client.connect()
asyncio.run(exercise())
assert sdk.Sandbox.instance.stopped
assert len(sdk.Sandbox.deleted) == 1
assert provider.sandbox_id is None
# These tests express requirements the current head violates.
def test_readiness_errors_do_not_expose_ingress_url(provider_case):
provider_cls, sdk = provider_case
provider = provider_cls(sdk=sdk, ingress_mode='public')
address = 'https://sensitive-capability.example.test'
with patch('openenv.core.containers.runtime.cwsandbox_provider.time.time',
side_effect=[0, 1]):
with pytest.raises(TimeoutError) as error:
provider.wait_for_ready(address, timeout_s=0)
assert address not in str(error.value)
def test_auth_errors_do_not_expose_tokens_or_urls(provider_case):
provider_cls, sdk = provider_case
marker = 'DUMMY_REVIEW_TOKEN https://sensitive-capability.example.test'
sdk.Sandbox.list_result = cw_tests._FakeRef(sdk.CWSandboxAuthenticationError(marker))
with pytest.raises(SystemExit) as error:
provider_cls.preflight(sdk=sdk)
assert marker not in str(error.value)
assert marker not in ''.join(traceback.format_exception(error.value))
def test_cleanup_warning_does_not_expose_sdk_error(provider_case, caplog):
provider_cls, sdk = provider_case
provider = provider_cls(sdk=sdk, ingress_mode='public')
marker = 'DUMMY_REVIEW_TOKEN https://sensitive-capability.example.test'
with (
patch.object(sdk.Sandbox.instance, 'wait', side_effect=ValueError('startup failed')),
patch.object(provider, 'stop_container', side_effect=RuntimeError(marker)),
caplog.at_level(logging.WARNING),
pytest.raises(ValueError, match='startup failed'),
):
provider.start_container('echo:latest')
assert marker not in caplog.text
def test_a_finite_positive_lifetime_is_sent_by_default(provider_case):
provider_cls, sdk = provider_case
provider = provider_cls(sdk=sdk, ingress_mode='public')
try:
provider.start_container('echo:latest')
lifetime = sdk.Sandbox.last_run_kwargs.get('max_lifetime_seconds')
assert lifetime is not None and 0 < lifetime < float('inf')
finally:
provider.close()
def test_installed_sdk_network_contract_is_compatible(provider_case):
provider_cls, _ = provider_case
module_name = 'cwsandbox' if provider_cls is CWSandboxProvider else 'wandb.sandbox'
sdk = importlib.import_module(module_name)
with patch.object(sdk.Sandbox, 'run', side_effect=RuntimeError('provisioning intercepted')) as run:
with pytest.raises(RuntimeError, match='provisioning intercepted'):
provider_cls(ingress_mode='public', image='echo:stored').start_container()
assert run.call_count == 1
Summary
Adds OpenEnv support for running environments in both CoreWeave Sandboxes and W&B Serverless Sandboxes.
Includes optional
cwsandboxandwandbextras, a W&B sandbox echo example, and unit tests for the new sandbox providers.Type of Change
Alignment Checklist
Before submitting, verify:
I have read
.claude/docs/PRINCIPLES.mdand this PR aligns with our principlesI have checked
.claude/docs/INVARIANTS.mdand no invariants are violatedI have run
/pre-submit-pr(orbash .claude/hooks/lint.shand tests) and addressed all issuesNote:
/pre-submit-prwas run via its underlying checks. The only remaining repo-wide formatting failures (uv run usort check src/ tests/anduv run ruff format src/ tests/ envs/ --check) reproduce onorigin/mainin a separate worktree, so they are pre-existing unrelated formatting drift and not new issues introduced by this PR.RFC Status
Test Plan
Claude Code Review
N/A
Note
Medium Risk
New remote runtime paths depend on correct HTTPS ingress and sandbox auth; public ingress is explicit, but misconfiguration could expose workloads or leak secrets if
surface_server_logsis enabled.Overview
Adds CoreWeave Sandbox and W&B Serverless Sandbox runtime providers so OpenEnv can provision remote sandboxes, launch the environment server inside the container, and connect via
GenericEnvClient.CWSandboxProviderdrives thecwsandboxSDK lifecycle (run → wait → validate URL → exec server start →/healthreadiness).WandbSandboxProviderreuses that behavior throughwandb.sandboxwith W&B auth and optional integration metadata. New optionalcwsandboxandwandbextras inpyproject.toml, plus docs on explicitingress_mode, HTTPS-onlyservice_address, and that bare SDK addresses /url_schemedo not imply TLS.Startup and readiness are fail-closed: missing image or ingress, non-HTTPS addresses, or startup errors trigger cleanup (stop/delete per policy) before clients connect; server crash logs stay hidden by default with best-effort redaction of injected env secrets. A conditional
examples/wandb_sandbox_echo_env.pyand broad mocked unit tests cover provisioning,GenericEnvClientwiring, and cleanup/ownership edge cases.Reviewed by Cursor Bugbot for commit 6716be6. Bugbot is set up for automated code reviews on this repo. Configure here.