Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 64 additions & 48 deletions src/api/providers/__tests__/openai.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { Anthropic } from "@anthropic-ai/sdk"
import OpenAI from "openai"
import { openAiModelInfoSaneDefaults } from "@roo-code/types"
import { Package } from "../../../shared/package"
import axios from "axios"

const mockCreate = vitest.fn()

Expand Down Expand Up @@ -69,12 +68,9 @@ vitest.mock("openai", () => {
}
})

// Mock axios for getOpenAiModels tests
vitest.mock("axios", () => ({
default: {
get: vitest.fn(),
},
}))
// Mock fetch for getOpenAiModels tests
const mockFetch = vitest.fn()
global.fetch = mockFetch

describe("OpenAiHandler", () => {
let handler: OpenAiHandler
Expand Down Expand Up @@ -787,80 +783,84 @@ describe("OpenAiHandler", () => {

describe("getOpenAiModels", () => {
beforeEach(() => {
vi.mocked(axios.get).mockClear()
mockFetch.mockClear()
})

it("should return empty array when baseUrl is not provided", async () => {
const result = await getOpenAiModels(undefined, "test-key")
expect(result).toEqual([])
expect(axios.get).not.toHaveBeenCalled()
expect(mockFetch).not.toHaveBeenCalled()
})

it("should return empty array when baseUrl is empty string", async () => {
const result = await getOpenAiModels("", "test-key")
expect(result).toEqual([])
expect(axios.get).not.toHaveBeenCalled()
expect(mockFetch).not.toHaveBeenCalled()
})

it("should trim whitespace from baseUrl", async () => {
const mockResponse = {
data: {
data: [{ id: "gpt-4" }, { id: "gpt-3.5-turbo" }],
},
const mockResponseData = {
data: [{ id: "gpt-4" }, { id: "gpt-3.5-turbo" }],
}
vi.mocked(axios.get).mockResolvedValueOnce(mockResponse)
mockFetch.mockResolvedValueOnce({
ok: true,
json: async () => mockResponseData,
})

const result = await getOpenAiModels(" https://api.openai.com/v1 ", "test-key")

expect(axios.get).toHaveBeenCalledWith("https://api.openai.com/v1/models", expect.any(Object))
expect(mockFetch).toHaveBeenCalledWith("https://api.openai.com/v1/models", expect.any(Object))
expect(result).toEqual(["gpt-4", "gpt-3.5-turbo"])
})

it("should handle baseUrl with trailing spaces", async () => {
const mockResponse = {
data: {
data: [{ id: "model-1" }, { id: "model-2" }],
},
const mockResponseData = {
data: [{ id: "model-1" }, { id: "model-2" }],
}
vi.mocked(axios.get).mockResolvedValueOnce(mockResponse)
mockFetch.mockResolvedValueOnce({
ok: true,
json: async () => mockResponseData,
})

const result = await getOpenAiModels("https://api.example.com/v1 ", "test-key")

expect(axios.get).toHaveBeenCalledWith("https://api.example.com/v1/models", expect.any(Object))
expect(mockFetch).toHaveBeenCalledWith("https://api.example.com/v1/models", expect.any(Object))
expect(result).toEqual(["model-1", "model-2"])
})

it("should handle baseUrl with leading spaces", async () => {
const mockResponse = {
data: {
data: [{ id: "model-1" }],
},
const mockResponseData = {
data: [{ id: "model-1" }],
}
vi.mocked(axios.get).mockResolvedValueOnce(mockResponse)
mockFetch.mockResolvedValueOnce({
ok: true,
json: async () => mockResponseData,
})

const result = await getOpenAiModels(" https://api.example.com/v1", "test-key")

expect(axios.get).toHaveBeenCalledWith("https://api.example.com/v1/models", expect.any(Object))
expect(mockFetch).toHaveBeenCalledWith("https://api.example.com/v1/models", expect.any(Object))
expect(result).toEqual(["model-1"])
})

it("should return empty array for invalid URL after trimming", async () => {
const result = await getOpenAiModels(" not-a-valid-url ", "test-key")
expect(result).toEqual([])
expect(axios.get).not.toHaveBeenCalled()
expect(mockFetch).not.toHaveBeenCalled()
})

it("should include authorization header when apiKey is provided", async () => {
const mockResponse = {
data: {
data: [{ id: "model-1" }],
},
const mockResponseData = {
data: [{ id: "model-1" }],
}
vi.mocked(axios.get).mockResolvedValueOnce(mockResponse)
mockFetch.mockResolvedValueOnce({
ok: true,
json: async () => mockResponseData,
})

await getOpenAiModels("https://api.example.com/v1", "test-api-key")

expect(axios.get).toHaveBeenCalledWith(
expect(mockFetch).toHaveBeenCalledWith(
"https://api.example.com/v1/models",
expect.objectContaining({
headers: expect.objectContaining({
Expand All @@ -871,20 +871,21 @@ describe("getOpenAiModels", () => {
})

it("should include custom headers when provided", async () => {
const mockResponse = {
data: {
data: [{ id: "model-1" }],
},
const mockResponseData = {
data: [{ id: "model-1" }],
}
vi.mocked(axios.get).mockResolvedValueOnce(mockResponse)
mockFetch.mockResolvedValueOnce({
ok: true,
json: async () => mockResponseData,
})

const customHeaders = {
"X-Custom-Header": "custom-value",
}

await getOpenAiModels("https://api.example.com/v1", "test-key", customHeaders)

expect(axios.get).toHaveBeenCalledWith(
expect(mockFetch).toHaveBeenCalledWith(
"https://api.example.com/v1/models",
expect.objectContaining({
headers: expect.objectContaining({
Expand All @@ -896,28 +897,43 @@ describe("getOpenAiModels", () => {
})

it("should handle API errors gracefully", async () => {
vi.mocked(axios.get).mockRejectedValueOnce(new Error("Network error"))
mockFetch.mockRejectedValueOnce(new Error("Network error"))

const result = await getOpenAiModels("https://api.example.com/v1", "test-key")

expect(result).toEqual([])
})

it("should handle non-ok response", async () => {
mockFetch.mockResolvedValueOnce({
ok: false,
status: 401,
})

const result = await getOpenAiModels("https://api.example.com/v1", "test-key")

expect(result).toEqual([])
})

it("should handle malformed response data", async () => {
vi.mocked(axios.get).mockResolvedValueOnce({ data: null })
mockFetch.mockResolvedValueOnce({
ok: true,
json: async () => null,
})

const result = await getOpenAiModels("https://api.example.com/v1", "test-key")

expect(result).toEqual([])
})

it("should deduplicate model IDs", async () => {
const mockResponse = {
data: {
data: [{ id: "gpt-4" }, { id: "gpt-4" }, { id: "gpt-3.5-turbo" }, { id: "gpt-4" }],
},
const mockResponseData = {
data: [{ id: "gpt-4" }, { id: "gpt-4" }, { id: "gpt-3.5-turbo" }, { id: "gpt-4" }],
}
vi.mocked(axios.get).mockResolvedValueOnce(mockResponse)
mockFetch.mockResolvedValueOnce({
ok: true,
json: async () => mockResponseData,
})

const result = await getOpenAiModels("https://api.example.com/v1", "test-key")

Expand Down
17 changes: 14 additions & 3 deletions src/api/providers/lm-studio.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Anthropic } from "@anthropic-ai/sdk"
import OpenAI from "openai"
import axios from "axios"

import { type ModelInfo, openAiModelInfoSaneDefaults, LMSTUDIO_DEFAULT_TEMPERATURE } from "@roo-code/types"

Expand Down Expand Up @@ -177,8 +176,20 @@ export async function getLmStudioModels(baseUrl = "http://localhost:1234") {
return []
}

const response = await axios.get(`${baseUrl}/v1/models`)
const modelsArray = response.data?.data?.map((model: any) => model.id) || []
// Use fetch instead of axios to respect VSCode's proxy settings
const response = await fetch(`${baseUrl}/v1/models`, {
method: "GET",
headers: {
"Content-Type": "application/json",
},
})

if (!response.ok) {
return []
}

const data = await response.json()
const modelsArray = data?.data?.map((model: any) => model.id) || []
return [...new Set<string>(modelsArray)]
} catch (error) {
return []
Expand Down
17 changes: 11 additions & 6 deletions src/api/providers/openai.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Anthropic } from "@anthropic-ai/sdk"
import OpenAI, { AzureOpenAI } from "openai"
import axios from "axios"

import {
type ModelInfo,
Expand Down Expand Up @@ -423,7 +422,6 @@ export async function getOpenAiModels(baseUrl?: string, apiKey?: string, openAiH
return []
}

const config: Record<string, any> = {}
const headers: Record<string, string> = {
...DEFAULT_HEADERS,
...(openAiHeaders || {}),
Expand All @@ -433,12 +431,19 @@ export async function getOpenAiModels(baseUrl?: string, apiKey?: string, openAiH
headers["Authorization"] = `Bearer ${apiKey}`
}

if (Object.keys(headers).length > 0) {
config["headers"] = headers
// Use fetch instead of axios to respect VSCode's proxy settings
// This matches how the OpenAI SDK makes requests internally
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be helpful to add a reference to the issue this fixes for future maintainability?

Suggested change
// This matches how the OpenAI SDK makes requests internally
// Use fetch instead of axios to respect VSCode's proxy settings
// This matches how the OpenAI SDK makes requests internally
// Fixes #6991: Model fetching now respects VSCode proxy settings (PAC files, etc.)

const response = await fetch(`${trimmedBaseUrl}/models`, {
method: "GET",
headers: headers,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with the LM Studio implementation, should we include a Content-Type header here? While it's not strictly necessary for GET requests, maintaining consistency across our fetch implementations would be good:

Suggested change
headers: headers,
const response = await fetch(`${trimmedBaseUrl}/models`, {
method: "GET",
headers: {
"Content-Type": "application/json",
...headers
},
})

})

if (!response.ok) {
return []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider adding error logging here to help debug issues when model fetching fails. Currently we silently return an empty array for non-ok responses, which might make troubleshooting difficult:

Suggested change
return []
if (!response.ok) {
console.error(`Failed to fetch OpenAI models: ${response.status} ${response.statusText}`);
return []
}

}

const response = await axios.get(`${trimmedBaseUrl}/models`, config)
const modelsArray = response.data?.data?.map((model: any) => model.id) || []
const data = await response.json()
const modelsArray = data?.data?.map((model: any) => model.id) || []
return [...new Set<string>(modelsArray)]
} catch (error) {
return []
Expand Down
Loading