fix: support AGENTMEMORY_EMBEDDING_PROVIDER and xenova/transformers aliases#412
fix: support AGENTMEMORY_EMBEDDING_PROVIDER and xenova/transformers aliases#412AyushKar2005 wants to merge 2 commits 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. |
📝 WalkthroughWalkthroughAdds AGENTMEMORY_EMBEDDING_PROVIDER override (normalizing xenova/transformers → "local"); adds MCP server env defaults; and makes standalone MCP tools/list return the full toolset when AGENTMEMORY_TOOLS=all in proxy or local-fallback code paths. ChangesAgentMemory runtime configuration and MCP tools
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
- standalone.ts: when AGENTMEMORY_TOOLS=all, override remote core list with getAllTools() so shim returns full tool surface - standalone.ts: fix local fallback to return getAllTools() when AGENTMEMORY_TOOLS=all instead of filtering to 7 IMPLEMENTED_TOOLS - plugin/.mcp.json: add AGENTMEMORY_URL, AGENTMEMORY_TOOLS=all, AGENTMEMORY_FORCE_PROXY=1 env vars
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/mcp/standalone.ts`:
- Around line 389-391: The current condition uses
process.env["AGENTMEMORY_TOOLS"] === "all" && remote.tools.length <
allTools.length which can miss cases where remote.tools has the same length but
wrong or duplicate names; change the check to validate tool names instead of
length: build a set of remote.tools names and verify that every name in allTools
is present (or that the set of names equals the set of allTools), and trigger
the override when any expected tool name is missing or mismatched; reference the
variables remote.tools, allTools and the AGENTMEMORY_TOOLS env check when
applying this replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c269539-39ce-4b1d-846e-678a7b5bc770
📒 Files selected for processing (2)
plugin/.mcp.jsonsrc/mcp/standalone.ts
| process.env["AGENTMEMORY_TOOLS"] === "all" && | ||
| remote.tools.length < allTools.length | ||
| ) { |
There was a problem hiding this comment.
Use name-based completeness validation instead of length-only check.
With AGENTMEMORY_TOOLS=all, comparing only remote.tools.length < allTools.length can miss incomplete remote lists that happen to have equal length (e.g., duplicates or wrong tool names), so the override may not trigger.
Proposed fix
- if (
- process.env["AGENTMEMORY_TOOLS"] === "all" &&
- remote.tools.length < allTools.length
- ) {
+ if (process.env["AGENTMEMORY_TOOLS"] === "all") {
+ const allNames = new Set(allTools.map((t) => t.name));
+ const remoteNames = new Set(
+ remote.tools
+ .map((t) =>
+ typeof t === "object" && t !== null && "name" in t
+ ? (t as { name?: unknown }).name
+ : undefined,
+ )
+ .filter((name): name is string => typeof name === "string"),
+ );
+ const isComplete =
+ remoteNames.size === allNames.size &&
+ [...allNames].every((name) => remoteNames.has(name));
+ if (!isComplete) {
if (debug) {
process.stderr.write(
`[`@agentmemory/mcp`] tools/list: AGENTMEMORY_TOOLS=all override — returning ${allTools.length} tools instead of ${remote.tools.length} from server\n`,
);
}
return { tools: allTools };
+ }
}🤖 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/mcp/standalone.ts` around lines 389 - 391, The current condition uses
process.env["AGENTMEMORY_TOOLS"] === "all" && remote.tools.length <
allTools.length which can miss cases where remote.tools has the same length but
wrong or duplicate names; change the check to validate tool names instead of
length: build a set of remote.tools names and verify that every name in allTools
is present (or that the set of names equals the set of allTools), and trigger
the override when any expected tool name is missing or mismatched; reference the
variables remote.tools, allTools and the AGENTMEMORY_TOOLS env check when
applying this replacement.
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