fix: address review feedback on idempotency cache

- Remove is_idempotent from tools with mutable external state: time,
  read_file, list_dir, list_jobs, job_status, memory_search,
  memory_read, memory_tree. Only truly pure tools (echo, json) remain.
- Tighten is_idempotent() doc to require pure-function semantics (no
  external state dependency, no side effects).
- Fix cache key canonicalization: recursively sort JSON object keys via
  BTreeMap so {"a":1,"b":2} and {"b":2,"a":1} produce the same hash.
  Added two tests for order independence (flat and nested).
- Move worker cache lookup from before approval/hooks/validation to
  after, so those checks always run even on cache hits. Cache now keys
  on post-hook params for consistency between get and put.
- Merge origin/main to pick up latest changes.

Co-Authored-By: Claude Opus 4.6 <[email protected]>

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, &params)
+                .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(&params, tool.sensitive_params());

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
     }

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.

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"))]

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(

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<String, serde_json::Value> =
+                    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());

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
     }