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
24 changes: 24 additions & 0 deletions src/api/providers/__tests__/openrouter.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,30 @@ describe("OpenRouterHandler", () => {
})
})

it("should warn when API key is missing or invalid", () => {
const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {})

// Test with missing API key
new OpenRouterHandler({})
expect(consoleSpy).toHaveBeenCalledWith(
"OpenRouter API key is missing or invalid. This may cause authentication errors.",
)

// Test with empty API key
new OpenRouterHandler({ openRouterApiKey: "" })
expect(consoleSpy).toHaveBeenCalledWith(
"OpenRouter API key is missing or invalid. This may cause authentication errors.",
)

// Test with whitespace-only API key
new OpenRouterHandler({ openRouterApiKey: " " })
expect(consoleSpy).toHaveBeenCalledWith(
"OpenRouter API key is missing or invalid. This may cause authentication errors.",
)

consoleSpy.mockRestore()
})

describe("fetchModel", () => {
it("returns correct model info when options are provided", async () => {
const handler = new OpenRouterHandler(mockOptions)
Expand Down
69 changes: 69 additions & 0 deletions src/api/providers/fetchers/__tests__/openrouter.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from "@roo-code/types"

import { getOpenRouterModelEndpoints, getOpenRouterModels } from "../openrouter"
import axios from "axios"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The axios import was added here but wasn't present in the original file structure. Could you verify this import is actually needed and properly available in the test environment?


nockBack.fixtures = path.join(__dirname, "fixtures")
nockBack.setMode("lockdown")
Expand Down Expand Up @@ -250,5 +251,73 @@ describe("OpenRouter API", () => {

nockDone()
})

it("should include authorization header when API key is provided", async () => {
const mockAxiosGet = vi.spyOn(axios, "get").mockResolvedValue({
data: { data: [] },
})

await getOpenRouterModels({ openRouterApiKey: "test-api-key" })

expect(mockAxiosGet).toHaveBeenCalledWith("https://openrouter.ai/api/v1/models", {
headers: { Authorization: "Bearer test-api-key" },
})

mockAxiosGet.mockRestore()
})

it("should not include authorization header when API key is not provided", async () => {
const mockAxiosGet = vi.spyOn(axios, "get").mockResolvedValue({
data: { data: [] },
})

await getOpenRouterModels()

expect(mockAxiosGet).toHaveBeenCalledWith("https://openrouter.ai/api/v1/models", { headers: {} })

mockAxiosGet.mockRestore()
})

it("should throw authentication error on 401 response", async () => {
const mockAxiosGet = vi.spyOn(axios, "get").mockRejectedValue({
isAxiosError: true,
response: { status: 401 },
})

await expect(getOpenRouterModels({ openRouterApiKey: "invalid-key" })).rejects.toThrow(
"OpenRouter API authentication failed. Please check your API key.",
)

mockAxiosGet.mockRestore()
})
})

describe("getOpenRouterModelEndpoints", () => {
it("should include authorization header when API key is provided", async () => {
const mockAxiosGet = vi.spyOn(axios, "get").mockResolvedValue({
data: { data: { id: "test", name: "test", endpoints: [] } },
})

await getOpenRouterModelEndpoints("test-model", { openRouterApiKey: "test-api-key" })

expect(mockAxiosGet).toHaveBeenCalledWith("https://openrouter.ai/api/v1/models/test-model/endpoints", {
headers: { Authorization: "Bearer test-api-key" },
})

mockAxiosGet.mockRestore()
})

it("should throw authentication error on 401 response", async () => {
const mockAxiosGet = vi.spyOn(axios, "get").mockRejectedValue({
isAxiosError: true,
response: { status: 401 },
})

await expect(
getOpenRouterModelEndpoints("test-model", { openRouterApiKey: "invalid-key" }),
).rejects.toThrow("OpenRouter API authentication failed. Please check your API key.")

mockAxiosGet.mockRestore()
})
})
})
5 changes: 4 additions & 1 deletion src/api/providers/fetchers/modelCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ export const getModels = async (options: GetModelsOptions): Promise<ModelRecord>
try {
switch (provider) {
case "openrouter":
models = await getOpenRouterModels()
models = await getOpenRouterModels({
openRouterApiKey: options.apiKey,
openRouterBaseUrl: options.baseUrl,
})
break
case "requesty":
// Requesty models endpoint requires an API key for per-user custom policies
Expand Down
26 changes: 24 additions & 2 deletions src/api/providers/fetchers/openrouter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,14 @@ export async function getOpenRouterModels(options?: ApiHandlerOptions): Promise<
const models: Record<string, ModelInfo> = {}
const baseURL = options?.openRouterBaseUrl || "https://openrouter.ai/api/v1"

// Prepare headers with API key if available
const headers: Record<string, string> = {}
if (options?.openRouterApiKey) {
headers.Authorization = `Bearer ${options.openRouterApiKey}`
}

try {
const response = await axios.get<OpenRouterModelsResponse>(`${baseURL}/models`)
const response = await axios.get<OpenRouterModelsResponse>(`${baseURL}/models`, { headers })
const result = openRouterModelsResponseSchema.safeParse(response.data)
const data = result.success ? result.data.data : response.data.data

Expand All @@ -118,6 +124,10 @@ export async function getOpenRouterModels(options?: ApiHandlerOptions): Promise<
})
}
} catch (error) {
if (axios.isAxiosError(error) && error.response?.status === 401) {
console.error("OpenRouter API authentication failed. Please check your API key.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the duplication of the same error message in both console.error and throw new Error intentional? Could we streamline this to avoid redundant logging?

throw new Error("OpenRouter API authentication failed. Please check your API key.")
}
console.error(
`Error fetching OpenRouter models: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,
)
Expand All @@ -137,8 +147,16 @@ export async function getOpenRouterModelEndpoints(
const models: Record<string, ModelInfo> = {}
const baseURL = options?.openRouterBaseUrl || "https://openrouter.ai/api/v1"

// Prepare headers with API key if available
const headers: Record<string, string> = {}
if (options?.openRouterApiKey) {
headers.Authorization = `Bearer ${options.openRouterApiKey}`
}

try {
const response = await axios.get<OpenRouterModelEndpointsResponse>(`${baseURL}/models/${modelId}/endpoints`)
const response = await axios.get<OpenRouterModelEndpointsResponse>(`${baseURL}/models/${modelId}/endpoints`, {
headers,
})
const result = openRouterModelEndpointsResponseSchema.safeParse(response.data)
const data = result.success ? result.data.data : response.data.data

Expand All @@ -157,6 +175,10 @@ export async function getOpenRouterModelEndpoints(
})
}
} catch (error) {
if (axios.isAxiosError(error) && error.response?.status === 401) {
console.error("OpenRouter API authentication failed. Please check your API key.")
throw new Error("OpenRouter API authentication failed. Please check your API key.")
}
console.error(
`Error fetching OpenRouter model endpoints: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,
)
Expand Down
12 changes: 11 additions & 1 deletion src/api/providers/openrouter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH
const baseURL = this.options.openRouterBaseUrl || "https://openrouter.ai/api/v1"
const apiKey = this.options.openRouterApiKey ?? "not-provided"

// Validate API key format
if (apiKey === "not-provided" || !apiKey || apiKey.trim() === "") {
console.warn("OpenRouter API key is missing or invalid. This may cause authentication errors.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API key validation happens in the constructor but API calls occur later. Could we consider more strict validation (e.g., checking expected prefixes or length) to catch format issues earlier?

}

this.client = new OpenAI({ baseURL, apiKey, defaultHeaders: DEFAULT_HEADERS })
}

Expand Down Expand Up @@ -175,11 +180,16 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH

public async fetchModel() {
const [models, endpoints] = await Promise.all([
getModels({ provider: "openrouter" }),
getModels({
provider: "openrouter",
apiKey: this.options.openRouterApiKey,
baseUrl: this.options.openRouterBaseUrl,
}),
getModelEndpoints({
router: "openrouter",
modelId: this.options.openRouterModelId,
endpoint: this.options.openRouterSpecificProvider,
...this.options,
}),
])

Expand Down
2 changes: 1 addition & 1 deletion src/shared/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export const getModelMaxOutputTokens = ({
// GetModelsOptions

export type GetModelsOptions =
| { provider: "openrouter" }
| { provider: "openrouter"; apiKey?: string; baseUrl?: string }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GetModelsOptions type makes apiKey optional, but the error handling suggests it's expected. Could we make this more explicit in the type definition for better type safety?

| { provider: "glama" }
| { provider: "requesty"; apiKey?: string }
| { provider: "unbound"; apiKey?: string }
Expand Down