feat(env-client): sync bootstrap constructors + public base_url (#935)

[OfficialAbhinavSingh]

Summary

Closes #935. Adds synchronous bootstrap entry points to EnvClient — from_docker_image_sync() / from_env_sync() — plus a public read-only base_url property, so synchronous consumers (e.g. the TRL GRPO example rollout loops) can spin up a container without an await and without hand-rolling the start_container() / wait_for_ready() sequence the async factories already implement.

The async from_docker_image / from_env are unchanged in behavior — both entry points now share small _bootstrap_container / _bootstrap_env helpers (start + readiness wait, no connect), so the only difference between sync and async is how the WebSocket is connected. Non-breaking: no existing signatures change, AutoEnv untouched.

This is option A from the issue discussion (👍'd by @burtenshaw). Option B (making the existing constructors dual-mode) was prototyped but breaks AutoEnv.from_env, which wraps the constructors in run_async_safely() → asyncio.run(coro) and needs a real coroutine.

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 (serves "minimize lifecycle deltas" — sync training loops use the same client; no principle traded off)
• I have checked .claude/docs/INVARIANTS.md and no invariants are violated (Gym reset/step/state signatures, generics, and the WebSocket/MCP boundary are all unchanged; additive to the bootstrap path only)
• I have run 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)

Raised directly in #935: asked @burtenshaw whether this additive change to src/openenv/core/ warrants an RFC. It adds public API but is non-breaking and fixes a filed limitation. Happy to write a short RFC before merge if preferred.

Test Plan

• PYTHONPATH=src:envs uv run pytest tests/ -m "not integration" — full core suite green (872 passed), including new TestSyncBootstrapConstructors (sync ctor returns a concrete connected client; kwarg forwarding; UV start-failure releases the provider) and TestBaseUrlProperty (normalization, None before start, read-only).
• uv run usort check + uv run ruff format --check + uv run ruff check on src/ tests/ — clean.
• Reviewers can verify the sync path with:

  env = MyEnv.from_docker_image_sync("coding-env:latest")  # no await
  print(env.base_url)
  env.reset()

Claude Code Review

Self-check against the two-tier model: no bugs, uninitialized state, or debug code; aligns with PRINCIPLES.md (same-interface training) and violates no INVARIANTS.md (Gym reset/step/state signatures, generics, and the agent/infra boundary all preserved). No alignment flags. Cursor Bugbot independently flagged this Low Risk — its summary is below.

---

Note

Low Risk
Additive, non-breaking API on the client bootstrap path; async factory behavior is preserved via shared helpers with existing UV startup cleanup.

Overview
Adds synchronous bootstrap entry points on EnvClient so callers without await can spin up Docker or Hugging Face Spaces the same way the async factories do: from_docker_image_sync() and from_env_sync() return an already-connected client by reusing shared _bootstrap_container / _bootstrap_env helpers (start + readiness only) and connecting via _run_sync(_connect_async) on the existing sync background loop.

Async from_docker_image and from_env behavior is unchanged; they now delegate bootstrap to those helpers then await connect(). UV-mode startup failures still call provider.stop() before re-raise when no client exists yet.

Restores a read-only base_url property (normalized URL, or None before lazy provider start) for sync consumers after the core refactor removed the public attribute.

Tests cover connected sync clients, kwarg forwarding, UV start-failure cleanup, and base_url normalization / read-only semantics.

^{Reviewed by Cursor Bugbot for commit 769eed0. Bugbot is set up for automated code reviews on this repo. Configure here.}

[burtenshaw]

Thanks for the PR @OfficialAbhinavSingh. I'm not a huge fan of this API design. Do you have any other suggestions? For example, chaining sync().

[OfficialAbhinavSingh]

Thanks for the feedback @burtenshaw — happy to rework it, and chaining does feel cleaner than the _sync suffixes.

The shape I have in mind: make from_docker_image / from_env regular classmethods returning a small awaitable handle (same idea as the existing _AutoAsyncResult), so both of these fall out:

env = await MyEnv.from_docker_image("coding-env:latest")   # unchanged
env = MyEnv.from_docker_image("coding-env:latest").sync()  # SyncEnvClient, no await

A nice side effect: AutoEnv's run_async_safely() thread-pool workaround around these constructors becomes a plain .sync() call, so the bootstrap connection lives on the persistent background loop instead of a throwaway asyncio.run loop.

Two consequences worth flagging before I build it:

• .sync() would return SyncEnvClient, consistent with the instance .sync().
• Bootstrap becomes lazy — the container starts at await / .sync() time rather than at call time.

I'd keep the public read-only base_url from this PR as-is.

Alternatively, if you'd rather avoid new factory API entirely: constructor-owned providers (e.g. LocalDockerProvider(image=...)) would make the existing EnvClient(provider=...).sync() chain work with zero new client methods — today that path raises because start_container() requires the image at call time. Happy to go that way instead.

Does either shape match what you had in mind? I'll rework the PR to whichever you prefer.

[OfficialAbhinavSingh]

Reworked to the chained .sync() shape you suggested, @burtenshaw — dropped the _sync suffix methods entirely.

from_docker_image / from_env are now regular classmethods that return a small awaitable bootstrap handle, so a single factory serves both modes:

env = await MyEnv.from_docker_image("coding-env:latest")   # async, unchanged
env = MyEnv.from_docker_image("coding-env:latest").sync()   # SyncEnvClient, no await

.sync() returns a SyncEnvClient, consistent with the instance-level .sync(). Bootstrap is lazy — the container/Space starts when the handle is awaited or .sync()'d, not at call time.

Non-breaking notes:

• The async path is unchanged (await from_docker_image(...) still returns the connected async client).
• AutoEnv keeps returning the async client; it resolves the handle through the existing run_async_safely path, so its return type and sync/async-agnostic behavior are preserved.
• Public read-only base_url from the original PR is kept as-is.

Full core suite is green (1435 passed) including the reworked .sync() bootstrap tests; usort / ruff format / ruff check clean on the changed files. Happy to adjust naming or the handle's return type if you'd prefer something different.

[OfficialAbhinavSingh]

Addressed both Bugbot findings in 3ebd09a (force-pushed):

High — asyncio.run / run_async_safely compatibility. Good catch. _BootstrapResult is now a full collections.abc.Coroutine (implements send / throw / close), so it's a drop-in for the previous async def factories: await factory(...), asyncio.run(factory(...)), and run_async_safely(factory(...)) all work again, alongside the new factory(...).sync() chain. As a result AutoEnv needs no changes at all — I dropped the _resolve_bootstrap shim, and auto_env.py is now byte-identical to main. Added a regression test asserting asyncio.iscoroutine(handle) plus asyncio.run(...) / run_async_safely(...) each return a connected client.

Medium — draft files. NUDGE_COMMENT.md / PR_BODY.md / PR_BODY_TEMPLATE.md / REPLY_DRAFT.md were local scratch that slipped in via a stray git add -A — removed from the tree.

Full core suite green (1436 passed) and usort / ruff format / ruff check clean on the changed files.

[OfficialAbhinavSingh]

Fixed the UV startup cleanup gap in feaa418 — and applied the same guard to the docker path.

Both branches of _bootstrap_env now construct the client inside the try/except that releases the provider, so a failure after a successful start() / start_container() — including an EnvClient.__init__ failure such as an invalid OPENENV_CLIENT_MODE — stops the spawned process / container (and, for a git+ project_path, the temp clone) before re-raising, instead of leaking it.

Added two regression tests: start succeeds, then construction fails on an invalid mode, and the provider's stop() / stop_container() is asserted called once.

Full core suite green (1438 passed), usort / ruff clean on the changed files.

[OfficialAbhinavSingh]

Good catch on the asymmetry — fixed in 2a44b17. _bootstrap_container (the from_docker_image path) now wraps start / readiness / construction in the same try/except and calls stop_container() on failure, matching _bootstrap_env.

I also audited every provider start site in env_client.py to close the whole class: the three bootstrap helpers (_bootstrap_container, _bootstrap_env docker + uv) now all release the provider if anything fails before a client exists, and _start_provider_if_needed was already covered by _connect_async's close() on failure. Added a regression test for the from_docker_image construction-failure path.

Full core suite green (1439 passed), lint clean.

[OfficialAbhinavSingh]

Two parts here, one of which I think is a false positive:

Shared-kwargs mutation — I believe this is a false positive. _bootstrap_env(**provider_kwargs) collects the kwargs into a fresh local dict on each call, so the .pop()s mutate that local copy, not the outer dict the lazy lambda closes over. A second resolve re-unpacks the unchanged outer dict. Quick check:

def bootstrap_env(repo_id, *, use_docker=True, provider=None, **provider_kwargs):
    start_args = {}
    for k in ("port", "env_vars", "workers"):
        if k in provider_kwargs:
            start_args[k] = provider_kwargs.pop(k)
    tag = provider_kwargs.pop("tag", "latest")
    return start_args, tag

outer = {"env_vars": {"A": "1"}, "tag": "v2"}
lam = lambda: bootstrap_env("repo", **outer)
print(lam())   # ({'env_vars': {'A': '1'}}, 'v2')
print(lam())   # ({'env_vars': {'A': '1'}}, 'v2')  -> identical, outer unchanged

So a retry keeps the original tag / env_vars. Happy to be corrected if I'm missing a path.

Single-use guard — good point, added in 5e70d40. The handle previously had no reuse protection, so a second .sync() (or await then .sync()) would start a second container/Space. It now tracks a one-shot _consume() and raises RuntimeError on a second resolve, restoring the "can't drive twice" safety a native coroutine had. Added a test asserting the second .sync() raises and only one container is started.

Full core suite green (1440 passed), lint clean.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 5e70d40. Configure here.}

[OfficialAbhinavSingh]

Fixed in HEAD (force-pushed). _BootstrapResult.sync() now wraps the connect step in try/except: since _consume() has already started the provider, a failure in the sync-loop setup (before _connect_async's own close() cleanup can run) now releases the provider via a new best-effort _stop_provider_best_effort() — stopping it directly rather than routing through the possibly-broken sync loop.

The async path was already covered (_resolve_async → await connect() → _connect_async cleans up on failure). With this, every path that starts a provider before a client is usable releases it on failure: the three bootstrap helpers, the async connect, and now the sync connect.

Added a regression test that forces the sync setup to fail after the container starts and asserts stop_container() is called. Full core suite green (1441 passed), lint clean.