fix: support AGENTMEMORY_EMBEDDING_PROVIDER and xenova/transformers aliases#412
fix: support AGENTMEMORY_EMBEDDING_PROVIDER and xenova/transformers aliases#412AyushKar2005 wants to merge 1 commit into
Conversation
|
@Adolfler 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 environment variable override support to embedding provider detection. ChangesEmbedding Provider Configuration
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (1)
src/config.ts (1)
185-191: ⚡ Quick winNormalize and trim forced provider values before alias mapping.
Current matching only handles exact lowercase strings, so
"Xenova"/" transformers "won’t resolve to"local".Proposed fix
-const forced = source["AGENTMEMORY_EMBEDDING_PROVIDER"] || source["EMBEDDING_PROVIDER"]; +const forcedRaw = + source["AGENTMEMORY_EMBEDDING_PROVIDER"] ?? source["EMBEDDING_PROVIDER"]; +const forced = forcedRaw?.trim().toLowerCase(); if (forced) { - if (forced === "xenova" || forced === "transformers") return "local"; - + return forced; }🤖 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/config.ts` around lines 185 - 191, The code reads the environment override into the variable forced but compares it directly, so values like "Xenova" or " transformers " don't match; normalize by trimming whitespace and converting to lowercase (e.g., let normalized = String(forced).trim().toLowerCase()) and use normalized for the alias checks in the existing block that maps "xenova" or "transformers" to "local" and otherwise returns the forced provider; ensure you still return the original normalized/mapped value rather than the unnormalized input.
🤖 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.
Nitpick comments:
In `@src/config.ts`:
- Around line 185-191: The code reads the environment override into the variable
forced but compares it directly, so values like "Xenova" or " transformers "
don't match; normalize by trimming whitespace and converting to lowercase (e.g.,
let normalized = String(forced).trim().toLowerCase()) and use normalized for the
alias checks in the existing block that maps "xenova" or "transformers" to
"local" and otherwise returns the forced provider; ensure you still return the
original normalized/mapped value rather than the unnormalized input.
2 participants
so i was looking at issue #395 and traced it down to
detectEmbeddingProviderin config.ts. it was readingEMBEDDING_PROVIDERbut the docs and startup logs all say to setAGENTMEMORY_EMBEDDING_PROVIDER— so the env var was just never being read.also noticed that even if someone set it correctly to
xenovaortransformers, it would still fall through to null because the switch in index.ts only handleslocal. added an alias that maps both tolocal.tested locally, embedding provider now correctly resolves instead of falling back to BM25-only mode.
Closes #395
Summary by CodeRabbit