-
Notifications
You must be signed in to change notification settings - Fork 344
Prevent duplicate messages when using addToolApprovalResponse #829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
commit: |
…lResponse This syncs documentation for cloudflare/agents PR #829, which fixes duplicate assistant messages when using addToolApprovalResponse with tools that have needsApproval. Changes: - Add addToolApprovalResponse to useAgentChat API reference - Document tool approval handling with example - Update human-in-the-loop guide to explain the fix - Add changelog entry for the bug fix Related to: cloudflare/agents#829
Claude Code ReviewOverall Assessment: Strong implementation with comprehensive test coverage. One race condition needs addressing. Issues FoundCritical: Race Condition in _applyToolApproval (index.ts:2086-2095)The synchronous SQL update followed by reload creates a race window. Between SQL execution and reload, another client could modify messages, causing lost updates or stale state. Fix: Add await to the SQL query at line 2087. Minor: Potential Stale Message in Broadcast (index.ts:2108-2114)After reloading messages, you call _findMessageByToolCallId again. You could use the already-updated message from line 2079 instead. Strengths
Architecture Alignment
PR resolves #790 effectively. |
|
add a changeset |
🦋 Changeset detectedLatest commit: b359a7e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Resolves #790
Summary
Fixes duplicate assistant messages when using
addToolApprovalResponsewith tools that haveneedsApproval: true.Problem
When a user approves a tool via
addToolApprovalResponseand then callssendMessage(), two assistant messages are persisted with the sametoolCallId:assistant_xxx) stuck ininput-availablestateapproval-respondedstateSolution
Wrap
addToolApprovalResponseinuseAgentChatto notify the server via a newCF_AGENT_TOOL_APPROVALmessage type. The server updates the message in place using the existing ID, preventing duplicates whensendMessage()is called.Changes
CF_AGENT_TOOL_APPROVALmessage type intypes.ts_applyToolApprovalserver method to update message in place inindex.tsaddToolApprovalResponseinuseAgentChatto notify server first inreact.tsxTesting
To test this fix, use the reproduction example on branch
reproduce/duplicate-message-needsApproval:git checkout reproduce/duplicate-message-needsApproval cd examples/approval-bug-repro npm install npm run devaddToolApprovalResponsetoolCallIdGenerated with OpenCode