fix(cli): stop the helper commands destroying files they do not own

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 = "<!-- rustmotion:start -->";
+pub const END: &str = "<!-- rustmotion:end -->";
+
+/// 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<String> {
+    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"));
+    }
+}

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<FixRefusal> {
+    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"), "<rustmotion></rustmotion>"),
+                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}");
+            }
+        }
+    }
 }

crates/rustmotion-cli/src/lib.rs

@@ -1,3 +1,4 @@
+mod claude_md;
 mod commands;
 mod skills;
 pub mod tui;

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;
+                }
+            }
         }
     }