fix(cli): stop the helper commands destroying files they do not own - #145
Merged
LeadcodeDev merged 1 commit intoAug 8, 2026
Merged
Conversation
`skills install` wrote CLAUDE.md wholesale and `skills uninstall` deleted it, treating a file the user authors as rustmotion's property: a project with its own build notes lost them on install and lost the file on uninstall. rustmotion now claims a delimited block and never touches anything outside it — install merges the block in place, uninstall removes only the block and deletes the file solely when that block was all it held. `--fix` serialises `LoadedScenario::raw`, which is the document *after* variable substitution and include resolution — not the document on disk. For a plain JSON scenario the two coincide; for anything templated the write silently replaces the source with its own expansion. That single cause produced three separate defects: an HTML input came back as JSON, a `config` block and every `$var` disappeared (making `--var` a no-op on the rewritten file), and `include` got inlined so path-based patches landed on nodes the source never contained. One rule closes all three: only write back a source `--fix` can reproduce. Anything else is refused with a message naming the file and what to do instead. The check reads the bytes on disk rather than the loaded tree, because by then substitution has already erased the markers that make the write unfaithful.
This was referenced Aug 8, 2026
LeadcodeDev
added a commit
that referenced
this pull request
Aug 9, 2026
…plementing it (#160) Nine confirmed audit findings on the geometry pass. Almost all of them are the same structural mistake: the validator forked a piece of the engine — a padding constant, a root style, a transform pivot — instead of calling the function the engine uses. Then the two drifted, and the validator started reasoning about a geometry that is not the one being rendered. The fix is mostly deletion of the duplicate, not correction of its value. False negatives — a broken video shipped with no warning: - `timeline` style states and audio-reactive transforms were invisible even under `--strict-anim`: the box tree was built once with `anim: None`, and both features only apply when a real animation context is present. It is now rebuilt per sample through `build_scene_from_refs` with a real `BuildAnimationCtx` — the same call `render_with_new_pipeline_iter` makes per rendered frame. - Animated rotation was not modelled at all: `transform_bbox` read only translate and scale, so a `spin` left the frame undetected at every sample. It now folds through `apply_static_node_transform`, the same 4-corner AABB the static path already used. - The 40-sample cap dropped `--strict-anim` to one sample every 1.5s on a 60s scene, so any excursion shorter than the step slipped through. Raised to 480, which holds the promised 8/s up to 60s. Measured cost on a 60s scene with 15 animated components: 309ms release, 543ms debug. False positives — a correct scenario blocked, and `--fix` then damaging it: - `unwrappable_text_overflow` was the only content-vs-own-box check outside the clipping-ancestor guard, so text legitimately clipped by an `overflow: hidden` ancestor was rejected and `--fix` stripped a correct `white-space`. - `transform-origin` was ignored by the static transform fold, which always pivoted at the box centre. - `content_overflows_card` is retired rather than patched. Its guard made it reachable only when neither the card nor anything between it and the node clips — which is exactly the `overflow: visible` case both CLAUDE.md and geometry-safety.md document as legal ("a badge sticking out of a card is legal"). When the card does clip, the guard already suppresses the whole block. There is no configuration where firing is both reachable and consistent with the documented contract. Content leaving the *device* is still caught — that is `check_viewport`'s job, and a test now pins that guarantee so the removal cannot silently become a blind spot. Wrong geometry reported: - `world` views were validated against the slide root layout. The centred default the world renderer synthesises is now a single shared `world_default_scene_layout`, used by both. - `check_auto_scroll` assumed 16+16px of padding and, for terminal, the wrong default font size and a CSS line-height the painter ignores. It now calls `CodeblockIntrinsic` and `TerminalIntrinsic`, the measurers layout itself uses. The `--fix` refusals from PR #145 were verified rather than rewritten: two end-to-end tests now drive `cmd_validate` over a templated scenario and a two-file `include` scenario, both with a real violation, and assert the source files come back byte-identical. Doing better than refusing is not possible for `include`, and not safe for templates: variable substitution can replace a scalar with an object, so a path computed on the resolved tree need not exist in the source. Tests: full workspace green on this branch alone. 7/8 examples validate; the eighth is issue #157, pre-existing on main.
LeadcodeDev
added a commit
that referenced
this pull request
Aug 10, 2026
…145) `skills install` wrote CLAUDE.md wholesale and `skills uninstall` deleted it, treating a file the user authors as rustmotion's property: a project with its own build notes lost them on install and lost the file on uninstall. rustmotion now claims a delimited block and never touches anything outside it — install merges the block in place, uninstall removes only the block and deletes the file solely when that block was all it held. `--fix` serialises `LoadedScenario::raw`, which is the document *after* variable substitution and include resolution — not the document on disk. For a plain JSON scenario the two coincide; for anything templated the write silently replaces the source with its own expansion. That single cause produced three separate defects: an HTML input came back as JSON, a `config` block and every `$var` disappeared (making `--var` a no-op on the rewritten file), and `include` got inlined so path-based patches landed on nodes the source never contained. One rule closes all three: only write back a source `--fix` can reproduce. Anything else is refused with a message naming the file and what to do instead. The check reads the bytes on disk rather than the loaded tree, because by then substitution has already erased the markers that make the write unfaithful.
LeadcodeDev
added a commit
that referenced
this pull request
Aug 10, 2026
…plementing it (#160) Nine confirmed audit findings on the geometry pass. Almost all of them are the same structural mistake: the validator forked a piece of the engine — a padding constant, a root style, a transform pivot — instead of calling the function the engine uses. Then the two drifted, and the validator started reasoning about a geometry that is not the one being rendered. The fix is mostly deletion of the duplicate, not correction of its value. False negatives — a broken video shipped with no warning: - `timeline` style states and audio-reactive transforms were invisible even under `--strict-anim`: the box tree was built once with `anim: None`, and both features only apply when a real animation context is present. It is now rebuilt per sample through `build_scene_from_refs` with a real `BuildAnimationCtx` — the same call `render_with_new_pipeline_iter` makes per rendered frame. - Animated rotation was not modelled at all: `transform_bbox` read only translate and scale, so a `spin` left the frame undetected at every sample. It now folds through `apply_static_node_transform`, the same 4-corner AABB the static path already used. - The 40-sample cap dropped `--strict-anim` to one sample every 1.5s on a 60s scene, so any excursion shorter than the step slipped through. Raised to 480, which holds the promised 8/s up to 60s. Measured cost on a 60s scene with 15 animated components: 309ms release, 543ms debug. False positives — a correct scenario blocked, and `--fix` then damaging it: - `unwrappable_text_overflow` was the only content-vs-own-box check outside the clipping-ancestor guard, so text legitimately clipped by an `overflow: hidden` ancestor was rejected and `--fix` stripped a correct `white-space`. - `transform-origin` was ignored by the static transform fold, which always pivoted at the box centre. - `content_overflows_card` is retired rather than patched. Its guard made it reachable only when neither the card nor anything between it and the node clips — which is exactly the `overflow: visible` case both CLAUDE.md and geometry-safety.md document as legal ("a badge sticking out of a card is legal"). When the card does clip, the guard already suppresses the whole block. There is no configuration where firing is both reachable and consistent with the documented contract. Content leaving the *device* is still caught — that is `check_viewport`'s job, and a test now pins that guarantee so the removal cannot silently become a blind spot. Wrong geometry reported: - `world` views were validated against the slide root layout. The centred default the world renderer synthesises is now a single shared `world_default_scene_layout`, used by both. - `check_auto_scroll` assumed 16+16px of padding and, for terminal, the wrong default font size and a CSS line-height the painter ignores. It now calls `CodeblockIntrinsic` and `TerminalIntrinsic`, the measurers layout itself uses. The `--fix` refusals from PR #145 were verified rather than rewritten: two end-to-end tests now drive `cmd_validate` over a templated scenario and a two-file `include` scenario, both with a real violation, and assert the source files come back byte-identical. Doing better than refusing is not possible for `include`, and not safe for templates: variable substitution can replace a scalar with an object, so a path computed on the resolved tree need not exist in the source. Tests: full workspace green on this branch alone. 7/8 examples validate; the eighth is issue #157, pre-existing on main.
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.
Refs #142 — CLI workstream, first of three. Four confirmed High findings, all of them cases where a helper command destroys user-authored content.
skills install/uninstallvs. the project's CLAUDE.mdinstallwroteCLAUDE.mdwholesale;uninstallcalledremove_fileon it unconditionally. Both treat a file the user authors as rustmotion's property.Reproduced in a clean directory holding
# My Project / Build with make. Never delete this.:skills installreplaced it outright,skills uninstalldeleted it;<!-- rustmotion:start -->…<!-- rustmotion:end -->block leaving the surrounding content intact, and uninstall removes only that block, keeping the file. The file is deleted only when our block was all it ever held.Re-installing replaces the block in place rather than stacking copies, which the previous whole-file write got right by accident and a naive append would have got wrong.
--fix, three defects and one cause--fixserialisesLoadedScenario::raw— documented invalidation.rs:36as the scenario after parsing, variable resolution and include resolution. That is not the document on disk. For a plain JSON scenario the two coincide and writing back is faithful; for anything templated it is not, and the write replaces the source with its own expansion.The audit filed these as three findings; they are one bug seen from three angles:
--fixwrote back.htmlconfig/$var--varbecomes a silent no-opincludeSo the fix is one rule, not three patches: only write back a source
--fixcan reproduce. Anything else is refused, with a message naming the file and saying what to do instead.The guard reads the bytes on disk rather than the loaded tree — by the time the tree exists, substitution has already erased the very markers that make the write unfaithful.
Verified end to end:
validate -f f.html --fixnow refuses and leaves the HTML byte-identical, where it previously overwrote it with JSON.Design note
refuse_fixreturns a typedFixRefusalrather than a bool so each case carries its own explanation;explain()is tested to always name the file and offer a next step, since a refusal with no way forward is just a different kind of dead end.Out of scope
batch(exit code, geometry bypass, path traversal) and the HTML dialect findings (<style>painted into the video, nested<scene>dropped, unknown tags degraded todiv, boolean coercion) are the two remaining CLI PRs in this chantier. Splitting them keeps each reviewable in one sitting.Verification
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(702 passed, 0 failed). 10 new tests: 5 on the CLAUDE.md merge, 5 on the fix guard.