diff --git a/packages/cli/src/config/extensions/github.ts b/packages/cli/src/config/extensions/github.ts index 15b6529324..885529412d 100644 --- a/packages/cli/src/config/extensions/github.ts +++ b/packages/cli/src/config/extensions/github.ts @@ -560,6 +560,19 @@ export interface DownloadOptions { headers?: Record; } +function cleanupAfterFileClosed( + file: fs.WriteStream, + err: Error, + cleanupAndReject: (err: Error) => void, +): void { + if (file.closed) { + cleanupAndReject(err); + return; + } + file.once('close', () => cleanupAndReject(err)); + file.destroy(); +} + export async function downloadFile( url: string, dest: string, @@ -577,7 +590,14 @@ export async function downloadFile( } return new Promise((resolve, reject) => { + let settled = false; + const cleanupAndReject = (error: Error) => { + if (settled) { + return; + } + settled = true; + // Try to clean up the partial file, but don't mask the original error fs.unlink(dest, (unlinkErr) => { if (unlinkErr && unlinkErr.code !== 'ENOENT') { @@ -619,17 +639,21 @@ export async function downloadFile( // Handle file write errors file.on('error', (err) => { res.destroy(); - cleanupAndReject(err); + cleanupAfterFileClosed(file, err, cleanupAndReject); }); // Handle response stream errors res.on('error', (err) => { - file.destroy(); - cleanupAndReject(err); + cleanupAfterFileClosed(file, err, cleanupAndReject); }); res.pipe(file); - file.on('finish', () => file.close(resolve as () => void)); + file.on('finish', () => { + if (!settled) { + settled = true; + file.close(resolve as () => void); + } + }); }) .on('error', (err) => { cleanupAndReject(err); diff --git a/packages/cli/src/runtime/providerMutations.issue1943.test.ts b/packages/cli/src/runtime/providerMutations.issue1943.test.ts new file mode 100644 index 0000000000..3d5d1dcf82 --- /dev/null +++ b/packages/cli/src/runtime/providerMutations.issue1943.test.ts @@ -0,0 +1,105 @@ +/** + * @issue #1943 - /toolformat is not persisted into profile ephemerals + * + * Behavioral tests for setActiveToolFormatOverride() writing to both + * SettingsService AND Config ephemeral settings, so the tool-format value + * is captured during profile saves. + * + * Before the fix, only settingsService.updateSettings() was called. + * After the fix, config.setEphemeralSetting('tool-format', value) is also called, + * mirroring the pattern used by updateActiveProviderBaseUrl. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mockConfig = { + setEphemeralSetting: vi.fn(), + getEphemeralSetting: vi.fn().mockReturnValue(undefined), +}; + +const mockSettingsService = { + updateSettings: vi.fn().mockResolvedValue(undefined), + getProviderSettings: vi.fn().mockReturnValue({}), +}; + +const mockProviderManager = { + getActiveProvider: vi.fn().mockReturnValue({ name: 'openai' }), + getActiveProviderName: vi.fn().mockReturnValue('openai'), +}; + +vi.mock('./runtimeAccessors.js', () => ({ + getCliRuntimeServices: () => ({ + config: mockConfig, + settingsService: mockSettingsService, + providerManager: mockProviderManager, + }), + _internal: { + getActiveProviderOrThrow: () => ({ name: 'openai' }), + getProviderSettingsSnapshot: () => ({}), + }, +})); + +import { setActiveToolFormatOverride } from './providerMutations.js'; + +describe('setActiveToolFormatOverride ephemeral persistence (issue #1943)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockSettingsService.getProviderSettings.mockReturnValue({}); + }); + + it('writes "openai" to config.setEphemeralSetting when setting toolFormat to "openai"', async () => { + await setActiveToolFormatOverride('openai'); + + expect(mockSettingsService.updateSettings).toHaveBeenCalledWith('openai', { + toolFormat: 'openai', + }); + expect(mockConfig.setEphemeralSetting).toHaveBeenCalledWith( + 'tool-format', + 'openai', + ); + }); + + it('writes "auto" to config.setEphemeralSetting when clearing override', async () => { + await setActiveToolFormatOverride(null); + + expect(mockSettingsService.updateSettings).toHaveBeenCalledWith('openai', { + toolFormat: 'auto', + }); + expect(mockConfig.setEphemeralSetting).toHaveBeenCalledWith( + 'tool-format', + 'auto', + ); + }); + + it('writes "auto" to config.setEphemeralSetting when explicitly setting to "auto"', async () => { + await setActiveToolFormatOverride('auto'); + + expect(mockSettingsService.updateSettings).toHaveBeenCalledWith('openai', { + toolFormat: 'auto', + }); + expect(mockConfig.setEphemeralSetting).toHaveBeenCalledWith( + 'tool-format', + 'auto', + ); + }); + + it('writes "kimi" to config.setEphemeralSetting when setting toolFormat to "kimi"', async () => { + await setActiveToolFormatOverride('kimi'); + + expect(mockSettingsService.updateSettings).toHaveBeenCalledWith('openai', { + toolFormat: 'kimi', + }); + expect(mockConfig.setEphemeralSetting).toHaveBeenCalledWith( + 'tool-format', + 'kimi', + ); + }); + + it('calls both settingsService and config (mirrors updateActiveProviderBaseUrl pattern)', async () => { + await setActiveToolFormatOverride('openai'); + + // Both should be called exactly once + expect(mockSettingsService.updateSettings).toHaveBeenCalledTimes(1); + expect(mockConfig.setEphemeralSetting).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/cli/src/runtime/providerMutations.ts b/packages/cli/src/runtime/providerMutations.ts index 780cb0e91e..738563ebdb 100644 --- a/packages/cli/src/runtime/providerMutations.ts +++ b/packages/cli/src/runtime/providerMutations.ts @@ -376,17 +376,19 @@ export async function getActiveToolFormatState(): Promise { export async function setActiveToolFormatOverride( formatName: ToolFormatOverrideLiteral | null, ): Promise { - const { settingsService } = getCliRuntimeServices(); + const { config, settingsService } = getCliRuntimeServices(); const provider = _internal.getActiveProviderOrThrow(); if (!formatName || formatName === 'auto') { await settingsService.updateSettings(provider.name, { toolFormat: 'auto' }); + config.setEphemeralSetting('tool-format', 'auto'); return getActiveToolFormatState(); } await settingsService.updateSettings(provider.name, { toolFormat: formatName, }); + config.setEphemeralSetting('tool-format', formatName); return getActiveToolFormatState(); } diff --git a/packages/core/src/providers/openai-vercel/OpenAIVercelProvider.issue1943.test.ts b/packages/core/src/providers/openai-vercel/OpenAIVercelProvider.issue1943.test.ts new file mode 100644 index 0000000000..e5b3ca937d --- /dev/null +++ b/packages/core/src/providers/openai-vercel/OpenAIVercelProvider.issue1943.test.ts @@ -0,0 +1,228 @@ +/** + * @issue #1943 - OpenAI providers ignore explicit toolFormat override for kimi model names + * + * Behavioral tests for OpenAIVercelProvider.getToolFormat() and + * convertToModelMessages() honoring provider toolFormat overrides from + * SettingsService before falling back to model-name auto-detection. + * + * Also covers per-call resolved model (options.resolved.model) being used + * for tool format detection in generateChatCompletionWithOptions rather than + * the provider's default model. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { OpenAIVercelProvider } from './OpenAIVercelProvider.js'; +import { SettingsService } from '../../settings/SettingsService.js'; +import { + clearActiveProviderRuntimeContext, + setActiveProviderRuntimeContext, + createProviderRuntimeContext, +} from '../../runtime/providerRuntimeContext.js'; +import type { ProviderRuntimeContext } from '../../runtime/providerRuntimeContext.js'; +import type { Config } from '../../config/config.js'; +import { resolveToolFormat } from '../utils/toolFormatDetection.js'; + +function createTestConfig(settingsService: SettingsService): Config { + const noop = () => {}; + return { + getSettingsService: () => settingsService, + getEphemeralSettings: () => ({}), + getEphemeralSetting: () => undefined, + setEphemeralSetting: noop, + getModel: () => 'gpt-4o', + setModel: noop, + getConversationLoggingEnabled: () => false, + setConversationLoggingEnabled: noop, + getTelemetryLogPromptsEnabled: () => false, + setTelemetryLogPromptsEnabled: noop, + getUsageStatisticsEnabled: () => false, + setUsageStatisticsEnabled: noop, + getDebugMode: () => false, + setDebugMode: noop, + isInteractive: () => false, + getSessionId: () => 'test-session', + setSessionId: noop, + getFlashFallbackMode: () => 'off', + setFlashFallbackMode: noop, + getProvider: () => 'openaivercel', + setProvider: noop, + getProviderSettings: () => ({}), + setProviderSettings: noop, + getProviderConfig: () => ({}), + setProviderConfig: noop, + resetProvider: noop, + resetProviderSettings: noop, + resetProviderConfig: noop, + getActiveWorkspace: () => undefined, + setActiveWorkspace: noop, + clearActiveWorkspace: noop, + getExtensionConfig: () => ({}), + setExtensionConfig: noop, + getFeatures: () => ({}), + setFeatures: noop, + setProviderManager: noop, + getProviderManager: () => undefined, + getProviderSetting: () => undefined, + getUserMemory: () => '', + getJitMemoryForPath: () => Promise.resolve(''), + setUserMemory: noop, + getQuotaErrorOccurred: () => false, + setQuotaErrorOccurred: noop, + getLlxprtMdFilePaths: () => [], + getLlxprtMdFileCount: () => 0, + getCoreMemoryFileCount: () => 0, + } as unknown as Config; +} + +describe('OpenAIVercelProvider.getToolFormat() - override vs auto-detection (issue #1943)', () => { + let settingsService: SettingsService; + let runtime: ProviderRuntimeContext; + + beforeEach(() => { + settingsService = new SettingsService(); + const config = createTestConfig(settingsService); + runtime = createProviderRuntimeContext({ + settingsService, + config, + runtimeId: 'test.openaivercel.runtime', + metadata: { source: 'test' }, + }); + setActiveProviderRuntimeContext(runtime); + }); + + afterEach(() => { + clearActiveProviderRuntimeContext(); + }); + + it('returns "openai" override for a kimi model when provider toolFormat is set to "openai"', () => { + settingsService.setProviderSetting('openaivercel', 'toolFormat', 'openai'); + const provider = new OpenAIVercelProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('moonshot-v1-kimi-k2'); + + expect(provider.getToolFormat()).toBe('openai'); + }); + + it('auto-detects "kimi" format when provider toolFormat is "auto"', () => { + settingsService.setProviderSetting('openaivercel', 'toolFormat', 'auto'); + const provider = new OpenAIVercelProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('moonshot-v1-kimi-k2'); + + expect(provider.getToolFormat()).toBe('kimi'); + }); + + it('auto-detects "kimi" format when no toolFormat override is set', () => { + const provider = new OpenAIVercelProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('moonshot-v1-kimi-k2'); + + expect(provider.getToolFormat()).toBe('kimi'); + }); + + it('returns explicit "kimi" override even for a standard model', () => { + settingsService.setProviderSetting('openaivercel', 'toolFormat', 'kimi'); + const provider = new OpenAIVercelProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('gpt-4o'); + + expect(provider.getToolFormat()).toBe('kimi'); + }); + + it('returns "openai" by default for standard models', () => { + const provider = new OpenAIVercelProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('gpt-4o'); + + expect(provider.getToolFormat()).toBe('openai'); + }); + + it('auto-detects "mistral" format for mistral model when no override', () => { + const provider = new OpenAIVercelProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('mistral-large-latest'); + + expect(provider.getToolFormat()).toBe('mistral'); + }); + + it('returns "openai" for GLM models when override is set to "openai"', () => { + settingsService.setProviderSetting('openaivercel', 'toolFormat', 'openai'); + const provider = new OpenAIVercelProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('openai:hf:zai-org/GLM-4.6'); + + expect(provider.getToolFormat()).toBe('openai'); + }); +}); + +describe('Per-call resolved model vs provider model (issue #1943 finding 1)', () => { + /** + * These tests verify that when options.resolved.model differs from the + * provider's getModel()/default, the resolved model is used for tool + * format detection in convertToModelMessages. + * + * Since convertToModelMessages is private, we test via resolveToolFormat + * with the resolved model to confirm the format changes correctly. + */ + + it('auto-detects kimi format when resolved model is kimi even if provider default is gpt-4o', () => { + const settingsService = new SettingsService(); + const format = resolveToolFormat( + 'moonshot-v1-kimi-k2', // resolved model + 'openaivercel', + settingsService, + ); + expect(format).toBe('kimi'); + }); + + it('auto-detects openai format when resolved model is gpt-4o even if provider default is kimi', () => { + const settingsService = new SettingsService(); + const format = resolveToolFormat( + 'gpt-4o', // resolved model + 'openaivercel', + settingsService, + ); + expect(format).toBe('openai'); + }); + + it('uses explicit openai override to suppress kimi auto-detection for resolved kimi model', () => { + const settingsService = new SettingsService(); + settingsService.setProviderSetting('openaivercel', 'toolFormat', 'openai'); + const format = resolveToolFormat( + 'moonshot-v1-kimi-k2', // resolved model is kimi + 'openaivercel', + settingsService, + ); + expect(format).toBe('openai'); // override suppresses auto-detection + }); + + it('uses explicit openai override to suppress mistral auto-detection for resolved mistral model', () => { + const settingsService = new SettingsService(); + settingsService.setProviderSetting('openaivercel', 'toolFormat', 'openai'); + const format = resolveToolFormat( + 'mistral-large-latest', // resolved model is mistral + 'openaivercel', + settingsService, + ); + expect(format).toBe('openai'); // override suppresses auto-detection + }); + + it('detects different format when resolved model changes mid-session from kimi to gpt', () => { + const settingsService = new SettingsService(); + // First call with kimi model + expect( + resolveToolFormat('moonshot-v1-kimi-k2', 'openaivercel', settingsService), + ).toBe('kimi'); + // Second call with gpt model (different resolved.model) + expect(resolveToolFormat('gpt-4o', 'openaivercel', settingsService)).toBe( + 'openai', + ); + }); + + it('detects different format when resolved model changes mid-session from gpt to mistral', () => { + const settingsService = new SettingsService(); + expect(resolveToolFormat('gpt-4o', 'openaivercel', settingsService)).toBe( + 'openai', + ); + expect( + resolveToolFormat( + 'mistral-large-latest', + 'openaivercel', + settingsService, + ), + ).toBe('mistral'); + }); +}); diff --git a/packages/core/src/providers/openai-vercel/OpenAIVercelProvider.ts b/packages/core/src/providers/openai-vercel/OpenAIVercelProvider.ts index 751f23ccbc..f0dc53e205 100644 --- a/packages/core/src/providers/openai-vercel/OpenAIVercelProvider.ts +++ b/packages/core/src/providers/openai-vercel/OpenAIVercelProvider.ts @@ -73,7 +73,7 @@ import { import { extractCacheMetrics } from '../utils/cacheMetricsExtractor.js'; import { extractThinkTagsAsBlock } from '../utils/thinkingExtraction.js'; import { sanitizeProviderText } from '../utils/textSanitizer.js'; -import { detectToolFormat } from '../utils/toolFormatDetection.js'; +import { resolveToolFormat } from '../utils/toolFormatDetection.js'; import { getContentPreview } from '../utils/contentPreview.js'; import { isQwenBaseURL } from '../utils/qwenEndpoint.js'; import { shouldRetryOnStatus } from '../utils/retryStrategy.js'; @@ -502,10 +502,18 @@ export class OpenAIVercelProvider extends BaseProvider implements IProvider { */ private convertToModelMessages( contents: IContent[], - options?: { includeReasoningInContext?: boolean }, + options?: { includeReasoningInContext?: boolean; resolvedModel?: string }, ): ModelMessage[] { - const toolFormat = detectToolFormat( - this.getModel() || this.getDefaultModel(), + const settings = this.resolveSettingsService(); + // Use per-call resolved model (options.resolved.model) when available, + // falling back to provider model/default. This ensures tool format + // detection matches the actual model being used for the request. + const modelName = + options?.resolvedModel ?? (this.getModel() || this.getDefaultModel()); + const toolFormat = resolveToolFormat( + modelName, + this.name, + settings, this.getLogger(), ); @@ -1680,7 +1688,10 @@ export class OpenAIVercelProvider extends BaseProvider implements IProvider { const filteredContents = filterThinkingForContext(contents, stripPolicy); const messages: ModelMessage[] = this.convertToModelMessages( filteredContents, - { includeReasoningInContext: rs.includeInContext }, + { + includeReasoningInContext: rs.includeInContext, + resolvedModel: modelId, + }, ); const formattedTools = convertToolsToOpenAIVercel(tools); @@ -1906,8 +1917,9 @@ export class OpenAIVercelProvider extends BaseProvider implements IProvider { override getToolFormat(): string { const modelName = this.getModel() || this.getDefaultModel(); + const settings = this.resolveSettingsService(); const logger = new DebugLogger('llxprt:provider:openaivercel'); - const format = detectToolFormat(modelName, logger); + const format = resolveToolFormat(modelName, this.name, settings, logger); logger.debug(() => `getToolFormat() called, returning: ${format}`, { provider: this.name, model: modelName, diff --git a/packages/core/src/providers/openai/OpenAIProvider.issue1943.test.ts b/packages/core/src/providers/openai/OpenAIProvider.issue1943.test.ts new file mode 100644 index 0000000000..2ca9709d35 --- /dev/null +++ b/packages/core/src/providers/openai/OpenAIProvider.issue1943.test.ts @@ -0,0 +1,156 @@ +/** + * @issue #1943 - OpenAI providers ignore explicit toolFormat override for kimi model names + * + * Behavioral tests for OpenAIProvider.getToolFormat() honoring provider + * toolFormat overrides from SettingsService before falling back to model-name + * auto-detection. + * + * These tests verify that: + * 1. When a toolFormat override is set (e.g. 'openai'), it is used even for kimi models + * 2. When override is 'auto' or absent, auto-detection based on model name kicks in + * 3. The provider name is correctly used for override lookup + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { OpenAIProvider } from './OpenAIProvider.js'; +import { SettingsService } from '../../settings/SettingsService.js'; +import { + clearActiveProviderRuntimeContext, + setActiveProviderRuntimeContext, + createProviderRuntimeContext, +} from '../../runtime/providerRuntimeContext.js'; +import type { ProviderRuntimeContext } from '../../runtime/providerRuntimeContext.js'; +import type { Config } from '../../config/config.js'; + +function createTestConfig(settingsService: SettingsService): Config { + const noop = () => {}; + return { + getSettingsService: () => settingsService, + getEphemeralSettings: () => ({}), + getEphemeralSetting: () => undefined, + setEphemeralSetting: noop, + getModel: () => 'gpt-4o', + setModel: noop, + getConversationLoggingEnabled: () => false, + setConversationLoggingEnabled: noop, + getTelemetryLogPromptsEnabled: () => false, + setTelemetryLogPromptsEnabled: noop, + getUsageStatisticsEnabled: () => false, + setUsageStatisticsEnabled: noop, + getDebugMode: () => false, + setDebugMode: noop, + isInteractive: () => false, + getSessionId: () => 'test-session', + setSessionId: noop, + getFlashFallbackMode: () => 'off', + setFlashFallbackMode: noop, + getProvider: () => 'openai', + setProvider: noop, + getProviderSettings: () => ({}), + setProviderSettings: noop, + getProviderConfig: () => ({}), + setProviderConfig: noop, + resetProvider: noop, + resetProviderSettings: noop, + resetProviderConfig: noop, + getActiveWorkspace: () => undefined, + setActiveWorkspace: noop, + clearActiveWorkspace: noop, + getExtensionConfig: () => ({}), + setExtensionConfig: noop, + getFeatures: () => ({}), + setFeatures: noop, + setProviderManager: noop, + getProviderManager: () => undefined, + getProviderSetting: () => undefined, + getUserMemory: () => '', + getJitMemoryForPath: () => Promise.resolve(''), + setUserMemory: noop, + getQuotaErrorOccurred: () => false, + setQuotaErrorOccurred: noop, + getLlxprtMdFilePaths: () => [], + getLlxprtMdFileCount: () => 0, + getCoreMemoryFileCount: () => 0, + } as unknown as Config; +} + +describe('OpenAIProvider.getToolFormat() - override vs auto-detection (issue #1943)', () => { + let settingsService: SettingsService; + let runtime: ProviderRuntimeContext; + + beforeEach(() => { + settingsService = new SettingsService(); + const config = createTestConfig(settingsService); + runtime = createProviderRuntimeContext({ + settingsService, + config, + runtimeId: 'test.openai.runtime', + metadata: { source: 'test' }, + }); + setActiveProviderRuntimeContext(runtime); + }); + + afterEach(() => { + clearActiveProviderRuntimeContext(); + }); + + it('returns "openai" override for a kimi model when provider toolFormat is set to "openai"', () => { + settingsService.setProviderSetting('openai', 'toolFormat', 'openai'); + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('moonshot-v1-kimi-k2'); + + expect(provider.getToolFormat()).toBe('openai'); + }); + + it('auto-detects "kimi" format when provider toolFormat is "auto"', () => { + settingsService.setProviderSetting('openai', 'toolFormat', 'auto'); + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('moonshot-v1-kimi-k2'); + + expect(provider.getToolFormat()).toBe('kimi'); + }); + + it('auto-detects "kimi" format when no toolFormat override is set', () => { + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('moonshot-v1-kimi-k2'); + + expect(provider.getToolFormat()).toBe('kimi'); + }); + + it('auto-detects "mistral" format for mistral model when no override is set', () => { + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('mistral-large-latest'); + + expect(provider.getToolFormat()).toBe('mistral'); + }); + + it('returns explicit "kimi" override even for a standard openai model', () => { + settingsService.setProviderSetting('openai', 'toolFormat', 'kimi'); + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('gpt-4o'); + + expect(provider.getToolFormat()).toBe('kimi'); + }); + + it('returns "openai" by default for standard models', () => { + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('gpt-4o'); + + expect(provider.getToolFormat()).toBe('openai'); + }); + + it('returns "qwen" for GLM models by auto-detection', () => { + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('openai:hf:zai-org/GLM-4.6'); + + expect(provider.getToolFormat()).toBe('qwen'); + }); + + it('returns "openai" for GLM models when override is set to "openai"', () => { + settingsService.setProviderSetting('openai', 'toolFormat', 'openai'); + const provider = new OpenAIProvider('test-key'); + vi.spyOn(provider, 'getModel').mockReturnValue('openai:hf:zai-org/GLM-4.6'); + + expect(provider.getToolFormat()).toBe('openai'); + }); +}); diff --git a/packages/core/src/providers/openai/OpenAIProvider.ts b/packages/core/src/providers/openai/OpenAIProvider.ts index 8eb553a320..a7520ab02e 100644 --- a/packages/core/src/providers/openai/OpenAIProvider.ts +++ b/packages/core/src/providers/openai/OpenAIProvider.ts @@ -46,7 +46,7 @@ import { ToolCallPipeline } from './ToolCallPipeline.js'; import { isLocalEndpoint } from '../utils/localEndpoint.js'; import { type DumpMode } from '../utils/dumpContext.js'; -import { detectToolFormat } from '../utils/toolFormatDetection.js'; +import { resolveToolFormat } from '../utils/toolFormatDetection.js'; import { isQwenBaseURL } from '../utils/qwenEndpoint.js'; import { shouldRetryOnStatus } from '../utils/retryStrategy.js'; @@ -614,6 +614,7 @@ export class OpenAIProvider extends BaseProvider implements IProvider { this.getDefaultModel(), options.config, logger, + this.name, ); const { @@ -664,12 +665,16 @@ export class OpenAIProvider extends BaseProvider implements IProvider { /** * @plan:PLAN-20251023-STATELESS-HARDENING.P08 * @requirement:REQ-SP4-003 - * Returns the detected tool format for the current model + * Returns the detected tool format for the current model, + * honoring explicit provider toolFormat overrides from SettingsService. */ override getToolFormat(): string { const modelName = this.getModel() || this.getDefaultModel(); - const format = detectToolFormat( + const settings = this.resolveSettingsService(); + const format = resolveToolFormat( modelName, + this.name, + settings, new DebugLogger('llxprt:provider:openai'), ); const logger = new DebugLogger('llxprt:provider:openai'); diff --git a/packages/core/src/providers/openai/OpenAIRequestPreparation.issue1943.test.ts b/packages/core/src/providers/openai/OpenAIRequestPreparation.issue1943.test.ts new file mode 100644 index 0000000000..010784ac19 --- /dev/null +++ b/packages/core/src/providers/openai/OpenAIRequestPreparation.issue1943.test.ts @@ -0,0 +1,298 @@ +/** + * @issue #1943 - Request/message path coverage for OpenAIRequestPreparation + * + * Behavioral tests for prepareRequest() verifying that tool format detection, + * message building, tool conversion, and request body construction all use + * the per-call resolved model (options.resolved.model) rather than relying + * on defaults, and that provider toolFormat overrides suppress auto-detection. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { prepareRequest } from './OpenAIRequestPreparation.js'; +import type { NormalizedGenerateChatOptions } from '../BaseProvider.js'; +import { DebugLogger } from '../../debug/index.js'; +import { SettingsService } from '../../settings/SettingsService.js'; + +vi.mock('../../core/prompts.js', () => ({ + getCoreSystemPromptAsync: vi.fn().mockResolvedValue('test system prompt'), +})); + +vi.mock('../../prompt-config/subagent-delegation.js', () => ({ + shouldIncludeSubagentDelegation: vi.fn().mockResolvedValue(false), +})); + +vi.mock('../utils/userMemory.js', () => ({ + resolveUserMemory: vi.fn().mockResolvedValue(''), +})); + +function createMockOptions( + overrides: Partial = {}, +): NormalizedGenerateChatOptions { + const settings = new SettingsService(); + return { + contents: [], + tools: undefined, + metadata: {}, + settings, + config: undefined, + invocation: { + requestId: 'test-request', + timestamp: Date.now(), + }, + resolved: { + model: 'gpt-4o', + authToken: { token: 'test-token', type: 'api-key' }, + }, + ...overrides, + } as unknown as NormalizedGenerateChatOptions; +} + +describe('OpenAIRequestPreparation.prepareRequest (issue #1943)', () => { + let logger: DebugLogger; + + beforeEach(() => { + logger = new DebugLogger('llxprt:provider:openai:test'); + }); + + it('uses options.resolved.model for tool format detection when resolved model differs from default', async () => { + const settings = new SettingsService(); + const options = createMockOptions({ + settings, + resolved: { + model: 'kimi-k2', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', // default model is gpt-4o + undefined, + logger, + 'openai', + ); + + // Should use kimi-k2 (resolved model) not gpt-4o (default) + expect(result.model).toBe('kimi-k2'); + expect(result.detectedFormat).toBe('kimi'); + }); + + it('uses options.resolved.model for mistral model detection', async () => { + const settings = new SettingsService(); + const options = createMockOptions({ + settings, + resolved: { + model: 'mistral-large-latest', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.model).toBe('mistral-large-latest'); + expect(result.detectedFormat).toBe('mistral'); + }); + + it('falls back to defaultModel when options.resolved.model is empty', async () => { + const settings = new SettingsService(); + const options = createMockOptions({ + settings, + resolved: { + model: '', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.model).toBe('gpt-4o'); + expect(result.detectedFormat).toBe('openai'); + }); + + it('applies explicit openai override to suppress kimi auto-detection for resolved kimi model', async () => { + const settings = new SettingsService(); + settings.setProviderSetting('openai', 'toolFormat', 'openai'); + const options = createMockOptions({ + settings, + resolved: { + model: 'moonshot-v1-kimi-k2', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.model).toBe('moonshot-v1-kimi-k2'); + expect(result.detectedFormat).toBe('openai'); // override suppresses kimi auto-detect + }); + + it('applies explicit openai override to suppress mistral auto-detection for resolved mistral model', async () => { + const settings = new SettingsService(); + settings.setProviderSetting('openai', 'toolFormat', 'openai'); + const options = createMockOptions({ + settings, + resolved: { + model: 'mistral-large-latest', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.model).toBe('mistral-large-latest'); + expect(result.detectedFormat).toBe('openai'); // override suppresses mistral auto-detect + }); + + it('ignores invalid toolFormat override and auto-detects for kimi model', async () => { + const settings = new SettingsService(); + settings.setProviderSetting('openai', 'toolFormat', 'bogus-format'); + const options = createMockOptions({ + settings, + resolved: { + model: 'kimi-k2', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.detectedFormat).toBe('kimi'); // invalid override ignored, auto-detected + }); + + it('includes model in request body matching resolved model', async () => { + const settings = new SettingsService(); + const options = createMockOptions({ + settings, + resolved: { + model: 'gpt-4-turbo', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.requestBody.model).toBe('gpt-4-turbo'); + }); + + it('includes system prompt message in messagesWithSystem', async () => { + const settings = new SettingsService(); + const options = createMockOptions({ + settings, + resolved: { + model: 'gpt-4o', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.messagesWithSystem.length).toBeGreaterThan(0); + expect(result.messagesWithSystem[0].role).toBe('system'); + }); + + it('detects qwen format for GLM-4 resolved model', async () => { + const settings = new SettingsService(); + const options = createMockOptions({ + settings, + resolved: { + model: 'glm-4-plus', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.detectedFormat).toBe('qwen'); + }); + + it('detects deepseek format for deepseek-reasoner resolved model', async () => { + const settings = new SettingsService(); + const options = createMockOptions({ + settings, + resolved: { + model: 'deepseek-reasoner', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.detectedFormat).toBe('deepseek'); + }); + + it('uses "auto" toolFormat override to allow auto-detection for kimi model', async () => { + const settings = new SettingsService(); + settings.setProviderSetting('openai', 'toolFormat', 'auto'); + const options = createMockOptions({ + settings, + resolved: { + model: 'kimi-k2', + authToken: { token: 'test-token', type: 'api-key' }, + }, + }); + + const result = await prepareRequest( + options, + 'gpt-4o', + undefined, + logger, + 'openai', + ); + + expect(result.detectedFormat).toBe('kimi'); // auto overrides → auto-detect + }); +}); diff --git a/packages/core/src/providers/openai/OpenAIRequestPreparation.ts b/packages/core/src/providers/openai/OpenAIRequestPreparation.ts index b7dc2d6c87..2b14e3de8b 100644 --- a/packages/core/src/providers/openai/OpenAIRequestPreparation.ts +++ b/packages/core/src/providers/openai/OpenAIRequestPreparation.ts @@ -23,7 +23,7 @@ import { convertToolsToOpenAI, type OpenAITool } from './schemaConverter.js'; import { getCoreSystemPromptAsync } from '../../core/prompts.js'; import { shouldIncludeSubagentDelegation } from '../../prompt-config/subagent-delegation.js'; import { resolveUserMemory } from '../utils/userMemory.js'; -import { detectToolFormat } from '../utils/toolFormatDetection.js'; +import { resolveToolFormat } from '../utils/toolFormatDetection.js'; import { buildMessagesWithReasoning } from './OpenAIRequestBuilder.js'; import { extractModelParamsFromOptions } from './OpenAIClientFactory.js'; import { type Config } from '../../config/config.js'; @@ -189,6 +189,7 @@ export async function prepareRequest( defaultModel: string, config: Config | undefined, logger: DebugLogger, + providerName?: string, ): Promise { const { contents, tools, metadata } = options; const model = options.resolved.model || defaultModel; @@ -196,7 +197,15 @@ export async function prepareRequest( const ephemeralSettings = options.invocation?.ephemerals ?? {}; // Detect the tool format to use BEFORE building messages - const detectedFormat = detectToolFormat(model, logger); + // Check for provider toolFormat override before auto-detecting + const settings = options.settings; + const resolvedProviderName = providerName ?? 'openai'; + const detectedFormat = resolveToolFormat( + model, + resolvedProviderName, + settings, + logger, + ); logger.debug( () => @@ -204,7 +213,7 @@ export async function prepareRequest( { model, detectedFormat, - provider: 'openai', + provider: resolvedProviderName, }, ); diff --git a/packages/core/src/providers/utils/toolFormatDetection.issue1943.test.ts b/packages/core/src/providers/utils/toolFormatDetection.issue1943.test.ts new file mode 100644 index 0000000000..91170ca3e0 --- /dev/null +++ b/packages/core/src/providers/utils/toolFormatDetection.issue1943.test.ts @@ -0,0 +1,278 @@ +/** + * @issue #1943 - OpenAI providers ignore explicit toolFormat override for kimi model names + * + * Behavioral tests for resolveToolFormat() and getToolFormatOverride() confirming + * that explicit provider-level toolFormat overrides take precedence over model-name + * auto-detection. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { + resolveToolFormat, + getToolFormatOverride, + VALID_TOOL_FORMATS, + type ToolFormatSettings, +} from './toolFormatDetection.js'; +import { detectToolFormat } from './toolFormatDetection.js'; + +function createMockSettings( + providerSettings: Record> = {}, +): ToolFormatSettings { + return { + getProviderSettings: (providerName: string) => + providerSettings[providerName], + }; +} + +describe('resolveToolFormat (issue #1943)', () => { + it('uses explicit "openai" override instead of auto-detecting "kimi" for kimi model names', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openai' }, + }); + const format = resolveToolFormat('moonshot-v1-kimi-k2', 'openai', settings); + expect(format).toBe('openai'); + }); + + it('uses explicit "openai" override instead of auto-detecting "mistral" for mistral model names', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openai' }, + }); + const format = resolveToolFormat( + 'mistral-large-latest', + 'openai', + settings, + ); + expect(format).toBe('openai'); + }); + + it('uses explicit "openai" override instead of auto-detecting "qwen" for GLM models', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openai' }, + }); + const format = resolveToolFormat('glm-4', 'openai', settings); + expect(format).toBe('openai'); + }); + + it('auto-detects "kimi" when override is "auto"', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'auto' }, + }); + const format = resolveToolFormat('moonshot-v1-kimi-k2', 'openai', settings); + expect(format).toBe('kimi'); + }); + + it('auto-detects "openai" when no override is set', () => { + const settings = createMockSettings({}); + const format = resolveToolFormat('gpt-4o', 'openai', settings); + expect(format).toBe('openai'); + }); + + it('auto-detects "kimi" when no override is set for kimi model', () => { + const settings = createMockSettings({}); + const format = resolveToolFormat('kimi-k2', 'openai', settings); + expect(format).toBe('kimi'); + }); + + it('uses explicit "kimi" override even for a standard model that would auto-detect as "openai"', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'kimi' }, + }); + const format = resolveToolFormat('gpt-4o', 'openai', settings); + expect(format).toBe('kimi'); + }); + + it('isolates overrides per provider - openai override does not affect kimi provider lookup', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openai' }, + // kimi provider has no override + }); + // Even though openai has override=kimi, looking up 'kimi' provider finds nothing + const format = resolveToolFormat('moonshot-v1-kimi-k2', 'kimi', settings); + expect(format).toBe('kimi'); // auto-detected since kimi provider has no override + }); + + it('falls back to detectToolFormat when settings returns undefined for provider', () => { + const settings: ToolFormatSettings = { + getProviderSettings: () => undefined, + }; + const format = resolveToolFormat( + 'mistral-small-latest', + 'openai', + settings, + ); + expect(format).toBe('mistral'); + }); + + it('ignores non-string toolFormat values in provider settings', () => { + const settings = createMockSettings({ + openai: { toolFormat: 42 }, // invalid: not a string + }); + const format = resolveToolFormat('kimi-k2', 'openai', settings); + expect(format).toBe('kimi'); // falls back to auto-detect + }); + + it('ignores invalid toolFormat string and falls back to auto-detect', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'invalid-format' }, + }); + const format = resolveToolFormat('kimi-k2', 'openai', settings); + expect(format).toBe('kimi'); // ignored, auto-detected + }); + + it('ignores misspelled toolFormat and falls back to auto-detect', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openaai' }, // typo + }); + const format = resolveToolFormat('gpt-4o', 'openai', settings); + expect(format).toBe('openai'); // ignored, auto-detected + }); + + it('logs warning when invalid toolFormat override is ignored', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'bogus' }, + }); + const mockLogger = { + debug: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + enabled: true, + }; + resolveToolFormat( + 'gpt-4o', + 'openai', + settings, + mockLogger as unknown as import('../../debug/index.js').DebugLogger, + ); + expect(mockLogger.warn).toHaveBeenCalled(); + }); + + it('ignores undefined toolFormat in provider settings', () => { + const settings = createMockSettings({ + openai: { toolFormat: undefined }, + }); + const format = resolveToolFormat('kimi-k2', 'openai', settings); + expect(format).toBe('kimi'); + }); +}); + +describe('getToolFormatOverride (issue #1943)', () => { + it('returns the explicit override value when set', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openai' }, + }); + expect(getToolFormatOverride('openai', settings)).toBe('openai'); + }); + + it('returns "auto" when override is set to auto', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'auto' }, + }); + expect(getToolFormatOverride('openai', settings)).toBe('auto'); + }); + + it('returns undefined when no override is set', () => { + const settings = createMockSettings({}); + expect(getToolFormatOverride('openai', settings)).toBeUndefined(); + }); + + it('returns undefined when provider settings are undefined', () => { + const settings: ToolFormatSettings = { + getProviderSettings: () => undefined, + }; + expect(getToolFormatOverride('openai', settings)).toBeUndefined(); + }); + + it('returns undefined for non-string toolFormat values', () => { + const settings = createMockSettings({ + openai: { toolFormat: 42 }, + }); + expect(getToolFormatOverride('openai', settings)).toBeUndefined(); + }); + + it('returns undefined for invalid toolFormat string not in VALID_TOOL_FORMATS', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'invalid-format' }, + }); + expect(getToolFormatOverride('openai', settings)).toBeUndefined(); + }); + + it('returns undefined for misspelled toolFormat string', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openaai' }, + }); + expect(getToolFormatOverride('openai', settings)).toBeUndefined(); + }); + + it('logs warning when invalid toolFormat override is provided', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'bogus' }, + }); + const mockLogger = { + debug: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + enabled: true, + }; + getToolFormatOverride( + 'openai', + settings, + mockLogger as unknown as import('../../debug/index.js').DebugLogger, + ); + expect(mockLogger.warn).toHaveBeenCalled(); + }); + + it('does not log warning for valid toolFormat strings', () => { + const settings = createMockSettings({ + openai: { toolFormat: 'openai' }, + }); + const mockLogger = { + debug: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + enabled: true, + }; + getToolFormatOverride( + 'openai', + settings, + mockLogger as unknown as import('../../debug/index.js').DebugLogger, + ); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); +}); + +describe('detectToolFormat (existing auto-detection, unchanged)', () => { + it('detects kimi format for Kimi K2 models', () => { + expect(detectToolFormat('kimi-k2')).toBe('kimi'); + expect(detectToolFormat('moonshot-v1-kimi-k2')).toBe('kimi'); + }); + + it('defaults to openai format for standard models', () => { + expect(detectToolFormat('gpt-4o')).toBe('openai'); + }); + + it('defaults to openai format for unknown models', () => { + expect(detectToolFormat('some-custom-model')).toBe('openai'); + }); +}); + +describe('VALID_TOOL_FORMATS', () => { + it('contains all known ToolFormat literals plus auto', () => { + const expectedFormats = [ + 'openai', + 'anthropic', + 'deepseek', + 'qwen', + 'kimi', + 'mistral', + 'hermes', + 'xml', + 'llama', + 'gemma', + 'auto', + ]; + for (const fmt of expectedFormats) { + expect(VALID_TOOL_FORMATS.has(fmt)).toBe(true); + } + expect(VALID_TOOL_FORMATS.size).toBe(expectedFormats.length); + }); +}); diff --git a/packages/core/src/providers/utils/toolFormatDetection.ts b/packages/core/src/providers/utils/toolFormatDetection.ts index 099bc7fb6e..c584519e39 100644 --- a/packages/core/src/providers/utils/toolFormatDetection.ts +++ b/packages/core/src/providers/utils/toolFormatDetection.ts @@ -22,6 +22,96 @@ import { } from '../../tools/ToolIdStrategy.js'; import type { DebugLogger } from '../../debug/index.js'; +/** + * All valid ToolFormat literal values from the IToolFormatter type, plus 'auto' + * for disabling the override and using model-based auto-detection. + * + * When a toolFormat override in provider settings does not match one of these + * values, it is ignored and auto-detection is used instead. + */ +export const VALID_TOOL_FORMATS: ReadonlySet = new Set([ + 'openai', + 'anthropic', + 'deepseek', + 'qwen', + 'kimi', + 'mistral', + 'hermes', + 'xml', + 'llama', + 'gemma', + 'auto', +]); + +/** + * Minimal interface for settings objects that can provide provider-specific + * toolFormat overrides. Matches the subset of SettingsService used for + * tool format resolution, allowing tests to supply lightweight stubs. + */ +export interface ToolFormatSettings { + getProviderSettings( + providerName: string, + ): Record | undefined; +} + +/** + * Get the tool format override from provider settings. + * + * Returns the explicit toolFormat override value if one is set (e.g. 'openai', + * 'kimi'), 'auto' if set to auto-detect, or undefined if no override exists. + * Invalid strings (not in VALID_TOOL_FORMATS) are ignored with a logged + * warning, and the function returns undefined so auto-detection is used. + * Only non-'auto' values should bypass auto-detection. + */ +export function getToolFormatOverride( + providerName: string, + settings: ToolFormatSettings, + logger?: DebugLogger, +): ToolFormat | 'auto' | undefined { + const providerSettings = settings.getProviderSettings(providerName); + const toolFormatOverride = providerSettings?.toolFormat; + if (typeof toolFormatOverride !== 'string') { + return undefined; + } + if (!VALID_TOOL_FORMATS.has(toolFormatOverride)) { + logger?.warn( + () => + `Ignoring invalid toolFormat override '${toolFormatOverride}' for provider '${providerName}'. Valid values: ${[...VALID_TOOL_FORMATS].join(', ')}. Falling back to auto-detection.`, + ); + return undefined; + } + return toolFormatOverride as ToolFormat | 'auto'; +} + +/** + * Resolve the effective tool format for a provider, checking for explicit + * overrides in provider settings before falling back to model-based + * auto-detection. + * + * This is the OpenAI-family equivalent of AnthropicProvider.detectToolFormat(). + * The pattern: + * 1. If provider has an explicit toolFormat override (not 'auto'), use it. + * 2. Otherwise, auto-detect based on model name. + */ +export function resolveToolFormat( + modelName: string, + providerName: string, + settings: ToolFormatSettings, + logger?: DebugLogger, +): ToolFormat { + const override = getToolFormatOverride(providerName, settings, logger); + + if (override !== undefined && override !== 'auto') { + logger?.debug( + () => + `Using explicit tool format override '${override}' for provider '${providerName}', model '${modelName}'`, + ); + return override; + } + + return detectToolFormat(modelName, logger); +} + /** * Auto-detect the tool format based on model name. *