[yongyin-leon]

Summary

A locally created task can run a script that calls task.run again. Task steps
reject direct task.* commands, but this indirect path repeatedly invokes the
command function pointers until the process overflows its stack. The outer
Session::run transaction does not bound that call chain.

Add a session-local task-running guard, following the existing notebook guard.
Indirect re-entry returns an error with the task-step and script-line context;
the marker clears on success, error or a caught panic and stays active across
project replacement. Command IDs, parameters, project files and imported-task
restrictions stay unchanged.

This extends the existing task-to-task restriction to indirect calls: even a
finite Task A → Script → Task B chain is refused. Ordinary Script → Task and
Task → nonrecursive Script remain supported. The existing outer undo/rollback
and notebook cell-error behavior are preserved; this adds no atomicity guarantee
for external I/O.

Minimal reproduction using public commands:

task.new {"name":"Loop","steps":[{"title":"again","command":"script.run","params":{"script":"task.run {\"task\":\"Loop\"}"}}]}
task.run {"task":"Loop"}

Testing

• Windows: 8 new focused regressions cover re-entry, finite scripts and one undo
  step, project/edit-count rollback, existing refusals, panic cleanup, project
  replacement and notebook paths. Also ran 8 existing related regressions,
  including hostile_params_never_panic.
• A disposable-process probe with a 1 MiB thread stack and a 10-second timeout
  reproduced baseline stack overflow (0xC00000FD). After the fix, that case
  returns the expected command error; it and three control cases exit normally.
• Engine Clippy across all targets with -D warnings, Engine formatting,
  cargo xtask layers and the Engine wasm32 library check pass.

These results apply to the patch based on 096e60e0. Full workspace CI,
macOS/Linux runtime tests and the complete Web App build have not been run.
No UI or dependency changes.

Related issue

None. No directly overlapping open issue or pull request was found in the latest
check. Maintainer review is welcome on extending the existing no-nested-tasks
policy to indirect calls, including finite chains between distinct tasks.

Summary by CodeRabbit

• Bug Fixes
  • Prevented a running task from starting another task through nested calls, scripts, or notebook execution.
  • Task execution can proceed again after an error or panic, and failed re-entry rolls back the active project transaction.
  • Finite task and script composition remains undoable as a single operation.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 86cabc49-7f6d-4ce9-a9e0-c386c1b8cff8

Commits

Reviewing files that changed from the base of the PR and between 342b2b2 and 1d32db1.

Files selected for processing: 4

• crates/engine/src/cmd/items.rs
• crates/engine/src/lib.rs
• crates/engine/src/tests.rs
• crates/engine/src/tests/task_reentry.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Walkthrough

Task execution now uses a session guard to reject re-entry, including re-entry through scripts and notebooks. The guard is cleared after success, errors, and caught panics. Tests cover re-entry, validation, rollback, project replacement, and undo behavior.

Changes: Task Re-entry Guard

Layer / File(s)	Summary
Session guard and task execution crates/engine/src/lib.rs, crates/engine/src/cmd/items.rs	Session now tracks whether a task is running. run_task rejects re-entry, catches panics, clears the guard after execution, and delegates step execution to run_task_steps.
Re-entry, cleanup, and rollback tests crates/engine/src/tests.rs, crates/engine/src/tests/task_reentry.rs	The registered tests cover script and notebook re-entry, task validation, panic cleanup, project replacement, transaction rollback, and undo and redo behavior.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant run_task
  participant run_task_steps
  participant ScriptOrNotebook
  run_task->>run_task_steps: Execute task steps
  run_task_steps->>ScriptOrNotebook: Run script or notebook path
  ScriptOrNotebook->>run_task: Attempt nested task execution
  run_task-->>ScriptOrNotebook: Return already-running error
  run_task->>run_task: Clear task_running after execution

Loading

Suggested labels: ci:windows

Suggested reviewers: giswqs

Merge Risk: Minimal · up to 1d32d

No merge-blocking issue was identified; merge after normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly and concisely identifies the main change: rejecting indirect task re-entry in the engine.
Description check		The description includes the required Summary, Testing, and Related issue sections. It explains the problem, implementation, compatibility constraints, test coverage, and known testing limits.
Docstring Coverage		Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. (1 skipped: 1 …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. (1 skipped: 1 too large.)

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks the task is clear,
Then hops through steps with care.
If scripts call back, the guard says no,
And clears its mark when runs are through.
Undo keeps the map in view.}

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Code review

I read the diff and the surrounding run_task, notebook.run and Session::run code. I found no bugs, security problems or guideline violations. I did not build the project or run the tests; I have no Bash tool here.

What I checked

• Guard correctness: run_task sets task_running, runs run_task_steps under catch_unwind, then clears the flag. It mirrors the existing notebook_running guard in cmd/notebook.rs. The flag is cleared on Ok, on Err and on a caught panic. No early return or ? sits between the set and the clear, because the body moved into run_task_steps.
• Indirect paths: Task → script.run → task.run and Task → notebook.run → task.run both reach run_task and are refused. Model steps already exclude task.* (items.rs:354), so I found no other route back into run_task.
• Transaction behavior: the error propagates to Session::run as an Err, which restores the project and undo state as for any failing command. The rollback test covers this.
• State handling: task_running is pub(crate), initialised to false, and not part of the project or undo snapshots. It survives project.new mid-task, and task_reentry_guard_survives_project_replacement tests that.
• AGENTS.md: the non-test code has no unwrap, expect or panic!, no unsafe, and nothing that affects wasm. The new test file uses unwrap, which is allowed in tests. The change adds no UI strings, dependencies or assets.
• Tests: the eight new regressions cover re-entry, a finite script with one undo step, rollback, existing refusals, panic cleanup, project replacement and notebooks. They are thorough.

Quality (minor, low confidence)

• Behavior change (about 30%): a panic that escapes run_task_steps, such as one in the lookup or enabled code outside the per-step catch, now returns "the task failed unexpectedly" from run_task. Before, the outer Session::run catch produced the message. The message differs, but the result is still a clean error and the test pins it. No action needed.
• Scope of the refusal (about 40%): the PR deliberately also refuses finite chains such as Task A → Script → Task B. That is a policy extension, and the description asks for maintainer input on it. Only the maintainer can settle that question.
• Cosmetic (about 20%): the error text says "a step can't run tasks", while the direct case says "step N would run another task". Using the same wording would make the two refusals easier to search for. This is optional.

Change size

Compared: the whole PR against main (merge base 096e60e to PR head 1d32db1)

Metric	Value
Files changed	4 (1 added, 3 modified, 0 removed, 0 renamed)
Size before	1.10 MB (1,151,014 B)
Size after	1.11 MB (1,159,770 B)
Total size change	+8.6 KB (+8,756 B)

File	Before	After	Change
crates/engine/src/tests/task_reentry.rs	0 B	7.6 KB	+7.6 KB
crates/engine/src/cmd/items.rs	52.2 KB	52.8 KB	+713 B
crates/engine/src/lib.rs	65.1 KB	65.4 KB	+248 B
crates/engine/src/tests.rs	1006.8 KB	1006.9 KB	+53 B

[github-actions]

📖 Docs preview

Item	Value
Preview	https://preview.rustgis.app/pr-941/
Web app	https://preview.rustgis.app/pr-941/app/
Commit	1d32db1

GitHub Pages takes a minute or two to publish a new commit.