refactor agent sandbox infrastructure with new agent backends + rework logprobs capture - #694
refactor agent sandbox infrastructure with new agent backends + rework logprobs capture#694rycerzes wants to merge 35 commits into
Conversation
- related tests
- fix agent handling
- tests
meta-cla
Bot
added
the
CLA Signed
Greptile SummaryThis PR refactors the hardwired OpenCode environment into a shared
Confidence Score: 2/5Not safe to merge as-is — the server layer imports client code through the package init, violating a hard architectural boundary, and there is a real correctness race between setup commands and the running agent. The server indirectly pulls in client.py via coding_agent_env/init.py every time CodingAgentEnvironment initializes, crossing a boundary the repo treats as non-negotiable. The _exec_with_retry bug means install retries never fire for the most common failure mode (non-empty stderr), so cold sandbox installs will fail non-deterministically in CI. The setup/agent race is explicitly noted as a known compromise but affects any task that writes input files or installs dependencies before the agent's first model call. envs/coding_agent_env/server/coding_environment.py (client-server violation + setup race), envs/coding_agent_env/init.py (root init imports client), src/openenv/core/harness/agents/cli_driver.py (retry logic bug) Important Files Changed
Sequence DiagramsequenceDiagram
participant Orch as Orchestrator
participant Env as CodingAgentEnvironment
participant Factory as SessionFactory
participant Driver as CLIAgentDriver
participant SB as SandboxBackend (E2B/Docker/HF)
participant Proxy as InterceptionProxy
Orch->>Env: run_rollout(agent, instruction, setup, verify)
Env->>Factory: create(task)
Factory->>Driver: create_session(task, config)
Driver->>SB: create(timeout_s)
SB-->>Driver: SandboxHandle
Driver->>Driver: _bootstrap_sandbox
alt "mode == transparent_proxy"
Driver->>SB: start_bg(interception.py)
SB-->>Driver: proxy BgJob
end
Driver->>SB: start_bg(agent CLI)
SB-->>Driver: agent BgJob
Driver-->>Factory: CLIAgentSession
Factory-->>Env: session (agent already running)
Note over Env: setup commands run HERE — races with agent
loop setup commands
Env->>SB: exec(cmd)
end
Env->>Factory: wait_for_completion()
loop verify commands
Env->>SB: exec(cmd)
end
Env->>SB: kill()
Env-->>Orch: RolloutResult (JSON)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…roxy the transparent proxy was a passive forwarder that captured logprobs by injecting logprobs=true into upstream requests. It is replaced by the interception_gate mode where the trainer owns the forward pass entirely — no proxy needed inside the sandbox.
…eneration
InterceptionServer (aiohttp) runs on the trainer host. Each rollout
registers a queue. The agent's OPENAI_BASE_URL points at
`{base_url}/rollout/{id}/v1`. When the agent makes an LLM call it
blocks at the server. The training loop dequeues the request, calls
vLLM with logprobs=True and return_token_ids=True, and delivers the
response back via deliver_response().
There was a problem hiding this comment.
Reworked logprob capture
- Removed:
transparent_proxymode andsandbox/interception.py— a FastAPI proxy that ran inside the sandbox and injectedlogprobs=trueinto upstream LLM calls. - Added:
interception_gatemode withInterceptionServerrunning on the trainer host. The agent'sOPENAI_BASE_URLpoints to{base_url}/rollout/{id}/v1. LLM calls block at the server; the training loop dequeues viaqueue.get(), runs its own vLLM forward pass, and returns the response viadeliver_response(). HMAC-signed request IDs prevent cross-rollout leakage.
Driver modes are now black_box (eval/demos) and interception_gate (RL training). CLIAgentSession gains next_request() and deliver() for the training loop. the trainer runs vLLM, the agent points OPENAI_BASE_URL at it, and the trainer owns the forward pass and logprob capture entirely.
Same pattern as AReaL, verifiers, rLLM, AWS AgentCore RL Toolkit, Atropos, OpenRLHF, and OpenClaw-RL.
Network reachability is caller-owned.
rycerzes
changed the title
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Alignment Review — PR #694
Reviewed by 4 parallel alignment-reviewer agents covering: core agents, core sandbox, environment changes, and tests.
Overall verdict: request_changes
Tier 1: Bugs & Mechanical Issues (16 findings)
Critical / High
| # | File | Issue |
|---|---|---|
| 1 | interception_server.py |
Deprecated API: asyncio.get_event_loop().create_future() → use asyncio.get_running_loop().create_future() (deprecated since 3.10, may error in future versions) |
| 2 | interception_server.py |
Race condition: self.intercepts and self.active_rollouts are plain dicts modified from concurrent coroutines without self._lock. Concurrent register_rollout + _handle_chat_completions + unregister_rollout can cause RuntimeError: dictionary changed size during iteration. Protect with asyncio.Lock. |
| 3 | interception_server.py |
Memory leak: Completed intercepts in self.intercepts are never removed after deliver_response(). Over long training runs every completed LLM call leaves a dead entry. Add del self.intercepts[request_id] after future resolves. |
| 4 | cli_driver.py |
Broken import: from openenv.core.harness import (...) references openenv.core.harness (singular) but the existing package is openenv.core.harnesses (plural). This will raise ModuleNotFoundError at runtime. |
| 5 | coding_environment.py |
NoneType crash: _build_session_factory calls self._E2BSandboxBackend(**kwargs) which is None when e2b is not installed. The old code raised a sentinel _RequiresE2B class. Add a guard: if self._E2BSandboxBackend is None: raise RuntimeError(...) |
| 6 | coding_environment.py |
Type mismatch: disable_thinking: Optional[bool] flows from the MCP tool through _run_rollout_impl into _build_agent_config(disable_thinking: bool). When None is passed, behavior may be wrong. Either pass disable_thinking_resolved or change the parameter type. |
| 7 | cli_driver.py |
Fragile private access: getattr(self._agent_bg_job, "_done", None) accesses a private BgJob implementation detail. If the attribute is renamed, next_request() silently spins until timeout. BgJob should expose a public is_done() method. |
Medium
| # | File | Issue |
|---|---|---|
| 8 | opencode.py |
Shell injection risk: _build_opencode_command interpolates {home}/{workdir} into shell commands without shlex.quote. If sandbox_home contains spaces or metacharacters, the command breaks or becomes injectable. |
| 9 | cli_driver.py |
Dead-code timeout logic: The two-step timeout resolution (budget = spec.default_timeout_s then override with config.agent_timeout_s) makes the first assignment dead when config has agent_timeout_s. Simplify to a single ternary chain. |
| 10 | docker_backend.py |
Invalid construction: DockerBgJob(poll_thread=None) with # type: ignore, then immediately mutated via job._poll_thread = poll_thread. Object is in invalid state between construction and mutation. |
| 11 | docker_backend.py |
Dead code: DockerBgJob._error is checked in wait() but never set anywhere. The error-raise branch is unreachable. Either wire it to the poll thread or remove it. |
| 12 | coding_environment.py |
Non-idempotent replay: Setup commands are run twice — once via setup_shell, then replayed individually "for observability". Common setup commands (pip install, mkdir) are not idempotent and may fail on the second run. |
| 13 | harness.py |
Leaky abstraction: CodingAgentSessionFactory._bootstrap_sandbox calls CLIAgentDriver private methods (_wait_for_sandbox_ready, _agent_already_installed, _exec_with_retry). If driver internals change, this breaks silently. Expose a public bootstrap() method instead. |
Low
| # | File | Issue |
|---|---|---|
| 14 | docker_backend.py |
UUID collision: uuid4().hex[:8] (2^32 values) for marker files. Use full hex (2^128) to avoid collisions under high-concurrency training. |
| 15 | test_hf_sandbox_backend.py |
Test isolation: _FakeSandboxAPI.calls is a class-level mutable list. Only one test clears it — others may see stale entries if run order changes. Move to instance variable or add autouse fixture. |
| 16 | test_docker_sandbox_backend.py |
Tautological assertion: assert isinstance(sandbox, SandboxBackend) or hasattr(sandbox, "exec") — the or arm defeats the purpose. Remove the fallback. |
Tier 2: Alignment Flags (8 findings)
Blocking Alignment Questions
FLAG 1 — Deleted security tests with no replacement
- Invariant: "No credential exposure" (INVARIANTS.md §Security §3)
- The old
test_opencode_env.pyhad two adversarial tests:test_start_proxy_keeps_upstream_key_out_of_command(shell injection withsk-test '$(leak)) andtest_interception_cli_reads_upstream_key_from_env. These verified API keys flow through env vars, not CLI argv. The new test suite only checksbg_envs["API_KEY"] == "sk-test-key"with clean inputs — no adversarial shell metacharacter testing. - Action needed: Restore equivalent adversarial security tests in
test_cli_agent_driver.py.
FLAG 2 — transparent_proxy mode removed / logprob capture moved outside env boundary
- Invariant: "Rewards inside environment" (RFC 002, INVARIANTS.md §Architectural §3)
_collect_proxy_turnsis now a stub returning[]. Logprob capture moves to the training loop viainterception_gate. This is a significant architectural boundary shift. Themode="transparent_proxy"string is still accepted by the API but silently produces emptyproxy_turns— a silent contract break.- Action needed: RFC documenting why logprob capture belongs outside the env boundary, or deprecation warning on
transparent_proxymode.
FLAG 3 — New harness infrastructure does not integrate with RFC 005
- Invariant: "One canonical way to build environments" (PRINCIPLES.md)
- RFC 005 defines
HarnessAdapter(ABC),HarnessConfig(Pydantic),HarnessEnvironment. This PR introduces a parallelCLIAgentSpec/CLIAgentDriver/ResourceSessionstack with no connection to RFC 005. Two diverging patterns for the same domain. - Action needed: Either extend RFC 005 abstractions or supersede with a new RFC.
Non-Blocking Alignment Concerns
FLAG 4 — InterceptionServer binds 0.0.0.0 (publicly exposed trainer host). Should default to 127.0.0.1.
FLAG 5 — --dangerously-skip-permissions hardcoded in OPENCODE_SPEC.base_command. Should be opt-in, not default.
FLAG 6 — docker_args passthrough allows callers to trivially bypass container isolation (--privileged, -v /:/mnt). Consider an allowlist.
FLAG 7 — HFSandboxCreateError may expose HF tokens via SDK exception messages propagated through f"... {last_error}".
FLAG 8 — No tests verify the dual API boundary invariant (agent cannot reach reset()/step()) in the new infrastructure. 5000+ lines of new code with no negative test for the most critical invariant.
What's Good
- The spec/driver declarative pattern is architecturally elegant — adding a new agent is writing a dataclass, not modifying the driver.
- The rename (
opencode_env→coding_agent_env) is clean and consistent across imports, docs, pyproject.toml, Dockerfile, and openenv.yaml. - The sandbox protocol abstraction (
SandboxBackend/SandboxHandle/BgJob) is well-designed and provides a clean seam for swapping backends. - Test coverage is extensive for the new driver lifecycle (1006 lines for cli_driver alone).
Recommended Next Steps
- Fix the 7 critical/high bugs (broken import, InterceptionServer race + leak, NoneType crash, type mismatch, deprecated API, private attribute access)
- Restore adversarial security tests for credential-in-argv
- Add a deprecation warning or validation for
mode="transparent_proxy" - Write an RFC (or RFC addendum) for the logprob ownership shift and the CLIAgentSpec/CLIAgentDriver pattern vs RFC 005
- Add a negative test asserting agents cannot access
reset()/step()through the new infrastructure
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review — PR #694
Tier 1: Bugs & Code Quality
Bug 1 — Deprecated asyncio.get_event_loop() in InterceptionServer
src/openenv/core/harness/agents/interception_server.py uses asyncio.get_event_loop().create_future() inside an async def. This is deprecated in Python 3.10+ and raises RuntimeError when no event loop is running in the current thread. Since the handler is a coroutine, use asyncio.get_running_loop().create_future() instead.
Bug 2 — Double bootstrap: setup commands run twice
envs/coding_agent_env/server/coding_environment.py (_run_rollout_impl): Setup commands are passed to CodingAgentTask (which runs them in _bootstrap_sandbox), then the same commands are re-run individually "for observability." Non-idempotent commands (e.g. pip install --upgrade, destructive rm -rf) will break. Either drop the re-run loop or don't pass setup_shell to the task — the two-pass design is architecturally incoherent.
Bug 3 — CodingAgentSessionFactory.create bypasses its own CLIAgentDriver
envs/coding_agent_env/harness.py: The factory creates a CLIAgentDriver in __init__, but create() never calls self._driver.create_session(). Instead it manually reproduces the bootstrap by calling private driver methods (_wait_for_sandbox_ready, _agent_already_installed, _exec_with_retry). The _driver field is dead weight. Either delegate to the driver or remove it.
Bug 4 — _build_session_factory crashes when E2B is not installed
envs/coding_agent_env/server/coding_environment.py: self._E2BSandboxBackend is set to None when e2b is not installed. Calling None(...) raises a cryptic TypeError. Add an explicit guard: if self._E2BSandboxBackend is None: raise RuntimeError("E2B not installed").
Bug 5 — Dropped security tests without replacement
tests/envs/test_coding_agent_env.py removes test_start_proxy_keeps_upstream_key_out_of_command and test_interception_cli_reads_upstream_key_from_env with no equivalent in the new test suite. These verified that API keys never appear in process argv — a real security property that should be re-tested for InterceptionServer.
Resource leak — InterceptionServer._stream_response leaks intercept entries on connection reset
When ConnectionResetError is caught, the intercept dict entry is not removed. On retries, new entries accumulate. Consider cleaning up in the except block.
Tier 2: Alignment Flags (need human sign-off)
FLAG 1 — Logprob capture removed from environment without RFC
The old design captured per-token logprobs inside the environment and returned them as proxy_turns in RolloutResult. The new design returns proxy_turns = [] always and moves logprob capture to the training loop via interception_gate. This changes a public API contract — callers reading result.proxy_turns will silently get nothing. The RolloutResult model still declares the field. This is a significant behavioral change that should go through an RFC.
FLAG 2 — InterceptionServer introduces a third API boundary
INVARIANTS.md states OpenEnv exposes exactly two APIs: WebSocket (Gym-like) and MCP (for agents). The InterceptionServer is a third HTTP server (OpenAI-compatible proxy) that sits between the agent and the LLM. Whether this is a violation or a legitimate extension of training infrastructure deserves explicit discussion. The invariants doc should be updated either way.
FLAG 3 — Two parallel harness namespaces in core
There is already src/openenv/core/harnesses/ (plural). This PR adds src/openenv/core/harness/ (singular) with a parallel structure. Developers onboarding will see two harness abstractions with no guidance on which to use, violating the "one canonical way" principle.
Verdict
Well-motivated refactor with a clean CLIAgentSpec design and good test coverage for the new driver. However, three mechanical bugs need fixing before merge (deprecated asyncio API, double bootstrap, bypassed driver abstraction), and three alignment flags need human sign-off (removed logprob contract, third API boundary, duplicate harness namespaces).
cc: @Darktex for alignment flags
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review — PR #694
Reviewed by 4 parallel alignment-reviewer agents covering: core agents, core sandbox, environment changes, and tests.
Overall verdict: request_changes
Tier 1: Bugs & Mechanical Issues (16 findings)
Critical / High
| # | File | Issue |
|---|---|---|
| 1 | interception_server.py |
Deprecated API: asyncio.get_event_loop().create_future() → use asyncio.get_running_loop().create_future() (deprecated since 3.10, may error in future versions) |
| 2 | interception_server.py |
Race condition: self.intercepts and self.active_rollouts are plain dicts modified from concurrent coroutines without self._lock. Concurrent register_rollout + _handle_chat_completions + unregister_rollout can cause RuntimeError: dictionary changed size during iteration. Protect with asyncio.Lock. |
| 3 | interception_server.py |
Memory leak: Completed intercepts in self.intercepts are never removed after deliver_response(). Over long training runs every completed LLM call leaves a dead entry. Add del self.intercepts[request_id] after future resolves. |
| 4 | cli_driver.py |
KeyError race: next_request() does bare server.intercepts[request_id] dict access after queue.get(). If close() calls unregister_rollout() from another thread between the queue read and the dict access, this raises an unhandled KeyError. Use .get() and handle None. |
| 5 | coding_environment.py |
NoneType crash: _build_session_factory calls self._E2BSandboxBackend(**kwargs) which is None when e2b is not installed. The old code raised a sentinel _RequiresE2B class. Add a guard: if self._E2BSandboxBackend is None: raise RuntimeError(...) |
| 6 | coding_environment.py |
Type mismatch: disable_thinking: Optional[bool] flows from the MCP tool through _run_rollout_impl into _build_agent_config(disable_thinking: bool). When None is passed, behavior may be wrong. Pass disable_thinking_resolved (the guaranteed-bool value) instead. |
| 7 | opencode.py |
Shell injection: _build_opencode_command uses "$(cat {instruction_file})" inside a double-quoted shell string. If the task instruction file contains shell metacharacters ($(...), backticks, "), they will be interpreted by the shell. Pass instruction via a method that doesn't expand shell metacharacters. |
Medium
| # | File | Issue |
|---|---|---|
| 8 | opencode.py + pi.py |
Unquoted paths: Both _build_opencode_command and _build_command interpolate home/workdir/instruction_file/log_file into shell strings without shlex.quote(). Paths with spaces or metacharacters will break. |
| 9 | docker_backend.py |
Invalid construction: DockerBgJob(poll_thread=None) with # type: ignore, then immediately mutated via job._poll_thread = poll_thread. Object is in invalid state between construction and mutation. Make poll_thread optional or use a factory method. |
| 10 | docker_backend.py |
Dead code: DockerBgJob._error is checked in wait() but never set anywhere. The error-raise branch is unreachable. Either wire it to the poll thread or remove it. |
| 11 | docker_backend.py |
Infinite loop: _poll_bg_job silences all exceptions with bare except Exception: pass. If the container is externally killed, the daemon thread loops forever at 0.5s intervals issuing docker exec calls against a gone container. Add a failure counter or container existence check. |
| 12 | coding_environment.py |
Double bootstrap: Setup commands run twice — once atomically inside _bootstrap_sandbox as setup_shell, then replayed individually post-factory.create() "for observability." Non-idempotent commands (pip install git+..., git clone, createdb) will fail on the second run. |
| 13 | harness.py |
Leaky abstraction: CodingAgentSessionFactory._bootstrap_sandbox calls CLIAgentDriver private methods (_wait_for_sandbox_ready, _agent_already_installed, _exec_with_retry). Expose a public bootstrap() method instead. |
Low
| # | File | Issue |
|---|---|---|
| 14 | docker_backend.py |
UUID collision: uuid4().hex[:8] (2^32 values) for marker files. Use full hex (2^128) to avoid collisions under high-concurrency training. |
| 15 | docker_backend.py + hf_backend.py |
Duplicated code: Identical _shell_quote implementation in both files. Move to base.py or a _util.py module. |
| 16 | interception_server.py |
Info leak: str(exc) from trainer-side exceptions sent to sandboxed agent in 500 responses. Replace with generic message; log detail server-side. |
Tier 2: Alignment Flags (8 findings)
Blocking Alignment Questions
FLAG 1 — Deleted security tests with no replacement
- Invariant: "No credential exposure" (INVARIANTS.md §Security §3)
- The old
test_opencode_env.pyhad two adversarial tests:test_start_proxy_keeps_upstream_key_out_of_command(shell injection withsk-test '$(leak)) andtest_interception_cli_reads_upstream_key_from_env. These verified API keys flow through env vars, not CLI argv. The new test suite only checksbg_envs["API_KEY"] == "sk-test-key"with clean inputs — no adversarial shell metacharacter testing. - Action needed: Restore equivalent adversarial security tests in
test_cli_agent_driver.py.
FLAG 2 — transparent_proxy mode removed / logprob capture moved outside env boundary
- Invariant: "Rewards inside environment" (RFC 002, INVARIANTS.md §Architectural §3)
_collect_proxy_turnsis now a stub returning[]. Logprob capture moves to the training loop viainterception_gate. This is a significant architectural boundary shift. Themode="transparent_proxy"string is still accepted by the API but silently produces emptyproxy_turns— a silent contract break.- Action needed: RFC documenting why logprob capture belongs outside the env boundary, or deprecation warning on
transparent_proxymode.
FLAG 3 — New harness infrastructure does not integrate with RFC 005
- Invariant: "One canonical way to build environments" (PRINCIPLES.md)
- RFC 005 defines
HarnessAdapter(ABC),HarnessConfig(Pydantic),HarnessEnvironment. This PR introduces a parallelCLIAgentSpec/CLIAgentDriver/ResourceSessionstack with no connection to RFC 005. Two diverging patterns for the same domain. - Action needed: Either extend RFC 005 abstractions or supersede with a new RFC.
FLAG 4 — InterceptionServer has zero dedicated tests
- Invariant: No credential exposure + Dual API boundary
- 324 new lines of security-critical async HTTP code (HMAC auth, per-rollout request queuing, streaming SSE) with exactly one test that only checks a constructor guard. No test of: 401 rejection, rollout-not-found 404,
deliver_responsepaths, concurrent rollout isolation, orunregister_rolloutcancellation. - Action needed: Add dedicated test suite for
InterceptionServer.
Non-Blocking Alignment Concerns
FLAG 5 — InterceptionServer binds 0.0.0.0 (publicly exposed trainer host). Should default to 127.0.0.1.
FLAG 6 — --dangerously-skip-permissions hardcoded in OPENCODE_SPEC.base_command. Should be opt-in, not default.
FLAG 7 — docker_args passthrough allows callers to trivially bypass container isolation (--privileged, -v /:/mnt). Consider an allowlist.
FLAG 8 — No tests verify the dual API boundary invariant (agent cannot reach reset()/step()) in the new infrastructure. 5000+ lines of new code with no negative test for the most critical invariant.
What's Good
- The spec/driver declarative pattern is architecturally elegant — adding a new agent is writing a dataclass, not modifying the driver.
- The rename (
opencode_env→coding_agent_env) is clean and consistent across imports, docs, pyproject.toml, Dockerfile, and openenv.yaml. - The sandbox protocol abstraction (
SandboxBackend/SandboxHandle/BgJob) is well-designed and provides a clean seam for swapping backends. - Test coverage is extensive for the new driver lifecycle (1006 lines for cli_driver alone).
Recommended Next Steps
- Fix the 7 critical/high bugs (broken import, InterceptionServer race + leak, NoneType crash, type mismatch, deprecated API, shell injection)
- Restore adversarial security tests for credential-in-argv
- Add a deprecation warning or validation for
mode="transparent_proxy" - Write an RFC (or RFC addendum) for the logprob ownership shift and the CLIAgentSpec/CLIAgentDriver pattern vs RFC 005
- Add a negative test asserting agents cannot access
reset()/step()through the new infrastructure
Automated review by Claude Code | Learn more
|
Thank you for the reviews @Darktex! I will be fixing them shortly. Also could you clean up the duplicated noise caused by claude so that the thread is cleaner? |
- Wire disable_thinking and max_tokens_cap through CodingAgentConfig - Raise RuntimeError on mkdir/cat failures in docker backend - Propagate QueueFull exceptions instead of silently swallowing - Change CommandResult.exit_code to int | None for bootstrap clarity
replace asyncio.Queue with stdlib queue.Queue for the request notification path (server → training loop). This makes both directions of the InterceptionServer cross-loop/cross-thread safe: - Request notifications: queue.Queue (inherently thread-safe) - Response delivery: asyncio.Future via _resolve_future_threadsafe (already cross-loop safe) The consumer (next_request) uses asyncio.to_thread(q.get, timeout=...) to await without blocking the event loop. This follows the same pattern used by OpenClaw-RL at scale. chunk_queue (internal SSE streaming) remains asyncio.Queue since both producer and consumer run on the server's own event loop.
…andling - soak test
There was a problem hiding this comment.
fixed all the following issues:
interception_gatewiring fixed inCodingAgentSessionFactory.create()(37e549d)_build_agent_configparam forwarding,DockerBackend.write_textreturn code checks,_put_queue_threadsafeno longer drops onQueueFull, andsetup_resultsexit codes no longer fabricated (8aa9d18, a2b4388/448f6905)- Whitespace secret bypass and hardcoded
/root/path resolved (61e5524) - Cross-loop
asyncio.Queuedeadlock fixed by replacing with stdlibqueue.Queuefor the request notification path; consumer usesasyncio.to_thread(q.get, timeout=...)to avoid blocking the event loop.chunk_queueintentionally stays asasyncio.Queuesince both producer and consumer run on the server's own event loop (a2b4388/b10a4483)
transparent_proxy migration docs are a non-issue — only one env used it and it's already been migrated.
sergiopaniego
left a comment
sergiopaniego
left a comment
There was a problem hiding this comment.
Inline comments on concrete issues found during review.
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Tier 1: Bugs & Issues
-
Bug (Critical):
coding_agent_envinterception_gate mode broken — opencode.json written with wrong base_url.CodingAgentSessionFactory.create()calls_bootstrap_sandboxbefore computingbase_url_override. The bootstrap writesopencode.jsonviaOPENCODE_SPEC.filesusing the originalconfig.base_url, not the interception rollout URL. The oldopencode_env/harness.pycorrectly rewrote opencode.json after bootstrap;coding_agent_env/harness.pydoes not. This silently falls back to direct LLM calls, bypassing the InterceptionServer entirely. -
Bug (Critical):
OPENAI_API_KEYnot set to interception secret.CodingAgentSession.start_agent()callsbuild_env_varswhich setsOPENAI_API_KEY = config.api_key(original key). Ininterception_gatemode theInterceptionServerexpects the bearer token to matchserver.secret. Every request to the InterceptionServer gets HTTP 401. -
Private attribute access:
coding_agent_env/harness.pylines 138, 143, 161 accessself._driver._interception_serverandself._driver._interception_base_url. This couples the env harness to driver internals and will break silently ifCLIAgentDriverrenames those fields. -
per_token_logpsfiltering removed: The diff changes from[float(x) for x in (rec.get("per_token_logps") or []) if x is not None]tolist(rec.get("per_token_logps") or []).Nonevalues are no longer filtered, which will fail Pydantic validation onRolloutTurn.per_token_logps: list[float].
Tier 2: Alignment
ALIGNMENT FLAG: transparent_proxy mode dropped — undocumented breaking change
- Invariant at risk: Minimize lifecycle deltas. The old
opencode_envsupportedmode="transparent_proxy"as the default for logprob capture.coding_agent_envdrops it entirely — onlyblack_boxandinterception_gateremain. Code that depended ontransparent_proxywill silently switch toblack_box(no logprob capture). This needs an RFC discussion per PRINCIPLES.md. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: InterceptionServer exposed on 0.0.0.0 only emits a warning
- Invariant at risk: Agent isolation / security. The interception secret is set as
OPENAI_API_KEYinside sandboxes. If exposed on all interfaces, any network peer observing the bearer token can inject responses into live training rollouts. The warning is too quiet for this risk. - Suggested reviewer: @Darktex
Summary
The CLIAgentSpec/CLIAgentDriver refactoring is architecturally sound and the InterceptionServer correctly rejects reserved tool names (reset, step, state, close). However, two critical bugs make interception_gate mode non-functional in coding_agent_env: opencode.json is written with the wrong base_url, and the API key isn't overridden to the interception secret. Both exist because CodingAgentSession.start_agent() bypasses CLIAgentDriver._start_agent().
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: SKIPPED — Manual review substituted.
- Debug code: CLEAN.
Tier 1: Fixes Required
-
src/openenv/core/harness/agents/interception_server.py:552—getattr(q, "_loop", None)accesses a CPython private attribute onasyncio.Queue. In Python 3.10+,Queue._loopisNonewhen created outside a running loop. The fallbackq.put_nowait(item)is not thread-safe for an asyncio queue owned by another thread's event loop. Fix: capture the serving event loop atInterceptionServer.start()time and useself._loop.call_soon_threadsafe(q.put_nowait, item). -
src/openenv/core/harness/agents/cli_driver.py:325-326— Bareassertstatements increate_session()are stripped bypython -O. Replace withif ... raise RuntimeError(...)guards. -
src/openenv/core/harness/agents/cli_driver.py:199— Same:assert server is not NoneinCLIAgentSession.next_request()is stripped by-O. -
envs/coding_agent_env/server/coding_environment.py:100,103— ImportsCLIAgentSessionFactoryfrom core at line 100, then shadows withCodingAgentSessionFactoryat line 103. The naming is confusing — use distinct aliases. -
src/openenv/core/harness/agents/cli_driver.py:487,503— The "generic"CLIAgentDriver._start_agent()contains two hardcodedif self.spec.name == "pi":branches for Pi-specific config. This breaks the declarative promise ofCLIAgentSpec. Move Pi-specific logic to data onCLIAgentSpec(e.g., apre_start_hookcallable or template fields). -
docs/source/environments.md:555— Card description still reads "optionally capturing per-token logpr..." — stale reference. The newcoding_agent_envdoes not offertransparent_proxymode in its MCP API.
Tier 2: Alignment Discussion
ALIGNMENT FLAG: interception_gate mode exposed as a passable string to the MCP tool
- Principle at stake: Agents cannot access simulation controls
- The concern: The
run_rolloutMCP tool acceptsmode: str = "black_box"with no validation. An agent could passmode="interception_gate". Currently this fails safely (noInterceptionServerwired to the server process), but the mode string leaks internal architecture and a future misconfiguration could allow an agent to trigger trainer-owned interception. Validatemodeat the MCP tool layer and reject"interception_gate"explicitly. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: Core architectural changes without an RFC
- Principle at stake: RFC-backed architectural decisions
- The concern: This PR moves sandbox protocols into
src/openenv/core/harness/, adds two new backend implementations, and shipsCLIAgentDriver+InterceptionServeras core-level APIs. RFC 005 covers the concept at a high level but its status is "In Review" and it doesn't enumerate theCLIAgentSpecdata model,InterceptionServer, orCLIAgentDriver. This is a non-trivial core API surface that addsaiohttpas a dependency and defines extension points other environments depend on. An RFC amendment should document the intended public surface. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: transparent_proxy mode removed from deployable HTTP env without migration path
- Principle at stake: Minimize lifecycle deltas; pre-1.0 breaking changes require documentation
- The concern: The rename drops
transparent_proxyfrom the deployable MCP tool. Existingopencode_envusers who relied onmode="transparent_proxy"inrun_rolloutwill get a silent fallback or error.proxy_turnsis removed fromRolloutResult,RolloutTurnis dropped from exports. No migration guide is provided. The new README asymmetrically liststransparent_proxyas available "through the in-processOpenCodeSessionFactory" but not via the HTTP API. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: Docker sandbox backend has no network isolation by default
- Principle at stake: Container isolation ("Network access must be explicitly configured")
- The concern:
DockerSandboxBackendlaunches containers withdocker run -d --add-host host.docker.internal:host-gatewayand no--networkflag, giving full access to the host's Docker bridge network. Any agent inside a Docker sandbox can reach other containers and, viahost-gateway, all host-bound services. Should default to--network=nonefor isolated workloads, opting into host access only when explicitly requested. - Suggested reviewer: @Darktex
Summary
- 6 mechanical issues to fix (thread-safety bug in async queue delivery, 2 bare asserts stripped by
-O, shadowed import, hardcoded agent-name check in driver, stale docs) - 4 alignment points for human review (agent-accessible mode string, missing RFC for core changes, breaking transparent_proxy removal, Docker network isolation)
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Review: PR #694 — refactor agent sandbox infrastructure + new backends + rework logprobs capture
Overall this is a well-designed refactoring with clear declarative intent. The CLIAgentSpec pattern is a genuine improvement over the monolithic harness. However there are two correctness/security issues that must be fixed before merge, plus four alignment discussion points.
Verdict: Request Changes
CRITICAL — Must fix before merge
1. Unauthenticated /exit endpoint in InterceptionServer
src/openenv/core/harness/agents/interception_server.py — _handle_exit:
_handle_chat_completions and _handle_tool_call both start with if not self._authorized(request): return 401. _handle_exit does not. Any process that can reach the server and guess a rollout_id can inject a fake exit sentinel, causing next_request() to return None early and silently aborting the training loop.
Fix: add the auth check as the first statement in _handle_exit, consistent with the other handlers. The _start_agent curl command already sends Authorization: Bearer {secret}, so fixing the server side does not break the legitimate exit path.
2. _put_queue_threadsafe uses asyncio.Queue._loop (removed in Python 3.10)
asyncio.Queue._loop was removed in Python 3.10. getattr(q, "_loop", None) returns None on all 3.10+ runtimes. When _loop is None the function falls through to q.put_nowait(item) called from an arbitrary thread — which is NOT thread-safe and can corrupt the queue or raise RuntimeError: Event loop is closed.
Fix: track the server's event loop at startup and use loop.call_soon_threadsafe(q.put_nowait, item) unconditionally when calling from a non-event-loop thread.
REQUIRED — Fix before merge
3. CommandResult.exit_code: int | None is a silent semantic break
exit_code changed from int to int | None = None. Any caller doing if cr.exit_code != 0 now silently passes when exit_code is None (because None != 0 is True). Callers using cr.exit_code > 0 will TypeError. Should be documented in migration notes.
4. Per-agent if self.spec.name == "pi" branches in generic CLIAgentDriver._start_agent
Lines contain agent-specific logic in the driver that was supposed to be agent-agnostic. Pi's models.json write and PI_CODING_AGENT_DIR env var injection should be expressed as spec callables or fields. The extension_dir_template pattern already exists for this purpose.
5. aiohttp added as a hard dependency to openenv-core
aiohttp (~100 kB + compiled transitive deps) is only needed for interception_gate mode on the trainer node. Should be an optional extra: pip install openenv-core[training].
6. E2BSandboxBackend = None stub is worse than removed _RequiresE2B
The old stub raised a clear ImportError with installation instructions. The new E2BSandboxBackend = None stub means E2BSandboxBackend() raises TypeError: 'NoneType' object is not callable — unhelpful. Restore the informative error.
Alignment Flags (human decision required)
ALIGNMENT FLAG 1: interception_gate mode — new trainer-host protocol not fully covered by RFC 005
- Principle at stake: Key design decisions documented in RFCs (PRINCIPLES.md)
- The concern: RFC 005 describes the wrapping pattern for agentic harnesses but not a host-side HTTP interception server that takes over the LLM forward pass. The
InterceptionServeris architecturally novel: it makes the trainer responsible for generation. The security model, failure modes, and relationship to the dual API boundary need explicit documentation. - Suggested reviewer: @Darktex
ALIGNMENT FLAG 2: The dual API boundary holds, but interception_gate blurs it in documentation
- Principle at stake: "Dual API boundary" — WebSocket for infrastructure, MCP for agents (INVARIANTS.md §1)
- The concern:
interception_gatecreates a third interface path (agent → HTTP → trainer host). It is correctly NOT a simulation control surface, but the architectural documentation should be updated to acknowledge this third path. - Suggested reviewer: @Darktex
Minor
OpenCodeSessionFactory.createaccessesself._driver._interception_serverandself._driver._exec_with_retrydirectly. Private members should be exposed as properties/public methods.- RFC checklist should reference RFC 005 rather than "Not required."
Note: This is an automated review by Claude Code, not a human review.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Alignment Review — PR #694
Tier 1: Fixes Required
- Private member access across module boundary (
coding_agent_env/harness.pyandopencode_env/harness.py): Reaches intoself._driver._interception_serverandself._driver._interception_base_url— will break silently ifCLIAgentDriverrenames internals. Should usecreate_session()or expose via a property. _put_queue_threadsafefallback broken on Python 3.10+ (interception_server.py:565):asyncio.Queue._loopwas removed in 3.10, so the fallback always callsq.put_nowait()from a non-loop thread, which may fail to wake awaiting coroutines. Useloop.call_soon_threadsafe(q.put_nowait, item)with a stored loop reference.- Marker file in
/tmpnot cleaned afterkill()(docker_backend.py:131): Minor — Docker container teardown cleans these, but accumulation risk exists for long-running containers. - Env-var values in
-e KEY=VALUE(docker_backend.py:244): Inconsistent quoting vs.HFSandboxBackendwhich usesshell_quote. Low risk but could fail on values with newlines.
Tier 2: Alignment Flags (for human review)
interception_gatetool injection:register_tool_handlerallows trainer to inject arbitrary tool definitions into the agent's chat context.RESERVED_TOOL_NAMESblocks reset/step/state/close, but a trainer-defined tool could have reset-equivalent effects. Should this new mechanism get an RFC?transparent_proxyarchitecture: In-sandbox HTTP proxy sees every agent-LLM message — doesn't fit cleanly into either WebSocket or MCP boundary. Consider migrating entirely tointerception_gateand deprecatingtransparent_proxy.CommandResult.exit_codebroadened toint | None: Nullable exit_code on a wire type shifts None-handling burden to all callers and hides setup failures.
Verdict: request_changes — Private member access is a concrete bug risk, and the asyncio.Queue thread-safety fallback needs fixing before merge.
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review — PR #694: Sandbox Refactor + Agent Backends
Automated Checks
- Lint: SKIP (network unavailable in review environment)
- Debug code: CLEAN
Tier 1: Fixes Required
-
cli_driver.py:325-326— Bareassertstatements increate_session(). In optimized builds (python -O) these are silently stripped. Replace withRuntimeErrorraises. -
cli_driver.py:199— Same:assert server is not Noneinnext_request(). Use explicitRuntimeError. -
envs/coding_agent_env/harness.py:138,143,161—CodingAgentSessionFactory.create()reaches intoself._driver._interception_serverandself._driver._interception_base_url(private attributes). Either delegate tocreate_session(), or promote these to read-only public properties onCLIAgentDriver. -
docker_backend.py—DockerBgJob.wait()— Returns0when_exit_code is None. This path is hit whenkill()sets_donewithout setting_exit_code. A killed process should not return success; use-1or137. -
hf_backend.py—HFBgJob.wait()— Same issue: returns success for killed processes.
Tier 2: Alignment Discussion
ALIGNMENT FLAG 1: Hard-coded spec.name == "pi" branches inside generic CLIAgentDriver._start_agent
- Principle at stake: "One canonical way to build environments" (PRINCIPLES.md); the driver's own docstring says it "reads [spec] fields mechanically without knowing anything about the specific agent."
- The concern:
cli_driver.py:487containsif self.spec.name == "pi":to setPI_CODING_AGENT_DIRand write models.json. This is exactly the imperative code theCLIAgentSpecabstraction eliminates. The spec should declare these needs viaextension_dir_templateandfilescallables. - Suggested reviewer: @Darktex
ALIGNMENT FLAG 2: ANTHROPIC_API_KEY set to interception secret for all agents
- Principle at stake: "No credential exposure" (INVARIANTS.md)
- The concern:
cli_driver.py:509unconditionally setsANTHROPIC_API_KEY = self._interception_server.secretfor every agent ininterception_gatemode. For agents that don't use Anthropic (OpenCode usesOPENAI_API_KEY; Pi usesHF_TOKEN), this leaks the interception bearer token into an extra env var that travels into the sandbox. - Suggested reviewer: @Darktex
ALIGNMENT FLAG 3: /exit endpoint — present in diff, absent from on-disk interception_server.py
- Principle at stake: Correctness / dual API boundary
- The concern: The diff shows
cli_driver.pyappendingcurl ... /exitto agent commands, but the on-diskinterception_server.pydoes not register this route. This needs confirmation — is agent-exit notification implemented or deferred? - Suggested reviewer: @Darktex
ALIGNMENT FLAG 4: Rename opencode_env -> coding_agent_env is incomplete
- Principle at stake: Clarity / "one canonical way"
- The concern: Both
/envs/opencode_env/and/envs/coding_agent_env/exist in the working tree. If these are intended to coexist, it should be documented. If this is a rename, the old directory should be removed. - Suggested reviewer: @Darktex
Summary
The CLIAgentSpec + CLIAgentDriver abstraction is the right architectural direction. The five Tier 1 fixes are straightforward (asserts -> RuntimeError, private attribute access, killed-process exit codes). The four Tier 2 items need human input, particularly the Pi-specific driver branches which undermine the abstraction this PR establishes.
Automated review by Claude Code | Learn more
Darktex
left a comment
Darktex
left a comment
There was a problem hiding this comment.
Note: This is an automated review by Claude Code, not a human review.
Alignment Review Report
Automated Checks
- Lint: PASS
- Debug code: CLEAN
Tier 1: Fixes Required (blocking)
-
Security: Unauthenticated
_handle_exitendpoint —interception_server.py:_handle_tool_calland_handle_chat_completionsboth checkself._authorized(request), but_handle_exitdoes not. Any process reachable from the sandbox can POST to/rollout/{rollout_id}/v1/exitand terminate a live rollout. Fix: add auth check as first line of_handle_exit. -
Secret exposure in shell command —
cli_driver.py: The bearer secret is passed viashlex.quote(auth_header)into a shell command string, making it visible in process listings (ps aux). Since it's already passed viaOPENAI_API_KEYenv var, use$OPENAI_API_KEYin the curl command instead of the literal secret.
Tier 2: Alignment Discussion
FLAG 1: interception_gate mode diverges from RFC 005's HarnessAdapter abstraction. RFC 005 keeps the harness's LLM calls opaque to the training loop; interception_gate inverts control — the trainer owns the forward pass. This is a competing alternative, not a refinement.
- Principle: Core changes require RFC (PRINCIPLES.md)
- Suggested reviewer: @Darktex
FLAG 2: InterceptionServer gates every agent LLM call through the trainer, raising a "dual API boundary" question — the agent's behavior is directly determined by whoever controls the InterceptionServer.
- Principle: Dual API boundary (INVARIANTS.md §1); RFC 005 §"Harness Security Boundary"
- Suggested reviewer: @Darktex
Summary
- 2 blocking mechanical issues (security: unauth endpoint + secret exposure)
- 2 alignment points (RFC 005 divergence + trainer-coupling model)
- Core refactoring work is well-executed with thorough tests. Blocking issues are straightforward fixes.
Automated review by Claude Code
burtenshaw
added
enhancement
There was a problem hiding this comment.
Closing this in favor of re-landing it as a stack of small, independently reviewable PRs — the direction is right, the packaging isn't.
Why close rather than rebase:
- Five separable changes in one diff (sandbox protocols → core, Docker backend, HF backend, agent adapter layer, logprob-capture rework). Each deserves its own review.
- Main has moved.
src/openenv/core/harness/is now the RFC 005 runtime (CLIHarnessAdapter,build_harness_rollout_func,collect.py, via #652/#903). This branch'score/harness/agents/namespace collides with it conceptually — the rebase is a redesign, and the branch is ~165 commits behind. - The interception server needs an RFC. It's a breaking change to core and went through several review rounds on auth/asyncio/bootstrap ordering without one. Writing that RFC now and validating the design against verifiers / AReaL / NVIDIA Polar / TRL-native rollout first.
4 participants
Summary
The coding environment was hardwired to OpenCode. This PR pulls the sandbox and harness infrastructure into
src/openenv/core/harness/so it's shared, adds a declarativeCLIAgentSpec+CLIAgentDriverthat any agent adapter can plug into, ships OpenCode and Pi as the first two adapters, adds Docker and HF Sandbox backends alongside the existing E2B one, and renamesopencode_env→coding_agent_env.Changes
Sandbox protocols moved to core (
src/openenv/core/harness/sandbox/).SandboxBackend,SandboxHandle,BgJobwere locked insideopencode_env. They're now the shared seam.E2BSandboxBackendis unchanged and already works with CubeSandbox (E2B-compatible self-hosted MicroVM sandbox) by pointingE2B_API_URLat it.Docker sandbox backend added (
src/openenv/core/harness/sandbox/docker_backend.py). Runs sandboxes viadocker run. Suitable for local development and CI.HF Sandbox backend added (
src/openenv/core/harness/sandbox/hf_backend.py). Wraps hf-sandbox.CLIAgentSpec+CLIAgentDriver(src/openenv/core/harness/agents/). Per-agent knowledge is expressed as a declarative dataclass — install script, files to upload, env vars, MCP config format, artifacts to collect — with three small callables for the parts that can't be data (command builder, MCP config serializer, stdout event parser). The driver reads these fields and runs the lifecycle; it has no per-agent branches. Same pattern as verifiers (Prime Intellect). Adding a new agent is writing a spec, not touching the driver.OpenCode refactored onto
CLIAgentSpec.envs/coding_agent_env/harness.pyis now a thin wrapper aroundCLIAgentSessionFactory.Pi adapter added (
src/openenv/core/harness/agents/pi.py). Runs Pi in print mode via--no-session -p @/instruction.txt, matching the verifiers Pi harness approach.opencode_env→coding_agent_env. The environment is for coding tasks; the harness is a config parameter.run_rolloutnow takesagent: str = "opencode"and supports"opencode"and"pi". Gradio UI gets an agent selector. All imports, docs,pyproject.toml,openenv.yaml, andDockerfileupdated.Tests
tests/core/test_cli_agent_driver.py— driver lifecycle via_FakeSandboxtests/core/test_harness_adapters.py— per-adapter: MCP config, CLI flags, event parsing, registrytests/core/test_docker_sandbox_backend.py— Docker backendtests/core/test_hf_sandbox_backend.py— HF backendtests/envs/test_coding_agent_env.py— end-to-end env tests (renamed, extended)References
CLIHarness— declarative harness pattern this followsremote_inf_engine.py— proxy-based logprob interception patternType of Change
Alignment Checklist
Before submitting, verify:
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violated/pre-submit-pr(orbash .claude/hooks/lint.shand tests) and addressed all issuesRFC Status
Claude Code Review
n/a
cc: @burtenshaw