[RFC 005] 4/4: production /harness WebSocket route and mode wiring - #1100
Open
splusq wants to merge 9 commits into
Open
[RFC 005] 4/4: production /harness WebSocket route and mode wiring#1100splusq wants to merge 9 commits into
splusq wants to merge 9 commits into
Conversation
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>
Exposes a harness environment directly to clients in production mode, and
gives deployments a way to actually select that mode.
`/harness` is registered only when mode is PRODUCTION *and* the env
factory produces a `HarnessEnvironment`. Connecting opens a session and
resets the env (starting the harness and injecting tools); each
`{"type": "message", "content": ...}` frame runs one turn, streamed back
as HarnessEvent frames terminated by turn_complete. Malformed frames get
a WSErrorResponse without dropping the connection; an adapter crash
streams a terminal error event and ends the session, since harness state
after a crash is undefined.
The handler is modelled on the existing WS /mcp handler rather than the
RFC's pseudocode: it goes through `_create_session()` so capacity limits,
the AsyncExitStack, per-session executors and the idle reaper all apply.
The RFC sketch calls the factory directly and would bypass all of it.
Session activity is touched per streamed event so a long turn is not
reaped as idle.
Mode wiring: `create_app` / `create_fastapi_app` (and the web-interface
factory) take a keyword-only `mode`, resolved from `OPENENV_MODE` when
omitted. Previously `create_fastapi_app` hardcoded `register_routes(app)`,
so nothing outside tests could ever select production. Default is
unchanged (simulation), mirroring the existing `OPENENV_CLIENT_MODE`
convention on the client side.
Two notes for review:
- Harness-env detection uses a lazy import of `HarnessEnvironment` inside
the method: `openenv.core.harness` imports env_server modules, so a
top-level import would be circular. Detection may instantiate the
factory once to probe it, which is safe because constructing a
HarnessEnvironment starts nothing -- asserted by a test.
- `websocket.close()` on an already-gone client raises RuntimeError under
TestClient but WebSocketDisconnect under a real ASGI server; both are
now caught. Found by the end-to-end test, which drives a real uvicorn
server. The pre-existing /mcp and /ws handlers have the same latent gap
and are deliberately left untouched here.
Includes the end-to-end test for the whole stack: a real subprocess
harness that reads the injected MCP config, calls an env tool over the
live bridge, and streams turns -- exercised through both the simulation
step() API and a real uvicorn server's /harness socket.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
splusq
marked this pull request as ready for review
August 31, 2026 21:23
![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.
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 58dd957. Configure here.
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>
…r4-harness-production-route
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack for RFC 005 — 4 of 4. Depends on #1099.
Where to start reviewing
Start with
test_harness_websocket_over_real_server. It is the shortest complete description of the flow this PR adds — a real uvicorn server, a real WebSocket client, and a real harness subprocess, with no mocks between them. Reading it top to bottom is the production request lifecycle:httpx.get("/health")/httpx.post("/reset")ws_connect(".../harness")→session_started_create_session()+reset_async()starting the harness and injecting toolssend({"type": "message", ...})→ framesHarnessEventsframes == [tool_call, tool_result, turn_complete]"sum=5" in ...["response"]send(...)→done is TrueSupporting cast, in the order it becomes relevant:
live_server— stands up the actual server on an ephemeral port; note it usescreate_fastapi_app(..., mode="production"), i.e. the wiring this PR adds.ScriptedCLIAdapter— what plays the harness. It reads the MCP config thatinject_toolswrote, connects to the bridge with a realfastmcp.Client, and maps JSON lines toHarnessEvents. Doubles as a reference for the concrete adapters coming in RFC PR 4.test_multi_turn_episode_through_real_stack— the simulation counterpart, same stack viareset()/step(), and where tool-conflict resolution (read_file→env_read_file) and rubric ordering are asserted.Then the implementation it drives:
_register_harness_routeand its gate atregister_routes.What
Exposes a harness environment directly to clients in production mode, and gives deployments a way to actually select that mode.
/harnessis registered only when mode isPRODUCTIONand the env factory produces aHarnessEnvironment. Connecting opens a session and resets the env (starting the harness, injecting tools); each{"type": "message", "content": ...}frame runs one turn, streamed back asHarnessEventframes terminated byturn_complete.Modelled on
WS /mcp, not on the RFC pseudocodeThe RFC sketch calls
self._env_factory()directly. This handler goes through_create_session()instead, so capacity limits, theAsyncExitStack, per-session executors and the idle reaper all apply — the RFC version would bypass all of it. Session activity is also touched per streamed event, so a long turn is not reaped as idle mid-stream.Error handling: malformed frames get a
WSErrorResponseand the connection stays usable; an adapter crash streams a terminalerrorevent and ends the session, since harness state after a crash is undefined and a reconnect gets a fresh one.Mode wiring
create_app/create_fastapi_app/ the web-interface factory take a keyword-onlymode, resolved fromOPENENV_MODEwhen omitted. Previouslycreate_fastapi_apphardcodedserver.register_routes(app), so nothing outside tests could ever select production — the only in-repo caller passingPRODUCTIONwas a test. Default is unchanged (simulation), and the env-var name mirrors the existingOPENENV_CLIENT_MODEconvention on the client side.Notes
openenv.core.harnessimportsenv_servermodules, so a top-level import back would be circular. Detection may instantiate the factory once to probe it, which is safe because constructing aHarnessEnvironmentstarts nothing — asserted by a test.Verification
Route tests cover the gate in both directions (absent in simulation, absent in production for a non-harness env), that
/reset///step///statestay absent while/healthworks, frame ordering, two turns reusing one adapter, malformed-frame recovery, mid-stream crash, disconnect cleanup withactive_sessions == 0, and capacity rejection. All three mode entry points are exercised: enum, string, andOPENENV_MODE.test_agentic_harness_e2e.pyis the one worth reading. Nothing on the critical path is faked: a real subprocess harness reads the injected MCP config, connects to the live bridge with a real MCP client, calls an env tool, and streams turns back — driven both through the simulationreset()/step()API and through a real uvicorn server's/harnesssocket with a real WebSocket client.Out of scope (follow-ups)
Concrete adapters for OpenClaw / Claude Code (RFC PR 4), an example env and user docs (RFC PR 5), a
HarnessEnvClient(streaming N frames per message does not fitEnvClient's one-request-one-response transport), and a guard rejecting harness envs on the per-request HTTP/reset///steppath.Also worth a small doc PR: the RFC text is stale in several places this stack had to work around (
ToolDefinition→Tool,env_factory=→env=,_mcp_server→mcp_server,run_until_complete→run_async_safely).Note
High Risk
Introduces a production WebSocket API, subprocess/bridge lifecycle, and mode gating that changes which orchestration routes are exposed—security- and capacity-sensitive server behavior.
Overview
Adds the RFC 005 production client surface and the plumbing to turn it on: when
ServerMode.PRODUCTIONand the env factory yields aHarnessEnvironment, the server registers/harness— a WebSocket where connect creates a session,reset_async()starts the harness and injects tools, and each{"type":"message","content":...}frame streams one turn asHarnessEventJSON ending inturn_complete, with turn wall-clock limits and session cleanup on crash or disconnect.Mode selection is no longer hardcoded to simulation:
create_app,create_fastapi_app, and the web-interface factory accept a keyword-onlymode(default fromOPENENV_MODE, else simulation) and pass it intoregister_routes.The harness package is split for the two APIs: trainer rollout helpers move to
openenv.core.harness.rollout(re-exported from the package root for back-compat); new modules cover turn-based harnesses (HarnessEnvironment,AgenticHarnessAdapter, loopbackHarnessMCPBridge,HarnessProcess, events/config/tool conflict resolution).collectimports rollout from the new module path.Reviewed by Cursor Bugbot for commit dc3cf59. Bugbot is set up for automated code reviews on this repo. Configure here.