mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-26 07:30:11 +00:00
feat(web): show error details for failed tool calls (#490)
* feat(web): show error details and input params for failed tool calls Failed tool calls in the gateway UI previously showed only a red X icon with an empty expandable body. This change: - Adds optional `error` and `parameters` fields to `ToolCompleted` SSE events so the browser receives failure details in real-time - Auto-expands failed tool cards to make errors immediately visible - Adds `StatusUpdate::tool_completed()` constructor that centralizes the 5 duplicated construction sites and applies `redact_params()` to prevent sensitive values (e.g. secret_save's "value" param) from leaking through SSE broadcasts - Adds `sensitive_params()` trait method to `Tool` for declaring which parameters must be redacted before logging, hooks, and UI display - Adds `redact_params()` utility and wires it through hooks, approvals, ActionRecord storage, and debug logs in dispatcher/worker - Adds `SecretListTool` and `SecretDeleteTool` for LLM-driven secret management (values never returned, only names/metadata) - Fixes auth flow: setup-only extensions show configure modal instead of OAuth card; auth_completed SSE dismisses both UI paths - CI: release workflow creates PR instead of pushing directly to main - Registry: MissingChecksum error enables source fallback for bootstrapping when checksums haven't been populated yet Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * style: apply cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: keep original params in PendingApproval for execution, redact only for display Address two PR review comments: 1. execute_chat_tool_standalone now redacts sensitive params before logging, matching the pattern already used in worker.rs. 2. PendingApproval previously stored redacted parameters, which meant approved tool calls received "[REDACTED]" instead of the actual values. Add a display_parameters field for UI/logs and keep parameters as the original values used for execution. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address PR review comments - worker.rs: redact sensitive params before BeforeToolCall hook, matching dispatcher.rs — hooks in the autonomous job path now receive redacted params instead of raw values - registry.rs: fix docstring for register_secrets_tools (list, delete, not save/list/delete — no SecretSaveTool is registered) - app.js: fix double toast/loadExtensions in submitConfigureModal — for non-OAuth success the auth_completed SSE already handles both, so skip them in the HTTP response handler to avoid duplicates [skip-regression-check] Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
Henry Park
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
13697976db
commit
902492bcdb
+65
-17
@@ -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
|
||||
// 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 in &tool_calls {
|
||||
turn.record_tool_call(&tc.name, tc.arguments.clone());
|
||||
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)
|
||||
// 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: tc.arguments.clone(),
|
||||
parameters: hook_params,
|
||||
user_id: message.user_id.clone(),
|
||||
context: "chat".to_string(),
|
||||
};
|
||||
@@ -388,8 +411,20 @@ impl Agent {
|
||||
}
|
||||
Ok(crate::hooks::HookOutcome::Continue {
|
||||
modified: Some(new_params),
|
||||
}) => match serde_json::from_str(&new_params) {
|
||||
Ok(parsed) => tc.arguments = parsed,
|
||||
}) => 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) = self.tools().get(&tc.name).await
|
||||
&& 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::ToolCompleted {
|
||||
name: tc.name.clone(),
|
||||
success: result.is_ok(),
|
||||
},
|
||||
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::ToolCompleted {
|
||||
name: tc.name.clone(),
|
||||
success: result.is_ok(),
|
||||
},
|
||||
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 = %params,
|
||||
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![],
|
||||
|
||||
@@ -148,8 +148,12 @@ pub struct PendingApproval {
|
||||
pub request_id: Uuid,
|
||||
/// Tool name requiring approval.
|
||||
pub tool_name: String,
|
||||
/// Tool parameters.
|
||||
/// Tool parameters (original values, used for execution).
|
||||
pub parameters: serde_json::Value,
|
||||
/// Redacted tool parameters (sensitive values replaced with `[REDACTED]`).
|
||||
/// Used for display in approval UI, logs, and SSE broadcasts.
|
||||
#[serde(default)]
|
||||
pub display_parameters: serde_json::Value,
|
||||
/// Description of what the tool will do.
|
||||
pub description: String,
|
||||
/// Tool call ID from LLM (for proper context continuation).
|
||||
@@ -950,6 +954,7 @@ mod tests {
|
||||
request_id: Uuid::new_v4(),
|
||||
tool_name: "shell".to_string(),
|
||||
parameters: serde_json::json!({"command": "rm -rf /"}),
|
||||
display_parameters: serde_json::json!({"command": "rm -rf /"}),
|
||||
description: "dangerous command".to_string(),
|
||||
tool_call_id: "call_123".to_string(),
|
||||
context_messages: vec![ChatMessage::user("do it")],
|
||||
@@ -974,6 +979,7 @@ mod tests {
|
||||
request_id: Uuid::new_v4(),
|
||||
tool_name: "http".to_string(),
|
||||
parameters: serde_json::json!({}),
|
||||
display_parameters: serde_json::json!({}),
|
||||
description: "test".to_string(),
|
||||
tool_call_id: "call_456".to_string(),
|
||||
context_messages: vec![],
|
||||
|
||||
+26
-15
@@ -21,6 +21,7 @@ use crate::channels::{IncomingMessage, StatusUpdate};
|
||||
use crate::context::JobContext;
|
||||
use crate::error::Error;
|
||||
use crate::llm::ChatMessage;
|
||||
use crate::tools::redact_params;
|
||||
|
||||
impl Agent {
|
||||
/// Hydrate a historical thread from DB into memory if not already present.
|
||||
@@ -357,7 +358,7 @@ impl Agent {
|
||||
let request_id = pending.request_id;
|
||||
let tool_name = pending.tool_name.clone();
|
||||
let description = pending.description.clone();
|
||||
let parameters = pending.parameters.clone();
|
||||
let parameters = pending.display_parameters.clone();
|
||||
thread.await_approval(pending);
|
||||
let _ = self
|
||||
.channels
|
||||
@@ -751,14 +752,17 @@ impl Agent {
|
||||
.execute_chat_tool(&pending.tool_name, &pending.parameters, &job_ctx)
|
||||
.await;
|
||||
|
||||
let tool_ref = self.tools().get(&pending.tool_name).await;
|
||||
let _ = self
|
||||
.channels
|
||||
.send_status(
|
||||
&message.channel,
|
||||
StatusUpdate::ToolCompleted {
|
||||
name: pending.tool_name.clone(),
|
||||
success: tool_result.is_ok(),
|
||||
},
|
||||
StatusUpdate::tool_completed(
|
||||
pending.tool_name.clone(),
|
||||
&tool_result,
|
||||
&pending.display_parameters,
|
||||
tool_ref.as_deref(),
|
||||
),
|
||||
&message.metadata,
|
||||
)
|
||||
.await;
|
||||
@@ -908,14 +912,17 @@ impl Agent {
|
||||
.execute_chat_tool(&tc.name, &tc.arguments, &job_ctx)
|
||||
.await;
|
||||
|
||||
let deferred_tool = self.tools().get(&tc.name).await;
|
||||
let _ = self
|
||||
.channels
|
||||
.send_status(
|
||||
&message.channel,
|
||||
StatusUpdate::ToolCompleted {
|
||||
name: tc.name.clone(),
|
||||
success: result.is_ok(),
|
||||
},
|
||||
StatusUpdate::tool_completed(
|
||||
tc.name.clone(),
|
||||
&result,
|
||||
&tc.arguments,
|
||||
deferred_tool.as_deref(),
|
||||
),
|
||||
&message.metadata,
|
||||
)
|
||||
.await;
|
||||
@@ -957,13 +964,16 @@ impl Agent {
|
||||
)
|
||||
.await;
|
||||
|
||||
let par_tool = tools.get(&tc.name).await;
|
||||
let _ = channels
|
||||
.send_status(
|
||||
&channel,
|
||||
StatusUpdate::ToolCompleted {
|
||||
name: tc.name.clone(),
|
||||
success: result.is_ok(),
|
||||
},
|
||||
StatusUpdate::tool_completed(
|
||||
tc.name.clone(),
|
||||
&result,
|
||||
&tc.arguments,
|
||||
par_tool.as_deref(),
|
||||
),
|
||||
&metadata,
|
||||
)
|
||||
.await;
|
||||
@@ -1086,6 +1096,7 @@ impl Agent {
|
||||
request_id: Uuid::new_v4(),
|
||||
tool_name: tc.name.clone(),
|
||||
parameters: tc.arguments.clone(),
|
||||
display_parameters: redact_params(&tc.arguments, tool.sensitive_params()),
|
||||
description: tool.description().to_string(),
|
||||
tool_call_id: tc.id.clone(),
|
||||
context_messages: context_messages.clone(),
|
||||
@@ -1095,7 +1106,7 @@ impl Agent {
|
||||
let request_id = new_pending.request_id;
|
||||
let tool_name = new_pending.tool_name.clone();
|
||||
let description = new_pending.description.clone();
|
||||
let parameters = new_pending.parameters.clone();
|
||||
let parameters = new_pending.display_parameters.clone();
|
||||
|
||||
{
|
||||
let mut sess = session.lock().await;
|
||||
@@ -1162,7 +1173,7 @@ impl Agent {
|
||||
let request_id = new_pending.request_id;
|
||||
let tool_name = new_pending.tool_name.clone();
|
||||
let description = new_pending.description.clone();
|
||||
let parameters = new_pending.parameters.clone();
|
||||
let parameters = new_pending.display_parameters.clone();
|
||||
thread.await_approval(new_pending);
|
||||
let _ = self
|
||||
.channels
|
||||
|
||||
+10
-6
@@ -18,8 +18,8 @@ use crate::llm::{
|
||||
ActionPlan, ChatMessage, LlmProvider, Reasoning, ReasoningContext, RespondResult, ToolSelection,
|
||||
};
|
||||
use crate::safety::SafetyLayer;
|
||||
use crate::tools::ToolRegistry;
|
||||
use crate::tools::rate_limiter::RateLimitResult;
|
||||
use crate::tools::{ToolRegistry, redact_params};
|
||||
|
||||
/// Shared dependencies for worker execution.
|
||||
///
|
||||
@@ -700,9 +700,10 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
// Run BeforeToolCall hook
|
||||
let params = {
|
||||
use crate::hooks::{HookError, HookEvent, HookOutcome};
|
||||
let hook_params = redact_params(params, tool.sensitive_params());
|
||||
let event = HookEvent::ToolCall {
|
||||
tool_name: tool_name.to_string(),
|
||||
parameters: params.clone(),
|
||||
parameters: hook_params,
|
||||
user_id: job_ctx.user_id.clone(),
|
||||
context: format!("job:{}", job_id),
|
||||
};
|
||||
@@ -758,9 +759,12 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
.into());
|
||||
}
|
||||
|
||||
// Redact sensitive parameter values (e.g. secret_save's "value") before
|
||||
// they touch any observability or audit path.
|
||||
let safe_params = redact_params(¶ms, tool.sensitive_params());
|
||||
tracing::debug!(
|
||||
tool = %tool_name,
|
||||
params = %params,
|
||||
params = %safe_params,
|
||||
job = %job_id,
|
||||
"Tool call started"
|
||||
);
|
||||
@@ -812,7 +816,7 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
match deps
|
||||
.context_manager
|
||||
.update_memory(job_id, |mem| {
|
||||
let rec = mem.create_action(tool_name, params.clone()).succeed(
|
||||
let rec = mem.create_action(tool_name, safe_params.clone()).succeed(
|
||||
output_str.clone(),
|
||||
output.result.clone(),
|
||||
elapsed,
|
||||
@@ -834,7 +838,7 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
.context_manager
|
||||
.update_memory(job_id, |mem| {
|
||||
let rec = mem
|
||||
.create_action(tool_name, params.clone())
|
||||
.create_action(tool_name, safe_params.clone())
|
||||
.fail(e.to_string(), elapsed);
|
||||
mem.record_action(rec.clone());
|
||||
rec
|
||||
@@ -853,7 +857,7 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
.context_manager
|
||||
.update_memory(job_id, |mem| {
|
||||
let rec = mem
|
||||
.create_action(tool_name, params.clone())
|
||||
.create_action(tool_name, safe_params.clone())
|
||||
.fail("Execution timeout", elapsed);
|
||||
mem.record_action(rec.clone());
|
||||
rec
|
||||
|
||||
Reference in New Issue
Block a user