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