fix: prefer embedding API key for OpenAI embeddings#802
Conversation
|
@FrankZ20 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 PR adds support for a dedicated OpenAI embedding API key environment variable. The embedding provider now checks ChangesEmbedding API key selection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (2)
test/embedding-provider.test.ts (1)
13-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
beforeEachdoesn't clearOPENAI_EMBEDDING_API_KEY.This block resets several provider keys but not
OPENAI_EMBEDDING_API_KEY. If the test environment has it set, the new test as well asreturns null when no API keys are setcould behave non-hermetically depending ondetectEmbeddingProvider(). Add the delete for consistency.💚 Suggested isolation fix
delete process.env["OPENROUTER_API_KEY"]; delete process.env["EMBEDDING_PROVIDER"]; + delete process.env["OPENAI_EMBEDDING_API_KEY"]; });🤖 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 setup in embedding-provider.test.ts fails to clear OPENAI_EMBEDDING_API_KEY, which can make detectEmbeddingProvider() and the "returns null when no API keys are set" test non-hermetic; update the beforeEach block (the one in test/embedding-provider.test.ts) to also delete process.env["OPENAI_EMBEDDING_API_KEY"] alongside the other deletes so the environment is fully reset before each test.src/providers/embedding/index.ts (1)
30-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate
detectEmbeddingProvider()to support embedding-only OpenAI configs (OPENAI_EMBEDDING_API_KEY)
detectEmbeddingProvider()insrc/config.tsreturns"openai"only whenOPENAI_API_KEYis set, ignoringOPENAI_EMBEDDING_API_KEY; this makescreateEmbeddingProvider()returnnullfor deployments that provide embeddings viaOPENAI_EMBEDDING_API_KEYbut notOPENAI_API_KEY.🤖 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 `@src/providers/embedding/index.ts` around lines 30 - 42, detectEmbeddingProvider currently only returns "openai" when OPENAI_API_KEY is present, causing createEmbeddingProvider (which expects "openai") to miss setups that only provide OPENAI_EMBEDDING_API_KEY; update detectEmbeddingProvider in src/config.ts to check for either OPENAI_EMBEDDING_API_KEY or OPENAI_API_KEY and return "openai" if either env var is set (use getEnvVar or equivalent), ensuring createEmbeddingProvider's case "openai" is reachable when only the embedding-specific key is configured.
🧹 Nitpick comments (1)
src/providers/embedding/index.ts (1)
38-42: 💤 Low valueSimplify OpenAIEmbeddingProvider construction to avoid duplicated key precedence
OpenAIEmbeddingProvider’s constructor already applies
apiKey || OPENAI_EMBEDDING_API_KEY || OPENAI_API_KEYand throws if the resulting key is empty, so passinggetEnvVar("OPENAI_EMBEDDING_API_KEY") || getEnvVar("OPENAI_API_KEY")!duplicates that logic.♻️ Proposed simplification
case "openai": return withDimensionGuard( - new OpenAIEmbeddingProvider( - getEnvVar("OPENAI_EMBEDDING_API_KEY") || getEnvVar("OPENAI_API_KEY")!, - ), + new OpenAIEmbeddingProvider(), );🤖 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 `@src/providers/embedding/index.ts` around lines 38 - 42, The code duplicates API-key precedence logic by passing getEnvVar("OPENAI_EMBEDDING_API_KEY") || getEnvVar("OPENAI_API_KEY") into the OpenAIEmbeddingProvider constructor even though OpenAIEmbeddingProvider already implements apiKey || OPENAI_EMBEDDING_API_KEY || OPENAI_API_KEY and throws on missing keys; fix this by constructing OpenAIEmbeddingProvider without providing the env key (i.e., new OpenAIEmbeddingProvider()) and keep it wrapped in withDimensionGuard so the provider's internal key resolution and validation are used.
🤖 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 `@src/providers/embedding/index.ts`:
- Around line 30-42: detectEmbeddingProvider currently only returns "openai"
when OPENAI_API_KEY is present, causing createEmbeddingProvider (which expects
"openai") to miss setups that only provide OPENAI_EMBEDDING_API_KEY; update
detectEmbeddingProvider in src/config.ts to check for either
OPENAI_EMBEDDING_API_KEY or OPENAI_API_KEY and return "openai" if either env var
is set (use getEnvVar or equivalent), ensuring createEmbeddingProvider's case
"openai" is reachable when only the embedding-specific key is configured.
In `@test/embedding-provider.test.ts`:
- Around line 13-21: The beforeEach setup in embedding-provider.test.ts fails to
clear OPENAI_EMBEDDING_API_KEY, which can make detectEmbeddingProvider() and the
"returns null when no API keys are set" test non-hermetic; update the beforeEach
block (the one in test/embedding-provider.test.ts) to also delete
process.env["OPENAI_EMBEDDING_API_KEY"] alongside the other deletes so the
environment is fully reset before each test.
---
Nitpick comments:
In `@src/providers/embedding/index.ts`:
- Around line 38-42: The code duplicates API-key precedence logic by passing
getEnvVar("OPENAI_EMBEDDING_API_KEY") || getEnvVar("OPENAI_API_KEY") into the
OpenAIEmbeddingProvider constructor even though OpenAIEmbeddingProvider already
implements apiKey || OPENAI_EMBEDDING_API_KEY || OPENAI_API_KEY and throws on
missing keys; fix this by constructing OpenAIEmbeddingProvider without providing
the env key (i.e., new OpenAIEmbeddingProvider()) and keep it wrapped in
withDimensionGuard so the provider's internal key resolution and validation are
used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1dd046f6-04aa-4d3c-8397-c49ca5f7ed39
📒 Files selected for processing (2)
src/providers/embedding/index.tstest/embedding-provider.test.ts
1 participant
Summary
Fix
createEmbeddingProvider()so OpenAI-compatible embeddings preferOPENAI_EMBEDDING_API_KEYwhen it is set.OpenAIEmbeddingProvideralready supports a dedicated embedding key internally, but the factory currently passesOPENAI_API_KEYdirectly into the constructor. That caller-provided text LLM key wins over the provider's internal fallback chain, so deployments with separate chat and embedding providers can accidentally send embedding requests with the chat key.Changes
OPENAI_EMBEDDING_API_KEY || OPENAI_API_KEYfrom the OpenAI embedding factory branch.Why
This matters for OpenAI-compatible deployments where chat and embeddings live on different providers/endpoints. For example, a chat proxy key in
OPENAI_API_KEYplus DashScope/Bailian inOPENAI_EMBEDDING_API_KEYcurrently causes vector indexing to call the embedding endpoint with the chat key and fail with401 invalid_api_key.Tests
embedding-provider.test.tsregression test.Summary by CodeRabbit
New Features
Tests