collective
Browse / github.com/storytold/effectcraft / patch
automation: a confined agent can't queue a render output outside the roots (#2479)
Automation: `renderQueue.add` without `output` on an unsaved project targets the home folder, outside the automation roots Split from #2478 at ticket-craft's request. Measured on EffectCraft 0.16.0 (Windows portable) with `mcp --automation-read-root <dir> --automation-write-root <dir>`. On a project that has never been saved, `renderQueue.add {comp}` without `output` queues an item whose destination (`renderQueue.list`) is `<home>\<comp name>.mp4`, i.e. outside every automation root. Once the project is saved, the default goes next to the project file, inside the root. I removed the item from the queue without rendering, so I have not checked whether `renderQueue.render` would then refuse the write or perform it.
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Anubhav-Rai (imported from pull request #2520, open)
- Base
bc4b48f67deef2458e9dd3e3b0e592d5f75768a1- Size
- +90 −14 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
95532bd24f5b79d6572e2060
How it went
No other operator has reported using it yet.
Diff
raw · crates/automation/src/backend.rs crates/automation/src/confine.rs crates/automation/src/tests.rs
@@ -4,7 +4,7 @@ use effectcraft_engine::Session; use effectcraft_engine::time::Tick; use serde_json::{Value, json}; -use crate::confine::Confinement;+use crate::confine::{Confinement, Lookup}; use crate::{BridgeClient, Error, Result, base64, encode_png, recompress_png}; pub struct Backend {@@ -63,12 +63,11 @@ impl Backend { pub fn exec(&mut self, id: &str, params: Value) -> Result<Value> { let params = if params.is_null() { json!({}) } else { params }; if let Some(c) = self.confinement.clone() {- let lookup = match c.check_command(id, ¶ms)? {- Some(crate::confine::Lookup::ProjectFile) => Some((crate::confine::Lookup::ProjectFile, self.run("project.summary", json!({}))?)),- Some(crate::confine::Lookup::RenderOutputs) => Some((crate::confine::Lookup::RenderOutputs, self.run("renderQueue.list", json!({}))?)),- None => None,- };- if let Some((lookup, found)) = lookup {+ for lookup in c.check_command(id, ¶ms)? {+ let found = match lookup {+ Lookup::ProjectFile | Lookup::DefaultOutput => self.run("project.summary", json!({}))?,+ Lookup::RenderOutputs => self.run("renderQueue.list", json!({}))?,+ }; c.check_lookup(&lookup, &found)?; } }@@ -6,8 +6,10 @@ //! `engine.batch`) is checked before it runs: its file parameters must be absolute paths inside //! the roots (symbolic links resolved, no `..`), any other absolute path in its parameters too, //! and commands that write to a file they are not given (Save, Increment and Save, rendering the-//! Render Queue) are checked against where they would write. Commands that run code or open-//! other applications (scripts, Execute File, Reveal in Finder…) are refused while confined.+//! Render Queue) are checked against where they would write. Queueing a render without naming+//! its output needs a saved project, because the default output of an unsaved one is the home+//! folder. Commands that run code or open other applications (scripts, Execute File, Reveal in+//! Finder…) are refused while confined. use std::path::{Component, Path, PathBuf}; @@ -84,16 +86,23 @@ const WRITES: &[(&str, &[&str])] = &[ ("essential.exportTemplate", &["path"]), ("shortcuts.export", &["path"]), ("renderQueue.add", &["output", "path"]),+ ("file.createProxy", &["path", "output"]), ]; /// Commands that write where they are not told: the project's file, the Render Queue's outputs. const IMPLICIT_WRITES: &[&str] = &["file.save", "file.incrementAndSave", "renderQueue.render", "render.preRender"]; +/// Commands that queue a render, whose output defaults to the project's folder when not given.+const QUEUES_AN_OUTPUT: &[&str] = &["renderQueue.add", "render.preRender", "file.createProxy"];+ /// What must be looked up before an implicitly writing command may run. #[derive(Debug, PartialEq, Eq)] pub enum Lookup { /// The open project's file (`project.summary` → `path`). ProjectFile,+ /// That the project has a file at all (`project.summary` → `path`), for a queued render whose+ /// output the agent did not name: that default is derived from where the project sits.+ DefaultOutput, /// Every output of the queued Render Queue items (`renderQueue.list`). RenderOutputs, }@@ -134,7 +143,7 @@ impl Confinement { /// Check engine command `id` with `params` before it runs. Returns what must still be looked /// up and passed to [`Confinement::check_lookup`] for commands that write implicitly.- pub fn check_command(&self, id: &str, params: &Value) -> Result<Option<Lookup>> {+ pub fn check_command(&self, id: &str, params: &Value) -> Result<Vec<Lookup>> { if REFUSED.contains(&id) { return Err(Error::BadArgs(format!("`{id}` is not available while file access is confined (--automation-read-root/--automation-write-root)"))); }@@ -147,11 +156,11 @@ impl Confinement { if named.iter().filter_map(|k| inner_params.get(*k)).any(has_reference) { return Err(Error::BadArgs(format!("`{inner}` in a batch: file paths can't be `$N` references while file access is confined"))); }- if self.check_command(inner, &inner_params)?.is_some() {+ if !self.check_command(inner, &inner_params)?.is_empty() { return Err(Error::BadArgs(format!("`{inner}` writes to its project's or queue's files: run it on its own while file access is confined"))); } }- return Ok(None);+ return Ok(vec![]); } let (named, access) = match (keys_of(WRITES, id), keys_of(READS, id)) { (Some(k), _) => (k, Access::Write),@@ -160,6 +169,10 @@ impl Confinement { (None, None) => (&[][..], Access::Read), }; let mut given = false;+ // Whether one of them is a file: not a Render Queue template, whose own output is relative+ // and resolved like one the agent never gave, and not an array, which the commands that+ // take a single output read as nothing at all.+ let mut gave_a_file = false; for key in named { let Some(v) = params.get(*key).filter(|v| !v.is_null()) else { continue }; given = true;@@ -172,6 +185,7 @@ impl Confinement { match p.as_str() { Some(s) if id == "render.preRender" && !looks_like_path(s) => {} Some(s) => {+ gave_a_file |= v.is_string(); self.check(s, access)?; } None => return Err(Error::BadArgs(format!("`{id}`: `{key}` must be a path"))),@@ -188,10 +202,19 @@ impl Confinement { if matches!(id, "file.openRecent" | "file.importRecent" | "anim.applyRecentPreset") && !given { return Err(Error::BadArgs(format!("`{id}`: give `path` while file access is confined"))); }+ let mut lookups = vec![];+ // An output the agent did not name is derived from where the project sits: the Default+ // Output Folder preference when that is set, beside the project file otherwise — and the+ // home folder when the project has never been saved, outside every root (#2479). Only+ // that last case is certainly outside, and reading the preference here would duplicate+ // the engine's precedence, so an unsaved project is told to name its output instead.+ if QUEUES_AN_OUTPUT.contains(&id) && !gave_a_file {+ lookups.push(Lookup::DefaultOutput);+ } if IMPLICIT_WRITES.contains(&id) && !(given && id == "file.save") {- return Ok(Some(if id == "file.save" || id == "file.incrementAndSave" { Lookup::ProjectFile } else { Lookup::RenderOutputs }));+ lookups.push(if id == "file.save" || id == "file.incrementAndSave" { Lookup::ProjectFile } else { Lookup::RenderOutputs }); }- Ok(None)+ Ok(lookups) } /// Check what a [`Lookup`] found: `project.summary` or `renderQueue.list`'s result.@@ -202,6 +225,14 @@ impl Confinement { Some(p) => self.check(p, Access::Write).map(drop), None => Err(Error::BadArgs("the project has no file yet: give `path` (Save As) inside an automation write root".into())), },+ Lookup::DefaultOutput => match found.get("path").and_then(Value::as_str) {+ // Saved: the default goes beside the project file, inside the roots, and what it+ // writes is checked when the queue is rendered.+ Some(_) => Ok(()),+ None => Err(Error::BadArgs(+ "the project has no file yet, so this would queue its default output (the home folder) outside the automation write roots: give `output`, or save the project inside a write root first".into(),+ )),+ }, Lookup::RenderOutputs => { let items = found.get("items").or(Some(found)).and_then(Value::as_array).cloned().unwrap_or_default(); for it in items.iter().filter(|it| it.get("render").and_then(Value::as_bool) != Some(false)) {@@ -1380,3 +1380,49 @@ fn automation_roots_confine_file_access() { assert_eq!(std::fs::read_dir(&outside).unwrap().count(), 1, "only the unconfined save"); let _ = std::fs::remove_dir_all(&base); }++/// #2479: a queued output the agent didn't name is derived from where the project sits, so on a+/// project that has never been saved it lands in the home folder, outside every automation root.+/// Queueing it there is refused, rather than leaving an item that can never be rendered.+#[test]+fn automation_roots_refuse_a_queued_output_from_an_unsaved_project() {+ use crate::confine::Confinement;+ let base = std::env::temp_dir().join(format!("ec-mcp-default-out-{}", std::process::id()));+ let _ = std::fs::remove_dir_all(&base);+ let write = base.join("out");+ std::fs::create_dir_all(&write).unwrap();+ let p = |f: &str| write.join(f).to_string_lossy().into_owned();+ let mut s = server();+ s.backend().confine(Confinement::new(&[], std::slice::from_ref(&write)).unwrap());+ call_json(&mut s, "execute_command", json!({"command": "comp.new", "params": {"name": "A", "width": 64, "height": 64}}));+ // Never saved: neither way of queueing a render may fall back to the home folder.+ for command in ["renderQueue.add", "render.preRender", "file.createProxy"] {+ let (c, err) = call(&mut s, "execute_command", json!({"command": command, "params": {"comp": "A"}}));+ assert!(err, "`{command}` without an output should be refused: {c:?}");+ let msg = c[0]["text"].as_str().unwrap_or_default();+ assert!(msg.contains("the project has no file yet"), "`{command}`: {msg}");+ }+ // An array where a path goes: the engine reads a scalar, so this is no output either.+ let (c, err) = call(&mut s, "execute_command", json!({"command": "file.createProxy", "params": {"comp": "A", "path": [p("arr.mov")]}}));+ assert!(err, "an array where the output goes should be refused: {c:?}");+ assert!(c[0]["text"].as_str().unwrap_or_default().contains("the project has no file yet"), "{c:?}");+ // Naming an Output Module template instead of a file is the same: its own output is relative.+ let (c, err) = call(&mut s, "execute_command", json!({"command": "render.preRender", "params": {"comp": "A", "output": "Lossless"}}));+ assert!(err, "a template output should be refused too: {c:?}");+ assert!(c[0]["text"].as_str().unwrap_or_default().contains("the project has no file yet"), "{c:?}");+ let queue = call_json(&mut s, "execute_command", json!({"command": "renderQueue.list"}));+ assert_eq!(queue["items"].as_array().map(Vec::len), Some(0), "nothing was queued: {queue}");+ // Naming an output inside a write root is how it is done.+ call_json(&mut s, "execute_command", json!({"command": "renderQueue.add", "params": {"comp": "A", "output": p("named.mp4")}}));+ // A proxy's own `path` is a write, not the read it used to be checked as (#2131 F-11).+ let (c, err) = call(&mut s, "execute_command", json!({"command": "file.createProxy", "params": {"comp": "A", "path": "/elsewhere/proxy.mov"}}));+ assert!(err, "a proxy outside the roots should be refused: {c:?}");+ assert!(c[0]["text"].as_str().unwrap_or_default().contains("cannot write"), "{c:?}");+ // Once the project is saved there, the default output (beside it) is queued as before.+ call_json(&mut s, "save_project", json!({"path": p("proj.ecproj")}));+ call_json(&mut s, "execute_command", json!({"command": "renderQueue.add", "params": {"comp": "A"}}));+ let queue = call_json(&mut s, "execute_command", json!({"command": "renderQueue.list"}));+ let out = queue["items"][1]["outputPath"].as_str().unwrap_or_default();+ assert!(out.starts_with(&*write.to_string_lossy()), "{queue}");+ let _ = std::fs::remove_dir_all(&base);+}