Add harness session runtime and BrowserGym adapter for training and evaluation

[burtenshaw]

Summary

• add a new harness-facing runtime in openenv.core with ResourceSession, ResourceSessionFactory, StepEnvSessionAdapter, SessionMCPBridge, and white-box/black-box harness adapters
• add a BrowserGym session factory and action translation helpers so BrowserGym can be driven through MCP-style tools instead of raw reset()/step() loops
• add a TRL-oriented BrowserGym harness example and document the new session-based rollout pattern in core and tutorial docs
• export the new harness APIs from the core and BrowserGym packages
• add unit and integration tests covering session lifecycle, MCP bridge behavior, rollout execution, and BrowserGym parity

Testing

• uv run pytest tests/core/test_harness_runtime.py tests/envs/test_browsergym_harness.py -q
• uv run ruff check src/openenv/core/harness.py envs/browsergym_env/harness.py tests/core/test_harness_runtime.py tests/envs/test_browsergym_harness.py tutorial/examples/browsergym_harness.py

[greptile-apps]

Greptile Summary

This PR introduces a session-level coordination layer (ResourceSession, ResourceSessionFactory, StepEnvSessionAdapter, SessionMCPBridge, MCPHarnessAdapter, CLIHarnessAdapter, build_harness_rollout_func) into openenv.core, and a corresponding BrowserGymSessionFactory + action-translation helpers in envs/browsergym_env. Together these allow TRL-style trainers to drive BrowserGym (and any step/reset env) through a unified MCP-style tool-calling surface while keeping token sampling and logprob collection inside the trainer.

Alignment Review Report

Automated Checks

• Lint: Not executed in this review — no bash execution available. Ruff targets are listed in the PR description.
• Debug code: None found.

Tier 1: Fixes Required

• src/openenv/core/harness.py:366 — SessionMCPBridge.handle_request propagates uncaught exceptions from session.call_tool() (e.g. KeyError for unknown tool name) instead of returning a JSON-RPC error object. Callers of CLIHarnessAdapter that pass an unrecognised tool name will crash the process.
• tutorial/examples/browsergym_harness.py:75 — build_browsergym_action_tool_call raises ValueError on any model output that doesn't match a BrowserGym action pattern; this propagates through the rollout loop and silently aborts the rest of the batch. Needs error handling or clear contract documentation.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: New first-class abstractions in openenv.core without an RFC

• Principle at stake: "Key decisions documented in RFCs should not be changed without a new RFC" (PRINCIPLES.md)
• The concern: harness.py introduces five new public abstractions (session layer, harness adapters, rollout builder) into openenv.core. None of the existing RFCs (001–003) discuss or ratify this intermediate session layer or its interaction with the existing Gym-style/MCP boundary model.
• Suggested reviewer: @darktex

ALIGNMENT FLAG: ResourceSession.verify() can compute rewards outside environment boundary

• Principle at stake: "Rewards inside environment" invariant (INVARIANTS.md §3, RFC 002)
• The concern: verify() returns a VerifyResult.reward that build_harness_rollout_func feeds directly to the TRL trainer. The default implementations faithfully forward the env's reward, but the abstract interface places no constraint on this — a custom verify_builder can synthesize arbitrary rewards in the orchestration layer without a Transform, violating the invariant that all reward computation stays inside the environment boundary.
• Suggested reviewer: @darktex

Summary

• 2 mechanical issues to fix (P1 exception propagation in bridge, unhandled ValueError in tutorial example)
• 1 P2 regex correctness issue (_FILL_RE mis-parses fill text containing embedded quotes)
• 2 alignment points for human review (core changes without RFC, reward boundary concern)

Confidence Score: 4/5

Not safe to merge as-is — the SessionMCPBridge exception-propagation bug can crash CLIHarnessAdapter callers at runtime, and the tutorial example can silently abort rollout batches on unexpected model output.

Score 4 because two P1 defects remain: uncaught exceptions in SessionMCPBridge.handle_request crash the bridge instead of returning JSON-RPC errors, and ValueError propagation in the tutorial model_step builder aborts the entire batch loop. Both are live runtime failure paths on the changed code. The two Tier 2 alignment flags are P2 design concerns that warrant human discussion but do not block merge on their own.

src/openenv/core/harness.py (exception handling in SessionMCPBridge) and tutorial/examples/browsergym_harness.py (ValueError propagation in model_step builder)

Important Files Changed

Filename	Overview
src/openenv/core/harness.py	New harness/session runtime layer. Two issues: SessionMCPBridge.handle_request does not catch exceptions from call_tool; ResourceSession.verify() abstract interface allows reward computation outside the environment boundary.
envs/browsergym_env/harness.py	BrowserGym session factory and action translation helpers. The _FILL_RE regex doesn't enforce matching quote delimiters and mis-parses text values containing the same quote character.
tutorial/examples/browsergym_harness.py	TRL rollout example. build_browsergym_action_tool_call raises ValueError on unrecognised model output, propagating through the rollout loop and aborting the full batch.
tests/core/test_harness_runtime.py	Good coverage of session lifecycle, MCP bridge happy-path, white-box harness, and rollout ordering. Missing test for bridge exception path (unknown tool name).
tests/envs/test_browsergym_harness.py	Solid integration tests covering tool exposure, reward parity, action text parsing, and full rollout helper. No issues identified.

Sequence Diagram

sequenceDiagram
    participant TRL as TRL Trainer
    participant RolloutFunc as build_harness_rollout_func
    participant Factory as ResourceSessionFactory
    participant Session as ResourceSession
    participant Harness as MCPHarnessAdapter
    participant Model as model_step (trainer-owned)
    participant Env as Underlying Env (reset/step)

    TRL->>RolloutFunc: rollout_func(prompts, trainer)
    loop per prompt
        RolloutFunc->>Factory: create(task, seed, episode_id)
        Factory->>Env: reset(**kwargs)
        Env-->>Factory: StepResult (initial obs)
        Factory-->>RolloutFunc: ResourceSession
        RolloutFunc->>Harness: run_white_box(model_step, session, limits)
        loop per turn
            Harness->>Model: model_step(messages, tools, sampling)
            Model-->>Harness: ModelStepResult (ids, logprobs, tool_calls)
            loop per tool call
                Harness->>Session: call_tool(name, args)
                Session->>Env: step(action)
                Env-->>Session: StepResult (obs, reward, done)
                Session-->>Harness: ToolResult
            end
        end
        Harness-->>RolloutFunc: HarnessRolloutResult
        RolloutFunc->>Session: verify(transcript, final_state)
        Session-->>RolloutFunc: VerifyResult (reward, metrics)
        RolloutFunc->>Session: close()
    end
    RolloutFunc-->>TRL: {prompt_ids, completion_ids, logprobs, env_reward, verify_metrics}

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/openenv/core/harness.py
Line: 358-378

Comment:
**`SessionMCPBridge` propagates exceptions instead of returning JSON-RPC errors**

`session.call_tool()` raises `KeyError` when an unknown tool name is passed (see `StepEnvSessionAdapter.call_tool`, line 297). `handle_request` does not catch this — or any other exception thrown by `call_tool` — so it propagates out of the bridge as an unhandled exception rather than returning a well-formed JSON-RPC error response.

`CLIHarnessAdapter` exposes this bridge directly to user-supplied `runner` callables. If a runner invokes `bridge.handle_request({"method": "tools/call", "params": {"name": "bad_tool", ...}})`, the process crashes instead of receiving a `{"error": {...}}` response.

```python
        if method == "tools/call":
            if "name" not in params:
                return JsonRpcResponse.error_response(
                    JsonRpcErrorCode.INVALID_PARAMS,
                    message="Missing tool name",
                    request_id=request_id,
                ).model_dump()

            try:
                result = self.session.call_tool(
                    params["name"],
                    dict(params.get("arguments", {})),
                )
            except KeyError as exc:
                return JsonRpcResponse.error_response(
                    JsonRpcErrorCode.METHOD_NOT_FOUND,
                    message=str(exc),
                    request_id=request_id,
                ).model_dump()
            except Exception as exc:
                return JsonRpcResponse.error_response(
                    JsonRpcErrorCode.INTERNAL_ERROR,
                    message=str(exc),
                    request_id=request_id,
                ).model_dump()
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: tutorial/examples/browsergym_harness.py
Line: 63-86

Comment:
**`build_browsergym_action_tool_call` raises `ValueError` on unrecognised model output, crashing the rollout**

If the model emits any text that is not a recognised BrowserGym action string (e.g., a free-text preamble like `"I will click the button: click('13')"`), `build_browsergym_action_tool_call` raises `ValueError`. This propagates through `model_step` → `run_white_box` → `rollout_func` without being caught, aborting the entire rollout and — because the `finally` block in `build_harness_rollout_func` calls `session.close()` but the outer prompt loop does not wrap the call — causing the rest of the batch to be skipped silently.

At minimum the tutorial example should document this contract; ideally, the `model_step` builder wraps the parse call and returns a `noop()` tool call (or surfaces the parse error as a `ToolResult` with `error` set) when parsing fails.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: envs/browsergym_env/harness.py
Line: 211-213

Comment:
**`_FILL_RE` mis-parses `text` values containing the same quote character**

The `text` capture group uses greedy `.*` between two `['"]` delimiters. Because the regex doesn't pin the opening and closing delimiters to the *same* quote character, `fill('42', 'O'Brien')` will match with `text = "O"` rather than raising an error or returning the full string. In the round-trip direction this is unlikely (the action-builder uses `repr()` which would emit `"O'Brien"`) but it is the live failure mode when models emit fill actions with text that literally contains the same quote character.

Consider using a back-reference to enforce matching delimiters:

```python
_FILL_RE = re.compile(
    r"^fill\(\s*(?P<bq>['\"])(?P<bid>.+?)(?P=bq)\s*,\s*(?P<tq>['\"])(?P<text>.*?)(?P=tq)\s*\)$"
)
```

This ties the opening and closing quote per argument and switches `text` to non-greedy, which is the correct semantic for a delimited string.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/openenv/core/harness.py
Line: 106-145

Comment:
**ALIGNMENT FLAG: Architectural addition to `openenv.core` without an RFC**

- **Principle at stake**: "Key decisions documented in RFCs should not be changed without a new RFC" (PRINCIPLES.md §Key Decisions Made).
- **The concern**: This PR introduces five new first-class abstractions into `openenv.core` — `ResourceSession`, `ResourceSessionFactory`, `StepEnvSessionAdapter`, `SessionMCPBridge`, and the harness adapter pair. These define an entirely new session-level coordination contract that sits between the Gym-style orchestration API and the agent-facing MCP surface. None of the existing RFCs (001–003) discuss or approve this intermediate layer. Changes to core that introduce novel architectural primitives typically require an RFC so that the trade-offs (e.g., "one session = one trajectory", how verify interacts with reward invariants) can be reviewed before they land as public API.
- **Suggested reviewer**: `@darktex`

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/openenv/core/harness.py
Line: 532-601

Comment:
**ALIGNMENT FLAG: `verify()` can compute rewards outside the environment boundary**

- **Principle at stake**: "Rewards inside environment" (INVARIANTS.md §3 / RFC 002) — reward computation must stay inside the environment boundary; external reward augmentation uses the Transform pipeline only.
- **The concern**: `ResourceSession.verify()` is an abstract method that returns a `VerifyResult` containing a `.reward` field, and `build_harness_rollout_func` feeds that value directly to the TRL trainer as `rewards.append(float(verify.reward or 0.0))`. The default implementations (`_default_verify`, `_build_browsergym_verify`) faithfully forward the reward already produced by the env, but the abstract interface places no constraint on this — a custom `verify_builder` can synthesize arbitrary rewards in the orchestration layer without going through a Transform. This opens a door that the "rewards inside environment" invariant was explicitly designed to close.
- **Suggested reviewer**: `@darktex`

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Add harness session runtime for training..." | Re-trigger Greptile}

[burtenshaw]

@Darktex This is a working implementation but it's not handcrafted yet. I would first just take a skim of the example and iterate on that : https://github.com/meta-pytorch/OpenEnv/pull/471/changes#diff-4c8c3dcc8f6059214ebf6ecf82ba057ae890f1db709d8bfca57599c4b181e156a

[burtenshaw]

Followed up on the review items on this branch:

• SessionMCPBridge.handle_request(...) now converts unknown tools, invalid params, and unexpected failures into JSON-RPC error responses instead of propagating exceptions.
• the BrowserGym TRL example now extracts a recognized action and falls back to noop() instead of aborting the rollout on free-form model text.
• _FILL_RE now uses matching quote backreferences for both arguments.
• the harness helpers are explicitly marked experimental while RFC 005 is in review.
• rollout reward resolution now validates that verify.env_reward only forwards reward already emitted by the environment.
• added regression coverage for the bridge error mapping and BrowserGym quote parsing in 896c8c0.

I also added the tutorial note tying client_factory back to the environment_factory mental model in f67070d.

[sergiopaniego]

LGTM!

[sergiopaniego]

The rest of the tutorials now have a Open in Colab badge with its corresponding notebook. Could be added here or as a follow-up PR.

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint: PASS (lockfile + source changes; only pre-existing print statements in unrelated modules)
• Debug code: CLEAN in new files

---

Tier 1 — Bugs / Code Quality

• envs/opencode_env/pyproject.toml — Hard-pinned dependency on a contributor's personal fork: git+https://github.com/adithya-s-k/OpenEnv.git@opencode-harness. That branch will be deleted or diverge; this will silently break every Docker build and every user install the moment that branch moves. Replace with either a proper openenv-core>=X.Y.Z pin or a local path = "../../" dependency before merge.

• envs/opencode_env/server/opencode_environment.py — Setup commands race with the agent. The factory calls start_agent() during create(), then _run_rollout_impl issues setup shell commands immediately after. The author notes this "races with the agent for ~1–2s". For slow setup steps (pip install, git clone, large downloads), the agent may have already made its first LLM call before setup completes. The ordering invariant (setup → agent → verify) is violated in the general case. Fix: restructure OpenCodeSessionFactory.create() to accept a pre-start hook, or run setup as part of _bootstrap_sandbox before start_agent() is called.

• src/openenv/core/harness/__init__.py _resolve_env_reward — Reward conflation bug: return verify_reward or 0.0 treats verify_reward = 0.0 as falsy and returns 0.0 anyway, but also treats verify_reward = None as 0.0. When neither the tool trace nor verify() produced a reward, the function silently signals a zero reward to the trainer instead of raising or returning None, making a "no reward" rollout indistinguishable from a "scored zero" rollout. Fix: return verify_reward if verify_reward is not None else 0.0.

• src/openenv/core/harness/collect.py top-level imports — huggingface_hub and rich are imported unconditionally at module level. Anyone who does from openenv.core.harness import MCPHarnessAdapter will pay the cost of these heavy optional imports at import time. Guard with TYPE_CHECKING or move inside the functions that use them (push_to_hf_hub, CollectRunner.run).

• envs/opencode_env/README.md — Example code imports from opencode_env.client import _extract_text. Underscore-prefixed names are private and must not appear in user-facing docs/examples. Use the public OpenCodeEnv.run_rollout() method directly.

• envs/browsergym_env/harness.py — BrowserGymSessionFactory does not subclass ResourceSessionFactory (the ABC from openenv.core.harness). It defines a create() method with the right signature but is not nominally a subclass. isinstance(factory, ResourceSessionFactory) checks will fail and static type-checkers will not catch adapter mismatches.

---

Tier 2 — Alignment / Process

ALIGNMENT FLAG: Diverges from RFC 005 design without documentation

• Principle at stake: RFC 005 (rfcs/005-agentic-harnesses.md) proposes HarnessEnvironment(MCPEnvironment), HarnessAdapter(ABC) with start()/stop()/inject_tools()/send_message(), and HarnessConfig. This PR implements a different abstraction: ResourceSession + ResourceSessionFactory + MCPHarnessAdapter with a tool-calling loop — not the subprocess-lifecycle pattern described in the RFC. These are architecturally incompatible (RFC: harness-owns-LLM-loop; this PR: trainer-owns-LLM-loop with harness as tool server).
• The concern: Merging a large harness API surface that contradicts the approved RFC will cause future contributors to build on the wrong abstraction. The RFC and PR should be reconciled, or the RFC updated to reflect the chosen direction.
• Suggested reviewer: a maintainer who owns RFC 005

ALIGNMENT FLAG: Single PR combines three separate semantic units

• Principle at stake: PR discipline (one meaningful semantic unit per PR).
• The concern: This PR lands (a) the new openenv.core.harness runtime (~700 lines of new core API), (b) a brand-new opencode_env environment with its own uv.lock (~3500 lines including ~3200 lines of lockfile), (c) BrowserGym and OpenSpiel/ReasoningGym harness adapters, and (d) the openenv collect CLI command. These are independent features with independent reviewability surfaces. RFC 005's own implementation plan calls for 5 separate PRs.
• Recommendation: Stack PRs (Graphite or manual) — core runtime first, then per-env adapters.

ALIGNMENT FLAG: SessionMCPBridge exposes a generic JSON-RPC bridge surface

• Principle at stake: INVARIANTS.md — agents cannot access reset/simulation controls; MCP tools must not expose simulation control to agents.
• The concern: SessionMCPBridge.handle_request() dispatches on arbitrary method strings. Currently only tools/list and tools/call are handled; the bridge has no allowlist enforcement — it relies on the absence of handler code. A RESERVED_TOOL_NAMES-style guard should be added so simulation-control method names (reset, step, state) can never be routed even if a handler is accidentally added later.

---

Summary

• 6 mechanical issues to fix (one is a build-breaking dependency pin that must be resolved before this can land)
• 3 alignment points for human review (the RFC divergence is the most significant — it should gate the rest)

Verdict rationale: The core API design diverges from the in-review RFC 005 without documentation, and the build-breaking personal-fork dependency pin makes this not yet mergeable in its current form.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint: CLEAN — no new debug statements in changed files
• Debug code: CLEAN — no breakpoints or debug prints in new files

---

On Prior Feedback (Darktex CHANGES_REQUESTED, 2026-05-07T13:06Z)

The fix commit da5a107d (pushed 17 minutes after the last review) addresses several items from prior rounds:

Resolved:

• _resolve_env_reward or 0.0 bug — fixed; now raises ValueError("rollout did not produce an environment reward") at src/openenv/core/harness/__init__.py:3919.
• SessionMCPBridge exception propagation — fixed; KeyError, ValueError, and bare Exception are all caught and returned as well-formed JSON-RPC error objects (__init__.py:4116–4138). New tests cover all three paths.
• BrowserGymSessionFactory nominal subclassing — fixed; line 851 now reads class BrowserGymSessionFactory(ResourceSessionFactory).
• Heavy optional imports — rich is now imported inside CollectRunner.run() (collect.py:4656) and huggingface_hub inside push_to_hf_hub() (collect.py:4915). No longer loaded on bare import openenv.core.harness.
• _FILL_RE embedded-quote concern — the regex concern raised in earlier rounds turns out to be wrong: Python's backtracking engine resolves fill('42', 'O'Brien') correctly (text="O'Brien"). The test at tests/envs/test_browsergym_harness.py:6673 should pass.

---

Tier 1: Fixes Required

• src/openenv/core/harness/collect.py:4733 — _reset_results_file silently deletes data. When resume=False is passed to CollectRunner.run(), the existing results.jsonl is unlinked with no warning. A user who accidentally passes --no-resume to openenv collect loses all previously collected data with no prompt or backup. This was flagged in Darktex's April 20 review and is still not addressed. At minimum, emit a warnings.warn() or write to a timestamped backup path before deletion.

• src/openenv/cli/commands/collect.py:3107 — noqa: F401 on Tool import claiming re-export, but no __all__. The comment says # noqa: F401 (re-exported for users) but there is no __all__ list in this module that actually exports Tool. This is a documentation/correctness mismatch — either add Tool to an __all__, or drop the import and the suppression.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG: RFC 005 architectural divergence not reconciled

• Principle at stake: "Key decisions documented in RFCs should not be changed without a new RFC" (PRINCIPLES.md §Key Decisions Made).
• The concern: RFC 005 (rfcs/005-agentic-harnesses.md, status: In Review) specifies HarnessEnvironment(MCPEnvironment) where the harness owns the LLM loop (send_message() per turn). This PR ships ResourceSession + MCPHarnessAdapter where the trainer owns the LLM loop (model_step() per turn). These are architecturally incompatible. The fix commit acknowledges this with a docstring comment ("experimental while RFC 005 remains in review") and the README states "experimental", but the API surface is being added to openenv.core — not to an experimental namespace — and will be imported by user code in examples and tutorials. If RFC 005 later lands in its original form, there will be two incompatible HarnessAdapter types in openenv.core (singular) and openenv.core.harnesses (plural) simultaneously. The PR should either (a) update RFC 005 to reflect the chosen design before merging, or (b) land in openenv.experimental with explicit deprecation-when-RFC-lands semantics.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: Single PR still combines multiple independent semantic units

• Principle at stake: PR discipline — one meaningful semantic unit per PR (CLAUDE.md global instructions, RFC 005 implementation plan calls for 5 separate PRs).
• The concern: This PR contains (a) the new openenv.core.harness runtime (~700 lines of new stable-ish core API), (b) BrowserGym harness adapter + eval helpers (~550 lines), (c) OpenSpiel harness adapter (~170 lines), (d) ReasoningGym harness adapter (~125 lines), (e) the openenv collect CLI command (~450 lines), and (f) a new sft-warmup tutorial doc. These have independent reviewability surfaces and independent correctness concerns. The scope concern was raised in Darktex's May 7 review and was not addressed by the fix commit. This is a process flag, not a correctness flag, but it makes thorough review harder.
• Suggested reviewer: @Darktex

---

Summary

The fix commit resolves the most critical mechanical issues from the prior round (reward conflation bug, bridge exception propagation, subclass registration, lazy imports). Two Tier 1 items remain open: the silent data deletion in _reset_results_file, and the misleading noqa comment on the Tool re-export. The two Tier 2 alignment flags (RFC divergence, PR scope) were raised before and are not resolved — they require human judgment about whether to update RFC 005 or redirect this code to an experimental namespace before merge.

This review is on the ~7k-line diff and cannot be fully exhaustive in one pass. The core runtime logic (MCPHarnessAdapter.run_white_box, StepEnvSessionAdapter.call_tool) has good test coverage and the invariant guards (RESERVED_TOOL_NAMES, _resolve_env_reward enforcement) are implemented correctly. The two open Tier 1 items are low-risk bugs; the Tier 2 flags are the substantive gate.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Review

Automated Checks

• Lint: PASS (ruff/usort clean on new files)
• Debug code: CLEAN

Tier 1: Fixes Required

• Unhandled ValueError in CollectRunner.run(): EpisodeRecord.from_rollout() calls _resolve_env_reward(), which raises ValueError on reward disagreement or missing reward. This propagates out of run() uncaught, crashing the entire collect job and losing the run summary. Either catch and count the error, or document that the exception aborts the run.

• sft_warmup.ipynb missing newline at end of file (\ No newline at end of file in the diff). Causes noisy diffs going forward.

• HF_TOKEN silently used as OpenAI key in examples/browsergym_harness_eval_common.py — the api_key or OPENAI_API_KEY or API_KEY or HF_TOKEN chain will produce confusing OpenAI auth errors when only HF_TOKEN is set. Remove the HF_TOKEN fallback or document explicitly.

Tier 2: Alignment Discussion

ALIGNMENT FLAG 1: Relationship to RFC 005 unclear

• Principle at stake: RFC 005 ("Agentic Harness Integration", currently In Review) defines the harness design.
• The concern: This PR introduces a session/tool-loop model (ResourceSession.call_tool(), StepEnvSessionAdapter, MCPHarnessAdapter) that is not explicitly described in RFC 005. The session model is arguably more training-friendly, but the divergence from the RFC's documented direction should be reconciled — either updated in RFC 005 or split into a follow-up RFC.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 2: build_harness_rollout_func accepts session_factory: Any instead of session_factory: ResourceSessionFactory

• Principle at stake: Generic type safety / ABC contract enforcement.
• The concern: ReasoningGymSessionFactory and OpenSpielSessionFactory do not extend ResourceSessionFactory; they duck-type it. The ABC is defined but not enforced consistently, which weakens the runtime contract.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 3: openenv collect CLI uses a hardcoded if/elif dispatch over env families

• Principle at stake: Extensibility / "be hands-on" principle.
• The concern: Adding a third env to collect requires editing CLI source rather than registering a factory. RFC 005's design assumes a clean per-env plugin boundary. Recommend moving to a registry/entry-points discovery, or documenting this as explicitly experimental and limited to the two supported envs.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 4: SessionMCPHttpServer introduces a custom HTTP JSON-RPC transport for MCP

• Principle at stake: INVARIANTS.md §4 — "WebSocket for all environment communication, no custom protocols".
• The concern: The helper is in examples/ only (used for the black-box Codex eval path), so the blast radius is limited. But it establishes an HTTP MCP pattern the team is actively deprecating. Recommend a deprecation notice or moving to a clearly experimental area.
• Suggested reviewer: @Darktex

Summary

3 mechanical issues to address, 4 alignment discussion points. The core implementation quality is strong — the RESERVED_TOOL_NAMES guard, _resolve_env_reward reward integrity check, and test coverage are well done. Primary discussion point is the relationship to RFC 005.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint: PASS on changed files in this diff
• Debug code: CLEAN in new harness files
• Tests: Substantial coverage added across test_harness_runtime.py, test_harness_collect.py, test_openspiel_harness.py, test_openspiel_collect.py, test_browsergym_harness.py, and CLI tests

---

Tier 1: Fixes Required

• src/openenv/core/harness/collect.py — Bare finally swallows rollout exceptions silently

  CollectRunner.run() wraps the full rollout + verify + record block in a bare try/finally. If run_white_box() raises (network error, env crash, reward resolution failure), the exception propagates immediately — the progress display locks up and the caller gets no partial-result summary. The session.close() is correctly in finally, but there should be an except Exception branch that logs the failure, increments a num_failed counter, and continues to the next episode rather than aborting the entire collection run. This is a correctness bug for long-running collection jobs.

• src/openenv/core/harness/__init__.py — Synchronous client.reset() called unconditionally in StepEnvSessionAdapter.__init__

  The constructor eagerly calls self._client.reset(**reset_payload) and assigns self._initial_result. If reset() raises, there is no error path — the exception propagates out of __init__ without closing the client. Add a try/except around the reset() call in __init__, or document that callers must not pass clients that can raise in reset().

• examples/browsergym_harness_eval_common.py — Silent 0.0 reward default in _finalize_episode

  reward = float(verify.env_reward or 0.0)

  This converts None (no reward produced) into 0.0 silently without raising, bypassing the _resolve_env_reward guard used everywhere else in the harness stack. The BrowserGym eval examples should use _resolve_env_reward or at least check for None explicitly to preserve the invariant. The test suite does not cover this code path. This is a code-level invariant violation in the examples.

• envs/browsergym_env/harness.py — Fill regex silently mismatches on mismatched quotes

  The _FILL_RE regex uses backreferences (?P=bq) and (?P=tq) to require the same opening/closing quote, but a test passes fill('42', 'O'Brien') — a string with an embedded single quote, which the regex cannot match correctly since the first ' ends the first argument. The test asserts this passes. Either the test is wrong (and the regex will fail in production) or the regex is unexpectedly lenient. Verify the regex behavior and add a negative test for the ambiguous case.

• src/openenv/core/harness/collect.py — _SUPPORTED_SAMPLING_KEYS silently drops unknown sampling params

  filtered_sampling = {k: v for k, v in sampling.items() if k in _SUPPORTED_SAMPLING_KEYS}

  Unrecognized sampling keys passed by a caller (e.g. top_k, repetition_penalty) are silently dropped. This could cause confusing model behavior. Consider at minimum logging a warning when keys are discarded.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG 1: New openenv.core.harness module introduced while governing RFC is still in review

• Principle at stake: PRINCIPLES.md — "These decisions are documented in RFCs and should not be changed without a new RFC." RFC 005 is cited throughout the new code as its design authority, but its status appears to be in-review, not Accepted.
• The concern: A substantial new public API surface (ResourceSession, ResourceSessionFactory, StepEnvSessionAdapter, SessionMCPBridge, MCPHarnessAdapter, CLIHarnessAdapter, build_harness_rollout_func, CollectRunner, RolloutSerializer, etc.) is being shipped while the governing RFC is still pending approval. The code is marked "experimental," but it is exported from openenv.core.harness (stable-looking import path) and shipped with CLI integration (openenv collect). Pre-1.0 breaking changes are acceptable per INVARIANTS.md, but shipping a CLI command and public imports against an in-review RFC sets a precedent that RFC review is not a gate.

---

ALIGNMENT FLAG 2: StepEnvSessionAdapter.call_tool() exposes step() to agents via a non-orchestration path

• Principle at stake: INVARIANTS.md Security Invariant 1 — "Agents cannot access reset/simulation controls. The WebSocket interface for reset/step is for orchestration only."
• The concern: StepEnvSessionAdapter.call_tool() directly invokes self._client.step(action) on behalf of an agent call (via SessionMCPBridge.handle_request → session.call_tool → client.step). In the white-box case (MCPHarnessAdapter), the caller is the training loop, so the orchestration boundary is respected. In the black-box case (CLIHarnessAdapter), SessionMCPBridge is exposed to an external CLI process (e.g. Codex) via SessionMCPHttpServer, which maps arbitrary HTTP POST → bridge.handle_request() → session.call_tool() → client.step(). The external CLI agent is effectively calling step() on the environment through this path. RFC 005 acknowledges this in its "Harness Security Boundary" section and argues it is acceptable because reset, step, state, and close are in RESERVED_TOOL_NAMES. However, step() is called internally by call_tool() — it is not a tool name. The external CLI never calls a tool named step, but the effect is equivalent: one tool call = one step() call. The question is whether "the harness is the agent" (RFC 005's framing) truly resolves the invariant. This is the central architectural trade-off of RFC 005 and needs explicit sign-off.

---

ALIGNMENT FLAG 3: VerifyResult.env_reward docstring constraint is advisory only — not enforceable at the ResourceSession ABC boundary

• Principle at stake: PRINCIPLES.md / INVARIANTS.md — "Rewards inside environment: Reward computation must stay inside environment boundary."
• The concern: VerifyResult documents that env_reward "must forward reward already produced inside the environment" and "must not synthesize a new reward in the orchestration layer." This is enforced at the EpisodeRecord.from_rollout level via _resolve_env_reward, but the ResourceSession.verify() ABC has no way to mechanically enforce this: any implementation of verify() can return a synthesized reward with no check. In examples/browsergym_harness_eval_common.py, _finalize_episode bypasses _resolve_env_reward entirely. Reward correctness in this architecture is thus a convention, not a structural guarantee. This should be called out as a known limitation in the RFC / docs, and the examples should be fixed to use _resolve_env_reward consistently.

---

ALIGNMENT FLAG 4: SessionMCPBridge and SessionMCPHttpServer introduce a new in-process / HTTP MCP interface not covered by RFC 003 or RFC 005

• Principle at stake: INVARIANTS.md Architectural Invariant 4 — "Communication patterns: WebSocket for all environment communication (Gym-like API + metadata). No custom protocols."
• The concern: SessionMCPHttpServer (in examples/browsergym_harness_eval_common.py) implements a custom HTTP/JSON-RPC server using ThreadingHTTPServer — not the existing FastAPI/WebSocket infrastructure. It is used as the bridge between CLIHarnessAdapter (e.g., Codex) and the session. The existing HTTP uses FastAPI (create_app). This new ad-hoc HTTP server is a third protocol path, bypassing the shared middleware stack. If this pattern needs to be long-lived, it should use the existing FastAPI foundation rather than a bespoke ThreadingHTTPServer.

---

ALIGNMENT FLAG 5: collect.py CollectRunner imports rich.progress lazily inside run() — hard dependency may not be declared

• The concern: rich is imported inside CollectRunner.run() (from rich.progress import ...). If rich is not installed, the first call to run() fails at runtime with an ImportError. Verify it is listed in the pyproject.toml for openenv-core. If not, this will cause silent breakage for users who install with minimal extras.

---

Summary

• 3 correctness bugs to fix before merge:

  1. Bare finally in CollectRunner.run() that aborts long-running collection jobs on any single-episode failure
  2. StepEnvSessionAdapter.__init__ calling reset() without error handling — resource leak on reset failure
  3. _finalize_episode in examples silently substituting 0.0 for None reward, bypassing the "rewards in env" guard

• 2 nits to address:
  4. BrowserGym fill-regex behavior under mismatched-quote inputs
  5. Silent discard of unknown sampling params

• 5 alignment discussion points for human review:

  1. Shipping public API and CLI against an in-review RFC
  2. Whether SessionMCPBridge → call_tool → step() satisfies the "agents cannot reset/step" invariant
  3. Reward boundary convention vs. enforcement — examples bypass _resolve_env_reward
  4. Ad-hoc ThreadingHTTPServer as a new protocol path outside the existing WebSocket/FastAPI stack
  5. rich hard dependency in CollectRunner.run() — packaging completeness

The core abstraction design is well-thought-out and the test coverage is strong. The reward enforcement via _resolve_env_reward and RESERVED_TOOL_NAMES are good structural guards. The alignment questions are genuine open issues that benefit from explicit RFC-level resolution before the harness layer exits experimental status.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review (automated)

Verdict: request_changes

This PR ships substantial, high-quality harness infrastructure with good test coverage. The core MCPHarnessAdapter / ResourceSession / build_harness_rollout_func design is clean, the reward integrity guard (_resolve_env_reward) is well-implemented, and RESERVED_TOOL_NAMES correctly blocks agents from accessing orchestration controls.

However, two blocking concerns and several alignment flags need resolution before merge.

Tier 1: Fixes Required

• examples/browsergym_harness_eval_common.py:2145 -- reward = float(verify.env_reward or 0.0) synthesizes a default 0.0 reward in the eval path, bypassing _resolve_env_reward (which correctly raises ValueError for None). Even in eval-only code this is inconsistent with the training-loop pattern and silently masks task failures. Replace with explicit handling: propagate None to the caller or call _resolve_env_reward here and catch ValueError.

• src/openenv/core/harness/__init__.py + src/openenv/core/harnesses/types.py -- Two incompatible HarnessEvent classes and two incompatible HarnessAdapter ABCs now live under the same openenv.core namespace:

  • openenv.core.harness.HarnessEvent: plain dataclass with type: str, payload: dict
  • openenv.core.harnesses.types.HarnessEvent: Pydantic BaseModel with type: HarnessEventType, timestamp: float, data: dict

  This collision will confuse importers and risks hard-to-debug isinstance failures if the types are mixed. The new package's docstring says it is "experimental while RFC 005 is under review", but RFC 005's foundation types are already merged as openenv.core.harnesses. These need to be consolidated or one must clearly subsume the other.

• src/openenv/core/harness/__init__.py StepEnvSessionAdapter._default_verify -- When state() is not available on the client, _read_state() returns None, the isinstance(state, State) check fails, and step_count silently disappears from verify.metrics. Downstream aggregation (e.g. avg_steps in summarize_episodes) silently returns 0 with no warning.

Tier 2: Alignment Discussion (cc @Darktex)

• Two parallel harness packages in openenv.core -- RFC 005 envisioned a sequential PR sequence on top of openenv.core.harnesses (plural). This PR introduces openenv.core.harness (singular) with a fundamentally different API: ResourceSession / StepEnvSessionAdapter bypass the Environment interface entirely, where RFC 005 placed HarnessEnvironment(Environment) inside the existing Gym-style WebSocket orchestration. Users cannot tell which to use. Needs a resolution call before merging.

• Client-server separation potential bleed -- envs/browsergym_env/harness.py, envs/openspiel_env/harness.py, envs/reasoning_gym_env/harness.py import Tool and State from openenv.core.env_server.*. INVARIANTS.md says "Clients must never import from server/ directory". mcp_types.Tool is effectively a shared wire type but lives under env_server/. Should Tool and State move to a shared types module, or is importing from env_server acceptable for orchestration-layer code?

• "One env = one trajectory" semantics -- build_harness_rollout_func iterates prompts sequentially, calling create() per prompt against a client_factory. This achieves logical isolation but not container isolation, which is what the principle was originally designed for. May be acceptable for training-time use, but deserves explicit documentation.

• SessionMCPHttpServer in examples/browsergym_harness_eval_common.py -- Hand-rolled HTTP/JSON-RPC bridge that re-implements MCP transport. INVARIANTS.md Architectural Invariant #4: "WebSocket for all environment communication. No custom protocols." Although in examples/, it's imported by browsergym_codex_eval.py which ships in the repo. Establishes a third transport (HTTP) alongside MCP-WebSocket and the Gym step API.

---

Generated by alignment-reviewer subagent

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review: PR #471 — Harness Session Runtime and BrowserGym Adapter

This PR implements the RFC 005 session-based harness runtime. RFC 005 exists and is "In Review," which is the correct pre-merge state for a change of this architectural scope. The implementation is largely faithful to the RFC design. The review below covers mechanical issues and the alignment questions humans should settle before merge.

---

Automated Checks

• Lint: FAIL — the lint.sh hook exited with code 2 and produced no output, suggesting the PR's new files are not on the working tree and cannot be linted in-place. The diff does not include a pyproject.toml update or src/openenv/core/harness/__init__.py line that would allow a direct lint run from the repo. Authors should confirm uv run ruff check and uv run usort check pass on the new files before merge.
• Debug code: CLEAN in new files — the check-debug.sh hits on existing files only (auto_env.py, mcp_client.py, etc.), none introduced by this PR.

---

Tier 1: Fixes Required

• src/openenv/core/harness/__init__.py (line ~3905-3919) — _resolve_env_reward raises on a mismatch only when BOTH rewards are present. If verify.env_reward is None and the tool trace also has no reward, the function raises ValueError("rollout did not produce an environment reward"). This is correct. However, if trace_reward is None and verify_reward is a non-None value, the function silently trusts verify_reward without cross-checking the environment. This is the reward-synthesis path the RFC explicitly warns against: a verify() implementation that fabricates a reward from outside the environment would pass without error. The check should also reject a verify_reward when the trace produced no reward at all (or require that only the trace reward is the source of truth).

• src/openenv/core/harness/collect.py (CollectRunner.run) — exception from EpisodeRecord.from_rollout is not caught. The try/finally block (line ~4686-4704) only has finally: session.close(). If EpisodeRecord.from_rollout raises (e.g., the reward mismatch ValueError), the exception propagates and aborts the entire collection run, discarding all future episodes. A failed episode should be counted as num_dropped and logged, not crash the runner. Wrap EpisodeRecord.from_rollout in a try/except that increments num_dropped.

• src/openenv/core/harness/__init__.py (StepEnvSessionAdapter.__init__) — reset() is called in the constructor. If reset() raises (e.g., a transient network error to the env server), the session object is half-initialized and close() may be called on it by the caller's finally block, potentially calling self._client.close() on a client that was never fully started. The _closed flag guards against double-close, but self._client may not have been assigned yet. This is low-severity but worth a comment or a guard.

• envs/browsergym_env/harness.py:build_browsergym_action_tool_call — ToolCall.id values are hardcoded non-unique strings (e.g., "browsergym-click"). In a multi-turn episode where the model issues the same action twice, both tool call IDs are identical. Standard MCP/OpenAI tool-call protocol requires unique IDs per invocation. This will cause issues if a downstream consumer (e.g., an Anthropic client) validates tool_use/tool_result pairing by ID.

• examples/browsergym_harness_eval_common.py:SessionMCPHttpServer — SessionMCPBridge.handle_request is called from the HTTP handler thread without any lock. ResourceSession.call_tool is inherently stateful (it calls client.step()). If two concurrent HTTP requests arrive (possible with ThreadingHTTPServer), both could call call_tool simultaneously, corrupting the session state. Either use ThreadingHTTPServer with a per-session lock around call_tool, or switch to HTTPServer (single-threaded). The bridge is session-scoped, so per-request concurrency is undesired.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG: RFC 005 is "In Review" but this PR ships the full implementation (714-line runtime + 545-line collector + CLI + 3 env adapters + tests).

• Principle at stake: RFC 000/PRINCIPLES.md — "Design/Alignment (human-owned): RFCs, principles, trade-off decisions." The RFC is not marked "Accepted."
• The concern: Merging a production-facing architectural layer (new CLI command, new public API surface, Hub integration) while the RFC is still under review creates a fait accompli. If the RFC review surfaces a design change, rolling it back post-merge is much harder than iterating on a draft. The RFC's implementation plan explicitly staged the work across PRs 2-5; this PR appears to bundle PRs 2-5 together.

---

ALIGNMENT FLAG: SessionMCPBridge exposes the initialize MCP method implicitly via the fall-through METHOD_NOT_FOUND path, but does not handle it.

• Principle at stake: Dual API Boundary (INVARIANTS.md §1) — MCP is the agent-facing API; the bridge must not leak orchestration controls.
• The concern: The MCP protocol requires clients to call initialize before tools/list. The current bridge returns METHOD_NOT_FOUND for initialize, which will cause conformant MCP clients (e.g., Claude Code as a black-box harness) to fail the handshake. Implementing initialize correctly to return server capabilities is safe, but it must not accidentally expose methods that map to reset/step/state.

---

ALIGNMENT FLAG: _resolve_env_reward uses a math.isclose tolerance check to validate that verify.env_reward matches the trace reward, but this check can be bypassed by any verify() implementation that returns env_reward=None.

• Principle at stake: Rewards inside environment (RFC 002, PRINCIPLES.md key decisions).
• The concern: RFC 005 §Harness Security Boundary states "The harness must not be able to invoke reward-related functions" and "The rubric runs after the harness turn completes." The current enforcement is soft: if verify() returns env_reward=None (which _default_verify does when last_result is None), _resolve_env_reward falls through to the verify_reward=None branch and raises — good. But if a custom verify_builder returns env_reward=42.0 without any tool trace, it silently wins. The invariant check is not symmetric.

---

ALIGNMENT FLAG: The collect CLI command hardcodes a registry of env packages (openspiel_env, reasoning_gym_env) inside src/openenv/cli/commands/collect.py.

• Principle at stake: Minimize lifecycle deltas / Be hands-on (PRINCIPLES.md). The pattern for extending OpenEnv is openenv.yaml auto-discovery, not a hardcoded list in CLI code.
• The concern: Every new env that wants openenv collect support requires a code change to collect.py. This is the opposite of the plugin/discovery pattern the rest of OpenEnv uses. The RFC does not address CLI extensibility. A follow-up should define a registration mechanism (entry-point discovery, openenv.yaml extension, etc.).

---

Verdict Rationale

The mechanical quality is high: the core abstractions are clean, RESERVED_TOOL_NAMES is enforced at both the session and bridge layer, reward synthesis is blocked (with the caveat noted above), and the test coverage is substantial. The Tier 1 issues are real bugs (uncaught ValueError aborting collection runs, duplicate tool-call IDs, threading hazard in the HTTP bridge) that should be fixed before merge.

The Tier 2 flags are the more important blockers. Shipping a full multi-module implementation of an RFC that is explicitly "In Review" — including a new public CLI command, a Hub integration, and three env adapters — without the RFC being accepted first bypasses the alignment process this repo has set up for exactly this kind of architectural change. The PR should either wait for RFC 005 acceptance, or be split so that only an experimental/internal module lands now and the public API surface waits for the RFC to close.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Scope

Deep-read: src/openenv/core/harness/__init__.py, collect.py, envs/browsergym_env/harness.py, envs/openspiel_env/harness.py, envs/reasoning_gym_env/harness.py, src/openenv/cli/commands/collect.py, src/openenv/core/llm_client.py. Skimmed: examples, docs, notebooks, test files (spot-checked invariant-critical paths).

Automated Checks

• Lint: SKIP - files only exist on the PR branch, not the current checkout. No lint errors identifiable from diff review.
• Debug code: CLEAN - no print/breakpoint statements in the new core harness files. Print calls in examples are intentional CLI output.

Tier 1: Fixes Required

• envs/openspiel_env/harness.py and envs/reasoning_gym_env/harness.py — OpenSpielSessionFactory and ReasoningGymSessionFactory are plain classes that do not inherit from ResourceSessionFactory (the ABC). BrowserGymSessionFactory does inherit it correctly. The CollectRunner type-hints session_factory: ResourceSessionFactory, so passing an OpenSpielSessionFactory is a type violation that would be caught by a type checker. All three factories should extend ResourceSessionFactory.

• src/openenv/core/harness/collect.py CollectRunner.run() — a ValueError from _resolve_env_reward (e.g., episode with no reward) or from EpisodeRecord.from_rollout propagates uncaught, aborting the entire collection job. For a 200+ episode run this is a significant reliability hazard. At minimum, wrap the inner episode block to catch and log per-episode failures, then increment a num_failed counter and continue, consistent with the num_dropped design.

• envs/browsergym_env/harness.py (_FILL_RE) — uses non-greedy (?P<text>.*?) with a backreference closing quote. Python's backtracking engine handles embedded single-quotes correctly (e.g., fill('42', 'O\u2019Brien') does parse to text=O'Brien), but this is subtle and brittle. The regex and the test test_browsergym_fill_parser_supports_single_quoted_embedded_quote do not document the backtracking dependency. At minimum, add a comment explaining the expected behavior.

• src/openenv/core/harness/collect.py — rich (for Progress) is imported via a deferred inline import inside CollectRunner.run(). This is unconventional given that rich is already a project dependency rather than a conditional optional one. Move the import to the top of the file.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: VerifyResult.env_reward docstring vs. enforcement asymmetry

• Principle at stake: Rewards inside environment (RFC 002).
• The concern: VerifyResult documents that env_reward "must forward reward already produced inside the environment" and _resolve_env_reward enforces this with a mismatch check. However, when the tool trace has no reward in its metadata (e.g., a custom env that puts reward only on the StepResult but whose tool_result_builder does not populate metadata["reward"]), _resolve_env_reward falls back to verify.env_reward alone — no mismatch check applies. This means a poorly-written verify_builder could synthesize an external reward and the invariant check silently passes. The design note in harness/README.md claims the invariant is "respected," but the fallback path skips the check.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: SessionMCPBridge is a public, exported symbol accessible to external callers

• Principle at stake: Agents cannot reset (INVARIANTS.md Security §1); Dual API boundary (INVARIANTS.md Architectural §1).
• The concern: SessionMCPBridge is exported from openenv.core.harness.__all__ and used by CLIHarnessAdapter to hand a full bridge to an external runner callable. The bridge filters out RESERVED_TOOL_NAMES (reset, step, state, close), which is the right defense. However, the bridge gives the caller a live session object too (runner(bridge, session, limits) in CLIHarnessAdapter.run_black_box). An untrusted runner could call session.close() mid-rollout or directly invoke methods on the underlying _client. The concern is whether the "black-box harness" path — intended for opaque CLI agents like Codex — correctly enforces the boundary when the runner callable is not part of the training infrastructure but could be arbitrary user code. This warrants explicit documentation of the trust model, or restricting runner to only receive the bridge (not session).
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: Harness layer translates MCP-style tool calls into Gym-like step() calls — confirm RFC 005 blesses this pattern

• Principle at stake: Dual API boundary — WebSocket for infrastructure (Gym-like), MCP for agents (INVARIANTS.md Architectural §1); Communication patterns §4.
• The concern: StepEnvSessionAdapter.__init__ calls self._client.reset() directly, and call_tool calls self._client.step(). These are the Gym-like orchestration API calls. The harness layer is orchestration infrastructure (training side), so this is architecturally correct per the invariant. However, the same StepEnvSessionAdapter is also exposed via SessionMCPBridge for black-box CLI harnesses, where a Codex/CLI agent's tool calls translate into step() calls. This is intended, but the README labels the harness "experimental while RFC 005 remains in review" — review should confirm RFC 005 explicitly blesses this pattern before the harness layer is promoted out of experimental status.
• Suggested reviewer: @Darktex

Summary

• 4 mechanical issues to fix (2 type/design, 1 reliability, 1 code style)
• 3 alignment points for human review (reward invariant gap, bridge trust model, RFC 005 status gate)

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Scope and Audit Coverage

This PR adds ~7,250 lines across 29 files. I read in full: src/openenv/core/harness/__init__.py, src/openenv/core/harness/collect.py, envs/browsergym_env/harness.py, envs/openspiel_env/harness.py, envs/reasoning_gym_env/harness.py, src/openenv/cli/commands/collect.py, the core/llm_client.py delta, and the four primary new test files (tests/core/test_harness_runtime.py, tests/core/test_harness_collect.py, tests/envs/test_browsergym_harness.py, tests/test_cli/test_collect.py). I sampled the examples/ scripts and the OpenSpiel/ReasoningGym test files. The four tutorial .md files and the SFT-warmup notebook were not audited for correctness — only for surface-level consistency with the API.

I read rfcs/005-agentic-harnesses.md, PRINCIPLES.md, INVARIANTS.md, and PATTERNS.md before forming alignment judgments.

Automated Checks

• Lint: not run end-to-end (large surface, many new files). Imports and naming look ruff-clean on inspection.
• Debug code: no print/breakpoint/pdb in new modules; the only print in the diff is inside examples/ scripts, which is conventional.
• Dependencies: rich>=13.0.0 is already declared in pyproject.toml, so CollectRunner.run's from rich.progress import … does not introduce a new hard dep.

Tier 1: Fixes Required

T1-1 (minor, structural): SessionMCPHttpServer lives in an examples/ file but is integration-tested

examples/browsergym_harness_eval_common.py defines SessionMCPHttpServer, which is imported by examples/browsergym_codex_eval.py and examples/browsergym_harness_eval.py, and exercised by tests/scripts/test_browsergym_harness_eval_examples.py. A class with this much load-bearing role is awkward as an examples/ artifact — examples/ is conventionally non-importable infrastructure. Consider promoting it to src/openenv/core/harness/ (or a sibling bridge/ module) and re-exporting from the example. Not a blocker for correctness.

T1-2 (worth a follow-up): per-episode failures abort CollectRunner.run

CollectRunner.run wraps the per-episode body in a try/finally whose only role is session.close(). A transient error (network blip, browser timeout) in any single episode propagates out and aborts the whole collection job — no num_errored counter, no continuation. For long-horizon collection runs (the very use case the PR targets — see examples/ttt_collect_with_llm.py), this is fragile. Consider catching exceptions at the per-episode boundary, recording an error tally on CollectResult, and continuing. If the design intent is fail-fast, a comment to that effect would help future readers.

I did not check whether _resolve_env_reward mismatch detection covers the trace_reward is None path, or whether the BrowserGym _FILL_RE regex handles nested-quote pathologies — those would deserve targeted unit tests if not already present.

Tier 2: Alignment Discussion

ALIGNMENT QUESTION 1 — Does this PR correspond to the RFC 005 delivery plan?

rfcs/005-agentic-harnesses.md exists on main and lays out a multi-PR delivery plan. This PR introduces a substantial new public surface (ResourceSession, ResourceSessionFactory, StepEnvSessionAdapter, SessionMCPBridge, white-box / black-box adapters, CollectRunner, plus a new openenv collect CLI). The PR body doesn't reference RFC 005 or map this work onto its phases. Two questions for human reviewers:

• Is src/openenv/core/harness/ the canonical module path RFC 005 envisioned, and does this PR supersede or extend the RFC's plan?
• The new openenv collect CLI command is a non-trivial addition not listed in the PR body's bullets; is its design covered by RFC 005 or another RFC?

Suggested reviewer: the RFC-005 author / maintainer.

ALIGNMENT CHECK — "Agents cannot reset" invariant: RESPECTED at the bridge layer

SessionMCPBridge defines RESERVED_TOOL_NAMES = frozenset({"reset", "step", "state", "close"}) and rejects both registration and dispatch of tools matching those names. The MCP-facing surface therefore cannot be used by an agent to reset/step the environment. Good.

ALIGNMENT QUESTION 2 — Network-level harness isolation is not enforced in this PR

RFC 005 §"Harness Security Boundary" calls for the harness subprocess to be unable to reach the orchestration HTTP/WebSocket port directly, not just the MCP bridge. This PR enforces the boundary at the JSON-RPC layer (via RESERVED_TOOL_NAMES), but a harness subprocess that knows the orchestration URL can still call /reset directly. The RFC acknowledges this and describes it as a follow-up; worth confirming that's still the plan and adding a # TODO: network isolation pointer in the bridge so future contributors see the contract.

ALIGNMENT CHECK — "Rewards inside environment" invariant

_resolve_env_reward is the right shape: it asserts that verify.env_reward matches the trace's last emitted reward when both are present. The accompanying test test_from_rollout_uses_resolve_env_reward enforces this. The case where the trace has no reward and verify synthesizes one is worth a docstring clarifying intent (terminal-only envs vs per-step envs); not a blocker.

Final Recommendation

Verdict: comment.

The session runtime is well-tested, the RESERVED_TOOL_NAMES gate respects the "agents cannot reset" invariant, and _resolve_env_reward keeps reward authority inside the environment. Tier 1 findings are minor (structural placement of SessionMCPHttpServer; per-episode error handling in CollectRunner). The substantive question is Tier 2: humans should confirm that this PR is the intended RFC 005 implementation path and that openenv collect is in scope. I'm not blocking on those, but the size of this PR and its new public API surface deserve explicit RFC sign-off before downstream work builds on it.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Scope

• Adds openenv.core.harness — a new session-based runtime layer (ResourceSession, StepEnvSessionAdapter, SessionMCPBridge, MCPHarnessAdapter, CLIHarnessAdapter, build_harness_rollout_func) for driving rollouts via an MCP tool-calling loop while the trainer owns model sampling and logprobs.
• Adds openenv.core.harness.collect — rollout serialization pipeline (CollectRunner, RolloutSerializer, EpisodeRecord, push_to_hf_hub) plus an openenv collect CLI command.
• Adds BrowserGym, OpenSpiel, and ReasoningGym session adapters (envs/*/harness.py) that wrap existing env clients into the new session interface, plus tutorials, examples, and a llm_client.py patch to support max_completion_tokens for newer OpenAI models.

---

Tier 1 — Bugs & Correctness

1. OpenSpielSessionFactory and ReasoningGymSessionFactory do not inherit from ResourceSessionFactory ABC.

envs/openspiel_env/harness.py:1074 declares class OpenSpielSessionFactory: and envs/reasoning_gym_env/harness.py:1194 declares class ReasoningGymSessionFactory: — neither inherits from the ABC. In contrast, envs/browsergym_env/harness.py:851 correctly declares class BrowserGymSessionFactory(ResourceSessionFactory):.

The consequence is twofold: (a) CollectRunner.__init__ at src/openenv/core/harness/collect.py:4614 annotates session_factory: ResourceSessionFactory, so passing an OpenSpielSessionFactory is a type error that mypy/pyright will catch and that defeats the documented interface contract. (b) The ABC's create() abstract method guard is skipped — any future breaking signature change goes undetected.

Fix: add (ResourceSessionFactory) to both class declarations and import the ABC.

2. CollectRunner.run() will surface an unguarded exception from EpisodeRecord.from_rollout as a collection-aborting crash.

src/openenv/core/harness/collect.py lines 4687–4704: the try…finally block wraps run_white_box, session.verify, and EpisodeRecord.from_rollout together. If from_rollout raises (e.g., the reward mismatch ValueError that enforces the "rewards in env" invariant), session.close() fires correctly but the exception propagates out of the for episode_id in planned_ids loop, aborting the entire collection run rather than skipping the episode. For a long-running collect job (hundreds of episodes) this is a reliability issue. Consider wrapping the body in except Exception with a warning log and a skip counter, or at minimum documenting the fail-fast behavior.

3. _resolve_env_reward error message swallows the actual values when a mismatch is detected.

src/openenv/core/harness/__init__.py lines 3905–3913:

raise ValueError(
    "verify.env_reward must forward the environment reward from the rollout"
)

This message does not include trace_reward or verify_reward, making debugging very hard. Suggested improvement:

raise ValueError(
    f"verify.env_reward ({verify_reward!r}) must match the reward from the "
    f"rollout tool trace ({trace_reward!r})"
)

4. _resolve_env_reward only checks the reward of the last tool call with a non-None reward, not the cumulative/final reward.

src/openenv/core/harness/__init__.py lines 3898–3902: the function scans reversed(rollout.tool_trace) and stops at the first entry that has a reward. In multi-step environments where intermediate steps carry partial rewards (e.g., step 1 reward=0.0, step 2 reward=0.5, step 3 reward=1.0), trace_reward will be the reward at the last tool call, not the cumulative reward. Verify the current behavior is intentional (terminal-reward-only environments), and document the assumption.

5. browsergym_harness_eval_common.py implements a custom SessionMCPHttpServer that duplicates the SessionMCPBridge HTTP transport.

examples/browsergym_harness_eval_common.py lines 2251–2366: a bespoke ThreadingHTTPServer wrapping SessionMCPBridge.handle_request. This is in the examples/ directory so it is not core, but it introduces protocol-level code outside the core transport layer. If this pattern is worth keeping, the HTTP wrapper should be promoted to openenv.core.harness as an optional transport, not buried in an example helper.

6. _FILL_RE regex for BrowserGym action parsing cannot handle embedded same-quote characters that the new test asserts.

envs/browsergym_env/harness.py lines 897–899:

_FILL_RE = re.compile(
    r"^fill\(\s*(?P<bq>['\"])(?P<bid>.+?)(?P=bq)\s*,\s*"
    r"(?P<tq>['\"])(?P<text>.*?)(?P=tq)\s*\)$"
)

The test test_browsergym_fill_parser_supports_single_quoted_embedded_quote at line 6673 asserts that fill('42', 'O'Brien') parses to text="O'Brien", but the regex uses a backreference (?P=tq) to the opening quote — the pattern will stop matching at the embedded ' before Brien. Either the test is incorrect, or the regex needs to support escaped/alternate-quoted literals; this should be clarified.

7. CollectResult.episode_ids returns ALL planned ids, including skipped ones.

src/openenv/core/harness/collect.py lines 4724–4728: CollectResult.episode_ids is populated with planned_ids (all planned ids regardless of resume skips or drops). Callers who iterate result.episode_ids expecting only newly-collected ids will be surprised. The field name suggests collected ids but the behavior returns planned ids. Rename to planned_episode_ids or filter to only actually-collected ids.

---

Tier 2 — Alignment & Design

ALIGNMENT FLAG: PR #471 introduces a harness runtime that diverges substantially from RFC 005, which is still "In Review."

• Principle at stake: "Design/Alignment (human-owned): RFCs, principles, trade-off decisions" (CLAUDE.md); "Changes to core without RFC" (alignment-review skill).
• The concern: RFC 005 (rfcs/005-agentic-harnesses.md) proposes HarnessEnvironment wrapping external agentic harnesses (OpenClaw, Claude Code) with step() = one conversational turn and HarnessAdapter = subprocess lifecycle management. PR #471 implements HarnessAdapter as a rollout-loop driver and ResourceSession as a wrapped env client — completely different semantics, same vocabulary. The PR reuses HarnessAdapter, HarnessEvent, HarnessRolloutResult, and MCPHarnessAdapter as top-level names that conflict with the RFC 005 design. Landing PR #471's naming in openenv.core before RFC 005 is resolved will make any future RFC 005 implementation require a breaking rename or a confusing dual-concept namespace.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: A new top-level concept ("session-based harness runtime") is added to openenv.core without a supporting RFC, marked "experimental" only in a docstring.

• Principle at stake: "Changes to core without RFC" (INVARIANTS.md); "Design/Alignment is human-owned."
• The concern: The PR's README says "these helpers are currently experimental while RFC 005 remains in review" — but RFC 005 describes a different pattern. The PR either needs its own RFC to document its design decisions (why ResourceSession rather than the RFC 005 HarnessEnvironment? why is MCPHarnessAdapter a rollout driver rather than a process manager?) or must wait for RFC 005 to be resolved so names do not conflict. There is no dedicated RFC for the ResourceSession / StepEnvSessionAdapter / build_harness_rollout_func design.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: openenv.core.harness imports Tool from openenv.core.env_server.mcp_types in env-package harness adapters.

• Principle at stake: "Client-server separation — Clients must never import from server/ directory" (INVARIANTS.md).
• The concern: envs/browsergym_env/harness.py:687, envs/openspiel_env/harness.py:972, and envs/reasoning_gym_env/harness.py:1144 all import Tool from openenv.core.env_server.mcp_types. The invariant names server/ directories specifically, and env_server is a subpackage of openenv.core, not an env-specific server directory. However, mcp_types.py is a shared types module that existing code already imports (e.g., src/openenv/core/mcp_client.py), so this is likely acceptable. The team should explicitly confirm whether openenv.core.env_server.mcp_types is considered part of the "server side" for purposes of the client-server invariant, or if Tool should be re-exported from a neutral location like openenv.core.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: The openenv collect CLI command hard-codes only openspiel and reasoning_gym as supported env families, with no extension point.

• Principle at stake: "Be hands-on: Provide ready-to-use implementations" vs. "One canonical way to build environments" (PRINCIPLES.md).
• The concern: src/openenv/cli/commands/collect.py's _build_session_factory function dispatches on a string prefix (openspiel:, reasoning_gym:). Adding a new env requires editing core CLI code. A plugin/registry pattern (session factories register themselves by name) would maintain the "one canonical way" without coupling CLI to every supported env. This is a scope and extensibility question worth discussing before the CLI ships.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: build_harness_rollout_func only supports white-box rollouts, leaving CLIHarnessAdapter (black-box) silently broken when passed.

• Principle at stake: "Minimize lifecycle deltas: Training → Evals → Production should use identical interfaces" (PRINCIPLES.md).
• The concern: The function produced by build_harness_rollout_func is documented as a "TRL-compatible rollout function" but silently ignores CLIHarnessAdapter (which only supports run_black_box). Passing a CLIHarnessAdapter to build_harness_rollout_func will raise NotImplementedError at runtime with no helpful message. This asymmetry between the two adapter types, combined with the fact that eval harnesses naturally use CLIHarnessAdapter, means the lifecycle-delta promise is not kept for the eval path.
• Suggested reviewer: @Darktex

---

Verdict

The core runtime mechanics are well-engineered. The "rewards in env" invariant is carefully guarded via _resolve_env_reward, the RESERVED_TOOL_NAMES list is enforced at construction and call time, and the test suite is unusually thorough for a feature PR of this size. However, there are two blockers that warrant changes before merge.

The primary blocker is alignment: PR #471 introduces HarnessAdapter, HarnessEvent, MCPHarnessAdapter, and related types into openenv.core with semantics that directly conflict with the RFC 005 design currently under review. Landing these names and abstractions in the stable core before RFC 005 is resolved will require either a painful rename or a confusing dual-meaning namespace. The PR's own docstrings acknowledge this tension ("experimental while RFC 005 remains in review") but RFC 005 describes a completely different thing (external harness subprocess management vs. the trainer-owned rollout loop this PR implements). This needs a human decision on whether to (a) resolve RFC 005 first and reconcile the naming, (b) write a new RFC for the ResourceSession design, or (c) accept the naming divergence and close RFC 005 as superseded.

The secondary blocker is mechanical: OpenSpielSessionFactory and ReasoningGymSessionFactory do not inherit from ResourceSessionFactory, making them invisible to the type checker when passed to CollectRunner and build_harness_rollout_func. This is a straightforward fix.

The remaining Tier 1 items (error message quality, the fill-regex test, the episode_ids naming) are minor but should be addressed in the same pass.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint: PASS (new files not yet on disk; existing codebase passes ruff/usort)
• Debug code: CLEAN (print statements found are pre-existing in unrelated files, not introduced by this PR)

Tier 1: Fixes Required

• src/openenv/core/harness/collect.py (~line 4688) — Unhandled episode exception kills the entire collection run. Inside CollectRunner.run(), if run_white_box(), session.verify(), or EpisodeRecord.from_rollout() raises (including the intentional ValueError from _resolve_env_reward on reward mismatch), the finally block closes the session correctly but the exception propagates and aborts all remaining episodes. A 300-episode collection run that hits one bad episode loses everything. Per-episode failures should be caught, logged, and counted as num_dropped; only systemic errors (e.g., factory.create() failure) should be fatal.

• src/openenv/core/harness/collect.py (~line 4706) — record is referenced after the try/finally exits with should_keep(record). If any statement inside the try raises, record is unbound and the exception propagates — Python will not reach line 4706, so this is not a visible crash in the happy path, but it is a latent UnboundLocalError waiting to appear if the caller's exception handling ever catches the propagated error and retries. Moving should_keep inside the try block eliminates the risk.

• examples/browsergym_harness_eval_common.py:SessionMCPHttpServer — Missing MCP initialize handshake. The ThreadingHTTPServer-based bridge handles tools/list and tools/call but does not implement the initialize method required by MCP 2025-03-26 spec. A conforming client (including current Codex CLI versions that implement the spec) will send initialize first; the bridge returns METHOD_NOT_FOUND and the connection is rejected. Add a minimal initialize handler returning {"protocolVersion": "2025-03-26", "capabilities": {"tools": {}}, "serverInfo": {"name": "openenv-session-bridge", "version": "0.1.0"}}.

• src/openenv/core/harness/collect.py:build_model_step — Verify run_async_safely call convention. The call run_async_safely(llm_client.complete_with_tools(...)) passes a coroutine object. Confirm that openenv/core/utils.py:run_async_safely accepts a coroutine (not a callable) — if the utility expects a zero-argument callable this is a silent runtime TypeError that only surfaces during actual collection.

• tests/envs/test_browsergym_harness.py:6537 — Fragile sys.path.insert with os.path relative to __file__ will break when pytest is invoked from any directory other than repo root. Remove; rely on PYTHONPATH=src:envs as documented in CLAUDE.md and used by all other test files.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: Two parallel harness packages with conflicting architectures under near-identical names

• Principle at stake: PRINCIPLES.md — "One canonical way to build environments"; RFC 005 implementation plan

• The concern: The repository now contains two packages claiming to implement RFC 005:

  1. src/openenv/core/harnesses/ (plural, already on disk) — HarnessEnvironment(MCPEnvironment) with reset_async/step_async, HarnessAdapter ABC with start/stop/send_message. This wraps external CLI harness processes (OpenClaw, Claude Code) where the agent lives inside the container.
  2. src/openenv/core/harness/ (singular, this PR) — ResourceSession/StepEnvSessionAdapter with call_tool/verify, MCPHarnessAdapter. This is a white-box training-loop model where the trainer owns LLM sampling and drives environment steps via MCP-style tool calls.

  Both export types named HarnessAdapter, HarnessEvent, HarnessRolloutResult. RFC 005's stated implementation plan (PR 2 = Foundation Types, PR 3 = Core Implementation) maps to the harnesses/ package. This PR implements an architecturally different pattern without reconciling what is already on disk. The new package's README labels itself "experimental while RFC 005 remains in review" — but so is harnesses/. A future contributor has no basis to choose between them. This needs an explicit decision before merge: does this PR replace harnesses/, extend it, or represent a distinct abstraction layer that needs its own RFC or RFC amendment?

• Suggested reviewer: @Darktex

ALIGNMENT FLAG: SessionMCPHttpServer introduces a custom HTTP protocol counter to the WebSocket-only direction

• Principle at stake: INVARIANTS.md §4 — "WebSocket for all environment communication. No custom protocols." (also: "We are in the process of deprecating HTTP in favor of WebSocket-only")
• The concern: examples/browsergym_harness_eval_common.py ships a hand-rolled ThreadingHTTPServer-based MCP JSON-RPC bridge. This is an examples/ file (not src/), but it directly contradicts the stated HTTP deprecation direction and differs from the RFC 005 production mode design, which explicitly uses a WebSocket endpoint (/harness). The Codex eval path should either use a WebSocket bridge or the MCP streamable-HTTP transport (which is spec-compliant), not a bespoke BaseHTTPRequestHandler implementation.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: reward_key parameter on build_harness_rollout_func loosens the reward-in-environment surface

• Principle at stake: RFC 002 / INVARIANTS.md §3 — "Rewards in environment"
• The concern: build_harness_rollout_func(reward_key="env_reward") makes the key under which the environment reward is returned to TRL configurable. The reward value is correctly constrained by _resolve_env_reward, but the configurable key means the return dict key can be changed to anything — which could obscure to future callers that this is the environment reward, not a synthetic one. Consider hardcoding "env_reward" to make the invariant explicit at the call site.
• Suggested reviewer: @Darktex

Summary

• 5 mechanical issues to fix (the episode-abort issue is the critical one for production use)
• 3 alignment points for human review

The core runtime logic — StepEnvSessionAdapter, MCPHarnessAdapter, CLIHarnessAdapter, _resolve_env_reward, RESERVED_TOOL_NAMES — is well-designed and thoroughly tested. The reward-invariant enforcement (raising on verify/trace mismatch, blocking reserved orchestration tool names at both the adapter and bridge layers) correctly implements the key invariants from RFC 002 and INVARIANTS.md. The principal blocker for merge is the unresolved relationship between openenv.core.harness (this PR) and openenv.core.harnesses (on disk): merging without an explicit decision leaves the repository with two competing harness abstractions under nearly identical names, which will create long-term maintenance confusion.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Summary

This PR adds a session-based harness runtime (openenv.core.harness), BrowserGym/OpenSpiel/ReasoningGym adapters, a collect pipeline, CLI command, and substantial documentation. The design intent is sound and aligns with RFC 005. However, there is a hard structural collision with the existing src/openenv/core/harnesses/ package that was already landed as RFC 005's PR 2 foundation, and two correctness bugs that need fixing before merge.

Tier 1 (Bugs/Lint)

• Name collision / broken import tree: The PR adds src/openenv/core/harness/ (singular) alongside the existing src/openenv/core/harnesses/ (plural). Both define HarnessEvent and HarnessAdapter as incompatible types. HarnessEvent in the new package is a @dataclass with a payload: dict field; in the existing package it is a Pydantic BaseModel with a data: dict field. HarnessAdapter in the new package has run_white_box/run_black_box; in the existing package it has start/stop/inject_tools/send_message. These must be reconciled — either by extending the existing package or by replacing it with a clear deprecation path.

• NameError on reward-missing episode in CollectRunner: In src/openenv/core/harness/collect.py, CollectRunner.run() wraps EpisodeRecord.from_rollout(...) in a try/finally. If from_rollout raises (e.g., _resolve_env_reward raises because the rollout has no reward), execution exits the try block with record unset. The code after the finally block then references record in the should_keep check, producing a NameError that shadows the original exception. The if should_keep ... block must be inside the try body or guarded with an explicit record sentinel.

• Fragile sync/async adaptation: StepEnvSessionAdapter.__init__ uses if hasattr(client, 'sync') and callable(client.sync) to unwrap async clients. If a caller passes an async client without a .sync() shim, all self._client.reset(...) and self._client.step(...) calls return unawaited coroutines, producing silent failures at runtime. The adaptation contract must be documented and ideally enforced with a type check or explicit isinstance guard.

• sys.path manipulation in example scripts: examples/browsergym_codex_eval.py and examples/browsergym_harness_eval.py use sys.path.insert(0, ...) to locate src/ and envs/. This is dev scaffolding and should be replaced with standard package installation or a pyproject.toml script entry.

Tier 2 (Alignment)

ALIGNMENT FLAG: Two parallel harness module trees with overlapping types

• Principle at stake: "One canonical way to build environments" (PRINCIPLES.md); RFC 005 staged implementation plan
• The concern: RFC 005 specifies a staged plan where PR 2 delivers foundation types and PR 3 delivers core implementation. The existing src/openenv/core/harnesses/ package is PR 2. This PR lands what appears to be PR 3 content but as a sibling package (harness/ singular) with a different abstraction model, leaving both definitions coexisting. The RFC needs to be updated to reflect the session-based model, or the two packages must be merged.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: _resolve_env_reward allows orchestration-layer reward synthesis by omission

• Principle at stake: "Rewards inside environment" (RFC 002, PRINCIPLES.md)
• The concern: If both tool_result.metadata['reward'] and tool_result.data['reward'] are absent, _resolve_env_reward silently accepts a verify_reward synthesized outside the environment. The guard only fires when both values are present and disagree. An adapter that never populates the reward in tool results can synthesize reward freely.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: Session rollout bypasses WebSocket orchestration plane

• Principle at stake: Dual API boundary (RFC 001, INVARIANTS.md)
• The concern: StepEnvSessionAdapter calls client.reset()/client.step() in-process, bypassing the WebSocket infrastructure API. Standard monitoring, replay, and orchestration tooling that hooks into the WebSocket layer will not observe harness rollouts.
• Suggested reviewer: @Darktex

Verdict

request_changes — the package naming collision is a merge blocker (it will break the existing harnesses package for anyone who has already adopted it), and the NameError in CollectRunner is a correctness bug. The alignment questions require human sign-off before the session model is considered canonical alongside RFC 005.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

Automated Checks

• Lint: PASS — ruff passes on all existing code paths that could be checked; the new src/openenv/core/harness/ module is not yet on disk so full lint could not be run on it directly.
• Debug code: CLEAN — no debug artifacts introduced by this PR.

---

Tier 1: Fixes Required

• src/openenv/core/harness/__init__.py and collect.py — _resolve_env_reward is a private function exported across module boundary without __all__ entry. collect.py imports _resolve_env_reward directly from the harness package init (from . import _resolve_env_reward), but _resolve_env_reward is intentionally absent from __all__. Either add it to __all__ (which requires a naming decision — it should probably be renamed without the underscore prefix) or factor it into a shared internal _util.py that both __init__.py and collect.py import from. As-is, the import works but is fragile: any refactor that moves the function will silently break collect.py because nothing in __all__ contracts it.

• src/openenv/core/harness/__init__.py:3892 — _resolve_env_reward raises ValueError when neither trace reward nor verify reward is present, but StepEnvSessionAdapter._default_verify returns env_reward=None on an empty rollout (no call_tool ever called). A zero-step rollout (e.g. agent immediately emits no tool calls) will crash build_harness_rollout_func with ValueError: rollout did not produce an environment reward rather than returning reward=0.0 or routing to a documented error path. Add a test for the zero-step case and decide the intended semantics.

• src/openenv/core/harness/__init__.py:4197 — early termination on no-tool-call response sets result.done = True but does not call session.verify(). MCPHarnessAdapter.run_white_box breaks out of the loop and falls through to the metrics population code, then returns. The caller (build_harness_rollout_func) calls session.verify() after the fact, which is correct. But when run_black_box on CLIHarnessAdapter is the path, the runner callable receives a SessionMCPBridge — it is up to the external runner to call verify, which is nowhere enforced or documented in the interface. The HarnessAdapter.run_black_box docstring should state verify responsibility explicitly.

• envs/browsergym_env/harness.py (line ~856 in diff) — BrowserGymSessionFactory.create() calls StepEnvSessionAdapter(...) which calls client.reset() in __init__. If reset() raises (e.g. network error against the HF Space), the exception propagates uncaught through session_factory.create(task=prompt) inside build_harness_rollout_func, skipping the finally: session.close() block because session was never assigned. This is a resource leak pattern — the try/finally in rollout_func only guards after session is bound. Restructure to: session = session_factory.create(...) before try, or wrap create() in its own try-except.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG: Naming collision between openenv.core.harnesses and openenv.core.harness

• Principle at stake: "Simple Gymnasium-style API" and "Minimize lifecycle deltas" (PRINCIPLES.md); coherence of the core module surface
• The concern: Two coexisting modules named harnesses (existing, RFC 005, manages subprocess harnesses: OpenClaw, Stdio) and harness (this PR, session runtime for training). Both define classes named HarnessAdapter, HarnessEvent, and HarnessConfig/HarnessRunLimits. The two HarnessAdapter ABCs have completely different method signatures: the old one has start() / stop() / inject_tools() / send_message() while the new one has run_white_box() / run_black_box(). Users importing from openenv.core.harness and openenv.core.harnesses will encounter identically named but incompatible interfaces. This is a strong candidate for consolidation or explicit renaming before landing. The RFC 005 does not describe this two-module split.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: SessionMCPBridge — name implies MCP but protocol is bespoke JSON-RPC

• Principle at stake: "MCP as universal standard — All agent-environment tool interaction via MCP" (PRINCIPLES.md, RFC 003); Dual API Boundary invariant (INVARIANTS.md)
• The concern: SessionMCPBridge.handle_request() implements a subset of JSON-RPC 2.0 with MCP-shaped method names (tools/list, tools/call). This is close to real MCP but is not wired through a FastMCP server — it is an in-process dict-in / dict-out interface. External agents that speak real MCP (over stdio or SSE) cannot reach this bridge without additional plumbing. The CLIHarnessAdapter receives a SessionMCPBridge and is expected to wire it up, but there is no standard adapter provided that exposes it as a real MCP transport. As a result, the "all agent tool interaction via MCP" invariant is satisfied in spirit (the tool surface is MCP-shaped) but not in transport (no real MCP server is instantiated). Whether this is acceptable for a training-only path should be confirmed. The SessionMCPBridge docstring should state it is an in-process bridge, not a network-accessible MCP server.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: VerifyResult.env_reward naming creates external reward synthesis risk

• Principle at stake: "Rewards inside environment" (RFC 002, PRINCIPLES.md); _resolve_env_reward enforcement
• The concern: VerifyResult.env_reward is described as "must forward reward already produced inside the environment" and _resolve_env_reward enforces that it matches the trace reward. However, the _default_verify path in StepEnvSessionAdapter sets env_reward = last_result.reward, which is the right value. Custom verify_builder callables supplied by env authors are trusted to follow the "forward, don't synthesize" rule — there is no enforcement at the builder level. A new env author writing a verify_builder could innocently write return VerifyResult(env_reward=my_rubric(transcript)) and bypass the trace-vs-verify consistency check entirely if the rollout has no tool calls in the trace (because _resolve_env_reward will accept verify_reward alone when trace_reward is None). The zero-step-trace hole means custom verify builders can synthesize external rewards undetected. This should either be documented as a known limitation or closed by requiring trace_reward to always be present when a custom verify builder is used.
• Suggested reviewer: @Darktex

---

Summary

• 4 mechanical issues to fix (private-function cross-module import, zero-step reward crash, unguarded resource leak in rollout_func, undocumented verify responsibility in black-box path)
• 3 alignment points for human review (module naming collision between harness and harnesses; SessionMCPBridge MCP-in-name-only transport; reward synthesis hole in custom verify builders)

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

What This PR Does

This PR adds a harness session runtime to openenv.core.harness (note: singular) consisting of:

• ResourceSession / ResourceSessionFactory / StepEnvSessionAdapter — adapts existing step/reset clients into a session interface for the training loop
• SessionMCPBridge — exposes a session's tools as an in-process MCP JSON-RPC endpoint
• MCPHarnessAdapter (white-box) and CLIHarnessAdapter (black-box) — run rollouts through the session
• build_harness_rollout_func — TRL-compatible rollout function factory
• HarnessRunLimits, ModelStepResult, HarnessRolloutResult — rollout plumbing types
• CollectRunner + RolloutSerializer + push_to_hf_hub — supervised dataset collection pipeline
• openenv collect CLI command
• BrowserGymSessionFactory, OpenSpielSessionFactory, ReasoningGymSessionFactory adapters
• SFT warm-up tutorial, BrowserGym harness tutorial, several example scripts

RFC 005 (status: In Review) is the design backing this PR.

---

Automated Checks

• Lint: FAIL — bash .claude/hooks/lint.sh exits with code 2 (no output beyond the header, likely usort or ruff failing on the new files which are not yet in the working tree). Cannot verify lint status against the PR branch from this sandbox.
• Debug code: CLEAN in src/ — no debug statements found in the new library code. Print statements in examples/ and tutorial docs are intentional.

---

Tier 1: Fixes Required

• src/openenv/core/harness/__init__.py — _resolve_env_reward raises unhandled ValueError in TRL training loop. If the model exhausts max_turns without making any tool call, rollout.tool_trace is empty and self._last_result is None in StepEnvSessionAdapter, making both trace_reward and verify.env_reward None. _resolve_env_reward then raises ValueError("rollout did not produce an environment reward"). This propagates through build_harness_rollout_func uncaught (only finally: session.close() is guarded) and crashes the TRL training batch. The fix is to either return 0.0 as a sentinel reward with a metric flag ("timed_out": True), or to catch the ValueError inside rollout_func and append 0.0.

• src/openenv/core/harness/__init__.py — HarnessEvent name clash with openenv.core.harnesses.types.HarnessEvent. This PR adds @dataclass class HarnessEvent(type: str, payload: dict) in openenv.core.harness, while openenv.core.harnesses.types.HarnessEvent is a Pydantic model with type: HarnessEventType (enum) and timestamp: float. Any file doing from openenv.core.harness import HarnessEvent and also importing from openenv.core.harnesses gets silent type confusion. These two classes are incompatible. The new dataclass should be renamed (e.g., RolloutEvent) to prevent import-time shadowing and runtime surprises.

• src/openenv/core/harness/collect.py — EpisodeRecord not serializable when task is a non-JSON-primitive. EpisodeRecord.to_dict() calls dataclasses.asdict(), which does not call json.dumps(default=str) — the json.dumps(record.to_dict(), default=str) in RolloutSerializer.write_episode does apply the default=str fallback, so this won't raise, but it means task objects are silently stringified. The to_dict() docstring / type hint should note this, or asdict should be replaced with a manual serializer that applies default=str consistently.

• src/openenv/cli/commands/collect.py — _reset_results_file is called before write_metadata in the resume=False path, but write_metadata does not clear existing metadata. When --no-resume is passed, _reset_results_file deletes results.jsonl but the old metadata.json is overwritten (not deleted). This is fine in practice because write_metadata overwrites it, but the comment on _reset_results_file implies a full reset which is incomplete. Minor but could confuse operators inspecting the output directory.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG 1: Two coexisting harness namespaces with overlapping purpose

• Principle at stake: "Minimize lifecycle deltas" (PRINCIPLES.md §Core Principles) — training, eval, and production should use identical interfaces.
• The concern: This PR creates openenv.core.harness (singular, session-based rollout runtime) alongside the existing openenv.core.harnesses (plural, RFC 005 HarnessEnvironment wrapping subprocess harnesses like OpenClaw). Both are under openenv.core, both address "how to run an agent inside an environment," and both name-clash on HarnessEvent, HarnessAdapter, ResourceSession, etc. RFC 005's implementation plan (PR 2–5) describes a single HarnessEnvironment-based path; this PR ships a parallel ResourceSession-based path without explicitly superseding or integrating the harnesses/ code. Future contributors will face ambiguity: which harness abstraction should I use? Is harnesses/ deprecated?
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 2: RFC 005 is still "In Review" — this PR ships substantial implementation

• Principle at stake: PRINCIPLES.md "When to Revisit These Principles" / CONTRIBUTING.md pre-RFC workflow.
• The concern: rfcs/005-agentic-harnesses.md has Status: In Review as of this PR. The PR ships ~700 lines of openenv.core.harness, 500 lines of collect.py, 3 env adapters, a CLI command, 2000+ lines of tests, and tutorial docs — all implementing this RFC while it has not been marked Accepted. If the RFC discussion is still live, implementations landed before approval make the RFC discussion academic. Is RFC 005 effectively ratified and this PR is an approved implementation step?
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 3: StepEnvSessionAdapter calls client.reset() at construction time, inside a framework that is intended to be orchestration-only

• Principle at stake: INVARIANTS.md §Security Invariants — "Agents cannot access reset/simulation controls."
• The concern: StepEnvSessionAdapter.__init__ calls self._client.reset(**reset_payload). The session is constructed by ResourceSessionFactory.create(), which is called inside build_harness_rollout_func's rollout_func. The rollout_func is called by TRL (the training loop / orchestrator) — so this is orchestration-side and is correct in intent. However, the ResourceSession abstraction is also returned to model_step_builder(trainer, session) before the rollout runs. The session object holds a reference to self._client, and self._client has reset() and state() methods exposed on it. If model_step_builder were to pass session._client to agent code, reset() would be accessible. The current interface does not expose _client publicly, but the underscore-convention-only encapsulation is fragile. This should be explicitly documented as a private implementation detail not accessible via ResourceSession's public API.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 4: _resolve_env_reward's cross-check enforces rewards-inside-env, but the verify() contract is underdocumented

• Principle at stake: INVARIANTS.md §Architectural Invariants — "Reward computation must stay inside environment boundary."
• The concern: _resolve_env_reward correctly raises if verify.env_reward disagrees with the tool trace reward. This is a good enforcement of the invariant. However, the VerifyResult docstring says env_reward must forward reward already produced inside the environment — the word "must" is a contract, but it is not enforced at the type level (it's a float | None). verify_builder is a caller-supplied callable. A careless verify_builder that synthesizes a new reward (rather than forwarding) and matches the trace reward by coincidence would pass the check. This is a documentation and code-review concern, not a runtime bug, but worth flagging as a place where the invariant enforcement has a hole.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 5: SessionMCPHttpServer in examples/browsergym_harness_eval_common.py implements a custom HTTP protocol

• Principle at stake: INVARIANTS.md §Communication Patterns — "WebSocket for all environment communication. No custom protocols."
• The concern: SessionMCPHttpServer is a hand-rolled ThreadingHTTPServer that implements MCP JSON-RPC over plain HTTP POST. INVARIANTS.md says WebSocket for all environment communication, no custom protocols. The INVARIANTS.md note acknowledges HTTP is being deprecated in favor of WebSocket-only. However, SessionMCPHttpServer is purpose-built for the Codex CLI black-box evaluation path (not training), and it lives in examples/ not src/. The question is whether introducing a second ad-hoc HTTP-based MCP transport in the codebase moves further away from WebSocket-only, even if only in examples. Should this use the existing HTTP infrastructure or is the deviation acceptable for evaluation-only examples?
• Suggested reviewer: @Darktex

---

Summary

• 3 mechanical issues to fix (unhandled ValueError in training loop, HarnessEvent name clash, incomplete _reset_results_file behavior)
• 1 documentation/type-safety issue (non-JSON task serialization behavior undocumented)
• 5 alignment points for human review (dual harness namespaces, RFC status vs. implementation, reset() accessibility, reward invariant enforcement gap, custom HTTP transport in examples)

The core runtime logic (ResourceSession, MCPHarnessAdapter, SessionMCPBridge, _resolve_env_reward) is well-structured and shows clear awareness of the agent-isolation and rewards-inside-environment invariants. The RESERVED_TOOL_NAMES guard at both the session layer and the MCP bridge layer is a good defense-in-depth pattern. The biggest blocking issue before merge is the ValueError crash in build_harness_rollout_func for rollouts that hit max_turns without any tool calls, plus the HarnessEvent name collision that will cause silent type errors.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review Report

What This PR Does

This PR adds a harness session runtime to openenv.core.harness (note: singular) consisting of:

• ResourceSession / ResourceSessionFactory / StepEnvSessionAdapter — adapts existing step/reset clients into a session interface for the training loop
• SessionMCPBridge — exposes a session's tools as an in-process MCP JSON-RPC endpoint
• MCPHarnessAdapter (white-box) and CLIHarnessAdapter (black-box) — run rollouts through the session
• build_harness_rollout_func — TRL-compatible rollout function factory
• HarnessRunLimits, ModelStepResult, HarnessRolloutResult — rollout plumbing types
• CollectRunner + RolloutSerializer + push_to_hf_hub — supervised dataset collection pipeline
• openenv collect CLI command
• BrowserGymSessionFactory, OpenSpielSessionFactory, ReasoningGymSessionFactory adapters
• SFT warm-up tutorial, BrowserGym harness tutorial, several example scripts

RFC 005 (status: In Review) is the design backing this PR.

---

Automated Checks

• Lint: FAIL — bash .claude/hooks/lint.sh exits with code 2 (no output beyond the header, likely usort or ruff failing on the new files which are not yet in the working tree). Cannot verify lint status against the PR branch from this sandbox.
• Debug code: CLEAN in src/ — no debug statements found in the new library code. Print statements in examples/ and tutorial docs are intentional.

---

Tier 1: Fixes Required

• src/openenv/core/harness/__init__.py — _resolve_env_reward raises unhandled ValueError in TRL training loop. If the model exhausts max_turns without making any tool call, rollout.tool_trace is empty and self._last_result is None in StepEnvSessionAdapter, making both trace_reward and verify.env_reward None. _resolve_env_reward then raises ValueError("rollout did not produce an environment reward"). This propagates through build_harness_rollout_func uncaught (only finally: session.close() is guarded) and crashes the TRL training batch. The fix is to either return 0.0 as a sentinel reward with a metric flag ("timed_out": True), or to catch the ValueError inside rollout_func and append 0.0.

• src/openenv/core/harness/__init__.py — HarnessEvent name clash with openenv.core.harnesses.types.HarnessEvent. This PR adds @dataclass class HarnessEvent(type: str, payload: dict) in openenv.core.harness, while openenv.core.harnesses.types.HarnessEvent is a Pydantic model with type: HarnessEventType (enum) and timestamp: float. Any file doing from openenv.core.harness import HarnessEvent and also importing from openenv.core.harnesses gets silent type confusion. These two classes are incompatible. The new dataclass should be renamed (e.g., RolloutEvent) to prevent import-time shadowing and runtime surprises.

• src/openenv/core/harness/collect.py — EpisodeRecord not serializable when task is a non-JSON-primitive. EpisodeRecord.to_dict() calls dataclasses.asdict(), which does not call json.dumps(default=str) — the json.dumps(record.to_dict(), default=str) in RolloutSerializer.write_episode does apply the default=str fallback, so this won't raise, but it means task objects are silently stringified. The to_dict() docstring / type hint should note this, or asdict should be replaced with a manual serializer that applies default=str consistently.

• src/openenv/cli/commands/collect.py — _reset_results_file is called before write_metadata in the resume=False path, but write_metadata does not clear existing metadata. When --no-resume is passed, _reset_results_file deletes results.jsonl but the old metadata.json is overwritten (not deleted). This is fine in practice because write_metadata overwrites it, but the comment on _reset_results_file implies a full reset which is incomplete. Minor but could confuse operators inspecting the output directory.

---

Tier 2: Alignment Discussion

ALIGNMENT FLAG 1: Two coexisting harness namespaces with overlapping purpose

• Principle at stake: "Minimize lifecycle deltas" (PRINCIPLES.md §Core Principles) — training, eval, and production should use identical interfaces.
• The concern: This PR creates openenv.core.harness (singular, session-based rollout runtime) alongside the existing openenv.core.harnesses (plural, RFC 005 HarnessEnvironment wrapping subprocess harnesses like OpenClaw). Both are under openenv.core, both address "how to run an agent inside an environment," and both name-clash on HarnessEvent, HarnessAdapter, ResourceSession, etc. RFC 005's implementation plan (PR 2–5) describes a single HarnessEnvironment-based path; this PR ships a parallel ResourceSession-based path without explicitly superseding or integrating the harnesses/ code. Future contributors will face ambiguity: which harness abstraction should I use? Is harnesses/ deprecated?
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 2: RFC 005 is still "In Review" — this PR ships substantial implementation

• Principle at stake: PRINCIPLES.md "When to Revisit These Principles" / CONTRIBUTING.md pre-RFC workflow.
• The concern: rfcs/005-agentic-harnesses.md has Status: In Review as of this PR. The PR ships ~700 lines of openenv.core.harness, 500 lines of collect.py, 3 env adapters, a CLI command, 2000+ lines of tests, and tutorial docs — all implementing this RFC while it has not been marked Accepted. If the RFC discussion is still live, implementations landed before approval make the RFC discussion academic. Is RFC 005 effectively ratified and this PR is an approved implementation step?
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 3: StepEnvSessionAdapter calls client.reset() at construction time, inside a framework that is intended to be orchestration-only

• Principle at stake: INVARIANTS.md §Security Invariants — "Agents cannot access reset/simulation controls."
• The concern: StepEnvSessionAdapter.__init__ calls self._client.reset(**reset_payload). The session is constructed by ResourceSessionFactory.create(), which is called inside build_harness_rollout_func's rollout_func. The rollout_func is called by TRL (the training loop / orchestrator) — so this is orchestration-side and is correct in intent. However, the ResourceSession abstraction is also returned to model_step_builder(trainer, session) before the rollout runs. The session object holds a reference to self._client, and self._client has reset() and state() methods exposed on it. If model_step_builder were to pass session._client to agent code, reset() would be accessible. The current interface does not expose _client publicly, but the underscore-convention-only encapsulation is fragile. This should be explicitly documented as a private implementation detail not accessible via ResourceSession's public API.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 4: _resolve_env_reward's cross-check enforces rewards-inside-env, but the verify() contract is underdocumented

• Principle at stake: INVARIANTS.md §Architectural Invariants — "Reward computation must stay inside environment boundary."
• The concern: _resolve_env_reward correctly raises if verify.env_reward disagrees with the tool trace reward. This is a good enforcement of the invariant. However, the VerifyResult docstring says env_reward must forward reward already produced inside the environment — the word "must" is a contract, but it is not enforced at the type level (it's a float | None). verify_builder is a caller-supplied callable. A careless verify_builder that synthesizes a new reward (rather than forwarding) and matches the trace reward by coincidence would pass the check. This is a documentation and code-review concern, not a runtime bug, but worth flagging as a place where the invariant enforcement has a hole.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG 5: SessionMCPHttpServer in examples/browsergym_harness_eval_common.py implements a custom HTTP protocol

• Principle at stake: INVARIANTS.md §Communication Patterns — "WebSocket for all environment communication. No custom protocols."
• The concern: SessionMCPHttpServer is a hand-rolled ThreadingHTTPServer that implements MCP JSON-RPC over plain HTTP POST. INVARIANTS.md says WebSocket for all environment communication, no custom protocols. The INVARIANTS.md note acknowledges HTTP is being deprecated in favor of WebSocket-only. However, SessionMCPHttpServer is purpose-built for the Codex CLI black-box evaluation path (not training), and it lives in examples/ not src/. The question is whether introducing a second ad-hoc HTTP-based MCP transport in the codebase moves further away from WebSocket-only, even if only in examples. Should this use the existing HTTP infrastructure or is the deviation acceptable for evaluation-only examples?
• Suggested reviewer: @Darktex

---

Summary

• 3 mechanical issues to fix (unhandled ValueError in training loop, HarnessEvent name clash, incomplete _reset_results_file behavior)
• 1 documentation/type-safety issue (non-JSON task serialization behavior undocumented)
• 5 alignment points for human review (dual harness namespaces, RFC status vs. implementation, reset() accessibility, reward invariant enforcement gap, custom HTTP transport in examples)

The core runtime logic (ResourceSession, MCPHarnessAdapter, SessionMCPBridge, _resolve_env_reward) is well-structured and shows clear awareness of the agent-isolation and rewards-inside-environment invariants. The RESERVED_TOOL_NAMES guard at both the session layer and the MCP bridge layer is a good defense-in-depth pattern. The biggest blocking issue before merge is the ValueError crash in build_harness_rollout_func for rollouts that hit max_turns without any tool calls, plus the HarnessEvent name collision that will cause silent type errors.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review — Harness Session Runtime + BrowserGym Adapter

Automated Checks

• Lint: NOT RUN — the new src/openenv/core/harness/ package is not present on the current local branch, so I reviewed from the diff. Please ensure uv run usort check + uv run ruff format --check + uv run ruff check pass over the new files before merging.
• Debug code: CLEAN per the standard hook.

---

Tier 1: Bugs / Mechanical Issues

These are flagged from reading the diff; please double-check on the actual branch:

• CollectRunner.run() (src/openenv/core/harness/collect.py) — the try/finally only guarantees session.close(). If EpisodeRecord.from_rollout() raises mid-batch (e.g., the _resolve_env_reward mismatch check), the exception propagates out of the per-episode loop and aborts the entire collect run. Intended behavior appears to be "drop the episode, log, continue" — please wrap the per-episode body so a single bad episode doesn't kill a whole batch.
• StepEnvSessionAdapter.__init__ — calls self._client.reset(...) from the constructor. If reset() raises (transient network error, env not ready), the constructor throws and the caller has no handle to call close() on. Either move reset() out of __init__ (lazy/explicit start()), or ensure close() is safe to call on a partially-constructed instance.
• build_browsergym_action_str (envs/browsergym_env/harness.py) — _quote is aliased to repr. repr() produces Python-style quoting; BrowserGym actions have their own quoting convention. This will silently corrupt action strings containing \n, \t, or non-ASCII. Use a dedicated escape function and add a test with awkward characters.
• SessionMCPHttpServer (examples/browsergym_harness_eval_common.py) — the broad except Exception in tools/call maps arbitrary server-side errors to INTERNAL_ERROR with str(exc) in the message, leaking internals to the CLI harness. Scope this more tightly or scrub the message.
• CollectRunner.run() — from rich.progress import … is inside the function body. If rich isn't installed this raises ImportError at runtime, not import time. Either declare rich as a hard dep or wrap with a graceful fallback.

---

Tier 2: Alignment Discussion

These are the more important questions for human review.

🚩 Parallel harness implementation duplicates the existing openenv.core.harnesses/ module. The repo already contains src/openenv/core/harnesses/ (plural — the RFC 005 implementation with HarnessEnvironment, HarnessAdapter, HarnessConfig, etc.). This PR adds a second module at src/openenv/core/harness/ (singular) with entirely different abstractions (ResourceSession, StepEnvSessionAdapter, SessionMCPBridge, MCPHarnessAdapter, CLIHarnessAdapter, build_harness_rollout_func). The two share no code and represent divergent takes on the same problem. The relationship between them needs to be made explicit — supersede, coexist, or rename.

🚩 RFC required. Per .claude/skills/rfc-check/SKILL.md: new APIs in src/openenv/core/, new abstractions, and changes to the two-interface model all require an RFC. This PR adds 6+ public abstractions under openenv.core.harness/, ships a public __all__, a tutorial, and a new openenv collect CLI command. Even with an "experimental" label, this is a production-facing commitment. RFC 005 (referenced in the PR) is still "In Review" and implements a different design; please either amend RFC 005 or file a new RFC before this lands in openenv.core.

🚩 Weakened "agents cannot reset" invariant. StepEnvSessionAdapter.__init__ calls client.reset(...), and BrowserGymSessionFactory.create() / OpenSpielSessionFactory.create() route through this adapter. In build_harness_rollout_func, session_factory.create(task=prompt) is invoked per-prompt inside the rollout — a path that can be reached from user-supplied model_step_builder closures. The invariant is currently enforced only by convention; the previous design kept reset() behind the HTTP boundary. Please consider whether the structural guarantee is being relaxed and document the intent.

🚩 External reward pathway via VerifyResult.env_reward. _resolve_env_reward permits verify.env_reward (produced by session.verify() outside the environment) to serve as the episode reward when no reward appears in tool_trace. The mismatch check can be bypassed if trace_reward is None, so custom verify_builder implementations can synthesize arbitrary rewards in the orchestration layer. PRINCIPLES/RFC 002 require rewards to be computed inside the environment; please confirm whether this is an intentional carve-out (and document it) or tighten the contract.

---

RFC Recommendation

Required. Before merge, please file an RFC (or amend RFC 005) that:

1. Reconciles openenv.core.harnesses (existing) with openenv.core.harness (new) — supersede, alias, or rename.
2. Sanctions the session-based rollout pattern alongside the direct reset()/step() loop.
3. Explicitly addresses the agents-cannot-reset invariant in the session-factory path.
4. Confirms or revises the rewards-in-environment contract for VerifyResult-based reward forwarding.

---

Summary

The implementation craft is solid — RESERVED_TOOL_NAMES, the _resolve_env_reward mismatch check, and the test coverage are thoughtful. The concerns are primarily about architectural alignment: two parallel harness modules in openenv.core, a missing RFC for a substantial new public surface, and two invariants that need explicit human sign-off. Requesting changes so these can be resolved before the new module ossifies.

---

Automated review by Claude Code | Learn more

[Darktex]

Note: This is an automated review by Claude Code, not a human review.

---

Alignment Review: Harness Session Runtime + BrowserGym Adapter

Substantial PR with strong test coverage (1460+ test lines) and a clear design intent. The "rewards inside environment" enforcement via _resolve_env_reward with float tolerance is a particular highlight. There are mechanical issues that should be addressed and alignment questions that warrant human sign-off before merging.

---

Tier 1: Fixes Required

[Bug] Potential NameError in CollectRunner.run() on episode failure
src/openenv/core/harness/collect.py (around the per-episode try/finally): if run_white_box, session.verify(), or EpisodeRecord.from_rollout() raises inside the try block, finally runs and closes the session correctly, then the exception re-raises — but record was never assigned. Code immediately after the try/finally accesses record unconditionally, masking the original error with a NameError. Fix by catching exceptions, counting them as failed episodes, and continue-ing.

[Bug] StepEnvSessionAdapter.__init__ calls client.reset() in the constructor
src/openenv/core/harness/__init__.py: if reset() raises, the adapter is partially constructed and downstream close() runs on a client in an undefined state. Move the reset() into an explicit open() step or guard it so close() is safe.

[Risk] _quote uses repr() for BrowserGym action strings
envs/browsergym_env/harness.py: Python repr() produces Python-style string literals. Whether BrowserGym's real action parser accepts these is undocumented; tests round-trip against a parser defined in the same PR. Either pin the quoting contract against the actual BrowserGym parser or narrow the quoter.

[Inconsistency] OpenSpielSessionFactory and ReasoningGymSessionFactory don't extend ResourceSessionFactory
Unlike BrowserGymSessionFactory, these factories are plain classes — they duck-type the create() signature but fail isinstance(factory, ResourceSessionFactory) checks that matter for build_harness_rollout_func typing.

[Style] sys.path.insert in test file
tests/envs/test_browsergym_harness.py: module-level sys.path.insert in a test breaks isolation. Use conftest.py or pyproject.toml pythonpath.

---

Tier 2: Alignment Discussion (for human reviewer)

[FLAG] RFC 005 is Status: In Review — this PR lands a 714-line public API that partially implements the RFC before it's approved.
The RFC's HarnessAdapter wraps an external CLI process (OpenClaw, Claude Code). This PR's MCPHarnessAdapter drives the model in a ReAct loop — a different abstraction reusing the same name. These directions are not obviously reconcilable without an explicit decision. Landing this first creates a fait-accompli that will pressure the RFC discussion. Recommend either (a) sign off RFC 005 first, or (b) update RFC 005 to reflect this PR's design before merging.

[FLAG] CLIHarnessAdapter.run_black_box passes both bridge and session to the runner callback.
The session reference exposes verify(), allowing the runner to observe reward signals mid-episode. The apparent intent is that black-box runners interact only via bridge (MCP JSON-RPC). Consider passing only bridge to the runner and keeping session cleanup in the caller. Relevant invariant: INVARIANTS.md "Dual API boundary — the Gym-like API is not accessible to the agent."

[FLAG] _resolve_env_reward gap when tool_trace is empty.
If only verify.env_reward is set and tool_trace is empty, the reward passes validation without cross-checking against any in-environment source. A verify_builder that synthesizes reward externally (violating "rewards inside environment") would not be caught here.

[FLAG] Namespace placement of an experimental module.
The README says the runtime is experimental "while RFC 005 remains in review" but openenv.core.harness sits in the stable package namespace. The repo already has src/openenv/experimental/. Consider placing this at openenv.experimental.harness until RFC 005 is approved.

---

Summary

• 5 mechanical issues to address
• 4 alignment points for human review, with RFC 005 pre-emption being the most significant

---

Automated review by Claude Code | Learn more

[burtenshaw]

Closing due to block.