fix: exclude chat/search variants from reasoning-model detection (#902)#1520
Open
rochmanofenna wants to merge 1 commit into
Open
fix: exclude chat/search variants from reasoning-model detection (#902)#1520rochmanofenna wants to merge 1 commit into
rochmanofenna wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #902.
OpenAIClientdecides whether to sendreasoning.effortviamodel.startswith(('gpt-5','o1','o3')). That also matches non-reasoning variants i.e.gpt-5-chat-latest,gpt-5.3-chat-latest,gpt-5-search-api, which reject the parameter and return:400 - Unsupported parameter: 'reasoning.effort' is not supported with this model.This is the failure reported in #902.
Type of Change
Objective
Extract the detection into a shared
_is_reasoning_model()helper used by both completion paths (the predicate was duplicated), and exclude-chat/-searchvariants so they route through the normal path without the reasoning param.Distinct from #1378/#1395, which change the effort value (
minimal→low). For chat/search variants no value works, the parameter itself is unsupported.gpt-5-miniand other true reasoning models remain correctly classified.Testing
Added
tests/llm_client/test_openai_reasoning_detection.py(parametrized: reasoning models → True, chat/search variants → False, gpt-4o/4.1 → False).make format,make lint,make pyrightall clean.Breaking Changes
Checklist
make lintpasses)Related Issues
Closes #902
Open question for maintainers
The same prefix check is mirrored in
mcp_server/src/services/factories.py. I scoped this PR tographiti_core(self-contained, benefits all consumers). Happy to extend to the MCP factory here or in a follow-up.