From f379528e3b42a2439a24645875ce742c3b379423 Mon Sep 17 00:00:00 2001 From: Kaiyi Date: Thu, 16 Jul 2026 16:38:14 +0800 Subject: [PATCH] fix: honor explicit thinking off on OpenAI-compatible providers An explicit withThinking('off') collapsed to the same internal state as "never configured" on chat-completions providers, so the history-based auto reasoning_effort injection (#1616) silently switched reasoning back on and could leak the field to models that reject it. Store the requested effort verbatim and derive the wire encoding per request, suppress the auto-enable for an explicit 'off', and report the accurate current effort ('on'/'off') instead of recording 'off' for both. --- .changeset/fix-openai-legacy-thinking-off.md | 5 + .../llmProtocol/providers/openai-legacy.ts | 24 ++- .../providers/openai-legacy.test.ts | 144 ++++++++++++++++++ .../protocol/protocolAdapterRegistry.test.ts | 2 - .../kosong/src/providers/openai-legacy.ts | 32 ++-- packages/kosong/test/openai-legacy.test.ts | 83 ++++++++++ 6 files changed, 270 insertions(+), 20 deletions(-) create mode 100644 .changeset/fix-openai-legacy-thinking-off.md create mode 100644 packages/agent-core-v2/test/app/llmProtocol/providers/openai-legacy.test.ts diff --git a/.changeset/fix-openai-legacy-thinking-off.md b/.changeset/fix-openai-legacy-thinking-off.md new file mode 100644 index 0000000000..5b46bbf3f7 --- /dev/null +++ b/.changeset/fix-openai-legacy-thinking-off.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Honor an explicit thinking "off" on OpenAI-compatible (chat completions) providers: it used to be indistinguishable from "never configured", so the history-based auto `reasoning_effort` injection kept the model reasoning (and could leak the field to models that reject it). The provider now also reports the actual current thinking effort ("on"/"off") instead of recording "off" for both. diff --git a/packages/agent-core-v2/src/app/llmProtocol/providers/openai-legacy.ts b/packages/agent-core-v2/src/app/llmProtocol/providers/openai-legacy.ts index 421cc2f310..8f6b4b0341 100644 --- a/packages/agent-core-v2/src/app/llmProtocol/providers/openai-legacy.ts +++ b/packages/agent-core-v2/src/app/llmProtocol/providers/openai-legacy.ts @@ -433,7 +433,7 @@ export class OpenAILegacyChatProvider implements ChatProvider { private _baseUrl: string | undefined; private _defaultHeaders: Record | undefined; private _reasoningKey: string | undefined; - private _reasoningEffort: string | undefined; + private _thinkingEffort: ThinkingEffort | undefined; private _generationKwargs: OpenAILegacyGenerationKwargs; private _toolMessageConversion: ToolMessageConversion; private _client: OpenAI | undefined; @@ -452,7 +452,7 @@ export class OpenAILegacyChatProvider implements ChatProvider { normalizedReasoningKey !== undefined && normalizedReasoningKey.length > 0 ? normalizedReasoningKey : undefined; - this._reasoningEffort = undefined; + this._thinkingEffort = undefined; this._generationKwargs = options.maxTokens !== undefined ? completionTokenKwargs(this._model, options.maxTokens) : {}; this._toolMessageConversion = options.toolMessageConversion ?? null; @@ -467,8 +467,7 @@ export class OpenAILegacyChatProvider implements ChatProvider { } get thinkingEffort(): ThinkingEffort | null { - if (this._reasoningEffort === undefined) return null; - return this._reasoningEffort === 'none' ? 'off' : this._reasoningEffort; + return this._thinkingEffort ?? null; } get maxCompletionTokens(): number | undefined { @@ -506,9 +505,19 @@ export class OpenAILegacyChatProvider implements ChatProvider { this._generationKwargs, ); - let reasoningEffort: string | undefined = this._reasoningEffort; + // 'off' and 'on' have no wire encoding; only a concrete effort is passed + // through. An explicit 'off' must also suppress the history-based + // auto-enable below (issue #1616), so it stays distinguishable from + // "never configured". + const effort = this._thinkingEffort; + let reasoningEffort: string | undefined = + effort === undefined || effort === 'off' || effort === 'on' ? undefined : effort; - if (reasoningEffort === undefined && kwargs['reasoning_effort'] === undefined) { + if ( + reasoningEffort === undefined && + effort !== 'off' && + kwargs['reasoning_effort'] === undefined + ) { const hasThinkPart = history.some((message) => message.content.some((part) => part.type === 'think'), ); @@ -560,9 +569,8 @@ export class OpenAILegacyChatProvider implements ChatProvider { } withThinking(effort: ThinkingEffort): OpenAILegacyChatProvider { - const reasoningEffort = effort === 'off' || effort === 'on' ? undefined : effort; const clone = this._clone(); - clone._reasoningEffort = reasoningEffort; + clone._thinkingEffort = effort; return clone; } diff --git a/packages/agent-core-v2/test/app/llmProtocol/providers/openai-legacy.test.ts b/packages/agent-core-v2/test/app/llmProtocol/providers/openai-legacy.test.ts new file mode 100644 index 0000000000..e70b251745 --- /dev/null +++ b/packages/agent-core-v2/test/app/llmProtocol/providers/openai-legacy.test.ts @@ -0,0 +1,144 @@ +/** + * Scenario: OpenAI-compatible (chat completions) thinking effort encoding. + * Responsibilities: encode withThinking onto the wire, keep an explicit 'off' + * distinct from "never configured", and report the accurate effort. + * Wiring: real v2 OpenAILegacy adapter with only the remote SDK client boundary + * replaced by mocks. + * Run: pnpm exec vitest run packages/agent-core-v2/test/app/llmProtocol/providers/openai-legacy.test.ts + */ +import type { Message } from '#/app/llmProtocol/message'; +import { OpenAILegacyChatProvider } from '#/app/llmProtocol/providers/openai-legacy'; +import { describe, expect, it, vi } from 'vitest'; + +const USER_TURN: Message = { + role: 'user', + content: [{ type: 'text', text: 'Think' }], + toolCalls: [], +}; + +const THINK_HISTORY: Message[] = [ + { role: 'user', content: [{ type: 'text', text: 'Hello' }], toolCalls: [] }, + { + role: 'assistant', + content: [ + { type: 'think', think: 'Thinking...' }, + { type: 'text', text: 'Hi!' }, + ], + toolCalls: [], + }, + { role: 'user', content: [{ type: 'text', text: 'How are you?' }], toolCalls: [] }, +]; + +function createProvider(model = 'gpt-4.1'): OpenAILegacyChatProvider { + return new OpenAILegacyChatProvider({ + model, + apiKey: 'test-key', + stream: false, + }); +} + +function makeChatCompletionResponse(model = 'test-model') { + return { + id: 'chatcmpl-test123', + object: 'chat.completion', + created: 1234567890, + model, + choices: [ + { + index: 0, + message: { role: 'assistant', content: 'Hello' }, + finish_reason: 'stop', + }, + ], + usage: { prompt_tokens: 10, completion_tokens: 5, total_tokens: 15 }, + }; +} + +async function captureRequestBody( + provider: OpenAILegacyChatProvider, + history: Message[], +): Promise> { + let capturedBody: Record | undefined; + ( + provider as unknown as { _client: { chat: { completions: { create: unknown } } } } + )._client.chat.completions.create = vi.fn().mockImplementation((params: unknown) => { + capturedBody = params as Record; + return Promise.resolve(makeChatCompletionResponse()); + }); + + const stream = await provider.generate('', [], history); + for await (const part of stream) void part; + + if (capturedBody === undefined) { + throw new Error('Expected provider.generate() to call chat.completions.create'); + } + return capturedBody; +} + +describe('OpenAILegacyChatProvider withThinking', () => { + it('passes a concrete effort through verbatim and reports it', async () => { + const provider = createProvider().withThinking('high'); + + const body = await captureRequestBody(provider, [USER_TURN]); + expect(body['reasoning_effort']).toBe('high'); + expect(provider.thinkingEffort).toBe('high'); + }); + + it('reports a null thinkingEffort until withThinking is called', () => { + expect(createProvider().thinkingEffort).toBeNull(); + }); + + it('sends no reasoning_effort for "on" without ThinkPart history and reports "on"', async () => { + const provider = createProvider().withThinking('on'); + + const body = await captureRequestBody(provider, [USER_TURN]); + expect(body['reasoning_effort']).toBeUndefined(); + expect(provider.thinkingEffort).toBe('on'); + }); + + it('sends no reasoning_effort for "off" and reports "off"', async () => { + const provider = createProvider().withThinking('off'); + + const body = await captureRequestBody(provider, [USER_TURN]); + expect(body['reasoning_effort']).toBeUndefined(); + expect(provider.thinkingEffort).toBe('off'); + }); + + it('clears a concrete effort set earlier when turned off', async () => { + const provider = createProvider().withThinking('high').withThinking('off'); + expect(provider.thinkingEffort).toBe('off'); + + const body = await captureRequestBody(provider, [USER_TURN]); + expect(body['reasoning_effort']).toBeUndefined(); + }); + + it('auto-injects reasoning_effort when ThinkPart history exists and thinking is unconfigured', async () => { + // Issue #1616: strict OpenAI-compatible gateways require a paired + // reasoning_effort when the history carries reasoning_content. + const body = await captureRequestBody(createProvider(), THINK_HISTORY); + expect(body['reasoning_effort']).toBe('medium'); + }); + + it('still auto-injects reasoning_effort for an explicit "on"', async () => { + const body = await captureRequestBody(createProvider().withThinking('on'), THINK_HISTORY); + expect(body['reasoning_effort']).toBe('medium'); + }); + + it('does not auto-inject reasoning_effort when thinking was explicitly turned off', async () => { + // An explicit withThinking('off') is not "never configured": with thinking + // off, the auto-enable must not switch reasoning back on (or leak the field + // to models that reject it). + const provider = createProvider().withThinking('off'); + + const body = await captureRequestBody(provider, THINK_HISTORY); + expect(body['reasoning_effort']).toBeUndefined(); + expect(provider.thinkingEffort).toBe('off'); + }); + + it('does not overwrite reasoning_effort pinned via withGenerationKwargs', async () => { + const provider = createProvider().withGenerationKwargs({ reasoning_effort: 'high' }); + + const body = await captureRequestBody(provider, THINK_HISTORY); + expect(body['reasoning_effort']).toBe('high'); + }); +}); diff --git a/packages/agent-core-v2/test/app/protocol/protocolAdapterRegistry.test.ts b/packages/agent-core-v2/test/app/protocol/protocolAdapterRegistry.test.ts index 64ba732382..6865bacb85 100644 --- a/packages/agent-core-v2/test/app/protocol/protocolAdapterRegistry.test.ts +++ b/packages/agent-core-v2/test/app/protocol/protocolAdapterRegistry.test.ts @@ -92,9 +92,7 @@ describe('ProtocolAdapterRegistry', () => { apiKey: 'sk', }); - expect(Reflect.get(provider.withThinking('max'), '_reasoningEffort')).toBe('max'); expect(provider.withThinking('max').thinkingEffort).toBe('max'); - expect(Reflect.get(provider.withThinking('medium'), '_reasoningEffort')).toBe('medium'); expect(provider.withThinking('medium').thinkingEffort).toBe('medium'); }); diff --git a/packages/kosong/src/providers/openai-legacy.ts b/packages/kosong/src/providers/openai-legacy.ts index 98e109e891..02c5e5f1af 100644 --- a/packages/kosong/src/providers/openai-legacy.ts +++ b/packages/kosong/src/providers/openai-legacy.ts @@ -482,7 +482,7 @@ export class OpenAILegacyChatProvider implements ChatProvider { private _baseUrl: string | undefined; private _defaultHeaders: Record | undefined; private _reasoningKey: string | undefined; - private _reasoningEffort: string | undefined; + private _thinkingEffort: ThinkingEffort | undefined; private _generationKwargs: OpenAILegacyGenerationKwargs; private _toolMessageConversion: ToolMessageConversion; private _client: OpenAI | undefined; @@ -505,7 +505,7 @@ export class OpenAILegacyChatProvider implements ChatProvider { normalizedReasoningKey !== undefined && normalizedReasoningKey.length > 0 ? normalizedReasoningKey : undefined; - this._reasoningEffort = undefined; + this._thinkingEffort = undefined; this._generationKwargs = options.maxTokens !== undefined ? completionTokenKwargs(this._model, options.maxTokens) : {}; this._toolMessageConversion = options.toolMessageConversion ?? null; @@ -520,8 +520,7 @@ export class OpenAILegacyChatProvider implements ChatProvider { } get thinkingEffort(): ThinkingEffort | null { - if (this._reasoningEffort === undefined) return null; - return this._reasoningEffort === 'none' ? 'off' : this._reasoningEffort; + return this._thinkingEffort ?? null; } get modelParameters(): Record { @@ -555,16 +554,27 @@ export class OpenAILegacyChatProvider implements ChatProvider { this._generationKwargs, ); - // Determine reasoning_effort - let reasoningEffort: string | undefined = this._reasoningEffort; + // Determine reasoning_effort. 'off' and 'on' have no wire encoding on + // chat-completions APIs, so they send no reasoning_effort field; only a + // concrete effort (low/medium/high/...) is passed through verbatim. + const effort = this._thinkingEffort; + let reasoningEffort: string | undefined = + effort === undefined || effort === 'off' || effort === 'on' ? undefined : effort; // Auto-enable reasoning_effort when the history contains ThinkPart but reasoning // was not explicitly configured. This prevents server validation errors from APIs // (e.g. One API) that require reasoning_effort when messages contain reasoning_content. // Skip when the caller already pinned reasoning_effort via withGenerationKwargs — - // their value would otherwise be silently overwritten below. + // their value would otherwise be silently overwritten below. An explicit 'off' + // from withThinking is honored as well: with thinking turned off the + // auto-enable must not silently switch reasoning back on (or leak the field + // to models that reject it). // See: https://github.com/MoonshotAI/kimi-code/issues/1616 - if (reasoningEffort === undefined && kwargs['reasoning_effort'] === undefined) { + if ( + reasoningEffort === undefined && + effort !== 'off' && + kwargs['reasoning_effort'] === undefined + ) { const hasThinkPart = history.some((message) => message.content.some((part) => part.type === 'think'), ); @@ -618,9 +628,11 @@ export class OpenAILegacyChatProvider implements ChatProvider { } withThinking(effort: ThinkingEffort): OpenAILegacyChatProvider { - const reasoningEffort = effort === 'off' || effort === 'on' ? undefined : effort; const clone = this._clone(); - clone._reasoningEffort = reasoningEffort; + // Store the requested effort verbatim; the wire encoding is derived per + // request so an explicit 'off' stays distinguishable from "never + // configured" (which the history-based auto-enable relies on). + clone._thinkingEffort = effort; return clone; } diff --git a/packages/kosong/test/openai-legacy.test.ts b/packages/kosong/test/openai-legacy.test.ts index 2cd371adea..915580eab9 100644 --- a/packages/kosong/test/openai-legacy.test.ts +++ b/packages/kosong/test/openai-legacy.test.ts @@ -992,6 +992,43 @@ describe('OpenAILegacyChatProvider', () => { expect(maxBody['reasoning_effort']).toBe('max'); expect(xhighBody['reasoning_effort']).toBe('xhigh'); }); + + it('.withThinking("off") sends no reasoning_effort and reports "off"', async () => { + const provider = createProvider().withThinking('off'); + const history: Message[] = [ + { role: 'user', content: [{ type: 'text', text: 'Think' }], toolCalls: [] }, + ]; + const body = await captureRequestBody(provider, '', [], history); + + expect(body['reasoning_effort']).toBeUndefined(); + expect(provider.thinkingEffort).toBe('off'); + }); + + it('.withThinking("on") sends no reasoning_effort without ThinkPart history and reports "on"', async () => { + const provider = createProvider().withThinking('on'); + const history: Message[] = [ + { role: 'user', content: [{ type: 'text', text: 'Think' }], toolCalls: [] }, + ]; + const body = await captureRequestBody(provider, '', [], history); + + expect(body['reasoning_effort']).toBeUndefined(); + expect(provider.thinkingEffort).toBe('on'); + }); + + it('reports a null thinkingEffort until withThinking is called', () => { + expect(createProvider().thinkingEffort).toBeNull(); + }); + + it('.withThinking("off") clears a concrete effort set earlier', async () => { + const provider = createProvider().withThinking('high').withThinking('off'); + expect(provider.thinkingEffort).toBe('off'); + + const history: Message[] = [ + { role: 'user', content: [{ type: 'text', text: 'Think' }], toolCalls: [] }, + ]; + const body = await captureRequestBody(provider, '', [], history); + expect(body['reasoning_effort']).toBeUndefined(); + }); }); describe('auto reasoning_effort', () => { @@ -1082,6 +1119,52 @@ describe('OpenAILegacyChatProvider', () => { expect(body['reasoning_effort']).toBe('high'); }); + + it('does not auto-inject reasoning_effort when thinking was explicitly turned off', async () => { + // An explicit withThinking('off') is not the same as "never configured": + // with thinking off, auto-injection must not silently switch reasoning + // back on (or leak reasoning_effort to models that reject the field). + const provider = createProvider({ model: 'some-model' }).withThinking('off'); + const history: Message[] = [ + { role: 'user', content: [{ type: 'text', text: 'Hello' }], toolCalls: [] }, + { + role: 'assistant', + content: [ + { type: 'think', think: 'Thinking...' }, + { type: 'text', text: 'Hi!' }, + ], + toolCalls: [], + }, + { role: 'user', content: [{ type: 'text', text: 'How are you?' }], toolCalls: [] }, + ]; + const body = await captureRequestBody(provider, '', [], history); + + expect(body['reasoning_effort']).toBeUndefined(); + expect(provider.thinkingEffort).toBe('off'); + }); + + it('still auto-injects reasoning_effort for an explicit "on"', async () => { + // 'on' keeps the #1616 behavior: thinking enabled without a concrete + // effort still pairs reasoning_effort with ThinkPart history so strict + // OpenAI-compatible gateways don't 400. + const provider = createProvider({ model: 'some-model' }).withThinking('on'); + const history: Message[] = [ + { role: 'user', content: [{ type: 'text', text: 'Hello' }], toolCalls: [] }, + { + role: 'assistant', + content: [ + { type: 'think', think: 'Thinking...' }, + { type: 'text', text: 'Hi!' }, + ], + toolCalls: [], + }, + { role: 'user', content: [{ type: 'text', text: 'How are you?' }], toolCalls: [] }, + ]; + const body = await captureRequestBody(provider, '', [], history); + + expect(body['reasoning_effort']).toBe('medium'); + expect(provider.thinkingEffort).toBe('on'); + }); }); describe('default reasoning protocol (no explicit reasoningKey)', () => {