[RFC 005] 3/4: HarnessEnvironment, subprocess helper, and MCP tool bridge by splusq · Pull Request #1099 · huggingface/OpenEnv

splusq · 2026-08-28T20:46:24Z

Stack for RFC 005 — 3 of 4. Depends on #1098.

Targets main because cross-fork PRs cannot chain bases, so the diff includes #1097 and #1098. Review the top commit only: f3d90c2.

1/4 — package split ([RFC 005] 1/4: split openenv.core.harness into a package #1097)
2/4 — foundation types ([RFC 005] 2/4: foundation types for agentic harnesses #1098)
3/4 — this PR: the environment runtime
4/4 — production /harness route + mode wiring

What

Makes the types from #1098 runnable: an environment that owns a harness subprocess, hands it the environment's MCP tools, and turns each step() into one conversational turn.

environment.py — HarnessAction + HarnessEnvironment(MCPEnvironment). reset() stops any live harness, enumerates and conflict-resolves env tools, starts the bridge, injects, then starts the harness (injection strictly before start, per the RFC). step() runs one turn; MCP actions keep their normal routing. Rubrics run after the turn, outside the harness's control loop — RFC 004's reward boundary.
process.py — HarnessProcess: readiness-gated start, stderr-tail diagnostics, idempotent stop with SIGTERM → SIGKILL escalation over the process group.
bridge.py — HarnessMCPBridge: serves the env's FastMCP tool surface over loopback HTTP for the harness to consume.

Three decisions I would like challenged

1. HarnessEnvironment subclasses MCPEnvironment, substituting an empty internal FastMCP when mcp=None.
The RFC writes mcp: Optional[FastMCP] = None with a conditional super().__init__(mcp), which cannot work: mcp_server is a required positional, and skipping super().__init__ would skip Environment.__init__ entirely (no rubric, no transform). Substituting an empty server keeps reserved-name validation, _async_handle_list_tools() for injection, and mcp_session() integration for free. Alternative considered: subclass Environment directly and hold a FastMCP — rejected, it means reimplementing all of the above.

2. subprocess.Popen + reader threads, not asyncio.create_subprocess_exec.
Asyncio subprocess transports are bound to their creating loop. This object has to survive three regimes: the server's long-lived loop, the sync facade (run_async_safely spins a fresh loop per call, so a subprocess created in reset()'s loop would be unusable in step()'s), and close() from an executor thread. Popen plus asyncio.to_thread is loop-agnostic. Precedent: envs/julia_env/server/julia_process_pool.py.

3. The bridge is a separate loopback server, not a reuse of the env server's /mcp endpoint.
Reusing /mcp fails on two counts: it lives on the same app as /reset, /step, /state, so pointing the harness at that origin hands the agent the orchestration API — a direct violation of the RFC's security boundary and RFC 001's agents cannot reset; and WS /mcp calls _create_session(), so the harness would talk to a different env instance than the one wrapping it. A separate tool-only server on 127.0.0.1 makes the boundary structural rather than filter-based.

Failure handling

Turn timeouts and harness crashes become terminal observations (done=True, metadata.error_type of turn_timeout / harness_crashed) rather than exceptions, so a training loop scores the episode and moves on instead of unwinding. Deliberate; happy to change if reviewers prefer raising.

Known limitation

HTTP POST /reset and /step create a fresh env per request and close it, which would mean a harness subprocess per HTTP call. Harness envs must be driven over the WebSocket session path. Documented in the class docstring; a route-level guard is a candidate follow-up.

Verification

76 passed

Real subprocesses (no mocks) for the lifecycle edge cases: startup timeout, immediate exit reporting code + stderr tail, crash mid-session, double-stop idempotency, and a child that ignores SIGTERM to prove kill escalation lands within the grace window. Real loopback HTTP for the bridge, connecting an actual fastmcp.Client to list and call a tool. Environment tests cover reset ordering, conflict resolution, rubric ordering, crash/timeout paths, sync-facade parity, and that overrides_method sees the async overrides (which is what makes the server pick the async path). Lint clean.

Subprocess behaviors live in tests/core/scripted_harness.py — a real, lintable module spawned by mode (echo, slow-start, exit-now, crash-after-echo, ignore-sigterm), rather than code embedded in string literals.

Note

Medium Risk
New core environment path spawns subprocesses and exposes env tools over loopback HTTP; failure handling and cleanup are heavily tested but this touches process lifecycle and tool-injection boundaries.

Overview
Adds the RFC 005 runtime so external agentic harnesses (OpenClaw, Claude Code, etc.) can run inside an OpenEnv container with one step() per conversational turn.

HarnessEnvironment (MCPEnvironment) lazy-starts on reset(): stop prior harness/bridge, conflict-resolve env MCP tools (env_ prefix), optionally start a loopback HarnessMCPBridge (tool-only HTTP on 127.0.0.1, with renamed-tool views via build_bridge_server), inject tools into the adapter, then start the harness. HarnessAction drives turns; crashes/timeouts become terminal observations (error_type: turn_timeout / harness_crashed) instead of bubbling exceptions. Rubrics run after each turn; MCP ListTools/CallTool routing is unchanged.

HarnessProcess manages long-lived CLI harnesses with Popen, reader threads, readiness checks, and SIGTERM→SIGKILL shutdown—chosen so sync run_async_safely and the HTTP server can share the same helper across event loops.

openenv.core.harness re-exports the turn-based API next to the existing trainer rollout API; a back-compat test asserts AgenticHarnessAdapter ≠ rollout HarnessAdapter.

Broad integration tests cover real subprocess lifecycles, loopback MCP clients, reset ordering, cancellation cleanup, and mode-specific tools excluded from injection.

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

Moves the trainer-side rollout API out of the package __init__ and into `openenv.core.harness.rollout`, leaving __init__ as a re-export shim. No behavior change: every name previously importable from `openenv.core.harness` still is, and is the same object. The module was ~730 lines living directly in __init__ with a docstring noting it sat outside the stable surface "while RFC 005 is still under review". Splitting it now makes room for the RFC 005 turn-based agentic harness layer to land in sibling modules instead of growing the __init__ further. Also re-exports the private `_resolve_env_reward`, which tests/scripts/test_browsergym_harness_eval_examples.py imports from the package root, and points `collect.py` at `.rollout` directly rather than importing from its own package. Consumers left untouched and verified: `openenv collect`, pi_env, opencode_env, browsergym_env, reasoning_gym_env, openspiel_env. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Adds the type layer for wrapping an external agentic harness (Claude Code, OpenClaw, Codex) as an OpenEnv environment. No runtime behavior yet — this PR is types plus their unit tests. - `config.py`: `HarnessConfig` / `HarnessTransport`. `session_timeout_s` is documented as bounding ONE conversational turn, per the RFC's temporal-semantics section (the field comment in the RFC is ambiguous; flagging for reviewer sign-off). - `events.py`: `HarnessEventType` / `HarnessEvent` / `HarnessResponse`, plus `events_to_metadata()`, the sanctioned JSON-safe path for putting events into `Observation.metadata` so they survive wire serialization. - `adapter.py`: `AgenticHarnessAdapter` ABC and its error hierarchy. - `tools.py`: `resolve_tool_conflicts()` for the RFC's tool-name collision rules (`env_` prefixing, error on ambiguity). Two deliberate deviations from the RFC text, both because the RFC is stale against the code: 1. The RFC's `ToolDefinition` does not exist; the type is `Tool` (`env_server/mcp_types.py`), reused here rather than duplicated. Same for `RESERVED_TOOL_NAMES`, which `resolve_tool_conflicts` re-checks as defense in depth. 2. `send_message()` is concrete rather than abstract. Streaming is the single abstract turn primitive and `send_message()` drains it, which removes duplication from every concrete adapter and makes the terminal TURN_COMPLETE event an enforced contract instead of a convention. The ABC is named `AgenticHarnessAdapter` to avoid colliding with the rollout layer's existing `HarnessAdapter`. Worth discussing whether to rename the rollout classes instead and reclaim the RFC's plain names -- see the PR description. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…idge Makes the RFC 005 types runnable: an environment that owns a harness subprocess, hands it the environment's MCP tools, and turns each step() into one conversational turn. - `environment.py`: `HarnessAction` + `HarnessEnvironment(MCPEnvironment)`. reset() stops any live harness, enumerates and conflict-resolves the env tools, starts the bridge, injects, then starts the harness -- injection strictly before start, per the RFC. step() runs one turn; MCP actions keep their normal routing. Rubrics run after the turn completes, outside the harness's control loop, preserving RFC 004's reward boundary. - `process.py`: `HarnessProcess`, a loop-agnostic Popen + reader-thread helper (readiness gating, stderr-tail diagnostics, idempotent stop with SIGTERM -> SIGKILL escalation over the process group). - `bridge.py`: `HarnessMCPBridge`, serving the env's FastMCP tool surface over loopback HTTP for the harness to consume. Three decisions worth reviewer attention: 1. `HarnessEnvironment` subclasses `MCPEnvironment` and substitutes an empty internal FastMCP when `mcp=None`. `MCPEnvironment` requires `mcp_server` positionally, so the RFC's optional-mcp constructor cannot be written literally; this keeps reserved-name validation, tool enumeration and mcp_session() integration for free. 2. Popen + threads rather than asyncio subprocess transports, because the same instance must work across event loops: the sync facade spins a fresh loop per call (run_async_safely) while the server keeps one long-lived loop. Asyncio subprocess transports are bound to their creating loop. 3. The bridge is a separate loopback server rather than a reuse of the env server's /mcp endpoint. Reusing /mcp would put the orchestration routes (/reset, /step, /state) on the same origin the harness can reach, violating the RFC's security boundary; it would also hand the harness a *different* env instance, since WS /mcp creates its own session. Keeping it separate makes the boundary structural rather than filter-based. Turn timeouts and harness crashes become terminal observations (done=True, metadata.error_type) rather than exceptions, so a training loop scores the episode and moves on instead of unwinding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

cursor

Stale Bugbot comment from a previous run.

Four findings from the automated review, all confirmed against the code before fixing. 1. Renamed tools were unreachable (High, reported on huggingface#1100). Conflict resolution renames a colliding env tool before injection (read_file -> env_read_file), but the bridge served the source FastMCP unchanged, so the harness was handed a name that did not resolve. Adds `build_bridge_server()`, which serves a renamed view built with FastMCP's own `Tool.from_tool(tool, name=...)`, and returns the source server untouched when there is nothing to rename. The new test fails against the old code with `['add', 'read_file'] != ['add', 'env_read_file']`, which is the bug exactly. 2. Reset skipped adapter cleanup (Medium). `reset_async` only stopped the adapter when `is_alive()` was true, but a harness that died on its own reports False while still holding an unwaited process, open pipes and live reader threads; the next `start()` then overwrote that state and leaked it. `stop()` is contractually idempotent, so it is now called unconditionally, and also on the `start()` failure path. 3. Subprocess I/O lacked an explicit encoding (Medium). `text=True` alone decodes with the locale encoding, which is frequently ASCII in a container, while harness output is routinely not. Worse, `UnicodeDecodeError` is a `ValueError`, which the reader thread caught and exited on -- so one non-ASCII byte silently stopped stdout pumping and the turn hung until its timeout. Now `encoding="utf-8"` with `errors="replace"`, and the reader's handler is narrowed to the pipe-closed case it was meant for. 4. Timeouts were reported as crashes (Low). `HarnessTurnTimeoutError` subclasses `HarnessError`, so an adapter raising the dedicated timeout exception was labelled `harness_crashed`. It is now caught first and mapped to `turn_timeout`. Also from finding 1's root cause: mode-specific tools registered with `tool(mode=...)` live on the environment, not the FastMCP server, so the bridge cannot serve them either. They are now excluded from injection with a warning rather than advertised and then failing on call. Supporting them needs a decision about what a mode means inside a harness turn, which is left as a follow-up. Note this only ever manifested when the env's `_mode` matched the tool's -- the test sets it explicitly, since otherwise the assertion would pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Two findings from the automated review on huggingface#1100. The third (renamed tools unreachable through the bridge) was the same root cause as a finding on huggingface#1099 and is fixed there. 1. Production turns ignored session_timeout_s (Medium). The /harness handler streamed `send_message_streaming` with no bound, while simulation mode wraps the same call in `asyncio.wait_for` inside `HarnessEnvironment._run_turn`. A hung harness therefore held its session open indefinitely, and since HarnessEnvironment is SUPPORTS_CONCURRENT_SESSIONS=False with the idle reaper off by default, the server stayed pinned at capacity. The turn is now bounded by the adapter's `session_timeout_s`, matching simulation semantics. 2. A stream that ended without TURN_COMPLETE hung the client (Medium). `send_message()` raises HarnessError in that case, but the socket loop silently went back to waiting for the next client frame, so a client blocking on the terminal event waited forever. The handler now detects it and emits a terminal ERROR event before ending the session. Both paths end the session rather than continuing, so a reconnect gets a fresh harness -- consistent with how a mid-stream crash was already handled. Adds `send_harness_error()` since three paths now emit the same terminal ERROR frame. Both new tests assert `server.active_sessions == 0` afterwards, which is the property finding 1 was really about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

caiotheodoro

src/openenv/core/harness/process.py:221 — bug: read_line(timeout_s=None) doesn't do what the docstring promises ("None blocks until a line or EOF"). The reader thread's for line in stream: sink(line) (line 253) just returns when the pipe closes — no sentinel goes into the queue — so self._stdout_queue.get(timeout=None) (line 236) blocks forever past real EOF.

Repro: start a process that prints one line and exits, drain that line, then call read_line(timeout_s=None) again.

first read_line -> hello
is_running() -> False
calling read_line(timeout_s=None) on an exited process...
BUG CONFIRMED: read_line(timeout_s=None) hung for 5s past EOF instead of returning None
STILL RUNNING AFTER 12s

Wrapping the call in asyncio.wait_for lets the caller move on, but the to_thread worker stays parked in queue.get() — it isn't one of the daemon _reader_threads that stop() joins, so it outlives stop() and (being a non-daemon default-executor thread) blocks the interpreter from exiting.

Nothing in this PR calls read_line with timeout_s=None yet, but it's public API on a class future harness adapters will drive, and the contract as documented is false. Have the reader push a sentinel (or close the queue) on stream EOF and have read_line return None for it instead of relying on a bare queue.get().

bot-ci-comment · 2026-09-17T09:12:51Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

cursor

Stale Bugbot comment from a previous run.

…nvironment-runtime

cursor

Stale Bugbot comment from a previous run.

splusq · 2026-09-17T17:30:41Z

@caiotheodoro, your EOF review is addressed by 1a6caa57, which is already included in this PR.

The stdout reader now enqueues an EOF sentinel from its finally block. read_line(timeout_s=None) therefore wakes and returns None once buffered output is drained, instead of leaving the executor worker blocked in queue.get(). The sentinel is reinserted so subsequent or concurrent readers also see EOF. Each process run owns a separate queue, so restarting does not mix old EOF notifications with new output.

Real-subprocess regression tests cover:

Draining buffered lines, then repeatedly reading EOF with both timeout_s=None and a finite timeout.
Stdout closing while the subprocess remains alive.
stop() waking a pending unbounded read, followed by a restart that reads fresh output.

These cases passed in the latest 79-test harness run. The full repository test run also passed: 2,552 passed, 120 skipped.

splusq · 2026-09-23T18:44:46Z

@caiotheodoro can you please review again?

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.}

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

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

splusq and others added 3 commits August 28, 2026 13:41

This was referenced Aug 28, 2026

[RFC 005] 4/4: production /harness WebSocket route and mode wiring #1100

Open

[diff view] RFC 005 3/4: environment runtime splusq/OpenEnv#2

Closed

splusq added 3 commits August 31, 2026 14:22

Merge branch 'main' into rfc-005/pr1-harness-package-split

f39c662

Merge branch 'main' into rfc-005/pr2-harness-foundation-types

b6e6984

Merge branch 'main' into rfc-005/pr3-harness-environment-runtime

025191d

splusq marked this pull request as ready for review August 31, 2026 21:23

cursor Bot reviewed Aug 31, 2026

View reviewed changes

Comment thread src/openenv/core/harness/environment.py

Comment thread src/openenv/core/harness/process.py

Comment thread src/openenv/core/harness/environment.py Outdated

Comment thread src/openenv/core/harness/environment.py

splusq and others added 3 commits September 2, 2026 09:55

Merge branch 'main' into rfc-005/pr1-harness-package-split

671f5c0

Merge branch 'main' into rfc-005/pr2-harness-foundation-types

933e113

caiotheodoro reviewed Sep 9, 2026

View reviewed changes

burtenshaw added 7 commits September 17, 2026 11:05

fix: refresh harness package split

7cda7a3

fix: refresh harness foundation types

c9e4316

fix: refresh harness runtime

cf760ba

fix: wake harness readers on eof

1a6caa5

fix: refresh latest core state changes

01b09e5

fix: refresh foundation base

5c11106

fix: refresh runtime base

8736543

burtenshaw added 3 commits September 17, 2026 11:12

chore: refresh latest main

a81f0a2

chore: refresh foundation base

4de0d41

chore: refresh runtime base

7401fe0

cursor Bot reviewed Sep 17, 2026

View reviewed changes

Comment thread src/openenv/core/harness/environment.py Outdated

Comment thread src/openenv/core/harness/process.py Outdated

splusq added 2 commits September 17, 2026 17:19

Merge remote-tracking branch 'origin/main' into rfc-005/pr3-harness-e…

2e622ad

…nvironment-runtime

fix(harness): apply transforms and clean up exited processes

4ad1fb2

cursor Bot reviewed Sep 17, 2026

View reviewed changes

Comment thread src/openenv/core/harness/environment.py

splusq and others added 2 commits September 17, 2026 17:37

fix(harness): clean up cancelled reset and startup

e317462

Merge branch 'main' into rfc-005/pr3-harness-environment-runtime

26a5043

splusq requested a review from caiotheodoro September 24, 2026 16:53

splusq added 2 commits September 24, 2026 09:53

Merge branch 'main' into rfc-005/pr3-harness-environment-runtime

a2bf6f6

Merge main into RFC 005 harness runtime; resolve package split conflicts

7225191

cursor Bot reviewed Sep 25, 2026

View reviewed changes

Comment thread src/openenv/core/harness/environment.py Outdated

fix(harness): clean up cancelled conversational turns

0cc7d91

Navigation Menu

[RFC 005] 3/4: HarnessEnvironment, subprocess helper, and MCP tool bridge - #1099

[RFC 005] 3/4: HarnessEnvironment, subprocess helper, and MCP tool bridge#1099
splusq wants to merge 26 commits into
huggingface:mainfrom
splusq:rfc-005/pr3-harness-environment-runtime

splusq commented Aug 28, 2026 •

edited by cursor Bot

Loading

Uh oh!

cursor Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caiotheodoro left a comment

Uh oh!

bot-ci-comment Bot commented Sep 17, 2026

Uh oh!

cursor Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

splusq commented Sep 17, 2026

Uh oh!

splusq commented Sep 23, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

splusq commented Aug 28, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Three decisions I would like challenged

Failure handling

Known limitation

Verification

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caiotheodoro left a comment

Choose a reason for hiding this comment

Uh oh!

bot-ci-comment Bot commented Sep 17, 2026

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

splusq commented Sep 17, 2026

Uh oh!

splusq commented Sep 23, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

splusq commented Aug 28, 2026 •

edited by cursor Bot

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading