fix(agent): detect and escalate repeated identical failing tool calls - #2338
Conversation
github-actions
Bot
added
scope: agent
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Ethereum wallet integration using WalletConnect v2, adding tools for wallet pairing and transaction submission, a callback registry for asynchronous results, and logic within the agentic loop to handle duplicate failing tool calls. Critical feedback identifies a potential deadlock caused by holding a mutex lock across an await point in the session listener and notes that the transaction tool is currently incomplete as it does not actually dispatch requests to the wallet. Further improvements are suggested regarding the security gating of high-risk tools and the robustness of the failure detection logic, which currently relies on fragile string matching.
| let mut client = self.wc_client.lock().await; | ||
| if let Some(ref wc) = *client { | ||
| match wc.next().await { |
There was a problem hiding this comment.
Holding the wc_client Mutex lock across the wc.next().await call in poll_event is a critical concurrency issue. Since poll_event is called in a continuous loop by the WalletConnectListener (in listener.rs), it will hold the lock while awaiting the next network event. This prevents any other part of the system, such as the request method (lines 100-109), from acquiring the lock to send transactions. This effectively deadlocks or severely starves transaction requests while the listener is active. The lock should only be held for short, non-blocking operations, or the client should be designed for concurrent access (e.g., by cloning a handle if the underlying client supports it).
| async fn execute( | ||
| &self, | ||
| params: serde_json::Value, | ||
| ctx: &JobContext, | ||
| ) -> Result<ToolOutput, ToolError> { | ||
| let start = std::time::Instant::now(); | ||
|
|
||
| // Must be paired first. | ||
| if !self.session.is_paired().await { | ||
| return Err(EthereumError::NotPaired.into()); | ||
| } | ||
|
|
||
| // Extract and validate parameters. | ||
| let to = require_str(¶ms, "to")?; | ||
| if !is_valid_eth_address(to) { | ||
| return Err(EthereumError::InvalidAddress { | ||
| address: to.to_string(), | ||
| } | ||
| .into()); | ||
| } | ||
|
|
||
| let value = require_str(¶ms, "value")?; | ||
| let data = params.get("data").and_then(|v| v.as_str()); | ||
| let chain_id = params.get("chain_id").and_then(|v| v.as_u64()); | ||
|
|
||
| // Generate a correlation ID for the async callback. | ||
| let correlation_id = uuid::Uuid::new_v4().to_string(); | ||
|
|
||
| // Extract channel/thread from context metadata for callback routing. | ||
| let channel = ctx | ||
| .metadata | ||
| .get("source_channel") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("unknown") | ||
| .to_string(); | ||
| let thread_id = ctx | ||
| .metadata | ||
| .get("thread_id") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|s| s.to_string()); | ||
|
|
||
| // Register callback so the result can be routed back. | ||
| self.callback_registry | ||
| .register( | ||
| correlation_id.clone(), | ||
| CallbackMetadata { | ||
| tool_name: "wallet_transact".to_string(), | ||
| user_id: ctx.user_id.clone(), | ||
| thread_id, | ||
| channel, | ||
| }, | ||
| ) | ||
| .await; | ||
|
|
||
| let mut result = json!({ | ||
| "status": "pending", | ||
| "correlation_id": correlation_id, | ||
| "to": to, | ||
| "value": value, | ||
| "message": "Transaction submitted to wallet for approval. \ | ||
| The wallet owner must confirm on their device." | ||
| }); | ||
|
|
||
| if data.is_some() { | ||
| result["data_present"] = json!(true); | ||
| } | ||
| if let Some(cid) = chain_id { | ||
| result["chain_id"] = json!(cid); | ||
| } | ||
|
|
||
| Ok(ToolOutput::success(result, start.elapsed())) | ||
| } |
There was a problem hiding this comment.
The WalletTransactTool::execute implementation appears to be incomplete. It validates parameters and registers a callback, but it never actually calls self.session.request() to submit the transaction to the paired wallet. As a result, the tool will always return a "pending" status to the LLM, but no request will ever reach the user's wallet. You should call session.request("eth_sendTransaction", ...) (or the appropriate JSON-RPC method) before returning the success output.
| fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { | ||
| // The wallet itself is the approval mechanism. | ||
| ApprovalRequirement::Never | ||
| } |
There was a problem hiding this comment.
The WalletTransactTool is designated as RiskLevel::High but returns ApprovalRequirement::Never. While Rule 11 allows for autonomous execution after command-level approval, the tool should still be integrated into the approval system to ensure that the initial gating occurs. Per Rule 13, the default for requires_approval_for should be false to support auto-approval, but the tool's requirement level should reflect its risk to trigger the appropriate UI flows and prevent unauthorized interactions or LLM spamming.
References
- For destructive or high-risk tools, user approval should be handled at the command-level (e.g., UI modal) before dispatch, allowing the tool to then run autonomously.
- The default implementation of Tool.requires_approval_for should return false to allow auto-approval to proceed unless a specific override is triggered.
| let all_failed = !tool_results.is_empty() | ||
| && tool_results.iter().all(|m| { | ||
| // Tool failure messages are wrapped by SafetyLayer: | ||
| // <tool_output name="...">Tool 'x' failed: ...</tool_output> | ||
| m.content.contains("failed:") | ||
| && m.content.contains("Tool '") | ||
| }); |
There was a problem hiding this comment.
The all_failed detection logic is brittle as it relies on specific string matching ("failed:" and "Tool '"). This dependency on the exact output format of the SafetyLayer or tools makes the loop-breaking mechanism fragile. If the error message format changes or if a tool returns a success message that happens to contain these substrings (e.g., in a user-provided string), the detection will be inaccurate. Consider using structured error indicators if available in the ChatMessage or tool result to distinguish failures from successes more reliably.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52c328a08a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut result = json!({ | ||
| "status": "pending", | ||
| "correlation_id": correlation_id, | ||
| "to": to, | ||
| "value": value, | ||
| "message": "Transaction submitted to wallet for approval. \ | ||
| The wallet owner must confirm on their device." |
There was a problem hiding this comment.
wallet_transact reports that the transaction was submitted and only registers a callback entry, but this execution path never invokes WalletConnectSession::request(...) (or any other send path) before returning success. As a result, users get a pending correlation ID even though no wallet signature request is actually dispatched, so these entries can only age out via timeout instead of completing.
Useful? React with 👍 / 👎.
| let mut client = self.wc_client.lock().await; | ||
| if let Some(ref wc) = *client { | ||
| match wc.next().await { | ||
| Ok(event) => event, |
There was a problem hiding this comment.
poll_event holds wc_client's mutex guard across wc.next().await, and the background listener calls this in a tight loop. Once paired, that can block other methods that need the same mutex (for example request and client replacement during re-pair) until an event arrives, causing transaction/re-pair operations to hang under idle or low-event conditions.
Useful? React with 👍 / 👎.
| if !crate::llm::llm_signals_tool_intent(&text) { | ||
| consecutive_tool_intent_nudges = 0; | ||
| } | ||
| dup_tracker.reset(); |
There was a problem hiding this comment.
The duplicate-tool tracker reset is placed after the tool-intent nudge branch, but that branch continues early, so text-only tool-intent iterations do not clear duplicate-failure state. This can incorrectly escalate to warning/force_text even when failures are not consecutive iterations, because the counter survives across those nudge-only text responses.
Useful? React with 👍 / 👎.
c76d048 to
a2cbed9
Compare
zmanian
changed the title
…#2240) When a tool call fails, the LLM often retries the exact same call with identical args, repeating up to max_iterations (50 for chat, 10 for jobs) with no mechanism to break the loop. Add a DuplicateToolCallTracker to the agentic loop that fingerprints each batch of tool calls by hashing (tool_name, canonicalized_args). When the same fingerprint appears in consecutive iterations and all tools fail: - After 3 consecutive duplicates: inject a warning message telling the LLM to try a different approach - After 5 consecutive duplicates: set force_text = true to disable tool calls entirely The tracker resets when the LLM calls different tools, any tool succeeds, or a text response is produced. This follows the same counter + threshold + escalation pattern as the existing tool intent nudge and truncation detection mechanisms. Also extracts canonicalize_json_value from agent/routine.rs to util.rs for reuse across modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dbdb088 to
8f8e06b
Compare
ilblackdragon
left a comment
ilblackdragon
left a comment
There was a problem hiding this comment.
Code Review
Overview
Adds a DuplicateToolCallTracker to the shared agentic loop that fingerprints each tool call batch by hashing (tool_name, canonicalized_args). When the LLM repeatedly issues identical failing tool calls:
- 3 consecutive → injects a warning message telling the LLM to try a different approach
- 5 consecutive → sets
force_text = trueto disable tool calls entirely, guaranteeing termination
Resets on: different tools called, any tool succeeds, or text response produced. Follows the existing escalation patterns (tool intent nudge, truncation detection).
Design Assessment
This is well-designed. It correctly follows the existing consecutive_tool_intent_nudges / truncation_count → force_text escalation pattern already in the agentic loop. The three-state progression (silent → warn → force) gives the LLM a chance to self-correct before cutting it off.
All three delegates (ChatDelegate, JobDelegate, ContainerDelegate) are updated to report last_tool_batch_all_failed, which is the right approach — the loop engine detects the pattern, the delegates just report facts.
Code Quality
Positives:
- No
.unwrap()/.expect()in production code tracing::debug!used correctly (notinfo!)- Clean extraction of
canonicalize_json_valuetoutil.rs— theroutine.rslocal function now delegates, no behavior change DuplicateToolCallTrackeris private to the module — correct encapsulation- Thorough unit tests (5 for the tracker, 3 for canonicalization, 3 integration tests for the loop)
- The
ChatDelegatecorrectly counts bothPreflightOutcome::Rejectedand execution errors as failures, while auth-required and needs-approval are not counted (correct — those are flow control)
Issues
| Severity | Issue |
|---|---|
| Low | Batch order sensitivity. Fingerprint hashes tool calls in array order, so [A, B] and [B, A] produce different fingerprints. If the LLM reorders tools within a batch but uses the same calls, the tracker won't detect the duplicate. Rare in practice (LLMs tend to produce consistent ordering), but worth a comment in the fingerprint doc. |
| Low | Warning messages accumulate. At counts 3, 4, and 5, a warning is pushed each time — the LLM sees 3 identical warning messages in context. Consider pushing only at the threshold crossing (count == 3 and count == 5) rather than >=, or accept the accumulation as increasing pressure. |
| Low | DefaultHasher is not guaranteed stable across Rust versions. Fine here since the hash is only compared within a single process lifetime — no persistence or cross-process use. Worth a one-line comment noting this is intentional. |
| Nit | tc.arguments.clone() in fingerprint. Clones the entire JSON value for canonicalization. Fine for typical tool arguments since they're bounded by the LLM output token limit. |
Design Questions
-
Should truncated tool calls (
FinishReason::Length) interact with the duplicate tracker? Currently, truncated batches hit thecontinuepath before reaching the duplicate tracking code. This means a truncated batch that happens to match the previous batch won't increment the duplicate counter. This seems correct (truncated calls are discarded before execution), but worth confirming the intent. -
The
last_tool_batch_all_failedfield onReasoningContextis a mutable flag used as a communication channel between the delegate and the loop. This follows the existingforce_textpattern and works because the loop is single-threaded. The doc comment explains the contract well.
Test Coverage
Excellent:
- 5 tracker unit tests: no-duplicate-on-success, counting, reset-on-success, different-args-restart, canonicalization
- 4 canonicalization tests: key sorting, recursive sorting, array order preservation, scalar passthrough
- 3 integration tests: warning injection at threshold 3, force_text at threshold 5, text-response reset
Verdict: Approve ✓
Clean, well-tested, follows established patterns. The issues are all low-severity suggestions.
…nearai#2240) (nearai#2338) When a tool call fails, the LLM often retries the exact same call with identical args, repeating up to max_iterations (50 for chat, 10 for jobs) with no mechanism to break the loop. Add a DuplicateToolCallTracker to the agentic loop that fingerprints each batch of tool calls by hashing (tool_name, canonicalized_args). When the same fingerprint appears in consecutive iterations and all tools fail: - After 3 consecutive duplicates: inject a warning message telling the LLM to try a different approach - After 5 consecutive duplicates: set force_text = true to disable tool calls entirely The tracker resets when the LLM calls different tools, any tool succeeds, or a text response is produced. This follows the same counter + threshold + escalation pattern as the existing tool intent nudge and truncation detection mechanisms. Also extracts canonicalize_json_value from agent/routine.rs to util.rs for reuse across modules. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Closes #2240
DuplicateToolCallTrackerto the shared agentic loop that fingerprints each tool call batch by hashing(tool_name, canonicalized_args)and detects consecutive identical failing batchesforce_text = trueto disable tool calls entirely, guaranteeing terminationcanonicalize_json_valuefromagent/routine.rstoutil.rsfor shared useReasoningContext.last_tool_batch_all_failedDesign
Follows the same counter + threshold + escalation pattern as the existing tool intent nudge (
consecutive_tool_intent_nudges/max_tool_intent_nudges) and truncation detection (truncation_count/force_text) mechanisms already in the agentic loop.Test plan
DuplicateToolCallTracker(fingerprinting, counting, reset on success, reset on different args, canonicalization of key order)canonicalize_json_valueinutil.rsforce_text = truecargo clippy --all --benches --tests --examples --all-featurespasses with zero warningscargo testpasses (4713 of 4714 pass; 1 pre-existing flaky SQLite test unrelated to this change)Generated with Claude Code