fix(envs): declare importable manifest app paths (from #1068) - #1164
Conversation
The manifest declared `app: envs.agent_world_model_env.server.app:app`, the only one of 35 envs not using `server.app:app`. The cloud providers read this field verbatim and run `cd /app/env && python -m uvicorn <app>`, so this env fails to start on Modal and Daytona with ModuleNotFoundError: images are built with the env directory as context, and the installed dist maps `agent_world_model_env` to `.`, so no `envs` package exists in either layout. Add a contract test resolving every manifest's app target against the env directory on disk. It does not import the modules: that would pull in playwright, carla and the rest of the optional-dependency tail and skip on exactly the CI machines that should be guarding this.
Seven manifests declared `app: server.app:app` while their server package imports `from ..models`. Under that name `server` is top-level, so the relative import climbs past it and the server dies with `ImportError: attempted relative import beyond top-level package`. Only the cloud providers read this field -- `ModalProvider._discover_server_cmd` and its Daytona twin run `cd <env root> && python -m uvicorn <app>` -- so the breakage is invisible locally, where `docker run` uses the Dockerfile CMD instead. Each manifest now names the same target its own Dockerfile launches: the env's distribution package for six of them, and `envs.grid_world_env...` for grid_world_env, which builds from the repo root. The contract test now checks importability rather than mere file existence, walking the reachable import graph with `ast`. It does not import the modules: that would pull in playwright, carla and the rest of the optional-dependency tail and skip on exactly the CI machines that should be guarding this. Imports wrapped in a try/except ImportError are not counted, since that fallback is how most envs already support both layouts. Validated against real interpreter behaviour across all 35 manifests: no false positives and no false negatives on every env whose dependencies are installed.
`_resolve` fell back to the repo root unconditionally, and the source tree always contains `envs/<env>/server/app.py`. Every env therefore accepted an `envs.` prefix -- including the original `envs.agent_world_model_env.server.app:app`, the exact value this suite was added to reject. The repo's layout is not evidence about the container's. A repo-root build context is not the right signal either: coding_env builds from the repo root but `pip install ./envs/coding_env/` makes it a real distribution, so its CMD launches `coding_env.server.app:app` with no prefix. The prefix is only importable when the image launches it that way, as grid_world_env does with /app on PYTHONPATH. Derive it from that. Add the negative tests whose absence let this through: the checks only proved the values in the tree pass, never that a wrong one fails. One asserts AWM's old target does not resolve, one that grid_world's legitimate target still does, and one pins repo-root-context and envs-prefix as different predicates.
burtenshaw
added
bug
Cursor
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
|
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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Alignment Review Report
Reviewed at PR head 8791ef7c. Checks were run against the actual PR content.
Automated Checks
- Lint: PASS (for this PR).
.claude/hooks/lint.shexits non-zero, but every file it flags is a pre-existingenvs/*file that the hook reformats repo-wide (e.g.opencode_env,pi_env,chat_env, plus Python code-fences in manyREADME.mds). The PR's only Python file —tests/envs/test_manifest_app_targets.py— passesruff format --check,ruff check, andusort checkin isolation. The 7 changedopenenv.yamlfiles aren't linted as Python. - Debug code: CLEAN (for this PR).
check-debug.shonly scanssrc/, which this PR doesn't touch; itsprint/TODOhits are all pre-existing insrc/. The new test file has noprint/breakpoint/TODO. - Focused test:
PYTHONPATH=src:envs pytest tests/envs/test_manifest_app_targets.py→ 76 passed, including the negative regression tests.
I verified the premise against the code: ModalProvider._discover_server_cmd and DaytonaProvider._discover_server_cmd read the manifest app field verbatim and run cd <dir-of-openenv.yaml> && python -m uvicorn <app>, using the Dockerfile CMD only as a fallback. Each changed manifest now matches the module its own Dockerfile launches, and the unguarded from ..models servers (awm, carla, chess, pelican, wildfire, grid_world) genuinely require the package-qualified path. The fix is correct.
Open RFCs Context
- RFC 002 – Env Spec (In Review; @Darktex, @pankit-eng, @jspisak, @zkwentz): owns the environment spec +
ContainerProviderpackaging decision — the surface this PR touches. - RFC 008 – Environment Auto-Validation (In Review; @zkwentz): designing
openenv validate+ grader registry / normalized-manifest schema; explicitly notes today "no manifest schema exists" and thatvalidate/pushrun divergent checks. - RFC 011 – Catalog Discovery (Draft): reads
openenv.yamlfor name/marker/framework/description, not theappfield. No conflict.
Tier 1: Fixes Required
None. No lint / type / import / security issues in the PR's own changes.
Tier 2: Alignment Discussion
Principle Conflicts
None identified. This is a deploy-config correctness fix + regression test; it does not touch the Gym API, MCP boundary, reset/agent isolation, reward placement, or client↔server separation — all INVARIANTS intact. It actively supports "minimize lifecycle deltas" by making cloud startup match local.
RFC Conflicts
ALIGNMENT FLAG: New manifest-correctness check lands as a standalone pytest rather than an RFC 008 grader
- RFC at stake: RFC 008 (Environment Auto-Validation, In Review)
- The concern: RFC 008 is designing the canonical mechanism for mechanically catching env defects (
openenv validate, grader registry, normalized-manifest schema) so operators/hubs inherit the same checks. This PR adds a genuinely valuable "theapptarget must import under its declared name" check, but as a repo-localtests/envs/test, so it won't be surfaced byopenenv validateor a hub intake. Worth deciding whether it should also become an RFC 008 grader. Non-blocking — the test is correct and useful as-is. - Suggested reviewer: @zkwentz
ALIGNMENT FLAG: The deploy-manifest app entrypoint contract is enforced but unspecified in an RFC
- RFC at stake: RFC 002 (Env Spec, In Review)
- The concern: The rule this PR encodes —
appmust be the distribution-qualified module the image actually launches, and providers run it verbatim over the DockerfileCMD— is real behavior inmodal_provider/daytona_providerbut isn't written down in the env-spec RFC. Documenting theapp/runtime/portcontract in RFC 002 would keep the ~35 manifests from drifting again. Non-blocking. - Suggested reviewer: @Darktex
Summary
- 0 mechanical issues to fix (Tier 1)
- 1 minor non-blocking nit (inline:
cd /app/envwording) - 2 RFC discussion points (RFC 008 grader placement; RFC 002 documenting the
appcontract) - 0 principle/invariant violations
Overall: a correct, well-tested bug fix — effectively ship-ready. The flags are for design discussion, not blockers.
Sent by Cursor Automation: Pre-review
| The cloud providers do not read the Dockerfile `CMD` when a manifest is | ||
| present: `ModalProvider._discover_server_cmd` (and its Daytona twin) locate | ||
| `openenv.yaml` inside the sandbox, take the `app` field verbatim, and run | ||
| `cd /app/env && python -m uvicorn <app>`. So the `app` field alone decides |
There was a problem hiding this comment.
Minor / non-blocking (accuracy of guidance): cd /app/env is the modern-layout path, but not what every env actually uses. Both providers compute env_root = <directory containing openenv.yaml> (fast-path /app/env, else find /app -maxdepth 4 -name openenv.yaml), so the real command is cd /app/carla_env for carla_env (COPY . /app/carla_env/) and cd /app/envs/grid_world_env for grid_world_env. Consider phrasing this as cd <dir containing openenv.yaml> here and in the two failure messages (lines 244 and 274) so the guidance stays accurate for non-/app/env layouts. The check logic itself is correct — it resolves against the source env dir, not a hardcoded container path.
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
3 participants
Summary
Replacement for #1068 (fork workflows stuck at
action_required). Same change: declare manifestapppaths the server can actually import under, with hermetic regressions.Please close #1068 once this lands.
Type of Change
Alignment Checklist
mainso required CI can run on-repoRFC Status
Test Plan
tests/envs/test_manifest_app_targets.pyClaude Code Review
N/A — release automation replacement for Ben's watch/merge ask on #1068.
Note
Low Risk
Manifest and CI-only validation changes; no runtime library logic, with low blast radius beyond cloud env boot paths.
Overview
Fixes Modal/Daytona startup by correcting
openenv.yamlappentries so uvicorn loads the FastAPI app under a module path that matches each image’s layout (not the DockerfileCMD, which local Docker still uses).Several envs move from bare
server.app:appto<env>.server.app:app;agent_world_model_envdrops the invalidenvs.prefix;grid_world_envkeepsenvs.grid_world_env.server.app:appwhere the runtime actually uses that prefix.Adds
tests/envs/test_manifest_app_targets.py, which parametrizes all manifests with anappfield: it checks the module exists and uses AST (no heavy imports) to catch relative imports that would break when the declared package root is wrong, with layout-specific guards for env-root vs repo-root images.Reviewed by Cursor Bugbot for commit b3005a7. Bugbot is set up for automated code reviews on this repo. Configure here.