From 1d32db1749bfdd3b4fdeec04fd65f0a55e5b9113 Mon Sep 17 00:00:00 2001 From: yongyin-leon <269641072+yongyin-leon@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:16:21 +0800 Subject: [PATCH] fix(engine): reject indirect task re-entry --- crates/engine/src/cmd/items.rs | 16 ++- crates/engine/src/lib.rs | 4 + crates/engine/src/tests.rs | 3 + crates/engine/src/tests/task_reentry.rs | 160 ++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 crates/engine/src/tests/task_reentry.rs diff --git a/crates/engine/src/cmd/items.rs b/crates/engine/src/cmd/items.rs index ed4aa8bcc..a847a21e8 100644 --- a/crates/engine/src/cmd/items.rs +++ b/crates/engine/src/cmd/items.rs @@ -664,8 +664,22 @@ fn task_mut<'a>(s: &'a mut Session, p: &Value) -> Result<&'a mut Task, String> { } } -/// Runs a task's steps (or one of them), stopping at the first that fails. +/// Runs a task without allowing its steps to re-enter tasks indirectly through scripts or +/// notebooks. Clear the session marker on errors and caught panics as well as success. fn run_task(s: &mut Session, p: &Value) -> CmdResult { + if s.task_running { + return Err("a task is already running (a step can't run tasks, even through a script or notebook)".into()); + } + s.task_running = true; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| run_task_steps(s, p))) + .unwrap_or_else(|_| Err("the task failed unexpectedly".into())); + s.task_running = false; + result +} + +/// Executes the selected task steps inside the caller's transaction and re-entry guard, +/// retaining the existing direct task ban and imported-task restrictions. +fn run_task_steps(s: &mut Session, p: &Value) -> CmdResult { let t = task_ref(s, p)?.clone(); // `"step": null` runs them all, as no step does. let steps: Vec<(usize, TaskStep)> = match p.get("step").filter(|v| !v.is_null()) { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 0b716e92c..5496c90ca 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -538,6 +538,9 @@ pub struct Session { /// Tasks made in this session (lower-cased names). Others came from files, and their steps /// may not touch files when run. pub trusted_tasks: std::collections::HashSet, + /// A task is executing: its steps may not start another task, even through scripts. + /// Kept across project replacement until the active call returns; never part of undo. + pub(crate) task_running: bool, /// Models made in this session (lower-cased names); the steps of others may not touch files. pub trusted_models: std::collections::HashSet, /// What the last model run did, node by node (`model::RunReport`), kept even when the run @@ -725,6 +728,7 @@ impl Session { keep_tool: false, active_layout: None, trusted_tasks: Default::default(), + task_running: false, trusted_models: Default::default(), model_run: None, project_starts: 0, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 86a107c7e..2bbc430e5 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2,6 +2,9 @@ use serde_json::{Value, json}; use crate::{Project, Session, sample}; +#[path = "tests/task_reentry.rs"] +mod task_reentry; + #[cfg(not(target_arch = "wasm32"))] #[path = "tests/remembered_database.rs"] mod remembered_database; diff --git a/crates/engine/src/tests/task_reentry.rs b/crates/engine/src/tests/task_reentry.rs new file mode 100644 index 000000000..91560f50d --- /dev/null +++ b/crates/engine/src/tests/task_reentry.rs @@ -0,0 +1,160 @@ +//! Task re-entry stays bounded without changing finite scripts or outer transactions. +use super::*; + +/// Create a locally trusted task through the public API, keeping the trust policy in the test. +fn task(s: &mut Session, name: &str, steps: Value) { + s.run("task.new", &json!({"name": name, "steps": steps})).unwrap(); +} + +/// A script step that indirectly enters the named task, including a different task. +fn reenter(name: &str) -> Value { + json!({"title":"again", "command":"script.run", "params":{"script":format!("task.run {}", json!({"task":name}))}}) +} + +/// A task cannot re-enter itself or another task through a script; cleanup permits a later run. +#[test] +fn task_reentry_through_script_is_refused_and_cleared() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + task(&mut s, "Loop", json!([reenter("Loop")])); + task(&mut s, "Other", json!([])); + task(&mut s, "Outer", json!([reenter("Other")])); + for name in ["Loop", "Outer"] { + let error = s.run("task.run", &json!({"task":name})).unwrap_err(); + assert!(error.contains("step 1") && error.contains("line 1") && error.contains("already running"), "{error}"); + assert!(!s.task_running); + assert!(s.run("task.run", &json!({"task":"Other"})).is_ok()); + } +} + +/// Successful script/task composition still changes the project in one undoable operation. +#[test] +fn task_reentry_guard_keeps_finite_scripts_and_single_undo() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + let before = s.map().name.clone(); + task(&mut s, "Finite", json!([{"title":"rename", "command":"script.run", "params":{"script":"map.rename {\"name\":\"Changed\"}"}}])); + let undo = s.undo_depth(); + s.run("script.run", &json!({"script":"task.run {\"task\":\"Finite\"}"})).unwrap(); + assert_eq!(s.map().name, "Changed"); + assert_eq!(s.undo_depth(), undo + 1); + assert!(!s.task_running); + s.run("edit.undo", &Value::Null).unwrap(); + assert_eq!(s.map().name, before); + s.run("edit.redo", &Value::Null).unwrap(); + assert_eq!(s.map().name, "Changed"); + assert!(!s.task_running, "undo/redo do not restore execution state"); +} + +/// A refusal after an earlier project mutation restores the project without an extra undo. +#[test] +fn task_reentry_failure_rolls_back_outer_project_transaction() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + s.registry.commands.push(crate::CommandSpec::new("test.countEdit", "Count Edit", "Test", |s, _| { + s.edit_counts.entry(7).or_default().modified += 1; + Ok(Value::Null) + })); + task( + &mut s, + "Loop", + json!([ + {"title":"rename", "command":"map.rename", "params":{"name":"Changed"}}, + {"title":"count", "command":"test.countEdit"}, + reenter("Loop") + ]), + ); + let undo = s.undo_depth(); + s.dirty = false; + let before = serde_json::to_value(&s.project).unwrap(); + assert!(s.run("task.run", &json!({"task":"Loop"})).unwrap_err().contains("already running")); + assert_eq!(serde_json::to_value(&s.project).unwrap(), before); + assert_eq!(s.undo_depth(), undo); + assert!(s.edit_counts.is_empty()); + assert!(!s.dirty && !s.task_running); +} + +/// Invalid parameters and imported/direct-step refusals leave the guard usable and trust intact. +#[test] +fn task_reentry_guard_preserves_existing_refusals() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + task(&mut s, "Direct", json!([{"title":"again", "command":"task.run", "params":{"task":"Direct"}}])); + s.run("task.new", &json!({"name":"Imported", "imported":true, "steps":[reenter("Direct")]})).unwrap(); + for (params, message) in [ + (json!({"task":"missing"}), "no task"), + (json!({"task":"Direct", "step":999}), "steps"), + (json!({"task":"Direct"}), "another task"), + (json!({"task":"Imported"}), "from a file"), + ] { + let error = s.run("task.run", ¶ms).unwrap_err(); + assert!(error.contains(message), "{error}"); + assert!(!s.task_running); + } +} + +/// Even a condition panic outside the step's own catch clears the outer task marker. +#[test] +fn task_reentry_guard_clears_after_condition_panic() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + s.registry.commands.push( + crate::CommandSpec::new("test.conditionPanic", "Panic", "Test", |_, _| Ok(Value::Null)).when(|_| panic!("test condition panic")), + ); + task(&mut s, "Panic", json!([{"title":"panic", "command":"test.conditionPanic"}])); + let error = s.run("task.run", &json!({"task":"Panic"})).unwrap_err(); + assert!(error.contains("failed unexpectedly"), "{error}"); + assert!(!s.task_running); + task(&mut s, "Fine", json!([])); + s.run("task.run", &json!({"task":"Fine"})).unwrap(); +} + +/// Project replacement must not clear the active marker and allow a newly created task to enter. +#[test] +fn task_reentry_guard_survives_project_replacement() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + let script = + format!("project.new {{\"name\":\"Replacement\"}}\ntask.new {}\ntask.run {{\"task\":\"New\"}}", json!({"name":"New", "steps":[]})); + task(&mut s, "Original", json!([{"title":"replace", "command":"script.run", "params":{"script":script}}])); + let before = serde_json::to_value(&s.project).unwrap(); + assert!(s.run("task.run", &json!({"task":"Original"})).unwrap_err().contains("already running")); + assert_eq!(serde_json::to_value(&s.project).unwrap(), before); + assert!(!s.task_running); +} + +/// A step-body panic also releases the marker, preserving the existing contextual task error. +#[test] +fn task_reentry_guard_clears_after_step_panic() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + s.registry.commands.push(crate::CommandSpec::new("test.stepPanic", "Panic", "Test", |_, _| panic!("test step panic"))); + task(&mut s, "Panic", json!([{"title":"panic", "command":"test.stepPanic"}])); + let error = s.run("task.run", &json!({"task":"Panic"})).unwrap_err(); + assert!(error.contains("step 1") && error.contains("failed unexpectedly"), "{error}"); + assert!(!s.task_running); + task(&mut s, "Fine", json!([])); + s.run("task.run", &json!({"task":"Fine"})).unwrap(); +} + +/// Notebook cells keep their error-reporting contract when a task would indirectly enter tasks. +#[test] +fn task_reentry_guard_covers_notebook_paths() { + isolate_plugins(); + let mut s = Session::new(Project::default()); + task(&mut s, "Loop", json!([reenter("Loop")])); + s.run("notebook.new", &json!({"name":"Notebook"})).unwrap(); + s.run("notebook.addCell", &json!({"notebook":"Notebook", "source":"task.run {\"task\":\"Loop\"}"})).unwrap(); + s.run("notebook.run", &json!({"notebook":"Notebook"})).unwrap(); + assert!(s.project.notebooks[0].cells.iter().any(|c| c.error && c.output.as_deref().is_some_and(|o| o.contains("already running")))); + task(&mut s, "ViaNotebook", json!([{"title":"notebook", "command":"notebook.run", "params":{"notebook":"Notebook"}}])); + // A second run must produce its own cell error, rather than pass on the first run's output. + for cell in &mut s.project.notebooks[0].cells { + cell.output = None; + cell.error = false; + } + let result = s.run("task.run", &json!({"task":"ViaNotebook"})).unwrap(); + assert!(result["ran"][0]["result"]["error"].is_u64(), "the task keeps the notebook's cell-error result: {result}"); + assert!(s.project.notebooks[0].cells.iter().any(|c| c.error && c.output.as_deref().is_some_and(|o| o.contains("already running")))); + assert!(!s.task_running && !s.notebook_running); +}