Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/api/providers/__tests__/openai.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,46 @@ describe("OpenAiHandler", () => {
expect(model.id).toBe("")
expect(model.info).toBeDefined()
})

it("should set preserveReasoning when openAiR1FormatEnabled is on", () => {
const r1Handler = new OpenAiHandler({
...mockOptions,
openAiR1FormatEnabled: true,
})
const model = r1Handler.getModel()
expect(model.info).toEqual({ ...openAiModelInfoSaneDefaults, preserveReasoning: true })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both new tests leave openAiCustomModelInfo unset, so info is value-identical to the defaults here — the { ...info } merge only gets exercised on the defaults path. A case combining custom model info (e.g. a distinct contextWindow) with the toggle would pin the merge for the llama.cpp / LM Studio / Ollama setups this PR targets. Worth adding?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 1a47887: should merge preserveReasoning into custom model info when openAiR1FormatEnabled is on builds openAiCustomModelInfo with a distinct contextWindow (32_768) and supportsImages: false, and asserts the full merged object { ...customInfo, preserveReasoning: true }, so the { ...info } merge path is actually exercised.

})

it("should set preserveReasoning for deepseek-reasoner model ids without the toggle", () => {
const deepseekHandler = new OpenAiHandler({
...mockOptions,
openAiModelId: "deepseek-reasoner",
})
const model = deepseekHandler.getModel()
expect(model.info.preserveReasoning).toBe(true)
})

it("should merge preserveReasoning into custom model info when openAiR1FormatEnabled is on", () => {
const customInfo: ModelInfo = {
...openAiModelInfoSaneDefaults,
contextWindow: 32_768,
supportsImages: false,
}
const r1Handler = new OpenAiHandler({
...mockOptions,
openAiCustomModelInfo: customInfo,
openAiR1FormatEnabled: true,
})
const model = r1Handler.getModel()
expect(model.info).toEqual({ ...customInfo, preserveReasoning: true })
expect(model.info.contextWindow).toBe(32_768)
expect(model.info.supportsImages).toBe(false)
})

it("should not set preserveReasoning by default", () => {
const model = handler.getModel()
expect(model.info.preserveReasoning).toBeUndefined()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pins the flag, but the consumer it feeds — buildCleanConversationHistory in Task.ts (preserveReasoning === true at Task.ts:5020) — is only covered by tests that re-implement the branch (reasoning-preservation.test.ts:223, 290, 347, 398), so deleting the production gate would still pass everything. Would a regression test calling the real method be worth adding here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 1a47887: two tests in reasoning-preservation.test.ts store a plain-text reasoning block through the real addToApiConversationHistory and then call the real Task.buildCleanConversationHistory for both the preserve and the strip path. I mutation-checked them: removing the production gate fails the preserve test, inverting it fails the strip test. Note that on current main the method takes requestModelInfo as a second argument (the gate now reads requestModelInfo.preserveReasoning === true, Task.ts:5800), so the tests pass the ModelInfo through the real signature instead of stubbing api.getModel().

})
})

describe("Azure AI Inference Service", () => {
Expand Down
22 changes: 19 additions & 3 deletions src/api/providers/openai.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,8 @@ export class OpenAiHandler extends BaseProvider implements SingleCompletionHandl
const { info: modelInfo, reasoning } = this.getModel()
const modelUrl = this.options.openAiBaseUrl ?? ""
const modelId = this.options.openAiModelId ?? ""
const enabledR1Format = this.options.openAiR1FormatEnabled ?? false
const isAzureAiInference = this._isAzureAiInference(modelUrl)
const deepseekReasoner = modelId.includes("deepseek-reasoner") || enabledR1Format
const deepseekReasoner = this.usesR1Format(modelId)

if (modelId.includes("o1") || modelId.includes("o3") || modelId.includes("o4")) {
yield* this.handleO3FamilyMessage(modelId, systemPrompt, messages, metadata)
Expand Down Expand Up @@ -294,6 +293,14 @@ export class OpenAiHandler extends BaseProvider implements SingleCompletionHandl
}
}

// Single gate for everything that depends on the R1 request format: message
// conversion in createMessage() and reasoning retention in getModel(). Both
// must agree, otherwise reasoning is converted for the request but stripped
// from the follow-up context (or vice versa).
private usesR1Format(modelId: string): boolean {
return modelId.includes("deepseek-reasoner") || (this.options.openAiR1FormatEnabled ?? false)
}

override getModel() {
const id = this.options.openAiModelId ?? ""
const info: ModelInfo = this.options.openAiCustomModelInfo ?? openAiModelInfoSaneDefaults
Expand All @@ -306,7 +313,16 @@ export class OpenAiHandler extends BaseProvider implements SingleCompletionHandl
settings: { ...this.options, reasoningEffort: info.reasoningEffort },
defaultTemperature: 0,
})
return { id, info, ...params }
// Local OpenAI-compatible reasoning models (llama.cpp, LM Studio, Ollama)
// stream reasoning_content, but Zoo Code strips it from the follow-up context
// unless info.preserveReasoning is set. Whenever createMessage() treats the
// model as R1 (deepseek-reasoner id or the R1 format toggle), also treat it
// as preserving reasoning so the chain is fed back.
return {
id,
info: this.usesR1Format(id) ? { ...info, preserveReasoning: true } : info,
...params,
}
}

async completePrompt(prompt: string, options?: CompletePromptOptions): Promise<string> {
Expand Down
66 changes: 66 additions & 0 deletions src/core/task/__tests__/reasoning-preservation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -512,4 +512,70 @@ describe("Task reasoning preservation", () => {
text: assistantText,
})
})

it("should keep plain text reasoning in buildCleanConversationHistory when preserveReasoning is true", async () => {
const task = new Task({
provider: mockProvider as ClineProvider,
apiConfiguration: mockApiConfiguration,
task: "Test task",
startTask: false,
})

const requestModelInfo: ModelInfo = {
contextWindow: 16000,
supportsPromptCache: true,
preserveReasoning: true,
}

await task["addToApiConversationHistory"](
{
role: "assistant",
content: [{ type: "text", text: "Reading the file." }],
},
"I should read the file first.",
)

const history = task["buildCleanConversationHistory"](task.apiConversationHistory, requestModelInfo)

expect(history).toHaveLength(1)
expect(history[0]).toMatchObject({
role: "assistant",
content: [
{ type: "reasoning", text: "I should read the file first.", summary: [] },
{ type: "text", text: "Reading the file." },
],
})
})

it("should strip plain text reasoning in buildCleanConversationHistory when preserveReasoning is not set", async () => {
const task = new Task({
provider: mockProvider as ClineProvider,
apiConfiguration: mockApiConfiguration,
task: "Test task",
startTask: false,
})

// preserveReasoning is undefined on this model info
const requestModelInfo: ModelInfo = {
contextWindow: 16000,
supportsPromptCache: true,
}

await task["addToApiConversationHistory"](
{
role: "assistant",
content: [{ type: "text", text: "Reading the file." }],
},
"I should read the file first.",
)

const history = task["buildCleanConversationHistory"](task.apiConversationHistory, requestModelInfo)

expect(history).toHaveLength(1)
expect(history[0]).toMatchObject({
role: "assistant",
content: "Reading the file.",
})
expect(JSON.stringify(history)).not.toContain("reasoning")
})
})
Loading