fix(tasks): review-loop fixes — parity revert + close Native* guardrail hole + CI enforcement (#801)

* revert(tasks): keep mujoco_playground pick/open_cabinet as step() overrides

The #798 migration of these two delta-control tasks to _process_action was NOT
strictly behavior-preserving at episode boundaries: the old step() rebinds
self._last_action = real_actions AFTER super().step() runs auto-reset, so for
envs that terminate this step the next-episode delta baseline is the final
action; moving the latch into the pre-step _process_action hook makes the reset
pose survive instead. A multi-angle review flagged this as a behavior change
smuggled into a 'behaviour identical' refactor (and handover, unmigrated, now
disagreed). Per the repo's load-bearing-parity rule, revert to the exact
step() overrides and document why; a genuine delta-reset fix should be a
separate, parity-tested change. Re-adds both to KNOWN_STEP_OVERRIDES. 7/7
guardrail tests pass; ruff clean.

* fix(tests): close Native* guardrail bypass, seed NativeLiberoEnv, enforce in CI

Multi-angle clean-context review found two real guardrail gaps:
- The Tier-3 exemption was NAME-based (Native*/Passthrough*), so NativeLiberoEnv
  (libero/native_libero.py) — a first-party BaseTaskEnv subclass with a seed-less
  reset + step + close — silently dodged the contract. Make the exemption
  PATH-only (_native/ , _passthrough); in-contract classes are now always checked.
  Add seed to NativeLiberoEnv.reset (deterministic native replay; forwarded
  best-effort to handler.set_seed). Allowlist its native-port step/close explicitly.
- The guardrail ran in NO CI (repo only had deploy-docs). Add
  .github/workflows/task-contract.yml running the backend-free guardrail on PRs
  touching tasks/ — so the contract is actually enforced, not manual-only.
Also harden test_unified_bases_accept_seed to catch async reset, and drop a
stray benchmark artifact swept into the revert commit.

7/7 guardrail tests pass; ruff clean; workflow YAML valid.

Author: geng-haoran

.github/workflows/task-contract.yml

@@ -0,0 +1,28 @@
+name: Task contract guardrail
+
+# Enforces the backend-free task-architecture conformance guardrail
+# (tests/test_task_reset_seed_contract.py): no Tier-1 task may drop reset(seed)
+# or add a new step()/close() override beyond the shrink-only allowlists.
+# Pure-AST, no simulator/torch — fast and runs on every PR.
+
+on:
+  pull_request:
+    paths:
+      - "roboverse_pack/tasks/**"
+      - "tests/test_task_reset_seed_contract.py"
+      - ".github/workflows/task-contract.yml"
+  push:
+    branches: [main, develop]
+  workflow_dispatch:
+
+jobs:
+  task-contract:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
+        with:
+          python-version: "3.11"
+      - run: python -m pip install --upgrade pip pytest
+      - name: Run task-contract guardrail
+        run: python -m pytest tests/test_task_reset_seed_contract.py -v

roboverse_pack/tasks/libero/native_libero.py

@@ -165,7 +165,13 @@ def _terminated(self, env_states=None) -> torch.Tensor:
     def _get_initial_states(self):
         return None
 
-    def reset(self, states=None, env_ids=None):
+    def reset(self, states=None, env_ids=None, seed=None):
+        # Native LIBERO replay is deterministic (fixed qpos0/qvel0, no RNG); ``seed`` is
+        # accepted for the env.reset(seed=) contract and forwarded best-effort to the handler.
+        if seed is not None:
+            set_seed = getattr(self.handler, "set_seed", None)
+            if callable(set_seed):
+                set_seed(seed)
         nq, nv = self._m.nq, self._m.nv
         with self.handler.physics.reset_context():
             self._d.qpos[:] = self._qpos0[:nq]

roboverse_pack/tasks/mujoco_playground/open_cabinet.py

@@ -161,21 +161,23 @@ def reset(self, states=None, env_ids=None, seed=None):
 
         return super().reset(states=states, env_ids=env_ids, seed=seed)
 
-    def _process_action(self, actions):
-        """Delta position control (sanctioned action hook).
+    def step(self, actions):
+        """Step with delta control.
 
-        Replaces the old ``step`` override: ``RLTaskEnv.step`` calls
-        ``_process_action`` before clamping/applying, so behaviour is identical
-        (delta + clamp, latch ``_last_action``) without overriding step.
+        Kept as a ``step`` override (not ``_process_action``) to preserve the
+        episode-boundary ``_last_action`` semantics: it is rebound to
+        ``real_actions`` *after* ``super().step()``'s auto-reset, so a migration
+        to the pre-step hook would change behaviour for terminating envs.
         """
         # mj: delta = action * self._config.action_scale
         # mj: ctrl = state.data.ctrl + delta
         # mj: ctrl = jp.clip(ctrl, self._lowers, self._uppers)
         delta_actions = actions * self._action_scale
         new_actions = self._last_action + delta_actions
         real_actions = torch.maximum(torch.minimum(new_actions, self._action_high), self._action_low)
+        obs, reward, terminated, time_out, info = super().step(real_actions)
         self._last_action = real_actions
-        return real_actions
+        return obs, reward, terminated, time_out, info
 
     def _reward_handle_target(self, env_states) -> torch.Tensor:
         """Reward for bringing handle to target position."""

roboverse_pack/tasks/mujoco_playground/pick.py

@@ -125,18 +125,22 @@ def reset(self, states=None, env_ids=None, seed=None):
             self._last_action[env_ids] = self._initial_states.robots[self.robot_name].joint_pos[env_ids, :]
         return super().reset(states=states, env_ids=env_ids, seed=seed)
 
-    def _process_action(self, actions):
-        """Delta position control (sanctioned action hook).
-
-        Replaces the old ``step`` override: ``RLTaskEnv.step`` calls
-        ``_process_action`` before clamping/applying, so behaviour is identical
-        (delta + clamp, latch ``_last_action``) without overriding step.
+    def step(self, actions):
+        """Step with delta control.
+
+        NOTE: kept as a ``step`` override (not migrated to ``_process_action``)
+        because ``_last_action`` is rebound to ``real_actions`` *after*
+        ``super().step()`` runs auto-reset — so for envs that terminate this
+        step, the next-episode delta baseline is the final action, not the reset
+        pose. Moving the latch into ``_process_action`` (pre-step) would change
+        that episode-boundary behaviour, so this stays explicit and allowlisted.
         """
         delta_actions = actions * self._action_scale
         new_actions = self._last_action + delta_actions
         real_actions = torch.maximum(torch.minimum(new_actions, self._action_high), self._action_low)
+        obs, reward, terminated, time_out, info = super().step(real_actions)
         self._last_action = real_actions
-        return real_actions
+        return obs, reward, terminated, time_out, info
 
     def _reward_box_target(self, env_states) -> torch.Tensor:
         """Reward for bringing box to target position and orientation."""

tests/test_task_reset_seed_contract.py

@@ -10,9 +10,10 @@
 
 * **Tier 1** — classes in the unified contract: they (transitively) subclass
   ``BaseTaskEnv`` / ``RLTaskEnv`` / ``ManagerBasedRVEnv``. Only these are checked.
-* **Tier 3** — external/native passthrough adapters (modules under ``_native``/
-  ``_passthrough`` or classes named ``Native*``/``Passthrough*``) are an
-  explicitly-exempt compatibility tier and are skipped.
+* **Tier 3** — external/native passthrough adapters under dedicated ``_native``/
+  ``_passthrough`` paths are an explicitly-exempt compatibility tier and skipped.
+  (Exemption is PATH-based only — a first-party in-contract task is checked even
+  if its class name starts with ``Native``; see ``_is_tier3``.)
 * Non-task helpers (controllers, sensors, command/actuator managers, success
   evaluators, sessions) are not in the inheritance graph to ``BaseTaskEnv`` and
   are therefore ignored.
@@ -68,6 +69,9 @@
 # blocks any NEW Tier-1 step()/close() override so the architecture cannot degrade
 # while the existing ones are migrated. Remove an entry when you drop its override.
 KNOWN_STEP_OVERRIDES = frozenset({
+    "libero/native_libero.py",  # native LIBERO replay port (Tier-1 by base, kept explicit)
+    "mujoco_playground/open_cabinet.py",
+    "mujoco_playground/pick.py",
     "beyondmimic/metasim/envs/base_legged_robot.py",
     "calvin/base_table.py",
     "humanoid/base/base_legged_robot.py",
@@ -89,6 +93,7 @@
     "simpler_env/_metasim/place_task.py",
 })
 KNOWN_CLOSE_OVERRIDES = frozenset({
+    "libero/native_libero.py",
     "robosuite/robosuite_env.py",
 })
 
@@ -126,11 +131,17 @@ def _in_contract(name: str, graph: dict[str, set[str]], seen: set[str] | None =
 
 
 def _is_tier3(rel_path: str, class_name: str) -> bool:
-    """Native/passthrough adapters are an explicitly-exempt compatibility tier."""
+    """Native/passthrough adapters are an explicitly-exempt compatibility tier.
+
+    Exemption is PATH-based only (dedicated ``_native``/``_passthrough`` dirs/files).
+    A NAME-based exemption (``Native*``/``Passthrough*``) was removed: it let
+    first-party in-contract tasks (e.g. ``NativeLiberoEnv(BaseTaskEnv)``, which
+    lives in ``libero/native_libero.py`` — not a ``_native/`` dir) silently dodge
+    the contract. In-contract classes are now always checked regardless of name;
+    only genuine adapters under ``_native``/``_passthrough`` paths are exempt.
+    """
     p = "/" + rel_path
-    if "/_native/" in p or "_passthrough" in rel_path:
-        return True
-    return class_name.startswith(("Native", "Passthrough"))
+    return "/_native/" in p or "_passthrough" in rel_path
 
 
 def _reset_lacks_seed(fn: ast.FunctionDef) -> bool:
@@ -207,7 +218,11 @@ def test_unified_bases_accept_seed():
         tree = ast.parse((_TASKS / rel).read_text(encoding="utf-8"))
         for cls in (n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)):
             for fn in cls.body:
-                if isinstance(fn, ast.FunctionDef) and fn.name == "reset" and _reset_lacks_seed(fn):
+                if (
+                    isinstance(fn, (ast.FunctionDef, ast.AsyncFunctionDef))
+                    and fn.name == "reset"
+                    and _reset_lacks_seed(fn)
+                ):
                     regressed.append(rel)
     assert not regressed, f"Base classes regressed to reset() without seed: {sorted(set(regressed))}"