Auto-deny unresolved tool approvals + tool-approval docs#230
Open
sethconvex wants to merge 1 commit intomainfrom
Open
Auto-deny unresolved tool approvals + tool-approval docs#230sethconvex wants to merge 1 commit intomainfrom
sethconvex wants to merge 1 commit intomainfrom
Conversation
When a user sends a new message instead of resolving pending tool approvals, inject synthetic denial responses rather than silently dropping the orphaned tool calls. This preserves context in the message history and prevents the AI SDK from receiving incomplete tool-call/result pairs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a tool approval feature with automatic denial handling. It adds documentation for the approval workflow, implements Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/tool-approval.mdx (1)
117-128:continueAfterApprovalsusespromptMessageIdfor the last approval message — verify this is the intended resume pattern.Passing
lastApprovalMessageIdaspromptMessageIdmeans the agent will treat the approval message as the "prompt" to resume from. This is a somewhat non-obvious API pattern. The doc could benefit from a brief note explaining why the approval message ID is used as the prompt message ID (i.e., it tells the agent to resume from where the approval was issued).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tool-approval.mdx` around lines 117 - 128, The code passes lastApprovalMessageId into approvalAgent.streamText as promptMessageId which is non-obvious; update the continueAfterApprovals declaration (or surrounding docs) to add a concise comment explaining that passing the approval message ID as promptMessageId tells the agent to resume generation from the approval message, and ensure the handler signature or argument naming (continueAfterApprovals args: lastApprovalMessageId) remains consistent with that explanation so future readers see why approvalAgent.streamText(ctx, { threadId }, { promptMessageId: lastApprovalMessageId }) is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tool-approval.mdx`:
- Around line 148-186: The snippet in Chat references lastApprovalMessageIdRef
and uses useEffect/useRef but never declares or imports them; update the Chat
example to either (a) add the missing import for React hooks (useEffect, useRef)
and declare const lastApprovalMessageIdRef = useRef<string | null>(null) inside
the Chat component before it’s used, or (b) explicitly add a brief inline
comment like “// useRef setup omitted for brevity: lastApprovalMessageIdRef
declared via useRef” next to the usages so readers understand the ref
exists—ensure references to lastApprovalMessageIdRef, useEffect, and useRef
match the symbols in the snippet.
---
Nitpick comments:
In `@docs/tool-approval.mdx`:
- Around line 117-128: The code passes lastApprovalMessageId into
approvalAgent.streamText as promptMessageId which is non-obvious; update the
continueAfterApprovals declaration (or surrounding docs) to add a concise
comment explaining that passing the approval message ID as promptMessageId tells
the agent to resume generation from the approval message, and ensure the handler
signature or argument naming (continueAfterApprovals args:
lastApprovalMessageId) remains consistent with that explanation so future
readers see why approvalAgent.streamText(ctx, { threadId }, { promptMessageId:
lastApprovalMessageId }) is used.
Comment on lines
+148
to
+186
| ```tsx | ||
| import { useUIMessages, type UIMessage } from "@convex-dev/agent/react"; | ||
| import type { ToolUIPart } from "ai"; | ||
|
|
||
| function Chat({ threadId }: { threadId: string }) { | ||
| const { results: messages } = useUIMessages( | ||
| api.chat.approval.listThreadMessages, | ||
| { threadId }, | ||
| { initialNumItems: 10, stream: true }, | ||
| ); | ||
|
|
||
| const submitApproval = useMutation(api.chat.approval.submitApproval); | ||
| const triggerContinuation = useMutation( | ||
| api.chat.approval.triggerContinuation, | ||
| ); | ||
|
|
||
| const hasPendingApprovals = messages.some((m) => | ||
| m.parts.some( | ||
| (p) => | ||
| p.type.startsWith("tool-") && | ||
| (p as ToolUIPart).state === "approval-requested", | ||
| ), | ||
| ); | ||
|
|
||
| // When all approvals are resolved, trigger continuation | ||
| useEffect(() => { | ||
| if (!hasPendingApprovals && lastApprovalMessageIdRef.current) { | ||
| void triggerContinuation({ | ||
| threadId, | ||
| lastApprovalMessageId: lastApprovalMessageIdRef.current, | ||
| }); | ||
| lastApprovalMessageIdRef.current = null; | ||
| } | ||
| }, [hasPendingApprovals, threadId, triggerContinuation]); | ||
|
|
||
| // Render approval buttons for tool parts with state "approval-requested" | ||
| // ... | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Client snippet is incomplete — consider adding a note or expanding it.
The React example references lastApprovalMessageIdRef (line 174, 177, 179) without declaring it, and omits useEffect/useRef imports. While documentation snippets are often abbreviated, this particular example may confuse readers since the ref is central to the continuation logic. Consider either adding a brief // ... useRef setup omitted for brevity comment or showing the ref declaration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tool-approval.mdx` around lines 148 - 186, The snippet in Chat
references lastApprovalMessageIdRef and uses useEffect/useRef but never declares
or imports them; update the Chat example to either (a) add the missing import
for React hooks (useEffect, useRef) and declare const lastApprovalMessageIdRef =
useRef<string | null>(null) inside the Chat component before it’s used, or (b)
explicitly add a brief inline comment like “// useRef setup omitted for brevity:
lastApprovalMessageIdRef declared via useRef” next to the usages so readers
understand the ref exists—ensure references to lastApprovalMessageIdRef,
useEffect, and useRef match the symbols in the snippet.
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
autoDenyUnresolvedApprovals()to inject synthetic denial responses for unresolvedtool-approval-requestparts instead of silently dropping orphaned tool callsfilterOutOrphanedToolMessages()to preserve tool-calls with pending approval requests (auto-deny handles them downstream)fetchContextWithPrompt()aroundmergeApprovalResponseMessages()docs/tool-approval.mdxdocumenting the full tool approval featureTest plan
autoDenyUnresolvedApprovalsunit tests pass (no unresolved, single, multiple, mixed, console.warn)filterOutOrphanedToolMessagestest passes with new expectationsexample/convex/approval.test.ts) still passnpm run typecheck— no type errorsnpm run lint— clean on all changed files🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes