Conversation
Greptile SummaryThis PR changes the default
Confidence Score: 4/5The default change is intentional and well-documented, but it is a breaking behavioral change for any channel using Fireworks or bailian without an explicit reasoning_field: none config. The new test uses assert.NoError where require.NoError is needed — if TransformRequest returns an error, req is nil and req.Body immediately panics, masking the root failure message. llm/transformer/openai/outbound_reasoning_test.go — the assert.NoError / require.NoError mismatch at lines 262 and 266 Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TransformRequest called] --> B{config.ReasoningField == empty?}
B -- "Yes (before PR)" --> C[reasoningField = ReasoningFieldNone\nstrip all reasoning fields]
B -- "Yes (after PR)" --> D[reasoningField = ReasoningFieldContent\npreserve reasoning_content]
B -- No --> E[reasoningField = config.ReasoningField]
C --> F[RequestFromLLM with ReasoningFieldNone\nFireworks / bailian safe]
D --> G[RequestFromLLM with ReasoningFieldContent\nMimo / DeepSeek / Gemini safe]
E --> H{Which field?}
H -- ReasoningFieldContent --> G
H -- ReasoningFieldReasoning --> I[RequestFromLLM with ReasoningFieldReasoning\nNanoGPT / OpenRouter]
H -- ReasoningFieldNone --> F
H -- ReasoningFieldAll --> J[RequestFromLLM with ReasoningFieldAll\nboth fields preserved]
Reviews (2): Last reviewed commit: "chore(llm): change openai default reason..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request updates the OpenAI outbound transformer to change the default reasoning field from ReasoningFieldNone to ReasoningFieldContent. This change ensures compatibility with providers like Mimo that require reasoning content to be echoed back in assistant messages. The documentation was also updated to reflect this new default and include Mimo in the list of supported providers. Feedback was provided to include 'OpenAI o-series' in the documentation for consistency with other comments in the codebase.
| // ReasoningField specifies which reasoning field to use in outbound messages. | ||
| // Use ReasoningFieldContent for DeepSeek/Gemini, ReasoningFieldReasoning for NanoGPT/OpenRouter, | ||
| // or ReasoningFieldNone (default) to strip all reasoning fields. | ||
| // Use ReasoningFieldContent (default) for DeepSeek/Mimo/Gemini, ReasoningFieldReasoning for NanoGPT/OpenRouter, |
There was a problem hiding this comment.
For consistency with the more detailed comment added in TransformRequest, it would be beneficial to also include OpenAI o-series in this list of providers that use ReasoningFieldContent.
| // Use ReasoningFieldContent (default) for DeepSeek/Mimo/Gemini, ReasoningFieldReasoning for NanoGPT/OpenRouter, | |
| // Use ReasoningFieldContent (default) for OpenAI o-series/DeepSeek/Mimo/Gemini, ReasoningFieldReasoning for NanoGPT/OpenRouter, |
| @@ -246,14 +258,17 @@ func TestRequestFromLLM_DefaultNone(t *testing.T) { | |||
| Reasoning: lo.ToPtr("Another reasoning"), | |||
| }, | |||
| }, | |||
| } | |||
| }) | |||
| assert.NoError(t, err) | |||
|
|
|||
| req := RequestFromLLM(llmReq, ReasoningFieldNone) | |||
| var oaiReq Request | |||
| err = json.Unmarshal(req.Body, &oaiReq) | |||
| assert.NoError(t, err) | |||
There was a problem hiding this comment.
require.NoError for both error checks so the test stops immediately with the real failure message instead of panicking on req.Body when the first call errors.
| req, err := transformer.TransformRequest(t.Context(), &llm.Request{ | |
| Model: "test-model", | |
| Messages: []llm.Message{ | |
| { | |
| Role: "assistant", | |
| ReasoningContent: &reasoningText, | |
| Reasoning: lo.ToPtr("Another reasoning"), | |
| }, | |
| }, | |
| }) | |
| require.NoError(t, err) | |
| var oaiReq Request | |
| err = json.Unmarshal(req.Body, &oaiReq) | |
| require.NoError(t, err) |
No description provided.