fix: Address PR #206 review feedback for OpenAI thinking mode

- Fix afterEach env var leak: use delete instead of assigning undefined
- Fix turn boundary detection: treat multimodal inputs (e.g. images)
  as new turns, not just text content
- Extract buildOpenAIRequestBody() for testability, replace constant-
  assertion tests with real function output verification

Signed-off-by: guunergooner <tongchao0923@gmail.com>

Author: guunergooner

src/services/api/openai/__tests__/thinking.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test, beforeEach, afterEach } from 'bun:test'
-import { isOpenAIThinkingEnabled } from '../index.js'
+import { isOpenAIThinkingEnabled, buildOpenAIRequestBody } from '../index.js'
 
 describe('isOpenAIThinkingEnabled', () => {
   const originalEnv = {
@@ -12,8 +12,13 @@ describe('isOpenAIThinkingEnabled', () => {
   })
 
   afterEach(() => {
-    // Restore original env var
-    process.env.OPENAI_ENABLE_THINKING = originalEnv.OPENAI_ENABLE_THINKING
+    // Restore original env var — delete key if it was originally undefined
+    // to avoid leaking the env key into subsequent tests
+    if (originalEnv.OPENAI_ENABLE_THINKING === undefined) {
+      delete process.env.OPENAI_ENABLE_THINKING
+    } else {
+      process.env.OPENAI_ENABLE_THINKING = originalEnv.OPENAI_ENABLE_THINKING
+    }
   })
 
   describe('OPENAI_ENABLE_THINKING env var', () => {
@@ -140,53 +145,82 @@ describe('isOpenAIThinkingEnabled', () => {
   })
 })
 
-describe('thinking request parameters', () => {
-  // Note: These tests verify the request body structure indirectly.
-  // The actual API call is mocked in integration tests.
-  // Here we document the expected parameter formats:
+describe('buildOpenAIRequestBody — thinking params', () => {
+  const baseParams = {
+    model: 'deepseek-reasoner',
+    messages: [{ role: 'user', content: 'hello' }],
+    tools: [] as any[],
+    toolChoice: undefined as any,
+  }
 
-  test('documents official DeepSeek API format: thinking: { type: "enabled" }', () => {
-    // Official DeepSeek API expects:
-    const officialFormat = {
-      thinking: { type: 'enabled' },
-    }
-    expect(officialFormat.thinking.type).toBe('enabled')
+  test('includes official DeepSeek API thinking format when enabled', () => {
+    const body = buildOpenAIRequestBody({ ...baseParams, enableThinking: true })
+    expect(body.thinking).toEqual({ type: 'enabled' })
   })
 
-  test('documents vLLM/self-hosted format: enable_thinking + chat_template_kwargs', () => {
-    // Self-hosted DeepSeek-V3.2/vLLM expects:
-    const vllmFormat = {
-      enable_thinking: true,
-      chat_template_kwargs: { thinking: true },
-    }
-    expect(vllmFormat.enable_thinking).toBe(true)
-    expect(vllmFormat.chat_template_kwargs.thinking).toBe(true)
+  test('includes vLLM/self-hosted thinking format when enabled', () => {
+    const body = buildOpenAIRequestBody({ ...baseParams, enableThinking: true })
+    expect(body.enable_thinking).toBe(true)
+    expect(body.chat_template_kwargs).toEqual({ thinking: true })
   })
 
-  test('both formats are added simultaneously when thinking is enabled', () => {
-    // The implementation adds both formats so each endpoint
-    // can use the one it recognizes:
-    const combinedFormat = {
-      // Official DeepSeek API format
-      thinking: { type: 'enabled' },
-      // Self-hosted DeepSeek-V3.2/vLLM format
-      enable_thinking: true,
-      chat_template_kwargs: { thinking: true },
-    }
-    expect(combinedFormat.thinking.type).toBe('enabled')
-    expect(combinedFormat.enable_thinking).toBe(true)
-    expect(combinedFormat.chat_template_kwargs.thinking).toBe(true)
+  test('includes both formats simultaneously when enabled', () => {
+    const body = buildOpenAIRequestBody({ ...baseParams, enableThinking: true })
+    expect(body.thinking).toEqual({ type: 'enabled' })
+    expect(body.enable_thinking).toBe(true)
+    expect(body.chat_template_kwargs.thinking).toBe(true)
   })
 
-  test('thinking params are NOT added when thinking is disabled', () => {
-    // When thinking is disabled, none of these params should be present:
-    const disabledFormat = {
-      model: 'gpt-4o',
-      messages: [],
-      stream: true,
-    }
-    expect((disabledFormat as any).thinking).toBeUndefined()
-    expect((disabledFormat as any).enable_thinking).toBeUndefined()
-    expect((disabledFormat as any).chat_template_kwargs).toBeUndefined()
+  test('does NOT include thinking params when disabled', () => {
+    const body = buildOpenAIRequestBody({ ...baseParams, enableThinking: false })
+    expect(body.thinking).toBeUndefined()
+    expect(body.enable_thinking).toBeUndefined()
+    expect(body.chat_template_kwargs).toBeUndefined()
+  })
+
+  test('always includes stream and stream_options', () => {
+    const body = buildOpenAIRequestBody({ ...baseParams, enableThinking: false })
+    expect(body.stream).toBe(true)
+    expect(body.stream_options).toEqual({ include_usage: true })
+  })
+
+  test('includes temperature when thinking is off and override is set', () => {
+    const body = buildOpenAIRequestBody({
+      ...baseParams,
+      enableThinking: false,
+      temperatureOverride: 0.7,
+    })
+    expect(body.temperature).toBe(0.7)
+  })
+
+  test('excludes temperature when thinking is on even if override is set', () => {
+    const body = buildOpenAIRequestBody({
+      ...baseParams,
+      enableThinking: true,
+      temperatureOverride: 0.7,
+    })
+    expect(body.temperature).toBeUndefined()
+  })
+
+  test('excludes temperature when thinking is off and no override', () => {
+    const body = buildOpenAIRequestBody({ ...baseParams, enableThinking: false })
+    expect(body.temperature).toBeUndefined()
+  })
+
+  test('includes tools and tool_choice when tools are provided', () => {
+    const body = buildOpenAIRequestBody({
+      ...baseParams,
+      tools: [{ type: 'function', function: { name: 'test' } }],
+      toolChoice: 'auto',
+      enableThinking: false,
+    })
+    expect(body.tools).toHaveLength(1)
+    expect(body.tool_choice).toBe('auto')
+  })
+
+  test('excludes tools when empty', () => {
+    const body = buildOpenAIRequestBody({ ...baseParams, enableThinking: false })
+    expect(body.tools).toBeUndefined()
+    expect(body.tool_choice).toBeUndefined()
   })
 })

src/services/api/openai/convertMessages.ts

@@ -59,12 +59,20 @@ export function anthropicMessagesToOpenAI(
       }
       if (msg.type === 'user' && hasSeenAssistant) {
         const content = msg.message.content
-        const hasText = typeof content === 'string'
+        // A user message starts a new turn if it contains any non-tool_result content
+        // (text, image, or other media). Tool results alone do NOT start a new turn
+        // because they are continuations of the previous assistant tool call.
+        const startsNewUserTurn = typeof content === 'string'
           ? content.length > 0
           : Array.isArray(content) && content.some(
-              (b: any) => typeof b === 'string' || b.type === 'text',
+              (b: any) =>
+                typeof b === 'string' ||
+                (b &&
+                  typeof b === 'object' &&
+                  'type' in b &&
+                  b.type !== 'tool_result'),
             )
-        if (hasText) {
+        if (startsNewUserTurn) {
           turnBoundaries.add(i)
         }
       }

src/services/api/openai/index.ts

@@ -63,6 +63,53 @@ export function isOpenAIThinkingEnabled(model: string): boolean {
   return modelLower.includes('deepseek-reasoner') || modelLower.includes('deepseek-v3.2')
 }
 
+/**
+ * Build the request body for OpenAI chat.completions.create().
+ * Extracted for testability — the thinking mode params are injected here.
+ *
+ * DeepSeek thinking mode: inject thinking params via request body.
+ * Two formats are added simultaneously to support different deployments:
+ * - Official DeepSeek API: `thinking: { type: 'enabled' }`
+ * - Self-hosted DeepSeek-V3.2: `enable_thinking: true` + `chat_template_kwargs: { thinking: true }`
+ * OpenAI SDK passes unknown keys through to the HTTP body.
+ * Each endpoint will use the format it recognizes and ignore the others.
+ * @internal Exported for testing purposes only
+ */
+export function buildOpenAIRequestBody(params: {
+  model: string
+  messages: any[]
+  tools: any[]
+  toolChoice: any
+  enableThinking: boolean
+  temperatureOverride?: number
+}): Record<string, any> {
+  const { model, messages, tools, toolChoice, enableThinking, temperatureOverride } = params
+  return {
+    model,
+    messages,
+    ...(tools.length > 0 && {
+      tools,
+      ...(toolChoice && { tool_choice: toolChoice }),
+    }),
+    stream: true,
+    stream_options: { include_usage: true },
+    // DeepSeek thinking mode: enable chain-of-thought output.
+    // When active, temperature/top_p/presence_penalty/frequency_penalty are ignored by DeepSeek.
+    ...(enableThinking && {
+      // Official DeepSeek API format
+      thinking: { type: 'enabled' },
+      // Self-hosted DeepSeek-V3.2 format
+      enable_thinking: true,
+      chat_template_kwargs: { thinking: true },
+    }),
+    // Only send temperature when thinking mode is off (DeepSeek ignores it anyway,
+    // but other providers may respect it)
+    ...(!enableThinking && temperatureOverride !== undefined && {
+      temperature: temperatureOverride,
+    }),
+  }
+}
+
 /**
  * OpenAI-compatible query path. Converts Anthropic-format messages/tools to
  * OpenAI format, calls the OpenAI-compatible endpoint, and converts the
@@ -177,40 +224,17 @@ export async function* queryModelOpenAI(
     )
 
     // 11. Call OpenAI API with streaming
-    // DeepSeek thinking mode: inject thinking params via request body.
-    // Two formats are added simultaneously to support different deployments:
-    // - Official DeepSeek API: `thinking: { type: 'enabled' }`
-    // - Self-hosted DeepSeek-V3.2: `enable_thinking: true` + `chat_template_kwargs: { thinking: true }`
-    // OpenAI SDK passes unknown keys through to the HTTP body.
-    // Each endpoint will use the format it recognizes and ignore the others.
+    const requestBody = buildOpenAIRequestBody({
+      model: openaiModel,
+      messages: openaiMessages,
+      tools: openaiTools,
+      toolChoice: openaiToolChoice,
+      enableThinking,
+      temperatureOverride: options.temperatureOverride,
+    })
     const stream = await client.chat.completions.create(
-      {
-        model: openaiModel,
-        messages: openaiMessages,
-        ...(openaiTools.length > 0 && {
-          tools: openaiTools,
-          ...(openaiToolChoice && { tool_choice: openaiToolChoice }),
-        }),
-        stream: true,
-        stream_options: { include_usage: true },
-        // DeepSeek thinking mode: enable chain-of-thought output.
-        // When active, temperature/top_p/presence_penalty/frequency_penalty are ignored by DeepSeek.
-        ...(enableThinking && {
-          // Official DeepSeek API format
-          thinking: { type: 'enabled' },
-          // Self-hosted DeepSeek-V3.2 format
-          enable_thinking: true,
-          chat_template_kwargs: { thinking: true },
-        }),
-        // Only send temperature when thinking mode is off (DeepSeek ignores it anyway,
-        // but other providers may respect it)
-        ...(!enableThinking && options.temperatureOverride !== undefined && {
-          temperature: options.temperatureOverride,
-        }),
-      },
-      {
-        signal,
-      },
+      requestBody,
+      { signal },
     )
 
     // 12. Convert OpenAI stream to Anthropic events, then process into