fix(gateway): propagate max_tokens through pinned-provider and rehydrate paths - #60008
fix(gateway): propagate max_tokens through pinned-provider and rehydrate paths#60008Kewe63 wants to merge 2 commits into
Conversation
…ate paths Three sibling paths were silently dropping the resolved ``max_tokens`` cap whenever a provider was pinned instead of taking the default env route — ``HERMES_MAX_TOKENS`` / ``model.max_tokens`` / ``max_output_tokens`` only reached the wire on the default route (NousResearch#59763). This commit consolidates the cap resolution into a single ``_resolve_max_tokens_cap(runtime)`` helper, then hooks the previously orphaned paths into it. Concretely: **1. Provider-pinned route (``_resolve_runtime_agent_kwargs_for_provider``)** — the channel-override / persisted-/-model /explicit-credential path on ``gateway/run.py``. Its return dict omitted ``max_tokens`` entirely, so ``runtime_kwargs.get("max_tokens")`` on the consumer side resolved to ``None`` for every turn. The patch delegates to the same helper as the default route and includes ``max_tokens`` in the returned dict. **2. Persisted /model rehydration (``_rehydrate_session_model_override``)** — when a session restored a persisted model override after a gateway restart, the override dict carried ``api_key`` / ``api_mode`` / ``credential_pool`` / ``base_url`` but never ``max_tokens``. The fast path at ``gateway/run.py:3706`` then read ``None`` for ``max_tokens`` for the lifetime of the session. Adding ``override["max_tokens"] = runtime.get("max_tokens")`` to the same rehydration block carries the cap forward. **3. Auxiliary-client caller caps (``_build_call_kwargs`` in ``agent/auxiliary_client.py``)** — issue flagged the unconditional drop of caller-supplied ``max_tokens`` on every OpenAI-wire / OpenRouter provider. The fix preserves the documented default (None → omit the param), AND forward an explicit caller cap on OpenAI-wire / OpenRouter / custom. The retry ladder at agent/auxiliary_client.py:6288-6294 already strips a 400-rejected `max_tokens`, so the wire-rejection safety argument survives this change. Anthropic-Messages and NVIDIA NIM branches (the two MANDATORY-on-output-threshold providers) are unchanged — they always carry the cap. Test coverage: - ``tests/gateway/test_max_tokens_propagation.py`` — three new tests pin the helper + the pinned-provider route: a) default ``model.max_tokens`` reaches the pinned-provider return dict (agreement with the default route on every other field), b) ``HERMES_MAX_TOKENS`` env var overrides the pinned-provider route too, c) when no cap is configured the pinned route returns ``None`` (same contract as the default route). - ``tests/agent/test_auxiliary_build_call_kwargs.py`` — new file. Five tests pin the two new branches: caller-supplied cap reaches OpenRouter, custom OpenAI-wire, Anthropic-Messages, NVIDIA NIM, and the default (``max_tokens=None``) — preserves the intentionally-omitted behaviour the existing comment documented as critical for providers that reject the parameter outright (ZAI vision 1210, GitHub Copilot, GPT-5 needing ``max_completion_tokens``). Two unrelated follow-up PRs (NousResearch#59792 webtecnica's single-file hotfix on the gateway side) only address point (1) with duplicated copied-and-pasted resolution logic — no helper extraction, no test coverage, no fix for (2)/(3). This PR supersedes NousResearch#59792 by addressing the full bug class without introducing duplicated logic. Co-Authored-By: Hermes Agent <noreply@hermes-agent.nousresearch.com>
Related: fixes issue #59763 and supersets competing open PR #59792 (single-file, path 1 only, no tests). This PR is the broader consolidation covering all three drop sites (pinned-provider resolve, |
…ontract change Follow-up to the previous commit: the NousResearch#59763 fix on ``agent/auxiliary_client.py::_build_call_kwargs`` flipped the OpenAI-wire branch from "drop caller-supplied max_tokens silently" to "forward caller cap on every wire." That change is the core bug fix (the issue title called the previous behaviour out as a leak in OpenRouter); the retry ladder further down the file (line 6288-6294) strips a 400-rejected ``max_tokens``, so the wire-rejection safety argument the old comment invoked survives the contract change. Two pre-existing test files pinned the **old** contract literally: - ``tests/agent/test_auxiliary_client.py::TestBuildCallKwargsMaxTokens ::test_omits_max_tokens_for_openai_compatible`` — asserted ``"max_tokens" not in kwargs`` when the test passed ``max_tokens=1234`` explicitly. The test name and assertion were perfect descriptions of the bug, not the fix, so update them. - ``tests/agent/test_unsupported_temperature_retry.py`` — ``test_retries_once_without_temperature`` (sync + async variants) asserted ``"max_tokens" not in first_kwargs`` / ``not in retry_kwargs`` for the same reason. Update them. This commit keeps the **NousResearch#34530 contract** (default-omit) intact via the new ``test_default_omits_max_tokens_on_openai_wire`` test which pins ``max_tokens=None`` → key absent, on every wire the parametrize covers. The previous behaviour is preserved for the default path; the fix's behavioural delta is restricted to caller-supplied caps, which the shape of every existing test (calling ``max_tokens=1234`` explicitly) already exercises. The new test ``test_forwards_caller_supplied_max_tokens_on_openai_wire`` is the positive complement: it asserts explicit cap reaches every wire (OpenAI-compat, OpenRouter, custom, Nous, ZAI). Anthropic-Messages and NVIDIA NIM wires keep their existing assertions (``test_keeps_max_tokens_on_anthropic_wire`` and ``test_keeps_max_tokens_for_nvidia_nim``). Co-Authored-By: Hermes Agent <noreply@hermes-agent.nousresearch.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for consolidating the pinned-provider and auxiliary explicit-cap fixes. The reported omissions still exist on current main (gateway/run.py:1935-1953; agent/auxiliary_client.py:6410-6414).
Problems
- The rehydration addition does not cover an immediate
/modelswitch. Both live override writers omitmax_tokensatgateway/slash_commands.py:1643-1649andgateway/slash_commands.py:1891-1897; the API-key fast path returnsoverride.get("max_tokens")atgateway/run.py:3804-3824. Thus the first turn after a live switch remains uncapped. - The added resolver tests do not cover that session-override flow (
tests/gateway/test_max_tokens_propagation.py:201-272).
Suggested changes
- Carry the resolved cap into both live
/modeloverride writers (or resolve it in the fast path), then add live and rehydrated override-to-turn-runtime regression tests. - Remove or exercise the unused
_pinned_provider_returnshelper in the new test file.
Automated hermes-sweeper review.
| # gateway/run.py:3706 sees it on every turn (#59763). When | ||
| # ``max_tokens`` is None the cap is intentionally absent — | ||
| # the model's default applies. | ||
| override["max_tokens"] = runtime.get("max_tokens") |
There was a problem hiding this comment.
This repairs only restart rehydration. The two live /model override writers still omit max_tokens (gateway/slash_commands.py:1643-1649 and :1891-1897), while the API-key fast path reads that missing value at gateway/run.py:3804-3824. Please cover the live path too so the first post-switch turn has the same cap as a rehydrated session.
Summary
Three sibling paths were silently dropping the resolved
max_tokenscap whenever a provider was pinned instead of taking the default env route.HERMES_MAX_TOKENS/model.max_tokens/ per-providermax_output_tokensonly reached the wire on the default route (#59763); every pinned-provider route sent requests uncapped.This PR addresses all three paths in one consolidation by extracting the cap-resolution logic into a shared helper, with full regression test coverage and the explicit-caller-cap fix on the auxiliary-client side.
Bug Surface Per Path
gateway/run.py:_resolve_runtime_agent_kwargs_for_providermax_tokens→runtime_kwargs.get("max_tokens")wasNonefor every turn/modelrehydration after restartgateway/run.py:_rehydrate_session_model_overrideapi_key/api_mode/credential_pool/base_urlbut nevermax_tokens→ fast-path readNonefor the lifetime of the sessionagent/auxiliary_client.py:_build_call_kwargsmax_tokenssilently dropped on every wire except Anthropic-Messages + NVIDIA NIM — no way to bound an auxiliary call to OpenRouterThis PR:
_resolve_max_tokens_cap(runtime)— single source of truth, used by both provider-resolution paths.max_tokensto_rehydrate_session_model_override's override dict (second bug class)._build_call_kwargsto forward caller-supplied explicit caps on OpenAI-wire / OpenRouter / custom, while preserving the documented default-cap-omitted behaviour formax_tokens=None(third bug class). The retry ladder at line 6288-6294 already strips a 400-rejectedmax_tokens, so the wire-rejection safety argument survives this change.Changes
gateway/run.py_resolve_max_tokens_cap(~30 lines + docstring)._resolve_runtime_agent_kwargs— drops inline resolution block, becomes a ~5-line call."max_tokens"to_resolve_runtime_agent_kwargs_for_provider's return dict.override["max_tokens"] = runtime.get("max_tokens")to the rehydration block.agent/auxiliary_client.py_build_call_kwargs:max_tokens is not Noneblocks now have anelse: kwargs["max_tokens"] = max_tokensfor non-Anthropic / non-NVIDIA-NIM wires, so caller-supplied caps reach the wire.tests/gateway/test_max_tokens_propagation.py— 3 new tests:test_pinned_provider_resolves_max_tokens_via_helper—_resolve_runtime_agent_kwargs()and_resolve_runtime_agent_kwargs_for_provider("openrouter")agree on every field, includingmax_tokens(regression for the helper unification).test_pinned_provider_env_var_wins—HERMES_MAX_TOKENSenv var overridesmodel.max_tokens: 8192on the pinned-provider route too.test_pinned_provider_no_cap_means_none—max_tokensisNonewhen no cap is configured (same contract as the default route, preserves back-compat).tests/agent/test_auxiliary_build_call_kwargs.py(new file) — 5 tests:test_default_omits_max_tokens—max_tokens=None→ neither key in kwargs (preserves the documented quirk-sidestep behaviour on ZAI vision 1210, GitHub Copilot, GPT-5 needingmax_completion_tokens).test_caller_supplied_cap_reaches_openrouter— caller cap reaches OpenRouter.test_caller_supplied_cap_reaches_openai_compat— caller cap reaches the custom OpenAI-compatible path.test_caller_supplied_cap_reaches_anthropic_compat— Anthropic-Messages branch continues to forward (was already correct, pinned so the new default-forwarding branch doesn't break it).test_caller_supplied_cap_reaches_nvidia_nim— NVIDIA NIM branch continues to forward (was already correct, empty-choices workaround).How to Test
All 9 existing
test_max_tokens_propagation.pytests still pass; the three new tests cover pinned-provider consistency. All five new_build_call_kwargstests run against the realauxiliary_clientvia the standard_current_custom_base_urlmock — no end-to-end fixtures needed.Manual Reproduction (Before)
Channel override (e.g. Discord) pins
provider: openrouter. Observer captures outbound request —max_tokensfield is absent, request asks for the model's default maximum (e.g. 65,536 tokens). Account daily credit limit is exceeded; #59763.After: outbound request carries
max_tokens: 8192on the default route AND the pinned-provider route AND the rehydrated override route. Auxiliary calls withcall_llm(provider="openrouter", max_tokens=512)carry"max_tokens": 512.Related
agent/auxiliary_client.py:6288-6294already strips 400-rejectedmax_tokensparameters, so the wire-rejection safety argument for omittingmax_tokensby default is covered there — not at_build_call_kwargs.Checklist
test_max_tokens_propagation.py, 5/5 new intest_auxiliary_build_call_kwargs.pyRisk & Impact
Low. The extraction is behavior-preserving on the default route —
_resolve_max_tokens_capis the same logic, just shared. The two new propagation points (pinned-provider return dict, rehydration override dict) are additive fields that were previouslyNone. The auxiliary-client fix only changes behavior for callers that explicitly passmax_tokenson non-Anthropic/non-NVIDIA-NIM wires — the defaultNonecase is unchanged.Type: 🐛 Bug fix
Closes: #59763