diff --git a/src/agent/worker.rs b/src/agent/worker.rs index 481b945a..05593fa4 100644 --- a/src/agent/worker.rs +++ b/src/agent/worker.rs @@ -746,21 +746,7 @@ Report when the job is complete or if you encounter issues you cannot resolve."# name: tool_name.to_string(), })?; - // Check idempotency cache before any expensive work let job_id_str = job_id.to_string(); - if tool.is_idempotent() - && let Some(cached) = deps - .idempotency_cache - .get(&job_id_str, tool_name, params) - .await - { - tracing::debug!( - tool = %tool_name, - job = %job_id, - "Idempotency cache hit" - ); - return Ok(cached); - } // Check approval: use context-aware check if available, else block all non-Never tools let requirement = tool.requires_approval(params); @@ -857,6 +843,22 @@ Report when the job is complete or if you encounter issues you cannot resolve."# .into()); } + // Check idempotency cache after approval/hooks/validation so those + // checks always run. Uses post-hook params for consistency with put(). + if tool.is_idempotent() + && let Some(cached) = deps + .idempotency_cache + .get(&job_id_str, tool_name, ¶ms) + .await + { + tracing::debug!( + tool = %tool_name, + job = %job_id, + "Idempotency cache hit" + ); + return Ok(cached); + } + // 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()); diff --git a/src/tools/builtin/file.rs b/src/tools/builtin/file.rs index 033d6861..72e0151c 100644 --- a/src/tools/builtin/file.rs +++ b/src/tools/builtin/file.rs @@ -170,10 +170,6 @@ impl Tool for ReadFileTool { true // File content could contain anything } - fn is_idempotent(&self) -> bool { - true - } - fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { ApprovalRequirement::UnlessAutoApproved } @@ -401,10 +397,6 @@ impl Tool for ListDirTool { false // Directory listings are safe } - fn is_idempotent(&self) -> bool { - true - } - fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { ApprovalRequirement::UnlessAutoApproved } diff --git a/src/tools/builtin/job.rs b/src/tools/builtin/job.rs index effab806..f502259f 100644 --- a/src/tools/builtin/job.rs +++ b/src/tools/builtin/job.rs @@ -896,10 +896,6 @@ impl Tool for ListJobsTool { fn requires_sanitization(&self) -> bool { false } - - fn is_idempotent(&self) -> bool { - true - } } /// Tool for checking job status. @@ -979,10 +975,6 @@ impl Tool for JobStatusTool { fn requires_sanitization(&self) -> bool { false } - - fn is_idempotent(&self) -> bool { - true - } } /// Tool for canceling a job. diff --git a/src/tools/builtin/memory.rs b/src/tools/builtin/memory.rs index 5109b92c..71fe8a3b 100644 --- a/src/tools/builtin/memory.rs +++ b/src/tools/builtin/memory.rs @@ -114,10 +114,6 @@ impl Tool for MemorySearchTool { fn requires_sanitization(&self) -> bool { false // Internal memory, trusted content } - - fn is_idempotent(&self) -> bool { - true - } } /// Tool for writing to workspace memory. @@ -381,10 +377,6 @@ impl Tool for MemoryReadTool { fn requires_sanitization(&self) -> bool { false // Internal memory } - - fn is_idempotent(&self) -> bool { - true - } } /// Tool for viewing workspace structure as a tree. @@ -504,10 +496,6 @@ impl Tool for MemoryTreeTool { fn requires_sanitization(&self) -> bool { false // Internal tool } - - fn is_idempotent(&self) -> bool { - true - } } #[cfg(all(test, feature = "postgres"))] diff --git a/src/tools/builtin/time.rs b/src/tools/builtin/time.rs index b188be6d..bafbd4d7 100644 --- a/src/tools/builtin/time.rs +++ b/src/tools/builtin/time.rs @@ -95,10 +95,6 @@ impl Tool for TimeTool { fn requires_sanitization(&self) -> bool { false // Internal tool, no external data } - - fn is_idempotent(&self) -> bool { - true - } } fn execute_now( diff --git a/src/tools/idempotency.rs b/src/tools/idempotency.rs index e010f0c1..8e3ee4d2 100644 --- a/src/tools/idempotency.rs +++ b/src/tools/idempotency.rs @@ -116,16 +116,37 @@ impl ToolIdempotencyCache { } /// Build a deterministic cache key from job_id, tool name, and params. + /// + /// JSON object keys are sorted recursively to ensure order-independent + /// hashing (`{"a":1,"b":2}` and `{"b":2,"a":1}` produce the same key). fn cache_key(job_id: &str, tool_name: &str, params: &serde_json::Value) -> String { let mut hasher = Sha256::new(); hasher.update(tool_name.as_bytes()); hasher.update(b":"); - // Canonical JSON via serde_json::to_string (keys are sorted in serde_json maps) - let params_str = serde_json::to_string(params).unwrap_or_default(); + let canonical = Self::canonicalize(params); + let params_str = serde_json::to_string(&canonical).unwrap_or_default(); hasher.update(params_str.as_bytes()); let hash = format!("{:x}", hasher.finalize()); format!("{}:{}:{}", job_id, tool_name, hash) } + + /// Recursively sort JSON object keys for canonical serialization. + fn canonicalize(value: &serde_json::Value) -> serde_json::Value { + match value { + serde_json::Value::Object(map) => { + let mut sorted: std::collections::BTreeMap = + std::collections::BTreeMap::new(); + for (k, v) in map { + sorted.insert(k.clone(), Self::canonicalize(v)); + } + serde_json::Value::Object(sorted.into_iter().collect()) + } + serde_json::Value::Array(arr) => { + serde_json::Value::Array(arr.iter().map(Self::canonicalize).collect()) + } + other => other.clone(), + } + } } #[cfg(test)] @@ -259,6 +280,31 @@ mod tests { assert_eq!(key1, key2); } + #[tokio::test] + async fn test_cache_key_order_independent() { + // JSON objects with different key insertion order must produce the same cache key + let key1 = + ToolIdempotencyCache::cache_key("j1", "echo", &serde_json::json!({"a": 1, "b": 2})); + let key2 = + ToolIdempotencyCache::cache_key("j1", "echo", &serde_json::json!({"b": 2, "a": 1})); + assert_eq!(key1, key2); + } + + #[tokio::test] + async fn test_cache_key_nested_order_independent() { + let key1 = ToolIdempotencyCache::cache_key( + "j1", + "tool", + &serde_json::json!({"x": {"c": 3, "d": 4}, "y": 1}), + ); + let key2 = ToolIdempotencyCache::cache_key( + "j1", + "tool", + &serde_json::json!({"y": 1, "x": {"d": 4, "c": 3}}), + ); + assert_eq!(key1, key2); + } + #[tokio::test] async fn test_overwrite_existing_entry() { let cache = ToolIdempotencyCache::new(config()); diff --git a/src/tools/tool.rs b/src/tools/tool.rs index 172925a3..8f2fdaef 100644 --- a/src/tools/tool.rs +++ b/src/tools/tool.rs @@ -312,13 +312,21 @@ pub trait Tool: Send + Sync { &[] } - /// Whether this tool produces the same output for the same input. + /// Whether this tool is a pure function of its input parameters. /// - /// Idempotent tools (read-only, pure functions) can have their results cached - /// to avoid re-execution when the LLM re-requests the same tool with identical - /// arguments (common during self-repair recovery or retry loops). + /// A tool marked idempotent must produce the same output for the same input + /// regardless of when it is called — no dependency on external mutable state + /// (filesystem, time, database, network) and no side effects. /// - /// Default: `false`. Override to return `true` for read-only tools. + /// Results of idempotent tools are cached to avoid re-execution when the LLM + /// re-requests the same tool with identical arguments (common during + /// self-repair recovery or retry loops). + /// + /// Examples: `echo` (returns input), `json` (parse/format). + /// Counter-examples: `read_file` (filesystem changes), `time` (clock), + /// `memory_search` (workspace mutations), `list_jobs` (job state changes). + /// + /// Default: `false`. Override to return `true` only for pure functions. fn is_idempotent(&self) -> bool { false }