Fix OpenAI embedding provider key selection#832
Conversation
|
@jinzheng8115 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request updates the OpenAI embedding provider initialization to prefer ChangesOpenAI Embedding API Key Preference
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates OpenAI embedding provider initialization to prefer a dedicated embedding API key, and adds test coverage for the new precedence behavior.
Changes:
- Prefer
OPENAI_EMBEDDING_API_KEYoverOPENAI_API_KEYwhen creating the OpenAI embedding provider. - Add a unit test validating the environment variable precedence for OpenAI embeddings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/embedding-provider.test.ts | Adds a test ensuring OPENAI_EMBEDDING_API_KEY takes precedence over OPENAI_API_KEY. |
| src/providers/embedding/index.ts | Updates OpenAI provider creation to use the embedding-specific API key when present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process.env["OPENAI_API_KEY"] = "llm-key-123"; | ||
| process.env["OPENAI_EMBEDDING_API_KEY"] = "embedding-key-456"; |
| const provider = createEmbeddingProvider() as OpenAIEmbeddingProvider & { | ||
| apiKey: string; | ||
| }; | ||
| expect(provider).toBeInstanceOf(OpenAIEmbeddingProvider); | ||
| expect(provider.apiKey).toBe("embedding-key-456"); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/embedding-provider.test.ts (1)
13-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
OPENAI_EMBEDDING_API_KEYin test setup to avoid env-dependent flakes.
beforeEachclearsOPENAI_API_KEYbut notOPENAI_EMBEDDING_API_KEY. If that variable exists in the runner environment, the “returns null when no API keys are set” test can fail nondeterministically.Suggested fix
beforeEach(() => { process.env = { ...originalEnv }; delete process.env["GEMINI_API_KEY"]; delete process.env["OPENAI_API_KEY"]; + delete process.env["OPENAI_EMBEDDING_API_KEY"]; delete process.env["VOYAGE_API_KEY"]; delete process.env["COHERE_API_KEY"]; delete process.env["OPENROUTER_API_KEY"]; delete process.env["EMBEDDING_PROVIDER"]; });Also applies to: 46-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/embedding-provider.test.ts` around lines 13 - 21, The beforeEach hook is clearing several API key environment variables but is missing OPENAI_EMBEDDING_API_KEY, which can cause nondeterministic test failures if this variable exists in the runner environment. Add a line to delete process.env["OPENAI_EMBEDDING_API_KEY"] in the beforeEach method alongside the other API key deletions, and apply the same fix to the other beforeEach hook mentioned at lines 46-54 to ensure consistent test isolation across all test setup methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/embedding-provider.test.ts`:
- Around line 13-21: The beforeEach hook is clearing several API key environment
variables but is missing OPENAI_EMBEDDING_API_KEY, which can cause
nondeterministic test failures if this variable exists in the runner
environment. Add a line to delete process.env["OPENAI_EMBEDDING_API_KEY"] in the
beforeEach method alongside the other API key deletions, and apply the same fix
to the other beforeEach hook mentioned at lines 46-54 to ensure consistent test
isolation across all test setup methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb6cd875-c1ee-436d-a264-0af025cf7272
📒 Files selected for processing (2)
src/providers/embedding/index.tstest/embedding-provider.test.ts
Summary
OPENAI_EMBEDDING_API_KEYwhen creating the OpenAI-compatible embedding providerOPENAI_API_KEYas the fallback for existing setupsWhy
When
agentmemoryis configured with different OpenAI-compatible providers for chat and embeddings,createEmbeddingProvider()was still passingOPENAI_API_KEYintoOpenAIEmbeddingProvider. In practice this breaks setups likeLLM=DeepSeekandembeddings=DashScope, because the DeepSeek key gets sent to the embedding endpoint.Verification
env -i HOME=/tmp/agentmemory-test-home PATH="$PATH" TERM="$TERM" npm test -- --run test/embedding-provider.test.tsenv -i HOME=/tmp/agentmemory-test-home PATH="$PATH" TERM="$TERM" npm run buildLLM=DeepSeek,embeddings=DashScope text-embedding-v4Summary by CodeRabbit
New Features
Tests