Repository navigation
Conversation
burtenshaw
added
feature
size: extra-large
Cursor
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| "import sys\n" | ||
| "import openenv.validation.manifest\n" | ||
| "assert not any(name.startswith('openenv.core') for name in sys.modules)\n", |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Alignment Review Report
Two-tier review of the openenvd isolated runtime (RFC 009 implementation). Automated checks were run and the full diff (~6,900 lines across 50 files) was read.
Automated Checks
- Lint: PASS (CI-relevant).
ruff check src/ tests/→ All checks passed!, and no.pyfile added/modified by this PR needsruff formatorusortchanges.- The local
.claude/hooks/lint.shprints "58 files would be reformatted", but 57 of 58 are pre-existing files this PR never touches (env READMEs /.pyunderenvs/, plussrc/openenv/core/README.mdand the CLI template README). This is ruff-version drift: the tree was formatted with an older ruff, whileuv syncinstalls ruff 0.16.8 which reformats embedded code in.md. CI's format gate only fails on.pydiffs (git diff --name-only -- '*.py') and does not scanenvs/, so this does not block CI. usortflags onlytests/envs/test_grid_world.pyandtests/envs/test_julia_env.py— both pre-existing and untouched here.
- The local
- Debug code: 1 minor finding.
src/openenv/core/openenvd/_network.py:520usesprint(..., file=sys.stderr)in the helper's__main__; add# ok-to-print(inline note left). Nobreakpoint()/pdb. All other print/TODO hits are in pre-existing non-PR files.
Test note: tests/core/test_openenvd_*.py = 71 passed / 9 skipped locally; the one non-skipped failure (could not create IPC namespace: Operation not permitted) is only because this unprivileged sandbox can't unshare(CLONE_NEWIPC). The PR's openenvd.yml runs the suite as root with the required caps, so this is an environment limitation, not a code defect.
Open RFCs Context
- RFC 009 – openenvd — Status:
Draft(the RFC this PR implements; author @zkwentz). - Governs/overlaps areas owned by RFC 001 (dual API boundary, agents-cannot-reset), RFC 003 (MCP), RFC 004 (rewards in env), RFC 005 (harness security boundary — "under review"), RFC 008 (normalized manifest / validation).
Tier 1: Fixes Required
-
src/openenv/core/openenvd/_network.py:520— add# ok-to-printto the intentional stderr print (satisfiescheck-debug.sh). -
src/openenv/core/openenvd/README.md— runuv run ruff formaton the file to settle embedded code-block whitespace (optional; not CI-gated).
No mechanical blockers: no syntax/type/import errors, and no credential exposure — token handling is notably careful (tokens read from env vars, compared via hmac.compare_digest, never forwarded to the child env, and EnvClient now scrubs headers from connection-error messages).
Tier 2: Alignment Discussion
RFC Conflicts
ALIGNMENT FLAG: Implements a Draft RFC and collapses its staged rollout into one PR
- Principle/RFC at stake: RFC 009 (Status: Draft); the
rfcs/README.mdlifecycle (Draft → In Review → Accepted → Implemented) - The concern: RFC 009's own Implementation Plan sequences the work as PR 1 = "Review and consensus on principals, surfaces, and the deployment shape", then PR 2 (types/validation), PR 3 (core), PR 4 (grader surface), PR 5 (observer + harness). This PR lands PR 2–5 while the RFC is still
Draft. Building before consensus pre-empts the design discussion the RFC exists to have. The team should decide whether to (a) advance RFC 009 to In Review/Accepted first, or (b) land incrementally per the RFC's own plan. - Suggested reviewer: @Darktex (author of INVARIANTS.md / PRINCIPLES.md). Note the RFC author (@zkwentz) is also the PR author, so an independent design sign-off is warranted.
ALIGNMENT FLAG: Reviewability / blast radius of a single ~6,900-line privileged change
- Principle/RFC at stake: RFC 009 Key Design Decision 1 (staged surfaces, "uniformly validated"); stability trade-off in PRINCIPLES.md
- The concern: The PR introduces seccomp, user/mount/IPC/network namespaces, cgroup v2, and nftables/NFLOG via raw
ctypes, plus a fork→exec isolation helper — all at once. This is high-risk, security-critical code where line-by-line review matters; a single PR makes that hard and runs counter to the RFC's staged plan. - Suggested reviewer: @Darktex (plus a security-focused reviewer)
Principle Conflicts
Most invariant-sensitive behavior is aligned and arguably strengthened: lifecycle is confined to the orchestrator surface (/ws, behind a bearer token + allow_lifecycle); SurfacePolicy structurally rejects reset/step/state/close and grader.* on the agent surface; rewards stay inside the environment; and clients avoid importing server code via the lazy __init__. Two points still merit a human check:
ALIGNMENT FLAG: "Agent surface is frozen" needs explicit verification
- Principle/RFC at stake: RFC 009 Key Design Decision 5 ("openenvd must not change a single byte of the agent's view"); INVARIANTS.md dual API boundary
- The concern: The agent MCP is served two ways — the daemon
/mcp(surfaces.py) and an in-namespace uvicorn app (worker.pylocal_agent_app). Please confirm both expose byte-identical tool lists / errors / headers to what agents see today, so training↔eval parity holds and a policy cannot detect openenvd's presence. - Suggested reviewer: @Darktex
ALIGNMENT FLAG: New HTTP MCP endpoints vs. the WebSocket-only direction
- Principle/RFC at stake: INVARIANTS.md "Communication patterns" (WebSocket for all env communication; HTTP being deprecated per PR #252)
- The concern:
surfaces.pyadds HTTPPOST /mcpandPOST /mcp/graderalongside the WS variants. HTTP is still allowed during the transition, but new HTTP surfaces grow the exact area slated for removal. Confirm this is intentional (parity with the existing production HTTP MCP path) rather than expanding the HTTP footprint. - Suggested reviewer: @Darktex
Summary
- 2 mechanical items (both minor; neither blocks CI):
# ok-to-printmarker;README.mdformatting. - 2 principle points for human review: verify the agent surface is frozen; new HTTP MCP endpoints vs. WS-only direction.
- 2 RFC conflicts to discuss: implementing a
DraftRFC before consensus, and the single-PR scope vs. RFC 009's staged 5-PR plan.
Sent by Cursor Automation: Pre-review
| try: | ||
| asyncio.run(run(sys.argv[1], cleanup=sys.argv[2:] == ["--cleanup"])) | ||
| except Exception as error: | ||
| print(f"openenvd network: {error}", file=sys.stderr) |
There was a problem hiding this comment.
Tier 1 (minor): check-debug.sh flags this print(...). It's legitimate CLI error reporting for the standalone helper, so per repo convention add a # ok-to-print marker to silence the hook:
print(f"openenvd network: {error}", file=sys.stderr) # ok-to-print
k21993
left a comment
k21993
left a comment
There was a problem hiding this comment.
Scoped review of c034c114. I think the direction is valuable, but I would not land the implementation in its current shape.
Design/staging gate
RFC 009 is still Draft, while its own implementation plan says PR 1 is consensus on principals/surfaces/deployment, followed by separate PRs for policy types, core runtime, grader, and observer/harness integration. This PR combines all of those stages into one conflicting 6,854-line / 50-file change. I recommend settling and accepting the RFC first, then splitting implementation along the RFC's stated seams. That will also keep security review findings from churning under a moving design.
Source-backed findings
-
The default enabled startup path executes environment application imports in the privileged daemon. In
daemon.py:54-71, omitting--factorycauses the manifest'sappmodule to be imported before the worker is spawned or credentials are dropped. That import runs arbitrary module-level environment code with daemon/root privileges and access to the daemon environment, including the surface tokens. This undercuts the care in_worker_bootstrap.pyto seal the worker before third-party imports. Even if the factory itself is trusted, discovering it by importing the whole app expands the privileged TCB substantially. For enabled mode, require the factory/action identifiers as declarative metadata or explicit arguments; do not discover them by importing the app in the daemon. -
grader.fs_diffis not episode-atomic with reset.surfaces.py:131-145callsruntime.read_file/runtime.workspace.diffdirectly in worker threads, whileRuntime.reset()serializes restoration only throughruntime.lock(runtime.py:366-381). A diff can therefore scan while reset deletes and recopies the workspace. BecauseWorkspace.scan()intentionally suppresses per-pathOSError, the result can be a plausible but partial mixture rather than an explicit failure. Route on-demand grader filesystem operations through lock-aware runtime methods, or capture/check an episode generation and retry/fail if it changes. -
A privileged oracle descendant can survive oracle cleanup and episode reset.
_run_oracle()starts the oracle in a new session, but cleanup only callskillpg(proc.pid)(runtime.py:510-548). A descendant that callssetsid()leaves that process group; it is not in the workload cgroup, and_reap_orphans()only reaps already-exited children. A daemonizing or buggy trusted oracle can therefore leave a daemon-privileged process alive across resets. Put oracle execution in its own cgroup (or otherwise supervise the complete descendant tree) and require it to empty before returning/restoring.
What I verified
- Changed Python files pass
ruff check. - The portable openenvd suite is 105 passed / 19 skipped. Four socket tests initially hit the local workspace sandbox and passed when rerun outside it.
- I did not execute cgroup, UID, namespace, seccomp, nftables/NFLOG, veth, or privileged cleanup behavior on this macOS host. The new privileged-Linux workflow is not currently present in the PR's reported checks, so those paths still need an actual Linux-capability run before merge.
- Minor existing automation findings remain: the intentional helper
printneeds the repository suppression marker, and the new openenvd README is the one changed file flagged byruff format --checkfor embedded Python formatting.
My recommended order is: accept/narrow RFC 009, split the implementation, resolve the three lifecycle/boundary issues above in the relevant slices, then run the privileged Linux matrix. I would be happy to re-review those smaller pieces.
…/pr-1242 fix(ci): harden GitHub Actions workflows (#1242)
| stop.cancel() | ||
| release.set() | ||
| with pytest.raises(asyncio.CancelledError): | ||
| await stop |
| tasks.append(server_task) | ||
| while not local_server.started: | ||
| if server_task.done(): | ||
| await server_task |
| tasks.append(asyncio.create_task(handle())) | ||
| done, _ = await asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED) | ||
| for task in done: | ||
| await task |
| # Every transport has its own session, so always kill that whole group. | ||
| try: | ||
| os.killpg(process.pid, signal.SIGKILL) | ||
| except ProcessLookupError: |
| await asyncio.gather(*readers, return_exceptions=True) | ||
| try: | ||
| os.killpg(proc.pid, signal.SIGKILL) | ||
| except ProcessLookupError: |
| }, | ||
| } | ||
| ) | ||
| except WebSocketDisconnect: |
| message = await asyncio.wait_for(websocket.receive(), 0.01) | ||
| if message["type"] == "websocket.disconnect": | ||
| return | ||
| except asyncio.TimeoutError: |
| for process in processes: | ||
| try: | ||
| os.killpg(process.pid, 9) | ||
| except ProcessLookupError: |
| for change in snapshot_changes( | ||
| self.workspace.baseline, self.workspace.scan() | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Blocking file I/O on async event loop in fs_diff
Medium Severity
fs_diff calls self.workspace.baseline and self.workspace.scan() synchronously on the async event loop thread. These perform file-system walks, reads, and SHA-256 hashing — the same heavy I/O that _observe_loop correctly wraps in asyncio.to_thread. On a large workspace this blocks the entire event loop, stalling the health endpoint, observer WebSocket, and any in-flight agent or grader request.
Reviewed by Cursor Bugbot for commit 7629e16. Configure here.
| current = await asyncio.to_thread(self.workspace.scan) | ||
| for change in snapshot_changes(previous, current): | ||
| self.collector.record(ObservationEventType.FS_CHANGE, change) | ||
| previous = current |
There was a problem hiding this comment.
Observation loop holds lock during network download
Medium Severity
_observe_loop calls _sync_workspace() (which downloads the full workspace archive over SSH) while holding the runtime lock. Every one-second observation cycle blocks all agent tool calls, resets, and grading for the entire duration of the network transfer. The previous implementation explicitly performed observation I/O outside the lock with the comment "File hashing must not hold up actions or resets."
Reviewed by Cursor Bugbot for commit 7629e16. Configure here.
…kernel networking Supervise one workload per episode with separate agent, grader, orchestrator, and observer surfaces. Enforce process and filesystem boundaries, restore episode state, and collect privileged observations. Enforce default-deny egress with Linux network namespaces, veth interfaces, and nftables under Python supervision. Add explicit destination allowances, NFLOG policy observations, crash-safe network cleanup, and Linux integration tests.
Replace custom Linux namespace, firewall, and cgroup management with OpenShell-managed workload and oracle sandboxes. Preserve principal boundaries through a private SSH control channel, bounded workspace exports, and confirmed deletion before reset. Update configuration, setup documentation, and portable and live integration tests without changing RFC 009.
7629e16 to
d44a676
Compare
|
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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d44a676. Configure here.
| ) | ||
| self._closed = True | ||
| self.id = None | ||
| return |
There was a problem hiding this comment.
Failed create breaks sandbox cleanup
High Severity
OpenShellSandbox.close() treats a missing sandbox with no stored id as an unknown create outcome and raises. name is assigned before sandbox create, so a failed create (bad image, policy, gateway) still enters this path. That replaces the original error and makes Runtime.close() raise before it deletes the private tree, leaking copied privileged assets and SSH config.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d44a676. Configure here.
| if member.isdir(): | ||
| destination.joinpath(*name.parts).chmod( | ||
| stat.S_IMODE(member.mode) & 0o777 | ||
| ) |
There was a problem hiding this comment.
Snapshot modes block grader reads
Medium Severity
Workspace downloads reapply archive permission bits, including mode 000, onto the daemon-owned snapshot. Later grader reads open that tree as the daemon user with O_RDONLY, which Linux denies on a mode-000 root or directory. A workload can chmod the workspace and make grader.read_file, grader.fs_diff, and oracle staging fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d44a676. Configure here.
|
Superseded by the stacked RFC 009 PRs #1354 (contract and pluggable enforcement backends) and #1355 (daemon, surfaces and declared processes). They carry this PR's OpenShell adapter and daemon forward onto a backend interface, with declared guarantees that refuse rather than degrade. Closing in favor of those. |
Summary
Add an opt-in execution daemon for environments that need enforced separation between agent tools, privileged grading, orchestration, and observation. Supervise one workload per episode, restore its managed workspace on reset, and enforce default-deny egress using Linux namespaces, veth interfaces, and nftables under Python supervision. Existing environments retain their normal startup when openenvd is absent or disabled.
This is the complete RFC 009 implementation branch in one commit: 50 files, including the draft RFC, runtime, integration tests, and documentation. It delivers a bounded initial implementation rather than every guarantee proposed by the RFC.
Interactive Explainer & Video
These are illustrative simulations. The Harbor verifier handoff is explicitly marked as proposed integration work, not functionality delivered by this PR.
Type 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 issuesThe lint hook was attempted but cannot run because
uvis unavailable locally. The equivalent checks were run directly from.venvon the runtime, policy, and runtime tests; results are below. The full pre-submit workflow and full repository test suite were not run.RFC Status
Risk & Review Guidance
src/openenv/core/openenvd/isolation.py(identity, namespaces, seccomp);_network.py(rule ordering, address reservation, crash cleanup);runtime.pyandsurfaces.py(reset ordering and principal boundaries).Decisions & Alternatives
libseccomp/libnetfilter_loglibraries. This avoids introducing Go, gVisor, a userspace TCP/IP stack, or hand-maintained syscall tables.Test Plan
Completed on the implementation now contained in this commit:
PYTHONPATH=src:envs .venv/bin/pytest tests/core/test_openenvd*.py tests/test_validation -q --tb=short— 214 passed, 19 skipped (Linux-only checks unavailable on macOS).python -m pytest tests/core/test_openenvd*.py -q --tb=short— 124 passed, no exclusions. Covers actual TCP/UDP allowances and denial, NFLOG observations, protected destinations, teardown/crash cleanup, reset, identity separation, and seccomp enforcement..venvRuff lint/format and usort checks on the runtime, policy, and runtime tests — passed.PYTHONPATH=src .venv/bin/python scripts/sync_validation_schemas.py --check— passed;git diff --check— passed.Claude Code Review
Alignment review performed by Codex using the repository's alignment-review skill:
uv. The debug scan reports repository-wide print statements; the network supervisor's stderr diagnostic is intentional.env.trajectoryintegration, and normalized RFC 008 contract-grader integration remain follow-ups. These limitations are documented in the runtime README.Follow-ups
Detailed enforcement-readiness reporting, live network-policy updates, domain-based allowances, and credential handling are deferred. Provisioning, fleet management, PTYs, and Jupyter integration are outside this implementation's scope.
Note
High Risk
Adds a security-critical privileged daemon (authentication, sandbox lifecycle, grader/oracle execution, and workspace snapshots); misconfiguration or boundary bugs could leak privileged tools or assets to agents.
Overview
Introduces openenvd, an opt-in RFC 009 privileged sidecar that runs environments behind policy-scoped surfaces (agent, grader, orchestrator, observer) instead of relying only on convention to separate privileges.
Runtime and config: Shared declarative models live in
_openenvd_config.py(principals,SurfacePolicy, OpenShell episode config, privileged assets). TheopenenvdCLI/python -m openenv.core.openenvdloadsopenenv.yaml, and whenopenenvd.enabledis false the manifest app still starts unchanged. When enabled, NVIDIA OpenShell provisions per-episode sandboxes over SSH, seeds/sandbox/workspace, keeps grading assets on the daemon host, and enforces reset only after confirmed sandbox teardown. Worker bootstrap seals the control channel before environment imports; grader tools (read_file,fs_diff,run_oracle, etc.) and/observestreams are served fromsurfaces.pywith distinct bearer tokens.Integrations:
EnvClientaccepts optional WebSocket headers for orchestrator auth (errors avoid leaking credentials).create_appexposesapp.state.openenv_specfor factory discovery.MCPHarnessAdaptercan forward rollout events viaHarnessEventSink(OPENENVD_EVENT_FD). Manifest validation and JSON schemas now include anopenenvdblock; RFC 009 and docs/repo walkthrough are updated.CI and packaging: New GitHub workflow runs portable
test_openenvd*tests and optional live OpenShell adversarial checks; base image addsiproute2; echo-env test Dockerfile is included.Reviewed by Cursor Bugbot for commit d44a676. Bugbot is set up for automated code reviews on this repo. Configure here.