Fix so that all artifact commands are returned regardless of caching (#…

…5005)

* Fix so that all artifact commands are returned regardless of caching

* Add some more docs and fix up old ones

src/wasm-lib/kcl/src/execution/mod.rs

@@ -2013,10 +2013,13 @@ impl ExecutorContext {
         // AND if we aren't in wasm it doesn't really matter.
         Ok(())
     }
-    // Given an old ast, old program memory and new ast, find the parts of the code that need to be
-    // re-executed.
-    // This function should never error, because in the case of any internal error, we should just pop
-    // the cache.
+    /// Given an old ast, old program memory and new ast, find the parts of the code that need to be
+    /// re-executed.
+    /// This function should never error, because in the case of any internal error, we should just pop
+    /// the cache.
+    ///
+    /// Returns `None` when there are no changes to the program, i.e. it is
+    /// fully cached.
     pub async fn get_changed_program(&self, info: CacheInformation) -> Option<CacheResult> {
         let Some(old) = info.old else {
             // We have no old info, we need to re-execute the whole thing.
@@ -2137,7 +2140,7 @@ impl ExecutorContext {
                 }
             }
             std::cmp::Ordering::Equal => {
-                // currently unreachable, but lets pretend like the code
+                // currently unreachable, but let's pretend like the code
                 // above can do something meaningful here for when we get
                 // to diffing and yanking chunks of the program apart.
 
@@ -2236,7 +2239,10 @@ impl ExecutorContext {
                 )
             })?;
         // Move the artifact commands to simplify cache management.
-        exec_state.global.artifact_commands = self.engine.take_artifact_commands();
+        exec_state
+            .global
+            .artifact_commands
+            .extend(self.engine.take_artifact_commands());
         let session_data = self.engine.get_session_data();
         Ok(session_data)
     }

src/wasm-lib/tests/executor/cache.rs

@@ -1,14 +1,18 @@
 //! Cache testing framework.
 
 use anyhow::Result;
-use kcl_lib::ExecError;
+use kcl_lib::{ExecError, ExecState};
 
+#[derive(Debug)]
 struct Variation<'a> {
     code: &'a str,
     settings: &'a kcl_lib::ExecutorSettings,
 }
 
-async fn cache_test(test_name: &str, variations: Vec<Variation<'_>>) -> Result<Vec<(String, image::DynamicImage)>> {
+async fn cache_test(
+    test_name: &str,
+    variations: Vec<Variation<'_>>,
+) -> Result<Vec<(String, image::DynamicImage, ExecState)>> {
     let first = variations
         .first()
         .ok_or_else(|| anyhow::anyhow!("No variations provided for test '{}'", test_name))?;
@@ -42,7 +46,7 @@ async fn cache_test(test_name: &str, variations: Vec<Variation<'_>>) -> Result<V
         // Save the snapshot.
         let path = crate::assert_out(&format!("cache_{}_{}", test_name, index), &img);
 
-        img_results.push((path, img));
+        img_results.push((path, img, exec_state.clone()));
 
         // Prepare the last state.
         old_ast_state = Some(kcl_lib::OldAstState {
@@ -216,3 +220,47 @@ async fn kcl_test_cache_change_highlight_edges_changes_visual() {
 
     assert!(first.1 != second.1);
 }
+
+#[tokio::test(flavor = "multi_thread")]
+async fn kcl_test_cache_add_line_preserves_artifact_commands() {
+    let code = r#"sketch001 = startSketchOn('XY')
+  |> startProfileAt([5.5229, 5.25217], %)
+  |> line([10.50433, -1.19122], %)
+  |> line([8.01362, -5.48731], %)
+  |> line([-1.02877, -6.76825], %)
+  |> line([-11.53311, 2.81559], %)
+  |> close(%)
+"#;
+    // Use a new statement; don't extend the prior pipeline.  This allows us to
+    // detect a prefix.
+    let code_with_extrude = code.to_owned()
+        + r#"
+extrude(4, sketch001)
+"#;
+
+    let result = cache_test(
+        "add_line_preserves_artifact_commands",
+        vec![
+            Variation {
+                code,
+                settings: &Default::default(),
+            },
+            Variation {
+                code: code_with_extrude.as_str(),
+                settings: &Default::default(),
+            },
+        ],
+    )
+    .await
+    .unwrap();
+
+    let first = result.first().unwrap();
+    let second = result.last().unwrap();
+
+    assert!(
+        first.2.global.artifact_commands.len() < second.2.global.artifact_commands.len(),
+        "Second should have all the artifact commands of the first, plus more. first={:?}, second={:?}",
+        first.2.global.artifact_commands.len(),
+        second.2.global.artifact_commands.len()
+    );
+}