Skip to content

Fix mjSpec signature ignoring the first equality constraint - #3549

Open
ukanwat wants to merge 3 commits into
google-deepmind:mainfrom
ukanwat:fix/signature-first-equality
Open

Fix mjSpec signature ignoring the first equality constraint#3549
ukanwat wants to merge 3 commits into
google-deepmind:mainfrom
ukanwat:fix/signature-first-equality

Conversation

@ukanwat

@ukanwat ukanwat commented Sep 5, 2026

Copy link
Copy Markdown

The equality loop in mjCModel::Signature() starts at i=1, so a spec with one equality hashes the same as a spec with none. Adding the first equality to an already compiled spec leaves spec and model signatures equal, and bind passes its check instead of asking for a recompile:

spec = mujoco.MjSpec(); ... ; model = spec.compile()
weld = spec.add_equality(...)
model.bind(weld)   # IndexError: Invalid index 0. Valid indices from 0 to -1
                   # expected: ValueError: The mjSpec does not match mjModel...

Every other list in Signature() starts at 0 and nothing pushes a placeholder into equalities_, so this looks like a copy-paste slip from cc2f57d. Fix is the loop bound; the new test compiles a spec and then adds one of each element type, asserting the signature changes every time. It fails on "first equality" without the fix.

Second commit is clang-format on a few pre-existing lines in the same file, otherwise the lint hook fails on any PR touching it.

Found while reading Signature() for #3397. I don't need git attribution if importing directly is easier.

The equality loop in mjCModel::Signature started at index 1, so a spec
with one equality hashed identically to a spec with none. Adding the
first equality to a compiled spec therefore left the spec and model
signatures equal, and bind passed its compatibility check instead of
asking for a recompile. Start the loop at 0 like every other element
list, and add a test that adding one element of each type changes the
signature.
Pre-existing lines in AttachPreservesJointOrder were not clang-format
clean, which fails the pre-commit lint hook for any PR touching this
file.
@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor

The loop-bound fix itself matches the surrounding Signature() construction, and I like that the regression checks the first equality as well as the other element classes rather than only asserting the literal i = 0 change.

I would add one small end-to-end regression for the failure mode described in the PR, though. The current test proves that spec->element->signature changes after adding an equality, but the user-visible bug is that a model compiled from the old spec still accepts a bind far enough to fail with an index error. A test that compiles, adds the first equality, and then verifies that binding the new element against the stale model is rejected as a spec/model mismatch would pin the actual contract as well as the hashing mechanism.

That would make the regression resilient to a future refactor of Signature() or bind validation where the signature-change test could still pass while the stale-model guard regressed.

Pins the user-visible contract behind the signature fix: compile, add
the first equality, and binding it against the compiled model must be
rejected as a spec/model mismatch rather than failing later with an
index error. Passes with the fix, fails without it.
@ukanwat

ukanwat commented Sep 6, 2026

Copy link
Copy Markdown
Author

Good point, thanks. Added a Python test that compiles, adds the first equality and expects model.bind() to raise the mismatch error. It fails on main with the IndexError from the description and passes with the fix.

@yuvaltassa
yuvaltassa requested a review from quagla September 7, 2026 16:06

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the current head. The signature loop now includes equality index 0, and the new Python regression pins the user-visible failure mode: after compiling a spec, adding the first equality makes binding against the stale model fail as a spec/model mismatch, while recompiling allows equality id 0 to bind normally. That closes the regression gap I raised. No blocker from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants