fix(mcp): merge parsed blocks to prevent closure panic (nushell#16884)

When executing pipelines with closures, the evaluator would panic with
"internal error: missing block" at engine_state.rs:903.

After parsing, blocks (including closures) exist only in the
StateWorkingSet. Without calling `working_set.render()` and
`engine_state.merge_delta()`, these blocks are never registered in the
engine state. When eval_block() tries to execute a closure, it calls
`engine_state.get_block(id)` which panics because the block ID doesn't
exist.

Simply adding merge_delta would pollute the shared engine state across
evaluations. To maintain isolation, we now:

1. Store a pristine EngineState (not Arc<EngineState>) in the Evaluator
2. Clone it for each evaluation (cheap due to internal Arcs)
3. Merge the parsed delta into the cloned state
4. Evaluate with the isolated, fully-populated state

This ensures each evaluation is independent while making all parsed
blocks available during execution.

Author: cablehead

crates/nu-mcp/src/evaluation.rs

@@ -17,15 +17,34 @@ use serde::{Deserialize, Serialize};
 
 const MAX_PAGE_SIZE: usize = 16 * 1024;
 
+/// Evaluates Nushell code in isolated contexts for MCP.
+///
+/// # Architecture
+///
+/// The evaluator maintains a pristine `EngineState` template. Each evaluation:
+/// 1. Clones the engine state (cheap due to internal `Arc`s)
+/// 2. Parses code into a `Block` and gets a `StateDelta` via `working_set.render()`
+/// 3. **Merges the delta** via `engine_state.merge_delta()` to register blocks
+/// 4. Evaluates the block with the merged state
+///
+/// Step 3 is critical: parsed blocks (including closures) are only stored in the
+/// `StateWorkingSet` initially. Without merging, `eval_block()` will panic with
+/// "missing block" when it tries to execute closures or other block references.
+///
+/// # Isolation
+///
+/// Each evaluation gets its own cloned state, so variables/definitions from one
+/// evaluation don't persist to the next.
+///
+/// This architecture also enables future parallel evaluation of multiple pipelines.
 pub struct Evaluator {
-    engine_state: Arc<EngineState>,
+    engine_state: EngineState,
     cache: Cache<String, Arc<PipelineBuffer>>,
 }
 
 impl Evaluator {
-    pub fn new(engine_state: Arc<EngineState>) -> Self {
+    pub fn new(mut engine_state: EngineState) -> Self {
         // Disable ANSI coloring for MCP - it's a computer-to-computer protocol
-        let mut engine_state = Arc::unwrap_or_clone(engine_state);
         let mut config = Config::clone(engine_state.get_config());
         config.use_ansi_coloring = UseAnsiColoring::False;
         engine_state.set_config(config);
@@ -35,7 +54,7 @@ impl Evaluator {
             .time_to_live(std::time::Duration::from_secs(300))
             .build();
         Self {
-            engine_state: Arc::new(engine_state),
+            engine_state,
             cache,
         }
     }
@@ -45,28 +64,39 @@ impl Evaluator {
         {
             pipeline_buffer
         } else {
-            let engine_state = Arc::clone(&self.engine_state);
-            let mut working_set = StateWorkingSet::new(&engine_state);
-
-            // Parse the source code
-            let block = parse(&mut working_set, None, nu_source.as_bytes(), false);
-
-            // Check for parse errors
-            if let Some(err) = working_set.parse_errors.first() {
-                return Err(McpError::internal_error(
-                    nu_protocol::format_cli_error(&working_set, err, None),
-                    None,
-                ));
-            }
+            // Clone the pristine engine state for this evaluation
+            let mut engine_state = self.engine_state.clone();
 
-            // Check for compile errors (IR compilation errors)
-            // These are caught during the parse/compile phase, before evaluation
-            if let Some(err) = working_set.compile_errors.first() {
-                return Err(McpError::internal_error(
-                    nu_protocol::format_cli_error(&working_set, err, None),
-                    None,
-                ));
-            }
+            let (block, delta) = {
+                let mut working_set = StateWorkingSet::new(&engine_state);
+
+                // Parse the source code
+                let block = parse(&mut working_set, None, nu_source.as_bytes(), false);
+
+                // Check for parse errors
+                if let Some(err) = working_set.parse_errors.first() {
+                    return Err(McpError::internal_error(
+                        nu_protocol::format_cli_error(&working_set, err, None),
+                        None,
+                    ));
+                }
+
+                // Check for compile errors (IR compilation errors)
+                // These are caught during the parse/compile phase, before evaluation
+                if let Some(err) = working_set.compile_errors.first() {
+                    return Err(McpError::internal_error(
+                        nu_protocol::format_cli_error(&working_set, err, None),
+                        None,
+                    ));
+                }
+
+                (block, working_set.render())
+            };
+
+            // Merge the parsed blocks into the engine state so they're available during eval
+            engine_state
+                .merge_delta(delta)
+                .map_err(|e| shell_error_to_mcp_error(e, &engine_state))?;
 
             // Eval the block with the input
             let mut stack = Stack::new().collect_value();
@@ -78,7 +108,7 @@ impl Evaluator {
             )
             .map_err(|e| shell_error_to_mcp_error(e, &engine_state))?;
 
-            let r = Arc::new(self.process_pipeline(output)?);
+            let r = Arc::new(self.process_pipeline(output, &engine_state)?);
             self.cache.insert(nu_source.to_string(), Arc::clone(&r));
             r
         };
@@ -104,8 +134,8 @@ impl Evaluator {
     fn process_pipeline(
         &self,
         pipeline_execution_data: PipelineExecutionData,
+        engine_state: &EngineState,
     ) -> Result<PipelineBuffer, McpError> {
-        let engine_state = Arc::clone(&self.engine_state);
         let span = pipeline_execution_data.span();
         // todo - this bystream use case won't work
         if let PipelineData::ByteStream(_stream, ..) = pipeline_execution_data.body {
@@ -128,7 +158,7 @@ impl Evaluator {
             let mut last_page_index = 0;
             for item in pipeline_execution_data.body {
                 let out = if let Value::Error { error, .. } = item {
-                    return Err(shell_error_to_mcp_error(*error, &engine_state));
+                    return Err(shell_error_to_mcp_error(*error, engine_state));
                 } else {
                     item.to_expanded_string("\n", config) + "\n"
                 };
@@ -147,7 +177,7 @@ impl Evaluator {
                 last_index = buffer.len();
 
                 write_all_and_flush(out, &mut buffer, "mcp_output", span, engine_state.signals())
-                    .map_err(|e| shell_error_to_mcp_error(e, &engine_state))?;
+                    .map_err(|e| shell_error_to_mcp_error(e, engine_state))?;
             }
             if pages.is_empty() {
                 pages.push(Page::new(0..buffer.len(), page_total));
@@ -238,7 +268,7 @@ mod tests {
             Some(Span::test_data()),
             false,
         )?;
-        let evaluator = Evaluator::new(Arc::new(engine_state));
+        let evaluator = Evaluator::new(engine_state);
         let result = evaluator.eval(&nuon_values, None)?;
         assert_eq!(result.summary.total, 3);
         assert!(result.summary.next_cursor.is_some());
@@ -248,7 +278,7 @@ mod tests {
     #[test]
     fn test_evaluator_parse_error_message() {
         let engine_state = create_default_context();
-        let evaluator = Evaluator::new(Arc::new(engine_state));
+        let evaluator = Evaluator::new(engine_state);
 
         // Invalid syntax - missing closing bracket
         let result = evaluator.eval("let x = [1, 2, 3", None);
@@ -285,7 +315,7 @@ mod tests {
     #[test]
     fn test_evaluator_compile_error_message() {
         let engine_state = create_default_context();
-        let evaluator = Evaluator::new(Arc::new(engine_state));
+        let evaluator = Evaluator::new(engine_state);
 
         // This will trigger a compile error (IR compilation)
         // because create_default_context doesn't fully compile blocks for pipelines
@@ -311,7 +341,7 @@ mod tests {
     #[test]
     fn test_evaluator_runtime_error_message() {
         let engine_state = create_default_context();
-        let evaluator = Evaluator::new(Arc::new(engine_state));
+        let evaluator = Evaluator::new(engine_state);
 
         // Use error make to create a runtime error with custom message and labels
         let result = evaluator.eval(
@@ -332,4 +362,27 @@ mod tests {
             "Error message should contain rich formatting and custom error message, but got: {err_msg}"
         );
     }
+
+    #[test]
+    fn test_closure_in_pipeline() {
+        // Use add_default_context which includes basic language commands
+        let engine_state = {
+            let engine_state = nu_protocol::engine::EngineState::new();
+            nu_cmd_lang::add_default_context(engine_state)
+        };
+        let evaluator = Evaluator::new(engine_state);
+
+        // Test with a simple closure using 'do' which is a lang command
+        // The closure { ... } creates a block that must be available when eval_block runs
+        // This tests that merge_delta properly registers blocks in the engine_state
+        let result = evaluator.eval(r#"do { |x| $x + 1 } 41"#, None);
+
+        assert!(
+            result.is_ok(),
+            "Pipeline with closure should succeed: {:?}",
+            result.err()
+        );
+        let output = result.unwrap();
+        assert_eq!(output.summary.total, 1, "Should have 1 result");
+    }
 }

crates/nu-mcp/src/server.rs

@@ -24,11 +24,11 @@ pub struct NushellMcpServer {
 #[tool_router]
 impl NushellMcpServer {
     pub fn new(mut engine_state: EngineState) -> Self {
+        // Configure the engine state for MCP
         if let Some(config) = Arc::get_mut(&mut engine_state.config) {
             config.use_ansi_coloring = UseAnsiColoring::False;
             config.color_config.clear();
         }
-        let engine_state = Arc::new(engine_state);
         NushellMcpServer {
             tool_router: Self::tool_router(),
             evaluator: Evaluator::new(engine_state),