fix(builtins): return, unset -v and read reject bad arguments

crates/bashkit/src/builtins/read.rs

@@ -56,7 +56,17 @@ impl Builtin for Read {
                             } else {
                                 &rest
                             };
-                            nchars = n_str.parse().ok();
+                            // Bash rejects a non-numeric count rather than
+                            // reading an unbounded line.
+                            match n_str.parse() {
+                                Ok(n) => nchars = Some(n),
+                                Err(_) => {
+                                    return Ok(ExecResult::err(
+                                        format!("read: {n_str}: invalid number\n"),
+                                        1,
+                                    ));
+                                }
+                            }
                             break;
                         }
                         'p' => {
@@ -76,7 +86,15 @@ impl Builtin for Read {
                             } else {
                                 &rest
                             };
-                            timeout = t_str.parse().ok().filter(|t: &f64| t.is_finite());
+                            match t_str.parse::<f64>() {
+                                Ok(t) if t.is_finite() => timeout = Some(t),
+                                _ => {
+                                    return Ok(ExecResult::err(
+                                        format!("read: {t_str}: invalid timeout specification\n"),
+                                        1,
+                                    ));
+                                }
+                            }
                             break;
                         }
                         'u' => {

crates/bashkit/src/interpreter/mod.rs

@@ -889,6 +889,18 @@ fn normalize_dev_path(path: &Path) -> PathBuf {
 /// THREAT[TM-INJ-009,TM-INJ-016]: Check if a variable name is an internal marker.
 /// Used by builtins and interpreter to block user assignment to internal prefixes.
 /// Note: `_TTY_` is intentionally excluded — it is user-configurable (bashkit extension).
+/// A name `unset -v` accepts: an identifier, optionally with a subscript.
+fn is_unset_target_name(name: &str) -> bool {
+    let base = match name.find('[') {
+        Some(i) if name.ends_with(']') => &name[..i],
+        Some(_) => return false,
+        None => name,
+    };
+    let mut chars = base.chars();
+    matches!(chars.next(), Some(c) if c.is_ascii_alphabetic() || c == '_')
+        && chars.all(|c| c.is_ascii_alphanumeric() || c == '_')
+}
+
 pub(crate) fn is_internal_variable(name: &str) -> bool {
     name.starts_with("SHOPT_")
         || name.starts_with("_NAMEREF_")
@@ -1438,6 +1450,10 @@ pub struct Interpreter {
     /// `unset LINENO` makes it an ordinary variable for the rest of the
     /// shell's life, like bash.
     lineno_unset: bool,
+    /// How many `source`d scripts are running. `return` is only legal inside
+    /// a function or a sourced script, so it needs to tell the two apart from
+    /// a plain top-level command.
+    source_depth: usize,
     /// HTTP client for network builtins (curl, wget)
     #[cfg(feature = "http_client")]
     http_client: Option<Arc<crate::network::HttpClient>>,
@@ -2046,6 +2062,7 @@ impl Interpreter {
             jobs: jobs::new_shared_job_table(),
             current_line: 1,
             lineno_unset: false,
+            source_depth: 0,
             #[cfg(feature = "http_client")]
             http_client: None,
             #[cfg(feature = "git")]
@@ -2189,6 +2206,7 @@ impl Interpreter {
             fs: Arc::clone(&self.fs),
             env: self.env.clone(),
             scoped: self.scoped.clone(),
+            source_depth: self.source_depth,
             flags: self.flags,
             cwd: self.cwd.clone(),
             last_exit_code: self.last_exit_code,
@@ -3354,6 +3372,13 @@ impl Interpreter {
                         break;
                     }
                 } else {
+                    // `return N` carries its status in the control flow, not
+                    // in `exit_code`, so a sourced script's `return 4` is
+                    // `source`'s status.
+                    if let ControlFlow::Return(code) = result.control_flow {
+                        exit_code = code;
+                        self.last_exit_code = code;
+                    }
                     stopped = true;
                     break;
                 }
@@ -7275,6 +7300,21 @@ impl Interpreter {
                 self.insert_variable_checked("_".to_string(), name.to_string());
             }
 
+            // `return` outside a function or a sourced script is a usage
+            // error in bash: it reports, exits 2 and the script carries on.
+            // `keeps_arg0` marks a function's frame; a frame that only carries
+            // top-level positional parameters (`bashkit script.sh a b`) does
+            // not make `return` legal.
+            let in_function = self.call_stack.iter().any(|f| f.keeps_arg0);
+            if name == "return" && !in_function && self.source_depth == 0 {
+                self.last_exit_code = 2;
+                return Ok(ExecResult::err(
+                    "bash: return: can only `return' from a function or sourced script\n"
+                        .to_string(),
+                    2,
+                ));
+            }
+
             // Check for nounset error from argument expansion
             if let Some(err_msg) = self.take_expansion_error() {
                 self.last_exit_code = 1;
@@ -8637,7 +8677,9 @@ impl Interpreter {
 
         // Execute the script commands in the current shell context.
         // Use execute_script_body (not execute) to preserve depth counters.
+        self.source_depth += 1;
         let exec_result = self.execute_script_body(&script, false, true).await;
+        self.source_depth -= 1;
 
         // Pop source depth and BASH_SOURCE (always, even on error)
         self.counters.pop_function();
@@ -8646,6 +8688,13 @@ impl Interpreter {
 
         let mut result = exec_result?;
 
+        // `return N` inside a sourced script is the status of `source`, and
+        // stops the sourced file only, not the script that sourced it.
+        if let ControlFlow::Return(code) = result.control_flow {
+            result.exit_code = code;
+            result.control_flow = ControlFlow::None;
+        }
+
         // Restore positional parameters
         if has_source_args {
             if let Some(saved) = saved_positional {
@@ -9382,19 +9431,39 @@ impl Interpreter {
     ) -> Result<ExecResult> {
         let mut unset_nameref = false;
         let mut unset_function = false;
+        let mut explicit_variable = false;
         let mut var_args: Vec<&String> = Vec::new();
         for arg in args {
             if arg == "-n" {
                 unset_nameref = true;
             } else if arg == "-f" {
                 unset_function = true;
             } else if arg == "-v" {
-                // -v (variable, default) - explicit variable mode
+                explicit_variable = true;
             } else {
                 var_args.push(arg);
             }
         }
 
+        // With an explicit `-v` bash insists the name is one it could have
+        // assigned; without it, and under `-f`, any word is accepted and a
+        // name that does not exist is simply skipped.
+        if explicit_variable && !unset_function {
+            for arg in &var_args {
+                if !is_unset_target_name(arg) {
+                    return self
+                        .apply_redirections(
+                            ExecResult::err(
+                                format!("bash: unset: `{arg}': not a valid identifier\n"),
+                                1,
+                            ),
+                            redirects,
+                        )
+                        .await;
+                }
+            }
+        }
+
         let mut stderr = String::new();
         let mut exit_code: i32 = 0;
 

crates/bashkit/tests/integration/builtin_validation_tests.rs

@@ -0,0 +1,87 @@
+//! Builtin arguments bash refuses, and what it says about them.
+//!
+//! `return` outside a function or a sourced script, a name `unset -v` could
+//! never have assigned, and a non-numeric `read -t`/`-n` are all usage errors
+//! in bash. Each reports and fails; the script keeps running.
+
+use bashkit::Bash;
+
+async fn run(script: &str) -> bashkit::ExecResult {
+    Bash::builder().build().exec(script).await.unwrap()
+}
+
+#[tokio::test]
+async fn return_outside_a_function_reports_and_keeps_going() {
+    let r = run("return\necho \"rc=$?\"\necho after").await;
+    assert_eq!(
+        r.stderr.to_string(),
+        "bash: return: can only `return' from a function or sourced script\n"
+    );
+    assert_eq!(r.stdout.to_string(), "rc=2\nafter\n");
+}
+
+#[tokio::test]
+async fn return_inside_a_function_still_works() {
+    let r = run("f() { return 3; }\nf\necho \"rc=$?\"").await;
+    assert_eq!(r.stderr.to_string(), "");
+    assert_eq!(r.stdout.to_string(), "rc=3\n");
+}
+
+#[tokio::test]
+async fn return_in_a_sourced_script_is_its_status() {
+    let r = run("printf 'echo in\\nreturn 4\\necho no\\n' > s.sh\n. ./s.sh\necho \"rc=$?\"").await;
+    assert_eq!(r.stderr.to_string(), "");
+    assert_eq!(r.stdout.to_string(), "in\nrc=4\n");
+}
+
+#[tokio::test]
+async fn unset_v_insists_on_an_identifier() {
+    for name in ["arr[", "a-b", "1bad"] {
+        let r = run(&format!("unset -v '{name}'\necho \"rc=$?\"")).await;
+        assert_eq!(
+            r.stderr.to_string(),
+            format!("bash: unset: `{name}': not a valid identifier\n")
+        );
+        assert_eq!(r.stdout.to_string(), "rc=1\n");
+    }
+}
+
+/// Without `-v`, and under `-f`, bash takes any word and skips what it
+/// cannot find.
+#[tokio::test]
+async fn unset_without_v_accepts_any_word() {
+    let r = run("unset 'arr['\nunset -f 'f['\necho \"rc=$?\"").await;
+    assert_eq!(r.stderr.to_string(), "");
+    assert_eq!(r.stdout.to_string(), "rc=0\n");
+}
+
+#[tokio::test]
+async fn unset_v_still_takes_a_subscript() {
+    let r = run("a=(x y)\nunset -v 'a[0]'\necho \"${a[@]}\"").await;
+    assert_eq!(r.stderr.to_string(), "");
+    assert_eq!(r.stdout.to_string(), "y\n");
+}
+
+#[tokio::test]
+async fn read_rejects_a_non_numeric_timeout() {
+    let r = run("read -t abc x <<< ''\necho \"rc=$?\"").await;
+    assert_eq!(
+        r.stderr.to_string(),
+        "read: abc: invalid timeout specification\n"
+    );
+    assert_eq!(r.stdout.to_string(), "rc=1\n");
+}
+
+#[tokio::test]
+async fn read_rejects_a_non_numeric_count() {
+    let r = run("read -n abc x <<< ''\necho \"rc=$?\"").await;
+    assert_eq!(r.stderr.to_string(), "read: abc: invalid number\n");
+    assert_eq!(r.stdout.to_string(), "rc=1\n");
+}
+
+#[tokio::test]
+async fn read_still_takes_a_numeric_count() {
+    let r = run("read -n 3 x <<< 'hello'\necho \"$x\"").await;
+    assert_eq!(r.stderr.to_string(), "");
+    assert_eq!(r.stdout.to_string(), "hel\n");
+}

crates/bashkit/tests/integration/main.rs

@@ -30,6 +30,7 @@ pub mod builtin_cap_security_tests;
 pub mod builtin_error_security_tests;
 pub mod builtin_fs_error_tests;
 pub mod builtin_registry_tests;
+pub mod builtin_validation_tests;
 pub mod byte_range_panic_tests;
 pub mod byte_stream_tests;
 pub mod cancellation_tests;

crates/bashkit/tests/spec_cases/bash/builtin-validation.test.sh

@@ -0,0 +1,69 @@
+### return_outside_a_function_reports_and_keeps_going
+return 2>/dev/null
+echo "rc=$?"
+echo after
+### expect
+rc=2
+after
+### end
+
+### return_inside_a_function_still_works
+f() { return 3; }
+f
+echo "rc=$?"
+### expect
+rc=3
+### end
+
+### return_in_a_sourced_script_is_its_status
+printf 'echo in\nreturn 4\necho no\n' > s.sh
+. ./s.sh
+echo "rc=$?"
+### expect
+in
+rc=4
+### end
+
+### unset_v_insists_on_an_identifier
+unset -v 'arr[' 2>/dev/null
+echo "rc=$?"
+### expect
+rc=1
+### end
+
+### unset_without_v_accepts_any_word
+unset 'arr['
+unset -f 'f['
+echo "rc=$?"
+### expect
+rc=0
+### end
+
+### unset_v_still_takes_a_subscript
+a=(x y)
+unset -v 'a[0]'
+echo "${a[@]}"
+### expect
+y
+### end
+
+### read_rejects_a_non_numeric_timeout
+read -t abc x <<< '' 2>/dev/null
+echo "rc=$?"
+### expect
+rc=1
+### end
+
+### read_rejects_a_non_numeric_count
+read -n abc x <<< '' 2>/dev/null
+echo "rc=$?"
+### expect
+rc=1
+### end
+
+### read_still_takes_a_numeric_count
+read -n 3 x <<< 'hello'
+echo "$x"
+### expect
+hel
+### end

knowledge/log.md

@@ -4,6 +4,7 @@
 
 * **Fix**: `grep` follows GNU grep 3.11. Prefixes print as `FILE:LINE:BYTE:` (context lines with `-`), group separators appear between non-adjacent groups and files whenever a context option is given (`-NUM`, `--group-separator`, `--no-group-separator`), trailing context after `-m` is printed in full, `-o` skips empty matches and is POSIX leftmost-longest (`grep -oE 'ab|abcd'` prints `abcd`), `-b -o` gives each match's offset. New `grep_pattern` translates GNU BRE/ERE (brackets, intervals, `\< \> \b \w`, literal leading `*` in BRE, dropped leading ERE operator with GNU's warning, glibc error messages) onto awk's leftmost-longest `AwkRegex`; back-references work in BRE and ERE via fancy-regex (TM-DOS-025 cap). Options parse like getopt_long (unique prefixes such as `--no-gr`, `-T`, `-d`, `--label`, `-` for stdin, `-R` follows symlinks), an empty `-f` file matches nothing, a directory operand without `-r` reports `Is a directory`, invalid UTF-8 output turns the file binary, `\r` is line data. Debian-oracle grep score 60.5% to 96.8% (185 cases); grep's probes in the regex set all pass except two raw-`\x01` patterns the shell drops. Remaining misses are readdir order (L-GREP-003); L-GREP-002 lists the other gaps. See foundations/builtins "grep".
 
+* **Fix**: builtins reject the arguments bash rejects. `return` outside a function or a sourced script reports `can only \`return' from a function or sourced script`, exits 2 and the script carries on (it used to end the script with status 0); `return N` inside a sourced script is now `source`'s status; `unset -v` insists the name is one it could have assigned (`arr[`, `a-b`, `1bad` are errors, `a[0]` is fine) while plain `unset` and `unset -f` still take any word; and `read -t`/`-n` report `invalid timeout specification` / `invalid number` instead of silently ignoring the option.
 * **Fix**: jq `implode` maps invalid code points to U+FFFD, `strptime`/`fromdate` fail with jq's `date "X" does not match format "F"`, and `@base64d` accepts unpadded input and non-zero trailing bits with jq's error texts.
 * **Feature**: jq destructuring alternatives `T as P1 ?// P2 | BODY`: every pattern variable is bound (null when unbound), an error in a pattern or the body moves to the next pattern, the last error propagates. jaq does not parse `?//`; `builtins/jq/altpat.rs` rewrites it to `try`/`catch` before compiling (body copied per pattern, at most 16 rewrites).
 * **Feature**: jq `$__loc__` (`{"file":"<top-level>","line":N}`), including `{$__loc__}` shorthand and string interpolation. jaq has no `$__loc__`, so the filter text is rewritten before compiling (`builtins/jq/loc.rs` skips strings and comments).