Skip to content

fix(builtins): read builtin respects local variable scoping #901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chaliy merged 1 commit into from
Mar 30, 2026
Merged

fix(builtins): read builtin respects local variable scoping #901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 59 additions & 31 deletions crates/bashkit/src/builtins/read.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,27 +158,30 @@ impl Builtin for Read {
var_args
};

// Assign words to variables
// Assign words to variables via side effects (respects local scoping)
let mut result = ExecResult::ok(String::new());
for (i, var_name) in var_names.iter().enumerate() {
// THREAT[TM-INJ-009]: Block internal variable prefix injection via read
if is_internal_variable(var_name) {
continue;
}
if i == var_names.len() - 1 {
let value = if i == var_names.len() - 1 {
// Last variable gets all remaining words
let remaining: Vec<&str> = words.iter().skip(i).copied().collect();
let value = remaining.join(" ");
ctx.variables.insert(var_name.to_string(), value);
remaining.join(" ")
} else if i < words.len() {
ctx.variables
.insert(var_name.to_string(), words[i].to_string());
words[i].to_string()
} else {
// Not enough words - set to empty
ctx.variables.insert(var_name.to_string(), String::new());
}
String::new()
};
result.side_effects.push(BuiltinSideEffect::SetVariable {
name: var_name.to_string(),
value,
});
}

Ok(ExecResult::ok(String::new()))
Ok(result)
}
}

Expand All @@ -199,6 +202,17 @@ mod tests {
(fs, cwd, variables)
}

/// Extract SetVariable side effects into a map for easy assertion.
fn extract_vars(result: &ExecResult) -> HashMap<String, String> {
let mut map = HashMap::new();
for effect in &result.side_effects {
if let BuiltinSideEffect::SetVariable { name, value } = effect {
map.insert(name.clone(), value.clone());
}
}
map
}

// ==================== no stdin ====================

#[tokio::test]
Expand Down Expand Up @@ -228,7 +242,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("REPLY").unwrap(), "hello world");
let vars = extract_vars(&result);
assert_eq!(vars.get("REPLY").unwrap(), "hello world");
}

// ==================== read into named variable ====================
Expand All @@ -248,7 +263,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("MY_VAR").unwrap(), "test_value");
let vars = extract_vars(&result);
assert_eq!(vars.get("MY_VAR").unwrap(), "test_value");
}

// ==================== read into multiple variables ====================
Expand All @@ -268,10 +284,11 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("A").unwrap(), "one");
assert_eq!(variables.get("B").unwrap(), "two");
let vars = extract_vars(&result);
assert_eq!(vars.get("A").unwrap(), "one");
assert_eq!(vars.get("B").unwrap(), "two");
// Last var gets remaining words
assert_eq!(variables.get("C").unwrap(), "three four");
assert_eq!(vars.get("C").unwrap(), "three four");
}

#[tokio::test]
Expand All @@ -289,9 +306,10 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("A").unwrap(), "one");
assert_eq!(variables.get("B").unwrap(), "");
assert_eq!(variables.get("C").unwrap(), "");
let vars = extract_vars(&result);
assert_eq!(vars.get("A").unwrap(), "one");
assert_eq!(vars.get("B").unwrap(), "");
assert_eq!(vars.get("C").unwrap(), "");
}

// ==================== -r flag (raw mode) ====================
Expand All @@ -311,7 +329,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("LINE").unwrap(), "hello\\world");
let vars = extract_vars(&result);
assert_eq!(vars.get("LINE").unwrap(), "hello\\world");
}

#[tokio::test]
Expand All @@ -329,8 +348,9 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
let vars = extract_vars(&result);
// Without -r, backslash-newline is line continuation
assert_eq!(variables.get("LINE").unwrap(), "helloworld");
assert_eq!(vars.get("LINE").unwrap(), "helloworld");
}

// ==================== -n flag (read N chars) ====================
Expand All @@ -350,7 +370,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("CHUNK").unwrap(), "abc");
let vars = extract_vars(&result);
assert_eq!(vars.get("CHUNK").unwrap(), "abc");
}

#[tokio::test]
Expand All @@ -368,7 +389,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("CHUNK").unwrap(), "hi");
let vars = extract_vars(&result);
assert_eq!(vars.get("CHUNK").unwrap(), "hi");
}

// ==================== -d flag (delimiter) ====================
Expand All @@ -388,7 +410,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("FIELD").unwrap(), "first");
let vars = extract_vars(&result);
assert_eq!(vars.get("FIELD").unwrap(), "first");
}

// ==================== -a flag (array mode) ====================
Expand Down Expand Up @@ -458,7 +481,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("V").unwrap(), "path\\to\\file");
let vars = extract_vars(&result);
assert_eq!(vars.get("V").unwrap(), "path\\to\\file");
}

// ==================== multiline input ====================
Expand All @@ -478,7 +502,8 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("LINE").unwrap(), "first");
let vars = extract_vars(&result);
assert_eq!(vars.get("LINE").unwrap(), "first");
}

// ==================== custom IFS ====================
Expand All @@ -499,8 +524,9 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("A").unwrap(), "foo");
assert_eq!(variables.get("B").unwrap(), "bar baz");
let vars = extract_vars(&result);
assert_eq!(vars.get("A").unwrap(), "foo");
assert_eq!(vars.get("B").unwrap(), "bar baz");
}

#[tokio::test]
Expand All @@ -524,10 +550,11 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("A").unwrap(), "one");
assert_eq!(variables.get("B").unwrap(), "");
assert_eq!(variables.get("C").unwrap(), "three");
assert_eq!(variables.get("D").unwrap(), "");
let vars = extract_vars(&result);
assert_eq!(vars.get("A").unwrap(), "one");
assert_eq!(vars.get("B").unwrap(), "");
assert_eq!(vars.get("C").unwrap(), "three");
assert_eq!(vars.get("D").unwrap(), "");
}

#[tokio::test]
Expand All @@ -546,6 +573,7 @@ mod tests {
);
let result = Read.execute(ctx).await.unwrap();
assert_eq!(result.exit_code, 0);
assert_eq!(variables.get("LINE").unwrap(), "no splitting here");
let vars = extract_vars(&result);
assert_eq!(vars.get("LINE").unwrap(), "no splitting here");
}
}
3 changes: 3 additions & 0 deletions crates/bashkit/src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5370,6 +5370,9 @@ impl Interpreter {
builtins::BuiltinSideEffect::SetLastExitCode(code) => {
self.last_exit_code = *code;
}
builtins::BuiltinSideEffect::SetVariable { name, value } => {
self.set_variable(name.clone(), value.clone());
}
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions crates/bashkit/src/interpreter/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ pub enum BuiltinSideEffect {
ClearHistory,
/// Set the last exit code (for wait builtin).
SetLastExitCode(i32),
/// Set a shell variable (respects local scoping via `set_variable`).
SetVariable { name: String, value: String },
}

/// Result of executing a bash script.
Expand Down
16 changes: 16 additions & 0 deletions crates/bashkit/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2469,6 +2469,22 @@ mod tests {
assert_eq!(result.stdout, "a b c\n");
}

#[tokio::test]
async fn test_read_respects_local_scope() {
// Regression: `local k; read -r k <<< "val"` must set k in local scope
let mut bash = Bash::new();
let result = bash
.exec(
r#"
fn() { local k; read -r k <<< "test"; echo "$k"; }
fn
"#,
)
.await
.unwrap();
assert_eq!(result.stdout, "test\n");
}

#[tokio::test]
async fn test_glob_star() {
let mut bash = Bash::new();
Expand Down
Loading