From 9011c6d704c4e03668f08cf4cccb245edbe6caec Mon Sep 17 00:00:00 2001 From: yoriswind Date: Mon, 2 Nov 2026 18:14:50 -0800 Subject: [PATCH] fix: capture shared action descendants at minimum call depth Use a FIFO dependency queue so a deep route cannot hide descendants reachable through a shallower call. Cover packaged Batch and Conditional replay, retain Custom exported enabled flags in regression coverage, and document the existing concurrent-writer limitation of Save and Close. --- crates/engine/src/cmd/actions_cmds.rs | 13 +++---- .../engine/tests/it/atn_review_regressions.rs | 35 +++++++++++++++++++ docs/atn.md | 3 ++ 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/actions_cmds.rs b/crates/engine/src/cmd/actions_cmds.rs index e5a1f36db2..f73a6b95b8 100644 --- a/crates/engine/src/cmd/actions_cmds.rs +++ b/crates/engine/src/cmd/actions_cmds.rs @@ -14,7 +14,7 @@ //! step, because the outer `actions.play` id would otherwise hide a recorded `file.*` command from //! the top-level check. -use std::collections::BTreeSet; +use std::collections::{BTreeSet, VecDeque}; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; @@ -273,7 +273,7 @@ pub fn called_actions(list: &[Action], steps: &mut [(String, Value)]) -> Vec None, } } - fn targets(list: &[Action], steps: &mut [(String, Value)], depth: usize, queue: &mut Vec<(usize, usize)>) { + fn targets(list: &[Action], steps: &mut [(String, Value)], depth: usize, queue: &mut VecDeque<(usize, usize)>) { let mut pending = Vec::new(); for (id, params) in steps.iter_mut() { pending.push((id.clone(), params)); @@ -303,14 +303,14 @@ pub fn called_actions(list: &[Action], steps: &mut [(String, Value)]) -> Vec { for key in ["then", "else"] { let Some(name) = params.get(key).and_then(Value::as_str) else { continue }; if let Some(i) = list.iter().position(|a| a.name == name) { - queue.push((i, depth)); + queue.push_back((i, depth)); } } } @@ -319,10 +319,11 @@ pub fn called_actions(list: &[Action], steps: &mut [(String, Value)]) -> Vec 16 || found.contains_key(&i) { continue; diff --git a/crates/engine/tests/it/atn_review_regressions.rs b/crates/engine/tests/it/atn_review_regressions.rs index f3ef59ca6d..ce205ebd8c 100644 --- a/crates/engine/tests/it/atn_review_regressions.rs +++ b/crates/engine/tests/it/atn_review_regressions.rs @@ -173,6 +173,8 @@ fn custom_duplicate_kernel_fields_are_kept_with_their_enabled_state() { assert_eq!(action.steps[0].0, atn_cmds::PHOTOSHOP_STEP); assert_eq!(action.disabled.contains(&0), !enabled); assert_eq!(exported(action), d); + let (bytes, _) = atn_cmds::export("Parent", &[action], &[], false).unwrap(); + assert_eq!(atn::parse(&bytes).unwrap().actions[0].items[0].enabled, enabled); } } } @@ -711,6 +713,39 @@ fn called_actions_packages_explicit_batch_children_and_replays_them() { } } +/// A deep route to a shared action must not hide children reached by a shallower playable branch. +#[test] +fn called_actions_packages_shared_descendants_at_the_shallowest_depth() { + let dir = tempfile::tempdir().unwrap(); + let input = dir.path().join("input.png"); + std::fs::write(&input, small_image(photocraft_codecs::Format::Png)).unwrap(); + let mut list = vec![ + Action::new("Shared", vec![("actions.play".into(), json!({"action":1}))]), + Action::new("Child", vec![("image.imageSize".into(), json!({"width":3,"height":1,"resample":"nearest"}))]), + ]; + // The long branch reaches Shared at depth16 and its child beyond the playback limit. + for i in 0..15 { + list.push(Action::new(format!("A{i}"), vec![("actions.play".into(), json!({"action":if i == 14 { 0 } else { i + 3 }}))])); + } + for (condition, then, otherwise) in [("always", "Shared", "A0"), ("never", "A0", "Shared")] { + let mut steps = vec![( + "file.automate.batch".into(), + json!({"input":[input],"output":dir.path().join(condition),"steps":[["actions.conditional",{"if":condition,"then":then,"else":otherwise}]]}), + )]; + let captured = photocraft_engine::actions_cmds::called_actions(&list, &mut steps); + assert_eq!(captured.iter().map(|a| &a.name).collect::>(), list.iter().map(|a| &a.name).collect::>()); + assert_eq!(captured[0].steps[0].1["action"], "Child"); + let mut s = Session::new(); + s.actions.list = captured; + let result = s.execute(&steps[0].0, steps[0].1.clone()).unwrap(); + assert!(result["errors"].as_array().unwrap().is_empty(), "{result}"); + assert_eq!(result["files"].as_array().unwrap().len(), 1, "{result}"); + let saved = std::fs::read(result["files"][0].as_str().unwrap()).unwrap(); + let doc = photocraft_io::import("saved.png", &saved).unwrap().document; + assert_eq!(doc.size, photocraft_doc::Size::new(3, 1)); + } +} + /// JSON containers do not consume action-call depth; the existing cycle and sixteen-call limits remain. #[test] fn called_actions_nested_batch_shapes_keep_cycle_and_action_depth_bounds() { diff --git a/docs/atn.md b/docs/atn.md index 50e8954487..5cee64108f 100644 --- a/docs/atn.md +++ b/docs/atn.md @@ -370,6 +370,9 @@ Save and Close keeps TIFF layers by default; explicit tiffLayers false is refused when it would lose layer data. TIFF color modes the writer cannot preserve are refused. Existing Folder save defaults are unchanged. +Save and Close assumes that other processes do not modify or replace its inputs +during the batch. The source-data checks do not provide concurrent-write protection. + Save and Close refuses format conversion, sources already open in the caller, and a result from a different document. An input must have an encoder for its exact extension; import-only/unknown/extensionless inputs require Save As and