Repository navigation
fix(envs): serve each environment's own State subclass on /schema and /state - #1198
Conversation
openenv#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 openenv#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.
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
There was a problem hiding this comment.
Merge-main re-APPROVE at 4ace122a
Ben merged main into this fork tip. Feature diff unchanged: env state_cls wiring + Harbor build_app(... state_cls=HarborState) with regressions.
Fork PR: repository CI still needs Approve-and-run. OpenEnv wheel cargo if merged before Thursday — would require refreshing draft #1211 + TestPyPI.
Sent by Cursor Automation: Release
There was a problem hiding this comment.
Re-APPROVE at 29b780c1 (merge-main only)
Ben merged main (26c9465e / #1179) into this fork PR. Stable feature patch-id is unchanged vs prior approved tip 4ace122a (9fc93a50…); Harbor/state_cls wiring + tests identical.
Prior technical approval stands. Still needs maintainer Approve and run. Package cargo if merged before Thursday — prefer hold through 0.6.0 to avoid another candidate/TestPyPI churn.
Sent by Cursor Automation: Release
|
@burtenshaw exact head The five repository workflows are still at |
|
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 55d7947. Configure here.
Signed-off-by: Karthik Suresh <7954591+k21993@users.noreply.github.com>
|
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. |
burtenshaw
left a comment
burtenshaw
left a comment
There was a problem hiding this comment.
Reviewed state-class wiring and schema behavior; 129 focused environment, Harbor, and core schema tests pass.
Summary
#1174 added
state_clsto the app factories, but callers were not passing it. This wires all 28 environments with custom State subclasses and the package-backed Harbor app to their own classes, so/schemaand/statepreserve the same fields as the WebSocket state frame.Refreshed against main at
5639e52a. The merge preserves upstream concurrency settings and removal ofopencode_envandpi_env, and wires the newly addedtau2_env. Existing signature guards inrepl_envandtextarena_envpreserve compatibility with older OpenEnv installations.Follow-up to #1174, which closed #1155.
Type of Change
Alignment Checklist
RFC Status
Test Plan
Notes for review
The environment regression scans app factory calls with
ast, so optional simulator dependencies are not needed. It checks every factory call and resolves signature-guarded kwargs. Harbor has a separate TestClient regression for both/schemaand/statebecause its app lives undersrc/openenv/harbor/.thinkingbox_envpasses its class directly toHTTPEnvServer;/schemaremains relevant in production mode. Environments without custom State subclasses keep the base class.Validation notes: the Harbor port-conflict failure reproduces on clean main (
5639e52a) with the same error: expectedalready in use, received a startup timeout. No new failures were found in the broad suite.Note
Low Risk
Call-site wiring only with no API signature changes; behavior change is richer HTTP state/schema payloads aligned with WebSocket, with regression tests guarding future omissions.
Overview
Wires every environment (and Harbor) that defines a custom
Statesubclass to passstate_clsintocreate_app/HTTPEnvServer, so HTTP/schemaand/stateexpose the same fields as WebSocket state instead of falling back to the baseState(episode_id,step_countonly).Coverage spans all listed
envs/*/server/app.pyservers plusopenenv.harbor.serving.build_appandthinkingbox_env’s directHTTPEnvServerconstruction.repl_envandtextarena_envonly setstate_clswheninspect.signature(create_app)includes the parameter, keeping older OpenEnv installs bootable.Adds an
ast-based regression test that scans envserver/app.pyfor factory calls missing or mis-wiringstate_cls, and a Harbor TestClient check that/schemaand/stateincludeHarborStatefields (e.g.llm_url).Reviewed by Cursor Bugbot for commit 993aa2d. Bugbot is set up for automated code reviews on this repo. Configure here.