Skip to content

fix(cli): honor runtime max_output_tokens fallback for custom providers - #40802

Open
Dusk1e wants to merge 2 commits into
NousResearch:mainfrom
Dusk1e:fix/cli-max-output-tokens-parity
Open

Dusk1e wants to merge 2 commits into
NousResearch:mainfrom
Dusk1e:fix/cli-max-output-tokens-parity

Conversation

@Dusk1e

@Dusk1e Dusk1e commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

What

Per-provider max_output_tokens (set on a providers: / custom_providers: entry)
was honored by the gateway but silently dropped on the interactive CLI and oneshot
(hermes chat / hermes -q). The same config therefore capped model output
differently depending on the surface.

Why

14275d7 (fix(gateway): honor per-provider max_output_tokens in max_tokens chain),
extended by 1c909e7, established the precedence chain
HERMES_MAX_TOKENS > model.max_tokens > per-provider max_output_tokens
for gateway + CLI. In practice only the gateway read runtime["max_output_tokens"]:
the interactive CLI set max_tokens from env/global config only, and oneshot didn't
forward max_tokens at all.

Change

  • Add a shared resolve_effective_max_tokens() resolver in
    hermes_cli/runtime_provider.py as the single source of truth for the precedence.
  • Use it in the gateway (behavior-preserving refactor), the interactive CLI
    (_ensure_runtime_credentials), and oneshot (_run_agent).

Global model.max_tokens and HERMES_MAX_TOKENS keep priority; the per-provider
cap only fills in when no global cap is configured.

Tests

Added:

  • tests/hermes_cli/test_max_tokens_parity.py — resolver precedence
    (env / global / provider cap / none / invalid) + oneshot end-to-end
    (AIAgent.max_tokens == provider cap).
  • tests/cli/test_cli_provider_resolution.py — CLI applies the provider cap;
    global still wins.

Results:

tests/hermes_cli/test_max_tokens_parity.py ............ 3 passed
tests/gateway/test_max_tokens_propagation.py .......... 6 passed
tests/cli/test_cli_provider_resolution.py ............. 22 passed
tests/hermes_cli/test_runtime_provider_resolution.py .. 127 passed
tests/hermes_cli/test_tui_resume_flow.py .............. 45 passed

No regressions in the runtime-resolution suites.

Per-provider max_output_tokens (providers/custom_providers) was applied by the gateway but silently dropped on the interactive CLI and oneshot (hermes chat / hermes -q), so the same config capped output differently across surfaces.

Extract a shared resolve_effective_max_tokens() resolver enforcing the documented precedence (HERMES_MAX_TOKENS > model.max_tokens > per-provider max_output_tokens) and apply it in the gateway, the interactive CLI (_ensure_runtime_credentials), and oneshot (_run_agent).

Ref: 14275d7 (fix(gateway): honor per-provider max_output_tokens in max_tokens chain), which established the precedence chain for gateway + CLI.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery labels Jun 6, 2026
Slices 1 and 5 hit pre-existing flaky tests unrelated to this change: tests/tools/test_browser_hardening.py (agent-browser install subprocess timed out >30s) and tests/run_agent/test_run_agent.py::TestConcurrentToolExecution::test_concurrent_handles_tool_error (concurrency race; passes locally). No source or test changes.
@liuhao1024

Copy link
Copy Markdown
Contributor

Positive verification

Reviewed the full diff — this is a clean, well-structured fix.

What it does: Extracts resolve_effective_max_tokens() as a single source of truth for output-token cap precedence (HERMES_MAX_TOKENS env > model.max_tokens > per-provider max_output_tokens > None) and wires it into all three surfaces (interactive CLI, gateway, oneshot) that previously had divergent logic.

Why the old behavior was a bug: The oneshot path (hermes -q) silently dropped a custom provider's max_output_tokens cap entirely — the gateway honored it but oneshot didn't, so the same config produced different output limits depending on how you invoked the agent.

Correctness checks:

  • The not isinstance(configured, bool) guard on model.max_tokens prevents True/False from leaking through as token counts — good edge case handling.
  • provider_cap > 0 rejects non-positive values.
  • The model_cfg parameter with None default allows callers to inject an already-loaded config (avoids redundant _get_model_config() calls in the gateway path).
  • The test for HERMES_MAX_TOKENS="not-an-int" confirms graceful fallthrough on unparseable env values.

Test coverage: Five tests across two files cover precedence, env override, bool rejection, oneshot forwarding, and interactive CLI. The oneshot test uses a _FakeAgent to capture kwargs — clean approach that avoids importing the full agent stack.

No issues found. The extraction is minimal, the precedence logic is correct, and the tests cover the important edge cases.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating a real cross-surface configuration bug. The premise still holds on current main: hermes_cli/runtime_provider.py:592-603 preserves provider caps, while hermes_cli/oneshot.py:375-417 resolves that runtime but does not pass max_tokens to AIAgent.

Problems

  • The interactive CLI edit is stale. _ensure_runtime_credentials moved from cli.py to hermes_cli/cli_agent_setup_mixin.py:25 in 094aa85c3; the PR patch no longer applies to cli.py.
  • The same loss remains for /background: hermes_cli/cli_agent_setup_mixin.py:184-192 omits max_tokens from the turn runtime, but hermes_cli/cli_commands_mixin.py:1644 consumes that key for the background agent.
  • Gateway provider-override runtime construction also omits the cap at gateway/run.py:1907-1925.

Suggested changes

  • Move the CLI resolver call into CLIAgentSetupMixin._ensure_runtime_credentials and add the cap to the CLI turn runtime used by /background.
  • Add regression coverage for the background path and provider-override runtime path alongside the existing CLI/oneshot cases.

Automated hermes-sweeper review.

Comment thread cli.py

# Honor a per-provider output cap (providers/custom_providers
# max_output_tokens) the same way the gateway does, so the same config
# caps output identically across surfaces. HERMES_MAX_TOKENS and the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main moved _ensure_runtime_credentials into hermes_cli/cli_agent_setup_mixin.py in 094aa85, so this cli.py hunk no longer applies. Please transplant this change to the mixin method during salvage.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 14, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants