From d2930b2554b3ae541efcede7399cbee811703804 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Sun, 18 Oct 2026 01:21:48 +0200 Subject: [PATCH 1/6] Fix #1743 (follow-up): commands that write to a given path never replace footage, a proxy or the project file Session::write_output (and refuse_export) is the one place that checks; Save Frame As (PNG/PSD/EXR), Export Lottie, Save a Copy (+XML), Collect Files, Export Timeline (and its pre-rendered movies), Export Template, Save Animation Preset, Export Guides and Export Shortcuts write through it. The control channel's render.frame/ui.screenshot, MCP render_frame and CLI render-frame --out check first (new query file.checkOutput for the bridged cases). Save/Save As/auto-save are untouched. Co-Authored-By: Claude Sonnet 5.5 --- apps/effectcraft-cli/src/main.rs | 2 + apps/effectcraft-cli/tests/it/cli.rs | 29 ++ crates/automation/src/backend.rs | 13 + crates/automation/src/tests.rs | 105 +++++++ crates/automation/src/tools.rs | 11 +- crates/engine/src/cmd/animation.rs | 3 +- crates/engine/src/cmd/app_more.rs | 3 +- crates/engine/src/cmd/comp_more.rs | 3 +- crates/engine/src/cmd/essential.rs | 3 +- crates/engine/src/cmd/file_more.rs | 115 +++++--- crates/engine/src/cmd/frame_export.rs | 5 +- crates/engine/src/cmd/lottie.rs | 3 +- crates/engine/src/cmd/settings.rs | 2 +- crates/engine/src/cmd/timeline.rs | 11 + crates/engine/src/cmd/view.rs | 3 +- crates/engine/src/output_guard.rs | 91 +++++- crates/engine/src/tests.rs | 1 + crates/engine/src/tests/export_overwrite.rs | 271 ++++++++++++++++++ crates/engine/src/tests/render_overwrite.rs | 33 ++- crates/ui-egui/src/control.rs | 12 + crates/ui-egui/tests/it/main.rs | 1 + .../ui-egui/tests/it/ui_export_overwrite.rs | 107 +++++++ docs/agents.md | 12 + 23 files changed, 767 insertions(+), 72 deletions(-) create mode 100644 crates/engine/src/tests/export_overwrite.rs create mode 100644 crates/ui-egui/tests/it/ui_export_overwrite.rs diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index fd77162ed..2befb5c4b 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -726,6 +726,8 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { } } let out = args.opt("--out").unwrap_or("frame.png").to_string(); + // Never over footage, a proxy or the project file (checked before rendering). + b.check_output(&out)?; let t0 = std::time::Instant::now(); let f = b.render_with(comp.as_ref(), time, max_side, args.flag("--transparent"))?; let ms = t0.elapsed().as_secs_f64() * 1000.0; diff --git a/apps/effectcraft-cli/tests/it/cli.rs b/apps/effectcraft-cli/tests/it/cli.rs index 7a770fa87..12a9bf4a3 100644 --- a/apps/effectcraft-cli/tests/it/cli.rs +++ b/apps/effectcraft-cli/tests/it/cli.rs @@ -419,6 +419,35 @@ fn render_out_is_relative_to_the_working_directory() { let _ = std::fs::remove_dir_all(&root); } +/// #1743: `render-frame --out` writes a PNG to the path it is given, and never over footage or +/// the project file. +#[test] +fn render_frame_never_writes_over_footage_or_the_project() { + let root = tmp("render-frame-overwrite"); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let (clip, project) = (root.join("clip.png"), root.join("p.ecproj")); + let (c, p) = (clip.to_str().unwrap(), project.to_str().unwrap()); + // A real PNG to import: a frame of the demo project. + ok_json(&["render-frame", "--max-side", "64", "--out", c]); + let imported = json!({"paths": [c]}).to_string(); + ok_json(&["run", "file.import", &imported, "comp.new", r#"{"name":"T","width":64,"height":36,"duration":1}"#, "--empty", "--save-as", p]); + let (footage, saved) = (std::fs::read(&clip).unwrap(), std::fs::read(&project).unwrap()); + for (out, what) in [(c, "footage"), (p, "project")] { + let (code, v) = run_json(&["render-frame", "--project", p, "--out", out]); + assert_eq!(code, 1, "{what}: {v}"); + let error = v["error"].as_str().unwrap(); + assert!(error.contains(out) && error.contains(what), "{what}: {v}"); + } + assert_eq!(std::fs::read(&clip).unwrap(), footage, "the footage is as it was"); + assert_eq!(std::fs::read(&project).unwrap(), saved, "the project file is as it was"); + // Anywhere else it writes. + let free = root.join("frame.png"); + ok_json(&["render-frame", "--project", p, "--out", free.to_str().unwrap()]); + assert_eq!(&std::fs::read(&free).unwrap()[..8], b"\x89PNG\r\n\x1a\n"); + let _ = std::fs::remove_dir_all(&root); +} + /// A real stdio client: each edit's reply is read before disconnecting or killing the server. struct McpChild { child: std::process::Child, diff --git a/crates/automation/src/backend.rs b/crates/automation/src/backend.rs index 5bba7172a..c0dca6c9a 100644 --- a/crates/automation/src/backend.rs +++ b/crates/automation/src/backend.rs @@ -85,6 +85,19 @@ impl Backend { } } + /// Refuse `path`, a file this caller is about to write itself (a frame, a screenshot), when + /// the open project reads it: footage, a proxy or the project file. The project is the + /// session's or the app's, so both are asked through `file.checkOutput`; a relative path is + /// made absolute here, against this process's folder, because the app has its own. + pub fn check_output(&mut self, path: &str) -> Result<()> { + let absolute = std::path::absolute(path).map_err(|e| Error::BadArgs(format!("cannot resolve {path}: {e}")))?; + let found = self.exec("file.checkOutput", json!({"path": absolute.to_string_lossy()}))?; + match found.get("blocked").and_then(Value::as_bool) { + Some(false) => Ok(()), + _ => Err(Error::BadArgs(found.get("reason").and_then(Value::as_str).unwrap_or("this file is one the project reads").to_string())), + } + } + /// Call a raw control-channel method (bridge only). pub fn control(&mut self, method: &str, params: Value) -> Result { match &mut self.target { diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index ff7eedef8..5e846dd3a 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -1615,3 +1615,108 @@ fn blender_export_requires_an_absolute_path_in_a_write_root() { } std::fs::remove_dir_all(&base).unwrap(); } + +/// `render_frame` writes the frame to the `path` it is given, and never over a file the open +/// project reads (#1743): footage, a proxy or the project file. The refusal comes before the +/// frame is rendered, and the file is untouched. +#[test] +fn render_frame_never_writes_over_a_file_the_project_reads() { + use effectcraft_engine::{ + color::Label, + project::{Footage, FootageKind, ItemKind}, + }; + use std::sync::Arc; + let dir = std::env::temp_dir().join(format!("effectcraft-mcp-overwrite-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (clip, project) = (dir.join("clip.png"), dir.join("Project.ecproj")); + std::fs::write(&clip, b"SOURCE FOOTAGE").unwrap(); + let mut session = Session::default(); + session.execute("comp.new", json!({"width": 16, "height": 16})).unwrap(); + session.execute("file.saveAs", json!({"path": project.to_string_lossy()})).unwrap(); + Arc::make_mut(&mut session.project).add_item( + "clip.png", + Label::Aqua, + None, + ItemKind::Footage(Footage { + path: clip.to_string_lossy().into_owned(), + kind: FootageKind::Still, + width: 16, + height: 16, + has_video: true, + ..Default::default() + }), + ); + let saved = std::fs::read(&project).unwrap(); + let mut s = McpServer::new(Backend::headless(session)); + for (file, what) in [(&clip, "footage"), (&project, "project")] { + let (c, err) = call(&mut s, "render_frame", json!({"path": file.to_string_lossy(), "inline": false})); + let text = c[0]["text"].as_str().unwrap(); + assert!(err && text.contains(&*file.to_string_lossy()) && text.contains(what), "{what}: {text}"); + } + assert_eq!(std::fs::read(&clip).unwrap(), b"SOURCE FOOTAGE", "the footage is as it was"); + assert_eq!(std::fs::read(&project).unwrap(), saved, "the project file is as it was"); + // Anywhere else it writes the PNG. + let free = dir.join("frame.png"); + let (c, err) = call(&mut s, "render_frame", json!({"path": free.to_string_lossy(), "inline": false})); + assert!(!err, "{c:?}"); + assert_eq!(&std::fs::read(&free).unwrap()[..8], b"\x89PNG\r\n\x1a\n"); + let _ = std::fs::remove_dir_all(&dir); +} + +/// The same through the desktop app: the app knows the project's files, so the tool asks it +/// (`file.checkOutput`) before it writes anything. +#[test] +fn bridged_render_frame_asks_the_app_before_it_writes() { + use effectcraft_engine::project::{Footage, FootageKind}; + struct Probe; + impl effectcraft_engine::Importer for Probe { + fn probe(&self, path: &str) -> Result { + Ok(Footage { path: path.to_string(), kind: FootageKind::Still, width: 4, height: 4, has_video: true, ..Default::default() }) + } + } + let dir = std::env::temp_dir().join(format!("effectcraft-mcp-bridge-overwrite-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let clip = dir.join("clip.png"); + std::fs::write(&clip, b"SOURCE FOOTAGE").unwrap(); + + let l = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = l.local_addr().unwrap().port(); + std::thread::spawn(move || { + let (stream, _) = l.accept().unwrap(); + let mut out = stream.try_clone().unwrap(); + let mut session = Session { importer: Some(std::sync::Arc::new(Probe)), ..Default::default() }; + for line in BufReader::new(stream).lines() { + let msg: Value = serde_json::from_str(&line.unwrap()).unwrap(); + let p = &msg["params"]; + let mut reply = match msg["method"].as_str().unwrap() { + "engine.execute" => match session.execute(p["command"].as_str().unwrap(), p["params"].clone()) { + Ok(v) => json!({"ok": true, "result": v}), + Err(e) => json!({"ok": false, "error": e.to_string()}), + }, + "render.frame" => { + let png = crate::encode_png(4, 2, vec![255; 32], 0).unwrap(); + json!({"ok": true, "result": {"comp": 1, "time": 0.5, "width": 4, "height": 2, "png": base64::encode(&png)}}) + } + m => json!({"ok": false, "error": format!("unknown method `{m}`")}), + }; + reply["id"] = msg["id"].clone(); + writeln!(out, "{reply}").unwrap(); + } + }); + let mut s = McpServer::new(Backend::bridge(&port.to_string()).unwrap()); + call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"width": 16, "height": 16}})); + call_json(&mut s, "execute_command", json!({"command": "file.import", "params": {"paths": [clip.to_string_lossy()]}})); + + let (c, err) = call(&mut s, "render_frame", json!({"path": clip.to_string_lossy(), "inline": false})); + let text = c[0]["text"].as_str().unwrap(); + assert!(err && text.contains("clip.png") && text.contains("footage"), "{text}"); + assert_eq!(std::fs::read(&clip).unwrap(), b"SOURCE FOOTAGE", "the footage is as it was"); + // Anywhere else it writes the PNG. + let free = dir.join("frame.png"); + let (c, err) = call(&mut s, "render_frame", json!({"path": free.to_string_lossy(), "inline": false})); + assert!(!err, "{c:?}"); + assert_eq!(&std::fs::read(&free).unwrap()[..8], b"\x89PNG\r\n\x1a\n"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index df7eb133b..aeffa464c 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -406,12 +406,17 @@ fn add_keyframe(b: &mut Backend, a: &Value) -> Result { fn render_frame(b: &mut Backend, a: &Value) -> Result { let max_side = get(a, "max_side").and_then(Value::as_u64).unwrap_or(960) as u32; let transparent = get(a, "transparent").and_then(Value::as_bool).unwrap_or(false); - let f = b.render_with(get(a, "comp"), get(a, "time").and_then(Value::as_f64), max_side, transparent)?; - let mut info = json!({"comp": f.comp, "time": f.time, "width": f.width, "height": f.height}); - if let Some(p) = get(a, "path").and_then(Value::as_str) { + let path = get(a, "path").and_then(Value::as_str); + if let Some(p) = path { if let Some(c) = b.confinement() { c.check(p, crate::confine::Access::Write)?; } + // Before the frame is rendered: never over footage, a proxy or the project file. + b.check_output(p)?; + } + let f = b.render_with(get(a, "comp"), get(a, "time").and_then(Value::as_f64), max_side, transparent)?; + let mut info = json!({"comp": f.comp, "time": f.time, "width": f.width, "height": f.height}); + if let Some(p) = path { std::fs::write(p, &f.png).map_err(|e| Error::Other(format!("cannot write {p}: {e}")))?; info["path"] = json!(p); } diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs index 4808d7538..2c2f23f1a 100644 --- a/crates/engine/src/cmd/animation.rs +++ b/crates/engine/src/cmd/animation.rs @@ -67,6 +67,7 @@ fn earliest_key<'a>(groups: impl IntoIterator) -> Option Result { let path = str_p(p, "path").ok_or_else(|| bad("anim.savePreset", "missing `path`"))?.to_string(); + s.refuse_export(&path)?; let comp = s.active_comp().ok_or(EngineError::NoComp)?; let t = s.time(); let mut effects: Vec<(LayerId, PropGroup)> = selected_effects(s); @@ -106,7 +107,7 @@ fn save_preset(s: &mut Session, p: &Value) -> Result { .unwrap_or_else(|| std::path::Path::new(&path).file_stem().map(|s| s.to_string_lossy().to_string()).unwrap_or_default()); let doc = json!({PRESET_TAG: PRESET_VERSION, "name": name, "properties": props_json, "effects": fx_json}); let text = serde_json::to_string_pretty(&doc).unwrap_or_default(); - s.services.write_file(&path, text.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, text.as_bytes())?; Ok(json!({"path": path, "properties": props.len(), "effects": effects.len()})) } diff --git a/crates/engine/src/cmd/app_more.rs b/crates/engine/src/cmd/app_more.rs index 29597a3bc..ea4230852 100644 --- a/crates/engine/src/cmd/app_more.rs +++ b/crates/engine/src/cmd/app_more.rs @@ -152,8 +152,9 @@ fn clear_recent_presets(s: &mut Session, _: &Value) -> Result { fn save_copy_xml(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("file.saveCopyAsXml", "missing `path` (.ecprojx)"))?; + s.refuse_export(path)?; let xml = crate::xml_project::to_xml(&s.project.to_file_json_with_view(&s.saved_view(), false)?).map_err(EngineError::Other)?; - s.services.write_file(path, xml.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, xml.as_bytes())?; s.toast(format!("Saved an XML copy to {path}")); Ok(json!({"path": path, "bytes": xml.len()})) } diff --git a/crates/engine/src/cmd/comp_more.rs b/crates/engine/src/cmd/comp_more.rs index 449444ed1..8b8340e78 100644 --- a/crates/engine/src/cmd/comp_more.rs +++ b/crates/engine/src/cmd/comp_more.rs @@ -275,6 +275,7 @@ fn save_frame(s: &mut Session, p: &Value) -> Result { return queue_frame(s, p); } let path = str_p(p, "path").ok_or_else(|| bad("comp.saveFrameAs", "missing `path` (.png)"))?.to_string(); + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?; let t = f_p(p, "time").map(Tick::from_seconds_f64).unwrap_or(s.time()); @@ -283,7 +284,7 @@ fn save_frame(s: &mut Session, p: &Value) -> Result { let img = s.render(cid, t, effectcraft_render::RenderOpts { scale, backend: effectcraft_render::Backend::Auto, ..Default::default() }); let rgba = if b_p(p, "transparent") == Some(false) { img.to_rgba8_over(bg) } else { img.to_rgba8() }; let png = encode_png(&rgba, img.width, img.height).map_err(|e| EngineError::Other(format!("PNG encoding failed: {e}")))?; - s.services.write_file(&path, &png).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &png)?; s.toast(format!("Saved frame to {path}")); Ok(json!({"path": path, "width": img.width, "height": img.height, "time": t.seconds()})) } diff --git a/crates/engine/src/cmd/essential.rs b/crates/engine/src/cmd/essential.rs index cb6702660..419bd7316 100644 --- a/crates/engine/src/cmd/essential.rs +++ b/crates/engine/src/cmd/essential.rs @@ -807,6 +807,7 @@ fn export_template(s: &mut Session, p: &Value) -> Result { const C: &str = "essential.exportTemplate"; let cid = eg_comp(s, p)?; let path = str_p(p, "path").ok_or_else(|| bad(C, "missing `path` (.ectemplate)"))?.to_string(); + s.refuse_export(&path)?; let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?.clone(); let eg = comp.essential.clone().filter(|e| !e.controls.is_empty()).ok_or_else(|| bad(C, "the composition has no Essential Graphics controls"))?; let comp_name = s.project.item(cid).map(|i| i.name.clone()).unwrap_or_default(); @@ -868,7 +869,7 @@ fn export_template(s: &mut Session, p: &Value) -> Result { entries.insert(0, ("project.ecproj".into(), sub.to_json().into_bytes())); entries.insert(0, ("manifest.json".into(), serde_json::to_vec_pretty(&manifest).unwrap_or_default())); let bytes = effectcraft_lottie::zip::store(&entries); - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; s.toast(format!("Exported template “{name}” to {path}")); Ok(json!({"path": path, "name": name, "bytes": bytes.len(), "controls": eg.flat().len(), "items": deps.len()})) } diff --git a/crates/engine/src/cmd/file_more.rs b/crates/engine/src/cmd/file_more.rs index c5018ae7c..168529c47 100644 --- a/crates/engine/src/cmd/file_more.rs +++ b/crates/engine/src/cmd/file_more.rs @@ -11,7 +11,7 @@ use serde_json::{Value, json}; use super::{CommandSpec, always, b_p, bad, f_p, has_comp, has_project_selection, str_p}; use crate::sequence::SequenceOptions; -use crate::{EngineError, Result, Session, cmd}; +use crate::{EngineError, Result, Session, cmd, query}; fn has_footage_selection(s: &Session) -> std::result::Result<(), String> { if selected_footage(s).is_empty() { Err("select footage in the Project panel".into()) } else { Ok(()) } @@ -78,11 +78,19 @@ fn close_project(s: &mut Session, _: &Value) -> Result { fn save_copy(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("file.saveCopy", "missing `path`"))?; + s.refuse_export(path)?; let json = super::file::file_text(s, path)?; - s.services.write_file(path, json.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, json.as_bytes())?; Ok(json!({"path": path, "bytes": json.len()})) } +/// `file.checkOutput {path}`: the commands that write to a path never replace footage, a proxy +/// or the project file; this says beforehand, for callers that write the file themselves. +fn check_output(s: &mut Session, p: &Value) -> Result { + let path = str_p(p, "path").ok_or_else(|| bad("file.checkOutput", "missing `path`"))?; + Ok(s.output_status(path)) +} + fn rate_of(p: &Value) -> FrameRate { f_p(p, "frameRate").map(|f| limits::frame_rate(FrameRate::from_f64(f), FrameRate::FPS_29_97)).unwrap_or(FrameRate::FPS_29_97) } @@ -376,41 +384,6 @@ fn find_missing(s: &mut Session, p: &Value) -> Result { fn collect_files(s: &mut Session, p: &Value) -> Result { let folder = str_p(p, "folder").ok_or_else(|| bad("file.collectFiles", "missing `folder`"))?.trim_end_matches('/').to_string(); - // Collect Files makes its folder (and the footage folder inside it). - #[cfg(not(target_arch = "wasm32"))] - std::fs::create_dir_all(format!("{folder}/(Footage)")).map_err(|e| EngineError::Other(format!("cannot create {folder}: {e}")))?; - let mut proj = (*s.project).clone(); - let mut copied = 0; - let mut errors = vec![]; - let mut used_names = BTreeSet::new(); - for it in proj.items.values_mut() { - let ItemKind::Footage(f) = &mut it.kind else { continue }; - if f.path.is_empty() { - continue; - } - let files: Vec = if f.sequence.is_empty() { vec![f.path.clone()] } else { f.sequence.clone() }; - let mut new_paths = vec![]; - for src in &files { - let mut name = std::path::Path::new(src).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "footage".into()); - let mut k = 2; - while !used_names.insert(name.clone()) { - name = format!("{k}_{name}"); - k += 1; - } - let dst = format!("{folder}/(Footage)/{name}"); - match s.services.read_file(src).and_then(|bytes| s.services.write_file(&dst, &bytes)) { - Ok(()) => copied += 1, - Err(e) => errors.push(format!("{src}: {e}")), - } - new_paths.push(dst); - } - if f.sequence.is_empty() { - f.path = new_paths.remove(0); - } else { - f.path = new_paths.first().cloned().unwrap_or_default(); - f.sequence = new_paths; - } - } let stem = s .path .as_deref() @@ -418,7 +391,67 @@ fn collect_files(s: &mut Session, p: &Value) -> Result { .map(|n| n.to_string_lossy().to_string()) .unwrap_or_else(|| "Untitled Project".into()); let out = format!("{folder}/{stem}.ecproj"); - s.services.write_file(&out, proj.to_json().as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {out}: {e}")))?; + // Where every copy goes, before anything is made: a copy never lands on a file the project + // reads (other footage, a proxy, the project file itself), and then nothing is written. + let protected = s.protected_files(); + let mut used_names = BTreeSet::new(); + let mut plan: Vec<(ItemId, Vec<(String, String)>)> = vec![]; + for it in s.project.items.values() { + let ItemKind::Footage(f) = &it.kind else { continue }; + if f.path.is_empty() { + continue; + } + let files: Vec<&String> = if f.sequence.is_empty() { vec![&f.path] } else { f.sequence.iter().collect() }; + let mut pairs = vec![]; + for src in files { + let mut name = std::path::Path::new(src).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "footage".into()); + let mut k = 2; + while !used_names.insert(name.clone()) { + name = format!("{k}_{name}"); + k += 1; + } + let dst = format!("{folder}/(Footage)/{name}"); + // A file that is already where its copy goes stays (it is not copied onto itself). + if !crate::output_guard::same_file(std::path::Path::new(src), std::path::Path::new(&dst)) + && let Some(source) = protected.find(std::path::Path::new(&dst)) + { + return Err(EngineError::Other(format!("Nothing was collected. {}", source.refusal(&dst, crate::output_guard::Use::Export)))); + } + pairs.push((src.clone(), dst)); + } + plan.push((it.id, pairs)); + } + if let Some(source) = protected.find(std::path::Path::new(&out)) { + return Err(EngineError::Other(format!("Nothing was collected. {}", source.refusal(&out, crate::output_guard::Use::Export)))); + } + // Collect Files makes its folder (and the footage folder inside it). + #[cfg(not(target_arch = "wasm32"))] + std::fs::create_dir_all(format!("{folder}/(Footage)")).map_err(|e| EngineError::Other(format!("cannot create {folder}: {e}")))?; + let mut proj = (*s.project).clone(); + let mut copied = 0; + let mut errors = vec![]; + for (id, pairs) in plan { + let mut new_paths = vec![]; + for (src, dst) in &pairs { + if crate::output_guard::same_file(std::path::Path::new(src), std::path::Path::new(dst)) { + copied += 1; + } else { + match s.services.read_file(src).and_then(|bytes| s.write_output_with(&protected, dst, &bytes).map_err(std::io::Error::other)) { + Ok(()) => copied += 1, + Err(e) => errors.push(format!("{src}: {e}")), + } + } + new_paths.push(dst.clone()); + } + let Some(ItemKind::Footage(f)) = proj.items.get_mut(&id).map(|it| &mut it.kind) else { continue }; + if f.sequence.is_empty() { + f.path = new_paths.pop().unwrap_or_default(); + } else { + f.path = new_paths.first().cloned().unwrap_or_default(); + f.sequence = new_paths; + } + } + s.write_output_with(&protected, &out, proj.to_json().as_bytes())?; s.toast(format!("Collected {copied} file(s) into {folder}")); Ok(json!({"project": out, "copied": copied, "errors": errors})) } @@ -1173,6 +1206,12 @@ pub fn specs() -> Vec { cmd!("file.close", "Close", ["File"], Some("Cmd+W"), "{comp?}", has_comp, close), cmd!("file.closeProject", "Close Project", ["File"], None, "{}", always, close_project), cmd!("file.saveCopy", "Save a Copy...", ["File", "Save As"], None, "{path}", always, save_copy), + query!( + "file.checkOutput", + "Check Output File", + "{path} → {path, blocked, source?: footage|proxy|project, reason?}: whether an export may be written to path (it may not replace a file the project reads)", + check_output + ), cmd!("file.importMultiple", "Multiple Files...", ["File", "Import"], Some("Cmd+Alt+I"), "{paths: [string]}", always, |s, p| s .execute("file.import", p.clone())), cmd!( diff --git a/crates/engine/src/cmd/frame_export.rs b/crates/engine/src/cmd/frame_export.rs index baad9b6ef..d8b49159c 100644 --- a/crates/engine/src/cmd/frame_export.rs +++ b/crates/engine/src/cmd/frame_export.rs @@ -195,6 +195,7 @@ pub fn layered_exr(s: &Session, cid: ItemId, t: Tick, scale: f64) -> Result<(Vec fn frame_args(s: &Session, p: &Value, cmd: &str, ext: &str) -> Result<(String, ItemId, Tick, f64)> { let path = str_p(p, "path").ok_or_else(|| bad(cmd, format!("missing `path` (.{ext})")))?.to_string(); + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let t = f_p(p, "time").map(Tick::from_seconds_f64).unwrap_or(s.time()); let scale = f_p(p, "scale").unwrap_or(1.0).clamp(0.01, 4.0); @@ -204,7 +205,7 @@ fn frame_args(s: &Session, p: &Value, cmd: &str, ext: &str) -> Result<(String, I fn save_psd(s: &mut Session, p: &Value) -> Result { let (path, cid, t, scale) = frame_args(s, p, "comp.saveFrameAsPsd", "psd")?; let (bytes, n, w, h) = layered_psd(s, cid, t, scale)?; - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; s.toast(format!("Saved {n} layers to {path}")); Ok(json!({"path": path, "layers": n, "width": w, "height": h, "time": t.seconds()})) } @@ -212,7 +213,7 @@ fn save_psd(s: &mut Session, p: &Value) -> Result { fn save_exr(s: &mut Session, p: &Value) -> Result { let (path, cid, t, scale) = frame_args(s, p, "comp.saveFrameAsExr", "exr")?; let (bytes, channels, w, h) = layered_exr(s, cid, t, scale)?; - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; s.toast(format!("Saved a multi-layer EXR to {path}")); Ok(json!({"path": path, "channels": channels, "width": w, "height": h, "time": t.seconds()})) } diff --git a/crates/engine/src/cmd/lottie.rs b/crates/engine/src/cmd/lottie.rs index 03a99b994..288d2e56e 100644 --- a/crates/engine/src/cmd/lottie.rs +++ b/crates/engine/src/cmd/lottie.rs @@ -8,6 +8,7 @@ use crate::{EngineError, Result, Session, cmd}; fn export_lottie(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("file.exportLottie", "missing `path`"))?.to_string(); + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let opts = ExportOptions { include_expressions: b_p(p, "includeExpressions").unwrap_or(false), text_as_shapes: b_p(p, "textAsShapes").unwrap_or(false) }; let services = s.services.clone(); @@ -20,7 +21,7 @@ fn export_lottie(s: &mut Session, p: &Value) -> Result { } else { res.to_string_compact().into_bytes() }; - s.services.write_file(&path, &bytes).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(&path, &bytes)?; let n = res.warnings.len(); s.toast(if n == 0 { format!("Exported Lottie to {path}") } else { format!("Exported Lottie to {path} ({n} warning{})", if n == 1 { "" } else { "s" }) }); Ok(json!({"path": path, "bytes": bytes.len(), "warnings": res.warnings})) diff --git a/crates/engine/src/cmd/settings.rs b/crates/engine/src/cmd/settings.rs index 272903c69..3673a9550 100644 --- a/crates/engine/src/cmd/settings.rs +++ b/crates/engine/src/cmd/settings.rs @@ -195,7 +195,7 @@ fn sc_export(s: &mut Session, p: &Value) -> Result { let doc = s.keymaps.export(str_p(p, "preset")).map_err(|e| bad("shortcuts.export", e))?; if let Some(path) = str_p(p, "path") { let text = serde_json::to_string_pretty(&doc).unwrap_or_default(); - s.services.write_file(path, text.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, text.as_bytes())?; return Ok(json!({"path": path, "preset": doc["name"]})); } Ok(doc) diff --git a/crates/engine/src/cmd/timeline.rs b/crates/engine/src/cmd/timeline.rs index ae57a3b88..f607a36dc 100644 --- a/crates/engine/src/cmd/timeline.rs +++ b/crates/engine/src/cmd/timeline.rs @@ -99,6 +99,7 @@ fn isolate(project: &Project, cid: ItemId, lid: LayerId) -> Option { /// What a pre-render needs from the session, so it can run on a background thread. struct PrerenderCx { + protected: crate::output_guard::Protected, exporter: Option>, footage: Arc, expr: Option>, @@ -157,6 +158,7 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { } path = format!("{path}.{}", format.extension()); } + s.refuse_export(&path)?; let cid = comp_id(s, p)?; let prerender_mode = match str_p(p, "prerender") { None => match p.get("prerender").and_then(Value::as_bool) { @@ -178,6 +180,7 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { let project = s.project.clone(); let services = s.services.clone(); let cx = PrerenderCx { + protected: crate::output_guard::Protected::of(s), exporter: s.exporter.clone(), footage: s.footage.clone(), expr: s.expr.clone(), @@ -189,6 +192,9 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { let wait = !b_p(p, "background").unwrap_or(false); s.spawn_task("exportTimeline", format!("Export {}", format.label()), wait, move |ctl| { let (bytes, mut result) = export_timeline_work(&cx, &project, cid, &opts, &dir, &stem, ctl)?; + if let Some(source) = cx.protected.find(std::path::Path::new(&path)) { + return Err(source.refusal(&path, crate::output_guard::Use::Export)); + } services.write_file(&path, &bytes).map_err(|e| format!("cannot write {path}: {e}"))?; result["path"] = json!(path); result["toast"] = json!(match format { @@ -220,6 +226,11 @@ fn export_timeline_work( let layer_name = project.comp(c).and_then(|cc| cc.layer(l)).map(|x| x.name.clone()).unwrap_or_default(); ctl.message(format!("Pre-render {layer_name}")); let file = dir.join(format!("{stem}_{}_{}_{}.mov", safe(&comp_name), safe(&layer_name), l.0)).to_string_lossy().to_string(); + // The movies are outputs too: never over footage, a proxy or the project file. + if let Some(source) = cx.protected.find(std::path::Path::new(&file)) { + errors.push(format!("{comp_name} ▸ {layer_name}: {}", source.refusal(&file, crate::output_guard::Use::Export))); + continue; + } // Progress over all the layers, each one's frames a share of it. let mut progress = |done: u64, total: u64| ctl.progress(i * 1000 + done.saturating_mul(1000) / total.max(1), n * 1000); match prerender(cx, project, c, l, &file, &mut progress) { diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index a36c53933..115da8667 100644 --- a/crates/engine/src/cmd/view.rs +++ b/crates/engine/src/cmd/view.rs @@ -83,10 +83,11 @@ fn clear_guides(s: &mut Session, p: &Value) -> Result { fn export_guides(s: &mut Session, p: &Value) -> Result { let path = str_p(p, "path").ok_or_else(|| bad("view.exportGuides", "missing `path`"))?; + s.refuse_export(path)?; let comp = s.project.comp(comp_id(s, p)?).ok_or(EngineError::NoComp)?; let doc = json!({"effectcraftGuides": 1, "width": comp.width, "height": comp.height, "guides": comp.guides}); let text = serde_json::to_string_pretty(&doc).unwrap_or_default(); - s.services.write_file(path, text.as_bytes()).map_err(|e| EngineError::Other(format!("cannot write {path}: {e}")))?; + s.write_output(path, text.as_bytes())?; Ok(json!({"path": path, "guides": comp.guides.len()})) } diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index 5d4dc289e..40dcc1e56 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -5,6 +5,11 @@ //! exporter. Existing aliases are compared by native file identity, including hard links. //! Missing sources remain protected by their normalized path. Checks happen before dispatch; //! filesystem changes between the check and the write require exporter-side revalidation. +//! +//! The commands that write to a path they are given (Save Frame As, Export Lottie, Save a Copy, +//! Collect Files, Export Timeline…) never replace one of those files either, with no flag: +//! they write through [`Session::write_output`], the one place that checks. Save and Save As +//! write the project's own file and do not. use std::collections::{HashMap, HashSet}; use std::path::{Component, Path, PathBuf}; @@ -12,6 +17,7 @@ use std::sync::Arc; use effectcraft_project::render_queue::{RenderQueueItem, log_path, numbered_template, sequence_frame, sequence_path}; use effectcraft_project::{Footage, ItemKind}; +use serde_json::{Value, json}; use crate::Session; @@ -26,6 +32,15 @@ const PROBE_LIMIT: u64 = 100_000; /// render time. const MAX_NUMBER: u32 = 1_000; +/// What a file is written for, which words a refusal. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Use { + /// A Render Queue output (or a preview render): the fix is Output To. + Render, + /// Anything else written to a path: a frame, an export, a copy. + Export, +} + /// What reads a file the project depends on. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum Source { @@ -38,12 +53,28 @@ pub(crate) enum Source { } impl Source { - /// Why `path`, a file this reads, cannot be a render's output. - pub(crate) fn refusal(&self, path: &str) -> String { + /// `footage`, `proxy` or `project`: the `source` of a blocked output. + pub(crate) fn kind(&self) -> &'static str { match self { - Source::Footage(name) => format!("{path} is the footage “{name}” of this project, and a render never replaces footage; choose another Output To"), - Source::Proxy(name) => format!("{path} is the proxy of “{name}”, and a render never replaces a proxy; choose another Output To"), - Source::Project => format!("{path} is this project's own file, and a render never replaces it; choose another Output To"), + Source::Footage(_) => "footage", + Source::Proxy(_) => "proxy", + Source::Project => "project", + } + } + + /// Why `path`, a file this reads, cannot be written for `used`. + pub(crate) fn refusal(&self, path: &str, used: Use) -> String { + match (used, self) { + (Use::Render, Source::Footage(name)) => { + format!("{path} is the footage “{name}” of this project, and a render never replaces footage; choose another Output To") + } + (Use::Render, Source::Proxy(name)) => format!("{path} is the proxy of “{name}”, and a render never replaces a proxy; choose another Output To"), + (Use::Render, Source::Project) => format!("{path} is this project's own file, and a render never replaces it; choose another Output To"), + (Use::Export, Source::Footage(name)) => { + format!("{path} is the footage “{name}” of this project, and footage is never written over; choose another file") + } + (Use::Export, Source::Proxy(name)) => format!("{path} is the proxy of “{name}”, and a proxy is never written over; choose another file"), + (Use::Export, Source::Project) => format!("{path} is this project's own file, which only Save writes; choose another file"), } } } @@ -178,8 +209,8 @@ fn fold(name: &str) -> String { if IGNORES_CASE { name.to_lowercase() } else { name.to_string() } } -#[cfg(test)] -fn same_file(a: &Path, b: &Path) -> bool { +/// Whether two paths are one file: the same name once resolved, or the same file on disk. +pub(crate) fn same_file(a: &Path, b: &Path) -> bool { if key(a) == key(b) { return true; } @@ -241,6 +272,50 @@ impl Session { Protected::of(self) } + /// What reads the file at `path`, if the project does. + fn blocking(&self, path: &str) -> Option { + if cfg!(target_arch = "wasm32") { + return None; + } + self.protected_files().find(Path::new(path)).cloned() + } + + /// `Err` when `path` is a file the project reads (footage, a proxy, the project file) and so + /// is not for an export to replace. For callers that write the file themselves; call it + /// before the work, not after it. + pub fn refuse_export(&self, path: &str) -> crate::Result<()> { + match self.blocking(path) { + Some(source) => Err(crate::EngineError::Other(source.refusal(path, Use::Export))), + None => Ok(()), + } + } + + /// Write `bytes` to `path` through the host's services, unless the project reads the file + /// there ([`Session::refuse_export`]). Every command that writes to a path it is given + /// writes through this; Save and Save As (the project's own file) do not. + pub fn write_output(&self, path: &str, bytes: &[u8]) -> crate::Result<()> { + self.write_output_with(&self.protected_files(), path, bytes) + } + + /// [`Session::write_output`] against files already found ([`Session::protected_files`]), + /// for a command that writes many. + pub(crate) fn write_output_with(&self, protected: &Protected, path: &str, bytes: &[u8]) -> crate::Result<()> { + if !cfg!(target_arch = "wasm32") + && let Some(source) = protected.find(Path::new(path)) + { + return Err(crate::EngineError::Other(source.refusal(path, Use::Export))); + } + self.services.write_file(path, bytes).map_err(|e| crate::EngineError::Other(format!("cannot write {path}: {e}"))) + } + + /// `file.checkOutput`'s reply for `path`. + pub(crate) fn output_status(&self, path: &str) -> Value { + match self.blocking(path) { + Some(source) => json!({"path": path, "blocked": true, "source": source.kind(), "reason": source.refusal(path, Use::Export)}), + None => json!({"path": path, "blocked": false}), + } + } + /// Reject protected outputs and ranges that cannot be checked completely. Ordinary-file /// replacement consent is left to the exporter, independently of this source guard. pub(crate) fn check_outputs(&self, items: &[Vec<(RenderQueueItem, String)>]) -> Vec { @@ -287,7 +362,7 @@ impl Session { .map(PathBuf::as_path) .chain(movie) .chain(Some(Path::new(&log))) - .find_map(|path| protected.find(path).map(|source| source.refusal(&path.to_string_lossy())))) + .find_map(|path| protected.find(path).map(|source| source.refusal(&path.to_string_lossy(), Use::Render)))) } /// A new item's default output must not be a file the project reads: `[compName]_1`, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index aee5e8550..e6ab33bd6 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -10,6 +10,7 @@ mod disk_cache; mod effects; mod essential; mod essential_more; +mod export_overwrite; mod face; mod fidelity; mod frame_export; diff --git a/crates/engine/src/tests/export_overwrite.rs b/crates/engine/src/tests/export_overwrite.rs new file mode 100644 index 000000000..c27febd2b --- /dev/null +++ b/crates/engine/src/tests/export_overwrite.rs @@ -0,0 +1,271 @@ +//! The commands that write to a path they are given leave a file the project reads alone (#1743, +//! beyond the Render Queue): footage, a proxy, a frame of an imported image sequence and the +//! project file are refused with nothing changed, and any other path is written. Save and Save As +//! write the project's own file and are not among them. + +use std::sync::Arc; + +use effectcraft_project::{Footage, FootageKind, ItemId, Proxy}; +use serde_json::{Value, json}; + +use super::render_overwrite::{Fixture, SOURCE}; + +/// Footage that is a file named `path`. +fn video(path: String) -> Footage { + Footage { path, kind: FootageKind::Video, has_video: true, ..Default::default() } +} + +/// `item` gets the file `file` of the project's folder as its proxy. +fn give_proxy(f: &mut Fixture, item: ItemId, file: &str) { + f.write(file, SOURCE); + let footage = video(f.text(file)); + Arc::make_mut(&mut f.s.project).item_mut(item).unwrap().proxy = Some(Box::new(Proxy { footage, enabled: true })); +} + +/// A project, saved as `Project.ecproj`, that reads `clip.` (footage) with `proxy.` as +/// its proxy and the frames `shot_00000.` to `shot_00004.` of an image sequence, all +/// holding `SOURCE`. Its composition `Main` holds a red solid. +fn reading(name: &str, ext: &str) -> Fixture { + let mut f = Fixture::new(name); + let clip = f.footage(&format!("clip.{ext}")); + give_proxy(&mut f, clip, &format!("proxy.{ext}")); + f.sequence_as("shot", ext, 0..5); + f.comp("Main"); + f.s.execute("layer.newSolid", json!({"name": "Red", "color": "#ff0000"})).unwrap(); + f +} + +/// The names in `dir`, sorted. +fn listing(dir: &std::path::Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + names.sort(); + names +} + +/// A command that writes to a path. +struct Case { + command: &'static str, + /// The extension of a path for it. + ext: &'static str, + /// What it needs beyond a composition holding a solid. + setup: fn(&mut Fixture), + /// Its parameters for `path`. + params: fn(&str) -> Value, +} + +fn nothing(_: &mut Fixture) {} + +/// The first layer of the active composition. +fn first_layer(f: &Fixture) -> u64 { + f.s.active_comp().unwrap().layers[0].id.0 +} + +/// The composition shows one control, for `essential.exportTemplate`. +fn expose_opacity(f: &mut Fixture) { + let layer = first_layer(f); + f.s.execute("essential.addProperty", json!({"layer": layer, "path": "transform/opacity"})).unwrap(); +} + +/// A property is selected, for `anim.savePreset`. +fn select_opacity(f: &mut Fixture) { + let layer = first_layer(f); + f.s.execute("prop.select", json!({"layer": layer, "path": "transform/opacity"})).unwrap(); +} + +/// The composition has a guide, for `view.exportGuides`. +fn add_guide(f: &mut Fixture) { + f.s.execute("view.addGuide", json!({"orientation": "vertical"})).unwrap(); +} + +/// Run `case` on every file the project reads: each is refused, the project's folder is as it was +/// and nothing was rendered; then on a free path, where it writes. +fn never_replaces_what_the_project_reads(case: &Case) { + let ext = case.ext; + let mut f = reading(&format!("{}-{ext}", case.command), ext); + (case.setup)(&mut f); + let mut reads = vec![(format!("clip.{ext}"), "footage"), (format!("proxy.{ext}"), "proxy"), (format!("shot_00002.{ext}"), "footage")]; + // (A timeline export adds its extension to `Project.ecproj`: it has a test of its own.) + if case.command != "file.exportTimeline" { + reads.push(("Project.ecproj".to_string(), "project")); + } + let before: Vec> = reads.iter().map(|(file, _)| f.read(file)).collect(); + let listed = listing(&f.dir); + for ((file, source), bytes) in reads.iter().zip(&before) { + let path = f.text(file); + let e = f.s.execute(case.command, (case.params)(&path)).expect_err(&format!("{} wrote over {file}", case.command)).to_string(); + assert!(e.contains(file.as_str()) && e.contains(source), "{}: {file}: {e}", case.command); + assert_eq!(&f.read(file), bytes, "{}: {file} is as it was", case.command); + } + assert_eq!(listing(&f.dir), listed, "{}: refusing changed nothing in the folder", case.command); + assert!(f.exported().is_empty(), "{}: nothing was rendered for it", case.command); + + let free = format!("out.{ext}"); + f.s.execute(case.command, (case.params)(&f.text(&free))).unwrap_or_else(|e| panic!("{}: {e}", case.command)); + assert_ne!(f.read(&free), SOURCE, "{}: wrote its file", case.command); + for ((file, _), bytes) in reads.iter().zip(&before) { + assert_eq!(&f.read(file), bytes, "{}: {file} is still as it was", case.command); + } +} + +macro_rules! never_replaces { + ($name:ident, $command:literal, $ext:literal, $setup:expr) => { + #[test] + fn $name() { + never_replaces_what_the_project_reads(&Case { command: $command, ext: $ext, setup: $setup, params: |path| json!({"path": path}) }); + } + }; +} + +never_replaces!(save_frame_as_png, "comp.saveFrameAs", "png", nothing); +never_replaces!(save_frame_as_photoshop_layers, "comp.saveFrameAsPsd", "psd", nothing); +never_replaces!(save_frame_as_proexr, "comp.saveFrameAsExr", "exr", nothing); +never_replaces!(export_lottie_json, "file.exportLottie", "json", nothing); +never_replaces!(export_lottie_dotlottie, "file.exportLottie", "lottie", nothing); +never_replaces!(save_a_copy, "file.saveCopy", "ecproj", nothing); +never_replaces!(save_a_copy_as_xml, "file.saveCopyAsXml", "ecprojx", nothing); +never_replaces!(export_timeline, "file.exportTimeline", "xml", nothing); +never_replaces!(export_template, "essential.exportTemplate", "ectemplate", expose_opacity); +never_replaces!(save_animation_preset, "anim.savePreset", "ecpreset", select_opacity); +never_replaces!(export_guides, "view.exportGuides", "json", add_guide); +never_replaces!(export_shortcuts, "shortcuts.export", "json", nothing); + +// ---------------------------------------------------------------- Collect Files + +/// A project reading the footage `in/`, with an empty `out/(Footage)` where Collect Files +/// would put its copy. +fn collecting(name: &str, file: &str) -> Fixture { + let mut f = Fixture::new(name); + std::fs::create_dir_all(f.path("in")).unwrap(); + std::fs::create_dir_all(f.path("out/(Footage)")).unwrap(); + f.footage(&format!("in/{file}")); + f +} + +/// Collect Files into the project folder's `out`. +fn collect(f: &mut Fixture) -> crate::Result { + let folder = f.text("out"); + f.s.execute("file.collectFiles", json!({"folder": folder})) +} + +/// `there`, the place the copy of the footage `in/` would go, is something the project +/// reads (`source`): Collect Files is refused before it copies or writes anything. +fn collect_is_refused(mut f: Fixture, original: &str, there: &str, source: &str) { + f.write(there, b"NOT TO BE REPLACED"); + let listed = listing(&f.path("out/(Footage)")); + let e = collect(&mut f).unwrap_err().to_string(); + assert!(e.contains(original) && e.contains(source), "{e}"); + assert_eq!(f.read(there), b"NOT TO BE REPLACED", "the file the project reads is as it was"); + assert_eq!(f.read(&format!("in/{original}")), SOURCE, "the original is as it was"); + assert!(!f.path("out/Project.ecproj").exists(), "the collected project was not written"); + assert_eq!(listing(&f.path("out/(Footage)")), listed, "nothing else was copied"); +} + +#[test] +fn collect_files_does_not_copy_over_other_footage() { + let mut f = collecting("collect-footage", "a.mov"); + f.footage("out/(Footage)/a.mov"); + collect_is_refused(f, "a.mov", "out/(Footage)/a.mov", "footage"); +} + +#[test] +fn collect_files_does_not_copy_over_a_proxy() { + let mut f = collecting("collect-proxy", "a.mov"); + let a = f.s.project.items.keys().next().copied().unwrap(); + give_proxy(&mut f, a, "out/(Footage)/a.mov"); + collect_is_refused(f, "a.mov", "out/(Footage)/a.mov", "proxy"); +} + +#[test] +fn collect_files_does_not_copy_over_a_sequence_frame() { + let mut f = collecting("collect-frame", "shot_00002.png"); + f.sequence_as("out/(Footage)/shot", "png", 0..5); + collect_is_refused(f, "shot_00002.png", "out/(Footage)/shot_00002.png", "footage"); +} + +/// Collecting into the project's own folder would write the collected copy over the project. +#[test] +fn collect_files_does_not_write_over_the_project_file() { + let mut f = collecting("collect-project", "a.mov"); + let project = f.read("Project.ecproj"); + let folder = f.dir.to_string_lossy().into_owned(); + let e = f.s.execute("file.collectFiles", json!({"folder": folder})).unwrap_err().to_string(); + assert!(e.contains("Project.ecproj") && e.contains("project"), "{e}"); + assert_eq!(f.read("Project.ecproj"), project, "the project file is as it was"); + assert!(!f.path("(Footage)").exists(), "no folder was made, nothing was copied"); +} + +#[test] +fn collect_files_copies_into_a_free_folder() { + let mut f = collecting("collect-free", "a.mov"); + let r = collect(&mut f).unwrap(); + assert_eq!((r["copied"].as_u64(), &r["errors"]), (Some(1), &json!([])), "{r}"); + assert_eq!(f.read("out/(Footage)/a.mov"), SOURCE); + assert_eq!(f.read("in/a.mov"), SOURCE, "the original stays"); + assert!(f.path("out/Project.ecproj").exists()); +} + +/// Footage that is where its copy would go is already collected: it is not copied onto itself. +#[test] +fn collect_files_leaves_footage_that_is_already_in_the_folder() { + let mut f = collecting("collect-in-place", "a.mov"); + f.footage("out/(Footage)/c.mov"); + f.write("out/(Footage)/c.mov", b"ALREADY COLLECTED"); + let r = collect(&mut f).unwrap(); + assert_eq!((r["copied"].as_u64(), &r["errors"]), (Some(2), &json!([])), "{r}"); + assert_eq!(f.read("out/(Footage)/c.mov"), b"ALREADY COLLECTED"); + assert_eq!(f.read("out/(Footage)/a.mov"), SOURCE); +} + +// ---------------------------------------------------------------- the rest + +/// The movies a timeline export renders beside it are written like any other output. +#[test] +fn a_timeline_export_does_not_pre_render_over_footage() { + let mut f = Fixture::new("timeline-prerender"); + f.comp("Main"); + f.s.execute("layer.newSolid", json!({"name": "Red", "color": "#ff0000"})).unwrap(); + // OpenTimelineIO has no solids: the solid is rendered to a movie beside the document. + let params = json!({"path": f.text("Main.otio"), "prerender": "unsupported"}); + let r = f.s.execute("file.exportTimeline", params.clone()).unwrap(); + let movies = f.exported(); + assert_eq!(movies.len(), 1, "{r}"); + assert_eq!(r["prerendered"].as_array().map(Vec::len), Some(1), "{r}"); + // That movie is footage now. + let movie = std::path::Path::new(&movies[0]).file_name().unwrap().to_string_lossy().into_owned(); + f.footage(&movie); + let r = f.s.execute("file.exportTimeline", params).unwrap(); + assert_eq!(f.read(&movie), SOURCE, "the footage is as it was"); + assert_eq!(f.exported().len(), 1, "no second render was started"); + assert_eq!(r["prerendered"], json!([]), "{r}"); + let warnings = r["warnings"].as_array().unwrap().iter().filter_map(Value::as_str).collect::>().join("\n"); + assert!(warnings.contains(&movie) && warnings.contains("footage"), "the export says why: {r}"); +} + +/// `file.checkOutput` answers before anything is written, for callers that write the file +/// themselves (the CLI, MCP's `render_frame`). +#[test] +fn check_output_names_the_files_an_export_may_not_use() { + let mut f = reading("check-output", "png"); + for (file, source) in [("clip.png", "footage"), ("proxy.png", "proxy"), ("shot_00003.png", "footage"), ("Project.ecproj", "project")] { + let r = f.s.execute("file.checkOutput", json!({"path": f.text(file)})).unwrap(); + assert_eq!((r["blocked"].as_bool(), r["source"].as_str()), (Some(true), Some(source)), "{file}: {r}"); + assert!(r["reason"].as_str().unwrap().contains(file), "{file}: {r}"); + assert_eq!(r["path"], json!(f.text(file)), "{r}"); + } + let r = f.s.execute("file.checkOutput", json!({"path": f.text("free.png")})).unwrap(); + assert_eq!((r["blocked"].as_bool(), r["source"].is_null()), (Some(false), true), "{r}"); + assert!(f.s.execute("file.checkOutput", json!({})).is_err(), "a path is needed"); + assert!(!f.path("free.png").exists(), "it only looks"); +} + +/// A project saved under a timeline's name is not overwritten by the timeline. +#[test] +fn a_timeline_export_does_not_write_over_the_project_file() { + let mut f = Fixture::new("timeline-project"); + f.comp("Main"); + f.s.execute("file.saveAs", json!({"path": f.text("Edit.xml")})).unwrap(); + let saved = f.read("Edit.xml"); + let e = f.s.execute("file.exportTimeline", json!({"path": f.text("Edit.xml")})).unwrap_err().to_string(); + assert!(e.contains("Edit.xml") && e.contains("project"), "{e}"); + assert_eq!(f.read("Edit.xml"), saved); +} diff --git a/crates/engine/src/tests/render_overwrite.rs b/crates/engine/src/tests/render_overwrite.rs index 420763da2..93c2ec45c 100644 --- a/crates/engine/src/tests/render_overwrite.rs +++ b/crates/engine/src/tests/render_overwrite.rs @@ -12,7 +12,7 @@ use serde_json::{Value, json}; use crate::{ExportJob, ExportResult, Exporter, Session}; -const SOURCE: &[u8] = b"SOURCE FOOTAGE"; +pub(crate) const SOURCE: &[u8] = b"SOURCE FOOTAGE"; const EARLIER: &[u8] = b"AN EARLIER RENDER"; const RENDERED: &[u8] = b"RENDERED"; @@ -51,14 +51,14 @@ impl Exporter for Writer { /// A session whose project is saved in a fresh folder (the footage goes there too), with an /// exporter that writes real files. -struct Fixture { - s: Session, +pub(crate) struct Fixture { + pub(crate) s: Session, writer: Arc, - dir: PathBuf, + pub(crate) dir: PathBuf, } impl Fixture { - fn new(name: &str) -> Fixture { + pub(crate) fn new(name: &str) -> Fixture { let dir = std::env::temp_dir().join("effectcraft-render-overwrite").join(format!("{}-{name}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); @@ -69,24 +69,24 @@ impl Fixture { Fixture { s, writer, dir } } - fn path(&self, file: &str) -> PathBuf { + pub(crate) fn path(&self, file: &str) -> PathBuf { self.dir.join(file) } - fn text(&self, file: &str) -> String { + pub(crate) fn text(&self, file: &str) -> String { self.path(file).to_string_lossy().into_owned() } - fn read(&self, file: &str) -> Vec { + pub(crate) fn read(&self, file: &str) -> Vec { std::fs::read(self.path(file)).unwrap() } - fn write(&self, file: &str, bytes: &[u8]) { + pub(crate) fn write(&self, file: &str, bytes: &[u8]) { std::fs::write(self.path(file), bytes).unwrap(); } /// A video file in the project's folder, imported. - fn footage(&mut self, file: &str) -> ItemId { + pub(crate) fn footage(&mut self, file: &str) -> ItemId { self.write(file, SOURCE); let f = Footage { path: self.text(file), @@ -102,9 +102,14 @@ impl Fixture { /// Numbered stills in the project's folder, imported as one image sequence. fn sequence(&mut self, name: &str, frames: std::ops::Range) -> ItemId { + self.sequence_as(name, "png", frames) + } + + /// [`Fixture::sequence`] of files with extension `ext`. + pub(crate) fn sequence_as(&mut self, name: &str, ext: &str, frames: std::ops::Range) -> ItemId { let files: Vec = frames .map(|n| { - let file = format!("{name}_{n:05}.png"); + let file = format!("{name}_{n:05}.{ext}"); self.write(&file, SOURCE); self.text(&file) }) @@ -119,11 +124,11 @@ impl Fixture { duration: Tick::from_seconds_f64(0.5), ..Default::default() }; - Arc::make_mut(&mut self.s.project).add_item(&format!("{name}_[00000-00004].png"), Label::None, None, ItemKind::Footage(f)) + Arc::make_mut(&mut self.s.project).add_item(&format!("{name}_[00000-00004].{ext}"), Label::None, None, ItemKind::Footage(f)) } /// A five-frame composition. - fn comp(&mut self, name: &str) { + pub(crate) fn comp(&mut self, name: &str) { self.s.execute("comp.new", json!({"name": name, "width": 16, "height": 16, "frameRate": 10, "duration": 0.5})).unwrap(); } @@ -133,7 +138,7 @@ impl Fixture { } /// The paths the exporter was started for. - fn exported(&self) -> Vec { + pub(crate) fn exported(&self) -> Vec { self.writer.jobs.lock().unwrap().clone() } diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index f67591bee..023bfa8b3 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -619,6 +619,12 @@ fn playback(app: &mut EffectcraftApp, p: &Value, now: f64) -> Outcome { /// when the capture is issued. fn screenshot(app: &mut EffectcraftApp, ctx: &egui::Context, p: &Value) -> Outcome { let s = |k: &str| p.get(k).and_then(Value::as_str); + // A file the project reads is never the output. + if let Some(path) = s("path") + && let Err(e) = app.session.refuse_export(path) + { + return err(e); + } // The panel/element is an identity, not a rectangle: it is resolved when the capture // is issued (after the UI settles), so a panel that moves meanwhile is not cropped // with its old rectangle. @@ -672,6 +678,12 @@ fn quit(app: &mut EffectcraftApp, ctx: &egui::Context, p: &Value) -> Outcome { /// temp file) or inline as base64 when `base64: true`. fn render_frame(app: &EffectcraftApp, p: &Value) -> Outcome { let s = &app.session; + // A file the project reads is never the output (checked before rendering). + if let Some(path) = p.get("path").and_then(Value::as_str) + && let Err(e) = s.refuse_export(path) + { + return err(e); + } let cid = match s.resolve_comp(p.get("comp")) { Ok(c) => c, Err(e) => return err(e), diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs index 180b56232..47758810b 100644 --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -26,6 +26,7 @@ mod ui_effect_controls; mod ui_effect_drop_highlight; mod ui_effect_names; mod ui_env_light_gpu; +mod ui_export_overwrite; mod ui_expression_values; mod ui_font_search; mod ui_form_fit; diff --git a/crates/ui-egui/tests/it/ui_export_overwrite.rs b/crates/ui-egui/tests/it/ui_export_overwrite.rs new file mode 100644 index 000000000..e14d9a7e8 --- /dev/null +++ b/crates/ui-egui/tests/it/ui_export_overwrite.rs @@ -0,0 +1,107 @@ +//! The control channel's `render.frame` and `ui.screenshot` write a PNG to the `path` they are +//! given, and never over a file the project reads (#1743): footage, a proxy or the project file. + +use std::path::PathBuf; + +use effectcraft_engine::Session; +use effectcraft_engine::color::Label; +use effectcraft_engine::project::{Footage, FootageKind, ItemKind}; +use effectcraft_ui_egui::EffectcraftApp; +use effectcraft_ui_egui::control::{self, ControlRequest, Outcome}; +use egui_kittest::Harness; +use serde_json::{Value, json}; + +const SOURCE: &[u8] = b"SOURCE FOOTAGE"; + +struct Rig { + h: Harness<'static, EffectcraftApp>, + dir: PathBuf, +} + +impl Rig { + /// A project saved as `Project.ecproj` in a fresh folder, reading `clip.png` beside it. + fn new(name: &str) -> Rig { + let dir = std::env::temp_dir().join("effectcraft-ui-export-overwrite").join(format!("{}-{name}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write(dir.join("clip.png"), SOURCE).unwrap(); + let mut s = Session::default(); + s.execute("comp.new", json!({"name": "Main", "width": 16, "height": 16, "frameRate": 10, "duration": 0.5})).unwrap(); + s.execute("file.saveAs", json!({"path": dir.join("Project.ecproj").to_string_lossy()})).unwrap(); + let clip = Footage { + path: dir.join("clip.png").to_string_lossy().into_owned(), + kind: FootageKind::Still, + width: 16, + height: 16, + has_video: true, + ..Default::default() + }; + std::sync::Arc::make_mut(&mut s.project).add_item("clip.png", Label::Aqua, None, ItemKind::Footage(clip)); + let mut app = Some(EffectcraftApp::new(s)); + let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| app.take().expect("app")); + h.run_steps(2); + Rig { h, dir } + } + + fn text(&self, file: &str) -> String { + self.dir.join(file).to_string_lossy().into_owned() + } + + fn read(&self, file: &str) -> Vec { + std::fs::read(self.dir.join(file)).unwrap() + } + + /// A control-channel request, answered at once or not. + fn send(&mut self, method: &str, params: Value) -> Outcome { + let ctx = self.h.ctx.clone(); + let (req, _rx) = ControlRequest::new(method, params); + let now = self.h.state().now(); + control::handle(self.h.state_mut(), &ctx, &req, now) + } + + /// The reply of a request that is answered at once. + fn reply(&mut self, method: &str, params: Value) -> Value { + match self.send(method, params) { + Outcome::Done(v) => v, + _ => panic!("{method}: no immediate reply"), + } + } +} + +impl Drop for Rig { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.dir); + } +} + +#[test] +fn render_frame_never_writes_over_a_file_the_project_reads() { + let mut r = Rig::new("render-frame"); + let project = r.read("Project.ecproj"); + for (file, what) in [("clip.png", "footage"), ("Project.ecproj", "project")] { + let v = r.reply("render.frame", json!({"path": r.text(file)})); + let error = v["error"].as_str().unwrap_or_default(); + assert!(v["ok"] == false && error.contains(file) && error.contains(what), "{file}: {v}"); + } + assert_eq!(r.read("clip.png"), SOURCE, "the footage is as it was"); + assert_eq!(r.read("Project.ecproj"), project, "the project file is as it was"); + // Anywhere else it writes the PNG. + let v = r.reply("render.frame", json!({"path": r.text("frame.png")})); + assert_eq!(v["ok"], true, "{v}"); + assert_eq!(&r.read("frame.png")[..8], b"\x89PNG\r\n\x1a\n"); +} + +#[test] +fn screenshot_never_writes_over_a_file_the_project_reads() { + let mut r = Rig::new("screenshot"); + let project = r.read("Project.ecproj"); + for (file, what) in [("clip.png", "footage"), ("Project.ecproj", "project")] { + let Outcome::Done(v) = r.send("ui.screenshot", json!({"path": r.text(file)})) else { panic!("{file}: the screenshot was taken for it") }; + let error = v["error"].as_str().unwrap_or_default(); + assert!(v["ok"] == false && error.contains(file) && error.contains(what), "{file}: {v}"); + } + assert_eq!(r.read("clip.png"), SOURCE, "the footage is as it was"); + assert_eq!(r.read("Project.ecproj"), project, "the project file is as it was"); + // Anywhere else it is taken (the frame that carries it comes later). + assert!(matches!(r.send("ui.screenshot", json!({"path": r.text("shot.png")})), Outcome::Screenshot { .. })); +} diff --git a/docs/agents.md b/docs/agents.md index d801fcec6..c23c20ec5 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -319,6 +319,18 @@ build real projects through these interfaces; they are worked examples of everyt proresProfile: 4444, output}` then `renderQueue.render {"wait": true}`. Check a render by importing it (`file.import`, `file.newCompFromSelection`) and `render_frame {transparent: true}` for its alpha (ProRes 4444 and WebM VP9 alpha both read back as straight alpha). +* **Other files written to a path you give** follow the same footage rule, with no flag and no + prompt: `comp.saveFrameAs` (also `…Psd`, `…Exr`), `file.exportLottie`, `file.saveCopy`, + `file.saveCopyAsXml`, `file.collectFiles` (every copy and the collected project; nothing is + written if one would land on a file the project reads), `file.exportTimeline` (and the movies it + pre-renders), `essential.exportTemplate`, `anim.savePreset`, `view.exportGuides`, + `shortcuts.export`, the control channel's `render.frame` / `ui.screenshot` with a `path`, the + MCP `render_frame` tool and `effectcraft-cli render-frame --out`. Footage (every frame of a + sequence), a proxy and the project file are refused with the reason and left untouched. + `file.checkOutput {path}` says beforehand (`{blocked, source, reason}`). Not covered: Save / + Save As / Increment and Save / auto-save (they write the project's own file), script `File` + writes (a script may rewrite a data file it also imports), and files the app extracts for + itself (template media, vector images). * **Essential Graphics**: controls can be addressed by name (`essential.set {"layer":"#1", "control":"Title","value":"John Smith"}`); a command that targets an explicit `comp` runs even when the active comp would disable it (`essential.exportTemplate {"comp":"Lower Third", …}`). From 7e68d4bbb457574c9132e9d97831a73ed09131e1 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 08:27:14 +0100 Subject: [PATCH 2/6] #1829: cover source protection during background timeline export --- crates/engine/src/tests/export_overwrite.rs | 50 +++++++++++++-------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/crates/engine/src/tests/export_overwrite.rs b/crates/engine/src/tests/export_overwrite.rs index c27febd2b..50f5d2ff8 100644 --- a/crates/engine/src/tests/export_overwrite.rs +++ b/crates/engine/src/tests/export_overwrite.rs @@ -221,24 +221,38 @@ fn collect_files_leaves_footage_that_is_already_in_the_folder() { /// The movies a timeline export renders beside it are written like any other output. #[test] fn a_timeline_export_does_not_pre_render_over_footage() { - let mut f = Fixture::new("timeline-prerender"); - f.comp("Main"); - f.s.execute("layer.newSolid", json!({"name": "Red", "color": "#ff0000"})).unwrap(); - // OpenTimelineIO has no solids: the solid is rendered to a movie beside the document. - let params = json!({"path": f.text("Main.otio"), "prerender": "unsupported"}); - let r = f.s.execute("file.exportTimeline", params.clone()).unwrap(); - let movies = f.exported(); - assert_eq!(movies.len(), 1, "{r}"); - assert_eq!(r["prerendered"].as_array().map(Vec::len), Some(1), "{r}"); - // That movie is footage now. - let movie = std::path::Path::new(&movies[0]).file_name().unwrap().to_string_lossy().into_owned(); - f.footage(&movie); - let r = f.s.execute("file.exportTimeline", params).unwrap(); - assert_eq!(f.read(&movie), SOURCE, "the footage is as it was"); - assert_eq!(f.exported().len(), 1, "no second render was started"); - assert_eq!(r["prerendered"], json!([]), "{r}"); - let warnings = r["warnings"].as_array().unwrap().iter().filter_map(Value::as_str).collect::>().join("\n"); - assert!(warnings.contains(&movie) && warnings.contains("footage"), "the export says why: {r}"); + for background in [false, true] { + let mut f = Fixture::new(&format!("timeline-prerender-{background}")); + f.comp("Main"); + f.s.execute("layer.newSolid", json!({"name": "Red", "color": "#ff0000"})).unwrap(); + // OpenTimelineIO has no solids: the solid is rendered to a movie beside the document. + let params = json!({"path": f.text("Main.otio"), "prerender": "unsupported", "background": background}); + let run = |s: &mut crate::Session, params: Value| { + let result = s.execute("file.exportTimeline", params).unwrap(); + if background { + assert!(result["job"].is_string(), "{result}"); + s.wait_jobs(); + let done = s.job_log.last().unwrap(); + assert_eq!(done.status, "done", "{}", done.message); + done.result.clone() + } else { + result + } + }; + let r = run(&mut f.s, params.clone()); + let movies = f.exported(); + assert_eq!(movies.len(), 1, "{r}"); + assert_eq!(r["prerendered"].as_array().map(Vec::len), Some(1), "{r}"); + // That movie is footage now. + let movie = std::path::Path::new(&movies[0]).file_name().unwrap().to_string_lossy().into_owned(); + f.footage(&movie); + let r = run(&mut f.s, params); + assert_eq!(f.read(&movie), SOURCE, "the footage is as it was"); + assert_eq!(f.exported().len(), 1, "no second render was started"); + assert_eq!(r["prerendered"], json!([]), "{r}"); + let warnings = r["warnings"].as_array().unwrap().iter().filter_map(Value::as_str).collect::>().join("\n"); + assert!(warnings.contains(&movie) && warnings.contains("footage"), "the export says why: {r}"); + } } /// `file.checkOutput` answers before anything is written, for callers that write the file From a0ad0d5bbdeb85e90476e649ef100ae162095d48 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 08:45:51 +0100 Subject: [PATCH 3/6] #1829: recheck queued screenshot paths before writing --- crates/ui-egui/src/app/screenshot.rs | 64 ++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/crates/ui-egui/src/app/screenshot.rs b/crates/ui-egui/src/app/screenshot.rs index a80a76bd6..bf7dace0d 100644 --- a/crates/ui-egui/src/app/screenshot.rs +++ b/crates/ui-egui/src/app/screenshot.rs @@ -251,6 +251,12 @@ impl EffectcraftApp { } let shot = self.pending_screenshots.remove(i); self.shot_bytes = self.shot_bytes.saturating_sub(shot.reserved_bytes); + if let Some(path) = shot.path.as_deref() + && let Err(error) = self.session.refuse_export(path) + { + let _ = shot.reply.send(json!({"ok": false, "error": error.to_string()})); + continue; + } let r = control::save_screenshot(&image, shot.path.as_deref(), shot.crop, [sx, sy]); let _ = shot.reply.send(r); } @@ -328,3 +334,61 @@ impl EffectcraftApp { } } } + +#[cfg(test)] +mod tests { + use super::*; + use effectcraft_engine::Session; + use effectcraft_engine::project::{Footage, FootageKind, ItemKind}; + use std::sync::Arc; + + #[test] + fn queued_screenshots_recheck_sources_when_the_capture_completes() { + let dir = std::env::temp_dir().join(format!("effectcraft-pending-shot-protection-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + for kind in ["footage", "project", "free"] { + let path = dir.join(format!("{kind}.png")); + let text = path.to_string_lossy().into_owned(); + let mut app = EffectcraftApp::new(Session::default()); + let ctx = egui::Context::default(); + let geom = DrawGeometry { rect: egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(2.0, 2.0)), ppp: 1.0, epoch: 0 }; + app.draw_geom = Some(geom); + assert!(app.session.refuse_export(&text).is_ok()); + let (tx, rx) = std::sync::mpsc::channel(); + app.admit_screenshot(&ctx, Some(text.clone()), ShotTarget::Window, tx); + let (slot, callback) = craft_ui::capture::CaptureSlot::new(&ctx, egui::ViewportId::ROOT, MAX_SHOT_BYTES); + let shot = app.pending_screenshots.last_mut().unwrap(); + shot.issued = Some(geom); + shot.slot = Some(slot); + // The destination becomes protected after admission, before pixels arrive. + let original = match kind { + "footage" => { + std::fs::write(&path, b"SOURCE FOOTAGE").unwrap(); + let footage = Footage { path: text.clone(), kind: FootageKind::Still, ..Default::default() }; + Arc::make_mut(&mut app.session.project).add_item("clip", effectcraft_engine::color::Label::None, None, ItemKind::Footage(footage)); + Some(std::fs::read(&path).unwrap()) + } + "project" => { + app.session.execute("file.saveAs", json!({"path": text})).unwrap(); + Some(std::fs::read(&path).unwrap()) + } + _ => None, + }; + callback.complete(Arc::new(egui::ColorImage::filled([2, 2], egui::Color32::RED))); + app.drain_completed_screenshots(geom); + let reply = rx.try_recv().unwrap(); + if let Some(original) = original { + assert_eq!(reply["ok"], false, "{kind}: {reply}"); + assert!(reply["error"].as_str().unwrap_or_default().contains(kind), "{reply}"); + assert_eq!(std::fs::read(&path).unwrap(), original); + } else { + assert_eq!(reply["ok"], true, "{reply}"); + assert!(std::fs::read(&path).unwrap().starts_with(b"\x89PNG\r\n\x1a\n")); + } + assert!(app.pending_screenshots.is_empty()); + assert_eq!(app.shot_bytes, 0); + std::fs::remove_file(path).unwrap(); + } + std::fs::remove_dir(dir).unwrap(); + } +} From 9c8f4f639dbbb28c9b9a76664d0cdd2f8aea23c9 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Fri, 30 Oct 2026 15:28:49 +0700 Subject: [PATCH 4/6] #1743: confine file.checkOutput's path as a read Co-Authored-By: Claude Opus 5.5 --- crates/automation/src/confine.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/automation/src/confine.rs b/crates/automation/src/confine.rs index 0e9d79d4b..d3c27dfb8 100644 --- a/crates/automation/src/confine.rs +++ b/crates/automation/src/confine.rs @@ -88,6 +88,8 @@ const READS: &[(&str, &[&str])] = &[ ("mediaBrowser.removeFavorite", &["path"]), ("face.model.install", &["path"]), ("roto.model.install", &["path"]), + // Looks up what the file at `path` is (writable roots are readable too). + ("file.checkOutput", &["path"]), // Reads the template at `path`; what it writes is in `MIXED_WRITES`. ("templates.create", &["path"]), ]; From 41148e9b72ce3fe0c8c640d4ba47344d34c8a372 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 09:41:38 +0100 Subject: [PATCH 5/6] #1743: validate background and bridged exports in their owning session --- Cargo.lock | 1 + Cargo.toml | 1 + apps/effectcraft-cli/src/main.rs | 5 +- crates/automation/src/backend.rs | 36 +++++++- crates/automation/src/confine.rs | 10 ++- crates/automation/src/tests.rs | 26 ++++-- crates/automation/src/tools.rs | 15 ++-- crates/engine/Cargo.toml | 1 + crates/engine/src/cmd/timeline.rs | 28 ++++-- crates/engine/src/cmd/timeline/staging.rs | 86 +++++++++++++++++++ crates/engine/src/output_guard.rs | 21 +++-- crates/engine/src/tests/export_overwrite.rs | 52 ++++++++++- crates/ui-egui/src/control.rs | 11 ++- .../ui-egui/tests/it/ui_export_overwrite.rs | 3 + docs/control-protocol.md | 2 +- 15 files changed, 249 insertions(+), 49 deletions(-) create mode 100644 crates/engine/src/cmd/timeline/staging.rs diff --git a/Cargo.lock b/Cargo.lock index 07564b7df..091a6ebf8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1458,6 +1458,7 @@ dependencies = [ "same-file", "serde", "serde_json", + "tempfile", "thiserror 2.0.21", "web-time", ] diff --git a/Cargo.toml b/Cargo.toml index afc59257b..e105557ed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,7 @@ harfrust = { version = "0.12", default-features = false, features = ["std"] } exr = "1.74" image = { version = "0.25", default-features = false, features = ["png", "jpeg"] } same-file = "1.0" +tempfile = "3.27" # media codecs + containers: FilmCraft's pure-Rust crates, pinned to a revision (plan/adr/0001). # Only `crates/media`, `crates/export` and `crates/interchange` may depend on these. diff --git a/apps/effectcraft-cli/src/main.rs b/apps/effectcraft-cli/src/main.rs index 2befb5c4b..816186bed 100644 --- a/apps/effectcraft-cli/src/main.rs +++ b/apps/effectcraft-cli/src/main.rs @@ -726,12 +726,9 @@ fn run(cmd: &str, args: &Args, json_out: bool) -> Result<(), Failure> { } } let out = args.opt("--out").unwrap_or("frame.png").to_string(); - // Never over footage, a proxy or the project file (checked before rendering). - b.check_output(&out)?; let t0 = std::time::Instant::now(); - let f = b.render_with(comp.as_ref(), time, max_side, args.flag("--transparent"))?; + let f = b.render_to_file(comp.as_ref(), time, max_side, args.flag("--transparent"), &out)?; let ms = t0.elapsed().as_secs_f64() * 1000.0; - std::fs::write(&out, &f.png).map_err(|e| format!("cannot write {out}: {e}"))?; let info = json!({"path": out, "comp": f.comp, "time": f.time, "width": f.width, "height": f.height, "ms": (ms * 10.0).round() / 10.0}); if json_out { emit(&info, true); diff --git a/crates/automation/src/backend.rs b/crates/automation/src/backend.rs index c0dca6c9a..e0116403a 100644 --- a/crates/automation/src/backend.rs +++ b/crates/automation/src/backend.rs @@ -85,10 +85,9 @@ impl Backend { } } - /// Refuse `path`, a file this caller is about to write itself (a frame, a screenshot), when - /// the open project reads it: footage, a proxy or the project file. The project is the - /// session's or the app's, so both are asked through `file.checkOutput`; a relative path is - /// made absolute here, against this process's folder, because the app has its own. + /// Query whether the current project protects `path`. This is an advisory snapshot, not a + /// reservation: use [`Self::render_to_file`] to validate and write in the owning session. + /// Relative paths are resolved against this process's folder, because the app has its own. pub fn check_output(&mut self, path: &str) -> Result<()> { let absolute = std::path::absolute(path).map_err(|e| Error::BadArgs(format!("cannot resolve {path}: {e}")))?; let found = self.exec("file.checkOutput", json!({"path": absolute.to_string_lossy()}))?; @@ -115,16 +114,40 @@ impl Backend { /// [`Backend::render`]; `transparent` keeps the frame's alpha instead of compositing it over /// the comp's background colour. pub fn render_with(&mut self, comp: Option<&Value>, time: Option, max_side: u32, transparent: bool) -> Result { + self.render_output(comp, time, max_side, transparent, None) + } + + /// Render and save in the owning session. In bridge mode, checking and writing are one + /// desktop operation, so another command cannot make the destination protected in between. + pub fn render_to_file(&mut self, comp: Option<&Value>, time: Option, max_side: u32, transparent: bool, path: &str) -> Result { + if let Some(confinement) = self.confinement() { + confinement.check(path, crate::confine::Access::Write)?; + } + let absolute = std::path::absolute(path).map_err(|e| Error::BadArgs(format!("cannot resolve {path}: {e}")))?; + self.render_output(comp, time, max_side, transparent, Some(&absolute.to_string_lossy())) + } + + fn render_output(&mut self, comp: Option<&Value>, time: Option, max_side: u32, transparent: bool, path: Option<&str>) -> Result { match &mut self.target { Target::Headless(s) => { + if let Some(path) = path { + s.refuse_export(path)?; + } let cid = s.resolve_comp(comp).map_err(|_| no_comp(comp))?; let t = time.map(Tick::from_seconds_f64).unwrap_or_else(|| s.time()); let (w, h, rgba) = s.render_rgba8_alpha(cid, t, max_side, transparent)?; let png = encode_png(w, h, rgba, max_side)?; + if let Some(path) = path { + s.write_output(path, &png)?; + } Ok(Frame { png, comp: json!(cid.0), time: t.seconds(), width: w, height: h }) } Target::Bridge(b) => { let mut p = json!({"max_side": max_side, "base64": true}); + if let Some(path) = path { + p["path"] = json!(path); + p["write"] = json!(true); + } if transparent { p["transparent"] = json!(true); } @@ -135,6 +158,11 @@ impl Backend { p["comp"] = c.clone(); } let r = b.call("render.frame", p)?; + if let Some(path) = path + && r.get("path").and_then(Value::as_str) != Some(path) + { + return Err(Error::App("render.frame did not confirm the saved path; update the connected app before saving frames".into())); + } let data = r.get("png").and_then(Value::as_str).ok_or_else(|| Error::App("render.frame returned no `png`".into()))?; let raw = base64::decode(data).ok_or_else(|| Error::App("render.frame: bad base64".into()))?; let (png, width, height) = recompress_png(&raw, max_side)?; diff --git a/crates/automation/src/confine.rs b/crates/automation/src/confine.rs index 0e9d79d4b..1cdd1b29d 100644 --- a/crates/automation/src/confine.rs +++ b/crates/automation/src/confine.rs @@ -103,6 +103,8 @@ const WRITES: &[(&str, &[&str])] = &[ ("file.saveCopy", &["path"]), ("file.saveCopyAsXml", &["path"]), ("file.collectFiles", &["folder"]), + // This query checks a proposed write destination, so it follows write confinement. + ("file.checkOutput", &["path"]), ("file.exportLottie", &["path"]), ("comp.saveFrameAs", &["path"]), ("comp.saveFrameAsPsd", &["path"]), @@ -472,8 +474,12 @@ mod tests { let steps = serde_json::json!({"steps": [{"command": "essential.templateInfo", "params": {"path": "t.ectemplate"}}]}); assert!(c.check_command("engine.batch", &steps).is_err()); // Writes: into a write root only. - assert!(c.check_command("file.exportTimeline", &serde_json::json!({"path": p(&read, "cut.xml")})).is_err()); - assert!(c.check_command("file.exportTimeline", &serde_json::json!({"path": p(&write, "cut.xml")})).is_ok()); + for id in ["file.exportTimeline", "file.checkOutput"] { + for bad in ["cut.xml".to_string(), p(&read, "cut.xml"), p(&outside, "cut.xml")] { + assert!(c.check_command(id, &serde_json::json!({"path": bad})).is_err(), "{id} {bad}"); + } + assert!(c.check_command(id, &serde_json::json!({"path": p(&write, "cut.xml")})).is_ok()); + } let create = |project: String| serde_json::json!({"path": p(&read, "t.ectemplate"), "projectPath": project}); assert!(c.check_command("templates.create", &create(p(&read, "new.ecproj"))).is_err(), "templates.create writes projectPath"); assert!(c.check_command("templates.create", &create(p(&write, "new.ecproj"))).is_ok()); diff --git a/crates/automation/src/tests.rs b/crates/automation/src/tests.rs index 5e846dd3a..497e3e303 100644 --- a/crates/automation/src/tests.rs +++ b/crates/automation/src/tests.rs @@ -486,6 +486,21 @@ fn autosave_flushes_on_read_errors_and_broken_output() { std::fs::remove_dir_all(dir).unwrap(); } +fn fake_render_frame(session: &Session, params: &Value) -> Value { + let png = crate::encode_png(4, 2, vec![255; 32], 0).unwrap(); + if params["write"] == true { + let Some(path) = params["path"].as_str() else { return json!({"ok": false, "error": "missing output path"}) }; + if let Err(error) = session.write_output(path, &png) { + return json!({"ok": false, "error": error.to_string()}); + } + } + let mut result = json!({"comp": 1, "time": 0.5, "width": 4, "height": 2, "png": base64::encode(&png)}); + if params["write"] == true { + result["path"] = params["path"].clone(); + } + json!({"ok": true, "result": result}) +} + /// A fake desktop control channel: answers `engine.execute` from a real session, and canned /// `render.frame` / `ui.screenshot` replies. fn fake_app() -> u16 { @@ -503,10 +518,7 @@ fn fake_app() -> u16 { Ok(v) => json!({"ok": true, "result": v}), Err(e) => json!({"ok": false, "error": e.to_string()}), }, - "render.frame" => { - let png = crate::encode_png(4, 2, vec![255; 32], 0).unwrap(); - json!({"ok": true, "result": {"comp": 1, "time": 0.5, "width": 4, "height": 2, "png": base64::encode(&png)}}) - } + "render.frame" => fake_render_frame(&session, p), "ui.screenshot" => { let png = crate::encode_png(8, 8, vec![128; 256], 0).unwrap(); std::fs::write(p["path"].as_str().unwrap(), png).unwrap(); @@ -1695,10 +1707,8 @@ fn bridged_render_frame_asks_the_app_before_it_writes() { Ok(v) => json!({"ok": true, "result": v}), Err(e) => json!({"ok": false, "error": e.to_string()}), }, - "render.frame" => { - let png = crate::encode_png(4, 2, vec![255; 32], 0).unwrap(); - json!({"ok": true, "result": {"comp": 1, "time": 0.5, "width": 4, "height": 2, "png": base64::encode(&png)}}) - } + "render.frame" if p["write"] == true && p["path"].is_string() => fake_render_frame(&session, p), + "render.frame" => json!({"ok": false, "error": "a frame save must be one app-owned write"}), m => json!({"ok": false, "error": format!("unknown method `{m}`")}), }; reply["id"] = msg["id"].clone(); diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index aeffa464c..cb1a3cdab 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -407,17 +407,14 @@ fn render_frame(b: &mut Backend, a: &Value) -> Result { let max_side = get(a, "max_side").and_then(Value::as_u64).unwrap_or(960) as u32; let transparent = get(a, "transparent").and_then(Value::as_bool).unwrap_or(false); let path = get(a, "path").and_then(Value::as_str); - if let Some(p) = path { - if let Some(c) = b.confinement() { - c.check(p, crate::confine::Access::Write)?; - } - // Before the frame is rendered: never over footage, a proxy or the project file. - b.check_output(p)?; - } - let f = b.render_with(get(a, "comp"), get(a, "time").and_then(Value::as_f64), max_side, transparent)?; + let comp = get(a, "comp"); + let time = get(a, "time").and_then(Value::as_f64); + let f = match path { + Some(path) => b.render_to_file(comp, time, max_side, transparent, path)?, + None => b.render_with(comp, time, max_side, transparent)?, + }; let mut info = json!({"comp": f.comp, "time": f.time, "width": f.width, "height": f.height}); if let Some(p) = path { - std::fs::write(p, &f.png).map_err(|e| Error::Other(format!("cannot write {p}: {e}")))?; info["path"] = json!(p); } if get(a, "inline").and_then(Value::as_bool) == Some(false) { diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index 4bb488569..8e0f23711 100644 --- a/crates/engine/Cargo.toml +++ b/crates/engine/Cargo.toml @@ -43,3 +43,4 @@ exr = { workspace = true } # Native only: the volume's own identity for a file, to tell that a hard link is the footage it leads to. [target.'cfg(not(target_arch = "wasm32"))'.dependencies] same-file = { workspace = true } +tempfile = { workspace = true } diff --git a/crates/engine/src/cmd/timeline.rs b/crates/engine/src/cmd/timeline.rs index f607a36dc..fbc11654f 100644 --- a/crates/engine/src/cmd/timeline.rs +++ b/crates/engine/src/cmd/timeline.rs @@ -16,6 +16,8 @@ use super::{CommandSpec, always, b_p, bad, comp_id, f_p, has_comp, str_p}; use crate::jobs::TaskCtl; use crate::{EngineError, Exporter, Result, Session, cmd}; +mod staging; + fn format_p(p: &Value, cmd: &str) -> Result> { match str_p(p, "format") { None | Some("") | Some("auto") => Ok(None), @@ -178,7 +180,6 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { let relative_to = Some(dir.to_string_lossy().to_string()).filter(|d| !d.is_empty()); let opts = ExportOptions { format, prerender: prerender_mode, precomps, relative_to }; let project = s.project.clone(); - let services = s.services.clone(); let cx = PrerenderCx { protected: crate::output_guard::Protected::of(s), exporter: s.exporter.clone(), @@ -191,17 +192,21 @@ fn export_timeline(s: &mut Session, p: &Value) -> Result { // in Window ▸ Progress instead of with the window waiting (#2269). let wait = !b_p(p, "background").unwrap_or(false); s.spawn_task("exportTimeline", format!("Export {}", format.label()), wait, move |ctl| { - let (bytes, mut result) = export_timeline_work(&cx, &project, cid, &opts, &dir, &stem, ctl)?; + let (bytes, mut result, staged) = export_timeline_work(&cx, &project, cid, &opts, &dir, &stem, ctl)?; if let Some(source) = cx.protected.find(std::path::Path::new(&path)) { return Err(source.refusal(&path, crate::output_guard::Use::Export)); } - services.write_file(&path, &bytes).map_err(|e| format!("cannot write {path}: {e}"))?; result["path"] = json!(path); result["toast"] = json!(match format { TimelineFormat::Fcp7Xml => format!("Exported {path} (Final Cut Pro XML; import it in Premiere Pro with File ▸ Import)"), _ => format!("Exported {path} ({})", format.label()), }); - Ok(Box::new(move |_: &mut Session| Ok(result))) + Ok(Box::new(move |s: &mut Session| { + s.refuse_export(&path)?; + staging::commit(staged, s)?; + s.write_output(&path, &bytes)?; + Ok(result) + })) }) } @@ -215,11 +220,15 @@ fn export_timeline_work( dir: &std::path::Path, stem: &str, ctl: &TaskCtl, -) -> std::result::Result<(Vec, Value), String> { +) -> std::result::Result<(Vec, Value, Vec), String> { let mut pre: HashMap<(ItemId, LayerId), Prerendered> = HashMap::new(); let mut rendered = vec![]; let mut errors = vec![]; + let mut staged = vec![]; let plan = effectcraft_interchange::plan_prerender(project, cid, opts); + if plan.len() > staging::MAX_MOVIES { + return Err(format!("too many pre-render movies (limit {})", staging::MAX_MOVIES)); + } let n = plan.len() as u64; for (i, (c, l)) in (0u64..).zip(plan) { let comp_name = project.item(c).map(|it| it.name.clone()).unwrap_or_default(); @@ -233,10 +242,13 @@ fn export_timeline_work( } // Progress over all the layers, each one's frames a share of it. let mut progress = |done: u64, total: u64| ctl.progress(i * 1000 + done.saturating_mul(1000) / total.max(1), n * 1000); - match prerender(cx, project, c, l, &file, &mut progress) { - Ok(r) => { + let movie = staging::Movie::new(&file)?; + match prerender(cx, project, c, l, &movie.path, &mut progress) { + Ok(mut r) => { + r.path = file; rendered.push(r.path.clone()); pre.insert((c, l), r); + staged.push(movie); } Err(_) if ctl.cancelled() => return Err(crate::render_queue::CANCELLED.into()), Err(e) => errors.push(format!("{comp_name} ▸ {layer_name}: {e}")), @@ -255,7 +267,7 @@ fn export_timeline_work( "prerendered": rendered, "warnings": warnings, }); - Ok((out.bytes, result)) + Ok((out.bytes, result, staged)) } /// The formats, for agents and the export dialog. diff --git a/crates/engine/src/cmd/timeline/staging.rs b/crates/engine/src/cmd/timeline/staging.rs new file mode 100644 index 000000000..103371cd2 --- /dev/null +++ b/crates/engine/src/cmd/timeline/staging.rs @@ -0,0 +1,86 @@ +//! Native pre-render files stay private until the app can check the current project. + +#[cfg(not(target_arch = "wasm32"))] +use crate::EngineError; +use crate::Session; + +pub(super) const MAX_MOVIES: usize = 4096; + +pub(super) struct Movie { + pub path: String, + #[cfg(not(target_arch = "wasm32"))] + directory: tempfile::TempDir, + #[cfg(not(target_arch = "wasm32"))] + destination: std::path::PathBuf, +} + +impl Movie { + pub fn new(path: &str) -> Result { + #[cfg(not(target_arch = "wasm32"))] + { + let path = std::path::Path::new(path); + let parent = path.parent().filter(|p| !p.as_os_str().is_empty()).unwrap_or(std::path::Path::new(".")); + std::fs::create_dir_all(parent).map_err(|e| format!("cannot create pre-render folder {}: {e}", parent.display()))?; + let directory = + tempfile::Builder::new().prefix(".effectcraft-prerender-").tempdir_in(parent).map_err(|e| format!("cannot stage pre-render: {e}"))?; + let name = path.file_name().ok_or_else(|| format!("pre-render has no file name: {}", path.display()))?; + let staged = directory.path().join(name).to_string_lossy().into_owned(); + Ok(Self { path: staged, directory, destination: parent.to_path_buf() }) + } + #[cfg(target_arch = "wasm32")] + { + // spawn_task is synchronous on WASM, so session mutations cannot interleave. + Ok(Self { path: path.to_string() }) + } + } +} + +pub(super) fn commit(movies: Vec, session: &Session) -> crate::Result<()> { + #[cfg(not(target_arch = "wasm32"))] + { + const FILES_PER_MOVIE: usize = 8; + if movies.len() > MAX_MOVIES { + return Err(EngineError::Other("too many staged pre-render movies".into())); + } + let mut files = Vec::new(); + let mut path_bytes = 0usize; + for movie in &movies { + let entries = std::fs::read_dir(movie.directory.path()).map_err(|e| EngineError::Other(format!("cannot read staged pre-render: {e}")))?; + for (index, entry) in entries.take(FILES_PER_MOVIE + 1).enumerate() { + if index == FILES_PER_MOVIE { + return Err(EngineError::Other("too many files in a staged pre-render".into())); + } + let entry = entry.map_err(|e| EngineError::Other(format!("cannot read staged pre-render entry: {e}")))?; + let kind = entry.file_type().map_err(|e| EngineError::Other(format!("cannot inspect staged pre-render: {e}")))?; + if !kind.is_file() { + return Err(EngineError::Other("a staged pre-render contains an unexpected directory or link".into())); + } + let source = entry.path(); + let destination = movie.destination.join(entry.file_name()); + path_bytes = path_bytes.saturating_add(source.as_os_str().len()).saturating_add(destination.as_os_str().len()); + if path_bytes > 16 * 1024 * 1024 { + return Err(EngineError::Other("staged pre-render paths exceed the 16 MiB limit".into())); + } + files.push((source, destination)); + } + } + // If a user imported a private staging file, leave the staging folders intact. + if let Some((source, _)) = files.iter().find(|(source, _)| session.refuse_export(&source.to_string_lossy()).is_err()) { + let source = source.display().to_string(); + for movie in movies { + let _kept = movie.directory.keep(); + } + return Err(EngineError::Other(format!("staged pre-render {source} is now used by the project; its temporary folder was kept"))); + } + // Preflight every destination before replacing any output, including render logs. + for (_, destination) in &files { + session.refuse_export(&destination.to_string_lossy())?; + } + for (source, destination) in files { + std::fs::rename(&source, &destination).map_err(|e| EngineError::Other(format!("cannot publish pre-render {}: {e}", destination.display())))?; + } + } + #[cfg(target_arch = "wasm32")] + let _ = (movies, session); + Ok(()) +} diff --git a/crates/engine/src/output_guard.rs b/crates/engine/src/output_guard.rs index 40dcc1e56..ca667cd99 100644 --- a/crates/engine/src/output_guard.rs +++ b/crates/engine/src/output_guard.rs @@ -12,7 +12,9 @@ //! write the project's own file and do not. use std::collections::{HashMap, HashSet}; -use std::path::{Component, Path, PathBuf}; +#[cfg(not(target_arch = "wasm32"))] +use std::path::Component; +use std::path::{Path, PathBuf}; use std::sync::Arc; use effectcraft_project::render_queue::{RenderQueueItem, log_path, numbered_template, sequence_frame, sequence_path}; @@ -180,6 +182,7 @@ fn identity_hash(handle: &same_file::Handle) -> u64 { } /// `path` made absolute, with `.` and `..` resolved by name alone (no file is looked at). +#[cfg(not(target_arch = "wasm32"))] fn lexical(path: &Path) -> PathBuf { let absolute = std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()); let mut out = PathBuf::new(); @@ -200,11 +203,19 @@ fn lexical(path: &Path) -> PathBuf { } /// The name a path is compared by: absolute, resolved by name, without case where volumes ignore it. +#[cfg(not(target_arch = "wasm32"))] fn key(path: &Path) -> String { let parts: Vec = lexical(path).components().map(|c| c.as_os_str().to_string_lossy().into_owned()).collect(); fold(&parts.join("/")) } +/// Browser file-table keys distinguish case and literal dot segments, but not a leading slash. +#[cfg(target_arch = "wasm32")] +fn key(path: &Path) -> String { + let path = path.to_string_lossy(); + if path.starts_with('/') { path.into_owned() } else { format!("/{path}") } +} + fn fold(name: &str) -> String { if IGNORES_CASE { name.to_lowercase() } else { name.to_string() } } @@ -274,9 +285,6 @@ impl Session { /// What reads the file at `path`, if the project does. fn blocking(&self, path: &str) -> Option { - if cfg!(target_arch = "wasm32") { - return None; - } self.protected_files().find(Path::new(path)).cloned() } @@ -300,9 +308,7 @@ impl Session { /// [`Session::write_output`] against files already found ([`Session::protected_files`]), /// for a command that writes many. pub(crate) fn write_output_with(&self, protected: &Protected, path: &str, bytes: &[u8]) -> crate::Result<()> { - if !cfg!(target_arch = "wasm32") - && let Some(source) = protected.find(Path::new(path)) - { + if let Some(source) = protected.find(Path::new(path)) { return Err(crate::EngineError::Other(source.refusal(path, Use::Export))); } self.services.write_file(path, bytes).map_err(|e| crate::EngineError::Other(format!("cannot write {path}: {e}"))) @@ -452,6 +458,7 @@ mod tests { } #[test] + #[cfg(not(target_arch = "wasm32"))] fn paths_compare_by_name_once_resolved() { let here = std::env::current_dir().unwrap(); assert_eq!(key(Path::new("a/b/../c.mp4")), key(Path::new("a/c.mp4"))); diff --git a/crates/engine/src/tests/export_overwrite.rs b/crates/engine/src/tests/export_overwrite.rs index 50f5d2ff8..5cc65b1d5 100644 --- a/crates/engine/src/tests/export_overwrite.rs +++ b/crates/engine/src/tests/export_overwrite.rs @@ -255,8 +255,8 @@ fn a_timeline_export_does_not_pre_render_over_footage() { } } -/// `file.checkOutput` answers before anything is written, for callers that write the file -/// themselves (the CLI, MCP's `render_frame`). +/// `file.checkOutput` answers before anything is written. Its reply describes current state; +/// callers still need a guarded write in the owning session, as CLI/MCP frame saves use. #[test] fn check_output_names_the_files_an_export_may_not_use() { let mut f = reading("check-output", "png"); @@ -283,3 +283,51 @@ fn a_timeline_export_does_not_write_over_the_project_file() { assert!(e.contains("Edit.xml") && e.contains("project"), "{e}"); assert_eq!(f.read("Edit.xml"), saved); } + +#[test] +fn a_background_timeline_export_rechecks_its_document_before_completion() { + let mut f = Fixture::new("timeline-current-project"); + f.comp("Main"); + let path = f.text("Pending.xml"); + let job = f.s.execute("file.exportTimeline", json!({"path": path, "background": true, "prerender": "none"})).unwrap(); + assert!(job["job"].is_string()); + // The app may save the project while a background export is waiting to finish. + f.s.execute("file.saveAs", json!({"path": path})).unwrap(); + let saved = f.read("Pending.xml"); + f.s.wait_jobs(); + let done = f.s.job_log.last().unwrap(); + assert_eq!(done.status, "failed", "the destination became the project: {}", done.message); + assert!(done.message.contains("project")); + assert_eq!(f.read("Pending.xml"), saved); +} + +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn background_pre_render_movies_are_checked_again_before_publication() { + for source in ["footage", "project"] { + let mut f = Fixture::new(&format!("timeline-staged-{source}")); + f.comp("Main"); + f.s.execute("layer.newSolid", json!({"name": "Red"})).unwrap(); + let params = json!({"path": f.text("Main.otio"), "prerender": "unsupported"}); + let first = f.s.execute("file.exportTimeline", params.clone()).unwrap(); + let movie = first["prerendered"][0].as_str().unwrap().to_string(); + let filename = std::path::Path::new(&movie).file_name().unwrap().to_string_lossy().into_owned(); + let document = f.read("Main.otio"); + let mut background = params; + background["background"] = json!(true); + f.s.execute("file.exportTimeline", background).unwrap(); + if source == "footage" { + f.footage(&filename); + } else { + f.s.execute("file.saveAs", json!({"path": movie})).unwrap(); + } + let protected = f.read(&filename); + f.s.wait_jobs(); + let done = f.s.job_log.last().unwrap(); + assert_eq!(done.status, "failed", "{}", done.message); + assert!(done.message.contains(source), "{}", done.message); + assert_eq!(f.read(&filename), protected); + assert_eq!(f.read("Main.otio"), document); + assert!(listing(&f.dir).iter().all(|name| !name.starts_with(".effectcraft-prerender-"))); + } +} diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 023bfa8b3..86bd20c93 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -673,9 +673,9 @@ fn quit(app: &mut EffectcraftApp, ctx: &egui::Context, p: &Value) -> Outcome { ok(Value::Null) } -/// `render.frame {comp?, time?, max_side?, path?, base64?}`: render a comp frame through the session +/// `render.frame {comp?, time?, max_side?, path?, base64?, write?}`: render a comp frame through the session /// (independent of the viewer zoom/resolution) and return it as PNG, written to `path` (default a -/// temp file) or inline as base64 when `base64: true`. +/// temp file) or inline as base64 when `base64: true`. `write: true` with `base64: true` does both. fn render_frame(app: &EffectcraftApp, p: &Value) -> Outcome { let s = &app.session; // A file the project reads is never the output (checked before rendering). @@ -702,7 +702,7 @@ fn render_frame(app: &EffectcraftApp, p: &Value) -> Outcome { let mut out = json!({"comp": cid.0, "time": t.seconds(), "width": w, "height": h}); // (the web has no file system: inline unless a path is given) let inline = p.get("base64").and_then(Value::as_bool).unwrap_or(cfg!(target_arch = "wasm32") && p.get("path").is_none()); - if inline { + if inline && !p.get("write").and_then(Value::as_bool).unwrap_or(false) { out["png"] = json!(base64(&png)); return ok(out); } @@ -711,7 +711,10 @@ fn render_frame(app: &EffectcraftApp, p: &Value) -> Outcome { .and_then(Value::as_str) .map(str::to_string) .unwrap_or_else(|| std::env::temp_dir().join(format!("effectcraft-frame-{}.png", std::process::id())).to_string_lossy().to_string()); - match std::fs::write(&path, png) { + if inline { + out["png"] = json!(base64(&png)); + } + match s.write_output(&path, &png) { Ok(()) => { out["path"] = json!(path); ok(out) diff --git a/crates/ui-egui/tests/it/ui_export_overwrite.rs b/crates/ui-egui/tests/it/ui_export_overwrite.rs index e14d9a7e8..997359479 100644 --- a/crates/ui-egui/tests/it/ui_export_overwrite.rs +++ b/crates/ui-egui/tests/it/ui_export_overwrite.rs @@ -89,6 +89,9 @@ fn render_frame_never_writes_over_a_file_the_project_reads() { let v = r.reply("render.frame", json!({"path": r.text("frame.png")})); assert_eq!(v["ok"], true, "{v}"); assert_eq!(&r.read("frame.png")[..8], b"\x89PNG\r\n\x1a\n"); + let v = r.reply("render.frame", json!({"path": r.text("inline.png"), "base64": true, "write": true})); + assert_eq!(v["ok"], true, "{v}"); + assert_eq!(v["result"]["png"].as_str(), Some(control::base64(&r.read("inline.png")).as_str())); } #[test] diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 900d9518a..60328099e 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -41,7 +41,7 @@ printf '{"id":1,"method":"engine.execute","params":{"command":"comp.new","params | UI commands (via either) | | `timeline.column {column, visible?}` (av, keys, label, num, comment, switches, modes, parent, in, out, duration, stretch), `timeline.sourceName {value?}`, `timeline.search {query}`, `timeline.reveal.` / `timeline.revealAdd.` (the property shortcuts and Shift+shortcut: position, scale, rotation, opacity, anchor, effects, masks, feather, levels, animated, and the double-press sets maskPath, maskOpacity, timeRemap, expressions, material, paint, selected, missingEffects, waveform), `timeline.keyAt.` (Alt+Shift+A/P/S/R/T), `timeline.twirlSelected` ``(Ctrl/Cmd+`)``, `timeline.collapseAll`, `flowchart.options {layers?, effects?, solids?, direction?: lr\|tb, comp?}`, `flowchart.graph {comp?}` → `{nodes, edges}`, `window.workspace {name}` (all After Effects workspaces). | | `engine.commands` | `{filter?, enabledOnly?}` | `[{id, label, menu, shortcut, params, enabled, why}]` | | `ui.menu.list` | `{}` | The menu tree with enablement. | -| `render.frame` | `{comp?, time?, max_side?, path?, base64?}` | Renders a comp frame through the session, independent of the viewer zoom or resolution. `comp` is an id or name (default: the active comp), `time` is in comp seconds (default: the CTI), `max_side` caps the longest side (0 = full size). Writes a PNG to `path` (default: a temp file) and returns `{comp, time, width, height, path}`. With `base64: true` it returns `{…, png: ""}` and writes no file. | +| `render.frame` | `{comp?, time?, max_side?, path?, base64?, write?}` | Renders a comp frame through the session, independent of the viewer zoom or resolution. `comp` is an id or name (default: the active comp), `time` is in comp seconds (default: the CTI), `max_side` caps the longest side (0 = full size). Writes a PNG to `path` (default: a temp file) and returns `{comp, time, width, height, path}`. With `base64: true` it returns `{…, png: ""}` and writes no file unless `write: true` also requests a guarded write in the same app operation. | Useful query commands for `engine.execute` (they aren't journaled or undoable): `project.summary`, `comp.info {comp?}`, `layer.tree {layer, comp?, depth?, time?}` (every node has a `path`), From b307c62a7f28393bbac9e61555ace5e50215cd18 Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Fri, 30 Oct 2026 10:00:33 +0100 Subject: [PATCH 6/6] #1743: reuse protected identities while publishing pre-renders --- crates/engine/src/cmd/timeline/staging.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/cmd/timeline/staging.rs b/crates/engine/src/cmd/timeline/staging.rs index 103371cd2..026b1200a 100644 --- a/crates/engine/src/cmd/timeline/staging.rs +++ b/crates/engine/src/cmd/timeline/staging.rs @@ -65,7 +65,8 @@ pub(super) fn commit(movies: Vec, session: &Session) -> crate::Result<()> } } // If a user imported a private staging file, leave the staging folders intact. - if let Some((source, _)) = files.iter().find(|(source, _)| session.refuse_export(&source.to_string_lossy()).is_err()) { + let protected = session.protected_files(); + if let Some((source, _)) = files.iter().find(|(source, _)| protected.find(source).is_some()) { let source = source.display().to_string(); for movie in movies { let _kept = movie.directory.keep(); @@ -74,7 +75,9 @@ pub(super) fn commit(movies: Vec, session: &Session) -> crate::Result<()> } // Preflight every destination before replacing any output, including render logs. for (_, destination) in &files { - session.refuse_export(&destination.to_string_lossy())?; + if let Some(found) = protected.find(destination) { + return Err(EngineError::Other(found.refusal(&destination.to_string_lossy(), crate::output_guard::Use::Export))); + } } for (source, destination) in files { std::fs::rename(&source, &destination).map_err(|e| EngineError::Other(format!("cannot publish pre-render {}: {e}", destination.display())))?;