Skip to content

feat(providers): add Azure OpenAI / AI Foundry provider with HTTP CONNECT proxy support#594

Open
nagarjunr wants to merge 2 commits into
rohitg00:mainfrom
nagarjunr:feat/azure-openai-foundry-proxy
Open

feat(providers): add Azure OpenAI / AI Foundry provider with HTTP CONNECT proxy support#594
nagarjunr wants to merge 2 commits into
rohitg00:mainfrom
nagarjunr:feat/azure-openai-foundry-proxy

Conversation

@nagarjunr
Copy link
Copy Markdown

@nagarjunr nagarjunr commented May 21, 2026

Summary

Adds first-class support for Azure-hosted LLMs as a memory provider, covering three deployment types and corporate proxy environments.

Motivation

Enterprise/corporate users running Azure OpenAI or Azure AI Foundry cannot currently use agentmemory as a memory provider because:

  1. No azure-openai provider type exists
  2. Node.js 18+ built-in fetch ignores HTTP_PROXY/HTTPS_PROXY, causing DNS failures on enterprise networks where all traffic must route through a corporate proxy

Changes (1 squash commit, 8 files)

New: azure-openai provider type

AZURE_OPENAI_API_KEY=...
AZURE_OPENAI_ENDPOINT=https://<resource>.openai.azure.com
AZURE_OPENAI_DEPLOYMENT=<deployment-name>
AZURE_OPENAI_API_VERSION=2024-08-01-preview   # optional

Auto-detected at startup — same pattern as OPENAI_API_KEY / ANTHROPIC_API_KEY. Routes through the existing OpenAIProvider; detectAzure() and detectFoundry() handle URL-shape routing internally. No separate class needed.

Azure AI Foundry (Anthropic) endpoint

AZURE_OPENAI_ENDPOINT=https://<resource>.services.ai.azure.com/anthropic

When the endpoint path contains /anthropic, the provider switches to the Anthropic Messages API wire format (x-api-key + anthropic-version headers, top-level system field). No extra config — path shape is the detection signal.

Corporate HTTP CONNECT proxy (src/providers/_proxy.ts — new file)

HTTP_PROXY/HTTPS_PROXY env vars are now respected:

  • Custom https.Agent tunnels via HTTP CONNECT using only Node.js built-ins (no tunnel-agent dependency)
  • node-fetch (optional dep) used as the fetch vehicle since Node.js 18+ built-in fetch ignores the agent option entirely
  • Handles Node.js v26 strict SNI validation (RFC 6066 §3: TLS SNI must not be an IP literal)
  • Falls back to global fetch with a stderr warning if node-fetch is not installed

fetchWithTimeout in _fetch.ts gains an optional fetchFn parameter and auto-detects proxy env vars with per-URL caching for TCP connection reuse.

Extended Azure hostname detection

detectAzure() in _openai-shared.ts now covers three Azure hostname patterns:

Hostname Service
*.openai.azure.com Classic Azure OpenAI
*.services.ai.azure.com Azure AI Services / AI Foundry OpenAI
*.cognitiveservices.azure.com Cognitive Services unified endpoint

No breaking changes

All existing provider paths (openai, anthropic, gemini, openrouter, minimax, agent-sdk) are unchanged. azure-openai activates only when AZURE_OPENAI_API_KEY is set and OPENAI_API_KEY is not.

Test plan

  • npm run build passes on upstream tip
  • Test with standard Azure OpenAI deployment (*.openai.azure.com)
  • Test with Azure AI Foundry Anthropic endpoint (*.services.ai.azure.com/anthropic)
  • Test with HTTPS_PROXY set (corporate proxy path)
  • Test without node-fetch installed — confirm fallback warning + graceful degradation
  • npm test — no regressions

Files changed

File Change
src/providers/_proxy.ts New — HTTP CONNECT proxy tunnel
src/providers/_fetch.ts Proxy-aware fetchWithTimeout
src/providers/_openai-shared.ts Extended Azure detection + Foundry helpers
src/providers/openai.ts Foundry support + proxy injection
src/providers/index.ts azure-openai case → OpenAIProvider
src/config.ts Azure env var auto-detection + VALID_PROVIDERS
src/types.ts "azure-openai" added to ProviderType
package.json node-fetch added to optionalDependencies

Summary by CodeRabbit

  • New Features

    • Added Azure OpenAI support with automatic environment credential detection
    • Added Anthropic Messages API (Azure AI Foundry) support
    • Improved HTTP/HTTPS proxy handling with optional runtime proxy loader
  • Bug Fixes

    • Harder validation for timeout values and clearer timeout/error messages
  • Chores

    • Normalized module footer formatting and added optional fetch dependency metadata

Review Change Stack

…pport

Adds first-class support for Azure-hosted LLMs as a provider for
compress/summarize, including Azure AI Foundry Anthropic endpoints and
corporate HTTP CONNECT proxy tunneling.

## What's added

### New `azure-openai` provider type

Set these env vars to activate:

    AZURE_OPENAI_API_KEY=...
    AZURE_OPENAI_ENDPOINT=https://<resource>.openai.azure.com
    AZURE_OPENAI_DEPLOYMENT=<deployment-name>
    AZURE_OPENAI_API_VERSION=2024-08-01-preview   # optional

Auto-detected at startup (same pattern as OPENAI_API_KEY / ANTHROPIC_API_KEY).

### Azure AI Foundry (Anthropic) endpoint

When AZURE_OPENAI_ENDPOINT contains `/anthropic` in the path, the
provider switches to the Anthropic Messages API wire format:

    AZURE_OPENAI_ENDPOINT=https://<resource>.services.ai.azure.com/anthropic

Foundry uses x-api-key + anthropic-version headers and the top-level
system field. No extra config — endpoint path is the detection signal.

### Corporate HTTP CONNECT proxy support

Node.js 18+ built-in fetch ignores HTTP_PROXY / HTTPS_PROXY, causing
DNS failures on enterprise networks.

- src/providers/_proxy.ts (new) — builds a proxy-aware fetch function via
  node-fetch (optional dep) + custom https.Agent tunneling via HTTP CONNECT.
  Uses only Node.js built-in modules for the tunnel (no tunnel-agent).
  Handles Node.js v26 strict SNI (RFC 6066 §3: SNI must not be an IP).
- src/providers/_fetch.ts — fetchWithTimeout gains an optional fetchFn
  parameter and auto-detects HTTPS_PROXY / HTTP_PROXY with per-URL caching.

### Extended Azure hostname detection (_openai-shared.ts)

detectAzure() now covers three hostnames (previously only .openai.azure.com):
- .openai.azure.com
- .services.ai.azure.com   (AI Foundry OpenAI deployments)
- .cognitiveservices.azure.com

### Routing (providers/index.ts)

New `azure-openai` case routes through OpenAIProvider — detectAzure()
and detectFoundry() handle URL-shape routing internally. The `openai`
case is unchanged.

### Types + VALID_PROVIDERS

ProviderType gains `"azure-openai"`. VALID_PROVIDERS in config.ts
includes `"azure-openai"`.

## Optional dependency

node-fetch added to optionalDependencies. Loaded only when proxy env vars
are present; falls back to global fetch with a stderr warning if absent.

## No breaking changes

All existing provider paths are unchanged. azure-openai activates only
when AZURE_OPENAI_API_KEY is set and OPENAI_API_KEY is not.
@vercel
Copy link
Copy Markdown

vercel Bot commented May 21, 2026

Someone is attempting to deploy a commit to the rohitg00's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

📝 Walkthrough

Walkthrough

This PR adds support for Azure OpenAI and Azure AI Foundry (Anthropic Messages API) providers with HTTP/HTTPS proxy infrastructure. It extends provider detection, updates the fetch layer to support custom fetch functions and proxy caching, adds Foundry-specific helpers, and routes requests appropriately based on endpoint type.

Changes

Azure OpenAI and Foundry provider support

Layer / File(s) Summary
Type definition and provider detection
src/types.ts, src/config.ts
ProviderType adds "azure-openai" literal. detectProvider recognizes AZURE_OPENAI_API_KEY, AZURE_OPENAI_ENDPOINT, and AZURE_OPENAI_DEPLOYMENT env vars; detectLlmProviderKind treats Azure credentials as "llm"; FALLBACK_PROVIDERS validation includes "azure-openai".
Fetch timeout and proxy caching infrastructure
package.json, src/providers/_fetch.ts
Optional node-fetch dependency added. fetchWithTimeout accepts optional fetchFn parameter and maintains module-level proxy fetch cache that rebuilds when HTTP_PROXY/HTTPS_PROXY env vars change.
HTTP/HTTPS proxy tunneling
src/providers/_proxy.ts
New buildProxyFetch returns a proxy-aware fetch wrapper that uses HTTP CONNECT tunneling with custom HTTPS agent; detects IP literal hosts for Node.js v26+ TLS SNI compliance; dynamically loads optional node-fetch with graceful fallback.
OpenAI-compatible provider helpers for Azure and Foundry
src/providers/_openai-shared.ts
detectAzure expanded to recognize multiple Azure host suffixes. New detectFoundry, buildFoundryUrl, and buildFoundryHeaders helpers detect Foundry endpoints, normalize URLs to /v1/messages, and construct Anthropic Messages API headers.
OpenAI provider configuration and Foundry routing
src/providers/index.ts, src/providers/openai.ts
createBaseProvider adds "azure-openai" switch branch to instantiate OpenAI provider with Azure env vars. OpenAI provider detects Foundry at construction, routes requests to new callFoundry method for Foundry endpoints, integrates proxy fetch into both Foundry and OpenAI-compatible paths, and hardens parsePositiveInt timeout validation.
Script footer normalization
plugin/scripts/*.mjs
Normalized trailing export { };export {}; and adjusted region/footer formatting across multiple plugin scripts; no runtime behavior changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Azure breezes, Foundry lights collide,
I tunnel the packets, swift as a hare,
New providers dancing on the developer's side,
Proxy-lined pathways hum through the air,
Hop—LLMs connect with care.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main changes: adding Azure OpenAI / AI Foundry provider support with HTTP CONNECT proxy functionality, which aligns with the core objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/providers/_proxy.ts (2)

34-34: 💤 Low value

keepAlive: false contradicts the cache rationale in _fetch.ts.

The cache comment at _fetch.ts:6 states "Caching preserves the same https.Agent across requests → TCP connection reuse." With keepAlive: false, every request still opens a fresh TCP/TLS handshake plus a new CONNECT round-trip to the proxy — so caching only saves agent allocation, not connection setup. Consider enabling keep-alive (and setting a reasonable maxSockets) so the cache actually delivers the advertised reuse, or update the comment to match reality.

🤖 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/_proxy.ts` at line 34, The https.Agent is created with
keepAlive: false which prevents TCP/TLS connection reuse despite the caching
rationale in _fetch.ts; update the agent creation (the const agent in _proxy.ts)
to enable keep-alive and limit concurrency (e.g., new https.Agent({ keepAlive:
true, maxSockets: <reasonable-number> })) so cached agents actually reuse
connections, and optionally set keepAliveMsecs if needed; if you choose not to
enable keep-alive instead, update the comment in _fetch.ts to remove or correct
the claim about TCP connection reuse.

28-28: 💤 Low value

Missing radix on parseInt.

Pass radix 10 explicitly to stay consistent with the rest of the codebase (e.g., safeParseInt in src/config.ts) and avoid lints flagging implicit-radix.

-  const proxyPort = parseInt(proxy.port || "3128");
+  const proxyPort = parseInt(proxy.port || "3128", 10);
🤖 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/_proxy.ts` at line 28, The parseInt call that sets proxyPort
uses an implicit radix; update the assignment in _proxy.ts (the proxyPort
initialization) to pass radix 10 explicitly or use the project's safeParseInt
helper from src/config.ts so it reads the port with a defined base (e.g.,
parseInt(proxy.port || "3128", 10) or safeParseInt(proxy.port, 3128)).
🤖 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/_proxy.ts`:
- Around line 59-74: The connect handler currently calls tls.connect
unconditionally when connectReq emits "connect", which starts TLS even for
non-2xx proxy responses; update the connectReq.once("connect", (_res, socket) =>
{ ... }) callback to first inspect the response statusCode (and optionally
statusMessage) on _res and only proceed to build tlsOptions and call tls.connect
when statusCode is in the 200–299 range; if the status is not 2xx, destroy the
socket and invoke cb with a clear Error containing the proxy status code/message
(and any response body if available) so callers get a proxy/auth error instead
of opaque TLS failures — keep existing use of isIPAddress(targetHost) for SNI
and still attach tlsSocket.once("error", cb) when TLS is started.

---

Nitpick comments:
In `@src/providers/_proxy.ts`:
- Line 34: The https.Agent is created with keepAlive: false which prevents
TCP/TLS connection reuse despite the caching rationale in _fetch.ts; update the
agent creation (the const agent in _proxy.ts) to enable keep-alive and limit
concurrency (e.g., new https.Agent({ keepAlive: true, maxSockets:
<reasonable-number> })) so cached agents actually reuse connections, and
optionally set keepAliveMsecs if needed; if you choose not to enable keep-alive
instead, update the comment in _fetch.ts to remove or correct the claim about
TCP connection reuse.
- Line 28: The parseInt call that sets proxyPort uses an implicit radix; update
the assignment in _proxy.ts (the proxyPort initialization) to pass radix 10
explicitly or use the project's safeParseInt helper from src/config.ts so it
reads the port with a defined base (e.g., parseInt(proxy.port || "3128", 10) or
safeParseInt(proxy.port, 3128)).
🪄 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: 90cc848b-e9ab-41b7-a35d-b1ddfd1c6ce6

📥 Commits

Reviewing files that changed from the base of the PR and between bc64107 and fa01cd0.

📒 Files selected for processing (8)
  • package.json
  • src/config.ts
  • src/providers/_fetch.ts
  • src/providers/_openai-shared.ts
  • src/providers/_proxy.ts
  • src/providers/index.ts
  • src/providers/openai.ts
  • src/types.ts

Comment thread src/providers/_proxy.ts
Comment on lines +59 to +74
connectReq.once("connect", (_res, socket) => {
const tlsOptions: tls.ConnectionOptions = {
socket,
rejectUnauthorized: options.rejectUnauthorized !== false,
};
// Skip SNI for IP addresses — Node.js v26 rejects IP SNI per RFC 6066 §3
if (!isIPAddress(targetHost)) {
tlsOptions.servername = targetHost;
}
const tlsSocket = tls.connect(tlsOptions, () => cb(null, tlsSocket));
tlsSocket.once("error", cb);
});

connectReq.once("error", cb);
connectReq.end();
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

CONNECT response status is not validated before starting TLS.

When the proxy responds to CONNECT with a non-2xx status (e.g., 407 Proxy Authentication Required, 403 Forbidden, 502 Bad Gateway), the "connect" event still fires but the tunnel is not established. The current code unconditionally calls tls.connect() on that socket, so users see opaque TLS protocol errors instead of a clear proxy/auth diagnostic — defeating much of the value of corporate-proxy support.

🛡️ Proposed fix
-    connectReq.once("connect", (_res, socket) => {
+    connectReq.once("connect", (res, socket) => {
+      if (!res.statusCode || res.statusCode < 200 || res.statusCode >= 300) {
+        socket.destroy();
+        cb(
+          new Error(
+            `HTTP CONNECT proxy returned ${res.statusCode} ${res.statusMessage ?? ""} for ${targetHost}:${targetPort}`,
+          ),
+        );
+        return;
+      }
       const tlsOptions: tls.ConnectionOptions = {
         socket,
         rejectUnauthorized: options.rejectUnauthorized !== false,
       };
       // Skip SNI for IP addresses — Node.js v26 rejects IP SNI per RFC 6066 §3
       if (!isIPAddress(targetHost)) {
         tlsOptions.servername = targetHost;
       }
       const tlsSocket = tls.connect(tlsOptions, () => cb(null, tlsSocket));
       tlsSocket.once("error", cb);
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
connectReq.once("connect", (_res, socket) => {
const tlsOptions: tls.ConnectionOptions = {
socket,
rejectUnauthorized: options.rejectUnauthorized !== false,
};
// Skip SNI for IP addresses — Node.js v26 rejects IP SNI per RFC 6066 §3
if (!isIPAddress(targetHost)) {
tlsOptions.servername = targetHost;
}
const tlsSocket = tls.connect(tlsOptions, () => cb(null, tlsSocket));
tlsSocket.once("error", cb);
});
connectReq.once("error", cb);
connectReq.end();
};
connectReq.once("connect", (res, socket) => {
if (!res.statusCode || res.statusCode < 200 || res.statusCode >= 300) {
socket.destroy();
cb(
new Error(
`HTTP CONNECT proxy returned ${res.statusCode} ${res.statusMessage ?? ""} for ${targetHost}:${targetPort}`,
),
);
return;
}
const tlsOptions: tls.ConnectionOptions = {
socket,
rejectUnauthorized: options.rejectUnauthorized !== false,
};
// Skip SNI for IP addresses — Node.js v26 rejects IP SNI per RFC 6066 §3
if (!isIPAddress(targetHost)) {
tlsOptions.servername = targetHost;
}
const tlsSocket = tls.connect(tlsOptions, () => cb(null, tlsSocket));
tlsSocket.once("error", cb);
});
connectReq.once("error", cb);
connectReq.end();
};
🤖 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/_proxy.ts` around lines 59 - 74, The connect handler currently
calls tls.connect unconditionally when connectReq emits "connect", which starts
TLS even for non-2xx proxy responses; update the connectReq.once("connect",
(_res, socket) => { ... }) callback to first inspect the response statusCode
(and optionally statusMessage) on _res and only proceed to build tlsOptions and
call tls.connect when statusCode is in the 200–299 range; if the status is not
2xx, destroy the socket and invoke cb with a clear Error containing the proxy
status code/message (and any response body if available) so callers get a
proxy/auth error instead of opaque TLS failures — keep existing use of
isIPAddress(targetHost) for SNI and still attach tlsSocket.once("error", cb)
when TLS is started.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
src/providers/_fetch.ts (2)

33-33: ⚡ Quick win

Improve type safety of the fetchFn parameter.

The init parameter is typed as any, reducing type safety. Consider using RequestInit to match the standard fetch signature.

♻️ Proposed fix
-  fetchFn?: (url: string, init: any) => Promise<Response>,
+  fetchFn?: (url: string, init: RequestInit) => Promise<Response>,
🤖 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/_fetch.ts` at line 33, The fetchFn parameter currently types
its second argument as any, reducing type safety; update the signature to use
the standard RequestInit type (e.g., change fetchFn?: (url: string, init: any)
=> Promise<Response> to use RequestInit) and ensure any necessary imports/types
are available in the file so callers get proper typing for headers, method,
body, etc.; reference the fetchFn parameter in src/providers/_fetch.ts and
adjust callers if their argument types need to satisfy RequestInit.

24-27: 💤 Low value

Remove or rewrite the JSDoc to avoid explaining WHAT.

The comment restates what the function name already conveys. As per coding guidelines, use clear naming instead of comments that explain WHAT the code does. If documentation is needed, focus on WHY or non-obvious behavior.

As per coding guidelines: "Avoid code comments explaining WHAT — use clear naming instead".

🤖 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/_fetch.ts` around lines 24 - 27, Remove the redundant “what”
JSDoc at the top of src/providers/_fetch.ts that merely repeats the function
name; either delete it or replace it with a brief note explaining non-obvious
behavior or rationale (e.g., why the wrapper auto-detects HTTPS_PROXY/HTTP_PROXY
and when a custom fetchFn should be provided), and keep the code-level name (the
exported fetch wrapper) as the source of truth for what the function does.
src/providers/_openai-shared.ts (1)

34-37: 💤 Low value

Remove or rewrite JSDoc comments that explain WHAT.

Multiple JSDoc comments restate what the function names already convey:

  • detectAzure → "Returns true when baseUrl targets..."
  • detectFoundry → "Returns true when baseUrl targets..."
  • buildFoundryUrl → "Normalizes a Foundry base URL..."
  • buildFoundryHeaders → "Returns Anthropic Messages API auth headers..."
  • buildChatUrl → "Builds the chat completions URL..."
  • buildEmbeddingUrl → "Builds the embeddings URL..."
  • buildAuthHeaders → "Returns request auth headers..."
  • normalizeBaseUrl → "Strips trailing slashes..."

If documentation is needed, focus on non-obvious behavior or WHY rather than WHAT.

As per coding guidelines: "Avoid code comments explaining WHAT — use clear naming instead".

Also applies to: 51-54, 64-64, 72-72, 127-127, 141-141, 155-159, 176-176

🤖 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 34 - 37, Remove or rewrite the
JSDoc comments that merely restate function names (detectAzure, detectFoundry,
buildFoundryUrl, buildFoundryHeaders, buildChatUrl, buildEmbeddingUrl,
buildAuthHeaders, normalizeBaseUrl and the other occurrences noted) so the
codebase doesn't duplicate WHAT; either delete those comments or replace them
with short notes that document non-obvious behavior, edge-cases, assumptions, or
WHY the implementation is needed (e.g., normalization rules, header composition
decisions, or Azure/Foundry hostname patterns) and keep comments focused on
intent rather than repeating the function signature.
src/providers/index.ts (1)

28-28: 💤 Low value

Remove or rewrite comments that explain WHAT.

The JSDoc on line 28 ("Creates a resilient provider wrapping...") and the inline comment on lines 118-121 restate what the code already shows. The function name createProvider and the switch case for "azure-openai" with the OpenAIProvider instantiation are self-explanatory.

As per coding guidelines: "Avoid code comments explaining WHAT — use clear naming instead".

Also applies to: 118-121

🤖 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/index.ts` at line 28, Remove the redundant JSDoc that merely
repeats the function name and behavior and delete or reword the inline comment
that duplicates the switch branch logic; specifically, remove or replace the
comment above createProvider and the explanatory comment adjacent to the
"azure-openai" switch case where OpenAIProvider is instantiated, leaving clear
self-descriptive names (createProvider, OpenAIProvider) to convey intent or
replace with a short WHY-focused note if additional context is needed.
🤖 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/_fetch.ts`:
- Line 33: The fetchFn parameter currently types its second argument as any,
reducing type safety; update the signature to use the standard RequestInit type
(e.g., change fetchFn?: (url: string, init: any) => Promise<Response> to use
RequestInit) and ensure any necessary imports/types are available in the file so
callers get proper typing for headers, method, body, etc.; reference the fetchFn
parameter in src/providers/_fetch.ts and adjust callers if their argument types
need to satisfy RequestInit.
- Around line 24-27: Remove the redundant “what” JSDoc at the top of
src/providers/_fetch.ts that merely repeats the function name; either delete it
or replace it with a brief note explaining non-obvious behavior or rationale
(e.g., why the wrapper auto-detects HTTPS_PROXY/HTTP_PROXY and when a custom
fetchFn should be provided), and keep the code-level name (the exported fetch
wrapper) as the source of truth for what the function does.

In `@src/providers/_openai-shared.ts`:
- Around line 34-37: Remove or rewrite the JSDoc comments that merely restate
function names (detectAzure, detectFoundry, buildFoundryUrl,
buildFoundryHeaders, buildChatUrl, buildEmbeddingUrl, buildAuthHeaders,
normalizeBaseUrl and the other occurrences noted) so the codebase doesn't
duplicate WHAT; either delete those comments or replace them with short notes
that document non-obvious behavior, edge-cases, assumptions, or WHY the
implementation is needed (e.g., normalization rules, header composition
decisions, or Azure/Foundry hostname patterns) and keep comments focused on
intent rather than repeating the function signature.

In `@src/providers/index.ts`:
- Line 28: Remove the redundant JSDoc that merely repeats the function name and
behavior and delete or reword the inline comment that duplicates the switch
branch logic; specifically, remove or replace the comment above createProvider
and the explanatory comment adjacent to the "azure-openai" switch case where
OpenAIProvider is instantiated, leaving clear self-descriptive names
(createProvider, OpenAIProvider) to convey intent or replace with a short
WHY-focused note if additional context is needed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 56e9ddf3-e432-484b-99c9-e55957bb0f94

📥 Commits

Reviewing files that changed from the base of the PR and between fa01cd0 and 77743da.

📒 Files selected for processing (16)
  • plugin/scripts/notification.mjs
  • plugin/scripts/post-commit.mjs
  • plugin/scripts/post-tool-failure.mjs
  • plugin/scripts/post-tool-use.mjs
  • plugin/scripts/pre-compact.mjs
  • plugin/scripts/pre-tool-use.mjs
  • plugin/scripts/prompt-submit.mjs
  • plugin/scripts/session-end.mjs
  • plugin/scripts/session-start.mjs
  • plugin/scripts/stop.mjs
  • plugin/scripts/subagent-start.mjs
  • plugin/scripts/subagent-stop.mjs
  • plugin/scripts/task-completed.mjs
  • src/providers/_fetch.ts
  • src/providers/_openai-shared.ts
  • src/providers/index.ts
✅ Files skipped from review due to trivial changes (12)
  • plugin/scripts/post-tool-failure.mjs
  • plugin/scripts/stop.mjs
  • plugin/scripts/subagent-stop.mjs
  • plugin/scripts/subagent-start.mjs
  • plugin/scripts/notification.mjs
  • plugin/scripts/task-completed.mjs
  • plugin/scripts/pre-tool-use.mjs
  • plugin/scripts/post-tool-use.mjs
  • plugin/scripts/pre-compact.mjs
  • plugin/scripts/session-end.mjs
  • plugin/scripts/prompt-submit.mjs
  • plugin/scripts/post-commit.mjs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant