Skip to content

Commit bf22cc5

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

File tree

3 files changed

+82
-38
lines changed

3 files changed

+82
-38
lines changed

.github/actions/validate-sdk/action.yaml

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,19 +43,24 @@ runs:
4343
working-directory: examples/nextjs-example
4444
run: npm install
4545

46+
- name: Setup pnpm
47+
uses: pnpm/action-setup@v4
48+
with:
49+
version: 9
50+
4651
- name: Typecheck nextjs-example
4752
shell: bash
4853
working-directory: examples/nextjs-example
49-
run: npx tsc --noEmit
54+
run: pnpm dlx tsc --noEmit
5055

5156
- name: Run unit tests
5257
shell: bash
5358
env:
5459
OPENROUTER_API_KEY: ${{ inputs.openrouter-api-key }}
55-
run: npx vitest --run --exclude 'tests/e2e/**'
60+
run: pnpm dlx vitest --run --exclude 'tests/e2e/**'
5661

5762
- name: Run e2e tests
5863
shell: bash
5964
env:
6065
OPENROUTER_API_KEY: ${{ inputs.openrouter-api-key }}
61-
run: npx vitest --run tests/e2e/
66+
run: pnpm dlx vitest --run tests/e2e/

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: 14 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,14 @@ 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 results = await Promise.all(toolCallPromises);
122+
123+
// Filter out null results (tools without execute functions)
124+
const roundResults = results.filter((r): r is ToolExecutionResult => r !== null);
123125

124126
toolExecutionResults.push(...roundResults);
125127

0 commit comments

Comments
 (0)