Skip to content

Harbor integration: serve Harbor task datasets through OpenEnv as trainable environments - #1036

Open
adithya-s-k wants to merge 59 commits into
huggingface:mainfrom
adithya-s-k:harbor-integration
Open

Harbor integration: serve Harbor task datasets through OpenEnv as trainable environments#1036
adithya-s-k wants to merge 59 commits into
huggingface:mainfrom
adithya-s-k:harbor-integration

Conversation

@adithya-s-k

@adithya-s-k adithya-s-k commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Closes #1035.

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

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

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

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

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

What a caller gets

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

The CLI

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

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

How capture works

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

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

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

The rollout graph

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

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

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

Inspiration

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

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

Agents supported today

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

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

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

Validation against ATIF

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

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

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

Sandboxing

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

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

Reward

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

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

Real-time updates

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

The failure model

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

Structure

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

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

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


Note

High Risk
Large new 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_logps when the LLM supports them.

A new capture layer in openenv.core.harness.capture sits 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_env packages a Space-deployable server (proxy at /capture). TRL loop-owning sessions live in HarborSessionFactory. Extra openenv[harbor] (Python ≥3.12).

Reviewed by Cursor Bugbot for commit e5d15fb. Bugbot is set up for automated code reviews on this repo. Configure here.

An OpenAI-spec proxy that sits between a coding agent and an inference
endpoint and records the exact token ids and per-token logprobs of every
model call, so a rollout is trainable.

Nothing is tokenised locally: the engine returns prompt_token_ids, so turn
k+1's prompt is the canonical tokenisation of everything before it and turns
link by exact token prefix. Re-rendering a prompt offline drifts from what
the model saw, and a drifted prompt silently fragments one conversation into
several.

Four wire dialects (chat-completions, OpenAI Responses, Anthropic Messages,
Google generateContent), adapted from the Polar gateway (Apache-2.0);
provenance in dialects/README.md. Vendored because the package named polar
on PyPI is unrelated. Two ideas are borrowed from verifiers: aux routes, so
a count_tokens call is answered without becoming a model turn, and
per-dialect streaming detection, since Google signals streaming in the URL.

Includes engine certification, which refuses an endpoint that cannot return
token ids, and port forwarding for sandboxes that cannot reach localhost.
Serves Harbor's task datasets over the Task API and runs a rollout through
one long-running MCP tool, with the agent and the sandbox chosen per call
rather than baked into the deployment.

A failed rollout returns a result, never an exception. That is the reason
this layer exists: in the in-process predecessor a rollout exception reached
the trainer and hung every rank at the NCCL barrier, which is why
trl.experimental.harbor wraps nearly every environment call individually.
Behind an HTTP boundary that failure class cannot occur.

Rewards are forwarded, never recomputed. Harbor's dict travels verbatim and
the scalar is chosen by an explicit rule, refusing rather than guessing when
several keys exist. reward=None is not zero: it means the verifier never ran,
and conflating them makes a dead sandbox look like a wrong answer.

Sandbox availability is asked for rather than assumed. A backend counts as
usable only if its class imports, its SDK is present, and Harbor's own
preflight passes; checking credentials alone reports a backend available and
then fails at rollout time.

Hosted deployments mount the capture proxy on the env server's own app, since
a Space has one port and one public URL and nothing needs forwarding there.
info reports what this machine can actually run. rollout runs one end to end
with no server involved, which halves the search space when something breaks:
if rollout works and serve does not, the fault is in the serving layer.
serve is the env server; push deploys the same thing to a Space.

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

push attaches the task suites as a bucket volume mounted at /data instead of
downloading them: a Harbor suite is thousands of small files and Space disk is
ephemeral, so a download is re-paid on every restart. Copies are server side,
by xet hash. The mount is verified before the server is pointed at it, and it
falls back to downloading rather than reading paths that may not exist.

The harbor extra installs every sandbox backend. Not harbor[cloud], which is
unsatisfiable: it pulls langsmith[sandbox] and tensorlake, which demand
incompatible websockets ranges.
Manifest, Dockerfile and ASGI entry point only; the logic lives in
openenv.harbor so the capture layer can be shared rather than duplicated per
agent environment.

The Dockerfile pins UV_PYTHON_INSTALL_DIR and copies it across the stage
boundary. Harbor needs Python >= 3.12 while openenv-base ships 3.11, so uv
downloads its own interpreter and the venv's bin/python is a symlink into it;
copying only .venv leaves a dangling link and the container dies with
'not found'. A build-time assertion now catches that at build rather than at
startup.

The entry point resolves and validates the served model the way harbor serve
does. Without it the proxy has no served model id and forwards whatever name
the harness used straight to the engine.
Each of these pins a failure that was silent in production and cheap to
reintroduce. No credentials or network needed.

Port ownership: a capture server used to report healthy on a port another
process owned, because the liveness probe connected to the incumbent while its
own bind error died unobserved on a background thread. Sessions were then
minted in one registry and rejected by another, producing a 401 and a rollout
with zero model calls.

Request normalisation: kimi-cli sends tools: [] once its loop has no tools
left, and vLLM rejects an empty array outright, truncating the trajectory
while leaving a well-formed graph behind.

Hosted serving: a Space must mount the capture proxy rather than forward it.
One test monkeypatches make_forwarder to raise, so a hosted deployment that
ever tries to forward fails the suite.
Copilot AI lite review requested due to automatic review settings August 2, 2026 19:24
@bot-ci-comment

bot-ci-comment Bot commented Aug 2, 2026

Copy link
Copy Markdown

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.
Comment thread src/openenv/core/harness/capture/forwarding.py Outdated
Comment thread src/openenv/harbor/models.py Outdated
Comment thread src/openenv/core/harness/capture/server.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

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

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

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

Pull request overview

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

Changes:

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

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

Comment thread src/openenv/harbor/tasks.py
Comment thread src/openenv/harbor/models.py Outdated
Comment thread src/openenv/core/harness/capture/contract.py Outdated
Comment thread src/openenv/harbor/capabilities.py
Copilot AI review requested due to automatic review settings August 2, 2026 19:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

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

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

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

Review details

Suppressed comments (4)

src/openenv/harbor/capabilities.py:168

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

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

Copilot AI review requested due to automatic review settings August 2, 2026 19:35
Takes harbor coverage from 19 tests to 112, no credentials or network needed.
The areas chosen are the ones that fail silently rather than loudly: a bug in
any of them produces plausible training data instead of an error.

  graph        prefix linking, roots, forks, discarded branches, loss masking
  rewards      the explicit selection rule, including 0.0 vs None
  seams        model-name normalisation, session threading, dialect coverage
  discovery    ordering stability, the symlink regression, spec classification
  validation   ingest checks and sandbox SDK detection
  rendering    result models, verdict states, contract.json

Two real bugs surfaced while writing them.

Google streaming requests were misclassified. `detect` tested
`"generateContent" in path`, but the streaming variant capitalises the G, so
every `:streamGenerateContent` call fell through to chat-completions and would
have been parsed by the wrong transformer. `wants_stream` already lowercased
the path; `detect` did not. gemini-cli passed the sweep because it used the
non-streaming route.

Anthropic tool calls were absent from results. `models._tool_calls` read only
the chat-completions `tool_calls` key, so claude-code's `tool_use` content
blocks never reached `HarborTurn`, leaving `contract.json` and the rendered
conversation showing an agent that produced text and took no actions.

Two expectations of mine were wrong rather than the code, and are now pinned
as behaviour: an empty served model raises instead of returning an empty
string, and a turn that sampled nothing warns rather than invalidating the
rollout.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

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

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

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

Review details

Suppressed comments (5)

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

  • _agent_nodes() only keeps nodes from the first role == "agent" sequence/root. This contradicts export._assign_roles()’s documented behavior that multiple agent roots are normal (e.g. harnesses that rewrite prompts mid-run) and will silently drop valid agent turns from to_turn_records() / to_trace_entries() output.
def _agent_nodes(graph: RolloutGraph, document: dict[str, Any]) -> list[TurnNode]:
    """Nodes on the agent's conversation, in arrival order, excluding discarded retries."""
    agent_rows = [r for r in document["sequences"] if r["role"] == "agent"]
    if not agent_rows:
        return []
    root = agent_rows[0]["root_id"]
    keep = set(agent_rows[0]["node_ids"])
    return [
        n
        for n in graph.nodes()
        if graph.root_of(n.node_id) == root and n.node_id in keep
    ]

src/openenv/harbor/capabilities.py:168

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

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

src/openenv/harbor/environment.py:28

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

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

`_PROC_ENV_LOCK` guards `os.environ` while an agent is constructed, because
Harbor's wrappers read credentials there rather than from the config. It was an
`asyncio.Lock`, which binds to the first event loop that uses it and then raises
"is bound to a different event loop" for every other one.

Rollouts arrive on several loops. The env server answers each request on its
own, and any caller using `asyncio.run` per rollout creates another. So the
first concurrent rollout succeeded and the rest failed instantly, with zero
model calls and no useful error.

It passed every test and every sequential run, and only appeared under real
concurrency: 96 of 98 rollouts failed within seconds of the first parallel
sweep.

`threading.Lock` is the right primitive: the resource is global to the process,
not to a loop. It is a blocking acquire inside an async function, which is
acceptable only because construction does no I/O worth speaking of, the sandbox
is booted later by `trial.run()` outside the lock.

The regression test drives the lock from eight event loops at once, which is
the shape that failed.
Copilot AI review requested due to automatic review settings August 2, 2026 19:46
Comment thread src/openenv/core/harness/capture/contract.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

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

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

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

Review details

Suppressed comments (5)

src/openenv/harbor/capabilities.py:168

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

src/openenv/harbor/models.py:252

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

docs/source/environments/harbor.md:28

  • This environment doc says the server refuses to start if the LLM lacks token-id capture, but the Space entry point is designed to boot and report llm.ok=false so the UI can show the fault. Align the docs with the hosted behavior (or change the Space entry point to hard-fail).
Without them it answers every request normally and returns no token ids, so captured rollouts are
empty and nothing reports an error. The server refuses to start rather than let that happen.
  • Files reviewed: 56/58 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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

`ok` is what a trainer filters on, so it has to mean "this row is usable". It
did not. Only trace reconciliation could clear it, while the capture document's
own validation findings were recorded in `findings` and otherwise ignored.

A 98-rollout parallel sweep surfaced the consequence: four rollouts came back
`ok=True` with zero model calls and zero trainable tokens, because
reconciliation agreed with the capture when both sides were empty. One of them
carried reward=1.0, which is the worst available shape, a row with nothing in it
and a positive reward attached.

Any FATAL from document validation now clears `ok` and becomes the error, so
"the intercept saw no model calls" is reported as a failed rollout rather than a
successful empty one.
Copilot AI review requested due to automatic review settings August 2, 2026 20:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

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

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

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

Review details

Suppressed comments (3)

src/openenv/harbor/capabilities.py:168

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

__all__ = ["HarborRolloutResult", "HarborTaskRef", "HarborTurn"]
  • Files reviewed: 56/58 changed files
  • Comments generated: 3
  • Review effort level: Lite

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

Comment thread src/openenv/core/harness/capture/upstream.py Outdated
Comment thread src/openenv/harbor/environment.py Outdated
Comment thread src/openenv/harbor/models.py
Copilot AI review requested due to automatic review settings August 3, 2026 07:15
Comment thread src/openenv/harbor/rollout.py Outdated
Comment thread src/openenv/core/harness/capture/server.py
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.
Copilot AI review requested due to automatic review settings August 6, 2026 14:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

`--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.
@burtenshaw

burtenshaw commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Re-reviewed: all previously identified P0/P1 blockers are addressed

@burtenshaw burtenshaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cc @sergiopaniego

@sergiopaniego sergiopaniego left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rycerzes added a commit to rycerzes/OpenEnv that referenced this pull request Aug 12, 2026
…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.
rycerzes added a commit to rycerzes/OpenEnv that referenced this pull request Aug 12, 2026
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()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1483c5d. Configure here.

trial = await Trial.create(config)
trial_result = await trial.run()
finally:
_PROC_ENV_LOCK.release()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

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 ""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Fix All in Cursor

❌ 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.

Comment thread envs/harbor_env/harness.py
Comment thread envs/harbor_env/harness.py
Comment thread envs/harbor_env/harness.py Outdated
Comment thread envs/harbor_env/harness.py Outdated
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenEnv × Harbor: make multi-harness agentic training the easy path

4 participants