fix: preserve OpenAI API promise helpers#3624
Conversation
|
Someone is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/ai/tests/openai.test.ts:485-503
**Mock-based tests gated behind `OPENAI_API_KEY`**
Both new regression tests use `conditionalTest`, which is `test.skip` when `OPENAI_API_KEY` is absent. Since these tests rely entirely on the `createMockAPIPromise` mock and never touch the real OpenAI API, they will be silently skipped in any CI environment that doesn't export `OPENAI_API_KEY`, defeating the stated goal of adding regression coverage. Consider switching these two tests (and the matching `responses` one below) to plain `test(...)` so they always run.
### Issue 2 of 3
packages/ai/tests/openai.test.ts:153-168
**`withResponse` mock returns stale `data` instead of awaiting the wrapped output**
`createMockAPIPromise` hard-codes `withResponse` to resolve with `{ data, ... }` where `data` is the raw input value. In production, `preserveAPIPromiseHelpers` replaces the mock's `withResponse` with its own implementation that awaits `wrappedPromise`, so the final `data` comes from the wrapper, not from this mock. The test's assertion `expect(data).toEqual(mockOpenAiChatResponse)` happens to pass only because the wrapper returns the same value — it does not verify that the wrapped `data` is actually flowing through. Consider replacing the mock's hard-coded `data` with a sentinel object so the test would fail if `wrappedPromise` stopped being used as the `data` source.
### Issue 3 of 3
packages/ai/src/openai/index.ts:60-64
**`withResponse` awaits `wrappedPromise`, blocking on PostHog capture**
The implementation awaits both `parentPromise.withResponse()` and `wrappedPromise` inside `Promise.all`. Because `wrappedPromise`'s `.then()` handler does `await captureAiGeneration(...)`, a caller who uses `.withResponse()` will not receive their `{ data, response }` until the PostHog event has been fully sent. This adds observable latency on every `.withResponse()` call. Consider whether PostHog capture should be fire-and-forget here (as it arguably is on the `await promise` path once you discard the capture result).
Reviews (1): Last reviewed commit: "fix: preserve OpenAI API promise helpers" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| conditionalTest('chat completions create preserves OpenAI APIPromise helpers', async () => { | ||
| const promise = client.chat.completions.create({ | ||
| model: 'gpt-4', | ||
| messages: [{ role: 'user', content: 'Hello' }], | ||
| posthogDistinctId: 'test-id', | ||
| }) | ||
|
|
||
| expect(typeof promise.asResponse).toBe('function') | ||
| expect(typeof promise.withResponse).toBe('function') | ||
|
|
||
| const rawResponse = await promise.asResponse() | ||
| const { data, response, request_id } = await promise.withResponse() | ||
|
|
||
| expect(rawResponse.headers.get('x-ratelimit-remaining-requests')).toBe('42') | ||
| expect(response.headers.get('x-ratelimit-remaining-requests')).toBe('42') | ||
| expect(request_id).toBe('req_test') | ||
| expect(data).toEqual(mockOpenAiChatResponse) | ||
| expect(mockPostHogClient.capture).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
OPENAI_API_KEY
Both new regression tests use conditionalTest, which is test.skip when OPENAI_API_KEY is absent. Since these tests rely entirely on the createMockAPIPromise mock and never touch the real OpenAI API, they will be silently skipped in any CI environment that doesn't export OPENAI_API_KEY, defeating the stated goal of adding regression coverage. Consider switching these two tests (and the matching responses one below) to plain test(...) so they always run.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/tests/openai.test.ts
Line: 485-503
Comment:
**Mock-based tests gated behind `OPENAI_API_KEY`**
Both new regression tests use `conditionalTest`, which is `test.skip` when `OPENAI_API_KEY` is absent. Since these tests rely entirely on the `createMockAPIPromise` mock and never touch the real OpenAI API, they will be silently skipped in any CI environment that doesn't export `OPENAI_API_KEY`, defeating the stated goal of adding regression coverage. Consider switching these two tests (and the matching `responses` one below) to plain `test(...)` so they always run.
How can I resolve this? If you propose a fix, please make it concise.| const createMockAPIPromise = <T>(data: T): Promise<T> & { asResponse: jest.Mock; withResponse: jest.Mock } => { | ||
| const response = new Response(JSON.stringify(data), { | ||
| headers: { | ||
| 'x-ratelimit-remaining-requests': '42', | ||
| }, | ||
| status: 200, | ||
| }) | ||
| return Object.assign(Promise.resolve(data), { | ||
| asResponse: jest.fn().mockResolvedValue(response), | ||
| withResponse: jest.fn().mockResolvedValue({ | ||
| data, | ||
| response, | ||
| request_id: 'req_test', | ||
| }), | ||
| }) | ||
| } |
There was a problem hiding this comment.
withResponse mock returns stale data instead of awaiting the wrapped output
createMockAPIPromise hard-codes withResponse to resolve with { data, ... } where data is the raw input value. In production, preserveAPIPromiseHelpers replaces the mock's withResponse with its own implementation that awaits wrappedPromise, so the final data comes from the wrapper, not from this mock. The test's assertion expect(data).toEqual(mockOpenAiChatResponse) happens to pass only because the wrapper returns the same value — it does not verify that the wrapped data is actually flowing through. Consider replacing the mock's hard-coded data with a sentinel object so the test would fail if wrappedPromise stopped being used as the data source.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/tests/openai.test.ts
Line: 153-168
Comment:
**`withResponse` mock returns stale `data` instead of awaiting the wrapped output**
`createMockAPIPromise` hard-codes `withResponse` to resolve with `{ data, ... }` where `data` is the raw input value. In production, `preserveAPIPromiseHelpers` replaces the mock's `withResponse` with its own implementation that awaits `wrappedPromise`, so the final `data` comes from the wrapper, not from this mock. The test's assertion `expect(data).toEqual(mockOpenAiChatResponse)` happens to pass only because the wrapper returns the same value — it does not verify that the wrapped `data` is actually flowing through. Consider replacing the mock's hard-coded `data` with a sentinel object so the test would fail if `wrappedPromise` stopped being used as the `data` source.
How can I resolve this? If you propose a fix, please make it concise.| if (typeof parentPromise.withResponse === 'function') { | ||
| apiPromise.withResponse = async () => { | ||
| const [response, data] = await Promise.all([parentPromise.withResponse(), wrappedPromise]) | ||
| return { ...response, data } as APIPromiseWithResponse<Output> | ||
| } |
There was a problem hiding this comment.
withResponse awaits wrappedPromise, blocking on PostHog capture
The implementation awaits both parentPromise.withResponse() and wrappedPromise inside Promise.all. Because wrappedPromise's .then() handler does await captureAiGeneration(...), a caller who uses .withResponse() will not receive their { data, response } until the PostHog event has been fully sent. This adds observable latency on every .withResponse() call. Consider whether PostHog capture should be fire-and-forget here (as it arguably is on the await promise path once you discard the capture result).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/src/openai/index.ts
Line: 60-64
Comment:
**`withResponse` awaits `wrappedPromise`, blocking on PostHog capture**
The implementation awaits both `parentPromise.withResponse()` and `wrappedPromise` inside `Promise.all`. Because `wrappedPromise`'s `.then()` handler does `await captureAiGeneration(...)`, a caller who uses `.withResponse()` will not receive their `{ data, response }` until the PostHog event has been fully sent. This adds observable latency on every `.withResponse()` call. Consider whether PostHog capture should be fire-and-forget here (as it arguably is on the `await promise` path once you discard the capture result).
How can I resolve this? If you propose a fix, please make it concise.
|
cc @PostHog/team-ai-observability |
pntech20
force-pushed
the
codex/preserve-openai-api-promise
branch
from
317f875 to
86437db
Compare
|
Pushed a follow-up on this branch:
I could not run the package tests in this checkout because |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
2 participants
Summary
chat.completions.create,responses.create,responses.parse, andembeddings.create.asResponse()/.withResponse()for headers.withResponse()plus a patch changesetFixes #3580.
Verification
npx --yes pnpm@10.23.0 install --frozen-lockfile --filter @posthog/ai...npx --yes pnpm@10.23.0 --filter @posthog/ai exec prettier --check src/openai/index.ts tests/openai.test.ts ../../.changeset/openai-api-promise-helpers.mdnpx --yes pnpm@10.23.0 --filter @posthog/ai exec eslint src/openai/index.ts tests/openai.test.tsOPENAI_API_KEY=test-key npx --yes pnpm@10.23.0 --filter @posthog/ai exec jest --runTestsByPath tests/openai.test.ts --runInBand -t "APIPromise helpers"Notes from this Windows/Node 25 environment:
pnpm --filter @posthog/ai buildis blocked by a missing optional Rollup native package:@rollup/rollup-win32-x64-msvc.tsc --noEmit --project packages/ai/tsconfig.jsonis blocked by workspace packages whose declarations resolve to unbuiltdistoutputs (posthog-node,@posthog/core). After narrowing output, there were no remainingsrc/openai/index.tserrors besides that unresolved workspace import.openai.test.tsfile hits an existing streaming-error test that rethrows from a background async task and exits under Node 25, so I ran the focused APIPromise regression tests directly.