testing pr 217 001

[zboyles]

No description provided.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/zboyles/agent/@convex-dev/agent@10

commit: a6232c5

[Copilot]

Pull request overview

This pull request implements AI SDK v6 tool approval workflow support for @convex-dev/agent, along with dependency updates and various improvements. The PR upgrades the package to require AI SDK v6 (ai@^6.0.0) and adds comprehensive support for tools that need user approval before execution.

Changes:

• Adds tool approval workflow with Agent.approveToolCall() and Agent.denyToolCall() methods
• Implements forceNewOrder mechanism to create separate messages for approval continuations
• Upgrades dependencies to AI SDK v6 (ai@^6.0.35, @ai-sdk/provider-utils@^4.0.6, provider packages v3.x)
• Adds deprecation warnings for v5 API patterns (args → inputSchema, handler → execute, textEmbeddingModel → embeddingModel)
• Improves error handling in streaming with try-catch wrapper
• Refactors React hook to use generation ref pattern instead of isMounted flag
• Adds comprehensive test coverage for approval workflows including edge cases
• Adds ChatApproval example demonstrating the approval UI
• Adds MIGRATION.md and CLAUDE.md documentation files

Reviewed changes

Copilot reviewed 39 out of 42 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
src/client/index.ts	Adds approveToolCall/denyToolCall methods with _findToolCallInfo helper, AI SDK v6 type checks, deprecation warnings
src/client/start.ts	Implements forceNewOrder flag to increment message order for approval continuations
src/client/streamText.ts	Adds try-catch wrapper to handle streaming errors and abort streams properly
src/client/createTool.ts	Updates type definitions to show deprecation errors for args/handler, adds runtime warnings
src/client/types.ts	Adds AssertAISDKv6 type check, unused generic prefix convention, forceNewOrder option
src/shared.ts	Adds createToolModelOutput helper for tool approval responses
src/mapping.ts	Filters out approval parts for provider compatibility, converts execution-denied to text format, improves token detail fields
src/UIMessages.ts	Pre-extracts approval parts before toModelMessage filtering, manages approval state with finalStates protection
src/react/useThreadMessages.ts	Replaces isMounted flag with generationRef for race condition handling
src/component/messages.ts	Adds overrideOrder parameter for forceNewOrder support
example/convex/chat/approval.ts	New file demonstrating tool approval workflow
example/ui/chat/ChatApproval.tsx	New React UI for tool approval with approve/deny buttons
MIGRATION.md	New comprehensive migration guide from v0.3.x to v0.6.0
CLAUDE.md	New AI assistant guidance document
docs/*	Typo fixes ("an pass" → "and pass", "the" → "then")
package.json	Updates dependencies: @convex-dev/rag@0.7.0, convex@^1.31.6, adds MIGRATION.md to files

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The _findToolCallInfo method only retrieves 20 messages from the thread and searches within them. This creates a limitation where approval requests older than 20 messages cannot be found. This is documented in the test file approval-bugs.test.ts lines 38-89, but it's a significant operational issue.

Consider either:

1. Increasing the pagination limit (though this has performance implications)
2. Fetching additional pages if not found in the first 20
3. Adding an index on approval IDs for direct lookup
4. Documenting this limitation prominently in the API docs for approveToolCall and denyToolCall

[Copilot]

There is a race condition in the approval workflow as documented in approval-bugs.test.ts lines 581-607. The approveToolCall and denyToolCall methods use separate transactions: one to check if an approval exists via _findToolCallInfo (which calls listMessages), and another to save the approval response via saveMessage. This creates a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability where two concurrent approval calls could both pass the check and both save responses.

This should be fixed by either:

1. Moving the check-and-write into a single mutation
2. Using optimistic concurrency control (e.g., a unique index on approvalId)
3. At minimum, documenting this limitation prominently

[Copilot]

The _findToolCallInfo method performs three sequential passes through the message list, which is inefficient. All three pieces of information (approval response, approval request, and tool call) could be extracted in a single pass.

Consider refactoring to:

for (const msg of messagesResult.page) {
  if (!Array.isArray(msg.message?.content)) continue;
  
  for (const part of msg.message.content) {
    // Check all three types in one pass
    if (part.type === "tool-approval-response" && ...) { ... }
    if (part.type === "tool-approval-request" && ...) { ... }
    if (part.type === "tool-call" && ...) { ... }
  }
}

[Copilot]

When a tool-call and tool-approval-request are in different messages (as tested in approval-bugs.test.ts lines 142-195), the parentMessageId will point to the approval request message rather than the tool-call message. This could cause the approval response to be saved with an incorrect parent relationship, potentially breaking message threading or order logic.

Consider validating that the approval request and tool-call are in the same message, or explicitly handling this edge case.

[Copilot]

The comment says "TypeScript can't infer order is defined here from the compound condition above" but the non-null assertion order! on line 169 could still fail at runtime if order is undefined. While useForceNewOrder checks order !== undefined, TypeScript correctly doesn't narrow the type because order could be reassigned between lines 164-166.

Consider using a type assertion or restructuring the logic:

if (useForceNewOrder) {
  if (order === undefined) {
    throw new Error("Cannot use forceNewOrder without an existing order");
  }
  order = order + 1;
  stepOrder = 0;
}

[Copilot]

Unused variable action.

Suggested change

const action = actionGeneric as ActionBuilder<DataModel, "public">;

[Copilot]

Unused variable noApprovalAgent.

Suggested change

	// Agent for testing tool execution without approval
	const noApprovalAgent = new Agent(components.agent, {
	name: "no-approval-agent",
	instructions: "Test agent without approval requirement",
	tools: {
	checkBalance: checkBalanceTool,
	},
	languageModel: mockModel({
	contentSteps: [
	[
	{
	type: "tool-call",
	toolCallId: "call-456",
	toolName: "checkBalance",
	input: JSON.stringify({ accountId: "ABC123" }),
	},
	],
	[{ type: "text", text: "Your balance is $500." }],
	],
	}),
	stopWhen: stepCountIs(5),
	});

[Copilot]

Unused variable initialOrder.

Suggested change

const initialOrder = firstMsg!.order;

[Copilot]

The value assigned to streamConsumed here is unused.

[Copilot]

This negation always evaluates to true.