fix: resolve confirmed tools by direct execution to prevent LLM argument drift#972
Draft
mkoruszowic wants to merge 8 commits into
Draft
fix: resolve confirmed tools by direct execution to prevent LLM argument drift#972mkoruszowic wants to merge 8 commits into
mkoruszowic wants to merge 8 commits into
Conversation
…s via direct tool execution
…ia direct execution
Contributor
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 1b3ae55 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Closes #969.
The existing confirmation flow asks the LLM to regenerate the tool call on the continuation turn after a user approves it. Because the LLM often produces semantically-identical but textually-different arguments (reordered nested keys, whitespace, rephrased free-text,
1.0vs1), the freshly computedconfirmation_idhash no longer matches the one the user approved — causing an infinite confirmation loop or a silent termination.This PR adds two complementary fixes:
1. Tool-name fallback in
HookManager(short-term band-aid) — when the exactconfirmation_idmatch fails, fall back to matching bytool_name. Keeps existing flows unblocked while the proper fix rolls out. Will be removed in a follow-up PR once consumers adopt (2).2. Direct tool execution from the chat layer (proper fix) — instead of the LLM re-emitting the tool call, the chat layer:
tool_name,arguments,tool_call_id) inChatContext.state["pending_confirmations"]when it streams aConfirmationRequest.Agent.execute_tool_directly(...)with the stored original arguments, bypassing LLM regeneration entirely.(assistant tool_use, tool result)pair into the same agent'shistory(mutated in place byresolve_pending_confirmations— no second agent, no extra run), so the LLM continues from the executed result without ever re-deciding the call."❌ User declined this action"tool result so the LLM can respond appropriately.Because we replay the original arguments verbatim, the PRE_TOOL confirmation hook's
confirmation_idhash matches by construction — no special bypass logic needed. Non-confirmation PRE_TOOL hooks (validation, access control) still run as before, and adenydecision still blocks execution.Chat-layer usage (one agent, minimal glue)
New public API
Agent.execute_tool_directly(tool_call_id, tool_name, arguments, context) -> ToolCallResultinject_tool_call(history, tool_call_id, tool_name, arguments, result) -> ChatFormat(inragbits.agents.history)ChatInterface.create_pending_confirmation_state(request) -> dict[str, Any]ChatInterface.resolve_pending_confirmations(agent, context) -> list[ChatResponseUnion]— mutatesagent.historyin placeConfirmationRequest.tool_call_idfield (new, required — only constructed internally byHookManager)Example
examples/chat/file_explorer_agent.pyupdated to demonstrate the new pattern using a singleAgentinstance.Test plan
Agent.execute_tool_directlycovering happy path, deny-by-hook, unknown tool, POST_TOOL hook behavior, and prior-confirmation matchinject_tool_callcovering shape, immutability, and result coercionChatInterface.create_pending_confirmation_stateandresolve_pending_confirmationscovering confirmed, declined, no-pending, and unknown-id paths; verifyagent.historyis mutated in placeHookManager.execute_pre_toolwith both new and legacy match paths (260 tests green acrossragbits-agentsandragbits-chat)file_explorer_agentexample (reviewer should verify no loop, tool runs once with original args)Follow-ups (not in this PR)
tool_namefallback fromHookManager._find_confirmationonce internal consumers migrate to the direct-execution pattern.sort_keys, numeric normalization) as defense-in-depth for any remaining hash-matched flows.