refactor(providers): extract OpenAI transport into shared module (#371)#462
Merged
Conversation
OpenAIProvider (LLM) and OpenAIEmbeddingProvider (embedding) both speak the OpenAI wire shape over POST /v1/chat/completions and POST /v1/embeddings — and both need to swap to the Azure shape when the base URL lands at *.openai.azure.com (api-key header instead of Authorization: Bearer, api-version query param, no /v1/ prefix because the deployment is already in the URL). Until now only the LLM provider knew about Azure. The embedding provider hardcoded /v1/embeddings + Bearer auth, so any user pointing OPENAI_BASE_URL at an Azure resource saw their LLM calls route correctly while embedding calls 404'd / 401'd. New src/providers/_openai-shared.ts owns: detectAzure(baseUrl) buildChatUrl(baseUrl, isAzure, apiVersion) buildEmbeddingUrl(baseUrl, isAzure, apiVersion) buildAuthHeaders(apiKey, isAzure) normalizeBaseUrl(raw) Both classes import these helpers. The embedding provider gains Azure auto-detection via the same OPENAI_BASE_URL hostname check the LLM provider already uses, plus the same OPENAI_API_VERSION knob (default 2024-08-01-preview). Closes the gap users hit when mirroring an Azure deployment on both surfaces. Net delta: ~40 LOC of duplicated transport collapsed into one shared file; both provider files are smaller and focused on their surface's unique concerns (LLM owns max_tokens / reasoning_effort / timeout precedence; embedding owns the MODEL_DIMENSIONS table). Approach diverges from the issue body (single class implementing both interfaces): the existing factory contracts at src/providers/index.ts dispatch LLM and embedding independently (detectProvider vs detectEmbeddingProvider), and merging into one class would force constructors to coordinate detection across two config surfaces. Shared transport keeps factories clean while delivering the DRY win and the Azure-on-embeddings parity gain. - src/providers/_openai-shared.ts: new shared transport module - src/providers/openai.ts: import shared helpers; drop local detectAzure + private buildUrl/buildHeaders - src/providers/embedding/openai.ts: import shared helpers; Azure auto-detection via OPENAI_BASE_URL host check, OPENAI_API_VERSION query param when Azure-shaped - test/openai-shared.test.ts: 17 cases covering all four helpers plus end-to-end embedding Azure shape (asserts URL path + api-key header substitution via mocked fetch) 1023/1023 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR extracts OpenAI and Azure transport logic (URL builders, Azure-style detection, auth header creation, base-url normalization) into ChangesOpenAI/Azure Transport Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 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 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: 2
🧹 Nitpick comments (1)
test/openai-shared.test.ts (1)
141-191: ⚡ Quick winNormalize headers using the Headers API for robust type handling.
RequestInit.headerscan beHeaders, header pairs array, or record. While current tests work (sincebuildAuthHeadersreturns plain objects), the cast toRecord<string, string>makes them brittle. Using theHeadersAPI constructor and.get()method normalizes all header input types and aligns with standard fetch semantics.Update both test cases (lines 147–191) to:
- Use
new Headers()instead ofRecord<string, string> = {}- Assign with
new Headers(init?.headers)instead of casting- Assert with
.get()method:.toBe("value")for present headers,.toBeNull()for absent (not.toBeUndefined())🤖 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/openai-shared.test.ts` around lines 141 - 191, The tests for OpenAIEmbeddingProvider.embedBatch should normalize RequestInit.headers using the Headers API: replace capturedHeaders: Record<string,string> with capturedHeaders = new Headers(), construct it via new Headers(init?.headers) inside the fetch mock, and change assertions to use capturedHeaders.get("api-key") / capturedHeaders.get("Authorization") returning expected strings or null (use .toBeNull() for absent) instead of checking undefined; this ensures robust handling of Headers, header pairs, and Records returned by buildAuthHeaders.
🤖 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/providers/_openai-shared.ts`:
- Around line 29-32: The azureUrl function currently appends path before
checking for an existing query string, causing the path to be treated as part of
query values; fix azureUrl by parsing baseUrl with the URL API (new
URL(baseUrl)), correctly joining the existing pathname and the provided path
(handle leading/trailing slashes) and then set/add the api-version via
url.searchParams.set('api-version', apiVersion) so query parameters are
preserved and encoded properly, finally return url.toString(); update references
inside azureUrl accordingly.
- Line 11: The constant DEFAULT_AZURE_API_VERSION currently set to
"2024-08-01-preview" must be replaced to use the v1 Azure OpenAI pattern: remove
or deprecate DEFAULT_AZURE_API_VERSION and update any code that appends
api-version to Azure OpenAI URLs to instead target the /openai/v1 endpoint
style; search for usages of DEFAULT_AZURE_API_VERSION and update functions that
build Azure URLs (e.g., any URL construction logic referencing
DEFAULT_AZURE_API_VERSION) so they no longer append an api-version query
parameter and instead use the /openai/v1 path for requests.
---
Nitpick comments:
In `@test/openai-shared.test.ts`:
- Around line 141-191: The tests for OpenAIEmbeddingProvider.embedBatch should
normalize RequestInit.headers using the Headers API: replace capturedHeaders:
Record<string,string> with capturedHeaders = new Headers(), construct it via new
Headers(init?.headers) inside the fetch mock, and change assertions to use
capturedHeaders.get("api-key") / capturedHeaders.get("Authorization") returning
expected strings or null (use .toBeNull() for absent) instead of checking
undefined; this ensures robust handling of Headers, header pairs, and Records
returned by buildAuthHeaders.
🪄 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: 45d1dc00-c5f1-4fc6-ad46-4ce2cc20de75
📒 Files selected for processing (4)
src/providers/_openai-shared.tssrc/providers/embedding/openai.tssrc/providers/openai.tstest/openai-shared.test.ts
| // across both surfaces and shaves ~40 LOC. | ||
|
|
||
| export const DEFAULT_OPENAI_BASE_URL = "https://api.openai.com"; | ||
| export const DEFAULT_AZURE_API_VERSION = "2024-08-01-preview"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest recommended Azure OpenAI API version as of May 2026?
💡 Result:
As of May 2026, the recommended approach for Azure OpenAI API integration is to use the v1 API [1][2]. The v1 API represents the next generation of Azure OpenAI APIs, which simplifies development by removing the requirement to specify dated api-version parameters [1][3]. By using the v1 API, developers gain ongoing access to the latest features without needing to update their code to accommodate new monthly API versions [1][3]. Key characteristics of the v1 API include: - No required api-version parameter [1][3]. - Support for the standard OpenAI client (using OpenAI instead of AzureOpenAI) [1][3]. - Integration by setting the base_url to the Azure OpenAI endpoint with /openai/v1 appended [1][3]. - Improved compatibility with OpenAI client libraries, allowing for easier switching between OpenAI and Azure OpenAI services [1][3]. For legacy or specific control plane operations that still require dated versions, the API continues to use the YYYY-MM-DD format [4][5]. However, for data plane inference, the v1 API is the current standard [4][6].
Citations:
- 1: https://learn.microsoft.com/en-us/azure/foundry/openai/api-version-lifecycle
- 2: https://learn.microsoft.com/en-us/azure/ai-services/openai/api-version-lifecycle?tabs=key
- 3: https://learn.microsoft.com/en-us/azure/ai-services/openai/api-version-deprecation
- 4: https://learn.microsoft.com/en-us/azure/foundry/openai/reference
- 5: https://learn.microsoft.com/en-us/azure/ai-foundry/openai/reference-preview
- 6: https://learn.microsoft.com/en-us/azure/foundry/openai/reference-preview-latest
Use the v1 API instead of the dated preview version.
The DEFAULT_AZURE_API_VERSION = "2024-08-01-preview" uses an outdated approach. As of May 2026, Microsoft recommends the v1 API for Azure OpenAI integrations, which removes the need for dated api-version parameters entirely and provides automatic access to the latest features without code updates. Migrate the integration to use the v1 API pattern with /openai/v1 endpoint configuration.
🤖 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/_openai-shared.ts` at line 11, The constant
DEFAULT_AZURE_API_VERSION currently set to "2024-08-01-preview" must be replaced
to use the v1 Azure OpenAI pattern: remove or deprecate
DEFAULT_AZURE_API_VERSION and update any code that appends api-version to Azure
OpenAI URLs to instead target the /openai/v1 endpoint style; search for usages
of DEFAULT_AZURE_API_VERSION and update functions that build Azure URLs (e.g.,
any URL construction logic referencing DEFAULT_AZURE_API_VERSION) so they no
longer append an api-version query parameter and instead use the /openai/v1 path
for requests.
4 tasks
Inline review on #462 surfaced an edge-case bug in the azureUrl helper: it tested baseUrl for an existing query string to pick the ?/& separator, then string-concatenated the route path BEFORE that separator. If a baseUrl arrived with pre-existing query params (corporate proxy carrying a tenant token, diagnostics endpoint, etc.), the route path got interpolated into the query string, producing a malformed URL like: baseUrl: https://proxy.example.com/openai/deployments/d?tenant=acme result: https://proxy.example.com/openai/deployments/d?tenant=acme/chat/completions&api-version=2024-08-01-preview ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ in the query, not the path Switch to the URL API: parse baseUrl, normalise the existing pathname (strip trailing slashes), join the route path with a single boundary slash, set api-version via url.searchParams.set (handles encoding without a manual encodeURIComponent), then toString. Existing query params are preserved. Test header normalisation pass: the OpenAIEmbeddingProvider Azure-detection tests captured RequestInit.headers as Record<string,string>. buildAuthHeaders happens to return a plain record today, but a future contributor migrating to HeadersInit/Headers would have these tests silently pass-but-stale because Record-style access would no longer match. Wrap the capture in new Headers(init?.headers) so the assertions go through .get() and tolerate any of the three valid HeadersInit shapes. Skipped — replacing DEFAULT_AZURE_API_VERSION with /openai/v1: the v1 Azure pattern is a recent addition and many deployments still require the api-version query param. Switching the default would break those deployments. Separate opt-in flag (e.g. OPENAI_AZURE_API_STYLE=v1) is the right shape but out of scope for this minimal-change review pass. - src/providers/_openai-shared.ts: azureUrl rewritten on URL API - test/openai-shared.test.ts: two new cases (query-param preservation, trailing-slash normalisation); embedding Azure tests switched to Headers API + .get() / .toBeNull() 1025/1025 tests pass.
Azure shipped a v1 GA endpoint pattern that mirrors the OpenAI wire shape one-for-one: POST https://<resource>.openai.azure.com/openai/v1/chat/completions No api-version query param, no /deployments/<dep> path segment — deployment name is passed in the request body as `model`. Existing OpenAI clients work as drop-in. The legacy URL pattern (/openai/deployments/<dep>/chat/completions?api-version=<date>) is still supported by Azure indefinitely for back-compat. Verification of the inline review's "remove DEFAULT_AZURE_API_VERSION and migrate all Azure calls to /openai/v1" instruction: removing it flat would break legacy users mid-flight (their existing OPENAI_BASE_URL=https://r.openai.azure.com/openai/deployments/d configs would still route through the legacy path and 400 without api-version). Hybrid auto-detect off the URL shape is the right shape: - baseUrl pathname contains `/openai/deployments/` -> legacy style, api-version still appended (DEFAULT_AZURE_API_VERSION fallback) - otherwise (bare resource host, /openai prefix, /openai/v1 prefix) -> v1 style, no api-version, route through /openai/v1/<path> Users migrate by stripping the /openai/deployments/<dep> suffix from their OPENAI_BASE_URL — no env-var changes required. azureStyleOf(baseUrl) is the new detection helper. v1AzureUrl preserves any pre-existing query params on the base URL (corporate proxy diagnostics tokens) but never appends api-version. The /openai/v1 prefix is normalised so configurations like OPENAI_BASE_URL=https://r.openai.azure.com/openai or .../openai/v1 don't double-prefix. - src/providers/_openai-shared.ts: azureStyleOf + v1AzureUrl; legacyAzureUrl (renamed from azureUrl) is unchanged; module header comment documents both styles and the migration path - test/openai-shared.test.ts: 3 new cases — v1 routing on bare host, /openai or /openai/v1 prefix normalisation, v1 on the embedding surface as well; legacy assertion updated to say "Azure legacy" so the contrast with v1 is explicit 1028/1028 tests pass.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/providers/_openai-shared.ts (1)
1-24: ⚡ Quick winRemove explanatory WHAT-comments to match repository style.
This file has multiple behavior-explaining comments; please rely on clear naming/tests instead and trim these comments per project convention.
As per coding guidelines, "Do not use code comments explaining WHAT — use clear naming instead".
Also applies to: 27-30, 34-34, 44-45, 60-63, 76-79, 82-85, 116-119
🤖 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/_openai-shared.ts` around lines 1 - 24, Remove the long explanatory "WHAT" comments in this shared OpenAI transport module and replace them with concise purpose-only comments; trim the multi-paragraph Azure/version discussion to a single-line summary and rely on clear names and tests instead (e.g., keep a short header describing that the module provides OpenAI-compatible transport helpers and that azureStyleOf() auto-detects legacy vs v1 URL shapes), delete the verbose block comments referenced in the review (the large Azure URL/style explanation and other WHAT-comments around azureStyleOf, the legacy/v1 description, and the examples), and if needed rename or clarify identifiers (like azureStyleOf, DEFAULT_AZURE_API_VERSION, enableLanceKnowledgeBase-style env knobs) so the code is self-descriptive and the tests cover behavior.
🤖 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/providers/_openai-shared.ts`:
- Around line 1-24: Remove the long explanatory "WHAT" comments in this shared
OpenAI transport module and replace them with concise purpose-only comments;
trim the multi-paragraph Azure/version discussion to a single-line summary and
rely on clear names and tests instead (e.g., keep a short header describing that
the module provides OpenAI-compatible transport helpers and that azureStyleOf()
auto-detects legacy vs v1 URL shapes), delete the verbose block comments
referenced in the review (the large Azure URL/style explanation and other
WHAT-comments around azureStyleOf, the legacy/v1 description, and the examples),
and if needed rename or clarify identifiers (like azureStyleOf,
DEFAULT_AZURE_API_VERSION, enableLanceKnowledgeBase-style env knobs) so the code
is self-descriptive and the tests cover behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2462cd8b-cc12-4b08-9283-f025ba9e795f
📒 Files selected for processing (2)
src/providers/_openai-shared.tstest/openai-shared.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/openai-shared.test.ts
rohitg00
added a commit
to cl0ckt0wer/agentmemory
that referenced
this pull request
May 18, 2026
Brings 10 days of main into the OpenCode plugin branch so the PR no longer conflicts on README + carries the new surfaces that shipped between v0.9.2 (when the branch opened) and v0.9.20: - v0.9.19 commit linking (rohitg00#498): KV.commits + Session.commitShas + memory_commit_lookup/memory_commits MCP tools (53 total now, plugin badge bumped from 51) - v0.9.19 Azure OpenAI v1 URL pattern (rohitg00#462) + Dijkstra graph retrieval (rohitg00#463) - v0.9.19 env passthrough on MCP server entries (rohitg00#460): ${VAR} expansion for AGENTMEMORY_URL / AGENTMEMORY_SECRET so one wired entry covers local + remote - v0.9.20 Codex Stop revert (rohitg00#501) README conflict resolution kept main's richer "Other agents" table shape (env-passthrough block + per-host config-file column + programmatic-access section) and re-added the OpenCode entry as two rows: "OpenCode (MCP only)" for the bare MCP wiring + "OpenCode (full plugin)" pointing at this plugin's 22-hook capture surface. src/triggers/api.ts auto-merged: PR's 3-line title->summary/firstPrompt addition (lines 535, 543, 544) survived alongside main's other api.ts churn since. plugin/opencode/plugin.json bumped 0.9.4 -> 0.9.20 to match the canonical version everything else ships on. plugin/opencode/README.md MCP-tool badge bumped 51 -> 53.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #371. Closes #199 (which #371 supersedes).
Problem
OpenAIProvider(LLM,src/providers/openai.ts) andOpenAIEmbeddingProvider(embedding,src/providers/embedding/openai.ts) both speak the OpenAI wire shape overPOST /v1/chat/completionsandPOST /v1/embeddings— and both need to swap to the Azure shape whenOPENAI_BASE_URLlands at a*.openai.azure.comresource (api-keyheader instead ofAuthorization: Bearer,api-versionquery param, no/v1/prefix because the deployment is already in the URL).Until now only the LLM provider knew about Azure. The embedding provider hardcoded
/v1/embeddings+ Bearer auth, so any user pointingOPENAI_BASE_URLat an Azure resource saw their LLM calls route correctly while embedding calls 404'd / 401'd. This was the latent bug behind the #371 ask.Approach (diverges from issue body)
The issue proposed merging both classes into one that implements
MemoryProvider+EmbeddingProvider. I picked a shared-transport module instead because:src/providers/index.tsdispatches LLM and embedding paths through independent detection functions (detectProvidervsdetectEmbeddingProvider). Merging the classes would force constructors to coordinate detection across two config surfaces — net code-complexity gain, not loss.New:
src/providers/_openai-shared.tsBoth provider classes import these. The LLM provider drops its local
detectAzure+buildUrl+buildHeaders. The embedding provider gains the same Azure auto-detection — same env var (OPENAI_BASE_URL), same api-version knob (OPENAI_API_VERSION, default2024-08-01-preview).Net delta
Functional gain: Azure embedding endpoint now works. Before this PR, only Azure LLM worked.
Tests
test/openai-shared.test.ts— 17 cases:detectAzure: positive (.openai.azure.com), negative (api.openai.com / DeepSeek / SiliconFlow / Ollama / vLLM), malformed URLsbuildChatUrl: standard/v1/chat/completions, Azure/chat/completions?api-version=..., URL-encoded api-versionbuildEmbeddingUrl: standard/v1/embeddings, Azure/embeddings?api-version=...(no/v1/prefix)buildAuthHeaders: Bearer for standard,api-keyfor AzurenormalizeBaseUrl: default fallback, trailing-slash strip, passthroughOpenAIEmbeddingProviderend-to-end: Azure shape (asserts captured URL +api-keyheader substitution via mocked fetch), standard OpenAI shape (Bearer +/v1/embeddings)1023/1023 tests pass locally (92 files, +15 new).
Verification
Out of scope
OPENAI_EMBEDDING_BASE_URLfor split LLM+embedding deployments) — separate enhancement, file if anyone needs it.Summary by CodeRabbit
Refactor
Tests