-
Notifications
You must be signed in to change notification settings - Fork 11
Add callModel API with multiple consumption patterns #49
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
Conversation
e442a2c to
01720f2
Compare
- Merged latest changes from main branch - Preserved getResponse feature files - Fixed eslint error in reusable-stream.ts - Regenerated SDK with speakeasy run
22fb462 to
aedcf10
Compare
Renamed all getResponse references to callModel throughout the codebase.
- Added callModel method to OpenRouter class using Speakeasy code regions - Exported tool types and ResponseWrapper from index
Add ToolType enum export along with EnhancedTool and MaxToolRounds type exports to make them accessible from the main SDK entry point.
Update getNewMessagesStream to yield ToolResponseMessage objects after tool execution completes, in addition to AssistantMessages. This allows consumers to receive the full message flow including tool call results.
- Add comprehensive tests for AssistantMessage shape validation - Add tests for ToolResponseMessage shape with toolCallId validation - Verify tool execution results match expected output schema - Validate message ordering (tool responses before final assistant) - Fix getNewMessagesStream to yield final assistant message after tools
- Validate all ChatStreamEvent types (content.delta, message.complete, tool.preliminary_result, pass-through) - Check required fields and types for each event type - Test content.delta has proper delta string - Test tool.preliminary_result has toolCallId and result - Use generator tool to test preliminary results when available
Split test run into unit tests and e2e tests for better visibility and clearer CI output.
The generated SDK code has type errors in Zod schemas where properties are optional but should be required. This causes examples typecheck to fail when importing from the SDK. TODO: Re-enable when Speakeasy fixes the generated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we should use pnpm + pnpm dlx for these actions still, unless we think there's no gain using pnpm -- at that point I'd want to move all to npm
| const consumer = this.reusableStream.createConsumer(); | ||
|
|
||
| for await (const event of consumer) { | ||
| if (!("type" in event)) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add bracket
| this.messagePromise = (async (): Promise<models.AssistantMessage> => { | ||
| await this.executeToolsIfNeeded(); | ||
|
|
||
| if (!this.finalResponse) { | ||
| throw new Error("Response not available"); | ||
| } | ||
|
|
||
| return extractMessageFromResponse(this.finalResponse); | ||
| })(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads like a weird pattern -- would like to improve the code quaity.
| const stream = new ReusableReadableStream(value as EventStream<models.OpenResponsesStreamEvent>); | ||
| currentResponse = await consumeStreamForCompletion(stream); | ||
| } else { | ||
| currentResponse = value as models.OpenResponsesNonStreamingResponse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these type cast didn't feel safe
| } | ||
|
|
||
| this.finalResponse = currentResponse; | ||
| })(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an iife here and I'm a bit iffy about them
subtleGradient
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review: PR #49 - feat: expose callModel from SDK client
Summary
This PR adds a comprehensive callModel API to the SDK that supports automatic tool execution with Zod schema validation and multiple response consumption patterns.
Good
- Excellent test coverage - Comprehensive tests for all major functionality including edge cases, error handling, and type inference (52+ tool-related tests)
- Strong type safety - Effective use of TypeScript generics and Zod for compile-time and runtime validation
- Multiple consumption patterns - Flexible API supporting getText(), getMessage(), streaming, and concurrent consumers via ReusableReadableStream
Questions & Suggestions
[question] Implementation complexity
The implementation adds ~2000 lines across 6 new files. While each file has clear responsibilities, is this level of complexity necessary? Could some utilities be simplified or consolidated?
- Evidence: [CODE] 6 new files totaling ~1886 lines for the feature
[question] Tool orchestration limitation
The tool orchestrator mentions using previousResponseId for continuation but doesn't implement it (src/lib/tool-orchestrator.ts:129-134). Is this a known limitation with the current API?
- Evidence: [CODE] Comment indicates planned functionality not yet implemented
[nit] Examples typecheck disabled
CI typecheck for examples is disabled due to "Speakeasy generation bugs". Consider adding a tracking issue to re-enable when the upstream issue is resolved.
- Evidence: [CODE] .github/actions/validate-sdk/action.yaml - typecheck commented out
[nit] Serial tool execution
Tools are executed serially rather than in parallel (src/lib/tool-orchestrator.ts:94-122). For multiple independent tool calls, parallel execution could improve performance.
- Evidence: [CODE] Sequential for-loop for tool execution
Verdict: APPROVE
This is a well-implemented feature with comprehensive testing and good documentation. The code is production-ready with proper error handling and safeguards against infinite loops. The questions raised are about optimization opportunities rather than blocking issues.
- Replace npx with pnpm dlx in CI actions for consistency - Add type guard for safer response type checking - Refactor IIFE patterns in getMessage/getText to private helper methods - Add braces to single-line if statement for readability
- Replace npx with pnpm dlx in CI actions for consistency - Add type guard for safer response type checking - Refactor IIFE patterns in getMessage/getText to private helper methods - Add braces to single-line if statement for readability - Add validation before assigning finalResponse
- Replace npx with pnpm dlx in CI actions for consistency - Add type guard for safer response type checking - Refactor IIFE patterns in getMessage/getText to private helper methods - Add braces to single-line if statement for readability - Add validation before assigning finalResponse - Implement parallel tool calling for improved performance
- Replace npx with pnpm dlx in CI actions for consistency - Add type guard for safer response type checking - Refactor IIFE patterns in getMessage/getText to private helper methods - Add braces to single-line if statement for readability - Add validation before assigning finalResponse - Implement parallel tool calling with Promise.allSettled for better error handling
- Add type guard for safer response type checking - Refactor IIFE patterns in getMessage/getText to private helper methods - Add braces to single-line if statement for readability - Add validation before assigning finalResponse - Implement parallel tool calling with Promise.allSettled for better error handling
Summary
Adds a new
client.callModel()API that provides multiple flexible ways to consume streaming responses from the OpenRouter API with automatic tool execution support.TODO
Tooltype - The Speakeasy-generatedTooltype (fromsrc/models/tool.ts) conflicts with our new flagshipTooltype. The generated type should be renamed (e.g., toChatToolorLegacyTool) in a future PR to avoid naming conflicts.New Types, Methods, and Enums
Enums
ToolType- Enum for tool types (currently supportsFunction = "function")Core Types
Tool Definition Types
Tool- Union type encompassing all tool types with automatic execution (ToolWithExecution | ToolWithStreamingExecution). Represents tools with Zod schemas that are automatically executed by the SDKToolWithExecution- Tool with a regular synchronous or asynchronous execute functionToolWithStreamingExecution- Tool with an async generator execute function that emits progress events during execution. Important: eventSchema events are for your app (e.g., UI updates) and are NOT sent to the model. Only the last yielded value (outputSchema) is sent to the model.TurnContext- Context object passed to tool execute functions containing: numberOfTurns, messageHistory, model/modelsParsedToolCall- Represents a parsed tool call from the API response with id, name, and argumentsToolResult- Result of executing a tool, including toolCallId, toolName, result, preliminaryResults (for streaming tools), and optional errorConfiguration Types
MaxToolRounds- Configuration for tool execution loop limits. Can be a number (max rounds) or a function(context: TurnContext) => booleanthat returns true to continue or false to stopCallModelOptions- Options for creating a ModelResponse, including request, client, options, tools, and maxToolRoundsStream Event Types
ResponsesStreamEvent- Stream event type that extends OpenResponsesStreamEvent with tool progress eventsToolProgressEvent- Event emitted during streaming tool execution containing type, toolCallId, result, and timestampToolStreamEvent- Stream events for tool execution, including delta (argument streaming) and progressChatStreamEvent- Stream events for chat format including content.delta, message.complete, tool.progress, and pass-through eventsClasses
ModelResponse- Main class for consuming API responses with multiple patterns. Provides methods for streaming and awaiting completionMethods (ModelResponse)
getMessage()- Returns a Promise that resolves to the complete AssistantMessage (tools auto-executed if provided)getText()- Returns a Promise that resolves to just the text content from the response (tools auto-executed if provided)getFullResponsesStream()- Returns an AsyncIterableIterator of ResponsesStreamEvent for all response events including tool progressgetTextStream()- Returns an AsyncIterableIterator of string for streaming text content deltasgetNewMessagesStream()- Returns an AsyncIterableIterator of AssistantMessage for streaming incremental message updatesgetReasoningStream()- Returns an AsyncIterableIterator of string for streaming reasoning deltas (for models that support reasoning)getToolStream()- Returns an AsyncIterableIterator of ToolStreamEvent for streaming tool call arguments and progress resultsgetFullChatStream()- Returns an AsyncIterableIterator of ChatStreamEvent for streaming in a chat-friendly format with content deltas, completion events, and tool progressgetToolCalls()- Returns a Promise that resolves to an array of ParsedToolCall from the completed responsegetToolCallsStream()- Returns an AsyncIterableIterator of ParsedToolCall for streaming structured tool calls as they completecancel()- Cancels the underlying stream and cleans up resourcesUtility Functions
isToolProgressEvent(event)- Type guard to check if an event is a ToolProgressEventhasExecuteFunction(tool)- Type guard to check if a tool has an execute functionisStreamingTool(tool)- Type guard to check if a tool uses streaming execution (has eventSchema)isExecutionTool(tool)- Type guard to check if a tool is a regular (non-streaming) execution toolSDK Method
openrouter.callModel(request, options)- New method on OpenRouter client that returns a ModelResponse for consuming responses with automatic tool execution supportFeatures
Tool Support with Context
Tools can now access conversation context during execution:
Streaming Tools with Progress Events
Tools can emit progress events using async generators. Important: These progress events are for your application (e.g., UI updates) and are NOT sent to the model. Only the final result (last yielded value) is sent to the model.
Consumption Patterns
Users can consume responses in any combination they prefer:
Key Features
maxToolRoundscan be a function for smart terminationAssistantMessagetype consistent with chat API