From 9f69196777db71a39f1f1424fde1b4f4e0d1a117 Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 14:11:21 +0200 Subject: [PATCH] fix(cli): stop the helper commands destroying files they do not own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- crates/rustmotion-cli/src/claude_md.rs | 128 +++++++++++++++++ .../rustmotion-cli/src/commands/validate.rs | 135 ++++++++++++++++++ crates/rustmotion-cli/src/lib.rs | 1 + crates/rustmotion-cli/src/skills.rs | 27 +++- 4 files changed, 285 insertions(+), 6 deletions(-) create mode 100644 crates/rustmotion-cli/src/claude_md.rs diff --git a/crates/rustmotion-cli/src/claude_md.rs b/crates/rustmotion-cli/src/claude_md.rs new file mode 100644 index 0000000..00a8da6 --- /dev/null +++ b/crates/rustmotion-cli/src/claude_md.rs @@ -0,0 +1,128 @@ +//! Merging rustmotion's guidance into a project `CLAUDE.md` without owning the file. +//! +//! `skills install` used to write `CLAUDE.md` wholesale and `skills uninstall` used +//! to delete it. Both treated a file the user authors as rustmotion's property: a +//! project with its own build notes lost them on install, and lost the file itself +//! on uninstall. +//! +//! Instead, rustmotion claims a delimited block and never touches anything outside +//! it. The markers are HTML comments so they stay invisible in rendered Markdown. + +pub const START: &str = ""; +pub const END: &str = ""; + +/// Byte range of the rustmotion block, markers included. +fn block_span(text: &str) -> Option<(usize, usize)> { + let start = text.find(START)?; + let end = text[start..].find(END)? + start + END.len(); + Some((start, end)) +} + +/// The document to write on install. +/// +/// - no file yet: the block alone; +/// - a file without a block: the block appended, existing content untouched; +/// - a file with a block: only the block replaced, in place. +pub fn merge(existing: Option<&str>, body: &str) -> String { + let block = format!("{START}\n{}\n{END}\n", body.trim_end()); + let Some(existing) = existing else { + return block; + }; + match block_span(existing) { + Some((start, end)) => { + let mut out = String::with_capacity(existing.len() + block.len()); + out.push_str(&existing[..start]); + out.push_str(block.trim_end()); + out.push_str(&existing[end..]); + out + } + None if existing.trim().is_empty() => block, + None => { + let mut out = existing.to_string(); + if !out.ends_with('\n') { + out.push('\n'); + } + out.push('\n'); + out.push_str(&block); + out + } + } +} + +/// The document to write on uninstall, or `None` when nothing rustmotion owns is +/// left and the file should be removed. +/// +/// A file the user also wrote in survives with its own content; a file that only +/// ever held our block is reported as removable. +pub fn strip(existing: &str) -> Option { + let Some((start, end)) = block_span(existing) else { + // No block: the file is entirely the user's. Never remove it. + return Some(existing.to_string()); + }; + let mut out = String::with_capacity(existing.len()); + out.push_str(&existing[..start]); + out.push_str(&existing[end..]); + if out.trim().is_empty() { + None + } else { + Some(format!("{}\n", out.trim_end())) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const BODY: &str = "# rustmotion\nGuidance."; + + #[test] + fn a_project_claude_md_survives_install_and_uninstall() { + let user = "# My Project\nBuild with `make`. Never delete this.\n"; + + let installed = merge(Some(user), BODY); + assert!( + installed.contains("Never delete this."), + "install destroyed the user's content: {installed}" + ); + assert!(installed.contains(BODY)); + + let uninstalled = strip(&installed).expect("a user-authored file is never removed"); + assert!(uninstalled.contains("Never delete this.")); + assert!( + !uninstalled.contains("Guidance."), + "uninstall left our block behind: {uninstalled}" + ); + assert!(!uninstalled.contains(START)); + } + + #[test] + fn reinstalling_replaces_the_block_instead_of_stacking_copies() { + let once = merge(None, BODY); + let twice = merge(Some(&once), "# rustmotion\nNewer guidance."); + assert_eq!(twice.matches(START).count(), 1, "block duplicated: {twice}"); + assert!(twice.contains("Newer guidance.")); + assert!(!twice.contains("Guidance.")); + } + + #[test] + fn a_file_we_alone_created_is_reported_as_removable() { + let ours = merge(None, BODY); + assert!(strip(&ours).is_none()); + } + + #[test] + fn a_file_without_our_block_is_returned_untouched() { + let user = "# Theirs\nnothing of ours here\n"; + assert_eq!(strip(user).as_deref(), Some(user)); + } + + #[test] + fn content_around_the_block_is_preserved_on_both_sides() { + let doc = format!("before\n\n{START}\nold\n{END}\n\nafter\n"); + let merged = merge(Some(&doc), BODY); + assert!(merged.starts_with("before")); + assert!(merged.trim_end().ends_with("after")); + assert!(merged.contains("Guidance.")); + assert!(!merged.contains("\nold\n")); + } +} diff --git a/crates/rustmotion-cli/src/commands/validate.rs b/crates/rustmotion-cli/src/commands/validate.rs index a5f65f6..a0aff12 100644 --- a/crates/rustmotion-cli/src/commands/validate.rs +++ b/crates/rustmotion-cli/src/commands/validate.rs @@ -27,6 +27,67 @@ fn announced_duration(scenario: &ResolvedScenario) -> f64 { rustmotion::encode::build_frame_tasks(scenario).len() as f64 / fps } +/// Why `--fix` must not write over this input. +/// +/// `--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 and writing back is faithful. For +/// anything templated they do not, and the write silently replaces the source +/// with its own expansion: the `config` block and every `$var` disappear, includes +/// get inlined into the parent, and an HTML input is replaced by JSON outright. +/// +/// One rule covers all three: only write back a source `--fix` can reproduce. +#[derive(Debug, PartialEq, Eq)] +enum FixRefusal { + HtmlSource, + Templated, + UsesInclude, +} + +impl FixRefusal { + fn explain(&self, path: &Path) -> String { + let p = path.display(); + match self { + Self::HtmlSource => format!( + "--fix cannot rewrite {p}: it is an HTML source, and the fixer only knows how to \ + emit JSON — applying it would replace your markup with the transpiled scenario. \ + Apply the fix to the HTML by hand, or transpile first and fix the JSON." + ), + Self::Templated => format!( + "--fix cannot rewrite {p}: it declares `config` or uses `$variables`, and the \ + fixer would write back the substituted scenario — dropping the template and \ + making `--var` a silent no-op. Fix the template by hand." + ), + Self::UsesInclude => format!( + "--fix cannot rewrite {p}: it uses `include`, and the fixer would write back the \ + resolved tree — inlining the included files into the parent and patching by a \ + path that no longer means the same node. Fix the included file directly." + ), + } + } +} + +/// `None` when `--fix` may write over `input`. +fn refuse_fix(input: &Path, raw_source: &str) -> Option { + if rustmotion::loader::is_html_path(input) { + return Some(FixRefusal::HtmlSource); + } + // Inspect the bytes on disk, not the loaded tree: by then substitution has + // already erased the very markers that make the write unfaithful. + let source: serde_json::Value = match serde_json::from_str(raw_source) { + Ok(v) => v, + // Unparseable source is not something we should be overwriting either. + Err(_) => return Some(FixRefusal::Templated), + }; + if source.get("config").is_some() || raw_source.contains("$") { + return Some(FixRefusal::Templated); + } + if raw_source.contains("\"include\"") { + return Some(FixRefusal::UsesInclude); + } + None +} + pub fn cmd_validate( input: &PathBuf, report: Option<&Path>, @@ -57,6 +118,10 @@ pub fn cmd_validate( let mut applied_fixes = 0usize; if fix && !report_out.geom_violations.is_empty() { + let raw_source = std::fs::read_to_string(input).unwrap_or_default(); + if let Some(refusal) = refuse_fix(input, &raw_source) { + return Err(RustmotionError::Generic(refusal.explain(input))); + } let mut json_value = loaded.raw.clone(); applied_fixes = apply_fixes(&mut json_value, &report_out.geom_violations); if applied_fixes > 0 { @@ -431,4 +496,74 @@ mod tests { "expected 3.0s with no transitions, got {duration}" ); } + + /// `--fix` writes back the *resolved* tree. Anything the resolution erased is + /// erased on disk too, so these three inputs must be refused rather than + /// silently rewritten. + mod fix_refusals { + use super::super::{refuse_fix, FixRefusal}; + use std::path::Path; + + const PLAIN: &str = r#"{"video":{"width":320,"height":240,"fps":30}, + "scenes":[{"duration":1.0,"children":[]}]}"#; + + #[test] + fn a_plain_json_scenario_is_writable() { + assert_eq!(refuse_fix(Path::new("s.json"), PLAIN), None); + } + + #[test] + fn an_html_source_is_refused() { + // Writing here replaces the author's markup with transpiled JSON. + assert_eq!( + refuse_fix(Path::new("s.html"), ""), + Some(FixRefusal::HtmlSource) + ); + } + + #[test] + fn a_templated_scenario_is_refused() { + // The write would bake in the substitution and make --var a no-op. + let with_config = r#"{"config":{"title":"hi"},"video":{"width":320,"height":240, + "fps":30},"scenes":[{"duration":1.0,"children":[]}]}"#; + assert_eq!( + refuse_fix(Path::new("s.json"), with_config), + Some(FixRefusal::Templated) + ); + + let with_var = r#"{"video":{"width":320,"height":240,"fps":30}, + "scenes":[{"duration":1.0,"children":[ + {"type":"text","content":"$title"}]}]}"#; + assert_eq!( + refuse_fix(Path::new("s.json"), with_var), + Some(FixRefusal::Templated) + ); + } + + #[test] + fn a_scenario_using_include_is_refused() { + // The resolved tree inlines the include, so a path-based patch lands on + // a node the source file does not contain. + let with_include = r#"{"video":{"width":320,"height":240,"fps":30}, + "scenes":[{"include":"part.json"}]}"#; + assert_eq!( + refuse_fix(Path::new("s.json"), with_include), + Some(FixRefusal::UsesInclude) + ); + } + + #[test] + fn every_refusal_names_the_file_and_says_what_to_do_instead() { + let p = Path::new("scenes/hero.json"); + for r in [ + FixRefusal::HtmlSource, + FixRefusal::Templated, + FixRefusal::UsesInclude, + ] { + let msg = r.explain(p); + assert!(msg.contains("scenes/hero.json"), "{msg}"); + assert!(msg.contains("by hand") || msg.contains("directly"), "{msg}"); + } + } + } } diff --git a/crates/rustmotion-cli/src/lib.rs b/crates/rustmotion-cli/src/lib.rs index 013f489..be15b12 100644 --- a/crates/rustmotion-cli/src/lib.rs +++ b/crates/rustmotion-cli/src/lib.rs @@ -1,3 +1,4 @@ +mod claude_md; mod commands; mod skills; pub mod tui; diff --git a/crates/rustmotion-cli/src/skills.rs b/crates/rustmotion-cli/src/skills.rs index 33c1306..1267d67 100644 --- a/crates/rustmotion-cli/src/skills.rs +++ b/crates/rustmotion-cli/src/skills.rs @@ -192,10 +192,13 @@ pub fn install(global: bool) -> Result<()> { } } - // Write CLAUDE.md only in local mode + // Write CLAUDE.md only in local mode. The project may already own this file — + // merge into a delimited block rather than claiming the whole document. if !global { let claude_path = root.join("CLAUDE.md"); - if write_if_changed(&claude_path, CLAUDE_MD)? { + let existing = std::fs::read_to_string(&claude_path).ok(); + let merged = crate::claude_md::merge(existing.as_deref(), CLAUDE_MD); + if write_if_changed(&claude_path, &merged)? { written += 1; } else { skipped += 1; @@ -299,12 +302,24 @@ pub fn uninstall(global: bool) -> Result<()> { std::fs::remove_dir_all(&skills_dir)?; let mut removed = 1; - // Remove CLAUDE.md only in local mode + // Remove only what we put there. A CLAUDE.md carrying the project's own + // instructions keeps them; the file is deleted only when our block was all it + // ever held. if !global { let claude_path = root.join("CLAUDE.md"); - if claude_path.exists() { - std::fs::remove_file(&claude_path)?; - removed += 1; + if let Ok(existing) = std::fs::read_to_string(&claude_path) { + match crate::claude_md::strip(&existing) { + Some(remaining) => { + if remaining != existing { + std::fs::write(&claude_path, remaining)?; + removed += 1; + } + } + None => { + std::fs::remove_file(&claude_path)?; + removed += 1; + } + } } }