fix: clear 0.4.3 blockers with cancel-safe teardown

[cursor]

Summary

• Supersedes fix: clear 0.4.3 release blockers (discovery path/XDG + WebSocket close) #1170 with its discovery path, XDG cache, and WebSocket lifecycle fixes.
• Addresses Bugbot's exact-head finding by running graceful current-socket teardown in a tracked, shielded task.
• Prevents reconnect from opening a replacement session until a cancelled close handshake finishes.

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 the equivalent lint and focused tests and addressed all issues

RFC Status

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

Test Plan

• PYTHONPATH=src:envs .venv/bin/python -m pytest tests/test_core/test_generic_client.py tests/discovery/test_catalog_contract.py tests/envs/test_discovery.py -q — 258 passed, 5 skipped
• Ruff format/check and usort check on all changed Python files — clean
• Before the fix, a focused reproducer showed the cancelled socket remained OPEN, had zero tracked close tasks, and allowed a replacement connection. The new regression cancels during the current socket's suspended close handshake and verifies reconnect waits for completion.

Claude Code Review

The external finding was reproduced and confirmed. The implementation centralizes socket-close task tracking, shields normal disconnect teardown from caller cancellation, preserves prompt receive-timeout behavior, and adds exact scheduling coverage.

After this lands, #1170 can be closed as superseded.

  

---

Note

Medium Risk
Changes session teardown, reconnect ordering, and discovery cache/path trust boundaries; regressions could leak server sessions or break catalog validation for edge-case paths.

Overview
Hardens discovery and client teardown for the 0.4.3 release: catalog relative paths now reject control characters and Unicode line/paragraph separators (models, packaged JSON schemas, docs, contract tests), while still allowing spaces and non-ASCII path segments.

Auto-discovery cache ignores relative XDG_CACHE_HOME and fails closed when HOME is not absolute, so an untrusted checkout cannot redirect cache loading.

EnvClient WebSocket lifecycle centralizes socket closes in tracked tasks, sends a graceful close message on disconnect, drains pending handshakes before reconnect (avoiding single-session capacity races), and makes close() cancel-safe—shielded drains, child-client shutdown with deferred cancellation, and provider teardown even when the caller is cancelled.

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

[cursor]

Stale comment

Blocking discovery finding at exact head 366941de505991507c603f27f48acfcbbecd9865:

U+2028 LINE SEPARATOR and U+2029 PARAGRAPH SEPARATOR remain valid because they are outside C0/DEL/C1. That is unsafe for this contract: Python str.splitlines() treats both as line boundaries, openenv catalog prints untrusted issue/source paths directly, and catalog serialization uses literal Unicode (ensure_ascii=False). A crafted path can therefore inject a second logical CLI/log line even after the current C0/C1 fix.

Before merge:

• reject U+2028/U+2029 in relative_path, _CONTROL_CHARACTER_PATTERN, and every _PATH_COMPONENT_PATTERN exclusion;
• regenerate all packaged discovery schemas;
• add model + JSON Schema regressions for both characters while retaining printable Unicode/spaces;
• update the guide wording to include Unicode line/paragraph separators.

Bidi/format controls such as U+202E are a separate human policy question; they do not split lines and need not block this fix. U+2028/U+2029 do split lines in Python consumers and must block the 0.4.3 release until rejected.

  

_{Sent by Cursor Automation: Release}

[cursor]

Stale comment

Current head 3b06765e3295051cc3f19bbcb023ef7e460b63c3 fixes the Unicode line-separator finding, but one teardown blocker remains.

_close_async() still closes children sequentially under suppress(Exception). asyncio.CancelledError is a BaseException, so cancellation while one await child.close() is suspended exits the loop immediately. The new finally: self._child_clients.clear() then drops every remaining child reference; later children never receive close() and can retain live WebSocket sessions/resources. The current regression has only one child, so it proves parent cleanup but cannot expose this multi-child leak.

Please guarantee that every captured child close is initiated/completed despite cancellation while preserving cancellation propagation afterward (for example, explicitly defer and re-raise CancelledError after processing all children, or use tracked/shielded child-close tasks), and add at least two children with cancellation during the first close, asserting the later child is closed before parent socket/provider teardown completes. Keep this draft until that regression, refreshed exact-head CI, and the requested security/transport approvals are complete.

  

_{Sent by Cursor Automation: Release}

[cursor]

Stale comment

Re-reviewed exact head f1069e8f2d080e3aeeb6f7926c75af171bf13c3c. The previously blocking multi-child cancellation path is fixed: all captured child closes start concurrently, their gather is shielded while caller cancellation is deferred, every child completes, parent socket/provider teardown runs, and the original CancelledError is re-raised afterward. Adversarial probes with one slow plus five fast children, a pending dropped socket, and repeated cancellation preserved every cleanup invariant.

The other release-blocker fixes remain sound at this head: current socket disconnect is tracked/shielded before its first await; reconnect drains it before opening another session; relative XDG and relative fallback HOME fail closed; and C0/C1 plus U+2028/U+2029 are rejected consistently by the model and generated schemas.

Verification: generic-client 109 passed / 5 expected skips; broader suite 1646 passed / 134 skipped with one unrelated pre-existing OpenCode/Jinja failure; changed-file Ruff/format clean. Exact-head GitHub Tests 3.11/3.12, ThinkingBox, Package CI, catalog, CodeQL, lint, locks, and docs are all green.

One low-severity edge remains: a child that independently raises CancelledError during its own close is surfaced as if the parent caller were cancelled. Real EnvClient child close paths do not self-cancel, so this is non-blocking.

Technical verdict: ready for human review. Keep the PR draft and do not merge until the requested exact-head security/schema (Darktex, thegovind) and transport (sergiopaniego / burtenshaw) approvals are recorded.

  

_{Sent by Cursor Automation: Release}

[cursor]

Alignment Review Report

Three independent 0.4.3 blockers bundled together: (1) cancel-safe WebSocket teardown in env_client.py, (2) XDG cache hardening in auto/_discovery.py, (3) control-character rejection for repository-relative paths in discovery/models.py + the four 0.1-draft schemas. All three are correctness/security hardening, well-scoped and thoroughly tested.

Automated Checks

• Lint: PASS (for changed files). The three CI lint checks — ruff format --check src/ tests/, ruff check src/ tests/, usort check — all pass on the six changed src/+tests/ files. bash .claude/hooks/lint.sh exits 1, but only on the ~pre-existing envs/**/README.md reformat drift (57 files, none in this diff); the hook reformats python fences inside env READMEs, unrelated to this PR.
• Debug code: CLEAN. check-debug.sh hits are all pre-existing or docstring examples (env_client.py:20,29,1134); none land on changed lines. No breakpoint()/pdb.
• Tests: PASS. The 3 touched suites (test_catalog_contract.py, test_discovery.py, test_generic_client.py) → 265 passed / 5 skipped. Broader core client suites (tests/test_core/ + tests/core/) → 901 passed / 13 skipped, so the teardown refactor doesn't regress the MCP/production-mode paths. The 4 new teardown tests and 2 new XDG tests were confirmed executing (not skipped).
• Freshness: diff base 08af2d2f == current origin/main HEAD (no drift); git merge-tree is conflict-free.

Open RFCs Context

• RFC 011 – ARD-backed catalog discovery (Draft, @thegovind) — the discovery/catalog profile that Parts 2 & 3 touch. Both changes align with its §8 security intent ("credentials and caches are scoped to the caller and configured origin"). See the one RFC note below.
• RFC 002 – env-spec (In Review) / RFC 001 – abstractions (In Review) — define EnvClient and its close() ("Explicit close() ensures proper resource cleanup"). Part 1 supports that intent and changes no public signature — aligned context, not a conflict.

Tier 1: Fixes Required

None.

Tier 2: Alignment Discussion

Principle Conflicts

None identified.

• Cancel-safe teardown reinforces production-readiness / minimize lifecycle deltas (no container/session leak under cancellation) and RFC 002's close() cleanup contract. env_client.py remains client-only (no server/ imports) — client-server separation intact. Gym API signatures, generics, Pydantic wire types, MCP/agent isolation, rewards-in-env all untouched.
• Both security fixes reinforce the security invariants: no credential exposure, and defense-in-depth against a local attacker planting a discovery cache / forging CLI/log lines via untrusted metadata.

RFC Conflicts

ALIGNMENT FLAG: Control-char path rejection narrows the Draft RFC 011 0.1-draft profile grammar

• Principle/RFC at stake: RFC 011 (Draft) — Environment Card / catalog relative-path profile
• The concern: This tightens the normative 0.1-draft path grammar (rejecting C0/DEL/C1 + U+2028/U+2029 in models.py and all four packaged schemas). RFC 011 (line 227) defines path only as "the normalized relative path" and is silent on control characters. The change is aligned with RFC 011 §8 (untrusted-metadata safety) and is clearly correct, but it advances the profile's normative constraints ahead of the RFC text while the RFC is still Draft. Non-blocking — worth folding the control-char exclusion (and the "printable UTF-8 incl. spaces" allowance) into RFC 011 §8 so the spec and schemas stay in step.
• Suggested reviewer: @thegovind (RFC 011 author + original discovery/models.py & schema author)

Suggested reviewers by area

• Cancel-safe teardown (env_client.py): @burtenshaw (top committer / client lifecycle) + @Darktex (INVARIANTS client-server + lifecycle-delta). Continues #1162's cancel-safe close work.
• XDG cache hardening (auto/_discovery.py): @Darktex + @burtenshaw (top _discovery.py committers); cc @thegovind (RFC 011 §8 cache scoping). This implements the relative-XDG_CACHE_HOME nit raised during #1167 review.
• Path control-char rejection (discovery/models.py + schemas): @thegovind.

Summary

• 0 mechanical issues to fix
• 1 alignment point for human review (non-blocking RFC 011 awareness)
• 0 blocking conflicts

Clean, security-positive, well-tested PR. Verdict is an assessment (COMMENT), not a gate.

  

_{Sent by Cursor Automation: Pre-review}

[cursor]

Key correctness fix for single-session environments. A timed-out _receive() detaches its socket and closes it in the background (to keep the timeout prompt); reconnecting must wait for that close handshake to finish before opening a replacement, otherwise the old server-side session still occupies the (often capacity-1) slot and the retry is rejected. Draining here — after the early-return for a reusable same-loop socket, so healthy reuse isn't penalized — is the right spot. Verified by test_reconnect_waits_for_dropped_socket_to_release_capacity.

[cursor]

The deferred-cancellation teardown is the crux of this PR and looks correct: child-close → pending-close drain → _disconnect_async (in a finally) → provider stop (outer finally) all run even when the caller is cancelled, and the first captured CancelledError is re-raised only after cleanup completes. This closes the prior regression where a cancel during the pending-close gather skipped provider teardown and leaked the container/process. The 4 new teardown tests cover child-cancel, current-socket-cancel, and pending+current-socket-cancel.

Optional (non-blocking): _close_async now nests three try/except CancelledError/finally layers — a one-line comment naming the layers (children → pending → disconnect → provider) would ease future maintenance.

[cursor]

Implements the XDG-spec hardening flagged during the #1167 review: a relative XDG_CACHE_HOME is now ignored (rather than resolved CWD-relative, which an untrusted checkout could exploit to plant a cache), and the ~/.cache fallback fails closed with RuntimeError when Path.home() isn't absolute. Good defense-in-depth on top of #1167's trusted-open/atomic-save. Covered by test_relative_xdg_cache_home_cannot_redirect_into_working_tree and test_relative_home_cannot_restore_a_relative_cache_path.

[cursor]

This separate unanchored negative clause (not just widening the char classes inside the main anchored pattern) is the load-bearing part. jsonschema compiles these patterns with Python's re, whose $ matches before a trailing , so the anchored ^…$ pattern alone would still admit e.g. envs/foo . The {"not": {"pattern": "[\\x00-\\x1f\\x7f-\\x9f\\u2028\\u2029]"}} clause rejects control chars anywhere and closes that bypass — which is exactly why the test suite moves envs/trailing , . , .. , etc. from the valid list to the invalid list (a real latent hole, not just a test churn).

Model (relative_path) and all four schemas use the identical codepoint set (0x00–0x1F, 0x7F–0x9F, U+2028/U+2029), so model↔schema parity holds — nicely pinned by test_environment_locator_is_a_safe_repository_relative_path asserting both raise, and test_schema_and_model_preserve_valid_relative_locators keeping envs/café/envs/東京 valid.

[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]

Stale Bugbot comment from a previous run.

[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 da95efb. Configure here.}

[cursor]

Stale comment

Do not merge — this tip regresses just-landed #1169.

da95efb2 (“Merge main while retaining safe MCP rollback”) removes shared-session production connect (/ws?session_id=), forces use_production_mode = False, and strips server attach support from http_server.py / tests. Bugbot Medium is correct: leftover use_production_mode HTTP tool paths no longer share the WebSocket session.

Please rebase the discovery + cancel-safe EnvClient teardown fixes on top of #1169 without rolling back production MCP shared-session. Keep #1169’s attach/deferred-close/cancel-safe connect intact.

(Own-PR limitation: cannot set CHANGES_REQUESTED via API; treating as hard hold in release automation.)

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Stale comment

Re-reviewed tip de95e72b (“isolate explicit HTTP tools mode”).

Still a hard hold vs just-landed #1169. This tip:

• Removes /ws?session_id= attach, _session_pending_closes, and attach tests from http_server.py
• Defaults _use_production_mode=False even for MCPToolClient(mode="production"), so default path is WS-only
• Makes use_production_mode=True tools-only HTTP (no WS) — fixes the dual-session Bugbot by abandoning shared-session rather than preserving #1169

Discovery + cancel-safe EnvClient teardown may still be good. Need Ben’s explicit call:

1. Keep #1169 — drop the MCP/http_server rollback from this PR; rebase discovery/teardown only, or
2. Accept rollback — deliberately replace shared-session with isolated tools-only opt-in (and communicate that to mugenkyou)

Won’t treat green CI as mergeable until that choice is recorded.

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Stale comment

Tip 5de7a2dc clears the #1169 regression hold: MCP client + http_server shared-session/pending_closes//ws?session_id= match main again; MCP rollback moved out (“separate shared MCP follow-up”).

Remaining diff is the intended 0.4.3 blocker set: discovery path/XDG hardening + cancel-safe EnvClient teardown/reconnect. Cleared hold on that scope.

Note: PR is BEHIND main after #1000 (bot 403 on update-branch). Please update-branch / rebase, wait for exact-head CI green, then human-APPROVE + squash-merge.

(Own-PR limitation: cannot APPROVE via API.)

View PR

  

_{Sent by Cursor Automation: Release}

[cursor]

Exact-head review d97979c9 (post keep-#1169)

Scope vs main cdcd41a5: only the intended 0.4.3 blocker set — discovery path/XDG hardening + cancel-safe EnvClient teardown/reconnect. Zero diff in mcp_client.py / http_server.py (shared-session production MCP from #1169 preserved). #1000 / #1081 landed on main separately and are not part of this PR’s file list.

Verified at this tip

• Relative paths reject C0/DEL/C1 + U+2028/U+2029; schemas regenerated; printable Unicode/spaces still allowed
• Relative XDG_CACHE_HOME ignored; non-absolute HOME fails closed
• Socket closes tracked/shielded; reconnect drains pending closes; multi-child cancel-safe teardown with deferred CancelledError

CI: required checks green at exact head (Tests 3.11/3.12, Package CI, lint, locks, docs, CodeQL, catalog). Bugbot still running — non-blocking once terminal.

Merge gate: own-PR limitation — cannot APPROVE via API. Needs a human APPROVE on d97979c9, then squash-merge with --match-head-commit (or merge directly). Ready for that once you approve.

Not a release-version decision: Thursday 0.4.3 vs 0.5.0 rollout is still a separate Ben call.

View PR

  

_{Sent by Cursor Automation: Release}