From 5328da1ca35b99108957dccc06b7a897b053fe30 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Tue, 25 Nov 2025 23:36:16 -0500 Subject: [PATCH] fix: update API handler when toolProtocol changes The original commit 8902facf4 added a forceRebuild option to rebuild the API handler when provider settings change, but it didn't consider that changing toolProtocol requires updating the assistantMessageParser in the Task (switching between XmlToolParser and NativeToolParser). Changes: - Add toolProtocol to the rebuild condition in updateTaskApiHandlerIfNeeded() - Call task.updateApiConfiguration() instead of directly setting task.api and task.apiConfiguration, since updateApiConfiguration properly handles parser synchronization - Change Task.updateApiConfiguration() from async to sync (no async operations) - Add test for toolProtocol changes triggering handler rebuild - Update test mocks to include updateApiConfiguration --- src/core/task/Task.ts | 6 +- src/core/webview/ClineProvider.ts | 26 +++++-- .../ClineProvider.apiHandlerRebuild.spec.ts | 78 ++++++++----------- .../ClineProvider.sticky-mode.spec.ts | 11 +++ 4 files changed, 65 insertions(+), 56 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index aaac412e6d8..fd908e89abd 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -550,7 +550,7 @@ export class Task extends EventEmitter implements TaskLike { try { const newState = await provider.getState() if (newState?.apiConfiguration) { - await this.updateApiConfiguration(newState.apiConfiguration) + this.updateApiConfiguration(newState.apiConfiguration) } } catch (error) { console.error( @@ -1167,7 +1167,7 @@ export class Task extends EventEmitter implements TaskLike { * * @param newApiConfiguration - The new API configuration to use */ - public async updateApiConfiguration(newApiConfiguration: ProviderSettings): Promise { + public updateApiConfiguration(newApiConfiguration: ProviderSettings): void { // Update the configuration and rebuild the API handler this.apiConfiguration = newApiConfiguration this.api = buildApiHandler(newApiConfiguration) @@ -1222,7 +1222,7 @@ export class Task extends EventEmitter implements TaskLike { // This ensures the parser state is synchronized with the selected model const newState = await provider.getState() if (newState?.apiConfiguration) { - await this.updateApiConfiguration(newState.apiConfiguration) + this.updateApiConfiguration(newState.apiConfiguration) } } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index a16de9b88a5..dbfaeb16bd8 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1322,16 +1322,28 @@ export class ClineProvider const prevConfig = task.apiConfiguration const prevProvider = prevConfig?.apiProvider const prevModelId = prevConfig ? getModelId(prevConfig) : undefined + const prevToolProtocol = prevConfig?.toolProtocol const newProvider = providerSettings.apiProvider const newModelId = getModelId(providerSettings) - - if (forceRebuild || prevProvider !== newProvider || prevModelId !== newModelId) { - task.api = buildApiHandler(providerSettings) + const newToolProtocol = providerSettings.toolProtocol + + const needsRebuild = + forceRebuild || + prevProvider !== newProvider || + prevModelId !== newModelId || + prevToolProtocol !== newToolProtocol + + if (needsRebuild) { + // Use updateApiConfiguration which handles both API handler rebuild and parser sync. + // This is important when toolProtocol changes - the assistantMessageParser needs to be + // created/destroyed to match the new protocol (XML vs native). + // Note: updateApiConfiguration is declared async but has no actual async operations, + // so we can safely call it without awaiting. + task.updateApiConfiguration(providerSettings) + } else { + // No rebuild needed, just sync apiConfiguration + ;(task as any).apiConfiguration = providerSettings } - - // Always sync the task's apiConfiguration with the latest provider settings. - // Note: Task.apiConfiguration is declared readonly in types, so we cast to any for runtime update. - ;(task as any).apiConfiguration = providerSettings } getProviderProfileEntries(): ProviderSettingsEntry[] { diff --git a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts index 1a86c06a6ed..3671019f5db 100644 --- a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts @@ -106,6 +106,9 @@ vi.mock("../../task/Task", () => ({ overwriteApiConversationHistory: vi.fn(), taskId: options?.historyItem?.id || "test-task-id", emit: vi.fn(), + updateApiConfiguration: vi.fn().mockImplementation(function (this: any, newConfig: any) { + this.apiConfiguration = newConfig + }), } // Define apiConfiguration as a property so tests can read it Object.defineProperty(mockTask, "apiConfiguration", { @@ -250,7 +253,7 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { }) describe("upsertProviderProfile", () => { - test("rebuilds API handler when provider/model unchanged but profile settings changed (explicit save)", async () => { + test("calls updateApiConfiguration when provider/model unchanged but profile settings changed (explicit save)", async () => { // Create a task with the current config const mockTask = new Task({ ...defaultTaskOptions, @@ -259,19 +262,15 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { openRouterModelId: "openai/gpt-4", }, }) - const originalApi = { + mockTask.api = { getModel: vi.fn().mockReturnValue({ id: "openai/gpt-4", info: { contextWindow: 128000 }, }), - } - mockTask.api = originalApi as any + } as any await provider.addClineToStack(mockTask) - // Clear the mock to track new calls - buildApiHandlerMock.mockClear() - // Save settings with SAME provider and model (simulating Save button click) await provider.upsertProviderProfile( "test-config", @@ -285,22 +284,22 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { true, ) - // Verify buildApiHandler WAS called because we force rebuild on explicit save/switch - expect(buildApiHandlerMock).toHaveBeenCalledWith( + // Verify updateApiConfiguration was called because we force rebuild on explicit save/switch + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( expect.objectContaining({ apiProvider: "openrouter", openRouterModelId: "openai/gpt-4", + rateLimitSeconds: 5, + modelTemperature: 0.7, }), ) - // Verify the task's api property was reassigned (new client) - expect(mockTask.api).not.toBe(originalApi) - // Verify task.apiConfiguration was synchronized with non-model fields + // Verify task.apiConfiguration was synchronized expect((mockTask as any).apiConfiguration.openRouterModelId).toBe("openai/gpt-4") expect((mockTask as any).apiConfiguration.rateLimitSeconds).toBe(5) expect((mockTask as any).apiConfiguration.modelTemperature).toBe(0.7) }) - test("rebuilds API handler when provider changes", async () => { + test("calls updateApiConfiguration when provider changes", async () => { const mockTask = new Task({ ...defaultTaskOptions, apiConfiguration: { @@ -317,8 +316,6 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.addClineToStack(mockTask) - buildApiHandlerMock.mockClear() - // Change provider to anthropic await provider.upsertProviderProfile( "test-config", @@ -329,8 +326,8 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { true, ) - // Verify buildApiHandler WAS called since provider changed - expect(buildApiHandlerMock).toHaveBeenCalledWith( + // Verify updateApiConfiguration was called since provider changed + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( expect.objectContaining({ apiProvider: "anthropic", apiModelId: "claude-3-5-sonnet-20241022", @@ -338,7 +335,7 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { ) }) - test("rebuilds API handler when model changes", async () => { + test("calls updateApiConfiguration when model changes", async () => { const mockTask = new Task({ ...defaultTaskOptions, apiConfiguration: { @@ -355,8 +352,6 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.addClineToStack(mockTask) - buildApiHandlerMock.mockClear() - // Change model to different model await provider.upsertProviderProfile( "test-config", @@ -367,8 +362,8 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { true, ) - // Verify buildApiHandler WAS called since model changed - expect(buildApiHandlerMock).toHaveBeenCalledWith( + // Verify updateApiConfiguration was called since model changed + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( expect.objectContaining({ apiProvider: "openrouter", openRouterModelId: "anthropic/claude-3-5-sonnet-20241022", @@ -395,7 +390,7 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { }) describe("activateProviderProfile", () => { - test("rebuilds API handler when provider/model unchanged but settings differ (explicit profile switch)", async () => { + test("calls updateApiConfiguration when provider/model unchanged but settings differ (explicit profile switch)", async () => { const mockTask = new Task({ ...defaultTaskOptions, apiConfiguration: { @@ -404,18 +399,15 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { modelTemperature: 0.3, }, }) - const originalApi = { + mockTask.api = { getModel: vi.fn().mockReturnValue({ id: "openai/gpt-4", info: { contextWindow: 128000 }, }), - } - mockTask.api = originalApi as any + } as any await provider.addClineToStack(mockTask) - buildApiHandlerMock.mockClear() - // Mock activateProfile to return same provider/model but different non-model setting ;(provider as any).providerSettingsManager.activateProfile = vi.fn().mockResolvedValue({ name: "test-config", @@ -428,22 +420,20 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.activateProviderProfile({ name: "test-config" }) - // Verify buildApiHandler WAS called due to forced rebuild on explicit switch - expect(buildApiHandlerMock).toHaveBeenCalledWith( + // Verify updateApiConfiguration was called due to forced rebuild on explicit switch + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( expect.objectContaining({ apiProvider: "openrouter", openRouterModelId: "openai/gpt-4", }), ) - // Verify the API reference changed - expect(mockTask.api).not.toBe(originalApi) // Verify task.apiConfiguration was synchronized expect((mockTask as any).apiConfiguration.openRouterModelId).toBe("openai/gpt-4") expect((mockTask as any).apiConfiguration.modelTemperature).toBe(0.9) expect((mockTask as any).apiConfiguration.rateLimitSeconds).toBe(7) }) - test("rebuilds API handler when provider changes and syncs task.apiConfiguration", async () => { + test("calls updateApiConfiguration when provider changes and syncs task.apiConfiguration", async () => { const mockTask = new Task({ ...defaultTaskOptions, apiConfiguration: { @@ -460,8 +450,6 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.addClineToStack(mockTask) - buildApiHandlerMock.mockClear() - // Mock activateProfile to return different provider ;(provider as any).providerSettingsManager.activateProfile = vi.fn().mockResolvedValue({ name: "anthropic-config", @@ -472,8 +460,8 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.activateProviderProfile({ name: "anthropic-config" }) - // Verify buildApiHandler WAS called - expect(buildApiHandlerMock).toHaveBeenCalledWith( + // Verify updateApiConfiguration was called + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( expect.objectContaining({ apiProvider: "anthropic", apiModelId: "claude-3-5-sonnet-20241022", @@ -484,7 +472,7 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { expect((mockTask as any).apiConfiguration.apiModelId).toBe("claude-3-5-sonnet-20241022") }) - test("rebuilds API handler when model changes and syncs task.apiConfiguration", async () => { + test("calls updateApiConfiguration when model changes and syncs task.apiConfiguration", async () => { const mockTask = new Task({ ...defaultTaskOptions, apiConfiguration: { @@ -501,8 +489,6 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.addClineToStack(mockTask) - buildApiHandlerMock.mockClear() - // Mock activateProfile to return different model ;(provider as any).providerSettingsManager.activateProfile = vi.fn().mockResolvedValue({ name: "test-config", @@ -513,8 +499,8 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.activateProviderProfile({ name: "test-config" }) - // Verify buildApiHandler WAS called - expect(buildApiHandlerMock).toHaveBeenCalledWith( + // Verify updateApiConfiguration was called + expect(mockTask.updateApiConfiguration).toHaveBeenCalledWith( expect.objectContaining({ apiProvider: "openrouter", openRouterModelId: "anthropic/claude-3-5-sonnet-20241022", @@ -545,7 +531,6 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { await provider.addClineToStack(mockTask) // First switch: A -> B (openrouter -> anthropic) - buildApiHandlerMock.mockClear() ;(provider as any).providerSettingsManager.activateProfile = vi.fn().mockResolvedValue({ name: "anthropic-config", id: "anthropic-id", @@ -554,12 +539,12 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { }) await provider.activateProviderProfile({ name: "anthropic-config" }) - expect(buildApiHandlerMock).toHaveBeenCalled() + expect(mockTask.updateApiConfiguration).toHaveBeenCalled() expect((mockTask as any).apiConfiguration.apiProvider).toBe("anthropic") expect((mockTask as any).apiConfiguration.apiModelId).toBe("claude-3-5-sonnet-20241022") // Second switch: B -> A (anthropic -> openrouter gpt-4) - buildApiHandlerMock.mockClear() + ;(mockTask.updateApiConfiguration as any).mockClear() ;(provider as any).providerSettingsManager.activateProfile = vi.fn().mockResolvedValue({ name: "test-config", id: "test-id", @@ -568,7 +553,8 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { }) await provider.activateProviderProfile({ name: "test-config" }) - // API handler may or may not rebuild depending on mock model id, but apiConfiguration must be updated + // updateApiConfiguration called again, and apiConfiguration must be updated + expect(mockTask.updateApiConfiguration).toHaveBeenCalled() expect((mockTask as any).apiConfiguration.apiProvider).toBe("openrouter") expect((mockTask as any).apiConfiguration.openRouterModelId).toBe("openai/gpt-4") }) diff --git a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts index 51a10d2bf7b..7ff767a9733 100644 --- a/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.sticky-mode.spec.ts @@ -73,6 +73,7 @@ vi.mock("../../task/Task", () => ({ setRootTask: vi.fn(), emit: vi.fn(), parentTask: options.parentTask, + updateApiConfiguration: vi.fn(), })), })) @@ -334,6 +335,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } // Add task to provider stack @@ -781,6 +783,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } // Add task to provider stack @@ -847,6 +850,7 @@ describe("ClineProvider - Sticky Mode", () => { }), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } // Add task to provider stack @@ -898,6 +902,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } // Add task to provider stack @@ -932,6 +937,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } // Add task to provider stack @@ -987,6 +993,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } // Add task to provider stack @@ -1033,6 +1040,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } const task2 = { @@ -1042,6 +1050,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } const task3 = { @@ -1051,6 +1060,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), } // Add tasks to provider stack @@ -1192,6 +1202,7 @@ describe("ClineProvider - Sticky Mode", () => { saveClineMessages: vi.fn(), clineMessages: [], apiConversationHistory: [], + updateApiConfiguration: vi.fn(), })) // Add all tasks to provider