fix: clamp unsupported reasoning effort to model's highest supported tier - #71
Open
Michael J. Jabbour (michaeljabbour) wants to merge 1 commit into
Open
fix: clamp unsupported reasoning effort to model's highest supported tier#71Michael J. Jabbour (michaeljabbour) wants to merge 1 commit into
Michael J. Jabbour (michaeljabbour) wants to merge 1 commit into
Conversation
…tier Addresses microsoft-amplifier/amplifier-support#289: with a provider-level `effort` (or request.reasoning_effort) set to a value that passes GLOBAL validation (e.g. "max") but isn't declared in the ACTIVE model's ModelCapabilities.supported_efforts (e.g. claude-sonnet-5, which tops out at "xhigh"), the provider warned on every single request and omitted output_config.effort entirely -- letting the API apply its own server-side default effort instead of the model's actual ceiling. User's "max effort" intent was silently downgraded to "default" with no way to tell from the response. Root cause: two independent validation passes with no way to reconcile a mismatch. Config/request effort is checked against a global legal list (low/medium/high/xhigh/max); the per-model gate at request-build time only ever did a hard membership test against that model's OWN supported_efforts, with warn-and-omit as its only fallback for a legal-but-unsupported value. Fix: - Introduce EFFORT_ORDER, a canonical module-level ladder, and reuse it at the "effort" ConfigField's choices and the config-level `effort` validation (previously two more hand-maintained copies of the same five strings -- exactly the kind of drift that let this gap open up). - Add _clamp_effort_to_supported(): walk the ladder down from the requested rank and return the highest tier the model actually supports (e.g. "max" -> "xhigh" on claude-sonnet-5). - On a mismatch, clamp instead of omit. Genuinely unrecognized values (not on the ladder at all, e.g. a typo) still fall back to the original warn-and-omit behavior -- clamping only applies to values the provider understands but the active model doesn't support yet. - Demote the downgrade notice from a per-request WARNING to a one-time (per model+requested-effort pair) INFO log, tracked in a module-level seen-set mirroring the existing _warned_deprecated_models pattern. - Models that support the requested effort are unaffected: the happy-path pass-through branch is untouched (fable-5/opus-4.8 + "max" -> "max"). Tests (TDD -- written first, watched fail, then implemented the fix): - tests/test_reasoning_effort.py: new TestEffortClampToSupportedTier (6 cases: sonnet-5 max->xhigh headline clamp, fable-5/opus-4.8 max pass-through, no-output_config-support model still omits the key, unknown effort string omitted+warned not clamped, kwargs["effort"] precedence is also clamped) and TestEffortDowngradeLoggedOnce (2 cases: repeated identical requests log once; a different model/effort pair logs again). - tests/test_opus_47.py: test_opus_47_invalid_effort_omits_output_config asserted the OLD warn-and-omit contract for reasoning_effort="max" on Opus 4.7 (which lacks "max" but has "xhigh"). Replaced with test_opus_47_max_effort_clamps_to_xhigh (asserts the new clamp-to-xhigh behavior) and added test_opus_47_unknown_effort_string_omits_output_config to keep the genuinely-unrecognized-value coverage that test used to imply. Also: - pyproject.toml/uv.lock: added amplifier-core as a dev dependency. The test suite imports amplifier_core directly (ModuleCoordinator, ChatRequest, message models) but it was not declared anywhere in the dependency graph, so `uv sync && uv run pytest` could not collect the test suite from a clean clone. Dev-only; no runtime dependency change. - README.md: updated the "Reasoning Effort" notes to describe the clamp behavior instead of the old omit-with-warning behavior it documented. Pre-existing, unrelated: tests/test_tool_repair.py has 3 failing streaming tests (MockStreamManager __aiter__ mismatch) on a clean origin/main checkout, confirmed unaffected by this change (same 3 failures before and after).
Author
Cross-provider resume hardening set — complete PR indexThis PR is the effort-clamp fix in the set tracked on microsoft-amplifier/amplifier-support#208. Full set for reviewers:
Design rationale and deferred items: support#208 comment. |
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.
Addresses microsoft-amplifier/amplifier-support#289 — a follow-up to the
shipped config-level
effortknob (#61/#64). The issue may stay open forother aspects of the per-model-capability-lag family (#304, #299), so this
PR addresses the clamp specifically rather than closing the issue outright.
Root cause
config["effort"]/request.reasoning_effortis validated against aglobal legal list (
low/medium/high/xhigh/max) at config-resolutiontime, but is applied against the active model's
ModelCapabilities. supported_effortsat request-build time. Those two checks can legitimatelydisagree: provider config is intentionally model-agnostic (one config block
can serve many models via a routing matrix or mid-session model switch), so
"max" can pass the global check yet not exist in, say,
claude-sonnet-5'scapability tuple (
("low", "medium", "high", "xhigh")— no"max"untilOpus territory).
Before this fix, that mismatch hit a hard membership test with only one
fallback: warn and omit
output_config.effortentirely. That meant:WARNINGlog line on every single request routed to theunder-capable model (the field-reported symptom in #289).
output_config.effortat all, so it applied its ownserver-side default effort — silently downgrading the user's requested
"max" intent to whatever the API defaults to, with no signal in the
response that this happened.
What this PR does
EFFORT_ORDER, a single canonical module-level ladder(
low < medium < high < xhigh < max), and reuses it at theeffortConfigField'schoicesand the config-leveleffortvalidation —previously two more hand-maintained literal copies of the same five
strings, which is exactly the kind of drift that let this gap open up in
the first place.
_clamp_effort_to_supported(): on a mismatch, walks the ladder downfrom the requested rank and returns the highest tier the model actually
supports (e.g.
"max"→"xhigh"onclaude-sonnet-5), instead ofomitting
output_config.effort. A genuinely unrecognized value (not on theladder at all — a typo like
"ultra") still falls back to the originalwarn-and-omit behavior; clamping only applies to values the provider
understands but the active model doesn't support yet.
WARNINGto a one-timeINFOlog per(model, requested-effort)pair, tracked in a module-levelseen-set that mirrors the existing
_warned_deprecated_modelspatternalready used for deprecated-model warnings in this file.
the requested effort pass it through untouched (
claude-fable-5/claude-opus-4-8+"max"→"max").Tests (TDD)
Written first (watched red), then the fix was implemented to turn them
green. Extended
tests/test_reasoning_effort.pywith:TestEffortClampToSupportedTier(6 cases): theclaude-sonnet-5"max"→"xhigh"headline clamp;claude-fable-5/claude-opus-4-8"max"pass-through unchanged; a model withoutoutput_configsupport atall still never gains the key; a genuinely unknown effort string is
omitted+warned, not clamped;
kwargs["effort"]precedence is alsoclamped (not just
request.reasoning_effort).TestEffortDowngradeLoggedOnce(2 cases, viacaplog): two identicalrequests log the downgrade notice exactly once; a different
(model, effort)pair logs again.Updated an existing test to the new contract (per the task's request to
check for this):
tests/test_opus_47.py::test_opus_47_invalid_effort_omits_ output_configasserted the old warn-and-omit behavior forreasoning_effort="max"on Opus 4.7 (which lacks"max"but has"xhigh"). Replaced withtest_opus_47_max_effort_clamps_to_xhigh(asserts the new clamp-to-
"xhigh"behavior) and added a newtest_opus_47_unknown_effort_string_omits_output_configto keep thegenuinely-unrecognized-value coverage that the old test's docstring implied
but didn't actually test (it used
"max", a recognized value, not anunrecognized one).
Full suite: 535 passed, plus 3 pre-existing failures in
tests/test_tool_repair.py(streamingMockStreamManager.__aiter__mismatch) confirmed present on a clean
origin/maincheckout before thischange and unaffected by it — unrelated to this fix, left untouched.
Also in this PR
pyproject.toml/uv.lock: addedamplifier-coreas a dev-onlydependency. The test suite imports
amplifier_coredirectly(
ModuleCoordinator,ChatRequest, message models) but it wasn'tdeclared anywhere in the dependency graph, so
uv sync && uv run pytestcould not even collect the test suite from a clean clone. No runtime
dependency changed.
README.md: updated the "Reasoning Effort" notes, which described theold omit-with-warning behavior, to describe the new clamp behavior.
Verification (offline payload assertions, no live API)
Ran the actual mocked-SDK payload through the provider and inspected
params["output_config"]on the call captured by the mock:Relationship to the broader hardening set
This PR is part of the cross-provider resume hardening set tracked on
microsoft-amplifier/amplifier-support#208. Sibling PRs: thinking-block
sanitization for #207 in this same repo, and a producer fix for #206 in
amplifier-module-provider-chat-completions.