feat(web): show error details for failed tool calls #490
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ use crate::channels::{IncomingMessage, StatusUpdate}; | |
| use crate::context::JobContext; | ||
| use crate::error::Error; | ||
| use crate::llm::{ChatMessage, Reasoning, ReasoningContext, RespondResult}; | ||
| use crate::tools::redact_params; | ||
|
|
||
| /// Result of the agentic loop execution. | ||
| pub(super) enum AgenticLoopResult { | ||
|
|
@@ -321,14 +322,25 @@ impl Agent { | |
| ) | ||
| .await; | ||
|
|
||
| // Record tool calls in the thread with sensitive params redacted. | ||
| // Look up each tool's sensitive_params before acquiring the session lock. | ||
| { | ||
| let mut redacted_args: Vec<serde_json::Value> = | ||
| Vec::with_capacity(tool_calls.len()); | ||
| for tc in &tool_calls { | ||
| let safe = if let Some(tool) = self.tools().get(&tc.name).await { | ||
| redact_params(&tc.arguments, tool.sensitive_params()) | ||
| } else { | ||
| tc.arguments.clone() | ||
| }; | ||
| redacted_args.push(safe); | ||
| } | ||
| let mut sess = session.lock().await; | ||
| if let Some(thread) = sess.threads.get_mut(&thread_id) | ||
| && let Some(turn) = thread.last_turn_mut() | ||
| { | ||
| for (tc, safe_args) in tool_calls.iter().zip(redacted_args) { | ||
| turn.record_tool_call(&tc.name, safe_args); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -357,11 +369,22 @@ impl Agent { | |
| for (idx, original_tc) in tool_calls.iter().enumerate() { | ||
| let mut tc = original_tc.clone(); | ||
|
|
||
| // Fetch the tool upfront so we can redact sensitive params | ||
| // before they touch hooks or approval display. | ||
| let tool_opt = self.tools().get(&tc.name).await; | ||
| let sensitive = tool_opt | ||
| .as_ref() | ||
| .map(|t| t.sensitive_params()) | ||
| .unwrap_or(&[]); | ||
|
|
||
| // Hook: BeforeToolCall (runs before approval so hooks can | ||
| // modify parameters — approval is checked on final params). | ||
| // Hooks receive redacted params so sensitive values are not | ||
| // exposed to hook handlers or their logs. | ||
| let hook_params = redact_params(&tc.arguments, sensitive); | ||
| let event = crate::hooks::HookEvent::ToolCall { | ||
| tool_name: tc.name.clone(), | ||
| parameters: hook_params, | ||
| user_id: message.user_id.clone(), | ||
| context: "chat".to_string(), | ||
| }; | ||
|
Comment on lines
+372
to
390
There was a problem hiding this comment. The This function is used by all chat-path tool calls (both serial and parallel) in
Collaborator
Author
There was a problem hiding this comment. Good catch! Fixed in b42446e — |
||
|
|
@@ -388,8 +411,20 @@ impl Agent { | |
| } | ||
| Ok(crate::hooks::HookOutcome::Continue { | ||
| modified: Some(new_params), | ||
| }) => match serde_json::from_str::<serde_json::Value>(&new_params) { | ||
| Ok(mut parsed) => { | ||
| // Restore original sensitive param values so a hook | ||
| // cannot overwrite them (they were sent as [REDACTED]). | ||
| if let Some(obj) = parsed.as_object_mut() { | ||
| for key in sensitive { | ||
| if let Some(orig_val) = original_tc.arguments.get(*key) | ||
| { | ||
| obj.insert((*key).to_string(), orig_val.clone()); | ||
| } | ||
| } | ||
| } | ||
| tc.arguments = parsed; | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| tool = %tc.name, | ||
|
|
@@ -404,7 +439,7 @@ impl Agent { | |
| // Check if tool requires approval on the final (post-hook) | ||
| // parameters. Skipped when auto_approve_tools is set. | ||
| if !self.config.auto_approve_tools | ||
| && let Some(tool) = tool_opt | ||
| { | ||
| use crate::tools::ApprovalRequirement; | ||
| let needs_approval = match tool.requires_approval(&tc.arguments) { | ||
|
|
@@ -451,14 +486,17 @@ impl Agent { | |
| .execute_chat_tool(&tc.name, &tc.arguments, &job_ctx) | ||
| .await; | ||
|
|
||
| let disp_tool = self.tools().get(&tc.name).await; | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::tool_completed( | ||
| tc.name.clone(), | ||
| &result, | ||
| &tc.arguments, | ||
| disp_tool.as_deref(), | ||
| ), | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
|
|
@@ -499,13 +537,16 @@ impl Agent { | |
| ) | ||
| .await; | ||
|
|
||
| let par_tool = tools.get(&tc.name).await; | ||
| let _ = channels | ||
| .send_status( | ||
| &channel, | ||
| StatusUpdate::tool_completed( | ||
| tc.name.clone(), | ||
| &result, | ||
| &tc.arguments, | ||
| par_tool.as_deref(), | ||
| ), | ||
| &metadata, | ||
| ) | ||
| .await; | ||
|
|
@@ -675,10 +716,15 @@ impl Agent { | |
|
|
||
| // Handle approval if a tool needed it | ||
| if let Some((approval_idx, tc, tool)) = approval_needed { | ||
| // Show redacted params in the approval UI — the user already knows | ||
| // the sensitive value (they provided it); showing it again is | ||
| // unnecessary and creates a leakage path through channel logs. | ||
| let display_params = redact_params(&tc.arguments, tool.sensitive_params()); | ||
| let pending = PendingApproval { | ||
| request_id: Uuid::new_v4(), | ||
| tool_name: tc.name.clone(), | ||
| parameters: tc.arguments.clone(), | ||
| display_parameters: display_params, | ||
| description: tool.description().to_string(), | ||
| tool_call_id: tc.id.clone(), | ||
| context_messages: context_messages.clone(), | ||
|
|
@@ -738,9 +784,10 @@ pub(super) async fn execute_chat_tool_standalone( | |
| .into()); | ||
| } | ||
|
|
||
| let safe_params = redact_params(params, tool.sensitive_params()); | ||
| tracing::debug!( | ||
| tool = %tool_name, | ||
| params = %safe_params, | ||
| "Tool call started" | ||
| ); | ||
|
|
||
|
|
@@ -1122,6 +1169,7 @@ mod tests { | |
| request_id: uuid::Uuid::new_v4(), | ||
| tool_name: "shell".to_string(), | ||
| parameters: serde_json::json!({"command": "echo hi"}), | ||
| display_parameters: serde_json::json!({"command": "echo hi"}), | ||
| description: "Run shell command".to_string(), | ||
| tool_call_id: "call_1".to_string(), | ||
| context_messages: vec![], | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performing
asynccalls (self.tools().get(&tc.name).await) inside a synchronous loop that precedes a mutex lock (session.lock().await) can lead to performance bottlenecks or potential deadlocks. It's generally better to complete allasyncoperations that retrieve data (like fetching tool details and sensitive parameters) before acquiring a long-held lock on shared state.Consider collecting all
toolreferences and theirsensitive_paramsin a separateVecbefore theforloop, and then iterate over thatVecwhen buildingredacted_args.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! Looking at this more closely, the code already does what you're suggesting — the async
.get()calls happen inside the loop (lines 326-332) and thesession.lock().awaitis acquired after the loop completes (line 334). The comment on line 322 even says "Look up each tool's sensitive_params before acquiring the session lock."So the structure is already: collect all redacted args (async, no lock held) → acquire lock → iterate and record. No deadlock or bottleneck risk here.