Enforce the LocalMotion contract before mutating the tree - #60
Merged
Merged
Conversation
_extend_along_edge inserted a custom validator's configurations as it went and only then checked the endpoint, and skipped that check when the payload was empty. A validator returning reached=True with no configurations on a nonzero motion therefore reported success with the source node, breaking the meaning of reached established in #47, and a malformed success could partially mutate the tree. The whole LocalMotion is now checked first. A zero-length motion succeeds without adding a node regardless of the payload. On a nonzero motion, reached=True with no configurations or with a final configuration that is not the exact target raises MotionContractError, since that is a validator bug. For custom validators, a claimed success containing an inadmissible configuration is rejected whole and nothing is stored; a partial (reached=False) result keeps its admissible prefix, which is documented as deliberate. Tests cover the empty-but-reached payload in direct growth, bidirectional connection, and shortcut smoothing; whole rejection of a claimed success with a bad middle state; prefix retention for partial results; zero-length motions; wrong-shaped endpoints; and that the default validator never trips the checks. Fixes #54. Refs #17, #46, #47.
11 tasks
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.
Why
#54: the endpoint check ran after configurations were inserted and only when the payload was nonempty, so
LocalMotion(configs=[], reached=True)on a nonzero motion reported success with the source node still current. Reproduced on main:_growreturned reached with a one-node tree, andsolvereturned a two-waypoint path whose only segment was never validated.What
The whole
LocalMotionis checked before the tree is touched, with one documented policy per case:reached=Truewith no configurations on a nonzero motion, or a final configuration whose shape or value is not the exact target): raises the newMotionContractError. Rationale: this is a bug in the validator, and treating it as "not reached" would hide it.reached=False): the admissible prefix is stored, as before. Documented as deliberate so partial extensions keep their progress.LocalMotion's docstring states the contract; changelog updated.Fixes #54. Refs #17, #46, #47.
Test plan
Eight new tests in test_motion.py: empty-but-reached raises in direct growth (tree untouched), during bidirectional connection, and during shortcut smoothing (a validator honest for growth but lying for long shortcuts); a claimed success with a bad middle state stores nothing, not even its admissible first config; a partial result keeps its prefix; zero-length motions succeed without duplicates for both a target-returning and an empty payload; a wrong-shaped endpoint raises; the default validator never trips the checks over seeded plans with smoothing. One #46 test updated to the whole-rejection policy.
uv run pytest tests/: 276 passed, 1 skipped; ruff clean.🤖 Generated with Claude Code