fix: positively gate reasoning-effort branch, extend gpt-5 alias fallback - #824
Open
WSxDemise wants to merge 6 commits into
Open
fix: positively gate reasoning-effort branch, extend gpt-5 alias fallback#824WSxDemise wants to merge 6 commits into
WSxDemise wants to merge 6 commits into
Conversation
The /model_settings reasoning_effort picker always hid xhigh/max behind supports_xhigh_reasoning/supports_max_reasoning flags that no code path ever set to True, so no OpenAI model -- including GPT-5.6, which actually documents "max" -- could ever offer it. o1/o3/o4-mini/ codex-mini never got reasoning_effort at all. - model_utils.get_openai_reasoning_effort_choices(): single source of truth mapping real OpenAI model ids to the none/low/medium/high/ xhigh/max values OpenAI documents for each (verified against the live OpenAI OpenAPI spec + platform docs). - model_settings_menu._get_setting_choices(): use it to gate the reasoning_effort picker instead of always-False boolean flags; legacy flags still work as an explicit widen-only override. - config.model_supports_setting(): recognize reasoning_effort dynamically for any recognized OpenAI model, regardless of whether its catalog entry declares supported_settings (extra_models.json / OAuth-sourced entries included). - add_model_menu._build_model_config(): use the same helper when adding a model from models.dev, so o-series/codex-mini models get reasoning_effort in their stored supported_settings too (previously only gpt-5* did), and list_available_models() reports it correctly. Recognized-but-not-configurable models (o1-mini, o1-preview, gpt-5-pro, *-chat-latest) correctly report no choices/no support rather than a broken empty picker.
Pre-PR review caught that exposing reasoning_effort in the /model_settings menu and catalog for these models wasn't enough: make_model_settings() only ever translated the generic reasoning_effort value into the wire field OpenAI's client actually reads (openai_reasoning_effort) inside the gpt-5-specific branch. For every other recognized OpenAI reasoning model the value sat inert in the settings dict and was silently dropped. Add a dedicated elif branch, gated on the same get_openai_reasoning_effort_choices() detection already used for the UI, that mirrors the GPT-5 branch's effort normalization/forwarding for these Chat-Completions-only models (no verbosity/summary, which are GPT-5-Responses-API-specific). Fixed-effort models (o1-mini, o1-preview, gpt-5-pro) correctly fall through untouched since the helper returns an empty list for them.
Second pre-PR review round found two real gaps in
get_openai_reasoning_effort_choices() and its callers:
1. Plain substring matching let short family tokens ("o1", "o3") false-
positive on unrelated custom-model aliases (e.g. "zoo1-claude"), which
could hijack a non-OpenAI model into the wrong make_model_settings()
branch and skip its real (e.g. Anthropic) handling entirely.
2. config.model_supports_setting() and model_factory.make_model_settings()
only checked the catalog key (model_name), not the underlying real
model id in model_config["name"] -- so an aliased extra_models.json
entry (a common, documented pattern) never got reasoning_effort
support at all, reintroducing this same PR's original bug for that
shape of config.
Fixes:
- get_openai_reasoning_effort_choices() now reuses the existing
boundary-aware _matches_model_tag() helper (DRY) instead of raw
substring/startswith matching, so "codex-gpt-5" still matches "gpt-5"
at a real delimiter boundary while "zoo1-claude" no longer falsely
matches "o1".
- config.model_supports_setting() and model_factory.make_model_settings()
both now fall back to model_config["name"] when model_name itself
isn't recognized, mirroring the fallback already used in
model_settings_menu._get_setting_choices().
- make_model_settings()'s new reasoning-effort branch is also gated on
the model NOT being Anthropic-typed, as defense in depth against any
future short-token collision.
New regression tests cover both the alias fallback and the collision
guard in config, model_factory, and the model_utils helper itself.
…back Third pre-PR review round found two remaining gaps from the round-2 fix: 1. The new elif in make_model_settings() only excluded Anthropic-typed models (via _is_anthropic_model), which is a negative exclusion -- an aliased gemini/zai/cerebras/openrouter/etc. catalog entry whose key or underlying model_config["name"] happens to contain a delimited OpenAI family token (e.g. a custom eval alias like "team-o1-eval") would still be hijacked into this branch and get the wrong wire format. Replaced with a positive whitelist, _OPENAI_COMPATIBLE_MODEL_TYPES (openai/chatgpt_oauth/ azure_foundry_openai/azure_openai/custom_openai*), mirroring how _is_anthropic_model positively identifies its own family. Every real catalog entry has an explicit "type" (ModelFactory.get_model() raises otherwise), so this is a safe positive check, not a functional regression. 2. This round's own alias-fallback work was inconsistent: the pre- existing "elif 'gpt-5' in model_name:" branch condition was left checking only the catalog key, so an aliased gpt-5 custom-endpoint entry (key without "gpt-5", real id in model_config["name"]) fell through into the new generic branch instead -- silently losing verbosity/summary/Responses-API/GPT-5.6 handling that config.model_supports_setting() (alias-aware since round 3) reports as supported. Extended that branch's own condition with the same model_config["name"] fallback. New regression tests cover both: a gemini-aliased "o1"-containing name proving the positive-type guard, and an aliased gpt-5 custom entry proving it still takes the full-featured GPT-5 branch (verbosity lands in extra_body) rather than the generic one.
Trim the essay-length inline commentary around OpenAI reasoning_effort detection down to the essentials. No behavior change (204 tests still pass).
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
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.
What
Fixes the
/model_settingsreasoning-effort picker so it exposes the full,real per-model
reasoning_effortscale that OpenAI documents — includingxhigh/maxwhere a model actually supports them — and makeso1/o3/o4-mini/codex-mini-latestsupportreasoning_effortat all(they had none before this change).
Why
xhigh/maxoptions were gated behind flags that no codepath ever set, so no model — not even ones that genuinely support
max— could ever show them.o1/o3/o4-mini/codex-mini-latesthad zeroreasoning_effortsupportin the UI, the catalog builder, or the API request itself, even though
OpenAI's docs list
low/medium/highfor all of them.model_factory.make_model_settings()never actually forwarded the chosenvalue to the OpenAI API for any non-GPT-5 model — the setting was visible
and selectable but silently dropped on the wire.
How
model_utils.get_openai_reasoning_effort_choices()as the singlesource of truth for which
reasoning_effortvalues each OpenAI modelfamily actually supports (per OpenAI's published docs/OpenAPI spec),
matched via the existing boundary-aware
_matches_model_tag()helper soshort family tokens (
o1,o3) can't false-positive on unrelatedaliases (e.g. a custom model called
zoo1-claude) while still matchinglegitimate embedded patterns like
codex-gpt-5.config.model_supports_setting()— so/model_settingsrecognizes thesetting dynamically, even for catalog entries that never declared it
explicitly (checking both the catalog key and the underlying
model_config["name"]alias).command_line/model_settings_menu.py— so the picker offers exactly thechoices that model supports.
command_line/add_model_menu.py— so newly added models get the rightcatalog defaults.
model_factory.make_model_settings()— a new branch that actuallyforwards the chosen
reasoning_efforttoopenai_reasoning_effortfornon-GPT-5 OpenAI reasoning models, normalizing legacy aliases
(
minimal→none,ultra→max). Gated on a positiveOpenAI-compatible-type whitelist (mirroring the existing
_is_anthropic_modelpattern) rather than a "not Anthropic" negative,so an aliased non-OpenAI catalog entry can't be misrouted into this
branch either.
catalog-name alias fallback, so an aliased
gpt-5*custom endpointstill gets full verbosity/summary/Responses-API handling instead of
falling into the plainer generic branch.
Testing
ruff check .andruff format --check .: clean.tests/test_config_full_coverage.py,tests/command_line/test_model_settings_menu_coverage.py, andtests/test_custom_model_settings.pycovering:gpt-5excludesxhigh/max,gpt-5.6includesmax, o-series getslow/medium/high,fixed-effort models like
o1-mini/gpt-5-proget no choices).model_supports_setting()detection without a catalog entry.reasoning_effort→openai_reasoning_effortonthe wire for o-series/codex-mini, with legacy alias normalization.
model_config["name"]) in both thesettings-menu and
make_model_settings()code paths.codex-gpt-5still matches thegpt-5bucket;an unrelated alias like
zoo1-claudeor a gemini-typed alias liketeam-o1-evaldoes not get hijacked into OpenAI-specific handling.Scope
code_puppy/model_utils.pycode_puppy/config.pycode_puppy/model_factory.pycode_puppy/command_line/model_settings_menu.pycode_puppy/command_line/add_model_menu.pytests/No dependency, lockfile, or CI/tooling config changes.