refactor(builtins): replace magic variable hack with BuiltinSideEffect enum

crates/bashkit/src/builtins/mod.rs

@@ -198,6 +198,9 @@ use crate::error::Result;
 use crate::fs::FileSystem;
 use crate::interpreter::ExecResult;
 
+// Re-export for use by builtins
+pub use crate::interpreter::BuiltinSideEffect;
+
 /// Resolve a path relative to the current working directory.
 ///
 /// If the path is absolute, returns it unchanged.

crates/bashkit/src/builtins/read.rs

@@ -2,7 +2,7 @@
 
 use async_trait::async_trait;
 
-use super::{Builtin, Context};
+use super::{Builtin, BuiltinSideEffect, Context};
 use crate::error::Result;
 use crate::interpreter::{ExecResult, is_internal_variable};
 
@@ -143,12 +143,12 @@ impl Builtin for Read {
             if is_internal_variable(arr_name) {
                 return Ok(ExecResult::ok(String::new()));
             }
-            // Store as _ARRAY_<name>_<idx> for the interpreter to pick up
-            ctx.variables.insert(
-                format!("_ARRAY_READ_{}", arr_name),
-                words.join("\x1F"), // unit separator as delimiter
-            );
-            return Ok(ExecResult::ok(String::new()));
+            let mut result = ExecResult::ok(String::new());
+            result.side_effects.push(BuiltinSideEffect::SetArray {
+                name: arr_name.to_string(),
+                elements: words.iter().map(|w| w.to_string()).collect(),
+            });
+            return Ok(result);
         }
 
         // If no variable names given, use REPLY
@@ -409,9 +409,14 @@ mod tests {
         );
         let result = Read.execute(ctx).await.unwrap();
         assert_eq!(result.exit_code, 0);
-        let stored = variables.get("_ARRAY_READ_ARR").unwrap();
-        let parts: Vec<&str> = stored.split('\x1F').collect();
-        assert_eq!(parts, vec!["one", "two", "three"]);
+        assert_eq!(result.side_effects.len(), 1);
+        match &result.side_effects[0] {
+            BuiltinSideEffect::SetArray { name, elements } => {
+                assert_eq!(name, "ARR");
+                assert_eq!(elements, &["one", "two", "three"]);
+            }
+            _ => panic!("Expected SetArray side effect"),
+        }
     }
 
     #[tokio::test]
@@ -429,7 +434,11 @@ mod tests {
         );
         let result = Read.execute(ctx).await.unwrap();
         assert_eq!(result.exit_code, 0);
-        assert!(variables.contains_key("_ARRAY_READ_REPLY"));
+        assert_eq!(result.side_effects.len(), 1);
+        match &result.side_effects[0] {
+            BuiltinSideEffect::SetArray { name, .. } => assert_eq!(name, "REPLY"),
+            _ => panic!("Expected SetArray side effect"),
+        }
     }
 
     // ==================== combined flags ====================

crates/bashkit/src/builtins/vars.rs

@@ -4,7 +4,7 @@
 
 use async_trait::async_trait;
 
-use super::{Builtin, Context};
+use super::{Builtin, BuiltinSideEffect, Context};
 use crate::error::Result;
 use crate::interpreter::{ExecResult, is_internal_variable};
 
@@ -93,17 +93,9 @@ fn format_set_plus_o(variables: &std::collections::HashMap<String, String>) -> S
 }
 
 impl Set {
-    /// Encode positional parameters as count\x1Farg1\x1Farg2... for the interpreter.
-    fn encode_positional(
-        variables: &mut std::collections::HashMap<String, String>,
-        positional: &[&str],
-    ) {
-        let mut encoded = positional.len().to_string();
-        for p in positional {
-            encoded.push('\x1F');
-            encoded.push_str(p);
-        }
-        variables.insert("_SET_POSITIONAL".to_string(), encoded);
+    /// Create a SetPositional side effect.
+    fn positional_effect(positional: &[&str]) -> BuiltinSideEffect {
+        BuiltinSideEffect::SetPositional(positional.iter().map(|s| s.to_string()).collect())
     }
 }
 
@@ -121,13 +113,14 @@ impl Builtin for Set {
             return Ok(ExecResult::ok(output));
         }
 
+        let mut side_effects = Vec::new();
         let mut i = 0;
         while i < ctx.args.len() {
             let arg = &ctx.args[i];
             if arg == "--" {
                 // Everything after `--` becomes positional parameters.
                 let positional: Vec<&str> = ctx.args[i + 1..].iter().map(|s| s.as_str()).collect();
-                Self::encode_positional(ctx.variables, &positional);
+                side_effects.push(Self::positional_effect(&positional));
                 break;
             } else if (arg.starts_with('-') || arg.starts_with('+'))
                 && arg.len() > 1
@@ -174,13 +167,15 @@ impl Builtin for Set {
             } else {
                 // Non-flag arg: this and everything after become positional params
                 let positional: Vec<&str> = ctx.args[i..].iter().map(|s| s.as_str()).collect();
-                Self::encode_positional(ctx.variables, &positional);
+                side_effects.push(Self::positional_effect(&positional));
                 break;
             }
             i += 1;
         }
 
-        Ok(ExecResult::ok(String::new()))
+        let mut result = ExecResult::ok(String::new());
+        result.side_effects = side_effects;
+        Ok(result)
     }
 }
 
@@ -193,12 +188,11 @@ impl Builtin for Shift {
         // Number of positions to shift (default 1)
         let n: usize = ctx.args.first().and_then(|s| s.parse().ok()).unwrap_or(1);
 
-        // In real bash, this shifts the positional parameters
-        // For now, we store the shift count for the interpreter to handle
-        ctx.variables
-            .insert("_SHIFT_COUNT".to_string(), n.to_string());
-
-        Ok(ExecResult::ok(String::new()))
+        let mut result = ExecResult::ok(String::new());
+        result
+            .side_effects
+            .push(BuiltinSideEffect::ShiftPositional(n));
+        Ok(result)
     }
 }
 

crates/bashkit/src/interpreter/mod.rs

@@ -16,7 +16,7 @@ mod state;
 
 #[allow(unused_imports)]
 pub use jobs::{JobTable, SharedJobTable};
-pub use state::{ControlFlow, ExecResult};
+pub use state::{BuiltinSideEffect, ControlFlow, ExecResult};
 // Re-export snapshot type for public API
 
 use std::collections::{HashMap, HashSet};
@@ -1054,6 +1054,7 @@ impl Interpreter {
             stderr_truncated,
             final_env,
             events,
+            ..Default::default()
         })
     }
 
@@ -4529,59 +4530,8 @@ impl Interpreter {
                 }
             };
 
-            // Post-process: read -a populates array from marker variable
-            let markers: Vec<(String, String)> = self
-                .variables
-                .iter()
-                .filter(|(k, _)| k.starts_with("_ARRAY_READ_"))
-                .map(|(k, v)| (k.clone(), v.clone()))
-                .collect();
-            for (marker, value) in markers {
-                let arr_name = marker.strip_prefix("_ARRAY_READ_").unwrap();
-                let mut arr = HashMap::new();
-                for (i, word) in value.split('\x1F').enumerate() {
-                    if !word.is_empty() {
-                        arr.insert(i, word.to_string());
-                    }
-                }
-                self.insert_array_checked(arr_name.to_string(), arr);
-                self.variables.remove(&marker);
-            }
-
-            // Post-process: shift builtin updates positional parameters
-            if let Some(shift_str) = self.variables.remove("_SHIFT_COUNT") {
-                let n: usize = shift_str.parse().unwrap_or(1);
-                if let Some(frame) = self.call_stack.last_mut() {
-                    if n <= frame.positional.len() {
-                        frame.positional.drain(..n);
-                    } else {
-                        frame.positional.clear();
-                    }
-                }
-            }
-
-            // Post-process: `set --` replaces positional parameters
-            // Encoded as count\x1Farg1\x1Farg2... to preserve empty args.
-            if let Some(encoded) = self.variables.remove("_SET_POSITIONAL") {
-                let parts: Vec<&str> = encoded.splitn(2, '\x1F').collect();
-                let count: usize = parts[0].parse().unwrap_or(0);
-                let new_positional: Vec<String> = if count == 0 {
-                    Vec::new()
-                } else if parts.len() > 1 {
-                    parts[1].split('\x1F').map(|s| s.to_string()).collect()
-                } else {
-                    Vec::new()
-                };
-                if let Some(frame) = self.call_stack.last_mut() {
-                    frame.positional = new_positional;
-                } else {
-                    self.call_stack.push(CallFrame {
-                        name: String::new(),
-                        locals: HashMap::new(),
-                        positional: new_positional,
-                    });
-                }
-            }
+            // Process structured side effects from builtins
+            self.apply_builtin_side_effects(&result);
 
             // Handle output redirections
             return self.apply_redirections(result, &command.redirects).await;
@@ -6051,6 +6001,7 @@ impl Interpreter {
                         git_client: self.git_client.as_ref(),
                     };
                     let mut result = builtin.execute(ctx).await?;
+                    self.apply_builtin_side_effects(&result);
                     result = self.apply_redirections(result, redirects).await?;
                     Ok(result)
                 } else {
@@ -6596,6 +6547,43 @@ impl Interpreter {
         Ok(stdin)
     }
 
+    /// Process structured side effects from builtin execution.
+    fn apply_builtin_side_effects(&mut self, result: &ExecResult) {
+        for effect in &result.side_effects {
+            match effect {
+                builtins::BuiltinSideEffect::SetArray { name, elements } => {
+                    let mut arr = HashMap::new();
+                    for (i, word) in elements.iter().enumerate() {
+                        if !word.is_empty() {
+                            arr.insert(i, word.clone());
+                        }
+                    }
+                    self.insert_array_checked(name.clone(), arr);
+                }
+                builtins::BuiltinSideEffect::ShiftPositional(n) => {
+                    if let Some(frame) = self.call_stack.last_mut() {
+                        if *n <= frame.positional.len() {
+                            frame.positional.drain(..*n);
+                        } else {
+                            frame.positional.clear();
+                        }
+                    }
+                }
+                builtins::BuiltinSideEffect::SetPositional(new_positional) => {
+                    if let Some(frame) = self.call_stack.last_mut() {
+                        frame.positional = new_positional.clone();
+                    } else {
+                        self.call_stack.push(CallFrame {
+                            name: String::new(),
+                            locals: HashMap::new(),
+                            positional: new_positional.clone(),
+                        });
+                    }
+                }
+            }
+        }
+    }
+
     /// Apply output redirections to command output
     async fn apply_redirections(
         &mut self,

crates/bashkit/src/interpreter/state.rs

@@ -13,6 +13,22 @@ pub enum ControlFlow {
     Return(i32),
 }
 
+/// Structured side-effect channel for builtins that need to communicate
+/// state changes back to the interpreter.
+///
+/// Replaces magic-prefixed variables (`_SHIFT_COUNT`, `_SET_POSITIONAL`,
+/// `_ARRAY_READ_*`) with a typed enum. The interpreter reads these from
+/// `ExecResult::side_effects` after builtin execution.
+#[derive(Debug, Clone)]
+pub enum BuiltinSideEffect {
+    /// Shift N positional parameters (replaces `_SHIFT_COUNT`).
+    ShiftPositional(usize),
+    /// Replace all positional parameters (replaces `_SET_POSITIONAL`).
+    SetPositional(Vec<String>),
+    /// Populate an indexed array variable (replaces `_ARRAY_READ_*`).
+    SetArray { name: String, elements: Vec<String> },
+}
+
 /// Result of executing a bash script.
 #[derive(Debug, Clone, Default)]
 pub struct ExecResult {
@@ -32,6 +48,9 @@ pub struct ExecResult {
     pub final_env: Option<std::collections::HashMap<String, String>>,
     /// Structured trace events (empty when `TraceMode::Off`).
     pub events: Vec<crate::trace::TraceEvent>,
+    /// Structured side effects from builtin execution.
+    /// The interpreter processes these after the builtin returns.
+    pub side_effects: Vec<BuiltinSideEffect>,
 }
 
 impl ExecResult {