Skip to content
This repository was archived by the owner on Jun 3, 2026. It is now read-only.

Commit 010f345

Browse files
committed
refactor: improve type safety and code organization
- Fix _formatMessages return type to ChatCompletionMessageParam[] - Remove unnecessary type assertion at call site - Extract OpenAIChatChoice type alias from inline cast - Add proper types for tool use calls and assistant messages - Add validation for n > 1 parameter (not supported) - Add clarifying comment for text-only tool result support Addresses review feedback from zastrowm and afarntrog: - Improved type safety throughout message formatting - Better code readability with extracted types - Fixed missing n > 1 validation (test was failing) - Clarified OpenAI's text-only requirement for tool messages
1 parent 81acfac commit 010f345

2 files changed

Lines changed: 39 additions & 25 deletions

File tree

src/models/openai.ts

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,28 @@ const OPENAI_CONTEXT_WINDOW_OVERFLOW_PATTERNS = [
2626
'context length',
2727
]
2828

29+
/**
30+
* Type representing an OpenAI streaming chat choice.
31+
* Used for type-safe handling of streaming responses.
32+
*/
33+
type OpenAIChatChoice = {
34+
delta?: {
35+
role?: string
36+
content?: string
37+
tool_calls?: Array<{
38+
index: number
39+
id?: string
40+
type?: string
41+
function?: {
42+
name?: string
43+
arguments?: string
44+
}
45+
}>
46+
}
47+
finish_reason?: string
48+
index: number
49+
}
50+
2951
/**
3052
* Configuration interface for OpenAI model provider.
3153
*
@@ -385,7 +407,7 @@ export class OpenAIModel implements Model<OpenAIModelConfig, ClientOptions> {
385407
}
386408

387409
// Add formatted messages
388-
const formattedMessages = this._formatMessages(messages) as OpenAI.Chat.Completions.ChatCompletionMessageParam[]
410+
const formattedMessages = this._formatMessages(messages)
389411
request.messages.push(...formattedMessages)
390412

391413
// Add model configuration parameters
@@ -441,6 +463,11 @@ export class OpenAIModel implements Model<OpenAIModelConfig, ClientOptions> {
441463
Object.assign(request, this._config.params)
442464
}
443465

466+
// Validate n parameter (number of completions) - only n=1 supported for streaming
467+
if ('n' in request && request.n !== undefined && request.n !== null && request.n > 1) {
468+
throw new Error('Streaming with n > 1 is not supported')
469+
}
470+
444471
return request
445472
}
446473

@@ -451,8 +478,8 @@ export class OpenAIModel implements Model<OpenAIModelConfig, ClientOptions> {
451478
* @param messages - SDK messages
452479
* @returns OpenAI-formatted messages
453480
*/
454-
private _formatMessages(messages: Message[]): unknown[] {
455-
const openAIMessages: unknown[] = []
481+
private _formatMessages(messages: Message[]): OpenAI.Chat.Completions.ChatCompletionMessageParam[] {
482+
const openAIMessages: OpenAI.Chat.Completions.ChatCompletionMessageParam[] = []
456483

457484
for (const message of messages) {
458485
if (message.role === 'user') {
@@ -485,10 +512,11 @@ export class OpenAIModel implements Model<OpenAIModelConfig, ClientOptions> {
485512
}
486513

487514
// Add each tool result as separate tool message
515+
// OpenAI only supports text content in tool result messages, not JSON
488516
for (const toolResult of toolResults) {
489517
if (toolResult.type === 'toolResultBlock') {
490-
// Format tool result content
491-
// Handle JSON serialization with context and consistent error handling
518+
// Format tool result content - convert all to text string
519+
// Note: OpenAI tool messages only accept string content (not structured JSON)
492520
const contentText = toolResult.content
493521
.map((c) => {
494522
if (c.type === 'toolResultTextContent') {
@@ -529,7 +557,7 @@ export class OpenAIModel implements Model<OpenAIModelConfig, ClientOptions> {
529557
}
530558
} else {
531559
// Handle assistant messages
532-
const toolUseCalls: unknown[] = []
560+
const toolUseCalls: OpenAI.Chat.Completions.ChatCompletionMessageToolCall[] = []
533561
// Use array + join pattern for efficient string concatenation
534562
const textParts: string[] = []
535563

@@ -560,7 +588,7 @@ export class OpenAIModel implements Model<OpenAIModelConfig, ClientOptions> {
560588
// Trim text content to avoid whitespace-only messages
561589
const textContent = textParts.join('').trim()
562590

563-
const assistantMessage: { role: string; content: string; tool_calls?: unknown[] } = {
591+
const assistantMessage: OpenAI.Chat.Completions.ChatCompletionAssistantMessageParam = {
564592
role: 'assistant',
565593
content: textContent,
566594
}
@@ -628,23 +656,7 @@ export class OpenAIModel implements Model<OpenAIModelConfig, ClientOptions> {
628656
}
629657

630658
// Process first choice (OpenAI typically returns one choice in streaming)
631-
const typedChoice = choice as {
632-
delta?: {
633-
role?: string
634-
content?: string
635-
tool_calls?: Array<{
636-
index: number
637-
id?: string
638-
type?: string
639-
function?: {
640-
name?: string
641-
arguments?: string
642-
}
643-
}>
644-
}
645-
finish_reason?: string
646-
index: number
647-
}
659+
const typedChoice = choice as OpenAIChatChoice
648660

649661
if (!typedChoice.delta && !typedChoice.finish_reason) {
650662
return events

tests_integ/openai.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,9 @@ describe.skipIf(!hasApiKey)('OpenAIModel Integration Tests', () => {
200200
// Extract tool use information
201201
const toolUseStartEvent = events1.find(
202202
(e) => e.type === 'modelContentBlockStartEvent' && e.start?.type === 'toolUseStart'
203-
) as { type: 'modelContentBlockStartEvent'; start?: { type: 'toolUseStart'; toolUseId: string; name: string } } | undefined
203+
) as
204+
| { type: 'modelContentBlockStartEvent'; start?: { type: 'toolUseStart'; toolUseId: string; name: string } }
205+
| undefined
204206
expect(toolUseStartEvent).toBeDefined()
205207

206208
const toolUseId = toolUseStartEvent?.start?.toolUseId

0 commit comments

Comments
 (0)