From 3e73dbe615683e8a4dec551793df5c85e8e631b9 Mon Sep 17 00:00:00 2001 From: Nige Date: Mon, 23 Mar 2026 00:48:02 +0000 Subject: [PATCH] perf(tools): remove unconditional params clone in shared execution (fix #893) (#926) * perf(tools): remove unconditional params clone in shared execution * Update src/tools/execute.rs Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * chore(fmt): apply rustfmt in worker container tool execution * fix(tools): restore owned param call sites * fix(tools): pass normalized_params to tool.execute() instead of raw params The ownership refactor accidentally passed the un-coerced `params` to `tool.execute()` while validation ran against the coerced `normalized_params`. This meant tools received un-normalized input (e.g. stringified JSON arrays instead of actual arrays). Since `normalized_params` is owned and unused after the execute call, passing it directly achieves the original zero-clone goal without breaking parameter coercion. Co-Authored-By: Claude Opus 4.6 (1M context) * fix(tools): update empty-tool-name test for owned params signature Adapts the test_execute_empty_tool_name_returns_not_found test (added on staging) to pass owned Value instead of &Value, matching the new execute_tool_with_safety signature. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: ilblackdragon@gmail.com Co-authored-by: Claude Opus 4.6 (1M context) --- src/agent/dispatcher.rs | 9 ++++++++- src/agent/scheduler.rs | 6 +----- src/tools/execute.rs | 23 ++++++++++------------- src/worker/container.rs | 11 ++++++++--- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 7fc8e0ca..3f29492d 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -915,7 +915,14 @@ pub(super) async fn execute_chat_tool_standalone( params: &serde_json::Value, job_ctx: &crate::context::JobContext, ) -> Result { - crate::tools::execute::execute_tool_with_safety(tools, safety, tool_name, params, job_ctx).await + crate::tools::execute::execute_tool_with_safety( + tools, + safety, + tool_name, + params.clone(), + job_ctx, + ) + .await } /// Parsed auth result fields for emitting StatusUpdate::AuthRequired. diff --git a/src/agent/scheduler.rs b/src/agent/scheduler.rs index 2e23b35f..1c4a7fde 100644 --- a/src/agent/scheduler.rs +++ b/src/agent/scheduler.rs @@ -549,11 +549,7 @@ impl Scheduler { // Delegate to shared tool execution pipeline let output_str = crate::tools::execute::execute_tool_with_safety( - &tools, - &safety, - tool_name, - &normalized_params, - &job_ctx, + &tools, &safety, tool_name, params, &job_ctx, ) .await?; diff --git a/src/tools/execute.rs b/src/tools/execute.rs index 86da157b..69c72e46 100644 --- a/src/tools/execute.rs +++ b/src/tools/execute.rs @@ -19,7 +19,7 @@ pub async fn execute_tool_with_safety( tools: &ToolRegistry, safety: &SafetyLayer, tool_name: &str, - params: &serde_json::Value, + params: serde_json::Value, job_ctx: &JobContext, ) -> Result { if tool_name.is_empty() { @@ -35,7 +35,7 @@ pub async fn execute_tool_with_safety( name: tool_name.to_string(), })?; - let normalized_params = prepare_tool_params(tool.as_ref(), params); + let normalized_params = prepare_tool_params(tool.as_ref(), ¶ms); // Validate tool parameters let validation = safety.validator().validate_tool_params(&normalized_params); @@ -63,10 +63,7 @@ pub async fn execute_tool_with_safety( // Execute with per-tool timeout let timeout = tool.execution_timeout(); let start = std::time::Instant::now(); - let result = tokio::time::timeout(timeout, async { - tool.execute(normalized_params.clone(), job_ctx).await - }) - .await; + let result = tokio::time::timeout(timeout, tool.execute(normalized_params, job_ctx)).await; let elapsed = start.elapsed(); match &result { @@ -149,7 +146,7 @@ pub async fn execute_tool_simple( tools: &ToolRegistry, safety: &SafetyLayer, tool_name: &str, - params: &serde_json::Value, + params: serde_json::Value, job_ctx: &JobContext, ) -> Result { execute_tool_with_safety(tools, safety, tool_name, params, job_ctx) @@ -308,7 +305,7 @@ mod tests { ®istry, &safety, "", - &serde_json::json!({}), + serde_json::json!({}), &test_job_ctx(), ) .await; @@ -331,7 +328,7 @@ mod tests { let params = serde_json::json!({"message": "hello"}); let result = - execute_tool_with_safety(®istry, &safety, "echo", ¶ms, &test_job_ctx()).await; + execute_tool_with_safety(®istry, &safety, "echo", params, &test_job_ctx()).await; assert!(result.is_ok(), "Echo tool should succeed"); let output = result.unwrap(); @@ -350,7 +347,7 @@ mod tests { ®istry, &safety, "nonexistent", - &serde_json::json!({}), + serde_json::json!({}), &test_job_ctx(), ) .await; @@ -373,7 +370,7 @@ mod tests { ®istry, &safety, "fail_tool", - &serde_json::json!({}), + serde_json::json!({}), &test_job_ctx(), ) .await; @@ -397,7 +394,7 @@ mod tests { ®istry, &safety, "slow_tool", - &serde_json::json!({}), + serde_json::json!({}), &test_job_ctx(), ) .await; @@ -425,7 +422,7 @@ mod tests { ®istry, &safety, "array_echo", - &serde_json::json!({"values": "[\"1\", \"2\", 3]"}), + serde_json::json!({"values": "[\"1\", \"2\", 3]"}), &test_job_ctx(), ) .await diff --git a/src/worker/container.rs b/src/worker/container.rs index 920cc2ce..e0933975 100644 --- a/src/worker/container.rs +++ b/src/worker/container.rs @@ -462,9 +462,14 @@ impl LoopDelegate for ContainerDelegate { ..Default::default() }; - let result = - execute_tool_simple(&self.tools, &self.safety, &tc.name, &tc.arguments, &job_ctx) - .await; + let result = execute_tool_simple( + &self.tools, + &self.safety, + &tc.name, + tc.arguments.clone(), + &job_ctx, + ) + .await; self.post_event( "tool_result",