Harbor integration: serve Harbor task datasets through OpenEnv as trainable environments - #1036
Merged
Merged
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
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.
Contributor
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.
adithya-s-k
force-pushed
the
harbor-integration
branch
from
August 2, 2026 19:36
39d65ac to
c684348
Compare
Contributor
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.
Contributor
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.
Contributor
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.
… `reward` Two docstring corrections, both found by running the contract against a live vLLM rather than a stub. contract.py's module docstring still instructed consumers to re-render the prompt with apply_chat_template, because `to_trace_entries` "has no field for the prompt's token ids". It has had that field since the previous commit. Leaving the old advice in place is how the original bug propagated -- a consumer reads the docstring, not the TypedDict -- so the text now records what the re-render actually cost (0/6 turns exact on Qwen3.5, 0/28 on -4B where the enable_thinking default is inverted) and states plainly that nothing re-renders any more. TraceEntry declares `reward` and nothing in capture emits it, which is correct but was nowhere written down: a proxy sees model calls, not task outcomes, and inventing a number would be worse than omitting one. An environment fills it in from its own verify(). The TypedDict is total=False precisely so a producer can omit what it cannot know, so consumers must read it with .get() -- indexing raises on every entry this module produces, which is exactly how this was noticed.
… something Found by running a real opencode rollout in a real sandbox, which the stub tests could not catch. The budget reply went back as a plain JSON body regardless of what the caller asked for. opencode streams. It got a non-SSE response on an SSE request, read it as a failed generation, and retried -- so the proxy answered the stop again, and again. One rollout logged "hit its model-call budget (10)" 281 times for a single session while it burned its entire timeout. The log said the cap was working the whole time. Two fixes. The check moves after `client_wants_stream` so the reply can go back through `sse.replay` in the dialect the caller used, exactly as the normal path does. And the message is no longer empty: an empty assistant message also reads as a failed generation and gets retried, so the content now states the budget is spent. It is deliberately about the BUDGET and not about the task, because it lands in the harness's own transcript and capture never records it -- it must not look like something the model chose to say about the work. Two tests, and both were confirmed to fail against the old behaviour before being kept: one asserts a streaming caller gets `text/event-stream` with a terminal `finish_reason` in the body, the other that the message is non-empty. Verified live afterwards: the same rollout now logs exactly ONE budget stop, the agent terminates, and 8 trainable turns come back with 1573 completion tokens whose prefixes chain exactly.
The repo's lint gate runs `usort format` and then `ruff format` and fails on any resulting diff, so running ruff alone locally is not enough to predict CI -- that misdiagnosis has cost a cycle before. Cosmetic only: one blank line and some test-file wrapping.
# Conflicts: # src/openenv/cli/__main__.py
… 40 concurrent
`run_rollout` was registered as a SYNC `@mcp.tool`. FastMCP dispatches a sync tool body through
`anyio.to_thread.run_sync` with no limiter (`fastmcp/utilities/async_utils.py:26-34`), which falls
back to anyio's default `CapacityLimiter(40)` (`anyio/_backends/_asyncio.py:3097`) — and the body
held that worker thread for the whole rollout. So the server admitted at most ~40 rollouts per
uvicorn process no matter what `MAX_CONCURRENT_ENVS` said; we had been running sweeps at 400.
It was invisible because `rollout.py:300` starts the clock INSIDE the tool body, after admission,
so queue time never entered `wall_s`. The throughput table we were reading off those runs
(48 -> 14.2, 96 -> 28.4, 150 -> 44.4 rollouts/min) is the identity `concurrency / mean_duration`
evaluated, not observed.
Measured, through the real dispatch chain (step_async -> _async_handle_call_tool ->
_async_call_tool -> mcp_session -> client.call_tool -> FunctionTool.run), 120 requests at a 1s hold:
sync PEAK=40 elapsed=3.15s (exactly ceil(120/40) waves)
async PEAK=120 elapsed=1.18s
Reproductions in experiments/openenv_x_harbor_learnings/tools/ — hermetic, no sandboxes or GPU.
This also closes a sandbox leak. `Trial._finalize` -> `_stop_agent_environment` asyncio.SHIELDS
`agent_environment.stop(delete=...)`, and `run_async_safely` ran each rollout under its own
`asyncio.run`, which closes the loop the moment the coroutine returns and cancels that shielded
teardown. Any cancelled or timed-out rollout leaked a paid E2B sandbox — which is why this has to
land before any rollout deadline, not after.
365 harbor+capture tests pass. The post-run capture block stays on the loop deliberately:
export_session measures 1.6-17.2 ms even for a 50-turn / 2.4M-token rollout, so moving it to a
thread executor is not justified; a loop-lag probe in the eval sweep will catch it if that is wrong.
…iscard the rest
`result.rewards` was built by a dict comprehension inside a single try, so the first key that could
not be coerced to a finite float threw and the handler discarded EVERY key and failed the whole
rollout. A suite emitting `tool_efficiency: null` alongside a perfectly good `correctness` therefore
lost the correctness too.
Measured on Qwen3.5-4B via opencode against HuggingEnvs/data-agent-harbor-test: of 250 tasks, the 86
whose test.sh invokes `grader.py --json` failed with "2 validation errors for VerifierResult"
whenever the agent actually produced an answer. Only the empty-submission branch of that grader
hardcodes floats, so the suite graded FAILURES correctly and crashed on every success. Graded rate
went from 1% to 97% once the reward key was also selectable per task.
Now each key is coerced on its own: usable keys are kept, unusable ones are dropped and NAMED in
`findings`, and the rollout only fails if the key actually being trained on is the unusable one --
which `_pick_reward` decides immediately below. An unmeasured key is an EXCLUSION, never a zero, so
it is dropped rather than coerced to 0.0.
Verified: {'correctness': 1.0, 'tool_efficiency': None} -> rewards {'correctness': 1.0}, picked
(1.0, 'correctness'), finding records the drop; forcing reward_key='tool_efficiency' still raises.
362 harbor/capture tests pass; usort+ruff clean; env docs in sync.
…ready # Conflicts: # src/openenv/core/mcp_client.py
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 39b36a5. Configure here.
scosman
added a commit
to scosman/OpenEnv
that referenced
this pull request
Sep 16, 2026
The `/ws` and `/mcp` endpoints close their socket in a `finally` block, which normally runs after the client has already hung up: uvicorn raises `ClientDisconnected` there and Starlette re-raises it as `WebSocketDisconnect`, so the `except RuntimeError` guard those blocks originally carried never matched and every clean teardown escaped to the ASGI layer as a logged traceback. Both guards already catch `WebSocketDisconnect` as of huggingface#1036. This pins that behaviour: each endpoint is driven against an ASGI peer whose closing handshake fails with `OSError` — what uvicorn does once the peer is gone — and the handler must return normally. Narrowing either guard back to `except RuntimeError` fails both cases. Fixes huggingface#1159 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcQ9a8DBfzEXxXCeiKG72z
Contributor
There was a problem hiding this comment.
Post-merge release review (release manager). This is now on main at 34825a77 and is therefore in the 0.5.0 candidate (#1190). One finding is holding publication; I am not opening a fix PR, so this needs an owner.
Release-gating: run_batch() defaults to expose="gradio", which publishes the capture proxy through a public tunnel, but it builds CaptureServer(...) with no admin_key. With the key unset, _admin_ok() returns True unconditionally (src/openenv/core/harness/capture/server.py:698), so GET /sessions, GET /sessions/{id}/rollout, GET /sessions/{id}/trace_entries, DELETE /sessions/{id}, and POST /sessions are all reachable without credentials on that public URL. That is precisely the enumeration, token-level trace disclosure, session deletion, and key-minting relay that _admin_ok's own docstring says the key exists to prevent. HarborService gets this right (src/openenv/harbor/serving.py:83 mints or reads OPENENV_CAPTURE_ADMIN_KEY); run_batch and the standalone python -m openenv.core.harness.capture.server CLI, which has no --admin-key flag, do not.
Both openenv.harbor and openenv.core.harness.capture are picked up by [tool.setuptools.packages.find], so this ships to PyPI users, which is why it blocks the release rather than just the Hub deployment.
Not release-gating, but a real bug: _ContextEnviron.__delitem__ deletes only from the base mapping, so pop("OVERLAY_ONLY_KEY", default) raises KeyError despite the default, and clear() empties real process-environment keys while leaving overlay keys visible. Verified by running it against this tree.
For the release I need one of: a fix that threads an admin key (or a loopback-only bind) through the public run_batch and CLI paths, a revert of this PR from the candidate, or an explicit accepted-risk decision recorded on #1190. The RFC 011 path-contract concern from earlier reviews is genuinely resolved by #1172 and is no longer a blocker.
Sent by Cursor Automation: Release
| trials_dir = trials_dir or Path("/tmp/openenv-harbor-trials") | ||
| trials_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| capture = CaptureServer( |
Contributor
There was a problem hiding this comment.
admin_key is not passed here, and expose defaults to "gradio" (line 36), so a few lines below forwarder.start(port) puts this proxy on a public URL. Because app.state.admin_key is then None, _admin_ok() short-circuits to True and every session-management route is open to anyone with the tunnel URL.
The contrast with HarborService.start (src/openenv/harbor/serving.py:83), which mints or reads OPENENV_CAPTURE_ADMIN_KEY, is what makes this look like an oversight rather than an intentional trade-off: the docstring on _admin_ok already states the key must be set "whenever the proxy is reachable from outside", and this path makes it reachable from outside by default.
| def __setitem__(self, key: str, value: str) -> None: | ||
| self._base[key] = value | ||
|
|
||
| def __delitem__(self, key: str) -> None: |
Contributor
There was a problem hiding this comment.
Deleting straight from self._base ignores overlay-only keys, and since this class inherits MutableMapping, both pop() and clear() route through here. Two consequences, both reproduced against this tree:
pop("OVERLAY_ONLY_KEY", "default")raisesKeyErroreven though a default was supplied, because the read resolves through the overlay but the delete hits a missing base key.clear()removes the real base keys and leaves the overlay keys still visible, so the mapping is neither empty nor intact and the process environment has been partially emptied.
Deleting from the overlay first and only falling through to the base when the key is not an overlay key would fix both.
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Harbor agents need a shared OpenEnv service for evaluating hosted models and collecting exact-token training rollouts. This adds the task/service/client integration, capture proxy and default Gradio playground used by the data-agent runs.
RFC 012 extends RFCs 005/006 and PR #941. Qualification is versioned evidence for tested combinations; it does not promise support for arbitrary models or endpoints. The proxy buffers responses, so traces update after each call. Training-row weighting stays with the trainer. Agent execution and sandbox lifecycle have separate timeout bounds.
Validation: final CI passes on Python 3.11/3.12, including package, lint, docs and lock checks. Local validation includes 2,393 CPU tests (unrelated QED services excluded), focused capture/session-budget tests, and 334 client/discovery/Harbor regressions after the final upstream merge. Desktop/mobile browser checks found no JavaScript errors or horizontal overflow; concurrent trace ownership is covered. The qualification report records exact passing subsets of 29 adapters × four provider profiles × two tasks. Local whole-repository lint reported 28 unchanged upstream formatting findings; changed files and CI are clean.
Related: huggingface/trl#6947 consumes the contract; adithya-s-k/HuggingEnvs#7 contains reproduction and deployed-Space GPU smoke receipts. Active training jobs and original artifacts were preserved.