Skip to content

Commit a0a6339

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
1 parent 9073996 commit a0a6339

File tree

2 files changed

+68
-26
lines changed

2 files changed

+68
-26
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

0 commit comments

Comments
 (0)