Skip to content

Commit ed59cd1

Browse files
committed
fix: address PR #49 review feedback
- 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
1 parent 9073996 commit ed59cd1

File tree

2 files changed

+94
-35
lines changed

2 files changed

+94
-35
lines changed

src/lib/response-wrapper.ts

Lines changed: 60 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,20 @@ export class ResponseWrapper {
7070
this.options = options;
7171
}
7272

73+
/**
74+
* Type guard to check if a value is a non-streaming response
75+
*/
76+
private isNonStreamingResponse(value: unknown): value is models.OpenResponsesNonStreamingResponse {
77+
return (
78+
value !== null &&
79+
typeof value === "object" &&
80+
"id" in value &&
81+
"object" in value &&
82+
"output" in value &&
83+
!("toReadableStream" in value)
84+
);
85+
}
86+
7387
/**
7488
* Initialize the stream if not already started
7589
* This is idempotent - multiple calls will return the same promise
@@ -265,21 +279,60 @@ export class ResponseWrapper {
265279
const value = newResult.value;
266280
if (value && typeof value === "object" && "toReadableStream" in value) {
267281
// It's a stream, consume it
268-
const stream = new ReusableReadableStream(value as EventStream<models.OpenResponsesStreamEvent>);
282+
const streamValue = value as EventStream<models.OpenResponsesStreamEvent>;
283+
const stream = new ReusableReadableStream(streamValue);
269284
currentResponse = await consumeStreamForCompletion(stream);
285+
} else if (this.isNonStreamingResponse(value)) {
286+
currentResponse = value;
270287
} else {
271-
currentResponse = value as models.OpenResponsesNonStreamingResponse;
288+
throw new Error("Unexpected response type from API");
272289
}
273290

274291
currentRound++;
275292
}
276293

294+
// Validate the final response has required fields
295+
if (!currentResponse || !currentResponse.id || !currentResponse.output) {
296+
throw new Error("Invalid final response: missing required fields");
297+
}
298+
299+
// Ensure the response is in a completed state (has output content)
300+
if (!Array.isArray(currentResponse.output) || currentResponse.output.length === 0) {
301+
throw new Error("Invalid final response: empty or invalid output");
302+
}
303+
277304
this.finalResponse = currentResponse;
278305
})();
279306

280307
return this.toolExecutionPromise;
281308
}
282309

310+
/**
311+
* Internal helper to get the message after tool execution
312+
*/
313+
private async getMessageInternal(): Promise<models.AssistantMessage> {
314+
await this.executeToolsIfNeeded();
315+
316+
if (!this.finalResponse) {
317+
throw new Error("Response not available");
318+
}
319+
320+
return extractMessageFromResponse(this.finalResponse);
321+
}
322+
323+
/**
324+
* Internal helper to get the text after tool execution
325+
*/
326+
private async getTextInternal(): Promise<string> {
327+
await this.executeToolsIfNeeded();
328+
329+
if (!this.finalResponse) {
330+
throw new Error("Response not available");
331+
}
332+
333+
return extractTextFromResponse(this.finalResponse);
334+
}
335+
283336
/**
284337
* Get the completed message from the response.
285338
* This will consume the stream until completion, execute any tools, and extract the first message.
@@ -290,16 +343,7 @@ export class ResponseWrapper {
290343
return this.messagePromise;
291344
}
292345

293-
this.messagePromise = (async (): Promise<models.AssistantMessage> => {
294-
await this.executeToolsIfNeeded();
295-
296-
if (!this.finalResponse) {
297-
throw new Error("Response not available");
298-
}
299-
300-
return extractMessageFromResponse(this.finalResponse);
301-
})();
302-
346+
this.messagePromise = this.getMessageInternal();
303347
return this.messagePromise;
304348
}
305349

@@ -312,16 +356,7 @@ export class ResponseWrapper {
312356
return this.textPromise;
313357
}
314358

315-
this.textPromise = (async () => {
316-
await this.executeToolsIfNeeded();
317-
318-
if (!this.finalResponse) {
319-
throw new Error("Response not available");
320-
}
321-
322-
return extractTextFromResponse(this.finalResponse);
323-
})();
324-
359+
this.textPromise = this.getTextInternal();
325360
return this.textPromise;
326361
}
327362

@@ -501,7 +536,9 @@ export class ResponseWrapper {
501536
const consumer = this.reusableStream.createConsumer();
502537

503538
for await (const event of consumer) {
504-
if (!("type" in event)) continue;
539+
if (!("type" in event)) {
540+
continue;
541+
}
505542

506543
// Transform responses events to chat-like format
507544
// This is a simplified transformation - you may need to adjust based on your needs

src/lib/tool-orchestrator.ts

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -88,26 +88,23 @@ export async function executeToolLoop(
8888
break;
8989
}
9090

91-
// Execute all tool calls
92-
const roundResults: ToolExecutionResult[] = [];
93-
94-
for (const toolCall of toolCalls) {
91+
// Execute all tool calls in parallel (parallel tool calling)
92+
const toolCallPromises = toolCalls.map(async (toolCall) => {
9593
const tool = findToolByName(tools, toolCall.name);
9694

9795
if (!tool) {
9896
// Tool not found in definitions
99-
roundResults.push({
97+
return {
10098
toolCallId: toolCall.id,
10199
toolName: toolCall.name,
102100
result: null,
103101
error: new Error(`Tool "${toolCall.name}" not found in tool definitions`),
104-
});
105-
continue;
102+
} as ToolExecutionResult;
106103
}
107104

108105
if (!hasExecuteFunction(tool)) {
109-
// Tool has no execute function - skip
110-
continue;
106+
// Tool has no execute function - return null to filter out
107+
return null;
111108
}
112109

113110
// Build turn context
@@ -117,9 +114,34 @@ export async function executeToolLoop(
117114
};
118115

119116
// Execute the tool
120-
const result = await executeTool(tool, toolCall, turnContext, onPreliminaryResult);
121-
roundResults.push(result);
122-
}
117+
return executeTool(tool, toolCall, turnContext, onPreliminaryResult);
118+
});
119+
120+
// Wait for all tool executions to complete in parallel
121+
const settledResults = await Promise.allSettled(toolCallPromises);
122+
123+
// Process settled results, handling both fulfilled and rejected promises
124+
const roundResults: ToolExecutionResult[] = [];
125+
settledResults.forEach((settled, i) => {
126+
const toolCall = toolCalls[i];
127+
if (!toolCall) return;
128+
129+
if (settled.status === "fulfilled") {
130+
if (settled.value !== null) {
131+
roundResults.push(settled.value);
132+
}
133+
} else {
134+
// Promise rejected - create error result
135+
roundResults.push({
136+
toolCallId: toolCall.id,
137+
toolName: toolCall.name,
138+
result: null,
139+
error: settled.reason instanceof Error
140+
? settled.reason
141+
: new Error(String(settled.reason)),
142+
});
143+
}
144+
});
123145

124146
toolExecutionResults.push(...roundResults);
125147

0 commit comments

Comments
 (0)