Harbor integration: serve Harbor task datasets through OpenEnv as trainable environments by adithya-s-k · Pull Request #1036 · huggingface/OpenEnv

adithya-s-k · 2026-08-02T19:24:05Z

Closes #1035.

Serves Harbor's task datasets via OpenEnv for training compatibility.

Pick any Harbor dataset, pick any task in it, pick any supported agent harness, and pick a sandbox to run it on, and you get back a proper trainable rollout.

All you have to provide is the URL of a hosted vLLM started with token capture on (--return-tokens-as-token-ids --logprobs-mode processed_logprobs). Without those flags any OpenAI-spec endpoint still works, you just get evals rather than trainable rollouts.

The rollout comes back as the exact token ids and per-token logprobs of every model call the agent made, together with the task's own reward. Agent and sandbox are chosen per rollout rather than baked into the deployment, so one server covers the whole matrix.

The reason to consume Harbor rather than re-implement it: today each coding agent costs OpenEnv a whole environment package, and each package carries its own copy of an interception proxy that has to be correct about token ids. Harbor already decouples task, harness and sandbox behind one interface, with roughly 39 agents and 23 backends. One integration makes all of them trainable, and adding the next agent becomes a table entry rather than a package.

What a caller gets

Rewards alone do not train a policy. On-policy methods need, per turn, (prompt_token_ids, completion_token_ids, per_token_logps) plus the reward, and producing that tuple is what this PR is for. It cannot be reconstructed afterwards: re-rendering a prompt offline with apply_chat_template drifts from what the model actually saw, and a prompt off by a single token silently fragments one long conversation into several short ones. Capture has to happen on the wire, at rollout time.

The CLI

command	what it does
`openenv harbor info`	Reports what this machine can actually run: whether the LLM returns token ids, which sandbox backends have both working credentials and an importable SDK, which datasets resolve and how many tasks each holds, and which harnesses are validated. Read-only, boots nothing.
`openenv harbor rollout`	Runs rollouts with no env server involved. Boots the capture proxy, publishes it so a remote sandbox can reach it, runs `-n` tasks and writes the full token-level JSON. Also the debugging path: if `rollout` works and `serve` does not, the fault is in the serving layer and nothing below it.
`openenv harbor serve`	The env server: Task API for discovery, one long-running `run_rollout` MCP tool for execution, and a web UI. Refuses to start if the LLM cannot return token ids.
`openenv harbor push`	Deploys the same server to a Hugging Face Space. Configuration travels as Space variables, provider credentials as Space secrets, and `--dry-run` prints exactly what would be sent first.

openenv harbor info    --llm-url $LLM --dataset org/train,org/eval
openenv harbor rollout --llm-url $LLM --dataset org/train --task-index 0 -n 5 --harness codex --sandbox modal
openenv harbor serve   --llm-url $LLM --dataset org/train,org/eval
openenv harbor push    --llm-url $LLM --dataset org/train,org/eval --repo-id you/harbor-env

--llm-url is required and has no default and no environment fallback, because an unset endpoint produces rollouts that look completely normal and carry no token ids.

How capture works

An OpenAI-spec proxy sits between the agent and the inference endpoint. Each agent is pointed at it by a per-agent seam, usually one environment variable, and the agent's API key is really a capture session id, which is how one proxy serves many concurrent rollouts without per-rollout ports.

flowchart LR
  A["agent, in a Harbor sandbox"] -->|"base URL = proxy<br/>API key = session id"| P["capture proxy"]
  P -->|"detect dialect, normalise to chat,<br/>force token ids and logprobs on"| E["vLLM"]
  E -->|"prompt_token_ids,<br/>sampled ids + logprobs"| P
  P -->|"replay in the agent's own dialect<br/>(SSE if it asked for SSE)"| A
  P --> G["rollout graph"]

Two properties make this general rather than per-agent. Nothing is tokenised locally: the engine tokenises each prompt in order to serve it and hands back prompt_token_ids, so turn k+1's prompt is by construction the canonical tokenisation of everything before it, tool results included. And four wire dialects are supported, because coding agents did not converge on one: chat-completions, OpenAI Responses, Anthropic Messages and Google generateContent.

The rollout graph

Turns are not appended to a list, they are linked by exact token prefix: a call whose prompt_token_ids begin with an existing node's full token sequence becomes that node's child. Nothing else is consulted, no request ids, no timestamps, no conversation headers, because those are per-agent and the prefix is not.

flowchart TD
  R["root: system + first user turn"] --> T1["turn 1"]
  T1 --> T2["turn 2"]
  T2 --> T3["turn 3"]
  T2 -.->|"same prefix, branch died"| T3b["turn 3', a retry"]
  S["second root: subagent,<br/>different system prompt"] --> S1["turn 1"]

That falls out into the structure a trainer needs. A root is a conversation that started fresh, so several roots mean the agent ran subagents or auxiliary calls rather than one long chain. A fork is a retry or resample. A path from root to leaf is one training sequence, on which every token is either a prompt token the model conditioned on or a sampled token with its logprob. Branches that led nowhere are marked discarded and excluded from paths while staying visible in the report.

Inspiration

Neither half of this is novel and we did not treat it as such. The dialect translation is adapted from the Polar gateway (Apache-2.0), which had already solved converting Anthropic, Responses and Google requests into chat-completions calls faithfully enough to replay in the original dialect; it is vendored into dialects/ rather than depended on because the package named polar on PyPI is unrelated, with provenance in dialects/README.md. Polar's engine and proxy layers are not vendored, since they target SGLang, which cannot return token ids at all (sgl-project/sglang#18378), so a ~160-line vLLM-only upstream.py replaces them.

verifiers solves the same problem from the other direction, and we referred to how its Dialect ABC handles two cases that are easy to get wrong: auxiliary routes, so a call like claude-code's count_tokens is answered without becoming a model turn, and per-dialect streaming detection, since Google signals streaming in the URL rather than the body.

Agents supported today

16 harnesses are validated end to end, grouped by the dialect they speak:

dialect	agents
chat-completions	`opencode`, `goose`, `qwen-coder`, `swe-agent`, `mini-swe-agent`, `openhands-sdk`, `openclaw`, `hermes`, `kimi-cli`, `pi`, `vibe`, `terminus-2`
OpenAI Responses	`codex`, `trae-agent`
Anthropic Messages	`claude-code`
Google generateContent	`gemini-cli`

Supporting all four dialects rather than chat-completions alone is what buys the last four rows. terminus-2 runs host-side in the server process, so it needs no public URL. Anything else Harbor supports can be reached with --harness module:Class, and adding it properly means one entry in the seam table.

Validation against ATIF

Capture is checked against Harbor's own trace format, ATIF, which the harness writes independently of anything here. Reconciliation compares the two call by call: turn count, per-call completion token counts, and which calls the harness considers real agent steps rather than auxiliary. A rollout comes back as atif="match", "MISMATCH" or "none" when the harness emits no trajectory.

This matters because it is the only check that is not self-referential. The proxy could be internally consistent and still wrong, and a mismatch has already caught a real bug: one harness sending an empty tools array got a 400 from vLLM, which truncated its trajectory while leaving a graph that looked perfectly well-formed. Calls that ATIF marks auxiliary are also demoted so they cannot be credited with the reward earned by solving the task.

Validation runs on ingest rather than export, because a turn whose logprobs are misaligned has to be caught while we still know which turn it was.

Sandboxing

All of it is Harbor's. This PR adds no sandbox code, no provider SDK imports and no image building: a TrialConfig names an environment type and Harbor does the rest, which is what makes 23 backends available instead of the two someone would have hand-written.

Worth stating because the words collide: every OpenEnv provider (local_docker, hf_sandbox, modal, aca, daytona, uv) is a ContainerProvider that hosts the env server and has no exec. Harbor's backends are the agent-exec sandboxes, and --sandbox refers to those. Availability is asked for rather than assumed: a backend counts as usable only if its class imports and Harbor's preflight() passes, since a provider with valid credentials but no SDK installed otherwise reports available and fails at rollout time.

Reward

Harbor's verifier produces a dict[str, float] and OpenEnv wants a scalar. The dict travels verbatim and the scalar is chosen by an explicit rule: one key, or one named reward, otherwise fail and require --reward-key. Combining keys automatically would be inventing reward semantics, and shaping belongs to the trainer.

reward=None is not zero. It means the verifier never ran, and conflating the two makes a dead sandbox look like a wrong answer.

Real-time updates

A rollout takes minutes, so it can be watched while it runs. The capture proxy exposes GET /sessions with per-session turn count, root count and seconds since the last model call, updated as calls land, and the UI streams the same numbers before rendering the finished graph and the per-turn token ids and logprobs behind an accordion. It also separates the two ways a rollout can look stuck: no session yet means the sandbox is still booting, while a session with zero turns means the agent is installed but has not called the model.

The failure model

A failed rollout returns a result, never an exception: HarborRolloutResult(ok=False, reward=None, error=...). This is the architectural reason the layer exists. In the in-process predecessor a rollout exception reached the trainer and hung every rank at the NCCL barrier forever, which is why trl.experimental.harbor runs to ~400 lines with nearly every environment call individually wrapped in try/except. Behind an HTTP boundary that failure class cannot occur, and eval and training collapse onto one code path so hardening applies to both.

Structure

path
`src/openenv/core/harness/capture/`	Dialect-agnostic capture: proxy, rollout graph, ingest validation, LLM certification, port forwarding. No Harbor knowledge.
`src/openenv/harbor/`	Harbor specifics: seams, task discovery, rollout, capabilities, serving, UI, client.
`envs/harbor_env/`	Deployment packaging only: manifest, Dockerfile, ASGI entry point.
`src/openenv/cli/commands/harbor.py`	`info` / `rollout` / `serve` / `push`.

Capture lives in core because it is the piece every future agent environment would otherwise duplicate, and openenv.harbor sits alongside core/cli/auto rather than inside envs/ so it can be shared. Keeping it all in envs/harbor_env would be a smaller diff and would forfeit exactly the reuse this is for.

Two ports locally: the env server faces trainers and browsers, the capture proxy faces the sandbox and is the only one published. A single port would expose the env server as soon as the proxy became reachable. Hosted, that inverts. A Space has one port and one URL, so the proxy is mounted on the env server's own app at /capture and reached at <space-url>/capture, with nothing forwarded. The Space has to be public for that to work, since a private one requires an auth header the agent inside the sandbox does not send. Public is safe here because the proxy rejects any caller without a registered session id, so the mount is not an open relay.

Note

High Risk
Large new training and deployment surface: a publicly reachable capture proxy, many third-party sandboxes and credentials, and correctness of token/logprob contracts directly affects RL—mitigated by probing, eval-only downgrade, and structured failure results rather than trainer exceptions.

Overview
Adds a Harbor-backed OpenEnv environment so one server can run many Harbor datasets, agent harnesses, and sandbox backends per rollout and return task rewards plus full traces—and, against vLLM/SGLang with the right flags, trainable (prompt_token_ids, completion_token_ids, per_token_logps) per model call.

The reusable piece is openenv.core.harness.capture: an OpenAI-spec capture proxy (session id as API key), four dialect translators (chat, Responses, Anthropic, Google), a prefix-linked rollout graph, LLM startup probes (token ids, processed logprobs, tool calling), and provider 400 compat fixes with explicit warnings when behavior changes. openenv.harbor wires Harbor tasks, agent seams, ATIF cross-check, serving, and a client; envs/harbor_env is Docker/Space packaging plus a TRL loop-owning HarborSessionFactory that maps results to TraceEntry and avoids shared MCP clients across concurrent rollouts. Context-local os.environ overlays (proc_env_context) let credential-by-env harnesses run concurrently without a global lock.

CLI: openenv harbor info | rollout | serve | push (optional openenv[harbor], Python 3.12+). push bundles source when needed, syncs datasets via HF buckets/volumes, prunes stale Space files, and documents public Spaces for /capture. Docs, .gradio/ gitignore, and scripts/logprob_parity.py for logprob alignment checks round out the change.

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

An OpenAI-spec proxy that sits between a coding agent and an inference endpoint and records the exact token ids and per-token logprobs of every model call, so a rollout is trainable. Nothing is tokenised locally: the engine returns prompt_token_ids, so turn k+1's prompt is the canonical tokenisation of everything before it and turns link by exact token prefix. Re-rendering a prompt offline drifts from what the model saw, and a drifted prompt silently fragments one conversation into several. Four wire dialects (chat-completions, OpenAI Responses, Anthropic Messages, Google generateContent), adapted from the Polar gateway (Apache-2.0); provenance in dialects/README.md. Vendored because the package named polar on PyPI is unrelated. Two ideas are borrowed from verifiers: aux routes, so a count_tokens call is answered without becoming a model turn, and per-dialect streaming detection, since Google signals streaming in the URL. Includes engine certification, which refuses an endpoint that cannot return token ids, and port forwarding for sandboxes that cannot reach localhost.

Serves Harbor's task datasets over the Task API and runs a rollout through one long-running MCP tool, with the agent and the sandbox chosen per call rather than baked into the deployment. A failed rollout returns a result, never an exception. That is the reason this layer exists: in the in-process predecessor a rollout exception reached the trainer and hung every rank at the NCCL barrier, which is why trl.experimental.harbor wraps nearly every environment call individually. Behind an HTTP boundary that failure class cannot occur. Rewards are forwarded, never recomputed. Harbor's dict travels verbatim and the scalar is chosen by an explicit rule, refusing rather than guessing when several keys exist. reward=None is not zero: it means the verifier never ran, and conflating them makes a dead sandbox look like a wrong answer. Sandbox availability is asked for rather than assumed. A backend counts as usable only if its class imports, its SDK is present, and Harbor's own preflight passes; checking credentials alone reports a backend available and then fails at rollout time. Hosted deployments mount the capture proxy on the env server's own app, since a Space has one port and one public URL and nothing needs forwarding there.

info reports what this machine can actually run. rollout runs one end to end with no server involved, which halves the search space when something breaks: if rollout works and serve does not, the fault is in the serving layer. serve is the env server; push deploys the same thing to a Space. --llm-url is required with no default and no environment fallback, because an unset endpoint produces rollouts that look completely normal and carry no token ids. push attaches the task suites as a bucket volume mounted at /data instead of downloading them: a Harbor suite is thousands of small files and Space disk is ephemeral, so a download is re-paid on every restart. Copies are server side, by xet hash. The mount is verified before the server is pointed at it, and it falls back to downloading rather than reading paths that may not exist. The harbor extra installs every sandbox backend. Not harbor[cloud], which is unsatisfiable: it pulls langsmith[sandbox] and tensorlake, which demand incompatible websockets ranges.

Manifest, Dockerfile and ASGI entry point only; the logic lives in openenv.harbor so the capture layer can be shared rather than duplicated per agent environment. The Dockerfile pins UV_PYTHON_INSTALL_DIR and copies it across the stage boundary. Harbor needs Python >= 3.12 while openenv-base ships 3.11, so uv downloads its own interpreter and the venv's bin/python is a symlink into it; copying only .venv leaves a dangling link and the container dies with 'not found'. A build-time assertion now catches that at build rather than at startup. The entry point resolves and validates the served model the way harbor serve does. Without it the proxy has no served model id and forwards whatever name the harness used straight to the engine.

Each of these pins a failure that was silent in production and cheap to reintroduce. No credentials or network needed. Port ownership: a capture server used to report healthy on a port another process owned, because the liveness probe connected to the incumbent while its own bind error died unobserved on a background thread. Sessions were then minted in one registry and rejected by another, producing a 401 and a rollout with zero model calls. Request normalisation: kimi-cli sends tools: [] once its loop has no tools left, and vLLM rejects an empty array outright, truncating the trajectory while leaving a well-formed graph behind. Hosted serving: a Space must mount the capture proxy rather than forward it. One test monkeypatches make_forwarder to raise, so a hosted deployment that ever tries to forward fails the suite.

bot-ci-comment · 2026-08-02T19:25:42Z

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

check-env-docs generates a docs stub per environment README and fails when one is missing, which it was. The README line about the capture proxy being the only thing forwarded publicly predated the hosted path and was wrong for a Space, where there is one port and one public URL and the proxy is mounted rather than forwarded. Fixed in the README so the generated stub follows. _toctree.yml is maintained by hand, so the generated page needs an entry there or it exists without being reachable from the sidebar.

Copilot

🟡 Not ready to approve

There are correctness issues in the capture/training contract plumbing (async asyncio.run usage, incomplete per-turn prompt IDs, and dropping additional agent roots) that would cause silent data loss or runtime failures in valid usage paths.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds a Harbor-backed environment integration that can serve Harbor task datasets through OpenEnv and produce trainable rollouts by capturing engine-native token IDs + per-token logprobs (via a multi-dialect capture proxy), plus Harbor verifier rewards.

Changes:

Introduces openenv.core.harness.capture: a capture proxy + rollout-graph + validation/export utilities (incl. dialect adapters and port-forwarding).
Adds openenv.harbor package: dataset discovery, capabilities/preflight reporting, rollout runner, serving layer (including hosted “single-port” mounting behavior), typed client, and ATIF reconciliation.
Adds deployable envs/harbor_env packaging (Space/FastAPI entrypoint + Dockerfile) and wires a new openenv harbor CLI group + openenv[harbor] extra.

File summaries

File	Description
tests/envs/test_harbor_hosted_serving.py	Pins single-port hosted/Space behavior (mount capture; never forward).
tests/envs/test_harbor_capture_server.py	Pins capture server port-ownership + instance-identity invariants.
tests/envs/test_harbor_capture_normalise.py	Asserts request normalization that avoids vLLM 400s that silently truncate rollouts.
src/openenv/harbor/tasks.py	Implements dataset spec resolution (HF repo/local/registry) with caching + prefetch.
src/openenv/harbor/startup.py	Startup gating/preflight (LLM capture capability, sandboxes, datasets) with report rendering.
src/openenv/harbor/serving.py	Serving layer that chooses between forwarding (local) vs mount-at-`/capture` (hosted).
src/openenv/harbor/runner.py	CLI rollout runner: boot capture + forwarder, run tasks, print batch reports.
src/openenv/harbor/rollout.py	Core rollout execution and “never raise” result shaping, plus ATIF reconciliation integration.
src/openenv/harbor/models.py	Wire models (`HarborRolloutResult`, `HarborTurn`, etc.) and document-to-wire transformations.
src/openenv/harbor/environment.py	MCPEnvironment wrapper exposing `run_rollout` + discovery tools and Task API duck-typing.
src/openenv/harbor/client.py	Typed client for Task API + MCP tool execution with long timeouts.
src/openenv/harbor/capabilities.py	Capability discovery for harnesses/sandboxes/datasets, using Harbor preflight.
src/openenv/harbor/atif.py	ATIF ingest + reconciliation, and optional merge of captured tokens/logprobs into ATIF.
src/openenv/harbor/init.py	Package overview and dependency/layering notes.
src/openenv/core/harness/capture/validate.py	Validation logic for per-turn/per-sequence/per-rollout invariants.
src/openenv/core/harness/capture/validate_llm.py	Live probe to certify LLM returns token IDs + logprobs required for capture.
src/openenv/core/harness/capture/upstream.py	vLLM-only upstream client + request/response normalization.
src/openenv/core/harness/capture/sse.py	Synthetic SSE replay: capture non-streaming, respond streaming for harness compatibility.
src/openenv/core/harness/capture/sessions.py	Session multiplexing/routing (API key == session id) and session summaries.
src/openenv/core/harness/capture/graph.py	Prefix-linked rollout graph + training-sequence flattening.
src/openenv/core/harness/capture/forwarding.py	Port forwarder strategies (direct/gradio/cloudflare) with preflight + reliability constraints.
src/openenv/core/harness/capture/export.py	Export graph to validated JSON training document + role assignment.
src/openenv/core/harness/capture/dialects/reasoning.py	Reasoning/thinking block round-trip helpers used by dialect transformers.
src/openenv/core/harness/capture/dialects/README.md	Provenance + transformer scope/notes for vendored dialect code.
src/openenv/core/harness/capture/dialects/openai_chat.py	Chat-completions transformer shim.
src/openenv/core/harness/capture/dialects/images.py	Multimodal/image block conversions across dialects.
src/openenv/core/harness/capture/dialects/base.py	Base transformer + request normalization helpers (developer role merge, per-model fixes).
src/openenv/core/harness/capture/dialects/init.py	Transformer dispatch manager by detected API dialect.
src/openenv/core/harness/capture/detection.py	Dialect detection logic (path/header/body heuristics).
src/openenv/core/harness/capture/contract.py	Adapter layer exporting capture to downstream consumer “contracts” (TRL, per-turn records).
src/openenv/core/harness/capture/init.py	Public exports for the capture subsystem.
src/openenv/cli/main.py	Adds `openenv harbor` Typer subcommand group.
pyproject.toml	Adds `openenv[harbor]` optional extra with Python>=3.12 marker.
envs/harbor_env/server/Dockerfile	Space/deployment image build (uv + Python 3.12 carry-through) and runtime entrypoint.
envs/harbor_env/server/app.py	ASGI app that validates LLM, starts/mounts capture, and builds the env server app.
envs/harbor_env/server/init.py	Package marker for deployed server module.
envs/harbor_env/README.md	Environment-level usage + config docs for Space deployment.
envs/harbor_env/pyproject.toml	Environment packaging deps (openenv + harbor extras + server deps).
envs/harbor_env/openenv.yaml	OpenEnv deployment manifest for the harbor_env Space runtime.
envs/harbor_env/models.py	Re-export wire types for `harbor_env.*` parity with other env packages.
envs/harbor_env/client.py	Re-export typed client for environment package ergonomics.
envs/harbor_env/init.py	Env package overview + re-exports.
.gitignore	Ignores Gradio UI build artifacts.

Review details

Files reviewed: 51/53 changed files
Comments generated: 4
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot

🟡 Not ready to approve

The current per-turn export populates prompt_token_ids only for the first turn (breaking the stated training contract), and there are a couple of concrete operational/error-message issues that should be corrected before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

src/openenv/harbor/capabilities.py:168

The missing-SDK hint tells users to install harbor[cloud], but this PR explicitly documents that harbor[cloud] is unsatisfiable (see envs/harbor_env/pyproject.toml and root extra rationale). This message will send operators down a dead-end and hide the real remediation.
src/openenv/harbor/models.py:242
turns_from_document only includes prompt_token_ids for the very first emitted turn (if index == 0 else []). This contradicts the stated training contract (“per turn (prompt_token_ids, completion_token_ids, per_token_logps)”) and causes every later turn in contract.json to have an empty prompt, making exact prompt-token fidelity impossible for multi-turn rollouts.
src/openenv/harbor/tasks.py:165
This download path is meant to avoid HF's symlink-based snapshot layout (so Harbor's tar uploads don't preserve dangling symlinks), but snapshot_download can still create symlinks depending on huggingface_hub settings/version. Setting local_dir_use_symlinks=False makes the “real files” guarantee explicit and future-proof.
envs/harbor_env/server/app.py:77
_service.start() can create background resources (capture server thread and/or external forwarder subprocess) when this module is run outside Spaces. Because startup happens at import time and there is no shutdown hook, those resources may leak until process exit (and named forwards can persist even longer). Register a shutdown handler so the service is always torn down cleanly.

# Resolve capture before the app is built. A Space gives no separate boot hook, the UI needs the
# proxy's public URL to exist by the time anyone presses Run, and `build_app` has to see the service
# in order to mount it.
if _LLM_URL:
    _service = HarborService(

Files reviewed: 51/53 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Takes harbor coverage from 19 tests to 112, no credentials or network needed. The areas chosen are the ones that fail silently rather than loudly: a bug in any of them produces plausible training data instead of an error. graph prefix linking, roots, forks, discarded branches, loss masking rewards the explicit selection rule, including 0.0 vs None seams model-name normalisation, session threading, dialect coverage discovery ordering stability, the symlink regression, spec classification validation ingest checks and sandbox SDK detection rendering result models, verdict states, contract.json Two real bugs surfaced while writing them. Google streaming requests were misclassified. `detect` tested `"generateContent" in path`, but the streaming variant capitalises the G, so every `:streamGenerateContent` call fell through to chat-completions and would have been parsed by the wrong transformer. `wants_stream` already lowercased the path; `detect` did not. gemini-cli passed the sweep because it used the non-streaming route. Anthropic tool calls were absent from results. `models._tool_calls` read only the chat-completions `tool_calls` key, so claude-code's `tool_use` content blocks never reached `HarborTurn`, leaving `contract.json` and the rendered conversation showing an agent that produced text and took no actions. Two expectations of mine were wrong rather than the code, and are now pinned as behaviour: an empty served model raises instead of returning an empty string, and a turn that sampled nothing warns rather than invalidating the rollout.

Copilot

🟡 Not ready to approve

The current implementation has a few concrete contract/API mismatches (notably capture contract node selection and HarborEnv export/docs consistency) plus a misleading install hint that should be corrected before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (5)

src/openenv/core/harness/capture/contract.py:43

_agent_nodes() only keeps nodes from the first role == "agent" sequence/root. This contradicts export._assign_roles()’s documented behavior that multiple agent roots are normal (e.g. harnesses that rewrite prompts mid-run) and will silently drop valid agent turns from to_turn_records() / to_trace_entries() output.

def _agent_nodes(graph: RolloutGraph, document: dict[str, Any]) -> list[TurnNode]:
    """Nodes on the agent's conversation, in arrival order, excluding discarded retries."""
    agent_rows = [r for r in document["sequences"] if r["role"] == "agent"]
    if not agent_rows:
        return []
    root = agent_rows[0]["root_id"]
    keep = set(agent_rows[0]["node_ids"])
    return [
        n
        for n in graph.nodes()
        if graph.root_of(n.node_id) == root and n.node_id in keep
    ]

src/openenv/harbor/capabilities.py:168

The missing-SDK hint recommends pip install 'harbor[cloud]', but the repo’s own dependency comments state harbor[cloud] is unsatisfiable (see root pyproject.toml harbor extra). This message will send users toward an install path that can’t work.
src/openenv/harbor/models.py:257
turns_from_document() only populates prompt_token_ids for index == 0 and leaves it empty for later turns. That conflicts with the stated training contract (“per turn -> (prompt_token_ids, completion_token_ids, per_token_logps)”) and the HarborTurn docstring implying prompt_token_ids is defined for each turn.
envs/harbor_env/init.py:9
The package docs/examples use from harbor_env import HarborEnv, but harbor_env/__init__.py doesn’t export HarborEnv (it only exports models). Either the docs are wrong or this module should re-export the client like other env packages (e.g. opencode_env).

from openenv.harbor.models import HarborRolloutResult, HarborTaskRef, HarborTurn

__all__ = ["HarborRolloutResult", "HarborTaskRef", "HarborTurn"]

src/openenv/harbor/environment.py:28

SUPPORTS_CONCURRENT_SESSIONS = True makes this environment explicitly support multiplexed concurrent trajectories on one server instance. This appears to conflict with the documented design principle “One env = one trajectory” (PRINCIPLES.md), so it would be good to confirm this is an intentional exception for harbor_env (and that downstream trainers/collectors won’t assume 1:1 env↔trajectory).

Files reviewed: 56/58 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

`_PROC_ENV_LOCK` guards `os.environ` while an agent is constructed, because Harbor's wrappers read credentials there rather than from the config. It was an `asyncio.Lock`, which binds to the first event loop that uses it and then raises "is bound to a different event loop" for every other one. Rollouts arrive on several loops. The env server answers each request on its own, and any caller using `asyncio.run` per rollout creates another. So the first concurrent rollout succeeded and the rest failed instantly, with zero model calls and no useful error. It passed every test and every sequential run, and only appeared under real concurrency: 96 of 98 rollouts failed within seconds of the first parallel sweep. `threading.Lock` is the right primitive: the resource is global to the process, not to a loop. It is a blocking acquire inside an async function, which is acceptable only because construction does no I/O worth speaking of, the sandbox is booted later by `trial.run()` outside the lock. The regression test drives the lock from eight event loops at once, which is the shape that failed.

Copilot

🟡 Not ready to approve

The current rollout contract output drops required per-turn prompt token ids and also truncates multi-root agent sequences, which can silently break training data correctness.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (5)

src/openenv/harbor/capabilities.py:168

The missing-SDK hint recommends installing harbor[cloud], but this PR explicitly documents harbor[cloud] as unsatisfiable due to dependency conflicts. Point users to openenv[harbor] (or backend-specific extras) to avoid sending them to an installation dead end.
src/openenv/core/harness/capture/contract.py:38
_agent_nodes only keeps the first agent sequence/root. This drops additional agent roots (e.g. harnesses that rewrite system prompts mid-run), contradicting the capture layer’s own stance that multiple agent roots can be legitimate agent work and should remain trainable.

    agent_rows = [r for r in document["sequences"] if r["role"] == "agent"]
    if not agent_rows:
        return []
    root = agent_rows[0]["root_id"]
    keep = set(agent_rows[0]["node_ids"])

src/openenv/harbor/models.py:252

turns_from_document only includes prompt_token_ids for the first turn; later turns get []. Since _write_contract() serializes prompt_token_ids per turn, this produces contract files with missing prompt token ids for multi-turn rollouts, violating the stated training tuple contract.
envs/harbor_env/README.md:28
This doc claims the server refuses to start when the LLM cannot return token ids, but the Space ASGI entry point (envs/harbor_env/server/app.py) intentionally boots even when LLM validation fails (to surface the error in the UI/capabilities). The docs should reflect this hosted vs CLI behavior difference.

| `MODAL_TOKEN_ID`, `MODAL_TOKEN_SECRET` | offer the `modal` sandbox |

docs/source/environments/harbor.md:28

This environment doc says the server refuses to start if the LLM lacks token-id capture, but the Space entry point is designed to boot and report llm.ok=false so the UI can show the fault. Align the docs with the hosted behavior (or change the Space entry point to hard-fail).

Without them it answers every request normally and returns no token ids, so captured rollouts are
empty and nothing reports an error. The server refuses to start rather than let that happen.

Files reviewed: 56/58 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

`ok` is what a trainer filters on, so it has to mean "this row is usable". It did not. Only trace reconciliation could clear it, while the capture document's own validation findings were recorded in `findings` and otherwise ignored. A 98-rollout parallel sweep surfaced the consequence: four rollouts came back `ok=True` with zero model calls and zero trainable tokens, because reconciliation agreed with the capture when both sides were empty. One of them carried reward=1.0, which is the worst available shape, a row with nothing in it and a positive reward attached. Any FATAL from document validation now clears `ok` and becomes the error, so "the intercept saw no model calls" is reported as a failed rollout rather than a successful empty one.

Copilot

🟡 Not ready to approve

Several concrete correctness/operability issues were found in the changed code paths (per-turn prompt ids missing in turn rows, unsafe asyncio.run usage, brittle top_logprobs handling, misleading install guidance, and a capture-server lifecycle leak on forwarder failures).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

src/openenv/harbor/capabilities.py:168

The missing-SDK hint currently recommends pip install 'harbor[cloud]', but this PR’s own pyproject.toml notes that harbor[cloud] is unsatisfiable due to conflicting websockets constraints. This message will send users toward an install that cannot succeed.
src/openenv/harbor/serving.py:104
If make_forwarder(...) or forwarder.start(...) fails, the capture server has already been started and will be left running. That leaks a listener/port and can make subsequent starts fail with “already in use”.
envs/harbor_env/init.py:9
Docs/examples import HarborEnv via from harbor_env import HarborEnv, but harbor_env/__init__.py doesn’t export it (only models). This makes the quickstart import fail for users.

from openenv.harbor.models import HarborRolloutResult, HarborTaskRef, HarborTurn

__all__ = ["HarborRolloutResult", "HarborTaskRef", "HarborTurn"]

Files reviewed: 56/58 changed files
Comments generated: 3
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

`serve_harbor` passed `require_llm=True`, so `prepare` refused to boot without one. That was the last place coupling a server whose real cost is its dataset tree to the boot order of a vLLM that restarts every run. It now requires an engine only when one was given, and the default capture level with no engine is `text` rather than `tokens` — with nothing measured, the weakest tier is the only honest default, so a rollout that somehow reaches the default is never mistaken for a trainable one. `harbor rollout` still passes `require_llm=True` and still gets the old error: it runs a batch itself and has no session to take an engine from. The message now says which case it is talking about. Verified live, one server serving both DataAgent splits with no --llm-url: train-flagged vLLM -> capture_level=tokens rollout_type=train flagless vLLM -> capture_level=logprobs rollout_type=eval Same server, same session route, tier decided by probing the endpoint named on the request.

…e server's Found by running an engineless server end to end: every agent call came back `404 The model 'Qwen3.5-2B' does not exist`, and the rollout captured 0 turns while still reporting `rollout_type=train` — the tier was right and there was nothing in it. The proxy rewrites `model` because harnesses mangle it. opencode is configured with `intercepted/<model>` and its provider layer forwards only the last path segment, so an engine serving `Qwen/Qwen3.5-2B` is asked for `Qwen3.5-2B`. That rewrite is not cosmetic, it is what makes the call work at all. It read `app.state.model`, which is empty on a server booted without an engine, so the rewrite was skipped and the mangled name went straight upstream. Now it reads the session's engine and falls back to the server default. With the fix the same cell captures 51 turns, 47 trainable, 3578 trainable tokens, token ids present. The regression test drives a real request through the proxy and asserts on what the client was handed, because asserting on the response would have passed while the wrong name was still being sent.

cursor · 2026-08-20T09:47:39Z

+        """
+        if session is not None and session.upstream is not None and session.upstream.model:
+            return session.upstream.model
+        return app.state.model or ""


Session model fallback uses default engine

Medium Severity

_model_of still falls back to app.state.model whenever session.upstream.model is empty, even if the session already named a different engine. That rewrites model to the boot engine's id and sends it to the session engine, which 404s or hits the wrong weights. The rewrite needs to stay scoped to the session's own engine, and skip if that engine has no resolved name.

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

`run_rollout` had no timeout parameter, so the only bound on a hung rollout was the MCP client's socket timeout — 1800s, and it reports as a transport error rather than as anything that knew what it was waiting for. Observed live: an eval rollout produced no upstream traffic at all and sat for the full 30 minutes before the client gave up. The task file's `[agent] timeout_sec` does not cover this. It bounds the AGENT run; a sandbox that wedges during setup, before the agent starts, is outside it entirely. `agent_timeout_sec=0` keeps deferring to the task file, so nothing changes for existing callers. A trainer should set one: a rollout holds a generation slot for the length of the call, and with `num_generations` slots a single wedge stalls the step behind it.

cursor · 2026-08-20T10:12:29Z

+                # covers the AGENT run only — a sandbox that wedges during setup is outside it, which
+                # is how a rollout ran past 30 minutes and was killed by the client's socket timeout
+                # rather than by anything that knew what it was waiting for.
+                agent_timeout_sec=agent_timeout_sec or None,


Timeout misses sandbox setup hangs

Medium Severity

agent_timeout_sec is described as a hard ceiling on the whole rollout, including a wedged sandbox boot, but it is only passed through as Harbor’s override_timeout_sec. That field overrides the task’s agent timeout and does not cover environment build or setup. MCP step still defaults to _ROLLOUT_TIMEOUT_S (1800s), so a hung boot can still occupy a trainer slot until the socket timeout rather than the caller-supplied bound.

Additional Locations (2)

src/openenv/harbor/client.py#L121-L125

src/openenv/harbor/environment.py#L83-L85

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

…a hosted server `opencode_env` ships its own `OpenCodeSession`/`OpenCodeSessionFactory`, which is what the published AsyncGRPO example consumes. `harbor_env` had no equivalent, so training against a Harbor server meant writing a bespoke rollout loop. This is that equivalent: `create()` -> `wait_for_completion()` -> `fetch_proxy_trace()` -> `verify()`, the whole contract TRL's loop-owning path knows. Nothing is added to TRL, and the training script stays the stock one. The engine is an argument, so a trainer points the server at the vLLM it is currently syncing weights into and the tier follows from what that engine can return. Three behaviours the tests pin, because each would otherwise look like a working run: * an EVAL rollout yields NO trainable turns — not rows of zeros — and says why * a server error returns non-zero instead of raising, since an exception in the rollout loop takes down every training rank waiting on the next batch * an ungraded rollout reports `None`, never 0: a crashed rollout is not a wrong answer, and scoring it zero poisons the group baseline with a value nobody measured `measure_prompt_skew` ships alongside because `TraceEntry` carries no prompt token ids, so TRL re-renders each prompt. Completions are exact; prompts are re-rendered, and this measures the difference against the engine's own ids for the model and harness actually in use rather than assuming it is free or assuming it is fatal.

Bugbot caught all four, and the first two would have broken a training run while it still looked healthy. **Tool calls reached TRL flattened.** `HarborTurn.tool_calls` is `{name, arguments}` on purpose — a reward function checking which tool ran should not walk a wire envelope — but TRL reads `message["tool_calls"]` verbatim, and `has_tool_call` is `bool(turn.tool_calls)` against the nested OpenAI form. So `train_turn_fn=has_tool_call`, the documented default for a coding agent, discarded every turn of a rollout that is almost entirely tool calls. Verified against the real TRL predicate: the turn now comes back KEPT. **A live client crossed a process boundary.** Building the dataset calls `prompt_rows()` -> `tasks()` -> `_client()` in the parent, then TRL pickles the factory into its spawned rollout loop. `__getstate__` drops the client and keeps the task map, which is plain data the child needs anyway. **Duplicate instructions collapsed onto one task.** Hashing instruction text last-write-wins meant two identical instructions produced two dataset rows resolving to one index, so a group trained on a task it was never given and nothing said so. First occurrence now wins, and the shadowed count is logged. **A zero timeout was treated as unset.** `timeout_s or default` replaces the documented "defer to the task file" value of 0. `OpenCodeSession` uses `is not None` for exactly this reason. The seven new tests assert through TRL's own predicate where they can, because the twelve existing tests passed with all four bugs live.

`EnvClient.close` is synchronous and dispatches to `_close_async` through `_dispatch`, which returns an awaitable in async code and a result in sync code — one definition serving both `client.close()` and `await client.close()`. `MCPClientBase` overrode `close` itself with an `async def`, so every synchronous caller built a coroutine, dropped it un-awaited, and returned as if it had worked. Nothing errored, which is what made it expensive. The websocket stayed open, so the server never reached its `_destroy_session` cleanup and sessions accumulated until `max_concurrent_envs` was exhausted. Measured against a live server capped at 16: exactly 16 of 20 connect-and-close cycles succeeded and the rest failed as `ConnectionClosedOK`, which reads as a network fault rather than a leak. It killed 8 of 20 cells in a validation matrix before being understood, and a training run — one session per rollout — would have hit the same wall partway through. 30/30 now. Also forwards `max_message_size_mb`, which `EnvClient` has always accepted and `MCPClientBase` dropped: a tool returning a large result closed the connection with `1009 message too big` and no client could ask for more. A 262-turn rollout exceeded the 100 MB default.

`stats["n_trainable_tokens"]` sums `n_trainable` per SEQUENCE, and forked paths share their prefix, so a node reached by several sequences is counted once per sequence. `turns_from_document` deliberately emits each node once — a duplicated row is the same model call credited twice and quietly doubles its weight in a gradient — so the headline number and the turns next to it disagreed. Measured across saved rollouts, 7 of 15 affected: gemini-cli t0 reported 6845 present 6201 gemini-cli t17 reported 32950 present 31720 gemini-cli t2 reported 2166 present 1705 A consumer comparing the two fields has to find them consistent, so this now counts the deduped turns. The sequence-wise total stays in `stats` for a consumer training on `sequences`, where counting per sequence is the correct reading.

The UI validated a typed LLM URL and then said "Rollouts will not use this endpoint... restart it with --llm-url" — the old server-pinned model, left behind when the engine became a per-rollout argument. Someone could validate their vLLM, see token ids and logprobs confirmed, press Run, and get a rollout against a different engine entirely, with the UI telling them to restart a server that no longer needs restarting. `on_run` now resolves the typed endpoint through the capture server's upstream pool, so the tier comes from a real probe of that endpoint and the rollout goes there. The probe is cached, so pressing Run repeatedly costs nothing after the first time. With nothing validated it falls back to the server's default, and with neither the rollout reports the missing engine rather than silently producing something untrainable.

…ach a gated engine Two follow-ups on the previous two commits, both caught in review. **The token total was always zero.** `n_trainable_tokens` was computed beside the other stats, which run before `result.turns` is filled in from the document — so it summed an empty list. The ordering was visible in the patch I wrote and I did not act on it. It is now computed where `turns` exists, which is also the only place it can be. **Run dropped the engine credential.** The validated state carried `authenticated: bool` but not the key, so validating a token-gated endpoint succeeded and pressing Run then failed to authenticate against that same URL. The key now travels in `gr.State`, which is held server-side and never rendered back into the page — the same rule the API key box follows.

…ke TRL does The wire format keeps tool-call `arguments` as a JSON string, which is what the OpenAI schema says. XML-style chat templates — Qwen3.5's among them — iterate it, and iterating a string raises `Can only get item pairs from a mapping`, so the function crashed instead of measuring anything. TRL decodes them before rendering (`_decode_tool_call_arguments`), so skipping it also meant measuring something other than what TRL feeds the template. With it working, measured over 670 captured turns on Qwen3.5-4B with thinking disabled: goose, mini-swe-agent, opencode, qwen-coder, pi, swe-agent, openclaw, openhands-sdk, terminus-2 .......... 100% exact, delta 0 claude-code, gemini-cli ...................... 0%, +2 tokens kimi-cli ..................................... 9%, -10 tokens So the loop-owning path — which re-renders prompts because `TraceEntry` carries no prompt ids — is lossless for nine of twelve harnesses and lossy for three. That is worth knowing before choosing a harness to train with, and it is the number this function exists to produce. One trap worth recording: the template kwarg must be passed through as a direct kwarg, the way TRL splats it. Passing `chat_template_kwargs={...}` to `apply_chat_template` silently does nothing, thinking stays on, and every one of those nine harnesses measures 0% exact at -2 tokens — a wrong answer that looks exactly like a real capture problem.

Failed 5/5 across DataAgent tasks before the agent started: `exit 127` from `curl -fsSL .../NousResearch/hermes-agent/main/scripts/install.sh | bash`. Every attempt spent a sandbox and several minutes to learn nothing, and listing it as validated pointed people at it. The capture layer is what surfaced this, reporting "the intercept saw no model calls: the agent never reached it" rather than blaming capture — which is the diagnostic working as intended. Seam, `InterceptHermes` and its import are gone. A comment stays where the seam was, with the evidence, so it is not re-added without fixing the install first.

A HarborEnv holds a single websocket, and the MCP transport sends-then-receives on it with no request-id correlation. Sharing one across concurrent rollouts is therefore not slow, it is wrong: the first smoke run died with `cannot call recv while another coroutine is already running recv` on every rollout of every step, so all 8 came back unscorable and the starved socket then dropped on a keepalive timeout. So `create()` builds its own client and owns it. `new_client()` is the seam, kept separate from `_client()` because the factory's own connection is long-lived and only serves task metadata, which is sequential and safe to share. The existing tests asserted on the fake the factory holds, which sessions no longer use — hence `factory_with` patching `new_client` too. The pickling test builds its factory directly: a lambda bound on the instance is itself unpicklable, so going through the helper would have tested the helper.

… lock on cancellation Two shutdown paths that fail the same way — a port stays bound and the *next* start reports a port conflict, which says nothing about what actually broke. `HarborService.stop` released the capture port only if the forwarder shut down cleanly. A wedged tunnel process is exactly when you most need the port back, so the capture stop moves into a `finally` — the same unwind `start()` already does on the way up. `_PROC_ENV_LOCK` was taken via `asyncio.to_thread`, which is not cancellable: cancelling the await abandons the future while the worker thread runs on and still takes the lock, with no frame left to release it. One cancelled goose rollout would wedge every later one. `_acquire_proc_env` hands the release to whoever actually acquires. Also drops the hermes half of the install-fixes tests, orphaned when the seam was removed (hermes-agent fails to install, exit 127, 5/5).

The hermes cases were the only async ones in this file. Local `ruff check` passed on the paths I had in hand and CI's did not, which is the whole value of CI here.

Harbor has no step cap — only a timeout — and that is a training problem, not just a cost one. AsyncGRPO packs every turn of a rollout into ONE training row, and each turn re-sends the whole conversation, so packed length grows with the SQUARE of the turn count. A 58-turn rollout is an order of magnitude larger than a 17-turn one, and smoke run 46428 died on it: forward passed, then the chunked LM head's backward failed with CUBLAS_STATUS_ALLOC_FAILED — an OOM wearing a cuBLAS mask — while every rollout log line looked healthy. An earlier Qwen3-4B run micro-stepped to 451 turns and 10.1M prompt tokens for the same reason. How to express a cap is per-agent knowledge, so it lives on the Seam: `step_limit` maps a count to whatever that harness's config calls it (mini-swe-agent takes a YAML `agent.step_limit` through Harbor's `config` mapping). It threads through run_rollout, the MCP tool, HarborEnv and HarborSessionFactory, with 0 meaning unbounded, so nothing changes for existing callers. Two decisions worth naming. A cap a seam cannot express is WARNED about rather than dropped — silently ignoring it would let a caller believe its rollouts are bounded when they are not, and the symptom surfaces much later and somewhere else. And the merge is deep: a shallow update of `{"config": {...}}` would replace a seam's whole config block with the step limit alone, which is how opencode's provider block and its base_url would quietly disappear.

…the loss step The failure it names surfaces as CUBLAS_STATUS_ALLOC_FAILED inside the chunked LM head's backward — an OOM wearing a cuBLAS mask, in a frame that mentions neither the rollout nor its turn count. A log line, not a rejection: what to do about an oversized rollout is the trainer's call, and dropping it here would silently shrink a GRPO group.

…cting every task `indices=[]` arrives from a caller whose filter matched nothing. Truthiness treated that as 'no selection' and fell back to the whole split, so a run that believed it had picked a handful of tasks would quietly train on all 2238.

cursor · 2026-08-24T20:39:23Z

+                self._task_index,
+                tokens,
+                getattr(result, "n_turns", -1),
+            )


Oversized warning checks wrong metric

Medium Severity

_warn_if_oversized compares n_trainable_tokens, which is only the sum of completion tokens under the loss mask. The OOM it aims to flag comes from packed row length, where each turn re-includes the full prompt history and size grows with the square of turn count. Long micro-stepping rollouts can stay under the 250k completion-token threshold while still packing millions of prompt tokens, so the warning can miss the failure mode it documents.

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

Harbor names each template from the task (trial.py:628 passes task.short_name), so a suite whose tasks share one image builds thousands of identical templates. Measured on the DataAgent train suite: all 2238 Dockerfiles hash to ONE value, the environment directories are byte-identical, and the aliases Harbor produced differed only in their task prefix — 0000_555_555434_qa_3__016e9c9f617d and 0000_650_650548_qa_2__016e9c9f617d carry the same hash. The cost is not just build time. goes true the moment a build STARTS, so a GRPO group hitting a task for the first time races itself and the losers 404 with 'tag default does not exist'. One alias means one build, ever, and nothing to race. The env hash stays in the alias, which is what makes this safe: a task whose environment genuinely differs still gets its own template, so this collapses identical environments rather than forcing unlike ones together. Opt-in via HARBOR_SHARED_ENV_NAME — the variable an older Harbor honoured natively before the knob was dropped. Tests assert on the environment_name Harbor's own constructor RECEIVES, since that is what the alias is built from; asserting on the wrapper would only prove the wrapper ran.

…cal os.environ reads claude-code, gemini-cli and goose read os.environ inside run() to build the env dict they hand to the sandbox (goose.py:653, claude_code.py:1393, gemini_cli.py:824). Since the API key IS the rollout's session id — that is how one capture proxy multiplexes N rollouts — each concurrent rollout needs a DIFFERENT value of one variable at one instant, in one process. os.environ is process-global, so the only correct answer was _PROC_ENV_LOCK: those three ran one rollout at a time while the other twelve ran in parallel. The observation that removes the lock is that those wrappers only READ, and only to build a dict. They do not need the value globally visible, they need it visible to them, now — which is a context-local read. So os.environ is replaced by a mapping that consults a contextvars overlay first and the real environment second. Each rollout sets its own overlay, concurrent rollouts see different values from the same expression, nothing global is mutated, and the lock is gone. Two properties chosen deliberately. Iteration and copy() return the MERGED view, because subprocess builds a child's environment from os.environ and a proxy that hid the overlay would launch subprocesses with no credentials — a failure that would look like a bad key rather than a bad proxy. And writes go to the real environment, so only reads are context-local and libraries that set a variable expecting it to persist keep working. The lock remains as the fallback path, and OPENENV_CONCURRENT_PROC_ENV=0 selects it, because this swaps out a global the whole process reads and that is worth being able to switch off without a rollback. Known limit, worth stating: contextvars propagate through asyncio tasks and asyncio.to_thread, but NOT into a bare threading.Thread. If a wrapper ever moves its env read onto a raw thread, the overlay is invisible and it reads the real environment instead — goose raises ValueError on a missing key, so that degrades loudly rather than silently.

cursor

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

There are 4 total unresolved issues (including 3 from previous reviews).

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

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

cursor · 2026-08-25T12:36:23Z

+        self._base[key] = value
+
+    def __delitem__(self, key: str) -> None:
+        del self._base[key]


Overlay keys break delete and clear

High Severity

_ContextEnviron treats overlay-only keys as present for reads, membership, length, and iteration, but __delitem__ only touches the real environment. pop then raises even with a default, and MutableMapping.clear deletes every real variable before failing on an overlay-only key, which can empty the process environment while a rollout overlay is active.

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

sergiopaniego

thanks again for this great work! ready to merge. some agent review comments below again but feel free to merge when ready:

sergiopaniego · 2026-08-26T15:02:46Z

+    trials_dir = trials_dir or Path("/tmp/openenv-harbor-trials")
+    trials_dir.mkdir(parents=True, exist_ok=True)
+
+    capture = CaptureServer(


this is the same gap I flagged in august and it's still here: run_batch builds CaptureServer with no admin_key while expose defaults to "gradio", so _admin_ok (server.py:614) falls open over a public tunnel. GET /sessions, GET /sessions/{id}/rollout, DELETE /sessions/{id} and POST /sessions are all reachable unauthenticated, and since POST /sessions mints a key the proxy honours, it's an open relay to the vLLM.

what changed is that _admin_ok's own docstring now says "serve/push set it whenever the proxy is reachable from outside". serving.py:82 does exactly that. rollout is the one path that doesn't, so the threat model is already written down and this is the hole in it.

either pass an admin_key here the way serve does, or refuse a public expose without one.

sergiopaniego · 2026-08-26T15:02:46Z

+                Stop the agent after this many steps. `0` leaves it unbounded. Worth setting for
+                training: each turn re-sends the whole conversation, so a packed training row grows
+                with the square of the turn count.
+            agent_timeout_sec (`float`, *optional*):


this says the parameter is the ceiling that covers a wedged sandbox boot, but it's wired to AgentConfig.override_timeout_sec (rollout.py:231), which overrides the very [agent] timeout_sec the docstring says doesn't cover setup. so a hung boot is still unbounded, which is the 30-minute failure that motivated adding the knob.

same as the open bugbot thread on environment.py:292. either wrap _run in asyncio.wait_for, or reword so it doesn't promise a bound it doesn't give.

sergiopaniego · 2026-08-26T15:02:46Z

+    def __setitem__(self, key: str, value: str) -> None:
+        self._base[key] = value
+
+    def __delitem__(self, key: str) -> None:


bugbot is right here. reads see the overlay but __delitem__ only touches _base, so os.environ.pop("SOME_OVERLAY_KEY", default) raises KeyError even with a default: MutableMapping.pop resolves self[key] fine and then blows up on the del. clear() stops halfway for the same reason.

low practical impact, but it's a two-line fix and this object replaces a global the whole process reads.

separate question on the same module: contextvars propagate into asyncio tasks and asyncio.to_thread, but not into a bare ThreadPoolExecutor.submit. worth a line in the docstring saying which one harbor is actually on.

sergiopaniego · 2026-08-26T15:02:47Z

+
+| | |
+|---|---|
+| **16 harnesses** | validated end to end, across 4 wire dialects |


docs went stale when hermes was dropped in b8b90e9. importing the module today gives 15 validated of 29, docs still say "16 validated of 30 known" and the harness table still lists hermes. same in docs/source/environments/harbor.md (18, 213, 286, 397), dialects/README.md:8, rollout.py:509 ("three of the sixteen"), and the PR description's table. check-env-docs passes because it only diffs the README against its stub.

bigger version of the same thing: the harbor docs haven't changed at all since 5986b79, so the per-call engine, agent_step_limit, agent_timeout_sec, the whole HarborSessionFactory (658 lines, the TRL entry point) and three new env knobs (OPENENV_CONCURRENT_PROC_ENV, HARBOR_SHARED_ENV_NAME, OPENENV_CAPTURE_ADMIN_KEY) are undocumented. worth closing here rather than after merge, since it's the surface people will read first.

sergiopaniego · 2026-08-26T15:02:47Z

+            "OPENAI_API_BASE": "{base_url}/v1",
+            "OPENAI_BASE_URL": "{base_url}/v1",
+        },
+        step_limit=_mini_swe_agent_step_limit,


only mini-swe-agent can express a step limit, the other 14 log a warning and run unbounded. fine in itself, but HarborSessionFactory sells agent_step_limit as the mitigation for the quadratic packed row and defaults harness="opencode", which is one of the 14. worth saying so in the factory docstring.

adithya-s-k added 5 commits August 2, 2026 19:22

Copilot AI lite review requested due to automatic review settings August 2, 2026 19:24

Copilot started reviewing on behalf of adithya-s-k August 2, 2026 19:24 View session

cursor Bot reviewed Aug 2, 2026

View reviewed changes

Comment thread src/openenv/core/harness/capture/forwarding.py Outdated

cursor Bot reviewed Aug 2, 2026

View reviewed changes

Comment thread src/openenv/harbor/models.py Outdated

Comment thread src/openenv/core/harness/capture/server.py Outdated

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Comment thread src/openenv/harbor/tasks.py

Comment thread src/openenv/harbor/models.py Outdated

Comment thread src/openenv/core/harness/capture/contract.py Outdated

Comment thread src/openenv/harbor/capabilities.py

Copilot AI review requested due to automatic review settings August 2, 2026 19:29

Copilot started reviewing on behalf of adithya-s-k August 2, 2026 19:29 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 2, 2026 19:35

adithya-s-k force-pushed the harbor-integration branch from 39d65ac to c684348 Compare August 2, 2026 19:36

Copilot started reviewing on behalf of adithya-s-k August 2, 2026 19:36 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 2, 2026 19:46

Copilot started reviewing on behalf of adithya-s-k August 2, 2026 19:47 View session

cursor Bot reviewed Aug 2, 2026

View reviewed changes

Comment thread src/openenv/core/harness/capture/contract.py Outdated

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 2, 2026 20:08

Copilot started reviewing on behalf of adithya-s-k August 2, 2026 20:08 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Comment thread src/openenv/core/harness/capture/upstream.py Outdated

Comment thread src/openenv/harbor/environment.py Outdated

Comment thread src/openenv/harbor/models.py

Copilot AI review requested due to automatic review settings August 3, 2026 07:15

adithya-s-k added 2 commits August 20, 2026 09:20

cursor Bot reviewed Aug 20, 2026

View reviewed changes

adithya-s-k added 2 commits August 20, 2026 09:56

capture: formatting (usort + ruff), as CI runs it

3924da8

cursor Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread envs/harbor_env/harness.py

Comment thread envs/harbor_env/harness.py

Comment thread envs/harbor_env/harness.py Outdated

Comment thread envs/harbor_env/harness.py Outdated

adithya-s-k added 4 commits August 20, 2026 18:15

cursor Bot reviewed Aug 24, 2026

View reviewed changes

Comment thread src/openenv/harbor/rollout.py Outdated

Comment thread src/openenv/harbor/ui.py

adithya-s-k added 4 commits August 24, 2026 18:00

cursor Bot reviewed Aug 24, 2026

View reviewed changes

Comment thread envs/harbor_env/harness.py Outdated

adithya-s-k added 5 commits August 24, 2026 20:02

harbor tests: drop the asyncio import the hermes deletion orphaned

2854581

The hermes cases were the only async ones in this file. Local `ruff check` passed on the paths I had in hand and CI's did not, which is the whole value of CI here.

cursor Bot reviewed Aug 24, 2026

View reviewed changes

adithya-s-k added 2 commits August 25, 2026 11:09

cursor Bot reviewed Aug 25, 2026

View reviewed changes

sergiopaniego approved these changes Aug 26, 2026

View reviewed changes

This was referenced Aug 27, 2026

Add AsyncGRPO Harbor example: any harness, any sandbox, any Harbor dataset, served through OpenEnv huggingface/trl#6947

Open

Remove the experimental Harbor integration (superseded by OpenEnv) huggingface/trl#6948

Open

Uh oh!

Conversation

adithya-s-k commented Aug 2, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What a caller gets

The CLI

How capture works

The rollout graph

Inspiration

Agents supported today

Validation against ATIF

Sandboxing

Reward

Real-time updates

The failure model

Structure

Uh oh!

bot-ci-comment Bot commented Aug 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (4)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (5)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (5)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor Bot Aug 20, 2026

Choose a reason for hiding this comment

Session model fallback uses default engine

Uh oh!

cursor Bot Aug 20, 2026

Choose a reason for hiding this comment

Timeout misses sandbox setup hangs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor Bot Aug 24, 2026

Choose a reason for hiding this comment

Oversized warning checks wrong metric

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Aug 25, 2026

Choose a reason for hiding this comment

Overlay keys break delete and clear

Uh oh!

adithya-s-k commented Aug 2, 2026 •

edited by cursor Bot

Loading