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

describe("reasoning effort mapping (OpenRouter)", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor organizational suggestion: This new test is in a separate describe block at the end of the file. Would it make more sense to integrate it with the existing "OpenRouterHandler" describe block above for better test organization?

it("passes 'minimal' through in reasoning.effort for OpenRouter requests", async () => {
const handler = new OpenRouterHandler({
openRouterApiKey: "test-key",
openRouterModelId: "openai/o1-pro",
reasoningEffort: "minimal",
} as ApiHandlerOptions)

// Prepare a model that supports reasoning effort (not budget)
;(handler as any).models = {
"openai/o1-pro": {
maxTokens: 8192,
contextWindow: 200000,
supportsImages: true,
supportsPromptCache: true,
inputPrice: 0.0,
outputPrice: 0.0,
description: "o1-pro test",
supportsReasoningEffort: true,
},
}

// Ensure endpoints map is empty so base model info is used
;(handler as any).endpoints = {}

// Mock OpenAI client call
const mockCreate = vitest.fn().mockResolvedValue({
async *[Symbol.asyncIterator]() {
yield {
id: "openai/o1-pro",
choices: [{ delta: { content: "ok" } }],
}
yield {
id: "usage-id",
choices: [{ delta: {} }],
usage: { prompt_tokens: 1, completion_tokens: 1, cost: 0 },
}
},
})
;(OpenAI as any).prototype.chat = {
completions: { create: mockCreate },
} as any

const systemPrompt = "system"
const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "hello" }]

// Stub fetchModel to use the handler's getModel (which applies getModelParams -> getOpenRouterReasoning)
const realGetModel = (handler as any).getModel.bind(handler)
;(handler as any).fetchModel = vitest.fn().mockImplementation(async () => realGetModel())

// Trigger a request
const gen = handler.createMessage(systemPrompt, messages)
// Drain iterator to ensure call is made
for await (const _ of gen) {
// noop
}

// Verify the API call included the normalized effort
expect(mockCreate).toHaveBeenCalledWith(
expect.objectContaining({
model: "openai/o1-pro",
reasoning: { effort: "minimal" }, // 'minimal' should be preserved for OpenRouter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great test coverage! Though I'm wondering if we should also test the edge case where reasoningEffort is undefined while supportsReasoningEffort is true? This would ensure we handle all scenarios properly.

stream: true,
}),
)
})
})
37 changes: 35 additions & 2 deletions src/api/transform/__tests__/reasoning.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,21 +530,54 @@ describe("reasoning.ts", () => {
expect(result).toEqual({ reasoning_effort: undefined })
})

it("should handle all reasoning effort values", () => {
it("should handle standard reasoning effort values", () => {
const efforts: Array<"low" | "medium" | "high"> = ["low", "medium", "high"]

efforts.forEach((effort) => {
const modelWithEffort: ModelInfo = {
...baseModel,
supportsReasoningEffort: true,
}

const settingsWithEffort: ProviderSettings = {
reasoningEffort: effort,
}

const options = { ...baseOptions, model: modelWithEffort, reasoningEffort: effort }
const options = {
...baseOptions,
model: modelWithEffort,
settings: settingsWithEffort,
reasoningEffort: effort,
}
const result = getOpenAiReasoning(options)
expect(result).toEqual({ reasoning_effort: effort })
})
})

it("should filter out minimal reasoning effort for OpenAI SDK compatibility", () => {
const modelWithEffort: ModelInfo = {
...baseModel,
supportsReasoningEffort: true,
}

const settingsWithMinimal: ProviderSettings = {
reasoningEffort: "minimal",
}

const options = {
...baseOptions,
model: modelWithEffort,
settings: settingsWithMinimal,
reasoningEffort: "minimal" as ReasoningEffortWithMinimal,
}

const result = getOpenAiReasoning(options)

// "minimal" is filtered out for OpenAI SDK compatibility
// OpenRouter handles "minimal" correctly in its own function
expect(result).toBeUndefined()
})

it("should not be affected by reasoningBudget parameter", () => {
const modelWithEffort: ModelInfo = {
...baseModel,
Expand Down
29 changes: 19 additions & 10 deletions src/api/transform/reasoning.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,21 @@ export const getOpenRouterReasoning = ({
reasoningBudget,
reasoningEffort,
settings,
}: GetModelReasoningOptions): OpenRouterReasoningParams | undefined =>
shouldUseReasoningBudget({ model, settings })
? { max_tokens: reasoningBudget }
: shouldUseReasoningEffort({ model, settings })
? reasoningEffort
? { effort: reasoningEffort }
: undefined
: undefined
}: GetModelReasoningOptions): OpenRouterReasoningParams | undefined => {
// If the model uses a budget-style reasoning config on OpenRouter, pass it through.
if (shouldUseReasoningBudget({ model, settings })) {
return { max_tokens: reasoningBudget! }
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 this intentional? The non-null assertion operator here could potentially cause runtime errors if reasoningBudget is undefined. Consider adding explicit validation or providing a default value:

Suggested change
return { max_tokens: reasoningBudget! }
return { max_tokens: reasoningBudget ?? 0 }

}

// Otherwise, if we support traditional reasoning effort, pass through the effort.
// Note: Some models (e.g., GPT‑5 via OpenRouter) may support "minimal".
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 comment mentions "GPT-5 via OpenRouter" specifically, but this change affects all models using OpenRouter with reasoning effort support. Could we make this comment more generic to avoid confusion? Something like:

Suggested change
// Note: Some models (e.g., GPT‑5 via OpenRouter) may support "minimal".
// Otherwise, if we support traditional reasoning effort, pass through the effort.
// Note: OpenRouter models may support additional effort levels like "minimal".

if (shouldUseReasoningEffort({ model, settings })) {
if (!reasoningEffort) return undefined
return { effort: reasoningEffort }
}

return undefined
}

export const getAnthropicReasoning = ({
model,
Expand All @@ -55,8 +62,10 @@ export const getOpenAiReasoning = ({
return undefined
}

// If model has reasoning effort capability, return object even if effort is undefined
// This preserves the reasoning_effort field in the API call
// Note: The OpenAI SDK doesn't include "minimal" in its type definitions,
// but GPT-5 via OpenRouter does support it. Since this function is for
// direct OpenAI API usage (not OpenRouter), we filter out "minimal" here.
// OpenRouter handles "minimal" correctly in getOpenRouterReasoning.
if (reasoningEffort === "minimal") {
return undefined
}
Expand Down
Loading