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.
Uh oh!
There was an error while loading. Please reload this page.
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.
LeadcodeDev added a commit
that referenced
this pull request
Aug 11, 2026
… over data (#174) Closes the "parameterised templates and data iteration" gap, aimed at the failure mode the original audit named as dominant: a generator duplicating a repeated structure by hand, every copy an opportunity to drift. Ten near-identical cards used to be ten JSON subtrees written out. Now: "components": { "stat_card": { "params": {...}, "template": {...} } }, "children": [{ "for-each": [ {...}, {...}, {...} ], "template": { "use": "stat_card", "props": {...} } }] `params` reuses `config`'s exact shape, so a component parameter is a variable scoped to one instance instead of the file. Omitting `default` makes it required. The overrides key is `props`, not `config`, and that is not an inconsistency: `variables::substitute` deliberately skips any object carrying a literal `config` key, to protect the root declarations block. Reusing the name would have left every `for-each` nested inside a `use` silently unsubstituted — the failure mode this work exists to remove. **`--fix` refuses on these scenarios**, exactly as it already does for `include`. Violation paths carry resolved indices while `--fix` navigates raw JSON; an iteration over ten items shifts everything after it by nine. PR #145 established the refusal for `include` and #160 confirmed doing better is impractical. Patching the wrong node silently is the one unacceptable answer. Every failure gets a named error saying where: a cycle reports the chain rather than overflowing the stack, iterating a non-array reports what it found instead, an unknown component, a missing required parameter and an undeclared prop key all say so. A `for-each` that silently produced nothing because a key was misspelled would be the worst possible outcome here. Pass order is fixed and documented: substitution, then expansion, then `include`, per document. So a `for-each` can iterate an array that came from a `config` variable, and `components` is strictly file-local — reaching into an included file's definitions is a named error in both directions rather than an accident of scope. One correction to the delivered work: the pre-expansion unresolved-reference scan reported every template binding as a typo — six warnings on the canonical example, each accusing the author of a mistake they had not made. Warnings that are reliably wrong teach the reader to ignore warnings, which would have cost more than the scan is worth, and would have undermined every real diagnostic this chantier added. That scan now skips the directive bodies, and runs again after expansion where the keys are gone and a leftover `$name` is unambiguous. Both directions are tested: a correct binding warns about nothing, a misspelled one is still caught.
This was referenced Aug 12, 2026
LeadcodeDev added a commit
that referenced
this pull request
Aug 12, 2026
Closes the "media metadata readable from the scenario" gap. Nothing answered "how long is this audio track" or "what size is this image", so an author timing a scene against a track had to measure it somewhere else, by hand. `rustmotion info` grows a Media assets section, following the same walk-and-print shape the Springs and Text sizes sections already use rather than adding a `probe` subcommand that would have rebuilt the same plumbing and split "everything knowable about this scenario" across two commands. Three probes, each reusing what already existed: - Audio goes through `decode_audio_file`, the same decoder every render uses — never a second decode path. The cost is a full decode, and that is stated rather than hidden: this repository has no header-only audio path. - Images use `image`'s `into_dimensions`, a header-only read the crate already offered and nothing used; every existing decode goes through `Image::from_encoded`, which rasterises in full to answer a question about dimensions. - Video shells one `ffprobe -show_streams -show_format`, mirroring the existing `ffmpeg_available()` guard, and never decodes a frame. **Probing can never fail the command.** A remote asset is never opened — not the network, not the disk — because reading a header is not worth an unbounded download. A missing file is reported as missing before any decode is attempted. An unreadable one carries the exact reason. In all three cases `info` exits 0 and every other section still prints: audio track 1: audio "does-not-exist.wav" → file not found layer 1: image "corrupt.png" → could not read: unexpected end of file layer 2: image "https://…/remote.png" → remote asset, not fetched The second gap in this batch — secondary artefacts emitted during the render — is deliberately not delivered. Three independent blockers, each sufficient alone, are recorded in the issue filed alongside: the only construction site of `ResolvedScenario` is outside this change's scope (and a second file would break compiling anyway); per-frame RGBA buffers only exist inside the per-format encode loops, so the one in-scope function able to produce a frame *is* the render, and calling it again reproduces exactly the cost of the separate `still` invocation the feature is meant to replace; and `captions.rs` writes non-atomically, contradicting the discipline PR #145 and #151 established, while being frozen here.
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 freeto 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.