Conversation
huggingface#1174 added state_cls to the app factories, but nothing passes it, so the 28 envs that declare a State subclass still publish a state schema of episode_id and step_count and still strip every subclass field from the /state body. Their WebSocket state frame returns the full object, so the two transports disagree. Pass each env's State subclass at its app factory call site, and add a static regression test that walks envs/ with ast, so a new env cannot quietly skip it. Follow-up to huggingface#1174.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
repl_env and textarena_env pick between two create_app calls by probing inspect.signature(create_app). The first branch is the one that runs against a current openenv; the second exists for a release predating gradio_builder. The previous commit wired only the second, so /schema and /state still served the base State model for these two envs. Pass state_cls in the live branch, guarded by the same signature probe the file already uses for its other newer kwargs, and drop it from the legacy branch, where an openenv old enough to take that path would reject the argument. The test inspected only the last factory call in a module, which is why it did not catch this. It now checks every call, resolves values passed through a splatted kwargs dict, and exempts calls inside a signature-guarded fallback.
|
Both findings were right, thanks. Fixed in The test had the same blind spot, which is why it stayed green through the first commit. It now checks every factory call in a module rather than the last one, resolves values passed through a splatted kwargs dict, and exempts only calls inside a signature-guarded fallback. Removing the guard from |
|
cursor review |
There was a problem hiding this comment.
REQUEST_CHANGES at 3b085d82 (Ben ask: cursor review)
The 28 env app.py wirings + repl/textarena branch logic + AST test are correct — 29/29 wiring tests pass. One completeness gap remains:
Tier 1 — Harbor still unwired
src/openenv/harbor/serving.py build_app() still calls create_app(HarborEnvironment, …) without state_cls=HarborState. HarborState lives under src/openenv/harbor/models.py, so the test’s envs/-only scanner finds no State subclass and silently skips Harbor. Live Harbor /schema and /state still drop the custom fields.
This PR does not touch Harbor, but the title/claim is “each environment’s own State subclass.” Please either:
- Wire
state_cls=HarborStateinbuild_app()and extend the scanner tosrc/openenv/<name>/(or an allowlist), or - Explicitly carve Harbor out of the claim with a tracked follow-up.
Not 0.6.0 wheel cargo (envs/** + test only; Harbor fix would be package cargo if included). Branch was refreshed onto main 228bd147.
Sent by Cursor Automation: Release
|
Addressed the Harbor review in da82e56. The new test failed before the fix because all six Harbor-specific fields were missing. After the fix, the Harbor serving, environment wiring, and core state-schema suites passed: 46 tests. Touched-file usort, Ruff format/check, and Fork workflows still require approval to run; no CI approval or rerun was performed. |
|
cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit da82e56. Configure here.
There was a problem hiding this comment.
APPROVE at da82e564
Prior Harbor blocker is fixed: build_app() now passes state_cls=HarborState, and test_harbor_app_serves_its_own_state_class asserts /schema + /state retain Harbor fields over HTTP (llm_url observed).
Validation:
tests/envs/test_harbor_hosted_serving.py+test_env_state_cls_wiring.py: 38/38 pass- lint/format clean on the touched files
Because this changes src/openenv/harbor/serving.py, treat as 0.6.0 package cargo if merged (refresh draft #1211 after green exact-head CI). Still needs fork Approve-and-run / repository checks before merge.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
APPROVE at 7a967005 (merge-main only)
Rebased onto main after #1178. Feature tip unchanged from prior APPROVE at da82e564: Harbor build_app() still passes state_cls=HarborState (serving.py). 0.6.0 package cargo if merged — refresh draft #1211 after green exact-head CI.
Still needs fork Approve-and-run / repository CI.
Sent by Cursor Automation: Release
2 participants
Summary
#1174 added
state_clsto the app factories, but nothing passes it yet, so the fix is not reaching users. All 28 envs that declare aStatesubclass still publish a state schema ofepisode_idandstep_count, and still strip every subclass field from the/statebody, while their WebSocketstateframe returns the full object. This wires each env to its own class and adds a static regression test so a new env cannot forget.Follow-up to #1174, which closed #1155.
Type of Change
Alignment Checklist
.claude/docs/PRINCIPLES.mdand this PR aligns with our principles.claude/docs/INVARIANTS.mdand no invariants are violateduvis not on my shellPATH, so I ran the checks in a Python 3.12 virtualenv with the test dependencies rather than through.claude/hooks/lint.sh.RFC Status
No API changes. Every edit is an argument at an existing call site.
Test Plan
New file
tests/envs/test_env_state_cls_wiring.py: for every env that declares aStatesubclass, assert its app factory passesstate_cls, that the value is a subclass declared in that env, and that the name is imported inserver/app.py. It walks the source withastrather than importing, so playwright, carla, dm_control and the rest of the optional-dependency tail stay out of it and the check runs everywhere. A guard case fails if the discovery ever matches nothing.envs/changes and rerunning it fails 28 of the 29, one per env, which is the point of the test.tests/core: 641 passed, 2 skipped.tests/envs/test_manifest_app_targets.py: 78 passed.ruff format --checkand compiles.usortreports the same 10 pre-existingenvs/**/app.pyfiles before and after this change, so nothing here adds to that drift. I left those alone rather than reformatting files this PR only touches by one line.Notes for review
Three cases needed care rather than a blanket edit:
thinkingbox_envbuildsHTTPEnvServerdirectly and registers routes in production mode, where/stateis not registered at all. It still getsstate_clsbecause/schemais served in production and was publishing the base model.coding_tools_env,finqa_env,jupyter_env,opencode_env,pi_env,terminus_env) takeCallToolObservationfromopenenv.core, so theirStateimport comes from the env's ownmodelsmodule and follows each file's existing in-repo and standalone import pattern.wildfire_envbuilds its state withWildfireState.model_construct, andopencode_env,pi_envandjupyter_envbind the class to an instance attribute first. All four still return their declared subclass, which is what the endpoints now serve.Envs that do not declare a
Statesubclass are untouched and keep the base model.Note
Low Risk
Mechanical wiring at existing factory call sites with no API signature changes; risk is limited to incorrect
state_clsmapping, which the new AST and Harbor tests guard against.Overview
Fixes inconsistent state over HTTP vs WebSocket by passing each environment’s
*Statesubclass intocreate_app(orHTTPEnvServerfor ThinkingBox) viastate_cls, so/schemaand/stateinclude the same fields WebSocket clients already get instead of onlyepisode_idandstep_count.The change is applied across 28 env
server/app.pyfiles plus Harbor (build_app).repl_envandtextarena_envonly setstate_clswheninspect.signature(create_app)supports it, leaving legacy fallback paths unchanged.Regression coverage: new
tests/envs/test_env_state_cls_wiring.pystatically checks every env that declares aStatesubclass; Harbor gets an HTTP integration test asserting custom fields (e.g.llm_url) appear on/schemaand/state.Reviewed by Cursor Bugbot for commit e438c49. Bugbot is set up for automated code reviews on this repo. Configure here.