Skip to content

fix(core): mirror Qwen reasoning history field#4289

Open
YuanHanzhong wants to merge 1 commit into
QwenLM:mainfrom
YuanHanzhong:fix/qwen-reasoning-field
Open

fix(core): mirror Qwen reasoning history field#4289
YuanHanzhong wants to merge 1 commit into
QwenLM:mainfrom
YuanHanzhong:fix/qwen-reasoning-field

Conversation

@YuanHanzhong
Copy link
Copy Markdown

Summary

  • Mirror reasoning_content to reasoning for self-hosted Qwen history messages.
  • Avoid mutating the shared history objects while shaping provider requests.
  • Keep the existing provider-specific paths unchanged.

Testing

  • npx vitest run packages/core/src/core/openaiContentGenerator/provider/default.test.ts
  • npm run typecheck --workspace packages/core
  • npm run lint --workspace packages/core -- src/core/openaiContentGenerator/provider/default.ts src/core/openaiContentGenerator/provider/default.test.ts
  • git diff --check

Fixes #4285

Copy link
Copy Markdown
Collaborator

@wenshao wenshao left a comment

Choose a reason for hiding this comment

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

Cross-file note: mirrorQwenReasoningContent adds reasoning field; MistralOpenAICompatibleProvider.buildRequest() calls super.buildRequest() (now running the mirror), then runs stripReasoningContent which removes reasoning_content but not reasoning. Low probability (requires Qwen model on Mistral endpoint) but if triggered, Mistral API may receive an orphaned reasoning field. Consider also stripping reasoning in stripReasoningContent.

— DeepSeek/deepseek-v4-pro via Qwen Code /review

max_tokens: effectiveMaxTokens,
};
}

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] Missing JSDoc on mirrorQwenReasoningContent. The method silently transforms messages by copying reasoning_content to reasoning, but there is no comment explaining why this is needed. Other methods like applyOutputTokenLimit and DeepSeek's equivalent have JSDoc blocks explaining the motivation and linked issues.

Suggested change
/**
* Mirrors `reasoning_content` to `reasoning` for Qwen model history.
* Qwen's self-hosted API requires the `reasoning` field (not just
* `reasoning_content`) when replaying assistant messages in multi-turn
* conversations. This copies the content without removing the original
* field to preserve backward compatibility.
*
* Fixes #4285
*/
private mirrorQwenReasoningContent(

— DeepSeek/deepseek-v4-pro via Qwen Code /review

});
});

it('should mirror reasoning_content to reasoning for Qwen model history', () => {
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 single test covers only the happy path (one assistant message with reasoning_content). Four critical branches are untested:

  1. Non-Qwen model no-opmodel: 'gpt-4' with reasoning_content should NOT inject reasoning
  2. Empty reasoning_contentreasoning_content: '' should skip mirroring (explicit length === 0 guard)
  3. Existing reasoning field — assistant message with both reasoning and reasoning_content should NOT overwrite reasoning
  4. Non-assistant messages — user/system messages with reasoning_content should be ignored (role !== 'assistant' guard)

These guards are the most frequently hit code paths and regressions here would silently break API behavior.

— DeepSeek/deepseek-v4-pro via Qwen Code /review

return message;
}

const extended = message as unknown as Record<string, unknown>;
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 double-cast as unknown as Record<string, unknown> to access reasoning_content can be replaced with the project's existing typed interface. ExtendedChatCompletionAssistantMessageParam (exported from converter.ts) already declares reasoning_content?: string | null. DeepSeek's equivalent function uses it directly:

Suggested change
const extended = message as unknown as Record<string, unknown>;
const extended = message as ExtendedChatCompletionAssistantMessageParam;
const reasoningContent = extended.reasoning_content;

This catches typos, provides IDE support, and is consistent with how ensureReasoningContentOnToolCalls in deepseek.ts accesses the same field. The write side (reasoning) still needs a cast since that field isn't in the type.

— qwen-latest-series-invite-beta-v28 via Qwen Code /review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Outgoing requests send reasoning_content instead of reasoning — vLLM ≥ 0.20 strips the field, leaving prior <think> blocks empty

2 participants