-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve Azure OpenAI connectivity and reasoning model support #6135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Use azureApiVersion as primary indicator for Azure detection (like Cline) - Add URL extraction method to handle Azure URLs with query parameters - Fix reasoning model support for Azure o1/o3/o4 models by handling system messages - Add better error messages for Azure-specific issues Fixes #1334
| const urlHost = this._getUrlHost(this.options.openAiBaseUrl) | ||
| const isAzureOpenAi = urlHost === "azure.com" || urlHost.endsWith(".azure.com") || options.openAiUseAzure | ||
| // Use azureApiVersion as primary indicator (like Cline), then fall back to URL patterns | ||
| const isAzureOpenAi = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enhanced Azure endpoint detection using azureApiVersion is clear, but the logic to compute isAzureOpenAi is repeated (e.g., here and again in createMessage). Consider extracting this check into a helper function to avoid redundancy.
| const requestOptions: OpenAI.Chat.Completions.ChatCompletionCreateParamsStreaming = { | ||
| model: modelId, | ||
| messages: [ | ||
| // Azure doesn't support "developer" role, so we need to combine system prompt with first user message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to merge the system prompt with the first user message for Azure endpoints is duplicated in both streaming and non‐streaming branches (and also in handleO3FamilyMessage). Consider refactoring this into a utility/helper function to enhance maintainability.
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
| try { | ||
| const url = new URL(fullUrl) | ||
| // For Azure OpenAI, we want just the origin (protocol + host) | ||
| if (url.host.includes("azure.com")) { |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
azure.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, replace the substring check url.host.includes("azure.com") with a more robust validation mechanism that ensures the host matches an explicit whitelist of allowed domains. This approach prevents bypasses by malicious URLs and ensures that only legitimate Azure domains are accepted.
The fix involves:
- Defining a whitelist of allowed Azure domains (e.g.,
["azure.com"]). - Using
url.hostto check if the host matches one of the allowed domains exactly or ends with one of the allowed domains (to account for subdomains likeservices.ai.azure.com).
Changes are required in the _extractAzureBaseUrl method to implement this validation.
-
Copy modified lines R499-R500
| @@ -498,3 +498,4 @@ | ||
| // For Azure OpenAI, we want just the origin (protocol + host) | ||
| if (url.host.includes("azure.com")) { | ||
| const allowedAzureDomains = ["azure.com"]; | ||
| if (allowedAzureDomains.some(domain => url.host === domain || url.host.endsWith(`.${domain}`))) { | ||
| return url.origin |
|
Dupe! I was quoting another post and it thought I was genuinely tagging it and not juts repeating within my quote. Please close. |
Summary
This PR improves Azure OpenAI connectivity by implementing the same approach used by Cline, addressing the issues reported in #1334.
Key Changes
Enhanced Azure Detection Logic
azureApiVersionas the primary indicator for Azure endpoints (like Cline)URL Extraction Method
_extractAzureBaseUrl()method to properly handle Azure URLs with query parametershttps://myresource.openai.azure.com/openai/deployments/gpt-4/chat/completions?api-version=2024-08-01-preview→https://myresource.openai.azure.com)Fixed Reasoning Model Support
Better Error Messages
Testing
Fixes
Fixes #1334
Context
As requested by @potatoqualitee, I examined how Cline implemented Azure AI Foundry connectivity and identified the key differences that were causing issues in Roo Code.
Important
Improves Azure OpenAI connectivity and reasoning model support by enhancing detection logic, URL handling, and error messages in
openai.ts.azureApiVersionas primary indicator for Azure endpoints inOpenAiHandler._extractAzureBaseUrl()to extract base URL from Azure endpoint URLs inopenai.ts.handleO3FamilyMessage().OpenAiHandler.createMessage()andhandleO3FamilyMessage().This description was created by
for 6e9abec. You can customize this summary. It will automatically update as commits are pushed.