Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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. |
|
@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 |
There was a problem hiding this comment.
Reviewed at 0b3e94ce9bab97b7ac27b350a6a73378ab06f50f: the seven manifest app targets match their actual container import roots, and the AST/Dockerfile-based regressions avoid optional-dependency skips; 76 focused tests pass. No code or alignment defect found. Before merge, refresh onto current main, approve the first-contributor workflow run, and require green exact-head CI; the required GitHub Actions jobs have not run on this branch.
Sent by Cursor Automation: Release
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.
0b3e94c to
add153a
Compare
|
@burtenshaw I've rebased #1068 and #1070 onto current main and updated both descriptions with fresh validation results. The 76 manifest tests and four lint-hook tests pass, and the broader local checks are documented in each PR. Could you approve the GitHub Actions runs for both PRs so CI can validate the new commits? |
There was a problem hiding this comment.
Re-reviewed exact head add153ada3e13720d1962dc8b274476174fa991f: no code or alignment defects found. The seven manifest app: targets match their container import roots, and the dependency-free AST/import-graph guard now rejects the earlier false-positive envs. fallback. All 76 focused tests pass on this head; the same tests fail exactly seven cases on current main, confirming the regressions are meaningful. The merge-tree with current main is clean, including #1138's discovery additions.
This is a narrow manifest correctness fix, but it is not merge-ready yet: the branch is two commits behind current main, no required GitHub Actions workflow has run on this exact head, and no human approval is recorded. Refresh, approve/run Tests/Package/docs/discovery CI, and obtain the already-requested burtenshaw review before merge.
Sent by Cursor Automation: Release
* fix(agent_world_model_env): point manifest app at the real server module 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. * fix(envs): declare app paths the server can actually be imported under 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. * test(envs): only accept the envs. prefix where the image really uses it `_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. --------- Co-authored-by: Karthik Suresh <7954591+k21993@users.noreply.github.com> Co-authored-by: burtenshaw <ben.burtenshaw@gmail.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: benjamin.burtenshaw <benjamin.burtenshaw@huggingface.co>
2 participants
Summary
Fix seven environment manifests whose
appvalues do not match their container import layout. Cloud providers use this field to launch uvicorn, so an invalid module name can prevent startup even when the Dockerfile's own command works.Six environments use their installed package name.
grid_world_envusesenvs.grid_world_env.server.app:app, matching its Dockerfile andPYTHONPATHlayout.Add static regression checks for module resolution and relative imports that escape the declared package. The checks run without optional environment dependencies and include negative cases for the incorrect
envs.prefix. They do not replace runtime import or deployment tests.Type of Change
Alignment Checklist
RFC Status
Test Plan
tests/envs/test_manifest_app_targets.py: 76 passed.main, these regression tests fail for all seven corrected manifests.main: 57 files need Ruff formatting acrosssrc/,tests/, andenvs/, and usort flagstest_grid_world.pyandtest_julia_env.py. The changed test file is clean.Note
Low Risk
Manifest-only startup path fixes plus CI guards; no runtime API or application logic changes beyond cloud env bootstrapping.
Overview
Corrects
apptargets in sevenopenenv.yamlmanifests so Modal/Daytona can start uvicorn from the manifest (not the DockerfileCMD). Six envs now use<env>.server.app:appinstead ofserver.app:app;agent_world_model_envdrops the invalidenvs.prefix, andgrid_world_envkeepsenvs.grid_world_env.server.app:appfor its repo-rootPYTHONPATHlayout.Adds
tests/envs/test_manifest_app_targets.py, which statically checks every manifest with anappfield: the module exists on disk, and AST analysis flags relative imports that would break under the declared package name—without importing heavy optional deps. Negative tests pin whenenvs.prefixes must be rejected vs allowed.Reviewed by Cursor Bugbot for commit 8791ef7. Bugbot is set up for automated code reviews on this repo. Configure here.