Harbor integration: serve Harbor task datasets through OpenEnv as trainable environments - #1036
Harbor integration: serve Harbor task datasets through OpenEnv as trainable environments#1036adithya-s-k wants to merge 59 commits into
Conversation
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.
|
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.
There was a problem hiding this comment.
🟡 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.harborpackage: dataset discovery, capabilities/preflight reporting, rollout runner, serving layer (including hosted “single-port” mounting behavior), typed client, and ATIF reconciliation. - Adds deployable
envs/harbor_envpackaging (Space/FastAPI entrypoint + Dockerfile) and wires a newopenenv harborCLI 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.
There was a problem hiding this comment.
🟡 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 thatharbor[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_documentonly includesprompt_token_idsfor 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 incontract.jsonto 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_downloadcan still create symlinks depending on huggingface_hub settings/version. Settinglocal_dir_use_symlinks=Falsemakes 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.
39d65ac to
c684348
Compare
There was a problem hiding this comment.
🟡 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 firstrole == "agent"sequence/root. This contradictsexport._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 fromto_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 stateharbor[cloud]is unsatisfiable (see rootpyproject.tomlharbor extra). This message will send users toward an install path that can’t work.
src/openenv/harbor/models.py:257 turns_from_document()only populatesprompt_token_idsforindex == 0and 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 theHarborTurndocstring implyingprompt_token_idsis defined for each turn.
envs/harbor_env/init.py:9- The package docs/examples use
from harbor_env import HarborEnv, butharbor_env/__init__.pydoesn’t exportHarborEnv(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 = Truemakes 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 forharbor_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.
There was a problem hiding this comment.
🟡 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 documentsharbor[cloud]as unsatisfiable due to dependency conflicts. Point users toopenenv[harbor](or backend-specific extras) to avoid sending them to an installation dead end.
src/openenv/core/harness/capture/contract.py:38 _agent_nodesonly keeps the firstagentsequence/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_documentonly includesprompt_token_idsfor the first turn; later turns get[]. Since_write_contract()serializesprompt_token_idsper 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=falseso 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.
There was a problem hiding this comment.
🟡 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 ownpyproject.tomlnotes thatharbor[cloud]is unsatisfiable due to conflictingwebsocketsconstraints. This message will send users toward an install that cannot succeed.
src/openenv/harbor/serving.py:104 - If
make_forwarder(...)orforwarder.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
HarborEnvviafrom harbor_env import HarborEnv, butharbor_env/__init__.pydoesn’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.
The running offset was advanced as if each turn contributed only prompt-plus-sampled tokens, so from the second turn on it pointed at interstitial context: the aux call's real completion tokens stayed at mask 1 and the function silently did nothing. `n_prompt` is already the sequence-coordinate start of a turn's sampled span, so use it directly. Tests parametrise the aux node's position, because the first-turn case passes under either arithmetic.
The estimator read `messages` and `system` only, so a `:countTokens` from gemini-cli hit the `max(1, ...)` floor and answered 1 no matter how long the conversation was. An agent uses that figure to decide when to compact, so a constant 1 means it never does and overruns its real context window. Reads `contents`/`parts` and `systemInstruction` in both spellings the REST API and the SDK emit.
`run_batch` bound the capture port in a background thread and only then built the forwarder, outside any guard. A cloudflared that would not start left that thread up, so the next invocation died on a port conflict naming nothing about the real cause. Teardown had the mirror gap: a forwarder whose `stop()` raised took the port with it. Both now follow `HarborService.start`.
vLLM masks the top-p/top-k tail to -inf and takes the log-softmax after (v1/sample/ops/topk_topp_sampler.py:135 then :139), so under processed_logprobs a captured logprob is renormalised over the surviving set: log p_full(t) - log(kept_mass). A trainer recomputing over the full vocabulary gets log p_full(t), which puts GRPO's step-0 importance ratio at kept_mass instead of 1 — bounded by top_p, unbounded for top_k, and reordering rather than shifting for the penalties. An on-policy rollout has to be drawn from the distribution being trained, so at the `tokens` tier these knobs go to their no-op values; the eval tiers keep whatever the harness asked for. What was requested is still recorded per turn and now also stated as a finding, so the override is never silent.
`--top-p` samples at a given top_p while the rescore stays full-vocab, which is exactly the capture-versus-trainer comparison. Reports the MEAN SIGNED residual alongside the existing max: a truncation bias is systematic and one-directional, so the mean is what reveals it, while the max is what reveals a misalignment. Measured on Qwen3.5-4B, vLLM DP=1 with processed_logprobs: top_p=1.0 +0.000358 nats ratio 0.9996 (noise floor) top_p=0.95 +0.002682 nats ratio 0.9973 top_p=0.9 +0.005786 nats ratio 0.9942 top_p=0.8 +0.015446 nats ratio 0.9847 Monotonic, positive as predicted, and the max residual per level tracks the -log(kept_mass) bound. Through the proxy, where prepare_request neutralises it, the same top_p=0.8 conversation returns -0.000209 nats / ratio 1.0002 — back at the noise floor.
|
Re-reviewed: all previously identified P0/P1 blockers are addressed |
burtenshaw
left a comment
There was a problem hiding this comment.
Looks great. It would be good to follow this PR up with a higher-level review, to make sure we don't get too bloated:
- deprecation of extra envs like harness envs
- inclusion and deprecation of some examples to focus on this approach
- alignment with TRL on the best way to use OpenEnv+TRL
sergiopaniego
left a comment
There was a problem hiding this comment.
this is great, lgtm!
one thing worth addressing (here or as a follow-up, not blocking): on the harbor rollout path, run_batch builds CaptureServer without an admin_key while opening a public tunnel by default (expose="gradio"). serve sets admin_key from env, but the rollout path doesn't, so _admin_ok falls open and the session control-plane (GET /sessions, GET /sessions/{id}/rollout, DELETE /sessions/{id}, POST /sessions) ends up reachable unauthenticated over the public URL. and since POST /sessions mints a key the proxy then honors, it's effectively an open relay too
two clean options: wire admin_key into the runner the same way serve does, or refuse a public expose when no admin_key is set
it's a concrete slice of the broader "auth the exposed channel" thing we've been discussing, but this one is just a gap between the serve and rollout paths
…1036) The RFC argued for a capture layer OpenEnv did not have. huggingface#1036 builds it, so this rewrites the document as the rationale and contract spec for a component that now exists, plus what remains open. Four decisions are settled by the implementation, three against what the RFC previously argued: - D3: OpenEnv assembles training rows, reversing "recorder, not assembler". The dependency argument that motivated the split does not apply — assembly is prefix arithmetic over engine-returned ids, so no tokenizer is involved. "Post-hoc" is a claim about staging, not about which repository; Polar and verifiers both assemble inside themselves. - D4: corrected. Parent is matched on prompt_ids + sampled_ids, not prompt_ids alone, and calls form a graph rather than chains. - D17: superseded. Roles are assigned structurally from graph shape and cross-checked against the harness's own ATIF trajectory, replacing the proposed boundary heuristics, which huggingface#1036 tried and rejected. - D18: settled as sandbox-dials-out to a published trainer-side proxy, with measured forwarder reliability. This matches the topology @sergiopaniego described in review and answers the open question in both directions. Adds D19 (four wire dialects) and records the failure modes huggingface#1036 found that the RFC had not anticipated: processed_logprobs interacting with distribution-narrowing sampling knobs, and the silent-success class that motivates graded ingest validation. Also flags that huggingface#1036 forks Problem 1 rather than closing it — the repo would ship two interception proxies, and pi_env reads the older one off disk while declaring opencode_env only as a comment in its pyproject. Status: Draft -> In Review.
A capture server was pinned to one inference endpoint at boot: `create_app` required `llm_url`, built a single `InferenceClient`, and baked the probed `capture_level` into the app. That put the durable thing at the mercy of the ephemeral one. A dataset server is thousands of task files and prebuilt sandbox templates; the engine behind it restarts every training run, and a train-tier engine and an eval-tier one are usually both wanted against the same task suite. Neither was possible: no URL meant no proxy at all, and a second engine meant a second server. So `Upstream` moves onto the `Session`, together with the level that engine was MEASURED at. `UpstreamPool` keeps one client and one probe per `(url, model, auth_header)`, which is what makes it affordable — a GRPO group is N sessions on one engine and pays for the probe once. `POST /sessions` takes the engine and probes it before returning, so a caller learns its tier at submit time instead of when the token fields come back empty. `llm_url` is now optional. With no default engine the proxy still listens and publishes, and a session that names none gets a 503 that says so, rather than calls to an empty base URL surfacing as a connection fault. Booting with `--llm-url` is unchanged: it becomes the default. The tier is never assumed. An engine that cannot be probed is `text`, the weakest, because claiming `tokens` without evidence is the one failure capture levels exist to prevent.
…ndbox `run_rollout` already took `harness` and `sandbox` per call; the engine was the one thing it could not. It now takes `llm_url` (plus `model`/`api_key`/`auth_header`), resolves it through the capture server's upstream pool inside the coroutine — the probe is async and cached — and passes both the descriptor and the measured level down, so the result cannot claim a tier the engine was not measured at. `HarborService` and `serve_harbor` stop requiring an engine, and both now carry `max_output_tokens`. That cap was unreachable from the outside and defaults to 8192, which is exactly what opencode asks for per turn: the clamp existed, matched, and changed nothing, and the agent's first call could then exceed a small context window and 400 every time.
`HarborTurn` had the engine's `prompt_token_ids` but not the messages that produced them, so a client could not rebuild TRL's `TraceEntry` — which is what the loop-owning `HarnessRolloutWorker` reads. The token fields alone do not say what prompt was sent. It also makes retokenization skew measurable: re-render the request locally and compare against `prompt_token_ids` from the same turn. That is the only way to know whether a local re-render is lossless for a given model and harness rather than assuming it in either direction. The data was already on the document's nodes; this just stops dropping it at the boundary.
`harbor serve --llm-url` becomes optional and gains `--max-output-tokens`; the deployed app reads `OPENENV_MAX_OUTPUT_TOKENS` and starts its capture service unconditionally. Gating the service on `OPENENV_LLM_URL` was what made an engineless server useless: every rollout answered "server not initialised: no capture proxy is running". Tests cover the shape that matters: a server boots with no engine; naming one probes it and reports the tier; a weaker engine comes back `eval` from the same server; two engines coexist; the probe is cached per engine; a session with no engine and no default is told so; booting with an engine still works and does not probe.
| def stop(self) -> None: | ||
| if self._forwarder is not None: | ||
| self._forwarder.stop() | ||
| self.capture.stop() |
There was a problem hiding this comment.
Capture port leak on teardown
Medium Severity
HarborService.stop calls forwarder.stop() and then capture.stop() with no try/finally. If tunnel teardown raises, the capture thread keeps the bound port, so the next serve fails with a port conflict instead of the original error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1483c5d. Configure here.
| trial = await Trial.create(config) | ||
| trial_result = await trial.run() | ||
| finally: | ||
| _PROC_ENV_LOCK.release() |
There was a problem hiding this comment.
Proc-env lock can leak
Medium Severity
The process-env lock is acquired with await asyncio.to_thread(_PROC_ENV_LOCK.acquire) before the try/finally that releases it. If that await is cancelled, the worker can still take the lock after the task is gone, and every later goose/claude-code-style rollout then blocks forever.
Reviewed by Cursor Bugbot for commit 1483c5d. Configure here.
`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.
| """ | ||
| if session is not None and session.upstream is not None and session.upstream.model: | ||
| return session.upstream.model | ||
| return app.state.model or "" |
There was a problem hiding this comment.
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.
| # 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, |
There was a problem hiding this comment.
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)
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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
There are 8 total unresolved issues (including 4 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 33b57cc. Configure here.
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.


Closes #1035.
Serves Harbor's task datasets via OpenEnv for training compatibility.
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 withapply_chat_templatedrifts 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
openenv harbor infoopenenv harbor rollout-ntasks and writes the full token-level JSON. Also the debugging path: ifrolloutworks andservedoes not, the fault is in the serving layer and nothing below it.openenv harbor serverun_rolloutMCP tool for execution, and a web UI. Refuses to start if the LLM cannot return token ids.openenv harbor push--dry-runprints exactly what would be sent first.--llm-urlis 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.
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 GooglegenerateContent.The rollout graph
Turns are not appended to a list, they are linked by exact token prefix: a call whose
prompt_token_idsbegin 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.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 namedpolaron PyPI is unrelated, with provenance indialects/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-onlyupstream.pyreplaces them.verifiers solves the same problem from the other direction, and we referred to how its
DialectABC handles two cases that are easy to get wrong: auxiliary routes, so a call like claude-code'scount_tokensis 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:
opencode,goose,qwen-coder,swe-agent,mini-swe-agent,openhands-sdk,openclaw,hermes,kimi-cli,pi,vibe,terminus-2codex,trae-agentclaude-codegemini-cliSupporting all four dialects rather than chat-completions alone is what buys the last four rows.
terminus-2runs 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
toolsarray 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
TrialConfignames 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 aContainerProviderthat hosts the env server and has noexec. Harbor's backends are the agent-exec sandboxes, and--sandboxrefers to those. Availability is asked for rather than assumed: a backend counts as usable only if its class imports and Harbor'spreflight()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 namedreward, otherwise fail and require--reward-key. Combining keys automatically would be inventing reward semantics, and shaping belongs to the trainer.reward=Noneis 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 /sessionswith 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 whytrl.experimental.harborruns to ~400 lines with nearly every environment call individually wrapped intry/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
src/openenv/core/harness/capture/src/openenv/harbor/envs/harbor_env/src/openenv/cli/commands/harbor.pyinfo/rollout/serve/push.Capture lives in
corebecause it is the piece every future agent environment would otherwise duplicate, andopenenv.harborsits alongsidecore/cli/autorather than insideenvs/so it can be shared. Keeping it all inenvs/harbor_envwould 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
/captureand 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 serving/proxy surface that forwards LLM traffic, handles API keys/session auth, and produces training contracts. Bugs in capture, dialect translation, or train/eval probing can silently poison RL data or expose a public proxy.
Overview
Adds a Harbor environment so one OpenEnv server can run any Harbor dataset, harness, and sandbox per rollout, returning verifier rewards plus engine-native
prompt_token_ids/completion_token_ids/per_token_logpswhen the LLM supports them.A new capture layer in
openenv.core.harness.capturesits between black-box coding agents and the model: four wire dialects (chat, Responses, Anthropic, Google), session-id API keys, token-prefix graph stitching, and startup probes that distinguish train vs eval (including processed vs raw logprobs). Hosted-provider 400s are retried with recorded param fixes.CLI:
openenv harbor info|rollout|serve|push.envs/harbor_envpackages a Space-deployable server (proxy at/capture). TRL loop-owning sessions live inHarborSessionFactory. Extraopenenv[harbor](Python ≥3.12).Reviewed by Cursor Bugbot for commit e5d15fb. Bugbot is set up for automated code reviews on this repo. Configure here.