Add tool streaming #1463
Add tool streaming #1463
Conversation
- Implemented `SSEStreamHelper` and `VercelDataStreamHelper` for streaming tool input and output. - Added methods for writing tool input start, delta, availability, and output (both success and error). - Created unit tests for tool streaming functionality in `tool-streaming.test.ts` to ensure proper behavior. - Updated `Agent` class to utilize streaming helpers for user-visible tool calls. - Mocked streaming methods in `AgentSession.test.ts` for testing purposes. - Enhanced documentation for new streaming methods in `stream-helpers.ts`.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
anubra266
changed the title
|
Claude finished @anubra266's task —— View job Comprehensive Code Review: Tool Streaming ImplementationI've completed a thorough technical review of this tool streaming feature from both engineering and system architecture perspectives. Todo List:
SummaryThis PR implements a comprehensive tool streaming system that provides real-time visibility into tool execution across dual streaming protocols. The implementation is generally well-architected with proper separation of concerns and comprehensive test coverage. Key Strengths1. Clean Architecture & Abstraction
2. Comprehensive Test Coverage
3. Robust Integration
Areas for Consideration1. Type Safety & Interface ConsistencyMedium Priority: Consider strengthening type constraints for tool streaming parameters: // Current (line 24-35 in stream-helpers.ts)
writeToolInputStart(params: { toolCallId: string; toolName: string }): Promise<void>;
// Consider adding branded types for better type safety
type ToolCallId = string & { readonly __brand: unique symbol };
type ToolName = string & { readonly __brand: unique symbol };Rationale: Tool call IDs and tool names are critical identifiers that should maintain referential integrity across the streaming lifecycle. 2. Error Handling & Recovery PatternsMedium Priority: The error handling could be more granular, particularly around stream state validation: // In stream-helpers.ts:575-580, consider more specific error types
async writeToolInputStart(params: { toolCallId: string; toolName: string }): Promise<void> {
if (this.isCompleted) {
// Consider throwing a specific error type vs. silent return
throw new StreamCompletedError('Cannot write tool input to completed stream');
}
// ... rest of implementation
}Rationale: Silent failures in streaming contexts can lead to confusing user experiences where tool execution appears to hang. 3. Potential Race ConditionsLow-Medium Priority: In // Current logic (lines 814-817)
if (this.isTextStreaming || gapFromLastTextEnd < STREAM_TEXT_GAP_THRESHOLD_MS) {
this.queuedEvents.push({ type: 'data-summary', event: summary });
return;
}Consideration: What happens if 4. Memory Management OptimizationLow Priority: The // In SSEStreamHelper.ts:122-128
private getToolIndex(toolCallId: string): number {
const existing = this.toolCallIndexById.get(toolCallId);
if (existing !== undefined) return existing;
const next = this.toolCallIndexById.size; // This only grows
this.toolCallIndexById.set(toolCallId, next);
return next;
}Suggestion: Consider cleanup strategies for long-running sessions or implement bounded growth patterns. 5. API Design ConsiderationsLow Priority: The tool streaming API is quite granular with 7 methods. Consider whether some consolidation might improve developer experience: // Current approach requires multiple calls:
await helper.writeToolInputStart({ toolCallId, toolName });
await helper.writeToolInputDelta({ toolCallId, inputTextDelta });
await helper.writeToolInputAvailable({ toolCallId, toolName, input });
// Alternative fluent API consideration:
await helper.toolStream(toolCallId)
.inputStart(toolName)
.inputDelta(inputTextDelta)
.inputAvailable(input);Trade-off: Current approach is more explicit and easier to test, but fluent APIs can reduce boilerplate. System-Level Architecture AssessmentPositive Design Decisions
Extensibility Considerations
RecommendationsFor Immediate Consideration:
For Future Enhancement:
VerdictThis is a high-quality implementation that follows TypeScript and system architecture best practices. The code is clean, well-tested, and properly integrated. The identified considerations are mostly about future-proofing and edge case handling rather than fundamental flaws. Recommendation: ✅ Approve with minor considerations The implementation successfully achieves its goals while maintaining consistency with existing patterns. The tool streaming functionality will significantly improve user experience by providing real-time visibility into agent tool execution. |
- Enhanced the `/api/chat` endpoint to handle approval responses for tool calls. - Implemented validation for conversation IDs in approval responses to ensure they are explicitly provided. - Updated the Zod schema to include a new part type for tool approval responses. - Added comprehensive unit tests to verify the behavior of approval response handling, including success and idempotent cases. - Ensured proper error handling for missing conversation IDs and unknown tool call IDs.
- Integrated ToolApprovalUiBus to manage approval requests and responses in the chat and data stream routes. - Enhanced the `/api/chat` and `/api/chatDataStream` endpoints to publish and subscribe to approval events. - Implemented unit tests to verify the behavior of approval UI event handling, including success and error cases. - Updated the Agent class to support delegated agent approval workflows. - Ensured proper error handling and validation for tool approval events.
No description provided.