Skip to content
Merged
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
135 changes: 135 additions & 0 deletions packages/core/src/core/openaiContentGenerator/pipeline.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1108,6 +1108,141 @@ describe('ContentGenerationPipeline', () => {
expect(apiCall.enable_thinking).toBeUndefined();
});

it('disables qwen thinking via chat_template_kwargs on a non-DashScope endpoint (vLLM/SGLang)', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The only coder-model test (line ~986) uses AuthType.QWEN_OAUTH, which causes isDashScopeProvider to return true. The || model === 'coder-model' branch inside the non-DashScope else arm is therefore never exercised in tests.

Consider adding a test with model: 'coder-model', baseUrl: 'https://llm.example.com/v1' (non-DashScope), includeThoughts: false, asserting chat_template_kwargs equals { enable_thinking: false }.

— qwen3.7-max via Qwen Code /review

// Self-hosted OpenAI-compatible servers render the chat template
// server-side and read the thinking switch from `chat_template_kwargs`,
// silently ignoring a top-level `enable_thinking`. A qwen model on such
// an endpoint must therefore get the switch nested, not top-level — and
// any top-level `enable_thinking: true` a provider preset injected via
// extra_body must be stripped so it can't contradict the opt-out.
mockContentGeneratorConfig = {
...mockContentGeneratorConfig,
baseUrl: 'https://llm.example.com/v1',
model: 'Qwen3.6-27B',
} as ContentGeneratorConfig;
mockConfig = {
...mockConfig,
contentGeneratorConfig: mockContentGeneratorConfig,
};
pipeline = new ContentGenerationPipeline(mockConfig);

(mockProvider.buildRequest as Mock).mockImplementation((req) => ({
...req,
enable_thinking: true, // Simulates extra_body injection
}));

const request: GenerateContentParameters = {
model: 'Qwen3.6-27B',
contents: [{ parts: [{ text: 'Suggest' }], role: 'user' }],
config: { thinkingConfig: { includeThoughts: false } },
};

(mockConverter.convertGeminiRequestToOpenAI as Mock).mockReturnValue([
{ role: 'user', content: 'Suggest' },
]);
(mockConverter.convertOpenAIResponseToGemini as Mock).mockReturnValue(
new GenerateContentResponse(),
);
(mockClient.chat.completions.create as Mock).mockResolvedValue({
id: 'r',
choices: [{ message: { content: 'ok' }, finish_reason: 'stop' }],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The test now verifies that a top-level enable_thinking: true injected via extra_body gets stripped — nice improvement. However, the chat_template_kwargs merge with pre-existing keys (other than enable_thinking) is still untested.

The production code at pipeline.ts spreads ...existing before appending enable_thinking: false. Consider adding a test case where mockProvider.buildRequest also returns chat_template_kwargs: { apply_chat_template: true } and asserting:

expect(apiCall.chat_template_kwargs).toEqual({ apply_chat_template: true, enable_thinking: false });

This guards against a future refactor that drops the ...existing spread and silently loses user-configured kwargs.

— qwen3.7-max via Qwen Code /review

} as OpenAI.Chat.ChatCompletion);

await pipeline.execute(request, 'forked_query');

const apiCall = (mockClient.chat.completions.create as Mock).mock
.calls[0][0];
expect(apiCall.chat_template_kwargs).toEqual({ enable_thinking: false });
expect(apiCall.enable_thinking).toBeUndefined();
});

it('disables coder-model thinking via chat_template_kwargs on a non-DashScope endpoint', async () => {
// `coder-model` is the QWEN_OAUTH default, but a user can point it at a
// self-hosted endpoint. The `model === 'coder-model'` arm must reach the
// non-DashScope chat_template_kwargs path just like a `qwen*` model.
mockContentGeneratorConfig = {
...mockContentGeneratorConfig,
baseUrl: 'https://llm.example.com/v1',
model: 'coder-model',
} as ContentGeneratorConfig;
mockConfig = {
...mockConfig,
contentGeneratorConfig: mockContentGeneratorConfig,
};
pipeline = new ContentGenerationPipeline(mockConfig);

const request: GenerateContentParameters = {
model: 'coder-model',
contents: [{ parts: [{ text: 'Suggest' }], role: 'user' }],
config: { thinkingConfig: { includeThoughts: false } },
};

(mockConverter.convertGeminiRequestToOpenAI as Mock).mockReturnValue([
{ role: 'user', content: 'Suggest' },
]);
(mockConverter.convertOpenAIResponseToGemini as Mock).mockReturnValue(
new GenerateContentResponse(),
);
(mockClient.chat.completions.create as Mock).mockResolvedValue({
id: 'r',
choices: [{ message: { content: 'ok' }, finish_reason: 'stop' }],
} as OpenAI.Chat.ChatCompletion);

await pipeline.execute(request, 'forked_query');

const apiCall = (mockClient.chat.completions.create as Mock).mock
.calls[0][0];
expect(apiCall.chat_template_kwargs).toEqual({ enable_thinking: false });
expect(apiCall.enable_thinking).toBeUndefined();
});

it('merges enable_thinking into pre-existing chat_template_kwargs on a non-DashScope endpoint', async () => {
// The non-DashScope path spreads any existing `chat_template_kwargs`
// before appending `enable_thinking: false`. Guard the merge so a future
// refactor can't silently drop user-configured kwargs.
mockContentGeneratorConfig = {
...mockContentGeneratorConfig,
baseUrl: 'https://llm.example.com/v1',
model: 'Qwen3.6-27B',
} as ContentGeneratorConfig;
mockConfig = {
...mockConfig,
contentGeneratorConfig: mockContentGeneratorConfig,
};
pipeline = new ContentGenerationPipeline(mockConfig);

(mockProvider.buildRequest as Mock).mockImplementation((req) => ({
...req,
chat_template_kwargs: { apply_chat_template: true },
}));

const request: GenerateContentParameters = {
model: 'Qwen3.6-27B',
contents: [{ parts: [{ text: 'Suggest' }], role: 'user' }],
config: { thinkingConfig: { includeThoughts: false } },
};

(mockConverter.convertGeminiRequestToOpenAI as Mock).mockReturnValue([
{ role: 'user', content: 'Suggest' },
]);
(mockConverter.convertOpenAIResponseToGemini as Mock).mockReturnValue(
new GenerateContentResponse(),
);
(mockClient.chat.completions.create as Mock).mockResolvedValue({
id: 'r',
choices: [{ message: { content: 'ok' }, finish_reason: 'stop' }],
} as OpenAI.Chat.ChatCompletion);

await pipeline.execute(request, 'forked_query');

const apiCall = (mockClient.chat.completions.create as Mock).mock
.calls[0][0];
expect(apiCall.chat_template_kwargs).toEqual({
apply_chat_template: true,
enable_thinking: false,
});
});

it('does NOT emit enable_thinking on a non-qwen model routed through DashScope', async () => {
// DashScope's compatible-mode endpoint routes multiple model families
// (qwen3, GLM, DeepSeek). Hostname alone is not enough — GLM uses
Expand Down
40 changes: 33 additions & 7 deletions packages/core/src/core/openaiContentGenerator/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -584,13 +584,39 @@ export class ContentGenerationPipeline {
// start with `qwen` but is the most common hybrid-thinking model
// for first-time users, so it must be covered.
const model = (context.model ?? '').toLowerCase();
if (
DashScopeOpenAICompatibleProvider.isDashScopeProvider(
this.contentGeneratorConfig,
) &&
(model.startsWith('qwen') || model === 'coder-model')
) {
typed['enable_thinking'] = false;
if (model.startsWith('qwen') || model === 'coder-model') {
if (
DashScopeOpenAICompatibleProvider.isDashScopeProvider(
this.contentGeneratorConfig,
)
) {
typed['enable_thinking'] = false;
} else {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] The non-DashScope branch sets chat_template_kwargs: { enable_thinking: false } but does not strip any top-level enable_thinking that extra_body may have already injected into the request.

When a provider preset (e.g. ModelScope's Qwen/Qwen3.5-397B-A17B) has enableThinking: true, provider-config.ts compiles it into extra_body: { enable_thinking: true }, and DefaultOpenAICompatibleProvider.buildRequest spreads that into the top-level request. The resulting wire request then carries both "enable_thinking": true (top-level) and "chat_template_kwargs": { "enable_thinking": false } — contradictory signals.

While vLLM/SGLang may ignore the top-level field (as the comment notes), other OpenAI-compatible servers (ModelScope, LiteLLM proxies) that read both could keep thinking enabled despite the explicit opt-out. The new test doesn't catch this because its mock doesn't inject enable_thinking: true via extra_body, so expect(apiCall.enable_thinking).toBeUndefined() passes trivially.

Suggested change
} else {
} else {
// Non-DashScope OpenAI-compatible servers (vLLM, SGLang, ...) render
// the model's chat template server-side and read the thinking switch
// from `chat_template_kwargs`, not a top-level `enable_thinking`
// (which they silently ignore). Send it there so hybrid qwen models
// actually stop emitting <think> when reasoning is disabled — e.g.
// the auto-mode permission classifier's short structured-output
// calls, which otherwise spend their small token budget on thinking
// and fail closed.
// Strip any top-level enable_thinking injected by extra_body
// (provider-config.ts sets it for models with enableThinking: true).
delete typed['enable_thinking'];
const existing = (typed['chat_template_kwargs'] ?? {}) as Record<
string,
unknown
>;
typed['chat_template_kwargs'] = {
...existing,
enable_thinking: false,
};
}

— qwen3.7-max via Qwen Code /review

// Non-DashScope OpenAI-compatible servers (vLLM, SGLang, ...) render

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] chat_template_kwargs is a vLLM/SGLang convention, but this else-branch fires for every non-DashScope endpoint — including LiteLLM proxies, Ollama, and other OpenAI-compatible servers. Most will silently ignore unknown fields (harmless), but servers that validate request bodies strictly could reject the request.

Consider documenting this assumption in the comment, or gating the field on a known server list.

— qwen3.7-max via Qwen Code /review

// the model's chat template server-side and read the thinking switch
// from `chat_template_kwargs`, not a top-level `enable_thinking`
// (which they silently ignore). Send it there so hybrid qwen models
// actually stop emitting <think> when reasoning is disabled — e.g.
// the auto-mode permission classifier's short structured-output
// calls, which otherwise spend their small token budget on thinking
// and fail closed. Servers that don't recognise `chat_template_kwargs`
// ignore the unknown field, so the switch is a harmless no-op there.
//
// Drop any top-level `enable_thinking` a provider preset injected via
// extra_body (provider-config.ts emits it for models configured with
// `enableThinking: true`): leaving it would contradict the
// `chat_template_kwargs` opt-out on servers that honour both, and
// keeps this path from leaking the qwen-specific field top-level.
delete typed['enable_thinking'];
const existing = (typed['chat_template_kwargs'] ?? {}) as Record<
string,
unknown
>;
typed['chat_template_kwargs'] = {
...existing,
enable_thinking: false,
};
}
}
// Strip reasoning config — extra_body could inject it, overriding
// buildReasoningConfig's decision to return {} for disabled thinking.
Expand Down
Loading