feat: support OPENAI_BASE_URL and OPENAI_EMBEDDING_MODEL for OpenAI-compatible endpoints#186
Conversation
Signed-off-by: Edison-A-N <zhangn661@gmail.com>
|
@Edison-A-N 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 OpenAI embedding provider was extended to support configurable OpenAI-compatible endpoints and embedding models. It now dynamically constructs the request URL using an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/embedding/openai.ts (1)
27-39:⚠️ Potential issue | 🟡 MinorNormalize trailing slash on
OPENAI_BASE_URL.If a user sets
OPENAI_BASE_URL=https://my-proxy.example.com/(a very common habit), the constructed URL becomeshttps://my-proxy.example.com//v1/embeddings, which some proxies reject or route differently.Proposed fix
- this.baseUrl = - getEnvVar("OPENAI_BASE_URL") || DEFAULT_BASE_URL; + this.baseUrl = ( + getEnvVar("OPENAI_BASE_URL") || DEFAULT_BASE_URL + ).replace(/\/+$/, "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/embedding/openai.ts` around lines 27 - 39, Normalize OPENAI_BASE_URL when initializing baseUrl: strip any trailing slash from the value returned by getEnvVar("OPENAI_BASE_URL") (falling back to DEFAULT_BASE_URL) so that baseUrl never ends with "/", preventing double slashes when building endpoints in methods like embedBatch which constructs `${this.baseUrl}/v1/embeddings`. Update the constructor logic that sets this.baseUrl to trim a trailing "/" (but preserve the host when value is empty or undefined by using DEFAULT_BASE_URL).
🧹 Nitpick comments (2)
test/embedding-provider.test.ts (2)
51-73: EnsureOPENAI_API_KEYis cleared in the new suite'sbeforeEach.The new
OpenAIEmbeddingProvidersuite only clearsOPENAI_BASE_URLandOPENAI_EMBEDDING_MODEL, but notOPENAI_API_KEY. The "throws when no API key is provided" test at Line 70-73 deletes it inline, but relies on the absence ofOPENAI_API_KEYin the ambient environment for correctness of ordering — if tests run in a shell whereOPENAI_API_KEYis exported, the other tests are fine (they pass"test-key"explicitly), but deleting it consistently inbeforeEachwould make the suite more self-contained and match the pattern of thecreateEmbeddingProvidersuite above.beforeEach(() => { process.env = { ...originalEnv }; + delete process.env["OPENAI_API_KEY"]; delete process.env["OPENAI_BASE_URL"]; delete process.env["OPENAI_EMBEDDING_MODEL"]; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/embedding-provider.test.ts` around lines 51 - 73, The suite's beforeEach currently deletes OPENAI_BASE_URL and OPENAI_EMBEDDING_MODEL but not OPENAI_API_KEY, making the "throws when no API key is provided" test brittle; update the beforeEach that runs for OpenAIEmbeddingProvider to also delete process.env["OPENAI_API_KEY"] so the OpenAIEmbeddingProvider constructor tests run in a clean environment (refer to the beforeEach block and the OpenAIEmbeddingProvider constructor and the test that deletes OPENAI_API_KEY).
101-101: Guard against missing fetch call arguments.
fetchSpy.mock.calls[0][1]will throw an unhelpfulTypeErroriffetchwas never called or called without a second argument. A small assertion improves failure diagnostics:- const body = JSON.parse((fetchSpy.mock.calls[0][1] as RequestInit).body as string); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const init = fetchSpy.mock.calls[0]?.[1] as RequestInit | undefined; + const body = JSON.parse(init?.body as string); expect(body.model).toBe("text-embedding-3-large");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/embedding-provider.test.ts` at line 101, Add a guard that verifies fetch was called with a second argument before accessing fetchSpy.mock.calls[0][1] to avoid a TypeError; e.g., assert fetchSpy.mock.calls.length > 0 and that fetchSpy.mock.calls[0].length > 1 (or use expect(fetchSpy).toHaveBeenCalled()) prior to the line that builds body (the const body = JSON.parse((fetchSpy.mock.calls[0][1] as RequestInit).body as string);) so failures show a clear assertion error instead of an unhelpful TypeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/embedding/openai.ts`:
- Around line 19-30: The class currently hardcodes readonly dimensions = 1536
while allowing model to be configured in the constructor (via model,
getEnvVar("OPENAI_EMBEDDING_MODEL") or DEFAULT_MODEL), causing a mismatch if a
different model (e.g. text-embedding-3-large) is used; change the implementation
so dimensions is computed from the configured model (use a small lookup map
keyed by model name to set the correct dimension) and allow an override via an
environment variable (OPENAI_EMBEDDING_DIMENSIONS) as a fallback; update the
constructor to read OPENAI_EMBEDDING_DIMENSIONS first, then map
model→dimensions, and finally default to 1536 if neither is present, keeping
references to the existing symbols dimensions, constructor, model, getEnvVar and
DEFAULT_MODEL for locating the change.
---
Outside diff comments:
In `@src/providers/embedding/openai.ts`:
- Around line 27-39: Normalize OPENAI_BASE_URL when initializing baseUrl: strip
any trailing slash from the value returned by getEnvVar("OPENAI_BASE_URL")
(falling back to DEFAULT_BASE_URL) so that baseUrl never ends with "/",
preventing double slashes when building endpoints in methods like embedBatch
which constructs `${this.baseUrl}/v1/embeddings`. Update the constructor logic
that sets this.baseUrl to trim a trailing "/" (but preserve the host when value
is empty or undefined by using DEFAULT_BASE_URL).
---
Nitpick comments:
In `@test/embedding-provider.test.ts`:
- Around line 51-73: The suite's beforeEach currently deletes OPENAI_BASE_URL
and OPENAI_EMBEDDING_MODEL but not OPENAI_API_KEY, making the "throws when no
API key is provided" test brittle; update the beforeEach that runs for
OpenAIEmbeddingProvider to also delete process.env["OPENAI_API_KEY"] so the
OpenAIEmbeddingProvider constructor tests run in a clean environment (refer to
the beforeEach block and the OpenAIEmbeddingProvider constructor and the test
that deletes OPENAI_API_KEY).
- Line 101: Add a guard that verifies fetch was called with a second argument
before accessing fetchSpy.mock.calls[0][1] to avoid a TypeError; e.g., assert
fetchSpy.mock.calls.length > 0 and that fetchSpy.mock.calls[0].length > 1 (or
use expect(fetchSpy).toHaveBeenCalled()) prior to the line that builds body (the
const body = JSON.parse((fetchSpy.mock.calls[0][1] as RequestInit).body as
string);) so failures show a clear assertion error instead of an unhelpful
TypeError.
🪄 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: 73adf18c-158e-4403-9fe5-fba69c843256
📒 Files selected for processing (2)
src/providers/embedding/openai.tstest/embedding-provider.test.ts
| readonly dimensions = 1536; | ||
| private apiKey: string; | ||
| private baseUrl: string; | ||
| private model: string; | ||
|
|
||
| constructor(apiKey?: string) { | ||
| this.apiKey = apiKey || getEnvVar("OPENAI_API_KEY") || ""; | ||
| if (!this.apiKey) throw new Error("OPENAI_API_KEY is required"); | ||
| this.baseUrl = | ||
| getEnvVar("OPENAI_BASE_URL") || DEFAULT_BASE_URL; | ||
| this.model = | ||
| getEnvVar("OPENAI_EMBEDDING_MODEL") || DEFAULT_MODEL; |
There was a problem hiding this comment.
dimensions is hardcoded to 1536 but model is now configurable.
If a user sets OPENAI_EMBEDDING_MODEL=text-embedding-3-large (3072 dims) or text-embedding-ada-002 (1536) or any other compatible model, provider.dimensions will report a value inconsistent with the vectors actually returned by embed(). Downstream consumers (e.g. vector store schemas) relying on dimensions will silently get wrong sizing.
Consider either:
- deriving
dimensionsfrom the configured model (lookup table with a sensible default), or - exposing
OPENAI_EMBEDDING_DIMENSIONSenv var, or - at minimum documenting the constraint that callers overriding the model are responsible for ensuring it returns 1536-dim vectors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/providers/embedding/openai.ts` around lines 19 - 30, The class currently
hardcodes readonly dimensions = 1536 while allowing model to be configured in
the constructor (via model, getEnvVar("OPENAI_EMBEDDING_MODEL") or
DEFAULT_MODEL), causing a mismatch if a different model (e.g.
text-embedding-3-large) is used; change the implementation so dimensions is
computed from the configured model (use a small lookup map keyed by model name
to set the correct dimension) and allow an override via an environment variable
(OPENAI_EMBEDDING_DIMENSIONS) as a fallback; update the constructor to read
OPENAI_EMBEDDING_DIMENSIONS first, then map model→dimensions, and finally
default to 1536 if neither is present, keeping references to the existing
symbols dimensions, constructor, model, getEnvVar and DEFAULT_MODEL for locating
the change.
|
Thanks @Edison-A-N for the contribution — merged! Mirrors the Opened a small follow-up at #189 to also derive |
Follow-up to #186. The PR made model configurable via OPENAI_EMBEDDING_MODEL but left dimensions hardcoded at 1536. A user pointing at text-embedding-3-large (3072 dims) would see provider.dimensions return 1536, which vector-store schemas and hybrid-search weights rely on for correct sizing. - Add MODEL_DIMENSIONS table: text-embedding-3-small=1536, text-embedding-3-large=3072, text-embedding-ada-002=1536. - Make dimensions a computed readonly field. - New OPENAI_EMBEDDING_DIMENSIONS env var for custom / self-hosted OpenAI-compatible endpoints not in the table. Positive integers only; reject non-numeric / non-positive values with a clear error. - Unknown model names fall back to 1536 with the explicit override available if the server returns a different size. - Tests cover known models, dimension override, unknown-model fallback, and validation of the override env var.
- README provider table: surface the no-op default and call out the opt-in Claude-subscription fallback with AGENTMEMORY_ALLOW_AGENT_SDK (from #187) instead of listing 'Claude subscription' as the default. - README env block: document OPENAI_BASE_URL / OPENAI_EMBEDDING_MODEL (#186) and OPENAI_EMBEDDING_DIMENSIONS (#189), plus MINIMAX_API_KEY. - Hero stat-tests SVG: 654 -> 827 (both dark and light variants) to match current suite size after recursion guard + idempotent lesson/crystal tests + openai dimension tests landed. - website/lib/generated-meta.json regenerated by website/scripts/gen-meta.mjs (v0.9.1, 51 tools, 12 hooks, 119 endpoints, 848 tests).
Rolls up #186 (OPENAI_BASE_URL / OPENAI_EMBEDDING_MODEL), #187 (Stop-hook recursion 5-layer defense + NoopProvider + AGENTMEMORY_ALLOW_AGENT_SDK opt-in), #188 (viewer empty-tabs + import-jsonl synthetic compression + auto-derived lessons/crystals + richer session detail + audit/replay/frontier shape fixes), #189 (OPENAI_EMBEDDING_DIMENSIONS + model-dimensions table), and #190 (README/website docs refresh). Bumps: package.json, plugin/.claude-plugin/plugin.json, src/version.ts, src/types.ts ExportData.version union, src/functions/export-import.ts supportedVersions, test/export-import.test.ts assertion, and packages/mcp/package.json shim (was stuck at 0.9.0).
2 participants
Summary
OPENAI_BASE_URLandOPENAI_EMBEDDING_MODELenv vars toOpenAIEmbeddingProvider, enabling use with OpenAI-compatible proxies (Azure OpenAI, vLLM, LM Studio, domestic relays, etc.)MinimaxProvider(MINIMAX_BASE_URL) — env var with sensible default, zero breaking changesDetails
Problem:
OpenAIEmbeddingProviderhardcodeshttps://api.openai.com/v1/embeddingsandtext-embedding-3-small. Users behind proxies or using compatible APIs cannot configure the endpoint.Fix: Two optional env vars with backward-compatible defaults:
OPENAI_BASE_URLhttps://api.openai.comOPENAI_EMBEDDING_MODELtext-embedding-3-smallFiles changed: 2 (provider + tests)
Tests added: 4 (default behavior, error case, base URL override, model override)
Related
See also #185 which takes a broader approach (full OpenAI-compatible provider + local runtime detection). This PR is intentionally minimal and focused on the embedding provider only.
How to verify
OPENAI_API_KEY=sk-test OPENAI_BASE_URL=https://your-proxy.com npm test -- test/embedding-provider.test.tsSummary by CodeRabbit
Release Notes