Skip to content

Commit cab46fb

Browse files
chaliyclaude
andauthored
fix(tool): preserve custom builtins across create_bash calls (#461)
## Summary - Changed `BashTool.builtins` from `Vec<(String, Box<dyn Builtin>)>` to `Vec<(String, Arc<dyn Builtin>)>` - `create_bash()` now clones Arc refs instead of `std::mem::take` which emptied builtins - Added `Builtin` impl for `Arc<dyn Builtin>` to enable Box wrapping ## Test plan - [x] `test_create_bash_preserves_builtins` — custom builtin available on second call - [x] All lib tests pass Closes #422 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 27743aa commit cab46fb

File tree

2 files changed

+55
-7
lines changed

2 files changed

+55
-7
lines changed

crates/bashkit/src/builtins/mod.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,17 @@ pub trait Builtin: Send + Sync {
387387
}
388388
}
389389

390+
#[async_trait]
391+
impl Builtin for std::sync::Arc<dyn Builtin> {
392+
async fn execute(&self, ctx: Context<'_>) -> Result<ExecResult> {
393+
(**self).execute(ctx).await
394+
}
395+
396+
fn llm_hint(&self) -> Option<&'static str> {
397+
(**self).llm_hint()
398+
}
399+
}
400+
390401
#[cfg(test)]
391402
mod tests {
392403
use super::*;

crates/bashkit/src/tool.rs

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -327,8 +327,8 @@ pub struct BashToolBuilder {
327327
limits: Option<ExecutionLimits>,
328328
/// Environment variables to set
329329
env_vars: Vec<(String, String)>,
330-
/// Custom builtins (name, implementation)
331-
builtins: Vec<(String, Box<dyn Builtin>)>,
330+
/// Custom builtins (name, implementation). Arc enables reuse across create_bash calls.
331+
builtins: Vec<(String, Arc<dyn Builtin>)>,
332332
}
333333

334334
impl BashToolBuilder {
@@ -368,7 +368,7 @@ impl BashToolBuilder {
368368
/// If the builtin implements [`Builtin::llm_hint`], its hint will be
369369
/// included in `help()` and `system_prompt()`.
370370
pub fn builtin(mut self, name: impl Into<String>, builtin: Box<dyn Builtin>) -> Self {
371-
self.builtins.push((name.into(), builtin));
371+
self.builtins.push((name.into(), Arc::from(builtin)));
372372
self
373373
}
374374

@@ -423,7 +423,7 @@ pub struct BashTool {
423423
hostname: Option<String>,
424424
limits: Option<ExecutionLimits>,
425425
env_vars: Vec<(String, String)>,
426-
builtins: Vec<(String, Box<dyn Builtin>)>,
426+
builtins: Vec<(String, Arc<dyn Builtin>)>,
427427
/// Names of custom builtins (for documentation)
428428
builtin_names: Vec<String>,
429429
/// LLM hints from registered builtins
@@ -452,9 +452,9 @@ impl BashTool {
452452
for (key, value) in &self.env_vars {
453453
builder = builder.env(key, value);
454454
}
455-
// Move builtins out to avoid borrow issues
456-
for (name, builtin) in std::mem::take(&mut self.builtins) {
457-
builder = builder.builtin(name, builtin);
455+
// Clone Arc builtins so they survive across multiple create_bash calls
456+
for (name, builtin) in &self.builtins {
457+
builder = builder.builtin(name.clone(), Box::new(Arc::clone(builtin)));
458458
}
459459

460460
builder.build()
@@ -1272,4 +1272,41 @@ mod tests {
12721272
assert_eq!(req.commands, "echo hello");
12731273
assert_eq!(req.timeout_ms, Some(5000));
12741274
}
1275+
1276+
// Issue #422: create_bash should not empty builtins after first call
1277+
#[tokio::test]
1278+
async fn test_create_bash_preserves_builtins() {
1279+
use crate::builtins::{Builtin, Context};
1280+
use crate::ExecResult;
1281+
use async_trait::async_trait;
1282+
1283+
struct TestBuiltin;
1284+
1285+
#[async_trait]
1286+
impl Builtin for TestBuiltin {
1287+
async fn execute(&self, _ctx: Context<'_>) -> crate::Result<ExecResult> {
1288+
Ok(ExecResult::ok("test_output\n"))
1289+
}
1290+
}
1291+
1292+
let mut tool = BashToolBuilder::new()
1293+
.builtin("testcmd", Box::new(TestBuiltin))
1294+
.build();
1295+
1296+
// First call
1297+
let mut bash1 = tool.create_bash();
1298+
let result1 = bash1.exec("testcmd").await.unwrap();
1299+
assert!(
1300+
result1.stdout.contains("test_output"),
1301+
"first call should have custom builtin"
1302+
);
1303+
1304+
// Second call should still have the builtin
1305+
let mut bash2 = tool.create_bash();
1306+
let result2 = bash2.exec("testcmd").await.unwrap();
1307+
assert!(
1308+
result2.stdout.contains("test_output"),
1309+
"second call should still have custom builtin"
1310+
);
1311+
}
12751312
}

0 commit comments

Comments
 (0)