From 5bd5e29f6f3c657784b08df449ca16439c34bc0b Mon Sep 17 00:00:00 2001 From: cte Date: Fri, 23 May 2025 22:28:11 -0700 Subject: [PATCH 1/2] Fix o1-pro on OpenRouter --- .../{ollama.test.ts => ollama.spec.ts} | 11 ++- ...i-native.test.ts => openai-native.spec.ts} | 78 ++++++++++++++++++- ....test.ts => openai-usage-tracking.spec.ts} | 16 ++-- .../{openai.test.ts => openai.spec.ts} | 19 ++--- src/api/providers/openai-native.ts | 10 +-- src/api/transform/model-params.ts | 17 +++- src/vitest.config.ts | 5 +- 7 files changed, 123 insertions(+), 33 deletions(-) rename src/api/providers/__tests__/{ollama.test.ts => ollama.spec.ts} (95%) rename src/api/providers/__tests__/{openai-native.test.ts => openai-native.spec.ts} (82%) rename src/api/providers/__tests__/{openai-usage-tracking.test.ts => openai-usage-tracking.spec.ts} (96%) rename src/api/providers/__tests__/{openai.test.ts => openai.spec.ts} (96%) diff --git a/src/api/providers/__tests__/ollama.test.ts b/src/api/providers/__tests__/ollama.spec.ts similarity index 95% rename from src/api/providers/__tests__/ollama.test.ts rename to src/api/providers/__tests__/ollama.spec.ts index 91b1468421..650ccfcdfc 100644 --- a/src/api/providers/__tests__/ollama.test.ts +++ b/src/api/providers/__tests__/ollama.spec.ts @@ -1,14 +1,17 @@ +// npx vitest run api/providers/__tests__/ollama.spec.ts + +import { vitest } from "vitest" import { Anthropic } from "@anthropic-ai/sdk" import { OllamaHandler } from "../ollama" import { ApiHandlerOptions } from "../../../shared/api" -// Mock OpenAI client -const mockCreate = jest.fn() -jest.mock("openai", () => { +const mockCreate = vitest.fn() + +vitest.mock("openai", () => { return { __esModule: true, - default: jest.fn().mockImplementation(() => ({ + default: vitest.fn().mockImplementation(() => ({ chat: { completions: { create: mockCreate.mockImplementation(async (options) => { diff --git a/src/api/providers/__tests__/openai-native.test.ts b/src/api/providers/__tests__/openai-native.spec.ts similarity index 82% rename from src/api/providers/__tests__/openai-native.test.ts rename to src/api/providers/__tests__/openai-native.spec.ts index c59eb70890..b0635d9c97 100644 --- a/src/api/providers/__tests__/openai-native.test.ts +++ b/src/api/providers/__tests__/openai-native.spec.ts @@ -1,17 +1,18 @@ -// npx jest src/api/providers/__tests__/openai-native.test.ts +// npx vitest run api/providers/__tests__/openai-native.spec.ts +import { vitest } from "vitest" import { Anthropic } from "@anthropic-ai/sdk" import { OpenAiNativeHandler } from "../openai-native" import { ApiHandlerOptions } from "../../../shared/api" // Mock OpenAI client -const mockCreate = jest.fn() +const mockCreate = vitest.fn() -jest.mock("openai", () => { +vitest.mock("openai", () => { return { __esModule: true, - default: jest.fn().mockImplementation(() => ({ + default: vitest.fn().mockImplementation(() => ({ chat: { completions: { create: mockCreate.mockImplementation(async (options) => { @@ -372,6 +373,75 @@ describe("OpenAiNativeHandler", () => { }) }) + describe("temperature parameter handling", () => { + it("should include temperature for models that support it", async () => { + // Test with gpt-4.1 which supports temperature + handler = new OpenAiNativeHandler({ + apiModelId: "gpt-4.1", + openAiNativeApiKey: "test-api-key", + }) + + await handler.completePrompt("Test prompt") + expect(mockCreate).toHaveBeenCalledWith({ + model: "gpt-4.1", + messages: [{ role: "user", content: "Test prompt" }], + temperature: 0, + }) + }) + + it("should strip temperature for o1 family models", async () => { + const o1Models = ["o1", "o1-preview", "o1-mini"] + + for (const modelId of o1Models) { + handler = new OpenAiNativeHandler({ + apiModelId: modelId, + openAiNativeApiKey: "test-api-key", + }) + + mockCreate.mockClear() + await handler.completePrompt("Test prompt") + + const callArgs = mockCreate.mock.calls[0][0] + // Temperature should be undefined for o1 models + expect(callArgs.temperature).toBeUndefined() + expect(callArgs.model).toBe(modelId) + } + }) + + it("should strip temperature for o3-mini model", async () => { + handler = new OpenAiNativeHandler({ + apiModelId: "o3-mini", + openAiNativeApiKey: "test-api-key", + }) + + await handler.completePrompt("Test prompt") + + const callArgs = mockCreate.mock.calls[0][0] + // Temperature should be undefined for o3-mini models + expect(callArgs.temperature).toBeUndefined() + expect(callArgs.model).toBe("o3-mini") + expect(callArgs.reasoning_effort).toBe("medium") + }) + + it("should strip temperature in streaming mode for unsupported models", async () => { + handler = new OpenAiNativeHandler({ + apiModelId: "o1", + openAiNativeApiKey: "test-api-key", + }) + + const stream = handler.createMessage(systemPrompt, messages) + // Consume the stream + for await (const _chunk of stream) { + // Just consume the stream + } + + const callArgs = mockCreate.mock.calls[0][0] + expect(callArgs).not.toHaveProperty("temperature") + expect(callArgs.model).toBe("o1") + expect(callArgs.stream).toBe(true) + }) + }) + describe("getModel", () => { it("should return model info", () => { const modelInfo = handler.getModel() diff --git a/src/api/providers/__tests__/openai-usage-tracking.test.ts b/src/api/providers/__tests__/openai-usage-tracking.spec.ts similarity index 96% rename from src/api/providers/__tests__/openai-usage-tracking.test.ts rename to src/api/providers/__tests__/openai-usage-tracking.spec.ts index 6df9a0bca5..9888475f31 100644 --- a/src/api/providers/__tests__/openai-usage-tracking.test.ts +++ b/src/api/providers/__tests__/openai-usage-tracking.spec.ts @@ -1,13 +1,17 @@ -import { OpenAiHandler } from "../openai" -import { ApiHandlerOptions } from "../../../shared/api" +// npx vitest run api/providers/__tests__/openai-usage-tracking.spec.ts + +import { vitest } from "vitest" import { Anthropic } from "@anthropic-ai/sdk" -// Mock OpenAI client with multiple chunks that contain usage data -const mockCreate = jest.fn() -jest.mock("openai", () => { +import { ApiHandlerOptions } from "../../../shared/api" +import { OpenAiHandler } from "../openai" + +const mockCreate = vitest.fn() + +vitest.mock("openai", () => { return { __esModule: true, - default: jest.fn().mockImplementation(() => ({ + default: vitest.fn().mockImplementation(() => ({ chat: { completions: { create: mockCreate.mockImplementation(async (options) => { diff --git a/src/api/providers/__tests__/openai.test.ts b/src/api/providers/__tests__/openai.spec.ts similarity index 96% rename from src/api/providers/__tests__/openai.test.ts rename to src/api/providers/__tests__/openai.spec.ts index f975421985..81c0b45e41 100644 --- a/src/api/providers/__tests__/openai.test.ts +++ b/src/api/providers/__tests__/openai.spec.ts @@ -1,15 +1,18 @@ -// npx jest src/api/providers/__tests__/openai.test.ts +// npx vitest run api/providers/__tests__/openai.spec.ts +import { vitest, vi } from "vitest" import { OpenAiHandler } from "../openai" import { ApiHandlerOptions } from "../../../shared/api" import { Anthropic } from "@anthropic-ai/sdk" +import OpenAI from "openai" -// Mock OpenAI client -const mockCreate = jest.fn() -jest.mock("openai", () => { +const mockCreate = vitest.fn() + +vitest.mock("openai", () => { + const mockConstructor = vitest.fn() return { __esModule: true, - default: jest.fn().mockImplementation(() => ({ + default: mockConstructor.mockImplementation(() => ({ chat: { completions: { create: mockCreate.mockImplementation(async (options) => { @@ -94,10 +97,8 @@ describe("OpenAiHandler", () => { }) it("should set default headers correctly", () => { - // Get the mock constructor from the jest mock system - const openAiMock = jest.requireMock("openai").default - - expect(openAiMock).toHaveBeenCalledWith({ + // Check that the OpenAI constructor was called with correct parameters + expect(vi.mocked(OpenAI)).toHaveBeenCalledWith({ baseURL: expect.any(String), apiKey: expect.any(String), defaultHeaders: { diff --git a/src/api/providers/openai-native.ts b/src/api/providers/openai-native.ts index 6a6def900d..1999637228 100644 --- a/src/api/providers/openai-native.ts +++ b/src/api/providers/openai-native.ts @@ -165,7 +165,7 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio const info: ModelInfo = openAiNativeModels[id] - const { temperature, ...params } = getModelParams({ + const params = getModelParams({ format: "openai", modelId: id, model: info, @@ -175,13 +175,7 @@ export class OpenAiNativeHandler extends BaseProvider implements SingleCompletio // The o3 models are named like "o3-mini-[reasoning-effort]", which are // not valid model ids, so we need to strip the suffix. - // Also note that temperature is not supported for o1 and o3-mini. - return { - id: id.startsWith("o3-mini") ? "o3-mini" : id, - info, - ...params, - temperature: id.startsWith("o1") || id.startsWith("o3-mini") ? undefined : temperature, - } + return { id: id.startsWith("o3-mini") ? "o3-mini" : id, info, ...params } } async completePrompt(prompt: string): Promise { diff --git a/src/api/transform/model-params.ts b/src/api/transform/model-params.ts index 5e99e770bf..9abe613714 100644 --- a/src/api/transform/model-params.ts +++ b/src/api/transform/model-params.ts @@ -25,7 +25,7 @@ type GetModelParamsOptions = { type BaseModelParams = { maxTokens: number | undefined - temperature: number + temperature: number | undefined reasoningEffort: "low" | "medium" | "high" | undefined reasoningBudget: number | undefined } @@ -114,12 +114,27 @@ export function getModelParams({ reasoning: getAnthropicReasoning({ model, reasoningBudget, reasoningEffort, settings }), } } else if (format === "openai") { + // Special case for o1 and o3-mini, which don't support temperature. + // TODO: Add a `supportsTemperature` field to the model info. + if (modelId.startsWith("o1") || modelId.startsWith("o3-mini")) { + params.temperature = undefined + } + return { format, ...params, reasoning: getOpenAiReasoning({ model, reasoningBudget, reasoningEffort, settings }), } } else { + // Special case for o1-pro, which doesn't support temperature. + // Note that OpenRouter's `supported_parameters` field includes + // `temperature`, which is probably a bug. + // TODO: Add a `supportsTemperature` field to the model info and populate + // it appropriately in the OpenRouter fetcher. + if (modelId === "openai/o1-pro") { + params.temperature = undefined + } + return { format, ...params, diff --git a/src/vitest.config.ts b/src/vitest.config.ts index c20af0e0c1..42d471a0ce 100644 --- a/src/vitest.config.ts +++ b/src/vitest.config.ts @@ -1,5 +1,8 @@ import { defineConfig } from "vitest/config" export default defineConfig({ - test: { include: ["**/__tests__/**/*.spec.ts"] }, + test: { + include: ["**/__tests__/**/*.spec.ts"], + globals: true, + }, }) From efa6a56de1a357cc81004f01c136ba2c87dee463 Mon Sep 17 00:00:00 2001 From: cte Date: Fri, 23 May 2025 23:02:41 -0700 Subject: [PATCH 2/2] Add changeset --- .changeset/slow-spies-walk.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/slow-spies-walk.md diff --git a/.changeset/slow-spies-walk.md b/.changeset/slow-spies-walk.md new file mode 100644 index 0000000000..ca650ffcd0 --- /dev/null +++ b/.changeset/slow-spies-walk.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Fix o1-pro on OpenRouter