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

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 ValueError → typer.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

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.py — Port 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-147 — Misleading 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:205 — os.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:272 — Incorrect 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

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

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-157 — Wrong 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:246 — Unhandled 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,342 — PYTHONPATH 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

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

This PR implements the previously-stubbed openenv serve command, replacing the "not yet implemented" placeholder with a working uvicorn launcher that reads openenv.yaml, validates env layout, sets up sys.path and cwd, and calls uvicorn.run. The design is clean and well-tested, including a properly guarded @pytest.mark.integration subprocess test. No invariants are violated — this is a pure CLI/developer-ergonomics feature that does not touch the WebSocket/MCP boundary or any agent-facing interface. A few items below warrant fixing or discussion before merge.

Tier 1: Bugs & Correctness

• src/openenv/cli/commands/serve.py — sys.path mutation is not restored on exit. The finally block (~line 247) restores os.getcwd() but not sys.path. When the CLI is used in-process (e.g., via CliRunner in tests), this leaks mutations across test cases — the test fixture in tests/test_cli/test_serve.py:287 restores sys.path[:] = path itself, which confirms the authors knew. The implementation should be self-contained: capture original_path = list(sys.path) before the mutations and restore with sys.path[:] = original_path in the finally block alongside the os.chdir.

• envs/echo_env/models.py — stub file added to satisfy validate_env_structure but contains no exports. validate_env_structure (_cli_utils.py:39) checks presence only, not imports. The stub is comment-only with no __all__/re-exports. It diverges from PATTERNS.md which says models.py defines Action/Observation/State types. Consider a brief docstring clarifying where echo's types live, or re-exporting from mcp_types.

(Noted and dismissed on closer inspection: the app_spec empty-string check at serve.py:199 is correct behavior; the stale TODO in __main__.py is correctly updated by the diff.)

Tier 2: Alignment

ALIGNMENT FLAG: Default host binding to 0.0.0.0.

• Principle at stake: Security Invariant — container isolation / no inadvertent network exposure.
• The concern: --host defaults to "0.0.0.0" (all interfaces). Now that the command actually starts a server, a developer running openenv serve . on a shared/cloud box (CI, devcontainer, EC2) would inadvertently expose the Gym-like /reset and /step endpoints on all interfaces. The safer default for a dev-only command is "127.0.0.1", with --host 0.0.0.0 as an explicit opt-in. This is the most impactful design question in the PR and warrants explicit sign-off.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: _find_repo_src_for_openenv walks parent directories to inject src/openenv onto sys.path (serve.py:77-82).

• Principle at stake: Client-server separation.
• The concern: The check (parent / "src" / "openenv").is_dir() is path-based only; it does not distinguish openenv.core from openenv.cli from server internals. In practice unlikely to cause an actual violation (the env server runs server code by design), but the intent and scope of the path injection should be documented in the code comment.
• Suggested reviewer: @Darktex

Verdict

One concrete Tier-1 fix recommended (restore sys.path in the finally block) plus the echo_env/models.py stub note. The 0.0.0.0 host default is the most impactful design question — defaulting to all-interfaces in a newly-activated developer command is a meaningful security-posture choice that should be resolved before merge. None of these are hard blockers to the feature's correctness, so: comment (please address the sys.path restore and confirm the host default).

Automated review by Claude Code | Learn more

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

Tier 1: Bugs & Correctness

Bug: sys.path mutation outside try/finally scope

src/openenv/cli/commands/serve.py:122-152

original_path = list(sys.path)        # save
...
sys.path.insert(0, repo_src_str)       # mutate
...
sys.path.insert(0, env_root)           # mutate
os.chdir(env_root)                     # cwd change — can raise OSError
try:                                   # protection starts AFTER mutation
    ...
finally:
    os.chdir(prev_cwd)
    sys.path[:] = original_path        # restore

If os.chdir(env_root) raises OSError (e.g., the directory was removed after validate_env_structure passed), sys.path is left permanently dirty for the rest of the process. Fix: move the sys.path mutation inside the try block so the finally always restores it.

Minor: redundant uvicorn import guard

src/openenv/cli/commands/serve.py:64-71 — The try: import uvicorn / except ImportError block (# pragma: no cover) is unreachable: uvicorn>=0.24.0 is a hard dependency in the main pyproject.toml. Consider removing it, or drop the # pragma: no cover and add a test asserting the error message if it must stay.

Minor: test subprocess zombie on SIGKILL path

tests/test_cli/test_serve.py:434-438 — After proc.kill() in the TimeoutExpired handler, if the subsequent proc.wait(timeout=5) itself times out there is no reap, leaving a zombie in CI. Consider communicate(timeout=...) or a best-effort proc.stdout.close().

Observation: PR summary lists files not in the diff

The description mentions "import-order fixes in test_grid_world.py and test_julia_env.py" but neither file appears in the diff — stale description or squashed out. If the fixes exist on the branch, they should be included.

Lint / format / debug checks

• Lint (ruff): PASS — all changed files pass ruff check.
• Format (ruff format): PASS.
• Debug code: CLEAN.

Tier 2: Alignment

ALIGNMENT FLAG: openenv serve runs the app directly on the host (local-dev escape hatch)

• Principle at stake: INVARIANTS.md §Security — "Environments run in isolated Docker containers."
• The concern: openenv serve deliberately runs the FastAPI app on the host, mutating sys.path and cwd. It is explicitly framed as local-development-only ("For production or training, use Docker (openenv build)") and does not expose reset/step to agents or violate the dual-API boundary. The concern is purely whether normalizing host-based serving via an official CLI surface could bleed into training pipelines. The docstring/README guard is reasonable but worth a deliberate human sign-off.

No other Tier 2 issues. Client-server separation is respected: serve.py imports only from openenv.cli._cli_utils and stdlib/third-party — no env server/ import. The manifest-driven app field pattern is consistent with openenv push/openenv build.

Solid implementation. The sys.path-restore bug is the one real fix; the rest are non-blocking. The host-serving alignment flag is for human decision.

Automated review by Claude Code | 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 src/openenv/cli/ and tests/test_cli/): PASS - no violations
• Debug code: CLEAN - no debug artifacts introduced by this PR

Tier 1: Fixes Required

• src/openenv/cli/commands/serve.py:96 — import uvicorn at module load will fail for openenv[cli]-only installs

  uvicorn is declared under [project.optional-dependencies] core in pyproject.toml (line 39), but not under cli (lines 43-51). The new serve.py adds import uvicorn as a top-level import, meaning any CLI invocation (even openenv --help) will raise ModuleNotFoundError for users who installed openenv-core[cli] without the core extra.

  Two acceptable fixes — pick one:

  Option A — lazy import (preferred, no dependency change needed):

  # inside serve(), just before uvicorn.run(...)
  try:
      import uvicorn  # noqa: PLC0415
  except ModuleNotFoundError as exc:
      raise typer.BadParameter(
          "uvicorn is required for 'openenv serve'. Install it with: pip install uvicorn"
      ) from exc

  Option B — add uvicorn>=0.24.0 to the cli extras in pyproject.toml. This makes sense semantically since serve is a CLI feature.

  Option B is cleaner because the README already shows openenv serve . as a first-class workflow. If the CLI promises serve, the cli extra should include its runtime dependency.

Tier 2: Alignment Discussion

None identified.

This PR is a developer-tooling CLI command only. It does not touch WebSocket/MCP APIs, does not expose reset/step/state to agents, does not cross the client-server boundary, and all rewards/simulation logic remains server-side. The _find_repo_src_for_openenv heuristic is dev-time only and does not affect production (Docker) paths. The openenv.yaml-driven design is consistent with the manifest-driven deployment principle.

Summary

• 1 mechanical issue to fix (missing uvicorn in cli extras / top-level import)
• 0 alignment points for human review

Automated review by Claude Code | Learn more

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

Alignment Review Report

Automated Checks

• Lint (existing files touched by PR): PASS — _cli_utils.py, __main__.py, and current serve.py pass ruff check and usort check cleanly.
• Debug code: CLEAN — no debug artifacts introduced by this PR.
• New files (echo_env/models.py, tests/test_cli/test_serve.py): reviewed from diff; no visible lint violations.
• Pre-existing usort failures in tests/envs/test_julia_env.py and tests/envs/test_grid_world.py are NOT fixed by this PR despite the PR description claiming they are — see Tier 1.

Tier 1: Fixes Required

[ ] tests/test_cli/test_serve.py (~lines 427–429) — ValueError on closed stdout

In test_serve_echo_env_health_subprocess, the except subprocess.TimeoutExpired branch after proc.kill() calls proc.stdout.close(), then immediately reads proc.stdout.read() without guarding for a closed file. On a closed io.TextIOWrapper that raises ValueError: I/O operation on closed file. Fix:

out = ""
try:
    if proc.stdout and not proc.stdout.closed:
        out = proc.stdout.read()
except (OSError, ValueError):
    pass

[ ] src/openenv/cli/commands/serve.py — --reload mode silently fails in uvicorn spawn workers

Uvicorn's reloader uses multiprocessing.get_context("spawn") (uvicorn/_subprocess.py), which spawns a fresh Python interpreter. spawn does not inherit parent sys.path mutations. The PR manually prepends env_root to sys.path before calling uvicorn.run(), but the spawned worker starts with a clean path → ModuleNotFoundError for the environment app when --reload is active. The mocked unit tests cannot catch this.

Fix: pass app_dir=env_root to uvicorn.run() (uvicorn serializes this into the pickled Config applied in the worker):

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

The repo_src prepend (for import openenv in dev-clone scenarios) survives only via a PYTHONPATH env-var mutation set before uvicorn.run. Please resolve explicitly.

[ ] src/openenv/cli/commands/serve.py — missing exists() / is_dir() guard (pattern divergence)

build.py and validate.py both check env_path_obj.exists() and env_path_obj.is_dir() before validating, producing clear error messages. serve.py delegates directly to validate_env_structure, which emits FileNotFoundError("Required file missing: openenv.yaml") when the directory doesn't exist at all — a confusing diagnostic. Add the standard two-line guard consistent with sibling commands.

[ ] PR description claims test_grid_world.py / test_julia_env.py import-order fixes are included — they are not

Neither file appears in the diff or PR file list, and usort check still fails on both. The test-plan statement "lint stays stable" is inaccurate. Remove the claim or include the fixes.

Tier 2: Alignment Discussion

ALIGNMENT FLAG: openenv serve lands as a first-class command without an RFC, replacing an explicitly deferred stub

• Principle at stake: PRINCIPLES.md — "Container isolation for reproducibility and security"; the previous stub was explicitly deferred ("TODO: Phase 4") with users directed to Docker or uv run.
• The concern: Making host-process serving a named, documented CLI command may signal that running directly on the host is a supported path beyond quick iteration, undermining the container-isolation invariant. The docstring says "For production or training, use Docker" but this caveat may not be prominent enough. The decision to un-defer this command and what "Phase 4" gating meant should be confirmed.
• Suggested reviewer: @Darktex

ALIGNMENT FLAG: envs/echo_env/models.py added as a side-effect of a CLI feature PR, with a non-canonical pattern

• Principle at stake: PATTERNS.md — "Every environment follows this structure: models.py — Action, Observation, State (Pydantic)"; CLAUDE.md — "echo_env is the reference implementation to study".
• The concern: The file exists solely to make validate_env_structure pass for the test fixture, and re-exports MCP types instead of defining env-local Pydantic Action/Observation/State models — diverging from the canonical pattern in the reference env. This should either be in a separate PR or the re-export pattern documented as an intentional exception in the file and PATTERNS.md.
• Suggested reviewer: @Darktex

Summary

• 4 mechanical issues to fix (stdout close/read race; --reload spawn-mode sys.path bug; missing existence pre-check; misleading PR description)
• 2 alignment points for human review (host-process serving as first-class feature without RFC; echo_env models.py side-effect with non-canonical pattern)

The --reload spawn bug is the most impactful: it causes a silent ModuleNotFoundError in a realistic dev workflow (openenv serve . --reload) and is invisible to the mocked unit tests.

Automated review by Claude Code | Learn more