fix(simulation): add_object's is_static is checked, not read by truthiness - #3554
Conversation
…iness is_static is tri-state, and the resolution that reads it tests by identity (is_static is False / is None) while every later read is a truthiness one, so a supplied value outside that domain escaped both on all three backends. Measured against a real compiled MuJoCo model, one add_object per case: shape="plane", is_static=False -> status="error" (documented) shape="plane", is_static=0 -> status="success", body static shape="plane", is_static=np.False_ -> status="success", body static 0 is the value that refusal answers (int(False) == 0), so one request got two verdicts by spelling, and the accepted spelling got exactly the quiet override the refusal exists to prevent. np.False_ is a value this flag's own domain accepts, and it took the same path because np.False_ is False is False. The truthy half inverted the other way: shape="box", is_static=False -> free body, fell 0.4751 m in 400 steps shape="box", is_static="false" -> welded body, fell 0.0000 m, success 'false' was then stored on SimObject.is_static, annotated bool and read by list_objects, the rebuild and randomization. Newton and Isaac read the flag purely by truthiness, so "false" fixed a body there too and 0 was stored raw. A supplied is_static now goes through the shared posture domain on all three backends and is normalized to a plain bool, so the identity reads below it are sound and no numpy scalar reaches the record. None stays the "unspecified" sentinel; the plane refusal, its wording and its named remedy are unchanged.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Closes the truthiness gap on add_object's tri-state is_static: the plane refusal tested by identity (is_static is False) while every later read was a truthiness one, so 0, "" and np.False_ reached the silent static override that refusal exists to prevent, and truthy strings ("false", "no", "off") welded bodies asked to be dynamic on all three backends. A supplied value now routes through the shared SimEngine._validate_posture_flags domain on MuJoCo, Newton and Isaac and is normalized to a plain bool so the identity reads below it are sound; None stays the documented "unspecified" sentinel and the plane refusal's wording is untouched.
What's good
- Guard placement is correct on all three backends: before any registry write, prim construction or solver rebuild, so a refusal leaves nothing behind (AGENTS.md > Review Learnings #86 > "Clean up on failure" is satisfied by never registering).
- Follows AGENTS.md > "Posture flags are checked, never read by truthiness" precisely, including the guard-where-the-caller-supplied-it rule and the numpy-bool normalization before the record annotated
bool. - Verified locally at head: all 66 tests in the contract file pass; with
strands_robots/simulation/reverted to the merge base and the head's tests kept, 35 fail — the pins do fail on pre-fix code, matching the PR description exactly. - Over-refusal guards (
True/False/None/np.True_/np.False_honored, the two postures measured as genuinely different physics) pass on both sides, so the declared domain cannot narrow silently. - Whole-tree graders spot-checked green at head: docstring xref roles, ASCII tool-result strings, test-name-describes-behaviour; ruff clean on every touched file; changelog fragment named with the PR number.
…arison grader refuses The docstring already states that 0 == False; asserting it in the body is a comparison decided when it is typed, which tests/test_no_vacuous_comparisons.py grades across the tree. The two add_object calls beneath it are the measurement.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Closes the truthiness gap on add_object's tri-state is_static on all three backends. The MuJoCo plane refusal tested the flag by identity (is_static is False) while every later read was a truthiness one, so 0, "" and np.False_ reached the silent static override that refusal exists to prevent, and truthy strings ("false", "no", "off") welded bodies asked to be dynamic — with the raw value then stored on SimObject.is_static (annotated bool). A supplied value now routes through the shared posture domain (SimEngine._validate_posture_flags → boolean_flag_error) on MuJoCo, Newton and Isaac, so a spelling one backend refuses is refused by all, and is normalized to a plain bool so the identity reads beneath it are sound. None remains the documented "unspecified" sentinel and the plane refusal, its wording and its remedy are unchanged.
What's good
- Exactly the AGENTS.md > "Posture flags are checked, never read by truthiness" shape: shared domain, refusal ahead of any side effect (nothing registered on refusal, asserted by the tests), structured error dicts throughout — never raises.
- The guard sits before the plane resolution, so
is_static=0gets the boolean refusal rather than the override, whilenp.False_(a value the domain accepts) is normalized and correctly reaches the plane refusal — both directions pinned. - Over-refusal guard cells (
True/False/None/np.True_/np.False_honored, plus the two-postures-are-different-physics non-vacuity check) pass pre-fix and post-fix, so the narrowing is exactly the non-boolean spellings. - Cross-backend parity is table-driven and GL-free via the existing
_newton_stub/_isaac_stub, extended with the inherited guard so it reads as exercised rather than absent. - Verified on the head tree:
tests/simulation/test_add_object_static_default_contract.py— 66 passed. Changelog fragment follows the<pr-number>-<slug>.mdconvention; added lines are ASCII-clean with no host paths.
What
is_staticis tri-state, and the resolution that reads it tests by identity (is_static is False/is None) while every later read is a truthiness one. A supplied value outside that domain escaped both, on all three backends. Measured on a real compiled MuJoCo model, oneadd_objectper case:is_static=Falseerror(documented)0on a planesuccessnp.False_on a planesuccess"false"on a boxsuccessNone/Truesuccess0is the value that refusal answers (int(False) == 0), so one request got two verdicts by spelling — and the accepted spelling got exactly the quiet override the refusal exists to prevent.np.False_is a value this flag's own domain (boolean_flag_error) accepts, and it took the same path becausenp.False_ is FalseisFalse.The truthy half inverted the other way. Both cubes below ask for a dynamic body; the left spells it
"false", the rightFalse:Both returned
status="success". The"false"body fell 0.0000 m in 400 steps; theFalsebody fell 0.4751 m.'false'was then stored onSimObject.is_static, annotatedbooland read bylist_objects, the scene rebuild and domain randomization. Newton and Isaac read the flag purely by truthiness, so"false"fixed a body there too and0was stored verbatim.Why
tests/simulation/test_add_object_static_default_contract.pyalready pins the plane refusal and names silently overriding it as "the obvious wrong fix... the caller would be told a dynamic plane was built and get a static one". That outcome was reachable through the refusal.Tests
A supplied
is_staticnow routes through the shared posture domain (SimEngine._validate_posture_flags) on MuJoCo, Newton and Isaac — so a spelling one backend refuses is refused by all of them — and is normalized to a plainboolso the identity reads below it are sound.Nonestays the "unspecified" sentinel; the plane refusal, its wording and its named remedy are untouched.69 cells added to the file that owns this contract, table-driven over the falsy/truthy/honored partitions and covering all three backends (GL-free stubs for Newton/Isaac). 35 fail on
main, all 66 pass after. The over-refusal cells (True/False/None/np.True_/np.False_honored, the two postures being genuinely different physics) pass both before and after — that is their job.Local gate:
ruff check+format --checkclean on 1,985 files;mypySuccess, 0 errors;tests/simulation/+ docstring/refusal graders 16,469 passed, 199 skipped; the 16 otheradd_object/is_staticconsumers 1,285 passed.Round changelog
Round 1 (
4dc50cbd+ base merge2bc1c8e5)call-test-lintwas red on one whole-tree grader,tests/test_no_vacuous_comparisons.py: the new test file asserted0 == Falseas a stated premise, and a comparison with a literal on both sides is decided when it is typed, so the grader refuses it. Dropped that one line; the docstring already states the premise and the twoadd_objectcalls beneath it are the measurement. This is the PR Workflow step 2 shape (narrow selection green, whole-tree grader red) - noted for the local gate above, which was atests/simulation/selection.Detect an untested overlap with the base branchwas red because fix(simulation): create_world's ground_plane is checked, not read by truthiness #3553 (create_world'sground_plane) landed onmujoco/simulation.pyandnewton/simulation.pyafter this branch's merge-base. Mergedmaininto the branch:git merge-treereported zero conflicts,git show --ccon the merge commit is 0 lines, and the PR's own diff againstmainis unchanged at 7 files, +296/-3. Composed tree: this PR's two test files 228 passed, fix(simulation): create_world's ground_plane is checked, not read by truthiness #3553'stest_create_world_ground_plane_posture_flag_domain.pypassed alongside.