Skip to content

feat(cli): add openenv serve for local uvicorn#668

Open
false200 wants to merge 4 commits into
huggingface:mainfrom
false200:feat/cli-openenv-serve
Open

feat(cli): add openenv serve for local uvicorn#668
false200 wants to merge 4 commits into
huggingface:mainfrom
false200:feat/cli-openenv-serve

Conversation

@false200

Copy link
Copy Markdown

Related to meta-pytorch/OpenEnv#613

Adds an openenv serve command that reads openenv.yaml (app, port, runtime), validates the env layout, sets cwd and sys.path like a normal env run, optionally prepends the OpenEnv repo src/ when the env lives under a clone, and starts the app with uvicorn. Documents openenv serve . in the README. Adds a minimal envs/echo_env/models.py so validate_env_structure passes. Includes CLI tests (mocked uvicorn + optional subprocess /health on echo). Includes small import-order fixes in test_grid_world.py and test_julia_env.py so usort/ruff format stay stable.

Summary

Introduces openenv serve as the supported way to run a FastAPI OpenEnv app locally from an environment directory, aligned with manifest-driven deployment and existing validation.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • New environment
  • Refactoring

Alignment Checklist

Before submitting, verify:

  • I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
  • I have checked .claude/docs/INVARIANTS.md and no invariants are violated
  • I have run /pre-submit-pr (or bash .claude/hooks/lint.sh and tests) and addressed all issues

(Dev-only CLI; does not change WebSocket/MCP contracts or agent-facing APIs.)

RFC Status

  • Not required (bug fix, docs, minor refactoring)
  • RFC exists: #___
  • RFC needed (will create before merge)

Test Plan

  1. Lint (matches CI):
    uv run usort format src/ tests/ && uv run ruff format src/ tests/ → no diff;
    uv run usort check src/ tests/ && uv run ruff check src/ tests/

  2. Targeted tests:
    PYTHONPATH=src:envs uv run pytest tests/test_cli/test_serve.py -v

  3. Manual: from envs/echo_env (or any valid env with runtime: fastapi):
    openenv serve . --host 127.0.0.1 --port <port>GET /health returns 200.

Claude Code Review

N/A

@meta-cla

meta-cla Bot commented May 11, 2026

Copy link
Copy Markdown

Hi @false200!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@greptile-apps

greptile-apps Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR implements openenv serve, promoting the command from a placeholder stub to a working local uvicorn launcher that reads openenv.yaml (app, port, runtime), validates the env layout, adjusts sys.path and CWD, and starts the FastAPI app. Supporting additions include a stub models.py for echo_env, unit + integration tests, and a README entry.

  • serve.py: Parses the YAML manifest, validates runtime is fastapi, resolves the repo src/ heuristic, mutates sys.path and CWD in-process, then delegates to uvicorn.run — two issues: the global state mutations contaminate any in-process caller (including the CliRunner-based unit tests), and the integration test has a deadlock path when the health check deadline is exceeded.
  • Alignment concern: openenv serve runs environment code directly on the host, bypassing the Docker container-isolation invariant in INVARIANTS.md; this trade-off was not discussed in an RFC and warrants explicit sign-off from @darktex.
  • Import-order fixes in test_grid_world.py and test_julia_env.py are cosmetic and correct.

Confidence Score: 3/5

Not safe to merge without fixing the integration test deadlock and the in-process global-state mutations that corrupt test isolation.

The serve command calls os.chdir and sys.path.insert before handing off to uvicorn; since CliRunner runs in-process, these mutations persist across every subsequent unit test in the suite. Separately, the integration test calls proc.stdout.read() on a live subprocess pipe before proc.terminate(), hanging the suite indefinitely whenever the server fails to start within the deadline.

src/openenv/cli/commands/serve.py (global-state mutations) and tests/test_cli/test_serve.py (missing CWD/sys.path teardown in unit tests, deadlock in integration test).

Important Files Changed

Filename Overview
src/openenv/cli/commands/serve.py Implements openenv serve with YAML manifest parsing and uvicorn launch; global os.chdir + sys.path mutations executed in-process will contaminate test runners and any process that invokes this function without subprocess isolation.
tests/test_cli/test_serve.py New CLI tests for serve; unit tests use in-process CliRunner and don't restore CWD/sys.path, and the integration test has a deadlock when the health-check deadline is exceeded before proc.terminate() is called.
src/openenv/cli/main.py Minor reformatting of the serve command registration; help text updated from placeholder to accurate description — no logic changes.
envs/echo_env/models.py Adds a stub models.py so validate_env_structure passes for echo_env; intentionally empty beyond the docstring.
README.md Documents the new openenv serve . invocation alongside the existing uv run server command — accurate and minimal.
tests/envs/test_grid_world.py Removes a stale inline comment to fix import ordering for usort/ruff; no logic changes.
tests/envs/test_julia_env.py Removes two stale inline comments to fix import ordering for usort/ruff; no logic changes.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as openenv CLI
    participant Serve as serve()
    participant FS as Filesystem
    participant UV as uvicorn

    User->>CLI: openenv serve env_path [--port] [--host]
    CLI->>Serve: invoke serve()
    Serve->>FS: validate_env_structure(env_path)
    FS-->>Serve: OK / FileNotFoundError
    Serve->>FS: open openenv.yaml - yaml.safe_load()
    FS-->>Serve: manifest dict
    Serve->>Serve: validate app, runtime fields
    Serve->>Serve: _find_repo_src_for_openenv() - sys.path.insert()
    Serve->>Serve: sys.path.insert(env_root)
    Serve->>Serve: os.chdir(env_root)
    Serve->>UV: uvicorn.run(app_spec, host, port, reload)
    UV-->>User: HTTP server listening
Loading

Comments Outside Diff (1)

  1. src/openenv/cli/commands/serve.py, line 1-30 (link)

    P2 ALIGNMENT FLAG: openenv serve bypasses Docker container isolation

    INVARIANTS.md §Security Invariants states: "Environments run in isolated Docker containers. Containers must not have access to host filesystem." openenv serve runs the FastAPI app directly in the host Python process — no container, no filesystem isolation, no network namespace. This trade-off was not discussed in an RFC and warrants explicit sign-off.

    • Principle at stake: Container isolation (INVARIANTS.md §Security)
    • The concern: Running env code on the host bypasses isolation guarantees and could normalise "no-container" deployments
    • Suggested reviewer: @darktex
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/openenv/cli/commands/serve.py
    Line: 1-30
    
    Comment:
    **ALIGNMENT FLAG: `openenv serve` bypasses Docker container isolation**
    
    INVARIANTS.md §Security Invariants states: *"Environments run in isolated Docker containers. Containers must not have access to host filesystem."* `openenv serve` runs the FastAPI app directly in the host Python process — no container, no filesystem isolation, no network namespace. This trade-off was not discussed in an RFC and warrants explicit sign-off.
    
    - **Principle at stake**: Container isolation (INVARIANTS.md §Security)
    - **The concern**: Running env code on the host bypasses isolation guarantees and could normalise "no-container" deployments
    - **Suggested reviewer**: `@darktex`
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
tests/test_cli/test_serve.py:124-126
**`proc.stdout.read()` deadlocks when the server never becomes healthy**

When the 60-second deadline expires with `ok = False`, line 125 calls `proc.stdout.read()` on a pipe connected to a still-running process. Because nothing has terminated the subprocess yet (`proc.terminate()` is in the `finally` block, which only executes *after* control leaves the `try` body), the read blocks indefinitely — `pytest.fail` is never reached and neither is the `finally` cleanup. Terminate the process before reading its output.

### Issue 2 of 3
src/openenv/cli/commands/serve.py:107-117
**Global process-state mutation breaks in-process test isolation**

`sys.path.insert` and `os.chdir` are irreversible global side effects. Because `typer.testing.CliRunner` runs the command in the *same* process (not a subprocess), every unit test that calls `runner.invoke` with `serve` will permanently move the test runner's CWD to `env_root` and prepend entries to `sys.path`. Tests that depend on the original CWD or a clean path (including subsequent tests in this very module) can silently break or import the wrong module version. The cleanest fix is to exec a child process so all global-state mutations are isolated, or add an autouse fixture that saves and restores `os.getcwd()` and `sys.path`.

### Issue 3 of 3
src/openenv/cli/commands/serve.py:1-30
**ALIGNMENT FLAG: `openenv serve` bypasses Docker container isolation**

INVARIANTS.md §Security Invariants states: *"Environments run in isolated Docker containers. Containers must not have access to host filesystem."* `openenv serve` runs the FastAPI app directly in the host Python process — no container, no filesystem isolation, no network namespace. This trade-off was not discussed in an RFC and warrants explicit sign-off.

- **Principle at stake**: Container isolation (INVARIANTS.md §Security)
- **The concern**: Running env code on the host bypasses isolation guarantees and could normalise "no-container" deployments
- **Suggested reviewer**: `@darktex`

Reviews (1): Last reviewed commit: "feat(cli): add openenv serve for local u..." | Re-trigger Greptile

Comment on lines +124 to +126
if not ok:
out = proc.stdout.read() if proc.stdout else ""
pytest.fail(f"/health never OK (last error={last_exc!r}): {out}")

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.

P1 proc.stdout.read() deadlocks when the server never becomes healthy

When the 60-second deadline expires with ok = False, line 125 calls proc.stdout.read() on a pipe connected to a still-running process. Because nothing has terminated the subprocess yet (proc.terminate() is in the finally block, which only executes after control leaves the try body), the read blocks indefinitely — pytest.fail is never reached and neither is the finally cleanup. Terminate the process before reading its output.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_cli/test_serve.py
Line: 124-126

Comment:
**`proc.stdout.read()` deadlocks when the server never becomes healthy**

When the 60-second deadline expires with `ok = False`, line 125 calls `proc.stdout.read()` on a pipe connected to a still-running process. Because nothing has terminated the subprocess yet (`proc.terminate()` is in the `finally` block, which only executes *after* control leaves the `try` body), the read blocks indefinitely — `pytest.fail` is never reached and neither is the `finally` cleanup. Terminate the process before reading its output.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b56360c commit

Comment on lines +107 to +117
repo_src = _find_repo_src_for_openenv(env_path_obj)
if repo_src is not None:
repo_src_str = str(repo_src.resolve())
if repo_src_str not in sys.path:
sys.path.insert(0, repo_src_str)

env_root = str(env_path_obj.resolve())
if env_root not in sys.path:
sys.path.insert(0, env_root)

os.chdir(env_root)

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.

P1 Global process-state mutation breaks in-process test isolation

sys.path.insert and os.chdir are irreversible global side effects. Because typer.testing.CliRunner runs the command in the same process (not a subprocess), every unit test that calls runner.invoke with serve will permanently move the test runner's CWD to env_root and prepend entries to sys.path. Tests that depend on the original CWD or a clean path (including subsequent tests in this very module) can silently break or import the wrong module version. The cleanest fix is to exec a child process so all global-state mutations are isolated, or add an autouse fixture that saves and restores os.getcwd() and sys.path.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/openenv/cli/commands/serve.py
Line: 107-117

Comment:
**Global process-state mutation breaks in-process test isolation**

`sys.path.insert` and `os.chdir` are irreversible global side effects. Because `typer.testing.CliRunner` runs the command in the *same* process (not a subprocess), every unit test that calls `runner.invoke` with `serve` will permanently move the test runner's CWD to `env_root` and prepend entries to `sys.path`. Tests that depend on the original CWD or a clean path (including subsequent tests in this very module) can silently break or import the wrong module version. The cleanest fix is to exec a child process so all global-state mutations are isolated, or add an autouse fixture that saves and restores `os.getcwd()` and `sys.path`.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in b56360c commit.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label May 11, 2026
@false200

Copy link
Copy Markdown
Author

Issue 3: host vs Docker

Hi @Darktex, openenv serve is just local dev (same idea as running uvicorn from the env dir), not “skip Docker in prod.” For isolation / real deployments, I’m still treating Docker / Spaces as the right path per INVARIANTS.

Happy to add a one line note in serve()’s docstring or README if you want that spelled out in tree.

@Darktex Darktex 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.

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Summary

The core implementation is correct and well-structured. One Tier 1 bug (unguarded int() on untrusted YAML input) and one Tier 1 test-hygiene issue (module-level import requests that blocks the mocked unit tests when requests is absent) should be fixed before merge. The claimed "import-order fixes" in test_grid_world.py / test_julia_env.py are misleading — they only remove comments, not the actual usort ordering violations. Two Tier 2 alignment points for maintainer consideration.


Automated Checks

  • Lint (changed files): PASS — usort/ruff clean on src/openenv/cli/ and tests/test_cli/.
  • Debug code: CLEAN.
  • Pre-existing usort failures in tests/envs/test_grid_world.py and tests/envs/test_julia_env.py remain unfixed despite the PR's claim (see Tier 1).

Tier 1: Fixes Required

  • src/openenv/cli/commands/serve.py — unguarded int() on manifest input. int(port if port is not None else manifest.get("port", 8000)) raises an unhandled ValueError (raw traceback, not a clean CLI error) if openenv.yaml has a non-integer port (e.g. port: "auto"). Wrap in try/except ValueError and raise typer.BadParameter.
  • tests/test_cli/test_serve.py — module-level import requests. This makes the entire test module fail to import when requests is not installed, blocking the mocked unit tests that don't need it. Move the import inside the integration test (test_serve_echo_env_health_subprocess) or use requests = pytest.importorskip("requests") there.
  • tests/envs/test_grid_world.py, tests/envs/test_julia_env.py — claimed "import-order fixes" don't fix the violation. usort check still flags both files after the diff; the changes only removed inline comments. Either run usort format to actually fix the ordering, or drop the claim from the PR description.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: serve bypasses the container-isolation invariant by design — needs explicit acknowledgment.

  • Principle at stake: "Container isolation for reproducibility and security" (PRINCIPLES.md); INVARIANTS.md §Security — environments run in isolated Docker containers.
  • Concern: openenv serve runs the FastAPI app directly in the host Python process, permanently mutating the host's sys.path and os.getcwd(). The PR explicitly targets a "dev-only" use case (reasonable), but there's no warning that the user is bypassing the canonical isolated execution model. A --dev flag, a console warning, or a help-text note acknowledging this divergence would preserve alignment with the isolation principle without blocking the feature.

ALIGNMENT FLAG: echo_env reference now needs a stub models.py to pass validate_env_structure.

  • Principle at stake: PATTERNS.md — models.py holds Action/Observation/State models; INVARIANTS.md §Architectural — client-server separation.
  • Concern: echo_env intentionally reuses openenv.core.env_server.mcp_types and has no custom models. The added envs/echo_env/models.py is a content-free stub added solely to satisfy validate_env_structure's file-presence check — meaning the validator now enforces a rule the canonical reference implementation can't satisfy without a workaround. Prefer making validate_env_structure accept MCP-only envs without a local models.py, or document echo_env as deliberately exempt, rather than papering over the mismatch.

Verdict: request_changes — fix the int() guard and the test-import issue, and reconcile the misleading "import-order fix" claim; the alignment flags are for maintainer decision.


Automated review by Claude Code | Learn more

@false200

Copy link
Copy Markdown
Author

Thank you for the detailed review, @Darktex! I'll review the feedback and make the necessary fixes.

@Darktex Darktex 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.

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint (usort + ruff): PASS — new serve.py and models.py are clean; subprocess, time, and requests imports in test_serve.py are all used in the integration test (test_serve_echo_env_health_subprocess).
  • Debug code: CLEAN — no debug artifacts introduced.

Tier 1: Notes (no blocking bugs found)

Minor: _restore_cwd_and_syspath fixture has incorrect return annotation (tests/test_cli/test_serve.py)

The function uses yield, so it is a generator function. Its annotation -> None is inaccurate; it should be -> Generator[None, None, None] (or -> Iterator[None]). Does not fail CI (ANN rules not enabled), but mypy would flag it.

Minor: env={..., "PYTHONPATH": ...} in mock-based tests is a no-op (tests/test_cli/test_serve.py)

CliRunner runs in-process; serve.py resolves imports via sys.path mutation, not PYTHONPATH. The tests pass, but the env= kwarg is misleading and can be removed from test_serve_calls_uvicorn_with_echo_manifest and test_serve_uses_manifest_port_when_omitted.

Minor: malformed manifest port raises an unfriendly error (src/openenv/cli/commands/serve.py)

int(port if port is not None else manifest.get("port", 8000)) raises a bare ValueError if openenv.yaml has a non-numeric port. Consider wrapping in try/except ValueErrortyper.BadParameter. Low priority (known manifests use integer ports).

Minor: --reload with a string app and mutated sys.path likely fails (src/openenv/cli/commands/serve.py)

uvicorn reload mode spawns a worker subprocess that does not inherit in-process sys.path mutations, so openenv serve . --reload may fail to import the app module. Suggest passing app_dir=env_root to uvicorn.run, which uvicorn uses to set the import path in string-import mode (and makes the os.chdir / sys.path.insert redundant for the non-reload case too).

uvicorn.run(app_spec, host=host, port=listen_port, reload=reload, app_dir=env_root)

Tier 2: Alignment Discussion

ALIGNMENT FLAG: openenv serve bypasses container isolation for local development

  • Principle at stake: "Container isolation for reproducibility and security" (PRINCIPLES.md); "Environments run in isolated Docker containers" (INVARIANTS.md §Security Invariants 2).
  • Concern: openenv serve runs the FastAPI app directly on the host via sys.path/cwd manipulation, with no Docker. Intentional for dev ergonomics, but the command is not gated/labeled --dev or experimental. A user who wires it into a training loop would bypass the isolation guarantees. Worth a brief alignment discussion.

ALIGNMENT FLAG: _find_repo_src_for_openenv auto-injects OpenEnv's own src/ into sys.path

  • Principle at stake: "Minimize lifecycle deltas" (PRINCIPLES.md §1) — training vs production should use identical interfaces.
  • Concern: Auto-discovery walks parents for src/openenv and prepends the dev tree when the env lives inside the monorepo, but uses the installed openenv-core package otherwise. The same openenv serve invocation silently loads different code depending on location, which can mask bugs that only appear with the installed package.

Summary

Functionally correct and well-structured. The echo_env/models.py stub is the right fix for validate_env_structure; YAML parsing, path manipulation, manifest validation, and uvicorn invocation are all handled correctly. The --reload + sys.path gap is the most impactful (non-blocking) note to address before merge. The two Tier 2 flags are design questions for human review.

  • 0 blocking Tier 1 issues
  • 4 non-blocking Tier 1 notes
  • 2 Tier 2 alignment points for human review

Automated review by Claude Code | Learn more

@false200 false200 force-pushed the feat/cli-openenv-serve branch 2 times, most recently from cbc4eeb to 2037b58 Compare June 14, 2026 07:54
false200 and others added 3 commits June 14, 2026 13:25
Related to huggingface#613

- Load app/host/port from openenv.yaml and run uvicorn with env cwd on sys.path
- Prepend repo src when the env path sits under an OpenEnv clone
- Register serve on the Typer CLI; document usage in README
- Add echo_env models.py stub required by validate_env_structure
- Add tests for serve (mocked uvicorn + echo /health subprocess)
Related to huggingface#613

- Autouse fixture restores cwd and sys.path after in-process CliRunner invokes
- Terminate subprocess before reading stdout when /health deadline exceeded (Greptile P1)
@false200 false200 force-pushed the feat/cli-openenv-serve branch from 2037b58 to 4a0d00c Compare June 14, 2026 07:56
@false200 false200 requested a review from Darktex June 14, 2026 07:59
@false200

Copy link
Copy Markdown
Author

@Darktex! I’ve made the required changes, could you please review and approve?

@Darktex Darktex 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.

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint (usort + ruff format + ruff check on changed files): PASS
  • Debug code: CLEAN — no new debug artifacts in the PR diff

Tier 1: Fixes Required

  • src/openenv/cli/commands/serve.pyPort range not validated. listen_port = int(raw_port) accepts 0, negative values, and values above 65535. Add a guard such as if not (1 <= listen_port <= 65535): raise typer.BadParameter(...) after the int() conversion.

  • src/openenv/cli/commands/serve.py:144-147Misleading ImportError message. uvicorn>=0.24.0 is a declared dependency in pyproject.toml (lines 17 and 39), so this except ImportError branch is dead under any normal install, and its message ("Install openenv-core with default dependencies") will confuse the rare user who does hit it (e.g. a custom venv). Prefer something like "uvicorn is not installed. Run: pip install 'uvicorn>=0.24.0'".

  • src/openenv/cli/commands/serve.py:205os.chdir() mutates global process state permanently. After uvicorn.run() returns (normal exit, --reload restart, or future programmatic use of serve()), the working directory stays changed. Harmless for a one-shot CLI binary, but if serve() is ever called programmatically or in a test without the fixture restoring cwd, it silently corrupts the environment. At minimum document the intentional mutation; ideally wrap uvicorn.run() in try/finally that restores cwd. The source of truth should be in the implementation, not just the test fixture.

  • tests/test_cli/test_serve.py:272Incorrect return-type annotation on a yield fixture. _restore_cwd_and_syspath() -> None uses yield, making it a generator-based fixture. The correct annotation is -> Generator[None, None, None] (from collections.abc import Generator) or -> Iterator[None]. With from __future__ import annotations this isn't a runtime crash, but ruff ANN rules / mypy will flag it. The repo's existing fixtures simply omit the annotation; do the same or fix it.


Tier 2: Alignment Discussion

None identified. This PR is scoped entirely to the developer-facing CLI layer. It does not touch WebSocket/MCP contracts, does not expose reset()/step() to agents, does not import server code from client modules, and does not alter reward computation. The _find_repo_src_for_openenv heuristic is a development convenience (it only activates when the env lives inside an OpenEnv repo clone) and is consistent with making local development easy. No RFC is needed.


Summary

  • 4 mechanical issues to fix (port validation, misleading ImportError message, os.chdir documentation/hygiene, fixture return type)
  • 0 alignment points for human review

The feature is a clean, useful addition. None of the findings are conceptual blockers — they're correctness/hygiene fixes that should be addressed before merge.


Automated review by Claude Code | Learn more

@false200 false200 requested a review from Darktex June 14, 2026 13:16

@Darktex Darktex 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.

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint (usort/ruff on new files): PASS - src/openenv/cli/commands/serve.py and tests/test_cli/test_serve.py are clean per usort check and ruff check/ruff format --check
  • Debug code: CLEAN - no print, breakpoint, or pdb in the new code
  • Pre-existing failures: usort reports Would sort tests/envs/test_grid_world.py and tests/envs/test_julia_env.py on current main. The PR description claims these are included as import-order fixes, but they do not appear in the diff. Either the diff is incomplete or the claim is inaccurate — clarify before merge.

Open RFCs Context

No open RFCs specifically cover the CLI serve command. RFC 000 describes openenv serve as a Phase 4 capability (deferred). The PR author checks "Not required" for RFC, noting this is a dev-only CLI. Reasonable — the change does not touch WebSocket/MCP contracts — but the Phase 4 deferral is worth noting.

Tier 1: Fixes Required

  • tests/test_cli/test_serve.py:344-347 - test_serve_uses_manifest_port_when_omitted accesses mock_run.call_args without first asserting result.exit_code == 0. If the command fails, mock_run.call_args is None and the unpack _, kwargs = mock_run.call_args raises TypeError: cannot unpack non-iterable NoneType, hiding the real failure. Add assert result.exit_code == 0, result.stdout before the call_args access, consistent with test_serve_calls_uvicorn_with_echo_manifest (line 319).

  • src/openenv/cli/commands/serve.py - Missing app = typer.Typer(...) module-level object. Every other command module (push.py, build.py, init.py, fork.py) keeps its own typer.Typer instance alongside the bare function; the PR drops it from serve.py. While nothing currently references serve.app, this breaks the established intra-module pattern and makes serve.py an outlier. Either add the app object back, or document the intentional deviation.

  • envs/echo_env/models.py (new file) - The file is a stub containing only a module docstring and no Python definitions. validate_env_structure only checks existence, so this passes today, but the docstring states echo reuses types from openenv.core.env_server.mcp_types. Add a clarifying comment (or __all__ = [] sentinel) so future contributors don't think it needs filling in.

  • Diff vs PR description mismatch: The PR body says it "Includes small import-order fixes in test_grid_world.py and test_julia_env.py" but these files do not appear in the diff. Confirm whether those fixes are actually in the branch and were omitted from the diff artifact, or if the PR description is inaccurate.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: Process-global sys.path mutation not reverted on exit

  • Principle at stake: "Minimize lifecycle deltas" (RFC 000); INVARIANTS.md §Security — no unintended host-side effects
  • The concern: src/openenv/cli/commands/serve.py (~lines 228-235) inserts paths into sys.path before calling uvicorn.run(). The finally block (~lines 247-251) restores os.cwd but does NOT restore sys.path. For in-process use (e.g. the test suite via CliRunner), this permanently pollutes sys.path for the rest of the process. The test file correctly guards against this with an autouse fixture, but that relies on test authors knowing about the side effect. If serve is ever called programmatically, the mutation is permanent. Fix: wrap sys.path mutation in the same try/finally as os.chdir, or document that serve is only safe to call once per process.
  • Suggested reviewer: @Darktex

ALIGNMENT FLAG: RFC 000 explicitly deferred openenv serve to a later phase

  • Principle at stake: RFC-first design for architectural changes (CLAUDE.md §Design Context)
  • The concern: RFC 000 describes the CLI serve command as Phase 4 work. The PR checks "RFC Not required" on the basis that this is a minor dev-only feature. Implementing a previously-deferred command re-opens a scoped deferral. The implementation is functionally sound and dev-only, but the decision to un-defer should be acknowledged — a note in the PR body pointing to RFC 000 Phase 4 (and the accepting issue, e.g. #613) would satisfy this concern. No new RFC is likely needed.
  • Suggested reviewer: @Darktex

Summary

  • 4 mechanical issues to fix (missing assert in test, missing app object in module, stub file clarity, diff/description mismatch)
  • 2 alignment points for human review (sys.path side-effect scope, RFC 000 deferral acknowledgment)

Automated review by Claude Code | Learn more

@Darktex Darktex 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.

Note: This is an automated review by Claude Code, not a human review.


Alignment Review Report

Automated Checks

  • Lint: PASS (new file follows the repo's usort/ruff import-ordering conventions)
  • Debug code: CLEAN (no leftover print, breakpoint, or TODO debug artifacts)

Tier 1: Fixes Required

  • src/openenv/cli/commands/serve.py:152-157Wrong exception type for ImportError. typer.BadParameter is for invalid CLI parameter values; it formats the error as "Invalid value: uvicorn is not installed …" and exits with code 2. A missing optional dependency should use typer.echo("Error: uvicorn is not installed …", err=True); raise typer.Exit(1), consistent with how the rest of the codebase treats runtime-missing-dependency conditions.

  • src/openenv/cli/commands/serve.py:246Unhandled OSError when the port is in use. uvicorn.run() raises OSError: [Errno 98] Address already in use when the port is occupied. This propagates past the finally block (cwd is correctly restored) and bubbles up to typer's top-level handler as a raw Python traceback. Wrap uvicorn.run(...) in try/except OSError as exc: raise typer.Exit(1) with a user-facing message, consistent with how other commands wrap subprocess calls.

  • tests/test_cli/test_serve.py:317,342PYTHONPATH in CliRunner.invoke(env=...) is a no-op. CliRunner sets env vars for subprocesses but does not re-initialise sys.path for the in-process invocation, so the env={"PYTHONPATH": …} argument is misleading dead code in the unit tests. The _find_repo_src_for_openenv heuristic already handles in-process path injection. Remove the env= argument from the unit tests (keep it for the subprocess integration test where it is actually effective).

  • envs/echo_env/models.py (new) — Stub breaks the PATTERNS.md contract. PATTERNS.md specifies models.py defines Action, Observation, and State Pydantic models. The added file is a docstring-only stub. If echo_env genuinely uses MCP types rather than its own models, document this exception explicitly (e.g., re-export/alias the types it uses) and add a comment to validate_env_structure acknowledging that models.py may be a delegation stub. As-is, this normalises an empty models.py as acceptable for every environment.


Tier 2: Alignment Discussion

ALIGNMENT FLAG: openenv serve bypasses container isolation as a first-class command

  • Principle at stake: INVARIANTS.md §Security — "Environments run in isolated Docker containers. Containers must not have access to host filesystem (except explicitly mounted volumes)." PRINCIPLES.md — "Container isolation for reproducibility and security."
  • The concern: The command launches the FastAPI app directly on the host Python process with host filesystem access, no network isolation, and no resource limits. The in-code disclaimer ("local development only") is a docstring and a console print, not an enforcement mechanism. Because openenv serve is now a first-class CLI command alongside openenv build, new users and CI pipelines may reach for it where container isolation is contractually required (e.g., as a drop-in for openenv build && docker run). No RFC covers when host-mode serving is acceptable.
  • Suggested reviewer: @Darktex

ALIGNMENT FLAG: _find_repo_src_for_openenv performs permanent sys.path mutation in the host process

  • Principle at stake: INVARIANTS.md §Architectural — CLI commands should be self-contained and not leave side-effects in the calling process; also relates to reproducibility.
  • The concern: sys.path.insert(0, repo_src_str) / sys.path.insert(0, env_root) modify the process's module search path for its lifetime. When openenv serve is invoked programmatically (a test harness or another tool), this can silently shadow installed packages and cause subtle import bugs. The test fixture restores sys.path, but production code paths have no such restoration. Consider scoping the path manipulation to a subprocess rather than mutating the CLI process's sys.path.
  • Suggested reviewer: @Darktex

Summary

  • 4 mechanical issues to fix (wrong exception type, unhandled port-in-use OSError, dead test code, stub contract gap)
  • 2 alignment points for human review (container-isolation bypass as a first-class command, permanent sys.path mutation)

The command is a genuinely useful local-dev affordance and the implementation is clean overall — the fixes above are about error-handling rigor and making the "local dev only" boundary explicit rather than advisory.


Automated review by Claude Code | Learn more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants