From 8d01ddd1261dbb2fb18abb5b5662f9ef4afa718b Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Sat, 14 Mar 2026 23:37:40 +0000 Subject: [PATCH] feat: merge PR #625 fixes, resolve issues #608 #616 #617 #618 - fix(bindings): surface pre-exec failures in stderr for Python/JS (#625) - docs(tool): add bash-vs-sh, chunked writes, limits to agent prompt (#608) - feat(jq): bump reported version to jq-1.8, add trim/abs/if tests (#616) - feat(awk): implement --csv flag with RFC 4180 field splitting (#617, #618) --- .../__test__/error-handling.spec.ts | 22 ++ crates/bashkit-js/src/lib.rs | 226 ++++++++---------- crates/bashkit-python/bashkit/deepagents.py | 14 +- crates/bashkit-python/bashkit/pydantic_ai.py | 2 + crates/bashkit-python/src/lib.rs | 60 +++-- crates/bashkit-python/tests/test_bashkit.py | 38 +++ crates/bashkit/src/builtins/awk.rs | 143 ++++++++++- crates/bashkit/src/builtins/jq.rs | 61 ++++- crates/bashkit/src/tool.rs | 55 ++++- crates/bashkit/tests/spec_cases/jq/jq.test.sh | 4 +- 10 files changed, 453 insertions(+), 172 deletions(-) diff --git a/crates/bashkit-js/__test__/error-handling.spec.ts b/crates/bashkit-js/__test__/error-handling.spec.ts index 86836aaf..cfa5abbc 100644 --- a/crates/bashkit-js/__test__/error-handling.spec.ts +++ b/crates/bashkit-js/__test__/error-handling.spec.ts @@ -194,3 +194,25 @@ test("unclosed quote returns non-zero or handles gracefully", (t) => { // Should either error or handle gracefully t.is(typeof r.exitCode, "number"); }); + +// ============================================================================ +// Pre-exec failure stderr surfacing (issue #606) +// ============================================================================ + +test("pre-exec parse error surfaces in stderr (Bash)", (t) => { + const bash = new Bash(); + const r = bash.executeSync("echo $("); + t.not(r.exitCode, 0); + t.truthy(r.error, "error field should contain the parse error"); + t.truthy(r.stderr, "stderr must not be empty on pre-exec failure"); + t.true(r.stderr.includes(r.error!), "stderr should contain the error message"); +}); + +test("pre-exec parse error surfaces in stderr (BashTool)", (t) => { + const tool = new BashTool(); + const r = tool.executeSync("echo $("); + t.not(r.exitCode, 0); + t.truthy(r.error, "error field should contain the parse error"); + t.truthy(r.stderr, "stderr must not be empty on pre-exec failure"); + t.true(r.stderr.includes(r.error!), "stderr should contain the error message"); +}); diff --git a/crates/bashkit-js/src/lib.rs b/crates/bashkit-js/src/lib.rs index 27f702c2..cf4f8ab2 100644 --- a/crates/bashkit-js/src/lib.rs +++ b/crates/bashkit-js/src/lib.rs @@ -11,8 +11,7 @@ use bashkit::{Bash as RustBash, BashTool as RustBashTool, ExecutionLimits, Tool} use napi_derive::napi; use std::collections::HashMap; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use std::sync::{LazyLock, RwLock}; +use std::sync::atomic::{AtomicBool, Ordering}; use tokio::sync::Mutex; // ============================================================================ @@ -119,12 +118,15 @@ impl Bash { exit_code: result.exit_code, error: None, }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } } }) } @@ -141,12 +143,15 @@ impl Bash { exit_code: result.exit_code, error: None, }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } } } @@ -184,77 +189,40 @@ impl Bash { } // ============================================================================ -// BashTool — handle-based registry pattern -// -// napi-rs stores #[napi] structs behind raw pointers. CodeQL flags transitive -// pointer chains from those raw pointers (rust/access-invalid-pointer). -// BashTool stores only a u64 handle; real state lives in a global REGISTRY, -// so the napi raw pointer never transitively reaches heap-allocated state. +// BashTool — interpreter + tool-contract metadata // ============================================================================ -static NEXT_HANDLE: AtomicU64 = AtomicU64::new(1); -static REGISTRY: LazyLock>>> = - LazyLock::new(|| RwLock::new(HashMap::new())); - -struct BashToolState { - interpreter: Arc>, - cancelled: Arc, - username: Option, - hostname: Option, - max_commands: Option, - max_loop_iterations: Option, -} - -fn tool_register(state: Arc) -> u64 { - let handle = NEXT_HANDLE.fetch_add(1, Ordering::Relaxed); - REGISTRY.write().unwrap().insert(handle, state); - handle -} - -fn tool_lookup(handle: u64) -> napi::Result> { - REGISTRY - .read() - .unwrap() - .get(&handle) - .cloned() - .ok_or_else(|| napi::Error::from_reason("BashTool instance has been disposed")) -} - -fn tool_unregister(handle: u64) { - REGISTRY.write().unwrap().remove(&handle); -} - /// Bash interpreter with tool-contract metadata (`description`, `help`, /// `system_prompt`, schemas). /// /// Use this when integrating with AI frameworks that need tool definitions. #[napi] pub struct BashTool { - handle: u64, -} - -impl Drop for BashTool { - fn drop(&mut self) { - tool_unregister(self.handle); - } + inner: Arc>, + rt: tokio::runtime::Runtime, + cancelled: Arc, + username: Option, + hostname: Option, + max_commands: Option, + max_loop_iterations: Option, } impl BashTool { - fn build_rust_tool(state: &BashToolState) -> RustBashTool { + fn build_rust_tool(&self) -> RustBashTool { let mut builder = RustBashTool::builder(); - if let Some(ref username) = state.username { + if let Some(ref username) = self.username { builder = builder.username(username); } - if let Some(ref hostname) = state.hostname { + if let Some(ref hostname) = self.hostname { builder = builder.hostname(hostname); } let mut limits = ExecutionLimits::new(); - if let Some(mc) = state.max_commands { + if let Some(mc) = self.max_commands { limits = limits.max_commands(mc as usize); } - if let Some(mli) = state.max_loop_iterations { + if let Some(mli) = self.max_loop_iterations { limits = limits.max_loop_iterations(mli as usize); } @@ -277,70 +245,90 @@ impl BashTool { ); let cancelled = bash.cancellation_token(); - let state = Arc::new(BashToolState { - interpreter: Arc::new(Mutex::new(bash)), + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .map_err(|e| napi::Error::from_reason(format!("Failed to create runtime: {e}")))?; + + Ok(Self { + inner: Arc::new(Mutex::new(bash)), + rt, cancelled, username: opts.username, hostname: opts.hostname, max_commands: opts.max_commands, max_loop_iterations: opts.max_loop_iterations, - }); - - Ok(Self { - handle: tool_register(state), }) } /// Execute bash commands synchronously. #[napi] pub fn execute_sync(&self, commands: String) -> napi::Result { - let state = tool_lookup(self.handle)?; - state.cancelled.store(false, Ordering::Relaxed); - let interpreter = state.interpreter.clone(); - let commands_owned = commands; - let rt = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .map_err(|e| napi::Error::from_reason(format!("Failed to create runtime: {e}")))?; - rt.block_on(async move { - let mut bash = interpreter.lock().await; - exec_to_result(&mut bash, &commands_owned).await + self.cancelled.store(false, Ordering::Relaxed); + let inner = self.inner.clone(); + self.rt.block_on(async move { + let mut bash = inner.lock().await; + match bash.exec(&commands).await { + Ok(result) => Ok(ExecResult { + stdout: result.stdout, + stderr: result.stderr, + exit_code: result.exit_code, + error: None, + }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } + } }) } /// Execute bash commands asynchronously, returning a Promise. #[napi] pub async fn execute(&self, commands: String) -> napi::Result { - let state = tool_lookup(self.handle)?; - let interpreter = state.interpreter.clone(); - let mut bash = interpreter.lock().await; - exec_to_result(&mut bash, &commands).await + let inner = self.inner.clone(); + let mut bash = inner.lock().await; + match bash.exec(&commands).await { + Ok(result) => Ok(ExecResult { + stdout: result.stdout, + stderr: result.stderr, + exit_code: result.exit_code, + error: None, + }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } + } } /// Cancel the currently running execution. #[napi] pub fn cancel(&self) { - if let Ok(state) = tool_lookup(self.handle) { - state.cancelled.store(true, Ordering::Relaxed); - } + self.cancelled.store(true, Ordering::Relaxed); } /// Reset interpreter to fresh state, preserving configuration. #[napi] pub fn reset(&self) -> napi::Result<()> { - let state = tool_lookup(self.handle)?; - let interpreter = state.interpreter.clone(); - let username = state.username.clone(); - let hostname = state.hostname.clone(); - let max_commands = state.max_commands; - let max_loop_iterations = state.max_loop_iterations; + let inner = self.inner.clone(); + let username = self.username.clone(); + let hostname = self.hostname.clone(); + let max_commands = self.max_commands; + let max_loop_iterations = self.max_loop_iterations; - let rt = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .map_err(|e| napi::Error::from_reason(format!("Failed to create runtime: {e}")))?; - rt.block_on(async move { - let mut bash = interpreter.lock().await; + self.rt.block_on(async move { + let mut bash = inner.lock().await; let new_bash = build_bash( username.as_deref(), hostname.as_deref(), @@ -367,30 +355,26 @@ impl BashTool { /// Get token-efficient tool description. #[napi] - pub fn description(&self) -> napi::Result { - let state = tool_lookup(self.handle)?; - Ok(Self::build_rust_tool(&state).description().to_string()) + pub fn description(&self) -> String { + self.build_rust_tool().description().to_string() } /// Get help as a Markdown document. #[napi] - pub fn help(&self) -> napi::Result { - let state = tool_lookup(self.handle)?; - Ok(Self::build_rust_tool(&state).help()) + pub fn help(&self) -> String { + self.build_rust_tool().help() } /// Get compact system-prompt text for orchestration. #[napi] - pub fn system_prompt(&self) -> napi::Result { - let state = tool_lookup(self.handle)?; - Ok(Self::build_rust_tool(&state).system_prompt()) + pub fn system_prompt(&self) -> String { + self.build_rust_tool().system_prompt() } /// Get JSON input schema as string. #[napi] pub fn input_schema(&self) -> napi::Result { - let state = tool_lookup(self.handle)?; - let schema = Self::build_rust_tool(&state).input_schema(); + let schema = self.build_rust_tool().input_schema(); serde_json::to_string_pretty(&schema) .map_err(|e| napi::Error::from_reason(format!("Schema serialization failed: {e}"))) } @@ -398,8 +382,7 @@ impl BashTool { /// Get JSON output schema as string. #[napi] pub fn output_schema(&self) -> napi::Result { - let state = tool_lookup(self.handle)?; - let schema = Self::build_rust_tool(&state).output_schema(); + let schema = self.build_rust_tool().output_schema(); serde_json::to_string_pretty(&schema) .map_err(|e| napi::Error::from_reason(format!("Schema serialization failed: {e}"))) } @@ -450,23 +433,6 @@ fn build_bash( builder.build() } -async fn exec_to_result(bash: &mut RustBash, commands: &str) -> napi::Result { - match bash.exec(commands).await { - Ok(result) => Ok(ExecResult { - stdout: result.stdout, - stderr: result.stderr, - exit_code: result.exit_code, - error: None, - }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), - } -} - /// Get the bashkit version string. #[napi] pub fn get_version() -> &'static str { diff --git a/crates/bashkit-python/bashkit/deepagents.py b/crates/bashkit-python/bashkit/deepagents.py index a6577cb7..a83bd7d2 100644 --- a/crates/bashkit-python/bashkit/deepagents.py +++ b/crates/bashkit-python/bashkit/deepagents.py @@ -71,6 +71,8 @@ def _make_bash_tool(bash_instance: NativeBashTool): def bashkit(command: str) -> str: result = bash_instance.execute_sync(command) output = result.stdout + if result.error: + output += f"\nError: {result.error}" if result.stderr: output += f"\n{result.stderr}" if result.exit_code != 0: @@ -134,7 +136,10 @@ def tools(self): def execute_sync(self, command: str) -> str: """Execute command synchronously (for setup scripts).""" result = self._bash.execute_sync(command) - return result.stdout + (result.stderr or "") + output = result.stdout + (result.stderr or "") + if result.error and result.error not in output: + output += f"\nError: {result.error}" + return output def reset(self) -> None: """Reset VFS to initial state.""" @@ -189,6 +194,8 @@ def create_middleware(self) -> BashkitMiddleware: def execute(self, command: str) -> ExecuteResponse: result = self._bash.execute_sync(command) output = result.stdout + (result.stderr or "") + if result.error and result.error not in output: + output += f"\nError: {result.error}" return ExecuteResponse(output=output, exit_code=result.exit_code, truncated=False) async def aexecute(self, command: str) -> ExecuteResponse: @@ -344,7 +351,10 @@ async def aupload_files(self, files: list[tuple[str, bytes]]) -> list[FileUpload def setup(self, script: str) -> str: """Execute setup script.""" result = self._bash.execute_sync(script) - return result.stdout + (result.stderr or "") + output = result.stdout + (result.stderr or "") + if result.error and result.error not in output: + output += f"\nError: {result.error}" + return output def reset(self) -> None: """Reset VFS.""" diff --git a/crates/bashkit-python/bashkit/pydantic_ai.py b/crates/bashkit-python/bashkit/pydantic_ai.py index be6ffd25..349aa0a9 100644 --- a/crates/bashkit-python/bashkit/pydantic_ai.py +++ b/crates/bashkit-python/bashkit/pydantic_ai.py @@ -73,6 +73,8 @@ async def bash(commands: str) -> str: result = await native.execute(commands) output = result.stdout + if result.error: + output += f"\nError: {result.error}" if result.stderr: output += f"\nSTDERR: {result.stderr}" if result.exit_code != 0: diff --git a/crates/bashkit-python/src/lib.rs b/crates/bashkit-python/src/lib.rs index 5f9b5de5..af4d68c9 100644 --- a/crates/bashkit-python/src/lib.rs +++ b/crates/bashkit-python/src/lib.rs @@ -268,12 +268,15 @@ impl PyBash { exit_code: result.exit_code, error: None, }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } } }) } @@ -293,12 +296,15 @@ impl PyBash { exit_code: result.exit_code, error: None, }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } } }) }) @@ -476,12 +482,15 @@ impl BashTool { exit_code: result.exit_code, error: None, }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } } }) } @@ -500,12 +509,15 @@ impl BashTool { exit_code: result.exit_code, error: None, }), - Err(e) => Ok(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: 1, - error: Some(e.to_string()), - }), + Err(e) => { + let msg = e.to_string(); + Ok(ExecResult { + stdout: String::new(), + stderr: msg.clone(), + exit_code: 1, + error: Some(msg), + }) + } } }) }) diff --git a/crates/bashkit-python/tests/test_bashkit.py b/crates/bashkit-python/tests/test_bashkit.py index c365fb44..e9a9f90a 100644 --- a/crates/bashkit-python/tests/test_bashkit.py +++ b/crates/bashkit-python/tests/test_bashkit.py @@ -954,3 +954,41 @@ def test_deeply_nested_schema_rejected(): tool = ScriptedTool("deep") with pytest.raises(ValueError, match="nesting depth"): tool.add_tool("deep", "Deep", callback=lambda p, s=None: "", schema=nested) + + +# =========================================================================== +# Pre-exec failure stderr surfacing (issue #606) +# =========================================================================== + + +def test_bash_pre_exec_error_in_stderr(): + """Pre-exec failures (parse errors) must appear in stderr, not only error field.""" + bash = Bash() + # Unclosed subshell triggers parse error -> Err path in bindings + r = bash.execute_sync("echo $(") + assert r.exit_code != 0 + assert r.error is not None + # Bug #606: stderr was empty even though error had the message + assert r.stderr != "", "stderr must contain the error message, not be empty" + assert r.error in r.stderr + + +def test_bashtool_pre_exec_error_in_stderr(): + """BashTool pre-exec failures must also surface in stderr.""" + tool = BashTool() + r = tool.execute_sync("echo $(") + assert r.exit_code != 0 + assert r.error is not None + assert r.stderr != "", "stderr must contain the error message, not be empty" + assert r.error in r.stderr + + +@pytest.mark.asyncio +async def test_bash_pre_exec_error_in_stderr_async(): + """Async path should also surface pre-exec errors in stderr.""" + bash = Bash() + r = await bash.execute("echo $(") + assert r.exit_code != 0 + assert r.error is not None + assert r.stderr != "", "stderr must contain the error message, not be empty" + assert r.error in r.stderr diff --git a/crates/bashkit/src/builtins/awk.rs b/crates/bashkit/src/builtins/awk.rs index 0515e98e..604f0922 100644 --- a/crates/bashkit/src/builtins/awk.rs +++ b/crates/bashkit/src/builtins/awk.rs @@ -116,6 +116,8 @@ struct AwkState { nr: usize, nf: usize, fnr: usize, + /// When true, fields are split per RFC 4180 CSV rules (--csv flag) + csv_mode: bool, } #[derive(Debug, Clone, PartialEq)] @@ -219,22 +221,62 @@ impl Default for AwkState { nr: 0, nf: 0, fnr: 0, + csv_mode: false, } } } +/// Parse a CSV line per RFC 4180: handle quoted fields, embedded commas, +/// and double-quote escaping. +fn csv_split_fields(line: &str) -> Vec { + let mut fields = Vec::new(); + let mut field = String::new(); + let mut in_quotes = false; + let mut chars = line.chars().peekable(); + + while let Some(c) = chars.next() { + if in_quotes { + if c == '"' { + if chars.peek() == Some(&'"') { + // Escaped quote + field.push('"'); + chars.next(); + } else { + in_quotes = false; + } + } else { + field.push(c); + } + } else if c == '"' { + in_quotes = true; + } else if c == ',' { + fields.push(std::mem::take(&mut field)); + } else { + field.push(c); + } + } + fields.push(field); + fields +} + impl AwkState { + /// Split a line into fields based on current mode (CSV or FS) + fn split_fields(&self, line: &str) -> Vec { + if self.csv_mode { + csv_split_fields(line) + } else if self.fs == " " { + line.split_whitespace().map(String::from).collect() + } else { + line.split(&self.fs).map(String::from).collect() + } + } + fn set_line(&mut self, line: &str) { self.nr += 1; self.fnr += 1; // Split by field separator - if self.fs == " " { - // Special: split on whitespace, collapse multiple spaces - self.fields = line.split_whitespace().map(String::from).collect(); - } else { - self.fields = line.split(&self.fs).map(String::from).collect(); - } + self.fields = self.split_fields(line); self.nf = self.fields.len(); @@ -287,11 +329,7 @@ impl AwkState { "$0" => { let s = value.as_string(); // Re-split fields when $0 is modified - if self.fs == " " { - self.fields = s.split_whitespace().map(String::from).collect(); - } else { - self.fields = s.split(&self.fs).map(String::from).collect(); - } + self.fields = self.split_fields(&s); self.nf = self.fields.len(); self.variables .insert("NF".to_string(), AwkValue::Number(self.nf as f64)); @@ -2837,11 +2875,15 @@ impl Builtin for Awk { let mut files: Vec = Vec::new(); let mut field_sep = " ".to_string(); let mut pre_vars: Vec<(String, String)> = Vec::new(); + let mut csv_mode = false; let mut i = 0; while i < ctx.args.len() { let arg = &ctx.args[i]; - if arg == "-F" { + if arg == "--csv" || arg == "-k" { + csv_mode = true; + field_sep = ",".to_string(); + } else if arg == "-F" { i += 1; if i < ctx.args.len() { field_sep = ctx.args[i].clone(); @@ -2902,6 +2944,10 @@ impl Builtin for Awk { let mut interp = AwkInterpreter::new(); interp.functions = program.functions.clone(); interp.state.fs = Self::process_escape_sequences(&field_sep); + if csv_mode { + interp.state.csv_mode = true; + interp.state.ofs = ",".to_string(); + } // Set pre-assigned variables (-v) for (name, value) in &pre_vars { @@ -3719,4 +3765,77 @@ mod tests { "expected pipe error, got: {err_msg}" ); } + + // ======================================================================== + // --csv flag (issues #617, #618) + // ======================================================================== + + #[tokio::test] + async fn test_awk_csv_basic() { + let result = run_awk(&["--csv", "{print $2}"], Some("a,b,c\nd,e,f")) + .await + .unwrap(); + assert_eq!(result.stdout, "b\ne\n"); + } + + #[tokio::test] + async fn test_awk_csv_quoted_fields() { + // Quoted field with embedded comma + let result = run_awk(&["--csv", "{print $2}"], Some(r#"1,"hello, world",3"#)) + .await + .unwrap(); + assert_eq!(result.stdout, "hello, world\n"); + } + + #[tokio::test] + async fn test_awk_csv_escaped_quotes() { + // Double-quote escaping per RFC 4180 + let result = run_awk(&["--csv", "{print $2}"], Some(r#"1,"she said ""hi""",3"#)) + .await + .unwrap(); + assert_eq!(result.stdout, "she said \"hi\"\n"); + } + + #[tokio::test] + async fn test_awk_csv_nf() { + let result = run_awk(&["--csv", "{print NF}"], Some("a,b,c")) + .await + .unwrap(); + assert_eq!(result.stdout, "3\n"); + } + + #[tokio::test] + async fn test_awk_csv_ofs() { + // In CSV mode, OFS defaults to comma + let result = run_awk(&["--csv", "{print $1, $3}"], Some("a,b,c")) + .await + .unwrap(); + assert_eq!(result.stdout, "a,c\n"); + } + + #[tokio::test] + async fn test_awk_csv_empty_fields() { + let result = run_awk(&["--csv", "{print NF}"], Some("a,,c,")) + .await + .unwrap(); + assert_eq!(result.stdout, "4\n"); + } + + #[tokio::test] + async fn test_awk_csv_k_flag_alias() { + // -k is an alias for --csv + let result = run_awk(&["-k", "{print $2}"], Some("a,b,c")).await.unwrap(); + assert_eq!(result.stdout, "b\n"); + } + + #[test] + fn test_csv_split_fields_unit() { + assert_eq!(csv_split_fields("a,b,c"), vec!["a", "b", "c"]); + assert_eq!( + csv_split_fields(r#"1,"hello, world",3"#), + vec!["1", "hello, world", "3"] + ); + assert_eq!(csv_split_fields(r#""a""b",c"#), vec!["a\"b", "c"]); + assert_eq!(csv_split_fields("a,,c,"), vec!["a", "", "c", ""]); + } } diff --git a/crates/bashkit/src/builtins/jq.rs b/crates/bashkit/src/builtins/jq.rs index 96a76afb..aafb0f6b 100644 --- a/crates/bashkit/src/builtins/jq.rs +++ b/crates/bashkit/src/builtins/jq.rs @@ -157,7 +157,7 @@ impl Builtin for Jq { // Check for --version flag first for arg in ctx.args { if arg == "-V" || arg == "--version" { - return Ok(ExecResult::ok("jq-1.7.1\n".to_string())); + return Ok(ExecResult::ok("jq-1.8\n".to_string())); } } @@ -1326,4 +1326,63 @@ mod tests { // SAFETY: This test is single-threaded (serial_test) so remove_var is safe unsafe { std::env::remove_var(unique_key) }; } + + // ======================================================================== + // jq 1.8 builtins (issue #616) + // ======================================================================== + + #[tokio::test] + async fn test_jq_version_string() { + let result = run_jq_result_with_args(&["--version"], "null") + .await + .unwrap(); + assert_eq!(result.stdout.trim(), "jq-1.8"); + } + + #[tokio::test] + async fn test_jq_abs() { + assert_eq!(run_jq("abs", "-42").await.unwrap().trim(), "42"); + assert_eq!(run_jq("abs", "3.14").await.unwrap().trim(), "3.14"); + assert_eq!(run_jq("abs", "-0.5").await.unwrap().trim(), "0.5"); + } + + #[tokio::test] + async fn test_jq_trim() { + assert_eq!( + run_jq("trim", r#"" hello ""#).await.unwrap().trim(), + r#""hello""# + ); + } + + #[tokio::test] + async fn test_jq_ltrim() { + assert_eq!( + run_jq("ltrim", r#"" hello ""#).await.unwrap().trim(), + r#""hello ""# + ); + } + + #[tokio::test] + async fn test_jq_rtrim() { + assert_eq!( + run_jq("rtrim", r#"" hello ""#).await.unwrap().trim(), + r#"" hello""# + ); + } + + #[tokio::test] + async fn test_jq_if_without_else() { + // jq 1.8: `if COND then EXPR end` without `else` uses identity + assert_eq!( + run_jq("if . > 0 then . * 2 end", "5").await.unwrap().trim(), + "10" + ); + assert_eq!( + run_jq("if . > 0 then . * 2 end", "-1") + .await + .unwrap() + .trim(), + "-1" + ); + } } diff --git a/crates/bashkit/src/tool.rs b/crates/bashkit/src/tool.rs index 4d463974..355e7ca6 100644 --- a/crates/bashkit/src/tool.rs +++ b/crates/bashkit/src/tool.rs @@ -1018,6 +1018,47 @@ fn build_bash_system_prompt(tool: &BashTool) -> String { )); } + // Operational guidance for LLM agents (issue #608) + parts.push( + localized( + tool.locale(), + "Use bash syntax; do not assume /bin/sh portability ($RANDOM, arrays, [[ ]] require bash).", + "Використовуйте синтаксис bash; не покладайтеся на /bin/sh ($RANDOM, масиви, [[ ]] потребують bash).", + ) + .to_string(), + ); + parts.push( + localized( + tool.locale(), + "Use `source` only when current-shell state must persist; otherwise run scripts directly.", + "Використовуйте `source` лише коли стан оболонки має зберігатися; інакше запускайте скрипти напряму.", + ) + .to_string(), + ); + parts.push( + localized( + tool.locale(), + "For large multi-file writes, prefer incremental batches over one giant script.", + "Для масових записів файлів віддавайте перевагу інкрементальним пакетам замість одного великого скрипту.", + ) + .to_string(), + ); + + // Surface configured limits + if let Some(ref limits) = tool.limits { + let mut limit_parts = Vec::new(); + limit_parts.push(format!("max_commands={}", limits.max_commands)); + limit_parts.push(format!( + "max_loop_iterations={}", + limits.max_loop_iterations + )); + parts.push(format!( + "{}: {}.", + localized(tool.locale(), "Limits", "Ліміти"), + limit_parts.join(", ") + )); + } + if !tool.builtin_hints.is_empty() { parts.extend(tool.builtin_hints.iter().cloned()); } @@ -1233,10 +1274,22 @@ mod tests { assert!(helptext.contains("50 commands")); assert!(helptext.contains("API_KEY")); - // system_prompt should include home + // system_prompt should include home and operational guidance let sysprompt = tool.system_prompt(); assert!(sysprompt.starts_with("bashkit:")); assert!(sysprompt.contains("Home /home/agent.")); + assert!( + sysprompt.contains("bash syntax"), + "system_prompt should warn about bash vs sh" + ); + assert!( + sysprompt.contains("incremental batches"), + "system_prompt should recommend chunked writes" + ); + assert!( + sysprompt.contains("max_commands=50"), + "system_prompt should surface configured limits" + ); } #[test] diff --git a/crates/bashkit/tests/spec_cases/jq/jq.test.sh b/crates/bashkit/tests/spec_cases/jq/jq.test.sh index 3601dc31..63b4aced 100644 --- a/crates/bashkit/tests/spec_cases/jq/jq.test.sh +++ b/crates/bashkit/tests/spec_cases/jq/jq.test.sh @@ -838,14 +838,14 @@ ab # Version flag outputs version string jq --version ### expect -jq-1.7.1 +jq-1.8 ### end ### jq_version_short # Short version flag outputs version string jq -V ### expect -jq-1.7.1 +jq-1.8 ### end ### jq_file_input