fix(tasks): review-loop fixes — parity revert + close Native* guardrail hole + CI enforcement#801
Merged
Conversation
…rrides 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.
…orce 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.
Deploying roboverse-release with
|
| Latest commit: |
5202998
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://052e1902.roboverse-release.pages.dev |
| Branch Preview URL: | https://deep-fix-revert-mjpg-process.roboverse-release.pages.dev |
geng-haoran
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidated fixes from the 5-angle clean-context review of the unification work.
1. Behavior parity (revert): keep
mujoco_playgroundpick/open_cabinet asstep()overrides — the #798_process_actionmigration changed episode-boundary_last_actionsemantics (latch survived auto-reset instead of being discarded). Restored exact behavior; re-allowlisted.2. Guardrail hole (the real one): the Tier-3 exemption was NAME-based, so
NativeLiberoEnv(BaseTaskEnv)(seed-less reset + step + close) silently dodged the contract. Made exemption PATH-only; in-contract classes now always checked. AddedseedtoNativeLiberoEnv.reset; allowlisted its native-port step/close.3. Enforcement gap: guardrail ran in NO CI → added
.github/workflows/task-contract.yml(backend-free, runs on tasks/ PRs).4. Hardening:
test_unified_bases_accept_seednow also catchesasyncreset.7/7 guardrail tests pass; ruff clean. Not a regression cleanup — the 5 reviewers found zero runtime regressions (seed reproducibility verified on MuJoCo); these close the soundness/parity gaps they flagged.
Tracked as follow-ups (pre-existing, not regressions): mjlab v2 unseeded
np.random.default_rng(sim-repro completeness), libero checker-reset ordering unification (P3 base merge), post-step hook for the reward-in-step overrides.