fix(envs): declare app paths the server can actually be imported under - #1068
fix(envs): declare app paths the server can actually be imported under#1068k21993 wants to merge 3 commits into
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
k21993
changed the title
|
@cursor Good catch — you were right, and the fix has been rewritten. Confirmed your diagnosis by reproduction:
Two follow-ons from your review: The bug is not one env. Auditing all 35 manifests turned up seven with the same defect ( The original test could not have caught this. It only checked that the module file existed on disk, which The static check was cross-validated against real |
|
Unable to authenticate your request. Please make sure to connect your GitHub account to Cursor. Go to Cursor |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4cee358. Configure here.
|
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. |
`_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.
|
@cursor Right again, and this one was worse than its Medium rating and the suite was passing the exact bug the PR exists to fix. Verified before fixing by putting the original value back: Fixed in 0b3e94c. The repo-root fallback is now conditional on evidence that the image actually imports under that prefix, rather than on the path existing in the source tree, which it always does. One refinement worth noting: "built from a repo-root context" is not the right predicate. Same value with the bad manifest restored, after the fix: Also added the negative tests whose absence let this through. Every check here only proved that the values in the tree pass — nothing asserted that a wrong value fails, which is why the regression was invisible to it:
Suite: 1604 passed, 133 skipped. |
|
Unable to authenticate your request. Please make sure to connect your GitHub account to Cursor. Go to Cursor |
1 participant
Summary
Seven manifests declare
app: server.app:appwhile their server package importsfrom ..models. Under that nameserveris top-level, so the relative import climbs past it and the server dies withImportError: attempted relative import beyond top-level package.Only the cloud providers read this field :
ModalProvider._discover_server_cmdand its Daytona twin runcd <env root> && python -m uvicorn <app>: so the breakage is invisible locally, wheredocker runuses the DockerfileCMDinstead. Each manifest now names the same target its own Dockerfile already launches.agent_world_model_envenvs.agent_world_model_env.server.app:appagent_world_model_env.server.app:appcarla_envserver.app:appcarla_env.server.app:appchess_envserver.app:appchess_env.server.app:appfinqa_envserver.app:appfinqa_env.server.app:appgrid_world_envserver.app:appenvs.grid_world_env.server.app:apppelican_svg_envserver.app:apppelican_svg_env.server.app:appwildfire_envserver.app:appwildfire_env.server.app:appgrid_world_envdiffers because it builds from the repo root (COPY envs/grid_world_env/ /app/envs/grid_world_env/,ENV PYTHONPATH=/app), soenvs.is the correct prefix there — its Dockerfile already says so.Envs that declare
server.app:appand import cleanly under it (echo_env,chat_env,repl_env, …) are untouched.Type of Change
Alignment Checklist
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violatedbash .claude/hooks/lint.shand tests and addressed all issuesTo be precise on the third box:
lint.shfails on this branch, but it fails identically on an untouchedmain—ruff format envs/ --checkreports 55 unformatted.pyfiles there, none touched here. The files in this diff passruff format --check,ruff check, andusort check. Unrelated reformatting was deliberately kept out.Separately,
lint.shleavesenvs/**/README.mddirty on every run: line 24 runsruff formatin place (this ruff version formats Python blocks inside Markdown), but the cleanup at line 27 filters to*.py, so the Markdown is never restored. Happy to send that one-line fix separately.RFC Status
RFC 002's cloud sandbox provider section maps "run a process inside the sandbox" to a provider-internal start command; this repairs that path rather than changing it.
Test Plan
tests/envs/test_manifest_app_targets.pychecks two things per manifest: the declared module exists, and it is importable under the name declared. The second is what the first version of this PR missed —server/app.pyexists whether or notserver.appcan be imported.It does not import the modules. That would pull in playwright, carla, dm_control and the rest of the optional-dependency tail, so the module would skip on exactly the CI machines that should be guarding this — which is how this drift went unnoticed. Instead it walks the reachable import graph with
ast, tracking each module's depth in the declared namespace and flagging any relative import whose level exceeds it. Imports wrapped intry/except ImportErrorare not counted, since that fallback is how most envs already support both layouts.Validation against real interpreter behaviour. The static check was cross-checked against actually running
import <declared>from each env directory, for all 35 manifests: no false positives and no false negatives on every env whose dependencies are installed locally. Two envs (chess_env,pelican_svg_env) could only be confirmed statically becausechess/resvg_pyare not installed; both already use the packaged form in their Dockerfile, which corroborates.Reproduce the failure on
main:Suite:
1601 passed, 133 skipped(baseline onmainis1530 passed, 133 skipped).python scripts/sync_env_docs.py --checkpasses.The three legacy manifests with no
appfield (browsergym_env,coding_env,julia_env) are skipped rather than failed. Migrating those to the current schema touches more envs, raises back-compat questions, andcoding_envhas its own build layout — separate PR.Claude Code Review
/alignment-review: 0 Tier 1 issues, 0 principle conflicts, 0 RFC conflicts. Automated checks report only pre-existing findings onmain(unformattedenvs/*.py;console.printinsrc/openenv/cli/), none in this diff.Note
Medium Risk
Changes only manifest strings and tests, but wrong
appvalues previously broke cloud sandboxes for affected envs; fixes are low-risk and aligned with existing Dockerfile CMDs.Overview
Fixes Modal/Daytona startup for seven envs by aligning each
openenv.yamlappvalue with the module path uvicorn can actually import (cloud runspython -m uvicorn <app>from the manifest, not the DockerfileCMD).Most envs move from
server.app:appto<env>.server.app:appso parent-relative imports likefrom ..modelsresolve.agent_world_model_envdrops the invalidenvs.prefix;grid_world_envkeepsenvs.grid_world_env.server.app:appto match its repo-root image layout.Adds
tests/envs/test_manifest_app_targets.py, which parametrizes all manifests with anappfield: checks the module exists, and uses AST (not runtime imports) to flag relative imports that escape the declared package root, with layout-specific rules for theenvs.prefix.Reviewed by Cursor Bugbot for commit 0b3e94c. Bugbot is set up for automated code reviews on this repo. Configure here.