fix(mcp): production JSON-RPC routing with sync-safe teardown (from #1169)

[cursor]

Summary

On-repo replacement for #1169 (mugenkyou). Routes MCPToolClient production mode through HTTP /mcp JSON-RPC instead of WebSocket step(), and fixes the sync-close session leak Bugbot flagged on the fork head.

Changes

• Set use_production_mode from self._mode == "production"
• Production connect() creates a persistent HTTP MCP session (no WebSocket)
• list_tools() / call_tool() use tools/list / tools/call over JSON-RPC
• Move session/httpx cleanup to _close_async so SyncEnvClient.close, sync __exit__, and _dispatch all tear down the MCP session
• Regression: sync connect+close closes openenv/session/close

Closes #1168 (via replacement for #1169).

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation
• New environment
• Refactoring

Alignment Checklist

Before submitting, verify:

• I have read .claude/docs/PRINCIPLES.md and this PR aligns with our principles
• I have checked .claude/docs/INVARIANTS.md and no invariants are violated
• I have run focused tests on the changed suite

RFC Status

• Not required (bug fix, docs, minor refactoring)
• RFC exists: #___
• RFC needed (will create before merge)

Test Plan

• PYTHONPATH=src:envs uv run pytest tests/core/test_mode_selection.py -q — 39 passed

Claude Code Review

N/A — release automation replacement for fork #1169.

  

---

Note

Medium Risk
Changes client connection/teardown and production tool transport paths used by default MCPToolClient; incorrect behavior could leak MCP sessions or break tool calls, though scope is limited to MCP client lifecycle.

Overview
Fixes production MCP routing and session teardown for MCPToolClient by tying use_production_mode to client mode and hooking lifecycle at the async connection/close hooks the sync wrapper already uses.

Production connect() now opens the WebSocket (for reset/step/state) and creates a persistent HTTP /mcp session for list_tools / call_tool over JSON-RPC (tools/list, tools/call), with close() invoked if session setup fails. MCP session and httpx cleanup moved from overriding close() to _close_async() so sync close(), context managers, and _dispatch still call openenv/session/close.

Tests replace WebSocket step expectations with JSON-RPC mocks and add coverage for connect (WS + session), connect failure cleanup, and sync close tearing down the MCP session.

^{Reviewed by Cursor Bugbot for commit 5951e3a. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Stale Bugbot comment from a previous run.

[bot-ci-comment]

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]

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 5951e3a. Configure here.}

[cursor]

Connect opens two independent sessions

High Severity

Production connect() now opens /ws and then creates a separate HTTP MCP session. Both call _create_session(), so they consume two capacity slots and bind Gym methods and tool calls to different environment instances. With the default max_concurrent_envs=1, the second create fails and connect() cannot complete.

 

^{Reviewed by Cursor Bugbot for commit 5951e3a. Configure here.}

[cursor]

Stale comment

Alignment Review Report

Two-tier review of the production JSON-RPC routing + sync-safe teardown fix. Verified against a clean checkout (base 08af2d2f == origin/main HEAD, no drift; two-dot == three-dot).

Automated Checks

• Lint: PASS — ruff format --check, ruff check src/ tests/, and usort check are all clean on both changed files. lint.sh exits 1 only on ~57 pre-existing envs/**/README.md reformat drift (neither changed file appears; CI lint scope is src/+tests/).
• Debug code: CLEAN — check-debug.sh reports only pre-existing src/openenv/cli/** console.print/TODO hits; nothing in mcp_client.py.
• Tests: PASS — tests/core/ = 613 passed / 2 skipped. test_mode_selection.py (39) + test_production_mode_routes.py = 92 passed / 1 skipped. No regressions.

Context (verified)

• Fixes #1168 (on-repo replacement for #1169): MCPToolClient(mode="production") left use_production_mode = False (hardcoded dead code since #351), so list_tools/call_tool fell back to WS step() instead of HTTP /mcp JSON-RPC. Setting it from self._mode == "production" activates the pre-existing production path. Since _mode is normalized+validated to "production" for all MCP clients, the flag is effectively always True.
• Server side matches: POST /mcp (http_server.py:1455) → mcp_handler implements openenv/session/create|close + tools/list|call ("persistent MCP lifecycles over HTTP /mcp"), so the client's activated path resolves against real endpoints.
• Sync-safe teardown is correct: SyncEnvClient.close() / __exit__ / _dispatch all invoke _close_async directly (sync_client.py:227), never a close() override — so the previous code (overriding close()) leaked the server-side MCP session on sync teardown. Moving cleanup to _close_async + await super()._close_async() is exactly the right fix, covered by test_production_mode_sync_close_closes_mcp_session.

Open RFCs Context

• RFC 003 — MCP Support (In Review; @Darktex, @pankit-eng) governs this area. The change supports RFC 003 (routes agent tool traffic through MCP JSON-RPC). No conflict.

Tier 1: Fixes Required

None. The core change is correct and all changed lines pass lint/format/import/type checks.

Non-blocking correctness/robustness (verified by repro)

MCPToolClient.reset() / step() / state() now raise a bare AssertionError in production mode. The new _connect_async creates an HTTP MCP session but never opens a WebSocket, so _send/_state_async's assert self._ws is not None fails (reproduced — all three raise AssertionError). Two consequences:

• Against a live server, reset()/step() first create a server-side MCP session (openenv/session/create) and then assert-fail, leaking that session.
• The MCPToolClient class docstring still advertises await env.reset() and await env.step(CallToolAction(...)) (~L407-427), which now break.

Suggestion (non-blocking): either raise a clear RuntimeError ("reset/step/state unavailable in production MCP mode; use list_tools()/call_tool()") or open the WS lazily for those, and update the docstring examples. This is arguably by-design (production bypasses the Gym step-loop), so it's a robustness/docs polish, not a merge blocker.

Tier 2: Alignment Discussion

Principle Conflicts

None. The fix strengthens "minimize lifecycle deltas / production-readiness" (production mode now actually uses the production transport) and is consistent with the Dual-API-boundary invariant (Gym-style reset/step/state are infra controls and are now unavailable to the production MCP client). mcp_client.py imports only framework/shared modules — client-server separation intact.

RFC / Invariant Awareness (non-blocking)

ALIGNMENT FLAG: Production MCP tool traffic routed over HTTP /mcp while the invariants steer toward WebSocket-only

• Invariant/RFC at stake: INVARIANTS.md §4 "Communication patterns" ("WebSocket for all environment communication"; "deprecating HTTP … in favor of WebSocket-only, still transitioning") + RFC 003 (In Review).
• The concern: This activates the HTTP /mcp transport for production list_tools/call_tool. The server also serves MCP JSON-RPC over WebSocket (@app.websocket("/mcp") and type:"mcp" frames on /ws), so a path aligned with the WebSocket-only direction already exists. The change matches the issue's stated intent (HTTP JSON-RPC) and pre-existing design, so it's likely the right near-term fix — but it deepens reliance on HTTP in an area the invariants flag as in-transition. Worth a team decision on whether production MCP should ultimately be MCP-over-WebSocket.
• Suggested reviewer: @Darktex (authored the Communication-patterns invariant b1a92e1b1 and the production MCP code in #351; RFC 003 co-author) + @pankit-eng (RFC 003); loop in swappy for the persistent MCP session behavior.

Summary

• 0 mechanical (Tier 1) issues to fix
• 1 verified non-blocking robustness/docs issue (production reset/step/state → AssertionError + stale docstring)
• 1 Tier-2 alignment awareness (HTTP vs WebSocket-only transport direction); 0 RFC conflicts (RFC 003 supported)

  

_{Sent by Cursor Automation: Pre-review}

[cursor]

Holding merge despite exact-head CI green on 5951e3a3 (cannot REQUEST_CHANGES on our own PR).

Bugbot’s dual-session finding is correct and blocking for default servers (max_concurrent_envs=1):

• /ws connect already calls _create_session()
• production _ensure_production_session() then calls HTTP openenv/session/create → a second _create_session()
• that second create hits capacity and connect() cannot complete; even when capacity allows, Gym methods and tool calls bind to different env instances

The client never learns the WebSocket session id, so HTTP tools cannot reuse the WS session today. HTTP-only connect (prior head) avoids the capacity clash but breaks reset/step/state.

This is the same architectural hole #1158 tried to close. Do not land the one-line use_production_mode flip (or this dual-connect approach) until production mode has a single shared session for Gym + tools. Prefer closing #1169/#1175 as incomplete unless we explicitly choose an HTTP-only tools-only mode with clear errors for Gym APIs and updated docs.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Alignment Review Report

Scope: 2 commits since main (e045fc6d, 5951e3a3); 2 files — src/openenv/core/mcp_client.py (+28 net) and tests/core/test_mode_selection.py (mostly new tests). This is a genuine core-client bug fix for #1168 (the on-repo replacement for #1169).

Update since the first commit: the HTTP-only-connect regression flagged earlier (production reset/step/state raising a bare AssertionError) is resolved by 5951e3a3, which reopens the WebSocket for the Gym lifecycle. Verified against the current head and covered by test_production_mode_connect_opens_websocket_and_http_session.

Automated Checks

• Lint: PASS — usort check, ruff format --check, and ruff check all pass on both changed files (ran ruff 0.16.7 / usort locally; .claude/hooks/lint.sh can't run on this review VM because uv isn't installed). No repo-wide lint regression attributable to this PR.
• Debug code: CLEAN — check-debug.sh reports only pre-existing print(...) examples inside docstrings; this PR adds no new prints, breakpoints, or TODO/FIXME.

Open RFCs Context

• RFC 003 — MCP Support (In Review) — directly relevant and supported. Its Transport section (amended Nov 15, 2025) endorses POST /mcp JSON-RPC for production/inference (Scenario 3), which is exactly what this PR activates. RFC 003 lists "No session management" as an early limitation, but the server already ships openenv/session/create|close as an intentional OpenEnv extension ("persistent MCP lifecycles over HTTP /mcp"), so that note reads as stale rather than a live conflict.
• RFC 000/001/002 (In Review) — Dual-API boundary + "WebSocket for step loop." The second commit keeps reset/step/state on /ws, so the Gym control plane stays on WebSocket.
• RFC 010 / 011 (Draft) — unrelated (ECHO world-model; ARD catalog discovery).

Tier 1: Fixes Required

• src/openenv/core/mcp_client.py:14-30 — (low severity, non-blocking) module docstring is now slightly inaccurate. With use_production_mode defaulting to True and the WebSocket reopened, production MCPToolClient uses both transports: /ws for reset/step/state and /mcp for list_tools/call_tool. The architecture diagram ("Production Mode … /mcp … Bypasses step()") and the "Client Usage" lines (MCPToolClient (default) → /ws, (production) → /mcp) don't reflect this dual-transport behavior, and imply a non-production MCPToolClient the constructor disallows. Suggest a quick update.

No lint, syntax, import, type, or security issues found.

Tier 2: Alignment Discussion

Principle Conflicts

None blocking. The change respects the dual-API boundary and "agents cannot reset": reset/step/state stay on /ws; only agent tool traffic uses /mcp. Client-server separation is intact (no server/ imports). The close → _close_async rename is the correct fix for sync-safe teardown — sync paths (SyncEnvClient.close, sync __exit__, _dispatch) invoke _close_async directly, so the old public-close override was skipped on sync teardown and leaked the server-side MCP session; now covered by test_production_mode_sync_close_closes_mcp_session.

ALIGNMENT FLAG (non-blocking; confirm direction): production tool traffic defaults to HTTP /mcp while the repo is transitioning off HTTP

• Principle/Invariant at stake: INVARIANTS.md §4 "Communication patterns" — "WebSocket for all environment communication … deprecating HTTP … in favor of WebSocket-only (transitioning, both available)."
• The concern: this PR makes HTTP /mcp the live default for production list_tools/call_tool. RFC 003 explicitly sanctions POST /mcp, so it's aligned near-term — but a server-side MCP-over-WebSocket path already exists (@app.websocket("/mcp") and type:"mcp" JSON-RPC frames on /ws). Worth confirming whether production MCP should ultimately converge on MCP-over-WebSocket to match the "WebSocket-only" direction, and reconciling the wording between INVARIANTS §4 and RFC 003.
• Suggested reviewer: @Darktex (authored the production MCP code in #351, INVARIANTS §4, and co-authors RFC 003)

RFC Conflicts

None. RFC 003 is supported, not conflicted. Optionally @pankit-eng (RFC 003 co-author) can confirm the HTTP MCP session lifecycle semantics.

Summary

• 1 mechanical issue to fix (module docstring accuracy; low severity, non-blocking)
• 1 alignment point for human review (HTTP-vs-WebSocket transport direction)
• 0 RFC conflicts to discuss (RFC 003 supported)

Overall: mechanically sound, well-tested, and the earlier reset/step/state regression is resolved. No blocking issues.

  

_{Sent by Cursor Automation: Pre-review}

[cursor]

Core fix (Tier 1). This is the crux of #1168: use_production_mode was hardcoded to False, so the entire HTTP /mcp production path (_production_mcp_request, _ensure_production_session, the production branches in list_tools/call_tool) was dead code. Since MCPClientBase only permits mode='production', this flag is now effectively always True for MCPToolClient — correct.

Minor doc nit: this makes the module docstring (lines 14–30) stale — production now uses both /ws (reset/step/state) and /mcp (list_tools/call_tool), but the architecture diagram / "Client Usage" section implies /mcp-only and a non-production MCPToolClient that can't be constructed. Worth a quick update.

[cursor]

Regression fix confirmed. Reopening the WebSocket (super()._connect_async()) before creating the HTTP MCP session correctly resolves the earlier HTTP-only-connect issue where reset/step/state raised a bare AssertionError (assert self._ws is not None) in production. The failure path (await self.close() → _close_async) is safe in both async and sync-loop contexts (close() dispatches to an awaitable _close_async). Well covered by test_production_mode_connect_opens_websocket_and_http_session and test_production_mode_connect_failure_cleans_up_resources.

Tier 2 (non-blocking): activating HTTP /mcp for production tool calls sits in mild tension with INVARIANTS.md §4 ("WebSocket for all environment communication … deprecating HTTP in favor of WebSocket-only"). It's sanctioned by RFC 003 and a server-side MCP-over-WS path already exists, so this is a direction to confirm with @Darktex / @pankit-eng — not a blocker.

[cursor]

Sync-safe teardown crux. Overriding _close_async rather than the public close is the right call: SyncEnvClient.close, sync __exit__, and _dispatch all invoke _close_async directly, so the previous close() override was skipped on sync teardown and leaked the server-side MCP session (via openenv/session/close + httpx cleanup). Async paths still reach this through close() → _dispatch(_close_async). Good regression coverage in test_production_mode_sync_close_closes_mcp_session.